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>
 <6fd5dea3-8889-c7bb-9df2-79493b2b6ab0@postgrespro.ru>
 <20180125172457.5967c022@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: <bc430af2-4388-4ad3-acda-f1a95b9310ae@postgrespro.ru>
Date: Fri, 26 Jan 2018 19:07:28 +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: <20180125172457.5967c022@wp.localdomain>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hello Ildus,

I continue reviewing your patch. Here are some thoughts.

1. When I set column storage to EXTERNAL then I cannot set compression.
Seems reasonable:
create table test(id serial, msg text);
alter table test alter column msg set storage external;
alter table test alter column msg set compression pg_lz4;
ERROR:  storage for "msg" should be MAIN or EXTENDED

But if I reorder commands then it's ok:
create table test(id serial, msg text);
alter table test alter column msg set compression pg_lz4;
alter table test alter column msg set storage external;
\d+ test
                 Table "public.test"
  Column |  Type   |  ...  | Storage  | Compression
--------+---------+  ...  +----------+-------------
  id     | integer |  ...  | plain    |
  msg    | text    |  ...  | external | pg_lz4

So we could either allow user to set compression settings even when
storage is EXTERNAL but with warning or prohibit user to set compression
and external storage at the same time. The same thing is with setting 
storage PLAIN.


2. I think TOAST_COMPRESS_SET_RAWSIZE macro could be rewritten like
following to prevent overwriting of higher bits of 'info':

((toast_compress_header *) (ptr))->info = \
	((toast_compress_header *) (ptr))->info & ~RAWSIZEMASK | (len);

It maybe does not matter at the moment since it is only used once, but
it could save some efforts for other developers in future.
In TOAST_COMPRESS_SET_CUSTOM() instead of changing individual bits you
may do something like this:

#define TOAST_COMPRESS_SET_CUSTOM(ptr) \
do { \
	((toast_compress_header *) (ptr))->info = \
		((toast_compress_header *) (ptr))->info & RAWSIZEMASK | ((uint32) 0x02
<< 30) \
} while (0)

Also it would be nice if bit flags were explained and maybe replaced by
a macro.


3. In AlteredTableInfo, BulkInsertStateData and some functions (eg
toast_insert_or_update) there is a hash table used to keep preserved
compression methods list per attribute. I think a simple array of List*
would be sufficient in this case.


4. In optionListToArray() you can use list_qsort() to sort options list 
instead of converting it manually into array and then back to a list.


5. Redundunt #includes:

	In heap.c:
		#include "access/reloptions.h"
	In tsvector.c:
		#include "catalog/pg_type.h"
		#include "common/pg_lzcompress.h"
	In relcache.c:
		#include "utils/datum.h"

6. Just a minor thing: no reason to change formatting in copy.c
-       heap_insert(resultRelInfo->ri_RelationDesc, tuple, mycid,
-                   hi_options, bistate);
+       heap_insert(resultRelInfo->ri_RelationDesc, tuple,
+                   mycid, hi_options, bistate);

7. Also in utility.c the extra new line was added which isn't relevant 
for this patch.

8. In parse_utilcmd.h the 'extern' keyword was removed from 
transformRuleStmt declaration which doesn't make sense in this patch.

9. Comments. Again, they should be read by a native speaker. So just a 
few suggestions:
toast_prepare_varlena() - comment needed
invalidate_amoptions_cache() - comment format doesn't match other 
functions in the file

In htup_details.h:
/* tuple contain custom compressed
  * varlenas */
should be "contains"

-- 
Ildar Musin
i.musin@postgrespro.ru