incorrect (incomplete) description for "alter domain"

On 2024-07-29 13:02 +0200, PG Doc comments form wrote:

In the Synopsis section of
https://www.postgresql.org/docs/current/sql-alterdomain.html
this is incorrect (incomplete):
"ALTER DOMAIN name ADD domain_constraint [ NOT VALID ]"
It should be
"ALTER DOMAIN name ADD CONSTRAINT domain_constraint [ NOT VALID ]"

No, the docs are correct. domain_constraint refers to this syntax
defined by CREATE DOMAIN:

"where constraint is:

[ CONSTRAINT constraint_name ]
{ NOT NULL | NULL | CHECK (expression) }"

Also, PG 17 renames this to "domain_constraint" with commit 9895b35cb8
to align ALTER DOMAIN with CREATE DOMAIN.

--
Erik

On Monday, July 29, 2024, PG Doc comments form <noreply@postgresql.org>
wrote:

The following documentation comment has been logged on the website:

Page: https://www.postgresql.org/docs/16/sql-alterdomain.html
Description:

In the Synopsis section of
https://www.postgresql.org/docs/current/sql-alterdomain.html
this is incorrect (incomplete):
"ALTER DOMAIN name ADD domain_constraint [ NOT VALID ]"
It should be
"ALTER DOMAIN name ADD CONSTRAINT domain_constraint [ NOT VALID ]"

The definition of “domain_constraint” includes the optional “constraint
constraint_name” clause. Though reading the page and seeing the number of
times we say “alter domain add constraint” I even more inclined to agree
that bringing the word constraint there is desirable. I am not a huge fan
of the indirect syntax references anyway. But I think your proposed fix is
technically wrong since the word constraint is optional but your change
makes it mandatory.

David J.

"David G. Johnston" <david.g.johnston@gmail.com> writes:

On Monday, July 29, 2024, PG Doc comments form <noreply@postgresql.org>
wrote:

In the Synopsis section of
https://www.postgresql.org/docs/current/sql-alterdomain.html
this is incorrect (incomplete):
"ALTER DOMAIN name ADD domain_constraint [ NOT VALID ]"
It should be
"ALTER DOMAIN name ADD CONSTRAINT domain_constraint [ NOT VALID ]"

The definition of “domain_constraint” includes the optional “constraint
constraint_name” clause. Though reading the page and seeing the number of
times we say “alter domain add constraint” I even more inclined to agree
that bringing the word constraint there is desirable. I am not a huge fan
of the indirect syntax references anyway.

I think the page is technically correct, but I'm inclined to duplicate
this text from the CREATE DOMAIN page:

where domain_constraint is:

[ CONSTRAINT constraint_name ]
{ NOT NULL | NULL | CHECK (expression) }

rather than making readers go look that up. Is that the same thing
you're thinking, or did you have a different idea?

regards, tom lane

I wrote:

I think the page is technically correct, but I'm inclined to duplicate
this text from the CREATE DOMAIN page:

where domain_constraint is:
[ CONSTRAINT constraint_name ]
{ NOT NULL | NULL | CHECK (expression) }

rather than making readers go look that up.

Actually, there *is* a bug in the description, because experimentation
shows that CREATE DOMAIN accepts NULL in this syntax (as advertised)
but ALTER DOMAIN does not. We could alternatively decide that that's
a code bug and make ALTER DOMAIN take it, but I don't think it's worth
any effort (and this behavior may actually have been intentional, too).
I think we should just add

where domain_constraint is:

[ CONSTRAINT constraint_name ]
{ NOT NULL | CHECK (expression) }

to the ALTER DOMAIN page, and then remove the claim that it's
identical to CREATE DOMAIN.

regards, tom lane

On Mon, Jul 29, 2024 at 8:17 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I wrote:

I think the page is technically correct, but I'm inclined to duplicate
this text from the CREATE DOMAIN page:

where domain_constraint is:
[ CONSTRAINT constraint_name ]
{ NOT NULL | NULL | CHECK (expression) }

rather than making readers go look that up.

Agreed

Actually, there *is* a bug in the description, because experimentation
shows that CREATE DOMAIN accepts NULL in this syntax (as advertised)
but ALTER DOMAIN does not. We could alternatively decide that that's
a code bug and make ALTER DOMAIN take it, but I don't think it's worth
any effort (and this behavior may actually have been intentional, too).
I think we should just add

where domain_constraint is:

[ CONSTRAINT constraint_name ]
{ NOT NULL | CHECK (expression) }

to the ALTER DOMAIN page, and then remove the claim that it's
identical to CREATE DOMAIN.

The inconsistency here and with create/alter table makes me want to make
alter domain work as well. But I agree it isn't really worth the effort
when one is supposed to use "drop not null" to accomplish the effect of
making a domain nullable. But it does open the question of why we document
"alter table alter column add null" - which does not get mentioned as being
a non-standard option on the alter table page.

Both create table and create domain say:

NULL: This clause is only intended for compatibility with nonstandard SQL
databases. Its use is discouraged in new applications.

But then create table goes on to say under Compatibility:

The NULL “constraint” (actually a non-constraint) is a PostgreSQL extension
to the SQL standard ...

But create domain has no matching language; it claims full conformity.

That this "non-constraint" can have a name given seems unusual though done
for ease of syntax I presume.

David J.

On 29.07.24 17:17, Tom Lane wrote:

I wrote:

I think the page is technically correct, but I'm inclined to duplicate
this text from the CREATE DOMAIN page:

where domain_constraint is:
[ CONSTRAINT constraint_name ]
{ NOT NULL | NULL | CHECK (expression) }

rather than making readers go look that up.

Actually, there *is* a bug in the description, because experimentation
shows that CREATE DOMAIN accepts NULL in this syntax (as advertised)
but ALTER DOMAIN does not. We could alternatively decide that that's
a code bug and make ALTER DOMAIN take it, but I don't think it's worth
any effort (and this behavior may actually have been intentional, too).
I think we should just add

where domain_constraint is:

[ CONSTRAINT constraint_name ]
{ NOT NULL | CHECK (expression) }

to the ALTER DOMAIN page, and then remove the claim that it's
identical to CREATE DOMAIN.

There was some discussion about these issues (ALTER DOMAIN vs CREATE
DOMAIN reference page, as well as the NOT NULL constraint syntax) in and
around
</messages/by-id/a4a344ea-9e79-4c42-a9af-899f85bd753b@eisentraut.org&gt;.
All that ended up dying because the NOT NULL constraints feature was
reverted. But there were some subtle details about why the syntax is
the way it is and/or whether that's really intentional and how to
document it. Might be worth reviewing again.

On Mon, Jul 29, 2024 at 11:17:41AM -0400, Tom Lane wrote:

I wrote:

I think the page is technically correct, but I'm inclined to duplicate
this text from the CREATE DOMAIN page:

where domain_constraint is:
[ CONSTRAINT constraint_name ]
{ NOT NULL | NULL | CHECK (expression) }

rather than making readers go look that up.

Actually, there *is* a bug in the description, because experimentation
shows that CREATE DOMAIN accepts NULL in this syntax (as advertised)
but ALTER DOMAIN does not. We could alternatively decide that that's
a code bug and make ALTER DOMAIN take it, but I don't think it's worth
any effort (and this behavior may actually have been intentional, too).
I think we should just add

where domain_constraint is:

[ CONSTRAINT constraint_name ]
{ NOT NULL | CHECK (expression) }

to the ALTER DOMAIN page, and then remove the claim that it's
identical to CREATE DOMAIN.

I have written the attached patch to document this. I assume this
should be backpatched to PG 12.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

When a patient asks the doctor, "Am I going to die?", he means
"Am I going to die soon?"

On Wed, Oct 16, 2024 at 05:11:54PM -0400, Bruce Momjian wrote:

Actually, there *is* a bug in the description, because experimentation
shows that CREATE DOMAIN accepts NULL in this syntax (as advertised)
but ALTER DOMAIN does not. We could alternatively decide that that's
a code bug and make ALTER DOMAIN take it, but I don't think it's worth
any effort (and this behavior may actually have been intentional, too).
I think we should just add

where domain_constraint is:

[ CONSTRAINT constraint_name ]
{ NOT NULL | CHECK (expression) }

to the ALTER DOMAIN page, and then remove the claim that it's
identical to CREATE DOMAIN.

I have written the attached patch to document this. I assume this
should be backpatched to PG 12.

Patch applied back to PG 12.

--
Bruce Momjian <bruce@momjian.us> https://momjian.us
EDB https://enterprisedb.com

When a patient asks the doctor, "Am I going to die?", he means
"Am I going to die soon?"