MIME-Version: 1.0
References: 
 <171085360143.2046436.7217841141682511557.pgcf@coridan.postgresql.org>
 <CA+TgmoY0=bT_xBP8csR=MFE=FxGE2n2-me2-31jBOgEcLvW7ug@mail.gmail.com>
 <CAPpHfdtp3_H_Rp1ubVYGUnnF0fXbam7+OOG_EMnWos1x2a_3Zg@mail.gmail.com>
 <859476bf-3cb0-455e-b093-b8ab5ef17f0e@postgrespro.ru>
 <CA+TgmoZux8hzQ0=1-Rvigm9VuojW0aHiGSzL+G_g43qyrLGiSg@mail.gmail.com>
 <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>
In-Reply-To: <67ecf5fb-e63d-4284-be75-d0479eef81aa@postgrespro.ru>
From: jian he <jian.universality@gmail.com>
Date: Thu, 5 Jun 2025 12:22:40 +0800
Message-ID: 
 <CACJufxFGPLTDW55bgaBTH1uCf0=YUZUVKWFrTBvOZNYKc49MxA@mail.gmail.com>
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
To: Dmitry Koval <d.koval@postgrespro.ru>
Cc: pgsql-hackers@lists.postgresql.org,
	Alexander Korotkov <aekorotkov@gmail.com>
Content-Type: multipart/mixed; boundary="00000000000023aa880636cb7913"
Archived-At: 
 <https://www.postgresql.org/message-id/CACJufxFGPLTDW55bgaBTH1uCf0%3DYUZUVKWFrTBvOZNYKc49MxA%40mail.gmail.com>
Precedence: bulk

--00000000000023aa880636cb7913
Content-Type: text/plain; charset="UTF-8"

hi.

the following are review of
v40-0001-Implement-ALTER-TABLE-.-MERGE-PARTITIONS-.-comma.patch

ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022,
sales_mar2022) INTO sales_feb_mar_apr2022;
There are no tests when sales_feb2022 or sales_mar2022 have any constraints.
a partition can have its own constraint.

What should we do when any to be merged partition has constraints?
----------------------------------------------------------------
DROP TABLE IF EXISTS sales_range cascade;
CREATE TABLE sales_range (salesperson_id INT, salesperson_name text,
sales_amount INT generated always as (1) stored, sales_date DATE)
PARTITION BY RANGE (sales_date);
CREATE TABLE sales_feb2022 PARTITION OF sales_range FOR VALUES FROM
('2022-02-01') TO ('2022-03-01');
CREATE TABLE sales_mar2022 PARTITION OF sales_range FOR VALUES FROM
('2022-03-01') TO ('2022-04-01');
ALTER TABLE sales_feb2022 ALTER COLUMN sales_amount SET EXPRESSION AS (10);
ALTER TABLE sales_mar2022 ALTER COLUMN sales_amount SET EXPRESSION AS (20);

ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022,
sales_mar2022) INTO sales_feb_mar2022;
with v40, sales_feb_mar2022 column sales_int generated expression is
(generated always as (1) stored)

maybe this is what we expected.
but we should have some tests on it.
----------------------------------------------------------------
DROP TABLE IF EXISTS sales_range cascade;
CREATE TABLE sales_range (salesperson_id INT, salesperson_name text,
sales_amount INT, sales_date DATE) PARTITION BY RANGE (sales_date);
CREATE TABLE sales_feb2022 PARTITION OF sales_range FOR VALUES FROM
('2022-02-01') TO ('2022-03-01');
CREATE TABLE sales_mar2022 PARTITION OF sales_range FOR VALUES FROM
('2022-03-01') TO ('2022-04-01');

CREATE VIEW x AS SELECT * FROM sales_mar2022;
ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022,
sales_mar2022) INTO sales_feb_mar2022;
ERROR:  cannot drop table public.sales_mar2022 because other objects
depend on it
DETAIL:  view public.x depends on table public.sales_mar2022
HINT:  Use DROP ... CASCADE to drop the dependent objects too.

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.
----------------------------------------------------------------
+ */
+static Relation
+createPartitionTable(RangeVar *newPartName, Relation modelRel, Oid ownerId)
+{
+ Relation newRel;
+ Oid newRelId;
+ TupleDesc descriptor;
+ List   *colList = NIL;
+ Oid relamId;
+ Oid namespaceId;
+
+ /* If existing rel is temp, it must belong to this session */
+ if (modelRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP &&
+ !modelRel->rd_islocaltemp)
+ ereport(ERROR,
+ errcode(ERRCODE_WRONG_OBJECT_TYPE),
+ errmsg("cannot create as partition of temporary relation of another
session"));

Looking at it, modelRel is the partitioned table we called ALTER TABLE.
for example:
ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022,
sales_mar2022) INTO sales_feb_mar2022;
modelRel is sales_range.

so this error check can be performed as early as the
transformPartitionCmdForMerge stage?
----------------------------------------------------------------
+ /* Look up the access method for new relation. */
+ relamId = (modelRel->rd_rel->relam != InvalidOid) ?
modelRel->rd_rel->relam : HEAP_TABLE_AM_OID;
looking at the output of "select * from pg_am;".

i think, we can do the following way:
if (modelRel->rd_rel->relam)
elog(ERROR, "error");
relamId = modelRel->rd_rel->relam;
----------------------------------------------------------------
Attached is some refactoring in moveMergedTablesRows, hope it's straightforward.

for example:
+ /* Extract data from old tuple. */
+ slot_getallattrs(srcslot);
+
+ if (tuple_map)
+ {
+ /* Need to use map to copy attributes. */
+ insertslot = execute_attr_map_slot(tuple_map->attrMap, srcslot, dstslot);
+ }
execute_attr_map_slot will call "slot_getallattrs(srcslot);" so the
first one is unncessary.

+ srcslot = MakeSingleTupleTableSlot(RelationGetDescr(mergingPartition),
+   table_slot_callbacks(mergingPartition));
can change to
srcslot = table_slot_create(mergingPartition, NULL);

--00000000000023aa880636cb7913
Content-Type: application/octet-stream;
	name="v40-minor_refactor_moveMergedTablesRows.no_cfbot"
Content-Disposition: attachment;
	filename="v40-minor_refactor_moveMergedTablesRows.no_cfbot"
Content-Transfer-Encoding: base64
Content-ID: <f_mbivecgq0>
X-Attachment-Id: f_mbivecgq0

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2NvbW1hbmRzL3RhYmxlY21kcy5jIGIvc3JjL2JhY2tl
bmQvY29tbWFuZHMvdGFibGVjbWRzLmMKaW5kZXggYjRlYzY2OTFkOGYuLjllMWFjMWU2MjM1IDEw
MDY0NAotLS0gYS9zcmMvYmFja2VuZC9jb21tYW5kcy90YWJsZWNtZHMuYworKysgYi9zcmMvYmFj
a2VuZC9jb21tYW5kcy90YWJsZWNtZHMuYwpAQCAtMjIzNzMsOSArMjIzNzMsNyBAQCBtb3ZlTWVy
Z2VkVGFibGVzUm93cyhSZWxhdGlvbiByZWwsIExpc3QgKm1lcmdpbmdQYXJ0aXRpb25zTGlzdCwK
IAliaXN0YXRlID0gR2V0QnVsa0luc2VydFN0YXRlKCk7CiAKIAkvKiBDcmVhdGUgbmVjZXNzYXJ5
IHR1cGxlIHNsb3QuICovCi0JZHN0c2xvdCA9IE1ha2VTaW5nbGVUdXBsZVRhYmxlU2xvdChSZWxh
dGlvbkdldERlc2NyKG5ld1BhcnRSZWwpLAotCQkJCQkJCQkJICAgdGFibGVfc2xvdF9jYWxsYmFj
a3MobmV3UGFydFJlbCkpOwotCUV4ZWNTdG9yZUFsbE51bGxUdXBsZShkc3RzbG90KTsKKwlkc3Rz
bG90ID0gdGFibGVfc2xvdF9jcmVhdGUobmV3UGFydFJlbCwgTlVMTCk7CiAKIAlmb3JlYWNoKGxp
c3RwdHIsIG1lcmdpbmdQYXJ0aXRpb25zTGlzdCkKIAl7CkBAIC0yMjM4Niw4ICsyMjM4NCw3IEBA
IG1vdmVNZXJnZWRUYWJsZXNSb3dzKFJlbGF0aW9uIHJlbCwgTGlzdCAqbWVyZ2luZ1BhcnRpdGlv
bnNMaXN0LAogCQlTbmFwc2hvdAlzbmFwc2hvdDsKIAogCQkvKiBDcmVhdGUgdHVwbGUgc2xvdCBm
b3IgbmV3IHBhcnRpdGlvbi4gKi8KLQkJc3Jjc2xvdCA9IE1ha2VTaW5nbGVUdXBsZVRhYmxlU2xv
dChSZWxhdGlvbkdldERlc2NyKG1lcmdpbmdQYXJ0aXRpb24pLAotCQkJCQkJCQkJCSAgIHRhYmxl
X3Nsb3RfY2FsbGJhY2tzKG1lcmdpbmdQYXJ0aXRpb24pKTsKKwkJc3Jjc2xvdCA9IHRhYmxlX3Ns
b3RfY3JlYXRlKG1lcmdpbmdQYXJ0aXRpb24sIE5VTEwpOwogCiAJCS8qCiAJCSAqIE1hcCBjb21w
dXRpbmcgZm9yIG1vdmluZyBhdHRyaWJ1dGVzIG9mIG1lcmdlZCBwYXJ0aXRpb24gdG8gbmV3CkBA
IC0yMjQwNCw4ICsyMjQwMSw3IEBAIG1vdmVNZXJnZWRUYWJsZXNSb3dzKFJlbGF0aW9uIHJlbCwg
TGlzdCAqbWVyZ2luZ1BhcnRpdGlvbnNMaXN0LAogCQl7CiAJCQlUdXBsZVRhYmxlU2xvdCAqaW5z
ZXJ0c2xvdDsKIAotCQkJLyogRXh0cmFjdCBkYXRhIGZyb20gb2xkIHR1cGxlLiAqLwotCQkJc2xv
dF9nZXRhbGxhdHRycyhzcmNzbG90KTsKKwkJCUNIRUNLX0ZPUl9JTlRFUlJVUFRTKCk7CiAKIAkJ
CWlmICh0dXBsZV9tYXApCiAJCQl7CkBAIC0yMjQxNCw2ICsyMjQxMCw4IEBAIG1vdmVNZXJnZWRU
YWJsZXNSb3dzKFJlbGF0aW9uIHJlbCwgTGlzdCAqbWVyZ2luZ1BhcnRpdGlvbnNMaXN0LAogCQkJ
fQogCQkJZWxzZQogCQkJeworCQkJCXNsb3RfZ2V0YWxsYXR0cnMoc3Jjc2xvdCk7CisKIAkJCQkv
KiBDb3B5IGF0dHJpYnV0ZXMgZGlyZWN0bHkuICovCiAJCQkJaW5zZXJ0c2xvdCA9IGRzdHNsb3Q7
CiAKQEAgLTIyNDMwLDggKzIyNDI4LDYgQEAgbW92ZU1lcmdlZFRhYmxlc1Jvd3MoUmVsYXRpb24g
cmVsLCBMaXN0ICptZXJnaW5nUGFydGl0aW9uc0xpc3QsCiAJCQkvKiBXcml0ZSB0aGUgdHVwbGUg
b3V0IHRvIHRoZSBuZXcgcmVsYXRpb24uICovCiAJCQl0YWJsZV90dXBsZV9pbnNlcnQobmV3UGFy
dFJlbCwgaW5zZXJ0c2xvdCwgbXljaWQsCiAJCQkJCQkJICAgdGlfb3B0aW9ucywgYmlzdGF0ZSk7
Ci0KLQkJCUNIRUNLX0ZPUl9JTlRFUlJVUFRTKCk7CiAJCX0KIAogCQl0YWJsZV9lbmRzY2FuKHNj
YW4pOwo=
--00000000000023aa880636cb7913--