Utils to differentiate internal-external and writable-readonly tables #73

sfc-gh-abozkurt · 2025-11-20T10:49:55Z

Preparation for #70.

sfc-gh-abozkurt · 2025-11-20T10:52:24Z

pg_lake_engine/include/pg_lake/pgduck/remote_storage.h


 extern PGDLLEXPORT int64 GetRemoteFileSize(char *path);
 extern PGDLLEXPORT int64 GetRemoteParquetFileRowCount(char *path);
-extern PGDLLEXPORT List *GetRemoteParquetColumnStats(char *path, List *leafFields);


redundant declaration

sfc-gh-abozkurt · 2025-11-20T10:52:35Z

pg_lake_engine/src/utils/rel_utils.c

+* GetForeignTablePath - get the path option for the foreign table.
+*/
+char *
+GetForeignTablePath(Oid foreignTableId)


moved from other place

sfc-gh-abozkurt · 2025-11-20T10:53:05Z

pg_lake_iceberg/src/iceberg/catalog.c

+
+
+IcebergCatalogType
+GetIcebergCatalogType(Oid relationId)


moved from other place

sfc-gh-okalaci

I was a bit undecided whether we should add these functions or not. My rationale was would that cause any confusion in the code.

But then, seeing this in action, I think I like this better, we already have these concepts in the code, and this makes them even more explicit & better documented

sfc-gh-okalaci · 2025-11-20T11:24:28Z

pg_lake_iceberg/src/iceberg/catalog.c

+	{
+		currentMetadataPath = GetMetadataLocationFromExternalObjectStoreCatalogForTable(relationId);
+	}
+	else


I think we should be more strict about this function, we shouldn't use it unless the table is an external iceberg table:

char * GetIcebergExternalMetadataLocation(Oid relationId) { IcebergCatalogType icebergCatalogType = GetIcebergCatalogType(relationId); if (icebergCatalogType == REST_CATALOG_READ_ONLY) { return GetMetadataLocationForRestCatalogForIcebergTable(relationId); } else if (icebergCatalogType == OBJECT_STORE_READ_ONLY) { return GetMetadataLocationFromExternalObjectStoreCatalogForTable(relationId); } else if (icebergCatalogType == NOT_ICEBERG_TABLE) { FindDataFormatAndCompression(tableType, path, options, &sourceFormat, &sourceCompression); if (sourceFormat == DATA_FORMAT_ICEBERG) return GetForeignTablePath(relationId);; } ereport(ERROR, "expected ... not found.."); }

Not sure, we might want to have another function on top that can get metadata location for any table type. I needed that recently. Maybe better to return NULL and let callers decide.

I still think we should check for FindDataFormatAndCompression and sourceFormat == DATA_FORMAT_ICEBERG. If not, we could return NULL instead of err, that's fine. In the worst case, that's useful for readability.

we have Assert(IsAnyExternalIcebergTable(relationId)); But can expand with format checks as well

I think format check is still useful for two things: (a) make it a lot more clearer for the readers of the code on what class of tables this is (b) in the future, we might continue adding new table types -- even iceberg types -- so prevents any assumptions on that.

Not sure, we might want to have another function on top that can get metadata location for any table type. I needed that recently. Maybe better to return NULL and let callers decide.

I'm still in favor of throwing error instead of returning NULL. There are like ~15 callers of this function, and otherwise we'd have to add if not null checks to all the callers.

If we need a caller which prefers NULL, then we could add errorIfMissing flag?

We now handle all external iceberg table cases properly, and throw error for any other table (considered misuse).

sfc-gh-okalaci · 2025-11-20T13:26:15Z

pg_lake_table/src/fdw/schema_operations/register_field_ids.c

-	{
-		currentMetadataPath = GetIcebergCatalogMetadataLocation(relationId, true);
-	}
+	char	   *currentMetadataPath = GetIcebergExternalMetadataLocation(relationId);


note that this is wrong. GetIcebergCatalogMetadataLocation is equivalent to Internal iceberg table. So, we should

yeah that was typo which caused the test failure :)

pg_lake_iceberg/src/iceberg/catalog.c

Given many changes happened, let me review again before merging

sfc-gh-abozkurt · 2025-11-20T14:53:42Z

pg_lake_table/src/ddl/vacuum.c

 		return;
 	}

+	if (!IsAnyInternalIcebergTable(relationId))


this was missing, we should only register missing fields for internal iceberg tables.

pg_lake_iceberg/src/iceberg/catalog.c

sfc-gh-okalaci · 2025-11-20T16:01:14Z

pg_lake_engine/src/utils/rel_utils.c

+
+
+/*
+ * IsAnyInternalIcebergTable - check if the table is an internal iceberg table (written by us).


hm, do we really need Any in these function names? I'd be fine with IsInternalIcebergTable etc

sfc-gh-okalaci · 2025-11-20T16:14:19Z

pg_lake_table/src/fdw/schema_operations/register_field_ids.c

 List *
 CreatePostgresColumnMappingsForIcebergTableFromExternalMetadata(Oid relationId)
 {
-	char	   *currentMetadataPath = NULL;


I think in an ideal world, we should add an assert IsAnyExternalIcebergTable(relationId) for being consistent with the function name & usage. But, that'd break VacuumRegisterMissingFields. VacuumRegisterMissingFields is really a special case, where we are dealing with an internal iceberg table, but we access its metadata.json for schema purposes.

So, I don't have a good suggestions here but if you can come up with something better, please do so,

I think no assert is fine. We get metadata location for any table, and operate on the metadata.json.

sfc-gh-okalaci · 2025-11-21T14:14:54Z

pg_lake_engine/src/utils/table_type.c

+	DefElem    *writableOption = GetOption(options, "writable");
+	PgLakeTableType tableType = GetPgLakeTableType(foreignTableId);
+
+	return tableType == PG_LAKE_ICEBERG_TABLE_TYPE ||


hmm, it could be tableType == PG_LAKE_ICEBERG_TABLE_TYPE but then also REST_CATALOG_READ_ONLY? Should we add tableType == PG_LAKE_ICEBERG_TABLE_TYPE && IsInternalIcebergTable || ...?

good catch, not related to this PR but needs the change because we also have writable object_store and rest catalog tables.

sfc-gh-okalaci

few questions/notes

sfc-gh-okalaci · 2025-11-24T05:36:23Z

pg_lake_copy/src/copy/copy.c

 	if (PgLakeCopyValidityCheckHook)
 		PgLakeCopyValidityCheckHook(relationId);

-	ErrorIfReadOnlyIcebergTable(relationId);


how can we remove this line any everything continues to work?

We use correct check at postgresIsForeignRelUpdatable in pg_lake_table.c.

Do we call that for pushdownable COPY cases (Iceberg from URL)?

uh sorry for confusion, we do not push down such cases at IsCopyFromPushdownable by correct checks. Afterwards, we hit into postgresIsForeignRelUpdatable as fallback.

sfc-gh-okalaci · 2025-11-24T05:44:06Z

pg_lake_table/src/fdw/schema_operations/register_field_ids.c

-		default:
-			return NULL;
+		Assert(IsExternalIcebergTable(relationId));
+		return GetDataFileSchemaForExternalIcebergTable(path);


what if path is NULL? We are overlooking some of the existing cases here?

Same applies to GetLeafFieldsForTable

It cannot be as GetIcebergMetadataLocation throws error in such cases. (same as before)

GetIcebergMetadataLocation can be NULL after #68 for writable rest tables.

sfc-gh-okalaci · 2025-11-24T05:58:04Z

pg_lake_iceberg/src/iceberg/catalog.c

-* throws an error if it is.
-*/
-void
-ErrorIfReadOnlyIcebergTable(Oid relationId)


I cannot quite follow why we remove these functions? It was nicely documented functions, all in a single place.

Now, we are scattering this logic around, having multiple copies, and the benefit is minor improvement in the error messages?

I think if you want to change anything about this ErrorIf or Warn.. functions that they should definitely be a separate PR.

This PR has become a looooot larger than I thought, and this part seems arbitrarily added to the PR.

Can you please get these functions back?

We do not need those in any places that will hit into table's foreign data wrapper. Instead, we hit into error via postgresIsForeignRelUpdatable check. This makes the things easier to maintain. Alter ddl hooks and truncate are the only 2 places that we need to add such errors because we did not hit into postgresIsForeignRelUpdatable check for those.

sfc-gh-abozkurt · 2025-11-24T21:01:07Z

pg_lake_table/src/ddl/vacuum.c

 		return;
 	}

+	if (!IsInternalIcebergTable(relationId))


vacuum should only register field for internal tables (that are in the internal tables catalog).

sfc-gh-okalaci

Thanks, cleans up the utils that have been incrementally added, gets them into a nicer shape and less bug/error-prone.

sfc-gh-okalaci · 2025-11-25T15:30:17Z

pg_lake_table/src/fdw/schema_operations/register_field_ids.c

+	if (!IsIcebergTable(relationId))
+		return NULL;
+
+	char	   *path = GetIcebergMetadataLocation(relationId, false);


nit: GetIcebergMetadataLocation can go into else part

sfc-gh-okalaci · 2025-11-25T15:32:01Z

pg_lake_table/src/fdw/schema_operations/register_field_ids.c

-		default:
-			return NULL;
+		Assert(IsExternalIcebergTable(relationId));
+		return GetDataFileSchemaForExternalIcebergTable(path);


GetIcebergMetadataLocation can be NULL after #68 for writable rest tables.

sfc-gh-okalaci · 2025-11-25T15:35:35Z

pg_lake_table/src/fdw/data_file_pruning.c

-		if (EnableHeavyAsserts && tableType == PG_LAKE_ICEBERG_TABLE_TYPE &&
-			!(GetIcebergCatalogType(relationId) == REST_CATALOG_READ_ONLY ||
-			  GetIcebergCatalogType(relationId) == OBJECT_STORE_READ_ONLY))
+		if (EnableHeavyAsserts && IsInternalIcebergTable(relationId))


soon with #68 we'd end up undoing this change, we cannot do heavy asserts for writable-rest at this point.

Should we keep here as is maybe?

lets handle and comment about it in #68 to be consistent in this PR.

sfc-gh-okalaci · 2025-11-25T15:38:00Z

pg_lake_table/src/fdw/snapshot.c

-			 * We cannot afford to have a different schema between the
-			 * Postgres catalogs and the iceberg catalog.
-			 */
+		if (catalogType == REST_CATALOG_READ_ONLY ||


Marco once asked why do we treat DATA_FORMAT_ICEBERG differently for ErrorIfSchemasDoNotMatch. We could enforce the same for DATA_FORMAT_ICEBERG

Though, let's not do it in this PR, mostly sharing some context.

yeah, I also though like that but schema check had failure for some of our tests. We need to dig into it in another PR.

Signed-off-by: Aykut Bozkurt <[email protected]>

sfc-gh-abozkurt force-pushed the aykut/refactor-internal-external branch from f6b7a6d to 126579e Compare November 20, 2025 10:51

sfc-gh-abozkurt requested a review from sfc-gh-okalaci November 20, 2025 10:51

sfc-gh-abozkurt commented Nov 20, 2025

View reviewed changes

sfc-gh-abozkurt force-pushed the aykut/refactor-internal-external branch from 126579e to d447e61 Compare November 20, 2025 10:55

sfc-gh-okalaci previously approved these changes Nov 20, 2025

View reviewed changes

sfc-gh-abozkurt force-pushed the aykut/refactor-internal-external branch 2 times, most recently from e42b446 to 7f03451 Compare November 20, 2025 13:26

sfc-gh-okalaci reviewed Nov 20, 2025

View reviewed changes

pg_lake_iceberg/src/iceberg/catalog.c Show resolved Hide resolved

sfc-gh-abozkurt force-pushed the aykut/refactor-internal-external branch 3 times, most recently from a33ede2 to 407d6b6 Compare November 20, 2025 14:51

sfc-gh-abozkurt commented Nov 20, 2025

View reviewed changes

sfc-gh-okalaci reviewed Nov 20, 2025

View reviewed changes

pg_lake_iceberg/src/iceberg/catalog.c Show resolved Hide resolved

sfc-gh-okalaci reviewed Nov 20, 2025

View reviewed changes

sfc-gh-abozkurt force-pushed the aykut/refactor-internal-external branch from 407d6b6 to 90f7ecc Compare November 21, 2025 03:07

sfc-gh-abozkurt requested a review from sfc-gh-okalaci November 21, 2025 03:14

sfc-gh-abozkurt force-pushed the aykut/refactor-internal-external branch from 90f7ecc to 8b40701 Compare November 21, 2025 09:48

sfc-gh-okalaci approved these changes Nov 21, 2025

View reviewed changes

sfc-gh-abozkurt force-pushed the aykut/refactor-internal-external branch 2 times, most recently from 22c7548 to 8b9897a Compare November 22, 2025 16:59

sfc-gh-abozkurt changed the title ~~Utils to differentiate internal and external tables~~ Utils to differentiate internal-external and writable-readonly tables Nov 22, 2025

sfc-gh-okalaci requested changes Nov 24, 2025

View reviewed changes

sfc-gh-abozkurt force-pushed the aykut/refactor-internal-external branch 2 times, most recently from 1b05eb3 to e3858c7 Compare November 24, 2025 20:37

sfc-gh-abozkurt commented Nov 24, 2025

View reviewed changes

sfc-gh-okalaci approved these changes Nov 25, 2025

View reviewed changes

Refactor table types

0d01cfc

Signed-off-by: Aykut Bozkurt <[email protected]>

sfc-gh-abozkurt force-pushed the aykut/refactor-internal-external branch from e3858c7 to 0d01cfc Compare November 25, 2025 20:37

sfc-gh-abozkurt merged commit 9590c14 into main Nov 25, 2025
63 checks passed

sfc-gh-abozkurt deleted the aykut/refactor-internal-external branch November 25, 2025 20:51

sfc-gh-okalaci mentioned this pull request Dec 1, 2025

Writable Iceberg tables with REST catalog support #68

Merged

22 tasks



		/*
		* IsAnyInternalIcebergTable - check if the table is an internal iceberg table (written by us).

Utils to differentiate internal-external and writable-readonly tables #73

Utils to differentiate internal-external and writable-readonly tables #73

Uh oh!

Conversation

sfc-gh-abozkurt commented Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-okalaci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-okalaci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-abozkurt Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-okalaci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-abozkurt commented Nov 20, 2025 •

edited

Loading

sfc-gh-abozkurt Nov 24, 2025 •

edited

Loading