Re: ExecRTCheckPerms() and many prunable partitions

On Thu, Jul 29, 2021 at 5:40 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Jul 2, 2021 at 9:40 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Jul 2, 2021 at 12:45 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > Perhaps, if we separated the rtable from the required-permissions data
> > > structure, then we could avoid pulling up otherwise-useless RTEs when
> > > flattening a view (or even better, not make the extra RTEs in the
> > > first place??), and thus possibly avoid that exponential planning-time
> > > growth for nested views.
>
> Think I've managed to get the first part done -- getting the
> permission-checking info out of the range table -- but have not
> seriously attempted the second -- doing away with the OLD/NEW range
> table entries in the view/rule action queries, assuming that is what
> you meant in the quoted.

I took a stab at the 2nd part, implemented in the attached 0002.

The patch removes UpdateRangeTableOfViewParse() which would add the
dummy OLD/NEW entries to a view rule's action query's rtable, citing
these reasons:

- * These extra RT entries are not actually used in the query,
- * except for run-time locking and permission checking.

0001 makes them unnecessary for permission checking.  Though, a
RELATION-kind RTE still be must be present in the rtable for run-time
locking, so I adjusted ApplyRetrieveRule() as follows:

@@ -1803,16 +1804,26 @@ ApplyRetrieveRule(Query *parsetree,
     * original RTE to a subquery RTE.
     */
    rte = rt_fetch(rt_index, parsetree->rtable);
+   subquery_rte = rte;

-   rte->rtekind = RTE_SUBQUERY;
-   rte->subquery = rule_action;
-   rte->security_barrier = RelationIsSecurityView(relation);
+   /*
+    * Before modifying, store a copy of itself so as to serve as the entry
+    * to be used by the executor to lock the view relation and for the
+    * planner to be able to record the view relation OID in the PlannedStmt
+    * that it produces for the query.
+    */
+   rte = copyObject(rte);
+   parsetree->rtable = lappend(parsetree->rtable, rte);
+
+   subquery_rte->rtekind = RTE_SUBQUERY;
+   subquery_rte->subquery = rule_action;
+   subquery_rte->security_barrier = RelationIsSecurityView(relation);
    /* Clear fields that should not be set in a subquery RTE */
-   rte->relid = InvalidOid;
-   rte->relkind = 0;
-   rte->rellockmode = 0;
-   rte->tablesample = NULL;
-   rte->inh = false;           /* must not be set for a subquery */
+   subquery_rte->relid = InvalidOid;
+   subquery_rte->relkind = 0;
+   subquery_rte->rellockmode = 0;
+   subquery_rte->tablesample = NULL;
+   subquery_rte->inh = false;          /* must not be set for a subquery */

    return parsetree;
 }

Outputs for a bunch of regression tests needed to be adjusted to
account for that pg_get_viewdef() no longer qualifies view column
names in the deparsed queries, that is, if they reference only a
single relation.   Previously, those dummy OLD/NEW entries tricked
make_ruledef(), get_query_def() et al into setting
deparse_context.varprefix to true.  contrib/postgre_fdw test output
likewise needed adjustment due to its deparse code being impacted by
those dummy entries no longer being present, I believe.

I haven't yet checked how this further improves the performance for
the case discussed at [1] that prompted this.

-- 
Amit Langote
EDB: http://www.enterprisedb.com

[1] https://www.postgresql.org/message-id/flat/797aff54-b49b-4914-9ff9-aa42564a4d7d%40www.fastmail.com