Re: [HACKERS] WAL logging problem in 9.4.3?

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

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