Skip to content

Commit 1b4d280

Browse files
tglsfdctglsfdc
committed
Get rid of the "new" and "old" entries in a view's rangetable.
The rule system needs "old" and/or "new" pseudo-RTEs in rule actions that are ON INSERT/UPDATE/DELETE. Historically it's put such entries into the ON SELECT rules of views as well, but those are really quite vestigial. The only thing we've used them for is to carry the view's relid forward to AcquireExecutorLocks (so that we can re-lock the view to verify it hasn't changed before re-using a plan) and to carry its relid and permissions data forward to execution-time permissions checks. What we can do instead of that is to retain these fields of the RTE_RELATION RTE for the view even after we convert it to an RTE_SUBQUERY RTE. This requires a tiny amount of extra complication in the planner and AcquireExecutorLocks, but on the other hand we can get rid of the logic that moves that data from one place to another. The principal immediate benefit of doing this, aside from a small saving in the pg_rewrite data for views, is that these pseudo-RTEs no longer trigger ruleutils.c's heuristic about qualifying variable names when the rangetable's length is more than 1. That results in quite a number of small simplifications in regression test outputs, which are all to the good IMO. Bump catversion because we need to dump a few more fields of RTE_SUBQUERY RTEs. While those will always be zeroes anyway in stored rules (because we'd never populate them until query rewrite) they are useful for debugging, and it seems like we'd better make sure to transmit such RTEs accurately in plans sent to parallel workers. I don't think the executor actually examines these fields after startup, but someday it might. Amit Langote Discussion: https://postgr.es/m/CA+HiwqEf7gPN4Hn+LoZ4tP2q_Qt7n3vw7-6fJKOf92tSEnX6Gg@mail.gmail.com
1 parent 2ff5ca8 commit 1b4d280

34 files changed

+774
-890
lines changed

‎contrib/postgres_fdw/expected/postgres_fdw.out

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2606,7 +2606,7 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN v5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1
26062606
Foreign Scan
26072607
Output: ft4.c1, ft5.c2, ft5.c1
26082608
Relations: (public.ft4) LEFT JOIN (public.ft5)
2609-
Remote SQL: SELECT r6.c1, r9.c2, r9.c1 FROM ("S 1"."T 3" r6 LEFT JOIN "S 1"."T 4" r9 ON (((r6.c1 = r9.c1)))) ORDER BY r6.c1 ASC NULLS LAST, r9.c1 ASC NULLS LAST LIMIT 10::bigint OFFSET 10::bigint
2609+
Remote SQL: SELECT r4.c1, r5.c2, r5.c1 FROM ("S 1"."T 3" r4 LEFT JOIN "S 1"."T 4" r5 ON (((r4.c1 = r5.c1)))) ORDER BY r4.c1 ASC NULLS LAST, r5.c1 ASC NULLS LAST LIMIT 10::bigint OFFSET 10::bigint
26102610
(4 rows)
26112611

26122612
SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN v5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 10;
@@ -2669,7 +2669,7 @@ SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c
26692669
Foreign Scan
26702670
Output: ft4.c1, t2.c2, t2.c1
26712671
Relations: (public.ft4) LEFT JOIN (public.ft5 t2)
2672-
Remote SQL: SELECT r6.c1, r2.c2, r2.c1 FROM ("S 1"."T 3" r6 LEFT JOIN "S 1"."T 4" r2 ON (((r6.c1 = r2.c1)))) ORDER BY r6.c1 ASC NULLS LAST, r2.c1 ASC NULLS LAST LIMIT 10::bigint OFFSET 10::bigint
2672+
Remote SQL: SELECT r4.c1, r2.c2, r2.c1 FROM ("S 1"."T 3" r4 LEFT JOIN "S 1"."T 4" r2 ON (((r4.c1 = r2.c1)))) ORDER BY r4.c1 ASC NULLS LAST, r2.c1 ASC NULLS LAST LIMIT 10::bigint OFFSET 10::bigint
26732673
(4 rows)
26742674

26752675
SELECT t1.c1, t2.c2 FROM v4 t1 LEFT JOIN ft5 t2 ON (t1.c1 = t2.c1) ORDER BY t1.c1, t2.c1 OFFSET 10 LIMIT 10;
@@ -6557,10 +6557,10 @@ CREATE VIEW rw_view AS SELECT * FROM foreign_tbl
65576557
a | integer | | | | plain |
65586558
b | integer | | | | plain |
65596559
View definition:
6560-
SELECT foreign_tbl.a,
6561-
foreign_tbl.b
6560+
SELECT a,
6561+
b
65626562
FROM foreign_tbl
6563-
WHERE foreign_tbl.a < foreign_tbl.b;
6563+
WHERE a < b;
65646564
Options: check_option=cascaded
65656565

65666566
EXPLAIN (VERBOSE, COSTS OFF)
@@ -6674,10 +6674,10 @@ CREATE VIEW rw_view AS SELECT * FROM parent_tbl
66746674
a | integer | | | | plain |
66756675
b | integer | | | | plain |
66766676
View definition:
6677-
SELECT parent_tbl.a,
6678-
parent_tbl.b
6677+
SELECT a,
6678+
b
66796679
FROM parent_tbl
6680-
WHERE parent_tbl.a < parent_tbl.b;
6680+
WHERE a < b;
66816681
Options: check_option=cascaded
66826682

66836683
EXPLAIN (VERBOSE, COSTS OFF)

‎src/backend/commands/lockcmds.c

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -194,15 +194,6 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context *context)
194194
char relkind = rte->relkind;
195195
char *relname = get_rel_name(relid);
196196

