Date: Thu, 7 Mar 2019 10:56:25 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Georgios Kokolatos <gkokolatos@pm.me>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: Tighten error control for OpenTransientFile/CloseTransientFile
Message-ID: <20190307015625.GC17293@paquier.xyz>
References: <20190301023338.GD1348@paquier.xyz>
 <20190302004022.GA1602@paquier.xyz>
 <155188409238.16480.11082213460365887952.pgcf@coridan.postgresql.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="mJm6k4Vb/yFcL9ZU"
Content-Disposition: inline
In-Reply-To: 
 <155188409238.16480.11082213460365887952.pgcf@coridan.postgresql.org>
Precedence: bulk


--mJm6k4Vb/yFcL9ZU
Content-Type: multipart/mixed; boundary="nmemrqcdn5VTmUEE"
Content-Disposition: inline


--nmemrqcdn5VTmUEE
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Wed, Mar 06, 2019 at 02:54:52PM +0000, Georgios Kokolatos wrote:
> Overall the patch looks good and according to the previous
> discussion fulfils its purpose.=20
>=20
> It might be worthwhile to also check for errors on close in
> SaveSlotToPath().

Thanks for the feedback, added.  I have spent some time
double-checking this stuff, and noticed that the new errors in
StartupReplicationOrigin() and CheckPointReplicationOrigin() should be
switched from ERROR to PANIC to be consistent.  One message in
dsm_impl_mmap() was not consistent either.

Are there any objections if I commit this patch?
--
Michael

--nmemrqcdn5VTmUEE
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="transient-fd-error-v2.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_s=
tat_statements/pg_stat_statements.c
index 9905593661..7b39283c89 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -1991,7 +1991,10 @@ qtext_load_file(Size *buffer_size)
 		return NULL;
 	}
=20
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(LOG,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", PGSS_TEXT_FILE)));
=20
 	*buffer_size =3D stat.st_size;
 	return buf;
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/hea=
p/rewriteheap.c
index f5cf9ffc9c..bce4274362 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1202,7 +1202,10 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
 				 errmsg("could not fsync file \"%s\": %m", path)));
 	pgstat_report_wait_end();
=20
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", path)));
 }
=20
 /* ---
@@ -1300,7 +1303,11 @@ CheckPointLogicalRewriteHeap(void)
 						(errcode_for_file_access(),
 						 errmsg("could not fsync file \"%s\": %m", path)));
 			pgstat_report_wait_end();
-			CloseTransientFile(fd);
+
+			if (CloseTransientFile(fd))
+				ereport(ERROR,
+						(errcode_for_file_access(),
+						 errmsg("could not close file \"%s\": %m", path)));
 		}
 	}
 	FreeDir(mappings_dir);
diff --git a/src/backend/access/transam/slru.c b/src/backend/access/transam=
/slru.c
index 3623352b9c..974d42fc86 100644
--- a/src/backend/access/transam/slru.c
+++ b/src/backend/access/transam/slru.c
@@ -599,7 +599,7 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
=20
 	SlruFileName(ctl, path, segno);
=20
-	fd =3D OpenTransientFile(path, O_RDWR | PG_BINARY);
+	fd =3D OpenTransientFile(path, O_RDONLY | PG_BINARY);
 	if (fd < 0)
 	{
 		/* expected: file doesn't exist */
@@ -621,7 +621,13 @@ SimpleLruDoesPhysicalPageExist(SlruCtl ctl, int pageno)
=20
 	result =3D endpos >=3D (off_t) (offset + BLCKSZ);
=20
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+	{
+		slru_errcause =3D SLRU_CLOSE_FAILED;
+		slru_errno =3D errno;
+		return false;
+	}
+
 	return result;
 }
=20
@@ -654,7 +660,7 @@ SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotn=
o)
 	 * SlruPhysicalWritePage).  Hence, if we are InRecovery, allow the case
 	 * where the file doesn't exist, and return zeroes instead.
 	 */
-	fd =3D OpenTransientFile(path, O_RDWR | PG_BINARY);
+	fd =3D OpenTransientFile(path, O_RDONLY | PG_BINARY);
 	if (fd < 0)
 	{
 		if (errno !=3D ENOENT || !InRecovery)
diff --git a/src/backend/access/transam/timeline.c b/src/backend/access/tra=
nsam/timeline.c
index c96c8b60ba..cbd9b2cee1 100644
--- a/src/backend/access/transam/timeline.c
+++ b/src/backend/access/transam/timeline.c
@@ -370,7 +370,11 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID par=
entTLI,
 			}
 			pgstat_report_wait_end();
 		}
-		CloseTransientFile(srcfd);
+
+		if (CloseTransientFile(srcfd))
+			ereport(ERROR,
+					(errcode_for_file_access(),
+					 errmsg("could not close file \"%s\": %m", path)));
 	}
=20
 	/*
@@ -416,7 +420,6 @@ writeTimeLineHistory(TimeLineID newTLI, TimeLineID pare=
ntTLI,
 				(errcode_for_file_access(),
 				 errmsg("could not close file \"%s\": %m", tmppath)));
=20
-
 	/*
 	 * Now move the completed history file into place with its final name.
 	 */
@@ -495,7 +498,6 @@ writeTimeLineHistoryFile(TimeLineID tli, char *content,=
 int size)
 				(errcode_for_file_access(),
 				 errmsg("could not close file \"%s\": %m", tmppath)));
=20
-
 	/*
 	 * Now move the completed history file into place with its final name.
 	 */
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/tra=
nsam/twophase.c
index 64679dd2de..21986e48fe 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1297,7 +1297,11 @@ ReadTwoPhaseFile(TransactionId xid, bool missing_ok)
 	}
=20
 	pgstat_report_wait_end();
-	CloseTransientFile(fd);
+
+	if (CloseTransientFile(fd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", path)));
=20
 	hdr =3D (TwoPhaseFileHeader *) buf;
 	if (hdr->magic !=3D TWOPHASE_MAGIC)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam=
/xlog.c
index 0fdd82a287..c7047738b6 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3469,7 +3469,10 @@ XLogFileCopy(XLogSegNo destsegno, TimeLineID srcTLI,=
 XLogSegNo srcsegno,
 				(errcode_for_file_access(),
 				 errmsg("could not close file \"%s\": %m", tmppath)));
=20
-	CloseTransientFile(srcfd);
+	if (CloseTransientFile(srcfd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", path)));
=20
 	/*
 	 * Now move the segment into place with its final name.
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 521236d7c9..f6293ab2ba 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -455,7 +455,11 @@ lo_import_internal(text *filename, Oid lobjOid)
 						fnamebuf)));
=20
 	inv_close(lobj);
-	CloseTransientFile(fd);
+
+	if (CloseTransientFile(fd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", fnamebuf)));
=20
 	return oid;
 }
@@ -524,7 +528,11 @@ be_lo_export(PG_FUNCTION_ARGS)
 							fnamebuf)));
 	}
=20
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", fnamebuf)));
+
 	inv_close(lobj);
=20
 	PG_RETURN_INT32(1);
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replica=
tion/logical/origin.c
index dad2b3d065..381e952ac3 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -658,7 +658,10 @@ CheckPointReplicationOrigin(void)
 						tmppath)));
 	}
=20
-	CloseTransientFile(tmpfd);
+	if (CloseTransientFile(tmpfd))
+		ereport(PANIC,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", tmppath)));
=20
 	/* fsync, rename to permanent file, fsync file and directory */
 	durable_rename(tmppath, path, PANIC);
@@ -793,7 +796,10 @@ StartupReplicationOrigin(void)
 				 errmsg("replication slot checkpoint has wrong checksum %u, expected %=
u",
 						crc, file_crc)));
=20
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(PANIC,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", path)));
 }
=20
 void
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/=
replication/logical/reorderbuffer.c
index 2b486b5e9f..2cfdf1c9ac 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -3360,7 +3360,10 @@ ApplyLogicalMappingFile(HTAB *tuplecid_data, Oid rel=
id, const char *fname)
 		}
 	}
