Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitaf1d395

Browse files
committed
Allow more cases to pass the unsafe-use-of-new-enum-value restriction.
Up to now we've rejected cases likeBEGIN;CREATE TYPE rainbow AS ENUM ();ALTER TYPE rainbow ADD VALUE 'red';-- use the value 'red', perhaps in a constraint or indexCOMMIT;The concern is that the uncommitted enum value 'red' might get intoan index and then break the index if we roll back the ALTER ADD.If the ALTER is in the same transaction as the CREATE then it's reallyperfectly safe, but we weren't taking the trouble to identify that.pg_dump in binary-upgrade mode will emit enum definitions that looklike the above, which up to now didn't fall foul of the unsafe-usagecheck because we processed each restore command as a separatetransaction. However an upcoming patch proposes to bundle the restorecommands into large transactions to reduce XID consumption duringpg_upgrade, and that makes this behavior a problem.To fix, remember the OIDs of enum types created in the currenttransaction, and allow use of enum values that are added to one laterin the same transaction. To do this fully correctly in the presenceof subtransactions, we'd have to track subtransaction nesting level ofthe CREATE and do maintenance work at every subsequent subtransactionexit. That seems expensive, and we don't need it to satisfy pg_dump'susage. Hence, apply the additional optimization only when the CREATEand ALTER are at outermost transaction level.Patch by me, reviewed by Andrew DunstanDiscussion:https://postgr.es/m/1548468.1711220438@sss.pgh.pa.us
1 parentd37e0d0 commitaf1d395

File tree

4 files changed

+181
-63
lines changed

4 files changed

+181
-63
lines changed

‎src/backend/catalog/pg_enum.c

Lines changed: 162 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -36,17 +36,35 @@
3636
Oidbinary_upgrade_next_pg_enum_oid=InvalidOid;
3737

3838
/*
39-
* Hash table of enum value OIDs created during the current transaction by
40-
* AddEnumLabel. We disallow using these values until the transaction is
39+
* We keep two transaction-lifespan hash tables, one containing the OIDs
40+
* of enum types made in the current transaction, and one containing the
41+
* OIDs of enum values created during the current transaction by
42+
* AddEnumLabel (but only if their enum type is not in the first hash).
43+
*
44+
* We disallow using enum values in the second hash until the transaction is
4145
* committed; otherwise, they might get into indexes where we can't clean
4246
* them up, and then if the transaction rolls back we have a broken index.
4347
* (See comments for check_safe_enum_use() in enum.c.) Values created by
4448
* EnumValuesCreate are *not* entered into the table; we assume those are
4549
* created during CREATE TYPE, so they can't go away unless the enum type
4650
* itself does.
51+
*
52+
* The motivation for treating enum values as safe if their type OID is
53+
* in the first hash is to allow CREATE TYPE AS ENUM; ALTER TYPE ADD VALUE;
54+
* followed by a use of the value in the same transaction. This pattern
55+
* is really just as safe as creating the value during CREATE TYPE.
56+
* We need to support this because pg_dump in binary upgrade mode produces
57+
* commands like that. But currently we only support it when the commands
58+
* are at the outermost transaction level, which is as much as we need for
59+
* pg_dump. We could track subtransaction nesting of the commands to
60+
* analyze things more precisely, but for now we don't bother.
4761
*/
48-
staticHTAB*uncommitted_enums=NULL;
62+
staticHTAB*uncommitted_enum_types=NULL;
63+
staticHTAB*uncommitted_enum_values=NULL;
4964

65+
staticvoidinit_uncommitted_enum_types(void);
66+
staticvoidinit_uncommitted_enum_values(void);
67+
staticboolEnumTypeUncommitted(Oidtyp_id);
5068
staticvoidRenumberEnumType(Relationpg_enum,HeapTuple*existing,intnelems);
5169
staticintsort_order_cmp(constvoid*p1,constvoid*p2);
5270

@@ -56,6 +74,11 @@ static intsort_order_cmp(const void *p1, const void *p2);
5674
*Create an entry in pg_enum for each of the supplied enum values.
5775
*
5876
* vals is a list of String values.
77+
*
78+
* We assume that this is called only by CREATE TYPE AS ENUM, and that it
79+
* will be called even if the vals list is empty. So we can enter the
80+
* enum type's OID into uncommitted_enum_types here, rather than needing
81+
* another entry point to do it.
5982
*/
6083
void
6184
EnumValuesCreate(OidenumTypeOid,List*vals)
@@ -70,6 +93,21 @@ EnumValuesCreate(Oid enumTypeOid, List *vals)
7093
CatalogIndexStateindstate;
7194
TupleTableSlot**slot;
7295

96+
/*
97+
* Remember the type OID as being made in the current transaction, but not
98+
* if we're in a subtransaction. (We could remember the OID anyway, in
99+
* case a subsequent ALTER ADD VALUE occurs at outer level. But that
100+
* usage pattern seems unlikely enough that we'd probably just be wasting
101+
* hashtable maintenance effort.)
102+
*/
103+
if (GetCurrentTransactionNestLevel()==1)
104+
{
105+
if (uncommitted_enum_types==NULL)
106+
init_uncommitted_enum_types();
107+
(void)hash_search(uncommitted_enum_types,&enumTypeOid,
108+
HASH_ENTER,NULL);
109+
}
110+
73111
num_elems=list_length(vals);
74112

75113
/*
@@ -211,20 +249,37 @@ EnumValuesDelete(Oid enumTypeOid)
211249
}
212250

213251
/*
214-
* Initialize the uncommitted enum table for this transaction.
252+
* Initialize the uncommitted enum types table for this transaction.
253+
*/
254+
staticvoid
255+
init_uncommitted_enum_types(void)
256+
{
257+
HASHCTLhash_ctl;
258+
259+
hash_ctl.keysize=sizeof(Oid);
260+
hash_ctl.entrysize=sizeof(Oid);
261+
hash_ctl.hcxt=TopTransactionContext;
262+
uncommitted_enum_types=hash_create("Uncommitted enum types",
263+
32,
264+
&hash_ctl,
265+
HASH_ELEM |HASH_BLOBS |HASH_CONTEXT);
266+
}
267+
268+
/*
269+
* Initialize the uncommitted enum values table for this transaction.
215270
*/
216271
staticvoid
217-
init_uncommitted_enums(void)
272+
init_uncommitted_enum_values(void)
218273
{
219274
HASHCTLhash_ctl;
220275

221276
hash_ctl.keysize=sizeof(Oid);
222277
hash_ctl.entrysize=sizeof(Oid);
223278
hash_ctl.hcxt=TopTransactionContext;
224-
uncommitted_enums=hash_create("Uncommittedenums",
225-
32,
226-
&hash_ctl,
227-
HASH_ELEM |HASH_BLOBS |HASH_CONTEXT);
279+
uncommitted_enum_values=hash_create("Uncommittedenum values",
280+
32,
281+
&hash_ctl,
282+
HASH_ELEM |HASH_BLOBS |HASH_CONTEXT);
228283
}
229284

230285
/*
@@ -520,12 +575,27 @@ AddEnumLabel(Oid enumTypeOid,
520575

521576
table_close(pg_enum,RowExclusiveLock);
522577

523-
/* Set up the uncommitted enum table if not already done in this tx */
524-
if (uncommitted_enums==NULL)
525-
init_uncommitted_enums();
578+
/*
579+
* If the enum type itself is uncommitted, we need not enter the new enum
580+
* value into uncommitted_enum_values, because the type won't survive if
581+
* the value doesn't. (This is basically the same reasoning as for values
582+
* made directly by CREATE TYPE AS ENUM.) However, apply this rule only
583+
* when we are not inside a subtransaction; if we're more deeply nested
584+
* than the CREATE TYPE then the conclusion doesn't hold. We could expend
585+
* more effort to track the subtransaction level of CREATE TYPE, but for
586+
* now we're only concerned about making the world safe for pg_dump in
587+
* binary upgrade mode, and that won't use subtransactions.
588+
*/
589+
if (GetCurrentTransactionNestLevel()==1&&
590+
EnumTypeUncommitted(enumTypeOid))
591+
return;
592+
593+
/* Set up the uncommitted values table if not already done in this tx */
594+
if (uncommitted_enum_values==NULL)
595+
init_uncommitted_enum_values();
526596

527597
/* Add the new value to the table */
528-
(void)hash_search(uncommitted_enums,&newOid,HASH_ENTER,NULL);
598+
(void)hash_search(uncommitted_enum_values,&newOid,HASH_ENTER,NULL);
529599
}
530600

531601

