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

Per Alvaro's advice, forking this from [1].

In light of my proposed changes to decouple permission checking from
the range table on that thread (now committed as a61b1f7482), I had
also been posting a patch there to rethink commands/view.c's
editorializing of a view rule action query' range table to add the
placeholder RTEs for checking the permissions of the view relation
among other things.

That patch came to life after Tom's comment in the same thread, where
he wondered if we could do away with those placeholder entries [2] if
permission checking details were to go elsewhere.

All but very recent versions of the patch were simply removing the
function UpdateRangeTableOfViewParse() that added those entries, such
that a view rule's action query would be stored with only the RTEs of
the relations mentioned in the view's query, with no trace whatsoever
of the view relation.  ApplyRetrieveRule() working with a given user
query on the view would add a placeholder entry for the view for the
purpose served by those no-longer-present placeholder RTEs in the rule
action query's range table.  It would accomplish that by adding a copy
of the query's view RTE with appropriate permission details filled in
before converting the latter into a RTE_SUBQUERY entry.  However, this
approach of not storing the placeholder in the stored rule would lead
to a whole lot of regression test output changes, because the stored
view queries of many regression tests involving views would now end up
with only 1 entry in the range table instead of 3, causing ruleutils.c
to no longer qualify the column names in the deparsed representation
of those queries appearing in those regression test expected outputs.

To avoid that churn (not sure if really a goal to strive for in this
case!), I thought it might be better to keep the OLD entry in the
stored action query while getting rid of the NEW entry.  Other than
avoiding the regression test output churn, this also makes the changes
of ApplyRetrieveRule unnecessary.  Actually, as I was addressing
Alvaro's comments on the now-committed patch, I was starting to get
concerned about the implications of the change in position of the view
relation RTE in the query's range table if ApplyRetrieveRule() adds
one from scratch instead of simply recycling the OLD entry from stored
rule action query, even though I could see that there are no
*user-visible* changes, especially after decoupling permission
checking from the range table.

Anyway, the attached patch implements this 2nd approach.

I'll add this to the January CF.

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

[1] https://www.postgresql.org/message-id/CA+HiwqGjJDmUhDSfv-U2qhKJjt9ST7Xh9JXC_irsAQ1TAUsJYg@mail.gmail.com
[2] https://www.postgresql.org/message-id/697679.1625154303%40sss.pgh.pa.us

On Wed, Dec 7, 2022 at 6:42 PM Amit Langote <amitlangote09@gmail.com> wrote:
> Per Alvaro's advice, forking this from [1].

Forgot to add Alvaro.

> In light of my proposed changes to decouple permission checking from
> the range table on that thread (now committed as a61b1f7482), I had
> also been posting a patch there to rethink commands/view.c's
> editorializing of a view rule action query' range table to add the
> placeholder RTEs for checking the permissions of the view relation
> among other things.
>
> That patch came to life after Tom's comment in the same thread, where
> he wondered if we could do away with those placeholder entries [2] if
> permission checking details were to go elsewhere.
>
> All but very recent versions of the patch were simply removing the
> function UpdateRangeTableOfViewParse() that added those entries, such
> that a view rule's action query would be stored with only the RTEs of
> the relations mentioned in the view's query, with no trace whatsoever
> of the view relation.  ApplyRetrieveRule() working with a given user
> query on the view would add a placeholder entry for the view for the
> purpose served by those no-longer-present placeholder RTEs in the rule
> action query's range table.  It would accomplish that by adding a copy
> of the query's view RTE with appropriate permission details filled in
> before converting the latter into a RTE_SUBQUERY entry.  However, this
> approach of not storing the placeholder in the stored rule would lead
> to a whole lot of regression test output changes, because the stored
> view queries of many regression tests involving views would now end up
> with only 1 entry in the range table instead of 3, causing ruleutils.c
> to no longer qualify the column names in the deparsed representation
> of those queries appearing in those regression test expected outputs.
>
> To avoid that churn (not sure if really a goal to strive for in this
> case!), I thought it might be better to keep the OLD entry in the
> stored action query while getting rid of the NEW entry.  Other than
> avoiding the regression test output churn, this also makes the changes
> of ApplyRetrieveRule unnecessary.  Actually, as I was addressing
> Alvaro's comments on the now-committed patch, I was starting to get
> concerned about the implications of the change in position of the view
> relation RTE in the query's range table if ApplyRetrieveRule() adds
> one from scratch instead of simply recycling the OLD entry from stored
> rule action query, even though I could see that there are no
> *user-visible* changes, especially after decoupling permission
> checking from the range table.
>
> Anyway, the attached patch implements this 2nd approach.
>
> I'll add this to the January CF.

