From: Noah Misch <noah@leadboat.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: robertmhaas@gmail.com
Cc: pgsql-hackers@postgresql.org
Cc: 9erthalion6@gmail.com
Cc: andrew.dunstan@2ndquadrant.com
Cc: hlinnaka@iki.fi
Cc: michael@paquier.xyz
Subject: Re: [HACKERS] WAL logging problem in 9.4.3?
Date: Sat, 22 Feb 2020 21:12:20 -0800
Message-ID: <20200223051220.GA4150059@rfd.leadboat.com> (raw)
In-Reply-To: <20200221.164959.653062648402657703.horikyota.ntt@gmail.com>
References: <20200127.192831.1430769560984064622.horikyota.ntt@gmail.com>
<20200219074452.GA4006615@rfd.leadboat.com>
<20200219.172908.1235030736223943908.horikyota.ntt@gmail.com>
<20200221.164959.653062648402657703.horikyota.ntt@gmail.com>
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?
inline patch 1: doc/src/sgml/config.sgml (+14/-14 lines) download inline-1.patch--- 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>
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, horikyota.ntt@gmail.com, robertmhaas@gmail.com, 9erthalion6@gmail.com, andrew.dunstan@2ndquadrant.com, hlinnaka@iki.fi, michael@paquier.xyz
Subject: Re: [HACKERS] WAL logging problem in 9.4.3?
In-Reply-To: <20200223051220.GA4150059@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox