Skip to content

Follow-ups from code review of R2 uniform-multipart patch #1

Description

@alleneubank

Context

Adversarial code review of the fork patch (bb/fix-r2-multipart-uniform-parts: f45f59d, 39f4c52, 6eccfb3) performed 2026-06-10 before promoting the CNPG overlay image to mainnet. The patch fixes intermittent Cloudflare R2 backup failures — InvalidPart on CompleteMultipartUpload — by emitting uniform fixed-size multipart parts (upstream issue: EnterpriseDB#954). Runtime diff vs barman 3.17.0 is confined to barman/cloud.py (+85/−25) plus tests (tests/test_cloud.py +174).

Review verdict: safe. Verified correct: byte-stream slicing (no drop/dup/reorder), sequential part numbering in the main process, parts materialized to disk before enqueue and sorted by PartNumber before CompleteMultipartUpload (safe with --jobs 2), no empty trailing part on exact part-size multiples, 10000-part max and 5 MiB min enforced, abort path exits nonzero, WAL archiving path untouched (uses upload_fileobj(), not CloudTarUploader). 239 tests pass including 9 new.

Validated in production: testnet canary (on-demand + scheduled backups, full restore drill with pg_amcheck --heapallindexed clean) and mainnet (41-minute backup 20260611T010836, zero retries/errors).

Non-blocking follow-ups

  • Clarify misleading "retry callers" comment in barman/cloud.py — the comment near _upload_part_buffer suggests retry semantics that don't match the actual call sites; reword to describe what the is_final flag actually guards.
  • Add compressor-path test — existing new tests cover the uncompressed tar path; add coverage for CloudTarUploader with a compressor configured (gzip/bzip2) to lock in uniform part sizing on the compressed stream.
  • Add close()-on-exact-multiple test — assert no empty trailing part is uploaded when the stream length is an exact multiple of the part size.

Pre-existing issues observed (not introduced by the patch)

  • Temp-file leak on abort path (barman/cloud.py:325-328) — on the already-fatal abort path, part temp files are not cleaned up. Process is exiting nonzero anyway, but worth tidying.
  • Zero-byte-stream edge in close() (barman/cloud.py:339-348) — a zero-byte stream would create a multipart upload with no parts; cannot occur in practice (tar streams are never empty) but cheap to guard.

Strategic

  • Upstream the fix as a PR against EnterpriseDB/barman (ref barman#954) so this fork can retire.
  • CNPG 1.30 migration: in-tree barmanObjectStore support is removed in CNPG 1.30 (deprecation warning already emitted on 1.29). The replacement barman-cloud plugin sidecar will need this same fix — track whether upstream lands it before we migrate, otherwise the overlay-image approach needs rethinking for the plugin image.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions