Обсуждение: cataloguing NOT NULL constraints

I've been working on having NOT NULL constraints have pg_constraint
rows.

Everything is working now.  Some things are a bit weird, and I would
like opinions on them:

1. In my implementation, you can have more than one NOT NULL
   pg_constraint row for a column.  What should happen if the user does
   ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL;
   ?  Currently it throws an error about the ambiguity (ie. which
   constraint to drop).
   Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull'
   bit is lost when the last one such constraint goes away.

2. If a table has a primary key, and a table is created that inherits
   from it, then the child has its column(s) marked attnotnull but there
   is no pg_constraint row for that.  This is not okay.  But what should
   happen?

   1. a CHECK(col IS NOT NULL) constraint is created for each column
   2. a PRIMARY KEY () constraint is created

Note that I've chosen not to create CHECK(foo IS NOT NULL) pg_constraint
rows for columns in the primary key, unless an explicit NOT NULL
declaration is also given.  Adding them would be a very easily solution
to problem 2 above, but ISTM that such constraints would be redundant
and not very nice.

After gathering input on these thing, I'll finish the patch and post it.
As far as I can tell, everything else is working (except the annoying
pg_dump tests, see below).

Thanks

Implementation notes:

In the current implementation I am using CHECK constraints, so these
constraints are contype='c', conkey={col} and the corresponding
expression.

pg_attribute.attnotnull is still there, and it is set true when at least
one "CHECK (col IS NOT NULL)" constraint (and it's been validated) or
PRIMARY KEY constraint exists for the column.

CHECK constraint names are no longer "tab_col_check" when the expression
is CHECK (foo IS NOT NULL).  The constraint is now going to be named
"tab_col_not_null"

If you say CREATE TABLE (a int NOT NULL), you'll get a CHECK constraint
printed by psql: (this is a bit more noisy that previously and it
changes a lot of regression tests output).

55489 16devel 1776237=# create table tab (a int not null);
CREATE TABLE
55489 16devel 1776237=# \d tab
                    Tabla «public.tab»
 Columna │  Tipo   │ Ordenamiento │ Nulable  │ Por omisión 
─────────┼─────────┼──────────────┼──────────┼─────────────
 a       │ integer │              │ not null │ 
Restricciones CHECK:
    "tab_a_not_null" CHECK (a IS NOT NULL)


pg_dump no longer prints NOT NULL in the table definition; rather, the
CHECK constraint is dumped as a separate table constraint (still within
the CREATE TABLE statement though).  This preserves any possible
constraint name, in case one was specified by the user at creation time.

In order to search for the correct constraint for each column for
various DDL actions, I just inspect each pg_constraint row for the table
and match conkey and the CHECK expression.  Some things would be easier
with a new pg_attribute column that carries a pg_constraint.oid of the
constraint for that column; however, that seems to be just catalog bloat
and is not normalized, so I decided not to do it.

Nice side-effect: if you add CHECK (foo IS NOT NULL) NOT VALID, and
later validate that constraint, the attnotnull bit becomes set.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

References to past discussions and patches:

https://postgr.es/m/CAKOSWNkN6HSyatuys8xZxzRCR-KL1OkHS5-b9qd9bf1Rad3PLA@mail.gmail.com
https://www.postgresql.org/message-id/flat/1343682669-sup-2532@alvh.no-ip.org
https://www.postgresql.org/message-id/20160109030002.GA671800@alvherre.pgsql

I started this time around from the newest of my patches in those
threads, but the implementation has changed considerably from what's
there.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote:
> I've been working on having NOT NULL constraints have pg_constraint
> rows.
> 
> Everything is working now.  Some things are a bit weird, and I would
> like opinions on them:
> 
> 1. In my implementation, you can have more than one NOT NULL
>    pg_constraint row for a column.  What should happen if the user does
>    ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL;
>    ?  Currently it throws an error about the ambiguity (ie. which
>    constraint to drop).

I'd say that is a good solution, particularly if there is a hint to drop
the constraint instead, similar to when you try to drop an index that
implements a constraint.

>    Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull'
>    bit is lost when the last one such constraint goes away.

Wouldn't it be the correct solution to set "attnotnumm" to FALSE only
when the last NOT NULL constraint is dropped?

> 2. If a table has a primary key, and a table is created that inherits
>    from it, then the child has its column(s) marked attnotnull but there
>    is no pg_constraint row for that.  This is not okay.  But what should
>    happen?
> 
>    1. a CHECK(col IS NOT NULL) constraint is created for each column
>    2. a PRIMARY KEY () constraint is created

I think it would be best to create a primary key constraint on the
partition.

Yours,
Laurenz Albe

On 2022-Aug-18, Laurenz Albe wrote:

> On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote:

> > 1. In my implementation, you can have more than one NOT NULL
> >    pg_constraint row for a column.  What should happen if the user does
> >    ALTER TABLE .. ALTER COLUMN .. DROP NOT NULL;
> >    ?  Currently it throws an error about the ambiguity (ie. which
> >    constraint to drop).
> 
> I'd say that is a good solution, particularly if there is a hint to drop
> the constraint instead, similar to when you try to drop an index that
> implements a constraint.

Ah, I didn't think about the hint.  I'll add that, thanks.

> >    Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull'
> >    bit is lost when the last one such constraint goes away.
> 
> Wouldn't it be the correct solution to set "attnotnumm" to FALSE only
> when the last NOT NULL constraint is dropped?

... when the last NOT NULL or PRIMARY KEY constraint is dropped.  We
have to keep attnotnull set when a PK exists even if there's no specific
NOT NULL constraint.

> > 2. If a table has a primary key, and a table is created that inherits
> >    from it, then the child has its column(s) marked attnotnull but there
> >    is no pg_constraint row for that.  This is not okay.  But what should
> >    happen?
> > 
> >    1. a CHECK(col IS NOT NULL) constraint is created for each column
> >    2. a PRIMARY KEY () constraint is created
> 
> I think it would be best to create a primary key constraint on the
> partition.

Sorry, I wasn't specific enough.  This applies to legacy inheritance
only; partitioning has its own solution (as you say: the PK constraint
exists), but legacy inheritance works differently.  Creating a PK in
children tables is not feasible (because unicity cannot be maintained),
but creating a CHECK (NOT NULL) constraint is possible.

I think a PRIMARY KEY should not be allowed to exist in an inheritance
parent, precisely because of this problem, but it seems too late to add
that restriction now.  This behavior is absurd, but longstanding:

55432 16devel 1787364=# create table parent (a int primary key);
CREATE TABLE
55432 16devel 1787364=# create table child () inherits (parent);
CREATE TABLE
55432 16devel 1787364=# insert into parent values (1);
INSERT 0 1
55432 16devel 1787364=# insert into child values (1);
INSERT 0 1
55432 16devel 1787364=# select * from parent;
 a 
───
 1
 1
(2 filas)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"But static content is just dynamic content that isn't moving!"
                http://smylers.hates-software.com/2007/08/15/fe244d0c.html

On Thu, 2022-08-18 at 11:04 +0200, Alvaro Herrera wrote:
> On 2022-Aug-18, Laurenz Albe wrote:
> > On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote:
> > >    Using ALTER TABLE DROP CONSTRAINT works fine, and the 'attnotnull'
> > >    bit is lost when the last one such constraint goes away.
> > 
> > Wouldn't it be the correct solution to set "attnotnumm" to FALSE only
> > when the last NOT NULL constraint is dropped?
> 
> ... when the last NOT NULL or PRIMARY KEY constraint is dropped.  We
> have to keep attnotnull set when a PK exists even if there's no specific
> NOT NULL constraint.

Of course, I forgot that.
I hope that is not too hard to implement.

> > > 2. If a table has a primary key, and a table is created that inherits
> > >    from it, then the child has its column(s) marked attnotnull but there
> > >    is no pg_constraint row for that.  This is not okay.  But what should
> > >    happen?
> > > 
> > >    1. a CHECK(col IS NOT NULL) constraint is created for each column
> > >    2. a PRIMARY KEY () constraint is created
> > 
> > I think it would be best to create a primary key constraint on the
> > partition.
> 
> Sorry, I wasn't specific enough.  This applies to legacy inheritance
> only; partitioning has its own solution (as you say: the PK constraint
> exists), but legacy inheritance works differently.  Creating a PK in
> children tables is not feasible (because unicity cannot be maintained),
> but creating a CHECK (NOT NULL) constraint is possible.
> 
> I think a PRIMARY KEY should not be allowed to exist in an inheritance
> parent, precisely because of this problem, but it seems too late to add
> that restriction now.  This behavior is absurd, but longstanding:

My mistake; you clearly said "inherits".

Since such an inheritance child currently does not have a primary key, you
can insert duplicates.  So automatically adding a NUT NULL constraint on the
inheritance child seems the only solution that does not break backwards
compatibility.  pg_upgrade would have to be able to cope with that.

Forcing a primary key constraint on the inheritance child could present an
upgrade problem.  Even if that is probably a rare and strange case, I don't
think we should risk that.  Moreover, if we force a primary key on the
inheritance child, using ALTER TABLE ... INHERIT might have to create a
unique index on the table, which can be cumbersome if the table is large.

So I think a NOT NULL constraint is the least evil.

Yours,
Laurenz Albe

On Thu, Aug 18, 2022 at 6:04 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> On 2022-Aug-18, Laurenz Albe wrote:
> > On Wed, 2022-08-17 at 20:12 +0200, Alvaro Herrera wrote:
> > > 2. If a table has a primary key, and a table is created that inherits
> > >    from it, then the child has its column(s) marked attnotnull but there
> > >    is no pg_constraint row for that.  This is not okay.  But what should
> > >    happen?
> > >
> > >    1. a CHECK(col IS NOT NULL) constraint is created for each column
> > >    2. a PRIMARY KEY () constraint is created
> >
> > I think it would be best to create a primary key constraint on the
> > partition.
>
> Sorry, I wasn't specific enough.  This applies to legacy inheritance
> only; partitioning has its own solution (as you say: the PK constraint
> exists), but legacy inheritance works differently.  Creating a PK in
> children tables is not feasible (because unicity cannot be maintained),
> but creating a CHECK (NOT NULL) constraint is possible.

Yeah, I think it makes sense to think of the NOT NULL constraints on
their own in this case, without worrying about the PK constraint that
created them in the first place.

BTW, maybe you are aware, but the legacy inheritance implementation is
not very consistent about wanting to maintain the same NULLness for a
given column in all members of the inheritance tree.  For example, it
allows one to alter the NULLness of an inherited column:

create table p (a int not null);
create table c (a int) inherits (p);
\d c
                 Table "public.c"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           | not null |
Inherits: p

alter table c alter a drop not null ;
ALTER TABLE
\d c
                 Table "public.c"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           |          |
Inherits: p

Contrast that with the partitioning implementation:

create table pp (a int not null) partition by list (a);
create table cc partition of pp default;
\d cc
                 Table "public.cc"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           | not null |
Partition of: pp DEFAULT

alter table cc alter a drop not null ;
ERROR:  column "a" is marked NOT NULL in parent table

IIRC, I had tried to propose implementing the same behavior for legacy
inheritance back in the day, but maybe we left it alone for not
breaking compatibility.

-- 
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

On 2022-Aug-22, Amit Langote wrote:

> Yeah, I think it makes sense to think of the NOT NULL constraints on
> their own in this case, without worrying about the PK constraint that
> created them in the first place.

Cool, that's enough votes that I'm comfortable implementing things that
way.

> BTW, maybe you are aware, but the legacy inheritance implementation is
> not very consistent about wanting to maintain the same NULLness for a
> given column in all members of the inheritance tree.  For example, it
> allows one to alter the NULLness of an inherited column:

Right ... I think what gives this patch most of its complexity is the
number of odd, inconsistent cases that have to preserve historical
behavior.  Luckily I think this particular behavior is easy to
implement.

> IIRC, I had tried to propose implementing the same behavior for legacy
> inheritance back in the day, but maybe we left it alone for not
> breaking compatibility.

Yeah, that wouldn't be surprising.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"The problem with the facetime model is not just that it's demoralizing, but
that the people pretending to work interrupt the ones actually working."
                                                           (Paul Graham)

So I was wrong in thinking that "this case was simple to implement" as I
replied upthread.  Doing that actually required me to rewrite large
parts of the patch.  I think it ended up being a good thing, because in
hindsight the approach I was using was somewhat bogus anyway, and the
current one should be better.  Please find it attached.

There are still a few problems, sadly.  Most notably, I ran out of time
trying to fix a pg_upgrade issue with pg_dump in binary-upgrade mode.
I have to review that again, but I think it'll need a deeper rethink of
how we pg_upgrade inherited constraints.  So the pg_upgrade tests are
known to fail.  I'm not aware of any other tests failing, but I'm sure
the cfbot will prove me wrong.

I reluctantly added a new ALTER TABLE subcommand type, AT_SetAttNotNull,
to allow setting pg_attribute.attnotnull without adding a CHECK
constraint (only used internally).  I would like to find a better way to
go about this, so I may remove it again, therefore it's not fully
implemented.

There are *many* changed regress expect files and I didn't carefully vet
all of them.  Mostly it's the addition of CHECK constraints in the
footers of many \d listings and stuff like that.  At a quick glance they
appear valid, but I need to review them more carefully still.

We've had pg_constraint.conparentid for a while now, but for some
constraints we continue to use conislocal/coninhcount.  I think we
should get rid of that and rely on conparentid completely.

An easily fixed issue is that of constraint naming.
ChooseConstraintName has an argument for passing known constraint names,
but this patch doesn't use it and it must.

One issue that I don't currently know how to fix, is the fact that we
need to know whether a column is a row type or not (because they need a
different null test).  At table creation time that's easy to know,
because we have the descriptor already built by the time we add the
constraints; but if you do ALTER TABLE .. ADD COLUMN .., ADD CONSTRAINT
then we don't.

Some ancient code comments suggest that allowing a child table's NOT
NULL constraint acquired from parent shouldn't be independently
droppable.  This patch doesn't change that, but it's easy to do if we
decide to.  However, that'd be a compatibility break, so I'd rather not
do it in the same patch that introduces the feature.

Overall, there's a lot more work required to get this to a good shape.
That said, I think it's the right direction.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

There were a lot more problems in that submission than I at first
realized, and I had to rewrite a lot of code in order to fix them.  I
have fixed all the user-visible problems I found in this version, and
reviewed the tests results more carefully so I am now more confident
that behaviourally it's doing the right thing; but

1. the pg_upgrade test problem is still unaddressed,
2. I haven't verified that catalog contents is correct, especially
   regarding dependencies,
3. there are way too many XXX and FIXME comments sprinkled everywhere.

I'm sure a couple of these XXX comments can be left for later work, and
there's a few that should be dealt with by merely removing them; but the
others (and all FIXMEs) represent pending work.

Also, I'm not at all happy about having this new ConstraintNotNull
artificial node there; perhaps this can be solved by using a regular
Constraint with some new flag, or maybe it will even work without any
extra flags by the fact that the node appears where it appears.  Anyway,
requires investigation.  Also, the AT_SetAttNotNull continues to irk me.

test_ddl_deparse is also unhappy.  This is probably an easy fix;
apparently, ATExecDropConstraint has been doing things wrong forever.

Anyway, here's version 2 of this, with apologies for those who spent
time reviewing version 1 with all its brokenness.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"On the other flipper, one wrong move and we're Fatal Exceptions"
(T.U.X.: Term Unit X  - http://www.thelinuxreview.com/TUX/)

 Hi Alvaro,

On Sat, Sep 10, 2022 at 2:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> There were a lot more problems in that submission than I at first
> realized, and I had to rewrite a lot of code in order to fix them.  I
> have fixed all the user-visible problems I found in this version, and
> reviewed the tests results more carefully so I am now more confident
> that behaviourally it's doing the right thing; but
>
> 1. the pg_upgrade test problem is still unaddressed,
> 2. I haven't verified that catalog contents is correct, especially
>    regarding dependencies,
> 3. there are way too many XXX and FIXME comments sprinkled everywhere.
>
> I'm sure a couple of these XXX comments can be left for later work, and
> there's a few that should be dealt with by merely removing them; but the
> others (and all FIXMEs) represent pending work.
>
> Also, I'm not at all happy about having this new ConstraintNotNull
> artificial node there; perhaps this can be solved by using a regular
> Constraint with some new flag, or maybe it will even work without any
> extra flags by the fact that the node appears where it appears.  Anyway,
> requires investigation.  Also, the AT_SetAttNotNull continues to irk me.
>
> test_ddl_deparse is also unhappy.  This is probably an easy fix;
> apparently, ATExecDropConstraint has been doing things wrong forever.
>
> Anyway, here's version 2 of this, with apologies for those who spent
> time reviewing version 1 with all its brokenness.

I have been testing this with the intention of understanding how you
made this work with inheritance.  While doing so with the previous
version, I ran into an existing issue (bug?) that I reported at [1].

I ran into another while testing version 2 that I think has to do with
this patch.  So this happens:

-- regular inheritance
create table foo (a int not null);
create table foo1 (a int not null);
alter table foo1 inherit foo;
alter table foo alter a drop not null ;
ERROR:  constraint "foo_a_not_null" of relation "foo1" does not exist

-- partitioning
create table parted (a int not null) partition by list (a);
create table part1 (a int not null);
alter table parted attach partition part1 default;
alter table parted alter a drop not null;
ERROR:  constraint "parted_a_not_null" of relation "part1" does not exist

In both of these cases, MergeConstraintsIntoExisting(), called by
CreateInheritance() when attaching the child to the parent, marks the
child's NOT NULL check constraint as the child constraint of the
corresponding constraint in parent, which seems fine and necessary.

However, ATExecDropConstraint_internal(), the new function called by
ATExecDropNotNull(), doesn't seem to recognize when recursing to the
child tables that a child's copy NOT NULL check constraint attached to
the parent's may have a different name, so scanning pg_constraint with
the parent's name is what gives the above error.  I wonder if it
wouldn't be better for ATExecDropNotNull() to handle its own recursion
rather than delegating it to the DropConstraint()?

The same error does not occur when the NOT NULL constraint is added to
parent after-the-fact and thus recursively to the children:

-- regular inheritance
create table foo (a int);
create table foo1 (a int not null) inherits (foo);
alter table foo alter a set not null;
alter table foo alter a drop not null ;
ALTER TABLE

-- partitioning
create table parted (a int) partition by list (a);
create table part1 partition of parted (a not null) default;
alter table parted alter a set not null;
alter table parted alter a drop not null;
ALTER TABLE

And the reason for that seems a bit accidental, because
MergeWithExistingConstraint(), called by AddRelationNewConstraints()
when recursively adding the NOT NULL check constraint to a child, does
not have the code to find the child's already existing constraint that
matches with it.  So, in this case, we get a copy of the parent's
constraint with the same name in the child.  There is a line in the
header comments of both MergeWithExistingConstraint() and
MergeConstraintsIntoExisting() asking to keep their code in sync, so
maybe the patch missed adding the new NOT NULL check constraint logic
to the former?

Also, it seems that the inheritance recursion for SET NOT NULL is now
occurring both in the prep phase and exec phase due to the following
new code added to ATExecSetNotNull():

@@ -7485,6 +7653,50 @@ ATExecSetNotNull(AlteredTableInfo *tab, Relation rel,
    InvokeObjectPostAlterHook(RelationRelationId,
                              RelationGetRelid(rel), attnum);
 ...
+       /* See if there's one already, and skip this if so. */
+       constr = findNotNullConstraintAttnum(rel, attnum, NULL);
+       if (constr && direct)
+           heap_freetuple(constr); /* nothing to do */
+       else
+       {
+           Constraint *newconstr;
+           ObjectAddress addr;
+           List       *children;
+           List       *already_done_rels;
+
+           newconstr = makeCheckNotNullConstraint(rel->rd_rel->relnamespace,
+                                                  constrname,
+
NameStr(rel->rd_rel->relname),
+                                                  colName,
+                                                  false, /* XXX is_row */
+                                                  InvalidOid);
+
+           addr = ATAddCheckConstraint_internal(wqueue, tab, rel, newconstr,
+                                                false, false, false, lockmode);
+           already_done_rels = list_make1_oid(RelationGetRelid(rel));
+
+           /* and recurse into children, if there are any */
+           children =
find_inheritance_children(RelationGetRelid(rel), lockmode);
+           ATAddCheckConstraint_recurse(wqueue, children, newconstr,

It seems harmless because ATExecSetNotNull() set up to run on the
children by the prep phase becomes a no-op due to the work done by the
above code, but maybe we should keep one or the other.

Regarding the following bit:

-   /* If rel is partition, shouldn't drop NOT NULL if parent has the same */
+   /*
+    * If rel is partition, shouldn't drop NOT NULL if parent has the same.
+    * XXX is this consideration still valid?  Can we get rid of this by
+    * changing the type of dependency between the two constraints instead?
+    */
    if (rel->rd_rel->relispartition)
    {
        Oid         parentId =
get_partition_parent(RelationGetRelid(rel), false);

Yes, it seems we can now prevent dropping a partition's NOT NULL
constraint by seeing that it is inherited, so no need for this block
which was written when the NOT NULL constraints didn't have the
inherited marking.

BTW, have you thought about making DROP NOT NULL command emit a
different error message than what one gets now:

create table foo (a int);
create table foo1 (a int) inherits (foo);
alter table foo alter a set not null;
alter table foo1 alter a drop not null ;
ERROR:  cannot drop inherited constraint "foo_a_not_null" of relation "foo1"

Like, say:

ERROR: cannot drop an inherited NOT NULL constraint

Maybe you did and thought that it's OK for it to spell out the
internally generated constraint name, because we already require users
to know that they exist, say to drop it using DROP CONSTRAINT.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

[1] https://www.postgresql.org/message-id/CA%2BHiwqFggpjAvsVqNV06HUF6CUrU0Vo3pLgGWCViGbPkzTiofg%40mail.gmail.com

On 09.09.22 19:58, Alvaro Herrera wrote:
> There were a lot more problems in that submission than I at first
> realized, and I had to rewrite a lot of code in order to fix them.  I
> have fixed all the user-visible problems I found in this version, and
> reviewed the tests results more carefully so I am now more confident
> that behaviourally it's doing the right thing; but

Reading through the SQL standard again, I think this patch goes a bit 
too far in folding NOT NULL and CHECK constraints together.  The spec 
says that you need to remember whether a column was defined as NOT NULL, 
and that the commands DROP NOT NULL and SET NOT NULL only affect 
constraints defined in that way.  In this implementation, a constraint 
defined as NOT NULL is converted to a CHECK (x IS NOT NULL) constraint 
and the original definition is forgotten.

Besides that, I think that users are not going to like that pg_dump 
rewrites their NOT NULL constraints into CHECK table constraints.

I suspect that this needs a separate contype for NOT NULL constraints 
that is separate from CONSTRAINT_CHECK.

On 2022-Sep-14, Peter Eisentraut wrote:

> Reading through the SQL standard again, I think this patch goes a bit too
> far in folding NOT NULL and CHECK constraints together.  The spec says that
> you need to remember whether a column was defined as NOT NULL, and that the
> commands DROP NOT NULL and SET NOT NULL only affect constraints defined in
> that way.  In this implementation, a constraint defined as NOT NULL is
> converted to a CHECK (x IS NOT NULL) constraint and the original definition
> is forgotten.

Hmm, I don't read it the same way.  Reading SQL:2016, they talk about a
nullability characteristic (/known not nullable/ or /possibly
nullable/):

: 4.13 Columns, fields, and attributes
: [...]
: Every column has a nullability characteristic that indicates whether the
: value from that column can be the null value. A nullability characteristic
: is either known not nullable or possibly nullable.
: Let C be a column of a base table T. C is known not nullable if and only
: if at least one of the following is true:
: — There exists at least one constraint NNC that is enforced and not
: deferrable and that simply contains a <search condition> that is a
: <boolean value expression> that is a readily-known-not-null condition for C.
: [other possibilities]

then in the same section they explain that this is derived from a
table constraint:

: A column C is described by a column descriptor. A column descriptor
: includes:
: [...]
: — If C is a column of a base table, then an indication of whether it is
: defined as NOT NULL and, if so, the constraint name of the associated
: table constraint definition.

   [aside: note that elsewhere (<boolean value expression>), they define
   "readily-known-not-null" in Syntax Rule 3), of 6.39 <boolean value
   expression>:

   : 3) Let X denote either a column C or the <key word> VALUE. Given a
   : <boolean value expression> BVE and X, the notion “BVE is a
   : readily-known-not-null condition for X” is defined as follows.
   : Case:
   :  a) If BVE is a <predicate> of the form “RVE IS NOT NULL”, where RVE is a
   :     <row value predicand> that is a <row value constructor predicand> that
   :     simply contains a <common value expression>, <boolean predicand>, or
   :     <row value constructor element> that is a <column reference> that
   :     references C, then BVE is a readily-known-not-null condition for C.
   :  b) If BVE is the <predicate> “VALUE IS NOT NULL”, then BVE is a
   :     readily-known-not-null condition for VALUE.
   :  c) Otherwise, BVE is not a readily-known-not-null condition for X.
   edisa]

Later, <column definition> says literally that specifying NOT NULL in a
column is equivalent to the CHECK (.. IS NOT NULL) table constraint:

: 11.4 <column definition>
: 
: Syntax Rules,
: 17) If a <column constraint definition> is specified, then let CND be
: the <constraint name definition> if one is specified and let CND be the
: zero-length character character string otherwise; let CA be the
: <constraint characteristics> if specified and let CA be the zero-length
: character string otherwise. The <column constraint definition> is
: equivalent to a <table constraint definition> as follows.
: 
: Case:
: 
: a) If a <column constraint definition> is specified that contains the
: <column constraint> NOT NULL, then it is equivalent to the following
: <table constraint definition>:
:    CND CHECK ( C IS NOT NULL ) CA

In my reading of it, it doesn't follow that you have to remember whether
the table constraint was created by saying explicitly by doing CHECK (c
IS NOT NULL) or as a plain NOT NULL column constraint.  The idea of
being able to do DROP NOT NULL when only a constraint defined as CHECK
(c IS NOT NULL) exists seems to follow from there; and also that you can
use DROP CONSTRAINT to remove one added via plain NOT NULL; and that
both these operations change the nullability characteristic of the
column.  This is made more explicit by the fact that they do state that
the nullability characteristic can *not* be "destroyed" for other types
of constraints, in 11.26 <drop table constraint definition>, Syntax Rule
11)

: 11) Destruction of TC shall not cause the nullability characteristic of
: any of the following columns of T to change from known not nullable to
: possibly nullable:
: 
: a) A column that is a constituent of the primary key of T, if any.
: b) The system-time period start column, if any.
: c) The system-time period end column, if any.
: d) The identity column, if any.

