Re: INSERT ... ON CONFLICT UPDATE and RLS

Dean,

* Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> On 9 January 2015 at 20:26, Stephen Frost <sfrost@snowman.net> wrote:
> > Where this leaves me, at least, is feeling like we should always apply
> > the INSERT WITH CHECK policy, then if there is a conflict, check the
> > UPDATE USING policy and throw an error if the row isn't visible but
> > otherwise perform the UPDATE and then check the UPDATE WITH CHECK
> > policy.
>
> Yeah, I can see the logic in that, but I'm starting to question the
> final "then" in the above, i.e., the order in which the checks are
> done.
>
> Currently we're applying RLS CHECKs after the INSERT or UPDATE, like
> WITH CHECK OPTIONs on views. The SQL spec says that WITH CHECK OPTIONs
> on views have to be applied after the INSERT/UPDATE on the base
> relation, but we're free to do something different for RLS CHECKs if
> that makes more sense. If we want RLS to be more like column-level
> privilege checking, then it does make sense to do the checks sooner,
> so perhaps we should be checking the RLS policies before the
> INSERT/UPDATE, like CHECK constraints.

I've been wondering about the placement of the checks also.  I keep
meaning to go back to the SQL spec and review the WITH CHECK requirement
as it feels a bit odd to me, but then again, it is the spec.  As you
say, we're not obligated to follow it for RLS in any case.

> Doing that, it makes sense to first check INSERT CHECK policies,
> potentially throwing an error before any PK conflict is detected, so
> an error would result even in the INSERT .. ON CONFLICT IGNORE case.
> That way INSERT CHECK policies would always be applied regardless of
> the path taken, as you suggest.

Right.

> I would still say that the RLS UPDATE USING/CHECK policies should only
> be applied if the UPDATE path is taken (before the UPDATE is applied),
> and they need to be applied to the (old/new) update tuples only. It's
> a bit like there is a WHERE <record already exists> clause attached to
> the UPDATE. The RLS UPDATE policies should only be applied to the rows
> affected by the UPDATE.

Agreed.  I don't see how we could possibly do otherwise.  The only tuple
we have at the outset is the to-be-INSERT'd tuple.  We won't find the
conflicting tuple until we try and even then that conflicting tuple
might be different from the to-be-INSERT'd tuple, so we can't check that
until we've discovered the conflict exists.  Further, we won't have the
tuple to check for the UPDATE's CHECK policy until all of the
expressions (if any) are run on the UPDATE side.

> > I see your point that this runs counter to the 'mod_count'
> > example use-case and could cause problems for users using RLS with such
> > a strategy.  For my part, I expect users of RLS to expect errors in such
> > a case rather than allowing it, but it's certainly a judgement call.
>
> Actually I don't think this is a problem for the mod_count example.
> This isn't the same as AND'ing together the INSERT and UPDATE quals,
> because each set of policies is being applied to a different tuple.
> There are 3 tuples in play here:-
>
> 1). The insert tuple, which has INSERT CHECK polcies applied to it
> 2). The existing (old) update tuple, which has UPDATE USING policies
> applied to it
> 3). The (new) update tuple, which has UPDATE CHECK policies applied to it

Right.

> NOTE: If we do change RLS CHECKs to be executed prior to modifying any
> data, that's potentially a change that could be made independently of
> the UPSERT patch. We should also probably then stop referring to them
> as WITH CHECK OPTIONs in the docs and in error messages because
> they're not the same thing, and the code might be neater if they had
> their own data-structure rather than overloading WithCheckOption.

I agree that's an independent change.  I'm not sure if it would end up
being neater or not offhand but I can see some advantages to breaking it
up from WithCheck as it might be simpler for users to understand
(particularly those users who are not already familiar with WithCheck).
Were you thinking about working up a patch for such a change?  If not,
I'll see about finding time to do it, unless someone else wants to
volunteer. :)
Thanks!
    Stephen