Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

On Mon, Apr 24, 2023 at 07:18:54PM -0500, Justin Pryzby wrote:
> On Thu, Mar 30, 2023 at 12:07:58AM -0500, Justin Pryzby wrote:
>> Actually .. I think it'd be a mistake if the relam needed to be
>> interpretted differently for partitioned tables than other relkinds.
>>
>> I updated the patch to allow intermediate partitioned tables to inherit
>> relam from their parent.
>
> Michael ?

Sorry for dropping the subject for so long. I have spent some time
looking at the patch. Here are a few comments.

pg_class.h includes the following:

/*
* Relation kinds that support tablespaces: All relation kinds with storage
* support tablespaces, except that we don't support moving sequences around
* into different tablespaces. Partitioned tables and indexes don't have
* physical storage, but they have a tablespace settings so that their
* children can inherit it.
*/
#define RELKIND_HAS_TABLESPACE(relkind) \
((RELKIND_HAS_STORAGE(relkind) || RELKIND_HAS_PARTITIONS(relkind)) \
&& (relkind) != RELKIND_SEQUENCE)
[...]
/*
* Relation kinds with a table access method (rd_tableam). Although sequences
* use the heap table AM, they are enough of a special case in most uses that
* they are not included here.
*/
#define RELKIND_HAS_TABLE_AM(relkind) \
((relkind) == RELKIND_RELATION || \
(relkind) == RELKIND_TOASTVALUE || \
(relkind) == RELKIND_MATVIEW)

It would look much more consistent with the tablespace case if we
included partitioned tables in this case, but this refers to
rd_tableam for the relcache which we surely don't want to fill for
partitioned tables. I guess that at minimum a comment is in order?
RELKIND_HAS_TABLE_AM() is much more spread than
RELKIND_HAS_TABLESPACE().

* No need to add an explicit dependency for the toast table, as the
* main table depends on it.
*/
- if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE)
+ if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) ||
+ relkind == RELKIND_PARTITIONED_TABLE)

The comment at the top of this code block needs an update.

if (stmt->accessMethod != NULL)
+ accessMethodId = get_table_am_oid(stmt->accessMethod, false);
else if (stmt->partbound &&
+ (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE))
{
+ /*
+ * For partitions, if no access method is specified, default to the AM
+ * of the parent table.
+ */
+ Assert(list_length(inheritOids) == 1);
+ accessMethodId = get_rel_relam(linitial_oid(inheritOids));
+ if (!OidIsValid(accessMethodId))
+ accessMethodId = get_table_am_oid(default_table_access_method, false);
}
+ else if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)
+ accessMethodId = get_table_am_oid(default_table_access_method, false);

This structure seems a bit weird. Could it be cleaner to group the
second and third blocks together? I would imagine:
if (accessMethod != NULL)
{
//Extract the AM defined in the statement
}
else
{
//This is a relkind that can use a default table AM.
if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)
{
if (stmt->partbound)
{
//This is a partition, so look at what its partitioned
//table holds.
}
else
{
//No partition, grab the default.
}
}
}

+ /*
+ * Only do this for partitioned tables, for which this is just a
+ * catalog change. Tables with storage are handled by Phase 3.
+ */
+ if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ ATExecSetAccessMethodNoStorage(rel, tab->newAccessMethod);

Okay, there is a parallel with tablespaces in this logic.

Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog
update even if ALTER TABLE is defined to use the same table AM as what
is currently set. There is no need to update the relation's pg_class
entry in this case. Be careful that InvokeObjectPostAlterHook() still
needs to be checked in this case. Perhaps some tests should be added
in test_oat_hooks.sql? It would be tempted to add a new SQL file for
that.

+ else if (relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ {
+ /* Do nothing: it's a catalog settings for partitions to inherit */
+ }
Actually, we could add an assertion telling that rd_rel->relam will
always be valid.

- if (RELKIND_HAS_TABLE_AM(tbinfo->relkind))
+ if (RELKIND_HAS_TABLE_AM(tbinfo->relkind) ||
+ tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
tableam = tbinfo->amname;
I have spent some time pondering on this particular change, concluding
that it should be OK. It passes my tests, as well.

+-- partition hierarchies
+-- upon ALTER, new children will inherit the new am, whereas the existing
+-- children will remain untouched
CREATE TABLE am_partitioned(x INT, y INT)
PARTITION BY hash (x);
+CREATE TABLE am_partitioned_1 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 0);
+CREATE TABLE am_partitioned_2 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 1);
+ALTER TABLE am_partitioned_1 SET ACCESS METHOD heap2;
ALTER TABLE am_partitioned SET ACCESS METHOD heap2;

Hmm. I think that we should rewrite a bit this test rather than just
adding contents on top of it. There is also an extra test I would be
interesting in here: a partition tree with 2 levels of depth, an ALTER
TABLE SET ACCESS METHOD run at level 1 on a partitioned table, and
some new partitions attached to it to check that the new partitions
inherit from the level 1 partitioned table, not the top-parent.

alter_table.sgml should be updated to explain what happens when SET
ACCESS METHOD is applied on a partitioned table. See for example SET
TABLESPACE that explains what happens to partitions created
afterwards, telling that there is no rewrite in this case.

The regression test added to check pg_dump with a partition tree and
the two table AMs was mixed with an existing one, but it seems to me
that it should be independent of the rest? I have tweaked that as in
the attached, on the way, using one partition that relies on the
default defined by the parent, and a second that has a USING clause on
heap. I did not touch the rest of the code.
--
Michael