Re: [HACKERS] Custom compression methods

On Mon, Jan 11, 2021 at 3:40 PM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
>
> On Mon, Jan 11, 2021 at 12:21 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> >
> > On Mon, Jan 11, 2021 at 12:11:54PM +0530, Dilip Kumar wrote:
> > > On Mon, Jan 11, 2021 at 11:00 AM Dilip Kumar <dilipbalaut(at)gmail(dot)com> wrote:
> > > > On Sun, Jan 10, 2021 at 10:59 PM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> > > > >
> > > > > On Mon, Jan 04, 2021 at 04:57:16PM +0530, Dilip Kumar wrote:
> > > > > > On Mon, Jan 4, 2021 at 6:52 AM Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
> > > > > > > And fails pg_upgrade check, apparently losing track of the compression (?)
> > > > > > >
> > > > > > > CREATE TABLE public.cmdata2 (
> > > > > > > - f1 text COMPRESSION lz4
> > > > > > > + f1 text
> > > > > > > );
> > > > > >
> > > > > > I did not get this? pg_upgrade check is passing for me.
> > > > >
> > > > > I realized that this was failing in your v16 patch sent Dec 25.
> > > > > It's passing on current patches because they do "DROP TABLE cmdata2", but
> > > > > that's only masking the error.
> > >
> > > I tested specifically pg_upgrade by removing all the DROP table and MV
> > > and it is passing. I don't see the reason why should it fail. I mean
> > > after the upgrade why COMPRESSION lz4 is missing?
> >
> > How did you test it ?
> >
> > I'm not completely clear how this is intended to work... has it been tested
> > before ? According to the comments, in binary upgrade mode, there's an ALTER
> > which is supposed to SET COMPRESSION, but that's evidently not happening.
>
> I am able to reproduce this issue, If I run pg_dump with
> binary_upgrade mode then I can see the issue (./pg_dump
> --binary-upgrade -d Postgres). Yes you are right that for fixing
> this there should be an ALTER..SET COMPRESSION method.
>
> > > > > I found that's the AM's OID in the old clsuter:
> > > > > regression=# SELECT * FROM pg_am WHERE oid=36447;
> > > > > oid | amname | amhandler | amtype
> > > > > -------+--------+-------------+--------
> > > > > 36447 | pglz2 | pglzhandler | c
> > > > >
> > > > > But in the new cluster, the OID has changed. Since that's written into table
> > > > > data, I think you have to ensure that the compression OIDs are preserved on
> > > > > upgrade:
> > > > >
> > > > > 16755 | pglz2 | pglzhandler | c
> > > >
> > > > Yeah, basically we are storing am oid in the compressed data so Oid
> > > > must be preserved. I will look into this and fix it.
> > >
> > > On further analysis, if we are dumping and restoring then we will
> > > compress the data back while inserting it so why would we need to old
> > > OID. I mean in the new cluster we are inserting data again so it will
> > > be compressed again and now it will store the new OID. Am I missing
> > > something here?
> >
> > I'm referring to pg_upgrade which uses pg_dump, but does *not* re-insert data,
> > but rather recreates catalogs only and then links to the old tables (either
> > with copy, link, or clone). Test with make -C src/bin/pg_upgrade (which is
> > included in make check-world).
>
> Got this as well.
>
> I will fix these two issues and post the updated patch by tomorrow.
>
> Thanks for your findings.

I have fixed this issue in the v18 version, please test and let me
know your thoughts. There is one more issue pending from an upgrade
perspective in v18-0003, basically, for the preserved method we need
to restore the dependency as well. I will work on this part and
shared the next version soon.

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