Thread: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Alvaro Herrera
Date:
Andres Freund wrote: > Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE. Hm, I just realized that the command tag for INSERT ON CONFLICT is still just INSERT. Is that okay? To me, the behavior is different enough that it should have its own tag. I'm not too set on this, but maybe others share this opinion? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Andres Freund
Date:
On 2015-05-20 18:58:16 -0300, Alvaro Herrera wrote: > Andres Freund wrote: > > Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE. > > Hm, I just realized that the command tag for INSERT ON CONFLICT is still > just INSERT. Is that okay? To me, the behavior is different enough > that it should have its own tag. I'm not too set on this, but maybe > others share this opinion? It's actually INSERT for DO NOTHING, and UPSERT for DO UPDATE. It's even documented ;) Andres
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Peter Geoghegan
Date:
On Wed, May 20, 2015 at 2:58 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Hm, I just realized that the command tag for INSERT ON CONFLICT is still > just INSERT. Is that okay? To me, the behavior is different enough > that it should have its own tag. I'm not too set on this, but maybe > others share this opinion? No it isn't. ON CONFLICT DO UPDATE has an "UPSERT" command tag. DO NOTHING has INSERT as its command tag. Are you using an old psql? I thought that that would just result in no command tag being displayed. -- Peter Geoghegan
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Alvaro Herrera
Date:
Peter Geoghegan wrote: > On Wed, May 20, 2015 at 2:58 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > Hm, I just realized that the command tag for INSERT ON CONFLICT is still > > just INSERT. Is that okay? To me, the behavior is different enough > > that it should have its own tag. I'm not too set on this, but maybe > > others share this opinion? > > No it isn't. ON CONFLICT DO UPDATE has an "UPSERT" command tag. DO > NOTHING has INSERT as its command tag. ... ah ... > Are you using an old psql? I thought that that would just result in no > command tag being displayed. Well, I'm using an editor to read the code of CreateCommandTag(), not executing anything. I guess that function needs an update, then, no? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Peter Geoghegan
Date:
On Wed, May 20, 2015 at 3:09 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> Are you using an old psql? I thought that that would just result in no >> command tag being displayed. > > Well, I'm using an editor to read the code of CreateCommandTag(), not > executing anything. I guess that function needs an update, then, no? I think you're right. The initial commit neglected to update that, and only handled it from ProcessQuery(). So it works for PlannedStmts, not raw parse trees. -- Peter Geoghegan
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Peter Geoghegan
Date:
On Wed, May 20, 2015 at 3:14 PM, Peter Geoghegan <pg@heroku.com> wrote: > I think you're right. The initial commit neglected to update that, and > only handled it from ProcessQuery(). So it works for PlannedStmts, not > raw parse trees. Attached patch fixes this. Thanks for the report. -- Peter Geoghegan
Attachment
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Andres Freund
Date:
On 2015-05-20 15:21:49 -0700, Peter Geoghegan wrote: > On Wed, May 20, 2015 at 3:14 PM, Peter Geoghegan <pg@heroku.com> wrote: > > I think you're right. The initial commit neglected to update that, and > > only handled it from ProcessQuery(). So it works for PlannedStmts, not > > raw parse trees. > > Attached patch fixes this. Thanks for the report. > diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c > index a95eff1..8fd5ee8 100644 > --- a/src/backend/tcop/utility.c > +++ b/src/backend/tcop/utility.c > @@ -1898,7 +1898,14 @@ CreateCommandTag(Node *parsetree) > { > /* raw plannable queries */ > case T_InsertStmt: > - tag = "INSERT"; > + { > + InsertStmt *stmt = (InsertStmt *) parsetree; > + > + tag = "INSERT"; > + if (stmt->onConflictClause && > + stmt->onConflictClause->action == ONCONFLICT_UPDATE) > + tag = "UPSERT"; > + } > break; > > case T_DeleteStmt: You realize there's other instances of this in the same damn function?
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Tom Lane
Date:
Andres Freund <andres@anarazel.de> writes: > You realize there's other instances of this in the same damn function? Not to mention that several places in libpq/fe-exec.c should be taught about this new tag. And who-knows-what in other client-side libraries. I am not really sure that it was a good idea to invent this command tag. In fact, I'm pretty sure it was a *bad* idea --- what will happen if we ever create a statement actually named UPSERT? I think we should fix this by ripping out the variant tag, not trying to propagate it everywhere it would need to go. Cute ideas are not the same as good ideas. regards, tom lane
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Andres Freund
Date:
On 2015-05-20 21:22:08 -0400, Tom Lane wrote: > Not to mention that several places in libpq/fe-exec.c should be > taught about this new tag. And who-knows-what in other client-side > libraries. I am not really sure that it was a good idea to invent > this command tag. In fact, I'm pretty sure it was a *bad* idea --- > what will happen if we ever create a statement actually named UPSERT? > > I think we should fix this by ripping out the variant tag, not trying > to propagate it everywhere it would need to go. Cute ideas are not > the same as good ideas. I'm not particularly worried about conflicting with a potential future UPSERT command. But I do see no corresponding benefit in having a differerent command tag, so I'm inclined to agree that ripping it out is likely the best way forward. On the other hand, this was noticed because Alvaro just argued that it *should* have a new command tag. Alvaro, where do you see the advantage? Greetings, Andres Freund
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Peter Geoghegan
Date:
On Wed, May 20, 2015 at 6:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I am not really sure that it was a good idea to invent > this command tag. In fact, I'm pretty sure it was a *bad* idea --- > what will happen if we ever create a statement actually named UPSERT? Why would we invent a statement actually named UPSERT? > I think we should fix this by ripping out the variant tag, not trying > to propagate it everywhere it would need to go. Cute ideas are not > the same as good ideas. I don't feel particularly strongly about it one way or the other. The way the command tag reports number of rows affected beside the INSERT tag in psql is relevant. If some of those rows were actually updated, that could mislead. I'm not saying that it outweighs your concern, but it was the reason for inventing a variant tag, and it is a consideration. -- Peter Geoghegan
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Peter Geoghegan
Date:
On Wed, May 20, 2015 at 6:12 PM, Andres Freund <andres@anarazel.de> wrote: > You realize there's other instances of this in the same damn function? I was misled by the argument name, "parsetree" -- in the past, CreateCommandTag() actually only processed raw parse trees. Beyond that, I wasn't aware that it is possible to produce a command tag for every possible representation of an optimizable statement at every stage of query processing. I guess that I'll know next time I add a command tag. -- Peter Geoghegan
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Robert Haas
Date:
On May 20, 2015, at 9:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Andres Freund <andres@anarazel.de> writes: >> You realize there's other instances of this in the same damn function? > > Not to mention that several places in libpq/fe-exec.c should be > taught about this new tag. And who-knows-what in other client-side > libraries. I am not really sure that it was a good idea to invent > this command tag. In fact, I'm pretty sure it was a *bad* idea --- > what will happen if we ever create a statement actually named UPSERT? > > I think we should fix this by ripping out the variant tag, not trying > to propagate it everywhere it would need to go. +1 ...Robert
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Heikki Linnakangas
Date:
On 05/21/2015 05:08 AM, Peter Geoghegan wrote: > On Wed, May 20, 2015 at 6:22 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I am not really sure that it was a good idea to invent >> this command tag. In fact, I'm pretty sure it was a *bad* idea --- >> what will happen if we ever create a statement actually named UPSERT? > > Why would we invent a statement actually named UPSERT? And if we did, surely it would do some sort of an upsert operation, we could use the UPSERT command tag for that too. That said, I'm also not sure adding the UPSERT command tag is worth the trouble. I'm OK with ripping it out. The row count returned in the command tag is handy in the simple cases, but it gets complicated as soon as you have rules or triggers, so you can't rely much on it anyway. So as long as we document what the count means for an INSERT ... ON CONFLICT, it should be OK to use the INSERT tag. - Heikki
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Alvaro Herrera
Date:
Andres Freund wrote: > On 2015-05-20 21:22:08 -0400, Tom Lane wrote: > > Not to mention that several places in libpq/fe-exec.c should be > > taught about this new tag. And who-knows-what in other client-side > > libraries. I am not really sure that it was a good idea to invent > > this command tag. In fact, I'm pretty sure it was a *bad* idea --- > > what will happen if we ever create a statement actually named UPSERT? > > > > I think we should fix this by ripping out the variant tag, not trying > > to propagate it everywhere it would need to go. Cute ideas are not > > the same as good ideas. > > I'm not particularly worried about conflicting with a potential future > UPSERT command. But I do see no corresponding benefit in having a > differerent command tag, so I'm inclined to agree that ripping it out is > likely the best way forward. > > On the other hand, this was noticed because Alvaro just argued that it > *should* have a new command tag. Alvaro, where do you see the advantage? Well, I was just skimming nearby code and noticed that CreateCommandTag hadn't been updated. As I said elsewhere, I'm not even running commands. I'm not really set on having the tag be different. That said, I'm not sure about having it be the same, either: first, I don't think we need to update the fe-exec.c code at all -- I mean, all the things I see there are very old compatibility stuff; reporting the OID of the just-inserted row? Seriously, who has OIDs in user tables anymore? Surely we wouldn't try to do that for INSERT ON CONFLICT DO UPDATE at all ... if anyone wants that functionality, they can use RETURNING, which is far saner. As for PQcmdTuples, what would a single number returned mean? Are we returning the sum of both inserted and updated tuples? Isn't this a bit odd? If the number is useful, then perhaps we should distinguish the cases; and if it's not useful, why not just return zero? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Alvaro Herrera
Date:
Alvaro Herrera wrote: > That said, I'm not sure about having it be the same, either: first, I > don't think we need to update the fe-exec.c code at all -- I mean, all > the things I see there are very old compatibility stuff; (But as I said earlier, it doesn't really affect me either way, so feel free to rip it out.) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Peter Geoghegan
Date:
On Thu, May 21, 2015 at 4:32 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > (But as I said earlier, it doesn't really affect me either way, so feel > free to rip it out.) That appears to be the consensus. Should I post a patch? -- Peter Geoghegan
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Andres Freund
Date:
On 2015-05-21 20:28:41 -0300, Alvaro Herrera wrote: > That said, I'm not sure about having it be the same, either: first, I > don't think we need to update the fe-exec.c code at all -- I mean, all > the things I see there are very old compatibility stuff; reporting the > OID of the just-inserted row? Seriously, who has OIDs in user tables > anymore? Surely we wouldn't try to do that for INSERT ON CONFLICT DO > UPDATE at all ... if anyone wants that functionality, they can use > RETURNING, which is far saner. The oid currently is reported for UPSERT... I agree it's not worth much, but it seems pointless to break it for a single command. > As for PQcmdTuples, what would a single number returned mean? Are we > returning the sum of both inserted and updated tuples? Yes. > Isn't this a bit odd? Imo it's pretty much in line with what's done with INSTEAD OF, FDWs and such. > If the number is useful, then perhaps we should distinguish the cases; > and if it's not useful, why not just return zero? There's libraries/frameworks checking if an insert succeeded by looking at that number, and it seems like a bad idea to needlessly break those. So I think we're good with ripping it out. Peter?
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Peter Geoghegan
Date:
On Thu, May 21, 2015 at 5:48 PM, Andres Freund <andres@anarazel.de> wrote: > >> If the number is useful, then perhaps we should distinguish the cases; >> and if it's not useful, why not just return zero? > > There's libraries/frameworks checking if an insert succeeded by looking > at that number, and it seems like a bad idea to needlessly break those. +1 > So I think we're good with ripping it out. Peter? I'll come up with a patch. -- Peter Geoghegan
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Peter Geoghegan
Date:
On Thu, May 21, 2015 at 5:51 PM, Peter Geoghegan <pg@heroku.com> wrote: >> So I think we're good with ripping it out. Peter? > > I'll come up with a patch. Attached patch rips the command tag out. -- Peter Geoghegan
Attachment
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Andres Freund
Date:
On 2015-05-22 12:23:32 -0700, Peter Geoghegan wrote: > On Thu, May 21, 2015 at 5:51 PM, Peter Geoghegan <pg@heroku.com> wrote: > >> So I think we're good with ripping it out. Peter? > > > > I'll come up with a patch. > > Attached patch rips the command tag out. Pushed.
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Amit Kapila
Date:
> Andres Freund wrote:
> > Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
>
> > Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
>
Few comments/questions:
1.
insert.sgml
+ column. For example, <literal>INSERT ... ON CONFLICT DO UPDATE
+ tab SET table_name.col = 1</> is invalid (this follows the general
+ behavior for <command>UPDATE</>).
Here in above example shouldn't table_name be used instead of *tab*
after UPDATE?
2.
+ <para>
+ Insert new distributor if possible; otherwise
+ <literal>DO NOTHING</literal>. Example assumes a unique index has been
+ defined that constrains values appearing in the
+ <literal>did</literal> column on a subset of rows where the
+ <literal>is_active</literal> boolean column evaluates to
+ <literal>true</literal>:
+<programlisting>
+ -- This statement could infer a partial unique index on "did"
+ -- with a predicate of "WHERE is_active", but it could also
+ -- just use a regular unique constraint on "did"
+ INSERT INTO distributors (did, dname) VALUES (10, 'Conrad International')
+ ON CONFLICT (did) WHERE is_active DO NOTHING;
+</programlisting>
+ </para>
What does WHERE index_predicate mean for non-partial indexes
or non-expression indexes?
Actually that could cause error even though it is not used
for a unique-index because it would mean that user needs
to have Select privilige on column in used in WHERE clause.
Create table spec_insert(c1 int, c2 int);
Create unique index idx_si on spec_insert(c1);
insert into spec_insert values(1) ON Conflict (c1) where c2 > 2 DO Nothing;
If above insert is executed by user who doesn't have Select privilege
on C2, it will give error.
3.
heap_abort_speculative()
+ /*
+ * Set the tuple header xmin to InvalidTransactionId. This makes the
+ * tuple immediately invisible everyone. (In particular, to any
+ * transactions waiting on the speculative token, woken up later.)
/invisible everyone/invisible to everyone
4.
ExecInsert()
+ * speculatively. See the executor README for a full discussion
+ * of speculative insertion.
I could not find any updates about speculative insertion in executor/README,
am I missing the update?
5.
ExecInsert()
{
..
if (onconflict != ONCONFLICT_NONE && resultRelInfo->ri_NumIndices > 0)
{
..
if (!ExecCheckIndexConstraints(slot, estate, &conflictTid,
arbiterIndexes))
..
specToken = SpeculativeInsertionLockAcquire(GetCurrentTransactionId());
..
}
Here why do we need to perform speculative insertion for the
case when there is no constraint/index that can cause conflict?
For example, below case:
Create table spec_insert(c1 int);
Create index idx_si on spec_insert(c1);
insert into spec_insert values(1) ON Conflict DO Nothing;
6.
ExecInsert()
{
..
if (ExecOnConflictUpdate(mtstate, resultRelInfo,
&conflictTid, planSlot, slot,
estate, canSetTag, &returning))
{
InstrCountFiltered2(&mtstate->ps, 1);
..
}
ExecOnConflictUpdate()
{
..
if (!ExecQual(onConflictSetWhere, econtext, false))
{
ReleaseBuffer(buffer);
InstrCountFiltered1(&mtstate->ps, 1);
..
}
If ExecOnConflictUpdate() returns due to Qual (Qualification
is not satisfied), then it will result in counting both
Filtered1 and Filtered2. I think for such a case only one
of them should be updated, probably Filtered1.
7.
create table t1(c1 int, c2 int);
create unique index idx_t1 on t1(c1);
insert into t1 values(1,1);
postgres=# insert into t1 values(1, 1) On Conflict(c1) Do Update set c1=2 where c2 > 3;
ERROR: column reference "c2" is ambiguous
LINE 1: ...alues(1, 1) On Conflict(c1) Do Update set c1=2 where c2 > 3;
Why alias is required in Where condition whereas it works for Set?
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Simon Riggs
Date:
On 22 May 2015 at 00:28, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
--
> On the other hand, this was noticed because Alvaro just argued that it
> *should* have a new command tag. Alvaro, where do you see the advantage?
Well, I was just skimming nearby code and noticed that CreateCommandTag
hadn't been updated. As I said elsewhere, I'm not even running
commands. I'm not really set on having the tag be different.
I agree with Alvaro that we need to be able to see a difference.
I also agree with Tom/Robert that we should not invent a new tag.
What I think should happen is that the command tag should vary according to whether an INSERT or an UPDATE was performed, so we get a visible difference without any new tags.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Alvaro Herrera
Date:
Simon Riggs wrote: > What I think should happen is that the command tag should vary according to > whether an INSERT or an UPDATE was performed, so we get a visible > difference without any new tags. The problem with doing that is that the same command might have updated some tuples and inserted others. Also, we build the tag before the command is executed. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.
From
Simon Riggs
Date:
On 27 May 2015 at 15:06, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
--
Simon Riggs wrote:
> What I think should happen is that the command tag should vary according to
> whether an INSERT or an UPDATE was performed, so we get a visible
> difference without any new tags.
The problem with doing that is that the same command might have updated
some tuples and inserted others. Also, we build the tag before the
command is executed.
Hmm, I assumed the various anomalies meant that it was a single row only command...
Anybody know where the various anomalies are documented? e.g. how if an insert has been committed that we can't yet see whether we are allowed to update that using same rules as update.
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services