Andres Freund <and...@anarazel.de> writes: > On 2017-07-26 16:28:38 -0400, Tom Lane wrote: >> It's certainly possible that there are long-running loops not involving >> any ExecProcNode recursion at all, but that would be a bug independent >> of this issue. The CFI in ExecProcNode itself can be replaced exactly >> either by asking all callers to do it, or by asking all callees to do it. >> I think the latter is going to be more uniform and harder to screw up.
> Looks a bit better. Still a lot of judgement-y calls tho, e.g. when one > node function just calls the next, or when there's loops etc. I found > a good number of missing CFIs... > What do you think? Here's a reviewed version of this patch. Differences from yours: * I think you put ExecScan's CFI in the wrong place; AFAICT yours only covers its fast path. * I think ExecAgg needs a CFI at the head, just to be sure it's hit in any path through that. * I agree that putting CFI inside ExecHashJoin's state machine loop is a good idea, because it might have to trawl through quite a lot of a batch file before finding a returnable tuple. But I think in merge and nestloop joins it's sufficient to put one CFI at the head. Neither of those cases can do very much processing without invoking a child node, where a CFI will happen. * You missed ExecLockRows altogether. * I added some comments and cosmetic tweaks. regards, tom lane
diff --git a/src/backend/executor/execProcnode.c b/src/backend/executor/execProcnode.c index 294ad2c..20fd9f8 100644 --- a/src/backend/executor/execProcnode.c +++ b/src/backend/executor/execProcnode.c @@ -399,8 +399,6 @@ ExecProcNode(PlanState *node) { TupleTableSlot *result; - CHECK_FOR_INTERRUPTS(); - if (node->chgParam != NULL) /* something changed */ ExecReScan(node); /* let ReScan handle this */ diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c index 4f131b3..6fde7cd 100644 --- a/src/backend/executor/execScan.c +++ b/src/backend/executor/execScan.c @@ -126,6 +126,8 @@ ExecScan(ScanState *node, ExprState *qual; ProjectionInfo *projInfo; + CHECK_FOR_INTERRUPTS(); + /* * Fetch data from node */ diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index de9a18e..377916d 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -677,6 +677,8 @@ fetch_input_tuple(AggState *aggstate) if (aggstate->sort_in) { + /* make sure we check for interrupts in either path through here */ + CHECK_FOR_INTERRUPTS(); if (!tuplesort_gettupleslot(aggstate->sort_in, true, false, aggstate->sort_slot, NULL)) return NULL; @@ -1414,6 +1416,8 @@ process_ordered_aggregate_multi(AggState *aggstate, while (tuplesort_gettupleslot(pertrans->sortstates[aggstate->current_set], true, true, slot1, &newAbbrevVal)) { + CHECK_FOR_INTERRUPTS(); + /* * Extract the first numTransInputs columns as datums to pass to the * transfn. (This will help execTuplesMatch too, so we do it @@ -2100,6 +2104,8 @@ ExecAgg(AggState *node) { TupleTableSlot *result = NULL; + CHECK_FOR_INTERRUPTS(); + if (!node->agg_done) { /* Dispatch based on strategy */ @@ -2563,6 +2569,8 @@ agg_retrieve_hash_table(AggState *aggstate) TupleTableSlot *hashslot = perhash->hashslot; int i; + CHECK_FOR_INTERRUPTS(); + /* * Find the next entry in the hash table */ diff --git a/src/backend/executor/nodeAppend.c b/src/backend/executor/nodeAppend.c index aae5e3f..58045e0 100644 --- a/src/backend/executor/nodeAppend.c +++ b/src/backend/executor/nodeAppend.c @@ -59,6 +59,7 @@ #include "executor/execdebug.h" #include "executor/nodeAppend.h" +#include "miscadmin.h" static bool exec_append_initialize_next(AppendState *appendstate); @@ -204,6 +205,8 @@ ExecAppend(AppendState *node) PlanState *subnode; TupleTableSlot *result; + CHECK_FOR_INTERRUPTS(); + /* * figure out which subplan we are currently processing */ diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c index 7e0ba03..cf109d5 100644 --- a/src/backend/executor/nodeBitmapHeapscan.c +++ b/src/backend/executor/nodeBitmapHeapscan.c @@ -41,6 +41,7 @@ #include "access/transam.h" #include "executor/execdebug.h" #include "executor/nodeBitmapHeapscan.h" +#include "miscadmin.h" #include "pgstat.h" #include "storage/bufmgr.h" #include "storage/predicate.h" @@ -192,6 +193,8 @@ BitmapHeapNext(BitmapHeapScanState *node) Page dp; ItemId lp; + CHECK_FOR_INTERRUPTS(); + /* * Get next page of results if needed */ diff --git a/src/backend/executor/nodeCustom.c b/src/backend/executor/nodeCustom.c index 69e2704..fc15974 100644 --- a/src/backend/executor/nodeCustom.c +++ b/src/backend/executor/nodeCustom.c @@ -15,6 +15,7 @@ #include "executor/nodeCustom.h" #include "nodes/execnodes.h" #include "nodes/plannodes.h" +#include "miscadmin.h" #include "parser/parsetree.h" #include "utils/hsearch.h" #include "utils/memutils.h" @@ -104,6 +105,8 @@ ExecInitCustomScan(CustomScan *cscan, EState *estate, int eflags) TupleTableSlot * ExecCustomScan(CustomScanState *node) { + CHECK_FOR_INTERRUPTS(); + Assert(node->methods->ExecCustomScan != NULL); return node->methods->ExecCustomScan(node); } diff --git a/src/backend/executor/nodeGather.c b/src/backend/executor/nodeGather.c index f83cd58..5dbe19c 100644 --- a/src/backend/executor/nodeGather.c +++ b/src/backend/executor/nodeGather.c @@ -128,6 +128,8 @@ ExecGather(GatherState *node) TupleTableSlot *slot; ExprContext *econtext; + CHECK_FOR_INTERRUPTS(); + /* * Initialize the parallel context and workers on first execution. We do * this on first execution rather than during node initialization, as it @@ -247,6 +249,8 @@ gather_getnext(GatherState *gatherstate) while (gatherstate->reader != NULL || gatherstate->need_to_scan_locally) { + CHECK_FOR_INTERRUPTS(); + if (gatherstate->reader != NULL) { MemoryContext oldContext; diff --git a/src/backend/executor/nodeGatherMerge.c b/src/backend/executor/nodeGatherMerge.c index 80ee1fc..0aff379 100644 --- a/src/backend/executor/nodeGatherMerge.c +++ b/src/backend/executor/nodeGatherMerge.c @@ -164,6 +164,8 @@ ExecGatherMerge(GatherMergeState *node) ExprContext *econtext; int i; + CHECK_FOR_INTERRUPTS(); + /* * As with Gather, we don't launch workers until this node is actually * executed. @@ -393,6 +395,8 @@ gather_merge_init(GatherMergeState *gm_state) reread: for (i = 0; i < nreaders + 1; i++) { + CHECK_FOR_INTERRUPTS(); + if (!gm_state->gm_tuple_buffers[i].done && (TupIsNull(gm_state->gm_slots[i]) || gm_state->gm_slots[i]->tts_isempty)) diff --git a/src/backend/executor/nodeGroup.c b/src/backend/executor/nodeGroup.c index af9ba49..fc5e0e5 100644 --- a/src/backend/executor/nodeGroup.c +++ b/src/backend/executor/nodeGroup.c @@ -24,6 +24,7 @@ #include "executor/executor.h" #include "executor/nodeGroup.h" +#include "miscadmin.h" /* @@ -40,6 +41,8 @@ ExecGroup(GroupState *node) TupleTableSlot *firsttupleslot; TupleTableSlot *outerslot; + CHECK_FOR_INTERRUPTS(); + /* * get state info from node */ diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c index 075f4ed..fbeb562 100644 --- a/src/backend/executor/nodeHash.c +++ b/src/backend/executor/nodeHash.c @@ -810,6 +810,9 @@ ExecHashIncreaseNumBuckets(HashJoinTable hashtable) idx += MAXALIGN(HJTUPLE_OVERHEAD + HJTUPLE_MINTUPLE(hashTuple)->t_len); } + + /* allow this loop to be cancellable */ + CHECK_FOR_INTERRUPTS(); } } @@ -1192,6 +1195,9 @@ ExecScanHashTableForUnmatched(HashJoinState *hjstate, ExprContext *econtext) hashTuple = hashTuple->next; } + + /* allow this loop to be cancellable */ + CHECK_FOR_INTERRUPTS(); } /* diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c index 668ed87..252960c 100644 --- a/src/backend/executor/nodeHashjoin.c +++ b/src/backend/executor/nodeHashjoin.c @@ -92,6 +92,14 @@ ExecHashJoin(HashJoinState *node) */ for (;;) { + /* + * It's possible to iterate this loop many times before returning a + * tuple, in some pathological cases such as needing to move much of + * the current batch to a later batch. So let's check for interrupts + * each time through. + */ + CHECK_FOR_INTERRUPTS(); + switch (node->hj_JoinState) { case HJ_BUILD_HASHTABLE: @@ -247,13 +255,6 @@ ExecHashJoin(HashJoinState *node) case HJ_SCAN_BUCKET: /* - * We check for interrupts here because this corresponds to - * where we'd fetch a row from a child plan node in other join - * types. - */ - CHECK_FOR_INTERRUPTS(); - - /* * Scan the selected hash bucket for matches to current outer */ if (!ExecScanHashBucket(node, econtext)) diff --git a/src/backend/executor/nodeIndexonlyscan.c b/src/backend/executor/nodeIndexonlyscan.c index 890e544..e2000764 100644 --- a/src/backend/executor/nodeIndexonlyscan.c +++ b/src/backend/executor/nodeIndexonlyscan.c @@ -34,6 +34,7 @@ #include "executor/execdebug.h" #include "executor/nodeIndexonlyscan.h" #include "executor/nodeIndexscan.h" +#include "miscadmin.h" #include "storage/bufmgr.h" #include "storage/predicate.h" #include "utils/memutils.h" @@ -117,6 +118,8 @@ IndexOnlyNext(IndexOnlyScanState *node) { HeapTuple tuple = NULL; + CHECK_FOR_INTERRUPTS(); + /* * We can skip the heap fetch if the TID references a heap page on * which all tuples are known visible to everybody. In any case, diff --git a/src/backend/executor/nodeIndexscan.c b/src/backend/executor/nodeIndexscan.c index 75b1011..6704ede 100644 --- a/src/backend/executor/nodeIndexscan.c +++ b/src/backend/executor/nodeIndexscan.c @@ -34,6 +34,7 @@ #include "executor/execdebug.h" #include "executor/nodeIndexscan.h" #include "lib/pairingheap.h" +#include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "optimizer/clauses.h" #include "utils/array.h" @@ -131,6 +132,8 @@ IndexNext(IndexScanState *node) */ while ((tuple = index_getnext(scandesc, direction)) != NULL) { + CHECK_FOR_INTERRUPTS(); + /* * Store the scanned tuple in the scan tuple slot of the scan state. * Note: we pass 'false' because tuples returned by amgetnext are @@ -233,6 +236,8 @@ IndexNextWithReorder(IndexScanState *node) for (;;) { + CHECK_FOR_INTERRUPTS(); + /* * Check the reorder queue first. If the topmost tuple in the queue * has an ORDER BY value smaller than (or equal to) the value last @@ -299,6 +304,8 @@ next_indextuple: { /* Fails recheck, so drop it and loop back for another */ InstrCountFiltered2(node, 1); + /* allow this loop to be cancellable */ + CHECK_FOR_INTERRUPTS(); goto next_indextuple; } } diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c index abd060d..2ed3523 100644 --- a/src/backend/executor/nodeLimit.c +++ b/src/backend/executor/nodeLimit.c @@ -23,6 +23,7 @@ #include "executor/executor.h" #include "executor/nodeLimit.h" +#include "miscadmin.h" #include "nodes/nodeFuncs.h" static void recompute_limits(LimitState *node); @@ -43,6 +44,8 @@ ExecLimit(LimitState *node) TupleTableSlot *slot; PlanState *outerPlan; + CHECK_FOR_INTERRUPTS(); + /* * get information from the node */ diff --git a/src/backend/executor/nodeLockRows.c b/src/backend/executor/nodeLockRows.c index f519794..dd4e2c5 100644 --- a/src/backend/executor/nodeLockRows.c +++ b/src/backend/executor/nodeLockRows.c @@ -26,6 +26,7 @@ #include "executor/executor.h" #include "executor/nodeLockRows.h" #include "foreign/fdwapi.h" +#include "miscadmin.h" #include "storage/bufmgr.h" #include "utils/rel.h" #include "utils/tqual.h" @@ -44,6 +45,8 @@ ExecLockRows(LockRowsState *node) bool epq_needed; ListCell *lc; + CHECK_FOR_INTERRUPTS(); + /* * get information from the node */ diff --git a/src/backend/executor/nodeMaterial.c b/src/backend/executor/nodeMaterial.c index 32b7269..3342949 100644 --- a/src/backend/executor/nodeMaterial.c +++ b/src/backend/executor/nodeMaterial.c @@ -45,6 +45,8 @@ ExecMaterial(MaterialState *node) bool eof_tuplestore; TupleTableSlot *slot; + CHECK_FOR_INTERRUPTS(); + /* * get state info from node */ diff --git a/src/backend/executor/nodeMergeAppend.c b/src/backend/executor/nodeMergeAppend.c index fef83db..d41def1 100644 --- a/src/backend/executor/nodeMergeAppend.c +++ b/src/backend/executor/nodeMergeAppend.c @@ -40,8 +40,8 @@ #include "executor/execdebug.h" #include "executor/nodeMergeAppend.h" - #include "lib/binaryheap.h" +#include "miscadmin.h" /* * We have one slot for each item in the heap array. We use SlotNumber @@ -175,6 +175,8 @@ ExecMergeAppend(MergeAppendState *node) TupleTableSlot *result; SlotNumber i; + CHECK_FOR_INTERRUPTS(); + if (!node->ms_initialized) { /* diff --git a/src/backend/executor/nodeMergejoin.c b/src/backend/executor/nodeMergejoin.c index 6a145ee..324b61b 100644 --- a/src/backend/executor/nodeMergejoin.c +++ b/src/backend/executor/nodeMergejoin.c @@ -95,6 +95,7 @@ #include "access/nbtree.h" #include "executor/execdebug.h" #include "executor/nodeMergejoin.h" +#include "miscadmin.h" #include "utils/lsyscache.h" #include "utils/memutils.h" @@ -610,6 +611,8 @@ ExecMergeJoin(MergeJoinState *node) bool doFillOuter; bool doFillInner; + CHECK_FOR_INTERRUPTS(); + /* * get information from node */ diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 77ba15d..637a582 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -1551,6 +1551,8 @@ ExecModifyTable(ModifyTableState *node) HeapTupleData oldtupdata; HeapTuple oldtuple; + CHECK_FOR_INTERRUPTS(); + /* * This should NOT get called during EvalPlanQual; we should have passed a * subplan tree to EvalPlanQual, instead. Use a runtime test not just diff --git a/src/backend/executor/nodeNestloop.c b/src/backend/executor/nodeNestloop.c index 0065fe6..bedc374 100644 --- a/src/backend/executor/nodeNestloop.c +++ b/src/backend/executor/nodeNestloop.c @@ -23,6 +23,7 @@ #include "executor/execdebug.h" #include "executor/nodeNestloop.h" +#include "miscadmin.h" #include "utils/memutils.h" @@ -69,6 +70,8 @@ ExecNestLoop(NestLoopState *node) ExprContext *econtext; ListCell *lc; + CHECK_FOR_INTERRUPTS(); + /* * get information from the node */ diff --git a/src/backend/executor/nodeProjectSet.c b/src/backend/executor/nodeProjectSet.c index 01048cc..3b69c7a 100644 --- a/src/backend/executor/nodeProjectSet.c +++ b/src/backend/executor/nodeProjectSet.c @@ -24,6 +24,7 @@ #include "executor/executor.h" #include "executor/nodeProjectSet.h" +#include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "utils/memutils.h" @@ -46,6 +47,8 @@ ExecProjectSet(ProjectSetState *node) PlanState *outerPlan; ExprContext *econtext; + CHECK_FOR_INTERRUPTS(); + econtext = node->ps.ps_ExprContext; /* diff --git a/src/backend/executor/nodeRecursiveunion.c b/src/backend/executor/nodeRecursiveunion.c index fc1c00d..2802fff 100644 --- a/src/backend/executor/nodeRecursiveunion.c +++ b/src/backend/executor/nodeRecursiveunion.c @@ -75,6 +75,8 @@ ExecRecursiveUnion(RecursiveUnionState *node) TupleTableSlot *slot; bool isnew; + CHECK_FOR_INTERRUPTS(); + /* 1. Evaluate non-recursive term */ if (!node->recursing) { diff --git a/src/backend/executor/nodeResult.c b/src/backend/executor/nodeResult.c index a753a53..f007f46 100644 --- a/src/backend/executor/nodeResult.c +++ b/src/backend/executor/nodeResult.c @@ -47,6 +47,7 @@ #include "executor/executor.h" #include "executor/nodeResult.h" +#include "miscadmin.h" #include "utils/memutils.h" @@ -70,6 +71,8 @@ ExecResult(ResultState *node) PlanState *outerPlan; ExprContext *econtext; + CHECK_FOR_INTERRUPTS(); + econtext = node->ps.ps_ExprContext; /* diff --git a/src/backend/executor/nodeSetOp.c b/src/backend/executor/nodeSetOp.c index 9c7812e..56c5643 100644 --- a/src/backend/executor/nodeSetOp.c +++ b/src/backend/executor/nodeSetOp.c @@ -47,6 +47,7 @@ #include "access/htup_details.h" #include "executor/executor.h" #include "executor/nodeSetOp.h" +#include "miscadmin.h" #include "utils/memutils.h" @@ -185,6 +186,8 @@ ExecSetOp(SetOpState *node) SetOp *plannode = (SetOp *) node->ps.plan; TupleTableSlot *resultTupleSlot = node->ps.ps_ResultTupleSlot; + CHECK_FOR_INTERRUPTS(); + /* * If the previously-returned tuple needs to be returned more than once, * keep returning it. @@ -428,6 +431,8 @@ setop_retrieve_hash_table(SetOpState *setopstate) */ while (!setopstate->setop_done) { + CHECK_FOR_INTERRUPTS(); + /* * Find the next entry in the hash table */ diff --git a/src/backend/executor/nodeSort.c b/src/backend/executor/nodeSort.c index 924b458..799a4e9 100644 --- a/src/backend/executor/nodeSort.c +++ b/src/backend/executor/nodeSort.c @@ -43,6 +43,8 @@ ExecSort(SortState *node) Tuplesortstate *tuplesortstate; TupleTableSlot *slot; + CHECK_FOR_INTERRUPTS(); + /* * get state info from node */ diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c index e8fa4c8..fe10e80 100644 --- a/src/backend/executor/nodeSubplan.c +++ b/src/backend/executor/nodeSubplan.c @@ -33,6 +33,7 @@ #include "executor/executor.h" #include "executor/nodeSubplan.h" #include "nodes/makefuncs.h" +#include "miscadmin.h" #include "optimizer/clauses.h" #include "utils/array.h" #include "utils/lsyscache.h" @@ -65,6 +66,8 @@ ExecSubPlan(SubPlanState *node, { SubPlan *subplan = node->subplan; + CHECK_FOR_INTERRUPTS(); + /* Set non-null as default */ *isNull = false; @@ -618,6 +621,8 @@ findPartialMatch(TupleHashTable hashtable, TupleTableSlot *slot, InitTupleHashIterator(hashtable, &hashiter); while ((entry = ScanTupleHashTable(hashtable, &hashiter)) != NULL) { + CHECK_FOR_INTERRUPTS(); + ExecStoreMinimalTuple(entry->firstTuple, hashtable->tableslot, false); if (!execTuplesUnequal(slot, hashtable->tableslot, numCols, keyColIdx, diff --git a/src/backend/executor/nodeTableFuncscan.c b/src/backend/executor/nodeTableFuncscan.c index bb016ec..2859363 100644 --- a/src/backend/executor/nodeTableFuncscan.c +++ b/src/backend/executor/nodeTableFuncscan.c @@ -440,6 +440,8 @@ tfuncLoadRows(TableFuncScanState *tstate, ExprContext *econtext) ListCell *cell = list_head(tstate->coldefexprs); int colno; + CHECK_FOR_INTERRUPTS(); + ExecClearTuple(tstate->ss.ss_ScanTupleSlot); /* diff --git a/src/backend/executor/nodeTidscan.c b/src/backend/executor/nodeTidscan.c index 96af2d2..c122473 100644 --- a/src/backend/executor/nodeTidscan.c +++ b/src/backend/executor/nodeTidscan.c @@ -26,6 +26,7 @@ #include "catalog/pg_type.h" #include "executor/execdebug.h" #include "executor/nodeTidscan.h" +#include "miscadmin.h" #include "optimizer/clauses.h" #include "storage/bufmgr.h" #include "utils/array.h" @@ -400,6 +401,8 @@ TidNext(TidScanState *node) node->tss_TidPtr--; else node->tss_TidPtr++; + + CHECK_FOR_INTERRUPTS(); } /* diff --git a/src/backend/executor/nodeUnique.c b/src/backend/executor/nodeUnique.c index 28cc1e9..db78c88 100644 --- a/src/backend/executor/nodeUnique.c +++ b/src/backend/executor/nodeUnique.c @@ -35,6 +35,7 @@ #include "executor/executor.h" #include "executor/nodeUnique.h" +#include "miscadmin.h" #include "utils/memutils.h" @@ -50,6 +51,8 @@ ExecUnique(UniqueState *node) TupleTableSlot *slot; PlanState *outerPlan; + CHECK_FOR_INTERRUPTS(); + /* * get information from the node */ diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c index 8f13fe0..9da35ac 100644 --- a/src/backend/executor/nodeWindowAgg.c +++ b/src/backend/executor/nodeWindowAgg.c @@ -1594,6 +1594,8 @@ ExecWindowAgg(WindowAggState *winstate) int i; int numfuncs; + CHECK_FOR_INTERRUPTS(); + if (winstate->all_done) return NULL; @@ -2371,6 +2373,9 @@ window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot) WindowAggState *winstate = winobj->winstate; MemoryContext oldcontext; + /* often called repeatedly in a row */ + CHECK_FOR_INTERRUPTS(); + /* Don't allow passing -1 to spool_tuples here */ if (pos < 0) return false;
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers