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

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(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?

> > 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í"