Fix TableHandles with ColumnHandles caching by ssheikin · Pull Request #14751 · trinodb/trino

ssheikin · 2022-10-25T14:38:26Z

Description

Non-technical explanation

Release notes

( ) 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:

# Section
* Fix some things. ({issue}`issuenumber`)

kokosing

Changes to me looks reasonable.

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

kokosing · 2022-10-25T14:56:26Z

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

Is it possible to provide a test?

@raunaqmorarka @sopel39 could you please advice which connector invokes PruneTableScanColumns#pruneColumns e.g. running BaseConnectorTest#testDeleteWithSubquery

That rule should trigger whenever there are more columns output from the table scan than are used by the rest of the query plan. E.g. select nationkey, count(*) from (select * from nation) group by 1 in tpch.tiny

@ssheikin it looks like a bug fix. do you have a query reproducing the problem?

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

Table handle contains column handles. When column handle is changed, data in cache for table handles is outdated. Test TableHandles cache invalidation on columns change.

DeleteRowIdColumnHandle does not belong to the table handle however and is not designed to belong to it. This column handle required during analysis phase. The column is used for row-level delete.

findepi

"Invalidate caches more coarsely"

findepi · 2022-10-26T10:30:26Z

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

    {
        delegate.setColumnComment(session, handle, column, comment);
-        invalidateColumnsCache(handle.asPlainTable().getSchemaTableName());
+        invalidateTableCaches(handle.asPlainTable().getSchemaTableName());


cmt msg

Invalidate caches more coarsely

Table handle contains column handles. When column handle is changed,
data in cache for table handles is outdated.

I'd suggest title like "Invalidate table handle cache when column changed"

then drop "handle" in "When column handle is changed," -- it's not the column handle what's changing (column handles are immutable), it's the column itself

BTW why having stale io.trino.plugin.jdbc.JdbcColumnHandle#comment value inside a JdbcTableHandle.columns matters?

I don't think it's used explicitly. If it's used implicitly (via equals), then maybe we have concurrency problem.
i.e. what happens if one query pulls a JdbcTableHandle and then (before the first query planning finishes), some other query performs setColumnComment.
The CachingJdbcClient state will be eventually consistent, but the first query will be planning on an inconsistent state.

I think the only reasonable usage of the value of the comment is in JdbcColumnHandle.getRetainedSizeInBytes()

How the situation described above differs from the e.g. addColumn?

I think the only reasonable usage of the value of the comment is in JdbcColumnHandle.getRetainedSizeInBytes()

so the invalidation doesn't matter.

How the situation described above differs from the e.g. addColumn?

Good question.
Generally the JDBC table handle won't carry columns until they are projected.

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

Line 256 in 5579e70

tableHandles.add(new JdbcTableHandle(schemaTableName, getRemoteTable(resultSet), getTableComment(resultSet)));

But I can imagine some JDBC connector eagerly populating the JdbcTableHandle.columns field within io.trino.plugin.jdbc.JdbcClient#getTableHandle(io.trino.spi.connector.ConnectorSession, io.trino.spi.connector.SchemaTableName) call. Then, addColumn changes the state in a way that would matter.

anyway, I am fine with doing invalidateTableCaches here, for consistency.
i just hope this is "for consistency" and not "a fix"

JdbcTableHandle.columns matters?

It matters. Plenty of connectors for getColumns do a short-circuit and return columns from table handle. If they do not matter we should remove them.

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

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

findepi · 2022-10-26T10:39:15Z

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

+        // do not throw when invoked, however do not allow to set non-empty comment until the connector supports setting column comments
+        verify(comment.isEmpty(), "This connector does not support setting column comments");


This isn't correct anyway.
setColumnComment invoked with empty comment should unset existing comment, so the verify's safety is an illusion.

what about:

// Ignore (not fail) for testing purposes.

without a verify?

This isn't correct anyway.

true.

unset existing comment

existing comment does not exist, because the connector does not support setting column comments.
And for the similar case with table comment getTableComment always returns Optional.empty(), so it's in tact.

without a verify

I'd left it as it is, because if I test setColumnComment, I'd not test it as setColumnComment(Optional.empty()), but with some value, and this could led to some higher expectations from tests which use this function.

unset existing comment

existing comment does not exist, because the connector does not support setting column comments.

it could exist if H2 supports column comments.

Trino is not the only gate to the underlying db

without a verify

I'd left it as it is, because if I test setColumnComment, I'd not test it as setColumnComment(Optional.empty()), but with some value, and this could led to some higher expectations from tests which use this function.

Fine. Leave it but please make it clear it's not correct, but just as a reminder.

I was thinking about it. I was thinking that setting empty comment is fine, for connectors that do not support it. However I was wrong:

It is a difference between empty comment and null comment

H2 may change and we could give a wrong impression that support it.

Yes, it's controversial. So better trade-off is not to test caching for addComment.

As agreed, left it as it is now.

findepi

"Fix verify in DefaultJdbcMetadata#applyProjection"

findepi · 2022-10-26T10:41:57Z

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

-            verify(tableColumnSet.containsAll(newColumnSet), "applyProjection called with columns %s and some are not available in existing query: %s", newColumnSet, tableColumnSet);
+            Set<JdbcColumnHandle> tableSyntheticColumnSet = ImmutableSet.<JdbcColumnHandle>builder()
+                    .addAll(tableColumnSet)
+                    .add((JdbcColumnHandle) getDeleteRowIdColumnHandle(session, table))


Make getDeleteRowIdColumnHandle return JdbcColumnHandle and remove the cast here.

findepi · 2022-10-26T10:43:05Z

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

@ssheikin it looks like a bug fix. do you have a query reproducing the problem?

findepi · 2022-10-26T10:43:48Z

@ssheikin is it still a Draft, or for review?

ssheikin · 2022-10-26T11:24:36Z

I've addressed comments for the first commit and extracted it as a separate PR: #14762

cla-bot bot added the cla-signed label Oct 25, 2022

ssheikin marked this pull request as draft October 25, 2022 14:38

kokosing changed the title ~~[WIP] [DO NOT review] Fix TableHandles with ColumnHandles caching~~ Fix TableHandles with ColumnHandles caching Oct 25, 2022

kokosing reviewed Oct 25, 2022

View reviewed changes

ssheikin commented Oct 25, 2022

View reviewed changes

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

ssheikin force-pushed the ssheikin/61/oss/try-reuse-columns branch from 9063668 to ce1ad4b Compare October 26, 2022 09:13

ssheikin added 2 commits October 26, 2022 11:14

Invalidate caches more coarsely

372d71d

Table handle contains column handles. When column handle is changed, data in cache for table handles is outdated. Test TableHandles cache invalidation on columns change.

Fix verify in DefaultJdbcMetadata#applyProjection

a54d488

DeleteRowIdColumnHandle does not belong to the table handle however and is not designed to belong to it. This column handle required during analysis phase. The column is used for row-level delete.

ssheikin force-pushed the ssheikin/61/oss/try-reuse-columns branch from ce1ad4b to a54d488 Compare October 26, 2022 09:14

findepi reviewed Oct 26, 2022

View reviewed changes

ssheikin closed this Oct 27, 2022

		// do not throw when invoked, however do not allow to set non-empty comment until the connector supports setting column comments
		verify(comment.isEmpty(), "This connector does not support setting column comments");

Conversation

ssheikin commented Oct 25, 2022

Description

Non-technical explanation

Release notes

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ssheikin Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findepi commented Oct 26, 2022

Uh oh!

ssheikin commented Oct 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

ssheikin Oct 25, 2022 •

edited

Loading