Skip to content

Conversation

@findepi
Copy link
Member

@findepi findepi commented Feb 6, 2023

Reverts #15225

The current implementation has the following problems

The revert is only temporary, the functionality will be reintroduced soon.

Fixes #12535

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
Copy link
Member Author

findepi commented Feb 6, 2023

Of course, we can also fix the functionality, instead of revert, if a fix exists.
Generally I would prefer the master branch to be always release-ready, and reverting the revert is trivial, so we lose nothing with this PR.

cc @trinodb/maintainers

@findepi
Copy link
Member Author

findepi commented Feb 6, 2023

CI failed with an Ubuntu update problem, going to be fixed by #15840
(cc @MiguelWeezardo that's why it would be nice to have an issue, #15840 (comment))

@findepi findepi force-pushed the revert-15225-david.stryker/add-column-change-callbacks branch from d404494 to 15f573e Compare February 6, 2023 12:35
@findepi
Copy link
Member Author

findepi commented Feb 6, 2023

(updated commit message, no other changes)

@findepi
Copy link
Member Author

findepi commented Feb 6, 2023

CI #15992

Copy link
Member

@djsagain djsagain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that this caused trouble.

@findepi
Copy link
Member Author

findepi commented Feb 6, 2023

Sorry that this caused trouble.

No worries @djsstarburst and thank you for your review!

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we can fix this trivially by doing the getTableMetadata() before the column operation. It should be guarded by the security management check, so that we don't fetch it unnecessarily.

So add column would look like

@Override
public void addColumn(Session session, TableHandle tableHandle, ColumnMetadata column)
{
    CatalogHandle catalogHandle = tableHandle.getCatalogHandle();
    CatalogMetadata catalogMetadata = getCatalogMetadataForWrite(session, catalogHandle);
    ConnectorMetadata metadata = catalogMetadata.getMetadata(session);

    SchemaTableName tableName = null;
    if (catalogMetadata.getSecurityManagement() != CONNECTOR) {
        tableName = getTableMetadata(session, tableHandle).getTable();
    }

    metadata.addColumn(session.toConnectorSession(catalogHandle), tableHandle.getConnectorHandle(), column);

    if (catalogMetadata.getSecurityManagement() != CONNECTOR) {
        CatalogSchemaTableName sourceTableName = new CatalogSchemaTableName(catalogHandle.getCatalogName(), tableName);
        systemSecurityMetadata.columnCreated(session, sourceTableName, column.getName());
    }
}

@findepi
Copy link
Member Author

findepi commented Feb 6, 2023

It seems like we can fix this trivially by doing the getTableMetadata() before the column operation.

That solves some, but not all, problems outlined above.

Anyway, looking forward in seeing this in a PR.

@findepi
Copy link
Member Author

findepi commented Feb 7, 2023

Marked this as a correctness because this fixes a correctness issue #12535.
Marked this as a RELEASE-BLOCKER because the correctness issue is a regression introduced in the last release (406; #12535 (comment))

Talking with @electrum @djsstarburst offline about this PR.

Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn’t realize this is correctness issue in an existing connector, when not using this feature. We should revert.

@findepi
Copy link
Member Author

findepi commented Feb 7, 2023

Thank you @electrum for your review!

@findepi
Copy link
Member Author

findepi commented Feb 8, 2023

CI #14637

@findepi findepi merged commit 6741a93 into master Feb 8, 2023
@findepi findepi deleted the revert-15225-david.stryker/add-column-change-callbacks branch February 8, 2023 10:02
@github-actions github-actions bot added this to the 407 milestone Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

Adding column in SQL Server connector sometimes fails even though column has been added successfully

4 participants