Horton

Thread

Commits

  1. Prevent creation of duplicate not-null constraints for domains

  2. Some "feature not supported" errors are better syntax errors, because the

  1. CREATE DOMAIN create two not null constraints

    jian he <jian.universality@gmail.com> — 2025-06-01T15:44:46Z

    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?

  2. Re: CREATE DOMAIN create two not null constraints

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-06-01T16:13:20Z

    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)

  3. Re: CREATE DOMAIN create two not null constraints

    jian he <jian.universality@gmail.com> — 2025-06-02T03:15:19Z

    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.

  4. Re: CREATE DOMAIN create two not null constraints

    Álvaro Herrera <alvherre@kurilemu.de> — 2025-07-03T09:52:56Z

    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)