I have reviewed the patch and it looks good to me. However I have one comment.
+ foreach(l, attachrel_children) + { + Oid partOid = lfirst_oid(l); + + CacheInvalidateRelcacheByRelid(partOid); + } Can we avoid using the extra variable 'partOid' and directly pass 'lfirst_oid(l)' to CacheInvalidateRelcacheByRelid(). Thanks & Regards, Nitin Jadhav On Fri, Aug 27, 2021 at 2:50 PM Amit Langote <amitlangot...@gmail.com> wrote: > > On Thu, Aug 5, 2021 at 11:32 AM Amit Langote <amitlangot...@gmail.com> wrote: > > On Wed, Jul 14, 2021 at 11:16 AM houzj.f...@fujitsu.com > > > On Tuesday, July 13, 2021 2:52 AM Alvaro Herrera > > > <alvhe...@alvh.no-ip.org> wrote: > > > > Did you have a misbehaving test for the ATTACH case? > > > > > > Thanks for the response. > > > > Thank you both. > > > > > Yes, I think the following example of ATTACH doesn't work as expected. > > > > Yeah, need the fix for the ATTACH case too. > > > > Couple more things: > > > > * We must invalidate not just the "direct" partitions of the table > > being attached/detached, but also any indirect ones, because all of > > their partition constraints would need to contain (or no longer > > contain) the root parent's partition constraint. > > > > * I think we should lock the partitions before sending the > > invalidation. The ATTACH code already locks the descendents for a > > different purpose, but DETACH doesn't, so the latter needs to be fixed > > to match. > > > > I've updated Alvaro's patch to address these points. Maybe, we should > > also add these cases to the regression and isolation suites? > > Apparently, I had posted a version of the patch that didn't even compile. > > I have fixed that in the attached and also added regression tests. > > Adding this to the next CF. > > -- > Amit Langote > EDB: http://www.enterprisedb.com