Re: BUG #19483: pg_upgrade fails with orphan records in pg_init_priv catalog table

On Fri, 2026-06-26 at 08:56 +0200, Hüseyin Demir wrote:
> > > This v6 patch adds a SAFE_INITPRIVS macro that filters aclitem[]
> > > arrays server-side by checking that each entry's grantor and grantee
> > > OID still exists in pg_roles. It is applied in exactly two queries:
> > >
> > > 1. getAdditionalACLs() -- the one-time fetch of pg_init_privs at startup
> > > 2. dumpTable() column ACL prepared statement -- per-table column initprivs
> >
> > No, that's not good. If you are running the complicated subquery for
> > every table dumped, you are re-introducing the performance regression
> > from the v4 patch that Tom justly complained about.
>
> The correlated subquery in getAdditionalACLs adds overhead
> proportional to pg_init_privs row count × aclitems per row, with each
> check being an indexed OID lookup. For a database with 5,000
> pg_init_privs rows this is probably a few extra milliseconds. I added
> performance test results with v6. I think it's a sustainable operation
> but If I'm missing sth. let me know.

I have no problem with the subquery being in getAdditionalACLs().
But dumpTable() is executed once per table that gets dumped, right?
So the complicated subquery will run once per table.

> > Here is my latest idea (hold your noses):
> > Instead of having pg_dump query "FROM pg_catalog.pg_init_privs pip",
> > how about writing "(FROM (VALUES (...), (...), ...) AS pip", where the
> > VALUES clause is composed from a query against pg_init_privs run once
> > at the beginning of pg_dump that excludes the bad entries?
> > Critizism I forsee is that a) this is ugly and b) very long VALUES
> > statements might also constitute a performance regression.
> > However, I have yet to see an extension that produces a hundred
> > initial privilege entries.
>
> I'm not against this approach but the tradeoff is the same with v6 I
> suppose. Since the overhead of processing the full VALUES clause on
> every EXECUTE is unnecessary. v6 goes directly to pg_init_privs
> indexed by objoid = $1.

Ah, so you are saying that the effort per table will be much less.

Could you measure the difference with a database with - say - 10000 tables?

> But definitely we can prepare a new patch to cover these expectations.

Let's agree on the proper approach before you go to the effort of writing
another patch.

> With that in mind, I would like to propose splitting the work into two
> separate patches:
>
> 1. pg_dump fix (backpatch to 14): the v6 SQL OID-level filter. Works
> on any server >= 9.6 without requiring a source-side update. Fixes the
> known breakage for users upgrading from pre-17 clusters that
> accumulated dangling entries via DROP ROLE.
>
> 2. putid() fix (HEAD only): improve aclitemout so that a dangling OID
> is rendered in a form that is unambiguously distinct from a valid role
> name. This is the right long-term fix for the text representation and
> would benefit any future tool that processes aclitem output. It is
> independent of the pg_dump fix and does not need to be backpatched for
> patch 1 to be correct.

I think that we all agree on that.

> New dangling entries will not live on modern servers. However, users
> upgrading from pre-17 clusters may have years of accumulated dangling
> entries that pg_upgrade will carry forward silently.

... or rather fail doing so ...

> The pg_dump fix
> is a one-time for that migration window.

Right.

> Does this split make sense to the reviewers? If so, I will prepare v7
> of the pg_dump patch (incorporating any further feedback on v6) and a
> separate patch for the putid() fix in HEAD.

It makes sense to me.

I'd say that your v6 patch is fine if the performance impact can be
shown to be small.

Yours,
Laurenz Albe