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
signature.asc
Description: PGP signature