From 5261b7058e5f3c6523670d5a026630af86a078c9 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Sat, 27 Jan 2024 10:05:35 -0500 Subject: [PATCH 1/4] Execute ExecuteRequests locally if possible --- store/store.go | 37 ++++++++-------- store/store_test.go | 101 +++++++++++++++----------------------------- 2 files changed, 54 insertions(+), 84 deletions(-) diff --git a/store/store.go b/store/store.go index a9f933c8..97a25594 100644 --- a/store/store.go +++ b/store/store.go @@ -1154,24 +1154,30 @@ func (s *Store) Request(eqr *proto.ExecuteQueryRequest) ([]*proto.ExecuteQueryRe return nil, ErrNotOpen } - if !s.RequiresLeader(eqr) { - if eqr.Level == proto.QueryRequest_QUERY_REQUEST_LEVEL_NONE && eqr.Freshness > 0 && - time.Since(s.raft.LastContact()).Nanoseconds() > eqr.Freshness { - return nil, ErrStaleRead - } + if s.QueriesOnly(eqr) { if eqr.Request.Transaction { // Transaction requested during query, but not going through consensus. This means // we need to block any database serialization during the query. s.queryTxMu.RLock() defer s.queryTxMu.RUnlock() } - return s.db.Request(eqr.Request, eqr.Timings) + + if eqr.Level == proto.QueryRequest_QUERY_REQUEST_LEVEL_NONE { + if eqr.Freshness > 0 && time.Since(s.raft.LastContact()).Nanoseconds() > eqr.Freshness { + return nil, ErrStaleRead + } + return s.db.Request(eqr.Request, eqr.Timings) + } else if eqr.Level == proto.QueryRequest_QUERY_REQUEST_LEVEL_WEAK { + if s.raft.State() != raft.Leader { + return nil, ErrNotLeader + } + return s.db.Request(eqr.Request, eqr.Timings) + } } if s.raft.State() != raft.Leader { return nil, ErrNotLeader } - if !s.Ready() { return nil, ErrNotReady } @@ -1635,24 +1641,21 @@ func (s *Store) Noop(id string) (raft.ApplyFuture, error) { return s.raft.Apply(bc, s.ApplyTimeout), nil } -// RequiresLeader returns whether the given ExecuteQueryRequest must be -// processed on the cluster Leader. -func (s *Store) RequiresLeader(eqr *proto.ExecuteQueryRequest) bool { - if eqr.Level != proto.QueryRequest_QUERY_REQUEST_LEVEL_NONE { - return true - } - +// QueriesOnly returns whether the given ExecuteQueryRequest contains only +// queries. +func (s *Store) QueriesOnly(eqr *proto.ExecuteQueryRequest) bool { for _, stmt := range eqr.Request.Statements { sql := stmt.Sql if sql == "" { continue } ro, err := s.db.StmtReadOnly(sql) - if !ro || err != nil { - return true + if ro && err == nil { + continue } + return false } - return false + return true } // setLogInfo records some key indexes about the log. diff --git a/store/store_test.go b/store/store_test.go index 2ea6a7a1..410894eb 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -2506,7 +2506,7 @@ func Test_IsVoter(t *testing.T) { } } -func Test_RequiresLeader(t *testing.T) { +func Test_QueriesOnly(t *testing.T) { s, ln := mustNewStore(t) defer ln.Close() @@ -2529,95 +2529,62 @@ func Test_RequiresLeader(t *testing.T) { } tests := []struct { - name string - stmts []string - lvl proto.QueryRequest_Level - requires bool + name string + stmts []string + queriesOnly bool }{ { - name: "Empty SQL", - stmts: []string{""}, - requires: false, - }, - { - name: "Junk SQL", - stmts: []string{"asdkflj asgkdj"}, - requires: true, - }, - { - name: "CREATE TABLE statement, already exists", - stmts: []string{"CREATE TABLE foo (id INTEGER NOT NULL PRIMARY KEY, name TEXT)"}, - requires: true, - }, - { - name: "CREATE TABLE statement, does not exists", - stmts: []string{"CREATE TABLE bar (id INTEGER NOT NULL PRIMARY KEY, name TEXT)"}, - requires: true, - }, - { - name: "Single INSERT", - stmts: []string{"INSERT INTO foo(id, name) VALUES(1, 'fiona')"}, - requires: true, - }, - { - name: "Single INSERT, non-existent table", - stmts: []string{"INSERT INTO qux(id, name) VALUES(1, 'fiona')"}, - requires: true, + name: "Empty SQL", + stmts: []string{""}, + queriesOnly: true, }, { - name: "Single SELECT with implicit NONE", - stmts: []string{"SELECT * FROM foo"}, - requires: false, + name: "Junk SQL", + stmts: []string{"asdkflj asgkdj"}, + queriesOnly: false, }, { - name: "Single SELECT with NONE", - stmts: []string{"SELECT * FROM foo"}, - lvl: proto.QueryRequest_QUERY_REQUEST_LEVEL_NONE, - requires: false, + name: "CREATE TABLE statement, already exists", + stmts: []string{"CREATE TABLE foo (id INTEGER NOT NULL PRIMARY KEY, name TEXT)"}, + queriesOnly: false, }, { - name: "Single SELECT from non-existent table with NONE", - stmts: []string{"SELECT * FROM qux"}, - lvl: proto.QueryRequest_QUERY_REQUEST_LEVEL_NONE, - requires: true, + name: "Single INSERT", + stmts: []string{"INSERT INTO foo(id, name) VALUES(1, 'fiona')"}, + queriesOnly: false, }, { - name: "Double SELECT with NONE", - stmts: []string{"SELECT * FROM foo", "SELECT * FROM foo WHERE id = 1"}, - lvl: proto.QueryRequest_QUERY_REQUEST_LEVEL_NONE, - requires: false, + name: "Single INSERT, non-existent table", + stmts: []string{"INSERT INTO qux(id, name) VALUES(1, 'fiona')"}, + queriesOnly: false, }, { - name: "Single SELECT with STRONG", - stmts: []string{"SELECT * FROM foo"}, - lvl: proto.QueryRequest_QUERY_REQUEST_LEVEL_STRONG, - requires: true, + name: "Single SELECT", + stmts: []string{"SELECT * FROM foo"}, + queriesOnly: true, }, { - name: "Single SELECT with WEAK", - stmts: []string{"SELECT * FROM foo"}, - lvl: proto.QueryRequest_QUERY_REQUEST_LEVEL_WEAK, - requires: true, + name: "Single SELECT from non-existent table", + stmts: []string{"SELECT * FROM qux"}, + queriesOnly: false, }, { - name: "Mix queries and executes with NONE", - stmts: []string{"SELECT * FROM foo", "INSERT INTO foo(id, name) VALUES(1, 'fiona')"}, - lvl: proto.QueryRequest_QUERY_REQUEST_LEVEL_NONE, - requires: true, + name: "Double SELECT", + stmts: []string{"SELECT * FROM foo", "SELECT * FROM foo WHERE id = 1"}, + queriesOnly: true, }, { - name: "Mix queries and executes with WEAK", - stmts: []string{"SELECT * FROM foo", "INSERT INTO foo(id, name) VALUES(1, 'fiona')"}, - lvl: proto.QueryRequest_QUERY_REQUEST_LEVEL_WEAK, - requires: true, + name: "Mix queries and executes", + stmts: []string{"SELECT * FROM foo", "INSERT INTO foo(id, name) VALUES(1, 'fiona')"}, + queriesOnly: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - requires := s.RequiresLeader(executeQueryRequestFromStrings(tt.stmts, tt.lvl, false, false)) - if requires != tt.requires { - t.Fatalf(" test %s failed, unexpected requires: expected %v, got %v", tt.name, tt.requires, requires) + requires := s.QueriesOnly(executeQueryRequestFromStrings(tt.stmts, proto.QueryRequest_QUERY_REQUEST_LEVEL_NONE, false, false)) + if requires != tt.queriesOnly { + t.Fatalf(" test %s failed, unexpected requires: expected %v, got %v", tt.name, tt.queriesOnly, requires) } }) } From ba203fde6df66f89e55381ba88d32b9caefaa13e Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Sat, 27 Jan 2024 10:07:09 -0500 Subject: [PATCH 2/4] CHANGELOG --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a346054e..1e2acbc5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +## 8.18.2 (unreleased) +### Implementation changes and bug fixes +- [PR #1635](https://github.com/rqlite/rqlite/pull/1635): Always execute ExecuteRequests locally if possible. + ## 8.18.1 (January 26th 2024) ### Implementation changes and bug fixes - [PR #1633](https://github.com/rqlite/rqlite/pull/1633): Improve error messages for internode communication failures. From 9df4c0527c54dbbf5cbba1d12eb5701c018ecfa5 Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Sat, 27 Jan 2024 21:39:43 -0500 Subject: [PATCH 3/4] Optimize Requests --- store/store.go | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/store/store.go b/store/store.go index caecfce6..1048a1f3 100644 --- a/store/store.go +++ b/store/store.go @@ -1164,16 +1164,28 @@ func (s *Store) Request(eqr *proto.ExecuteQueryRequest) ([]*proto.ExecuteQueryRe defer s.queryTxMu.RUnlock() } + convertFn := func(qr []*proto.QueryRows) []*proto.ExecuteQueryResponse { + resp := make([]*proto.ExecuteQueryResponse, len(qr)) + for i := range qr { + resp[i] = &proto.ExecuteQueryResponse{ + Result: &proto.ExecuteQueryResponse_Q{Q: qr[i]}, + } + } + return resp + } + if eqr.Level == proto.QueryRequest_QUERY_REQUEST_LEVEL_NONE { if eqr.Freshness > 0 && time.Since(s.raft.LastContact()).Nanoseconds() > eqr.Freshness { return nil, ErrStaleRead } - return s.db.Request(eqr.Request, eqr.Timings) + qr, err := s.db.Query(eqr.Request, eqr.Timings) + return convertFn(qr), err } else if eqr.Level == proto.QueryRequest_QUERY_REQUEST_LEVEL_WEAK { if s.raft.State() != raft.Leader { return nil, ErrNotLeader } - return s.db.Request(eqr.Request, eqr.Timings) + qr, err := s.db.Query(eqr.Request, eqr.Timings) + return convertFn(qr), err } } @@ -1652,10 +1664,9 @@ func (s *Store) QueriesOnly(eqr *proto.ExecuteQueryRequest) bool { continue } ro, err := s.db.StmtReadOnly(sql) - if ro && err == nil { - continue + if err != nil || !ro { + return false } - return false } return true } From 4bb5e4b5069704b27b5d0579fbbde11e5444216e Mon Sep 17 00:00:00 2001 From: Philip O'Toole Date: Sat, 27 Jan 2024 21:42:53 -0500 Subject: [PATCH 4/4] Remove superfluous log message --- cmd/rqlited/main.go | 1 - 1 file changed, 1 deletion(-) diff --git a/cmd/rqlited/main.go b/cmd/rqlited/main.go index 0ab85f66..7d514fe9 100644 --- a/cmd/rqlited/main.go +++ b/cmd/rqlited/main.go @@ -165,7 +165,6 @@ func main() { if err != nil { log.Fatalf("failed to start HTTP server: %s", err.Error()) } - log.Printf("HTTP server started") // Now, open store. How long this takes does depend on how much data is being stored by rqlite. if err := str.Open(); err != nil {