Remove extra key field from SpockGroupEntry struct - part of SPOC-384 #294
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
danolivo
force-pushed
the
spoc-384
branch
2 times, most recently
from
c0022dc to
653f91f
Compare
The SpockGroupEntry structure contained a redundant key field that duplicated the key already embedded in its progress member (SpockGroupEntry.key vs SpockGroupEntry.progress.key). This redundancy violated the DRY principle and created potential for inconsistency if the two keys ever diverged. This commit removes the duplicate key field from SpockGroupEntry, relying solely on the key embedded in SpockApplyProgress. The hash table already uses SpockProgressKey (the first field of SpockApplyProgress) as the hash key, making the separate SpockGroupEntry.key field unnecessary. Changes: 1. Structural cleanup: - Removed SpockGroupEntry.key field (the duplicate) - Kept SpockApplyProgress.key as the single source of truth - Renamed SpockGroupKey to SpockProgressKey for clarity 2. Code quality improvements: - Created init_progress_fields() helper for safe initialization - Replaced fragile pointer arithmetic with explicit field initialization - Follows PostgreSQL conventions (similar to CommitTsShmemInit) - Added comprehensive documentation explaining hash table design 3. Documentation and comments: - Added header explaining Group Registry architecture - Clarified that SpockApplyProgress.key MUST be first field - Documented persistence strategy (WAL + file snapshot) - Improved TODO comments to be actionable The functional behavior is unchanged; this is a refactoring that improves code quality and reduces redundancy.
Being loaded via shared_preload_libraries, Spock always initializes its
shared memory segment and hash tables in the postmaster process during
shmem_startup_hook. The shmem_startup_hook is called exactly once per
postmaster lifetime while holding AddinShmemInitLock.
There is no case where the postmaster does not call shmem_startup_hook()
after allocating shared memory. Also, there is no case where the postmaster
performs any operations requiring Spock's shared memory before initialization.
During recovery, the evidence that shared memory is already initialized and
operable is the fact that redo operations can acquire locks and access the
hash tables.
This commit removes the redundant spock_shmem_attach() routine and its calls
from checkpoint hook, WAL redo startup, and bgworker initialization. This
simplifies the code by eliminating the attach/detach complexity.
Changes:
1. Removed spock_shmem_attach() function entirely
- Startup process, checkpointer, and bgworkers no longer need to "attach"
- All processes can directly access shared memory after postmaster init
- Removed the static 'attached' flag tracking per-process attachment
2. Simplified spock_group_shmem_startup()
- Removed 'found' parameter (always creating new structures)
- Removed conditional file loading (always load on initialization)
- The function is only called once via shmem_startup_hook, so checks
for existing structures are unnecessary
3. Simplified spock_shmem_init_internal()
- Removed 'found' return value and tracking
- Removed conditional initialization (if (!SpockCtx), etc.)
- ShmemInit* functions are safely called once under AddinShmemInitLock
- Changed return type from bool to void
4. Code quality improvements
- Updated comments to reflect simplified initialization model
- Improved DEBUG log message clarity
- Removed outdated references to 'all_found' parameter
Benefits:
- Eliminates ~70 lines of unnecessary attachment logic
- Makes initialization flow clearer and easier to understand
- Removes per-process attachment tracking overhead
- Prevents potential bugs from missed spock_shmem_attach() calls
The functional behavior is unchanged; all processes still have access to
Spock's shared memory structures after postmaster startup.
danolivo
changed the title
Contributor
Author
|
@mason-sharp, this fix is ready to be discovered. Please feel free to set up a meeting in the next two hours if you have any questions. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit made it clear that the recently added (c436818) spock_shmem_attach is unnecessary, complicates the code, and is likely to cause future bugs. I removed that old stuff and manually tested how it works during recovery, during checkpoints, and after a hard postmaster SIGKILL.