Re: [HACKERS] Re: Patch to add support of IF NOT EXISTS to others CREATE statements

2014-02-28 Thread Alvaro Herrera
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

2014-02-28 Thread Fabrízio de Royes Mello
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-30 Thread Pavel Stehule
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

2014-01-19 Thread Tom Lane
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

2014-01-18 Thread Stephen Frost
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

2013-07-26 Thread Abhijit Menon-Sen
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

2013-07-13 Thread Abhijit Menon-Sen
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)
 +