Skip to content

Commit 5e2d2a1

Browse files
committed
Address Onder's amazing comprehensive review
Harden catalog server security, configuration, and cross-catalog safety Refactor GetRestCatalogOptionsFromCatalog into a clean dispatcher (ResolveRestCatalogOptions) with two explicit builders: BuildRestCatalogOptionsFromGUCs for the built-in 'rest' path and BuildRestCatalogOptionsFromServer for user-created servers, plus shared helpers ApplyGUCDefaults, ApplyServerOptionOverrides, and ValidateRestCatalogOptions. Add CopyRestCatalogOptions for safe deep-copy into a target memory context. Unify the three separate server option lists (whitelist array, errhint string, applier chain) into a single IcebergCatalogOptionDesc descriptor table with type, offsetof, and validation flags. Adding or removing an option is now a one-line change. Add DDL-time value validation: reject empty strings for client_id, client_secret, scope, catalog_name; require a URI scheme for rest_endpoint, oauth_endpoint, location_prefix. Require ACL_USAGE on user-created iceberg_catalog servers at CREATE TABLE time, matching core's CREATE FOREIGN TABLE semantics. Record a DEPENDENCY_NORMAL from iceberg tables to their catalog server in pg_depend so DROP SERVER is blocked (and CASCADE drops them). Block ALTER SERVER RENAME for iceberg_catalog servers since dependent tables store the server name as a string in ftoptions. Block ALTER SERVER SET rest_endpoint when dependent writable tables exist to prevent silently redirecting them to a different REST catalog. Make GetRestCatalogName always return get_database_name(MyDatabaseId) for writable tables so ALTER SERVER ADD catalog_name cannot re-route an existing table to a different namespace. Fix token cache hash key regression: zero the key buffer with MemSet before strlcpy in BuildTokenCacheKey. Add syscache invalidation callback on FOREIGNSERVEROID to reset the token cache on ALTER/DROP SERVER, using CacheMemoryContext as parent. Add NULL guard on opts in GetRestCatalogAccessToken. Fix default_catalog GUC check hook to accept values outside a transaction (ALTER SYSTEM + pg_reload_conf path), mirroring how PostgreSQL handles check_default_tablespace. Introduce ValidateXactRestCatalog as a fail-fast guard that checks cross-catalog DML at statement time rather than at XACT_EVENT_PRE_COMMIT. Planted in postgresBeginForeignModify and AddQueryResultToTable. The existing pre-commit check is retained as a belt-and-suspenders fallback. Parametrize test_writable_rest_iceberg_table over built-in 'rest' and user-created server paths. Add tests for USAGE enforcement, dependency tracking, server rename blocking, rest_endpoint blocking, catalog_name re-routing, token cache invalidation, ALTER SYSTEM deferred validation, option value validation, multi-table same-server transactions, and cross-catalog rejection cleanup. Signed-off-by: sfc-gh-npuka <naisila.puka@snowflake.com>
1 parent e82da75 commit 5e2d2a1

12 files changed

Lines changed: 1399 additions & 157 deletions

File tree

pg_lake_iceberg/include/pg_lake/rest_catalog/rest_catalog.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,8 +101,9 @@ typedef struct RestCatalogRequest
101101
#define GET_REST_CATALOG_METADATA_LOCATION "%s/api/catalog/v1/%s/namespaces/%s/tables/%s"
102102

103103
/* Catalog options resolution */
104-
extern PGDLLEXPORT RestCatalogOptions * GetRestCatalogOptionsFromCatalog(const char *catalog);
104+
extern PGDLLEXPORT RestCatalogOptions * ResolveRestCatalogOptions(const char *catalog);
105105
extern PGDLLEXPORT RestCatalogOptions * GetRestCatalogOptionsForRelation(Oid relationId);
106+
extern PGDLLEXPORT RestCatalogOptions * CopyRestCatalogOptions(MemoryContext dst, const RestCatalogOptions * src);
106107

107108
extern PGDLLEXPORT void RegisterNamespaceToRestCatalog(RestCatalogOptions * opts, const char *catalogName, const char *namespaceName);
108109
extern PGDLLEXPORT void StartStageRestCatalogIcebergTableCreate(Oid relationId);

pg_lake_iceberg/src/init.c

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -375,10 +375,16 @@ IcebergDefaultCatalogCheckHook(char **newvalue, void **extra, GucSource source)
375375
return true;
376376

377377
/*
378-
* When catalog access is available, also accept user-created
379-
* iceberg_catalog foreign servers with TYPE 'rest'.
378+
* Outside a transaction we cannot do catalog lookups to verify that the
379+
* name refers to a valid iceberg_catalog server. Accept the value on
380+
* faith; an invalid name will error at first use. This mirrors how
381+
* PostgreSQL handles check_default_tablespace (see
382+
* src/backend/commands/tablespace.c).
380383
*/
381-
if (IsTransactionState() && IsRestCatalog(newCatalog))
384+
if (!IsTransactionState())
385+
return true;
386+
387+
if (IsRestCatalog(newCatalog))
382388
return true;
383389

384390
GUC_check_errdetail("pg_lake_iceberg: allowed iceberg catalog options are '" POSTGRES_CATALOG_NAME "', "

0 commit comments

Comments
 (0)