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