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

Support broken database cluster on startup #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bbrodriges
Copy link
Contributor

Error pops up while running newly added test with -race flag

$ go test -v -race -run=^TestCluster_BrokenAtStartup$ ./...
You use x86_64 version of selected tool.
=== RUN   TestCluster_BrokenAtStartup
=== RUN   TestCluster_BrokenAtStartup/broken_primary
==================
WARNING: DATA RACE
Read at 0x00c0001901d8 by goroutine 21:
  github.com/DATA-DOG/go-sqlmock.(*rowSets).Next()
      /Users/gzuykov/go/pkg/mod/github.com/!d!a!t!a-!d!o!g/[email protected]/rows.go:45 +0xa8
  database/sql.(*Rows).nextLocked()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2967 +0x1d0
  database/sql.(*Rows).Next.func1()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2945 +0x48
  database/sql.withLock()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:3396 +0x74
  database/sql.(*Rows).Next()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2944 +0x7c
  database/sql.(*Row).Scan()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:3333 +0x108
  golang.yandex/hasql/checkers.Check()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/checkers/check.go:29 +0xd8
  golang.yandex/hasql/checkers.PostgreSQL()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/checkers/postgresql.go:26 +0x54
  golang.yandex/hasql.checkExecutor.func1()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:149 +0x80
  golang.yandex/hasql.checkNodes.func1()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:109 +0x88

Previous write at 0x00c0001901d8 by goroutine 20:
  github.com/DATA-DOG/go-sqlmock.(*rowSets).Next()
      /Users/gzuykov/go/pkg/mod/github.com/!d!a!t!a-!d!o!g/[email protected]/rows.go:45 +0xc0
  database/sql.(*Rows).nextLocked()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2967 +0x1d0
  database/sql.(*Rows).Next.func1()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2945 +0x48
  database/sql.withLock()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:3396 +0x74
  database/sql.(*Rows).Next()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:2944 +0x7c
  database/sql.(*Row).Scan()
      /Users/gzuykov/.ya/tools/v4/1217047391/src/database/sql/sql.go:3333 +0x108
  golang.yandex/hasql/checkers.Check()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/checkers/check.go:29 +0xd8
  golang.yandex/hasql/checkers.PostgreSQL()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/checkers/postgresql.go:26 +0x54
  golang.yandex/hasql.checkExecutor.func1()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:149 +0x80
  golang.yandex/hasql.checkNodes.func1()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:109 +0x88

Goroutine 21 (running) created at:
  golang.yandex/hasql.checkNodes()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:106 +0x450
  golang.yandex/hasql.(*Cluster).updateNodes()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/cluster.go:321 +0x1b4
  golang.yandex/hasql.(*Cluster).backgroundNodesUpdate()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/cluster.go:297 +0x30

diff --git a/cluster_test.go b/cluster_test.go
index 10c22eb..c816f2f 100644
--- a/cluster_test.go
+++ b/cluster_test.go
@@ -29,6 +29,8 @@ import (
        "github.com/gofrs/uuid"
        "github.com/stretchr/testify/assert"
        "github.com/stretchr/testify/require"
+
+       "golang.yandex/hasql/checkers"
 )

 func TestNewCluster(t *testing.T) {
@@ -543,6 +545,89 @@ func TestCluster_WaitForStandbyPreferred(t *testing.T) {
        }
 }

+// TestCluster_BrokenAtStartup checks if broken at startup cluster behaves properly
+func TestCluster_BrokenAtStartup(t *testing.T) {
+       isPrimaryRow := sqlmock.NewRows([]string{`NOT pg_is_in_recovery()`}).AddRow(true)
+       isStandbyRow := sqlmock.NewRows([]string{`NOT pg_is_in_recovery()`}).AddRow(false)
+
+       testCases := []struct {
+               name  string
+               nodes []Node
+       }{
+               {
+                       name: "broken_primary",
+                       nodes: func() []Node {
+                               db1, mock1, _ := sqlmock.New() // primary
+                               db2, mock2, _ := sqlmock.New() // standby
+                               db3, mock3, _ := sqlmock.New() // standby
+
+                               mock1.ExpectQuery(`SELECT NOT pg_is_in_recovery()`).WillDelayFor(20 * time.Millisecond)
+                               mock2.ExpectQuery(`SELECT NOT pg_is_in_recovery()`).WillReturnRows(isStandbyRow)
+                               mock3.ExpectQuery(`SELECT NOT pg_is_in_recovery()`).WillReturnRows(isStandbyRow)
+
+                               return []Node{
+                                       NewNode("primary", db1),
+                                       NewNode("standby1", db2),
+                                       NewNode("standby2", db3),
+                               }
+                       }(),
+               },
+               {
Goroutine 20 (finished) created at:
  golang.yandex/hasql.checkNodes()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/check_nodes.go:106 +0x450
  golang.yandex/hasql.(*Cluster).updateNodes()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/cluster.go:321 +0x1b4
  golang.yandex/hasql.(*Cluster).backgroundNodesUpdate()
      /Users/gzuykov/dev/github.com/yandex/go-hasql/cluster.go:297 +0x30
==================
    testing.go:1152: race detected during execution of test
=== RUN   TestCluster_BrokenAtStartup/broken_standby
=== RUN   TestCluster_BrokenAtStartup/broken_standbys
    cluster_test.go:624:
        	Error Trace:	cluster_test.go:624
        	Error:      	Received unexpected error:
        	            	context deadline exceeded
        	Test:       	TestCluster_BrokenAtStartup/broken_standbys
    cluster_test.go:625:
        	Error Trace:	cluster_test.go:625
        	Error:      	Expected value not to be nil.
        	Test:       	TestCluster_BrokenAtStartup/broken_standbys
    cluster_test.go:626:
        	Error Trace:	cluster_test.go:626
        	Error:      	Should NOT be empty, but was []
        	Test:       	TestCluster_BrokenAtStartup/broken_standbys
=== CONT  TestCluster_BrokenAtStartup
    testing.go:1152: race detected during execution of test
--- FAIL: TestCluster_BrokenAtStartup (0.01s)
    --- FAIL: TestCluster_BrokenAtStartup/broken_primary (0.00s)
    --- PASS: TestCluster_BrokenAtStartup/broken_standby (0.00s)
    --- FAIL: TestCluster_BrokenAtStartup/broken_standbys (0.01s)
=== CONT
    testing.go:1152: race detected during execution of test
FAIL
FAIL	golang.yandex/hasql	0.286s
?   	golang.yandex/hasql/checkers	[no test files]
testing: warning: no tests to run
PASS
ok  	golang.yandex/hasql/sqlx	(cached) [no tests to run]
FAIL

@bbrodriges bbrodriges requested a review from sidh March 5, 2022 09:36
@sidh
Copy link
Collaborator

sidh commented Jun 9, 2022

Race is triggered because sqlmock.NewRows is not thread-safe. You must create one for each WillReturnRows.

I will look into the issue further.

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

Successfully merging this pull request may close these issues.

2 participants