Skip to content

Fixes for Iceberg Puffin Delete Vectors - #2715

Open
ianton-ru wants to merge 8 commits into
antalya-26.8from
bugfix/antalya-26.8/puffin-dv-audit-fixes
Open

ianton-ru wants to merge 8 commits into
antalya-26.8from
bugfix/antalya-26.8/puffin-dv-audit-fixes

Conversation

@ianton-ru

Copy link
Copy Markdown

Changelog category (leave one):

  • Bug Fix

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Iceberg Puffin deletion-vector cache reads are pinned to the listed etag, including when s3_validate_etag_on_read or azure_validate_etag_on_read is off. A changed object fails the query instead of being applied under the listed offset, length, and cardinality. Materialization stays within PUFFIN_DV_MAX_MATERIALIZED_POSITIONS when cardinality is omitted. PuffinFilesCache clear, setMaxSizeInBytes, and setMaxCount update the footer memo through CacheBase. Cluster reads keep an output-only schema_transform. Archive and queue objects keep their type in ObjectInfo::clone.

Documentation entry for user-facing changes

No new setting or syntax. With the Puffin files cache enabled, a deletion-vector read is tied to the etag from the listing. S3 and Azure send that etag as If-Match even when etag validation is disabled. Local, HDFS, and Web compare the etag after the read. A mismatch raises an error and is not stored in the cache. An empty or weak etag is not used as a cache key. A deletion vector is rejected when it would materialize more than 100000000 positions, whether or not a cardinality was declared. SYSTEM DROP ... cache clear and a runtime cache resize drop memoized Puffin footers together with cached deletion vectors. A distributed read keeps a schema evolution transform that only adds columns. Bucket splitting no longer drops the archive path or the queue file metadata.

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Unit tests
  • Performance tests
  • Aarch64 tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • CAS (content-addressed storage; Antalya only)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • OAuth (5m)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

Title: Pin Puffin deletion-vector reads and preserve cluster schema and object identity

Motivation

A Puffin deletion-vector cache miss recorded the object etag and then read the bytes without requiring that etag when s3_validate_etag_on_read or azure_validate_etag_on_read was off. An in-place overwrite could be stored under the old key. Local, HDFS, and Web ignore StoredObject::etag, so the same race existed there even with validation enabled. A first version of the pin retried the read without the etag, which could apply the new object under the listed offset, length, and cardinality.

Two related limits were incomplete. Omitting expected_cardinality skipped PUFFIN_DV_MAX_MATERIALIZED_POSITIONS and let a compact roaring bitmap expand without a ceiling. PuffinFilesCache::clear, setMaxSizeInBytes, and setMaxCount hid the non-virtual CacheBase methods, so a base pointer left the footer memo resident.

On the cluster read path, a schema_transform that only adds columns has outputs and no inputs. Deserialize treated that the same as the empty DAG used when there is no transform, and the old protocol did not reject it. Bucket splitting copies each object through ObjectInfo::clone. IcebergDataObjectInfo already copied itself. ObjectInfoInArchive and ObjectStorageQueueObjectInfo fell back to the base copy and lost the archive path, reader, and file info, or the queue file_metadata.

Approach

The cache-miss load passes the listed etag into createReadBuffer even when the validation settings are off. S3 and Azure send it as If-Match. Local, HDFS, and Web compare the etag after the bytes are read. A mismatch throws. getOrSetDeletionVector does not insert when the loader throws, and the query fails instead of reading the current object under the old identity. An empty or weak etag is not a cache key.

deserializeRoaringPositionBitmap rejects a bitmap whose cardinality would pass PUFFIN_DV_MAX_MATERIALIZED_POSITIONS before expanding it, including when no cardinality was declared. A fully empty DAG is still the on-wire stand-in for a missing schema_transform. Any DAG with inputs or outputs is kept, and an old worker protocol rejects it. CacheBase::clear, setMaxCount, and setMaxSizeInBytes are virtual, and PuffinFilesCache overrides them so the footer memo follows the same limits. ObjectInfoInArchive and ObjectStorageQueueObjectInfo implement clone.

Tests

CreateReadBufferCanPinListedEtagWhenValidationIsOff checks that If-Match is sent only when the pin is requested. PuffinDeletionVectorCardinality.RejectsOmittedCardinalityAboveMaterializationLimit builds a run-length bitmap of 100000001 positions and expects a rejection before expansion. PuffinFooterMemo.BasePointerUpdatesFooterMemo clears and shrinks the footer memo through a CacheBase reference. ClusterFunctionReadTaskResponse.RoundTripsOutputOnlySchemaTransformOnSupportedProtocol keeps an added-column transform, and the old protocol rejects it. ObjectInfoClone checks that an archive entry and a queue entry stay their own types after clone.

ianton-ru and others added 6 commits October 27, 2026 14:13
A cache miss records the object etag and then reads the bytes. The GET now requires that etag, including when `s3_validate_etag_on_read` or `azure_validate_etag_on_read` is off, so an in-place overwrite is not stored under the old key. A weak etag is not used as a cache key.

Co-authored-by: Cursor <cursoragent@cursor.com>
Omitting `expected_cardinality` skipped `PUFFIN_DV_MAX_MATERIALIZED_POSITIONS`. `deserializeRoaringPositionBitmap` now rejects a bitmap above that ceiling before expanding it into positions.

Co-authored-by: Cursor <cursoragent@cursor.com>
`PuffinFilesCache` hid non-virtual `clear`, `setMaxSizeInBytes`, and `setMaxCount`, so a `CacheBase` pointer left the footer memo in place. Those methods are now virtual.

