pgduck_server: optional TCP listener for Kubernetes / multi-host deployments#338
Closed
timmclaughlin wants to merge 14 commits into
Closed
pgduck_server: optional TCP listener for Kubernetes / multi-host deployments#338timmclaughlin wants to merge 14 commits into
timmclaughlin wants to merge 14 commits into
Conversation
…e-Labs#316) ExecConstraints was running before IcebergErrorOrClampSlotInPlace, so values like bounded numeric NaN and multidimensional arrays passed the NOT NULL check in their original (non-null) form, then got clamped to NULL and silently stored in NOT NULL columns. Move clamping before ExecConstraints in both postgresExecForeignInsert and postgresExecForeignUpdate so constraint checks see post-clamp values. Factor the clamping + ExecConstraints sequence into a shared ClampAndCheckConstraints helper used by both INSERT and UPDATE paths. --------- Signed-off-by: sfc-gh-npuka <naisila.puka@snowflake.com>
lint-check-18 will error so we cannot merge a PR, but will allow the rest of the tests to run without needing to immediately fix a simple formatting bug. --- Signed-off-by: Naisila Puka <naisila.puka@snowflake.com>
Mark these two as PGDLLEXPORT so external extensions can reuse the shippability walker and the associated description helper. The pg_lake codebase builds with -fvisibility=hidden, so the declarations would otherwise be unreachable from another dylib loaded into the same backend. Made-with: Cursor Signed-off-by: Marco Slot <marco.slot@snowflake.com>
Signed-off-by: Marco Slot <marco.slot@snowflake.com>
Signed-off-by: Naisila Puka <naisila.puka@snowflake.com>
Some tests that rely on this codebase generate a lot of replication slots; let's just go ahead and set to a value that we are likely to never need to exceed. Signed-off-by: David Christensen <david.christensen@snowflake.com>
…Snowflake-Labs#326) When PQconsumeInput() returned false (broken connection), WaitForResult called ReleasePGDuckConnection() before re-throwing the error. The PG_FINALLY block in ExecuteCopyToCommandOnPGDuckConnection (and every other caller that wraps GetPGDuckConnection/ExecuteQueryOnPGDuckConnection in a PG_TRY) then called ReleasePGDuckConnection() a second time on the same hash entry. After HASH_REMOVE the entry's memory is returned to dynahash's freelist. If the slot was reused for the retry connection created inside ExecuteQueryOnPGDuckConnection, the second call freed the *new* connection. If the slot was not reused the second PQfinish() was called on an already- freed PGconn, producing the bogus address seen in the crash: #0 __GI___libc_free (mem=0xf5c9c67aca47e118) at malloc.c:3375 Snowflake-Labs#1 pqReleaseConnHosts (conn=0x30e138c0) at fe-connect.c:4723 Snowflake-Labs#4 ReleasePGDuckConnection at src/pgduck/client.c:186 Fix: remove the ReleasePGDuckConnection() call from WaitForResult. Connection lifetime is exclusively the caller's responsibility, managed via the PG_FINALLY block. Signed-off-by: Marco Slot <marco.slot@snowflake.com>
DuckDB's implicit CAST(TIMETZ AS TIME) drops the timezone offset
without shifting the time digits to UTC, so '23:59:59.999+05:30'
would land in Iceberg as '23:59:59.999' instead of '18:29:59.999'.
The Iceberg write projection applied this cast to every timetz column,
silently corrupting non-UTC values on any path that fed native TIMETZ
to DuckDB: INSERT..SELECT pushdown, COPY FROM pushdown, CTAS, and
downstream snapshot/initial-copy paths that use postgres_scan
(pg_lake_replication, snowflake_cdc). The regular per-row INSERT path
was already safe because TimeTzOutForPGDuck UTC-normalizes values
before handing them to DuckDB as CSV strings.
Fix it in the existing native-type query wrap:
- Generalize IcebergWrapQueryWithIntervalConversion (and its helpers)
into IcebergWrapQueryWithNativeTypeConversion, covering both
INTERVAL and TIMETZ, recursing through arrays, composites, maps,
and domains just like the interval path does.
- For TIMETZ leaves, emit
CAST(CAST(<expr> AS TIMETZ) AT TIME ZONE 'UTC' AS TIME). The outer
AT TIME ZONE 'UTC' / CAST AS TIME sequence folds the offset into
the time digits; the inner ::TIMETZ is defensive and keeps the
expression well-typed even when the source is already plain TIME,
which happens for TIMETZ fields read back from inside an Iceberg
composite (Parquet has no time-with-tz type, so DuckDB sees the
struct field as TIME). Without the inner cast DuckDB would reject
the query with "No function matches timezone(STRING_LITERAL, TIME)"
as soon as the wrap recursed into a composite field.
- Rename the wrapNativeIntervals parameter / doc comments to
wrapNativeTypes to reflect the broader meaning; callers in
writable_table.c, write_data.c and query_pushdown.c are updated.
Downstream callers (pg_lake_replication, snowflake_cdc) pass by
position and so continue to compile unchanged; their local variable
names can be renamed in a follow-up.
Regression tests in test_iceberg_timetz_type.py cover both the
pushdown wrap and the non-pushdown CSV path:
- test_iceberg_timetz_as_utc_time (existing) exercises scalar TIMETZ
and timetz[] through iceberg->iceberg INSERT..SELECT, a pg_lake
CSV foreign table source, and COPY FROM, asserting pushdown and
UTC-normalized round-trip values.
- test_timetz_insert_select_from_heap locks in the heap -> iceberg
FDW/CSV path (which is NOT pushed down -- heap sources aren't
DuckDB-shippable -- but is still UTC-safe via TimeTzOutForPGDuck).
- test_insert_select_timetz_in_composite_pushdown, ..._in_map_pushdown
and ..._deeply_nested_pushdown exercise the recursive traversal of
AppendNativeConversionExpression through arrays, composites and
maps -- the deeply-nested case stacks
composite -> array -> composite -> {timetz, timetz[]} and a sibling
map<text, timetz>. Each asserts Custom Scan (Query Pushdown) in
EXPLAIN plus the expected wrap SQL (struct_pack, list_transform,
map_from_entries, and the CAST(... AT TIME ZONE 'UTC' AS TIME) leaf).
- test_insert_select_timetz_quoted_identifiers_pushdown verifies that
quote_identifier is preserved at every level of the recursion for
both reserved keywords ("order", "time", "UTC") and quoted
mixed-case / whitespace identifiers ("Mixed CS", "At Time").
The domain-at-top-level case is deliberately not covered: iceberg
INSERT..SELECT pushdown is rejected at plan time for any target
column with a domain type (regardless of the base type), so the
wrap's domain-unwrap branch is not reachable through that path. The
branch is still exercised indirectly via TypeNeedsNativeConversion,
which recursively unwraps domains when deciding whether to invoke
the wrap at all.
Made-with: Cursor
When a single statement rewrites many manifests -- a DELETE that
touches data spanning thousands of manifests, or manifest-merge-on-
write compaction -- pg_lake_iceberg used to read every manifest's
entry list and Partition Field Map into the caller's memory context
and never release that memory until the surrounding statement
completed. On tables with high INSERT/DELETE churn this produces
memory peaks that grow linearly with the manifest count: backend RSS
climbs into multi-GB territory from a single in-flight DELETE, with
most of it sitting in one SPI Proc context populated by thousands of
sibling "Partition Field Map" / "Iceberg partitioned manifest entry
hash" sub-contexts.
The effect is amplified in long-running transactions and on backends
that anchor replication slots, because the memory cannot be reclaimed
until the statement (and any surrounding transaction) completes -- so
the peak observed in a `pg_log_backend_memory_contexts` snapshot is
also the floor for the rest of the transaction.
This commit introduces a private per-manifest memory context in the
two manifest-rewrite paths inside pg_lake_iceberg:
- FinalizeNewSnapshot's row-removal loop (the path that runs for
DELETE / TRUNCATE-style removal). The per-manifest body is
extracted into a focused helper RewriteManifestForRemoval that
owns the lifecycle of its own per-manifest context, so the
surrounding loop has no memory-context machinery left in it.
- RemoveDeletedManifestEntriesInternal (manifest-merge-on-write /
compaction). Already operates on one manifest at a time, so the
per-manifest context lives directly in that function.
The READ state -- the manifest entries list, Partition Field Map,
and transient avro decode allocations -- is allocated in
perManifestCtx and freed before the helper returns. After the
change, memory used by these loops is O(one manifest) instead of
O(N manifests): on tables with thousands of manifests this is the
difference between a multi-GB peak and tens of MB per call.
The new IcebergManifest headers and the deferred S3-upload temp
files continue to be allocated in the caller's context. This split
is required, not just convenient: UploadIcebergManifestToURI calls
GenerateTempFileName, which registers a cleanup callback on
CurrentMemoryContext that unlink()s the local temp file when that
context is reset; the matching upload is deferred until commit, so
the callback has to outlive the per-manifest scope. The helper
docstring spells this invariant out so future contributors see why
the WRITE phase must run in the caller's context.
Made-with: Cursor
Some Iceberg workloads have a strong reason to keep data files in the order they were written: time-bucketed analytics that prune by ingestion time, scan paths that need to read the newest writes first, and tables where most logical deletes are expected to land as metadata-only operations and therefore rely on the file list keeping its append shape. For those workloads, the file-rewrite stage of autovacuum is actively counter-productive -- it disturbs the very ordering the workload depends on -- but the rest of the vacuum pipeline (snapshot expiry, manifest merge, deletion-queue drain, orphan-file cleanup, field-id backfill) is still needed; without it the table accumulates state indefinitely. The only existing knob, `autovacuum_enabled`, is all-or-nothing: setting it to false on a table turns the entire worker off and hands those housekeeping costs back to the operator. This commit adds a finer iceberg table option scoped specifically to the compaction stage, so an operator can keep autovacuum running on the rest of the pipeline while opting that one stage out. Defaults are unchanged: every table -- existing or new -- behaves exactly as it did before unless the option is explicitly set to false. The option is autovac-scoped, mirroring the heap-level `autovacuum_enabled` storage parameter -- manual `VACUUM (ICEBERG) tbl` continues to compact unconditionally, since an explicit VACUUM is already a deliberate user request. Made-with: Cursor
…ake-Labs#313) Adds --with-pam to all PG version compile flags, pam-devel to build dependencies, and PAM runtime libraries to the runtime base image. This enables building and running extensions that use PAM-based authentication. Signed-off-by: David Christensen <david.christensen@snowflake.com>
When a user creates a read-only foreign table with an empty column list
against the REST catalog,
CREATE TABLE foo () USING iceberg
WITH (catalog='rest', read_only=true,
catalog_namespace='ns', catalog_table_name='t');
`DescribeColumnsFromIcebergMetadataURI` derives the postgres columns
from the iceberg schema but always leaves `ColumnDef.is_not_null = false`.
Iceberg fields with `required: true` therefore land as nullable on the
postgres side, and `ErrorIfSchemasDoNotMatch` (snapshot.c:413) trips
the strict equality check
columnMapping->attNotNull != icebergField->required
at the first projection — surfacing as
Schema mismatch between Iceberg and Postgres for field ids 1 vs 1
HINT: Please drop and recreate the table "..."
The current workaround forces users to enumerate every column by hand
with explicit `NOT NULL` annotations matching iceberg's `required` flags,
which defeats the point of the empty-column form. Set the flag from the
iceberg field at column construction time so the auto-detect path Just
Works against any externally-managed REST catalog table.
Reported in issue Snowflake-Labs#83.
Signed-off-by: Marco Slot <marco.slot@snowflake.com>
Adds test_rest_catalog_required_columns_autodetect, which is the
regression test for the previous commit. Before that fix:
CREATE TABLE foo () USING iceberg
WITH (catalog='rest', read_only=true, ...)
would auto-detect every column as nullable regardless of the iceberg
schema's `required` flag, then fail on the first projection inside
ErrorIfSchemasDoNotMatch with
Schema mismatch between Iceberg and Postgres for field ids 1 vs 1
The new test creates a REST-catalog table with a mix of required and
optional fields, registers it in postgres via the empty-column-list
form, and asserts:
1. pg_attribute.attnotnull is true for required iceberg fields and
false for optional ones (verifies the propagation),
2. SELECT actually returns rows (verifies ErrorIfSchemasDoNotMatch
no longer trips),
3. as a sanity check, an explicit definition that omits NOT NULL
on a required field still produces the expected schema-mismatch
error -- so the auto-detect path is satisfying the same check,
not bypassing it.
Existing tests in test_polaris_catalog.py only exercise field-count,
type, and default-value mismatches, all against schemas with every
field required=False. This closes the gap on the attNotNull/required
branch of the comparison.
Also trims the over-long block comment on the new column->is_not_null
assignment to a one-liner pointing at ErrorIfSchemasDoNotMatch (the
code itself is self-explanatory).
Made-with: Cursor
Adds --listen_addresses (PostgreSQL-style comma-separated list of
addresses) so pgduck_server can bind TCP listeners in addition to its
existing Unix domain socket.
Behavior:
- Default: --listen_addresses unset (or empty) → no TCP, Unix socket
only. Existing single-host docker-compose deployments unchanged.
- Set: e.g. "0.0.0.0,::" → bind both IPv4 and IPv6 on --port (the
same port already used for the Unix socket suffix). Each comma-
separated address gets its own listening socket; up to
MAX_TCP_LISTEN_SOCKETS (16) total.
Implementation:
- command_line: new --listen_addresses option, no default (NULL).
- pgserver: PGServer struct gains tcpSockets[] / numTcpSockets;
pgserver_init takes the new tcpListenAddresses parameter; new
static helpers create_and_bind_tcp_sockets() and
bind_one_tcp_addr() set up TCP listeners; pgserver_run replaces
the single-socket accept() call with a poll() across all listening
sockets and dispatches via a new dispatch_accepted_client() helper
factored out of the previous inline accept path; pgserver_destroy
now closes all listening sockets.
- main: passes options.listen_addresses to pgserver_init.
Use cases:
- Kubernetes operators (CloudNativePG, Zalando, etc.) that don't
permit sidecar containers in their managed Pod specs. The TCP
listener lets pgduck_server run as its own Deployment reachable
via a Service from the operator-managed PG pods.
- Shared cache: a single pgduck_server pool with a warm parquet
cache can serve multiple PostgreSQL replicas.
- Independent scaling: the analytical compute layer (DuckDB) can
scale separately from the OLTP layer.
Auth: TCP intentionally has no built-in authentication. Operators
should run it on a trusted network (e.g., k8s pod-to-pod inside a
private cluster, gated by NetworkPolicy). Documentation updates +
optional password auth could be a follow-up.
Wire protocol: unchanged. libpq handles both Unix and TCP transports
natively, so pg_lake's existing connection-string config
(pg_lake_engine.host) just takes a regular libpq DSN —
"host=/socket-dir port=5332" for Unix or
"host=hostname port=5332" for TCP.
Tested locally with:
pgduck_server --listen_addresses=127.0.0.1 --port=5332 \
--unix_socket_directory=/tmp ...
psql -h 127.0.0.1 -p 5332 -c 'SELECT 1'
psql -h /tmp -p 5332 -c 'SELECT 1'
Contributor
Author
|
Closing as superseded. This PR was the TCP-listener slice ( New PR: link will follow as a follow-up comment once filed. The TCP-listener content from |
Contributor
Author
|
Replaced by #345 — same TCP-listener code (commit 1, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Add support to
pgduck_serverfor listening on TCP sockets in addition to the existing Unix domain socket. Behind opt-in flags so the default behavior (Unix socket only) is unchanged.Proposed CLI:
Reuses the existing
--portfor the TCP listener (matches PostgreSQL convention whereportserves both the Unix socket suffix and the TCP port).Why
pgduck_serveris currently Unix-socket only and assumes co-location with PostgreSQL on the same host (the docker-compose pattern indocker/). This is great for single-host deployments but doesn't work cleanly on Kubernetes with operators like CloudNativePG (CNPG), which:hostPathvolume mountscommand:so user-controlledENTRYPOINTwrappers are bypassedFor CNPG-managed PostgreSQL clusters that want pg_lake, the only viable pattern is running pgduck_server in a separate Pod (own Deployment / StatefulSet) reachable from the PostgreSQL pods over the cluster network. That requires TCP.
Side benefits even outside Kubernetes:
Shared tmp in our setup
The shared
pgsql_tmprequirement for hybrid-query bridge can be solved with a ReadWriteMany volume (Filestore) for my k8s deployment. Depending on the runtime, there are other solutions.Auth
pgduck_servercurrently relies on filesystem permissions on the Unix socket for access control. For TCP, suggest: