| From: | Nitin Motiani <nitinmotiani(at)google(dot)com> |
|---|---|
| To: | Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> |
| Cc: | David Rowley <dgrowleyml(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org |
| Subject: | Re: [PATCH v1] Fix for bug #19770 : Reject NO INHERIT not-null constraints copied by LIKE to partitioned tables |
| Date: | 2026-10-22 12:48:17 |
| Message-ID: | CAH5HC97RWh3Bn=otf6otvKX7GjYAaUQni+nPS_tghfeat3P1TQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On Oct 22, 2026, at 10:55, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> Looking at the test added in cacef1722, we do seem to expect that a NO
> INHERIT CHECK constraint will block the creation of a partitioned
> table with CREATE TABLE LIKE, so you could argue that it makes sense
> to do the same for NOT NULL constraints. If we were to go down that
> route, then I think the correct place to put the check would be in
> StoreRelNotNull() as the neighbouring StoreConstraints() is where
> cacef1722's test hits the error for the CHECK constraint. Putting it
> there also helps ensure there's no other place that could bypass this
> check.
>
Thanks David for looking into this. StoreRelNotNull looks like the
better spot for this.
> I've included a patch which puts a new error check in
> StoreRelNotNull() and spent some time thinking about the best way to
> resolve databases with wrongly set connoinherit flags for partitioned
> tables. If those are not fixed then dump/restore will fail as the
> CREATE TABLE will include a " NO INHERIT" for the NOT NULL
> constraint(s) for the partitioned table. I added some advice in the
> commit message which basically gives a query to find these, then goes
> on to mention using DROP NOT NULL to get rid of them and ALTER
> CONSTRAINT ... INHERIT to fix them.
>
Should the release notes also explicitly state that NULLs must be
found and fixed directly on the partitions, because a 'delete ...
where col is null' on the parent table would not delete anything?
> We could also entertain the idea of forcing these to connoinherit =
> false or ignoring no inherit constraints
> (RelationGetNotNullConstraints() does have a flag for that), but that
> could always be done at a later date when there is more time to
> consider any unwanted side-effects of that.
>
I had considered that, but it didn't seem right to silently drop a
constraint or change its connoinherit when the semantics of LIKE
suggest that it should be the same as the source. If something like
this is done in the future, it should also give a NOTICE/WARNING to
the user.
> The attached adds the ERROR, but puts that ereport() in the equivalent
> place as it is for CHECK constraints. There are arguments to improve
> the error message to include the constraint name here, or specify that
> we're talking about a NOT NULL constraint. I made it the same text as
> for CHECK, as that requires no new translations.
>
The existing test on HEAD copies a table that has both a NO INHERIT
CHECK constraint and a NO INHERIT NOT NULL constraint. With the v2,
that statement now fails on the NOT NULL constraint because the
not-null constraints are stored during DefineRelation() before the
CHECK constraints are added. With VERBOSITY verbose, I got this :
CREATE TABLE x (LIKE noinh_con_copy INCLUDING ALL) PARTITION BY LIST (a);
ERROR: 42P16: cannot add NO INHERIT constraint to partitioned table "x"
LOCATION: StoreRelNotNull, heap.c:2290
The two statements v2 adds cover the NOT NULL alone (fails) and
neither constraint (succeeds), so no test exercises LIKE copying a NO
INHERIT CHECK constraint by itself into a partitioned table any more.
Since both errors currently have the same text, the expected output
doesn't show which check fired.
The v1 had a test for that case (noinh_chk_copy in
create_table_like.sql), and I think it's worth keeping:
-- A NO INHERIT CHECK constraint alone must also be rejected.
CREATE TABLE noinh_chk_copy (a int CHECK (a > 0) NO INHERIT);
CREATE TABLE noinh_chk_copy_parted (LIKE noinh_chk_copy INCLUDING CONSTRAINTS)
PARTITION BY LIST (a); -- fail
DROP TABLE noinh_chk_copy;
Please let me know what you think.
Regards,
Nitin Motiani
Google
| From | Date | Subject | |
|---|---|---|---|
| Next Message | jian he | 2026-10-22 13:12:53 | Re: json/jsonb cleanup + FmgrInfo caching |
| Previous Message | Dilip Kumar | 2026-10-22 12:45:02 | Re: Proposal: Conflict log history table for Logical Replication |