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