Date: Fri, 13 Nov 2020 16:47:48 +0900 (JST)
Message-Id: <20201113.164748.2087059631966193679.horikyota.ntt@gmail.com>
To: tsunakawa.takay@fujitsu.com
Cc: osumi.takamichi@fujitsu.com, sfrost@snowman.net,
 masao.fujii@oss.nttdata.com, ashutosh.bapat.oss@gmail.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: 
 <TYAPR01MB2990D25E5CE77A095ADC90A3FEE60@TYAPR01MB2990.jpnprd01.prod.outlook.com>
References: <20201111.173317.460890039962481381.horikyota.ntt@gmail.com>
	<20201113.132201.148696021363404735.horikyota.ntt@gmail.com>
	<TYAPR01MB2990D25E5CE77A095ADC90A3FEE60@TYAPR01MB2990.jpnprd01.prod.outlook.com>
User-Agent: Mew version 6.8 on Emacs 26.1
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Precedence: bulk

At Fri, 13 Nov 2020 06:43:13 +0000, "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com> wrote in 
> Hi Horiguchi-san,
> 
> 
> Thank you for making a patch so quickly.  I've started looking at it.
> 
> What makes you think this is a PoC?  Documentation and test cases?  If there's something you think that doesn't work or are concerned about, can you share it?

The latest version is heavily revised and is given much comment so it
might have exited from PoC state.  The necessity of documentation is
doubtful since this patch doesn't user-facing behavior other than
speed. Some tests are required especialy about recovery and
replication perspective but I haven't been able to make it. (One of
the tests needs to cause crash while a transaction is running.)

> Do you know the reason why data copy was done before?  And, it may be odd for me to ask this, but I think I saw someone referred to the past discussion that eliminating data copy is difficult due to some processing at commit.  I can't find it.

To imagine that, just because it is simpler considering rollback and
code sharing, and maybe no one have been complained that SET
LOGGED/UNLOGGED looks taking a long time than required/expected.

The current implement is simple.  It's enough to just discard old or
new relfilenode according to the current transaction ends with commit
or abort. Tweaking of relfilenode under use leads-in some skews in
some places.  I used pendingDelete mechanism a bit complexified way
and a violated an abstraction (I think, calling AM-routines from
storage.c is not good.) and even introduce a new fork kind only to
mark a init fork as "not committed yet".  There might be better way,
but I haven't find it.

(The patch scans all shared buffer blocks for each relation).

> (1)
> @@ -168,6 +168,8 @@ extern PGDLLIMPORT int32 *LocalRefCount;
>   */
>  #define BufferGetPage(buffer) ((Page)BufferGetBlock(buffer))
>  
> +struct SmgrRelationData;
> 
> This declaration is already in the file:
> 
> /* forward declared, to avoid having to expose buf_internals.h here */
> struct WritebackContext;
> 
> /* forward declared, to avoid including smgr.h here */
> struct SMgrRelationData;

Hmmm. Nice chatch. And will fix in the next version.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center