Skip to content

Commit aac07b5

Browse files
tglsfdctglsfdc
committed
Fix broken handling of domains in atthasmissing logic.
If a domain type has a default, adding a column of that type (without any explicit DEFAULT clause) failed to install the domain's default value in existing rows, instead leaving the new column null. This is unexpected, and it used to work correctly before v11. The cause is confusion in the atthasmissing mechanism about which default value to install: we'd only consider installing an explicitly-specified default, and then we'd decide that no table rewrite is needed. To fix, take the responsibility for filling attmissingval out of StoreAttrDefault, and instead put it into ATExecAddColumn's existing logic that derives the correct value to fill the new column with. Also, centralize the logic that determines the need for default-related table rewriting there, instead of spreading it over four or five places. In the back branches, we'll leave the attmissingval-filling code in StoreAttrDefault even though it's now dead, for fear that some extension may be depending on that functionality to exist there. A separate HEAD-only patch will clean up the now-useless code. Reported-by: jian he <jian.universality@gmail.com> Author: jian he <jian.universality@gmail.com> Author: Tom Lane <tgl@sss.pgh.pa.us> Discussion: https://postgr.es/m/CACJufxHFssPvkP1we7WMhPD_1kwgbG52o=kQgL+TnVoX5LOyCQ@mail.gmail.com Backpatch-through: 13
1 parent ebe919e commit aac07b5

File tree

5 files changed

+233
-35
lines changed

5 files changed

+233
-35
lines changed

‎src/backend/catalog/heap.c

Lines changed: 62 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2105,6 +2105,60 @@ RelationClearMissing(Relation rel)
21052105
table_close(attr_rel, RowExclusiveLock);
21062106
}
21072107

2108+
/*
2109+
* StoreAttrMissingVal
2110+
*
2111+
* Set the missing value of a single attribute.
2112+
*/
2113+
void
2114+
StoreAttrMissingVal(Relation rel, AttrNumber attnum, Datum missingval)
2115+
{
2116+
Datum valuesAtt[Natts_pg_attribute] = {0};
2117+
bool nullsAtt[Natts_pg_attribute] = {0};
2118+
bool replacesAtt[Natts_pg_attribute] = {0};
2119+
Relation attrrel;
2120+
Form_pg_attribute attStruct;
2121+
HeapTuple atttup,
2122+
newtup;
2123+
2124+
/* This is only supported for plain tables */
2125+
Assert(rel->rd_rel->relkind == RELKIND_RELATION);
2126+
2127+
/* Fetch the pg_attribute row */
2128+
attrrel = table_open(AttributeRelationId, RowExclusiveLock);
2129+
2130+
atttup = SearchSysCache2(ATTNUM,
2131+
ObjectIdGetDatum(RelationGetRelid(rel)),
2132+
Int16GetDatum(attnum));
2133+
if (!HeapTupleIsValid(atttup)) /* shouldn't happen */
2134+
elog(ERROR, "cache lookup failed for attribute %d of relation %u",
2135+
attnum, RelationGetRelid(rel));
2136+
attStruct = (Form_pg_attribute) GETSTRUCT(atttup);
2137+
2138+
/* Make a one-element array containing the value */
2139+
missingval = PointerGetDatum(construct_array(&missingval,
2140+
1,
2141+
attStruct->atttypid,
2142+
attStruct->attlen,
2143+
attStruct->attbyval,
2144+
attStruct->attalign));
2145+
2146+
/* Update the pg_attribute row */
2147+
valuesAtt[Anum_pg_attribute_atthasmissing - 1] = BoolGetDatum(true);
2148+
replacesAtt[Anum_pg_attribute_atthasmissing - 1] = true;
2149+
2150+
valuesAtt[Anum_pg_attribute_attmissingval - 1] = missingval;
2151+
replacesAtt[Anum_pg_attribute_attmissingval - 1] = true;
2152+
2153+
newtup = heap_modify_tuple(atttup, RelationGetDescr(attrrel),
2154+
valuesAtt, nullsAtt, replacesAtt);
2155+
CatalogTupleUpdate(attrrel, &newtup->t_self, newtup);
2156+
2157+
/* clean up */
2158+
ReleaseSysCache(atttup);
2159+
table_close(attrrel, RowExclusiveLock);
2160+
}
2161+
21082162
/*
21092163
* SetAttrMissing
21102164
*
@@ -2261,7 +2315,13 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
22612315
valuesAtt[Anum_pg_attribute_atthasdef - 1] = true;
22622316
replacesAtt[Anum_pg_attribute_atthasdef - 1] = true;
22632317

2264-
if (rel->rd_rel->relkind == RELKIND_RELATION && add_column_mode &&
2318+
/*
2319+
* Note: this code is dead so far as core Postgres is concerned,
2320+
* because no caller passes add_column_mode = true anymore. We keep
2321+
* it in back branches on the slight chance that some extension is
2322+
* depending on it.
2323+
*/
2324+
if (rel->rd_rel->relkind == RELKIND_RELATION && add_column_mode &&
22652325
!attgenerated)
22662326
{
22672327
expr2 = expression_planner(expr2);
@@ -2621,13 +2681,8 @@ AddRelationNewConstraints(Relation rel,
26212681
castNode(Const, expr)->constisnull))
26222682
continue;
26232683

2624-
/* If the DEFAULT is volatile we cannot use a missing value */
2625-
if (colDef->missingMode &&
2626-
contain_volatile_functions_after_planning((Expr *) expr))
2627-
colDef->missingMode = false;
2628-
26292684
defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal,
2630-
colDef->missingMode);
2685+
false);
26312686

26322687
cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
26332688
cooked->contype = CONSTR_DEFAULT;

‎src/backend/commands/tablecmds.c

Lines changed: 58 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -6579,14 +6579,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
65796579
rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
65806580
rawEnt->attnum = attribute.attnum;
65816581
rawEnt->raw_default = copyObject(colDef->raw_default);
6582-
6583-
/*
6584-
* Attempt to skip a complete table rewrite by storing the specified
6585-
* DEFAULT value outside of the heap. This may be disabled inside
6586-
* AddRelationNewConstraints if the optimization cannot be applied.
6587-
*/
6588-
rawEnt->missingMode = (!colDef->generated);
6589-
6582+
rawEnt->missingMode = false; /* XXX vestigial */
65906583
rawEnt->generated = colDef->generated;
65916584

65926585
/*
@@ -6598,13 +6591,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
65986591

65996592
/* Make the additional catalog changes visible */
66006593
CommandCounterIncrement();
6601-
6602-
/*
6603-
* Did the request for a missing value work? If not we'll have to do a
6604-
* rewrite
6605-
*/
6606-
if (!rawEnt->missingMode)
6607-
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
66086594
}
66096595

66106596
/*
@@ -6621,9 +6607,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
66216607
* rejects nulls. If there are any domain constraints then we construct
66226608
* an explicit NULL default value that will be passed through
66236609
* CoerceToDomain processing. (This is a tad inefficient, since it causes
6624-
* rewriting the table which we really don't have to do, but the present
6625-
* design of domain processing doesn't offer any simple way of checking
6626-
* the constraints more directly.)
6610+
* rewriting the table which we really wouldn't have to do; but we do it
6611+
* to preserve the historical behavior that such a failure will be raised
6612+
* only if the table currently contains some rows.)
66276613
*
66286614
* Note: we use build_column_default, and not just the cooked default
66296615
* returned by AddRelationNewConstraints, so that the right thing happens
@@ -6642,6 +6628,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
66426628
*/
66436629
if (RELKIND_HAS_STORAGE(relkind) && attribute.attnum > 0)
66446630
{
6631+
bool has_domain_constraints;
6632+
bool has_missing = false;
6633+
66456634
/*
66466635
* For an identity column, we can't use build_column_default(),
66476636
* because the sequence ownership isn't set yet. So do it manually.
@@ -6654,14 +6643,13 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
66546643
nve->typeId = typeOid;
66556644

66566645
defval = (Expr *) nve;
6657-
6658-
/* must do a rewrite for identity columns */
6659-
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
66606646
}
66616647
else
66626648
defval = (Expr *) build_column_default(rel, attribute.attnum);
66636649

6664-
if (!defval && DomainHasConstraints(typeOid))
6650+
/* Build CoerceToDomain(NULL) expression if needed */
6651+
has_domain_constraints = DomainHasConstraints(typeOid);
6652+
if (!defval && has_domain_constraints)
66656653
{
66666654
Oid baseTypeId;
66676655
int32 baseTypeMod;
@@ -6687,18 +6675,61 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
66876675
{
66886676
NewColumnValue *newval;
66896677

6678+
/* Prepare defval for execution, either here or in Phase 3 */
6679+
defval = expression_planner(defval);
6680+
6681+
/* Add the new default to the newvals list */
66906682
newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue));
66916683
newval->attnum = attribute.attnum;
6692-
newval->expr = expression_planner(defval);
6684+
newval->expr = defval;
66936685
newval->is_generated = (colDef->generated != '\0');
66946686

66956687
tab->newvals = lappend(tab->newvals, newval);
6696-
}
66976688

6698-
if (DomainHasConstraints(typeOid))
6699-
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
6689+
/*
6690+
* Attempt to skip a complete table rewrite by storing the
6691+
* specified DEFAULT value outside of the heap. This is only
6692+
* allowed for plain relations and non-generated columns, and the
6693+
* default expression can't be volatile (stable is OK). Note that
6694+
* contain_volatile_functions deems CoerceToDomain immutable, but
6695+
* here we consider that coercion to a domain with constraints is
6696+
* volatile; else it might fail even when the table is empty.
6697+
*/
6698+
if (rel->rd_rel->relkind == RELKIND_RELATION &&
6699+
!colDef->generated &&
6700+
!has_domain_constraints &&
6701+
!contain_volatile_functions((Node *) defval))
6702+
{
6703+
EState *estate;
6704+
ExprState *exprState;
6705+
Datum missingval;
6706+
bool missingIsNull;
6707+
6708+
/* Evaluate the default expression */
6709+
estate = CreateExecutorState();
6710+
exprState = ExecPrepareExpr(defval, estate);
6711+
missingval = ExecEvalExpr(exprState,
6712+
GetPerTupleExprContext(estate),
6713+
&missingIsNull);
6714+
/* If it turns out NULL, nothing to do; else store it */
6715+
if (!missingIsNull)
6716+
{
6717+
StoreAttrMissingVal(rel, attribute.attnum, missingval);
6718+
has_missing = true;
6719+
}
6720+
FreeExecutorState(estate);
6721+
}
6722+
else
6723+
{
6724+
/*
6725+
* Failed to use missing mode. We have to do a table rewrite
6726+
* to install the value.
6727+
*/
6728+
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
6729+
}
6730+
}
67006731

6701-
if (!TupleDescAttr(rel->rd_att, attribute.attnum - 1)->atthasmissing)
6732+
if (!has_missing)
67026733
{
67036734
/*
67046735
* If the new column is NOT NULL, and there is no missing value,

‎src/include/catalog/heap.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ typedef struct RawColumnDefault
2828
{
2929
AttrNumber attnum; /* attribute to attach default to */
3030
Node *raw_default; /* default value (untransformed parse tree) */
31-
bool missingMode; /* true if part of add column processing */
31+
bool missingMode; /* obsolete, no longer used */
3232
char generated; /* attgenerated setting */
3333
} RawColumnDefault;
3434

@@ -113,6 +113,9 @@ extern List *AddRelationNewConstraints(Relation rel,
113113
const char *queryString);
114114

115115
extern void RelationClearMissing(Relation rel);
116+
117+
extern void StoreAttrMissingVal(Relation rel, AttrNumber attnum,
118+
Datum missingval);
116119
extern void SetAttrMissing(Oid relid, char *attname, char *value);
117120

