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