Обсуждение: Patch to add support of "IF NOT EXISTS" to others "CREATE" statements

On 6/12/13 1:29 PM, Fabrízio de Royes Mello wrote:
> The attached patch add support to "IF NOT EXISTS" to "CREATE" statements
> listed below:
> 
> - 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.

For example, why doesn't your list include CREATE FUNCTION?

I have on my personal todo list to add "OR REPLACE" support to CREATE
AGGREGATE and CREATE OPERATOR.  They are kind of like functions, after
all, and CREATE OR REPLACE FUNCTION is clearly widely useful.

I suppose both could be useful, but if we're going to make sweeping
changes, perhaps that should be clarified.

Btw., I also want REPLACE BUT DO NOT CREATE.

<div dir="ltr">On 13 June 2013 04:30, Peter Eisentraut <span dir="ltr"><<a href="mailto:peter_e@gmx.net"
target="_blank">peter_e@gmx.net</a>></span>wrote:<br /><div class="gmail_extra"><div class="gmail_quote"><blockquote
class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im"><span
style="color:rgb(34,34,34)">I'mwondering where "IF NOT EXISTS" and "OR REPLACE" will meet.</span><br
/></div></blockquote></div><br/></div><div class="gmail_extra">CREATE OR REPLACE (or ALTER / UPDATE ?) would definitely
beuseful for enums, where it would be nice if we could teach an ORM to generate DDL based on the current values of the
enumin code, and know that after the operation had completed, the database enum type matched the code enum type. I
don'tthink a sequence of ALTER TYPE ADD VALUE IF NOT EXISTS quite does the trick, as it doesn't guarantee that the db
enumis in the same order as the code enum, which may or may not be important. I'd expect a CREATE OR ALTER for enums to
raisean error if any of the elements were out of order.</div><div class="gmail_extra"><br /></div><div
class="gmail_extra">Currentlyto get to a known state for enums you have to write manual migration scripts, and while
thattends to be how I roll anyway, often when starting projects in rails / grails / hibernate etc people rely on db
schemasgenerated by the framework as it lets them prototype with less mucking around. It would be nice for those
frameworksto be able to generate enum types in a known state.</div><div class="gmail_extra"><br /></div><div
class="gmail_extra">Cheers</div><divclass="gmail_extra"><br /></div><div class="gmail_extra">Tom</div></div> 

On Wed, 2013-06-12 at 16:31 -0300, Fabrízio de Royes Mello wrote:
> > Btw., I also want REPLACE BUT DO NOT CREATE.
> 
> Can you explain more about it?
> 
Replace/alter the object if it already exists, but fail if it does not
exist.

The complete set of variants is:

- object does not exist:
   - proceed (normal CREATE)   - error (my above description)

- object exists:
   - replace (CREATE OR REPLACE)   - skip (CREATE IF NOT EXISTS)   - error (normal CREATE)

<div dir="ltr">On Mon, Jun 17, 2013 at 11:33 PM, Peter Eisentraut <<a
href="mailto:peter_e@gmx.net">peter_e@gmx.net</a>>wrote:<br />><br />> Replace/alter the object if it already
exists,but fail if it does not<br /> > exist.<br />><br />> The complete set of variants is:<br />><br
/>>- object does not exist:<br />><br />>     - proceed (normal CREATE)<br />>     - error (my above
description)<br/>><br />> - object exists:<br /> ><br />>     - replace (CREATE OR REPLACE)<br />>     -
skip(CREATE IF NOT EXISTS)<br />>     - error (normal CREATE)<br />><br /><br />I understood. <br /><br />The
syntaxcan be like that?<br />- CREATE [ OR REPLACE | IF NOT EXISTS ] AGGREGATE ...<br /> - CREATE [ OR REPLACE | IF NOT
EXISTS] OPERATOR ...<br />- CREATE [ OR REPLACE | IF NOT EXISTS ] FUNCTION ...<br /><br />I can add this features too,
butIMHO it is more prudent at this CF we just implement the IF NOT EXISTS according the initial proposal.<br /><br
/>I'mplanning another patch do next CF to add support to "IF NOT EXISTS" to others "CREATE" statements. See my planning
[1].<br/><br />Regards,<br /><br />[1] <a
href="https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing">https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing</a><br
/><br/>--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/>>> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/> >> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/></div> 

On Wed, Jun 19, 2013 at 12:45 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> On Mon, Jun 17, 2013 at 11:33 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
>>
>> Replace/alter the object if it already exists, but fail if it does not
>> exist.
>>
>> The complete set of variants is:
>>
>> - object does not exist:
>>
>>     - proceed (normal CREATE)
>>     - error (my above description)
>>
>> - object exists:
>>
>>     - replace (CREATE OR REPLACE)
>>     - skip (CREATE IF NOT EXISTS)
>>     - error (normal CREATE)
>>
>
> I understood.
>
> The syntax can be like that?
> - CREATE [ OR REPLACE | IF NOT EXISTS ] AGGREGATE ...
> - CREATE [ OR REPLACE | IF NOT EXISTS ] OPERATOR ...
> - CREATE [ OR REPLACE | IF NOT EXISTS ] FUNCTION ...
>
> I can add this features too, but IMHO it is more prudent at this CF we just
> implement the IF NOT EXISTS according the initial proposal.
>
> I'm planning another patch do next CF to add support to "IF NOT EXISTS" to
> others "CREATE" statements. See my planning [1].
>

Is it possible to:

CREATE [ OR REPLACE | IF NOT EXISTS ] OPERATOR CLASS

I am in a situation where I need to conditionally create an operator
class (that is, create only if already does not exist).

For example, currently, while trying out pg_trgm and a new external
module pg_bigm, I found that, currently, only  one of them can be
installed in a database at a time. pg_bigm for backward compatibility
also creates pg_trgm_ops operator class with its member functions
being the ones implemented by pg_bigm. So, if pg_trgm already exists,
then I won't be able to add pg_bigm (which has its own use cases and
we can probably have the two co-exist) and vice versa. It would be
nice if we had the above feature so that pg_bigm or pg_trgm can use
'IF NOT EXISTS' while creating pg_trgm_ops operator class.

Thoughts?


--
Amit Langote

<div dir="ltr"><br />On Thu, Jun 20, 2013 at 1:52 AM, Amit Langote <<a
href="mailto:amitlangote09@gmail.com">amitlangote09@gmail.com</a>>wrote:<br />><br />> Is it possible to:<br
/>><br/>> CREATE [ OR REPLACE | IF NOT EXISTS ] OPERATOR CLASS<br /> ><br />> I am in a situation where I
needto conditionally create an operator<br />> class (that is, create only if already does not exist).<br />><br
/>>[...]<br />><br /><br />The intention is cover all "CREATE OPERATOR" variants. See my planning [1].<br /><br
/>Regards,<br/><br />[1] <a
href="https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing">https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing</a><br
/><br/>--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/>>> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/> >> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/></div> 

On Thu, Jun 20, 2013 at 9:48 PM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
>
> On Thu, Jun 20, 2013 at 1:52 AM, Amit Langote <amitlangote09@gmail.com>
> wrote:
>>
>> Is it possible to:
>>
>> CREATE [ OR REPLACE | IF NOT EXISTS ] OPERATOR CLASS
>>
>> I am in a situation where I need to conditionally create an operator
>> class (that is, create only if already does not exist).
>>
>> [...]
>>
>
> The intention is cover all "CREATE OPERATOR" variants. See my planning [1].
>
>
> Regards,
>
> [1]
> https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing

Hmm, okay. Last time I checked, the CREATE OPERATOR CLASS row was
empty, so asked.


--
Amit Langote

On Wed, Jun 12, 2013 at 3:00 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 6/12/13 1:29 PM, Fabrízio de Royes Mello wrote:
>> The attached patch add support to "IF NOT EXISTS" to "CREATE" statements
>> listed below:
>>
>> - 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.

> Btw., I also want REPLACE BUT DO NOT CREATE.

That's a mouthful.  What's it good for?

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

On 6/20/13 11:04 AM, Robert Haas 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.

I tend to agree.

>> > Btw., I also want REPLACE BUT DO NOT CREATE.
> That's a mouthful.  What's it good for?

If you run an upgrade SQL script that is supposed to replace, say, a
bunch of functions with new versions, you'd want the behavior that it
replaces the existing function if it exists, but errors out if it
doesn't, because then you're perhaps connected to the wrong database.

It's a marginal feature, and I'm not going to pursue it, but if someone
wanted to make the CREATE commands fully featured, there is use for this.

On 2013-06-12 14:29:59 -0300, Fabrízio de Royes Mello wrote:
> On Fri, May 24, 2013 at 12:22 PM, Fabrízio de Royes Mello <
> fabriziomello@gmail.com> wrote:
>
> > Hi all,
> >
> > I working in a patch to include support of "IF NOT EXISTS" into "CREATE"
> > statements that not have it yet.
> >
> > I started with "DefineStmt" section from "src/backend/parser/gram.y":
> > - CREATE AGGREGATE [ IF NOT EXISTS ] ...
> > - CREATE OPERATOR [ IF NOT EXISTS ] ...
> > - CREATE TYPE [ IF NOT EXISTS ] ... [AS [{ENUM | RANGE}] (...)]
> > - CREATE TEXT SEARCH {PARSER | DITIONARY | TEMPLATE | CONFIGURATION} [ IF
> > NOT EXISTS ] ...
> > - CREATE COLLATION [ IF NOT EXISTS ] ...
> >
> >
> The attached patch add support to "IF NOT EXISTS" to "CREATE" statements
> listed below:
>
> - 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'd argue if we go that way - which seems to be a good idea - we really
ought to make a complete pass and add it to all commands where it's
currently missing.

* CREATE DOMAIN
* CREATE GROUP
* CREATE TABLE AS
* CREATE MATERIALIZED VIEW
* CREATE SEQUENCE (we have ALTER but not CREATE?)
* CREATE TABLESPACE (arguably slightly harder)
* CREATE FOREIGN DATA WRAPPER
* CREATE SERVER
* CREATE DATABASE
* CREATE USER MAPPING
* CREATE TRIGGER
* CREATE EVENT TRIGGER
* CREATE INDEX
* CLUSTER

Cases that seem useful, even though we have OR REPLACE:
* CREATE VIEW
* CREATE FUNCTION

Of dubious use:
* CREATE OPERATOR CLASS
* CREATE OPERATOR FAMILY
* CREATE RULE
* CREATE CONVERSION

Greetings,

Andres Freund

-- Andres Freund                       http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training &
Services

At 2013-06-12 14:29:59 -0300, fabriziomello@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)
>  {
>      Relation    pg_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)
> +    if (operatorAlreadyDefined && !if_not_exists)
>          ereport(ERROR,
>                  (errcode(ERRCODE_DUPLICATE_FUNCTION),
>                   errmsg("operator %s already exists",
>                          operatorName)));
> +    if (operatorAlreadyDefined && if_not_exists) {
> +        ereport(NOTICE,
> +                (errcode(ERRCODE_DUPLICATE_FUNCTION),
> +                 errmsg("operator %s already exists, skipping",
> +                        operatorName)));
> +        return InvalidOid;
> +    }

Everywhere else, you're doing something like this:
   if (exists)   {       if (!if_not_exists)           ERROR       else           NOTICE   }

So you should do the same thing here. Failing that, at least reorder the
blocks so that you don't test both !if_not_exists and if_not_exists:
   if (operatorAlreadyDefined && if_not_exists)   {       NOTICE;       return InvalidOid;   }
   if (operatorAlreadyDefined)       ERROR

(But first see below.)

> diff --git a/src/backend/catalog/pg_type.c b/src/backend/catalog/pg_type.c
> index 23ac3dd..3d55360 100644
> --- a/src/backend/catalog/pg_type.c
> +++ b/src/backend/catalog/pg_type.c
> @@ -397,9 +398,20 @@ TypeCreate(Oid newTypeOid,
>           * shell type, however.
>           */
>          if (((Form_pg_type) GETSTRUCT(tup))->typisdefined)
> -            ereport(ERROR,
> -                    (errcode(ERRCODE_DUPLICATE_OBJECT),
> -                     errmsg("type \"%s\" already exists", typeName)));
> +        {
> +            if (!ifNotExists)
> +                ereport(ERROR,
> +                        (errcode(ERRCODE_DUPLICATE_OBJECT),
> +                         errmsg("type \"%s\" already exists", typeName)));
> +            else
> +            {
> +                ereport(NOTICE,
> +                        (errcode(ERRCODE_DUPLICATE_OBJECT),
> +                         errmsg("type \"%s\" already exists, skipping", typeName)));
> +                heap_close(pg_type_desc, RowExclusiveLock);
> +                return InvalidOid;
> +            }
> +        }

I'd strongly prefer to see this written everywhere as follows:
   if (already_exists)   {       if (ifNotExists)       {           ereport(NOTICE, …);
heap_close(pg_type_desc,RowExclusiveLock);           return InvalidOid;       }       ereport(ERROR, …);   }
 

The error can be in an else {} or not, I have only a weak preference in
that matter. But the "if (!ifNotExists)" is pretty awkward. Ultimately,
the patch is adding special handling for a new flag, so it makes sense
to test if the flag is set and behave specially, rather than testing if
it's not set to do what was done before.

(Actually, I like the way AlterEnumStmt calls this "skipIfExists". That
reads much more nicely. But there are enough "if_not_exists" elsewhere
in the code that I won't argue to change them in this patch, at least.)

Sorry to nitpick, but I feel quite strongly about this.

> diff --git a/src/backend/commands/aggregatecmds.c b/src/backend/commands/aggregatecmds.c
> index 4a03786..9f128bd 100644
> --- a/src/backend/commands/aggregatecmds.c
> +++ b/src/backend/commands/aggregatecmds.c
> @@ -224,6 +224,7 @@ DefineAggregate(List *name, List *args, bool oldstyle, List *parameters)
>                             transfuncName,        /* step function name */
>                             finalfuncName,        /* final function name */
>                             sortoperatorName,    /* sort operator name */
> -                           transTypeId, /* transition data type */
> -                           initval);    /* initial condition */
> +                           transTypeId,    /* transition data type */
> +                           initval,        /* initial condition */
> +                           ifNotExists);    /* if not exists flag */

You should settle on "if not exists" or "if not exists flag" for your
comments. I suggest the former (i.e. no "flag").

I don't see any other problems with the patch. It passes "make check".

I'm marking this as "Waiting on Author".

-- Abhijit

On Sun, Jul 14, 2013 at 03:36:09AM -0300, Fabrízio de Royes Mello wrote:
> > Next, changes in src/backend, starting with parser changes: the patch
> > adds "IF_P NOT EXISTS" variants for various productions. For example:

<snip>

> > I think opt_if_not_exists should be used for the others as well.
> >
>
> I could not use the "opt_if_not_exists" because bison emits an error:
>
> /usr/bin/bison -d -o gram.c gram.y
> gram.y: conflicts: 10 shift/reduce
> gram.y: expected 0 shift/reduce conflicts
> make[3]: *** [gram.c] Error 1
>
> I really don't know how to solve this problem. I'm just do ajustments like
> that:

This probably isn't solvable, which is why the coding is double in many
existing places. The issue is that by using opt_if_not_exists you make
that bison has to decide much earlier which rule it is parsing. Bison
only has one token lookahead and if that's not enough you get errors.

BTW, bison dumps a large file describing all its states that you should
be able to work out from that where the exact problem lies.

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> He who writes carelessly confesses thereby at the very outset that he does
> not attach much importance to his own thoughts.  -- Arthur Schopenhauer

Hello,
patch works fine but is there any reason to comparing each ifNotExists
in different way?
i.e.
ProcedureCreate
if (!ifNotExists)
...
else
{
...
return
}
----
TypeCreate
if (ifNotExists)
{
...
return
}
...
---
Shouldn't it be more consistent?
Regards,
Karol

On 24-07-2013 14:49, Karol Trzcionka wrote:
> Hello,
> patch works fine but is there any reason to comparing each ifNotExists
> in different way?
> i.e.
> ProcedureCreate
> if (!ifNotExists)
> ...
> else
> {
> ...
> return
> }
> ----
> TypeCreate
> if (ifNotExists)
> {
> ...
> return
> }
> ...
> ---
> Shouldn't it be more consistent?

Should be... I fix that in attached patch.

Regards,


--
    Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
    PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

W dniu 26.07.2013 02:44, Fabrízio de Royes Mello pisze:
> Should be... I fix that in attached patch.
Hello, as I can see there are more inconsistent places.
First style:
OperatorCreate
---
Second style:
ProcedureCreate
TypeCreate
DefineTSParser
DefineType
DefineEnum
---
Third style:
CreateCast
DefineTSDictionary
DefineTSTemplate
DefineTSConfiguration
DefineRange
DefineCompositeType
Regards,
Karol

On 26-07-2013 05:39, Karol Trzcionka wrote:
> W dniu 26.07.2013 02:44, Fabrízio de Royes Mello pisze:
>> Should be... I fix that in attached patch.
> Hello, as I can see there are more inconsistent places.
> First style:
> OperatorCreate
> ---

This style is already right :

if ( ifNotExists )
{
   skip
   return
}
error

> Second style:
> ProcedureCreate
> TypeCreate
> DefineTSParser
> DefineType
> DefineEnum
> ---
> Third style:
> CreateCast
> DefineTSDictionary
> DefineTSTemplate
> DefineTSConfiguration
> DefineRange
> DefineCompositeType
>

Fixed... thanks.

Regards,

--
    Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
    PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

At 2013-07-26 10:39:00 +0200, karlikt@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

At 2013-07-26 18:55:21 -0300, fabrizio@timbira.com.br wrote:
>
> Fixed... thanks.

Ah, sorry. I didn't see this patch immediately because you dropped me
from the Cc: list.

You missed a couple of places. Updated patch attached.

This is now ready for committer, I reckon.

-- Abhijit

On 26-07-2013 23:57, Abhijit Menon-Sen wrote:
> At 2013-07-26 18:55:21 -0300, fabrizio@timbira.com.br wrote:
>>
>> Fixed... thanks.
>
> Ah, sorry. I didn't see this patch immediately because you dropped me
> from the Cc: list.
>

I'm sorry... unfortunately this patch was moved do CF2 [1]

> You missed a couple of places. Updated patch attached.
>

You all right... I just fix some spaces and comments on code.


> This is now ready for committer, I reckon.
>

Can you send a comment to this patch on commitfest [1] ?

Regards,

[1] https://commitfest.postgresql.org/action/patch_view?id=1133

--
    Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
    PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

On 26-07-2013 23:31, Abhijit Menon-Sen wrote:
> At 2013-07-26 10:39:00 +0200, karlikt@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:
>
> [...]
>

Hi all,

I'm sending, from the PGBR2013 (The Brazilian PostgreSQL Conference)
[1], the second part of this patch to cover another CREATE statements:

- CREATE SEQUENCE [ IF NOT EXISTS ]
- CREATE DOMAIN [ IF NOT EXISTS ]
- CREATE EVENT TRIGGER [ IF NOT EXISTS ]

Soon I'll sent the third and final part to finish this patch.


Regards,

[1] http://pgbr.postgresql.org.br/2013/noticias.php

--
    Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
    PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

On 17-08-2013 18:10, Fabrízio de Royes Mello wrote:
> On 26-07-2013 23:31, Abhijit Menon-Sen wrote:
>> At 2013-07-26 10:39:00 +0200, karlikt@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:
>>
>> [...]
>>
>
> Hi all,
>
> I'm sending, from the PGBR2013 (The Brazilian PostgreSQL Conference)
> [1], the second part of this patch to cover another CREATE statements:
>
> - CREATE SEQUENCE [ IF NOT EXISTS ]
> - CREATE DOMAIN [ IF NOT EXISTS ]
> - CREATE EVENT TRIGGER [ IF NOT EXISTS ]
>
> Soon I'll sent the third and final part to finish this patch.
>

Attached some doc fixes.

Regards,

--
    Fabrízio de Royes Mello         Timbira - http://www.timbira.com.br/
    PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

All,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Wed, Jun 12, 2013 at 3:00 PM, Peter Eisentraut <peter_e@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

Stephen Frost <sfrost@snowman.net> writes:
> * Robert Haas (robertmhaas@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

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 Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

<div dir="ltr"><div class="gmail_extra"><br />On Sat, Jan 18, 2014 at 11:12 PM, Stephen Frost <<a
href="mailto:sfrost@snowman.net">sfrost@snowman.net</a>>wrote:<br />><br />> Fabrízio, can you clarify the
use-casefor things like CREATE AGGREGATE<br /> > to have IF NOT EXISTS rather than OR REPLACE, or if there is a
reason<br/>> why both should exist?  Complicating our CREATE options is not something<br />> we really wish to do
withoutgood reason and we certainly don't want to<br /> > add something now that we'll wish to remove in another
versionor two.<br />><br /><br /></div><div class="gmail_extra">Hi Stephen,<br /><br />First I'm really sorry about
thelong time without an answer. I'm very busy in this start of the year.<br /><br /></div><div class="gmail_extra">Well
Ihave a scenario with many servers to deploy DDL scripts, and most of them we must run without transaction control
becausesome tasks like CREATE INDEX CONCURRENTLY, DROP/CREATE DATABASE, CLUSTER, etc.<br /><br />When an error occurs
thescript stops, but the previous commands was commited, then we must review the script to comment parts that was
alreadyexecuted and then run it again. Until now is not a really trouble, but in some cases we must deploy another DDL
scriptthat contains a new version of some object before we finish to fix the previous version that was in production,
andif we have CINE for all CREATE objects this task will more easy because we just run it again without care if will
replacethe content and do not produce an error.<br /><br />I know that is a very specific case, but in my mind I don't
seeany problem to have CINE and COR to this objects. The behavior is totally different.<br /></div><div
class="gmail_extra"><br/></div><div class="gmail_extra"> Regards,<br /></div><div class="gmail_extra"><br />--<br
/>Fabríziode Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/> >> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

Fabrízio de Royes Mello <fabriziomello@gmail.com> writes:
> On Sat, Jan 18, 2014 at 11:12 PM, Stephen Frost <sfrost@snowman.net> wrote:
>> 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.

> Well I have a scenario with many servers to deploy DDL scripts, and most of
> them we must run without transaction control because some tasks like CREATE
> INDEX CONCURRENTLY, DROP/CREATE DATABASE, CLUSTER, etc.

> When an error occurs the script stops, but the previous commands was
> commited, then we must review the script to comment parts that was already
> executed and then run it again. Until now is not a really trouble, but in
> some cases we must deploy another DDL script that contains a new version of
> some object before we finish to fix the previous version that was in
> production, and if we have CINE for all CREATE objects this task will more
> easy because we just run it again without care if will replace the content
> and do not produce an error.

Why wouldn't COR semantics answer that requirement just as well, if not
better?
        regards, tom lane

<div dir="ltr"><div class="gmail_extra"><br />On Sat, Mar 1, 2014 at 2:11 PM, Tom Lane <<a
href="mailto:tgl@sss.pgh.pa.us">tgl@sss.pgh.pa.us</a>>wrote:<br />><br />> Fabrízio de Royes Mello <<a
href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>>writes:<br /> > > On Sat, Jan 18, 2014 at
11:12PM, Stephen Frost <<a href="mailto:sfrost@snowman.net">sfrost@snowman.net</a>> wrote:<br />> >>
Fabrízio,can you clarify the use-case for things like CREATE AGGREGATE<br /> > >> to have IF NOT EXISTS rather
thanOR REPLACE, or if there is a reason<br />> >> why both should exist?  Complicating our CREATE options is
notsomething<br />> >> we really wish to do without good reason and we certainly don't want to<br /> >
>>add something now that we'll wish to remove in another version or two.<br />><br />> > Well I have a
scenariowith many servers to deploy DDL scripts, and most of<br />> > them we must run without transaction
controlbecause some tasks like CREATE<br /> > > INDEX CONCURRENTLY, DROP/CREATE DATABASE, CLUSTER, etc.<br
/>><br/>> > When an error occurs the script stops, but the previous commands was<br />> > commited, then
wemust review the script to comment parts that was already<br /> > > executed and then run it again. Until now is
nota really trouble, but in<br />> > some cases we must deploy another DDL script that contains a new version
of<br/>> > some object before we finish to fix the previous version that was in<br /> > > production, and
ifwe have CINE for all CREATE objects this task will more<br />> > easy because we just run it again without care
ifwill replace the content<br />> > and do not produce an error.<br />><br /> > Why wouldn't COR semantics
answerthat requirement just as well, if not<br />> better?<br />><br /><br /></div><div class="gmail_extra">Just
becauseit will replace the object content... and in some cases this cannot happen because it will regress the schema to
anold version.<br /><br />I know it's a very specific use case, but in a scenario with many servers and many automated
tasksin different pipelines, CINE will be very useful. I have this kind of troubles mostly with functions (we use COR),
andsometimes we will discover that the production version of function is wrong after we receive a user notify, and in
thissituation many times we spend a lot of effort do fix the whole damage.<br /></div><div class="gmail_extra"><br
/></div><divclass="gmail_extra">Grettings,<br /><br /></div><div class="gmail_extra">--<br />Fabrízio de Royes Mello<br
/>Consultoria/CoachingPostgreSQ<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/> >> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/>>> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/> >> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

Fabrízio de Royes Mello <fabriziomello@gmail.com> writes:
> On Sat, Mar 1, 2014 at 2:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> [ re schema upgrade scenarios ]
>> Why wouldn't COR semantics answer that requirement just as well, if not
>> better?

> Just because it will replace the object content... and in some cases this
> cannot happen because it will regress the schema to an old version.

That argument seems awfully darn flimsy.  On what grounds would you argue
that the script you're sourcing contains versions you want of objects that
aren't there, but not versions you want of objects that are there?  If
the script is out of date, it seems more likely that you'd end up with
back-rev versions of the newly created objects, which very possibly won't
interact well with the newer objects that were already in the database.

In any case, given the existence of DO it's simple to code up
create-if-not-exists behavior with a couple lines of plpgsql; that seems
to me to be a sufficient answer for corner cases.  create-or-replace is
not equivalently fakable if the system doesn't supply the functionality.
        regards, tom lane

* Fabrízio de Royes Mello (fabriziomello@gmail.com) wrote:
> - CREATE SEQUENCE [ IF NOT EXISTS ]
> - CREATE DOMAIN [ IF NOT EXISTS ]
> - CREATE EVENT TRIGGER [ IF NOT EXISTS ]
> - CREATE ROLE [ IF NOT EXISTS ]
>
> Seems that no one reviewed this part or was rejected with others?

Why don't those fall into the same concern, specifically that what we
really want is 'CREATE-OR-REPLACE' semantics for them instead?
Thanks,
    Stephen

<div dir="ltr"><div class="gmail_extra">On Mon, Mar 31, 2014 at 4:52 PM, Stephen Frost <<a
href="mailto:sfrost@snowman.net">sfrost@snowman.net</a>>wrote:<br />><br />> * Fabrízio de Royes Mello (<a
href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>)wrote:<br /> > > - CREATE SEQUENCE [ IF NOT
EXISTS]<br />> > - CREATE DOMAIN [ IF NOT EXISTS ]<br />> > - CREATE EVENT TRIGGER [ IF NOT EXISTS ]<br
/>>> - CREATE ROLE [ IF NOT EXISTS ]<br />> ><br />> > Seems that no one reviewed this part or was
rejectedwith others?<br /> ><br />> Why don't those fall into the same concern, specifically that what we<br
/>>really want is 'CREATE-OR-REPLACE' semantics for them instead?<br />><br /><br /></div><div
class="gmail_extra">Ok,but I think this semantics is desirable just to "CREATE DOMAIN" and "CREATE EVENT TRIGGER". <br
/><br/>Isn't desirable add CINE to SEQUENCEs and ROLEs?<br /><br /></div><div class="gmail_extra">Regards,<br
/></div><divclass="gmail_extra"><br />--<br />Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br
/>>>Timbira: <a href="http://www.timbira.com.br">http://www.timbira.com.br</a><br /> >> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/>>> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/> >> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

* Fabrízio de Royes Mello (fabriziomello@gmail.com) wrote:
> On Mon, Mar 31, 2014 at 4:52 PM, Stephen Frost <sfrost@snowman.net> wrote:
> >
> > * Fabrízio de Royes Mello (fabriziomello@gmail.com) wrote:
> > > - CREATE SEQUENCE [ IF NOT EXISTS ]
> > > - CREATE DOMAIN [ IF NOT EXISTS ]
> > > - CREATE EVENT TRIGGER [ IF NOT EXISTS ]
> > > - CREATE ROLE [ IF NOT EXISTS ]
> > >
> > > Seems that no one reviewed this part or was rejected with others?
> >
> > Why don't those fall into the same concern, specifically that what we
> > really want is 'CREATE-OR-REPLACE' semantics for them instead?
> >
>
> Ok, but I think this semantics is desirable just to "CREATE DOMAIN" and
> "CREATE EVENT TRIGGER".
>
> Isn't desirable add CINE to SEQUENCEs and ROLEs?

Why would it be difficult to have COR for sequences..?  Or roles?
Thanks,
    Stephen

<div dir="ltr"><div class="gmail_extra"><br />On Mon, Mar 31, 2014 at 5:00 PM, Stephen Frost <<a
href="mailto:sfrost@snowman.net">sfrost@snowman.net</a>>wrote:<br />><br />> * Fabrízio de Royes Mello (<a
href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>)wrote:<br /> > > On Mon, Mar 31, 2014 at 4:52
PM,Stephen Frost <<a href="mailto:sfrost@snowman.net">sfrost@snowman.net</a>> wrote:<br />> > ><br
/>>> > * Fabrízio de Royes Mello (<a href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>)
wrote:<br/> > > > > - CREATE SEQUENCE [ IF NOT EXISTS ]<br />> > > > - CREATE DOMAIN [ IF NOT
EXISTS]<br />> > > > - CREATE EVENT TRIGGER [ IF NOT EXISTS ]<br />> > > > - CREATE ROLE [ IF
NOTEXISTS ]<br /> > > > ><br />> > > > Seems that no one reviewed this part or was rejected
withothers?<br />> > ><br />> > > Why don't those fall into the same concern, specifically that what
we<br/> > > > really want is 'CREATE-OR-REPLACE' semantics for them instead?<br />> > ><br />>
><br/>> > Ok, but I think this semantics is desirable just to "CREATE DOMAIN" and<br />> > "CREATE EVENT
TRIGGER".<br/> > ><br />> > Isn't desirable add CINE to SEQUENCEs and ROLEs?<br />><br />> Why would
itbe difficult to have COR for sequences..?  Or roles?<br />><br /><br />Because they maintain user data?<br /><br
/>--<br/>Fabrízio de Royes Mello<br /> Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/> >> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

* Fabrízio de Royes Mello (fabriziomello@gmail.com) wrote:
> Because they maintain user data?

Eh?  You mean like the sequence #?  Yes, I'd expect 'CREATE OR REPLACE
SEQUENCE' to want a minvalue or something on a 'replace' case to ensure
that it doesn't roll backwards unless explicitly asked for.  Perhaps
the same for any non-default parameters as well, though I'd look at the
other COR cases to see what they do.

CREATE OR REPLACE ROLE is actually easier, no?  All you'd be updating
are the various role attributes, I'd think, since only those are
available at CREATE time today.  Any role memberships or ownership
would be left alone.
Thanks,
    Stephen

<div dir="ltr"><div class="gmail_extra">On Mon, Mar 31, 2014 at 5:46 PM, Stephen Frost <<a
href="mailto:sfrost@snowman.net">sfrost@snowman.net</a>>wrote:<br />><br />> * Fabrízio de Royes Mello (<a
href="mailto:fabriziomello@gmail.com">fabriziomello@gmail.com</a>)wrote:<br /> > > Because they maintain user
data?<br/>><br />> Eh?  You mean like the sequence #?  Yes, I'd expect 'CREATE OR REPLACE<br />> SEQUENCE' to
wanta minvalue or something on a 'replace' case to ensure<br /> > that it doesn't roll backwards unless explicitly
askedfor.  Perhaps<br />> the same for any non-default parameters as well, though I'd look at the<br />> other
CORcases to see what they do.<br />><br /><br /></div><div class="gmail_extra">You mean if we execute 'CREATE OR
REPLACE'must we verify the default values of this statement and compare with the existing ones?<br /></div><div
class="gmail_extra"><br/><br />> CREATE OR REPLACE ROLE is actually easier, no?  All you'd be updating<br /> >
arethe various role attributes, I'd think, since only those are<br />> available at CREATE time today.  Any role
membershipsor ownership<br />> would be left alone.<br />><br /><br /></div><div class="gmail_extra"> Think about
thestatements below:<br /></div><div class="gmail_extra"><br /></div><div class="gmail_extra">CREATE ROLE test
NOLOGIN;<br/></div><div class="gmail_extra">CREATE OR REPLACE ROLE test;<br /></div><div class="gmail_extra"><br
/></div><divclass="gmail_extra">If we execute the statements above the result should be the role 'test' can login.
Correct?<br/></div><div class="gmail_extra"><br /></div><div class="gmail_extra">Regards,<br /><br /></div><div
class="gmail_extra">--<br/>Fabrízio de Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog sobre TI: <a
href="http://fabriziomello.blogspot.com">http://fabriziomello.blogspot.com</a><br/> >> Perfil Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a></div></div>

On Tue, Apr 1, 2014 at 7:28 AM, Fabrízio de Royes Mello
<fabriziomello@gmail.com> wrote:
> Think about the statements below:
>
> CREATE ROLE test NOLOGIN;
> CREATE OR REPLACE ROLE test;
>
> If we execute the statements above the result should be the role 'test' can
> login. Correct?
Except if I am missing something, the second query means that it is
going to replace the existing user test with a new one, with the
settings specified in the 2nd query, all being default values. As the
default for login is NOLOGIN, the user test should not be able to log
in the server.
--
Michael

* Michael Paquier (michael.paquier@gmail.com) wrote:
> On Tue, Apr 1, 2014 at 7:28 AM, Fabrízio de Royes Mello
> <fabriziomello@gmail.com> wrote:
> > Think about the statements below:
> >
> > CREATE ROLE test NOLOGIN;
> > CREATE OR REPLACE ROLE test;
> >
> > If we execute the statements above the result should be the role 'test' can
> > login. Correct?

> Except if I am missing something, the second query means that it is
> going to replace the existing user test with a new one, with the
> settings specified in the 2nd query, all being default values. As the
> default for login is NOLOGIN, the user test should not be able to log
> in the server.

That's more-or-less the behavior we're trying to work out.  I've been
meaning to go back and look at what we've been doing with the existing
COR cases and just haven't gotten to it yet.  The pertinent question
being if we assume the user intended for the values not specified to be
reset to their defaults, or not.

Where this is a bit more interesting is in the case of sequences, where
resetting the sequence to zero may cause further inserts into an
existing table to fail.  Of course, were a user to use 'drop if exists'
followed by a 'create', they'd get the same behavior..  However, 'create
if not exists' would leave the sequence alone, but in a potentially
unknown state.
Thanks,
    Stephen

On Tue, Apr 1, 2014 at 1:34 PM, Stephen Frost <sfrost@snowman.net> wrote:
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> On Tue, Apr 1, 2014 at 7:28 AM, Fabrízio de Royes Mello
>> <fabriziomello@gmail.com> wrote:
>> > Think about the statements below:
>> >
>> > CREATE ROLE test NOLOGIN;
>> > CREATE OR REPLACE ROLE test;
>> >
>> > If we execute the statements above the result should be the role 'test' can
>> > login. Correct?
>
>> Except if I am missing something, the second query means that it is
>> going to replace the existing user test with a new one, with the
>> settings specified in the 2nd query, all being default values. As the
>> default for login is NOLOGIN, the user test should not be able to log
>> in the server.
>
> That's more-or-less the behavior we're trying to work out.  I've been
> meaning to go back and look at what we've been doing with the existing
> COR cases and just haven't gotten to it yet.

For example, on views, COR fails if it the new view does not contain
the old list of columns, same order and same data type, and can be
completed with new columns. The ownership of the view remains the same
as well. For functions, the argument types and return type need to
remain the same. As I understand, COR are useful because they
guarantee that no objects depending on it would be broken and are made
when a user wants to extend an object or redefine its internals. For
example, we should not allow that IMO:
CREATE ROLE foo LOGIN REPLICATION; -- ok
CREATE OR REPLACE ROLE foo NOREPLICATION; --error
Because with the 2nd query replication would break replication.

For roles, I am not completely sure how you would to that, but I would
imagine that you would need to keep track of all the parameters are
using non-default settings and specified directly by the user in
CREATE ROLE/USER. Then COR would fail if user tries to change some of
those parameters to values that do not map the non-default ones in the
first query (by tracking them in a new pg_authid column, berk, without
thinking about complications induced by IN ROLE, IN GROUP and
friends...). Perhaps I am thinking too much though.

> The pertinent question being if we assume the user intended for the
> values not specified to be reset to their defaults, or not.
Isn't it what ALTER ROLE aims at?

> Where this is a bit more interesting is in the case of sequences, where
> resetting the sequence to zero may cause further inserts into an
> existing table to fail.  Of course, were a user to use 'drop if exists'
> followed by a 'create', they'd get the same behavior..  However, 'create
> if not exists' would leave the sequence alone, but in a potentially
> unknown state.
You could face failures on a serial column as well by changing the
increment sign of its sequence with a COR, so you would need more
guarantees than a min value.
Regards,
--
Michael

Stephen Frost <sfrost@snowman.net> writes:
> * Michael Paquier (michael.paquier@gmail.com) wrote:
>> Except if I am missing something, the second query means that it is
>> going to replace the existing user test with a new one, with the
>> settings specified in the 2nd query, all being default values. As the
>> default for login is NOLOGIN, the user test should not be able to log
>> in the server.

> That's more-or-less the behavior we're trying to work out.  I've been
> meaning to go back and look at what we've been doing with the existing
> COR cases and just haven't gotten to it yet.  The pertinent question
> being if we assume the user intended for the values not specified to be
> reset to their defaults, or not.

Yes, it has to be that way.  The entire argument for COR hinges on the
assumption that if you execute the statement, and it succeeds, the
properties of the object are equivalent to what they'd be if there had
been no predecessor object.  Otherwise it's just the same as CINE,
which offers no guarantees worth mentioning about the object's
properties.

I'm willing to bend that to the extent of saying that COR leaves in place
subsidiary properties that you might add *with additional statements* ---
for example, foreign keys for a table, or privilege grants for a role.
But the properties of the role itself have to be predictable from the COR
statement, or it's useless.

> Where this is a bit more interesting is in the case of sequences, where
> resetting the sequence to zero may cause further inserts into an
> existing table to fail.

Yeah.  Sequences do have contained data, which makes COR harder to define
--- that's part of the reason why we have CINE not COR for tables, and
maybe we have to do the same for sequences.  The point being exactly
that if you use CINE, you're implicitly accepting that you don't know
the ensuing state fully.
        regards, tom lane

On Tue, Apr 1, 2014 at 10:03 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I'm willing to bend that to the extent of saying that COR leaves in place
> subsidiary properties that you might add *with additional statements* ---
> for example, foreign keys for a table, or privilege grants for a role.
> But the properties of the role itself have to be predictable from the COR
> statement, or it's useless.

+1.

>> Where this is a bit more interesting is in the case of sequences, where
>> resetting the sequence to zero may cause further inserts into an
>> existing table to fail.
>
> Yeah.  Sequences do have contained data, which makes COR harder to define
> --- that's part of the reason why we have CINE not COR for tables, and
> maybe we have to do the same for sequences.  The point being exactly
> that if you use CINE, you're implicitly accepting that you don't know
> the ensuing state fully.

Yeah.  I think CINE is more sensible than COR for sequences, for
precisely the reason that they do have contained data (even if it's
basically only one value).

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

On 04/14/2014 10:31 PM, Fabrízio de Royes Mello wrote:
> On Tue, Apr 1, 2014 at 2:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>>>> Where this is a bit more interesting is in the case of sequences, where
>>>> resetting the sequence to zero may cause further inserts into an
>>>> existing table to fail.
>>>
>>> Yeah.  Sequences do have contained data, which makes COR harder to
> define
>>> --- that's part of the reason why we have CINE not COR for tables, and
>>> maybe we have to do the same for sequences.  The point being exactly
>>> that if you use CINE, you're implicitly accepting that you don't know
>>> the ensuing state fully.
>>
>> Yeah.  I think CINE is more sensible than COR for sequences, for
>> precisely the reason that they do have contained data (even if it's
>> basically only one value).
>>
>
> The attached patch contains CINE for sequences.
>
> I just strip this code from the patch rejected before.

Committed with minor changes:

* The documentation promised too much. It said that it would not throw 
an error "if a sequence with the same name exists". In fact, it will not 
throw an error if any relation with the same name exists. I rewrote that 
paragraph to emphasize that more, re-using the phrases from the CREATE 
TABLE manual page.

* don't call RangeVarGetAndCheckCreationNamespace unnecessarily when IF 
NOT EXISTS is not used.

- Heikki

On Tue, Aug 26, 2014 at 4:20 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
> On 04/14/2014 10:31 PM, Fabrízio de Royes Mello wrote:
>> The attached patch contains CINE for sequences.
>>
>> I just strip this code from the patch rejected before.
>
> Committed with minor changes

Hmm, the CommitFest app lists Marko Tiikkaja as the reviewer, but I
can't find his review anywhere...

The documentation claims:
CREATE [ IF NOT EXISTS ] SEQUENCE name
But grammar implements it the other way around:
CREATE SEQUENCE IF NOT EXISTS name;

Regards,
Marti

On 10/3/14, 4:38 AM, Fabrízio de Royes Mello wrote:
> On Thu, Oct 2, 2014 at 9:38 PM, Marti Raudsepp <marti@juffo.org> wrote:
>>
>> On Tue, Aug 26, 2014 at 4:20 PM, Heikki Linnakangas
>> <hlinnakangas@vmware.com> wrote:
>>> On 04/14/2014 10:31 PM, Fabrízio de Royes Mello wrote:
>>>> The attached patch contains CINE for sequences.
>>>>
>>>> I just strip this code from the patch rejected before.
>>>
>>> Committed with minor changes
>>
>> Hmm, the CommitFest app lists Marko Tiikkaja as the reviewer, but I
>> can't find his review anywhere...
>>
>
> Maybe he have no time to review it.

Yes, Heikki picked this up before I had a chance to review it.  Sorry 
about that :-(


.marko

On 10/03/2014 05:38 AM, Fabrízio de Royes Mello wrote:
> On Thu, Oct 2, 2014 at 9:38 PM, Marti Raudsepp <marti@juffo.org> wrote:
>> The documentation claims:
>> CREATE [ IF NOT EXISTS ] SEQUENCE name
>> But grammar implements it the other way around:
>> CREATE SEQUENCE IF NOT EXISTS name;
>
> You are correct. Fix attached.

Thanks, fixed.

- Heikki