From f9eb7c14b08d2cc5eda62ffaf37a356c05e89b93 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 8 Oct 2018 10:41:34 -0400 Subject: [PATCH] Avoid O(N^2) cost in ExecFindRowMark(). If there are many ExecRowMark structs, we spent O(N^2) time in ExecFindRowMark during executor startup. Once upon a time this was not of great concern, but the addition of native partitioning has squeezed out enough other costs that this can become the dominant overhead in some use-cases for tables with many partitions. To fix, simply replace that List data structure with an array. This adds a little bit of cost to execCurrentOf(), but not much, and anyway that code path is neither of large importance nor very efficient now. If we ever decide it is a bottleneck, constructing a hash table for lookup-by-tableoid would likely be the thing to do. Per complaint from Amit Langote, though this is different from his fix proposal. Discussion: https://postgr.es/m/468c85d9-540e-66a2-1dde-fec2b741e688@lab.ntt.co.jp --- src/backend/executor/execCurrent.c | 11 +-- src/backend/executor/execMain.c | 120 +++++++++++++++-------------- src/backend/executor/execUtils.c | 9 ++- src/include/nodes/execnodes.h | 13 ++-- 4 files changed, 84 insertions(+), 69 deletions(-) diff --git a/src/backend/executor/execCurrent.c b/src/backend/executor/execCurrent.c index 7480cf50ad..aadf749382 100644 --- a/src/backend/executor/execCurrent.c +++ b/src/backend/executor/execCurrent.c @@ -91,21 +91,22 @@ execCurrentOf(CurrentOfExpr *cexpr, * the other code can't, while the non-FOR-UPDATE case allows use of WHERE * CURRENT OF with an insensitive cursor. */ - if (queryDesc->estate->es_rowMarks) + if (queryDesc->estate->es_rowmarks) { ExecRowMark *erm; - ListCell *lc; + Index i; /* * Here, the query must have exactly one FOR UPDATE/SHARE reference to * the target table, and we dig the ctid info out of that. */ erm = NULL; - foreach(lc, queryDesc->estate->es_rowMarks) + for (i = 0; i < queryDesc->estate->es_range_table_size; i++) { - ExecRowMark *thiserm = (ExecRowMark *) lfirst(lc); + ExecRowMark *thiserm = queryDesc->estate->es_rowmarks[i]; - if (!RowMarkRequiresRowShareLock(thiserm->markType)) + if (thiserm == NULL || + !RowMarkRequiresRowShareLock(thiserm->markType)) continue; /* ignore non-FOR UPDATE/SHARE items */ if (thiserm->relid == table_oid) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index b6abad554a..0a766ef1e5 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -909,61 +909,68 @@ InitPlan(QueryDesc *queryDesc, int eflags) } /* - * Next, build the ExecRowMark list from the PlanRowMark(s), if any. + * Next, build the ExecRowMark array from the PlanRowMark(s), if any. */ - estate->es_rowMarks = NIL; - foreach(l, plannedstmt->rowMarks) + if (plannedstmt->rowMarks) { - PlanRowMark *rc = (PlanRowMark *) lfirst(l); - Oid relid; - Relation relation; - ExecRowMark *erm; - - /* ignore "parent" rowmarks; they are irrelevant at runtime */ - if (rc->isParent) - continue; - - /* get relation's OID (will produce InvalidOid if subquery) */ - relid = exec_rt_fetch(rc->rti, estate)->relid; - - /* open relation, if we need to access it for this mark type */ - switch (rc->markType) + estate->es_rowmarks = (ExecRowMark **) + palloc0(estate->es_range_table_size * sizeof(ExecRowMark *)); + foreach(l, plannedstmt->rowMarks) { - case ROW_MARK_EXCLUSIVE: - case ROW_MARK_NOKEYEXCLUSIVE: - case ROW_MARK_SHARE: - case ROW_MARK_KEYSHARE: - case ROW_MARK_REFERENCE: - relation = ExecGetRangeTableRelation(estate, rc->rti); - break; - case ROW_MARK_COPY: - /* no physical table access is required */ - relation = NULL; - break; - default: - elog(ERROR, "unrecognized markType: %d", rc->markType); - relation = NULL; /* keep compiler quiet */ - break; + PlanRowMark *rc = (PlanRowMark *) lfirst(l); + Oid relid; + Relation relation; + ExecRowMark *erm; + + /* ignore "parent" rowmarks; they are irrelevant at runtime */ + if (rc->isParent) + continue; + + /* get relation's OID (will produce InvalidOid if subquery) */ + relid = exec_rt_fetch(rc->rti, estate)->relid; + + /* open relation, if we need to access it for this mark type */ + switch (rc->markType) + { + case ROW_MARK_EXCLUSIVE: + case ROW_MARK_NOKEYEXCLUSIVE: + case ROW_MARK_SHARE: + case ROW_MARK_KEYSHARE: + case ROW_MARK_REFERENCE: + relation = ExecGetRangeTableRelation(estate, rc->rti); + break; + case ROW_MARK_COPY: + /* no physical table access is required */ + relation = NULL; + break; + default: + elog(ERROR, "unrecognized markType: %d", rc->markType); + relation = NULL; /* keep compiler quiet */ + break; + } + + /* Check that relation is a legal target for marking */ + if (relation) + CheckValidRowMarkRel(relation, rc->markType); + + erm = (ExecRowMark *) palloc(sizeof(ExecRowMark)); + erm->relation = relation; + erm->relid = relid; + erm->rti = rc->rti; + erm->prti = rc->prti; + erm->rowmarkId = rc->rowmarkId; + erm->markType = rc->markType; + erm->strength = rc->strength; + erm->waitPolicy = rc->waitPolicy; + erm->ermActive = false; + ItemPointerSetInvalid(&(erm->curCtid)); + erm->ermExtra = NULL; + + Assert(erm->rti > 0 && erm->rti <= estate->es_range_table_size && + estate->es_rowmarks[erm->rti - 1] == NULL); + + estate->es_rowmarks[erm->rti - 1] = erm; } - - /* Check that relation is a legal target for marking */ - if (relation) - CheckValidRowMarkRel(relation, rc->markType); - - erm = (ExecRowMark *) palloc(sizeof(ExecRowMark)); - erm->relation = relation; - erm->relid = relid; - erm->rti = rc->rti; - erm->prti = rc->prti; - erm->rowmarkId = rc->rowmarkId; - erm->markType = rc->markType; - erm->strength = rc->strength; - erm->waitPolicy = rc->waitPolicy; - erm->ermActive = false; - ItemPointerSetInvalid(&(erm->curCtid)); - erm->ermExtra = NULL; - - estate->es_rowMarks = lappend(estate->es_rowMarks, erm); } /* @@ -2394,13 +2401,12 @@ ExecUpdateLockMode(EState *estate, ResultRelInfo *relinfo) ExecRowMark * ExecFindRowMark(EState *estate, Index rti, bool missing_ok) { - ListCell *lc; - - foreach(lc, estate->es_rowMarks) + if (rti > 0 && rti <= estate->es_range_table_size && + estate->es_rowmarks != NULL) { - ExecRowMark *erm = (ExecRowMark *) lfirst(lc); + ExecRowMark *erm = estate->es_rowmarks[rti - 1]; - if (erm->rti == rti) + if (erm) return erm; } if (!missing_ok) @@ -3131,6 +3137,7 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree) estate->es_range_table_array = parentestate->es_range_table_array; estate->es_range_table_size = parentestate->es_range_table_size; estate->es_relations = parentestate->es_relations; + estate->es_rowmarks = parentestate->es_rowmarks; estate->es_plannedstmt = parentestate->es_plannedstmt; estate->es_junkFilter = parentestate->es_junkFilter; estate->es_output_cid = parentestate->es_output_cid; @@ -3148,7 +3155,6 @@ EvalPlanQualStart(EPQState *epqstate, EState *parentestate, Plan *planTree) } /* es_result_relation_info must NOT be copied */ /* es_trig_target_relations must NOT be copied */ - estate->es_rowMarks = parentestate->es_rowMarks; estate->es_top_eflags = parentestate->es_top_eflags; estate->es_instrument = parentestate->es_instrument; /* es_auxmodifytables must NOT be copied */ diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c index d98e90e711..71c6b5dc0a 100644 --- a/src/backend/executor/execUtils.c +++ b/src/backend/executor/execUtils.c @@ -113,6 +113,7 @@ CreateExecutorState(void) estate->es_range_table_array = NULL; estate->es_range_table_size = 0; estate->es_relations = NULL; + estate->es_rowmarks = NULL; estate->es_plannedstmt = NULL; estate->es_junkFilter = NULL; @@ -142,8 +143,6 @@ CreateExecutorState(void) estate->es_tupleTable = NIL; - estate->es_rowMarks = NIL; - estate->es_processed = 0; estate->es_lastoid = InvalidOid; @@ -709,6 +708,12 @@ ExecInitRangeTable(EState *estate, List *rangeTable) */ estate->es_relations = (Relation *) palloc0(estate->es_range_table_size * sizeof(Relation)); + + /* + * es_rowmarks is also parallel to the es_range_table_array, but it's + * allocated only if needed. + */ + estate->es_rowmarks = NULL; } /* diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 657b593663..834708944b 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -34,6 +34,7 @@ struct PlanState; /* forward references in this file */ struct ParallelHashJoinState; +struct ExecRowMark; struct ExprState; struct ExprContext; struct RangeTblEntry; /* avoid including parsenodes.h here */ @@ -491,6 +492,8 @@ typedef struct EState Index es_range_table_size; /* size of the range table arrays */ Relation *es_relations; /* Array of per-range-table-entry Relation * pointers, or NULL if not yet opened */ + struct ExecRowMark **es_rowmarks; /* Array of per-range-table-entry + * ExecRowMarks, or NULL if none */ PlannedStmt *es_plannedstmt; /* link to top of plan tree */ const char *es_sourceText; /* Source text from QueryDesc */ @@ -537,8 +540,6 @@ typedef struct EState List *es_tupleTable; /* List of TupleTableSlots */ - List *es_rowMarks; /* List of ExecRowMarks */ - uint64 es_processed; /* # of tuples processed */ Oid es_lastoid; /* last oid processed (by INSERT) */ @@ -607,7 +608,9 @@ typedef struct EState * node that sources the relation (e.g., for a foreign table the FDW can use * ermExtra to hold information). * - * EState->es_rowMarks is a list of these structs. + * EState->es_rowmarks is an array of these structs, indexed by RT index, + * with NULLs for irrelevant RT indexes. es_rowmarks itself is NULL if + * there are no rowmarks. */ typedef struct ExecRowMark { @@ -629,7 +632,7 @@ typedef struct ExecRowMark * additional runtime representation of FOR [KEY] UPDATE/SHARE clauses * * Each LockRows and ModifyTable node keeps a list of the rowmarks it needs to - * deal with. In addition to a pointer to the related entry in es_rowMarks, + * deal with. In addition to a pointer to the related entry in es_rowmarks, * this struct carries the column number(s) of the resjunk columns associated * with the rowmark (see comments for PlanRowMark for more detail). In the * case of ModifyTable, there has to be a separate ExecAuxRowMark list for @@ -638,7 +641,7 @@ typedef struct ExecRowMark */ typedef struct ExecAuxRowMark { - ExecRowMark *rowmark; /* related entry in es_rowMarks */ + ExecRowMark *rowmark; /* related entry in es_rowmarks */ AttrNumber ctidAttNo; /* resno of ctid junk attribute, if any */ AttrNumber toidAttNo; /* resno of tableoid junk attribute, if any */ AttrNumber wholeAttNo; /* resno of whole-row junk attribute, if any */