On Wed, Oct 6, 2021 at 10:40 PM Pavel Borisov <pashkin.e...@gmail.com> wrote: > > Hi, hackers! > > We've reviewed patch v3 and found it right. Completely agree that in case we > attach/detach partition relcaches for all child relations (if they exist) > need invalidation. > Installcheck world succeeds after the patch. Tests from the patch fail as > they should when run on the master branch. Found no problems. > > Overall patch is good and I'd recommend it to be committed. > > We've made v4 patch according to Nitin's advice and tested it, but still have > no objections to patch v3. Each can be committed, equally good.
Thanks Pavel, Nitin for your reviews.
I was looking again at the following hunk in the patch and started
wondering if the lockmode for the children in
DetachPartitionFinalize() shouldn't be the same as used for the parent
mentioned in the DETACH PARTITION command:
@@ -18150,6 +18168,26 @@ DetachPartitionFinalize(Relation rel,
Relation partRel, bool concurrent,
* included in its partition descriptor.
*/
CacheInvalidateRelcache(rel);
+
+ /*
+ * If the partition we just detached is partitioned itself, invalidate
+ * relcache for all descendent partitions too to ensure that their
+ * rd_partcheck expression trees are rebuilt; must lock partitions
+ * before doing so.
+ */
+ if (partRel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ List *partRel_children =
+ find_all_inheritors(RelationGetRelid(partRel),
+ AccessExclusiveLock, NULL);
The lock taken on the parent is either ShareUpdateExclusiveLock or
AccessExclusiveLock depending on whether CONCURRENTLY is specified or
not. Maybe that should be considered also when locking the children.
I've updated the patch that way. (Also, reintroduced the slightly
longer commit message that I had added in v3. :))
--
Amit Langote
EDB: http://www.enterprisedb.com
v5-0001-Invalidate-partitions-of-table-being-attached-det.patch
Description: Binary data
