Обсуждение: MERGE/SPLIT partition commands should create new partitions in the parent's tablespace?

Hi,

I noticed that ALTER TABLE MERGE PARTITIONS and SPLIT PARTITION commands
always create new partitions in the default tablespace, regardless of
the parent's tablespace. However, the indexes of these new partitions inherit
the tablespaces of their parent indexes. This inconsistency seems odd.
Is this an oversight or intentional?

Here are the steps I used to test this:

-------------------------------------------------------
CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
CREATE TABLE t (i int PRIMARY KEY USING INDEX TABLESPACE tblspc)
   PARTITION BY RANGE (i) TABLESPACE tblspc;

CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);

ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;

SELECT tablename, tablespace FROM pg_tables WHERE tablename IN ('t', 'tp_0_2') ORDER BY tablename;
  tablename | tablespace
-----------+------------
  t         | tblspc
  tp_0_2    | (null)
(2 rows)

SELECT indexname, tablespace FROM pg_indexes WHERE tablename IN ('t', 'tp_0_2') ORDER BY indexname;
   indexname  | tablespace
-------------+------------
  t_pkey      | tblspc
  tp_0_2_pkey | tblspc
-------------------------------------------------------


If it's an oversight, I've attached a patch to ensure these commands create
new partitions in the parent's tablespace.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Sat, Jul 6, 2024 at 4:06 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> Hi,
>
> I noticed that ALTER TABLE MERGE PARTITIONS and SPLIT PARTITION commands
> always create new partitions in the default tablespace, regardless of
> the parent's tablespace. However, the indexes of these new partitions inherit
> the tablespaces of their parent indexes. This inconsistency seems odd.
> Is this an oversight or intentional?
>
> Here are the steps I used to test this:
>
> -------------------------------------------------------
> CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
> CREATE TABLE t (i int PRIMARY KEY USING INDEX TABLESPACE tblspc)
>    PARTITION BY RANGE (i) TABLESPACE tblspc;
>
> CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
> CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
>
> ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;
>
> SELECT tablename, tablespace FROM pg_tables WHERE tablename IN ('t', 'tp_0_2') ORDER BY tablename;
>   tablename | tablespace
> -----------+------------
>   t         | tblspc
>   tp_0_2    | (null)
> (2 rows)
>
> SELECT indexname, tablespace FROM pg_indexes WHERE tablename IN ('t', 'tp_0_2') ORDER BY indexname;
>    indexname  | tablespace
> -------------+------------
>   t_pkey      | tblspc
>   tp_0_2_pkey | tblspc
> -------------------------------------------------------
>
>
> If it's an oversight, I've attached a patch to ensure these commands create
> new partitions in the parent's tablespace.

+1

Since creating a child table through the CREATE TABLE statement sets
its parent table's tablespace as the child table's tablespace, it is
logical to set the parent table's tablespace as the merged table's
tablespace.

While the patch does not include test cases for SPLIT PARTITIONS,
which is understandable as these commands use the common function that
we have fixed, I believe it would be prudent to test SPLIT PARTITIONS
as well since we could change it in the future development.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On 2024/07/10 12:13, Masahiko Sawada wrote:
> On Sat, Jul 6, 2024 at 4:06 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>> Hi,
>>
>> I noticed that ALTER TABLE MERGE PARTITIONS and SPLIT PARTITION commands
>> always create new partitions in the default tablespace, regardless of
>> the parent's tablespace. However, the indexes of these new partitions inherit
>> the tablespaces of their parent indexes. This inconsistency seems odd.
>> Is this an oversight or intentional?
>>
>> Here are the steps I used to test this:
>>
>> -------------------------------------------------------
>> CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
>> CREATE TABLE t (i int PRIMARY KEY USING INDEX TABLESPACE tblspc)
>>     PARTITION BY RANGE (i) TABLESPACE tblspc;
>>
>> CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
>> CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
>>
>> ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;
>>
>> SELECT tablename, tablespace FROM pg_tables WHERE tablename IN ('t', 'tp_0_2') ORDER BY tablename;
>>    tablename | tablespace
>> -----------+------------
>>    t         | tblspc
>>    tp_0_2    | (null)
>> (2 rows)
>>
>> SELECT indexname, tablespace FROM pg_indexes WHERE tablename IN ('t', 'tp_0_2') ORDER BY indexname;
>>     indexname  | tablespace
>> -------------+------------
>>    t_pkey      | tblspc
>>    tp_0_2_pkey | tblspc
>> -------------------------------------------------------
>>
>>
>> If it's an oversight, I've attached a patch to ensure these commands create
>> new partitions in the parent's tablespace.
> 
> +1
> 
> Since creating a child table through the CREATE TABLE statement sets
> its parent table's tablespace as the child table's tablespace, it is
> logical to set the parent table's tablespace as the merged table's
> tablespace.

Thanks for the review!


> While the patch does not include test cases for SPLIT PARTITIONS,
> which is understandable as these commands use the common function that
> we have fixed, I believe it would be prudent to test SPLIT PARTITIONS
> as well since we could change it in the future development.

Unless I'm mistaken, the patch already includes tests for the split case.
Could you please check the tests added to partition_split.sql?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Jul 10, 2024 at 4:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2024/07/10 12:13, Masahiko Sawada wrote:
> > On Sat, Jul 6, 2024 at 4:06 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >>
> >> Hi,
> >>
> >> I noticed that ALTER TABLE MERGE PARTITIONS and SPLIT PARTITION commands
> >> always create new partitions in the default tablespace, regardless of
> >> the parent's tablespace. However, the indexes of these new partitions inherit
> >> the tablespaces of their parent indexes. This inconsistency seems odd.
> >> Is this an oversight or intentional?
> >>
> >> Here are the steps I used to test this:
> >>
> >> -------------------------------------------------------
> >> CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
> >> CREATE TABLE t (i int PRIMARY KEY USING INDEX TABLESPACE tblspc)
> >>     PARTITION BY RANGE (i) TABLESPACE tblspc;
> >>
> >> CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
> >> CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
> >>
> >> ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;
> >>
> >> SELECT tablename, tablespace FROM pg_tables WHERE tablename IN ('t', 'tp_0_2') ORDER BY tablename;
> >>    tablename | tablespace
> >> -----------+------------
> >>    t         | tblspc
> >>    tp_0_2    | (null)
> >> (2 rows)
> >>
> >> SELECT indexname, tablespace FROM pg_indexes WHERE tablename IN ('t', 'tp_0_2') ORDER BY indexname;
> >>     indexname  | tablespace
> >> -------------+------------
> >>    t_pkey      | tblspc
> >>    tp_0_2_pkey | tblspc
> >> -------------------------------------------------------
> >>
> >>
> >> If it's an oversight, I've attached a patch to ensure these commands create
> >> new partitions in the parent's tablespace.
> >
> > +1
> >
> > Since creating a child table through the CREATE TABLE statement sets
> > its parent table's tablespace as the child table's tablespace, it is
> > logical to set the parent table's tablespace as the merged table's
> > tablespace.
>
> Thanks for the review!
>
>
> > While the patch does not include test cases for SPLIT PARTITIONS,
> > which is understandable as these commands use the common function that
> > we have fixed, I believe it would be prudent to test SPLIT PARTITIONS
> > as well since we could change it in the future development.
>
> Unless I'm mistaken, the patch already includes tests for the split case.
> Could you please check the tests added to partition_split.sql?
>

Oops, sorry, I missed that part for some reason.So the patch looks good to me.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On Sat, 6 Jul 2024 at 19:06, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> I noticed that ALTER TABLE MERGE PARTITIONS and SPLIT PARTITION commands
> always create new partitions in the default tablespace, regardless of
> the parent's tablespace. However, the indexes of these new partitions inherit
> the tablespaces of their parent indexes. This inconsistency seems odd.
> Is this an oversight or intentional?

My expectation of this feature is that the tablespace choice would
work the same as what was done in ca4103025 to make it inherit from
the partition table's tablespace. I imagine we might get complaints if
it does not follow the same logic.

I've not looked at your patch, but if the behaviour is as you describe
and the patch changes that to follow ca4103025, then +1 from me.

David

On Wed, Jul 10, 2024 at 5:14 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Jul 10, 2024 at 4:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >
> >
> >
> > On 2024/07/10 12:13, Masahiko Sawada wrote:
> > > On Sat, Jul 6, 2024 at 4:06 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > >>
> > >> Hi,
> > >>
> > >> I noticed that ALTER TABLE MERGE PARTITIONS and SPLIT PARTITION commands
> > >> always create new partitions in the default tablespace, regardless of
> > >> the parent's tablespace. However, the indexes of these new partitions inherit
> > >> the tablespaces of their parent indexes. This inconsistency seems odd.
> > >> Is this an oversight or intentional?
> > >>
> > >> Here are the steps I used to test this:
> > >>
> > >> -------------------------------------------------------
> > >> CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
> > >> CREATE TABLE t (i int PRIMARY KEY USING INDEX TABLESPACE tblspc)
> > >>     PARTITION BY RANGE (i) TABLESPACE tblspc;
> > >>
> > >> CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
> > >> CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
> > >>
> > >> ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;
> > >>
> > >> SELECT tablename, tablespace FROM pg_tables WHERE tablename IN ('t', 'tp_0_2') ORDER BY tablename;
> > >>    tablename | tablespace
> > >> -----------+------------
> > >>    t         | tblspc
> > >>    tp_0_2    | (null)
> > >> (2 rows)
> > >>
> > >> SELECT indexname, tablespace FROM pg_indexes WHERE tablename IN ('t', 'tp_0_2') ORDER BY indexname;
> > >>     indexname  | tablespace
> > >> -------------+------------
> > >>    t_pkey      | tblspc
> > >>    tp_0_2_pkey | tblspc
> > >> -------------------------------------------------------
> > >>
> > >>
> > >> If it's an oversight, I've attached a patch to ensure these commands create
> > >> new partitions in the parent's tablespace.
> > >
> > > +1
> > >
> > > Since creating a child table through the CREATE TABLE statement sets
> > > its parent table's tablespace as the child table's tablespace, it is
> > > logical to set the parent table's tablespace as the merged table's
> > > tablespace.

