Add column add/drop/rename events#15225
Merged
electrum merged 1 commit intotrinodb:masterfrom Jan 23, 2023
Merged
Conversation
djsagain
force-pushed
the
david.stryker/add-column-change-callbacks
branch
from
8b98c4d to
a5039cd
Compare
electrum
approved these changes
Nov 29, 2022
electrum
requested changes
Nov 29, 2022
core/trino-main/src/main/java/io/trino/metadata/MetadataManager.java
Outdated
Show resolved
Hide resolved
djsagain
force-pushed
the
david.stryker/add-column-change-callbacks
branch
from
a5039cd to
39bcbaa
Compare
djsagain
force-pushed
the
david.stryker/add-column-change-callbacks
branch
from
39bcbaa to
c1d2607
Compare
Add methods in SystemSecurityMetadata to provide notification of adding, dropping and renaming columns.
djsagain
force-pushed
the
david.stryker/add-column-change-callbacks
branch
from
c1d2607 to
c337861
Compare
findepi
reviewed
Feb 1, 2023
| metadata.dropColumn(session.toConnectorSession(catalogHandle), tableHandle.getConnectorHandle(), column); | ||
| if (catalogMetadata.getSecurityManagement() != CONNECTOR) { | ||
| String columnName = getColumnMetadata(session, tableHandle, column).getName(); | ||
| TableMetadata tableMetadata = getTableMetadata(session, tableHandle); |
Member
There was a problem hiding this comment.
@djsstarburst
It seems it doesn't work for some connectors.
For example, with Hive connector, metadata.dropColumn sets the SemiTransactionalHiveMetastore into EXCLUSIVE_OPERATION_BUFFERED state and then getTableMetadata fails
io.trino.testing.QueryFailedException: Unsupported combination of operations in a single transaction
at io.trino.testing.AbstractTestingTrinoClient.execute(AbstractTestingTrinoClient.java:122)
at io.trino.testing.DistributedQueryRunner.execute(DistributedQueryRunner.java:490)
at io.trino.testing.QueryAssertions.assertUpdate(QueryAssertions.java:71)
at io.trino.testing.AbstractTestQueryFramework.assertUpdate(AbstractTestQueryFramework.java:365)
at io.trino.testing.AbstractTestQueryFramework.assertUpdate(AbstractTestQueryFramework.java:360)
at io.trino.testing.BaseConnectorTest.testAddAndDropColumnName(BaseConnectorTest.java:4308)
at io.trino.testing.BaseConnectorTest.testAddAndDropColumnName(BaseConnectorTest.java:4286)
...
Suppressed: java.lang.Exception: SQL: ALTER TABLE tcn_lowercaseh3yyp7xswo DROP COLUMN lowercase
at io.trino.testing.DistributedQueryRunner.execute(DistributedQueryRunner.java:493)
... 29 more
Caused by: io.trino.spi.TrinoException: Unsupported combination of operations in a single transaction
at io.trino.plugin.hive.metastore.SemiTransactionalHiveMetastore.checkReadable(SemiTransactionalHiveMetastore.java:2357)
at io.trino.plugin.hive.metastore.SemiTransactionalHiveMetastore.getTable(SemiTransactionalHiveMetastore.java:244)
at io.trino.plugin.hive.HiveMetadata.doGetTableMetadata(HiveMetadata.java:583)
at io.trino.plugin.hive.HiveMetadata.getTableMetadata(HiveMetadata.java:569)
at io.trino.plugin.hive.HiveMetadata.getTableMetadata(HiveMetadata.java:563)
at io.trino.plugin.base.classloader.ClassLoaderSafeConnectorMetadata.getTableMetadata(ClassLoaderSafeConnectorMetadata.java:263)
...
at io.trino.metadata.MetadataManager.getTableMetadata(MetadataManager.java:450)
at io.trino.metadata.MetadataManager.dropColumn(MetadataManager.java:763)
at io.trino.execution.DropColumnTask.execute(DropColumnTask.java:99)
findepi
reviewed
Feb 2, 2023
| String columnName = getColumnMetadata(session, tableHandle, column).getName(); | ||
| TableMetadata tableMetadata = getTableMetadata(session, tableHandle); | ||
| CatalogSchemaTableName sourceTableName = new CatalogSchemaTableName(catalogHandle.getCatalogName(), tableMetadata.getTable()); | ||
| systemSecurityMetadata.columnDropped(session, sourceTableName, columnName); |
Member
There was a problem hiding this comment.
this may be problematic from transaction perspective.
what if we dispatch the systemSecurityMetadata.columnDropped event but the transaction doesn't get committed eventually and we end up still having the column?
credits to @losipiuk for thinking about that
Member
There was a problem hiding this comment.
a bit related, but vice versa, a failure here will cause Trino transaction failure even if the connector applied the changes immediately (many do). This led to #12535, see #12535 (comment) for explanation
Member
There was a problem hiding this comment.
Notice that problem also exists for other systemSecurityMetadata notification methods too, like alter table rename
Member
|
Per offline discussion, I propose a temporary revert #15984. |
findepi
added a commit
that referenced
this pull request
Feb 6, 2023
This reverts commit 33fdaa0. The current implementation has the following problems: - does not work for Hive connector with `hive.security=system` (#15225 (comment)) - notifications are propagated even if transaction doesn't end up committing the DDL changes (#15225 (comment)) - notification propagation may disrupt transaction finish even if changes already applied in the external system (#12535 (comment)), for any connector that's using SYSTEM security (Iceberg, Delta with system security, or any JDBC connector)
findepi
added a commit
that referenced
this pull request
Feb 8, 2023
This reverts commit 33fdaa0. The current implementation has the following problems: - does not work for Hive connector with `hive.security=system` (#15225 (comment)) - notifications are propagated even if transaction doesn't end up committing the DDL changes (#15225 (comment)) - notification propagation may disrupt transaction finish even if changes already applied in the external system (#12535 (comment)), for any connector that's using SYSTEM security (Iceberg, Delta with system security, or any JDBC connector)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add methods in SystemSecurityMetadata to provide
notification of adding, dropping and renaming columns.
Description
Additional context and related issues
Release notes
(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: