Re: ExecRTCheckPerms() and many prunable partitions

On Thu, Jul 29, 2021 at 5:40 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> On Fri, Jul 2, 2021 at 9:40 AM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Fri, Jul 2, 2021 at 12:45 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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