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

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions