MIME-Version: 1.0
References: <19483-80de42dc4e62cfd6@postgresql.org>
 <CAKAnmmLYXcBSV8i5MVuiwu8tkX-JDGmZtJ38_YbdUTwoDW7xKg@mail.gmail.com>
 <104812.1780847519@sss.pgh.pa.us>
 <CAB5wL7bo-7okBCJ24fdVn4UhnuPLPG22eQjaLJZ9GJtv0FEWMg@mail.gmail.com>
 <CAB5wL7bunPCi93Bwi7zN_1gFAZhKLwS=Fsub91SW417xujGtNA@mail.gmail.com>
 <CAB5wL7aKGFFEF1SM54GxvaQPH=7Bs=HuUCqi7F4Ve025+Z950A@mail.gmail.com>
 <CAB5wL7aK6UaukMSegzEG68y0VrKOg7wFnPNRaDaHreBf2iVWag@mail.gmail.com>
 <d305af372949be0c2c4155168ca7ba97e160bc52.camel@cybertec.at>
 <5184088806e70ed5db7e83bd6540a016b3802c1d.camel@cybertec.at>
 <CAB5wL7ZuFwu2Sd-xmn-w5-BbrKk_Mu_1ubYow_qaqFGhJOiMXw@mail.gmail.com>
 <CAB5wL7ai8AiaFV6B8J=w6vK1Q6Z9Hmp_y3PFL2Zzpx6Y8xPZ2A@mail.gmail.com>
 <0d7531c192859057c6aad54c89a8d453376bd00c.camel@cybertec.at>
 <592839.1782153054@sss.pgh.pa.us>
 <CAB5wL7btFp-XRBfv_J-YperoK+e0uYje3ZYsfTzCuHcUWWrvbw@mail.gmail.com>
 <CAB5wL7YFeL0VP7EZu6wtWx=UhXuDp3dTBVJwFvSRzBi7qNscfQ@mail.gmail.com>
 <f55bf7dc0a503f82abfb6f02ac91278fe59d40d9.camel@cybertec.at>
 <CAB5wL7Zy89DeVXHFvTL72KChkq=2jLXhH14zrC2n5sutLTqZDg@mail.gmail.com>
 <daee495807e183ea6f48c3c0844239753d7f9f6d.camel@cybertec.at>
In-Reply-To: <daee495807e183ea6f48c3c0844239753d7f9f6d.camel@cybertec.at>
From: =?UTF-8?Q?H=C3=BCseyin_Demir?= <huseyin.d3r@gmail.com>
Date: Fri, 26 Jun 2026 08:56:00 +0200
Message-ID: 
 <CAB5wL7YQs7CDduxx0cv3iKPnQ7fxT-mj0AqPWv2WPO3aaF_VeQ@mail.gmail.com>
Subject: Re: BUG #19483: pg_upgrade fails with orphan records in pg_init_priv
 catalog table
To: Laurenz Albe <laurenz.albe@cybertec.at>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Greg Sabino Mullane <htamfids@gmail.com>,
	pgsql-bugs@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAB5wL7YQs7CDduxx0cv3iKPnQ7fxT-mj0AqPWv2WPO3aaF_VeQ%40mail.gmail.com>
Precedence: bulk

> > 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 startu=
p
> > 2. dumpTable() column ACL prepared statement -- per-table column initpr=
ivs
>
> 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 =C3=97 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 =3D $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 >=3D 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.