Re: Patch to add support of "IF NOT EXISTS" to others "CREATE" statements

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 ] ...

My intention is cover anothers CREATE statements too, not just the above.

If has no objection about this implementation I'll finish him and soon I
sent the patch.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

On Fri, May 24, 2013 at 12:22 PM, Fabrízio de Royes Mello <
fabriziomello(at)gmail(dot)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}] (...)]

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

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.

On Wed, Jun 12, 2013 at 4:00 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>
>
> 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.
>

I did not include "CREATE FUNCTION" precisely because I had the same doubts.

IMO the "IF NOT EXISTS" and "OR REPLACE" are differents, and can coexists in
the same statements but not used at the same time:

CREATE [ OF REPLACE | IF NOT EXISTS ] FUNCTION ...

I can use "IF NOT EXISTS" to "CREATE" a {FUNCTION | AGGREGATE | OPERATOR}
without replace (OR REPLACE) its definition to just create missing objects
and don't
raise an exception if already exists.

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

Can you explain more about it?

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

On 13 June 2013 04:30, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> I'm wondering where "IF NOT EXISTS" and "OR REPLACE" will meet.
>

CREATE OR REPLACE (or ALTER / UPDATE ?) would definitely be useful for
enums, where it would be nice if we could teach an ORM to generate DDL
based on the current values of the enum in code, and know that after the
operation had completed, the database enum type matched the code enum type.
I don't think a sequence of ALTER TYPE ADD VALUE IF NOT EXISTS quite does
the trick, as it doesn't guarantee that the db enum is 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 raise an error if any of the elements were out of order.

Currently to get to a known state for enums you have to write manual
migration scripts, and while that tends to be how I roll anyway, often when
starting projects in rails / grails / hibernate etc people rely on db
schemas generated by the framework as it lets them prototype with less
mucking around. It would be nice for those frameworks to be able to
generate enum types in a known state.

Cheers

Tom

Hi,

Did some basic checks on this patch. List-wise feedback below.

- Removed unnecessary extra-lines: Yes
- Cleanly applies to Git-Head: Yes
- Documentation Updated: Yes
- Tests Updated: Yes
- All tests pass: Yes. (But see Note below)
- Does it Work (CREATE AGGREGATE): Yes
- Does it Work (CREATE OPERATOR): Yes
- Does it Work (CREATE TYPE): Yes
- Does it Work (CREATE TEXT SEARCH): Yes
- Does it Work (CREATE COLLATION): Yes

- Do we want it?: ???

- Is this a new feature: Yes
- Does it support pg_dump: Unable to test currently :(
- Does it follow coding guidelines: Yes

- Any visible issues: No
- Any corner cases missed out: Some tests are not extensive (eg. CREATE
COLLATION).
- Performance tests required: No
- Any compiler warnings: A scan.c warning (scan.c:10181:23: warning: unused
variable ‘yyg’ [-Wunused-variable]) although I doubt that is being caused
by this patch.
- Are comments sufficient: Can't comment much on code comments.

- Others:
Number of new lines added not covered by tests: ~208

======
A typical kind of ERROR is emitted in most tests. (Verified at least in
CREATE AGGREGATE / OPERATOR / TEXT SEARCH TEMPLATE).

For e.g. CREATE OPERATOR IF NOT EXISTS tries to create an OPERATOR that is
already created in the test a few lines above. So although the feature is
tested, the test unnecessarily creates the first OPERATOR. If you need to
maintain 'completeness' within each tests, you could use unique numbering
of objects instead.

CREATE OPERATOR ## (
leftarg = path,
rightarg = path,
procedure = path_inter,
commutator = ##
);
CREATE OPERATOR ## (
leftarg = path,
rightarg = path,
procedure = path_inter,
commutator = ##
);
ERROR: operator ## already exists
CREATE OPERATOR IF NOT EXISTS ## (
leftarg = path,
rightarg = path,
procedure = path_inter,
commutator = ##
);
NOTICE: operator ## already exists, skipping

=====

--
Robins Tharakan

On 24 May 2013 20:52, Fabrízio de Royes Mello <fabriziomello(at)gmail(dot)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 ] ...
>
> My intention is cover anothers CREATE statements too, not just the above.
>
> If has no objection about this implementation I'll finish him and soon I
> sent the patch.
>
> Regards,
>
> --
> Fabrízio de Royes Mello
> Consultoria/Coaching PostgreSQL
> >> Blog sobre TI: http://fabriziomello.blogspot.com
> >> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
> >> Twitter: http://twitter.com/fabriziomello
>

On 24 May 2013 20:52, Fabrízio de Royes Mello <fabriziomello(at)gmail(dot)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 ] ...
>
> My intention is cover anothers CREATE statements too, not just the above.
>
> If has no objection about this implementation I'll finish him and soon I
> sent the patch.
>
> Regards,
>
> --
> Fabrízio de Royes Mello
> Consultoria/Coaching PostgreSQL
> >> Blog sobre TI: http://fabriziomello.blogspot.com
> >> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
> >> Twitter: http://twitter.com/fabriziomello
>

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)

On Mon, Jun 17, 2013 at 12:36 AM, Robins Tharakan <tharakan(at)gmail(dot)com>wrote:

> Hi,
>
> Did some basic checks on this patch. List-wise feedback below.
>
> [...]
>
>
Dear Robins,

Thanks for your review. I attach your considerations to Commit Fest [1].

Regards,

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

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

On Mon, Jun 17, 2013 at 11:33 PM, Peter Eisentraut <peter_e(at)gmx(dot)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].

Regards,

[1]
https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

On Wed, Jun 19, 2013 at 12:45 PM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
> On Mon, Jun 17, 2013 at 11:33 PM, Peter Eisentraut <peter_e(at)gmx(dot)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

On Thu, Jun 20, 2013 at 1:52 AM, Amit Langote <amitlangote09(at)gmail(dot)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

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

On Thu, Jun 20, 2013 at 9:48 PM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)com> wrote:
>
> On Thu, Jun 20, 2013 at 1:52 AM, Amit Langote <amitlangote09(at)gmail(dot)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(at)gmx(dot)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(at)gmail(dot)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

On Thu, Jun 20, 2013 at 1:24 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> 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.

I agree if is possible to have OR REPLACE then we must do that, but in
other hands
I don't see a problem if we have support to both IF NOT EXISTS and OR
REPLACE. In
some cases we don't really want to replace the object body if its already
exists so
IF NOT EXISTS is useful to don't break the transaction inside a upgrade
script.

> >> > 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.
>

Well, my intention is do that for all CREATE commands.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

On Mon, Jun 24, 2013 at 8:05 AM, Andres Freund <andres(at)2ndquadrant(dot)com>
wrote:
>
>
> 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.
>

Yeah... this is my purpose, but I decide do that in two steps. First with
the patch already
sent to CF1 and second with another patch to cover the remaining CREATE
commands.

I created a simple spreadsheet [1] to control my work. Suggestions are
welcome.

>
> * 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
>

Ok.

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

+1

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

In fact I would say that will be seldom used, but I don't see any
problem to implement them.

Regards,

[1]
https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

At 2013-06-12 14:29:59 -0300, fabriziomello(at)gmail(dot)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 Sat, Jul 13, 2013 at 7:21 AM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:
>
> At 2013-06-12 14:29:59 -0300, fabriziomello(at)gmail(dot)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.
>

Abhijit, thanks for your review.

> 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).
>

Ok.

> 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.
>

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:

CREATE AGGREGATE opt_if_not_exists func_name aggr_args
definition
{
DefineStmt *n = makeNode(DefineStmt);
n->kind = OBJECT_AGGREGATE;
n->oldstyle = false;
n->defnames = $4;
n->args = $5;
n->definition = $6;
n->if_not_exists = $3;
$$ = (Node *)n;
}

I changed all statements to use "opt_if_not_exists" (like above) but bison
do not
accept it.

Looking more carefully at gram.y when we use "IF_P EXISTS" (DROP ROLE IF
EXISTS) all was written using variants of original statement. And exists
the rule
"opt_if_exists" too, that's used in "DROP CAST". Because of this reason I
use
"opt_if_not_exists" in "CREATE CAST".

> 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.
>

Ok.

> > 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.
>

Fixed.

> > 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.
>

Fixed.

> > 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).
>

Fixed.

> > @@ -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.)
>

Fixed.

> > 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.
>

Fixed.

> (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.)
>

You all right, and in other places we have "missing_ok".

When I finish this patch and the next one (to add IF NOT EXISTS to
remaining CREATE statements) I send a patch to normalize that.

> 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").
>

Fixed.

> I don't see any other problems with the patch. It passes "make check".
>
> I'm marking this as "Waiting on Author".
>

And now I'm marking this as "Needs Review" (again) ;-)

Regards,

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

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(at)svana(dot)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(at)gmail(dot)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(at)timbira(dot)com(dot)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(at)timbira(dot)com(dot)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(at)gmail(dot)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(at)gmail(dot)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(at)gmail(dot)com) wrote:
> On Wed, Jun 12, 2013 at 3:00 PM, Peter Eisentraut <peter_e(at)gmx(dot)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(at)snowman(dot)net> writes:
> * Robert Haas (robertmhaas(at)gmail(dot)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

2014-01-19 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Robert Haas (robertmhaas(at)gmail(dot)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(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

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

On Sat, Jan 18, 2014 at 11:12 PM, Stephen Frost <sfrost(at)snowman(dot)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.
>

Hi Stephen,

First I'm really sorry about the long time without an answer. I'm very busy
in this start of the year.

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.

I know that is a very specific case, but in my mind I don't see any problem
to have CINE and COR to this objects. The behavior is totally different.

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

On Fri, Feb 28, 2014 at 5:05 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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

=?ISO-8859-1?Q?Fabr=EDzio_de_Royes_Mello?= <fabriziomello(at)gmail(dot)com> writes:
> On Sat, Jan 18, 2014 at 11:12 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> Fabrzio, 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

On Sat, Mar 1, 2014 at 2:11 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> =?ISO-8859-1?Q?Fabr=EDzio_de_Royes_Mello?= <fabriziomello(at)gmail(dot)com>
writes:
> > On Sat, Jan 18, 2014 at 11:12 PM, Stephen Frost <sfrost(at)snowman(dot)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?
>

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.

I know it's a very specific use case, but in a scenario with many servers
and many automated tasks in different pipelines, CINE will be very useful.
I have this kind of troubles mostly with functions (we use COR), and
sometimes we will discover that the production version of function is wrong
after we receive a user notify, and in this situation many times we spend a
lot of effort do fix the whole damage.

Grettings,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQ
>> 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

=?ISO-8859-1?Q?Fabr=EDzio_de_Royes_Mello?= <fabriziomello(at)gmail(dot)com> writes:
> On Sat, Mar 1, 2014 at 2:11 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On Sat, Mar 1, 2014 at 7:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> =?ISO-8859-1?Q?Fabr=EDzio_de_Royes_Mello?= <fabriziomello(at)gmail(dot)com>
writes:
> > On Sat, Mar 1, 2014 at 2:11 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.

Sorry, I know my use case is very specific...

We don't have this feature is a strong argument just because we can
implement COR instead? Or maybe just we don't want to add more complexity
to source code?

The complexity to source code added by this feature is minimal, but the
result is very useful, and can be used for many tools (i.e. rails
migrations, python alembic, doctrine, and others)

> 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.
>

You are completely right.

But we already have "DROP ... IF EXISTS", then I think if we would have
"CREATE ... IF NOT EXISTS" (the inverse behavior) will be very natural...
and I agree in implement "CREATE OR REPLACE" too.

Grettings,

--
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

On Sun, Mar 2, 2014 at 1:04 AM, Fabrízio de Royes Mello <
fabriziomello(at)gmail(dot)com> wrote:
>
> On Sat, Mar 1, 2014 at 7:39 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >
> > =?ISO-8859-1?Q?Fabr=EDzio_de_Royes_Mello?= <fabriziomello(at)gmail(dot)com>
writes:
> > > On Sat, Mar 1, 2014 at 2:11 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.
>
> Sorry, I know my use case is very specific...
>
> We don't have this feature is a strong argument just because we can
implement COR instead? Or maybe just we don't want to add more complexity
to source code?
>
> The complexity to source code added by this feature is minimal, but the
result is very useful, and can be used for many tools (i.e. rails
migrations, python alembic, doctrine, and others)
>
>
> > 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.
> >
>
> You are completely right.
>
> But we already have "DROP ... IF EXISTS", then I think if we would have
"CREATE ... IF NOT EXISTS" (the inverse behavior) will be very natural...
and I agree in implement "CREATE OR REPLACE" too.
>

Hi all,

Sorry to return with this thread, but I think we missed something during
the review.

In 17th August 2013 [1] I added more code to patch [2]:

- 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?

Regards,

[1] https://commitfest.postgresql.org/action/patch_view?id=1133
[2] http://www.postgresql.org/message-id/520FE6D4.8050402@timbira.com.br

--
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

* Fabrízio de Royes Mello (fabriziomello(at)gmail(dot)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

On Mon, Mar 31, 2014 at 4:52 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> * Fabrízio de Royes Mello (fabriziomello(at)gmail(dot)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?

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

* Fabrízio de Royes Mello (fabriziomello(at)gmail(dot)com) wrote:
> On Mon, Mar 31, 2014 at 4:52 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> >
> > * Fabrízio de Royes Mello (fabriziomello(at)gmail(dot)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

On Mon, Mar 31, 2014 at 5:00 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> * Fabrízio de Royes Mello (fabriziomello(at)gmail(dot)com) wrote:
> > On Mon, Mar 31, 2014 at 4:52 PM, Stephen Frost <sfrost(at)snowman(dot)net>
wrote:
> > >
> > > * Fabrízio de Royes Mello (fabriziomello(at)gmail(dot)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?
>

Because they maintain user data?

--
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

* Fabrízio de Royes Mello (fabriziomello(at)gmail(dot)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

On Mon, Mar 31, 2014 at 5:46 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> * Fabrízio de Royes Mello (fabriziomello(at)gmail(dot)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.
>

You mean if we execute 'CREATE OR REPLACE' must we verify the default
values of this statement and compare with the existing ones?

> 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.
>

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?

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

On Tue, Apr 1, 2014 at 7:28 AM, Fabrízio de Royes Mello
<fabriziomello(at)gmail(dot)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

On Tue, Apr 1, 2014 at 1:14 AM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
wrote:
>
> On Tue, Apr 1, 2014 at 7:28 AM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)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.
>

Yeah... you are correct... I meant:

CREATE ROLE test LOGIN;
CREATE OR REPLACE ROLE test;

Then the COR will replace the user 'test' setting a new default value to
NOLOGIN. Correct?

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

* Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
> On Tue, Apr 1, 2014 at 7:28 AM, Fabrízio de Royes Mello
> <fabriziomello(at)gmail(dot)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(at)snowman(dot)net> wrote:
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)com) wrote:
>> On Tue, Apr 1, 2014 at 7:28 AM, Fabrízio de Royes Mello
>> <fabriziomello(at)gmail(dot)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(at)snowman(dot)net> writes:
> * Michael Paquier (michael(dot)paquier(at)gmail(dot)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(at)sss(dot)pgh(dot)pa(dot)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 Tue, Apr 1, 2014 at 2:46 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Apr 1, 2014 at 10:03 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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).
>
>
Well then I'll separate CINE for sequences for the previous rejected... is
this a material for 9.5?

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

On Tue, Apr 1, 2014 at 2:46 PM, Robert Haas <robertmhaas(at)gmail(dot)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.

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

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(at)gmail(dot)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 10:20 AM, Heikki Linnakangas <
hlinnakangas(at)vmware(dot)com> wrote:

> 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(at)gmail(dot)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.
>
>
Thanks!

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On Tue, Aug 26, 2014 at 4:20 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)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 Thu, Oct 2, 2014 at 9:38 PM, Marti Raudsepp <marti(at)juffo(dot)org> wrote:
>
> On Tue, Aug 26, 2014 at 4:20 PM, Heikki Linnakangas
> <hlinnakangas(at)vmware(dot)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.

> 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.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

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(at)juffo(dot)org> wrote:
>>
>> On Tue, Aug 26, 2014 at 4:20 PM, Heikki Linnakangas
>> <hlinnakangas(at)vmware(dot)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(at)juffo(dot)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