Let virtual table implementations know which SQL string is being executed #87

lucasvr · 2022-12-12T19:36:34Z

SQLite provides an interface to expose different data sources (such as files or other database systems) as virtual tables. There are several callbacks one must implement to enable e.g., connection handling and pagination, but one key attribute is missing on all callbacks: the query string being executed by the application.

The query string is especially relevant when virtual tables are used to mirror data from external databases hosted on different machines. A virtual table implementation that knows about the SQL string is able to reduce the amount of data transferred over the network when an application is only concerned about a subset of the columns.

This patch adds a new opcode VPrepareSql that's called immediately before VFilter when dispatching queries to a virtual table. The new opcode triggers the execution of the new xPrepareSql method which provides the SQL text to the virtual table implementation.

I am open to suggestions regarding the implementation approach and the new method's name. I'm looking forward to reading your feedback. Thanks!

This commit adds a new method named xPrepareSql to sqlite3_module and an associated opcode VPrepareSql. That opcode is emitted immediately before a VFilter opcode when querying virtual tables. xPrepareSql takes two arguments: a pointer to the virtual table's cursor and, missing from existing sqlite3_module methods, the SQL string being executed by the application. Prior to the introduction of this method, a virtual table that mirrored a remote table had no way to tell which columns of the remote server were requested by the application. Therefore, such an implementation would typically `SELECT *` from the remote table and let callbacks such as xColumn() and xNext() decide which data to send back to the application. The problem with this approach is that queries that SELECT just a subset of the remote columns trigger the transfer of data over the network even though they won't be used. xPrepareSql gives an opportunity for the virtual table implementation to inspect the query string and selectively choose which columns from the remote server to pull and cache locally. Because this change introduces a new method to sqlite3_module, new modules wishing to implement a callback for xPrepareSql must declare a module version (iVersion) 4 or above.

psarna

Looks great! Left a few minor comments.

psarna · 2022-12-12T19:55:28Z

src/sqlite.h.in

@@ -7076,6 +7076,8 @@ struct sqlite3_module {
  /* The methods above are in versions 1 and 2 of the sqlite_module object.
  ** Those below are for version 3 and greater. */
  int (*xShadowName)(const char*);
+  /** The methods below are for version 4 and greater. */


That's going to get ambiguous if/when SQLite adds some other field and bump the current version to 4, and we'd like to keep libSQL compatible for as long as possible. There's no perfect solution here, but in an (unmerged yet) pull request I used version "700", to indicate that it's a libSQL extension, and it makes the collision less likely to happen in the next few years. Perhaps the same trick could be applied here?

That makes sense. HDF5 adopts a similar approach when dealing with I/O filters: a certain range of numbers is reserved for filters contributed by the community. I will follow your suggestion and will check for iVersion >= 700.

psarna · 2022-12-12T19:58:43Z

src/vdbe.c

+    }
+  }
+
+  if( rc ) goto abort_due_to_error;


this line looks redundant - it's already checked in line 8101, and line 8100 is the only one in this block that assigns to rc

Oops, indeed. Thanks for spotting that!

psarna · 2022-12-12T19:59:37Z

src/vdbe.c

