From: | Sergei Kornilov <sk(at)zsrv(dot)org> |
---|---|
To: | David Rowley <david(dot)rowley(at)2ndquadrant(dot)com> |
Cc: | PostgreSQL mailing lists <pgsql-hackers(at)postgresql(dot)org>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Stephen Frost <sfrost(at)snowman(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, Ildar Musin <i(dot)musin(at)postgrespro(dot)ru>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Subject: | Re: using index or check in ALTER TABLE SET NOT NULL |
Date: | 2019-03-05 19:41:09 |
Message-ID: | 9345791551814869@myt2-cd7fa496c4f7.qloud-c.yandex.net |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
Attachment | Content-Type | Size |
---|---|---|
0001_alter_table_set_not_null_v11.patch | text/x-diff | 14.2 KB |
0002_alter_table_set_not_null_v11_debuglevel_tests.patch | text/x-diff | 5.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Paul Ramsey | 2019-03-05 19:58:08 | Re: Allowing extensions to supply operator-/function-specific info |
Previous Message | legrand legrand | 2019-03-05 19:13:09 | Re: any plan to support shared servers like Oracle in PG? |