then General Rule 7) explains that this does indeed happen for columns
declared to have some sort of NOT NULL constraint, without saying
exactly how was that constraint defined:

: 7) If TC causes some column COL to be known not nullable and no other
: constraint causes COL to be known not nullable, then the nullability
: characteristic of the column descriptor of COL is changed to possibly
: nullable.

> Besides that, I think that users are not going to like that pg_dump rewrites
> their NOT NULL constraints into CHECK table constraints.

This is a good point, but we could get around it by decreeing that
pg_dump dumps the NOT NULL in the old way if the name is not changed
from whatever would be generated normally.  This would require some
games to remove the CHECK one; and it would also mean that partitions
would not use the same constraint as the parent, but rather it'd have to
generate a new constraint name that uses its own table name, rather than
the parent's.

(This makes me wonder what should happen if you rename a table: should
we go around and rename all the automatically-named constraints as well?
Probably not, but this may annoy people that creates table under one
name, then rename them into their final places afterwards.  pg_dump may
behave funny for those.  We can tackle that later, if ever.  But
consider that moving the table across schemas might cause even weirder
problems, since the standard says constraint names must not conflict
within a schema ...)

> I suspect that this needs a separate contype for NOT NULL constraints that
> is separate from CONSTRAINT_CHECK.

Maybe it is possible to read this in the way you propose, but I think
that interpretation is strictly less useful than the one I propose.
Also, see this reply from Tom to Vitaly Burovoy who was proposing
something that seems to derivate from this interpretation:
https://www.postgresql.org/message-id/flat/17684.1462339177%40sss.pgh.pa.us

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Hay dos momentos en la vida de un hombre en los que no debería
especular: cuando puede permitírselo y cuando no puede" (Mark Twain)

On Wed, Aug 17, 2022 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> If you say CREATE TABLE (a int NOT NULL), you'll get a CHECK constraint
> printed by psql: (this is a bit more noisy that previously and it
> changes a lot of regression tests output).
>
> 55489 16devel 1776237=# create table tab (a int not null);
> CREATE TABLE
> 55489 16devel 1776237=# \d tab
>                     Tabla «public.tab»
>  Columna │  Tipo   │ Ordenamiento │ Nulable  │ Por omisión
> ─────────┼─────────┼──────────────┼──────────┼─────────────
>  a       │ integer │              │ not null │
> Restricciones CHECK:
>     "tab_a_not_null" CHECK (a IS NOT NULL)

In a table with many columns, most of which are NOT NULL, this is
going to produce a ton of clutter. I don't like that.

I'm not sure what a good alternative would be, though.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Isaac Morland <isaac.morland@gmail.com> writes:
> I thought I saw some discussion about the SQL standard saying that there is
> a difference between putting NOT NULL in a column definition, and CHECK
> (column_name NOT NULL). So if we're going to take this seriously, I think
> that means there needs to be a field in pg_constraint which identifies
> whether a constraint is a "real" one created explicitly as a constraint, or
> if it is just one created because a field is marked NOT NULL.

If we're going to go that way, I think that we should take the further
step of making not-null constraints be their own contype rather than
an artificially generated CHECK.  The bloat in pg_constraint from CHECK
expressions made this way seems like an additional reason not to like
doing it like that.

            regards, tom lane

On Mon, 19 Sept 2022 at 15:32, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Aug 17, 2022 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > If you say CREATE TABLE (a int NOT NULL), you'll get a CHECK constraint
> > printed by psql: (this is a bit more noisy that previously and it
> > changes a lot of regression tests output).
> >
> > 55489 16devel 1776237=# create table tab (a int not null);
> > CREATE TABLE
> > 55489 16devel 1776237=# \d tab
> >                     Tabla «public.tab»
> >  Columna │  Tipo   │ Ordenamiento │ Nulable  │ Por omisión
> > ─────────┼─────────┼──────────────┼──────────┼─────────────
> >  a       │ integer │              │ not null │
> > Restricciones CHECK:
> >     "tab_a_not_null" CHECK (a IS NOT NULL)
>
> In a table with many columns, most of which are NOT NULL, this is
> going to produce a ton of clutter. I don't like that.
>
> I'm not sure what a good alternative would be, though.

I'm not sure on the 'good' part of this alternative, but we could go
with a single row-based IS NOT NULL to reduce such clutter, utilizing
the `ROW() IS NOT NULL` requirement of a row only matching IS NOT NULL
when all attributes are also IS NOT NULL:

    Check constraints:
        "tab_notnull_check" CHECK (ROW(a, b, c, d, e) IS NOT NULL)

instead of:

    Check constraints:
        "tab_a_not_null" CHECK (a IS NOT NULL)
        "tab_b_not_null" CHECK (b IS NOT NULL)
        "tab_c_not_null" CHECK (c IS NOT NULL)
        "tab_d_not_null" CHECK (d IS NOT NULL)
        "tab_e_not_null" CHECK (e IS NOT NULL)

But the performance of repeated row-casting would probably not be as
good as our current NULL checks if we'd use the current row
infrastructure, and constraint failure reports wouldn't be as helpful
as the current attribute NOT NULL failures.

Kind regards,

Matthias van de Meent

On 2022-Sep-19, Robert Haas wrote:

> On Wed, Aug 17, 2022 at 2:12 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > 55489 16devel 1776237=# \d tab
> >                     Tabla «public.tab»
> >  Columna │  Tipo   │ Ordenamiento │ Nulable  │ Por omisión
> > ─────────┼─────────┼──────────────┼──────────┼─────────────
> >  a       │ integer │              │ not null │
> > Restricciones CHECK:
> >     "tab_a_not_null" CHECK (a IS NOT NULL)
> 
> In a table with many columns, most of which are NOT NULL, this is
> going to produce a ton of clutter. I don't like that.
> 
> I'm not sure what a good alternative would be, though.

Perhaps that can be solved by displaying the constraint name in the
table:

 55489 16devel 1776237=# \d tab
                     Tabla «public.tab»
  Columna │  Tipo   │ Ordenamiento │     Nulable    │ Por omisión
 ─────────┼─────────┼──────────────┼────────────────┼─────────────
  a       │ integer │              │ tab_a_not_null │ 

(Perhaps we can change the column title also, not sure.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"The Gord often wonders why people threaten never to come back after they've
been told never to return" (www.actsofgord.com)

On 2022-Sep-19, Isaac Morland wrote:

> I thought I saw some discussion about the SQL standard saying that there is
> a difference between putting NOT NULL in a column definition, and CHECK
> (column_name NOT NULL). So if we're going to take this seriously,

Was it Peter E.'s reply to this thread?

https://postgr.es/m/bac841ed-b86d-e3c2-030d-02a3db067307@enterprisedb.com

because if it wasn't there, then I would like to know what you source
is.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Thou shalt not follow the NULL pointer, for chaos and madness await
thee at its end." (2nd Commandment for C programmers)

On 2022-Sep-19, Matthias van de Meent wrote:

> I'm not sure on the 'good' part of this alternative, but we could go
> with a single row-based IS NOT NULL to reduce such clutter, utilizing
> the `ROW() IS NOT NULL` requirement of a row only matching IS NOT NULL
> when all attributes are also IS NOT NULL:
> 
>     Check constraints:
>         "tab_notnull_check" CHECK (ROW(a, b, c, d, e) IS NOT NULL)

There's no way to mark this NOT VALID individually or validate it
afterwards, though.

> But the performance of repeated row-casting would probably not be as
> good as our current NULL checks

The NULL checks would still be mostly done by the attnotnull checks
internally, so there shouldn't be too much of a difference.

.. though I'm now wondering if there's additional overhead from checking
the constraint twice on each row: first the attnotnull bit, then the
CHECK itself.  Hmm.  That's probably quite bad.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 2022-Sep-20, Isaac Morland wrote:

> On Tue, 20 Sept 2022 at 06:56, Alvaro Herrera <alvherre@alvh.no-ip.org>
> wrote:
> 
> > .. though I'm now wondering if there's additional overhead from checking
> > the constraint twice on each row: first the attnotnull bit, then the
> > CHECK itself.  Hmm.  That's probably quite bad.
> 
> Another reason to treat NOT NULL-implementing constraints differently.

Yeah.

> My thinking is that pg_constraint entries for NOT NULL columns are mostly
> an implementation detail. I've certainly never cared whether I had an
> actual constraint corresponding to my NOT NULL columns.

Naturally, all catalog entries are implementation details; a user never
really cares if an entry exists or not, only that the desired semantics
are provided.  In this case, we want the constraint row because it gives
us some additional features, such as the ability to mark NOT NULL
constraints NOT VALID and validating them later, which is a useful thing
to do in large production databases.  We have some hacks to provide part
of that functionality using straight CHECK constraints, but you cannot
cleanly get the `attnotnull` flag set for a column (which means it's
hard to add a primary key, for example).

It is also supposed to fix some inconsistencies such as disallowing to
remove a constraint on a table when it is implied from a constraint on
an ancestor table.  Right now we have ad-hoc protections for partitions,
but we don't do that for legacy inheritance.

That said, the patch I posted for this ~10 years ago used a separate
contype and was simpler than what I ended up with now, but amusingly
enough it was returned at the time with the argument that it would be
better to treat them as normal CHECK constraints; so I want to be very
sure that we're not just going around in circles.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Tue, Sep 20, 2022 at 10:39 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> That said, the patch I posted for this ~10 years ago used a separate
> contype and was simpler than what I ended up with now, but amusingly
> enough it was returned at the time with the argument that it would be
> better to treat them as normal CHECK constraints; so I want to be very
> sure that we're not just going around in circles.

I don't have an intrinsic view on whether we ought to have one contype
or two, but I think this comment of yours from a few messages ago is
right on point: ".. though I'm now wondering if there's additional
overhead from checking
the constraint twice on each row: first the attnotnull bit, then the
CHECK itself.  Hmm.  That's probably quite bad." For that exact
reason, it seems absolutely necessary to be able to somehow identify
the "redundant" check constraints, so that we don't pay the expense of
revalidating them. Another contype would be one way of identifying
such constraints, but it could probably be done in other ways, too.
Perhaps it could even be discovered dynamically, like when we build a
relcache entry. I actually have no idea what design is best.

I am a little confused as to why we want to do this, though. While
we're on the topic of what is more complicated and simpler, what
functionality do we get out of adding all of these additional catalog
entries that then have to be linked back to the corresponding
attnotnull markings? And couldn't we get that functionality in some
much simpler way? Like, if you want to track whether the NOT NULL
constraint has been validated, we could add an attnotnullvalidated
column, or probably better, change the existing attnotnull column to a
character used as an enum, or maybe an integer bit-field of some kind.
I'm not trying to blow up your patch with dynamite or anything, but
I'm a little suspicious that this may be one of those cases where
pgsql-hackers discussed turns a complicated project into an even more
complicated project to no real benefit.

One thing that I don't particularly like about this whole design is
that it feels like it creates a bunch of catalog bloat. Now all of the
attnotnull flags also generate additional pg_constraint rows. The
catalogs in the default install will be bigger than before, and the
catalogs after user tables are created will be more bigger. If we get
some nifty benefit out of all that, cool! But if we're just
anti-optimizing the catalog storage out of some feeling that the
result will be intellectually purer than some competing design, maybe
we should reconsider. It's not stupid to optimize for common special
cases, and making a column as NOT NULL is probably at least one and
maybe several orders of magnitude more common than putting some other
CHECK constraint on it.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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.

0002 is the most interesting part.

Things that are curious:

- Inheritance and primary keys.  If you have a table with a primary key,
and create a child of it, that child is going to have a NOT NULL in the
column that is the primary key.

- Inheritance and plain constraints.  It is not allowed to remove the
NOT NULL constraint from a child; currently, NO INHERIT constraints are
not supported.  I would say this is an useless feature, but perhaps not.

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.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Los trabajadores menos efectivos son sistematicamente llevados al lugar
donde pueden hacer el menor daño posible: gerencia."  (El principio Dilbert)

Hmm, so it turned out that cfbot didn't like this because I didn't patch
one of the compression.out alternate files.  Fixed here.  I think in the
future I'm not going to submit the 0003 patch, because it's not very
interesting while being way too bulky and also the one most likely to
have conflicts.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Hay que recordar que la existencia en el cosmos, y particularmente la
elaboración de civilizaciones dentro de él no son, por desgracia,
nada idílicas" (Ijon Tichy)

On Tue, Feb 28, 2023 at 08:15:37PM +0100, Alvaro Herrera wrote:
> 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.

One concern here is that the title "NOT NULL Constraint" is itself
pretty wide, which is an issue for tables which have no not-null
constraints.

On Wed, Mar 01, 2023 at 01:03:48PM +0100, Alvaro Herrera wrote:
> Hmm, so it turned out that cfbot didn't like this because I didn't patch
> one of the compression.out alternate files.  Fixed here.  I think in the
> future I'm not going to submit the 0003 patch, because it's not very
> interesting while being way too bulky and also the one most likely to
> have conflicts.

I like \dt++, and it seems like the obvious thing to do here, to avoid
changing lots of regression test output, which seems worth avoiding in
any case, due to ensuing conflicts in other patches being developed, and
in backpatching.

Right now, \dt+ includes a bit too much output, including things like
sizes, which makes it hard to test.  Moving some things into \dt++ would
make \dt+ more testable (and more usable BTW).  Even if that's not true
of (or not a good idea) for \dt+, I'm sure it applies to other slash
commands.  Currently, fourty-five (45) psql commands support verbose
"plus" variants, and the sql regression tests exercise fifteen (15) of
them.

I proposed \dn++, \dA++, and \db++ in 2ndary patches here:
https://commitfest.postgresql.org/42/3256/

I've considered sending a patch with "plusplus" commands as 001, to
propose that on its own merits rather than in the context of \d[Abn]++

-- 
Justin

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?

> 0002 is the most interesting part.

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.

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

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.

> 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.  In practice, 
the name of the constraint is rarely needed.

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

As you wrote elsewhere, we can leave this patch alone for now.

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)

Here's v5.  I removed the business of renaming constraints in child
relations: recursing now just relies on matching column names.  Each
column has only one NOT NULL constraint; if you try to add another,
nothing happens.  All in all, this code is pretty similar to how we
handle inheritance of columns, which I think is good.

I added a mention that this funny syntax
  ALTER TABLE tab ADD CONSTRAINT NOT NULL col;
is not standard.  Maybe it's OK, but it seems a bit too prominent to me.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On 15.03.23 23:44, Alvaro Herrera wrote:
> Here's v5.  I removed the business of renaming constraints in child
> relations: recursing now just relies on matching column names.  Each
> column has only one NOT NULL constraint; if you try to add another,
> nothing happens.  All in all, this code is pretty similar to how we
> handle inheritance of columns, which I think is good.

This patch looks pretty okay to me now.  It matches all the functional 
expectations.

I suggest going through the tests carefully again and make sure all the 
changes are sensible and all the comments are correct.  There are a few 
places where the behavior of tests has changed (intentionally) but the 
surrounding comments don't match anymore, or objects that previously 
weren't created now succeed but then affect following tests.  Also, it 
seems some tests are left over from the first variant of this patch 
(where not-null constraints were converted to check constraints), and 
test names or comments should be updated to the current behavior.

I suppose we don't need any changes in pg_dump, since ruleutils.c 
handles that?

The information schema should be updated.  I think the following views:

- CHECK_CONSTRAINTS
- CONSTRAINT_COLUMN_USAGE
- DOMAIN_CONSTRAINTS
- TABLE_CONSTRAINTS

It looks like these have no test coverage; maybe that could be addressed 
at the same time.

On 27.03.23 15:55, Peter Eisentraut wrote:
> The information schema should be updated.  I think the following views:
> 
> - CHECK_CONSTRAINTS
> - CONSTRAINT_COLUMN_USAGE
> - DOMAIN_CONSTRAINTS
> - TABLE_CONSTRAINTS
> 
> It looks like these have no test coverage; maybe that could be addressed 
> at the same time.

Here are patches for this.  I haven't included the expected files for 
the tests; this should be checked again that output is correct or the 
changes introduced by this patch set are as expected.

The reason we didn't have tests for this before was probably in part 
because the information schema made up names for not-null constraints 
involving OIDs, so the test wouldn't have been stable.

Feel free to integrate this, or we can add it on afterwards.

On 2023-Mar-27, Peter Eisentraut wrote:

> I suggest going through the tests carefully again and make sure all the
> changes are sensible and all the comments are correct.  There are a few
> places where the behavior of tests has changed (intentionally) but the
> surrounding comments don't match anymore, or objects that previously weren't
> created now succeed but then affect following tests.  Also, it seems some
> tests are left over from the first variant of this patch (where not-null
> constraints were converted to check constraints), and test names or comments
> should be updated to the current behavior.

Thanks for reviewing!

Yeah, there were some obsolete tests.  I fixed those, added a couple
more, and while doing that I realized that failing to have NO INHERIT
constraints may be seen as regressing feature-wise, because there would
be no way to return to the situation where a parent table has a NOT NULL
but the children don't necessarily.  So I added that, and that led me to
changing the code structure a bit more in order to support *not* copying
the attnotnull flag in the cases where the parent only has it because of
a NO INHERIT constraint.

I'll go over this again tomorrow with fresh eyes, but I think it should
be pretty close to ready.  (Need to amend docs to note the new NO
INHERIT option for NOT NULL table constraints, and make sure pg_dump
complies.)

Tests are currently running: https://cirrus-ci.com/build/6261827823206400

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Las navajas y los monos deben estar siempre distantes"   (Germán Poo)

Hi,

On 2023-04-06 01:33:56 +0200, Alvaro Herrera wrote:
> I'll go over this again tomorrow with fresh eyes, but I think it should
> be pretty close to ready.  (Need to amend docs to note the new NO
> INHERIT option for NOT NULL table constraints, and make sure pg_dump
> complies.)

Missed this thread somehow.  This is not a review - I just want to say that I
am very excited that we might finally catalogue NOT NULL constraints. This has
been a long time in the making...

Greetings,

Andres Freund

On Wed, Apr 05, 2023 at 06:54:54PM -0700, Andres Freund wrote:
> On 2023-04-06 01:33:56 +0200, Alvaro Herrera wrote:
>> I'll go over this again tomorrow with fresh eyes, but I think it should
>> be pretty close to ready.  (Need to amend docs to note the new NO
>> INHERIT option for NOT NULL table constraints, and make sure pg_dump
>> complies.)
>
> Missed this thread somehow.  This is not a review - I just want to say that I
> am very excited that we might finally catalogue NOT NULL constraints. This has
> been a long time in the making...

+1!
--
Michael

On Thu, Apr 06, 2023 at 01:33:56AM +0200, Alvaro Herrera wrote:
> -   The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>),
> +   The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>, and
> +   except for the <literal>NOT NULL <replaceable>column_name</replaceable></literal>
> +   form to add a table constraint),

The "except" part seems pretty incoherent to me :(

> +     if (isnull)
> +             elog(ERROR, "null conkey for NOT NULL constraint %u", conForm->oid);

could use SysCacheGetAttrNotNull()

> +        if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
> +            ereport(ERROR,
> +                    errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> +                    errmsg("cannot add constraint to only the partitioned table when partitions exist"),
> +                    errhint("Do not specify the ONLY keyword."));
> +        else
> +            ereport(ERROR,
> +                    errcode(ERRCODE_INVALID_TABLE_DEFINITION),
> +                    errmsg("cannot add constraint to table with inheritance children"),

missing "only" ?

> +    conrel = table_open(ConstraintRelationId, RowExclusiveLock);

Should this be opened after the following error check ?

> +        arr = DatumGetArrayTypeP(adatum);    /* ensure not toasted */
> +        numkeys = ARR_DIMS(arr)[0];
> +        if (ARR_NDIM(arr) != 1 ||
> +            numkeys < 0 ||
> +            ARR_HASNULL(arr) ||
> +            ARR_ELEMTYPE(arr) != INT2OID)
> +            elog(ERROR, "conkey is not a 1-D smallint array");
> +        attnums = (int16 *) ARR_DATA_PTR(arr);
> +
> +        for (int i = 0; i < numkeys; i++)
> +            unconstrained_cols = lappend_int(unconstrained_cols, attnums[i]);
> +    }

Does "arr" need to be freed ?

> +             * Since the above deletion has been made visible, we can now
> +             * search for any remaining constraints on this column (or these
> +             * columns, in the case we're dropping a multicol primary key.)
> +             * Then, verify whether any further NOT NULL or primary key exist,

If I'm reading it right, I think it should say "exists"

> +/*
> + * When a primary key index on a partitioned table is to be attached an index
> + * on a partition, the partition's columns should also be marked NOT NULL.
> + * Ensure that is the case.

I think the comment may be missing words, or backwards.
The index on the *partitioned* table wouldn't be attached.
Is the index on the *partition* that's attached *to* the former index.

> +create table c() inherits(inh_p1, inh_p2, inh_p3, inh_p4);
> +NOTICE:  merging multiple inherited definitions of column "f1"
> +NOTICE:  merging multiple inherited definitions of column "f1"
> +ERROR:  relation "c" already exists

Do you intend to make an error here ?

Also, I think these table names may be too generic, and conflict with
other parallel tests, now or in the future.

> +create table d(a int not null, f1 int) inherits(inh_p3, c);
> +ERROR:  relation "d" already exists

And here ?

> +-- with explicitely specified not null constraints

sp: explicitly

-- 
Justin

On 2023-Apr-06, Justin Pryzby wrote:

> On Thu, Apr 06, 2023 at 01:33:56AM +0200, Alvaro Herrera wrote:
> > -   The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>),
> > +   The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>, and
> > +   except for the <literal>NOT NULL <replaceable>column_name</replaceable></literal>
> > +   form to add a table constraint),
> 
> The "except" part seems pretty incoherent to me :(

Yeah, I feared that would be the case.  I can't think of a wording
that doesn't take two lines, so suggestions welcome.

I handled your other comments, except these:

> > +    conrel = table_open(ConstraintRelationId, RowExclusiveLock);
> 
> Should this be opened after the following error check ?

Added new code in the middle when I found a small problem, so now the
table_open is necessary there.  (To wit: if we DROP NOT NULL a
constraint that is both locally defined in the table and inherited, we
should remove the "conislocal" flag and it's done.  Previously, we were
throwing an error that the constraint is inherited, but that's wrong.)

> > +        arr = DatumGetArrayTypeP(adatum);    /* ensure not toasted */
> 
> Does "arr" need to be freed ?

I see this pattern in one or two other places and we don't worry about
such small allocations too much.  (I copied this code almost verbatim
from somewhere IIRC).

Anyway, I found a couple of additional minor problems when playing with
some additional corner case scenarios; I cleaned up the test cases, per
Peter.  Then I realized that pg_dump support was missing completely, so
I filled that in.  Sadly, the binary-upgrade mode is a bit of a mess and
thus the pg_upgrade test is failing.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La experiencia nos dice que el hombre peló millones de veces las patatas,
pero era forzoso admitir la posibilidad de que en un caso entre millones,
las patatas pelarían al hombre" (Ijon Tichy)

I think this should fix the pg_upgrade issues.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La conclusión que podemos sacar de esos estudios es que
no podemos sacar ninguna conclusión de ellos" (Tanenbaum)

On Fri, Apr 07, 2023 at 04:14:13AM +0200, Alvaro Herrera wrote:
> On 2023-Apr-06, Justin Pryzby wrote:

> > +ERROR:  relation "c" already exists
>
> Do you intend to make an error here ?

These still look like mistakes in the tests.

> Also, I think these table names may be too generic, and conflict with
> other parallel tests, now or in the future.
>
> > +create table d(a int not null, f1 int) inherits(inh_p3, c);
> > +ERROR:  relation "d" already exists

> Sadly, the binary-upgrade mode is a bit of a mess and thus the
> pg_upgrade test is failing.

Hi,

I think there's some test instability:

Fail:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=parula&dt=2023-04-07%2018%3A43%3A02
Subsequent success, without relevant changes:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=parula&dt=2023-04-07%2020%3A22%3A01
Followed by a failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=parula&dt=2023-04-07%2020%3A31%3A02

Similar failures on other animals:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=komodoensis&dt=2023-04-07%2020%3A27%3A43
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=siskin&dt=2023-04-07%2020%3A09%3A25

There's also as second type of failure:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=dragonet&dt=2023-04-07%2020%3A23%3A35
..

I suspect there's a naming conflict between tests in different groups.

Greetings,

Andres Freund

Hi,

On 2023-04-07 13:38:43 -0700, Andres Freund wrote:
> I suspect there's a naming conflict between tests in different groups.

Yep:

test: create_aggregate create_function_sql create_cast constraints triggers select inherit typed_table vacuum
drop_if_existsupdatable_views roleattributes create_am hash_func errors infinite_recurse
 

src/test/regress/sql/inherit.sql
851:create table child(f1 int not null, f2 text not null) inherits(inh_parent_1, inh_parent_2);

src/test/regress/sql/triggers.sql
2127:create table child partition of parent for values in ('AAA');
2266:create table child () inherits (parent);
2759:create table child () inherits (parent);

The inherit.sql part is new.

I'll see how hard it is to fix.

Greetings,

Andres Freund

On 2023-Apr-07, Andres Freund wrote:

> src/test/regress/sql/triggers.sql
> 2127:create table child partition of parent for values in ('AAA');
> 2266:create table child () inherits (parent);
> 2759:create table child () inherits (parent);
> 
> The inherit.sql part is new.

Yeah.

> I'll see how hard it is to fix.

Running the tests for it now -- it's a short fix.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Learn about compilers. Then everything looks like either a compiler or
a database, and now you have two problems but one of them is fun."
            https://twitter.com/thingskatedid/status/1456027786158776329

Hi,

On 2023-04-07 23:00:01 +0200, Alvaro Herrera wrote:
> On 2023-Apr-07, Andres Freund wrote:
> 
> > src/test/regress/sql/triggers.sql
> > 2127:create table child partition of parent for values in ('AAA');
> > 2266:create table child () inherits (parent);
> > 2759:create table child () inherits (parent);
> > 
> > The inherit.sql part is new.
> 
> Yeah.
> 
> > I'll see how hard it is to fix.
> 
> Running the tests for it now -- it's a short fix.

I just pushed a fix - sorry, I thought you might have stopped working for the
day and CI finished with the modification a few seconds before your email
arrived...

Greetings,

Andres Freund

On 2023-Apr-07, Andres Freund wrote:

> I just pushed a fix - sorry, I thought you might have stopped working for the
> day and CI finished with the modification a few seconds before your email
> arrived...

Ah, cool, no worries.  I would have stopped indeed, but I had to stay
around in case of any test failures.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto.  No es cierto, y si fuera cierto,
 no me acuerdo."                 (Augusto Pinochet a una corte de justicia)

Hi,

On 2023-04-07 23:11:55 +0200, Alvaro Herrera wrote:
> On 2023-Apr-07, Andres Freund wrote:
> 
> > I just pushed a fix - sorry, I thought you might have stopped working for the
> > day and CI finished with the modification a few seconds before your email
> > arrived...
> 
> Ah, cool, no worries.  I would have stopped indeed, but I had to stay
> around in case of any test failures.

Looks like there's work for you if you want ;)
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2023-04-07%2018%3A52%3A13

But IMO fixing sepgsql can easily wait till tomorrow.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2023-04-07 23:11:55 +0200, Alvaro Herrera wrote:
>> Ah, cool, no worries.  I would have stopped indeed, but I had to stay
>> around in case of any test failures.

> Looks like there's work for you if you want ;)
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2023-04-07%2018%3A52%3A13

> But IMO fixing sepgsql can easily wait till tomorrow.

I can deal with that one -- it's a bit annoying to work with sepgsql
if you're not on a Red Hat platform.

After quickly eyeing the diffs, I'm just going to take the new output
as good.  I'm not surprised that there are additional output messages
given the additional catalog entries this made.  I *am* a bit surprised
that some messages seem to have disappeared --- are there places where
this resulted in fewer catalog accesses than before?  Nonetheless,
there's no good reason to assume this test is exposing any bugs.

            regards, tom lane

Hi,

On 2023-04-07 17:46:33 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2023-04-07 23:11:55 +0200, Alvaro Herrera wrote:
> >> Ah, cool, no worries.  I would have stopped indeed, but I had to stay
> >> around in case of any test failures.
> 
> > Looks like there's work for you if you want ;)
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros&dt=2023-04-07%2018%3A52%3A13
> 
> > But IMO fixing sepgsql can easily wait till tomorrow.
> 
> I can deal with that one -- it's a bit annoying to work with sepgsql
> if you're not on a Red Hat platform.

Indeed. I tried to get them running a while back, to enable the tests with
meson, without lot of success. Then I realized that they're also not wired up
in make... ;)


> After quickly eyeing the diffs, I'm just going to take the new output
> as good.  I'm not surprised that there are additional output messages
> given the additional catalog entries this made.  I *am* a bit surprised
> that some messages seem to have disappeared --- are there places where
> this resulted in fewer catalog accesses than before?  Nonetheless,
> there's no good reason to assume this test is exposing any bugs.

I wonder if the issue is that the new paths miss a hook invocation.

@@ -160,11 +160,7 @@
 ALTER TABLE regtest_table ALTER b SET DEFAULT 'XYZ';    -- not supported yet
 ALTER TABLE regtest_table ALTER b DROP DEFAULT;         -- not supported yet
 ALTER TABLE regtest_table ALTER b SET NOT NULL;
-LOG:  SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema_2.regtest_table.b" permissive=0
 
-LOG:  SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema.regtest_table_2.b" permissive=0
 
 ALTER TABLE regtest_table ALTER b DROP NOT NULL;
-LOG:  SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema_2.regtest_table.b" permissive=0
 
-LOG:  SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema.regtest_table_2.b" permissive=0
 
 ALTER TABLE regtest_table ALTER b SET STATISTICS -1;
 LOG:  SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema_2.regtest_table.b" permissive=0
 
 LOG:  SELinux: allowed { setattr } scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0
tcontext=unconfined_u:object_r:sepgsql_table_t:s0tclass=db_column name="regtest_schema.regtest_table_2.b" permissive=0
 

The 'not supported yet' cases don't emit messages. Previously SET NOT NULL
wasn't among that set, but seemingly it now is.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> On 2023-04-07 17:46:33 -0400, Tom Lane wrote:
>> After quickly eyeing the diffs, I'm just going to take the new output
>> as good.  I'm not surprised that there are additional output messages
>> given the additional catalog entries this made.  I *am* a bit surprised
>> that some messages seem to have disappeared --- are there places where
>> this resulted in fewer catalog accesses than before?  Nonetheless,
>> there's no good reason to assume this test is exposing any bugs.

> I wonder if the issue is that the new paths miss a hook invocation.

Perhaps.  I'm content to silence the buildfarm for today; we can
investigate more closely later.

            regards, tom lane

... BTW, shouldn't
https://commitfest.postgresql.org/42/3869/
now get closed as committed?

            regards, tom lane

Hi,

On 2023-04-07 18:26:28 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2023-04-07 17:46:33 -0400, Tom Lane wrote:
> >> After quickly eyeing the diffs, I'm just going to take the new output
> >> as good.  I'm not surprised that there are additional output messages
> >> given the additional catalog entries this made.  I *am* a bit surprised
> >> that some messages seem to have disappeared --- are there places where
> >> this resulted in fewer catalog accesses than before?  Nonetheless,
> >> there's no good reason to assume this test is exposing any bugs.
> 
> > I wonder if the issue is that the new paths miss a hook invocation.
> 
> Perhaps.  I'm content to silence the buildfarm for today; we can
> investigate more closely later.

Makes sense.

I think
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2023-04-07%2021%3A16%3A04
might point out a problem with the pg_dump or pg_upgrade backward compat
paths:

--- C:\\prog\\bf/root/upgrade.drongo/HEAD/origin-REL9_5_STABLE.sql.fixed    2023-04-07 23:51:27.641328600 +0000
+++ C:\\prog\\bf/root/upgrade.drongo/HEAD/converted-REL9_5_STABLE-to-HEAD.sql.fixed    2023-04-07 23:51:27.672571900
+0000
@@ -416,9 +416,9 @@
 -- Name: entry; Type: TABLE; Schema: public; Owner: buildfarm
 --
 CREATE TABLE public.entry (
-    accession text,
-    eid integer,
-    txid smallint
+    accession text NOT NULL,
+    eid integer NOT NULL,
+    txid smallint NOT NULL
 );
 ALTER TABLE public.entry OWNER TO buildfarm;
 --

Looks like we're making up NOT NULL constraints when migrating from 9.5, for
some reason?

Greetings,

Andres Freund

Hi,

On 2023-04-07 17:19:42 -0700, Andres Freund wrote:
> I think
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2023-04-07%2021%3A16%3A04
> might point out a problem with the pg_dump or pg_upgrade backward compat
> paths:
> 
> --- C:\\prog\\bf/root/upgrade.drongo/HEAD/origin-REL9_5_STABLE.sql.fixed    2023-04-07 23:51:27.641328600 +0000
> +++ C:\\prog\\bf/root/upgrade.drongo/HEAD/converted-REL9_5_STABLE-to-HEAD.sql.fixed    2023-04-07 23:51:27.672571900
+0000
> @@ -416,9 +416,9 @@
>  -- Name: entry; Type: TABLE; Schema: public; Owner: buildfarm
>  --
>  CREATE TABLE public.entry (
> -    accession text,
> -    eid integer,
> -    txid smallint
> +    accession text NOT NULL,
> +    eid integer NOT NULL,
> +    txid smallint NOT NULL
>  );
>  ALTER TABLE public.entry OWNER TO buildfarm;
>  --
> 
> Looks like we're making up NOT NULL constraints when migrating from 9.5, for
> some reason?

My compiler complains:

../../../../home/andres/src/postgresql/src/backend/catalog/heap.c: In function ‘AddRelationNotNullConstraints’:
../../../../home/andres/src/postgresql/src/backend/catalog/heap.c:2829:37: warning: ‘conname’ may be used uninitialized
[-Wmaybe-uninitialized]
 2829 |                                 if (strcmp(lfirst(lc2), conname) == 0)
      |                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../../../home/andres/src/postgresql/src/backend/catalog/heap.c:2802:29: note: ‘conname’ was declared here
 2802 |                 char       *conname;
      |                             ^~~~~~~

I think the compiler may be right - I think the first use of conname might
have been intended as constr->conname?

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:
> I think
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=drongo&dt=2023-04-07%2021%3A16%3A04
> might point out a problem with the pg_dump or pg_upgrade backward compat
> paths:

Yeah, this patch has broken every single upgrade-from-back-branch test.

I think there's a second problem, though: even without considering
back branches, this has changed pg_dump output in a way that
I fear is unacceptable.  Consider for instance this table definition
(from rules.sql):

create table rule_and_refint_t1 (
    id1a integer,
    id1b integer,
    primary key (id1a, id1b)
);

This used to be dumped as

CREATE TABLE public.rule_and_refint_t1 (
    id1a integer NOT NULL,
    id1b integer NOT NULL
);
...
... load data ...
...
ALTER TABLE ONLY public.rule_and_refint_t1
    ADD CONSTRAINT rule_and_refint_t1_pkey PRIMARY KEY (id1a, id1b);

In the new dispensation, pg_dump omits the NOT NULL clauses.
Great, you say, that makes the output more like what the user wrote.
I'm not so sure.  This means that the ALTER TABLE will be compelled
to perform a full-table scan to verify that there are no nulls in the
already-loaded data before it can add the missing NOT NULL constraint.
The old dump output was carefully designed to avoid the need for that
scan.  Admittedly, we have to do a scan anyway to build the index,
so this is strictly less than a 2X penalty on the ALTER, but is
that acceptable?  It might be all right in the context of regular
dump/restore, where we're surely doing a lot of per-row work anyway
to load the data and make the index.  In the context of pg_upgrade,
though, it seems absolutely disastrous: there will now be a per-row
cost where there was none before, and that is surely a deal-breaker.

BTW, I note from testing that the NOT NULL clauses *are* still
emitted in at least some cases when doing --binary-upgrade from an old
version.  (This may be directly related to the buildfarm failures,
not sure.)  That's no solution though, because now what you get in
pg_constraint will differ depending on which way you upgraded,
which seems unacceptable too.

I'm inclined to think that this idea of suppressing the implied
NOT NULL from PRIMARY KEY is a nonstarter and we should just
go ahead and make such a constraint.  Another idea could be for
pg_dump to emit the NOT NULL, load data, do the ALTER ADD PRIMARY
KEY, and then ALTER DROP NOT NULL.

In any case, I wonder whether that's the sort of redesign we should
be doing post-feature-freeze.  It might be best to revert and try
again in v17.

            regards, tom lane

On 2023-Apr-09, Tom Lane wrote:

> In the new dispensation, pg_dump omits the NOT NULL clauses.
> Great, you say, that makes the output more like what the user wrote.
> I'm not so sure.  This means that the ALTER TABLE will be compelled
> to perform a full-table scan to verify that there are no nulls in the
> already-loaded data before it can add the missing NOT NULL constraint.

Yeah, I agree that this unintended consequence isn't very palatable.  I
think the other pg_upgrade problem is easily fixed (haven't tried yet),
but having to rethink the pg_dump representation would likely take
longer than we'd like.

> I'm inclined to think that this idea of suppressing the implied
> NOT NULL from PRIMARY KEY is a nonstarter and we should just
> go ahead and make such a constraint.  Another idea could be for
> pg_dump to emit the NOT NULL, load data, do the ALTER ADD PRIMARY
> KEY, and then ALTER DROP NOT NULL.

I like that second idea, yeah.  It might be tough to make it work, but
I'll try.

> In any case, I wonder whether that's the sort of redesign we should
> be doing post-feature-freeze.  It might be best to revert and try
> again in v17.

Yeah, sounds like reverting for now and retrying in v17 with the
discussed changes might be better.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La espina, desde que nace, ya pincha" (Proverbio africano)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>> I'm inclined to think that this idea of suppressing the implied
>> NOT NULL from PRIMARY KEY is a nonstarter and we should just
>> go ahead and make such a constraint.  Another idea could be for
>> pg_dump to emit the NOT NULL, load data, do the ALTER ADD PRIMARY
>> KEY, and then ALTER DROP NOT NULL.

> I like that second idea, yeah.  It might be tough to make it work, but
> I'll try.

Yeah, I've been thinking more about it, and this might also yield a
workable solution for the TestUpgrade breakage.  The idea would be,
roughly, for pg_dump to emit NOT NULL column decoration in all the
same places it does now, and then to drop it again immediately after
doing ADD PRIMARY KEY if it judges that there was no other reason
to have it.  This gets rid of the inconsistency for --binary-upgrade
which I think is what is causing the breakage.

I also ran into something else I didn't much care for:

regression=# create table foo(f1 int primary key, f2 int);
CREATE TABLE
regression=# create table foochild() inherits(foo);
CREATE TABLE
regression=# alter table only foo alter column f2 set not null;
ERROR:  cannot add constraint only to table with inheritance children
HINT:  Do not specify the ONLY keyword.

Previous versions accepted this case, and I don't really see why
we can't do so with this new implementation -- isn't this exactly
what pg_constraint.connoinherit was invented to represent?  Moreover,
existing pg_dump files can contain precisely this construct, so
blowing it off isn't going to be zero-cost.

            regards, tom lane

OK, so here's a new attempt to get this working correctly.  This time I
did try the new pg_upgrade when starting with a pg_dumpall produced by a
server in branch 14 after running the regression tests.  The pg_upgrade
support is *really* finicky ...

The main novelty in this version of the patch, is that we now emit
"throwaway" NOT NULL constraints when a column is part of the primary
key.  Then, after the PK is created, we run a DROP for that constraint.
That lets us create the PK without having to scan the table during
pg_upgrade.  (I thought about adding a new dump object, either one per
table or just a single one for the whole dump, which would carry the
ALTER TABLE .. DROP CONSTRAINT commands for those throwaway constraints.
I decided that this is unnecessary, so the code the command in the same
dump object that does ALTER TABLE ADD PRIMARY KEY seems good enough.  If
somebody sees a reason to do it differently, we can.)


There's new funny business with RelationGetIndexList and primary keys of
partitioned tables.  With the patch, we continue to store the OID of the
PK even when that index is marked invalid.  The reason for this is
pg_dump: when it does the ALTER TABLE to drop the NOT NULLs, the columns
would become marked nullable, because since the PK is invalid, it's not
considered to protect the columns.  I guess it might be possible to
implement this in some other way, but I found none that were reasonable.
I didn't find that did had any undesirable side-effects anyway.


Scanning this thread, I think I left one reported issue unfixed related
to tables created LIKE others.  I'll give it a look later.  Other than
that I think all bases are covered, but I intend to leave the patch open
until near the end of the CF, in case someone wants to play with it.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"It takes less than 2 seconds to get to 78% complete; that's a good sign.
A few seconds later it's at 90%, but it seems to have stuck there.  Did
somebody make percentages logarithmic while I wasn't looking?"
                http://smylers.hates-software.com/2005/09/08/1995c749.html

Hi,

On 2023-06-30 13:44:03 +0200, Alvaro Herrera wrote:
> OK, so here's a new attempt to get this working correctly.

Thanks for continuing to work on this!


> The main novelty in this version of the patch, is that we now emit
> "throwaway" NOT NULL constraints when a column is part of the primary
> key.  Then, after the PK is created, we run a DROP for that constraint.
> That lets us create the PK without having to scan the table during
> pg_upgrade.

Have you considered extending the DDL statement for this purpose? We have
  ALTER TABLE ... ADD CONSTRAINT ... PRIMARY KEY USING INDEX ...;
we could just do something similar for the NOT NULL constraint?  Which would
then delete the separate constraint NOT NULL constraint.

Greetings,

Andres Freund

On 2023-Jun-30, Andres Freund wrote:

> On 2023-06-30 13:44:03 +0200, Alvaro Herrera wrote:
> 
> > The main novelty in this version of the patch, is that we now emit
> > "throwaway" NOT NULL constraints when a column is part of the primary
> > key.  Then, after the PK is created, we run a DROP for that constraint.
> > That lets us create the PK without having to scan the table during
> > pg_upgrade.
> 
> Have you considered extending the DDL statement for this purpose? We have
>   ALTER TABLE ... ADD CONSTRAINT ... PRIMARY KEY USING INDEX ...;
> we could just do something similar for the NOT NULL constraint?  Which would
> then delete the separate constraint NOT NULL constraint.

Hmm, I hadn't.  I think if we have to explicitly list the constraint
that we want dropped, then it's pretty much the same than as if we used
a comma-separated list of subcommands, like 

ALTER TABLE ... ADD CONSTRAINT .. PRIMARY KEY (a,b),
   DROP CONSTRAINT pgdump_throwaway_notnull_0,
   DROP CONSTRAINT pgdump_throwaway_notnull_1;

However, I think it would be ideal if we *don't* have to specify the
list of constraints: we would do this on any ALTER TABLE .. ADD
CONSTRAINT PRIMARY KEY, without having any additional clause.

But how to distinguish which NOT NULL markings to drop?  Maybe we would
have to specify a flag at NOT NULL constraint creation time.  So pg_dump
would emit something like

CREATE TABLE foo (a int CONSTRAINT NOT NULL THROWAWAY);
... (much later) ...
ALTER TABLE foo ADD CONSTRAINT .. PRIMARY KEY;

and by the time this second command is run, those throwaway constraints
are removed.  The problems now are 1) how to make this CREATE statement
more SQL-conformant (answer: make pg_dump emit a separate ALTER TABLE
command for the constraint addition; it already knows how to do this, so
it'd be very little code); but also 2) where to store the flag
server-side flag that says this constraint has this property.  I think
it'd have to be a new pg_constraint column, and I don't like to add one
for such a minor issue.

On 2023-Jun-30, Alvaro Herrera wrote:

> Scanning this thread, I think I left one reported issue unfixed related
> to tables created LIKE others.  I'll give it a look later.  Other than
> that I think all bases are covered, but I intend to leave the patch open
> until near the end of the CF, in case someone wants to play with it.

So it was [1] that I meant, where this example was provided:

# create table t1 (c int primary key null unique);
                                             
 
# create table t2 (like t1);
                                             
 
# alter table t2 alter c drop not null;
                                             
 
ERROR:  no NOT NULL constraint found to drop

The problem here is that because we didn't give INCLUDING INDEXES in the
LIKE clause, we end up with a column marked NOT NULL for which we have
no pg_constraint row.  Okay, I thought, we can just make sure *not* to
mark that case as not null; that works fine and looks reasonable.
However, it breaks the following use case, which is already in use in
the regression tests and possibly by users:

 CREATE TABLE pk (a int PRIMARY KEY) PARTITION BY RANGE (a);
 CREATE TABLE pk4 (LIKE pk);
 ALTER TABLE pk ATTACH PARTITION pk4 FOR VALUES FROM (3000) TO (4000);
+ERROR:  column "a" in child table must be marked NOT NULL

The problem here is that we were assuming, by the time the third command
is run, that the column had been marked NOT NULL by the second command.
So my solution above is simply not acceptable.  What we must do, in
order to handle this backward-compatibly, is to ensure that a column
part of a PK automatically gets a NOT NULL constraint for all the PK
columns, for the case where INCLUDING INDEXES is not given.  This is the
same we do for regular INHERITS children and PKs.

I'll go write this code now; should be simple enough.

[1] https://postgr.es/m/CAMbWs48astPDb3K+L89wb8Yju0jM_Czm8svmU=Uzd+WM61Cr6Q@mail.gmail.com

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

On 30.06.23 13:44, Alvaro Herrera wrote:
> OK, so here's a new attempt to get this working correctly.

Attached is a small fixup patch for the documentation.

Furthermore, there are a few outdated comments that are probably left 
over from previous versions of this patch set.


* src/backend/catalog/pg_constraint.c

Outdated comment:

+   /* only tuples for CHECK constraints should be given */
+   Assert(((Form_pg_constraint) GETSTRUCT(constrTup))->contype == 
CONSTRAINT_NOTNULL);


* src/backend/parser/gram.y

Should processCASbits() set &n->skip_validation, like in the CHECK
case?  _outConstraint() looks at the field, so it seems relevant.


* src/backend/parser/parse_utilcmd.c

The updated comment says

     List       *ckconstraints;  /* CHECK and NOT NULL constraints */

but it seems to me that NOT NULL constraints are not actually added
there but in nnconstraints instead.

It would be nice if the field nnconstraints was listed after
ckconstraints consistently throughout the file.  It's a bit random
right now.

This comment is outdated:

+               /*
+                * For NOT NULL declarations, we need to mark the column as
+                * not nullable, and set things up to have a CHECK 
constraint
+                * created.  Also, duplicate NOT NULL declarations are not
+                * allowed.
+                */

About this:

             case CONSTR_CHECK:
                 cxt->ckconstraints = lappend(cxt->ckconstraints, 
constraint);
+
+               /*
+                * XXX If the user says CHECK (IS NOT NULL), should we turn
+                * that into a regular NOT NULL constraint?
+                */
                 break;

I think this was decided against.

+   /*
+    * Copy NOT NULL constraints, too (these do not require any option 
to have
+    * been given).
+    */

Shouldn't that be governed by the INCLUDING CONSTRAINTS option?

Btw., there is some asymmetry here between check constraints and
not-null constraints: Check constraints are in the tuple descriptor,
but not-null constraints are not.  Should that be unified?  Or at
least explained?


* src/include/nodes/parsenodes.h

This comment appears to be outdated:

+ * intermixed in tableElts, and constraints and notnullcols are NIL.  After
+ * parse analysis, tableElts contains just ColumnDefs, notnullcols has been
+ * filled with not-nullable column names from various sources, and 
constraints
+ * contains just Constraint nodes (in fact, only CONSTR_CHECK nodes, in the
+ * present implementation).

There is no "notnullcolls".


* src/test/regress/parallel_schedule

This change appears to be correct, but unrelated to this patch, so I
suggest committing this separately.


* src/test/regress/sql/create_table.sql

-SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 
'part_b'::regclass;
+SELECT conislocal, coninhcount FROM pg_constraint WHERE conrelid = 
'part_b'::regclass ORDER BY coninhcount DESC, conname;

Maybe add conname to the select list here as well, for consistency with 
nearby queries.

On 2023-Jul-03, Peter Eisentraut wrote:

> On 30.06.23 13:44, Alvaro Herrera wrote:
> > OK, so here's a new attempt to get this working correctly.
> 
> Attached is a small fixup patch for the documentation.
> 
> Furthermore, there are a few outdated comments that are probably left over
> from previous versions of this patch set.

Thanks!  I've incorporated your doc fixes and applied fixes for almost
all the other issues you listed; and fixed a couple of additional
issues, such as

* adding a test to regress for an error message that wasn't covered (and
  removed the XXX comment about that)
* remove a pointless variable addition to pg_dump (leftover from a
  previous implementation of constraint capture)
* adapt the sepgsql tests again (we don't recurse to children when
  there's nothing to do, so an object hook invocation doesn't happen
  anymore -- I think)
* made ATExecSetAttNotNull return the constraint address
* more outdated comments adjustment in MergeAttributes

Most importantly, I fixed table creation for LIKE inheritance, as I
described upthread already.

The one thing I have not touched is add ¬_valid to processCASbits()
in gram.y; rather I added a comment that NOT VALID is not yet suported.
I think adding support for that is a reasonably easy on top of this
patch, but since it also requires more pg_dump support and stuff, I'd
rather not mix it in at this point.  The pg_upgrade support is already
quite a house of cards and it drove me crazy.

So, attached is v10.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees."                                      (E. Dijkstra)

I left two questions unanswered here, so here I respond to them while
giving one more revision of the patch.

I realized that the AT_CheckNotNull stuff is now dead code, so in this
version I remove it.  I also changed on heap_getattr to
SysCacheGetAttrNotNull, per a very old review comment from Justin that I
hadn't acted upon.  The other changes are minor code comments and test
adjustments.

At this point I think this is committable.

On 2023-Jul-03, Peter Eisentraut wrote:

> +   /*
> +    * Copy NOT NULL constraints, too (these do not require any option to have
> +    * been given).
> +    */
> 
> Shouldn't that be governed by the INCLUDING CONSTRAINTS option?

To clarify: this is in LIKE, such as 
CREATE TABLE (LIKE someother);
and the reason we don't want to make this behavior depend on INCLUDING
CONSTRAINTS, is backwards compatibility; NOT NULL markings have
traditionally been propagated, so it can be used to create partitions
based on the parent table, and if we made that require the option to be
specified, that would no longer occur in the default case.  Maybe we can
change that behavior, but I'm pretty sure it would be resisted.

> Btw., there is some asymmetry here between check constraints and
> not-null constraints: Check constraints are in the tuple descriptor,
> but not-null constraints are not.  Should that be unified?  Or at
> least explained?

Well, the reason check constraints are in the descriptor, is that they
are needed to verify a table.  NOT NULL constraint as catalog objects
are (at present) only useful from a DDL point of view; they won't change
the underlying implementation, which still depends on just the
attnotnull markings.


-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

In this version I mistakenly included an unwanted change, which broke
the test_ddl_deparse test.  Here's v12 with that removed.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Las mujeres son como hondas:  mientras más resistencia tienen,
 más lejos puedes llegar con ellas"  (Jonas Nightingale, Leap of Faith)

v13, because a conflict was just committed to alter_table.sql.

Here I also call out the relcache.c change by making it a separate
commit.  I'm likely to commit it that way, too.  To recap: the change is
to have a partitioned table's index list include the primary key, even
when said primary key is marked invalid.  This turns out to be necessary
for the currently proposed pg_dump strategy to work; if this is not in
place, attaching the per-partition PK indexes to the parent index fails
because it sees that the columns are not marked NOT NULL.

I don't see any obvious problem with this change; but if someone does
and this turns out to be unacceptable, then the pg_dump stuff would need
some surgery.

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

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Essentially, you're proposing Kevlar shoes as a solution for the problem
that you want to walk around carrying a loaded gun aimed at your foot.
(Tom Lane)

On Wed, 12 Jul 2023 at 18:11, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> v13, because a conflict was just committed to alter_table.sql.
>
> Here I also call out the relcache.c change by making it a separate
> commit.  I'm likely to commit it that way, too.  To recap: the change is
> to have a partitioned table's index list include the primary key, even
> when said primary key is marked invalid.  This turns out to be necessary
> for the currently proposed pg_dump strategy to work; if this is not in
> place, attaching the per-partition PK indexes to the parent index fails
> because it sees that the columns are not marked NOT NULL.
>

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.

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

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.

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.

Regards,
Dean

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@alvh.no-ip.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)

On Thu, 20 Jul 2023 at 16:31, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2023-Jul-13, Dean Rasheed wrote:
>
> > 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?
>

Hmm, I don't particularly like that approach, because I think it will
be difficult to cram any additional details into the table, and also I
don't know whether having multiple not null constraints for a
particular column can be entirely ruled out.

I may well be in the minority here, but I think the best way is to
list them in a separate footer section, in the same way as CHECK
constraints, allowing other constraint properties to be included. So
it might look something like:

\d foo
                Table "public.foo"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           | not null |
 b      | integer |           | not null |
 c      | integer |           | not null |
 d      | integer |           | not null |
Indexes:
    "foo_pkey" PRIMARY KEY, btree (a, b)
Check constraints:
    "foo_a_check" CHECK (a > 0)
    "foo_b_check" CHECK (b > 0) NO INHERIT NOT VALID
Not null constraints:
    "foo_c_not_null" NOT NULL c
    "foo_d_not_null" NOT NULL d NO INHERIT

As for CHECK constraints, the contents of each constraint line would
match the "table_constraint" SQL syntax needed to reconstruct the
constraint. Doing it this way allows for things like NOT VALID and
DEFERRABLE to be added in the future.

I think that's probably too verbose for a plain "\d", but I think it
would be OK with "\d+".

Regards,
Dean

On Thu, 20 Jul 2023 at 16:31, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2023-Jul-13, Dean Rasheed wrote:
>
> > 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.
>

Hmm, I'm not so sure. I think perhaps multiple NOT NULL constraints on
the same column should just be allowed, otherwise things might get
confusing. For example:

create table p1 (a int not null check (a > 0));
create table p2 (a int not null check (a > 0));
create table foo () inherits (p1, p2);

causes foo to have 2 CHECK constraints, but only 1 NOT NULL constraint:

\d foo
                Table "public.foo"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           | not null |
Check constraints:
    "p1_a_check" CHECK (a > 0)
    "p2_a_check" CHECK (a > 0)
Inherits: p1,
          p2

select conname from pg_constraint where conrelid = 'foo'::regclass;
    conname
---------------
 p1_a_not_null
 p2_a_check
 p1_a_check
(3 rows)

which I find a little counter-intuitive / inconsistent. If I then drop
the p1 constraints:

alter table p1 drop constraint p1_a_check;
alter table p1 drop constraint p1_a_not_null;

I end up with column "a" still being not null, and the "p1_a_not_null"
constraint still being there on foo, which seems even more
counter-intuitive, because I just dropped that constraint, and it
really should now be the "p2_a_not_null" constraint that makes "a" not
null:

\d foo
                Table "public.foo"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           | not null |
Check constraints:
    "p2_a_check" CHECK (a > 0)
Inherits: p1,
          p2

select conname from pg_constraint where conrelid = 'foo'::regclass;
    conname
---------------
 p1_a_not_null
 p2_a_check
(2 rows)

I haven't thought through various other cases in any detail, but I
can't help feeling that it would be simpler and more logical /
consistent to just allow multiple NOT NULL constraints on a column,
rather than trying to enforce a rule that only one is allowed. That
way, I think it would be easier for the user to keep track of why a
column is not null.

So I'd say that ALTER TABLE ... ADD NOT NULL should always add a
constraint, even if there already is one. For example ALTER TABLE ...
ADD UNIQUE does nothing to prevent multiple unique constraints on the
same column(s). It seems pretty dumb, but maybe there is a reason to
allow it, and it doesn't feel like we should be second-guessing what
the user wants.

Regards,
Dean

Something else I noticed: the error message from ALTER TABLE ... ADD
CONSTRAINT in the case of a duplicate constraint name is not very
friendly:

ERROR:  duplicate key value violates unique constraint
"pg_constraint_conrelid_contypid_conname_index"
DETAIL:  Key (conrelid, contypid, conname)=(16540, 0, nn) already exists.

To match the error message for other constraint types, this should be:

ERROR:  constraint "nn" for relation "foo" already exists

Regards,
Dean

On 2023-Jul-24, Dean Rasheed wrote:

> Hmm, I don't particularly like that approach, because I think it will
> be difficult to cram any additional details into the table, and also I
> don't know whether having multiple not null constraints for a
> particular column can be entirely ruled out.
> 
> I may well be in the minority here, but I think the best way is to
> list them in a separate footer section, in the same way as CHECK
> constraints, allowing other constraint properties to be included. So
> it might look something like:

That's the first thing I proposed actually.  I got one vote down from
Robert Haas[1], but while the idea seems to have had support from Justin
Pryzby (in \dt++) [2] and definitely did from Peter Eisentraut [3], I do
not like it too much myself, mainly because the partition list has a
very similar treatment and I find that one an annoyance.

> and also I don't know whether having multiple not null constraints for
> a particular column can be entirely ruled out.

I had another look at the standard.  In 11.26 (<drop table
constraint definition>) it says that "If [the constraint being removed]
causes some column COL to be known not nullable and no other constraint
causes COL to be known not nullable, then the nullability characteristic
of the column descriptor of COL is changed to possibly nullable".  Which
supports the idea that there might be multiple such constraints.
(However, we could also read this as meaning that the PK could be one
such constraint while NOT NULL is another one.)

However, 11.16 (<drop column not null clause> as part of 11.12 <alter
column definition>), says that DROP NOT NULL causes the indication of
the column as NOT NULL to be removed.  This, to me, says that if you do
have multiple such constraints, you'd better remove them all with that
command.  All in all, I lean towards allowing just one as best as we
can.

[1] https://postgr.es/m/CA+Tgmobnoxt83y1QesBNVArhFm-fLwWkDUyiV84e+psayDwB7A@mail.gmail.com
[2] https://postgr.es/m/20230301223214.GC4268%40telsasoft.com
[3] https://postgr.es/m/1c4f3755-2d10-cae9-647f-91a9f006410e%40enterprisedb.com

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
“Cuando no hay humildad las personas se degradan” (A. Christie)

On 2023-Jul-24, Dean Rasheed wrote:

> Hmm, I'm not so sure. I think perhaps multiple NOT NULL constraints on
> the same column should just be allowed, otherwise things might get
> confusing. For example:
> 
 create table p1 (a int not null check (a > 0));
 create table p2 (a int not null check (a > 0));
 create table foo () inherits (p1, p2);

Have a look at the conislocal / coninhcount values.  These should
reflect the fact that the constraint has multiple sources; and the
constraint does disappear if you drop it from both sources.

> If I then drop the p1 constraints:
> 
> alter table p1 drop constraint p1_a_check;
> alter table p1 drop constraint p1_a_not_null;
> 
> I end up with column "a" still being not null, and the "p1_a_not_null"
> constraint still being there on foo, which seems even more
> counter-intuitive, because I just dropped that constraint, and it
> really should now be the "p2_a_not_null" constraint that makes "a" not
> null:

I can see that it might make sense to not inherit the constraint name in
some cases.  Perhaps:

1. never inherit a name.  Each table has its own constraint name always
2. only inherit if there's a single parent
3. always inherit the name from the first parent (current implementation)

> So I'd say that ALTER TABLE ... ADD NOT NULL should always add a
> constraint, even if there already is one. For example ALTER TABLE ...
> ADD UNIQUE does nothing to prevent multiple unique constraints on the
> same column(s). It seems pretty dumb, but maybe there is a reason to
> allow it, and it doesn't feel like we should be second-guessing what
> the user wants.

That was my initial implementation but I changed it to allowing a single
constraint because of the way the standard describes SET NOT NULL;
specifically, 11.15 <set column not null clause> says that "If the
column descriptor of C does not contain an indication that C is defined
as NOT NULL, then:" a constraint is added; otherwise (i.e., such an
indication does exist), nothing happens.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"La virtud es el justo medio entre dos defectos" (Aristóteles)

On 2023-Jul-24, Dean Rasheed wrote:

> Something else I noticed: the error message from ALTER TABLE ... ADD
> CONSTRAINT in the case of a duplicate constraint name is not very
> friendly:
> 
> ERROR:  duplicate key value violates unique constraint
> "pg_constraint_conrelid_contypid_conname_index"
> DETAIL:  Key (conrelid, contypid, conname)=(16540, 0, nn) already exists.
> 
> To match the error message for other constraint types, this should be:
> 
> ERROR:  constraint "nn" for relation "foo" already exists

Hmm, how did you get this one?  I can't reproduce it:

55490 17devel 3166154=# create table foo (a int constraint nn not null);
CREATE TABLE
55490 17devel 3166154=# alter table foo add constraint nn not null a;
ERROR:  column "a" of table "foo" is already NOT NULL

55490 17devel 3166154=# drop table foo;
DROP TABLE

55490 17devel 3166154=# create table foo (a int);
CREATE TABLE
Duración: 1,472 ms
55490 17devel 3166154=# alter table foo add constraint nn not null a, add constraint nn not null a;
ERROR:  column "a" of table "foo" is already NOT NULL

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

Hello,

While discussing the matter of multiple constraints with Vik Fearing, I
noticed that we were throwing an unnecessary error if you used

CREATE TABLE foo (a int NOT NULL NOT NULL);

That would die with "redundant NOT NULL declarations", but current
master doesn't do that; and we don't do it for UNIQUE UNIQUE either.
So I modified the patch to make it ignore the dupe and create a single
constraint.  This (and rebasing to current master) are the only changes
in v15.

I have not changed the psql presentation, but I'll do as soon as we have
rough consensus on what to do.  To reiterate, the options are:

1. Don't show the constraint names.  This is what the current patch does

2. Show the constraint name in \d+ in the "nullable" column.
   I did this early on, to much booing.

3. Show the constraint name in \d++ (a new command) tabular output

4. Show the constraint name in the footer of \d+
   I also did this at some point; there are some +1s and some -1s.

5. Show the constraint name in the footer of \d++


Many thanks, Dean, for the discussion so far.

-- 
Álvaro Herrera         PostgreSQL Developer  —  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)

On 2023-Jul-24, Alvaro Herrera wrote:

> That would die with "redundant NOT NULL declarations", but current
> master doesn't do that; and we don't do it for UNIQUE UNIQUE either.
> So I modified the patch to make it ignore the dupe and create a single
> constraint.  This (and rebasing to current master) are the only changes
> in v15.

Did I forget the attachments once more?  Yup, I did.  Here they are.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On 7/24/23 18:42, Alvaro Herrera wrote:
> 55490 17devel 3166154=# create table foo (a int constraint nn not null);
> CREATE TABLE
> 55490 17devel 3166154=# alter table foo add constraint nn not null a;
> ERROR:  column "a" of table "foo" is already NOT NULL

Surely this should be a WARNING or INFO?  I see no reason to ERROR here.
-- 
Vik Fearing

On Mon, 24 Jul 2023 at 17:42, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2023-Jul-24, Dean Rasheed wrote:
>
> > Something else I noticed: the error message from ALTER TABLE ... ADD
> > CONSTRAINT in the case of a duplicate constraint name is not very
> > friendly:
> >
> > ERROR:  duplicate key value violates unique constraint
> > "pg_constraint_conrelid_contypid_conname_index"
> > DETAIL:  Key (conrelid, contypid, conname)=(16540, 0, nn) already exists.
> >

To reproduce this error, try to create 2 constraints with the same
name on different columns:

create table foo(a int, b int);
alter table foo add constraint nn not null a;
alter table foo add constraint nn not null b;

I found another, separate issue:

create table p1(a int not null);
create table p2(a int);
create table foo () inherits (p1,p2);
alter table p2 add not null a;

ERROR:  column "a" of table "foo" is already NOT NULL

whereas doing "alter table p2 alter column a set not null" works OK,
merging the constraints as expected.

Regards,
Dean

On Mon, Jul 24, 2023 at 6:33 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> That's the first thing I proposed actually.  I got one vote down from
> Robert Haas[1], but while the idea seems to have had support from Justin
> Pryzby (in \dt++) [2] and definitely did from Peter Eisentraut [3], I do
> not like it too much myself, mainly because the partition list has a
> very similar treatment and I find that one an annoyance.

I think I might want to retract my earlier -1 vote. I mean, I agree
with former me that having the \d+ output get a whole lot longer is
not super-appealing. But I also agree with Dean that having this
information available somewhere is probably important, and I also
agree with your point that inventing \d++ for this isn't necessarily a
good idea. I fear that will just result in having to type an extra
plus sign any time you want to see all of the table details, to make
sure that psql knows that you really mean it. So, maybe showing it in
the \d+ output as Dean proposes is the least of evils.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2023-Jul-24, Robert Haas wrote:

> I think I might want to retract my earlier -1 vote. I mean, I agree
> with former me that having the \d+ output get a whole lot longer is
> not super-appealing. But I also agree with Dean that having this
> information available somewhere is probably important, and I also
> agree with your point that inventing \d++ for this isn't necessarily a
> good idea. I fear that will just result in having to type an extra
> plus sign any time you want to see all of the table details, to make
> sure that psql knows that you really mean it. So, maybe showing it in
> the \d+ output as Dean proposes is the least of evils.

Okay then, I've made these show up in the footer of \d+.  This is in
patch 0003 here.  Please let me know what do you think of the regression
changes.

On 2023-Jul-24, Dean Rasheed wrote:

> To reproduce this error, try to create 2 constraints with the same
> name on different columns:
> 
> create table foo(a int, b int);
> alter table foo add constraint nn not null a;
> alter table foo add constraint nn not null b;

Ah, of course.  Fixed.

> I found another, separate issue:
> 
> create table p1(a int not null);
> create table p2(a int);
> create table foo () inherits (p1,p2);
> alter table p2 add not null a;
> 
> ERROR:  column "a" of table "foo" is already NOT NULL
> 
> whereas doing "alter table p2 alter column a set not null" works OK,
> merging the constraints as expected.

True.  I made it a non-error.  I initially changed the message to INFO,
as suggested by Vik nearby; but after noticing that SET NOT NULL just
does the same thing with no message, I removed this message altogether,
for consistence.  Now that I did it, though, I wonder: if the user
specified a constraint name, and that name does not match the existing
constraint, maybe we should have an INFO or NOTICE or WARNING message
that the requested constraint name was not satisfied.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On Tue, Jul 25, 2023 at 8:36 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Okay then, I've made these show up in the footer of \d+.  This is in
> patch 0003 here.  Please let me know what do you think of the regression
> changes.

Seems OK.

I'm not really thrilled with the idea of every not-null constraint
having a name, to be honest. Of all the kinds of constraints that we
have in the system, NOT NULL constraints are probably the ones where
naming them is least likely to be interesting, because they don't
really have any interesting properties. A CHECK constraint has an
expression; a foreign key constraint has columns that it applies to on
each side plus the identity of the table and opclass information, but
a NOT NULL constraint seems like it can never have any property other
than which column. So it sort of seems like a waste to name it. But if
we want it catalogued then we don't really have an option, so I
suppose we just have to accept a bit of clutter as the price of doing
business.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2023-Jul-25, Isaac Morland wrote:

> I agree. I definitely do *not* want a bunch of NOT NULL constraint names
> cluttering up displays. Can we legislate that all NOT NULL implementing
> constraints are named by mashing together the table name, column name, and
> something to identify it as a NOT NULL constraint?

All constraints are named like that already, and NOT NULL constraints
just inherited the same idea.  The names are <table>_<column>_not_null
for NOT NULL constraints.  pg_dump goes great lengths to avoid printing
constraint names when they have this pattern.

I do not want these constraint names cluttering the output either.
That's why I propose moving them to a new \d++ command, where they will
only bother you if you absolutely need them.  But so far I have only one
vote supporting that idea.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Tue, Jul 25, 2023 at 1:33 PM Isaac Morland <isaac.morland@gmail.com> wrote:
> My suggestion is for \d+ to show NOT NULL constraints only if there is something weird going on (wrong name,
duplicateconstraints, …). If there is nothing weird about the constraint then explicitly listing it provides absolutely
noinformation that is not given by "not null" in the "Nullable" column. Easier said than done I suppose. I'm just
worriedabout my \d+ displays becoming less useful. 

I mean, the problem is that if you want to ALTER TABLE .. DROP
CONSTRAINT, you need to know what the valid arguments to that command
are, and the names of these constraints will be just as valid as the
names of any other constraints.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, Jul 25, 2023 at 3:07 PM Isaac Morland <isaac.morland@gmail.com> wrote:
> OK, I suppose ALTER CONSTRAINT to change the deferrable status and validity (that is why we're doing this, right?)
needsthe constraint name. But the constraint name is formulaic by default, and my proposal is to suppress it only when
itmatches the formula, so you could just construct the constraint name using the documented formula if it's not
explicitlylisted. 
>
> I really don’t see it as a good use of space to add n lines to the \d+ display just to confirm that the "not null"
designationsin the "Nullable" column are implemented by named constraints with the expected names. 

Yeah, I mean, I get that. That was my initial concern, too. But I also
think if there's some complicated rule that determines what gets
displayed and what doesn't, nobody's going to remember it, and then
when you don't see something, you're never going to be sure exactly
what's going on. Displaying everything is going to be clunky
especially if, like me, you tend to be careful to mark columns NOT
NULL when they are, but when something goes wrong, the last thing you
want to do is run a \d command and have it show you incomplete
information.

I can't count the number of times that somebody's shown me the output
of a query against pg_locks or pg_stat_activity that had been filtered
to remove irrelevant information and it turned out that the hidden
information was not so irrelevant as the person who wrote the query
thought. It happens all the time. I don't want to create the same kind
of situation here.

--
Robert Haas
EDB: http://www.enterprisedb.com

On Tue, 25 Jul 2023 at 13:36, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Okay then, I've made these show up in the footer of \d+.  This is in
> patch 0003 here.  Please let me know what do you think of the regression
> changes.
>

The new \d+ output certainly makes testing and reviewing easier,
though I do understand people's concerns that this may make the output
significantly longer in many real-world cases. I don't think it would
be a good idea to filter the list in any way though, because I think
that will only lead to confusion. I think it should be all-or-nothing,
though I'm not necessarily opposed to using something like \d++ to
enable it, if that turns out to be the least-bad option.

Going back to this example:

drop table if exists p1, p2, foo;
create table p1(a int not null check (a > 0));
create table p2(a int not null check (a > 0));
create table foo () inherits (p1,p2);
\d+ foo

                                           Table "public.foo"
 Column |  Type   | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
 a      | integer |           | not null |         | plain   |
    |              |
Check constraints:
    "p1_a_check" CHECK (a > 0)
    "p2_a_check" CHECK (a > 0)
Not null constraints:
    "p1_a_not_null" NOT NULL "a" (inherited)
Inherits: p1,
          p2
Access method: heap

I remain of the opinion that that should create 2 NOT NULL constraints
on foo, for consistency with CHECK constraints, and the misleading
name that results if p1_a_not_null is dropped from p1. That way, the
names of inherited NOT NULL constraints could be kept in sync, as they
are for other constraint types, making it easier to keep track of
where they come from, and it wouldn't be necessary to treat them
differently (e.g., matching by column number, when dropping NOT NULL
constraints).

Doing a little more testing, I found some other issues.


Given the following sequence:

drop table if exists p,c;
create table p(a int primary key);
create table c() inherits (p);
alter table p drop constraint p_pkey;

p.a ends up being nullable, where previously it would have been left
non-nullable. That change makes sense, and is presumably one of the
benefits of tying the nullability of columns to pg_constraint entries.
However, c.a remains non-nullable, with a NOT NULL constraint that
claims to be inherited:

\d+ c
                                            Table "public.c"
 Column |  Type   | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
 a      | integer |           | not null |         | plain   |
    |              |
Not null constraints:
    "c_a_not_null" NOT NULL "a" (inherited)
Inherits: p
Access method: heap

That's a problem, because now the NOT NULL constraint on c cannot be
dropped (attempting to drop it on c errors out because it thinks it's
inherited, but it can't be dropped via p, because p.a is already
nullable).

I wonder if NOT NULL constraints created as a result of inherited PKs
should have names based on the PK name (e.g.,
<PK_name>_<col_name>_not_null), to make it more obvious where they
came from. That would be more consistent with the way NOT NULL
constraint names are inherited.


Given the following sequence:

drop table if exists p,c;
create table p(a int);
create table c() inherits (p);
alter table p add primary key (a);

c.a ends up non-nullable, but there is no pg_constraint entry
enforcing the constraint:

\d+ c
                                            Table "public.c"
 Column |  Type   | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
 a      | integer |           | not null |         | plain   |
    |              |
Inherits: p
Access method: heap


Given a database containing these 2 tables:

create table p(a int primary key);
create table c() inherits (p);

doing a pg_dump and restore fails to restore the NOT NULL constraint
on c, because all constraints created by the dump are local to p.


That's it for now. I'll try to do more testing later.

Regards,
Dean

Thanks for spending so much time with this patch -- really appreciated.

On 2023-Jul-26, Dean Rasheed wrote:

> On Tue, 25 Jul 2023 at 13:36, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > Okay then, I've made these show up in the footer of \d+.  This is in
> > patch 0003 here.  Please let me know what do you think of the regression
> > changes.
> 
> The new \d+ output certainly makes testing and reviewing easier,
> though I do understand people's concerns that this may make the output
> significantly longer in many real-world cases. I don't think it would
> be a good idea to filter the list in any way though, because I think
> that will only lead to confusion. I think it should be all-or-nothing,
> though I'm not necessarily opposed to using something like \d++ to
> enable it, if that turns out to be the least-bad option.

Yeah, at this point I'm inclined to get the \d+ version committed
immediately after the main patch, and we can tweak the psql UI after the
fact -- for instance so that they are only shown in \d++, or some other
idea we may come across.

> Going back to this example:
> 
> drop table if exists p1, p2, foo;
> create table p1(a int not null check (a > 0));
> create table p2(a int not null check (a > 0));
> create table foo () inherits (p1,p2);

> I remain of the opinion that that should create 2 NOT NULL constraints
> on foo, for consistency with CHECK constraints, and the misleading
> name that results if p1_a_not_null is dropped from p1. That way, the
> names of inherited NOT NULL constraints could be kept in sync, as they
> are for other constraint types, making it easier to keep track of
> where they come from, and it wouldn't be necessary to treat them
> differently (e.g., matching by column number, when dropping NOT NULL
> constraints).

I think having two constraints is more problematic, UI-wise.  Previous
versions of this patchset did it that way, and it's not great: for
example ALTER TABLE ALTER COLUMN DROP NOT NULL fails and tells you to
choose which exact constraint you want to drop and use DROP CONSTRAINT
instead.  And when searching for the not-null constraints for a column,
the code had to consider the case of there being multiple ones, which
led to strange contortions.  Allowing a single one is simpler and covers
all important cases well.

Anyway, you still can't drop the doubly-inherited constraint directly,
because it'll complain that it is an inherited constraint.  So you have
to deinherit first and only then can you drop the constraint.

Now, one possible improvement here would be to ignore the parent
constraint's name, and have 'foo' recompute its own constraint name from
scratch, inheriting the name only if one of the parents had a
manually-specified constraint name (and we would choose the first one,
if there's more than one).  I think complicating things more than that
is unnecessary -- particularly considering that legacy inheritance is,
well, legacy, and I doubt people are relying too much on it.


> Given the following sequence:
> 
> drop table if exists p,c;
> create table p(a int primary key);
> create table c() inherits (p);
> alter table p drop constraint p_pkey;
> 
> p.a ends up being nullable, where previously it would have been left
> non-nullable. That change makes sense, and is presumably one of the
> benefits of tying the nullability of columns to pg_constraint entries.

Right.

> However, c.a remains non-nullable, with a NOT NULL constraint that
> claims to be inherited:
> 
> \d+ c
>                                             Table "public.c"
>  Column |  Type   | Collation | Nullable | Default | Storage |
> Compression | Stats target | Description
> --------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
>  a      | integer |           | not null |         | plain   |
>     |              |
> Not null constraints:
>     "c_a_not_null" NOT NULL "a" (inherited)
> Inherits: p
> Access method: heap
> 
> That's a problem, because now the NOT NULL constraint on c cannot be
> dropped (attempting to drop it on c errors out because it thinks it's
> inherited, but it can't be dropped via p, because p.a is already
> nullable).

Oh, I think the bug here is just that this constraint should not claim
to be inherited, but standalone.  So you can drop it afterwards; but if
you drop it and end up with NULL values in your PK-labelled column in
the parent table, that's on you.

> I wonder if NOT NULL constraints created as a result of inherited PKs
> should have names based on the PK name (e.g.,
> <PK_name>_<col_name>_not_null), to make it more obvious where they
> came from. That would be more consistent with the way NOT NULL
> constraint names are inherited.

Hmm, interesting idea.  I'll play with it.  (It may quickly lead to
constraint names that are too long, though.)

> Given the following sequence:
> 
> drop table if exists p,c;
> create table p(a int);
> create table c() inherits (p);
> alter table p add primary key (a);
> 
> c.a ends up non-nullable, but there is no pg_constraint entry
> enforcing the constraint:
> 
> \d+ c
>                                             Table "public.c"
>  Column |  Type   | Collation | Nullable | Default | Storage |
> Compression | Stats target | Description
> --------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
>  a      | integer |           | not null |         | plain   |
>     |              |
> Inherits: p
> Access method: heap

Oh, this one's a bad omission.  I'll fix it.


> Given a database containing these 2 tables:
> 
> create table p(a int primary key);
> create table c() inherits (p);
> 
> doing a pg_dump and restore fails to restore the NOT NULL constraint
> on c, because all constraints created by the dump are local to p.

Strange.  I'll see about fixing this one too.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)

On 2023-Jul-26, Alvaro Herrera wrote:

> On 2023-Jul-26, Dean Rasheed wrote:
> 
> > The new \d+ output certainly makes testing and reviewing easier,
> > though I do understand people's concerns that this may make the output
> > significantly longer in many real-world cases.
> 
> Yeah, at this point I'm inclined to get the \d+ version committed
> immediately after the main patch, and we can tweak the psql UI after the
> fact -- for instance so that they are only shown in \d++, or some other
> idea we may come across.

(For example, maybe we could add \dtc [PATTERN] or some such, that lists
all the constraints of all kinds in tables matching PATTERN.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"If you want to have good ideas, you must have many ideas.  Most of them
will be wrong, and what you have to learn is which ones to throw away."
                                                         (Linus Pauling)

> > Given the following sequence:
> > 
> > drop table if exists p,c;
> > create table p(a int primary key);
> > create table c() inherits (p);
> > alter table p drop constraint p_pkey;

> > However, c.a remains non-nullable, with a NOT NULL constraint that
> > claims to be inherited:
> > 
> > \d+ c
> >                                             Table "public.c"
> >  Column |  Type   | Collation | Nullable | Default | Storage |
> > Compression | Stats target | Description
> > --------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
> >  a      | integer |           | not null |         | plain   |
> >     |              |
> > Not null constraints:
> >     "c_a_not_null" NOT NULL "a" (inherited)
> > Inherits: p
> > Access method: heap
> > 
> > That's a problem, because now the NOT NULL constraint on c cannot be
> > dropped (attempting to drop it on c errors out because it thinks it's
> > inherited, but it can't be dropped via p, because p.a is already
> > nullable).

So I implemented a fix for this (namely: fix the inhcount to be 0
initially), and it works well, but it does cause a definitional problem:
any time we create a child table that inherits from another table that
has a primary key, all the columns in the child table will get normal,
visible, droppable NOT NULL constraints.  Thus, pg_dump for example will
output that constraint exactly as if the user had specified it in the
child's CREATE TABLE command.  By itself this doesn't bother me, though
I admit it seems a little odd.

When you restore such a setup from pg_dump, things work perfectly -- I
mean, you don't get a second constraint.  But if you do drop the
constraint, then it will be reinstated by the next pg_dump as if you
hadn't dropped it, by way of it springing to life from the PK.

To avoid that, one option would be to make this NN constraint
undroppable ...  but I don't see how.  One option might be to add a
pg_depend row that links the NOT NULL constraint to its PK constraint.
But this will be a strange case that occurs nowhere else, since other
NOT NULL constraint don't have such pg_depend rows.  Also, I won't know
how pg_dump likes this until I implement it.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 24.07.23 12:32, Alvaro Herrera wrote:
> However, 11.16 (<drop column not null clause> as part of 11.12 <alter
> column definition>), says that DROP NOT NULL causes the indication of
> the column as NOT NULL to be removed.  This, to me, says that if you do
> have multiple such constraints, you'd better remove them all with that
> command.  All in all, I lean towards allowing just one as best as we
> can.

Another clue is in 11.15 <set column not null clause>, which says

     1) Let C be the column identified by the <column name> CN in the
     containing <alter column definition>. If the column descriptor of C
     does not contain an indication that C is defined as NOT NULL, then:

     [do things]

Otherwise it does nothing.  So there can only be one such constraint per 
table.

On 2023-Jul-28, Alvaro Herrera wrote:

> To avoid that, one option would be to make this NN constraint
> undroppable ...  but I don't see how.  One option might be to add a
> pg_depend row that links the NOT NULL constraint to its PK constraint.
> But this will be a strange case that occurs nowhere else, since other
> NOT NULL constraint don't have such pg_depend rows.  Also, I won't know
> how pg_dump likes this until I implement it.

I've been completing the implementation for this.  It seems to work
reasonably okay; pg_dump requires somewhat strange contortions, but they
are similar to what we do in flagInhTables already, so I don't feel too
bad about that.

What *is* odd and bothersome is that it also causes a problem dropping
the child table.  For example,

CREATE TABLE parent (a int primary key);
CREATE TABLE child () INHERITS (parent);
\d+ child

                                                 Tabla «public.child»
 Columna │  Tipo   │ Ordenamiento │ Nulable  │ Por omisión │ Almacenamiento │ Compresión │ Estadísticas │ Descripción 
─────────┼─────────┼──────────────┼──────────┼─────────────┼────────────────┼────────────┼──────────────┼─────────────
 a       │ integer │              │ not null │             │ plain          │            │              │ 
Not null constraints:
    "child_a_not_null" NOT NULL "a"
Hereda: parent
Método de acceso: heap

This is the behavior that I think we wanted to prevent drop of the child
constraint, and it seems okay to me:

=# alter table child drop constraint child_a_not_null;
ERROR:  cannot drop constraint child_a_not_null on table child because constraint parent_pkey on table parent requires
it
SUGERENCIA:  You can drop constraint parent_pkey on table parent instead.

But the problem is this:

=# drop table child;
ERROR:  cannot drop table child because other objects depend on it
DETALLE:  constraint parent_pkey on table parent depends on table child
SUGERENCIA:  Use DROP ... CASCADE to drop the dependent objects too.


To be clear, what my patch is doing is add one new dependency:

                    dep                     │                  ref                   │ deptype 
────────────────────────────────────────────┼────────────────────────────────────────┼─────────
 type foo                                   │ table foo                              │ i
 table foo                                  │ schema public                          │ n
 constraint foo_pkey on table foo           │ column a of table foo                  │ a
 type bar                                   │ table bar                              │ i
 table bar                                  │ schema public                          │ n
 table bar                                  │ table foo                              │ n
 constraint bar_a_not_null on table bar     │ column a of table bar                  │ a
 constraint child_a_not_null on table child │ column a of table child                │ a
 constraint child_a_not_null on table child │ constraint parent_pkey on table parent │ i

the last row here is what is new.  I'm not sure what's the right fix.
Maybe I need to invert the direction of that dependency.


Even with that fixed, I'd still need to write more code so that ALTER
TABLE INHERIT adds the link (I already patched the DROP INHERIT part).
Not sure what else might I be missing.

Separately, I also noticed that some code that's currently
dropconstraint_internal needs to be moved to DropConstraintById, because
if the PK is dropped for some other reason than ALTER TABLE DROP
CONSTRAINT, some ancillary actions are not taken.

Sigh.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
 Are you not unsure you want to delete Firefox?
       [Not unsure]     [Not not unsure]    [Cancel]
                   http://smylers.hates-software.com/2008/01/03/566e45b2.html

On Fri, 4 Aug 2023 at 19:10, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2023-Jul-28, Alvaro Herrera wrote:
>
> > To avoid that, one option would be to make this NN constraint
> > undroppable ...  but I don't see how.  One option might be to add a
> > pg_depend row that links the NOT NULL constraint to its PK constraint.
> > But this will be a strange case that occurs nowhere else, since other
> > NOT NULL constraint don't have such pg_depend rows.  Also, I won't know
> > how pg_dump likes this until I implement it.
>
> I've been completing the implementation for this.  It seems to work
> reasonably okay; pg_dump requires somewhat strange contortions, but they
> are similar to what we do in flagInhTables already, so I don't feel too
> bad about that.
>
> What *is* odd and bothersome is that it also causes a problem dropping
> the child table.
>

Hmm, thinking about this some more, I think this might be the wrong
approach to fixing the original problem. I think it was probably OK
that the NOT NULL constraint on the child was marked as inherited, but
I think what should have happened is that dropping the PRIMARY KEY
constraint on the parent should have caused the NOT NULL constraint on
the child to have been deleted (in the same way as it would have been,
if it had been a NOT NULL constraint on the parent).

Regards,
Dean

On 2023-Aug-05, Dean Rasheed wrote:

> Hmm, thinking about this some more, I think this might be the wrong
> approach to fixing the original problem. I think it was probably OK
> that the NOT NULL constraint on the child was marked as inherited, but
> I think what should have happened is that dropping the PRIMARY KEY
> constraint on the parent should have caused the NOT NULL constraint on
> the child to have been deleted (in the same way as it would have been,
> if it had been a NOT NULL constraint on the parent).

Yeah, something like that.  However, if the child had a NOT NULL
constraint of its own, then it should not be deleted when the
PK-on-parent is, but merely marked as no longer inherited.  (This is
also what happens with a straight NOT NULL constraint.)  I think what
this means is that at some point during the deletion of the PK we must
remove the dependency link rather than letting it be followed.  I'm not
yet sure how to do this.

Anyway, I was at the same time fixing the other problem you reported
with inheritance (namely, adding a PK ends up with the child column
being marked NOT NULL but no corresponding constraint).

At some point I wondered if the easy way out wouldn't be to give up on
the idea that creating a PK causes the child columns to be marked
not-nullable.  However, IIRC I decided against that because it breaks
restoring of old dumps, so it wouldn't be acceptable.

To make matters worse: pg_dump creates the PK as 

  ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... )

