Re: [HACKERS] MERGE SQL Statement for PG11

Greetings Pavan,

* Pavan Deolasee (pavan(dot)deolasee(at)gmail(dot)com) wrote:
> On Tue, Feb 6, 2018 at 3:37 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > Coming out of that, my understanding is that Simon is planning to have a
> > patch which implements RLS and partitioning (though the query plans for
> > partitioning may be sub-par and not ideal) as part of MERGE and I've
> > agreed to review at least the RLS bits (though my intention is to at
> > least go through the rest of the patch as well, though likely in less
> > detail). Of course, I encourage others to review it as well.
>
> Thanks for volunteering to review the RLS bits. I am planning to send a
> revised version soon. As I work through it, I am faced with some semantic
> questions again. Would appreciate if you or anyone have answers to those.

Thanks for working on this.

> While executing MERGE, for existing tuples in the target table, we may end
> up doing an UPDATE or DELETE, depending on the WHEN MATCHED AND conditions.
> So it seems unlikely that we would be able to push down USING security
> quals down to the scan. For example, if the target row is set for deletion,
> it seems wrong to exclude the row from the join based on UPDATE policy's
> USING quals. So I am thinking that we should apply the respective USING
> quals *after* the decision to either update, delete or do nothing for the
> given target tuple is made.
>
> The question I have is, if the USING qual evaluates to false or NULL,
> should we silently ignore the tuple (like regular UPDATE does) or throw an
> error (like INSERT ON CONFLICT DO UPDATE)? ISTM that we might have decided
> to throw an error in case of INSERT ON CONFLICT to avoid any confusion
> where the tuple is neither inserted nor updated. Similar situation may
> arise with MERGE because for a source row, we may neither do UPDATE
> (because of RLS) nor INSERT because a matching tuple already exists. But
> someone may argue that we should stay closer to regular UPDATE/DELETE.

The reasoning behind the INSERT ON CONFLICT DO UPDATE approach when it
comes to RLS is that we specifically didn't want to end up "losing"
data- an INSERT which doesn't actually INSERT a row (or take the UPDATE
action if the row already exists) ends up throwing that data away even
though clearly the user expected us to do something with it, which is
why we throw an error in that case instead.

For my part, at least, it seems like MERGE would likewise possibly be
throwing away data that the user is expecting to be incorporated if no
action is taken due to RLS and that then argues that we should be
throwing an error in such a case, similar to the INSERT ON CONFLICT DO
UPDATE case.

> Apart from that, are there any security angles that we need to be mindful
> of and would those impact the choice?

Regarding the above, no security issues come to mind with either
approach. The security concerns are primairly not allowing rows to be
directly seen which the user does not have access to, and not allowing
the action to result in rows being added, modified, or removed in a way
which would violate the policies defined.

> SELECT policies will be applied to the target table during the scan and
> rows which do not pass SELECT quals will not be processed at all. If there
> are NOT MATCHED actions, we might end up inserting duplicate rows in that
> case or throw errors, but I don't see anything wrong with that. Similar
> things would have happened if someone tried to insert rows into the table
> using regular INSERT.

I agree, this seems like the right approach.

> Similarly, INSERT policies will be applied when MERGE attempts to INSERT a
> row into the table and error will be thrown if the row does not satisfy
> INSERT policies.

That sounds correct to me.

Thanks!

Stephen