Date: Sat, 23 Aug 2025 07:45:05 -0700
From: Noah Misch <noah@leadboat.com>
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: Alexander Lakhin <exclusion@gmail.com>, pgsql-hackers@postgresql.org,
	Robert Haas <robertmhaas@gmail.com>
Subject: Re: Test instability when pg_dump orders by OID
Message-ID: <20250823144505.88.nmisch@google.com>
References: <20250718191702.45.nmisch@google.com>
 <CA+Tgmoa8FumzaUUSnjK0x=6_oXDXggQPpzgPfX-KE=PVrTyamg@mail.gmail.com>
 <20250725022740.30.nmisch@google.com>
 <CA+TgmoZXmAZmB9OZaXTiy-HG2_1d7UiYrFKXf17C_yLfPa5pQg@mail.gmail.com>
 <20250804000321.8b.nmisch@google.com>
 <d32aaa8d-df7c-4f94-bcb3-4c85f02bea21@gmail.com>
 <CALdSSPhZyLua3Vz2Ug8VOASbhP41h-7784MT8s9_+90eE06Aaw@mail.gmail.com>
 <20250810163659.71.nmisch@google.com>
 <CALdSSPjDpzTAJsOwyuBy9Hxz7dmZyNTamQWBjEsKJD0KFcbphg@mail.gmail.com>
 <20250823052019.43.nmisch@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20250823052019.43.nmisch@google.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250823144505.88.nmisch%40google.com>
Precedence: bulk

On Fri, Aug 22, 2025 at 10:20:19PM -0700, Noah Misch wrote:
> On Mon, Aug 11, 2025 at 12:20:09AM +0500, Kirill Reshke wrote:
> > On Sun, 10 Aug 2025 at 21:37, Noah Misch <noah@leadboat.com> wrote:
> > > Thanks.  Could you make src/test/regress create regression database objects so
> > > the code addition has coverage?  Using pg_signal_backend and
> > > pg_read_all_settings as the default ACL role names should avoid that suite's
> > > limitations.  (The suite must run under any role name and must drop any roles
> > > it creates, so it can't assume any particular non-system role name survives
> > > the suite.)
> > 
> > Here is my attempt at implementing necessary legwork. It's v3 because
> > I accidentally cleared the CC list in my previous attempt. Noah kindly
> > explained to me how additions to the regress test will cause pg_dump
> > logic to be tested as well.
> > TIL 002_pg_upgarde.pl runs a regression suite, so if we create any
> > database objects in it, it will end up being dumped and restored in
> > that test.
> > So, I checked that without  changes in pg_dump_sort.c, 002_pg_upgarde
> > fails and with changes it does not.
> 
> Great.
> 
> > PFA. I am not horribly sure about my additions to the
> > `src/test/regress/sql/privileges.sql` file, maybe appending SQL to the
> > end of the file is not the best option and there is a better place.
> 
> I like how src/test/regress/sql/collate.icu.utf8.sql puts that kind of thing
> just after cleanup, so I put it there.  Pushed as b61a5c4 with a few other
> cosmetic changes.  Thanks.

TestUpgradeXversion fails:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=copperhead&dt=2025-08-23%2007%3A34%3A38

--- /home/pgbf/buildroot/upgrade.copperhead/REL_18_STABLE/origin-REL_16_STABLE.sql.fixed	2025-08-23 10:28:16.464887433 +0200
+++ /home/pgbf/buildroot/upgrade.copperhead/REL_18_STABLE/converted-REL_16_STABLE-to-REL_18_STABLE.sql.fixed	2025-08-23 10:28:16.508887289 +0200
@@ -606490,13 +606490,13 @@
 --
 -- Name: DEFAULT PRIVILEGES FOR TABLES; Type: DEFAULT ACL; Schema: -; Owner: pg_read_all_settings
 --
-ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings REVOKE SELECT,INSERT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLES FROM pg_read_all_settings;
-ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings GRANT SELECT,REFERENCES,DELETE,TRIGGER,TRUNCATE,UPDATE ON TABLES TO pg_read_all_settings;
+ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings REVOKE ALL ON TABLES FROM pg_read_all_settings;
+ALTER DEFAULT PRIVILEGES FOR ROLE pg_read_all_settings GRANT SELECT,REFERENCES,DELETE,TRIGGER,TRUNCATE,MAINTAIN,UPDATE ON TABLES TO pg_read_all_settings;


Crossing the boundary of the MAINTAIN privilege existing seems relevant.  Will
fix.  (My checklist did tell me to do a local run of TestUpgradeXversion.  I
skipped it, betting this patch wouldn't break that test.  I lost that bet.)