Skip to content

sql: support SQL declare (cursor) #41412

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

Closed
mattcrdb opened this issue Oct 7, 2019 · 23 comments
Closed

sql: support SQL declare (cursor) #41412

mattcrdb opened this issue Oct 7, 2019 · 23 comments
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-aws-dms Blocking support for AWS Database Migration Service A-tools-tableau C-wishlist A wishlist feature. docs-done docs-known-limitation T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-anchored-telemetry The issue number is anchored by telemetry references.

Comments

@mattcrdb
Copy link

mattcrdb commented Oct 7, 2019

Is your feature request related to a problem? Please describe.
A customer tried to use tableau to connect to cockroachDB, using this guide.

Tableau uses declare for its queries, CockroachDB does not currently support compatibility with SQL level declare.

Tableau can not be used.

Describe the solution you'd like
Support for the declare keyword.

postgres docs on declare.

Epic: CRDB-11397

@mattcrdb mattcrdb added the C-wishlist A wishlist feature. label Oct 7, 2019
@RoachietheSupportRoach
Copy link
Collaborator

Zendesk ticket #3853 has been linked to this issue.

@rafiss rafiss added the A-sql-pgcompat Semantic compatibility with PostgreSQL label Oct 18, 2019
@tfoldi
Copy link

tfoldi commented Dec 8, 2019

I can easily fix this by adding Cockroach as a supported database driver to Tableau. Only thing you have to do is to set params["UseDeclareFetch"] = "0"; in connectionBuilder.js in the Tableau TACO file.

If you're interested I can show how to do build a proper driver customization for Tableau...

@tfoldi
Copy link

tfoldi commented Dec 8, 2019

OK, I tested it and works like a charm. Code is here: https://github.com/tfoldi/tableau_cockroachdb_connector

@mattcrdb
Copy link
Author

mattcrdb commented Dec 9, 2019

cc @jordanlewis

@jordanlewis jordanlewis added the X-anchored-telemetry The issue number is anchored by telemetry references. label Apr 2, 2020
@otan
Copy link
Contributor

otan commented Jul 14, 2020

this is needed for QGIS support for PostGIS.

@jordanlewis
Copy link
Member

@otan, could you say more? Including links to sample queries or use cases would be very helpful.

@otan
Copy link
Contributor

otan commented Jul 15, 2020

QGIS uses DECLARE BINARY CURSORS cursors: https://github.com/qgis/QGIS/blob/b4c21fa44e25c23c9ee542f56f031b3de1a94fea/src/providers/postgres/qgspostgresconn.cpp#L1353

and then FETCH: https://github.com/qgis/QGIS/blob/b4c21fa44e25c23c9ee542f56f031b3de1a94fea/src/providers/postgres/qgspostgresconn.cpp#L1698

... all for the sake of deducing endianness :3

this is a startup sequence; i'm unable to open the tool without it, unfortunately.


QGIS is a widely used (and quite neat!) open source geospatial plotting tool as well.

@otan otan mentioned this issue Jul 28, 2020
2 tasks
@jordanlewis
Copy link
Member

@otan @rafiss would we be able to send a PR to qgis to use a simpler method of deducing endianness?

@otan
Copy link
Contributor

otan commented Aug 14, 2020

Maybe, but I found something perhaps even more useful -- the ogr2ogr tool requires DECLARE CURSOR to work so we can export from CRDB into other exotic data formats.

Modifying this code is hard because it works on a general cause, and it has no way to paginate results otherwise.

Code: https://github.com/OSGeo/gdal/blob/f031bd34baf9ab136637cb1e790c942aa947b828/gdal/ogr/ogrsf_frmts/pg/ogrpglayer.cpp#L1433

@jordanlewis
Copy link
Member

Note that there is more support required than just DECLARE CURSOR here.

FETCH is also required. FETCH has a few variants, including relative, absolute, and forward and backward fetches. The linked code significantly needs both forward relative and absolute fetches. Implementing absolute fetches is significant because it requires buffering all of the results of the cursor on the gateway, presumably in memory and then spilling to disk. Just wanted to say it is likely just the tip of the iceberg and we have to get a realistic estimate of the work required before we dive into this.

@otan would it be possible for you to run some of these tools against Postgres and collect a log of the statements produced? I would like to make sure we are not missing any other features. Significantly, if you can figure out whether "multiple active result sets" are used (aka - are there other SQL statements intereleaved in between the cursor FETCH statements on the same connection?) that would be very helpful for effort estimation.

