support fast default for domain with constraints

jian he <jian.universality@gmail.com> writes:

Thanks to commit aaaf9449ec6be62cb0d30ed3588dc384f56274bf[1],
ExprState.escontext (ErrorSaveContext) was added, and ExecEvalConstraintNotNull,
ExecEvalConstraintCheck were changed to use errsave instead of hard error.
Now we can use it to evaluate CoerceToDomain in a soft error way, that
is what this patch intended to do.

This patch appears to summarily throw away a couple of
backwards-compatibility concerns that the previous round
took care to preserve:

* not throwing an error if the default would fail the domain
constraints, but the table is empty so there is no need to
instantiate the default.

* not assuming that the domain constraints are immutable.

Now it's fair to question how important the second point is
considering that we mostly treat domain constraints as immutable
elsewhere.  But I think the first point has actual practical uses
--- for example, if you want to set things up so that inserts must
specify that column explicitly.  So I don't think it's okay to
discard that behavior.

Maybe we need a regression test case demonstrating that that
behavior exists, to discourage people from breaking it ...

regards, tom lane

On Wed, Mar 5, 2025 at 11:13 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

This patch appears to summarily throw away a couple of
backwards-compatibility concerns that the previous round
took care to preserve:

* not throwing an error if the default would fail the domain
constraints, but the table is empty so there is no need to
instantiate the default.

hi. Thanks for pointing this out.
I noticed an empty table scarenio, but didn't check it thoroughly.
The attached patch preserves this backwards-compatibility.
now it's aligned with master behavior, i think.

main gotcha is:
ALTER TABLE ADD COLUMN...
If no explicitly DEFAULT, the defval either comes from pg_type.typdefaultbin,
or constructed via makeNullConst branch.
In that case, we need to use soft error evaluation, because we allow
these cases for an empty table;
In other cases, we can directly evaluate explicitly the DEFAULT clause.

* not assuming that the domain constraints are immutable.

Now it's fair to question how important the second point is
considering that we mostly treat domain constraints as immutable
elsewhere.  But I think the first point has actual practical uses
--- for example, if you want to set things up so that inserts must
specify that column explicitly.  So I don't think it's okay to
discard that behavior.

in v2-0003. I created a new function:
bool DomainHaveVolatileConstraints(Oid type_id, bool *have_volatile)
within DomainHaveVolatileConstraints
i use contain_volatile_functions to test whether check_expr is volatile or not.
contain_volatile_functions won't be expensive, i think.

if true then have_volatile is set to true.
if have_volatile is true then we need table rewrite.

hi.

rearrange the patch.
v3-0001 and v3-0002 is preparare patches.
v3-0001 add function: ExecPrepareExprSafe and ExecInitExprSafe.
v3-0002 add function: DomainHaveVolatileConstraints

v3-0003 tests and apply fast default for domain with constraints.
v3-0003 table with empty rows aligned with master behavior.
also no table rewrite if the domain has volatile check constraints,
so less surprising behavior.

On Thu, Mar 6, 2025 at 11:04 AM jian he <jian.universality@gmail.com> wrote:

hi.

rearrange the patch.
v3-0001 and v3-0002 is preparare patches.
v3-0001 add function: ExecPrepareExprSafe and ExecInitExprSafe.
v3-0002 add function: DomainHaveVolatileConstraints

i actually do need DomainHaveVolatileConstraints
for virtual generated columns over domain with constraints in [1]/messages/by-id/CACJufxHArQysbDkWFmvK+D1TPHQWWTxWN15cMuUaTYX3xhQXgg@mail.gmail.com,
which I am working on.

for example:
create domain d1 as int check(value > random(min=>11::int, max=>12));
create domain d2 as int check(value > 12);
create table t(a int);
insert into t select g from generate_series(1, 10) g;

----we do need table rewrite in phase 3.
alter table t add column b d1 generated always as (a+11) virtual;

--we can only do table scan in phase 3.
alter table t add column c d2 generated always as (a + 12) virtual;

Generally, table rewrite is more expensive than table scan.
In the above case, if domain constraints are not volatile, table scan
should be fine.

[1]: /messages/by-id/CACJufxHArQysbDkWFmvK+D1TPHQWWTxWN15cMuUaTYX3xhQXgg@mail.gmail.com

hi.

rebase because of commit: 8dd7c7cd0a2605d5301266a6b67a569d6a305106
also did minor enhancement.

v4-0001 add function: ExecPrepareExprSafe and ExecInitExprSafe.
v4-0002 add function: DomainHaveVolatileConstraints
v4-0003 tests and apply fast default for domain with constraints.

v4-0003 table with empty rows aligned with master behavior.
also will do table rewrite if the new column is domain with volatile
check constraints,
so less surprising behavior.

