Re: [HACKERS] Re: Patch to add support of IF NOT EXISTS to others CREATE statements
Pavel Stehule escribió: I agree with Tom proposal - CINE - where object holds data, COR everywhere else. But it means, so all functionality from this patch have to be rewritten :( So we return this patch with feedback, right? I don't think it's reasonable to continue waiting this late. I have marked as such in the CF app. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training 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] Re: Patch to add support of IF NOT EXISTS to others CREATE statements
On Fri, Feb 28, 2014 at 5:05 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Pavel Stehule escribió: I agree with Tom proposal - CINE - where object holds data, COR everywhere else. But it means, so all functionality from this patch have to be rewritten :( So we return this patch with feedback, right? I don't think it's reasonable to continue waiting this late. I have marked as such in the CF app. Sorry guys... I'm very busy in this start of the year, so I have no time to dedicate to this patch. Maybe in the next commit fest I have time and do a rework on it. Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] Re: Patch to add support of IF NOT EXISTS to others CREATE statements
2014-01-19 Tom Lane t...@sss.pgh.pa.us: Stephen Frost sfr...@snowman.net writes: * Robert Haas (robertmh...@gmail.com) wrote: I kind of don't see the point of having IF NOT EXISTS for things that have OR REPLACE, and am generally in favor of implementing OR REPLACE rather than IF NOT EXISTS where possible. The point is usually to get the object to a known state, and OR REPLACE will generally accomplish that better than IF NOT EXISTS. However, if the object has complex structure (like a table that contains data) then replacing it is a bad plan, so IF NOT EXISTS is really the best you can do - and it's still useful, even if it does require more care. This patch is in the most recent commitfest and marked as Ready for Committer, so I started reviewing it and came across the above. I find myself mostly agreeing with the above comments from Robert, but it doesn't seem like we've really done a comprehensive review of the various commands to make a 'command' decision on each as to if it should have IF NOT EXISTS or OR REPLACE options. There's been pretty extensive theorizing about this in the past (try searching the pghackers archives for CINE and COR), and I think the rough consensus was that it's hard to do COR sensibly for objects containing persistent state (ie tables) or with separately-declarable substructure (again, mostly tables, though composite types have some of the same issues). However, if COR does make sense then CINE is an inferior alternative, because of the issue about not knowing the resulting state of the object for sure. Given this list I would absolutely reject CINE for aggregates (why in the world would we make them act differently from functions?), and likewise for casts, collations, operators, and types. I don't see any reason not to prefer COR for these object kinds. There is room for argument about the text search stuff, though, because of the fact that some of the text search object types have separately declarable substructure. The one difficulty that I do see with the 'OR REPLACE' option is when we can't simply replace an existing object due to dependencies on the existing definition of that object. Still, if that's the case, wouldn't you want an error? The main knock on COR is that there's no way for the system to completely protect itself from the possibility that you replaced the object definition with something that behaves incompatibly. For instance, if we had COR for collations and you redefined a collation, that might (or might not) break indexes whose ordering depends on that collation. However, we already bought into that type of risk when we invented COR for functions, and by and large there have been few complaints about it. The ability to substitute an improved version of a function seems to be worth the risks of substituting a broken version. regards, tom lane I agree with Tom proposal - CINE - where object holds data, COR everywhere else. But it means, so all functionality from this patch have to be rewritten :( Regards Pavel -- 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] Re: Patch to add support of IF NOT EXISTS to others CREATE statements
Stephen Frost sfr...@snowman.net writes: * Robert Haas (robertmh...@gmail.com) wrote: I kind of don't see the point of having IF NOT EXISTS for things that have OR REPLACE, and am generally in favor of implementing OR REPLACE rather than IF NOT EXISTS where possible. The point is usually to get the object to a known state, and OR REPLACE will generally accomplish that better than IF NOT EXISTS. However, if the object has complex structure (like a table that contains data) then replacing it is a bad plan, so IF NOT EXISTS is really the best you can do - and it's still useful, even if it does require more care. This patch is in the most recent commitfest and marked as Ready for Committer, so I started reviewing it and came across the above. I find myself mostly agreeing with the above comments from Robert, but it doesn't seem like we've really done a comprehensive review of the various commands to make a 'command' decision on each as to if it should have IF NOT EXISTS or OR REPLACE options. There's been pretty extensive theorizing about this in the past (try searching the pghackers archives for CINE and COR), and I think the rough consensus was that it's hard to do COR sensibly for objects containing persistent state (ie tables) or with separately-declarable substructure (again, mostly tables, though composite types have some of the same issues). However, if COR does make sense then CINE is an inferior alternative, because of the issue about not knowing the resulting state of the object for sure. Given this list I would absolutely reject CINE for aggregates (why in the world would we make them act differently from functions?), and likewise for casts, collations, operators, and types. I don't see any reason not to prefer COR for these object kinds. There is room for argument about the text search stuff, though, because of the fact that some of the text search object types have separately declarable substructure. The one difficulty that I do see with the 'OR REPLACE' option is when we can't simply replace an existing object due to dependencies on the existing definition of that object. Still, if that's the case, wouldn't you want an error? The main knock on COR is that there's no way for the system to completely protect itself from the possibility that you replaced the object definition with something that behaves incompatibly. For instance, if we had COR for collations and you redefined a collation, that might (or might not) break indexes whose ordering depends on that collation. However, we already bought into that type of risk when we invented COR for functions, and by and large there have been few complaints about it. The ability to substitute an improved version of a function seems to be worth the risks of substituting a broken version. 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
[HACKERS] Re: Patch to add support of IF NOT EXISTS to others CREATE statements
All,
* Robert Haas (robertmh...@gmail.com) wrote:
On Wed, Jun 12, 2013 at 3:00 PM, Peter Eisentraut pete...@gmx.net wrote:
On 6/12/13 1:29 PM, Fabrízio de Royes Mello wrote:
- CREATE AGGREGATE [ IF NOT EXISTS ] ...
- CREATE CAST [ IF NOT EXISTS ] ...
- CREATE COLLATION [ IF NOT EXISTS ] ...
- CREATE OPERATOR [ IF NOT EXISTS ] ...
- CREATE TEXT SEARCH {PARSER | DICTIONARY | TEMPLATE | CONFIGURATION} [
IF NOT EXISTS ] ...
- CREATE TYPE [ IF NOT EXISTS ] ... [AS [{ENUM | RANGE}] (...)]
I'm wondering where IF NOT EXISTS and OR REPLACE will meet.
I kind of don't see the point of having IF NOT EXISTS for things that
have OR REPLACE, and am generally in favor of implementing OR REPLACE
rather than IF NOT EXISTS where possible. The point is usually to get
the object to a known state, and OR REPLACE will generally accomplish
that better than IF NOT EXISTS. However, if the object has complex
structure (like a table that contains data) then replacing it is a
bad plan, so IF NOT EXISTS is really the best you can do - and it's
still useful, even if it does require more care.
This patch is in the most recent commitfest and marked as Ready for
Committer, so I started reviewing it and came across the above.
I find myself mostly agreeing with the above comments from Robert, but
it doesn't seem like we've really done a comprehensive review of the
various commands to make a 'command' decision on each as to if it should
have IF NOT EXISTS or OR REPLACE options.
The one difficulty that I do see with the 'OR REPLACE' option is when we
can't simply replace an existing object due to dependencies on the
existing definition of that object. Still, if that's the case, wouldn't
you want an error? The 'IF NOT EXISTS' case is the no-error option, but
then you might not know what kind of state the system is in.
Fabrízio, can you clarify the use-case for things like CREATE AGGREGATE
to have IF NOT EXISTS rather than OR REPLACE, or if there is a reason
why both should exist? Complicating our CREATE options is not something
we really wish to do without good reason and we certainly don't want to
add something now that we'll wish to remove in another version or two.
Thanks!
Stephen
signature.asc
Description: Digital signature
[HACKERS] Re: Patch to add support of IF NOT EXISTS to others CREATE statements
At 2013-07-26 10:39:00 +0200, karl...@gmail.com wrote:
Hello, as I can see there are more inconsistent places.
Right. This is what I was referring to in my original review. All of the
relevant sites (pre-patch) that currently do:
if (already exists)
ereport(ERROR …)
should instead be made to do:
if (already exists)
{
if (ifNotExists)
{
ereport(NOTICE …)
return
}
ereport(ERROR …)
}
or even (very slightly easier to review):
if (already exists ifNotExists)
{
ereport(ERROR …)
return
}
if (already exists)
ereport(ERROR …)
I don't care much which of the two is used, so long as it's (a) the same
everywhere, and (b) there's no if (!ifNot anywhere.
-- Abhijit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: Patch to add support of IF NOT EXISTS to others CREATE statements
At 2013-06-12 14:29:59 -0300, fabriziome...@gmail.com wrote:
The attached patch add support to IF NOT EXISTS to CREATE
statements listed below: […]
I noticed that this patch was listed as Needs Review with no
reviewers, so I had a quick look.
It applies with a couple of trailing whitespace warnings. Compiles OK.
Documentation changes look fine other than a couple of minor whitespace
errors (literal tabs in some continuation lines).
First, I looked at the changes in src/include: adding if_not_exists to
the relevant parse nodes, and adding ifNotExists parameters to various
functions (e.g. OperatorCreate). The changes look fine. There's a bit
more whitespace quirkiness, though (removed tabs).
Next, changes in src/backend, starting with parser changes: the patch
adds IF_P NOT EXISTS variants for various productions. For example:
src/backend/parser/gram.y:4605:
DefineStmt:
CREATE AGGREGATE func_name aggr_args definition
{
DefineStmt *n = makeNode(DefineStmt);
n-kind = OBJECT_AGGREGATE;
n-oldstyle = false;
n-defnames = $3;
n-args = $4;
n-definition = $5;
n-if_not_exists = false;
$$ = (Node *)n;
}
| CREATE AGGREGATE IF_P NOT EXISTS func_name aggr_args definition
{
DefineStmt *n = makeNode(DefineStmt);
n-kind = OBJECT_AGGREGATE;
n-oldstyle = false;
n-defnames = $6;
n-args = $7;
n-definition = $8;
n-if_not_exists = true;
$$ = (Node *)n;
}
Although there is plenty of precedent for this kind of doubling of rules
(e.g. CREATE SCHEMA, CREATE EXTENSION), it doesn't strike me as the best
idea. There's an opt_if_not_exists, and this patch uses it for CREATE
CAST (and it was already used for AlterEnumStmt).
I think opt_if_not_exists should be used for the others as well.
Moving on, the patch adds the if_not_exists field to the relevant
functions in {copyfuncs,equalfuncs}.c and adds stmt-if_not_exists
to the calls to DefineAggregate() etc. in tcop/utility.c. Fine.
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 64ca312..851c314 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
/*
@@ -1219,7 +1220,8 @@ heap_create_with_catalog(const char *relname,
-1, /* typmod */
0, /* array dimensions for
typBaseType */
false, /* Type NOT NULL */
-InvalidOid); /* rowtypes never have a
collation */
+InvalidOid, /* rowtypes never have a
collation */
+false);
Parameter needs a comment.
diff --git a/src/backend/catalog/pg_aggregate.c
b/src/backend/catalog/pg_aggregate.c
index e80b600..4452ba3 100644
--- a/src/backend/catalog/pg_aggregate.c
+++ b/src/backend/catalog/pg_aggregate.c
@@ -228,7 +229,7 @@ AggregateCreate(const char *aggName,
procOid = ProcedureCreate(aggName,
aggNamespace,
- false,/* no
replacement */
+ false,/*
replacement */
false,/*
doesn't return a set */
finaltype,
/* returnType */
GetUserId(),
/* proowner */
What's up with this? We're calling ProcedureCreate with replace==false,
so no replacement sounds correct to me.
diff --git a/src/backend/catalog/pg_operator.c
b/src/backend/catalog/pg_operator.c
index 3c4fedb..7466e76 100644
--- a/src/backend/catalog/pg_operator.c
+++ b/src/backend/catalog/pg_operator.c
@@ -336,7 +336,8 @@ OperatorCreate(const char *operatorName,
Oid restrictionId,
Oid joinId,
bool canMerge,
-bool canHash)
+bool canHash,
+bool if_not_exists)
{
Relationpg_operator_desc;
HeapTuple tup;
This should be ifNotExists (to match the header file, and all your
other changes).
@@ -416,11 +417,18 @@ OperatorCreate(const char *operatorName,
rightTypeId,
operatorAlreadyDefined);
- if (operatorAlreadyDefined)
+
