> On 15 Feb 2016, at 14:46, Magnus Hagander <mag...@hagander.net> wrote: > > On Mon, Feb 15, 2016 at 7:15 AM, Craig Ringer <cr...@2ndquadrant.com > <mailto:cr...@2ndquadrant.com>> wrote: > On 15 February 2016 at 04:48, Magnus Hagander <mag...@hagander.net > <mailto:mag...@hagander.net>> wrote: > I was working on adding the tar streaming functionality we talked about at > the developer meeting to pg_basebackup, and rapidly ran across the issue that > Andres has been complaining about for a while. The code in receivelog.c just > passes an insane number of parameters around. Adding or changing even a small > thing ends up touching a huge number of places. > > Other than the lack of comments on the fields in StreamCtl to indicate their > functions I think this looks good. > > I copied that lack of comments from the previous implementation :P > > But yeah, I agree, it's probably a good idea to add them. > > I didn't find any mistakes, but I do admit my eyes started glazing over after > a bit. > > I'd rather not have StreamCtl as a typedef of an anonymous struct if it's > exposed in a header though. It should have a name that can be used in forward > declarations etc. > > It's not exactly uncommon with anonymous ones either in our code, but I see > no problem adding that.
Short review of this patch: The patch applies, and builds, cleanly on top of master as of today. No new functionality is introduced and thus no new tests or doc patches etc are applicable. The main point of the patch is to improve readability and reduce the number of parameters passed, goals which are reached. However, I agree with Craig that comments on the struct fields should be added to improve readability even further. The comment on ReceiveXlogStream() also now reads a bit strange since it describes fields inside the StreamCtl without referencing StreamCtl at all. For first time readers of the code it could perhaps be helpful with a brief note that the discussed parameters are in StreamCtl to avoid potential confusion. Overall I think this patch is an improvement on the current code and consider it ready for committer. cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
