Documentation review changes#460
Conversation
📝 WalkthroughWalkthroughThis pull request comprehensively updates Spock's documentation to clarify function signatures, error behaviors, parameter naming, and feature behavior. The changes reflect that conflict resolution now only supports ChangesSpock Documentation API Clarification
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/modify/zodan/zodan_tutorial.md`:
- Around line 755-756: The doc currently lists "1200 seconds = 20 minutes" here
but earlier mentions an "internal 180-second timeout"; reconcile by either
aligning the numbers or adding an explicit clarification: update the sentence
that reads "The timeout (1200 seconds = 20 minutes) prevents waiting forever if
something goes wrong." to explain that the 1200s value is the overall
operation/monitoring timeout while ZODAN's internal per-request timeout remains
180 seconds (or change one of the values so both refer to the same timeout), and
also update the earlier "internal 180-second timeout" sentence to reference the
overall timeout if you choose to keep 1200s—ensure both phrases ("1200 seconds =
20 minutes" and "internal 180-second timeout") are consistent or clearly
differentiated in wording.
- Around line 847-848: The examples documenting pg_replication_slot_advance
incorrectly declare the result as AS t(result bool); update both occurrences to
reflect the actual return signature of pg_replication_slot_advance by using a
record type with the two columns (slot_name name, end_lsn pg_lsn) so the
examples match PostgreSQL v17/v18 and will copy/paste without errors; locate the
examples around the pg_replication_slot_advance usage and replace the incorrect
AS t(result bool) with an AS clause declaring (slot_name name, end_lsn pg_lsn)
for both instances.
In `@docs/monitoring/spock_sync_event.md`:
- Around line 71-78: The earlier inline call to spock.wait_for_sync_event() is
misleading because the function requires parameters; update the intro/reference
(where it currently shows spock.wait_for_sync_event() on Line 19) to a symbolic
form such as spock.wait_for_sync_event(...) so readers know it’s illustrative
and not a valid zero-arg invocation; ensure references to the procedure name
spock.wait_for_sync_event and the OUT placeholder usage (NULL) remain consistent
with the full example shown later.
In `@docs/spock_functions/gen_mgmt.md`:
- Around line 8-12: The table rows for spock_max_proto_version,
spock_min_proto_version, spock.node_info, spock_version, and spock_version_num
use trailing pipe style; update each row in docs/spock_functions/gen_mgmt.md to
use the markdownlint MD055 "leading_only" pipe style by removing the trailing
pipe at the end of each table line while keeping the same cell order and content
so the table renders identically but complies with the linter.
In `@docs/spock_functions/index.md`:
- Around line 23-25: The table rows for the spock function links (e.g.,
spock.repset_add_seq, spock.repset_add_all_seqs, spock.repset_remove_seq and the
other rows around the 44-53 block) are using leading-only pipe style; update
each Markdown table row to include trailing pipes so the table uses
leading_and_trailing style (add a trailing "|" at the end of each affected row)
to satisfy MD055.
In `@docs/spock_functions/repset_mgmt.md`:
- Around line 213-214: The quick-start "Removing a Table" SQL snippet uses the
old two-argument form; update that snippet to match the documented signature
spock.repset_remove_table(set_name name, relation regclass, include_partitions
boolean DEFAULT true) by adding the include_partitions boolean argument
(true/false as appropriate) to the call so both snippets show the same
three-argument form and reflect the include_partitions behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 78ffb7ac-e7ef-4502-9463-0a34d1e58f7b
📒 Files selected for processing (27)
README.mddocs/configuring.mddocs/conflict_types.mddocs/limitations.mddocs/managing/read_only.mddocs/managing/spock_autoddl.mddocs/modify/zodan/zodan_tutorial.mddocs/monitoring/spock_info.mddocs/monitoring/spock_sync_event.mddocs/recovery/catastrophic_node_failure.mddocs/spock_functions/functions/spock_max_proto_version.mddocs/spock_functions/functions/spock_min_proto_version.mddocs/spock_functions/functions/spock_node_drop_interface.mddocs/spock_functions/functions/spock_repset_add_seq.mddocs/spock_functions/functions/spock_repset_add_table.mddocs/spock_functions/functions/spock_repset_remove_seq.mddocs/spock_functions/functions/spock_repset_remove_table.mddocs/spock_functions/functions/spock_sub_add_repset.mddocs/spock_functions/functions/spock_sub_alter_interface.mddocs/spock_functions/functions/spock_sync_event.mddocs/spock_functions/gen_mgmt.mddocs/spock_functions/index.mddocs/spock_functions/repset_mgmt.mddocs/spock_functions/sub_mgmt.mddocs/spock_release_notes.mddocs/troubleshooting.mddocs/two_node_cluster.md
| The timeout (1200 seconds = 20 minutes) prevents waiting forever | ||
| if something goes wrong. |
There was a problem hiding this comment.
Timeout value is inconsistent with the earlier prerequisite note.
This section says 1200 seconds, but the earlier warning still says ZODAN uses an internal 180-second timeout. Please reconcile or explicitly explain the difference to avoid operator confusion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/modify/zodan/zodan_tutorial.md` around lines 755 - 756, The doc
currently lists "1200 seconds = 20 minutes" here but earlier mentions an
"internal 180-second timeout"; reconcile by either aligning the numbers or
adding an explicit clarification: update the sentence that reads "The timeout
(1200 seconds = 20 minutes) prevents waiting forever if something goes wrong."
to explain that the 1200s value is the overall operation/monitoring timeout
while ZODAN's internal per-request timeout remains 180 seconds (or change one of
the values so both refer to the same timeout), and also update the earlier
"internal 180-second timeout" sentence to reference the overall timeout if you
choose to keep 1200s—ensure both phrases ("1200 seconds = 20 minutes" and
"internal 180-second timeout") are consistent or clearly differentiated in
wording.
| ) AS t(result bool); | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PostgreSQL documentation (v17/v18): What is the exact return signature of pg_replication_slot_advance(slot_name, lsn)?
💡 Result:
The exact return signature of pg_replication_slot_advance(slot_name name, upto_lsn pg_lsn) is a record with fields (slot_name name, end_lsn pg_lsn). This is consistent across PostgreSQL v17 [1] and v18 [2] official documentation, as well as secondary sources like pgPedia [3] and source code [4]. The function advances the confirmed position of the named replication slot to the specified LSN (or as far as possible without going backwards or beyond the current WAL insert location) and returns the slot name and the actual end LSN achieved [1][2].
Citations:
- 1: http://www.postgresql.org/docs/17/functions-admin.html
- 2: https://www.postgresql.org/docs/current/functions-admin.html
- 3: https://pgpedia.info/p/pg_replication_slot_advance.html
- 4: https://github.com/postgres/postgres/blob/0b84f5c419a300dc1b1a70cf63b9907208e52643/src/backend/replication/slotfuncs.c
pg_replication_slot_advance result type is documented incorrectly in both examples.
AS t(result bool) is incorrect. The function returns (slot_name name, end_lsn pg_lsn) per PostgreSQL v17/v18 documentation, not a single boolean field. This will cause copy/paste failures when used in actual PostgreSQL code.
Proposed doc fix
-) AS t(result bool);
+) AS t(slot_name text, end_lsn pg_lsn);Also applies to: lines 878-879
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/modify/zodan/zodan_tutorial.md` around lines 847 - 848, The examples
documenting pg_replication_slot_advance incorrectly declare the result as AS
t(result bool); update both occurrences to reflect the actual return signature
of pg_replication_slot_advance by using a record type with the two columns
(slot_name name, end_lsn pg_lsn) so the examples match PostgreSQL v17/v18 and
will copy/paste without errors; locate the examples around the
pg_replication_slot_advance usage and replace the incorrect AS t(result bool)
with an AS clause declaring (slot_name name, end_lsn pg_lsn) for both instances.
| ```sql | ||
| CALL spock.wait_for_sync_event(NULL, 'provider_node', '0/16342B0', 10); | ||
| -- result: true (if applied within 10s), false otherwise | ||
| ``` | ||
|
|
||
| The first parameter is the OUT `result` placeholder; pass `NULL` for it in | ||
| the `CALL` statement and read the OUT value from the procedure result. | ||
|
|
There was a problem hiding this comment.
Clarify the earlier wait_for_sync_event invocation style to avoid a misleading zero-arg call.
This updated example is correct, but the earlier inline spock.wait_for_sync_event() (Line 19) reads like a literal invocation and conflicts with the required parameters shown here. Please make the intro reference explicitly symbolic (for example, spock.wait_for_sync_event(...)) for consistency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/monitoring/spock_sync_event.md` around lines 71 - 78, The earlier inline
call to spock.wait_for_sync_event() is misleading because the function requires
parameters; update the intro/reference (where it currently shows
spock.wait_for_sync_event() on Line 19) to a symbolic form such as
spock.wait_for_sync_event(...) so readers know it’s illustrative and not a valid
zero-arg invocation; ensure references to the procedure name
spock.wait_for_sync_event and the OUT placeholder usage (NULL) remain consistent
with the full example shown later.
| | [spock_max_proto_version](functions/spock_max_proto_version.md) | Returns the maximum protocol version supported by Spock. Defined in the public schema (no `spock.` prefix). | | ||
| | [spock_min_proto_version](functions/spock_min_proto_version.md) | Returns the minimum protocol version supported by Spock. Defined in the public schema (no `spock.` prefix). | | ||
| | [spock.node_info](functions/spock_node_info.md) | Returns information about the local Spock node. | | ||
| | [spock.version](functions/spock_version.md) | Returns the version string of the Spock extension. | | ||
| | [spock.version_num](functions/spock_version_num.md) | Returns the version number of Spock as an integer. | | ||
| | [spock_version](functions/spock_version.md) | Returns the version string of the Spock extension. Defined in the public schema (no `spock.` prefix). | | ||
| | [spock_version_num](functions/spock_version_num.md) | Returns the version number of Spock as an integer. Defined in the public schema (no `spock.` prefix). | |
There was a problem hiding this comment.
Fix table pipe style to match markdownlint config.
These rows use trailing pipes, but markdownlint expects leading_only style (MD055) in this file.
Suggested fix
-| [spock_max_proto_version](functions/spock_max_proto_version.md) | Returns the maximum protocol version supported by Spock. Defined in the public schema (no `spock.` prefix). |
-| [spock_min_proto_version](functions/spock_min_proto_version.md) | Returns the minimum protocol version supported by Spock. Defined in the public schema (no `spock.` prefix). |
+| [spock_max_proto_version](functions/spock_max_proto_version.md) | Returns the maximum protocol version supported by Spock. Defined in the public schema (no `spock.` prefix).
+| [spock_min_proto_version](functions/spock_min_proto_version.md) | Returns the minimum protocol version supported by Spock. Defined in the public schema (no `spock.` prefix).
| [spock.node_info](functions/spock_node_info.md) | Returns information about the local Spock node. |
-| [spock_version](functions/spock_version.md) | Returns the version string of the Spock extension. Defined in the public schema (no `spock.` prefix). |
-| [spock_version_num](functions/spock_version_num.md) | Returns the version number of Spock as an integer. Defined in the public schema (no `spock.` prefix). |
+| [spock_version](functions/spock_version.md) | Returns the version string of the Spock extension. Defined in the public schema (no `spock.` prefix).
+| [spock_version_num](functions/spock_version_num.md) | Returns the version number of Spock as an integer. Defined in the public schema (no `spock.` prefix).🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 8-8: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
[warning] 9-9: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
[warning] 10-10: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
[warning] 11-11: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
[warning] 12-12: Table pipe style
Expected: leading_only; Actual: leading_and_trailing; Unexpected trailing pipe
(MD055, table-pipe-style)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/spock_functions/gen_mgmt.md` around lines 8 - 12, The table rows for
spock_max_proto_version, spock_min_proto_version, spock.node_info,
spock_version, and spock_version_num use trailing pipe style; update each row in
docs/spock_functions/gen_mgmt.md to use the markdownlint MD055 "leading_only"
pipe style by removing the trailing pipe at the end of each table line while
keeping the same cell order and content so the table renders identically but
complies with the linter.
| | [spock.repset_add_seq](functions/spock_repset_add_seq.md) | Adds a sequence to a replication set. | ||
| | [spock.repset_add_all_seqs](functions/spock_repset_add_all_seqs.md) | Adds all sequences from the specified schemas to a replication set. | ||
| | [spock.repset_remove_seq](functions/spock_repset_remove_seq.md) | Remove a sequence from a replication set. |
There was a problem hiding this comment.
Add missing trailing pipes for table rows (MD055).
These changed rows are using leading_only style, but this file expects leading_and_trailing pipe style per markdownlint.
Suggested fix
-| [spock.repset_add_seq](functions/spock_repset_add_seq.md) | Adds a sequence to a replication set.
-| [spock.repset_add_all_seqs](functions/spock_repset_add_all_seqs.md) | Adds all sequences from the specified schemas to a replication set.
-| [spock.repset_remove_seq](functions/spock_repset_remove_seq.md) | Remove a sequence from a replication set.
+| [spock.repset_add_seq](functions/spock_repset_add_seq.md) | Adds a sequence to a replication set. |
+| [spock.repset_add_all_seqs](functions/spock_repset_add_all_seqs.md) | Adds all sequences from the specified schemas to a replication set. |
+| [spock.repset_remove_seq](functions/spock_repset_remove_seq.md) | Remove a sequence from a replication set. |
...
-| spock_version | Returns the Spock version in a major/minor version form: `4.0.10`. Defined in the public schema (no `spock.` prefix).
-| spock_version_num | Returns the Spock version in a single numeric form: `40010`. Defined in the public schema (no `spock.` prefix).
+| spock_version | Returns the Spock version in a major/minor version form: `4.0.10`. Defined in the public schema (no `spock.` prefix). |
+| spock_version_num | Returns the Spock version in a single numeric form: `40010`. Defined in the public schema (no `spock.` prefix). |
...
-| spock_max_proto_version | The highest Spock native protocol supported by the current binary/build. Defined in the public schema (no `spock.` prefix).
-| spock_min_proto_version | The lowest build for which this Spock binary is backward compatible. Defined in the public schema (no `spock.` prefix).
+| spock_max_proto_version | The highest Spock native protocol supported by the current binary/build. Defined in the public schema (no `spock.` prefix). |
+| spock_min_proto_version | The lowest build for which this Spock binary is backward compatible. Defined in the public schema (no `spock.` prefix). |Also applies to: 44-53
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 23-23: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
[warning] 24-24: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
[warning] 25-25: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/spock_functions/index.md` around lines 23 - 25, The table rows for the
spock function links (e.g., spock.repset_add_seq, spock.repset_add_all_seqs,
spock.repset_remove_seq and the other rows around the 44-53 block) are using
leading-only pipe style; update each Markdown table row to include trailing
pipes so the table uses leading_and_trailing style (add a trailing "|" at the
end of each affected row) to satisfy MD055.
| `spock.repset_remove_table(set_name name, relation regclass, | ||
| include_partitions boolean DEFAULT true)` |
There was a problem hiding this comment.
Align the earlier “Removing a Table” SQL snippet with this updated signature.
This section correctly documents include_partitions, but the earlier quick-start snippet still shows the two-argument form. Please update that earlier snippet so users don’t get conflicting function forms in the same page.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/spock_functions/repset_mgmt.md` around lines 213 - 214, The quick-start
"Removing a Table" SQL snippet uses the old two-argument form; update that
snippet to match the documented signature spock.repset_remove_table(set_name
name, relation regclass, include_partitions boolean DEFAULT true) by adding the
include_partitions boolean argument (true/false as appropriate) to the call so
both snippets show the same three-argument form and reflect the
include_partitions behavior.
Clarify function definition location and add details.
Clarify the schema definition of the spock_min_proto_version function and its read-only nature.
2 participants
No description provided.