lib / pq

Pure Go Postgres driver for database/sql
https://pkg.go.dev/github.com/lib/pq
MIT License
8.87k stars 907 forks source link

Implement Named Parameters #534

Open itsjamie opened 7 years ago

itsjamie commented 7 years ago

The addition of named parameters support in Go 1.8 is documented in #531, tracking this individual change here.

Notes from the document:

Drivers should be clear in their documentation what form of names they accept. 
For example, does the driver support "@ID", "?ID", ":ID", "$ID", "ID". If no 
placeholder is specified and the query language requires one, will the driver 
prepend the default symbol to the name if none are found? This behavior 
should be documented in the driver.

If mixing named parameters with ordinal position, the ordinal position is always 
assigned to, values don't skip named parameter positions. 
For example: "... where ID = :ID and Name = ?", the second placeholder ("?") 
will be ordinal position 2.

I would like to implement this in lib/pq, but figured some discussion would be good. For example, should lib/pq require a specific prefix? I would imagine no because they will be reduced down to $1 and $2 ordinal operators.

I would still want to implement named parameters though so that any shared names in a query can be automatically reduced to the same ordinal parameter.

Thoughts?

nathany commented 7 years ago

Looks like PostgreSQL doesn't have any special support for named parameters in prepared statements, so is the idea that this would be implemented entirely in the driver?

sql.Named("Name", value) substitutes :Name for $1 in SQL, which receives the value.

Not 100% sure if it's worthwhile at the driver level, since it can also be done at a higher level: http://jmoiron.github.io/sqlx/#namedParams

(But if so, that implementation could be worth looking at)

PostgreSQL does have a named parameter notation when calling functions: https://www.postgresql.org/docs/current/static/sql-syntax-calling-funcs.html a => 'Hello' this syntax appears to be 9.5+ a := 'Hello' this syntax is still supported

Not sure if it provides any benefit over passing $1 over to PostgreSQL if the SQL sent to the driver looks the same either way.

cbandy commented 7 years ago

should lib/pq require a specific prefix? I would imagine no because they will be reduced down to $1 and $2 ordinal operators.

I don't understand what you're asking here.

I would still want to implement named parameters though so that any shared names in a query can be automatically reduced to the same ordinal parameter.

The parameter symbols/markers $n can be used more than once in a query to refer to the same parameter value/argument. Is that what you mean by "shared?" Perhaps I'm misunderstanding what you desire here.

For example, the following query multiplies a number by three:

err := db.QueryRow(`SELECT $1 + $1 + $1`, x).Scan(&y)
itsjamie commented 7 years ago

When speaking to the prefix, I was referencing the google document for database/sql 1.8 changes.

Specifically, there was a section (appears to me gone/edited) that I quoted from the above document in the linked ticket which appeared to suggest the driver pick a prefix and only accept that.

I was initially thinking about just supporting the database/sql changes to ensure it made Go 1.8. Now, particularly it's just a case of options. Also, with named parameters, the driver can do a little grunt work and deduplicate the parameters to the minimal number of ordinal arguments. reducing the query size it might be worthwhile?

Example:

err := db.QueryRow(`SELECT :Num + :Num + :Num`, sql.Named("Num", x)).Scan(&y)

The above should result in the same query in your example, just the driver would dedupe the references and mark them all as the same ordinal argument.

Thanks for your time.

nathany commented 7 years ago

@itsjamie That deduping makes sense to me, and how I would expect it to work. I'm just an onlooker though, not the project lead.

dewey4iv commented 7 years ago

I'd really like to see this supported in the driver. @itsjamie has there been any work/plans to see this worked on?

Re: the de-duping - I agree with @nathany - the logic seems sound to me.

itsjamie commented 7 years ago

I haven't planned anything around it, was waiting for someone to give the OK on implementation before starting any work.

cbandy commented 7 years ago

It seems to me that the only safe way to implement this is with a parser that can handle all forms of quoting. Without it, any prefix we pick will break an existing app out there.

We could skimp on the parser by making the user specify a prefix. In that case, using a literal containing their prefix is their fault. No configured prefix could mean no named arguments. At the same time, configuring lib/pq while staying compatible with libpq connection strings has its challenges.

Re: de-duping. I see why I was confused. In your example the driver receives only one NamedValue. From my perspective there's nothing to reduce/consolidate. Are there any guidelines for how a driver should behave if two NamedValue arrive with the same Name? Perhaps database/sql prevents it? What about a Name that isn't present in the SQL?

