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

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.

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