From 818d3f95b23ca62e35706092d4f6223f58c59b52 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Tue, 11 Aug 2026 14:46:46 +0100 Subject: [PATCH 1/3] Reuse the curl handle and grow the response buffer geometrically Benchmarked before changing anything, as the issue asked. provider_do_curl_request() built a fresh handle for every request, and a fresh handle knows of no connection, so each batch opened its own. Measured against a local stub: 200 requests made 200 TCP connections. With the handle kept for the life of the backend that becomes 200 requests over 1 connection, and 274ms falls to 189ms over loopback, where establishing a connection is about as cheap as it can be. Against a real provider each avoided connection also avoids a TLS handshake, so the saving there is one to two extra round trips per batch to a host that is rarely nearby. curl_easy_reset() clears the options without disturbing the connection pool, which is what lets reuse survive between requests. That is the load-bearing assumption of the whole change, so it was verified rather than taken from the documentation: the connection count above is the evidence. The handle is deliberately never cleaned up, since it lives as long as the backend and an idle keep-alive connection is closed by the server at its own timeout. The response buffer grew to exactly the size wanted on every write callback, which is a repalloc for every chunk curl delivers, each liable to copy everything received so far. It now tracks capacity and doubles, so a response of any size costs a handful of reallocations rather than hundreds. A note for whoever benchmarks this next. A first attempt showed the change making things twenty-six times slower, which was the measuring instrument rather than the code: Python's http.server does not set TCP_NODELAY, so once keep-alive put every request on one socket each one paid the 40ms delayed-ACK stall, 200 of which is the 8.4 seconds observed. Setting disable_nagle_algorithm on the stub removed it entirely. Closes #28 --- src/provider_common.c | 73 ++++++++++++++++++++++++++++++++++++------- src/provider_common.h | 3 +- 2 files changed, 63 insertions(+), 13 deletions(-) diff --git a/src/provider_common.c b/src/provider_common.c index 6a773b7..e9ee0d4 100644 --- a/src/provider_common.c +++ b/src/provider_common.c @@ -17,6 +17,15 @@ #include "utils/memutils.h" +/* + * Per-backend curl handle, kept so that its connection pool outlives a + * single request. See provider_do_curl_request() for why. + */ +static CURL *cached_curl = NULL; + +/* Initial response buffer size; embedding responses run to megabytes. */ +#define RESPONSE_BUFFER_INITIAL 32768 + /* * Curl write callback - accumulates response data into a ResponseBuffer. */ @@ -25,10 +34,28 @@ provider_write_callback(void *contents, size_t size, size_t nmemb, void *userp) { size_t realsize = size * nmemb; ResponseBuffer *mem = (ResponseBuffer *) userp; + size_t needed = mem->size + realsize + 1; + + /* + * Grow geometrically rather than to the exact size wanted. curl hands + * the body over in chunks of its own choosing, so sizing precisely meant + * a repalloc per chunk for the whole response: an embedding batch runs to + * megabytes, which is hundreds of them, each liable to copy everything + * received so far. Doubling makes that a handful. + */ + if (needed > mem->capacity) + { + size_t newcap = (mem->capacity > 0) ? mem->capacity : needed; + + while (newcap < needed) + newcap *= 2; - /* repalloc never returns NULL - it throws on allocation failure */ - mem->data = repalloc(mem->data, mem->size + realsize + 1); - /* flawfinder: ignore - buffer was realloced to mem->size + realsize + 1 */ + /* repalloc never returns NULL - it throws on allocation failure */ + mem->data = repalloc(mem->data, newcap); + mem->capacity = newcap; + } + + /* flawfinder: ignore - buffer holds at least mem->size + realsize + 1 */ memcpy(&(mem->data[mem->size]), contents, realsize); /* nosemgrep */ mem->size += realsize; mem->data[mem->size] = 0; @@ -366,18 +393,42 @@ provider_do_curl_request(const char *url, const char *auth_header, long response_code; /* Initialize response buffer */ - response_out->data = palloc(1); + response_out->capacity = RESPONSE_BUFFER_INITIAL; + response_out->data = palloc(response_out->capacity); response_out->data[0] = '\0'; response_out->size = 0; - curl = curl_easy_init(); - if (!curl) + /* + * Reuse this backend's handle rather than making a new one per request. + * A fresh handle knows nothing of any connection, so every batch paid for + * a TCP connection and, against a real provider, a TLS handshake as well: + * one or two extra round trips to a host that is usually not nearby, on + * every batch of a table that may have thousands. Keeping the handle + * keeps curl's connection pool with it, so subsequent batches to the same + * endpoint reuse the established connection. + * + * curl_easy_reset() clears the options set below without disturbing that + * pool, which is what makes reuse survive the reset. + * + * The handle is deliberately never cleaned up: it lives as long as the + * backend, and an idle keep-alive connection is closed by the server at + * its own timeout, so nothing is held open indefinitely. + */ + if (cached_curl == NULL) { - *error_msg = pstrdup("Failed to initialize libcurl"); - pfree(response_out->data); - response_out->data = NULL; - return false; + cached_curl = curl_easy_init(); + if (!cached_curl) + { + *error_msg = pstrdup("Failed to initialize libcurl"); + pfree(response_out->data); + response_out->data = NULL; + return false; + } } + else + curl_easy_reset(cached_curl); + + curl = cached_curl; /* Set up headers */ headers = curl_slist_append(headers, "Content-Type: application/json; charset=utf-8"); @@ -402,13 +453,11 @@ provider_do_curl_request(const char *url, const char *auth_header, *error_msg = psprintf("curl_easy_perform() failed: %s", curl_easy_strerror(res)); curl_slist_free_all(headers); - curl_easy_cleanup(curl); return false; } curl_easy_getinfo(curl, CURLINFO_RESPONSE_CODE, &response_code); curl_slist_free_all(headers); - curl_easy_cleanup(curl); if (response_code != 200) { diff --git a/src/provider_common.h b/src/provider_common.h index eb9f61d..cb3437e 100644 --- a/src/provider_common.h +++ b/src/provider_common.h @@ -19,7 +19,8 @@ typedef struct { char *data; - size_t size; + size_t size; /* bytes of response held, excluding the terminator */ + size_t capacity; /* bytes allocated in data, including the terminator */ } ResponseBuffer; /* From 7b7ce3452abeec6f59a046265e66db08e8fca960 Mon Sep 17 00:00:00 2001 From: Mason Sharp Date: Tue, 11 Aug 2026 16:06:13 -0700 Subject: [PATCH 2/3] Stop the write callback throwing from inside curl repalloc raises on failure and the callback runs inside curl_easy_perform(), so the longjmp abandoned the handle mid-transfer. With the handle now cached for the life of the backend, every later request reused the wreckage: measured against a stub, a response over MaxAllocSize failed and then every subsequent request in that backend failed with "Failed initialization". The callback now checks the size itself, uses MCXT_ALLOC_NO_OOM, and returns short so curl reports CURLE_WRITE_ERROR through the existing error path. Doubling that overshoots MaxAllocSize retries at the exact size, restoring the ~1GB ceiling that geometric growth had halved. --- src/provider_common.c | 46 +++++++++++++++++++++++++++++++++++++++---- src/provider_common.h | 3 +++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/provider_common.c b/src/provider_common.c index e9ee0d4..f78374a 100644 --- a/src/provider_common.c +++ b/src/provider_common.c @@ -42,16 +42,43 @@ provider_write_callback(void *contents, size_t size, size_t nmemb, void *userp) * a repalloc per chunk for the whole response: an embedding batch runs to * megabytes, which is hundreds of them, each liable to copy everything * received so far. Doubling makes that a handful. + * + * Nothing here may throw. This runs inside curl_easy_perform(), and a + * PostgreSQL error is a longjmp, which would abandon curl's handle and its + * connection mid-transfer. The handle is cached for the life of the + * backend, so every later request would go on reusing the wreckage. On + * failure we record why and return short instead, which curl reports as + * CURLE_WRITE_ERROR through the caller's existing error path. */ if (needed > mem->capacity) { size_t newcap = (mem->capacity > 0) ? mem->capacity : needed; + char *newdata; while (newcap < needed) newcap *= 2; - /* repalloc never returns NULL - it throws on allocation failure */ - mem->data = repalloc(mem->data, newcap); + /* + * Doubling can overshoot MaxAllocSize while the response itself would + * still fit, so try the exact size before giving up on it. + */ + if (!AllocSizeIsValid(newcap)) + newcap = needed; + + if (!AllocSizeIsValid(newcap)) + { + mem->alloc_failed = true; + return 0; + } + + newdata = repalloc_extended(mem->data, newcap, MCXT_ALLOC_NO_OOM); + if (newdata == NULL) + { + mem->alloc_failed = true; + return 0; + } + + mem->data = newdata; mem->capacity = newcap; } @@ -397,6 +424,7 @@ provider_do_curl_request(const char *url, const char *auth_header, response_out->data = palloc(response_out->capacity); response_out->data[0] = '\0'; response_out->size = 0; + response_out->alloc_failed = false; /* * Reuse this backend's handle rather than making a new one per request. @@ -450,8 +478,18 @@ provider_do_curl_request(const char *url, const char *auth_header, if (res != CURLE_OK) { - *error_msg = psprintf("curl_easy_perform() failed: %s", - curl_easy_strerror(res)); + /* + * A buffer we could not grow aborts the transfer, so check it first: + * curl only knows the write callback returned short, and would report + * that as a generic write failure. + */ + if (response_out->alloc_failed) + *error_msg = psprintf("Response from %s is too large to buffer " + "(stopped after %zu bytes)", + provider_name, response_out->size); + else + *error_msg = psprintf("curl_easy_perform() failed: %s", + curl_easy_strerror(res)); curl_slist_free_all(headers); return false; } diff --git a/src/provider_common.h b/src/provider_common.h index cb3437e..39f5199 100644 --- a/src/provider_common.h +++ b/src/provider_common.h @@ -21,6 +21,9 @@ typedef struct char *data; size_t size; /* bytes of response held, excluding the terminator */ size_t capacity; /* bytes allocated in data, including the terminator */ + bool alloc_failed; /* write callback could not grow data; see + * provider_write_callback() for why it may not + * simply raise an error */ } ResponseBuffer; /* From 5b3428cc0a30ef1bce352aa09fb8ebf68533f840 Mon Sep 17 00:00:00 2001 From: Mason Sharp Date: Tue, 11 Aug 2026 19:14:22 -0700 Subject: [PATCH 3/3] Tell the two response-buffer growth failures apart A single alloc_failed flag reported an allocation refused for want of memory as a response too large to buffer, which points the operator at their provider when the response size may be perfectly ordinary and the server is simply short of memory. Record which of the two happened and say so. Raised by CodeRabbit on #61. --- src/provider_common.c | 16 ++++++++++------ src/provider_common.h | 17 ++++++++++++++--- 2 files changed, 24 insertions(+), 9 deletions(-) diff --git a/src/provider_common.c b/src/provider_common.c index f78374a..8a36d02 100644 --- a/src/provider_common.c +++ b/src/provider_common.c @@ -67,14 +67,14 @@ provider_write_callback(void *contents, size_t size, size_t nmemb, void *userp) if (!AllocSizeIsValid(newcap)) { - mem->alloc_failed = true; + mem->grow_failure = RESPONSE_GROW_TOO_LARGE; return 0; } newdata = repalloc_extended(mem->data, newcap, MCXT_ALLOC_NO_OOM); if (newdata == NULL) { - mem->alloc_failed = true; + mem->grow_failure = RESPONSE_GROW_NO_MEMORY; return 0; } @@ -424,7 +424,7 @@ provider_do_curl_request(const char *url, const char *auth_header, response_out->data = palloc(response_out->capacity); response_out->data[0] = '\0'; response_out->size = 0; - response_out->alloc_failed = false; + response_out->grow_failure = RESPONSE_GROW_OK; /* * Reuse this backend's handle rather than making a new one per request. @@ -483,9 +483,13 @@ provider_do_curl_request(const char *url, const char *auth_header, * curl only knows the write callback returned short, and would report * that as a generic write failure. */ - if (response_out->alloc_failed) - *error_msg = psprintf("Response from %s is too large to buffer " - "(stopped after %zu bytes)", + if (response_out->grow_failure == RESPONSE_GROW_TOO_LARGE) + *error_msg = psprintf("Response from %s is larger than can be " + "buffered (stopped after %zu bytes)", + provider_name, response_out->size); + else if (response_out->grow_failure == RESPONSE_GROW_NO_MEMORY) + *error_msg = psprintf("Out of memory buffering the response from " + "%s (stopped after %zu bytes)", provider_name, response_out->size); else *error_msg = psprintf("curl_easy_perform() failed: %s", diff --git a/src/provider_common.h b/src/provider_common.h index 39f5199..fb3e44a 100644 --- a/src/provider_common.h +++ b/src/provider_common.h @@ -13,6 +13,19 @@ #include "pgedge_vectorizer.h" #include +/* + * Why the write callback could not grow the response buffer. Kept apart + * because the two want different advice: one says the provider sent more than + * can be held, the other says this server is short of memory. See + * provider_write_callback() for why the callback may not simply raise. + */ +typedef enum +{ + RESPONSE_GROW_OK = 0, + RESPONSE_GROW_TOO_LARGE, /* would exceed MaxAllocSize */ + RESPONSE_GROW_NO_MEMORY /* allocation refused */ +} ResponseGrowFailure; + /* * Response buffer for libcurl callbacks */ @@ -21,9 +34,7 @@ typedef struct char *data; size_t size; /* bytes of response held, excluding the terminator */ size_t capacity; /* bytes allocated in data, including the terminator */ - bool alloc_failed; /* write callback could not grow data; see - * provider_write_callback() for why it may not - * simply raise an error */ + ResponseGrowFailure grow_failure; } ResponseBuffer; /*