Add table function to execute stored procedure in SQLServer #15982

Praveen2112 · 2023-02-06T08:21:49Z

Description

A dedicated table function which allows us to run stored procedures in SQLServer. We can't use existing query table function - as it wraps with inside a query like SELECT * FROM (EXEC my_procedure) o - so we are handing them via a different function. All the pushdown are disabled for JdbcProcedureHandle resolved by this Procedure.

Slightly different implementation from #15813

Limitation

In case of stored procedures with multiple statement - we process the ResultSet only for the first statement. (Test to be added)
Stored procedures with output variables are not supported.
Input variables for the stored procedures are not parameterized(yet).

Example

SELECT * FROM TABLE(system.procedure(query => 'CALL my_new_procedure()')

Release notes

( ) This is not user-visible or docs only and no release notes are required.
(x) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# SQLServer 
* Add support for executing stored procedures using `procedure` table function.

findepi

Overall LGTM. @kokosing please review

findepi · 2023-02-06T09:10:22Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcDynamicFilteringSplitManager.java

@@ -69,6 +69,10 @@ public ConnectorSplitSource getSplits(
            DynamicFilter dynamicFilter,
            Constraint constraint)
    {
+        if (table instanceof JdbcProcedureHandle) {
+            return delegateSplitManager.getSplits(transaction, session, table, dynamicFilter, constraint);


add a code comment

findepi · 2023-02-06T09:11:49Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcRecordCursor.java

@@ -63,7 +64,7 @@
    private ResultSet resultSet;
    private boolean closed;

-    public JdbcRecordCursor(JdbcClient jdbcClient, ExecutorService executor, ConnectorSession session, JdbcSplit split, JdbcTableHandle table, List<JdbcColumnHandle> columnHandles)
+    public JdbcRecordCursor(JdbcClient jdbcClient, ExecutorService executor, ConnectorSession session, JdbcSplit split, ConnectorTableHandle table, List<JdbcColumnHandle> columnHandles)


Let's have a BaseJdbcConnectorTableHandle marker interface and let JdbcProcedureHandle, JdbcTableHandle both extend it; use here

It should be as an abstract class right ?

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcRecordSet.java

findepi · 2023-02-06T09:13:22Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcRecordSetProvider.java

+            // If no columns are recorded, it means that applyProjection never got called (e.g., in the case all columns are being used) and all
+            // table columns should be returned. TODO: this is something that should be addressed once the getRecordSet API is revamped
+            jdbcTable.getColumns()
+                    .ifPresent(tableColumns -> verify(ImmutableSet.copyOf(tableColumns).containsAll(columns)));


Why not for procedure handle?
Have same check for them too

In case of ProcedureHandle we won't face the no columns kind of a situation - plus the columns are populated when we are creating the table handle. Add the ProcedureHandle doesn't support project pushdown there is a good chance the column we pass on RecordSetProvider might be a subset of overall columns present in ProcedureHandle. Will implement it.

findepi · 2023-02-06T09:16:39Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ptf/Procedure.java

+        }
+    }
+
+    public record ProcedureQuery(@JsonProperty String query)


not sure if we use records for between-nodes serialization (I think we are starting to)

then ProcedureFunctionHandle above can be a record too

also, i don't think we need @JsonProperty and @JsonCreator for records (airlift/airlift#993), do we?

ProcedureFunctionHandle - wanted be in-sync with other TableHandle implementation. I don't have any strong opinions on it. Will move it to a record.

findepi · 2023-02-06T09:18:42Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

+        if (handle instanceof QueryFunctionHandle queryFunctionHandle) {
+            return Optional.of(getTableFunctionApplicationResult(session, queryFunctionHandle.getTableHandle()));
+        }
+        else if (handle instanceof ProcedureFunctionHandle procedureFunctionHandle) {


redundant else

findepi · 2023-02-06T09:21:22Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

        return jdbcClient.delete(session, (JdbcTableHandle) handle);
    }

    @Override
    public void truncateTable(ConnectorSession session, ConnectorTableHandle tableHandle)
    {
+        verify(!(tableHandle instanceof JdbcProcedureHandle), "Not a table reference: %s", tableHandle);


You don't need this. The code will never get invoked with JdbcProcedureHandle, and if it would, it would fail explicitly anyway

findepi · 2023-02-06T09:21:36Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultJdbcMetadata.java

@@ -904,6 +985,7 @@ public void setColumnType(ConnectorSession session, ConnectorTableHandle table,
    @Override
    public void renameTable(ConnectorSession session, ConnectorTableHandle table, SchemaTableName newTableName)
    {
+        verify(!(table instanceof JdbcProcedureHandle), "Not a table reference: %s", table);


You don't need this.

Can we replicate it for all alter options ?

Praveen2112 · 2023-02-06T10:20:06Z

@findepi AC.

kokosing

looks good

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/JdbcTableHandle.java

vlad-lyutenko · 2023-02-06T13:31:00Z

We can't use existing query table function - as it wraps with inside a query like SELECT * FROM (CALL my_procedure()) o

As for me it's really new topic, can you please point me to some entry point, to understand what we have now (I mean existing query table function) and why we can't use it for this connectors

findepi · 2023-02-06T15:52:29Z

@vlad-lyutenko this all boils down to two things

Connection.prepareStatement vs Connection.prepareCall
blocking additional SQL pushdown for calls

mosabua · 2023-02-06T20:09:35Z

Given that this is a very powerful user facing feature that potentially has huge security and functionality implications I request the following:

Make sure documentation is included in this PR similar to the query passthrough table function with the relevant warnings and suggestions about security in each connector doc
Figure out how this works in terms of privileges or other access control we potentially have to add

cc @colebow @jhlodin @k-haley1

mosabua · 2023-02-06T20:22:36Z

Also for reference following is a link to the section in the MySQL connector docs where a new subsection for the new table function is needed.

https://trino.io/docs/current/connector/mysql.html#table-functions

And related question... will this table function have a similar restriction where it is required that the stored procedure returns a result set? If so .. that could be limiting functionality. And if not ... the security implications are even bigger since arbitrary stored procedures can be called.

Last.. I assume that the user accessing the database from the catalog configuration has to have the right to run the stored procedure .. can you detail what is actually specifically needed in MySQL and SQL Server terminology?

Praveen2112 · 2023-02-07T07:30:18Z

will this table function have a similar restriction where it is required that the stored procedure returns a result set?

Yes we have a same restriction where the stored procedure has to return a ResultSet - we don't support multiple ResultSet as of now.

I assume that the user accessing the database from the catalog configuration has to have the right to run the stored procedure

Similar to tables - if the user specified in the catalog configuration has access to run the stored procedures then we would support it.

Praveen2112 · 2023-02-07T07:30:27Z

@kokosing AC

kasiafi · 2023-02-08T14:23:04Z

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/ptf/Procedure.java

+                    SCHEMA_NAME,
+                    NAME,
+                    List.of(ScalarArgumentSpecification.builder()
+                            .name("QUERY")


I think it would be more user-friendly if the argument was renamed to procedure and the user would only pass the procedure call, that is "procedure_name()" without the CALL keyword.

Depending on vendor you are using CALL or EXEC. So maybe we could rename name of function to call to exec depending on vendor (connector) and only pass the name of procedure. This could work only if we are not expecting to pass parameters any parameters to procedures.

In case of arguments we could either pass them as an Array<String> and handle them accordingly.

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

mosabua · 2023-02-13T19:13:57Z

Need any help with the docs here @Praveen2112 ? Should I send a separate parallel PR?

kokosing

skimmed. I am fine with the approach

kokosing · 2023-02-24T12:08:57Z

docs/src/main/sphinx/connector/sqlserver.rst

+      );
+
+If the stored procedure ``employee_sp`` requires any input, it can be specified via ``inputs``
+argument as an ``array<varchar>``::


what if procedure expects an integer?

For now we pass the arguments as string - we pass it without quotes for integer and with additional quotes for string (if requested by the underlying datasource).

Should we document this?

But for SQLServer, it doesn't expect additional quotes. So do we need to document it ?

It’d be better to pass the arguments as a ROW, so each one has the proper type. You get type safety and avoid conversions that might not be possible to do easily or reliably.

Can you provide pointer on where/how it is established during the analysis step - Should we use a different Argument specification for this.

Cc @kasiafi

The type of scalar argument is established at TF declaration.

Even if we could pass arbitrary scalars, it seems right to use an array of varchar in this case. It shifts the responsibility of formatting them the right way to the user. That's in the spirit of query pass-through. This way it is harder for the user to pass parameters, or non-literal expressions, but I don't know if a real use-case would need those capabilities.

Like @kokosing noticed, we must watch out for possible abuse (injection).

Table argument is the only argument type of table functions that is polymorphic . Technically, we could make use of it, and pass a scalar subquery containing parameters. But then we'd have to refactor the PTF to use the operator-based execution.

kokosing · 2023-02-24T12:13:39Z

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerQueryBuilder.java

+    {
+        return new ProcedureQuery(
+                "EXECUTE %s.%s %s"
+                        .formatted(


it looks like one could inject some SQL by using inputs.

Thanks for pointing it out. Have added additional checks.

hashhar

Approach looks good, some early comments.

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java

hashhar · 2023-03-02T14:06:36Z

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java

+    public void testProcedureWithDropOperation()
+    {
+        try (TestTable table = new TestTable(onRemoteDatabase(), "table_to_drop", "(id BIGINT)")) {
+            try (TestProcedure testProcedure = createTestingProcedure("DROP TABLE " + table.getName())) {


https://learn.microsoft.com/en-us/sql/t-sql/queries/output-clause-transact-sql?view=sql-server-ver16 exists so can we test with it as well?

Have added test with OUTPUT clause.

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/BaseSqlServerConnectorTest.java

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java

Praveen2112 · 2023-03-03T13:37:35Z

@hashhar , @skrzypo987 AC

Praveen2112 · 2023-03-06T02:20:10Z

Rebased the PR to resolve conflicts.

hashhar

LGTM with the recent changes.

mosabua

Docs look good. Did not look at the code..

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestProcedure.java

kokosing · 2023-03-07T10:26:24Z

docs/src/main/sphinx/connector/sqlserver.rst

+      );
+
+If the stored procedure ``employee_sp`` requires any input, it can be specified via ``inputs``
+argument as an ``array<varchar>``::


Should we document this?

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/BaseJdbcClient.java

findepi · 2023-03-17T20:26:29Z

plugin/trino-sqlserver/src/main/java/io/trino/plugin/sqlserver/SqlServerClient.java

+    public JdbcProcedureHandle getProcedureHandle(ConnectorSession session, ProcedureInformation procedureInformation)
+
+    {
+        if (procedureInformation.inputArguments().stream().anyMatch(argument -> argument.contains(";"))) {


If we can process arguments via preparedCall.setParameter we wouldn't need this.
if we cannot process arguments via preparedCall.setParameter we shouldn't pretend we can.
If the user provides parameter values as string literals to be concatenated with the procedure call, why can't they provide the whole procedure call (including the parameters)? Just like we do with SQL pushdown?

If we can process arguments via preparedCall.setParameter we wouldn't need this.

It could be possible - like if we could get the procedure column information and map them accordingly. I would like implement them in an incremental way as executing stored procedure is kind of an experimental feature. WDYT ?

why can't they provide the whole procedure call (including the parameters)? Just like we do with SQL pushdown?

The initial approach we have is like SQL Pushdown - where we don't do any processing but consume a String as an input. For some datasources like SQLServer it would be helpful if we could get the schema and procedure name - so we could apply some checks to ensure the SP doesn't any DDL or DML operations. So it more of an extended version of SQLPushdown - where we take some of them in parts and concat all the pieces.

that was just for emphasis.

These methods can be reused for Procedures PTF - Extract building columns from ResultSetMetaData as a separate method. - Extract creating connection based on session

Praveen2112 · 2023-03-21T09:30:25Z

Rebased due to conflicts.

Praveen2112 · 2023-03-29T06:28:49Z

Overridden by #16696

Praveen2112 requested review from findepi, atanasenko, huberty89, kokosing, kasiafi and skrzypo987 February 6, 2023 08:21

cla-bot bot added the cla-signed label Feb 6, 2023

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch 2 times, most recently from e17613b to eec9f7d Compare February 6, 2023 08:52

findepi reviewed Feb 6, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch 2 times, most recently from 584e83c to 47bdf91 Compare February 6, 2023 10:19

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch from 47bdf91 to db1d540 Compare February 6, 2023 11:10

kokosing reviewed Feb 6, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch from db1d540 to 9bf4ed0 Compare February 7, 2023 07:25

Praveen2112 mentioned this pull request Feb 8, 2023

Add table function to execute stored procedure in MySQL & SQLServer #15813

Closed

kasiafi reviewed Feb 8, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch from 9bf4ed0 to 10dc303 Compare February 22, 2023 12:02

Praveen2112 changed the title ~~Add table function to execute stored procedure in MySQL & SQLServer~~ Add table function to execute stored procedure in SQLServer Feb 22, 2023

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch 2 times, most recently from 6d8b5c4 to f464ee5 Compare February 22, 2023 14:14

github-actions bot added the docs label Feb 22, 2023

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch from be36f5f to daec542 Compare February 24, 2023 06:43

kokosing reviewed Feb 24, 2023

View reviewed changes

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch from daec542 to d383be0 Compare February 27, 2023 10:10

Praveen2112 requested a review from hashhar February 28, 2023 14:03

hashhar reviewed Mar 2, 2023

View reviewed changes

skrzypo987 reviewed Mar 3, 2023

View reviewed changes

plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/CachingJdbcClient.java Outdated Show resolved Hide resolved

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch from d383be0 to 3792ce9 Compare March 3, 2023 13:33

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch 2 times, most recently from 3301e93 to 91ee0b7 Compare March 6, 2023 02:19

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch from 91ee0b7 to 5fa16b1 Compare March 6, 2023 06:14

hashhar approved these changes Mar 6, 2023

View reviewed changes

mosabua approved these changes Mar 6, 2023

View reviewed changes

huberty89 reviewed Mar 7, 2023

View reviewed changes

plugin/trino-base-jdbc/src/test/java/io/trino/plugin/jdbc/TestProcedure.java Outdated Show resolved Hide resolved

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch from 5fa16b1 to 8515486 Compare March 7, 2023 10:03

kokosing approved these changes Mar 7, 2023

View reviewed changes

kokosing force-pushed the master branch from 3f05134 to 58d6356 Compare March 14, 2023 11:34

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch from 8515486 to 47e7213 Compare March 16, 2023 08:18

findepi previously requested changes Mar 17, 2023

View reviewed changes

Praveen2112 added 4 commits March 21, 2023 14:53

Extract MappedPageSource and MappedRecordSet to toolkit

66bd995

Introduce BaseJdbcConnectorTableHandle

1a651e0

Extract methods in BaseJdbcClient

0a6a5b4

These methods can be reused for Procedures PTF - Extract building columns from ResultSetMetaData as a separate method. - Extract creating connection based on session

Add table function to execute stored procedure in SQLServer

88d57a0

Praveen2112 force-pushed the praveen/stored_procedure_ptf_2 branch from 47e7213 to 88d57a0 Compare March 21, 2023 09:30

Praveen2112 mentioned this pull request Mar 23, 2023

Add table function to execute stored procedure in SQLServer #16696

Merged

Praveen2112 closed this Mar 29, 2023

Add table function to execute stored procedure in SQLServer #15982

Add table function to execute stored procedure in SQLServer #15982

Conversation

Praveen2112 commented Feb 6, 2023 • edited Loading

Description

Limitation

Example

Release notes

findepi 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

Praveen2112 Feb 6, 2023 • edited Loading

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

Praveen2112 commented Feb 6, 2023

kokosing left a comment

Choose a reason for hiding this comment

vlad-lyutenko commented Feb 6, 2023 • edited Loading

findepi commented Feb 6, 2023

mosabua commented Feb 6, 2023 • edited Loading

mosabua commented Feb 6, 2023 • edited Loading

Praveen2112 commented Feb 7, 2023

Praveen2112 commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mosabua commented Feb 13, 2023

kokosing 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

hashhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Praveen2112 commented Mar 3, 2023

Praveen2112 commented Mar 6, 2023

hashhar left a comment

Choose a reason for hiding this comment

mosabua 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

Praveen2112 commented Mar 21, 2023

Praveen2112 commented Mar 29, 2023

Praveen2112 commented Feb 6, 2023 •

edited

Loading

Praveen2112 Feb 6, 2023 •

edited

Loading

vlad-lyutenko commented Feb 6, 2023 •

edited

Loading

mosabua commented Feb 6, 2023 •

edited

Loading

mosabua commented Feb 6, 2023 •

edited

Loading