VReplication: extended e2e test for workflows with tables containing foreign key constraints #14327

rohit-nayak-ps · 2023-10-21T11:06:00Z

Description

This PR adds a more comprehensive end-to-end test for vreplication workflows with tables containing foreign keys. More details in the issue at #13954.

This also fixes a regression in vplayer caused by the new foreign key flag handling while playing binlog events. Previously we were not checking constraints during the copy phase and the new handling, which was intended only for the atomic copy mode, causes failures in the copy phase for schemas with table constraints. Fix at https://github.com/vitessio/vitess/pull/14327/files#diff-5b79231a161c35f368514ad138a743e3c74db9fe1fe0cece290e2377e9250a51L156

The new e2e test has been added as a separate workflow for now to isolate issues and enable rerunning often, and also confirm that the test is not flaky before making it Required. Also, this test will be enhanced with additional load generators: like the one currently used in the fk stress test in vtgate, requiring this test to possibly be run on the larger runners.

The PR includes test framework changes that were needed since we now have a load generator that runs concurrently and needs to handle expected transient errors during traffic switching/vtgate buffering. For some reason VDIff Show also gives temporary connection errors, possibly related to buffering, which had to be ignored to prevent false positives in reported errors.

Related Issue(s)

Fixes #13954

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Deployment Notes

vitess-bot · 2023-10-21T11:06:04Z

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

Ensure that the Pull Request has a descriptive title.
Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

Apply the release notes (needs details) label if users need to know about this change.
New features should be documented.
There should be some code comments as to why things are implemented the way they are.
There should be a comment at the top of each new or modified test to explain what the test does.

New flags

Is this flag really necessary?
Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

Each item in Jobs should be named in order to mark it as required.
If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

Protobuf changes should be wire-compatible.
Changes to _vt tables and RPCs need to be backward compatible.
RPC changes should be compatible with vitess-operator
If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
vtctl command output order should be stable and awk-able.

…or Reshard VDiffs Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com> Working test upto reverse traffic in import, with load stopped before switch traffic Signed-off-by: Rohit Nayak <rohit@planetscale.com> Working import with vtctl, vtctld still failing. VDiff concurrency bug fixes Signed-off-by: Rohit Nayak <rohit@planetscale.com> Add new FKExt workflow to CI Signed-off-by: Rohit Nayak <rohit@planetscale.com> Moving tables from one keyspace to another after importing works, but not if replica constraints are dropped Signed-off-by: Rohit Nayak <rohit@planetscale.com> Working materialize. Still dropping replica constraints doesn't work Signed-off-by: Rohit Nayak <rohit@planetscale.com> Some cleanup Signed-off-by: Rohit Nayak <rohit@planetscale.com> Drop replica constraints correctly. This fixes previously failing reverse workflows during the move to keyspaces. Materialize also works Signed-off-by: Rohit Nayak <rohit@planetscale.com> Fix failing VDiff2 test Signed-off-by: Rohit Nayak <rohit@planetscale.com> Fix shard name for the vreplication foreign key stress test Signed-off-by: Rohit Nayak <rohit@planetscale.com> Fix issue with vtctld switch writes. All current workflows work Signed-off-by: Rohit Nayak <rohit@planetscale.com> WIP Signed-off-by: Rohit Nayak <rohit@planetscale.com> Reshard working for split/merge and merge. We think, because vdiff is giving some strange errors Signed-off-by: Rohit Nayak <rohit@planetscale.com> Better retry and error handling. However vdiffs seem to be an issue Signed-off-by: Rohit Nayak <rohit@planetscale.com> Use vdiff v1 which seems to work fine during reshard Signed-off-by: Rohit Nayak <rohit@planetscale.com> Revert bug fixes that have been fixed in separate PRs Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

…failing for some reason. Additional logging to debug vdiff issue Signed-off-by: Rohit Nayak <rohit@planetscale.com>

…and reverse workflows Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

…ys, since deferForeignKeys cannot be used for schemas with foreign keys Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

mattlord

Thank you for working on this! This is great. In addition to all the bugs that it uncovered during the work, it will help to catch new things going forward. ❤️

Signed-off-by: Matt Lord <mattalord@gmail.com>

…rectly Signed-off-by: Rohit Nayak <rohit@planetscale.com>

shlomi-noach

This looks good! I have a couple questions where I don't understand the logic. Specifically one in fk_ext_test.go and one in vplayer.go.

shlomi-noach · 2023-11-22T15:07:37Z

go/vt/vttablet/tabletmanager/vreplication/vplayer.go

+ if vp.vr.WorkflowSubType == int32(binlogdatapb.VReplicationWorkflowSubType_AtomicCopy) {
+ // If this is an atomic copy, we must update the foreign_key_checks state even when the vplayer runs during
+ // the copy phase, i.e., for catchup and fastforward.
+ mustUpdate = true


I'm just curious, and want to understand the logic better. Why do we need to always update FK checks in atomic-copy?

In atomic-copy workflows we run with foreign_key_checks=on by default, but not for regular workflows.

During catchup/fastforward, we may get a transaction that sets foreign_key_checks=off. So we need to set the state, otherwise the transactions will fail. If not running in atomic_copy the checks are off always.

shlomi-noach · 2023-11-22T15:14:41Z

go/test/endtoend/vreplication/helper_test.go

@@ -65,8 +71,31 @@ func execMultipleQueries(t *testing.T, conn *mysql.Conn, database string, lines
 execVtgateQuery(t, conn, database, string(query))
 }
 }
