Subject: Re: pglz performance
To: Andres Freund <andres@anarazel.de>
Cc: Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 Andrey Borodin <x4mmm@yandex-team.ru>,
 Konstantin Knizhnik <k.knizhnik@postgrespro.ru>,
 Michael Paquier <michael@paquier.xyz>,
 pgsql-hackers <pgsql-hackers@postgresql.org>,
 Vladimir Leskov <vladimirlesk@yandex-team.ru>
References: <20190802170039.o4pabnzm4xy3z7uj@development>
 <20190802171258.nob5zo2vu7siqo2b@alap3.anarazel.de>
 <20190802175239.5ds6rhdso6hwy4vk@development>
 <20190802182003.u477q6itwudieqxf@alap3.anarazel.de>
 <20190802194859.qxzxq2achzottkup@development>
 <d8576096-76ba-487d-515b-44fdedba8bb5@2ndquadrant.com>
 <20190804115237.nliwf6hhaqfd62ps@development>
 <a5cbd707-e4a4-70e7-e970-75f1cc68c20f@2ndquadrant.com>
 <20190804221533.3lpbs5krfnjisflj@alap3.anarazel.de>
 <dc736f7e-7b32-cb41-853c-0b0c46bef9f3@2ndquadrant.com>
 <20190805072625.pzebnrs3fvk5wjen@alap3.anarazel.de>
From: Petr Jelinek <petr@2ndquadrant.com>
Message-ID: <704b2f0a-fef3-cee0-bd55-b5704074f4c3@2ndquadrant.com>
Date: Wed, 25 Sep 2019 23:24:18 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20190805072625.pzebnrs3fvk5wjen@alap3.anarazel.de>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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/