Thread: Re: basebackup/lz4 crash
On Wed, Mar 30, 2022 at 10:35 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > It looks like maybe this code was copied from > bbstreamer_lz4_compressor_finalize() which has an Assert rather than expanding > the buffer like in bbstreamer_lz4_compressor_new() Hmm, this isn't great. On the server side, we set up a chain of bbsink buffers that can't be resized later. Each bbsink tells the next bbsink how to make its buffer, but the successor bbsink has control of that buffer and resizing it on-the-fly is not allowed. It looks like bbstreamer_lz4_compressor_new() is mimicking that logic, but not well. It sets the initial buffer size to LZ4F_compressBound(streamer->base.bbs_buffer.maxlen, prefs), but streamer->base.bbs_buffer.maxlen is not any kind of promise from the caller about future chunk sizes. It's just whatever initStringInfo() happens to do. My guess is that Dipesh thought that the buffer wouldn't need to be resized because "we made it big enough already" but that's not the case. The server knows how much data it is going to read from disk at a time, but the client has to deal with whatever the server sends. I think your proposed change is OK, modulo some comments. But I think maybe we ought to delete all the stuff related to compressed_bound from bbstreamer_lz4_compressor_new() as well, because I don't see that there's any point. And then I think we should also add logic similar to what you've added here to bbstreamer_lz4_compressor_finalize(), so that we're not making the assumption that the buffer will get enlarged at some earlier point. Thoughts? -- Robert Haas EDB: http://www.enterprisedb.com
Hi,
> I think your proposed change is OK, modulo some comments. But I think
> maybe we ought to delete all the stuff related to compressed_bound
> from bbstreamer_lz4_compressor_new() as well, because I don't see that
> there's any point. And then I think we should also add logic similar
> to what you've added here to bbstreamer_lz4_compressor_finalize(), so
> that we're not making the assumption that the buffer will get enlarged
> at some earlier point.
>
> Thoughts?
> maybe we ought to delete all the stuff related to compressed_bound
> from bbstreamer_lz4_compressor_new() as well, because I don't see that
> there's any point. And then I think we should also add logic similar
> to what you've added here to bbstreamer_lz4_compressor_finalize(), so
> that we're not making the assumption that the buffer will get enlarged
> at some earlier point.
>
> Thoughts?
I agree that we should remove the compression bound stuff from
bbstreamer_lz4_compressor_new() and add a fix in
bbstreamer_lz4_compressor_content() and bbstreamer_lz4_compressor_finalize()
to enlarge the buffer if it falls short of the compress bound.
Patch attached.
Thanks,
Dipesh
Attachment
On Thu, Mar 31, 2022 at 3:19 AM Dipesh Pandit <dipesh.pandit@gmail.com> wrote: > Patch attached. Committed. Thanks. -- Robert Haas EDB: http://www.enterprisedb.com