Skip to content

Commit 7b8b8a4

Browse files
tglsfdctglsfdc
committed
Improve representation of PlanRowMark.
This patch fixes two inadequacies of the PlanRowMark representation. First, that the original LockingClauseStrength isn't stored (and cannot be inferred for foreign tables, which always get ROW_MARK_COPY). Since some PlanRowMarks are created out of whole cloth and don't actually have an ancestral RowMarkClause, this requires adding a dummy LCS_NONE value to enum LockingClauseStrength, which is fairly annoying but the alternatives seem worse. This fix allows getting rid of the use of get_parse_rowmark() in FDWs (as per the discussion around commits 462bd95 and 8ec8760), and it simplifies some things elsewhere. Second, that the representation assumed that all child tables in an inheritance hierarchy would use the same RowMarkType. That's true today but will soon not be true. We add an "allMarkTypes" field that identifies the union of mark types used in all a parent table's children, and use that where appropriate (currently, only in preprocess_targetlist()). In passing fix a couple of minor infelicities left over from the SKIP LOCKED patch, notably that _outPlanRowMark still thought waitPolicy is a bool. Catversion bump is required because the numeric values of enum LockingClauseStrength can appear in on-disk rules. Extracted from a much larger patch to support foreign table inheritance; it seemed worth breaking this out, since it's a separable concern. Shigeru Hanada and Etsuro Fujita, somewhat modified by me
1 parent 9fac5fd commit 7b8b8a4

File tree

14 files changed

+108
-85
lines changed

14 files changed

+108
-85
lines changed

‎contrib/postgres_fdw/postgres_fdw.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -822,13 +822,14 @@ postgresGetForeignPlan(PlannerInfo *root,
822822
}
823823
else
824824
{
825-
RowMarkClause *rc = get_parse_rowmark(root->parse, baserel->relid);
825+
PlanRowMark *rc = get_plan_rowmark(root->rowMarks, baserel->relid);
826826

827827
if (rc)
828828
{
829829
/*
830830
* Relation is specified as a FOR UPDATE/SHARE target, so handle
831-
* that.
831+
* that. (But we could also see LCS_NONE, meaning this isn't a
832+
* target relation after all.)
832833
*
833834
* For now, just ignore any [NO] KEY specification, since (a) it's
834835
* not clear what that means for a remote table that we don't have
@@ -837,6 +838,9 @@ postgresGetForeignPlan(PlannerInfo *root,
837838
*/
838839
switch (rc->strength)
839840
{
841+
case LCS_NONE:
842+
/* No locking needed */
843+
break;
840844
case LCS_FORKEYSHARE:
841845
case LCS_FORSHARE:
842846
appendStringInfoString(&sql, " FOR SHARE");

‎src/backend/nodes/copyfuncs.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -991,6 +991,8 @@ _copyPlanRowMark(const PlanRowMark *from)
991991
COPY_SCALAR_FIELD(prti);
992992
COPY_SCALAR_FIELD(rowmarkId);
993993
COPY_SCALAR_FIELD(markType);
994+
COPY_SCALAR_FIELD(allMarkTypes);
995+
COPY_SCALAR_FIELD(strength);
994996
COPY_SCALAR_FIELD(waitPolicy);
995997
COPY_SCALAR_FIELD(isParent);
996998

@@ -2510,7 +2512,7 @@ _copyXmlSerialize(const XmlSerialize *from)
25102512
static RoleSpec *
25112513
_copyRoleSpec(const RoleSpec *from)
25122514
{
2513-
RoleSpec *newnode = makeNode(RoleSpec);
2515+
RoleSpec *newnode = makeNode(RoleSpec);
25142516

25152517
COPY_SCALAR_FIELD(roletype);
25162518
COPY_STRING_FIELD(rolename);

‎src/backend/nodes/outfuncs.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -852,7 +852,9 @@ _outPlanRowMark(StringInfo str, const PlanRowMark *node)
852852
WRITE_UINT_FIELD(prti);
853853
WRITE_UINT_FIELD(rowmarkId);
854854
WRITE_ENUM_FIELD(markType, RowMarkType);
855-
WRITE_BOOL_FIELD(waitPolicy);
855+
WRITE_INT_FIELD(allMarkTypes);
856+
WRITE_ENUM_FIELD(strength, LockClauseStrength);
857+
WRITE_ENUM_FIELD(waitPolicy, LockWaitPolicy);
856858
WRITE_BOOL_FIELD(isParent);
857859
}
858860

‎src/backend/optimizer/plan/planner.c

Lines changed: 31 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -2219,35 +2219,42 @@ preprocess_rowmarks(PlannerInfo *root)
22192219
if (rte->rtekind != RTE_RELATION)
22202220
continue;
22212221

2222-
/*
2223-
* Similarly, ignore RowMarkClauses for foreign tables; foreign tables
2224-
* will instead get ROW_MARK_COPY items in the next loop. (FDWs might
2225-
* choose to do something special while fetching their rows, but that
2226-
* is of no concern here.)
2227-
*/
2228-
if (rte->relkind == RELKIND_FOREIGN_TABLE)
2229-
continue;
2230-
22312222
rels = bms_del_member(rels, rc->rti);
22322223

22332224
newrc = makeNode(PlanRowMark);
22342225
newrc->rti = newrc->prti = rc->rti;
22352226
newrc->rowmarkId = ++(root->glob->lastRowMarkId);
2236-
switch (rc->strength)
2227+
if (rte->relkind == RELKIND_FOREIGN_TABLE)
22372228
{
2238-
case LCS_FORUPDATE:
2239-
newrc->markType = ROW_MARK_EXCLUSIVE;
2240-
break;
2241-
case LCS_FORNOKEYUPDATE:
2242-
newrc->markType = ROW_MARK_NOKEYEXCLUSIVE;
2243-
break;
2244-
case LCS_FORSHARE:
2245-
newrc->markType = ROW_MARK_SHARE;
2246-
break;
2247-
case LCS_FORKEYSHARE:
2248-
newrc->markType = ROW_MARK_KEYSHARE;
2249-
break;
2229+
/* For now, we force all foreign tables to use ROW_MARK_COPY */
2230+
newrc->markType = ROW_MARK_COPY;
2231+
}
2232+
else
2233+
{
2234+
/* regular table, apply the appropriate lock type */
2235+
switch (rc->strength)
2236+
{
2237+
case LCS_NONE:
2238+
/* we intentionally throw an error for LCS_NONE */
2239+
elog(ERROR, "unrecognized LockClauseStrength %d",
2240+
(int) rc->strength);
2241+
break;
2242+
case LCS_FORKEYSHARE:
2243+
newrc->markType = ROW_MARK_KEYSHARE;
2244+
break;
2245+
case LCS_FORSHARE:
2246+
newrc->markType = ROW_MARK_SHARE;
2247+
break;
2248+
case LCS_FORNOKEYUPDATE:
2249+
newrc->markType = ROW_MARK_NOKEYEXCLUSIVE;
2250+
break;
2251+
case LCS_FORUPDATE:
2252+
newrc->markType = ROW_MARK_EXCLUSIVE;
2253+
break;
2254+
}
22502255
}
2256+
newrc->allMarkTypes = (1 << newrc->markType);
2257+
newrc->strength = rc->strength;
22512258
newrc->waitPolicy = rc->waitPolicy;
22522259
newrc->isParent = false;
22532260

@@ -2276,6 +2283,8 @@ preprocess_rowmarks(PlannerInfo *root)
22762283
newrc->markType = ROW_MARK_REFERENCE;
22772284
else
22782285
newrc->markType = ROW_MARK_COPY;
2286+
newrc->allMarkTypes = (1 << newrc->markType);
2287+
newrc->strength = LCS_NONE;
22792288
newrc->waitPolicy = LockWaitBlock; /* doesn't matter */
22802289
newrc->isParent = false;
22812290

‎src/backend/optimizer/prep/prepsecurity.c

Lines changed: 7 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,8 @@ expand_security_quals(PlannerInfo *root, List *tlist)
7373
rt_index = 0;
7474
foreach(cell, parse->rtable)
7575
{
76-
bool targetRelation = false;
77-
RangeTblEntry *rte = (RangeTblEntry *) lfirst(cell);
76+
bool targetRelation = false;
77+
RangeTblEntry *rte = (RangeTblEntry *) lfirst(cell);
7878

7979
rt_index++;
8080

@@ -241,30 +241,10 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
241241
rc = get_plan_rowmark(root->rowMarks, rt_index);
242242
if (rc != NULL)
243243
{
244-
switch (rc->markType)
245-
{
246-
case ROW_MARK_EXCLUSIVE:
247-
applyLockingClause(subquery, 1, LCS_FORUPDATE,
248-
rc->waitPolicy, false);
249-
break;
250-
case ROW_MARK_NOKEYEXCLUSIVE:
251-
applyLockingClause(subquery, 1, LCS_FORNOKEYUPDATE,
252-
rc->waitPolicy, false);
253-
break;
254-
case ROW_MARK_SHARE:
255-
applyLockingClause(subquery, 1, LCS_FORSHARE,
256-
rc->waitPolicy, false);
257-
break;
258-
case ROW_MARK_KEYSHARE:
259-
applyLockingClause(subquery, 1, LCS_FORKEYSHARE,
260-
rc->waitPolicy, false);
261-
break;
262-
case ROW_MARK_REFERENCE:
263-
case ROW_MARK_COPY:
264-
/* No locking needed */
265-
break;
266-
}
267-
root->rowMarks = list_delete(root->rowMarks, rc);
244+
if (rc->strength != LCS_NONE)
245+
applyLockingClause(subquery, 1, rc->strength,
246+
rc->waitPolicy, false);
247+
root->rowMarks = list_delete_ptr(root->rowMarks, rc);
268248
}
269249

270250
/*
@@ -276,6 +256,7 @@ expand_security_qual(PlannerInfo *root, List *tlist, int rt_index,
276256
if (targetRelation)
277257
applyLockingClause(subquery, 1, LCS_FORUPDATE,
278258
LockWaitBlock, false);
259+
279260
/*
280261
* Replace any variables in the outer query that refer to the
281262
* original relation RTE with references to columns that we will

‎src/backend/optimizer/prep/preptlist.c

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -92,9 +92,9 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
9292
if (rc->rti != rc->prti)
9393
continue;
9494

95-
if (rc->markType != ROW_MARK_COPY)
95+
if (rc->allMarkTypes & ~(1 << ROW_MARK_COPY))
9696
{
97-
/* It's a regular table, so fetch its TID */
97+
/* Need to fetch TID */
9898
var = makeVar(rc->rti,
9999
SelfItemPointerAttributeNumber,
100100
TIDOID,
@@ -125,9 +125,9 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
125125
tlist = lappend(tlist, tle);
126126
}
127127
}
128-
else
128+
if (rc->allMarkTypes & (1 << ROW_MARK_COPY))
129129
{
130-
/* Not a table, so we need the whole row as a junk var */
130+
/* Need the whole row as a junk var */
131131
var = makeWholeRowVar(rt_fetch(rc->rti, range_table),
132132
rc->rti,
133133
0,

‎src/backend/optimizer/prep/prepunion.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1389,9 +1389,14 @@ expand_inherited_rtentry(PlannerInfo *root, RangeTblEntry *rte, Index rti)
13891389
newrc->prti = rti;
13901390
newrc->rowmarkId = oldrc->rowmarkId;
13911391
newrc->markType = oldrc->markType;
1392+
newrc->allMarkTypes = (1 << newrc->markType);
1393+
newrc->strength = oldrc->strength;
13921394
newrc->waitPolicy = oldrc->waitPolicy;
13931395
newrc->isParent = false;
13941396

1397+
/* Include child's rowmark type in parent's allMarkTypes */
1398+
oldrc->allMarkTypes |= newrc->allMarkTypes;
1399+
13951400
root->rowMarks = lappend(root->rowMarks, newrc);
13961401
}
13971402

‎src/backend/parser/analyze.c

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2254,11 +2254,18 @@ transformCreateTableAsStmt(ParseState *pstate, CreateTableAsStmt *stmt)
22542254
}
22552255

22562256

2257-
char *
2257+
/*
2258+
* Produce a string representation of a LockClauseStrength value.
2259+
* This should only be applied to valid values (not LCS_NONE).
2260+
*/
2261+
const char *
22582262
LCS_asString(LockClauseStrength strength)
22592263
{
22602264
switch (strength)
22612265
{
2266+
case LCS_NONE:
2267+
Assert(false);
2268+
break;
22622269
case LCS_FORKEYSHARE:
22632270
return "FOR KEY SHARE";
22642271
case LCS_FORSHARE:
@@ -2279,6 +2286,8 @@ LCS_asString(LockClauseStrength strength)
22792286
void
22802287
CheckSelectLocking(Query *qry, LockClauseStrength strength)
22812288
{
2289+
Assert(strength != LCS_NONE); /* else caller error */
2290+
22822291
if (qry->setOperations)
22832292
ereport(ERROR,
22842293
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
@@ -2498,6 +2507,8 @@ applyLockingClause(Query *qry, Index rtindex,
24982507
{
24992508
RowMarkClause *rc;
25002509

2510+
Assert(strength != LCS_NONE); /* else caller error */
2511+
25012512
/* If it's an explicit clause, make sure hasForUpdate gets set */
25022513
if (!pushedDown)
25032514
qry->hasForUpdate = true;
@@ -2506,20 +2517,21 @@ applyLockingClause(Query *qry, Index rtindex,
25062517
if ((rc = get_parse_rowmark(qry, rtindex)) != NULL)
25072518
{
25082519
/*
2509-
* If the same RTE is specified for more than one locking strength,
2510-
* treat is as the strongest. (Reasonable, since you can't take both
2511-
* a shared and exclusive lock at the same time; it'll end up being
2512-
* exclusive anyway.)
2520+
* If the same RTE is specified with more than one locking strength,
2521+
* use the strongest. (Reasonable, since you can't take both a shared
2522+
* and exclusive lock at the same time; it'll end up being exclusive
2523+
* anyway.)
25132524
*
2514-
* Similarly, if the same RTE is specified with more than one lock wait
2515-
* policy, consider that NOWAIT wins over SKIP LOCKED, which in turn
2516-
* wins over waiting for the lock (the default). This is a bit more
2517-
* debatable but raising an error doesn't seem helpful. (Consider for
2518-
* instance SELECT FOR UPDATE NOWAIT from a view that internally
2525+
* Similarly, if the same RTE is specified with more than one lock
2526+
* wait policy, consider that NOWAIT wins over SKIP LOCKED, which in
2527+
* turn wins over waiting for the lock (the default). This is a bit
2528+
* more debatable but raising an error doesn't seem helpful. (Consider
2529+
* for instance SELECT FOR UPDATE NOWAIT from a view that internally
25192530
* contains a plain FOR UPDATE spec.) Having NOWAIT win over SKIP
25202531
* LOCKED is reasonable since the former throws an error in case of
2521-
* coming across a locked tuple, which may be undesirable in some cases
2522-
* but it seems better than silently returning inconsistent results.
2532+
* coming across a locked tuple, which may be undesirable in some
2533+
* cases but it seems better than silently returning inconsistent
2534+
* results.
25232535
*
25242536
* And of course pushedDown becomes false if any clause is explicit.
25252537
*/

‎src/backend/tcop/utility.c

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2395,26 +2395,22 @@ CreateCommandTag(Node *parsetree)
23952395
else if (stmt->rowMarks != NIL)
23962396
{
23972397
/* not 100% but probably close enough */
2398-
switch (((PlanRowMark *) linitial(stmt->rowMarks))->markType)
2398+
switch (((PlanRowMark *) linitial(stmt->rowMarks))->strength)
23992399
{
2400-
case ROW_MARK_EXCLUSIVE:
2401-
tag = "SELECT FOR UPDATE";
2402-
break;
2403-
case ROW_MARK_NOKEYEXCLUSIVE:
2404-
tag = "SELECT FOR NO KEY UPDATE";
2400+
case LCS_FORKEYSHARE:
2401+
tag = "SELECT FOR KEY SHARE";
24052402
break;
2406-
case ROW_MARK_SHARE:
2403+
case LCS_FORSHARE:
24072404
tag = "SELECT FOR SHARE";
24082405
break;
2409-
case ROW_MARK_KEYSHARE:
2410-
tag = "SELECT FOR KEY SHARE";
2406+
case LCS_FORNOKEYUPDATE:
2407+
tag = "SELECT FOR NO KEY UPDATE";
24112408
break;
2412-
case ROW_MARK_REFERENCE:
2413-
case ROW_MARK_COPY:
2414-
tag = "SELECT";
2409+
case LCS_FORUPDATE:
2410+
tag = "SELECT FOR UPDATE";
24152411
break;
24162412
default:
2417-
tag = "???";
2413+
tag = "SELECT";
24182414
break;
24192415
}
24202416
}

‎src/backend/utils/adt/ruleutils.c

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4453,6 +4453,11 @@ get_select_query_def(Query *query, deparse_context *context,
44534453

44544454
switch (rc->strength)
44554455
{
4456+
case LCS_NONE:
4457+
/* we intentionally throw an error for LCS_NONE */
4458+
elog(ERROR, "unrecognized LockClauseStrength %d",
4459+
(int) rc->strength);
4460+
break;
44564461
case LCS_FORKEYSHARE:
44574462
appendContextKeyword(context, " FOR KEY SHARE",
44584463
-PRETTYINDENT_STD, PRETTYINDENT_STD, 0);

‎src/include/catalog/catversion.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,6 @@
5353
*/
5454

5555
/* yyyymmddN */
56-
#define CATALOG_VERSION_NO 201503061
56+
#define CATALOG_VERSION_NO 201503151
5757

5858
#endif

‎src/include/nodes/lockoptions.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
*/
2121
typedef enum LockClauseStrength
2222
{
23+
LCS_NONE, /* no such clause - only used in PlanRowMark */
2324
LCS_FORKEYSHARE, /* FOR KEY SHARE */
2425
LCS_FORSHARE, /* FOR SHARE */
2526
LCS_FORNOKEYUPDATE, /* FOR NO KEY UPDATE */

‎src/include/nodes/plannodes.h

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -820,7 +820,7 @@ typedef enum RowMarkType
820820
ROW_MARK_NOKEYEXCLUSIVE, /* obtain no-key exclusive tuple lock */
821821
ROW_MARK_SHARE, /* obtain shared tuple lock */
822822
ROW_MARK_KEYSHARE, /* obtain keyshare tuple lock */
823-
ROW_MARK_REFERENCE, /* just fetch the TID */
823+
ROW_MARK_REFERENCE, /* just fetch the TID, don't lock it */
824824
ROW_MARK_COPY /* physically copy the row value */
825825
} RowMarkType;
826826

@@ -841,7 +841,9 @@ typedef enum RowMarkType
841841
* list for each child relation (including the target rel itself in its role
842842
* as a child). The child entries have rti == child rel's RT index and
843843
* prti == parent's RT index, and can therefore be recognized as children by
844-
* the fact that prti != rti.
844+
* the fact that prti != rti. The parent's allMarkTypes field gets the OR
845+
* of (1<<markType) across all its children (this definition allows children
846+
* to use different markTypes).
845847
*
846848
* The planner also adds resjunk output columns to the plan that carry
847849
* information sufficient to identify the locked or fetched rows. For
@@ -851,6 +853,8 @@ typedef enum RowMarkType
851853
* The tableoid column is only present for an inheritance hierarchy.
852854
* When markType == ROW_MARK_COPY, there is instead a single column named
853855
* wholerow%u whole-row value of relation
856+
* (An inheritance hierarchy could have all three resjunk output columns,
857+
* if some children use a different markType than others.)
854858
* In all three cases, %u represents the rowmark ID number (rowmarkId).
855859
* This number is unique within a plan tree, except that child relation
856860
* entries copy their parent's rowmarkId. (Assigning unique numbers
@@ -867,6 +871,8 @@ typedef struct PlanRowMark
867871
Index prti; /* range table index of parent relation */
868872
Index rowmarkId; /* unique identifier for resjunk columns */
869873
RowMarkType markType; /* see enum above */
874+
int allMarkTypes; /* OR of (1<<markType) for all children */
875+
LockClauseStrength strength; /* LockingClause's strength, or LCS_NONE */
870876
LockWaitPolicy waitPolicy; /* NOWAIT and SKIP LOCKED options */
871877
bool isParent; /* true if this is a "dummy" parent entry */
872878
} PlanRowMark;

0 commit comments

Comments
 (0)