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

On Wed, Dec 2, 2020 at 3:57 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Wed, Dec 2, 2020 at 1:52 PM tsunakawa.takay@fujitsu.com
> <tsunakawa.takay@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.
>

Sorry, I missed to add the foreign partition use case, specified as
[1] in my previous mail.

[1]
CREATE TABLE tbl1 (
        a INT,
        b INT,
        c TEXT default 'stuff',
        d TEXT,
        e TEXT
) PARTITION BY LIST (b);

CREATE FOREIGN TABLE foreign_part_tbl1_1(c TEXT, b INT, a INT, e TEXT,
d TEXT) SERVER loopback;
CREATE TABLE part_tbl1_2 (a INT, c TEXT, b INT, d TEXT, e TEXT);

ALTER TABLE tbl1 ATTACH PARTITION foreign_part_tbl1_1 FOR VALUES IN(1);
ALTER TABLE tbl1 ATTACH PARTITION part_tbl1_2 FOR VALUES IN(2);

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