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

Support for doltgres prepared statements #2239

Merged
merged 28 commits into from
Jan 10, 2024
Merged

Support for doltgres prepared statements #2239

merged 28 commits into from
Jan 10, 2024

Conversation

zachmu
Copy link
Member

@zachmu zachmu commented Jan 5, 2024

See dolthub/vitess#299 for new handler interface

This includes three main changes:

  1. Implements new ExtendedHandler methods for doltgres prepared statement support
  2. Refactors the handler and engine to pull out functionality common to MySQL and Postgres prepared statement execution paths
  3. Adds type information to BindVars where they can be implicitly recognized, as required for the Describe message in Doltgres. This tells the client the postgres types required for parameters in query strings before binding. Doltgres inspects the plan node to find bindvars and their types to return to the client.

I'm most interested in feedback on 3) before I go deeper on it. It's implemented only for insert values and comparison expressions (used in filters). A complete solution also requires support for UPDATE and probably some other expression types. If this approach is too hacky or is likely to break or interfere with other things, I want to know before continuing. If the latter, it would be pretty easy to introduce a bind context specific to this use case and not assign types unless it's set.

See dolthub/doltgresql#87 for how doltgres uses this new bindvar type information.

@max-hoffman
Copy link
Contributor

There is enough here that I understand what you're trying to do. My worry is how to make sure we can reliably expand the type inference going into the future. We can do a lot with comparison functions with one level deep bindvars, but manually pattern matching more complicated expression trees is brittle. For example, supporting arithmetic functions, case statements, subqueries,...etc are all supportable for single-nesting only. Unary functions can obfuscate bindvars in arbitrary complicated ways that hinder the one-level deep pattern matching (ex; (1 + (-$2)). Extending the current format to work in the general case looks like k! expressions walks to true-up bind var types, where k is the number of expressions with >1 children.

We probably need some sort of bottom-up info passing to know that a subtree contains a bindvar, and only trying to do true ups when we missed info.

The structure of this problem is similar to type coercion, which is the more general problem of validating expression types. Bindvar coercion for DML statements is the simplest case. We only need to pass top-down hints from the target columns through to bindvars. Resolving SELECT statements like SELECT * FROM test WHERE concat(s, '!') = $1 on the other hand requires two tree passes for type inference. We resolve types for the equals on a first pass. When we see an unresolved bindvar as a concat child, whose type we've successfully resolved, we trigger a second pass that coerces the bindvar to a string type to resolve all children. If we have an intermediate node with an unresolved bind var that itself is unresolved, we punt the problem to a higher level node that will hopefully figure out what type its subtree should be.

The problem with type coercion in general is that it inserts expression nodes into the tree in the process of resolving intermediate node types. A partial type inference that supports bindvar types would add functional type information to bindvars, but hides the same info on intermediate expressions in a way that is opaque to execution. The pros are that a type coercion framework is a precursor to fixing other type problems, and organizationally separates typing code from the rest of analysis.

@zachmu zachmu marked this pull request as ready for review January 10, 2024 21:21
@zachmu zachmu merged commit 66c569a into main Jan 10, 2024
7 checks passed
@Hydrocharged Hydrocharged deleted the zachmu/prepared branch February 7, 2024 13:45
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

Successfully merging this pull request may close these issues.

None yet

2 participants