Disable automatic write protection on replicas #42450

HParker · 2021-06-10T23:20:35Z

This allows users to use write protection if they want, but no longer treats it as a catch all. Write protection is not accurate enough to classify all cases correctly. Instead users should configure their replicas to not allow writes and rely on the database erroring if the query is not allowed.

Alternative solution for: #42432
Fixes: #42432

ghiculescu · 2021-06-10T23:47:28Z

I am in favor of this. Is there any other reason for rails to know if the db is a replica? If not, I imagine you can remove a fair bit more code.

eileencodes · 2021-06-11T12:20:47Z

Is there any other reason for rails to know if the db is a replica? If not, I imagine you can remove a fair bit more code.

Yes, we need to know a configuration is a replica so that we don't run rake tasks against it. I don't think offhand there is any other code to remove since we're not dropping replica support, just dropping the automatic prevent_writes from it.

prevent_writes was designed for the case where you're sending read queries to your primary and want to make sure they behave similarly to a replica. For example if you're using the auto switcher middleware you dont want your GET requests to have any writes in them (without switching to writing) because when it goes to the replica it won't write. To make GET's going to the primary and replica behave the same, we added this functionality.

eileencodes

Thanks Adam! This also needs a changelog entry.

activerecord/test/cases/base_test.rb

ghiculescu · 2021-06-11T15:24:18Z

activerecord/CHANGELOG.md

+*   Disable automatic write protection on replicas
+
+    Write protection no longer automatically enable for replicas.
+    Now each application controls when to enable automatic write protection.


Do you think it's worth mentioning this on https://edgeguides.rubyonrails.org/active_record_multiple_databases.html too?

I am reading now, if it was mentioned before we should probably remove it, but I don't know how/if we should mention rails not doing something you maybe didn't know it did at all before.

I had a quick read through and couldn't find any mention of this behaviour. I think AR.while_preventing_writes could be mentioned but it's not really in the scope of this PR.

I can't find a spot that seems like it needs updating, but it is totally possible I am missing it. Let me know if I missed anything @ghiculescu 🙂

I would add onto the guides and explain that if you want replicas to act like replicas they need to be configured with readonly users

^ that is alluded to, kind of

@HParker I'll make a separate PR, because I don't think I'm making much sense here.

Under the setup section we have:

Second, the username for the writers and replicas should be different, and the replica user's permissions should be set to only read and not write.

When using a replica database, you need to add a replica: true entry to the replica in the database.yml. This is because Rails otherwise has no way of knowing which one is a replica and which one is the writer.

https://edgeguides.rubyonrails.org/active_record_multiple_databases.html#setting-up-your-application

Should I expand on that?

ghiculescu · 2021-06-11T16:38:22Z

Just noticed this line: https://github.com/rails/rails/blob/eca7582718225ef40a7618293a13924561194aae/activerecord/lib/active_record/connection_handling.rb#L355. Should it also be removed? It gets called by connected_to; the equivalent line in connected_to_many has been removed.

HParker · 2021-06-11T17:20:15Z

Thanks @ghiculescu that line in connection_handling turned up a lot of places I missed ❤️

activerecord/test/cases/base_test.rb

activerecord/test/cases/connection_adapters/legacy_connection_handlers_multi_db_test.rb

eileencodes

I think the CHANGELOG can be clearer - but once that's fixed and the conflicts are resolved this looks good to me 👍🏼

eileencodes · 2021-06-16T11:00:24Z

activerecord/CHANGELOG.md

@@ -1,3 +1,10 @@
+*   Disable automatic write protection on replicas


Suggested change

* Disable automatic write protection on replicas

* Disable automatic write protection on replicas.

eileencodes · 2021-06-16T11:01:30Z

activerecord/CHANGELOG.md

@@ -1,3 +1,10 @@
+*   Disable automatic write protection on replicas
+
+    Write protection no longer automatically enable for replicas.


Suggested change

Write protection no longer automatically enable for replicas.

Write protection is no longer automatically enabled for replicas. Write protection should be enabled by the database user settings.

eileencodes · 2021-06-16T11:01:36Z

activerecord/CHANGELOG.md

+*   Disable automatic write protection on replicas
+
+    Write protection no longer automatically enable for replicas.
+    Now each application controls when to enable automatic write protection.


Suggested change

Now each application controls when to enable automatic write protection.

This allows users to use write protection if they want, but no longer treats it as a catch all. Write protection is not accurate enough to classify all cases correctly. Instead users should configure their replicas to not allow writes and rely on the database erroring if the query is not allowed. Co-authored-by: Alex Ghiculescu <alexghiculescu@gmail.com>

HParker · 2021-06-16T11:33:49Z

Thanks @eileencodes! updated

rafaelfranca · 2021-06-17T21:34:59Z

activerecord/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   Disable automatic write protection on replicas.
+
+    Write protection is no longer automatically enabled for replicas. Write protection should be enabled by the database user settings.


Can we expand with an example what "Write protection should be enabled by the database user settings." means?

Good idea @rafaelfranca, I put together #42529

I am not sure how much detail to add since it is not only database specific, but also configuration specific. Depending on how your primary and replica are setup you might want a totally different configuration. Ideally you would setup a local database user to be very similar to how your production users are setup.

rails-bot bot added the activerecord label Jun 10, 2021

HParker marked this pull request as ready for review June 10, 2021 23:36

zzak requested a review from eileencodes June 11, 2021 01:03

eileencodes requested changes Jun 11, 2021

View reviewed changes

activerecord/test/cases/base_test.rb Show resolved Hide resolved

HParker force-pushed the disable-prevent-write-on-replica branch 2 times, most recently from 42cfc1f to eca7582 Compare June 11, 2021 15:21

ghiculescu reviewed Jun 11, 2021

View reviewed changes

HParker force-pushed the disable-prevent-write-on-replica branch 2 times, most recently from bf9000d to 8d7d4d9 Compare June 11, 2021 19:50

ghiculescu mentioned this pull request Jun 14, 2021

Blank query can't be executed on a readonly database connection #42432

Closed

ghiculescu reviewed Jun 14, 2021

View reviewed changes

activerecord/test/cases/base_test.rb Show resolved Hide resolved

ghiculescu reviewed Jun 14, 2021

View reviewed changes

activerecord/test/cases/connection_adapters/legacy_connection_handlers_multi_db_test.rb Outdated Show resolved Hide resolved

ghiculescu mentioned this pull request Jun 14, 2021

Postgres: allow blank queries while preventing writes #42433

Closed

HParker force-pushed the disable-prevent-write-on-replica branch 4 times, most recently from 16034e2 to 5d47eb3 Compare June 15, 2021 15:38

zzak requested a review from eileencodes June 16, 2021 01:59

eileencodes requested changes Jun 16, 2021

View reviewed changes

HParker force-pushed the disable-prevent-write-on-replica branch from 5d47eb3 to 951deec Compare June 16, 2021 11:31

eileencodes merged commit 25292a3 into rails:main Jun 16, 2021

rafaelfranca reviewed Jun 17, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable automatic write protection on replicas #42450

Disable automatic write protection on replicas #42450

HParker commented Jun 10, 2021 •

edited

ghiculescu commented Jun 10, 2021

eileencodes commented Jun 11, 2021

eileencodes left a comment •

edited

ghiculescu Jun 11, 2021

HParker Jun 11, 2021

ghiculescu Jun 11, 2021

HParker Jun 11, 2021

eileencodes Jun 11, 2021

ghiculescu Jun 11, 2021

HParker Jun 11, 2021 •

edited

ghiculescu Jun 11, 2021

ghiculescu commented Jun 11, 2021

HParker commented Jun 11, 2021

eileencodes left a comment

eileencodes Jun 16, 2021

eileencodes Jun 16, 2021

eileencodes Jun 16, 2021

HParker commented Jun 16, 2021

rafaelfranca Jun 17, 2021

HParker Jun 17, 2021

		@@ -1,3 +1,10 @@
		* Disable automatic write protection on replicas

		@@ -1,3 +1,10 @@
		* Disable automatic write protection on replicas

		Write protection no longer automatically enable for replicas.

	Write protection no longer automatically enable for replicas.
	Write protection is no longer automatically enabled for replicas. Write protection should be enabled by the database user settings.

		@@ -1,3 +1,9 @@
		* Disable automatic write protection on replicas.

		Write protection is no longer automatically enabled for replicas. Write protection should be enabled by the database user settings.

Disable automatic write protection on replicas #42450

Disable automatic write protection on replicas #42450

Conversation

HParker commented Jun 10, 2021 • edited

ghiculescu commented Jun 10, 2021

eileencodes commented Jun 11, 2021

eileencodes left a comment • edited

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

HParker Jun 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghiculescu commented Jun 11, 2021

HParker commented Jun 11, 2021

eileencodes 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

HParker commented Jun 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HParker commented Jun 10, 2021 •

edited

eileencodes left a comment •

edited

HParker Jun 11, 2021 •

edited