Feature/datalake extension - #1
Open
MisterRaindrop wants to merge 2 commits into
Open
MisterRaindrop wants to merge 2 commits into
MisterRaindrop wants to merge 2 commits into
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi, @MisterRaindrop welcome!🎊 Thanks for taking the effort to make our project better! 🙌 Keep making such awesome contributions!
MisterRaindrop
commented
Jul 31, 2026
MisterRaindrop
force-pushed
the
feature/datalake-extension
branch
2 times, most recently
from
July 31, 2026 02:42
253b3de to
70adea1
Compare
MisterRaindrop
changed the base branch from
liuxiaoyu/iceberg_base
to
liuxiaoyu/iceberg_base_v2
July 31, 2026 02:54
MisterRaindrop
force-pushed
the
feature/datalake-extension
branch
2 times, most recently
from
July 31, 2026 06:53
de2bcae to
35a2237
Compare
MisterRaindrop
commented
Jul 31, 2026
MisterRaindrop
commented
Jul 31, 2026
MisterRaindrop
force-pushed
the
feature/datalake-extension
branch
8 times, most recently
from
August 3, 2026 09:02
b8c2da6 to
1f27bf8
Compare
Add contrib/datalake_fdw, a skeleton for Iceberg lake-table support that needs
no kernel changes: a lake table is an ordinary CREATE TABLE ... USING iceberg,
and it names a catalog server and a volume server in its reloptions, both
created through foreign-data wrappers this extension registers.
Mapping a table to a pair of foreign servers is what keeps the kernel out of it.
Server options, ownership, privileges and dump/restore already exist for foreign
servers, reloptions already reach every segment with pg_class, and recording the
two servers in pg_depend makes DROP SERVER refuse to strand a table -- none of
which needs new catalogs or grammar.
Where that mapping is kept is one function's business. Every operation reads it
through pg_iceberg_get_table_info(), whose signature and result types match the
existing datalake_fdw implementation this work is the upstream half of -- that
one keeps the same mapping in a system catalog of its own, which an extension
cannot add. Holding the interface still means the layers above it are the same
code on both sides, and that the storage can be reconsidered later without
touching a caller.
The DDL path is complete against a stub metadata engine, so CREATE TABLE and
DROP TABLE work end to end with no catalog service, object store, Arrow or JVM
in the picture. Everything that would touch data reports a clean
"iceberg: <operation> is not supported yet". The interfaces the later work
plugs into ship whole so they can be reviewed before there is an implementation
behind them: the IcebergMetaEngine vtable with a capability bitmap the registry
validates and dispatches through, the FormatReader/FormatWriter instance
interfaces, and the storage facade over open/read/write/list.
Details worth a reviewer's attention:
* Table metadata always goes through one engine, the Java agent, and nothing
selects between implementations -- no option, no setting. The vtable stays
because the implementation is expected to change; that is a property of the
build, never of a table or a session, so an existing table can never be
reinterpreted by a configuration change.
* The table access method fills every callback GetTableAmRoutine() asserts.
ANALYZE succeeds as a zero-sample no-op through relation_acquire_sample_rows,
which keeps it off the scan path that reports not-supported, and VACUUM is a
no-op, so database-wide maintenance never dies on a lake table.
* The object-access hook records the server dependencies on the coordinator and
on every segment, while only GP_ROLE_DISPATCH calls the metadata engine, so
each node can protect its own catalog and the remote side sees one call.
Utility-mode DDL is refused rather than creating local state without dispatch.
* VACUUM FULL is refused in the utility hook, not in the access method: relation
rewriting creates a transient relation first, which reaches OAT_POST_CREATE
and has the engine create a table remotely before the rewrite reports its
error, leaving an orphan behind.
* Credentials are refused in server options and belong in user mappings, which
stay optional so ambient object-store credentials remain usable. Binding
resolution never reads them, so DDL and DROP work with none configured.
* Volume URIs are parsed once, in the options layer, into a versioned
DatalakeLocation; backends receive only that canonical form.
* Option names are macros in per-wrapper option modules, next to the typed
struct each one parses into and the per-catalog-type parse function that fills
it, so that support for a further catalog or storage protocol is an addition
rather than a rewrite. Option lookup itself is one shared set of accessors.
The keys users write are Apache Iceberg's -- uri, warehouse, and rest for a
catalog reached over the REST protocol -- because the specification defines one
protocol that several implementations answer, and an SQL surface tied to one of
them would make every other one need a second spelling. polaris is accepted as
an alias of rest, since that is what the existing implementation calls it. The
macro names, struct names and field names stay that implementation's, so the
divergence is one string per key rather than a different shape.
* A DlErrCode says which kind of failure occurred and nothing else, which is not
enough to diagnose one -- a remote catalog's message, its own error class, and
a stack from wherever it threw have to arrive somewhere. Implementations
record that alongside the code they return, and the entry points facing
PostgreSQL turn both into one report: the message as DETAIL, a stack only for a
session that asked for log-level detail. Recording allocates nothing and
raises nothing, so a cleanup path crossing back from C++ can use it. The
SQLSTATE follows the code rather than being internal_error throughout, which
also keeps a source location out of user-visible output.
* C++ translation units reach the server headers through common/dl_pg_api.h,
which applies extern "C" -- without it the module builds and then fails to
dlopen on a mangled errmsg. The C/C++ boundary macros follow the PAX pattern,
including deferring ereport() until after the catch handler is left, since
longjmp() out of a handler is undefined. Exported symbols are limited to the
PG entry points listed in exports.txt, ELF and Mach-O each getting the right
linker mechanism, so a future static Arrow cannot leak into other extensions.
* A schema-level dump round-trips. pg_dump writes DISTRIBUTED RANDOMLY and
ALTER TABLE ... OWNER TO for a table like this, so both are accepted -- a
guard that refuses what this module's own dump emits refuses to restore it.
Neither can desynchronise anything: the distribution clause asks for the
policy that would have been injected anyway, and ownership is local catalog
state. Every other ALTER form, and a distribution clause naming columns,
stay refused. Dumping the *contents* of a lake table still fails, because
scanning does; a full pg_dump of a database containing one therefore does not
work yet, and what a dump of externally owned table data should even mean is
the open question behind that.
Test material lives under test/automation, one directory per category, with the
module's Makefile pointing pg_regress at the category that needs no external
service; make installcheck from the module and make test from the harness run
the same cases. Testing against a real catalog or object store cannot be done by
comparing against a recorded transcript, so the harness is what those categories
will be added to, and it already reports a category whose services are absent as
skipped rather than passed.
The suite covers the DDL path including per-segment catalog state, the rejection
matrices and the privilege model; installcheck is green on a three-segment
cluster and does not depend on the order the cases run in. Per-segment
assertions compare against gp_segment_configuration rather than naming segments,
so they hold on a cluster of any size, and each guard has a case showing what it
does *not* refuse -- renaming a schema that holds no lake table, for instance --
because a guard wider than its problem passes its own tests just as well.
CI runs the suite as its own matrix entry, ic-datalake-fdw, whose demo cluster is
created with shared_preload_libraries='datalake_fdw' -- the module installs
process-wide hooks, so _PG_init refuses to load any other way, and a generic
cluster could not run these cases at all. That is the same mechanism two
existing entries already use. ("make check" would need the temp-config this
module also ships; it exists in-tree only, since PGXS refuses the target.)
The error channel has no coverage yet: no statement can make the stub engine
fail, so the first implementation that can fail is what brings a case for it.
MisterRaindrop
force-pushed
the
feature/datalake-extension
branch
from
August 3, 2026 09:29
1f27bf8 to
36675eb
Compare
…covery The test sets shared_preload_libraries and restarts with "gpstop -raiq". An immediate shutdown skips the shutdown checkpoint, so the control file is left in a state other than DB_SHUTDOWNED and the next startup performs crash recovery: xlogrecovery.c sets InRecovery, xlog.c calls PerformWalRecovery(), which signals PMSIGNAL_RECOVERY_STARTED, and the postmaster moves to PM_RECOVERY. In that state canAcceptConnections() answers CAC_NOTCONSISTENT, reported as "the database system is not accepting connections" with detail "Hot standby mode is disabled". gpstart makes exactly such a connection right after pg_ctl returns, to read the segment configuration, so gpstop -r exits CRITICAL and the restart is reported as failed. The damage does not stop there. psql gives up at the \c that follows, so every statement in the file is skipped and the test fails as a whole; the cleanup at the end of the file never runs; and gpstart never got past starting the coordinator in admin mode, so the cluster is left with no segments up. Suites that run after this one in the same job then lose their Gather Motion nodes and fail as well. Shut down fast instead. A fast shutdown writes the shutdown checkpoint, the control file says DB_SHUTDOWNED, no recovery runs, PM_RECOVERY is never entered, and CAC_NOTCONSISTENT cannot be returned -- the failure becomes unreachable rather than merely less likely. Fast is also what the rest of the tree already uses: gpstop -raf/-arf appear in dozens of places, and this file was the only user of -raiq. Measured on a three-segment demo cluster, dirtying 1.5M coordinator rows before each restart so that recovery is slow enough to lose the race reliably: -raiq failed 2/2 with the message above, -rafq passed 3/3 with all three segments still up afterwards. pg_controldata confirms the mechanism at the other end -- "in production" after an immediate shutdown, "shut down" after a fast one. The test still passes under pg_regress with the change.
MisterRaindrop
pushed a commit
that referenced
this pull request
Sep 13, 2026
…che#1946) Upstream PostgreSQL commit "Remove configure probe for sockaddr_in6 and require AF_INET6." (bcc8b14) deleted the HAVE_IPV6 probe and stripped the #ifdef HAVE_IPV6 guards from its own code, since AF_INET6 is now always available. Cloudberry-specific code still gated IPv6 handling behind #ifdef HAVE_IPV6, so after the PG16 merge those blocks became dead code: HAVE_IPV6 is never defined on non-Windows builds. The practical effect: on an IPv6-only cluster, getDnsCachedAddress() never populates its cache entry (the IPv6 branch was compiled out), then returns e->hostinfo with e == NULL -- a bogus non-NULL pointer (offsetof key[]) -- which the caller passes to pstrdup(), crashing in strlen(). This shows up as a coordinator/FtsProbe SIGSEGV: #0 __strlen_evex #1 MemoryContextStrdup apache#2 getCdbComponentInfo apache#3 cdbcomponent_getCdbComponents apache#4 FtsProbeMain Remove the leftover #ifdef HAVE_IPV6 guards so the IPv6 paths compile unconditionally, matching what upstream did to its own files. Also guard the cache return against a NULL entry so an unresolvable segment logs a clean "cannot resolve network address" error instead of segfaulting. Files: cdbutil.c (both getDnsCachedAddress copies), auth.c, and the interconnect listener setup (ic_common.c, ic_tcp.c, ic_udpifc.c). Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Fixes #ISSUE_Number
What does this PR do?
Type of Change
Breaking Changes
Test Plan
make installcheckmake -C src/test installcheck-cbdb-parallelImpact
Performance:
User-facing changes:
Dependencies:
Checklist
Additional Context
This is the upstream half of an implementation that already exists. A
datalake_fdw with the same shape -- the same two hooks, the same table access
method, largely the same file names -- runs in production elsewhere, and the
intent is that this becomes the single source for it rather than a second
lookalike. Two consequences a reviewer should see explicitly:
The mapping accessor is deliberately shaped to that implementation. A lake
table's mapping to its catalog and volume servers is read only through
pg_iceberg_get_table_info(), whose signature and result types(
IcebergTableInfo,IcebergTableOptions) match it field for field. Thatimplementation keeps the same mapping in a system catalog of its own, which an
extension cannot add, so here the function reads reloptions instead -- but the
difference stops inside the function body, and the layers above it can be the
same code on both sides.
Two option names will look wrong, and are chosen anyway. Catalog servers take
url, not the Iceberg specification'suri, and the REST-based catalog type isnamed
polarisrather than a genericrest. Both follow the releasedimplementation. One vocabulary across the two trees is worth more than either
name, and this side has never been released, so it is the side that yields. A
generic
resttype is deferred rather than rejected;hadoopands3are inthe vocabulary but refused until something implements them.
Two limits worth stating rather than leaving to be found: the mapping stores
server names, so
ALTER SERVER ... RENAMEon a referenced server is refuseduntil it stores OIDs; and the error-detail channel added for engine failures has
no regression coverage, because no statement can make the stub engine fail.
CI Skip Instructions