Thread: 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().
Attachment
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.
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
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?
Attachment
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.
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.
Attachment
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.
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.