MIME-Version: 1.0
References: 
 <171085360143.2046436.7217841141682511557.pgcf@coridan.postgresql.org>
 <f56c654f-f152-4dc8-b221-4d4484bed612@postgrespro.ru>
 <CAPpHfdtXDuBaHscJn-wada6Kaf3fxmS4k-1TTVhpS+Kx=uveyw@mail.gmail.com>
 <4e2764f7-f6be-478f-8d7f-f921dbd10f5f@postgrespro.ru>
 <CAPpHfdv2V2tsvVn5LYyrJkzmPA==vbdCEKiT8j9yaaiDTfaQtA@mail.gmail.com>
 <ff30dc7e-ebca-42a2-9d49-a66c9a3c7ee5@postgrespro.ru>
 <CALDaNm3V5DQSbZGxhYH+3MMMm-YdgOg+RdJ-ni6D0TZx_=Xd7w@mail.gmail.com>
 <7fdaf3e8-4c05-4739-bee6-98acc068d1c7@postgrespro.ru>
 <CAPpHfduH83+CW1dBppFVjxauPFGC+-ux+Y7wB0koZJCs029SXA@mail.gmail.com>
 <4433b492-9e97-4334-96d9-ddaaa5083099@postgrespro.ru>
 <CACJufxEi_JVg+=_BSS0o+TYUE_Hq+jmga_yxxuKLSGbQiAnZDg@mail.gmail.com>
 <67ecf5fb-e63d-4284-be75-d0479eef81aa@postgrespro.ru>
 <CACJufxFGPLTDW55bgaBTH1uCf0=YUZUVKWFrTBvOZNYKc49MxA@mail.gmail.com>
 <CACJufxHHnJm6Jb2YQpuRU1RX__tO=JJNJ5=EUMuzif_KNxGd9A@mail.gmail.com>
 <CACJufxFkscyhak2cSr8t+EPfwv11yccfEZBBnPbX4tRc_mWkXQ@mail.gmail.com>
 <884bcf9e-d6e3-4b0c-8dcb-7f2110070ac9@postgrespro.ru>
In-Reply-To: <884bcf9e-d6e3-4b0c-8dcb-7f2110070ac9@postgrespro.ru>
From: Junwang Zhao <zhjwpku@gmail.com>
Date: Tue, 10 Jun 2025 23:58:04 +0800
Message-ID: 
 <CAEG8a3Lpw-h+T+Pfqcchyc9mp0r5UcWT5+uZYsew=1ZSkg-H+w@mail.gmail.com>
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
To: Dmitry Koval <d.koval@postgrespro.ru>
Cc: jian he <jian.universality@gmail.com>, pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAEG8a3Lpw-h%2BT%2BPfqcchyc9mp0r5UcWT5%2BuZYsew%3D1ZSkg-H%2Bw%40mail.gmail.com>
Precedence: bulk

Hi Dmitry,

On Tue, Jun 10, 2025 at 6:48=E2=80=AFAM Dmitry Koval <d.koval@postgrespro.r=
u> wrote:
>
> Hi, Jian He!
>
> Thanks for the suggestions and patches!
> This email contains comments to three emails (05/06/2025).
> I hope to read two emails (for 06/06/2025) tomorrow.
>
> 1.
>  >What should we do when any to be merged partition has constraints?
>  >...
>  >Maybe this is expected, but we need to mention it somewhere and have
>  >some tests on it saying that MERGE PARTITIONS will effectively drop
>  >the partitions, so if any object depends on that partition
>  >then MERGE PARTITIONS can not be done.
>
> Added following phrases to the documentation (I hope this should be
> enough?):
>
> If merged partitions have individual constraints, those constraints will
> be dropped because command uses partitioned table as a model to create
> the constraints.
> If merged partitions have some objects dependent on them, the command
> can not be done (CASCADE is not used, an error will be returned).
>
>
> 2.
>  > ... so this error check can be performed as early as the
>  >transformPartitionCmdForMerge stage?
>
> Function createPartitionTable will be used for various other cases
> besides MERGE PARTITIONS: for SPLIT PARTITION, for PARTITION BY
> REFERENCE (I hope).
> So I think it's better to minimize the amount of code and not move the
> same one check into different functions (transformPartitionCmdForMerge,
> transformPartitionCmdForSplit, ...).
>
>
> 3.
>  >i think, we can do the following way:
>  >if (modelRel->rd_rel->relam)
>  >  elog(ERROR, "error");
>  >relamId =3D modelRel->rd_rel->relam;
>
> Can you clarify what is reason to change the current AM-logic for
> creating a new partition?
>
> +       /* Look up the access method for new relation. */
> +       relamId =3D (modelRel->rd_rel->relam !=3D InvalidOid) ?
> modelRel->rd_rel->relam : HEAP_TABLE_AM_OID;
>
> (If AM is set for a partitioned table, then use it, otherwise use AM for
> heap tables.)
>
>
> 4.
>  > Attached is some refactoring in moveMergedTablesRows, hope it's
> straightforward.
>
> Thanks, these changes are useful.
>
>
> 5.
>  >bug in transformPartitionCmdForMerge "equal(name, name2))"
>  > ...
>  >ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022,
>  >public.sales_feb2022) INTO sales_feb_mar2022;
>  >ERROR:  lower bound of partition "sales_feb2022" conflicts with upper
>  >bound of previous partition "sales_feb2022"
>  >in this context. "sales_feb2022" is the same as "public.sales_feb2022".
>
> Added check and test for this case.
>
>
> 6.
>  >When using ALTER TABLE ... MERGE PARTITIONS, some of the new
>  >partition's properties will not be inherited from to be merged
>  >partitions; instead, they will be directly copied from the root
>  >partitioned table.
>  >So we need to test this behavior.
>  >The attached test file is for test table properties:
>  >(COMMENTS, COMPRESSION, DEFAULTS, GENERATED, STATISTICS, STORAGE).
>
> Some tests already exist (GENERATED, DEFAULTS) - see
> partition_merge.sql, lines after:
>
> +-- Test for:
> +--   * composite partition key;
> +--   * GENERATED column;
> +--   * column with DEFAULT value.
> ...
>
> But the complex test is probably also interesting.
> Test added.
>
> --
>
> Similar changes are made for the second commit (SPLIT PARTITION).
>
> --
> With best regards,
> Dmitry Koval
>
> Postgres Professional: http://postgrespro.com

I had one trivial comment and a question if you don't mind.

+ /* If existing rel is temp, it must belong to this session */
+ if (modelRel->rd_rel->relpersistence =3D=3D RELPERSISTENCE_TEMP &&
+ !modelRel->rd_islocaltemp)
+ ereport(ERROR,
+ errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot create as partition of temporary relation of another
session"));

Would it be better to use RELATION_IS_OTHER_TEMP in this case?
I noticed that while other parts of tablecmds.c don=E2=80=99t use the macro=
,
all other files consistently use RELATION_IS_OTHER_TEMP.

+ /*
+ * We intended to create the partition with the same persistence as the
+ * parent table, but we still need to recheck because that might be
+ * affected by the search_path.  If the parent is permanent, so must be
+ * all of its partitions.
+ */

I have trouble understanding how this is possible, can you kindly
give me some guidance on this logic?

--=20
Regards
Junwang Zhao