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, 23 Aug 2024 03:56:23 +0300
Message-ID: <CAPpHfdtp3_H_Rp1ubVYGUnnF0fXbam7+OOG_EMnWos1x2a_3Zg@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/CAPpHfdtp3_H_Rp1ubVYGUnnF0fXbam7%2BOOG_EMnWos1x2a_3Zg%40mail.gmail.com>
Precedence: bulk

On Thu, Aug 22, 2024 at 8:25=E2=80=AFPM Robert Haas <robertmhaas@gmail.com>=
 wrote:
>
> On Thu, Aug 22, 2024 at 12:43=E2=80=AFPM Alexander Korotkov
> <aekorotkov@gmail.com> wrote:
> > Thank you for your feedback.  Yes, it seems that there is not enough
> > time to even carefully analyze all the issues in these features.  The
> > rule of thumb I can get from this experience is "think multiple times
> > before accessing something already opened by its name".  I'm going to
> > revert these features during next couple days.
>
> Thanks, and sorry about that. I would say even "think multiple times"
> is possibly not strong enough -- it might almost be "just don't ever
> do it". Even if (in some particular case) the invalidation mechanism
> seems to protect you from getting wrong answers, there are often holes
> in that, specifically around search_path =3D foo, bar and you're
> operating on an object in schema bar and an identically-named object
> is created in schema foo at just the wrong time. Sometimes there are
> problems even when search_path is not involved, but when it is, there
> are more.
>
> 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.
>
> 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.
>
> 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.

Thank you for so valuable feedback!  When I have another go over this
patch I will ensure this is addressed.

------
Regards,
Alexander Korotkov
Supabase