> On 1 Jul 2025, at 17:42, Tom Lane wrote:
> There are some minor typos in the proposed commit message, too.
It seems that my grasp of the english language went on holiday before I did.
The attached v9 is hopefully less terrible.
--
Daniel Gustafsson
v9-0001-pg_dump-Fix-compression-API-error
Daniel Gustafsson writes:
> In preparing for concluding this I've attached a v8 which is the patchset in
> v7
> squashed into a single commit with an attempt at a commit message.
Glancing through this, I observe a couple of minor typos:
+ * Returns number of bytes read (this might be less t
> On 1 Jul 2025, at 17:11, Tomas Vondra wrote:
> On 7/1/25 16:24, Daniel Gustafsson wrote:
>> This version has been tested against v17 and v16 where it applies and passes
>> all tests (the latter isn't as assuring as it should be since there is a lack
>> of testcoverage).
>
> Could you elaborate
On 7/1/25 16:24, Daniel Gustafsson wrote:
>> On 26 Jun 2025, at 20:01, Daniel Gustafsson wrote:
>>
>>> On 26 Jun 2025, at 15:33, Tom Lane wrote:
>>
>>> So on the whole I prefer the "void" approach. I'm not dead
>>> set on that though, it's just a niggling worry.
>>
>> I think the likelyhood o
> On 26 Jun 2025, at 20:01, Daniel Gustafsson wrote:
>
>> On 26 Jun 2025, at 15:33, Tom Lane wrote:
>
>> So on the whole I prefer the "void" approach. I'm not dead
>> set on that though, it's just a niggling worry.
>
> I think the likelyhood of it being a problem in practice is pretty slim, b
> On 26 Jun 2025, at 15:33, Tom Lane wrote:
> So on the whole I prefer the "void" approach. I'm not dead
> set on that though, it's just a niggling worry.
I think the likelyhood of it being a problem in practice is pretty slim, but
it's still a stronger argument than my "match an API we're stil
Daniel Gustafsson writes:
> On 25 Jun 2025, at 17:58, Tom Lane wrote:
>> It doesn't return true anymore. Should be more like
>> + * Returns nothing. Exits via pg_fatal for all error conditions.
> Instead of this I changed the write_func signature to return the number of
> bytes written as size
> On 25 Jun 2025, at 17:58, Tom Lane wrote:
> I looked over this patchset briefly, and found a couple of nits:
Thanks for looking!
> 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
>
Daniel Gustafsson writes:
> 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.
> On 16 Jun 2025, at 22:49, Tom Lane wrote:
>
> Tomas Vondra 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
> branch
> On 16 Jun 2025, at 21:45, Tomas Vondra wrote:
> Regarding the Z_NULL, I believe it has always been ignored like this, at
> least since 9.1. The code simply returns what gzgets() returns, and then
> compares that to NULL, etc. Is there there's a better way to deal with
> Z_NULL? I suppose we cou
Tomas Vondra 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_resto
On 6/16/25 19:56, Tom Lane wrote:
> ...
>
> After looking around a bit more I realized that this API is a complete
> disaster: it's not only bug-prone, but there are actual bugs in
> multiple callers, eg _ReadBuf() totally fails to detect early EOF as
> it intends to. We need to fix it, not slap
On 6/16/25 17:41, Daniel Gustafsson wrote:
>> On 16 Jun 2025, at 16:20, Tom Lane wrote:
>>
>> Daniel Gustafsson writes:
On 16 Jun 2025, at 15:56, Tom Lane wrote:
I've not checked to see what the other users of this API do, but
if they're all like this then we need to fix that comm
Daniel Gustafsson writes:
> On 16 Jun 2025, at 16:20, Tom Lane wrote:
>> This API is actually quite bizarrely defined, if you ask me.
>> Returning the byte count is optional, but if you don't pay
>> attention to the byte count you cannot know if you got any
>> data. At best, that's bug-encouragi
> On 16 Jun 2025, at 16:20, Tom Lane wrote:
>
> Daniel Gustafsson writes:
>>> On 16 Jun 2025, at 15:56, Tom Lane wrote:
>>> I've not checked to see what the other users of this API do, but
>>> if they're all like this then we need to fix that comment.
>
>> AFAICT all other callers of this API
Daniel Gustafsson writes:
>> On 16 Jun 2025, at 15:56, Tom Lane wrote:
>> I've not checked to see what the other users of this API do, but
>> if they're all like this then we need to fix that comment.
> AFAICT all other callers of this API are throwing an error with pg_fatal, and
> so does the f
> On 16 Jun 2025, at 15:56, Tom Lane wrote:
> I've not checked to see what the other users of this API do, but
> if they're all like this then we need to fix that comment.
AFAICT all other callers of this API are throwing an error with pg_fatal, and
so does the function in question for ZStd deco
I think the real problem here is that what the code is doing is
precisely not what is documented in compress_io.h:
/*
* Read 'size' bytes of data from the file and store them into 'ptr'.
* Optionally it will store the number of bytes read in 'rsize'.
*
* Returns true on suc
16.06.2025 15:43, Daniel Gustafsson пишет:
On 16 Jun 2025, at 11:26, Evgeniy Gorbanev wrote:
I ran tests for pg_dump and they all passed. Logs attached.
Files=7, Tests=11918, 35 wallclock secs ( 0.59 usr 0.07 sys + 7.92 cusr 4.32
csys = 12.90 CPU)
That seems like a low number of tests exec
> On 16 Jun 2025, at 11:26, Evgeniy Gorbanev wrote:
> I ran tests for pg_dump and they all passed. Logs attached.
Files=7, Tests=11918, 35 wallclock secs ( 0.59 usr 0.07 sys + 7.92 cusr 4.32
csys = 12.90 CPU)
That seems like a low number of tests executed, with ZStd enabled I see over
13200
16.06.2025 15:00, Daniel Gustafsson пишет:
On 16 Jun 2025, at 10:52, Evgeniy Gorbanev wrote:
16.06.2025 14:25, Daniel Gustafsson пишет:
On 16 Jun 2025, at 10:14, Evgeniy Gorbanev wrote:
In src/bin/pg_dump/compress_zstd.c, the Zstd_read function always
returns true. But if you look at the Zstd
> On 16 Jun 2025, at 10:52, Evgeniy Gorbanev wrote:
>
> 16.06.2025 14:25, Daniel Gustafsson пишет:
>>> On 16 Jun 2025, at 10:14, Evgeniy Gorbanev wrote:
>>> In src/bin/pg_dump/compress_zstd.c, the Zstd_read function always
>>> returns true. But if you look at the Zstd_gets and Zstd_getc function
16.06.2025 14:25, Daniel Gustafsson пишет:
On 16 Jun 2025, at 10:14, Evgeniy Gorbanev wrote:
In src/bin/pg_dump/compress_zstd.c, the Zstd_read function always
returns true. But if you look at the Zstd_gets and Zstd_getc functions,
where Zstd_read is called via CFH->read_func, it is expected th
> On 16 Jun 2025, at 10:14, Evgeniy Gorbanev wrote:
> In src/bin/pg_dump/compress_zstd.c, the Zstd_read function always
> returns true. But if you look at the Zstd_gets and Zstd_getc functions,
> where Zstd_read is called via CFH->read_func, it is expected that
> the Zstd_read function can also r
Hello.
In src/bin/pg_dump/compress_zstd.c, the Zstd_read function always
returns true. But if you look at the Zstd_gets and Zstd_getc functions,
where Zstd_read is called via CFH->read_func, it is expected that
the Zstd_read function can also return false. In case of
a read-from-file error, the p
26 matches
Mail list logo
