internal/types/acceptor: added an option configuration to fold a multi-batch by key #1115

ryanluu12345 · 2025-01-17T21:44:41Z

Previously, there was a bug resulting from performing a combination of inserts and deletes could lead to data inconsistency because of how deletes and inserts are handled in the AcceptMultiBatch to handle the possibility of FK updates/deletes. As a result, we do a FoldByKey for each temporal batch in the multi batch before we flush to the target, so that only the last mutation for a single table and single PK remains. This makes it so that it is safe to perform the deletes first and then the updates after.

This is also extensible in the future since we can control the behavior for this folding via a configuration flag, instead of having to plumb in a new acceptor each time; this allows us to turn on and off this ability to fold.

Resolves: CC-30958
Release Note: None

TODO

Add new tests for the previously wrong behavior
Remove hack for FoldByKey (but right now may require refactor)

This change is

…i-batch by key Previously, there was a bug resulting from performing a combination of inserts and deletes could lead to data inconsistency because of how deletes and inserts are handled in the AcceptMultiBatch to handle the possibility of FK updates/deletes. As a result, we do a FoldByKey for each temporal batch in the multi batch before we flush to the target, so that only the last mutation for a single table and single PK remains. This makes it so that it is safe to perform the deletes first and then the updates after. This is also extensible in the future since we can control the behavior for this folding via a configuration flag, instead of having to plumb in a new acceptor each time; this allows us to turn on and off this ability to fold. Resolves: CC-30958 Release Note: None

ryanluu12345 · 2025-01-17T21:52:54Z

internal/types/acceptors.go

+// Need to refactor this into a new place so that this and msort both
+// import from `types`.
+// This is a workaround.
+func FoldByKey(x []Mutation) ([]Mutation, error) {


@sravotto @ZhouXing19 any thoughts on how to best resolve this issue? I was thinking of moving the acceptors into their own package since keeping it in types seems too coupled and also leads to import cycles like this.

Any alternatives?

…ly and acceptor code

ryanluu12345 · 2025-01-19T04:55:43Z

internal/types/utils.go

+	"github.com/pkg/errors"
+)
+
+// FoldByKey now lives in this package since it's crucial to the


Before considering this approach I did a local prototype of moving acceptor to its own package so we avoid the import cycle. However, this ends up leading to a mass refactor across the codebase that would impact in-flight PRs. After doing more investigation, I also realized that the end result is still an import cycle because types is required by acceptor and vice versa.

Ended up just moving all references to point to types.FoldByKey and this is a much simpler and safer change.

Implemented this, but the open question here is should we keep this off or on by default. If we keep this off, then we have the same behavior as before, but is exposed to this correctness issue. If we keep this on, then we may have a perf impact because we do the collapsing every time we accept a batch, which in PG and MySQL right now would do it every time commit happens.

ryanluu12345 · 2025-01-19T04:58:04Z

internal/source/pglogical/conn.go

@@ -157,6 +157,7 @@ func (c *Conn) accumulateBatch(

 			if err := c.acceptor.AcceptTemporalBatch(ctx, batch, &types.AcceptOptions{
 				TargetQuerier: tx,
+				FoldKeys:      true,


A TODO that we must consider when we pick this back up next week is to ensure that this FoldKeys behavior is gated by a flag (I recommend --collapseUpdates or --useBatchMode) so that we can turn on or off the behavior. Here are the tradeoffs:

If FoldKeys is set to on: then we do extra O(N) processing to do the collapsing and reassign the proper batches

If FoldKeys is set to off: then we may run the risk of a correctness issue for the same PK being updated multiple times (since we have the bug in the FK logic)

What we have learned from previous customers is that it's helpful to have this escape hatch so they can turn off undesirable behavior as needed.

ZhouXing19 · 2025-01-20T16:51:22Z

The whole logic LGTM, just thinking that tests are needed.
I'm thinking why we should leave the foldbykey as an option rather than always executing it, as I thought we should always prioritize correctness over performance.

ryanluu12345 · 2025-01-20T16:59:34Z

The whole logic LGTM, just thinking that tests are needed. I'm thinking why we should leave the foldbykey as an option rather than always executing it, as I thought we should always prioritize correctness over performance.

I think mainly because this fixes a correctness issue in the case that you could have an update/insert/update in the same batch. However, not all customer's workloads are like that. In certain customer cases, this may regress their performance at scale because the extra O(N) operations. And also because we do expensive JSON decoding to do this folding. So, wanted to give an escape hatch here if they understand their workload.

How open are you to just making this flag true by default so it's correct behavior. But if a customer knows for a fact their workload won't hit this, they can turn it off with the flag?

ZhouXing19 · 2025-01-20T17:01:47Z

I lean toward true as default but might want more PM involvement in the discussion too

sravotto

I would consider moving msort.go into the types package. I think UniqueByTimeKey is only referenced once.
Other than that, looks good to me.

Reviewable status: 0 of 14 files reviewed, 4 unresolved discussions (waiting on @Jeremyyang920, @noelcrl, @ryanluu12345, and @ZhouXing19)

internal/types/utils.go line 28 at r4 (raw file):

)

// FoldByKey now lives in this package since it's crucial to the

Would it make sense just to move the whole msort.go (and test) under types?

ryanluu12345

I'm also in agreement that true should be the default. Will loop in PM review here.

@sravotto , good callout, I'll go ahead and move msort and relevant tests to types and update all references.

Reviewable status: 0 of 17 files reviewed, 4 unresolved discussions (waiting on @Jeremyyang920, @noelcrl, and @ZhouXing19)

ryanluu12345

One more note here: I'll work on adding the INSERT -> DELETE use case some time this week and will bottom out on the default behavior.

Reviewable status: 0 of 17 files reviewed, 4 unresolved discussions (waiting on @Jeremyyang920, @noelcrl, and @ZhouXing19)

ryanluu12345 · 2025-01-21T19:00:54Z

One more note here: I'll work on adding the INSERT -> DELETE use case some time this week and will bottom out on the default behavior.

Reviewable status: 0 of 17 files reviewed, 4 unresolved discussions (waiting on @Jeremyyang920, @noelcrl, and @ZhouXing19)

@ZhouXing19 @sravotto so I'm looking into our testing right now and see that we have internal/source/pglogical/integration_test.go which has a TestPGLogical which has a helper that does the setup for schema, create table, and the data movement.

In MySQL, there is internal/source/mylogical/integration_test.go and in CRDB webhook, there is internal/source/cdc/server/integration_test.go TestIntegration.

I plan to add a new set of configurations that helps to reproduce the INSERT -> DELETE -> INSERT logic here and then add a new test case. Thoughts?

sravotto

Makes sense to me.

Reviewed 1 of 5 files at r1, 4 of 10 files at r6.
Reviewable status: 4 of 18 files reviewed, 3 unresolved discussions (waiting on @Jeremyyang920, @noelcrl, and @ZhouXing19)

ryanluu12345 · 2025-01-21T22:03:19Z

@sravotto @ZhouXing19 when you get a chance, wondering if you can give a review on the latest iteration? Just added in the testing and have a path forward for the flag or no flag.

@rohan-joshi and I chatted and he requested that I benchmark to understand better how much timing difference doing this collapseMutation takes vs. not doing it. If the impact is trivial, then the vote goes towards not adding the flag. I'll do this sometime early next week.

ryanluu12345 requested review from sravotto, Jeremyyang920, ZhouXing19 and noelcrl January 17, 2025 21:44

ryanluu12345 commented Jan 17, 2025

View reviewed changes

ryanluu12345 mentioned this pull request Jan 17, 2025

[WIP] acceptors: added in a concept of deferred deletes for multi batches #1112

Closed

ryanluu12345 added 2 commits January 17, 2025 14:25

fixed comment

66e5c9f

refactored FoldByKey into types so that it can be used across the app…

8d88a7d

…ly and acceptor code

ryanluu12345 commented Jan 19, 2025

View reviewed changes

ryanluu12345 force-pushed the user/rluu/fold-acceptor branch 2 times, most recently from 2c8a8b9 to f9ba3a4 Compare January 19, 2025 05:16

sravotto requested changes Jan 21, 2025

View reviewed changes

added in the config flags for doing this collapseMutations mode

85d3768

ryanluu12345 force-pushed the user/rluu/fold-acceptor branch from f9ba3a4 to 85d3768 Compare January 21, 2025 18:05

updated location of msort

9fe8ee7

ryanluu12345 commented Jan 21, 2025

View reviewed changes

ryanluu12345 requested a review from sravotto January 21, 2025 18:12

added test case for TestOrderedAcceptor

cd99df7

sravotto reviewed Jan 21, 2025

View reviewed changes

ryanluu12345 requested a review from sravotto January 21, 2025 21:59

added test coverage for integration

5f756e2

ryanluu12345 force-pushed the user/rluu/fold-acceptor branch from 3e6743d to 5f756e2 Compare January 21, 2025 22:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/types/acceptor: added an option configuration to fold a multi-batch by key #1115

internal/types/acceptor: added an option configuration to fold a multi-batch by key #1115

ryanluu12345 commented Jan 17, 2025 •

edited

Loading

ryanluu12345 Jan 17, 2025

ryanluu12345 Jan 19, 2025

ryanluu12345 Jan 19, 2025

ryanluu12345 Jan 19, 2025

ZhouXing19 commented Jan 20, 2025

ryanluu12345 commented Jan 20, 2025

ZhouXing19 commented Jan 20, 2025

sravotto left a comment

ryanluu12345 left a comment

ryanluu12345 left a comment

ryanluu12345 commented Jan 21, 2025 •

edited

Loading

sravotto left a comment

ryanluu12345 commented Jan 21, 2025

internal/types/acceptor: added an option configuration to fold a multi-batch by key #1115

Are you sure you want to change the base?

internal/types/acceptor: added an option configuration to fold a multi-batch by key #1115

Conversation

ryanluu12345 commented Jan 17, 2025 • edited Loading

ryanluu12345 Jan 17, 2025

Choose a reason for hiding this comment

ryanluu12345 Jan 19, 2025

Choose a reason for hiding this comment

ryanluu12345 Jan 19, 2025

Choose a reason for hiding this comment

ryanluu12345 Jan 19, 2025

Choose a reason for hiding this comment

ZhouXing19 commented Jan 20, 2025

ryanluu12345 commented Jan 20, 2025

ZhouXing19 commented Jan 20, 2025

sravotto left a comment

Choose a reason for hiding this comment

ryanluu12345 left a comment

Choose a reason for hiding this comment

ryanluu12345 left a comment

Choose a reason for hiding this comment

ryanluu12345 commented Jan 21, 2025 • edited Loading

sravotto left a comment

Choose a reason for hiding this comment

ryanluu12345 commented Jan 21, 2025

ryanluu12345 commented Jan 17, 2025 •

edited

Loading

ryanluu12345 commented Jan 21, 2025 •

edited

Loading