Hello. I confirmed that this patch fixes the crash. At Tue, 17 Jul 2018 20:01:05 -0400, Tom Lane <t...@sss.pgh.pa.us> wrote in <14892.1531872...@sss.pgh.pa.us> > I wrote: > >> So I said I didn't want to do extra work on this, but I am looking into > >> fixing it by having these aux process types run a ResourceOwner that can > >> be told to clean up any open buffer pins at exit. > > > That turned out to be a larger can of worms than I'd anticipated, as it > > soon emerged that we'd acquired a whole lot of cargo-cult programming > > around ResourceOwners. ... > > I'm mostly pretty happy with this, but I think there are a couple of > > loose ends in logicalfuncs.c and slotfuncs.c: those are creating > > non-standalone ResourceOwners (children of whatever the active > > ResourceOwner is) and doing nothing much to clean them up. That seems > > pretty bogus. > > Further investigation showed that the part of that code that was > actually needed was not the creation of a child ResourceOwner, but rather > restoration of the old CurrentResourceOwner setting after exiting the > logical decoding loop. Apparently we can come out of that with the > TopTransaction resowner being active. This still seems a bit bogus; > maybe there should be a save-and-restore happening somewhere else? > But I'm not really excited about doing more than commenting it.
CurrentResourceOwner doesn't seem to be changed. It is just saved during snapshot export and used just as a flag only for checking for duplicate snapshot exporting. > Also, most of the other random creations of ResourceOwners seem to just > not be necessary at all, even with the new rule that you must have a > resowner to acquire buffer pins. So the attached revised patch just > gets rid of them, and improves some misleading/wrong comments on the > topic. It'd still be easy to use CreateAuxProcessResourceOwner in any > process where we discover we need one, but I don't see the value in adding > useless overhead. +1 for unifying to resowner for auxiliary processes. I found the comment below just before ending cleanup of auxiliary process main funcs. | * These operations are really just a minimal subset of | * AbortTransaction(). We don't have very many resources to worry | * about in checkpointer, but we do have LWLocks, buffers, and temp | * files. So couldn't we use TopTransactionResourceOwner instead of AuxProcessResrouceOwner? I feel a bit uneasy that bootstrap and standalone-backend have *AuxProcess*ResourceOwner. It's not about this ptch, but while looking this closer, I found the following comment on ShutdownXLOG(). | /* | * This must be called ONCE during postmaster or standalone-backend shutdown | */ Is the "postmaster" typo of "bootstrap process"? It is also called from checkpointer when non-standlone mode. > At this point I'm leaning to just applying this in HEAD and calling it > good. The potential for assertion failures isn't relevant to production > builds, and my best guess at this point is that there isn't really any > other user-visible bug. The resowners that were being created and not > adequately cleaned up all seem to have been dead code anyway (ie they'd > never acquire any resources). Agreed. > We could have a problem if, say, the > bgwriter exited via the FATAL path while holding a pin, but I don't think > there's a reason for it to do that except in a database-wide shutdown, > where the consequences of a leaked pin seem pretty minimal. > > Any objections? Anyone want to do further review? FWIW I think we won't be concerned about leaked pins after FATAL. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
