Re: [HACKERS] Custom compression methods

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 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