Thread: Subtle bug in clog.c

I'm wondering about the following statement in TransactionIdSetStatus():
   *byteptr |= (status << bshift);

When the status is SUBCOMMITTED, the bytemask will have 11 for those two
bytes; therefore, when OR-ing a 10 or 01 mask (committed or aborted),
shouldn't the byte remaing the same as before?  That is, changing from
SUBCOMMITTED to either COMMIT or ABORT does not actually do anything?

I wonder why this works.  In the phantom Xid patch I had to set the
bytes to zero first, and then set the new bits ... it took me quite a
while to figure it out and I'm still wondering why the current code
doesn't break.

In the phantom patch I had to add this line before the OR-ing:

+       *byteptr &= ~(TRANSACTION_STATUS_SUB_COMMITTED << bshift);


My theory is that the current code is a no-op and that it works because
the server recurses up the subtrans tree to find the parent state ... An
aborted subxact only works because we don't mark it SUBCOMMIT, so
committed subxacts with aborted parent _always_ have to recurse up the
tree subtrans tree.  (An experiment to prove this theory could involve
marking SUBCOMMIT all subtransaction at start -- things will break all
around and they shouldn't.)

-- 
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"Always assume the user will do much worse than the stupidest thing
you can imagine."                                (Julien PUYDT)

On Fri, Jul 02, 2004 at 01:09:56PM -0400, Alvaro Herrera wrote:

Sorry:

> When the status is SUBCOMMITTED, the bytemask will have 11 for those two
^^^^^^^^should be bitmask
 
> bytes; therefore, when OR-ing a 10 or 01 mask (committed or aborted), ^^^^^ should be bits
> shouldn't the byte remaing the same as before?  That is, changing from
> SUBCOMMITTED to either COMMIT or ABORT does not actually do anything?

-- 
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"La experiencia nos dice que el hombre peló millones de veces las patatas,
pero era forzoso admitir la posibilidad de que en un caso entre millones,
las patatas pelarían al hombre" (Ijon Tichy)

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> I'm wondering about the following statement in TransactionIdSetStatus():
>     *byteptr |= (status << bshift);

> When the status is SUBCOMMITTED, the bytemask will have 11 for those two
> bytes; therefore, when OR-ing a 10 or 01 mask (committed or aborted),

Um.  That code is assuming that the initial state was either 00 or the
target value (per the former state of the Assert above it).  You're
right, we need to change that.

At one time this behavior was important because there wasn't
synchronization between writers and readers of clog; so transformations
could only set bits and not clear them.  That doesn't matter anymore
though, because there are page-level locks taken in clog.c.

> I wonder why this works.

It doesn't --- the state remains SUBCOMMITTED.

> My theory is that the current code is a no-op and that it works because
> the server recurses up the subtrans tree to find the parent state ...

Right.

I'm off to dinner but will commit a fix for this when I return.
        regards, tom lane