Hi all, I have a PR open that relaxes `gc.enabled` for metadata-only snapshot expiry, and review feedback has convinced me the underlying problem is that we've never defined what `gc.enabled` means in full. I'd like to try and settle the definition on-list before the behaviour change goes anywhere.
https://github.com/apache/iceberg/pull/17791 The property is not in the spec. The only normative-ish text is one line in docs/docs/configuration.md: > gc.enabled | true | Allows garbage collection operations such as > expiring snapshots and removing orphan files That wording arguably supports the strictest reading ("expiry is a GC operation, so it is blocked"), which is why I don't think of this as a bug fix -- it's a semantic decision the community should make. Java implements at least three different meanings today: 1. `CatalogUtil.dropTableData` proceeds when `gc.enabled=false`: it skips data files but still deletes manifests, manifest lists, statistics, previous metadata and the metadata file. No error. That is exactly the `METADATA_ONLY` semantic we just added to `ExpireSnapshots.CleanupLevel`. 2. `RemoveSnapshots` (before this PR) hard-failed in its constructor, even for an expiry configured to delete nothing at all. 3. `SparkCatalog.purgeTable`, `ExpireSnapshotsSparkAction`, `DeleteOrphanFilesSparkAction` and `DeleteReachableFilesSparkAction` all hard-fail on construction. 4. `HMSTablePropertyHelper` maps `gc.enabled` to Hive's `external.table.purge`, i.e. treats it as a drop-purge flag. Its own javadoc already concedes the two flags can disagree and produce surprising behaviour. So within one repository the same flag means "skip data files, delete metadata", "refuse the whole operation", and "control DROP TABLE purge". The clients have inherited the ambiguity, not resolved it: - iceberg-rust rejects expiry at commit time in crates/iceberg/src/transaction/expire_snapshots.rs, with the comment "Expiring metadata defeats a user's explicit decision to disable GC (Java refuses too)" -- citing Java as the authority. Meanwhile table_properties.rs documents the same key as "enabling garbage collection on drop... data files will not be deleted when a table is dropped". Two definitions in one crate. - iceberg-go documents it as "Snapshot expiration is rejected when false, and data-file cleanup skips deletion." - iceberg-cpp rejects expiry in the `ExpireSnapshots` constructor, with the identical error string to Java's pre-PR check ("Cannot expire snapshots: GC is disabled (deleting files may corrupt other tables)") in the same construction-time position. It is a direct port of the behaviour this PR removes. src/iceberg/update/expire_snapshots.cc - iceberg-python has a full `expire_snapshots` implementation and no `gc.enabled` check that I can find under any spelling, so it already permits what Rust, Go and C++ reject. So two clients have hard-coded Java's current behaviour -- one citing Java in a comment, the other copying its error message verbatim -- while a third ignores the property for this operation entirely. Whichever definition we pick, the worst outcome for those implementations is Java changing quietly, which is why I'd rather settle it here than in a Java PR. Proposal. Define `gc.enabled=false` as: > Physical file deletion is disabled for this table. No client may > delete a file from the table's storage, whether or not the table's > metadata still references it. Operations that only rewrite table > metadata are unaffected. Consequences: - Snapshot expiry that deletes no files is permitted; expiry that would delete files is rejected. - `remove_orphan_files` and drop-with-purge stay blocked, as today. - `dropTableData` currently proceeds under `gc.enabled=false` and deletes manifests, manifest lists, statistics and metadata files, skipping only data files. Under this definition that becomes non-conformant. As far as I can tell nothing in-tree depends on it -- Nessie ignores `purge` entirely -- but it is a behaviour change well outside the scope of the PR above and would need to be its own discussion. - The rule is stated once, in the spec's table-property appendix, so clients implement one thing. The alternative, raised by Amogh in review and worth taking seriously: keep `gc.enabled` as a blunt "no snapshot removal at all" switch, on the grounds that catalogs and platform operators may already rely on it that way, and introduce a separate, finer-grained property for cleanup control. I lean against adding a property when `CleanupLevel` already expresses the granularity, but I don't think the concern about existing opinionated deployments is wrong. Questions to answer: 1. Which definition do we want -- "no physical deletion" or "no snapshot removal"? 2. Should this land in the spec (table-property appendix) or stay documentation-only? 3. Error or silent skip when a cleanup operation is blocked? Java currently does both. 4. Should the strict reading extend to metadata files on drop, making `dropTableData` skip manifests and metadata too, or do we grandfather its current behaviour? 5. Does anything need saying about REST catalogs, which can reject the remove-snapshot update server-side regardless of what clients do? Happy to write the spec PR and open tracking issues in the Python, Rust, Go and C++ repos once there's a direction. Thanks, Xander
