On Sat, Mar 1, 2025 at 2:43 PM Tom Lane <[email protected]> wrote:
>
> I do not believe that case should require a table rewrite.
> Both the default and the check constraint are immutable,
> so we ought to be able to apply the check once and then
> use the default as the attmissingval.
>
> > Attach a patch to fix this issue by cause it to table rewrite.
>
> I see no attached patch, but in any case forcing a table rewrite
> seems like the wrong direction...
>
forcing table rewrite would be an easier fix.
but not forcing table write seems doable.
\d pg_attrdef
Table "pg_catalog.pg_attrdef"
Column | Type | Collation | Nullable | Default
---------+--------------+-----------+----------+---------
oid | oid | | not null |
adrelid | oid | | not null |
adnum | smallint | | not null |
adbin | pg_node_tree | C | not null |
Indexes:
"pg_attrdef_oid_index" PRIMARY KEY, btree (oid)
"pg_attrdef_adrelid_adnum_index" UNIQUE CONSTRAINT, btree (adrelid, adnum)
pg_attrdef_adrelid_adnum_index means
a column can only have one default expression adbin.
if we store domain's default expression in pg_attrdef it may have error like:
CREATE DOMAIN int_domain AS int DEFAULT 11;
ALTER TABLE t2 ADD COLUMN b int_domain default 3;
ERROR: duplicate key value violates unique constraint
"pg_attrdef_adrelid_adnum_index"
DETAIL: Key (adrelid, adnum)=(18102, 2) already exists.
currently function StoreAttrDefault will
1. set pg_attribute.atthasdef
2. compute and set atthasmissing, attmissingval
3. insert an entry in pg_attrdef.
but we only want 2.
So I duplicated StoreAttrDefault and removed pg_attribute.atthasdef,
pg_attrdef related code.
and it works fine.
From 4f87b744b19f34b02817d252cc69666e33da2e18 Mon Sep 17 00:00:00 2001
From: jian he <[email protected]>
Date: Sat, 1 Mar 2025 22:01:21 +0800
Subject: [PATCH v1 1/1] apply fast default for adding new column over domain
with default value
discussion: https://postgr.es/m/CACJufxHFssPvkP1we7WMhPD_1kwgbG52o=kqgl+tnvox5lo...@mail.gmail.com
---
src/backend/catalog/pg_attrdef.c | 108 +++++++++++++++++++++
src/backend/commands/tablecmds.c | 26 ++++-
src/include/catalog/pg_attrdef.h | 3 +
src/test/regress/expected/fast_default.out | 49 ++++++++++
src/test/regress/sql/fast_default.sql | 36 +++++++
5 files changed, 220 insertions(+), 2 deletions(-)
diff --git a/src/backend/catalog/pg_attrdef.c b/src/backend/catalog/pg_attrdef.c
index 91dafc81021..3b47139f94f 100644
--- a/src/backend/catalog/pg_attrdef.c
+++ b/src/backend/catalog/pg_attrdef.c
@@ -31,6 +31,114 @@
#include "utils/syscache.h"
+
+/*
+ * XXX is this the righ place?
+ * Store a attmissingval datum value for column attnum of relation rel.
+ *
+ * This closely resembles StoreAttrDefault, like:
+ * Sets atthasmissing to true and adds attmissingval for the specified attnum.
+ * but with two differences:
+ * - Does not set pg_attribute.atthasdef to true.
+ * - Does not store the default expression in pg_attrdef.
+ */
+void
+StoreAttrMissingVal(Relation rel, AttrNumber attnum,
+ Node *expr, bool is_internal, bool add_column_mode)
+{
+ Relation attrrel;
+ HeapTuple atttup;
+ Form_pg_attribute attStruct;
+ 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;
+
+ /*
+ * 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);
+
+ /*
+ * since we *only* restore default expression evaluated value in pg_attribute.attmissingval
+ * for attribute attnum, not store default expression entry on pg_attrdef
+ * we can't set pg_attribute.atthasdef (Anum_pg_attribute_atthasdef) to true. That's the
+ * main difference with StoreAttrDefault.
+ */
+ if (!attStruct->atthasdef && attStruct->attgenerated == '\0' &&
+ rel->rd_rel->relkind == RELKIND_RELATION && add_column_mode)
+ {
+ 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)
+ missingval = (Datum) 0;
+ else
+ {
+ /* 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);
+
+ /*
+ * Post creation hook for attribute defaults.
+ *
+ * XXX. ALTER TABLE ALTER COLUMN SET/DROP DEFAULT is implemented with a
+ * couple of deletion/creation of the attribute's default entry, so the
+ * callee should check existence of an older version of this entry if it
+ * needs to distinguish.
+ *
+ * XXX is the above comments applys here?
+ */
+ InvokeObjectPostCreateHookArg(AttrDefaultRelationId,
+ RelationGetRelid(rel), attnum, is_internal);
+}
+
/*
* Store a default expression for column attnum of relation rel.
*
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ce7d115667e..83a8f7b6541 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, true);
+ /* 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/pg_attrdef.h b/src/include/catalog/pg_attrdef.h
index 192799cfed7..d0437dcc4e0 100644
--- a/src/include/catalog/pg_attrdef.h
+++ b/src/include/catalog/pg_attrdef.h
@@ -56,6 +56,9 @@ DECLARE_UNIQUE_INDEX_PKEY(pg_attrdef_oid_index, 2657, AttrDefaultOidIndexId, pg_
DECLARE_FOREIGN_KEY((adrelid, adnum), pg_attribute, (attrelid, attnum));
+extern void StoreAttrMissingVal(Relation rel, AttrNumber attnum,
+ Node *expr, bool is_internal,
+ bool add_column_mode);
extern Oid StoreAttrDefault(Relation rel, AttrNumber attnum,
Node *expr, bool is_internal,
bool add_column_mode);
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
