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

> > 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.

> On the other hand, I agree with you that Tom's idea to make this fix
> depend on a minor update of the source server that fixes the string
> representation of aclitems is not so great.  Few people undergo the
> hassle of applying the latest minor update to a server they are about
> to update (and I am not even speaking about the users who keep running
> on the 14.3 they went into production with).  Yes, the problem that
> the present patch is trying to address is a rare one, and we should
> keep the maintenance and performance burden incurred moderate.
> But what good is a fix that won't work for a good percentage of the
> affected cases, even if they are few?
>
> 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.

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

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.

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.  The pg_dump fix
is a one-time for that migration window.
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.

Regards,
Demir.