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

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:

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

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.

-- 
Justin