Re: Timing of relcache inval at parallel worker init

At Sat, 17 Oct 2020 04:53:06 -0700, Noah Misch <noah(at)leadboat(dot)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