New Catalog Configuration via CREATE SERVER

[sfc-gh-npuka]

Note to reviewer: Let's assume we can save client secrets as server options for now. Easier to review commit by commit as each commit has its own description. The credentials are part of USER MAPPING in this PR #255

How users can create REST catalogs

CREATE SERVER my_polaris TYPE 'rest'
  FOREIGN DATA WRAPPER iceberg_catalog
  OPTIONS (rest_endpoint 'http://polaris:8181',
           client_id '...', client_secret '...',
           location_prefix 's3://my-bucket/warehouse');

CREATE TABLE t (a int) USING iceberg WITH (catalog = 'my_polaris');

A server with no options is also valid — all settings fall back to GUCs. This allows creating a named handle for organizational purposes while relying entirely on the global GUC configuration.

Extension Upgrade Script

The upgrade script creates:

1. iceberg_catalog FDW — a handler-less FDW with a validator function (iceberg_catalog_validator) that accepts server-level options.
2. GRANT USAGE ON FOREIGN DATA WRAPPER iceberg_catalog TO lake_write — so non-superusers with lake_write can create catalog servers.

The reserved catalog names postgres, object_store, and rest are not created as foreign servers. They are built-in identifiers whose configuration comes entirely from GUC settings. Only user-created catalogs (via CREATE SERVER ... FOREIGN DATA WRAPPER iceberg_catalog) result in actual foreign server objects.

Connection Resolution

ResolveRestCatalogOptions(catalog) is the single entry point for building a RestCatalogOptions. GetRestCatalogOptionsForRelation(relationId) reads the table's catalog option and delegates to it.

The resolution path is split into two builders plus a dispatcher:

1. ResolveRestCatalogOptions(catalog) — if catalog matches the built-in rest name (case-insensitive via pg_strcasecmp), call BuildRestCatalogOptionsFromGUCs(); otherwise call BuildRestCatalogOptionsFromServer(catalog).
2. BuildRestCatalogOptionsFromGUCs() — materializes options entirely from GUCs. Sets opts->catalog to the canonical REST_CATALOG_NAME constant ("rest").
3. BuildRestCatalogOptionsFromServer(serverName) — looks up the server in pg_foreign_server, initializes all fields from GUC defaults via ApplyGUCDefaults, then overrides with any options explicitly set on the server via ApplyServerOptionOverrides (driven by iceberg_catalog_option_descs[]).
4. ValidateRestCatalogOptions — errors if rest_endpoint is still unset after both sources.

catalog_name on the server is stored in opts->catalogName when present, but it is not defaulted to get_database_name() inside resolution. For read-only tables, the database-name default is applied at CREATE TABLE time: if the table does not specify catalog_name, create_table.c inherits the server's catalog_name option (if set) or defaults to get_database_name(MyDatabaseId), then bakes the result into the table's ftoptions. Writable tables ignore server catalog_name at runtime (see below).

Per-Catalog Token Cache

The old code used a single global token (RestCatalogAccessToken). With multiple catalogs potentially using different credentials, the token cache is now a hash table (RestCatalogTokenCache) keyed by catalog name. Each catalog gets its own cached OAuth token with independent expiry tracking.

The hash table and token strings are allocated in a dedicated RestTokenCacheCtx memory context (under CacheMemoryContext), keeping the cache memory isolated. BuildTokenCacheKey uses opts->catalog as the cache key (zeroed to TOKEN_CACHE_KEY_LEN to avoid hash collisions on short names).

Invalidation on server DDL: a syscache invalidation callback (InvalidateRestTokenCache) is registered once per backend via CacheRegisterSyscacheCallback(FOREIGNSERVEROID, ...). Any ALTER SERVER or DROP SERVER resets the entire token cache (MemoryContextReset(RestTokenCacheCtx)) so stale credentials are never reused in open sessions. The cache is rebuilt lazily on the next token lookup. TokenCacheCallbackRegistered ensures the callback is registered exactly once (the syscache callback array is fixed-size).

SendRequestToRestCatalog implements its own retry loop (up to 3 retries) with a ClassifyRestCatalogRequestRetry classifier that determines whether to back off, refresh auth, or stop. When a 419 (token expired) response is received, the retry logic force-refreshes the token via GetRestCatalogAccessToken(opts, true) and patches the Authorization header in-place via UpdateAuthorizationHeader. The opts parameter is passed directly to SendRequestToRestCatalog, avoiding any global state. Pass opts = NULL for the token-fetch request itself to avoid recursion.

Single-Catalog Transaction Constraint

Modifying tables from different REST catalogs in the same transaction is rejected at statement time by BindRelationToXactRestCatalog(relationId), called from both FDW write entry points:

• postgresBeginForeignModify() — row-by-row DML
• AddQueryResultToTable() — INSERT...SELECT and COPY...FROM pushdown

All per-transaction REST catalog state is grouped in a single PgLakeXactRestCatalogContext struct (static PgLakeXactRestCatalog), which contains:

• requestsHash — hash table of per-table REST catalog requests
• commitContext — pre-allocated 1MB memory context for XACT_COMMIT
• catalogOpts — deep-copied RestCatalogOptions for the single catalog

On the first REST-backed write of the transaction, BindRelationToXactRestCatalog pre-resolves the relation's full RestCatalogOptions and deep-copies them into TopTransactionContext (PgLakeXactRestCatalog->catalogOpts). This happens before any Parquet data is written to S3. Subsequent writes in the same transaction must target the same catalog (compared with pg_strcasecmp); otherwise ERRCODE_FEATURE_NOT_SUPPORTED is raised immediately.

DDL paths (CREATE TABLE / DROP TABLE) reach the same protection indirectly via RecordRestCatalogRequestInTx(), which is invoked synchronously at statement time from the utility hook. That function retains a belt-and-suspenders cross-catalog check for any code path that forgets to bind at statement time.

At XACT_EVENT_COMMIT time, PostAllRestCatalogRequests uses the pre-resolved PgLakeXactRestCatalog->catalogOpts to build URLs and authentication headers. This avoids syscache lookups during commit, which are forbidden (AssertCouldGetRelation fires during TRANS_COMMIT state).

When an iceberg table is created on a user-defined server, RecordIcebergCatalogServerDependency records a DEPENDENCY_NORMAL edge from the table to the server in pg_depend. This ensures DROP SERVER is blocked while dependent tables exist (and DROP SERVER CASCADE drops them). Built-in catalog names (rest, postgres, object_store) do not get a dependency entry because they are not backed by a user-managed pg_foreign_server row.

DDL Protection (ValidateIcebergCatalogServerDDL)

A single ProcessUtility hook validates all DDL on iceberg_catalog servers. The handler is registered in pg_lake_iceberg/src/init.c (where the function is defined), not in pg_lake_table.

