iframe-proxy

jkartseva · 2026-07-02T01:53:46Z

Related:
https://github.com/ClickHouse/clickhouse-private/issues/57471
https://github.com/ClickHouse/clickhouse-private/issues/62693

Description

BackupImpl::readBackupMetadata parsed the entire .backup manifest into an in-memory Poco::XML DOM tree before walking it. For large incremental backups the manifest can list millions of files, so the DOM tree alone consumes multiple gigabytes of RAM and is a large part of the front-loaded cost of opening a base backup.

This replaces the DOM parse with a streaming Poco::XML::SAXParser driven by a small BackupMetadataHandler: the header elements are applied when <contents> opens, and each <file> is folded into the existing maps as its closing tag is seen, instead of the whole document being retained in a tree. The on-disk format is unchanged — only the way the manifest is read changes. Callback exceptions are captured and rethrown after parsing, because they must not propagate through the underlying expat callbacks.

Validated end-to-end against a server built from this branch: a new SQL round-trip test (full + incremental + archive backup, restored and compared) and a gtest for BackupMetadataHandler covering the base-backup dedup fields, header-before-files ordering, callback-exception capture, and malformed input.

Changelog category (leave one):

Improvement

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

Reduced memory usage when opening a backup (for RESTORE, or as the base of an incremental BACKUP). The .backup metadata is now parsed as a stream instead of being loaded into an in-memory XML document tree, which for large (especially incremental) backups avoids allocating a multi-gigabyte DOM tree.

`BackupImpl::readBackupMetadata` parsed the whole `.backup` manifest into an in-memory `Poco::XML` DOM tree. For large incremental backups the manifest lists millions of files, so the DOM tree alone costs multiple gigabytes. Replace it with a streaming `SAXParser`: header elements are applied when `<contents>` opens, and each `<file>` is folded into the existing maps as its closing tag is seen, instead of being retained in a tree. The on-disk format is unchanged. Callback exceptions are captured and rethrown after parsing, since they must not propagate through the underlying expat callbacks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Backs up a table (full, incremental on top of the full one, and into an archive), restores each into a separate table, and checks the restored data matches. This exercises `readBackupMetadata` across the disk and archive read paths and the base-backup dedup fields, so a mis-parse of the `.backup` manifest would fail the RESTORE or restore wrong data. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Move the streaming SAX `ContentHandler` out of the anonymous namespace of `BackupImpl.cpp` into `Backups/BackupMetadataHandler.{h,cpp}`. The handler was already fully decoupled from `BackupImpl` (it communicates only through the `on_header`/`on_file` callbacks), so this is a pure code move that makes it a named, directly unit-testable unit. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Covers the cases the SQL round-trip test cannot easily construct: header and per-file field extraction, header-applied-before-any-file ordering, empty `<contents>`, per-file map reset, capture-and-short-circuit of callback exceptions (which must not propagate through the expat parser), and a malformed manifest reported by the parser itself. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

clickhouse-gh · 2026-07-02T01:54:19Z

The Fast test build is compiled without the `minizip` library, so backing up to / restoring from a `.zip` archive fails with `minizip library is disabled` (`SUPPORT_IS_DISABLED`). The archive step differs from the disk path only in where the `ReadBuffer` for `.backup` comes from - the metadata parsing is identical - and archive round-trips are already covered by other tests, so drop it here and keep the full + incremental disk round-trip, which runs everywhere. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109107&sha=0f61bc54919a89fa269b20d968906d5c9fc561d7&name_0=PR&name_1=Fast%20test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

clickhouse-gh · 2026-07-02T02:32:50Z

+    {
+        current_text.clear();
+        const String & name = qname.empty() ? local_name : qname;
+        if (name == "contents")


If the manifest omits <contents>, on_header never fires, parseMemoryNP still returns successfully, and readBackupMetadata leaves version/uuid unset while treating the backup as empty. That means a damaged .backup can slip past open-time validation and only fail much later with a much less obvious error. Please track whether the header/<contents> section was seen and throw BACKUP_DAMAGED after parsing if it was not; a focused gtest for <config>...header...</config> without <contents> would cover the regression.

Claude Fable 5 found this same issue

Copilot

Pull request overview

This PR reduces memory usage when opening backups by replacing .backup metadata parsing in BackupImpl::readBackupMetadata from a Poco::XML DOM parse to a streaming Poco::XML::SAXParser with a dedicated BackupMetadataHandler, avoiding multi-gigabyte DOM allocations for large manifests.

Changes:

Added BackupMetadataHandler (SAX DefaultHandler) to collect header fields and stream <file> entries one at a time.
Switched BackupImpl::readBackupMetadata to SAX-based parsing and deferred rethrow of callback exceptions after parsing.
Added coverage via a new stateless round-trip backup/restore query test and a new gtest suite for handler behavior.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/Backups/BackupImpl.cpp`	Replaces DOM-based `.backup` parsing with SAX handler callbacks while building the same in-memory maps.
`src/Backups/BackupMetadataHandler.h`	Declares streaming SAX handler interface, callback hooks, and captured-exception contract.
`src/Backups/BackupMetadataHandler.cpp`	Implements SAX event handling for header and per-file leaf extraction with exception capture.
`src/Backups/tests/gtest_backup_metadata_handler.cpp`	Adds unit tests for handler ordering, field reset, exception capture, and malformed XML behavior.
`tests/queries/0_stateless/04494_backup_metadata_round_trip.sh`	Adds end-to-end backup/restore round-trip validation (full + incremental).
`tests/queries/0_stateless/04494_backup_metadata_round_trip.reference`	Expected output for the new round-trip test.

pamarcos · 2026-07-02T15:35:44Z

+    /// Callbacks must not throw through expat; a captured exception is rethrown here.
+    if (handler.saved_exception)
+        std::rethrow_exception(handler.saved_exception);


Claude Fable 5 also flagged this same issue

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

clickhouse-gh · 2026-07-02T07:46:38Z

LLVM Coverage Report

Metric	Baseline	Current	Δ
Lines	85.50%	85.50%	+0.00%
Functions	92.60%	92.60%	+0.00%
Branches	77.70%	77.70%	+0.00%

Changed lines: Changed C/C++ lines covered: 193/213 (90.61%) · Uncovered code

Full report · Diff report

pamarcos

Validated end-to-end against a server built from this branch

Do you have some before/after numbers so that we can compare the amount of memory used in one case and the other? Feel free to assume a very bad scenario where this improvement would shine.

pamarcos · 2026-07-02T15:35:44Z

+    /// Callbacks must not throw through expat; a captured exception is rethrown here.
+    if (handler.saved_exception)
+        std::rethrow_exception(handler.saved_exception);


Claude Fable 5 also flagged this same issue

pamarcos · 2026-07-02T15:36:14Z

+    {
+        current_text.clear();
+        const String & name = qname.empty() ? local_name : qname;
+        if (name == "contents")


Claude Fable 5 found this same issue

pamarcos · 2026-07-02T15:39:49Z

+        version = parse<int>(req("version"));
+        if ((version < INITIAL_BACKUP_VERSION) || (version > CURRENT_BACKUP_VERSION))
+            throw Exception(
+                ErrorCodes::BACKUP_VERSION_NOT_SUPPORTED, "Backup {}: Version {} is not supported", backup_name_for_logging, version);


Feedback from Claude Fable 5:

[src/Backups/BackupImpl.cpp:606, 636-642, 648, 655] Value parsing became lenient where it used to fail closed on damaged manifests. Old XMLUtils::getBool → Poco::AbstractConfiguration::parseBool threw SyntaxException on unrecognized text (and accepted yes/on/case-insensitive True); the new get_bool (636-642) and the inline check for BASE_BACKUP_COPY_S3_CREDENTIALS_FROM_BACKUP (606) silently map anything other than exactly "true"/"1" to false. Old getUInt64/getInt → Poco::NumberParser threw on trailing garbage; DB::parse (648, 655) parses the digit prefix and silently ignores the rest (12x34 → 12; 1garbage → 1). ClickHouse itself only writes canonical values, so round-trips are unaffected — but for a corrupted manifest a silently misread encrypted_by_disk=false means restoring ciphertext as plaintext data instead of raising BACKUP_DAMAGED. Fix: use strict full-consumption parsing (e.g. parseFromString-style with EOF assertion) and throw BACKUP_DAMAGED on unrecognized bool text.

pamarcos · 2026-07-02T15:41:18Z

+full_backup="Disk('backups', '$full_id')"
+incr_backup="Disk('backups', '$incr_id')"


Here we're testing full and incremental, but what about archive backup?

pamarcos · 2026-07-02T15:47:54Z

+        if (name == "file")
+        {
+            if (on_file)
+                on_file(file_fields);


Feedback from Claude Fable 5:

endElement fires on_file for a at any depth (lines 45-48), and startElement re-fires on_header for any nested (lines 19-23). Concrete trace: n0 — the leaves land at depth 4, so file_fields is fully populated and on_file runs with no header applied and no version check; if <object_key> is present it reaches the LOGICAL_ERROR throw in BackupImpl.cpp instead of BACKUP_DAMAGED. The old DOM code only iterated direct children of /. Note this is not fixed by the "throw if was never seen" check alone — a document that does contain later can still process a misplaced before the header. Since path is already maintained, please gate the callbacks: fire on_header only when opens at depth 1, and on_file only when the path is exactly [root, contents, file].

pamarcos · 2026-07-02T15:51:19Z

+    xml_parser.parseMemoryNP(str.data(), str.size());
+
+    /// Callbacks must not throw through expat; a captured exception is rethrown here.
+    if (handler.saved_exception)
+        std::rethrow_exception(handler.saved_exception);


Feedback from Claude Fable 5:

If a callback exception was captured and the document is also malformed later on, parseMemoryNP throws the secondary XML error and saved_exception (the root cause, e.g. BACKUP_VERSION_NOT_SUPPORTED) is lost. Consider wrapping the parse in try/catch and preferring saved_exception when rethrowing. Relatedly, after a captured fatal error the parser still scans the remainder of a potentially multi-GB manifest with no way to abort early.

jkartseva and others added 4 commits July 1, 2026 23:19

clickhouse-gh Bot added the pr-improvement Pull request with some product improvements label Jul 2, 2026

clickhouse-gh Bot reviewed Jul 2, 2026

View reviewed changes

jkartseva requested a review from Copilot July 2, 2026 02:32

Copilot started reviewing on behalf of jkartseva July 2, 2026 02:33 View session

Copilot AI reviewed Jul 2, 2026

View reviewed changes

jkartseva and others added 2 commits July 1, 2026 20:57

DROP TABLE ... IF EXISTS

4a6c668

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

#include cleanup

55c6088

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

pamarcos self-assigned this Jul 2, 2026

pamarcos self-requested a review July 2, 2026 11:59

pamarcos reviewed Jul 2, 2026

View reviewed changes

alexey-milovidov mentioned this pull request Jul 4, 2026

Reject CREATE TABLE ... CLONE AS SELECT / table function #105004

Open

1 task

		full_backup="Disk('backups', '$full_id')"
		incr_backup="Disk('backups', '$incr_id')"

Sunbelt Computer Software

PL/B Language Development and Support

Uh oh!

Conversation

jkartseva commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changelog category (leave one):

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

Uh oh!

clickhouse-gh Bot commented Jul 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

AI Review

Summary

Findings

Tests

Final Verdict

Uh oh!

clickhouse-gh Bot Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

pamarcos Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

pamarcos Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

clickhouse-gh Bot commented Jul 2, 2026

LLVM Coverage Report

Uh oh!

pamarcos left a comment

Choose a reason for hiding this comment

Uh oh!

pamarcos Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

pamarcos Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

pamarcos Jul 2, 2026 • 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!

pamarcos Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

pamarcos Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

pamarcos Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jkartseva commented Jul 2, 2026 •

edited

Loading

clickhouse-gh Bot commented Jul 2, 2026 •

edited

Loading

pamarcos Jul 2, 2026 •

edited

Loading