graph,graphql,server,store: Subgraph Sql Service #5382

gusinacio · 2024-04-30T00:02:57Z

Semiotic Labs is thrilled to present the Subgraph SQL Service in partnership with TheGuild and Edge & Node. This exposes a GraphQL type to any graph-node user, providing a secure SQL interface to directly query a subgraph's entities.

SQL Service

We introduce a new sql field on the "Query" type that exposes inputs so graph-node can receive raw SQL.

We also implemented a lot of safe sql rewrite to remove functions that we think may break the postgres instance along with some schema safety.

We also expose each of the tables via Common Table Expressions where we can have granular control over what is exposed or not and some workaround exposing only the latest block data.

What we cover on this PR

New sql query type
Defined API for inputs for backward compatibility with future work
Environment variable/Config to enable or disable the SQL service
Safety measures for SQL threats via complete AST SQL scan.
Latest block data
Single subgraph query
JSON and CSV union output

What we do not cover on this PR

Cross subgraph joins
Query on specific block/snapshot
Query optimizations
Query parameters binding
Query/Join between two different blocks/snapshots

graph/src/data/graphql/queryable_type.rs

graph/src/schema/api.rs

lutter · 2024-05-01T23:49:09Z

Wow .. that's a really nice addition!

Before I start reviewing this in detail, I noticed a few things from a quick look at the PR:

there is no end-user documentation. Can you add something to docs/ that explains how to run queries? Obvious questions that should answer are: how do I send a query (example)? how do I use bind params? how does the GraphQL schema relate to the tables that this can query? What are the attributes/columns that can be queried? How much of SQL does this cover? I haven't looked but does this allow e.g., aggregations, window functions, CTEs etc.?
for operators: since SQL queries can be arbitrarily complex and take an arbitrary amount of time, how do they guard against long-running queries?
a minor nit is that commit messages should follow the guidelines here This won't cause us to not approve the PR, but it would be nice to keep with the current scheme.
this might be too much work, but I see that a sql crate appears and is then folded into the store - would it be possible to squash various commits so that there are fewer commits that fix up previous ones, not just for that crate but other fix commits, too? It'll make reviewing a lot easier

graphql/src/execution/execution.rs

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai>

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai> executor: impelement methods to execute arbitary sql in query store and deployment store

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai>

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai> create store/postgres/src/sql validator and formater: full refactor Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai> refactor: move sql to store/postgres/src/sql Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai> parser: test for array of byte fixed Signed-off-by: Tümay Tuzcu <tumay@semiotic.ai> parser: block_range and block$ added as available columns for tables, sequence functions moved to black list Signed-off-by: Tümay Tuzcu <tumay@semiotic.ai> parser: use latest block as cte filter parser: fix escape columns and tables Fixes STU-217 Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai>

…able_type Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai>

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai>

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai> docs: add sql service docs section

… prelude CTE Fixes SQL-266 Signed-off-by: Tümay Tuzcu <tumay@semiotic.ai>

gusinacio · 2024-05-09T17:48:53Z

Wow .. that's a really nice addition!

Before I start reviewing this in detail, I noticed a few things from a quick look at the PR:

there is no end-user documentation. Can you add something to docs/ that explains how to run queries? Obvious questions that should answer are: how do I send a query (example)? how do I use bind params? how does the GraphQL schema relate to the tables that this can query? What are the attributes/columns that can be queried? How much of SQL does this cover? I haven't looked but does this allow e.g., aggregations, window functions, CTEs etc.?

for operators: since SQL queries can be arbitrarily complex and take an arbitrary amount of time, how do they guard against long-running queries?

a minor nit is that commit messages should follow the guidelines here This won't cause us to not approve the PR, but it would be nice to keep with the current scheme.

this might be too much work, but I see that a sql crate appears and is then folded into the store - would it be possible to squash various commits so that there are fewer commits that fix up previous ones, not just for that crate but other fix commits, too? It'll make reviewing a lot easier

Hey @lutter, thank you for the information. Here are some notes:

we added end-user documentation about the SQL-service to /docs as requested
graph-node has some build-in mechanisms to cancel long-running queries and we are relying on that
thanks for pointing out the guidelines, we rebased and reworded all the commits to make it easier to review
while rebasing, we removed how it was made and created commits of the "happy-path". We removed this appearance and fold of the SQL crate and fix up some commits.

## Issue As work takes place to [add support for SQL queries](graphprotocol/graph-node#5382) the Gateway should invalidate such queries in the meantime. ## Solution The Gateway should reject queries containing SQL fields. We want to reuse this code in the `sql-gateway`. Therefore we use the `SqlFieldBehavior` enum to inject the desired behavior. --- Signed-off-by: Joseph Livesey <joseph@semiotic.ai> Co-authored-by: Gustavo Inacio <gustavo@semiotic.ai>

lutter

I am not entirely through reviewing this, but didn't want to hold the review back for even longer.

Besides the comments I made inline, one major missing piece are end-to-end tests. For GraphQL queries we have a pretty comprehensive test suite in store/test-store/tests/core/interfaces.rs and store/test-store/tests/graphql/query.rs. I would want a similarly comprehensive test suite for SQL queries - basically if a change to graph-node breaks SQL queries, I want to find out about that from a failing test similar to the ones for GraphQL.

lutter · 2024-05-09T23:40:27Z

store/postgres/Cargo.toml

@@ -30,6 +30,8 @@ git-testament = "0.2.5"
 itertools = "0.12.1"
 hex = "0.4.3"
 pretty_assertions = "1.4.0"
+sqlparser = { version = "0.40.0", features = ["visitor"] }
+thiserror = "1.0.25"


We try to avoid importing crates more than once as much as possible (and we're far from perfect on that).

There's two ways in which we do that:

(the old way) Import the crate in graph/src/Cargo.toml and reexport it in graph/src/lib.rs. Other crates then say use graph::<a crate>

(the new way) Import the crate in the workspace Cargo.toml and add <a crate>.workspace = true in crates that use it

I just opened a PR #5403 to update sqlparser to the latest. Please use the workspace one and update this code for it.

lutter · 2024-05-09T23:42:40Z

graph/src/schema/api.rs


 use crate::data::graphql::ext::{
    camel_cased_names, DefinitionExt, DirectiveExt, DocumentExt, ValueExt,
 };
+use crate::prelude::s::*;
+use crate::prelude::*;


Please avoid * imports - they become a huge headache later in some refactorings. Just list everything you actually need.

Also, the single letter crates, like s, q and r should always be used as such, i.e., code should always say s::Document instead of just Document

lutter · 2024-05-09T23:52:10Z

graphql/src/store/resolver.rs

@@ -414,7 +413,7 @@ impl Resolver for StoreResolver {
            return self.lookup_meta(field).await;
        }

-        if object_type.is_sql() {
+        if ENV_VARS.graphql.enable_sql_service && object_type.is_sql() {
            return self.handle_sql(field);
        }


The ENV_VARS check shouldn't be necessary - when SQL is turned off, a query that uses it shouldn't even make it here as it would have been rejected during validation.

the reason why we have this check here is in the remote case where the schema contains a type called Sql which a disabled SQL service graph-node would allow it.

lutter · 2024-05-10T00:02:29Z

graphql/src/execution/execution.rs

+            name if is_introspection_field(name) => intro_set.push(field)?,
+            META_FIELD_NAME | "__typename" => meta_items.push(field),
+            SQL_FIELD_NAME => sql_items.push(field),
+            _ => data_set.push(field)?,


I think the comment above should say something why sql_items gets split out here (it's also missing an explanation why meta_items are split out). In a nutshell:

intro_set needs to be handled differently because we use a different resolver (IntrospectionResolver)

meta_items need to be handled separately because prefetch can not handle them

sql_items need to be handled separately for the same reason (?) If so, maybe we can just put them into meta_items and rename that variable to something more descriptive (maybe noprefetch_items though I don't love that either)

lutter · 2024-05-10T00:06:43Z

graphql/src/store/resolver.rs

+        let result = result.into_iter().map(|q| q.0).collect::<Vec<_>>();
+        let row_count = result.len();
+        // columns should be available even if there's no data
+        // diesel doesn't support "dynamic query" so it doesn't return column names


There's now a DynamicSelectStatement; PR #5372 uses that, but it requires quite a bit of ceremony. For now, this is fine, but would be great to avoid the whole to_jsonb dance at a later date.

This is good to know. I got to this crate when I was developing but at the time graph-node wasn't using diesel v2. Gonna take a look at it

lutter · 2024-05-16T00:29:02Z

docs/sql_service.md

+field accepts values of this type.
+
+To enable the Subgraph:SQL Service, set the `GRAPH_GRAPHQL_ENABLE_SQL_SERVICE` environment
+variable to `true` or `1`. This allows clients to execute SQL queries using the


I would just say to set this to true

lutter · 2024-05-16T00:29:26Z

docs/sql_service.md

+environment variable.
+
+- **Environment Variable:** `GRAPH_GRAPHQL_ENABLE_SQL_SERVICE`
+- **Default State:** Off (Disabled)


If you set this to true to turn it on, it would make sense to say that the default is false

lutter · 2024-05-16T00:30:45Z

docs/sql_service.md

+permitted to be used within SQL queries executed by the Subgraph:SQL Service, while `POSTGRES_BLACKLISTED_FUNCTIONS`
+serves as a safety mechanism to restrict the usage of certain PostgreSQL functions within SQL
+queries. These blacklisted functions are deemed inappropriate or potentially harmful to the
+system's integrity or performance. Both constants are defined in `store/postgres/src/sql/constants.rs`.


It's sorta icky, but it would be nicer for users to just list what's on the whitelist. My understanding is that anything not on the whitelist is forbidden. Mentioning a blacklist here just makes users wonder what happens for functions that are neither on the whitelist or the blacklist

lutter · 2024-05-16T00:36:04Z

docs/sql_service.md

+
+The GraphQL schema provided by the Subgraph:SQL Service reflects the structure of the SQL queries
+it can execute. It does not directly represent tables in a database. Users need to
+construct SQL queries compatible with their database schema.


This does not explain at all how I go from the GraphQL subgraph schema to the schema against which I can execute queries. If I have an entity type DailyPositionSnapshot, what's the table I query? Does it matter if that type is mutable, immutable, or a timeseries?

To avoid locking ourselves into how data is currently stored, I would suggest the following:

the table for an @entity type is the name of the type in snakecase

the columns of the table are all the non-derived attributes of the type, also snakecased

for aggregations, the table name is the name of the type snakecased together with the interval, i.e. type Stats @aggregation(intervals: ["hour", "day"], source: "Data" { .. } becomes stats('hour') in a SQL query

queries are always executed at a specific block, determined by the block constraint on the sql element

Thanks, you (@lutter) have already summed up some part of it. Let's expand on this with examples.
Can you give a subgraph example with aggregation ?

This subgraph is kinda silly because it just aggregates block numbers, but it explains how to use aggregations in various ways. The aggregation docs also have some more realistic examples.

lutter · 2024-05-16T00:38:14Z

graph/src/data/graphql/queryable_type.rs

+use super::{DocumentExt, ObjectTypeExt};
+
+#[derive(Copy, Clone, Debug)]
+pub enum QueryableType<'a> {


I like the name QueryableType much better than ObjectOrInterface; as I understand it, adding union here is necessary because of the return type of the sql query element, right?

Exactly! we want the user to select which type of response: JSON or CSV. This is also future-proof for possible new formats.

suchapalaver mentioned this pull request Apr 30, 2024

feat: add sql enabled detection edgeandnode/gateway#710

Closed

suchapalaver reviewed Apr 30, 2024

View reviewed changes

graph/src/data/graphql/queryable_type.rs Show resolved Hide resolved

suchapalaver reviewed Apr 30, 2024

View reviewed changes

graph/src/schema/api.rs Outdated Show resolved Hide resolved

suchapalaver mentioned this pull request Apr 30, 2024

feat: add sql enabled detection edgeandnode/gateway#711

Closed

lutter self-requested a review May 1, 2024 23:50

suchapalaver mentioned this pull request May 2, 2024

feat: add support for gateway to reject sql queries edgeandnode/gateway#712

Merged

suchapalaver reviewed May 3, 2024

View reviewed changes

graphql/src/execution/execution.rs Outdated Show resolved Hide resolved

suchapalaver mentioned this pull request May 3, 2024

docs: document merge of graph node to graphprotocol semiotic-ai/graph-node#27

Closed

gusinacio force-pushed the sql-post-rebase branch 2 times, most recently from 064e937 to cc50425 Compare May 8, 2024 01:00

suchapalaver mentioned this pull request May 8, 2024

docs: add sql service docs section semiotic-ai/graph-node#29

Merged

suchapalaver force-pushed the sql-post-rebase branch from 4ae5f4a to 082ff49 Compare May 8, 2024 01:37

gusinacio and others added 8 commits May 8, 2024 08:53

graph: add sql to schema

b765438

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai>

graph, store: create database sql executor

b365a81

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai> executor: impelement methods to execute arbitary sql in query store and deployment store

graph, graphql: add resolver for sql

8154735

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai>

graph, graphql, server, store: convert object_or_interface into query…

49949c6

…able_type Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai>

graph, graphql: add sql service config toggle

86345d8

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai>

docs: add sql service documentation

ee44541

Signed-off-by: Gustavo Inacio <gustavo@semiotic.ai> docs: add sql service docs section

store: Use double quotation format for parsed sql tables matched with…

a1a7c64

… prelude CTE Fixes SQL-266 Signed-off-by: Tümay Tuzcu <tumay@semiotic.ai>

suchapalaver force-pushed the sql-post-rebase branch from e577010 to a1a7c64 Compare May 8, 2024 16:52

lutter requested changes May 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

graph,graphql,server,store: Subgraph Sql Service #5382

graph,graphql,server,store: Subgraph Sql Service #5382

gusinacio commented Apr 30, 2024 •

edited

lutter commented May 1, 2024

gusinacio commented May 9, 2024 •

edited

lutter left a comment

lutter May 9, 2024

lutter May 9, 2024

lutter May 9, 2024

gusinacio May 21, 2024

lutter May 10, 2024

lutter May 10, 2024

gusinacio May 21, 2024

lutter May 16, 2024

lutter May 16, 2024

lutter May 16, 2024

lutter May 16, 2024

tumaysem May 22, 2024

lutter May 22, 2024

lutter May 16, 2024

gusinacio May 21, 2024

graph,graphql,server,store: Subgraph Sql Service #5382

Are you sure you want to change the base?

graph,graphql,server,store: Subgraph Sql Service #5382

Conversation

gusinacio commented Apr 30, 2024 • edited

SQL Service

What we cover on this PR

What we do not cover on this PR

lutter commented May 1, 2024

gusinacio commented May 9, 2024 • edited

lutter 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

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

gusinacio commented Apr 30, 2024 •

edited

gusinacio commented May 9, 2024 •

edited