@@ -614,19 +684,37 @@ RenameEnumLabel(Oid enumTypeOid,
614684

615685

616686
/*
617-
* Test if the given enum value is in the table of uncommitted enums.
687+
* Test if the given type OID is in the table of uncommitted enum types.
688+
*/
689+
staticbool
690+
EnumTypeUncommitted(Oidtyp_id)
691+
{
692+
boolfound;
693+
694+
/* If we've made no uncommitted types table, it's not in the table */
695+
if (uncommitted_enum_types==NULL)
696+
return false;
697+
698+
/* Else, is it in the table? */
699+
(void)hash_search(uncommitted_enum_types,&typ_id,HASH_FIND,&found);
700+
returnfound;
701+
}
702+
703+
704+
/*
705+
* Test if the given enum value is in the table of uncommitted enum values.
618706
*/
619707
bool
620708
EnumUncommitted(Oidenum_id)
621709
{
622710
boolfound;
623711

624-
/* If we've made no uncommitted table,all values are safe */
625-
if (uncommitted_enums==NULL)
712+
/* If we've made no uncommittedvaluestable,it's not in the table */
713+
if (uncommitted_enum_values==NULL)
626714
return false;
627715

628716
/* Else, is it in the table? */
629-
(void)hash_search(uncommitted_enums,&enum_id,HASH_FIND,&found);
717+
(void)hash_search(uncommitted_enum_values,&enum_id,HASH_FIND,&found);
630718
returnfound;
631719
}
632720

@@ -638,11 +726,12 @@ void
638726
AtEOXact_Enum(void)
639727
{
640728
/*
641-
* Reset the uncommittedtable, as all ourenum valuesare now committed.
642-
*Thememory will go away automatically when TopTransactionContext is
643-
*freed;it's sufficient to clear ourpointer.
729+
* Reset the uncommittedtables, as all ourtuplesare now committed. The
730+
* memory will go away automatically when TopTransactionContext is freed;
731+
* it's sufficient to clear ourpointers.
644732
*/
645-
uncommitted_enums=NULL;
733+
uncommitted_enum_types=NULL;
734+
uncommitted_enum_values=NULL;
646735
}
647736

648737

@@ -723,15 +812,15 @@ sort_order_cmp(const void *p1, const void *p2)
723812
Size
724813
EstimateUncommittedEnumsSpace(void)
725814
{
726-
size_tentries;
815+
size_tentries=0;
727816

728-
if (uncommitted_enums)
729-
entries=hash_get_num_entries(uncommitted_enums);
730-
else
731-
entries=0;
817+
if (uncommitted_enum_types)
818+
entries+=hash_get_num_entries(uncommitted_enum_types);
819+
if (uncommitted_enum_values)
820+
entries+=hash_get_num_entries(uncommitted_enum_values);
732821

733-
/* Addone for theterminator. */
734-
returnsizeof(Oid)* (entries+1);
822+
/* Addtwo for theterminators. */
823+
returnsizeof(Oid)* (entries+2);
735824
}
736825

737826
void
@@ -740,51 +829,78 @@ SerializeUncommittedEnums(void *space, Size size)
740829
Oid*serialized= (Oid*)space;
741830

742831
/*
743-
* Make sure the hashtable hasn't changed in size since the caller
832+
* Make sure the hashtables haven't changed in size since the caller
744833
* reserved the space.
745834
*/
746835
Assert(size==EstimateUncommittedEnumsSpace());
747836

748-
/* Write out all thevalues from the hash table, if there is one. */
749-
if (uncommitted_enums)
837+
/* Write out all theOIDs from the types hash table, if there is one. */
838+
if (uncommitted_enum_types)
750839
{
751840
HASH_SEQ_STATUSstatus;
752841
Oid*value;
753842

754-
hash_seq_init(&status,uncommitted_enums);
843+
hash_seq_init(&status,uncommitted_enum_types);
755844
while ((value= (Oid*)hash_seq_search(&status)))
756845
*serialized++=*value;
757846
}
758847

759848
/* Write out the terminator. */
760-
*serialized=InvalidOid;
849+
*serialized++=InvalidOid;
850+
851+
/* Write out all the OIDs from the values hash table, if there is one. */
852+
if (uncommitted_enum_values)
853+
{
854+
HASH_SEQ_STATUSstatus;
855+
Oid*value;
856+
857+
hash_seq_init(&status,uncommitted_enum_values);
858+
while ((value= (Oid*)hash_seq_search(&status)))
859+
*serialized++=*value;
860+
}
861+
862+
/* Write out the terminator. */
863+
*serialized++=InvalidOid;
761864

762865
/*
763866
* Make sure the amount of space we actually used matches what was
764867
* estimated.
765868
*/
766-
Assert((char*)(serialized+1)== ((char*)space)+size);
869+
Assert((char*)serialized== ((char*)space)+size);
767870
}
768871

769872
void
770873
RestoreUncommittedEnums(void*space)
771874
{
772875
Oid*serialized= (Oid*)space;
773876

774-
Assert(!uncommitted_enums);
877+
Assert(!uncommitted_enum_types);
878+
Assert(!uncommitted_enum_values);
775879

776880
/*
777-
*As a special case, if thelist is empty then don't even bother to
778-
*create the hashtable. This is theusual case, sinceenum alteration
779-
*is expected to be rare.
881+
*If eitherlist is empty then don't even bother to create that hash
882+
* table. This is thecommon case, sincemost transactions don't create
883+
*or alter enums.
780884
*/
781-
if (!OidIsValid(*serialized))
782-
return;
783-
784-
/* Read all the values into a new hash table. */
785-
init_uncommitted_enums();
786-
do
885+
if (OidIsValid(*serialized))
787886
{
788-
hash_search(uncommitted_enums,serialized++,HASH_ENTER,NULL);
789-
}while (OidIsValid(*serialized));
887+
/* Read all the types into a new hash table. */
888+
init_uncommitted_enum_types();
889+
do
890+
{
891+
(void)hash_search(uncommitted_enum_types,serialized++,
892+
HASH_ENTER,NULL);
893+
}while (OidIsValid(*serialized));
894+
}
895+
serialized++;
896+
if (OidIsValid(*serialized))
897+
{
898+
/* Read all the values into a new hash table. */
899+
init_uncommitted_enum_values();
900+
do
901+
{
902+
(void)hash_search(uncommitted_enum_values,serialized++,
903+
HASH_ENTER,NULL);
904+
}while (OidIsValid(*serialized));
905+
}
790906
}

‎src/backend/utils/adt/enum.c

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -49,11 +49,12 @@ static ArrayType *enum_range_internal(Oid enumtypoid, Oid lower, Oid upper);
4949
* We don't implement that fully right now, but we do allow free use of enum
5050
* values created during CREATE TYPE AS ENUM, which are surely of the same
5151
* lifespan as the enum type. (This case is required by "pg_restore -1".)
52-
* Values added by ALTER TYPE ADD VALUE are currently restricted, but could
53-
* be allowed if the enum type could be proven to have been created earlier
54-
* in the same transaction. (Note that comparing tuple xmins would not work
55-
* for that, because the type tuple might have been updated in the current
56-
* transaction. Subtransactions also create hazards to be accounted for.)
52+
* Values added by ALTER TYPE ADD VALUE are also allowed if the enum type
53+
* is known to have been created earlier in the same transaction. (Note that
54+
* we have to track that explicitly; comparing tuple xmins is insufficient,
55+
* because the type tuple might have been updated in the current transaction.
56+
* Subtransactions also create hazards to be accounted for; currently,
57+
* pg_enum.c only handles ADD VALUE at the outermost transaction level.)
5758
*
5859
* This function needs to be called (directly or indirectly) in any of the
5960
* functions below that could return an enum value to SQL operations.
@@ -81,10 +82,10 @@ check_safe_enum_use(HeapTuple enumval_tup)
8182
return;
8283

8384
/*
84-
* Check if the enum value is uncommitted. If not, it's safe, because it
85-
*was made during CREATE TYPE AS ENUM andcan't be shorter-lived than its
86-
*owning type. (This'd alsobe false for values made by other
87-
*transactions; but the previous testsshould have handled all of those.)
85+
* Check if the enum value islisted asuncommitted. If not, it's safe,
86+
*because itcan't be shorter-lived than its owning type. (This'd also
87+
* be false for values made by other transactions; but the previous tests
88+
* should have handled all of those.)
8889
*/
8990
if (!EnumUncommitted(en->oid))
9091
return;

‎src/test/regress/expected/enum.out

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -684,16 +684,18 @@ select enum_range(null::bogon);
684684
(1 row)
685685

686686
ROLLBACK;
687-
-- ideally, we'd allow this usage; but it requires keeping track of whether
688-
-- the enum type was created in the current transaction, which is expensive
687+
-- we must allow this usage to support pg_dump in binary upgrade mode
689688
BEGIN;
690689
CREATE TYPE bogus AS ENUM('good');
691690
ALTER TYPE bogus RENAME TO bogon;
692691
ALTER TYPE bogon ADD VALUE 'bad';
693692
ALTER TYPE bogon ADD VALUE 'ugly';
694-
select enum_range(null::bogon); -- fails
695-
ERROR: unsafe use of new value "bad" of enum type bogon
696-
HINT: New enum values must be committed before they can be used.
693+
select enum_range(null::bogon);
694+
enum_range
695+
-----------------
696+
{good,bad,ugly}
697+
(1 row)
698+
697699
ROLLBACK;
698700
--
699701
-- Cleanup

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp