Re: on placeholder entries in view rule action query's range table
On Fri, 9 Dec 2022 at 12:20, Amit Langote wrote: > > On Fri, Dec 9, 2022 at 3:07 PM Amit Langote wrote: > > On Thu, Dec 8, 2022 at 6:12 PM Alvaro Herrera > > 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
Re: on placeholder entries in view rule action query's range table
Amit Langote 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
Re: on placeholder entries in view rule action query's range table
Amit Langote writes: > On Mon, Jan 9, 2023 at 5:58 AM Tom Lane 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
Re: on placeholder entries in view rule action query's range table
On Thu, Jan 12, 2023 at 10:06 AM Tom Lane wrote: > Amit Langote writes: > > On Mon, Jan 9, 2023 at 5:58 AM Tom Lane 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
Re: on placeholder entries in view rule action query's range table
Amit Langote writes: > On Thu, Jan 12, 2023 at 10:06 AM Tom Lane 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
Re: on placeholder entries in view rule action query's range table
On Thu, Jan 12, 2023 at 12:45 PM Tom Lane wrote: > Amit Langote writes: > > On Thu, Jan 12, 2023 at 10:06 AM Tom Lane 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
Re: on placeholder entries in view rule action query's range table
On Wed, Jan 11, 2023 at 10:45:33PM -0500, Tom Lane wrote: > Amit Langote writes: > > On Thu, Jan 12, 2023 at 10:06 AM Tom Lane 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.gl6...@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
Re: on placeholder entries in view rule action query's range table
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 writes: >>> On Thu, Jan 12, 2023 at 10:06 AM Tom Lane 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.gl6...@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
Re: on placeholder entries in view rule action query's range table
Andrew Dunstan 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
Re: on placeholder entries in view rule action query's range table
On Thu, Jan 12, 2023 at 09:54:09AM -0500, Tom Lane wrote: > Andrew Dunstan 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
Re: on placeholder entries in view rule action query's range table
Justin Pryzby 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
Re: on placeholder entries in view rule action query's range table
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
Re: on placeholder entries in view rule action query's range table
Andrew Dunstan 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
Re: on placeholder entries in view rule action query's range table
On Wed, Dec 7, 2022 at 6:42 PM Amit Langote 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
Re: on placeholder entries in view rule action query's range table
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/
Re: on placeholder entries in view rule action query's range table
On Thu, Jan 12, 2023 at 10:06 AM Tom Lane wrote: > Amit Langote writes: > > On Mon, Jan 9, 2023 at 5:58 AM Tom Lane 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/ ScanQueryForLocks-lock-views.patch Description: Binary data
Re: on placeholder entries in view rule action query's range table
Amit Langote 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
Re: on placeholder entries in view rule action query's range table
I wrote: > Amit Langote 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
Re: on placeholder entries in view rule action query's range table
On Thu, Apr 6, 2023 at 3:33 Tom Lane wrote: > Amit Langote 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; Oops, I was working in the wrong local branch. Thanks for pushing. I agree that there’s no live bug as such right now, but still good to be consistent. > -- Thanks, Amit Langote EDB: http://www.enterprisedb.com