note the ONLY there.  It seems I'm forced to cause the PK to affect
children even though ONLY is given.  This is undesirable but I don't see
a way out of that.

It is all a bit of a rat's nest.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Nunca se desea ardientemente lo que solo se desea por razón" (F. Alexandre)

On Sat, 5 Aug 2023 at 18:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Yeah, something like that.  However, if the child had a NOT NULL
> constraint of its own, then it should not be deleted when the
> PK-on-parent is, but merely marked as no longer inherited.  (This is
> also what happens with a straight NOT NULL constraint.)  I think what
> this means is that at some point during the deletion of the PK we must
> remove the dependency link rather than letting it be followed.  I'm not
> yet sure how to do this.
>

I'm not sure that adding that new dependency was the right thing to
do. I think perhaps this could just be made to work using conislocal
and coninhcount to track whether the child constraint needs to be
deleted, or just updated.

> Anyway, I was at the same time fixing the other problem you reported
> with inheritance (namely, adding a PK ends up with the child column
> being marked NOT NULL but no corresponding constraint).
>
> At some point I wondered if the easy way out wouldn't be to give up on
> the idea that creating a PK causes the child columns to be marked
> not-nullable.  However, IIRC I decided against that because it breaks
> restoring of old dumps, so it wouldn't be acceptable.
>
> To make matters worse: pg_dump creates the PK as
>
>   ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... )
>
> note the ONLY there.  It seems I'm forced to cause the PK to affect
> children even though ONLY is given.  This is undesirable but I don't see
> a way out of that.
>
> It is all a bit of a rat's nest.
>

I wonder if that could be made to work in the same way as inherited
CHECK constraints -- dump the child's inherited NOT NULL constraints,
and then manually update conislocal in pg_constraint.

Regards,
Dean

On 05.08.23 21:50, Dean Rasheed wrote:
>> Anyway, I was at the same time fixing the other problem you reported
>> with inheritance (namely, adding a PK ends up with the child column
>> being marked NOT NULL but no corresponding constraint).
>>
>> At some point I wondered if the easy way out wouldn't be to give up on
>> the idea that creating a PK causes the child columns to be marked
>> not-nullable.  However, IIRC I decided against that because it breaks
>> restoring of old dumps, so it wouldn't be acceptable.
>>
>> To make matters worse: pg_dump creates the PK as
>>
>>    ALTER TABLE ONLY parent ADD PRIMARY KEY ( ... )
>>
>> note the ONLY there.  It seems I'm forced to cause the PK to affect
>> children even though ONLY is given.  This is undesirable but I don't see
>> a way out of that.
>>
>> It is all a bit of a rat's nest.
>>
> 
> I wonder if that could be made to work in the same way as inherited
> CHECK constraints -- dump the child's inherited NOT NULL constraints,
> and then manually update conislocal in pg_constraint.

I wonder whether the root of these problems is that we mix together 
primary key constraints and not-null constraints.  I understand that 
right now, with the proposed patch, when a table inherits from a parent 
table with a primary key constraint, we generate not-null constraints on 
the child, in order to enforce the not-nullness.  What if we did 
something like this instead: In the child table, we don't generate a 
not-null constraint, but instead a primary key constraint entry.  But we 
mark the primary key constraint somehow to say, this is just for the 
purpose of inheritance, don't enforce uniqueness, but enforce 
not-nullness.  Would that work?

On 2023-Aug-09, Peter Eisentraut wrote:

> I wonder whether the root of these problems is that we mix together primary
> key constraints and not-null constraints.  I understand that right now, with
> the proposed patch, when a table inherits from a parent table with a primary
> key constraint, we generate not-null constraints on the child, in order to
> enforce the not-nullness.  What if we did something like this instead: In
> the child table, we don't generate a not-null constraint, but instead a
> primary key constraint entry.  But we mark the primary key constraint
> somehow to say, this is just for the purpose of inheritance, don't enforce
> uniqueness, but enforce not-nullness.  Would that work?

Hmm.  One table can have many parents, and many of them can have primary
keys.  If we tried to model it the way you suggest, the child table
would need to have several primary keys.  I don't think this would work.

But I think I just need to stare at the dependency graph a little while
longer.  Maybe I just need to add some extra edges to make it work
correctly.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On 2023-Aug-05, Dean Rasheed wrote:

> On Sat, 5 Aug 2023 at 18:37, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >
> > Yeah, something like that.  However, if the child had a NOT NULL
> > constraint of its own, then it should not be deleted when the
> > PK-on-parent is, but merely marked as no longer inherited.  (This is
> > also what happens with a straight NOT NULL constraint.)  I think what
> > this means is that at some point during the deletion of the PK we must
> > remove the dependency link rather than letting it be followed.  I'm not
> > yet sure how to do this.
> 
> I'm not sure that adding that new dependency was the right thing to
> do. I think perhaps this could just be made to work using conislocal
> and coninhcount to track whether the child constraint needs to be
> deleted, or just updated.

Right, in the end I got around to that point of view.  I abandoned the
idea of adding these dependency links, and I'm back at relying on the
coninhcount/conislocal markers.  But there were a couple of bugs in the
accounting for that, so I've fixed some of those, but it's not yet
complete:

- ALTER TABLE parent ADD PRIMARY KEY
  needs to create NOT NULL constraints in children.  I added this, but
  I'm not yet sure it works correctly (for example, if a child already
  has a NOT NULL constraint, we need to bump its inhcount, but we
  don't.)
- ALTER TABLE parent ADD PRIMARY KEY USING index
  Not sure if this is just as above or needs separate handling
- ALTER TABLE DROP PRIMARY KEY
  needs to decrement inhcount or drop the constraint if there are no
  other sources for that constraint to exist.  I've adjusted the drop
  constraint code to do this.
- ALTER TABLE INHERIT
  needs to create a constraint on the new child, if parent has PK. Not
  implemented
- ALTER TABLE NO INHERIT
  needs to delink any constraints (decrement inhcount, possibly drop
  the constraint).

I also need to add tests for those scenarios, because I think there
aren't any for most of them.

There's also another a pg_upgrade problem: we now get spurious ALTER
TABLE SET NOT NULL commands in a dump after pg_upgrade for the columns
that get the constraint from a primary key.  (This causes a pg_upgrade
test failure).  I need to adjust pg_dump to suppress those; I think
something like flagInhTables would do.

(I had mentioned that I needed to move code from dropconstraint_internal
to RemoveConstraintById.  However, now I can't figure out exactly what
case was having a problem, so I've left it alone.)

Here's v17, which is a step forward, but several holes remain.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"I can't go to a restaurant and order food because I keep looking at the
fonts on the menu.  Five minutes later I realize that it's also talking
about food" (Donald Knuth)

On Fri, 11 Aug 2023 at 14:54, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> Right, in the end I got around to that point of view.  I abandoned the
> idea of adding these dependency links, and I'm back at relying on the
> coninhcount/conislocal markers.  But there were a couple of bugs in the
> accounting for that, so I've fixed some of those, but it's not yet
> complete:
>
> - ALTER TABLE parent ADD PRIMARY KEY
>   needs to create NOT NULL constraints in children.  I added this, but
>   I'm not yet sure it works correctly (for example, if a child already
>   has a NOT NULL constraint, we need to bump its inhcount, but we
>   don't.)
> - ALTER TABLE parent ADD PRIMARY KEY USING index
>   Not sure if this is just as above or needs separate handling
> - ALTER TABLE DROP PRIMARY KEY
>   needs to decrement inhcount or drop the constraint if there are no
>   other sources for that constraint to exist.  I've adjusted the drop
>   constraint code to do this.
> - ALTER TABLE INHERIT
>   needs to create a constraint on the new child, if parent has PK. Not
>   implemented
> - ALTER TABLE NO INHERIT
>   needs to delink any constraints (decrement inhcount, possibly drop
>   the constraint).
>

I think perhaps for ALTER TABLE INHERIT, it should check that the
child has a NOT NULL constraint, and error out if not. That's the
current behaviour, and also matches other constraints types (e.g.,
CHECK constraints).

More generally though, I'm worried that this is starting to get very
complicated. I wonder if there might be a different, simpler approach.
One vague idea is to have a new attribute on the column that counts
the number of constraints (local and inherited PK and NOT NULL
constraints) that make the column not null.

Something else I noticed when reading the SQL standard is that a
user-defined CHECK (col IS NOT NULL) constraint should be recognised
by the system as also making the column not null (setting its
"nullability characteristic" to "known not nullable"). I think that's
more than just an artefact of how they say NOT NULL constraints should
be implemented, because the effect of such a CHECK constraint should
be exposed in the "columns" view of the information schema -- the
value of "is_nullable" should be "NO" if the column is "known not
nullable".

In this sense, the standard does allow multiple not null constraints
on a column, independently of whether the column is "defined as NOT
NULL". My understanding of the standard is that ALTER COLUMN ...
SET/DROP NOT NULL change whether or not the column is "defined as NOT
NULL", and manage a single system-generated constraint, but there may
be any number of other user-defined constraints that also make the
column "known not nullable", and they need to be tracked in some way.

I'm also wondering whether creating a pg_constraint entry for *every*
not-nullable column is actually going too far. If we were to
distinguish between "defined as NOT NULL" and being not null as a
result of one or more constraints, in the way that the standard seems
to suggest, perhaps the former (likely to be much more common) could
simply be a new attribute stored on the column. I think we actually
only need to create pg_constraint entries if a constraint name or any
additional constraint properties such as NOT VALID are specified. That
would lead to far fewer new constraints, less catalog bloat, and less
noise in the \d output.

Regards,
Dean

On 2023-Aug-15, Dean Rasheed wrote:

> I think perhaps for ALTER TABLE INHERIT, it should check that the
> child has a NOT NULL constraint, and error out if not. That's the
> current behaviour, and also matches other constraints types (e.g.,
> CHECK constraints).

Yeah, I reached the same conclusion yesterday while trying it out, so
that's what I implemented.  I'll post later today.

> More generally though, I'm worried that this is starting to get very
> complicated. I wonder if there might be a different, simpler approach.
> One vague idea is to have a new attribute on the column that counts
> the number of constraints (local and inherited PK and NOT NULL
> constraints) that make the column not null.

Hmm.  I grant that this is different, but I don't see that it is
simpler.

> Something else I noticed when reading the SQL standard is that a
> user-defined CHECK (col IS NOT NULL) constraint should be recognised
> by the system as also making the column not null (setting its
> "nullability characteristic" to "known not nullable").

I agree with this view actually, but I've refrained from implementing
it(*) because our SQL-standards people have advised against it.  Insider
knowledge?  I don't know.  I think this is a comparatively smaller
consideration though, and we can adjust for it afterwards.

(*) Rather: at some point I removed the implementation of that from the
patch.

> I'm also wondering whether creating a pg_constraint entry for *every*
> not-nullable column is actually going too far. If we were to
> distinguish between "defined as NOT NULL" and being not null as a
> result of one or more constraints, in the way that the standard seems
> to suggest, perhaps the former (likely to be much more common) could
> simply be a new attribute stored on the column. I think we actually
> only need to create pg_constraint entries if a constraint name or any
> additional constraint properties such as NOT VALID are specified. That
> would lead to far fewer new constraints, less catalog bloat, and less
> noise in the \d output.

There is a problem if we do this, though, which is that we cannot use
the constraints for the things that we want them for -- for example,
remove_useless_groupby_columns() would like to use unique constraints,
not just primary keys; but it depends on the NOT NULL rows being there
for invalidation reasons (namely: if the NOT NULL constraint is dropped,
we need to be able to replan.  Without catalog rows, we don't have a
mechanism to let that happen).

If we don't add all those redundant catalog rows, then this is all for
naught.

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

On 15.08.23 11:57, Dean Rasheed wrote:
> Something else I noticed when reading the SQL standard is that a
> user-defined CHECK (col IS NOT NULL) constraint should be recognised
> by the system as also making the column not null (setting its
> "nullability characteristic" to "known not nullable"). I think that's
> more than just an artefact of how they say NOT NULL constraints should
> be implemented, because the effect of such a CHECK constraint should
> be exposed in the "columns" view of the information schema -- the
> value of "is_nullable" should be "NO" if the column is "known not
> nullable".

Nullability determination is different from not-null constraints.  The 
nullability characteristic of a column can be derived from multiple 
sources, including not-null constraints, check constraints, primary key 
constraints, domain constraints, as well as more complex rules in case 
of views, joins, etc.  But this is all distinct and separate from the 
issue of not-null constraints that we are discussing here.

I have two small patches that you can integrate into your patch set:

The first just changes the punctuation of "Not-null constraints" in the 
psql output to match what the documentation mostly uses.

The second has some changes to ddl.sgml to reflect that not-null 
constraints are now named and can be operated on like other constraints. 
  You might want to read that again to make sure it matches your latest 
intentions, but I think it catches all the places that are required to 
change.

Okay, so here's another version of this, where I fixed the creation of
NOT NULLs derived from PKs.  It turned out that what I was doing wasn't
doing recursion correctly, so for example if you have NOT NULLs in
grand-child tables they would not be marked as inherited from the PK
(thus wrongly droppable).  I had to rewrite it to go through ATPrepCmd
and friends; and we had no way to indicate inheritance that way, so I
had to add an "int inhcount" to the Constraint node.  (I think it might
be OK to make it just a "bool inherited" instead).

There is one good thing about this, which is that currently
AddRelationNewConstraints() has a strange "bool is_local" parameter
(added by commit cd902b331d, 2008), which is somewhat strange, and which
we could remove to instead use this new Constraint->inhcount mechanism
to pass down the flag.


Also: it turns out that you can do this
CREATE TABLE parent (a int);
CREATE TABLE child (NOT NULL a) INHERITS (parent);

that is, the column has no local definition on the child, but the
constraint does.  This required some special fixes but also works
correctly now AFAICT.

On 2023-Aug-16, Peter Eisentraut wrote:

> I have two small patches that you can integrate into your patch set:
> 
> The first just changes the punctuation of "Not-null constraints" in the psql
> output to match what the documentation mostly uses.
> 
> The second has some changes to ddl.sgml to reflect that not-null constraints
> are now named and can be operated on like other constraints.  You might want
> to read that again to make sure it matches your latest intentions, but I
> think it catches all the places that are required to change.

I've incorporated both of those, verbatim for now; I'll give the docs
another look tomorrow.

On 2023-Aug-11, Alvaro Herrera wrote:

> - ALTER TABLE parent ADD PRIMARY KEY
>   needs to create NOT NULL constraints in children.  I added this, but
>   I'm not yet sure it works correctly (for example, if a child already
>   has a NOT NULL constraint, we need to bump its inhcount, but we
>   don't.)
> - ALTER TABLE parent ADD PRIMARY KEY USING index
>   Not sure if this is just as above or needs separate handling
> - ALTER TABLE DROP PRIMARY KEY
>   needs to decrement inhcount or drop the constraint if there are no
>   other sources for that constraint to exist.  I've adjusted the drop
>   constraint code to do this.
> - ALTER TABLE INHERIT
>   needs to create a constraint on the new child, if parent has PK. Not
>   implemented
> - ALTER TABLE NO INHERIT
>   needs to delink any constraints (decrement inhcount, possibly drop
>   the constraint).
>
> I also need to add tests for those scenarios, because I think there
> aren't any for most of them.

I've added tests for the ones I caught missing, including leaving some
tables to exercise the pg_upgrade side of things.

> There's also another a pg_upgrade problem: we now get spurious ALTER
> TABLE SET NOT NULL commands in a dump after pg_upgrade for the columns
> that get the constraint from a primary key.

I fixed this too.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

I went over the whole patch and made a very large number of additional
cleanups[1], to the point where I think this is truly ready for commit now.
There are some relatively minor things that could still be subject of
debate, such as what to name constraints that derive from PKs or from
multiple inheritance parents.  I have one commented out Assert() because
of that.  But other than those and a couple of not-terribly-important
XXX comments, this is as ready as it'll ever be.

I'll put it through CI soon.  It's been a while since I tested using
pg_upgrade from older versions, so I'll do that too.  If no problems
emerge, I intend to get this committed soon.

[1] https://github.com/alvherre/postgres/tree/catalog-notnull-9

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"If you want to have good ideas, you must have many ideas.  Most of them
will be wrong, and what you have to learn is which ones to throw away."
                                                         (Linus Pauling)

I have now pushed this again.  Hopefully it'll stick this time.

We may want to make some further tweaks to the behavior in some cases --
for example, don't disallow ALTER TABLE DROP NOT NULL when the
constraint is both inherited and has a local definition; the other
option is to mark the constraint as no longer having a local definition.
I left it the other way because that's what CHECK does; maybe we would
like to change both at once.

I ran it through CI, and the pg_upgrade test with a dump from 14's
regression test database and everything worked well, but it's been a
while since I tested the sepgsql part of it, so that might the first
thing to explode.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On 2023-Aug-25, Alvaro Herrera wrote:

> I have now pushed this again.  Hopefully it'll stick this time.

Hmm, failed under the Czech locale[1]; apparently "inh_grandchld" sorts
earlier than "inh_child1" there.  I think I'll rename inh_grandchld to
inh_child3 or something like that.

[1] https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hippopotamus&dt=2023-08-25%2011%3A33%3A07

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On 25.08.23 13:38, Alvaro Herrera wrote:
> I have now pushed this again.  Hopefully it'll stick this time.
> 
> We may want to make some further tweaks to the behavior in some cases --
> for example, don't disallow ALTER TABLE DROP NOT NULL when the
> constraint is both inherited and has a local definition; the other
> option is to mark the constraint as no longer having a local definition.
> I left it the other way because that's what CHECK does; maybe we would
> like to change both at once.
> 
> I ran it through CI, and the pg_upgrade test with a dump from 14's
> regression test database and everything worked well, but it's been a
> while since I tested the sepgsql part of it, so that might the first
> thing to explode.

It looks like we forgot about domain constraints?  For example,

create domain testdomain as int not null;

should create a row in pg_constraint?

On 2023-Aug-28, Peter Eisentraut wrote:

> It looks like we forgot about domain constraints?  For example,
> 
> create domain testdomain as int not null;
> 
> should create a row in pg_constraint?

Well, at some point I purposefully left them out; they were sufficiently
different from the ones in tables that doing both things at the same
time was not saving any effort.  I guess we could try to bake them in
now.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem."  (Tom Lane)

I have now pushed this again.  Hopefully it'll stick this time.

On 2023-Mar-29, Peter Eisentraut wrote:

> On 27.03.23 15:55, Peter Eisentraut wrote:
> > The information schema should be updated.  I think the following views:
> > 
> > - CHECK_CONSTRAINTS
> > - CONSTRAINT_COLUMN_USAGE
> > - DOMAIN_CONSTRAINTS
> > - TABLE_CONSTRAINTS
> > 
> > It looks like these have no test coverage; maybe that could be addressed
> > at the same time.
> 
> Here are patches for this.  I haven't included the expected files for the
> tests; this should be checked again that output is correct or the changes
> introduced by this patch set are as expected.
> 
> The reason we didn't have tests for this before was probably in part because
> the information schema made up names for not-null constraints involving
> OIDs, so the test wouldn't have been stable.
> 
> Feel free to integrate this, or we can add it on afterwards.

I'm eyeing patch 0002 here.  I noticed that in view check_constraints it
defines the not-null constraint definition as substrings over the
pg_get_constraintdef() function[q1], so I wondered whether it might be
better to join to pg_attribute instead.  I see two options:

1. add a scalar subselect in the select list for each constraint [q2]
2. add a LEFT JOIN to pg_attribute to the main FROM list [q3]
   ON con.conrelid=att.attrelid AND con.conkey[1] = con.attrelid

With just the regression test tables in place, these forms are all
pretty much the same in execution time.  I then created 20k tables with
6 columns each and NOT NULL constraint on each column[4].  That's not a
huge amount but it's credible for a medium-size database with a bunch of
partitions (it's amazing what passes for a medium-size database these
days).  I was surprised to find out that q3 (~130ms) is three times
faster than q2 (~390ms), which is in turn more than twice faster than
your proposed q1 (~870ms).  So unless you have another reason to prefer
it, I think we should use q3 here.


In constraint_column_usage, you're adding a relkind to the catalog scan
that goes through pg_depend for CHECK constraints.  Here we can rely on
a simple conkey[1] check and a separate UNION ALL arm[q5]; this is also
faster when there are many tables.

The third view definition looks ok.  It's certainly very nice to be able
to delete XXX comments there.


[q1]
SELECT current_database()::information_schema.sql_identifier AS constraint_catalog,
    rs.nspname::information_schema.sql_identifier AS constraint_schema,
    con.conname::information_schema.sql_identifier AS constraint_name,
        CASE con.contype
            WHEN 'c'::"char" THEN "left"(SUBSTRING(pg_get_constraintdef(con.oid) FROM 8), '-1'::integer)
            WHEN 'n'::"char" THEN SUBSTRING(pg_get_constraintdef(con.oid) FROM 10) || ' IS NOT NULL'::text
                              
 
            ELSE NULL::text
        END::information_schema.character_data AS check_clause
   FROM pg_constraint con
     LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace
     LEFT JOIN pg_class c ON c.oid = con.conrelid
     LEFT JOIN pg_type t ON t.oid = con.contypid
               
 
  WHERE pg_has_role(COALESCE(c.relowner, t.typowner), 'USAGE'::text) AND (con.contype = ANY (ARRAY['c'::"char",
'n'::"char"]));

[q2]
SELECT current_database()::information_schema.sql_identifier AS constraint_catalog,
    rs.nspname::information_schema.sql_identifier AS constraint_schema,
    con.conname::information_schema.sql_identifier AS constraint_name,
        CASE con.contype
            WHEN 'c'::"char" THEN "left"(SUBSTRING(pg_get_constraintdef(con.oid) FROM 8), '-1'::integer)
            WHEN 'n'::"char" THEN FORMAT('CHECK (%s IS NOT NULL)',
                                         (SELECT attname FROM pg_attribute WHERE attrelid = conrelid AND attnum =
conkey[1]))
            ELSE NULL::text
        END::information_schema.character_data AS check_clause
   FROM pg_constraint con
     LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace
     LEFT JOIN pg_class c ON c.oid = con.conrelid
     LEFT JOIN pg_type t ON t.oid = con.contypid
  WHERE pg_has_role(COALESCE(c.relowner, t.typowner), 'USAGE'::text) AND (con.contype = ANY (ARRAY['c'::"char",
'n'::"char"]));

[q3]
SELECT current_database()::information_schema.sql_identifier AS constraint_catalog,
    rs.nspname::information_schema.sql_identifier AS constraint_schema,
    con.conname::information_schema.sql_identifier AS constraint_name,
        CASE con.contype
            WHEN 'c'::"char" THEN "left"(SUBSTRING(pg_get_constraintdef(con.oid) FROM 8), '-1'::integer)
            WHEN 'n'::"char" THEN FORMAT('CHECK (%s IS NOT NULL)', at.attname)
                              
 
            ELSE NULL::text
        END::information_schema.character_data AS check_clause
   FROM pg_constraint con
     LEFT JOIN pg_namespace rs ON rs.oid = con.connamespace
     LEFT JOIN pg_class c ON c.oid = con.conrelid
     LEFT JOIN pg_type t ON t.oid = con.contypid
     LEFT JOIN pg_attribute at ON (con.conrelid = at.attrelid AND con.conkey[1] = at.attnum)
  WHERE pg_has_role(COALESCE(c.relowner, t.typowner), 'USAGE'::text) AND (con.contype = ANY (ARRAY['c'::"char",
'n'::"char"]));

[4]
do $$ begin for i in 0 .. 20000 loop
 execute format('create table t_%s (a1 int not null, a2 int not null, a3 int not null,
    a4 int not null, a5 int not null, a6 int not null);',
   i);
 if i % 1000 = 0 then commit; end if;
