Date: Fri, 1 Mar 2019 11:33:38 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Cc: Joe Conway <mail@joeconway.com>
Subject: Tighten error control for OpenTransientFile/CloseTransientFile
Message-ID: <20190301023338.GD1348@paquier.xyz>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="MIdTMoZhcV1D07fI"
Content-Disposition: inline
Precedence: bulk


--MIdTMoZhcV1D07fI
Content-Type: multipart/mixed; boundary="d9ADC0YsG2v16Js0"
Content-Disposition: inline


--d9ADC0YsG2v16Js0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

Hi all,

Joe's message here has reminded me that we have lacked a lot of error
handling around CloseTransientFile():
https://www.postgresql.org/message-id/c49b69ec-e2f7-ff33-4f17-0eaa4f2cef27@joeconway.com

This has been mentioned by Alvaro a couple of months ago (cannot find
the thread about that at quick glance), and I just forgot about it at
that time.  Anyway, attached is a patch to do some cleanup for all
that:
- Switch OpenTransientFile to read-only where sufficient.
- Add more error handling for CloseTransientFile
A major take of this patch is to make sure that the new error messages
generated have an elevel consistent with their neighbors.

Just on time for this last CF.  Thoughts?
--
Michael

--d9ADC0YsG2v16Js0
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="transient-fd-error-v1.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 ecd12fc53a..86b7f051a8 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3476,7 +3476,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..6b5b5116f7 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(ERROR,
+				(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(ERROR,
+				(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..c2bb0d96f2 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1377,7 +1377,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
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..60189c57d4 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -916,7 +916,14 @@ 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 file \"%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..a008e0e942 100644
--- a/src/common/controldata_utils.c
+++ b/src/common/controldata_utils.c
@@ -100,9 +100,17 @@ 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. */

--d9ADC0YsG2v16Js0--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAlx4mgIACgkQnvQgOdby
QH0XSw//T4+qeLvu/F6hhalhMTkwqSM1CTK9roRjsV6FoMu5r52m8KiG4rrhzphH
DdAqc+AMRlLuPrbnVKsZhHJ1uGi2kmAGVe+dZ0dNQMidGGEEXRYrzrdGwZAowHBY
i4pzEloYmwutdajP+MxXWX++vyBqHbMRRp5whUOmii/aqA8G6qKQ/3m8Y3uvmjgA
hEt5WiyQXZrAOFDaqZCAs8GvzJuiMIKIqwblhhjauy8SeNohnSCIFTeTS8uSgxIG
o/2K9+zVdOT+A5lU5s8mmGHJ16zQ2avuEAmfU9XRv4IvLq0jZrAbRM3q1lYtoVJu
C+/388W5+2TkdRJeD366Y/NzV4C8b1TTGh8Wkj2SbTMZyrndnmK8NjKFOXemvIUf
ctVWdfjSUoDMNcPwjGYkYDN7Rm4BNVZ/biNHaXavVG8NPJQVD41f4vs2uscJLBs1
pSlFxY4YaEIWFREyVVx7M8DHEIJ1rDr3BPxHDHwLMg8W1hBdsaIILPej9X5k8E1T
pnagvUv4YVc6g2b1eEbBfXtsKmUoKM1SOiIrrVRyjUM0IZFNE7SXAV6RBzRfXGsT
rVSqi4C7V/y6YjjPClkok94mD14sUUrd2h50JXZy75AJdzLS8BMUw4nG4sfES0gQ
vLNxoZPsBKe26yRWcP9RjWoVvVw6LU7NKahtDaV0VYBvATDkv+U=
=v9LB
-----END PGP SIGNATURE-----

--MIdTMoZhcV1D07fI--