Done.

https://commitfest.postgresql.org/41/4048/

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

On 2022-Dec-07, Amit Langote wrote:

> However, this
> approach of not storing the placeholder in the stored rule would lead
> to a whole lot of regression test output changes, because the stored
> view queries of many regression tests involving views would now end up
> with only 1 entry in the range table instead of 3, causing ruleutils.c
> to no longer qualify the column names in the deparsed representation
> of those queries appearing in those regression test expected outputs.
> 
> To avoid that churn (not sure if really a goal to strive for in this
> case!), I thought it might be better to keep the OLD entry in the
> stored action query while getting rid of the NEW entry.

If the *only* argument for keeping the RTE for OLD is to avoid
regression test churn, then definitely it is not worth doing and it
should be ripped out.

> Other than avoiding the regression test output churn, this also makes
> the changes of ApplyRetrieveRule unnecessary.

But do these changes mean the code is worse afterwards?  Changing stuff,
per se, is not bad.  Also, since you haven't posted the "complete" patch
since Nov 7th, it's not easy to tell what those changes are.

Maybe you should post both versions of the patch -- one that removes
just NEW, and one that removes both OLD and NEW, so that we can judge.

> Actually, as I was addressing Alvaro's comments on the now-committed
> patch, I was starting to get concerned about the implications of the
> change in position of the view relation RTE in the query's range table
> if ApplyRetrieveRule() adds one from scratch instead of simply
> recycling the OLD entry from stored rule action query, even though I
> could see that there are no *user-visible* changes, especially after
> decoupling permission checking from the range table.

Hmm, I think I see the point, though I don't necessarily agree that
there is a problem.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Thu, Dec 8, 2022 at 6:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Dec-07, Amit Langote wrote:
> > However, this
> > approach of not storing the placeholder in the stored rule would lead
> > to a whole lot of regression test output changes, because the stored
> > view queries of many regression tests involving views would now end up
> > with only 1 entry in the range table instead of 3, causing ruleutils.c
> > to no longer qualify the column names in the deparsed representation
> > of those queries appearing in those regression test expected outputs.
> >
> > To avoid that churn (not sure if really a goal to strive for in this
> > case!), I thought it might be better to keep the OLD entry in the
> > stored action query while getting rid of the NEW entry.
>
> If the *only* argument for keeping the RTE for OLD is to avoid
> regression test churn, then definitely it is not worth doing and it
> should be ripped out.
>
> > Other than avoiding the regression test output churn, this also makes
> > the changes of ApplyRetrieveRule unnecessary.
>
> But do these changes mean the code is worse afterwards?  Changing stuff,
> per se, is not bad.  Also, since you haven't posted the "complete" patch
> since Nov 7th, it's not easy to tell what those changes are.
>
> Maybe you should post both versions of the patch -- one that removes
> just NEW, and one that removes both OLD and NEW, so that we can judge.

OK, I gave the previous approach another try to see if I can change
ApplyRetrieveRule() in a bit more convincing way this time around, now
that the RTEPermissionInfo patch is in.

I would say I'm more satisfied with how it turned out this time.  Let
me know what you think.

> > Actually, as I was addressing Alvaro's comments on the now-committed
> > patch, I was starting to get concerned about the implications of the
> > change in position of the view relation RTE in the query's range table
> > if ApplyRetrieveRule() adds one from scratch instead of simply
> > recycling the OLD entry from stored rule action query, even though I
> > could see that there are no *user-visible* changes, especially after
> > decoupling permission checking from the range table.
>
> Hmm, I think I see the point, though I don't necessarily agree that
> there is a problem.

Yeah, I'm not worried as much with the new version.  That is helped by
the fact that I've made ApplyRetrieveRule() now do basically what
UpdateRangeTableOfViewParse() would do with the stored rule query.
Also, our making add_rtes_to_flat_rtable() add perminfos in the RTE
order helped find the bug with the last version.

Attaching both patches.

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

On Fri, Dec 9, 2022 at 3:07 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Dec 8, 2022 at 6:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > On 2022-Dec-07, Amit Langote wrote:
> > > However, this
> > > approach of not storing the placeholder in the stored rule would lead
> > > to a whole lot of regression test output changes, because the stored
> > > view queries of many regression tests involving views would now end up
> > > with only 1 entry in the range table instead of 3, causing ruleutils.c
> > > to no longer qualify the column names in the deparsed representation
> > > of those queries appearing in those regression test expected outputs.
> > >
> > > To avoid that churn (not sure if really a goal to strive for in this
> > > case!), I thought it might be better to keep the OLD entry in the
> > > stored action query while getting rid of the NEW entry.
> >
> > If the *only* argument for keeping the RTE for OLD is to avoid
> > regression test churn, then definitely it is not worth doing and it
> > should be ripped out.
> >
> > > Other than avoiding the regression test output churn, this also makes
> > > the changes of ApplyRetrieveRule unnecessary.
> >
> > But do these changes mean the code is worse afterwards?  Changing stuff,
> > per se, is not bad.  Also, since you haven't posted the "complete" patch
> > since Nov 7th, it's not easy to tell what those changes are.
> >
> > Maybe you should post both versions of the patch -- one that removes
> > just NEW, and one that removes both OLD and NEW, so that we can judge.
>
> OK, I gave the previous approach another try to see if I can change
> ApplyRetrieveRule() in a bit more convincing way this time around, now
> that the RTEPermissionInfo patch is in.
>
> I would say I'm more satisfied with how it turned out this time.  Let
> me know what you think.
>
> > > Actually, as I was addressing Alvaro's comments on the now-committed
> > > patch, I was starting to get concerned about the implications of the
> > > change in position of the view relation RTE in the query's range table
> > > if ApplyRetrieveRule() adds one from scratch instead of simply
> > > recycling the OLD entry from stored rule action query, even though I
> > > could see that there are no *user-visible* changes, especially after
> > > decoupling permission checking from the range table.
> >
> > Hmm, I think I see the point, though I don't necessarily agree that
> > there is a problem.
>
> Yeah, I'm not worried as much with the new version.  That is helped by
> the fact that I've made ApplyRetrieveRule() now do basically what
> UpdateRangeTableOfViewParse() would do with the stored rule query.
> Also, our making add_rtes_to_flat_rtable() add perminfos in the RTE
> order helped find the bug with the last version.
>
> Attaching both patches.

Looks like I forgot to update some expected output files.

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

On Fri, 9 Dec 2022 at 12:20, Amit Langote <amitlangote09@gmail.com> wrote:
>
> On Fri, Dec 9, 2022 at 3:07 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Thu, Dec 8, 2022 at 6:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > > On 2022-Dec-07, Amit Langote wrote:
> > > > However, this
> > > > approach of not storing the placeholder in the stored rule would lead
> > > > to a whole lot of regression test output changes, because the stored
> > > > view queries of many regression tests involving views would now end up
> > > > with only 1 entry in the range table instead of 3, causing ruleutils.c
> > > > to no longer qualify the column names in the deparsed representation
> > > > of those queries appearing in those regression test expected outputs.
> > > >
> > > > To avoid that churn (not sure if really a goal to strive for in this
> > > > case!), I thought it might be better to keep the OLD entry in the
> > > > stored action query while getting rid of the NEW entry.
> > >
> > > If the *only* argument for keeping the RTE for OLD is to avoid
> > > regression test churn, then definitely it is not worth doing and it
> > > should be ripped out.
> > >
> > > > Other than avoiding the regression test output churn, this also makes
> > > > the changes of ApplyRetrieveRule unnecessary.
> > >
> > > But do these changes mean the code is worse afterwards?  Changing stuff,
> > > per se, is not bad.  Also, since you haven't posted the "complete" patch
> > > since Nov 7th, it's not easy to tell what those changes are.
> > >
> > > Maybe you should post both versions of the patch -- one that removes
> > > just NEW, and one that removes both OLD and NEW, so that we can judge.
> >
> > OK, I gave the previous approach another try to see if I can change
> > ApplyRetrieveRule() in a bit more convincing way this time around, now
> > that the RTEPermissionInfo patch is in.
> >
> > I would say I'm more satisfied with how it turned out this time.  Let
> > me know what you think.
> >
> > > > Actually, as I was addressing Alvaro's comments on the now-committed
> > > > patch, I was starting to get concerned about the implications of the
> > > > change in position of the view relation RTE in the query's range table
> > > > if ApplyRetrieveRule() adds one from scratch instead of simply
> > > > recycling the OLD entry from stored rule action query, even though I
> > > > could see that there are no *user-visible* changes, especially after
> > > > decoupling permission checking from the range table.
> > >
> > > Hmm, I think I see the point, though I don't necessarily agree that
> > > there is a problem.
> >
> > Yeah, I'm not worried as much with the new version.  That is helped by
> > the fact that I've made ApplyRetrieveRule() now do basically what
> > UpdateRangeTableOfViewParse() would do with the stored rule query.
> > Also, our making add_rtes_to_flat_rtable() add perminfos in the RTE
> > order helped find the bug with the last version.
> >
> > Attaching both patches.
>
> Looks like I forgot to update some expected output files.

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
54afdcd6182af709cb0ab775c11b90decff166eb ===
=== applying patch
./v1-0001-Do-not-add-the-NEW-entry-to-view-rule-action-s-ra.patch
Hunk #1 succeeded at 1908 (offset 1 line).
=== applying patch ./v2-0001-Remove-UpdateRangeTableOfViewParse.patch
patching file contrib/postgres_fdw/expected/postgres_fdw.out
Hunk #1 FAILED at 2606.
Hunk #2 FAILED at 2669.
2 out of 4 hunks FAILED -- saving rejects to file
contrib/postgres_fdw/expected/postgres_fdw.out.rej

[1] - http://cfbot.cputube.org/patch_41_4048.log

Regards,
Vignesh

On Wed, Jan 4, 2023 at 7:17 PM vignesh C <vignesh21@gmail.com> wrote:
> On Fri, 9 Dec 2022 at 12:20, Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Dec 9, 2022 at 3:07 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Thu, Dec 8, 2022 at 6:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > > > On 2022-Dec-07, Amit Langote wrote:
> > > > > However, this
> > > > > approach of not storing the placeholder in the stored rule would lead
> > > > > to a whole lot of regression test output changes, because the stored
> > > > > view queries of many regression tests involving views would now end up
> > > > > with only 1 entry in the range table instead of 3, causing ruleutils.c
> > > > > to no longer qualify the column names in the deparsed representation
> > > > > of those queries appearing in those regression test expected outputs.
> > > > >
> > > > > To avoid that churn (not sure if really a goal to strive for in this
> > > > > case!), I thought it might be better to keep the OLD entry in the
> > > > > stored action query while getting rid of the NEW entry.
> > > >
> > > > If the *only* argument for keeping the RTE for OLD is to avoid
> > > > regression test churn, then definitely it is not worth doing and it
> > > > should be ripped out.
> > > >
> > > > > Other than avoiding the regression test output churn, this also makes
> > > > > the changes of ApplyRetrieveRule unnecessary.
> > > >
> > > > But do these changes mean the code is worse afterwards?  Changing stuff,
> > > > per se, is not bad.  Also, since you haven't posted the "complete" patch
> > > > since Nov 7th, it's not easy to tell what those changes are.
> > > >
> > > > Maybe you should post both versions of the patch -- one that removes
> > > > just NEW, and one that removes both OLD and NEW, so that we can judge.
> > >
> > > OK, I gave the previous approach another try to see if I can change
> > > ApplyRetrieveRule() in a bit more convincing way this time around, now
> > > that the RTEPermissionInfo patch is in.
> > >
> > > I would say I'm more satisfied with how it turned out this time.  Let
> > > me know what you think.
> > >
> > > > > Actually, as I was addressing Alvaro's comments on the now-committed
> > > > > patch, I was starting to get concerned about the implications of the
> > > > > change in position of the view relation RTE in the query's range table
> > > > > if ApplyRetrieveRule() adds one from scratch instead of simply
> > > > > recycling the OLD entry from stored rule action query, even though I
> > > > > could see that there are no *user-visible* changes, especially after
> > > > > decoupling permission checking from the range table.
> > > >
> > > > Hmm, I think I see the point, though I don't necessarily agree that
> > > > there is a problem.
> > >
> > > Yeah, I'm not worried as much with the new version.  That is helped by
> > > the fact that I've made ApplyRetrieveRule() now do basically what
> > > UpdateRangeTableOfViewParse() would do with the stored rule query.
> > > Also, our making add_rtes_to_flat_rtable() add perminfos in the RTE
> > > order helped find the bug with the last version.
> > >
> > > Attaching both patches.
> >
> > Looks like I forgot to update some expected output files.
>
> The patch does not apply on top of HEAD as in [1], please post a rebased patch:
> === Applying patches on top of PostgreSQL commit ID
> 54afdcd6182af709cb0ab775c11b90decff166eb ===
> === applying patch
> ./v1-0001-Do-not-add-the-NEW-entry-to-view-rule-action-s-ra.patch
> Hunk #1 succeeded at 1908 (offset 1 line).
> === applying patch ./v2-0001-Remove-UpdateRangeTableOfViewParse.patch
> patching file contrib/postgres_fdw/expected/postgres_fdw.out
> Hunk #1 FAILED at 2606.
> Hunk #2 FAILED at 2669.
> 2 out of 4 hunks FAILED -- saving rejects to file
> contrib/postgres_fdw/expected/postgres_fdw.out.rej

Thanks for the heads up.  cfbot fails because it's applying both the
patches which, being alternative approaches to address $subject, are
mutually conflicting.

I've attached just the patch that we should move forward with, as
Alvaro might agree.

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

Amit Langote <amitlangote09@gmail.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.)

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.

Anyway, if you don't feel like tackling that then I'd go back to the
patch version that kept the OLD RTE.  (Maybe we could rename that to
something else, though, such as "*VIEW*"?)

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.

            regards, tom lane

On Mon, Jan 9, 2023 at 5:58 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.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

Amit Langote <amitlangote09@gmail.com> writes:
> On Mon, Jan 9, 2023 at 5:58 AM Tom Lane <tgl@sss.pgh.pa.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

 On Thu, Jan 12, 2023 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Mon, Jan 9, 2023 at 5:58 AM Tom Lane <tgl@sss.pgh.pa.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

Amit Langote <amitlangote09@gmail.com> writes:
>  On Thu, Jan 12, 2023 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

It looks like we're not out of the woods on this: the buildfarm
members that run cross-version-upgrade tests are all unhappy.
Most of them are not reporting any useful details, but I suspect
that they are barfing because dumps from the old server include
table-qualified variable names in some CREATE VIEW commands while
dumps from HEAD omit the qualifications.  I don't see any
mechanism in TestUpgradeXversion.pm that could deal with that
conveniently, and in any case we'd have to roll out a client
script update to the affected animals.  I fear we may have to
revert this pending development of better TestUpgradeXversion.pm
support.

            regards, tom lane

On Thu, Jan 12, 2023 at 12:45 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> >  On Thu, Jan 12, 2023 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> 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.
>
> It looks like we're not out of the woods on this: the buildfarm
> members that run cross-version-upgrade tests are all unhappy.
> Most of them are not reporting any useful details, but I suspect
> that they are barfing because dumps from the old server include
> table-qualified variable names in some CREATE VIEW commands while
> dumps from HEAD omit the qualifications.  I don't see any
> mechanism in TestUpgradeXversion.pm that could deal with that
> conveniently, and in any case we'd have to roll out a client
> script update to the affected animals.  I fear we may have to
> revert this pending development of better TestUpgradeXversion.pm
> support.

Ah, OK, no problem.

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

On Wed, Jan 11, 2023 at 10:45:33PM -0500, Tom Lane wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> >  On Thu, Jan 12, 2023 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> 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.
> 
> It looks like we're not out of the woods on this: the buildfarm
> members that run cross-version-upgrade tests are all unhappy.
> Most of them are not reporting any useful details, but I suspect
> that they are barfing because dumps from the old server include
> table-qualified variable names in some CREATE VIEW commands while
> dumps from HEAD omit the qualifications.  I don't see any
> mechanism in TestUpgradeXversion.pm that could deal with that
> conveniently, and in any case we'd have to roll out a client
> script update to the affected animals.  I fear we may have to
> revert this pending development of better TestUpgradeXversion.pm
> support.

There's a diffs available for several of them:

- SELECT citext_table.id,
-    citext_table.name
+ SELECT id,
+    name

It looks like TestUpgradeXversion.pm is using the diff command I sent to
get tigher bounds on allowable changes.

20210415153722.GL6091@telsasoft.com

It's ugly and a terrible hack, and I don't know whether anyone would say
it's good enough, but one could can probably avoid the diff like:

sed -r '/CREATE/,/^$/{ s/\w+\.//g }'

You'd still have to wait for it to be deployed, though.

-- 
Justin

On 2023-01-12 Th 00:12, Justin Pryzby wrote:
> On Wed, Jan 11, 2023 at 10:45:33PM -0500, Tom Lane wrote:
>> Amit Langote <amitlangote09@gmail.com> writes:
>>>  On Thu, Jan 12, 2023 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>> 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.
>> It looks like we're not out of the woods on this: the buildfarm
>> members that run cross-version-upgrade tests are all unhappy.
>> Most of them are not reporting any useful details, but I suspect
>> that they are barfing because dumps from the old server include
>> table-qualified variable names in some CREATE VIEW commands while
>> dumps from HEAD omit the qualifications.  I don't see any
>> mechanism in TestUpgradeXversion.pm that could deal with that
>> conveniently, and in any case we'd have to roll out a client
>> script update to the affected animals.  I fear we may have to
>> revert this pending development of better TestUpgradeXversion.pm
>> support.
> There's a diffs available for several of them:
>
> - SELECT citext_table.id,
> -    citext_table.name
> + SELECT id,
> +    name
>
> It looks like TestUpgradeXversion.pm is using the diff command I sent to
> get tigher bounds on allowable changes.
>
> 20210415153722.GL6091@telsasoft.com
>
> It's ugly and a terrible hack, and I don't know whether anyone would say
> it's good enough, but one could can probably avoid the diff like:
>
> sed -r '/CREATE/,/^$/{ s/\w+\.//g }'
>
> You'd still have to wait for it to be deployed, though.


