improving basebackup.c's file-reading code
Hi,
basebackup.c's code to read from files uses fread() and friends. This
is not great, because it's not documented to set errno. See commit
286af0ce12117bc673b97df6228d1a666594d247 and related discussion. It
seems like a better idea would be to use pg_pgread(), which not only
does set errno, but also lets us eliminate a bit of code that uses
fseek().
There are a couple of other things here that can also be improved. One
is that it seems like a good idea to set a wait event while doing I/O
here, as we do elsewhere. Another is that it seems like a good idea to
report short reads in a non-confusing, non-wrong sort of way. I here
adopted the convention previously mentioned in
/messages/by-id/20200128020303.GA1552@paquier.xyz
Patch, for v14, attached.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
v1-0001-Improve-server-code-to-read-files-as-part-of-a-ba.patchapplication/octet-stream; name=v1-0001-Improve-server-code-to-read-files-as-part-of-a-ba.patchDownload+88-69
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed
The idea and the patch looks good to me.
It makes sense to change fread to basebackup_read_file which internally calls pg_pread which is a portable function as opposed to read. As you've mentioned, this is much better for error handling.
I guess it is more of a personal choice, but I would suggest making the while conditions consistent as well. The while loop in the patch @ line 121 conditions over return value of "basebackup_read_file" whereas @ line 177, it has a condition "(len < statbuf->st_size)".
The new status of this patch is: Ready for Committer
On Wed, Apr 29, 2020 at 5:52 AM Hamid Akhtar <hamid.akhtar@gmail.com> wrote:
The idea and the patch looks good to me.
It makes sense to change fread to basebackup_read_file which internally calls pg_pread which is a portable function as opposed to read. As you've mentioned, this is much better for error handling.
Thanks for the review. I have now committed the patch, after rebasing
and adjusting one comment slightly.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 17 Jun 2020, at 17:45, Robert Haas <robertmhaas@gmail.com> wrote:
On Wed, Apr 29, 2020 at 5:52 AM Hamid Akhtar <hamid.akhtar@gmail.com> wrote:
The idea and the patch looks good to me.
It makes sense to change fread to basebackup_read_file which internally calls pg_pread which is a portable function as opposed to read. As you've mentioned, this is much better for error handling.
Thanks for the review. I have now committed the patch, after rebasing
and adjusting one comment slightly.
As this went in, can we close the 2020-07 CF entry or is there anything left in
the patchseries?
cheers ./daniel
On Thu, Jun 25, 2020 at 10:29 AM Daniel Gustafsson <daniel@yesql.se> wrote:
As this went in, can we close the 2020-07 CF entry or is there anything left in
the patchseries?
Done. Thanks for the reminder.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company