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]https://www.postgresql.org/docs/current/sql-alterdomain.html[2]https://www.postgresql.org/docs/current/sql-createdomain.html, 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]https://www.postgresql.org/docs/current/sql-alterdomain.html. 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

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"

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)

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

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.

On Wed, 29 Nov 2023 at 01:14, Peter Eisentraut <peter@eisentraut.org> 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 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/domain.out
2024-01-14 15:42:23.013332625 +0000
@@ -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

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 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/domain.out
2024-01-14 15:42:23.013332625 +0000
@@ -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?

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 +0000
+++ /tmp/cirrus-ci-build/src/test/regress/results/domain.out
2024-01-14 15:42:23.013332625 +0000
@@ -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.

On Wed, Feb 7, 2024 at 4:11 PM Peter Eisentraut <peter@eisentraut.org> 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 <replaceable class="parameter">name</replaceable> 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)

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.

Peter Eisentraut <peter@eisentraut.org> 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

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
/messages/by-id/20230315224440.cz3brr6323fcrxs6@alvherre.pgsql

and then we debated it again later, starting at the bottom of
/messages/by-id/CAEZATCUA_iPo5kqUun4myghoZtgqbY3jx62=GwcYKRMmxFUq_g@mail.gmail.com
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 Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"People get annoyed when you try to debug them." (Larry Wall)

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

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.

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"

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 <constraint specification>

should be to add a constraint.

In the current implementation, the behavior is different for different
constraint types.

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.

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

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 */