Skip to content

Skip the COPY file header when scanning for row boundaries#532

Merged
staticlibs merged 1 commit into
duckdb:mainfrom
marknefedov:fix/binary-copy-header-boundary-scan
Jul 25, 2026
Merged

Skip the COPY file header when scanning for row boundaries#532
staticlibs merged 1 commit into
duckdb:mainfrom
marknefedov:fix/binary-copy-header-boundary-scan

Conversation

@marknefedov

@marknefedov marknefedov commented Jul 24, 2026

Copy link
Copy Markdown

Problem

PostgresBinaryFileReader::FindLastCompleteRow starts scanning at byte zero of the first buffer, so it reads the 19 byte binary COPY file header as if it were a tuple:

  • 'P','G' → field count 20551
  • 'C','O','P','Y' → field length 1129270361

That length exceeds any buffer, so no row is ever completed, the scan returns 0, and FillBuffer throws:

IO Error: Postgres binary file contains a row larger than the read buffer (N bytes). Increase buffer_size.

This fires for any file that does not fit in a single buffer. With the default DEFAULT_BUFFER_SIZE of 32 MiB, every postgres binary file larger than 32 MiB is rejected, no matter how small its rows are.

Files that fit in one buffer are unaffected, because file_offset >= file_size makes FillBuffer fall back to valid = total and the misparse is never observed. Every existing fixture in test/sql/misc/postgres_binary_read_basic.test is a few dozen bytes, which is why this was not caught.

Fix

Two parts:

  1. Skip the header while scanning for row boundaries on the first fill, keeping it in the buffer so CheckHeader still validates it. Subsequent fills start at a real row boundary and are unchanged.

  2. Treat the header itself as a valid first boundary. The first fill only has buffer_size - 19 bytes available after the header, so a first row larger than that but no larger than buffer_size would otherwise still be rejected even though it fits. When no complete row fits alongside the header, the parser is handed just the header and the partial row carries over to the next fill, where the whole buffer is available. CheckHeader's bound goes from >= to > so a buffer holding exactly the header is accepted. A row therefore only has to fit in buffer_size, not in buffer_size - 19.

COPY_FILE_HEADER_SIZE is derived from PostgresConversion::COPY_HEADER_LENGTH so it stays in step with what CheckHeader consumes.

Verification

FindLastCompleteRow, the patched FillBuffer loop and CheckHeader were reproduced verbatim in a standalone harness and driven with the exact byte streams PostgresBinaryWriter produces:

  • 1000 rows × 10 bytes at buffer sizes 19, 20, 24, 28, 29, 32, 64 and 4096 — all read 1000 rows with correct payloads (buffer_size=19 is a buffer of exactly the header)
  • every row size from buffer_size - 18 through buffer_size, for buffer sizes 32/40/48/56/64 — all succeed, covering the header-carry path
  • rows of buffer_size + 1 bytes — still rejected with row larger than the read buffer
  • degenerate inputs: CSV data and a buffer smaller than the header still raise invalid header; header-only files read as zero rows

Test

test/sql/misc/postgres_binary_read_buffer.test:

  • 1000 row file at buffer_size 19, 24, 32 and 64, with an exact round trip via EXCEPT
  • variable width columns and NULLs spanning buffer boundaries
  • 56 byte rows read at buffer_size=64 (larger than bs - 19, smaller than bs) and at buffer_size=56 (exactly one row per buffer)
  • 106 byte rows at buffer_size=64, which must still raise row larger than the read buffer

Behaviour notes for review

  • A file containing only the 19 byte header and no footer now reads as zero rows instead of raising invalid header. The parser already accepts files that end after N rows without a footer, so this matches existing leniency rather than adding new — but it is a change; say the word if footer validation is preferred.
  • With buffer_size smaller than the header itself (< 19), the constructor still fails, but with invalid header rather than a message about the buffer being too small. The header is valid in that case — the buffer just cannot hold it. Pathological configuration; left as is to keep the change minimal.
  • The pre-existing valid = total fallback at EOF (which tolerates trailing garbage in the last buffer) is preserved unchanged.

PostgresBinaryFileReader::FindLastCompleteRow started scanning at byte
zero of the first buffer, so it read the 19 byte binary COPY file header
as if it were a tuple: 'PG' became a field count of 20551 and 'COPY'
became a field length of 1129270361. No row could ever be completed, the
scan returned 0, and the reader threw

  Postgres binary file contains a row larger than the read buffer

for any file that did not fit in a single buffer. With the default 32MB
buffer that rejected every postgres binary file larger than 32MB,
regardless of how small its rows were.

Skip the header while scanning for row boundaries on the first fill, but
keep it in the buffer so CheckHeader can still validate it.

When no complete row fits alongside the header, hand the parser just the
header and carry the partial row over to the next fill, where the whole
buffer is available for it. Otherwise a row larger than buffer_size - 19
but no larger than buffer_size would still be rejected even though it
fits. CheckHeader's bound becomes '>' so that a buffer holding exactly
the header is accepted.

Files that fit in one buffer were unaffected, which is why the existing
tests - all of which use tiny fixtures - did not catch this.
@marknefedov
marknefedov force-pushed the fix/binary-copy-header-boundary-scan branch from a0f93a5 to a404c80 Compare July 24, 2026 21:39
@marknefedov
marknefedov marked this pull request as ready for review July 24, 2026 21:45

@staticlibs staticlibs left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Looks good to me.

@staticlibs
staticlibs merged commit 9ab3255 into duckdb:main Jul 25, 2026
7 checks passed
staticlibs pushed a commit that referenced this pull request Jul 25, 2026
This is a backport of the PR #532 to `v1.5-variegata` stable branch.

PostgresBinaryFileReader::FindLastCompleteRow started scanning at byte
zero of the first buffer, so it read the 19 byte binary COPY file header
as if it were a tuple: 'PG' became a field count of 20551 and 'COPY'
became a field length of 1129270361. No row could ever be completed, the
scan returned 0, and the reader threw

  Postgres binary file contains a row larger than the read buffer

for any file that did not fit in a single buffer. With the default 32MB
buffer that rejected every postgres binary file larger than 32MB,
regardless of how small its rows were.

Skip the header while scanning for row boundaries on the first fill, but
keep it in the buffer so CheckHeader can still validate it.

When no complete row fits alongside the header, hand the parser just the
header and carry the partial row over to the next fill, where the whole
buffer is available for it. Otherwise a row larger than buffer_size - 19
but no larger than buffer_size would still be rejected even though it
fits. CheckHeader's bound becomes '>' so that a buffer holding exactly
the header is accepted.

Files that fit in one buffer were unaffected, which is why the existing
tests - all of which use tiny fixtures - did not catch this.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants