Skip to content

Commit 3e1297a

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 3f968b9 commit 3e1297a

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
@@ -3391,6 +3391,9 @@ AbortCurrentTransaction(void)
33913391
* could issue more commands and possibly cause a failure after the statement
33923392
* completes). Subtransactions are verboten too.
33933393
*
3394+
* We must also set XACT_FLAGS_NEEDIMMEDIATECOMMIT in MyXactFlags, to ensure
3395+
* that postgres.c follows through by committing after the statement is done.
3396+
*
33943397
* isTopLevel: passed down from ProcessUtility to determine whether we are
33953398
* inside a function. (We will always fail if this is false, but it's
33963399
* convenient to centralize the check here instead of making callers do it.)
@@ -3432,7 +3435,9 @@ PreventInTransactionBlock(bool isTopLevel, const char *stmtType)
34323435
if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
34333436
CurrentTransactionState->blockState != TBLOCK_STARTED)
34343437
elog(FATAL, "cannot prevent transaction chain");
3435-
/* all okay */
3438+
3439+
/* All okay. Set the flag to make sure the right thing happens later. */
3440+
MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
34363441
}
34373442

34383443
/*
@@ -3529,6 +3534,13 @@ IsInTransactionBlock(bool isTopLevel)
35293534
CurrentTransactionState->blockState != TBLOCK_STARTED)
35303535
return true;
35313536

3537+
/*
3538+
* If we tell the caller we're not in a transaction block, then inform
3539+
* postgres.c that it had better commit when the statement is done.
3540+
* Otherwise our report could be a lie.
3541+
*/
3542+
MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
3543+
35323544
return false;
35333545
}
35343546

‎src/backend/tcop/postgres.c

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

20852092
/*
20862093
* We must copy the sourceText and prepStmtName into MessageContext in
2087-
* case the portal is destroyed during finish_xact_command. Can avoid the
2088-
* copy if it's not an xact command, though.
2094+
* case the portal is destroyed during finish_xact_command. We do not
2095+
* make a copy of the portalParams though, preferring to just not print
2096+
* them in that case.
20892097
*/
2090-
if (is_xact_command)
2091-
{
2092-
sourceText = pstrdup(portal->sourceText);
2093-
if (portal->prepStmtName)
2094-
prepStmtName = pstrdup(portal->prepStmtName);
2095-
else
2096-
prepStmtName = "<unnamed>";
2097-
2098-
/*
2099-
* An xact command shouldn't have any parameters, which is a good
2100-
* thing because they wouldn't be around after finish_xact_command.
2101-
*/
2102-
portalParams = NULL;
2103-
}
2098+
sourceText = pstrdup(portal->sourceText);
2099+
if (portal->prepStmtName)
2100+
prepStmtName = pstrdup(portal->prepStmtName);
21042101
else
2105-
{
2106-
sourceText = portal->sourceText;
2107-
if (portal->prepStmtName)
2108-
prepStmtName = portal->prepStmtName;
2109-
else
2110-
prepStmtName = "<unnamed>";
2111-
portalParams = portal->portalParams;
2112-
}
2102+
prepStmtName = "<unnamed>";
2103+
portalParams = portal->portalParams;
21132104

21142105
/*
21152106
* Report query to various monitoring facilities.
@@ -2208,13 +2199,24 @@ exec_execute_message(const char *portal_name, long max_rows)
22082199

22092200
if (completed)
22102201
{
2211-
if (is_xact_command)
2202+
if (is_xact_command || (MyXactFlags & XACT_FLAGS_NEEDIMMEDIATECOMMIT))
22122203
{
22132204
/*
22142205
* If this was a transaction control statement, commit it. We
22152206
* will start a new xact command for the next command (if any).
2207+
* Likewise if the statement required immediate commit. Without
2208+
* this provision, we wouldn't force commit until Sync is
2209+
* received, which creates a hazard if the client tries to
2210+
* pipeline immediate-commit statements.
22162211
*/
22172212
finish_xact_command();
2213+
2214+
/*
2215+
* These commands typically don't have any parameters, and even if
2216+
* one did we couldn't print them now because the storage went
2217+
* away during finish_xact_command. So pretend there were none.
2218+
*/
2219+
portalParams = NULL;
22182220
}
22192221
else
22202222
{

‎src/include/access/xact.h

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

110+
/*
111+
* XACT_FLAGS_NEEDIMMEDIATECOMMIT - records whether the top level statement
112+
* is one that requires immediate commit, such as CREATE DATABASE.
113+
*/
114+
#define XACT_FLAGS_NEEDIMMEDIATECOMMIT (1U << 2)
115+
110116
/*
111117
* start- and end-of-transaction callbacks for dynamically loaded modules
112118
*/

0 commit comments

Comments
 (0)