Thread: Re: Support NOT VALID / VALIDATE constraint options for named NOT NULL constraints

Hello,

Thanks!

I noticed a typo 'constrint' in several places; fixed in the attached.

I discovered that this sequence, taken from added regression tests,

CREATE TABLE notnull_tbl1 (a int);
ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn_parent not null a not valid;
CREATE TABLE notnull_chld (a int);
ALTER TABLE notnull_chld ADD CONSTRAINT nn_child not null a not valid;
ALTER TABLE notnull_chld INHERIT notnull_tbl1;
ALTER TABLE notnull_tbl1 VALIDATE CONSTRAINT nn_parent;

does mark the constraint as validated in the child, but only in
pg_constraint -- pg_attribute continues to be marked as 'i', so if you
try to use it for a PK, it fails:

alter table notnull_chld add constraint foo primary key (a);
ERROR:  column "a" of table "notnull_chld" is marked as NOT VALID NOT NULL constrint

I thought that was going to be a quick fix, so I tried to do so; since
we already have a function 'set_attnotnull', I thought it was the
perfect tool to changing attnotnull.  However, it's not great, because
since that part of the code is already doing the validation, I don't
want it to queue the validation again, so the API needs a tweak; I
changed it to receiving separately which new value to update attnotnull
to, and whether to queue validation.  With that change it works
correctly, but it is a bit ugly at the callers' side.  Maybe it works to
pass two booleans instead?  Please have a look at whether that can be
improved.


I also noticed the addition of function getNNConnameForAttnum(), which
does pretty much the same as findNotNullConstraintAttnum(), only it
ignores all validate constraints instead of ignoring all non-validated
constraints.  So after looking at the callers of the existing function
and wondering which ones of them really wanted only the validated
constraints?  It turns that the answer is none of them.  So I decided to
remove the check for that, and instead we need to add checks to every
caller of both findNotNullConstraintAttnum() and findNotNullConstraint()
so that it acts appropriately when a non-validated constraint is
returned.  I added a few elog(WARNING)s when this happens; running the
tests I notice that none of them fire.  I'm pretty sure this indicates
holes in testing: we have no test cases for these scenarios, and we
should have them for assurance that we're doing the right things.  I
recommend that you go over those WARNINGs, add test cases that make them
fire, and then fix the code so that the test cases do the right thing.
Also, just to be sure, please go over _all_ the callers of
those two functions and make sure all cases are covered by tests that
catch invalid constraints.


I also noticed that in the one place where getNNConnameForAttnum() was
called, we were passing the parent table's column number.  But in child
tables, even in partitions, the column numbers can differ from parent to
children.  So we need to walk down the hierarchy using the column name,
not the column number.  This would have become visible if the test cases
had included inheritance trees with varying column shapes.


The docs continue to say this:
      This form adds a new constraint to a table using the same constraint
      syntax as <link linkend="sql-createtable"><command>CREATE TABLE</command></link>, plus the option <literal>NOT
      VALID</literal>, which is currently only allowed for foreign key
      and CHECK constraints.
which is missing to indicate that NOT VALID is valid for NOT NULL.

Also I think the docs for attnotnull in catalogs.sgml are a bit too
terse; I would write "The value 't' indicates that a not-null constraint
exists for the column; 'i' for an invalid constraint, 'f' for none."
which please feel free to use if you want, but if you want to come up
with your own wording, that's great too.


The InsertOneNull() function used in bootstrap would not test values for
nullness in presence of invalid constraints.  This change is mostly
pro-forma, since we don't expect invalid constraints during bootstrap,
but it seemed better to be tidy.

I have not looked at the pg_dump code yet, so the changes included here
are just pgindent.

Thank you!

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

On Fri, Feb 21, 2025 at 11:43 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> I have not looked at the pg_dump code yet, so the changes included here
> are just pgindent.
>

I ran pg_upgrade suite with the patches. 002_pg_upgrade fails with
following test log
not ok 14 - run of pg_upgrade for new instance
not ok 15 - pg_upgrade_output.d/ removed after pg_upgrade success
# === pg_upgrade logs found - appending to
/build/dev/testrun/pg_upgrade/002_pg_upgrade/log/regress_log_002_pg_upgrade
===
# === appending

/build/dev/testrun/pg_upgrade/002_pg_upgrade/data/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20250221T144705.724/log/pg_upgrade_dump_16384.log
===
# === appending

/build/dev/testrun/pg_upgrade/002_pg_upgrade/data/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20250221T144705.724/log/pg_upgrade_dump_1.log
===
# === appending

/build/dev/testrun/pg_upgrade/002_pg_upgrade/data/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20250221T144705.724/log/pg_upgrade_server.log
===
# === appending

/build/dev/testrun/pg_upgrade/002_pg_upgrade/data/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20250221T144705.724/log/pg_upgrade_utility.log
===
# === appending

/build/dev/testrun/pg_upgrade/002_pg_upgrade/data/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20250221T144705.724/log/pg_upgrade_dump_16387.log
===
# === appending

/build/dev/testrun/pg_upgrade/002_pg_upgrade/data/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20250221T144705.724/log/pg_upgrade_dump_16385.log
===
# === appending

/build/dev/testrun/pg_upgrade/002_pg_upgrade/data/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20250221T144705.724/log/pg_upgrade_dump_16386.log
===
# === appending

/build/dev/testrun/pg_upgrade/002_pg_upgrade/data/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20250221T144705.724/log/pg_upgrade_internal.log
===
# === appending

/build/dev/testrun/pg_upgrade/002_pg_upgrade/data/t_002_pg_upgrade_new_node_data/pgdata/pg_upgrade_output.d/20250221T144705.724/log/pg_upgrade_dump_5.log
===
not ok 16 - check that locales in new cluster match original cluster
ok 17 - dump after running pg_upgrade
not ok 18 - old and new dumps match after pg_upgrade
1..18
# test failed
stderr:
#   Failed test 'run of pg_upgrade for new instance'
#   at /pg/src/bin/pg_upgrade/t/002_pg_upgrade.pl line 478.
#   Failed test 'pg_upgrade_output.d/ removed after pg_upgrade success'
#   at /pg/src/bin/pg_upgrade/t/002_pg_upgrade.pl line 487.
#   Failed test 'check that locales in new cluster match original cluster'
#   at /pg/src/bin/pg_upgrade/t/002_pg_upgrade.pl line 522.
#          got: '0|c|en_US.UTF-8|en_US.UTF-8|'
#     expected: '6|b|C|C|C.UTF-8'
#   Failed test 'old and new dumps match after pg_upgrade'
#   at /pg/src/test/perl/PostgreSQL/Test/Utils.pm line 797.
#          got: '1'
#     expected: '0'
# Looks like you failed 4 tests of 18.

(test program exited with status code 4)
------------------------------------------------------------------------------

--
Best Wishes,
Ashutosh Bapat

On Fri, Feb 21, 2025 at 3:37 PM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
>
>
>
> On Fri, Feb 21, 2025 at 2:59 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>>
>> On Fri, Feb 21, 2025 at 11:43 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>> >
>> > I have not looked at the pg_dump code yet, so the changes included here
>> > are just pgindent.
>> >
>>
>> I ran pg_upgrade suite with the patches. 002_pg_upgrade fails with
>> following test log
>
>
> This is strange because when I run the tests it's all passing for me.
>
> # +++ tap check in src/bin/pg_upgrade +++
> t/001_basic.pl .......... ok
> t/002_pg_upgrade.pl ..... ok
> t/003_logical_slots.pl .. ok
> t/004_subscription.pl ... ok
> All tests successful.
> Files=4, Tests=53, 38 wallclock secs ( 0.01 usr  0.00 sys +  2.88 cusr  2.11 csys =  5.00 CPU)
> Result: PASS
>
> Note: I applied the patch on commit 7202d72787d3b93b692feae62ee963238580c877.

I applied patches on 984410b923263cac901fa81e0efbe523e9c36df3 and I am
using meson.

If I apply your patches, build binaries, I see failure. I reverted
your patches, built binaries, I don't see failure. I apply your
patches again, built binaries, it fails again.

--
Best Wishes,
Ashutosh Bapat

On 2025-Feb-21, Ashutosh Bapat wrote:

> If I apply your patches, build binaries, I see failure. I reverted
> your patches, built binaries, I don't see failure. I apply your
> patches again, built binaries, it fails again.

I can't reproduce the problem either.  Are you running asserts disabled
or enabled?  Can you please share what upgrade problems are reported?
Do you have additional tests in pg_upgrade that aren't in the tree?

I see a nonrepeatable problem under valgrind which I'm going to look
into.

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

On 2025-Feb-21, Alvaro Herrera wrote:

> I see a nonrepeatable problem under valgrind which I'm going to look
> into.

Sorry, pilot error.  The pg_upgrade test works fine under valgrind.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Having your biases confirmed independently is how scientific progress is
made, and hence made our great society what it is today" (Mary Gardiner)

On Fri, Feb 21, 2025 at 6:25 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Feb-21, Ashutosh Bapat wrote:
>
> > If I apply your patches, build binaries, I see failure. I reverted
> > your patches, built binaries, I don't see failure. I apply your
> > patches again, built binaries, it fails again.
>
> I can't reproduce the problem either.  Are you running asserts disabled
> or enabled?  Can you please share what upgrade problems are reported?

I have shared the relevant output from regress_*_ log in an earlier
email. Do you need something else?

> Do you have additional tests in pg_upgrade that aren't in the tree?

No. I did intend to run my dump/restore test but the test even without
those patches applied.

--
Best Wishes,
Ashutosh Bapat

On 2025-Mar-10, Rushabh Lathia wrote:

> I adjusted the set_attnotnull() API and removed the added
> queue_validation parameter.  Rather, the function start using wqueue
> input parameter as a check.
> If wqueue is NULL, skip the queue_validation.  Attaching patch here,
> but not sure how clear it is, in term of understanding the API.  Your
> thoughts/inputs?

Yeah, I think this makes sense, if it supports all the cases correctly.
(If in your code you have any calls of set_attnotnull that pass a NULL
wqueue during ALTER TABLE in order to avoid queueing a check, you had
better have comments to explain this.)

Kindly do not send patches standalone, because the CFbot doesn't
understand that it needs to apply it on top of the three previous
patches.  You need to send all 4 patches every time.  You can see the
result here:
https://commitfest.postgresql.org/patch/5554/
If you click on the "need rebase!" label you'll see that it tried to
apply patch 0004 to the top of the master branch, and that obviously
failed.  (FWIW if you click on the "summary" label you'll also see that
the patch has been failing the CI tests since Feb 27.)

> Looking further for AdjustNotNullInheritance() I don't see need of
> rename this API as it's just adding new error check now for an
> existing NOT NULL constraint.

OK.

> JFYI, I can reproduce the failure Ashutosh Bapat reported, while
> running the pg_upgrade test through meson commands.  I am
> investigating that further.

Ah good, thanks.

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

On Tue, Mar 11, 2025 at 3:46 PM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:

>>
>> > JFYI, I can reproduce the failure Ashutosh Bapat reported, while
>> > running the pg_upgrade test through meson commands.  I am
>> > investigating that further.
>>
>> Ah good, thanks.
>
>
> Somehow, I am now not able to reproduce after the clean build.  Yesterday
> I was able to reproduce, so I was happy, but again trying to analyze the issue
> when I start with the

If the test passes for you, can you please try the patches at [1] on
top of your patches? Please apply those, set and export environment
variable PG_TEST_EXTRA=regress_dump_test, and run 002_pg_upgrade test?
I intended to do this but can not do it since the test always fails
with your patches applied.

[1] https://www.postgresql.org/message-id/CAExHW5uQoyOddBKLBBJpfxXqqok%3DBTeMvt5OpnM6gw0SroiUUw%40mail.gmail.com


--
Best Wishes,
Ashutosh Bapat

On 2025-Mar-12, Ashutosh Bapat wrote:

> If the test passes for you, can you please try the patches at [1] on
> top of your patches? Please apply those, set and export environment
> variable PG_TEST_EXTRA=regress_dump_test, and run 002_pg_upgrade test?
> I intended to do this but can not do it since the test always fails
> with your patches applied.

Oh, I need to enable a PG_TEST_EXTRA option in order for the test to
run?  FFS.  That explains why the tests passed just fine for me.
I'll re-run.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  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 Wed, Mar 12, 2025 at 3:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-12, Ashutosh Bapat wrote:
>
> > If the test passes for you, can you please try the patches at [1] on
> > top of your patches? Please apply those, set and export environment
> > variable PG_TEST_EXTRA=regress_dump_test, and run 002_pg_upgrade test?
> > I intended to do this but can not do it since the test always fails
> > with your patches applied.
>
> Oh, I need to enable a PG_TEST_EXTRA option in order for the test to
> run?  FFS.  That explains why the tests passed just fine for me.
> I'll re-run.

You need PG_TEST_EXTRA to run the testcase I am adding there. Rest of
the testcases run without PG_TEST_EXTRA.

--
Best Wishes,
Ashutosh Bapat

On Wed, Mar 12, 2025 at 3:52 PM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
>
>
> Hi Alvaro,
>
> Here are the latest patches, which includes the regression fix.
>

The 002_pg_upgrade test passes with and without my patches now. But
then the tests added here do not leave behind any parent-child table.
Previously we have found problems in dumping and restoring constraints
in an inheritance hierarchy. I think the test should leave behind all
the combinations of parent and child NOT NULL constraints so that
0002_pg_upgrade can test those.

We should add more scenarios for constraint inheritance. E.g.
#CREATE TABLE notnull_tbl1 (a int);
#ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn_parent not null a not valid;
#CREATE TABLE notnull_chld (a int);
#ALTER TABLE notnull_chld ADD CONSTRAINT nn_child not null a;
#ALTER TABLE notnull_chld INHERIT notnull_tbl1;
#SELECT conname, convalidated FROM pg_catalog.pg_constraint WHERE conrelid
in ('notnull_tbl1'::regclass, 'notnull_chld'::regclass);
  conname  | convalidated
-----------+--------------
 nn_parent | f
 nn_child  | t
(2 rows)

Is it expected that a child may have VALID constraint but parent has
not valid constraint?

Same case with partitioned table. We should leave partitioned table
hierarchy behind for 002_pg_upgrade to test. And we need tests to test
scenarios where a partitioned table has valid constraint but we try to
change constraint on a partition to not valid and vice versa. I think
we shouldn't allow such assymetry in partitioned table hierarchy and
having a test would be better.

--
Best Wishes,
Ashutosh Bapat

On 2025-Mar-12, Ashutosh Bapat wrote:

> The 002_pg_upgrade test passes with and without my patches now. But
> then the tests added here do not leave behind any parent-child table.
> Previously we have found problems in dumping and restoring constraints
> in an inheritance hierarchy. I think the test should leave behind all
> the combinations of parent and child NOT NULL constraints so that
> 0002_pg_upgrade can test those.

I agree.

> Is it expected that a child may have VALID constraint but parent has
> not valid constraint?

Sure.  Consider: if the parent has an unvalidated constraint, we cannot
make any assertions about the state of its children.  The children may
have additional constraints of their own -- in this case, a child can
have a validated constraint even though the parent has none, or only an
unvalidatec constraint.

But the opposite is not allowed: if you know something to be true about
a parent table (to wit: that no row in it is NULL), then this must
necessarily apply to its children as well.  Therefore, if there's a
valid constraint in the parent, then all children must have the same
constraint, and all such constraints must be known valid.

> Same case with partitioned table. We should leave partitioned table
> hierarchy behind for 002_pg_upgrade to test. And we need tests to test
> scenarios where a partitioned table has valid constraint but we try to
> change constraint on a partition to not valid and vice versa. I think
> we shouldn't allow such assymetry in partitioned table hierarchy and
> having a test would be better.

Agreed.

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

On 2025-Mar-12, Rushabh Lathia wrote:

> Hi Alvaro,
> 
> Here are the latest patches, which includes the regression fix.

Thank you.

Taking a step back after discussing this with some colleagues, I need to
contradict what I said at the start of this thread.  There's a worry
that changing pg_attribute.attnotnull in the way I initially suggested
might not be such a great idea after all.  I did a quick search using
codesearch.debian.net for applications reading that column and thinking
about how they would react to this change; I think in the end it's going
to be quite disastrous.  We would break a vast number of these apps, and
there are probably countless other apps and frameworks that we would
also break.  Everybody would hate us forever.  Upgrading to Postgres 18
would become as bad an experience as the drastic change of implicit
casts to text in 8.3.  Nothing else in the intervening 17 years strikes
me as so problematic as this change would be.

So I think we may need to go back and somehow leave pg_attribute alone,
to avoid breaking the whole world.  Maybe it would be sufficient to
change the CompactAttribute->attnotnull to no longer be a boolean, but
the new char representation instead.  I'm not sure if this would
actually work.

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

hi.
I played around with it.

current syntax, we don't need to deal with column constraint grammar.
like the following can fail directly:
create table t0(a int constraint nn not null a not valid);
we only support table constraint cases like:
alter table lp add constraint nc1 not null a not valid;

since CREATE TABLE with invalid constraint does not make sense, so
we can just issue a warning. like:
create table t0(a int, constraint nn not null a not valid);
WARNING:  CREATE TABLE NOT NULL NOT VALID CONSTRAINT WILL SET TO VALID
the wording needs to change...

for not null not valid syntax, we only need to support:
ALTER TABLE ADD CONSTRAINT conname NOT NULL column_name NOT VALID
ALTER TABLE ADD NOT NULL column_name NOT VALID
ALTER TABLE VALIDATE CONSTRAINT conname

The attached is what I came up with:
--------------------------------------------------------------------
ALTER TABLE ADD CONSTRAINT conname NOT NULL column_name NOT VALID
will create an invalidated check constraint. like
ALTER TABLE ADD CONSTRAINT conname CHECK (column_name IS NOT NULL) NOT VALID

