REPACK: warn about skipping foreign partitions

Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Repack/cluster errors out when called directly on a foreign table. In
contrast to vacuum, repack/cluster doesn't warn when the table itself
is local but has foreign partitions.

The attached patch adds a similar warning to these commands, making
this skip clearly visible to users.

I don't object to the new warning, however this does not look like REPACK
regression. AFAICS, CLUSTER in v18 behaves in the same way. Please add the
patch to the next CF. Thanks.

--
Antonin Houska
Web: https://www.cybertec-postgresql.com

Hi,

On Sun, Aug 30, 2026 at 11:41 PM Antonin Houska <ah@cybertec.at> wrote:

Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Repack/cluster errors out when called directly on a foreign table. In
contrast to vacuum, repack/cluster doesn't warn when the table itself
is local but has foreign partitions.

The attached patch adds a similar warning to these commands, making
this skip clearly visible to users.

I don't object to the new warning, however this does not look like REPACK
regression. AFAICS, CLUSTER in v18 behaves in the same way. Please add the
patch to the next CF. Thanks.

Yes, silently skipping foreign partitions seems like an existing
behavior. If we want to fix this, IMHO, ensuring the behavior and the
code and warning message stay common across vacuum, vacuum full,
repack, concurrent repack, and cluster (both database-wide and with a
list of relations) makes sense. So attempting to fix it on HEAD seems
right to me.

I'm not sure if we need to explicitly mention in the docs that we emit
a warning. Specifying that we skip foreign partitions is fine.

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

On Mon, 31 Aug 2026, Antonin Houska <ah@cybertec.at> wrote:

I don't object to the new warning, however this does not look like REPACK
regression. AFAICS, CLUSTER in v18 behaves in the same way.

You are right, I also should have mentioned CLUSTER in the subject. The reason I focused on REPACK is that the release notes mention it as a replacement of VACUUM FULL, which does emit this warning.

https://commitfest.postgresql.org/patch/7240/

Hi Zsolt,

A few small things.

In the non index case the new find_all_inheritors call returns the
same list as the one right below it. You could reuse it there.

For the docs I would write "are skipped with a warning".

The last test, the parent with only foreign partitions, goes through
the same code path as the one before it. I would drop it.

Otherwise it looks good to me.

Thanks
Shihao

Thank you for the comments! I attached v2 that should address everything.

v2 looks good to me.

I have marked it Ready for Committer.

v3 is a trivial rebase to resolve a documentation conflict

On Sat, Oct 10, 2026 at 12:57 AM Zsolt Parragi
<zsolt.parragi@percona.com> wrote:

v3 is a trivial rebase to resolve a documentation conflict

In v3, CLUSTER/REPACK warn about foreign partitions during partition
collection, without locking them or checking privileges. OTOH,
VACUUM/VACUUM FULL warn only after locking each partition and
checking privileges.

So, for example, if the user lacks privileges on a foreign partition,
VACUUM/VACUUM FULL report a permission warning, whereas CLUSTER/REPACK
report that foreign tables cannot be processed.

Are these differences intentional, or should CLUSTER/REPACK perform the
checks in the same order as VACUUM/VACUUM FULL?

Regards,

--
Fujii Masao

Thank you for the review!

So, for example, if the user lacks privileges on a foreign partition,
VACUUM/VACUUM FULL report a permission warning, whereas CLUSTER/REPACK
report that foreign tables cannot be processed.

That was an oversight, it's fixed in v4.

In v3, CLUSTER/REPACK warn about foreign partitions during partition
collection, without locking them or checking privileges. OTOH,
VACUUM/VACUUM FULL warn only after locking each partition and
checking privileges.

The 'without locking' part seems good to me. We already have
AccessExclusiveLock on the parent, and we never actually open the
foreign partitions. And this is a preexisting repack design (checking
permissions without locking first, and then checking again after
locking, just before processing it)

On Sun, Oct 11, 2026 at 2:28 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

That was an oversight, it's fixed in v4.

Thanks for updating the patch!

+ if (rel_is_index)
+ inhoids = find_all_inheritors(relid, NoLock, NULL);

It's better to call list_free(inhoids) before replacing it with the index
OID list? The table OID list is no longer needed after the warning loop.

Especially when an index is specified, the patch adds another
find_all_inheritors() call and loop while holding AccessExclusiveLock on
the parent. Is this overhead unnoticeable and acceptable for tables
with many small partitions?

The 'without locking' part seems good to me. We already have
AccessExclusiveLock on the parent, and we never actually open the
foreign partitions. And this is a preexisting repack design (checking
permissions without locking first, and then checking again after
locking, just before processing it)

Understood.

So a partition can be dropped concurrently while warnings are being
issued for foreign partitions, and get_rel_name(tableoid) can return NULL
before the warning is emitted. Is that correct? If so, as in
repack_is_permitted_for_relation(), we should check whether
get_rel_name(tableoid) returns NULL and skip the warning if it does?

Regards,

--
Fujii Masao

Especially when an index is specified, the patch adds another
find_all_inheritors() call and loop while holding AccessExclusiveLock on
the parent. Is this overhead unnoticeable and acceptable for tables
with many small partitions?

After taking another look I think it's the opposite: we can avoid
another find_all_inheritors with plain REPACK, but we need at least
some find_inheritance_children calls for USING INDEX / CLUSTER as
foreign partitions don't have indexes.

v5 is this alternative approach (adding the warning inside
get_tables_to_repack_partitioned). I tried to measure them, but I
couldn't measure any difference between the two, only noise.

So a partition can be dropped concurrently while warnings are being
issued for foreign partitions, and get_rel_name(tableoid) can return NULL
before the warning is emitted.

Oops, I missed that nested partitions can do this because we only lock
the top level parent. So yes, that null check is needed.

And because of this, I started thinking about why
find_all_inheritors(..., NoLock, ...) works at all with multi level
partition tables (root-parent-parent2-child) against a concurrent drop
of parent2 with only root locked... and it doesn't. That's an existing
race in vacuum/analyze/repack and lock table, I'll start a separate
thread about it tomorrow.

Hi Zsolt,

I tested the v5 patch, “Warn when REPACK or CLUSTER skips a foreign
table,” against the updated PostgreSQL source.
The patch applied successfully, and the build completed without
errors. All regression tests passed.
For manual testing, I created a partitioned table with both a local
partition and a foreign-table partition using 'postgres_fdw'
Before applying the patch, 'CLUSTER' skipped the foreign partition
without displaying a warning.
After applying the patch, I verified that both 'CLUSTER' and 'REPACK'
displayed the expected warnings when skipping the foreign partition.
The local and foreign partition data remained accessible, and both
commands completed successfully in my test case.

The behavior looks good based on these tests. I did not observe any
issues during testing.

Thanks for the patch and the work on this improvement.

Regards
solai

Hi Zsolt,

I read through the thread, very nice points were raised.
I looked at v5. It applies cleanly on master and builds without warnings.

One concern with the new USING INDEX branch in
get_tables_to_repack_partitioned():
parent_oid = IndexGetRelation(child_oid, true);
...
partoids = find_inheritance_children(parent_oid, NoLock);
For a partitioned table below the first level, parent_oid isn't locked, so
it can be dropped concurrently (with root -> p1 -> p11 -> ..., DROP TABLE
p11 only locks p1). find_inheritance_children() goes through
has_subclass(), which raises "cache lookup failed for relation" when the
pg_class row is gone. The surrounding code already handles a vanished
relation (IndexGetRelation(..., true) and the NULL check in
warn_foreign_partition()), but this call doesn't.

This is the same race as in your find_all_inheritors(NoLock) thread [1]/messages/by-id/CAN4CZFMp2oRXb_AC6+6+NxdjPHE2DJHFz_5WTpm99ovjKD9xTA@mail.gmail.com. As
I read v2 there, the missing_ok handling is internal to
find_all_inheritors(), so it wouldn't cover this new call. Maybe expose the
missing_ok variant and use it here, or go back to the v4 shape
(find_all_inheritors() on the table), since you saw no measurable
difference between them and v4 would get that fix for free.

Minor comment, database-wide REPACK still skips foreign tables silently,
which matches database-wide VACUUM, so I think that's right. It might be
worth saying so in the commit message, given Bharath's point about keeping
all the forms consistent.

Otherwise the permission ordering and the tests look good to me.

[1]: /messages/by-id/CAN4CZFMp2oRXb_AC6+6+NxdjPHE2DJHFz_5WTpm99ovjKD9xTA@mail.gmail.com

/messages/by-id/CAN4CZFMp2oRXb_AC6+6+NxdjPHE2DJHFz_5WTpm99ovjKD9xTA@mail.gmail.com

Regards,
Surya Poondla

This is the same race as in your find_all_inheritors(NoLock) thread [1]. As
I read v2 there, the missing_ok handling is internal to
find_all_inheritors(), so it wouldn't cover this new call. Maybe expose the
missing_ok variant and use it here, or go back to the v4 shape
(find_all_inheritors() on the table), since you saw no measurable
difference between them and v4 would get that fix for free.

You are right, I realized that while I was working on it, but it
didn't seem like a big issue with find_all_inheritors behaving the
same way already, and then I forgot about it when starting the other
thread.

Since (1) I think it's better if this thread doesn't depend on the
outcome of the other and (2) I think the v4 version was overall
simpler, I'm attaching the version of that with the original concerns
fixed as v6. I thought a bit more about the performance question, but
since we are already at the partitioned table code, it had to do the
same iterations anyway, so it didn't make the indexed case better,
even in theory.

v6 has exactly the same issue as v5 without the fix from the other
thread, but if that lands, it will handle both locations.

I also did verify that there's no easy way to make the
find_all_inheritors for repack locking and avoiding deadlocks, at
least none I found.