GeertJohan commented 7 years ago

Is this really something that the driver should do? I think sqlx handles this quite well. To me it doesn't feel like this should be handled at the driver level as it's not a postgres feature.. http://jmoiron.github.io/sqlx/#namedParams

dewey4iv commented 7 years ago

@GeertJohan - Not everyone uses the sqlx package and I don't think they should be required to in order to have access to Go stdlib features. I agree, it isn't a Postgres feature, but it is a feature of database/sql as of Go 1.8.

A quick excerpt from the 1.8 release notes:

"The other features require driver support in database/sql/driver. Driver authors should review the new interfaces. Users of existing driver should review the driver documentation to see what it supports and any system specific documentation on each feature." https://golang.org/doc/go1.8

To me, at least, this clearly states that it is the responsibility of the driver to implement these features.

cbandy commented 7 years ago

To me, at least, this clearly states that it is the responsibility of the driver to implement these features.

I agree that a database/sql/driver is the only thing that can implement these database/sql features. My interpretation of the 1.8 release notes is:

  1. drivers may (or may not) implement any number of these features.
  2. drivers should document which features are (and are not) supported.
  3. users should read driver documentation.
itsjamie commented 7 years ago

I agree with your interpretation @cbandy. I submitted this issue because I feel it is likely the next generation of query builders for database/sql will likely use some combination of named parameter values and the typed column data to automate.

Also, when writing composable queries, such as using WITH ( subquery ) as temp UPDATE... it is error-prone to have to rewrite the subquery simply to rename the ordinal parameters, or remember to update the parameter number in the UPDATE query should the subquery change.

dewey4iv commented 7 years ago

@cbandy - I agree. My interpretation was assuming that you agreed to adding this feature. @itsjamie - that would be a nice feature to have!

buyology commented 7 years ago

any progress on this? :blush: 🚀

jeromedoucet commented 5 years ago

I will try to work on it. Working on a Golang backend with huge SQL query, such feature can help us a lot to have cleaner SQL code.

flibustenet commented 5 years ago

@jeromedoucet there is an issue in sqlx about named query that can help you https://github.com/jmoiron/sqlx/issues/206 with @detaoin proposing a sqltokenizer https://github.com/detaoin/sql2http/blob/master/sqltokenizer.go

I'm not sure it should be implemented in a driver, but if it will be i will use it !

detaoin commented 5 years ago

Hi @jeromedoucet The simple sqltokenizer @flibustenet mentions was implemented because I needed named parameters for various driver backends. And sqlx doesn't handle quoting and comments (it's a simple substring matching approach). However the code I wrote hasn't been deeply tested. (I think) I'm the only user for now. If you intend to use it, that be a great motivation for me to start writing some serious testing.

Let me know if I can help.

bmizerany commented 3 years ago

I too want this, but I also understand the concerns and hesitations around bringing it in. I have a workaround for now, and few ideas and would love feedback on:

Currently I use this workaround:

func prep(q string, args ...interface{}) (qq string, newArgs []interface{}) {
        // TODO(bmizerany): submit PR for lib/pq to support named params

        var keys []string // as strings.NewReplacer requires
        var vals []interface{}

        for i, a := range args {
                if na, ok := a.(sql.NamedArg); ok {
                        keys = append(keys, fmt.Sprintf("@%s", na.Name))
                        keys = append(keys, fmt.Sprintf("$%d", i+1))
                        vals = append(vals, na.Value)
                } else {
                        vals = append(vals, a) 
                }
        }
        q = strings.NewReplacer(keys...).Replace(q)
        return q, vals
} 

and use it like:

        qq, args := prep(q,
                sql.Named("before", before),
                sql.Named("topic", topic),
                sql.Named("n", n),
        )

        rows, err := m.db.QueryContext(ctx, qq, args...)
        if err != nil {
                return nil, err
        }

It's a bit crude and naive, but it works for basic use cases.

To land support in lib/pq I propose an incremental approach:

  1. Introduce lib/pqx: This will be an experimental driver that is a thin shell over lib/pq. The idea being it coerces queries into what lib/pq supports today while supporting sql.NamedArg.
  2. In lib/pqx support a new connection string option named=@. If present, Query, Exec, et al will play nicely with NamedArg. This allows for backwards compatibility and non-interference with other experimental features in pqx that may one day be introduced.
  3. If the community is accepting and we decided to move forward, we can port sql.NamedArg support into lib/pq after enough vetting and hardening in pqx.

I'm excited to hear any feedback!

-b