Re: [PATCH 03/14] Add simple xlogdump tool

On 2012-11-15 11:31:55 -0500, Peter Eisentraut wrote:
> 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

Good Point.

> >
> > 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

Dito.

> > +#
> > +#-------------------------------------------------------------------------
> > +
> > +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 $(at)$(X)
>
> This looks pretty evil, and there is no documentation about what it is
> supposed to do.

There has been some talk about this before and this clearly isn't an
acceptable solution. The previously stated idea was to split of the
_desc routines so we don't need to link with the whole backend.

Alvaro stared to work on that a bit:
http://archives.postgresql.org/message-id/1346268803-sup-9854%40alvh.no-ip.org

(What the above does is simply collect all backend object files, remove
main.o from that list an dlist them as dependencies.)

> Windows build support needs some thought.

I don't have the slightest clue how the windows build environment works,
is there still a problem if we only link to a very selected list of
backend object files? Or do we need to link them to some external
location?

> > 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.

Well, we're not linking to main.o which causes the problem, but yes,
really fixing this is definitely the goal, but not really possible yet.

> > +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).

True, its noticeable that this mostly was a development tool. But it
shouldn't stay that way. There have been several bugreports of late
where a bin/pg_xlogdump would have been very helpful...

> This should be the PostgreSQL version.
>
>
> also:
>
> no man page
>
> no nls.mk

Will try to provide some actually submittable version once the
xlogreader situation is finalized and the _desc routines are splitted...

Thanks!

Andres Freund

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