Skip to content

Conversation

Tishj
Copy link
Collaborator

@Tishj Tishj commented Sep 2, 2025

This PR is an attempt to fix performance issues caused by nested namespaces.

As the title says, it replaces the current logic of running "LoadEntries" when first hitting the schema entry set in a transaction, which does a "list namespaces" request.

The replacement being to instead do a HEAD request to check the existence of the namespace in the lookup.
This is done for both IRCSchemaSet::GetEntry and IRCTableSet::GetEntry

@carlopi carlopi self-requested a review September 2, 2025 11:52
Copy link
Collaborator

@Tmonster Tmonster left a comment

Choose a reason for hiding this comment

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

Thanks! Looks good, but seems like Polaris is failing? I can imagine it's because they don't support the HEAD request to check if tables exist. Maybe I can take a look using our snowflake catalog

@Tmonster
Copy link
Collaborator

Tmonster commented Sep 2, 2025

So I think for Polaris you need to check for if (response->Success() || response->status == HTTPStatusCode::NoContent_204) {

There seems to be another error though, looking into that

@Tmonster
Copy link
Collaborator

Tmonster commented Sep 2, 2025

The other error is because the httpfs submodule pointer on main does not have the code to create a secret with just a bearer token. Working on that. duckdb/duckdb-httpfs#108 needs to merge, then merge v1.4-andium into main, then update the submodule pointer

Copy link
Collaborator

@Tmonster Tmonster left a comment

Choose a reason for hiding this comment

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

LGTM now.
@Tishj feel free to look at my last commit, otherwise I think we can merge

@carlopi
Copy link
Contributor

carlopi commented Sep 4, 2025

I added myself as reviewer, but that's not really adding much. This seems sound and great to have

@Tmonster
Copy link
Collaborator

Tmonster commented Sep 4, 2025

Awesome to have this in 👍

@Tmonster Tmonster merged commit 0522efe into duckdb:main Sep 4, 2025
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants