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 |
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 |