Обсуждение: 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 http://postgr.es/m/20200128020303.GA1552@paquier.xyz Patch, for v14, attached. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Вложения
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 opposedto 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 loopin 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 asopposed 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 asopposed 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