Andrew Dunstan <and...@dunslane.net> writes:
> OK, did that. Here is a patch that is undocumented but I think is
> otherwise complete. It's been tested a bit and we haven't been able to
> break it. Comments welcome.
Got around to looking at this. Attached is a revised version that I think
is in committable shape. The main non-cosmetic change is that the test
for "type was created in same transaction as new value" now consists of
comparing the xmins of the pg_type and pg_enum rows, without consulting
GetCurrentTransactionId(). I did not like the original coding because
it would pointlessly disallow references to enum values that were created
in a parent transaction of the current subxact. This way is still leaving
some subxact use-cases on the table, as noted in the code comments, but
it's more flexible than before.
Barring objections I'll push this soon.
regards, tom lane
diff --git a/doc/src/sgml/ref/alter_type.sgml b/doc/src/sgml/ref/alter_type.sgml
index 9789881..aec73f6 100644
*** a/doc/src/sgml/ref/alter_type.sgml
--- b/doc/src/sgml/ref/alter_type.sgml
*************** ALTER TYPE <replaceable class="PARAMETER
*** 266,273 ****
<title>Notes</title>
<para>
! <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to an
! enum type) cannot be executed inside a transaction block.
</para>
<para>
--- 266,275 ----
<title>Notes</title>
<para>
! If <command>ALTER TYPE ... ADD VALUE</> (the form that adds a new value to
! an enum type) is executed inside a transaction block, the new value cannot
! be used until after the transaction has been committed, except in the case
! that the enum type itself was created earlier in the same transaction.
</para>
<para>
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index ce04211..8e7be78 100644
*** a/src/backend/commands/typecmds.c
--- b/src/backend/commands/typecmds.c
*************** DefineEnum(CreateEnumStmt *stmt)
*** 1221,1227 ****
* Adds a new label to an existing enum.
*/
ObjectAddress
! AlterEnum(AlterEnumStmt *stmt, bool isTopLevel)
{
Oid enum_type_oid;
TypeName *typename;
--- 1221,1227 ----
* Adds a new label to an existing enum.
*/
ObjectAddress
! AlterEnum(AlterEnumStmt *stmt)
{
Oid enum_type_oid;
TypeName *typename;
*************** AlterEnum(AlterEnumStmt *stmt, bool isTo
*** 1236,1260 ****
if (!HeapTupleIsValid(tup))
elog(ERROR, "cache lookup failed for type %u", enum_type_oid);
- /*
- * Ordinarily we disallow adding values within transaction blocks, because
- * we can't cope with enum OID values getting into indexes and then having
- * their defining pg_enum entries go away. However, it's okay if the enum
- * type was created in the current transaction, since then there can be no
- * such indexes that wouldn't themselves go away on rollback. (We support
- * this case because pg_dump --binary-upgrade needs it.) We test this by
- * seeing if the pg_type row has xmin == current XID and is not
- * HEAP_UPDATED. If it is HEAP_UPDATED, we can't be sure whether the type
- * was created or only modified in this xact. So we are disallowing some
- * cases that could theoretically be safe; but fortunately pg_dump only
- * needs the simplest case.
- */
- if (HeapTupleHeaderGetXmin(tup->t_data) == GetCurrentTransactionId() &&
- !(tup->t_data->t_infomask & HEAP_UPDATED))
- /* safe to do inside transaction block */ ;
- else
- PreventTransactionChain(isTopLevel, "ALTER TYPE ... ADD");
-
/* Check it's an enum and check user has permission to ALTER the enum */
checkEnumOwner(tup);
--- 1236,1241 ----
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index ac50c2a..ac64135 100644
*** a/src/backend/tcop/utility.c
--- b/src/backend/tcop/utility.c
*************** ProcessUtilitySlow(Node *parsetree,
*** 1359,1365 ****
break;
case T_AlterEnumStmt: /* ALTER TYPE (enum) */
! address = AlterEnum((AlterEnumStmt *) parsetree, isTopLevel);
break;
case T_ViewStmt: /* CREATE VIEW */
--- 1359,1365 ----
break;
case T_AlterEnumStmt: /* ALTER TYPE (enum) */
! address = AlterEnum((AlterEnumStmt *) parsetree);
break;
case T_ViewStmt: /* CREATE VIEW */
diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
index 135a544..47d5355 100644
*** a/src/backend/utils/adt/enum.c
--- b/src/backend/utils/adt/enum.c
***************
*** 19,24 ****
--- 19,25 ----
#include "catalog/indexing.h"
#include "catalog/pg_enum.h"
#include "libpq/pqformat.h"
+ #include "storage/procarray.h"
#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
*************** static Oid enum_endpoint(Oid enumtypoid,
*** 31,36 ****
--- 32,124 ----
static ArrayType *enum_range_internal(Oid enumtypoid, Oid lower, Oid upper);
+ /*
+ * Disallow use of an uncommitted pg_enum tuple.
+ *
+ * We need to make sure that uncommitted enum values don't get into indexes.
+ * If they did, and if we then rolled back the pg_enum addition, we'd have
+ * broken the index because value comparisons will not work reliably without
+ * an underlying pg_enum entry. (Note that removal of the heap entry
+ * containing an enum value is not sufficient to ensure that it doesn't appear
+ * in upper levels of indexes.) To do this we prevent an uncommitted row from
+ * being used for any SQL-level purpose. This is stronger than necessary,
+ * since the value might not be getting inserted into a table or there might
+ * be no index on its column, but it's easy to enforce centrally.
+ *
+ * However, it's okay to allow use of uncommitted values belonging to enum
+ * types that were themselves created in the same transaction, because then
+ * any such index would also be new and would go away altogether on rollback.
+ * (This case is required by pg_upgrade.)
+ *
+ * This function needs to be called (directly or indirectly) in any of the
+ * functions below that could return an enum value to SQL operations.
+ */
+ static void
+ check_safe_enum_use(HeapTuple enumval_tup)
+ {
+ TransactionId xmin;
+ Form_pg_enum en;
+ HeapTuple enumtyp_tup;
+
+ /*
+ * If the row is hinted as committed, it's surely safe. This provides a
+ * fast path for all normal use-cases.
+ */
+ if (HeapTupleHeaderXminCommitted(enumval_tup->t_data))
+ return;
+
+ /*
+ * Usually, a row would get hinted as committed when it's read or loaded
+ * into syscache; but just in case not, let's check the xmin directly.
+ */
+ xmin = HeapTupleHeaderGetXmin(enumval_tup->t_data);
+ if (!TransactionIdIsInProgress(xmin) &&
+ TransactionIdDidCommit(xmin))
+ return;
+
+ /* It is a new enum value, so check to see if the whole enum is new */
+ en = (Form_pg_enum) GETSTRUCT(enumval_tup);
+ enumtyp_tup = SearchSysCache1(TYPEOID, ObjectIdGetDatum(en->enumtypid));
+ if (!HeapTupleIsValid(enumtyp_tup))
+ elog(ERROR, "cache lookup failed for type %u", en->enumtypid);
+
+ /*
+ * We insist that the type have been created in the same (sub)transaction
+ * as the enum value. It would be safe to allow the type's originating
+ * xact to be a subcommitted child of the enum value's xact, but not vice
+ * versa (since we might now be in a subxact of the type's originating
+ * xact, which could roll back along with the enum value's subxact). The
+ * former case seems a sufficiently weird usage pattern as to not be worth
+ * spending code for, so we're left with a simple equality check.
+ *
+ * We also insist that the type's pg_type row not be HEAP_UPDATED. If it
+ * is, we can't tell whether the row was created or only modified in the
+ * apparent originating xact, so it might be older than that xact. (We do
+ * not worry whether the enum value is HEAP_UPDATED; if it is, we might
+ * think it's too new and throw an unnecessary error, but we won't allow
+ * an unsafe case.)
+ */
+ if (xmin == HeapTupleHeaderGetXmin(enumtyp_tup->t_data) &&
+ !(enumtyp_tup->t_data->t_infomask & HEAP_UPDATED))
+ {
+ /* same (sub)transaction, so safe */
+ ReleaseSysCache(enumtyp_tup);
+ return;
+ }
+
+ /*
+ * There might well be other tests we could do here to narrow down the
+ * unsafe conditions, but for now just raise an exception.
+ */
+ ereport(ERROR,
+ (errcode(ERRCODE_UNSAFE_NEW_ENUM_VALUE_USAGE),
+ errmsg("unsafe use of new value \"%s\" of enum type %s",
+ NameStr(en->enumlabel),
+ format_type_be(en->enumtypid)),
+ errhint("New enum values must be committed before they can be used.")));
+ }
+
+
/* Basic I/O support */
Datum
*************** enum_in(PG_FUNCTION_ARGS)
*** 59,64 ****
--- 147,155 ----
format_type_be(enumtypoid),
name)));
+ /* check it's safe to use in SQL */
+ check_safe_enum_use(tup);
+
/*
* This comes from pg_enum.oid and stores system oids in user tables. This
* oid must be preserved by binary upgrades.
*************** enum_recv(PG_FUNCTION_ARGS)
*** 124,129 ****
--- 215,223 ----
format_type_be(enumtypoid),
name)));
+ /* check it's safe to use in SQL */
+ check_safe_enum_use(tup);
+
enumoid = HeapTupleGetOid(tup);
ReleaseSysCache(tup);
*************** enum_endpoint(Oid enumtypoid, ScanDirect
*** 327,335 ****
--- 421,436 ----
enum_tuple = systable_getnext_ordered(enum_scan, direction);
if (HeapTupleIsValid(enum_tuple))
+ {
+ /* check it's safe to use in SQL */
+ check_safe_enum_use(enum_tuple);
minmax = HeapTupleGetOid(enum_tuple);
+ }
else
+ {
+ /* should only happen with an empty enum */
minmax = InvalidOid;
+ }
systable_endscan_ordered(enum_scan);
index_close(enum_idx, AccessShareLock);
*************** enum_range_internal(Oid enumtypoid, Oid
*** 490,495 ****
--- 591,599 ----
if (left_found)
{
+ /* check it's safe to use in SQL */
+ check_safe_enum_use(enum_tuple);
+
if (cnt >= max)
{
max *= 2;
diff --git a/src/backend/utils/errcodes.txt b/src/backend/utils/errcodes.txt
index be924d5..e7bdb92 100644
*** a/src/backend/utils/errcodes.txt
--- b/src/backend/utils/errcodes.txt
*************** Section: Class 55 - Object Not In Prereq
*** 398,403 ****
--- 398,404 ----
55006 E ERRCODE_OBJECT_IN_USE object_in_use
55P02 E ERRCODE_CANT_CHANGE_RUNTIME_PARAM cant_change_runtime_param
55P03 E ERRCODE_LOCK_NOT_AVAILABLE lock_not_available
+ 55P04 E ERRCODE_UNSAFE_NEW_ENUM_VALUE_USAGE unsafe_new_enum_value_usage
Section: Class 57 - Operator Intervention
diff --git a/src/include/commands/typecmds.h b/src/include/commands/typecmds.h
index e4c86f1..847b770 100644
*** a/src/include/commands/typecmds.h
--- b/src/include/commands/typecmds.h
*************** extern void RemoveTypeById(Oid typeOid);
*** 26,32 ****
extern ObjectAddress DefineDomain(CreateDomainStmt *stmt);
extern ObjectAddress DefineEnum(CreateEnumStmt *stmt);
extern ObjectAddress DefineRange(CreateRangeStmt *stmt);
! extern ObjectAddress AlterEnum(AlterEnumStmt *stmt, bool isTopLevel);
extern ObjectAddress DefineCompositeType(RangeVar *typevar, List *coldeflist);
extern Oid AssignTypeArrayOid(void);
--- 26,32 ----
extern ObjectAddress DefineDomain(CreateDomainStmt *stmt);
extern ObjectAddress DefineEnum(CreateEnumStmt *stmt);
extern ObjectAddress DefineRange(CreateRangeStmt *stmt);
! extern ObjectAddress AlterEnum(AlterEnumStmt *stmt);
extern ObjectAddress DefineCompositeType(RangeVar *typevar, List *coldeflist);
extern Oid AssignTypeArrayOid(void);
diff --git a/src/test/regress/expected/enum.out b/src/test/regress/expected/enum.out
index 1a61a5b..d4a45a3 100644
*** a/src/test/regress/expected/enum.out
--- b/src/test/regress/expected/enum.out
*************** DROP TYPE bogus;
*** 560,584 ****
-- check transactional behaviour of ALTER TYPE ... ADD VALUE
--
CREATE TYPE bogus AS ENUM('good');
! -- check that we can't add new values to existing enums in a transaction
BEGIN;
! ALTER TYPE bogus ADD VALUE 'bad';
! ERROR: ALTER TYPE ... ADD cannot run inside a transaction block
COMMIT;
-- check that we recognize the case where the enum already existed but was
! -- modified in the current txn
BEGIN;
ALTER TYPE bogus RENAME TO bogon;
ALTER TYPE bogon ADD VALUE 'bad';
! ERROR: ALTER TYPE ... ADD cannot run inside a transaction block
ROLLBACK;
DROP TYPE bogus;
! -- check that we *can* add new values to existing enums in a transaction,
! -- if the type is new as well
BEGIN;
! CREATE TYPE bogus AS ENUM();
! ALTER TYPE bogus ADD VALUE 'good';
ALTER TYPE bogus ADD VALUE 'ugly';
ROLLBACK;
--
-- Cleanup
--- 560,631 ----
-- check transactional behaviour of ALTER TYPE ... ADD VALUE
--
CREATE TYPE bogus AS ENUM('good');
! -- check that we can add new values to existing enums in a transaction
! -- but we can't use them
BEGIN;
! ALTER TYPE bogus ADD VALUE 'new';
! SAVEPOINT x;
! SELECT 'new'::bogus; -- unsafe
! ERROR: unsafe use of new value "new" of enum type bogus
! LINE 1: SELECT 'new'::bogus;
! ^
! HINT: New enum values must be committed before they can be used.
! ROLLBACK TO x;
! SELECT enum_first(null::bogus); -- safe
! enum_first
! ------------
! good
! (1 row)
!
! SELECT enum_last(null::bogus); -- unsafe
! ERROR: unsafe use of new value "new" of enum type bogus
! HINT: New enum values must be committed before they can be used.
! ROLLBACK TO x;
! SELECT enum_range(null::bogus); -- unsafe
! ERROR: unsafe use of new value "new" of enum type bogus
! HINT: New enum values must be committed before they can be used.
! ROLLBACK TO x;
COMMIT;
+ SELECT 'new'::bogus; -- now safe
+ bogus
+ -------
+ new
+ (1 row)
+
+ SELECT enumlabel, enumsortorder
+ FROM pg_enum
+ WHERE enumtypid = 'bogus'::regtype
+ ORDER BY 2;
+ enumlabel | enumsortorder
+ -----------+---------------
+ good | 1
+ new | 2
+ (2 rows)
+
-- check that we recognize the case where the enum already existed but was
! -- modified in the current txn; this should not be considered safe
BEGIN;
ALTER TYPE bogus RENAME TO bogon;
ALTER TYPE bogon ADD VALUE 'bad';
! SELECT 'bad'::bogon;
! ERROR: unsafe use of new value "bad" of enum type bogon
! LINE 1: SELECT 'bad'::bogon;
! ^
! HINT: New enum values must be committed before they can be used.
ROLLBACK;
DROP TYPE bogus;
! -- check that we can add new values to existing enums in a transaction
! -- and use them, if the type is new as well
BEGIN;
! CREATE TYPE bogus AS ENUM('good');
! ALTER TYPE bogus ADD VALUE 'bad';
ALTER TYPE bogus ADD VALUE 'ugly';
+ SELECT enum_range(null::bogus);
+ enum_range
+ -----------------
+ {good,bad,ugly}
+ (1 row)
+
ROLLBACK;
--
-- Cleanup
diff --git a/src/test/regress/sql/enum.sql b/src/test/regress/sql/enum.sql
index 88a835e..d25e8de 100644
*** a/src/test/regress/sql/enum.sql
--- b/src/test/regress/sql/enum.sql
*************** DROP TYPE bogus;
*** 262,287 ****
--
CREATE TYPE bogus AS ENUM('good');
! -- check that we can't add new values to existing enums in a transaction
BEGIN;
! ALTER TYPE bogus ADD VALUE 'bad';
COMMIT;
-- check that we recognize the case where the enum already existed but was
! -- modified in the current txn
BEGIN;
ALTER TYPE bogus RENAME TO bogon;
ALTER TYPE bogon ADD VALUE 'bad';
ROLLBACK;
DROP TYPE bogus;
! -- check that we *can* add new values to existing enums in a transaction,
! -- if the type is new as well
BEGIN;
! CREATE TYPE bogus AS ENUM();
! ALTER TYPE bogus ADD VALUE 'good';
ALTER TYPE bogus ADD VALUE 'ugly';
ROLLBACK;
--
--- 262,303 ----
--
CREATE TYPE bogus AS ENUM('good');
! -- check that we can add new values to existing enums in a transaction
! -- but we can't use them
BEGIN;
! ALTER TYPE bogus ADD VALUE 'new';
! SAVEPOINT x;
! SELECT 'new'::bogus; -- unsafe
! ROLLBACK TO x;
! SELECT enum_first(null::bogus); -- safe
! SELECT enum_last(null::bogus); -- unsafe
! ROLLBACK TO x;
! SELECT enum_range(null::bogus); -- unsafe
! ROLLBACK TO x;
COMMIT;
+ SELECT 'new'::bogus; -- now safe
+ SELECT enumlabel, enumsortorder
+ FROM pg_enum
+ WHERE enumtypid = 'bogus'::regtype
+ ORDER BY 2;
-- check that we recognize the case where the enum already existed but was
! -- modified in the current txn; this should not be considered safe
BEGIN;
ALTER TYPE bogus RENAME TO bogon;
ALTER TYPE bogon ADD VALUE 'bad';
+ SELECT 'bad'::bogon;
ROLLBACK;
DROP TYPE bogus;
! -- check that we can add new values to existing enums in a transaction
! -- and use them, if the type is new as well
BEGIN;
! CREATE TYPE bogus AS ENUM('good');
! ALTER TYPE bogus ADD VALUE 'bad';
ALTER TYPE bogus ADD VALUE 'ugly';
+ SELECT enum_range(null::bogus);
ROLLBACK;
--
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
