Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Tom Lane
> The originally reported bug is fixed.  Not making any claims about other
> bugs ...

I'm sorry I couldn't reply to you.  I've recently been in a situation where I 
can't use my time for development.  I think I'll be able to rejoin the 
community activity soon.

I confirmed your patch fixed the problem.  And the code looks perfect.  Thank 
you very much.

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

Catalin Iacob  writes:
> When reading this I also realized that the backend does send responses for
> every individual query in a multi-query request, it's only libpq's PQexec
> that throws away the intermediate results and only provides access to the
> last one.

If you want to see them all, you can use PQsendQuery/PQgetResult.

https://www.postgresql.org/docs/current/static/libpq-async.html

There's a case to be made that we should change psql to use these
and print all the results not just the last one.  I've not looked
to see how much work that would be; but now that we're actually
documenting how to script multi-command queries, it might be
a good idea to fix it before too many people have scripts that
rely on the current behavior.

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Catalin Iacob]

On Thu, Sep 7, 2017 at 8:07 PM, Tom Lane  wrote:
> I've pushed up an attempt at this:
>
>
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b976499480bdbab6d69a11e47991febe53865adc
>
> Feel free to suggest improvements.

Thank you, this helps a lot. Especially since some of the behavior is a bit
surprising, for example stopping on error leading to ROLLBACK not being
done and the retroactive upgrade of preceding commands in an implicit block
to a transaction block when a BEGIN appears.

When reading this I also realized that the backend does send responses for
every individual query in a multi-query request, it's only libpq's PQexec
that throws away the intermediate results and only provides access to the
last one. I always thought the backend did that. The docs hinted that it's
the frontend ("psql only prints the last one", "PGresult describes the
result of the last command") but to assure myself I looked with tcpdump.

It's a pity that the underlying protocol has 2 ways to do batching of
queries but the official library hides both. I guess I should go review the
"Batch/pipelining support for libpq" patch rather than complaining.

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

Simon Riggs  writes:
> On 7 September 2017 at 11:31, Tom Lane  wrote:
>> Haas' idea of some kind of syntactic extension, like "LET guc1 = x,
>> guc2 = y FOR statement" seems more feasible to me.  I'm not necessarily
>> wedded to that particular syntax, but I think it has to look like
>> a single-statement construct of some kind.

> Always happy to use a good idea... (any better way to re-locate that
> discussion?)

https://www.postgresql.org/message-id/ca+tgmobgd_uzrs44couty1odnbr0c_hjsxvx_dmrevz-cwu...@mail.gmail.com

> Requires a new GUC mode for "statement local" rather than "transaction local"

Yeah, something along that line.

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

Simon Riggs  writes:
> On 7 September 2017 at 11:24, Tom Lane  wrote:
>> Not hearing anything, I already pushed my patch an hour or three ago.

> Yes, I saw. Are you saying that doc commit is all we need? ISTM we
> still had an actual bug.

The originally reported bug is fixed.  Not making any claims about
other bugs ...

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Simon Riggs]

On 7 September 2017 at 11:31, Tom Lane  wrote:
> Simon Riggs  writes:
>> I would like to relax the restriction to allow this specific use case...
>>   SET work_mem = X; SET max_parallel_workers = 4; SELECT ...
>> so we still have only one command (the last select), yet we have
>> multiple GUC settings beforehand.
>
> On what basis do you claim that's only one command?  It would return
> multiple CommandCompletes, for starters, so that it breaks the protocol
> just as effectively as any other loosening.
>
> Moreover, I imagine the semantics you really want is that the SETs only
> apply for the duration of the command.  This wouldn't provide that
> result either.

> Haas' idea of some kind of syntactic extension, like "LET guc1 = x,
> guc2 = y FOR statement" seems more feasible to me.  I'm not necessarily
> wedded to that particular syntax, but I think it has to look like
> a single-statement construct of some kind.

Always happy to use a good idea... (any better way to re-locate that
discussion?)

1. Allow SET to set multiple parameters...
SET guc1 = x, guc2 = y
This looks fairly straightforward

2. Allow SET to work only for a single command...
SET guc1 = x, guc2 = y FOR query
Don't see anything too bad about that...
Requires a new GUC mode for "statement local" rather than "transaction local"

-- 
Simon Riggshttp://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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Simon Riggs]

On 7 September 2017 at 11:24, Tom Lane  wrote:
> Simon Riggs  writes:
>> On 5 September 2017 at 10:22, Tom Lane  wrote:
>>> Does anyone want to do further review on this patch?  If so, I'll
>>> set the CF entry back to "Needs Review".
>
>> OK, I'll review Michael's patch (and confirm my patch is dead)
>
> Not hearing anything, I already pushed my patch an hour or three ago.

