Date: Mon, 5 Oct 2020 00:07:13 +0200
From: Tomas Vondra <tomas.vondra@2ndquadrant.com>
To: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>,
	David Steele <david@pgmasters.net>,
	Ildus Kurbangaliev <i.kurbangaliev@gmail.com>,
	Dmitry Dolgov <9erthalion6@gmail.com>,
	PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [HACKERS] Custom compression methods
Message-ID: <20201004220713.6vlmm2e3amlz2dil@development>
References: <20190228164415.62bd4107@ildus-work.localdomain>
 <53ab954a-bf44-2bff-fbe0-29f093899604@pgmasters.net>
 <CAPpHfdsQtby_G0HxPMrDA9G=swhC4o0sbm9hZ35A_uP8uw2TEQ@mail.gmail.com>
 <CA+TgmobSDVgUage9qQ5P_=F_9jaMkCgyKxUQGtFQU7oN4kX-AA@mail.gmail.com>
 <CAFiTN-uUpX3ck=K0mLEk-G_kUQY=SNOTeqdaNRR9FMdQrHKebw@mail.gmail.com>
 <CAFiTN-viudxb0kGwtwsXhSCAaNZ4OUYcP-7PM+tr-rEbgGeHQw@mail.gmail.com>
 <CA+TgmoYYeMv=eJqf2JF8VWDPe=S6rYfH78DOxzE-Sa2kQHhiPw@mail.gmail.com>
 <CAFiTN-vcbfy5ScKVUp16c1N_wzP0RL6EkPBAg_Jm3eDK0ftO5Q@mail.gmail.com>
 <CAFiTN-t+0y5xnPx+sSvverfXPk9E6OMUkdfgMg7mcTTbs8rokQ@mail.gmail.com>
 <CAFiTN-v2_h2+nPv6A-XLrs+E-ms970Xr78wGYv8A9fmDb1P0AQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: 
 <CAFiTN-v2_h2+nPv6A-XLrs+E-ms970Xr78wGYv8A9fmDb1P0AQ@mail.gmail.com>
Precedence: bulk

Hi,

I took a look at this patch after a long time, and done a bit of a
review+testing. I haven't re-read the whole thread since 2017 so some of
the following comments might be mistaken - sorry about that :-(


1) The "cmapi.h" naming seems unnecessarily short. I'd suggest using
simply compression or something like that. I see little reason to
shorten "compression" to "cm", or to prefix files with "cm_". For
example compression/cm_zlib.c might just be compression/zlib.c.


2) I see index_form_tuple does this:

     Datum  cvalue = toast_compress_datum(untoasted_values[i],
                                          DefaultCompressionMethod);

which seems wrong - why shouldn't the indexes use the same compression
method as the underlying table?


3) dumpTableSchema in pg_dump.c does this:

     switch (tbinfo->attcompression[j])
     {
         case 'p':
             cmname = "pglz";
         case 'z':
             cmname = "zlib";
     }

which is broken as it's missing break, so 'p' will produce 'zlib'.


4) The name ExecCompareCompressionMethod is somewhat misleading, as the
functions is not merely comparing compression methods - it also
recompresses the data.


5) CheckCompressionMethodsPreserved should document what the return
value is (true when new list contains all old values, thus not requiring
a rewrite). Maybe "Compare" would be a better name?


6) The new field in ColumnDef is missing a comment.


7) It's not clear to me what "partial list" in the PRESERVE docs means.

+ which of them should be kept on the column. Without PRESERVE or partial
+ list of compression methods the table will be rewritten.


8) The initial synopsis in alter_table.sgml includes the PRESERVE
syntax, but then later in the page it's omitted (yet the section talks
about the keyword).


9) attcompression ...

The main issue I see is what the patch does with attcompression. Instead
of just using it to store a the compression method, it's also used to
store the preserved compression methods. And using NameData to store
this seems wrong too - if we really want to store this info, the correct
way is either using text[] or inventing charvector or similar.

But to me this seems very much like a misuse of attcompression to track
dependencies on compression methods, necessary because we don't have a
separate catalog listing compression methods. If we had that, I think we
could simply add dependencies between attributes and that catalog.

Moreover, having the catalog would allow adding compression methods
(from extensions etc) instead of just having a list of hard-coded
compression methods. Which seems like a strange limitation, considering
this thread is called "custom compression methods".


10) compression parameters?

I wonder if we could/should allow parameters, like compression level
(and maybe other stuff, depending on the compression method). PG13
allowed that for opclasses, so perhaps we should allow it here too.


11) pg_column_compression

When specifying compression method not present in attcompression, we get
this error message and hint:

   test=# alter table t alter COLUMN a set compression "pglz" preserve (zlib);
   ERROR:  "zlib" compression access method cannot be preserved
   HINT:  use "pg_column_compression" function for list of compression methods

but there is no pg_column_compression function, so the hint is wrong.


regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services