Skip to content

Conversation

@theory
Copy link
Collaborator

@theory theory commented Dec 3, 2025

Add a new struct, ch_query, and replace all instances of passing a char * with passing ch_query. This allows it to included additional fields relevant to the query, starting here with sql and settings, the latter currently being a PostgreSQL List of the settings parsed from pg_clickhouse.session_settings. Provide new_query() to keep its construction in one place; in the future it might take parameters to merge additional settings fetched from a server or user mapping to merge into the GUC's settings.

Update ch_binary_simple_query() to iterate over the list of settings in the ch_query and apply them to the query as a QuerySettings map. Add a comment to do the same for inserts, held until clickhouse-cpp#453 is merged and released.

Update ch_http_simple_query() to use curl_url_set() to build each request URL, starting with the base URL and query ID, then adding another query parameter for each item in ch_query.settings. This also eliminates the need for ch_http_connection_t.base_url_len.

Add tests to ensure consistent application of a variety of settings via both the binary and http engines.

While at it, fix a bug where http query results treated an empty string as NULL and add a test for it. Also replace a buffer allocation with simple use of psprintf(), which is automatically cleaned up in the current PostgreSQL memory context.

Other bits:

  • Document pg_clickhouse.session_settings
  • Rename connect.h to engine.h, which better describes its purpose providing objects and functions used by both the http and binary engines
  • Remove the unused cursor_free_method field from the libclickhouse_methods struct

@theory theory force-pushed the apply-settings branch 4 times, most recently from 918c1c1 to 3721f7e Compare December 4, 2025 18:29
@theory theory marked this pull request as ready for review December 4, 2025 18:29
@theory theory self-assigned this Dec 4, 2025
@theory theory requested a review from serprex December 4, 2025 18:29
@theory theory added the enhancement New feature or request label Dec 4, 2025
@theory theory changed the title Apply settings Apply session_settings to every query Dec 4, 2025
@iskakaushik
Copy link
Collaborator

Nice refactor. Few things:

  1. http.c - the psprintf() calls in the foreach loop for URL params should be pfree()'d
    after each curl_url_set(). Same for the query_id one. Curl copies the string so we can free
    immediately.
  2. kill_query() lost its pfree() - the old code freed the query string, new code doesn't.
    Might need to inline the macro instead of using new_query() to get the sql pointer back for
    freeing.
  3. chfdw_construct_create_tables() reassigns query.sql in a loop without freeing previous -
    minor but adds up.

Nits:

  • engine.h comment says "ch_connection_details" but should say "ch_query"
  • The (List *) cast in binary.cpp:180 is fine but a one-liner comment explaining it's
    because foreach needs non-const would help future readers

Rest looks good, especially the empty string fix and using curl's URL API.

@theory
Copy link
Collaborator Author

theory commented Dec 5, 2025

Nice refactor. Few things:

  1. http.c - the psprintf() calls in the foreach loop for URL params should be pfree()'d
    after each curl_url_set(). Same for the query_id one. Curl copies the string so we can free
    immediately.

Makes sense.

  1. kill_query() lost its pfree() - the old code freed the query string, new code doesn't.
    Might need to inline the macro instead of using new_query() to get the sql pointer back for
    freeing.

It shouldn't need to pfree(); the memory will be freed when the memory context is freed. That's the benefit of using palloc() and friends.

  1. chfdw_construct_create_tables() reassigns query.sql in a loop without freeing previous -
    minor but adds up.

Check.

Nits:

  • engine.h comment says "ch_connection_details" but should say "ch_query"
  • The (List *) cast in binary.cpp:180 is fine but a one-liner comment explaining it's
    because foreach needs non-const would help future readers

Rest looks good, especially the empty string fix and using curl's URL API.

Thanks!

Add a new struct, `ch_query`, and replace all instances of passing a
`char *` with passing `ch_query`. This allows it to included additional
fields relevant to the query, starting here with `sql` and `settings`,
the latter currently being a PostgreSQL `List` of the settings parsed
from `pg_clickhouse.session_settings`. Provide `new_query()` to keep its
construction in one place; in the future it might take parameters to
merge additional settings fetched from a server or user mapping to merge
into the GUC's settings.

Update `ch_binary_simple_query()` to iterate over the list of settings
in the `ch_query` and apply them to the query as a `QuerySettings` map.
Add a comment to do the same for inserts, held until
[clickhouse-cpp#453] is merged and released.

Update `ch_http_simple_query()` to use `curl_url_set()` to build each
request URL, starting with the base URL and query ID, then adding
another query parameter for each item in `ch_query.settings`. This also
eliminates the need for `ch_http_connection_t.base_url_len`.

Add tests to ensure consistent application of a variety of settings via
both the binary and http engines.

While at it, fix a bug where http query results treated an empty string
as `NULL` and add a test for it. Also replace a buffer allocation with
simple use of `psprintf()`, which is automatically cleaned up in the
current PostgreSQL memory context.

Other bits:

*   Document `pg_clickhouse.session_settings`
*   Rename `connect.h` to `engine.h`, which better describes its purpose
    providing objects and functions used by both the http and binary
    engines
*   Remove the unused `cursor_free_method` field from the
    `libclickhouse_methods` struct

  [clickhouse-cpp#453]: ClickHouse/clickhouse-cpp#453
@theory
Copy link
Collaborator Author

theory commented Dec 5, 2025

Fixed @iskakaushik's issues in b015f06 (diff).

@theory theory merged commit b015f06 into main Dec 5, 2025
38 checks passed
@theory theory deleted the apply-settings branch December 5, 2025 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants