Re: chained transactions

Peter Eisentraut wrote:

> Subject: [PATCH v1 1/8] Update function comments
>
> After a6542a4b6870a019cd952d055d2e7af2da2fe102, some function comments
> were misplaced. Fix that.

Note typo WarnNoTranactionChain in one comment. The patch leaves
CheckTransactionChain with no comment whatsoever; maybe add four words
to indicate that it's implementation for the other two? The phrase
"Thus this is an inverse for PreventTransactionChain" seems to apply to
both functions, maybe it should be in plural? Or perhaps "thus this
behavior is the inverse of"?

> Subject: [PATCH v1 2/8] Rename TransactionChain functions
>
> We call this thing a "transaction block" everywhere except in a few
> functions, where it is mysteriously called a "transaction chain". In
> the SQL standard, a transaction chain is something different. So rename
> these functions to match the common terminology.

Seems reasonable to me; doesn't change any functionality.

> Subject: [PATCH v1 3/8] Simplify parse representation of savepoint commands
>
> Instead of embedding the savepoint name in a list and then requiring
> complex code to unpack it, just add another struct field to store it
> directly.

Obvious in hindsight.

> Subject: [PATCH v1 4/8] Improve savepoint error messages
>
> Include the savepoint name in the error message and rephrase it a bit to
> match common style.

A no-brainer. It's a bit disquieting that this changes so few test
results ...

> Subject: [PATCH v1 5/8] Change transaction state debug strings to match enum
> symbols
>
> In some cases, these were different for no apparent reason, making
> debugging unnecessarily mysterious.

I guess I was trying to save bytes (573a71a5da70) ... looks OK to me.

> From 517bc6d9fefdee9135857d9562f644f2984ace32 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut <peter_e(at)gmx(dot)net>
> Date: Sun, 18 Feb 2018 09:33:53 -0500
> Subject: [PATCH v1 6/8] Turn transaction_isolation into GUC enum
>
> XXX no idea why it was done the way it was, but this seems much simpler
> and apparently doesn't change any functionality.

Enums are recent -- 52a8d4f8f7e2, only 10 years old, and the commit
didn't convert all cases, leaving some for later. Funnily enough,
default_transaction_isolation was changed afterwards by ad6bf716baa7 but
not this one ... I guess "later" is now upon us for it.

No opinions (yet?) on the rest of it.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services