Skip to content

Commit f929441

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 3cabe45 commit f929441

File tree

4 files changed

+104
-26
lines changed

4 files changed

+104
-26
lines changed

‎doc/src/sgml/protocol.sgml

+58
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

+13-1
Original file line numberDiff line numberDiff line change
@@ -3453,6 +3453,9 @@ AbortCurrentTransaction(void)
34533453
* could issue more commands and possibly cause a failure after the statement
34543454
* completes). Subtransactions are verboten too.
34553455
*
3456+
* We must also set XACT_FLAGS_NEEDIMMEDIATECOMMIT in MyXactFlags, to ensure
3457+
* that postgres.c follows through by committing after the statement is done.
3458+
*
34563459
* isTopLevel: passed down from ProcessUtility to determine whether we are
34573460
* inside a function. (We will always fail if this is false, but it's
34583461
* convenient to centralize the check here instead of making callers do it.)
@@ -3494,7 +3497,9 @@ PreventInTransactionBlock(bool isTopLevel, const char *stmtType)
34943497
if (CurrentTransactionState->blockState != TBLOCK_DEFAULT &&
34953498
CurrentTransactionState->blockState != TBLOCK_STARTED)
34963499
elog(FATAL, "cannot prevent transaction chain");
3497-
/* all okay */
3500+
3501+
/* All okay. Set the flag to make sure the right thing happens later. */
3502+
MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
34983503
}
34993504

35003505
/*
@@ -3591,6 +3596,13 @@ IsInTransactionBlock(bool isTopLevel)
35913596
CurrentTransactionState->blockState != TBLOCK_STARTED)
35923597
return true;
35933598

3599+
/*
3600+
* If we tell the caller we're not in a transaction block, then inform
3601+
* postgres.c that it had better commit when the statement is done.
3602+
* Otherwise our report could be a lie.
3603+
*/
3604+
MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
3605+
35943606
return false;
35953607
}
35963608

‎src/backend/tcop/postgres.c

+27-25
Original file line numberDiff line numberDiff line change
@@ -1277,6 +1277,13 @@ exec_simple_query(const char *query_string)
12771277
}
12781278
else
12791279
{
1280+
/*
1281+
* We had better not see XACT_FLAGS_NEEDIMMEDIATECOMMIT set if
1282+
* we're not calling finish_xact_command(). (The implicit
1283+
* transaction block should have prevented it from getting set.)
1284+
*/
1285+
Assert(!(MyXactFlags & XACT_FLAGS_NEEDIMMEDIATECOMMIT));
1286+
12801287
/*
12811288
* We need a CommandCounterIncrement after every query, except
12821289
* those that start or end a transaction block.
@@ -2092,32 +2099,16 @@ exec_execute_message(const char *portal_name, long max_rows)
20922099

20932100
/*
20942101
* We must copy the sourceText and prepStmtName into MessageContext in
2095-
* case the portal is destroyed during finish_xact_command. Can avoid the
2096-
* copy if it's not an xact command, though.
2102+
* case the portal is destroyed during finish_xact_command. We do not
2103+
* make a copy of the portalParams though, preferring to just not print
2104+
* them in that case.
20972105
*/
2098-
if (is_xact_command)
2099-
{
2100-
sourceText = pstrdup(portal->sourceText);
2101-
if (portal->prepStmtName)
2102-
prepStmtName = pstrdup(portal->prepStmtName);
2103-
else
2104-
prepStmtName = "<unnamed>";
2105-
2106-
/*
2107-
* An xact command shouldn't have any parameters, which is a good
2108-
* thing because they wouldn't be around after finish_xact_command.
2109-
*/
2110-
portalParams = NULL;
2111-
}
2106+
sourceText = pstrdup(portal->sourceText);
2107+
if (portal->prepStmtName)
2108+
prepStmtName = pstrdup(portal->prepStmtName);
21122109
else
2113-
{
2114-
sourceText = portal->sourceText;
2115-
if (portal->prepStmtName)
2116-
prepStmtName = portal->prepStmtName;
2117-
else
2118-
prepStmtName = "<unnamed>";
2119-
portalParams = portal->portalParams;
2120-
}
2110+
prepStmtName = "<unnamed>";
2111+
portalParams = portal->portalParams;
21212112

21222113
/*
21232114
* Report query to various monitoring facilities.
@@ -2216,13 +2207,24 @@ exec_execute_message(const char *portal_name, long max_rows)
22162207

22172208
if (completed)
22182209
{
2219-
if (is_xact_command)
2210+
if (is_xact_command || (MyXactFlags & XACT_FLAGS_NEEDIMMEDIATECOMMIT))
22202211
{
22212212
/*
22222213
* If this was a transaction control statement, commit it. We
22232214
* will start a new xact command for the next command (if any).
2215+
* Likewise if the statement required immediate commit. Without
2216+
* this provision, we wouldn't force commit until Sync is
2217+
* received, which creates a hazard if the client tries to
2218+
* pipeline immediate-commit statements.
22242219
*/
22252220
finish_xact_command();
2221+
2222+
/*
2223+
* These commands typically don't have any parameters, and even if
2224+
* one did we couldn't print them now because the storage went
2225+
* away during finish_xact_command. So pretend there were none.
2226+
*/
2227+
portalParams = NULL;
22262228
}
22272229
else
22282230
{

‎src/include/access/xact.h

+6
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,12 @@ extern PGDLLIMPORT 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)