Thread

1. Do not lock tables in get_tables_to_repack

   cca5507 <cca5507@qq.com> — 2026-06-16T07:34:55Z

   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
   

2. Re: Do not lock tables in get_tables_to_repack

   Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2026-06-27T19:56:04Z

   Hi,
   
   On Tue, Jun 16, 2026 at 12:35 AM cca5507 <cca5507@qq.com> wrote:
   >
   > 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?
   
   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
   
   
   
   

3. Re: Do not lock tables in get_tables_to_repack

   Álvaro Herrera <alvherre@kurilemu.de> — 2026-07-07T16:53:39Z

   On 2026-Jun-16, cca5507 wrote:
   
   > 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.
   
   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/
   

4. Re: Do not lock tables in get_tables_to_repack

   cca5507 <cca5507@qq.com> — 2026-07-08T02:36:38Z

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