12 ms·
Years ago (2010 iirc), I reported to the Postgres list, with a patch, that (one of) the reason Postgres “didn’t work on NFS” was because it couldn’t deal with s
by georgebarnett 8y ago
Years ago (2010 iirc), I reported to the Postgres list, with a patch, that (one of) the reason Postgres “didn’t work on NFS” was because it couldn’t deal with short writes. I got told the usual “you’re holding it wrong” instead of an acknowledgement of PG not sticking to the spec.
I patched my own systems (wrap the calls in a loop as per standard practice) and then proceeded to run literally hundreds of thousands of PostgreSQL instances on NFS for many more years with no problems.
The patch was eventually integrated it seems but I never found out why because I lost interest in trying to work with the community after that experience.
- anarazel 8y agoThe relevant thread: https://www.postgresql.org/message-id/46ED7555-9383-45D2-A5C0-6373BE2FDCB6%40atlassian.com https://www.postgresql.org/message-id/46ED7555-9383-45D2-A5C... There's literally one person saying that NFS is crapshot anyway, and several +1s for retries. And the former was accompanied with several questions about the concrete implementation.
- georgebarnett 8y agoOne the messages told me to go lobby NFS vendors. Many others were as you note bikeshedding the commit. Only one, if I recall, questioned why it was a controversial patch at all. If you check the patch that was committed eventually, IIRC it’s identical to what I proposed. As I said, I don’t look back particularly favourably on my interactions with the community.
- pgaddict 8y agoWhich of the messages on the thread do you consider as bikeshedding? I personally don't see any - that's not to say I agree with everything said on the thread, but overall it seems like a reasonable discussion. IMHO it's a bit strange to assume everyone will agree with your patch from the very beginning. People may lack proper understanding of the issue, or they may see it from a different angle, and so on. Convincing others that your solution is the right one is an important part of getting patch done. I don't see anything wrong with that.
- javajosh 8y agoLooking at your submission, it was very good: a clean way to reproduce, reference to the specification that tells users of a syscall how to use it. I'm perplexed that you got so much pushback on this when it seemed pretty straight-forward. I'd guess that this was your first patch to postgres and the default reaction is to be defensive and decline the patch. Thinking about the social structure for a minute, I honestly think you might have done better to leave out some of the content of your patch submission, let people push back on the more obvious stuff, and have ready answers. There's a phenomena where curators feel they must push back, and they feel weird when there's nothing to criticize - you can get around this by giving them something to criticize, with a ready-at-hand response! Sorry this happened.
- noir_lord 8y agoI know that as ‘Take out the duck’. http://mamchenkov.net/wordpress/2012/07/23/a-duck/ http://mamchenkov.net/wordpress/2012/07/23/a-duck/
- sam_goody 8y agoSomewhat OT, but that is generally referred to as a duck[1], and it _definitely_ has its uses. > The artist working on the queen animations for Battle Chess... did the animations for the queen the way that he felt would be best, with one addition: he gave the queen a pet duck. He animated this duck through all of the queen’s animations, had it flapping around the corners. He also took great care to make sure that it never overlapped the “actual” animation. > As expected, he was asked to remove the duck, which he did, without altering the real Queen animation. [1]: https://softwareengineering.stackexchange.com/a/122148 https://softwareengineering.stackexchange.com/a/122148
- javajosh 8y agoHaha cool, I've been around a while but I've never heard of this. I wonder if this concept can be combined with confessional debugging, aka rubber-ducking? :)
- acqq 8y agoIt’s sill wrong to suggest to every contributor to put “the duck” in his contribution, when the fact is then that it’s the people who handle the contribution who are the problem.
- jeffdavis 8y agoI feel this is a little unfair to the community and I encourage others to read the thread before forming a negative opinion. It seems like a fairly good example of an engineering discussion. Even if the patch is correct and conforming and solves a problem, that doesn't mean there is no use for further discussion, and the surrounding discussion does seem to have merit. Things like: * Are there closely-related problems still left unsolved (e.g. retrying reads)? * Is something not configured according to best practices known by other community members? * Expressing an opinion that the use case is dangerous, so that nobody (including other people reading the thread in the future) will take it as an endorsement for running postgres on NFS. * Some legitimate-sounding questions about the specifics of the patch (around zero returns, what writeAll handles versus its callers, etc.). * At least one person agreed with you that it's a reasonable thing to do. I don't think HN is really the right place to assess the technical merits of a postgres patch, but the discussion itself seems well within reasonable bounds.
- nmadden 8y agoThis reply from Tom Lane reminded me of the PC losering issue from the “worse is better” essay [1]. > 2. What is the rationale for supposing that a retry a nanosecond later will help? If it will help, why didn't the kernel just do that? [1] https://www.jwz.org/doc/worse-is-better.html https://www.jwz.org/doc/worse-is-better.html
- networkimprov 8y agoBy "short writes" you mean partial writes?
- georgebarnett 8y agoYes, thank you.
- DannyBee 8y agoThis saddens me. Looking at this thread - the patch is obviously right. It doesn't matter about NFS, Linux, or any host of random crap people brought up. (I unfortunately suspect if you hadn't mentioned any of that, and just said "write doesn't guarantee full writes, this handles that" it may have gone better). The documentation (literally on every system) is quite clear: "The return value is the number of bytes actually written. This may be size, but can always be smaller. Your program should always call write in a loop, iterating until all the data is written." (see https://www.gnu.org/software/libc/manual/html_node/I_002fO-Primitives.html#I_002fO-Primitives https://www.gnu.org/software/libc/manual/html_node/I_002fO-P...) Literally all documentation you can find about write on all systems that implement it clearly mentions it doesn't have to write all the bytes you ask it to. Instead of saying "yeah, that's right", people go off in every random direction, with people arguing about your NFS mount options, what things can cause write to do that and whether they are possible[1] or they should care about them. Sigh. [1] spoiler alert: it doesn't matter. The API allows for this, you need to handle it.
- anarazel 8y agoI'm a bit confused by this perception of the thread. There was one person doubtful the patch is the right approach (Tom), but still offered concrete review comments (of actual issues that'd at the very least need to be documented in code comments). Others agreed that it's something that needs to be done. > Instead of saying "yeah, that's right", people go off in every random direction, with people arguing about your NFS mount options, what things can cause write to do that and whether they are possible[1] or they should care about them. Given that changing the mount option solved the immediate problem for the OP, I fail to see how it's random. > [1] spoiler alert: it doesn't matter. The API allows for this, you need to handle it. Sure, it's not like the return value was ignored before though. I agree retrying is the better response, but erroring out is a form of handling (and will also lead to retries in several places, just with a loop that's not tightly around the write()).
- lugg 8y agoYou had me until you tried to justify bubbling the error instead of handling it at its source while possible.
- _pmf_ 8y agoI think you patch was too perfect. Always leave some small thing for the gatekeepers to latch on, then be all "oh, yes, totally overlooked this, thank you, I'll fix it ASAP".
- beobab 8y agoI think one of the Dilbert books had this concept. Make one bullet point ridiculous so your boss can have some input and ask you to remove the “Be involved in a land war in Asia” step in your ten point plan.
- icelancer 8y agoThis is the first I'm hearing of the "take out the duck" concept. I've run software development teams for quite some time now and I have zero problem taking patches that are perfect without nitpicking them. What the hell is wrong with people?
- hueving 8y agoIt's something that emerges in big open source projects. Not really related to software development teams that all answer to the same boss.
- rleigh 8y agoIt's by far not limited to only open source projects. There are many situations where some fault, no matter how small or irrelevant, has to be found in order to sate the ego of the person doing the review or consideration of a problem or piece of work.
- vidarh 8y agoI don't think it's limited to open source project either. But open source projects are unusual in that often even when commit rights etc. are controlled, being able to opine on submitted patches is open, and a way of building social status within the project. And while that often works well, it also does attract people who may even mean well, but will be overly critical because it's their way of feeling they're contributing. In most corporate projects, odds are you're dealing with a much smaller pool of reviewers.
- lazyjones 8y agoI have found the Postgres community extremely helpful and friendly. Perhaps this part of the interaction explains why they dropped the issue for a while? https://www.postgresql.org/message-id/223BF4DE-B274-4428-8325-7C8E8250073B%40atlassian.com https://www.postgresql.org/message-id/223BF4DE-B274-4428-832...
- infinity0 8y agoDo you have some examples where you disagreed with them or pointed out their mistakes? Obviously everyone is "extremely helpful and friendly" when you agree with them or don't point out their mistakes. The "asshole reviewer" is definitely a thing.
- pgaddict 8y agoI don't think I've encountered "asshole reviewer" in postgres community, but maybe my "pain threshold" is just higher than yours. More often than not it's a valid disagreement about technical matters.
- rleigh 8y agoReview in the PostgreSQL community does tend to be on the "harsh" side. I've only submitted one patch (\pset linestyle unicode for psql), and with a few rounds of review and revision it made it in. Overall I found this process nitpicky but productive. However, there is a point at which the harshly critical review can be detrimental, and from the other comments mentioned here, it sounds like this has been the case in the past. With regard to the semantics of write(2) and fwrite(3) these are clearly documented in SUS and other standards, and the "valid disagreement" in this case may have been very counter-productive if it killed off proper review and acceptance of a genuine bug. There are, of course, problems with fsync(2) which have had widespread discussion for years.
- pgaddict 8y ago> Review in the PostgreSQL community does tend to be on the "harsh" side. Can you share an example of a review that you consider harsh? (feel free to share privately, I'm simply interested in what you consider harsh) I admit some of the reviews may be a bit more direct, particularly between senior hackers who know each other, and from time to time there are arguments. It's not just rainbows and unicorns all the time, but in general I find the discussion extremely civilized (particularly for submissions from new contributors). But maybe that's just survivor bias, and the experience is much worse for others ... > I've only submitted one patch (\pset linestyle unicode for psql), and with a few rounds of review and revision it made it in. Overall I found this process nitpicky but productive. However, there is a point at which the harshly critical review can be detrimental, and from the other comments mentioned here, it sounds like this has been the case in the past. Oh, 2009 - good old days ;-) Thanks for the patch, BTW. There's a fine line between nitpicking and attention to detail. We do want to accept patches, but OTOH we don't want to make the code worse (not just by introducing bugs, even code style matters too). At some point the committer may decide the patch is in "good enough" shape and polish it before the commit, but most of that should happen during the review.
- sqldba 8y agoTry committing to a Microsoft open source project or working with a lot of ex-Microsoft employees. You’ll want to kill yourself.
- faical 8y ago<off-topic>I committed to a Microsoft open source project and worked with (ex-)Microsoft people. The experience was great :)</off-topic>
- GordonS 8y agoI've had this experience when I've tried to contribute to ASP.NET Core and .NET Core - in the case of ASP.NET Core, they weren't in the slightest bit interested in fixing something that's obviously broken. In the case of .NET Core, it was made clear that adding something new to the encryption library was going to take a long time, possibly years, even though there was demand for it. I've given up on both :/
- DoofusOfDeath 8y agoIt sounds like you have an interesting anecdote to share. If you can just give us the anecdote rather than making a vague, sweeping critique of MS OSS projects, you might avoid the downvotes.
- garganzol 8y agoThat's an excellent patch. Partial writes (and reads) is a known behavior which is often overlooked. It rarely (if ever) occurs on local file systems, and even the network ones manifest it only from time to time, usually when there is some kind of congestion/segmentation is going on. It's a pity a lot developers out there do not have an ingrained awareness about this.
- jeffdavis 8y agoEveryone in that thread knows what a partial write is. That's why the patch author saw this error message: > 2011-07-31 22:13:35 EST postgres postgres [local] LOG: connection authorized: user=postgres database=postgres > 2011-07-31 22:13:35 EST ERROR: could not write block 1 of relation global/2671: wrote only 4096 of 8192 bytes > 2011-07-31 22:13:35 EST HINT: Check free disk space. > 2011-07-31 22:13:35 EST CONTEXT: writing block 1 of relation global/2671 > 2011-07-31 22:13:35 EST [unknown] [unknown] LOG: connection received: host=[local] If they didn't know what a partial write was, they wouldn't have identified it specifically as an error case with its own informative error message. The proposed patch retries rather than throwing an error.
- agumonkey 8y agothis is why we have facebook
- jarym 8y agoActually as great as Postgres is and as generally approachable the community is - my experience was the same a few times and I read it on the mailing list happening to others: Someone comes along with a patch or idea. Bunch of big Postgres people come knock it and it dies right there. Happened to me when I suggested a more multi-tenant approach back around 2010 and today we have Citus. I was told that (paraphrased) no users were asking for that sort of thing. I see it kind of happening with the Foreign Key Array patch that the author asked for help to rebase and no one bothered to reply. Someone suggested replacing the Postmaster with a threaded approach so it could scale better (and showed benchmarks of their implementation handling more connections). Community response was there were already 3rd party connection pools that do the job. An outsider looking in considers this nuts - most people would not run additional components if they did not need to! Another example: NAMEDATALEN restricts every identifier to 63 bytes - a limit that causes frequent issues (more so now that we have partitioning) and also a problem for multi-lingual table names. It’s been proposed to increase this limit a few times. Every time the answer has been: abbreviate your table names or recompile your own version of Postgres. Could name a few other examples too I’ve noticed over the years and just sighed at. I don’t expect every idea to be accepted or even welcomed - but there is that sense of bias against change.
- anarazel 8y agoYea, that's definitely something that happens. Partially due to some conservatism, partially due to having way more patches than review bandwidth, ... > Another example: NAMEDATALEN restricts every identifier to 63 bytes - a limit that causes frequent issues (more so now that we have partitioning) and also a problem for multi-lingual table names. It’s been proposed to increase this limit a few times. Every time the answer has been: abbreviate your table names or recompile your own version of Postgres. I think if it were easy, it'd immediately be fixed, but there's a fair bit of complexity in fixing it nicely. For partially historical and partially good reasons object names are allocated with NAMEDATALEN space, even for shorter names. So just increasing it would further waste memory... We're going to have to fix this properly one of these days.
- jarym 8y ago
- cheez 8y agoIn any large organizations, fiefdoms are bound to develop. If you don't have principles for dealing with it, what ends up happening is that new ideas will be crowded out and claimed by one of the leaders of the fiefdom. The Linux model has formalized this process. I think more large open source projects should use this model.
- draw_down 8y agoYeah, that’s my experience with most groups of people who work with technology. Deny deny deny, default everyone is wrong but me. It’s why I never even tried to be part of some active open source community or god forbid, be the maintainer of something lots of people use. I don’t understand how anyone intends to learn or grow with that mindset. Some are more polite about it than others but it’s still there. So many pretend they work in a meritocracy. But when someone from outside their clique tries to follow their rules, well, it’s always something.
- Gravityloss 8y agoYou can't tell people anything. http://habitatchronicles.com/2004/04/you-cant-tell-people-anything/ http://habitatchronicles.com/2004/04/you-cant-tell-people-an...
- deleted 8y ago[deleted]
- quotemstr 8y agoThe same pattern emerges in big commercial projects too. Why do you think it took 20 years to fix console resizing in Windows?
- mixmastamyk 8y agoBallmer and his ilk wouldn't allow it.