Remove unnecessary spock_shmem_attach routine

Being loaded on startup, Spock always initializes its shared memory segment
and shared HTABs in the postmaster process as well as all local pointers to
shared memory.

There is no case when the postmaster does not call shmem_startup_hook() after
cleaning up its shared memory. Also, there is no case when the postmaster
does any sensitive operations before initializing shared memory.

In case of recovery, evidence that shared memory is already initialized and
operable is the fact that redo operations can acquire locks.

Remove the 'attach' routine and its calls in checkpoint hook and bgworker
initialization to make the code more straightforward, reduce the code base,
and reduce potential errors.

Additional code quality improvements:
- Clarify TODO comment about undefined behavior with padding bytes
- Improve function documentation for return values (spock_group_progress_update)
- Remove redundant Assert statement that duplicated runtime check
- Clarify comments about progress structure initialization

This is a partial revert of commit c436818.

Author: danolivo

include/spock_group.h

@@ -156,7 +156,7 @@ typedef struct SpockGroupEntry
 
 /* shmem setup */
 extern void spock_group_shmem_request(void);
-extern void spock_group_shmem_startup(int napply_groups, bool found);
+extern void spock_group_shmem_startup(int napply_groups);
 
 SpockGroupEntry *spock_group_attach(Oid dbid, Oid node_id, Oid remote_node_id);
 void		spock_group_detach(void);

include/spock_worker.h

@@ -159,7 +159,6 @@ extern void handle_sigterm(SIGNAL_ARGS);
 extern void spock_subscription_changed(Oid subid, bool kill);
 
 extern void spock_worker_shmem_init(void);
-extern void spock_shmem_attach(void);
 
 extern int	spock_worker_register(SpockWorker *worker);
 extern void spock_worker_attach(int slot, SpockWorkerType type);

src/spock_group.c

@@ -113,16 +113,13 @@ spock_group_shmem_request(void)
  * Initialize shared resources for db-origin management
  */
 void
-spock_group_shmem_startup(int napply_groups, bool found)
+spock_group_shmem_startup(int napply_groups)
 {
 	HASHCTL		hctl;
 
 	if (prev_shmem_startup_hook != NULL)
 		prev_shmem_startup_hook();
 
-	if (SpockGroupHash)
-		return;
-
 	MemSet(&hctl, 0, sizeof(hctl));
 	hctl.keysize = sizeof(SpockGroupKey);
 	hctl.entrysize = sizeof(SpockGroupEntry);
@@ -137,35 +134,16 @@ spock_group_shmem_startup(int napply_groups, bool found)
 								   HASH_SHARED_MEM | HASH_PARTITION |
 								   HASH_FIXED_SIZE);
 
-	if (!SpockGroupHash)
-		elog(ERROR, "spock_group_shmem_startup: failed to init group map");
-
-	/*
-	 * If the shared memory structures already existed (found = true), then
-	 * we're a background process attaching to structures created by the
-	 * postmaster. The hash was already seeded from the file during postmaster
-	 * startup, so skip loading.
-	 *
-	 * If found = false, we're the postmaster doing initial setup. Load the
-	 * file to quickly seed the hash, then WAL recovery will run afterward and
-	 * provide authoritative updates.
-	 *
-	 * Note: ShmemInitHash() doesn't have a 'found' output parameter like
-	 * ShmemInitStruct(), so we rely on the 'found' status of other Spock
-	 * structures (SpockCtx, etc.) as a proxy since they're all created
-	 * together.
-	 */
-	if (found)
-		return;
-
-	elog(DEBUG1, "spock_group_shmem_startup: loading resource file to seed hash");
 	spock_group_resource_load();
+	elog(DEBUG1,
+		 "spock_group_shmem_startup: loading resource file to seed hash. loaded records: %lu",
+		 hash_get_num_entries(SpockGroupHash));
 }
 
 /*
- * TODO:
- * here is always looming issue with UB around padding until we implement
- * our own 'hash' and 'cmp' routines for the key operations.
+ * TODO: There is a looming issue with undefined behavior (UB) due to
+ * uninitialized padding bytes in the key structure. We should implement
+ * custom hash and comparison routines that avoid memcmp on padded structs.
  */
 static inline SpockGroupKey
 make_key(Oid dbid, Oid node_id, Oid remote_node_id)
@@ -617,15 +595,6 @@ spock_checkpoint_hook(XLogRecPtr checkPointRedo, int flags)
 	if ((flags & (CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_END_OF_RECOVERY)) == 0)
 		return;
 
-	/*
-	 * Ensure we're attached to shared memory before accessing it.
-	 *
-	 * spock_shmem_attach() is idempotent - it tracks attachment per process
-	 * and only does the actual work once, so it's safe to call on every
-	 * qualifying checkpoint.
-	 */
-	spock_shmem_attach();
-
 	/* Dump group progress to resource.dat */
 	spock_group_resource_dump();
 }

src/spock_output_plugin.c

@@ -1403,8 +1403,6 @@ spock_output_plugin_shmem_startup(void)
 
 	/* Get the shared resources */
 	LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
