Horton

Thread

Commits

  1. Fix bogus grammar for a CREATE CONSTRAINT TRIGGER error

  2. Remove dead code

  1. refactor AlterDomainAddConstraint (alter domain add constraint)

    jian he <jian.universality@gmail.com> — 2024-12-06T08:31:38Z

    hi.
attached patch refactor AlterDomainAddConstraint

* change the error message:
alter domain d_int add constraint nn not null no inherit;
from
ERROR:  NOT NULL constraints cannot be marked NO INHERIT
to
ERROR:  DOMAIN with NOT NULL constraints cannot be marked NO INHERIT

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")


* error out check constraint no inherit with domain. so the following
should fail.
alter domain d_int add constraint cc check(value > 1) no inherit; --should fail

* delete code in AlterDomainAddConstraint, since it will be unreachable.

* alter domain d_int add constraint cc2 check(value > 11) not
deferrable initially immediate not valid;
"not deferrable", "initially immediate" cannot error out at the moment.
maybe we can just document it in create_domain.sgml?

* some failed regress test, similar to thread (Pass ParseState as down
to utility functions)

you may also see the patch draft commit message.

  2. Re: refactor AlterDomainAddConstraint (alter domain add constraint)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-12-06T14:48:37Z

    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)

  3. Re: refactor AlterDomainAddConstraint (alter domain add constraint)

    jian he <jian.universality@gmail.com> — 2024-12-06T16:01:53Z

    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?

  4. Re: refactor AlterDomainAddConstraint (alter domain add constraint)

    jian he <jian.universality@gmail.com> — 2024-12-09T08:41:48Z

    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.

  5. Re: refactor AlterDomainAddConstraint (alter domain add constraint)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-01-14T16:37:14Z

    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)

  6. Re: refactor AlterDomainAddConstraint (alter domain add constraint)

    jian he <jian.universality@gmail.com> — 2025-01-15T05:34:58Z

    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

  7. Re: refactor AlterDomainAddConstraint (alter domain add constraint)

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-16T13:45:51Z

    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.

  8. Re: refactor AlterDomainAddConstraint (alter domain add constraint)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-03-10T18:37:19Z

    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
  https://postgr.es/m/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/

  9. Re: refactor AlterDomainAddConstraint (alter domain add constraint)

    Peter Eisentraut <peter@eisentraut.org> — 2025-11-24T14:45:48Z

    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
>    https://postgr.es/m/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?

  10. Re: refactor AlterDomainAddConstraint (alter domain add constraint)

    jian he <jian.universality@gmail.com> — 2025-11-26T05:07:50Z

    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
> >    https://postgr.es/m/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
https://postgr.es/m/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: https://postgr.es/m/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

  11. Re: refactor AlterDomainAddConstraint (alter domain add constraint)

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-11-26T12:07:38Z

    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.
                               http://postgr.es/m/482D1632.8010507@sigaev.ru