Add kv_list and refactor session_settings with it

A new structure, kv_list, represents a list of key/value pairs. Its
constructor takes a Postgres `List` argument, iterates over the keys and
values, allocates memory for each, and copies them into the struct. It
uses `malloc()` to allocate the memory, because the we store the value
in a global that mustn't be subject to the GUC memory context.

Use it in `chfdw_settings_assign_hook()` to create a `kv_list` from the
`List` returned by `chfdw_parse_options()` and assign it to the
`ch_session_settings_list` global, which can then be fetched via
`chfdw_get_session_settings()`.

The downside to this approach is that the new value must be parsed
twice. We consider this acceptable, since it means they're no longer
parsed for every query.

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

Author: theory

src/binary.cpp

@@ -173,23 +173,17 @@ static void set_resp_error(ch_binary_response_t * resp, const char * str)
  */
 static QuerySettings ch_binary_settings(const ch_query *query)
 {
-   ListCell   *lc;
-   auto res = QuerySettings{};
-   foreach (lc, (List *) query->settings)
-   {
-		/*
-		 * foreach reads a non-const, so we have to cast. Would be nice to use
-		 * foreach_ptr:
-		 *
-		 *     foreach_ptr(DefElem, setting, query->settings)
-		 *
-		 * But it's only available in Postgres 17 and later.
-		*/
-       DefElem    *setting = (DefElem *) lfirst(lc);
-       res.insert_or_assign(setting->defname, QuerySettingsField{strVal(setting->arg), 1});
-   }
+	int	   i;
+	kv_pair *pair;
+	auto res = QuerySettings{};
 
-   return res;
+	for (i = 0; i < query->settings->length; i++)
+	{
+		pair = query->settings->items[i];
+		res.insert_or_assign(pair->name, QuerySettingsField{pair->value, 1});
+	}
+
+	return res;
 }
 
 static void set_state_error(ch_binary_read_state_t * state, const char * str)

src/http.c

@@ -158,8 +158,8 @@ ch_http_simple_query(ch_http_connection_t * conn, const ch_query * query)
 	static char errbuffer[CURL_ERROR_SIZE];
 	struct curl_slist *headers = NULL;
 	CURLU	   *cu = curl_url();
-	ListCell   *lc;
-	DefElem    *setting;
+	int			i;
+	kv_pair    *pair;
 	char	   *buf = NULL;
 
 	ch_http_response_t *resp = calloc(sizeof(ch_http_response_t), 1);
@@ -178,10 +178,11 @@ ch_http_simple_query(ch_http_connection_t * conn, const ch_query * query)
 	pfree(buf);
 
 	/* Append each of the settings as a query param. */
-	foreach(lc, (List *) query->settings)
+
+	for (i = 0; i < query->settings->length; i++)
 	{
-		setting = (DefElem *) lfirst(lc);
-		buf = psprintf("%s=%s", setting->defname, strVal(setting->arg));
+		pair = query->settings->items[i];
+		buf = psprintf("%s=%s", pair->name, pair->value);
 		curl_url_set(cu, CURLUPART_QUERY, buf, CURLU_APPENDQUERY | CURLU_URLENCODE);
 		pfree(buf);
 	}

src/include/engine.h

@@ -1,7 +1,7 @@
 #ifndef CLICKHOUSE_ENGINE_H
 #define CLICKHOUSE_ENGINE_H
 
-#include "nodes/pathnodes.h"
+#include "kv_list.h"
 
 /*
  * ch_connection_details defines the details for connecting to ClickHouse.
@@ -20,10 +20,10 @@ typedef struct
  */
 typedef struct
 {
-	const char	   *sql;
-	const List	   *settings;
-}			ch_query;
+   const char     *sql;
+   const kv_list    *settings;
+}          ch_query;
 
-#define new_query(sql) {sql, chfdw_parse_options(ch_session_settings, true, false)}
+#define new_query(sql) {sql, chfdw_get_session_settings()}
 
 #endif							/* CLICKHOUSE_ENGINE_H */

src/include/fdw.h

@@ -199,7 +199,8 @@ extern void chfdw_report_error(int elevel, ch_connection conn,
 							   bool clear, const char *sql);
 
 /* in option.c */
-extern char *ch_session_settings;
+extern kv_list * chfdw_get_session_settings(void);
+
 extern void
 			chfdw_extract_options(List * defelems, char **driver, char **host, int *port,
 								  char **dbname, char **username, char **password);

src/include/kv_list.h

@@ -0,0 +1,53 @@
+#ifndef PG_CLICKHOUSE_KV_LIST_H
+#define PG_CLICKHOUSE_KV_LIST_H
+
+#include <stdbool.h>
+#include "postgres.h"
+#include "nodes/pathnodes.h"
+
+/* A key/value pair. */
+typedef struct kv_pair
+{
+	char	   *name;
+	char	   *value;
+}			kv_pair;
+
+/*
+ * A simple data structure with a list of key/value string pairs. Use
+ * new_kv_list_from_list() to create.
+ *
+ *     kv_list * pairs = new_kv_list_from_pg_list(list);
+ *     if (!kv_list)
+ *     {
+ *          printf("out of memory\n");
+ *          exit(1);
+ *     }
+ *
+ *     for (int i = 0; i < kv_list->length; i++) {
+ *         kv_pair * pair = kv_list->items[i];
+ *         printf("%s => %s\n", pair->name, pair->value);
+ *     }
+ *
+ *     kv_list_free(pairs);
+ */
+typedef struct kv_list
+{
+	int			length;
+	kv_pair		**items;
+}			kv_list;
+
+/*
+ * Create an empty kv_list, with length set to zero and items uninitialized.
+*/
+kv_list * new_empty_kv_list(void);
+
+/*
+ * Create a new kv_list from a PostgreSQL List of DefElem. Logs an error
+ * message and returns NULL on memory errors.
+*/
+kv_list * new_kv_list_from_pg_list(List * list);
+
+/* Frees the memory owned by the kv_list. */
+void kv_list_free(kv_list * pairs);
+
+#endif							/* PG_CLICKHOUSE_KV_LIST_H */

src/kv_list.c

@@ -0,0 +1,88 @@
+#include <stddef.h>
+#include "kv_list.h"
+#include "utils/elog.h"
+#include "nodes/pathnodes.h"
+#include "utils/guc.h"
+
+extern kv_list * new_empty_kv_list(void)
+{
+	kv_list    *pairs = (kv_list *) malloc(sizeof(kv_list));
+
+	if (pairs == NULL)
+	{
+		ereport(LOG, errcode(ERRCODE_FDW_OUT_OF_MEMORY), errmsg("out of memory"));
+		return NULL;
+	}
+	pairs->length = 0;
+	return pairs;
+}
+
+extern kv_list * new_kv_list_from_pg_list(List * list)
+{
+	ListCell   *lc;
+	DefElem    *elem;
+	kv_list    *pairs;
+	int			i = 0;
+
+	/* Allocate the memory for the pairs object. */
+	pairs = (kv_list *) malloc(sizeof(kv_list));
+	if (pairs == NULL)
+	{
+		ereport(LOG, errcode(ERRCODE_FDW_OUT_OF_MEMORY), errmsg("out of memory"));
+		return NULL;
+	}
+
+	/* Allocate the memory for kv_pair pointers to all the items in list. */
+	pairs->items = (kv_pair * *) malloc(list_length(list) * sizeof(kv_pair *));
+	if (pairs->items == NULL)
+	{
+		free(pairs);
+		ereport(LOG, errcode(ERRCODE_FDW_OUT_OF_MEMORY), errmsg("out of memory"));
+		return NULL;
+	}
+	pairs->length = list_length(list);
+
+	/* Copy the values from list into pairs. */
+	foreach(lc, list)
+	{
+		kv_pair    *pair = malloc(sizeof(kv_pair));
+
+		if (!pair)
+		{
+			kv_list_free(pairs);
+			ereport(LOG, errcode(ERRCODE_FDW_OUT_OF_MEMORY), errmsg("out of memory"));
+			return NULL;
+		}
+		elem = (DefElem *) lfirst(lc);
+		pair->name = strdup(elem->defname);
+		pair->value = strdup(strVal(elem->arg));
+		if (!pair->name || !pair->value)
+		{
+			kv_list_free(pairs);
+			ereport(LOG, errcode(ERRCODE_FDW_OUT_OF_MEMORY), errmsg("out of memory"));
+			return NULL;
+		}
+		pairs->items[i] = pair;
+		i++;
+	}
+
+	return pairs;
+}
+
+extern void
+kv_list_free(kv_list * pairs)
+{
+	if (pairs == NULL)
+		return;
+	for (int i = 0; i < pairs->length; i++)
+		if (pairs->items[i] != NULL)
+		{
+			if (pairs->items[i]->name)
+				free(pairs->items[i]->name);
+			if (pairs->items[i]->value)
+				free(pairs->items[i]->value);
+			free(pairs->items[i]);
+		}
+	free(pairs->items);
+	free(pairs);
+}

src/option.c

@@ -15,6 +15,7 @@
 #include "postgres.h"
 
 #include "fdw.h"
+#include "kv_list.h"
 
 #include "access/reloptions.h"
 #include "catalog/pg_foreign_server.h"
@@ -25,6 +26,7 @@
 #include "nodes/makefuncs.h"
 #include "utils/guc.h"
 #include "utils/varlena.h"
+#include "utils/builtins.h"
 
 static char *DEFAULT_DBNAME = "default";
 
@@ -66,7 +68,8 @@ static const ChFdwOption ch_options[] =
 /*
  * GUC parameters
  */
-char	   *ch_session_settings = NULL;
+static char *ch_session_settings = NULL;
+static kv_list * ch_session_settings_list = NULL;
 
 /*
  * Helper functions
@@ -469,35 +472,79 @@ chfdw_parse_options(const char *options_string, bool with_comma, bool with_equal
 		/*
 		 * Now that we have the name and the value, store the record.
 		 */
-		options = lappend(options, makeDefElem(strdup(pname), (Node *) makeString(strdup(pval)), -1));
+		options = lappend(options, makeDefElem(pstrdup(pname), (Node *) makeString(pstrdup(pval)), -1));
 	}
 
 	return options;
 }
 
 /*
- * check_settings_guc
- *
+ * Return the current list of current session settings parsed from the
+ * session_settings GUC.
+ */
+kv_list    *
+chfdw_get_session_settings()
+{
+	return ch_session_settings_list;
+}
+
+/*
  * Validates the provided settings key/value pairs.
  */
 static bool
-check_settings_guc(char **newval, void **extra, GucSource source)
+chfdw_check_settings_guc(char **newval, void **extra, GucSource source)
 {
 	/*
 	 * The value may be an empty string, so we have to accept that value.
+	 * Leave extra unset; chfdw_settings_assign_hook() will assign NULL to
+	 * ch_session_settings_list.
 	 */
 	if (*newval == NULL || *newval[0] == '\0')
 		return true;
 
-	/*
-	 * Make sure we can parse the settings.
-	 */
+	/* Make sure we can parse the settings. */
 	chfdw_parse_options(*newval, true, false);
 
+	/* All good if no errors. */
+	return true;
+}
+
+/*
+ * Assigns the kv_list stored in extra to the ch_session_settings_list global.
+ */
+static void
+chfdw_settings_assign_hook(const char *newval, void *extra)
+{
 	/*
-	 * All good if no error.
+	 * From PostgreSQL's POV: (a) failure here is not acceptable, and (b) it
+	 * is not necessarily called inside a transaction, so e.g. catalog lookups
+	 * are not okay. IOW, keep it as simple as possible, and leave error
+	 * returning behavior to chfdw_check_settings_guc(). Therefore wrap the
+	 * parsing in PG_TRY() to prevent errors.
 	 */
-	return true;
+	kv_list_free(ch_session_settings_list);
+	PG_TRY();
+	{
+		if (newval == NULL || newval[0] == '\0')
+			ch_session_settings_list = new_empty_kv_list();
+		else
+		{
+			List	   *list = chfdw_parse_options(newval, true, false);
+
+			ch_session_settings_list = new_kv_list_from_pg_list(list);
+		}
+	}
+	PG_CATCH();
+	{
+		/*
+		 * This should not happen, since chfdw_check_settings_guc successfully
+		 * parsed the options, but we could get here due to memory errors.
+		 */
+		ereport(LOG,
+				(errcode(ERRCODE_FDW_ERROR),
+				 errmsg("unexpected error parsing \"%s\"", newval)));
+	}
+	PG_END_TRY();
 }
 
 /*
@@ -522,8 +569,8 @@ _PG_init(void)
 							   "join_use_nulls 1, group_by_use_nulls 1, final 1",
 							   PGC_USERSET,
 							   0,
-							   check_settings_guc,
-							   NULL,
+							   chfdw_check_settings_guc,
+							   chfdw_settings_assign_hook,
 							   NULL);
 
 #if PG_VERSION_NUM >= 150000