+   if( pModule->iVersion>=4 ){
+    if( pModule->xPrepareSql && p->zSql ){
+      rc = pModule->xPrepareSql(pVCur, p->zSql);
+      if( rc ) goto abort_due_to_error; // TODO set and test error msg


could you elaborate on the TODO? As long as the implementation of xPrepareSql assigns a proper error code, it will be properly handled and printed in the code that follows abort_due_to_error.

That was a reminder to test the error path, but I ended up leaving it behind by mistake. I will remove it.

psarna · 2022-12-12T20:08:53Z

src/vdbe.c

+
+  /* Invoke the xPrepareSql method */
+   if( pModule->iVersion>=4 ){
+    if( pModule->xPrepareSql && p->zSql ){


Question - in which cases is p->zSql null? I wonder if the responsibility for checking and handling NULL shouldn't be pushed to the implementation of xPrepareSql - perhaps somebody would like to do something specific even for cursors that don't have the SQL text?

This is a genuine question, as I haven't implemented any virtual SQLite tables in my life (: if it doesn't make sense, then feel free to leave the code as is.

I have added that as a sanity check. A NULL value for p->zsql would be possible when handling the Init or Trace opcodes, but that is not expected when processing a valid SQL statement.

psarna · 2022-12-19T11:09:03Z

@lucasvr just to prevent this PR from going stale - would you like us to take over and apply the fixes or would you prefer working on v2 yourself?

lucasvr · 2022-12-19T12:15:57Z

@lucasvr just to prevent this PR from going stale - would you like us to take over and apply the fixes or would you prefer working on v2 yourself?

Apologies for the late response! Let me work on v2 - I will send you an updated PR in the next few hours. Also, I appreciate your review and comments -- thanks for that!

This commit also removes a comment that was left over on the previous changeset.

@psarna

Following @psarna's advice, use iVersion >= 700 for community-contributed methods.

This name change is motivated to prevent misunderstandings: the callback is not meant to output a prepared SQL statement. Rather, the callback is being informed of the SQL statement prepared by SQLite.

lucasvr · 2022-12-19T17:34:06Z

@psarna I've added 2 commits on top of the original one that address your comments. I have also introduced a name change: the method is now calledPreparedSql (as opposed to PrepareSql) to prevent misunderstandings about what the callback is supposed to do. Thanks again for your prompt review.

psarna

Thanks! Looks good to me. @penberg could you take a cursory look just to have a second pair of eyes on it? And then we can go ahead and merge

penberg · 2022-12-21T12:54:52Z

Merged, thanks! @lucasvr @psarna perhaps a note about this in doc/libsql_extensions.md is in order?

asg017 · 2022-12-21T17:50:09Z

For reference, the current SQLite vtab implementation does already give information about how the table is being queried, in the xBestIndex method.

Specifically, the sqlite3_index_info.colUsed field states which columns are requested (ex "only these 2 out of 10 columns are requested"), which can be used for projection pushdowns. Also the sqlite3_index_info.aConstraint[] fields state all the WHERE filters that are applied to the query (ex "only returns rows with created_at >= '2022-01-01'"), which can be use for predicate pushdowns.

It's not the friendliest API, but I find dealing with those fields much easier than parsing raw SQL. It also doesn't handle GROUP BY aggregates well, ORDER BY is awkward, and only the first 63 columns are mentioned in colUsed.

lucasvr · 2022-12-21T17:58:54Z

@asg017 xBestIndex is indeed helpful. In my particular case I wanted to use SQLite as a trampoline to an external database, so forwarding the raw SQL query string made sense. I wasn't aware of the limitations of xBestIndex that you've mentioned, though -- thanks for sharing!

lucasvr · 2022-12-21T17:59:35Z

Merged, thanks! @lucasvr @psarna perhaps a note about this in doc/libsql_extensions.md is in order?

Will do. I'll send a new PR soon.

Enhance documentation by describing the new xPreparedSql callback for virtual tables. Refs tursodatabase#87

Enhance documentation by describing the new xPreparedSql callback for virtual tables. Refs #87

Enhance documentation by describing the new xPreparedSql callback for virtual tables. Refs tursodatabase#87

Fix data row serialization for the PostgreSQL wire protocol: - Send column values as part of a data row instead a data row per column. - Fix column name and type information in row description. Refs #87

87: box parser r=psarna a=MarinPostma This pr puts the parser on the heap. It's quite chunky, and caused stackoverflows in the past. I'm working to get a nicer patch to upstream lemon-rs, but it requires more work for somilar effects. I also remove the dependency on my patch Co-authored-by: ad hoc <postma.marin@protonmail.com>

psarna self-requested a review December 12, 2022 19:47

psarna reviewed Dec 12, 2022

View reviewed changes

Abdur-rahmaanJ mentioned this pull request Dec 13, 2022

Extra Mode branch #88

Open

lucasvr added 3 commits December 19, 2022 11:42

vdbeExec: remove redundant goto on error path.

86826ae

This commit also removes a comment that was left over on the previous changeset.

xPrepareSql: require iVersion >= 700

9e10fd8

Following @psarna's advice, use iVersion >= 700 for community-contributed methods.

Rename method from PrepareSql to PreparedSql

25e2204

This name change is motivated to prevent misunderstandings: the callback is not meant to output a prepared SQL statement. Rather, the callback is being informed of the SQL statement prepared by SQLite.

psarna approved these changes Dec 20, 2022

View reviewed changes

penberg merged commit 7beaa40 into tursodatabase:main Dec 21, 2022

lucasvr added a commit to lucasvr/libsql that referenced this pull request Dec 22, 2022

Document xPreparedSql virtual table callback

f946e39

Enhance documentation by describing the new xPreparedSql callback for virtual tables. Refs tursodatabase#87

lucasvr mentioned this pull request Dec 22, 2022

Document xPreparedSql virtual table callback #93

Merged

psarna pushed a commit that referenced this pull request Dec 23, 2022

Document xPreparedSql virtual table callback (#93)

4aeac89

Enhance documentation by describing the new xPreparedSql callback for virtual tables. Refs #87

MarinPostma pushed a commit to MarinPostma/libsql that referenced this pull request Jan 2, 2023

Document xPreparedSql virtual table callback (tursodatabase#93)

eb8f152

Enhance documentation by describing the new xPreparedSql callback for virtual tables. Refs tursodatabase#87

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let virtual table implementations know which SQL string is being executed #87

Let virtual table implementations know which SQL string is being executed #87

lucasvr commented Dec 12, 2022

psarna left a comment

psarna Dec 12, 2022

lucasvr Dec 19, 2022

psarna Dec 12, 2022

lucasvr Dec 19, 2022

psarna Dec 12, 2022

lucasvr Dec 19, 2022

psarna Dec 12, 2022

lucasvr Dec 19, 2022

psarna commented Dec 19, 2022

lucasvr commented Dec 19, 2022

lucasvr commented Dec 19, 2022

psarna left a comment

penberg commented Dec 21, 2022

asg017 commented Dec 21, 2022

lucasvr commented Dec 21, 2022

lucasvr commented Dec 21, 2022

Let virtual table implementations know which SQL string is being executed #87

Let virtual table implementations know which SQL string is being executed #87

Conversation

lucasvr commented Dec 12, 2022

psarna left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psarna commented Dec 19, 2022

lucasvr commented Dec 19, 2022

lucasvr commented Dec 19, 2022

psarna left a comment

Choose a reason for hiding this comment

penberg commented Dec 21, 2022

asg017 commented Dec 21, 2022

lucasvr commented Dec 21, 2022

lucasvr commented Dec 21, 2022