Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix ALTER DOMAIN NOT NULL syntax

  2. Catalog domain not-null constraints

  3. Add tests for domain-related information schema views

  1. Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2023-11-23T06:56:48Z

    This patch set applies the explicit catalog representation of not-null 
constraints introduced by b0e96f3119 for table constraints also to 
domain not-null constraints.

Since there is no inheritance or primary keys etc., this is much simpler 
and just applies the existing infrastructure to domains as well.  As a 
result, domain not-null constraints now appear in the information schema 
correctly.  Another effect is that you can now use the ALTER DOMAIN ... 
ADD/DROP CONSTRAINT syntax for not-null constraints as well.  This makes 
everything consistent overall.

For the most part, I structured the code so that there are now separate 
sibling subroutines for domain check constraints and domain not-null 
constraints.  This seemed to work out best, but one could also consider 
other ways to refactor this.

  2. Re: Catalog domain not-null constraints

    Aleksander Alekseev <aleksander@timescale.com> — 2023-11-23T13:13:30Z

    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

  3. Re: Catalog domain not-null constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-11-23T16:35:25Z

    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"

  4. Re: Catalog domain not-null constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2023-11-23T16:38:30Z

    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)

  5. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2023-11-27T07:08:08Z

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

  6. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2023-11-28T19:43:54Z

    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.

  7. Re: Catalog domain not-null constraints

    vignesh C <vignesh21@gmail.com> — 2024-01-17T12:15:29Z

    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

  8. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-01-18T06:53:57Z

    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?

  9. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-02-07T08:10:52Z

    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.

  10. Re: Catalog domain not-null constraints

    jian he <jian.universality@gmail.com> — 2024-02-08T12:17:09Z

    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)

  11. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-02-11T21:10:10Z

    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.

  12. Re: Catalog domain not-null constraints

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-02-11T21:42:44Z

    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

  13. Re: Catalog domain not-null constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-02-12T10:24:13Z

    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 Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"People get annoyed when you try to debug them."  (Larry Wall)

  14. Re: Catalog domain not-null constraints

    jian he <jian.universality@gmail.com> — 2024-02-21T08:01:16Z

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

  15. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-14T12:55:11Z

    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.

  16. Re: Catalog domain not-null constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-03-14T14:03:07Z

    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"

  17. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-14T14:35:52Z

    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.

  18. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-18T07:46:28Z

    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.

  19. Re: Catalog domain not-null constraints

    Aleksander Alekseev <aleksander@timescale.com> — 2024-03-18T10:02:17Z

    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

  20. Re: Catalog domain not-null constraints

    jian he <jian.universality@gmail.com> — 2024-03-19T09:57:57Z

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

  21. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-20T09:41:33Z

    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.

  22. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-20T09:43:47Z

    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.

  23. Re: Catalog domain not-null constraints

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2024-03-20T11:22:58Z

    On Wed, 20 Mar 2024 at 09:43, Peter Eisentraut <peter@eisentraut.org> 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

  24. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-21T11:23:34Z

    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.

<canofworms>

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?

</canofworms>

  25. Re: Catalog domain not-null constraints

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-21T14:30:10Z

    Peter Eisentraut <peter@eisentraut.org> writes:
> <canofworms>
> 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?
> </canofworms>

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

  26. Re: Catalog domain not-null constraints

    Isaac Morland <isaac.morland@gmail.com> — 2024-03-21T17:36:25Z

    On Thu, 21 Mar 2024 at 10:30, Tom Lane <tgl@sss.pgh.pa.us> 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.

  27. Re: Catalog domain not-null constraints

    Vik Fearing <vik@postgresfriends.org> — 2024-03-21T22:55:13Z

    On 3/21/24 15:30, Tom Lane wrote:
> Peter Eisentraut <peter@eisentraut.org> writes:
>> <canofworms>
>> 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?
>> </canofworms>
> 
> 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 <cast specification> 
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

  28. Re: Catalog domain not-null constraints

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-21T23:17:28Z

    Vik Fearing <vik@postgresfriends.org> 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 <cast specification>,
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 <cast specification> 
> 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

  29. Re: Catalog domain not-null constraints

    Vik Fearing <vik@postgresfriends.org> — 2024-03-21T23:38:09Z

    On 3/22/24 00:17, Tom Lane wrote:
