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: Sun, 15 Mar 2020 20:46:47 -0700
Message-ID: <20200316034647.GA1121601@rfd.leadboat.com> (raw)
In-Reply-To: <20200304.162919.898938381201316571.horikyota.ntt@gmail.com>
References: <20200227.160024.1603714516899165010.horikyota.ntt@gmail.com>
<20200301195632.GA135286@rfd.leadboat.com>
<20200302.165353.1408159864331844170.horikyota.ntt@gmail.com>
<20200304.162919.898938381201316571.horikyota.ntt@gmail.com>
On Wed, Mar 04, 2020 at 04:29:19PM +0900, Kyotaro Horiguchi wrote:
> The attached is back-patches from 9.5 through master.
Thanks. I've made some edits. I'll plan to push the attached patches on
Friday or Saturday.
> log_newpage_range has been introduced at PG12. Fortunately the
> required infrastructure is introduced at PG9.5 so what I need to do
> for PG95-PG11 is back-patching the function and its counter part in
> xlog_redo. It doen't WAL format itself but XLOG_FPI gets to have 2 or
> more backup pages so the compatibility is forward only. That is, newer
> minor versions read WAL from older minor versions, but not vise
> versea. I'm not sure it is back-patchable so in the attached the
> end-of-xact WAL feature is separated for PG9.5-PG11.
> (000x-Add-end-of-xact-WAL-feature-of-WAL-skipping.patch)
The main patch's introduction of XLOG_GIST_ASSIGN_LSN already creates a WAL
upgrade hazard. Changing XLOG_FPI is riskier, because an old server will
apply the first FPI and ignore the rest. For v11 and earlier, I decided to
introduce XLOG_FPI_MULTI. It behaves exactly like XLOG_FPI, but this PANICs
if one reads post-update WAL with a pre-update server. The main alternative
would be to issue one XLOG_FPI per page, but I was concerned that would cause
a notable performance loss.
> In the patchset for 12, I let the functions heap_sync,
> heapam_methods.finish_bulk_insert and table_finish_bulk_insert left
> as-is. As the result heapam_finish_bulk_insert becomes no-op.
heapam_finish_bulk_insert() is a static function, so I deleted it.
> begin_heap_rewrite is a public function but the last parameter is
> useless and rather harmful as it looks as if it works. So I removed
> the parameter.
Agreed. Also, pgxn contains no references to begin_heap_rewrite().
> For 9.6, mdexists() creates the specified file while bootstrap mode
> and that leads to assertion failure of smgrDoPendingSyncs. So I made
> CreateStorage not register pending sync while bootstrap mode.
> gistbuild generates the LSN for root page of a newly created index
> using gistGetFakeLSN(heap), which fires assertion failure in
> gistGetFakeLSN. I think we should use index instead of heap there,
> but it doesn't matter if we don't have the new pending sync mechanism,
> so I didn't split it as a separate patch.
v11 and v10, too, had the gistGetFakeLSN(heap) problem. I saw that and other
problems by running the following on each branch:
make check-world
printf '%s\n%s\n%s\n' 'log_statement = all' 'wal_level = minimal' 'max_wal_senders = 0' >/tmp/minimal.conf
make check-world TEMP_CONFIG=/tmp/minimal.conf
make -C doc # catch breakage when XML changes don't work in SGML
> For 9.5, pg_visibility does not exist so I dropped the test for the
> module.
The test would have required further changes to work in v11 or earlier, so I
deleted the test. It was a low-importance test.
> It lacks a part of TAP infrastructure nowadays we have, but I
> want to have the test (and it actually found a bug I made during this
> work). So I added a patch to back-patch TestLib.pm, PostgresNode.pm
> and RecursiveCopy.pm along with 018_wal_optimize.pl.
> (0004-Add-TAP-test-for-WAL-skipping-feature.patch)
That is a good idea. Rather than make it specific to this test, I would like
to back-patch all applicable test files from 9.6 src/test/recovery. I'll plan
to push that one part on Thursday.
Other notable changes:
- Like you suggested earlier, I moved restoration of old*Subid from
index_create() back to ATExecAddIndex(). I decided to do this when I
observed that pg_idx_advisor calls index_create(). That's not a strong
reason, but it was enough to change a decision that had been arbitrary.
- Updated the wal_skip_threshold GUC category to WAL_SETTINGS, for consistency
with the documentation move. Added the GUC to postgresql.conf.sample.
- In released branches, I moved the new public struct fields to the end. This
reduces the number of extensions requiring a recompile. From a grep of
pgxn, one extension ("citus") relies on sizeof(RelationData), and nothing
relies on sizeof(IndexStmt).
- In 9.6, I rewrote the mdimmedsync() changes so the function never ignores
FileSync() failure.
Other observations:
- The new test file takes ~62s longer on 9.6 and 9.5, mostly due to commit
c61559ec3 first appearing in v10. I am fine with this.
- This is the most-demanding back-branch fix I've ever attempted. Hopefully
I've been smarter than usual while reviewing it, but that is unlikely.
Thanks,
nm
Attachments:
[application/x-tar-gz] skip-wal-v38nm.tar.gz (216.4K, ../20200316034647.GA1121601@rfd.leadboat.com/2-skip-wal-v38nm.tar.gz)
download
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: <20200316034647.GA1121601@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