Re: chained transactions

From: Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>
To: Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com>
Cc: Michael Paquier <michael(at)paquier(dot)xyz>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: chained transactions
Date: 2018-12-26 08:20:55
Message-ID: alpine.DEB.2.21.1812260824270.32444@lancre
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Hello Peter,

>> Shouldn't psql tab completion be updated as well?
> Done. (I only did the AND CHAIN, not the AND NO CHAIN.)

Sure, that is the useful part.

>> I must admit that do not like much the three global variables &
>> Save/Restore functions. I'd suggest saving directly into 3 local variables
>> in function CommitTransactionCommand, and restoring them when needed. Code
>> should not be longer. I'd would not bother to skip saving when not
>> chaining.
> We're also using those functions in the 0002 patch.

Hmmm. I have not looked at the second one yet.

> Should we just repeat the code three times, or use macros?

I try to avoid global variables when possible as a principle, because I
paid to learn that they are bad:-) Maybe I'd do a local struct, declare an
instance within the function, and write two functions to dump/restore the
transaction status variables into the referenced struct. Maybe this is not
worth the effort.

>> Copying & comparing nodes are updated. Should making, outing and reading
>> nodes also be updated?
> TransactionStmt isn't covered by the node serialization functions, so I
> didn't see anything to update. What did you have in mind?

Sigh. I had in mind that the serialization feature would work with all
possible nodes, not just some of them… which seems quite naïve. The whole
make/copy/cmp/in/out functions depress me, all this verbose code should be
automatically generated from struct declarations. I'm pretty sure there
are hidden bugs in there.

>> About the tests: I'd suggest to use more options on the different tests,
>> eg SERIALIZABLE, READ ONLY… Also ISTM that tests should show
>> transaction_read_only value as well.
> OK, updated a bit. (Using READ ONLY doesn't work because then you can't
> write anything inside the transaction.)

Sure. Within a read-only tx, it could check that transaction_read_only is
on, and still on when chained, though.

> Updated patch attached.

First patch applies cleanly, compiles, make check ok.

Further remarks, distinct from the above comments and suggestions:

The documentation should probably tell in the compatibility sections that
AND CHAIN is standard conforming.

In the automatic rollback tests, maybe I'd insert 3 just once, and use
another value for the chained transaction.

Tests may eventually vary BEGIN/START, COMMIT/END, ROLLBACK/ABORT.

As you added a SAVEPOINT, maybe I'd try rollbacking to it.


In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Mitar 2018-12-26 08:26:32 Re: Feature: triggers on materialized views
Previous Message Fabien COELHO 2018-12-26 07:40:00 RE: Timeout parameters