Re: REPACK: warn about skipping foreign partitions

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

https://www.postgresql.org/message-id/flat/CAN4CZFMp2oRXb_AC6%2B6%2BNxdjPHE2DJHFz_5WTpm99ovjKD9xTA%40mail.gmail.com

Regards,
Surya Poondla