alter enum add value if not exists
Here is a patch for this feature, which should alleviate some of the
woes caused by adding labels not being transactional (and thus not
allowing for the catching of errors).
(Also available on the add_enum_ine branch at
<https://bitbucket.org/adunstan/pgdevel>)
cheers
andrew
Attachments:
add-enum-ine.patchtext/x-patch; name=add-enum-ine.patchDownload
*** a/doc/src/sgml/ref/alter_type.sgml
--- b/doc/src/sgml/ref/alter_type.sgml
***************
*** 28,34 **** ALTER TYPE <replaceable class="PARAMETER">name</replaceable> OWNER TO <replaceab
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME ATTRIBUTE <replaceable class="PARAMETER">attribute_name</replaceable> TO <replaceable class="PARAMETER">new_attribute_name</replaceable>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME TO <replaceable class="PARAMETER">new_name</replaceable> [ CASCADE | RESTRICT ]
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> SET SCHEMA <replaceable class="PARAMETER">new_schema</replaceable>
! ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE <replaceable class="PARAMETER">new_enum_value</replaceable> [ { BEFORE | AFTER } <replaceable class="PARAMETER">existing_enum_value</replaceable> ]
<phrase>where <replaceable class="PARAMETER">action</replaceable> is one of:</phrase>
--- 28,34 ----
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME ATTRIBUTE <replaceable class="PARAMETER">attribute_name</replaceable> TO <replaceable class="PARAMETER">new_attribute_name</replaceable>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME TO <replaceable class="PARAMETER">new_name</replaceable> [ CASCADE | RESTRICT ]
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> SET SCHEMA <replaceable class="PARAMETER">new_schema</replaceable>
! ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE [ IF NOT EXISTS ] <replaceable class="PARAMETER">new_enum_value</replaceable> [ { BEFORE | AFTER } <replaceable class="PARAMETER">existing_enum_value</replaceable> ]
<phrase>where <replaceable class="PARAMETER">action</replaceable> is one of:</phrase>
***************
*** 106,112 **** ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE <replacea
</varlistentry>
<varlistentry>
! <term><literal>ADD VALUE [ BEFORE | AFTER ]</literal></term>
<listitem>
<para>
This form adds a new value to an enum type. If the new value's place in
--- 106,112 ----
</varlistentry>
<varlistentry>
! <term><literal>ADD VALUE [ IF NOT EXISTS ] [ BEFORE | AFTER ]</literal></term>
<listitem>
<para>
This form adds a new value to an enum type. If the new value's place in
***************
*** 114,119 **** ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE <replacea
--- 114,124 ----
<literal>AFTER</literal>, then the new item is placed at the end of the
list of values.
</para>
+ <para>
+ If <literal>IF NOT EXISTS</literal is used, it is not an error if the
+ type already contains the new value, and no action is taken. Otherwise,
+ an error will occur if the new value is already present.
+ </para>
</listitem>
</varlistentry>
*** a/src/backend/catalog/pg_enum.c
--- b/src/backend/catalog/pg_enum.c
***************
*** 177,183 **** void
AddEnumLabel(Oid enumTypeOid,
const char *newVal,
const char *neighbor,
! bool newValIsAfter)
{
Relation pg_enum;
Oid newOid;
--- 177,184 ----
AddEnumLabel(Oid enumTypeOid,
const char *newVal,
const char *neighbor,
! bool newValIsAfter,
! bool skipIfExists)
{
Relation pg_enum;
Oid newOid;
***************
*** 199,204 **** AddEnumLabel(Oid enumTypeOid,
--- 200,220 ----
errdetail("Labels must be %d characters or less.",
NAMEDATALEN - 1)));
+ /* Do the "IF NOT EXISTS" test if specified */
+ if (skipIfExists)
+ {
+ HeapTuple tup;
+
+ tup = SearchSysCache2(ENUMTYPOIDNAME,
+ ObjectIdGetDatum(enumTypeOid),
+ CStringGetDatum(newVal));
+ if (HeapTupleIsValid(tup))
+ {
+ ReleaseSysCache(tup);
+ return;
+ }
+ }
+
/*
* Acquire a lock on the enum type, which we won't release until commit.
* This ensures that two backends aren't concurrently modifying the same
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
***************
*** 1187,1193 **** AlterEnum(AlterEnumStmt *stmt)
/* Add the new label */
AddEnumLabel(enum_type_oid, stmt->newVal,
! stmt->newValNeighbor, stmt->newValIsAfter);
ReleaseSysCache(tup);
}
--- 1187,1194 ----
/* Add the new label */
AddEnumLabel(enum_type_oid, stmt->newVal,
! stmt->newValNeighbor, stmt->newValIsAfter,
! stmt->skipIfExists);
ReleaseSysCache(tup);
}
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 3041,3046 **** _copyAlterEnumStmt(const AlterEnumStmt *from)
--- 3041,3047 ----
COPY_STRING_FIELD(newVal);
COPY_STRING_FIELD(newValNeighbor);
COPY_SCALAR_FIELD(newValIsAfter);
+ COPY_SCALAR_FIELD(skipIfExists);
return newnode;
}
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 1430,1435 **** _equalAlterEnumStmt(const AlterEnumStmt *a, const AlterEnumStmt *b)
--- 1430,1436 ----
COMPARE_STRING_FIELD(newVal);
COMPARE_STRING_FIELD(newValNeighbor);
COMPARE_SCALAR_FIELD(newValIsAfter);
+ COMPARE_SCALAR_FIELD(skipIfExists);
return true;
}
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 470,476 **** static void processCASbits(int cas_bits, int location, const char *constrType,
%type <windef> window_definition over_clause window_specification
opt_frame_clause frame_extent frame_bound
%type <str> opt_existing_window_name
!
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
--- 470,476 ----
%type <windef> window_definition over_clause window_specification
opt_frame_clause frame_extent frame_bound
%type <str> opt_existing_window_name
! %type <boolean> opt_if_not_exists
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
***************
*** 4618,4652 **** enum_val_list: Sconst
*****************************************************************************/
AlterEnumStmt:
! ALTER TYPE_P any_name ADD_P VALUE_P Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $6;
n->newValNeighbor = NULL;
n->newValIsAfter = true;
$$ = (Node *) n;
}
! | ALTER TYPE_P any_name ADD_P VALUE_P Sconst BEFORE Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $6;
! n->newValNeighbor = $8;
n->newValIsAfter = false;
$$ = (Node *) n;
}
! | ALTER TYPE_P any_name ADD_P VALUE_P Sconst AFTER Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $6;
! n->newValNeighbor = $8;
n->newValIsAfter = true;
$$ = (Node *) n;
}
;
/*****************************************************************************
*
--- 4618,4659 ----
*****************************************************************************/
AlterEnumStmt:
! ALTER TYPE_P any_name ADD_P VALUE_P opt_if_not_exists Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $7;
n->newValNeighbor = NULL;
n->newValIsAfter = true;
+ n->skipIfExists = $6;
$$ = (Node *) n;
}
! | ALTER TYPE_P any_name ADD_P VALUE_P opt_if_not_exists Sconst BEFORE Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $7;
! n->newValNeighbor = $9;
n->newValIsAfter = false;
+ n->skipIfExists = $6;
$$ = (Node *) n;
}
! | ALTER TYPE_P any_name ADD_P VALUE_P opt_if_not_exists Sconst AFTER Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $7;
! n->newValNeighbor = $9;
n->newValIsAfter = true;
+ n->skipIfExists = $6;
$$ = (Node *) n;
}
;
+ opt_if_not_exists: IF_P NOT EXISTS { $$ = true; }
+ | /* empty */ { $$ = false; }
+ ;
+
/*****************************************************************************
*
*** a/src/include/catalog/pg_enum.h
--- b/src/include/catalog/pg_enum.h
***************
*** 65,70 **** typedef FormData_pg_enum *Form_pg_enum;
extern void EnumValuesCreate(Oid enumTypeOid, List *vals);
extern void EnumValuesDelete(Oid enumTypeOid);
extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
! const char *neighbor, bool newValIsAfter);
#endif /* PG_ENUM_H */
--- 65,71 ----
extern void EnumValuesCreate(Oid enumTypeOid, List *vals);
extern void EnumValuesDelete(Oid enumTypeOid);
extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
! const char *neighbor, bool newValIsAfter,
! bool skipIfExists);
#endif /* PG_ENUM_H */
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 2306,2311 **** typedef struct AlterEnumStmt
--- 2306,2312 ----
char *newVal; /* new enum value's name */
char *newValNeighbor; /* neighboring enum value, if specified */
bool newValIsAfter; /* place new enum value after neighbor? */
+ bool skipIfExists; /* ignore statement if label already exists */
} AlterEnumStmt;
/* ----------------------
*** a/src/test/regress/expected/enum.out
--- b/src/test/regress/expected/enum.out
***************
*** 95,100 **** ERROR: invalid enum label "plutoplutoplutoplutoplutoplutoplutoplutoplutoplutopl
--- 95,122 ----
DETAIL: Labels must be 63 characters or less.
ALTER TYPE planets ADD VALUE 'pluto' AFTER 'zeus';
ERROR: "zeus" is not an existing enum label
+ -- if not exists tests
+ -- existing value gives error
+ -- We can't do this test because the error contains the
+ -- offending Oid value, which is unpredictable.
+ -- ALTER TYPE planets ADD VALUE 'mercury';
+ -- unless IF NOT EXISTS is specified
+ ALTER TYPE planets ADD VALUE IF NOT EXISTS 'mercury';
+ -- should be neptune, not mercury
+ SELECT enum_last(NULL::planets);
+ enum_last
+ -----------
+ neptune
+ (1 row)
+
+ ALTER TYPE planets ADD VALUE IF NOT EXISTS 'pluto';
+ -- should be pluto, i.e. the new value
+ SELECT enum_last(NULL::planets);
+ enum_last
+ -----------
+ pluto
+ (1 row)
+
--
-- Test inserting so many values that we have to renumber
--
*** a/src/test/regress/sql/enum.sql
--- b/src/test/regress/sql/enum.sql
***************
*** 54,59 **** ALTER TYPE planets ADD VALUE
--- 54,79 ----
ALTER TYPE planets ADD VALUE 'pluto' AFTER 'zeus';
+ -- if not exists tests
+
+ -- existing value gives error
+
+ -- We can't do this test because the error contains the
+ -- offending Oid value, which is unpredictable.
+ -- ALTER TYPE planets ADD VALUE 'mercury';
+
+ -- unless IF NOT EXISTS is specified
+ ALTER TYPE planets ADD VALUE IF NOT EXISTS 'mercury';
+
+ -- should be neptune, not mercury
+ SELECT enum_last(NULL::planets);
+
+ ALTER TYPE planets ADD VALUE IF NOT EXISTS 'pluto';
+
+ -- should be pluto, i.e. the new value
+ SELECT enum_last(NULL::planets);
+
+
--
-- Test inserting so many values that we have to renumber
--
On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
Here is a patch for this feature, which should alleviate some of the woes
caused by adding labels not being transactional (and thus not allowing for
the catching of errors).
I haven't actually checked the code in detail, but if it's not
transactional, how does it actually prevent race conditions? Doesn't
it at least have to do it's check *after* the enum is locked?
I don't recall the exact discussion, but was there something about
enum labels that made it impossible to make them transactional, or was
it just "lots of work, let's do that later instead" to get the feature
in? If the second, does anyone have plans to fix it? It is a quite
annoying limitation :(
That said, this functionality would be useful even *if* the enum label
addition was made transactional...
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On 08/23/2012 06:47 AM, Magnus Hagander wrote:
On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
Here is a patch for this feature, which should alleviate some of the woes
caused by adding labels not being transactional (and thus not allowing for
the catching of errors).I haven't actually checked the code in detail, but if it's not
transactional, how does it actually prevent race conditions? Doesn't
it at least have to do it's check *after* the enum is locked?
Well, you can't remove a label, and if the test succeeds it results in
your doing nothing, so my possibly naive thinking was that that wasn't
necessary. But I could easily be wrong :-)
I don't recall the exact discussion, but was there something about
enum labels that made it impossible to make them transactional, or was
it just "lots of work, let's do that later instead" to get the feature
in? If the second, does anyone have plans to fix it? It is a quite
annoying limitation :(
I don't know of any plans to fix it.
That said, this functionality would be useful even *if* the enum label
addition was made transactional...
Right.
cheers
andrew
On Thu, Aug 23, 2012 at 1:35 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
On 08/23/2012 06:47 AM, Magnus Hagander wrote:
On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew@dunslane.net>
wrote:Here is a patch for this feature, which should alleviate some of the woes
caused by adding labels not being transactional (and thus not allowing
for
the catching of errors).I haven't actually checked the code in detail, but if it's not
transactional, how does it actually prevent race conditions? Doesn't
it at least have to do it's check *after* the enum is locked?Well, you can't remove a label, and if the test succeeds it results in your
doing nothing, so my possibly naive thinking was that that wasn't necessary.
But I could easily be wrong :-)
Ah, good point.
But still: what if:
Session A checks if the label is present, it's not.
Session B checks if the label is present, it's not.
Session A locks the enum, and adds the label, then releases lock.
Session B locks the enum, and tries to add it -- and you still get a failure.
It doesn't break, of course ,since it's protected by the unique index.
But aren't you at risk of getting the very error message you're trying
to avoid?
Or am I missing something? (I probably am :D - I still haven't looked
at it in detail)
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On 08/23/2012 07:39 AM, Magnus Hagander wrote:
On Thu, Aug 23, 2012 at 1:35 PM, Andrew Dunstan <andrew@dunslane.net> wrote:
On 08/23/2012 06:47 AM, Magnus Hagander wrote:
On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <andrew@dunslane.net>
wrote:Here is a patch for this feature, which should alleviate some of the woes
caused by adding labels not being transactional (and thus not allowing
for
the catching of errors).I haven't actually checked the code in detail, but if it's not
transactional, how does it actually prevent race conditions? Doesn't
it at least have to do it's check *after* the enum is locked?Well, you can't remove a label, and if the test succeeds it results in your
doing nothing, so my possibly naive thinking was that that wasn't necessary.
But I could easily be wrong :-)Ah, good point.
But still: what if:Session A checks if the label is present, it's not.
Session B checks if the label is present, it's not.
Session A locks the enum, and adds the label, then releases lock.
Session B locks the enum, and tries to add it -- and you still get a failure.It doesn't break, of course ,since it's protected by the unique index.
But aren't you at risk of getting the very error message you're trying
to avoid?Or am I missing something? (I probably am :D - I still haven't looked
at it in detail)
Yeah, looking further this was probably a thinko on my part. Thanks for
noticing. I've moved the test down so it's done right after the lock is
acquired. Revised patch attached.
cheers
andrew
Attachments:
add-enum-inev2.patchtext/x-patch; name=add-enum-inev2.patchDownload
*** a/doc/src/sgml/ref/alter_type.sgml
--- b/doc/src/sgml/ref/alter_type.sgml
***************
*** 28,34 **** ALTER TYPE <replaceable class="PARAMETER">name</replaceable> OWNER TO <replaceab
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME ATTRIBUTE <replaceable class="PARAMETER">attribute_name</replaceable> TO <replaceable class="PARAMETER">new_attribute_name</replaceable>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME TO <replaceable class="PARAMETER">new_name</replaceable> [ CASCADE | RESTRICT ]
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> SET SCHEMA <replaceable class="PARAMETER">new_schema</replaceable>
! ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE <replaceable class="PARAMETER">new_enum_value</replaceable> [ { BEFORE | AFTER } <replaceable class="PARAMETER">existing_enum_value</replaceable> ]
<phrase>where <replaceable class="PARAMETER">action</replaceable> is one of:</phrase>
--- 28,34 ----
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME ATTRIBUTE <replaceable class="PARAMETER">attribute_name</replaceable> TO <replaceable class="PARAMETER">new_attribute_name</replaceable>
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> RENAME TO <replaceable class="PARAMETER">new_name</replaceable> [ CASCADE | RESTRICT ]
ALTER TYPE <replaceable class="PARAMETER">name</replaceable> SET SCHEMA <replaceable class="PARAMETER">new_schema</replaceable>
! ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE [ IF NOT EXISTS ] <replaceable class="PARAMETER">new_enum_value</replaceable> [ { BEFORE | AFTER } <replaceable class="PARAMETER">existing_enum_value</replaceable> ]
<phrase>where <replaceable class="PARAMETER">action</replaceable> is one of:</phrase>
***************
*** 106,112 **** ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE <replacea
</varlistentry>
<varlistentry>
! <term><literal>ADD VALUE [ BEFORE | AFTER ]</literal></term>
<listitem>
<para>
This form adds a new value to an enum type. If the new value's place in
--- 106,112 ----
</varlistentry>
<varlistentry>
! <term><literal>ADD VALUE [ IF NOT EXISTS ] [ BEFORE | AFTER ]</literal></term>
<listitem>
<para>
This form adds a new value to an enum type. If the new value's place in
***************
*** 114,119 **** ALTER TYPE <replaceable class="PARAMETER">name</replaceable> ADD VALUE <replacea
--- 114,124 ----
<literal>AFTER</literal>, then the new item is placed at the end of the
list of values.
</para>
+ <para>
+ If <literal>IF NOT EXISTS</literal is used, it is not an error if the
+ type already contains the new value, and no action is taken. Otherwise,
+ an error will occur if the new value is already present.
+ </para>
</listitem>
</varlistentry>
*** a/src/backend/catalog/pg_enum.c
--- b/src/backend/catalog/pg_enum.c
***************
*** 177,183 **** void
AddEnumLabel(Oid enumTypeOid,
const char *newVal,
const char *neighbor,
! bool newValIsAfter)
{
Relation pg_enum;
Oid newOid;
--- 177,184 ----
AddEnumLabel(Oid enumTypeOid,
const char *newVal,
const char *neighbor,
! bool newValIsAfter,
! bool skipIfExists)
{
Relation pg_enum;
Oid newOid;
***************
*** 209,214 **** AddEnumLabel(Oid enumTypeOid,
--- 210,230 ----
*/
LockDatabaseObject(TypeRelationId, enumTypeOid, 0, ExclusiveLock);
+ /* Do the "IF NOT EXISTS" test if specified */
+ if (skipIfExists)
+ {
+ HeapTuple tup;
+
+ tup = SearchSysCache2(ENUMTYPOIDNAME,
+ ObjectIdGetDatum(enumTypeOid),
+ CStringGetDatum(newVal));
+ if (HeapTupleIsValid(tup))
+ {
+ ReleaseSysCache(tup);
+ return;
+ }
+ }
+
pg_enum = heap_open(EnumRelationId, RowExclusiveLock);
/* If we have to renumber the existing members, we restart from here */
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
***************
*** 1187,1193 **** AlterEnum(AlterEnumStmt *stmt)
/* Add the new label */
AddEnumLabel(enum_type_oid, stmt->newVal,
! stmt->newValNeighbor, stmt->newValIsAfter);
ReleaseSysCache(tup);
}
--- 1187,1194 ----
/* Add the new label */
AddEnumLabel(enum_type_oid, stmt->newVal,
! stmt->newValNeighbor, stmt->newValIsAfter,
! stmt->skipIfExists);
ReleaseSysCache(tup);
}
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 3041,3046 **** _copyAlterEnumStmt(const AlterEnumStmt *from)
--- 3041,3047 ----
COPY_STRING_FIELD(newVal);
COPY_STRING_FIELD(newValNeighbor);
COPY_SCALAR_FIELD(newValIsAfter);
+ COPY_SCALAR_FIELD(skipIfExists);
return newnode;
}
*** a/src/backend/nodes/equalfuncs.c
--- b/src/backend/nodes/equalfuncs.c
***************
*** 1430,1435 **** _equalAlterEnumStmt(const AlterEnumStmt *a, const AlterEnumStmt *b)
--- 1430,1436 ----
COMPARE_STRING_FIELD(newVal);
COMPARE_STRING_FIELD(newValNeighbor);
COMPARE_SCALAR_FIELD(newValIsAfter);
+ COMPARE_SCALAR_FIELD(skipIfExists);
return true;
}
*** a/src/backend/parser/gram.y
--- b/src/backend/parser/gram.y
***************
*** 470,476 **** static void processCASbits(int cas_bits, int location, const char *constrType,
%type <windef> window_definition over_clause window_specification
opt_frame_clause frame_extent frame_bound
%type <str> opt_existing_window_name
!
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
--- 470,476 ----
%type <windef> window_definition over_clause window_specification
opt_frame_clause frame_extent frame_bound
%type <str> opt_existing_window_name
! %type <boolean> opt_if_not_exists
/*
* Non-keyword token types. These are hard-wired into the "flex" lexer.
***************
*** 4618,4652 **** enum_val_list: Sconst
*****************************************************************************/
AlterEnumStmt:
! ALTER TYPE_P any_name ADD_P VALUE_P Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $6;
n->newValNeighbor = NULL;
n->newValIsAfter = true;
$$ = (Node *) n;
}
! | ALTER TYPE_P any_name ADD_P VALUE_P Sconst BEFORE Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $6;
! n->newValNeighbor = $8;
n->newValIsAfter = false;
$$ = (Node *) n;
}
! | ALTER TYPE_P any_name ADD_P VALUE_P Sconst AFTER Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $6;
! n->newValNeighbor = $8;
n->newValIsAfter = true;
$$ = (Node *) n;
}
;
/*****************************************************************************
*
--- 4618,4659 ----
*****************************************************************************/
AlterEnumStmt:
! ALTER TYPE_P any_name ADD_P VALUE_P opt_if_not_exists Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $7;
n->newValNeighbor = NULL;
n->newValIsAfter = true;
+ n->skipIfExists = $6;
$$ = (Node *) n;
}
! | ALTER TYPE_P any_name ADD_P VALUE_P opt_if_not_exists Sconst BEFORE Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $7;
! n->newValNeighbor = $9;
n->newValIsAfter = false;
+ n->skipIfExists = $6;
$$ = (Node *) n;
}
! | ALTER TYPE_P any_name ADD_P VALUE_P opt_if_not_exists Sconst AFTER Sconst
{
AlterEnumStmt *n = makeNode(AlterEnumStmt);
n->typeName = $3;
! n->newVal = $7;
! n->newValNeighbor = $9;
n->newValIsAfter = true;
+ n->skipIfExists = $6;
$$ = (Node *) n;
}
;
+ opt_if_not_exists: IF_P NOT EXISTS { $$ = true; }
+ | /* empty */ { $$ = false; }
+ ;
+
/*****************************************************************************
*
*** a/src/include/catalog/pg_enum.h
--- b/src/include/catalog/pg_enum.h
***************
*** 65,70 **** typedef FormData_pg_enum *Form_pg_enum;
extern void EnumValuesCreate(Oid enumTypeOid, List *vals);
extern void EnumValuesDelete(Oid enumTypeOid);
extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
! const char *neighbor, bool newValIsAfter);
#endif /* PG_ENUM_H */
--- 65,71 ----
extern void EnumValuesCreate(Oid enumTypeOid, List *vals);
extern void EnumValuesDelete(Oid enumTypeOid);
extern void AddEnumLabel(Oid enumTypeOid, const char *newVal,
! const char *neighbor, bool newValIsAfter,
! bool skipIfExists);
#endif /* PG_ENUM_H */
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 2306,2311 **** typedef struct AlterEnumStmt
--- 2306,2312 ----
char *newVal; /* new enum value's name */
char *newValNeighbor; /* neighboring enum value, if specified */
bool newValIsAfter; /* place new enum value after neighbor? */
+ bool skipIfExists; /* ignore statement if label already exists */
} AlterEnumStmt;
/* ----------------------
*** a/src/test/regress/expected/enum.out
--- b/src/test/regress/expected/enum.out
***************
*** 95,100 **** ERROR: invalid enum label "plutoplutoplutoplutoplutoplutoplutoplutoplutoplutopl
--- 95,122 ----
DETAIL: Labels must be 63 characters or less.
ALTER TYPE planets ADD VALUE 'pluto' AFTER 'zeus';
ERROR: "zeus" is not an existing enum label
+ -- if not exists tests
+ -- existing value gives error
+ -- We can't do this test because the error contains the
+ -- offending Oid value, which is unpredictable.
+ -- ALTER TYPE planets ADD VALUE 'mercury';
+ -- unless IF NOT EXISTS is specified
+ ALTER TYPE planets ADD VALUE IF NOT EXISTS 'mercury';
+ -- should be neptune, not mercury
+ SELECT enum_last(NULL::planets);
+ enum_last
+ -----------
+ neptune
+ (1 row)
+
+ ALTER TYPE planets ADD VALUE IF NOT EXISTS 'pluto';
+ -- should be pluto, i.e. the new value
+ SELECT enum_last(NULL::planets);
+ enum_last
+ -----------
+ pluto
+ (1 row)
+
--
-- Test inserting so many values that we have to renumber
--
*** a/src/test/regress/sql/enum.sql
--- b/src/test/regress/sql/enum.sql
***************
*** 54,59 **** ALTER TYPE planets ADD VALUE
--- 54,79 ----
ALTER TYPE planets ADD VALUE 'pluto' AFTER 'zeus';
+ -- if not exists tests
+
+ -- existing value gives error
+
+ -- We can't do this test because the error contains the
+ -- offending Oid value, which is unpredictable.
+ -- ALTER TYPE planets ADD VALUE 'mercury';
+
+ -- unless IF NOT EXISTS is specified
+ ALTER TYPE planets ADD VALUE IF NOT EXISTS 'mercury';
+
+ -- should be neptune, not mercury
+ SELECT enum_last(NULL::planets);
+
+ ALTER TYPE planets ADD VALUE IF NOT EXISTS 'pluto';
+
+ -- should be pluto, i.e. the new value
+ SELECT enum_last(NULL::planets);
+
+
--
-- Test inserting so many values that we have to renumber
--
Andrew Dunstan <andrew@dunslane.net> writes:
On 08/23/2012 07:39 AM, Magnus Hagander wrote:
It doesn't break, of course ,since it's protected by the unique index.
But aren't you at risk of getting the very error message you're trying
to avoid?
Yeah, looking further this was probably a thinko on my part. Thanks for
noticing. I've moved the test down so it's done right after the lock is
acquired. Revised patch attached.
This patch looks sane as far as it goes. It strikes me though that if
we're going to invent an opt_if_not_exists production in the grammar,
there are a lot of other places where it should be used too, for
consistency if nothing else.
However, it would be reasonable to do that mop-up as a separate
commit. If you prefer, commit what you've got and then I'll see
about the other thing.
regards, tom lane
On 09/20/2012 06:34 PM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
On 08/23/2012 07:39 AM, Magnus Hagander wrote:
It doesn't break, of course ,since it's protected by the unique index.
But aren't you at risk of getting the very error message you're trying
to avoid?Yeah, looking further this was probably a thinko on my part. Thanks for
noticing. I've moved the test down so it's done right after the lock is
acquired. Revised patch attached.This patch looks sane as far as it goes. It strikes me though that if
we're going to invent an opt_if_not_exists production in the grammar,
there are a lot of other places where it should be used too, for
consistency if nothing else.However, it would be reasonable to do that mop-up as a separate
commit. If you prefer, commit what you've got and then I'll see
about the other thing.
The enum piece is now committed.
I agree cleaning this up would be a good idea.
cheers
andrew
Andrew Dunstan <andrew@dunslane.net> writes:
The enum piece is now committed.
BTW, looking at that a second time ... the other CREATE IF NOT EXISTS
options we have issue a NOTICE when skipping the CREATE action. Is
there a reason this shouldn't do the same?
regards, tom lane
On 09/22/2012 05:39 PM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
The enum piece is now committed.
BTW, looking at that a second time ... the other CREATE IF NOT EXISTS
options we have issue a NOTICE when skipping the CREATE action. Is
there a reason this shouldn't do the same?
Not really, I guess we should for the sake of consistency, although TBH
I find it just useless noise and rather wish we hadn't started the trend
when we did the first DROP IF NOT EXISTS stuff.
I'll add it.
cheers
andrew
On 09/22/2012 11:49 PM, Andrew Dunstan wrote:
On 09/22/2012 05:39 PM, Tom Lane wrote:
Andrew Dunstan <andrew@dunslane.net> writes:
The enum piece is now committed.
BTW, looking at that a second time ... the other CREATE IF NOT EXISTS
options we have issue a NOTICE when skipping the CREATE action. Is
there a reason this shouldn't do the same?Not really, I guess we should for the sake of consistency, although TBH
I find it just useless noise and rather wish we hadn't started the
trend when we did the first DROP IF NOT EXISTS stuff.
Time for a GUC
existence_notice = none | exists | not_exists | all
?
Cheers,
Hannu Krosing
Andrew Dunstan <andrew@dunslane.net> writes:
On 09/22/2012 05:39 PM, Tom Lane wrote:
BTW, looking at that a second time ... the other CREATE IF NOT EXISTS
options we have issue a NOTICE when skipping the CREATE action. Is
there a reason this shouldn't do the same?
I'll add it.
I'm on it already.
regards, tom lane
Hannu Krosing <hannu@2ndQuadrant.com> writes:
On 09/22/2012 11:49 PM, Andrew Dunstan wrote:
Not really, I guess we should for the sake of consistency, although TBH
I find it just useless noise and rather wish we hadn't started the
trend when we did the first DROP IF NOT EXISTS stuff.
Time for a GUC
existence_notice = none | exists | not_exists | all
Not another one :-( ... isn't client_min_messages good enough?
We sort of had this discussion before w.r.t. the notices about creating
primary key indexes etc. I wonder whether we should make a formal
effort to split NOTICE message level into, say, NOTICE and NOVICE
levels, where the latter contains all the "training wheels" stuff that
experienced users would really rather not see. Or maybe just redefine
NOTICE as meaning novice-oriented messages, and push anything that
doesn't seem to fit that categorization into another existing message
level?
regards, tom lane
I wrote:
... It strikes me though that if
we're going to invent an opt_if_not_exists production in the grammar,
there are a lot of other places where it should be used too, for
consistency if nothing else.
BTW, I tried to do this and realized that it doesn't work, because IF
is not a reserved word. The only way that opt_if_not_exists isn't
ambiguous is if it must appear before something that's not an
identifier, which is to say it works in ALTER TYPE ADD VALUE ... Sconst
and nowhere else. Otherwise you have to spell it out with duplicate
productions so that bison doesn't have to make a shift/reduce decision
till it's seen the whole phrase.
If we're ever forced to make IF reserved for other reasons, we could
clean up a lot of both IF EXISTS and IF NOT EXISTS productions.
There are other ways we could refactor the productions involved to
reduce duplication; for instance I think that we could make it work for
CREATE TABLE IF NOT EXISTS by defining a nonterminal that expands to
either "qualified_name" or "IF NOT EXISTS qualified_name". But that
seems ugly enough to not be much of an improvement, not least because
the nonterminal would need to return two separate pieces of info,
and that's not terribly easy in bison.
regards, tom lane
On 09/22/2012 07:05 PM, Tom Lane wrote:
I wrote:
... It strikes me though that if
we're going to invent an opt_if_not_exists production in the grammar,
there are a lot of other places where it should be used too, for
consistency if nothing else.BTW, I tried to do this and realized that it doesn't work, because IF
is not a reserved word. The only way that opt_if_not_exists isn't
ambiguous is if it must appear before something that's not an
identifier, which is to say it works in ALTER TYPE ADD VALUE ... Sconst
and nowhere else. Otherwise you have to spell it out with duplicate
productions so that bison doesn't have to make a shift/reduce decision
till it's seen the whole phrase.If we're ever forced to make IF reserved for other reasons, we could
clean up a lot of both IF EXISTS and IF NOT EXISTS productions.There are other ways we could refactor the productions involved to
reduce duplication; for instance I think that we could make it work for
CREATE TABLE IF NOT EXISTS by defining a nonterminal that expands to
either "qualified_name" or "IF NOT EXISTS qualified_name". But that
seems ugly enough to not be much of an improvement, not least because
the nonterminal would need to return two separate pieces of info,
and that's not terribly easy in bison.
:-(
I remember running into this when I did the DINE stuff. I was actually
pleasantly surprised that it worked with the enum command.
cheers
andrew
On Sat, Sep 22, 2012 at 06:08:19PM -0400, Tom Lane wrote:
Hannu Krosing <hannu@2ndQuadrant.com> writes:
On 09/22/2012 11:49 PM, Andrew Dunstan wrote:
Not really, I guess we should for the sake of consistency, although TBH
I find it just useless noise and rather wish we hadn't started the
trend when we did the first DROP IF NOT EXISTS stuff.Time for a GUC
existence_notice = none | exists | not_exists | allNot another one :-( ... isn't client_min_messages good enough?
We sort of had this discussion before w.r.t. the notices about creating
primary key indexes etc. I wonder whether we should make a formal
effort to split NOTICE message level into, say, NOTICE and NOVICE
levels, where the latter contains all the "training wheels" stuff that
experienced users would really rather not see. Or maybe just redefine
NOTICE as meaning novice-oriented messages, and push anything that
doesn't seem to fit that categorization into another existing message
level?
I have always wanted a "novice" level, so we could warn about things
like unjoined tables.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +