Thread: a slightly stale comment
While mucking around in src/backend/utils/time/tqual.c today, I noticed the following comment attached to HeapTupleSatisfiesNow: * mao says 17 march 1993: the tests in this routine are correct;* if you think they're not, you're wrong, andyou should think* about it again. i know, it happened to me. we don't need to* check commit time against thestart time of this transaction* because 2ph locking protects us from doing the wrong thing.* if you mess aroundhere, you'll break serializability. the only* problem with this code is that it does the wrong thing for system* catalog updates, because the catalogs aren't subject to 2ph, so* the serializability guarantees we providedon't extend to xacts* that do catalog accesses. this is unfortunate, but not critical. Much as I hate to disturb a comment just before its 19th birthday, the bit about two-phase locking and serializability hasn't been correct since around 1999 (when MVCC was added). :-) Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/
On Wed, Mar 7, 2012 at 2:05 AM, Dan Ports <drkp@csail.mit.edu> wrote: > While mucking around in src/backend/utils/time/tqual.c today, I noticed > the following comment attached to HeapTupleSatisfiesNow: > > * mao says 17 march 1993: the tests in this routine are correct; > * if you think they're not, you're wrong, and you should think > * about it again. i know, it happened to me. we don't need to > * check commit time against the start time of this transaction > * because 2ph locking protects us from doing the wrong thing. > * if you mess around here, you'll break serializability. the only > * problem with this code is that it does the wrong thing for system > * catalog updates, because the catalogs aren't subject to 2ph, so > * the serializability guarantees we provide don't extend to xacts > * that do catalog accesses. this is unfortunate, but not critical. > > Much as I hate to disturb a comment just before its 19th birthday, the > bit about two-phase locking and serializability hasn't been correct > since around 1999 (when MVCC was added). :-) There is much wisdom there and much wisdom in leaving ancient warnings as we find them. Are these the words you object to? "we don't need to > * check commit time against the start time of this transaction > * because 2ph locking protects us from doing the wrong thing." -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Simon Riggs <simon@2ndQuadrant.com> wrote: > Dan Ports <drkp@csail.mit.edu> wrote: >> While mucking around in src/backend/utils/time/tqual.c today, I >> noticed the following comment attached to HeapTupleSatisfiesNow: >> [a comment explaining that if you think the code needs to be >> changed, you are wrong, because 2 phase locking will prevent >> any problems] >> Much as I hate to disturb a comment just before its 19th >> birthday, the bit about two-phase locking and serializability >> hasn't been correct since around 1999 (when MVCC was added). :-) > > There is much wisdom there and much wisdom in leaving ancient > warnings as we find them. What? That comment makes multiple references to 2 phase locking, which was supplanted by MVCC, and the tests in that routine have been rather heavily modified since that warning was added -- so apparently the tests have become wrong several times since then. If the point is that great care should be taken in modifying this section of code, and you think it is significantly more fragile than other code, then a comment explaining why that is currently true would be a good thing; but I don't see any value in littering the code with comments about why it might have been true before MVCC was implemented. > Are these the words you object to? > >> * we don't need to check commit time against the start time >> * of this transaction because 2ph locking protects us from >> * doing the wrong thing. Well, certainly any reference to 2 phase locking would be wrong. What part of it do you find to be accurate or helpful? -Kevin
On Wed, Mar 7, 2012 at 4:09 PM, Kevin Grittner <Kevin.Grittner@wicourts.gov> wrote: > What part of it do you find to be accurate or helpful? I think its funny and scary, as well as being of historical interest. But please suggest a new wording so we can discuss it. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Mar 7, 2012 at 12:17 PM, Simon Riggs <simon@2ndquadrant.com> wrote: > On Wed, Mar 7, 2012 at 4:09 PM, Kevin Grittner > <Kevin.Grittner@wicourts.gov> wrote: > >> What part of it do you find to be accurate or helpful? > > I think its funny and scary, as well as being of historical interest. > > But please suggest a new wording so we can discuss it. I think we should just remove it. It doesn't actually say anything of any value, and it's been wrong for 13 years. Surely that's enough reason to just forget about it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> wrote: > Simon Riggs <simon@2ndquadrant.com> wrote: >> I think its funny and scary, as well as being of historical >> interest. Well, I something of a similar reaction, but I think that's outweighed by the probable waste of time for anyone trying to make sense of it. > I think we should just remove it. It doesn't actually say > anything of any value, and it's been wrong for 13 years. Surely > that's enough reason to just forget about it. That works for me. -Kevin
On Wed, Mar 07, 2012 at 07:46:32AM +0000, Simon Riggs wrote: > There is much wisdom there and much wisdom in leaving ancient warnings > as we find them. The comment is a wise and insightful statement -- about a totally different system than we have today. > Are these the words you object to? > > "we don't need to > > * check commit time against the start time of this transaction > > * because 2ph locking protects us from doing the wrong thing." Yes, that clearly isn't true, and the subsequent bit about catalog accesses isn't right either -- they may not be serializable, but that isn't the reason why. I don't particularly object to the warning that "the tests in this routine are correct" (although indeed the fact that they've changed over the years does seem to belie it). So I'm also in favor of just removing the comment entirely. Dan -- Dan R. K. Ports MIT CSAIL http://drkp.net/