Skip to content

Conversation

@theory
Copy link
Collaborator

@theory theory commented Dec 13, 2025

Compare with #95.

A new structure, kv_list, represents a list of key/value pairs. It does not use an array of structs for each pair, because a custom GUC value must be allocated as a single block,

The kv_list constructor takes a Postgres List argument, iterates over the keys and values, and then allocates a block of memory for them (with options to use palloc, malloc, or guc_malloc (which falls back on malloc prior to Postgrs 16)). It then iterates over them again to pack them into the memory space beyond the kv_list struct itself. The design follows the precedent of the ConvertTimeZoneAbbrevs() function in Postgres's datetime.c.

Use it in chfdw_check_settings_guc() to create a kv_list from the List returned by chfdw_parse_options() and assign it to **extra. Postgres hands this pointer to chfdw_settings_assign_hook(), where we assign it to the ch_session_settings_list global, which can then be fetched via chfdw_get_session_settings().

Of course this would be a tricky interface to iterate manually, so also add another struct, kv_iter, that handles the logic of iterating through the keys and values in a kv_list. Since it sets up the values for each iteration of the loop, the logic using it is simpler than the previous use of List, as seen in the http and binary engines.

While at it, cChange chfdw_parse_options() to use pstrdup() instead of strdup, since its memory is handled by a Postgres memory context (and surely was leaking before).

@theory theory requested a review from serprex December 13, 2025 19:42
@theory theory self-assigned this Dec 13, 2025
@theory theory force-pushed the kv-list-guc branch 2 times, most recently from f42492b to a79b05a Compare December 13, 2025 19:52
@serprex
Copy link
Member

serprex commented Dec 13, 2025

I prefer this to #95 but see merit of both

Base automatically changed from fix-tpch-links to main December 15, 2025 21:36
@theory theory force-pushed the kv-list-guc branch 2 times, most recently from 5825410 to 4c379aa Compare December 15, 2025 23:00
A new structure, kv_list, represents a list of key/value pairs. It does
not use an array of structs for each pair, because a custom GUC value
must be allocated as a single block,

The `kv_list` constructor takes a Postgres `List` argument, iterates
over the keys and values, and then allocates a block of memory for them
(with options to use `palloc`, `malloc`, or `guc_malloc` (which falls
back on `malloc` prior to Postgrs 16)). It then iterates over them again
to pack them into the memory space beyond the `kv_list` struct itself.
The design follows the precedent of the `ConvertTimeZoneAbbrevs()`
function in Postgres's `datetime.c`.

Use it in `chfdw_check_settings_guc()` to create a `kv_list` from the
`List` returned by `chfdw_parse_options()` and assign it to `**extra`.
Postgres hands this pointer to `chfdw_settings_assign_hook()`, where we
assign it to the `ch_session_settings_list` global, which can then be
fetched via `chfdw_get_session_settings()`.

Of course this would be a tricky interface to iterate manually, so also
add another struct, `kv_iter`, that handles the logic of iterating
through the keys and values in a `kv_list`. Since it sets up the values
for each iteration of the loop, the logic using it is simpler than the
previous use of `List`, as seen in the http and binary engines.

While at it, cChange `chfdw_parse_options()` to use `pstrdup()` instead
of `strdup`, since its memory is handled by a Postgres memory context
(and surely was leaking before).
@theory theory merged commit 709c07f into main Dec 15, 2025
34 checks passed
@theory theory deleted the kv-list-guc branch December 15, 2025 23:44
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