Skip to content

Fix misleading iceberg catalog option error messages #368

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sfc-gh-npuka wants to merge 1 commit into
base: main
Choose a base branch
from
+148 −23
Open

Fix misleading iceberg catalog option error messages #368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions pg_lake_table/src/ddl/alter_table.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
{
Expand All @@ -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");

Expand All @@ -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");

Expand Down
33 changes: 17 additions & 16 deletions pg_lake_table/src/fdw/option.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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)
{
Expand Down Expand Up @@ -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
{
Expand All @@ -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")));

}
}
Expand Down
47 changes: 47 additions & 0 deletions pg_lake_table/tests/pytests/test_object_store_catalog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
72 changes: 72 additions & 0 deletions pg_lake_table/tests/pytests/test_writable_iceberg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading