Skip to content

Fix information_schema.tables failure for redirected tables#8684

Merged
phd3 merged 3 commits intotrinodb:masterfrom
phd3:fix-failure-listing-redirection
Oct 13, 2021
Merged

Fix information_schema.tables failure for redirected tables#8684
phd3 merged 3 commits intotrinodb:masterfrom
phd3:fix-failure-listing-redirection

Conversation

@phd3
Copy link
Copy Markdown
Member

@phd3 phd3 commented Jul 27, 2021

No description provided.

@cla-bot cla-bot bot added the cla-signed label Jul 27, 2021
@phd3 phd3 requested review from electrum and findepi July 27, 2021 22:05
@findepi findepi requested a review from sopel39 July 28, 2021 08:09
@phd3 phd3 force-pushed the fix-failure-listing-redirection branch from 1898cb1 to ffd4025 Compare July 29, 2021 19:11
Copy link
Copy Markdown
Member

@sopel39 sopel39 Jul 30, 2021

Choose a reason for hiding this comment

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

I'm not sure that is always a valid assumption. Do we check for existence of proper table handle in other places that use redirection?
IMO we should do the actual check, because it's cheap and target handle might not be valid

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think the point here is return correctly in

public List<QualifiedObjectName> listTables(Session session, QualifiedTablePrefix prefix)
{
requireNonNull(prefix, "prefix is null");
Optional<QualifiedObjectName> objectName = prefix.asQualifiedObjectName();
if (objectName.isPresent()) {
if (isExistingRelation(session, objectName.get())) {
return ImmutableList.of(objectName.get());
}
return ImmutableList.of();

we have options

  • table that redirects correctly
  • table that redirects to non-existent

including both cases in SHOW TABLES looks like a correct thing to do, since we cannot throw.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, so how do we handle redirections during listing (when no object name is provided)? We should be consistent with that

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

how do we handle redirections during listing (when no object name is provided)

unfiltered listTables doesn't care about redirections, just returns connector's results.

@findepi for the redirected+filtered case, do you mean we should try getting redirected table handle, and return empty if there's an exception doing that? That'd be closer to current filtered+unredirected case, but different from unfiltered+redirected case.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

try getting redirected table handle, and return empty if there's an exception doing that?

that could be reasonable when we can emit a warning at the same time

for now, i would rather fail instead.
the situation where we do redirect but then get "404", looks like misconfiguration.
we're probably yet to learn what are the conditions where such situation would be legitimate (outside of our control)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@findepi does the fixup commit look better?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So we don't filter on target table name? I though this redirection is logically similar to symlink so we perform every action (e.g access control checks) on targets.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

for table listing, we don't filter on target. If a is redirected to b, we're only listing a, and not b. #7606 (comment)

@phd3 phd3 force-pushed the fix-failure-listing-redirection branch from ffd4025 to 8acdedb Compare September 22, 2021 22:57
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

the isExistingRelation is currently used only in io.trino.metadata.MetadataManager#listTables where we decided not to confirm table existence with the target catalog. The filtered case could be done consistently.
However, we currently have no SPI for checking relation existence without following redirects. getTableHandle would be that, but it also does additional checks ("not a Hive table", "not an iceberg table").

It seems we could benefit from consolidated resolveRelation SPI which would determine whether relation is a table, redirect, view or materialized view (#9400 cc @losipiuk).

For now, add a comment and maybe a TODO link.

Also, please expand the line

// If the table is redirected, but the target table handle is missing, an exception is thrown

-- currently it looks like the behavior is desireable, while it not necessarily is. It's just documenting current state of things.

phd3 added 3 commits October 1, 2021 18:59
Queries on information_schema.tables failed when the filters pointed
to a specific table, and the table was redirected.
empty object names cause SchemaTableName creation to fail.
@phd3 phd3 force-pushed the fix-failure-listing-redirection branch from 8acdedb to dee6701 Compare October 1, 2021 23:00
@phd3
Copy link
Copy Markdown
Member Author

phd3 commented Oct 4, 2021

@findepi AC

@findepi findepi changed the title Fix failure with table listing for redirected tables Fix information_schema.tables failure for redirected tables Oct 12, 2021
@phd3 phd3 merged commit 611b468 into trinodb:master Oct 13, 2021
@phd3
Copy link
Copy Markdown
Member Author

phd3 commented Oct 13, 2021

Merged #8684 into master.

@github-actions github-actions bot added this to the 364 milestone Oct 13, 2021
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.

3 participants