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

Hello,


ALTER TABLE SET LOGGED/UNLOGED on a partitioned table completes successfully, but nothing changes.  I expected the
underlyingpartitions are changed accordingly.  The attached patch fixes this. 


Regards
Takayuki Tsunakawa

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.

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

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

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 undone
whenthe 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
switchstatement 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

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

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

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

From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
> 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?

I'd like to think that the logged/unlogged property is basically specific to each storage unit, which is a partition
here,and ALTER TABLE on a partitioned table conveniently changes the properties of underlying storage units.  (In that
regard,it's unfortunate that it fails with an ERROR to try to change storage parameters like fillfactor with ALTER
TABLE.)


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

No, I don't see any problem.


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

I was hesitant to add this test in postgres_fdw, but it seems to be a good place considering that postgres_fdw, and
othercontrib modules as well, are part of Postgres core.
 


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

I'd like to add a statement in the description of ALTER TABLE SET LOGGED/UNLOGGED as other ALTER actions do.  I don't
wantto add a new partition section for all CREATE/ALTER actions in this patch.
 

If there's no objection, I think I'll submit the (hopefully final) revised patch after a few days.


Regards
Takayuki Tsunakawa

On Mon, Dec 7, 2020 at 11:36 AM tsunakawa.takay@fujitsu.com
<tsunakawa.takay@fujitsu.com> wrote:
>
> From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
> > 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?
>
> I'd like to think that the logged/unlogged property is basically specific to each storage unit, which is a partition
here,and ALTER TABLE on a partitioned table conveniently changes the properties of underlying storage units.  (In that
regard,it's unfortunate that it fails with an ERROR to try to change storage parameters like fillfactor with ALTER
TABLE.)
>

Do you mean to say that if we detach all the partitions(assuming they
are all unlogged) then the parent table(assuming logged) gets changed
to unlogged? Does it happen on master? Am I missing something here?

>
> > I think we can add foreign partition case into postgres_fdw.sql so
> > that the tests will run as part of make check-world.
>
> I was hesitant to add this test in postgres_fdw, but it seems to be a good place considering that postgres_fdw, and
othercontrib modules as well, are part of Postgres core. 
>

+1 to add tests in postgres_fdw.

>
> > How about documenting all these points in alter_table.sgml,
> > create_table.sgml and create_foreign_table.sgml under the partition
> > section?
>
> I'd like to add a statement in the description of ALTER TABLE SET LOGGED/UNLOGGED as other ALTER actions do.  I don't
wantto add a new partition section for all CREATE/ALTER actions in this patch. 
>

+1.

>
> If there's no objection, I think I'll submit the (hopefully final) revised patch after a few days.
>

Please do so. Thanks.

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

Does "ALTER TABLE ONLY parent" work correctly?  Namely, do not affect
existing partitions, but cause future partitions to acquire the new
setting.

This sounds very much related to previous discussion on REPLICA IDENTITY
not propagating to partitions; see
https://postgr.es/m/201902041630.gpadougzab7v@alvherre.pgsql
particularly Robert Haas' comments at
http://postgr.es/m/CA+TgmoYjksObOzY8b1U1=BsP_m+702eOf42fqRtQTYio1NunbA@mail.gmail.com
and downstream discussion from there.

From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
> Do you mean to say that if we detach all the partitions(assuming they
> are all unlogged) then the parent table(assuming logged) gets changed
> to unlogged? Does it happen on master? Am I missing something here?

No, the parent remains logged in that case both on master and with patched.  I understand this behavior is based on the
ideathat (1) each storage unit (=partition) is independent, and (2) a partitioned table has no storage so the
logged/unloggedsetting has no meaning.  (I don't know there was actually such an idea and the feature was implemented
onthat idea, though.)
 


Regards
Takayuki Tsunakawa

From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> Does "ALTER TABLE ONLY parent" work correctly?  Namely, do not affect
> existing partitions, but cause future partitions to acquire the new
> setting.

Yes, it works correctly in the sense that ALTER TABLE ONLY on a partitioned table does nothing because it has no
storageand therefore logged/unlogged has no meaning. 


> This sounds very much related to previous discussion on REPLICA IDENTITY
> not propagating to partitions; see
> https://postgr.es/m/201902041630.gpadougzab7v@alvherre.pgsql
> particularly Robert Haas' comments at
> http://postgr.es/m/CA+TgmoYjksObOzY8b1U1=BsP_m+702eOf42fqRtQTYio
> 1NunbA@mail.gmail.com
> and downstream discussion from there.

