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}
};
|
|
||
| * `spock.enable_ddl_replication` enables replication of ddl statements | ||
| through the default replication set. Some DDL statements are intentionally | ||
| not replicated (like `CREATE DATABASE`). Other DDL statements are |
There was a problem hiding this comment.
Just note. With an AI helper, we might maintain exact lists of replicated/restricted DDL and utility commands and reference them here.
There was a problem hiding this comment.
@danolivo If you want to share that/those lists, I'll be glad to add them!
| ``` | ||
| 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?
|
|
||
| `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?
| 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.
| only removes Spock's logical representation of the interface. | ||
|
|
||
| Other nodes that were using this interface to connect will no longer be able | ||
| to use it. Ensure no active subscriptions are relying on this interface |
There was a problem hiding this comment.
Vague term 'ensure'. What may happen if I forget to clean all dependencies? Crash, error, something else?
| ### 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.
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