On Thu, Feb 11, 2021 at 1:37 AM Robert Haas <robertmh...@gmail.com> wrote: > > On Wed, Feb 10, 2021 at 9:52 AM Dilip Kumar <dilipbal...@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