Conversation
adsharma
changed the title
adsharma
changed the title
Skeleton extension with: - Catalog: discovers node_* and rel_* tables from PG information_schema via libpq, creates NodeTableCatalogEntry or RelGroupCatalogEntry - Scan function: stub for reading data from PG via libpq during execution - CMake: finds PostgreSQL via find_package, links libpq
… and scan Implement the three main components to finish the pg_client extension skeleton: - Catalog (pg_client_catalog): queries PG information_schema via libpq to discover node_* and rel_* tables, creates foreign table catalog entries with scan functions for each discovered table - Scan function (pg_client_scan): table function that executes queries against PostgreSQL via libpq, converts PG result rows to Ladybug vectors with proper type handling and pagination - Connector (pg_client_connector): libpq wrapper with connection management, query execution, and PG OID to Ladybug type conversion - Storage extension: registers the PG_CLIENT storage type with attach support using connection string (dbname=... or postgresql:// URI) - Custom catalog entry (PgClientTableCatalogEntry): extends TableCatalogEntry to store PgClientTableScanInfo for foreign table scanning support - Entry point (pg_client_extension): registers the storage extension with the database on load Also: - Add pg_client to the official extension build list - Fix CMakeLists to properly propagate object files to the build system
Fixes: - Use schema name (default 'public') not catalog name (database) when generating PG table references in scan queries - Remove 'rowid' from PG column names list since PostgreSQL doesn't have a rowid pseudo-column; internal IDs are now synthesized from row indices - Map DATE, TIMESTAMP, INTERVAL, UUID, BYTEA to STRING type to avoid physical type mismatch (INT32/INT64 vectors receiving string data) Tests: - Add test/test_pg_client.py: 10 test cases using pgembed to spin up a temporary PostgreSQL instance with test data - Add test/test_files/pg_client.test: end-to-end test file compatible with the Ladybug e2e test framework (requires PG_CLIENT_CONNECTION_STRING env)
- Detect rel_ prefixed tables in init() using case-insensitive check
(rfind('rel_', 0) == 0), matching the duckdb_rel branch pattern
- Query PostgreSQL information_schema for foreign key constraints to
determine src/dst node tables for rel_* tables
- Create RelGroupCatalogEntry in the main catalog with scan function
and bind data for auto-discovered relationship tables
- Fall back to createForeignNodeTable when FK info is not available
- Link shadow NodeTableCatalogEntry to attached PgClientTableCatalogEntry
via setReferencedEntry so planner can find scan functions
- Call StorageManager::createTable() for auto-discovered rel tables
- Update tests to cover LOAD FROM with filters on rel tables and
multi-table node scans
- Add mutex guard to PgClientConnector::executeQuery since libpq connections are not thread-safe and ForeignRelTable scans can happen from parallel worker threads (HashJoinBuild) - Add null checks in scan function (initSharedState, tableFunc) with descriptive error messages for easier debugging - Revert rel table registration from RelGroupCatalogEntry (which required ForeignRelTable with thread-safety issues) to shadow NodeTableCatalogEntry — rel_* tables are registered as discoverable node shadows without createTable(), avoiding the ForeignRelTable path entirely - Remove createTable() for shadow entries that are backed by rel_* tables (ForeignRelTable crashes due to null sharedState)
The previous fix reverted rel table registration to a discoverable node
shadow (NodeTableCatalogEntry + ShadowTag) and skipped createTable() to
sidestep ForeignRelTable initScanState thread-safety issues. That broke
MATCH (a)-[k:rel_knows]->(b) because the planner can't anchor a rel
scan on a shadow node entry.
Restore RelGroupCatalogEntry registration in createForeignRelTable:
* build a RelTableCatalogInfo with src/dst node table IDs and a fresh
rel OID so the planner has a real rel entry to anchor MATCH on
* pass the rel scan function and bind data so StorageManager
instantiates a ForeignRelTable for it
* call createTable() so the rel table is registered in storage
Morsel-driven correctness in the scan function:
* wrap currentOffset advancement in TableFuncSharedState::mtx so
parallel worker threads (HashJoinBuild) atomically claim morsels
instead of duplicating or skipping rows. The PgClientConnector
mutex added in the previous commit serializes the underlying
libpq calls; this mutex serializes the offset.
Broaden FK column detection to also recognise from_*/to_* (used by
the e2e schema) in addition to src_*/dst_*/dest_*.
Tests:
* test_06b_match_rel_table / test_06c_match_rel_count_parallel:
MATCH (a:node_person)-[k:rel_knows]->(b:node_person) RETURN
count(*) must return 5. Exercises the full rel table scan path
through ForeignRelTable driven by HashJoinBuild workers.
* pg_client.test: new MatchOnRelTable case covering the same
query for the e2e runner.
Contributor
Author
|
Accidentally merged by a rogue agent. Fixing issues in new PRs. |
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.
Why a second postgres extension?
There's already an existing postgres extension that uses libpq + DuckDB as an intermediate query engine: it spins up an in-memory DuckDB instance, installs the
postgresDuckDB extension, and runs all queries through DuckDB'spostgres_scanner. That works, but everyATTACHships DuckDB into the process, which is a heavy dependency for what is, at the protocol level, a thin client over libpq.pg_client drops DuckDB and talks to PostgreSQL directly through libpq. No intermediate engine, no auxiliary copy, no auxiliary attached database. The trade-offs:
postgres(DuckDB)pg_client(libpq)postgres_scannervia libpqATTACHCREATE REL TABLE ... WITH (storage='...')patternWhat this PR does
Restores
RelGroupCatalogEntryregistration forrel_*tables soMATCH (a)-[k:rel_knows]->(b)works again. A previous fix had reverted this to a discoverable node shadow to sidestepForeignRelTablethread-safety issues, but that broke the planner anchoring a rel scan on the shadow.Catalog (
createForeignRelTable)RelTableCatalogInfowith src/dst node table IDs and a fresh rel OID so the planner has a real rel entry to anchorMATCHonStorageManagerinstantiates aForeignRelTablecreateTable()so the rel table is registered in storagefrom_*/to_*in addition tosrc_*/dst_*/dest_*)Morsel-driven correctness (
tableFunc)currentOffsetadvancement inTableFuncSharedState::mtxso parallel worker threads (HashJoinBuild) atomically claim morsels instead of duplicating or skipping rows. The connector mutex added in the previous fix serializes the underlying libpq calls; this mutex serializes the offset.Companion change
ladybug#734 —
ScanRelTable::initLocalStateInternalin the main repo fixes a pre-existing bug where the firstscan()fires before any re-init call site, soForeignRelTable::scanInternalwas dereferencing an uninitialized shared state. With that fixed, restoringRelGroupCatalogEntryregistration here no longer crashes.Tests
13 Python tests under
pg_client/test/test_pg_client.py(run with apgembed-spun-up local PG) and 8 e2e cases underpg_client/test/test_files/pg_client.test(run against a CI-provisioned PG viaPG_CLIENT_CONNECTION_STRING). Coverage:LOAD EXTENSION,ATTACH <conn_str> AS <name> (DBTYPE PG_CLIENT), customSCHEMAoption,SHOW_TABLES,TABLE_INFOLOAD FROM testdb.node_*with and without WHERE-clause filter pushdownMATCH (p:node_*)in-place CypherLOAD FROM testdb.rel_*with and without WHERE-clause filterMATCH (a:node_*)-[k:rel_*]->(b:node_*):test_06b_match_rel_table—RETURN count(*)returns the expected row count. Exercises the full rel table scan path throughForeignRelTabledriven byHashJoinBuildworkers, with the morsel-offset mutex ensuring parallel workers don't double-count.test_06c_match_rel_count_parallel— duplicate of the count test as a tight parallel-correctness check.MatchOnRelTable— same query in the e2e runner.node_/rel_prefix are not exposed (security: only the intended tables become queryable).The push-down optimizer (
ForeignJoinPushDownOptimizer) correctly lowersMATCH ... RETURN count(*)to a singleCOUNT_REL_TABLEoperator onrel_knows, skipping the hash join with the node tables entirely.Known limitation
The projection form (
RETURN a.name, b.name, k.since) requires a separate PG-ID → internal-node-ID mapping layer, since the node table scan currently synthesizes internal node IDs as scan row indices. Tracked separately.