Re: No error checking when reading from file using zstd in pg_dump

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Tomas Vondra <tomas(at)vondra(dot)me>, Evgeniy Gorbanev <gorbanyoves(at)basealt(dot)ru>, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: No error checking when reading from file using zstd in pg_dump
Date: 2025-06-25 14:20:59
Message-ID: EBEE0042-E30B-43CB-8BAA-F51908248928@yesql.se
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> On 16 Jun 2025, at 22:49, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> Tomas Vondra <tomas(at)vondra(dot)me> writes:
>> For a moment I was worried about breaking ABI when fixing this in the
>> backbranches, but I guess that's not an issue for tools like pg_dump.
>
> Yeah, I think it'd be okay to change compress_io.h APIs in the back
> branches; it's hard to see how anything outside pg_dump+pg_restore
> would be depending on that.

Agreed, and the attached patchset takes that one step further by also changing
the signature of write_func to make errorhandling easier. More on that below.

I spent a little bit of time reading over all the implementations and cross
referencing the API for conformity, and came up with the attached. The 0001
patch is the one from upstream, and each subsequent commit is fixing one
function for all the implementations. Before pushing it should all be squashed
into a single commit IMHO.

Each patch has a commitmessage which describes the what/why so I won't go into
full detail here, but the main changes introduced are:

* Make write_func throw an error on all error conditions
* Ensure that functions return an error when assumed to, and call pg_fatal
when assumed to not return on error
* Try to capture more errors and ensure that errno has been reset

The reason for the jump in version number is that I've traded patches off-list
with Tomas over the past few days, some of which are omitted here due to being
v19 material. I might well have missed some changes which aren't required to
backpatch, and if so we need to pull those out.

One thing this doesn't address is the lack of testing, which also should be
tackled (having fault injection capabilities for client utils would be neat).

--
Daniel Gustafsson

Attachment Content-Type Size
v5-0001-Initial-patch-by-Tom-Lane.patch application/octet-stream 8.3 KB
v5-0002-pg_dump-compression-API-write_func.patch application/octet-stream 7.9 KB
v5-0003-pg_dump-compression-API-open_func.patch application/octet-stream 4.9 KB
v5-0004-pg_dump-compression-API-close_func.patch application/octet-stream 4.8 KB
v5-0005-pg_dump-compression-API-LZ4Stream_init.patch application/octet-stream 1.3 KB
v5-0006-pg_dump-compression-API-read_func-gets_func.patch application/octet-stream 6.4 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nathan Bossart 2025-06-25 14:36:02 Re: pg_dump --with-* options
Previous Message Dean Rasheed 2025-06-25 14:12:43 MERGE docs: indentation in synopsis section