Re: pglz performance

Hi,

On 05/08/2019 09:26, Andres Freund wrote:
> Hi,
>
> On 2019-08-05 00:19:28 +0200, Petr Jelinek wrote:
>> It carries that information inside the compressed value, like I said in the
>> other reply, that's why the extra byte.
>
> I'm not convinced that that is a good plan - imo the reference to the
> compressed data should carry that information.
>
> I.e. in the case of toast, at least toast pointers should hold enough
> information to determine the compression algorithm. And in the case of
> WAL, the WAL record should contain that.
>
Point taken.

>
> For external datums I suggest encoding the compression method as a
> distinct VARTAG_ONDISK_COMPRESSED, and then have that include the
> compression method as a field.

So the new reads/writes will use this and reads of old format won't
change? Sounds fine.

>
> For in-line compressed values (so VARATT_IS_4B_C), doing something
> roughly like you did, indicating the type of metadata following using
> the high bit sounds reasonable. But I think I'd make it so that if the
> highbit is set, the struct is instead entirely different, keeping a full
> 4 byte byte length, and including the compression type header inside the
> struct. Perhaps I'd combine the compression type with the high-bit-set
> part? So when the high bit is set, it'd be something like
>
> {
> int32 vl_len_; /* varlena header (do not touch directly!) */
>
> /*
> * Actually only 7 bit, the high bit determines whether this
> * is the old compression header (if unset), or this type of header
> * (if set).
> */
> uint8 type;
>
> /*
> * Stored as uint8[4], to avoid unnecessary alignment padding.
> */
> uint8[4] length;
>
> char va_data[FLEXIBLE_ARRAY_MEMBER];
> }
>

Won't this break BW compatibility on big-endian (if I understand
corretly what you are trying to achieve here)?

> I think it's worth spending some effort trying to get this right - we'll
> be bound by design choices for a while.
>

Sure, however I am not in business of redesigning TOAST from scratch
right now, even if I do agree that the current header is far from ideal.

--
Petr Jelinek
2ndQuadrant - PostgreSQL Solutions for the Enterprise
https://www.2ndQuadrant.com/