On Thu, Jun 09, 2022 at 11:21:58AM -0700, Soumyadeep Chakraborty wrote: > Thanks. Fixed and rebased.
I think that I am OK with the concept of this patch to use a partitioned table's relam as a reference when creating a partition rather than relying on the default GUC, in a way similar to tablespaces. One worry I see is that forcing a recursion on the leaves on ALTER TABLE could silently break partitions where multiple table AMs are used across separate partitions if ALTER TABLE SET ACCESS METHOD is used on one of the parents, though it seems like this is not something I would much worry about as now the command is an error. A second worry is that we would just break existing creation flows that rely on the GUC defining the default AM. This is worth a close lookup at pg_dump to make sure that we do things correctly with this patch in place.. Did you check dump and restore flows with partition trees and --no-table-access-method? Perhaps there should be some regression tests with partitioned tables? + /* + * For partitioned tables, when no access method is specified, we + * default to the parent table's AM. + */ + Assert(list_length(inheritOids) == 1); + /* XXX: should implement get_rel_relam? */ + relid = linitial_oid(inheritOids); + tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid)); + if (!HeapTupleIsValid(tup)) + elog(ERROR, "cache lookup failed for relation %u", relid); Having a wrapper for that could be useful, yes. We don't have any code paths that would directly need that now, from what I can see, though. This patch gives one reason to have one. -- Michael
signature.asc
Description: PGP signature
