Re: [HACKERS] WAL logging problem in 9.4.3?

At Sat, 22 Feb 2020 21:12:20 -0800, Noah Misch <noah(at)leadboat(dot)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(dot)ntt(at)gmail(dot)com> wrote in
> > > At Tue, 18 Feb 2020 23:44:52 -0800, Noah Misch <noah(at)leadboat(dot)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