=20
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", path)));
 }
=20
=20
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/repl=
ication/logical/snapbuild.c
index a59896f082..3e9d4cd79f 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1651,7 +1651,11 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr ls=
n)
 				 errmsg("could not fsync file \"%s\": %m", tmppath)));
 	}
 	pgstat_report_wait_end();
-	CloseTransientFile(fd);
+
+	if (CloseTransientFile(fd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", tmppath)));
=20
 	fsync_fname("pg_logical/snapshots", true);
=20
@@ -1846,7 +1850,10 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
 	}
 	COMP_CRC32C(checksum, ondisk.builder.committed.xip, sz);
=20
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", path)));
=20
 	FIN_CRC32C(checksum);
=20
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 33b23b6b6d..99a2cafbb4 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1315,7 +1315,10 @@ SaveSlotToPath(ReplicationSlot *slot, const char *di=
r, int elevel)
 	}
 	pgstat_report_wait_end();
=20
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(elevel,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", path)));
=20
 	/* rename to permanent file, fsync file and directory */
 	if (rename(tmppath, path) !=3D 0)
@@ -1377,7 +1380,7 @@ RestoreSlotFromDisk(const char *name)
=20
 	elog(DEBUG1, "restoring replication slot from \"%s\"", path);
=20
-	fd =3D OpenTransientFile(path, O_RDWR | PG_BINARY);
+	fd =3D OpenTransientFile(path, O_RDONLY | PG_BINARY);
=20
 	/*
 	 * We do not need to handle this as we are rename()ing the directory into
@@ -1477,7 +1480,10 @@ RestoreSlotFromDisk(const char *name)
 							path, readBytes, (Size) cp.length)));
 	}
=20
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(PANIC,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", path)));
=20
 	/* now verify the CRC */
 	INIT_CRC32C(checksum);
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/=
walsender.c
index 9b143f361b..4bb98ef352 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -521,7 +521,11 @@ SendTimeLineHistory(TimeLineHistoryCmd *cmd)
 		pq_sendbytes(&buf, rbuf.data, nread);
 		bytesleft -=3D nread;
 	}
-	CloseTransientFile(fd);
+
+	if (CloseTransientFile(fd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", path)));
=20
 	pq_endmessage(&buf);
 }
diff --git a/src/backend/storage/file/copydir.c b/src/backend/storage/file/=
copydir.c
index 1f766d20d1..342d078a8f 100644
--- a/src/backend/storage/file/copydir.c
+++ b/src/backend/storage/file/copydir.c
@@ -218,7 +218,10 @@ copy_file(char *fromfile, char *tofile)
 				(errcode_for_file_access(),
 				 errmsg("could not close file \"%s\": %m", tofile)));
=20
-	CloseTransientFile(srcfd);
+	if (CloseTransientFile(srcfd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", fromfile)));
=20
 	pfree(buffer);
 }
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 1ba0ddac10..fdac9850e0 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -646,7 +646,14 @@ durable_rename(const char *oldfile, const char *newfil=
e, int elevel)
 					 errmsg("could not fsync file \"%s\": %m", newfile)));
 			return -1;
 		}
-		CloseTransientFile(fd);
+
+		if (CloseTransientFile(fd))
+		{
+			ereport(elevel,
+					(errcode_for_file_access(),
+					 errmsg("could not close file \"%s\": %m", newfile)));
+			return -1;
+		}
 	}
=20
 	/* Time to do the real deal... */
@@ -3295,7 +3302,10 @@ pre_sync_fname(const char *fname, bool isdir, int el=
evel)
 	 */
 	pg_flush_data(fd, 0, 0);
=20
-	(void) CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(elevel,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", fname)));
 }
=20
 #endif							/* PG_FLUSH_DATA_WORKS */