197-
/*
198-
* The OLD and NEW placeholder entries in the view's rtable are
199-
* skipped.
200-
*/
201-
if (relid == context->viewoid &&
202-
(strcmp(rte->eref->aliasname, "old") == 0 ||
203-
strcmp(rte->eref->aliasname, "new") == 0))
204-
continue;
205-
206197
/* Currently, we only allow plain tables or views to be locked. */
207198
if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE &&
208199
relkind != RELKIND_VIEW)

‎src/backend/commands/view.c

Lines changed: 0 additions & 107 deletions
Original file line numberDiff line numberDiff line change
@@ -353,107 +353,6 @@ DefineViewRules(Oid viewOid, Query *viewParse, bool replace)
353353
*/
354354
}
355355

356-
/*---------------------------------------------------------------
357-
* UpdateRangeTableOfViewParse
358-
*
359-
* Update the range table of the given parsetree.
360-
* This update consists of adding two new entries IN THE BEGINNING
361-
* of the range table (otherwise the rule system will die a slow,
362-
* horrible and painful death, and we do not want that now, do we?)
363-
* one for the OLD relation and one for the NEW one (both of
364-
* them refer in fact to the "view" relation).
365-
*
366-
* Of course we must also increase the 'varnos' of all the Var nodes
367-
* by 2...
368-
*
369-
* These extra RT entries are not actually used in the query,
370-
* except for run-time locking.
371-
*---------------------------------------------------------------
372-
*/
373-
static Query *
374-
UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse)
375-
{
376-
Relation viewRel;
377-
List *new_rt;
378-
ParseNamespaceItem *nsitem;
379-
RangeTblEntry *rt_entry1,
380-
*rt_entry2;
381-
RTEPermissionInfo *rte_perminfo1;
382-
ParseState *pstate;
383-
ListCell *lc;
384-
385-
/*
386-
* Make a copy of the given parsetree. It's not so much that we don't
387-
* want to scribble on our input, it's that the parser has a bad habit of
388-
* outputting multiple links to the same subtree for constructs like
389-
* BETWEEN, and we mustn't have OffsetVarNodes increment the varno of a
390-
* Var node twice. copyObject will expand any multiply-referenced subtree
391-
* into multiple copies.
392-
*/
393-
viewParse = copyObject(viewParse);
394-
395-
/* Create a dummy ParseState for addRangeTableEntryForRelation */
396-
pstate = make_parsestate(NULL);
397-
398-
/* need to open the rel for addRangeTableEntryForRelation */
399-
viewRel = relation_open(viewOid, AccessShareLock);
400-
401-
/*
402-
* Create the 2 new range table entries and form the new range table...
403-
* OLD first, then NEW....
404-
*/
405-
nsitem = addRangeTableEntryForRelation(pstate, viewRel,
406-
AccessShareLock,
407-
makeAlias("old", NIL),
408-
false, false);
409-
rt_entry1 = nsitem->p_rte;
410-
rte_perminfo1 = nsitem->p_perminfo;
411-
nsitem = addRangeTableEntryForRelation(pstate, viewRel,
412-
AccessShareLock,
413-
makeAlias("new", NIL),
414-
false, false);
415-
rt_entry2 = nsitem->p_rte;
416-
417-
/*
418-
* Add only the "old" RTEPermissionInfo at the head of view query's list
419-
* and update the other RTEs' perminfoindex accordingly. When rewriting a
420-
* query on the view, ApplyRetrieveRule() will transfer the view
421-
* relation's permission details into this RTEPermissionInfo. That's
422-
* needed because the view's RTE itself will be transposed into a subquery
423-
* RTE that can't carry the permission details; see the code stanza toward
424-
* the end of ApplyRetrieveRule() for how that's done.
425-
*/
426-
viewParse->rteperminfos = lcons(rte_perminfo1, viewParse->rteperminfos);
427-
foreach(lc, viewParse->rtable)
428-
{
429-
RangeTblEntry *rte = lfirst(lc);
430-
431-
if (rte->perminfoindex > 0)
432-
rte->perminfoindex += 1;
433-
}
434-
435-
/*
436-
* Also make the "new" RTE's RTEPermissionInfo undiscoverable. This is a
437-
* bit of a hack given that all the non-child RTE_RELATION entries really
438-
* should have a RTEPermissionInfo, but this dummy "new" RTE is going to
439-
* go away anyway in the very near future.
440-
*/
441-
rt_entry2->perminfoindex = 0;
442-
443-
new_rt = lcons(rt_entry1, lcons(rt_entry2, viewParse->rtable));
444-
445-
viewParse->rtable = new_rt;
446-
447-
/*
448-
* Now offset all var nodes by 2, and jointree RT indexes too.
449-
*/
450-
OffsetVarNodes((Node *) viewParse, 2, 0);
451-
452-
relation_close(viewRel, AccessShareLock);
453-
454-
return viewParse;
455-
}
456-
457356
/*
458357
* DefineView
459358
* Execute a CREATE VIEW command.
@@ -616,12 +515,6 @@ DefineView(ViewStmt *stmt, const char *queryString,
616515
void
617516
StoreViewQuery(Oid viewOid, Query *viewParse, bool replace)
618517
{
619-
/*
620-
* The range table of 'viewParse' does not contain entries for the "OLD"
621-
* and "NEW" relations. So... add them!
622-
*/
623-
viewParse = UpdateRangeTableOfViewParse(viewOid, viewParse);
624-
625518
/*
626519
* Now create the rules associated with the view.
627520
*/

‎src/backend/nodes/outfuncs.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -512,6 +512,10 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
512512
case RTE_SUBQUERY:
513513
WRITE_NODE_FIELD(subquery);
514514
WRITE_BOOL_FIELD(security_barrier);
515+
/* we re-use these RELATION fields, too: */
516+
WRITE_OID_FIELD(relid);
517+
WRITE_INT_FIELD(rellockmode);
518+
WRITE_UINT_FIELD(perminfoindex);
515519
break;
516520
case RTE_JOIN:
517521
WRITE_ENUM_FIELD(jointype, JoinType);
@@ -545,10 +549,11 @@ _outRangeTblEntry(StringInfo str, const RangeTblEntry *node)
545549
case RTE_NAMEDTUPLESTORE:
546550
WRITE_STRING_FIELD(enrname);
547551
WRITE_FLOAT_FIELD(enrtuples);
548-
WRITE_OID_FIELD(relid);
549552
WRITE_NODE_FIELD(coltypes);
550553
WRITE_NODE_FIELD(coltypmods);
551554
WRITE_NODE_FIELD(colcollations);
555+
/* we re-use these RELATION fields, too: */
556+
WRITE_OID_FIELD(relid);
552557
break;
553558
case RTE_RESULT:
554559
/* no extra fields */

‎src/backend/nodes/readfuncs.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -478,6 +478,10 @@ _readRangeTblEntry(void)
478478
case RTE_SUBQUERY:
479479
READ_NODE_FIELD(subquery);
480480
READ_BOOL_FIELD(security_barrier);
481+
/* we re-use these RELATION fields, too: */
482+
READ_OID_FIELD(relid);
483+
READ_INT_FIELD(rellockmode);
484+
READ_UINT_FIELD(perminfoindex);
481485
break;
482486
case RTE_JOIN:
483487
READ_ENUM_FIELD(jointype, JoinType);
@@ -520,10 +524,11 @@ _readRangeTblEntry(void)
520524
case RTE_NAMEDTUPLESTORE:
521525
READ_STRING_FIELD(enrname);
522526
READ_FLOAT_FIELD(enrtuples);
523-
READ_OID_FIELD(relid);
524527
READ_NODE_FIELD(coltypes);
525528
READ_NODE_FIELD(coltypmods);
526529
READ_NODE_FIELD(colcollations);
530+
/* we re-use these RELATION fields, too: */
531+
READ_OID_FIELD(relid);
527532
break;
528533
case RTE_RESULT:
529534
/* no extra fields */

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

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -405,13 +405,15 @@ add_rtes_to_flat_rtable(PlannerInfo *root, bool recursing)
405405
*
406406
* At top level, we must add all RTEs so that their indexes in the
407407
* flattened rangetable match up with their original indexes. When
408-
* recursing, we only care about extracting relation RTEs.
408+
* recursing, we only care about extracting relation RTEs (and subquery
409+
* RTEs that were once relation RTEs).
409410
*/
410411
foreach(lc, root->parse->rtable)
411412
{
412413
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
413414

414-
if (!recursing || rte->rtekind == RTE_RELATION)
415+
if (!recursing || rte->rtekind == RTE_RELATION ||
416+
(rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)))
415417
add_rte_to_flat_rtable(glob, root->parse->rteperminfos, rte);
416418
}
417419

