Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 95f6506

Browse files
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 99f8f3f commit 95f6506

File tree

6 files changed

+235
-34
lines changed

6 files changed

+235
-34
lines changed

src/backend/catalog/heap.c

+56-6
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,7 @@
6969
#include "pgstat.h"
7070
#include "storage/lmgr.h"
7171
#include "storage/predicate.h"
72+
#include "utils/array.h"
7273
#include "utils/builtins.h"
7374
#include "utils/fmgroids.h"
7475
#include "utils/inval.h"
@@ -2010,6 +2011,60 @@ RelationClearMissing(Relation rel)
20102011
table_close(attr_rel, RowExclusiveLock);
20112012
}
20122013

2014+
/*
2015+
* StoreAttrMissingVal
2016+
*
2017+
* Set the missing value of a single attribute.
2018+
*/
2019+
void
2020+
StoreAttrMissingVal(Relation rel, AttrNumber attnum, Datum missingval)
2021+
{
2022+
Datum valuesAtt[Natts_pg_attribute] = {0};
2023+
bool nullsAtt[Natts_pg_attribute] = {0};
2024+
bool replacesAtt[Natts_pg_attribute] = {0};
2025+
Relation attrrel;
2026+
Form_pg_attribute attStruct;
2027+
HeapTuple atttup,
2028+
newtup;
2029+
2030+
/* This is only supported for plain tables */
2031+
Assert(rel->rd_rel->relkind == RELKIND_RELATION);
2032+
2033+
/* Fetch the pg_attribute row */
2034+
attrrel = table_open(AttributeRelationId, RowExclusiveLock);
2035+
2036+
atttup = SearchSysCache2(ATTNUM,
2037+
ObjectIdGetDatum(RelationGetRelid(rel)),
2038+
Int16GetDatum(attnum));
2039+
if (!HeapTupleIsValid(atttup)) /* shouldn't happen */
2040+
elog(ERROR, "cache lookup failed for attribute %d of relation %u",
2041+
attnum, RelationGetRelid(rel));
2042+
attStruct = (Form_pg_attribute) GETSTRUCT(atttup);
2043+
2044+
/* Make a one-element array containing the value */
2045+
missingval = PointerGetDatum(construct_array(&missingval,
2046+
1,
2047+
attStruct->atttypid,
2048+
attStruct->attlen,
2049+
attStruct->attbyval,
2050+
attStruct->attalign));
2051+
2052+
/* Update the pg_attribute row */
2053+
valuesAtt[Anum_pg_attribute_atthasmissing - 1] = BoolGetDatum(true);
2054+
replacesAtt[Anum_pg_attribute_atthasmissing - 1] = true;
2055+
2056+
valuesAtt[Anum_pg_attribute_attmissingval - 1] = missingval;
2057+
replacesAtt[Anum_pg_attribute_attmissingval - 1] = true;
2058+
2059+
newtup = heap_modify_tuple(atttup, RelationGetDescr(attrrel),
2060+
valuesAtt, nullsAtt, replacesAtt);
2061+
CatalogTupleUpdate(attrrel, &newtup->t_self, newtup);
2062+
2063+
/* clean up */
2064+
ReleaseSysCache(atttup);
2065+
table_close(attrrel, RowExclusiveLock);
2066+
}
2067+
20132068
/*
20142069
* SetAttrMissing
20152070
*
@@ -2392,13 +2447,8 @@ AddRelationNewConstraints(Relation rel,
23922447
castNode(Const, expr)->constisnull))
23932448
continue;
23942449

2395-
/* If the DEFAULT is volatile we cannot use a missing value */
2396-
if (colDef->missingMode &&
2397-
contain_volatile_functions_after_planning((Expr *) expr))
2398-
colDef->missingMode = false;
2399-
24002450
defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal,
2401-
colDef->missingMode);
2451+
false);
24022452

24032453
cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
24042454
cooked->contype = CONSTR_DEFAULT;

src/backend/catalog/pg_attrdef.c

+6
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,12 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
120120
valuesAtt[Anum_pg_attribute_atthasdef - 1] = true;
121121
replacesAtt[Anum_pg_attribute_atthasdef - 1] = true;
122122

123+
/*
124+
* Note: this code is dead so far as core Postgres is concerned,
125+
* because no caller passes add_column_mode = true anymore. We keep
126+
* it in back branches on the slight chance that some extension is
127+
* depending on it.
128+
*/
123129
if (rel->rd_rel->relkind == RELKIND_RELATION && add_column_mode &&
124130
!attgenerated)
125131
{

src/backend/commands/tablecmds.c

+60-27
Original file line numberDiff line numberDiff line change
@@ -7321,14 +7321,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
73217321
rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
73227322
rawEnt->attnum = attribute->attnum;
73237323
rawEnt->raw_default = copyObject(colDef->raw_default);
7324-
7325-
/*
7326-
* Attempt to skip a complete table rewrite by storing the specified
7327-
* DEFAULT value outside of the heap. This may be disabled inside
7328-
* AddRelationNewConstraints if the optimization cannot be applied.
7329-
*/
7330-
rawEnt->missingMode = (colDef->generated != ATTRIBUTE_GENERATED_STORED);
7331-
7324+
rawEnt->missingMode = false; /* XXX vestigial */
73327325
rawEnt->generated = colDef->generated;
73337326

73347327
/*
@@ -7340,13 +7333,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
73407333

73417334
/* Make the additional catalog changes visible */
73427335
CommandCounterIncrement();
7343-
7344-
/*
7345-
* Did the request for a missing value work? If not we'll have to do a
7346-
* rewrite
7347-
*/
7348-
if (!rawEnt->missingMode)
7349-
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
73507336
}
73517337

73527338
/*
@@ -7363,9 +7349,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
73637349
* rejects nulls. If there are any domain constraints then we construct
73647350
* an explicit NULL default value that will be passed through
73657351
* CoerceToDomain processing. (This is a tad inefficient, since it causes
7366-
* rewriting the table which we really don't have to do, but the present
7367-
* design of domain processing doesn't offer any simple way of checking
7368-
* the constraints more directly.)
7352+
* rewriting the table which we really wouldn't have to do; but we do it
7353+
* to preserve the historical behavior that such a failure will be raised
7354+
* only if the table currently contains some rows.)
73697355
*
73707356
* Note: we use build_column_default, and not just the cooked default
73717357
* returned by AddRelationNewConstraints, so that the right thing happens
@@ -7384,6 +7370,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
73847370
*/
73857371
if (RELKIND_HAS_STORAGE(relkind))
73867372
{
7373+
bool has_domain_constraints;
7374+
bool has_missing = false;
7375+
73877376
/*
73887377
* For an identity column, we can't use build_column_default(),
73897378
* because the sequence ownership isn't set yet. So do it manually.
@@ -7396,14 +7385,13 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
73967385
nve->typeId = attribute->atttypid;
73977386

73987387
defval = (Expr *) nve;
7399-
7400-
/* must do a rewrite for identity columns */
7401-
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
74027388
}
74037389
else
74047390
defval = (Expr *) build_column_default(rel, attribute->attnum);
74057391

7406-
if (!defval && DomainHasConstraints(attribute->atttypid))
7392+
/* Build CoerceToDomain(NULL) expression if needed */
7393+
has_domain_constraints = DomainHasConstraints(attribute->atttypid);
7394+
if (!defval && has_domain_constraints)
74077395
{
74087396
Oid baseTypeId;
74097397
int32 baseTypeMod;
@@ -7429,18 +7417,63 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
74297417
{
74307418
NewColumnValue *newval;
74317419

7420+
/* Prepare defval for execution, either here or in Phase 3 */
7421+
defval = expression_planner(defval);
7422+
7423+
/* Add the new default to the newvals list */
74327424
newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue));
74337425
newval->attnum = attribute->attnum;
7434-
newval->expr = expression_planner(defval);
7426+
newval->expr = defval;
74357427
newval->is_generated = (colDef->generated != '\0');
74367428

74377429
tab->newvals = lappend(tab->newvals, newval);
7438-
}
74397430

