Re: pg_rewind in contrib

Поиск
Список
Период
Сортировка
От Amit Kapila
Тема Re: pg_rewind in contrib
Дата
Msg-id CAA4eK1LL_cF-+qO2z=dFM4R3bdmuc0L6HhuLQAoUCZmrtqvVLA@mail.gmail.com
обсуждение исходный текст
Ответ на Re: pg_rewind in contrib  (Heikki Linnakangas <hlinnaka@iki.fi>)
Ответы Re: pg_rewind in contrib  (Alvaro Herrera <alvherre@2ndquadrant.com>)
Re: pg_rewind in contrib  (Heikki Linnakangas <hlinnaka@iki.fi>)
Список pgsql-hackers
Дерево обсуждения
On Fri, Mar 13, 2015 at 1:13 AM, Heikki Linnakangas <hlinnaka@iki.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

В списке pgsql-hackers по дате отправления:

Предыдущее
От: David Rowley
Дата:
Сообщение: Re: Performance improvement for joins where outer side is unique
Следующее
От: Petr Jelinek
Дата:
Сообщение: Re: Add min and max execute statement time in pg_stat_statement