Date: Tue, 25 Feb 2020 21:36:12 -0800
From: Noah Misch <noah@leadboat.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: robertmhaas@gmail.com, pgsql-hackers@postgresql.org,
	9erthalion6@gmail.com, andrew.dunstan@2ndquadrant.com,
	hlinnaka@iki.fi, michael@paquier.xyz
Subject: Re: [HACKERS] WAL logging problem in 9.4.3?
Message-ID: <20200226053612.GA22911@rfd.leadboat.com>
References: <20200219.172908.1235030736223943908.horikyota.ntt@gmail.com>
 <20200221.164959.653062648402657703.horikyota.ntt@gmail.com>
 <20200223051220.GA4150059@rfd.leadboat.com>
 <20200225.100151.2230637753040571699.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200225.100151.2230637753040571699.horikyota.ntt@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

On Tue, Feb 25, 2020 at 10:01:51AM +0900, Kyotaro Horiguchi wrote:
> At Sat, 22 Feb 2020 21:12:20 -0800, Noah Misch <noah@leadboat.com> wrote in 
> > On Fri, Feb 21, 2020 at 04:49:59PM +0900, Kyotaro Horiguchi wrote:
> > > At Wed, 19 Feb 2020 17:29:08 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> > > > At Tue, 18 Feb 2020 23:44:52 -0800, Noah Misch <noah@leadboat.com> wrote in 

> > > In swap_relation_files, we can remove rel2-related code when #ifndef
> > > USE_ASSERT_CHECKING.
> > 
> > When state is visible to many compilation units, we should avoid making that
> > state depend on --enable-cassert.  That would be a recipe for a Heisenbug.  In
> > a hot code path, it might be worth the risk.
> 
> I aggree that the new #ifdef can invite a Heisenbug. I thought that
> you didn't want that because it doesn't make substantial difference.

v35nm added swap_relation_files() code so AssertPendingSyncs_RelationCache()
could check rd_droppedSubid relations.  v30nm, which did not have
rd_droppedSubid, removed swap_relation_files() code that wasn't making a
difference.

> If we decide to keep the consistency there, I would like to describe
> the code is there for consistency, not for the benefit of a specific
> assertion.
> 
> (cluster.c:1116)
> -    * new. The next step for rel2 is deletion, but copy rd_*Subid for the
> -    * benefit of AssertPendingSyncs_RelationCache().
> +    * new. The next step for rel2 is deletion, but copy rd_*Subid for the
> +    * consistency of the fieles. It is checked later by
> +    * AssertPendingSyncs_RelationCache().

I think the word "consistency" is too vague for "consistency of the fields" to
convey information.  May I just remove the last sentence of the comment
(everything after "* new.")?

> > > config.sgml:
> > > +        When <varname>wal_level</varname> is <literal>minimal</literal> and a
> > > +        transaction commits after creating or rewriting a permanent table,
> > > +        materialized view, or index, this setting determines how to persist
> > > 
> > > "creating or truncation" a permanent table?  and maybe "refreshing
> > > matview and reindex". I'm not sure that they can be merged that way.
> ...
> > I like mentioning truncation, but I dislike how this implies that CREATE
> > INDEX, CREATE MATERIALIZED VIEW, and ALTER INDEX SET TABLESPACE aren't in
> > scope.  While I usually avoid the word "relation" in documentation, I can
> > justify it here to make the sentence less complex.  How about the following?
> > 
> > --- a/doc/src/sgml/config.sgml
> > +++ b/doc/src/sgml/config.sgml
> > @@ -2484,9 +2484,9 @@ include_dir 'conf.d'
> >          In <literal>minimal</literal> level, no information is logged for
> > -        tables or indexes for the remainder of a transaction that creates or
> > -        truncates them.  This can make bulk operations much faster (see
> > -        <xref linkend="populate-pitr"/>).  But minimal WAL does not contain
> > -        enough information to reconstruct the data from a base backup and the
> > -        WAL logs, so <literal>replica</literal> or higher must be used to
> > -        enable WAL archiving (<xref linkend="guc-archive-mode"/>) and
> > -        streaming replication.
> > +        permanent relations for the remainder of a transaction that creates,
> > +        rewrites, or truncates them.  This can make bulk operations much
> > +        faster (see <xref linkend="populate-pitr"/>).  But minimal WAL does
> > +        not contain enough information to reconstruct the data from a base
> > +        backup and the WAL logs, so <literal>replica</literal> or higher must
> > +        be used to enable WAL archiving (<xref linkend="guc-archive-mode"/>)
> > +        and streaming replication.
> >         </para>
> > @@ -2891,9 +2891,9 @@ include_dir 'conf.d'
> >          When <varname>wal_level</varname> is <literal>minimal</literal> and a
> > -        transaction commits after creating or rewriting a permanent table,
> > -        materialized view, or index, this setting determines how to persist
> > -        the new data.  If the data is smaller than this setting, write it to
> > -        the WAL log; otherwise, use an fsync of the data file.  Depending on
> > -        the properties of your storage, raising or lowering this value might
> > -        help if such commits are slowing concurrent transactions.  The default
> > -        is two megabytes (<literal>2MB</literal>).
> > +        transaction commits after creating, rewriting, or truncating a
> > +        permanent relation, this setting determines how to persist the new
> > +        data.  If the data is smaller than this setting, write it to the WAL
> > +        log; otherwise, use an fsync of the data file.  Depending on the
> > +        properties of your storage, raising or lowering this value might help
> > +        if such commits are slowing concurrent transactions.  The default is
> > +        two megabytes (<literal>2MB</literal>).
> >         </para>
> 
> I agree that relation works as the generic name of table-like
> objects. Addition to that, doesn't using the word "storage file" make
> it more clearly?  I'm not confident on the wording itself, but it will
> look like the following.
> 
> > @@ -2484,9 +2484,9 @@ include_dir 'conf.d'
> In <literal>minimal</literal> level, no information is logged for
> permanent relations for the remainder of a transaction that creates,
> replaces, or truncates the on-disk file.  This can make bulk
> operations much

The docs rarely use "storage file" or "on-disk file" as terms.  I hesitate to
put more emphasis on files, because they are part of the implementation, not
part of the user interface.  The term "rewrites"/"rewriting" has the same
problem, though.  Yet another alternative would be to talk about operations
that change the pg_relation_filenode() return value:

  In <literal>minimal</literal> level, no information is logged for permanent
  relations for the remainder of a transaction that creates them or changes
  what <function>pg_relation_filenode</function> returns for them.

What do you think?