On Fri, Mar 11, 2016 at 9:40 AM, Daniel Gustafsson <dan...@yesql.se> wrote:
> > 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. > Pushed with updated comments and a named stsruct. Thanks! -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
