Fix various tests

[BuonOmo]

No description provided.

[BuonOmo]

@rafiss I'm working on the Serialization Failure related tests. I saw some different behaviour between pg and crdb. Do you have more inputs about it? It seems like PG behaviour is more protecting in case of concurrent updates.

The reproduction below should be ran with two different connections (I used two terminals ^^)

-- any thread
SET sql_safe_updates = false;
DROP TABLE IF EXISTS "samples";
DROP TABLE IF EXISTS "bits";

CREATE TABLE "samples" ("id" serial primary key, "value" integer);
CREATE TABLE "bits" ("id" serial primary key, "value" integer);

INSERT INTO "samples" ("value") VALUES (1) RETURNING "id";

-- thread right
BEGIN ISOLATION LEVEL SERIALIZABLE;
UPDATE "samples" SET "value" = 2;

-- thread left
BEGIN ISOLATION LEVEL SERIALIZABLE;
INSERT INTO "samples" ("value") VALUES (3) RETURNING "id";
SAVEPOINT active_record_1;

--thread right
COMMIT;

--thread left
UPDATE "samples" SET "value" = 4;
RELEASE SAVEPOINT active_record_1; -- <-------- This fails on PG and works on CRDB (ignoring previous update)

INSERT INTO "bits" ("value") VALUES (1) RETURNING "id";

COMMIT;

-- any thread
DROP TABLE IF EXISTS "samples";
DROP TABLE IF EXISTS "bits";

[rafiss]

This seems like an expected difference, due to the differences in how CRDB vs PG has decided to implement SERIALIZABLE. The short explanation is that in CRDB SERIALIZABLE, reads block on in-progress writes for as long as those writes are in progress. However, PG does not have this "read block on write" behavior, and so rather than allowing the left-hand-side to execute, it must instead abort that transaction. Both are valid ways to implement SERIALIZABLE.

So for this test case, maybe we should just skip it.

[BuonOmo]

@rafiss there are many test cases impacted, I think I'll rewrite them (or at least some of them) to be sure we keep coverage! Thank you for that explanation :)

[rafiss]

No problem!

Are these tests explicitly using SERIALIZABLE even with Postgres? If it's just using the default isolation level, another option is to configure CRDB to use READ COMMITTED isolation by default.

[BuonOmo]

Are these tests explicitly using SERIALIZABLE even with Postgres?

Yes they are!

I'm also wondering when rewriting the tests : how can I generate a serialization failure in CRDB to have correct tests? I'm playing around now to find it, but if you have a clear example it might help me!

EDIT: I found inject_retry_errors_enabled, but I'd rather find a proper use case if possible!

Also, I think there is still an issue. 5 minutes tests. In PG, they raise a deadlock, in CRDB, they hang...

Here's what the tests is supposed to do:

DROP TABLE IF EXISTS "samples";
DROP TABLE IF EXISTS "bits";

CREATE TABLE "samples" ("id" text primary key, "value" integer);
CREATE TABLE "bits" ("id" serial primary key, "value" integer);

-- LEFT THREAD

INSERT INTO "samples" ("id", "value") VALUES ('s1', 1);
INSERT INTO "samples" ("id", "value") VALUES ('s2', 2);

BEGIN;
SELECT * FROM "bits" LIMIT 1; -- make_parent_transaction_dirty
SAVEPOINT active_record_1;
SELECT * FROM "samples" WHERE "samples"."id" = 's1' FOR UPDATE; -- s1.lock!


-- RIGHT THREAD

BEGIN;
SELECT * FROM "bits" LIMIT 1; -- make_parent_transaction_dirty
SAVEPOINT active_record_1;
SELECT * FROM "samples" WHERE "samples"."id" = 's2' FOR UPDATE; -- s2.lock!
UPDATE "samples" SET "value" = 2 WHERE "samples"."id" = 's1'; -- s1.update


-- LEFT THREAD

UPDATE "samples" SET "value" = 1 WHERE "samples"."id" = 's2'; -- s2.update

This generates an error:

ERROR: restart transaction: TransactionRetryWithProtoRefreshError: TransactionAbortedError(ABORT_REASON_PUSHER_ABORTED): "sql txn" meta={id=e120e22f key=/Table/26717/1/"s1"/0 iso=Serializable pri=0.03969097 epo=0 ts=1721389306.397247000,0 min=1721389290.379415000,0 seq=0} lock=true stat=ABORTED rts=1721389290.379415000,0 wto=false gul=1721389290.879415000,0
SQLSTATE: 40001
HINT: See: https://www.cockroachlabs.com/docs/v23.2/transaction-retry-error-reference.html#abort_reason_pusher_aborted

But with the transaction retry logic implemented client side, it seems like we catch this error, rerun a bunch of queries (I'll isolate them later, it's a tedious task!) and finally a deadlock happens...

[BuonOmo]

Here's the curated sql repro for the test test_deadlock_raises_Deadlocked_inside_nested_SavepointTransaction:

DROP TABLE IF EXISTS "samples";
DROP TABLE IF EXISTS "bits";

CREATE TABLE "samples" ("id" text primary key, "value" integer);
CREATE TABLE "bits" ("id" serial primary key, "value" integer);

INSERT INTO "samples" ("id", "value") VALUES ('s1', 1);
INSERT INTO "samples" ("id", "value") VALUES ('s2', 2);

-- thread left
BEGIN; -- manually added (not logged directly by rails)
SELECT "bits".* FROM "bits" LIMIT 1; -- attempts 0
SELECT "samples".* FROM "samples" WHERE "samples"."id" = 's2' LIMIT 1 FOR UPDATE; -- attempts 0
SAVEPOINT active_record_1; -- attempts 0


-- thread right
BEGIN; -- attempts 0
SELECT "bits".* FROM "bits" LIMIT 1; -- attempts 0
-- NOTE: it seems like we start hanging on this query below.
SELECT "samples".* FROM "samples" WHERE "samples"."id" = 's1' LIMIT 1 FOR UPDATE; -- attempts 0
SAVEPOINT active_record_1; -- attempts 0
UPDATE "samples" SET "value" = 1 WHERE "samples"."id" = 's2'; -- attempts 0


-- thread left
UPDATE "samples" SET "value" = 2 WHERE "samples"."id" = 's1'; -- attempts 0


-- thread right
RELEASE SAVEPOINT active_record_1; -- attempts 0
COMMIT; -- attempts 0


-- thread left
ROLLBACK TO SAVEPOINT active_record_1; -- attempts 0
SELECT "samples".* FROM "samples" WHERE "samples"."id" = 's2' LIMIT 1 FOR UPDATE; -- attempts 1
SAVEPOINT active_record_1; -- attempts 1
SELECT "samples".* FROM "samples" WHERE "samples"."id" = 's2' LIMIT 1 FOR UPDATE; -- attempts 2
SAVEPOINT active_record_1; -- attempts 2
SELECT "samples".* FROM "samples" WHERE "samples"."id" = 's2' LIMIT 1 FOR UPDATE; -- attempts 3
SAVEPOINT active_record_1; -- attempts 3
ROLLBACK; -- attempts 3
BEGIN; -- attempts 1
SELECT "bits".* FROM "bits" LIMIT 1; -- attempts 1
SELECT "samples".* FROM "samples" WHERE "samples"."id" = 's2' LIMIT 1 FOR UPDATE; -- attempts 0
SAVEPOINT active_record_1; -- attempts 0
ROLLBACK TO SAVEPOINT active_record_1; -- attempts 0
ROLLBACK; -- attempts 0

Note that this transcript includes a call to sigint and likely a rescue block issuing some sql queries afterwards, I don't know where during this mess yet

---

EDIT: if I get rid of the client side retry logic, we now get a serialization failure. Validating the previous message

[rafiss]

Nice investigation! So if disabling the automatic retry logic causes the proper error as expected, then I think it's fair for us to skip this test in the CRDB test suite.

If there's a way to tell the test to turn off the auto-retry behavior, then we can keep the test. But I don't think we should spend time on adding that functionality.

[BuonOmo]

Skipping for now. I'll see what's up with 7.2

Ready to merge when you are @rafiss !

[rafiss]

I noticed this error. it must have been happening before, but I hadn't seen it before.

/home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:55:in 'PG::Connection#exec': PG::FeatureNotSupported: ERROR:  at or near "before_insert_trigger": syntax error: unimplemented: this syntax (ActiveRecord::StatementInvalid)
DETAIL:  source SQL:
CREATE TRIGGER before_insert_trigger
               ^
HINT:  You have attempted to use a feature that is not yet implemented.
See: https://go.crdb.dev/issue-v/28296/v23.2
Using cockroachdb

	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:55:in 'block (2 levels) in ActiveRecord::ConnectionAdapters::PostgreSQL::DatabaseStatements#raw_execute'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:1027:in 'block in ActiveRecord::ConnectionAdapters::AbstractAdapter#with_raw_connection'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activesupport/lib/active_support/concurrency/null_lock.rb:9:in 'ActiveSupport::Concurrency::NullLock#synchronize'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:999:in 'ActiveRecord::ConnectionAdapters::AbstractAdapter#with_raw_connection'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:54:in 'block in ActiveRecord::ConnectionAdapters::PostgreSQL::DatabaseStatements#raw_execute'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activesupport/lib/active_support/notifications/instrumenter.rb:58:in 'ActiveSupport::Notifications::Instrumenter#instrument'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:1142:in 'ActiveRecord::ConnectionAdapters::AbstractAdapter#log'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:53:in 'ActiveRecord::ConnectionAdapters::PostgreSQL::DatabaseStatements#raw_execute'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:521:in 'ActiveRecord::ConnectionAdapters::DatabaseStatements#internal_execute'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/abstract/database_statements.rb:131:in 'ActiveRecord::ConnectionAdapters::DatabaseStatements#execute'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/abstract/query_cache.rb:25:in 'ActiveRecord::ConnectionAdapters::AbstractAdapter#execute'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/lib/active_record/connection_adapters/postgresql/database_statements.rb:47:in 'ActiveRecord::ConnectionAdapters::PostgreSQL::DatabaseStatements#execute'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/test/schema/schema.rb:1493:in '<top (required)>'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/test/support/load_schema_helper.rb:12:in 'Kernel#load'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/test/support/load_schema_helper.rb:12:in 'LoadSchemaHelper#load_schema'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/test/cases/helper_cockroachdb.rb:25:in 'LoadSchemaHelperExt#load_schema'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/test/cases/test_case.rb:250:in '<class:TestCase>'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/test/cases/test_case.rb:20:in '<module:ActiveRecord>'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/test/cases/test_case.rb:16:in '<top (required)>'
	from /home/runner/.rubies/ruby-head/lib/ruby/3.4.0+0/bundled_gems.rb:75:in 'Kernel.require'
	from /home/runner/.rubies/ruby-head/lib/ruby/3.4.0+0/bundled_gems.rb:75:in 'block (2 levels) in Kernel#replace_require'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/bundler/gems/rails-b05945903e36/activerecord/test/cases/helper.rb:8:in '<top (required)>'
	from /home/runner/.rubies/ruby-head/lib/ruby/3.4.0+0/bundled_gems.rb:75:in 'Kernel.require'
	from /home/runner/.rubies/ruby-head/lib/ruby/3.4.0+0/bundled_gems.rb:75:in 'block (2 levels) in Kernel#replace_require'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/test/cases/helper_cockroachdb.rb:31:in '<top (required)>'
	from /home/runner/.rubies/ruby-head/lib/ruby/3.4.0+0/bundled_gems.rb:75:in 'Kernel.require'
	from /home/runner/.rubies/ruby-head/lib/ruby/3.4.0+0/bundled_gems.rb:75:in 'block (2 levels) in Kernel#replace_require'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:21:in 'block in <main>'
	from /home/runner/work/activerecord-cockroachdb-adapter/activerecord-cockroachdb-adapter/vendor/bundle/ruby/3.4.0+0/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in 'Array#select'