Skip to content

fix(duckdb): use SET GLOBAL for all settings to ensure pool consistency#172

Merged
xe-nvdk merged 1 commit intoBasekick-Labs:mainfrom
khalid244:fix/use-set-global-for-pool-settings
Jan 29, 2026
Merged

fix(duckdb): use SET GLOBAL for all settings to ensure pool consistency#172
xe-nvdk merged 1 commit intoBasekick-Labs:mainfrom
khalid244:fix/use-set-global-for-pool-settings

Conversation

@khalid244
Copy link
Copy Markdown

Summary

Change all SET statements to SET GLOBAL to ensure settings apply to all connections in the pool.

Arc uses a connection pool (SetMaxOpenConns), so SET only affects one connection while other pooled connections use defaults.

Changes

  • SET memory_limitSET GLOBAL memory_limit
  • SET threadsSET GLOBAL threads
  • SET enable_object_cacheSET GLOBAL enable_object_cache
  • SET preserve_insertion_orderSET GLOBAL preserve_insertion_order
  • SET cache_httpfs_typeSET GLOBAL cache_httpfs_type
  • SET cache_httpfs_max_in_mem_cache_block_countSET GLOBAL ...
  • SET cache_httpfs_in_mem_cache_block_timeout_millisecSET GLOBAL ...
  • SET custom_profiling_settingsSET GLOBAL custom_profiling_settings

Test plan

  • Build passes
  • All database tests pass
  • No functional change, just scope correction

Copy link
Copy Markdown
Member

@xe-nvdk xe-nvdk left a comment

Choose a reason for hiding this comment

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

Good catch on the connection pool issue — SET only applies to the connection that runs it, so other pooled connections would use defaults. SET GLOBAL is the right fix for most of these.

Two issues:

1. custom_profiling_settings should stay SET, not SET GLOBAL

QueryWithProfileContext is meant to enable profiling for a specific diagnostic query. Making it SET GLOBAL enables profiling on all connections, adding overhead to every query — not just the one being profiled. This should remain SET (or ideally be scoped to just that connection/transaction).

2. Conflicts with PR #171

This PR still references enable_object_cache (line 140), but PR #171 replaces it with parquet_metadata_cache. These two PRs will conflict — please coordinate with #171 and rebase after it merges.

Change configuration settings to SET GLOBAL to ensure they apply to all
connections in the pool, not just the one executing the statement.

Keep custom_profiling_settings as SET since it's used for per-query
diagnostics and should not affect all connections.
Copy link
Copy Markdown
Member

@xe-nvdk xe-nvdk left a comment

Choose a reason for hiding this comment

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

Thanks for catching the connection pool scoping issue, Khalid — this is a solid fix. We'll handle the rebase against #171 on our end and apply the remaining SET GLOBAL changes to the new settings. Merging as-is and fixing up locally.

@xe-nvdk xe-nvdk merged commit b616932 into Basekick-Labs:main Jan 29, 2026
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.

2 participants