Subject: Do not lock tables in get_tables_to_repack

From: "cca5507" <[email-masked]>
Date: Tue, 16 Jun 2026 15:34:55 +0800

Hi hackers,

When doing a whole database repack, we build a list of repackable
tables and take a lock on them to prevent concurrent drops. But
concurrent drops can always happen after we build the list because
we process each table in a separate transaction. The
ConditionalLockRelationOid() also makes the default behavior like
SKIP_LOCKED, which is unexpected.

To remove the locks, we need to make repack_is_permitted_for_relation()
handles concurrent drops correctly: it should not report an error
when failing to search the syscache in pg_class_aclcheck(). Use
pg_class_aclcheck_ext() instead to detect a concurrent drop. Also
check the return value of get_rel_name().

Thoughts?

--
Regards,
ChangAo Chen

---

From: Bharath Rupireddy <[email-masked]>
Date: Sat, 27 Jun 2026 12:56:04 -0700

Hi,

> check the return value of get_rel_name().

I don't have a strong opinion on fixing this for REPACK. For vacuum,
we wanted to get consistent behavior across all modes of invoking it
[1]. Whether to handle concurrent drops for REPACK depends on how long
it spends scanning pg_index/pg_class in get_tables_to_repack. Also,
vacuum is more commonly run database-wide (against all tables), and
without field experience showing this is a real problem for REPACK,
I'm not sure the fix is needed. That said, others may have a different
take - adding the REPACK authors here for their input.

[1] https://www.postgresql.org/message-id/CALj2ACUbhDFJWSwiyoUxXED2S2fr9Xis%3D08Rnjq53UzBE4FvzA%40mail.gmail.com

--
Bharath Rupireddy
Amazon Web Services: https://aws.amazon.com

---

From: Álvaro Herrera <[email-masked]>
Date: Tue, 7 Jul 2026 18:53:39 +0200

On 2026-Jun-16, cca5507 wrote:

> we process each table in a separate transaction.

Not only that.  We have actually three ways to obtain the list of tables
to repack, and only one of these obtains the locks.  So this code is
internally inconsistent.  I agree that we should do something like your
patch.  I wanted to be a little more defensive though; how about the
attached?

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

---

From: "cca5507" <[email-masked]>
Date: Wed, 8 Jul 2026 10:36:38 +0800

> patch.  I wanted to be a little more defensive though; how about the

-                       classtup = SearchSysCache1(RELOID, ObjectIdGetDatum(index->indrelid));
+                       classtup = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(index->indrelid));

Do we really need to copy it? We hold a refcount on it so it won't be freed
until we release it. Otherwise LGTM.

--
Regards,
ChangAo Chen