server: disable DistSQL in server's InternalExecutor until migrations complete #51053
Conversation
|
This change is |
| @@ -610,6 +610,23 @@ func (s *sqlServer) start( | |||
| // present situation). | |||
| DistSQLMode: sessiondata.DistSQLOff, | |||
| }) | |||
| { | |||
There was a problem hiding this comment.
I guess I'd get this of the migrationsExecutor.
There was a problem hiding this comment.
I'm not sure I understand this comment.
pkg/server/server_sql.go
Outdated
| // The server's internalExecutor might be used as a side effect of | ||
| // migrations, so disable distribution for the same reason as above until | ||
| // all migrations have been run. | ||
| sessionDataCopy, sessionDataWasNil := s.internalExecutor.GetSessionData() |
There was a problem hiding this comment.
Can the results of GetSessionData() vary at all? If not, let's just assert that they are what they should be and don't bother keeping track of the old settings. In fact, I wouldn't even call GetSessionData().
There was a problem hiding this comment.
I don't think so since this is all in the startup path. Removed GetSessionData.
pkg/sql/internal.go
Outdated
| // | ||
| // SetSessionData cannot be called concurently with query execution. | ||
| func (ie *InternalExecutor) SetSessionData(sessionData *sessiondata.SessionData) { | ||
| ie.s.populateMinimalSessionData(sessionData) | ||
| if sessionData != nil { |
There was a problem hiding this comment.
I don't think this guy should accept a nil. I hope this becomes moot if it's true that GetSessionData() always returns nil in Server.Start.
|
Look's like the internal executor is already in use at this point ( Something I'm confused about is how exactly an internal executor uses |
… complete DistSQL is fragile on node startup (see cockroachdb#44101) and is therefore disabled in the migration manager's internal executor. However, migrations sometimes have side effects and sometimes end up using the SQL server's internal executor which had DistSQL enabled. This commit disabled DistSQL for this internal executor until the migrations complete to avoid flakiness. Release note: None (testing flake fix)
|
Planning to add a test to this. |
There was a problem hiding this comment.
Something I'm confused about is how exactly an internal executor uses DistSQL? It doesn't look like anywhere sets an explicit sessiondata with distsql enabled. An empty sessiondata is equivalent to having distsql turned off right?
Good question. I've just tested it and the following results in off.
func TestXXX(t *testing.T) {
defer leaktest.AfterTest(t)()
defer log.Scope(t).Close(t)
ctx := context.Background()
params, _ := tests.CreateTestServerParams()
s, _, _ := serverutils.StartServer(t, params)
defer s.Stopper().Stop(ctx)
ie := s.InternalExecutor().(*sql.InternalExecutor)
row, err := ie.QueryRowEx(ctx, "test", nil, /* txn */
sqlbase.InternalExecutorSessionDataOverride{User: security.RootUser},
"SHOW DISTSQL")
if err != nil {
t.Fatal(err)
}
if len(row) != 1 {
t.Fatalf("expected 1 col, got: %d", len(row))
}
r, ok := row[0].(*tree.DString)
log.Infof(ctx, "!!! ok: %t. r: %s", ok, r)
}
I think before #47019 maybe the session vars for an internal executor were initialized from the GlobalsDefaults, but not now (and I don't think that was generally the case then either).
Let's figure this out. Can you detail what seems to happen in #50000 ?
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andreimatei and @asubiotto)
pkg/server/server_sql.go, line 613 at r1 (raw file):
Previously, asubiotto (Alfonso Subiotto Marqués) wrote…
I'm not sure I understand this comment.
You can just have migMgr use s.internalExecutor, can you not? There's no longer a point to migrationsExecutor, is there?
There was a problem hiding this comment.
I should have said that uses of the internal executor from normal user sessions inherit the parent session's data through a SetSessionData() - which does get its original values from those GlobalDefaults, but the migrations start with empty data, so there should be nothing for their children to inherit I would think.
Reviewable status:
|
Won't be able to get to this until tomorrow. In #50000, some migration calls |
|
Is there a way to move forward on this, viz failing roachtests. |
|
Checked the distsql version on a manual run forcing migrations and it seems that distsql is off so don't think this PR would change anything. Guess it's back to the drawing board. Recent runs of |
not at this time no unfortunately. But i'll be on the lookout.
Yes that's what I'd do too. |
|
Superseded by #52624 |
DistSQL is fragile on node startup (see #44101) and is therefore disabled in
the migration manager's internal executor. However, migrations sometimes have
side effects and sometimes end up using the SQL server's internal executor
which had DistSQL enabled.
This commit disabled DistSQL for this internal executor until the migrations
complete to avoid flakiness.
Release note: None (testing flake fix)
Fixes #50000