Re: 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, 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). :-)

Dan

--
Dan R. K. Ports MIT CSAIL http://drkp.net/

On Wed, Mar 7, 2012 at 2:05 AM, Dan Ports <drkp(at)csail(dot)mit(dot)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(at)2ndQuadrant(dot)com> wrote:
> Dan Ports <drkp(at)csail(dot)mit(dot)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(dot)Grittner(at)wicourts(dot)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(at)2ndquadrant(dot)com> wrote:
> On Wed, Mar 7, 2012 at 4:09 PM, Kevin Grittner
> <Kevin(dot)Grittner(at)wicourts(dot)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(at)gmail(dot)com> wrote:
> Simon Riggs <simon(at)2ndquadrant(dot)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/