Re: using index or check in ALTER TABLE SET NOT NULL

Hello, David!

> I've made a pass over v10. I think it's in pretty good shape, but I
> did end up changing a few small things.

Thank you! I merged your changes to new patch version.

> The only thing that I'm a bit unsure of is the tests. I've read the
> thread and I see the discussion above about it. I'd personally have
> thought INFO was fine since ATTACH PARTITION does that, but I see
> objections.

It is unclear for me if we have consensus about INFO ereport in ATTACH PARTITION.

> It appears that all the tests just assume that the CHECK
> constraint was used to validate the SET NOT NULL. I'm not all that
> sure if there's any good reason not to set client_min_messages =
> 'debug1' just before your test then RESET client_min_messages;
> afterwards. No other tests seem to do it, but my only thoughts on the
> reason not to would be that it might fail if someone added another
> debug somewhere, but so what? they should update the expected results
> too.

Not sure we have consensus about debug messages in tests, so I did such test changes in additional patch.

> separate that part out and
> put back the function named PartConstraintImpliedByRelConstraint and
> have it do the IS NOT NULL building before calling
> ConstraintImpliedByRelConstraint(). No duplicate code that way and you
> also don't need to touch partbound.c

In this case we need extra argument for ConstraintImpliedByRelConstraint for some caller-provided existConstraint, right? Along with Relation itself? Then I need make copy of existConstraint, append relation constraints and call predicate_implied_by. If I understood correctly pseudocode would be:

PartConstraintImpliedByRelConstraint(rel, testConstraint)
  generate notNullConstraint from NOT NULL table attributes
  call ConstraintImpliedByRelConstraint(rel, testConstraint, notNullConstraint)

ConstraintImpliedByRelConstraint(rel, testConstraint, existsConstraint)
  copy existsConstraint to localExistsConstraint variable
  append relation constraints to localExistsConstraint list
  call predicate_implied_by
  
I thing redundant IS NOT NULL check is not issue here and we not need extra arguments for ConstraintImpliedByRelConstraint. Was not changed in attached version, but I will change if you think this would be better design.

regards, Sergei