Re: ON CONFLICT DO SELECT (take 3) - Mailing list pgsql-hackers
| From | Viktor Holmberg |
|---|---|
| Subject | Re: ON CONFLICT DO SELECT (take 3) |
| Date | |
| Msg-id | c41471bd-deaf-4030-ab80-af4008ac1fa2@Spark Whole thread Raw |
| In response to | Re: ON CONFLICT DO SELECT (take 3) (Dean Rasheed <dean.a.rasheed@gmail.com>) |
| Responses |
Re: ON CONFLICT DO SELECT (take 3)
|
| List | pgsql-hackers |
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.
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
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
pgsql-hackers by date: