Re: Suspicious behaviour on applying XLOG_HEAP2_VISIBLE.

On Tue, Apr 26, 2016 at 12:39 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Apr 26, 2016 at 11:47 AM, Andres Freund <andres(at)anarazel(dot)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 <andres(at)anarazel(dot)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