Co-authored-by: Cursor <cursoragent@cursor.com>
A DAG that only adds columns has no inputs. The cluster protocol treated that as a missing `schema_transform` and dropped it. An empty DAG remains the sentinel for no transform.

Co-authored-by: Cursor <cursoragent@cursor.com>
Bucket splitting copies through `clone`. `ObjectInfoInArchive` and `ObjectStorageQueueObjectInfo` fell back to the base copy and dropped the archive identity and `file_metadata`.

Co-authored-by: Cursor <cursoragent@cursor.com>
A mismatch used to retry without the pin and could apply a later object under the listed offset, length, and cardinality. `getOrSetDeletionVector` already skips the insert when the loader throws.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions

github-actions Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Workflow [PR], commit [afa617e]

@ianton-ru ianton-ru changed the title Bugfix/antalya 26.8/puffin dv audit fixes Puffin dv audit fixes Oct 27, 2026
@ianton-ru

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-29T08:47:13.152582Z afa617e Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Failed to set up container
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ianton-ru

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Failed to set up container
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

The debug build treats a missing prototype as an error, so the cluster read-task helper cannot stay at file scope.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ianton-ru

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Failed to set up container
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ianton-ru

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Something went wrong. Try again later by commenting “@codex review”.

Failed to set up container
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ianton-ru

Copy link
Copy Markdown
Author

@blau-ai

@blau-ai

blau-ai commented Oct 28, 2026

Copy link
Copy Markdown
Collaborator

CI triage for afa617e

5 failing checks, 0 caused by this PR. Every one of them fails the same way on other recent PRs into antalya-26.8 that don't touch Puffin, Iceberg DV or the object-storage source. Nothing to fix in this PR. The suites that cover the changed code all pass: Unit tests (asan_ubsan) 23731/0, Fast test, Stateless (amd_debug, parallel), and the Stateless tests with distributed plan / db disk on the selected tests.

Check Failures Verdict Reference
Integration tests (amd_asan_ubsan, db disk, old analyzer, 5/8) 21 × test_storage_iceberg_with_spark/test_export_partition_iceberg.py already failing on base same 21 on #2704 and #2700
Integration tests (… 6/8) 17 × test_database_backup/test.py already failing on base same 17 on #2704
Integration tests (… 7/8) 2 × test_backward_compatibility/test_custom_s3_disk_remote_url_allow_hosts.py already failing on base same 2 on #2704
Regression release oauth 8 scenarios in /oauth/access control, /oauth/groups already failing on base identical list on #2700, #2680
Regression release swarms cluster discovery/check cluster discovery with wrong cluster name, node failure/cpu overload, node failure/cpu overload all swarm nodes already failing on base identical list on #2700, #2680

Details

1. test_export_partition_iceberg.py (all 21 tests): they fail in setup, in create_iceberg_table, before any data is read:

Code: 497. DB::Exception: default: Not enough privileges. To execute this query, it's necessary to have the grant SHOW NAMED COLLECTIONS SECRETS ON s3.

This is an access-control problem with the test's named collection or user config. Your diff doesn't touch it, and #2704 fails with exactly the same message.

2. test_database_backup/test.py (17 tests): the server aborts during a backup to S3, and the whole module goes down at teardown:

<Fatal> : Logical error: 'Chosen multipart upload for an empty file. This must not happen'.

This comes from the S3 write path. The PR only adds a unit test for ReadBufferFromS3 (gtest_readbuffer_s3.cpp) and doesn't touch any S3 write code. #2704 hit the same Fatal on 2026-10-27, before this PR existed. This looks like a real bug on the base branch, probably connected to CAS / backup disks, and someone should own it separately.

3. test_custom_s3_disk_remote_url_allow_hosts.py (2 tests): the server never starts after the upgrade:

Exception: Timed out while waiting for instance `node' with ip address 172.16.5.7 to start.

#2704 fails the same way.

4. Regression oauth / swarms: both runs fail on exactly the same scenarios as the runs for #2700 (certificate backport) and #2680, which don't touch this code. Swarms deserved a closer look because the PR changes ClusterFunctionReadTask. Even so, the failing scenarios (cluster discovery with a wrong cluster name, CPU overload) are the same set that fails on base, and none of the swarms read scenarios regressed.

About the DROPPED jobs (Stateless sequential / s3 / cas storage, Stress, AST fuzzer, BuzzHouse, SQLLogic, Install/Compatibility): they show up as DROPPED with no info. #2704 has the same 7 Stateless jobs dropped, so this is how CI behaves on this branch, not something this PR caused. Just keep in mind that the s3-storage stateless and stress coverage didn't run for this commit.

Recommendation

The PR is fine as far as CI can tell. Merging is safe from a CI point of view, or rerun the failed jobs if you want a cleaner signal. Two of the base-branch failures should get their own tickets: the Chosen multipart upload for an empty file LOGICAL_ERROR in test_database_backup and the SHOW NAMED COLLECTIONS SECRETS grant in test_export_partition_iceberg.

Evidence: praktika result_pr.json for this PR, #2704 and #2700; gh run view --log-failed for the regression jobs (run 40655802387 here, 40359016010 for #2700).

@ianton-ru

Copy link
Copy Markdown
Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Already looking forward to the next diff.

Reviewed commit: afa617e00c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@ianton-ru ianton-ru added antalya port-antalya PRs to be ported to all new Antalya releases antalya-26.8 Session label (releasy session config) labels Oct 29, 2026
@ianton-ru ianton-ru changed the title Puffin dv audit fixes Fixes for Iceberg Puffin Delete Vectors Oct 29, 2026
@ianton-ru
ianton-ru marked this pull request as ready for review October 29, 2026 11:03

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

antalya antalya-26.8 Session label (releasy session config) port-antalya PRs to be ported to all new Antalya releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants