Exception Handling during Metadata listing

[phd3]

https://github.com/trinodb/trino/pull/6374#discussion_r551777069

Some metadata listing implementations in MetadataManager invoke different ConnectorMetadata APIs depending on the provided filters. For example, MetadataManager#listViews invokes getView and listViews with and without a filter respectively. MetadataManager#listTables invokes getTableHandle and getView in filtered case, but listTables otherwise.

One issue with such an optimization is the possible difference in exception handling. A ConnectorMetadata#list.... implementation may want to ignore bad values, so that partial results can be returned (e.g. ignoring hive views that fail translation). However, when a filter for a specific table/view is used, an error is thrown because of the usage of focused APIs like getView or getTableHandle.

Any suggestions on avoiding such inconsistencies? I can think of the following approaches:

• Don't ignore failures in connector's listing methods. OR
• Modify MetadataManager's listing methods to catch exceptions from ConnectorMetadata#listXXX and ConnectorMetadata#getXXX methods. Change ConnectorMetadata#listXXX to return an iterator instead of list.

cc @findepi

[findepi]

cc @trinodb/maintainers

[kokosing]

How about simple convention where we expect ConnectorMetadata#listXXX to not to throw exception (ignore failures) while ConnectorMetadata#getXXX to throw when object incorrect? Then in metadata listing implementations we would need to handle all exceptions from any ConnectorMetadata#getXXX that is called there.

It sounds to me inline with what we have now. As I understand we only have gaps here.

[phd3]

Then in metadata listing implementations we would need to handle all exceptions from any ConnectorMetadata#getXXX that is called there.

This is the main gap currently, I think.

There's a minor incompatibility issue for a query that may be relying on such an exception being thrown in the filtered case -that would receive empty results with this approach. But IMO that shouldn't be very common. So this approach sounds good to me overall in the interest of consistency.

[findepi]

@electrum @martint curious about your thoughts on this

[electrum]

@lxynov introduced new listing APIs as part of the table redirection work. I need to do the final review of that and will see how this relates.

We ignore failures when listing since the alternative is an unusable system due to a single bad table. This is obviously different than our stance on not returning incorrect results from a query, but these are different operations with different user exceptions.

Part of the problem is that we haven’t documented this expectation for the APIs. I think the result in regard to exceptions should be consistent regardless of filtering. Also, tangentially related, IIRC filtering on these APIs is best effort and not required, but this might not be documented.

[findepi]

@lxynov introduced new listing APIs as part of the table redirection work. I need to do the final review of that and will see how this relates.

ref https://github.com/trinodb/trino/pull/5160

Part of the problem is that we haven’t documented this expectation for the APIs.

We should have a repository for such decisions. it doesn't need to be very structured at first, we can structure later and if needed.

I think the result in regard to exceptions should be consistent regardless of filtering. Also, tangentially related, IIRC filtering on these APIs is best effort and not required, but this might not be documented.

To maintain this, we would have some way to handle situations when code like MetadataListing decides to switch from one connector API to another. Nothing complex, but we need some agreed-upon pattern for this, to avoid every connector being unique.

[losipiuk]

Problem came back

@phd3, @findepi, @kokosing, @electrum, @hashhar, @martint The topic recently struck back so I want to revive the discussion.

The problem with unparsable Hive views may occur when Trino is rendering system.jdbc.columns. This is due to the listTableColumns() call here which eventually calls out to HiveMetadata.getView() for a single view (and there is no exception filtering in place then). But the problem is more general. It is not limited to parsability of Hive views. Any table/view with broken metadata can make entity listing unusable.

Another part of the puzzle is that different consumers of the ConnectorMetadata API (like getView() from the example above) have different expectations when it comes to error reporting. E.g

• when the method is used in column listing code, we want to ignore views with broken metadata (but probably still consume transient errors like connectivity issues)
• when the method is called from query planning code, we want to consume all the errors.

Just ignoring all the errors thrown from getXXX in lister consumer code (as suggested here) seems too simplistic. It is up to the connector to distinguish if the problem it stumbled upon is because the table/view is misdefined or is it a transient error (like connectivity issue) that should be (probably) always reported to the caller.

In offline discussion, @electrum suggested that we should extend the ConnectorMetadata SPI such that consumer may specify expected exception handling behavior. And the expectation is satisfied on the connector side where we have a full understanding of the nature of an error.

Solution?

As a base (a strawman probably) to start a discussion what do you think of changing the SPI in the following way:

• introduce FailurePropagation enum

  enum FailurePropagation {
  ALL, // propagate all failures to the caller
  ONLY_TRANSIENT, // propagate only transient errors like connectivity issues to the caller
  NONE // propagate no errors to the caller
  }

• add overrides for ConnectorMetadata methods, adding extra failurePropagation argument. Methods to be altered:
  • getView
  • getViews
  • getMaterializedView
  • getMaterializedViews
  • list* methods (we can start with listTables, listViews, listMaterializedViews)
• By default:
  • old getXXX(...) calls out to new getXXX(..., ALL)
  • old listXXX(...) calls out to new listXXX(..., ONLY_TRANSIENT)
  • this should mostly match current semantics
• for getXXX methods if failure is filtered out the method returns Optional.empty() (do we need more information in the return, or maybe throw specific exception to reacted upon in the caller code)
• for listXXX methods we filter out entries for which we encountered ignorable failure
• we update connectors code
• we start using new overrides in listing code and code used for rendering information_schema/system.jdbc tables.

[electrum]

@losipiuk thanks for starting this discussion again. When would NONE be used? Or put another way, what operation would want to ignore all errors, including connectivity issues (i.e. entire catalog is effectively offline)?

I'd like to look at this from the other end of the problem. Let's enumerate all of the different SQL features such as DESCRIBE, SHOW TABLES, information_schema, etc., that use metadata and discuss what the expected behavior should be for various types of errors. For example, should DESCRIBE (operation on a single table) fail for any error?

[tooptoop4]

relates to https://github.com/trinodb/trino/pull/3152

[losipiuk]

When would NONE be used

Good point. Probably not needed. I added it as it felt natural but I agree we should have it dropped if is not obviously usable.

Let's enumerate all of the different SQL features such as DESCRIBE, SHOW TABLES, information_schema, etc., that use metadata and discuss what the expected behavior should be for various types of errors. For example, should DESCRIBE (operation on a single table) fail for any error?

Attempt captured in the table below:

SQL Feature	Accessed SPIs	Failure propagation mode	Notes
information_schema.applicable_roles	CM.listApplicableRoles()	ALL	pick safer option for security related API
information_schema.columns	CM.redirectTable() CM.streamTableColumns() CM.getTableMetadata() CM.getView() CM.getViews() CM.getMaterializedView() CM.getMaterializedViews()	ONLY_TRANSIENT	In case of a permanent error whole table should be filtered out (not individuals columns)
information_schema.enabled_roles	?	ALL	pick safer option for security related API
information_schema.role_authorization_descriptors	?	ALL	pick safer option for security related API
information_schema.roles	?	ALL	pick safer option for security related API
information_schema.schemata	CM.listSchemaNames()	ONLY_TRANSIENT
information_schema.table_privileges	?	ALL	pick safer option for security related API
information_schema.tables	CM.getMaterializedView() CM.getView() CM.getTableHandle() CM.listTables() CM.listViews()	ONLY_TRANSIENT
information_schema.views	CM.getView() CM.getViews()	ONLY_TRANSIENT
DESCRIBE table	see SHOW COLUMNS		alias for SHOW COLUMNS FROM
SHOW CATALOGS	-	N/A	no SPI calls
SHOW COLUMNS FROM table LIKE pattern	CM.redirectTable() CM.getMaterializedView() CM.getView() CM.getTableMetadata()	ALL(?)
SHOW CREATE MATERIALIZED VIEW	CM.getMaterializedView()	ALL	relates to single entity (filtering out columns makes little sense)
SHOW CREATE SCHEMA	CM.getSchemaProperties() CM.getSchemaOwner()	ALL	relates to single entity
SHOW CREATE TABLE	CM.redirectTable() CM.getTableMetadata()	ALL	relates to single entity (filtering out columns makes little sense)
SHOW CREATE VIEW	CM.getView()	ALL	relates to single entity (filtering out columns makes little sense)
SHOW FUNCTIONS	-	N/A	no SPI calls
SHOW GRANTS	?	ALL	pick safer option for security related API
SHOW ROLE GRANTS	?	ALL	pick safer option for security related API
SHOW ROLES	?	ALL	pick safer option for security related API
SHOW SCHEMAS	-	ONLY_TRANSIENT	rewrite to information_schema.schemata
SHOW TABLES	-	ONLY_TRANSIENT	rewrite to information_schema.tables
system.jdbc.attributes	-	N/A	empty result
system.jdbc.catalogs	-	N/A	no SPI calls
system.jdbc.columns	CM.redirectTable() CM.streamTableColumns() CM.streamTableColumns() CM.getTableMetadata() CM.getView() CM.getViews() CM.getMaterializedView() CM.getMaterializedViews() CM.getTableHandle() CM.listTables() CM.listSchemaNames()	ONLY_TRANSIENT	In case of a permanent error whole table should be filtered out (not individuals columns)
system.jdbc.procedure_columns	-	N/A	empty result
system.jdbc.procedures	-	N/A	empty result
system.jdbc.pseudo_columns	-	N/A	empty result
system.jdbc.schemas	CM.listSchemaNames()	ONLY_TRANSIENT
system.jdbc.super_tables	-	N/A	empty result
system.jdbc.super_types	-	N/A	empty result
system.jdbc.table_types	-	N/A	constant result
system.jdbc.tables	CM.listViews() CM.getMaterializedView() CM.getView() CM.getTableHandle() CM.listTables()	ONLY_TRANSIENT
system.jdbc.types	-	N/A	no SPI calls
system.jdbc.udts	-	N/A	empty result

Note the table here shows that for full coverage we need to add failurePropagation argument to more methods than listed above

Next steps

I would love to focus first on:

• information_schema.columns
• information_schema.tables (and SHOW TABLES)
• information_schema.views
• system.jdbc.columns
• system.jdbc.tables

As problems with those seem to impact users most often.

In the meantime let's discuss other features.

I will update the table above as the discussion progresses.

cc: @findepi, @electrum

[electrum]

For listing tables and columns, there are way too many different APIs. We should have a single API for each operation that returns all the necessary information and supports the filtering needed for performance.

The current code that deals with schema prefixes, table vs view handling, etc. all needs to go away. The connector should be called once and return a lazy stream or similar.

The new APIs can have default implementations in the SPI that call the old APIs. Nothing in the engine should call the old APIs. We can then convert all the connectors and deprecate the old ones.

[losipiuk]

For listing tables and columns, there are way too many different APIs

For tables, yes (separate API for tables/views/materialized views). Not really the case for columns, though. We have just CM.streamTableColumns. We also sometimes use CM.getTableMetadata in column listing flow, but this is the API we cannot get rid of. We may just stop calling it when we list table columns.

We should have a single API for each operation that returns all the necessary information and supports the filtering needed for performance. The current code that deals with schema prefixes, table vs view handling, etc. all needs to go away. The connector should be called once and return a lazy stream or similar.

That seems reasonable.

Although I see some issues that should be talked out:

[1] Different callers may have very different requirements regarding returned output.

For example regarding tables:

• for information_schema.tables we just need names of all tables/views/materialized views
• for information_schema.views we need just views, but we need more information than i_s.tables; we need view_definition For columns situation seems better.

This can be handled by elastic filtering capabilities in the new API. But that leads us to [2]

[2] Making API elastic. E.g allowing the caller to specify complex filters as parameters put a lot of burden on connector implementations. Every connector will have to implement the API, and the more complex it is, the more (repeatable) work it is. And more places to make a mistake. The benefit of what we have right now is that the connector just provides simple building blocks and a huge part of logic (which otherwise would have to be repeated) exists in MetadataManager and in MetadataListing.

[3] Longer time to market. Probably we should not care and just go with what we believe is better for code quality and future maintenance (unless cost difference is prohibitive). Putting still for completeness.

@findepi something I missed. I am mostly concerned by [2], but maybe I am not seeing something, and that would not be an issue in real world. WDYT?

[electrum]

Not really the case for columns, though. We have just CM.streamTableColumns

Sorry, I was referring to both information_schema.columns and system.jdbc.columns. I think the list should be the same for both, since they both call MetadataListing.listTableColumns() which then calls Metadata.listTableColumns() which then does the extra calls for views and materialized views.

The CMlistTables(), CM.listViews() and CM.listMaterializedViews(), are designed wrong for this requirement. They should take SchemaTablePrefix rather than Optional<String> schema to allow filtering for a specific table. Though if we change it, we might make SchemaTablePrefix a more extensible object with a Predicate allowing flexible filtering on schema or table names.

Also, the fact that tables/views/mviews are all different and then merged in the engine with preference seems error prone. That was a mistake or consequence of the fact that views were added on after we already had tables.

Every connector will have to implement the API, and the more complex it is, the more (repeatable) work it is. And more places to make a mistake. The benefit of what we have right now is that the connector just provides simple building blocks

I agree that we don't want to enforce a large burden on the connector. However, the simple building blocks don't work when we have different requirements for error handling -- the connector has to handle that correctly. I think we could make this easy enough to implement for the simple case by making filtering optional, while allowing efficient implementations for connectors that can do so.

We have to change the APIs anyway, so I think it makes sense to think about (1) the requirements (2) what the APIs could look like (3) how hard it is to implement.

BTW, thanks for putting together the above table. That is really helpful.

[losipiuk]

Sorry, I was referring to both information_schema.columns and system.jdbc.columns. I think the list should be the same for both, since they both call MetadataListing.listTableColumns() which then calls Metadata.listTableColumns() which then does the extra calls for views and materialized views.

Oh yeah, good point, omission from my side (updated). Still list for system.jdbc.columns is longer due to fact that the code is calling out to listTables and then listing columns for each individual table. Probably something to be fixed anyway, we do not want that even if we have new APIs in place. I see how hard it is to send PR for that. Separately I will try to prototype the new API shape and put up a link here for further discussion.