Feedback-ID: ia2694551:Fastmail
Date: Fri, 10 Oct 2025 16:11:32 +0200
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Antonin Houska <ah@cybertec.at>
Subject: Re: Adding REPACK [concurrently]
Message-ID: <202510101352.vvp4p3p2dblu@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="sm6wwjwl24kwg5xb"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <202507262156.sb455angijk6@alvherre.pgsql>
Archived-At: 
 <https://www.postgresql.org/message-id/202510101352.vvp4p3p2dblu%40alvherre.pgsql>
Precedence: bulk


--sm6wwjwl24kwg5xb
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

Hello,

Here's patch v24.  I was hoping to push this today, but I think there
were too many changes from v23 for that.  Here's what I did:

- pg_stat_progress_cluster is no longer a view on top of the low-level
  pg_stat_get_progress_info() function.  Instead, it's a view on top of
  pg_stat_progress_repack.  The only change it applies on top of that
  one is change the command from REPACK to one of VACUUM FULL or
  CLUSTER, depending on whether an index is being used or not.  This
  should keep the behavior identical to previous versions.
  Alternatively we could just hide rows where the command is REPACK, but
  I don't think that would be any better.  This way, we maintain
  compatibility with tools reading pg_stat_progress_cluster.  Maybe this
  is useless and we should just drop the view, not sure, we can discuss
  separately.

- pg_stat_progress_repack itself now shows the command.  Also I got rid
  of the separate enum values for the command, and instead used the
  values from the parse node (RepackCommand); this removes about a dozen
  lines of C code.  To forestall potentially bogus usage of value 0, I
  made the enum start from 1.

- I noticed that you can do "CLUSTER pg_class ON some_index" and it will
  happily modify pg_index.indisclustered, which is a bit weird
  considering that allow_system_table_mods is off -- if you later try
  ALTER TABLE .. SET WITHOUT CLUSTER, it won't let you.  I think this is
  bogus and we should change it so that CLUSTER refuses to change the
  clustered index on a system catalog, unless allow_system_table_mods is
  on.  However, that would be a change from longstanding behavior which
  is specifically tested for in regression tests, so I didn't do it.
  We can discuss such a change separately.  But I did make REPACK refuse
  to do that, because we don't need to propagate bogus historical
  behavior.  So REPACK will fail if you try to change the indisclustered
  index, but it will work fine if you repack based on the same index as
  before, or repack with no index.

- pg_repackdb: if you try with a non-superuser without specifying a
  table name, it will fail as soon as it hits the first catalog table or
  whatever with "ERROR: cannot lock this table".  This is sorta fine for
  vacuumdb, but only because VACUUM itself will instead say "WARNING:
  cannot lock table XYZ, skipping", so it's not an error and vacuumdb
  keeps running.  IMO this is bogus: vacuumdb should not try to process
  tables that it doesn't have privileges to.  However, not wanting to
  change longstanding behavior, I left that alone.  For pg_repackdb, I
  added a condition in the WHERE clause there to only fetch tables that
  the current user has MAINTAIN privilege over.  Then you can do a
  "pg_repackdb -U foobar" and it will nicely process the tables that
  that user is allowed to process.  We can discuss changing the vacuumdb
  behavior separately.

- Added some additional tests for pg_repackdb and REPACK.

- Updated the docs.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

--sm6wwjwl24kwg5xb
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v24-0001-Add-REPACK-command.patch"
Content-Transfer-Encoding: 8bit