Re: BufFileRead() error signalling

2020-06-16 Thread Michael Paquier
On Tue, Jun 16, 2020 at 05:41:31PM +1200, Thomas Munro wrote: > Thanks! Pushed. I went ahead and made it void in master only. Thanks. -- Michael signature.asc Description: PGP signature

Re: BufFileRead() error signalling

2020-06-15 Thread Thomas Munro
On Tue, Jun 9, 2020 at 2:32 PM Michael Paquier wrote: > Looks fine to me. Are you planning to send an extra patch to switch > BufFileWrite() to void for 14~? Thanks! Pushed. I went ahead and made it void in master only.

Re: BufFileRead() error signalling

2020-06-08 Thread Michael Paquier
On Mon, Jun 08, 2020 at 05:50:44PM +1200, Thomas Munro wrote: > On Fri, Jun 5, 2020 at 8:14 PM Michael Paquier wrote:\ >> Anyway, why are we sure that it is fine to not complain even if >> BufFileWrite() does a partial write? fwrite() is mentioned at the >> top >> of the thread, but why is that

Re: BufFileRead() error signalling

2020-06-08 Thread Thomas Munro
On Tue, Jun 9, 2020 at 2:49 AM Alvaro Herrera wrote: > I think using our standard "exception" mechanism makes sense. As for > additional context, I think usefulness of the error messages would be > improved by showing the file path (because then user knows which > filesystem/tablespace was full,

Re: BufFileRead() error signalling

2020-06-08 Thread Alvaro Herrera
On 2020-Jun-08, Thomas Munro wrote: > Stepping back a bit, one of the problems here is that we tried to > model the functions on fread(), but we didn't provide the > companion ferror() and feof() functions, and then we were a bit fuzzy > on when errno is set, even though the functions don't >

Re: BufFileRead() error signalling

2020-06-07 Thread Thomas Munro
On Fri, Jun 5, 2020 at 8:14 PM Michael Paquier wrote: > On Fri, Jun 05, 2020 at 06:03:59PM +1200, Thomas Munro wrote: > > I didn't change BufFileWrite() to be void, to be friendly to existing > > callers outside the tree (if there are any), though I removed all the > > code that checks the return

Re: BufFileRead() error signalling

2020-06-05 Thread Michael Paquier
On Fri, Jun 05, 2020 at 06:03:59PM +1200, Thomas Munro wrote: > I didn't change BufFileWrite() to be void, to be friendly to existing > callers outside the tree (if there are any), though I removed all the > code that checks the return code. We can make it void later. Missing one entry in

Re: BufFileRead() error signalling

2020-06-05 Thread Thomas Munro
On Thu, May 28, 2020 at 7:10 PM Michael Paquier wrote: > On Wed, May 27, 2020 at 11:59:59AM -0400, Alvaro Herrera wrote: > > In the discussion that led to 811b6e36a9e2 I did suggest to use "read > > only M of N" instead, but there wasn't enough discussion on that fine > > point so we settled on

Re: BufFileRead() error signalling

2020-05-28 Thread Michael Paquier
On Wed, May 27, 2020 at 11:59:59AM -0400, Alvaro Herrera wrote: > In the discussion that led to 811b6e36a9e2 I did suggest to use "read > only M of N" instead, but there wasn't enough discussion on that fine > point so we settled on what you now call prevalent without a lot of > support

Re: BufFileRead() error signalling

2020-05-27 Thread Alvaro Herrera
On 2020-May-28, Thomas Munro wrote: > My indecision on the back-patching question has been resolved by your > crash report and a search on codesearch.debian.org. Great news! > BufFileRead() and BufFileWrite() aren't referenced by any of the > extensions they package, so by that standard it's OK

Re: BufFileRead() error signalling

2020-05-27 Thread Thomas Munro
On Thu, May 28, 2020 at 4:16 AM Alvaro Herrera wrote: > On 2020-Jan-27, Robert Haas wrote: > > OK, now that I've waxed eloquent on that topic, let me have a go at > > your actual questions. Regarding back-patching, I don't mind > > back-patching error handling patches like this, but I don't feel

Re: BufFileRead() error signalling

2020-05-27 Thread Robert Haas
On Wed, May 27, 2020 at 12:16 PM Alvaro Herrera wrote: > I do have evidence of postgres crashes because of a problem that could > be explained by this bug, so I +1 backpatching this to all supported > branches. > > (The problem I saw is a hash-join spilling data to temp tablespace, > which fills

Re: BufFileRead() error signalling

2020-05-27 Thread Alvaro Herrera
On 2020-Jan-27, Robert Haas wrote: > OK, now that I've waxed eloquent on that topic, let me have a go at > your actual questions. Regarding back-patching, I don't mind > back-patching error handling patches like this, but I don't feel it's > necessary if we have no evidence that data is actually

Re: BufFileRead() error signalling

2020-05-27 Thread Alvaro Herrera
On 2020-Jan-29, Michael Paquier wrote: > On Tue, Jan 28, 2020 at 03:51:54PM -0500, Robert Haas wrote: > > I quickly reread that thread and I don't see that there's any firm > > consensus there in favor of "read %d of %zu" over "read only %d of %zu > > bytes". Now, if most people prefer the

Re: BufFileRead() error signalling

2020-04-01 Thread David Steele
Hi Thomas, On 11/29/19 9:46 PM, Thomas Munro wrote: Ok. Here is a first attempt at that. It's been a few CFs since this patch received an update, though there has been plenty of discussion. Perhaps it would be best to mark it RwF until you have a chance to produce an update patch?

Re: BufFileRead() error signalling

2020-01-30 Thread Melanie Plageman
On Mon, Jan 27, 2020 at 7:09 AM Robert Haas wrote: > Rather than answering your actual question, I'd like to complain about > this: > > if (BufFileRead(file, ptr, BLCKSZ) != BLCKSZ) > - elog(ERROR, "could not read temporary file: %m"); > + elog(ERROR, "could not read temporary file"); > > I

Re: BufFileRead() error signalling

2020-01-29 Thread Michael Paquier
On Wed, Jan 29, 2020 at 10:01:31AM -0500, Robert Haas wrote: > Your grep misses one instance of 'read only %d of %d bytes' because > you grepped for %zu specifically, but that doesn't really change the > overall picture. Yes, the one in pg_checksums.c. That could actually be changed with a cast

Re: BufFileRead() error signalling

2020-01-29 Thread Robert Haas
On Wed, Jan 29, 2020 at 1:26 AM Michael Paquier wrote: > On Tue, Jan 28, 2020 at 03:51:54PM -0500, Robert Haas wrote: > > I quickly reread that thread and I don't see that there's any firm > > consensus there in favor of "read %d of %zu" over "read only %d of %zu > > bytes". Now, if most people

Re: BufFileRead() error signalling

2020-01-28 Thread Michael Paquier
On Tue, Jan 28, 2020 at 03:51:54PM -0500, Robert Haas wrote: > I quickly reread that thread and I don't see that there's any firm > consensus there in favor of "read %d of %zu" over "read only %d of %zu > bytes". Now, if most people prefer the former, so be it, but I don't > think that's clear

Re: BufFileRead() error signalling

2020-01-28 Thread Robert Haas
On Mon, Jan 27, 2020 at 9:03 PM Michael Paquier wrote: > That's actually not the best fit, because this does not take care of > the pluralization of the second message if you have only 1 byte to > read ;) But ... if you have only one byte to read, you cannot have a short read. > A second point

Re: BufFileRead() error signalling

2020-01-27 Thread Michael Paquier
On Mon, Jan 27, 2020 at 10:09:30AM -0500, Robert Haas wrote: > I recognize that your commit message explains this change by saying > that this code will now never be reached except as a result of a short > read, but I don't think that will necessarily be clear to future > readers of those code, or

Re: BufFileRead() error signalling

2020-01-27 Thread Robert Haas
On Fri, Jan 24, 2020 at 11:12 PM Thomas Munro wrote: > On Wed, Dec 11, 2019 at 2:07 AM Ibrar Ahmed wrote: > > You are checking file->dirty twice, first before calling the function and > > within the function too. Same for the Assert. For example. > > True. Thanks for the review. Before I post

Re: BufFileRead() error signalling

2020-01-24 Thread Thomas Munro
On Wed, Dec 11, 2019 at 2:07 AM Ibrar Ahmed wrote: > You are checking file->dirty twice, first before calling the function and > within the function too. Same for the Assert. For example. True. Thanks for the review. Before I post a new version, any opinions on whether to back-patch, and

Re: BufFileRead() error signalling

2019-12-10 Thread Ibrar Ahmed
You are checking file->dirty twice, first before calling the function and within the function too. Same for the Assert. For example. size_t BufFileRead(BufFile *file, void *ptr, size_t size) {        size_t      nread = 0;      size_t      nthistime;      if (file->dirty)      {            

Re: BufFileRead() error signalling

2019-11-29 Thread Thomas Munro
On Thu, Nov 21, 2019 at 11:31 AM Tom Lane wrote: > Thomas Munro writes: > > I think the choices are: (1) switch to ssize_t and return -1, (2) add > > at least one of BufFileEof(), BufFileError(), (3) have BufFileRead() > > raise errors with elog(). I lean towards (2), and I doubt we need > >

Re: BufFileRead() error signalling

2019-11-20 Thread Tom Lane
Thomas Munro writes: > As noted by Amit Khandhekar yesterday[1], BufFileLoad() silently eats > pread()'s error and makes them indistinguishable from EOF. That's definitely bad. > I think the choices are: (1) switch to ssize_t and return -1, (2) add > at least one of BufFileEof(),

Re: BufFileRead() error signalling

2019-11-20 Thread Thomas Munro
On Thu, Nov 21, 2019 at 10:50 AM Thomas Munro wrote: > As noted by Amit Khandhekar yesterday[1], BufFileLoad() silently eats Erm, Khandekar, sorry for the extra h!