Do not lock tables in get_tables_to_repack

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]: /messages/by-id/CALj2ACUbhDFJWSwiyoUxXED2S2fr9Xis=08Rnjq53UzBE4FvzA@mail.gmail.com
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]: /messages/by-id/CALj2ACUbhDFJWSwiyoUxXED2S2fr9Xis=08Rnjq53UzBE4FvzA@mail.gmail.com

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

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/

 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

On 2026-Jul-08, cca5507 wrote:

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

Yeah, I guess it doesn't matter. I have removed the copy and updated
some comments. I also realized that there are some places where we
weren't dealing correctly with the possibility that the relation goes
away, or is replaced with something different, so I added that too.

While looking at it I also realized that get_tables_to_repack_partitioned
is likewise not careful enough about it: we do IndexGetRelation(, false)
which fails hard if the pg_index tuple cannot be found, which is the
wrong thing to do.

At the same time, it's annoying that half of the code that clearly
belongs in that routine is actually in ExecRepack(). I moved that to
where it rightfully belongs. (The only somewhat annoying thing is that
we have to NULL-out the Relation pointer after returning; but that's not
*too* bad IMO.)

Any opinions on this?

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"No me acuerdo, pero no es cierto. No es cierto, y si fuera cierto,
no me acuerdo." (Augusto Pinochet a una corte de justicia)

Hi,

On Fri, Jul 10, 2026 at 7:22 AM Álvaro Herrera <alvherre@kurilemu.de> wrote:

On 2026-Jul-08, cca5507 wrote:

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

Thanks for committing this.

Here is my overall take. Even before this commit, a database-wide
REPACK does not error on a concurrently dropped table.
get_tables_to_repack takes a brief lock on each table before the ACL
check, which blocks a concurrent drop while the list is being built.
Later each table is re-opened under its lock and skipped if it is
already gone. So the brief lock is not needed for correctness. What it
does is let an unprivileged user lock relations it may not even be
allowed to repack, including system catalogs, before we have checked
its privileges. I am fine with removing the lock as the commit did.

I would also like to tighten repack_is_permitted_for_relation so a
caller that holds a lock still errors out on a missing relation
instead of skipping it silently
(/messages/by-id/akPhEffRipH4isWF@nathan).
Attached as a follow-up patch. Please have a look.

Yeah, I guess it doesn't matter. I have removed the copy and updated
some comments. I also realized that there are some places where we
weren't dealing correctly with the possibility that the relation goes
away, or is replaced with something different, so I added that too.

I agree that the SearchSysCacheCopy1 in v2-0002 adds no benefit. Since
no lock is held on the relation, there is no point in copying the
syscache tuple.

While looking at it I also realized that get_tables_to_repack_partitioned
is likewise not careful enough about it: we do IndexGetRelation(, false)
which fails hard if the pg_index tuple cannot be found, which is the
wrong thing to do.

At the same time, it's annoying that half of the code that clearly
belongs in that routine is actually in ExecRepack(). I moved that to
where it rightfully belongs. (The only somewhat annoying thing is that
we have to NULL-out the Relation pointer after returning; but that's not
*too* bad IMO.)

Any opinions on this?

I will take a closer look at this in a bit.

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

Hi,

On Fri, Jul 10, 2026 at 9:03 AM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:

While looking at it I also realized that get_tables_to_repack_partitioned
is likewise not careful enough about it: we do IndexGetRelation(, false)
which fails hard if the pg_index tuple cannot be found, which is the
wrong thing to do.

Agreed. For a single-level partitioned table, dropping a leaf blocks
on the parent's AccessExclusiveLock, which REPACK holds, so the leaf
can't go away mid-build. But for a multi-level table, REPACK holds a
lock only on the top parent, so a lower-level leaf whose immediate
parent is an intermediate table isn't protected and can be dropped
concurrently. So IndexGetRelation(, false) failing hard there is
indeed the wrong thing.

At the same time, it's annoying that half of the code that clearly
belongs in that routine is actually in ExecRepack(). I moved that to
where it rightfully belongs.

+1, that's a good cleanup. That code clearly belongs in
get_tables_to_repack_partitioned().

(The only somewhat annoying thing is that
we have to NULL-out the Relation pointer after returning; but that's not
*too* bad IMO.)

That's fine. We already NULL out rel after
get_tables_to_repack_partitioned() today, so the patch isn't adding
anything new there. Relying on a non-NULL return from
process_single_relation() to mean partitioned table is a little
indirect on first read, but the comments explains it well, and I
wouldn't complicate the code to avoid it.

Any opinions on this?

I will take a closer look at this in a bit.

Patch looks good to me. One nit:

+ table_oid = IndexGetRelation(child_oid, true);
+ if (!OidIsValid(table_oid))
+ continue;

How about a short comment on why this is needed even with a lock on
the parent, since that lock doesn't cover lower-level leaves in a
multi-level partition tree?

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

 > Any opinions on this?

 I will take a closer look at this in a bit.

Patch looks good to me. One nit:

+ table_oid = IndexGetRelation(child_oid, true);
+ if (!OidIsValid(table_oid))
+ continue;

How about a short comment on why this is needed even with a lock on
the parent, since that lock doesn't cover lower-level leaves in a
multi-level partition tree?

I think it's worth a comment, too. Patch LGTM.

--
Regards,
ChangAo Chen

On 2026-Jul-11, cca5507 wrote:

Bharath Rupireddy wrote:

+ table_oid = IndexGetRelation(child_oid, true);
+ if (!OidIsValid(table_oid))
+ continue;

How about a short comment on why this is needed even with a lock on
the parent, since that lock doesn't cover lower-level leaves in a
multi-level partition tree?

I think it's worth a comment, too. Patch LGTM.

Thanks, added that and pushed.

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Puedes vivir sólo una vez, pero si lo haces bien, una vez es suficiente"