Re: alter table set TABLE ACCESS METHOD

On Tue, Jun 08, 2021 at 05:33:31PM -0700, Jeff Davis wrote:
> New version attached, with the detoasting code removed. Could use
> another round of validation/cleanup in case I missed something during
> the merge.

This looks rather sane to me, thanks.

    /* Create the transient table that will receive the re-ordered data */
    OIDNewHeap = make_new_heap(tableOid, tableSpace,
+                              accessMethod
It strikes me that we could extend CLUSTER/VACUUM FULL to support this
option, in the same vein as TABLESPACE.  Perhaps that's not something to
implement or have, just wanted to mention it.

+ALTER TABLE heaptable SET ACCESS METHOD heap2;
+explain (analyze, costs off, summary off, timing off) SELECT * FROM heaptable;
+SELECT COUNT(a), COUNT(1) FILTER(WHERE a=1) FROM heaptable;
+DROP TABLE heaptable;
There is a mix of upper and lower-case characters here.  It could be
more consistent.  It seems to me that this test should actually check
that pg_class.relam reflects the new value.

+   /* Save info for Phase 3 to do the real work */
+   if (OidIsValid(tab->newAccessMethod))
+       ereport(ERROR,
+               (errcode(ERRCODE_SYNTAX_ERROR),
+                errmsg("cannot have multiple SET ACCESS METHOD subcommands")));
Worth adding a test?

- * with the specified persistence, which might differ from the original's.
+ * NewTableSpace/accessMethod/persistence, which might differ from those
Nit: the name of the variable looks inconsistent with this comment.
The original is weird as well.

I am wondering if it would be a good idea to set the new tablespace
and new access method fields to InvalidOid within ATGetQueueEntry.  We
do that for the persistence.  Not critical at all, still..

+           pass = AT_PASS_MISC;
Maybe add a comment here?
--
Michael