On Thu, Aug 22, 2024 at 12:43 PM Alexander Korotkov <aekorot...@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 = 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. -- Robert Haas EDB: http://www.enterprisedb.com
