Re: [bug fix] ALTER TABLE SET LOGGED/UNLOGGED on a partitioned table does nothing silently

On Wed, Dec 2, 2020 at 1:52 PM tsunakawa(dot)takay(at)fujitsu(dot)com
<tsunakawa(dot)takay(at)fujitsu(dot)com> wrote:
>
> ALTER TABLE SET LOGGED/UNLOGED on a partitioned table completes successfully, but nothing changes.
>

Yeah, true.

>
> I expected the underlying partitions are changed accordingly. The attached patch fixes this.
>

My thoughts:
1) What happens if a partitioned table has a foreign partition along
with few other local partitions[1]? Currently, if we try to set
logged/unlogged of a foreign table, then an "ERROR: "XXXX" is not a
table" is thrown. This makes sense. With your patch also we see the
same error, but I'm not quite sure, whether it is setting the parent
and local partitions to logged/unlogged and then throwing the error?
Or do we want the parent relation and the all the local partitions
become logged/unlogged and give a warning saying foreign table can not
be made logged/unlogged?
2) What is the order in which the parent table and the partitions are
made logged/unlogged? Is it that first the parent table and then all
the partitions? What if an error occurs as in above point for a
foreign partition or a partition having foreign key reference to a
logged table? During the rollback of the txn, will we undo the setting
logged/unlogged?
3) Say, I have two logged tables t1 and t2. I altered t1 to be
unlogged, and then I attach logged table t2 as a partition to t1, then
what's the expectation? While attaching the partition, should we also
make t2 as unlogged?
4) Currently, if we try to set logged/unlogged of a foreign table,
then an "ERROR: "XXXX" is not a table" is thrown. I also see that, in
general ATWrongRelkindError() throws an error saying the given
relation is not of expected types, but it doesn't say what is the
given relation kind. Should we improve ATWrongRelkindError() by
passing the actual relation type along with the allowed relation types
to show a bit more informative error message, something like "ERROR:
"XXXX" is a foreign table" with a hint "Allowed relation types are
table, view, index."
5) Coming to the patch, it is missing to add test cases.

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com