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

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