ALTER TABLE SET ACCESS METHOD on partitioned tables

Hi,

+       tup = SearchSysCache1(RELOID, ObjectIdGetDatum(relid));
+       accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam;

-   /* look up the access method, verify it is for a table */
-   if (accessMethod != NULL)
-       accessMethodId = get_table_am_oid(accessMethod, false);
+       if (!HeapTupleIsValid(tup))
+           elog(ERROR, "cache lookup failed for relation %u", relid);

Shouldn't the validity of tup be checked before relam field is accessed ?

Cheers

Thanks for copying me.

I didn't look closely yet, but this comment is wrong:

+ * Since these have no storage the tablespace can be updated with a simple                                                                                                                                                        
+ * metadata only operation to update the tablespace.                                                                                                                                                                              

As I see it, AMs are a strong parallel to tablespaces. The default tablespace
is convenient: 1) explicitly specified tablespace; 2) tablespace of parent,
partitioned table; 3) DB tablespace; 4) default_tablespace:
/messages/by-id/20190423222633.GA8364@alvherre.pgsql

It'd be convenient if AMs worked the same way (and a bit odd that they don't).
Note that in v15, pg_dump/restore now allow --no-table-am, an exact parallel to
--no-tablespace.

--
Justin

On Wed, May 18, 2022 at 4:14 PM Justin Pryzby <pryzby@telsasoft.com> wrote:

I didn't look closely yet, but this comment is wrong:

+ * Since these have no storage the tablespace can be updated with a

simple

+ * metadata only operation to update the tablespace.

Good catch. Fixed.

It'd be convenient if AMs worked the same way (and a bit odd that they

don't).

Note that in v15, pg_dump/restore now allow --no-table-am, an exact

parallel to

--no-tablespace.

I agree that ATSET AM should behave in a similar fashion to ATSET
tablespaces.
However, the way that ATSET tablespace currently behaves is not consistent
with
the ONLY clause.

On a given partition root:
ALTER TABLE ONLY am_partitioned SET TABLESPACE ts;
has the same effect as:
ALTER TABLE am_partitioned SET TABLESPACE ts;

We are missing out on the feature to set the AM/tablespace throughout the
partition hierarchy, with one command.

Regards,
Soumyadeep (VMware)

On Wed, May 18, 2022 at 5:49 PM Soumyadeep Chakraborty <
soumyadeep2007@gmail.com> wrote:

On Wed, May 18, 2022 at 4:14 PM Justin Pryzby <pryzby@telsasoft.com>
wrote:

I didn't look closely yet, but this comment is wrong:

+ * Since these have no storage the tablespace can be updated with a

simple

+ * metadata only operation to update the tablespace.

Good catch. Fixed.

It'd be convenient if AMs worked the same way (and a bit odd that they

don't).

Note that in v15, pg_dump/restore now allow --no-table-am, an exact

parallel to

--no-tablespace.

I agree that ATSET AM should behave in a similar fashion to ATSET
tablespaces.
However, the way that ATSET tablespace currently behaves is not consistent
with
the ONLY clause.

On a given partition root:
ALTER TABLE ONLY am_partitioned SET TABLESPACE ts;
has the same effect as:
ALTER TABLE am_partitioned SET TABLESPACE ts;

We are missing out on the feature to set the AM/tablespace throughout the
partition hierarchy, with one command.

Regards,
Soumyadeep (VMware)

Hi,

+ accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam;

-   /* look up the access method, verify it is for a table */
-   if (accessMethod != NULL)
-       accessMethodId = get_table_am_oid(accessMethod, false);
+       if (!HeapTupleIsValid(tup))
+           elog(ERROR, "cache lookup failed for relation %u", relid);

The validity check of tup should be done before fetching the value of
relam field.

Cheers

On Wed, May 18, 2022 at 6:26 PM Zhihong Yu <zyu@yugabyte.com> wrote:

+ accessMethodId = ((Form_pg_class) GETSTRUCT(tup))->relam;

-   /* look up the access method, verify it is for a table */
-   if (accessMethod != NULL)
-       accessMethodId = get_table_am_oid(accessMethod, false);
+       if (!HeapTupleIsValid(tup))
+           elog(ERROR, "cache lookup failed for relation %u", relid);

The validity check of tup should be done before fetching the value of

relam field.

Thanks. Fixed and rebased.

Regards,
Soumyadeep (VMware)

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

On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:

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?

I was looking at the patch, and as I suspected the dumps generated
are forgetting to apply the AM to the partitioned tables. For
example, assuming that the default AM is heap, the patch creates
child_0_10 with heap2 as AM, which is what we want:
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
CREATE TABLE parent_tab (id int) PARTITION BY RANGE (id) USING heap2;
CREATE TABLE child_0_10 PARTITION OF parent_tab
FOR VALUES FROM (0) TO (10);

However a dump produces that (content cut except for its most relevant
parts):
CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
SET default_tablespace = '';
CREATE TABLE public.parent_tab (
id integer
)
PARTITION BY RANGE (id);
SET default_table_access_method = heap2;
CREATE TABLE public.child_0_10 (
id integer
);

This would restore the previous contents incorrectly, where parent_tab
would use heap and child_0_10 would use heap2, causing any partitions
created after the restore to use silently heap. This is going to
require a logic similar to tablespaces, where generate SET commands
on default_table_access_method so as --no-table-access-method in
pg_dump and pg_restore are able to work correctly. Having tests to
cover all that is a must-have.
--
Michael

On Tue, Mar 28, 2023 at 09:13:10AM +0900, Michael Paquier wrote:

On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:

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?

I was looking at the patch, and as I suspected the dumps generated
are forgetting to apply the AM to the partitioned tables.

The patch said:

+ else if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)

pg_dump was missing a similar change that's conditional on
RELKIND_HAS_TABLE_AM(). It looks like those are the only two places
that need be be specially handled.

I dug up my latest patch from 2021 and incorporated the changes from the
0001 patch here, and added a test case.

I realized that one difference with tablespaces is that, as written,
partitioned tables will *always* have an AM specified, and partitions
will never use default_table_access_method. Is that what's intended ?

Or do we need logic similar tablespaces, such that the relam of a
partitioned table is set only if it differs from default_table_am ?

--
Justin

On Mon, Mar 27, 2023 at 11:34:35PM -0500, Justin Pryzby wrote:

I realized that one difference with tablespaces is that, as written,
partitioned tables will *always* have an AM specified, and partitions
will never use default_table_access_method. Is that what's intended ?

Or do we need logic similar tablespaces, such that the relam of a
partitioned table is set only if it differs from default_table_am ?

Hmm. This is a good point. It is true that the patch feels
incomplete on this side. I don't see why we could not be flexible,
and allow a value of 0 in a partitioned table's relam to mean that we
would pick up the database default in this case when a partition is
is created on it. This would have the advantage to be consistent with
older versions where we fallback on the default. We cannot be
completely consistent with the reltablespace of the leaf partitions
unfortunately, as relam should always be set if a relation has
storage. And allowing a value of 0 means that there are likely other
tricky cases with dumps?

Another thing: would it make sense to allow an empty string in
default_table_access_method so as we'd always fallback to a database
default?
--
Michael

On Tue, Mar 28, 2023 at 02:56:28PM +0900, Michael Paquier wrote:

Hmm. This is a good point. It is true that the patch feels
incomplete on this side. I don't see why we could not be flexible,
and allow a value of 0 in a partitioned table's relam to mean that we
would pick up the database default in this case when a partition is
is created on it. This would have the advantage to be consistent with
older versions where we fallback on the default. We cannot be
completely consistent with the reltablespace of the leaf partitions
unfortunately, as relam should always be set if a relation has
storage. And allowing a value of 0 means that there are likely other
tricky cases with dumps?

Another thing: would it make sense to allow an empty string in
default_table_access_method so as we'd always fallback to a database
default?

FYI, I am not sure that I will be able to look more at this patch by
the end of the commit fest, and there are quite more points to
consider. Perhaps at this stage we'd better mark it as returned with
feedback? I understand that I've arrived late at this party :/
--
Michael

On Mon, Mar 27, 2023 at 11:34:36PM -0500, Justin Pryzby wrote:

On Tue, Mar 28, 2023 at 09:13:10AM +0900, Michael Paquier wrote:

On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:

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?

I was looking at the patch, and as I suspected the dumps generated
are forgetting to apply the AM to the partitioned tables.

The patch said:

+ else if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)

pg_dump was missing a similar change that's conditional on
RELKIND_HAS_TABLE_AM(). It looks like those are the only two places
that need be be specially handled.

I dug up my latest patch from 2021 and incorporated the changes from the
0001 patch here, and added a test case.

I realized that one difference with tablespaces is that, as written,
partitioned tables will *always* have an AM specified, and partitions
will never use default_table_access_method. Is that what's intended ?

Or do we need logic similar tablespaces, such that the relam of a
partitioned table is set only if it differs from default_table_am ?

Actually .. I think it'd be a mistake if the relam needed to be
interpretted differently for partitioned tables than other relkinds.

I updated the patch to allow intermediate partitioned tables to inherit
relam from their parent.

Michael wrote:

.. and there are quite more points to consider.

What more points ? There's nothing that's been raised here. In fact,
your message last week is the first comment since last June ..

--
Justin

On Thu, Mar 30, 2023 at 12:07:58AM -0500, Justin Pryzby wrote:

On Mon, Mar 27, 2023 at 11:34:36PM -0500, Justin Pryzby wrote:

On Tue, Mar 28, 2023 at 09:13:10AM +0900, Michael Paquier wrote:

On Mon, Mar 20, 2023 at 09:30:50AM +0900, Michael Paquier wrote:

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?

I was looking at the patch, and as I suspected the dumps generated
are forgetting to apply the AM to the partitioned tables.

The patch said:

+ else if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)

pg_dump was missing a similar change that's conditional on
RELKIND_HAS_TABLE_AM(). It looks like those are the only two places
that need be be specially handled.

I dug up my latest patch from 2021 and incorporated the changes from the
0001 patch here, and added a test case.

I realized that one difference with tablespaces is that, as written,
partitioned tables will *always* have an AM specified, and partitions
will never use default_table_access_method. Is that what's intended ?

Or do we need logic similar tablespaces, such that the relam of a
partitioned table is set only if it differs from default_table_am ?

Actually .. I think it'd be a mistake if the relam needed to be
interpretted differently for partitioned tables than other relkinds.

I updated the patch to allow intermediate partitioned tables to inherit
relam from their parent.

Michael wrote:

.. and there are quite more points to consider.

What more points ? There's nothing that's been raised here. In fact,
your message last week is the first comment since last June ..

Michael ?

On Thu, Mar 30, 2023 at 12:07:58AM -0500, Justin Pryzby wrote:

What more points ? There's nothing that's been raised here. In fact,
your message last week is the first comment since last June ..

When I wrote this message, I felt like this may still be missing
something in the area of dump/restore. Perhaps my feeling on the
matter is wrong, so consider this as a self-reminder not to be taken
seriously until I can have a closer look at what's proposed here for
v17. :p
--
Michael

On Mon, Apr 24, 2023 at 07:18:54PM -0500, Justin Pryzby wrote:

On Thu, Mar 30, 2023 at 12:07:58AM -0500, Justin Pryzby wrote:

Actually .. I think it'd be a mistake if the relam needed to be
interpretted differently for partitioned tables than other relkinds.

I updated the patch to allow intermediate partitioned tables to inherit
relam from their parent.

Michael ?

Sorry for dropping the subject for so long. I have spent some time
looking at the patch. Here are a few comments.

pg_class.h includes the following:

/*
* Relation kinds that support tablespaces: All relation kinds with storage
* support tablespaces, except that we don't support moving sequences around
* into different tablespaces. Partitioned tables and indexes don't have
* physical storage, but they have a tablespace settings so that their
* children can inherit it.
*/
#define RELKIND_HAS_TABLESPACE(relkind) \
((RELKIND_HAS_STORAGE(relkind) || RELKIND_HAS_PARTITIONS(relkind)) \
&& (relkind) != RELKIND_SEQUENCE)
[...]
/*
* Relation kinds with a table access method (rd_tableam). Although sequences
* use the heap table AM, they are enough of a special case in most uses that
* they are not included here.
*/
#define RELKIND_HAS_TABLE_AM(relkind) \
((relkind) == RELKIND_RELATION || \
(relkind) == RELKIND_TOASTVALUE || \
(relkind) == RELKIND_MATVIEW)

It would look much more consistent with the tablespace case if we
included partitioned tables in this case, but this refers to
rd_tableam for the relcache which we surely don't want to fill for
partitioned tables. I guess that at minimum a comment is in order?
RELKIND_HAS_TABLE_AM() is much more spread than
RELKIND_HAS_TABLESPACE().

         * No need to add an explicit dependency for the toast table, as the
         * main table depends on it.
         */
-       if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE)
+       if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) ||
+               relkind == RELKIND_PARTITIONED_TABLE)

The comment at the top of this code block needs an update.

    if (stmt->accessMethod != NULL)
+       accessMethodId = get_table_am_oid(stmt->accessMethod, false);
    else if (stmt->partbound &&
+           (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE))
    {
+       /*
+        * For partitions, if no access method is specified, default to the AM
+        * of the parent table.
+        */
+       Assert(list_length(inheritOids) == 1);
+       accessMethodId = get_rel_relam(linitial_oid(inheritOids));
+       if (!OidIsValid(accessMethodId))
+           accessMethodId = get_table_am_oid(default_table_access_method, false);
    }
+   else if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)
+       accessMethodId = get_table_am_oid(default_table_access_method, false);

This structure seems a bit weird. Could it be cleaner to group the
second and third blocks together? I would imagine:
if (accessMethod != NULL)
{
//Extract the AM defined in the statement
}
else
{
//This is a relkind that can use a default table AM.
if (RELKIND_HAS_TABLE_AM(relkind) || relkind == RELKIND_PARTITIONED_TABLE)
{
if (stmt->partbound)
{
//This is a partition, so look at what its partitioned
//table holds.
}
else
{
//No partition, grab the default.
}
}
}

+           /*
+            * Only do this for partitioned tables, for which this is just a
+            * catalog change.  Tables with storage are handled by Phase 3.
+            */
+           if (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+               ATExecSetAccessMethodNoStorage(rel, tab->newAccessMethod);

Okay, there is a parallel with tablespaces in this logic.

Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog
update even if ALTER TABLE is defined to use the same table AM as what
is currently set. There is no need to update the relation's pg_class
entry in this case. Be careful that InvokeObjectPostAlterHook() still
needs to be checked in this case. Perhaps some tests should be added
in test_oat_hooks.sql? It would be tempted to add a new SQL file for
that.

+   else if (relation->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+   {
+       /* Do nothing: it's a catalog settings for partitions to inherit */
+   }
Actually, we could add an assertion telling that rd_rel->relam will
always be valid.

-       if (RELKIND_HAS_TABLE_AM(tbinfo->relkind))
+       if (RELKIND_HAS_TABLE_AM(tbinfo->relkind) ||
+           tbinfo->relkind == RELKIND_PARTITIONED_TABLE)
            tableam = tbinfo->amname;
I have spent some time pondering on this particular change, concluding
that it should be OK.  It passes my tests, as well.

+-- partition hierarchies
+-- upon ALTER, new children will inherit the new am, whereas the existing
+-- children will remain untouched
 CREATE TABLE am_partitioned(x INT, y INT)
   PARTITION BY hash (x);
+CREATE TABLE am_partitioned_1 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 0);
+CREATE TABLE am_partitioned_2 PARTITION OF am_partitioned FOR VALUES WITH (MODULUS 3,REMAINDER 1);
+ALTER TABLE am_partitioned_1 SET ACCESS METHOD heap2;
 ALTER TABLE am_partitioned SET ACCESS METHOD heap2;

Hmm. I think that we should rewrite a bit this test rather than just
adding contents on top of it. There is also an extra test I would be
interesting in here: a partition tree with 2 levels of depth, an ALTER
TABLE SET ACCESS METHOD run at level 1 on a partitioned table, and
some new partitions attached to it to check that the new partitions
inherit from the level 1 partitioned table, not the top-parent.

alter_table.sgml should be updated to explain what happens when SET
ACCESS METHOD is applied on a partitioned table. See for example SET
TABLESPACE that explains what happens to partitions created
afterwards, telling that there is no rewrite in this case.

The regression test added to check pg_dump with a partition tree and
the two table AMs was mixed with an existing one, but it seems to me
that it should be independent of the rest? I have tweaked that as in
the attached, on the way, using one partition that relies on the
default defined by the parent, and a second that has a USING clause on
heap. I did not touch the rest of the code.
--
Michael

On Thu, May 25, 2023 at 03:49:12PM +0900, Michael Paquier wrote:

looking at the patch. Here are a few comments.

...

* No need to add an explicit dependency for the toast table, as the
* main table depends on it.
*/
-       if (RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE)
+       if ((RELKIND_HAS_TABLE_AM(relkind) && relkind != RELKIND_TOASTVALUE) ||
+               relkind == RELKIND_PARTITIONED_TABLE)

The comment at the top of this code block needs an update.

What do you think the comment ought to say ? It already says:

src/backend/catalog/heap.c- * Make a dependency link to force the relation to be deleted if its
src/backend/catalog/heap.c- * access method is.

Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog
update even if ALTER TABLE is defined to use the same table AM as what
is currently set. There is no need to update the relation's pg_class
entry in this case. Be careful that InvokeObjectPostAlterHook() still
needs to be checked in this case. Perhaps some tests should be added
in test_oat_hooks.sql? It would be tempted to add a new SQL file for
that.

Are you suggesting to put this in a conditional: if oldrelam!=newAccessMethod ?

+       ((Form_pg_class) GETSTRUCT(tuple))->relam = newAccessMethod;
+       CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+       /* Update dependency on new AM */
+       changeDependencyFor(RelationRelationId, relid, AccessMethodRelationId,
+                                               oldrelam, newAccessMethod);

Why is that desirable ? I'd prefer to see it written with fewer
conditionals, not more; then, it hits the same path every time.

This ought to address your other comments.

--
Justin

On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote:

What do you think the comment ought to say ? It already says:

src/backend/catalog/heap.c- * Make a dependency link to force the relation to be deleted if its
src/backend/catalog/heap.c- * access method is.

This is the third location where we rely on the fact that
RELKIND_HAS_TABLE_AM() does not include RELKIND_PARTITIONED_TABLE, so
it seems worth documenting what we are relying on as a comment? Say:
* Make a dependency link to force the relation to be deleted if its
* access method is.
*
* No need to add an explicit dependency for the toast table, as the
* main table depends on it. Partitioned tables have a table access
* method defined, and RELKIND_HAS_TABLE_AM ignores them.

Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog
update even if ALTER TABLE is defined to use the same table AM as what
is currently set. There is no need to update the relation's pg_class
entry in this case. Be careful that InvokeObjectPostAlterHook() still
needs to be checked in this case. Perhaps some tests should be added
in test_oat_hooks.sql? It would be tempted to add a new SQL file for
that.

Are you suggesting to put this in a conditional: if oldrelam!=newAccessMethod ?

Yes, that's what I would add with a few lines close to the beginning
of ATExecSetTableSpaceNoStorage() to save from catalog updates if
these are not needed.
--
Michael

Have you had a chance to address the comments raised by Michael in his last
review such that a new patch revision can be submitted?

--
Daniel Gustafsson

On Thu, Jun 01, 2023 at 08:50:50AM -0400, Michael Paquier wrote:

Speaking of which, ATExecSetAccessMethodNoStorage() does a catalog
update even if ALTER TABLE is defined to use the same table AM as what
is currently set. There is no need to update the relation's pg_class
entry in this case. Be careful that InvokeObjectPostAlterHook() still
needs to be checked in this case. Perhaps some tests should be added
in test_oat_hooks.sql? It would be tempted to add a new SQL file for
that.

Are you suggesting to put this in a conditional: if oldrelam!=newAccessMethod ?

Yes, that's what I would add with a few lines close to the beginning
of ATExecSetTableSpaceNoStorage() to save from catalog updates if
these are not needed.

I understand that it's possible for it to be conditional, but I don't
undertand why it's desirable/important ?

It still seems preferable to be unconditional.

On Wed, May 31, 2023 at 06:35:34PM -0500, Justin Pryzby wrote:

Why is that desirable ? I'd prefer to see it written with fewer
conditionals, not more; then, it hits the same path every time.

It's not conditional in the tablespace code that this follows, nor
others like ATExecSetStatistics(), ATExecForceNoForceRowSecurity(),
ATExecSetCompression(), etc, some of which are recently-added. If it's
important to do here, isn't it also important to do everywhere else ?

--
Justin

On Sun, Jul 16, 2023 at 09:37:28PM -0500, Justin Pryzby wrote:

I understand that it's possible for it to be conditional, but I don't
undertand why it's desirable/important ?

Because it's cheaper on repeated commands, like no CCI necessary.

It's not conditional in the tablespace code that this follows, nor
others like ATExecSetStatistics(), ATExecForceNoForceRowSecurity(),
ATExecSetCompression(), etc, some of which are recently-added. If it's
important to do here, isn't it also important to do everywhere else ?

Good point here. I am OK to discard this point.
--
Michael