Operation	Effect
CREATE SERVER with reserved name (postgres, object_store, rest, case-insensitive)	Blocked
CREATE SERVER with TYPE 'postgres' or 'object_store'	Blocked
CREATE SERVER without TYPE 'rest' (NULL or non-rest)	Blocked
CREATE SERVER with TYPE 'rest'	Allowed
ALTER SERVER ... RENAME TO any reserved name	Blocked
ALTER SERVER ... RENAME TO any non-reserved name on an iceberg_catalog server	Blocked (dependent tables store catalog='<name>' in ftoptions)
ALTER SERVER OPTIONS (SET/ADD rest_endpoint ...) when dependent REST iceberg tables exist	Blocked (writable and read-only, via ServerHasDependentRestIcebergTable)
ALTER/DROP/OWNER on reserved names	Fail naturally (no server object exists)
All other DDL on user-created servers	Allowed

All protection checks are skipped during CREATE EXTENSION / ALTER EXTENSION.

CREATE FOREIGN TABLE on any iceberg_catalog server is also blocked since the FDW has no handler. The error hints users to use CREATE TABLE ... USING iceberg instead.

Catalog Type Helpers (pg_lake_engine/src/utils/catalog_type.c)

• IsRestCatalog(catalog) — unified check that returns true for the literal 'rest' (case-insensitive) or any iceberg_catalog server whose TYPE is 'rest'. Asserts that the server type is non-null and non-empty when a server object exists. Used by the GUC check hook for default_catalog to accept user-created server names.

• IsCatalogOwnedByExtension(catalog) — returns true for the literals 'rest', 'object_store', or 'postgres' (all case-insensitive via pg_strcasecmp). Used by the DDL protection handler and dependency recording.

Option Validation

Server options are defined once in iceberg_catalog_option_descs[] — the single source of truth for the whitelist (FindCatalogOptionDesc), the user-facing hint (GetValidCatalogOptionsHint), and the option-to-struct applier (ApplyCatalogOptionValue). Adding or removing an option requires changing only this table.

FDW option names and auth type values are compared case-insensitively (pg_strcasecmp). Server names remain case-sensitive (PostgreSQL's GetForeignServerByName is case-sensitive), but the reserved built-in names are checked case-insensitively.

At CREATE/ALTER SERVER time, ValidateCatalogOptionValue enforces:

• rest_endpoint, oauth_endpoint, location_prefix: non-empty and must contain :// (URI scheme required)
• catalog_name, client_id, client_secret, scope: non-empty when present
• rest_auth_type: must be 'oauth2', 'default' (alias for oauth2), or 'horizon'
• enable_vended_credentials: valid boolean

The validator hint string is built lazily from iceberg_catalog_option_descs[] and cached in TopMemoryContext so it survives transaction aborts on the error path (a per-statement context would leave a dangling pointer on the second invalid-option failure in the same backend).

catalog_name Server Option

The catalog_name option on an iceberg_catalog server specifies the catalog prefix used in REST API URLs (opts->catalogName).

At CREATE TABLE time (read-only tables): if the table does not specify catalog_name, it inherits the server's catalog_name option (via ResolveRestCatalogOptions) or defaults to get_database_name(MyDatabaseId). The resolved value is baked into the table's ftoptions.

At runtime (GetRestCatalogName):

• Writable tables always return get_database_name(MyDatabaseId). Server and table catalog_name options are ignored. This prevents a subsequent ALTER SERVER ... ADD/SET catalog_name from silently re-routing an existing writable table.
• Read-only tables return the table-level catalog_name from ftoptions. If missing, error (should never happen after CREATE TABLE inheritance).

Writable tables cannot be created on a server with catalog_name set — enforced at CREATE TABLE time.

Accepted Server Options

The iceberg_catalog_validator accepts the following server-level options (all defined in iceberg_catalog_option_descs[]):

Option	Description
rest_endpoint	REST catalog base URL (required; non-empty, must have URI scheme)
rest_auth_type	'oauth2' (default), 'default' (alias for oauth2), or 'horizon'
oauth_endpoint	Custom OAuth token endpoint
scope	OAuth scope (default from GUC: PRINCIPAL_ROLE:ALL)
enable_vended_credentials	Boolean, default true
location_prefix	S3/storage prefix, overrides the GUC default
catalog_name	Catalog name passed to REST API (affects read-only table CREATE TABLE; ignored at runtime for writable tables)
client_id	OAuth client ID
client_secret	OAuth client secret

Credentials are currently stored as server options. USER MAPPING support is planned in a follow-up PR.

Backward Compatibility

• CREATE TABLE ... WITH (catalog = 'rest') continues to work. The catalog option value 'rest' resolves to the built-in configuration sourced entirely from GUCs (case-insensitive: 'REST', 'rEst', etc. all work).
• All existing GUCs remain functional and serve as fallback defaults.
• No changes to postgres or object_store catalog behavior.

Fixes part of #230

---

Checklist

• Probably need to intercept create server commands that use variations of case-insensitive "postgres", "rest" and "object_store" reserved literals.
• I have tested my changes and added tests if necessary
• I updated documentation if needed
• I confirm that all my commits are signed off (DCO)

[sfc-gh-abozkurt]

that sounds a bit weird. maybe IsRestCatalogOwnedByExtension and IsRestCatalogOwnedByUsers

[sfc-gh-npuka]

First I renamed to IsRestCatalogOwnedByUsers but then I realized we don't really need the distinction so I changed it to IsRestCatalog. Details in the last commit of this PR

[sfc-gh-abozkurt]

maybe IsCatalogOwnedByExtension, that catches all our catalogs

[sfc-gh-abozkurt]

should we disallow that?

[sfc-gh-npuka]

we can allow since 'rest' is the only type that is allowed for users for now

[sfc-gh-okalaci]

+1 for disallowing that, perhaps on create server, and we can assert here that TYPE cannot be NULL or empty.

[sfc-gh-abozkurt]

instead of GUC, cant we still have server name for our catalogs?

[sfc-gh-npuka]

makes sense

[sfc-gh-abozkurt]

is this user mapping or server option?

[sfc-gh-npuka]

User mapping. Here only for the sake of separate server infrastructure.

[sfc-gh-abozkurt]

a wrapper seems useful

[sfc-gh-npuka]

Since both extension-owned and user-created REST type catalog servers fallback to GUC, there is no need for a distinction here with if-else, as I changed in my last commit.

[sfc-gh-abozkurt]

we might fetch conn inside GetMetadataLocationFromRestCatalog and do not change its signature maybe.

[sfc-gh-npuka]

We would either need the relation ID or the server name to fetch conn from the inside, so signature change is needed.

[sfc-gh-abozkurt]

I guess this comment should stay.

[sfc-gh-abozkurt]

same, comment should stay

[sfc-gh-abozkurt]

we need to add tests where we modify multiple rest tables inside the same transaction to cover the changes.

[sfc-gh-abozkurt]

Good job! It seems very close to merge to me.

[sfc-gh-abozkurt]

nit: this can be a more generic place

[sfc-gh-abozkurt]

nit: does it makes sense to name default (oauth2 or such)?

[sfc-gh-abozkurt]

maybe BlockDdlOnExtensionCatalogs

[sfc-gh-abozkurt]

Can you add a test tho show ALTER SERVER OWNER TO is also disabled?

[sfc-gh-npuka]

Good point, I may need to intercept AlterOwnerStmt for that

[sfc-gh-abozkurt]

should we prevent that at CREATE FOREIGN SERVER time by hooking into it? Here we can have assertion.

[sfc-gh-npuka]

Good catch - actually by this point the server is guaranteed to be iceberg_catalog, so just an Assert is sufficient.

[sfc-gh-abozkurt]

hint could be alter server option or GUC, same for others for which it applies

[sfc-gh-abozkurt]

shouldnt we invalidate only one token that belongs to the catalog which we send request to? i.e. HASH_REMOVE

[sfc-gh-npuka]

Good point, we could pass the servername to SendRequestToRestCatalog

[sfc-gh-abozkurt]

hooks should exist in pg_lake_table extension. pg_lake_iceberg is only used for iceberg spec and catalog implementations and iceberg specific internal tables or views.

[sfc-gh-abozkurt]

Just asking. Cant we still send a single request? At the first sight, I see polaris open api seems to not allow it. @sfc-gh-okalaci

[sfc-gh-npuka]

You can't send a single HTTP request for multiple hosts. The code currently creates batches based on host.

But, now that you mention it, what if we have two different catalogs, from the same host? This could be a bug. Maybe we should batch on (host, catalog_name) and not simply on (host)

[sfc-gh-abozkurt]

precreated_rest?

[sfc-gh-abozkurt]

I think we should disable table creation with iceberg_catalog fdw as it has no handler via hooks. (we already have create table hooks)

postgres=# create foreign table test (a timestamptz) server postgres;
CREATE FOREIGN TABLE
postgres=# table test;
ERROR:  foreign-data wrapper "iceberg_catalog" has no handler

[sfc-gh-okalaci]

I think we should undo most of the changes here. It is extremely hard to support multiple rest catalogs involved in a tx, especially with modifications involved.

We currently do not even have a mechanism to handle failures if the request for a single REST catalog fails.

When we have multiple REST catalogs involved, the failure handling / recovery becomes extremely hard to handle. What happens if request to server A succeeds and request to server B fails?

I think if we merge this PR as-is, we'd make it much harder to handle the recovery scenarios, which we have not even handled even for a single end-point.

So, let's just fail the transaction pre-commit XACT_EVENT_PRE_COMMIT, perhaps in RecordRestCatalogRequestInTx where we throw error if you ever try to record changes to more than one rest catalog.

In general, many query engines do not even support multi-table transactions yet along multi-server :) So, we are not contradicting with any user expectations.

[sfc-gh-npuka]

Good point, I overlooked this part.

[sfc-gh-okalaci]

should this be case insensitive? It is more common in pg_lake and in Postgres to allow case insensitive keywords.

Make sure it works in other places as well if we change it, for example some tests should use different cases

[sfc-gh-okalaci]

similarly: can we allow case insensitive values? make sure if we allow it here, we check in other places similarly

[sfc-gh-okalaci]

nit: I think we have some indentation issues in the PR, such as this one. Can you maybe go over once more regarding that? Do you use make reindent? That seems to fix some of these on my local

[sfc-gh-okalaci]

static variables are always painful to maintain, what if SendHttpRequestWithRetry throws an error? We are left with the old server name. I'm pretty sure that cause hard-to-understand issues/consequences.

I think what we are trying here is to pass ShouldRetryRequestToRestCatalog the server name.

Can't we change HttpRetryFn signature to pass the server name?

[sfc-gh-okalaci]

uf, the name postgres might be used by the users, say for an postgres_fdw server.

I wonder if we should pick something harder to hit? Can you think of anything prefix/postfix for the pre-created servers?

[sfc-gh-okalaci]

hm, we remove the token from the cache, and return true, so the caller would simply re-use the existing headers to retry, which means will use the old token.

I think we should re-generate token and add here? Especially given we return true, we should do something like that.

[sfc-gh-npuka]

Good point, we can use GetRestCatalogAccessToken like the previous code was using. We wouldn't even need hash_search(..., HASH_REMOVE, ...) since there is existing forceRefreshToken logic.
Note that Claude is suggesting we had a previous bug of using the same pre-built headers after GetRestCatalogAccessToken(forceRefreshToken); So taking care of that as well.

[sfc-gh-npuka]

Opened an issue for this: #276
Not sure if we should solve it in this PR or separately

EDIT: maybe better to solve this separately as this PR is getting huge #278

[sfc-gh-okalaci]

we dont seem to be using location_prefix ? I mean, even if I provide that, we still use GetIcebergDefaultLocationPrefix when needed in rest_catalog.c?

I think if we support this, we should have tests such that we set GetIcebergDefaultLocationPrefix to return a location that's not working, create/modify/vacuum/ddl tables such that make sure we don't accidentally use it.

[sfc-gh-npuka]

Sorry about that, huge miss from my part.

[sfc-gh-npuka]

nit: I am reusing the "remove trailing slash" code - think I'll open a separate PR about that since we are also using it for stage_location and other things.

EDIT: opened and merged separate PR on this #275

[sfc-gh-okalaci]

aren't we supposed to allow use-defined catalogs?

I thought one of the most important pieces of this PR is to let users easily use/switch between the servers, and this seems like a good place to be able to do that?

Needs tests to show we allow user created rest server, but not allow say postgres_fdw servers or unrelated/non-existing ones

[sfc-gh-npuka]

Good catch! I forgot about this part.

[sfc-gh-okalaci]

I think postgres server name is case sensitive? https://github.com/postgres/postgres/blob/4f433025f666fa4a6209f0e847715767fb1c7ace/src/backend/foreign/foreign.c#L794

I think for server names we should use strcmp ?

ps: good idea to test two servers: CREATE SERVER "test" and CREATE SERVER "TEST" in the same db, with different options and make sure they both work fine?

[sfc-gh-npuka]

Could do that.
In that case, we would allow names like "Object_Store", so functions like HasObjectStoreCatalogTableOption should also be case sensitive.

[sfc-gh-npuka]

on second thought, I think we should keep case insensitive comparisons against POSTGRES_CATALOG_NAME, OBJECT_STORE_CATALOG_NAME and REST_CATALOG_NAME. We can't have "rest" and "REST". But we can have "test" and "TEST".

[sfc-gh-okalaci]

I think this comment keyed by host + clientId) is stale?

[sfc-gh-npuka]

true, it's keyed by server name

[sfc-gh-okalaci]

we'll remove this code, but anyway this is not safe, we can have multiple REST catalogs in the same host? So we cannot really compare the host?

If we need this kind of grouping in any other place in the code, we should do with the OID of the server instead?

[sfc-gh-npuka]

It is actually checking both host and catalog name which is unique.

if (strcmp(requestPerTable->conn->host, batchHost) != 0 ||
strcmp(requestPerTable->catalogName, catalogName) != 0)

Server name/oid would also work (better since it would be a single check).

[sfc-gh-okalaci]

should we register this in pg_lake_icebeg? Any reason to do it in pg_lake_table?

[sfc-gh-npuka]

#242 (comment)

[sfc-gh-okalaci]

Optional) Cold-start test: ALTER SYSTEM SET pg_lake_iceberg.default_catalog = '<user_server>'; SELECT pg_reload_conf(); and verify a fresh connection can use it. This exercises the GUC check hook against a user-created server outside of a transaction state. Which, I think will not work given the IsTransactionState() check we have?

This seems to be broken, can you if Postgres has a different way of handling it?

alter system set pg_lake_iceberg.default_catalog to 'my_open_catalog_2';
ALTER SYSTEM
Time: 4.330 ms
postgres=# select pg_reload_conf();
 pg_reload_conf 
----------------
 t
(1 row)

Time: 1.879 ms
postgres=# show pg_lake_iceberg.default_catalog;
 pg_lake_iceberg.default_catalog 
---------------------------------
 postgres
(1 row)

Time: 1.216 ms

[sfc-gh-okalaci]

What's the expected behavior when dropping a server? Can you see what postgres_fdw does?

It seems we somehow let the

set pg_lake_iceberg.default_catalog to 'my_open_catalog_2';  
 create table t_test_rest_read_3() using iceberg WITH (read_only=true, catalog_table_name='t_test_rest_write');
table t_test_rest_read_3;
 a 
---
 1
 2
 3
 4
(4 rows)

drop server my_open_catalog_2 ;

 select * from t_test_rest_read_3;
ERROR:  missing field for column

And, the backtrace suggests we treat this as an postgres catalog table, not REST catalog table:

 bt
* thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1
  * frame #0: 0x0000000100fb7814 postgres`errfinish(filename="src/fdw/schema_operations/field_id_mapping_catalog.c", lineno=125, funcname="GetRegisteredFieldForAttributes") at elog.c:479:33
    frame #1: 0x0000000101e6df3c pg_lake_table.dylib`GetRegisteredFieldForAttributes(relationId=251939, attrNos=0x0000000b84c70da8) at field_id_mapping_catalog.c:123:3
    frame #2: 0x0000000101e6ed50 pg_lake_table.dylib`GetDataFileSchemaForInternalIcebergTable(relationId=251939) at field_id_mapping_catalog.c:401:20
    frame #3: 0x0000000101e71124 pg_lake_table.dylib`GetDataFileSchemaForTableInternal(relationId=251939) at register_field_ids.c:305:10
    frame #4: 0x0000000101e710d8 pg_lake_table.dylib`GetDataFileSchemaForTable(relationId=251939) at register_field_ids.c:324:9
    frame #5: 0x0000000101e457c8 pg_lake_table.dylib`BuildReadDataSourceQueryForTableScan(tableScan=0x0000000b84e796a8, skipFullMatchFiles=false, projection=0x0000000000000000) at transform_query_to_duckdb.c:216:12
    frame #6: 0x0000000101e4548c pg_lake_table.dylib`ReplaceReadTableFunctionCalls(query=" SELECT a\n   FROM __lake_read_table('public.t_test_rest_read_3'::text, 24) t_test_rest_read_3(a)", snapshot=0x0000000b852b1ff8, scanFlags=0) at transform_query_to_duckdb.c:100:28
    frame #7: 0x0000000101e7b3e8 pg_lake_table.dylib`QueryPushdownBeginScan(node=0x0000000b852b0350, estate=0x0000000b852b0100, eflags=32) at query_pushdown.c:1436:25
    frame #8: 0x0000000100a82498 postgres`ExecInitCustomScan(cscan=0x0000000b84e2d5c0, estate=0x0000000b852b0100, eflags=32) at nodeCustom.c:108:2
    frame #9: 0x0000000100a621a8 postgres`ExecInitNode(node=0x0000000b84e2d5c0, estate=0x0000000b852b0100, eflags=32) at execProcnode.c:290:27
    frame #10: 0x0000000100a5498c postgres`InitPlan(queryDesc=0x0000000b85136d00, eflags=32) at execMain.c:959:14
    frame #11: 0x0000000100a54308 postgres`standard_ExecutorStart(queryDesc=0x0000000b85136d00, eflags=32) at execMain.c:260:2
    frame #12: 0x0000000101652960 pgaudit.dylib`pgaudit_ExecutorStart_hook + 452
    frame #13: 0x00000001016d0b0c auto_explain.dylib`explain_ExecutorStart + 392
    frame #14: 0x0000000101db98ec pg_stat_statements.dylib`pgss_ExecutorStart + 56
    frame #15: 0x0000000100a53f9c postgres`ExecutorStart(queryDesc=0x0000000b85136d00, eflags=0) at execMain.c:134:3
    frame #16: 0x0000000100d729b0 postgres`PortalStart(portal=0x0000000b85020100, params=0x0000000000000000, eflags=0, snapshot=0x0000000000000000) at pquery.c:517:5
    frame #17: 0x0000000100d6de7c postgres`exec_simple_query(query_string="select * from t_test_rest_read_3;") at postgres.c:1239:3
    frame #18: 0x0000000100d6d0d0 postgres`PostgresMain(dbname="postgres", username="okalaci") at postgres.c:4767:7
    frame #19: 0x0000000100d64d60 postgres`BackendMain(startup_data="", startup_data_len=4) at backend_startup.c:105:2
    frame #20: 0x0000000100c53ec8 postgres`postmaster_child_launch(child_type=B_BACKEND, startup_data="", startup_data_len=4, client_sock=0x000000016f6fa638) at launch_backend.c:277:3
    frame #21: 0x0000000100c5b43c postgres`BackendStartup(client_sock=0x000000016f6fa638) at postmaster.c:3596:8
    frame #22: 0x0000000100c585b8 postgres`ServerLoop at postmaster.c:1678:6
    frame #23: 0x0000000100c575ac postgres`PostmasterMain(argc=3, argv=0x0000000102bd2670) at postmaster.c:1376:11
    frame #24: 0x0000000100af1428 postgres`main(argc=3, argv=0x0000000102bd2670) at main.c:199:3
    frame #25: 0x0000000182241d54 dyld`start + 7184
(lldb) c

[sfc-gh-okalaci]

Let's go back to where this PR was at ee6a75c

The DROP SERVER my_open_catalog_2 silent-orphan I flagged earlier in a line comment got me pulling on the thread, and once I re-read the commit history on this branch I realized the PR already had the right architecture and my own earlier review comment is what pushed it off course. I'd like to walk that back.

What ee6a75c had, and what I said about it

At ee6a75c ("Add iceberg_catalog FDW and refactor REST catalog for multi-server support"), the extension install script created real pg_foreign_server rows for the built-ins:

CREATE SERVER postgres      TYPE 'postgres'     FOREIGN DATA WRAPPER iceberg_catalog;
CREATE SERVER object_store  TYPE 'object_store' FOREIGN DATA WRAPPER iceberg_catalog;

They were extension-owned, so DEPENDENCY_EXTENSION pinned them. User catalogs were just more CREATE SERVER … FOREIGN DATA WRAPPER iceberg_catalog. Every foreign table would have then ended up with ftserver = <its catalog's OID> — the plain postgres_fdw model.

At review I then said:

uf, the name postgres might be used by the users, say for a postgres_fdw server. I wonder if we should pick something harder to hit? Can you think of anything prefix/postfix for the pre-created servers?

The naming concern was real — CREATE SERVER postgres FOREIGN DATA WRAPPER postgres_fdw is in every tutorial, and a fresh CREATE EXTENSION pg_lake_iceberg would fail on any database that already has one. But the response it triggered was much bigger than the fix needed.

048b9bb went further than it had to

"Treat postgres, object_store, and rest as built-in names, not servers" deleted the two CREATE SERVER lines and reshaped the whole option-resolution layer around string sentinels:

• GetRestCatalogOptionsFromCatalog grew a "return GUC-only opts for the built-in rest/postgres/object_store names without looking up a foreign server" branch.
• IsIcebergCatalogServer was removed.
• BlockDDLOnExtensionCatalogs was collapsed to mostly reject CREATE SERVER with reserved names at DDL parse time.
• RestCatalogOptions.serverName was renamed to .catalog — a string, no longer an OID handle.
• 200+ lines of tests exercising the extension-owned-server behavior were dropped.

Once the built-ins became string sentinels rather than real servers, the whole tree followed: catalog became a string stored in ftoptions, pg_foreign_table.ftserver on iceberg tables collapsed onto the single shared pg_lake_iceberg row, pg_depend lost its natural hook (no OID to point at for the built-in case, so user catalogs also had to be represented as strings for consistency), and every piece of postgres_fdw server-management plumbing had to be rebuilt by hand — or, as it turns out, simply went missing. That last category is what the rest of this comment is about.

Only the name was the problem. None of this restructuring was necessary to fix it.

What we missed — the small fix

The minimum change to address my naming concern without touching the architecture:

1. Prefix the internal names so they can't collide with what users already have:

   CREATE SERVER pg_lake_postgres_catalog     TYPE 'postgres'     FOREIGN DATA WRAPPER iceberg_catalog;
   CREATE SERVER pg_lake_object_store_catalog TYPE 'object_store' FOREIGN DATA WRAPPER iceberg_catalog;
   CREATE SERVER pg_lake_rest_catalog         TYPE 'rest'         FOREIGN DATA WRAPPER iceberg_catalog;

   (And now rest gets a real server too, which is a nice side-benefit — GUC-based config becomes one code path instead of a special case.)

2. Keep the user-facing DDL CREATE TABLE … USING iceberg WITH (catalog='postgres', …) exactly as it is. Internally, map the short name to the prefixed server name at DDL time:

   static const char *
   ResolveCatalogToServerName(const char *catalog)
   {
       if (pg_strcasecmp(catalog, "postgres")     == 0) return "pg_lake_postgres_catalog";
       if (pg_strcasecmp(catalog, "object_store") == 0) return "pg_lake_object_store_catalog";
       if (pg_strcasecmp(catalog, "rest")         == 0) return "pg_lake_rest_catalog";
       return catalog;   /* user catalogs: server name == catalog name */
   }

3. Keep the existing ValidateIcebergCatalogServerDDL guard so users still can't literally CREATE SERVER postgres FOREIGN DATA WRAPPER iceberg_catalog. The three short names stay unambiguous at DDL-parse time, error messages still talk about the names the user typed, and nothing users have in their database can collide.

   And to be fully explicit on the original concern: a pre-existing CREATE SERVER postgres FOREIGN DATA WRAPPER postgres_fdw OPTIONS (host '…', dbname '…') is entirely unrelated to us. We don't create a server called postgres anymore (it's pg_lake_postgres_catalog), ResolveCatalogToServerName maps the short name at DDL rewrite time without ever calling GetForeignServerByName("postgres"), and the ValidateIcebergCatalogServerDDL guard only fires on FOREIGN DATA WRAPPER iceberg_catalog. Users' postgres_fdw servers, their postgres_fdw user mappings, their foreign tables against them — all of it works untouched. CREATE EXTENSION pg_lake_iceberg cannot fail on name conflict the way ee6a75c could have. Though, still good to have a simple test with postgres_fdw server named postgres just to be sure.

That's it. My naming concern is answered, and the ee6a75c architecture is preserved.

What this means concretely for this PR

Revert the structural parts of 048b9bb and keep ee6a75c's shape:

• Restore the extension-owned built-in servers, under prefixed names, including a new pg_lake_rest_catalog.
• Restore IsIcebergCatalogServer (or equivalent) and the path in GetRestCatalogOptionsFromCatalog that resolves by OID via GetForeignServerByName, not by string sentinel.
• Rename RestCatalogOptions.catalog back to serverName (or equivalent) — it's an OID/server handle, not a free-form string.
• Restore the tests 048b9bb removed.
• Add ResolveCatalogToServerName at the DDL rewrite layer.
• Store the catalog reference in pg_foreign_table.ftserver (via CREATE FOREIGN TABLE … SERVER <resolved>) and drop the catalog option from ftoptions. Today every iceberg foreign table has ftserver = <shared pg_lake_iceberg OID> and the actual catalog appears only as the string value of the catalog option — so even after we restore real built-in servers, recordDependencyOn on its own is still not quite enough: ALTER SERVER foo RENAME TO bar updates pg_foreign_server.srvname but nothing rewrites strings inside dependents' ftoptions, and the next query calls GetForeignServerByName("foo", false) and fails. Moving the reference into ftserver is what makes rename Just Work, for the same reason it does in postgres_fdw: everyone references the server by its stable OID, and renames don't touch OIDs. This is also the piece that lets us get pg_depend edges, ACL_USAGE at CREATE TABLE, and correct pg_dump ordering from core's CreateForeignTable for free rather than hand-rolling them. pg_lake_table FDW and the pg_lake server for parquet/csv/json analytics stay exactly as they are — out of scope.

I'm sorry for the churn here — if I'd thought harder about the naming concern the first time, the resolver idea was the obvious answer and none of this would have come up.

---

Appendix — what the current string-sentinel shape costs us

Every row here is something core Postgres gets for free from postgres_fdw via pg_foreign_table.ftserver + the recordDependencyOn(&tableAddr, &serverAddr, DEPENDENCY_NORMAL) call in CreateForeignTable. All of these behaviors come back as soon as we go back to real servers referenced by OID.