One expectation I had for MERGE PARTITION was that if all partition
tables to be merged are in the same tablespace, the merged table is
also created in the same tablespace. But it would be an exceptional
case in a sense, and I agree with the proposed behavior as it's
consistent. It might be a good idea that we can specify the tablespace
for each merged/split table in the future.

BTW the new regression tests don't check the table and index names.
Isn't it better to show table and index names for better
diagnosability?

+-- Check the new partition inherits parent's tablespace
+CREATE TABLE t (i int PRIMARY KEY USING INDEX TABLESPACE regress_tblspace)
+  PARTITION BY RANGE (i) TABLESPACE regress_tblspace;
+CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
+CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
+ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;
+SELECT tablespace FROM pg_tables WHERE tablename IN ('t', 'tp_0_2')
ORDER BY tablespace;
+    tablespace
+------------------
+ regress_tblspace
+ regress_tblspace
+(2 rows)
+
+SELECT tablespace FROM pg_indexes WHERE tablename IN ('t', 'tp_0_2')
ORDER BY tablespace;
+    tablespace
+------------------
+ regress_tblspace
+ regress_tblspace
+(2 rows)
+
+DROP TABLE t;


Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On 2024/07/10 22:35, Masahiko Sawada wrote:
> BTW the new regression tests don't check the table and index names.
> Isn't it better to show table and index names for better
> diagnosability?

Sounds good to me. I've updated the patch as suggested.
Please see the attached patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On Wed, Jul 10, 2024 at 9:36 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Jul 10, 2024 at 5:14 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > On Wed, Jul 10, 2024 at 4:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > >
> > >
> > >
> > > On 2024/07/10 12:13, Masahiko Sawada wrote:
> > > > On Sat, Jul 6, 2024 at 4:06 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > > >>
> > > >> Hi,
> > > >>
> > > >> I noticed that ALTER TABLE MERGE PARTITIONS and SPLIT PARTITION commands
> > > >> always create new partitions in the default tablespace, regardless of
> > > >> the parent's tablespace. However, the indexes of these new partitions inherit
> > > >> the tablespaces of their parent indexes. This inconsistency seems odd.
> > > >> Is this an oversight or intentional?
> > > >>
> > > >> Here are the steps I used to test this:
> > > >>
> > > >> -------------------------------------------------------
> > > >> CREATE TABLESPACE tblspc LOCATION '/tmp/tblspc';
> > > >> CREATE TABLE t (i int PRIMARY KEY USING INDEX TABLESPACE tblspc)
> > > >>     PARTITION BY RANGE (i) TABLESPACE tblspc;
> > > >>
> > > >> CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
> > > >> CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
> > > >>
> > > >> ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;
> > > >>
> > > >> SELECT tablename, tablespace FROM pg_tables WHERE tablename IN ('t', 'tp_0_2') ORDER BY tablename;
> > > >>    tablename | tablespace
> > > >> -----------+------------
> > > >>    t         | tblspc
> > > >>    tp_0_2    | (null)
> > > >> (2 rows)
> > > >>
> > > >> SELECT indexname, tablespace FROM pg_indexes WHERE tablename IN ('t', 'tp_0_2') ORDER BY indexname;
> > > >>     indexname  | tablespace
> > > >> -------------+------------
> > > >>    t_pkey      | tblspc
> > > >>    tp_0_2_pkey | tblspc
> > > >> -------------------------------------------------------
> > > >>
> > > >>
> > > >> If it's an oversight, I've attached a patch to ensure these commands create
> > > >> new partitions in the parent's tablespace.
> > > >
> > > > +1
> > > >
> > > > Since creating a child table through the CREATE TABLE statement sets
> > > > its parent table's tablespace as the child table's tablespace, it is
> > > > logical to set the parent table's tablespace as the merged table's
> > > > tablespace.
>
> One expectation I had for MERGE PARTITION was that if all partition
> tables to be merged are in the same tablespace, the merged table is
> also created in the same tablespace. But it would be an exceptional
> case in a sense, and I agree with the proposed behavior as it's
> consistent. It might be a good idea that we can specify the tablespace
> for each merged/split table in the future.

