Re: cataloguing NOT NULL constraints - Mailing list pgsql-hackers
From | Alvaro Herrera |
---|---|
Subject | Re: cataloguing NOT NULL constraints |
Date | |
Msg-id | 202405082042.soxl2pzu5dwl@alvherre.pgsql Whole thread Raw |
In response to | Re: cataloguing NOT NULL constraints (Kyotaro Horiguchi <horikyota.ntt@gmail.com>) |
Responses |
Re: cataloguing NOT NULL constraints
|
List | pgsql-hackers |
On 2024-May-07, Kyotaro Horiguchi wrote: > Hello, > > At Wed, 1 May 2024 19:49:35 +0200, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote in > > Here are two patches that I intend to push soon (hopefully tomorrow). > > This commit added and edited two error messages, resulting in using > slightly different wordings "in" and "on" for relation constraints. > > + errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" on relation \"%s\"", > === > + errmsg("cannot change NO INHERIT status of NOT NULL constraint \"%s\" in relation \"%s\"", Thank you, I hadn't noticed the inconsistency -- I fix this in the attached series. While trying to convince myself that I could mark the remaining open item for this work closed, I discovered that pg_dump fails to produce working output for some combinations. Notably, if I create Andrew Bille's example in 16: create table test_0 (id serial primary key); create table test_1 (id integer primary key) inherits (test_0); then current master's pg_dump produces output that the current server fails to restore, failing the PK creation in test_0: ALTER TABLE ONLY public.test_0 ADD CONSTRAINT test_0_pkey PRIMARY KEY (id); ERROR: cannot change NO INHERIT status of NOT NULL constraint "pgdump_throwaway_notnull_0" in relation "test_1" because we have already created the NOT NULL NO INHERIT constraint in test_1 when we created it, and because of d45597f72fe5, we refuse to change it into a regular inheritable constraint, which the PK in its parent table needs. I spent a long time trying to think how to fix this, and I had despaired wanting to write that I would need to revert the whole NOT NULL business for pg17 -- but that was until I realized that we don't actually need this NOT NULL NO INHERIT business except during pg_upgrade, and that simplifies things enough to give me confidence that the whole feature can be kept. Because, remember: the idea of those NO INHERIT "throwaway" constraints is that we can skip reading the data when we create the PRIMARY KEY during binary upgrade. We don't actually need the NO INHERIT constraints for anything during regular pg_dump. So what we can do, is restrict the usage of NOT NULL NO INHERIT so that they occur only during pg_upgrade. I think this will make Justin P. happier, because we no longer have these unsightly NOT NULL NO INHERIT nonstandard syntax in dumps. The attached patch series does that. Actually, it does a little more, but it's not really much: 0001: fix the typos pointed out by Kyotaro. 0002: A mechanical code movement that takes some ugly ballast out of getTableAttrs into its own routine. I realized that this new code was far too ugly and messy to be in the middle of filling the tbinfo struct of attributes. If you use "git show --color-moved --color-moved-ws=ignore-all-space" with this commit you can see that nothing happens apart from the code move. 0003: pgindent, fixes the comments just moved to account for different indentation depth. 0004: moves again the moved PQfnumber() calls back to getTableAttrs(), for efficiency (we don't want to search the result for those resnums for every single attribute of all tables being dumped). 0005: This is the actual code change I describe above. We restrict use_throwaway_nulls so that it's only set during binary upgrade mode. This changes pg_dump output; in the normal case, we no longer have NOT NULL NO INHERIT. I added one test stanza to verify that pg_upgrade retains these clauses, where they are critical. 0006: Tighten up what d45597f72fe5 did, in that outside of binary upgrade mode, we no longer accept changes to NOT NULL NO INHERIT constraints so that they become INHERIT. Previously we accepted that during recursion, but this isn't really very principled. (I had accepted this because pg_dump required it for some other cases). This changes some test output, and I also simplify some test cases that were testing stuff that's no longer interesting. (To push, I'll squash 0002+0003+0004 as a single one, and perhaps 0005 with them; I produced them like this only to make them easy to see what's changing.) I also have a pending patch for 16 that adds tables like the problematic ones so that they remain for future pg_upgrade testing. With the changes in this series, the whole thing finally works AFAICT. I did notice one more small bit of weirdness, which is that at the end of the process you may end up with constraints that retain the throwaway name. This doesn't seem at all critical, considering that you can't drop them anyway and such names do not survive a further dump (because they are marked as inherited constraint without a "local" definition, so they're not dumped separately). I would still like to fix it, but it seems to require unduly contortions so I may end up not doing anything about it. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Attachment
- 0001-Fix-typos-in-error-messages.patch
- 0002-Mechanical-move-of-not-null-code-out-of-getTableAttr.patch
- 0003-pgindent-run.patch
- 0004-Take-PQfnumber-calls-out-of-the-routine.patch
- 0005-Use-NOT-NULL-NO-INHERIT-only-in-pg_upgrade-mode.patch
- 0006-Don-t-accept-NO-INHERIT-changes-to-INHERIT-in-normal.patch
pgsql-hackers by date: