Re: Support NOT VALID / VALIDATE constraint options for named NOT NULL constraints - Mailing list pgsql-hackers

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/