On Fri, Sep 17, 2021 at 08:12:42AM +0000, gkokolatos@pm.me wrote:
> Yeah, I was considering it to split them over to a separate commit,
> then decided against it. Will do so in the future.
I have been digging into the issue I saw in the TAP tests when closing
a segment, and found the problem. The way you manipulate
frameInfo.contentSize by just setting it to WalSegSz when *opening*
a segment causes problems on LZ4F_compressEnd(), making the code
throw a ERROR_frameSize_wrong. In lz4frame.c, the end of
LZ4F_compressEnd() triggers this check and the error:
if (cctxPtr->prefs.frameInfo.contentSize) {
if (cctxPtr->prefs.frameInfo.contentSize != cctxPtr->totalInSize)
return err0r(LZ4F_ERROR_frameSize_wrong);
}
We don't really care about contentSize as long as a segment is not
completed. Rather than filling contentSize all the time we write
something, we'd better update frameInfo once the segment is
completed and closed. That would also take take of the error as this
is not checked if contentSize is 0. It seems to me that we should
fill in the information when doing a CLOSE_NORMAL.
- if (stream->walmethod->compression() == 0 &&
+ if (stream->walmethod->compression() == COMPRESSION_NONE &&
stream->walmethod->existsfile(fn))
This one was a more serious issue, as the compression() callback would
return an integer for the compression level but v5 compared it to a
WalCompressionMethod. In order to take care of this issue, mainly for
pg_basebackup, I think that we have to update the compression()
callback to compression_method(), and it is cleaner to save the
compression method as well as the compression level for the tar data.
I am attaching a new patch, on which I have done many tweaks and
adjustments while reviewing it. The attached patch fixes the second
issue, and I have done nothing about the first issue yet, but that
should be simple enough to address as this needs an update of the
frame info when closing a completed segment. Could you look at it?
Thanks,
--
Michael