@@ -3394,7 +3404,13 @@ fsync_fname_ext(const char *fname, bool isdir, bool =
ignore_perm, int elevel)
 		return -1;
 	}
=20
-	(void) CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+	{
+		ereport(elevel,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m", fname)));
+		return -1;
+	}
=20
 	return 0;
 }
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/d=
sm_impl.c
index aeda32c9c5..a22c7928e7 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -916,7 +916,15 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size reque=
st_size,
 	}
 	*mapped_address =3D address;
 	*mapped_size =3D request_size;
-	CloseTransientFile(fd);
+
+	if (CloseTransientFile(fd))
+	{
+		ereport(elevel,
+				(errcode_for_file_access(),
+				 errmsg("could not close shared memory segment \"%s\": %m",
+						name)));
+		return false;
+	}
=20
 	return true;
 }
diff --git a/src/backend/utils/cache/relmapper.c b/src/backend/utils/cache/=
relmapper.c
index 5e61d908fd..3e676d55b2 100644
--- a/src/backend/utils/cache/relmapper.c
+++ b/src/backend/utils/cache/relmapper.c
@@ -747,7 +747,11 @@ load_relmap_file(bool shared)
 	}
 	pgstat_report_wait_end();
=20
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m",
+						mapfilename)));
=20
 	/* check for correct magic number, etc */
 	if (map->magic !=3D RELMAPPER_FILEMAGIC ||
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index 000f3c66d6..9ebb90b844 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -100,9 +100,18 @@ get_controlfile(const char *DataDir, const char *progn=
ame, bool *crc_ok_p)
 	}
=20
 #ifndef FRONTEND
-	CloseTransientFile(fd);
+	if (CloseTransientFile(fd))
+		ereport(ERROR,
+				(errcode_for_file_access(),
+				 errmsg("could not close file \"%s\": %m",
+						ControlFilePath)));
 #else
-	close(fd);
+	if (close(fd))
+	{
+		fprintf(stderr, _("%s: could not close file \"%s\": %s"),
+				progname, ControlFilePath, strerror(errno));
+		exit(EXIT_FAILURE);
+	}
 #endif
=20
 	/* Check the CRC. */

--nmemrqcdn5VTmUEE--

--mJm6k4Vb/yFcL9ZU
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlyAekkACgkQnvQgOdby
QH3fhBAAi5Ixx+inVwmTPjtf3LIL728b1K3sBkg96l3C2yZGdu8BzCs9mSxwwRPj
9GtuMPGZx5y4sIbbRRY1VpH1IOUBungKM4alYB4bL17X/p0hvcrhvL8KlZZu8hEl
GqLw3nXAxhLFHezWUMUmh/YYx4A2oXV+G29P/4cA4rSjk9yIFdlS98RNxrwuppju
GSYBa3QEkPd4+Bxm9du/n4GaFfooomuh9POSXj81Amaqe3int08ghWQUb80Gaw7b
atjQDXZ8U7XPaXPoTcJpmThdaqct5KFLRqBX6wFphxgKbNdh0dSvEIEidLpaXJRT
CAfHQW8wIoFuzy0icPv+JC2P/oNM+QYKqYY1edLBk8SoSULuCH11C5MGRg4X28ED
BJelIOlKI6goIJHZBQrrfqa0UsY4QMa/XCa8cFHVutUYEEUDZBDJeXlXIvZQ8uAz
wPNpI+g5qgh3XQfuHKaoQ2Z3no6A9n6WxvV6uehUcI7Z1i/trMqR/fPkUVWiHXMa
E9lvs52oV9woYzszM/kun4aqPxB4HffDReUhd9vwSO7vdiMjJ0MExbS2icY+RILH
STxRZEhth6qFJOmNNl8n0KPwMJbbITFSLbX0MbNSsNBzeVlY65m4eGxAzbwd64G2
EbdcTe0jL5KICYcRW9eIuXp2jB1ym0qwq3MzjZ0dA638Kc5kw78=
=Sase
-----END PGP SIGNATURE-----

--mJm6k4Vb/yFcL9ZU--