Re: create table like: ACCESS METHOD

On Fri, Aug 27, 2021 at 02:38:43PM +0900, Michael Paquier wrote:
> +CREATE ACCESS METHOD heapdup TYPE TABLE HANDLER heap_tableam_handler;
> +CREATE TABLE likeam() USING heapdup;
> +CREATE TABLE likeamlike(LIKE likeam INCLUDING ALL);
> Rather than creating a custom AM in this test path, I would be
> tempted to move that to create_am.sql.

+ /* ACCESS METHOD doesn't apply and isn't copied for partitioned tables */
+ if ((table_like_clause->options & CREATE_TABLE_LIKE_ACCESS_METHOD) != 0 &&
+ !cxt->ispartitioned)
+ cxt->accessMethod = get_am_name(relation->rd_rel->relam);

If the new table is partitioned, this would work. Now I think that we
should also add here a (relation->rd_rel->relkind == RELKIND_RELATION)
to make sure that we only copy an access method if the original
relation is a table. Note that the original relation could be as well
a view, a foreign table or a composite type.

@@ -349,6 +351,9 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
[...]
+ if (cxt.accessMethod != NULL)
+ stmt->accessMethod = cxt.accessMethod;

This bit is something I have been chewing on a bit. It means that if
we find out an AM to copy from any of the LIKE clauses, we would
blindly overwrite the AM defined in an existing CreateStmt. We could
also argue in favor of keeping the original AM defined by USING from
the query rather than having an error. This means to check that
stmt->accessMethod is overwritten only if NULL at this point. Anyway,
the patch is wrong with this implementation.

This makes me actually wonder if this patch is really a good idea at
the end. The interactions with USING and LIKE would be confusing to
the end-user one way or the other. The argument of upthread regarding
INCLUDING ALL or INCLUDING ACCESS METHOD with multiple original
relations also goes in this sense. If we want to move forward here, I
think that we should really be careful and have a clear definition
behind all those corner cases. The patch fails this point for now.
--
Michael