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(dot)takay(at)fujitsu(dot)com
<tsunakawa(dot)takay(at)fujitsu(dot)com> wrote:
>
> 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.
>

Agreed.

>
> > 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.
>

IMHO, we should also change the parent table. Say, I have 2 local
partitions for a logged table, then I alter that table to
unlogged(with your patch, parent table doesn't become unlogged whereas
the partitions will), and I detach all the partitions for some reason.
Now, the user would think that he set the parent table to unlogged but
it didn't happen. So, I think we should also change the parent table's
logged/unlogged property though it may not have data associated with
it when it has all the partitions. Thoughts?

>
> If some partition has undesirable foreign key relationship, the entire ALTER command fails. All the effects are undone when the transaction rolls back.
>

Hmm.

>
> > 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.
>

This may lead to a situation where a partition table has few
partitions as logged and few as unlogged. Will it lead to some
problems? I'm not quite sure though. I feel while attaching a
partition to a table, we have to check whether the parent is
logged/unlogged and set the partition accordingly. While detaching a
partition we do not need to do anything, just retain the existing
state. I read this from the docs "Any indexes created on an unlogged
table are automatically unlogged as well". So, on the similar lines we
also should automatically make partitions logged/unlogged based on the
parent table. We may have to also think of cases where there exists a
multi level parent child relationship i.e. the table to which a
partition is being attached may be a child to another parent table.
Thoughts?

>
> > 5) Coming to the patch, it is missing to add test cases.
>
> Yes, added in the revised patch.
>

I think we can add foreign partition case into postgres_fdw.sql so
that the tests will run as part of make check-world.

How about documenting all these points in alter_table.sgml,
create_table.sgml and create_foreign_table.sgml under the partition
section?

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