Re: pg_rewind in contrib

On Fri, Mar 13, 2015 at 1:13 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
>
> The cause was a silly typo in truncate_target_file:
>
>> @@ -397,7 +397,7 @@ truncate_target_file(const char *path, off_t newsize)
>>
>> snprintf(dstpath, sizeof(dstpath), "%s/%s", datadir_target,
path);
>>
>> - fd = open(path, O_WRONLY, 0);
>> + fd = open(dstpath, O_WRONLY, 0);
>> if (fd < 0)
>> pg_fatal("could not open file \"%s\" for truncation:
%s\n",
>> dstpath, strerror(errno));
>
>
> Attached is a new version of the patch, including that fix, and rebased
over current git master.
>

I have verified that new patch has fixed the problem.

Few more observations:

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?

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

3.
+void
+remove_target(file_entry_t *entry)
{
..
+ case FILE_TYPE_REGULAR:
+ remove_target_symlink(entry->path);
+
break;
+
+ case FILE_TYPE_SYMLINK:
+ remove_target_file(entry-
>path);
+ break;
}

It seems function calls *_symlink and *_file are reversed, in reality it
might not matter much because the code for both functions is same,
but still it might diverge in future.

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?

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.

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?

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

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com