Re: pg_rewind in contrib

On 03/14/2015 02:31 PM, Amit Kapila wrote:
> Getting below linking error with Asserts enabled in Windows build.
>
> 1>xlogreader.obj : error LNK2019: unresolved external symbol
> ExceptionalCondition referenced in function
> XLogReadRecord
> 1>.\Debug\pg_rewind\pg_rewind.exe : fatal error LNK1120: 1 unresolved
> externals
>
> Am I doing anything wrong while building?

Works for me. Perhaps there were some changes to #includes that
inadvertently fixed it..

> 2.
> msvc\clean.bat has below way to clean xlogreader.c for pg_xlogdump,
> shouldn't something similar required for pg_rewind?
>
> REM clean up files copied into contrib\pg_xlogdump
> if exist contrib\pg_xlogdump\xlogreader.c del /q contrib
> \pg_xlogdump\xlogreader.c
> for %%f in (contrib\pg_xlogdump\*desc.c) do if not %%f==contrib\pg_xlogdump
> \rmgrdesc.c del /q %%f
> y.

I changed the way pg_xlogdump does that, and pg_rewind follows the new
example. (see http://www.postgresql.org/message-id/550B14A5.7060708@iki.fi)

> 4.
> Copyright notice contains variation in terms of years
>
> + * Copyright (c) 2010-2015, PostgreSQL Global Development Group
> + * Copyright (c) 2013-2015, PostgreSQL Global Development Group
>
> + * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
>
> Is there any particular reason for the same?

I've created many of the files by copying an old file and modifying
heavily. The copyright notices have been carried over from the original
files. Many of the files would still contain some of the original copied
code, while others might not. I'm not sure what the best way to deal
with that is - stamp everything as 2015, 2013-2015, or leave them as
they are. It doesn't really matter in practice.

> 5.
> + * relation files. Other forks are alwayes copied in toto, because we
> cannot
> + * reliably track changes to
> the, because WAL only contains block references
> + * for the main fork.
> + */
> +static bool
> +isRelDataFile(const
> char *path)
>
> Sentence near "track changes to the, .." looks incomplete.

Fixed, it was supposed to be "them", not "the".

> 6.
> +libpqConnect(const char *connstr)
> {
> ..
> + /*
> + * Also check that full_page-writes are enabled. We can get torn pages if
> + * a page is
> modified while we read it with pg_read_binary_file(), and we
> + * rely on full page images to fix them.
> +
> */
> + str = run_simple_query("SHOW full_page_writes");
> + if (strcmp(str, "on") != 0)
> +
> pg_fatal("full_page_writes must be enabled in the source server\n");
> + pg_free(str);
> ..
> }
>
> Do you think it is worth to mention this information in docs?

Added.

> 7.
> Function execute_pagemap() exists in both copy_fetch.c and
> libpq_fetch.c, are you expecting that they will get diverged
> in future?

They look pretty much identical, but the copy_file_range functions they
call are in fact separate functions, and differ a lot. I have renamed
the libpq version to avoid confusion.

I have committed this, with some more kibitzing. hope I have not missed
any comments given so far. Many thanks for the review, and please
continue reviewing and testing it :-).

- Heikki