Re: [HACKERS] Support for pg_receivexlog --format=plain|tar

On Mon, Jan 16, 2017 at 9:12 PM, Magnus Hagander <magnus@hagander.net> wrote:
> Where did your research point to then? :) I just read the gzip rfc
> (http://www.zlib.org/rfc-gzip.html) which seems to call it that at least?

Well, OK. I was not aware of this RFC. I guessed it by looking at the
code of gzip, that uses the CRC as well. I also found some reference
into a blog post.

>> > Finally, I think we should make the error message clearly say
>> > "compressed
>> > segment file" - just to make things extra clear.
>>
>> Sure.
>
> AFAICT the
> +                       iscompress ? "compressed" : "",
> part of the error handling is unnecessary, because iscompressed will always
> be true in that block. All the other error messages in that codepath has
> compressed hardcoded in them, as should this one.

Fat-fingered here..

>> Hm. It looks that you are right. zlib goes down to _tr_flush_bits() to
>> flush some output, but this finishes only with put_byte(). As the fd
>> is opaque in gzFile, it would be just better to open() the file first,
>> and then use gzdopen to get the gzFile. Let's use as well the existing
>> fd field to save it. gzclose() closes as well the parent fd per the
>> documentation of zlib.
>
> This version throws a warning:
> walmethods.c: In function ‘dir_open_for_write’:
> walmethods.c:170:11: warning: ‘gzfp’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
>    f->gzfp = gzfp;

gcc and clang did not complain here, what did you use?

> I can't see that there is any code path where this can actually happen
> though, so we should probably just initialize it to NULL at variable
> declaration.  Or do you see a path where this could actually be incorrect?

Not that I see. All the code paths using gzfp are under
data_dir->compression > 0.

> If you agree with those two comments, I will go ahead and push with those
> minor fixes.

No problem for me, thanks for the review!