[bug fix] Savepoint-related statements terminates connection

Please add this to the next commitfest.

I think there's some misunderstanding between exec_simple_query() and
the way we manage transaction block state machine.

In exec_simple_query()
952 * We'll tell PortalRun it's a top-level command iff there's
exactly one
953 * raw parsetree. If more than one, it's effectively a
transaction block
954 * and we want PreventTransactionChain to reject unsafe
commands. (Note:
955 * we're assuming that query rewrite cannot add commands that are
956 * significant to PreventTransactionChain.)
957 */
958 isTopLevel = (list_length(parsetree_list) == 1);

it assumes that a multi-statement command is a transaction block. But
for every statement in this multi-statement, we toggle between
TBLOCK_STARTED and TBLOCK_DEFAULT never entering TBLOCK_INPROGRESS as
expected by a transaction block. It looks like we have to fix this
transaction block state machine for multi-statement commands. One way
to fix it is to call finish_xact_command() in exec_simple_query() at
958 when it sees that it's a transaction block. I am not sure if
that's correct. We have to at least fix the comment above or even stop
setting isTopLevel for mult-statement commands.

I don't think the fix in the patch is on the right track, since
RequireTransactionChain() is supposed to do exactly what the patch
intends to do.
3213 /*
3214 * RequireTransactionChain
3215 *
3216 * This routine is to be called by statements that must run inside
3217 * a transaction block, because they have no effects that persist past
3218 * transaction end (and so calling them outside a transaction block
3219 * is presumably an error). DECLARE CURSOR is an example.

Incidently we allow cursor operations in a multi-statement command
psql -d postgres -c "select 1; declare curs cursor for select * from
pg_class; fetch from curs;"
   relname    | relnamespace | reltype | reloftype | relowner | relam
| relfilenode | reltablespace | relpages | reltuples | relallvisible |
reltoastre
lid | relhasindex | relisshared | relpersistence | relkind | relnatts
| relchecks | relhasoids | relhaspkey | relhasrules | relhastriggers |
relhassubc
lass | relrowsecurity | relforcerowsecurity | relispopulated |
relreplident | relispartition | relfrozenxid | relminmxid |
relacl
| reloptions | relpartbound
--------------+--------------+---------+-----------+----------+-------+-------------+---------------+----------+-----------+---------------+-----------
----+-------------+-------------+----------------+---------+----------+-----------+------------+------------+-------------+----------------+-----------
-----+----------------+---------------------+----------------+--------------+----------------+--------------+------------+-----------------------------
+------------+--------------
 pg_statistic |           11 |   11258 |         0 |       10 |     0
|        2619 |             0 |       16 |       388 |            16 |
         2
840 | t           | f           | p              | r       |       26
|         0 | f          | f          | f           | f              |
f
     | f              | f                   | t              | n
     | f              |          547 |          1 |
{ashutosh=arwdDxt/ashutosh}
|            |
(1 row)

Then the question is why not to allow savepoints as well? For that we
have to fix transaction block state machine.

On Fri, Mar 31, 2017 at 12:40 PM, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

Hello,

I found a trivial bug that terminates the connection. The attached patch fixes this.

PROBLEM
========================================

Savepoint-related statements in a multi-command query terminates the connection unexpectedly, as follows.

$ psql -d postgres -c "SELECT 1; SAVEPOINT sp"
FATAL: DefineSavepoint: unexpected state STARTED
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
connection to server was lost

CAUSE
========================================

1. In exec_simple_query(), isTopLevel is set to false.

isTopLevel = (list_length(parsetree_list) == 1);

Then it is passed to PortalRun().

(void) PortalRun(portal,
FETCH_ALL,
isTopLevel,
receiver,
receiver,
completionTag);

2. The isTopLevel flag is passed through ProcessUtility() to RequireTransactionChain().

RequireTransactionChain(isTopLevel, "SAVEPOINT");

3. CheckTransactionChain() returns successfully here:

/*
* inside a function call?
*/
if (!isTopLevel)
return;

4. Finally, unexpectedly called DefineSavepoint() reports invalid transaction block state.

/* These cases are invalid. */
case TBLOCK_DEFAULT:
case TBLOCK_STARTED:
...
elog(FATAL, "DefineSavepoint: unexpected state %s",
BlockStateAsString(s->blockState));

SOLUTION
========================================

The manual page says "Savepoints can only be established when inside a transaction block." So just check for it.

https://www.postgresql.org/docs/devel/static/sql-savepoint.html

RESULT AFTER FIX
========================================

$ psql -d postgres -c "SELECT 1; SAVEPOINT sp"
ERROR: SAVEPOINT can only be used in transaction blocks

Regards
Takayuki Tsunakawa

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Ashutosh Bapat wrote:

Please add this to the next commitfest.

If this cannot be reproduced in 9.6, then it must be added to the
Open Items wiki page instead.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Apr 1, 2017 at 1:06 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:

Ashutosh Bapat wrote:

Please add this to the next commitfest.

If this cannot be reproduced in 9.6, then it must be added to the
Open Items wiki page instead.

The behavior reported can be reproduced further down (just tried on
9.3, gave up below). Like Tsunakawa-san, I am surprised to see that an
elog() message is exposed to the user.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: pgsql-hackers-owner@postgresql.org

[mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Alvaro Herrera
Ashutosh Bapat wrote:

Please add this to the next commitfest.

If this cannot be reproduced in 9.6, then it must be added to the Open Items
wiki page instead.

I added this in next CF.

Regards
Takayuki Tsunakawa

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Mar 31, 2017 at 9:58 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Then the question is why not to allow savepoints as well? For that we
have to fix transaction block state machine.

I agree with this argument. I have been looking at the patch, and what
it does is definitely incorrect. Any query string including multiple
queries sent to the server is executed as a single transaction. So,
while the current behavior of the server is definitely incorrect for
savepoints in this case, the proposed patch does not fix anything but
actually makes things worse. I think that instead of failing,
savepoints should be able to work properly. As you say cursors are
handled correctly, savepoints should fall under the same rules.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

From: Michael Paquier [mailto:michael.paquier@gmail.com]

On Fri, Mar 31, 2017 at 9:58 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Then the question is why not to allow savepoints as well? For that we
have to fix transaction block state machine.

I agree with this argument. I have been looking at the patch, and what it
does is definitely incorrect. Any query string including multiple queries
sent to the server is executed as a single transaction. So, while the current
behavior of the server is definitely incorrect for savepoints in this case,
the proposed patch does not fix anything but actually makes things worse.
I think that instead of failing, savepoints should be able to work properly.
As you say cursors are handled correctly, savepoints should fall under the
same rules.

Yes, I'm in favor of your opinion. I'll put more thought into whether it's feasible with invasive code.

Regards
Takayuki Tsunakawa

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 17 May 2017 at 08:38, Tsunakawa, Takayuki
<tsunakawa.takay@jp.fujitsu.com> wrote:

From: Michael Paquier [mailto:michael.paquier@gmail.com]

On Fri, Mar 31, 2017 at 9:58 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:

Then the question is why not to allow savepoints as well? For that we
have to fix transaction block state machine.

I agree with this argument. I have been looking at the patch, and what it
does is definitely incorrect. Any query string including multiple queries
sent to the server is executed as a single transaction. So, while the current
behavior of the server is definitely incorrect for savepoints in this case,
the proposed patch does not fix anything but actually makes things worse.
I think that instead of failing, savepoints should be able to work properly.
As you say cursors are handled correctly, savepoints should fall under the
same rules.

Yes, I'm in favor of your opinion. I'll put more thought into whether it's feasible with invasive code.

I'm not sure I see the use case for anyone using SAVEPOINTs in this
context, so simply throwing a good error message is enough.

Clearly nobody is using this, so lets just lock the door. I don't
think fiddling with the transaction block state machine is anything
anybody wants to do in back branches, at least without a better reason
than this.

Simpler version of original patch attached.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Sep 1, 2017 at 3:05 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

I'm not sure I see the use case for anyone using SAVEPOINTs in this
context, so simply throwing a good error message is enough.

Clearly nobody is using this, so lets just lock the door. I don't
think fiddling with the transaction block state machine is anything
anybody wants to do in back branches, at least without a better reason
than this.

I don't think you can say that, per se the following recent report:
/messages/by-id/CAH2-V61vxNEnTfj2V-zd+mA-g6kQMJgd5SvXoU3JBvdzQH0Yfw@mail.gmail.com
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1 September 2017 at 08:09, Michael Paquier <michael.paquier@gmail.com> wrote:

On Fri, Sep 1, 2017 at 3:05 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

I'm not sure I see the use case for anyone using SAVEPOINTs in this
context, so simply throwing a good error message is enough.

Clearly nobody is using this, so lets just lock the door. I don't
think fiddling with the transaction block state machine is anything
anybody wants to do in back branches, at least without a better reason
than this.

I don't think you can say that, per se the following recent report:
/messages/by-id/CAH2-V61vxNEnTfj2V-zd+mA-g6kQMJgd5SvXoU3JBvdzQH0Yfw@mail.gmail.com

AIUI, nobody is saying this should work, we're just discussing how to
produce an error message. We should fix it, but not spend loads of
time on it.

I've added tests to the recent patch to show it works.

Any objection to me backpatching this, please say.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Simon Riggs <simon@2ndquadrant.com> writes:

I've added tests to the recent patch to show it works.

I don't think those test cases prove anything (ie, they work fine
on an unpatched server). With a backslash maybe they would.

