Обсуждение: Removing cruft in access/transam/xact.c

Patchers,

This patch removes the unnecesary TRANS_* states that supposedly
represented "low level transaction state".  The state is actually
unnecesary because the states can be accurately represented using the
TBLOCK_* states.  This simplifies the code somewhat.

It also allows the state machinery to be actually represented as a
directed graph.  If you look closely you'll see that the first graph I
posted yesterday was missing the StartTransactionCommand stuff; it
wasn't representable in an obvious ways.  This patch corrects that, by
adding a new TBLOCK_STARTED state.

I also attach the graph that represents this machinery.  Don't add this
to the source; it will probably be better to add the .dot source file.

I also removed some #ifdef NOT_USED code, and the IsTransactionState()
function, which can be replaced by IsTransactionOrTransactionBlock().
Maybe the latter can be renamed ...

This passes the full regression test suite.  Please review and apply if
OK.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"I think my standards have lowered enough that now I think 'good design'
is when the page doesn't irritate the living f*ck out of me." (JWZ)

Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> This patch removes the unnecesary TRANS_* states that supposedly
> represented "low level transaction state".  The state is actually
> unnecesary because the states can be accurately represented using the
> TBLOCK_* states.

Really?

Your changes to StartTransaction() are alone enough to refute the above
claim.  With these changes, there is no longer any state in xact.c that
can detect a failure during transaction startup.

The similar changes that remove the ability to recognize failures
during AbortTransaction are even worse, because cleanup after a failed
transaction is exactly where you would most expect software bugs to
pop up.

We could talk about a different solution to detecting such failures
(maybe it's okay to convert the whole thing into a critical section)
but simply removing all hope of detecting a failure won't do.

The comments in xact.c make it perfectly clear that it took several
tries for the Berkeley crew to get this code right.  I'm not eager
to simplify it on the basis of one person's unsupported assertion
that the complexity is unnecessary.  If you can prove it's unnecessary,
let's see the proof.

> This passes the full regression test suite.

The regression test suite does not cover "can't-happen" cases ...

            regards, tom lane

On Sat, Mar 27, 2004 at 12:21:07AM -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> > This patch removes the unnecesary TRANS_* states that supposedly
> > represented "low level transaction state".  The state is actually
> > unnecesary because the states can be accurately represented using the
> > TBLOCK_* states.
>
> Really?
>
> Your changes to StartTransaction() are alone enough to refute the above
> claim.  With these changes, there is no longer any state in xact.c that
> can detect a failure during transaction startup.

Yeah, I'd agree with you, but as you can see there is no decision and no
action taken based on the value of the transaction state in the current
code.  At most there are some elog(WARNING), and besides showing a
message to the user, there is no side effect.  Actually, there is
*a single* elog(FATAL) in CleanupTransaction().

> The similar changes that remove the ability to recognize failures
> during AbortTransaction are even worse, because cleanup after a failed
> transaction is exactly where you would most expect software bugs to
> pop up.

> We could talk about a different solution to detecting such failures
> (maybe it's okay to convert the whole thing into a critical section)
> but simply removing all hope of detecting a failure won't do.

I think each of StartTransaction, AbortTransaction, CleanupTransaction
and CommitTransaction should verify the TBLOCK state before doing
anything, and elog(FATAL) if they are called from a state they
shouldn't.

In fact, I remember clearly seeing a WARNING from this code in the
Chris K-L's failure when he got a disk full.  I haven't read that log
but I suspect it would have been better if a stronger check had taken
place.

> The comments in xact.c make it perfectly clear that it took several
> tries for the Berkeley crew to get this code right.

Yeah.  Fact is, the code has been hacked and whacked a lot.  I think the
only cleanups I've seen in the CVS logs were by yourself, several years
ago ...

> I'm not eager to simplify it on the basis of one person's unsupported
> assertion that the complexity is unnecessary.  If you can prove it's
> unnecessary, let's see the proof.

I'm not sure how a proof should be, but the fact that the TRANS states
are not used except for log readers amusement should be an indicator.

> > This passes the full regression test suite.
>
> The regression test suite does not cover "can't-happen" cases ...

Of course.

The current situation is barely sustainable for the current transaction
machinery.  If something goes wild, going on with the current routine
may cause little or no harm.  With subtransactions the whole thing will
be different (corrupting the transaction stack, for example) and we will
need stronger state checking.

Would you agree to the change if I add checks as I outlined above?

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.              (Don Knuth)

On Sat, Mar 27, 2004 at 09:12:15PM -0400, Alvaro Herrera wrote:
> On Sat, Mar 27, 2004 at 12:21:07AM -0500, Tom Lane wrote:
> > Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> > > This patch removes the unnecesary TRANS_* states that supposedly
> > > represented "low level transaction state".  The state is actually
> > > unnecesary because the states can be accurately represented using the
> > > TBLOCK_* states.
> >
> > Really?

> > The similar changes that remove the ability to recognize failures
> > during AbortTransaction are even worse, because cleanup after a failed
> > transaction is exactly where you would most expect software bugs to
> > pop up.

I think I see your point.  CleanupTransaction is not allowed to "clean
up" if AbortTransaction did not finish properly.  However if this is the
criterion to apply to all those routines, I think they should all have
elog(FATAL) in case they do not see the correct state.  Why do they only
have elog(WARNING) and are allowed to continue?

> > We could talk about a different solution to detecting such failures
> > (maybe it's okay to convert the whole thing into a critical section)
> > but simply removing all hope of detecting a failure won't do.

IMHO all of StartTransaction, CommitTransaction, CleanupTransaction and
AbortTransaction should be critical sections.  However, they do quite a
lot of work.  Is this acceptable?  If not, maybe I'll leave the TRANS
states as means to detect incomplete execution, but clean up the code
anyway and change all those elog(WARNING) into elog(FATAL).

Comments?

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"La verdad no siempre es bonita, pero el hambre de ella sí"

On Sat, Mar 27, 2004 at 12:21:07AM -0500, Tom Lane wrote:

[...]

> The similar changes that remove the ability to recognize failures
> during AbortTransaction are even worse, because cleanup after a failed
> transaction is exactly where you would most expect software bugs to
> pop up.

Hey, I was just adding the code back when I noticed that
AbortTransaction() sets the TRANS_ABORT state just _before_ doing it's
work, not after.  And all functions are executed with interrupts held
(HOLD_INTERRUPTS / RELEASE_INTERRUPTS).  So the comments I made earlier
are irrelevant.

After all this, I still think the TRANS state is unnecesary.  I will add
checks in the low level routines so they see what TBLOCK state they are
called in, which should be enough to keep the current functionality
and robustness.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"A wizard is never late, Frodo Baggins, nor is he early.
He arrives precisely when he means to."  (Gandalf, en LoTR FoTR)

On Sun, Mar 28, 2004 at 06:16:59PM -0400, Alvaro Herrera wrote:

> After all this, I still think the TRANS state is unnecesary.  I will add
> checks in the low level routines so they see what TBLOCK state they are
> called in, which should be enough to keep the current functionality
> and robustness.

Please review this one.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"La persona que no quería pecar / estaba obligada a sentarse
en duras y empinadas sillas    / desprovistas, por cierto
de blandos atenuantes"                          (Patricio Vogel)