[PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.

Marti Raudsepp <marti@juffo.org> writes:

This patch returns command tag "CREATE X" or "REPLACE X" for
LANGAUGE/VIEW/RULE/FUNCTION. This is done by passing completionTag to
from ProcessUtility to more functions, and adding a 'bool *didUpdate'
argument to some lower-level functions. I'm not sure if passing back
the status in a bool* is considered good style, but this way all the
functions look consistent.

This is going to break clients that expect commands to return the same
command tag as they have in the past. I doubt that whatever usefulness
is gained will outweigh the compatibility problems.

regards, tom lane

On Sun, Nov 28, 2010 at 11:29 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Marti Raudsepp <marti@juffo.org> writes:

This patch returns command tag "CREATE X" or "REPLACE X" for
LANGAUGE/VIEW/RULE/FUNCTION. This is done by passing completionTag to
from ProcessUtility to more functions, and adding a 'bool *didUpdate'
argument to some lower-level functions. I'm not sure if passing back
the status in a bool* is considered good style, but this way all the
functions look consistent.

This is going to break clients that expect commands to return the same
command tag as they have in the past.  I doubt that whatever usefulness
is gained will outweigh the compatibility problems.

You complained about this when we changed the SELECT tag for 9.0 to
include row-counts for CTAS etc. where it hadn't before. Have we
gotten any complaints about that change breaking clients?

I think more expessive command tags are in general a good thing. The
idea that this particular change would be useful primarily for humans
examining the psql output seems a bit weak to me, but I can easily see
it being useful for programs. Right now a program has no reason to
look at this command tag anyway; it'll always be the same.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

I think more expessive command tags are in general a good thing. The
idea that this particular change would be useful primarily for humans
examining the psql output seems a bit weak to me, but I can easily see
it being useful for programs. Right now a program has no reason to
look at this command tag anyway; it'll always be the same.

Hmm ... that's a good point, although I'm not sure that it's 100% true.

regards, tom lane

I tried to pick up this patch to review.

It seems to me fine, enough simple and works as explained in the
implementation level, apart from reasonability of this feature.
(Tom was not 100% agree with this feature 1.5month ago.)

I'm not certain whether the current regression test should be
updated, or not. The pg_regress launches psql with -q option,
so completionTag is always ignored.

Thanks,

(2010/11/29 0:14), Marti Raudsepp wrote:

Hi list,

Often enough when developing PostgreSQL views and functions, I have
pasted the CREATE OR REPLACE commands into the wrong window/shell and
ran them there without realizing that I'm creating a function in the
wrong database, instead of replacing. Currently psql does not provide
any feedback of which action really occured.

Only after writing this patch I realized that I could instead raise a
NOTICE, like current IF EXISTS/IF NOT EXISTS clauses do. Is that a
better way to solve this?

This patch returns command tag "CREATE X" or "REPLACE X" for
LANGAUGE/VIEW/RULE/FUNCTION. This is done by passing completionTag to
from ProcessUtility to more functions, and adding a 'bool *didUpdate'
argument to some lower-level functions. I'm not sure if passing back
the status in a bool* is considered good style, but this way all the
functions look consistent.

Regards,
Marti

--
KaiGai Kohei <kaigai@ak.jp.nec.com>

2011/1/13 KaiGai Kohei <kaigai@ak.jp.nec.com>:

I tried to pick up this patch to review.

It seems to me fine, enough simple and works as explained in the
implementation level, apart from reasonability of this feature.
(Tom was not 100% agree with this feature 1.5month ago.)

Did you check whether this updated the code for 100% of the object
types where this could apply?

I'm not certain whether the current regression test should be
updated, or not. The pg_regress launches psql with -q option,
so completionTag is always ignored.

Well, I don't see any easy way of regression testing it, then. Am I
missing something?

Also, I don't really like the way this spreads knowledge of the
completionTag out all over the backend. I think it would be better to
follow the existing model used by the COPY and COMMIT commands,
whereby the return value indicates what happened and
standard_ProcessUtility() uses that to set the command tag.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Robert Haas's message of vie ene 14 08:40:07 -0300 2011:

Also, I don't really like the way this spreads knowledge of the
completionTag out all over the backend. I think it would be better to
follow the existing model used by the COPY and COMMIT commands,
whereby the return value indicates what happened and
standard_ProcessUtility() uses that to set the command tag.

Yeah, that looks ugly. However it's already ugly elsewhere: for example
see PerformPortalFetch. I am not sure if it should be this patch's
responsability to clean that stuff up. (Maybe we should decree that at
least this patch shouldn't make the situation worse.)

--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On Fri, Jan 14, 2011 at 9:24 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:

Excerpts from Robert Haas's message of vie ene 14 08:40:07 -0300 2011:

Also, I don't really like the way this spreads knowledge of the
completionTag out all over the backend.  I think it would be better to
follow the existing model used by the COPY and COMMIT commands,
whereby the return value indicates what happened and
standard_ProcessUtility() uses that to set the command tag.

Yeah, that looks ugly.  However it's already ugly elsewhere: for example
see PerformPortalFetch.  I am not sure if it should be this patch's
responsability to clean that stuff up.  (Maybe we should decree that at
least this patch shouldn't make the situation worse.)

Agreed: it's not the patch's job to clean it up, but it shouldn't make
the situation worse.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Robert Haas's message of vie ene 14 08:40:07 -0300 2011:

Also, I don't really like the way this spreads knowledge of the
completionTag out all over the backend. I think it would be better to
follow the existing model used by the COPY and COMMIT commands,
whereby the return value indicates what happened and
standard_ProcessUtility() uses that to set the command tag.

Yeah, that looks ugly. However it's already ugly elsewhere: for example
see PerformPortalFetch. I am not sure if it should be this patch's
responsability to clean that stuff up. (Maybe we should decree that at
least this patch shouldn't make the situation worse.)

I thought we were going to reject the patch outright anyway. The
compatibility consequences of changing command tags are not worth the
benefit, independently of how ugly the backend-side code may or may
not be.

regards, tom lane

On Fri, 2011-01-14 at 12:07 -0500, Tom Lane wrote:

I thought we were going to reject the patch outright anyway. The
compatibility consequences of changing command tags are not worth the
benefit, independently of how ugly the backend-side code may or may
not be.

+1

--
Simon Riggs http://www.2ndQuadrant.com/books/
PostgreSQL Development, 24x7 Support, Training and Services

On Fri, Jan 14, 2011 at 12:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alvaro Herrera <alvherre@commandprompt.com> writes:

Excerpts from Robert Haas's message of vie ene 14 08:40:07 -0300 2011:

Also, I don't really like the way this spreads knowledge of the
completionTag out all over the backend.  I think it would be better to
follow the existing model used by the COPY and COMMIT commands,
whereby the return value indicates what happened and
standard_ProcessUtility() uses that to set the command tag.

Yeah, that looks ugly.  However it's already ugly elsewhere: for example
see PerformPortalFetch.  I am not sure if it should be this patch's
responsability to clean that stuff up.  (Maybe we should decree that at
least this patch shouldn't make the situation worse.)

I thought we were going to reject the patch outright anyway.  The
compatibility consequences of changing command tags are not worth the
benefit, independently of how ugly the backend-side code may or may
not be.

My previous response to this criticism was here:

http://archives.postgresql.org/pgsql-hackers/2010-11/msg01899.php

Your response, which seemed at least partially in agreement, is here:

http://archives.postgresql.org/pgsql-hackers/2010-11/msg01901.php

If we're going to reject this patch on backwards-compatibility
grounds, we need to make an argument that the backward-compatibility
hazards are a real concern. So, again, has anyone complained about
the changes we made in this area in 9.0? And under what circumstances
do we foresee someone relying on the command tag of a command that
always returns the same tag? I'm as quick as anyone to bow before a
compelling argument, but I don't think anyone's made such an argument.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

If we're going to reject this patch on backwards-compatibility
grounds, we need to make an argument that the backward-compatibility
hazards are a real concern. So, again, has anyone complained about
the changes we made in this area in 9.0?

That 9.0 change was far less invasive than this: it only added a count
field to SELECT and CTAS result tags. Quite aside from the fact that
the tag name stayed the same, in the SELECT case it's unlikely anyone
would have checked the tag at all rather than just testing for
PQresultStatus() == PGRES_TUPLES_OK. So it was basically only changing
the result for *one* command type. I don't think it's a good basis for
arguing that this patch won't cause problems.

regards, tom lane

On Fri, Jan 14, 2011 at 1:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

If we're going to reject this patch on backwards-compatibility
grounds, we need to make an argument that the backward-compatibility
hazards are a real concern.  So, again, has anyone complained about
the changes we made in this area in 9.0?

That 9.0 change was far less invasive than this: it only added a count
field to SELECT and CTAS result tags.  Quite aside from the fact that
the tag name stayed the same, in the SELECT case it's unlikely anyone
would have checked the tag at all rather than just testing for
PQresultStatus() == PGRES_TUPLES_OK.  So it was basically only changing
the result for *one* command type.  I don't think it's a good basis for
arguing that this patch won't cause problems.

Yeah, but that one command tag was SELECT. That's a pretty commonly
used command. Most production environments probably use all of the
commands affected by this patch together an order of magnitude less
often than they use SELECT.

Again, on what basis are we arguing that people are going to be
looking at the command tag of a command that always returns the same
tag? That seems pretty darn unlikely to me.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Jan 14, 2011 at 1:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

That 9.0 change was far less invasive than this: it only added a count
field to SELECT and CTAS result tags. �Quite aside from the fact that
the tag name stayed the same, in the SELECT case it's unlikely anyone
would have checked the tag at all rather than just testing for
PQresultStatus() == PGRES_TUPLES_OK.

Yeah, but that one command tag was SELECT. That's a pretty commonly
used command.

You're ignoring the point that people would probably use PQresultStatus
in preference to checking the tag at all, when dealing with SELECT.
psql itself is an example --- it never looks at the tag, nor shows it to
the user, in the SELECT case. That patch only really changed the
exposed behavior for CREATE TABLE AS SELECT / SELECT INTO, neither of
which can be claimed to be hugely popular things for programs to issue.

The other side of the argument that needs to be considered is what the
benefit is. There was a fairly clear functional gain from reporting
the rowcount for CTAS. I'm less convinced that sending back REPLACE
is a big benefit worth taking big compatibility risks for.

regards, tom lane

On Fri, Jan 14, 2011 at 2:07 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Jan 14, 2011 at 1:35 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

That 9.0 change was far less invasive than this: it only added a count
field to SELECT and CTAS result tags.  Quite aside from the fact that
the tag name stayed the same, in the SELECT case it's unlikely anyone
would have checked the tag at all rather than just testing for
PQresultStatus() == PGRES_TUPLES_OK.

Yeah, but that one command tag was SELECT.  That's a pretty commonly
used command.

You're ignoring the point that people would probably use PQresultStatus
in preference to checking the tag at all, when dealing with SELECT.

I would assume they would also use PQresultStatus() when checking
whether CREATE OR REPLACE FUNCTION worked. Even if they were using
PQcmdStatus() for some reason, which seems like an odd thing to do,
there'd be no reason to check for anything beyond "is it empty?". The
idea that there are massive amounts of code out there that are
expecting the command tag to be *exactly* CREATE FUNCTION and will
break if it differs by a byte seems quite improbable. Can you produce
an example of any such code?

The other side of the argument that needs to be considered is what the
benefit is.  There was a fairly clear functional gain from reporting
the rowcount for CTAS.  I'm less convinced that sending back REPLACE
is a big benefit worth taking big compatibility risks for.

Asserting that there are "big compatibility risks" doesn't make it so
- you've offered no evidence of that. Even if a handful of people had
complained about that one, I would still felt it was a good change,
but it doesn't seem that there are any at all. I classify this as one
of a dozen or two minor usability enhancements that we make in every
release, and most people don't care, and those who do go "oh, that's
handy". I think before we reject a patch for breaking things, we
ought to be able to identify either some actual application that is
broken by it, or at least some reasonably plausible coding pattern
that would blow up.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thanks for reviewing!

On Fri, Jan 14, 2011 at 13:40, Robert Haas <robertmhaas@gmail.com> wrote:

Did you check whether this updated the code for 100% of the object
types where this could apply?

I walked through all the CREATE statements in the documentation and
these four seem to be the only ones that accept FOR REPLACE.

There's a similar case with CREATE TABLE IF NOT EXISTS, maybe this is
worth covering in an updated patch too?
And if I change that, people might expect the same from DROP X IF EXISTS too?

Also, I don't really like the way this spreads knowledge of the
completionTag out all over the backend.  I think it would be better to
follow the existing model used by the COPY and COMMIT commands,
whereby the return value indicates what happened and
standard_ProcessUtility() uses that to set the command tag.

Right. I created this pattern after PerformPortalFetch() which already
took a completionTag argument. But your approach seems more
reasonable.

Regards,
Marti

On Fri, Jan 14, 2011 at 3:45 PM, Marti Raudsepp <marti@juffo.org> wrote:

There's a similar case with CREATE TABLE IF NOT EXISTS, maybe this is
worth covering in an updated patch too?
And if I change that, people might expect the same from DROP X IF EXISTS too?

It's far less clear what you'd change those cases to say, and they
already emit a NOTICE, so it seems unnecessary.

Also, I don't really like the way this spreads knowledge of the
completionTag out all over the backend.  I think it would be better to
follow the existing model used by the COPY and COMMIT commands,
whereby the return value indicates what happened and
standard_ProcessUtility() uses that to set the command tag.

Right. I created this pattern after PerformPortalFetch() which already
took a completionTag argument. But your approach seems more
reasonable.

OK.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Here's an updated patch that reports command status back to
ProcessUtility via 'bool' return value.

I was a bit unsure about using bool return values because it's not
immediately obvious what "true" or "false" refer to, but defining a
new enum seemed like overkill, so I went with bool anyway. Any better
ideas?

The 2nd patch also moves MOVE/FETCH command tag formatting up to
ProcessUtility, hopefully this change is for the better.

Regards,
Marti

On fre, 2011-01-14 at 18:45 -0500, Robert Haas wrote:

On Fri, Jan 14, 2011 at 3:45 PM, Marti Raudsepp <marti@juffo.org>
wrote:

There's a similar case with CREATE TABLE IF NOT EXISTS, maybe this

is

worth covering in an updated patch too?
And if I change that, people might expect the same from DROP X IF

EXISTS too?

It's far less clear what you'd change those cases to say, and they
already emit a NOTICE, so it seems unnecessary.

Maybe instead of the proposed patch, a notice could be added:

NOTICE: existing object was replaced

On Mon, Jan 17, 2011 at 9:41 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

On fre, 2011-01-14 at 18:45 -0500, Robert Haas wrote:

On Fri, Jan 14, 2011 at 3:45 PM, Marti Raudsepp <marti@juffo.org>
wrote:

There's a similar case with CREATE TABLE IF NOT EXISTS, maybe this

is

worth covering in an updated patch too?
And if I change that, people might expect the same from DROP X IF

EXISTS too?

It's far less clear what you'd change those cases to say, and they
already emit a NOTICE, so it seems unnecessary.

Maybe instead of the proposed patch, a notice could be added:

NOTICE: existing object was replaced

Well, that would eliminate the backward-compatibility hazard, pretty
much, but it seems noisy. I already find some of these notices to be
unduly informative.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company