Horton

Thread

Commits

  1. Fix misleading comments about TOAST access macros.

  1. VARDATA_COMPRESSED_GET_COMPRESS_METHOD comment?

    Christoph Berg <myon@debian.org> — 2021-09-07T15:55:54Z

    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.

Context: Me figuring out https://github.com/credativ/toastinfo/blob/master/toastinfo.c#L119-L128

Christoph

  2. Re: VARDATA_COMPRESSED_GET_COMPRESS_METHOD comment?

    Robert Haas <robertmhaas@gmail.com> — 2021-09-07T18:13:05Z

    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

  3. Re: VARDATA_COMPRESSED_GET_COMPRESS_METHOD comment?

    Christoph Berg <myon@debian.org> — 2021-09-08T15:33:22Z

    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

  4. Re: VARDATA_COMPRESSED_GET_COMPRESS_METHOD comment?

    Robert Haas <robertmhaas@gmail.com> — 2021-09-08T16:27:58Z

    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

  5. Re: VARDATA_COMPRESSED_GET_COMPRESS_METHOD comment?

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-09-08T18:01:49Z

    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