> Vik Fearing <vik@postgresfriends.org> 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 <cast specification>,
> 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 <cast specification>.

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

  30. Re: Catalog domain not-null constraints

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-03-22T00:46:52Z

    Vik Fearing <vik@postgresfriends.org> writes:
> On 3/22/24 00:17, Tom Lane wrote:
>> Vik Fearing <vik@postgresfriends.org> writes:
>>> As also said somewhere in that thread, I think that <cast specification>
>>> 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

  31. Re: Catalog domain not-null constraints

    Vik Fearing <vik@postgresfriends.org> — 2024-03-22T01:12:48Z

    On 3/22/24 01:46, Tom Lane wrote:
> Vik Fearing <vik@postgresfriends.org> 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

  32. Re: Catalog domain not-null constraints

    jian he <jian.universality@gmail.com> — 2024-03-22T08:28:39Z

    On Thu, Mar 21, 2024 at 7:23 PM Peter Eisentraut <peter@eisentraut.org> 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,
+   &n->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(&buf,
"NOT NULL VALUE");
+                                       appendStringInfoString(&buf,
"CHECK (VALUE IS NOT NULL)");
seems works.

  33. Re: Catalog domain not-null constraints

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2024-03-25T18:28:28Z

    On Fri, 22 Mar 2024 at 08:28, jian he <jian.universality@gmail.com> wrote:
>
> On Thu, Mar 21, 2024 at 7:23 PM Peter Eisentraut <peter@eisentraut.org> 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 <literal>NOT NULL</literal> in this command is a
+   <productname>PostgreSQL</productname> extension.  (A standard-conforming
+   way to write the same would be <literal>CHECK (VALUE IS NOT
+   NULL)</literal>.  However, per <xref linkend="sql-createdomain-notes"/>,
+   such constraints a best avoided in practice anyway.)  The
+   <literal>NULL</literal> <quote>constraint</quote> is a
+   <productname>PostgreSQL</productname> extension (see also <xref
+   linkend="sql-createtable-compatibility"/>).

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, &n->skip_validation,
+                                  &n->is_no_inherit, yyscanner);
+                   n->initially_valid = !n->skip_validation;

+                   /* no NOT VALID support yet */
+                   processCASbits($3, @3, "NOT NULL",
+                                  NULL, NULL, NULL,
+                                  &n->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

  34. Re: Catalog domain not-null constraints

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-03-26T07:30:45Z

    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 Herrera        Breisgau, 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)

  35. Re: Catalog domain not-null constraints

    Dean Rasheed <dean.a.rasheed@gmail.com> — 2024-03-26T09:04:20Z

    On Tue, 26 Mar 2024 at 07:30, Alvaro Herrera <alvherre@alvh.no-ip.org> 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

  36. Re: Catalog domain not-null constraints

    jian he <jian.universality@gmail.com> — 2024-04-01T00:00:00Z

    On Tue, Mar 26, 2024 at 2:28 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
>
> On Fri, 22 Mar 2024 at 08:28, jian he <jian.universality@gmail.com> wrote:
> >
> > On Thu, Mar 21, 2024 at 7:23 PM Peter Eisentraut <peter@eisentraut.org> 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.

  37. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-04-08T09:53:40Z

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

  38. Re: Catalog domain not-null constraints

    jian he <jian.universality@gmail.com> — 2024-04-09T08:44:18Z

    On Mon, Apr 8, 2024 at 5:53 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> 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.)


in ruleutils.c
/* conkey is null for domain not-null constraints */
appendStringInfoString(&buf, "NOT NULL VALUE");

should be

/* conkey is null for domain not-null constraints */
appendStringInfoString(&buf, "NOT NULL ");
?


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.



the url link destination is fine, but the url rendered name is "per
the section called “Notes”" which seems strange,
please see attached.

  39. Re: Catalog domain not-null constraints

    Peter Eisentraut <peter@eisentraut.org> — 2024-04-15T07:43:08Z

    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(&buf, "NOT NULL VALUE");
> 
> should be
> 
> /* conkey is null for domain not-null constraints */
> appendStringInfoString(&buf, "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.