Re: Add BufFileRead variants with short read and EOF detection

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.