Moving work into new branch for PR against v5_STABLE#406
Moving work into new branch for PR against v5_STABLE#406susan-pgedge wants to merge 13 commits intov5_STABLEfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Up to standards ✅🟢 Issues
|
docs/managing/batch_inserts.md
Outdated
| # Using Batch Inserts | ||
|
|
||
| Using batch inserts improves replication performance for transactions that perform multiple inserts into a single table. To enable batch mode, modify the `postgresql.conf` file, setting: | ||
| Using batch inserts improves replication performance for transactions that |
There was a problem hiding this comment.
I see zero test coverage on this feature in the code. Maybe skip it or name it as an experimental feature?
docs/managing/batch_inserts.md
Outdated
| the `postgresql.conf` file, setting: | ||
|
|
||
| * the `spock.batch_inserts` parameter to `true`. | ||
| * the `spock.conflict_resolution` parameter to `error`. |
There was a problem hiding this comment.
If I read the code correctly, there is no such option as 'error':
static const struct config_enum_entry SpockConflictResolvers[] = {
/*
* Disabled until we can clearly define their desired behavior. Jan Wieck
* 2024-08-12
*
* {"error", SPOCK_RESOLVE_ERROR, false}, {"apply_remote",
* SPOCK_RESOLVE_APPLY_REMOTE, false}, {"keep_local",
* SPOCK_RESOLVE_KEEP_LOCAL, false}, {"first_update_wins",
* SPOCK_RESOLVE_FIRST_UPDATE_WINS, false},
*/
{"last_update_wins", SPOCK_RESOLVE_LAST_UPDATE_WINS, false},
{NULL, 0, false}
};
| ``` | ||
| Note that the value of `sub_enabled` is `t` if the subscription is currently replicating. | ||
| Note that the value of `sub_enabled` is `t` if the subscription is currently | ||
| replicating. |
There was a problem hiding this comment.
Are you sure? I think a more accurate wording is 'active', because the actual subscription status we extract from the spock.sub_show_status() call.
| provider and a subscriber node in a Spock logical replication setup. You can | ||
| use `spock.sync_event` to ensure that all changes up to a specific point | ||
| (indicated by the PostgreSQL Log Sequence Number or LSN) on the provider have | ||
| been received and applied on the subscriber. |
There was a problem hiding this comment.
Since commit fdd9587, I'm not totally sure. Their comment about bypassing the reorder buffer leaves me a little nervous about that. So it should be @mason-sharp , who would approve this statement.
| ### SYNOPSIS | ||
|
|
||
| `spock.spock_gen_slot_name (dbname name, provider_node name, subscription name)` | ||
| spock.spock_gen_slot_name(dbname name, provider_node name, |
There was a problem hiding this comment.
It looks like this function has a non-conventional name: the 'spock' used twice here, isn't it?
There was a problem hiding this comment.
I found that name referenced in a .sql file for the project - I think it's correct in the docs... @mason-sharp , comment?
|
|
||
| `spock.spock_gen_slot_name (dbname name, provider_node name, subscription name)` | ||
| spock.spock_gen_slot_name(dbname name, provider_node name, | ||
| subscription name) |
There was a problem hiding this comment.
@mason-sharp , shouldn't we do more in this function? I mean, check and reserve this name till the end of the transaction to avoid potential conflicts. Also, following ReplicationSlotNameForTablesync, we could use more compact and stable OIDs and GetSystemIdentifier() as Postgres core does.
| ## NAME | ||
|
|
||
| `spock.spock_max_proto_version()` | ||
| spock.spock_max_proto_version() |
There was a problem hiding this comment.
The same issue with naming.
Also, we might simplify the UI by consolidating these four functions into a single VIEW spock.info, which would be extensible and clearer. @mason-sharp , what do you think?
| The additional connection string to the node. The user in this string should equal the OS user. This connection string should be reachable from outside and match the one used later in the sub-create command. Example: host=10.1.2.5 port= 5432 user=rocky | ||
| node_add_interface | ||
| -------------------- | ||
| 1239112588 |
There was a problem hiding this comment.
It seems we should switch here from unstable hashes to stable OIDs, like we already do with node_id and like Postgres does. @mason-sharp ?
| Optional country code or name associated with the node. The default is | ||
| NULL. | ||
|
|
||
| info |
There was a problem hiding this comment.
This field might contain internal fields that affect the conflict-resolution logic. I'm not sure if it was documented somehow, maybe reference here?
There was a problem hiding this comment.
I'll need a dev to provide that information @danolivo
@rasifr or @ibrarahmad , is there anything else we should include here?
| inventory=# SELECT spock.node_create('n3','host=10.0.0.12 port=5432 dbname=inventory'); | ||
| node_create | ||
| ------------- | ||
| 9057 |
There was a problem hiding this comment.
@mason-sharp , the same question as for the interface OID generation.
| ### SYNOPSIS | ||
|
|
||
| `spock.node_info ()` | ||
| spock.node_info() |
There was a problem hiding this comment.
This place looks like a good place to add all the 'spock_version' stuff described earlier.
There was a problem hiding this comment.
@rasifr @ibrarahmad Can you share any information missing from this doc page?
No functional changes. Pure formatting: every C function CREATE statement in spock--5.0.0.sql, spock--5.0.0--5.0.1.sql, and spock--5.0.1--5.0.2.sql is reformatted: - One parameter per line, indented two spaces - IN parameter names column-aligned within each function - RETURNS clause on its own line - AS 'MODULE_PATHNAME', 'symbol' on its own line - LANGUAGE C [attributes] as the final line before the semicolon This matches the convention used by PostgreSQL core contrib extensions (pageinspect, amcheck, postgres_fdw, etc.) and makes signatures easier to read and diff.
danolivo
left a comment
danolivo
left a comment
There was a problem hiding this comment.
Almost all is good. Minor changes needed - see comments here and in Slack.
- Rename spock_sub_sync.md → spock_sub_alter_sync.md (sub_sync → sub_alter_sync)
- Delete spock_seq_sync.md (content already in spock_sync_seq.md)
- Rename spock_xact_timestamp_origin.md → spock_xact_commit_timestamp_origin.md
- Remove redundant spock. prefix from spock_version, spock_version_num, spock_max_proto_version, spock_min_proto_version docs
- Update forward_origins default from {all} to {} with explanation in sub_mgmt.md and spock_sub_create.md
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
There is a draft version for review at: TEMP contains the draft version