pgsql: Fix handling of savepoint commands within multi-statement Query

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-committers(at)postgresql(dot)org
Subject: pgsql: Fix handling of savepoint commands within multi-statement Query
Date: 2017-09-07 13:50:04
Message-ID: E1dpxC0-000060-KO@gemulon.postgresql.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-committers

Fix handling of savepoint commands within multi-statement Query strings.

Issuing a savepoint-related command in a Query message that contains
multiple SQL statements led to a FATAL exit with a complaint about
"unexpected state STARTED". This is a shortcoming of commit 4f896dac1,
which attempted to prevent such misbehaviors in multi-statement strings;
its quick hack of marking the individual statements as "not top-level"
does the wrong thing in this case, and isn't a very accurate description
of the situation anyway.

To fix, let's introduce into xact.c an explicit model of what happens for
multi-statement Query strings. This is an "implicit transaction block
in progress" state, which for many purposes works like the normal
TBLOCK_INPROGRESS state --- in particular, IsTransactionBlock returns true,
causing the desired result that PreventTransactionChain will throw error.
But in case of error abort it works like TBLOCK_STARTED, allowing the
transaction to be cancelled without need for an explicit ROLLBACK command.

Commit 4f896dac1 is reverted in toto, so that we go back to treating the
individual statements as "top level". We could have left it as-is, but
this allows sharpening the error message for PreventTransactionChain
calls inside functions.

Except for getting a normal error instead of a FATAL exit for savepoint
commands, this patch should result in no user-visible behavioral change
(other than that one error message rewording). There are some things
we might want to do in the line of changing the appearance or wording of
error and warning messages around this behavior, which would be much
simpler to do now that it's an explicitly modeled state. But I haven't
done them here.

Although this fixes a long-standing bug, no backpatch. The consequences
of the bug don't seem severe enough to justify the risk that this commit
itself creates some new issue.

Patch by me, but it owes something to previous investigation by
Takayuki Tsunakawa, who also reported the bug in the first place.
Also thanks to Michael Paquier for reviewing.

Discussion: https://postgr.es/m/0A3221C70F24FB45833433255569204D1F6BE40D@G01JPEXMBYT05

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/6eb52da3948dc8bc7c8a61cbacac14823b670c58

Modified Files
--------------
src/backend/access/transam/xact.c | 160 ++++++++++++++++++++++++++---
src/backend/tcop/postgres.c | 57 ++++++----
src/include/access/xact.h | 2 +
src/test/regress/expected/transactions.out | 84 +++++++++++++++
src/test/regress/sql/transactions.sql | 54 ++++++++++
5 files changed, 320 insertions(+), 37 deletions(-)

Browse pgsql-committers by date

  From Date Subject
Next Message Robert Haas 2017-09-07 15:16:49 pgsql: Even if some partitions are foreign, allow tuple routing.
Previous Message Tom Lane 2017-09-07 12:50:06 pgsql: Further marginal hacking on generic atomic ops.