Date: Sun, 28 Feb 2021 21:20:26 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: pgsql-hackers@postgresql.org, Jacob Champion <pchampion@vmware.com>,
	Heikki Linnakangas <hlinnaka@iki.fi>,
	Ashwin Agrawal <aagrawal@pivotal.io>,
	Ashutosh Sharma <ashu.coek88@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Robert Haas <robertmhaas@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Dilip Kumar <dilipbalaut@gmail.com>
Subject: Re: alter table set TABLE ACCESS METHOD
Message-ID: <20210301032026.GI20769@telsasoft.com>
References: <20210228222530.GD20769@telsasoft.com>
 <YDxOhIGLPEGYINNP@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YDxOhIGLPEGYINNP@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Mon, Mar 01, 2021 at 11:16:36AM +0900, Michael Paquier wrote:
> On Sun, Feb 28, 2021 at 04:25:30PM -0600, Justin Pryzby wrote:
> > I called this "set TABLE access method" rather than just "set access method"
> > for the reasons given on the LIKE thread:
> > https://www.postgresql.org/message-id/20210119210331.GN8560@telsasoft.com
> 
> ALTER TABLE applies to a table (or perhaps a sequence, still..), so
> that sounds a bit weird to me to add again the keyword "TABLE" for
> that.

I don't know if you're following along the toast compression patch -
Alvaro had suggested that instead of making a new catalog just for a handful of
tuples for compression types, to instead store them in pg_am, with a new
am_type='c'.  So I proposed a patch for
| CREATE TABLE .. (LIKE other INCLUDING ACCESS METHOD),
but then decided that it should say INCLUDING *TABLE* ACCESS METHOD, since
otherwise it was somewhat strange that it didn't include the compression access
methods (which have a separate LIKE option).

> > I've tested this with zedstore, but the lack of 2nd, in-core table AM limits
> > testing possibilities.  It also limits at least my own ability to reason about
> > the AM API.
> >
> > For example, I was surprised to hear that toast is a concept that's
> > intended to be applied to AMs other than heap.
> > https://www.postgresql.org/message-id/flat/CA%2BTgmoYTuT4sRtviMLOOO%2B79VnDCpCNyy9rK6UZFb7KEAVt21w%40mail.gmail.com
> 
> What kind of advanced testing do you have in mind?  It sounds pretty
> much enough to me for a basic patch to use the trick with heap2 as
> your patch does.  That would be enough to be sure that the rewrite
> happens and that data is still around.

The issue is that the toast data can be compressed, so it needs to be detoasted
before pushing it to the other AM, which otherwise may not know how to
decompress it.

If it's not detoasted, this works with "COMPRESSION lz4" (since zedstore
happens to know how to decompress it) but that's just an accident, and it fails
with when using pglz.  That's got to do with 2 non-core patches - when core has
only heap, then I don't see how something like this can be exercized.

postgres=# DROP TABLE t; CREATE TABLE t (a TEXT COMPRESSION pglz) USING heap; INSERT INTO t SELECT repeat(a::text,9999) FROM generate_series(1,99)a; ALTER TABLE t SET ACCESS METHOD zedstore; SELECT * FROM t;
DROP TABLE
CREATE TABLE
INSERT 0 99
ALTER TABLE
2021-02-28 20:50:42.653 CST client backend[14958] psql ERROR:  compressed lz4 data is corrupt
2021-02-28 20:50:42.653 CST client backend[14958] psql STATEMENT:  SELECT * FROM t;

-- 
Justin