Spoc 384 (second part) #298
Open
+392
−383
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
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.
Shape the spock_group module a little: * Remove unused lookup function. * Make spock_group_foreach static, forasmuch as no one uses it outside the module. * Add forgotten ‘extern’ declarations.
There are plenty of places where Spock touches shared memory structures that may cause races. It is always an issue, even considering these structures are rarely accessed for writing. But the progress state is a high-frequency updated structure, and guarding it with a lock is critical.
After moving to HTAB implementation, we actually don't have a target connection. So, remove it. While here, fix forgotten 'extern' declaration in the 'spock_group.h' module.
In Spock 5, we used to employ a progress table and a transactional snapshot to ensure that the progress corresponds to the replication slot (snapshot, LSN), and we could request this state at any time until the transaction ends. With an HTAB, it's no longer true, and we need to invent extra hacks. With this commit, we take the progress state right before LR slot creation, which is as close as possible to the real state. It is still not entirely consistent in the case of Z0DAN, but while we use it just for information, it is ok. XXX: The code related to table syncing stays as is. Right now, we aren't sure if the progress state needs to be corrected if we re-sync a single table from the database. So, just keep the code untouched.
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.
No description provided.