On 2021-Nov-19, Michael Paquier wrote: > I think that you should keep this block as it is now. The part where > a relkind does not support table AMs and does not require storage > would get uncovered, and this new code would just attempt to create > storage, so it seems to me that the existing code is better when it > comes to prevent mistakes from developers? You could as well use an > else if (RELKIND_INDEX || RELKIND_SEQUENCE) for > RelationCreateStorage(), and fall back to Assert(false) in the else > branch, to get the same result as the original without impacting the > level of safety of the original code.
Hmm, I think the added condition and else would make it safe and clear: + if (RELKIND_HAS_TABLE_AM(rel->rd_rel->relkind)) + table_relation_set_new_filenode(rel, &rel->rd_node, + relpersistence, + relfrozenxid, relminmxid); + else if (RELKIND_HAS_STORAGE(rel->rd_rel->relkind)) + RelationCreateStorage(rel->rd_node, relpersistence); + else + Assert(false); This is the same coding the patch proposed to put in RelationSetNewRelfilenode, which IMO validates the idea. In init_fork(), there's a typo: + * For tables, the AM callback creates both the main and the init fork. should read: + * For tables, the AM callback creates both the main and the init forks. In heap_create_with_catalog, you have + if (!relid) should be + if (!OidIsValid(relid)) Overall, LGTM. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
