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.

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

Unbound variables in subqueries #383

Closed
paralin opened this issue Apr 23, 2021 · 17 comments
Closed

Unbound variables in subqueries #383

paralin opened this issue Apr 23, 2021 · 17 comments
Assignees

Comments

@paralin
Copy link
Contributor

paralin commented Apr 23, 2021

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.

@zachmu
Copy link
Member

zachmu commented May 15, 2021

@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

@paralin
Copy link
Contributor Author

paralin commented May 16, 2021

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

@paralin
Copy link
Contributor Author

paralin commented May 25, 2021

@zachmu I still don't have a reproduce but this commit fixes it 100%: aperturerobotics@2fe0a75

@bojanz
Copy link

bojanz commented Oct 6, 2021

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.

@okhowang
Copy link
Contributor

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

@zachmu
Copy link
Member

zachmu commented Feb 24, 2022

Seems likely that bindvars are not propagating to subqueries.

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

@darluc
Copy link

darluc commented Mar 12, 2022

Met the same error.

@timsehn
Copy link
Contributor

timsehn commented Mar 12, 2022

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

@max-hoffman
Copy link
Contributor

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.

@paralin
Copy link
Contributor Author

paralin commented Mar 13, 2022

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

@zachmu zachmu changed the title Unbound variables when using the sql driver Unbound variables in subqueries Mar 14, 2022
@zachmu
Copy link
Member

zachmu commented Mar 14, 2022

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.

@max-hoffman
Copy link
Contributor

cherry picked subquery specific fixes in this PR: #871

@paralin
Copy link
Contributor Author

paralin commented Mar 15, 2022

@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 <christian@paral.in>
Date: Fri, 23 Apr 2021 05:51:24 -0700
Subject: [PATCH] fix(driver): add v prefix to ordinal values

Signed-off-by: Christian Stewart <christian@paral.in>
---
 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

@max-hoffman
Copy link
Contributor

@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?

@paralin
Copy link
Contributor Author

paralin commented Mar 21, 2022

... 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.

@zachmu
Copy link
Member

zachmu commented Oct 6, 2022

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 zachmu closed this as completed Oct 6, 2022
@paralin
Copy link
Contributor Author

paralin commented Oct 6, 2022

@zachmu Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants