Fix misleading iceberg catalog option error messages

The FDW validator in pg_lake_iceberg used error messages that said
options were "only valid for writable catalog=\"rest\"" when they are
actually valid for read-only external catalog tables (both catalog=rest
and catalog=object_store). Similarly, "required for catalog=\"rest\""
omitted that the same options are required for catalog=object_store
read-only tables. The error raised for an unknown "catalog" value also
listed only "rest" and "postgres", omitting "object_store".

Changes:
- Reword all "only valid for" / "required for" error messages for
  catalog_name, catalog_namespace, catalog_table_name, and read_only
  to accurately describe which catalog types they apply to.
- Update the "invalid catalog option" errdetail and surrounding block
  comment to include object_store alongside rest and postgres.
- Remove a dead "catalog_namespace required" check: it only applied to
  REST read-only (object_store excluded), but the REST ALTER whitelist
  never allows touching catalog_namespace and CREATE auto-fills a
  default, so the path was unreachable.
- Rework the comments in ProcessAlterTable: the outer block comment
  now describes external catalog handling (was "rest" only) and the
  validator interaction, and each per-branch comment accurately
  describes which options that catalog type allows changing (the
  previous object_store branch comment was a stale copy-paste of the
  rest branch comment).
- Add a comment in option.c noting that the remaining "required"
  checks act as a safety net for ALTER DROP scenarios.

Tests:
- test_iceberg_catalog_option_validation_errors (test_writable_iceberg.py):
  exercises the four "only valid for read-only external catalog tables"
  rejection paths plus the "invalid catalog option" path without
  needing external infrastructure.
- test_object_store_read_only_alter_drop_required_options
  (test_object_store_catalog.py): exercises the two "required" paths
  via ALTER FOREIGN TABLE OPTIONS (DROP ...) on an object_store
  read-only table.

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: sfc-gh-npuka <naisila.puka@snowflake.com>

Author: sfc-gh-npuka

pg_lake_table/src/ddl/alter_table.c

@@ -248,9 +248,11 @@ ProcessAlterTable(ProcessUtilityParams * processUtilityParams, void *arg)
 	}
 
 	/*
-	 * For read-only rest catalog iceberg tables, we only allow changing
-	 * catalog_table_name option. All other operations or any other DDLs are
-	 * disallowed.
+	 * For read-only external catalog iceberg tables, we only allow changing a
+	 * subset of catalog options. All other operations are disallowed. After
+	 * the whitelist check passes, we return false so PostgreSQL processes the
+	 * ALTER normally (which invokes the FDW validator on the merged option
+	 * set).
 	 */
 	if (HasOnlyCatalogAlterTableOptions(alterStmt))
 	{
@@ -259,8 +261,9 @@ ProcessAlterTable(ProcessUtilityParams * processUtilityParams, void *arg)
 		if (icebergCatalogType == REST_CATALOG_READ_ONLY)
 		{
 			/*
-			 * We currently only allow changing catalog_table_name option for
-			 * read-only rest catalog iceberg tables.
+			 * For read-only rest catalog iceberg tables, only
+			 * catalog_table_name can be changed. catalog_name and
+			 * catalog_namespace are pinned at creation time.
 			 */
 			List	   *allowedOptions = list_make1("catalog_table_name");
 
@@ -271,8 +274,10 @@ ProcessAlterTable(ProcessUtilityParams * processUtilityParams, void *arg)
 		else if (icebergCatalogType == OBJECT_STORE_READ_ONLY)
 		{
 			/*
-			 * We currently only allow changing catalog_table_name option for
-			 * read-only rest catalog iceberg tables.
+			 * For read-only object_store catalog iceberg tables, the full
+			 * catalog identifier (catalog_name + catalog_namespace +
+			 * catalog_table_name) can be changed to re-point the table at a
+			 * different source.
 			 */
 			List	   *allowedOptions = list_make3("catalog_table_name", "catalog_namespace", "catalog_name");
 

pg_lake_table/src/fdw/option.c

@@ -765,8 +765,9 @@ pg_lake_iceberg_validator(PG_FUNCTION_ARGS)
 			char	   *icebergCatalogName = defGetString(def);
 
 			/*
-			 * We only accept "rest" and "postgres" for now. If not provided,
-			 * assume "postgres" by default. Don't allow anything.
+			 * We only accept "rest", "object_store" and "postgres". If not
+			 * provided, the postgres catalog is assumed by default (see
+			 * ProcessCreateIcebergTableFromForeignTableStmt).
 			 */
 			if (pg_strncasecmp(icebergCatalogName, REST_CATALOG_NAME, strlen(icebergCatalogName)) == 0)
 			{
@@ -793,7 +794,7 @@ pg_lake_iceberg_validator(PG_FUNCTION_ARGS)
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 						 errmsg("invalid catalog option: %s", icebergCatalogName),
-						 errdetail("Only " REST_CATALOG_NAME " and " POSTGRES_CATALOG_NAME " are supported for now.")));
+						 errdetail("Only " REST_CATALOG_NAME ", " OBJECT_STORE_CATALOG_NAME " and " POSTGRES_CATALOG_NAME " are supported.")));
 		}
 		else if (catalog == ForeignTableRelationId && strcmp(def->defname, "read_only") == 0)
 		{
@@ -908,30 +909,30 @@ pg_lake_iceberg_validator(PG_FUNCTION_ARGS)
 		!(icebergCatalogType == REST_CATALOG_READ_ONLY || icebergCatalogType == OBJECT_STORE_READ_ONLY))
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("\"read_only\" option is only valid for catalog=\"rest\"")));
+				 errmsg("\"read_only\" option is only valid for catalog=\"rest\" or catalog=\"object_store\"")));
 
 	if (catalog == ForeignTableRelationId)
 	{
 		if (icebergCatalogType == REST_CATALOG_READ_ONLY || icebergCatalogType == OBJECT_STORE_READ_ONLY)
 		{
 			/*
-			 * catalog_namespace, catalog_table_name and catalog_name is
-			 * required for catalog=rest
+			 * catalog_name and catalog_table_name are required for read-only
+			 * external catalog tables, i.e. catalog=rest and
+			 * catalog=object_store
+			 *
+			 * On CREATE, ProcessCreateIcebergTableFromForeignTableStmt
+			 * auto-fills defaults for these options, so these checks act as a
+			 * safety net for ALTER DROP scenarios.
 			 */
 			if (!catalogName)
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("\"catalog_name\" option is required for catalog=\"rest\"")));
-
-			if (!catalogNamespace && icebergCatalogType != OBJECT_STORE_READ_ONLY)
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("\"catalog_namespace\" option is required for catalog=\"rest\"")));
+						 errmsg("\"catalog_name\" option is required for read-only external catalog tables")));
 
 			if (!catalogTableName)
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("\"catalog_table_name\" option is required for catalog=\"rest\"")));
+						 errmsg("\"catalog_table_name\" option is required for read-only external catalog tables")));
 		}
 		else
 		{
@@ -941,17 +942,17 @@ pg_lake_iceberg_validator(PG_FUNCTION_ARGS)
 			if (catalogNamespace)
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("\"catalog_namespace\" option is only valid for writable catalog=\"rest\"")));
+						 errmsg("\"catalog_namespace\" option is only valid for read-only external catalog tables")));
 
 			if (catalogTableName)
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("\"catalog_table_name\" option is only valid for writable catalog=\"rest\"")));
+						 errmsg("\"catalog_table_name\" option is only valid for read-only external catalog tables")));
 
 			if (catalogName)
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg("\"catalog_name\" option is only valid for writable catalog=\"rest\"")));
+						 errmsg("\"catalog_name\" option is only valid for read-only external catalog tables")));
 
 		}
 	}

pg_lake_table/tests/pytests/test_object_store_catalog.py

@@ -127,6 +127,53 @@ def test_read_only_object_store_with_non_existing_options(
     pg_conn.commit()
 
 
+def test_object_store_read_only_alter_drop_required_options(
+    pg_conn, s3, extension, with_default_location, adjust_object_store_settings
+):
+    schema = "test_alter_drop_required"
+    run_command(f"CREATE SCHEMA {schema}", pg_conn)
+
+    run_command(
+        f"CREATE TABLE {schema}.wrt_tbl(a INT) USING iceberg WITH (catalog='object_store')",
+        pg_conn,
+    )
+    pg_conn.commit()
+    wait_until_object_store_writable_table_pushed(pg_conn, schema, "wrt_tbl")
+
+    run_command(
+        f"CREATE TABLE {schema}.ro_tbl() USING iceberg WITH (catalog='object_store', read_only=True, catalog_table_name='wrt_tbl')",
+        pg_conn,
+    )
+    pg_conn.commit()
+
+    # dropping catalog_name should fail — it is required
+    error = run_command(
+        f"ALTER FOREIGN TABLE {schema}.ro_tbl OPTIONS (DROP catalog_name)",
+        pg_conn,
+        raise_error=False,
+    )
+    assert (
+        '"catalog_name" option is required for read-only external catalog tables'
+        in str(error)
+    )
+    pg_conn.rollback()
+
+    # dropping catalog_table_name should fail — it is required
+    error = run_command(
+        f"ALTER FOREIGN TABLE {schema}.ro_tbl OPTIONS (DROP catalog_table_name)",
+        pg_conn,
+        raise_error=False,
+    )
+    assert (
+        '"catalog_table_name" option is required for read-only external catalog tables'
+        in str(error)
+    )
+    pg_conn.rollback()
+
+    run_command(f"DROP SCHEMA {schema} CASCADE", pg_conn)
+    pg_conn.commit()
+
+
 # basic flow for object store catalog tables
 # changes on the source is reflected on the target
 def test_read_only_object_store_read_write(

pg_lake_table/tests/pytests/test_writable_iceberg.py

@@ -120,6 +120,78 @@ def test_writable_iceberg_create_wrong_option(pg_conn, extension):
     pg_conn.commit()
 
 
+def test_iceberg_catalog_option_validation_errors(pg_conn, extension):
+    schema = "test_catalog_option_validation"
+    run_command(f"CREATE SCHEMA {schema}", pg_conn)
+    pg_conn.commit()
+
+    # only rest, object_store and postgres are accepted as catalog values
+    error = run_command(
+        f"""CREATE FOREIGN TABLE {schema}.ft (id int) SERVER pg_lake_iceberg
+            OPTIONS (location 's3://bucket/path', catalog 'bogus')""",
+        pg_conn,
+        raise_error=False,
+    )
+    assert "invalid catalog option: bogus" in str(error)
+    assert "Only rest, object_store and postgres are supported" in str(error)
+    pg_conn.rollback()
+
+    # read_only is only valid for catalog="rest" or catalog="object_store"
+    error = run_command(
+        f"""CREATE FOREIGN TABLE {schema}.ft (id int) SERVER pg_lake_iceberg
+            OPTIONS (location 's3://bucket/path', read_only 'true')""",
+        pg_conn,
+        raise_error=False,
+    )
+    assert (
+        '"read_only" option is only valid for catalog="rest" or catalog="object_store"'
+        in str(error)
+    )
+    pg_conn.rollback()
+
+    # catalog_name is only valid for read-only external catalog tables
+    error = run_command(
+        f"""CREATE FOREIGN TABLE {schema}.ft (id int) SERVER pg_lake_iceberg
+            OPTIONS (location 's3://bucket/path', catalog_name 'cat')""",
+        pg_conn,
+        raise_error=False,
+    )
+    assert (
+        '"catalog_name" option is only valid for read-only external catalog tables'
+        in str(error)
+    )
+    pg_conn.rollback()
+
+    # catalog_namespace is only valid for read-only external catalog tables
+    error = run_command(
+        f"""CREATE FOREIGN TABLE {schema}.ft (id int) SERVER pg_lake_iceberg
+            OPTIONS (location 's3://bucket/path', catalog_namespace 'ns')""",
+        pg_conn,
+        raise_error=False,
+    )
+    assert (
+        '"catalog_namespace" option is only valid for read-only external catalog tables'
+        in str(error)
+    )
+    pg_conn.rollback()
+
+    # catalog_table_name is only valid for read-only external catalog tables
+    error = run_command(
+        f"""CREATE FOREIGN TABLE {schema}.ft (id int) SERVER pg_lake_iceberg
+            OPTIONS (location 's3://bucket/path', catalog_table_name 'tbl')""",
+        pg_conn,
+        raise_error=False,
+    )
+    assert (
+        '"catalog_table_name" option is only valid for read-only external catalog tables'
+        in str(error)
+    )
+    pg_conn.rollback()
+
+    run_command(f"DROP SCHEMA {schema} CASCADE", pg_conn)
+    pg_conn.commit()
+
+
 def test_writable_iceberg_table_failure(
     installcheck,
     install_iceberg_to_duckdb,