From 220cc278ab4fc7757231a591dcb505be43b93857 Mon Sep 17 00:00:00 2001 From: amitlan Date: Mon, 21 Nov 2022 15:27:56 +0900 Subject: [PATCH v27 4/5] Do not add the NEW entry to view rule action's range table The OLD entry suffices as a placeholder for the view relation when it is queried, such as checking its permissions during a query's execution, but the NEW entry has no role to play whatsoever. Because there are now fewer entries in the view query's range table, this change affects how the deparsed queries look, where the output of deparsing (such as alias names) depends on using RT indexs and the original query references a view. To wit, some postgres_fdw regression tests whose expected output changes as a result of this have been are updated to match. --- .../postgres_fdw/expected/postgres_fdw.out | 4 +- src/backend/commands/lockcmds.c | 6 +- src/backend/commands/view.c | 74 +++++++------------ src/backend/rewrite/rewriteDefine.c | 6 +- src/backend/rewrite/rewriteHandler.c | 4 +- 5 files changed, 35 insertions(+), 59 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 558e94b845..a84176cf65 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -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 Foreign Scan Output: ft4.c1, ft5.c2, ft5.c1 Relations: (public.ft4) LEFT JOIN (public.ft5) - 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 + Remote SQL: SELECT r5.c1, r7.c2, r7.c1 FROM ("S 1"."T 3" r5 LEFT JOIN "S 1"."T 4" r7 ON (((r5.c1 = r7.c1)))) ORDER BY r5.c1 ASC NULLS LAST, r7.c1 ASC NULLS LAST LIMIT 10::bigint OFFSET 10::bigint (4 rows) 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 Foreign Scan Output: ft4.c1, t2.c2, t2.c1 Relations: (public.ft4) LEFT JOIN (public.ft5 t2) - 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 + Remote SQL: SELECT r5.c1, r2.c2, r2.c1 FROM ("S 1"."T 3" r5 LEFT JOIN "S 1"."T 4" r2 ON (((r5.c1 = r2.c1)))) ORDER BY r5.c1 ASC NULLS LAST, r2.c1 ASC NULLS LAST LIMIT 10::bigint OFFSET 10::bigint (4 rows) 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; diff --git a/src/backend/commands/lockcmds.c b/src/backend/commands/lockcmds.c index b0747ce291..ce0e6ac112 100644 --- a/src/backend/commands/lockcmds.c +++ b/src/backend/commands/lockcmds.c @@ -195,12 +195,10 @@ LockViewRecurse_walker(Node *node, LockViewRecurse_context *context) char *relname = get_rel_name(relid); /* - * The OLD and NEW placeholder entries in the view's rtable are - * skipped. + * The OLD placeholder entry in the view's rtable is skipped. */ if (relid == context->viewoid && - (strcmp(rte->eref->aliasname, "old") == 0 || - strcmp(rte->eref->aliasname, "new") == 0)) + (strcmp(rte->eref->aliasname, "old") == 0)) continue; /* Currently, we only allow plain tables or views to be locked. */ diff --git a/src/backend/commands/view.c b/src/backend/commands/view.c index 6bb707fd51..347c797b58 100644 --- a/src/backend/commands/view.c +++ b/src/backend/commands/view.c @@ -356,29 +356,25 @@ DefineViewRules(Oid viewOid, Query *viewParse, bool replace) /*--------------------------------------------------------------- * UpdateRangeTableOfViewParse * - * Update the range table of the given parsetree. - * This update consists of adding two new entries IN THE BEGINNING - * of the range table (otherwise the rule system will die a slow, - * horrible and painful death, and we do not want that now, do we?) - * one for the OLD relation and one for the NEW one (both of - * them refer in fact to the "view" relation). + * Update the range table of the given parsetree to add a placeholder entry + * for the view relation and increase the 'varnos' of all the Var nodes + * by 1 to account for its addition. * - * Of course we must also increase the 'varnos' of all the Var nodes - * by 2... - * - * These extra RT entries are not actually used in the query, - * except for run-time locking. + * This extra RT entry for the view relation is not actually used in the query + * but it is needed so that 1) the executor can checks the permissions via the + * RTEPermissionInfo that is also added in this function, 2) the executor can + * lock the view, and 3) the planner can record the view's OID in + * PlannedStmt.relationOids such that any concurrent changes to its schema + * would invlidate the plans refencing the view. *--------------------------------------------------------------- */ static Query * UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse) { Relation viewRel; - List *new_rt; ParseNamespaceItem *nsitem; - RangeTblEntry *rt_entry1, - *rt_entry2; - RTEPermissionInfo *rte_perminfo1; + RangeTblEntry *rt_entry; + RTEPermissionInfo *rte_perminfo; ParseState *pstate; ListCell *lc; @@ -399,31 +395,25 @@ UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse) viewRel = relation_open(viewOid, AccessShareLock); /* - * Create the 2 new range table entries and form the new range table... - * OLD first, then NEW.... + * Create the new range table entry and form the new range table where + * the OLD entry is added first, followed by the entries in the view + * query's range table. */ nsitem = addRangeTableEntryForRelation(pstate, viewRel, AccessShareLock, makeAlias("old", NIL), false, false); - rt_entry1 = nsitem->p_rte; - rte_perminfo1 = nsitem->p_perminfo; - nsitem = addRangeTableEntryForRelation(pstate, viewRel, - AccessShareLock, - makeAlias("new", NIL), - false, false); - rt_entry2 = nsitem->p_rte; + rt_entry = nsitem->p_rte; + rte_perminfo = nsitem->p_perminfo; /* - * Add only the "old" RTEPermissionInfo at the head of view query's list - * and update the other RTEs' perminfoindex accordingly. When rewriting a - * query on the view, ApplyRetrieveRule() will transfer the view relation's - * permission details into this RTEPermissionInfo. That's needed because - * the view's RTE itself will be transposed into a subquery RTE that can't - * carry the permission details; see the code stanza toward the end of - * ApplyRetrieveRule() for how that's done. + * When rewriting a query on the view, ApplyRetrieveRule() will transfer + * the view relation's permission details into this RTEPermissionInfo. + * That's needed because the view's RTE itself will be transposed into a + * subquery RTE that can't carry the permission details; see the code + * stanza toward the end of ApplyRetrieveRule() for how that's done. */ - viewParse->rtepermlist = lcons(rte_perminfo1, viewParse->rtepermlist); + viewParse->rtepermlist = lcons(rte_perminfo, viewParse->rtepermlist); foreach(lc, viewParse->rtable) { RangeTblEntry *rte = lfirst(lc); @@ -431,22 +421,12 @@ UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse) if (rte->perminfoindex > 0) rte->perminfoindex += 1; } - /* - * Also make the "new" RTE's RTEPermissionInfo undiscoverable. This is bit - * of a hack given that all the non-child RTE_RELATION entries really - * should have a RTEPermissionInfo, but this dummy "new" RTE is going to - * go away anyway in the very near future. - */ - rt_entry2->perminfoindex = 0; - - new_rt = lcons(rt_entry1, lcons(rt_entry2, viewParse->rtable)); - - viewParse->rtable = new_rt; + viewParse->rtable = lcons(rt_entry, viewParse->rtable); /* - * Now offset all var nodes by 2, and jointree RT indexes too. + * Now offset all var nodes by 1, and jointree RT indexes too. */ - OffsetVarNodes((Node *) viewParse, 2, 0); + OffsetVarNodes((Node *) viewParse, 1, 0); relation_close(viewRel, AccessShareLock); @@ -616,8 +596,8 @@ void StoreViewQuery(Oid viewOid, Query *viewParse, bool replace) { /* - * The range table of 'viewParse' does not contain entries for the "OLD" - * and "NEW" relations. So... add them! + * Add a placeholder entry for the "OLD" relation to the range table of + * 'viewParse'; see the header comment for why it's needed. */ viewParse = UpdateRangeTableOfViewParse(viewOid, viewParse); diff --git a/src/backend/rewrite/rewriteDefine.c b/src/backend/rewrite/rewriteDefine.c index 3b2649f7a0..dd31bc90ae 100644 --- a/src/backend/rewrite/rewriteDefine.c +++ b/src/backend/rewrite/rewriteDefine.c @@ -801,10 +801,8 @@ checkRuleResultList(List *targetList, TupleDesc resultDesc, bool isSelect, * * Note: for a view (ON SELECT rule), the checkAsUser field of the OLD * RTE entry's RTEPermissionInfo will be overridden when the view rule is - * expanded, and the checkAsUser for the NEW RTE entry's RTEPermissionInfo is - * irrelevant because its requiredPerms bits will always be zero. However, for - * other types of rules it's important to set these fields to match the rule - * owner. So we just set them always. + * expanded. However, for other types of rules it's important to set these + * fields to match the rule owner. So we just set them always. */ void setRuleCheckAsUser(Node *node, Oid userid) diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index ac142da7b9..45d03bacbe 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -1900,8 +1900,8 @@ ApplyRetrieveRule(Query *parsetree, * * NB: this must agree with the parser's transformLockingClause() routine. * However, unlike the parser we have to be careful not to mark a view's - * OLD and NEW rels for updating. The best way to handle that seems to be - * to scan the jointree to determine which rels are used. + * OLD rel for updating. The best way to handle that seems to be to scan + * the jointree to determine which rels are used. */ static void markQueryForLocking(Query *qry, Node *jtnode, -- 2.35.3