There was a hot discussion.  I've read through it.  I hope I'm not doing strange in light of that discussioin.


Regards
Takayuki Tsunakawa

On 2020-Dec-08, tsunakawa.takay@fujitsu.com wrote:

> From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> > Does "ALTER TABLE ONLY parent" work correctly?  Namely, do not affect
> > existing partitions, but cause future partitions to acquire the new
> > setting.
> 
> Yes, it works correctly in the sense that ALTER TABLE ONLY on a
> partitioned table does nothing because it has no storage and therefore
> logged/unlogged has no meaning.

But what happens when you create another partition after you change the
"loggedness" of the partitioned table?

> > This sounds very much related to previous discussion on REPLICA IDENTITY
> > not propagating to partitions; see
> > https://postgr.es/m/201902041630.gpadougzab7v@alvherre.pgsql
> > particularly Robert Haas' comments at
> > http://postgr.es/m/CA+TgmoYjksObOzY8b1U1=BsP_m+702eOf42fqRtQTYio
> > 1NunbA@mail.gmail.com
> > and downstream discussion from there.
> 
> There was a hot discussion.  I've read through it.  I hope I'm not
> doing strange in light of that discussioin.

Well, my main take from that is we should strive to change all
subcommands together, in some principled way, rather than change only
one or some, in arbitrary ways.

From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> But what happens when you create another partition after you change the
> "loggedness" of the partitioned table?

The new partition will have a property specified when the user creates it.  That is, while the storage property of each
storageunit (=partition) is basically independent, ALTER TABLE on a partitioned table is a convenient way to set the
sameproperty value to all its underlying storage units. 


> Well, my main take from that is we should strive to change all
> subcommands together, in some principled way, rather than change only
> one or some, in arbitrary ways.

I got an impression from the discussion that some form of consensus on the principle was made and you were trying to
createa fix for REPLICA IDENTITY.  Do you think the principle was unclear and we should state it first (partly to
refreshpeople's memories)? 

I'm kind of for it, but I'm hesitant to create the fix for all ALTER actions, because it may take a lot of time and
energyas you were afraid.  Can we define the principle, and then create individual fixes independently based on that
principle?


Regards
Takayuki Tsunakawa

On 2020-Dec-09, tsunakawa.takay@fujitsu.com wrote:

> From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> > But what happens when you create another partition after you change the
> > "loggedness" of the partitioned table?
> 
> The new partition will have a property specified when the user creates
> it.  That is, while the storage property of each storage unit
> (=partition) is basically independent, ALTER TABLE on a partitioned
> table is a convenient way to set the same property value to all its
> underlying storage units.

Well, that definition seems unfriendly to me.  I prefer the stance that
if you change the value for the parent, then future partitions inherit
that value.

> I got an impression from the discussion that some form of consensus on
> the principle was made and you were trying to create a fix for REPLICA
> IDENTITY.  Do you think the principle was unclear and we should state
> it first (partly to refresh people's memories)?

I think the principle was sound -- namely, that we should make all those
commands recurse by default, and for cases where it matters, the
parent's setting should determine the default for future children.

> I'm kind of for it, but I'm hesitant to create the fix for all ALTER
> actions, because it may take a lot of time and energy as you were
> afraid.  Can we define the principle, and then create individual fixes
> independently based on that principle?

That seems acceptable to me, as long as we get all changes in the same
release.  What we don't want (according to my reading of that
discussion) is to change the semantics of a few subcommands in one
release, and the semantics of a few other subcommands in another
release.

On Wed, Dec 9, 2020 at 6:22 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
> On 2020-Dec-09, tsunakawa.takay@fujitsu.com wrote:
>
> > From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> > > But what happens when you create another partition after you change the
> > > "loggedness" of the partitioned table?
> >
> > The new partition will have a property specified when the user creates
> > it.  That is, while the storage property of each storage unit
> > (=partition) is basically independent, ALTER TABLE on a partitioned
> > table is a convenient way to set the same property value to all its
> > underlying storage units.
>
> Well, that definition seems unfriendly to me.  I prefer the stance that
> if you change the value for the parent, then future partitions inherit
> that value.
>
> > I got an impression from the discussion that some form of consensus on
> > the principle was made and you were trying to create a fix for REPLICA
> > IDENTITY.  Do you think the principle was unclear and we should state
> > it first (partly to refresh people's memories)?
>
> I think the principle was sound -- namely, that we should make all those
> commands recurse by default, and for cases where it matters, the
> parent's setting should determine the default for future children.
>
> > I'm kind of for it, but I'm hesitant to create the fix for all ALTER
> > actions, because it may take a lot of time and energy as you were
> > afraid.  Can we define the principle, and then create individual fixes
> > independently based on that principle?
>
> That seems acceptable to me, as long as we get all changes in the same
> release.  What we don't want (according to my reading of that
> discussion) is to change the semantics of a few subcommands in one
> release, and the semantics of a few other subcommands in another
> release.
>

I'm not sure how many more of such commands exist which require changes.

How about doing it this way?

1) Have a separate common thread listing the commands and specifying
clearly the agreed behaviour for such commands.
2) Whenever the separate patches are submitted(hopefully) by the
hackers for such commands, after the review we can make a note of that
patch in the common thread.
3) Once the patches for all the listed commands are
received(hopefully) , then they can be committed together in one
release.

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

