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)