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

This patch optimizes the above mentioned issues, as follows:

1. In the target data directory, do not delete the WAL files before the divergence.

2. When copying files from the source server, do not copy the WAL files before the divergence and the WAL files after the current WAL insert localtion.

This is the new pacth with TAP test and use Macro XLOGDIR.

On 2017-09-23 01:59:0, "Alexander Korotkov" <a.korotkov@postgrespro.ru> wrote:

On Fri, Sep 22, 2017 at 7:16 PM, chenhj <chjischj@163.com> wrote:

This is the new pacth with TAP test and use Macro XLOGDIR.

Good.  I took a quick look over the patch.

Why do you need master_query(), standby_query() and run_query() in RewindTest.pm?

You can do just $node_master->safe_psql() and $node_slave->safe_psql() instead.

Ooh, i did not notice that function.Thank you for your advice!

On 2017-09-28 01:29:29，"Alexander Korotkov" <a.korotkov@postgrespro.ru> wrote:

It appears that your patch conflicts with fc49e24f.  Please, rebase it.

Yes, i had rebased it, Please check the new patch. 

On 2017-09-29 00:43:18，"Alexander Korotkov" <a.korotkov@postgrespro.ru> wrote:

On Thu, Sep 28, 2017 at 6:44 PM, chenhj <chjischj@163.com> wrote:

On 2017-09-28 01:29:29，"Alexander Korotkov" <a.korotkov@postgrespro.ru> wrote:

It appears that your patch conflicts with fc49e24f.  Please, rebase it.

Yes, i had rebased it, Please check the new patch. 

Good, now it applies cleanly.

else if (strncmp(path, XLOGDIR"/", strlen(XLOGDIR"/")) == 0 &&
 IsXLogFileName(path + strlen(XLOGDIR"/")) &&
 (strcmp(path + strlen(XLOGDIR"/") + 8, divergence_wal_filename + 8) < 0 ||
  strcmp(path + strlen(XLOGDIR"/") + 8, last_source_wal_filename + 8) > 0))

According to our conding style, you should leave a space betwen XLOGDIF and "/".

Also, you do a trick by comparison xlog segment numbers using strcmp().  It's nice, but I would prefer seeing XLogFromFileName() here.  It would improve code readability and be less error prone during further modifications.

Thanks for advice!

I had modified it.

On 2017-09-29 05:31:51, "Alexander Korotkov" <a.korotkov@postgrespro.ru> wrote:

On Thu, Sep 28, 2017 at 10:52 PM, chenhj <chjischj@163.com> wrote:

On 2017-09-29 00:43:18，"Alexander Korotkov" <a.korotkov@postgrespro.ru> wrote:

On Thu, Sep 28, 2017 at 6:44 PM, chenhj <chjischj@163.com> wrote:

On 2017-09-28 01:29:29，"Alexander Korotkov" <a.korotkov@postgrespro.ru> wrote:

It appears that your patch conflicts with fc49e24f.  Please, rebase it.

Yes, i had rebased it, Please check the new patch. 

Good, now it applies cleanly.

else if (strncmp(path, XLOGDIR"/", strlen(XLOGDIR"/")) == 0 &&
 IsXLogFileName(path + strlen(XLOGDIR"/")) &&
 (strcmp(path + strlen(XLOGDIR"/") + 8, divergence_wal_filename + 8) < 0 ||
  strcmp(path + strlen(XLOGDIR"/") + 8, last_source_wal_filename + 8) > 0))

According to our conding style, you should leave a space betwen XLOGDIF and "/".

Also, you do a trick by comparison xlog segment numbers using strcmp().  It's nice, but I would prefer seeing XLogFromFileName() here.  It would improve code readability and be less error prone during further modifications.

Thanks for advice!

I had modified it.

OK. Patch becomes better.

I also have more general question.  Why do we need upper bound for segment number (last_source_segno)?  I understand the purpose of lower bound (divergence_segno) which save us from copying extra WAL files, but what is upper bound for?  As I understood, we anyway need to replay most recent WAL records to reach consistent state after pg_rewind.  I propose to remove last_source_segno unless I'm missing something.

Thanks for relay!

When checkpoint occurs, some old WAL files will be renamed as future WAL files for later use.

The upper bound for segment number (last_source_segno) is used to avoid copying these extra WAL files.

When the parameter max_wal_size or max_min_size is large,these may be many renamed old WAL files for reused.

For example, I have just looked at one of our production systems (max_wal_size = 64GB, min_wal_size = 2GB), 

the total size of WALs is about 30GB, and contains about 4GB renamed old WAL files.

[postgres@hostxxx pg_xlog]$ ll

...

-rw------- 1 postgres postgres 16777216 Sep 29 14:05 0000000100000BCF00000078

-rw------- 1 postgres postgres 16777216 Sep 29 14:05 0000000100000BCF00000079

-rw------- 1 postgres postgres 16777216 Sep 29 14:05 0000000100000BCF0000007A

-rw------- 1 postgres postgres 16777216 Sep 29 14:05 0000000100000BCF0000007B

-rw------- 1 postgres postgres 16777216 Sep 29 14:05 0000000100000BCF0000007C

-rw------- 1 postgres postgres 16777216 Sep 29 14:05 0000000100000BCF0000007D

-rw------- 1 postgres postgres 16777216 Sep 29 11:22 0000000100000BCF0000007E //after this, there are about 4GB WALs for reuse

-rw------- 1 postgres postgres 16777216 Sep 29 11:08 0000000100000BCF0000007F

-rw------- 1 postgres postgres 16777216 Sep 29 11:06 0000000100000BCF00000080

-rw------- 1 postgres postgres 16777216 Sep 29 12:05 0000000100000BCF00000081

-rw------- 1 postgres postgres 16777216 Sep 29 11:28 0000000100000BCF00000082

-rw------- 1 postgres postgres 16777216 Sep 29 11:06 0000000100000BCF00000083

...

On 2017-09-30 00:53:31，"chenhj" <chjischj@163.com> wrote:

On 2017-09-29 19:29:40，"Alexander Korotkov" <a.korotkov@postgrespro.ru> wrote:

On Fri, Sep 29, 2017 at 10:07 AM, chenhj <chjischj@163.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.