Yes, I saw. Are you saying that doc commit is all we need? ISTM we
still had an actual bug.

-- 
Simon Riggshttp://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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

Simon Riggs  writes:
> I would like to relax the restriction to allow this specific use case...
>   SET work_mem = X; SET max_parallel_workers = 4; SELECT ...
> so we still have only one command (the last select), yet we have
> multiple GUC settings beforehand.

On what basis do you claim that's only one command?  It would return
multiple CommandCompletes, for starters, so that it breaks the protocol
just as effectively as any other loosening.

Moreover, I imagine the semantics you really want is that the SETs only
apply for the duration of the command.  This wouldn't provide that
result either.

Haas' idea of some kind of syntactic extension, like "LET guc1 = x,
guc2 = y FOR statement" seems more feasible to me.  I'm not necessarily
wedded to that particular syntax, but I think it has to look like
a single-statement construct of some kind.

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

Simon Riggs  writes:
> On 5 September 2017 at 10:22, Tom Lane  wrote:
>> Does anyone want to do further review on this patch?  If so, I'll
>> set the CF entry back to "Needs Review".

> OK, I'll review Michael's patch (and confirm my patch is dead)

Not hearing anything, I already pushed my patch an hour or three ago.

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Simon Riggs]

On 7 September 2017 at 11:07, Tom Lane  wrote:
> I wrote:
>> Yeah, it seems like we have now made this behavior official enough that
>> it's time to document it better.  My thought is to create a new subsection
>> in the FE/BE Protocol chapter that explains how multi-statement Query
>> messages are handled, and then to link to that from appropriate places
>> elsewhere.  If anyone thinks the reference section would be better put
>> somewhere else than Protocol, please say where.
>
> I've pushed up an attempt at this:
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b976499480bdbab6d69a11e47991febe53865adc
>
> Feel free to suggest improvements.

Not so much an improvement as a follow-on thought:

All of this applies to simple queries.

At present we restrict using multi-statement requests in extended
protocol, saying that we don't allow it because of a protocol
restriction. The precise restriction is that we can't return more than
one reply. The restriction is implemented via this test
if (list_length(parsetree_list) > 1)
ereport(ERROR,
 (errcode(ERRCODE_SYNTAX_ERROR),
  errmsg("cannot insert multiple commands into a prepared
statement")));
at line 1277 of exec_parse_message()
which is actually more restrictive than it needs to be.

I would like to relax the restriction to allow this specific use case...
  SET work_mem = X; SET max_parallel_workers = 4; SELECT ...
so we still have only one command (the last select), yet we have
multiple GUC settings beforehand.

Any reason to disallow that?

-- 
Simon Riggshttp://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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Simon Riggs]

On 5 September 2017 at 10:22, Tom Lane  wrote:
> Michael Paquier  writes:
>> On Mon, Sep 4, 2017 at 11:15 PM, Tom Lane  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".

OK, I'll review Michael's patch (and confirm my patch is dead)

-- 
Simon Riggshttp://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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

I wrote:
> Yeah, it seems like we have now made this behavior official enough that
> it's time to document it better.  My thought is to create a new subsection
> in the FE/BE Protocol chapter that explains how multi-statement Query
> messages are handled, and then to link to that from appropriate places
> elsewhere.  If anyone thinks the reference section would be better put
> somewhere else than Protocol, please say where.

I've pushed up an attempt at this:

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b976499480bdbab6d69a11e47991febe53865adc

Feel free to suggest improvements.

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

Catalin Iacob  writes:
> On Mon, Sep 4, 2017 at 4:15 PM, Tom Lane  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.

Yeah, it seems like we have now made this behavior official enough that
it's time to document it better.  My thought is to create a new subsection
in the FE/BE Protocol chapter that explains how multi-statement Query
messages are handled, and then to link to that from appropriate places
elsewhere.  If anyone thinks the reference section would be better put
somewhere else than Protocol, please say where.

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Catalin Iacob]

On Mon, Sep 4, 2017 at 4:15 PM, Tom Lane  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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

Michael Paquier  writes:
> On Mon, Sep 4, 2017 at 11:15 PM, Tom Lane  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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Michael Paquier]

On Mon, Sep 4, 2017 at 11:15 PM, Tom Lane  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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

Michael Paquier  writes:
> Hmm. While this patch looks to me in a better shape than what Simon's
> is proposing, thinking about
> cah2-v61vxnentfj2v-zd+ma-g6kqmjgd5svxou3jbvdzqh0...@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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Michael Paquier]

On Mon, Sep 4, 2017 at 7:20 AM, Tom Lane  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-g6kqmjgd5svxou3jbvdzqh0...@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


introduce-implicit-transaction-blocks-3-michael.patch
Description: Binary data

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[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

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5e7e812..8b33676 100644
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*** typedef enum TBlockState
*** 145,150 
--- 145,151 
  	/* transaction block states */
  	TBLOCK_BEGIN,/* starting transaction block */
  	TBLOCK_INPROGRESS,			/* live transaction */
+ 	TBLOCK_IMPLICIT_INPROGRESS, /* live transaction after implicit BEGIN */
  	TBLOCK_PARALLEL_INPROGRESS, /* live transaction inside parallel worker */
  	TBLOCK_END,	/* COMMIT received */
  	TBLOCK_ABORT,/* failed xact, awaiting ROLLBACK */
*** StartTransactionCommand(void)
*** 2700,2705 
--- 2701,2707 
  			 * previous CommitTransactionCommand.)
  			 */
  		case TBLOCK_INPROGRESS:
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
  		case TBLOCK_SUBINPROGRESS:
  			break;
  
*** CommitTransactionCommand(void)
*** 2790,2795 
--- 2792,2798 
  			 * counter and return.
  			 */
  		case TBLOCK_INPROGRESS:
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
  		case TBLOCK_SUBINPROGRESS:
  			CommandCounterIncrement();
  			break;
*** AbortCurrentTransaction(void)
*** 3014,3023 
  			break;
  
  			/*
! 			 * if we aren't in a transaction block, we just do the basic abort
! 			 * & cleanup transaction.
  			 */
  		case TBLOCK_STARTED:
  			AbortTransaction();
  			CleanupTransaction();
  			s->blockState = TBLOCK_DEFAULT;
--- 3017,3028 
  			break;
  
  			/*
! 			 * If we aren't in a transaction block, we just do the basic abort
! 			 * & cleanup transaction.  For this purpose, we treat an implicit
! 			 * transaction block as if it were a simple statement.
  			 */
  		case TBLOCK_STARTED:
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
  			AbortTransaction();
  			CleanupTransaction();
  			s->blockState = TBLOCK_DEFAULT;
*** AbortCurrentTransaction(void)
*** 3148,3156 
   *	completes).  Subtransactions are verboten too.
   *
   *	isTopLevel: passed down from ProcessUtility to determine whether we are
!  *	inside a function or multi-query querystring.  (We will always fail if
!  *	this is false, but it's convenient to centralize the check here instead of
!  *	making callers do it.)
   *	stmtType: statement type name, for error messages.
   */
  void
--- 3153,3160 
   *	completes).  Subtransactions are verboten too.
   *
   *	isTopLevel: passed down from ProcessUtility to determine whether we are
!  *	inside a function.  (We will always fail if this is false, but it's
!  *	convenient to centralize the check here instead of making callers do it.)
   *	stmtType: statement type name, for error messages.
   */
  void
*** PreventTransactionChain(bool isTopLevel,
*** 3183,3190 
  		ereport(ERROR,
  (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
  		/* translator: %s represents an SQL statement name */
!  errmsg("%s cannot be executed from a function or multi-command string",
! 		stmtType)));
  
  	/* If we got past IsTransactionBlock test, should be in default state */
  	if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
--- 3187,3193 
  		ereport(ERROR,
  (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
  		/* translator: %s represents an SQL statement name */
!  errmsg("%s cannot be executed from a function", stmtType)));
  
  	/* If we got past IsTransactionBlock test, should be in default state */
  	if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
*** BeginTransactionBlock(void)
*** 3429,3434 
--- 3432,3446 
  			break;
  
  			/*
+ 			 * BEGIN converts an implicit transaction block to a regular one.
+ 			 * (Note that we allow this even if we've already done some
+ 			 * commands, which is a bit odd but matches 

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

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

diff --git a/src/backend/access/transam/xact.c b/src/backend/access/transam/xact.c
index 5e7e812..ba4b2da 100644
*** a/src/backend/access/transam/xact.c
--- b/src/backend/access/transam/xact.c
*** typedef enum TBlockState
*** 145,150 
--- 145,151 
  	/* transaction block states */
  	TBLOCK_BEGIN,/* starting transaction block */
  	TBLOCK_INPROGRESS,			/* live transaction */
+ 	TBLOCK_IMPLICIT_INPROGRESS, /* live transaction after implicit BEGIN */
  	TBLOCK_PARALLEL_INPROGRESS, /* live transaction inside parallel worker */
  	TBLOCK_END,	/* COMMIT received */
  	TBLOCK_ABORT,/* failed xact, awaiting ROLLBACK */
*** StartTransactionCommand(void)
*** 2700,2705 
--- 2701,2707 
  			 * previous CommitTransactionCommand.)
  			 */
  		case TBLOCK_INPROGRESS:
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
  		case TBLOCK_SUBINPROGRESS:
  			break;
  
*** CommitTransactionCommand(void)
*** 2790,2795 
--- 2792,2798 
  			 * counter and return.
  			 */
  		case TBLOCK_INPROGRESS:
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
  		case TBLOCK_SUBINPROGRESS:
  			CommandCounterIncrement();
  			break;
*** AbortCurrentTransaction(void)
*** 3014,3023 
  			break;
  
  			/*
! 			 * if we aren't in a transaction block, we just do the basic abort
! 			 * & cleanup transaction.
  			 */
  		case TBLOCK_STARTED:
  			AbortTransaction();
  			CleanupTransaction();
  			s->blockState = TBLOCK_DEFAULT;
--- 3017,3028 
  			break;
  
  			/*
! 			 * If we aren't in a transaction block, we just do the basic abort
! 			 * & cleanup transaction.  For this purpose, we treat an implicit
! 			 * transaction block as if it were a simple statement.
  			 */
  		case TBLOCK_STARTED:
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
  			AbortTransaction();
  			CleanupTransaction();
  			s->blockState = TBLOCK_DEFAULT;
*** BeginTransactionBlock(void)
*** 3429,3434 
--- 3434,3448 
  			break;
  
  			/*
+ 			 * BEGIN converts an implicit transaction block to a regular one.
+ 			 * (Note that we allow this even if we've already done some
+ 			 * commands, which is a bit odd but matches historical practice.)
+ 			 */
+ 		case TBLOCK_IMPLICIT_INPROGRESS:
+ 			s->blockState = TBLOCK_BEGIN;
+ 			break;
+ 
+ 			/*
  			 * Already a transaction block in progress.
  			 */
  		case TBLOCK_INPROGRESS:
*** PrepareTransactionBlock(char *gid)
*** 3503,3509 
  			 * ignore case where we are not in a transaction;
  			 * EndTransactionBlock already issued a warning.
  			 */
! 			Assert(s->blockState == TBLOCK_STARTED);
  			/* Don't send back a PREPARE result tag... */
  			result = false;
  		}
--- 3517,3524 
  			 * ignore case where we are not in a transaction;
  			 * EndTransactionBlock already issued a warning.
  			 */
! 			Assert(s->blockState == TBLOCK_STARTED ||
!    s->blockState == 

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

Simon Riggs  writes:
> On 1 September 2017 at 15:19, Tom Lane  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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Simon Riggs]

On 1 September 2017 at 15:19, Tom Lane  wrote:
> Simon Riggs  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 Riggshttp://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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tom Lane]

Simon Riggs  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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Simon Riggs]

On 1 September 2017 at 08:09, Michael Paquier  wrote:
> On Fri, Sep 1, 2017 at 3:05 PM, Simon Riggs  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:
> https://www.postgresql.org/message-id/cah2-v61vxnentfj2v-zd+ma-g6kqmjgd5svxou3jbvdzqh0...@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 Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


prevent_multistatement_savepoints.v2.patch
Description: Binary data

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Michael Paquier]

On Fri, Sep 1, 2017 at 3:05 PM, Simon Riggs  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:
https://www.postgresql.org/message-id/cah2-v61vxnentfj2v-zd+ma-g6kqmjgd5svxou3jbvdzqh0...@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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Simon Riggs]

On 17 May 2017 at 08:38, Tsunakawa, Takayuki
 wrote:
> From: Michael Paquier [mailto:michael.paqu...@gmail.com]
>> On Fri, Mar 31, 2017 at 9:58 PM, Ashutosh Bapat
>>  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 Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


prevent_multistatement_savepoints.v1.patch
Description: Binary data

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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tsunakawa, Takayuki]

From: Michael Paquier [mailto:michael.paqu...@gmail.com]
> On Fri, Mar 31, 2017 at 9:58 PM, Ashutosh Bapat
>  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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Michael Paquier]

On Fri, Mar 31, 2017 at 9:58 PM, Ashutosh Bapat
 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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Tsunakawa, Takayuki]

From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Michael Paquier]

On Sat, Apr 1, 2017 at 1:06 AM, Alvaro Herrera  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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[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.

-- 
Álvaro Herrerahttps://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

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Ashutosh Bapat]

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
 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)
>