From: | Magnus Hagander <magnus(at)hagander(dot)net> |
---|---|
To: | Daniel Gustafsson <daniel(at)yesql(dot)se> |
Cc: | Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Refectoring of receivelog.c |
Date: | 2016-03-11 10:15:57 |
Message-ID: | CABUevEzRdyq9AhFVTXLxdabEQCrgrufPc7Vgd93ZFQ4sdx72SQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Fri, Mar 11, 2016 at 9:40 AM, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
> > On 15 Feb 2016, at 14:46, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> >
> > On Mon, Feb 15, 2016 at 7:15 AM, Craig Ringer <craig(at)2ndquadrant(dot)com
> <mailto:craig(at)2ndquadrant(dot)com>> wrote:
> > On 15 February 2016 at 04:48, Magnus Hagander <magnus(at)hagander(dot)net
> <mailto:magnus(at)hagander(dot)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/
From | Date | Subject | |
---|---|---|---|
Next Message | Thomas Munro | 2016-03-11 10:30:33 | Re: WIP: Detecting SSI conflicts before reporting constraint violations |
Previous Message | Alexey Grishchenko | 2016-03-11 10:09:03 | Re: Endless loop calling PL/Python set returning functions |