Skip to content

Commit fdba460

Browse files
tglsfdctglsfdc
committed
Create an RTE field to record the query's lock mode for each relation.
Add RangeTblEntry.rellockmode, which records the appropriate lock mode for each RTE_RELATION rangetable entry (either AccessShareLock, RowShareLock, or RowExclusiveLock depending on the RTE's role in the query). This patch creates the field and makes all creators of RTE nodes fill it in reasonably, but for the moment nothing much is done with it. The plan is to replace assorted post-parser logic that re-determines the right lockmode to use with simple uses of rte->rellockmode. For now, just add Asserts in each of those places that the rellockmode matches what they are computing today. (In some cases the match isn't perfect, so the Asserts are weaker than you might expect; but this seems OK, as per discussion.) This passes check-world for me, but it seems worth pushing in this state to see if the buildfarm finds any problems in cases I failed to test. catversion bump due to change of stored rules. Amit Langote, reviewed by David Rowley and Jesper Pedersen, and whacked around a bit more by me Discussion: https://postgr.es/m/468c85d9-540e-66a2-1dde-fec2b741e688@lab.ntt.co.jp
1 parent 8bddc86 commit fdba460

29 files changed

+159
-38
lines changed

‎src/backend/catalog/dependency.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1415,6 +1415,7 @@ recordDependencyOnSingleRelExpr(const ObjectAddress *depender,
14151415
rte.rtekind = RTE_RELATION;
14161416
rte.relid = relId;
14171417
rte.relkind = RELKIND_RELATION; /* no need for exactness here */
1418+
rte.rellockmode = AccessShareLock;
14181419

14191420
context.rtables = list_make1(list_make1(&rte));
14201421

‎src/backend/catalog/heap.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2549,6 +2549,7 @@ AddRelationNewConstraints(Relation rel,
25492549
pstate->p_sourcetext = queryString;
25502550
rte = addRangeTableEntryForRelation(pstate,
25512551
rel,
2552+
AccessShareLock,
25522553
NULL,
25532554
false,
25542555
true);

‎src/backend/commands/copy.c

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -833,20 +833,21 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
833833

834834
if (stmt->relation)
835835
{
836+
LOCKMODE lockmode = is_from ? RowExclusiveLock : AccessShareLock;
837+
RangeTblEntry *rte;
836838
TupleDesc tupDesc;
837839
List *attnums;
838840
ListCell *cur;
839-
RangeTblEntry *rte;
840841

841842
Assert(!stmt->query);
842843

843844
/* Open and lock the relation, using the appropriate lock type. */
844-
rel = heap_openrv(stmt->relation,
845-
(is_from ? RowExclusiveLock : AccessShareLock));
845+
rel = heap_openrv(stmt->relation, lockmode);
846846

847847
relid = RelationGetRelid(rel);
848848

849-
rte = addRangeTableEntryForRelation(pstate, rel, NULL, false, false);
849+
rte = addRangeTableEntryForRelation(pstate, rel, lockmode,
850+
NULL, false, false);
850851
rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
851852

852853
tupDesc = RelationGetDescr(rel);

‎src/backend/commands/createas.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,7 @@ intorel_startup(DestReceiver *self, int operation, TupleDesc typeinfo)
528528
rte->rtekind = RTE_RELATION;
529529
rte->relid = intoRelationAddr.objectId;
530530
rte->relkind = relkind;
531+
rte->rellockmode = RowExclusiveLock;
531532
rte->requiredPerms = ACL_INSERT;
532533

533534
for (attnum = 1; attnum <= intoRelationDesc->rd_att->natts; attnum++)

‎src/backend/commands/policy.c

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -567,7 +567,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
567567
qual_expr = stringToNode(qual_value);
568568

569569
/* Add this rel to the parsestate's rangetable, for dependencies */
570-
addRangeTableEntryForRelation(qual_pstate, rel, NULL, false, false);
570+
addRangeTableEntryForRelation(qual_pstate, rel,
571+
AccessShareLock,
572+
NULL, false, false);
571573

572574
qual_parse_rtable = qual_pstate->p_rtable;
573575
free_parsestate(qual_pstate);
@@ -589,8 +591,9 @@ RemoveRoleFromObjectPolicy(Oid roleid, Oid classid, Oid policy_id)
589591
with_check_qual = stringToNode(with_check_value);
590592

591593
/* Add this rel to the parsestate's rangetable, for dependencies */
592-
addRangeTableEntryForRelation(with_check_pstate, rel, NULL, false,
593-
false);
594+
addRangeTableEntryForRelation(with_check_pstate, rel,
595+
AccessShareLock,
596+
NULL, false, false);
594597

595598
with_check_parse_rtable = with_check_pstate->p_rtable;
596599
free_parsestate(with_check_pstate);
@@ -752,11 +755,13 @@ CreatePolicy(CreatePolicyStmt *stmt)
752755

753756
/* Add for the regular security quals */
754757
rte = addRangeTableEntryForRelation(qual_pstate, target_table,
758+
AccessShareLock,
755759
NULL, false, false);
756760
addRTEtoQuery(qual_pstate, rte, false, true, true);
757761

758762
/* Add for the with-check quals */
759763
rte = addRangeTableEntryForRelation(with_check_pstate, target_table,
764+
AccessShareLock,
760765
NULL, false, false);
761766
addRTEtoQuery(with_check_pstate, rte, false, true, true);
762767

@@ -928,6 +933,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
928933
ParseState *qual_pstate = make_parsestate(NULL);
929934

930935
rte = addRangeTableEntryForRelation(qual_pstate, target_table,
936+
AccessShareLock,
931937
NULL, false, false);
932938

933939
addRTEtoQuery(qual_pstate, rte, false, true, true);
@@ -950,6 +956,7 @@ AlterPolicy(AlterPolicyStmt *stmt)
950956
ParseState *with_check_pstate = make_parsestate(NULL);
951957

952958
rte = addRangeTableEntryForRelation(with_check_pstate, target_table,
959+
AccessShareLock,
953960
NULL, false, false);
954961

955962
addRTEtoQuery(with_check_pstate, rte, false, true, true);
@@ -1096,8 +1103,9 @@ AlterPolicy(AlterPolicyStmt *stmt)
10961103
qual = stringToNode(qual_value);
10971104

10981105
/* Add this rel to the parsestate's rangetable, for dependencies */
1099-
addRangeTableEntryForRelation(qual_pstate, target_table, NULL,
1100-
false, false);
1106+
addRangeTableEntryForRelation(qual_pstate, target_table,
1107+
AccessShareLock,
1108+
NULL, false, false);
11011109

11021110
qual_parse_rtable = qual_pstate->p_rtable;
11031111
free_parsestate(qual_pstate);
@@ -1137,8 +1145,9 @@ AlterPolicy(AlterPolicyStmt *stmt)
11371145
with_check_qual = stringToNode(with_check_value);
11381146

11391147
/* Add this rel to the parsestate's rangetable, for dependencies */
1140-
addRangeTableEntryForRelation(with_check_pstate, target_table, NULL,
1141-
false, false);
1148+
addRangeTableEntryForRelation(with_check_pstate, target_table,
1149+
AccessShareLock,
1150+
NULL, false, false);
11421151

11431152
with_check_parse_rtable = with_check_pstate->p_rtable;
11441153
free_parsestate(with_check_pstate);

‎src/backend/commands/tablecmds.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13632,7 +13632,8 @@ transformPartitionSpec(Relation rel, PartitionSpec *partspec, char *strategy)
1363213632
* rangetable entry. We need a ParseState for transformExpr.
1363313633
*/
1363413634
pstate = make_parsestate(NULL);
13635-
rte = addRangeTableEntryForRelation(pstate, rel, NULL, false, true);
13635+
rte = addRangeTableEntryForRelation(pstate, rel, AccessShareLock,
13636+
NULL, false, true);
1363613637
addRTEtoQuery(pstate, rte, true, true, true);
1363713638

1363813639
/* take care of any partition expressions */

‎src/backend/commands/trigger.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -577,10 +577,12 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
577577
* 'OLD' must always have varno equal to 1 and 'NEW' equal to 2.
578578
*/
579579
rte = addRangeTableEntryForRelation(pstate, rel,
580+
AccessShareLock,
580581
makeAlias("old", NIL),
581582
false, false);
582583
addRTEtoQuery(pstate, rte, false, true, true);
583584
rte = addRangeTableEntryForRelation(pstate, rel,
585+
AccessShareLock,
584586
makeAlias("new", NIL),
585587
false, false);
586588
addRTEtoQuery(pstate, rte, false, true, true);

‎src/backend/commands/view.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,7 @@ DefineViewRules(Oid viewOid, Query *viewParse, bool replace)
353353
* by 2...
354354
*
355355
* These extra RT entries are not actually used in the query,
356-
* except for run-time permission checking.
356+
* except for run-time locking and permission checking.
357357
*---------------------------------------------------------------
358358
*/
359359
static Query *
@@ -386,9 +386,11 @@ UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse)
386386
* OLD first, then NEW....
387387
*/
388388
rt_entry1 = addRangeTableEntryForRelation(pstate, viewRel,
389+
AccessShareLock,
389390
makeAlias("old", NIL),
390391
false, false);
391392
rt_entry2 = addRangeTableEntryForRelation(pstate, viewRel,
393+
AccessShareLock,
392394
makeAlias("new", NIL),
393395
false, false);
394396
/* Must override addRangeTableEntry's default access-check flags */

‎src/backend/executor/execMain.c

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -864,6 +864,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
864864
Relation resultRelation;
865865

866866
resultRelationOid = getrelid(resultRelationIndex, rangeTable);
867+
Assert(rt_fetch(resultRelationIndex, rangeTable)->rellockmode == RowExclusiveLock);
867868
resultRelation = heap_open(resultRelationOid, RowExclusiveLock);
868869

869870
InitResultRelInfo(resultRelInfo,
@@ -904,6 +905,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
904905
Relation resultRelDesc;
905906

906907
resultRelOid = getrelid(resultRelIndex, rangeTable);
908+
Assert(rt_fetch(resultRelIndex, rangeTable)->rellockmode == RowExclusiveLock);
907909
resultRelDesc = heap_open(resultRelOid, RowExclusiveLock);
908910
InitResultRelInfo(resultRelInfo,
909911
resultRelDesc,
@@ -924,8 +926,11 @@ InitPlan(QueryDesc *queryDesc, int eflags)
924926
/* We locked the roots above. */
925927
if (!list_member_int(plannedstmt->rootResultRelations,
926928
resultRelIndex))
929+
{
930+
Assert(rt_fetch(resultRelIndex, rangeTable)->rellockmode == RowExclusiveLock);
927931
LockRelationOid(getrelid(resultRelIndex, rangeTable),
928932
RowExclusiveLock);
933+
}
929934
}
930935
}
931936
}
@@ -955,6 +960,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
955960
{
956961
PlanRowMark *rc = (PlanRowMark *) lfirst(l);
957962
Oid relid;
963+
LOCKMODE rellockmode;
958964
Relation relation;
959965
ExecRowMark *erm;
960966

@@ -964,6 +970,7 @@ InitPlan(QueryDesc *queryDesc, int eflags)
964970

965971
/* get relation's OID (will produce InvalidOid if subquery) */
966972
relid = getrelid(rc->rti, rangeTable);
973+
rellockmode = rt_fetch(rc->rti, rangeTable)->rellockmode;
967974

968975
/*
969976
* If you change the conditions under which rel locks are acquired
@@ -975,9 +982,13 @@ InitPlan(QueryDesc *queryDesc, int eflags)
975982
case ROW_MARK_NOKEYEXCLUSIVE:
976983
case ROW_MARK_SHARE:
977984
case ROW_MARK_KEYSHARE:
985+
Assert(rellockmode == RowShareLock);
978986
relation = heap_open(relid, RowShareLock);
979987
break;
980988
case ROW_MARK_REFERENCE:
989+
/* RTE might be a query target table */
990+
Assert(rellockmode == AccessShareLock ||
991+
rellockmode == RowExclusiveLock);
981992
relation = heap_open(relid, AccessShareLock);
982993
break;
983994
case ROW_MARK_COPY:

‎src/backend/executor/execUtils.c

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -657,20 +657,32 @@ ExecOpenScanRelation(EState *estate, Index scanrelid, int eflags)
657657
/*
658658
* Determine the lock type we need. First, scan to see if target relation
659659
* is a result relation. If not, check if it's a FOR UPDATE/FOR SHARE
660-
* relation. In either of those cases, we got the lock already.
660+
* relation.
661+
*
662+
* Note: we may have already gotten the desired lock type, but for now
663+
* don't try to optimize; this logic is going away soon anyhow.
661664
*/
662665
lockmode = AccessShareLock;
663666
if (ExecRelationIsTargetRelation(estate, scanrelid))
664-
lockmode = NoLock;
667+
lockmode = RowExclusiveLock;
665668
else
666669
{
667670
/* Keep this check in sync with InitPlan! */
668671
ExecRowMark *erm = ExecFindRowMark(estate, scanrelid, true);
669672

670-
if (erm != NULL && erm->relation != NULL)
671-
lockmode = NoLock;
673+
if (erm != NULL)
674+
{
675+
if (erm->markType == ROW_MARK_REFERENCE ||
676+
erm->markType == ROW_MARK_COPY)
677+
lockmode = AccessShareLock;
678+
else
679+
lockmode = RowShareLock;
680+
}
672681
}
673682

683+
/* lockmode per above logic must not be more than we previously acquired */
684+
Assert(lockmode <= rt_fetch(scanrelid, estate->es_range_table)->rellockmode);
685+
674686
/* Open the relation and acquire lock as needed */
675687
reloid = getrelid(scanrelid, estate->es_range_table);
676688
rel = heap_open(reloid, lockmode);

‎src/backend/nodes/copyfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2356,6 +2356,7 @@ _copyRangeTblEntry(const RangeTblEntry *from)
23562356
COPY_SCALAR_FIELD(rtekind);
23572357
COPY_SCALAR_FIELD(relid);
23582358
COPY_SCALAR_FIELD(relkind);
2359+
COPY_SCALAR_FIELD(rellockmode);
23592360
COPY_NODE_FIELD(tablesample);
23602361
COPY_NODE_FIELD(subquery);
23612362
COPY_SCALAR_FIELD(security_barrier);

‎src/backend/nodes/equalfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2630,6 +2630,7 @@ _equalRangeTblEntry(const RangeTblEntry *a, const RangeTblEntry *b)
26302630
COMPARE_SCALAR_FIELD(rtekind);
26312631
COMPARE_SCALAR_FIELD(relid);
26322632
COMPARE_SCALAR_FIELD(relkind);
2633+
COMPARE_SCALAR_FIELD(rellockmode);
26332634
COMPARE_NODE_FIELD(tablesample);
26342635
COMPARE_NODE_FIELD(subquery);
26352636
COMPARE_SCALAR_FIELD(security_barrier);

‎src/backend/nodes/outfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3131,6 +3131,7 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
31313131
case RTE_RELATION:
31323132
WRITE_OID_FIELD(relid);
31333133
WRITE_CHAR_FIELD(relkind);
3134+
WRITE_INT_FIELD(rellockmode);
31343135
WRITE_NODE_FIELD(tablesample);
31353136
break;
31363137
case RTE_SUBQUERY:

‎src/backend/nodes/readfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1361,6 +1361,7 @@ _readRangeTblEntry(void)
13611361
case RTE_RELATION:
13621362
READ_OID_FIELD(relid);
13631363
READ_CHAR_FIELD(relkind);
1364+
READ_INT_FIELD(rellockmode);
13641365
READ_NODE_FIELD(tablesample);
13651366
break;
13661367
case RTE_SUBQUERY:

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6021,6 +6021,7 @@ plan_cluster_use_sort(Oid tableOid, Oid indexOid)
60216021
rte->rtekind = RTE_RELATION;
60226022
rte->relid = tableOid;
60236023
rte->relkind = RELKIND_RELATION; /* Don't be too picky. */
6024+
rte->rellockmode = AccessShareLock;
60246025
rte->lateral = false;
60256026
rte->inh = false;
60266027
rte->inFromCl = true;
@@ -6143,6 +6144,7 @@ plan_create_index_workers(Oid tableOid, Oid indexOid)
61436144
rte->rtekind = RTE_RELATION;
61446145
rte->relid = tableOid;
61456146
rte->relkind = RELKIND_RELATION; /* Don't be too picky. */
6147+
rte->rellockmode = AccessShareLock;
61466148
rte->lateral = false;
61476149
rte->inh = true;
61486150
rte->inFromCl = true;

‎src/backend/parser/analyze.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1037,6 +1037,7 @@ transformOnConflictClause(ParseState *pstate,
10371037
*/
10381038
exclRte = addRangeTableEntryForRelation(pstate,
10391039
targetrel,
1040+
RowExclusiveLock,
10401041
makeAlias("excluded", NIL),
10411042
false, false);
10421043
exclRte->relkind = RELKIND_COMPOSITE_TYPE;

‎src/backend/parser/parse_clause.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,7 @@ setTargetTable(ParseState *pstate, RangeVar *relation,
217217
* Now build an RTE.
218218
*/
219219
rte = addRangeTableEntryForRelation(pstate, pstate->p_target_relation,
220+
RowExclusiveLock,
220221
relation->alias, inh, false);
221222
pstate->p_target_rangetblentry = rte;
222223

‎src/backend/parser/parse_relation.c

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1207,16 +1207,23 @@ addRangeTableEntry(ParseState *pstate,
12071207
rte->rtekind = RTE_RELATION;
12081208
rte->alias = alias;
12091209

1210+
/*
1211+
* Identify the type of lock we'll need on this relation. It's not the
1212+
* query's target table (that case is handled elsewhere), so we need
1213+
* either RowShareLock if it's locked by FOR UPDATE/SHARE, or plain
1214+
* AccessShareLock otherwise.
1215+
*/
1216+
lockmode = isLockedRefname(pstate, refname) ? RowShareLock : AccessShareLock;
1217+
12101218
/*
12111219
* Get the rel's OID. This access also ensures that we have an up-to-date
12121220
* relcache entry for the rel. Since this is typically the first access
1213-
* to a rel in a statement, be careful to get the right access level
1214-
* depending on whether we're doing SELECT FOR UPDATE/SHARE.
1221+
* to a rel in a statement, we must open the rel with the proper lockmode.
12151222
*/
1216-
lockmode = isLockedRefname(pstate, refname) ? RowShareLock : AccessShareLock;
12171223
rel = parserOpenTable(pstate, relation, lockmode);
12181224
rte->relid = RelationGetRelid(rel);
12191225
rte->relkind = rel->rd_rel->relkind;
1226+
rte->rellockmode = lockmode;
12201227

12211228
/*
12221229
* Build the list of effective column names using user-supplied aliases
@@ -1262,10 +1269,20 @@ addRangeTableEntry(ParseState *pstate,
12621269
*
12631270
* This is just like addRangeTableEntry() except that it makes an RTE
12641271
* given an already-open relation instead of a RangeVar reference.
1272+
*
1273+
* lockmode is the lock type required for query execution; it must be one
1274+
* of AccessShareLock, RowShareLock, or RowExclusiveLock depending on the
1275+
* RTE's role within the query. The caller should always hold that lock mode
1276+
* or a stronger one.
1277+
*
1278+
* Note: properly, lockmode should be declared LOCKMODE not int, but that
1279+
* would require importing storage/lock.h into parse_relation.h. Since
1280+
* LOCKMODE is typedef'd as int anyway, that seems like overkill.
12651281
*/
12661282
RangeTblEntry *
12671283
addRangeTableEntryForRelation(ParseState *pstate,
12681284
Relation rel,
1285+
int lockmode,
12691286
Alias *alias,
12701287
bool inh,
12711288
bool inFromCl)
@@ -1275,10 +1292,15 @@ addRangeTableEntryForRelation(ParseState *pstate,
12751292

12761293
Assert(pstate != NULL);
12771294

1295+
Assert(lockmode == AccessShareLock ||
1296+
lockmode == RowShareLock ||
1297+
lockmode == RowExclusiveLock);
1298+
12781299
rte->rtekind = RTE_RELATION;
12791300
rte->alias = alias;
12801301
rte->relid = RelationGetRelid(rel);
12811302
rte->relkind = rel->rd_rel->relkind;
1303+
rte->rellockmode = lockmode;
12821304

12831305
/*
12841306
* Build the list of effective column names using user-supplied aliases

0 commit comments

Comments
 (0)