Re: [proposal] de-TOAST'ing using a iterator

John Naylor <john.naylor@2ndquadrant.com> 于2019年8月19日周一 下午12:55写道：

> init_toast_buffer():
>
> + * Note the constrain buf->position <= buf->limit may be broken
> + * at initialization. Make sure that the constrain is satisfied
> + * when consume chars.
>
> s/constrain/constraint/ (2 times)
> s/consume/consuming/
>
> Also, this comment might be better at the top the whole function?
>

The constraint is broken in the if branch, so I think put this comment in
the branch
is more precise.

The check
> if (iter->buf != iter->fetch_datum_iterator->buf)
> is what we need to do for the compressed case. Could we use this
> directly instead of having a separate state variable iter->compressed,
> with a macro like this?
> #define TOAST_ITER_COMPRESSED(iter) \
>     (iter->buf != iter->fetch_datum_iterator->buf)


 The logic of the macro may be hard to understand, so I think it's ok to
just check the compressed state variable.

+ * If "ctrlc" field in iterator is equal to INVALID_CTRLC, it means that
> + * the field is invalid and need to read the control byte from the
> + * source buffer in the next iteration, see pglz_decompress_iterate().
> + */
> +#define INVALID_CTRLC 8
>
> I think the macro might be better placed in pg_lzcompress.h, and for
> consistency used in pglz_decompress(). Then the comment can be shorter
> and more general. With my additional comment in
> init_detoast_iterator(), hopefully it will be clear to readers.
>

The main role of this macro is to explain the iterator's "ctrlc" state, IMO
it's reasonable to put
the macro and definition of de-TOAST iterator together.

Thanks for your suggestion, I have updated the patch.
-- 
Best regards,
Binguo Bao