Extend syntax for Dynamic Catalogs #22188

ssheikin · 2024-05-29T11:46:47Z

Description

Add ALTER CATALOG commands to change name, properties and owner with security checks
RENAME, SET PROPERTIES implemented
catalog owner is not supported yet.
catalog comment is not supported yet.

Additional context and related issues

Trino epic #12709

Release notes

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

# Section
* Add support for ALTER CATALOG RENAME statement. ({issue}`22188`)
* Add support for ALTER CATALOG SET PROPERTIES statement. ({issue}`22188`)

ssheikin · 2024-06-03T19:06:22Z

@martint @dain @kokosing @nineinchnick @SemionPar please review

ebyhr · 2024-06-04T09:53:07Z

core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4

@@ -155,6 +162,7 @@ statement
    | EXPLAIN ANALYZE VERBOSE? statement                               #explainAnalyze
    | SHOW CREATE TABLE qualifiedName                                  #showCreateTable
    | SHOW CREATE SCHEMA qualifiedName                                 #showCreateSchema
+    | SHOW CREATE CATALOG identifier                                   #showCreateCatalog


The current implementation exposes all properties even if it's marked as @ConfigHidden or @ConfigSecuritySensitive. I think we should hide or redact such properties.

@ssheikin could you maybe extract SHOW CREATE CATALOG to a separate PR? @martint would that help with getting the less controversial parts merged sooner?

I have removed SHOW CREATE CATALOG from this PR. It should be possible to re-add it once the following are merged:

Add connector SPI for returning redactable properties #24562

Redact sensitive information in catalog queries #24563

I'm not sure if we want to introduce CREATE CATALOG LIKE (at least not at this stage). One question that comes to mind is how to define access rules for it. According to the SQL specification for CREATE TABLE LIKE:

If a <like clause> is contained in a <table definition>, then the applicable privileges for A shall include SELECT privilege on the table identified in the <like clause>.

The issue is that we don’t have a SELECT privilege for catalogs.
I’m also not aware of any databases that provide CREATE ... LIKE functionality for anything other than tables. This makes it difficult to infer whether there’s a common approach we could adopt in this scenario.

core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4

github-actions · 2024-07-15T17:02:52Z

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

mosabua · 2024-07-15T17:18:03Z

I am adding the stale-ignore label under the assumption that you are still driving this PR to completion @ssheikin

ebyhr · 2024-07-26T13:07:35Z

core/trino-main/src/main/java/io/trino/execution/RenameCatalogTask.java

+        Session session = stateMachine.getSession();
+
+        accessControl.checkCanRenameCatalog(session.toSecurityContext(), statement.getSource().getValue(), statement.getTarget().getValue());
+
+        catalogManager.renameCatalog(new CatalogName(statement.getSource().getValue()), new CatalogName(statement.getTarget().getValue()));


Did you omit lowercase intentionally?

trino> CREATE CATALOG test USING memory; CREATE CATALOG trino> ALTER CATALOG test RENAME TO TEST; RENAME CATALOG trino> show catalogs; Catalog --------- TEST memory system tpch (4 rows)

I think it was unintentional. I updated it to match how other catalog statements work.

piotrrzysko · 2025-01-07T09:40:25Z

Current status of this PR: it will no longer include SHOW CREATE CATALOG and CREATE CATALOG LIKE. The former will likely be re-added once redacting security-sensitive information (#24563) is merged. The latter probably needs more discussion. I don’t have permissions to update the PR description. Could one of the maintainers or @ssheikin update it?

kokosing · 2025-01-07T10:21:58Z

I have updated the description

core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java

hashhar · 2025-01-15T12:37:47Z

core/trino-main/src/main/java/io/trino/connector/CoordinatorDynamicCatalogManager.java

+            createCatalogLikeInternal(catalogName, newCatalogName, ImmutableMap.of());
+            catalogStore.removeCatalog(catalogName);
+            activeCatalogs.remove(catalogName);


how do we ensure this is "transactional"?

There was a bug that caused activeCatalogs to be updated even when persisting the catalog specification failed. I’ve fixed this.

Currently, I see one problematic scenario:

catalogStore.addOrReplaceCatalog(newCatalog) succeeds.

catalogStore.removeCatalog(oldCatalog) fails.

If this happens, we end up with two catalogs, and the only way to recover is to manually DROP the old one.

To address this, we might need to extend the SPI by adding CatalogStore.replace(oldCatalog, newCatalog) and requiring implementations to perform this operation transactionally.

the replace would probably also allow implementing ALTER CATALOG ... RENAME properly instead of as DROP + CREATE.

Let's skip for now and discuss it later.

core/trino-main/src/test/java/io/trino/execution/TestRenameCatalogTask.java

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

hashhar · 2025-01-15T12:40:35Z

@martint @kasiafi do you want to take a look?

The original concerns were addressed by removing both SHOW CREATE CATALOG AND CREATE CATALOG LIKE....

martint · 2025-01-16T23:29:50Z

core/trino-main/src/main/java/io/trino/security/AccessControl.java

+    /**
+     * Check if identity is allowed to set properties to the specified catalog.
+     *
+     * @throws AccessDeniedException if not allowed


If any of the listed properties are not allowed?

I see that this follows the pattern used for other methods in this file. I believe the idea is that this exception is thrown when accessing a given resource is not allowed.

core/trino-main/src/main/java/io/trino/security/AccessControl.java

core/trino-parser/src/main/java/io/trino/sql/tree/RenameCatalog.java

core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java

core/trino-main/src/main/java/io/trino/connector/CoordinatorDynamicCatalogManager.java

martint · 2025-01-16T23:49:14Z

core/trino-main/src/main/java/io/trino/connector/CoordinatorDynamicCatalogManager.java

+                throw new TrinoException(ALREADY_EXISTS, format("Catalog '%s' already exists", newCatalogName));
+            }
+
+            createCatalogLikeInternal(catalogName, newCatalogName, ImmutableMap.of());


Why is rename modeled as "clone the catalog into a new one, drop the old one" vs adding proper support for renaming catalogs? I'm concerned about the impact of any state that needs to be dropped and re-created, caches, etc., that would otherwise not be necessary to manage with a proper rename.

Renaming requires the following steps:

Creating a new connector instance, with the catalog name passed to ConnectorFactory::create.

Updating in-memory data structures in CoordinatorDynamicCatalogManager (activeCatalogs and allCatalogs).

Persisting the updated catalog name and version via CatalogStore.

Marking the old catalog as inactive and eventually pruning it.

It looks like this is exactly what clone and drop do. Do you think there's a less invasive way to implement rename?

Got it. It's not ideal, though. It shouldn't be necessary to re-create the catalog just to give it a new name.

I guess the underlying problem is that ConnectorFactory::create takes a name. We should look into (for a future improvement) what it would take to decouple the instance from its name so that the name is just an entry in the catalog maps.

core/trino-main/src/test/java/io/trino/execution/TestRenameCatalogTask.java

core/trino-main/src/test/java/io/trino/execution/TestSetCatalogPropertiesTask.java

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

piotrrzysko

I've addressed all the comments. Along the way, I discovered two bugs that should now be fixed:

Previously, duplicated properties in ALTER CATALOG ... SET PROPERTIES caused an exception that could expose potentially security-sensitive property values. Now, only the last value is used, matching the behavior of CREATE CATALOG and ALTER TABLE ... SET PROPERTIES.
The in-memory state (activeCatalogs) was being updated even when persisting the catalog specification failed. Now, it is updated only when the corresponding CatalogStore operation succeeds.

piotrrzysko · 2025-01-17T16:45:26Z

core/trino-main/src/main/java/io/trino/security/AccessControl.java

+    /**
+     * Check if identity is allowed to set properties to the specified catalog.
+     *
+     * @throws AccessDeniedException if not allowed


I see that this follows the pattern used for other methods in this file. I believe the idea is that this exception is thrown when accessing a given resource is not allowed.

piotrrzysko · 2025-01-19T15:46:28Z

core/trino-main/src/main/java/io/trino/connector/CoordinatorDynamicCatalogManager.java

+                throw new TrinoException(ALREADY_EXISTS, format("Catalog '%s' already exists", newCatalogName));
+            }
+
+            createCatalogLikeInternal(catalogName, newCatalogName, ImmutableMap.of());


Renaming requires the following steps:

Creating a new connector instance, with the catalog name passed to ConnectorFactory::create.

Updating in-memory data structures in CoordinatorDynamicCatalogManager (activeCatalogs and allCatalogs).

Persisting the updated catalog name and version via CatalogStore.

Marking the old catalog as inactive and eventually pruning it.

It looks like this is exactly what clone and drop do. Do you think there's a less invasive way to implement rename?

piotrrzysko · 2025-01-19T16:12:58Z

core/trino-main/src/main/java/io/trino/connector/CoordinatorDynamicCatalogManager.java

+        }
+    }
+
+    private CatalogConnector createCatalogLikeInternal(CatalogName oldCatalogName, CatalogName catalogName, Map<String, Optional<String>> properties)


I've inlined this. The logic for rename and alter differs slightly, and I believe it's easier to follow the flow of these operations when all the steps are in one place.

piotrrzysko · 2025-01-19T16:24:42Z

core/trino-main/src/main/java/io/trino/connector/CoordinatorDynamicCatalogManager.java

+            createCatalogLikeInternal(catalogName, newCatalogName, ImmutableMap.of());
+            catalogStore.removeCatalog(catalogName);
+            activeCatalogs.remove(catalogName);


There was a bug that caused activeCatalogs to be updated even when persisting the catalog specification failed. I’ve fixed this.

Currently, I see one problematic scenario:

catalogStore.addOrReplaceCatalog(newCatalog) succeeds.

catalogStore.removeCatalog(oldCatalog) fails.

If this happens, we end up with two catalogs, and the only way to recover is to manually DROP the old one.

To address this, we might need to extend the SPI by adding CatalogStore.replace(oldCatalog, newCatalog) and requiring implementations to perform this operation transactionally.

core/trino-parser/src/test/java/io/trino/sql/parser/TestSqlParser.java

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

martint

A couple of minor comments.

Also, you should squash the first and second commits. Otherwise, the first commit is "broken", since there's no handling of the new statements in the analyzer. Either, it should fail with a proper error saying "RENAME not yet supported" or implement the functionality.

core/trino-main/src/main/java/io/trino/connector/CoordinatorDynamicCatalogManager.java

core/trino-main/src/test/java/io/trino/execution/TestRenameCatalogTask.java

piotrrzysko · 2025-02-02T20:36:33Z

I've addressed the latest comments.

Co-authored-by: Jan Waś <[email protected]>

- RENAME - SET PROPERTIES

piotrrzysko · 2025-02-07T17:11:05Z

@martint Could you please take a look? I addressed your recent comments.

mosabua · 2025-02-11T23:49:50Z

This will also need docs before merge. Please work with @jhlodin and myself to get that added here or in a separate PR.

cla-bot bot added the cla-signed label May 29, 2024

ssheikin requested review from dain, nineinchnick, kokosing, SemionPar and martint May 29, 2024 11:47

ssheikin changed the title ~~Improve usability of Dynamic Catologs~~ Extend syntax for Dynamic Catologs May 29, 2024

ssheikin changed the title ~~Extend syntax for Dynamic Catologs~~ Extend syntax for Dynamic Catalogs May 29, 2024

hashhar added the syntax-needs-review label May 29, 2024

nineinchnick approved these changes Jun 3, 2024

View reviewed changes

ebyhr reviewed Jun 4, 2024

View reviewed changes

martint reviewed Jun 21, 2024

View reviewed changes

core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4 Outdated Show resolved Hide resolved

github-actions bot added the stale label Jul 15, 2024

mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Jul 15, 2024

ksobolew mentioned this pull request Jul 16, 2024

Implement getCatalogProperties in StaticCatalogManager #22669

Closed

ebyhr reviewed Jul 26, 2024

View reviewed changes

hashhar mentioned this pull request Jul 31, 2024

Dynamic Catalogs #12709

Open

21 tasks

bitsondatadev mentioned this pull request Sep 9, 2024

Trino dev ex, education, and small local uses design proposal #23344

Open

piotrrzysko force-pushed the ssheikin/90/trino/dc branch 2 times, most recently from 93954a1 to 315ed4d Compare January 6, 2025 20:09

hashhar approved these changes Jan 15, 2025

View reviewed changes

martint requested changes Jan 16, 2025

View reviewed changes

piotrrzysko force-pushed the ssheikin/90/trino/dc branch from 315ed4d to aba00a2 Compare January 21, 2025 12:00

piotrrzysko reviewed Jan 21, 2025

View reviewed changes

piotrrzysko force-pushed the ssheikin/90/trino/dc branch 2 times, most recently from db7c4b6 to c3cc7db Compare January 21, 2025 14:10

piotrrzysko requested a review from martint January 21, 2025 15:22

martint reviewed Feb 1, 2025

View reviewed changes

piotrrzysko force-pushed the ssheikin/90/trino/dc branch from c3cc7db to fb94819 Compare February 2, 2025 19:41

piotrrzysko requested a review from martint February 2, 2025 20:36

ssheikin and others added 2 commits February 5, 2025 13:44

Add ALTER CATALOG

ff89e50

Co-authored-by: Jan Waś <[email protected]>

Extend access control for catalogs

b76977a

- RENAME - SET PROPERTIES

piotrrzysko force-pushed the ssheikin/90/trino/dc branch from fb94819 to b76977a Compare February 5, 2025 12:44

ebyhr mentioned this pull request Apr 10, 2025

[Feature] Add ALTER CATALOG #25542

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend syntax for Dynamic Catalogs #22188

Extend syntax for Dynamic Catalogs #22188

ssheikin commented May 29, 2024 •

edited

Loading

ssheikin commented Jun 3, 2024

ebyhr Jun 4, 2024

nineinchnick Dec 27, 2024

piotrrzysko Jan 2, 2025

github-actions bot commented Jul 15, 2024

mosabua commented Jul 15, 2024

ebyhr Jul 26, 2024

piotrrzysko Jan 6, 2025

piotrrzysko commented Jan 7, 2025

kokosing commented Jan 7, 2025

hashhar Jan 15, 2025

piotrrzysko Jan 19, 2025

hashhar Jan 21, 2025

hashhar commented Jan 15, 2025

martint Jan 16, 2025

piotrrzysko Jan 17, 2025

martint Jan 16, 2025

piotrrzysko Jan 19, 2025

martint Jan 31, 2025

piotrrzysko left a comment

piotrrzysko Jan 17, 2025

piotrrzysko Jan 19, 2025

piotrrzysko Jan 19, 2025

piotrrzysko Jan 19, 2025

martint left a comment

piotrrzysko commented Feb 2, 2025

piotrrzysko commented Feb 7, 2025

mosabua commented Feb 11, 2025

Extend syntax for Dynamic Catalogs #22188

Are you sure you want to change the base?

Extend syntax for Dynamic Catalogs #22188

Conversation

ssheikin commented May 29, 2024 • edited Loading

Description

Additional context and related issues

Release notes

ssheikin commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 15, 2024

mosabua commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piotrrzysko commented Jan 7, 2025

kokosing commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashhar commented Jan 15, 2025

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

piotrrzysko 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

martint left a comment

Choose a reason for hiding this comment

piotrrzysko commented Feb 2, 2025

piotrrzysko commented Feb 7, 2025

mosabua commented Feb 11, 2025

ssheikin commented May 29, 2024 •

edited

Loading