-	SpockCtx->slot_group_master_lock = &((GetNamedLWLockTranche("spock_slot_groups")[0]).lock);
-	SpockCtx->slot_ngroups = nworkers;
 	slot_groups = ShmemInitStruct("spock_slot_groups",
 								  spock_output_plugin_shmem_size(nworkers),
 								  &found);

src/spock_rmgr.c

@@ -156,15 +156,6 @@ spock_rmgr_identify(uint8 info)
 void
 spock_rmgr_startup(void)
 {
-	/*
-	 * During WAL recovery in the startup process, we need to attach to the
-	 * shared memory structure (SpockGroupHash) that was created by the
-	 * postmaster's shmem_startup_hook.
-	 *
-	 * spock_shmem_attach() handles resetting inherited globals and properly
-	 * re-attaching to shared memory before WAL replay begins.
-	 */
-	spock_shmem_attach();
 }
 
 void

src/spock_worker.c

@@ -79,7 +79,7 @@ static void wait_for_worker_startup(SpockWorker *worker,
 									BackgroundWorkerHandle *handle);
 static void signal_worker_xact_callback(XactEvent event, void *arg);
 static uint32 spock_ch_stats_hash(const void *key, Size keysize);
-static bool spock_shmem_init_internal(int nworkers);
+static void spock_shmem_init_internal(int nworkers);
 
 
 void
@@ -344,15 +344,6 @@ spock_worker_on_exit(int code, Datum arg)
 void
 spock_worker_attach(int slot, SpockWorkerType type)
 {
-	/*
-	 * Ensure shared memory is attached before using SpockCtx.
-	 *
-	 * Background workers inherit global variables from the postmaster but the
-	 * pointers may not be valid. Always call spock_shmem_attach() which is
-	 * idempotent and handles proper re-initialization.
-	 */
-	spock_shmem_attach();
-
 	Assert(slot >= 0);
 	Assert(slot < SpockCtx->total_workers);
 
@@ -838,9 +829,18 @@ spock_worker_shmem_startup(void)
 	 * This is kludge for Windows (Postgres does not define the GUC variable
 	 * as PGDLLIMPORT)
 	 */
-	nworkers = atoi(GetConfigOptionByName("max_worker_processes", NULL,
-										  false));
+	nworkers = atoi(GetConfigOptionByName("max_worker_processes", NULL, false));
+
+	/*
+	 * Reset in case this is a restart within the postmaster
+	 * Spock extension must be loaded on startup. In case of a fatal error and
+	 * further restart, postmaster clean up shared memory but local variables
+	 * stay the same. So, we need to clean outdated pointers in advance.
+	 */
 	SpockCtx = NULL;
+	SpockHash = NULL;
+	SpockGroupHash = NULL;
+	exception_log_ptr = NULL;
 
 	/* Avoid possible race-conditions when initializing shared memory. */
 	LWLockAcquire(AddinShmemInitLock, LW_EXCLUSIVE);
@@ -850,7 +850,7 @@ spock_worker_shmem_startup(void)
 	 * This internally calls spock_group_shmem_startup() to handle group
 	 * initialization and file loading.
 	 */
-	(void) spock_shmem_init_internal(nworkers);
+	spock_shmem_init_internal(nworkers);
 
 	LWLockRelease(AddinShmemInitLock);
 }
@@ -878,70 +878,52 @@ spock_ch_stats_hash(const void *key, Size keysize)
  * This centralizes the logic for setting up all Spock shared memory pointers,
  * which is used by both the initial startup hook and the attach function.
  */
