Open
Conversation
📝 WalkthroughWalkthroughThis PR marks the Spock 6.0.0 release and introduces migration scripts for upgrading from Spock 5.0.6/5.0.7, alongside a comprehensive test suite validating schema equivalence after upgrade from 5.0.7 to 6.0.0. ChangesSpock 6.0.0 Release & Upgrade Support
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
rasifr
force-pushed
the
task/SPOC-432/prepare-5.1.0-release
branch
from
868f54b to
1cf3438
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/tap/t/018_upgrade_schema_match.pl (1)
150-165: Minor: Temporary file cleanup on early exit.If
psql_querydies between creating$tmpfileand theunlink, the temp file is left behind. Consider using a block-scoped cleanup or wrapping in eval.♻️ Suggested improvement
sub psql_query { my ($port, $sql) = `@_`; my $tmpfile = "/tmp/spock_018_$$.sql"; + my $out; open my $fh, '>', $tmpfile or die "Cannot write $tmpfile: $!"; print $fh $sql; close $fh; - open $fh, '-|', "$PG_BIN/psql", '-X', '-t', '-A', - '-p', $port, '-d', $DB_NAME, '-U', $DB_USER, '-f', $tmpfile - or die "Cannot run psql: $!"; - my $out = join '', <$fh>; - close $fh; - unlink $tmpfile; + eval { + open $fh, '-|', "$PG_BIN/psql", '-X', '-t', '-A', + '-p', $port, '-d', $DB_NAME, '-U', $DB_USER, '-f', $tmpfile + or die "Cannot run psql: $!"; + $out = join '', <$fh>; + close $fh; + }; + my $err = $@; + unlink $tmpfile; + die $err if $err; chomp $out;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/tap/t/018_upgrade_schema_match.pl` around lines 150 - 165, psql_query currently writes a temp file and may leave it behind if the function dies before the final unlink; modify psql_query to guarantee cleanup by using a safe temporary-file helper (e.g., File::Temp->new or a block-scoped temp file) or wrap the write/psql invocation in an eval/finally-like construct (local $@; eval { ... }; my $err = $@; unlink $tmpfile if -e $tmpfile; die $err if $err) so that the temp file is always removed; reference the psql_query function and the $tmpfile variable when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/tap/t/018_upgrade_schema_match.pl`:
- Around line 150-165: psql_query currently writes a temp file and may leave it
behind if the function dies before the final unlink; modify psql_query to
guarantee cleanup by using a safe temporary-file helper (e.g., File::Temp->new
or a block-scoped temp file) or wrap the write/psql invocation in an
eval/finally-like construct (local $@; eval { ... }; my $err = $@; unlink
$tmpfile if -e $tmpfile; die $err if $err) so that the temp file is always
removed; reference the psql_query function and the $tmpfile variable when making
the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87b0828a-fe94-4595-8692-810148ddddff
⛔ Files ignored due to path filters (2)
tests/regress/expected/init_1.outis excluded by!**/*.outtests/regress/expected/init_fail.outis excluded by!**/*.out
📒 Files selected for processing (6)
include/spock.hsql/spock--5.0.6--5.1.0.sqlsql/spock--5.1.0.sqltests/regress/sql/init_fail.sqltests/tap/scheduletests/tap/t/018_upgrade_schema_match.pl
mason-sharp
changed the title
Member
|
We will also need to pull in 5.0.7 |
- Rename sql/spock--6.0.0-devel.sql → spock--6.0.0.sql - Rename sql/spock--5.0.6--6.0.0-devel.sql → spock--5.0.7--6.0.0.sql - Add sql/spock--5.0.6--5.0.7.sql step upgrade script - Bump version in include/spock.h - Update regression test fixtures for 6.0.0 version strings Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ma parity TAP test that installs spock 5.0.7, upgrades to 6.0.0, and compares the resulting schema against a fresh 6.0.0 install — covering tables, columns, functions, views, sequences, view definitions, and function bodies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
rasifr
force-pushed
the
task/SPOC-432/prepare-5.1.0-release
branch
from
1cf3438 to
e49aa36
Compare
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sql/spock--5.0.6--5.0.7.sql`:
- Line 37: Replace the invalid PL/pgSQL assignment-from-query "target_id :=
node_id FROM spock.node_info();" with a proper SELECT ... INTO form; e.g. use
"SELECT node_id INTO target_id FROM spock.node_info();" (or "SELECT INTO STRICT"
if you expect exactly one row) for both occurrences referencing target_id,
node_id and spock.node_info() (the instances around lines 37 and 109).
In `@tests/tap/t/018_upgrade_schema_match.pl`:
- Around line 276-280: The stop_pg helper ignores failures from pg_ctl which can
leave the old postmaster running; modify stop_pg (the function `stop_pg`) to
check the result/exit status of `run_logged("$PG_BIN/pg_ctl", 'stop', ...)` and
fail the test (or die) if pg_ctl returns non-zero, and optionally wait/retry
until the postmaster is confirmed stopped (e.g. poll for PID file removal or use
`pg_ctl status`) before returning so subsequent phase 6 upgrade assertions won't
run against a still-running old postmaster.
- Around line 156-161: The current code reads psql output from a piped
filehandle ($fh) but never checks the exit status of the spawned psql process,
which can return non-zero and produce empty output; after close $fh add a check
of the child's exit status (e.g., inspect $? and die with a clear error
including $tmpfile, the command and exit code) so that when the psql invocation
(the open using "$PG_BIN/psql" with $tmpfile) fails the test fails fast instead
of using empty output for schema matching.
🪄 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: 99ba0d47-b3ac-43ef-b218-8c30016e8fcb
⛔ Files ignored due to path filters (2)
tests/regress/expected/init_1.outis excluded by!**/*.outtests/regress/expected/init_fail.outis excluded by!**/*.out
📒 Files selected for processing (7)
include/spock.hsql/spock--5.0.6--5.0.7.sqlsql/spock--5.0.7--6.0.0.sqlsql/spock--6.0.0.sqltests/regress/sql/init_fail.sqltests/tap/scheduletests/tap/t/018_upgrade_schema_match.pl
✅ Files skipped from review due to trivial changes (2)
- include/spock.h
- tests/tap/schedule
| IF origin_id IS NULL THEN | ||
| RAISE EXCEPTION 'Invalid NULL origin_id'; | ||
| END IF; | ||
| target_id := node_id FROM spock.node_info(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Expect no matches after the fix.
rg -nP ':\=\s*[A-Za-z_][A-Za-z0-9_]*\s+FROM\s+' sql/spock--5.0.6--5.0.7.sqlRepository: pgEdge/spock
Length of output: 173
🏁 Script executed:
cat -n sql/spock--5.0.6--5.0.7.sql | head -50Repository: pgEdge/spock
Length of output: 2380
🏁 Script executed:
cat -n sql/spock--5.0.6--5.0.7.sql | sed -n '25,50p'Repository: pgEdge/spock
Length of output: 1133
🏁 Script executed:
cat -n sql/spock--5.0.6--5.0.7.sql | sed -n '100,120p'Repository: pgEdge/spock
Length of output: 937
Fix invalid PL/pgSQL assignment-from-query syntax at lines 37 and 109.
The := ... FROM ... construct is not valid PL/pgSQL and will fail when the procedures are created during the upgrade.
Proposed fix
- target_id := node_id FROM spock.node_info();
+ SELECT node_id INTO target_id FROM spock.node_info();- origin_id := node_id FROM spock.node WHERE node_name = origin;
+ SELECT node_id INTO origin_id FROM spock.node WHERE node_name = origin;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| target_id := node_id FROM spock.node_info(); | |
| SELECT node_id INTO target_id FROM spock.node_info(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/spock--5.0.6--5.0.7.sql` at line 37, Replace the invalid PL/pgSQL
assignment-from-query "target_id := node_id FROM spock.node_info();" with a
proper SELECT ... INTO form; e.g. use "SELECT node_id INTO target_id FROM
spock.node_info();" (or "SELECT INTO STRICT" if you expect exactly one row) for
both occurrences referencing target_id, node_id and spock.node_info() (the
instances around lines 37 and 109).
Comment on lines
+156
to
+161
| open $fh, '-|', "$PG_BIN/psql", '-X', '-t', '-A', | ||
| '-p', $port, '-d', $DB_NAME, '-U', $DB_USER, '-f', $tmpfile | ||
| or die "Cannot run psql: $!"; | ||
| my $out = join '', <$fh>; | ||
| close $fh; | ||
| unlink $tmpfile; |
There was a problem hiding this comment.
Fail fast when psql_query execution fails.
Line 156–Line 161 do not validate the piped psql exit status. A failed query can collapse to empty output and produce false-positive schema matches.
💡 Proposed fix
my $out = join '', <$fh>;
- close $fh;
+ my $ok = close $fh;
+ my $rc = $? >> 8;
unlink $tmpfile;
+ die "psql query failed (rc=$rc) for $tmpfile" if !$ok || $rc != 0;
chomp $out;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/tap/t/018_upgrade_schema_match.pl` around lines 156 - 161, The current
code reads psql output from a piped filehandle ($fh) but never checks the exit
status of the spawned psql process, which can return non-zero and produce empty
output; after close $fh add a check of the child's exit status (e.g., inspect $?
and die with a clear error including $tmpfile, the command and exit code) so
that when the psql invocation (the open using "$PG_BIN/psql" with $tmpfile)
fails the test fails fast instead of using empty output for schema matching.
Comment on lines
+276
to
+280
| sub stop_pg { | ||
| my ($datadir) = @_; | ||
| run_logged("$PG_BIN/pg_ctl", 'stop', '-D', $datadir, '-m', 'fast', '-w'); | ||
| sleep 2; | ||
| } |
There was a problem hiding this comment.
Check pg_ctl stop result before continuing upgrade flow.
Line 276–Line 280 ignore stop failures; Phase 6 may continue with an old postmaster still running, making the binary-swap/version assertions flaky.
💡 Proposed fix
sub stop_pg {
my ($datadir) = `@_`;
- run_logged("$PG_BIN/pg_ctl", 'stop', '-D', $datadir, '-m', 'fast', '-w');
+ my $rc = run_logged("$PG_BIN/pg_ctl", 'stop', '-D', $datadir, '-m', 'fast', '-w');
+ BAIL_OUT("Failed to stop postgres datadir=$datadir (rc=$rc). See $LOG_FILE") if $rc;
sleep 2;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| sub stop_pg { | |
| my ($datadir) = @_; | |
| run_logged("$PG_BIN/pg_ctl", 'stop', '-D', $datadir, '-m', 'fast', '-w'); | |
| sleep 2; | |
| } | |
| sub stop_pg { | |
| my ($datadir) = `@_`; | |
| my $rc = run_logged("$PG_BIN/pg_ctl", 'stop', '-D', $datadir, '-m', 'fast', '-w'); | |
| BAIL_OUT("Failed to stop postgres datadir=$datadir (rc=$rc). See $LOG_FILE") if $rc; | |
| sleep 2; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/tap/t/018_upgrade_schema_match.pl` around lines 276 - 280, The stop_pg
helper ignores failures from pg_ctl which can leave the old postmaster running;
modify stop_pg (the function `stop_pg`) to check the result/exit status of
`run_logged("$PG_BIN/pg_ctl", 'stop', ...)` and fail the test (or die) if pg_ctl
returns non-zero, and optionally wait/retry until the postmaster is confirmed
stopped (e.g. poll for PID file removal or use `pg_ctl status`) before returning
so subsequent phase 6 upgrade assertions won't run against a still-running old
postmaster.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Bump version to 6.0.0 and add upgrade path from 5.0.7
SPOCK_VERSIONto"6.0.0"andSPOCK_VERSION_NUMto60000ininclude/spock.hspock--6.0.0-devel.sql→spock--6.0.0.sqlandspock--5.0.6--6.0.0-devel.sql→spock--5.0.7--6.0.0.sqlspock--5.0.6--5.0.7.sqlstep upgrade script (pause/resume workers,wait_for_sync_eventrewrite,sync_eventoptionaltransactionalarg,sub_skip_schemacolumn relabel)progress/lag_trackerviews, addssub_id_generatorsequence, migrates conflict type names, and addscleanup_resolutions+ subscription stats functions018_upgrade_schema_matchto verify that a 5.0.7 → 6.0.0 upgrade produces a schema identical to a fresh 6.0.0 install