Re: Removing cruft in access/transam/xact.c

On Sat, Mar 27, 2004 at 12:21:07AM -0500, Tom Lane wrote:
> Alvaro Herrera <alvherre(at)dcc(dot)uchile(dot)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)