public inbox for [email protected]help / color / mirror / Atom feed
Fix domain fast defaults on empty tables 11+ messages / 5 participants [nested] [flat]
* Fix domain fast defaults on empty tables @ 2026-06-05 07:48 Chao Li <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Chao Li @ 2026-06-05 07:48 UTC (permalink / raw) To: Postgres hackers <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; jian he <[email protected]> Hi, I tested "[a0b6ef29a] Enable fast default for domains with non-volatile constraints". After tracing some cases from the regression tests, I came up with this test case and found a bug: ``` evantest=# create domain d_div as int check (1 / (value - 1) > 0); CREATE DOMAIN evantest=# create table t (a int); CREATE TABLE evantest=# alter table t add column b d_div default 1; ERROR: division by zero ``` It looks like errors inside the CHECK expression itself, such as the int4div division-by-zero in this test, are still hard errors that can fail the ALTER TABLE command. For the fix, I worked out two solutions: Solution 1 ======== We can add PG_TRY/PG_CATCH to capture those hard errors. But as a0b6ef29a's commit message mentions, the fallback should only apply while evaluating the CoerceToDomain expression. To avoid broadly suppressing hard errors, I only catch hard errors from domain_check_safe(), which verifies the default value against the domain. The default expression itself is still evaluated with hard errors. My concern with this solution is that there might be some error from domain_check_safe() that I am not aware of and that would be hidden by the try-catch. But that may be acceptable, because it matches the behavior before a0b6ef29a, so at least it is not a regression. Solution 2 ======== This solution is simpler and is based on the purpose of the original feature. a0b6ef29a's commit message says the purpose of the feature is to avoid table rewriting. Since an empty table has no data to rewrite, we can bypass the fast path for empty tables. The problem with this solution is that I currently use RelationGetNumberOfBlocks(rel) to decide whether the table is empty, so it cannot handle cases like: ``` Begin; Delete from t; Alter table t add column … ``` See the attached patches for details. v1-s1 is solution 1, and v1-s2 is solution 2. Please let me know which solution is preferred. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ Attachments: [application/octet-stream] v1-s1-0001-Handle-throwing-domain-checks-when-probing-fas.patch (9.8K, 2-v1-s1-0001-Handle-throwing-domain-checks-when-probing-fas.patch) download | inline diff: From 576164f2334a958dcaf39747ba8e068c852e4fc3 Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Thu, 4 Jun 2026 18:06:01 +0800 Subject: [PATCH v1-s1] Handle throwing domain checks when probing fast defaults. ALTER TABLE ADD COLUMN tries to prove that a default value can be stored as a missing value, using soft errors so that a failed domain constraint falls back to a table rewrite. However, a domain CHECK expression can throw an error before returning false, for example due to division by zero. That made the fast-default probe fail immediately, even for an empty table where the rewrite path would have succeeded. For direct domain coercions, evaluate the default expression separately and then validate the resulting datum with domain_check_safe(). Catch errors only around the domain validation step, so errors from the default expression itself are still reported normally. Author: Chao Li <[email protected]> --- src/backend/commands/tablecmds.c | 66 ++++++++++++++++++---- src/test/regress/expected/fast_default.out | 19 ++++++- src/test/regress/sql/fast_default.sql | 17 +++++- 3 files changed, 87 insertions(+), 15 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index a1845240a98..585df658258 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7640,23 +7640,67 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, ExprState *exprState; Datum missingval; bool missingIsNull; + volatile bool caught_error = false; ErrorSaveContext escontext = {T_ErrorSaveContext}; + CoerceToDomain *ctest = NULL; - /* Evaluate the default expression with soft errors */ + if (has_domain_constraints && IsA(defval, CoerceToDomain)) + ctest = castNode(CoerceToDomain, defval); + + /* + * Evaluate the default expression, using soft errors where + * possible + */ estate = CreateExecutorState(); - exprState = ExecPrepareExprWithContext(defval, estate, - (Node *) &escontext); - missingval = ExecEvalExpr(exprState, - GetPerTupleExprContext(estate), - &missingIsNull); + if (ctest) + { + MemoryContext oldcxt = CurrentMemoryContext; + + /* + * Evaluate the default expression itself with hard + * errors. + */ + exprState = ExecPrepareExpr(ctest->arg, estate); + missingval = ExecEvalExpr(exprState, + GetPerTupleExprContext(estate), + &missingIsNull); + + /* + * A domain CHECK expression can fail by throwing an + * error, rather than by returning false. Treat that like + * any other failed proof that the value is safe to cache. + */ + PG_TRY(); + { + (void) domain_check_safe(missingval, missingIsNull, + ctest->resulttype, + NULL, NULL, + (Node *) &escontext); + } + PG_CATCH(); + { + MemoryContextSwitchTo(oldcxt); + FlushErrorState(); + caught_error = true; + } + PG_END_TRY(); + } + else + { + exprState = ExecPrepareExprWithContext(defval, estate, + (Node *) &escontext); + missingval = ExecEvalExpr(exprState, + GetPerTupleExprContext(estate), + &missingIsNull); + } /* - * If the domain constraint check failed (via errsave), - * missingval is unreliable. Fall back to a table rewrite; - * Phase 3 will re-evaluate with hard errors, so the user gets - * an error only if the table has rows. + * If the domain constraint check failed, missingval is + * unreliable. Fall back to a table rewrite; Phase 3 will + * re-evaluate with hard errors, so the user gets an error + * only if the table has rows. */ - if (escontext.error_occurred) + if (caught_error || escontext.error_occurred) { missingIsNull = true; tab->rewrite |= AT_REWRITE_DEFAULT_VAL; diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out index 5813f1c61a5..bdaa5aedb42 100644 --- a/src/test/regress/expected/fast_default.out +++ b/src/test/regress/expected/fast_default.out @@ -322,12 +322,23 @@ CREATE DOMAIN domain5 AS int CHECK(value > 10) DEFAULT 8; CREATE DOMAIN domain6 as int CHECK(value > 10) DEFAULT random(min=>11, max=>100); CREATE DOMAIN domain7 as int CHECK((value + random(min=>11::int, max=>11)) > 12); CREATE DOMAIN domain8 as int NOT NULL; +CREATE DOMAIN domain9 AS int CHECK(1 / (value - 1) > 0); CREATE TABLE test_add_domain_col(a int); -- succeeds despite constraint-violating default because table is empty ALTER TABLE test_add_domain_col ADD COLUMN a1 domain5; NOTICE: rewriting table test_add_domain_col for reason 2 ALTER TABLE test_add_domain_col DROP COLUMN a1; INSERT INTO test_add_domain_col VALUES(1),(2); +-- likewise, an empty table succeeds if the domain check expression errors +CREATE TABLE test_add_domain_col_empty(a int); +ALTER TABLE test_add_domain_col_empty ADD COLUMN b domain9 DEFAULT 1; +NOTICE: rewriting table test_add_domain_col_empty for reason 2 +DROP TABLE test_add_domain_col_empty; +-- but errors in the default expression itself should not be hidden +CREATE TABLE test_add_domain_col_bad_default(a int); +ALTER TABLE test_add_domain_col_bad_default ADD COLUMN b domain5 DEFAULT 1 / 0; +ERROR: division by zero +DROP TABLE test_add_domain_col_bad_default; -- tests with non-empty table ALTER TABLE test_add_domain_col ADD COLUMN b domain5; -- table rewrite, then fail NOTICE: rewriting table test_add_domain_col for reason 2 @@ -338,6 +349,9 @@ ERROR: domain domain8 does not allow null values ALTER TABLE test_add_domain_col ADD COLUMN b domain5 DEFAULT 1; -- table rewrite, then fail NOTICE: rewriting table test_add_domain_col for reason 2 ERROR: value for domain domain5 violates check constraint "domain5_check" +ALTER TABLE test_add_domain_col ADD COLUMN b domain9 DEFAULT 1; -- table rewrite, then fail +NOTICE: rewriting table test_add_domain_col for reason 2 +ERROR: division by zero ALTER TABLE test_add_domain_col ADD COLUMN b domain5 DEFAULT 12; -- ok, no table rewrite -- explicit column default expression overrides domain's default -- expression, so no table rewrite @@ -365,8 +379,8 @@ ALTER TABLE test_add_domain_col ADD COLUMN f domain7; NOTICE: rewriting table test_add_domain_col for reason 2 -- domain with both volatile and non-volatile CHECK constraints: the -- volatile one forces a table rewrite -CREATE DOMAIN domain9 AS int CHECK(value > 10) CHECK((value + random(min=>1::int, max=>1)) > 0); -ALTER TABLE test_add_domain_col ADD COLUMN g domain9 DEFAULT 14; +CREATE DOMAIN domain10 AS int CHECK(value > 10) CHECK((value + random(min=>1::int, max=>1)) > 0); +ALTER TABLE test_add_domain_col ADD COLUMN g domain10 DEFAULT 14; NOTICE: rewriting table test_add_domain_col for reason 2 -- virtual generated columns cannot have domain types ALTER TABLE test_add_domain_col ADD COLUMN h domain5 @@ -383,6 +397,7 @@ DROP DOMAIN domain6; DROP DOMAIN domain7; DROP DOMAIN domain8; DROP DOMAIN domain9; +DROP DOMAIN domain10; 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 e5d9a3d2fd1..290321fe60a 100644 --- a/src/test/regress/sql/fast_default.sql +++ b/src/test/regress/sql/fast_default.sql @@ -292,6 +292,7 @@ CREATE DOMAIN domain5 AS int CHECK(value > 10) DEFAULT 8; CREATE DOMAIN domain6 as int CHECK(value > 10) DEFAULT random(min=>11, max=>100); CREATE DOMAIN domain7 as int CHECK((value + random(min=>11::int, max=>11)) > 12); CREATE DOMAIN domain8 as int NOT NULL; +CREATE DOMAIN domain9 AS int CHECK(1 / (value - 1) > 0); CREATE TABLE test_add_domain_col(a int); -- succeeds despite constraint-violating default because table is empty @@ -299,10 +300,21 @@ ALTER TABLE test_add_domain_col ADD COLUMN a1 domain5; ALTER TABLE test_add_domain_col DROP COLUMN a1; INSERT INTO test_add_domain_col VALUES(1),(2); +-- likewise, an empty table succeeds if the domain check expression errors +CREATE TABLE test_add_domain_col_empty(a int); +ALTER TABLE test_add_domain_col_empty ADD COLUMN b domain9 DEFAULT 1; +DROP TABLE test_add_domain_col_empty; + +-- but errors in the default expression itself should not be hidden +CREATE TABLE test_add_domain_col_bad_default(a int); +ALTER TABLE test_add_domain_col_bad_default ADD COLUMN b domain5 DEFAULT 1 / 0; +DROP TABLE test_add_domain_col_bad_default; + -- tests with non-empty table ALTER TABLE test_add_domain_col ADD COLUMN b domain5; -- table rewrite, then fail ALTER TABLE test_add_domain_col ADD COLUMN b domain8; -- table rewrite, then fail ALTER TABLE test_add_domain_col ADD COLUMN b domain5 DEFAULT 1; -- table rewrite, then fail +ALTER TABLE test_add_domain_col ADD COLUMN b domain9 DEFAULT 1; -- table rewrite, then fail ALTER TABLE test_add_domain_col ADD COLUMN b domain5 DEFAULT 12; -- ok, no table rewrite -- explicit column default expression overrides domain's default @@ -325,8 +337,8 @@ ALTER TABLE test_add_domain_col ADD COLUMN f domain7; -- domain with both volatile and non-volatile CHECK constraints: the -- volatile one forces a table rewrite -CREATE DOMAIN domain9 AS int CHECK(value > 10) CHECK((value + random(min=>1::int, max=>1)) > 0); -ALTER TABLE test_add_domain_col ADD COLUMN g domain9 DEFAULT 14; +CREATE DOMAIN domain10 AS int CHECK(value > 10) CHECK((value + random(min=>1::int, max=>1)) > 0); +ALTER TABLE test_add_domain_col ADD COLUMN g domain10 DEFAULT 14; -- virtual generated columns cannot have domain types ALTER TABLE test_add_domain_col ADD COLUMN h domain5 @@ -343,6 +355,7 @@ DROP DOMAIN domain6; DROP DOMAIN domain7; DROP DOMAIN domain8; DROP DOMAIN domain9; +DROP DOMAIN domain10; DROP FUNCTION foo(INT); -- Fall back to full rewrite for volatile expressions -- 2.50.1 (Apple Git-155) [application/octet-stream] v1-s2-0001-Preserve-empty-table-behavior-for-domain-fast-.patch (8.6K, 3-v1-s2-0001-Preserve-empty-table-behavior-for-domain-fast-.patch) download | inline diff: From 697a9568279634b0cf4e000be7850e8845e37ce0 Mon Sep 17 00:00:00 2001 From: "Chao Li (Evan)" <[email protected]> Date: Thu, 4 Jun 2026 18:06:01 +0800 Subject: [PATCH v1-s2] Preserve empty-table behavior for domain fast defaults. Commit a0b6ef29a allowed ALTER TABLE ADD COLUMN to use the missing-value fast path for domains with non-volatile constraints. However, while proving that the default can be stored as a missing value, a domain CHECK expression can throw an error, for example division by zero. That changed behavior for freshly empty tables, where the old rewrite path scanned no rows and therefore did not evaluate the invalid domain value during ALTER TABLE. Skip the fast-default probe for constrained domains when the relation has no heap blocks. In that case there is no data rewrite to avoid, and using the rewrite path preserves the previous behavior. Add regression coverage for an empty table whose domain CHECK expression throws during evaluation, while keeping the non-empty case as an error. Author: Chao Li <[email protected]> --- src/backend/commands/tablecmds.c | 13 +++++++++++++ src/test/regress/expected/fast_default.out | 18 ++++++++++++++++-- src/test/regress/sql/fast_default.sql | 17 +++++++++++++++-- 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index a1845240a98..d74635d0a23 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -7548,6 +7548,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, bool has_domain_constraints; bool has_missing = false; bool has_volatile = false; + bool skip_fast_default = false; /* * For an identity column, we can't use build_column_default(), @@ -7599,6 +7600,17 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, elog(ERROR, "failed to coerce base type to domain"); } + /* + * For constrained domains, a physically empty table does not need the + * fast default optimization. Use the rewrite path instead, + * preserving the old behavior of not evaluating the domain constraint + * when there are no heap tuples to rewrite. + */ + if (has_domain_constraints && + rel->rd_rel->relkind == RELKIND_RELATION && + RelationGetNumberOfBlocks(rel) == 0) + skip_fast_default = true; + if (defval) { NewColumnValue *newval; @@ -7632,6 +7644,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, * such a failure is only raised when the table has rows. */ if (rel->rd_rel->relkind == RELKIND_RELATION && + !skip_fast_default && !colDef->generated && !has_volatile && !contain_volatile_functions((Node *) defval)) diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out index 5813f1c61a5..1bcc7be43e3 100644 --- a/src/test/regress/expected/fast_default.out +++ b/src/test/regress/expected/fast_default.out @@ -322,12 +322,23 @@ CREATE DOMAIN domain5 AS int CHECK(value > 10) DEFAULT 8; CREATE DOMAIN domain6 as int CHECK(value > 10) DEFAULT random(min=>11, max=>100); CREATE DOMAIN domain7 as int CHECK((value + random(min=>11::int, max=>11)) > 12); CREATE DOMAIN domain8 as int NOT NULL; +CREATE DOMAIN domain9 AS int CHECK(1 / (value - 1) > 0); CREATE TABLE test_add_domain_col(a int); -- succeeds despite constraint-violating default because table is empty ALTER TABLE test_add_domain_col ADD COLUMN a1 domain5; NOTICE: rewriting table test_add_domain_col for reason 2 ALTER TABLE test_add_domain_col DROP COLUMN a1; INSERT INTO test_add_domain_col VALUES(1),(2); +-- likewise, an empty table succeeds if the domain check expression errors +CREATE TABLE test_add_domain_col_empty(a int); +ALTER TABLE test_add_domain_col_empty ADD COLUMN b domain9 DEFAULT 1; +NOTICE: rewriting table test_add_domain_col_empty for reason 2 +DROP TABLE test_add_domain_col_empty; +-- but errors in the default expression itself should not be hidden +CREATE TABLE test_add_domain_col_bad_default(a int); +ALTER TABLE test_add_domain_col_bad_default ADD COLUMN b domain5 DEFAULT 1 / 0; +ERROR: division by zero +DROP TABLE test_add_domain_col_bad_default; -- tests with non-empty table ALTER TABLE test_add_domain_col ADD COLUMN b domain5; -- table rewrite, then fail NOTICE: rewriting table test_add_domain_col for reason 2 @@ -338,6 +349,8 @@ ERROR: domain domain8 does not allow null values ALTER TABLE test_add_domain_col ADD COLUMN b domain5 DEFAULT 1; -- table rewrite, then fail NOTICE: rewriting table test_add_domain_col for reason 2 ERROR: value for domain domain5 violates check constraint "domain5_check" +ALTER TABLE test_add_domain_col ADD COLUMN b domain9 DEFAULT 1; -- fast proof fails +ERROR: division by zero ALTER TABLE test_add_domain_col ADD COLUMN b domain5 DEFAULT 12; -- ok, no table rewrite -- explicit column default expression overrides domain's default -- expression, so no table rewrite @@ -365,8 +378,8 @@ ALTER TABLE test_add_domain_col ADD COLUMN f domain7; NOTICE: rewriting table test_add_domain_col for reason 2 -- domain with both volatile and non-volatile CHECK constraints: the -- volatile one forces a table rewrite -CREATE DOMAIN domain9 AS int CHECK(value > 10) CHECK((value + random(min=>1::int, max=>1)) > 0); -ALTER TABLE test_add_domain_col ADD COLUMN g domain9 DEFAULT 14; +CREATE DOMAIN domain10 AS int CHECK(value > 10) CHECK((value + random(min=>1::int, max=>1)) > 0); +ALTER TABLE test_add_domain_col ADD COLUMN g domain10 DEFAULT 14; NOTICE: rewriting table test_add_domain_col for reason 2 -- virtual generated columns cannot have domain types ALTER TABLE test_add_domain_col ADD COLUMN h domain5 @@ -383,6 +396,7 @@ DROP DOMAIN domain6; DROP DOMAIN domain7; DROP DOMAIN domain8; DROP DOMAIN domain9; +DROP DOMAIN domain10; 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 e5d9a3d2fd1..8b58c6d44d0 100644 --- a/src/test/regress/sql/fast_default.sql +++ b/src/test/regress/sql/fast_default.sql @@ -292,6 +292,7 @@ CREATE DOMAIN domain5 AS int CHECK(value > 10) DEFAULT 8; CREATE DOMAIN domain6 as int CHECK(value > 10) DEFAULT random(min=>11, max=>100); CREATE DOMAIN domain7 as int CHECK((value + random(min=>11::int, max=>11)) > 12); CREATE DOMAIN domain8 as int NOT NULL; +CREATE DOMAIN domain9 AS int CHECK(1 / (value - 1) > 0); CREATE TABLE test_add_domain_col(a int); -- succeeds despite constraint-violating default because table is empty @@ -299,10 +300,21 @@ ALTER TABLE test_add_domain_col ADD COLUMN a1 domain5; ALTER TABLE test_add_domain_col DROP COLUMN a1; INSERT INTO test_add_domain_col VALUES(1),(2); +-- likewise, an empty table succeeds if the domain check expression errors +CREATE TABLE test_add_domain_col_empty(a int); +ALTER TABLE test_add_domain_col_empty ADD COLUMN b domain9 DEFAULT 1; +DROP TABLE test_add_domain_col_empty; + +-- but errors in the default expression itself should not be hidden +CREATE TABLE test_add_domain_col_bad_default(a int); +ALTER TABLE test_add_domain_col_bad_default ADD COLUMN b domain5 DEFAULT 1 / 0; +DROP TABLE test_add_domain_col_bad_default; + -- tests with non-empty table ALTER TABLE test_add_domain_col ADD COLUMN b domain5; -- table rewrite, then fail ALTER TABLE test_add_domain_col ADD COLUMN b domain8; -- table rewrite, then fail ALTER TABLE test_add_domain_col ADD COLUMN b domain5 DEFAULT 1; -- table rewrite, then fail +ALTER TABLE test_add_domain_col ADD COLUMN b domain9 DEFAULT 1; -- fast proof fails ALTER TABLE test_add_domain_col ADD COLUMN b domain5 DEFAULT 12; -- ok, no table rewrite -- explicit column default expression overrides domain's default @@ -325,8 +337,8 @@ ALTER TABLE test_add_domain_col ADD COLUMN f domain7; -- domain with both volatile and non-volatile CHECK constraints: the -- volatile one forces a table rewrite -CREATE DOMAIN domain9 AS int CHECK(value > 10) CHECK((value + random(min=>1::int, max=>1)) > 0); -ALTER TABLE test_add_domain_col ADD COLUMN g domain9 DEFAULT 14; +CREATE DOMAIN domain10 AS int CHECK(value > 10) CHECK((value + random(min=>1::int, max=>1)) > 0); +ALTER TABLE test_add_domain_col ADD COLUMN g domain10 DEFAULT 14; -- virtual generated columns cannot have domain types ALTER TABLE test_add_domain_col ADD COLUMN h domain5 @@ -343,6 +355,7 @@ DROP DOMAIN domain6; DROP DOMAIN domain7; DROP DOMAIN domain8; DROP DOMAIN domain9; +DROP DOMAIN domain10; DROP FUNCTION foo(INT); -- Fall back to full rewrite for volatile expressions -- 2.50.1 (Apple Git-155) ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix domain fast defaults on empty tables @ 2026-06-05 09:04 Heikki Linnakangas <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 2 replies; 11+ messages in thread From: Heikki Linnakangas @ 2026-06-05 09:04 UTC (permalink / raw) To: [email protected]; Chao Li <[email protected]>; Postgres hackers <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; jian he <[email protected]> On 5 June 2026 10:48:00 EEST, Chao Li <[email protected]> wrote: >Hi, > >I tested "[a0b6ef29a] Enable fast default for domains with non-volatile constraints". After tracing some cases from the regression tests, I came up with this test case and found a bug: >``` >evantest=# create domain d_div as int check (1 / (value - 1) > 0); >CREATE DOMAIN >evantest=# create table t (a int); >CREATE TABLE >evantest=# alter table t add column b d_div default 1; >ERROR: division by zero >``` > >It looks like errors inside the CHECK expression itself, such as the int4div division-by-zero in this test, are still hard errors that can fail the ALTER TABLE command. It seems totally reasonable to get an error in that case. '1' is not a valid value for the datatype, whether or not there are any rows in the table. - Heikki ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix domain fast defaults on empty tables @ 2026-06-05 09:52 Chao Li <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 0 replies; 11+ messages in thread From: Chao Li @ 2026-06-05 09:52 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: [email protected]; Andrew Dunstan <[email protected]>; jian he <[email protected]> > On Jun 5, 2026, at 17:04, Heikki Linnakangas <[email protected]> wrote: > > > > On 5 June 2026 10:48:00 EEST, Chao Li <[email protected]> wrote: >> Hi, >> >> I tested "[a0b6ef29a] Enable fast default for domains with non-volatile constraints". After tracing some cases from the regression tests, I came up with this test case and found a bug: >> ``` >> evantest=# create domain d_div as int check (1 / (value - 1) > 0); >> CREATE DOMAIN >> evantest=# create table t (a int); >> CREATE TABLE >> evantest=# alter table t add column b d_div default 1; >> ERROR: division by zero >> ``` >> >> It looks like errors inside the CHECK expression itself, such as the int4div division-by-zero in this test, are still hard errors that can fail the ALTER TABLE command. > > It seems totally reasonable to get an error in that case. '1' is not a valid value for the datatype, whether or not there are any rows in the table. > > - Heikki I agree that rejecting the default is semantically reasonable. But the concern is that this is a user-visible behavior change, and the feature didn’t seem intended to make that change. Before a0b6ef29a, this ALTER TABLE command could succeed. If we now want to reject such defaults, I think that should be documented explicitly. Also, there might be some practical use for the current behavior. For example, an invalid default can be used to prevent INSERT from omitting the column. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix domain fast defaults on empty tables @ 2026-06-05 14:08 Tom Lane <[email protected]> parent: Heikki Linnakangas <[email protected]> 1 sibling, 3 replies; 11+ messages in thread From: Tom Lane @ 2026-06-05 14:08 UTC (permalink / raw) To: Heikki Linnakangas <[email protected]>; +Cc: [email protected]; Chao Li <[email protected]>; Andrew Dunstan <[email protected]>; jian he <[email protected]> Heikki Linnakangas <[email protected]> writes: > On 5 June 2026 10:48:00 EEST, Chao Li <[email protected]> wrote: >> evantest=# create domain d_div as int check (1 / (value - 1) > 0); >> CREATE DOMAIN >> evantest=# create table t (a int); >> CREATE TABLE >> evantest=# alter table t add column b d_div default 1; >> ERROR: division by zero > It seems totally reasonable to get an error in that case. '1' is not a valid value for the datatype, whether or not there are any rows in the table. I think there's reason for concern here, which is that we do not throw an error for the apparently equivalent case regression=# create table t2 (a int, b d_div default 1); CREATE TABLE This will give you an error at INSERT, but not CREATE. So this is inconsistent, as well as different from the pre-v19 behavior. Concretely, I'm pretty sure it is a hazard for pg_dump, which thinks it can freely transform bits of CREATE operations into ALTERs. I didn't try to make an example case, but I suspect it is now possible to create a database that will fail dump/restore because of this inconsistency. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix domain fast defaults on empty tables @ 2026-06-06 12:41 Andrew Dunstan <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 1 reply; 11+ messages in thread From: Andrew Dunstan @ 2026-06-06 12:41 UTC (permalink / raw) To: Tom Lane <[email protected]>; Heikki Linnakangas <[email protected]>; +Cc: [email protected]; Chao Li <[email protected]>; jian he <[email protected]> On 2026-06-05 Fr 10:08 AM, Tom Lane wrote: > Heikki Linnakangas<[email protected]> writes: >> On 5 June 2026 10:48:00 EEST, Chao Li<[email protected]> wrote: >>> evantest=# create domain d_div as int check (1 / (value - 1) > 0); >>> CREATE DOMAIN >>> evantest=# create table t (a int); >>> CREATE TABLE >>> evantest=# alter table t add column b d_div default 1; >>> ERROR: division by zero >> It seems totally reasonable to get an error in that case. '1' is not a valid value for the datatype, whether or not there are any rows in the table. > I think there's reason for concern here, which is that we do not throw > an error for the apparently equivalent case > > regression=# create table t2 (a int, b d_div default 1); > CREATE TABLE > > This will give you an error at INSERT, but not CREATE. So this > is inconsistent, as well as different from the pre-v19 behavior. > > Concretely, I'm pretty sure it is a hazard for pg_dump, which thinks > it can freely transform bits of CREATE operations into ALTERs. > I didn't try to make an example case, but I suspect it is now possible > to create a database that will fail dump/restore because of this > inconsistency. > > Seems reasonable. So which of Chao's solutions do you prefer? I think both will meet the pg_dump issue, not sure how much we care about the case where we have deleted all the rows but not truncated the table. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix domain fast defaults on empty tables @ 2026-06-06 20:19 Tom Lane <[email protected]> parent: Andrew Dunstan <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Tom Lane @ 2026-06-06 20:19 UTC (permalink / raw) To: Andrew Dunstan <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; [email protected]; Chao Li <[email protected]>; jian he <[email protected]> Andrew Dunstan <[email protected]> writes: > On 2026-06-05 Fr 10:08 AM, Tom Lane wrote: >> Concretely, I'm pretty sure it is a hazard for pg_dump, which thinks >> it can freely transform bits of CREATE operations into ALTERs. >> I didn't try to make an example case, but I suspect it is now possible >> to create a database that will fail dump/restore because of this >> inconsistency. > Seems reasonable. So which of Chao's solutions do you prefer? I think > both will meet the pg_dump issue, not sure how much we care about the > case where we have deleted all the rows but not truncated the table. [ studies it a bit ...] TBH, I don't like either of these, nor do I like a0b6ef29a to begin with. The fundamental problem with this whole mess is that it treats certain kinds of default-expression evaluation error (i.e., domain CHECK failures) differently from others. There is no way that that leads to a consistent user experience. The current complaint is one manifestation of that, but I'm sure there are others, eg having to do with domain coercions lower down in a default expression. It could work if ExecPrepareExprWithContext were capable of soft-trapping essentially all execution errors, but that's not true today and I strongly doubt it ever will be true. I think Chao's v1-s2-0001 points the way towards what could be a workable solution: if we see that the table is known empty (after we already have exclusive lock on it!), we could skip both the table rewrite and the insertion of an attmissingval, and thereby not need to evaluate the default at all during ALTER TABLE. As Chao says, simplistic versions of "known empty" would expose some user-visible behavioral inconsistencies, but I'm not sure how much that matters. For pg_dump and similar applications we would get the behavior we needed even with just an is-physically-empty check, since all their CREATE/ALTER DDL happens before we ever insert any data. You could imagine going further and scanning the table for live tuples, but I don't know that that's going to be worth the cycles. However, I doubt that a bare "RelationGetNumberOfBlocks() == 0" check is acceptable: we probably need to let the table access method have control of this. heapam could do "RelationGetNumberOfBlocks() == 0", but other TAMs might need to do something else. So that means that this path requires a new TableAmRoutine method, and that probably puts it in the too-late-for-v19 category. My recommendation: we ought to revert a0b6ef29a for now and redesign the optimization for v20. If you don't like that answer, I'd be firmly against v1-s1-0001 in any case. It repeats the classic mistake of supposing that we can PG_CATCH random errors and not invoke transaction cleanup. BTW, I do not like the fact that a0b6ef29a removed the comment stanza explaining why we need this fake default expression at all. That's still largely applicable AFAICS. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix domain fast defaults on empty tables @ 2026-06-07 02:09 Chao Li <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 11+ messages in thread From: Chao Li @ 2026-06-07 02:09 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andrew Dunstan <[email protected]>; Heikki Linnakangas <[email protected]>; [email protected]; jian he <[email protected]> > On Jun 7, 2026, at 04:19, Tom Lane <[email protected]> wrote: > > Andrew Dunstan <[email protected]> writes: >> On 2026-06-05 Fr 10:08 AM, Tom Lane wrote: >>> Concretely, I'm pretty sure it is a hazard for pg_dump, which thinks >>> it can freely transform bits of CREATE operations into ALTERs. >>> I didn't try to make an example case, but I suspect it is now possible >>> to create a database that will fail dump/restore because of this >>> inconsistency. > >> Seems reasonable. So which of Chao's solutions do you prefer? I think >> both will meet the pg_dump issue, not sure how much we care about the >> case where we have deleted all the rows but not truncated the table. > > [ studies it a bit ...] TBH, I don't like either of these, > nor do I like a0b6ef29a to begin with. The fundamental problem > with this whole mess is that it treats certain kinds of > default-expression evaluation error (i.e., domain CHECK failures) > differently from others. There is no way that that leads to a > consistent user experience. The current complaint is one > manifestation of that, but I'm sure there are others, eg having > to do with domain coercions lower down in a default expression. > > It could work if ExecPrepareExprWithContext were capable of > soft-trapping essentially all execution errors, but that's > not true today and I strongly doubt it ever will be true. > > I think Chao's v1-s2-0001 points the way towards what could be a > workable solution: if we see that the table is known empty (after > we already have exclusive lock on it!), we could skip both the table > rewrite and the insertion of an attmissingval, and thereby not need > to evaluate the default at all during ALTER TABLE. As Chao says, > simplistic versions of "known empty" would expose some user-visible > behavioral inconsistencies, but I'm not sure how much that matters. > For pg_dump and similar applications we would get the behavior we > needed even with just an is-physically-empty check, since all their > CREATE/ALTER DDL happens before we ever insert any data. You could > imagine going further and scanning the table for live tuples, but > I don't know that that's going to be worth the cycles. > > However, I doubt that a bare "RelationGetNumberOfBlocks() == 0" check > is acceptable: we probably need to let the table access method have > control of this. heapam could do "RelationGetNumberOfBlocks() == 0", > but other TAMs might need to do something else. So that means that > this path requires a new TableAmRoutine method, and that probably > puts it in the too-late-for-v19 category. > > My recommendation: we ought to revert a0b6ef29a for now and > redesign the optimization for v20. > > If you don't like that answer, I'd be firmly against v1-s1-0001 > in any case. It repeats the classic mistake of supposing that > we can PG_CATCH random errors and not invoke transaction cleanup. > > BTW, I do not like the fact that a0b6ef29a removed the comment > stanza explaining why we need this fake default expression at all. > That's still largely applicable AFAICS. > > regards, tom lane Thanks, Tom, for the detailed explanation. I'll hold off here and wait for Andrew's decision on how to proceed. Sounds like reverting a0b6ef29a may be the preferred option for now. For v1-s2, I’m not ware of a better way to decide whether a table is known empty. I ever considered a count(*)-style scan or a LIMIT 1 scan, but those seem worse, they would add a new table scan inside ALTER TABLE just to decide whether to evaluate the default. Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/ ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix domain fast defaults on empty tables @ 2026-06-08 00:33 jian he <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 1 reply; 11+ messages in thread From: jian he @ 2026-06-08 00:33 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; [email protected]; Chao Li <[email protected]>; Andrew Dunstan <[email protected]> On Fri, Jun 5, 2026 at 10:08 PM Tom Lane <[email protected]> wrote: > > Heikki Linnakangas <[email protected]> writes: > > On 5 June 2026 10:48:00 EEST, Chao Li <[email protected]> wrote: > >> evantest=# create domain d_div as int check (1 / (value - 1) > 0); > >> CREATE DOMAIN > >> evantest=# create table t (a int); > >> CREATE TABLE > >> evantest=# alter table t add column b d_div default 1; > >> ERROR: division by zero > > > It seems totally reasonable to get an error in that case. '1' is not a valid value for the datatype, whether or not there are any rows in the table. > > I think there's reason for concern here, which is that we do not throw > an error for the apparently equivalent case > > regression=# create table t2 (a int, b d_div default 1); > CREATE TABLE > > This will give you an error at INSERT, but not CREATE. So this > is inconsistent, as well as different from the pre-v19 behavior. > However, this is normal behavior for non-domain types. create table t2 (a numeric default (1::numeric/0.0::float4)); -- ok alter table t2 add column b numeric default ((1::numeric/0.0::float4)); -- error ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix domain fast defaults on empty tables @ 2026-06-08 08:20 jian he <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 0 replies; 11+ messages in thread From: jian he @ 2026-06-08 08:20 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; [email protected]; Chao Li <[email protected]>; Andrew Dunstan <[email protected]> On Fri, Jun 5, 2026 at 10:08 PM Tom Lane <[email protected]> wrote: > > Heikki Linnakangas <[email protected]> writes: > > On 5 June 2026 10:48:00 EEST, Chao Li <[email protected]> wrote: > >> evantest=# create domain d_div as int check (1 / (value - 1) > 0); > >> CREATE DOMAIN > >> evantest=# create table t (a int); > >> CREATE TABLE > >> evantest=# alter table t add column b d_div default 1; > >> ERROR: division by zero > > > It seems totally reasonable to get an error in that case. '1' is not a valid value for the datatype, whether or not there are any rows in the table. > > I think there's reason for concern here, which is that we do not throw > an error for the apparently equivalent case > > regression=# create table t2 (a int, b d_div default 1); > CREATE TABLE > > This will give you an error at INSERT, but not CREATE. So this > is inconsistent, as well as different from the pre-v19 behavior. > > Concretely, I'm pretty sure it is a hazard for pg_dump, which thinks > it can freely transform bits of CREATE operations into ALTERs. > I didn't try to make an example case, but I suspect it is now possible > to create a database that will fail dump/restore because of this > inconsistency. Per the docs [1], we have two relevant forms: ALTER [ COLUMN ] column_name SET DEFAULT expression ADD [ COLUMN ] [ IF NOT EXISTS ] column_name data_type ... [ DEFAULT default_expr ] ATExecColumnDefault -> AddRelationNewConstraints does NOT evaluate the default expression. So ALTER COLUMN SET DEFAULT will never error out on a bad expression because it is never evaluated at DDL time. ALTER TABLE ADD COLUMN with a DEFAULT: pg_dump.c will never emit such a command. You can verify this by searching for the keyword "add" in src/bin/pg_dump/pg_dump.c, and looking at each occurrence one by one. Looking at dumpTableSchema() confirms the same: we do not produce command: ALTER TABLE ADD COLUMN. See dumpTableSchema below comments related FOR LOOP part also /* * Dump additional per-column properties that we can't handle in the * main CREATE TABLE command. */ [1] https://www.postgresql.org/docs/current/sql-altertable.html -------------------------------------------- create or replace function dummy() returns numeric AS $$ BEGIN RETURN 1/0; END$$ immutable LANGUAGE plpgsql; create table t1(a numeric default dummy()); alter table t1 add column b numeric default dummy(); ERROR: division by zero CONTEXT: PL/pgSQL expression "1/0" PL/pgSQL function dummy() line 3 at RETURN As you can see, if pg_dump somehow converted the CREATE default expression into an ALTER TABLE ADD COLUMN DEFAULT, we would already be facing this issue. Am I missing something? -- jian https://www.enterprisedb.com/ ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix domain fast defaults on empty tables @ 2026-06-08 15:40 Andrew Dunstan <[email protected]> parent: Chao Li <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Andrew Dunstan @ 2026-06-08 15:40 UTC (permalink / raw) To: Chao Li <[email protected]>; Tom Lane <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; [email protected]; jian he <[email protected]> On 2026-06-06 Sa 10:09 PM, Chao Li wrote: > >> On Jun 7, 2026, at 04:19, Tom Lane <[email protected]> wrote: >> >> Andrew Dunstan <[email protected]> writes: >>> On 2026-06-05 Fr 10:08 AM, Tom Lane wrote: >>>> Concretely, I'm pretty sure it is a hazard for pg_dump, which thinks >>>> it can freely transform bits of CREATE operations into ALTERs. >>>> I didn't try to make an example case, but I suspect it is now possible >>>> to create a database that will fail dump/restore because of this >>>> inconsistency. >>> Seems reasonable. So which of Chao's solutions do you prefer? I think >>> both will meet the pg_dump issue, not sure how much we care about the >>> case where we have deleted all the rows but not truncated the table. >> [ studies it a bit ...] TBH, I don't like either of these, >> nor do I like a0b6ef29a to begin with. The fundamental problem >> with this whole mess is that it treats certain kinds of >> default-expression evaluation error (i.e., domain CHECK failures) >> differently from others. There is no way that that leads to a >> consistent user experience. The current complaint is one >> manifestation of that, but I'm sure there are others, eg having >> to do with domain coercions lower down in a default expression. >> >> It could work if ExecPrepareExprWithContext were capable of >> soft-trapping essentially all execution errors, but that's >> not true today and I strongly doubt it ever will be true. >> >> I think Chao's v1-s2-0001 points the way towards what could be a >> workable solution: if we see that the table is known empty (after >> we already have exclusive lock on it!), we could skip both the table >> rewrite and the insertion of an attmissingval, and thereby not need >> to evaluate the default at all during ALTER TABLE. As Chao says, >> simplistic versions of "known empty" would expose some user-visible >> behavioral inconsistencies, but I'm not sure how much that matters. >> For pg_dump and similar applications we would get the behavior we >> needed even with just an is-physically-empty check, since all their >> CREATE/ALTER DDL happens before we ever insert any data. You could >> imagine going further and scanning the table for live tuples, but >> I don't know that that's going to be worth the cycles. >> >> However, I doubt that a bare "RelationGetNumberOfBlocks() == 0" check >> is acceptable: we probably need to let the table access method have >> control of this. heapam could do "RelationGetNumberOfBlocks() == 0", >> but other TAMs might need to do something else. So that means that >> this path requires a new TableAmRoutine method, and that probably >> puts it in the too-late-for-v19 category. >> >> My recommendation: we ought to revert a0b6ef29a for now and >> redesign the optimization for v20. >> >> If you don't like that answer, I'd be firmly against v1-s1-0001 >> in any case. It repeats the classic mistake of supposing that >> we can PG_CATCH random errors and not invoke transaction cleanup. >> >> BTW, I do not like the fact that a0b6ef29a removed the comment >> stanza explaining why we need this fake default expression at all. >> That's still largely applicable AFAICS. >> >> regards, tom lane > Thanks, Tom, for the detailed explanation. > > I'll hold off here and wait for Andrew's decision on how to proceed. Sounds like reverting a0b6ef29a may be the preferred option for now. > > For v1-s2, I’m not ware of a better way to decide whether a table is known empty. I ever considered a count(*)-style scan or a LIMIT 1 scan, but those seem worse, they would add a new table scan inside ALTER TABLE just to decide whether to evaluate the default. > Given Tom's reasonable suggestion that we really need a new Table AM callback to do this sensibly, I agree we should revert it. Will do. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Fix domain fast defaults on empty tables @ 2026-06-08 18:12 Tom Lane <[email protected]> parent: jian he <[email protected]> 0 siblings, 0 replies; 11+ messages in thread From: Tom Lane @ 2026-06-08 18:12 UTC (permalink / raw) To: jian he <[email protected]>; +Cc: Heikki Linnakangas <[email protected]>; [email protected]; Chao Li <[email protected]>; Andrew Dunstan <[email protected]> jian he <[email protected]> writes: > On Fri, Jun 5, 2026 at 10:08 PM Tom Lane <[email protected]> wrote: >> I think there's reason for concern here, which is that we do not throw >> an error for the apparently equivalent case >> regression=# create table t2 (a int, b d_div default 1); >> CREATE TABLE >> This will give you an error at INSERT, but not CREATE. So this >> is inconsistent, as well as different from the pre-v19 behavior. > However, this is normal behavior for non-domain types. > create table t2 (a numeric default (1::numeric/0.0::float4)); -- ok > alter table t2 add column b numeric default ((1::numeric/0.0::float4)); -- error Well, that's not great either. The idea of avoiding evaluating the default expression altogether when the table is empty could ameliorate that problem too. But my point stands: a0b6ef29a introduces different treatment for domain-CHECK-constraint errors than other types of runtime errors in the default expression. I think that is fundamentally the wrong direction to go in. We want more consistency here, not less. regards, tom lane ^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2026-06-08 18:12 UTC | newest] Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-06-05 07:48 Fix domain fast defaults on empty tables Chao Li <[email protected]> 2026-06-05 09:04 ` Heikki Linnakangas <[email protected]> 2026-06-05 09:52 ` Chao Li <[email protected]> 2026-06-05 14:08 ` Tom Lane <[email protected]> 2026-06-06 12:41 ` Andrew Dunstan <[email protected]> 2026-06-06 20:19 ` Tom Lane <[email protected]> 2026-06-07 02:09 ` Chao Li <[email protected]> 2026-06-08 15:40 ` Andrew Dunstan <[email protected]> 2026-06-08 00:33 ` jian he <[email protected]> 2026-06-08 18:12 ` Tom Lane <[email protected]> 2026-06-08 08:20 ` jian he <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox