From: Jakub Wartak <Jakub.Wartak@tomtom.com>
To: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
CC: "tsunakawa.takay@fujitsu.com" <tsunakawa.takay@fujitsu.com>,
	"osumi.takamichi@fujitsu.com" <osumi.takamichi@fujitsu.com>,
	"sfrost@snowman.net" <sfrost@snowman.net>, "masao.fujii@oss.nttdata.com"
	<masao.fujii@oss.nttdata.com>, "ashutosh.bapat.oss@gmail.com"
	<ashutosh.bapat.oss@gmail.com>, "pgsql-hackers@lists.postgresql.org"
	<pgsql-hackers@lists.postgresql.org>
Subject: RE: In-placre persistance change of a relation
Thread-Topic: In-placre persistance change of a relation
Thread-Index: 
 AQHWuAVeo56RaA8LA0Wy6+ngHR+ctqnFeQaAgAAnc4CAAAkSgIAAEt0AgCD3ggCAH3JAAIABDyoAgBZeBICAADPEgIAGW7QAgAMMrYCAbcolgIAAAfKAgaPXqwCABJJJgIAACYDQ
Date: Mon, 20 Dec 2021 07:59:29 +0000
Message-ID: 
 <AM8PR07MB824804C5C4DEDE7FA5121ACFF67B9@AM8PR07MB8248.eurprd07.prod.outlook.com>
References: <20210325.140805.1356865766630580851.horikyota.ntt@gmail.com>
	<20210325.141503.245781197327278199.horikyota.ntt@gmail.com>
	<AM8PR07MB8248217A3AFEC0DD4D59ED83F6789@AM8PR07MB8248.eurprd07.prod.outlook.com>
 <20211220.152823.1386247767464518843.horikyota.ntt@gmail.com>
In-Reply-To: <20211220.152823.1386247767464518843.horikyota.ntt@gmail.com>
Accept-Language: en-US
Content-Language: pl-PL
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?uHG08JQKHbJm5PV3V1qc3xJAwwsXMqHR/LarOtdwF8ZIRbK6DiiB14vnukfF?=
 =?us-ascii?Q?qC6OjadyAQT/2nDgYVKyeN3zP1E0JhbBv1KsCmD1wetUhs7gmbPPgU/plwUV?=
 =?us-ascii?Q?D4ZhDDuDtNNJozJl/OQkdX+WuEji4KFp4Xoigq+4EgBqD34qc6LbGRcR7AEQ?=
 =?us-ascii?Q?WQDVUdjdA0/q57pwK9Yvt9id1LYM4XbzTSpwN12Akj/zJwLPGQDFpjHGUNJh?=
 =?us-ascii?Q?a65S5hrzVE4vi7r3MtMd6wLpig3kbneEWbk7xlm0wv7Xsbw4Z+IwKxFnYd6L?=
 =?us-ascii?Q?ZJ8vVAMpvdzaybpHwC7lS9FJL9K5FsgpqMTwcBHkotenOpwPBQDj5dztvCNQ?=
 =?us-ascii?Q?cuvVr4n2kTn1iIVUiRHdr/ycmxlSdrsgVklF/1edT7aqKtwibVrROW2M6Edg?=
 =?us-ascii?Q?jzlUe5jFYSmCjKvPQy0toQB2iR8iGAwTyj5qWrAouNKdhlvZ4bSQxuNDt2Sh?=
 =?us-ascii?Q?w7yUVH+0wsaiZv8tTuLqeaA4OMn09K5drtJ9LumBXz+65iOAwoBwtRRxTArM?=
 =?us-ascii?Q?xAzSaWfryrA1yarnmdm3Ojpb9mBmeOwcJZYHPKAVo8koV87vXER2yYM4D8+C?=
 =?us-ascii?Q?AS0F8MceBviIELnrxf7ECloecUg3hmmiqGbHJTWrLT9FF+FN9siOn0YJIYkM?=
 =?us-ascii?Q?IgZVDxZ/fkmmMwKky0fPweVl/pnnWhe1LTlN9FDP1ZaLLxcCJE8d9su8rxzi?=
 =?us-ascii?Q?7FFcFSGPokC86wOQ0YmxzPJq3EJn0zG+7bea8iakvlUbVu0j1avnr61cfJZ5?=
 =?us-ascii?Q?k4MrhN8wStq4uLk5QStOno1c5vjMwUeqJ4cyYWlmNRTEMd2rm7vYpK7N02mH?=
 =?us-ascii?Q?G4k4Zy5mNRfQIEBzVTCubXhMQ25whayWqQpKa8BLOW9isguuNH3h8Prsy0oh?=
 =?us-ascii?Q?ShiEG5H7lCLEmD+eVmqEXvrkRhPodkmhnqqBvBFd0/dlbBMkwRFE/RDlwTPq?=
 =?us-ascii?Q?Q/X6uKX8A2WTeYZoRvfA5gmrpE1aLIMSKcAdrdCvzc5Mmjr/UJhFCORZQhtN?=
 =?us-ascii?Q?F9BZgMrNlwhMr4V/dH+QaXG+j+lxJiylMpFiA/AyC0MjpTgQ+nxwPS/Q0WqH?=
 =?us-ascii?Q?Ltr3rIfNypuyRSBBc+z63ni1ym2siiY/PeFu5pFCC0vf88D4w58xroDS53vR?=
 =?us-ascii?Q?8ihruMZ6Xo29rkrJXkMzQSqEyRp3hLwXP1q0L9kDYGWFZj80FCLhN1CizHs7?=
 =?us-ascii?Q?aa6iHvcZQnnqHRYheyViIp9L0zVjwADXCWWnrxluqyDoFBPR2WfysVtrKjUy?=
 =?us-ascii?Q?nL0OsLG1KBGUdgGe4TbnmhqInjSenGSreLNjy+tvjCMbf9/PHVZdc+zQzqQI?=
 =?us-ascii?Q?PLb864jA2TsPrh7M1KG4fJ2pQrcPQ8/fdvy9vTL5DeOh0Ks9YqWLkiBo7Z+y?=
 =?us-ascii?Q?gkBzpKiKrMpz8EmcSCDQuUe71DHYC9xACLhMFr7zjJmMcngcNasFIvMtc+h5?=
 =?us-ascii?Q?aXPAtPM0uedHIE/C7EXOpO8RBqd3zv5CXb0VaFCl0/YeeEFnI2BbJnWiDbHr?=
 =?us-ascii?Q?CgaFR863r++irR/802SFchYM6cbgH43YFH94rcCC7h7jG6uWz8MB54XGsrV1?=
 =?us-ascii?Q?31uhYdTYR8x0sZ3EkTDuU3nH5fLIYnX3GZ6u/4/yzZiG1Fcmn+AZfRHwgR7K?=
 =?us-ascii?Q?mw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: tomtom.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AM8PR07MB8248.eurprd07.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 
 fd50cf55-6ce3-4c27-1b1a-08d9c38ea6cb
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Dec 2021 07:59:29.9643
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 374f8026-7b54-4a3a-b87d-328fa26ec10d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 
 nLj/nIM3iS/she/Qkjel6eI3ZOcH767ih6G3t63j29xzPGEQ/ZlU8c9L4GVWqYE9uOIzvmiPBnmwSFcY6d2buw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0701MB3015
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: 
 <https://www.postgresql.org/message-id/AM8PR07MB824804C5C4DEDE7FA5121ACFF67B9%40AM8PR07MB8248.eurprd07.prod.outlook.com>
Precedence: bulk

Hi Kyotaro, I'm glad you are still into this

> I didn't register for some reasons.

Right now in v8 there's a typo in ./src/backend/catalog/storage.c :

storage.c: In function 'RelationDropInitFork':
storage.c:385:44: error: expected statement before ')' token
    pending->unlink_forknum !=3D INIT_FORKNUM)) <-- here, one ) too much

> 1. I'm not sure that we want to have the new mark files.

I can't help with such design decision, but if there are doubts maybe then =
add checking return codes around:
a) pg_fsync() and fsync_parent_path() (??) inside mdcreatemark()
b) mdunlinkmark() inside mdunlinkmark()
and PANIC if something goes wrong?

> 2. Aside of possible bugs, I'm not confident that the crash-safety of
>  this patch is actually water-tight. At least we need tests for some
>  failure cases.
>
> 3. As mentioned in transam/README, failure in removing smgr mark files
>    leads to immediate shut down.  I'm not sure this behavior is acceptabl=
e.

Doesn't it happen for most of the stuff already? There's even data_sync_ret=
ry GUC.

> 4. Including the reasons above, this is not fully functionally.
>    For example, if we execute the following commands on primary,
>    replica dones't work correctly. (boom!)
>=20
>    =3D# CREATE UNLOGGED TABLE t (a int);
>    =3D# ALTER TABLE t SET LOGGED;
>=20
>=20
> The following fixes are done in the attched v8.
>=20
> - Rebased. Referring to Jakub and Justin's work, I replaced direct
>   access to ->rd_smgr with RelationGetSmgr() and removed calls to
>   RelationOpenSmgr(). I still separate the "ALTER TABLE ALL IN
>   TABLESPACE SET LOGGED/UNLOGGED" statement part.
>=20
> - Fixed RelationCreate/DropInitFork's behavior for non-target
>   relations. (From Jakub's work).
>=20
> - Fixed wording of some comments.
>=20
> - As revisited, I found a bug around recovery. If the logged-ness of a
>   relation gets flipped repeatedly in a transaction, duplicate
>   pending-delete entries are accumulated during recovery and work in a
>   wrong way. sgmr_redo now adds up to one entry for a action.
>=20
> - The issue 4 above is not fixed (yet).

Thanks again, If you have any list of crush tests ideas maybe I'll have som=
e minutes=20
to try to figure them out. Is there is any goto list of stuff to be checked=
 to add confidence
to this patch (as per point #2) ?

BTW fast feedback regarding that ALTER patch  (there were 4 unlogged tables=
):
#  ALTER TABLE ALL IN TABLESPACE tbs1 set logged;
WARNING:  unrecognized node type: 349

-J.