Horton

Re: cataloguing NOT NULL constraints

Zhihong Yu <zyu@yugabyte.com>

View thread
From: Zhihong Yu <zyu@yugabyte.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Amit Langote <amitlangote09@gmail.com>, Laurenz Albe <laurenz.albe@cybertec.at>, Pg Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2022-08-31T23:08:52Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Revert structural changes to not-null constraints

  2. Fix inconsistencies in error messages

  3. Disallow direct change of NO INHERIT of not-null constraints

  4. Disallow NO INHERIT not-null constraints on partitioned tables

  5. Better handle indirect constraint drops

  6. Don't try to assign smart names to constraints

  7. Fix restore of not-null constraints with inheritance

  8. ATTACH PARTITION: Don't match a PK with a UNIQUE constraint

  9. Fix propagating attnotnull in multiple inheritance

  10. Check stack depth in new recursive functions

  11. Move privilege check to the right place

  12. Update information_schema definition for not-null constraints

  13. Fix not-null constraint test

  14. Disallow changing NO INHERIT status of a not-null constraint

  15. Catalog not-null constraints

  16. parallel_schedule: add comment on event_trigger test dependency

  17. Revert "Catalog NOT NULL constraints" and fallout

  18. Adjust contrib/sepgsql regression test expected outputs.

  19. Fix table name clash in recently introduced test

  20. Catalog NOT NULL constraints

  21. Change the rules for inherited CHECK constraints to be essentially the same

On Wed, Aug 31, 2022 at 3:19 PM Alvaro Herrera <alvherre@alvh.no-ip.org>
wrote:

> So I was wrong in thinking that "this case was simple to implement" as I
> replied upthread.  Doing that actually required me to rewrite large
> parts of the patch.  I think it ended up being a good thing, because in
> hindsight the approach I was using was somewhat bogus anyway, and the
> current one should be better.  Please find it attached.
>
> There are still a few problems, sadly.  Most notably, I ran out of time
> trying to fix a pg_upgrade issue with pg_dump in binary-upgrade mode.
> I have to review that again, but I think it'll need a deeper rethink of
> how we pg_upgrade inherited constraints.  So the pg_upgrade tests are
> known to fail.  I'm not aware of any other tests failing, but I'm sure
> the cfbot will prove me wrong.
>
> I reluctantly added a new ALTER TABLE subcommand type, AT_SetAttNotNull,
> to allow setting pg_attribute.attnotnull without adding a CHECK
> constraint (only used internally).  I would like to find a better way to
> go about this, so I may remove it again, therefore it's not fully
> implemented.
>
> There are *many* changed regress expect files and I didn't carefully vet
> all of them.  Mostly it's the addition of CHECK constraints in the
> footers of many \d listings and stuff like that.  At a quick glance they
> appear valid, but I need to review them more carefully still.
>
> We've had pg_constraint.conparentid for a while now, but for some
> constraints we continue to use conislocal/coninhcount.  I think we
> should get rid of that and rely on conparentid completely.
>
> An easily fixed issue is that of constraint naming.
> ChooseConstraintName has an argument for passing known constraint names,
> but this patch doesn't use it and it must.
>
> One issue that I don't currently know how to fix, is the fact that we
> need to know whether a column is a row type or not (because they need a
> different null test).  At table creation time that's easy to know,
> because we have the descriptor already built by the time we add the
> constraints; but if you do ALTER TABLE .. ADD COLUMN .., ADD CONSTRAINT
> then we don't.
>
> Some ancient code comments suggest that allowing a child table's NOT
> NULL constraint acquired from parent shouldn't be independently
> droppable.  This patch doesn't change that, but it's easy to do if we
> decide to.  However, that'd be a compatibility break, so I'd rather not
> do it in the same patch that introduces the feature.
>
> Overall, there's a lot more work required to get this to a good shape.
> That said, I think it's the right direction.
>
> --
> Álvaro Herrera               48°01'N 7°57'E  —
> https://www.EnterpriseDB.com/
> "La primera ley de las demostraciones en vivo es: no trate de usar el
> sistema.
> Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)
>

Hi,
For findNotNullConstraintAttnum():

+       if (multiple == NULL)
+           break;

Shouldn't `pfree(arr)` be called before breaking ?

+static Constraint *makeNNCheckConstraint(Oid nspid, char *constraint_name,

You used `NN` because there is method makeCheckNotNullConstraint, right ?
I think it would be better to expand `NN` so that its meaning is easy to
understand.

Cheers