> On Thu, Nov 22, 2018 at 6:04 PM Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> Absolutely agree. Looking at the history of the patch I see that it went
> through some extensive review and even was marked as Ready for Committer before
> the commentary from Peter, but since then some changes were made (rebase and
> code reorganization). Can someone from the reviewers confirm (or not) if the
> patch is in a good shape? If no, then I'll try to allocate some time for that,
> but can't promise any exact date.
Ok, I've reviewed this patch a bit. I don't see any significant problems with
the code itself, but to be honest I've got an impression that simplicity of
this patch is misleading and it covers too narrow use case. E.g. one can do
something equal to SET NOT NULL, like adding a new constraint
CREATE TABLE test(data text, CHECK(data IS NOT NULL));
ALTER TABLE test ADD CONTSTRAINT data_chk CHECK (data is not null);
or use not null domain
CREATE DOMAIN not_null AS text CHECK(VALUE IS NOT NULL);
CREATE TABLE test(data not_null);
ALTER TABLE test ALTER COLUMN data SET NOT NULL;
and it still leads to a full table scan. For foreign tables the control flow
jump over NotNullImpliedByRelConstraints, so this logic is never checked. I'm
not sure if taking care of mentioned examples is enough (maybe there are more
of them), or the patch needs to suggest some more general solution here.
Of course there is a possibility that improvement even in this form for this
narrow use case is better than potentially more general approach in the future.
Any opinions about this?