-static bool
+static void
 spock_shmem_init_internal(int nworkers)
 {
 	HASHCTL		hctl;
 	bool		found;
-	bool		all_found = true;
-
 
 	/* Init signaling context for the various processes. */
-	if (!SpockCtx)
+	SpockCtx = ShmemInitStruct("spock_context",
+							   worker_shmem_size(nworkers, false), &found);
+	if (!found)
 	{
-		SpockCtx = ShmemInitStruct("spock_context",
-								   worker_shmem_size(nworkers, false), &found);
-		if (!SpockCtx)
-			elog(ERROR, "failed to initialize spock_context");
-
-		if (!found)
-		{
-			/* First time - initialize the structure */
-			SpockCtx->lock = &((GetNamedLWLockTranche("spock")[0]).lock);
-			SpockCtx->apply_group_master_lock = &((GetNamedLWLockTranche(SPOCK_GROUP_TRANCHE_NAME)[0]).lock);
-			SpockCtx->supervisor = NULL;
-			SpockCtx->subscriptions_changed = false;
-			SpockCtx->total_workers = nworkers;
-			memset(SpockCtx->workers, 0,
-				   sizeof(SpockWorker) * SpockCtx->total_workers);
-			all_found = false;
-		}
+		/* First time - initialize the structure */
+		SpockCtx->lock = &((GetNamedLWLockTranche("spock")[0]).lock);
+		SpockCtx->apply_group_master_lock = &((GetNamedLWLockTranche(SPOCK_GROUP_TRANCHE_NAME)[0]).lock);
+		SpockCtx->slot_group_master_lock = &((GetNamedLWLockTranche("spock_slot_groups")[0]).lock);
+		SpockCtx->slot_ngroups = nworkers;
+		SpockCtx->supervisor = NULL;
+		SpockCtx->subscriptions_changed = false;
+		SpockCtx->total_workers = nworkers;
+		memset(SpockCtx->workers, 0,
+			   sizeof(SpockWorker) * SpockCtx->total_workers);
 	}
 
 	/*
 	 * Initialize exception log pointer array.
 	 */
-	if (!exception_log_ptr)
-	{
-		exception_log_ptr = ShmemInitStruct("spock_exception_log_ptr",
-											worker_shmem_size(nworkers, false), &found);
-		if (!exception_log_ptr)
-			elog(ERROR, "failed to initialize spock_exception_log_ptr");
+	exception_log_ptr = ShmemInitStruct("spock_exception_log_ptr",
+										worker_shmem_size(nworkers, false), &found);
 
-		if (!found)
-		{
-			memset(exception_log_ptr, 0, sizeof(SpockExceptionLog) * nworkers);
-			all_found = false;
-		}
+	if (!found)
+	{
+		memset(exception_log_ptr, 0, sizeof(SpockExceptionLog) * nworkers);
 	}
 
 	/*
 	 * Initialize SpockHash - channel stats hash.
 	 */
-	if (!SpockHash)
-	{
-		memset(&hctl, 0, sizeof(hctl));
-		hctl.keysize = sizeof(spockStatsKey);
-		hctl.entrysize = sizeof(spockStatsEntry);
-		hctl.hash = spock_ch_stats_hash;
-		SpockHash = ShmemInitHash("spock channel stats hash",
-								  spock_stats_max_entries,
-								  spock_stats_max_entries,
-								  &hctl,
-								  HASH_ELEM | HASH_FUNCTION | HASH_FIXED_SIZE);
-		if (!SpockHash)
-			elog(ERROR, "failed to initialize spock channel stats hash");
-	}
+	memset(&hctl, 0, sizeof(hctl));
+	hctl.keysize = sizeof(spockStatsKey);
+	hctl.entrysize = sizeof(spockStatsEntry);
+	hctl.hash = spock_ch_stats_hash;
+	SpockHash = ShmemInitHash("spock channel stats hash",
+							  spock_stats_max_entries,
+							  spock_stats_max_entries,
+							  &hctl,
+							  HASH_ELEM | HASH_FUNCTION | HASH_FIXED_SIZE);
 
 	/*
 	 * Initialize SpockGroupHash via the spock_group module. This handles both
@@ -950,68 +932,7 @@ spock_shmem_init_internal(int nworkers)
 	 * Note: We pass 'all_found' to indicate whether this is first-time setup
 	 * or attachment to existing structures.
 	 */
-	spock_group_shmem_startup(nworkers, all_found);
-
-	return all_found;
-}
-
-/*
- * spock_shmem_attach
- *
- * Attach (or re-attach) to existing shared memory structures.
- *
- * This is called from processes that need to access Spock's shared memory:
- * - Startup process (via spock_rmgr_startup) - once per recovery
- * - Checkpointer (via spock_checkpoint_hook) - once per process lifetime
- * - Background workers (via spock_worker_attach) - once per worker
- *
- * IMPORTANT: Auxiliary processes (startup, checkpointer) inherit global
- * variable values from the postmaster, but these pointers might not be valid
- * in their address space.
- *
- * This function uses a static flag to ensure we only attach once per process,
- * avoiding redundant shared memory lookups on subsequent calls.
- *
- * Unlike spock_worker_shmem_startup(), this doesn't acquire AddinShmemInitLock
- * or register hooks - it just looks up and attaches to existing structures.
- */
-void
-spock_shmem_attach(void)
-{
-	static bool attached = false;
-	int			nworkers;
-
-	/* If already attached in this process, nothing to do */
-	if (attached)
-		return;
-
-	/*
-	 * Reset globals to NULL to force proper attachment.
-	 *
-	 * This is critical for auxiliary processes (startup, checkpointer) that
-	 * inherit invalid global values from the postmaster.
-	 */
-	SpockCtx = NULL;
-	SpockHash = NULL;
-	SpockGroupHash = NULL;
-	exception_log_ptr = NULL;
-
-	/*
-	 * Get max_worker_processes to know the size of structures.
-	 */
-	nworkers = atoi(GetConfigOptionByName("max_worker_processes", NULL, false));
-	if (nworkers <= 0)
-		nworkers = 9;
-
-	/*
-	 * Attach to all structures using the common initialization logic. Returns
-	 * true if structures were found (normal case), false if they had to be
-	 * created (shouldn't happen but harmless).
-	 */
-	(void) spock_shmem_init_internal(nworkers);
-
-	/* Mark as attached to avoid redundant work on subsequent calls */
-	attached = true;
+	spock_group_shmem_startup(nworkers);
 }
 
 /*