On Tue, Jan 18, 2022 at 9:43 AM Jeevan Ladhe <jeevan.la...@enterprisedb.com> wrote: > The patch surely needs some grooming, but I am expecting some initial > review, specially in the area where we are trying to close the zstd stream > in bbsink_zstd_end_archive(). We need to tell the zstd library to end the > compression by calling ZSTD_compressStream2() thereby sending a > ZSTD_e_end flag. But, this also needs some input string, which per > example[1] line # 686, I have taken as an empty ZSTD_inBuffer.
As far as I can see, this is correct. I found https://zstd.docsforge.com/dev/api-documentation/#streaming-compression-howto which seems to endorse what you've done here. One (minor) thing that I notice is that, the way you've written the loop in bbsink_zstd_end_archive(), I think it will typically call bbsink_archive_contents() twice. It will flush whatever is already present in the next sink's buffer as a result of the previous calls to bbsink_zstd_archive_contents(), and then it will call ZSTD_compressStream2() which will partially refill the buffer you just emptied, and then there will be nothing left in the internal buffer, so it will call bbsink_archive_contents() again. But ... the initial flush may not have been necessary. It could be that there was enough space already in the output buffer for the ZSTD_compressStream2() call to succeed without a prior flush. So maybe: do { yet_to_flush = ZSTD_compressStream2(..., ZSTD_e_end); check ZSTD_isError here; if (mysink->zstd_outBuf.pos > 0) bbsink_archive_contents(); } while (yet_to_flush > 0); I believe this might be very slightly more efficient. -- Robert Haas EDB: http://www.enterprisedb.com
