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

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

In response to

Responses

Browse pgsql-hackers by date

  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?