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
Date: Sat, 4 Aug 2018 21:18:34 +0900
Message-ID: <20180804121834.GG20967@paquier.xyz> (raw)
In-Reply-To: <31797.1533326676@sss.pgh.pa.us>
References: <E1fXHAd-0004Qv-8y@gemulon.postgresql.org>
<31797.1533326676@sss.pgh.pa.us>
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
Attachments:errno-write-init.patch (text/x-diff, 5.0K) downloaddiff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.cindex 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 = xlrec->num_mappings * sizeof(LogicalRewriteMappingData);
/* write out tail end of mapping file (again) */
+ errno = 0;
pgstat_report_wait_start(WAIT_EVENT_LOGICAL_REWRITE_MAPPING_WRITE);
if (write(fd, data, len) != len)
{
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.cindex 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)));
/* Write content and CRC */
+ errno = 0;
pgstat_report_wait_start(WAIT_EVENT_TWOPHASE_FILE_WRITE);
if (write(fd, content, len) != len)
{
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.cindex 822c96d1c2..bf97dcdee4 100644--- a/src/backend/replication/logical/origin.c+++ b/src/backend/replication/logical/origin.c@@ -576,6 +576,7 @@ CheckPointReplicationOrigin(void)
tmppath)));
/* write magic */
+ errno = 0;
if ((write(tmpfd, &magic, sizeof(magic))) != sizeof(magic))
{
int save_errno = errno;
@@ -619,6 +620,7 @@ CheckPointReplicationOrigin(void)
/* make sure we only write out a commit that's persistent */
XLogFlush(local_lsn);
+ errno = 0;
if ((write(tmpfd, &disk_state, sizeof(disk_state))) !=
sizeof(disk_state))
{
@@ -641,6 +643,7 @@ CheckPointReplicationOrigin(void)
/* write out the CRC */
FIN_CRC32C(crc);
+ errno = 0;
if ((write(tmpfd, &crc, sizeof(crc))) != sizeof(crc))
{
int save_errno = errno;
diff --git a/src/backend/replication/logical/reorderbuffer.c b/src/backend/replication/logical/reorderbuffer.cindex 9b55b94227..1d43a165ad 100644--- a/src/backend/replication/logical/reorderbuffer.c+++ b/src/backend/replication/logical/reorderbuffer.c@@ -2421,6 +2421,7 @@ ReorderBufferSerializeChange(ReorderBuffer *rb, ReorderBufferTXN *txn,
ondisk->size = sz;
+ errno = 0;
pgstat_report_wait_start(WAIT_EVENT_REORDER_BUFFER_WRITE);
if (write(fd, rb->outbuf, ondisk->size) != ondisk->size)
{
diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.cindex 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)));
+ errno = 0;
pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_WRITE);
if ((write(fd, ondisk, needed_length)) != needed_length)
{
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.cindex 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);
+ errno = 0;
pgstat_report_wait_start(WAIT_EVENT_REPLICATION_SLOT_WRITE);
if ((write(fd, &cp, sizeof(cp))) != sizeof(cp))
{
diff --git a/src/bin/pg_basebackup/walmethods.c b/src/bin/pg_basebackup/walmethods.cindex 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 *temp_suffix, size_t pad_to_
zerobuf = pg_malloc0(XLOG_BLCKSZ);
for (bytes = 0; bytes < pad_to_size; bytes += XLOG_BLCKSZ)
{
+ errno = 0;
if (write(fd, zerobuf, XLOG_BLCKSZ) != XLOG_BLCKSZ)
{
int save_errno = errno;
@@ -445,6 +446,7 @@ tar_write_compressed_data(void *buf, size_t count, bool flush)
{
size_t len = ZLIB_OUT_SIZE - tar_data->zp->avail_out;
+ errno = 0;
if (write(tar_data->fd, tar_data->zlibOut, len) != len)
{
/*
@@ -629,6 +631,7 @@ tar_open_for_write(const char *pathname, const char *temp_suffix, size_t pad_to_
if (!tar_data->compression)
{
+ errno = 0;
if (write(tar_data->fd, tar_data->currentfile->header, 512) != 512)
{
save_errno = errno;
@@ -829,6 +832,7 @@ tar_close(Walfile f, WalCloseMethod method)
return -1;
if (!tar_data->compression)
{
+ errno = 0;
if (write(tar_data->fd, tf->header, 512) != 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 = 0;
if (write(tar_data->fd, zerobuf, sizeof(zerobuf)) != sizeof(zerobuf))
{
/* if write didn't set errno, assume problem is no disk space */
@@ -933,6 +938,7 @@ tar_finish(void)
{
size_t len = ZLIB_OUT_SIZE - tar_data->zp->avail_out;
+ errno = 0;
if (write(tar_data->fd, tar_data->zlibOut, len) != len)
{
/*
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: michael@paquier.xyz, tgl@sss.pgh.pa.us, pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Address set of issues with errno handling
In-Reply-To: <20180804121834.GG20967@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox