On 2018-06-19 03:06:54 +0900, Fujii Masao wrote:
> On Sat, Jun 16, 2018 at 3:28 AM, Andres Freund <andres@anarazel.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.
Greetings,
Andres Freund