From: | Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com> |
---|---|
To: | gkokolatos(at)pm(dot)me, Michael Paquier <michael(at)paquier(dot)xyz> |
Cc: | Justin Pryzby <pryzby(at)telsasoft(dot)com>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Rachel Heaton <rachelmheaton(at)gmail(dot)com> |
Subject: | Re: Add LZ4 compression in pg_dump |
Date: | 2023-01-18 14:00:34 |
Message-ID: | 41036470-041b-08a3-469d-fe07de2bc58e@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 1/16/23 16:14, gkokolatos(at)pm(dot)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
Attachment | Content-Type | Size |
---|---|---|
warnings.log | text/x-log | 1.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2023-01-18 15:06:02 | Re: ANY_VALUE aggregate |
Previous Message | Andrew Dunstan | 2023-01-18 13:43:01 | Re: Issue with psql's create_help.pl under perlcritic |