Re: pg_upgrade fails when FK constraint with same name exists on partitioned table and its partition

Hi,

A follow-up and a correction to my review from earlier today.

Correction: I wrote that dropping the partition's constraint is always
safe. That is not right. The check matches by name only, so dropping is
safe only when the parent's FK has the same columns and the same
referenced table. Renaming the partition's constraint is the fix that is
always safe.

I also looked at when the upgrade really fails. pg_dump sorts these
constraints by (table schema, constraint name, table name), all compared
byte-wise. With the same constraint name, the restore fails only when
the ancestor's ADD CONSTRAINT comes first, that is when (ancestor schema,
ancestor table) < (partition schema, partition table) in C order. Then
the ancestor's statement recurses into the partition and creates a child
with the same name, and the partition's own statement fails with "already
exists". In the other order the clone gets a generated name and the
restore works. For multi-level trees, the ancestor that matters is the
one whose FK has conparentid = 0.

I tested 13 cases with an old cluster on 17.12 and master 952bab8b74.
Unpatched, pg_upgrade --check passed in all of them and the real upgrade
failed in 5 (including cross-schema, dotted names, 3 levels, and an FK
added only on a middle table). With v1, --check catches all 5, and it
also reports 5 cases where the upgrade would have worked, not 2 as I
said before. A VALID FK, an FK only on the partition, and a detached
partition are not reported.

A catalog-only query that follows this order matches the real result in
all 13 cases. Still, I would keep the broad check: the narrow query copies
pg_dump's internal sort order, so if that changes, the check would miss
cases silently and the upgrade would fail after "Clusters are compatible".
The false positives are rare and easy to fix with a rename.

