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
Show quoted text
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@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}] (...)]
Regards,
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
Blog sobre TI: http://fabriziomello.blogspot.com
Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
Twitter: http://twitter.com/fabriziomello
Attachments:
create_if_not_exists.patchapplication/octet-stream; name=create_if_not_exists.patchDownload+802-122
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 12, 2013 at 4:00 PM, Peter Eisentraut <peter_e@gmx.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
Show quoted text
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@gmx.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@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 ] ...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 PostgreSQLBlog 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@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 ] ...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 PostgreSQLBlog 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)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 17, 2013 at 12:36 AM, Robins Tharakan <tharakan@gmail.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]https://commitfest.postgresql.org/action/patch_view?id=1133.
Regards,
[1]: https://commitfest.postgresql.org/action/patch_view?id=1133
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
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@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]https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing.
Regards,
[1]: https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing
https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
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@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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]https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing.
Regards,
[1]: https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing
https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
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@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,
Hmm, okay. Last time I checked, the CREATE OPERATOR CLASS row was
empty, so asked.
--
Amit Langote
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 20, 2013 at 1:24 PM, Peter Eisentraut <peter_e@gmx.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
Show quoted text
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@2ndquadrant.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]https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing 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
https://docs.google.com/spreadsheet/ccc?key=0Ai7oCVcVQiKFdEctQUxNNlR1R2xRTUpJNFNDcFo4MUE&usp=sharing
--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
Show quoted text
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@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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Jul 13, 2013 at 7:21 AM, Abhijit Menon-Sen <ams@2ndquadrant.com>
wrote:
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.
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
Attachments:
create_if_not_exists_v2.patchapplication/octet-stream; name=create_if_not_exists_v2.patchDownload+798-111
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 1I 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
