Date: Thu, 24 Aug 2023 11:22:32 +0900 (JST)
Message-Id: <20230824.112232.1895676924995181515.horikyota.ntt@gmail.com>
To: nathandbossart@gmail.com
Cc: jakub.wartak@enterprisedb.com, stark.cfm@gmail.com, hlinnaka@iki.fi,
 barwick@gmail.com, jchampion@timescale.com, pryzby@telsasoft.com,
 tgl@sss.pgh.pa.us, rjuju123@gmail.com, jakub.wartak@tomtom.com,
 pgsql-hackers@lists.postgresql.org
Subject: Re: In-placre persistance change of a relation
From: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
In-Reply-To: <20230814193848.GA1573015@nathanxps13>
References: <CAKZiRmxCn3QZWXuGfbB0zVMR0A4sHR7sugczHCaEpuWDp6BTRA@mail.gmail.com>
	<20230428.105853.743427640418401945.horikyota.ntt@gmail.com>
	<20230814193848.GA1573015@nathanxps13>
User-Agent: Mew version 6.8 on Emacs 27.2
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/20230824.112232.1895676924995181515.horikyota.ntt%40gmail.com>
Precedence: bulk

Thank you for looking this!

At Mon, 14 Aug 2023 12:38:48 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> I think there are some good ideas here.  I started to take a look at the
> patches, and I've attached a rebased version of the patch set.  Apologies
> if I am repeating any discussions from upthread.
> 
> First, I tested the time difference in ALTER TABLE SET UNLOGGED/LOGGED with
> the patch applied, and the results looked pretty impressive.
> 
> 	before:
> 	postgres=# alter table test set unlogged;
> 	ALTER TABLE
> 	Time: 5108.071 ms (00:05.108)
> 	postgres=# alter table test set logged;
> 	ALTER TABLE
> 	Time: 6747.648 ms (00:06.748)
> 
> 	after:
> 	postgres=# alter table test set unlogged;
> 	ALTER TABLE
> 	Time: 25.609 ms
> 	postgres=# alter table test set logged;
> 	ALTER TABLE
> 	Time: 1241.800 ms (00:01.242)

Thanks for confirmation. The difference between the both directions is
that making a table logged requires to emit WAL records for the entire
content.

> My first question is whether 0001 is a prerequisite to 0002.  I'm assuming
> it is, but the reason wasn't immediately obvious to me.  If it's just

In 0002, if a backend crashes after creating an init fork file but
before the associated commit, a lingering fork file could result in
data loss on the next startup. Thus, an utterly reliable file cleanup
mechanism is essential. 0001 also addresses the orphan storage files
issue arising from ALTER TABLE and similar commands.

> nice-to-have, perhaps we could simplify the patch set a bit.  I see that
> Heikki had some general concerns with the marker file approach [0], so
> perhaps it is at least worth brainstorming some alternatives if we _do_
> need it.

The rationale behind the file-based implementation is that any
leftover init fork file from a crash needs to be deleted before the
reinit(INIT) process kicks in, which happens irrelevantly to WAL,
before the start of crash recovery. I could implement it separately
from the reinit module, but I didn't since that results in almost a
duplication.

As commented in xlog.c, the purpose of the pre-recovery reinit CLEANUP
phase is to ensure hot standbys don't encounter erroneous unlogged
relations.  Based on that requirement, we need a mechanism to
guarantee that additional crucial operations are executed reliably at
the next startup post-crash, right before recovery kicks in (or reinit
CLEANUP). 0001 persists this data on a per-operation basis tightly
bonded to their target objects.

I could turn this into something like undo longs in a simple form, but
I'd rather not craft a general-purpose undo log system for this unelss
it's absolutely necessary.


> [0] https://postgr.es/m/9827ebd3-de2e-fd52-4091-a568387b1fc2%40iki.fi

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center