Re: pg_rewind in contrib

On 01/06/2015 09:13 AM, Michael Paquier wrote:
> Some more comments:
> - Nitpicking: the header formats of filemap.c, datapagemap.c,
> datapagemap.h and util.h are incorrect (I pushed a fix about that in
> pg_rewind itself, feel free to pick it up).

Ah, fixed.

> - parsexlog.c has a copyright mention to Nippon Telegraph and
> Telephone Corporation. Cannot we drop it safely?

Removed. The file used to contain code for handling different WAL record
types that was originally copied from pg_lesslog, hence the NTT
copyright. However, that code is no longer there.

> - Error codes needs to be generated before building pg_rewind. If I do
> for example a simple configure followed by make I get a failure:
> $ ./configure
> $ cd contrib/pg_rewind && make
> In file included from parsexlog.c:16:
> In file included from ../../src/include/postgres.h:48:
> ../../src/include/utils/elog.h:69:10: fatal error: 'utils/errcodes.h'
> file not found
> #include "utils/errcodes.h"

Many other contrib modules have the same problem. And other
subdirectories too. It's not something that this patch needs to fix.

> - MSVC build is not supported yet. You need to do something similar to
> pg_xlogdump, aka some magic with for example xlogreader.c.
> - Build fails with MinGW as there is visibly some unportable code:

Fixed all the errors I got on MSVC. The biggest change was rewriting the
code that determines if a file is a relation file, based on its
filename. It used a regular expression, which I replaced with a bunch of
sscanf calls, and a cross-check that GetRelationPath() returns the same
filename.

> copy_fetch.c: In function 'check_samefile':
> copy_fetch.c:298:2: warning: passing argument 2 of '_fstat64i32' from incompatib
> le pointer type [enabled by default]
> if (fstat(fd1, &statbuf1) < 0)
> ^
> In file included from ../../src/include/port.h:283:0,
> from ../../src/include/c.h:1050,
> from ../../src/include/postgres_fe.h:25,
> from copy_fetch.c:10:
> c:\mingw\include\sys\stat.h:200:32: note: expected 'struct _stat64i32 *' but arg
> ument is of type 'struct stat *'
> __CRT_MAYBE_INLINE int __cdecl _fstat64i32(int desc, struct _stat64i32 *_stat)
> {

Strange. There isn't anything special about the fstat() calls in
pg_rewind. Do you get these from other modules that call fstat, e.g.
pg_stat_statements?

I did not see these warnings when building with MSVC, and don't have
MinGW installed currently.

> Hm. I think that this is something we should try to fix first
> upstream.

Yeah, possibly. But here's a new patch anyway.

- Heikki