Re: Do not lock tables in get_tables_to_repack

Hi,

On Fri, Jul 10, 2026 at 7:22 AM Álvaro Herrera <alvherre(at)kurilemu(dot)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
(https://www.postgresql.org/message-id/akPhEffRipH4isWF%40nathan)
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