Re: [HACKERS] Refectoring of receivelog.c

[Magnus Hagander]

On Sun, Mar 13, 2016 at 12:15 AM, Tomas Vondra  wrote:

> On 03/11/2016 11:15 AM, Magnus Hagander wrote:
>
>>
>> ...
>
>>
>>
>> Pushed with updated comments and a named stsruct.
>>
>
> Pretty sure this memset call in pg_basebackup.c is incorrect, as it passes
> parameters in the wrong order:
>
> MemSet(&stream, sizeof(stream), 0);
>
> It seems benign, because we're setting all the fields explicitly, but gcc
> is nagging about it.
>

Indeed,that's backwards. Interestingly enough I thought I did a c&p between
that and the one in pg_receivexlog.c, but clearly I did not. And my gcc did
not nag about it.

Fixed, thanks for the pointer!


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Refectoring of receivelog.c

[Tomas Vondra]

On 03/11/2016 11:15 AM, Magnus Hagander wrote:



...



Pushed with updated comments and a named stsruct.


Pretty sure this memset call in pg_basebackup.c is incorrect, as it 
passes parameters in the wrong order:


MemSet(&stream, sizeof(stream), 0);

It seems benign, because we're setting all the fields explicitly, but 
gcc is nagging about it.


regards

--
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Refectoring of receivelog.c

[Magnus Hagander]

On Fri, Mar 11, 2016 at 9:40 AM, Daniel Gustafsson  wrote:

> > On 15 Feb 2016, at 14:46, Magnus Hagander  wrote:
> >
> > On Mon, Feb 15, 2016 at 7:15 AM, Craig Ringer  > wrote:
> > On 15 February 2016 at 04:48, Magnus Hagander  > 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/

Re: [HACKERS] Refectoring of receivelog.c

[Daniel Gustafsson]

> On 15 Feb 2016, at 14:46, Magnus Hagander  wrote:
> 
> On Mon, Feb 15, 2016 at 7:15 AM, Craig Ringer  > wrote:
> On 15 February 2016 at 04:48, Magnus Hagander  > 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

Re: [HACKERS] Refectoring of receivelog.c

[Magnus Hagander]

On Mon, Feb 15, 2016 at 7:15 AM, Craig Ringer  wrote:

> On 15 February 2016 at 04:48, Magnus Hagander  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.

Thanks!


-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] Refectoring of receivelog.c

[Craig Ringer]

On 15 February 2016 at 04:48, Magnus Hagander  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 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.

After recently working with the XLogReader I can't emphasise enough how
important *useful* comments on the fields in these sorts of structs are -
the XLogReaderState has ReadRecPtr, readSegNo + readOff + readLen,
currRecPtr AND latestPagePtr. Which actually do have comments, just not
super helpful ones.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

[HACKERS] Refectoring of receivelog.c

[Magnus Hagander]

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.

Here's an attempt to refactor the code to instead pass around a control
structure. I think it's a definite win already now, and we can't just keep
adding new functionality on top of the current one.

I'll proceed to work on the actual functionality I was working on to go on
top of this separately, but would appreciate a review of this part
independently. It's mostly mechanical, but there may definitely be mistakes
- or thinkos in the whole idea...

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
***
*** 372,381  typedef struct
  static int
  LogStreamerMain(logstreamer_param *param)
  {
! 	if (!ReceiveXlogStream(param->bgconn, param->startptr, param->timeline,
! 		   param->sysidentifier, param->xlogdir,
! 		   reached_end_position, standby_message_timeout,
! 		   NULL, false, true))
  
  		/*
  		 * Any errors will already have been reported in the function process,
--- 372,391 
  static int
  LogStreamerMain(logstreamer_param *param)
  {
! 	StreamCtl	stream;
! 
! 	MemSet(&stream, sizeof(stream), 0);
! 	stream.startpos = param->startptr;
! 	stream.timeline = param->timeline;
! 	stream.sysidentifier = param->sysidentifier;
! 	stream.stream_stop = reached_end_position;
! 	stream.standby_message_timeout = standby_message_timeout;
! 	stream.synchronous = false;
! 	stream.mark_done = true;
! 	stream.basedir = param->xlogdir;
! 	stream.partial_suffix = NULL;
! 
! 	if (!ReceiveXlogStream(param->bgconn, &stream))
  
  		/*
  		 * Any errors will already have been reported in the function process,
*** a/src/bin/pg_basebackup/pg_receivexlog.c
--- b/src/bin/pg_basebackup/pg_receivexlog.c
***
*** 276,285  FindStreamingStart(uint32 *tli)
  static void
  StreamLog(void)
  {
! 	XLogRecPtr	startpos,
! serverpos;
! 	TimeLineID	starttli,
! servertli;
  
  	/*
  	 * Connect in replication mode to the server
--- 276,286 
  static void
  StreamLog(void)
  {
! 	XLogRecPtr	serverpos;
! 	TimeLineID	servertli;
! 	StreamCtl	stream;
! 
! 	MemSet(&stream, 0, sizeof(stream));
  
  	/*
  	 * Connect in replication mode to the server
***
*** 311,327  StreamLog(void)
  	/*
  	 * Figure out where to start streaming.
  	 */
! 	startpos = FindStreamingStart(&starttli);
! 	if (startpos == InvalidXLogRecPtr)
  	{
! 		startpos = serverpos;
! 		starttli = servertli;
  	}
  
  	/*
  	 * Always start streaming at the beginning of a segment
  	 */
! 	startpos -= startpos % XLOG_SEG_SIZE;
  
  	/*
  	 * Start the replication
--- 312,328 
  	/*
  	 * Figure out where to start streaming.
  	 */
! 	stream.startpos = FindStreamingStart(&stream.timeline);
! 	if (stream.startpos == InvalidXLogRecPtr)
  	{
! 		stream.startpos = serverpos;
! 		stream.timeline = servertli;
  	}
  
  	/*
  	 * Always start streaming at the beginning of a segment
  	 */
! 	stream.startpos -= stream.startpos % XLOG_SEG_SIZE;
  
  	/*
  	 * Start the replication
***
*** 329,340  StreamLog(void)
  	if (verbose)
  		fprintf(stderr,
  _("%s: starting log streaming at %X/%X (timeline %u)\n"),
! progname, (uint32) (startpos >> 32), (uint32) startpos,
! starttli);
  
! 	ReceiveXlogStream(conn, startpos, starttli, NULL, basedir,
! 	  stop_streaming, standby_message_timeout, ".partial",
! 	  synchronous, false);
  
  	PQfinish(conn);
  	conn = NULL;
--- 330,346 
  	if (verbose)
  		fprintf(stderr,
  _("%s: starting log streaming at %X/%X (timeline %u)\n"),
! 		progname, (uint32) (stream.startpos >> 32), (uint32) stream.startpos,
! stream.timeline);
! 
! 	stream.stream_stop = stop_streaming;
! 	stream.standby_message_timeout = standby_message_timeout;
! 	stream.synchronous = synchronous;
! 	stream.mark_done = false;
! 	stream.basedir = basedir;
! 	stream.partial_suffix = ".partial";
  
! 	ReceiveXlogStream(conn, &stream);
  
  	PQfinish(conn);
  	conn = NULL;
*** a/src/bin/pg_basebackup/receivelog.c
--- b/src/bin/pg_basebackup/receivelog.c
***
*** 33,59  static XLogRecPtr lastFlushPosition = InvalidXLogRecPtr;
  
  static bool still_sending = true;		/* feedback still needs to be sent? */
  
! static PGresult *HandleCopyStream(PGconn *conn, XLogRecPtr startpos,
!  uint32 timeline, char *basedir,
! 			   stream_stop_callback stream_stop, int standby_message_timeout,
!  char *partial_suffix, XLogRecPtr *stoppos,
!  bool synchronous, bool mark_done);
  static int	CopyStreamPoll(PGconn *conn, long timeout_ms);
  static int	CopyStreamReceive(PGconn *conn, long t