Hi Alvaro, Sorry I totally failed to see the v2 you had posted and a couple of other emails where you mentioned the issues I brought up.
On Thu, Sep 24, 2020 at 12:23 AM Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > On 2020-Sep-23, Amit Langote wrote: > I suspect that we don't really need this defensive constraint. I mean > > even after committing the 1st transaction, the partition being > > detached still has relispartition set to true and > > RelationGetPartitionQual() still returns the partition constraint, so > > it seems the constraint being added above is duplicative. > > Ah, thanks for thinking through that. I had vague thoughts about this > being unnecessary in the current mechanics, but hadn't fully > materialized the thought. (The patch was completely different a few > unposted iterations ago). > > > Moreover, the constraint is not removed as part of any cleaning up > > after the DETACH process, so repeated attach/detach of the same > > partition results in the constraints piling up: > > Yeah, I knew about this issue (mentioned in my self-reply on Aug 4) and > didn't worry too much about it because I was thinking I'd rather get rid > of the constraint addition in the first place. Okay, gotcha. > > Noticed a bug/typo in the patched RelationBuildPartitionDesc(): > > > > - inhoids = find_inheritance_children(RelationGetRelid(rel), NoLock); > > + inhoids = find_inheritance_children(RelationGetRelid(rel), NoLock, > > + include_detached); > > > > You're passing NoLock for include_detached which means you never > > actually end up including detached partitions from here. > > I fixed this in the version I posted on Sept 10. I think you were > reading the version posted at the start of this thread. I am trying the v2 now and I can confirm that those problems are now fixed. However, I am a bit curious about including detached partitions in some cases while not in other, which can result in a (to me) surprising behavior as follows: Session 1: create table foo (a int, b int) partition by range (a); create table foo1 partition of foo for values from (1) to (2); create table foo2 partition of foo for values from (2) to (3); ...attach GDB and set breakpoint so as to block right after finishing the 1st transaction of DETACH PARTITION CONCURRENTLY... alter table foo detach partition foo2 concurrently; <hits breakpoint, wait...> Session 2: begin; insert into foo values (2); -- ok select * from foo; select * from foo; -- ?! a | b ---+--- (0 rows) Maybe, it's fine to just always exclude detached partitions, although perhaps I am missing some corner cases that you have thought of? Also, I noticed that looking up a parent's partitions via RelationBuildPartitionDesc or directly will inspect inhdetached to include or exclude partitions, but checking if a child table is a partition of a given parent table via get_partition_parent doesn't. Now if you fix get_partition_parent() to also take into account inhdetached, for example, to return InvalidOid if true, then the callers would need to not consider the child table a valid partition. So, RelationGetPartitionQual() on a detached partition should actually return NIL, making my earlier claim about not needing the defensive CHECK constraint invalid. But maybe that's fine if all places agree on a detached partition not being a valid partition anymore? -- Amit Langote EnterpriseDB: http://www.enterprisedb.com