On Mon, Mar 24, 2025 at 7:14 PM jian he <jian.universality@gmail.com> wrote:

v4-0003 table with empty rows aligned with master behavior.
also will do table rewrite if the new column is domain with volatile
check constraints,
so less surprising behavior.

I found out that my v4-0003 is wrong.

For example, the following ALTER TABLE ADD COLUMN should not fail.
CREATE DOMAIN domain5 AS int check(value > 10) default 8;
CREATE TABLE t3(a int);
ALTER TABLE t3 ADD COLUMN b domain5 default 1; --ok, table rewrite

I also reduced the bloated tests.
summary of the behavior that is different from master:
if domain constraint is not volatile *and* domain's default expression satisfy
constraint's condition then no need table rewrite.

hi.
attached is to fix the regress test failure[0]https://api.cirrus-ci.com/v1/artifact/task/6014753866252288/log/src/test/regress/regression.diffs in V5.
I also did some test simplification.

[0]: https://api.cirrus-ci.com/v1/artifact/task/6014753866252288/log/src/test/regress/regression.diffs

hi.

in previous patches v6-0001 to v6-0003, we added support for ALTER TABLE ADD
COLUMN with fast defaults for domains having non-volatile constraints.

inspired by another patch of mine: https://commitfest.postgresql.org/patch/5907
I believe it's doable to perform only a table scan when using ALTER TABLE ADD
COLUMN with a domain that has volatile constraints.

some example:
CREATE DOMAIN domain8 as int check((value + random(min=>11::int,
max=>11)) > 12);
CREATE TABLE t3(a int);
INSERT INTO t3 VALUES(1),(2);
ALTER TABLE t3 ADD COLUMN f domain8 default 1; --error while coercing to domain
ALTER TABLE t3 ADD COLUMN f domain8 default 20; --ok

The idea is the same as mentioned in [1]/messages/by-id/CACJufxFhWyWzf2sJS9txSKeyA8hstxGDb8q2QWWwbo5Q1smPMA@mail.gmail.com,
for struct NewColumnValue, add another field (scan_only) to indicate
that we use table scan to evaluate the CoerceToDomain node.

summary of the attached v7.
v7-0001, v7-00002: preparatory patch.
v7-0003 adds fast default support for ALTER TABLE ADD COLUMN when the domain has
non-volatile constraints.
A table rewrite is still required for domains with volatile constraints.

v7-0004 skip table rewrite (table scan only) for ALTER TABLE ADD
COLUMN with domains has volatile constraints.

[1]: /messages/by-id/CACJufxFhWyWzf2sJS9txSKeyA8hstxGDb8q2QWWwbo5Q1smPMA@mail.gmail.com

On Mon, Sep 1, 2025 at 2:27 PM jian he <jian.universality@gmail.com> wrote:

summary of the attached v7.
v7-0001, v7-00002: preparatory patch.
v7-0003 adds fast default support for ALTER TABLE ADD COLUMN when the domain has
non-volatile constraints.
A table rewrite is still required for domains with volatile constraints.

v7-0004 skip table rewrite (table scan only) for ALTER TABLE ADD
COLUMN with domains has volatile constraints.

Hi.

rebase, and further simplified.

maybe we could perform a table scan for ALTER TABLE ADD COLUMN when the domain
has volatile constraints like v7-0004, avoiding a table rewrite.
However, this approach
feels inelegant, so I do not plan to pursue it.

So, the fast default now applies to domains with non-volatile constraint
expressions only.

Regarding the prior discussion about empty table behavior. This patch is
consistent with the master: not throwing an error if the default would fail the
domain constraints.

--
jian
https://www.enterprisedb.com/

On 2026-01-26 Mo 2:52 AM, jian he wrote:

On Mon, Sep 1, 2025 at 2:27 PM jian he <jian.universality@gmail.com> wrote:

summary of the attached v7.
v7-0001, v7-00002: preparatory patch.
v7-0003 adds fast default support for ALTER TABLE ADD COLUMN when the domain has
non-volatile constraints.
A table rewrite is still required for domains with volatile constraints.

v7-0004 skip table rewrite (table scan only) for ALTER TABLE ADD
COLUMN with domains has volatile constraints.

Hi.

rebase, and further simplified.

maybe we could perform a table scan for ALTER TABLE ADD COLUMN when the domain
has volatile constraints like v7-0004, avoiding a table rewrite.
However, this approach
feels inelegant, so I do not plan to pursue it.

So, the fast default now applies to domains with non-volatile constraint
expressions only.

Regarding the prior discussion about empty table behavior. This patch is
consistent with the master: not throwing an error if the default would fail the
domain constraints.

here's an updated patch set.

main changes:

. renamed DomainHaveVolatileConstraints renamed to
DomainHasVolatileConstraints, improved the comments and code structure

. squashed two commits into one, as there's only one user for the
soft-error functions

. rename  ExecPrepareExprExtended to ExecPrepareExprWithContext and
ExecInitExprExtended to ExecInitExprWithContext, with improved comments.

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

On Wed, Mar 11, 2026 at 1:18 AM Andrew Dunstan <andrew@dunslane.net> wrote:

here's an updated patch set.

+/*
+ * ExecPrepareExprWithContext: same as ExecPrepareExpr, but with an optional
+ * ErrorSaveContext for soft error handling during domain constraint
evaluation.
+ *
+ * See ExecInitExprWithContext for details on the escontext parameter.
+ */
+ExprState *
+ExecPrepareExprWithContext(Expr *node, EState *estate, Node *escontext)

Since ExecPrepareExprWithContext can be used more broadly, we should delete the
mention of domain constraint from the above comments.

I checked alter_table.sgml again, no need to change it, I think.

Slightly changed the regression test comments.

--
jian
https://www.enterprisedb.com/

I’ve been burned my this issue in the past so would be great if this could get in.

+/*
+ * DomainHasVolatileConstraints --- check if a domain has constraints with
+ * volatile expressions
+ *
+ * Returns true if the domain has any constraints at all. If have_volatile
+ * is not NULL, also checks whether any CHECK constraint contains a volatile
+ * expression and sets *have_volatile accordingly.
+ *
+ * The caller must initialize *have_volatile before calling (typically to
+ * false). This function only ever sets it to true, never to false.
+ *
+ * This is defined to return false, not fail, if type is not a domain.
+ */
+bool
+DomainHasVolatileConstraints(Oid type_id, bool *have_volatile)

Call it CheckDomainConstraints or something instead? IMO it's confusing
the have it not return what it's called.

Also, it'd make it more self-contained and thus safer to initialise have_volatile to false.

+	if (typentry->domainData != NULL)
+	{
+ if (have_volatile)
+ {
+ foreach_node(DomainConstraintState, constrstate,
+ typentry->domainData->constraints)
+ {
+ if (constrstate->constrainttype == DOM_CONSTRAINT_CHECK &&
+ contain_volatile_functions((Node *) constrstate->check_expr))
+ {
+ *have_volatile = true;
+ break;
+ }
+ }
+ }
+
+ return true;
+	}
+
+	return false;
+}
Could simplify the code by doing an early return if domainData == NULL?
(same with have_volatile below)

+ /*
+ * If the domain has volatile constraints, we must do a table rewrite
+ * since the constraint result could differ per row and cannot be
+ * evaluated once and cached as a missing value.
+ */
+ if (has_volatile)
+ {
+ Assert(has_domain_constraints);
+ tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
+ }

I'm not sure. But seems to me this makes the pre-existing guard for virtual columns
redundant?
I mean this code on line 7633:
if (colDef->generated != ATTRIBUTE_GENERATED_VIRTUAL)
 tab->rewrite |= AT_REWRITE_DEFAULT_VAL;

+-- test fast default over domains with constraints
+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;

Would be nice to test domains with both volatile and non-volatile checks.
Also, perhaps virtual generated columns could use a test?

/Viktor Holmberg

On 2026-03-11 We 6:34 AM, Viktor Holmberg wrote:

I’ve been burned my this issue in the past so would be great if this
could get in.

+/*
+ * DomainHasVolatileConstraints --- check if a domain has constraints 
with
+ * volatile expressions
+ *
+ * Returns true if the domain has any constraints at all. If 
have_volatile
+ * is not NULL, also checks whether any CHECK constraint contains a 
volatile
+ * expression and sets *have_volatile accordingly.
+ *
+ * The caller must initialize *have_volatile before calling (typically to
+ * false). This function only ever sets it to true, never to false.
+ *
+ * This is defined to return false, not fail, if type is not a domain.
+ */
+bool
+DomainHasVolatileConstraints(Oid type_id, bool *have_volatile)

Call it CheckDomainConstraints or something instead? IMO it's confusing
the have it not return what it's called.

Also, it'd make it more self-contained and thus safer to initialise
have_volatile to false.

+ if (typentry->domainData != NULL)
+ {
+ if (have_volatile)
+ {
+ foreach_node(DomainConstraintState, constrstate,
+ typentry->domainData->constraints)
+ {
+ if (constrstate->constrainttype == DOM_CONSTRAINT_CHECK &&
+ contain_volatile_functions((Node *) constrstate->check_expr))
+ {
+ *have_volatile = true;
+ break;
+ }
+ }
+ }
+
+ return true;
+ }
+
+ return false;
+}
Could simplify the code by doing an early return if domainData == NULL?
(same with have_volatile below)

