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

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 think this patch needs to be specifically concerned with pg_upgrade, so I
> suggest to not drop your tables and MVs, to allow the pg_upgrade test to check
> them.  That exposes this issue:

Thanks for the suggestion I will try this.

> pg_dump: error: Error message from server: ERROR:  cache lookup failed for access method 36447
> pg_dump: error: The command was: COPY public.cmdata (f1) TO stdout;
> pg_dumpall: error: pg_dump failed on database "regression", exiting
> waiting for server to shut down.... done
> server stopped
> pg_dumpall of post-upgrade database cluster failed
>
> 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.


> In my brief attempt to inspect it, I got this crash:
>
> $ tmp_install/usr/local/pgsql/bin/postgres -D src/bin/pg_upgrade/tmp_check/data &
> regression=# SELECT pg_column_compression(f1) FROM cmdata a;
> server closed the connection unexpectedly
>
> Thread 1 "postgres" received signal SIGSEGV, Segmentation fault.
> __strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:120
> 120     ../sysdeps/x86_64/multiarch/../strlen.S: No such file or directory.
> (gdb) bt
> #0  __strlen_sse2 () at ../sysdeps/x86_64/multiarch/../strlen.S:120
> #1  0x000055c6049fde62 in cstring_to_text (s=0x0) at varlena.c:193
> #2  pg_column_compression () at varlena.c:5335
>
> (gdb) up
> #2  pg_column_compression () at varlena.c:5335
> 5335            PG_RETURN_TEXT_P(cstring_to_text(get_am_name(
> (gdb) l
> 5333            varvalue = (struct varlena *) DatumGetPointer(value);
> 5334
> 5335            PG_RETURN_TEXT_P(cstring_to_text(get_am_name(
> 5336                                                                    toast_get_compression_oid(varvalue))));
>
> I guess a missing AM here is a "shouldn't happen" case, but I'd prefer it to be
> caught with an elog() (maybe in get_am_name()) or at least an Assert.

Yeah, this makes sense.

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