Simplify pushdown disabling logic (#2008)

FerretDB · Feb 16, 2023 · 23c574f · 23c574f
1 parent 32238ee
commit 23c574f
Show file tree

Hide file tree

Showing 8 changed files with 48 additions and 57 deletions.
diff --git a/internal/handlers/pg/msg_explain.go b/internal/handlers/pg/msg_explain.go
@@ -59,7 +59,6 @@ func (h *Handler) MsgExplain(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg,
 	}
 
 	qp.Explain = true
-	qp.DisablePushdown = h.DisablePushdown
 
 	explain, err := common.GetRequiredParam[*types.Document](document, "explain")
 	if err != nil {
@@ -71,6 +70,10 @@ func (h *Handler) MsgExplain(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg,
 		return nil, lazyerrors.Error(err)
 	}
 
+	if h.DisablePushdown {
+		qp.Filter = nil
+	}
+
 	var queryPlanner *types.Document
 	err = dbPool.InTransaction(ctx, func(tx pgx.Tx) error {
 		var err error

diff --git a/internal/handlers/pg/pg.go b/internal/handlers/pg/pg.go
@@ -42,11 +42,12 @@ type Handler struct {
 
 // NewOpts represents handler configuration.
 type NewOpts struct {
-	PostgreSQLURL   string
-	DisablePushdown bool
+	PostgreSQLURL string
+
 	L               *zap.Logger
 	Metrics         *connmetrics.ConnMetrics
 	StateProvider   *state.Provider
+	DisablePushdown bool
 }
 
 // New returns a new handler.

diff --git a/internal/handlers/pg/pgdb/query.go b/internal/handlers/pg/pgdb/query.go
@@ -42,12 +42,11 @@ type FetchedDocs struct {
 // QueryParam represents options/parameters used for SQL query.
 type QueryParam struct {
 	// Query filter for possible pushdown; may be ignored in part or entirely.
-	Filter          *types.Document
-	DB              string
-	Collection      string
-	Comment         string
-	Explain         bool
-	DisablePushdown bool
+	Filter     *types.Document
+	DB         string
+	Collection string
+	Comment    string
+	Explain    bool
 }
 
 // Explain returns SQL EXPLAIN results for given query parameters.
@@ -84,14 +83,8 @@ func Explain(ctx context.Context, tx pgx.Tx, qp *QueryParam) (*types.Document, e
 
 	query += ` FROM ` + pgx.Identifier{qp.DB, table}.Sanitize()
 
-	var args []any
-
-	if qp.Filter != nil && !qp.DisablePushdown {
-		var where string
-
-		where, args = prepareWhereClause(qp.Filter)
-		query += where
-	}
+	where, args := prepareWhereClause(qp.Filter)
+	query += where
 
 	rows, err := tx.Query(ctx, query, args...)
 	if err != nil {
@@ -189,12 +182,11 @@ func queryById(ctx context.Context, tx pgx.Tx, schema, table string, id any) (*t
 
 // iteratorParams contains parameters for building an iterator.
 type iteratorParams struct {
-	schema          string
-	table           string
-	comment         string
-	explain         bool
-	disablePushdown bool
-	filter          *types.Document
+	schema  string
+	table   string
+	comment string
+	explain bool
+	filter  *types.Document
 }
 
 // buildIterator returns an iterator to fetch documents for given iteratorParams.
@@ -207,7 +199,7 @@ func buildIterator(ctx context.Context, tx pgx.Tx, p *iteratorParams) (iterator.
 
 	query += `SELECT _jsonb `
 
-	if c := p.comment; c != "" && !p.disablePushdown {
+	if c := p.comment; c != "" {
 		// prevent SQL injections
 		c = strings.ReplaceAll(c, "/*", "/ *")
 		c = strings.ReplaceAll(c, "*/", "* /")
@@ -217,14 +209,8 @@ func buildIterator(ctx context.Context, tx pgx.Tx, p *iteratorParams) (iterator.
 
 	query += ` FROM ` + pgx.Identifier{p.schema, p.table}.Sanitize()
 
-	var args []any
-
-	if p.filter != nil && !p.disablePushdown {
-		var where string
-
-		where, args = prepareWhereClause(p.filter)
-		query += where
-	}
+	where, args := prepareWhereClause(p.filter)
+	query += where
 
 	rows, err := tx.Query(ctx, query, args...)
 	if err != nil {

diff --git a/internal/handlers/registry/registry.go b/internal/handlers/registry/registry.go
@@ -44,7 +44,7 @@ type NewHandlerOpts struct {
 	Logger          *zap.Logger
 	Metrics         *connmetrics.ConnMetrics
 	StateProvider   *state.Provider
-	DisablePushdown bool // defaults to false
+	DisablePushdown bool
 
 	// for `pg` handler
 	PostgreSQLURL string
@@ -85,7 +85,8 @@ func init() {
 
 	registry["pg"] = func(opts *NewHandlerOpts) (handlers.Interface, error) {
 		handlerOpts := &pg.NewOpts{
-			PostgreSQLURL:   opts.PostgreSQLURL,
+			PostgreSQLURL: opts.PostgreSQLURL,
+
 			L:               opts.Logger,
 			Metrics:         opts.Metrics,
 			StateProvider:   opts.StateProvider,

diff --git a/internal/handlers/registry/tigris.go b/internal/handlers/registry/tigris.go
@@ -25,13 +25,15 @@ import (
 func init() {
 	registry["tigris"] = func(opts *NewHandlerOpts) (handlers.Interface, error) {
 		handlerOpts := &tigris.NewOpts{
-			ClientID:      opts.TigrisClientID,
-			ClientSecret:  opts.TigrisClientSecret,
-			Token:         opts.TigrisToken,
-			URL:           opts.TigrisURL,
-			L:             opts.Logger,
-			Metrics:       opts.Metrics,
-			StateProvider: opts.StateProvider,
+			ClientID:     opts.TigrisClientID,
+			ClientSecret: opts.TigrisClientSecret,
+			Token:        opts.TigrisToken,
+			URL:          opts.TigrisURL,
+
+			L:               opts.Logger,
+			Metrics:         opts.Metrics,
+			StateProvider:   opts.StateProvider,
+			DisablePushdown: opts.DisablePushdown,
 		}
 		return tigris.New(handlerOpts)
 	}

diff --git a/internal/handlers/tigris/msg_explain.go b/internal/handlers/tigris/msg_explain.go
@@ -60,11 +60,12 @@ func (h *Handler) MsgExplain(ctx context.Context, msg *wire.OpMsg) (*wire.OpMsg,
 		return nil, lazyerrors.Error(err)
 	}
 
-	var queryFilter string
-	if !h.DisablePushdown {
-		queryFilter = string(tigrisdb.BuildFilter(filter))
+	if h.DisablePushdown {
+		filter = nil
 	}
 
+	queryFilter := string(tigrisdb.BuildFilter(filter))
+
 	queryPlanner := must.NotFail(types.NewDocument(
 		"Filter", queryFilter,
 	))

diff --git a/internal/handlers/tigris/tigris.go b/internal/handlers/tigris/tigris.go
@@ -32,14 +32,15 @@ import (
 
 // NewOpts represents handler configuration.
 type NewOpts struct {
-	ClientID        string
-	ClientSecret    string
-	Token           string
-	URL             string
-	DisablePushdown bool
+	ClientID     string
+	ClientSecret string
+	Token        string
+	URL          string
+
 	L               *zap.Logger
 	Metrics         *connmetrics.ConnMetrics
 	StateProvider   *state.Provider
+	DisablePushdown bool
 }
 
 // Handler implements handlers.Interface on top of Tigris.

diff --git a/internal/handlers/tigris/tigrisdb/query.go b/internal/handlers/tigris/tigrisdb/query.go
@@ -34,10 +34,9 @@ import (
 // QueryParam represents options/parameters used by the fetch/query.
 type QueryParam struct {
 	// Query filter for possible pushdown; may be ignored in part or entirely.
-	Filter          *types.Document
-	DB              string
-	Collection      string
-	DisablePushdown bool
+	Filter     *types.Document
+	DB         string
+	Collection string
 }
 
 // QueryDocuments fetches documents from the given collection.
@@ -68,10 +67,7 @@ func (tdb *TigrisDB) QueryDocuments(ctx context.Context, param *QueryParam) (ite
 		return nil, lazyerrors.Error(err)
 	}
 
-	var filter driver.Filter
-	if !param.DisablePushdown {
-		filter = BuildFilter(param.Filter)
-	}
+	filter := BuildFilter(param.Filter)
 
 	tdb.l.Sugar().Debugf("Read filter: %s", filter)