On Fri, Dec 4, 2020 at 8:22 AM tsunakawa.ta...@fujitsu.com <tsunakawa.ta...@fujitsu.com> wrote: > > From: Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> > > 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? > > Good point, thanks. I think the foreign partitions should be ignored, > otherwise the user would have to ALTER each local partition manually or > detach the foreign partitions temporarily. Done like that. > > > > 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? > > The parent is not changed because it does not have storage. > If some partition has undesirable foreign key relationship, the entire ALTER > command fails. All the effects are undone when the transaction rolls back. > > > > 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? > > The attached partition retains its property. > > > > 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." > > Ah, maybe that's a bit more friendly. But I don't think it's worth bothering > to mess ATWrongRelkindError() with a long switch statement to map a relation > kind to its string representation. Anyway, I'd like it to be a separate > topic. > > > > 5) Coming to the patch, it is missing to add test cases. > > Yes, added in the revised patch. > > I added this to the next CF. >
Thanks! I will review the v2 patch and provide my thoughts. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com