On Thu, Mar 07, 2024 at 08:02:00PM -0600, Justin Pryzby wrote:
> As you wrote it, you pass the "defaultAccessMethod" bool to
> ATExecSetAccessMethodNoStorage(), which seems odd. Why not just pass
> the target amoid as newAccessMethod ?
+ /*
+ * Check that the table access method exists.
+ * Use the access method, if specified, otherwise (when not specified) 0
+ * for partitioned tables or the configured default AM.
+ */
+ if (amname != NULL)
+ amoid = get_table_am_oid(amname, false);
+ else if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+ amoid = 0;
+ else
+ amoid = get_table_am_oid(default_table_access_method, false);
.. While using this flow to choose the AM oid, that's neater than the
versions I could come up with, pretty cool.
> When I fooled with this on my side, I called it "chgAccessMethod" to
> follow "chgPersistence". I think "is default" isn't the right data
> structure.
>
> Attached a relative patch with my version.
Thanks. I've applied the patch to add the DEFAULT clause for now, to
ease the work on this patch.
> Also: I just realized that instead of adding a bool, we could test
> (tab->rewrite & AT_REWRITE_ACCESS_METHOD) != 0
Hmm. I've considered that, but the boolean style is able to do the
work, while being more consistent, so I'm OK with what you are doing
in your 0002.
> +-- Default and AM set in in clause are the same, relam should be set.
>
> in in?
Oops, fixed.
I have spent more time reviewing the whole and the tests (I didn't see
much value in testing the DEFAULT clause twice for the partitioned
table case and there is a test in d61a6cad6418), tweaked a few
comments and the documentation, did an indentation and a commit
message draft.
How does that look to you? The test coverage and the semantics do
what we want them to do, so that looks rather reasonable here. A
second or even third pair of eyes would not hurt.
--
Michael