Обсуждение: CREATE DOMAIN create two not null constraints

hi.

CREATE DOMAIN int_domain1 AS INT CONSTRAINT nn1 NOT NULL CONSTRAINT
nn2 NOT NULL;

will install two not-null pg_constraint entries.
we should have only one?

On 2025-Jun-01, jian he wrote:

> hi.
> 
> CREATE DOMAIN int_domain1 AS INT CONSTRAINT nn1 NOT NULL CONSTRAINT
> nn2 NOT NULL;
> 
> will install two not-null pg_constraint entries.
> we should have only one?

Hmm, I think it would be more consistent to reject the case of duplicate
constraints, instead of silently ignoring it.  So you'd do it in the
loop that checks for constraints before creating anything, like


diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 45ae7472ab5..b5daa61260b 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -944,6 +944,12 @@ DefineDomain(ParseState *pstate, CreateDomainStmt *stmt)
                             errcode(ERRCODE_SYNTAX_ERROR),
                             errmsg("conflicting NULL/NOT NULL constraints"),
                             parser_errposition(pstate, constr->location));
+
+                if (nullDefined)
+                    ereport(ERROR,
+                            errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+                            errmsg("redundant NOT NULL constraint definition"));
+
                 if (constr->is_no_inherit)
                     ereport(ERROR,
                             errcode(ERRCODE_INVALID_OBJECT_DEFINITION),

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Porque Kim no hacía nada, pero, eso sí,
con extraordinario éxito" ("Kim", Kipling)

On Mon, Jun 2, 2025 at 12:13 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> Hmm, I think it would be more consistent to reject the case of duplicate
> constraints, instead of silently ignoring it.  So you'd do it in the
> loop that checks for constraints before creating anything, like
>
>
> diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
> index 45ae7472ab5..b5daa61260b 100644
> --- a/src/backend/commands/typecmds.c
> +++ b/src/backend/commands/typecmds.c
> @@ -944,6 +944,12 @@ DefineDomain(ParseState *pstate, CreateDomainStmt *stmt)
>                                                         errcode(ERRCODE_SYNTAX_ERROR),
>                                                         errmsg("conflicting NULL/NOT NULL constraints"),
>                                                         parser_errposition(pstate, constr->location));
> +
> +                               if (nullDefined)
> +                                       ereport(ERROR,
> +                                                       errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
> +                                                       errmsg("redundant NOT NULL constraint definition"));
> +
>                                 if (constr->is_no_inherit)
>                                         ereport(ERROR,
>                                                         errcode(ERRCODE_INVALID_OBJECT_DEFINITION),

I don't have a preference.
error out would be fine, since it's a corner case.

On 2025-Jun-02, jian he wrote:

> On Mon, Jun 2, 2025 at 12:13 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:
> >
> > Hmm, I think it would be more consistent to reject the case of duplicate
> > constraints, instead of silently ignoring it.  So you'd do it in the
> > loop that checks for constraints before creating anything, like

> I don't have a preference.
> error out would be fine, since it's a corner case.

Yeah, if you were to specify that they have different properties, it'd
be messy to allow two to be given because you somehow have to merge
them.  Also, if something creates a constraint with a certain name, then
the jonstraint name should exist.  If we allow two names to be given,
one of them is lost which is undesirable.

Pushed that way.

I noticed that for the "conflicting NULL/NOT NULL constraints" error we
use the SYNTAX_ERROR errcode but for redundant ones we use
INVALID_OBJECT_DEFINITION.  Apparently this was changed in commit
33d4c828fde8 in 2003, in whose commit message Peter wrote

    Some "feature not supported" errors are better syntax errors, because the
    feature they complain about isn't a feature or cannot be implemented without
    definitional changes.

... not sure what to think about this.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El hombre nunca sabe de lo que es capaz hasta que lo intenta" (C. Dickens)