Re: [HACKERS] Custom compression methods

On Wed, Oct 7, 2020 at 10:26 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Oct 6, 2020 at 10:21 PM Tomas Vondra
> <tomas.vondra@2ndquadrant.com> wrote:
> >
> > On Tue, Oct 06, 2020 at 11:00:55AM +0530, Dilip Kumar wrote:
> > >On Mon, Oct 5, 2020 at 9:34 PM Tomas Vondra
> > ><tomas.vondra@2ndquadrant.com> wrote:
> > >>
> > >> On Mon, Oct 05, 2020 at 07:57:41PM +0530, Dilip Kumar wrote:
> > >> >On Mon, Oct 5, 2020 at 5:53 PM Tomas Vondra
> > >> ><tomas.vondra@2ndquadrant.com> wrote:
> > >> >>
> > >> >> On Mon, Oct 05, 2020 at 11:17:28AM +0530, Dilip Kumar wrote:
> > >> >> >On Mon, Oct 5, 2020 at 3:37 AM Tomas Vondra
> > >> >> ><tomas.vondra@2ndquadrant.com> wrote:
> > >> >> >
> > >> >> >Thanks, Tomas for your feedback.
> > >> >> >
> > >> >> >> 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.
> > >> >> >
> > >> >> >The reason for using the NameData is the get it in the fixed part of
> > >> >> >the data structure.
> > >> >> >
> > >> >>
> > >> >> Why do we need that? It's possible to have varlena fields with direct
> > >> >> access (see pg_index.indkey for example).
> > >> >
> > >> >I see.  While making it NameData I was thinking whether we have an
> > >> >option to direct access the varlena. Thanks for pointing me there. I
> > >> >will change this.
> > >> >
> > >> > Adding NameData just to make
> > >> >> it fixed-length means we're always adding 64B even if we just need a
> > >> >> single byte, which means ~30% overhead for the FormData_pg_attribute.
> > >> >> That seems a bit unnecessary, and might be an issue with many attributes
> > >> >> (e.g. with many temp tables, etc.).
> > >> >
> > >> >You are right.  Even I did not like to keep 64B for this, so I will change it.
> > >> >
> > >> >>
> > >> >> >> 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.
> > >> >> >
> > >> >> >Basically, up to this patch, we are having only built-in compression
> > >> >> >methods and those can not be dropped so we don't need any dependency
> > >> >> >at all.  We just want to know what is the current compression method
> > >> >> >and what is the preserve compression methods supported for this
> > >> >> >attribute.  Maybe we can do it better instead of using the NameData
> > >> >> >but I don't think it makes sense to add a separate catalog?
> > >> >> >
> > >> >>
> > >> >> Sure, I understand what the goal was - all I'm saying is that it looks
> > >> >> very much like a workaround needed because we don't have the catalog.
> > >> >>
> > >> >> I don't quite understand how could we support custom compression methods
> > >> >> without listing them in some sort of catalog?
> > >> >
> > >> >Yeah for supporting custom compression we need some 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".
> > >> >> >
> > >> >> >I think I forgot to mention while submitting the previous patch that
> > >> >> >the next patch I am planning to submit is, Support creating the custom
> > >> >> >compression methods wherein we can use pg_am catalog to insert the new
> > >> >> >compression method.  And for dependency handling, we can create an
> > >> >> >attribute dependency on the pg_am row.  Basically, we will create the
> > >> >> >attribute dependency on the current compression method AM as well as
> > >> >> >on the preserved compression methods AM.   As part of this, we will
> > >> >> >add two build-in AMs for zlib and pglz, and the attcompression field
> > >> >> >will be converted to the oid_vector (first OID will be of the current
> > >> >> >compression method, followed by the preserved compression method's
> > >> >> >oids).
> > >> >> >
> > >> >>
> > >> >> Hmmm, ok. Not sure pg_am is the right place - compression methods don't
> > >> >> quite match what I though AMs are about, but maybe it's just my fault.
> > >> >>
> > >> >> FWIW it seems a bit strange to first do the attcompression magic and
> > >> >> then add the catalog later - I think we should start with the catalog
> > >> >> right away. The advantage is that if we end up committing only some of
> > >> >> the patches in this cycle, we already have all the infrastructure etc.
> > >> >> We can reorder that later, though.
> > >> >
> > >> >Hmm,  yeah we can do this way as well that first create a new catalog
> > >> >table and add entries for these two built-in methods and the
> > >> >attcompression can store the oid vector.  But if we only commit the
> > >> >build-in compression methods part then does it make sense to create an
> > >> >extra catalog or adding these build-in methods to the existing catalog
> > >> >(if we plan to use pg_am).  Then in attcompression instead of using
> > >> >one byte for each preserve compression method, we need to use oid.  So
> > >> >from Robert's mail[1], it appeared to me that he wants that the
> > >> >build-in compression methods part should be independently committable
> > >> >and if we think from that perspective then adding a catalog doesn't
> > >> >make much sense.  But if we are planning to commit the custom method
> > >> >also then it makes more sense to directly start with the catalog
> > >> >because that way it will be easy to expand without much refactoring.
> > >> >
> > >> >[1]
https://www.postgresql.org/message-id/CA%2BTgmobSDVgUage9qQ5P_%3DF_9jaMkCgyKxUQGtFQU7oN4kX-AA%40mail.gmail.com
> > >> >
> > >>
> > >> Hmmm. Maybe I'm missing something subtle, but I think that plan can be
> > >> interpreted in various ways - it does not really say whether the initial
> > >> list of built-in methods should be in some C array, or already in a proper
> > >> catalog.
> > >>
> > >> All I'm saying is it seems a bit weird to first implement dependencies
> > >> based on strange (mis)use of attcompression attribute, and then replace
> > >> it with a proper catalog. My understanding is those patches are expected
> > >> to be committable one by one, but the attcompression approach seems a
> > >> bit too hacky to me - not sure I'd want to commit that ...
> > >
> > >Okay, I will change this.  So I will make create a new catalog
> > >pg_compression and add the entry for two built-in compression methods
> > >from the very first patch.
> > >
> >
> > OK.

I have changed the first 2 patches, basically, now we are providing a
new catalog pg_compression and the pg_attribute is storing the oid of
the compression method.  The patches still need some cleanup and there
is also one open comment that for index we should use its table
compression.

I am still working on the preserve patch.  For preserving the
compression method I am planning to convert the attcompression field
to the oidvector so that we can store the oid of the preserve method
also.  I am not sure whether we can access this oidvector as a fixed
part of the FormData_pg_attribute or not.  The reason is that for
building the tuple descriptor, we need to give the size of the fixed
part (#define ATTRIBUTE_FIXED_PART_SIZE \
(offsetof(FormData_pg_attribute,attcompression) + sizeof(Oid))).  But
if we convert this to the oidvector then we don't know the size of the
fixed part.  Am I missing something?

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com