Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Sat, Jan 22, 2011 at 9:11 PM, Robert Haas robertmh...@gmail.com wrote: It seems no one wants to put any further effort into this problem. Bummer. Since no one has felt the need to dispute the above statement in the last 6 days, it seems clear to mark this Returned with Feedback, which I have now done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Fri, Jan 28, 2011 at 21:04, Robert Haas robertmh...@gmail.com wrote: On Sat, Jan 22, 2011 at 9:11 PM, Robert Haas robertmh...@gmail.com wrote: It seems no one wants to put any further effort into this problem. Bummer. Since no one has felt the need to dispute the above statement in the last 6 days, it seems clear to mark this Returned with Feedback, which I have now done. Just to be clear, I could put more effort into this, but I agree with your concerns about this introducing inconsistency. Given how few people are interested in this change, it's probably best to drop it. Regards, Marti -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Mon, Jan 17, 2011 at 2:52 PM, Robert Haas robertmh...@gmail.com wrote: On Mon, Jan 17, 2011 at 2:25 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Jan 17, 2011 at 9:41 AM, Peter Eisentraut pete...@gmx.net wrote: 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. ROTFL ... There has been some previous banter about reorganizing or reclassifying the various NOTICE messages to make them more useful and/or less noisy; but I don't think we've ever had a really concrete proposal for better behavior. Maybe it's time to reopen that discussion. I do agree with Peter's underlying point: it would be pretty inconsistent for CREATE OR REPLACE to report this bit of info via command tag when CREATE IF NOT EXISTS is reporting an absolutely equivalent bit of info via elog(NOTICE). There's a fine line between serious discussion, humor, and outright mockery here, and I'm not too sure which one Peter's currently engaged in. I guess the point here for me is that commands tags for SELECT, INSERT, UPDATE, and DELETE all return some useful information about what actually happened - especially, a row count. If it's reasonable for those commands to return a row count in the command tag, then there's no reason why utility commands shouldn't also be allowed to return high-level status information as part of the command tag. On the flip side we could just rip out command tags altogether and have psql print out the first two words of the input string. The asymmetry between DROP-IF-EXISTS and CREATE-IF-NOT-EXISTS and the proposed CREATE-OR-REPLACE behavior doesn't bother me very much, because it's already asymmetric: the first two currently report what happened, and the third one currently doesn't. If you want to propose to make all of them consistent, how? I don't particularly like the idea of adding a NOTICE here; we've got too many of those already[1]. Making DIE report that it didn't do anything via a command tag clearly won't work, because you can say DROP IF EXISTS foo, bar, baz and the answer might not be the same in all three cases, but COR has no such issue. It seems no one wants to put any further effort into this problem. Bummer. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On fre, 2011-01-14 at 18:45 -0500, Robert Haas wrote: On Fri, Jan 14, 2011 at 3:45 PM, Marti Raudsepp ma...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Mon, Jan 17, 2011 at 9:41 AM, Peter Eisentraut pete...@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 ma...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On mån, 2011-01-17 at 10:05 -0500, Robert Haas wrote: On Mon, Jan 17, 2011 at 9:41 AM, Peter Eisentraut pete...@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 ma...@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. I'm also anti-NOTICE. I'm just saying, we propose that CREATE OR REPLACE should return a tag of CREATE or REPLACE depending on what it did, then DROP IF NOT EXISTS should also return a tag of DROP or ??? depending on what it did. Since the latter question was settled by a notice, that would also be the proper answer for the former. Perhaps the next thing is that MERGE should return INSERT or UPDATE depending on the outcome? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
Robert Haas robertmh...@gmail.com writes: On Mon, Jan 17, 2011 at 9:41 AM, Peter Eisentraut pete...@gmx.net wrote: 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. ROTFL ... There has been some previous banter about reorganizing or reclassifying the various NOTICE messages to make them more useful and/or less noisy; but I don't think we've ever had a really concrete proposal for better behavior. Maybe it's time to reopen that discussion. I do agree with Peter's underlying point: it would be pretty inconsistent for CREATE OR REPLACE to report this bit of info via command tag when CREATE IF NOT EXISTS is reporting an absolutely equivalent bit of info via elog(NOTICE). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Mon, Jan 17, 2011 at 09:23:07PM +0200, Peter Eisentraut wrote: On mån, 2011-01-17 at 10:05 -0500, Robert Haas wrote: On Mon, Jan 17, 2011 at 9:41 AM, Peter Eisentraut pete...@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 ma...@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. I'm also anti-NOTICE. I'm just saying, we propose that CREATE OR REPLACE should return a tag of CREATE or REPLACE depending on what it did, then DROP IF NOT EXISTS should also return a tag of DROP or ??? depending on what it did. Since the latter question was settled by a notice, that would also be the proper answer for the former. Perhaps the next thing is that MERGE should return INSERT or UPDATE depending on the outcome? Given that it can do both in a single statement, I'm guessing that this is intended to be facetious. Or are you suggesting that the command tags become an array? This has all kinds of interesting possibilities, but would of course break all kinds of stuff in the process. Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Mon, Jan 17, 2011 at 2:25 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Jan 17, 2011 at 9:41 AM, Peter Eisentraut pete...@gmx.net wrote: 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. ROTFL ... There has been some previous banter about reorganizing or reclassifying the various NOTICE messages to make them more useful and/or less noisy; but I don't think we've ever had a really concrete proposal for better behavior. Maybe it's time to reopen that discussion. I do agree with Peter's underlying point: it would be pretty inconsistent for CREATE OR REPLACE to report this bit of info via command tag when CREATE IF NOT EXISTS is reporting an absolutely equivalent bit of info via elog(NOTICE). There's a fine line between serious discussion, humor, and outright mockery here, and I'm not too sure which one Peter's currently engaged in. I guess the point here for me is that commands tags for SELECT, INSERT, UPDATE, and DELETE all return some useful information about what actually happened - especially, a row count. If it's reasonable for those commands to return a row count in the command tag, then there's no reason why utility commands shouldn't also be allowed to return high-level status information as part of the command tag. On the flip side we could just rip out command tags altogether and have psql print out the first two words of the input string. The asymmetry between DROP-IF-EXISTS and CREATE-IF-NOT-EXISTS and the proposed CREATE-OR-REPLACE behavior doesn't bother me very much, because it's already asymmetric: the first two currently report what happened, and the third one currently doesn't. If you want to propose to make all of them consistent, how? I don't particularly like the idea of adding a NOTICE here; we've got too many of those already[1]. Making DIE report that it didn't do anything via a command tag clearly won't work, because you can say DROP IF EXISTS foo, bar, baz and the answer might not be the same in all three cases, but COR has no such issue. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company [1] rhaas=# create table foo (a serial primary key); NOTICE: CREATE TABLE will create implicit sequence foo_a_seq for serial column foo.a NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index foo_pkey for table foo CREATE TABLE Well, yeah, why did I say primary key if I didn't want a primary key index to be created, and why did I say serial if I didn't want a sequence? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
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
From 25a19ca972e6f02ba350b6e4112935ff1ed44b24 Mon Sep 17 00:00:00 2001
From: Marti Raudsepp ma...@juffo.org
Date: Sun, 28 Nov 2010 16:49:41 +0200
Subject: [PATCH 1/2] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
This affects CREATE OR REPLACE LANGUAGE/VIEW/RULE/FUNCTION
Related functions that previously returned void have been changed to
bool to report status up. Others have a new argument bool *didReplace.
---
src/backend/catalog/pg_aggregate.c |3 +-
src/backend/catalog/pg_proc.c |6 -
src/backend/commands/functioncmds.c | 13 --
src/backend/commands/proclang.c | 34 ++-
src/backend/commands/view.c | 22 +
src/backend/rewrite/rewriteDefine.c | 33 ++-
src/backend/tcop/utility.c | 43 +++---
src/include/catalog/pg_proc_fn.h|3 +-
src/include/commands/defrem.h |2 +-
src/include/commands/proclang.h |2 +-
src/include/commands/view.h |2 +-
src/include/rewrite/rewriteDefine.h |4 +-
12 files changed, 119 insertions(+), 48 deletions(-)
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index 86e8c6b..95e4469 100644
--- a/src/backend/catalog/pg_aggregate.c
+++ b/src/backend/catalog/pg_aggregate.c
@@ -229,7 +229,8 @@ AggregateCreate(const char *aggName,
NIL, /* parameterDefaults */
PointerGetDatum(NULL), /* proconfig */
1, /* procost */
- 0); /* prorows */
+ 0, /* prorows */
+ NULL); /* didReplace */
/*
* Okay to create the pg_aggregate entry.
diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 2ab87d2..8b15efc 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -85,7 +85,8 @@ ProcedureCreate(const char *procedureName,
List *parameterDefaults,
Datum proconfig,
float4 procost,
-float4 prorows)
+float4 prorows,
+bool *didReplace)
{
Oid retval;
int parameterCount;
@@ -650,6 +651,9 @@ ProcedureCreate(const char *procedureName,
AtEOXact_GUC(true, save_nestlevel);
}
+ if(didReplace)
+ *didReplace = is_update;
+
return retval;
}
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 2a2b7c7..4678d9c 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -768,8 +768,11 @@ interpret_AS_clause(Oid languageOid, const char *languageName,
/*
* CreateFunction
* Execute a CREATE FUNCTION utility statement.
+ *
+ * Returns TRUE if function was replaced via CREATE OR REPLACE, FALSE
+ * if created.
*/
-void
+bool
CreateFunction(CreateFunctionStmt *stmt, const char *queryString)
{
char *probin_str;
@@ -791,7 +794,8 @@ CreateFunction(CreateFunctionStmt *stmt, const char *queryString)
Oid requiredResultType;
bool isWindowFunc,
isStrict,
-security;
+security,
+didReplace;
char volatility;
ArrayType *proconfig;
float4 procost;
@@ -958,7 +962,10 @@ CreateFunction(CreateFunctionStmt *stmt, const char *queryString)
parameterDefaults,
PointerGetDatum(proconfig),
procost,
- prorows);
+ prorows,
+ didReplace);
+
+ return didReplace;
}
diff --git a/src/backend/commands/proclang.c b/src/backend/commands/proclang.c
index 3860105..7a1520c 100644
--- a/src/backend/commands/proclang.c
+++ b/src/backend/commands/proclang.c
@@ -49,7 +49,7 @@ typedef struct
char *tmpllibrary; /* path of shared library */
} PLTemplate;
-static void create_proc_lang(const char *languageName, bool replace,
+static bool create_proc_lang(const char *languageName, bool replace,
Oid languageOwner, Oid handlerOid, Oid inlineOid,
Oid valOid, bool trusted);
static PLTemplate *find_language_template(const char *languageName);
@@ -59,9 +59,12 @@ static void AlterLanguageOwner_internal(HeapTuple tup, Relation rel,
/* -
* CREATE PROCEDURAL LANGUAGE
+ *
+ * Returns TRUE if language was replaced via CREATE OR REPLACE, FALSE
+ * if created.
* -
*/
-void
+bool
CreateProceduralLanguage(CreatePLangStmt *stmt)
{
char *languageName;
@@ -146,7 +149,8 @@ CreateProceduralLanguage(CreatePLangStmt *stmt)
NIL,
PointerGetDatum(NULL),
1,
- 0);
+ 0,
+
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
2011/1/13 KaiGai Kohei kai...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
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 alvhe...@commandprompt.com The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Fri, Jan 14, 2011 at 9:24 AM, Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Fri, Jan 14, 2011 at 12:07 PM, Tom Lane t...@sss.pgh.pa.us wrote: Alvaro Herrera alvhe...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Fri, Jan 14, 2011 at 1:35 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
Robert Haas robertmh...@gmail.com writes: On Fri, Jan 14, 2011 at 1:35 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Fri, Jan 14, 2011 at 2:07 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Fri, Jan 14, 2011 at 1:35 PM, Tom Lane t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
Thanks for reviewing! On Fri, Jan 14, 2011 at 13:40, Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Fri, Jan 14, 2011 at 3:45 PM, Marti Raudsepp ma...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
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 kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
Marti Raudsepp ma...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
On Sun, Nov 28, 2010 at 11:29 AM, Tom Lane t...@sss.pgh.pa.us wrote: Marti Raudsepp ma...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Return command tag 'REPLACE X' for CREATE OR REPLACE statements.
Robert Haas robertmh...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