On 2020-Dec-09, Bharath Rupireddy wrote:

> I'm not sure how many more of such commands exist which require changes.

The other thread has a list.  I think it is complete, but if you want to
double-check, that would be helpful.

> How about doing it this way?
> 
> 1) Have a separate common thread listing the commands and specifying
> clearly the agreed behaviour for such commands.
> 2) Whenever the separate patches are submitted(hopefully) by the
> hackers for such commands, after the review we can make a note of that
> patch in the common thread.
> 3) Once the patches for all the listed commands are
> received(hopefully) , then they can be committed together in one
> release.

Sounds good.  I think this thread is a good place to collect those
patches, but if you would prefer to have a new thread, feel free to
start one (I'd suggest CC'ing me and Tsunakawa-san).

From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> Well, that definition seems unfriendly to me.  I prefer the stance that
> if you change the value for the parent, then future partitions inherit
> that value.

That would be right when the storage property is an optional specification such as fillfactor.  For example, when I run
ALTERTABLE mytable SET (fillfactor = 70) and then CREATE TABLE mytable_p1 PARTITION OF mytable, I find it nice that the
fillfactoros mytable_p1 is also 70 (but I won't complain if it isn't, since I can run ALTER TABLE SET on the parent
tableagain.) 

OTOH, CREATE TABLE and CREATE UNLOGGED TABLE is an explicit request to create a logged and unlogged relation
respectively. I feel it a strange? if CREATE TABLE mytable_p1 PARTITION OF mytable creates an unlogged partition. 


> > I got an impression from the discussion that some form of consensus on
> > the principle was made and you were trying to create a fix for REPLICA
> > IDENTITY.  Do you think the principle was unclear and we should state
> > it first (partly to refresh people's memories)?
>
> I think the principle was sound -- namely, that we should make all those
> commands recurse by default, and for cases where it matters, the
> parent's setting should determine the default for future children.

Yeah, recurse by default sounded nice.  But I didn't find a consensus related to "parent's setting should determine the
defaultfor future children."  Could you point me there? 


> > I'm kind of for it, but I'm hesitant to create the fix for all ALTER
> > actions, because it may take a lot of time and energy as you were
> > afraid.  Can we define the principle, and then create individual fixes
> > independently based on that principle?
>
> That seems acceptable to me, as long as we get all changes in the same
> release.  What we don't want (according to my reading of that
> discussion) is to change the semantics of a few subcommands in one
> release, and the semantics of a few other subcommands in another
> release.

All fixes at one release seems constricting to me...  Reading from the following quote in the past discussion, I
understoodconsistency is a must and simultaneous release is an ideal.  So, I think we can release individual fixes
separately. I don't think it won't worsen the situation for users at least. 

"try to make them all work the same, ideally in one release, rather than changing them at different times,
back-patchingsometimes, and having no consistency in the details. 

BTW, do you think you can continue to work on your REPLICA IDENTITY patch soon?


Regards
Takayuki Tsunakawa

