At Sat, 17 Oct 2020 04:53:06 -0700, Noah Misch <n...@leadboat.com> wrote in > While reviewing what became commit fe4d022, I was surprised at the sequence of > relfilenode values that RelationInitPhysicalAddr() computed for pg_class, > during ParallelWorkerMain(), when running the last command of this recipe: > > begin; > cluster pg_class using pg_class_oid_index; > set force_parallel_mode = 'regress'; > values (1); > > There's $OLD_NODE (relfilenode in the committed relation map) and $NEW_NODE > (relfilenode in this transaction's active_local_updates). The worker performs > RelationInitPhysicalAddr(pg_class) four times: > > 1) $OLD_NODE in BackgroundWorkerInitializeConnectionByOid(). > 2) $OLD_NODE in RelationCacheInvalidate() directly. > 3) $OLD_NODE in RelationReloadNailed(), indirectly via > RelationCacheInvalidate(). > 4) $NEW_NODE indirectly as part of the executor running the query. > > I did expect $OLD_NODE in (1), since ParallelWorkerMain() calls > BackgroundWorkerInitializeConnectionByOid() before > StartParallelWorkerTransaction(). I expected $NEW_NODE in (2) and (3); that > didn't happen, because ParallelWorkerMain() calls InvalidateSystemCaches() > before RestoreRelationMap(). Let's move InvalidateSystemCaches() later. > Invalidation should follow any worker initialization step that changes the > results of relcache validation; otherwise, we'd need to ensure the > InvalidateSystemCaches() will not validate any relcache entry. Invalidation > should precede any step that reads from a cache; otherwise, we'd need to redo > that step after inval. (Currently, no step reads from a cache.) Many steps, > e.g. AttachSerializableXact(), have no effect on relcache validation, so it's > arbitrary whether they happen before or after inval. I'm putting inval as
I agree to both the discussions. > late as possible, because I think it's easier to confirm that a step doesn't > read from a cache than to confirm that a step doesn't affect relcache > validation. An also-reasonable alternative would be to move inval and its > prerequisites as early as possible. The steps became moved before the invalidation by this patch seems to be in the lower layer than snapshot, so it seems to be reasonable. > For reasons described in the attached commit message, this doesn't have > user-visible consequences today. Innocent-looking relcache.c changes might > upheave that, so I'm proposing this on robustness grounds. No need to > back-patch. I'm not sure about the necessity but lower-to-upper initialization order is neat. I agree about not back-patching. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
