Repository navigation
datalake_fdw: storage I/O over S3 and pluggable storage backends - #2044
Merged
Merged
Conversation
MisterRaindrop
force-pushed
the
feature/datalake-s3
branch
3 times, most recently
from
September 24, 2026 10:26
dbd0f51 to
eb15606
Compare
MisterRaindrop
force-pushed
the
feature/datalake-s3
branch
from
October 2, 2026 12:12
eb15606 to
c4dc70a
Compare
Where a lake table's files live was decided by a switch on the URI scheme, and only s3 and hdfs were written into it. This replaces that with a contract: a backend answers one question -- given a location and its options, which arrow::fs::FileSystem reads and writes it -- and everything else stays on this extension's side of the boundary. What lands here: - The contract, in five headers that "make install" puts under $(includedir_server)/extension/datalake_fdw/, so a backend can be built outside this tree against nothing else. It carries an ABI version, the Arrow version, and a fingerprint over the compiler major and _GLIBCXX_USE_CXX11_ABI, because shared_ptr and arrow::Result cross the boundary by value. - A registry keyed by URI scheme, found through a rendezvous variable. Registration happens during preload and in any order: the registering side pulls in datalake_fdw through load_external_function, inside a PG_TRY so an ereport cannot unwind through a plug-in's C++ frames. Each process initializes a backend at its first mount and registers the matching finalizer then, because on_exit_reset() clears what a postmaster child inherits. - The facade over that filesystem: opening, listing, deleting, turning a URI into a native path, and classifying failures into DlErrCode. Backends never re-parse a URI, and an empty listing is ruled not-found here rather than by each backend, because object storage and a filesystem disagree about what an empty prefix means. A backend that classified a failure itself is believed; anything else is read as a whole status rather than as a message, since Arrow puts a failed open's errno in a detail -- and only phrases a filesystem writes about itself are matched, because a service's numeric code found inside the text would turn an error about a path into an error about the path not existing. - A file backend for shared mounts, which creates with O_EXCL and, if it has to give up, removes the file this writer created and nothing else. The stream takes ownership before the guard is released and removes the file from its destructor, so an exception leaves nothing behind either. - Credential scrubbing in one place. Every value under a key containing secret, token or password is remembered per process, and dl_error_set -- the single exit every DlErrCode error goes through -- removes it from the message, so an unexpected C++ exception and a third-party backend's own status are covered by construction. No value is declined for being short or for being the thirty-third, and a message is scrubbed as it is copied rather than after: a field that truncates would otherwise leave the first half of a key behind. The list is held with malloc, so recording a credential cannot ereport out of the C++ frame that is holding one; if it fails anyway, the message is withheld instead of shown. - An example backend, built as a library of its own under dltest/. It is not part of this one: it compiles against the installed headers, registers through datalake_storage_register, and a server that does not name it has no dltest scheme. Building it in would put a test-only scheme on every cluster -- one a volume could be created against -- and would demonstrate nothing about the boundary it exists to demonstrate. The parser now accepts file:// as well as s3://, and names what it rejects against the registry rather than a fixed list, so a scheme a plug-in registered can be written into a base_path. What it quotes back is redacted first: a rejected URI's userinfo and its query are reported as present rather than reproduced, one being able to hold a password and the other a presigned signature. Userinfo is cut at the last "@" in the authority rather than the first, because it may hold one of its own. Three DETAIL lines in iceberg_am_reject change with it. "." and ".." are refused as whole path segments, whatever the scheme. Which paths a volume covers is decided by comparing prefixes as text, and "file:///v/inside/../secret" satisfies that comparison while resolving somewhere else, so the URI never reaches it. The old s3 stub is removed here rather than adapted twice; the backend comes back, written against this contract, in the commit that follows.
s3 returns as an arrow::fs::FileSystem of our own over the AWS SDK for C++, rather than Arrow's S3FileSystem: no RPM of Arrow is built with S3 support -- EPEL's and the Arrow project's own both set use_s3 0 -- so depending on it would mean asking every user to build Arrow. A synchronous S3Client, so a cancelled query cannot leave a callback holding a backend's memory. Reads go through PreallocatedStreamBuf straight into a tracked buffer rather than the SDK's default stringstream. The range is clamped against the size the file was opened at, so every byte asked for existed when the request was made and a short answer is a truncated response or an object replaced underneath the reader -- an error, not an end of file. Writes buffer 8 MiB and begin a multipart upload only when they exceed it, so a small file is one PutObject. Cleanup depends on whether an upload is still live rather than on whether the stream was closed, so a Close() that fails at its last part still aborts instead of leaving parts to be billed for, and the destructor does the same. A listing follows continuation tokens and gives up if one stops advancing, rather than trusting the service to end it. Credentials are taken as a pair or not at all -- half of one is a configuration error, not a reason to fall back to the host's own identity -- and a bucket that does not exist is reported as missing rather than as a directory. The options are spelled as the DDL spells them, access_key_id and secret_access_key and session_token, rather than under a second vocabulary of the backend's own. Connect timeout 5 s, request timeout 300 s, three retries, set here rather than inherited, so a black-holed endpoint answers in about 24 seconds on every Arrow version. The SDK is found by prefix -- the one AWS_SDK_PREFIX names, or the usual places a hand-built one lands. Without it the module still builds and says so, and opening an s3:// location reports that it was left out; a prefix with no SDK in it is an error rather than a silent fallback. It is linked statically even where that prefix also holds shared copies of it, and adds libcurl, OpenSSL and zlib to NEEDED and nothing else.
The Parquet reader and writer opened paths through the local file system directly. They now open through the storage facade, so a fragment names a filesystem and a path relative to its mount, and the same reader works against a volume on s3 as on a shared mount. "Create only" moves with them: the writer asks the facade, which is the one place that checks a name is free, and giving up is the stream's Abort rather than a delete by path -- after a failed write the name may already belong to somebody else. A volume resolves to a location plus credentials in one place, checking USAGE on the server and reading the user mapping, the PUBLIC mapping, or neither. What its validator quotes back when it rejects a base_path goes through the same redaction as everything else that echoes one, so a password in the userinfo does not reach the server log.
Three backends that must behave identically are worth one body of tests, not three that drift. A shared SQL file is parameterised over a URI prefix and a volume and run for file://, for s3:// and for the test backend: the type round trip, field-id projection, a row-group range, listing, a rejected overwrite, a write that fails halfway leaving nothing behind, a prefix nothing was ever written under, and -- where there is a filesystem underneath -- one holding nothing but an empty directory. Both of those have to read as not found rather than as an empty listing, which is the disagreement between a bucket and a directory that the facade exists to settle. The write that fails halfway carries thirty-two distinct md5s a row and asks for no compression, because a kilobyte of one repeated character encodes to nothing: twenty thousand such rows came to 174 KB, so the case that was meant to fail in the middle of a multipart upload had never begun one. It now writes 14 MiB before it fails. What the listing afterwards proves is that no object is left; that no upload is left running is asserted outside the suite, against the service's own list, because an abandoned upload leaves no object either. Volume resolution gets its own case, on a file volume so it runs everywhere: no USAGE, a PUBLIC mapping, a URI outside its volume, a volume that does not exist, a base_path whose userinfo or query must not come back in the error -- including one whose userinfo holds an "@" of its own -- and a URI that would leave the volume through "..". Listing past one page is a few thousand requests and minutes of wall clock, so it is a case of its own and runs where DATALAKE_TEST_S3_PAGINATION asks for it rather than on every build.
Volumes and their options, where credentials come from and in what order, what a file:// volume requires of the filesystem, how to build with S3 support, and how to write a backend -- the worked example is dltest/, which is compiled and run rather than written out by hand, and the sketch in the text says plainly which of the obligations below it does not meet. Plus the limits that are accepted rather than fixed, among them what s3 costs: a backend process holds about 15 MiB of resident memory that gp_vmem_protect_limit does not see, and that stays flat as the object grows -- 7.9 MiB for a 2 MB file, 14.8 MiB for a 149 MB one -- because it is the client, its connection and one part buffer rather than a cost per byte.
MisterRaindrop
force-pushed
the
feature/datalake-s3
branch
from
October 2, 2026 12:48
c4dc70a to
7e689a6
Compare
The s3 half of contrib/datalake_fdw's regression had nothing to run against. Three things were missing. The AWS SDK for C++, which no distribution packages: built from source with BUILD_ONLY="s3;sts", and cached under the version, the distribution and the architecture, since nothing else changes it. A miss costs about three minutes; the cache is 4 MB and restores in one second. Only the build dependencies the image lacks are installed, asked for by capability -- naming a package it already has makes dnf try to upgrade it, and on Rocky 10 the newest libcurl-devel wants a libcurl no enabled repository carries. A service that speaks S3: SeaweedFS, one static binary, pinned, and started with an identity file -- without one it accepts any credentials, and the case that asserts a wrong secret is refused would pass by not being tested. Readiness waits on the S3 port itself, which begins listening seconds after the master elects itself. Its coordinates, which "su - gpadmin" drops along with the rest of the environment, so they are named on the command line. Every other test entry leaves that empty, and a leg where the service did not start skips the s3 cases rather than failing them. Arrow on Rocky 9 and 10 now comes from the Arrow project's repository pinned to 17.0.0 and 21.0.0, as it already did on Rocky 8. EPEL's moves when EPEL does, and Arrow is the library this extension's ABI is shared with. A green job is only evidence about the cases that ran, so afterwards the three s3 case names are looked for in the log the run wrote; a variable that failed to arrive makes the Makefile skip them and report success. The wrong-secret value the suite asserts against is then searched for in the segment logs and the service's own log, which is where a credential would land that never reached the client, and the service is asked whether any multipart upload is still in progress -- an upload nobody aborted leaves no object to notice it by.
MisterRaindrop
force-pushed
the
feature/datalake-s3
branch
from
October 2, 2026 13:43
7e689a6 to
fe391b8
Compare
MisterRaindrop
marked this pull request as ready for review
October 4, 2026 01:39
ostinru
added
the
datalake
leborchuk
reviewed
Oct 14, 2026
yjhjstz
approved these changes
Oct 15, 2026
… 26 elsewhere Arrow 23 moved its headers to C++20 (std::span, std::bit_width), so the Makefile now picks -std=c++20 when Arrow is 23 or newer and keeps C++17 below that. C++20 deprecates ++ and -- on a volatile, which the server's own HOLD_INTERRUPTS does, so -Wvolatile is off with it. Arrow 26 removed the FileReader::GetRecordBatchReader overload that returns its reader through an out parameter. The Result-returning form exists from Arrow 19, so the reader uses it from there and keeps the old spelling for older versions. CI installs 22.0.0 on Rocky 8, the last release whose headers are C++17, since Rocky 8's gcc 8 has no C++20 library, and 26.0.0 on Rocky 9 and 10, so both the C++17 and the C++20 build are tested.
3 of 9 tasks
Vlasdislav
reviewed
Oct 21, 2026
Vlasdislav
reviewed
Oct 21, 2026
…L_WRAP DL_WRAP_END turned a PostgreSQL error into a DlPgError and did nothing else, which leaves two things behind that a PG_CATCH block that does not rethrow has to deal with. errfinish() leaves ErrorContext current when it longjmps, so whatever the caller allocated next went into a context the next error resets. And the error stayed on the error stack, with its message reaching nobody: every report built from a DlPgError said only "PostgreSQL error". DL_WRAP_START now notes the memory context, and DL_WRAP_END hands it to dl_error_take_pg_error(), which switches back to it, copies the message into the exception and flushes the error state. Copying can fail in turn, out of memory being the likely reason to be there, and then the generic text stands. The exception keeps the message in fixed storage, as dl_error_set() does, because it is filled where nothing may raise. Seen with an error injected into storage_registry(): after datalake_storage_scheme_registered() swallowed it, CurrentMemoryContext was ErrorContext and geterrcode() still found XX000 on the stack; with this change the context is the caller's again, the stack is empty, and a failed open reports "fs_open: <the injected message>".
The finalizers that run at process exit caught everything a plugin threw and said nothing. Use the cleanup guard instead, as the other cleanup paths do: one plugin still cannot stop the rest from being finalized, but its failure is now reported as a WARNING. The lookup of the registry at the top of the same function swallowed the same way and uses the same guard now. datalake_storage_scheme_registered() returns straight from its try and catch rather than through a variable. Suggested in review by Vlasdislav.
… fails datalake_storage_register() catches the error a failed load raises so that it does not unwind through the plugin's _PG_init, but it only flushed the error state. errfinish() had made ErrorContext current, so whatever the plugin allocated afterwards lived in a context the next error resets. Switch back to the caller's context first. The reason the load failed was also thrown away: the code returned says that registration failed, not that a library or a symbol is missing. Log it. Copying it allocates, so it is copied under a PG_TRY of its own and falls back to no reason rather than unwinding. Seen by injecting a failure into the helper for the dltest module: its _PG_init came back in ErrorContext, and now comes back in the postmaster's context with the reason in the log.
… a scheme The redactor that makes a location URI safe for an error message kept everything before the first "://" as the scheme. A base_path such as 'user:secret@evil://x/y' or '?token=secret://b/c' therefore had its secret quoted in both the error and its detail, since a scheme nobody registered is reported before userinfo or a query is looked for. Check the scheme as RFC 3986 spells one, a letter followed by letters, digits, "+", "-" or ".". The parser rejects anything else before it looks the scheme up, and the redactor prints "***" in its place.
datalake_fs_list() took rc still being DL_ERR_INTERNAL, its initial value, to mean that the listing had succeeded. A backend whose failure carried DL_ERR_INTERNAL in its detail was then reported as "nothing is stored", and one whose failure carried DL_OK as a success with nothing listed. Record whether the listing succeeded instead, and never let a failed status become DL_OK when it is converted. dl_storage_status_to_err() also loses a message variable that was always empty when it was tested.
…nly once The library is compiled as C++20 against Arrow 23 and later, whose headers need it, but the dltest module kept -std=c++17. It built only because nothing it includes happens to need C++20 yet. The parent now hands its choice down, and a standalone build of the module makes the same choice itself. install depends on all, which builds the module, and on dltest-install, which builds it again; under -j the two recursive makes could compile the same objects at once. dltest-install now follows dltest-all.
The check that the s3 cases ran took the first file matching make-ic-datalake-fdw-config*.log, and make-ic-datalake-fdw-config0-results.log matches too. Which one sorts first depends on the locale; picking the results log would report every s3 case as not run.
iceberg_volume_resolve() took any server with a base_path option for a volume, and handed on the options of the caller's user mapping for it. The catalog and table paths already check that a volume server belongs to iceberg_volume_fdw; do the same here.
…x a comment mount, initialize and finalize are called from C++ frames, which an ereport(ERROR) would longjmp past, and finalize runs during process exit, where an ERROR becomes FATAL and the backends after it are never finalized. Say in the installed header that they report failure through what they return. dl_error_add_secret() ignores NULL and empty values and nothing else; its comment in the same installed headers claimed a six-character minimum that the implementation explicitly does not have.
# Conflicts: # .github/workflows/build-cloudberry.yml # contrib/datalake_fdw/Makefile
Vlasdislav
reviewed
Oct 22, 2026
Collaborator
I propose we want |
Collaborator
|
I noticed that Are there plans to support automatic schema discovery, so users can query a Parquet file without specifying column names and types manually, similar to DuckDB’s read_parquet? Would this be provided through a different interface, such as automatically creating a foreign table, given the current |
Contributor
Author
|
@Vlasdislav Thanks for looking at it. The user-facing path is Iceberg tables ( Querying bare Parquet files is the foreign-table path in #1683 §2.3, which #2008 leaves out of scope for this round. If it is picked up later, I'd expect schema discovery to come through |
MisterRaindrop
enabled auto-merge (squash)
October 26, 2026 03:22
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
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 does this PR do?
contrib/datalake_fdwreads and writes Parquet since #1951, but only on alocal file system. This adds the storage layer underneath: an
s3backendover the AWS SDK for C++, and a published contract so a backend this extension
does not ship can be added from outside it.
Closes #2009.
Decisions worth a look:
arrow::fs::FileSystemreads and writes it. Opening files, listing,classifying errors, accounting for memory and translating a URI into a native
path all stay on this extension's side of the boundary, so a backend is a
mount function and nothing else.
s3is our ownarrow::fs::FileSystemover the AWS SDK, not Arrow's.No RPM of Arrow is built with S3 support -- EPEL's and the Arrow project's
own both set
use_s3 0-- so depending on it would mean asking every user tobuild Arrow. The SDK is a
BUILD_ONLY="s3;sts"static build that takes wellunder a minute, links statically, and adds libcurl, OpenSSL and zlib to
NEEDEDand nothing else.request 300 s, three retries, so a black-holed endpoint answers in about 24
seconds rather than hanging a session, on every Arrow version.
O_EXCLand unlinks that path alone; the S3 one keys cleanup on whether anupload is still live rather than on whether the stream is closed, so a
Close()that fails at the last part still aborts the upload instead ofleaving parts to be billed for.
secret,tokenorpasswordis remembered per process, anddl_error_set-- the single exit every
DlErrCodeerror goes through -- removes it fromthe message. An unexpected C++ exception and a third-party backend's own
status are covered by construction rather than by remembering to call
something. A
base_paththat carries a password in its userinfo is redactedthe same way, in the message as well as the detail. Where one remembered
secret is a prefix of another, the longer is the one masked; and a
?or#inside the userinfo of a malformed URI does not stop the redactor shortof the
@that hides the password.the parser: a backend that registered
minecan be written into abase_pathwithout this extension being changed.version, and a fingerprint over the compiler major and
_GLIBCXX_USE_CXX11_ABI.Registration happens during preload and in any order -- the registering side
pulls in
datalake_fdwthroughload_external_function, inside aPG_TRYso an
ereportcannot unwind through the plug-in's C++ frames.backend's:
mountis handed a host struct carrying it, and the fiveinstalled headers give a backend no way to reach Arrow's default pool by
accident.
Six commits, each buildable and green on its own: the storage layer, the
s3backend, the Parquet reader and writer moving onto the facade, the conformance
suite, the README, and the CI wiring.
Type of Change
Test Plan
One body of storage behaviour runs against every backend rather than once per
backend:
storage_conformanceparameterises a shared SQL file over a URIprefix and a volume, and runs it for
file://, fors3://and for the testbackend.
storage_localcoversthe facade and the registry: four kinds of bad registration, a duplicate
scheme, path escapes, a name already in use, a missing file.
storage_s3covers the backend against a real service: round trip,9 MB through multipart,
ListObjectsV2paging,HeadObject,DeleteObject, deleting twice, a missing key, a missing bucket, a wrongsecret (and that the message does not contain it), path style with an
endpoint override, and a black-holed endpoint inside 30 s.
storage_conformanceadds the type round trip, field-id projection, arow-group range, listing, a rejected overwrite, a write that fails
halfway leaving nothing behind, a prefix nothing was written under and
one whose objects were just deleted (both not-found, which is the point:
object storage and a filesystem disagree and the facade decides), and
volume resolution: no
USAGE, aPUBLICmapping, a URI outside itsvolume, a volume that does not exist, and a rejected
base_paththatmust not echo its own password.
make installcheck-- every category, on a three-segment clusterwith the module preloaded, against MinIO and against SeaweedFS, on
Arrow 9.0.0 and Arrow 17.0.0. Each of the six commits was built and
checked separately, so the series bisects. The server that runs the
suite has to preload the test backend as well -- see Running the
tests below.
make -C src/test installcheck-cbdb-parallel(not run)Beyond the suite:
.ccfile that includesonly the five installed headers and PostgreSQL's server headers compiles to a
.sothat registers a backend; put beforedatalake_fdwinshared_preload_librariesthe cluster starts and the scheme works, andbuilt with a stale
abi_fingerprintthe registration is refused with bothvalues in the message.
Parquet file through
s3://costs about 15 MiB of resident memory more thanthrough
file://-- and that difference stays flat as the object grows(7.9 MiB for a 2 MB file, 14.8 MiB for a 149 MB one), so neither the response
body nor the multipart buffer accumulates.
statement fails; with
AWS_ACCESS_KEY_IDandAWS_SECRET_ACCESS_KEYin thepostmaster's environment it succeeds.
the suite. No SQL can see an abandoned upload -- listing a prefix does
not show one -- so a mutation that deletes the
AbortMultipartUploadcallstill passes
installcheck. What does catch it is the server's own list ofin-flight uploads: one entry with the mutation, none without it, on MinIO
and on SeaweedFS alike. CI therefore asks SeaweedFS for its
.uploadsafterthe run and fails if anything is there.
MinIO answering the first one to four ranged GETs with
503 SlowDown, theread recovers after one, two and three of them with the bytes intact, and
after four -- the request and its three retries -- fails with an error.
Each attempt writes the body from the start of the destination, so an
error body left by a failed attempt cannot crowd out the retry.
ListObjectsV2page with "more",pg_cancel_backendends the statement inabout 0.1 s with the usual
canceling statement due to user request; thepage loop checks for a pending interrupt and hands it back to C, since it
cannot be serviced from a C++ frame.
--with-llvm. With bitcode generation on, the module and thetest backend both build, every
.bcincluded.nm -Dlists the PostgreSQL entry points, oneregistration function and the test extension's UDFs; no Arrow and no AWS
symbols, on all three build variants.
Impact
Dependencies: an optional build dependency on the AWS SDK for C++. Without
it the extension builds as before and says so, and opening an
s3://locationreports that it was built without it; naming a prefix that has no SDK in it is
an error rather than a silent fallback. The module is off by default and not in
the RPM, so packaging is unchanged.
CI builds the SDK from source once per distribution and architecture and caches
it, starts SeaweedFS for the s3 cases, and now takes Arrow from the Arrow
project's own repository on Rocky 9 and 10 as well, pinned -- 17.0.0 and
21.0.0, so a version change in EPEL cannot arrive without a commit.
What that costs the
ic-datalake-fdwjob, measured on all three legs: buildingthe SDK takes 175-228 s on a cache miss and the cache restores in 1 s (it is
4 MB); SeaweedFS is up 6 s after the step starts; the s3 cases add about 35 s,
of which 22 s is one deliberate connect timeout.
Running the tests: the test backend is a library of its own,
datalake_fdw_dltest, built and installed besidedatalake_fdwbut neverloaded by it -- so production clusters have no test scheme. A server that runs
installcheckneeds both:make checkand CI already set this. Without itstorage_localandconformance_dltestfail, because the scheme they test is not registered.User-facing changes: a volume's
base_pathnow acceptsfile://as wellas
s3://, and the scheme it rejects is named against what is registeredrather than against a fixed list. A
.or..path segment is rejectedanywhere in a location, so a URI cannot climb out of the volume it names.
Three
iceberg_am_rejectDETAIL lines andtwo ERROR lines change wording, the latter because they used to quote back a
URI that can carry a password.
Checklist
contrib/datalake_fdw/README.mddocuments volumes, where credentials comefrom, building with S3 support, and writing a backend -- the example in it is
compiled against the installed headers rather than written out by hand. The
test extension's new functions name a path on the server's file system and are
revoked from
PUBLIClike the two that were already there.Additional Context
Known limits, deliberately rather than by oversight:
file://volume must be the same directory on every host. Nothingchecks it; the README says so.
take it. Iceberg's file names are unique by construction, so it does not
arise there.
bucket lifecycle rule that expires incomplete multipart uploads is the usual
answer; the README says so.
gp_vmem_protect_limit--the SDK client, its connection and one part buffer are allocated by the SDK
rather than through the tracked pool.
ABI.
mountreturns a struct holding astd::shared_ptrand astd::string; the fingerprint catches the mismatches that occur in practice(compiler major, the libstdc++ dual ABI, Arrow version), not every possible
one. A backend is supported when it is rebuilt with the same compiler major
and Arrow package as this module.
10,000-part limit is left to the server to enforce.
sources it downloads; both are pinned by version only.
it returns as a backend when someone needs it.