Re: ON CONFLICT issues around whole row vars, - Mailing list pgsql-hackers

On Tue, Sep 29, 2015 at 8:24 AM, Andres Freund <andres@anarazel.de> wrote:
> So, took a bit longer than "tomorrow. I fought for a long while with a
> mysterious issue, which turned out to be separate bug: The excluded
> relation was affected by row level security policies, which doesn't make
> sense.

Why? You certainly thought that it made sense for conventional column
permissions due to potential problems with before row insert triggers.
Why would RLS be different? Some of my concerns with RLS were that it
is different to the existing permissions model in a way that seems a
bit arbitrary. I don't think that they were changed to do anything
special with SELECT ... FOR UPDATE, which has always required some
amount of conventional UPDATE privilege.

I specifically remember discussing this with you off list (on IM,
roughly a couple of weeks prior to initial commit). I recommended that
we err towards a more restrictive behavior in the absence of any
strong principle pushing us one way or the other. You seemed to agree.

> My proposal in this WIP patch is to make it a bit clearer that
> 'EXCLUDED' isn't a real relation. I played around with adding a
> different rtekind, but that's too heavy a hammer. What I instead did was
> to set relkind to composite - which seems to signal pretty well that
> we're not dealing with a real relation. That immediately fixes the RLS
> issue as fireRIRrules has the following check:
>                 if (rte->rtekind != RTE_RELATION ||
>                         rte->relkind != RELKIND_RELATION)
>                         continue;

Well, not sure that that's a good thing. Let's discuss.

> It also makes it relatively straightforward to fix the system column
> issue by adding an additional relkind check to scanRTEForColumn's system
> column handling.

That seems fine.

> WRT to the wholerow issue: There's currently two reasons we need a
> targetlist entry for excluded wholerow vars: 1) setrefs.c errors out
> without - that can relativley easily be worked around 2) ruleutils.c
> expects an entry in the child tlist. That could also be worked around,
> but it's a bit more verbose.  I'm inclined to not go the pullup route
> but instead simply unconditionally add a wholerow var to the excluded
> tlist.

I suppose that we have a tight enough grip on the targetlist that it's
hard to imagine anything else being introduced there spuriously. I had
thought that the pull-up did allow useful additional
defense/sanitization, but that may not be an excellent argument. The
only remaining argument is that my approach is closer to RETURNING,
but that doesn't seem like an excellent argument.

Basically, I think that this is fine.

However, there were a number of small stylistic tweaks made in passing
within my original patch -- minor things around consistency. Please
either restore these, or commit them separately.

-- 
Peter Geoghegan