Skip to content

Commit a874fe7

Browse files
tglsfdctglsfdc
committed
Refactor the executor's API to support data-modifying CTEs better.
The originally committed patch for modifying CTEs didn't interact well with EXPLAIN, as noted by myself, and also had corner-case problems with triggers, as noted by Dean Rasheed. Those problems show it is really not practical for ExecutorEnd to call any user-defined code; so split the cleanup duties out into a new function ExecutorFinish, which must be called between the last ExecutorRun call and ExecutorEnd. Some Asserts have been added to these functions to help verify correct usage. It is no longer necessary for callers of the executor to call AfterTriggerBeginQuery/AfterTriggerEndQuery for themselves, as this is now done by ExecutorStart/ExecutorFinish respectively. If you really need to suppress that and do it for yourself, pass EXEC_FLAG_SKIP_TRIGGERS to ExecutorStart. Also, refactor portal commit processing to allow for the possibility that PortalDrop will invoke user-defined code. I think this is not actually necessary just yet, since the portal-execution-strategy logic forces any non-pure-SELECT query to be run to completion before we will consider committing. But it seems like good future-proofing.
1 parent 67a5e72 commit a874fe7

File tree

21 files changed

+350
-222
lines changed

21 files changed

+350
-222
lines changed

‎contrib/auto_explain/auto_explain.c

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ static int nesting_level = 0;
4040
/* Saved hook values in case of unload */
4141
static ExecutorStart_hook_type prev_ExecutorStart = NULL;
4242
static ExecutorRun_hook_type prev_ExecutorRun = NULL;
43+
static ExecutorFinish_hook_type prev_ExecutorFinish = NULL;
4344
static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
4445

4546
#define auto_explain_enabled() \
@@ -53,6 +54,7 @@ static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags);
5354
static void explain_ExecutorRun(QueryDesc *queryDesc,
5455
ScanDirection direction,
5556
long count);
57+
static void explain_ExecutorFinish(QueryDesc *queryDesc);
5658
static void explain_ExecutorEnd(QueryDesc *queryDesc);
5759

5860

@@ -132,6 +134,8 @@ _PG_init(void)
132134
ExecutorStart_hook = explain_ExecutorStart;
133135
prev_ExecutorRun = ExecutorRun_hook;
134136
ExecutorRun_hook = explain_ExecutorRun;
137+
prev_ExecutorFinish = ExecutorFinish_hook;
138+
ExecutorFinish_hook = explain_ExecutorFinish;
135139
prev_ExecutorEnd = ExecutorEnd_hook;
136140
ExecutorEnd_hook = explain_ExecutorEnd;
137141
}
@@ -145,6 +149,7 @@ _PG_fini(void)
145149
/* Uninstall hooks. */
146150
ExecutorStart_hook = prev_ExecutorStart;
147151
ExecutorRun_hook = prev_ExecutorRun;
152+
ExecutorFinish_hook = prev_ExecutorFinish;
148153
ExecutorEnd_hook = prev_ExecutorEnd;
149154
}
150155

@@ -211,6 +216,29 @@ explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, long count)
211216
PG_END_TRY();
212217
}
213218

219+
/*
220+
* ExecutorFinish hook: all we need do is track nesting depth
221+
*/
222+
static void
223+
explain_ExecutorFinish(QueryDesc *queryDesc)
224+
{
225+
nesting_level++;
226+
PG_TRY();
227+
{
228+
if (prev_ExecutorFinish)
229+
prev_ExecutorFinish(queryDesc);
230+
else
231+
standard_ExecutorFinish(queryDesc);
232+
nesting_level--;
233+
}
234+
PG_CATCH();
235+
{
236+
nesting_level--;
237+
PG_RE_THROW();
238+
}
239+
PG_END_TRY();
240+
}
241+
214242
/*
215243
* ExecutorEnd hook: log results if needed
216244
*/

‎contrib/pg_stat_statements/pg_stat_statements.c

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,7 @@ static int nested_level = 0;
122122
static shmem_startup_hook_type prev_shmem_startup_hook = NULL;
123123
static ExecutorStart_hook_type prev_ExecutorStart = NULL;
124124
static ExecutorRun_hook_type prev_ExecutorRun = NULL;
125+
static ExecutorFinish_hook_type prev_ExecutorFinish = NULL;
125126
static ExecutorEnd_hook_type prev_ExecutorEnd = NULL;
126127
static ProcessUtility_hook_type prev_ProcessUtility = NULL;
127128

@@ -173,6 +174,7 @@ static void pgss_ExecutorStart(QueryDesc *queryDesc, int eflags);
173174
static void pgss_ExecutorRun(QueryDesc *queryDesc,
174175
ScanDirection direction,
175176
long count);
177+
static void pgss_ExecutorFinish(QueryDesc *queryDesc);
176178
static void pgss_ExecutorEnd(QueryDesc *queryDesc);
177179
static void pgss_ProcessUtility(Node *parsetree,
178180
const char *queryString, ParamListInfo params, bool isTopLevel,
@@ -269,6 +271,8 @@ _PG_init(void)
269271
ExecutorStart_hook = pgss_ExecutorStart;
270272
prev_ExecutorRun = ExecutorRun_hook;
271273
ExecutorRun_hook = pgss_ExecutorRun;
274+
prev_ExecutorFinish = ExecutorFinish_hook;
275+
ExecutorFinish_hook = pgss_ExecutorFinish;
272276
prev_ExecutorEnd = ExecutorEnd_hook;
273277
ExecutorEnd_hook = pgss_ExecutorEnd;
274278
prev_ProcessUtility = ProcessUtility_hook;
@@ -285,6 +289,7 @@ _PG_fini(void)
285289
shmem_startup_hook = prev_shmem_startup_hook;
286290
ExecutorStart_hook = prev_ExecutorStart;
287291
ExecutorRun_hook = prev_ExecutorRun;
292+
ExecutorFinish_hook = prev_ExecutorFinish;
288293
ExecutorEnd_hook = prev_ExecutorEnd;
289294
ProcessUtility_hook = prev_ProcessUtility;
290295
}
@@ -549,6 +554,29 @@ pgss_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, long count)
549554
PG_END_TRY();
550555
}
551556

557+
/*
558+
* ExecutorFinish hook: all we need do is track nesting depth
559+
*/
560+
static void
561+
pgss_ExecutorFinish(QueryDesc *queryDesc)
562+
{
563+
nested_level++;
564+
PG_TRY();
565+
{
566+
if (prev_ExecutorFinish)
567+
prev_ExecutorFinish(queryDesc);
568+
else
569+
standard_ExecutorFinish(queryDesc);
570+
nested_level--;
571+
}
572+
PG_CATCH();
573+
{
574+
nested_level--;
575+
PG_RE_THROW();
576+
}
577+
PG_END_TRY();
578+
}
579+
552580
/*
553581
* ExecutorEnd hook: store results if needed
554582
*/

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

Lines changed: 32 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1753,12 +1753,10 @@ CommitTransaction(void)
17531753
Assert(s->parent == NULL);
17541754

17551755
/*
1756-
* Do pre-commit processing (most of this stuff requires database access,
1757-
* and in fact could still cause an error...)
1758-
*
1759-
* It is possible for CommitHoldablePortals to invoke functions that queue
1760-
* deferred triggers, and it's also possible that triggers create holdable
1761-
* cursors. So we have to loop until there's nothing left to do.
1756+
* Do pre-commit processing that involves calling user-defined code, such
1757+
* as triggers. Since closing cursors could queue trigger actions,
1758+
* triggers could open cursors, etc, we have to keep looping until there's
1759+
* nothing left to do.
17621760
*/
17631761
for (;;)
17641762
{
@@ -1768,19 +1766,23 @@ CommitTransaction(void)
17681766
AfterTriggerFireDeferred();
17691767

17701768
/*
1771-
* Convert any open holdable cursors into static portals. If there
1772-
* weren't any, we are done ... otherwise loop back to check if they
1773-
* queued deferred triggers. Lather, rinse, repeat.
1769+
* Close open portals (converting holdable ones into static portals).
1770+
* If there weren't any, we are done ... otherwise loop back to check
1771+
* if they queued deferred triggers. Lather, rinse, repeat.
17741772
*/
1775-
if (!CommitHoldablePortals())
1773+
if (!PreCommit_Portals(false))
17761774
break;
17771775
}
17781776

1779-
/* Now we can shut down the deferred-trigger manager */
1780-
AfterTriggerEndXact(true);
1777+
/*
1778+
* The remaining actions cannot call any user-defined code, so it's
1779+
* safe to start shutting down within-transaction services. But note
1780+
* that most of this stuff could still throw an error, which would
1781+
* switch us into the transaction-abort path.
1782+
*/
17811783

1782-
/* Close any open regular cursors */
1783-
AtCommit_Portals();
1784+
/* Shut down the deferred-trigger manager */
1785+
AfterTriggerEndXact(true);
17841786

17851787
/*
17861788
* Let ON COMMIT management do its thing (must happen after closing
@@ -1954,12 +1956,10 @@ PrepareTransaction(void)
19541956
Assert(s->parent == NULL);
19551957

19561958
/*
1957-
* Do pre-commit processing (most of this stuff requires database access,
1958-
* and in fact could still cause an error...)
1959-
*
1960-
* It is possible for PrepareHoldablePortals to invoke functions that
1961-
* queue deferred triggers, and it's also possible that triggers create
1962-
* holdable cursors. So we have to loop until there's nothing left to do.
1959+
* Do pre-commit processing that involves calling user-defined code, such
1960+
* as triggers. Since closing cursors could queue trigger actions,
1961+
* triggers could open cursors, etc, we have to keep looping until there's
1962+
* nothing left to do.
19631963
*/
19641964
for (;;)
19651965
{
@@ -1969,19 +1969,23 @@ PrepareTransaction(void)
19691969
AfterTriggerFireDeferred();
19701970

19711971
/*
1972-
* Convert any open holdable cursors into static portals. If there
1973-
* weren't any, we are done ... otherwise loop back to check if they
1974-
* queued deferred triggers. Lather, rinse, repeat.
1972+
* Close open portals (converting holdable ones into static portals).
1973+
* If there weren't any, we are done ... otherwise loop back to check
1974+
* if they queued deferred triggers. Lather, rinse, repeat.
19751975
*/
1976-
if (!PrepareHoldablePortals())
1976+
if (!PreCommit_Portals(true))
19771977
break;
19781978
}
19791979

1980-
/* Now we can shut down the deferred-trigger manager */
1981-
AfterTriggerEndXact(true);
1980+
/*
1981+
* The remaining actions cannot call any user-defined code, so it's
1982+
* safe to start shutting down within-transaction services. But note
1983+
* that most of this stuff could still throw an error, which would
1984+
* switch us into the transaction-abort path.
1985+
*/
19821986

1983-
/* Close any open regular cursors */
1984-
AtCommit_Portals();
1987+
/* Shut down the deferred-trigger manager */
1988+
AfterTriggerEndXact(true);
19851989

19861990
/*
19871991
* Let ON COMMIT management do its thing (must happen after closing

‎src/backend/commands/copy.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1463,6 +1463,7 @@ EndCopyTo(CopyState cstate)
14631463
if (cstate->queryDesc != NULL)
14641464
{
14651465
/* Close down the query and free resources. */
1466+
ExecutorFinish(cstate->queryDesc);
14661467
ExecutorEnd(cstate->queryDesc);
14671468
FreeQueryDesc(cstate->queryDesc);
14681469
PopActiveSnapshot();

‎src/backend/commands/discard.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,10 +61,11 @@ DiscardAll(bool isTopLevel)
6161
*/
6262
PreventTransactionChain(isTopLevel, "DISCARD ALL");
6363

64+
/* Closing portals might run user-defined code, so do that first. */
65+
PortalHashTableDeleteAll();
6466
SetPGVariable("session_authorization", NIL, false);
6567
ResetAllOptions();
6668
DropAllPreparedStatements();
67-
PortalHashTableDeleteAll();
6869
Async_UnlistenAll();
6970
LockReleaseAll(USER_LOCKMETHOD, true);
7071
ResetPlanCache();

‎src/backend/commands/explain.c