That looks quite awful. I don't think you could persuade me to deploy it
(We don't use sed anyway). It might be marginally better if the pattern
were /CREATE.*VIEW/ and we ignored that first line, but it still seems
awful to me.

Another approach might be simply to increase the latitude allowed for
old versions <= 15 with new versions >= 16. Currently we allow 90 for
cases where the versions differ, but we could increase it to, say, 200
in such cases (we'd need to experiment a bit to find the right limit).


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-12 Th 00:12, Justin Pryzby wrote:
>> It's ugly and a terrible hack, and I don't know whether anyone would say
>> it's good enough, but one could can probably avoid the diff like:
>> sed -r '/CREATE/,/^$/{ s/\w+\.//g }'

> That looks quite awful. I don't think you could persuade me to deploy it
> (We don't use sed anyway). It might be marginally better if the pattern
> were /CREATE.*VIEW/ and we ignored that first line, but it still seems
> awful to me.

Yeah, does not sound workable: it would risk ignoring actual problems.

I was wondering whether we could store a per-version patch or Perl
script that edits the old dump file to remove known discrepancies
from HEAD.  If well-maintained, that could eliminate the need for the
arbitrary "fuzz factors" that are in TestUpgradeXversion.pm right now.
I'd really want these files to be kept in the community source tree,
though, so that we do not need a new BF client release to change them.

This isn't the first time this has come up, but now we have a case
where it's actually blocking development, so maybe it's time to
make something happen.  If you want I can work on a patch for the
BF client.

            regards, tom lane

On Thu, Jan 12, 2023 at 09:54:09AM -0500, Tom Lane wrote:
> Andrew Dunstan <andrew@dunslane.net> writes:
> > On 2023-01-12 Th 00:12, Justin Pryzby wrote:
> >> It's ugly and a terrible hack, and I don't know whether anyone would say
> >> it's good enough, but one could can probably avoid the diff like:
> >> sed -r '/CREATE/,/^$/{ s/\w+\.//g }'
> 
> > That looks quite awful. I don't think you could persuade me to deploy it
> > (We don't use sed anyway). It might be marginally better if the pattern
> > were /CREATE.*VIEW/ and we ignored that first line, but it still seems
> > awful to me.
> 
> Yeah, does not sound workable: it would risk ignoring actual problems.
> 
> I was wondering whether we could store a per-version patch or Perl
> script that edits the old dump file to remove known discrepancies
> from HEAD.  If well-maintained, that could eliminate the need for the
> arbitrary "fuzz factors" that are in TestUpgradeXversion.pm right now.
> I'd really want these files to be kept in the community source tree,
> though, so that we do not need a new BF client release to change them.
> 
> This isn't the first time this has come up, but now we have a case
> where it's actually blocking development, so maybe it's time to
> make something happen.  If you want I can work on a patch for the
> BF client.

What about also including a dump from an old version, too ?
Then the upgrade test can test actual upgrades.

A new dump file would need to be updated at every release; the old ones
could stick around, maybe forever.

-- 
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:
> What about also including a dump from an old version, too ?
> Then the upgrade test can test actual upgrades.

The BF clients already do that (if enabled), but they work from
up-to-date installations of the respective branch tips.  I'd not
want to have some branches including hypothetical output of
other branches, because it'd be too easy for those files to get
out of sync and deliver misleading answers.

            regards, tom lane

On 2023-01-12 Th 09:54, Tom Lane wrote:
>
> I was wondering whether we could store a per-version patch or Perl
> script that edits the old dump file to remove known discrepancies
> from HEAD.  If well-maintained, that could eliminate the need for the
> arbitrary "fuzz factors" that are in TestUpgradeXversion.pm right now.
> I'd really want these files to be kept in the community source tree,
> though, so that we do not need a new BF client release to change them.
>
> This isn't the first time this has come up, but now we have a case
> where it's actually blocking development, so maybe it's time to
> make something happen.  If you want I can work on a patch for the
> BF client.
>
>             


I wouldn't worry too much about the client for now. What we'd need is a)
a place in the source code where we know to find the module b) a module
name c) one or more functions to call to make the adjustment(s).

so, say in src/test/perl we have PostgreSQL/AdjustUpgrade.pm with a
subroutine adjust_dumpfile($oldversion, $dumpfile).

That would be fairly easy to look for and call, and a good place to
start. More ambitiously we might also provide a function do do most of
the pre_upgrade adjustments made in TestUpgradeXversion.pm at lines
405-604. But let's walk before we try to run. This is probably a good
time to be doing this as I want to push out a new release pretty soon to
deal with the up-to-date check issues.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Andrew Dunstan <andrew@dunslane.net> writes:
> On 2023-01-12 Th 09:54, Tom Lane wrote:
>> I was wondering whether we could store a per-version patch or Perl
>> script that edits the old dump file to remove known discrepancies
>> from HEAD.

> so, say in src/test/perl we have PostgreSQL/AdjustUpgrade.pm with a
> subroutine adjust_dumpfile($oldversion, $dumpfile).

Seems reasonable.  I was imagining per-old-version .pm files, but
there's likely to be a fair amount of commonality between what to
do for different old versions, so probably that approach would be
too duplicative.

> That would be fairly easy to look for and call, and a good place to
> start. More ambitiously we might also provide a function do do most of
> the pre_upgrade adjustments made in TestUpgradeXversion.pm at lines
> 405-604. But let's walk before we try to run.

I think that part is also very very important to abstract out of the
BF client.  We've been burnt on that before too.  So, perhaps one
subroutine that can apply updates to the source DB just before
we dump it, and then a second that can edit the dump file after?

We could imagine a third custom subroutine that abstracts the
actual dump file comparison, but I'd prefer to get to a place
where we just expect exact match after the edit step.

I'll work on a straw-man patch.

            regards, tom lane

On Thu, Jan 12, 2023 at 10:06 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Mon, Jan 9, 2023 at 5:58 AM Tom Lane <tgl@sss.pgh.pa.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.

While thinking about query view locking in context of [1], I realized
that we have missed also fixing AcquirePlannerLocks() /
ScanQueryForLocks() to consider that an RTE_SUBQUERY rte may belong to
a view, which must be locked the same as RTE_RELATION entries.  Note
we did fix AcquireExecutorLocks() in 47bb9db75 as follows:

@@ -1769,7 +1769,8 @@ AcquireExecutorLocks(List *stmt_list, bool acquire)
        {
            RangeTblEntry *rte = (RangeTblEntry *) lfirst(lc2);

-           if (rte->rtekind != RTE_RELATION)
+           if (!(rte->rtekind == RTE_RELATION ||
+                 (rte->rtekind == RTE_SUBQUERY && OidIsValid(rte->relid))))

Attached a patch to fix.

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

[1] https://commitfest.postgresql.org/42/3478/

Amit Langote <amitlangote09@gmail.com> writes:
> While thinking about query view locking in context of [1], I realized
> that we have missed also fixing AcquirePlannerLocks() /
> ScanQueryForLocks() to consider that an RTE_SUBQUERY rte may belong to
> a view, which must be locked the same as RTE_RELATION entries.

I think you're right about that, because AcquirePlannerLocks is supposed
to reacquire whatever locks parsing+rewriting would have gotten.
However, what's with this hunk?

@@ -527,7 +527,7 @@ standard_planner(Query *parse, const char *query_string, int cursorOptions,
     result->partPruneInfos = glob->partPruneInfos;
     result->rtable = glob->finalrtable;
     result->permInfos = glob->finalrteperminfos;
-    result->viewRelations = glob->viewRelations;
+    result->viewRelations = NIL;
     result->resultRelations = glob->resultRelations;
     result->appendRelations = glob->appendRelations;
     result->subplans = glob->subplans;

            regards, tom lane

I wrote:
> Amit Langote <amitlangote09@gmail.com> writes:
>> While thinking about query view locking in context of [1], I realized
>> that we have missed also fixing AcquirePlannerLocks() /
>> ScanQueryForLocks() to consider that an RTE_SUBQUERY rte may belong to
>> a view, which must be locked the same as RTE_RELATION entries.

> I think you're right about that, because AcquirePlannerLocks is supposed
> to reacquire whatever locks parsing+rewriting would have gotten.

After poking at this a bit more, I'm not sure there is any observable bug,
because we still notice the view change in AcquireExecutorLocks and
loop back to re-plan after that.  It still seems like a good idea to
notice such changes sooner not later to reduce wasted work, so I went
ahead and pushed the patch.

The only way it'd be a live bug is if the planner actually fails because
it's working with a stale view definition.  I tried to make it fail by
adjusting the view to no longer use an underlying table and then
dropping that table ... but AcquirePlannerLocks still detected that,
because of course it recurses and locks the table reference it finds
in the view subquery.  Maybe you could make a failure case involving
dropping a user-defined function instead, but I thought that was getting
pretty far afield, so I didn't pursue it.

            regards, tom lane