Re: Suspicious behaviour on applying XLOG_HEAP2_VISIBLE.

From: Andres Freund <andres(at)anarazel(dot)de>
To: Robert Haas <robertmhaas(at)gmail(dot)com>, Dean Rasheed <dean(dot)a(dot)rasheed(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Simon Riggs <simon(at)2ndQuadrant(dot)com>, Noah Misch <noah(at)leadboat(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Suspicious behaviour on applying XLOG_HEAP2_VISIBLE.
Date: 2016-04-25 18:10:23
Message-ID: 20160425181023.4fnvr4kbzblt34hq@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Dean, Robert,

Afaics the problem described below was introduced in b4e07417, do you
have a different/better proposal than
s/CacheInvalidateSmgr/CacheInvalidateRelcache/? Doing that doesn't feel
quite right either, because it only makes the file extension visible at
end-of-xact - which is mostly harmless, but still.

On 2016-04-23 19:51:17 -0700, Andres Freund wrote:
> It fixes the problem at hand, but essentially it's just luck that the
> patch is sufficient. The first layer of the issue is that queued
> invalidation messages aren't sent; but for vm_extend() there's another
> side to it. Namely vm_extend() does
>
> /*
> * Send a shared-inval message to force other backends to close any smgr
> * references they may have for this rel, which we are about to change.
> * This is a useful optimization because it means that backends don't have
> * to keep checking for creation or extension of the file, which happens
> * infrequently.
> */
> CacheInvalidateSmgr(rel->rd_smgr->smgr_rnode);
>
> but CacheInvalidateSmgr is non-transactional as it's comment explains:
> *
> * Note: because these messages are nontransactional, they won't be captured
> * in commit/abort WAL entries. Instead, calls to CacheInvalidateSmgr()
> * should happen in low-level smgr.c routines, which are executed while
> * replaying WAL as well as when creating it.
> *
>
> as far as I can see vm_extend() is the only current caller forgetting
> that rule. I don't think it's safe to use CacheInvalidateSmgr() outside
> smgr.c.
>
> The reason this all ends up working nonetheless is that the
> heap_inplace_update()s in vacuum triggers a CacheInvalidateHeapTuple()
> which queues a relcache invalidation, which in turn does a
> RelationCloseSmgr() in RelationClearRelation(). Thereby effectively
> doing a transactional CacheInvalidateSmgr(). But that seems more than
> fragile.
>
> 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.

- Andres

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Joshua D. Drake 2016-04-25 18:38:07 Re: Rename max_parallel_degree?
Previous Message Tom Lane 2016-04-25 18:09:27 Apparent race condition in standby promotion