Re: [HACKERS] WAL logging problem in 9.4.3?

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.

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

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

> --- a/doc/src/sgml/config.sgml
> +++ b/doc/src/sgml/config.sgml
> @@ -2889,13 +2889,13 @@ include_dir 'conf.d'
>        <listitem>
>         <para>
>          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 or truncating a permanent table,
> +        refreshing a materialized view, or reindexing, 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 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>