public inbox for [email protected]  
help / color / mirror / Atom feed
From: jian he <[email protected]>
To: Dmitry Koval <[email protected]>
Cc: [email protected]
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Date: Sat, 12 Jul 2025 22:01:22 +0800
Message-ID: <CACJufxHM0sD8opy2hUxXLcdY3CQOCaMfsQtJs7yF3TS2YxSqKg@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <171085360143.2046436.7217841141682511557.pgcf@coridan.postgresql.org>
	<CACJufxHRX6xQtzn+KenK3SDR2+4kUpVnO+0axforV_pohwMnFw@mail.gmail.com>
	<[email protected]>
	<CACJufxHNvb6PpeGDEkzZMnKHjZrQGBHRUENKb2AwZ4N3Q8G3fg@mail.gmail.com>
	<[email protected]>
	<CACJufxE6mU7Za2X5Z4Cs9jEmgfjTYPy_ChiVXND5ZEZ3xM665g@mail.gmail.com>
	<[email protected]>
	<CACJufxHakPGgUHfe4uThY2u_Hzq3YOP9m4B3gjZUTUm2mbVryA@mail.gmail.com>
	<CACJufxHo0u=PzgCn88VZEW0mtB=JopchqBTESudUMH+HCyiTpg@mail.gmail.com>
	<[email protected]>
	<CACJufxFTiAiN_27FzAE60Hm5yQ124EJ+b1dpNDstcSdCApFzLw@mail.gmail.com>
	<[email protected]>
	<CACJufxHjpdeDtOsmz9TyGX7jFU73LBEjX=JQpRQdAvDadT0coA@mail.gmail.com>
	<[email protected]>
	<CACJufxGSwvUwu2gyZfTzv=3TJ8Zu4=Kq-GB_pg3x5_5dpwYQkA@mail.gmail.com>
	<[email protected]>

On Wed, Jun 25, 2025 at 5:28 AM Dmitry Koval <[email protected]> wrote:
>
> Hi!
> Thanks for notes!
>
hi.

+static void
+transformPartitionCmdForMerge(CreateStmtContext *cxt, PartitionCmd *partcmd)
+{
+
+ /* Is current partition a DEFAULT partition? */
+ defaultPartOid =
get_default_oid_from_partdesc(RelationGetPartitionDesc(parent, true));
+
this comment seems wrong?
it should be "does this partitioned table (parent) have a default partition?


+ case AT_MergePartitions:
+ {
+ PartitionCmd *partcmd = (PartitionCmd *) cmd->def;
+
+ if (list_length(partcmd->partlist) < 2)
+ ereport(ERROR,
+ errcode(ERRCODE_INVALID_OBJECT_DEFINITION),
+ errmsg("list of new partitions should contain at least two items"));
This error message is not good, IMHO. I don't really have any good ideas though.
if we polish this message later, now we can add a comment: "FIXME
improve this error message".


in ATExecMergePartitions we have:
    RangeVarGetAndCheckCreationNamespace(cmd->name, NoLock, &existingRelid);
it will call RangeVarAdjustRelationPersistence
after it, the cmd->name->relpersistence will be set properly.

so the following in createPartitionTable can be checked way earlier.
    /*
     * 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.
     */
    if (parent_rel->rd_rel->relpersistence != RELPERSISTENCE_TEMP &&
        newRel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
        ereport(ERROR,
                errcode(ERRCODE_WRONG_OBJECT_TYPE),
                errmsg("cannot create a temporary relation as
partition of permanent relation \"%s\"",
                       RelationGetRelationName(parent_rel)));
    /* Permanent rels cannot be partitions belonging to temporary parent */
    if (newRel->rd_rel->relpersistence != RELPERSISTENCE_TEMP &&
        parent_rel->rd_rel->relpersistence == RELPERSISTENCE_TEMP)
        ereport(ERROR,
                errcode(ERRCODE_WRONG_OBJECT_TYPE),
                errmsg("cannot create a permanent relation as
partition of temporary relation \"%s\"",
                       RelationGetRelationName(parent_rel)));

after createPartitionTable->heap_create_with_catalog do the error check seems
unintuitive to me.
it can be checked right after RangeVarGetAndCheckCreationNamespace.





view thread (165+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
  In-Reply-To: <CACJufxHM0sD8opy2hUxXLcdY3CQOCaMfsQtJs7yF3TS2YxSqKg@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox