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?
Attachments:
v1-0001-Fix-CREATE-DOMAIN-create-two-not-null-constraint.patchapplication/x-patch; name=v1-0001-Fix-CREATE-DOMAIN-create-two-not-null-constraint.patchDownload
From 8328340ac98daa3e26ac13cc06348560a468abf0 Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Sun, 1 Jun 2025 11:32:28 +0800
Subject: [PATCH v1 1/1] Fix CREATE DOMAIN create two not null constraint
When you currently create a domain like int_domain1 with multiple NOT NULL
constraints, for example:
CREATE DOMAIN int_domain1 AS INT CONSTRAINT nn1 NOT NULL CONSTRAINT nn2 NOT NULL;
Currently we install two separate NOT NULL constraints. However, only one NOT
NULL constraint should be installed
discussion: https://postgr.es/m/
---
src/backend/commands/typecmds.c | 11 ++++++++---
src/test/regress/expected/domain.out | 10 ++++++++++
src/test/regress/sql/domain.sql | 6 ++++++
3 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 45ae7472ab5..b2a8d480866 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -731,6 +731,7 @@ DefineDomain(ParseState *pstate, CreateDomainStmt *stmt)
int32 basetypeMod;
Oid baseColl;
ObjectAddress address;
+ bool notnull_added = false;
/* Convert list of names to a name and namespace */
domainNamespace = QualifiedNameGetCreationNamespace(stmt->domainname,
@@ -1141,9 +1142,13 @@ DefineDomain(ParseState *pstate, CreateDomainStmt *stmt)
break;
case CONSTR_NOTNULL:
- domainAddNotNullConstraint(address.objectId, domainNamespace,
- basetypeoid, basetypeMod,
- constr, domainName, NULL);
+ if (!notnull_added)
+ {
+ domainAddNotNullConstraint(address.objectId, domainNamespace,
+ basetypeoid, basetypeMod,
+ constr, domainName, NULL);
+ notnull_added = true;
+ }
break;
/* Other constraint types were fully processed above */
diff --git a/src/test/regress/expected/domain.out b/src/test/regress/expected/domain.out
index ba6f05eeb7d..974e6637ff5 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -1013,6 +1013,16 @@ drop cascades to type dtop
-- enforced correctly, even if there's no default value for the new
-- column. Per bug #1433
create domain str_domain as text not null;
+create domain int_domain1 as int constraint nn1 not null constraint nn2 not null;
+select conname, contype
+from pg_constraint
+where contypid = 'int_domain1'::regtype
+order by conname;
+ conname | contype
+---------+---------
+ nn1 | n
+(1 row)
+
create table domain_test (a int, b int);
insert into domain_test values (1, 2);
insert into domain_test values (1, 2);
diff --git a/src/test/regress/sql/domain.sql b/src/test/regress/sql/domain.sql
index b752a63ab5f..97b0b6a822e 100644
--- a/src/test/regress/sql/domain.sql
+++ b/src/test/regress/sql/domain.sql
@@ -594,6 +594,12 @@ drop domain vchar4 cascade;
-- column. Per bug #1433
create domain str_domain as text not null;
+create domain int_domain1 as int constraint nn1 not null constraint nn2 not null;
+select conname, contype
+from pg_constraint
+where contypid = 'int_domain1'::regtype
+order by conname;
+
create table domain_test (a int, b int);
insert into domain_test values (1, 2);
--
2.34.1
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, likediff --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)