Any objection to me backpatching this, please say.

This patch makes me itch. Why is it correct for these three checks,
and only these three checks out of the couple dozen uses of isTopLevel
in standard_ProcessUtility, to instead do something else?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 1 September 2017 at 15:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

I've added tests to the recent patch to show it works.

I don't think those test cases prove anything (ie, they work fine
on an unpatched server). With a backslash maybe they would.

Any objection to me backpatching this, please say.

This patch makes me itch. Why is it correct for these three checks,
and only these three checks out of the couple dozen uses of isTopLevel
in standard_ProcessUtility, to instead do something else?

No problem, it was a quick fix, not a deep one.

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Simon Riggs <simon@2ndquadrant.com> writes:

On 1 September 2017 at 15:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:

This patch makes me itch. Why is it correct for these three checks,
and only these three checks out of the couple dozen uses of isTopLevel
in standard_ProcessUtility, to instead do something else?

No problem, it was a quick fix, not a deep one.

My thought is that what we need to do is find a way for isTopLevel
to be false if we're processing a multi-command string. It looks
like exec_simple_query is already doing the right thing in terms
of what it tells PortalRun; why is that not propagating down to
ProcessUtility?

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

I wrote:

My thought is that what we need to do is find a way for isTopLevel
to be false if we're processing a multi-command string.

Nah, that's backwards, the problem is exactly that isTopLevel is
false if we're processing a multi-command string. That allows
DefineSavepoint to think that it's inside a function, and we don't
disallow savepoints inside functions. (Or at least, xact.c doesn't
enforce any such prohibition; it's up to spi.c and the individual PLs
to decide if they could support that.)

After contemplating my navel for awhile, I think that this case proves
that the quick hack embodied in commit 4f896dac1 is inadequate. Rather
than piling another quick hack on top and hoping that the result is OK,
I think it's time to bite the bullet and represent the behavior we want
explicitly in the transaction machinery. Accordingly, PFA a patch
that invents a notion of an "implicit" transaction block.

I also added a bunch of test cases exercising the behavior. Except
for the problem of FATAL exits for savepoint commands, all these
cases work exactly like they do in unpatched code. However, now that
we have an explicit representation, it'd be easy to tweak the behavior
if we want to. For instance, I'm not entirely sure whether we want
the behavior that COMMIT and ROLLBACK in this state print warnings.
Good luck changing that before; but now it'd be a straightforward
adjustment.

I'm inclined to complete the reversion of 4f896dac1 by also undoing
its error message text change in PreventTransactionChain,

-                 errmsg("%s cannot be executed from a function", stmtType)));
+                 errmsg("%s cannot be executed from a function or multi-command string",
+                        stmtType)));

but this patch doesn't include that change.

My feeling about this is that we don't need a back-patch. Throwing
FATAL rather than ERROR for a misplaced savepoint command is a bit
unpleasant, but it doesn't break other sessions, and the upshot is
really the same: don't do that.

regards, tom lane

I wrote:

... PFA a patch
that invents a notion of an "implicit" transaction block.

On further consideration, I think the control logic I added in
exec_simple_query() is a shade bogus. I set it up to only force
an implicit transaction block when there are at least two statements
remaining to execute. However, that has the result of allowing, eg,

begin\; select 1\; commit\; vacuum;

Now in principle it's perfectly OK to allow that, since the vacuum
is alone in its transaction. But it feels more like an implementation
artifact than a good design. The existing code doesn't allow it,
and we might have a hard time duplicating this behavior if we ever
significantly rewrote the transaction infrastructure. Plus I'd hate
to have to explain it to users. I think we'd be better off enforcing
transaction block restrictions on every statement in a multi-command
string, regardless of the location of any COMMIT/ROLLBACK within the
string.

Hence, attached a v2 that does it like that. I also fully reverted
4f896dac1 by undoing its changes to PreventTransactionChain; other
than that, the changes in xact.c are the same as before.

regards, tom lane

On Mon, Sep 4, 2017 at 7:20 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:
On further consideration, I think the control logic I added in
exec_simple_query() is a shade bogus. I set it up to only force
an implicit transaction block when there are at least two statements
remaining to execute. However, that has the result of allowing, eg,

begin\; select 1\; commit\; vacuum;

Now in principle it's perfectly OK to allow that, since the vacuum
is alone in its transaction. But it feels more like an implementation
artifact than a good design. The existing code doesn't allow it,
and we might have a hard time duplicating this behavior if we ever
significantly rewrote the transaction infrastructure. Plus I'd hate
to have to explain it to users. I think we'd be better off enforcing
transaction block restrictions on every statement in a multi-command
string, regardless of the location of any COMMIT/ROLLBACK within the
string.

Hence, attached a v2 that does it like that. I also fully reverted
4f896dac1 by undoing its changes to PreventTransactionChain; other
than that, the changes in xact.c are the same as before.

Hmm. While this patch looks to me in a better shape than what Simon's
is proposing, thinking about
CAH2-V61vxNEnTfj2V-zd+mA-g6kQMJgd5SvXoU3JBvdzQH0Yfw@mail.gmail.com
which involved a migration Oracle->Postgres, I have been wondering if
it is possible to still allow savepoints in those cases to ease the
pain and surprise of some users. And while looking around, it seems to
me that it is possible. Please find the attached to show my idea,
based on Tom's v2. The use of a new transaction state like
IMPLICIT_INPROGRESS is something that I got in mind upthread, but I
have not shaped that into a fully-blown patch.

All the following sequences are working as I would think they should
(a couple of inserts done within each savepoint allowed me to check
that the transactions happened correctly, though the set of
regressions presented in v2 looks enough):
BEGIN; SELECT 1; SAVEPOINT sp; RELEASE sp; SAVEPOINT sp; ROLLBACK TO
SAVEPOINT sp; COMMIT;
BEGIN; SELECT 1; SAVEPOINT sp; RELEASE sp; SAVEPOINT sp; ROLLBACK TO
SAVEPOINT sp; ROLLBACK;
SELECT 1; SAVEPOINT sp; RELEASE sp; SAVEPOINT sp; ROLLBACK TO SAVEPOINT sp;
So sequences of multiple commands are working with the patch attached
even if a BEGIN is not explicitly added. On HEAD or with v2, if BEGIN
is not specified, savepoint commands cause a failure.
--
Michael

Michael Paquier <michael.paquier@gmail.com> writes:

Hmm. While this patch looks to me in a better shape than what Simon's
is proposing, thinking about
CAH2-V61vxNEnTfj2V-zd+mA-g6kQMJgd5SvXoU3JBvdzQH0Yfw@mail.gmail.com
which involved a migration Oracle->Postgres, I have been wondering if
it is possible to still allow savepoints in those cases to ease the
pain and surprise of some users.

I don't want to go there, and was thinking we should expand the new
comment in DefineSavepoint to explain why not. It's certainly not that
much additional work to allow a savepoint so far as xact.c is concerned,
as your patch shows. The problem is that intra-string savepoints seem
inconsistent with exec_simple_query's behavior of abandoning the whole
query string upon error. If you do

insert ...\; savepoint\; insert ...\; release savepoint\; insert ...;

wouldn't you sort of expect that the savepoint commands mean to keep going
if the second insert fails? If they don't mean that, what do they mean?

Also, the main thing that we need xact.c's involvement for in the first
place is the fact that implicit transaction blocks, unlike regular ones,
auto-cancel on an error, leaving you outside a block not inside a failed
one. So I don't exactly see how savepoints would fit into that.

Now I do not think we can change exec_simple_query's behavior without big
compatibility problems --- to the extent that there's a justifiable
use-case for multi-query strings at all, a big part of it is the implied
"do B only if A succeeds" semantics. But if that's what happens, then
having savepoint commands in the string is just a can of worms from both
definitional and practical points of view. If an error happens, did it
happen before or after the savepoint, and what state is the session left
in? You can't easily tell because of the lack of reporting about
savepoint state. Right now, the only real issue after a failure is "are
we in a transaction block or not", which the server does return enough
info to distinguish.

Now admittedly, the same set of issues pops up if one uses an
explicit transaction block in a multi-query string:

begin\; insert ...\; savepoint\; insert ...\; release savepoint\; insert ...\; commit;

If one of the inserts fails, you don't really know which one unless you
were counting command-complete replies (which PQexec doesn't let you do).
But that behavior was there already, we aren't proposing to make it worse.
(I think this approach is also the correct workaround to give those
Oracle-conversion folk: their real problem is failure to convert from
Oracle's implicit-BEGIN behavior to our explicit-BEGIN.)

In short, -1 for relaxing the prohibition on SAVEPOINT.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 4, 2017 at 11:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't want to go there, and was thinking we should expand the new
comment in DefineSavepoint to explain why not.

Okay.

It's certainly not that
much additional work to allow a savepoint so far as xact.c is concerned,
as your patch shows. The problem is that intra-string savepoints seem
inconsistent with exec_simple_query's behavior of abandoning the whole
query string upon error. If you do

insert ...\; savepoint\; insert ...\; release savepoint\; insert ...;

wouldn't you sort of expect that the savepoint commands mean to keep going
if the second insert fails? If they don't mean that, what do they mean?

Hmm. I spent more time looking at my patch and I see what you are
pointing out here. Using something like that with a second insert
failing I would expect the first insert to be visible, but that's not
the case:
savepoint rs; insert into exists values (1); savepoint rs2; insert
into not_exists values (1); rollback to savepoint rs2; commit;'
So this approach makes things inconsistent.

Now admittedly, the same set of issues pops up if one uses an
explicit transaction block in a multi-query string:

begin\; insert ...\; savepoint\; insert ...\; release savepoint\; insert ...\; commit;

If one of the inserts fails, you don't really know which one unless you
were counting command-complete replies (which PQexec doesn't let you do).
But that behavior was there already, we aren't proposing to make it worse.
(I think this approach is also the correct workaround to give those
Oracle-conversion folk: their real problem is failure to convert from
Oracle's implicit-BEGIN behavior to our explicit-BEGIN.)

Sure there is this workaround.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Michael Paquier <michael.paquier@gmail.com> writes:

On Mon, Sep 4, 2017 at 11:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I don't want to go there, and was thinking we should expand the new
comment in DefineSavepoint to explain why not.

Okay.

Does anyone want to do further review on this patch? If so, I'll
set the CF entry back to "Needs Review".

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Sep 4, 2017 at 4:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Also, the main thing that we need xact.c's involvement for in the first
place is the fact that implicit transaction blocks, unlike regular ones,
auto-cancel on an error, leaving you outside a block not inside a failed
one. So I don't exactly see how savepoints would fit into that.

I think this hits the nail on the head and should have a place in the
official docs as I now realize I didn't grasp this distinction before
I read this. My mental model was always "sending a bunch of semicolon
separated queries without BEGIN/COMMIT/ROLLBACK; in one PQexec is like
sending them one by one preceeded by a BEGIN; and followed by a
COMMIT; except you only get the response from the last one". Also,
explain what happens when there are BEGIN/ROLLBACK/COMMIT inside that
multiquery string, that's still not completely clear to me and I don't
want to reverse engineer it from your patch.

Now admittedly, the same set of issues pops up if one uses an
explicit transaction block in a multi-query string:

begin\; insert ...\; savepoint\; insert ...\; release savepoint\; insert ...\; commit;

According to my mental model described above, this would be exactly
the same as without the begin; and commit; which is not the case so I
think the distinction is worth explaining.

I think the lack of a more detailed explanation about the stuff above
confuses *a lot* of people, especially newcomers, and the confusion is
only increased by what client drivers do on top (like issuing implicit
BEGIN if configured in various modes specified by
language-specific-DB-independent specs like Python's DBAPI or Java's
JDBC) and one's background from other DBs that do it differently.

Speaking of the above, psql also doesn't explicitly document how it
groups lines of the file it's executing into PQexec calls. See below
for a personal example of the confusions all this generates.

I also encountered this FATAL a month ago in the context of "we have
some (migration schema) queries in some files and want to orchestrate
running them for testing". Initially we started with calling psql but
then we needed some client side logic for some other stuff and
switched to Python and Psycopg2. We did "read the whole file in a
Python string" and then call Psycopg2's execute() on that string. Note
that Psycopg2 only uses PQexec to issue queries. We had some SAVEPOINT
statements in the file which lead to the backend stopping and the next
Psycopg2 execute() on that connection saying Connection closed.
It was already confusing why Psycopg2 behaves differently than psql
(because we were issuing the whole file in one PQexec vs. psql
splitting on ; and issuing multiple PQexecs and SAVEPOINTs working
there) and the backend stopping only added to that confusion. Add on
top of that "Should we put BEGIN; and COMMIT; in the file itself? Or
is a single Psycopg2 execute() enough to have this schema migration be
applied transactionally? Is there a difference between the two?".

I searched the docs for existing explanations of multiquery strings
and found these references but all of them are a bit hand wavy:
- psql's reference explaining -c
- libpq's PQexec explanation
- the message flow document in the FE/BE protocol description

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers