On 08/23/2012 07:39 AM, Magnus Hagander wrote:
On Thu, Aug 23, 2012 at 1:35 PM, Andrew Dunstan <and...@dunslane.net> wrote:
On 08/23/2012 06:47 AM, Magnus Hagander wrote:
On Mon, Aug 20, 2012 at 4:52 PM, Andrew Dunstan <and...@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
*** 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
--
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
