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.

Sign up for GitHub

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

schema.Reload(): ignore column reading errors for views only, error for tables #13442

Merged
merged 8 commits into from
Jul 7, 2023
Merged

schema.Reload(): ignore column reading errors for views only, error for tables #13442

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
61e3ce5
schema.Reload(): ignore column reading errors for views only, error f…
shlomi-noach Jul 5, 2023
d48fea6
restore to pre-PR
shlomi-noach Jul 5, 2023
bfd05a2
add ERNoSuchUser error (view's definer not found)
shlomi-noach Jul 5, 2023
20dfcc5
Error
shlomi-noach Jul 5, 2023
bb7bd0a
ignore either missing columns or unknown definer -- for views
shlomi-noach Jul 5, 2023
b18c261
adapt tests
shlomi-noach Jul 5, 2023
f034dcf
cleanup
shlomi-noach Jul 5, 2023
72d2e6d
update comment
shlomi-noach Jul 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions go/mysql/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,7 @@ const (
ERIllegalValueForType = ErrorCode(1367)
ERDataTooLong = ErrorCode(1406)
ErrWrongValueForType = ErrorCode(1411)
ERNoSuchUser = ErrorCode(1449)
ERForbidSchemaChange = ErrorCode(1450)
ERWrongValue = ErrorCode(1525)
ERDataOutOfRange = ErrorCode(1690)
Expand Down
16 changes: 14 additions & 2 deletions go/vt/mysqlctl/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,14 @@ import (

var autoIncr = regexp.MustCompile(` AUTO_INCREMENT=\d+`)

type EmptyColumnsErr struct {
dbName, tableName, query string
}

func (e EmptyColumnsErr) Error() string {
return fmt.Sprintf("unable to get columns for table %s.%s using query %s", e.dbName, e.tableName, e.query)
}

// executeSchemaCommands executes some SQL commands. It uses the dba connection parameters, with credentials.
func (mysqld *Mysqld) executeSchemaCommands(ctx context.Context, sql string) error {
params, err := mysqld.dbcfgs.DbaConnector().MysqlParams()
Expand Down Expand Up @@ -283,6 +291,10 @@ const (
GetFieldsQuery = "SELECT %s FROM %s WHERE 1 != 1"
)

// GetColumnsList returns the column names for a given table/view, using a query generating function.
// Returned values:
// - selectColumns: a string of comma delimited qualified names to be used in a SELECT query. e.g. "`id`, `name`, `val`"
// - err: error
func GetColumnsList(dbName, tableName string, exec func(string, int, bool) (*sqltypes.Result, error)) (string, error) {
var dbName2 string
if dbName == "" {
Expand All @@ -296,8 +308,8 @@ func GetColumnsList(dbName, tableName string, exec func(string, int, bool) (*sql
return "", err
}
if qr == nil || len(qr.Rows) == 0 {
err = fmt.Errorf("unable to get columns for table %s.%s using query %s", dbName, tableName, query)
log.Errorf("%s", fmt.Errorf("unable to get columns for table %s.%s using query %s", dbName, tableName, query))
err := &EmptyColumnsErr{dbName: dbName, tableName: tableName, query: query}
log.Error(err.Error())
return "", err
}
selectColumns := ""
Expand Down
28 changes: 26 additions & 2 deletions go/vt/vttablet/tabletserver/schema/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,19 @@ import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
"strings"
"sync"
"time"

"golang.org/x/exp/maps"

"vitess.io/vitess/go/sqltypes"
"vitess.io/vitess/go/vt/concurrency"
"vitess.io/vitess/go/vt/mysqlctl"
"vitess.io/vitess/go/vt/mysqlctl/tmutils"
"vitess.io/vitess/go/vt/sidecardb"

"vitess.io/vitess/go/acl"
Expand Down Expand Up @@ -441,6 +446,7 @@ func (se *Engine) reload(ctx context.Context, includeStats bool) error {
return err
}

rec := concurrency.AllErrorRecorder{}
// curTables keeps track of tables in the new snapshot so we can detect what was dropped.
curTables := map[string]bool{"dual": true}
// changedTables keeps track of tables that have changed so we can reload their pk info.
Expand Down Expand Up @@ -491,9 +497,24 @@ func (se *Engine) reload(ctx context.Context, includeStats bool) error {
}

log.V(2).Infof("Reading schema for table: %s", tableName)
table, err := LoadTable(conn, se.cp.DBName(), tableName, row[1].String(), row[3].ToString())
tableType := row[1].String()
table, err := LoadTable(conn, se.cp.DBName(), tableName, tableType, row[3].ToString())
if err != nil {
log.Warningf("Failed reading schema for the table: %s, error: %v", tableName, err)
isView := strings.Contains(tableType, tmutils.TableView)
var emptyColumnsError mysqlctl.EmptyColumnsErr
if errors.As(err, &emptyColumnsError) && isView {
log.Warningf("Failed reading schema for the table: %s, error: %v", tableName, err)
continue
}
sqlErr, isSQLErr := mysql.NewSQLErrorFromError(err).(*mysql.SQLError)
if isSQLErr && sqlErr != nil && sqlErr.Number() == mysql.ERNoSuchUser && isView {
// A VIEW that has an invalid DEFINER, leading to:
// ERROR 1449 (HY000): The user specified as a definer (...) does not exist
log.Warningf("Failed reading schema for the table: %s, error: %v", tableName, err)
Comment on lines +510 to +513
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not the only error that can happen on view.
The table on which the view is defined does not exist can also happen and in that case there can be a different error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The scenario you're refrring to is covered by the fact the query on information_schema does not return a row. It is handled here: https://github.com/vitessio/vitess/pull/13442/files#diff-106487fd3d9cbbb2b783c2d8aff23d3f448de495303a13801c6eebdbcefc0c7aR311

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up on this, do we have a unit test for this case?

continue
}
// Non recoverable error:
rec.RecordError(vterrors.Wrapf(err, "in Engine.reload(), reading table %s", tableName))
continue
}
if includeStats {
Expand All @@ -508,6 +529,9 @@ func (se *Engine) reload(ctx context.Context, includeStats bool) error {
created = append(created, table)
}
}
if rec.HasErrors() {
return rec.Error()
}

dropped := se.getDroppedTables(curTables, changedViews, mismatchTables)

Expand Down
6 changes: 2 additions & 4 deletions go/vt/vttablet/tabletserver/schema/engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,18 +447,16 @@ func TestOpenFailedDueToLoadTableErr(t *testing.T) {
db.AddQueryPattern(fmt.Sprintf(mysql.GetColumnNamesQueryPatternForTable, "test_view"),
sqltypes.MakeTestResult(sqltypes.MakeTestFields("column_name", "varchar"), ""))
// rejecting the impossible query
db.AddRejectedQuery("SELECT * FROM `fakesqldb`.`test_view` WHERE 1 != 1", errors.New("The user specified as a definer ('root'@'%') does not exist (errno 1449) (sqlstate HY000)"))
db.AddRejectedQuery("SELECT * FROM `fakesqldb`.`test_view` WHERE 1 != 1", mysql.NewSQLErrorFromError(errors.New("The user specified as a definer ('root'@'%') does not exist (errno 1449) (sqlstate HY000)")))

AddFakeInnoDBReadRowsResult(db, 0)
se := newEngine(10, 1*time.Second, 1*time.Second, 0, db)
err := se.Open()
// failed load should not return any error, instead should be logged.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment has to change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated comment

require.NoError(t, err)
assert.ErrorContains(t, err, "Row count exceeded")

logs := tl.GetAllLogs()
logOutput := strings.Join(logs, ":::")
assert.Contains(t, logOutput, "WARNING:Failed reading schema for the table: test_table")
assert.Contains(t, logOutput, "Row count exceeded")
assert.Contains(t, logOutput, "WARNING:Failed reading schema for the table: test_view")
assert.Contains(t, logOutput, "The user specified as a definer ('root'@'%') does not exist (errno 1449) (sqlstate HY000)")
}
Expand Down
3 changes: 2 additions & 1 deletion go/vt/vttablet/tabletserver/schema/load_table.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"vitess.io/vitess/go/mysql/collations"
"vitess.io/vitess/go/sqltypes"
"vitess.io/vitess/go/vt/mysqlctl"
"vitess.io/vitess/go/vt/mysqlctl/tmutils"
querypb "vitess.io/vitess/go/vt/proto/query"
"vitess.io/vitess/go/vt/sqlparser"
"vitess.io/vitess/go/vt/vtgate/evalengine"
Expand All @@ -48,7 +49,7 @@ func LoadTable(conn *connpool.DBConn, databaseName, tableName, tableType string,
return nil, err
}
ta.Type = Message
case strings.Contains(tableType, "VIEW"):
case strings.Contains(tableType, tmutils.TableView):
ta.Type = View
}
return ta, nil
Expand Down