Re: [PATCH]make pg_rewind to not copy useless WAL files

On Fri, Sep 29, 2017 at 8:10 PM, chenhj <chjischj(at)163(dot)com> wrote:

> On 2017-09-30 00:53:31，"chenhj" <chjischj(at)163(dot)com> wrote:
>
> On 2017-09-29 19:29:40，"Alexander Korotkov" <a(dot)korotkov(at)postgrespro(dot)ru>
> wrote:
>
> On Fri, Sep 29, 2017 at 10:07 AM, chenhj <chjischj(at)163(dot)com> wrote:
>>
>>
>>
> OK. That makes sense. Thank you for the explanation.
>
> I still have some minor comments.
>
>
>> /*
>> + * Save the WAL filenames of the divergence and the current WAL insert
>> + * location of the source server. Later only the WAL files between
>> those
>> + * would be copied to the target data directory.
>>
>
> Comment is outdated. We don't save filenames anymore, now we save segment
> numbers.
>
>
>> + * Note:The later generated WAL files in the source server before the
>> end
>> + * of the copy of the data files must be made available when the
>> target
>> + * server is started. This can be done by configuring the target
>> server as
>> + * a standby of the source server.
>> + */
>
>
> You miss space after "Note:". Also, it seems reasonable for me to leave
> empty line before "Note:".
>
> # Setup parameter for WAL reclaim
>
>
> Parameter*s*, because you're setting up multiple of them.
>
> # The accuracy of imodification from pg_ls_waldir() is seconds, so sleep
>> one seconds
>
>
> One second without "s".
>
> Also, please check empty lines in 006_wal_copy.pl to be just empty lines
> without tabs.
>
>
> Thanks for your comments, i had fix above problems.
> And also add several line breaks at long line in 006_wal_copy.pl
> Please check this patch again.
>
>
> Sorry, patch v6 did not remove tabs in two empty lines, please use the new
> one.
>

Great. Now code of this patch looks good for me.
However, we forgot about documentation.

<para>
> The result is equivalent to replacing the target data directory with the
> source one. Only changed blocks from relation files are copied;
> all other files are copied in full, including configuration files. The
> advantage of <application>pg_rewind</> over taking a new base backup, or
> tools like <application>rsync</>, is that <application>pg_rewind</> does
> not require reading through unchanged blocks in the cluster. This makes
> it a lot faster when the database is large and only a small
> fraction of blocks differ between the clusters.
> </para>

At least, this paragraph need to be adjusted, because it states whose files
are copied. And probably latter paragraphs whose state about WAL files.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company