> I have been going over this, with an eye to committing it.
Thanks for looking at this.
> Looking at the first change to create_policy.sgml, it's not quite
> right, but on closer inspection neither was the original text. The
> issue is that in the case of an INSERT ... ON CONFLICT DO NOTHING,
> SELECT policies are only applied if an arbiter clause is present.
> Since that's a pre-existing bug, it should be fixed and back-patched
> separately -- see 0001, attached.
>
> Looking at the paragraph on insert.sgml describing column privileges
> required, I realised that there is another pre-existing bug there --
> it fails to mention privileges required by columns referred to by the
> arbiter index/constraint, and this applies to all forms of ON
> CONFLICT, not just DO UPDATE. Again, I think that should be fixed and
> back-patched separately, which I've done in 0002, in a way intended to
> need no update for ON CONFLICT DO SELECT.
Nice that you found this.
>
> I don't really like that ExecOnConflictSelect() passes CMD_INSERT to
> ExecProcessReturning(), because that's inconsistent with
> ExecOnConflictUpdate(). We could pass CMD_SELECT, adding a new case to
> the switch statement, but I think a neater solution is to change the
> cmdType parameter to a bool "isDelete" parameter, which makes the code
> slightly simpler, because only the DELETE case behaves differently
> from other cases. I considered pulling this out as a separate commit,
> but I don't think that it's really worth it.
Nice - as I mentioned earlier I noticed this was funky but I didn’t want to add
more changes to review. But I’m glad you sorted it as it’s way nicer now.
> Aside from those changes, I made a few other cosmetic changes. If
> you're happy with those, I think it's pretty-much good-to-go.
>
> (Though I need to wait for the release freeze to expire before I can
> push and back-patch 0001 and 0002).
>
> Regards,
> Dean
Thanks for sorting all these changes Dean, all look good to me. The only
thing that gave me pause is this change in analyse.c lines 1238-1247:
/* Process DO SELECT/UPDATE */
if (onConflictClause->action == ONCONFLICT_UPDATE ||
onConflictClause->action == ONCONFLICT_SELECT)
{
/*
* Expressions in the UPDATE targetlist need to be handled like
UPDATE
* not INSERT. We don't need to save/restore this because all
INSERT
* expressions have been parsed already.
*/
pstate->p_is_insert = false;
So in v.24 we set p_is_insert = false for DO SELECT as well.
If I’m understanding the code correctly, this is only used when using
indirection
on target columns for updates, like setting a value inside an array, like for
example:
update example set arr[2] = 10;
So changing this should have any effect, as the DO SELECT doesn’t have any SET
clause.
Logically it makes sense that DO SELECT is p_is_insert = false.
But maybe the comment should be updated to explain this?
I’m not sure how to put it, but maybe:
/*
* Expressions in the UPDATE targetlist need to be handled like UPDATE
* not INSERT. For DO SELECT, this doesn't matter as there is no
* SET clause. We don't need to save/restore this because all INSERT
* expressions have been parsed already.
*/
?
Best
/Viktor
