Date: Sat, 4 Aug 2018 21:18:34 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Address set of issues with errno handling
Message-ID: <20180804121834.GG20967@paquier.xyz>
References: <E1fXHAd-0004Qv-8y@gemulon.postgresql.org>
 <31797.1533326676@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="/Gk0KcsbyUMelFU1"
Content-Disposition: inline
In-Reply-To: <31797.1533326676@sss.pgh.pa.us>
Precedence: bulk


--/Gk0KcsbyUMelFU1
Content-Type: multipart/mixed; boundary="doKZ0ri6bHmN2Q5y"
Content-Disposition: inline


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

On Fri, Aug 03, 2018 at 04:04:36PM -0400, Tom Lane wrote:
> That's not good enough, because there is no reason to suppose that errno
> is initially zero; in reality it'll be whatever was left over from the
> last failed syscall, perhaps far distant from here.  The places that do
> this correctly do it like so:
>
> [... code ...]

Yes, I have visibly assumed that the last syscall was already failing
so those conditions would not be hit.  If the code gets changed again
that would be a problem.

> You need to go back and add the pre-clearing of errno in each of these
> places, otherwise the added code is basically useless.

I looked at all code paths enforcing ENOSPC on write() calls, and
attached is a patch to address this issue for all of them.  What do you
think?  I can of course get that addressed before the next set of minor
releases.
--
Michael

--doKZ0ri6bHmN2Q5y
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="errno-write-init.patch"
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/hea=
p/rewriteheap.c
index ed7ba181c7..85f92973c9 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -1166,6 +1166,7 @@ heap_xlog_logical_rewrite(XLogReaderState *r)
 	len =3D xlrec->num_mappings * sizeof(LogicalRewriteMappingData);
=20
 	/* write out tail end of mapping file (again) */
+	errno =3D 0;
 	pgstat_report_wait_start(WAIT_EVENT_LOGICAL_REWRITE_MAPPING_WRITE);
 	if (write(fd, data, len) !=3D len)
 	{
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/tra=
nsam/twophase.c
index 306861bb79..0ae0722794 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1669,6 +1669,7 @@ RecreateTwoPhaseFile(TransactionId xid, void *content=
, int len)
 				 errmsg("could not recreate file \"%s\": %m", path)));
=20
 	/* Write content and CRC */
+	errno =3D 0;
 	pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_WRITE);
 	if (write(fd, content, len) !=3D len)
 	{
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replica=
tion/logical/origin.c
index 822c96d1c2..bf97dcdee4 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -576,6 +576,7 @@ CheckPointReplicationOrigin(void)
 						tmppath)));
=20
 	/* write magic */
+	errno =3D 0;
 	if ((write(tmpfd, &magic, sizeof(magic))) !=3D sizeof(magic))
 	{
 		int			save_errno =3D errno;
@@ -619,6 +620,7 @@ CheckPointReplicationOrigin(void)
 		/* make sure we only write out a commit that's persistent */
 		XLogFlush(local_lsn);
=20
+		errno =3D 0;
 		if ((write(tmpfd, &disk_state, sizeof(disk_state))) !=3D
 			sizeof(disk_state))
 		{
@@ -641,6 +643,7 @@ CheckPointReplicationOrigin(void)
=20
 	/* write out the CRC */
 	FIN_CRC32C(crc);
+	errno =3D 0;
 	if ((write(tmpfd, &crc, sizeof(crc))) !=3D sizeof(crc))
 	{
 		int			save_errno =3D errno;
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/=
replication/logical/reorderbuffer.c
index 9b55b94227..1d43a165ad 100644
--- a/src/backend/replication/logical/reorderbuffer.c
+++ b/src/backend/replication/logical/reorderbuffer.c
@@ -2421,6 +2421,7 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, Reord=
erBufferTXN *txn,
=20
 	ondisk->size =3D sz;
=20
+	errno =3D 0;
 	pgstat_report_wait_start(WAIT_EVENT_REORDER_BUFFER_WRITE);
 	if (write(fd, rb->outbuf, ondisk->size) !=3D ondisk->size)
 	{
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/repl=
ication/logical/snapbuild.c
index 1359d9b20a..a6cd6c67d1 100644
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -1609,6 +1609,7 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
 		ereport(ERROR,
 				(errmsg("could not open file \"%s\": %m", path)));
=20
+	errno =3D 0;
 	pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_WRITE);
 	if ((write(fd, ondisk, needed_length)) !=3D needed_length)
 	{
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 6c36398058..19978d9a9e 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1275,6 +1275,7 @@ SaveSlotToPath(ReplicationSlot *slot, const char *dir=
, int elevel)
 				SnapBuildOnDiskChecksummedSize);
 	FIN_CRC32C(cp.checksum);
=20
+	errno =3D 0;
 	pgstat_report_wait_start(WAIT_EVENT_REPLICATION_SLOT_WRITE);
 	if ((write(fd, &cp, sizeof(cp))) !=3D sizeof(cp))
 	{
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/wal=
methods.c
index fbfee05a5a..42e3f3023a 100644
--- a/src/bin/pg_basebackup/walmethods.c
+++ b/src/bin/pg_basebackup/walmethods.c
@@ -122,6 +122,7 @@ dir_open_for_write(const char *pathname, const char *te=
mp_suffix, size_t pad_to_
 		zerobuf =3D pg_malloc0(XLOG_BLCKSZ);
 		for (bytes =3D 0; bytes < pad_to_size; bytes +=3D XLOG_BLCKSZ)
 		{
+			errno =3D 0;
 			if (write(fd, zerobuf, XLOG_BLCKSZ) !=3D XLOG_BLCKSZ)
 			{
 				int			save_errno =3D errno;
@@ -445,6 +446,7 @@ tar_write_compressed_data(void *buf, size_t count, bool=
 flush)
 		{
 			size_t		len =3D ZLIB_OUT_SIZE - tar_data->zp->avail_out;
=20
+			errno =3D 0;
 			if (write(tar_data->fd, tar_data->zlibOut, len) !=3D len)
 			{
 				/*
@@ -629,6 +631,7 @@ tar_open_for_write(const char *pathname, const char *te=
mp_suffix, size_t pad_to_
=20
 	if (!tar_data->compression)
 	{
+		errno =3D 0;
 		if (write(tar_data->fd, tar_data->currentfile->header, 512) !=3D 512)
 		{
 			save_errno =3D errno;
@@ -829,6 +832,7 @@ tar_close(Walfile f, WalCloseMethod method)
 		return -1;
 	if (!tar_data->compression)
 	{
+		errno =3D 0;
 		if (write(tar_data->fd, tf->header, 512) !=3D 512)
 		{
 			/* if write didn't set errno, assume problem is no disk space */
@@ -901,6 +905,7 @@ tar_finish(void)
 	MemSet(zerobuf, 0, sizeof(zerobuf));
 	if (!tar_data->compression)
 	{
+		errno =3D 0;
 		if (write(tar_data->fd, zerobuf, sizeof(zerobuf)) !=3D sizeof(zerobuf))
 		{
 			/* if write didn't set errno, assume problem is no disk space */
@@ -933,6 +938,7 @@ tar_finish(void)
 			{
 				size_t		len =3D ZLIB_OUT_SIZE - tar_data->zp->avail_out;
=20
+				errno =3D 0;
 				if (write(tar_data->fd, tar_data->zlibOut, len) !=3D len)
 				{
 					/*

--doKZ0ri6bHmN2Q5y--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAltlmZoACgkQnvQgOdby
QH3hJRAAkKbkxqV8d1LN5Dtv6BzRyghaoddOYRlqbP9yCjYff6yDl2N3IWeW6kWR
xYE+sW4tfDHZFP2HwZmY2J0dKJYvO5USH8F/fUZ/J8KNUVbHzxEMZSRSMhYwwxsN
JAvFl+5qPxrhFJFP7VSBlAy4fT+ySI6bPhLv0dUHzqUab5Ap8nEy5bPffNCdR83O
3+Pm/liFCk3B56mDupXfs/LrkBspKqyOphhvy6W6pXa2cilf+hjPo1eGag4s1C05
90VQiX3Hx+L9FrCUkjYimqQHvdJIewPnvCB+ihfo6CP+mhgOiMyKBTJ90E2Cyddo
uzmuUCKP4+sfSZfiTOylYopXDiAjCuYg/sfA5IUEbQuZS7AjlT/wfKoCz+yklYIB
oUOLZFygpTrYjhNYWroz6sObRvy/Y52Fh2wXt9vBxtYqSf5f4RQBt+2e6GpB4bBV
gyJcJac+7we9Ajn2WflgtDuTOroq9mteGRDJpRIjq8Sc6F9nnXn/6zz3goSzhRyw
mY3I/M6LSyLmR/NjCDvTybt3q6c0mfUOgBH+YTwOJwHgLz2vXCl2aUz7p7M13lE5
dsnGQ3hYPpvAcITSjJtUYq6mAoBvDJw/JzEKER+26Y/jrii2QaMc8SmxEwE472TD
3d27uGr5buPLLoYlslhaRvmDnOZYPvldS9VI1HNA/58+e9HQS2I=
=6obx
-----END PGP SIGNATURE-----

--/Gk0KcsbyUMelFU1--