Thread: 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