VARDATA_COMPRESSED_GET_COMPRESS_METHOD comment?

On Tue, Sep 7, 2021 at 11:56 AM Christoph Berg <myon@debian.org> wrote:

In postgres.h, there are these macros for working with compressed
toast:

vvvvvvvv
/* Decompressed size and compression method of an external compressed Datum */
#define VARDATA_COMPRESSED_GET_EXTSIZE(PTR) \
(((varattrib_4b *) (PTR))->va_compressed.va_tcinfo & VARLENA_EXTSIZE_MASK)
#define VARDATA_COMPRESSED_GET_COMPRESS_METHOD(PTR) \
(((varattrib_4b *) (PTR))->va_compressed.va_tcinfo >> VARLENA_EXTSIZE_BITS)

/* Same, when working directly with a struct varatt_external */
#define VARATT_EXTERNAL_GET_EXTSIZE(toast_pointer) \
((toast_pointer).va_extinfo & VARLENA_EXTSIZE_MASK)
#define VARATT_EXTERNAL_GET_COMPRESS_METHOD(toast_pointer) \
((toast_pointer).va_extinfo >> VARLENA_EXTSIZE_BITS)

On the first line, is the comment "external" correct? It took me quite
a while to realize that the first two macros are the methods to be
used on an *inline* compressed Datum, when the second set is used for
varlenas in toast tables.

Well ... technically the second set are used on a TOAST pointer, which
is not really the same thing as a varlena. The varlena would start
with a 1-byte header identifying it as a TOAST pointer, and then
there'd be a 1-byte saying what kind of TOAST pointer it is, which
would be VARTAG_ONDISK if this is coming from a tuple on disk, and
then the TOAST pointer would start after that. So toast_pointer =
varlena_pointer + 2, if I'm not confused here.

But I agree with you that referring to the argument to
VARDATA_COMPRESSED_GET_EXTSIZE or
VARDATA_COMPRESSED_GET_COMPRESS_METHOD as an "external compressed
Datum" doesn't seem quite right. It is compressed, but it is not
external, at least in the sense that I understand that term.

--
Robert Haas
EDB: http://www.enterprisedb.com

Re: Robert Haas

But I agree with you that referring to the argument to
VARDATA_COMPRESSED_GET_EXTSIZE or
VARDATA_COMPRESSED_GET_COMPRESS_METHOD as an "external compressed
Datum" doesn't seem quite right. It is compressed, but it is not
external, at least in the sense that I understand that term.

How about "compressed-in-line Datum" like on the comment 5 lines above?

/* caution: this will not work on an external or compressed-in-line Datum */
/* caution: this will return a possibly unaligned pointer */
#define VARDATA_ANY(PTR) \
(VARATT_IS_1B(PTR) ? VARDATA_1B(PTR) : VARDATA_4B(PTR))

/* Decompressed size and compression method of an external compressed Datum */
#define VARDATA_COMPRESSED_GET_EXTSIZE(PTR) \
(((varattrib_4b *) (PTR))->va_compressed.va_tcinfo & VARLENA_EXTSIZE_MASK)
#define VARDATA_COMPRESSED_GET_COMPRESS_METHOD(PTR) \
(((varattrib_4b *) (PTR))->va_compressed.va_tcinfo >> VARLENA_EXTSIZE_BITS)

This "external" there cost me about one hour of extra poking around
until I realized this is actually the macro I wanted.

-> /* Decompressed size and compression method of a compressed-in-line Datum */

Christoph

On Wed, Sep 8, 2021 at 11:33 AM Christoph Berg <myon@debian.org> wrote:

How about "compressed-in-line Datum" like on the comment 5 lines above?

That seems reasonable to me, but I think Tom Lane is responsible for
the current form of that comment, so it'd be nice to hear what he
thinks.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Sep 8, 2021 at 11:33 AM Christoph Berg <myon@debian.org> wrote:

How about "compressed-in-line Datum" like on the comment 5 lines above?

That seems reasonable to me, but I think Tom Lane is responsible for
the current form of that comment, so it'd be nice to hear what he
thinks.

Hmm ... looks like I copied-and-pasted that comment to the wrong place
while rearranging stuff in aeb1631ed. The comment just below is
off-point too. Will fix.

regards, tom lane