bug when apply fast default mechanism for adding new column over domain with default value
hi.
While looking at ATExecAddColumn, I saw there are two DomainHasConstraints,
which are cache unfriendly,
then I think I found a bug for applying fast default over domain with
default value.
bug demo:
create domain int_domain3 as int check(value > 1) default 11;
create domain int_domain4 as int default 11;
drop table if exists t0;
create table t0(a int);
insert into t0 values(1),(2);
alter table t0 add column b int_domain3;
select * from t0;
a | b
---+----
1 | 11
2 | 11
(2 rows)
alter table t0 add column c int_domain4;
select * from t0;
a | b | c
---+----+---
1 | 11 |
2 | 11 |
(2 rows)
I expect column "c" value also be value 11.
column b type is domain int_domain3, which has a constraint.
As a result, adding column b triggers a table rewrite, ensuring the
domain default value is successfully applied.
column c is domain int_domain4, which has no constraint.
This allows for a fast default mechanism applied to column c, but we cannot.
StoreAttrDefault, pg_attrdef can not cope with domain with default expression.
also domain default expression is stored at pg_tye.typdefaultbin.
Attach a patch to fix this issue by cause it to table rewrite.
also minor refactor to avoid double DomainHasConstraints function call.
jian he <jian.universality@gmail.com> writes:
then I think I found a bug for applying fast default over domain with
default value.
Yeah, that's definitely a bug: it worked correctly in versions
before we invented the attmissingval mechanism.
column b type is domain int_domain3, which has a constraint.
As a result, adding column b triggers a table rewrite, ensuring the
domain default value is successfully applied.
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...
regards, tom lane
On Sat, Mar 1, 2025 at 2:43 PM Tom Lane <tgl@sss.pgh.pa.us> 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.
Attachments:
v1-0001-apply-fast-default-for-adding-new-column-over-dom.patchtext/x-patch; charset=US-ASCII; name=v1-0001-apply-fast-default-for-adding-new-column-over-dom.patchDownload
From 4f87b744b19f34b02817d252cc69666e33da2e18 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
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+TnVoX5LOyCQ@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
jian he <jian.universality@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.
(We could possibly get back down to one update by moving code
in ATExecAddColumn so that we know before calling
InsertPgAttributeTuples whether the column will have a non-null
default, and then we could set atthasdef correctly in the
originally-inserted tuple. That's an optimization though,
not part of the basic bug fix.)
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.
I kind of feel that StoreAttrMissingVal belongs in heap.c;
it's got about nothing to do with pg_attrdef.
regards, tom lane
On Sun, Mar 2, 2025 at 7:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
jian he <jian.universality@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.
Attachments:
v2-0001-apply-fast-default-for-adding-new-column-over-dom.patchtext/x-patch; charset=US-ASCII; name=v2-0001-apply-fast-default-for-adding-new-column-over-dom.patchDownload
From bd79aede786a16e0c105dc28f6a0f7e6f045bed0 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@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+TnVoX5LOyCQ@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
jian he <jian.universality@gmail.com> writes:
On Sun, Mar 2, 2025 at 7:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think the fundamental problem here is that StoreAttrDefault
shouldn't be involved in this in the first place.
i've split missingval related code into StoreAttrMissingVal.
Hm, this does nothing to improve the modularity of the affected
code; if anything it's worse than before. (Fools around for
awhile...) I had something more like the attached in mind.
The idea here is to centralize the control of whether we are
storing a missingval or doing a table rewrite in ATExecAddColumn.
StoreAttrMissingVal has nothing to do with pg_attrdef nor does
StoreAttrDefault have anything to do with attmissingval.
I looked briefly at determining the presence of a default
earlier so we could avoid the extra update of pg_attribute
when both those changes need to happen, but I concluded that
it'd take more refactoring than it's worth. The problem is
the logic way down inside AddRelationNewConstraints that
checks for the eventually-cooked default expression just
being a null constant.
regards, tom lane
Attachments:
v3-0001-Fix-broken-handling-of-domains-in-atthasmissing-l.patchtext/x-diff; charset=us-ascii; name*0=v3-0001-Fix-broken-handling-of-domains-in-atthasmissing-l.p; name*1=atchDownload
From 97c896cefa8a0ea4203974b40bfed6008213b95a Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 2 Mar 2025 15:34:05 -0500
Subject: [PATCH v3 1/2] 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
---
src/backend/catalog/heap.c | 71 ++++++++++++++++--
src/backend/catalog/pg_attrdef.c | 6 ++
src/backend/commands/tablecmds.c | 85 +++++++++++++++-------
src/include/catalog/heap.h | 5 +-
src/test/regress/expected/fast_default.out | 65 +++++++++++++++++
src/test/regress/sql/fast_default.sql | 44 +++++++++++
6 files changed, 242 insertions(+), 34 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 956f196fc95..d28ed3eb396 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -69,6 +69,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"
@@ -2008,6 +2009,69 @@ RelationClearMissing(Relation rel)
table_close(attr_rel, RowExclusiveLock);
}
+/*
+ * StoreAttrMissingVal
+ *
+ * Set the missing value of a single attribute.
+ */
+void
+StoreAttrMissingVal(Relation rel, AttrNumber attnum,
+ Datum missingval, bool missingIsNull)
+{
+ Datum valuesAtt[Natts_pg_attribute] = {0};
+ bool nullsAtt[Natts_pg_attribute] = {0};
+ bool replacesAtt[Natts_pg_attribute] = {0};
+ Relation attrrel;
+ Form_pg_attribute attStruct;
+ HeapTuple atttup,
+ newtup;
+
+ /* This is only supported for plain tables */
+ Assert(rel->rd_rel->relkind == RELKIND_RELATION);
+
+ /* Update the pg_attribute row */
+ attrrel = table_open(AttributeRelationId, RowExclusiveLock);
+
+ atttup = SearchSysCache2(ATTNUM,
+ ObjectIdGetDatum(RelationGetRelid(rel)),
+ Int16GetDatum(attnum));
+ if (!HeapTupleIsValid(atttup)) /* shouldn't happen */
+ elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+ attnum, RelationGetRelid(rel));
+ attStruct = (Form_pg_attribute) GETSTRUCT(atttup);
+
+ if (missingIsNull)
+ {
+ /* if the default value is 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,
+ attStruct->atttypid,
+ attStruct->attlen,
+ attStruct->attbyval,
+ attStruct->attalign));
+ }
+
+ valuesAtt[Anum_pg_attribute_atthasmissing - 1] = BoolGetDatum(!missingIsNull);
+ replacesAtt[Anum_pg_attribute_atthasmissing - 1] = true;
+
+ valuesAtt[Anum_pg_attribute_attmissingval - 1] = missingval;
+ nullsAtt[Anum_pg_attribute_attmissingval - 1] = missingIsNull;
+ replacesAtt[Anum_pg_attribute_attmissingval - 1] = true;
+
+ newtup = heap_modify_tuple(atttup, RelationGetDescr(attrrel),
+ valuesAtt, nullsAtt, replacesAtt);
+ CatalogTupleUpdate(attrrel, &newtup->t_self, newtup);
+
+ /* clean up */
+ ReleaseSysCache(atttup);
+ table_close(attrrel, RowExclusiveLock);
+}
+
/*
* SetAttrMissing
*
@@ -2390,13 +2454,8 @@ AddRelationNewConstraints(Relation rel,
castNode(Const, expr)->constisnull))
continue;
- /* If the DEFAULT is volatile we cannot use a missing value */
- if (colDef->missingMode &&
- contain_volatile_functions_after_planning((Expr *) expr))
- colDef->missingMode = false;
-
defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal,
- colDef->missingMode);
+ false);
cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
cooked->contype = CONSTR_DEFAULT;
diff --git a/src/backend/catalog/pg_attrdef.c b/src/backend/catalog/pg_attrdef.c
index 91dafc81021..a58815a5134 100644
--- a/src/backend/catalog/pg_attrdef.c
+++ b/src/backend/catalog/pg_attrdef.c
@@ -120,6 +120,12 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
valuesAtt[Anum_pg_attribute_atthasdef - 1] = true;
replacesAtt[Anum_pg_attribute_atthasdef - 1] = true;
+ /*
+ * Note: this code is dead so far as core Postgres is concerned,
+ * because no caller passes add_column_mode = true anymore. We keep
+ * it in back branches on the slight chance that some extension is
+ * depending on it.
+ */
if (rel->rd_rel->relkind == RELKIND_RELATION && add_column_mode &&
!attgenerated)
{
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ce7d115667e..cd90f2c7367 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7321,14 +7321,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
rawEnt->attnum = attribute->attnum;
rawEnt->raw_default = copyObject(colDef->raw_default);
-
- /*
- * Attempt to skip a complete table rewrite by storing the specified
- * DEFAULT value outside of the heap. This may be disabled inside
- * AddRelationNewConstraints if the optimization cannot be applied.
- */
- rawEnt->missingMode = (colDef->generated != ATTRIBUTE_GENERATED_STORED);
-
+ rawEnt->missingMode = false; /* XXX vestigial */
rawEnt->generated = colDef->generated;
/*
@@ -7340,13 +7333,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
/* Make the additional catalog changes visible */
CommandCounterIncrement();
-
- /*
- * Did the request for a missing value work? If not we'll have to do a
- * rewrite
- */
- if (!rawEnt->missingMode)
- tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
}
/*
@@ -7363,9 +7349,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
* rejects nulls. If there are any domain constraints then we construct
* an explicit NULL default value that will be passed through
* CoerceToDomain processing. (This is a tad inefficient, since it causes
- * rewriting the table which we really don't have to do, but the present
- * design of domain processing doesn't offer any simple way of checking
- * the constraints more directly.)
+ * rewriting the table which we really wouldn't have to do; but we do it
+ * to preserve the historical behavior that such a failure will be raised
+ * only if the table currently contains some rows.)
*
* Note: we use build_column_default, and not just the cooked default
* returned by AddRelationNewConstraints, so that the right thing happens
@@ -7384,6 +7370,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
*/
if (RELKIND_HAS_STORAGE(relkind))
{
+ bool has_domain_constraints;
+ bool has_missing = false;
+
/*
* For an identity column, we can't use build_column_default(),
* because the sequence ownership isn't set yet. So do it manually.
@@ -7396,14 +7385,13 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
nve->typeId = attribute->atttypid;
defval = (Expr *) nve;
-
- /* must do a rewrite for identity columns */
- tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
}
else
defval = (Expr *) build_column_default(rel, attribute->attnum);
- if (!defval && DomainHasConstraints(attribute->atttypid))
+ /* Build CoerceToDomain(NULL) expression if needed */
+ has_domain_constraints = DomainHasConstraints(attribute->atttypid);
+ if (!defval && has_domain_constraints)
{
Oid baseTypeId;
int32 baseTypeMod;
@@ -7429,18 +7417,61 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
{
NewColumnValue *newval;
+ /* Prepare defval for execution, either here or in Phase 3 */
+ defval = expression_planner(defval);
+
+ /* Add the new default to the newvals list */
newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue));
newval->attnum = attribute->attnum;
- newval->expr = expression_planner(defval);
+ newval->expr = defval;
newval->is_generated = (colDef->generated != '\0');
tab->newvals = lappend(tab->newvals, newval);
- }
- if (DomainHasConstraints(attribute->atttypid))
- tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
+ /*
+ * Attempt to skip a complete table rewrite by storing the
+ * specified DEFAULT value outside of the heap. This is only
+ * allowed for plain relations and non-generated columns, and the
+ * default expression can't be volatile (stable is OK). Note that
+ * contain_volatile_functions deems CoerceToDomain immutable, but
+ * here we consider that coercion to a domain with constraints is
+ * volatile; else it might fail even when the table is empty.
+ */
+ if (rel->rd_rel->relkind == RELKIND_RELATION &&
+ !colDef->generated &&
+ !has_domain_constraints &&
+ !contain_volatile_functions((Node *) defval))
+ {
+ EState *estate;
+ ExprState *exprState;
+ Datum missingval;
+ bool missingIsNull;
+
+ /* Evaluate the default expression */
+ estate = CreateExecutorState();
+ exprState = ExecPrepareExpr(defval, estate);
+ missingval = ExecEvalExpr(exprState,
+ GetPerTupleExprContext(estate),
+ &missingIsNull);
+ /* ... and store it. If it's null, nothing is stored. */
+ StoreAttrMissingVal(rel, attribute->attnum,
+ missingval, missingIsNull);
+ has_missing = !missingIsNull;
+ FreeExecutorState(estate);
+ }
+ else
+ {
+ /*
+ * Failed to use missing mode. We have to do a table rewrite
+ * to install the value --- unless it's a virtual generated
+ * column.
+ */
+ if (colDef->generated != ATTRIBUTE_GENERATED_VIRTUAL)
+ tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
+ }
+ }
- if (!TupleDescAttr(rel->rd_att, attribute->attnum - 1)->atthasmissing)
+ if (!has_missing)
{
/*
* If the new column is NOT NULL, and there is no missing value,
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 19c594458bd..aa6aaaaa9d8 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -29,7 +29,7 @@ typedef struct RawColumnDefault
{
AttrNumber attnum; /* attribute to attach default to */
Node *raw_default; /* default value (untransformed parse tree) */
- bool missingMode; /* true if part of add column processing */
+ bool missingMode; /* obsolete, no longer used */
char generated; /* attgenerated setting */
} RawColumnDefault;
@@ -121,6 +121,9 @@ extern List *AddRelationNotNullConstraints(Relation rel,
List *old_notnulls);
extern void RelationClearMissing(Relation rel);
+
+extern void StoreAttrMissingVal(Relation rel, AttrNumber attnum,
+ Datum missingval, bool missingIsNull);
extern void SetAttrMissing(Oid relid, char *attname, char *value);
extern Node *cookDefault(ParseState *pstate,
diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out
index 272b57e48cd..ccbcdf8403f 100644
--- a/src/test/regress/expected/fast_default.out
+++ b/src/test/regress/expected/fast_default.out
@@ -257,6 +257,71 @@ SELECT comp();
(1 row)
DROP TABLE T;
+-- Test domains with default value for table rewrite.
+CREATE DOMAIN domain1 AS int DEFAULT 11; -- constant
+CREATE DOMAIN domain2 AS int DEFAULT random(min=>10, max=>100); -- volatile
+CREATE DOMAIN domain3 AS text DEFAULT foo(4); -- stable
+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;
+SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
+FROM pg_attribute
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+ attnum | attname | atthasmissing | atthasdef | attmissingval
+--------+---------+---------------+-----------+---------------
+ 1 | a | f | f |
+ 2 | b | t | t | {3}
+(2 rows)
+
+-- 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
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+ 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 attnum, attname, atthasmissing, atthasdef, attmissingval
+FROM pg_attribute
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+ attnum | attname | atthasmissing | atthasdef | attmissingval
+--------+---------+---------------+-----------+---------------
+ 1 | a | f | f |
+ 2 | b | f | t |
+ 3 | c | f | t |
+ 4 | d | f | f |
+ 5 | e | f | f |
+(5 rows)
+
+SELECT a, b, length(c) = 3 as c_ok, d, e >= 10 as e_ok FROM t2;
+ a | b | c_ok | d | e_ok
+---+---+------+-------------------------------+------
+ 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..068dd0bc8aa 100644
--- a/src/test/regress/sql/fast_default.sql
+++ b/src/test/regress/sql/fast_default.sql
@@ -248,6 +248,50 @@ SELECT comp();
DROP TABLE T;
+-- Test domains with default value for table rewrite.
+CREATE DOMAIN domain1 AS int DEFAULT 11; -- constant
+CREATE DOMAIN domain2 AS int DEFAULT random(min=>10, max=>100); -- volatile
+CREATE DOMAIN domain3 AS text DEFAULT foo(4); -- stable
+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;
+
+SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
+FROM pg_attribute
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+
+-- 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
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+
+-- table rewrite should happen
+ALTER TABLE t2 ADD COLUMN e domain2;
+
+SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
+FROM pg_attribute
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+
+SELECT a, b, length(c) = 3 as c_ok, d, e >= 10 as e_ok 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.43.5
v3-0002-Remove-now-dead-code-in-StoreAttrDefault.patchtext/x-diff; charset=us-ascii; name=v3-0002-Remove-now-dead-code-in-StoreAttrDefault.patchDownload
From 84d5360fec2b598369d68d2c0101812488141adc Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 2 Mar 2025 16:56:00 -0500
Subject: [PATCH v3 2/2] Remove now-dead code in StoreAttrDefault().
Get rid of RawColumnDefault.missingMode, too, as we no longer
need that to pass information around.
While here, clean up some sloppy coding in StoreAttrDefault(),
such as failure to use XXXGetDatum macros. These aren't bugs
but they're not good code either.
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
---
src/backend/catalog/heap.c | 5 +-
src/backend/catalog/pg_attrdef.c | 79 +++-----------------------------
src/backend/commands/tablecmds.c | 8 +---
src/include/catalog/heap.h | 1 -
src/include/catalog/pg_attrdef.h | 3 +-
5 files changed, 12 insertions(+), 84 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index d28ed3eb396..1f9250aa065 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2327,7 +2327,7 @@ StoreConstraints(Relation rel, List *cooked_constraints, bool is_internal)
{
case CONSTR_DEFAULT:
con->conoid = StoreAttrDefault(rel, con->attnum, con->expr,
- is_internal, false);
+ is_internal);
break;
case CONSTR_CHECK:
con->conoid =
@@ -2454,8 +2454,7 @@ AddRelationNewConstraints(Relation rel,
castNode(Const, expr)->constisnull))
continue;
- defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal,
- false);
+ defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal);
cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
cooked->contype = CONSTR_DEFAULT;
diff --git a/src/backend/catalog/pg_attrdef.c b/src/backend/catalog/pg_attrdef.c
index a58815a5134..1f95bc6f752 100644
--- a/src/backend/catalog/pg_attrdef.c
+++ b/src/backend/catalog/pg_attrdef.c
@@ -14,17 +14,12 @@
*/
#include "postgres.h"
-#include "access/genam.h"
#include "access/relation.h"
#include "access/table.h"
-#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/indexing.h"
#include "catalog/objectaccess.h"
#include "catalog/pg_attrdef.h"
-#include "executor/executor.h"
-#include "optimizer/optimizer.h"
-#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
#include "utils/rel.h"
@@ -35,22 +30,16 @@
* Store a default expression for column attnum of relation rel.
*
* Returns the OID of the new pg_attrdef tuple.
- *
- * add_column_mode must be true if we are storing the default for a new
- * attribute, and false if it's for an already existing attribute. The reason
- * for this is that the missing value must never be updated after it is set,
- * which can only be when a column is added to the table. Otherwise we would
- * in effect be changing existing tuples.
*/
Oid
StoreAttrDefault(Relation rel, AttrNumber attnum,
- Node *expr, bool is_internal, bool add_column_mode)
+ Node *expr, bool is_internal)
{
char *adbin;
Relation adrel;
HeapTuple tuple;
- Datum values[4];
- static bool nulls[4] = {false, false, false, false};
+ Datum values[Natts_pg_attrdef];
+ static bool nulls[Natts_pg_attrdef] = {false, false, false, false};
Relation attrrel;
HeapTuple atttup;
Form_pg_attribute attStruct;
@@ -72,8 +61,8 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
attrdefOid = GetNewOidWithIndex(adrel, AttrDefaultOidIndexId,
Anum_pg_attrdef_oid);
values[Anum_pg_attrdef_oid - 1] = ObjectIdGetDatum(attrdefOid);
- values[Anum_pg_attrdef_adrelid - 1] = RelationGetRelid(rel);
- values[Anum_pg_attrdef_adnum - 1] = attnum;
+ values[Anum_pg_attrdef_adrelid - 1] = ObjectIdGetDatum(RelationGetRelid(rel));
+ values[Anum_pg_attrdef_adnum - 1] = Int16GetDatum(attnum);
values[Anum_pg_attrdef_adbin - 1] = CStringGetTextDatum(adbin);
tuple = heap_form_tuple(adrel->rd_att, values, nulls);
@@ -92,7 +81,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
/*
* Update the pg_attribute entry for the column to show that a default
- * exists.
+ * exists. (In some cases atthasdef will already be set.)
*/
attrrel = table_open(AttributeRelationId, RowExclusiveLock);
atttup = SearchSysCacheCopy2(ATTNUM,
@@ -105,71 +94,17 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
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;
+ valuesAtt[Anum_pg_attribute_atthasdef - 1] = BoolGetDatum(true);
replacesAtt[Anum_pg_attribute_atthasdef - 1] = true;
- /*
- * Note: this code is dead so far as core Postgres is concerned,
- * because no caller passes add_column_mode = true anymore. We keep
- * it in back branches on the slight chance that some extension is
- * depending on it.
- */
- 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);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cd90f2c7367..cf519fb77d9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -967,7 +967,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
rawEnt->attnum = attnum;
rawEnt->raw_default = colDef->raw_default;
- rawEnt->missingMode = false;
rawEnt->generated = colDef->generated;
rawDefaults = lappend(rawDefaults, rawEnt);
attr->atthasdef = true;
@@ -7321,7 +7320,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
rawEnt->attnum = attribute->attnum;
rawEnt->raw_default = copyObject(colDef->raw_default);
- rawEnt->missingMode = false; /* XXX vestigial */
rawEnt->generated = colDef->generated;
/*
@@ -8075,7 +8073,6 @@ ATExecColumnDefault(Relation rel, const char *colName,
rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
rawEnt->attnum = attnum;
rawEnt->raw_default = newDefault;
- rawEnt->missingMode = false;
rawEnt->generated = '\0';
/*
@@ -8113,7 +8110,7 @@ ATExecCookedColumnDefault(Relation rel, AttrNumber attnum,
RemoveAttrDefault(RelationGetRelid(rel), attnum, DROP_RESTRICT, false,
true);
- (void) StoreAttrDefault(rel, attnum, newDefault, true, false);
+ (void) StoreAttrDefault(rel, attnum, newDefault, true);
ObjectAddressSubSet(address, RelationRelationId,
RelationGetRelid(rel), attnum);
@@ -8576,7 +8573,6 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
rawEnt->attnum = attnum;
rawEnt->raw_default = newExpr;
- rawEnt->missingMode = false;
rawEnt->generated = attgenerated;
/* Store the generated expression */
@@ -14128,7 +14124,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
RemoveAttrDefault(RelationGetRelid(rel), attnum, DROP_RESTRICT, true,
true);
- StoreAttrDefault(rel, attnum, defaultexpr, true, false);
+ (void) StoreAttrDefault(rel, attnum, defaultexpr, true);
}
ObjectAddressSubSet(address, RelationRelationId,
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index aa6aaaaa9d8..d8fbbf68a5b 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -29,7 +29,6 @@ typedef struct RawColumnDefault
{
AttrNumber attnum; /* attribute to attach default to */
Node *raw_default; /* default value (untransformed parse tree) */
- bool missingMode; /* obsolete, no longer used */
char generated; /* attgenerated setting */
} RawColumnDefault;
diff --git a/src/include/catalog/pg_attrdef.h b/src/include/catalog/pg_attrdef.h
index 192799cfed7..3067a8357c0 100644
--- a/src/include/catalog/pg_attrdef.h
+++ b/src/include/catalog/pg_attrdef.h
@@ -57,8 +57,7 @@ DECLARE_FOREIGN_KEY((adrelid, adnum), pg_attribute, (attrelid, attnum));
extern Oid StoreAttrDefault(Relation rel, AttrNumber attnum,
- Node *expr, bool is_internal,
- bool add_column_mode);
+ Node *expr, bool is_internal);
extern void RemoveAttrDefault(Oid relid, AttrNumber attnum,
DropBehavior behavior,
bool complain, bool internal);
--
2.43.5
On Mon, Mar 3, 2025 at 6:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
jian he <jian.universality@gmail.com> writes:
On Sun, Mar 2, 2025 at 7:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I think the fundamental problem here is that StoreAttrDefault
shouldn't be involved in this in the first place.i've split missingval related code into StoreAttrMissingVal.
Hm, this does nothing to improve the modularity of the affected
code; if anything it's worse than before. (Fools around for
awhile...) I had something more like the attached in mind.
The idea here is to centralize the control of whether we are
storing a missingval or doing a table rewrite in ATExecAddColumn.
StoreAttrMissingVal has nothing to do with pg_attrdef nor does
StoreAttrDefault have anything to do with attmissingval.
your patch looks very good!!!
within ATExecAddColumn, we can
if (!missingIsNull)
StoreAttrMissingVal(rel, attribute->attnum, missingval, missingIsNull);
to save some cycles?
+ /* ... and store it. If it's null, nothing is stored. */
+ StoreAttrMissingVal(rel, attribute->attnum,
+ missingval, missingIsNull);
+ has_missing = !missingIsNull;
+ FreeExecutorState(estate);
do we need pfree missingval?+ else
+ {
+ /*
+ * Failed to use missing mode. We have to do a table rewrite
+ * to install the value --- unless it's a virtual generated
+ * column.
+ */
+ if (colDef->generated != ATTRIBUTE_GENERATED_VIRTUAL)
+ tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
+ }
maybe here add comments mentioning that the identity column needs table rewrite.
since we removed ``tab->rewrite |= AT_REWRITE_DEFAULT_VAL;``
in the if (colDef->identity) branch.StoreAttrDefault is at the end of
ATExecAlterColumnType, ATExecCookedColumnDefault, StoreConstraints
these function later will call CommandCounterIncrement
I also checked AddRelationNewConstraints surrounding code.
overall i think StoreAttrDefault doesn't have CommandCounterIncrement
should be fine.
looking at DefineRelation comments:
* We can set the atthasdef flags now in the tuple descriptor; this just
* saves StoreAttrDefault from having to do an immediate update of the
* pg_attribute rows.
this seems not right?
DefineRelation->heap_create_with_catalog->heap_create->RelationBuildLocalRelation->CreateTupleDescCopy
don't copy atthasdef.
RelationBuildLocalRelation later didn't touch atthasdef.
populate_compact_attribute didn't touch atthasdef.
so StoreAttrDefault has to update that pg_attribute row.
somehow related, if you look at AddRelationNewConstraints.
```
foreach_ptr(RawColumnDefault, colDef, newColDefaults)
{
defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal);
cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
cooked->contype = CONSTR_DEFAULT;
cooked->conoid = defOid;
}
maybe we can minor change comments in struct CookedConstraint.conoid.
We can mention: if contype is CONSTR_DEFAULT then it is pg_attrdef.oid
for the default expression.
On Mon, Mar 3, 2025 at 4:45 PM jian he <jian.universality@gmail.com> wrote:
looking at DefineRelation comments:
* We can set the atthasdef flags now in the tuple descriptor; this just
* saves StoreAttrDefault from having to do an immediate update of the
* pg_attribute rows.
this seems not right?
DefineRelation->heap_create_with_catalog->heap_create->RelationBuildLocalRelation->CreateTupleDescCopy
don't copy atthasdef.
RelationBuildLocalRelation later didn't touch atthasdef.
populate_compact_attribute didn't touch atthasdef.
so StoreAttrDefault has to update that pg_attribute row.
CREATE TABLE (COLUMN x DEFAULT y):
for each (Relation rel, AttrNumber attnum), we can enter StoreAttrDefault once.
Also for the above quoted reason,
in DefineRelation, we didn't change pg_attribute.atthasdef before
entering StoreAttrDefault.
ALTER TABLE ALTER COLUMN SET/DROP DEFAULT:
in ATExecColumnDefault will first do RemoveAttrDefault, then re add
the default expression.
In this case, in StoreAttrDefault: attStruct->atthasdef will be false.
overall, i think DefineRelation about StoreAttrDefault comments can be removed.
and StoreAttrDefault, "if (!attStruct->atthasdef)" part logic can also
be changed.
The attached is the minor change I came up with, based on v3-0001 and v3-0002.
Attachments:
v3-0001-minor-refactoring-DefineRelation-StoreAttrDefa.no-cfbotapplication/octet-stream; name=v3-0001-minor-refactoring-DefineRelation-StoreAttrDefa.no-cfbotDownload
From 71be37ae101836a5dd272a81e32df5c2d53c3f4a Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Mon, 3 Mar 2025 22:15:00 +0800
Subject: [PATCH v3 1/1] minor refactoring DefineRelation, StoreAttrDefault
make StoreAttrDefault as the single place for setting pg_attribute.atthasdef to true.
---
src/backend/catalog/pg_attrdef.c | 23 ++++++++++++-----------
src/backend/commands/tablecmds.c | 8 --------
2 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/src/backend/catalog/pg_attrdef.c b/src/backend/catalog/pg_attrdef.c
index 1f95bc6f752..706cc82e296 100644
--- a/src/backend/catalog/pg_attrdef.c
+++ b/src/backend/catalog/pg_attrdef.c
@@ -47,6 +47,9 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
Oid attrdefOid;
ObjectAddress colobject,
defobject;
+ Datum valuesAtt[Natts_pg_attribute] = {0};
+ bool nullsAtt[Natts_pg_attribute] = {0};
+ bool replacesAtt[Natts_pg_attribute] = {0};
adrel = table_open(AttrDefaultRelationId, RowExclusiveLock);
@@ -92,20 +95,18 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
attnum, RelationGetRelid(rel));
attStruct = (Form_pg_attribute) GETSTRUCT(atttup);
attgenerated = attStruct->attgenerated;
- if (!attStruct->atthasdef)
- {
- Datum valuesAtt[Natts_pg_attribute] = {0};
- bool nullsAtt[Natts_pg_attribute] = {0};
- bool replacesAtt[Natts_pg_attribute] = {0};
+ if (attStruct->atthasdef)
+ elog(ERROR, "expect atthasdef be false for attribute %d of relation %u",
+ attnum, RelationGetRelid(rel));
- valuesAtt[Anum_pg_attribute_atthasdef - 1] = BoolGetDatum(true);
- replacesAtt[Anum_pg_attribute_atthasdef - 1] = true;
+ valuesAtt[Anum_pg_attribute_atthasdef - 1] = BoolGetDatum(true);
+ replacesAtt[Anum_pg_attribute_atthasdef - 1] = true;
- atttup = heap_modify_tuple(atttup, RelationGetDescr(attrrel),
- valuesAtt, nullsAtt, replacesAtt);
+ atttup = heap_modify_tuple(atttup, RelationGetDescr(attrrel),
+ valuesAtt, nullsAtt, replacesAtt);
+
+ CatalogTupleUpdate(attrrel, &atttup->t_self, atttup);
- CatalogTupleUpdate(attrrel, &atttup->t_self, atttup);
- }
table_close(attrrel, RowExclusiveLock);
heap_freetuple(atttup);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cf519fb77d9..74a23242044 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -941,10 +941,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
* while raw defaults go into a list of RawColumnDefault structs that will
* be processed by AddRelationNewConstraints. (We can't deal with raw
* expressions until we can do transformExpr.)
- *
- * We can set the atthasdef flags now in the tuple descriptor; this just
- * saves StoreAttrDefault from having to do an immediate update of the
- * pg_attribute rows.
*/
rawDefaults = NIL;
cookedDefaults = NIL;
@@ -953,10 +949,8 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
foreach(listptr, stmt->tableElts)
{
ColumnDef *colDef = lfirst(listptr);
- Form_pg_attribute attr;
attnum++;
- attr = TupleDescAttr(descriptor, attnum - 1);
if (colDef->raw_default != NULL)
{
@@ -969,7 +963,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
rawEnt->raw_default = colDef->raw_default;
rawEnt->generated = colDef->generated;
rawDefaults = lappend(rawDefaults, rawEnt);
- attr->atthasdef = true;
}
else if (colDef->cooked_default != NULL)
{
@@ -987,7 +980,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
cooked->inhcount = 0; /* ditto */
cooked->is_no_inherit = false;
cookedDefaults = lappend(cookedDefaults, cooked);
- attr->atthasdef = true;
}
populate_compact_attribute(descriptor, attnum - 1);
--
2.34.1
jian he <jian.universality@gmail.com> writes:
within ATExecAddColumn, we can
if (!missingIsNull)
StoreAttrMissingVal(rel, attribute->attnum, missingval, missingIsNull);
to save some cycles?
Probably not really worth it: surely that's going to be a very
infrequent edge case. We already eliminated cases with a simple
null-Constant default, so we'd only get here with a more complex
expression that evaluates to null.
+ /* ... and store it. If it's null, nothing is stored. */ + StoreAttrMissingVal(rel, attribute->attnum, + missingval, missingIsNull); + has_missing = !missingIsNull; + FreeExecutorState(estate); do we need pfree missingval?
I don't see the point. It's not like this code leaks nothing else,
and we shouldn't be running in a long-lived memory context.
maybe here add comments mentioning that the identity column needs table rewrite.
since we removed ``tab->rewrite |= AT_REWRITE_DEFAULT_VAL;``
in the if (colDef->identity) branch.
We don't need that because it's not a special case anymore: the
NextValueExpr will correctly be seen as a volatile default.
The existing code needs that hack because it creates the
NextValueExpr after the point at which expression volatility
is checked, but that's just poor design.
looking at DefineRelation comments:
* We can set the atthasdef flags now in the tuple descriptor; this just
* saves StoreAttrDefault from having to do an immediate update of the
* pg_attribute rows.
this seems not right?
DefineRelation->heap_create_with_catalog->heap_create->RelationBuildLocalRelation->CreateTupleDescCopy
don't copy atthasdef.
RelationBuildLocalRelation later didn't touch atthasdef.
populate_compact_attribute didn't touch atthasdef.
so StoreAttrDefault has to update that pg_attribute row.
You are right: that code has no effect, and if it did have an
effect it would be wrong in the case where somebody writes
"DEFAULT NULL" explicitly. We'd end with atthasdef set and
nothing in pg_attrdef, which would upset various places later.
So we should remove that comment and also the adjustments of
atthasdef. That seems like an independent patch though.
regards, tom lane
I wrote:
jian he <jian.universality@gmail.com> writes:
within ATExecAddColumn, we can
if (!missingIsNull)
StoreAttrMissingVal(rel, attribute->attnum, missingval, missingIsNull);
to save some cycles?
Probably not really worth it: surely that's going to be a very
infrequent edge case.
On second thought, it is worth doing like that, not on speed
grounds but because we can make StoreAttrMissingVal simpler
by not handling the is-null case at all.
Pushed with that adjustment and some other minor polishing.
regards, tom lane
Hello Tom,
03.03.2025 20:38, Tom Lane wrote:
Pushed with that adjustment and some other minor polishing.
I've discovered that 95f650674 introduced a defect similar to bug #18297,
but this time with DEFAULT. Namely, the following script:
CREATE TABLE a (aa text);
CREATE TABLE c (cc text) INHERITS (a);
CREATE TABLE d (dd text) INHERITS (c, a);
ALTER TABLE a ADD COLUMN i int DEFAULT 1;
fails with:
ERROR: XX000: tuple already updated by self
LOCATION: simple_heap_update, heapam.c:4421
Best regards,
Alexander Lakhin
Neon (https://neon.tech)
Alexander Lakhin <exclusion@gmail.com> writes:
I've discovered that 95f650674 introduced a defect similar to bug #18297,
but this time with DEFAULT. Namely, the following script:
CREATE TABLE a (aa text);
CREATE TABLE c (cc text) INHERITS (a);
CREATE TABLE d (dd text) INHERITS (c, a);
ALTER TABLE a ADD COLUMN i int DEFAULT 1;
fails with:
ERROR: XX000: tuple already updated by self
LOCATION: simple_heap_update, heapam.c:4421
Hmm, yeah. The failing call is here:
/* Bump the existing child att's inhcount */
...
CatalogTupleUpdate(attrdesc, &tuple->t_self, tuple);
so I think you're right that that code path is now short a
CommandCounterIncrement() somewhere. I'll look tomorrow if
nobody beats me to it.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> 于2025年4月2日周三 09:05写道：
Alexander Lakhin <exclusion@gmail.com> writes:
I've discovered that 95f650674 introduced a defect similar to bug #18297,
but this time with DEFAULT. Namely, the following script:
CREATE TABLE a (aa text);
CREATE TABLE c (cc text) INHERITS (a);
CREATE TABLE d (dd text) INHERITS (c, a);
ALTER TABLE a ADD COLUMN i int DEFAULT 1;fails with:
ERROR: XX000: tuple already updated by self
LOCATION: simple_heap_update, heapam.c:4421Hmm, yeah. The failing call is here:
/* Bump the existing child att's inhcount */
...
CatalogTupleUpdate(attrdesc, &tuple->t_self, tuple);so I think you're right that that code path is now short a
CommandCounterIncrement() somewhere. I'll look tomorrow if
nobody beats me to it.
Yes, when table a process its children, which are table c and table d.
Table c is first to be done.
At the same time, table d is also child of table c, so after updating own
pg_attribute tuple, table c will
process its child table d. And table d update its pg_attribute catalog
tuple.
After finishing table c, the logic returning to continue to process table
a's children, which this time is table d.
Between table d pg_attribute tuple updated as child of table c and updating
table d pg_attribute tuple again as child of table a,
there is no call CommandCounterIncrement().
So let's add CommandCounterIncrement() after calling StoreAttrMissingVal().
--
Thanks, Tender Wang
Attachments:
0001-Fix-tuple-already-updated-by-self-issue.patchtext/plain; charset=US-ASCII; name=0001-Fix-tuple-already-updated-by-self-issue.patchDownload
From ee2bc290cab83daf0ebecf9f29cc23539ba19741 Mon Sep 17 00:00:00 2001
From: Tender Wang <tndrwang@gmail.com>
Date: Wed, 2 Apr 2025 09:52:22 +0800
Subject: [PATCH] Fix "tuple already updated by self" issue.
---
src/backend/commands/tablecmds.c | 2 ++
src/test/regress/expected/inherit.out | 17 +++++++++++++++++
src/test/regress/sql/inherit.sql | 8 ++++++++
3 files changed, 27 insertions(+)
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 10624353b0a..67e0aabf4a7 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7518,6 +7518,8 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
{
StoreAttrMissingVal(rel, attribute->attnum, missingval);
has_missing = true;
+ /* Make above changes visible */
+ CommandCounterIncrement();
}
FreeExecutorState(estate);
}
diff --git a/src/test/regress/expected/inherit.out b/src/test/regress/expected/inherit.out
index 4d07d0bd79b..ba4327e7acb 100644
--- a/src/test/regress/expected/inherit.out
+++ b/src/test/regress/expected/inherit.out
@@ -1108,6 +1108,23 @@ Child tables: inhtb,
inhtd
DROP TABLE inhta, inhtb, inhtc, inhtd;
+-- Test for adding a column wiht DEFAULT value
+CREATE TABLE inhtda (aa text);
+CREATE TABLE inhtdc (cc text) INHERITS (inhtda);
+CREATE TABLE inhtdd (dd text) INHERITS (inhtdc, inhtda);
+NOTICE: merging multiple inherited definitions of column "aa"
+ALTER TABLE inhtda ADD COLUMN i int DEFAULT 1;
+NOTICE: merging definition of column "i" for child "inhtdd"
+\d+ inhtda
+ Table "public.inhtda"
+ Column | Type | Collation | Nullable | Default | Storage | Stats target | Description
+--------+---------+-----------+----------+---------+----------+--------------+-------------
+ aa | text | | | | extended | |
+ i | integer | | | 1 | plain | |
+Child tables: inhtdc,
+ inhtdd
+
+DROP TABLE inhtda, inhtdc, inhtdd;
-- Test for renaming in diamond inheritance
CREATE TABLE inht2 (x int) INHERITS (inht1);
CREATE TABLE inht3 (y int) INHERITS (inht1);
diff --git a/src/test/regress/sql/inherit.sql b/src/test/regress/sql/inherit.sql
index 941189761fd..d9fd6340798 100644
--- a/src/test/regress/sql/inherit.sql
+++ b/src/test/regress/sql/inherit.sql
@@ -384,6 +384,14 @@ ALTER TABLE inhta ADD COLUMN i int;
\d+ inhta
DROP TABLE inhta, inhtb, inhtc, inhtd;
+-- Test for adding a column wiht DEFAULT value
+CREATE TABLE inhtda (aa text);
+CREATE TABLE inhtdc (cc text) INHERITS (inhtda);
+CREATE TABLE inhtdd (dd text) INHERITS (inhtdc, inhtda);
+ALTER TABLE inhtda ADD COLUMN i int DEFAULT 1;
+\d+ inhtda
+DROP TABLE inhtda, inhtdc, inhtdd;
+
-- Test for renaming in diamond inheritance
CREATE TABLE inht2 (x int) INHERITS (inht1);
CREATE TABLE inht3 (y int) INHERITS (inht1);
--
2.34.1
Tender Wang <tndrwang@gmail.com> writes:
So let's add CommandCounterIncrement() after calling StoreAttrMissingVal().
Pushed with minor editorialization. I thought for a bit about whether
the CCI call should be inside StoreAttrMissingVal; but its cohorts in
heap.c don't do their own CCI calls, so this way seems the least
surprising.
regards, tom lane
Tom Lane <tgl@sss.pgh.pa.us> 于2025年4月2日周三 23:15写道：
Tender Wang <tndrwang@gmail.com> writes:
So let's add CommandCounterIncrement() after calling
StoreAttrMissingVal().
Pushed with minor editorialization. I thought for a bit about whether
the CCI call should be inside StoreAttrMissingVal; but its cohorts in
heap.c don't do their own CCI calls, so this way seems the least
surprising.
Yes, there is no direct call CCI in heap.c. I also noticed this.
Thanks for pushing.
--
Thanks, Tender Wang