Content-Type: multipart/mixed; boundary="------------yXH9GIG10Kin5w60VmsZ9biJ"
Message-ID: <41036470-041b-08a3-469d-fe07de2bc58e@enterprisedb.com>
Date: Wed, 18 Jan 2023 15:00:34 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: Add LZ4 compression in pg_dump
Content-Language: en-US
To: gkokolatos@pm.me, Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>, pgsql-hackers@lists.postgresql.org,
 Rachel Heaton <rachelmheaton@gmail.com>
References: <IZzs7Z2ny5hRK1EhTynSg3DnCo_z-UvEJN4Q6Hf0CUHWx3BcNwt-2z5vvvvA-Z8PQL-AYiL9YQPlKylATZ_Z6HnJ55YZUHgL66H9MZQFi48=@pm.me>
 <20221220152610.GG1153@telsasoft.com>
 <f8Wmj1EmRT7CHBfMcg0jM_Wts_hyV22WsLWzf5X5KImQo_5CtZXHK3UjxL8Wne6g7zSccwXLNToLCINfEIPbNni53gKO17AuHplMjm-XBmA=@pm.me>
 <20221222170859.GO1153@telsasoft.com> <20230108194524.GA27637@telsasoft.com>
 <20230114214308.GC9837@telsasoft.com> <Y8SoUuqbNoMBnFJp@paquier.xyz>
 <20230116015625.GH9837@telsasoft.com> <Y8S8dkXvymKauvUh@paquier.xyz>
 <twKgLFeqHEshCjdrbnzkwTMHiqfqvrrPSni7eKZjcxkForO-sI7HL4AKFgk9En1nGk3i4TXOY_An2xnCzv8MNjoLz4iupACaRB-0VRV20V4=@pm.me>
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
In-Reply-To: <twKgLFeqHEshCjdrbnzkwTMHiqfqvrrPSni7eKZjcxkForO-sI7HL4AKFgk9En1nGk3i4TXOY_An2xnCzv8MNjoLz4iupACaRB-0VRV20V4=@pm.me>
Archived-At: <https://www.postgresql.org/message-id/41036470-041b-08a3-469d-fe07de2bc58e%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------yXH9GIG10Kin5w60VmsZ9biJ
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Hi,

On 1/16/23 16:14, gkokolatos@pm.me wrote:
> Hi,
> 
> I admit I am completely at lost as to what is expected from me anymore.
> 

:-(

I understand it's frustrating not to know why a patch is not moving
forward. Particularly when is seems fairly straightforward ...

Let me briefly explain my personal (and admittedly very subjective) view
on picking what patches to review/commit. I'm sure other committers have
other criteria, but maybe this will help.

There are always more patches than I can review/commit, so I have to
prioritize, and pick which patches to look at. For me, it's mostly about
cost/benefit of the patch. The cost is e.g. the amount of time I need to
spend to review/commit the stuff, maybe read the thread, etc. Benefits
is mainly the new features/improvements.

It's oversimplified, we could talk about various bits that contribute to
the costs and benefits, but this is what it boils down.

There's always the aspect of time - patches A and B have roughly the
same benefits, but with A we get it "immediately" while B requires
additional parts that we don't have ready yet (and if they don't make it
we get no benefit), I'll probably pick A.

Unfortunately, this plays against this patch - I'm certainly in favor of
adding lz4 (and other compression algos) into pg_dump, but if I commit
0001 we get little benefit, and the other parts actually adding lz4/zstd
are treated as "WIP / for completeness" so it's unclear when we'd get to
commit them.

So if I could recommend one thing, it'd be to get at least one of those
WIP patches into a shape that's likely committable right after 0001.

> I had posted v19-0001 for a committer's consideration and v19-000{2,3} for completeness.
> Please find a rebased v20 attached.
> 

I took a quick look at 0001, so a couple comments (sorry if some of this
was already discussed in the thread):

1) I don't think a "refactoring" patch should reference particular
compression algorithms (lz4/zstd), and in particular I don't think we
should have "not yet implemented" messages. We only have a couple other
places doing that, when we didn't have a better choice. But here we can
simply reject the algorithm when parsing the options, we don't need to
do that in a dozen other places.

2) I wouldn't reorder the cases in WriteDataToArchive, i.e. I'd keep
"none" at the end. It might make backpatches harder.

3) While building, I get bunch of warnings about missing cfdopen()
prototype and pg_backup_archiver.c not knowing about cfdopen() and
adding an implicit prototype (so I doubt it actually works).

4) "cfp" struct no longer wraps gzFile, but the comment was not updated.
FWIW I'm not sure switching to "void *" is an improvement, maybe it'd be
better to have a "union" of correct types?

5) cfopen/cfdopen are missing comments. cfopen_internal has an updated
comment, but that's a static function while cfopen/cfdopen are the
actual API.

> Also please let me know if I should silently step away from it and let other people lead
> it. I would be glad to comply either way.
> 

Please don't. I promise to take a look at this patch again.

Thanks for doing all the work.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--------------yXH9GIG10Kin5w60VmsZ9biJ
Content-Type: text/x-log; charset=UTF-8; name="warnings.log"
Content-Disposition: attachment; filename="warnings.log"
Content-Transfer-Encoding: base64

UmVhZERhdGFGcm9tQXJjaGl2ZVpsaWIKCgpjb21wcmVzc19pby5jOjYwNToxOiB3YXJuaW5n
OiBubyBwcmV2aW91cyBwcm90b3R5cGUgZm9yIOKAmGNmZG9wZW7igJkgWy1XbWlzc2luZy1w
cm90b3R5cGVzXQogIDYwNSB8IGNmZG9wZW4oaW50IGZkLCBjb25zdCBjaGFyICptb2RlLAog
ICAgICB8IF5+fn5+fn4KcGdfYmFja3VwX2FyY2hpdmVyLmM6IEluIGZ1bmN0aW9uIOKAmFNl
dE91dHB1dOKAmToKcGdfYmFja3VwX2FyY2hpdmVyLmM6MTUyODoyNjogd2FybmluZzogaW1w
bGljaXQgZGVjbGFyYXRpb24gb2YgZnVuY3Rpb24g4oCYY2Zkb3BlbuKAmTsgZGlkIHlvdSBt
ZWFuIOKAmGNmb3BlbuKAmT8gWy1XaW1wbGljaXQtZnVuY3Rpb24tZGVjbGFyYXRpb25dCiAx
NTI4IHwgICAgICAgICAgICAgICAgIEFILT5PRiA9IGNmZG9wZW4oZHVwKGZuKSwgbW9kZSwg
Y29tcHJlc3Npb25fc3BlYyk7CiAgICAgIHwgICAgICAgICAgICAgICAgICAgICAgICAgIF5+
fn5+fn4KICAgICAgfCAgICAgICAgICAgICAgICAgICAgICAgICAgY2ZvcGVuCnBnX2JhY2t1
cF9hcmNoaXZlci5jOjE1Mjg6MjQ6IHdhcm5pbmc6IGFzc2lnbm1lbnQgdG8g4oCYdm9pZCAq
4oCZIGZyb20g4oCYaW504oCZIG1ha2VzIHBvaW50ZXIgZnJvbSBpbnRlZ2VyIHdpdGhvdXQg
YSBjYXN0IFstV2ludC1jb252ZXJzaW9uXQogMTUyOCB8ICAgICAgICAgICAgICAgICBBSC0+
T0YgPSBjZmRvcGVuKGR1cChmbiksIG1vZGUsIGNvbXByZXNzaW9uX3NwZWMpOwogICAgICB8
ICAgICAgICAgICAgICAgICAgICAgICAgXgpwZ19iYWNrdXBfYXJjaGl2ZXIuYzogSW4gZnVu
Y3Rpb24g4oCYX2FsbG9jQUjigJk6CnBnX2JhY2t1cF9hcmNoaXZlci5jOjIyMzY6MTY6IHdh
cm5pbmc6IGFzc2lnbm1lbnQgdG8g4oCYdm9pZCAq4oCZIGZyb20g4oCYaW504oCZIG1ha2Vz
IHBvaW50ZXIgZnJvbSBpbnRlZ2VyIHdpdGhvdXQgYSBjYXN0IFstV2ludC1jb252ZXJzaW9u
XQogMjIzNiB8ICAgICAgICAgQUgtPk9GID0gY2Zkb3BlbihkdXAoZmlsZW5vKHN0ZG91dCkp
LCBQR19CSU5BUllfQSwgb3V0X2NvbXByZXNzX3NwZWMpOwogICAgICB8ICAgICAgICAgICAg
ICAgIF4K

--------------yXH9GIG10Kin5w60VmsZ9biJ--