Re: INSERT ... ON CONFLICT UPDATE and RLS

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Jan 7, 2015 at 4:04 AM, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com> wrote:
> > I think the policies applied should depend on the path taken, so if it
> > does an INSERT, then only the INSERT CHECK policy should be applied
> > (after the insert), but if it ends up doing an UPDATE, I would expect
> > the UPDATE USING policy to be applied (before the update) and the
> > UPDATE CHECK policy to be applied (after the update). I would not
> > expect the INSERT CHECK policy to be applied on the UPDATE path.
>
> I agree.

I can certainly understand the appeal of this approach, but I don't
think it makes sense. Consider what happens later on down the road,
after the code has been written and deployed using INSERT .. ON CONFLICT
UPDATE where 99% of the time only one path or the other is taken. Then
the other path is taken and suddenly the exact same command and row ends
up returning errors. Additional testing should have been done to check
if that happens, of course, but I really don't like the idea that the
exact same command, with the exact same policies, would succeed or fail,
due to policies, based on the data in the database.

That's not to say that, generally, speaking, the data in the database
shouldn't impact policy failures, but in this case, the policies might
not even reference any other tables in the database.

> > As to whether the UPDATE USING policy should cause an error to be
> > thrown if it is not satisfied, my inclination would be to not error,
> > and use the command tag to report that no rows were updated, since
> > that is what would happen with a regular UPDATE.
>
> My inclination would be to error, but I'd be OK with your proposal.

I'm pretty strongly in favor of erroring. :)

> > So overall INSERT .. ON CONFLICT UPDATE would be consistent with
> > either an INSERT or an UPDATE, depending on whether the row existed
> > beforehand, which is easier to explain than having some special UPSERT
> > semantics.
>
> Yeah. We won't escape the question so easily where triggers are
> concerned, but at least for RLS it seems like it should be possible to
> avoid confusing, one-off semantics.

Triggers are another question..

One approach that I don't recall seeing (and I'm not convinced that it's
a good idea either, but thought it deserved mention) is the idea of
having an UPSERT-specific set of policies (and perhaps an
UPSERT-specific trigger...?). That gets pretty grotty when you consider
existing systems which have INSERT and UPDATE triggers already, but
UPSERT is a pretty big deal and a big change and for users who are just
starting to use it, requiring that they write triggers and policies
specifically to address that case might be acceptable. That doesn't
address the cases where users have direct SQL access and might be able
to then bypass existing triggers, but we might be able to work out an
answer to that case..

Other databases have this capability and have triggers and at least one
ends up firing both INSERT and UPDATE triggers, with many complaints
from users about how that ends up making the performance suck. Perhaps
we could use that as a fallback but support the explicit single trigger
option too.. Just some thoughts, apologies if it's already been
convered in depth previously.

Thanks!

Stephen