From 5fe31cfc153ce2b62e2e672e67191c3c17eee35b Mon Sep 17 00:00:00 2001 From: Alexander Korotkov Date: Sat, 17 Jun 2023 22:55:32 +0300 Subject: [PATCH 7/8] Introduce RowRefType, which describes the table row identifier Currently, the table row could be identified by the ctid or the whole row (foreign table). But the row identifier is mixed together with lock mode in RowMarkType. This commit separates row identifier type into separate enum RowRefType. --- contrib/postgres_fdw/postgres_fdw.c | 2 +- doc/src/sgml/fdwhandler.sgml | 22 ++++++++---- src/backend/executor/execMain.c | 35 ++++++++++++-------- src/backend/optimizer/plan/planner.c | 33 +++++++++++------- src/backend/optimizer/prep/preptlist.c | 4 +-- src/backend/optimizer/util/inherit.c | 27 +++++++-------- src/include/foreign/fdwapi.h | 3 +- src/include/nodes/execnodes.h | 4 +++ src/include/nodes/plannodes.h | 46 ++++++++++++++++---------- src/include/optimizer/planner.h | 3 +- src/tools/pgindent/typedefs.list | 1 + 11 files changed, 113 insertions(+), 67 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index 142dcfc9957..b0000790292 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -7636,7 +7636,7 @@ make_tuple_from_result_row(PGresult *res, * If we have a CTID to return, install it in both t_self and t_ctid. * t_self is the normal place, but if the tuple is converted to a * composite Datum, t_self will be lost; setting t_ctid allows CTID to be - * preserved during EvalPlanQual re-evaluations (see ROW_MARK_COPY code). + * preserved during EvalPlanQual re-evaluations (see ROW_REF_COPY code). */ if (ctid) tuple->t_self = tuple->t_data->t_ctid = *ctid; diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml index b80320504d6..51bc0e1029a 100644 --- a/doc/src/sgml/fdwhandler.sgml +++ b/doc/src/sgml/fdwhandler.sgml @@ -1160,13 +1160,16 @@ ExecForeignTruncate(List *rels, RowMarkType GetForeignRowMarkType(RangeTblEntry *rte, - LockClauseStrength strength); + LockClauseStrength strength, + RowRefType *refType); Report which row-marking option to use for a foreign table. - rte is the RangeTblEntry node for the table - and strength describes the lock strength requested by the - relevant FOR UPDATE/SHARE clause, if any. The result must be + rte is the RangeTblEntry node for the table; + strength describes the lock strength requested by the + relevant FOR UPDATE/SHARE clause, if any; + refType point to the value of RowRefType + specifying the way to reference the row. The result must be a member of the RowMarkType enum type. @@ -1177,9 +1180,16 @@ GetForeignRowMarkType(RangeTblEntry *rte, or DELETE. + + If the value pointed by refType is not changed, + the ROW_REF_COPY option is used. + + If the GetForeignRowMarkType pointer is set to - NULL, the ROW_MARK_COPY option is always used. + NULL, the ROW_MARK_REFERENCE option + for row mark type and ROW_REF_COPY option for the row + reference type are always used. (This implies that RefetchForeignRow will never be called, so it need not be provided either.) @@ -1213,7 +1223,7 @@ RefetchForeignRow(EState *estate, defined by erm->markType, which is the value previously returned by GetForeignRowMarkType. (ROW_MARK_REFERENCE means to just re-fetch the tuple - without acquiring any lock, and ROW_MARK_COPY will + without acquiring any lock. This shouldn't and ROW_MARK_COPY will never be seen by this routine.) diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 7eb1f7d0209..3b03f03a98d 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -875,22 +875,19 @@ InitPlan(QueryDesc *queryDesc, int eflags) /* 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) + /* + * Open relation, if we need to access it for this reference type. + */ + switch (rc->refType) { - case ROW_MARK_EXCLUSIVE: - case ROW_MARK_NOKEYEXCLUSIVE: - case ROW_MARK_SHARE: - case ROW_MARK_KEYSHARE: - case ROW_MARK_REFERENCE: + case ROW_REF_TID: relation = ExecGetRangeTableRelation(estate, rc->rti); break; - case ROW_MARK_COPY: - /* no physical table access is required */ + case ROW_REF_COPY: relation = NULL; break; default: - elog(ERROR, "unrecognized markType: %d", rc->markType); + elog(ERROR, "unrecognized refType: %d", rc->refType); relation = NULL; /* keep compiler quiet */ break; } @@ -906,6 +903,7 @@ InitPlan(QueryDesc *queryDesc, int eflags) erm->prti = rc->prti; erm->rowmarkId = rc->rowmarkId; erm->markType = rc->markType; + erm->refType = rc->refType; erm->strength = rc->strength; erm->waitPolicy = rc->waitPolicy; erm->ermActive = false; @@ -2402,10 +2400,14 @@ ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist) aerm->rowmark = erm; - /* Look up the resjunk columns associated with this rowmark */ - if (erm->markType != ROW_MARK_COPY) + /* + * Look up the resjunk columns associated with this rowmark's reference + * type. + */ + if (erm->refType != ROW_REF_COPY) { /* need ctid for all methods other than COPY */ + Assert(erm->refType == ROW_REF_TID); snprintf(resname, sizeof(resname), "ctid%u", erm->rowmarkId); aerm->ctidAttNo = ExecFindJunkAttributeInTlist(targetlist, resname); @@ -2656,7 +2658,12 @@ EvalPlanQualFetchRowMark(EPQState *epqstate, Index rti, TupleTableSlot *slot) } } - if (erm->markType == ROW_MARK_REFERENCE) + /* + * For non-locked relation, the row mark type should be + * ROW_MARK_REFERENCE. Fetch the tuple accodring to reference type. + */ + Assert(erm->markType == ROW_MARK_REFERENCE); + if (erm->refType == ROW_REF_TID) { Assert(erm->relation != NULL); @@ -2709,7 +2716,7 @@ EvalPlanQualFetchRowMark(EPQState *epqstate, Index rti, TupleTableSlot *slot) } else { - Assert(erm->markType == ROW_MARK_COPY); + Assert(erm->refType == ROW_REF_COPY); /* fetch the whole-row Var for the relation */ datum = ExecGetJunkAttribute(epqstate->origslot, diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 38d070fa004..4b9c9deee84 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -2309,7 +2309,7 @@ preprocess_rowmarks(PlannerInfo *root) * Ignore RowMarkClauses for subqueries; they aren't real tables and * can't support true locking. Subqueries that got flattened into the * main query should be ignored completely. Any that didn't will get - * ROW_MARK_COPY items in the next loop. + * ROW_REF_COPY items in the next loop. */ if (rte->rtekind != RTE_RELATION) continue; @@ -2319,8 +2319,8 @@ preprocess_rowmarks(PlannerInfo *root) newrc = makeNode(PlanRowMark); newrc->rti = newrc->prti = rc->rti; newrc->rowmarkId = ++(root->glob->lastRowMarkId); - newrc->markType = select_rowmark_type(rte, rc->strength); - newrc->allMarkTypes = (1 << newrc->markType); + newrc->markType = select_rowmark_type(rte, rc->strength, &newrc->refType); + newrc->allRefTypes = (1 << newrc->refType); newrc->strength = rc->strength; newrc->waitPolicy = rc->waitPolicy; newrc->isParent = false; @@ -2344,8 +2344,8 @@ preprocess_rowmarks(PlannerInfo *root) newrc = makeNode(PlanRowMark); newrc->rti = newrc->prti = i; newrc->rowmarkId = ++(root->glob->lastRowMarkId); - newrc->markType = select_rowmark_type(rte, LCS_NONE); - newrc->allMarkTypes = (1 << newrc->markType); + newrc->markType = select_rowmark_type(rte, LCS_NONE, &newrc->refType); + newrc->allRefTypes = (1 << newrc->refType); newrc->strength = LCS_NONE; newrc->waitPolicy = LockWaitBlock; /* doesn't matter */ newrc->isParent = false; @@ -2357,29 +2357,38 @@ preprocess_rowmarks(PlannerInfo *root) } /* - * Select RowMarkType to use for a given table + * Select RowMarkType and RowRefType to use for a given table */ RowMarkType -select_rowmark_type(RangeTblEntry *rte, LockClauseStrength strength) +select_rowmark_type(RangeTblEntry *rte, LockClauseStrength strength, + RowRefType *refType) { if (rte->rtekind != RTE_RELATION) { - /* If it's not a table at all, use ROW_MARK_COPY */ - return ROW_MARK_COPY; + /* + * If it's not a table at all, use ROW_MARK_REFERENCE and + * ROW_REF_COPY. + */ + *refType = ROW_REF_COPY; + return ROW_MARK_REFERENCE; } else if (rte->relkind == RELKIND_FOREIGN_TABLE) { /* Let the FDW select the rowmark type, if it wants to */ FdwRoutine *fdwroutine = GetFdwRoutineByRelId(rte->relid); + /* Set row reference type as ROW_REF_COPY by default */ + *refType = ROW_REF_COPY; + if (fdwroutine->GetForeignRowMarkType != NULL) - return fdwroutine->GetForeignRowMarkType(rte, strength); - /* Otherwise, use ROW_MARK_COPY by default */ - return ROW_MARK_COPY; + return fdwroutine->GetForeignRowMarkType(rte, strength, refType); + /* Otherwise, use ROW_MARK_REFERENCE by default */ + return ROW_MARK_REFERENCE; } else { /* Regular table, apply the appropriate lock type */ + *refType = ROW_REF_TID; switch (strength) { case LCS_NONE: diff --git a/src/backend/optimizer/prep/preptlist.c b/src/backend/optimizer/prep/preptlist.c index 7698bfa1a58..4599b0dc761 100644 --- a/src/backend/optimizer/prep/preptlist.c +++ b/src/backend/optimizer/prep/preptlist.c @@ -210,7 +210,7 @@ preprocess_targetlist(PlannerInfo *root) if (rc->rti != rc->prti) continue; - if (rc->allMarkTypes & ~(1 << ROW_MARK_COPY)) + if (rc->allRefTypes & (1 << ROW_REF_TID)) { /* Need to fetch TID */ var = makeVar(rc->rti, @@ -226,7 +226,7 @@ preprocess_targetlist(PlannerInfo *root) true); tlist = lappend(tlist, tle); } - if (rc->allMarkTypes & (1 << ROW_MARK_COPY)) + if (rc->allRefTypes & (1 << ROW_REF_COPY)) { /* Need the whole row as a junk var */ var = makeWholeRowVar(rt_fetch(rc->rti, range_table), diff --git a/src/backend/optimizer/util/inherit.c b/src/backend/optimizer/util/inherit.c index 5c7acf8a901..b4b076d1cb1 100644 --- a/src/backend/optimizer/util/inherit.c +++ b/src/backend/optimizer/util/inherit.c @@ -91,7 +91,7 @@ expand_inherited_rtentry(PlannerInfo *root, RelOptInfo *rel, LOCKMODE lockmode; PlanRowMark *oldrc; bool old_isParent = false; - int old_allMarkTypes = 0; + int old_allRefTypes = 0; Assert(rte->inh); /* else caller error */ @@ -131,8 +131,8 @@ expand_inherited_rtentry(PlannerInfo *root, RelOptInfo *rel, { old_isParent = oldrc->isParent; oldrc->isParent = true; - /* Save initial value of allMarkTypes before children add to it */ - old_allMarkTypes = oldrc->allMarkTypes; + /* Save initial value of allRefTypes before children add to it */ + old_allRefTypes = oldrc->allRefTypes; } /* Scan the inheritance set and expand it */ @@ -239,15 +239,15 @@ expand_inherited_rtentry(PlannerInfo *root, RelOptInfo *rel, */ if (oldrc) { - int new_allMarkTypes = oldrc->allMarkTypes; + int new_allRefTypes = oldrc->allRefTypes; Var *var; TargetEntry *tle; char resname[32]; List *newvars = NIL; /* Add TID junk Var if needed, unless we had it already */ - if (new_allMarkTypes & ~(1 << ROW_MARK_COPY) && - !(old_allMarkTypes & ~(1 << ROW_MARK_COPY))) + if (new_allRefTypes & (1 << ROW_REF_TID) && + !(old_allRefTypes & (1 << ROW_REF_TID))) { /* Need to fetch TID */ var = makeVar(oldrc->rti, @@ -266,8 +266,8 @@ expand_inherited_rtentry(PlannerInfo *root, RelOptInfo *rel, } /* Add whole-row junk Var if needed, unless we had it already */ - if ((new_allMarkTypes & (1 << ROW_MARK_COPY)) && - !(old_allMarkTypes & (1 << ROW_MARK_COPY))) + if ((new_allRefTypes & (1 << ROW_REF_COPY)) && + !(old_allRefTypes & (1 << ROW_REF_COPY))) { var = makeWholeRowVar(planner_rt_fetch(oldrc->rti, root), oldrc->rti, @@ -441,7 +441,7 @@ expand_partitioned_rtentry(PlannerInfo *root, RelOptInfo *relinfo, * where the hierarchy is flattened during RTE expansion.) * * PlanRowMarks still carry the top-parent's RTI, and the top-parent's - * allMarkTypes field still accumulates values from all descendents. + * allRefTypes field still accumulates values from all descendents. * * "parentrte" and "parentRTindex" are immediate parent's RTE and * RTI. "top_parentrc" is top parent's PlanRowMark. @@ -580,8 +580,9 @@ expand_single_inheritance_child(PlannerInfo *root, RangeTblEntry *parentrte, childrc->rowmarkId = top_parentrc->rowmarkId; /* Reselect rowmark type, because relkind might not match parent */ childrc->markType = select_rowmark_type(childrte, - top_parentrc->strength); - childrc->allMarkTypes = (1 << childrc->markType); + top_parentrc->strength, + &childrc->refType); + childrc->allRefTypes = (1 << childrc->refType); childrc->strength = top_parentrc->strength; childrc->waitPolicy = top_parentrc->waitPolicy; @@ -592,8 +593,8 @@ expand_single_inheritance_child(PlannerInfo *root, RangeTblEntry *parentrte, */ childrc->isParent = (childrte->relkind == RELKIND_PARTITIONED_TABLE); - /* Include child's rowmark type in top parent's allMarkTypes */ - top_parentrc->allMarkTypes |= childrc->allMarkTypes; + /* Include child's rowmark type in top parent's allRefTypes */ + top_parentrc->allRefTypes |= childrc->allRefTypes; root->rowMarks = lappend(root->rowMarks, childrc); } diff --git a/src/include/foreign/fdwapi.h b/src/include/foreign/fdwapi.h index 0968e0a01ec..868e04e813e 100644 --- a/src/include/foreign/fdwapi.h +++ b/src/include/foreign/fdwapi.h @@ -129,7 +129,8 @@ typedef TupleTableSlot *(*IterateDirectModify_function) (ForeignScanState *node) typedef void (*EndDirectModify_function) (ForeignScanState *node); typedef RowMarkType (*GetForeignRowMarkType_function) (RangeTblEntry *rte, - LockClauseStrength strength); + LockClauseStrength strength, + RowRefType *refType); typedef void (*RefetchForeignRow_function) (EState *estate, ExecRowMark *erm, diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index 1774c56ae31..a1ccf6e6811 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -455,6 +455,9 @@ typedef struct ResultRelInfo /* relation descriptor for result relation */ Relation ri_RelationDesc; + /* row indentifier for result relation */ + RowRefType ri_RowRefType; + /* # of indices existing on result relation */ int ri_NumIndices; @@ -750,6 +753,7 @@ typedef struct ExecRowMark Index prti; /* parent range table index, if child */ Index rowmarkId; /* unique identifier for resjunk columns */ RowMarkType markType; /* see enum in nodes/plannodes.h */ + RowRefType refType; /* row indentifier for relation */ LockClauseStrength strength; /* LockingClause's strength, or LCS_NONE */ LockWaitPolicy waitPolicy; /* NOWAIT and SKIP LOCKED */ bool ermActive; /* is this mark relevant for current tuple? */ diff --git a/src/include/nodes/plannodes.h b/src/include/nodes/plannodes.h index 7f3db5105db..d7f9c389dac 100644 --- a/src/include/nodes/plannodes.h +++ b/src/include/nodes/plannodes.h @@ -1311,16 +1311,8 @@ typedef struct Limit * * When doing UPDATE/DELETE/MERGE/SELECT FOR UPDATE/SHARE, we have to uniquely * identify all the source rows, not only those from the target relations, so - * that we can perform EvalPlanQual rechecking at need. For plain tables we - * can just fetch the TID, much as for a target relation; this case is - * represented by ROW_MARK_REFERENCE. Otherwise (for example for VALUES or - * FUNCTION scans) we have to copy the whole row value. ROW_MARK_COPY is - * pretty inefficient, since most of the time we'll never need the data; but - * fortunately the overhead is usually not performance-critical in practice. - * By default we use ROW_MARK_COPY for foreign tables, but if the FDW has - * a concept of rowid it can request to use ROW_MARK_REFERENCE instead. - * (Again, this probably doesn't make sense if a physical remote fetch is - * needed, but for FDWs that map to local storage it might be credible.) + * that we can perform EvalPlanQual rechecking at need. ROW_MARK_REFERENCE + * represents this case. */ typedef enum RowMarkType { @@ -1329,9 +1321,29 @@ typedef enum RowMarkType ROW_MARK_SHARE, /* obtain shared tuple lock */ ROW_MARK_KEYSHARE, /* obtain keyshare tuple lock */ ROW_MARK_REFERENCE, /* just fetch the TID, don't lock it */ - ROW_MARK_COPY, /* physically copy the row value */ } RowMarkType; +/* + * RowRefType - + * enums for types of row identifiers + * + * For plain tables we can just fetch the TID, much as for a target relation; + * this case is represented by ROW_REF_TID. Otherwise (for example for VALUES + * or FUNCTION scans) we have to copy the whole row value. ROW_REF_COPY is + * pretty inefficient, since most of the time we'll never need the data; but + * fortunately the overhead is usually not performance-critical in practice. + * By default we use ROW_REF_COPY for foreign tables, but if the FDW has + * a concept of rowid it can request to use ROW_REF_TID instead. + * (Again, this probably doesn't make sense if a physical remote fetch is + * needed, but for FDWs that map to local storage it might be credible.) + * In future we may allow more types of row identifiers. + */ +typedef enum RowRefType +{ + ROW_REF_TID, /* Item pointer (block, offset) */ + ROW_REF_COPY /* Full row copy */ +} RowRefType; + #define RowMarkRequiresRowShareLock(marktype) ((marktype) <= ROW_MARK_KEYSHARE) /* @@ -1340,8 +1352,7 @@ typedef enum RowMarkType * * When doing UPDATE/DELETE/MERGE/SELECT FOR UPDATE/SHARE, we create a separate * PlanRowMark node for each non-target relation in the query. Relations that - * are not specified as FOR UPDATE/SHARE are marked ROW_MARK_REFERENCE (if - * regular tables or supported foreign tables) or ROW_MARK_COPY (if not). + * are not specified as FOR UPDATE/SHARE are marked ROW_MARK_REFERENCE. * * Initially all PlanRowMarks have rti == prti and isParent == false. * When the planner discovers that a relation is the root of an inheritance @@ -1351,16 +1362,16 @@ typedef enum RowMarkType * child relations will also have entries with isParent = true. The child * entries have rti == child rel's RT index and prti == top parent's RT index, * and can therefore be recognized as children by the fact that prti != rti. - * The parent's allMarkTypes field gets the OR of (1<