Обсуждение: 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.)

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

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.)

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.

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.)