From: Tom Lane <tgl@sss.pgh.pa.us>
To: Daniel Gustafsson <daniel@yesql.se>
cc: Tomas Vondra <tomas@vondra.me>, Evgeniy Gorbanev <gorbanyoves@basealt.ru>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: No error checking when reading from file using zstd in pg_dump
In-reply-to: <EBEE0042-E30B-43CB-8BAA-F51908248928@yesql.se>
References: <6b9817a8-88ec-4efd-b441-9e2a0439c6b8@basealt.ru>
 <C22FF2A1-783D-4334-998B-2D4432EFD7C5@yesql.se>
 <95255a86-4bc7-4ff4-aa26-4441aafdf0fb@basealt.ru>
 <522DB522-3962-4F91-A691-2DE790F6CCD6@yesql.se>
 <b653a2f4-1dcb-45ba-b899-fc6aa80794f2@basealt.ru>
 <D38B293E-E886-4F0D-BD5A-C958DCB2344C@yesql.se>
 <e2b7edd1-c5b8-484d-9c79-7a7c4466fad2@basealt.ru>
 <517794.1750082166@sss.pgh.pa.us>
 <3815B6CD-924B-40FA-823A-55E2F8F24602@yesql.se>
 <520142.1750083607@sss.pgh.pa.us>
 <D011594F-7FA3-4BE4-BF89-B1AB18CD3456@yesql.se>
 <644360.1750096605@sss.pgh.pa.us>
 <f2dec453-b6af-4495-a75d-a39628411510@vondra.me>
 <666035.1750106940@sss.pgh.pa.us>
 <EBEE0042-E30B-43CB-8BAA-F51908248928@yesql.se>
Comments: In-reply-to Daniel Gustafsson <daniel@yesql.se>
	message dated "Wed, 25 Jun 2025 16:20:59 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1296194.1750867134.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Wed, 25 Jun 2025 11:58:54 -0400
Message-ID: <1296195.1750867134@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1296195.1750867134%40sss.pgh.pa.us>
Precedence: bulk

Daniel Gustafsson <daniel@yesql.se> writes:
> I spent a little bit of time reading over all the implementations and cr=
oss
> 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 s=
quashed
> into a single commit IMHO.

Thanks for tackling this!

I looked over this patchset briefly, and found a couple of nits:

v5-0002, in compress_io.h:

+	 * Returns true on success and throws error for all error conditions.

It doesn't return true anymore.  Should be more like

+	 * Returns nothing.  Exits via pg_fatal for all error conditions.

In LZ4Stream_write: you dropped the bit about

-			errno =3D (errno) ? errno : ENOSPC;

but I think that's still necessary: we must assume ENOSPC if fwrite
doesn't set errno.  Other fwrite callers (write_none, Zstd_write) need
this too.  v5-0004 has an instance too, in Zstd_close.  I did not check
to see if other fwrite calls are OK, but it'd be good to verify
that they all follow the pattern of presetting errno to 0 and then
replacing that with ENOSPC.

			regards, tom lane