7440-
if (DomainHasConstraints(attribute->atttypid))
7441-
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
7431+
/*
7432+
* Attempt to skip a complete table rewrite by storing the
7433+
* specified DEFAULT value outside of the heap. This is only
7434+
* allowed for plain relations and non-generated columns, and the
7435+
* default expression can't be volatile (stable is OK). Note that
7436+
* contain_volatile_functions deems CoerceToDomain immutable, but
7437+
* here we consider that coercion to a domain with constraints is
7438+
* volatile; else it might fail even when the table is empty.
7439+
*/
7440+
if (rel->rd_rel->relkind == RELKIND_RELATION &&
7441+
!colDef->generated &&
7442+
!has_domain_constraints &&
7443+
!contain_volatile_functions((Node *) defval))
7444+
{
7445+
EState *estate;
7446+
ExprState *exprState;
7447+
Datum missingval;
7448+
bool missingIsNull;
7449+
7450+
/* Evaluate the default expression */
7451+
estate = CreateExecutorState();
7452+
exprState = ExecPrepareExpr(defval, estate);
7453+
missingval = ExecEvalExpr(exprState,
7454+
GetPerTupleExprContext(estate),
7455+
&missingIsNull);
7456+
/* If it turns out NULL, nothing to do; else store it */
7457+
if (!missingIsNull)
7458+
{
7459+
StoreAttrMissingVal(rel, attribute->attnum, missingval);
7460+
has_missing = true;
7461+
}
7462+
FreeExecutorState(estate);
7463+
}
7464+
else
7465+
{
7466+
/*
7467+
* Failed to use missing mode. We have to do a table rewrite
7468+
* to install the value --- unless it's a virtual generated
7469+
* column.
7470+
*/
7471+
if (colDef->generated != ATTRIBUTE_GENERATED_VIRTUAL)
7472+
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
7473+
}
7474+
}
74427475

7443-
if (!TupleDescAttr(rel->rd_att, attribute->attnum - 1)->atthasmissing)
7476+
if (!has_missing)
74447477
{
74457478
/*
74467479
* If the new column is NOT NULL, and there is no missing value,

src/include/catalog/heap.h

+4-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ typedef struct RawColumnDefault
2929
{
3030
AttrNumber attnum; /* attribute to attach default to */
3131
Node *raw_default; /* default value (untransformed parse tree) */
32-
bool missingMode; /* true if part of add column processing */
32+
bool missingMode; /* obsolete, no longer used */
3333
char generated; /* attgenerated setting */
3434
} RawColumnDefault;
3535

@@ -121,6 +121,9 @@ extern List *AddRelationNotNullConstraints(Relation rel,
121121
List *old_notnulls);
122122

123123
extern void RelationClearMissing(Relation rel);
124+
125+
extern void StoreAttrMissingVal(Relation rel, AttrNumber attnum,
126+
Datum missingval);
124127
extern void SetAttrMissing(Oid relid, char *attname, char *value);
125128

126129
extern Node *cookDefault(ParseState *pstate,

src/test/regress/expected/fast_default.out

+65
Original file line numberDiff line numberDiff line change
@@ -257,6 +257,71 @@ SELECT comp();
257257
(1 row)
258258

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

src/test/regress/sql/fast_default.sql

+44
Original file line numberDiff line numberDiff line change
@@ -248,6 +248,50 @@ SELECT comp();
248248

249249
DROP TABLE T;
250250

251+
-- Test domains with default value for table rewrite.
252+
CREATE DOMAIN domain1 AS int DEFAULT 11; -- constant
253+
CREATE DOMAIN domain2 AS int DEFAULT random(min=>10, max=>100); -- volatile
254+
CREATE DOMAIN domain3 AS text DEFAULT foo(4); -- stable
255+
CREATE DOMAIN domain4 AS text[]
256+
DEFAULT ('{"This", "is", "' || foo(4) || '","the", "real", "world"}')::TEXT[];
257+
258+
CREATE TABLE t2 (a domain1);
259+
INSERT INTO t2 VALUES (1),(2);
260+
261+
-- no table rewrite
262+
ALTER TABLE t2 ADD COLUMN b domain1 default 3;
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 c domain3 default left(random()::text,3);
271+
272+
-- no table rewrite
273+
ALTER TABLE t2 ADD COLUMN d domain4;
274+
275+
SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
276+
FROM pg_attribute
277+
WHERE attnum > 0 AND attrelid = 't2'::regclass
278+
ORDER BY attnum;
279+
280+
-- table rewrite should happen
281+
ALTER TABLE t2 ADD COLUMN e domain2;
282+
283+
SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
284+
FROM pg_attribute
285+
WHERE attnum > 0 AND attrelid = 't2'::regclass
286+
ORDER BY attnum;
287+
288+
SELECT a, b, length(c) = 3 as c_ok, d, e >= 10 as e_ok FROM t2;
289+
290+
DROP TABLE t2;
291+
DROP DOMAIN domain1;
292+
DROP DOMAIN domain2;
293+
DROP DOMAIN domain3;
294+
DROP DOMAIN domain4;
251295
DROP FUNCTION foo(INT);
252296

253297
-- Fall back to full rewrite for volatile expressions

0 commit comments

Comments
 (0)