Re: alter table set TABLE ACCESS METHOD

On Sun, Mar 07, 2021 at 07:07:07PM -0600, Justin Pryzby wrote:
> This renames to use SET ACCESS METHOD (resolving a silly typo);
> And handles the tuple slots more directly;
> And adds docs and tab completion;
>
> Also, since 8586bf7ed8889f39a59dd99b292014b73be85342:
> | For now it's not allowed to set a table AM for a partitioned table, as
> | we've not resolved how partitions would inherit that. Disallowing
> | allows us to introduce, if we decide that's the way forward, such a
> | behaviour without a compatibility break.
>
> I propose that it should behave like tablespace for partitioned rels:
> ca4103025dfe, 33e6c34c3267

Sounds sensible from here. Based on the patch at hand, changing the
AM of a partitioned table does nothing for the existing partitions,
and newly-created partitions would inherit the new AM assigned to its
parent. pg_dump is handling things right.

From what I can see, the patch in itself is simple, with central parts
in swap_relation_files() to handle the rewrite and make_new_heap() to
assign the new correct AM.

+ * Since these have no storage the tablespace can be updated with a simple
+ * metadata only operation to update the tablespace.
+ */
+static void
+ATExecSetAccessMethodNoStorage(Relation rel, Oid newAccessMethod
This comment still refers to tablespaces.

+ /*
+ * Record dependency on AM. This is only required for relations
+ * that have no physical storage.
+ */
+ changeDependencyFor(RelationRelationId, RelationGetRelid(rel),
+ AccessMethodRelationId, oldrelam,
+ newAccessMethod);
And? Relations with storage also require this dependency.

- if (tab->newTableSpace)
+ if (OidIsValid(tab->newTableSpace))
You are right, but this is just a noise diff in this patch.

+ swaptemp = relform1->relam;
+ relform1->relam = relform2->relam;
+ relform2->relam = swaptemp;
swap_relation_files() holds the central logic, but I can see that no
comments of this routine have been updated (header, comment when
looking at valid relfilenode{1,2}).

It seems to me that 0002 and 0003 should just be merged together.

+ /* Need to detoast tuples when changing AM XXX: should
check if one AM is heap and one isn't? */
+ if (newrel->rd_rel->relam != oldrel->rd_rel->relam)
+ {
+ HeapTuple htup = toast_build_flattened_tuple(oldTupDesc,
+ oldslot->tts_values,
+ oldslot->tts_isnull);
This toast issue is a kind of interesting one, and it seems rather
right to rely on toast_build_flattened_tuple() to decompress things if
both table AMs support toast with the internals of toast knowing what
kind of compression has been applied to the stored tuple, rather than
have the table AM try to extra a toast tuple by itself. I wonder
whether we should have a table AM API to know what kind of compression
is supported for a given table AMs at hand, because there is no need
to flatten things if both the origin and the target match their
compression algos, which would be on HEAD to make sure that both the
origin and table AMs have toast (relation_toast_am). Your patch,
here, would flatten each tuples as long as the table AMs don't
match. That can be made cheaper in some cases.
--
Michael