On Wed, Apr 28, 2021 at 8:32 AM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > On 2021-Apr-27, Alvaro Herrera wrote: > > > This v3 handles things as you suggested and works correctly AFAICT. I'm > > going to add some more tests cases to verify the behavior in the > > scenarios you showed, and get them to run under cache-clobber options to > > make sure it's good. > > Yep, it seems to work. Strangely, the new isolation case doesn't > actually crash before the fix -- it merely throws a memory allocation > error.
Thanks. Yeah, it does seem to work. I noticed that rd_partdesc_nodetached_xmin can sometimes end up with value 0. While you seem to be already aware of that, because otherwise you wouldn't have added TransactionIdIsValid(...) in condition in RelationGetPartitionDesc(), the comments nearby don't mention why such a thing might happen. Also, I guess it can't be helped that the partdesc_nodetached will have to be leaked when the xmin is 0, but that shouldn't be as problematic as the case we discussed earlier. + /* + * But first, a kluge: if there's an old context for this type of + * descriptor, it contains an old partition descriptor that may still be + * referenced somewhere. Preserve it, while not leaking it, by + * reattaching it as a child context of the new one. Eventually it will + * get dropped by either RelationClose or RelationClearRelation. + * + * We keep the regular partdesc in rd_pdcxt, and the partdesc-excluding- + * detached-partitions in rd_pddcxt. + */ + context = is_omit ? &rel->rd_pddcxt : &rel->rd_pdcxt; + if (*context != NULL) + MemoryContextSetParent(*context, new_pdcxt); + *context = new_pdcxt; Would it be a bit more readable to just duplicate this stanza in the blocks that assign to rd_partdesc_nodetached and rd_partdesc, respectively? That's not much code to duplicate and it'd be easier to see which context is for which partdesc. + TransactionId rd_partdesc_nodetached_xmin; /* xmin for the above */ Could you please expand this description a bit? -- Amit Langote EDB: http://www.enterprisedb.com
