Re: Speedup of relation deletes during recovery

On 2018-06-18 11:13:47 -0700, Andres Freund wrote:
> On 2018-06-19 03:06:54 +0900, Fujii Masao wrote:
> > On Sat, Jun 16, 2018 at 3:28 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > Hi,
> > >
> > > On 2018-06-15 10:45:04 -0700, Andres Freund wrote:
> > >> > +
> > >> > + srels = palloc(sizeof(SMgrRelation) * ndelrels);
> > >> > for (i = 0; i < ndelrels; i++)
> > >> > - {
> > >> > - SMgrRelation srel = smgropen(delrels[i], InvalidBackendId);
> > >> > + srels[i] = smgropen(delrels[i], InvalidBackendId);
> > >> >
> > >> > - smgrdounlink(srel, false);
> > >> > - smgrclose(srel);
> > >> > - }
> > >> > + smgrdounlinkall(srels, ndelrels, false);
> > >> > +
> > >> > + for (i = 0; i < ndelrels; i++)
> > >> > + smgrclose(srels[i]);
> > >> > + pfree(srels);
> > >
> > > Hm. This will probably cause another complexity issue. If we just
> > > smgropen() the relation will be unowned. Which means smgrclose() will
> > > call remove_from_unowned_list(), which is O(open-relations). Which means
> > > this change afaict creates a new O(ndrels^2) behaviour?
> > >
> > > It seems like the easiest fix for that would be to have a local
> > > SMgrRelationData in that loop, that we assign the relations to? That's
> > > a bit hacky, but afaict should work?
> >
> > The easier (but tricky) fix for that is to call smgrclose() for
> > each SMgrRelation in the reverse order. That is, we should do
> >
> > for (i = ndelrels - 1; i >= 0; i--)
> > smgrclose(srels[i]);
> >
> > instead of
> >
> > for (i = 0; i < ndelrels; i++)
> > smgrclose(srels[i]);
>
> We could do that - but add_to_unowned_list() is actually a bottleneck in
> other places during recovery too. We pretty much never (outside of
> dropping relations / databases) close opened relations during recovery -
> which is obviously problematic since nearly all of the entries are
> unowned. I've come to the conclusion that we should have a global
> variable that just disables adding anything to the global lists.

On second thought: I think we should your approach in the back branches,
and something like I'm suggesting in master once open.

Greetings,

Andres Freund