I think it's cleaner just to modify the existing function with an extra
parameter, which the existing callers will pass as NULL.

Would be nice to test domains with both volatile and non-volatile checks.
Also, perhaps virtual generated columns could use a test?

Also added some tests.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

On Thu, Mar 12, 2026 at 3:50 AM Andrew Dunstan <andrew@dunslane.net> wrote:

Also added some tests.

V11 looks good to me.

On Wed, Mar 11, 2026 at 6:34 PM Viktor Holmberg <v@viktorh.net> wrote:

I’ve been burned my this issue in the past so would be great if this could get in.

+ /*
+ * If the domain has volatile constraints, we must do a table rewrite
+ * since the constraint result could differ per row and cannot be
+ * evaluated once and cached as a missing value.
+ */
+ if (has_volatile)
+ {
+ Assert(has_domain_constraints);
+ tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
+ }

I'm not sure. But seems to me this makes the pre-existing guard for virtual columns
redundant?
I mean this code on line 7633:
if (colDef->generated != ATTRIBUTE_GENERATED_VIRTUAL)
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;

If the first `if (has_volatile)` is false, then

if (colDef->generated != ATTRIBUTE_GENERATED_VIRTUAL)
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;

is still reachable.

Also, perhaps virtual generated columns could use a test?

Virtual generated columns based on domain are not currently supported.
I have a patch for it: https://commitfest.postgresql.org/patch/5725
but it's not doable now because of
https://git.postgresql.org/cgit/postgresql.git/commit/?id=0cd69b3d7ef357f2b43258de5831c4de0bd51dec
You may also be interested in https://commitfest.postgresql.org/patch/5907

--
jian
https://www.enterprisedb.com/

On 12 Mar 2026 at 04:36 +0100, jian he <jian.universality@gmail.com>, wrote:

On Thu, Mar 12, 2026 at 3:50 AM Andrew Dunstan <andrew@dunslane.net> wrote:

Also added some tests.

V11 looks good to me.

On Wed, Mar 11, 2026 at 6:34 PM Viktor Holmberg <v@viktorh.net> wrote:

I’ve been burned my this issue in the past so would be great if this could get in.

+ /*
+ * If the domain has volatile constraints, we must do a table rewrite
+ * since the constraint result could differ per row and cannot be
+ * evaluated once and cached as a missing value.
+ */
+ if (has_volatile)
+ {
+ Assert(has_domain_constraints);
+ tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
+ }

I'm not sure. But seems to me this makes the pre-existing guard for virtual columns
redundant?
I mean this code on line 7633:
if (colDef->generated != ATTRIBUTE_GENERATED_VIRTUAL)
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;

If the first `if (has_volatile)` is false, then

if (colDef->generated != ATTRIBUTE_GENERATED_VIRTUAL)
tab->rewrite |= AT_REWRITE_DEFAULT_VAL;

is still reachable.

Also, perhaps virtual generated columns could use a test?

Virtual generated columns based on domain are not currently supported.
I have a patch for it: https://commitfest.postgresql.org/patch/5725
but it's not doable now because of
https://git.postgresql.org/cgit/postgresql.git/commit/?id=0cd69b3d7ef357f2b43258de5831c4de0bd51dec
You may also be interested in https://commitfest.postgresql.org/patch/5907

--
jian
https://www.enterprisedb.com/

Nice, v11 looks good to me. I’ll change the status of the commitfest entry to ready for committer.

On 2026-03-11 We 11:36 PM, jian he wrote:

On Thu, Mar 12, 2026 at 3:50 AM Andrew Dunstan<andrew@dunslane.net> wrote:

Also added some tests.

V11 looks good to me.

Pushed after further minor review.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Nice that this was pushed. On a minor note, I saw that my email got confused in the commit (viktor.holmberg@aiven.io) instead of v@viktorh.net. (I don’t know what aiven.io is). I don’t know if there is a way to change this without messing up the git log? If not it’s no problem, probably unlikely that anyone will contact me about it anyways.

/Viktor

On 2026-03-13 Fr 4:43 AM, Viktor Holmberg wrote:

Nice that this was pushed. On a minor note, I saw that my email got
confused in the commit (viktor.holmberg@aiven.io
<https://mailto:viktor.holmberg@aiven.io&gt;) instead of v@viktorh.net.
(I don’t know what aiven.io is). I don’t know if there is a way to
change this without messing up the git log? If not it’s no problem,
probably unlikely that anyone will contact me about it anyways.

It's not really possible. My deepest apologies. Some how or other the
gadget I have for ensuring I credit everyone I should got confused. I
will disable it until I can figure out where it went wrong.

cheers

andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com

Ok, understand. No worries at all, just thought I’d flag it up.

/Viktor