chore: prevent crash on WAL UPDATE without old tuple#1700
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds defensive nil checks to PostgreSQL replication changeset decoding. ChangesReplication Changeset Nil-Tuple Handling
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested Reviewers
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 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. Review rate limit: 0/1 reviews remaining, refill in 43 minutes and 46 seconds.Comment |
Time Submission Status
You can submit time with the command. Example: See available commands to help comply with our Guidelines. |
|
@holdex pr submit-time 4h |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
node/pg/repl_changeset_test.go (1)
15-82: 💤 Low valueNew test harness and regression tests look correct and well-structured.
The
newTestChangesetWriterhelper is appropriate — the comment explaining thatSerializeChangesetis never invoked forNullValuecolumns is accurate and important. The regression testTestDecodeUpdate_NilOldTuplecleanly covers the crash scenario.One additional assertion worth adding to
TestDecodeUpdate_NilOldTupleisce.Kind()to lock in the current (Insert) classification and make the behavioral trade-off visible and intentional:assert.Nil(t, ce.OldTuple, "OldTuple must remain unset when WAL omits it") require.Len(t, ce.NewTuple, 1) assert.Equal(t, NullValue, ce.NewTuple[0].ValueType) +// Kind() returns Insert (not Update) because OldTuple is nil/empty. +// This means ApplyChangesetEntry will call applyInserts (INSERT ON CONFLICT DO NOTHING), +// effectively discarding the update if the row already exists. +assert.Equal(t, CSEntryKindInsert, ce.Kind(), "nil-OldTuple UPDATE is currently classified as Insert")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@node/pg/repl_changeset_test.go` around lines 15 - 82, Add an assertion in TestDecodeUpdate_NilOldTuple that verifies the changeset entry kind is the expected Insert classification: after retrieving ce := (<-csChan).(*ChangesetEntry) assert ce.Kind() == Insert (or the enum/value used for insert in ChangesetEntry.Kind) to lock in the current behavior; reference the ChangesetEntry type and its Kind() method so the test explicitly documents the Insert classification choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@node/pg/repl_changeset.go`:
- Around line 551-560: The emitted ChangesetEntry can have both OldTuple and
NewTuple nil causing DecodeTuples to return nil and applyDeletes to build a
"DELETE ... WHERE " SQL with no predicates; update applyDeletes to defensively
check the decoded record from ChangesetEntry.DecodeTuples(rel) (or check
ce.OldTuple/ce.NewTuple) and if the record is nil/empty return a clear error (or
skip the entry) instead of constructing a WHERE-less DELETE; reference the
applyDeletes function and ChangesetEntry/DecodeTuples/Kind so you add a guard
early that prevents building the malformed SQL and surfaces a descriptive error
to the caller.
- Around line 507-534: The code treats a nil update.OldTuple as an insert
(Kind() sees len(ce.OldTuple)==0) which causes WAL UPDATEs with no old tuple to
be silently handled as inserts; to fix, detect the update.OldTuple == nil case
right after the existing if update.OldTuple != nil block (where convertPgxTuple
is called) and emit a clear warning log (using the same logger/context used in
this file) stating that an UPDATE with nil OldTuple will be treated as an insert
and may be dropped by applyInserts; leave ce.OldTuple nil and continue
populating ce.NewTuple as before so behavior is unchanged but observable
(alternatively, if the intended policy is to drop the event, return early
instead of emitting—choose one and implement consistently), and reference
Kind(), ApplyChangesetEntry, and applyInserts in the log/context for operator
clarity.
---
Nitpick comments:
In `@node/pg/repl_changeset_test.go`:
- Around line 15-82: Add an assertion in TestDecodeUpdate_NilOldTuple that
verifies the changeset entry kind is the expected Insert classification: after
retrieving ce := (<-csChan).(*ChangesetEntry) assert ce.Kind() == Insert (or the
enum/value used for insert in ChangesetEntry.Kind) to lock in the current
behavior; reference the ChangesetEntry type and its Kind() method so the test
explicitly documents the Insert classification choice.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 74f9dd67-66ac-4537-a043-f92acbbd29ec
📒 Files selected for processing (2)
node/pg/repl_changeset.gonode/pg/repl_changeset_test.go
1 participant
resolves: https://github.com/truflation/website/issues/3757
already live on mainnet
Summary by CodeRabbit
Bug Fixes
Tests