From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
> I'm not sure how many more of such commands exist which require changes.
> 
> How about doing it this way?
> 
> 1) Have a separate common thread listing the commands and specifying
> clearly the agreed behaviour for such commands.
> 2) Whenever the separate patches are submitted(hopefully) by the
> hackers for such commands, after the review we can make a note of that
> patch in the common thread.
> 3) Once the patches for all the listed commands are
> received(hopefully) , then they can be committed together in one
> release.

That sounds interesting and nice.  Having said that, I rather think it's better to release the fixes separately.  Of
course,we confirm the principle of consistency that individual fixes base on beforehand and what actions need sixing
(SETLOGGED/UNLOGGED was missing in the past thread.)
 


Regards
Takayuki Tsunakawa

On Wed, Dec 09, 2020 at 09:52:17AM -0300, Alvaro Herrera wrote:
> On 2020-Dec-09, tsunakawa.takay@fujitsu.com wrote:
>> The new partition will have a property specified when the user creates
>> it.  That is, while the storage property of each storage unit
>> (=partition) is basically independent, ALTER TABLE on a partitioned
>> table is a convenient way to set the same property value to all its
>> underlying storage units.
>
> Well, that definition seems unfriendly to me.  I prefer the stance that
> if you change the value for the parent, then future partitions inherit
> that value.

That's indeed more interesting from the user perspective.  So +1 from
me.
--
Michael

On Wed, Dec 09, 2020 at 10:56:45AM -0300, Alvaro Herrera wrote:
> Sounds good.  I think this thread is a good place to collect those
> patches, but if you would prefer to have a new thread, feel free to
> start one (I'd suggest CC'ing me and Tsunakawa-san).

There is an entry listed in the CF for this thread:
https://commitfest.postgresql.org/31/2857/

And it seems to me that waiting on author is most appropriate here, so
I have changed the entry to reflect that.
--
Michael

From: Michael Paquier <michael@paquier.xyz>
> On Wed, Dec 09, 2020 at 09:52:17AM -0300, Alvaro Herrera wrote:
> > Well, that definition seems unfriendly to me.  I prefer the stance
> > that if you change the value for the parent, then future partitions
> > inherit that value.
>
> That's indeed more interesting from the user perspective.  So +1 from me.

As I mentioned as below, some properties apply to that, and some don't.

--------------------------------------------------
That would be right when the storage property is an optional specification such as fillfactor.  For example, when I run
ALTERTABLE mytable SET (fillfactor = 70) and then CREATE TABLE mytable_p1 PARTITION OF mytable, I find it nice that the
fillfactoros mytable_p1 is also 70 (but I won't complain if it isn't, since I can run ALTER TABLE SET on the parent
tableagain.) 

OTOH, CREATE TABLE and CREATE UNLOGGED TABLE is an explicit request to create a logged and unlogged relation
respectively. I feel it a strange? if CREATE TABLE mytable_p1 PARTITION OF mytable creates an unlogged partition. 
--------------------------------------------------


Anyway, I think I'll group ALTER TABLE/INDEX altering actions based on some common factors and suggest what would be a
desirablebehavior, asking for opinions.  I'd like to explore the consensus on the basic policy for fixes.  Then, I hope
wewill be able to work on fixes for each ALTER action in patches that can be released separately.  I'd like to regist
requiringall fixes to be arranged at once, since that may become a high bar for those who volunteer to fix some of the
actions. (Even a committer Alvaro-san struggled to fix one action, ALTER TABLE REPLICA IDENTITY.) 


Regards
Takayuki Tsunakawa

At Sat, 26 Dec 2020 01:56:02 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in 
> From: Michael Paquier <michael@paquier.xyz>
> > On Wed, Dec 09, 2020 at 09:52:17AM -0300, Alvaro Herrera wrote:
> > > Well, that definition seems unfriendly to me.  I prefer the stance
> > > that if you change the value for the parent, then future partitions
> > > inherit that value.
> > 
> > That's indeed more interesting from the user perspective.  So +1 from me.
> 
> As I mentioned as below, some properties apply to that, and some don't.
> 
> --------------------------------------------------
> That would be right when the storage property is an optional
> specification such as fillfactor.  For example, when I run ALTER
> TABLE mytable SET (fillfactor = 70) and then CREATE TABLE mytable_p1
> PARTITION OF mytable, I find it nice that the fillfactor os
> mytable_p1 is also 70 (but I won't complain if it isn't, since I can
> run ALTER TABLE SET on the parent table again.)
> 
> OTOH, CREATE TABLE and CREATE UNLOGGED TABLE is an explicit request
> to create a logged and unlogged relation respectively.  I feel it a
> strange? if CREATE TABLE mytable_p1 PARTITION OF mytable creates an
> unlogged partition.
> --------------------------------------------------

"CREATE TABLE" is not "CREATE LOGGED TABLE". We can assume that as
"CREATE <default logged-ness> TABLE", where the default logged-ness
varies according to context. Or it might have been so since the
beginning. Currently we don't have the syntax "CREATE LOGGED TABLE",
but we can add that syntax.

> Anyway, I think I'll group ALTER TABLE/INDEX altering actions based
> on some common factors and suggest what would be a desirable
> behavior, asking for opinions.  I'd like to explore the consensus on
> the basic policy for fixes.  Then, I hope we will be able to work on
> fixes for each ALTER action in patches that can be released
> separately.  I'd like to regist requiring all fixes to be arranged
> at once, since that may become a high bar for those who volunteer to
> fix some of the actions.  (Even a committer Alvaro-san struggled to
> fix one action, ALTER TABLE REPLICA IDENTITY.)

I'd vote +1 to:

 ALTER TABLE/INDEX on a parent recurses to all descendants. ALTER
 TABLE/INDEX ONLY doesn't, and the change takes effect on future
 children.

 We pursue relasing all fixes at once but we might release all fixes
 other than some items that cannot be fixed for some technical reasons
 at the time, like REPLICA IDENITTY.

I'm not sure how long we will wait for the time of release, though.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> "CREATE TABLE" is not "CREATE LOGGED TABLE". We can assume that as
> "CREATE <default logged-ness> TABLE", where the default logged-ness
> varies according to context. Or it might have been so since the beginning.
> Currently we don't have the syntax "CREATE LOGGED TABLE", but we can add
> that syntax.

Yes, I thought of that possibility after a while too.  But I felt a bit(?) hesitant to do it considering back-patching.
Also, the idea requires ALTER TABLE ATTACH PARTITION will have to modify the logged-ness property of the target
partitionand its subpartitions with that of the parent partitioned table.  However, your idea is one candidate worth
pursuing,including whether or not to back-patch what. 


>  We pursue relasing all fixes at once but we might release all fixes  other than
> some items that cannot be fixed for some technical reasons  at the time, like
> REPLICA IDENITTY.
>
> I'm not sure how long we will wait for the time of release, though.

Anyway, we have to define the ideal behavior for each ALTER action based on some comprehensible principle.  Yeah...
thismay become a long, tiring journey.  (I anticipate some difficulty and compromise in reaching agreement, as was seen
inthe past discussion for the fix for ALTER TABLE REPLICA IDENTITY.  Scary) 

FWIW, I wonder why Postgres decided to allow different logical structure of tables such as DEFAULT values and
constraintsbetween the parent partitioned table and a child partition.  That extra flexibility might stand in the way
toconsensus.  I think it'd be easy to understand that partitions are simply physically independent containers that have
identicallogical structure. 


Regards
Takayuki Tsunakawa

At Wed, 27 Jan 2021 05:30:29 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in 
> From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
> > "CREATE TABLE" is not "CREATE LOGGED TABLE". We can assume that as
> > "CREATE <default logged-ness> TABLE", where the default logged-ness
> > varies according to context. Or it might have been so since the beginning.
> > Currently we don't have the syntax "CREATE LOGGED TABLE", but we can add
> > that syntax.
> 
> Yes, I thought of that possibility after a while too.  But I felt a bit(?) hesitant to do it considering
back-patching. Also, the idea requires ALTER TABLE ATTACH PARTITION will have to modify the logged-ness property of the
targetpartition and its subpartitions with that of the parent partitioned table.  However, your idea is one candidate
worthpursuing, including whether or not to back-patch what.
 

I think this and all possible similar changes won't be back-patched at
all.  It is a desaster for any establish version to change this kind
of behavior.

> 
> >  We pursue relasing all fixes at once but we might release all fixes  other than
> > some items that cannot be fixed for some technical reasons  at the time, like
> > REPLICA IDENITTY.
> > 
> > I'm not sure how long we will wait for the time of release, though.
> 
> Anyway, we have to define the ideal behavior for each ALTER action based on some comprehensible principle.  Yeah...
thismay become a long, tiring journey.  (I anticipate some difficulty and compromise in reaching agreement, as was seen
inthe past discussion for the fix for ALTER TABLE REPLICA IDENTITY.  Scary)
 

It seems to me that we have agreed that the ideal behavior for ALTER
TABLE on a parent to propagate to descendants. An observed objection
is that behavior is dangerous for operations that requires large
amount of resources that could lead to failure after elapsing a long
time. The revealed difficulty of REPLICA IDENTITY is regarded as a
technical issue that should be overcome.

> FWIW, I wonder why Postgres decided to allow different logical structure of tables such as DEFAULT values and
constraintsbetween the parent partitioned table and a child partition.  That extra flexibility might stand in the way
toconsensus.  I think it'd be easy to understand that partitions are simply physically independent containers that have
identicallogical structure.
 

I understand that -- once upon a time the "traditional" partitioning
was mere a use case of inheritance tables. Parents and children are
basically independent from each other other than inherited attributes.
The nature of table inheritance compel column addition to a parent to
inherit to children, but other changes need not to be propagated. As
time goes by, partition becomes the major use and new features added
at the time are affected by the recognition. After the appearance of
native partitioning, the table inheritance seems to be regarded as a
heritage that we need to maintain.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On 1/27/21 1:00 AM, Kyotaro Horiguchi wrote:
> At Wed, 27 Jan 2021 05:30:29 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in
>> From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
>>> "CREATE TABLE" is not "CREATE LOGGED TABLE". We can assume that as
>>> "CREATE <default logged-ness> TABLE", where the default logged-ness
>>> varies according to context. Or it might have been so since the beginning.
>>> Currently we don't have the syntax "CREATE LOGGED TABLE", but we can add
>>> that syntax.
>>
>> Yes, I thought of that possibility after a while too.  But I felt a bit(?) hesitant to do it considering
back-patching. Also, the idea requires ALTER TABLE ATTACH PARTITION will have to modify the logged-ness property of the
targetpartition and its subpartitions with that of the parent partitioned table.  However, your idea is one candidate
worthpursuing, including whether or not to back-patch what.
 
> 
> I think this and all possible similar changes won't be back-patched at
> all.  It is a desaster for any establish version to change this kind
> of behavior.
>>
>>>   We pursue relasing all fixes at once but we might release all fixes  other than
>>> some items that cannot be fixed for some technical reasons  at the time, like
>>> REPLICA IDENITTY.
>>>
>>> I'm not sure how long we will wait for the time of release, though.
>>
>> Anyway, we have to define the ideal behavior for each ALTER action based on some comprehensible principle.  Yeah...
thismay become a long, tiring journey.  (I anticipate some difficulty and compromise in reaching agreement, as was seen
inthe past discussion for the fix for ALTER TABLE REPLICA IDENTITY.  Scary)
 
> 
> It seems to me that we have agreed that the ideal behavior for ALTER
> TABLE on a parent to propagate to descendants. An observed objection
> is that behavior is dangerous for operations that requires large
> amount of resources that could lead to failure after elapsing a long
> time. The revealed difficulty of REPLICA IDENTITY is regarded as a
> technical issue that should be overcome.

This thread seems to be stalled. Since it represents a bug fix is there 
something we can do in the meantime while a real solution is worked out?

Perhaps just inform the user that nothing was done? Or get something 
into the documentation?

Regards,
-- 
-David
david@pgmasters.net

On Thu, Mar 25, 2021 at 10:21 PM David Steele <david@pgmasters.net> wrote:
> This thread seems to be stalled. Since it represents a bug fix is there
> something we can do in the meantime while a real solution is worked out?
>
> Perhaps just inform the user that nothing was done? Or get something
> into the documentation?

Attaching a small patch that emits a warning when the persistence
setting of a partitioned table is changed and also added a note into
the docs. Please have a look at it.

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

From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
> Attaching a small patch that emits a warning when the persistence setting of a
> partitioned table is changed and also added a note into the docs. Please have a
> look at it.

Thank you.  However, I think I'll withdraw this CF entry since I'm not sure I can take time for the time being to
researchand fix other various ALTER TABLE/INDEX issues.  I'll mark this as withdrawn around the middle of next week
unlesssomeone wants to continue this.
 


Regards
Takayuki Tsunakawa