From a5e517366955c1a417c8138eca7782e7db27f224 Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Wed, 31 Jul 2019 21:27:04 -0700 Subject: [PATCH v1] WIP-fix-crash-ROW_MARK_COPY. Author: Reviewed-By: Discussion: https://postgr.es/m/CAHU7rYZo_C4ULsAx_LAj8az9zqgrD8WDd4hTegDTMM1LMqrBsg@mail.gmail.com Backpatch: --- src/backend/executor/execMain.c | 30 +++++++++++++++++++----------- src/backend/executor/execScan.c | 21 +++++++++++++++++++-- src/backend/executor/execUtils.c | 1 + src/include/nodes/execnodes.h | 18 +++++++++++------- 4 files changed, 50 insertions(+), 20 deletions(-) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index dbd7dd9bcd4..3b619db691b 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -981,6 +981,7 @@ InitPlan(QueryDesc *queryDesc, int eflags) /* mark EvalPlanQual not active */ estate->es_epqTupleSlot = NULL; + estate->es_epqTupleDatum = NULL; estate->es_epqScanDone = NULL; /* @@ -2541,6 +2542,8 @@ EvalPlanQualSlot(EPQState *epqstate, TupleTableSlot **slot; Assert(rti > 0 && rti <= epqstate->estate->es_range_table_size); + Assert(relation != NULL); + slot = &epqstate->estate->es_epqTupleSlot[rti - 1]; if (*slot == NULL) @@ -2549,13 +2552,7 @@ EvalPlanQualSlot(EPQState *epqstate, oldcontext = MemoryContextSwitchTo(epqstate->estate->es_query_cxt); - if (relation) - *slot = table_slot_create(relation, - &epqstate->estate->es_tupleTable); - else - *slot = ExecAllocTableSlot(&epqstate->estate->es_tupleTable, - epqstate->origslot->tts_tupleDescriptor, - &TTSOpsVirtual); + *slot = table_slot_create(relation, &epqstate->estate->es_tupleTable); MemoryContextSwitchTo(oldcontext); } @@ -2586,9 +2583,11 @@ EvalPlanQualFetchRowMarks(EPQState *epqstate) if (RowMarkRequiresRowShareLock(erm->markType)) elog(ERROR, "EvalPlanQual doesn't support locking rowmarks"); - /* clear any leftover test tuple for this rel */ - slot = EvalPlanQualSlot(epqstate, erm->relation, erm->rti); - ExecClearTuple(slot); + /* clear any leftover data for this mark */ + epqstate->estate->es_epqTupleDatum[erm->rti - 1] = 0; + slot = epqstate->estate->es_epqTupleSlot[erm->rti - 1]; + if (slot) + ExecClearTuple(slot); /* if child rel, must check whether it produced this row */ if (erm->rti != erm->prti) @@ -2623,6 +2622,9 @@ EvalPlanQualFetchRowMarks(EPQState *epqstate) if (isNull) continue; + if (!slot) + slot = EvalPlanQualSlot(epqstate, erm->relation, erm->rti); + /* fetch requests on foreign tables must be passed to their FDW */ if (erm->relation->rd_rel->relkind == RELKIND_FOREIGN_TABLE) { @@ -2672,7 +2674,10 @@ EvalPlanQualFetchRowMarks(EPQState *epqstate) if (isNull) continue; - ExecStoreHeapTupleDatum(datum, slot); + // XXX: Do we need to care about memory lifetime here? Seems like + // not, as origslot ought to live long enough for the entire round + // of EPQ? + epqstate->estate->es_epqTupleDatum[erm->rti - 1] = datum; } } } @@ -2885,11 +2890,14 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree) if (parentestate->es_epqTupleSlot != NULL) { estate->es_epqTupleSlot = parentestate->es_epqTupleSlot; + estate->es_epqTupleDatum = parentestate->es_epqTupleDatum; } else { estate->es_epqTupleSlot = (TupleTableSlot **) palloc0(rtsize * sizeof(TupleTableSlot *)); + estate->es_epqTupleDatum = (Datum *) + palloc0(rtsize * sizeof(Datum)); } /* diff --git a/src/backend/executor/execScan.c b/src/backend/executor/execScan.c index c0e4a5376c3..1ab8c0a3bce 100644 --- a/src/backend/executor/execScan.c +++ b/src/backend/executor/execScan.c @@ -63,7 +63,8 @@ ExecScanFetch(ScanState *node, ExecClearTuple(slot); /* would not be returned by scan */ return slot; } - else if (estate->es_epqTupleSlot[scanrelid - 1] != NULL) + else if (estate->es_epqTupleSlot[scanrelid - 1] != NULL || + estate->es_epqTupleDatum[scanrelid - 1] != 0) { TupleTableSlot *slot = node->ss_ScanTupleSlot; @@ -73,7 +74,23 @@ ExecScanFetch(ScanState *node, /* Else mark to remember that we shouldn't return more */ estate->es_epqScanDone[scanrelid - 1] = true; - slot = estate->es_epqTupleSlot[scanrelid - 1]; + if (estate->es_epqTupleSlot[scanrelid - 1] != NULL) + { + slot = estate->es_epqTupleSlot[scanrelid - 1]; + } + else + { + ExecRowMark *erm = estate->es_rowmarks[scanrelid - 1]; + Datum datum; + + Assert(erm->markType == ROW_MARK_COPY); + + datum = estate->es_epqTupleDatum[scanrelid - 1]; + ExecStoreHeapTupleDatum(datum, slot); + + // XXX: Do we need to materialize this slot? I don't + // immediately see why we would. + } /* Return empty slot if we haven't got a test tuple */ if (TupIsNull(slot)) diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c index c1fc0d54e95..b1c925665f6 100644 --- a/src/backend/executor/execUtils.c +++ b/src/backend/executor/execUtils.c @@ -158,6 +158,7 @@ CreateExecutorState(void) estate->es_per_tuple_exprcontext = NULL; estate->es_epqTupleSlot = NULL; + estate->es_epqTupleDatum = NULL; estate->es_epqScanDone = NULL; estate->es_sourceText = NULL; diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 98bdcbcef5a..0811382e076 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -572,15 +572,19 @@ typedef struct EState /* * These fields are for re-evaluating plan quals when an updated tuple is - * substituted in READ COMMITTED mode. es_epqTupleSlot[] contains test - * tuples that scan plan nodes should return instead of whatever they'd - * normally return, or an empty slot if there is nothing to return; if - * es_epqTupleSlot[] is not NULL if a particular array entry is valid; and - * es_epqScanDone[] is state to remember if the tuple has been returned - * already. Arrays are of size es_range_table_size and are indexed by - * scan node scanrelid - 1. + * substituted in READ COMMITTED mode. The arrays are of size + * es_range_table_size and are indexed by scan node scanrelid - 1. + + * es_epqTupleSlot[] and es_epqTupleDatum[] contain test tuples that scan + * plan nodes should return instead of whatever they'd normally + * return. es_epqTupleDatum is used for ROW_MARK_COPY rowmarks, + * es_epqTupleSlot for everything else. If there is nothing to return the + * corresponding es_epqTupleSlot is empty, and the corresponding + * es_epqTupleDatum is 0. es_epqScanDone[] is state to remember if the + * tuple has been returned already. */ TupleTableSlot **es_epqTupleSlot; /* array of EPQ substitute tuples */ + Datum *es_epqTupleDatum; /* array of EPQ substitute tuples */ bool *es_epqScanDone; /* true if EPQ tuple has been fetched */ bool es_use_parallel_mode; /* can we use parallel workers? */ -- 2.22.0.545.g9c9b961d7e