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 <amitlangote09@gmail.com> wrote:
>
> On Thu, Aug 5, 2021 at 11:32 AM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Wed, Jul 14, 2021 at 11:16 AM houzj.fnst@fujitsu.com
> > > On Tuesday, July 13, 2021 2:52 AM Alvaro Herrera <alvherre@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
