On Tue, Mar 24, 2026 at 12:18 AM Viktor Holmberg <[email protected]> wrote: > > This appears to address some of my comments but not this one? > > CREATE DOMAIN domain1 AS INT CHECK(VALUE > 1) NOT NULL; > CREATE TABLE t_const_using(a INT); > INSERT INTO t_const_using VALUES(-2); > ALTER TABLE t_const_using ALTER COLUMN a SET DATA TYPE domain1 USING 5; > SELECT a FROM t_const_using; -- should be 5 after rewrite, not -2 > a > ---- > -2
Sure, these tests are added to v6. -- jian https://www.enterprisedb.com/
From e4dcc252ba665fb8b1799b61f8ee10130a0ff58c Mon Sep 17 00:00:00 2001 From: jian he <[email protected]> Date: Tue, 24 Mar 2026 09:11:20 +0800 Subject: [PATCH v6 1/1] Skipping table rewrites for changing column types in some cases If a table rewrite is required, there's nothing we can do about it. We can add a new boolean field need_compute to NewColumnValue. This field is currently set to true only when changing an existing column's type to a constrained domain. In such cases, a table scan alone is sufficient. discussion: https://postgr.es/m/cacjufxfx0dupbf5+dbnf3mxcfntz1y7jpt11+tcd_fcyadh...@mail.gmail.com commitfest: https://commitfest.postgresql.org/patch/5907 --- doc/src/sgml/ref/alter_table.sgml | 4 +- src/backend/commands/tablecmds.c | 81 ++++++++++++++++++--- src/test/regress/expected/event_trigger.out | 13 ++++ src/test/regress/expected/fast_default.out | 40 ++++++++++ src/test/regress/sql/event_trigger.sql | 11 +++ src/test/regress/sql/fast_default.sql | 28 +++++++ 6 files changed, 167 insertions(+), 10 deletions(-) diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 8591a6b5014..b794cccd33f 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -1668,7 +1668,9 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM if the <literal>USING</literal> clause does not change the column contents and the old type is either binary coercible to the new type or an unconstrained domain over the new type, a table rewrite is not - needed. However, indexes will still be rebuilt unless the system + needed. If the new type is a constrained domain over the old type, table + rewrite is not needed, however table scan is required. + Even if table rewrite is not needed, indexes will still be rebuilt unless the system can verify that the new index would be logically equivalent to the existing one. For example, if the collation for a column has been changed, an index rebuild is required because the new sort diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 219f604df7b..e8a1c1551d1 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -232,6 +232,12 @@ typedef struct NewConstraint * are just copied from the old table during ATRewriteTable. Note that the * expr is an expression over *old* table values, except when is_generated * is true; then it is an expression over columns of the *new* tuple. + * + * If need_compute is true, using table scan to evaluate the new column value in + * Phase 3, no table rewrite is required. Currently, this is only used in the + * ALTER COLUMN SET DATA TYPE command, where the column’s data type is being + * changed to a constrained domain. need_compute is irrelevant in case of table + * rewrite. */ typedef struct NewColumnValue { @@ -239,6 +245,7 @@ typedef struct NewColumnValue Expr *expr; /* expression to compute */ ExprState *exprstate; /* execution state */ bool is_generated; /* is it a GENERATED expression? */ + bool need_compute; } NewColumnValue; /* @@ -658,7 +665,7 @@ static void ATPrepAlterColumnType(List **wqueue, bool recurse, bool recursing, AlterTableCmd *cmd, LOCKMODE lockmode, AlterTableUtilityContext *context); -static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno); +static bool ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno, bool *scan_only); static ObjectAddress ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel, AlterTableCmd *cmd, LOCKMODE lockmode); static void RememberAllDependentForRebuilding(AlteredTableInfo *tab, AlterTableType subtype, @@ -6077,7 +6084,7 @@ ATRewriteTables(AlterTableStmt *parsetree, List **wqueue, LOCKMODE lockmode, * rebuild data. */ if (tab->constraints != NIL || tab->verify_new_notnull || - tab->partition_constraint != NULL) + tab->partition_constraint != NULL || tab->newvals) ATRewriteTable(tab, InvalidOid); /* @@ -6272,6 +6279,9 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap) /* expr already planned */ ex->exprstate = ExecInitExpr(ex->expr, NULL); + + if (ex->need_compute) + needscan = true; } notnull_attrs = notnull_virtual_attrs = NIL; @@ -6500,6 +6510,48 @@ ATRewriteTable(AlteredTableInfo *tab, Oid OIDNewHeap) * new constraints etc. */ insertslot = oldslot; + + /* + * To safely evaluate the NewColumnValue expression against + * the original table slot, temporarily switch oldslot's + * tts_tupleDescriptor to oldTupDesc, and then switch it back + * to newTupDesc afterwards. + * + * Rationale: 1. If there is no table rewrite, the contents of + * oldslot are guaranteed to be fully compatible with + * oldTupDesc. See the comments in slot_deform_heap_tuple + * regarding slot_getmissingattrs. + * + * 2. Passing the updated newTupDesc into ExecEvalExpr would + * cause CheckVarSlotCompatibility to fail, we have to + * temporarily use oldTupDesc + */ + if (tab->newvals != NIL) + { + bool isnull pg_attribute_unused(); + + insertslot->tts_tupleDescriptor = oldTupDesc; + econtext->ecxt_scantuple = insertslot; + + foreach(l, tab->newvals) + { + NewColumnValue *ex = lfirst(l); + + if (!ex->need_compute) + continue; + + /* + * ExecEvalExprNoReturn cannot be used here because + * the expression was compiled via ExecInitExpr. We + * only need to check if oldslot satisfies new + * expression (NewColumnValue->expr), so it's fine to + * ignore value returned by ExecEvalExpr. + */ + (void) ExecEvalExpr(ex->exprstate, econtext, &isnull); + } + + insertslot->tts_tupleDescriptor = newTupDesc; + } } /* Now check any constraints on the possibly-changed tuple */ @@ -14713,6 +14765,8 @@ ATPrepAlterColumnType(List **wqueue, else if (tab->relkind == RELKIND_RELATION || tab->relkind == RELKIND_PARTITIONED_TABLE) { + bool scan_only = false; + /* * Set up an expression to transform the old data value to the new * type. If a USING option was given, use the expression as @@ -14777,9 +14831,15 @@ ATPrepAlterColumnType(List **wqueue, newval->expr = (Expr *) transform; newval->is_generated = false; - tab->newvals = lappend(tab->newvals, newval); - if (ATColumnChangeRequiresRewrite(transform, attnum)) + if (ATColumnChangeRequiresRewrite(transform, attnum, &scan_only)) + { tab->rewrite |= AT_REWRITE_COLUMN_REWRITE; + newval->need_compute = true; + } + else if (!tab->rewrite && scan_only) + newval->need_compute = true; + + tab->newvals = lappend(tab->newvals, newval); } else if (transform) ereport(ERROR, @@ -14910,15 +14970,17 @@ ATPrepAlterColumnType(List **wqueue, * rewrite in these cases: * * - the old type is binary coercible to the new type - * - the new type is an unconstrained domain over the old type * - {NEW,OLD} or {OLD,NEW} is {timestamptz,timestamp} and the timezone is UTC * * In the case of a constrained domain, we could get by with scanning the - * table and checking the constraint rather than actually rewriting it, but we - * don't currently try to do that. + * table and checking the constraint rather than actually rewriting it, + * in that case, scan_only will set to true. + * + * The caller must initialize *scan_only to false before calling. + * If any constrained domain is found, *scan_only is set to true. */ static bool -ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno) +ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno, bool *scan_only) { Assert(expr != NULL); @@ -14934,7 +14996,8 @@ ATColumnChangeRequiresRewrite(Node *expr, AttrNumber varattno) CoerceToDomain *d = (CoerceToDomain *) expr; if (DomainHasConstraints(d->resulttype, NULL)) - return true; + *scan_only = true; + expr = (Node *) d->arg; } else if (IsA(expr, FuncExpr)) diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out index f897b079e67..41b7c34d0ff 100644 --- a/src/test/regress/expected/event_trigger.out +++ b/src/test/regress/expected/event_trigger.out @@ -609,13 +609,26 @@ DROP MATERIALIZED VIEW heapmv; -- shouldn't trigger a table_rewrite event alter table rewriteme alter column foo type numeric(12,4); begin; +create domain d_ts as timestamp check(value is null); +create domain d_tsnn as timestamp check(value is not null); +create domain d_tstz as timestamptz check(value is null); +SAVEPOINT s1; +set timezone to 'UTC'; +alter table rewriteme alter column bar type d_tsnn; -- error, no table rewrite +ERROR: value for domain d_tsnn violates check constraint "d_tsnn_check" +ROLLBACK TO SAVEPOINT s1; set timezone to 'UTC'; alter table rewriteme alter column bar type timestamp; +alter table rewriteme alter column bar type d_ts; set timezone to '0'; alter table rewriteme alter column bar type timestamptz; +alter table rewriteme alter column bar type d_tstz; +alter table rewriteme add column anotherone timestamptz; set timezone to 'Europe/London'; alter table rewriteme alter column bar type timestamp; -- does rewrite NOTICE: Table 'rewriteme' is being rewritten (reason = 4) +alter table rewriteme alter column anotherone type d_ts; -- does rewrite +NOTICE: Table 'rewriteme' is being rewritten (reason = 4) rollback; -- typed tables are rewritten when their type changes. Don't emit table -- name, because firing order is not stable. diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out index ffbc47089b1..3f3b59f675e 100644 --- a/src/test/regress/expected/fast_default.out +++ b/src/test/regress/expected/fast_default.out @@ -384,6 +384,46 @@ DROP DOMAIN domain7; DROP DOMAIN domain8; DROP DOMAIN domain9; DROP FUNCTION foo(INT); +-- Test chaning column data type to constrained domain +CREATE DOMAIN domain1 AS INT CHECK(VALUE > 1) NOT NULL; +CREATE DOMAIN domain2 AS domain1 CHECK(VALUE > 1) NOT NULL; +CREATE DOMAIN domain3 AS domain1 CHECK(VALUE > random(min=>10, max=>10)) CHECK(VALUE > 1) NOT NULL; +CREATE DOMAIN domain4 AS TEXT COLLATE "POSIX" CHECK (value <> 'hello'); +CREATE DOMAIN domain5 AS int[] NOT NULL; +CREATE DOMAIN domain6 AS varchar COLLATE "POSIX" CHECK (value <> 'hello'); +CREATE TABLE t22(a INT, b INT, c text COLLATE "C", col1 INT[]); +INSERT INTO t22 VALUES(-2, -1, 'hello', NULL); +-- all of the following no need table rewrite, some may fail because of domain constraint violation +ALTER TABLE t22 ALTER COLUMN a SET DATA TYPE domain2 USING ((a))::domain2; +ERROR: value for domain domain2 violates check constraint "domain1_check" +ALTER TABLE t22 ALTER COLUMN a SET DATA TYPE domain3 USING ((a))::domain3; +ERROR: value for domain domain3 violates check constraint "domain1_check" +ALTER TABLE t22 ALTER COLUMN c SET DATA TYPE domain4; +ERROR: value for domain domain4 violates check constraint "domain4_check" +ALTER TABLE t22 ALTER COLUMN c SET DATA TYPE domain6; +ERROR: value for domain domain6 violates check constraint "domain6_check" +ALTER TABLE t22 ALTER COLUMN col1 SET DATA TYPE domain5 USING col1::int4[]::domain5; +ERROR: domain domain5 does not allow null values +UPDATE t22 set col1 = '{-2,null}'; +ALTER TABLE t22 ALTER COLUMN col1 SET DATA TYPE domain5 USING col1::int4[]::domain5; +ALTER TABLE t22 ALTER COLUMN col1 SET DATA TYPE int4[] USING col1::int4[]; +-- all of the above no need table rewrite +ALTER TABLE t22 ALTER COLUMN a SET DATA TYPE domain2 USING ((b))::domain2; -- table rewrite +NOTICE: rewriting table t22 for reason 4 +ERROR: value for domain domain2 violates check constraint "domain1_check" +ALTER TABLE t22 ALTER COLUMN b SET DATA TYPE domain2 USING -2; -- table rewrite, fail +NOTICE: rewriting table t22 for reason 4 +ERROR: value for domain domain2 violates check constraint "domain1_check" +ALTER TABLE t22 ALTER COLUMN b SET DATA TYPE domain2 USING 15; -- table rewrite, ok +NOTICE: rewriting table t22 for reason 4 +TABLE t22; + a | b | c | col1 +----+----+-------+----------- + -2 | 15 | hello | {-2,NULL} +(1 row) + +DROP TABLE t22; +DROP DOMAIN domain1, domain2, domain3, domain4, domain5, domain6; -- Fall back to full rewrite for volatile expressions CREATE TABLE T(pk INT NOT NULL PRIMARY KEY); INSERT INTO T VALUES (1); diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql index 32e9bb58c5e..0e5c8607bd9 100644 --- a/src/test/regress/sql/event_trigger.sql +++ b/src/test/regress/sql/event_trigger.sql @@ -445,12 +445,23 @@ DROP MATERIALIZED VIEW heapmv; -- shouldn't trigger a table_rewrite event alter table rewriteme alter column foo type numeric(12,4); begin; +create domain d_ts as timestamp check(value is null); +create domain d_tsnn as timestamp check(value is not null); +create domain d_tstz as timestamptz check(value is null); +SAVEPOINT s1; +set timezone to 'UTC'; +alter table rewriteme alter column bar type d_tsnn; -- error, no table rewrite +ROLLBACK TO SAVEPOINT s1; set timezone to 'UTC'; alter table rewriteme alter column bar type timestamp; +alter table rewriteme alter column bar type d_ts; set timezone to '0'; alter table rewriteme alter column bar type timestamptz; +alter table rewriteme alter column bar type d_tstz; +alter table rewriteme add column anotherone timestamptz; set timezone to 'Europe/London'; alter table rewriteme alter column bar type timestamp; -- does rewrite +alter table rewriteme alter column anotherone type d_ts; -- does rewrite rollback; -- typed tables are rewritten when their type changes. Don't emit table diff --git a/src/test/regress/sql/fast_default.sql b/src/test/regress/sql/fast_default.sql index 8ff29cf2697..5272c1b73a7 100644 --- a/src/test/regress/sql/fast_default.sql +++ b/src/test/regress/sql/fast_default.sql @@ -345,6 +345,34 @@ DROP DOMAIN domain8; DROP DOMAIN domain9; DROP FUNCTION foo(INT); +-- Test chaning column data type to constrained domain +CREATE DOMAIN domain1 AS INT CHECK(VALUE > 1) NOT NULL; +CREATE DOMAIN domain2 AS domain1 CHECK(VALUE > 1) NOT NULL; +CREATE DOMAIN domain3 AS domain1 CHECK(VALUE > random(min=>10, max=>10)) CHECK(VALUE > 1) NOT NULL; +CREATE DOMAIN domain4 AS TEXT COLLATE "POSIX" CHECK (value <> 'hello'); +CREATE DOMAIN domain5 AS int[] NOT NULL; +CREATE DOMAIN domain6 AS varchar COLLATE "POSIX" CHECK (value <> 'hello'); +CREATE TABLE t22(a INT, b INT, c text COLLATE "C", col1 INT[]); +INSERT INTO t22 VALUES(-2, -1, 'hello', NULL); + +-- all of the following no need table rewrite, some may fail because of domain constraint violation +ALTER TABLE t22 ALTER COLUMN a SET DATA TYPE domain2 USING ((a))::domain2; +ALTER TABLE t22 ALTER COLUMN a SET DATA TYPE domain3 USING ((a))::domain3; +ALTER TABLE t22 ALTER COLUMN c SET DATA TYPE domain4; +ALTER TABLE t22 ALTER COLUMN c SET DATA TYPE domain6; +ALTER TABLE t22 ALTER COLUMN col1 SET DATA TYPE domain5 USING col1::int4[]::domain5; +UPDATE t22 set col1 = '{-2,null}'; +ALTER TABLE t22 ALTER COLUMN col1 SET DATA TYPE domain5 USING col1::int4[]::domain5; +ALTER TABLE t22 ALTER COLUMN col1 SET DATA TYPE int4[] USING col1::int4[]; +-- all of the above no need table rewrite + +ALTER TABLE t22 ALTER COLUMN a SET DATA TYPE domain2 USING ((b))::domain2; -- table rewrite +ALTER TABLE t22 ALTER COLUMN b SET DATA TYPE domain2 USING -2; -- table rewrite, fail +ALTER TABLE t22 ALTER COLUMN b SET DATA TYPE domain2 USING 15; -- table rewrite, ok +TABLE t22; +DROP TABLE t22; +DROP DOMAIN domain1, domain2, domain3, domain4, domain5, domain6; + -- Fall back to full rewrite for volatile expressions CREATE TABLE T(pk INT NOT NULL PRIMARY KEY); -- 2.34.1
