Обсуждение: Bogus rte->relkind for EXCLUDED pseudo-relation
In the wake of b23cd185f (pushed just now), I tried adding Asserts to rewriteHandler.c that relkinds in RTEs don't change, as attached. This blew up the regression tests immediately. On investigation, I find that (1) ON CONFLICT's EXCLUDED pseudo-relation is assigned rte->relkind = RELKIND_COMPOSITE, a rather horrid hack installed by commit ad2278379. (2) If a stored rule involves ON CONFLICT, then while loading the rule AcquireRewriteLocks overwrites that with the actual relkind, ie RELKIND_RELATION. Or it did without the attached Assert, anyway. It appears to me that this means whatever safeguards are created by the use of RELKIND_COMPOSITE will fail to apply in a rule. Maybe that's okay because the relevant behaviors only occur at parse time not rewrite/planning/execution, but even to write that is to doubt how reliable and future-proof the assumption is. I'm inclined to think we'd be well advised to undo that aspect of ad2278379 and solve it some other way. Maybe a new RTEKind would be a better idea. Alternatively, we could drop rewriteHandler.c's attempts to update relkind. Theoretically that's safe now, but I hadn't quite wanted to just pull that trigger right away ... regards, tom lane diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index fb0c687bd8..49e0f54355 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -193,6 +193,7 @@ AcquireRewriteLocks(Query *parsetree, * While we have the relation open, update the RTE's relkind, * just in case it changed since this rule was made. */ + Assert(rte->relkind == rel->rd_rel->relkind); rte->relkind = rel->rd_rel->relkind; table_close(rel, NoLock); @@ -3223,6 +3224,7 @@ rewriteTargetView(Query *parsetree, Relation view) * While we have the relation open, update the RTE's relkind, just in case * it changed since this view was made (cf. AcquireRewriteLocks). */ + Assert(base_rte->relkind == base_rel->rd_rel->relkind); base_rte->relkind = base_rel->rd_rel->relkind; /*
Hi, On 2022-12-02 12:34:36 -0500, Tom Lane wrote: > In the wake of b23cd185f (pushed just now), I tried adding Asserts > to rewriteHandler.c that relkinds in RTEs don't change, as attached. > This blew up the regression tests immediately. On investigation, > I find that > > (1) ON CONFLICT's EXCLUDED pseudo-relation is assigned > rte->relkind = RELKIND_COMPOSITE, a rather horrid hack > installed by commit ad2278379. Is it that horrid? I guess we can add a full blown relkind for it, but that'd not really change that we'd logic to force AcquireRewriteLocks() to keep it's hand off the relkind? We don't really have a different way to represent something that looks like a table's tuple, but without system columns, and that shouldn't affected by RLS rewrite magic. We could add a distinct RELKIND of course, but that'd afaict look very similar to RELKIND_COMPOSITE_TYPE. > I'm inclined to think we'd be well advised to undo that aspect of > ad2278379 and solve it some other way. Maybe a new RTEKind would > be a better idea. Alternatively, we could drop rewriteHandler.c's > attempts to update relkind. Theoretically that's safe now, but > I hadn't quite wanted to just pull that trigger right away ... I think it'd be good to not have rewriteHandler.c update relkind, even if we undo the RELKIND_COMPOSITE aspect of ad2278379. Changing relkind seems fairly dangerous to me, particularly because we don't ever expect that to happen now. I think it might make sense to make it an elog() rather than an Assert() though. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > On 2022-12-02 12:34:36 -0500, Tom Lane wrote: >> I find that >> (1) ON CONFLICT's EXCLUDED pseudo-relation is assigned >> rte->relkind = RELKIND_COMPOSITE, a rather horrid hack >> installed by commit ad2278379. > Is it that horrid? It's pretty bad IMO. You didn't even bother to update the comments for RangeTblEntry to explain that char relkind; /* relation kind (see pg_class.relkind) */ might now not be the rel's relkind at all. Changing RTEKind would likely be a better way, though of course we couldn't do that in back branches. And I think that we do have an issue in the back branches. According to the commit message for ad2278379, 4) References to EXCLUDED were rewritten by the RLS machinery, as EXCLUDED was treated as if it were the underlying relation. That rewriting would be post-rule-load, so it sure seems to me that a rule containing EXCLUDED would be improperly subject to RLS rewriting. I don't have enough familiarity with RLS to come up with a test case, and I don't see any relevant examples in the mailing list threads referenced by ad2278379, but I bet that it is broken. The back-branch fix could just be to teach rewriteHandler.c to not overwrite RELKIND_COMPOSITE_TYPE, perhaps. We can't remove the update completely because of the table-to-view case. regards, tom lane