Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Branch deletes (and maybe other things) don't replicate #3815

Closed
timsehn opened this issue Jul 12, 2022 · 6 comments
Closed

Branch deletes (and maybe other things) don't replicate #3815

timsehn opened this issue Jul 12, 2022 · 6 comments

Comments

@timsehn
Copy link
Sponsor Contributor

timsehn commented Jul 12, 2022

Repro.

  1. Set up a master
replication_example $ dolt init
Successfully initialized dolt data repository.
replication_example $ dolt remote add origin timsehn/replication_example
replication_example $ dolt config --add --local sqlserver.global.dolt_replicate_to_remote origin
Config successfully updated.
replication_example $ dolt sql -q "create table test (pk int, c1 int, primary key(pk))"
replication_example $  dolt sql -q "call dolt_commit('-am', 'trigger replication')"
+----------------------------------+
| hash                             |
+----------------------------------+
| oer1pgkhtj0ni23tlesp753oucuvog4u |
+----------------------------------+
  1. Create timsehn/replication_example on DoltHub: https://www.dolthub.com/repositories/timsehn/replication_example

  2. Set up a replica

dolt $ dolt clone timsehn/replication_example read_replica
cloning https://doltremoteapi.dolthub.com/timsehn/replication_example
9 of 9 chunks complete. 0 chunks being downloaded currently.
dolt $ cd read_replica/
read_replica $ dolt config --add --local sqlserver.global.dolt_read_replica_remote origin
Config successfully updated.
read_replica $ dolt config --add --local sqlserver.global.dolt_replicate_all_heads 1
Config successfully updated.
read_replica $ dolt ls
Tables in working set:
	 test

  1. Write to the replica on a branch
replication_example $ dolt sql -q "call dolt_checkout('-b', 'branch1'); insert into test values (1,1); call dolt_commit('-am', 'Inserted (1,1)');"
+--------+
| status |
+--------+
| 0      |
+--------+
Query OK, 1 row affected
+----------------------------------+
| hash                             |
+----------------------------------+
| g3g20hs8nhiblg9oa8lakp7jvhojc1sj |
+----------------------------------+
  1. Read from replica
read_replica $ dolt sql -q "call dolt_checkout('branch1'); select * from test;"
+--------+
| status |
+--------+
| 0      |
+--------+
+----+----+
| pk | c1 |
+----+----+
| 1  | 1  |
+----+----+
  1. Delete branch on master
replication_example $ dolt sql -q "call dolt_branch('-df', 'branch1');"
+--------+
| status |
+--------+
| 0      |
+--------+
  1. Branch still on DoltHub: https://www.dolthub.com/repositories/timsehn/replication_example
  2. Can still read it on replica.
read_replica $ dolt sql -q "call dolt_checkout('branch1'); select * from test;"
+--------+
| status |
+--------+
| 0      |
+--------+
+----+----+
| pk | c1 |
+----+----+
| 1  | 1  |
+----+----+
  1. Worse, if I create the same branch again on master:
replication_example $ dolt sql -q "call dolt_checkout('-b', 'branch1'); insert into test values (1,1); call dolt_commit('-am', 'Inserted (1,1)');"
+--------+
| status |
+--------+
| 0      |
+--------+
Query OK, 1 row affected
+----------------------------------+
| hash                             |
+----------------------------------+
| s389numuhtaaeomu4k36idiakv6ejanr |
+----------------------------------+
  1. The replica now freaks out.
read_replica $ dolt sql -q "call dolt_checkout('branch1'); select * from test;"
error on line 1 for query call dolt_checkout('branch1'): replication failed: dataset head is not ancestor of commit
replication failed: dataset head is not ancestor of commit

To delete a remote branch in git you need to run git push --delete <branch>. Dolt does not support this syntax.

Not sure how to solve this.

Also Max notes that other Dolt procedures that modify the commit graph may also have the same problem with replication.

@zachmu
Copy link
Member

zachmu commented Jul 12, 2022

Seems like replication should push branch deletions by default.

@timsehn
Copy link
Sponsor Contributor Author

timsehn commented Jul 12, 2022

To do that we would have to implement dolt push --delete <branch> functionality, at least in replication. Might as well make it work in the CLI and stored procedure as well.

@zachmu
Copy link
Member

zachmu commented Jul 12, 2022

The other side of this is that we need to then pull those branch deletions to each replica. I don't have a good idea how that would work off the top of my head. Replicas pull on read from a particular branch, there's no general mechanism for them to pull pre-emptively to cover this kind of use case.

@max-hoffman
Copy link
Contributor

This PR moves deletions to and from replication remote intermediaries with the replicate_all_heads setting. So read replicas can now have branches, including their active head branch, deleted. The replicate_heads setting will not delete the branches it pulls.

#3816

A more complete/user friendly solution would maybe have replicas to mirror the master's working set, so that branch changes, deletions, and merges are followed more closely.

@reltuk
Copy link
Contributor

reltuk commented Jul 13, 2022

What's the behavior of a read-replica on a force-push / non-fast-forward merge? My understanding is that in the current behavior, the read replica stops replication / fails to replicate – maybe it's a bug, or maybe it's intentional because of the potential for data loss? Branch deletion seems like a more extreme example of the force-push use case.

Not saying we shouldn't replicate, but maybe it should be an option and/or the same option should control whether force-pushes also replicate successfully?

@timsehn
Copy link
Sponsor Contributor Author

timsehn commented Jul 19, 2022

This is fixed. I'm closing the issue.

We need a "rethink replication" project. I think it makes sense to make a backup the middleman in replication as opposed to a remote. I also think it should be configured in server.yaml. Whether we deprecate the current remote mediated replication in favor of backup mediated is up for debate.

We'll schedule that post storage work.

@timsehn timsehn closed this as completed Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants