Re: on placeholder entries in view rule action query's range table - Mailing list pgsql-hackers
From | Tom Lane |
---|---|
Subject | Re: on placeholder entries in view rule action query's range table |
Date | |
Msg-id | 782120.1673485582@sss.pgh.pa.us Whole thread Raw |
In response to | Re: on placeholder entries in view rule action query's range table (Amit Langote <amitlangote09@gmail.com>) |
Responses |
Re: on placeholder entries in view rule action query's range table
Re: on placeholder entries in view rule action query's range table |
List | pgsql-hackers |
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
pgsql-hackers by date: