Date: Tue, 10 Sep 2019 07:45:17 -0400
From: Noah Misch <noah@leadboat.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: pgsql-hackers@postgresql.org, 9erthalion6@gmail.com,
	andrew.dunstan@2ndquadrant.com, hlinnaka@iki.fi,
	robertmhaas@gmail.com, michael@paquier.xyz
Subject: Re: [HACKERS] WAL logging problem in 9.4.3?
Message-ID: <20190910114517.GA29650@gust.leadboat.com>
References: <20190822.210606.07927021.horikyota.ntt@gmail.com>
 <20190826050843.GB3153606@rfd.leadboat.com>
 <20190827.154932.250364935.horikyota.ntt@gmail.com>
 <20190828.154210.204505676.horikyota.ntt@gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2oS5YaxWCcQjTEyO"
Content-Disposition: inline
In-Reply-To: <20190828.154210.204505676.horikyota.ntt@gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--2oS5YaxWCcQjTEyO
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

[Casual readers with opinions on GUC naming: consider skipping to the end.]

MarkBufferDirtyHint() writes WAL even when rd_firstRelfilenodeSubid or
rd_createSubid is set; see attached test case.  It needs to skip WAL whenever
RelationNeedsWAL() returns false.

On Tue, Aug 27, 2019 at 03:49:32PM +0900, Kyotaro Horiguchi wrote:
> At Sun, 25 Aug 2019 22:08:43 -0700, Noah Misch <noah@leadboat.com> wrote in <20190826050843.GB3153606@rfd.leadboat.com>
> > Consider a one-page relfilenode.  Doing all the things you list for a single
> > page may be cheaper than locking millions of buffer headers.
> 
> If I understand you correctly, I would say that *all* buffers
> that don't belong to in-transaction-created files are skipped
> before taking locks. No lock conflict happens with other
> backends.
> 
> FlushRelationBuffers uses double-checked-locking as follows:

I had misread the code; you're right.

> > This should be GUC-controlled, especially since this is back-patch material.
> 
> Is this size of patch back-patchable?

Its size is not an obstacle.  It's not ideal to back-patch such a user-visible
performance change, but it would be worse to leave back branches able to
corrupt data during recovery.

On Wed, Aug 28, 2019 at 03:42:10PM +0900, Kyotaro Horiguchi wrote:
> - Use log_newpage instead of fsync for small tables.

> I'm trying to measure performance difference on WAL/fsync.

I would measure it with simultaneous pgbench instances:

1. DDL pgbench instance repeatedly creates and drops a table of X kilobytes,
   using --rate to make this happen a fixed number of times per second.
2. Regular pgbench instance runs the built-in script at maximum qps.

For each X, try one test run with effective_io_block_size = X-1 and one with
effective_io_block_size = X.  If the regular pgbench instance gets materially
higher qps with effective_io_block_size = X-1, the ideal default is <X.
Otherwise, the ideal default is >=X.

> +     <varlistentry id="guc-effective-io-block-size" xreflabel="effective_io_block_size">
> +      <term><varname>effective_io_block_size</varname> (<type>integer</type>)
> +      <indexterm>
> +       <primary><varname>effective_io_block_size</varname> configuration parameter</primary>
> +      </indexterm>
> +      </term>
> +      <listitem>
> +       <para>
> +        Specifies the expected maximum size of a file for which <function>fsync</function> returns in the minimum required duration. It is approximately the size of a track or sylinder for magnetic disks.
> +        The value is specified in kilobytes and the default is <literal>64</literal> kilobytes.
> +       </para>
> +       <para>
> +        When <xref linkend="guc-wal-level"/> is <literal>minimal</literal>,
> +        WAL-logging is skipped for tables created in-trasaction.  If a table
> +        is smaller than that size at commit, it is WAL-logged instead of
> +        issueing <function>fsync</function> on it.
> +
> +       </para>
> +      </listitem>
> +     </varlistentry>

Cylinder and track sizes are obsolete as user-visible concepts.  (They're not
constant for a given drive, and I think modern disks provide no way to read
the relevant parameters.)  I like the name "wal_skip_threshold", and my second
choice would be "wal_skip_min_size".  Possibly documented as follows:

  When wal_level is minimal 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 64 kilobytes (64kB).

Any other opinions on the GUC name?

--2oS5YaxWCcQjTEyO
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="wal-optimize-noah-tests-v3.patch"

diff --git a/src/test/recovery/t/018_wal_optimize.pl b/src/test/recovery/t/018_wal_optimize.pl
index 95063ab..5d476a4 100644
--- a/src/test/recovery/t/018_wal_optimize.pl
+++ b/src/test/recovery/t/018_wal_optimize.pl
@@ -11,7 +11,7 @@ use warnings;
 
 use PostgresNode;
 use TestLib;
-use Test::More tests => 28;
+use Test::More tests => 32;
 
 sub check_orphan_relfilenodes
 {
@@ -43,6 +43,8 @@ sub run_wal_optimize
 	$node->append_conf('postgresql.conf', qq(
 wal_level = $wal_level
 max_prepared_transactions = 1
+wal_log_hints = on
+effective_io_block_size = 0
 ));
 	$node->start;
 
@@ -194,6 +196,24 @@ max_prepared_transactions = 1
 	is($result, qq(3),
 	   "wal_level = $wal_level, SET TABLESPACE in subtransaction");
 
+	$node->safe_psql('postgres', "
+		BEGIN;
+		CREATE TABLE test3a5 (c int PRIMARY KEY);
+		SAVEPOINT q; INSERT INTO test3a5 VALUES (1); ROLLBACK TO q;
+		CHECKPOINT;
+		INSERT INTO test3a5 VALUES (1);  -- set index hint bit
+		INSERT INTO test3a5 VALUES (2);
+		COMMIT;");
+	$node->stop('immediate');
+	$node->start;
+	$result = $node->psql('postgres', );
+	my($ret, $stdout, $stderr) = $node->psql(
+		'postgres', "INSERT INTO test3a5 VALUES (2);");
+	is($ret, qq(3),
+	   "wal_level = $wal_level, unique index LP_DEAD");
+	like($stderr, qr/violates unique/,
+	   "wal_level = $wal_level, unique index LP_DEAD message");
+
 	# UPDATE touches two buffers; one is BufferNeedsWAL(); the other is not.
 	$node->safe_psql('postgres', "
 		BEGIN;

--2oS5YaxWCcQjTEyO--