refactor AlterDomainAddConstraint (alter domain add constraint)

On 2024-Dec-06, jian he wrote:

basically processCASbits
from
processCASbits($3, @3, "NOT NULL")
processCASbits($5, @5, "CHECK")
to
processCASbits($3, @3, "DOMAIN with NOT NULL")
processCASbits($5, @5, "DOMAIN with CHECK")

This doesn't actually work from a translation point of view, because the
actual error message is split in two parts. I think it might be better
to pass a non-NULL variable to processCASbits, then in the caller check
whether that flag is true; if so, raise the error in a single ereport().

BTW the way to test this is to apply your patch, then do "make
update-po", then look at the src/backend/po/*.po.new files which contain
the merged strings. In this case, your new "DOMAIN with NOT NULL" string
is not going to appear in the message catalog, because processCASbits()
is not listed in GETTEXT_TRIGGERS in nls.mk.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)

On Fri, Dec 6, 2024 at 10:48 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

On 2024-Dec-06, jian he wrote:

basically processCASbits
from
processCASbits($3, @3, "NOT NULL")
processCASbits($5, @5, "CHECK")
to
processCASbits($3, @3, "DOMAIN with NOT NULL")
processCASbits($5, @5, "DOMAIN with CHECK")

This doesn't actually work from a translation point of view, because the
actual error message is split in two parts. I think it might be better
to pass a non-NULL variable to processCASbits, then in the caller check
whether that flag is true; if so, raise the error in a single ereport().

i let the error fail at AlterDomainAddConstraint.

what do you think?

hi.
ALTER DOMAIN ADD CONSTRAINT syntax more simple than CREATE DOMAIN.
So in gram.y, I changed DomainConstraintElem to the following:

DomainConstraintElem:
CHECK '(' a_expr ')'
{
Constraint *n = makeNode(Constraint);
n->contype = CONSTR_CHECK;
n->location = @1;
n->raw_expr = $3;
n->cooked_expr = NULL;
n->initially_valid = true;
$$ = (Node *) n;
}
| CHECK '(' a_expr ')' NOT VALID
{
Constraint *n = makeNode(Constraint);
n->contype = CONSTR_CHECK;
n->location = @1;
n->raw_expr = $3;
n->cooked_expr = NULL;
n->initially_valid = false;
n->skip_validation = true;
$$ = (Node *) n;
}
| NOT NULL_P
{
Constraint *n = makeNode(Constraint);
n->contype = CONSTR_NOTNULL;
n->location = @1;
n->keys = list_make1(makeString("value"));
n->initially_valid = true;
$$ = (Node *) n;
}

Now everything is the same as alter domain synopsis.
It all looks so simple.

Hello,

On 2024-Dec-09, jian he wrote:

ALTER DOMAIN ADD CONSTRAINT syntax more simple than CREATE DOMAIN.

Your proposed patch makes the code simpler, yes, but I think it also
makes the error messages worse. I don't think that's an improvement
from the user point of view.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)

On Wed, Jan 15, 2025 at 12:37 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

Hello,

On 2024-Dec-09, jian he wrote:

ALTER DOMAIN ADD CONSTRAINT syntax more simple than CREATE DOMAIN.

Your proposed patch makes the code simpler, yes, but I think it also
makes the error messages worse. I don't think that's an improvement
from the user point of view.

hi.
thanks for the comments!
we cannot error out AlterDomainAddConstraint for cases like ALTER
DOMAIN ADD CHECK NO INHERIT.
because "NO INHERIT" is actually a separate Constraint Node, and
AlterDomainAddConstraint
can only handle one Constraint node.

i believe I have addressed all the syntax problems related to the
ALTER DOMAIN command.
feel free to try the attached new patch.

examples with master:
create domain d_int as int4;
alter domain d_int add constraint cc check(value > 1) no inherit ;
---ok. success

alter domain d_int add constraint cc check(value > 1) not enforced; --error
ERROR: CHECK constraints cannot be marked NOT ENFORCED

alter domain d_int add constraint cc1 check(value > 1) not deferrable
initially immediate; --ok. success.
--------------------------------------------------------------------------
examples with patch:

alter domain d_int add constraint cc check(value > 1) no inherit;
ERROR: constraint specified as no-inherit is not supported for domains

alter domain d_int add constraint cc check(value > 1) not enforced;
ERROR: specifying constraint enforceability not supported for domains

alter domain d_int add constraint cc1 check(value > 1) not deferrable
initially immediate;
ERROR: specifying constraint deferrability not supported for domains

I have committed the part of this patch that removes the switch
statement in AlterDomainAddConstraint(). That one is dead code.

I'll study the other discussion a bit more. I agree the current code
isn't satisfactory.

Hello,

On 2025-Jan-15, jian he wrote:

we cannot error out AlterDomainAddConstraint for cases like ALTER
DOMAIN ADD CHECK NO INHERIT.
because "NO INHERIT" is actually a separate Constraint Node, and
AlterDomainAddConstraint
can only handle one Constraint node.

I had forgotten this thread, and I ended up implementing a different
solution for this issue, which I just posted at
/messages/by-id/202503101758.ipn3g64twfye@alvherre.pgsql

I like my patch better than this approach because it allows us to solve
the same problem as it appears in other parts of the grammar, and also
because it avoids the bit fiddling which is harder to maintain later on.
If you'd care to have a look at it, I'd appreciate it.

Thanks

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/

On 10.03.25 19:37, Alvaro Herrera wrote:

On 2025-Jan-15, jian he wrote:

we cannot error out AlterDomainAddConstraint for cases like ALTER
DOMAIN ADD CHECK NO INHERIT.
because "NO INHERIT" is actually a separate Constraint Node, and
AlterDomainAddConstraint
can only handle one Constraint node.

I had forgotten this thread, and I ended up implementing a different
solution for this issue, which I just posted at
/messages/by-id/202503101758.ipn3g64twfye@alvherre.pgsql

I like my patch better than this approach because it allows us to solve
the same problem as it appears in other parts of the grammar, and also
because it avoids the bit fiddling which is harder to maintain later on.
If you'd care to have a look at it, I'd appreciate it.

Where are we on this? Which of the two patches should we pursue?

On Mon, Nov 24, 2025 at 10:45 PM Peter Eisentraut <peter@eisentraut.org> wrote:

On 10.03.25 19:37, Alvaro Herrera wrote:

I had forgotten this thread, and I ended up implementing a different
solution for this issue, which I just posted at
/messages/by-id/202503101758.ipn3g64twfye@alvherre.pgsql

I like my patch better than this approach because it allows us to solve
the same problem as it appears in other parts of the grammar, and also
because it avoids the bit fiddling which is harder to maintain later on.
If you'd care to have a look at it, I'd appreciate it.

Where are we on this? Which of the two patches should we pursue?

hi.

if you go to this link
/messages/by-id/202503101758.ipn3g64twfye@alvherre.pgsql
check v2-0001-Improve-processCASbits-API-with-a-seen-struct.patch
you will find that it added a struct CAS_flags to processCASbits.

+typedef struct CAS_flags
+{
+ bool seen_deferrability;
+ bool seen_enforced;
+ bool seen_valid;
+ bool seen_inherit;
+} CAS_flags;

In v2-0001, most of the case processCASbits just pass a NULL CAS_flags(seen).
CAS_flags are not used in table constraints at all.
but CAS_flags indeed used for error message handling in ALTER DOMAIN
ADD CONSTRAINT.

(IMHO, it looks like big efforts to solve the same problem, also these bit
fiddling for domain constraint unlikely to change in the future, e.g. we are
unlike to add DEFERRABLE, INITIALLY DEFERRED, NO INHERIT constraints to domain.)

anyway, both patches are attached.
this thread: v5-0001-ALTER-DOMAIN-ADD-CONSTRAINT-error-message-enhance.patch

thread: /messages/by-id/202503101758.ipn3g64twfye@alvherre.pgsql
v5-0001-Improve-processCASbits-API-with-a-seen-struct.no-cfbot
v5-0002-handle-constraints-on-domains-too.no-cfbot

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

On 2025-Nov-26, jian he wrote:

(IMHO, it looks like big efforts to solve the same problem, also these bit
fiddling for domain constraint unlikely to change in the future, e.g. we are
unlike to add DEFERRABLE, INITIALLY DEFERRED, NO INHERIT constraints to domain.)

True.

I was thinking we would use the this mechanism to solve the issue for
constraint triggers, but in commit 87251e114967 I did that in a
different way. I think this one

this thread: v5-0001-ALTER-DOMAIN-ADD-CONSTRAINT-error-message-enhance.patch

is a reasonable implementation, ugly though it is. We could handle the
checks for deferrability before calling processCASbits() though.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
Tom: There seems to be something broken here.
Teodor: I'm in sackcloth and ashes... Fixed.
/messages/by-id/482D1632.8010507@sigaev.ru