Re: cataloguing NOT NULL constraints

On 2023-Jul-13, Dean Rasheed wrote:

> Hmm, looking at that change, it looks a little ugly. I think someone
> reading that code in the future will have no idea why it's including
> some invalid indexes, and not others.

True. I've added a longish comment in 0001 to explain why we do this.
0002 has two bugfixes, described below.

> On Wed, 12 Jul 2023 at 18:11, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:

> > There are no other changes from v12. One thing I should probably get
> > to, is fixing the constraint name comparison code in pg_dump. Right now
> > it's a bit dumb and will get in silly trouble with overlength
> > table/column names (nothing that would actually break, just that it will
> > emit constraint names when there's no need to.)
>
> Yeah, that seems a bit ugly. Presumably, also, if something like a
> column rename happens, the constraint name will no longer match.

Well, we never rename constraints (except AFAIR for unique constraints
when the unique index is renamed), and I'm not sure that it's a good
idea to automatically rename a not null constraint when the column or
the table are renamed.

(I think trying to make pg_dump be smarter about the constraint name
when the table/column names are very long, would require exporting
makeObjectName() for frontend use. It looks an easy task, but I haven't
done it.)

(Maybe it would be reasonable to rename the NOT NULL constraint when the
table or column are renamed, iff the original constraint name is the
default one. Initially I lean towards not doing it, though.)

Anyway, what does happen when the name doesn't match what pg_dump thinks
is the default name (<table>_<column>_not_null) is that the constraint
name is printed in the output. So if you have this table

create table one (a int not null, b int not null);
and rename column b to c, then pg_dump will print the table like this:

CREATE TABLE public.one (
a integer NOT NULL,
c integer CONSTRAINT one_b_not_null NOT NULL
);

In other words, the name is preserved across a dump. I think this is
not terrible.

> I see that it's already been discussed, but I don't like the fact that
> there is no way to get hold of the new constraint names in psql. I
> think for the purposes of dropping named constraints, and also
> possible future stuff like NOT VALID / DEFERRABLE constraints, having
> some way to get their names will be important.

Yeah, so there are two proposals:

1. Have \d+ replace the "not null" literal in the \d+ display with the
constraint name; if the column is not nullable because of the primary
key, it says "(primary key)" instead. There's a patch for this in the
thread somewhere.

2. Same, but use \d++ for this purpose

Using ++ would be a novelty in psql, so I'm hesitant to make that an
integral part of the current proposal. However, to me (2) seems to most
comfortable way forward, because while you're right that people do need
the constraint name from time to time, this is very seldom the case, so
polluting \d+ might inconvenience people for not much gain. And people
didn't like having the constraint name in \d+.

Do you have an opinion on these ideas?

> Something else I noticed is that the result from "ALTER TABLE ...
> ALTER COLUMN ... DROP NOT NULL" is no longer easily predictable -- if
> there are multiple NOT NULL constraints on the column, it just drops
> one (chosen at random?) and leaves the others. I think that it should
> either drop all the constraints, or throw an error. Either way, I
> would expect that if DROP NOT NULL succeeds, the result is that the
> column is nullable.

Hmm, there shouldn't be multiple NOT NULL constraints for the same
column; if there's one, a further SET NOT NULL should do nothing. At
some point the code was creating two constraints, but I realized that
trying to support multiple constraints caused other problems, and it
seems to serve no purpose, so I removed it. Maybe there are ways to end
up with multiple constraints, but at this stage I would say that those
are bugs to be fixed, rather than something we want to keep.

... oh, I did find a bug here -- indeed,

ALTER TABLE tab ADD CONSTRAINT NOT NULL col;

was not checking whether a constraint already existed, and created a
duplicate. In v14-0002 I made that throw an error instead. And having
done that, I discovered another bug: in test_ddl_deparse we CREATE TABLE
LIKE from SERIAL PRIMARY KEY column, so that was creating two NOT NULL
constraints, one for the lack of INCLUDING INDEXES on the PK, and
another for the NOT NULL itself which comes implicit with SERIAL. So I
fixed that too, by having transformTableLikeClause skip creating a NOT
NULL for PK columns if we're going to create one for a NOT NULL
constraint.

--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"El número de instalaciones de UNIX se ha elevado a 10,
y se espera que este número aumente" (UPM, 1972)