+
+func execQueryWithRetry(t *testing.T, conn *mysql.Conn, query string, timeout time.Duration) *sqltypes.Result {
+ timer := time.NewTimer(timeout)


Not very important, ignore if you like; I prefer this model:

ctx, cancel := context.WithTimeout(ctx.Background(), timeout) defer cancel() ticker := time.NewTicker(defaultTick) defer ticker.Stop() for { ... select { case <- ctx.Done(): ... case <-ticker.C: ... } ...

This way the timeout overrides the tick's sleep.

Anyway, just a styling comment.

Modified to use the suggested pattern.

shlomi-noach · 2023-11-22T15:18:39Z

go/test/endtoend/vreplication/helper_test.go

+func getRowCount(t *testing.T, vtgateConn *mysql.Conn, table string) (int, error) {
+ query := fmt.Sprintf("select count(*) from %s", table)
+ qr := execVtgateQuery(t, vtgateConn, "", query)
+ if qr == nil {


execVtgateQuery calls execQuery which runs require.NoError(t, err)
In which case qr cannot be nil, and this function does not need to return an error type, just as execQuery does not return an error type.

Good catch, fixed.

go/test/endtoend/vreplication/helper_test.go

shlomi-noach · 2023-11-22T15:47:36Z

go/test/endtoend/vreplication/helper_test.go

+ for {
+ select {
+ case <-ticker.C:
+ if condition() {


In this implementation you necessarily first have to wait once for tickInterval before checking condition(). Is this intentional? Alternatively, move this block to run before the select.

We already check the condition at the start of the function. So the next time should be after a tick.

shlomi-noach · 2023-11-22T15:49:52Z

go/test/endtoend/vreplication/schema/fkext/source_schema.sql

@@ -0,0 +1,2 @@
+create table if not exists parent(id int, name varchar(128), primary key(id)) engine=innodb;
+create table if not exists child(id int, parent_id int, name varchar(128), primary key(id), foreign key(parent_id) references parent(id) on delete cascade) engine=innodb;


What's the deal with if not exists especially compared with materialize_schema.sql which does not contain this clause?

We use these queries with ApplySchema while resharding for the entire keyspace, expecting it to only run on the new shards and be noops on existing ones. The materialize schema doesn't have this requirement.

shlomi-noach · 2023-11-26T12:07:15Z

go/test/endtoend/vreplication/fk_ext_test.go

+
+ t.Run("MoveTables from unsharded to sharded keyspace", func(t *testing.T) {
+ // Migrate data from target1Keyspace to the sharded target2Keyspace. Streams only from primaries
+ // for one shard and replica for the other shard. Constraints have been dropped from this replica.


I'm not sure what "Streams only from primaries for one shard and replica for the other shard" mean here, as we're importing from an unsharded keyspace?

Comment was incorrect/ambiguous. They were in the context of the following materialize workflow which is from the sharded target2 to the unsharded target1. I updated the comments to reflect that.

shlomi-noach · 2023-11-26T12:45:14Z

go/test/endtoend/vreplication/fk_ext_test.go

+// the binlogs created by the primary. This will confirm that vtgate is doing the cascades correctly.
+func dropReplicaConstraints(t *testing.T, keyspaceName string, tablet *cluster.VttabletProcess) {
+ var dropConstraints []string
+


Let's require.Equal(...) that the tablet type is REPLICA (or non-primary).

shlomi-noach · 2023-11-26T12:46:23Z

go/test/endtoend/vreplication/fk_ext_test.go

+const getConstraintsQuery = `
+SELECT CONSTRAINT_NAME, TABLE_NAME 
+FROM INFORMATION_SCHEMA.KEY_COLUMN_USAGE 
+WHERE TABLE_SCHEMA = ` + "'%s'" + ` AND REFERENCED_TABLE_NAME IS NOT NULL;


Not sure I see the need to break the string.

Suggested change

WHERE TABLE_SCHEMA = ` + "'%s'" + ` AND REFERENCED_TABLE_NAME IS NOT NULL;

WHERE TABLE_SCHEMA = `'%s'` AND REFERENCED_TABLE_NAME IS NOT NULL;

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

…foreign key constraints (vitessio#14327) Signed-off-by: Rohit Nayak <rohit@planetscale.com> Signed-off-by: Matt Lord <mattalord@gmail.com> Co-authored-by: Matt Lord <mattalord@gmail.com>

rohit-nayak-ps added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: VReplication Type: Testing labels Oct 21, 2023

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Oct 21, 2023

github-actions bot added this to the v19.0.0 milestone Oct 21, 2023

rohit-nayak-ps removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request labels Oct 21, 2023

rohit-nayak-ps mentioned this pull request Oct 23, 2023

VDiff: wait for shard streams of one table diff to complete for before starting that of the next table #14345

Merged

4 tasks

rohit-nayak-ps force-pushed the foreign-key-e2e branch 2 times, most recently from 13cf85f to 5e0ee68 Compare October 27, 2023 13:17

rohit-nayak-ps mentioned this pull request Oct 31, 2023

VDiffs on reverse Reshard workflows never start #14400

Closed

rohit-nayak-ps force-pushed the foreign-key-e2e branch 2 times, most recently from 7a43ef8 to a7eae29 Compare October 31, 2023 19:50

This was referenced Oct 31, 2023

VDiff tablet selection: pick non-serving tablets in Reshard workflows #14413

Merged

Vtctld SwitchReads: fix bug where writes were also being switched as part of switching reads when all traffic was switched using SwitchTraffic #14360

Merged

rohit-nayak-ps force-pushed the foreign-key-e2e branch from 89b98f5 to f338ef2 Compare November 1, 2023 21:09

rohit-nayak-ps added 10 commits November 3, 2023 14:02

Add option to tablet picker to chose non-serving tablets and use it f…

df9bbc2

…or Reshard VDiffs Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Set workflow type correctly!

1a04822

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Cleanup

5cd9bc7

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Added back vdiff after SwitchTraffic

c3596ac

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Improve error handling edge cases. Cleanup

b431942

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Fix VDiff2 test

9b265d5

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Compare counts on source and target instead of vdiff, which is still …

f03366b

…failing for some reason. Additional logging to debug vdiff issue Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Disabled throttling temporarily and now vdiffs work for both forward …

e0990bc

…and reverse workflows Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Remove rowstreamer hack which ignored throttling

026f8f9

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps added 2 commits November 3, 2023 14:02

Trigger rebuild

4886266

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Refactor tstWorkflowExec to use options for atomicCopy/deferForeignKe…

15f768b

…ys, since deferForeignKeys cannot be used for schemas with foreign keys Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps force-pushed the foreign-key-e2e branch from f338ef2 to 15f768b Compare November 3, 2023 14:29

rohit-nayak-ps added 4 commits November 3, 2023 16:37

Refactor from self-review

9102128

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Refactor load generator

a288811

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

More refactor

f3a8b76

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Improve comments of the new test

01beaed

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps marked this pull request as ready for review November 5, 2023 15:17

rohit-nayak-ps requested review from GuptaManan100, frouioui, deepthi, mattlord, harshit-gangal and shlomi-noach as code owners November 5, 2023 15:17

mattlord approved these changes Nov 17, 2023

View reviewed changes

mattlord and others added 4 commits November 17, 2023 13:54

Address my own minor nits

1ca6268

Signed-off-by: Matt Lord <mattalord@gmail.com>

Merge remote-tracking branch 'origin/main' into foreign-key-e2e

58a0709

Signed-off-by: Matt Lord <mattalord@gmail.com>

Address mistake during merge

3403aa5

Signed-off-by: Matt Lord <mattalord@gmail.com>

Fix regression where the test cluster was not setting the primary cor…

5f3aed2

…rectly Signed-off-by: Rohit Nayak <rohit@planetscale.com>

shlomi-noach approved these changes Nov 26, 2023

View reviewed changes

rohit-nayak-ps added 2 commits November 26, 2023 22:29

Address some review comments

89ec701

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

Address more review comments

d525b8e

Signed-off-by: Rohit Nayak <rohit@planetscale.com>

rohit-nayak-ps merged commit 1843826 into vitessio:main Nov 27, 2023
116 checks passed

rohit-nayak-ps deleted the foreign-key-e2e branch November 27, 2023 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VReplication: extended e2e test for workflows with tables containing foreign key constraints #14327

VReplication: extended e2e test for workflows with tables containing foreign key constraints #14327

rohit-nayak-ps commented Oct 21, 2023 •

edited by mattlord

Loading

vitess-bot bot commented Oct 21, 2023

mattlord left a comment

shlomi-noach left a comment

shlomi-noach Nov 22, 2023

rohit-nayak-ps Nov 27, 2023

shlomi-noach Nov 22, 2023

rohit-nayak-ps Nov 27, 2023

shlomi-noach Nov 22, 2023

rohit-nayak-ps Nov 27, 2023

shlomi-noach Nov 22, 2023

rohit-nayak-ps Nov 27, 2023

shlomi-noach Nov 22, 2023

rohit-nayak-ps Nov 27, 2023

shlomi-noach Nov 26, 2023

rohit-nayak-ps Nov 27, 2023

shlomi-noach Nov 26, 2023

rohit-nayak-ps Nov 27, 2023

shlomi-noach Nov 26, 2023

rohit-nayak-ps Nov 27, 2023

		@@ -0,0 +1,2 @@
		create table if not exists parent(id int, name varchar(128), primary key(id)) engine=innodb;
		create table if not exists child(id int, parent_id int, name varchar(128), primary key(id), foreign key(parent_id) references parent(id) on delete cascade) engine=innodb;

	WHERE TABLE_SCHEMA = ` + "'%s'" + ` AND REFERENCED_TABLE_NAME IS NOT NULL;
	WHERE TABLE_SCHEMA = `'%s'` AND REFERENCED_TABLE_NAME IS NOT NULL;

VReplication: extended e2e test for workflows with tables containing foreign key constraints #14327

VReplication: extended e2e test for workflows with tables containing foreign key constraints #14327

Conversation

rohit-nayak-ps commented Oct 21, 2023 • edited by mattlord Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Oct 21, 2023

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

mattlord left a comment

Choose a reason for hiding this comment

shlomi-noach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohit-nayak-ps commented Oct 21, 2023 •

edited by mattlord

Loading