go-mysql-server icon indicating copy to clipboard operation
go-mysql-server copied to clipboard

Unbound variables in subqueries

Open paralin opened this issue 3 years ago • 15 comments

When using the "driver" package, I kept running into the following errors:

DEBU[0115] [6.873ms] [rows:-] SELECT count(*) FROM information_schema.tables WHERE table_schema = 'test-db' AND table_name = 'entries' AND table_type = 'BASE TABLE' 
DEBU[0115] [0.853ms] [rows:0] CREATE TABLE `entries` (`id` bigint AUTO_INCREMENT,`value` bigint,PRIMARY KEY (`id`)) 
ERRO[0115] unbound variable "v1" in query: [0.778ms] [rows:0] INSERT INTO `entries` (`value`,`id`) VALUES (4,1) 
ERRO[0115] unbound variable "v1" in query: [0.614ms] [rows:0] INSERT INTO `entries` (`value`,`id`) VALUES (10,2) 
ERRO[0115] unbound variable "v1" in query: [0.622ms] [rows:0] INSERT INTO `entries` (`value`,`id`) VALUES (30,3) 

The fix appears to be changing driver/value.go to add a "v" prefix to ordinal placeholder values:

                        name = "v"+strconv.FormatInt(int64(v.Ordinal), 10)

... because the SQL plan code appears to expect "v1" "v2" and so on.

paralin avatar Apr 23 '21 12:04 paralin

@paralin,

Can you provide a short program that reproduces this problem? We have end-to-end tests of bind variables, but it seems like we are missing something.

Thanks!

Zach

zachmu avatar May 15 '21 22:05 zachmu

@zachmu no problem, will try to split things up and get a reproduce

paralin avatar May 16 '21 23:05 paralin

@zachmu I still don't have a reproduce but this commit fixes it 100%: https://github.com/paralin/go-mysql-server/commit/2fe0a7536bc52ea5c6b2b921d77392aa4307b231

paralin avatar May 25 '21 03:05 paralin

Running into this as well, but only in my storage method that performs a subquery.

The query:

SELECT * FROM test WHERE namespace = ? AND id IN (SELECT test_id FROM test_members WHERE user_id = ?) ORDER BY id ASC LIMIT 51

The error:

unbound variable "v2" in query

I can 't say for certain that the subquery is responsible, will try to investigate in the next 24h. I do have other queries with multiple bindvars that are running properly.

bojanz avatar Oct 06 '21 21:10 bojanz

subquery with bindvar has same error run with _example/main.go

	db, err := sql.Open("mysql", "root@tcp(localhost:3306)/mydb")
	if err != nil {
		panic(err)
	}
	var name string
	err = db.QueryRow("select exists(select name from mytable where name=?)", "John Doe").Scan(&name)
	if err != nil {
		panic(err)
	}
	fmt.Println(name)
Error 1105: unbound variable "v1" in query

okhowang avatar Jan 13 '22 15:01 okhowang

Seems likely that bindvars are not propagating to subqueries.

@max-hoffman, please investigate as part of your work on prepared query optimization

zachmu avatar Feb 24 '22 18:02 zachmu

Met the same error.

darluc avatar Mar 12 '22 16:03 darluc

@max-hoffman Have you made any progress here? Seems like a real popular bug :-)

timsehn avatar Mar 12 '22 16:03 timsehn

This PR https://github.com/dolthub/go-mysql-server/pull/795 implements prepared statements and doubles our testing coverage for BindVars, including subqueries. Hopefully we will merge and release in the next week.

max-hoffman avatar Mar 12 '22 16:03 max-hoffman

@max-hoffman Thanks again for all your work on this.

paralin avatar Mar 13 '22 01:03 paralin

This PR #795 implements prepared statements and doubles our testing coverage for BindVars, including subqueries. Hopefully we will merge and release in the next week.

Max, let's address this bug separately from the above PR. Bindvars not propagating to subqueries is a very specific issue that has nothing to do with saving the query plan for efficiency between executions.

zachmu avatar Mar 14 '22 20:03 zachmu

cherry picked subquery specific fixes in this PR: https://github.com/dolthub/go-mysql-server/pull/871

max-hoffman avatar Mar 14 '22 22:03 max-hoffman

@max-hoffman I still have the issue on "main":

[rows:0] CREATE TABLE `entries` (`id` bigint AUTO_INCREMENT,`value` bigint,PRIMARY KEY (`id`)) 
unbound variable "v1" in query: [0.400ms] [rows:0] INSERT INTO `entries` (`value`,`id`) VALUES (4,1) 
unbound variable "v1" in query: [0.265ms] [rows:0] INSERT INTO `entries` (`value`,`id`) VALUES (10,2) 
unbound variable "v1" in query: [0.265ms] [rows:0] INSERT INTO `entries` (`value`,`id`) VALUES (30,3) 

The issue is resolved if I apply this patch:

From d18204a140a5ba96b18c381af3c5323234f45bd7 Mon Sep 17 00:00:00 2001
From: Christian Stewart <[email protected]>
Date: Fri, 23 Apr 2021 05:51:24 -0700
Subject: [PATCH] fix(driver): add v prefix to ordinal values

Signed-off-by: Christian Stewart <[email protected]>
---
 driver/value.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/driver/value.go b/driver/value.go
index 4f0e1fdf..8dc4dad4 100644
--- a/driver/value.go
+++ b/driver/value.go
@@ -93,7 +93,7 @@ func namedValuesToBindings(v []driver.NamedValue) (map[string]sql.Expression, er
 	for _, v := range v {
 		name := v.Name
 		if name == "" {
-			name = strconv.FormatInt(int64(v.Ordinal), 10)
+			name = "v" + strconv.FormatInt(int64(v.Ordinal), 10)
 		}
 
 		b[name], err = valueToExpr(v.Value)
-- 
2.35.1

paralin avatar Mar 15 '22 07:03 paralin

@paralin Can you share more info for how to reproduce the error? This seems like it deserves a new issue.

Do you ComPrepare INSERT INTO INSERT INTO `entries` (`value`,`id`) VALUES (?,?), and then exec with row values?

max-hoffman avatar Mar 15 '22 13:03 max-hoffman

... and in cases where I'm passing binding values (I guess the case that fails that we're talking about here)

DriverProvider uses sql.NewDatabaseProvider to build the provider, then return it to gdriver.Provider.

Am using the full database/sql stack:

import (
	"context"
	"database/sql"

	gdriver "github.com/dolthub/go-mysql-server/driver"
)

// NewSqlDriver constructs a sql driver from a transaction.
func NewSqlDriver(tx *Tx, driverOpts *gdriver.Options) *gdriver.Driver {
	provider := NewDriverProvider(tx)
	return gdriver.New(provider, driverOpts)
}

// NewSqlDb opens the sql database driver.
func NewSqlDb(tx *Tx) (*sql.DB, error) {
	driver := NewSqlDriver(tx, &gdriver.Options{})
	// as of writing this: the dsn parsing only allows for overriding jsonAs
	var dsn string
	conn, err := driver.OpenConnector(dsn)
	if err != nil {
		return nil, err
	}
	return sql.OpenDB(conn), nil
}

... I'll try to make a reproduce repo here soon, sorry for the random code snippets.

paralin avatar Mar 21 '22 14:03 paralin

I think we have pretty good confidence that bindvars are being correctly propagated everywhere we know of now. If you find specific cases where this isn't true, please open another issue.

zachmu avatar Oct 06 '22 22:10 zachmu

@zachmu Thanks!

paralin avatar Oct 06 '22 22:10 paralin