Subject: Re: [HACKERS] Custom compression methods
To: Ildus Kurbangaliev <i.kurbangaliev@postgrespro.ru>
References: <20171201194859.le5hvnnrjzhxhm2t@alvherre.pgsql>
 <bc4e4595-24c5-c2eb-323e-9f31e9c98140@2ndquadrant.com>
 <20171206180716.75ba9ba9@postgrespro.ru>
 <CA+TgmobjDLp5xxodzN+TwhuhdQxM3wJw6AaLatDaBgPQm+N+kw@mail.gmail.com>
 <20171211155555.05ddd2fc@postgrespro.ru>
 <CA+Tgmoa7U9Pgc8YV-eRkKAK2s74aL6Yw1Wsj81PTjyrn0Eg99g@mail.gmail.com>
 <CAPpHfdsO0wSy6ZEoOQS5AkxTRXeCzv4uz=S8hOOJNgOxPmJW4A@mail.gmail.com>
 <CA+TgmoYqG2DFHDEM9ixMQA0p9d=_w6pYaa-+Jue4PP56VNSCWQ@mail.gmail.com>
 <CAPpHfduP77qxdETcPRc_FGOVJpeSCwNGoSUgNU3BCHXu57RVMA@mail.gmail.com>
 <CA+TgmoZqJYP+othk0c-r6acjVWWvBoxkWm5eHLUE=8VyezQKKw@mail.gmail.com>
 <20171213151818.75a20259@postgrespro.ru>
 <CA+TgmoZ68LyL0rFPDgC1J-6p9ZQZc2xS4Txd-9661s+w6_MwBg@mail.gmail.com>
 <20171218115431.2b3e29ba@wp.localdomain> <20180115024930.48583c69@hh>
 <0633c6e5-5328-5c5b-708b-c4409e3fd131@postgrespro.ru>
 <20180123160454.131ade09@wp.localdomain>
Cc: Robert Haas <robertmhaas@gmail.com>,
 Alexander Korotkov <a.korotkov@postgrespro.ru>,
 Tomas Vondra <tomas.vondra@2ndquadrant.com>,
 Alvaro Herrera <alvherre@2ndquadrant.com>,
 =?UTF-8?B?0JXQstCz0LXQvdC40Lkg0KjQuNGI0LrQuNC9?= <itparanoia@gmail.com>,
 Andres Freund <andres@anarazel.de>, Oleg Bartunov <obartunov@gmail.com>,
 Craig Ringer <craig@2ndquadrant.com>,
 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
 Chapman Flack <chap@anastigmatix.net>
From: Ildar Musin <i.musin@postgrespro.ru>
Message-ID: <6fd5dea3-8889-c7bb-9df2-79493b2b6ab0@postgrespro.ru>
Date: Thu, 25 Jan 2018 16:03:20 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <20180123160454.131ade09@wp.localdomain>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Ildus,

On 23.01.2018 16:04, Ildus Kurbangaliev wrote:
> On Mon, 22 Jan 2018 23:26:31 +0300
> Ildar Musin <i.musin@postgrespro.ru> wrote:
>
> Thanks for review! Attached new version of the patch. Fixed few bugs,
> added more documentation and rebased to current master.
>
>> You need to rebase to the latest master, there are some conflicts.
>> I've applied it to the three days old master to try it.
>
> Done.
>
>>
>> As I can see the documentation is not yet complete. For example, there
>> is no section for ALTER COLUMN ... SET COMPRESSION in ddl.sgml; and
>> section "Compression Access Method Functions" in compression-am.sgml
>> hasn't been finished.
>
> Not sure about ddl.sgml, it contains more common things, but since
> postgres contains only pglz by default there is not much to show.
>
>>
>> I've implemented an extension [1] to understand the way developer
>> would go to work with new infrastructure. And for me it seems clear.
>> (Except that it took me some effort to wrap my mind around varlena
>> macros but it is probably a different topic).
>>
>> I noticed that you haven't cover 'cmdrop' in the regression tests and
>> I saw the previous discussion about it. Have you considered using
>> event triggers to handle the drop of column compression instead of
>> 'cmdrop' function? This way you would kill two birds with one stone:
>> it still provides sufficient infrastructure to catch those events
>> (and it something postgres already has for different kinds of ddl
>> commands) and it would be easier to test.
>
> I have added support for event triggers for ALTER SET COMPRESSION in
> current version. Event trigger on ALTER can be used to replace cmdrop
> function but it will be far from trivial. There is not easy way to
> understand that's attribute compression is really dropping in the
> command.
>

I've encountered unexpected behavior in command 'CREATE TABLE ... (LIKE 
...)'. It seems that it copies compression settings of the table 
attributes no matter which INCLUDING options are specified. E.g.

create table xxx(id serial, msg text compression pg_lz4);
alter table xxx alter column msg set storage external;
\d+ xxx
                 Table "public.xxx"
  Column |  Type   |  ...  | Storage  | Compression |
--------+---------+  ...  +----------+-------------+
  id     | integer |  ...  | plain    |             |
  msg    | text    |  ...  | external | pg_lz4      |

Now copy the table structure with "INCLUDING ALL":

create table yyy (like xxx including all);
\d+ yyy
                 Table "public.yyy"
  Column |  Type   |  ...  | Storage  | Compression |
--------+---------+  ...  +----------+-------------+
  id     | integer |  ...  | plain    |             |
  msg    | text    |  ...  | external | pg_lz4      |

And now copy without "INCLUDING ALL":

create table zzz (like xxx);
\d+ zzz
                 Table "public.zzz"
  Column |  Type   |  ...  | Storage  | Compression |
--------+---------+  ...  +----------+-------------+
  id     | integer |  ...  | plain    |             |
  msg    | text    |  ...  | extended | pg_lz4      |

As you see, compression option is copied anyway. I suggest adding new 
INCLUDING COMPRESSION option to enable user to explicitly specify 
whether they want or not to copy compression settings.


I found a few phrases in documentation that can be improved. But the 
documentation should be checked by a native speaker.

In compression-am.sgml:
"an compression access method" -> "a compression access method"
"compression method method" -> "compression method"
"compability" -> "compatibility"
Probably "local-backend cached state" would be better to replace with 
"per backend cached state"?
"Useful to store the parsed view of the compression options" -> "It 
could be useful for example to cache compression options"
"and stores result of" -> "and stores the result of"
"Called when CompressionAmOptions is creating." -> "Called when 
<structname>CompressionAmOptions</structname> is being initialized"

"Note that in any system cache invalidation related with 
pg_attr_compression relation the options will be cleaned" -> "Note that 
any <literal>pg_attr_compression</literal> relation invalidation will 
cause all the cached <literal>acstate</literal> options cleared."
"Function used to ..." -> "Function is used to ..."

I think it would be nice to mention custom compression methods in 
storage.sgml. At this moment it only mentions built-in pglz compression.

-- 
Ildar Musin
i.musin@postgrespro.ru