MIME-Version: 1.0
References: <171085360143.2046436.7217841141682511557.pgcf@coridan.postgresql.org>
 <171085858750.2046434.12246066729700037354.pgcf@coridan.postgresql.org>
 <909e16b4-9eda-435c-b033-8294ae4adc05@postgrespro.ru> <CAPpHfdsVQwnZPxaQPwcu1g_01F0d-o=ECRcf14X3CmavJB6vSQ@mail.gmail.com>
 <a83d5a56-ff33-47e8-b2d8-7e9a5d59ee1e@postgrespro.ru> <CAPpHfdsCCV8DOv0Ovu4dTSnuWXiEz_AjuW-J6TPj++tcfEGvRw@mail.gmail.com>
 <3a4b9c1c-3edf-4e29-9629-b4113aefb0f3@postgrespro.ru> <CAPpHfduNYkMVuUSxZjD6u4tZG6aKdhgQcmhr-K9OQfeg56MNww@mail.gmail.com>
 <c3730d78-6081-4c41-9715-d1d192734576@postgrespro.ru> <CAPpHfdtj7YsPaASoVPN+N3H4_Ct+kQw8QY1d_9u7FPnbghkicw@mail.gmail.com>
 <20240808171351.a9.nmisch@google.com> <19c1e04c-2012-4475-8460-821dd633a6ce@postgrespro.ru>
 <CAPpHfdtYgQ8PUZMS043c8USeKcKh5isrAW0QQKMAxQacX-+nuA@mail.gmail.com>
 <768cbb42-d3f8-45de-89ad-72861f078dc2@postgrespro.ru> <CAPpHfdu+iX5TP65Cyn6vUe8JKGtLmMA=sKKKgE7tB6G6Qk=18Q@mail.gmail.com>
 <CALT9ZEHhVt-i1vGL_eCfNqAisTiC6VaM6U-fxP3kD306uPLEcw@mail.gmail.com>
 <CAPpHfdsEtAWhpcPEaUky+gO20=rz0P03OskAS0c8teuTtHP9hQ@mail.gmail.com>
 <CALT9ZEGTFdwrP+jO1tKxHHeGPM-enMm734MP0wCwJTQZDembYA@mail.gmail.com>
 <CAPpHfdst=eqJ8-n=yhSk=5Xk2QHLF90HWryHWgARuityB3=b7A@mail.gmail.com>
 <CA+TgmobHYix=Nn8D4RUHa6fhUVPR88KGAMq1pBfnGfOfEjRixA@mail.gmail.com>
 <CAPpHfdt_Apbzjx-q2moST3VyNq9Shx8ByGxTF-aqBs3Gaf_n-Q@mail.gmail.com> <CA+TgmoY0=bT_xBP8csR=MFE=FxGE2n2-me2-31jBOgEcLvW7ug@mail.gmail.com>
In-Reply-To: <CA+TgmoY0=bT_xBP8csR=MFE=FxGE2n2-me2-31jBOgEcLvW7ug@mail.gmail.com>
From: Alexander Korotkov <aekorotkov@gmail.com>
Date: Fri, 31 Jan 2025 11:19:29 +0200
Message-ID: <CAPpHfdtSxrcxQERO82cyQ2heN3+A7VC63k8SmL=EEiph-8rfHg@mail.gmail.com>
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
To: Robert Haas <robertmhaas@gmail.com>
Cc: Pavel Borisov <pashkin.elfe@gmail.com>, Dmitry Koval <d.koval@postgrespro.ru>, 
	Noah Misch <noah@leadboat.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/CAPpHfdtSxrcxQERO82cyQ2heN3%2BA7VC63k8SmL%3DEEiph-8rfHg%40mail.gmail.com>
Precedence: bulk

Hi!

I'd like to share some thoughts on which particular way this patch could go=
.

On Thu, Aug 22, 2024 at 8:25=E2=80=AFPM Robert Haas <robertmhaas@gmail.com>=
 wrote:
> Here, aside from the name lookup issues, there are also problems with
> expression evaluation: we can't split partitions without reindexing
> rows that those partitions contain, and it is critical to think
> through which is going to do the evaluation and make sure it's
> properly sandboxed. I think we might need
> SECURITY_RESTRICTED_OPERATION here.

+1 for use SECURITY_RESTRICTED_OPERATION

> Another thing I want to highlight if you do have another go at this
> patch is that it's really critical to think about where every single
> property of the newly-created tables comes from. The original patch
> didn't consider relpersistence or tableam, and here I just discovered
> that owner is also an issue that probably needs more consideration,
> but it goes way beyond that. For example, I was surprised to discover
> that if I put per-partition constraints or triggers on a partition and
> then split it, they were not duplicated to the new partitions. Now,
> maybe that's actually the behavior we want -- I'm not 100% positive --
> but it sure wasn't what I was expecting. If we did duplicate them when
> splitting, then what's supposed to happen when merging occurs? That is
> not at all obvious, at least to me, but it needs careful thought. ACLs
> and rules and default values and foreign keys (both outbond and
> inbound) all need to be considered too, along with 27 other things
> that I'm sure I'm not thinking about right now. Some of this behavior
> should probably be explicitly documented, but all of it should be
> considered carefully enough before commit to avoid surprises later. I
> say that both from a security point of view and also just from a user
> experience point of view. Even if things aren't insecure, they can
> still be annoying, but it's not uncommon in cases like this for
> annoying things to turn out to also be insecure.

Yes, I think it's a good idea to duplicate dependent objects of split
partition to new partitions.  But it important to very carefully check
user have relevant permissions for all of them.  We could also provide
a syntax to exclude some of them (and even define new ones?), but I
strongly suspect that would overcomplicate patch for now and we need
to postpone this.

Regarding the merge, I think it would be good to provide a syntax to
let user choose a model partition between partitions to be merged.

> Finally, if you do revisit this, I believe it would be a good idea to
> think a bit harder about how data is moved around. My impression (and
> please correct me if I am mistaken) is that currently, any split or
> merge operation rewrites all the data in the source partition(s). If a
> large partition is being split nearly equally, I think that has a good
> chance of being optimal, but I think that might be the only case. If
> we're merging partitions, wouldn't it be better to adjust the
> constraints on the first partition -- or perhaps the largest partition
> if we want to be clever -- and insert the data from all of the others
> into it? Maybe that would even have syntax that puts the user in
> control of which partition survives, e.g. ALTER TABLE tab1 MERGE
> PARTITION part1 WITH part2, part3, .... That would also make it really
> obvious to the user what all of the properties of part1 will be after
> the merge: they will be exactly the same as they were before the
> merge, except that the partition constraint will have been adjusted.
> You basically dodge everything in the previous paragraph in one shot,
> and it seems like it would also be faster. Splitting there's no
> similar get-out-of-jail free card, at least not that I can see. Even
> if you add syntax that splits a partition by using INSERT/DELETE to
> move some rows to a newly-created partition, you still have to make at
> least one new partition. But possibly that syntax is worth having
> anyway, because it would be a lot quicker in the case of a highly
> asymmetric split. On the other hand, maybe even splits are much more
> likely and we don't really need it. I don't know.

Hmm... I think the important aspect for this DDL operation is to be
atomic and transactional.  And that seems to be extremely hard to
achieve if we move the data between existing relnodes.  How can we
rollback or recover after error?  So, it least for initial
implementation I would leave data movement as it is.

------
Regards,
Alexander Korotkov
Supabase