From 8f7d8e8220bcb6fb7fa0ae29bdcade1927a86971 Mon Sep 17 00:00:00 2001 From: amitlan Date: Mon, 21 Nov 2022 15:27:56 +0900 Subject: [PATCH v1] 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 for checking its permissions during a query's execution, but the NEW entry has no role whatsoever, so stop adding it. With there now being fewer entries in the view query's range table, this change affects how the deparsed queries involving views look, especially in the cases where the output of deparsing depends on using RT indexs (such as automatically generated RTE alias names in postgres_fdw deparser). To wit, some postgres_fdw regression tests whose expected output changes due to this have been updated to match. --- .../postgres_fdw/expected/postgres_fdw.out | 4 +- src/backend/commands/lockcmds.c | 6 +- src/backend/commands/view.c | 81 +++++++------------ src/backend/rewrite/rewriteDefine.c | 6 +- src/backend/rewrite/rewriteHandler.c | 4 +- 5 files changed, 38 insertions(+), 63 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 2ab3f1efaa..ccf36a3f67 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 8e3c1efae4..97ad8ad663 100644 --- a/src/backend/commands/view.c +++ b/src/backend/commands/view.c @@ -356,29 +356,24 @@ 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 view relation's + * permissions via the RTEPermissionInfo that is also added in this function, + * 2) the executor can lock the view relation, and 3) the planner can record + * the view relation's OID in PlannedStmt.relationOids. *--------------------------------------------------------------- */ 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 +394,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 a placeholder RTE for the view relation named "OLD" and add it + * as the 1st entry of the new range table, followed by the entries in the + * view query's range table. Do the same for the corresponding + * RTEPermissionInfo, which means we must adjust the view query's RTEs' + * perminfoindex to cope. + * + * Note that when rewriting a query on the view, ApplyRetrieveRule() will + * transfer the view relation's permission details into this + * placeholder RTEPermissionInfo. That's needed because the view's RTE + * itself in that query will be transposed into a subquery RTE that can't + * be made to any RTEPermissionInfo; see the code stanza at the end of + * ApplyRetrieveRule() for more details. */ 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; - - /* - * 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. - */ - viewParse->rteperminfos = lcons(rte_perminfo1, viewParse->rteperminfos); + rt_entry = nsitem->p_rte; + rte_perminfo = nsitem->p_perminfo; foreach(lc, viewParse->rtable) { RangeTblEntry *rte = lfirst(lc); @@ -431,23 +420,13 @@ UpdateRangeTableOfViewParse(Oid viewOid, Query *viewParse) if (rte->perminfoindex > 0) rte->perminfoindex += 1; } + viewParse->rtable = lcons(rt_entry, viewParse->rtable); + viewParse->rteperminfos = lcons(rte_perminfo, viewParse->rteperminfos); /* - * Also make the "new" RTE's RTEPermissionInfo undiscoverable. This is a - * 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; - - /* - * 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); @@ -617,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 9f3afe965a..8bfaefd098 100644 --- a/src/backend/rewrite/rewriteDefine.c +++ b/src/backend/rewrite/rewriteDefine.c @@ -636,10 +636,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 ea56ff79c8..11b8e449bd 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -1907,8 +1907,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