Re: Catalog domain not-null constraints
On 09.04.24 10:44, jian he wrote: After studying this a bit more, I think moving forward in this direction is the best way. Attached is a new patch version, mainly with a more elaborate commit message. This patch makes the not-null constraint syntax consistent between CREATE DOMAIN and ALTER DOMAIN, and also makes the respective documentation correct. (Note that, as I show in the commit message, commit e5da0fe3c22 had in passing fixed a couple of bugs in CREATE and ALTER DOMAIN, so just reverting that commit wouldn't be a complete solution.) in ruleutils.c /* conkey is null for domain not-null constraints */ appendStringInfoString(, "NOT NULL VALUE"); should be /* conkey is null for domain not-null constraints */ appendStringInfoString(, "NOT NULL "); Good catch, fixed. currently src6=# \dD connotnull / QUERY */ SELECT n.nspname as "Schema", t.typname as "Name", pg_catalog.format_type(t.typbasetype, t.typtypmod) as "Type", (SELECT c.collname FROM pg_catalog.pg_collation c, pg_catalog.pg_type bt WHERE c.oid = t.typcollation AND bt.oid = t.typbasetype AND t.typcollation <> bt.typcollation) as "Collation", CASE WHEN t.typnotnull THEN 'not null' END as "Nullable", t.typdefault as "Default", pg_catalog.array_to_string(ARRAY( SELECT pg_catalog.pg_get_constraintdef(r.oid, true) FROM pg_catalog.pg_constraint r WHERE t.oid = r.contypid ), ' ') as "Check" FROM pg_catalog.pg_type t LEFT JOIN pg_catalog.pg_namespace n ON n.oid = t.typnamespace WHERE t.typtype = 'd' AND t.typname OPERATOR(pg_catalog.~) '^(connotnull)$' COLLATE pg_catalog.default AND pg_catalog.pg_type_is_visible(t.oid) ORDER BY 1, 2; // --- Since the last column is already named as "Check", maybe we need to change the query to pg_catalog.array_to_string(ARRAY( SELECT pg_catalog.pg_get_constraintdef(r.oid, true) FROM pg_catalog.pg_constraint r WHERE t.oid = r.contypid and r.contype = 'c' ), ' ') as "Check" That means domain can be associated with check constraint and not-null constraint. Yes, I changed it like you wrote. the url link destination is fine, but the url rendered name is "per the section called “Notes”" which seems strange, please see attached. Hmm, changing that would be an independent project. I have committed the patch with the two amendments you provided. I had also added a test of \dD and that caused some test output instability, so I added a ORDER BY r.conname to the \dD query.
Re: Catalog domain not-null constraints
On 21.03.24 12:23, Peter Eisentraut wrote:
All the examples in the tests append "value" to this, presumably by
analogy with CHECK constraints, but it looks as though anything works,
and is simply ignored:
ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL xxx; -- works
That doesn't seem particularly satisfactory. I think it should not
require (and reject) a column name after "NOT NULL".
Hmm. CREATE DOMAIN uses column constraint syntax, but ALTER DOMAIN uses
table constraint syntax. As long as you are only dealing with CHECK
constraints, there is no difference, but it shows up when using NOT NULL
constraint syntax. I agree that this is unsatisfactory. Attached is a
patch to try to sort this out.
After studying this a bit more, I think moving forward in this direction
is the best way. Attached is a new patch version, mainly with a more
elaborate commit message. This patch makes the not-null constraint
syntax consistent between CREATE DOMAIN and ALTER DOMAIN, and also makes
the respective documentation correct.
(Note that, as I show in the commit message, commit e5da0fe3c22 had in
passing fixed a couple of bugs in CREATE and ALTER DOMAIN, so just
reverting that commit wouldn't be a complete solution.)
From 4e4de402dda81798bb0286a09d39c6ed2f087228 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 8 Apr 2024 11:39:48 +0200
Subject: [PATCH v2] Fix ALTER DOMAIN NOT NULL syntax
In CREATE DOMAIN, a NOT NULL constraint looks like
CREATE DOMAIN d1 AS int [ CONSTRAINT conname ] NOT NULL
(Before e5da0fe3c22, the constraint name was accepted but ignored.)
But in ALTER DOMAIN, a NOT NULL constraint looks like
ALTER DOMAIN d1 ADD [ CONSTRAINT conname ] NOT NULL VALUE
where VALUE is where for a table constraint the column name would be.
(This works as of e5da0fe3c22. Before e5da0fe3c22, this syntax
resulted in an internal error.)
But for domains, this latter syntax is confusing and needlessly
inconsistent between CREATE and ALTER. So this changes it to just
ALTER DOMAIN d1 ADD [ CONSTRAINT conname ] NOT NULL
(None of these syntaxes are per SQL standard; we are just living with
the bits of inconsistency that have built up over time.)
Discussion:
https://www.postgresql.org/message-id/flat/9ec24d7b-633d-463a-84c6-7acff769c9e8%40eisentraut.org
---
doc/src/sgml/ref/create_domain.sgml | 17 ++--
src/backend/parser/gram.y| 60 ++--
src/test/regress/expected/domain.out | 8 ++--
src/test/regress/sql/domain.sql | 8 ++--
4 files changed, 79 insertions(+), 14 deletions(-)
diff --git a/doc/src/sgml/ref/create_domain.sgml
b/doc/src/sgml/ref/create_domain.sgml
index 73f9f28d6cf..078bea8410d 100644
--- a/doc/src/sgml/ref/create_domain.sgml
+++ b/doc/src/sgml/ref/create_domain.sgml
@@ -24,9 +24,9 @@
CREATE DOMAIN name [ AS ]
data_type
[ COLLATE collation ]
[ DEFAULT expression ]
-[ constraint [ ... ] ]
+[ domain_constraint [ ... ] ]
-where constraint
is:
+where domain_constraint
is:
[ CONSTRAINT constraint_name ]
{ NOT NULL | NULL | CHECK (expression) }
@@ -190,7 +190,7 @@ Parameters
-
+
Notes
@@ -279,6 +279,17 @@ Compatibility
The command CREATE DOMAIN conforms to the SQL
standard.
+
+
+ The syntax NOT NULL in this command is a
+ PostgreSQL extension. (A standard-conforming
+ way to write the same would be CHECK (VALUE IS NOT
+ NULL). However, per ,
+ such constraints a best avoided in practice anyway.) The
+ NULL constraint is a
+ PostgreSQL extension (see also ).
+
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 0523f7e891e..e8b619926ef 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -524,7 +524,7 @@ static Node *makeRecursiveViewSelect(char *relname, List
*aliases, Node *query);
%type generic_set set_rest set_rest_more generic_reset reset_rest
SetResetClause FunctionSetResetClause
-%typeTableElement TypedTableElement ConstraintElem TableFuncElement
+%typeTableElement TypedTableElement ConstraintElem
DomainConstraintElem TableFuncElement
%typecolumnDef columnOptions optionalPeriodName
%type def_elem reloption_elem old_aggr_elem operator_def_elem
%typedef_arg columnElem where_clause where_or_current_clause
@@ -596,7 +596,7 @@ static Node *makeRecursiveViewSelect(char *relname, List
*aliases, Node *query);
%type col_name_keyword reserved_keyword
%type bare_label_keyword
-%typeTableConstraint TableLikeClause
+%typeDomainConstraint TableConstraint TableLikeClause
%typeTableLikeOptionList TableLikeOption
%type column_compression opt_column_compression
column_storage opt_column_storage
%typeColQualList
@@ -4334,6 +4334,60 @@ ConstraintElem:
}
;
+/*
+ * DomainConstraint is separate from TableConstraint because the syntax for
+ * NOT NULL constraints is
Re: Catalog domain not-null constraints
On Tue, Mar 26, 2024 at 2:28 AM Dean Rasheed wrote: > > On Fri, 22 Mar 2024 at 08:28, jian he wrote: > > > > On Thu, Mar 21, 2024 at 7:23 PM Peter Eisentraut > > wrote: > > > > > > Hmm. CREATE DOMAIN uses column constraint syntax, but ALTER DOMAIN uses > > > table constraint syntax. Attached is a patch to try to sort this out. > > > > also you should also change src/backend/utils/adt/ruleutils.c? > > > > src6=# \dD > > List of domains > > Schema |Name | Type | Collation | Nullable | Default | > > Check > > +-+-+---+--+-+-- > > public | domain_test | integer | | not null | | > > CHECK (VALUE > 0) NOT NULL VALUE > > (1 row) > > > > probably change to CHECK (VALUE IS NOT NULL) > > I'd say it should just output "NOT NULL", since that's the input > syntax that created the constraint. But then again, why display NOT > NULL constraints in that column at all, when there's a separate > "Nullable" column? > create table sss(a int not null); SELECT pg_get_constraintdef(oid) FROM pg_constraint WHERE conname = 'sss_a_not_null'; returns " NOT NULL a" I think just outputting "NOT NULL" is ok for the domain, given the table constraint is "NOT NULL" + table column, per above example. yech, we already have a "Nullable" column, so we don't need to display NOT NULL constraints.
Re: Catalog domain not-null constraints
On Tue, 26 Mar 2024 at 07:30, Alvaro Herrera wrote: > > On 2024-Mar-25, Dean Rasheed wrote: > > > Also (not this patch's fault), psql doesn't seem to offer a way to > > display domain constraint names -- something you need to know to drop > > or alter them. Perhaps \dD+ could be made to do that? > > Ooh, I remember we had offered a patch for \d++ to display these > constraint names for tables, but didn't get around to gather consensus > for it. We did gather consensus on *not* wanting \d+ to display them, > but we need *something*. I suppose we should do something symmetrical > for tables and domains. How about \dD++ and \dt++? > Personally, I quite like the fact that \d+ displays NOT NULL constraints, because it puts them on an equal footing with CHECK constraints. However, I can appreciate that it will significantly increase the length of the output in some cases. With \dD it's not so nice because of the way it puts all the details on one line. The obvious output might look something like this: \dD List of domains Schema | Name | Type | Collation | Nullable | Default | Check +--+-+---+--+-+--- public | d1 | integer | | NOT NULL | | CHECK (VALUE > 0) \dD+ List of domains Schema | Name | Type | Collation | Nullable | Default |Check | Access privileges | Description +--+-+---+-+-+---+---+- public | d1 | integer | | CONSTRAINT d1_not_null NOT NULL | | CONSTRAINT d1_check CHECK (VALUE > 0) | | So you'd need quite a wide window to easily view it (or use \x). I suppose the width could be reduced by dropping the word "CONSTRAINT" in the \dD+ case, but it's probably still going to be wider than the average window. Regards, Dean
Re: Catalog domain not-null constraints
On 2024-Mar-25, Dean Rasheed wrote: > Also (not this patch's fault), psql doesn't seem to offer a way to > display domain constraint names -- something you need to know to drop > or alter them. Perhaps \dD+ could be made to do that? Ooh, I remember we had offered a patch for \d++ to display these constraint names for tables, but didn't get around to gather consensus for it. We did gather consensus on *not* wanting \d+ to display them, but we need *something*. I suppose we should do something symmetrical for tables and domains. How about \dD++ and \dt++? -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'. After collecting 500 such letters, he mused, a university somewhere in Arizona would probably grant him a degree. (Don Knuth)
Re: Catalog domain not-null constraints
On Fri, 22 Mar 2024 at 08:28, jian he wrote: > > On Thu, Mar 21, 2024 at 7:23 PM Peter Eisentraut wrote: > > > > Hmm. CREATE DOMAIN uses column constraint syntax, but ALTER DOMAIN uses > > table constraint syntax. Attached is a patch to try to sort this out. > > also you should also change src/backend/utils/adt/ruleutils.c? > > src6=# \dD > List of domains > Schema |Name | Type | Collation | Nullable | Default | > Check > +-+-+---+--+-+-- > public | domain_test | integer | | not null | | > CHECK (VALUE > 0) NOT NULL VALUE > (1 row) > > probably change to CHECK (VALUE IS NOT NULL) I'd say it should just output "NOT NULL", since that's the input syntax that created the constraint. But then again, why display NOT NULL constraints in that column at all, when there's a separate "Nullable" column? Also (not this patch's fault), psql doesn't seem to offer a way to display domain constraint names -- something you need to know to drop or alter them. Perhaps \dD+ could be made to do that? + The syntax NOT NULL in this command is a + PostgreSQL extension. (A standard-conforming + way to write the same would be CHECK (VALUE IS NOT + NULL). However, per , + such constraints a best avoided in practice anyway.) The + NULL constraint is a + PostgreSQL extension (see also ). I didn't verify this, but I thought that according to the SQL standard, only non-NULL values should be passed to CHECK constraints, so there is no standard-conforming way to write a NOT NULL domain constraint. FWIW, I think NOT NULL domain constraints are a useful feature to have, and I suspect that there are more people out there who use them and like them, than who care what the SQL standard says. If so, I'm in favour of allowing them to be named and managed in the same way as NOT NULL table constraints. + processCASbits($5, @5, "CHECK", + NULL, NULL, >skip_validation, + >is_no_inherit, yyscanner); + n->initially_valid = !n->skip_validation; + /* no NOT VALID support yet */ + processCASbits($3, @3, "NOT NULL", + NULL, NULL, NULL, + >is_no_inherit, yyscanner); + n->initially_valid = true; NO INHERIT is allowed for domain constraints? What does that even mean? There's something very wonky about this: CREATE DOMAIN d1 AS int CHECK (value > 0) NO INHERIT; -- Rejected ERROR: check constraints for domains cannot be marked NO INHERIT CREATE DOMAIN d1 AS int; ALTER DOMAIN d1 ADD CHECK (value > 0) NO INHERIT; -- Allowed CREATE DOMAIN d2 AS int NOT NULL NO INHERIT; -- Now allowed (used to syntax error) CREATE DOMAIN d3 AS int; ALTER DOMAIN d3 ADD NOT NULL NO INHERIT; -- Allowed Presumably all of those should be rejected in the grammar. Regards, Dean
Re: Catalog domain not-null constraints
On Thu, Mar 21, 2024 at 7:23 PM Peter Eisentraut wrote:
>
> On 20.03.24 12:22, Dean Rasheed wrote:
> > Hmm, for CHECK constraints, the ALTER DOMAIN syntax for adding a
> > constraint is the same as for CREATE DOMAIN, but that's not the case
> > for NOT NULL constraints. So, for example, these both work:
> >
> > CREATE DOMAIN d AS int CONSTRAINT c1 CHECK (value > 0);
> >
> > ALTER DOMAIN d ADD CONSTRAINT c2 CHECK (value < 10);
> >
> > However, for NOT NULL constraints, the ALTER DOMAIN syntax differs
> > from the CREATE DOMAIN syntax, because it expects "NOT NULL" to be
> > followed by a column name. So the following CREATE DOMAIN syntax
> > works:
> >
> > CREATE DOMAIN d AS int CONSTRAINT nn NOT NULL;
> >
> > but the equivalent ALTER DOMAIN syntax doesn't work:
> >
> > ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL;
> >
> > ERROR: syntax error at or near ";"
> > LINE 1: ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL;
> > ^
> >
> > All the examples in the tests append "value" to this, presumably by
> > analogy with CHECK constraints, but it looks as though anything works,
> > and is simply ignored:
> >
> > ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL xxx; -- works
> >
> > That doesn't seem particularly satisfactory. I think it should not
> > require (and reject) a column name after "NOT NULL".
>
> Hmm. CREATE DOMAIN uses column constraint syntax, but ALTER DOMAIN uses
> table constraint syntax. As long as you are only dealing with CHECK
> constraints, there is no difference, but it shows up when using NOT NULL
> constraint syntax. I agree that this is unsatisfactory. Attached is a
> patch to try to sort this out.
>
+ | NOT NULL_P ConstraintAttributeSpec
+ {
+ Constraint *n = makeNode(Constraint);
+
+ n->contype = CONSTR_NOTNULL;
+ n->location = @1;
+ n->keys = list_make1(makeString("value"));
+ /* no NOT VALID support yet */
+ processCASbits($3, @3, "NOT NULL",
+ NULL, NULL, NULL,
+ >is_no_inherit, yyscanner);
+ n->initially_valid = true;
+ $$ = (Node *) n;
+ }
i don't understand this part.
+ n->keys = list_make1(makeString("value"));
also you should also change src/backend/utils/adt/ruleutils.c?
src6=# create domain domain_test integer;
alter domain domain_test add constraint pos1 check (value > 0);
alter domain domain_test add constraint constr1 not null ;
CREATE DOMAIN
ALTER DOMAIN
ALTER DOMAIN
src6=# \dD
List of domains
Schema |Name | Type | Collation | Nullable | Default |
Check
+-+-+---+--+-+--
public | domain_test | integer | | not null | |
CHECK (VALUE > 0) NOT NULL VALUE
(1 row)
probably change to CHECK (VALUE IS NOT NULL)
- appendStringInfoString(,
"NOT NULL VALUE");
+ appendStringInfoString(,
"CHECK (VALUE IS NOT NULL)");
seems works.
Re: Catalog domain not-null constraints
On 3/22/24 01:46, Tom Lane wrote: Vik Fearing writes: Anyway, I will bring this up with the committee and report back. My proposed solution will be for CAST to check domain constraints even if the input is NULL. Please do not claim that that is the position of the Postgres project. Everything that I do on the SQL Committee is in my own name. I do not speak for either PostgreSQL or for EDB (my employer), even though my opinions are of course often influenced by some combination of both. -- Vik Fearing
Re: Catalog domain not-null constraints
Vik Fearing writes: > On 3/22/24 00:17, Tom Lane wrote: >> Vik Fearing writes: >>> As also said somewhere in that thread, I think that >>> short-cutting a NULL input value without considering the constraints of >>> a domain is a bug that needs to be fixed in the standard. >> I think it's probably intentional. It certainly fits with the lack of >> syntax for DOMAIN NOT NULL. Also, it's been like that since SQL99; >> do you think nobody's noticed it for 25 years? > Haven't we (postgres) had bug reports of similar age? Well, they've looked it at it since then. SQL99 has c) If SV is the null value, then the result is the null value. SQL:2008 and later have the text I quoted: c) If SV is the null value, then the result of CS is the null value and no further General Rules of this Sub-clause are applied. I find it *extremely* hard to believe that they would have added that explicit text without noticing exactly which operations they were saying to skip. > Anyway, I will bring this up with the committee and report back. My > proposed solution will be for CAST to check domain constraints even if > the input is NULL. Please do not claim that that is the position of the Postgres project. regards, tom lane
Re: Catalog domain not-null constraints
On 3/22/24 00:17, Tom Lane wrote: Vik Fearing writes: On 3/21/24 15:30, Tom Lane wrote: The SQL spec's answer to that conundrum appears to be "NULL is a valid value of every domain, and if you don't like it, tough". I don't see how you can infer this from the standard at all. I believe where we got that from is 6.13 , which quoth (general rule 2): c) If SV is the null value, then the result of CS is the null value and no further General Rules of this Subclause are applied. In particular, that short-circuits application of the domain constraints (GR 23), implying that CAST(NULL AS some_domain) is always successful. Now you could argue that there's some other context that would reject nulls, but being inconsistent with CAST would seem more like a bug than a feature. I think the main bug is in what you quoted from . I believe that the POLA for casting to a domain is for all constraints of the domain to be verified for ALL values including the null value. As also said somewhere in that thread, I think that short-cutting a NULL input value without considering the constraints of a domain is a bug that needs to be fixed in the standard. I think it's probably intentional. It certainly fits with the lack of syntax for DOMAIN NOT NULL. Also, it's been like that since SQL99; do you think nobody's noticed it for 25 years? Haven't we (postgres) had bug reports of similar age? There is also the possibility that no one has noticed because major players have not implemented domains. For example, Oracle only just got them last year: https://blogs.oracle.com/coretec/post/less-coding-with-sql-domains-in-23c Anyway, I will bring this up with the committee and report back. My proposed solution will be for CAST to check domain constraints even if the input is NULL. -- Vik Fearing
Re: Catalog domain not-null constraints
Vik Fearing writes: > On 3/21/24 15:30, Tom Lane wrote: >> The SQL spec's answer to that conundrum appears to be "NULL is >> a valid value of every domain, and if you don't like it, tough". > I don't see how you can infer this from the standard at all. I believe where we got that from is 6.13 , which quoth (general rule 2): c) If SV is the null value, then the result of CS is the null value and no further General Rules of this Subclause are applied. In particular, that short-circuits application of the domain constraints (GR 23), implying that CAST(NULL AS some_domain) is always successful. Now you could argue that there's some other context that would reject nulls, but being inconsistent with CAST would seem more like a bug than a feature. > As also said somewhere in that thread, I think that > short-cutting a NULL input value without considering the constraints of > a domain is a bug that needs to be fixed in the standard. I think it's probably intentional. It certainly fits with the lack of syntax for DOMAIN NOT NULL. Also, it's been like that since SQL99; do you think nobody's noticed it for 25 years? regards, tom lane
Re: Catalog domain not-null constraints
On 3/21/24 15:30, Tom Lane wrote: Peter Eisentraut writes: A quick reading of the SQL standard suggests to me that the way we are doing null handling in domain constraints is all wrong. The standard says that domain constraints are only checked on values that are not null. So both the handling of constraints using the CHECK syntax is nonstandard and the existence of explicit NOT NULL constraints is an extension. The CREATE DOMAIN reference page already explains why all of this is a bad idea. Do we want to document all of that further, or maybe we just want to rip out domain not-null constraints, or at least not add further syntax for it? Yeah. The real problem with domain not null is: how can a column that's propagated up through the nullable side of an outer join still be considered to belong to such a domain? Per spec, it is not considered to be so. The domain only applies to table storage and CASTs and gets "forgotten" in a query. The SQL spec's answer to that conundrum appears to be "NULL is a valid value of every domain, and if you don't like it, tough". I don't see how you can infer this from the standard at all. I'm too lazy to search the archives, but we have had at least one previous discussion about how we should adopt the spec's semantics. It'd be an absolutely trivial fix in CoerceToDomain (succeed immediately if input is NULL), but the question is what to do with existing "DOMAIN NOT NULL" DDL. Here is a semi-random link into a conversation you and I have recently had about this: https://www.postgresql.org/message-id/a13db59c-c68f-4a30-87a5-177fe135665e%40postgresfriends.org As also said somewhere in that thread, I think that short-cutting a NULL input value without considering the constraints of a domain is a bug that needs to be fixed in the standard. -- Vik Fearing
Re: Catalog domain not-null constraints
On Thu, 21 Mar 2024 at 10:30, Tom Lane wrote: > The SQL spec's answer to that conundrum appears to be "NULL is > a valid value of every domain, and if you don't like it, tough". > To be fair, NULL is a valid value of every type. Even VOID has NULL. In this context, it’s a bit weird to be able to decree up front when defining a type that no table column of that type, anywhere, may ever contain a NULL. It would be nice if there was a way to reverse the default so that if you (almost or) never want NULLs anywhere that’s what you get without saying "NOT NULL" all over the place, and instead just specify "NULLABLE" (or something) where you want. But that effectively means optionally changing the behaviour of CREATE TABLE and ALTER TABLE.
Re: Catalog domain not-null constraints
Peter Eisentraut writes: > > A quick reading of the SQL standard suggests to me that the way we are > doing null handling in domain constraints is all wrong. The standard > says that domain constraints are only checked on values that are not > null. So both the handling of constraints using the CHECK syntax is > nonstandard and the existence of explicit NOT NULL constraints is an > extension. The CREATE DOMAIN reference page already explains why all of > this is a bad idea. Do we want to document all of that further, or > maybe we just want to rip out domain not-null constraints, or at least > not add further syntax for it? > Yeah. The real problem with domain not null is: how can a column that's propagated up through the nullable side of an outer join still be considered to belong to such a domain? The SQL spec's answer to that conundrum appears to be "NULL is a valid value of every domain, and if you don't like it, tough". I'm too lazy to search the archives, but we have had at least one previous discussion about how we should adopt the spec's semantics. It'd be an absolutely trivial fix in CoerceToDomain (succeed immediately if input is NULL), but the question is what to do with existing "DOMAIN NOT NULL" DDL. Anyway, now that I recall all that, e5da0fe3c is throwing good work after bad, and I wonder if we shouldn't revert it. regards, tom lane
Re: Catalog domain not-null constraints
On 20.03.24 12:22, Dean Rasheed wrote:
Hmm, for CHECK constraints, the ALTER DOMAIN syntax for adding a
constraint is the same as for CREATE DOMAIN, but that's not the case
for NOT NULL constraints. So, for example, these both work:
CREATE DOMAIN d AS int CONSTRAINT c1 CHECK (value > 0);
ALTER DOMAIN d ADD CONSTRAINT c2 CHECK (value < 10);
However, for NOT NULL constraints, the ALTER DOMAIN syntax differs
from the CREATE DOMAIN syntax, because it expects "NOT NULL" to be
followed by a column name. So the following CREATE DOMAIN syntax
works:
CREATE DOMAIN d AS int CONSTRAINT nn NOT NULL;
but the equivalent ALTER DOMAIN syntax doesn't work:
ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL;
ERROR: syntax error at or near ";"
LINE 1: ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL;
^
All the examples in the tests append "value" to this, presumably by
analogy with CHECK constraints, but it looks as though anything works,
and is simply ignored:
ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL xxx; -- works
That doesn't seem particularly satisfactory. I think it should not
require (and reject) a column name after "NOT NULL".
Hmm. CREATE DOMAIN uses column constraint syntax, but ALTER DOMAIN uses
table constraint syntax. As long as you are only dealing with CHECK
constraints, there is no difference, but it shows up when using NOT NULL
constraint syntax. I agree that this is unsatisfactory. Attached is a
patch to try to sort this out.
Looking in the SQL spec, it seems to only mention adding CHECK
constraints to domains, so the option to add NOT NULL constraints
should probably be listed in the "Compatibility" section.
A quick reading of the SQL standard suggests to me that the way we are
doing null handling in domain constraints is all wrong. The standard
says that domain constraints are only checked on values that are not
null. So both the handling of constraints using the CHECK syntax is
nonstandard and the existence of explicit NOT NULL constraints is an
extension. The CREATE DOMAIN reference page already explains why all of
this is a bad idea. Do we want to document all of that further, or
maybe we just want to rip out domain not-null constraints, or at least
not add further syntax for it?
From 0bdc4944879670b7b53447833fa9f9b385eb0309 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 21 Mar 2024 12:15:11 +0100
Subject: [PATCH] WIP: Fix ALTER DOMAIN NOT NULL syntax
---
doc/src/sgml/ref/create_domain.sgml | 17 ++--
src/backend/parser/gram.y| 60 ++--
src/test/regress/expected/domain.out | 8 ++--
src/test/regress/sql/domain.sql | 8 ++--
4 files changed, 79 insertions(+), 14 deletions(-)
diff --git a/doc/src/sgml/ref/create_domain.sgml
b/doc/src/sgml/ref/create_domain.sgml
index 73f9f28d6cf..078bea8410d 100644
--- a/doc/src/sgml/ref/create_domain.sgml
+++ b/doc/src/sgml/ref/create_domain.sgml
@@ -24,9 +24,9 @@
CREATE DOMAIN name [ AS ]
data_type
[ COLLATE collation ]
[ DEFAULT expression ]
-[ constraint [ ... ] ]
+[ domain_constraint [ ... ] ]
-where constraint
is:
+where domain_constraint
is:
[ CONSTRAINT constraint_name ]
{ NOT NULL | NULL | CHECK (expression) }
@@ -190,7 +190,7 @@ Parameters
-
+
Notes
@@ -279,6 +279,17 @@ Compatibility
The command CREATE DOMAIN conforms to the SQL
standard.
+
+
+ The syntax NOT NULL in this command is a
+ PostgreSQL extension. (A standard-conforming
+ way to write the same would be CHECK (VALUE IS NOT
+ NULL). However, per ,
+ such constraints a best avoided in practice anyway.) The
+ NULL constraint is a
+ PostgreSQL extension (see also ).
+
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index 39a801a1c38..391a708467e 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -522,7 +522,7 @@ static Node *makeRecursiveViewSelect(char *relname, List
*aliases, Node *query);
%type generic_set set_rest set_rest_more generic_reset reset_rest
SetResetClause FunctionSetResetClause
-%typeTableElement TypedTableElement ConstraintElem TableFuncElement
+%typeTableElement TypedTableElement ConstraintElem
DomainConstraintElem TableFuncElement
%typecolumnDef columnOptions
%type def_elem reloption_elem old_aggr_elem operator_def_elem
%typedef_arg columnElem where_clause where_or_current_clause
@@ -593,7 +593,7 @@ static Node *makeRecursiveViewSelect(char *relname, List
*aliases, Node *query);
%type col_name_keyword reserved_keyword
%type bare_label_keyword
-%typeTableConstraint TableLikeClause
+%typeDomainConstraint TableConstraint TableLikeClause
%typeTableLikeOptionList TableLikeOption
%type column_compression opt_column_compression
column_storage opt_column_storage
%typeColQualList
@@ -4251,6 +4251,60 @@ ConstraintElem:
Re: Catalog domain not-null constraints
On Wed, 20 Mar 2024 at 09:43, Peter Eisentraut wrote: > > On 19.03.24 10:57, jian he wrote: > > this new syntax need to be added into the alter_domain.sgml's synopsis and > > also > > need an explanation varlistentry? > > The ALTER DOMAIN reference page refers to CREATE DOMAIN about the > details of the constraint syntax. I believe this is still accurate. We > could add more detail locally on the ALTER DOMAIN page, but that is not > this patch's job. For example, the details of CHECK constraints are > also not shown on the ALTER DOMAIN page right now. > Hmm, for CHECK constraints, the ALTER DOMAIN syntax for adding a constraint is the same as for CREATE DOMAIN, but that's not the case for NOT NULL constraints. So, for example, these both work: CREATE DOMAIN d AS int CONSTRAINT c1 CHECK (value > 0); ALTER DOMAIN d ADD CONSTRAINT c2 CHECK (value < 10); However, for NOT NULL constraints, the ALTER DOMAIN syntax differs from the CREATE DOMAIN syntax, because it expects "NOT NULL" to be followed by a column name. So the following CREATE DOMAIN syntax works: CREATE DOMAIN d AS int CONSTRAINT nn NOT NULL; but the equivalent ALTER DOMAIN syntax doesn't work: ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL; ERROR: syntax error at or near ";" LINE 1: ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL; ^ All the examples in the tests append "value" to this, presumably by analogy with CHECK constraints, but it looks as though anything works, and is simply ignored: ALTER DOMAIN d ADD CONSTRAINT nn NOT NULL xxx; -- works That doesn't seem particularly satisfactory. I think it should not require (and reject) a column name after "NOT NULL". Looking in the SQL spec, it seems to only mention adding CHECK constraints to domains, so the option to add NOT NULL constraints should probably be listed in the "Compatibility" section. Regards, Dean
Re: Catalog domain not-null constraints
On 19.03.24 10:57, jian he wrote: this new syntax need to be added into the alter_domain.sgml's synopsis and also need an explanation varlistentry? The ALTER DOMAIN reference page refers to CREATE DOMAIN about the details of the constraint syntax. I believe this is still accurate. We could add more detail locally on the ALTER DOMAIN page, but that is not this patch's job. For example, the details of CHECK constraints are also not shown on the ALTER DOMAIN page right now. + false, /* connoinherit */ + false, /* conwithoutoverlaps */ + false); /* is_internal */ /* conwithoutoverlaps */ should be /* conperiod */ Good catch, thanks.
Re: Catalog domain not-null constraints
On 18.03.24 11:02, Aleksander Alekseev wrote: Hi, Anyway, in order to move this forward, here is an updated patch where the ADD CONSTRAINT ... NOT NULL behavior for domains matches the idempotent behavior of tables. This uses the patch that Jian He posted. I tested the patch on Raspberry Pi 5 and Intel MacBook and also experimented with it. Everything seems to work properly. Personally I believe new functions such as validateDomainNotNullConstraint() and findDomainNotNullConstraint() could use a few lines of comments (accepts..., returns..., etc). Done. Also I think that the commit message should explicitly say that supporting NOT VALID constraints is out of scope of this patch. Not done. I don't know what NOT VALID has to do with this. This patch just changes the internal catalog representation, it doesn't claim to add or change any features. The documentation already accurately states that NOT VALID is not supported for NOT NULL constraints. Except for named nitpicks v4 LGTM. Committed, thanks.
Re: Catalog domain not-null constraints
create domain connotnull integer; create table domconnotnulltest ( col1 connotnull , col2 connotnull ); alter domain connotnull add not null value; --- the above query does not work in pg16. ERROR: syntax error at or near "not". after applying the patch, now this works. this new syntax need to be added into the alter_domain.sgml's synopsis and also need an explanation varlistentry? + /* + * Store the constraint in pg_constraint + */ + ccoid = + CreateConstraintEntry(constr->conname, /* Constraint Name */ + domainNamespace, /* namespace */ + CONSTRAINT_NOTNULL, /* Constraint Type */ + false, /* Is Deferrable */ + false, /* Is Deferred */ + !constr->skip_validation, /* Is Validated */ + InvalidOid, /* no parent constraint */ + InvalidOid, /* not a relation constraint */ + NULL, + 0, + 0, + domainOid, /* domain constraint */ + InvalidOid, /* no associated index */ + InvalidOid, /* Foreign key fields */ + NULL, + NULL, + NULL, + NULL, + 0, + ' ', + ' ', + NULL, + 0, + ' ', + NULL, /* not an exclusion constraint */ + NULL, + NULL, + true, /* is local */ + 0, /* inhcount */ + false, /* connoinherit */ + false, /* conwithoutoverlaps */ + false); /* is_internal */ /* conwithoutoverlaps */ should be /* conperiod */
Re: Catalog domain not-null constraints
Hi, > Anyway, in order to move this forward, here is an updated patch where > the ADD CONSTRAINT ... NOT NULL behavior for domains matches the > idempotent behavior of tables. This uses the patch that Jian He posted. I tested the patch on Raspberry Pi 5 and Intel MacBook and also experimented with it. Everything seems to work properly. Personally I believe new functions such as validateDomainNotNullConstraint() and findDomainNotNullConstraint() could use a few lines of comments (accepts..., returns..., etc). Also I think that the commit message should explicitly say that supporting NOT VALID constraints is out of scope of this patch. Except for named nitpicks v4 LGTM. -- Best regards, Aleksander Alekseev
Re: Catalog domain not-null constraints
Anyway, in order to move this forward, here is an updated patch where
the ADD CONSTRAINT ... NOT NULL behavior for domains matches the
idempotent behavior of tables. This uses the patch that Jian He posted.
From a0075e4bcd5f2db292f92fc2b70576ccebd07210 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 23 Nov 2023 07:35:32 +0100
Subject: [PATCH v4 1/2] Add tests for domain-related information schema views
Discussion:
https://www.postgresql.org/message-id/flat/9ec24d7b-633d-463a-84c6-7acff769c9e8%40eisentraut.org
---
src/test/regress/expected/domain.out | 47
src/test/regress/sql/domain.sql | 24 ++
2 files changed, 71 insertions(+)
diff --git a/src/test/regress/expected/domain.out
b/src/test/regress/expected/domain.out
index 6d94e84414a..e70aebd70c0 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -1207,3 +1207,50 @@ create domain testdomain1 as int constraint unsigned
check (value > 0);
alter domain testdomain1 rename constraint unsigned to unsigned_foo;
alter domain testdomain1 drop constraint unsigned_foo;
drop domain testdomain1;
+--
+-- Information schema
+--
+SELECT * FROM information_schema.column_domain_usage
+ WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+ ORDER BY domain_name;
+ domain_catalog | domain_schema | domain_name | table_catalog | table_schema |
table_name | column_name
++---+-+---+--++-
+ regression | public| con | regression| public |
domcontest | col1
+ regression | public| dom | regression| public |
domview| col1
+ regression | public| things | regression| public |
thethings | stuff
+(3 rows)
+
+SELECT * FROM information_schema.domain_constraints
+ WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+ ORDER BY constraint_name;
+ constraint_catalog | constraint_schema | constraint_name | domain_catalog |
domain_schema | domain_name | is_deferrable | initially_deferred
++---+-++---+-+---+
+ regression | public| con_check | regression |
public| con | NO| NO
+ regression | public| meow| regression |
public| things | NO| NO
+ regression | public| pos_int_check | regression |
public| pos_int | NO| NO
+(3 rows)
+
+SELECT * FROM information_schema.domains
+ WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+ ORDER BY domain_name;
+ domain_catalog | domain_schema | domain_name | data_type |
character_maximum_length | character_octet_length | character_set_catalog |
character_set_schema | character_set_name | collation_catalog |
collation_schema | collation_name | numeric_precision | numeric_precision_radix
| numeric_scale | datetime_precision | interval_type | interval_precision |
domain_default | udt_catalog | udt_schema | udt_name | scope_catalog |
scope_schema | scope_name | maximum_cardinality | dtd_identifier
++---+-+---+--++---+--++---+--++---+-+---++---+++-++--+---+--++-+
+ regression | public| con | integer |
|| | |
| | ||
32 | 2 | 0 ||
||| regression | pg_catalog
| int4 | | || | 1
+ regression | public| dom | integer |
|| | |
| | ||
32 | 2 | 0 ||
||| regression | pg_catalog
| int4 | | || | 1
+ regression | public| pos_int | integer |
|| | |
| | |
Re: Catalog domain not-null constraints
On 14.03.24 15:03, Alvaro Herrera wrote: On 2024-Mar-14, Peter Eisentraut wrote: Perhaps it would make sense if we change the ALTER TABLE command to be like ALTER TABLE t1 ADD IF NOT EXISTS NOT NULL c1 Then the behavior is like one would expect. For ALTER TABLE, we would reject this command if IF NOT EXISTS is not specified. (Since this is mainly for pg_dump, it doesn't really matter for usability.) For ALTER DOMAIN, we could accept both variants. I don't understand why you want to change this behavior, though. Because in the abstract, the behavior of ALTER TABLE t1 ADD should be to add a constraint. In the current implementation, the behavior is different for different constraint types.
Re: Catalog domain not-null constraints
On 2024-Mar-14, Peter Eisentraut wrote: > Perhaps it would make sense if we change the ALTER TABLE command to be like > > ALTER TABLE t1 ADD IF NOT EXISTS NOT NULL c1 > > Then the behavior is like one would expect. > > For ALTER TABLE, we would reject this command if IF NOT EXISTS is not > specified. (Since this is mainly for pg_dump, it doesn't really matter for > usability.) For ALTER DOMAIN, we could accept both variants. I don't understand why you want to change this behavior, though. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "La victoria es para quien se atreve a estar solo"
Re: Catalog domain not-null constraints
On 12.02.24 11:24, Alvaro Herrera wrote: On 2024-Feb-11, Peter Eisentraut wrote: But I see that table constraints do not work that way. A command like ALTER TABLE t1 ADD NOT NULL c1 does nothing if the column already has a NOT NULL constraint. I'm not sure this is correct. At least it's not documented. We should probably make the domains feature work the same way, but I would like to understand why it works that way first. The main source of nastiness, when we allow multiple constraints, is constraint inheritance. If we allow just one constraint per column, then it's always easy to know what to do on inheritance attach and detach: just coninhcount+1 or coninhcount-1 of the one relevant constraint (which can be matched by column name). If we have multiple ones, we have to know which one(s) to match and how (by constraint name?); if the parent has two and the child has one, we need to create another in the child, with its own coninhcount adjustments; if the parent has one named parent_col_not_null and the child also has child_col_not_null, then at ADD INHERIT do we match these ignoring the differing name, or do we rename the one on child so that we now have two? Also, the clutter in psql/pg_dump becomes worse. I would suggest that domain not-null constraints should also allow just one per column. Perhaps it would make sense if we change the ALTER TABLE command to be like ALTER TABLE t1 ADD IF NOT EXISTS NOT NULL c1 Then the behavior is like one would expect. For ALTER TABLE, we would reject this command if IF NOT EXISTS is not specified. (Since this is mainly for pg_dump, it doesn't really matter for usability.) For ALTER DOMAIN, we could accept both variants.
Re: Catalog domain not-null constraints
wandering around the function AlterDomainNotNull,
the following code can fix the previous undesired behavior.
seems pretty simple, am I missing something?
based on v3-0001-Add-tests-for-domain-related-information-schema-v.patch
and v3-0002-Catalog-domain-not-null-constraints.patch
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index 2f94e375..9069465a 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -2904,7 +2904,7 @@ AlterDomainAddConstraint(List *names, Node *newConstraint,
Form_pg_type typTup;
Constraint *constr;
char *ccbin;
- ObjectAddress address;
+ ObjectAddress address = InvalidObjectAddress;
/* Make a TypeName so we can use standard type lookup machinery */
typename = makeTypeNameFromNameList(names);
@@ -3003,6 +3003,12 @@ AlterDomainAddConstraint(List *names, Node
*newConstraint,
}
else if (constr->contype == CONSTR_NOTNULL)
{
+ /* Is the domain already set NOT NULL */
+ if (typTup->typnotnull)
+ {
+ table_close(typrel, RowExclusiveLock);
+ return address;
+ }
domainAddNotNullConstraint(domainoid, typTup->typnamespace,
typTup->typbasetype, typTup->typtypmod,
constr, NameStr(typTup->typname), constrAddr);
Re: Catalog domain not-null constraints
On 2024-Feb-11, Peter Eisentraut wrote: > But I see that table constraints do not work that way. A command like ALTER > TABLE t1 ADD NOT NULL c1 does nothing if the column already has a NOT NULL > constraint. I'm not sure this is correct. At least it's not documented. > We should probably make the domains feature work the same way, but I would > like to understand why it works that way first. It's an intentional design decision actually; I had it creating multiple constraints at first, but it had some ugly problems, so I made it behave this way (which was no small amount of changes). I think the first time I posted an implementation that worked this way was here https://postgr.es/m/20230315224440.cz3brr6323fcrxs6@alvherre.pgsql and then we debated it again later, starting at the bottom of https://www.postgresql.org/message-id/flat/CAEZATCUA_iPo5kqUun4myghoZtgqbY3jx62%3DGwcYKRMmxFUq_g%40mail.gmail.com#482db1d21bcf8a4c3ef4fbee609810f4 A few messages later, I quoted the SQL standard for DROP NOT NULL, which is pretty clear that if you run that command, then the column becomes possibly nullable, which means that we'd have to drop all matching constraints, or something. The main source of nastiness, when we allow multiple constraints, is constraint inheritance. If we allow just one constraint per column, then it's always easy to know what to do on inheritance attach and detach: just coninhcount+1 or coninhcount-1 of the one relevant constraint (which can be matched by column name). If we have multiple ones, we have to know which one(s) to match and how (by constraint name?); if the parent has two and the child has one, we need to create another in the child, with its own coninhcount adjustments; if the parent has one named parent_col_not_null and the child also has child_col_not_null, then at ADD INHERIT do we match these ignoring the differing name, or do we rename the one on child so that we now have two? Also, the clutter in psql/pg_dump becomes worse. I would suggest that domain not-null constraints should also allow just one per column. -- Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ "People get annoyed when you try to debug them." (Larry Wall)
Re: Catalog domain not-null constraints
Peter Eisentraut writes: > But I see that table constraints do not work that way. A command like > ALTER TABLE t1 ADD NOT NULL c1 does nothing if the column already has a > NOT NULL constraint. I'm not sure this is correct. At least it's not > documented. We should probably make the domains feature work the same > way, but I would like to understand why it works that way first. That's probably a hangover from when the underlying state was just a boolean (attnotnull). Still, I'm a little hesitant to change the behavior. I do agree that named constraints need to "stack", so that you'd have to remove each one before not-nullness stops being enforced. Less sure about unnamed properties. regards, tom lane
Re: Catalog domain not-null constraints
On 08.02.24 13:17, jian he wrote: I think I found a bug. connotnull already set to not null. every execution of `alter domain connotnull add not null value ;` would concatenate 'NOT NULL VALUE' for the "Check" column, I would have expected that. Each invocation adds a new constraint. But I see that table constraints do not work that way. A command like ALTER TABLE t1 ADD NOT NULL c1 does nothing if the column already has a NOT NULL constraint. I'm not sure this is correct. At least it's not documented. We should probably make the domains feature work the same way, but I would like to understand why it works that way first.
Re: Catalog domain not-null constraints
On Wed, Feb 7, 2024 at 4:11 PM Peter Eisentraut wrote: > > > > > Interesting. I couldn't reproduce this locally, even across different > > operating systems. The cfbot failures appear to be sporadic, but also > > happening across multiple systems, so it's clearly not just a local > > environment failure. Can anyone else perhaps reproduce this locally? > > This patch set needed a rebase, so here it is. > do you think add following ALTER DOMAIN name ADD NOT NULL VALUE to doc/src/sgml/ref/alter_domain.sgml synopsis makes sense? otherwise it would be hard to find out this command, i think. I think I found a bug. connotnull already set to not null. every execution of `alter domain connotnull add not null value ;` would concatenate 'NOT NULL VALUE' for the "Check" column, That means changes in the function pg_get_constraintdef_worker are not 100% correct. see below demo: src8=# \dD+ List of domains Schema |Name| Type | Collation | Nullable | Default | Check | Access privileges | Description ++-+---+--+-++---+- public | connotnull | integer | | | | NOT NULL VALUE | | public | nnint | integer | | not null | | NOT NULL VALUE | | (2 rows) src8=# alter domain connotnull add not null value ; ALTER DOMAIN src8=# \dD+ List of domains Schema |Name| Type | Collation | Nullable | Default | Check | Access privileges | Descript ion ++-+---+--+-+---+---+- public | connotnull | integer | | not null | | NOT NULL VALUE NOT NULL VALUE | | public | nnint | integer | | not null | | NOT NULL VALUE| | (2 rows) src8=# alter domain connotnull add not null value ; ALTER DOMAIN src8=# \dD+ List of domains Schema |Name| Type | Collation | Nullable | Default | Check | Access privil eges | Description ++-+---+--+-+--+-- -+- public | connotnull | integer | | not null | | NOT NULL VALUE NOT NULL VALUE NOT NULL VALUE | | public | nnint | integer | | not null | | NOT NULL VALUE | | (2 rows)
Re: Catalog domain not-null constraints
On 18.01.24 07:53, Peter Eisentraut wrote:
On 17.01.24 13:15, vignesh C wrote:
One of the test has failed in CFBot at [1] with:
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/domain.out
/tmp/cirrus-ci-build/src/test/regress/results/domain.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/domain.out
2024-01-14 15:40:01.793434601 +
+++ /tmp/cirrus-ci-build/src/test/regress/results/domain.out
2024-01-14 15:42:23.013332625 +
@@ -1271,11 +1271,4 @@
FROM information_schema.domain_constraints
WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
ORDER BY constraint_name;
- constraint_catalog | constraint_schema | constraint_name |
check_clause
-+---+--+---
- regression | public | con_check | (VALUE > 0)
- regression | public | meow | (VALUE <
11)
- regression | public | pos_int_check | (VALUE > 0)
- regression | public | pos_int_not_null | VALUE IS
NOT NULL
-(4 rows)
-
+ERROR: could not open relation with OID 36379
[1] - https://cirrus-ci.com/task/4536440638406656
[2] -
https://api.cirrus-ci.com/v1/artifact/task/4536440638406656/log/src/test/regress/regression.diffs
Interesting. I couldn't reproduce this locally, even across different
operating systems. The cfbot failures appear to be sporadic, but also
happening across multiple systems, so it's clearly not just a local
environment failure. Can anyone else perhaps reproduce this locally?
This patch set needed a rebase, so here it is.
About the sporadic test failure above, I think that is an existing issue
that is just now exposed through some test timing changes. The
pg_get_expr() function used in information_schema.check_constraints has
no locking against concurrent drops of tables. I think in this
particular case, the tests "domain" and "alter_table" are prone to this
conflict. If I move "domain" to a separate test group, the issue goes
away. I'll start a separate discussion about this issue.
From cd48c8eb8aa5958020e899b8406cbd7866805db5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 23 Nov 2023 07:35:32 +0100
Subject: [PATCH v3 1/2] Add tests for domain-related information schema views
Discussion:
https://www.postgresql.org/message-id/flat/9ec24d7b-633d-463a-84c6-7acff769c9e8%40eisentraut.org
---
src/test/regress/expected/domain.out | 47
src/test/regress/sql/domain.sql | 24 ++
2 files changed, 71 insertions(+)
diff --git a/src/test/regress/expected/domain.out
b/src/test/regress/expected/domain.out
index 6d94e84414a..e70aebd70c0 100644
--- a/src/test/regress/expected/domain.out
+++ b/src/test/regress/expected/domain.out
@@ -1207,3 +1207,50 @@ create domain testdomain1 as int constraint unsigned
check (value > 0);
alter domain testdomain1 rename constraint unsigned to unsigned_foo;
alter domain testdomain1 drop constraint unsigned_foo;
drop domain testdomain1;
+--
+-- Information schema
+--
+SELECT * FROM information_schema.column_domain_usage
+ WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+ ORDER BY domain_name;
+ domain_catalog | domain_schema | domain_name | table_catalog | table_schema |
table_name | column_name
++---+-+---+--++-
+ regression | public| con | regression| public |
domcontest | col1
+ regression | public| dom | regression| public |
domview| col1
+ regression | public| things | regression| public |
thethings | stuff
+(3 rows)
+
+SELECT * FROM information_schema.domain_constraints
+ WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+ ORDER BY constraint_name;
+ constraint_catalog | constraint_schema | constraint_name | domain_catalog |
domain_schema | domain_name | is_deferrable | initially_deferred
++---+-++---+-+---+
+ regression | public| con_check | regression |
public| con | NO| NO
+ regression | public| meow| regression |
public| things | NO| NO
+ regression | public| pos_int_check | regression |
public| pos_int | NO| NO
+(3 rows)
+
+SELECT * FROM information_schema.domains
+ WHERE domain_name IN ('con', 'dom', 'pos_int', 'things')
+ ORDER BY domain_name;
+ domain_catalog | domain_schema | domain_name | data_type |
character_maximum_length | character_octet_length | character_set_catalog |
character_set_schema | character_set_name | collation_catalog |
collation_schema | collation_name | numeric_precision |
Re: Catalog domain not-null constraints
On 17.01.24 13:15, vignesh C wrote:
One of the test has failed in CFBot at [1] with:
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/domain.out
/tmp/cirrus-ci-build/src/test/regress/results/domain.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/domain.out
2024-01-14 15:40:01.793434601 +
+++ /tmp/cirrus-ci-build/src/test/regress/results/domain.out
2024-01-14 15:42:23.013332625 +
@@ -1271,11 +1271,4 @@
FROM information_schema.domain_constraints
WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
ORDER BY constraint_name;
- constraint_catalog | constraint_schema | constraint_name | check_clause
-+---+--+---
- regression | public| con_check| (VALUE > 0)
- regression | public| meow | (VALUE < 11)
- regression | public| pos_int_check| (VALUE > 0)
- regression | public| pos_int_not_null | VALUE IS NOT NULL
-(4 rows)
-
+ERROR: could not open relation with OID 36379
[1] - https://cirrus-ci.com/task/4536440638406656
[2] -
https://api.cirrus-ci.com/v1/artifact/task/4536440638406656/log/src/test/regress/regression.diffs
Interesting. I couldn't reproduce this locally, even across different
operating systems. The cfbot failures appear to be sporadic, but also
happening across multiple systems, so it's clearly not just a local
environment failure. Can anyone else perhaps reproduce this locally?
Re: Catalog domain not-null constraints
On Wed, 29 Nov 2023 at 01:14, Peter Eisentraut wrote:
>
> On 23.11.23 14:13, Aleksander Alekseev wrote:
> > =# create domain connotnull1 integer;
> > =# create domain connotnull2 integer;
> > =# alter domain connotnull1 add not null value;
> > =# alter domain connotnull2 set not null;
> > =# \dD
> > ERROR: unexpected null value in cached tuple for catalog
> > pg_constraint column conkey
>
> Yeah, for domain not-null constraints pg_constraint.conkey is indeed
> null. Should we put something in there?
>
> Attached is an updated patch that avoids the error by taking a separate
> code path for domain constraints in ruleutils.c. But maybe there is
> another way to arrange this.
One of the test has failed in CFBot at [1] with:
diff -U3 /tmp/cirrus-ci-build/src/test/regress/expected/domain.out
/tmp/cirrus-ci-build/src/test/regress/results/domain.out
--- /tmp/cirrus-ci-build/src/test/regress/expected/domain.out
2024-01-14 15:40:01.793434601 +
+++ /tmp/cirrus-ci-build/src/test/regress/results/domain.out
2024-01-14 15:42:23.013332625 +
@@ -1271,11 +1271,4 @@
FROM information_schema.domain_constraints
WHERE domain_name IN ('con', 'dom', 'pos_int', 'things'))
ORDER BY constraint_name;
- constraint_catalog | constraint_schema | constraint_name | check_clause
-+---+--+---
- regression | public| con_check| (VALUE > 0)
- regression | public| meow | (VALUE < 11)
- regression | public| pos_int_check| (VALUE > 0)
- regression | public| pos_int_not_null | VALUE IS NOT NULL
-(4 rows)
-
+ERROR: could not open relation with OID 36379
[1] - https://cirrus-ci.com/task/4536440638406656
[2] -
https://api.cirrus-ci.com/v1/artifact/task/4536440638406656/log/src/test/regress/regression.diffs
Regards,
Vignesh
Re: Catalog domain not-null constraints
On 23.11.23 14:13, Aleksander Alekseev wrote:
=# create domain connotnull1 integer;
=# create domain connotnull2 integer;
=# alter domain connotnull1 add not null value;
=# alter domain connotnull2 set not null;
=# \dD
ERROR: unexpected null value in cached tuple for catalog
pg_constraint column conkey
Yeah, for domain not-null constraints pg_constraint.conkey is indeed
null. Should we put something in there?
Attached is an updated patch that avoids the error by taking a separate
code path for domain constraints in ruleutils.c. But maybe there is
another way to arrange this.From c297c458766a0e1ee65408d3ced469f32cf5e7d8 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 28 Nov 2023 20:38:16 +0100
Subject: [PATCH v2 2/2] Catalog domain not-null constraints
This applies the explicit catalog representation of not-null
constraints introduced by b0e96f3119 for table constraints also to
domain not-null constraints.
TODO: catversion
Discussion:
https://www.postgresql.org/message-id/flat/9ec24d7b-633d-463a-84c6-7acff769c9e8%40eisentraut.org
---
src/backend/catalog/information_schema.sql | 2 +-
src/backend/catalog/pg_constraint.c| 40 +++
src/backend/commands/typecmds.c| 313 +++--
src/backend/utils/adt/ruleutils.c | 8 +
src/backend/utils/cache/typcache.c | 2 +-
src/bin/pg_dump/pg_dump.c | 2 +-
src/include/catalog/pg_constraint.h| 1 +
src/test/regress/expected/domain.out | 49 +++-
src/test/regress/sql/domain.sql| 26 ++
9 files changed, 339 insertions(+), 104 deletions(-)
diff --git a/src/backend/catalog/information_schema.sql
b/src/backend/catalog/information_schema.sql
index 10b34c3c5b..95a1b34560 100644
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -448,7 +448,7 @@ CREATE VIEW check_constraints AS
SELECT current_database()::information_schema.sql_identifier AS
constraint_catalog,
rs.nspname::information_schema.sql_identifier AS constraint_schema,
con.conname::information_schema.sql_identifier AS constraint_name,
- pg_catalog.format('%s IS NOT NULL',
at.attname)::information_schema.character_data AS check_clause
+ pg_catalog.format('%s IS NOT NULL', coalesce(at.attname,
'VALUE'))::information_schema.character_data AS check_clause
FROM pg_constraint con
LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace
LEFT JOIN pg_class c ON c.oid = con.conrelid
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index e9d4d6006e..4f1a5e1c84 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -629,6 +629,46 @@ findNotNullConstraint(Oid relid, const char *colname)
return findNotNullConstraintAttnum(relid, attnum);
}
+HeapTuple
+findDomainNotNullConstraint(Oid typid)
+{
+ Relationpg_constraint;
+ HeapTuple conTup,
+ retval = NULL;
+ SysScanDesc scan;
+ ScanKeyData key;
+
+ pg_constraint = table_open(ConstraintRelationId, AccessShareLock);
+ ScanKeyInit(,
+ Anum_pg_constraint_contypid,
+ BTEqualStrategyNumber, F_OIDEQ,
+ ObjectIdGetDatum(typid));
+ scan = systable_beginscan(pg_constraint,
ConstraintRelidTypidNameIndexId,
+ true, NULL, 1, );
+
+ while (HeapTupleIsValid(conTup = systable_getnext(scan)))
+ {
+ Form_pg_constraint con = (Form_pg_constraint) GETSTRUCT(conTup);
+
+ /*
+* We're looking for a NOTNULL constraint that's marked
validated.
+*/
+ if (con->contype != CONSTRAINT_NOTNULL)
+ continue;
+ if (!con->convalidated)
+ continue;
+
+ /* Found it */
+ retval = heap_copytuple(conTup);
+ break;
+ }
+
+ systable_endscan(scan);
+ table_close(pg_constraint, AccessShareLock);
+
+ return retval;
+}
+
/*
* Given a pg_constraint tuple for a not-null constraint, return the column
* number it is for.
diff --git a/src/backend/commands/typecmds.c b/src/backend/commands/typecmds.c
index aaf9728697..57389be9ed 100644
--- a/src/backend/commands/typecmds.c
+++ b/src/backend/commands/typecmds.c
@@ -126,15 +126,19 @@ static OidfindTypeSubscriptingFunction(List
*procname, Oid typeOid);
static Oid findRangeSubOpclass(List *opcname, Oid subtype);
static Oid findRangeCanonicalFunction(List *procname, Oid typeOid);
static Oid findRangeSubtypeDiffFunction(List *procname, Oid subtype);
-static void validateDomainConstraint(Oid domainoid, char *ccbin);
+static void validateDomainCheckConstraint(Oid domainoid, char
Re: Catalog domain not-null constraints
On 23.11.23 17:38, Alvaro Herrera wrote: If you create a table with column of domain that has a NOT NULL constraint, what happens? I mean, is the table column marked attnotnull, and how does it behave? No, the domain does not affect the catalog entry for the column. This is the same way it behaves now. Is there a separate pg_constraint row for the constraint in the table? What happens if you do ALTER TABLE ... DROP NOT NULL for that column? Those are separate. After dropping the NOT NULL for a column, null values for the column could still be rejected by a domain. (This is the same way CHECK constraints work.)
Re: Catalog domain not-null constraints
On 2023-Nov-23, Peter Eisentraut wrote: > This patch set applies the explicit catalog representation of not-null > constraints introduced by b0e96f3119 for table constraints also to domain > not-null constraints. I like the idea of having domain not-null constraints appear in pg_constraint. > Since there is no inheritance or primary keys etc., this is much simpler and > just applies the existing infrastructure to domains as well. If you create a table with column of domain that has a NOT NULL constraint, what happens? I mean, is the table column marked attnotnull, and how does it behave? Is there a separate pg_constraint row for the constraint in the table? What happens if you do ALTER TABLE ... DROP NOT NULL for that column? -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "Por suerte hoy explotó el califont porque si no me habría muerto de aburrido" (Papelucho)
Re: Catalog domain not-null constraints
On 2023-Nov-23, Aleksander Alekseev wrote: > Interestingly enough according to the documentation this syntax is > already supported [1][2], but the actual query will fail on `master`: > > ``` > =# create domain connotnull integer; > CREATE DOMAIN > =# alter domain connotnull add not null value; > ERROR: unrecognized constraint subtype: 1 > ``` Hah, nice ... this only fails in this way on master, though, as a side-effect of previous NOT NULL work during this cycle. So if we take Peter's patch, we don't need to worry about it. In 16 it behaves properly, with a normal syntax error. > ``` > =# create domain connotnull1 integer; > =# create domain connotnull2 integer; > =# alter domain connotnull1 add not null value; > =# alter domain connotnull2 set not null; > =# \dD > ERROR: unexpected null value in cached tuple for catalog > pg_constraint column conkey > ``` This is also a master-only problem, as "add not null" is rejected in 16 with a syntax error (and obviously \dD doesn't fail). > NOT VALID is not supported: > > ``` > =# alter domain connotnull add not null value not valid; > ERROR: NOT NULL constraints cannot be marked NOT VALID > ``` Yeah, it'll take more work to let NOT NULL constraints be marked NOT VALID, both on domains and on tables. It'll be a good feature for sure. -- Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/ "La victoria es para quien se atreve a estar solo"
Re: Catalog domain not-null constraints
Hi, > This patch set applies the explicit catalog representation of not-null > constraints introduced by b0e96f3119 for table constraints also to > domain not-null constraints. Interestingly enough according to the documentation this syntax is already supported [1][2], but the actual query will fail on `master`: ``` =# create domain connotnull integer; CREATE DOMAIN =# alter domain connotnull add not null value; ERROR: unrecognized constraint subtype: 1 ``` I wonder if we should reflect this limitation in the documentation and/or show better error messages. This could be quite surprising to the user. However if we change the documentation on the `master` branch this patch will have to change it back. I was curious about the semantic difference between `SET NOT NULL` and `ADD NOT NULL value`. When I wanted to figure this out I discovered something that seems to be a bug: ``` =# create domain connotnull1 integer; =# create domain connotnull2 integer; =# alter domain connotnull1 add not null value; =# alter domain connotnull2 set not null; =# \dD ERROR: unexpected null value in cached tuple for catalog pg_constraint column conkey ``` Also it turned out that I can do both: `SET NOT NULL` and `ADD NOT NULL value` for the same domain. Is it an intended behavior? We should either forbid it or cover this case with a test. NOT VALID is not supported: ``` =# alter domain connotnull add not null value not valid; ERROR: NOT NULL constraints cannot be marked NOT VALID ``` ... and this is correct: "NOT VALID is only accepted for CHECK constraints" [1]. This code path however doesn't seem to be test-covered even on `master`. While on it, I suggest fixing this. [1]: https://www.postgresql.org/docs/current/sql-alterdomain.html [2]: https://www.postgresql.org/docs/current/sql-createdomain.html -- Best regards, Aleksander Alekseev
