Skip to content

Commit

Permalink
Avoid re-assigning the global pid when we cannot retrieve local node …
Browse files Browse the repository at this point in the history
…id without unsafe catalog access
  • Loading branch information
onurctirtir committed Dec 17, 2024
1 parent 0355b12 commit 381231e
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 1 deletion.
10 changes: 10 additions & 0 deletions src/backend/distributed/metadata/metadata_cache.c
Original file line number Diff line number Diff line change
Expand Up @@ -4545,6 +4545,16 @@ GetLocalNodeId(void)
}


/*
* CachedLocalNodeIdIsValid return true if the cached local node id is valid.
*/
bool
CachedLocalNodeIdIsValid(void)
{
return LocalNodeId != -1;
}


/*
* RegisterLocalGroupIdCacheCallbacks registers the callbacks required to
* maintain LocalGroupId at a consistent value. It's separate from
Expand Down
10 changes: 9 additions & 1 deletion src/backend/distributed/shared_library_init.c
Original file line number Diff line number Diff line change
Expand Up @@ -2899,6 +2899,13 @@ ApplicationNameAssignHook(const char *newval, void *extra)
* So we set the FinishedStartupCitusBackend flag in StartupCitusBackend to
* indicate when this responsibility handoff has happened.
*
* On the other hand, even if now it's this hook's responsibility to update
* the global pid, we cannot do so if the cached local node id is invalidated
* and we're not allowed to access the catalog tables. Within a transaction
* block, we can access the catalog tables. For this reason, in addition to
* checking FinishedStartupCitusBackend, we also require either being in a
* transaction block or the cached local node id to be valid.
*
* Another solution to the catalog table acccess problem would be to update
* global pid lazily, like we do for HideShards. But that's not possible
* for the global pid, since it is stored in shared memory instead of in a
Expand All @@ -2907,7 +2914,8 @@ ApplicationNameAssignHook(const char *newval, void *extra)
* as reasonably possible, which is also why we extract global pids in the
* AuthHook already (extracting doesn't require catalog access).
*/
if (FinishedStartupCitusBackend)
if (FinishedStartupCitusBackend &&
(IsTransactionState() || CachedLocalNodeIdIsValid()))
{
AssignGlobalPID(newval);
}
Expand Down
1 change: 1 addition & 0 deletions src/include/distributed/metadata_cache.h
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ extern CitusTableCacheEntry * LookupCitusTableCacheEntry(Oid relationId);
extern DistObjectCacheEntry * LookupDistObjectCacheEntry(Oid classid, Oid objid, int32
objsubid);
extern int32 GetLocalGroupId(void);
extern bool CachedLocalNodeIdIsValid(void);
extern int32 GetLocalNodeId(void);
extern void CitusTableCacheFlushInvalidatedEntries(void);
extern Oid LookupShardRelationFromCatalog(int64 shardId, bool missing_ok);
Expand Down

0 comments on commit 381231e

Please sign in to comment.