Operation	Today on this branch	What postgres_fdw does
DROP SERVER foo with dependent iceberg tables	succeeds silently, next query on the table crashes with missing field for column	errors with "other objects depend on it"
DROP SERVER foo CASCADE	orphans the dependent iceberg tables	cascades to drop them
ALTER SERVER foo RENAME TO bar	succeeds, all dependent tables silently break (their catalog option still says foo)	succeeds, tables keep working because they reference the stable OID
REVOKE USAGE ON FOREIGN SERVER foo FROM alice	does not prevent alice from CREATE TABLE … catalog='foo' — there is no USAGE check at CREATE TABLE time (see separate ACL_USAGE_CHECK_MISSING.md line comment)	object_aclcheck(ACL_USAGE) runs inside CreateForeignTable
DROP OWNED BY alice where alice owns a catalog server	leaves iceberg tables dangling against a now-dropped server	cascades via the dependency edge
DROP FOREIGN DATA WRAPPER iceberg_catalog CASCADE	does not reach pg_lake iceberg tables	full FDW → servers → tables cascade
DROP EXTENSION pg_lake_iceberg CASCADE	for built-in postgres / object_store / rest — no servers to cascade through, user tables get orphaned	extension-owned servers cascade to their dependent tables
pg_dump + pg_restore of a database with iceberg tables and user catalog servers	can emit the table before the server (no edge to order them); manual --section sequencing needed	dependency walker orders server before table automatically
Concurrent DROP SERVER foo during an active query against a dependent table	race; no lock held on the server during the query	core holds AccessShareLock on the server via the edge, DROP SERVER blocks until the query releases
iceberg_catalog_server_options[] vs. GetRestCatalogOptionsFromCatalog consumer branch (tracked separately in OPTION_SCHEMA_SINGLE_SOURCE.md)	two independent lists that silently drift; unhandled options are dropped	single FDW validator + single consumer, no two-FDW sync problem

Everything above goes away — or becomes core-behavior-for-free — once we go back to the ee6a75c shape with prefixed names and the resolver.

[sfc-gh-okalaci]

nit: GetRestCatalogOptionsFromCatalog is doing two different things behind one name.

For the built-in rest name we never read from a catalog at all — we just materialize GUCs. For a user-created server we read pg_foreign_server and layer it on top of the GUC defaults. A single function called ...FromCatalog hides that split and makes the GUC-only path look like "reading from a catalog".

I'd split it into a small dispatcher + two builders (plus an optional helper), so each function name matches exactly one thing:

/* Entry point: picks the right source based on the catalog identifier. */
RestCatalogOptions *
ResolveRestCatalogOptions(const char *catalog)
{
    if (pg_strcasecmp(catalog, REST_CATALOG_NAME) == 0)
        return BuildRestCatalogOptionsFromGUCs();

    return BuildRestCatalogOptionsFromServer(catalog);
}

/* Built-in 'rest' catalog: GUCs only, no server lookup. */
static RestCatalogOptions *
BuildRestCatalogOptionsFromGUCs(void)
{
    RestCatalogOptions *opts = palloc0(...);
    opts->catalog = pstrdup(REST_CATALOG_NAME);   /* canonical form */
    ApplyGUCDefaults(opts);
    ValidateRestCatalogOptions(opts, REST_CATALOG_NAME);
    return opts;
}

/* User-created iceberg_catalog server: GUC defaults + server option overrides. */
static RestCatalogOptions *
BuildRestCatalogOptionsFromServer(const char *serverName)
{
    ForeignServer      *server = GetForeignServerByName(serverName, false);
    ForeignDataWrapper *fdw    = GetForeignDataWrapper(server->fdwid);

    Assert(strcmp(fdw->fdwname, ICEBERG_CATALOG_FDW_NAME) == 0);

    RestCatalogOptions *opts = palloc0(...);
    opts->catalog = pstrdup(serverName);          /* case-sensitive, as stored */
    ApplyGUCDefaults(opts);                       /* defaults first... */
    ApplyServerOptionOverrides(opts, server);     /* ...then per-server overrides */
    ValidateRestCatalogOptions(opts, serverName);
    return opts;
}

/* Small shared helpers keep the two builders symmetric. */
static void ApplyGUCDefaults(RestCatalogOptions *opts);
static void ApplyServerOptionOverrides(RestCatalogOptions *opts, ForeignServer *server);
static void ValidateRestCatalogOptions(const RestCatalogOptions *opts, const char *catalog);

Why this is clearer:

• The name ResolveRestCatalogOptions conveys "figure out where the config comes from" rather than "read from a catalog".
• BuildRestCatalogOptionsFromGUCs / BuildRestCatalogOptionsFromServer make the two sources explicit at call sites and in stack traces.
• The if (catalog != 'rest') { ... } negative branch and the "GUCs as defaults, then override" two-phase logic disappear into named helpers, so the top-level flow reads as a single dispatch.
• Callers like GetRestCatalogOptionsForRelation only need to know about ResolveRestCatalogOptions; they don't encode the built-in-vs-server distinction.

If you want to go one step further, the GUC side could return a cached/shared RestCatalogOptions * since it never depends on inputs — but that's orthogonal to the rename.

[sfc-gh-okalaci]

Also, let's always do pstrdup for the string fields into the context of the returned RestCatalogOptions struct, otherwise we'll have a struct where its fields are created in different contexts.

[sfc-gh-okalaci]

Flagging this as a serious regression — token caching is broken, so we're doing a fresh OAuth handshake on every catalog request.

On main one token was reused for ~1h; after this PR every metadata GET and every commit POST is preceded by a POST /oauth/tokens, which roughly doubles the latency of every Iceberg op and 2×'s the load on Polaris / the IdP. We should be a bit more careful about perf regressions in this path.

Fix

In BuildTokenCacheKey, zero the key buffer before strlcpy:

MemSet(key, 0, TOKEN_CACHE_KEY_LEN);
strlcpy(key, opts->catalog, TOKEN_CACHE_KEY_LEN);

The hash table is HASH_BLOBS, so dynahash hashes the full TOKEN_CACHE_KEY_LEN bytes of the key. The caller passes a stack-allocated char cacheKey[TOKEN_CACHE_KEY_LEN] and strlcpy only writes strlen(catalog)+1 bytes, so the trailing bytes are uninitialized stack garbage — every lookup ends up with a different effective key and found is always false, forcing a token re-fetch.

How to see it yourself (useful trick)

There's a GUC that traces every HTTP request the REST catalog makes — super handy for debugging this kind of thing:

SET pg_lake_iceberg.http_client_trace_traffic TO ON;

With that on, on the current (broken) code you'll see this pattern repeat for every statement:

INSERT INTO t_cat_2_write_2 VALUES (1);
INFO:  POST .../oauth/tokens         ← token fetch
INFO:  GET  .../tables/t_cat_2_write_2
INFO:  POST .../oauth/tokens         ← AGAIN
INFO:  GET  .../tables/t_cat_2_write_2
INFO:  POST .../transactions/commit

INSERT INTO t_cat_2_write_2 VALUES (1);
INFO:  POST .../oauth/tokens         ← AGAIN
INFO:  GET  .../tables/t_cat_2_write_2
INFO:  POST .../oauth/tokens         ← AGAIN
...

After the fix the first statement does one OAuth POST, and subsequent ops in the same session reuse it (until expires_in - 60s):

INSERT INTO t_cat_2_write_2 VALUES (1);
INFO:  making POST  …/oauth/tokens …           -- token #1, expires_in=3600
INFO:  making GET   …/namespaces/public/tables/t_cat_2_write_2
INFO:  making GET   …/namespaces/public/tables/t_cat_2_write_2
INFO:  making POST  …/transactions/commit …
INSERT INTO t_cat_2_write_2 VALUES (1);        -- no oauth POST anymore
INFO:  making GET   …/namespaces/public/tables/t_cat_2_write_2
INFO:  making GET   …/namespaces/public/tables/t_cat_2_write_2
INFO:  making POST  …/transactions/commit …

