Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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 historical
Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection
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 == TBLOCK_IMPLICIT_INPROGR
Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection
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
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
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
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
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
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
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
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
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
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
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
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) > return; > > > 4. Fi