Horton

Thread

Commits

  1. Add BufFileRead variants with short read and EOF detection

  2. Fix some BufFileRead() error reporting

  3. Push lpp variable closer to usage in heapgetpage()

  4. Fix buffile.c error handling.

  1. Add BufFileRead variants with short read and EOF detection

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-12-28T10:47:02Z

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

  2. Re: Add BufFileRead variants with short read and EOF detection

    Amit Kapila <amit.kapila16@gmail.com> — 2023-01-02T12:13:29Z

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

  3. Re: Add BufFileRead variants with short read and EOF detection

    vignesh C <vignesh21@gmail.com> — 2023-01-03T10:33:30Z

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

  4. Re: Add BufFileRead variants with short read and EOF detection

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-06T12:48:49Z

    On 02.01.23 13:13, Amit Kapila wrote:
> On Wed, Dec 28, 2022 at 4:17 PM Peter Eisentraut
> <peter.eisentraut@enterprisedb.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?

  5. Re: Add BufFileRead variants with short read and EOF detection

    Amit Kapila <amit.kapila16@gmail.com> — 2023-01-10T06:20:15Z

    On Fri, Jan 6, 2023 at 6:18 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 02.01.23 13:13, Amit Kapila wrote:
> > On Wed, Dec 28, 2022 at 4:17 PM Peter Eisentraut
> > <peter.eisentraut@enterprisedb.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.

  6. Re: Add BufFileRead variants with short read and EOF detection

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-12T09:14:11Z

    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.

  7. Re: Add BufFileRead variants with short read and EOF detection

    Amit Kapila <amit.kapila16@gmail.com> — 2023-01-14T06:01:23Z

    On Thu, Jan 12, 2023 at 2:44 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.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.

  8. Re: Add BufFileRead variants with short read and EOF detection

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2023-01-16T10:16:38Z

    On 14.01.23 07:01, Amit Kapila wrote:
> On Thu, Jan 12, 2023 at 2:44 PM Peter Eisentraut
> <peter.eisentraut@enterprisedb.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.