Re: Re: [HACKERS] Custom compression methods

On Sat, Sep 19, 2020 at 1:19 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
>
> On Tue, Aug 25, 2020 at 11:20 PM Robert Haas <robertmhaas@gmail.com> wrote:
> >
> > On Mon, Aug 24, 2020 at 2:12 AM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > IIUC,  the main reason for using this flag is for taking the decision
> > > whether we need any detoasting for this tuple.  For example, if we are
> > > rewriting the table because the compression method is changed then if
> > > HEAP_HASCUSTOMCOMPRESSED bit is not set in the tuple header and tuple
> > > length, not tup->t_len > TOAST_TUPLE_THRESHOLD then we don't need to
> > > call heap_toast_insert_or_update function for this tuple.  Whereas if
> > > this flag is set then we need to because we might need to uncompress
> > > and compress back using a different compression method.  The same is
> > > the case with INSERT into SELECT * FROM.
> >
> > This doesn't really seem worth it to me. I don't see how we can
> > justify burning an on-disk bit just to save a little bit of overhead
> > during a rare maintenance operation. If there's a performance problem
> > here we need to look for another way of mitigating it. Slowing CLUSTER
> > and/or VACUUM FULL down by a large amount for this feature would be
> > unacceptable, but is that really a problem? And if so, can we solve it
> > without requiring this bit?
>
> Okay, if we want to avoid keeping the bit then there are multiple ways
> to handle this, but the only thing is none of that will be specific to
> those scenarios.
> approach1. In ExecModifyTable, we can process the source tuple and see
> if any of the varlena attributes is compressed and its stored
> compression method is not the same as the target table attribute then
> we can decompress it.
> approach2. In heap_prepare_insert,  always call the
> heap_toast_insert_or_update, therein we can check if any of the source
> tuple attributes are compressed with different compression methods
> then the target table then we can decompress it.
>
> With either of the approach, we have to do this in a generic path
> because the source of the tuple is not known, I mean it can be a
> output from a function, or the join tuple or a subquery.  So in the
> attached patch, I have implemented with approach1.
>
> For testing, I have implemented using approach1 as well as using
> approach2 and I have checked the performance of the pg_bench to see
> whether it impacts the performance of the generic paths or not, but I
> did not see any impact.
>
> >
> > > I have already extracted these 2 patches from the main patch set.
> > > But, in these patches, I am still storing the am_oid in the toast
> > > header.  I am not sure can we get rid of that at least for these 2
> > > patches?  But, then wherever we try to uncompress the tuple we need to
> > > know the tuple descriptor to get the am_oid but I think that is not
> > > possible in all the cases.  Am I missing something here?
> >
> > I think we should instead use the high bits of the toast size word for
> > patches #1-#4, as discussed upthread.
> >
> > > > > Patch #3. Add support for changing the compression method associated
> > > > > with a column, forcing a table rewrite.
> > > > >
> > > > > Patch #4. Add support for PRESERVE, so that you can change the
> > > > > compression method associated with a column without forcing a table
> > > > > rewrite, by including the old method in the PRESERVE list, or with a
> > > > > rewrite, by not including it in the PRESERVE list.
> > >
> > > Does this make sense to have Patch #3 and Patch #4, without having
> > > Patch #5? I mean why do we need to support rewrite or preserve unless
> > > we have the customer compression methods right? because the build-in
> > > compression method can not be dropped so why do we need to preserve?
> >
> > I think that patch #3 makes sense because somebody might have a table
> > that is currently compressed with pglz and they want to switch to lz4,
> > and I think patch #4 also makes sense because they might want to start
> > using lz4 for future data but not force a rewrite to get rid of all
> > the pglz data they've already got. Those options are valuable as soon
> > as there is more than one possible compression algorithm, even if
> > they're all built in. Now, as I said upthread, it's also true that you
> > could do #5 before #3 and #4. I don't think that's insane. But I
> > prefer it in the other order, because I think having #5 without #3 and
> > #4 wouldn't be too much fun for users.
>
> Details of the attached patch set
>
> 0001: This provides syntax to set the compression method from the
> built-in compression method (pglz or zlib).  pg_attribute stores the
> compression method (char) and there are conversion functions to
> convert that compression method to the built-in compression array
> index.  As discussed up thread the first 2 bits will be storing the
> compression method index using that we can directly get the handler
> routing using the built-in compression method array.
>
> 0002: This patch provides an option to changes the compression method
> for an existing column and it will rewrite the table.
>
> Next, I will be working on providing an option to alter the
> compression method without rewriting the whole table, basically, we
> can provide a preserve list to preserve old compression methods.

I have rebased the patch and I have also done a couple of defect fixes
and some cleanup.

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