Re: [HACKERS] WAL logging problem in 9.4.3?

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 
> > > > - When reusing an index build, instead of storing the dropped relid in the
> > > >   IndexStmt and opening the dropped relcache entry in ATExecAddIndex(), store
> > > >   the subid fields in the IndexStmt.  This is less code, and I felt
> > > >   RelationIdGetRelationCache() invited misuse.
> > > 
> > > Hmm. I'm not sure that index_create having the new subid parameters is
> > > good. And the last if(OidIsValid) clause handles storage persistence
> > > so I did that there. But I don't strongly against it.
> 
> Agreed.  My choice there was not a clear improvement.
> 
> > The change on alter_table.sql and create_table.sql is expecting to
> > cause assertion failure.  Don't we need that kind of explanation in
> > the comment? 
> 
> Test comments generally describe the feature unique to that test, not how the
> test might break.  Some tests list bug numbers, but that doesn't apply here.

Agreed. 

> > 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.
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().

> > The patch adds the test for createSubid to pg_visibility.out. It
> > doesn't fail without CLOBBER_CACHE_ALWAYS while regression test but
> > CLOBBER_CACHE_ALWAYS causes initdb fail and the added check won't be
> > reached. I'm not sure it is useful.
> 
> I agree it's not clearly useful, but tests don't need to meet a "clearly
> useful" standard.  When a fast test is not clearly redundant with another
> test, we generally accept it.  In the earlier patch version that inspired this
> test, RELCACHE_FORCE_RELEASE sufficed to make it fail.
> 
> > 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

> @@ -2891,9 +2891,9 @@ include_dir 'conf.d'
When <varname>wal_level</varname> is <literal>minimal</literal> and a
transaction commits after creating, replacing, or truncating the
on-disk file, this setting determines how to persist the new data.  If
the data is smaller than this setting, write it to the WAL

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center