@@ -501,8 +503,9 @@ flatten_rtes_walker(Node *node, flatten_rtes_walker_context *cxt)
501503
{
502504
RangeTblEntry *rte = (RangeTblEntry *) node;
503505

504-
/* As above, we need only save relation RTEs */
505-
if (rte->rtekind == RTE_RELATION)
506+
/* As above, we need only save relation RTEs and former relations */
507+
if (rte->rtekind == RTE_RELATION ||
508+
(rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)))
506509
add_rte_to_flat_rtable(cxt->glob, cxt->query->rteperminfos, rte);
507510
return false;
508511
}
@@ -560,7 +563,8 @@ add_rte_to_flat_rtable(PlannerGlobal *glob, List *rteperminfos,
560563
glob->finalrtable = lappend(glob->finalrtable, newrte);
561564

562565
/*
563-
* If it's a plain relation RTE, add the table to relationOids.
566+
* If it's a plain relation RTE (or a subquery that was once a view
567+
* reference), add the relation OID to relationOids.
564568
*
565569
* We do this even though the RTE might be unreferenced in the plan tree;
566570
* this would correspond to cases such as views that were expanded, child
@@ -570,7 +574,8 @@ add_rte_to_flat_rtable(PlannerGlobal *glob, List *rteperminfos,
570574
* Note we don't bother to avoid making duplicate list entries. We could,
571575
* but it would probably cost more cycles than it would save.
572576
*/
573-
if (newrte->rtekind == RTE_RELATION)
577+
if (newrte->rtekind == RTE_RELATION ||
578+
(newrte->rtekind == RTE_SUBQUERY && OidIsValid(newrte->relid)))
574579
glob->relationOids = lappend_oid(glob->relationOids, newrte->relid);
575580

576581
/*
@@ -3403,14 +3408,11 @@ extract_query_dependencies_walker(Node *node, PlannerInfo *context)
34033408
{
34043409
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc);
34053410

3406-
if (rte->rtekind == RTE_RELATION)
3411+
if (rte->rtekind == RTE_RELATION ||
3412+
(rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid)) ||
3413+
(rte->rtekind == RTE_NAMEDTUPLESTORE && OidIsValid(rte->relid)))
34073414
context->glob->relationOids =
34083415
lappend_oid(context->glob->relationOids, rte->relid);
3409-
else if (rte->rtekind == RTE_NAMEDTUPLESTORE &&
3410-
OidIsValid(rte->relid))
3411-
context->glob->relationOids =
3412-
lappend_oid(context->glob->relationOids,
3413-
rte->relid);
34143416
}
34153417

34163418
/* And recurse into the query's subexpressions */

‎src/backend/parser/parse_relation.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3834,7 +3834,7 @@ addRTEPermissionInfo(List **rteperminfos, RangeTblEntry *rte)
38343834
{
38353835
RTEPermissionInfo *perminfo;
38363836

3837-
Assert(rte->rtekind == RTE_RELATION);
3837+
Assert(OidIsValid(rte->relid));
38383838
Assert(rte->perminfoindex == 0);
38393839

38403840
/* Nope, so make one and add to the list. */

‎src/backend/rewrite/rewriteDefine.c

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -633,13 +633,6 @@ checkRuleResultList(List *targetList, TupleDesc resultDesc, bool isSelect,
633633
* setRuleCheckAsUser
634634
* Recursively scan a query or expression tree and set the checkAsUser
635635
* field to the given userid in all RTEPermissionInfos of the query.
636-
*
637-
* Note: for a view (ON SELECT rule), the checkAsUser field of the OLD
638-
* RTE entry's RTEPermissionInfo will be overridden when the view rule is
639-
* expanded, and the checkAsUser for the NEW RTE entry's RTEPermissionInfo is
640-
* irrelevant because its requiredPerms bits will always be zero. However, for
641-
* other types of rules it's important to set these fields to match the rule
642-
* owner. So we just set them always.
643636
*/
644637
void
645638
setRuleCheckAsUser(Node *node, Oid userid)

‎src/backend/rewrite/rewriteHandler.c

Lines changed: 12 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -1715,10 +1715,7 @@ ApplyRetrieveRule(Query *parsetree,
17151715
List *activeRIRs)
17161716
{
17171717
Query *rule_action;
1718-
RangeTblEntry *rte,
1719-
*subrte;
1720-
RTEPermissionInfo *perminfo,
1721-
*sub_perminfo;
1718+
RangeTblEntry *rte;
17221719
RowMarkClause *rc;
17231720

17241721
if (list_length(rule->actions) != 1)
@@ -1830,32 +1827,20 @@ ApplyRetrieveRule(Query *parsetree,
18301827
* original RTE to a subquery RTE.
18311828
*/
18321829
rte = rt_fetch(rt_index, parsetree->rtable);
1833-
perminfo = getRTEPermissionInfo(parsetree->rteperminfos, rte);
18341830

18351831
rte->rtekind = RTE_SUBQUERY;
18361832
rte->subquery = rule_action;
18371833
rte->security_barrier = RelationIsSecurityView(relation);
1838-
/* Clear fields that should not be set in a subquery RTE */
1839-
rte->relid = InvalidOid;
1840-
rte->relkind = 0;
1841-
rte->rellockmode = 0;
1842-
rte->tablesample = NULL;
1843-
rte->perminfoindex = 0; /* no permission checking for this RTE */
1844-
rte->inh = false; /* must not be set for a subquery */
18451834

18461835
/*
1847-
* We move the view's permission check data down to its RTEPermissionInfo
1848-
* contained in the view query, which the OLD entry in its range table
1849-
* points to.
1836+
* Clear fields that should not be set in a subquery RTE. Note that we
1837+
* leave the relid, rellockmode, and perminfoindex fields set, so that the
1838+
* view relation can be appropriately locked before execution and its
1839+
* permissions checked.
18501840
*/
1851-
subrte = rt_fetch(PRS2_OLD_VARNO, rule_action->rtable);
1852-
Assert(subrte->relid == relation->rd_id);
1853-
sub_perminfo = getRTEPermissionInfo(rule_action->rteperminfos, subrte);
1854-
sub_perminfo->requiredPerms = perminfo->requiredPerms;
1855-
sub_perminfo->checkAsUser = perminfo->checkAsUser;
1856-
sub_perminfo->selectedCols = perminfo->selectedCols;
1857-
sub_perminfo->insertedCols = perminfo->insertedCols;
1858-
sub_perminfo->updatedCols = perminfo->updatedCols;
1841+
rte->relkind = 0;
1842+
rte->tablesample = NULL;
1843+
rte->inh = false; /* must not be set for a subquery */
18591844

18601845
return parsetree;
18611846
}
@@ -1867,9 +1852,10 @@ ApplyRetrieveRule(Query *parsetree,
18671852
* aggregate. We leave it to the planner to detect that.
18681853
*
18691854
* NB: this must agree with the parser's transformLockingClause() routine.
1870-
* However, unlike the parser we have to be careful not to mark a view's
1871-
* OLD and NEW rels for updating. The best way to handle that seems to be
1872-
* to scan the jointree to determine which rels are used.
1855+
* However, we used to have to avoid marking a view's OLD and NEW rels for
1856+
* updating, which motivated scanning the jointree to determine which rels
1857+
* are used. Possibly that could now be simplified into just scanning the
1858+
* rangetable as the parser does.
18731859
*/
18741860
static void
18751861
markQueryForLocking(Query *qry, Node *jtnode,

‎src/backend/utils/cache/plancache.c

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1769,7 +1769,8 @@ AcquireExecutorLocks(List *stmt_list, bool acquire)
17691769
{
17701770
RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc2);
17711771

1772-
if (rte->rtekind != RTE_RELATION)
1772+
if (!(rte->rtekind == RTE_RELATION ||
1773+
(rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid))))
17731774
continue;
17741775

17751776
/*

0 commit comments

Comments
 (0)