Skip to content

Commit 20428d3

Browse files
peterepetere
committed
Add BufFileRead variants with short read and EOF detection
Most callers of BufFileRead() want to check whether they read the full specified length. Checking this at every call site is very tedious. This patch provides additional variants BufFileReadExact() and BufFileReadMaybeEOF() that include the length checks. I considered changing BufFileRead() itself, but this function is also used in extensions, and so changing the behavior like this would create a lot of problems there. The new names are analogous to the existing LogicalTapeReadExact(). Reviewed-by: Amit Kapila <amit.kapila16@gmail.com> Discussion: https://www.postgresql.org/message-id/flat/f3501945-c591-8cc3-5ef0-b72a2e0eaa9c@enterprisedb.com
1 parent 1561612 commit 20428d3

File tree

9 files changed

+73
-134
lines changed

9 files changed

+73
-134
lines changed

‎src/backend/access/gist/gistbuildbuffers.c

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -753,14 +753,9 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
753753
static void
754754
ReadTempFileBlock(BufFile *file, long blknum, void *ptr)
755755
{
756-
size_t nread;
757-
758756
if (BufFileSeekBlock(file, blknum) != 0)
759757
elog(ERROR, "could not seek to block %ld in temporary file", blknum);
760-
nread = BufFileRead(file, ptr, BLCKSZ);
761-
if (nread != BLCKSZ)
762-
elog(ERROR, "could not read temporary file: read only %zu of %zu bytes",
763-
nread, (size_t) BLCKSZ);
758+
BufFileReadExact(file, ptr, BLCKSZ);
764759
}
765760

766761
static void

‎src/backend/backup/backup_manifest.c

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -362,17 +362,10 @@ SendBackupManifest(backup_manifest_info *manifest, bbsink *sink)
362362
while (manifest_bytes_done < manifest->manifest_size)
363363
{
364364
size_t bytes_to_read;
365-
size_t rc;
366365

367366
bytes_to_read = Min(sink->bbs_buffer_length,
368367
manifest->manifest_size - manifest_bytes_done);
369-
rc = BufFileRead(manifest->buffile, sink->bbs_buffer,
370-
bytes_to_read);
371-
if (rc != bytes_to_read)
372-
ereport(ERROR,
373-
(errcode_for_file_access(),
374-
errmsg("could not read from temporary file: read only %zu of %zu bytes",
375-
rc, bytes_to_read)));
368+
BufFileReadExact(manifest->buffile, sink->bbs_buffer, bytes_to_read);
376369
bbsink_manifest_contents(sink, bytes_to_read);
377370
manifest_bytes_done += bytes_to_read;
378371
}

‎src/backend/executor/nodeHashjoin.c

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1260,28 +1260,18 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
12601260
* we can read them both in one BufFileRead() call without any type
12611261
* cheating.
12621262
*/
1263-
nread = BufFileRead(file, header, sizeof(header));
1263+
nread = BufFileReadMaybeEOF(file, header, sizeof(header), true);
12641264
if (nread == 0) /* end of file */
12651265
{
12661266
ExecClearTuple(tupleSlot);
12671267
return NULL;
12681268
}
1269-
if (nread != sizeof(header))
1270-
ereport(ERROR,
1271-
(errcode_for_file_access(),
1272-
errmsg("could not read from hash-join temporary file: read only %zu of %zu bytes",
1273-
nread, sizeof(header))));
12741269
*hashvalue = header[0];
12751270
tuple = (MinimalTuple) palloc(header[1]);
12761271
tuple->t_len = header[1];
1277-
nread = BufFileRead(file,
1278-
((char *) tuple + sizeof(uint32)),
1279-
header[1] - sizeof(uint32));
1280-
if (nread != header[1] - sizeof(uint32))
1281-
ereport(ERROR,
1282-
(errcode_for_file_access(),
1283-
errmsg("could not read from hash-join temporary file: read only %zu of %zu bytes",
1284-
nread, header[1] - sizeof(uint32))));
1272+
BufFileReadExact(file,
1273+
(char *) tuple + sizeof(uint32),
1274+
header[1] - sizeof(uint32));
12851275
ExecForceStoreMinimalTuple(tuple, tupleSlot, true);
12861276
return tupleSlot;
12871277
}

‎src/backend/replication/logical/worker.c

Lines changed: 4 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -2069,19 +2069,13 @@ apply_spooled_messages(FileSet *stream_fileset, TransactionId xid,
20692069
CHECK_FOR_INTERRUPTS();
20702070

20712071
/* read length of the on-disk record */
2072-
nbytes = BufFileRead(stream_fd, &len, sizeof(len));
2072+
nbytes = BufFileReadMaybeEOF(stream_fd, &len, sizeof(len), true);
20732073

20742074
/* have we reached end of the file? */
20752075
if (nbytes == 0)
20762076
break;
20772077

20782078
/* do we have a correct length? */
2079-
if (nbytes != sizeof(len))
2080-
ereport(ERROR,
2081-
(errcode_for_file_access(),
2082-
errmsg("could not read from streaming transaction's changes file \"%s\": read only %zu of %zu bytes",
2083-
path, nbytes, sizeof(len))));
2084-
20852079
if (len <= 0)
20862080
elog(ERROR, "incorrect length %d in streaming transaction's changes file \"%s\"",
20872081
len, path);
@@ -2090,12 +2084,7 @@ apply_spooled_messages(FileSet *stream_fileset, TransactionId xid,
20902084
buffer = repalloc(buffer, len);
20912085

20922086
/* and finally read the data into the buffer */
2093-
nbytes = BufFileRead(stream_fd, buffer, len);
2094-
if (nbytes != len)
2095-
ereport(ERROR,
2096-
(errcode_for_file_access(),
2097-
errmsg("could not read from streaming transaction's changes file \"%s\": read only %zu of %zu bytes",
2098-
path, nbytes, (size_t) len)));
2087+
BufFileReadExact(stream_fd, buffer, len);
20992088

21002089
BufFileTell(stream_fd, &fileno, &offset);
21012090

@@ -3993,7 +3982,6 @@ static void
39933982
subxact_info_read(Oid subid, TransactionId xid)
39943983
{
39953984
char path[MAXPGPATH];
3996-
size_t nread;
39973985
Size len;
39983986
BufFile *fd;
39993987
MemoryContext oldctx;
@@ -4013,12 +4001,7 @@ subxact_info_read(Oid subid, TransactionId xid)
40134001
return;
40144002

40154003
/* read number of subxact items */
4016-
nread = BufFileRead(fd, &subxact_data.nsubxacts, sizeof(subxact_data.nsubxacts));
4017-
if (nread != sizeof(subxact_data.nsubxacts))
4018-
ereport(ERROR,
4019-
(errcode_for_file_access(),
4020-
errmsg("could not read from streaming transaction's subxact file \"%s\": read only %zu of %zu bytes",
4021-
path, nread, sizeof(subxact_data.nsubxacts))));
4004+
BufFileReadExact(fd, &subxact_data.nsubxacts, sizeof(subxact_data.nsubxacts));
40224005

40234006
len = sizeof(SubXactInfo) * subxact_data.nsubxacts;
40244007

@@ -4037,14 +4020,7 @@ subxact_info_read(Oid subid, TransactionId xid)
40374020
MemoryContextSwitchTo(oldctx);
40384021

40394022
if (len > 0)
4040-
{
4041-
nread = BufFileRead(fd, subxact_data.subxacts, len);
4042-
if (nread != len)
4043-
ereport(ERROR,
4044-
(errcode_for_file_access(),
4045-
errmsg("could not read from streaming transaction's subxact file \"%s\": read only %zu of %zu bytes",
4046-
path, nread, len)));
4047-
}
4023+
BufFileReadExact(fd, subxact_data.subxacts, len);
40484024

40494025
BufFileClose(fd);
40504026
}

‎src/backend/storage/file/buffile.c

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -573,14 +573,19 @@ BufFileDumpBuffer(BufFile *file)
573573
}
574574

575575
/*
576-
* BufFileRead
576+
* BufFileRead variants
577577
*
578578
* Like fread() except we assume 1-byte element size and report I/O errors via
579579
* ereport().
580+
*
581+
* If 'exact' is true, then an error is also raised if the number of bytes
582+
* read is not exactly 'size' (no short reads). If 'exact' and 'eofOK' are
583+
* true, then reading zero bytes is ok.
580584
*/
581-
size_t
582-
BufFileRead(BufFile *file, void *ptr, size_t size)
585+
static size_t
586+
BufFileReadCommon(BufFile *file, void *ptr, size_t size, bool exact, bool eofOK)
583587
{
588+
size_t start_size = size;
584589
size_t nread = 0;
585590
size_t nthistime;
586591

@@ -612,9 +617,48 @@ BufFileRead(BufFile *file, void *ptr, size_t size)
612617
nread += nthistime;
613618
}
614619

620+
if (exact &&
621+
(nread != start_size && !(nread == 0 && eofOK)))
622+
ereport(ERROR,
623+
errcode_for_file_access(),
624+
file->name ?
625+
errmsg("could not read from file set \"%s\": read only %zu of %zu bytes",
626+
file->name, nread, start_size) :
627+
errmsg("could not read from temporary file: read only %zu of %zu bytes",
628+
nread, start_size));
629+
615630
return nread;
616631
}
617632

633+
/*
634+
* Legacy interface where the caller needs to check for end of file or short
635+
* reads.
636+
*/
637+
size_t
638+
BufFileRead(BufFile *file, void *ptr, size_t size)
639+
{
640+
return BufFileReadCommon(file, ptr, size, false, false);
641+
}
642+
643+
/*
644+
* Require read of exactly the specified size.
645+
*/
646+
void
647+
BufFileReadExact(BufFile *file, void *ptr, size_t size)
648+
{
649+
BufFileReadCommon(file, ptr, size, true, false);
650+
}
651+
652+
/*
653+
* Require read of exactly the specified size, but optionally allow end of
654+
* file (in which case 0 is returned).
655+
*/
656+
size_t
657+
BufFileReadMaybeEOF(BufFile *file, void *ptr, size_t size, bool eofOK)
658+
{
659+
return BufFileReadCommon(file, ptr, size, true, eofOK);
660+
}
661+
618662
/*
619663
* BufFileWrite
620664
*

‎src/backend/utils/sort/logtape.c

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -281,19 +281,12 @@ ltsWriteBlock(LogicalTapeSet *lts, long blocknum, const void *buffer)
281281
static void
282282
ltsReadBlock(LogicalTapeSet *lts, long blocknum, void *buffer)
283283
{
284-
size_t nread;
285-
286284
if (BufFileSeekBlock(lts->pfile, blocknum) != 0)
287285
ereport(ERROR,
288286
(errcode_for_file_access(),
289287
errmsg("could not seek to block %ld of temporary file",
290288
blocknum)));
291-
nread = BufFileRead(lts->pfile, buffer, BLCKSZ);
292-
if (nread != BLCKSZ)
293-
ereport(ERROR,
294-
(errcode_for_file_access(),
295-
errmsg("could not read block %ld of temporary file: read only %zu of %zu bytes",
296-
blocknum, nread, (size_t) BLCKSZ)));
289+
BufFileReadExact(lts->pfile, buffer, BLCKSZ);
297290
}
298291

299292
/*

‎src/backend/utils/sort/sharedtuplestore.c

Lines changed: 6 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -422,23 +422,10 @@ sts_read_tuple(SharedTuplestoreAccessor *accessor, void *meta_data)
422422
*/
423423
if (accessor->sts->meta_data_size > 0)
424424
{
425-
if (BufFileRead(accessor->read_file,
426-
meta_data,
427-
accessor->sts->meta_data_size) !=
428-
accessor->sts->meta_data_size)
429-
ereport(ERROR,
430-
(errcode_for_file_access(),
431-
errmsg("could not read from shared tuplestore temporary file"),
432-
errdetail_internal("Short read while reading meta-data.")));
425+
BufFileReadExact(accessor->read_file, meta_data, accessor->sts->meta_data_size);
433426
accessor->read_bytes += accessor->sts->meta_data_size;
434427
}
435-
if (BufFileRead(accessor->read_file,
436-
&size,
437-
sizeof(size)) != sizeof(size))
438-
ereport(ERROR,
439-
(errcode_for_file_access(),
440-
errmsg("could not read from shared tuplestore temporary file"),
441-
errdetail_internal("Short read while reading size.")));
428+
BufFileReadExact(accessor->read_file, &size, sizeof(size));
442429
accessor->read_bytes += sizeof(size);
443430
if (size > accessor->read_buffer_size)
444431
{
@@ -455,13 +442,7 @@ sts_read_tuple(SharedTuplestoreAccessor *accessor, void *meta_data)
455442
this_chunk_size = Min(remaining_size,
456443
BLCKSZ * STS_CHUNK_PAGES - accessor->read_bytes);
457444
destination = accessor->read_buffer + sizeof(uint32);
458-
if (BufFileRead(accessor->read_file,
459-
destination,
460-
this_chunk_size) != this_chunk_size)
461-
ereport(ERROR,
462-
(errcode_for_file_access(),
463-
errmsg("could not read from shared tuplestore temporary file"),
464-
errdetail_internal("Short read while reading tuple.")));
445+
BufFileReadExact(accessor->read_file, destination, this_chunk_size);
465446
accessor->read_bytes += this_chunk_size;
466447
remaining_size -= this_chunk_size;
467448
destination += this_chunk_size;
@@ -473,12 +454,7 @@ sts_read_tuple(SharedTuplestoreAccessor *accessor, void *meta_data)
473454
/* We are now positioned at the start of an overflow chunk. */
474455
SharedTuplestoreChunk chunk_header;
475456

476-
if (BufFileRead(accessor->read_file, &chunk_header, STS_CHUNK_HEADER_SIZE) !=
477-
STS_CHUNK_HEADER_SIZE)
478-
ereport(ERROR,
479-
(errcode_for_file_access(),
480-
errmsg("could not read from shared tuplestore temporary file"),
481-
errdetail_internal("Short read while reading overflow chunk header.")));
457+
BufFileReadExact(accessor->read_file, &chunk_header, STS_CHUNK_HEADER_SIZE);
482458
accessor->read_bytes = STS_CHUNK_HEADER_SIZE;
483459
if (chunk_header.overflow == 0)
484460
ereport(ERROR,
@@ -489,13 +465,7 @@ sts_read_tuple(SharedTuplestoreAccessor *accessor, void *meta_data)
489465
this_chunk_size = Min(remaining_size,
490466
BLCKSZ * STS_CHUNK_PAGES -
491467
STS_CHUNK_HEADER_SIZE);
492-
if (BufFileRead(accessor->read_file,
493-
destination,
494-
this_chunk_size) != this_chunk_size)
495-
ereport(ERROR,
496-
(errcode_for_file_access(),
497-
errmsg("could not read from shared tuplestore temporary file"),
498-
errdetail_internal("Short read while reading tuple.")));
468+
BufFileReadExact(accessor->read_file, destination, this_chunk_size);
499469
accessor->read_bytes += this_chunk_size;
500470
remaining_size -= this_chunk_size;
501471
destination += this_chunk_size;
@@ -551,7 +521,6 @@ sts_parallel_scan_next(SharedTuplestoreAccessor *accessor, void *meta_data)
551521
if (!eof)
552522
{
553523
SharedTuplestoreChunk chunk_header;
554-
size_t nread;
555524

556525
/* Make sure we have the file open. */
557526
if (accessor->read_file == NULL)
@@ -570,13 +539,7 @@ sts_parallel_scan_next(SharedTuplestoreAccessor *accessor, void *meta_data)
570539
(errcode_for_file_access(),
571540
errmsg("could not seek to block %u in shared tuplestore temporary file",
572541
read_page)));
573-
nread = BufFileRead(accessor->read_file, &chunk_header,
574-
STS_CHUNK_HEADER_SIZE);
575-
if (nread != STS_CHUNK_HEADER_SIZE)
576-
ereport(ERROR,
577-
(errcode_for_file_access(),
578-
errmsg("could not read from shared tuplestore temporary file: read only %zu of %zu bytes",
579-
nread, STS_CHUNK_HEADER_SIZE)));
542+
BufFileReadExact(accessor->read_file, &chunk_header, STS_CHUNK_HEADER_SIZE);
580543

581544
/*
582545
* If this is an overflow chunk, we skip it and any following

‎src/backend/utils/sort/tuplestore.c

Lines changed: 6 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1468,15 +1468,11 @@ getlen(Tuplestorestate *state, bool eofOK)
14681468
unsigned int len;
14691469
size_t nbytes;
14701470

1471-
nbytes = BufFileRead(state->myfile, &len, sizeof(len));
1472-
if (nbytes == sizeof(len))
1471+
nbytes = BufFileReadMaybeEOF(state->myfile, &len, sizeof(len), eofOK);
1472+
if (nbytes == 0)
1473+
return 0;
1474+
else
14731475
return len;
1474-
if (nbytes != 0 || !eofOK)
1475-
ereport(ERROR,
1476-
(errcode_for_file_access(),
1477-
errmsg("could not read from tuplestore temporary file: read only %zu of %zu bytes",
1478-
nbytes, sizeof(len))));
1479-
return 0;
14801476
}
14811477

14821478

@@ -1528,25 +1524,12 @@ readtup_heap(Tuplestorestate *state, unsigned int len)
15281524
unsigned int tuplen = tupbodylen + MINIMAL_TUPLE_DATA_OFFSET;
15291525
MinimalTuple tuple = (MinimalTuple) palloc(tuplen);
15301526
char *tupbody = (char *) tuple + MINIMAL_TUPLE_DATA_OFFSET;
1531-
size_t nread;
15321527

15331528
USEMEM(state, GetMemoryChunkSpace(tuple));
15341529
/* read in the tuple proper */
15351530
tuple->t_len = tuplen;
1536-
nread = BufFileRead(state->myfile, tupbody, tupbodylen);
1537-
if (nread != (size_t) tupbodylen)
1538-
ereport(ERROR,
1539-
(errcode_for_file_access(),
1540-
errmsg("could not read from tuplestore temporary file: read only %zu of %zu bytes",
1541-
nread, (size_t) tupbodylen)));
1531+
BufFileReadExact(state->myfile, tupbody, tupbodylen);
15421532
if (state->backward) /* need trailing length word? */
1543-
{
1544-
nread = BufFileRead(state->myfile, &tuplen, sizeof(tuplen));
1545-
if (nread != sizeof(tuplen))
1546-
ereport(ERROR,
1547-
(errcode_for_file_access(),
1548-
errmsg("could not read from tuplestore temporary file: read only %zu of %zu bytes",
1549-
nread, sizeof(tuplen))));
1550-
}
1533+
BufFileReadExact(state->myfile, &tuplen, sizeof(tuplen));
15511534
return (void *) tuple;
15521535
}

‎src/include/storage/buffile.h

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,9 @@ typedef struct BufFile BufFile;
3838

3939
extern BufFile *BufFileCreateTemp(bool interXact);
4040
extern void BufFileClose(BufFile *file);
41-
extern size_t BufFileRead(BufFile *file, void *ptr, size_t size);
41+
extern pg_nodiscard size_t BufFileRead(BufFile *file, void *ptr, size_t size);
42+
extern void BufFileReadExact(BufFile *file, void *ptr, size_t size);
43+
extern size_t BufFileReadMaybeEOF(BufFile *file, void *ptr, size_t size, bool eofOK);
4244
extern void BufFileWrite(BufFile *file, const void *ptr, size_t size);
4345
extern int BufFileSeek(BufFile *file, int fileno, off_t offset, int whence);
4446
extern void BufFileTell(BufFile *file, int *fileno, off_t *offset);

0 commit comments

Comments
 (0)