Re: [HACKERS] Custom compression methods - Mailing list pgsql-hackers

On Mon, Jan 11, 2021 at 12:21 PM Justin Pryzby <pryzby@telsasoft.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@gmail.com> wrote:
> > > On Sun, Jan 10, 2021 at 10:59 PM Justin Pryzby <pryzby@telsasoft.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@telsasoft.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.


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