Skip to content

Commit 968b892

Browse files
tglsfdctglsfdc
committed
Force immediate commit after CREATE DATABASE etc in extended protocol.
We have a few commands that "can't run in a transaction block", meaning that if they complete their processing but then we fail to COMMIT, we'll be left with inconsistent on-disk state. However, the existing defenses for this are only watertight for simple query protocol. In extended protocol, we didn't commit until receiving a Sync message. Since the client is allowed to issue another command instead of Sync, we're in trouble if that command fails or is an explicit ROLLBACK. In any case, sitting in an inconsistent state while waiting for a client message that might not come seems pretty risky. This case wasn't reachable via libpq before we introduced pipeline mode, but it's always been an intended aspect of extended query protocol, and likely there are other clients that could reach it before. To fix, set a flag in PreventInTransactionBlock that tells exec_execute_message to force an immediate commit. This seems to be the approach that does least damage to existing working cases while still preventing the undesirable outcomes. While here, add some documentation to protocol.sgml that explicitly says how to use pipelining. That's latent in the existing docs if you know what to look for, but it's better to spell it out; and it provides a place to document this new behavior. Per bug #17434 from Yugo Nagata. It's been wrong for ages, so back-patch to all supported branches. Discussion: https://postgr.es/m/17434-d9f7a064ce2a88a3@postgresql.org
1 parent 042554d commit 968b892

File tree

4 files changed

+104
-26
lines changed

4 files changed

+104
-26
lines changed

‎doc/src/sgml/protocol.sgml

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1050,6 +1050,64 @@ SELCT 1/0;<!-- this typo is intentional -->
10501050
</note>
10511051
</sect2>
10521052

1053+
<sect2 id="protocol-flow-pipelining">
1054+
<title>Pipelining</title>
1055+
1056+
<indexterm zone="protocol-flow-pipelining">
1057+
<primary>pipelining</primary>
1058+
<secondary>protocol specification</secondary>
1059+
</indexterm>
1060+
1061+
<para>
1062+
Use of the extended query protocol
1063+
allows <firstterm>pipelining</firstterm>, which means sending a series
1064+
of queries without waiting for earlier ones to complete. This reduces
1065+
the number of network round trips needed to complete a given series of
1066+
operations. However, the user must carefully consider the required
1067+
behavior if one of the steps fails, since later queries will already
1068+
be in flight to the server.
1069+
</para>
1070+
1071+
<para>
1072+
One way to deal with that is to make the whole query series be a
1073+
single transaction, that is wrap it in <command>BEGIN</command> ...
1074+
<command>COMMIT</command>. However, this does not help if one wishes
1075+
for some of the commands to commit independently of others.
1076+
</para>
1077+
1078+
<para>
1079+
The extended query protocol provides another way to manage this
1080+
concern, which is to omit sending Sync messages between steps that
1081+
are dependent. Since, after an error, the backend will skip command
1082+
messages until it finds Sync, this allows later commands in a pipeline
1083+
to be skipped automatically when an earlier one fails, without the
1084+
client having to manage that explicitly with <command>BEGIN</command>
1085+
and <command>COMMIT</command>. Independently-committable segments
1086+
of the pipeline can be separated by Sync messages.
1087+
</para>
1088+
1089+
<para>
1090+
If the client has not issued an explicit <command>BEGIN</command>,
1091+
then each Sync ordinarily causes an implicit <command>COMMIT</command>
1092+
if the preceding step(s) succeeded, or an
1093+
implicit <command>ROLLBACK</command> if they failed. However, there
1094+
are a few DDL commands (such as <command>CREATE DATABASE</command>)
1095+
that cannot be executed inside a transaction block. If one of
1096+
these is executed in a pipeline, it will, upon success, force an
1097+
immediate commit to preserve database consistency.
1098+
A Sync immediately following one of these has no effect except to
1099+
respond with ReadyForQuery.
1100+
</para>
1101+
1102+
<para>
1103+
When using this method, completion of the pipeline must be determined
1104+
by counting ReadyForQuery messages and waiting for that to reach the
1105+
number of Syncs sent. Counting command completion responses is
1106+
unreliable, since some of the commands may not be executed and thus not
1107+
produce a completion message.
1108+
</para>
1109+
</sect2>
1110+
10531111
<sect2>
10541112
<title>Function Call</title>
10551113

‎src/backend/access/transam/xact.c

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3343,6 +3343,9 @@ AbortCurrentTransaction(void)
33433343
* could issue more commands and possibly cause a failure after the statement
33443344
* completes). Subtransactions are verboten too.
33453345
*
3346+
* We must also set XACT_FLAGS_NEEDIMMEDIATECOMMIT in MyXactFlags, to ensure
3347+
* that postgres.c follows through by committing after the statement is done.
3348+
*
33463349
* isTopLevel: passed down from ProcessUtility to determine whether we are
33473350
* inside a function. (We will always fail if this is false, but it's
33483351
* convenient to centralize the check here instead of making callers do it.)
@@ -3384,7 +3387,9 @@ PreventInTransactionBlock(bool isTopLevel, const char *stmtType)
33843387
if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
33853388
CurrentTransactionState->blockState != TBLOCK_STARTED)
33863389
elog(FATAL, "cannot prevent transaction chain");
3387-
/* all okay */
3390+
3391+
/* All okay. Set the flag to make sure the right thing happens later. */
3392+
MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
33883393
}
33893394

33903395
/*
@@ -3482,6 +3487,13 @@ IsInTransactionBlock(bool isTopLevel)
34823487
CurrentTransactionState->blockState != TBLOCK_STARTED)
34833488
return true;
34843489

3490+
/*
3491+
* If we tell the caller we're not in a transaction block, then inform
3492+
* postgres.c that it had better commit when the statement is done.
3493+
* Otherwise our report could be a lie.
3494+
*/
3495+
MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
3496+
34853497
return false;
34863498
}
34873499

‎src/backend/tcop/postgres.c

Lines changed: 27 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1248,6 +1248,13 @@ exec_simple_query(const char *query_string)
12481248
}
12491249
else
12501250
{
1251+
/*
1252+
* We had better not see XACT_FLAGS_NEEDIMMEDIATECOMMIT set if
1253+
* we're not calling finish_xact_command(). (The implicit
1254+
* transaction block should have prevented it from getting set.)
1255+
*/
1256+
Assert(!(MyXactFlags & XACT_FLAGS_NEEDIMMEDIATECOMMIT));
1257+
12511258
/*
12521259
* We need a CommandCounterIncrement after every query, except
12531260
* those that start or end a transaction block.
@@ -1985,32 +1992,16 @@ exec_execute_message(const char *portal_name, long max_rows)
19851992

19861993
/*
19871994
* We must copy the sourceText and prepStmtName into MessageContext in
1988-
* case the portal is destroyed during finish_xact_command. Can avoid the
1989-
* copy if it's not an xact command, though.
1995+
* case the portal is destroyed during finish_xact_command. We do not
1996+
* make a copy of the portalParams though, preferring to just not print
1997+
* them in that case.
19901998
*/
1991-
if (is_xact_command)
1992-
{
1993-
sourceText = pstrdup(portal->sourceText);
1994-
if (portal->prepStmtName)
1995-
prepStmtName = pstrdup(portal->prepStmtName);
1996-
else
1997-
prepStmtName = "<unnamed>";
1998-
1999-
/*
2000-
* An xact command shouldn't have any parameters, which is a good
2001-
* thing because they wouldn't be around after finish_xact_command.
2002-
*/
2003-
portalParams = NULL;
2004-
}
1999+
sourceText = pstrdup(portal->sourceText);
2000+
if (portal->prepStmtName)
2001+
prepStmtName = pstrdup(portal->prepStmtName);
20052002
else
2006-
{
2007-
sourceText = portal->sourceText;
2008-
if (portal->prepStmtName)
2009-
prepStmtName = portal->prepStmtName;
2010-
else
2011-
prepStmtName = "<unnamed>";
2012-
portalParams = portal->portalParams;
2013-
}
2003+
prepStmtName = "<unnamed>";
2004+
portalParams = portal->portalParams;
20142005

20152006
/*
20162007
* Report query to various monitoring facilities.
@@ -2098,13 +2089,24 @@ exec_execute_message(const char *portal_name, long max_rows)
20982089

20992090
if (completed)
21002091
{
2101-
if (is_xact_command)
2092+
if (is_xact_command || (MyXactFlags & XACT_FLAGS_NEEDIMMEDIATECOMMIT))
21022093
{
21032094
/*
21042095
* If this was a transaction control statement, commit it. We
21052096
* will start a new xact command for the next command (if any).
2097+
* Likewise if the statement required immediate commit. Without
2098+
* this provision, we wouldn't force commit until Sync is
2099+
* received, which creates a hazard if the client tries to
2100+
* pipeline immediate-commit statements.
21062101
*/
21072102
finish_xact_command();
2103+
2104+
/*
2105+
* These commands typically don't have any parameters, and even if
2106+
* one did we couldn't print them now because the storage went
2107+
* away during finish_xact_command. So pretend there were none.
2108+
*/
2109+
portalParams = NULL;
21082110
}
21092111
else
21102112
{

‎src/include/access/xact.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,12 @@ extern int MyXactFlags;
103103
*/
104104
#define XACT_FLAGS_ACQUIREDACCESSEXCLUSIVELOCK (1U << 1)
105105

106+
/*
107+
* XACT_FLAGS_NEEDIMMEDIATECOMMIT - records whether the top level statement
108+
* is one that requires immediate commit, such as CREATE DATABASE.
109+
*/
110+
#define XACT_FLAGS_NEEDIMMEDIATECOMMIT (1U << 2)
111+
106112
/*
107113
* start- and end-of-transaction callbacks for dynamically loaded modules
108114
*/

0 commit comments

Comments
 (0)