Re: cataloguing NOT NULL constraints

On 2023-Mar-03, Peter Eisentraut wrote:

> On 28.02.23 20:15, Alvaro Herrera wrote:
> > So I reworked this to use a new contype value for the NOT NULL
> > pg_constraint rows; I attach it here. I think it's fairly clean.
> >
> > 0001 is just a trivial change that seemed obvious as soon as I ran into
> > the problem.
>
> This looks harmless enough, but I wonder what the reason for it is. What
> command can cause this error (no test case?)? Is there ever a confusion
> about what table is in play?

Hmm, I realize now that the only reason I have this is that I had a bug
at some point: the case where it's not evident which table it is, is
when you're adding a PK to a partitioned table and one of the partitions
doesn't have the NOT NULL marking. But if you add a PK with the patch,
the partitions are supposed to get the nullability marking
automatically; the bug is that they didn't. So we don't need patch 0001
at all.

> > 0002 is the most interesting part.

Another thing I realized after posting, is that the constraint naming
business is mistaken. It's currently written to work similarly to CHECK
constraints, that is: each descendent needs to have the constraint named
the same (this is so that descent works correctly when altering/dropping
the constraint afterwards). But for NOT NULL constraints, that is not
necessary, because when descending down the hierarchy, we can just match
the constraint based on column name, since each column has at most one
NOT NULL constraint. So the games with constraint renaming are
altogether unnecessary and can be removed from the patch. We just need
to ensure that coninhcount/conislocal is updated appropriately.

> Where did this syntax come from:
>
> --- a/doc/src/sgml/ref/create_table.sgml
> +++ b/doc/src/sgml/ref/create_table.sgml
> @@ -77,6 +77,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } |
> UNLOGGED ] TABLE [ IF NOT EXI
>
> [ CONSTRAINT <replaceable class="parameter">constraint_name</replaceable> ]
> { CHECK ( <replaceable class="parameter">expression</replaceable> ) [ NO
> INHERIT ] |
> + NOT NULL <replaceable class="parameter">column_name</replaceable> |
> UNIQUE [ NULLS [ NOT ] DISTINCT ] ( <replaceable
> class="parameter">column_name</replaceable> [, ... ] ) <replaceable
> class="parameter">in>
> PRIMARY KEY ( <replaceable class="parameter">column_name</replaceable> [,
> ... ] ) <replaceable class="parameter">index_parameters</replac>
> EXCLUDE [ USING <replaceable class="parameter">index_method</replaceable>
> ] ( <replaceable class="parameter">exclude_element</replaceable>
>
> I don't see that in the standard.

Yeah, I made it up because I needed table-level constraints for some
reason that doesn't come to mind right now.

> If we need it for something, we should at least document that it's an
> extension.

OK.

> The test tables in foreign_key.sql are declared with columns like
>
> id bigint NOT NULL PRIMARY KEY,
>
> which is a bit weird and causes expected output diffs in your patch. Is
> that interesting for this patch? Otherwise I suggest dropping the NOT NULL
> from those table definitions to avoid these extra diffs.

The behavior is completely different if you drop the primary key. If
you don't have NOT NULL, then when you drop the PK the columns becomes
nullable. If you do have a NOT NULL constraint in addition to the PK,
and drop the PK, then the column remains non nullable.

Now, if you want to suggest that dropping the PK ought to leave the
column as NOT NULL (that is, it automatically acquires a NOT NULL
constraint), then let's discuss that. But I understand the standard as
saying otherwise.

> > 0003:
> > Since nobody liked the idea of listing the constraints in psql \d's
> > footer, I changed \d+ so that the "not null" column shows the name of
> > the constraint if there is one, or the string "(primary key)" if the
> > attnotnull marking for the column comes from the primary key. The new
> > column is going to be quite wide in some cases; if we want to hide it
> > further, we could add the mythical \d++ and have *that* list the
> > constraint name, keeping \d+ as current.
>
> I think my rough preference here would be to leave the existing output style
> (column header "Nullable", content "not null") alone and display the
> constraint name somewhere in the footer optionally.

Well, there is resistance to showing the name of the constraint in the
footer also because it's too verbose. In the end, I think a
"super-verbose" mode is the most convincing way forward. (I think the
list of partitions in the footer of a partitioned table is a terrible
design. Let's not repeat that.)

> In practice, the name of the constraint is rarely needed.

That is true.

> I do like the idea of mentioning primary key-ness inside the table somehow.

Maybe change the "not null" to "primary key" in the Nullable column and
nothing else.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Cómo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar
al alfarero y ver qué formas se pueden sacar del otro" (C. Halloway en
La Feria de las Tinieblas, R. Bradbury)