Re: on placeholder entries in view rule action query's range table

On Thu, Jan 12, 2023 at 10:06 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > On Mon, Jan 9, 2023 at 5:58 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Conceivably we could make it work by allowing RTE_SUBQUERY RTEs to
> >> carry a relation OID and associated RTEPermissionInfo, so that when a
> >> view's RTE_RELATION RTE is transmuted to an RTE_SUBQUERY RTE it still
> >> carries the info needed to let us lock and permission-check the view.
> >> That might be a bridge too far from the ugliness perspective ...
> >> although certainly the business with OLD and/or NEW RTEs isn't very
> >> pretty either.
>
> > I had thought about that idea but was a bit scared of trying it,
> > because it does sound like something that might become a maintenance
> > burden in the future. Though I gave that a try today given that it
> > sounds like I may have your permission. ;-)
>
> Given the small number of places that need to be touched, I don't
> think it's a maintenance problem. I agree with your fear that you
> might have missed some, but I also searched and found no more.

OK, thanks.

> > I was
> > surprised that nothing failed with a -DWRITE_READ_PARSE_PLAN_TREES
> > build, because of the way RTEs are written and read -- relid,
> > rellockmode are not written/read for RTE_SUBQUERY RTEs.
>
> I think that's mostly accidental, stemming from the facts that:
> (1) Stored rules wouldn't have these fields populated yet anyway.
> (2) The regression tests haven't got any good way to check that a
> needed lock was actually acquired. It looks to me like with the
> patch as you have it, when a plan tree is copied into the plan
> cache the view relid is lost (if pg_plan_query stripped it thanks
> to -DWRITE_READ_PARSE_PLAN_TREES) and thus we won't re-acquire the
> view lock in AcquireExecutorLocks during later plan uses. But
> that would have no useful effect unless it forced a re-plan due to
> a concurrent view replacement, which is a scenario I'm pretty sure
> we don't actually exercise in the tests.

Ah, does it make sense to have isolation tests cover this?

> (3) The executor doesn't look at these fields after startup, so
> failure to transmit them to parallel workers doesn't hurt.
>
> In any case, it would clearly be very foolish not to fix
> outfuncs/readfuncs to preserve all the fields we're using.
>
> I've pushed this with some cleanup --- aside from fixing
> outfuncs/readfuncs, I did some more work on the comments, which
> I think you were too sloppy about.

Thanks a lot for the fixes.

> Sadly, the original nested-views test case still has O(N^2)
> planning time :-(. I dug into that harder and now see where
> the problem really lies. The rewriter recursively replaces
> the view RTE_RELATION RTEs with RTE_SUBQUERY all the way down,
> which takes it only linear time. However, then we have a deep
> nest of RTE_SUBQUERYs, and the initial copyObject in
> pull_up_simple_subquery repeatedly copies everything below the
> current pullup recursion level, so that it's still O(N^2)
> even though the final rtable will have only N entries.

That makes sense.

> I'm afraid to remove the copyObject step, because that would
> cause problems in the cases where we try to perform pullup
> and have to abandon it later. (Maybe we could get rid of
> all such cases, but I'm not sanguine about that succeeding.)
> I'm tempted to try to fix it by taking view replacement out
> of the rewriter altogether and making prepjointree.c handle
> it during the same recursive scan that does subquery pullup,
> so that we aren't applying copyObject to already-expanded
> RTE_SUBQUERY nests. However, that's more work than I care to
> put into the problem right now.

OK, I will try to give your idea a shot sometime later.

BTW, I noticed that we could perhaps remove the following in the
fireRIRrules()'s loop that calls ApplyRetrieveRule(), because we no
longer put any unreferenced OLD/NEW RTEs in the view queries.

/*
* If the table is not referenced in the query, then we ignore it.
* This prevents infinite expansion loop due to new rtable entries
* inserted by expansion of a rule. A table is referenced if it is
* part of the join set (a source table), or is referenced by any Var
* nodes, or is the result table.
*/
if (rt_index != parsetree->resultRelation &&
!rangeTableEntry_used((Node *) parsetree, rt_index, 0))
continue;

Commenting this out doesn't break make check.

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