On Fri, Mar 10, 2023 at 03:36:10PM -0500, Tom Lane wrote: > Justin Pryzby <pry...@telsasoft.com> writes: > > Update to address a compiler warning in the supplementary patches adding > > assertions. > > I took a look through this. It seems like basically a good solution, > but the count_leaf_partitions() function is bothering me, for two > reasons: > > 1. It seems like a pretty expensive thing to do. Don't we have the > info at hand somewhere already?
I don't know where that would be. We need the list of both direct *and* indirect partitions. See: https://www.postgresql.org/message-id/5073D187-4200-4A2D-BAC0-91C657E3C22E%40gmail.com If it would help to avoid the concern, then I might consider proposing not to call get_rel_relkind() ... > 2. Is it really safe to do find_all_inheritors with NoLock? If so, > a comment explaining why would be good. In both cases (both for the parent and for case of a partitioned child with pre-existing indexes being ATTACHed), the table itself is already locked by DefineIndex(): lockmode = concurrent ? ShareUpdateExclusiveLock : ShareLock; rel = table_open(relationId, lockmode); and childrel = table_open(childRelid, lockmode); ... table_close(childrel, NoLock); And, find_all_inheritors() will also have been called by ProcessUtilitySlow(). Maybe it's sufficient to mention that ? -- Justin