end loop; end $$;

[q5]
SELECT CAST(current_database() AS sql_identifier) AS table_catalog,
            CAST(tblschema AS sql_identifier) AS table_schema,
            CAST(tblname AS sql_identifier) AS table_name,
            CAST(colname AS sql_identifier) AS column_name,
            CAST(current_database() AS sql_identifier) AS constraint_catalog,
            CAST(cstrschema AS sql_identifier) AS constraint_schema,
            CAST(cstrname AS sql_identifier) AS constraint_name

     FROM (
         /* check constraints */
         SELECT DISTINCT nr.nspname, r.relname, r.relowner, a.attname, nc.nspname, c.conname
           FROM pg_namespace nr, pg_class r, pg_attribute a, pg_depend d, pg_namespace nc, pg_constraint c
           WHERE nr.oid = r.relnamespace
             AND r.oid = a.attrelid
             AND d.refclassid = 'pg_catalog.pg_class'::regclass
             AND d.refobjid = r.oid
             AND d.refobjsubid = a.attnum
             AND d.classid = 'pg_catalog.pg_constraint'::regclass
             AND d.objid = c.oid
             AND c.connamespace = nc.oid
             AND c.contype = 'c'
             AND r.relkind IN ('r', 'p')
             AND NOT a.attisdropped

        UNION ALL

        /* not-null constraints */
        SELECT DISTINCT nr.nspname, r.relname, r.relowner, a.attname, nc.nspname, c.conname
          FROM pg_namespace nr, pg_class r, pg_attribute a, pg_namespace nc, pg_constraint c
          WHERE nr.oid = r.relnamespace
        AND r.oid = a.attrelid
        AND r.oid = c.conrelid
        AND a.attnum = c.conkey[1]
        AND c.connamespace = nc.oid
        AND c.contype = 'n'
        AND r.relkind in ('r', 'p')
        AND not a.attisdropped

         UNION ALL

         /* unique/primary key/foreign key constraints */
         SELECT nr.nspname, r.relname, r.relowner, a.attname, nc.nspname, c.conname
           FROM pg_namespace nr, pg_class r, pg_attribute a, pg_namespace nc,
                pg_constraint c
           WHERE nr.oid = r.relnamespace
             AND r.oid = a.attrelid
             AND nc.oid = c.connamespace
             AND r.oid = CASE c.contype WHEN 'f' THEN c.confrelid ELSE c.conrelid END
             AND a.attnum = ANY (CASE c.contype WHEN 'f' THEN c.confkey ELSE c.conkey END)
             AND NOT a.attisdropped
             AND c.contype IN ('p', 'u', 'f')
             AND r.relkind IN ('r', 'p')

       ) AS x (tblschema, tblname, tblowner, colname, cstrschema, cstrname)

     WHERE pg_has_role(x.tblowner, 'USAGE') ;

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Hello Alexander,

Thanks for testing.

On 2023-Aug-31, Alexander Lakhin wrote:

> 25.08.2023 14:38, Alvaro Herrera wrote:
> > I have now pushed this again.  Hopefully it'll stick this time.
> 
> I've found that after that commit the following query:
> CREATE TABLE t(a int PRIMARY KEY) PARTITION BY RANGE (a);
> CREATE TABLE tp1(a int);
> ALTER TABLE t ATTACH PARTITION tp1 FOR VALUES FROM (0) to (1);
> 
> triggers a server crash:

Hmm, that's some weird code I left there all right.  Can you please try
this patch?  (Not final; I'll review it more completely later,
particularly to add this test case.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
<Schwern> It does it in a really, really complicated way
<crab> why does it need to be complicated?
<Schwern> Because it's MakeMaker.

31.08.2023 13:26, Alvaro Herrera wrote:
> Hmm, that's some weird code I left there all right.  Can you please try
> this patch?  (Not final; I'll review it more completely later,
> particularly to add this test case.)

Yes, your patch fixes the issue. I get the same error now:
ERROR:  column "a" in child table must be marked NOT NULL

Thank you!

Best regards,
Alexander

On 2023-Aug-31, Alvaro Herrera wrote:

> Hmm, that's some weird code I left there all right.  Can you please try
> this patch?  (Not final; I'll review it more completely later,
> particularly to add this test case.)

The change in MergeAttributesIntoExisting turned out to be close but not
quite there, so I pushed another version of the fix.

In case you're wondering, the change in MergeConstraintsIntoExisting is
a related but different case, for which I added the other test case you
see there.

I also noticed, while looking at this, that there's another problem when
a child has a NO INHERIT not-null constraint and the parent has a
primary key in the same column.  It should refuse, or take over by
marking it no longer NO INHERIT.  But it just accepts silently and all
appears to be good.  The problems appear when you add a child to that
child.  I'll look into this later; it's not exactly the same code.  At
least it's not a crasher.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

Looking at your 0001 patch, which adds tests for some of the
information_schema views, I think it's a bad idea to put them in
whatever other regression .sql files; they would be subject to
concurrent changes depending on what other tests are being executed in
the same parallel test.  I suggest to put them all in a separate .sql
file, and schedule that to run in the last concurrent group, together
with the tablespace test.  This way, it would capture all the objects
left over by other test files.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On 31.08.23 12:02, Alvaro Herrera wrote:
> In constraint_column_usage, you're adding a relkind to the catalog scan
> that goes through pg_depend for CHECK constraints.  Here we can rely on
> a simple conkey[1] check and a separate UNION ALL arm[q5]; this is also
> faster when there are many tables.
> 
> The third view definition looks ok.  It's certainly very nice to be able
> to delete XXX comments there.

The following information schema views are affected by the not-null 
constraint catalog entries:

1. CHECK_CONSTRAINTS
2. CONSTRAINT_COLUMN_USAGE
3. DOMAIN_CONSTRAINTS
4. TABLE_CONSTRAINTS

Note that 1 and 3 also contain domain constraints.  So as long as the 
domain not-null constraints are not similarly catalogued, we can't 
delete the separate not-null union branch.  (3 never had one, so 
arguably a bit buggy.)

I think we can fix up 4 by just deleting the not-null union branch.

For 2, the simple fix is also easy, but there are some other options, as 
you discuss above.

How do you want to proceed?

On 2023-Sep-05, Peter Eisentraut wrote:

> The following information schema views are affected by the not-null
> constraint catalog entries:
> 
> 1. CHECK_CONSTRAINTS
> 2. CONSTRAINT_COLUMN_USAGE
> 3. DOMAIN_CONSTRAINTS
> 4. TABLE_CONSTRAINTS
> 
> Note that 1 and 3 also contain domain constraints.  So as long as the domain
> not-null constraints are not similarly catalogued, we can't delete the
> separate not-null union branch.  (3 never had one, so arguably a bit buggy.)
> 
> I think we can fix up 4 by just deleting the not-null union branch.
> 
> For 2, the simple fix is also easy, but there are some other options, as you
> discuss above.
> 
> How do you want to proceed?

I posted as a patch in a separate thread[1].  Let me fix up the
definitions for views 1 and 3 for domains per your comments, and I'll
post in that thread again.

[1] https://postgr.es/m/202309041710.psytrxlsiqex@alvherre.pgsql

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Ninguna manada de bestias tiene una voz tan horrible como la humana" (Orual)

On 2023-Sep-05, Peter Eisentraut wrote:

> The following information schema views are affected by the not-null
> constraint catalog entries:
> 
> 1. CHECK_CONSTRAINTS
> 2. CONSTRAINT_COLUMN_USAGE
> 3. DOMAIN_CONSTRAINTS
> 4. TABLE_CONSTRAINTS
> 
> Note that 1 and 3 also contain domain constraints.

After looking at what happens for domain constraints in older versions
(I tested 15, but I suppose this applies everywhere), I notice that we
don't seem to handle them anywhere that I can see.  My quick exercise is
just

create domain nnint as int not null;
create table foo (a nnint);

and then verify that this constraint shows nowhere -- it's not in
DOMAIN_CONSTRAINTS for starters, which is I think the most obvious place.
And nothing is shown in CHECK_CONSTRAINTS nor TABLE_CONSTRAINTS either.

This did ever work in the past?  I tested with 9.3 and didn't see
anything there either.

I am hesitant to try to add domain not-null constraint support to
information_schema in the same commit as these changes.  I think this
should be fixed separately.

(Note that if, in older versions, you change the table to be
 create table foo (a nnint NOT NULL);
 then you do get a row in table_constraints, but nothing in
 check_constraints.  With my proposed definition this constraint appears
 in check_constraints, table_constraints and constraint_column_usage.)

On 2023-Sep-04, Tom Lane wrote:

> I object very very strongly to this proposed test method.  It
> completely undoes the work I did in v15 (cc50080a8 and related)
> to make the core regression test scripts mostly independent of each
> other.  Even without considering the use-case of running a subset of
> the tests, the new test's expected output will constantly be needing
> updates as side effects of unrelated changes.

You're absolutely right, this would be disastrous.  A better alternative
is that the new test file creates a few objects for itself, either by
using a separate role or by using a separate schema, and we examine the
information_schema display for those objects only.  Then it'll be better
isolated.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Subversion to GIT: the shortest path to happiness I've ever heard of
                                                (Alexey Klyukin)

On 2023-Sep-05, Alvaro Herrera wrote:

> After looking at what happens for domain constraints in older versions
> (I tested 15, but I suppose this applies everywhere), I notice that we
> don't seem to handle them anywhere that I can see.  My quick exercise is
> just
> 
> create domain nnint as int not null;
> create table foo (a nnint);
> 
> and then verify that this constraint shows nowhere -- it's not in
> DOMAIN_CONSTRAINTS for starters, which is I think the most obvious place.
> And nothing is shown in CHECK_CONSTRAINTS nor TABLE_CONSTRAINTS either.

Looking now at what to do for CHECK_CONSTRAINTS with domain constraints,
I admit I'm completely confused about what this view is supposed to
show.  Currently, we show the constraint name and a definition like
"CHECK (column IS NOT NULL)".  But since the table name is not given, it
is not possible to know to what table the column name refers to.  For
domains, we could show "CHECK (VALUE IS NOT NULL)" but again with no
indication of what domain it applies to, or anything at all that would
make this useful in any way whatsoever.

So this whole thing seems pretty futile and I'm disinclined to waste
much time on it.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 9/5/23 19:15, Alvaro Herrera wrote:
> On 2023-Sep-05, Alvaro Herrera wrote:
> 
> Looking now at what to do for CHECK_CONSTRAINTS with domain constraints,
> I admit I'm completely confused about what this view is supposed to
> show.  Currently, we show the constraint name and a definition like
> "CHECK (column IS NOT NULL)".  But since the table name is not given, it
> is not possible to know to what table the column name refers to.  For
> domains, we could show "CHECK (VALUE IS NOT NULL)" but again with no
> indication of what domain it applies to, or anything at all that would
> make this useful in any way whatsoever.

Constraint names are supposed to be unique per schema[1] so the view 
contains the minimum required information to identify the constraint.

> So this whole thing seems pretty futile and I'm disinclined to waste
> much time on it.

Until PostgreSQL either
   A) obeys the spec on this uniqueness, or
   B) decides to deviate from the information_schema spec;
this view will be completely useless for actually getting any useful 
information.

I would like to see us do A because it is the right thing to do.  Our 
autogenerated names obey this rule, but who knows how many duplicate 
names per schema are out there in the wild from people specifying their 
own names.

I don't know what the project would think about doing B.


[1] SQL:2023-2 11.4 <table constraint definition> Syntax Rule 4
-- 
Vik Fearing

On 9/6/23 00:14, David G. Johnston wrote:
> 
> I'm not all that for either A or B since the status quo seems workable.

Pray tell, how is it workable?  The view does not identify a specific 
constraint because we don't obey the rules on one side and we do obey 
the rules on the other side.  It is completely useless and unworkable.

> Though ideally if the system has unique names per schema then everything
> should just work - having the views produce duplicated information (as
> opposed to nothing) if they are used when the DBA doesn't enforce the
> standard's requirements seems plausible.
Let us not engage in victim blaming.  Postgres is the problem here.
-- 
Vik Fearing

Vik Fearing <vik@postgresfriends.org> writes:
> On 9/6/23 00:14, David G. Johnston wrote:
>> I'm not all that for either A or B since the status quo seems workable.

> Pray tell, how is it workable?  The view does not identify a specific 
> constraint because we don't obey the rules on one side and we do obey 
> the rules on the other side.  It is completely useless and unworkable.

What solution do you propose?  Starting to enforce the spec's rather
arbitrary requirement that constraint names be unique per-schema is
a complete nonstarter.  Changing the set of columns in a spec-defined
view is also a nonstarter, or at least we've always taken it as such.

If you'd like to see some forward progress in this area, maybe you
could lobby the SQL committee to make constraint names unique per-table
not per-schema, and then make the information_schema changes that would
be required to support that.

In general though, the fact that we have any DDL extensions at all
compared to the standard means that there will be Postgres databases
that are not adequately represented by the information_schema views.
I'm not sure it's worth being more outraged about constraint names
than anything else.  Or do you also want us to rip out (for starters)
unique indexes on expressions, or unique partial indexes?

            regards, tom lane

On 9/6/23 02:53, Tom Lane wrote:
> Vik Fearing <vik@postgresfriends.org> writes:
>> On 9/6/23 00:14, David G. Johnston wrote:
>>> I'm not all that for either A or B since the status quo seems workable.
> 
>> Pray tell, how is it workable?  The view does not identify a specific
>> constraint because we don't obey the rules on one side and we do obey
>> the rules on the other side.  It is completely useless and unworkable.
> 
> What solution do you propose?  Starting to enforce the spec's rather
> arbitrary requirement that constraint names be unique per-schema is
> a complete nonstarter.  Changing the set of columns in a spec-defined
> view is also a nonstarter, or at least we've always taken it as such.

I both semi-agree and semi-disagree that these are nonstarters.  One of 
them has to give.

> If you'd like to see some forward progress in this area, maybe you
> could lobby the SQL committee to make constraint names unique per-table
> not per-schema, and then make the information_schema changes that would
> be required to support that.

I could easily do that; but now you are asking to denormalize the 
standard, because the constraints could be from tables, domains, or 
assertions.

I don't think that will go over well, starting with my own opinion.

And for this reason, I do not believe that this is a "rather arbitrary 
requirement".

> In general though, the fact that we have any DDL extensions at all
> compared to the standard means that there will be Postgres databases
> that are not adequately represented by the information_schema views.

Sure.

> I'm not sure it's worth being more outraged about constraint names
> than anything else.  Or do you also want us to rip out (for starters)
> unique indexes on expressions, or unique partial indexes?

Indexes of any kind are not part of the standard so these examples are 
basically invalid.

SQL:2023-11 Schemata is not the part I am most familiar with, but I 
don't even see where regular multi-column unique constraints are listed 
out, so that is both a lack in the standard and a knockdown of this 
argument.  I am happy to be shown wrong about this.
-- 
Vik Fearing

Vik Fearing <vik@postgresfriends.org> writes:
> On 9/6/23 02:53, Tom Lane wrote:
>> What solution do you propose?  Starting to enforce the spec's rather
>> arbitrary requirement that constraint names be unique per-schema is
>> a complete nonstarter.  Changing the set of columns in a spec-defined
>> view is also a nonstarter, or at least we've always taken it as such.

> I both semi-agree and semi-disagree that these are nonstarters.  One of 
> them has to give.

[ shrug... ] if you stick to a SQL-compliant schema setup, then the
information_schema views will serve for introspection.  If you don't,
they won't, and you'll need to look at Postgres-specific catalog data.
This compromise has served for twenty years or so, and I'm not in a
hurry to change it.  I think the odds of changing to the spec's
restriction without enormous pushback are nil, and I do not think
that the benefit could possibly be worth the ensuing pain to users.
(It's not even the absolute pain level that is a problem, so much
as the asymmetry: the pain would fall exclusively on users who get
no benefit, because they weren't relying on these views anyway.
If you think that's an easy sell, you're mistaken.)

It could possibly be a little more palatable to add column(s) to the
information_schema views, but I'm having a hard time seeing how that
moves the needle.  The situation would still be precisely describable
as "if you stick to a SQL-compliant schema setup, then the standard
columns of the information_schema views will serve for introspection.
If you don't, they won't, and you'll need to look at Postgres-specific
columns".  That doesn't seem like a big improvement.  Also, given your
point about normalization, how would we define the additions exactly?

            regards, tom lane

On 05.09.23 18:24, Alvaro Herrera wrote:
> On 2023-Sep-05, Peter Eisentraut wrote:
> 
>> The following information schema views are affected by the not-null
>> constraint catalog entries:
>>
>> 1. CHECK_CONSTRAINTS
>> 2. CONSTRAINT_COLUMN_USAGE
>> 3. DOMAIN_CONSTRAINTS
>> 4. TABLE_CONSTRAINTS
>>
>> Note that 1 and 3 also contain domain constraints.
> 
> After looking at what happens for domain constraints in older versions
> (I tested 15, but I suppose this applies everywhere), I notice that we
> don't seem to handle them anywhere that I can see.  My quick exercise is
> just
> 
> create domain nnint as int not null;
> create table foo (a nnint);
> 
> and then verify that this constraint shows nowhere -- it's not in
> DOMAIN_CONSTRAINTS for starters, which is I think the most obvious place.
> And nothing is shown in CHECK_CONSTRAINTS nor TABLE_CONSTRAINTS either.
> 
> This did ever work in the past?  I tested with 9.3 and didn't see
> anything there either.

No, this was never implemented.  (As I wrote in my other message on the 
other thread, arguably a bit buggy.)  We could fix this separately, 
unless we are going to implement catalogued domain not-null constraints 
soon.

On 9/6/23 05:40, Tom Lane wrote:
> Vik Fearing <vik@postgresfriends.org> writes:
>> On 9/6/23 02:53, Tom Lane wrote:
>>> What solution do you propose?  Starting to enforce the spec's rather
>>> arbitrary requirement that constraint names be unique per-schema is
>>> a complete nonstarter.  Changing the set of columns in a spec-defined
>>> view is also a nonstarter, or at least we've always taken it as such.
> 
>> I both semi-agree and semi-disagree that these are nonstarters.  One of
>> them has to give.
> 
> [ shrug... ] if you stick to a SQL-compliant schema setup, then the
> information_schema views will serve for introspection.  If you don't,
> they won't, and you'll need to look at Postgres-specific catalog data.


As someone who regularly asks people to cite chapter and verse of the 
standard, do you not see this as a problem?

If there is /one thing/ I wish we were 100% compliant on, it's 
information_schema.


> This compromise has served for twenty years or so, and I'm not in a
> hurry to change it.  


Has it?  Or is this just the first time someone has complained?


> I think the odds of changing to the spec's
> restriction without enormous pushback are nil, and I do not think
> that the benefit could possibly be worth the ensuing pain to users.


That is a valid opinion, and probably one that will win out for quite a 
while.


> (It's not even the absolute pain level that is a problem, so much
> as the asymmetry: the pain would fall exclusively on users who get
> no benefit, because they weren't relying on these views anyway.
> If you think that's an easy sell, you're mistaken.)


I am curious how many people we are selling this to.  In my career as a 
consultant, I have never once come across anyone specifying their own 
constraint names.  That is certainly anecdotal, and by no means means it 
doesn't happen, but my personal experience says that it is very low.

And since our generated names obey the spec (see ChooseConstraintName() 
which even says some apps depend on this), I don't see making this 
change being a big problem in the real world.

Mind you, I am not pushing (right now) to make this change; I am just 
saying that it is the right thing to do.


> It could possibly be a little more palatable to add column(s) to the
> information_schema views, but I'm having a hard time seeing how that
> moves the needle.  The situation would still be precisely describable
> as "if you stick to a SQL-compliant schema setup, then the standard
> columns of the information_schema views will serve for introspection.
> If you don't, they won't, and you'll need to look at Postgres-specific
> columns".  That doesn't seem like a big improvement.  Also, given your
> point about normalization, how would we define the additions exactly?


This is precisely my point.
-- 
Vik Fearing

Hi Alvaro,

25.08.2023 14:38, Alvaro Herrera wrote:
> I have now pushed this again.  Hopefully it'll stick this time.

I've discovered that that commit added several recursive functions, and
some of them are not protected from stack overflow.

Namely, with "max_locks_per_transaction = 600" and default ulimit -s (8192),
I observe server crashes with the following scripts:
# ATExecSetNotNull()
(n=40000; printf "create table t0 (a int, b int);";
for ((i=1;i<=$n;i++)); do printf "create table t$i() inherits(t$(( $i - 1 ))); "; done;
printf "alter table t0 alter b set not null;" ) | psql >psql.log

# dropconstraint_internal()
(n=20000; printf "create table t0 (a int, b int not null);";
for ((i=1;i<=$n;i++)); do printf "create table t$i() inherits(t$(( $i - 1 ))); "; done;
printf "alter table t0 alter b drop not null;" ) | psql >psql.log

# set_attnotnull()
(n=110000; printf "create table tp (a int, b int, primary key(a, b)) partition by range (a); create table tp0 (a int 
primary key, b int) partition by range (a);";
for ((i=1;i<=$n;i++)); do printf "create table tp$i partition of tp$(( $i - 1 )) for values from ($i) to (1000000) 
partition by range (a);"; done;
printf "alter table tp attach partition tp0 for values from (0) to (1000000);") | psql >psql.log # this takes half an 
hour on my machine

May be you would find appropriate to add check_stack_depth() to these
functions.

(ATAddCheckNNConstraint() is protected because it calls
AddRelationNewConstraints(), which in turn calls StoreRelCheck() ->
CreateConstraintEntry() ->  recordDependencyOnSingleRelExpr() ->
find_expr_references_walker() ->  expression_tree_walker() ->
expression_tree_walker() -> check_stack_depth().)

(There were patches prepared for similar cases [1], but they don't cover new
functions, of course, and I'm not sure how to handle all such instances.)

[1] https://commitfest.postgresql.org/45/4239/

Best regards,
Alexander

On 2023-Oct-12, Alexander Lakhin wrote:

Hello,

> I've discovered that that commit added several recursive functions, and
> some of them are not protected from stack overflow.

True.  I reproduced the first two, but didn't attempt to reproduce the
third one -- patching all these to check for stack depth is cheap
protection.  I also patched ATAddCheckNNConstraint:

> (ATAddCheckNNConstraint() is protected because it calls
> AddRelationNewConstraints(), which in turn calls StoreRelCheck() ->
> CreateConstraintEntry() ->  recordDependencyOnSingleRelExpr() ->
> find_expr_references_walker() ->  expression_tree_walker() ->
> expression_tree_walker() -> check_stack_depth().)

because it seems uselessly risky to rely on depth checks that exist on
completely unrelated pieces of code, when the function visibly recurses
on itself.  Especially so since the test cases that demonstrate crashes
are so expensive to run, which means we're not going to detect it if at
some point that other stack depth check stops being called for whatever
reason.

BTW probably the tests could be made much cheaper by running the server
with a lower "ulimit -s" setting.  I didn't try.

I noticed one more crash while trying to "drop table" one of the
hierarchies your scripts create.  But it's a preexisting issue which
needs a backpatched fix, and I think Egor already reported it in the
other thread.

Thank you

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Industry suffers from the managerial dogma that for the sake of stability
and continuity, the company should be independent of the competence of
individual employees."                                      (E. Dijkstra)

Hello Alvaro,

Please look at an anomaly introduced with b0e96f311.
The following script:
CREATE TABLE a ();
CREATE TABLE b (i int) INHERITS (a);
CREATE TABLE c () INHERITS (a, b);

ALTER TABLE a ADD COLUMN i int NOT NULL;

results in:
NOTICE:  merging definition of column "i" for child "b"
NOTICE:  merging definition of column "i" for child "c"
ERROR:  tuple already updated by self

(This is similar to bug #18297, but ATExecAddColumn() isn't guilty in this
case.)

Best regards,
Alexander

On Fri, Feb 02, 2024 at 07:00:01PM +0300, Alexander Lakhin wrote:
> results in:
> NOTICE:  merging definition of column "i" for child "b"
> NOTICE:  merging definition of column "i" for child "c"
> ERROR:  tuple already updated by self
>
> (This is similar to bug #18297, but ATExecAddColumn() isn't guilty in this
> case.)

Still I suspect that the fix should be similar, soI'll go put a coin
on a missing CCI().
--
Michael

On 2024-Feb-05, Michael Paquier wrote:

> On Fri, Feb 02, 2024 at 07:00:01PM +0300, Alexander Lakhin wrote:
> > results in:
> > NOTICE:  merging definition of column "i" for child "b"
> > NOTICE:  merging definition of column "i" for child "c"
> > ERROR:  tuple already updated by self
> > 
> > (This is similar to bug #18297, but ATExecAddColumn() isn't guilty in this
> > case.)
> 
> Still I suspect that the fix should be similar, so I'll go put a coin
> on a missing CCI().

Hmm, let me have a look, I can probably get this one fixed today before
embarking on a larger fix elsewhere in the same feature.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"After a quick R of TFM, all I can say is HOLY CR** THAT IS COOL! PostgreSQL was
amazing when I first started using it at 7.2, and I'm continually astounded by
learning new features and techniques made available by the continuing work of
the development team."
Berend Tober, http://archives.postgresql.org/pgsql-hackers/2007-08/msg01009.php

On 2024-Feb-05, Alvaro Herrera wrote:

> Hmm, let me have a look, I can probably get this one fixed today before
> embarking on a larger fix elsewhere in the same feature.

You know what -- this missing CCI has a much more visible impact, which
is that the attnotnull marker that a primary key imposes on a partition
is propagated early.  So this regression test no longer fails:

create table cnn2_parted(a int primary key) partition by list (a);
create table cnn2_part1(a int);
alter table cnn2_parted attach partition cnn2_part1 for values in (1);

Here, in the existing code the ALTER TABLE ATTACH fails with the error
message that
  ERROR:  primary key column "a" is not marked NOT NULL
but with the patch, this no longer occurs.

I'm not sure that this behavior change is desirable ... I have vague
memories of people complaining that this sort of error was not very
welcome ... but on the other hand it seems now pretty clear that if it
*is* desirable, then its implementation is no good, because a single
added CCI breaks it.

I'm leaning towards accepting the behavior change, but I'd like to
investigate a little bit more first, but what do others think?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 2024-Feb-05, Alvaro Herrera wrote:

> Subject: [PATCH v1] Fix failure to merge NOT NULL constraints in inheritance
> 
> set_attnotnull() was not careful to CommandCounterIncrement() in cases
> of multiple recursion.  Omission in b0e96f311985.

Eh, this needs to read "multiple inheritance" rather than "multiple
recursion".  (I'd also need to describe the change for the partitioning
cases in the commit message.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Someone said that it is at least an order of magnitude more work to do
production software than a prototype. I think he is wrong by at least
an order of magnitude."                              (Brian Kernighan)

On 2024-Feb-05, Alvaro Herrera wrote:

> So this regression test no longer fails:
> 
> create table cnn2_parted(a int primary key) partition by list (a);
> create table cnn2_part1(a int);
> alter table cnn2_parted attach partition cnn2_part1 for values in (1);

> Here, in the existing code the ALTER TABLE ATTACH fails with the error
> message that
>   ERROR:  primary key column "a" is not marked NOT NULL
> but with the patch, this no longer occurs.

I think this change is OK.  In the partition, the primary key is created
in the partition anyway (as expected) which marks the column as
attnotnull[*], and the table is scanned for presence of NULLs if there's
no not-null constraint, and not scanned if there's one.  (The actual
scan is inevitable anyway because we must check the partition
constraint).  This seems the behavior we want.

[*] This attnotnull constraint is lost if you DETACH the partition and
drop the primary key, which is also the behavior we want.


While playing with it I noticed this other behavior change from 16,

create table pa (a int primary key) partition by list (a);
create table pe (a int unique);
alter table pa attach partition pe for values in (1, null);

In 16, we get the error:
ERROR:  column "a" in child table must be marked NOT NULL
which is correct (because the PK requires not-null).  In master we just
let that through, but that seems to be a separate bug.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Saca el libro que tu religión considere como el indicado para encontrar la
oración que traiga paz a tu alma. Luego rebootea el computador
y ve si funciona" (Carlos Duclós)

On 2024-Feb-05, Alvaro Herrera wrote:

> While playing with it I noticed this other behavior change from 16,
> 
> create table pa (a int primary key) partition by list (a);
> create table pe (a int unique);
> alter table pa attach partition pe for values in (1, null);
> 
> In 16, we get the error:
> ERROR:  column "a" in child table must be marked NOT NULL
> which is correct (because the PK requires not-null).  In master we just
> let that through, but that seems to be a separate bug.

Hmm, so my initial reaction was to make the constraint-matching code
ignore the constraint in the partition-to-be if it's not the same type
(this is what patch 0002 here does) ... but what ends up happening is
that we create a separate, identical constraint+index for the primary
key.  I don't like that behavior too much myself, as it seems too
magical and surprising, since it could cause the ALTER TABLE ATTACH
operation of a large partition become costly and slower, since it needs
to create an index instead of merely scanning the whole data.

I'll look again at the idea of raising an error if the not-null
constraint is not already present.  That seems safer (and also, it's
what we've been doing all along).

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Mon, Feb 5, 2024 at 5:51 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2024-Feb-05, Alvaro Herrera wrote:
>
> > Hmm, let me have a look, I can probably get this one fixed today before
> > embarking on a larger fix elsewhere in the same feature.
>
> You know what -- this missing CCI has a much more visible impact, which
> is that the attnotnull marker that a primary key imposes on a partition
> is propagated early.  So this regression test no longer fails:
>
> create table cnn2_parted(a int primary key) partition by list (a);
> create table cnn2_part1(a int);
> alter table cnn2_parted attach partition cnn2_part1 for values in (1);
>
> Here, in the existing code the ALTER TABLE ATTACH fails with the error
> message that
>   ERROR:  primary key column "a" is not marked NOT NULL
> but with the patch, this no longer occurs.
>
> I'm not sure that this behavior change is desirable ... I have vague
> memories of people complaining that this sort of error was not very
> welcome ... but on the other hand it seems now pretty clear that if it
> *is* desirable, then its implementation is no good, because a single
> added CCI breaks it.
>
> I'm leaning towards accepting the behavior change, but I'd like to
> investigate a little bit more first, but what do others think?
>

if you place CommandCounterIncrement inside the `if (recurse)` branch,
then the regression test will be ok.

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 9f516967..25e225c2 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7719,6 +7719,9 @@ set_attnotnull(List **wqueue, Relation rel,
AttrNumber attnum, bool recurse,

                         false));
                        retval |= set_attnotnull(wqueue, childrel, childattno,

  recurse, lockmode);
+
+                       CommandCounterIncrement();

(I think I had already argued this point, but I don't see it in the
archives, so here it is again).

On 2024-Feb-07, jian he wrote:

> if you place CommandCounterIncrement inside the `if (recurse)` branch,
> then the regression test will be ok.

Yeah, but don't you think this is too magical?  I mean, randomly added
CCIs in the execution path for other reasons would break this.  Worse --
how can we _ensure_ that no CCIs occur at all?  I mean, it's possible
that an especially crafted multi-subcommand ALTER TABLE could contain
just the right CCI to break things in the opposite way.  The difference
in behavior would be difficult to justify.  (For good or ill, ALTER
TABLE ATTACH PARTITION cannot run in a multi-subcommand ALTER TABLE, so
this concern might be misplaced.  Still, more certainty seems better
than less.)

I've pushed both these patches now, adding what seemed a reasonable set
of test cases.  If there still are cases behaving in unexpected ways,
please let me know.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"La espina, desde que nace, ya pincha" (Proverbio africano)

On 2024-Jan-25, Andrew Bille wrote:

> Starting from b0e96f31, pg_upgrade fails with inherited NOT NULL constraint:
> For example upgrade from 9c13b6814a (or REL_12_STABLE .. REL_16_STABLE) to
> b0e96f31 (or master) with following two tables (excerpt from
> src/test/regress/sql/rules.sql)
> 
> create table test_0 (id serial primary key);
> create table test_1 (id integer primary key) inherits (test_0);

I have pushed a fix which should hopefully fix this problem
(d9f686a72e).  Please give this a look.  Thanks for reporting the issue.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"I apologize for the confusion in my previous responses.
 There appears to be an error."                   (ChatGPT)

Hello Alvaro,

18.04.2024 16:39, Alvaro Herrera wrote:
> I have pushed a fix which should hopefully fix this problem
> (d9f686a72e).  Please give this a look.  Thanks for reporting the issue.

Please look at an assertion failure, introduced with d9f686a72:
CREATE TABLE t(a int, NOT NULL a NO INHERIT);
CREATE TABLE t2() INHERITS (t);

ALTER TABLE t ADD CONSTRAINT nna NOT NULL a;
TRAP: failed Assert("lockmode != NoLock || IsBootstrapProcessingMode() || CheckRelationLockedByMe(r, AccessShareLock, 
true)"), File: "relation.c", Line: 67, PID: 2980258

On d9f686a72~1 this script results in:
ERROR:  cannot change NO INHERIT status of inherited NOT NULL constraint "t_a_not_null" on relation "t"

Best regards,
Alexander

Hi Alexander,

On 2024-Apr-18, Alexander Lakhin wrote:

> 18.04.2024 16:39, Alvaro Herrera wrote:
> > I have pushed a fix which should hopefully fix this problem
> > (d9f686a72e).  Please give this a look.  Thanks for reporting the issue.
> 
> Please look at an assertion failure, introduced with d9f686a72:
> CREATE TABLE t(a int, NOT NULL a NO INHERIT);
> CREATE TABLE t2() INHERITS (t);
> 
> ALTER TABLE t ADD CONSTRAINT nna NOT NULL a;
> TRAP: failed Assert("lockmode != NoLock || IsBootstrapProcessingMode() ||
> CheckRelationLockedByMe(r, AccessShareLock, true)"), File: "relation.c",
> Line: 67, PID: 2980258

Ah, of course -- we're missing acquiring locks during the prep phase for
the recursive case of ADD CONSTRAINT.  So we just need to add
find_all_inheritors() to do so in the AT_AddConstraint case in
ATPrepCmd().  However these naked find_all_inheritors() call look a bit
ugly to me, so I couldn't resist the temptation of adding a static
function ATLockAllDescendants to clean it up a bit.  I'll also add your
script to the tests and push shortly.

> On d9f686a72~1 this script results in:
> ERROR:  cannot change NO INHERIT status of inherited NOT NULL constraint "t_a_not_null" on relation "t"

Right.  Now I'm beginning to wonder if allowing ADD CONSTRAINT to mutate
a pre-existing NO INHERIT constraint into a inheritable constraint
(while accepting a constraint name in the command that we don't heed) is
really what we want.  Maybe we should throw some error when the affected
constraint is the topmost one, and only accept the inheritance status
change when we're recursing.

Also I just noticed that in 9b581c534186 (which introduced this error
message) I used ERRCODE_DATATYPE_MISMATCH ... Is that really appropriate
here?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL.  This is by far the most pleasant management experience of
any database I've worked on."                             (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php

On 2024-Apr-22, Alvaro Herrera wrote:

> > On d9f686a72~1 this script results in:
> > ERROR:  cannot change NO INHERIT status of inherited NOT NULL constraint "t_a_not_null" on relation "t"
> 
> Right.  Now I'm beginning to wonder if allowing ADD CONSTRAINT to mutate
> a pre-existing NO INHERIT constraint into a inheritable constraint
> (while accepting a constraint name in the command that we don't heed) is
> really what we want.  Maybe we should throw some error when the affected
> constraint is the topmost one, and only accept the inheritance status
> change when we're recursing.

So I added a restriction that we only accept such a change when
recursively adding a constraint, or during binary upgrade.  This should
limit the damage: you're no longer able to change an existing constraint
from NO INHERIT to YES INHERIT merely by doing another ALTER TABLE ADD
CONSTRAINT.

One thing that has me a little nervous about this whole business is
whether we're set up to error out where some child table down the
hierarchy has nulls, and we add a not-null constraint to it but fail to
do a verification scan.  I tried a couple of cases and AFAICS it works
correctly, but maybe there are other cases I haven't thought about where
it doesn't.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"You're _really_ hosed if the person doing the hiring doesn't understand
relational systems: you end up with a whole raft of programmers, none of
whom has had a Date with the clue stick."              (Andrew Sullivan)
https://postgr.es/m/20050809113420.GD2768@phlogiston.dyndns.org

24.04.2024 20:36, Alvaro Herrera wrote:
> So I added a restriction that we only accept such a change when
> recursively adding a constraint, or during binary upgrade.  This should
> limit the damage: you're no longer able to change an existing constraint
> from NO INHERIT to YES INHERIT merely by doing another ALTER TABLE ADD
> CONSTRAINT.
>
> One thing that has me a little nervous about this whole business is
> whether we're set up to error out where some child table down the
> hierarchy has nulls, and we add a not-null constraint to it but fail to
> do a verification scan.  I tried a couple of cases and AFAICS it works
> correctly, but maybe there are other cases I haven't thought about where
> it doesn't.
>

Thank you for the fix!

While studying the NO INHERIT option, I've noticed that the documentation
probably misses it's specification for NOT NULL:
https://www.postgresql.org/docs/devel/sql-createtable.html

where column_constraint is:
...
[ CONSTRAINT constraint_name ]
{ NOT NULL |
   NULL |
   CHECK ( expression ) [ NO INHERIT ] |

Also, I've found a weird behaviour with a non-inherited NOT NULL
constraint for a partitioned table:
CREATE TABLE pt(a int NOT NULL NO INHERIT) PARTITION BY LIST (a);
CREATE TABLE dp(a int NOT NULL);
ALTER TABLE pt ATTACH PARTITION dp DEFAULT;
ALTER TABLE pt DETACH PARTITION dp;
fails with:
ERROR:  relation 16389 has non-inherited constraint "dp_a_not_null"

Though with an analogous check constraint, I get:
CREATE TABLE pt(a int, CONSTRAINT nna CHECK (a IS NOT NULL) NO INHERIT) PARTITION BY LIST (a);
ERROR:  cannot add NO INHERIT constraint to partitioned table "pt"

Best regards,
Alexander

On 2024-Apr-25, Alexander Lakhin wrote:

> While studying the NO INHERIT option, I've noticed that the documentation
> probably misses it's specification for NOT NULL:
> https://www.postgresql.org/docs/devel/sql-createtable.html
> 
> where column_constraint is:
> ...
> [ CONSTRAINT constraint_name ]
> { NOT NULL |
>   NULL |
>   CHECK ( expression ) [ NO INHERIT ] |

Hmm, okay, will fix.

> Also, I've found a weird behaviour with a non-inherited NOT NULL
> constraint for a partitioned table:
> CREATE TABLE pt(a int NOT NULL NO INHERIT) PARTITION BY LIST (a);
> CREATE TABLE dp(a int NOT NULL);
> ALTER TABLE pt ATTACH PARTITION dp DEFAULT;
> ALTER TABLE pt DETACH PARTITION dp;
> fails with:
> ERROR:  relation 16389 has non-inherited constraint "dp_a_not_null"

Ugh.  Maybe a way to handle this is to disallow NO INHERIT in
constraints on partitioned tables altogether.  I mean, they are a
completely useless gimmick, aren't they?

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On 2024-Apr-25, Alvaro Herrera wrote:

> > Also, I've found a weird behaviour with a non-inherited NOT NULL
> > constraint for a partitioned table:
> > CREATE TABLE pt(a int NOT NULL NO INHERIT) PARTITION BY LIST (a);

> Ugh.  Maybe a way to handle this is to disallow NO INHERIT in
> constraints on partitioned tables altogether.  I mean, they are a
> completely useless gimmick, aren't they?

Here are two patches that I intend to push soon (hopefully tomorrow).

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto.  No es cierto, y si fuera cierto,
 no me acuerdo."                 (Augusto Pinochet a una corte de justicia)

Hello Alvaro,

01.05.2024 20:49, Alvaro Herrera wrote:
> Here are two patches that I intend to push soon (hopefully tomorrow).
>

Thank you for fixing those issues!

Could you also clarify, please, how CREATE TABLE ... LIKE is expected to
work with NOT NULL constraints?

I wonder whether EXCLUDING CONSTRAINTS (ALL) should cover not-null
constraints too. What I'm seeing now, is that:
CREATE TABLE t1 (i int, CONSTRAINT nn NOT NULL i);
CREATE TABLE t2 (LIKE t1 EXCLUDING ALL);
\d+ t2
-- ends with:
Not-null constraints:
     "nn" NOT NULL "i"

Or a similar case with PRIMARY KEY:
CREATE TABLE t1 (i int PRIMARY KEY);
CREATE TABLE t2 (LIKE t1 EXCLUDING CONSTRAINTS EXCLUDING INDEXES);
\d+ t2
-- leaves:
Not-null constraints:
     "t2_i_not_null" NOT NULL "i"

Best regards,
Alexander

Hello Alexander

On 2024-May-02, Alexander Lakhin wrote:

> Could you also clarify, please, how CREATE TABLE ... LIKE is expected to
> work with NOT NULL constraints?

It should behave identically to 16.  If in 16 you end up with a
not-nullable column, then in 17 you should get a not-null constraint.

> I wonder whether EXCLUDING CONSTRAINTS (ALL) should cover not-null
> constraints too. What I'm seeing now, is that:
> CREATE TABLE t1 (i int, CONSTRAINT nn NOT NULL i);
> CREATE TABLE t2 (LIKE t1 EXCLUDING ALL);
> \d+ t2
> -- ends with:
> Not-null constraints:
>     "nn" NOT NULL "i"

In 16, this results in
                                           Table "public.t2"
 Column │  Type   │ Collation │ Nullable │ Default │ Storage │ Compression │ Stats target │ Description 
────────┼─────────┼───────────┼──────────┼─────────┼─────────┼─────────────┼──────────────┼─────────────
 i      │ integer │           │ not null │         │ plain   │             │              │ 
Access method: heap

so the fact that we have a not-null constraint in pg17 is correct.


> Or a similar case with PRIMARY KEY:
> CREATE TABLE t1 (i int PRIMARY KEY);
> CREATE TABLE t2 (LIKE t1 EXCLUDING CONSTRAINTS EXCLUDING INDEXES);
> \d+ t2
> -- leaves:
> Not-null constraints:
>     "t2_i_not_null" NOT NULL "i"

Here you also end up with a not-nullable column in 16, so I made it do
that.

Now you could argue that EXCLUDING CONSTRAINTS is explicit in saying
that we don't want the constraints; but in that case why did 16 mark the
columns as not-null?  The answer seems to be that the standard requires
this.  Look at 11.3 <table definition> syntax rule 9) b) iii) 4):

  4) If the nullability characteristic included in LCDi is known not
  nullable, then let LNCi be NOT NULL; otherwise, let LNCi be the
  zero-length character string.

where LCDi is "1) Let LCDi be the column descriptor of the i-th column
of LT." and then

  5) Let CDi be the <column definition>
     LCNi LDTi LNCi


Now, you could claim that the standard doesn't mention
INCLUDING/EXCLUDING CONSTRAINTS, therefore since we have come up with
its definition then we should make it affect not-null constraints.
However, there's also this note:

  NOTE 520 — <column constraint>s, except for NOT NULL, are not included in
  CDi; <column constraint definition>s are effectively transformed to <table
  constraint definition>s and are thereby also excluded.

which is explicitly saying that not-null constraints are treated
differently; in essence, with INCLUDING CONSTRAINTS we choose to affect
the constraints that the standard says to ignore.


Thanks for looking!

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Learn about compilers. Then everything looks like either a compiler or
a database, and now you have two problems but one of them is fun."
            https://twitter.com/thingskatedid/status/1456027786158776329

02.05.2024 19:21, Alvaro Herrera wrote:

> Now, you could claim that the standard doesn't mention
> INCLUDING/EXCLUDING CONSTRAINTS, therefore since we have come up with
> its definition then we should make it affect not-null constraints.
> However, there's also this note:
>
>    NOTE 520 — <column constraint>s, except for NOT NULL, are not included in
>    CDi; <column constraint definition>s are effectively transformed to <table
>    constraint definition>s and are thereby also excluded.
>
> which is explicitly saying that not-null constraints are treated
> differently; in essence, with INCLUDING CONSTRAINTS we choose to affect
> the constraints that the standard says to ignore.

Thank you for very detailed and convincing explanation!

Now I see what the last sentence here (from [1]) means:
INCLUDING CONSTRAINTS

     CHECK constraints will be copied. No distinction is made between
     column constraints and table constraints. _Not-null constraints are
     always copied to the new table._

(I hadn't paid enough attention to it, because this exact paragraph is
also presented in previous versions...)

[1] https://www.postgresql.org/docs/devel/sql-createtable.html

Best regards,
Alexander

Hello,

At Wed, 1 May 2024 19:49:35 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in 
> Here are two patches that I intend to push soon (hopefully tomorrow).

This commit added and edited two error messages, resulting in using
slightly different wordings "in" and "on" for relation constraints.

+   errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"",
===
+   errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"",

I think we usually use on in this case.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 2024-May-07, Kyotaro Horiguchi wrote:

> Hello,
> 
> At Wed, 1 May 2024 19:49:35 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in 
> > Here are two patches that I intend to push soon (hopefully tomorrow).
> 
> This commit added and edited two error messages, resulting in using
> slightly different wordings "in" and "on" for relation constraints.
> 
> +   errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"",
> ===
> +   errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"",

Thank you, I hadn't noticed the inconsistency -- I fix this in the
attached series.

While trying to convince myself that I could mark the remaining open
item for this work closed, I discovered that pg_dump fails to produce
working output for some combinations.  Notably, if I create Andrew
Bille's example in 16:

create table test_0 (id serial primary key);
create table test_1 (id integer primary key) inherits (test_0);

then current master's pg_dump produces output that the current server
fails to restore, failing the PK creation in test_0:

ALTER TABLE ONLY public.test_0
    ADD CONSTRAINT test_0_pkey PRIMARY KEY (id);
ERROR:  cannot change NO INHERIT status of NOT NULL constraint "pgdump_throwaway_notnull_0" in relation "test_1"

because we have already created the NOT NULL NO INHERIT constraint in
test_1 when we created it, and because of d45597f72fe5, we refuse to
change it into a regular inheritable constraint, which the PK in its
parent table needs.

I spent a long time trying to think how to fix this, and I had despaired
wanting to write that I would need to revert the whole NOT NULL business
for pg17 -- but that was until I realized that we don't actually need
this NOT NULL NO INHERIT business except during pg_upgrade, and that
simplifies things enough to give me confidence that the whole feature
can be kept.

Because, remember: the idea of those NO INHERIT "throwaway" constraints
is that we can skip reading the data when we create the PRIMARY KEY
during binary upgrade.  We don't actually need the NO INHERIT
constraints for anything during regular pg_dump.  So what we can do, is
restrict the usage of NOT NULL NO INHERIT so that they occur only during
pg_upgrade.  I think this will make Justin P. happier, because we no
longer have these unsightly NOT NULL NO INHERIT nonstandard syntax in
dumps.

The attached patch series does that.  Actually, it does a little more,
but it's not really much:

0001: fix the typos pointed out by Kyotaro.

0002: A mechanical code movement that takes some ugly ballast out of
getTableAttrs into its own routine.  I realized that this new code was
far too ugly and messy to be in the middle of filling the tbinfo struct
of attributes.  If you use "git show --color-moved
--color-moved-ws=ignore-all-space" with this commit you can see that
nothing happens apart from the code move.

0003: pgindent, fixes the comments just moved to account for different
indentation depth.

0004: moves again the moved PQfnumber() calls back to getTableAttrs(),
for efficiency (we don't want to search the result for those resnums for
every single attribute of all tables being dumped).

0005: This is the actual code change I describe above.  We restrict
use_throwaway_nulls so that it's only set during binary upgrade mode.
This changes pg_dump output; in the normal case, we no longer have NOT
NULL NO INHERIT.  I added one test stanza to verify that pg_upgrade
retains these clauses, where they are critical.

0006: Tighten up what d45597f72fe5 did, in that outside of binary
upgrade mode, we no longer accept changes to NOT NULL NO INHERIT
constraints so that they become INHERIT.  Previously we accepted that
during recursion, but this isn't really very principled.  (I had
accepted this because pg_dump required it for some other cases).  This
changes some test output, and I also simplify some test cases that were
testing stuff that's no longer interesting.

(To push, I'll squash 0002+0003+0004 as a single one, and perhaps 0005
with them; I produced them like this only to make them easy to see
what's changing.)


I also have a pending patch for 16 that adds tables like the problematic
ones so that they remain for future pg_upgrade testing.  With the
changes in this series, the whole thing finally works AFAICT.

I did notice one more small bit of weirdness, which is that at the end
of the process you may end up with constraints that retain the throwaway
name.  This doesn't seem at all critical, considering that you can't
drop them anyway and such names do not survive a further dump (because
they are marked as inherited constraint without a "local" definition, so
they're not dumped separately).  I would still like to fix it, but it
seems to require unduly contortions so I may end up not doing anything
about it.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

On Wed, May 8, 2024 at 4:42 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> I spent a long time trying to think how to fix this, and I had despaired
> wanting to write that I would need to revert the whole NOT NULL business
> for pg17 -- but that was until I realized that we don't actually need
> this NOT NULL NO INHERIT business except during pg_upgrade, and that
> simplifies things enough to give me confidence that the whole feature
> can be kept.

Yeah, I have to admit that the ongoing bug fixing here has started to
make me a bit nervous, but I also can't totally follow everything
that's under discussion, so I don't want to rush to judgement. I feel
like we might need some documentation or a README or something that
explains the takeaway from the recent commits dealing with no-inherit
constraints. None of those commits updated the documentation, which
may be fine, but neither the resulting behavior nor the reasoning
behind it is obvious. It's not enough for it to be correct -- it has
to be understandable enough to the hive mind that we can maintain it
going forward.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2024-May-09, Robert Haas wrote:

> Yeah, I have to admit that the ongoing bug fixing here has started to
> make me a bit nervous, but I also can't totally follow everything
> that's under discussion, so I don't want to rush to judgement.

I have found two more problems that I think are going to require some
more work to fix, so I've decided to cut my losses now and revert the
whole.  I'll come back again in 18 with these problems fixed.

Specifically, the problem is that I mentioned that we could restrict the
NOT NULL NO INHERIT addition in pg_dump for primary keys to occur only
in pg_upgrade; but it turns this is not correct.  In normal
dump/restore, there's an additional table scan to check for nulls when
the constraints is not there, so the PK creation would become measurably
slower.  (In a table with a million single-int rows, PK creation goes
from 2000ms to 2300ms due to the second scan to check for nulls).

The addition of NOT NULL NO INHERIT constraints for this purpose
collides with addition of constraints for other reasons, and it forces
us to do unpleasant things such as altering an existing constraint to go
from NO INHERIT to INHERIT.  If this happens only during pg_upgrade,
that would be okay IMV; but if we're forced to allow in normal operation
(and in some cases we are), it could cause inconsistencies, so I don't
want to do that.  I see a way to fix this (adding another query in
pg_dump that detects which columns descend from ones used in PKs in
ancestor tables), but that's definitely too much additional mechanism to
be adding this late in the cycle.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On 2024-May-11, Alvaro Herrera wrote:

> I have found two more problems that [] require some more work to fix,
> so I've decided to cut my losses now and revert the whole.

Here's the revert patch, which I intend to push early tomorrow.

Commits reverted are:
21ac38f498b33f0231842238b83847ec63dfe07b
d45597f72fe53a53f6271d5ba4e7acf8fc9308a1
13daa33fa5a6d340f9be280db14e7b07ed11f92e
0cd711271d42b0888d36f8eda50e1092c2fed4b3
d72d32f52d26c9588256de90b9bc54fe312cee60
d9f686a72ee91f6773e5d2bc52994db8d7157a8e
c3709100be73ad5af7ff536476d4d713bca41b1a
3af7217942722369a6eb7629e0fb1cbbef889a9b
b0f7dd915bca6243f3daf52a81b8d0682a38ee3b
ac22a9545ca906e70a819b54e76de38817c93aaf
d0ec2ddbe088f6da35444fad688a62eae4fbd840
9b581c53418666205938311ef86047aa3c6b741f
b0e96f311985bceba79825214f8e43f65afa653a

with some significant conflict fixes (mostly in the last one).

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El sentido de las cosas no viene de las cosas, sino de
las inteligencias que las aplican a sus problemas diarios
en busca del progreso." (Ernesto Hernández-Novich)

On Sat, May 11, 2024 at 5:40 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> I have found two more problems that I think are going to require some
> more work to fix, so I've decided to cut my losses now and revert the
> whole.  I'll come back again in 18 with these problems fixed.

Bummer, but makes sense.

> Specifically, the problem is that I mentioned that we could restrict the
> NOT NULL NO INHERIT addition in pg_dump for primary keys to occur only
> in pg_upgrade; but it turns this is not correct.  In normal
> dump/restore, there's an additional table scan to check for nulls when
> the constraints is not there, so the PK creation would become measurably
> slower.  (In a table with a million single-int rows, PK creation goes
> from 2000ms to 2300ms due to the second scan to check for nulls).

I have a feeling that any theory of the form "X only needs to happen
during pg_upgrade" is likely to be wrong. pg_upgrade isn't really
doing anything especially unusual: just creating some objects and
loading data. Those things can also be done at other times, so
whatever is needed during pg_upgrade is also likely to be needed at
other times. Maybe that's not sound reasoning for some reason or
other, but that's my intuition.

> The addition of NOT NULL NO INHERIT constraints for this purpose
> collides with addition of constraints for other reasons, and it forces
> us to do unpleasant things such as altering an existing constraint to go
> from NO INHERIT to INHERIT.  If this happens only during pg_upgrade,
> that would be okay IMV; but if we're forced to allow in normal operation
> (and in some cases we are), it could cause inconsistencies, so I don't
> want to do that.  I see a way to fix this (adding another query in
> pg_dump that detects which columns descend from ones used in PKs in
> ancestor tables), but that's definitely too much additional mechanism to
> be adding this late in the cycle.

I'm sorry that I haven't been following this thread closely, but I'm
confused about how we ended up here. What exactly are the user-visible
behavior changes wrought by this patch, and how do they give rise to
these issues? One change I know about is that a constraint that is
explicitly catalogued (vs. just existing implicitly) has a name. But
it isn't obvious to me that such a difference, by itself, is enough to
cause all of these problems: if a NOT NULL constraint is created
without a name, then I suppose we just have to generate one. Maybe the
fact that the constraints have names somehow causes ugliness later,
but I can't quite understand why it would.

The other possibility that occurs to me is that I think the motivation
for cataloging NOT NULL constraints was that we wanted to be able to
track dependencies on them, or something like that, which seems like
it might be able to create issues of the type that you're facing, but
the details aren't clear to me. Changing any behavior in this area
seems like it could be quite tricky, because of things like the
interaction between PRIMARY KEY and NOT NULL, which is rather
idiosyncratic but upon which a lot of existing SQL (including SQL not
controlled by us) likely depends. If there's not a clear plan for how
we keep all the stuff that works today working, I fear we'll end up in
an endless game of whack-a-mole. If you've already written the design
ideas down someplace, I'd appreciate a pointer in the right direction.

Or maybe there's some other issue entirely. In any case, sorry about
the revert, and sorry that I haven't paid more attention to this.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2024-May-13, Robert Haas wrote:

> On Sat, May 11, 2024 at 5:40 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > Specifically, the problem is that I mentioned that we could restrict the
> > NOT NULL NO INHERIT addition in pg_dump for primary keys to occur only
> > in pg_upgrade; but it turns this is not correct.  In normal
> > dump/restore, there's an additional table scan to check for nulls when
> > the constraints is not there, so the PK creation would become measurably
> > slower.  (In a table with a million single-int rows, PK creation goes
> > from 2000ms to 2300ms due to the second scan to check for nulls).
> 
> I have a feeling that any theory of the form "X only needs to happen
> during pg_upgrade" is likely to be wrong. pg_upgrade isn't really
> doing anything especially unusual: just creating some objects and
> loading data. Those things can also be done at other times, so
> whatever is needed during pg_upgrade is also likely to be needed at
> other times. Maybe that's not sound reasoning for some reason or
> other, but that's my intuition.

True.  It may be that by setting up the upgrade SQL script differently,
we don't need to make the distinction at all.  I hope to be able to do
that.

> I'm sorry that I haven't been following this thread closely, but I'm
> confused about how we ended up here. What exactly are the user-visible
> behavior changes wrought by this patch, and how do they give rise to
> these issues?

The problematic point is the need to add NOT NULL constraints during
table creation that don't exist in the table being dumped, for
performance of primary key creation -- I called this a throwaway
constraint.  We needed to be able to drop those constraints after the PK
was created.  These were marked NO INHERIT to allow them to be dropped,
which is easier if the children don't have them.  This all worked fine.

However, at some point we realized that we needed to add NOT NULL
constraints in child tables for the columns in which the parent had a
primary key.  Then things become messy because we had the throwaway
constraints on one hand and the not-nulls that descend from the PK on
the other hand, where one was NO INHERIT and the other wasn't; worse if
the child also has a primary key.

It turned out that we didn't have any mechanism to transform a NO
INHERIT constraint into a regular one that would be inherited.  I added
one, didn't like the way it worked, tried to restrict it but that caused
other problems; this is the mess that led to the revert (pg_dump in
normal mode would emit scripts that fail for some legitimate cases).

One possible way forward might be to make pg_dump smarter by adding one
more query to know the relationship between constraints that must be
dropped and those that don't.  Another might be to allow multiple
not-null constraints on the same column (one inherits, the other
doesn't, and you can drop them independently).  There may be others.

> The other possibility that occurs to me is that I think the motivation
> for cataloging NOT NULL constraints was that we wanted to be able to
> track dependencies on them, or something like that, which seems like
> it might be able to create issues of the type that you're facing, but
> the details aren't clear to me.

NOT VALID constraints would be extremely useful, for one thing (because
then you don't need to exclusively-lock the table during a long scan in
order to add a constraint), and it's just one step away from having
these constraints be catalogued.  It was also fixing some inconsistent
handling of inheritance cases.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Mon, May 13, 2024 at 9:44 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> The problematic point is the need to add NOT NULL constraints during
> table creation that don't exist in the table being dumped, for
> performance of primary key creation -- I called this a throwaway
> constraint.  We needed to be able to drop those constraints after the PK
> was created.  These were marked NO INHERIT to allow them to be dropped,
> which is easier if the children don't have them.  This all worked fine.

This seems really weird to me. Why is it necessary? I mean, in
existing releases, if you declare a column as PRIMARY KEY, the columns
included in the key are forced to be NOT NULL, and you can't change
that for so long as they are included in the PRIMARY KEY. So I would
have thought that after this patch, you'd end up with the same thing.
One way of doing that would be to make the PRIMARY KEY depend on the
now-catalogued NOT NULL constraints, and the other way would be to
keep it as an ad-hoc prohibition, same as now. In PostgreSQL 16, I get
a dump like this:

CREATE TABLE public.foo (
    a integer NOT NULL,
    b text
);

COPY public.foo (a, b) FROM stdin;
\.

ALTER TABLE ONLY public.foo
    ADD CONSTRAINT foo_pkey PRIMARY KEY (a);

If I'm dumping from an existing release, I don't see why any of that
needs to change. The NOT NULL decoration should lead to a
system-generated constraint name. If I'm dumping from a new release,
the NOT NULL decoration needs to be replaced with CONSTRAINT
existing_constraint_name NOT NULL. But I don't see why I need to end
up with what the patch generates, which seems to be something like
CONSTRAINT pgdump_throwaway_notnull_0 NOT NULL NO INHERIT. That kind
of thing suggests that we're changing around the order of operations
in pg_dump, probably by adding the NOT NULL constraints at a later
stage than currently, and I think the proper solution is most likely
to be to avoid doing that in the first place.

> However, at some point we realized that we needed to add NOT NULL
> constraints in child tables for the columns in which the parent had a
> primary key.  Then things become messy because we had the throwaway
> constraints on one hand and the not-nulls that descend from the PK on
> the other hand, where one was NO INHERIT and the other wasn't; worse if
> the child also has a primary key.

This seems like another problem that is created by changing the order
of operations in pg_dump.

> > The other possibility that occurs to me is that I think the motivation
> > for cataloging NOT NULL constraints was that we wanted to be able to
> > track dependencies on them, or something like that, which seems like
> > it might be able to create issues of the type that you're facing, but
> > the details aren't clear to me.
>
> NOT VALID constraints would be extremely useful, for one thing (because
> then you don't need to exclusively-lock the table during a long scan in
> order to add a constraint), and it's just one step away from having
> these constraints be catalogued.  It was also fixing some inconsistent
> handling of inheritance cases.

I agree that NOT VALID constraints would be very useful. I'm a little
scared by the idea of fixing inconsistent handling of inheritance
cases, just for fear that there may be more things relying on the
inconsistent behavior than we realize. I feel like this is an area
where it's easy for changes to be scarier than they at first seem. I
still have memories of discovering some of the current behavior back
in the mid-2000s when I was learning PostgreSQL (and databases
generally). It struck me as fiddly back then, and it still does. I
feel like there are probably some behaviors that look like arbitrary
decisions but are actually very important for some undocumented
reason. That's not to say that we shouldn't try to make improvements,
just that it may be hard to get right.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2024-May-13, Robert Haas wrote:

> On Mon, May 13, 2024 at 9:44 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > The problematic point is the need to add NOT NULL constraints during
> > table creation that don't exist in the table being dumped, for
> > performance of primary key creation -- I called this a throwaway
> > constraint.  We needed to be able to drop those constraints after the PK
> > was created.  These were marked NO INHERIT to allow them to be dropped,
> > which is easier if the children don't have them.  This all worked fine.
> 
> This seems really weird to me. Why is it necessary? I mean, in
> existing releases, if you declare a column as PRIMARY KEY, the columns
> included in the key are forced to be NOT NULL, and you can't change
> that for so long as they are included in the PRIMARY KEY.

The point is that a column can be in a primary key and not have an
explicit not-null constraint.  This is different from having a column be
NOT NULL and having a primary key on top.  In both cases the attnotnull
flag is set; the difference between these two scenarios is what happens
if you drop the primary key.  If you do not have an explicit not-null
constraint, then the attnotnull flag is lost as soon as you drop the
primary key.  You don't have to do DROP NOT NULL for that to happen.

This means that if you have a column that's in the primary key but does
not have an explicit not-null constraint, then we shouldn't make one up.
(Which we would, if we were to keep an unadorned NOT NULL that we can't
drop at the end of the dump.)

> So I would have thought that after this patch, you'd end up with the
> same thing.

At least as I interpret the standard, you wouldn't.

> One way of doing that would be to make the PRIMARY KEY depend on the
> now-catalogued NOT NULL constraints, and the other way would be to
> keep it as an ad-hoc prohibition, same as now.

That would be against what [I think] the standard says.

> But I don't see why I need to end up with what the patch generates,
> which seems to be something like CONSTRAINT pgdump_throwaway_notnull_0
> NOT NULL NO INHERIT. That kind of thing suggests that we're changing
> around the order of operations in pg_dump, probably by adding the NOT
> NULL constraints at a later stage than currently, and I think the
> proper solution is most likely to be to avoid doing that in the first
> place.

The point of the throwaway constraints is that they don't remain after
the dump has restored completely.  They are there only so that we don't
have to scan the data looking for possible nulls when we create the
primary key.  We have a DROP CONSTRAINT for the throwaway not-nulls as
soon as the PK is created.

We're not changing any order of operations as such.

> That's not to say that we shouldn't try to make improvements, just
> that it may be hard to get right.

Sure, that's why this patch has now been reverted twice :-) and has been
in the works for ... how many years now?

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

On Mon, May 13, 2024 at 12:45 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> The point is that a column can be in a primary key and not have an
> explicit not-null constraint.  This is different from having a column be
> NOT NULL and having a primary key on top.  In both cases the attnotnull
> flag is set; the difference between these two scenarios is what happens
> if you drop the primary key.  If you do not have an explicit not-null
> constraint, then the attnotnull flag is lost as soon as you drop the
> primary key.  You don't have to do DROP NOT NULL for that to happen
>
> This means that if you have a column that's in the primary key but does
> not have an explicit not-null constraint, then we shouldn't make one up.
> (Which we would, if we were to keep an unadorned NOT NULL that we can't
> drop at the end of the dump.)

It seems to me that the practical thing to do about this problem is
just decide not to solve it. I mean, it's currently the case that if
you establish a PRIMARY KEY when you create a table, the columns of
that key are marked NOT NULL and remain NOT NULL even if the primary
key is later dropped. So, if that didn't change, we would be no less
compliant with the SQL standard (or your reading of it) than we are
now. And if you do really want to make that change, why not split it
out into its own patch, so that the patch that does $SUBJECT is
changing the minimal number of other things at the same time? That
way, reverting something might not involve reverting everything, plus
you could have a separate design discussion about what that fix ought
to look like, separate from the issues that are truly inherent to
cataloging NOT NULL constraints per se.

What I meant about changing the order of operations is that,
currently, the database knows that the column is NOT NULL before the
COPY happens, and I don't think we can change that. I think you agree
-- that's why you invented the throwaway constraints. As far as I can
see, the problems all have to do with getting the "throwaway" part to
happen correctly. It can't be a problem to just mark the relevant
columns NOT NULL in the relevant tables -- we already do that. But if
you want to discard some of those NOT NULL markings once the PRIMARY
KEY is added, you have to know which ones to discard. If we just
consider the most straightforward scenario where somebody does a full
dump-and-restore, getting that right may be annoying, but it seems
like it surely has to be possible. The dump will just have to
understand which child tables (or, more generally, descendent tables)
got a NOT NULL marking on a column because of the PK and which ones
had an explicit marking in the old database and do the right thing in
each case.

But what if somebody does a selective restore of one table from a
partitioning hierarchy? Currently, the columns that would have been
part of the primary key end up NOT NULL, but the primary key itself is
not restored because it can't be. What will happen in this new system?
If you don't apply any NOT NULL constraints to those columns, then a
user who restores one partition from an old dump and tries to reattach
it to the correct partitioned table has to recheck the NOT NULL
constraint, unlike now. If you apply a normal-looking garden-variety
NOT NULL constraint to that column, you've invented a constraint that
didn't exist in the source database. And if you apply a throwaway NOT
NULL constraint but the user never attaches that table anywhere, then
the throwaway constraint survives. None of those options sound very
good to me.

Another scenario: Say that you have a table with a PRIMARY KEY. For
some reason, you want to drop the primary key and then add it back.
Well, with this definitional change, as soon as you drop it, you
forget that the underlying columns don't contain any nulls, so when
you add it back, you have to check them again. I don't know who would
find that behavior an improvement over what we have today.

So I don't really think it's a great idea to change this behavior, but
even if it is, is it such a good idea that we want to sink the whole
patch set repeatedly over it, as has already happened twice now? I
feel that if we did what Tom suggested a year ago in
https://www.postgresql.org/message-id/3801207.1681057430@sss.pgh.pa.us
-- "I'm inclined to think that this idea of suppressing the implied
NOT NULL from PRIMARY KEY is a nonstarter and we should just go ahead
and make such a constraint" -- there's a very good chance that a
revert would have been avoided here and it would still be just as
valid to think of revisiting this particular question in a future
release as it is now.

--
Robert Haas
EDB: http://www.enterprisedb.com

On 2024-May-13, Robert Haas wrote:

> It seems to me that the practical thing to do about this problem is
> just decide not to solve it. I mean, it's currently the case that if
> you establish a PRIMARY KEY when you create a table, the columns of
> that key are marked NOT NULL and remain NOT NULL even if the primary
> key is later dropped. So, if that didn't change, we would be no less
> compliant with the SQL standard (or your reading of it) than we are
> now.
[...]
> So I don't really think it's a great idea to change this behavior, but
> even if it is, is it such a good idea that we want to sink the whole
> patch set repeatedly over it, as has already happened twice now? I
> feel that if we did what Tom suggested a year ago in
> https://www.postgresql.org/message-id/3801207.1681057430@sss.pgh.pa.us
> -- "I'm inclined to think that this idea of suppressing the implied
> NOT NULL from PRIMARY KEY is a nonstarter and we should just go ahead
> and make such a constraint" [...]

Hmm, I hadn't interpreted Tom's message the way you suggest, and you may
be right that it might be a good way forward.  I'll keep this in mind
for next time.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"No es bueno caminar con un hombre muerto"

On Sun, May 12, 2024 at 04:56:09PM +0200, Álvaro Herrera wrote:
> On 2024-May-11, Alvaro Herrera wrote:
> 
> > I have found two more problems that [] require some more work to fix,
> > so I've decided to cut my losses now and revert the whole.
> 
> Here's the revert patch, which I intend to push early tomorrow.
> 
> Commits reverted are:
> 21ac38f498b33f0231842238b83847ec63dfe07b
> d45597f72fe53a53f6271d5ba4e7acf8fc9308a1
> 13daa33fa5a6d340f9be280db14e7b07ed11f92e
> 0cd711271d42b0888d36f8eda50e1092c2fed4b3
> d72d32f52d26c9588256de90b9bc54fe312cee60
> d9f686a72ee91f6773e5d2bc52994db8d7157a8e
> c3709100be73ad5af7ff536476d4d713bca41b1a
> 3af7217942722369a6eb7629e0fb1cbbef889a9b
> b0f7dd915bca6243f3daf52a81b8d0682a38ee3b
> ac22a9545ca906e70a819b54e76de38817c93aaf
> d0ec2ddbe088f6da35444fad688a62eae4fbd840
> 9b581c53418666205938311ef86047aa3c6b741f
> b0e96f311985bceba79825214f8e43f65afa653a
> 
> with some significant conflict fixes (mostly in the last one).

Turns out these commits generated a single release note item, which I
have now removed with the attached committed patch.

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

  Only you can decide what is important to you.

On Mon, May 13, 2024 at 09:00:28AM -0400, Robert Haas wrote:
> > Specifically, the problem is that I mentioned that we could restrict the
> > NOT NULL NO INHERIT addition in pg_dump for primary keys to occur only
> > in pg_upgrade; but it turns this is not correct.  In normal
> > dump/restore, there's an additional table scan to check for nulls when
> > the constraints is not there, so the PK creation would become measurably
> > slower.  (In a table with a million single-int rows, PK creation goes
> > from 2000ms to 2300ms due to the second scan to check for nulls).
> 
> I have a feeling that any theory of the form "X only needs to happen
> during pg_upgrade" is likely to be wrong. pg_upgrade isn't really
> doing anything especially unusual: just creating some objects and
> loading data. Those things can also be done at other times, so
> whatever is needed during pg_upgrade is also likely to be needed at
> other times. Maybe that's not sound reasoning for some reason or
> other, but that's my intuition.

I assume Alvaro is saying that pg_upgrade has only a single session,
which is unique and might make things easier for him.

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

  Only you can decide what is important to you.

On 2024-May-14, Bruce Momjian wrote:

> Turns out these commits generated a single release note item, which I
> have now removed with the attached committed patch.

Hmm, but the commits about not-null constraints for domains were not
reverted, only the ones for constraints on relations.  I think the
release notes don't properly address the ones on domains.  I think it's
at least these two commits:

> -Author: Peter Eisentraut <peter@eisentraut.org>
> -2024-03-20 [e5da0fe3c] Catalog domain not-null constraints
> -Author: Peter Eisentraut <peter@eisentraut.org>
> -2024-04-15 [9895b35cb] Fix ALTER DOMAIN NOT NULL syntax

It may still be a good idea to make a note about those, at least to
point out that information_schema now lists them.  For example, pg11
release notes had this item

<!--
2018-02-07 [32ff26911] Add more information_schema columns
-->

       <para>
        Add <literal>information_schema</literal> columns related to table
        constraints and triggers (Peter Eisentraut)
       </para>

       <para>
        Specifically,
        <structname>triggers</structname>.<structfield>action_order</structfield>,
        <structname>triggers</structname>.<structfield>action_reference_old_table</structfield>,
        and
        <structname>triggers</structname>.<structfield>action_reference_new_table</structfield>
        are now populated, where before they were always null.  Also,
        <structname>table_constraints</structname>.<structfield>enforced</structfield>
        now exists but is not yet usefully populated.
       </para>


-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On 15.05.24 09:50, Alvaro Herrera wrote:
> On 2024-May-14, Bruce Momjian wrote:
> 
>> Turns out these commits generated a single release note item, which I
>> have now removed with the attached committed patch.
> 
> Hmm, but the commits about not-null constraints for domains were not
> reverted, only the ones for constraints on relations.  I think the
> release notes don't properly address the ones on domains.  I think it's
> at least these two commits:
> 
>> -Author: Peter Eisentraut <peter@eisentraut.org>
>> -2024-03-20 [e5da0fe3c] Catalog domain not-null constraints
>> -Author: Peter Eisentraut <peter@eisentraut.org>
>> -2024-04-15 [9895b35cb] Fix ALTER DOMAIN NOT NULL syntax

I'm confused that these were kept.  The first one was specifically to 
make the catalog representation of domain not-null constraints 
consistent with table not-null constraints.  But the table part was 
reverted, so now the domain constraints are inconsistent again.

The second one refers to the first one, but it might also fix some 
additional older issue, so it would need more investigation.

On Wed, May 15, 2024 at 09:50:36AM +0200, Álvaro Herrera wrote:
> On 2024-May-14, Bruce Momjian wrote:
> 
> > Turns out these commits generated a single release note item, which I
> > have now removed with the attached committed patch.
> 
> Hmm, but the commits about not-null constraints for domains were not
> reverted, only the ones for constraints on relations.  I think the
> release notes don't properly address the ones on domains.  I think it's
> at least these two commits:
> 
> > -Author: Peter Eisentraut <peter@eisentraut.org>
> > -2024-03-20 [e5da0fe3c] Catalog domain not-null constraints
> > -Author: Peter Eisentraut <peter@eisentraut.org>
> > -2024-04-15 [9895b35cb] Fix ALTER DOMAIN NOT NULL syntax
> 
> It may still be a good idea to make a note about those, at least to
> point out that information_schema now lists them.  For example, pg11
> release notes had this item

Let me explain what I did to adjust the release notes.  I took your
commit hashes, which were longer than mine, and got the commit subject
text from them.  I then searched the release notes to see which commit
subjects existed in the document.  Only the first three did, and the
release note item has five commits.

The then tested if the last two patches could be reverted, and 'patch'
thought they could be, so that confirmed they were not reverted.

However, there was no text in the release note item that corresponded to
the commits, so I just removed the entire item.

What I now think happened is that the last two commits were considered
part of the larger NOT NULL change, and not worth mentioning separately,
but now that the NOT NULL part is reverted, we might need to mention
them.

I rarely handle such complex cases so I don't think I was totally
correct in my handling.  Let's get a reply to Peter Eisentraut's
question and we can figure out what to do.

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

  Only you can decide what is important to you.