Date: Fri, 20 Jan 2023 15:03:30 +0000
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
From: gkokolatos@pm.me
Cc: Michael Paquier <michael@paquier.xyz>, Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@lists.postgresql.org, Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <Qzw-4fJWmzIR99t4Wa3IjGtpYIpF6zCOdDhGRPVTDXfmB8RkM0tqBlK2pN5vY1BgyuMB8KlZ2u6apkVv4VXwCtSlevQpTIh8cSoEmtI_MSM=@pm.me>
In-Reply-To: <ce49a4f6-f7cc-28d7-3761-ddee7ef732af@enterprisedb.com>
References: <IZzs7Z2ny5hRK1EhTynSg3DnCo_z-UvEJN4Q6Hf0CUHWx3BcNwt-2z5vvvvA-Z8PQL-AYiL9YQPlKylATZ_Z6HnJ55YZUHgL66H9MZQFi48=@pm.me> <Y8S8dkXvymKauvUh@paquier.xyz> <twKgLFeqHEshCjdrbnzkwTMHiqfqvrrPSni7eKZjcxkForO-sI7HL4AKFgk9En1nGk3i4TXOY_An2xnCzv8MNjoLz4iupACaRB-0VRV20V4=@pm.me> <41036470-041b-08a3-469d-fe07de2bc58e@enterprisedb.com> <ljb5G0qPPj_MM5y-C5fRT24uuuj6n5I5RY__MpT7JdQ_B_AIE8vIOgdRO1uiBee6cymDzFLAKbYh3wDFv86jprLhVKOdx70D4g_4JTjqnY0=@pm.me> <6ab89d42-0345-b96e-02d4-68e171cdbe03@enterprisedb.com> <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me> <d8cbc586-a8e3-1662-c158-9022d5c8de83@enterprisedb.com> <ce49a4f6-f7cc-28d7-3761-ddee7ef732af@enterprisedb.com>
Feedback-ID: 6383013:user:proton
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/Qzw-4fJWmzIR99t4Wa3IjGtpYIpF6zCOdDhGRPVTDXfmB8RkM0tqBlK2pN5vY1BgyuMB8KlZ2u6apkVv4VXwCtSlevQpTIh8cSoEmtI_MSM%3D%40pm.me>
Precedence: bulk


------- Original Message -------
On Friday, January 20th, 2023 at 12:34 AM, Tomas Vondra <tomas.vondra@enter=
prisedb.com> wrote:


>=20
>=20
> On 1/19/23 18:55, Tomas Vondra wrote:
>=20
> > Hi,
> >=20
> > On 1/19/23 17:42, gkokolatos@pm.me wrote:
> >=20
> > > ...
> > >=20
> > > Agreed. It was initially submitted as one patch. Then it was requeste=
d to be
> > > split up in two parts, one to expand the use of the existing API and =
one to
> > > replace with the new interface. Unfortunately the expansion of usage =
of the
> > > existing API requires some tweaking, but that is not a very good reas=
on for
> > > the current patch set. I should have done a better job there.
> > >=20
> > > Please find v22 attach which combines back 0001 and 0002. It is missi=
ng the
> > > documentation that was discussed above as I wanted to give a quick fe=
edback.
> > > Let me know if you think that the combined version is the one to move=
 forward
> > > with.
> >=20
> > Thanks, I'll take a look.
>=20
>=20
> After taking a look and thinking about it a bit more, I think we should
> keep the two parts separate. I think Michael (or whoever proposed) the
> split was right, it makes the patches easier to grok.
>=20

Excellent. I will attempt a better split this time round.

>=20
> While reading the thread, I also noticed this:
>=20
> > By the way, I think that this 0002 should drop all the default clauses
> > in the switches for the compression method so as we'd catch any
> > missing code paths with compiler warnings if a new compression method
> > is added in the future.
>=20
>=20
> Now I realize why there were "not yet implemented" errors for lz4/zstd
> in all the switches, and why after removing them you had to add a
> default branch.
>=20
> We DON'T want a default branch, because the idea is that after adding a
> new compression algorithm, we get warnings about switches not handling
> it correctly.
>=20
> So I guess we should walk back this change too :-( It's probably easier
> to go back to v20 from January 16, and redo the couple remaining things
> I commented on.
>=20

Sure.

>=20
> FWIW I think this is a hint that adding LZ4/ZSTD options, in 5e73a6048,
> but without implementation, was not a great idea. It mostly defeats the
> idea of getting the compiler warnings - all the places already handle
> PG_COMPRESSION_LZ4/PG_COMPRESSION_ZSTD by throwing a pg_fatal. So you'd
> have to grep for the options, inspect all the places or something like
> that anyway. The warnings would only work for entirely new methods.
>=20
> However, I now also realize the compressor API in 0002 replaces all of
> this with calls to a generic API callback, so trying to improve this was
> pretty silly from me.

I can try to do a better job at splitting things up.

>=20
> Please, fix the couple remaining details in v20, add the docs for the
> callbacks, and I'll try to polish it and get it committed.

Excellent. Allow me an attempt to polish and expect a new version soon.

Cheers,
//Georgios

>=20
>=20
> regards
>=20
> --
> Tomas Vondra
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company