File API cleanup
Started by Peter Eisentrautabout 3 years ago5 messages
Here are a couple of patches that clean up the internal File API and
related things a bit:
0001-Update-types-in-File-API.patch
Make the argument types of the File API match stdio better:
- Change the data buffer to void *, from char *.
- Change FileWrite() data buffer to const on top of that.
- Change amounts to size_t, from int.
In passing, change the FilePrefetch() amount argument from int to
off_t, to match the underlying posix_fadvise().
0002-Remove-unnecessary-casts.patch
Some code carefully cast all data buffer arguments for
BufFileWrite() and BufFileRead() to void *, even though the
arguments are already void * (and AFAICT were never anything else).
Remove this unnecessary clutter.
(I had initially thought these casts were related to the first patch,
but as I said the BufFile API never used char * arguments, so this
turned out to be unrelated, but still weird.)
Attachments:
0001-Update-types-in-File-API.patchtext/plain; charset=UTF-8; name=0001-Update-types-in-File-API.patchDownload
From 0ca35b75e383272356ba49211913d8aead5ca10d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 1 Dec 2022 08:36:09 +0100
Subject: [PATCH 1/2] Update types in File API
Make the argument types of the File API match stdio better:
- Change the data buffer to void *, from char *.
- Change FileWrite() data buffer to const on top of that.
- Change amounts to size_t, from int.
In passing, change the FilePrefetch() amount argument from int to
off_t, to match the underlying posix_fadvise().
---
src/backend/storage/file/fd.c | 8 ++++----
src/include/storage/fd.h | 6 +++---
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 4151cafec547..f6c938202309 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -1980,7 +1980,7 @@ FileClose(File file)
* to read into.
*/
int
-FilePrefetch(File file, off_t offset, int amount, uint32 wait_event_info)
+FilePrefetch(File file, off_t offset, off_t amount, uint32 wait_event_info)
{
#if defined(USE_POSIX_FADVISE) && defined(POSIX_FADV_WILLNEED)
int returnCode;
@@ -2031,7 +2031,7 @@ FileWriteback(File file, off_t offset, off_t nbytes, uint32 wait_event_info)
}
int
-FileRead(File file, char *buffer, int amount, off_t offset,
+FileRead(File file, void *buffer, size_t amount, off_t offset,
uint32 wait_event_info)
{
int returnCode;
@@ -2039,7 +2039,7 @@ FileRead(File file, char *buffer, int amount, off_t offset,
Assert(FileIsValid(file));
- DO_DB(elog(LOG, "FileRead: %d (%s) " INT64_FORMAT " %d %p",
+ DO_DB(elog(LOG, "FileRead: %d (%s) " INT64_FORMAT " %zu %p",
file, VfdCache[file].fileName,
(int64) offset,
amount, buffer));
@@ -2087,7 +2087,7 @@ FileRead(File file, char *buffer, int amount, off_t offset,
}
int
-FileWrite(File file, char *buffer, int amount, off_t offset,
+FileWrite(File file, const void *buffer, size_t amount, off_t offset,
uint32 wait_event_info)
{
int returnCode;
diff --git a/src/include/storage/fd.h b/src/include/storage/fd.h
index c0a212487d92..7144fc9f6050 100644
--- a/src/include/storage/fd.h
+++ b/src/include/storage/fd.h
@@ -102,9 +102,9 @@ extern File PathNameOpenFile(const char *fileName, int fileFlags);
extern File PathNameOpenFilePerm(const char *fileName, int fileFlags, mode_t fileMode);
extern File OpenTemporaryFile(bool interXact);
extern void FileClose(File file);
-extern int FilePrefetch(File file, off_t offset, int amount, uint32 wait_event_info);
-extern int FileRead(File file, char *buffer, int amount, off_t offset, uint32 wait_event_info);
-extern int FileWrite(File file, char *buffer, int amount, off_t offset, uint32 wait_event_info);
+extern int FilePrefetch(File file, off_t offset, off_t amount, uint32 wait_event_info);
+extern int FileRead(File file, void *buffer, size_t amount, off_t offset, uint32 wait_event_info);
+extern int FileWrite(File file, const void *buffer, size_t amount, off_t offset, uint32 wait_event_info);
extern int FileSync(File file, uint32 wait_event_info);
extern off_t FileSize(File file);
extern int FileTruncate(File file, off_t offset, uint32 wait_event_info);
base-commit: 43351557d0d2b9c5e20298b5fee2849abef86aff
--
2.38.1
0002-Remove-unnecessary-casts.patchtext/plain; charset=UTF-8; name=0002-Remove-unnecessary-casts.patchDownload
From 8d0530fbf9095bc16fa3465b69aef2c31c4c3c9b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Thu, 1 Dec 2022 08:36:09 +0100
Subject: [PATCH 2/2] Remove unnecessary casts
Some code carefully cast all data buffer arguments for BufFileWrite()
and BufFileRead() to void *, even though the arguments are already
void * (and AFAICT were never anything else). Remove this unnecessary
clutter.
---
src/backend/executor/nodeHashjoin.c | 8 ++++----
src/backend/utils/sort/tuplestore.c | 12 ++++++------
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 2718c2113f58..3e1a997f920d 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -1227,8 +1227,8 @@ ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue,
*fileptr = file;
}
- BufFileWrite(file, (void *) &hashvalue, sizeof(uint32));
- BufFileWrite(file, (void *) tuple, tuple->t_len);
+ BufFileWrite(file, &hashvalue, sizeof(uint32));
+ BufFileWrite(file, tuple, tuple->t_len);
}
/*
@@ -1260,7 +1260,7 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
* we can read them both in one BufFileRead() call without any type
* cheating.
*/
- nread = BufFileRead(file, (void *) header, sizeof(header));
+ nread = BufFileRead(file, header, sizeof(header));
if (nread == 0) /* end of file */
{
ExecClearTuple(tupleSlot);
@@ -1275,7 +1275,7 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
tuple = (MinimalTuple) palloc(header[1]);
tuple->t_len = header[1];
nread = BufFileRead(file,
- (void *) ((char *) tuple + sizeof(uint32)),
+ ((char *) tuple + sizeof(uint32)),
header[1] - sizeof(uint32));
if (nread != header[1] - sizeof(uint32))
ereport(ERROR,
diff --git a/src/backend/utils/sort/tuplestore.c b/src/backend/utils/sort/tuplestore.c
index f605ece721eb..cc884ab11649 100644
--- a/src/backend/utils/sort/tuplestore.c
+++ b/src/backend/utils/sort/tuplestore.c
@@ -1468,7 +1468,7 @@ getlen(Tuplestorestate *state, bool eofOK)
unsigned int len;
size_t nbytes;
- nbytes = BufFileRead(state->myfile, (void *) &len, sizeof(len));
+ nbytes = BufFileRead(state->myfile, &len, sizeof(len));
if (nbytes == sizeof(len))
return len;
if (nbytes != 0 || !eofOK)
@@ -1512,10 +1512,10 @@ writetup_heap(Tuplestorestate *state, void *tup)
/* total on-disk footprint: */
unsigned int tuplen = tupbodylen + sizeof(int);
- BufFileWrite(state->myfile, (void *) &tuplen, sizeof(tuplen));
- BufFileWrite(state->myfile, (void *) tupbody, tupbodylen);
+ BufFileWrite(state->myfile, &tuplen, sizeof(tuplen));
+ BufFileWrite(state->myfile, tupbody, tupbodylen);
if (state->backward) /* need trailing length word? */
- BufFileWrite(state->myfile, (void *) &tuplen, sizeof(tuplen));
+ BufFileWrite(state->myfile, &tuplen, sizeof(tuplen));
FREEMEM(state, GetMemoryChunkSpace(tuple));
heap_free_minimal_tuple(tuple);
@@ -1533,7 +1533,7 @@ readtup_heap(Tuplestorestate *state, unsigned int len)
USEMEM(state, GetMemoryChunkSpace(tuple));
/* read in the tuple proper */
tuple->t_len = tuplen;
- nread = BufFileRead(state->myfile, (void *) tupbody, tupbodylen);
+ nread = BufFileRead(state->myfile, tupbody, tupbodylen);
if (nread != (size_t) tupbodylen)
ereport(ERROR,
(errcode_for_file_access(),
@@ -1541,7 +1541,7 @@ readtup_heap(Tuplestorestate *state, unsigned int len)
nread, (size_t) tupbodylen)));
if (state->backward) /* need trailing length word? */
{
- nread = BufFileRead(state->myfile, (void *) &tuplen, sizeof(tuplen));
+ nread = BufFileRead(state->myfile, &tuplen, sizeof(tuplen));
if (nread != sizeof(tuplen))
ereport(ERROR,
(errcode_for_file_access(),
--
2.38.1
Re: File API cleanup
On Thu, Dec 1, 2022 at 1:55 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
Here are a couple of patches that clean up the internal File API and
related things a bit:0001-Update-types-in-File-API.patch
Make the argument types of the File API match stdio better:
- Change the data buffer to void *, from char *.
- Change FileWrite() data buffer to const on top of that.
- Change amounts to size_t, from int.In passing, change the FilePrefetch() amount argument from int to
off_t, to match the underlying posix_fadvise().0002-Remove-unnecessary-casts.patch
Some code carefully cast all data buffer arguments for
BufFileWrite() and BufFileRead() to void *, even though the
arguments are already void * (and AFAICT were never anything else).
Remove this unnecessary clutter.(I had initially thought these casts were related to the first patch,
but as I said the BufFile API never used char * arguments, so this
turned out to be unrelated, but still weird.)
Thanks. Please note that I've not looked at the patches attached.
However, I'm here after reading the $subject - can we have a generic,
single function file_exists() in fd.c/file_utils.c so that both
backend and frontend code can use it? I see there are 3 uses and
definitions of it in jit.c, dfmgr.c and pg_regress.c. This will reduce
the code duplication. Thoughts?
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Re: File API cleanup
On 01.12.22 09:55, Bharath Rupireddy wrote:
can we have a generic,
single function file_exists() in fd.c/file_utils.c so that both
backend and frontend code can use it? I see there are 3 uses and
definitions of it in jit.c, dfmgr.c and pg_regress.c. This will reduce
the code duplication. Thoughts?
Well, the first problem with that would be that all three of those
implementations are slightly different. Maybe that is intentional, or
maybe not, in which case a common implementation might be beneficial.
(Another thing to consider is that checking whether a file exists is not
often actually useful. If you want to use the file, you should just
open it and then check for any errors. The cases above have special
requirements, so there obviously are uses, but I'm not sure how many in
the long run.)
Re: File API cleanup
On 01.12.22 09:25, Peter Eisentraut wrote:
Here are a couple of patches that clean up the internal File API and
related things a bit:
Here are two follow-up patches that clean up some stuff related to the
earlier patch set. I suspect these are all historically related.
0001-Remove-unnecessary-casts.patch
Some code carefully cast all data buffer arguments for data write
and read function calls to void *, even though the respective
arguments are already void *. Remove this unnecessary clutter.
0002-Add-const-to-BufFileWrite.patch
Make data buffer argument to BufFileWrite a const pointer and bubble
this up to various callers and related APIs. This makes the APIs
clearer and more consistent.
Attachments:
0001-Remove-unnecessary-casts.patchtext/plain; charset=UTF-8; name=0001-Remove-unnecessary-casts.patchDownload
From cec7ec188f2473d983b048b9e1eddc7dbc3a4241 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 23 Dec 2022 08:35:42 +0100
Subject: [PATCH 1/2] Remove unnecessary casts
Some code carefully cast all data buffer arguments for data write and
read function calls to void *, even though the respective arguments
are already void *. Remove this unnecessary clutter.
---
src/backend/executor/nodeAgg.c | 6 +++---
src/backend/storage/file/buffile.c | 4 ++--
src/backend/utils/sort/logtape.c | 18 +++++++++---------
src/backend/utils/sort/tuplesort.c | 2 +-
src/backend/utils/sort/tuplesortvariants.c | 16 ++++++++--------
5 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 30c9143183..f15bb83a1a 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -2961,10 +2961,10 @@ hashagg_spill_tuple(AggState *aggstate, HashAggSpill *spill,
tape = spill->partitions[partition];
- LogicalTapeWrite(tape, (void *) &hash, sizeof(uint32));
+ LogicalTapeWrite(tape, &hash, sizeof(uint32));
total_written += sizeof(uint32);
- LogicalTapeWrite(tape, (void *) tuple, tuple->t_len);
+ LogicalTapeWrite(tape, tuple, tuple->t_len);
total_written += tuple->t_len;
if (shouldFree)
@@ -3029,7 +3029,7 @@ hashagg_batch_read(HashAggBatch *batch, uint32 *hashp)
tuple->t_len = t_len;
nread = LogicalTapeRead(tape,
- (void *) ((char *) tuple + sizeof(uint32)),
+ (char *) tuple + sizeof(uint32),
t_len - sizeof(uint32));
if (nread != t_len - sizeof(uint32))
ereport(ERROR,
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index b0b4eeb3bd..b07cca28d6 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -607,7 +607,7 @@ BufFileRead(BufFile *file, void *ptr, size_t size)
memcpy(ptr, file->buffer.data + file->pos, nthistime);
file->pos += nthistime;
- ptr = (void *) ((char *) ptr + nthistime);
+ ptr = (char *) ptr + nthistime;
size -= nthistime;
nread += nthistime;
}
@@ -655,7 +655,7 @@ BufFileWrite(BufFile *file, void *ptr, size_t size)
file->pos += nthistime;
if (file->nbytes < file->pos)
file->nbytes = file->pos;
- ptr = (void *) ((char *) ptr + nthistime);
+ ptr = (char *) ptr + nthistime;
size -= nthistime;
}
}
diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c
index c384f98e13..9db220b7ea 100644
--- a/src/backend/utils/sort/logtape.c
+++ b/src/backend/utils/sort/logtape.c
@@ -319,7 +319,7 @@ ltsReadFillBuffer(LogicalTape *lt)
datablocknum += lt->offsetBlockNumber;
/* Read the block */
- ltsReadBlock(lt->tapeSet, datablocknum, (void *) thisbuf);
+ ltsReadBlock(lt->tapeSet, datablocknum, thisbuf);
if (!lt->frozen)
ltsReleaseBlock(lt->tapeSet, datablocknum);
lt->curBlockNumber = lt->nextBlockNumber;
@@ -806,7 +806,7 @@ LogicalTapeWrite(LogicalTape *lt, void *ptr, size_t size)
/* set the next-pointer and dump the current block. */
TapeBlockGetTrailer(lt->buffer)->next = nextBlockNumber;
- ltsWriteBlock(lt->tapeSet, lt->curBlockNumber, (void *) lt->buffer);
+ ltsWriteBlock(lt->tapeSet, lt->curBlockNumber, lt->buffer);
/* initialize the prev-pointer of the next block */
TapeBlockGetTrailer(lt->buffer)->prev = lt->curBlockNumber;
@@ -826,7 +826,7 @@ LogicalTapeWrite(LogicalTape *lt, void *ptr, size_t size)
lt->pos += nthistime;
if (lt->nbytes < lt->pos)
lt->nbytes = lt->pos;
- ptr = (void *) ((char *) ptr + nthistime);
+ ptr = (char *) ptr + nthistime;
size -= nthistime;
}
}
@@ -888,7 +888,7 @@ LogicalTapeRewindForRead(LogicalTape *lt, size_t buffer_size)
lt->buffer_size - lt->nbytes);
TapeBlockSetNBytes(lt->buffer, lt->nbytes);
- ltsWriteBlock(lt->tapeSet, lt->curBlockNumber, (void *) lt->buffer);
+ ltsWriteBlock(lt->tapeSet, lt->curBlockNumber, lt->buffer);
}
lt->writing = false;
}
@@ -953,7 +953,7 @@ LogicalTapeRead(LogicalTape *lt, void *ptr, size_t size)
memcpy(ptr, lt->buffer + lt->pos, nthistime);
lt->pos += nthistime;
- ptr = (void *) ((char *) ptr + nthistime);
+ ptr = (char *) ptr + nthistime;
size -= nthistime;
nread += nthistime;
}
@@ -1004,7 +1004,7 @@ LogicalTapeFreeze(LogicalTape *lt, TapeShare *share)
lt->buffer_size - lt->nbytes);
TapeBlockSetNBytes(lt->buffer, lt->nbytes);
- ltsWriteBlock(lt->tapeSet, lt->curBlockNumber, (void *) lt->buffer);
+ ltsWriteBlock(lt->tapeSet, lt->curBlockNumber, lt->buffer);
}
lt->writing = false;
lt->frozen = true;
@@ -1031,7 +1031,7 @@ LogicalTapeFreeze(LogicalTape *lt, TapeShare *share)
if (lt->firstBlockNumber == -1L)
lt->nextBlockNumber = -1L;
- ltsReadBlock(lt->tapeSet, lt->curBlockNumber, (void *) lt->buffer);
+ ltsReadBlock(lt->tapeSet, lt->curBlockNumber, lt->buffer);
if (TapeBlockIsLast(lt->buffer))
lt->nextBlockNumber = -1L;
else
@@ -1098,7 +1098,7 @@ LogicalTapeBackspace(LogicalTape *lt, size_t size)
return seekpos;
}
- ltsReadBlock(lt->tapeSet, prev, (void *) lt->buffer);
+ ltsReadBlock(lt->tapeSet, prev, lt->buffer);
if (TapeBlockGetTrailer(lt->buffer)->next != lt->curBlockNumber)
elog(ERROR, "broken tape, next of block %ld is %ld, expected %ld",
@@ -1142,7 +1142,7 @@ LogicalTapeSeek(LogicalTape *lt, long blocknum, int offset)
if (blocknum != lt->curBlockNumber)
{
- ltsReadBlock(lt->tapeSet, blocknum, (void *) lt->buffer);
+ ltsReadBlock(lt->tapeSet, blocknum, lt->buffer);
lt->curBlockNumber = blocknum;
lt->nbytes = TapeBlockPayloadSize;
lt->nextBlockNumber = TapeBlockGetTrailer(lt->buffer)->next;
diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index 416f02ba3c..dd5fba0e36 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -2908,7 +2908,7 @@ markrunend(LogicalTape *tape)
{
unsigned int len = 0;
- LogicalTapeWrite(tape, (void *) &len, sizeof(len));
+ LogicalTapeWrite(tape, &len, sizeof(len));
}
/*
diff --git a/src/backend/utils/sort/tuplesortvariants.c b/src/backend/utils/sort/tuplesortvariants.c
index dfe132660a..12ca6d7d75 100644
--- a/src/backend/utils/sort/tuplesortvariants.c
+++ b/src/backend/utils/sort/tuplesortvariants.c
@@ -1002,10 +1002,10 @@ writetup_heap(Tuplesortstate *state, LogicalTape *tape, SortTuple *stup)
/* total on-disk footprint: */
unsigned int tuplen = tupbodylen + sizeof(int);
- LogicalTapeWrite(tape, (void *) &tuplen, sizeof(tuplen));
- LogicalTapeWrite(tape, (void *) tupbody, tupbodylen);
+ LogicalTapeWrite(tape, &tuplen, sizeof(tuplen));
+ LogicalTapeWrite(tape, tupbody, tupbodylen);
if (base->sortopt & TUPLESORT_RANDOMACCESS) /* need trailing length word? */
- LogicalTapeWrite(tape, (void *) &tuplen, sizeof(tuplen));
+ LogicalTapeWrite(tape, &tuplen, sizeof(tuplen));
}
static void
@@ -1475,10 +1475,10 @@ writetup_index(Tuplesortstate *state, LogicalTape *tape, SortTuple *stup)
unsigned int tuplen;
tuplen = IndexTupleSize(tuple) + sizeof(tuplen);
- LogicalTapeWrite(tape, (void *) &tuplen, sizeof(tuplen));
- LogicalTapeWrite(tape, (void *) tuple, IndexTupleSize(tuple));
+ LogicalTapeWrite(tape, &tuplen, sizeof(tuplen));
+ LogicalTapeWrite(tape, tuple, IndexTupleSize(tuple));
if (base->sortopt & TUPLESORT_RANDOMACCESS) /* need trailing length word? */
- LogicalTapeWrite(tape, (void *) &tuplen, sizeof(tuplen));
+ LogicalTapeWrite(tape, &tuplen, sizeof(tuplen));
}
static void
@@ -1564,10 +1564,10 @@ writetup_datum(Tuplesortstate *state, LogicalTape *tape, SortTuple *stup)
writtenlen = tuplen + sizeof(unsigned int);
- LogicalTapeWrite(tape, (void *) &writtenlen, sizeof(writtenlen));
+ LogicalTapeWrite(tape, &writtenlen, sizeof(writtenlen));
LogicalTapeWrite(tape, waddr, tuplen);
if (base->sortopt & TUPLESORT_RANDOMACCESS) /* need trailing length word? */
- LogicalTapeWrite(tape, (void *) &writtenlen, sizeof(writtenlen));
+ LogicalTapeWrite(tape, &writtenlen, sizeof(writtenlen));
}
static void
base-commit: f4f2f2b84a0bbf9edbfc4a8684040a941cd6d085
--
2.39.0
0002-Add-const-to-BufFileWrite.patchtext/plain; charset=UTF-8; name=0002-Add-const-to-BufFileWrite.patchDownload
From 3fec92a1eaff51f639a8b5f9de7970ef2af03a0b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Fri, 23 Dec 2022 08:42:08 +0100
Subject: [PATCH 2/2] Add const to BufFileWrite
Make data buffer argument to BufFileWrite a const pointer and bubble
this up to various callers and related APIs. This makes the APIs
clearer and more consistent.
---
src/backend/access/gist/gistbuildbuffers.c | 4 ++--
src/backend/backup/backup_manifest.c | 4 ++--
src/backend/storage/file/buffile.c | 4 ++--
src/backend/utils/sort/logtape.c | 8 ++++----
src/include/storage/buffile.h | 2 +-
src/include/utils/logtape.h | 2 +-
6 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/src/backend/access/gist/gistbuildbuffers.c b/src/backend/access/gist/gistbuildbuffers.c
index 538e3880c9..60911e6aac 100644
--- a/src/backend/access/gist/gistbuildbuffers.c
+++ b/src/backend/access/gist/gistbuildbuffers.c
@@ -38,7 +38,7 @@ static long gistBuffersGetFreeBlock(GISTBuildBuffers *gfbb);
static void gistBuffersReleaseBlock(GISTBuildBuffers *gfbb, long blocknum);
static void ReadTempFileBlock(BufFile *file, long blknum, void *ptr);
-static void WriteTempFileBlock(BufFile *file, long blknum, void *ptr);
+static void WriteTempFileBlock(BufFile *file, long blknum, const void *ptr);
/*
@@ -764,7 +764,7 @@ ReadTempFileBlock(BufFile *file, long blknum, void *ptr)
}
static void
-WriteTempFileBlock(BufFile *file, long blknum, void *ptr)
+WriteTempFileBlock(BufFile *file, long blknum, const void *ptr)
{
if (BufFileSeekBlock(file, blknum) != 0)
elog(ERROR, "could not seek to block %ld in temporary file", blknum);
diff --git a/src/backend/backup/backup_manifest.c b/src/backend/backup/backup_manifest.c
index a54185fdab..68a2337a21 100644
--- a/src/backend/backup/backup_manifest.c
+++ b/src/backend/backup/backup_manifest.c
@@ -21,7 +21,7 @@
#include "utils/builtins.h"
#include "utils/json.h"
-static void AppendStringToManifest(backup_manifest_info *manifest, char *s);
+static void AppendStringToManifest(backup_manifest_info *manifest, const char *s);
/*
* Does the user want a backup manifest?
@@ -385,7 +385,7 @@ SendBackupManifest(backup_manifest_info *manifest, bbsink *sink)
* Append a cstring to the manifest.
*/
static void
-AppendStringToManifest(backup_manifest_info *manifest, char *s)
+AppendStringToManifest(backup_manifest_info *manifest, const char *s)
{
int len = strlen(s);
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index b07cca28d6..2202ba43f7 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -622,7 +622,7 @@ BufFileRead(BufFile *file, void *ptr, size_t size)
* ereport().
*/
void
-BufFileWrite(BufFile *file, void *ptr, size_t size)
+BufFileWrite(BufFile *file, const void *ptr, size_t size)
{
size_t nthistime;
@@ -655,7 +655,7 @@ BufFileWrite(BufFile *file, void *ptr, size_t size)
file->pos += nthistime;
if (file->nbytes < file->pos)
file->nbytes = file->pos;
- ptr = (char *) ptr + nthistime;
+ ptr = (const char *) ptr + nthistime;
size -= nthistime;
}
}
diff --git a/src/backend/utils/sort/logtape.c b/src/backend/utils/sort/logtape.c
index 9db220b7ea..23a5e7f2a0 100644
--- a/src/backend/utils/sort/logtape.c
+++ b/src/backend/utils/sort/logtape.c
@@ -220,7 +220,7 @@ struct LogicalTapeSet
};
static LogicalTape *ltsCreateTape(LogicalTapeSet *lts);
-static void ltsWriteBlock(LogicalTapeSet *lts, long blocknum, void *buffer);
+static void ltsWriteBlock(LogicalTapeSet *lts, long blocknum, const void *buffer);
static void ltsReadBlock(LogicalTapeSet *lts, long blocknum, void *buffer);
static long ltsGetBlock(LogicalTapeSet *lts, LogicalTape *lt);
static long ltsGetFreeBlock(LogicalTapeSet *lts);
@@ -235,7 +235,7 @@ static void ltsInitReadBuffer(LogicalTape *lt);
* No need for an error return convention; we ereport() on any error.
*/
static void
-ltsWriteBlock(LogicalTapeSet *lts, long blocknum, void *buffer)
+ltsWriteBlock(LogicalTapeSet *lts, long blocknum, const void *buffer)
{
/*
* BufFile does not support "holes", so if we're about to write a block
@@ -759,7 +759,7 @@ LogicalTapeSetForgetFreeSpace(LogicalTapeSet *lts)
* There are no error returns; we ereport() on failure.
*/
void
-LogicalTapeWrite(LogicalTape *lt, void *ptr, size_t size)
+LogicalTapeWrite(LogicalTape *lt, const void *ptr, size_t size)
{
LogicalTapeSet *lts = lt->tapeSet;
size_t nthistime;
@@ -826,7 +826,7 @@ LogicalTapeWrite(LogicalTape *lt, void *ptr, size_t size)
lt->pos += nthistime;
if (lt->nbytes < lt->pos)
lt->nbytes = lt->pos;
- ptr = (char *) ptr + nthistime;
+ ptr = (const char *) ptr + nthistime;
size -= nthistime;
}
}
diff --git a/src/include/storage/buffile.h b/src/include/storage/buffile.h
index a4922d1853..636a0b9071 100644
--- a/src/include/storage/buffile.h
+++ b/src/include/storage/buffile.h
@@ -39,7 +39,7 @@ typedef struct BufFile BufFile;
extern BufFile *BufFileCreateTemp(bool interXact);
extern void BufFileClose(BufFile *file);
extern size_t BufFileRead(BufFile *file, void *ptr, size_t size);
-extern void BufFileWrite(BufFile *file, void *ptr, size_t size);
+extern void BufFileWrite(BufFile *file, const void *ptr, size_t size);
extern int BufFileSeek(BufFile *file, int fileno, off_t offset, int whence);
extern void BufFileTell(BufFile *file, int *fileno, off_t *offset);
extern int BufFileSeekBlock(BufFile *file, long blknum);
diff --git a/src/include/utils/logtape.h b/src/include/utils/logtape.h
index 8c742ac491..1afd527e13 100644
--- a/src/include/utils/logtape.h
+++ b/src/include/utils/logtape.h
@@ -66,7 +66,7 @@ extern LogicalTape *LogicalTapeCreate(LogicalTapeSet *lts);
extern LogicalTape *LogicalTapeImport(LogicalTapeSet *lts, int worker, TapeShare *shared);
extern void LogicalTapeSetForgetFreeSpace(LogicalTapeSet *lts);
extern size_t LogicalTapeRead(LogicalTape *lt, void *ptr, size_t size);
-extern void LogicalTapeWrite(LogicalTape *lt, void *ptr, size_t size);
+extern void LogicalTapeWrite(LogicalTape *lt, const void *ptr, size_t size);
extern void LogicalTapeRewindForRead(LogicalTape *lt, size_t buffer_size);
extern void LogicalTapeFreeze(LogicalTape *lt, TapeShare *share);
extern size_t LogicalTapeBackspace(LogicalTape *lt, size_t size);
--
2.39.0
Re: File API cleanup
On 23.12.22 09:33, Peter Eisentraut wrote:
On 01.12.22 09:25, Peter Eisentraut wrote:
Here are a couple of patches that clean up the internal File API and
related things a bit:Here are two follow-up patches that clean up some stuff related to the
earlier patch set. I suspect these are all historically related.
Another patch under this theme. Here, I'm addressing the smgr API,
which effectively sits one level above the previously-dealt with "File" API.
Specifically, I'm changing the data buffer to void *, from char *, and
adding const where appropriate. As you can see in the patch, most
callers were unhappy with the previous arrangement and required casts.
(I pondered whether "Page" might be the right data type instead, since
the writers all write values of that type. But the readers don't read
into pages directly. So "Block" seemed more appropriate, and Block is
void * (bufmgr.h), so this makes sense.)
Attachments:
0001-Update-types-in-smgr-API.patchtext/plain; charset=UTF-8; name=0001-Update-types-in-smgr-API.patchDownload
From 0246d0cf9cd3472a3922c60f255c4daf3f39e829 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <peter@eisentraut.org>
Date: Mon, 20 Feb 2023 10:36:29 +0100
Subject: [PATCH] Update types in smgr API
Change data buffer to void *, from char *, and add const where
appropriate. This makes it match the File API (see also
2d4f1ba6cfc2f0a977f1c30bda9848041343e248) and stdio.
---
contrib/bloom/blinsert.c | 2 +-
src/backend/access/heap/rewriteheap.c | 4 ++--
src/backend/access/nbtree/nbtree.c | 2 +-
src/backend/access/nbtree/nbtsort.c | 6 +++---
src/backend/access/spgist/spginsert.c | 6 +++---
src/backend/storage/buffer/bufmgr.c | 4 ++--
src/backend/storage/smgr/md.c | 6 +++---
src/backend/storage/smgr/smgr.c | 12 ++++++------
src/include/storage/md.h | 6 +++---
src/include/storage/smgr.h | 6 +++---
10 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/contrib/bloom/blinsert.c b/contrib/bloom/blinsert.c
index f81442efb3..dcd8120895 100644
--- a/contrib/bloom/blinsert.c
+++ b/contrib/bloom/blinsert.c
@@ -178,7 +178,7 @@ blbuildempty(Relation index)
*/
PageSetChecksumInplace(metapage, BLOOM_METAPAGE_BLKNO);
smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BLOOM_METAPAGE_BLKNO,
- (char *) metapage, true);
+ metapage, true);
log_newpage(&(RelationGetSmgr(index))->smgr_rlocator.locator, INIT_FORKNUM,
BLOOM_METAPAGE_BLKNO, metapage, true);
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 8993c1ed5a..ae0282a70e 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -326,7 +326,7 @@ end_heap_rewrite(RewriteState state)
PageSetChecksumInplace(state->rs_buffer, state->rs_blockno);
smgrextend(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM,
- state->rs_blockno, (char *) state->rs_buffer, true);
+ state->rs_blockno, state->rs_buffer, true);
}
/*
@@ -692,7 +692,7 @@ raw_heap_insert(RewriteState state, HeapTuple tup)
PageSetChecksumInplace(page, state->rs_blockno);
smgrextend(RelationGetSmgr(state->rs_new_rel), MAIN_FORKNUM,
- state->rs_blockno, (char *) page, true);
+ state->rs_blockno, page, true);
state->rs_blockno++;
state->rs_buffer_valid = false;
diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c
index 1cc88da032..3f7b541e9d 100644
--- a/src/backend/access/nbtree/nbtree.c
+++ b/src/backend/access/nbtree/nbtree.c
@@ -165,7 +165,7 @@ btbuildempty(Relation index)
*/
PageSetChecksumInplace(metapage, BTREE_METAPAGE);
smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, BTREE_METAPAGE,
- (char *) metapage, true);
+ metapage, true);
log_newpage(&RelationGetSmgr(index)->smgr_rlocator.locator, INIT_FORKNUM,
BTREE_METAPAGE, metapage, true);
diff --git a/src/backend/access/nbtree/nbtsort.c b/src/backend/access/nbtree/nbtsort.c
index 67b7b1710c..02b9601bec 100644
--- a/src/backend/access/nbtree/nbtsort.c
+++ b/src/backend/access/nbtree/nbtsort.c
@@ -664,7 +664,7 @@ _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno)
/* don't set checksum for all-zero page */
smgrextend(RelationGetSmgr(wstate->index), MAIN_FORKNUM,
wstate->btws_pages_written++,
- (char *) wstate->btws_zeropage,
+ wstate->btws_zeropage,
true);
}
@@ -678,14 +678,14 @@ _bt_blwritepage(BTWriteState *wstate, Page page, BlockNumber blkno)
{
/* extending the file... */
smgrextend(RelationGetSmgr(wstate->index), MAIN_FORKNUM, blkno,
- (char *) page, true);
+ page, true);
wstate->btws_pages_written++;
}
else
{
/* overwriting a block we zero-filled before */
smgrwrite(RelationGetSmgr(wstate->index), MAIN_FORKNUM, blkno,
- (char *) page, true);
+ page, true);
}
pfree(page);
diff --git a/src/backend/access/spgist/spginsert.c b/src/backend/access/spgist/spginsert.c
index 4ed067f0d9..718a88335d 100644
--- a/src/backend/access/spgist/spginsert.c
+++ b/src/backend/access/spgist/spginsert.c
@@ -170,7 +170,7 @@ spgbuildempty(Relation index)
*/
PageSetChecksumInplace(page, SPGIST_METAPAGE_BLKNO);
smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, SPGIST_METAPAGE_BLKNO,
- (char *) page, true);
+ page, true);
log_newpage(&(RelationGetSmgr(index))->smgr_rlocator.locator, INIT_FORKNUM,
SPGIST_METAPAGE_BLKNO, page, true);
@@ -179,7 +179,7 @@ spgbuildempty(Relation index)
PageSetChecksumInplace(page, SPGIST_ROOT_BLKNO);
smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, SPGIST_ROOT_BLKNO,
- (char *) page, true);
+ page, true);
log_newpage(&(RelationGetSmgr(index))->smgr_rlocator.locator, INIT_FORKNUM,
SPGIST_ROOT_BLKNO, page, true);
@@ -188,7 +188,7 @@ spgbuildempty(Relation index)
PageSetChecksumInplace(page, SPGIST_NULL_BLKNO);
smgrwrite(RelationGetSmgr(index), INIT_FORKNUM, SPGIST_NULL_BLKNO,
- (char *) page, true);
+ page, true);
log_newpage(&(RelationGetSmgr(index))->smgr_rlocator.locator, INIT_FORKNUM,
SPGIST_NULL_BLKNO, page, true);
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 98904a7c05..0a05577b68 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -1003,7 +1003,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
/* new buffers are zero-filled */
MemSet((char *) bufBlock, 0, BLCKSZ);
/* don't set checksum for all-zero page */
- smgrextend(smgr, forkNum, blockNum, (char *) bufBlock, false);
+ smgrextend(smgr, forkNum, blockNum, bufBlock, false);
pgstat_count_io_op(io_object, io_context, IOOP_EXTEND);
@@ -1032,7 +1032,7 @@ ReadBuffer_common(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
else
INSTR_TIME_SET_ZERO(io_start);
- smgrread(smgr, forkNum, blockNum, (char *) bufBlock);
+ smgrread(smgr, forkNum, blockNum, bufBlock);
pgstat_count_io_op(io_object, io_context, IOOP_READ);
diff --git a/src/backend/storage/smgr/md.c b/src/backend/storage/smgr/md.c
index 8da813600c..352958e1fe 100644
--- a/src/backend/storage/smgr/md.c
+++ b/src/backend/storage/smgr/md.c
@@ -447,7 +447,7 @@ mdunlinkfork(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo)
*/
void
mdextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
- char *buffer, bool skipFsync)
+ const void *buffer, bool skipFsync)
{
off_t seekpos;
int nbytes;
@@ -669,7 +669,7 @@ mdwriteback(SMgrRelation reln, ForkNumber forknum,
*/
void
mdread(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
- char *buffer)
+ void *buffer)
{
off_t seekpos;
int nbytes;
@@ -734,7 +734,7 @@ mdread(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
*/
void
mdwrite(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
- char *buffer, bool skipFsync)
+ const void *buffer, bool skipFsync)
{
off_t seekpos;
int nbytes;
diff --git a/src/backend/storage/smgr/smgr.c b/src/backend/storage/smgr/smgr.c
index b2bd749d77..dc466e5414 100644
--- a/src/backend/storage/smgr/smgr.c
+++ b/src/backend/storage/smgr/smgr.c
@@ -49,13 +49,13 @@ typedef struct f_smgr
void (*smgr_unlink) (RelFileLocatorBackend rlocator, ForkNumber forknum,
bool isRedo);
void (*smgr_extend) (SMgrRelation reln, ForkNumber forknum,
- BlockNumber blocknum, char *buffer, bool skipFsync);
+ BlockNumber blocknum, const void *buffer, bool skipFsync);
bool (*smgr_prefetch) (SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
void (*smgr_read) (SMgrRelation reln, ForkNumber forknum,
- BlockNumber blocknum, char *buffer);
+ BlockNumber blocknum, void *buffer);
void (*smgr_write) (SMgrRelation reln, ForkNumber forknum,
- BlockNumber blocknum, char *buffer, bool skipFsync);
+ BlockNumber blocknum, const void *buffer, bool skipFsync);
void (*smgr_writeback) (SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, BlockNumber nblocks);
BlockNumber (*smgr_nblocks) (SMgrRelation reln, ForkNumber forknum);
@@ -491,7 +491,7 @@ smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo)
*/
void
smgrextend(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
- char *buffer, bool skipFsync)
+ const void *buffer, bool skipFsync)
{
smgrsw[reln->smgr_which].smgr_extend(reln, forknum, blocknum,
buffer, skipFsync);
@@ -530,7 +530,7 @@ smgrprefetch(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum)
*/
void
smgrread(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
- char *buffer)
+ void *buffer)
{
smgrsw[reln->smgr_which].smgr_read(reln, forknum, blocknum, buffer);
}
@@ -552,7 +552,7 @@ smgrread(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
*/
void
smgrwrite(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
- char *buffer, bool skipFsync)
+ const void *buffer, bool skipFsync)
{
smgrsw[reln->smgr_which].smgr_write(reln, forknum, blocknum,
buffer, skipFsync);
diff --git a/src/include/storage/md.h b/src/include/storage/md.h
index bcada9ff22..8f32af9ef3 100644
--- a/src/include/storage/md.h
+++ b/src/include/storage/md.h
@@ -27,13 +27,13 @@ extern void mdcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
extern bool mdexists(SMgrRelation reln, ForkNumber forknum);
extern void mdunlink(RelFileLocatorBackend rlocator, ForkNumber forknum, bool isRedo);
extern void mdextend(SMgrRelation reln, ForkNumber forknum,
- BlockNumber blocknum, char *buffer, bool skipFsync);
+ BlockNumber blocknum, const void *buffer, bool skipFsync);
extern bool mdprefetch(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
extern void mdread(SMgrRelation reln, ForkNumber forknum, BlockNumber blocknum,
- char *buffer);
+ void *buffer);
extern void mdwrite(SMgrRelation reln, ForkNumber forknum,
- BlockNumber blocknum, char *buffer, bool skipFsync);
+ BlockNumber blocknum, const void *buffer, bool skipFsync);
extern void mdwriteback(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, BlockNumber nblocks);
extern BlockNumber mdnblocks(SMgrRelation reln, ForkNumber forknum);
diff --git a/src/include/storage/smgr.h b/src/include/storage/smgr.h
index 56233c4d21..0935144f42 100644
--- a/src/include/storage/smgr.h
+++ b/src/include/storage/smgr.h
@@ -91,13 +91,13 @@ extern void smgrcreate(SMgrRelation reln, ForkNumber forknum, bool isRedo);
extern void smgrdosyncall(SMgrRelation *rels, int nrels);
extern void smgrdounlinkall(SMgrRelation *rels, int nrels, bool isRedo);
extern void smgrextend(SMgrRelation reln, ForkNumber forknum,
- BlockNumber blocknum, char *buffer, bool skipFsync);
+ BlockNumber blocknum, const void *buffer, bool skipFsync);
extern bool smgrprefetch(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum);
extern void smgrread(SMgrRelation reln, ForkNumber forknum,
- BlockNumber blocknum, char *buffer);
+ BlockNumber blocknum, void *buffer);
extern void smgrwrite(SMgrRelation reln, ForkNumber forknum,
- BlockNumber blocknum, char *buffer, bool skipFsync);
+ BlockNumber blocknum, const void *buffer, bool skipFsync);
extern void smgrwriteback(SMgrRelation reln, ForkNumber forknum,
BlockNumber blocknum, BlockNumber nblocks);
extern BlockNumber smgrnblocks(SMgrRelation reln, ForkNumber forknum);
--
2.39.2