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

From: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)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.

Regards
Takayuki Tsunakawa