Re: [HACKERS] MERGE SQL Statement for PG11 - Mailing list pgsql-hackers

Greetings Pavan, all,

* Pavan Deolasee (pavan.deolasee@gmail.com) wrote:
> On 9 March 2018 at 08:29, Peter Geoghegan <pg@bowt.ie> wrote:
> >  My #1 concern has become RLS, and
> > perhaps only because I haven't studied it in enough detail.
>
> Sure. I've done what I thought is the right thing to do, but please check.
> Stephen also wanted to review RLS related code; I don't know if he had
> chance to do so.

I've started looking through the code from an RLS perspective and, at
least on my initial review, it looks alright.

A couple test that aren't included that I think should be in the
regression suire are where both tables have RLS policies and
where the RLS tables have actual SELECT policies beyond just 'true'.

I certainly see SELECT policies which limits the records that
individuals are allowed to see very frequently and it's an
important case to ensure works correctly.  I did a few tests here myself
and they behaved as I expected, and reading through the code it looks
reasonable, but they should be simple to write tests which run very
quickly.

I'm a bit on the fence about it, but as MERGE is added in quite a few
places which previously mentioned UPDATE and DELETE throughout the
system, I wonder if we shouldn't do better than this:

=*# create policy p1 on t1 for merge using ((c1 % 2) = 0);
ERROR:  syntax error at or near "merge"
LINE 1: create policy p1 on t1 for merge using ((c1 % 2) = 0);

Specifically, perhaps we should change that to pick up on MERGE being
asked for there and return an error saying that policies for the MERGE
command aren't supported with a HINT that MERGE respects
INSERT/UPDATE/DELETE policies and that users should use those instead.

Also, in nodeModifyTable.c, there's a comment:

 * The WITH CHECK quals are applied in ExecUpdate() and hence we need
 * not do anything special to handle them.

Which I believe is actually getting at the fact that ExecUpdate() will
run ExecWithCheckOptions(WCO_RLS_UPDATE_CHECK ...) and therefore in
ExecMergeMatched() we don't need to check WCO_RLS_UPDATE_CHECK, but we
do still need to check WCO_RLS_MERGE_UPDATE_CHECK (and that's what the
code does).

  One thing I wonder about there though is if we really need
to segregate those..?  What's actually making WCO_RLS_MERGE_UPDATE_CHECK
different from WCO_RLS_UPDATE_CHECK?  I get that the DELETE case is
different, because in a regular DELETE we'll never even see the row, but
for MERGE, we will see the row (assuming it passes SELECT policies, of
course) and then will check if it matches and that's when we realize
that we've been asked to run a DELETE, so we have the special-case of
WCO_RLS_MERGE_DELETE_CHECK, but that's not true of UPDATE, so I think
this might be adding a bit of unnecessary complication by introducing
WCO_RLS_MERGE_UPDATE_CHECK.