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

On Mon, Jan 9, 2023 at 5:58 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Amit Langote <amitlangote09(at)gmail(dot)com> writes:
> > I've attached just the patch that we should move forward with, as
> > Alvaro might agree.
>
> I've looked at this briefly but don't like it very much, specifically
> the business about retroactively adding an RTE and RTEPermissionInfo
> into the view's replacement subquery. That seems expensive and bug-prone:
> if you're going to do that you might as well just leave the OLD entry
> in place, as the earlier patch did, because you're just reconstructing
> that same state of the parsetree a bit later on.
>
> Furthermore, if that's where we end up then I'm not really sure this is
> worth doing at all. The idea driving this was that if we could get rid
> of *both* OLD and NEW RTE entries then we'd not have O(N^2) behavior in
> deep subquery pull-ups due to the rangetable getting longer with each one.
> But getting it down from two extra entries to one extra entry isn't going
> to fix that big-O problem. (The patch as presented still has O(N^2)
> planning time for the nested-views example discussed earlier.)

Hmm, that's true.

> 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. ;-)

So, in the attached updated version, I removed the bits of
ApplyRetrieveRule() that would add the placeholder entry (OLD) and
also the existing lines that would reset relid, rellockmode, and
perminfoindex of the view RTE that's converted into a RTE_SUBQUERY
one. Then I fixed places to deal with subquery RTEs sometimes having
the relid, etc. set, just enough to pass make check-world. 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.

> 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. I think the original rationale
> for UpdateRangeTableOfViewParse was that we needed to keep the rtables
> of ON SELECT rules looking similar to those of other types of rules.
> Maybe we've cleaned up all the places that used to depend on that,
> but I'm not convinced.

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

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