Re: Add BufFileRead variants with short read and EOF detection - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Add BufFileRead variants with short read and EOF detection
Date
Msg-id CAA4eK1Jh1t8910Gxkj2cf=+VKqnD5j+8wu8GdFZ-QV0LQcGdEg@mail.gmail.com
Whole thread Raw
In response to Re: Add BufFileRead variants with short read and EOF detection  (Peter Eisentraut <peter.eisentraut@enterprisedb.com>)
Responses Re: Add BufFileRead variants with short read and EOF detection
List pgsql-hackers
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.



pgsql-hackers by date:

Previous
From: Brar Piening
Date:
Subject: Re: doc: add missing "id" attributes to extension packaging page
Next
From: Peter Geoghegan
Date:
Subject: Re: Fixing a couple of buglets in how VACUUM sets visibility map bits