On Sun, Mar 2, 2025 at 7:54 AM Tom Lane <t...@sss.pgh.pa.us> wrote: > > jian he <jian.universal...@gmail.com> writes: > > So I duplicated StoreAttrDefault and removed pg_attribute.atthasdef, > > pg_attrdef related code. > > and it works fine. > > I think the fundamental problem here is that StoreAttrDefault > shouldn't be involved in this in the first place. It looks like > somebody did that in the hopes of avoiding two updates of the > new pg_attribute row (one to set atthasdef and the other to fill > attmissingval), but it's just bad code structure. We should > take that code out of StoreAttrDefault, not duplicate it, because > the assumption that the missingval is identical to what goes into > pg_attrdef is just wrong. >
in StoreAttrDefault, i've split missingval related code into StoreAttrMissingVal. > Also, since the "if (RELKIND_HAS_STORAGE(relkind))" stanza > in ATExecAddColumn is already calling expression_planner, > we should be able to avoid doing that twice on the way to > discovering whether the expression is a constant. done. > I kind of feel that StoreAttrMissingVal belongs in heap.c; > it's got about nothing to do with pg_attrdef. heap.c seems fine.
From bd79aede786a16e0c105dc28f6a0f7e6f045bed0 Mon Sep 17 00:00:00 2001 From: jian he <jian.universal...@gmail.com> Date: Sun, 2 Mar 2025 17:49:03 +0800 Subject: [PATCH v2 1/1] apply fast default for adding new column over domain with default value split part of logic in StoreAttrDefault to new function: StoreAttrMissingVal. it will handle: construct attmissingval value and update pg_attribute.attmissingval, atthasmissing for the specific attnum. it will optionally update atthasdef. we use StoreAttrMissingVal to apply fast default mechanism to domain with default value. discussion: https://postgr.es/m/CACJufxHFssPvkP1we7WMhPD_1kwgbG52o=kqgl+tnvox5lo...@mail.gmail.com --- src/backend/catalog/heap.c | 110 +++++++++++++++++++++ src/backend/catalog/pg_attrdef.c | 82 +-------------- src/backend/commands/tablecmds.c | 26 ++++- src/include/catalog/heap.h | 4 + src/test/regress/expected/fast_default.out | 49 +++++++++ src/test/regress/sql/fast_default.sql | 36 +++++++ 6 files changed, 225 insertions(+), 82 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index 956f196fc95..77358a3b4e1 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -57,6 +57,7 @@ #include "commands/tablecmds.h" #include "commands/typecmds.h" #include "common/int.h" +#include "executor/executor.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" #include "optimizer/optimizer.h" @@ -69,6 +70,7 @@ #include "pgstat.h" #include "storage/lmgr.h" #include "storage/predicate.h" +#include "utils/array.h" #include "utils/builtins.h" #include "utils/fmgroids.h" #include "utils/inval.h" @@ -2068,6 +2070,114 @@ SetAttrMissing(Oid relid, char *attname, char *value) table_close(tablerel, AccessExclusiveLock); } +/* + * StoreAttrMissingVal + * Stores the datum value of pg_attribute.attmissingval for the attribute attnum + * optionally update atthasdef. currently this is mainly used within + * StoreAttrDefault, but it can be used seperately. + * + * attnum: The attribute number we are interested into. + * expr: The expression to be evaluated, whose resulting datum value is stored + * in pg_attribute.attmissingval. + * add_column_mode: it has the same meaning as in StoreAttrDefault. should pass + * as true is not used in the context of StoreAttrDefault. + * stored_default: If true, pg_attribute.atthasdef is updated to true. + * att_generated: If not NULL, it will set as pg_attribute.ttgenerated for attnum. + */ +void +StoreAttrMissingVal(Relation rel, AttrNumber attnum, Node *expr, + bool add_column_mode, bool stored_default, char *att_generated) +{ + Relation attrrel; + HeapTuple atttup; + Form_pg_attribute attStruct; + Form_pg_attribute defAttStruct; + char attgenerated; + + /* + * Update the pg_attribute entry for the column to show that we store attmissingval + * on it. + */ + attrrel = table_open(AttributeRelationId, RowExclusiveLock); + atttup = SearchSysCacheCopy2(ATTNUM, + ObjectIdGetDatum(RelationGetRelid(rel)), + Int16GetDatum(attnum)); + if (!HeapTupleIsValid(atttup)) + elog(ERROR, "cache lookup failed for attribute %d of relation %u", + attnum, RelationGetRelid(rel)); + attStruct = (Form_pg_attribute) GETSTRUCT(atttup); + attgenerated = attStruct->attgenerated; + if (att_generated) + *att_generated = attgenerated; + + if (!attStruct->atthasdef) + { + ExprState *exprState; + Expr *expr2 = (Expr *) expr; + EState *estate = NULL; + ExprContext *econtext; + Datum valuesAtt[Natts_pg_attribute] = {0}; + bool nullsAtt[Natts_pg_attribute] = {0}; + bool replacesAtt[Natts_pg_attribute] = {0}; + Datum missingval = (Datum) 0; + bool missingIsNull = true; + + if (stored_default) + { + valuesAtt[Anum_pg_attribute_atthasdef - 1] = true; + replacesAtt[Anum_pg_attribute_atthasdef - 1] = true; + } + + if (rel->rd_rel->relkind == RELKIND_RELATION && add_column_mode && + !attgenerated) + { + if (!IsA(expr2, Const)) + expr2 = expression_planner(expr2); + estate = CreateExecutorState(); + exprState = ExecPrepareExpr(expr2, estate); + econtext = GetPerTupleExprContext(estate); + + missingval = ExecEvalExpr(exprState, econtext, + &missingIsNull); + + FreeExecutorState(estate); + + defAttStruct = TupleDescAttr(rel->rd_att, attnum - 1); + + if (missingIsNull) + { + /* if the default evaluates to NULL, just store a NULL array */ + missingval = (Datum) 0; + } + else + { + /* otherwise make a one-element array of the value */ + missingval = PointerGetDatum(construct_array(&missingval, + 1, + defAttStruct->atttypid, + defAttStruct->attlen, + defAttStruct->attbyval, + defAttStruct->attalign)); + } + + valuesAtt[Anum_pg_attribute_atthasmissing - 1] = !missingIsNull; + replacesAtt[Anum_pg_attribute_atthasmissing - 1] = true; + valuesAtt[Anum_pg_attribute_attmissingval - 1] = missingval; + replacesAtt[Anum_pg_attribute_attmissingval - 1] = true; + nullsAtt[Anum_pg_attribute_attmissingval - 1] = missingIsNull; + } + atttup = heap_modify_tuple(atttup, RelationGetDescr(attrrel), + valuesAtt, nullsAtt, replacesAtt); + + CatalogTupleUpdate(attrrel, &atttup->t_self, atttup); + + if (!missingIsNull) + pfree(DatumGetPointer(missingval)); + } + table_close(attrrel, RowExclusiveLock); + heap_freetuple(atttup); +} + /* * Store a check-constraint expression for the given relation. * diff --git a/src/backend/catalog/pg_attrdef.c b/src/backend/catalog/pg_attrdef.c index 91dafc81021..bad7267513c 100644 --- a/src/backend/catalog/pg_attrdef.c +++ b/src/backend/catalog/pg_attrdef.c @@ -19,6 +19,7 @@ #include "access/table.h" #include "catalog/catalog.h" #include "catalog/dependency.h" +#include "catalog/heap.h" #include "catalog/indexing.h" #include "catalog/objectaccess.h" #include "catalog/pg_attrdef.h" @@ -51,9 +52,6 @@ StoreAttrDefault(Relation rel, AttrNumber attnum, HeapTuple tuple; Datum values[4]; static bool nulls[4] = {false, false, false, false}; - Relation attrrel; - HeapTuple atttup; - Form_pg_attribute attStruct; char attgenerated; Oid attrdefOid; ObjectAddress colobject, @@ -90,83 +88,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum, heap_freetuple(tuple); pfree(adbin); - /* - * Update the pg_attribute entry for the column to show that a default - * exists. - */ - attrrel = table_open(AttributeRelationId, RowExclusiveLock); - atttup = SearchSysCacheCopy2(ATTNUM, - ObjectIdGetDatum(RelationGetRelid(rel)), - Int16GetDatum(attnum)); - if (!HeapTupleIsValid(atttup)) - elog(ERROR, "cache lookup failed for attribute %d of relation %u", - attnum, RelationGetRelid(rel)); - attStruct = (Form_pg_attribute) GETSTRUCT(atttup); - attgenerated = attStruct->attgenerated; - if (!attStruct->atthasdef) - { - Form_pg_attribute defAttStruct; - - ExprState *exprState; - Expr *expr2 = (Expr *) expr; - EState *estate = NULL; - ExprContext *econtext; - Datum valuesAtt[Natts_pg_attribute] = {0}; - bool nullsAtt[Natts_pg_attribute] = {0}; - bool replacesAtt[Natts_pg_attribute] = {0}; - Datum missingval = (Datum) 0; - bool missingIsNull = true; - - valuesAtt[Anum_pg_attribute_atthasdef - 1] = true; - replacesAtt[Anum_pg_attribute_atthasdef - 1] = true; - - if (rel->rd_rel->relkind == RELKIND_RELATION && add_column_mode && - !attgenerated) - { - expr2 = expression_planner(expr2); - estate = CreateExecutorState(); - exprState = ExecPrepareExpr(expr2, estate); - econtext = GetPerTupleExprContext(estate); - - missingval = ExecEvalExpr(exprState, econtext, - &missingIsNull); - - FreeExecutorState(estate); - - defAttStruct = TupleDescAttr(rel->rd_att, attnum - 1); - - if (missingIsNull) - { - /* if the default evaluates to NULL, just store a NULL array */ - missingval = (Datum) 0; - } - else - { - /* otherwise make a one-element array of the value */ - missingval = PointerGetDatum(construct_array(&missingval, - 1, - defAttStruct->atttypid, - defAttStruct->attlen, - defAttStruct->attbyval, - defAttStruct->attalign)); - } - - valuesAtt[Anum_pg_attribute_atthasmissing - 1] = !missingIsNull; - replacesAtt[Anum_pg_attribute_atthasmissing - 1] = true; - valuesAtt[Anum_pg_attribute_attmissingval - 1] = missingval; - replacesAtt[Anum_pg_attribute_attmissingval - 1] = true; - nullsAtt[Anum_pg_attribute_attmissingval - 1] = missingIsNull; - } - atttup = heap_modify_tuple(atttup, RelationGetDescr(attrrel), - valuesAtt, nullsAtt, replacesAtt); - - CatalogTupleUpdate(attrrel, &atttup->t_self, atttup); - - if (!missingIsNull) - pfree(DatumGetPointer(missingval)); - } - table_close(attrrel, RowExclusiveLock); - heap_freetuple(atttup); + StoreAttrMissingVal(rel, attnum, expr, add_column_mode, true, &attgenerated); /* * Make a dependency so that the pg_attrdef entry goes away if the column diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index ce7d115667e..0844d6d0817 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7139,6 +7139,8 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, AlterTableCmd *childcmd; ObjectAddress address; TupleDesc tupdesc; + bool is_domain = false; + bool is_domain_has_constaints = false; /* since this function recurses, it could be driven to stack overflow */ check_stack_depth(); @@ -7403,7 +7405,27 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, else defval = (Expr *) build_column_default(rel, attribute->attnum); - if (!defval && DomainHasConstraints(attribute->atttypid)) + is_domain = (get_typtype(attribute->atttypid) == TYPTYPE_DOMAIN); + if(is_domain) + is_domain_has_constaints = DomainHasConstraints(attribute->atttypid); + + /* + * domain have constraints on it will cause table rewrite + * we may loose it in future. + */ + if (defval && is_domain && !is_domain_has_constaints) + { + if (contain_volatile_functions_after_planning((Expr *) defval)) + tab->rewrite |= AT_REWRITE_DEFAULT_VAL; + else if (!attribute->atthasdef && attribute->attgenerated == '\0') + { + StoreAttrMissingVal(rel, attribute->attnum, (Node *) defval, true, false, NULL); + /* we have changed pg_attribute, we need refresh the cache */ + CommandCounterIncrement(); + } + } + + if (!defval && is_domain_has_constaints) { Oid baseTypeId; int32 baseTypeMod; @@ -7437,7 +7459,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, tab->newvals = lappend(tab->newvals, newval); } - if (DomainHasConstraints(attribute->atttypid)) + if (is_domain_has_constaints) tab->rewrite |= AT_REWRITE_DEFAULT_VAL; if (!TupleDescAttr(rel->rd_att, attribute->attnum - 1)->atthasmissing) diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h index 19c594458bd..aeb61bdd778 100644 --- a/src/include/catalog/heap.h +++ b/src/include/catalog/heap.h @@ -123,6 +123,9 @@ extern List *AddRelationNotNullConstraints(Relation rel, extern void RelationClearMissing(Relation rel); extern void SetAttrMissing(Oid relid, char *attname, char *value); +extern void StoreAttrMissingVal(Relation rel, AttrNumber attnum, Node *expr, + bool add_column_mode, bool stored_default, + char *att_generated); extern Node *cookDefault(ParseState *pstate, Node *raw_default, Oid atttypid, @@ -162,4 +165,5 @@ extern void RemovePartitionKeyByRelId(Oid relid); extern void StorePartitionBound(Relation rel, Relation parent, PartitionBoundSpec *bound); + #endif /* HEAP_H */ diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out index 272b57e48cd..d8c99c46fd3 100644 --- a/src/test/regress/expected/fast_default.out +++ b/src/test/regress/expected/fast_default.out @@ -257,6 +257,55 @@ SELECT comp(); (1 row) DROP TABLE T; +---test domain with default value for table rewrite. +CREATE DOMAIN domain1 AS int DEFAULT 11; +CREATE DOMAIN domain2 AS int DEFAULT random(min=>10::int, max=>100); --volatile expression. +CREATE DOMAIN domain3 AS text DEFAULT foo(4); +CREATE DOMAIN domain4 AS text[] DEFAULT ('{"This", "is", "' || foo(4) || '","the", "real", "world"}')::TEXT[]; +CREATE TABLE t2 (a domain1); +INSERT INTO t2 VALUES (1),(2); +--no table rewrite +ALTER TABLE t2 ADD COLUMN b domain1 default 3; +--table rewrite should happen +ALTER TABLE t2 ADD COLUMN c domain3 default left(random()::text,3); +NOTICE: rewriting table t2 for reason 2 +--no table rewrite +ALTER TABLE t2 ADD COLUMN d domain4; +SELECT attnum, attname,atthasmissing, atthasdef, attmissingval +FROM pg_attribute pa +where attnum >0 and attrelid = 't2'::regclass; + attnum | attname | atthasmissing | atthasdef | attmissingval +--------+---------+---------------+-----------+----------------------------------- + 1 | a | f | f | + 2 | b | f | t | + 3 | c | f | t | + 4 | d | t | f | {"{This,is,abcd,the,real,world}"} +(4 rows) + +--table rewrite should happen +ALTER TABLE t2 ADD COLUMN e domain2; +NOTICE: rewriting table t2 for reason 2 +SELECT count(*) = 0 as expect_true +FROM pg_attribute pa +where attnum >0 and attrelid = 't2'::regclass +and (atthasmissing is true or attmissingval is not null); + expect_true +------------- + t +(1 row) + +SELECT a,b,length(c) = 3 as expect_true, d, e >=10 as expect_true FROM t2; + a | b | expect_true | d | expect_true +---+---+-------------+-------------------------------+------------- + 1 | 3 | t | {This,is,abcd,the,real,world} | t + 2 | 3 | t | {This,is,abcd,the,real,world} | t +(2 rows) + +DROP TABLE t2; +DROP DOMAIN domain1; +DROP DOMAIN domain2; +DROP DOMAIN domain3; +DROP DOMAIN domain4; DROP FUNCTION foo(INT); -- Fall back to full rewrite for volatile expressions CREATE TABLE T(pk INT NOT NULL PRIMARY KEY); diff --git a/src/test/regress/sql/fast_default.sql b/src/test/regress/sql/fast_default.sql index 6e7f37b17b2..d1a4bf9a625 100644 --- a/src/test/regress/sql/fast_default.sql +++ b/src/test/regress/sql/fast_default.sql @@ -248,6 +248,42 @@ SELECT comp(); DROP TABLE T; +---test domain with default value for table rewrite. +CREATE DOMAIN domain1 AS int DEFAULT 11; +CREATE DOMAIN domain2 AS int DEFAULT random(min=>10::int, max=>100); --volatile expression. +CREATE DOMAIN domain3 AS text DEFAULT foo(4); +CREATE DOMAIN domain4 AS text[] DEFAULT ('{"This", "is", "' || foo(4) || '","the", "real", "world"}')::TEXT[]; + +CREATE TABLE t2 (a domain1); +INSERT INTO t2 VALUES (1),(2); + +--no table rewrite +ALTER TABLE t2 ADD COLUMN b domain1 default 3; + +--table rewrite should happen +ALTER TABLE t2 ADD COLUMN c domain3 default left(random()::text,3); + +--no table rewrite +ALTER TABLE t2 ADD COLUMN d domain4; + +SELECT attnum, attname,atthasmissing, atthasdef, attmissingval +FROM pg_attribute pa +where attnum >0 and attrelid = 't2'::regclass; + +--table rewrite should happen +ALTER TABLE t2 ADD COLUMN e domain2; + +SELECT count(*) = 0 as expect_true +FROM pg_attribute pa +where attnum >0 and attrelid = 't2'::regclass +and (atthasmissing is true or attmissingval is not null); + +SELECT a,b,length(c) = 3 as expect_true, d, e >=10 as expect_true FROM t2; +DROP TABLE t2; +DROP DOMAIN domain1; +DROP DOMAIN domain2; +DROP DOMAIN domain3; +DROP DOMAIN domain4; DROP FUNCTION foo(INT); -- Fall back to full rewrite for volatile expressions -- 2.34.1
