File API cleanup
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.)From 0ca35b75e383272356ba49211913d8aead5ca10d Mon Sep 17 00:00:00 2001 From: Peter Eisentraut 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 From 8d0530fbf9095bc16fa3465b69aef2c31c4c3c9b Mon Sep 17 00:00:00 2001 From: Peter Eisentraut 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/e
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. From cec7ec188f2473d983b048b9e1eddc7dbc3a4241 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut 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; -
Re: File API cleanup
On Thu, Dec 1, 2022 at 1:55 PM Peter Eisentraut 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 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.) From 0246d0cf9cd3472a3922c60f255c4daf3f39e829 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut 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) {