Re: ALTER TABLE SET ACCESS METHOD on partitioned tables

On Fri, Mar 01, 2024 at 10:56:50AM +0900, Michael Paquier wrote:
> On Thu, Feb 29, 2024 at 08:51:31AM -0600, Justin Pryzby wrote:
> > On Wed, Feb 28, 2024 at 05:08:49PM +0900, Michael Paquier wrote:
> >> I have implemented that so as we keep the default, historical
> >> behavior: if pg_class.relam is 0 for a partitioned table, use the AM
> >> defined by default_table_access_method.  The patch only adds a path to
> >> switch to a different AM than the GUC when creating a new partition if
> >> and only if a partitioned table has been manipulated with ALTER TABLE
> >> SET ACCESS METHOD to update its AM to something else than the GUC.
> >> Similarly to tablespaces, CREATE TABLE USING is *not* supported for
> >> partitioned tables, same behavior as previously.
> > 
> > This patch allows resetting relam=0 by running ALTER TABLE SET AM to the
> > same value as the GUC.  Maybe it'd be better to have an explicit SET
> > DEFAULT (as in b9424d01 and 4f622503).
> 
> Outside the scope of this patch's thread, this looks like a good idea
> even for tables/matviews.  And the semantics are pretty easy: if DEFAULT
> is specified, just set the access method to NULL in the parser and let
> tablecmds.c go the AM OID lookup in the prep phase if set to NULL.
> See 0001 attached.  This one looks pretty good taken as an independent
> piece.
> 
> When it comes to partitioned tables, there is a still a tricky case:
> what should we do when a user specifies a non-default value in the SET
> ACCESS METHOD clause and it matches default_table_access_method?

I don't think it's tricky - it seems more like a weird hack in the
previous patch version to make AMs behave like tablespaces, despite not
being completely parallel, due to the absence of a pg_default AM.

With the new 001, the hack can go away, and so it should.

> Should the relam be 0 or should we force relam to be the OID of the
> given value given by the query?

You said "force" it to be the user-specified value, but I think that's
not "forcing", it's respecting (but to take the user's desired value,
and conditionally store 0 instead, that could be described as
"forcing.")

> Implementation-wise, forcing the value to 0 is simpler, but I can get
> why it could be confusing as well, because the state of the catalogs
> does not reflect what was provided in the query.

> At the same time, the user has explicitly set the access method to be
> the same as the default, so perhaps 0 makes sense anyway in this case.

I think if the user sets something "explicitly", the catalog should
reflect what they set.  Tablespaces have dattablespace, but AMs don't --
it's a simpler case.

For 001: we don't *need* to support "ALTER SET AM default" for leaf
tables.  It doesn't do anything that's not already possible.  But, if
AMs for partitioned tables are optional rather than required, then seems
to be needed to allow (re)settinng relam=0.

But for partitioned tables, I think it should set relam=0 directly.
Currently it 1) falls through to default_table_am; and 2) detects that
it's the default, so then sets relam to 0.  Since InvalidOid 

On Fri, Mar 01, 2024 at 02:03:48PM +0900, Michael Paquier wrote:
> Fun topic, especially once coupled with the internals of tablecmds.c
> that uses InvalidOid for the new access AM as a special value to work
> as a no-op.

Since InvalidOid is already taken, I guess you might need to introduce a
boolean flag, like set_relam, indicating that the statement has an
ACCESS METHOD clause.

> + * method defined so as their children can inherit it; however, this is handled

so that

> +         * Do nothing: access methods is a setting that partitions can

method (singular), or s/is/are/

On Wed, Feb 28, 2024 at 05:08:49PM +0900, Michael Paquier wrote:
> Similarly to tablespaces, CREATE TABLE USING is *not* supported for
> partitioned tables, same behavior as previously.

Maybe I misunderstood what you're trying to say, but CREATE..TABLESPACE
*is* supported for partitioned tables.  I'm not sure why it wouldn't be
supported to set the AM, too.

In any case, it'd be a bit confusing for the error message to still say:

postgres=# CREATE TABLE a(i int) PARTITION BY RANGE(a) USING heap2;
ERROR:  specifying a table access method is not supported on a partitioned table

-- 
Justin