[HACKERS] walsender parser patch

2011-01-10 Thread Magnus Hagander
Attached is an updated version of Heikki's patch to use a parser for
the walsender commands, instead of parsing things manually. It also
does some minor refactoring in walsender.c to break out
IdentifySystem() and StartReplication() to their own functions to make
it more readable.

While having an actual parser here isn't *necessary* at this point, it
makes things easier. And it will become increasingly useful as we add
new features (for example, the include all wal files option for
streaming base backup, and I'm sure that sync rep will require some
additional commands or changes to commands).

Any objections to doing this?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/
*** a/doc/src/sgml/protocol.sgml
--- b/doc/src/sgml/protocol.sgml
***
*** 1460,1475  The commands accepted in walsender mode are:
/varlistentry
  
varlistentry
! termBASE_BACKUP replaceableoptions/literal;/replaceablelabel//term
  listitem
   para
Instructs the server to start streaming a base backup.
!   The system will automatically be put in backup mode with the label
!   specified in replaceablelabel/ before the backup is started, and
!   taken out of it when the backup is complete. The following options
!   are accepted:
variablelist
 varlistentry
  termliteralPROGRESS//term
  listitem
   para
--- 1460,1486 
/varlistentry
  
varlistentry
! termBASE_BACKUP [literalLABEL/literal replaceable'label'/replaceable] [literalPROGRESS/literal]/term
  listitem
   para
Instructs the server to start streaming a base backup.
!   The system will automatically be put in backup mode before the backup
!   is started, and taken out of it when the backup is complete. The
!   following options are accepted:
variablelist
 varlistentry
+ termliteralLABEL/literal replaceable'label'/replaceable/term
+ listitem
+  para
+   Sets the label of the backup. If none is specified, a backup label
+   of literalbase backup/literal will be used. The quoting rules
+   for the label are the same as a standard SQL string with
+   xref linkend=guc-standard-conforming-strings turned on.
+  /para
+ /listitem
+/varlistentry
+ 
+varlistentry
  termliteralPROGRESS//term
  listitem
   para
*** a/src/backend/replication/Makefile
--- b/src/backend/replication/Makefile
***
*** 12,17  subdir = src/backend/replication
  top_builddir = ../../..
  include $(top_builddir)/src/Makefile.global
  
! OBJS = walsender.o walreceiverfuncs.o walreceiver.o basebackup.o
  
  include $(top_srcdir)/src/backend/common.mk
--- 12,40 
  top_builddir = ../../..
  include $(top_builddir)/src/Makefile.global
  
! OBJS = walsender.o walreceiverfuncs.o walreceiver.o basebackup.o \
! 	repl_gram.o
  
  include $(top_srcdir)/src/backend/common.mk
+ 
+ # repl_scanner is compiled as part of repl_gram
+ repl_gram.o: repl_scanner.c
+ 
+ # See notes in src/backend/parser/Makefile about the following two rules
+ 
+ repl_gram.c: repl_gram.y
+ ifdef BISON
+ 	$(BISON) -d $(BISONFLAGS) -o $@ $
+ else
+ 	@$(missing) bison $ $@
+ endif
+ 
+ repl_scanner.c: repl_scanner.l
+ ifdef FLEX
+ 	$(FLEX) $(FLEXFLAGS) -o'$@' $
+ else
+ 	@$(missing) flex $ $@
+ endif
+ 
+ # repl_gram.c and repl_scanner.c are in the distribution tarball, so
+ # they are not cleaned here.
*** a/src/backend/replication/basebackup.c
--- b/src/backend/replication/basebackup.c
***
*** 64,75  base_backup_cleanup(int code, Datum arg)
   * pg_stop_backup() for the user.
   */
  void
! SendBaseBackup(const char *options)
  {
  	DIR		   *dir;
  	struct dirent *de;
- 	char	   *backup_label = strchr(options, ';');
- 	bool		progress = false;
  	List	   *tablespaces = NIL;
  	tablespaceinfo *ti;
  	MemoryContext backup_context;
--- 64,73 
   * pg_stop_backup() for the user.
   */
  void
! SendBaseBackup(const char *backup_label, bool progress)
  {
  	DIR		   *dir;
  	struct dirent *de;
  	List	   *tablespaces = NIL;
  	tablespaceinfo *ti;
  	MemoryContext backup_context;
***
*** 83,100  SendBaseBackup(const char *options)
  	old_context = MemoryContextSwitchTo(backup_context);
  
  	if (backup_label == NULL)
! 		ereport(FATAL,
! (errcode(ERRCODE_PROTOCOL_VIOLATION),
!  errmsg(invalid base backup options: %s, options)));
! 	backup_label++;/* Walk past the semicolon */
! 
! 	/* Currently the only option string supported is PROGRESS */
! 	if (strncmp(options, PROGRESS, 8) == 0)
! 		progress = true;
! 	else if (options[0] != ';')
! 		ereport(FATAL,
! (errcode(ERRCODE_PROTOCOL_VIOLATION),
!  errmsg(invalid base backup options: %s, options)));
  
  	/* Make sure we can open the directory with tablespaces in it */
  	dir = AllocateDir(pg_tblspc);
--- 81,87 
  	old_context = 

Re: [HACKERS] walsender parser patch

2011-01-10 Thread Dimitri Fontaine
Magnus Hagander mag...@hagander.net writes:
 Attached is an updated version of Heikki's patch to use a parser for
 the walsender commands, instead of parsing things manually. It also
 does some minor refactoring in walsender.c to break out
 IdentifySystem() and StartReplication() to their own functions to make
 it more readable.

Nice work.

 While having an actual parser here isn't *necessary* at this point, it
 makes things easier. And it will become increasingly useful as we add
 new features (for example, the include all wal files option for
 streaming base backup, and I'm sure that sync rep will require some
 additional commands or changes to commands).

Is that option on the roadmap for 9.1? That's huge! Go Magnus!

 Any objections to doing this?

None here :)
-- 
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

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