On Thu, Feb 11, 2021 at 1:37 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Feb 10, 2021 at 9:52 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > [ new patches ]
>
> I think that in both varattrib_4b and toast_internals.h it would be
> better to pick a less generic field name. In toast_internals.h it's
> just info; in postgres.h it's va_info. But:
>
> [rhaas pgsql]$ git grep info | wc -l
> 24552
>
> There are no references in the current source tree to va_info, so at
> least that one is greppable, but it's still not very descriptive. I
> suggest info -> tcinfo and va_info -> va_tcinfo, where "tc" stands for
> "TOAST compression". Looking through 24552 references to info to find
> the ones that pertain to this feature might take longer than searching
> the somewhat shorter list of references to tcinfo, which prepatch is
> just:
>
> [rhaas pgsql]$ git grep tcinfo | wc -l
> 0
Done as suggested
>
> I don't see why we should allow for datum_decompress to be optional,
> as toast_decompress_datum_slice does. Likely every serious compression
> method will support that anyway. If not, the compression AM can deal
> with the problem, rather than having the core code do it. That will
> save some tiny amount of performance, too.
Done
> src/backend/access/compression/Makefile is missing a copyright header.
Fixed
> It's really sad that lz4_cmdecompress_slice allocates
> VARRAWSIZE_4B_C(value) + VARHDRSZ rather than slicelength + VARHDRSZ
> as pglz_cmdecompress_slice() does. Is that a mistake, or is that
> necessary for some reason? If it's a mistake, let's fix it. If it's
> necessary, let's add a comment about why, probably starting with
> "Unfortunately, ....".
In older versions of the lz4 there was a problem that the decompressed
data size could be bigger than the slicelength which is resolved now
so we can allocate slicelength + VARHDRSZ, I have fixed it.
Please refer the latest patch at
https://www.postgresql.org/message-id/CAFiTN-u2pyXDDDwZXJ-fVUwbLhJSe9TbrVR6rfW_rhdyL1A5bg%40mail.gmail.com
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
