Support NOT VALID / VALIDATE constraint options for named NOT NULL constraints

Hello Rushabh,

On 2025-Feb-06, Rushabh Lathia wrote:

Commit 14e87ffa5c543b5f30ead7413084c25f7735039f
<https://github.com/postgres/postgres/commit/14e87ffa5c543b5f30ead7413084c25f7735039f&gt;
added the support for named NOT NULL constraints. We can now support
the NOT VALID/VALID named NOT NULL constraints.

This patch supports the NOT VALID and VALIDATE CONSTRAINT for name NOT
NULL constraints. In order to achieve this patch,

Thank you very much for working on this.

1) Converted the pg_attribute.attnotnull to CHAR type, so that it can
hold the INVALID flag for the constraint.

This looks good to me. It'll have implications for client-side queries,
but I think they will need to adapt. One school of thought says we
should rename the column, so that every tool is forced to think about
the issue and adapt accordingly, instead of only realizing the problem
the first time they break.

4) Added related testcases.

Please remember to add test cases for tables with not-valid constraint
that are not dropped at the end. That way, the pg_upgrade test will try
to process that table and we'll know if the roundtrip via pg_dump works
correctly.

I haven't looked at 0002 too closely, but I think it has the right
shape.

3) Support for pg_dump, where we now dumping the INVALID NOT NULL as
separate from table schemes, just like CHECK Constraints.

I think you copied a little bit too much of the code for check
constraints. If a constraint is accumulated in invalidnotnulloids, you
already know that it's not validated and needs to be dumped separately.
So your new query doesn't need to bring convalidated (we know it's
false). This would simplify a few lines in this new code. Also, the
pg_log_info() line is mistaken about what this block is doing.

I think it'd be good to have NOT VALID NO INHERIT constraints in the
tests as well. Also consider the case where the child table is created
first with a valid constraint, then the parent table is created later
with a not valid constraint -- if the pg_dump table scans find the child
first, does pg_dump do the right thing or does it try to create the
parent constraint first? Also, what if the constraint in the child has
a different name from the constraint in the parent? This should be
pg_dump round-tripped as well. (I bet there are tons of other corner
cases here that should be verified.) Please add something to
pg_dump/t/002_pg_dump.pl.

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

Recursion to child tables is incorrectly trying to locate the constraint
by name:

create table notnull_tbl1 (a int);
alter table notnull_tbl1 add constraint foo not null a not valid;
create table notnull_chld (a int);
alter table notnull_chld add constraint blah not null a not valid;
alter table notnull_chld inherit notnull_tbl1 ;

-- this fails but shouldn't:
alter table notnull_tbl1 validate constraint foo;
ERROR: constraint "foo" of relation "notnull_chld" does not exist

The end result here should be that the constraint `blah` in table
notnull_chld is marked as validated.

--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

Thanks Alvaro.

On Thu, Feb 6, 2025 at 9:58 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Hello Rushabh,

On 2025-Feb-06, Rushabh Lathia wrote:

Commit 14e87ffa5c543b5f30ead7413084c25f7735039f
<

https://github.com/postgres/postgres/commit/14e87ffa5c543b5f30ead7413084c25f7735039f

added the support for named NOT NULL constraints. We can now support
the NOT VALID/VALID named NOT NULL constraints.

This patch supports the NOT VALID and VALIDATE CONSTRAINT for name NOT
NULL constraints. In order to achieve this patch,

Thank you very much for working on this.

1) Converted the pg_attribute.attnotnull to CHAR type, so that it can
hold the INVALID flag for the constraint.

This looks good to me. It'll have implications for client-side queries,
but I think they will need to adapt. One school of thought says we
should rename the column, so that every tool is forced to think about
the issue and adapt accordingly, instead of only realizing the problem
the first time they break.

I am open to suggestions here.

4) Added related testcases.

