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

On Wed, Dec 28, 2022 at 4:17 PM Peter Eisentraut
<peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>
> 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().
>

+1 for the new APIs. I have noticed that some of the existing places
use %m and the file path in messages which are not used in the new
common function.

--
With Regards,
Amit Kapila.

On Wed, 28 Dec 2022 at 16:17, Peter Eisentraut
<peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>
> 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().

The patch does not apply on top of HEAD as in [1], please post a rebased patch:
=== Applying patches on top of PostgreSQL commit ID
e351f85418313e97c203c73181757a007dfda6d0 ===
=== applying patch
./0001-Add-BufFileRead-variants-with-short-read-and-EOF-det.patch
patching file src/backend/access/gist/gistbuildbuffers.c
...
Hunk #1 FAILED at 38.
1 out of 1 hunk FAILED -- saving rejects to file
src/include/storage/buffile.h.rej

[1] - http://cfbot.cputube.org/patch_41_4089.log

Regards,
Vignesh

On 02.01.23 13:13, Amit Kapila wrote:
> On Wed, Dec 28, 2022 at 4:17 PM Peter Eisentraut
> <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>>
>> 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().
>>
>
> +1 for the new APIs. I have noticed that some of the existing places
> use %m and the file path in messages which are not used in the new
> common function.

The existing uses of %m are wrong. This was already fixed once in
7897e3bb902c557412645b82120f4d95f7474906, but the affected areas of code
were apparently developed at around the same time and didn't get the
fix. So I have attached a separate patch to fix this first, which could
be backpatched.

The original patch is then rebased on top of that. I have adjusted the
error message to include the file set name if available.

What this doesn't keep is the purpose of the temp file in some cases,
like "hash-join temporary file". We could maybe make this an additional
argument or an error context, but it seems cumbersome in any case.
Thoughts?

On Fri, Jan 6, 2023 at 6:18 PM Peter Eisentraut
<peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>
> On 02.01.23 13:13, Amit Kapila wrote:
> > On Wed, Dec 28, 2022 at 4:17 PM Peter Eisentraut
> > <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
> >>
> >> 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().
> >>
> >
> > +1 for the new APIs. I have noticed that some of the existing places
> > use %m and the file path in messages which are not used in the new
> > common function.
>
> The existing uses of %m are wrong. This was already fixed once in
> 7897e3bb902c557412645b82120f4d95f7474906, but the affected areas of code
> were apparently developed at around the same time and didn't get the
> fix. So I have attached a separate patch to fix this first, which could
> be backpatched.
>

+1. The patch is not getting applied due to a recent commit.

> The original patch is then rebased on top of that. I have adjusted the
> error message to include the file set name if available.
>
> What this doesn't keep is the purpose of the temp file in some cases,
> like "hash-join temporary file". We could maybe make this an additional
> argument or an error context, but it seems cumbersome in any case.
>

Yeah, we can do that but not sure if it is worth doing any of those
because there are already other places that don't use the exact
context.

--
With Regards,
Amit Kapila.

On 10.01.23 07:20, Amit Kapila wrote:
>> The existing uses of %m are wrong. This was already fixed once in
>> 7897e3bb902c557412645b82120f4d95f7474906, but the affected areas of code
>> were apparently developed at around the same time and didn't get the
>> fix. So I have attached a separate patch to fix this first, which could
>> be backpatched.
>>
> +1. The patch is not getting applied due to a recent commit.
>
>> The original patch is then rebased on top of that. I have adjusted the
>> error message to include the file set name if available.
>>
>> What this doesn't keep is the purpose of the temp file in some cases,
>> like "hash-join temporary file". We could maybe make this an additional
>> argument or an error context, but it seems cumbersome in any case.
>>
> Yeah, we can do that but not sure if it is worth doing any of those
> because there are already other places that don't use the exact
> context.

Ok, updated patches attached.

On Thu, Jan 12, 2023 at 2:44 PM Peter Eisentraut
<peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>
> On 10.01.23 07:20, Amit Kapila wrote:
> > Yeah, we can do that but not sure if it is worth doing any of those
> > because there are already other places that don't use the exact
> > context.
>
> Ok, updated patches attached.
>

Both the patches look good to me.

--
With Regards,
Amit Kapila.

On 14.01.23 07:01, Amit Kapila wrote:
> On Thu, Jan 12, 2023 at 2:44 PM Peter Eisentraut
> <peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
>>
>> On 10.01.23 07:20, Amit Kapila wrote:
>>> Yeah, we can do that but not sure if it is worth doing any of those
>>> because there are already other places that don't use the exact
>>> context.
>>
>> Ok, updated patches attached.
>
> Both the patches look good to me.

Committed, and the first part backpatched.