Lines changed: 6 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -360,6 +360,8 @@ ExplainOnePlan(PlannedStmt *plannedstmt, ExplainState *es,
360360
if (es->buffers)
361361
instrument_option |= INSTRUMENT_BUFFERS;
362362

363+
INSTR_TIME_SET_CURRENT(starttime);
364+
363365
/*
364366
* Use a snapshot with an updated command ID to ensure this query sees
365367
* results of any previously executed queries.
@@ -371,12 +373,6 @@ ExplainOnePlan(PlannedStmt *plannedstmt, ExplainState *es,
371373
GetActiveSnapshot(), InvalidSnapshot,
372374
None_Receiver, params, instrument_option);
373375

374-
INSTR_TIME_SET_CURRENT(starttime);
375-
376-
/* If analyzing, we need to cope with queued triggers */
377-
if (es->analyze)
378-
AfterTriggerBeginQuery();
379-
380376
/* Select execution options */
381377
if (es->analyze)
382378
eflags = 0; /* default run-to-completion flags */
@@ -392,7 +388,10 @@ ExplainOnePlan(PlannedStmt *plannedstmt, ExplainState *es,
392388
/* run the plan */
393389
ExecutorRun(queryDesc, ForwardScanDirection, 0L);
394390

395-
/* We can't clean up 'till we're done printing the stats... */
391+
/* run cleanup too */
392+
ExecutorFinish(queryDesc);
393+
394+
/* We can't run ExecutorEnd 'till we're done printing the stats... */
396395
totaltime += elapsed_time(&starttime);
397396
}
398397

@@ -401,18 +400,6 @@ ExplainOnePlan(PlannedStmt *plannedstmt, ExplainState *es,
401400
/* Create textual dump of plan tree */
402401
ExplainPrintPlan(es, queryDesc);
403402

404-
/*
405-
* If we ran the command, run any AFTER triggers it queued. (Note this
406-
* will not include DEFERRED triggers; since those don't run until end of
407-
* transaction, we can't measure them.) Include into total runtime.
408-
*/
409-
if (es->analyze)
410-
{
411-
INSTR_TIME_SET_CURRENT(starttime);
412-
AfterTriggerEndQuery(queryDesc->estate);
413-
totaltime += elapsed_time(&starttime);
414-
}
415-
416403
/* Print info about runtime of triggers */
417404
if (es->analyze)
418405
{

‎src/backend/commands/extension.c

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -726,10 +726,9 @@ execute_sql_string(const char *sql, const char *filename)
726726
GetActiveSnapshot(), NULL,
727727
dest, NULL, 0);
728728

729-
AfterTriggerBeginQuery();
730729
ExecutorStart(qdesc, 0);
731730
ExecutorRun(qdesc, ForwardScanDirection, 0);
732-
AfterTriggerEndQuery(qdesc->estate);
731+
ExecutorFinish(qdesc);
733732
ExecutorEnd(qdesc);
734733

735734
FreeQueryDesc(qdesc);

‎src/backend/commands/portalcmds.c

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -254,7 +254,14 @@ PortalCleanup(Portal portal)
254254
queryDesc = PortalGetQueryDesc(portal);
255255
if (queryDesc)
256256
{
257+
/*
258+
* Reset the queryDesc before anything else. This prevents us
259+
* from trying to shut down the executor twice, in case of an
260+
* error below. The transaction abort mechanisms will take care
261+
* of resource cleanup in such a case.
262+
*/
257263
portal->queryDesc = NULL;
264+
258265
if (portal->status != PORTAL_FAILED)
259266
{
260267
ResourceOwner saveResourceOwner;
@@ -264,7 +271,7 @@ PortalCleanup(Portal portal)
264271
PG_TRY();
265272
{
266273
CurrentResourceOwner = portal->resowner;
267-
/* we do not need AfterTriggerEndQuery() here */
274+
ExecutorFinish(queryDesc);
268275
ExecutorEnd(queryDesc);
269276
FreeQueryDesc(queryDesc);
270277
}
@@ -371,7 +378,7 @@ PersistHoldablePortal(Portal portal)
371378
* Now shut down the inner executor.
372379
*/
373380
portal->queryDesc = NULL; /* prevent double shutdown */
374-
/* we do not need AfterTriggerEndQuery() here */
381+
ExecutorFinish(queryDesc);
375382
ExecutorEnd(queryDesc);
376383
FreeQueryDesc(queryDesc);
377384

‎src/backend/commands/trigger.c

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3676,9 +3676,9 @@ AfterTriggerBeginQuery(void)
36763676
* we invoke all AFTER IMMEDIATE trigger events queued by the query, and
36773677
* transfer deferred trigger events to the global deferred-trigger list.
36783678
*
3679-
* Note that this should be called just BEFORE closing down the executor
3679+
* Note that this must be called BEFORE closing down the executor
36803680
* with ExecutorEnd, because we make use of the EState's info about
3681-
* target relations.
3681+
* target relations. Normally it is called from ExecutorFinish.
36823682
* ----------
36833683
*/
36843684
void

‎src/backend/executor/README

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,12 +95,17 @@ This is a sketch of control flow for full query processing:
9595
CreateExprContext
9696
creates per-tuple context
9797
ExecInitExpr
98+
AfterTriggerBeginQuery
9899

99100
ExecutorRun
100101
ExecProcNode --- recursively called in per-query context
101102
ExecEvalExpr --- called in per-tuple context
102103
ResetExprContext --- to free memory
103104

105+
ExecutorFinish
106+
ExecPostprocessPlan --- run any unfinished ModifyTable nodes
107+
AfterTriggerEndQuery
108+
104109
ExecutorEnd
105110
ExecEndNode --- recursively releases resources
106111
FreeExecutorState

0 commit comments

Comments
 (0)