Would be great to also add a small regression test that asserts FetchRestCatalogAccessToken is called exactly once across N back-to-back catalog ops in the same session, so we don't silently regress this again. I don't know how to do that, but Claude might help :)

[sfc-gh-npuka]

Very important catch. Added test_token_cache_reuses_token_across_catalog_ops: 3 back to back inserts in the same session. Exactly one token fetch. Also verified that the test fails when commenting out MemSet(key, 0, TOKEN_CACHE_KEY_LEN);

[sfc-gh-okalaci]

nit: CacheMemoryContext is probably slightly better over TopMemoryContext

[sfc-gh-okalaci]

Follow-up: the token cache isn't invalidated on ALTER SERVER, so stale credentials silently keep working in open sessions.

Repro (same backend, token already cached):

-- session #1, already has a valid cached bearer token from client_id='real'
ALTER SERVER my_open_catalog_2 OPTIONS (SET client_id 'bogus');   -- invalid creds now

INSERT INTO t_cat_2_write_2 VALUES (1);
-- INSERT 0 1   <-- succeeds (!), using the cached token from the previous client_id

But from a fresh connection, the same statement correctly fails:

-- session #2 (new psql), no cache yet
INSERT INTO t_cat_2_write_2 VALUES (1);
-- ERROR:  Rest Catalog OAuth token request failed (HTTP 401)
-- DETAIL: {"error":"unauthorized_client","error_description":"The client is not authorized"}

Symmetric problem: rotating a good secret via ALTER SERVER won't pick up until the bearer token's 1-hour TTL elapses (or the backend restarts), so users have to kick every connection manually after a rotation.

The fix is to hook a syscache invalidation callback on pg_foreign_server from InitTokenCacheIfNeeded and reset RestTokenCacheCtx on any change:

static void
InvalidateRestTokenCache(Datum arg, int cacheid, uint32 hashvalue)
{
    if (RestCatalogTokenCache == NULL)
        return;

    MemoryContextReset(RestTokenCacheCtx);
    RestCatalogTokenCache = NULL;   /* rebuilt lazily on next lookup */
}

/* in InitTokenCacheIfNeeded, after hash_create: */
CacheRegisterSyscacheCallback(FOREIGNSERVEROID, InvalidateRestTokenCache, (Datum) 0);

• Would be useful to have a test on this.
• This clears all servers' cache, but given we expect handful of server with un-frequent changes to server, this should be all fine.

[sfc-gh-npuka]

Also very important catch, thank you. Added test_alter_server_credentials_invalidates_token_cache test. It tests that cache is invalidated on bogus credentials (1 fetch, insert commit fails), then cache is invalidated again on restored credentials (1 fetch, insert commit succeeds). Also verified that the test fails when commenting out InvalidateRestTokenCache fix.

[sfc-gh-okalaci]

super rare, but I think opts can be NULL in some edge/failure cases, so we better throw error if opts=NULL here.

[sfc-gh-okalaci]

Good that we prevent this on CREATE TABLE time, but I think we need a similar check in the ALTER SERVER time, otherwise we can get into the same situation that we try to prevent here.

-- 1) Server WITHOUT catalog_name
CREATE SERVER my_open_catalog_4 TYPE 'rest'
  FOREIGN DATA WRAPPER iceberg_catalog
  OPTIONS (rest_endpoint 'https://preprod8_datalake_polaris_test.preprod8.us-west-2.aws.snowflakecomputing.com/polaris', client_id '....=', client_secret '/....=',
           location_prefix 's3://....-uswest2');

-- 2) Writable table succeeds: server has no catalog_name at create time
CREATE TABLE t_cat_4_write(a int) USING iceberg
  WITH (catalog = 'my_open_catalog_4');

-- 3) Observe "before" routing (uses MyDatabaseId -> 'postgres')
SET pg_lake_iceberg.http_client_trace_traffic TO ON;
INSERT INTO t_cat_4_write VALUES (1);
-- ...POST /polaris/api/catalog/v1/postgres/transactions/commit ...

-- 4) Add catalog_name to the SAME server AFTER the table exists
ALTER SERVER my_open_catalog_4 OPTIONS (ADD catalog_name 'otherdb');

-- 5) Same table, same transaction boundary — commit now goes to 'otherdb' namespace
INSERT INTO t_cat_4_write VALUES (1);
INFO:  making GET request to URL https://preprod8_datalake_polaris_test.preprod8.us-west-2.aws.snowflakecomputing.com/polaris/api/catalog/v1/otherdb/namespaces/public/tables/t_cat_4_write
INFO:  received response with status code 404, body: {"error":{"message":"Table does not exist: public.t_cat_4_write","type":"NoSuchTableException","code":404}}
ERROR:  HTTP request failed (HTTP 404)
DETAIL:  Table does not exist: public.t_cat_4_write
HINT:  The rest catalog returned error type: NoSuchTableException
Time: 2813.779 ms (00:02.814)

[sfc-gh-okalaci]

I feel like the right fix is to change GetRestCatalogName for READ_WRITE tables to always return get_database_name as already done in the main

[sfc-gh-okalaci]

(I think we might have a similar bug in for changing rest_endpoint for a given server. That, we should perhaps hook into ALTER FOREIGN SERVER and perhaps throw error if we have tables with the old endpoint registered already?)

[sfc-gh-okalaci]

nit:

postgres_fdw's validator type-checks every option it knows about; we should at least match that bar. Suggested minimum checks:

• rest_endpoint: non-empty, and strstr(value, "://") != NULL (reject anything without a scheme).
• oauth_endpoint: same, when present.
• location_prefix: non-empty and contains ://.
• catalog_name: non-empty when present.
• client_id / client_secret / scope: non-empty when present.

Catching these at DDL time means a typo fails with ERROR: invalid "rest_endpoint": "…" next to the CREATE SERVER statement, instead of surfacing as "Rest Catalog OAuth token request failed (HTTP 0): CURLE_URL_MALFORMAT" an hour later during someone else's INSERT.

[sfc-gh-okalaci]

DML: cross-catalog transaction check fires too late

This restriction is fine for DDL — CREATE TABLE and DROP TABLE call RecordRestCatalogRequestInTx synchronously from the utility handler, so a mismatch errors out at statement time.

For DML it fires way too late: ApplyTrackedIcebergMetadataChanges only runs at XACT_EVENT_PRE_COMMIT, by which time every INSERT/UPDATE/DELETE in the transaction has already run to completion and pushed Parquet files to S3. The user burns a full write roundtrip (and waits for it) just to be told at commit time that the second statement wasn't allowed in the first place. We should fail fast on the statement itself.

We should add a relation-level guard that resolves the catalog itself and no-ops for non-REST relations, so every DML entry point becomes a single-line call.

(EnsureRelationInXactRestCatalog is not the best name, please find something else)

/* track_iceberg_metadata_changes.h */
extern PGDLLEXPORT void EnsureRelationInXactRestCatalog(Oid relationId);

