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

sql: support placeholder in identifier and literal string positions #8270

Closed
knz opened this issue Aug 2, 2016 · 6 comments
Closed

sql: support placeholder in identifier and literal string positions #8270

knz opened this issue Aug 2, 2016 · 6 comments
Labels
A-sql-semantics C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@knz
Copy link
Contributor

knz commented Aug 2, 2016

e.g. CREATE TABLE ? (x INT) or INSERT INTO TABLE X (?, ?) VALUES (1, 2, 3)

So the starting observation is that we have this parser.QueryArguments map which is either empty during prepare (placeholders need to be typed, don't have a value yet) or full during execute. During lexing we can peek into this structure to determine what to do:

  • if the map is empty (we're in prepare); then upon encountering the syntax for a placeholder in identifier position, 1) return the token type PLACEHOLDER (as currently) and 2) pre-populate the PlaceholderTypes map with 'string' for the placeholder 3) during type checking, assume the type is unknown for anything that needs a schema derived from a table/column name for which there is a placeholder; many of our typing rules then become weaker but we can probably still work on the statement conservatively.
  • if the map is already pre-populated (we're in execute), then upon encountering the syntax for a placeholder in identifier position in the lexer, simply substitute the placeholder for its value and return the IDENT token type instead.
  • For CREATE USER statements (sql: add CREATE USER stmt and pgwire password-based authentication #9794), the grammar must be changed to support placeholders.

cc @mjibson

@knz
Copy link
Contributor Author

knz commented Aug 2, 2016

A possible drawback from this approach is that we'd need to double check the typing rules that we can't get conflicting typing between prepare and execute. To alleviate this we could also considering implementing this feature only after we start caching typing information between prepare and execute (another morcel on the roadmap)

@knz knz added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-sql-semantics labels Aug 2, 2016
@petermattis
Copy link
Collaborator

Some uses of placeholders for identifiers seem easier than others. For example, CREATE DATABASE $1 seems relatively straightforward because we can statically determine the type of the placeholder as string.

See also #4596 and #4597.

@maddyblue
Copy link
Contributor

I'm in favor of supporting ? for any name as long as we can resolve types for all placeholders at prepare time. I don't think we should support cases like SELECT * FROM ? WHERE a = ? for now.

@asubiotto asubiotto changed the title sql: support placeholder in identifier positions sql: support placeholder in identifier and literal string positions Oct 11, 2016
@petermattis petermattis added this to the Later milestone Feb 22, 2017
@petermattis petermattis removed this from the Later milestone Oct 5, 2018
@jordanlewis jordanlewis added the X-wontfix Closed as we're not going to fix it, even though it's a legitimate issue. label Apr 11, 2019
@maddyblue
Copy link
Contributor

Noooo this is my favorite feature I've wanted since like high school.

@jordanlewis
Copy link
Member

Heh, won't argue with that - I wish there was a way to leave an issue open as "a reasonable idea" without having that also mean "it's on a backlog".

@jordanlewis jordanlewis removed the X-wontfix Closed as we're not going to fix it, even though it's a legitimate issue. label Apr 11, 2019
@knz
Copy link
Contributor Author

knz commented Apr 12, 2019

That is what the C-wishlist label is for!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-semantics C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

4 participants