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
betterdesign.
regards, Sergei
