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

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.

> 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.
(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.

>> BTW, I don't entirely understand why this patch is passing regression
>> tests, because it's failed to deal with numerous places that have
>> hard-wired knowledge about these extra RTEs. Look for references to
>> PRS2_OLD_VARNO and PRS2_NEW_VARNO.

> AFAICS, the places that still have hard-wired knowledge of these
> placeholder RTEs only manipulate non-SELECT rules, so don't care about
> views.

Yeah, I looked through them too and didn't find any problems.

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.

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.

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.

regards, tom lane