Please remember to add test cases for tables with not-valid constraint
that are not dropped at the end. That way, the pg_upgrade test will try
to process that table and we'll know if the roundtrip via pg_dump works
correctly.

Sure, will cover this.

I haven't looked at 0002 too closely, but I think it has the right
shape.

3) Support for pg_dump, where we now dumping the INVALID NOT NULL as
separate from table schemes, just like CHECK Constraints.

I think you copied a little bit too much of the code for check
constraints. If a constraint is accumulated in invalidnotnulloids, you
already know that it's not validated and needs to be dumped separately.
So your new query doesn't need to bring convalidated (we know it's
false). This would simplify a few lines in this new code. Also, the
pg_log_info() line is mistaken about what this block is doing.

Even though we are accumulating invalidnotnulloids, we need the condition
for the invalid not null constraint, as there can me multiple not null
constraint
on the table - mix of valid and invalid.

Initially, I tried re-using the code but it was not very clear, so thought
of
making at separate code for check and not null constraint. That way it's
very clear.

I think it'd be good to have NOT VALID NO INHERIT constraints in the
tests as well.

Sure, will take care in the new version of the patch.

Also consider the case where the child table is created
first with a valid constraint, then the parent table is created later
with a not valid constraint -- if the pg_dump table scans find the child
first, does pg_dump do the right thing or does it try to create the
parent constraint first?

yes, I tested this with the patch and pg_dump doing the right this
for that scenario.

Also, what if the constraint in the child has
a different name from the constraint in the parent? This should be
pg_dump round-tripped as well. (I bet there are tons of other corner
cases here that should be verified.) Please add something to
pg_dump/t/002_pg_dump.pl.

Okay, I will add tests to pg_dump/t/002_pg_dump.pl.

regards.
Rushabh Lathia
www.EnterpriseDB.com

On Fri, Feb 7, 2025 at 4:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Recursion to child tables is incorrectly trying to locate the constraint
by name:

create table notnull_tbl1 (a int);
alter table notnull_tbl1 add constraint foo not null a not valid;
create table notnull_chld (a int);
alter table notnull_chld add constraint blah not null a not valid;
alter table notnull_chld inherit notnull_tbl1 ;

-- this fails but shouldn't:
alter table notnull_tbl1 validate constraint foo;
ERROR: constraint "foo" of relation "notnull_chld" does not exist

The end result here should be that the constraint `blah` in table
notnull_chld is marked as validated.

Yes, I agree. Here we need a separate Queue for NotNull constraint
validation,
which fetches the respective Non-Validate-Not-Null constraint name from
the child table

I am working on the patch and will post the update patch soon.

--
Álvaro Herrera 48°01'N 7°57'E —
https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

--
Rushabh Lathia

Hi Alvaro,

I have incorporated the suggested changes, and here is the latest version
of the patch:

- Added more test cases to the regression suite.
- Included tests in the pg_dump test.
- Left objects with *INVALID NOT NULL* for pg_upgrade.
- Fixed an issue where recursion to child tables was incorrectly
attempting to locate the constraint by name.
- Introduced a new function, QueueNNConstraintValidation(), for
handling *NOT
NULL* constraints.

The only remaining task for this patch is updating the documentation. I
will work on that and submit the final version soon.

Please share your review comments.
Thanks,

On Mon, Feb 10, 2025 at 12:28 AM Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

On Fri, Feb 7, 2025 at 4:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Recursion to child tables is incorrectly trying to locate the constraint
by name:

create table notnull_tbl1 (a int);
alter table notnull_tbl1 add constraint foo not null a not valid;
create table notnull_chld (a int);
alter table notnull_chld add constraint blah not null a not valid;
alter table notnull_chld inherit notnull_tbl1 ;

-- this fails but shouldn't:
alter table notnull_tbl1 validate constraint foo;
ERROR: constraint "foo" of relation "notnull_chld" does not exist

The end result here should be that the constraint `blah` in table
notnull_chld is marked as validated.

Yes, I agree. Here we need a separate Queue for NotNull constraint
validation,
which fetches the respective Non-Validate-Not-Null constraint name from
the child table

I am working on the patch and will post the update patch soon.

--
Álvaro Herrera 48°01'N 7°57'E —
https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

--
Rushabh Lathia

--
Rushabh Lathia
www.EnterpriseDB.com

On Mon, Feb 10, 2025 at 10:48 PM Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

Hi Alvaro,

I have incorporated the suggested changes, and here is the latest version
of the patch:

- Added more test cases to the regression suite.
- Included tests in the pg_dump test.
- Left objects with *INVALID NOT NULL* for pg_upgrade.
- Fixed an issue where recursion to child tables was incorrectly
attempting to locate the constraint by name.
- Introduced a new function, QueueNNConstraintValidation(), for
handling *NOT NULL* constraints.

The only remaining task for this patch is updating the documentation. I
will work on that and submit the final version soon.

Attaching documentation and tab-complete patch here.

Please share your review comments.
Thanks,

On Mon, Feb 10, 2025 at 12:28 AM Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

On Fri, Feb 7, 2025 at 4:44 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

Recursion to child tables is incorrectly trying to locate the constraint
by name:

create table notnull_tbl1 (a int);
alter table notnull_tbl1 add constraint foo not null a not valid;
create table notnull_chld (a int);
alter table notnull_chld add constraint blah not null a not valid;
alter table notnull_chld inherit notnull_tbl1 ;

-- this fails but shouldn't:
alter table notnull_tbl1 validate constraint foo;
ERROR: constraint "foo" of relation "notnull_chld" does not exist

The end result here should be that the constraint `blah` in table
notnull_chld is marked as validated.

Yes, I agree. Here we need a separate Queue for NotNull constraint
validation,
which fetches the respective Non-Validate-Not-Null constraint name from
the child table

I am working on the patch and will post the update patch soon.

--
Álvaro Herrera 48°01'N 7°57'E —
https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

--
Rushabh Lathia

--
Rushabh Lathia
www.EnterpriseDB.com

--
Rushabh Lathia

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

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

--
Rushabh Lathia

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

Thank Alvaro for the fixup patch.

On Fri, Feb 21, 2025 at 11:43 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

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 haven't given much thought to improving the API, but I'll look into it
now. Also,
I'm considering renaming AdjustNotNullInheritance() since it now also
checks for invalid NOT NULL constraints. What do you think?

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 reviewed the added WARNING in the fixup patch and addressed the case in
AdjustNotNullInheritance() and ATExecSetNotNull(). I also added the related
test case to the test suite.

Regarding the WARNING in MergeAttributeIntoExisting(), it won’t be
triggered
since this block executes only when the child table has
ATTRIBUTE_NOTNULL_FALSE.
Let me know if I’m missing anything.

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.

Done changes, as per the suggestions.

Regards,
Rushabh Lathia
www.EnterpriseDB.com

On Thu, Feb 27, 2025 at 3:25 PM Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:

Thank Alvaro for the fixup patch.

On Fri, Feb 21, 2025 at 11:43 AM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

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 haven't given much thought to improving the API, but I'll look into it
now. Also,
I'm considering renaming AdjustNotNullInheritance() since it now also
checks for invalid NOT NULL constraints. What do you think?

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?

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.

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

Thanks,
Rushabh Lathia

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 Mon, Mar 10, 2025 at 5:20 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

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

Done.

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

Sure, attaching the rebased patch here.

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.

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

Thanks,
Rushabh Lathia
www.EnterpriseDB.com

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]/messages/by-id/CAExHW5uQoyOddBKLBBJpfxXqqok=BTeMvt5OpnM6gw0SroiUUw@mail.gmail.com 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]: /messages/by-id/CAExHW5uQoyOddBKLBBJpfxXqqok=BTeMvt5OpnM6gw0SroiUUw@mail.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)