/* track_iceberg_metadata_changes.c */
void
EnsureRelationInXactRestCatalog(Oid relationId)
{
    if (!IsPgLakeIcebergForeignTableById(relationId) ||
        GetIcebergCatalogType(relationId) != REST_CATALOG_READ_WRITE)
        return;                          /* not a REST-backed relation, nothing to check */

    char *catalog = GetStringOption(GetForeignTable(relationId)->options,
                                    "catalog", false);

    if (catalog == NULL ||
        PgLakeXactRestCatalog == NULL ||
        PgLakeXactRestCatalog->catalogOpts == NULL)
        return;                          /* nothing locked in yet, or relation has no catalog option */

    if (strcmp(PgLakeXactRestCatalog->catalogOpts->catalog, catalog) != 0)
        ereport(ERROR,
                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                 errmsg("cannot modify tables from different REST catalogs "
                        "in the same transaction"),
                 errdetail("This transaction already targets catalog \"%s\", "
                           "but the current statement targets \"%s\".",
                           PgLakeXactRestCatalog->catalogOpts->catalog, catalog)));
}

There are three DML entry points that bypass each other, and all of them need the guard — otherwise the two pushdown paths silently go to pre-commit today:

1. Row-by-row DML (INSERT/UPDATE/DELETE without pushdown) — goes through the executor's ModifyTable machinery:

/* pg_lake_table/src/fdw/pg_lake_table.c — postgresBeginForeignModify */
void
postgresBeginForeignModify(ModifyTableState *mtstate,
                           ResultRelInfo *resultRelInfo, ...)
{
    EnsureRelationInXactRestCatalog(RelationGetRelid(resultRelInfo->ri_RelationDesc));

    /* …existing body… */
}

2. INSERT ... SELECT pushdown — TransformPushdownableInsertSelect rewrites the INSERT into a pure CMD_SELECT, so the executor never instantiates a ModifyTable and postgresBeginForeignModify is never called.

3. COPY ... FROM pushdown — ProcessPgLakeCopyFrom short-circuits at the doCopyPushdown branch and never calls BeginCopyFrom/CopyFrom, so postgresBeginForeignModify is never called here either.

Both 2. and 3. pushdown paths converge on AddQueryResultToTable in pg_lake_table/src/fdw/writable_table.c, and the first thing it does downstream (PrepareToAddQueryResultToTable) is the DuckDB query that writes Parquet to S3. So plant the guard at the top of that function:

/* pg_lake_table/src/fdw/writable_table.c — AddQueryResultToTable */
int64
AddQueryResultToTable(Oid relationId, char *readQuery, TupleDesc queryTupleDesc,
                      bool wrapNativeIntervals)
{
    Assert(queryTupleDesc != NULL && queryTupleDesc->natts > 0);

    EnsureRelationInXactRestCatalog(relationId);

    /* …existing body… */
}

Once the three guards above are in place, the existing cross-catalog check here is effectively dead code — every realistic DML/DDL path now errors out at statement time before ever reaching pre-commit. It's still worth keeping as a defensive fallback for any future entry point we add, so maybe add a comment:

/*
 * Belt-and-suspenders check. All DML and DDL entry points already call
 * EnsureRelationInXactRestCatalog() at statement time, so in practice
 * we never reach here with a mismatched catalog. Kept as a last line
 * of defense for any future code path that forgets to do so.
 */

[sfc-gh-okalaci]

we maintain the list of options in 3 different places in the code, and I find it difficult in the future to add/remove new options. Can you think of a way to unify the list at a single point, then each caller relies on that?

1. `iceberg_catalog_server_options[]` — the whitelist used by `is_valid_iceberg_catalog_option` to reject unknown options at `CREATE`/`ALTER SERVER`.
2. The `errhint` inside `iceberg_catalog_validator` — the human-readable list shown to users who typo an option name.
3. The long `else if (pg_strcasecmp(def->defname, "…") == 0)` chain in `GetRestCatalogOptionsFromCatalog` — the *consumer* that actually applies each option.

[sfc-gh-okalaci]

missing USAGE check on the iceberg_catalog server at CREATE TABLE time

Core's CreateForeignTable at foreigncmds.c line 1447 does this right before it writes the pg_foreign_table row:

aclresult = object_aclcheck(ForeignServerRelationId, server->serverid,
                            ownerId, ACL_USAGE);
if (aclresult != ACLCHECK_OK)
    aclcheck_error(aclresult, OBJECT_FOREIGN_SERVER, server->servername);

pg_lake's iceberg CREATE TABLE path never performs this check. Today any user with CREATE on a schema can attach a pg_lake iceberg table to any admin-configured iceberg_catalog server, regardless of whether USAGE was granted to them on that server. The DBA who said "this role must not touch the prod REST catalog" has no way to enforce it.

Fix

In ProcessCreateIcebergTableFromForeignTableStmt (pg_lake_table/src/ddl/create_table.c), at the top of the if (hasObjectStoreCatalogOption || hasRestCatalogOption) block — once, before either downstream GetRestCatalogOptionsFromCatalog call site is reached. Guard with !IsCatalogOwnedByExtension(catalogOptionValue) so the reserved built-in sentinels (postgres/object_store/rest) are skipped:

if (hasRestCatalogOption)
{
    char *catalogOptionValue = GetStringOption(createStmt->options, "catalog", false);

    if (!IsCatalogOwnedByExtension(catalogOptionValue))
    {
        ForeignServer *server = GetForeignServerByName(catalogOptionValue, false);
        AclResult aclresult = object_aclcheck(ForeignServerRelationId,
                                              server->serverid,
                                              GetUserId(), ACL_USAGE);
        if (aclresult != ACLCHECK_OK)
            aclcheck_error(aclresult, OBJECT_FOREIGN_SERVER, server->servername);
    }
}

One check covers both the read-only REST path (line ~770) and the writable REST path (line ~809); we only want to do it once at DDL time, not in every query-time caller of GetRestCatalogOptionsFromCatalog, matching core's "CREATE-time only" semantic.

Definitely worth adding a test

[sfc-gh-npuka]

This seems to be broken, can you if Postgres has a different way of handling it?

alter system set pg_lake_iceberg.default_catalog to 'my_open_catalog_2';
ALTER SYSTEM
Time: 4.330 ms
postgres=# select pg_reload_conf();
 pg_reload_conf 
----------------
 t
(1 row)

Time: 1.879 ms
postgres=# show pg_lake_iceberg.default_catalog;
 pg_lake_iceberg.default_catalog 
---------------------------------
 postgres
(1 row)

Time: 1.216 ms

@sfc-gh-okalaci Very good catch. Currently, any user-created server name will fail if not in a transaction state because the syscache lookup requires a transaction.

	if (IsTransactionState() && IsRestCatalog(newCatalog))
		return true;

I will follow Postgres behavior on this https://github.com/postgres/postgres/blob/master/src/backend/commands/tablespace.c#L1099-L1138
Which means, accept the value optimistically when not in a transaction state, and defer real validation to first use. I.e. convert to:

if (!IsTransactionState())
    return true;
if (IsRestCatalog(newCatalog))
    return true;