On 11/14/12 8:17 PM, Andres Freund wrote:
> diff --git a/src/bin/Makefile b/src/bin/Makefile
> index b4dfdba..9992f7a 100644
> --- a/src/bin/Makefile
> +++ b/src/bin/Makefile
> @@ -14,7 +14,7 @@ top_builddir = ../..
>  include $(top_builddir)/src/Makefile.global
>  
>  SUBDIRS = initdb pg_ctl pg_dump \
> -     psql scripts pg_config pg_controldata pg_resetxlog pg_basebackup
> +     psql scripts pg_config pg_controldata pg_resetxlog pg_basebackup 
> xlogdump

should be pg_xlogdump

>  
>  ifeq ($(PORTNAME), win32)
>  SUBDIRS += pgevent
> diff --git a/src/bin/xlogdump/Makefile b/src/bin/xlogdump/Makefile
> new file mode 100644
> index 0000000..d54640a
> --- /dev/null
> +++ b/src/bin/xlogdump/Makefile
> @@ -0,0 +1,25 @@
> +#-------------------------------------------------------------------------
> +#
> +# Makefile for src/bin/xlogdump
> +#
> +# Copyright (c) 1998-2012, PostgreSQL Global Development Group
> +#
> +# src/bin/pg_resetxlog/Makefile

fix that

> +#
> +#-------------------------------------------------------------------------
> +
> +PGFILEDESC = "xlogdump"
> +PGAPPICON=win32
> +
> +subdir = src/bin/xlogdump
> +top_builddir = ../../..
> +include $(top_builddir)/src/Makefile.global
> +
> +OBJS= xlogdump.o \
> +      $(WIN32RES)
> +
> +all: xlogdump
> +
> +
> +xlogdump: $(OBJS) $(shell find ../../backend ../../timezone -name 
> objfiles.txt|xargs cat|tr -s " " "\012"|grep -v /main.o|sed 
> 's/^/..\/..\/..\//')
> +     $(CC) $(CFLAGS) $^ $(LDFLAGS) $(LDFLAGS_EX) $(LIBS) -o $@$(X)

This looks pretty evil, and there is no documentation about what it is
supposed to do.

Windows build support needs some thought.


> diff --git a/src/bin/xlogdump/xlogdump.c b/src/bin/xlogdump/xlogdump.c
> new file mode 100644
> index 0000000..0f984e4
> --- /dev/null
> +++ b/src/bin/xlogdump/xlogdump.c
> @@ -0,0 +1,468 @@
> +#include "postgres.h"
> +
> +#include <unistd.h>
> +
> +#include "access/xlogreader.h"
> +#include "access/rmgr.h"
> +#include "miscadmin.h"
> +#include "storage/ipc.h"
> +#include "utils/memutils.h"
> +#include "utils/guc.h"
> +
> +#include "getopt_long.h"
> +
> +/*
> + * needs to be declared because otherwise its defined in main.c which we 
> cannot
> + * link from here.
> + */
> +const char *progname = "xlogdump";

Which may be a reason not to link with main.o.  We generally don't want
to hardcode the program name inside the program.

> +static void
> +usage(void)
> +{
> +     printf(_("%s reads/writes postgres transaction logs for 
> debugging.\n\n"),
> +                progname);
> +     printf(_("Usage:\n"));
> +     printf(_("  %s [OPTION]...\n"), progname);
> +     printf(_("\nOptions:\n"));
> +     printf(_("  -v, --version          output version information, then 
> exit\n"));
> +     printf(_("  -h, --help             show this help, then exit\n"));
> +     printf(_("  -s, --start            from where recptr onwards to 
> read\n"));
> +     printf(_("  -e, --end              up to which recptr to read\n"));
> +     printf(_("  -t, --timeline         which timeline do we want to 
> read\n"));
> +     printf(_("  -i, --inpath           from where do we want to read? 
> cwd/pg_xlog is the default\n"));
> +     printf(_("  -o, --output           where to write [start, end]\n"));
> +     printf(_("  -f, --file             wal file to parse\n"));
> +}

Options list should be in alphabetic order (or some other less random
order).  Most of these descriptions are not very intelligible (at least
without additional documentation).

> +
> +int main(int argc, char **argv)
> +{
> +     uint32 xlogid;
> +     uint32 xrecoff;
> +     XLogReaderState *xlogreader_state;
> +     XLogDumpPrivateData private;
> +     XLogRecPtr from = InvalidXLogRecPtr;
> +     XLogRecPtr to = InvalidXLogRecPtr;
> +     bool bad_argument = false;
> +
> +     static struct option long_options[] = {
> +             {"help", no_argument, NULL, 'h'},
> +             {"version", no_argument, NULL, 'v'},

Standard letters for help and version are ? and V.

> +             {"start", required_argument, NULL, 's'},
> +             {"end", required_argument, NULL, 'e'},
> +             {"timeline", required_argument, NULL, 't'},
> +             {"inpath", required_argument, NULL, 'i'},
> +             {"outpath", required_argument, NULL, 'o'},
> +             {"file", required_argument, NULL, 'f'},
> +             {NULL, 0, NULL, 0}
> +     };
> +     int                     c;
> +     int                     option_index;
> +
> +     memset(&private, 0, sizeof(XLogDumpPrivateData));
> +
> +     while ((c = getopt_long(argc, argv, "hvs:e:t:i:o:f:",

This could also be in a less random order.

> +                                                     long_options, 
> &option_index)) != -1)
> +     {
> +             switch (c)
> +             {
> +                     case 'h':
> +                             usage();
> +                             exit(0);
> +                             break;
> +                     case 'v':
> +                             printf("Version: 0.1\n");
> +                             exit(0);
> +                             break;

This should be the PostgreSQL version.


also:

no man page

no nls.mk



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

Reply via email to