I agree this is a good idea, so I tried to support this feature.

The attached patch v3-0001 is exactly the same as v2-0001, v3-0002 is
a patch for specifying tablespace for each merged/split table.

I'm not sure this addressed David's concern about the tablespace choice
in ca4103025 though.

>
> BTW the new regression tests don't check the table and index names.
> Isn't it better to show table and index names for better
> diagnosability?
>
> +-- Check the new partition inherits parent's tablespace
> +CREATE TABLE t (i int PRIMARY KEY USING INDEX TABLESPACE regress_tblspace)
> +  PARTITION BY RANGE (i) TABLESPACE regress_tblspace;
> +CREATE TABLE tp_0_1 PARTITION OF t FOR VALUES FROM (0) TO (1);
> +CREATE TABLE tp_1_2 PARTITION OF t FOR VALUES FROM (1) TO (2);
> +ALTER TABLE t MERGE PARTITIONS (tp_0_1, tp_1_2) INTO tp_0_2;
> +SELECT tablespace FROM pg_tables WHERE tablename IN ('t', 'tp_0_2')
> ORDER BY tablespace;
> +    tablespace
> +------------------
> + regress_tblspace
> + regress_tblspace
> +(2 rows)
> +
> +SELECT tablespace FROM pg_indexes WHERE tablename IN ('t', 'tp_0_2')
> ORDER BY tablespace;
> +    tablespace
> +------------------
> + regress_tblspace
> + regress_tblspace
> +(2 rows)
> +
> +DROP TABLE t;
>
>
> Regards,
>
> --
> Masahiko Sawada
> Amazon Web Services: https://aws.amazon.com
>
>


--
Regards
Junwang Zhao

On Thu, Jul 11, 2024 at 8:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> On 2024/07/10 22:35, Masahiko Sawada wrote:
> > BTW the new regression tests don't check the table and index names.
> > Isn't it better to show table and index names for better
> > diagnosability?
>
> Sounds good to me. I've updated the patch as suggested.
> Please see the attached patch.
>

Thank you for updating the patch! LGTM.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

On 2024/07/12 21:17, Masahiko Sawada wrote:
> On Thu, Jul 11, 2024 at 8:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2024/07/10 22:35, Masahiko Sawada wrote:
>>> BTW the new regression tests don't check the table and index names.
>>> Isn't it better to show table and index names for better
>>> diagnosability?
>>
>> Sounds good to me. I've updated the patch as suggested.
>> Please see the attached patch.
>>
> 
> Thank you for updating the patch! LGTM.

Thanks for reviewing the patch! I've pushed it.

However, some buildfarm members reported errors, so I'll investigate further.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2024/07/15 13:33, Fujii Masao wrote:
> 
> 
> On 2024/07/12 21:17, Masahiko Sawada wrote:
>> On Thu, Jul 11, 2024 at 8:14 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>
>>>
>>>
>>> On 2024/07/10 22:35, Masahiko Sawada wrote:
>>>> BTW the new regression tests don't check the table and index names.
>>>> Isn't it better to show table and index names for better
>>>> diagnosability?
>>>
>>> Sounds good to me. I've updated the patch as suggested.
>>> Please see the attached patch.
>>>
>>
>> Thank you for updating the patch! LGTM.
> 
> Thanks for reviewing the patch! I've pushed it.
> 
> However, some buildfarm members reported errors, so I'll investigate further.

Attached patch fixes unstable tests. Currently testing before pushing.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2024/07/15 14:00, Fujii Masao wrote:
> Attached patch fixes unstable tests. Currently testing before pushing.

I pushed the patch at commit 4e5d6c4091, and some buildfarm animals
are back to green, but crake still reported an error. At first glance,
the error messages don't seem related to the recent patches.
I'll investigate further.

Waiting for replication conn standby_1's replay_lsn to pass 0/15428F78 on primary
[01:31:11.920](206.483s) # poll_query_until timed out executing this query:
# SELECT '0/15428F78' <= replay_lsn AND state = 'streaming'
#          FROM pg_catalog.pg_stat_replication
#          WHERE application_name IN ('standby_1', 'walreceiver')
# expecting this output:
# t
# last actual query output:
# f
# with stderr:
timed out waiting for catchup at /home/andrew/bf/root/HEAD/pgsql/src/test/recovery/t/027_stream_regress.pl line 103.
# Postmaster PID for node "primary" is 99205

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION