On Tue, Apr 26, 2016 at 12:39 PM, Michael Paquier <michael.paqu...@gmail.com> wrote: > On Tue, Apr 26, 2016 at 11:47 AM, Andres Freund <and...@anarazel.de> wrote: >> Thanks for looking into this. >> >> On 2016-04-26 11:43:06 +0900, Michael Paquier wrote: >>> On Sun, Apr 24, 2016 at 11:51 AM, Andres Freund <and...@anarazel.de> wrote: >>> > ISTM we should additionally replace the CacheInvalidateSmgr() with a >>> > CacheInvalidateRelcache() and document that that implies an smgr >>> > invalidation. Alternatively we could log smgr (and relmapper) >>> > invalidations as well, but that's not quite non-invasive either; but >>> > might be a good long-term idea to keep things simpler. >>> > >>> > Comments? >>> >>> Yeah, this looks like a good idea at the end. >> >> You mean the bit about making smgr invalidations logged? > > Sorry for the lack of precision in my words. I was referring to your > idea of replacing CacheInvalidateSmgr() by CacheInvalidateRelcache() > sounds like a good option as this would make the invalidation to be > logged at commit. Thinking about the logging of smgr invalidations, > this is quite interesting. But what would we actually gain in doing > that? Do you foresee any advantages in doing so? The only case where > this would be useful now is for vm_extend by looking at the code. > >>> As the invalidation patch is aimed at being backpatched, this may be >>> something to do as well in back-branches. >> >> I'm a bit split here. I think forcing processing of invalidations at >> moments they've previously never been processed is a bit risky for the >> back branches. But on the other hand relcache invalidations are only >> processed at end-of-xact, which isn't really correct for the code at >> hand :/ > > Oh, OK. So you mean that this patch is not aimed for back-branches > with this new record type, but that's only for HEAD.. To be honest, I > thought that we had better address this issue on back-branches with a > patch close to what you are proposing (which is more or less what > Simon has actually sent), and keep the change of CacheInvalidateSmgr() > in visibilitymap.c to be HEAD-only.
Ah, and actually as I'm on it from your previous patch there is this bit: + else if (msg->id == SHAREDINVALRELMAP_ID) + appendStringInfoString(buf, " relmap"); + else if (msg->id == SHAREDINVALRELMAP_ID) + appendStringInfo(buf, " relmap db %u", msg->rm.dbId); You surely want to check for !OidIsValid(msg->rm.dbId) in the first one. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
