Обсуждение: Re: [COMMITTERS] pgsql: Add support for INSERT ... ON CONFLICT DO NOTHING/UPDATE.

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

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

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

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

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

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

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?

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

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

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

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

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

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

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

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

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

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?

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

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

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.

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