On Wed, Dec 2, 2020 at 1:52 PM tsunakawa.ta...@fujitsu.com <tsunakawa.ta...@fujitsu.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
