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 argument for AS OF SYSTEM TIME #30955

Open
RaduBerinde opened this issue Oct 4, 2018 · 8 comments
Open

sql: support placeholder argument for AS OF SYSTEM TIME #30955

RaduBerinde opened this issue Oct 4, 2018 · 8 comments
Labels
A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) docs-done docs-known-limitation E-quick-win Likely to be a quick win for someone experienced. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-sql-queries SQL Queries Team

Comments

@RaduBerinde
Copy link
Member

RaduBerinde commented Oct 4, 2018

We don't support placeholders for AS OF SYSTEM TIME:

[email protected]:39921/defaultdb> prepare a as select 1 from t as of system time $1;
pq: AS OF SYSTEM TIME: only constant expressions are allowed

This means that the time value has to be embedded in the SQL string. Given that the time changes a lot, this does not play nicely with prepared statement reuse by drivers (and in the future, with caching plans). Ideally, we should support a placeholder here.

Semantically this is a bit dubious, given that the table schemas (and even existence) can vary with time. We would require that the statement parses correctly with the state of the world at prepare time (in other words, we prepare "as of now").

If we fix this, backporting to 2.1 would be a good idea, to avoid proliferation of the undesirable usage.

Note: @mjibson points out that in some cases, a relative time can be used as a workaround, eg AS OF SYSTEM TIME '-1m'.

CC @knz

Jira issue: CRDB-4816

@RaduBerinde RaduBerinde added this to the 2.2 milestone Oct 4, 2018
@RaduBerinde RaduBerinde added the A-sql-optimizer SQL logical planning and optimizations. label Oct 4, 2018
@knz knz added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Oct 13, 2018
@knz
Copy link
Contributor

knz commented Apr 19, 2021

For additional context:

  • one proximate issue is that we restrict the parsing to constant literals. To accept placeholders, we'd need to extend this.
  • we cannot extend the parsing to arbitrary scalar expressions, because these may contain subqueries or other things that necessitate a valid txn object.
  • we could extend the parsing to "EITHER literal OR placeholder"

if we considered that last avenue, we'd then hit another followup snag. The handling of placeholders inside a scalar expression is done as part of the general type checking logic. This also requires a valid current txn object.

So in order to move forward here, we'd need to:

  • accept a placeholder alongside literals
  • do not store the placeholder as an Expr, and instead store it as a specific thing
  • add special placeholder insertion logic in the handling of AOST that detects when the parameter is a placeholder specifically, and pulls the value from the pgwire-level placeholder values provided by the client.

@knz knz removed this from the 19.1 milestone Apr 19, 2021
@ajwerner
Copy link
Contributor

do not store the placeholder as an Expr, and instead store it as a specific thing

I may be misunderstanding. The code we have today for dealing with AS OF SYSTEM TIME is already working in Expr terms. If the placeholder can be substituted with an expr here then it might just work?

The below tree.AsOfClause could just as well be tree.Expr.

// EvalAsOfTimestamp evaluates the timestamp argument to an AS OF SYSTEM TIME query.
func EvalAsOfTimestamp(
ctx context.Context, asOf AsOfClause, semaCtx *SemaContext, evalCtx *EvalContext,
) (tsss hlc.Timestamp, err error) {

@ajwerner
Copy link
Contributor

I think the lift here isn't too crazy. Firstly, we have a semaCtx inside EvalAsOfTimesamp. We can just use its value, if we have one, to populate the timestamp. The biggest lift is about detecting when, during prepare, we do not have a value but we do have a placeholder and teaching the memo to track that fact. We'd also need to properly detect that we can't reuse the memo in the case that it's stale.

I'm not saying this is a trivial change by any stretch, but I'm not sure how many new concepts we really need.

@rytaft rytaft added the E-quick-win Likely to be a quick win for someone experienced. label Apr 27, 2021
@rafiss rafiss added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label May 12, 2021
@jlinder jlinder added the T-sql-queries SQL Queries Team label Jun 16, 2021
@exalate-issue-sync exalate-issue-sync bot removed the T-sql-queries SQL Queries Team label Aug 4, 2022
@leiffoged
Copy link

+1 to this -- this is the main thing that's preventing our use of prepared statements

@rafiss
Copy link
Collaborator

rafiss commented Aug 22, 2022

Another note if we implement this: Right now, since the argument is a constant expression, we use that constant as the timestamp to use when preparing the statement. A suggestion was made above that we could instead prepare the statement as of "now." However, that doesn't work well with the pgwire extended protocol, which says that a sequence of Parse (aka Prepare), Bind, Execute, then Sync commands should all be executed as part of the same implicit transaction. We aren't able to change the transaction timestamp after it reads data, so the idea of using "now" as the timestamp when preparing, but using the resolved placeholder value when executing does not work.

@russoj88
Copy link

Here's a use case I was hoping to use prepared statements and AS OF SYSTEM TIME for. Hopefully the context helps.

Server API

The API serves a simple paged search. Requests pass in a timestamp ($2), and responses return one as well. The idea is to use AS OF SYSTEM TIME to tie the results to a certain time, preventing duplicates on multiple pages.

SELECT id
FROM data_table
     AS OF SYSTEM TIME $2
ORDER BY id
LIMIT 10 OFFSET $3

Client

The client passes the timestamp in all search requests (updating the timestamp as the server's responses do).

@ddrao
Copy link

ddrao commented Feb 9, 2024

Plus one. Time travel queries with bound parameters are absolutely necessary. On the client side the postgres driver cannot use prepared statements and spends a lot of time in parsing the query every single time as a result.

@yuzefovich yuzefovich added the T-sql-queries SQL Queries Team label May 2, 2024
@jklamer
Copy link

jklamer commented Aug 6, 2024

Dropping in to comment my support for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-optimizer SQL logical planning and optimizations. C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) docs-done docs-known-limitation E-quick-win Likely to be a quick win for someone experienced. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) T-sql-queries SQL Queries Team
Projects
Status: Backlog
Development

No branches or pull requests