118121
extern Oid StoreAttrDefault(Relation rel, AttrNumber attnum,

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

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,71 @@ SELECT comp();
245245
(1 row)
246246

247247
DROP TABLE T;
248+
-- Test domains with default value for table rewrite.
249+
CREATE DOMAIN domain1 AS int DEFAULT 11; -- constant
250+
CREATE DOMAIN domain2 AS int DEFAULT 10 + (random() * 10)::int; -- volatile
251+
CREATE DOMAIN domain3 AS text DEFAULT foo(4); -- stable
252+
CREATE DOMAIN domain4 AS text[]
253+
DEFAULT ('{"This", "is", "' || foo(4) || '","the", "real", "world"}')::TEXT[];
254+
CREATE TABLE t2 (a domain1);
255+
INSERT INTO t2 VALUES (1),(2);
256+
-- no table rewrite
257+
ALTER TABLE t2 ADD COLUMN b domain1 default 3;
258+
SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
259+
FROM pg_attribute
260+
WHERE attnum > 0 AND attrelid = 't2'::regclass
261+
ORDER BY attnum;
262+
attnum | attname | atthasmissing | atthasdef | attmissingval
263+
--------+---------+---------------+-----------+---------------
264+
1 | a | f | f |
265+
2 | b | t | t | {3}
266+
(2 rows)
267+
268+
-- table rewrite should happen
269+
ALTER TABLE t2 ADD COLUMN c domain3 default left(random()::text,3);
270+
NOTICE: rewriting table t2 for reason 2
271+
-- no table rewrite
272+
ALTER TABLE t2 ADD COLUMN d domain4;
273+
SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
274+
FROM pg_attribute
275+
WHERE attnum > 0 AND attrelid = 't2'::regclass
276+
ORDER BY attnum;
277+
attnum | attname | atthasmissing | atthasdef | attmissingval
278+
--------+---------+---------------+-----------+-----------------------------------
279+
1 | a | f | f |
280+
2 | b | f | t |
281+
3 | c | f | t |
282+
4 | d | t | f | {"{This,is,abcd,the,real,world}"}
283+
(4 rows)
284+
285+
-- table rewrite should happen
286+
ALTER TABLE t2 ADD COLUMN e domain2;
287+
NOTICE: rewriting table t2 for reason 2
288+
SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
289+
FROM pg_attribute
290+
WHERE attnum > 0 AND attrelid = 't2'::regclass
291+
ORDER BY attnum;
292+
attnum | attname | atthasmissing | atthasdef | attmissingval
293+
--------+---------+---------------+-----------+---------------
294+
1 | a | f | f |
295+
2 | b | f | t |
296+
3 | c | f | t |
297+
4 | d | f | f |
298+
5 | e | f | f |
299+
(5 rows)
300+
301+
SELECT a, b, length(c) = 3 as c_ok, d, e >= 10 as e_ok FROM t2;
302+
a | b | c_ok | d | e_ok
303+
---+---+------+-------------------------------+------
304+
1 | 3 | t | {This,is,abcd,the,real,world} | t
305+
2 | 3 | t | {This,is,abcd,the,real,world} | t
306+
(2 rows)
307+
308+
DROP TABLE t2;
309+
DROP DOMAIN domain1;
310+
DROP DOMAIN domain2;
311+
DROP DOMAIN domain3;
312+
DROP DOMAIN domain4;
248313
DROP FUNCTION foo(INT);
249314
-- Fall back to full rewrite for volatile expressions
250315
CREATE TABLE T(pk INT NOT NULL PRIMARY KEY);

‎src/test/regress/sql/fast_default.sql

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,50 @@ SELECT comp();
237237

238238
DROP TABLE T;
239239

240+
-- Test domains with default value for table rewrite.
241+
CREATE DOMAIN domain1 AS int DEFAULT 11; -- constant
242+
CREATE DOMAIN domain2 AS int DEFAULT 10 + (random() * 10)::int; -- volatile
243+
CREATE DOMAIN domain3 AS text DEFAULT foo(4); -- stable
244+
CREATE DOMAIN domain4 AS text[]
245+
DEFAULT ('{"This", "is", "' || foo(4) || '","the", "real", "world"}')::TEXT[];
246+
247+
CREATE TABLE t2 (a domain1);
248+
INSERT INTO t2 VALUES (1),(2);
249+
250+
-- no table rewrite
251+
ALTER TABLE t2 ADD COLUMN b domain1 default 3;
252+
253+
SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
254+
FROM pg_attribute
255+
WHERE attnum > 0 AND attrelid = 't2'::regclass
256+
ORDER BY attnum;
257+
258+
-- table rewrite should happen
259+
ALTER TABLE t2 ADD COLUMN c domain3 default left(random()::text,3);
260+
261+
-- no table rewrite
262+
ALTER TABLE t2 ADD COLUMN d domain4;
263+
264+
SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
265+
FROM pg_attribute
266+
WHERE attnum > 0 AND attrelid = 't2'::regclass
267+
ORDER BY attnum;
268+
269+
-- table rewrite should happen
270+
ALTER TABLE t2 ADD COLUMN e domain2;
271+
272+
SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
273+
FROM pg_attribute
274+
WHERE attnum > 0 AND attrelid = 't2'::regclass
275+
ORDER BY attnum;
276+
277+
SELECT a, b, length(c) = 3 as c_ok, d, e >= 10 as e_ok FROM t2;
278+
279+
DROP TABLE t2;
280+
DROP DOMAIN domain1;
281+
DROP DOMAIN domain2;
282+
DROP DOMAIN domain3;
283+
DROP DOMAIN domain4;
240284
DROP FUNCTION foo(INT);
241285

242286
-- Fall back to full rewrite for volatile expressions

0 commit comments

Comments
 (0)