Attached:
- v2-0001: v1 unchanged (Arseny's patch)
- v2-0002: report with quoted, schema-qualified names, "may fail"
wording, and a hint that says rename always works and drop only when
the FKs are equivalent
- v2-0003: TAP test t/010_duplicate_fk_names.pl (7 tests: a dotted-name
cross-schema case, the report, fix by drop and by rename, then the
real pg_upgrade)

With all three patches and oldinstall set to 17.12, the TAP test passes
7/7; with only the test patch it fails, as expected. make -C
src/bin/pg_upgrade check passes (169 tests). No build warnings, pgindent
gives no diff. Not tested: old versions 11-16, meson, Windows.

Regards,
João Rito

Em ter., 10 de nov. de 2026 às 19:51, João Rito
<joaovictor(dot)rito(at)gmail(dot)com> escreveu:
>
> Hi Arseny,
>
> Thanks for the patch. I reviewed v1 (CF entry 7062) and tested it locally.
>
> What I tested
> - Patch applies cleanly with "git apply" on master at 952bab8b74d (no
> rebase needed). Build with --enable-cassert has no warnings.
> - Old cluster: PostgreSQL 17.12, new cluster: master 952bab8b74d.
> - Your reproducer from the first message: without the patch,
> "pg_upgrade --check" reports "Clusters are compatible" and the real
> run fails in pg_restore with: constraint "fk_trn_acc" for relation
> "trn_part1" already exists. With the patch, --check fails with your
> new message and lists xxi.trn_part1.fk_trn_acc.
> - Cases that are correctly not reported: a VALID FK on the partition
> (it is linked to the parent, conparentid != 0), a NOT VALID FK that
> exists only on the partition, and a partition that was detached from
> the parent.
> - make -C src/bin/pg_upgrade check (master to master) passes with the patch.
>
> Comments
> 1. False positives. The check also fires when the restore would
> succeed, because it depends on the order in which pg_dump emits the
> constraints. I saw two such cases where the upgrade works without the
> patch but is now blocked by --check: (a) the partition sorts before
> the parent (e.g. parent "zz_trn", partition "aa_part", same FK
> statements as in your example), and (b) a three-level hierarchy where
> the leaf has its own NOT VALID FK with the same name as the root's FK.
> You mention that the check flags both orderings; I think that is
> acceptable given Álvaro's reservations, but it may be worth saying in
> the commit message or the error text that the upgrade "may" fail, and
> that the suggested fix is always safe. If it is easy, narrowing the
> query to the cases that actually fail would be nicer, but I could not
> find a simple catalog-only condition for that.
> 2. Report format. The report prints nspname.relname.conname without
> quoting, so with dots in names it is ambiguous (I tried schema c,
> table "p.y", constraint "fk.z", which prints c.p.y.fk.z). Other checks
> have the same style, so this is minor; quote_ident-style output or
> separate columns would be clearer.
> 3. Hint text. It says "ALTER TABLE ONLY partition_name DROP CONSTRAINT
> constraint_name". Maybe use schema.partition_name, since the report
> prints the schema, and mention that the parent's constraint still
> covers the partition after dropping it (the partition's own NOT VALID
> FK is redundant).
> 4. Root cause. The patch only adds a check; pg_dump output is
> unchanged. That is the option Álvaro mentioned (while saying he was
> not fully convinced of it), and since the partition's constraint is
> basically redundant and the problem only exists for 11-17 source
> clusters, a --check seems like the right scope to me rather than
> changing pg_dump.
> 5. The query and the version range (11 through 17) look right to me.
> Joining only the direct parent is fine for multi-level partitioning,
> since the intermediate partition inherits the root's FK under the same
> name.
>
> Test
> The patch has no test. I wrote a TAP test
> (src/bin/pg_upgrade/t/010_duplicate_fk_names.pl, plus the meson.build
> entry). It needs the "oldinstall" environment variable pointing to a
> version 11-17 installation and is skipped otherwise. It creates your
> reproducer plus two negative cases, checks that --check fails and
> lists the constraint, drops the partition constraint as suggested, and
> then checks that --check and the real pg_upgrade succeed.
> I ran it with oldinstall = PostgreSQL 17.12: without your patch it
> fails (--check does not fail), with your patch it passes (6 tests).
> Without oldinstall it is skipped.
> I am happy to send it as a patch to this thread if that is useful.
>
> Best regards,
> João Rito
>
> Em ter., 10 de nov. de 2026 às 19:46, Arseny Kositsin
> <a(dot)kositsyn(at)postgrespro(dot)ru> escreveu:
> >
> >
> > > So why not just drop the constraint in the partition? Seems rather
> > > useless. If there's a valid reason to have a constraint that's
> > > different from the one in the parent table, then you can rename it.
> > >
> > > I guess we could add a check to "pg_upgrade --check" to report this kind
> > > of thing ... I'm not really convinced of this though. Anyhow, I don't
> > > feel inclined to try to have pg_upgrade/pg_dump handle this case cleanly.
> >
> > Thanks for the reply, and sorry for the long delay in responding.
> >
> > You're right that the constraint on the partition can simply be dropped
> > or renamed. However, since the failure occurs during pg_upgrade and
> > the error message is not very obvious, I think it would be helpful to
> > detect this situation with "pg_upgrade --check" beforehand, as you
> > suggested.
> >
> > Attached is a patch that implements this check. It detects NOT VALID
> > foreign keys on partitions that share a name with a foreign key on the
> > parent partitioned table and were not linked to it (conparentid = 0).
> > Such constraints may cause a name conflict during restore: pg_dump emits
> > them as separate ALTER TABLE ADD CONSTRAINT statements for both the
> > parent and the partition, and when the parent's constraint is restored
> > first, it is inherited by the partition, so the subsequent attempt to
> > create the same-named constraint on the partition errors out with
> > "constraint ... for relation ... already exists".
> >
> > Whether the failure actually occurs depends on the order in which
> > pg_dump emits the constraints, which is determined by alphabetical
> > sorting of table names; the check flags both cases.
> >
> > Only NOT VALID foreign keys are affected. VALID foreign keys on
> > partitions are matched and linked (conparentid is set) by
> > tryAttachPartitionForeignKey() during ATTACH PARTITION, so pg_dump
> > does not emit them separately. This matching logic excluded NOT VALID
> > constraints until PG 18, where it was fixed by commit b663b9436e7.
> > The check therefore applies to old clusters from PG 11 (when conparentid
> > was introduced) through PG 17.
> >
> > Other constraint types are not affected: CHECK constraints can have
> > duplicate names on partitions, but pg_dump emits them inline in
> > CREATE TABLE; PRIMARY KEY, UNIQUE, and EXCLUDE constraints create
> > indexes, so duplicate names are prevented by an index name conflict;
> > NOT NULL constraints as named entries in pg_constraint exist only since
> > PG 18.
> >
> > The patch is for the master branch. I did not add a separate test
> > because I did not find dedicated tests for other similar check
> > functions, but I am ready to add one if needed.
> >
> > Best regards,
> > Arseny Kositsyn.