@jordanlewis
Copy link
Member

I would also be curious to see the comprehensive list of tools we need to support for spatial, and of that list, which tools require cursor support. I continue to find it surprising that geospatial tools all need DECLARE CURSOR. Why would that be exactly? Is this a case of 2 tools (ogr2ogr and qgis) needing the feature making it seem like they all do? Do they actually all need cursors? And in the latter case, why is that exactly? Is it because they're old?

Sorry for all of the questions. I feel we need an improved understanding of the situation here to appropriately understand and prioritize this work.

@otan
Copy link
Contributor

otan commented Sep 16, 2020

@otan would it be possible for you to run some of these tools against Postgres and collect a log of the statements produced?

See logs: https://drive.google.com/drive/folders/1IQ8Y3VvsrcbEAaQku3d3vXJuPVcWHu5W?usp=sharing

@otan would it be possible for you to run some of these tools against Postgres and collect a log of the statements produced? I would like to make sure we are not missing any other features. Significantly, if you can figure out whether "multiple active result sets" are used (aka - are there other SQL statements intereleaved in between the cursor FETCH statements on the same connection?) that would be very helpful for effort estimation.

AFAICT from reading code (they're both open source), they do not require anything like these -- just lots of FETCHes to get lots of data.

@otan
Copy link
Contributor

otan commented Sep 16, 2020

I would also be curious to see the comprehensive list of tools we need to support for spatial, and of that list, which tools require cursor support. I continue to find it surprising that geospatial tools all need DECLARE CURSOR. Why would that be exactly? Is this a case of 2 tools (ogr2ogr and qgis) needing the feature making it seem like they all do? Do they actually all need cursors? And in the latter case, why is that exactly? Is it because they're old?

I'm not sure exactly why -- my guess is that it's easy to say "fetch me all data in a paginated manner" without having to read the schema and figure out exactly how it works.

@otan
Copy link
Contributor

otan commented Sep 16, 2020

I will also note ArcGIS also required DECLARE CURSOR -- I've included it in the folder.

@otan
Copy link
Contributor

otan commented Sep 16, 2020

I would also be curious to see the comprehensive list of tools we need to support for spatial, and of that list, which tools require cursor support. I continue to find it surprising that geospatial tools all need DECLARE CURSOR. Why would that be exactly? Is this a case of 2 tools (ogr2ogr and qgis) needing the feature making it seem like they all do? Do they actually all need cursors? And in the latter case, why is that exactly? Is it because they're old?

I don't think we've got a comprehensive list anywhere (@awoods187 do you?) -- these are ones that came up that seem popular in either import/export of data (ogr2ogr is the tool) or visualisations (ArcGIS is an extremely well known and used proprietary tool, QGIS is referenced and used heavily from visualisations I've seen on other websites).

@dilip2048
Copy link

OK, I tested it and works like a charm. Code is here: https://github.com/tfoldi/tableau_cockroachdb_connector

How do I install it to my tableau?

Just starting using tableau. I want to connect my cockraochDB to tableau and visualize the data on it.

@tfoldi
Copy link

tfoldi commented Sep 23, 2020

Just starting using tableau. I want to connect my cockraochDB to tableau and visualize the data on it.

The process is documented here: https://tableau.github.io/connector-plugin-sdk/docs/share

@xvaara
Copy link

xvaara commented Dec 23, 2020

Do you have plans to implement cursor support?

@otan
Copy link
Contributor

otan commented Jan 3, 2021

not yet @xvaara -- do you have a use case to share with us? this helps with prioritisation!

@FranckErnewein
Copy link

FranckErnewein commented Jan 11, 2021

Hi @otan,
We use tools (stitchdata.com) to synchronise our Postgres databases to our datawarehouse (aws redshift).
We started to migrate services on cockroach but we can not use stitchdata anymore because it scans source database using CURSOR to copy new/updated lines to Redshift.

I hope this will help to prioritize this feature

@xvaara
Copy link

xvaara commented Jan 12, 2021

I have many triggers, functions etc. so I can't use crdb. But I'd like to use it via postgres_fdw to sync instances and save versioning data. But I'd need to access that directly from postgresql. I'm thinking I'll try to use dblink if this still isn't on the roadmap. I asked about it somewhere few years ago.

@yob
Copy link

yob commented Jan 27, 2021

I also ran into this while experimenting with accessing cockroachdb via the postgres_fdw extension.

I was curious to see if it's possible to use FDW as part of the solution to incrementally migrating from postgres to cockroachdb. Possibly by creating a view that sits on top of the same table in postgres and cockroach, making them look like a single table to users of the database.

I followed the postgres_fdw setup docs and importing the cockroachdb table into postgres appeared to work, however querying it fails. Here's how it looks from the postgres side:

experiment=# \dE
             List of relations
 Schema | Name  |     Type      |  Owner   
--------+-------+---------------+----------
 public | users | foreign table | postgres

experiment=# \d+ users
                                                Foreign table "public.users"
 Column |       Type        | Collation | Nullable | Default |      FDW options      | Storage  | Stats target | Description 
--------+-------------------+-----------+----------+---------+-----------------------+----------+--------------+-------------
 id     | bigint            |           | not null |         | (column_name 'id')    | plain    |              | 
 uuid   | uuid              |           |          |         | (column_name 'uuid')  | plain    |              | 
 email  | character varying |           |          |         | (column_name 'email') | extended |              | 
 name   | character varying |           |          |         | (column_name 'name')  | extended |              | 
Server: cockroachdb
FDW options: (schema_name 'public', table_name 'users')

experiment=# select * from users limit 1;
ERROR:  at or near "declare": syntax error: unimplemented: this syntax
DETAIL:  source SQL:
DECLARE c1 CURSOR FOR
^
HINT:  You have attempted to use a feature that is not yet implemented.
See: https://go.crdb.dev/issue-v/41412/v20.2
CONTEXT:  remote SQL command: SELECT id, uuid, email, name FROM public.users LIMIT 1::bigint

@ajstorm ajstorm added the A-tools-aws-dms Blocking support for AWS Database Migration Service label Nov 22, 2021
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) A-aws-dms and removed A-tools-aws-dms Blocking support for AWS Database Migration Service labels Nov 25, 2021
@rafiss rafiss added A-tools-aws-dms Blocking support for AWS Database Migration Service A-tools-tableau and removed A-aws-dms labels Nov 29, 2021
@jordanlewis jordanlewis changed the title Support for SQL declare (cursor) sql: support for SQL declare (cursor) Dec 17, 2021
@jordanlewis jordanlewis changed the title sql: support for SQL declare (cursor) sql: support SQL declare (cursor) Dec 17, 2021
craig bot pushed a commit that referenced this issue Feb 21, 2022
74006: sql: add support for DECLARE, FETCH, CLOSE CURSOR using internal executors r=jordanlewis a=jordanlewis

Touches #41412.

This PR implements the SQL CURSOR methods DECLARE, FETCH and CLOSE using streaming internal executors.

Here's the Postgres docs on DECLARE: https://www.postgresql.org/docs/current/sql-declare.html

Things it implements:

- DECLARE for forward-only cursors
- FETCH forward, relative, and absolute variants for forward-only cursors
- CLOSE a cursor
- pg_catalog.pg_cursors

Things it's missing:
- BINARY CURSOR support, which returns data in the Postgres binary format
- MOVE support, which allows advancing the cursor without returning any rows. This should be quite similar to FETCH.
- WITH HOLD support (allows keeping a cursor open for longer than a transaction by writing its results into a buffer)
- Scrollable cursor (reverse fetch) support, we'd have to implement this also by savings results into a buffer.
- FOR UPDATE support
- Respect for `SAVEPOINT`. Cursor definitions will not disappear if rolled back to a savepoint before they were created. But we also don't do this right for settings (see #69396) so I don't think this is a major problem.

Co-authored-by: Jordan Lewis <[email protected]>
@rafiss
Copy link
Collaborator

rafiss commented Feb 27, 2022

I'm closing this since initial DECLARE support will be in v22.1. I've filed issues for additional work on cursors:

#77099
#77100
#77101
#77102
#77103
#77104

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-sql-pgcompat Semantic compatibility with PostgreSQL A-tools-aws-dms Blocking support for AWS Database Migration Service A-tools-tableau C-wishlist A wishlist feature. docs-done docs-known-limitation T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-anchored-telemetry The issue number is anchored by telemetry references.
Projects
None yet
Development

No branches or pull requests