SPOC-513: Restore a usable upgrade chain from 5.x to 6.0.0-devel on main#425
SPOC-513: Restore a usable upgrade chain from 5.x to 6.0.0-devel on main#425
main#425Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | -2 |
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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a full Spock extension install script (v5.0.0) defining catalog tables, views, aggregates, and many SQL/C/PLpgSQL API functions; introduces two upgrade scripts (5.0.6→5.0.7 and 5.0.7→6.0.0-devel) that add/remove pause/resume functions and relabel a catalog column; removes internal apply helpers from ChangesSpock Extension Installation (v5.0.0)
Spock Extension Upgrade (5.0.6 → 5.0.7)
Spock Extension Upgrade (5.0.7 → 6.0.0-devel)
Apply heap implementation cleanup
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sql/spock--5.0.7--6.0.0-devel.sql (1)
6-19: Catalog relabel is correct; consider adding a defensive predicate for idempotency.The approach is sound:
textandtext[]share identical physical storage attributes (typlen=-1,typbyval=false,typalign='i',typstorage='x'), soattlen/attbyval/attalign/attstorageneed not change. Existing on-disk values were already written viastrlist_to_textarray(src/spock_node.c:903-907) and read viaDatumGetArrayTypeP(src/spock_node.c:1129-1137), so simply relabelingatttypidand settingattndims = 1fixes SQL-level access (SELECT, unnest, array ops) without tuple rewrites, and the UPDATE triggers normal relcache invalidation on commit.One hardening suggestion: scope the UPDATE to the exact prior state so the statement is a strict no-op if re-run or if a site has already been patched:
Proposed defensive predicate
UPDATE pg_catalog.pg_attribute SET atttypid = 'text[]'::regtype, attndims = 1 WHERE attrelid = 'spock.subscription'::regclass - AND attname = 'sub_skip_schema'; + AND attname = 'sub_skip_schema' + AND atttypid = 'text'::regtype;Direct DML on
pg_catalog.pg_attributerequires superuser privileges, whichALTER EXTENSION … UPDATEprovides, so no GUC adjustment is needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/spock--5.0.7--6.0.0-devel.sql` around lines 6 - 19, The UPDATE should be made idempotent by restricting it to rows that still have the old catalog state; modify the UPDATE on pg_catalog.pg_attribute (targeting spock.subscription, attname = 'sub_skip_schema') to only set atttypid = 'text[]'::regtype and attndims = 1 when the current atttypid equals 'text'::regtype (and/or attndims = 0), so the statement becomes a no-op if the column is already relabeled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sql/spock--5.0.7--6.0.0-devel.sql`:
- Around line 6-19: The UPDATE should be made idempotent by restricting it to
rows that still have the old catalog state; modify the UPDATE on
pg_catalog.pg_attribute (targeting spock.subscription, attname =
'sub_skip_schema') to only set atttypid = 'text[]'::regtype and attndims = 1
when the current atttypid equals 'text'::regtype (and/or attndims = 0), so the
statement becomes a no-op if the column is already relabeled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0de3266-3351-4286-9c28-96102bad964d
📒 Files selected for processing (3)
sql/spock--5.0.0.sqlsql/spock--5.0.6--5.0.7.sqlsql/spock--5.0.7--6.0.0-devel.sql
danolivo
force-pushed
the
spoc-upgrade-507-devel
branch
from
c6bdcfc to
3b01731
Compare
On main there was no base install file and no upgrade path from 5.x, so CREATE EXTENSION could only land on 6.0.0-devel via its single full install script. Disabling that script exposed the gap — ALTER EXTENSION had nothing to walk. Restore the full chain: sql/spock--5.0.0.sql -- base install (from v5_STABLE) sql/spock--5.0.6--5.0.7.sql -- from v5_STABLE sql/spock--5.0.7--6.0.0-devel.sql The new 5.0.7 -> 6.0.0-devel step is derived from the old 5.0.6 -> 6.0.0-devel script minus the pause/resume_apply_workers function definitions, which already exist in 5.0.7 with identical signatures and C symbols. It also fixes a pre-existing catalogue-type mismatch on spock.subscription.sub_skip_schema. The column was added as text in the 5.0.1 -> 5.0.2 upgrade, but the C code on both read and write has always treated it as text[] (strlist_to_textarray on write, DatumGetArrayTypeP on read). The feature worked because the bytes on disk were a valid ArrayType all along; only SQL-level queries against the column saw garbage (\x01...). Relabel the column in place via pg_attribute so SELECT / unnest / array operators work as expected, without rewriting any tuples.
danolivo
force-pushed
the
spoc-upgrade-507-devel
branch
from
3b01731 to
275615e
Compare
1 participant
Motivation
On
mainthere was no base install script and no upgrade path from any 5.x release — the only SQL install wasspock--6.0.0-devel.sql. Disabling that script (e.g. to simulate an ALTER EXTENSION from a 5.x node) produced:ERROR: extension "spock" has no installation script nor update path for version "6.0.0-devel"
Meanwhile
v5_STABLEalready shippedspock--5.0.0.sqlas a base and a5.0.6 -> 5.0.7upgrade.mainneeds to meet that chain so a site running 5.0.7 on v5_STABLE has a supported path forward.Changes
sql/spock--5.0.0.sql— base install taken fromv5_STABLE.sql/spock--5.0.6--5.0.7.sql— taken fromv5_STABLE.sql/spock--5.0.7--6.0.0-devel.sql— new. Derived from the previous5.0.6--6.0.0-devel.sqlminus thepause_apply_workers/resume_apply_workersCREATEs, which already exist in 5.0.7 with identical signatures and C symbols.sql/spock--5.0.6--6.0.0-devel.sql— superseded by the two-step5.0.6 -> 5.0.7 -> 6.0.0-develpath.Post-change chain:
5.0.0 -> 5.0.1 -> ... -> 5.0.6 -> 5.0.7 -> 6.0.0-devel
Fix for
sub_skip_schemacatalogue-type mismatchThe column was added as
textinsql/spock--5.0.1--5.0.2.sql, but the C code has always treated it astext[]on both paths:PointerGetDatum(strlist_to_textarray(sub->skip_schema))(src/spock_node.c, two sites)DatumGetArrayTypeP(d)+textarray_to_list(...)(src/spock_node.c)Because
heap_form_tuple/heap_getattrdon't validate Datum-vs-column type and bothtextandArrayTypeare varlena, the mismatch ran silently: the bytes stored in thetextcolumn were, in fact, a validArrayType. The feature worked end-to-end; only SQL-level queries against the column saw garbage (\x01...), which the regression suite exposed viaalter_options.sql/spock--5.0.7--6.0.0-devel.sqlnow relabels the column in place: