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

On Fri, Dec 4, 2020 at 8:22 AM tsunakawa.takay@fujitsu.com
<tsunakawa.takay@fujitsu.com> wrote:
>
> From: Bharath Rupireddy <bharath.rupireddyforpostgres@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
localpartition 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
undonewhen 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
longswitch 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