when you validate the not-null constraint (internally it's a check constraint)
it will drop the check constraint and install a not-null constraint
with the same name.

drop a check constraint, it will call RemoveConstraintById.
within RemoveConstraintById it will lock pg_constraint.conrelid in
AccessExclusiveLock mode,
which is not ideal, because
ALTER TABLE VALIDATE CONSTRAINT only needs ShareUpdateExclusiveLock.
so we have to find a way to release that AccessExclusiveLock.

because we have converted a not-null constraint to a check constraint,
we need to somehow distinguish this case,
so pg_constraint adds another column: coninternaltype.
(the naming is not good, i guess)
because we dropped a invalid check constraint, but the inherited
constraint cannot be dropped.
so this ALTER TABLE VALIDATE CONSTRAINT will not work for partitions,
but it will work for the root partitioned table. (same logic for table
inheritance).
----------------------------------
demo:

create table t(a int);
alter table t add constraint nc1 not null a not valid;
\d t
                 Table "public.t"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           |          |
Check constraints:
    "nc1" CHECK (a IS NOT NULL) NOT VALID

insert into t default values;
ERROR:  new row for relation "t" violates check constraint "nc1"
DETAIL:  Failing row contains (null).

alter table t validate constraint nc1;
\d+ t
                                            Table "public.t"
 Column |  Type   | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------+--------------+-------------
 a      | integer |           | not null |         | plain   |
    |              |
Not-null constraints:
    "nc1" NOT NULL "a"
Access method: heap

-------------------------------------------------------------------
some regress tests added.
need more polishing, but overall it works as the above described.

not sure if this idea is crazy or not,
what do you think?

On 2025-Mar-17, jian he wrote:

> hi.
> I played around with it.
> 
> current syntax, we don't need to deal with column constraint grammar.
> like the following can fail directly:
> create table t0(a int constraint nn not null a not valid);
> we only support table constraint cases like:
> alter table lp add constraint nc1 not null a not valid;
> 
> since CREATE TABLE with invalid constraint does not make sense, so
> we can just issue a warning. like:
> create table t0(a int, constraint nn not null a not valid);
> WARNING:  CREATE TABLE NOT NULL NOT VALID CONSTRAINT WILL SET TO VALID
> the wording needs to change...

Yeah, we discussed this elsewhere.  I have an alpha-quality patch for
that, but I wasn't too sure about it ...

[1] https://postgr.es/m/CACJufxEQcHNhN6M18JY1mQcgQq9Gn9ofMeop47SdFDE5B8wbug@mail.gmail.com

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

Hello

On 2025-Mar-20, Rushabh Lathia wrote:

> Attached is another version of the patch (WIP), where I have
> introduced a new catalog column, pg_attribute.attinvalidnotnull
> (boolean). This column will default to FALSE but will be set to TRUE
> when an INVALID NOT NULL constraint is created.  With this approach,
> we can avoid performing extra scans on the catalog table to identify
> INVALID NOT NULL constraints, ensuring there is no performance impact.

I find this a reasonable way forward.  We won't impact any applications
that are currently relying on the boolean semantics of attnotnull, while
still allowing the backend to know the real status of the constraint
without having to scan pg_constraint for it.

I also checked the struct layout, which is currently

struct FormData_pg_attribute {
    Oid                        attrelid;             /*     0     4 */
    NameData                   attname;              /*     4    64 */
    /* --- cacheline 1 boundary (64 bytes) was 4 bytes ago --- */
    Oid                        atttypid;             /*    68     4 */
    int16                      attlen;               /*    72     2 */
    int16                      attnum;               /*    74     2 */
    int32                      atttypmod;            /*    76     4 */
    int16                      attndims;             /*    80     2 */
    _Bool                      attbyval;             /*    82     1 */
    char                       attalign;             /*    83     1 */
    char                       attstorage;           /*    84     1 */
    char                       attcompression;       /*    85     1 */
    _Bool                      attnotnull;           /*    86     1 */
    _Bool                      atthasdef;            /*    87     1 */
    _Bool                      atthasmissing;        /*    88     1 */
    char                       attidentity;          /*    89     1 */
    char                       attgenerated;         /*    90     1 */
    _Bool                      attisdropped;         /*    91     1 */
    _Bool                      attislocal;           /*    92     1 */

    /* XXX 1 byte hole, try to pack */

    int16                      attinhcount;          /*    94     2 */
    Oid                        attcollation;         /*    96     4 */

    /* size: 100, cachelines: 2, members: 20 */
    /* sum members: 99, holes: 1, sum holes: 1 */
    /* last cacheline: 36 bytes */
};

Since there's a one-byte hole after the lot of bools/chars, the new bool
will use it and this won't enlarge the storage, neither in memory nor on
disk.

I have not reviewed this patch in detail yet.

Thank you!

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

On Thu, Mar 20, 2025 at 5:54 PM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
>
hi. looking at the regress tests.

+-- verify NOT NULL VALID/NOT VALID
+CREATE TABLE notnull_tbl1 (a INTEGER, b INTEGER);
+INSERT INTO notnull_tbl1 VALUES (NULL, 1);
+INSERT INTO notnull_tbl1 VALUES (NULL, 2);
+INSERT INTO notnull_tbl1 VALUES (300, 3);
+-- Below statement should throw an error
+ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn NOT NULL a;
+ERROR:  column "a" of relation "notnull_tbl1" contains null values
+ALTER TABLE notnull_tbl1 ADD CONSTRAINT nn NOT NULL a NOT VALID;
+\d+ notnull_tbl1
+                               Table "public.notnull_tbl1"
+ Column |  Type   | Collation | Nullable | Default | Storage | Stats
target | Description
+--------+---------+-----------+----------+---------+---------+--------------+-------------
+ a      | integer |           | not null |         | plain   |              |
+ b      | integer |           |          |         | plain   |              |
+Not-null constraints:
+    "nn" NOT NULL "a" NOT VALID

as I mentioned in an offline discussion.
as you can see the output of `\d+ notnull_tbl1`
That means the pg_attribute.attnotnull definition is changed.

current description in
https://www.postgresql.org/docs/current/catalog-pg-attribute.html
attnotnull bool
This represents a not-null constraint.

now the "attnotnull" column means:
This represents a not-null constraint, it may not be validated.
This attribute column may already contain NULLs on it.


so doc/src/sgml/catalogs.sgml the following part will need to adjust
accordingly.

     <row>
      <entry role="catalog_table_entry"><para role="column_definition">
       <structfield>attnotnull</structfield> <type>bool</type>
      </para>
      <para>
       This column has a not-null constraint.
      </para></entry>
     </row>

On 2025-Mar-20, jian he wrote:

> as you can see the output of `\d+ notnull_tbl1`
> That means the pg_attribute.attnotnull definition is changed.

That's correct, it changed in that way.  I propose for the new docs:

>      <row>
>       <entry role="catalog_table_entry"><para role="column_definition">
>        <structfield>attnotnull</structfield> <type>bool</type>
>       </para>
>       <para>
>        This column has a not-null constraint.
>       </para></entry>
>      </row>

"This column has a possibly unvalidated not-null constraint".  The
description for the new column would say "the not-null constraint for
this column is validated" (or the opposite).  My recommendation is to
rename the other column from attinvalidnotnull (Rushabh's proposal) to
"attnotnullvalid" and invert the sense of the boolean.  I think that's
less confusing.

Thanks

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

On Thu, Mar 20, 2025 at 3:25 PM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
>
> Hi Alvaro,
>
> Thank you for the offline discussion.
>
> As we all agree, changing the attnotnull datatype would not be a good idea since it is
> a commonly used catalog column, and many applications and extensions depend on it.
>
> Attached is another version of the patch (WIP), where I have introduced a new catalog column,
> pg_attribute.attinvalidnotnull (boolean). This column will default to FALSE but will be set to TRUE
> when an INVALID NOT NULL constraint is created.  With this approach, we can avoid performing
> extra scans on the catalog table to identify INVALID NOT NULL constraints, ensuring there is no
> performance impact.
>
> Also updated the pg_dump implementation patch and attaching the same here.
>

These patches do not address comments discussed in [1]. Since there
was a change in design, I am assuming that those will be addressed
once the design change is accepted.

[1] https://www.postgresql.org/message-id/202503121157.3zabg6m3anwp@alvherre.pgsql


--
Best Wishes,
Ashutosh Bapat

On Thu, Mar 20, 2025 at 8:20 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> On Thu, Mar 20, 2025 at 3:25 PM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
> >
> > Hi Alvaro,
> >
> > Thank you for the offline discussion.
> >
> > As we all agree, changing the attnotnull datatype would not be a good idea since it is
> > a commonly used catalog column, and many applications and extensions depend on it.
> >
> > Attached is another version of the patch (WIP), where I have introduced a new catalog column,
> > pg_attribute.attinvalidnotnull (boolean). This column will default to FALSE but will be set to TRUE
> > when an INVALID NOT NULL constraint is created.  With this approach, we can avoid performing
> > extra scans on the catalog table to identify INVALID NOT NULL constraints, ensuring there is no
> > performance impact.
> >
> > Also updated the pg_dump implementation patch and attaching the same here.
> >
>
> These patches do not address comments discussed in [1]. Since there
> was a change in design, I am assuming that those will be addressed
> once the design change is accepted.
>
> [1] https://www.postgresql.org/message-id/202503121157.3zabg6m3anwp@alvherre.pgsql

> Is it expected that a child may have VALID constraint but parent has
> not valid constraint?
but the MergeConstraintsIntoExisting logic is when
ALTER TABLE ATTACH PARTITION,
it expects the child table to also have an equivalent constraint
definition on it.
see MergeConstraintsIntoExisting:
            ereport(ERROR,
                    (errcode(ERRCODE_DATATYPE_MISMATCH),
                     errmsg("child table is missing constraint \"%s\"",
                            NameStr(parent_con->conname))));

So I decided not to support it.

main idea:
NOT NULL NOT VALID
* one column one NOT NULL, if you want to change status, it's not
allowed, it will error out, give you hints.
* it will logically be equivalent to CHECK(x IS NOT NULL) NOT VALID.
* it can only be added using ALTER TABLE, not with CREATE TABLE (a
warning will be issued)
* pg_attribute.attinvalidnotnull meaning: this attnum has a
(convalidated == false) NOT NULL pg_constraint entry to it.

* if attnotnull is true, then attinvalidnotnull should be false.
  Conversely, if attinvalidnotnull is true, then attnotnull should be false.
* an invalid not-null cannot be used while adding a primary key.
* if attinvalidnotnull is true, this column can not accept NULL values,
  but the existing column value may contain NULLs, we need to
  VALIDATE the not-null constraint to check if this column exists NULL
values or not.
* partitioned table can not have NOT NULL NOT VALID.

On 2025-Mar-20, jian he wrote:

> > Is it expected that a child may have VALID constraint but parent has
> > not valid constraint?
>
> but the MergeConstraintsIntoExisting logic is when
> ALTER TABLE ATTACH PARTITION,
> it expects the child table to also have an equivalent constraint
> definition on it.
> see MergeConstraintsIntoExisting:
>             ereport(ERROR,
>                     (errcode(ERRCODE_DATATYPE_MISMATCH),
>                      errmsg("child table is missing constraint \"%s\"",
>                             NameStr(parent_con->conname))));
> 
> So I decided not to support it.

> * partitioned table can not have NOT NULL NOT VALID.

I'm not sure I understand what you're saying here.  I think a
partitioned table can be allowed to have a NOT VALID constraint.  BUT if
it does, then all the children must have a corresponding constraint.  The
constraint on children may be valid or may be invalid; the parent
doesn't force the issue one way or the other.  But it has to exist.
Also, if you run ALTER TABLE VALIDATE CONSTRAINT on the parent, then at
that point you have to validate that all those corresponding constraints on
the children are also validated.

> * one column one NOT NULL, if you want to change status, it's not
> allowed, it will error out, give you hints.

I think we discussed this already.  If you say 
ALTER TABLE .. ALTER COLUMN .. SET NOT NULL
and an invalid constraint exists, then we can simply validate that
constraint.

However, if you say
ALTER TABLE .. ADD CONSTRAINT foobar NOT NULL col;
and an invalid constraint exists whose name is different from foobar,
then we should raise an error, because the user's requirement that the
constraint is named foobar cannot be satisfied.  If the constraint is
named foobar, OR if the user doesn't specify a constraint name
  ALTER TABLE .. ADD NOT NULL col;
then it's okay to validate that constraint without raising an error.
The important thing being that the user requirement is satisfied.

> * it can only be added using ALTER TABLE, not with CREATE TABLE (a
> warning will be issued)

I think the issue of adding constraints with NOT VALID during CREATE
TABLE is the topic of another thread.  We already silently ignore the
NOT VALID markers during CREATE TABLE for other types of constraints.

> * pg_attribute.attinvalidnotnull meaning: this attnum has a
> (convalidated == false) NOT NULL pg_constraint entry to it.
> * if attnotnull is true, then attinvalidnotnull should be false.
>   Conversely, if attinvalidnotnull is true, then attnotnull should be false.

I don't like this.  It seems baroque and it will break existing
applications, because they currently query for attnotnull and assume
that inserting a null value will work, but in reality it will fail
because attinvalidnotnull is true (meaning an invalid constraint exists,
which prevents inserting nulls).

I think the idea should be: attnotnull means that a constraint exists;
it doesn't imply anything regarding the constraint being valid or not.
attnotnullvalid will indicate whether the constraint is valid; this
column can only be true if attnotnull is already true.

> * an invalid not-null cannot be used while adding a primary key.

Check.

> * if attinvalidnotnull is true, this column can not accept NULL values,
>   but the existing column value may contain NULLs, we need to
>   VALIDATE the not-null constraint to check if this column exists NULL
> values or not.

Check.


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

On Thu, Mar 20, 2025 at 11:53 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-20, jian he wrote:
>
> > > Is it expected that a child may have VALID constraint but parent has
> > > not valid constraint?
> >
> > but the MergeConstraintsIntoExisting logic is when
> > ALTER TABLE ATTACH PARTITION,
> > it expects the child table to also have an equivalent constraint
> > definition on it.
> > see MergeConstraintsIntoExisting:
> >             ereport(ERROR,
> >                     (errcode(ERRCODE_DATATYPE_MISMATCH),
> >                      errmsg("child table is missing constraint \"%s\"",
> >                             NameStr(parent_con->conname))));
> >
> > So I decided not to support it.
>
> > * partitioned table can not have NOT NULL NOT VALID.
>
> I'm not sure I understand what you're saying here.  I think a
> partitioned table can be allowed to have a NOT VALID constraint.  BUT if
> it does, then all the children must have a corresponding constraint.  The
> constraint on children may be valid or may be invalid; the parent
> doesn't force the issue one way or the other.  But it has to exist.
> Also, if you run ALTER TABLE VALIDATE CONSTRAINT on the parent, then at
> that point you have to validate that all those corresponding constraints on
> the children are also validated.

* if partitioned table have valid not-null, then partition with
invalid not-null can not attach to the partition tree.
  if partitioned table have not valid not-null, we *can* attach a
valid not-null to the partition tree.
  (inheritance hierarchy behaves the same).
this part does not require a lot of code changes.
However, to make the pg_dump working with partitioned table we need to
tweak AdjustNotNullInheritance a little bit.


>
> > * one column one NOT NULL, if you want to change status, it's not
> > allowed, it will error out, give you hints.
>
> I think we discussed this already.  If you say
> ALTER TABLE .. ALTER COLUMN .. SET NOT NULL
> and an invalid constraint exists, then we can simply validate that
> constraint.
>
> However, if you say
> ALTER TABLE .. ADD CONSTRAINT foobar NOT NULL col;
> and an invalid constraint exists whose name is different from foobar,
> then we should raise an error, because the user's requirement that the
> constraint is named foobar cannot be satisfied.  If the constraint is
> named foobar, OR if the user doesn't specify a constraint name
>   ALTER TABLE .. ADD NOT NULL col;
> then it's okay to validate that constraint without raising an error.
> The important thing being that the user requirement is satisfied.
>
i changed this accordingly.
ALTER TABLE .. ALTER COLUMN .. SET NOT NULL
will validate not-null and set attnotnull, attinvalidnotnull accordingly.


> > * pg_attribute.attinvalidnotnull meaning: this attnum has a
> > (convalidated == false) NOT NULL pg_constraint entry to it.
> > * if attnotnull is true, then attinvalidnotnull should be false.
> >   Conversely, if attinvalidnotnull is true, then attnotnull should be false.
>
> I don't like this.  It seems baroque and it will break existing
> applications, because they currently query for attnotnull and assume
> that inserting a null value will work, but in reality it will fail
> because attinvalidnotnull is true (meaning an invalid constraint exists,
> which prevents inserting nulls).
>
> I think the idea should be: attnotnull means that a constraint exists;
> it doesn't imply anything regarding the constraint being valid or not.
> attnotnullvalid will indicate whether the constraint is valid; this
> column can only be true if attnotnull is already true.
>
i basically model NOT NULL NOT VALID == CHECK (x IS NOT NULL).
i think your idea may need more refactoring?
all the "if (attr->attnotnull" need change to "if (attr->attnotnull &&
attr->attnotnullvalid)"
or am i missing something?

Anyway, I will just share my idea first, and will explore your idea later.

in my attached patch, you will only create an not-null not valid
pg_constraint entry
If `if (constr->contype == CONSTR_NOTNULL && constr->skip_validation)`
in ATAddCheckNNConstraint conditions are satisfied.


imho, my approach is less bug-prone, almost no need to refactor current code.
we can even add a assert in InsertPgAttributeTuples:
Assert(!attrs->attinvalidnotnull);


new patch attached:
* Rushabh's pg_dump relation code incorporated into a single one patch.
* pg_dump works fine, mainly by tweak AdjustNotNullInheritance
following the same logic in MergeConstraintsIntoExisting.
if not do it, pg_constraint.conislocal meta info will be wrong.

On 2025-Mar-21, jian he wrote:

> * if partitioned table have valid not-null, then partition with
> invalid not-null can not attach to the partition tree.

Correct.

>   if partitioned table have not valid not-null, we *can* attach a
> valid not-null to the partition tree.

Also correct.

>   (inheritance hierarchy behaves the same).

Good -- it should! :-)

> this part does not require a lot of code changes.
> However, to make the pg_dump working with partitioned table we need to
> tweak AdjustNotNullInheritance a little bit.

Hmm, well, modifying a function to suite what we need it to do is part
of code patching :-)

> i changed this accordingly.
> ALTER TABLE .. ALTER COLUMN .. SET NOT NULL
> will validate not-null and set attnotnull, attinvalidnotnull accordingly.

Okay.


> i basically model NOT NULL NOT VALID == CHECK (x IS NOT NULL).
> i think your idea may need more refactoring?
> all the "if (attr->attnotnull" need change to "if (attr->attnotnull &&
> attr->attnotnullvalid)"
> or am i missing something?

In some places, yes we will need to change like that.  However, many
places do not need to change like that.  In particular, (most?) client
applications do not necessarily need that change, and to me, that's the
most important part, because we do not control external applications,
and --as I said upthread-- we do not have the luxury of breaking them.

> Anyway, I will just share my idea first, and will explore your idea later.

Thank you.

> in my attached patch, you will only create an not-null not valid
> pg_constraint entry
> If `if (constr->contype == CONSTR_NOTNULL && constr->skip_validation)`
> in ATAddCheckNNConstraint conditions are satisfied.
> 
> 
> imho, my approach is less bug-prone, almost no need to refactor current code.

I'll give this a look ... probably won't have time today ...  however,
IMO the consideration of external applications (ORMs, LibreOffice, admin
GUIs, etc) not breaking is the most important thing to keep in mind.
You can go over the code found by codesearch.debian.net when searching
for `attnotnull` to see the sort of code that would be affected.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"¿Qué importan los años?  Lo que realmente importa es comprobar que
a fin de cuentas la mejor edad de la vida es estar vivo"  (Mafalda)

On Wed, Mar 12, 2025 at 2:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Taking a step back after discussing this with some colleagues, I need to
> contradict what I said at the start of this thread.  There's a worry
> that changing pg_attribute.attnotnull in the way I initially suggested
> might not be such a great idea after all.  I did a quick search using
> codesearch.debian.net for applications reading that column and thinking
> about how they would react to this change; I think in the end it's going
> to be quite disastrous.  We would break a vast number of these apps, and
> there are probably countless other apps and frameworks that we would
> also break.  Everybody would hate us forever.  Upgrading to Postgres 18
> would become as bad an experience as the drastic change of implicit
> casts to text in 8.3.  Nothing else in the intervening 17 years strikes
> me as so problematic as this change would be.

I don't agree with this conclusion. The 8.3 casting changes were
problematic because any piece of SQL you'd ever written could have
problems. This change will only break queries that look at the
attnotnull column. While there may be quite a few of those, it can't
possibly be of the same order. I think it's routine that changing the
name or type of system catalog columns breaks things for a few people
(e.g. procpid->pid, or relistemp->relpersistence) and we sometimes get
complaints about that, but at least you can grep for it and it's
mostly going to affect admin tools rather than all the queries
everywhere.

That's not to say that adding a second bool column instead of changing
the existing column's data type is necessarily the wrong way to go.
But I think you're overestimating the blast radius by quite a lot.

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

On 2025-Mar-21, Robert Haas wrote:

> I don't agree with this conclusion.

Uhm.

> The 8.3 casting changes were problematic because any piece of SQL
> you'd ever written could have problems.

Okay, this much I agree with.

> This change will only break queries that look at the attnotnull
> column.  While there may be quite a few of those, it can't possibly be
> of the same order. I think it's routine that changing the name or type
> of system catalog columns breaks things for a few people (e.g.
> procpid->pid, or relistemp->relpersistence) and we sometimes get
> complaints about that, but at least you can grep for it and it's
> mostly going to affect admin tools rather than all the queries
> everywhere.

In several of the cases that I checked, the application just tests the
returned value for boolean truth.  If we change the column from boolean
to char, they would stop working properly because both the 't' and the
'f' char values would test as true.  But suppose we were to rename the
column; that would cause developers to have to examine the code to
determine how to react.  That might even be good, because we're end up
in a situation were no application uses outdated assumptions about
nullness in a column.  However, consider the rationale given in 
https://postgr.es/m/2542644.1733418030@sss.pgh.pa.us
that removing attndims would break PHP -- after that discussion, we
decided against removing the column, even though it's completely
useless, because we don't want to break PHP.  You know, removing
attnotnull would break PHP in exactly the same way, or maybe in some
worse way.  I don't see how can we reach a different conclusion for this
change that for that one.

> That's not to say that adding a second bool column instead of changing
> the existing column's data type is necessarily the wrong way to go.
> But I think you're overestimating the blast radius by quite a lot.

I am just going by some truth established by previous discussion.
If we agree to remove attnotnull or to change the way it works, then we
can also remove attndims.

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

hi.
you may like the attached. it's based on your idea: attnotnullvalid.

I came across a case, not sure if it's a bug.
CREATE TABLE ttchk (a INTEGER);
ALTER TABLE ttchk ADD CONSTRAINT cc check (a is NOT NULL) NOT VALID;
CREATE TABLE ttchk_child(a INTEGER) INHERITS(ttchk);
ttchk_child's constraint cc will default to valid,
but pg_dump && pg_restore will make ttchk_child's constraint invalid.
since it's an existing behavior, so not-null constraint will align with it.
--------------------------------------------------------------------
-----the following text is copied from the commit message------------

NOT NULL NOT VALID

* TODO: In doc/src/sgml/ref/alter_table.sgml, under the
<title>Compatibility</title> section,
  clarify how the "NOT NULL NOT VALID" syntax conforms with the standard.
* TODO: Should CREATE TABLE LIKE copy an existing invalid not-null
constraint to the new table,
  and if so, the new table's not-null will be marked as valid.

description entry of pg_attribute.attnotnullvalid:
+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>attnotnullvalid</structfield> <type>bool</type>
+      </para>
+      <para>
+       The not-null constraint validity status of the column.
+       If true, it means this column has a valid not-null constraint,
+       false means this column doesn't have a not-null constraint or
has an unvalidated one.
+       If <structfield>attnotnull</structfield> is false, this must be false.
       </para></entry>

* attnotnull means that a not-null constraint exists; it doesn't imply anything
  regarding the constraint being valid or not.
  attnotnullvalid will indicate whether the constraint is valid; this column can
  only be true if attnotnull is already true.
  attnotnullvalid only added to FormData_pg_attribute, didn't add to
CompactAttribute.
  mainly because invalid not-null is not being commonly used.
  TupleDesc->TupleConstr->has_not_null now also represents invalid not-null
  constraint.

* For table in pg_catalog schema, if that column attnotnull attribute is true,
 then attnotnullvalid attribute is also true.  Similarly, if
attnotnull is false,
 then attnotnullvalid is false.  I added an SQL check at the end of
 src/test/regress/sql/constraints.sql (not sure it's necessary)

* CREATE TABLE specifying not valid not-null constraint will be set to valid,
  a warning is issued within function transformCreateStmt.
  that means InsertPgAttributeTuples can not insert attribute
  that is (attnotnull && !attnotnullvalid).
 I added an Assert in InsertPgAttributeTuples.
(also added to other places, to demo i didn't mess something, maybe
it's necessary).

* table rewrite won't validate invalid not-null constraint, that is aligned
  with check constraint.

* attnotnullvalid mainly changed in these two places:
  1. ATAddCheckNNConstraint, if you specified "NOT NULL NOT VALID", it
will change
    it from false to false, but will set attnotnull to true.
  2. QueueNNConstraintValidation, subroutine of ATExecValidateConstraint.
    when validing an not valid not-null constraint, toggle it from
false to true,
    also set attnotnull to true.

* A partitioned table can have an invalid NOT NULL constraint while its
  partitions have a valid one, but not the other way around.
  but pg_dump/pg_restore may not preserve the constraint name properly, but
  that's fine for not-null constraint, i think.

* regular table invalid not null constraint pg_dump also works fine.

On Fri, Mar 21, 2025 at 2:04 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> In several of the cases that I checked, the application just tests the
> returned value for boolean truth.  If we change the column from boolean
> to char, they would stop working properly because both the 't' and the
> 'f' char values would test as true.  But suppose we were to rename the
> column; that would cause developers to have to examine the code to
> determine how to react.  That might even be good, because we're end up
> in a situation were no application uses outdated assumptions about
> nullness in a column.

Yes.

> However, consider the rationale given in
> https://postgr.es/m/2542644.1733418030@sss.pgh.pa.us
> that removing attndims would break PHP -- after that discussion, we
> decided against removing the column, even though it's completely
> useless, because we don't want to break PHP.  You know, removing
> attnotnull would break PHP in exactly the same way, or maybe in some
> worse way.  I don't see how can we reach a different conclusion for this
> change that for that one.

Well, that discussion seems awfully weird to me. I can recall plenty
of cases where we've changed stuff in the system catalog and rebuffed
complaints from people who were grumpy about having to adjust their
queries. I don't really understand what makes that case different.

I mean, maybe there's an argument that some changes are more
disruptive than others. For instance, if removing attndims would force
drivers to run extra more complicated queries to learn whether a
certain type is an array type, one could argue that taking it away
without providing some alternative is breaking the drivers in some
fundamental way. I'm not sure whether that's a real problem, but there
is no such argument to be made here. There's no proposal on the table
to entirely remove any information from pg_attribute -- only to turn
something that is presently two-valued into something three-valued.
So, it will still be possible for it to learn the same facts that it
can learn today, it will just perhaps need to be adjusted in terms of
exactly how it does that. But if that is prohibited then what made it
OK all the other times we've done it?

Again, I'm not 100% positive that changing the Boolean column to a
three-valued column is the right way forward, but it does have the
advantage of saving a byte, and the width of system catalog tables has
been a periodic concern. Also, relpersistence is an example of a
seemingly very similar kind of change - turning a Boolean column into
a char column so it can support three values. Do we with the benefit
of hindsight consider that to have been a mistake? I don't, because I
think that PostgreSQL development will be paralyzed if we prohibit
such changes, but opinions might vary.

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

On 2025-Mar-24, Robert Haas wrote:

> I mean, maybe there's an argument that some changes are more
> disruptive than others. For instance, if removing attndims would force
> drivers to run extra more complicated queries to learn whether a
> certain type is an array type, one could argue that taking it away
> without providing some alternative is breaking the drivers in some
> fundamental way. I'm not sure whether that's a real problem, but there
> is no such argument to be made here. There's no proposal on the table
> to entirely remove any information from pg_attribute -- only to turn
> something that is presently two-valued into something three-valued.
> So, it will still be possible for it to learn the same facts that it
> can learn today, it will just perhaps need to be adjusted in terms of
> exactly how it does that. But if that is prohibited then what made it
> OK all the other times we've done it?

I think use of attnotnull is much more prevalent than that of fields
we've removed or changed previously.  In pg_attribute we recently
removed attcacheoff, which pretty much nobody uses; going back to 2016 I
couldn't find any other removal or type change.  In pg_class, I can see
that we got rid of reltoastidxid in 2013, but that one was quite
esoteric and I doubt anybody would be interested in that.  One commit
that I see removed a lot of columns is 739adf32eecf, but that was in
2002.

One we removed not so long ago was protransform, which was turned into
prosupport by commit 1fb57af92069 in 2019.  But I'm sure that
protransform was much less used by external code, because it's very
specialized, so there weren't so many complaints.  We removed
proisagg/proiswindow to turn them into prokind with commit fd1a421fe661
(2018), and you can find plenty of user complaints about that.

Use of attnotnull is very widespread.

> Again, I'm not 100% positive that changing the Boolean column to a
> three-valued column is the right way forward, but it does have the
> advantage of saving a byte, and the width of system catalog tables has
> been a periodic concern.

In this case, as I already said, the new boolean column would go in
what's currently padding space, so there's no widening taking place.

> Also, relpersistence is an example of a seemingly very similar kind of
> change - turning a Boolean column into a char column so it can support
> three values. Do we with the benefit of hindsight consider that to
> have been a mistake?

Are you talking about 5f7b58fad8f4 "Generalize concept of temporary
relations to "relation persistence"." from 2010?  That was 15 years ago,
and maybe it was not a mistake because the ecosystem around Postgres was
different then, but also relistemp/relpersistence is not as widely used
as attnotnull.  There were complaints regarding that change though:
https://www.postgresql.org/message-id/flat/F0ADACAC-15A3-4E5A-A27E-6C9EE090589C@kineticode.com

> I don't, because I think that PostgreSQL development will be paralyzed
> if we prohibit such changes, but opinions might vary.

It's very rare that we need to make catalog changes like this, so I
think you're being overly dramatic.  This is not going to paralyze the
development.  But I agree that we should still allow ourselves to change
system schema, even if we'd break a few things here and there, as long
as it's not the world.


Please don't think that I'm in favor of doing it the difficult way for
no reason.  Previous versions of the patch (changing attnotnull to
char), from the Postgres core code perspective, were pretty much ready
-- we're having this discussion only to avoid the breakage.  I could
save us a bunch of time here and just push those patches, but I think
the responsible thing here (the one we're less likely to regret) is not
to.

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

On Mon, Mar 24, 2025 at 12:29 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> > Again, I'm not 100% positive that changing the Boolean column to a
> > three-valued column is the right way forward, but it does have the
> > advantage of saving a byte, and the width of system catalog tables has
> > been a periodic concern.
>
> In this case, as I already said, the new boolean column would go in
> what's currently padding space, so there's no widening taking place.

IMHO, there will almost certainly be more single-byte columns at some
point in the future, so I feel like using up padding space is not much
different than actual widening over the long term.

> Please don't think that I'm in favor of doing it the difficult way for
> no reason.  Previous versions of the patch (changing attnotnull to
> char), from the Postgres core code perspective, were pretty much ready
> -- we're having this discussion only to avoid the breakage.  I could
> save us a bunch of time here and just push those patches, but I think
> the responsible thing here (the one we're less likely to regret) is not
> to.

Fair enough. I think I've said what I want to say, and I'm not here to
substitute my judgement for yours.

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

On 2025-Mar-24, jian he wrote:

> hi.
> you may like the attached. it's based on your idea: attnotnullvalid.

This is quite close to what I was thinking, yeah.  I noticed a couple of
bugs however, and ended up cleaning up the whole thing.  Here's what I
have so far.  I'm not sure the pg_dump bits are okay (apart from what
you report below) -- I think it's losing the constraint names, which is
of course unacceptable.

I think the warnings about creating constraints as valid when
originating as invalid are unnecessary at this point.  We should add
those, or not, for all constraint types, not just not-null.  That's IMO
a separate discussion.

> I came across a case, not sure if it's a bug.
> CREATE TABLE ttchk (a INTEGER);
> ALTER TABLE ttchk ADD CONSTRAINT cc check (a is NOT NULL) NOT VALID;
> CREATE TABLE ttchk_child(a INTEGER) INHERITS(ttchk);
> ttchk_child's constraint cc will default to valid,
> but pg_dump && pg_restore will make ttchk_child's constraint invalid.
> since it's an existing behavior, so not-null constraint will align with it.

Hmm, yes, such pg_dump behavior would be incorrect.  I'll give the
pg_dump code another look tomorrow.

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

On Fri, Mar 28, 2025 at 3:25 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-24, jian he wrote:
>
> > hi.
> > you may like the attached. it's based on your idea: attnotnullvalid.
>
> This is quite close to what I was thinking, yeah.  I noticed a couple of
> bugs however, and ended up cleaning up the whole thing.  Here's what I
> have so far.  I'm not sure the pg_dump bits are okay (apart from what
> you report below) -- I think it's losing the constraint names, which is
> of course unacceptable.
>

hi.

        /*
         * Update constraint/default info has_not_null also include invalid
         * not-null constraint
         */
this comment needs a period. it should be:
        /*
         * Update constraint/default info. has_not_null also include invalid
         * not-null constraint
         */


gram.y:
                    /* no NOT VALID support yet */
                    processCASbits($4, @4, "NOT NULL",
                                   NULL, NULL, NULL, &n->skip_validation,
                                   &n->is_no_inherit, yyscanner);
                    n->initially_valid = !n->skip_validation;
                    $$ = (Node *) n;
comment  "/* no NOT VALID support yet */" should be removed?


get_relation_constraints:
- if (att->attnotnull && !att->attisdropped)
+ if (att->attnotnull /* && att->attnotnullvalid */ && !att->attisdropped)
looking at how we deal with check constraints,
i think we need
+ if (att->attnotnull && att->attnotnullvalid && !att->attisdropped)


set_attnotnull comments should say something about attnotnullvalid?
since it will change pg_attribute.attnotnullvalid


ATPrepAddPrimaryKey
+ if (!conForm->convalidated)
+ ereport(ERROR,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("not-null constraint \"%s\" of table \"%s\" has not been validated",
+   NameStr(conForm->conname),
+   RelationGetRelationName(rel)),
+ errhint("You will need to use ALTER TABLE ... VALIDATE CONSTRAINT to
validate it."));

I think the error message is less helpful.
Overall, I think we should say that:
to add the primary key on column x requires a validated not-null
constraint on column x.

------------------------------------------------------------------------
i think your patch messed up with pg_constraint.conislocal.
for example:

CREATE TABLE parted (id bigint default 1,id_abc bigint) PARTITION BY LIST (id);
alter TABLE parted add CONSTRAINT dummy_constr not null id not valid;
CREATE TABLE parted_1 (id bigint default 1,id_abc bigint);
alter TABLE parted_1 add CONSTRAINT dummy_constr not null id;
ALTER TABLE parted ATTACH PARTITION parted_1 FOR VALUES IN ('1');

select conrelid::regclass, conname, conislocal
from pg_constraint where conname = 'dummy_constr';

 conrelid |   conname    | conislocal
----------+--------------+------------
 parted   | dummy_constr | t
 parted_1 | dummy_constr | f
(2 rows)


if you  do pg_dump, and execute the pg_dump output
pg_dump  --no-statistics --clean --table-and-children=*parted*
--no-owner --verbose --column-inserts --file=dump.sql --no-acl

select conrelid::regclass, conname, conislocal
from pg_constraint where conname = 'dummy_constr';
output is

 conrelid |   conname    | conislocal
----------+--------------+------------
 parted   | dummy_constr | t
 parted_1 | dummy_constr | t
(2 rows)



because pg_dump will produce
CREATE TABLE public.parted (id bigint DEFAULT 1, id_abc bigint )
PARTITION BY LIST (id);
CREATE TABLE public.parted_1 ( id bigint DEFAULT 1 CONSTRAINT
dummy_constr NOT NULL, id_abc bigint);
ALTER TABLE public.parted ADD CONSTRAINT dummy_constr NOT NULL id NOT VALID;
the third sql command didn't override the parted_1 constraint
dummy_constr  conislocal value.
you may check my code change in AdjustNotNullInheritance

On 2025-Mar-28, jian he wrote:

> i think your patch messed up with pg_constraint.conislocal.
> for example:
> 
> CREATE TABLE parted (id bigint default 1,id_abc bigint) PARTITION BY LIST (id);
> alter TABLE parted add CONSTRAINT dummy_constr not null id not valid;
> CREATE TABLE parted_1 (id bigint default 1,id_abc bigint);
> alter TABLE parted_1 add CONSTRAINT dummy_constr not null id;
> ALTER TABLE parted ATTACH PARTITION parted_1 FOR VALUES IN ('1');
> 
> select conrelid::regclass, conname, conislocal
> from pg_constraint where conname = 'dummy_constr';
> 
>  conrelid |   conname    | conislocal
> ----------+--------------+------------
>  parted   | dummy_constr | t
>  parted_1 | dummy_constr | f
> (2 rows)
> 
> 
> if you  do pg_dump, and execute the pg_dump output
> pg_dump  --no-statistics --clean --table-and-children=*parted*
> --no-owner --verbose --column-inserts --file=dump.sql --no-acl
> 
> select conrelid::regclass, conname, conislocal
> from pg_constraint where conname = 'dummy_constr';
> output is
> 
>  conrelid |   conname    | conislocal
> ----------+--------------+------------
>  parted   | dummy_constr | t
>  parted_1 | dummy_constr | t
> (2 rows)

Interesting.  Yeah, I removed the code you had there because it was
super weird, had no comments, and removing it had zero effect (no tests
failed), so I thought it was useless.  But apparently something is going
on here that's not what we want.

To fix this, we could say that pg_dump should realize the difference and
dump in a different way ... however I think that'd require looking at
conislocal differently, which I definitely don't want to mess with.

Maybe the real problem here is that making the (valid) child constraint
no longer local when the parent constraint is not valid is not sensible,
precisely because pg_dump won't be able to produce good output.  That
sounds more workable to me ... except that we'd have to ensure that
validating the parent constraint would turn the child constraints as not
local anymore, which might be a bit weird.  But maybe not weirder than
the other approach.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"I must say, I am absolutely impressed with what pgsql's implementation of
VALUES allows me to do. It's kind of ridiculous how much "work" goes away in
my code.  Too bad I can't do this at work (Oracle 8/9)."       (Tom Allison)
           http://archives.postgresql.org/pgsql-general/2007-06/msg00016.php

On Sat, Mar 29, 2025 at 2:42 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2025-Mar-28, jian he wrote:
>
> > i think your patch messed up with pg_constraint.conislocal.
> > for example:
> >
> > CREATE TABLE parted (id bigint default 1,id_abc bigint) PARTITION BY LIST (id);
> > alter TABLE parted add CONSTRAINT dummy_constr not null id not valid;
> > CREATE TABLE parted_1 (id bigint default 1,id_abc bigint);
> > alter TABLE parted_1 add CONSTRAINT dummy_constr not null id;
> > ALTER TABLE parted ATTACH PARTITION parted_1 FOR VALUES IN ('1');
> >
> > select conrelid::regclass, conname, conislocal
> > from pg_constraint where conname = 'dummy_constr';
> >
> >  conrelid |   conname    | conislocal
> > ----------+--------------+------------
> >  parted   | dummy_constr | t
> >  parted_1 | dummy_constr | f
> > (2 rows)
> >
> >
> > if you  do pg_dump, and execute the pg_dump output
> > pg_dump  --no-statistics --clean --table-and-children=*parted*
> > --no-owner --verbose --column-inserts --file=dump.sql --no-acl
> >
> > select conrelid::regclass, conname, conislocal
> > from pg_constraint where conname = 'dummy_constr';
> > output is
> >
> >  conrelid |   conname    | conislocal
> > ----------+--------------+------------
> >  parted   | dummy_constr | t
> >  parted_1 | dummy_constr | t
> > (2 rows)
>
> Interesting.  Yeah, I removed the code you had there because it was
> super weird, had no comments, and removing it had zero effect (no tests
> failed), so I thought it was useless.  But apparently something is going
> on here that's not what we want.
>

my change in AdjustNotNullInheritance is copied from
MergeConstraintsIntoExisting
``
            /*
             * OK, bump the child constraint's inheritance count.  (If we fail
             * later on, this change will just roll back.)
             */
            child_copy = heap_copytuple(child_tuple);
            child_con = (Form_pg_constraint) GETSTRUCT(child_copy);
            if (pg_add_s16_overflow(child_con->coninhcount, 1,
                                    &child_con->coninhcount))
                ereport(ERROR,
                        errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
                        errmsg("too many inheritance parents"));
            /*
             * In case of partitions, an inherited constraint must be
             * inherited only once since it cannot have multiple parents and
             * it is never considered local.
             */
            if (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
            {
                Assert(child_con->coninhcount == 1);
                child_con->conislocal = false;
            }
``

if you look at MergeConstraintsIntoExisting, then it won't be weird.
AdjustNotNullInheritance is kind of doing the same thing as
MergeConstraintsIntoExisting, I think.

hi.
in notnull-notvalid.patch

+ if (coninfo->contype == 'c')
+ keyword = "CHECK CONSTRAINT";
+ else
+ keyword = "INVALID NOT NULL CONSTRAINT";

we have a new TocEntry->desc kind.
so the following related code within src/bin/pg_dump also needs change
----------------

                                if (strcmp(te->desc, "CONSTRAINT") == 0 ||
                                    strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
                                    strcmp(te->desc, "FK CONSTRAINT") == 0)
                                    strcpy(buffer, "DROP CONSTRAINT");
                                else
                                    snprintf(buffer, sizeof(buffer), "DROP %s",
                                             te->desc);
----------------
            else if (strcmp(te->desc, "CONSTRAINT") == 0 ||
                     strcmp(te->desc, "CHECK CONSTRAINT") == 0 ||
                     strcmp(te->desc, "FK CONSTRAINT") == 0 ||
                     strcmp(te->desc, "INDEX") == 0 ||
                     strcmp(te->desc, "RULE") == 0 ||
                     strcmp(te->desc, "TRIGGER") == 0)
                te->section = SECTION_POST_DATA;
----------------
    /* these object types don't have separate owners */
    else if (strcmp(type, "CAST") == 0 ||
             strcmp(type, "CHECK CONSTRAINT") == 0 ||
             strcmp(type, "CONSTRAINT") == 0 ||
             strcmp(type, "DATABASE PROPERTIES") == 0 ||
             strcmp(type, "DEFAULT") == 0 ||
             strcmp(type, "FK CONSTRAINT") == 0 ||
             strcmp(type, "INDEX") == 0 ||
             strcmp(type, "RULE") == 0 ||
             strcmp(type, "TRIGGER") == 0 ||
             strcmp(type, "ROW SECURITY") == 0 ||
             strcmp(type, "POLICY") == 0 ||
             strcmp(type, "USER MAPPING") == 0)
    {
        /* do nothing */
    }

On 2025-Mar-31, jian he wrote:

> hi.
> in notnull-notvalid.patch
> 
> + if (coninfo->contype == 'c')
> + keyword = "CHECK CONSTRAINT";
> + else
> + keyword = "INVALID NOT NULL CONSTRAINT";
> we have a new TocEntry->desc kind.

Yeah, I wasn't sure that this change made much actual sense.  I think it
may be better to stick with just CONSTRAINT.  There probably isn't
sufficient reason to have a different ->desc value.

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

On Fri, Mar 28, 2025 at 2:42 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Maybe the real problem here is that making the (valid) child constraint
> no longer local when the parent constraint is not valid is not sensible,
> precisely because pg_dump won't be able to produce good output.  That
> sounds more workable to me ... except that we'd have to ensure that
> validating the parent constraint would turn the child constraints as not
> local anymore, which might be a bit weird.  But maybe not weirder than
> the other approach.

It seems like a bad idea to make conislocal and coninhcount have
anything to do with whether the constraint is valid. We need those to
mean what they have traditionally meant just to make the correct
things happen when the constraint is dropped, either directly from the
child or at some ancestor of that child. If something is dropped at an
ancestor table, we cascade down the tree and decrement coninhcount. If
something is dropped at a child table, we can set conislocal=false.
The constraint goes away, IIUC, when coninhcount=0 and
conislocal=false, which directly corresponds to "this constraint no
longer has a remaining definition either locally or by inheritance". I
don't see how you can change much of anything here without breaking
the existing structure. Validity could have separate tracking of some
sort, possibly as elaborate as convalidlocal and convalidinhcount, but
I don't think it can get away with redefining the tracking that we
already have for existence.

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

On 2025-Mar-28, jian he wrote:

> ATPrepAddPrimaryKey
> + if (!conForm->convalidated)
> + ereport(ERROR,
> + errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
> + errmsg("not-null constraint \"%s\" of table \"%s\" has not been validated",
> +   NameStr(conForm->conname),
> +   RelationGetRelationName(rel)),
> + errhint("You will need to use ALTER TABLE ... VALIDATE CONSTRAINT to
> validate it."));
> 
> I think the error message is less helpful.
> Overall, I think we should say that:
> to add the primary key on column x requires a validated not-null
> constraint on column x.

I think you're right that this isn't saying what the problem is; we
should be saying something like

ERROR:  cannot add primary key because of invalid not-null constraint "the_constr"
HINT:  You will need to use ALTER TABLE .. VALIDATE CONSTRAINT to validate it.

> ------------------------------------------------------------------------
> i think your patch messed up with pg_constraint.conislocal.
> for example:
> 
> CREATE TABLE parted (id bigint default 1,id_abc bigint) PARTITION BY LIST (id);
> alter TABLE parted add CONSTRAINT dummy_constr not null id not valid;
> CREATE TABLE parted_1 (id bigint default 1,id_abc bigint);
> alter TABLE parted_1 add CONSTRAINT dummy_constr not null id;
> ALTER TABLE parted ATTACH PARTITION parted_1 FOR VALUES IN ('1');

It's still not clear to me what to do to fix this problem.  But while
trying to understand it, I had the chance to rework the pg_dump code
somewhat, so here it is.  Feel free to propose fixes on top of this.
(BTW, I think the business of assigning to tbinfo->checkexprs both the
block for check constraints and the one for not-null constraints is
bogus.  I didn't find what this breaks, but it looks wrong.  We probably
need another struct _constraintInfo pointer in TableInfo.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"¿Cómo puedes confiar en algo que pagas y que no ves,
y no confiar en algo que te dan y te lo muestran?" (Germán Poo)

hi.

the following are reviews of changes in pg_dump
on v6-0001-NOT-NULL-NOT-VALID.patch

minor style tweak:
+ "CASE WHEN NOT co.convalidated THEN co.oid"
+ " ELSE NULL END AS notnull_invalidoid,\n"

align with surrounding code convention:
leave white space at the end, not beginning.
maybe we can
+ "CASE WHEN NOT co.convalidated THEN co.oid "
+ "ELSE NULL END AS notnull_invalidoid,\n"


+ * For invalid constraints, we need to store their OIDs for processing
+ * elsewhere, so we bring the pg_constraint.oid value when the constraint
+ * is invalid, and NULL otherwise.
+ *
looking at below surrounding code
    if (fout->remoteVersion >= 180000)
        appendPQExpBufferStr(q,
                             " LEFT JOIN pg_catalog.pg_constraint co ON "
                             "(a.attrelid = co.conrelid\n"
                             "   AND co.contype = 'n' AND "
                             "co.conkey = array[a.attnum])\n");
so this will only apply to the not-null constraint currently.
maybe we should say:
+ * For invalid not-null constraints, we need to store their OIDs for processing


I have some confusion about determineNotNullFlags comments.
 * 3) The column has an invalid not-null constraint.  This must be treated
 *    as a separate object (because it must be created after the table data
 *    is loaded).  So we add its OID to invalidnotnulloids for processing
 *    elsewhere and do nothing further with it here.  We distinguish this
 *    case because the "invalidoid" column has been set to a non-NULL value,
 *    which is the constraint OID.  Valid constraints have a null OID.

The last sentence is not clear to me. Maybe I failed to grasp the
English language implicit
reference. i think it should be:

 * We distinguish this
 *  case because the "notnull_invalidoid" column has been set to a
non-NULL value,
 *   which is the constraint OID. for valid not-null constraints, this
column is NULL value.

determineNotNullFlags comments:
* In case 3 above, the name comparison is a bit of a hack;
should change to
* In case 4 above, the name comparison is a bit of a hack;
?


--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -365,10 +365,11 @@ typedef struct _tableInfo
  * there isn't one on this column. If
  * empty string, unnamed constraint
  * (pre-v17) */
+ bool   *notnull_validated; /* true if NOT NULL is valid */
notnull_validated was never being used, should we remove it?


minor style tweak:
+ "CASE WHEN NOT co.convalidated THEN co.oid"
+ " ELSE NULL END AS notnull_invalidoid,\n"

align with surrounding code convention:
leave white space at the end, not beginning.
maybe we can
+ "CASE WHEN NOT co.convalidated THEN co.oid "
+ "ELSE NULL END AS notnull_invalidoid,\n"


+ * For invalid constraints, we need to store their OIDs for processing
+ * elsewhere, so we bring the pg_constraint.oid value when the constraint
+ * is invalid, and NULL otherwise.
+ *
looking at below surrounding code
    if (fout->remoteVersion >= 180000)
        appendPQExpBufferStr(q,
                             " LEFT JOIN pg_catalog.pg_constraint co ON "
                             "(a.attrelid = co.conrelid\n"
                             "   AND co.contype = 'n' AND "
                             "co.conkey = array[a.attnum])\n");
so this will only apply to the not-null constraint currently.
maybe we should say:
+ * For invalid not-null constraints, we need to store their OIDs for processing


I have some confusion about determineNotNullFlags comments.
 * 3) The column has an invalid not-null constraint.  This must be treated
 *    as a separate object (because it must be created after the table data
 *    is loaded).  So we add its OID to invalidnotnulloids for processing
 *    elsewhere and do nothing further with it here.  We distinguish this
 *    case because the "invalidoid" column has been set to a non-NULL value,
 *    which is the constraint OID.  Valid constraints have a null OID.

The last sentence is not clear to me. Maybe I failed to grasp the
English language implicit
reference. i think it should be:

 * We distinguish this
 *  case because the "notnull_invalidoid" column has been set to a
non-NULL value,
 *   which is the constraint OID. for valid not-null constraints, this
column is NULL value.

determineNotNullFlags comments:
* In case 3 above, the name comparison is a bit of a hack;
should change to
* In case 4 above, the name comparison is a bit of a hack;
?


--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -365,10 +365,11 @@ typedef struct _tableInfo
  * there isn't one on this column. If
  * empty string, unnamed constraint
  * (pre-v17) */
+ bool   *notnull_validated; /* true if NOT NULL is valid */
notnull_validated was never being used, should we remove it?

Hello, thanks for the review.

On 2025-Apr-02, jian he wrote:

> the following are reviews of changes in pg_dump
> on v6-0001-NOT-NULL-NOT-VALID.patch
> 
> minor style tweak:
> + "CASE WHEN NOT co.convalidated THEN co.oid"
> + " ELSE NULL END AS notnull_invalidoid,\n"
> 
> align with surrounding code convention:
> leave white space at the end, not beginning.
> maybe we can
> + "CASE WHEN NOT co.convalidated THEN co.oid "
> + "ELSE NULL END AS notnull_invalidoid,\n"

I put that space there on purpose, actually, because it makes it clear
that the second line is subsidiary to the previous one (a continuation
of the CASE expression).  But you're right, we don't do this elsewhere
in the same query, so I'll use the style you suggest.

> + * For invalid constraints, we need to store their OIDs for processing
> + * elsewhere, so we bring the pg_constraint.oid value when the constraint
> + * is invalid, and NULL otherwise.
> + *
> looking at below surrounding code
>     if (fout->remoteVersion >= 180000)
>         appendPQExpBufferStr(q,
>                              " LEFT JOIN pg_catalog.pg_constraint co ON "
>                              "(a.attrelid = co.conrelid\n"
>                              "   AND co.contype = 'n' AND "
>                              "co.conkey = array[a.attnum])\n");
> so this will only apply to the not-null constraint currently.
> maybe we should say:
> + * For invalid not-null constraints, we need to store their OIDs for processing

Well, the whole comment is talking only about not-null constraints, so
it didn't seem necessary.

> I have some confusion about determineNotNullFlags comments.
>  * 3) The column has an invalid not-null constraint.  This must be treated
>  *    as a separate object (because it must be created after the table data
>  *    is loaded).  So we add its OID to invalidnotnulloids for processing
>  *    elsewhere and do nothing further with it here.  We distinguish this
>  *    case because the "invalidoid" column has been set to a non-NULL value,
>  *    which is the constraint OID.  Valid constraints have a null OID.
> 
> The last sentence is not clear to me. Maybe I failed to grasp the
> English language implicit reference. i think it should be:
> 
>  * We distinguish this
>  *  case because the "notnull_invalidoid" column has been set to a
> non-NULL value,
>  *   which is the constraint OID. for valid not-null constraints, this
> column is NULL value.

I agree with the change of "invalidoid" to "notnull_invalidoid", because
that's the actual name of the column.  But I don't think we need the
second change (from "Valid constraints have..." to "For valid not-null
constraints ...") because the whole function is only concerned with
not-null constraints, and the whole comment is only talking about
not-null constraints, so I think it's quite clear that the constraints
in question are not-null constraints.


> determineNotNullFlags comments:
> * In case 3 above, the name comparison is a bit of a hack;
> should change to
> * In case 4 above, the name comparison is a bit of a hack;
> ?

Ah yes, thanks.


> --- a/src/bin/pg_dump/pg_dump.h
> +++ b/src/bin/pg_dump/pg_dump.h
> @@ -365,10 +365,11 @@ typedef struct _tableInfo
>   * there isn't one on this column. If
>   * empty string, unnamed constraint
>   * (pre-v17) */
> + bool   *notnull_validated; /* true if NOT NULL is valid */
> notnull_validated was never being used, should we remove it?

You're right, thanks.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"This is what I like so much about PostgreSQL.  Most of the surprises
are of the "oh wow!  That's cool" Not the "oh shit!" kind.  :)"
Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php

On 2025-Mar-31, Robert Haas wrote:

> It seems like a bad idea to make conislocal and coninhcount have
> anything to do with whether the constraint is valid. We need those to
> mean what they have traditionally meant just to make the correct
> things happen when the constraint is dropped, either directly from the
> child or at some ancestor of that child. If something is dropped at an
> ancestor table, we cascade down the tree and decrement coninhcount. If
> something is dropped at a child table, we can set conislocal=false.
> The constraint goes away, IIUC, when coninhcount=0 and
> conislocal=false, which directly corresponds to "this constraint no
> longer has a remaining definition either locally or by inheritance". I
> don't see how you can change much of anything here without breaking
> the existing structure. Validity could have separate tracking of some
> sort, possibly as elaborate as convalidlocal and convalidinhcount, but
> I don't think it can get away with redefining the tracking that we
> already have for existence.

Yeah, I think you're (at least partly) right.  I tried a quick
experiment with check constraints on Postgres 10 and I think it behaves
as you say -- namely this script:

create table parted (a int) partition by list (a);
create table part1 partition of parted for values in (1, -1);
alter table part1 add constraint parted_a_check check (a > 0);
alter table parted add constraint parted_a_check check (a > 0) not valid;

results in the constraint in the child having conislocal=false and
pg_dump producing this:

    CREATE TABLE public.parted (
        a integer
    )
    PARTITION BY LIST (a);


    ALTER TABLE public.parted OWNER TO alvherre;

    --
    -- Name: part1; Type: TABLE; Schema: public; Owner: alvherre
    --

    CREATE TABLE public.part1 (
        a integer,
        CONSTRAINT parted_a_check CHECK ((a > 0))
    );
    ALTER TABLE ONLY public.parted ATTACH PARTITION public.part1 FOR VALUES IN (1, '-1');


    ALTER TABLE public.part1 OWNER TO alvherre;

    --
    -- Name: parted parted_a_check; Type: CHECK CONSTRAINT; Schema: public; Owner: alvherre
    --

    ALTER TABLE public.parted
        ADD CONSTRAINT parted_a_check CHECK ((a > 0)) NOT VALID;

I don't quite love this behavior, but since there have been no
complaints, I suppose it's okay and we should just do the same for
not-nulls.


FWIW the part that I think you're not right on, is that constraints on
partitioned tables never have local definitions.  Even if you start with
a constraint defined locally in the partition, the ATTACH operation will
change its conislocal flag to false.  So you can never "drop" it from
the partition.  For regular inheritance, we don't flip the conislocal
flag to false, but you're still prevented from "dropping" the constraint
from the child while the inheritance relationship exists (i.e. you can
never set conislocal=false in such a case).

-- first case
create table parent (a int);
create table child () inherits (parent);
alter table child add constraint parent_a_check check (a > 0);
alter table parent add constraint parent_a_check check (a > 0) not valid;

-- second case
drop table parent, child;
create table parent (a int);
create table child (a int);
alter table child add constraint parent_a_check check (a > 0);
alter table parent add constraint parent_a_check check (a > 0) not valid;
alter table child inherit parent;

In both these cases, the constraint ends up with conislocal=true, but
ALTER TABLE child DROP CONSTRAINT parent_a_check will fail with
ERROR:  cannot drop inherited constraint "parent_a_check" of relation "child"


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

> > ------------------------------------------------------------------------
> > i think your patch messed up with pg_constraint.conislocal.
> > for example:
> >
> > CREATE TABLE parted (id bigint default 1,id_abc bigint) PARTITION BY LIST (id);
> > alter TABLE parted add CONSTRAINT dummy_constr not null id not valid;
> > CREATE TABLE parted_1 (id bigint default 1,id_abc bigint);
> > alter TABLE parted_1 add CONSTRAINT dummy_constr not null id;
> > ALTER TABLE parted ATTACH PARTITION parted_1 FOR VALUES IN ('1');
>
> It's still not clear to me what to do to fix this problem.  But while
> trying to understand it, I had the chance to rework the pg_dump code
> somewhat, so here it is.  Feel free to propose fixes on top of this.

we need special code for handing parent is invalid, child is valid
(table inheritance or partitioning).
To do that we need to check if these valid children have invalid
parent constraints or not.
Therefore another ExecuteSqlQuery query execution is needed.
it may cause performance problems for pg_dump, I guess.
There may be other simple ways to do it.
just sharing what i comp up with.

I also attached a sql test file. i tested all the check constraints,
not-null constraints
where parent is invalid while child is valid scarenio.

then i use
``
create table after_dump as
select conrelid::regclass::text, conname, convalidated, coninhcount,
conislocal, conparentid, contype
from pg_constraint
where conrelid::regclass::text = ANY('{...}';
``
to query before and after dumping whether pg_constraint information
remains as is or not.



The attached patch is based on v6-0001-NOT-NULL-NOT-VALID.patch.
It resolves the issue described in [1] and [2], (sql example demo)
where pg_dump restore failed to retain all pg_constraint properties in cases
where the child constraint was valid while the parent was invalid.

[1]  https://postgr.es/m/CACJufxECVsdWSC4J0wo2LF-+QoacsfX_Scv-NGzQxWjzPF1coA@mail.gmail.com
[2] https://postgr.es/m/CACJufxGnXTj59WM_qqH_JNQ2xC8HQNbJdhAiXnCS2vr3j_17GA@mail.gmail.com

On Wed, Apr 2, 2025 at 5:17 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> I don't quite love this behavior, but since there have been no
> complaints, I suppose it's okay and we should just do the same for
> not-nulls.

I don't understand the issue. It seems like the pg_dump output shown
here would recreate the catalog state.

> FWIW the part that I think you're not right on, is that constraints on
> partitioned tables never have local definitions.  Even if you start with
> a constraint defined locally in the partition, the ATTACH operation will
> change its conislocal flag to false.  So you can never "drop" it from
> the partition.  For regular inheritance, we don't flip the conislocal
> flag to false, but you're still prevented from "dropping" the constraint
> from the child while the inheritance relationship exists (i.e. you can
> never set conislocal=false in such a case).

Hmm. I think this is different from attislocal/attinhcount.

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

On 2025-Apr-02, jian he wrote:

> we need special code for handing parent is invalid, child is valid
> (table inheritance or partitioning).

Hmmmm.  I'm going to focus on this case, which is the simplest one we
care about (no multi-level hierarchy, only not null constraint):

create table singlepp (id bigint default 1) partition by list (id);
alter table singlepp add constraint dummy_constr not null id not valid;
create table singlepp_1 (id bigint default 1);
alter table singlepp_1 add constraint dummy_constr not null id;
alter table singlepp attach partition singlepp_1 for values in ('1');

Here, conislocal for the constraint on singlepp_1 is false.

select conislocal from pg_constraint where conrelid = 'singlepp_1'::regclass;
 conislocal 
────────────
 f

if I run pg_dump and restore in a different database, it emits this:

    CREATE TABLE public.singlepp (
        id bigint DEFAULT 1
    )
    PARTITION BY LIST (id);
    CREATE TABLE public.singlepp_1 (
        id bigint DEFAULT 1 CONSTRAINT dummy_constr NOT NULL
    );
    ALTER TABLE ONLY public.singlepp ATTACH PARTITION public.singlepp_1 FOR VALUES IN ('1');
    ALTER TABLE public.singlepp
        ADD CONSTRAINT dummy_constr NOT NULL id NOT VALID;

If you restore this, you'll get the exact same database.  The only
difference is that conislocal is now true.  If you dump the new
database, it'll produce exactly the same output as above.  You can also
drop the constraint, you'll end up with identical state; or not drop it
an detach the partition instead, and you'll end up with identical state.

If you do pg_upgrade of this cluster, these two databases are identical
to the original ones (down to the conislocal setting on each).

So I'm having a really hard time finding a reason to care about this
conislocal difference.


> Therefore another ExecuteSqlQuery query execution is needed.
> it may cause performance problems for pg_dump, I guess.
> There may be other simple ways to do it.

Yeah, I'm 100% sure we don't want *any* slowdown to get an effect that's
virtually indetectable from the user point of view.

I think one way to fix it might be to make flagInhAttrs do something so
that the conislocal flag is created the other way.  But frankly, I see
little reason to spend time on it.

Do you see anything different?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"You don't solve a bad join with SELECT DISTINCT" #CupsOfFail
https://twitter.com/connor_mc_d/status/1431240081726115845

On Thu, Apr 3, 2025 at 2:24 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> create table singlepp (id bigint default 1) partition by list (id);
> alter table singlepp add constraint dummy_constr not null id not valid;
> create table singlepp_1 (id bigint default 1);
> alter table singlepp_1 add constraint dummy_constr not null id;
> alter table singlepp attach partition singlepp_1 for values in ('1');
>
> Here, conislocal for the constraint on singlepp_1 is false.
>
> select conislocal from pg_constraint where conrelid = 'singlepp_1'::regclass;
>  conislocal
> ────────────
>  f
>
> if I run pg_dump and restore in a different database, it emits this:
>
>         CREATE TABLE public.singlepp (
>             id bigint DEFAULT 1
>         )
>         PARTITION BY LIST (id);
>         CREATE TABLE public.singlepp_1 (
>             id bigint DEFAULT 1 CONSTRAINT dummy_constr NOT NULL
>         );
>         ALTER TABLE ONLY public.singlepp ATTACH PARTITION public.singlepp_1 FOR VALUES IN ('1');
>         ALTER TABLE public.singlepp
>             ADD CONSTRAINT dummy_constr NOT NULL id NOT VALID;
>

Thanks for mentioning flagInhAttrs!
For table partitioning, the V6 pg_dump output is correct.
conislocal's discrepancy in before and after pg_dump can be
fixed(adjust) in AdjustNotNullInheritance.

per above quoted example, The main idea is
ALTER TABLE public.singlepp
    ADD CONSTRAINT dummy_constr NOT NULL id NOT VALID;
will cascade to table singlepp_1 .
However, since singlepp_1 already has a valid NOT NULL constraint,
merging occurs.
like, singlepp_1's coninhcount value increases from 0 to 1.
while at it, we can also set conislocal to false.
with the same idea, the pg_constraint.convalidated discrepancy before
and after pg_dump also resolved.


but we need to change the pg_dump output for table inheritance.
for table inheritance:
CREATE TABLE inhnn (a INTEGER);
ALTER TABLE inhnn ADD CONSTRAINT cc not null a NOT VALID;
CREATE TABLE inhnn_cc(a INTEGER) INHERITS(inhnn);

the V6 output is
CREATE TABLE public.inhnn (a integer);
CREATE TABLE public.inhnn_cc ( a integer) INHERITS (public.inhnn);
ALTER TABLE public.inhnn ADD CONSTRAINT cc NOT NULL a NOT VALID;

we need change it to
CREATE TABLE public.inhnn (a integer);
CREATE TABLE public.inhnn_cc (a integer CONSTRAINT cc NOT NULL)
INHERITS (public.inhnn);
ALTER TABLE public.inhnn ADD CONSTRAINT cc NOT NULL a NOT VALID;

so that after pg_dump we can still have a state where the parent's
constraint is invalid and the child's is valid.



summary:
For parents invalid children valid cases, pg_dump's output changes the
convalidate and conislocal column value.

To resolve this issue:
For table partitioning: V6 pg_dump output works fine, but need change
function AdjustNotNullInheritance
For table inheritance: need change pg_dump output, also change
MergeWithExistingConstraint.


needless to say, attach scratch96.sql  is used to test pg_dump before
and after the difference.
you can compare V6 and my changes.

It occurred to me that we will also want to have NOT NULL NOT ENFORCED 
constraints eventually.  As we have discussed elsewhere, the NOT 
ENFORCED state is closely related to the NOT VALID state.  So that 
should probably be considered in the design here.

Reading up on this again now, I'm confused about putting the NOT VALID 
state for not-null constraints into pg_attribute.  We have catalogued 
not-null constraints now, so we can put metadata for them into 
pg_constraint!  And we have NOT VALID and NOT ENFORCED flags in 
pg_constraint already.

So what is the purpose of the attnotnullvalid field?  In the latest 
posted patch, I don't see this column used in the executor for the 
actual constraint checking.  So is this all merely for clients to 
understand the constraint metadata?  If we add more metadata for 
not-null constraints, do we need to add a new pg_attribute flag for each 
one?  That doesn't seem right.

On 2025-Apr-03, Peter Eisentraut wrote:

> It occurred to me that we will also want to have NOT NULL NOT ENFORCED
> constraints eventually.  As we have discussed elsewhere, the NOT
> ENFORCED state is closely related to the NOT VALID state.  So that
> should probably be considered in the design here.

Yeah, I don't think there's time to shoehorn NOT ENFORCED status for
not-null constraints.  I'd guess that it'd take at least a couple of
weeks to make that work.

> Reading up on this again now, I'm confused about putting the NOT VALID
> state for not-null constraints into pg_attribute.  We have catalogued
> not-null constraints now, so we can put metadata for them into
> pg_constraint!  And we have NOT VALID and NOT ENFORCED flags in
> pg_constraint already.
> 
> So what is the purpose of the attnotnullvalid field?  In the latest posted
> patch, I don't see this column used in the executor for the actual
> constraint checking.  So is this all merely for clients to understand the
> constraint metadata?  If we add more metadata for not-null constraints, do
> we need to add a new pg_attribute flag for each one?  That doesn't seem
> right.

The new flag is there for quick access by get_relation_info.  We could
easily not have it otherwise, because clients don't need it, but its
lack would probably make planning measurably slower because it'd have to
do syscache access for every single not-null constraint to figure out if
it's valid or not.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)

On 03.04.25 10:07, Alvaro Herrera wrote:
> The new flag is there for quick access by get_relation_info.  We could
> easily not have it otherwise, because clients don't need it, but its
> lack would probably make planning measurably slower because it'd have to
> do syscache access for every single not-null constraint to figure out if
> it's valid or not.

In the v6 patch, you are adding a attnullability field to the 
CompactAttribute in the tuple descriptor and use that in 
get_relation_info().  That seems like the right approach, because then 
you're doing all that preprocessing about which constraint is active in 
the relcache.  So I don't see where the extra pg_attribute field 
attnotnullvalid is getting used.