Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On 2017-10-02 23:24:30,"Alexander Korotkov" wrote: On Sun, Oct 1, 2017 at 8:27 PM, chenhj wrote: Now, this patch looks good for me. It applies cleanly, builds cleanly, passes regression tests, new functionality is covered by regression tests. Code is OK for me and docs too. I'm marking this patch as "Ready for committer". BTW, authors field in the commitfest app is empty (https://commitfest.postgresql.org/15/1302/). Please, put your name there. I hope this patch will be committed during 2017-11 commitfest. Be ready to rebase this patch if needed. Thank you for your work. I had filled the authors field of this patch in commitfest, and will rebase this patch if needed. Thank you for your help! -- Best Regards, Chen Huajun
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On Sun, Oct 1, 2017 at 8:27 PM, chenhj wrote: > On 2017-10-01 04:09:19,"Alexander Korotkov" > wrote: > > On Sat, Sep 30, 2017 at 8:18 PM, chenhj wrote: > >> On 2017-09-30 02:17:54,"Alexander Korotkov" > > wrote: >> >> >> Great. Now code of this patch looks good for me. >> However, we forgot about documentation. >> >> >>>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 pg_rewind over taking a new base backup, >>> or >>>tools like rsync, is that 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. >>> >> >> >> At least, this paragraph need to be adjusted, because it states whose >> files are copied. And probably latter paragraphs whose state about WAL >> files. >> >> >> >> Your are rigth. >> I wrote a draft as following, but i'm afraid whether the english >> statement is accurate. >> > > I'm not native english speaker too :( > > Only the WAL files between the point of divergence and the current WAL >> insert location of the source server are copied, *for* other WAL files are >> useless for the target server. > > > I'm not sure about this usage of word *for*. For me, it probably should > be just removed. Rest of changes looks good for me. Please, integrate > them into the patch. > > > I had removed the *for* , Pleae check the new patch again. > Now, this patch looks good for me. It applies cleanly, builds cleanly, passes regression tests, new functionality is covered by regression tests. Code is OK for me and docs too. I'm marking this patch as "Ready for committer". BTW, authors field in the commitfest app is empty (https://commitfest.postgresql.org/15/1302/). Please, put your name there. I hope this patch will be committed during 2017-11 commitfest. Be ready to rebase this patch if needed. Thank you for your work. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On 2017-10-01 04:09:19,"Alexander Korotkov" wrote: On Sat, Sep 30, 2017 at 8:18 PM, chenhj wrote: On 2017-09-30 02:17:54,"Alexander Korotkov" wrote: Great. Now code of this patch looks good for me. However, we forgot about documentation. 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 pg_rewind over taking a new base backup, or tools like rsync, is that 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. At least, this paragraph need to be adjusted, because it states whose files are copied. And probably latter paragraphs whose state about WAL files. Your are rigth. I wrote a draft as following, but i'm afraid whether the english statement is accurate. I'm not native english speaker too :( Only the WAL files between the point of divergence and the current WAL insert location of the source server are copied, *for* other WAL files are useless for the target server. I'm not sure about this usage of word *for*. For me, it probably should be just removed. Rest of changes looks good for me. Please, integrate them into the patch. I had removed the *for* , Pleae check the new patch again. --- Best Regards, Chen Huajun pg_rewind_wal_copy_reduce_v8.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On Sat, Sep 30, 2017 at 8:18 PM, chenhj wrote: > On 2017-09-30 02:17:54,"Alexander Korotkov" > wrote: > > > Great. Now code of this patch looks good for me. > However, we forgot about documentation. > > >>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 pg_rewind over taking a new base backup, >> or >>tools like rsync, is that 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. >> > > > At least, this paragraph need to be adjusted, because it states whose > files are copied. And probably latter paragraphs whose state about WAL > files. > > > > Your are rigth. > I wrote a draft as following, but i'm afraid whether the english statement > is accurate. > I'm not native english speaker too :( Only the WAL files between the point of divergence and the current WAL > insert location of the source server are copied, *for* other WAL files are > useless for the target server. I'm not sure about this usage of word *for*. For me, it probably should be just removed. Rest of changes looks good for me. Please, integrate them into the patch. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On 2017-09-30 02:17:54,"Alexander Korotkov" wrote: Great. Now code of this patch looks good for me. However, we forgot about documentation. 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 pg_rewind over taking a new base backup, or tools like rsync, is that 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. At least, this paragraph need to be adjusted, because it states whose files are copied. And probably latter paragraphs whose state about WAL files. Your are rigth. I wrote a draft as following, but i'm afraid whether the english statement is accurate. -- diff --git a/doc/src/sgml/ref/pg_rewind.sgml b/doc/src/sgml/ref/pg_rewind.sgml index d5430d4..bcd094b 100644 --- a/doc/src/sgml/ref/pg_rewind.sgml +++ b/doc/src/sgml/ref/pg_rewind.sgml @@ -50,9 +50,12 @@ PostgreSQL documentation 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 pg_rewind over taking a new base backup, or - tools like rsync, is that pg_rewind does + all other files except WAL are copied in full, including configuration + files. Only the WAL files between the point of divergence and the current + WAL insert location of the source server are copied, for other WAL files + are useless for the target server. The advantage of + pg_rewind over taking a new base backup, or tools + like rsync, is that 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. @@ -231,7 +234,7 @@ PostgreSQL documentation Copy all other files such as pg_xact and configuration files from the source cluster to the target cluster - (everything except the relation files). + (everything except the relation files and some WAL files). -- Best Regars, Chen Huajun
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On Fri, Sep 29, 2017 at 8:10 PM, chenhj wrote: > On 2017-09-30 00:53:31,"chenhj" wrote: > > On 2017-09-29 19:29:40,"Alexander Korotkov" > wrote: > > On Fri, Sep 29, 2017 at 10:07 AM, chenhj 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. >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 pg_rewind over taking a new base backup, or >tools like rsync, is that 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. > 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
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On 2017-09-30 00:53:31,"chenhj" wrote: On 2017-09-29 19:29:40,"Alexander Korotkov" wrote: On Fri, Sep 29, 2017 at 10:07 AM, chenhj 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. Best Regards, Chen Huajun pg_rewind_wal_copy_reduce_v7.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On 2017-09-29 19:29:40,"Alexander Korotkov" wrote: On Fri, Sep 29, 2017 at 10:07 AM, chenhj 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. -- Best Regards Chen Huajun pg_rewind_wal_copy_reduce_v6.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On Fri, Sep 29, 2017 at 10:07 AM, chenhj wrote: > On 2017-09-29 05:31:51, "Alexander Korotkov" > wrote: > > On Thu, Sep 28, 2017 at 10:52 PM, chenhj wrote: > >> On 2017-09-29 00:43:18,"Alexander Korotkov" >> wrote: >> >> On Thu, Sep 28, 2017 at 6:44 PM, chenhj wrote: >> >>> On 2017-09-28 01:29:29,"Alexander Korotkov" >>> 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 > 00010BCF0078 > -rw--- 1 postgres postgres 16777216 Sep 29 14:05 > 00010BCF0079 > -rw--- 1 postgres postgres 16777216 Sep 29 14:05 > 00010BCF007A > -rw--- 1 postgres postgres 16777216 Sep 29 14:05 > 00010BCF007B > -rw--- 1 postgres postgres 16777216 Sep 29 14:05 > 00010BCF007C > -rw--- 1 postgres postgres 16777216 Sep 29 14:05 > 00010BCF007D > -rw--- 1 postgres postgres 16777216 Sep 29 11:22 > 00010BCF007E //after this, there are about 4GB WALs for reuse > -rw--- 1 postgres postgres 16777216 Sep 29 11:08 > 00010BCF007F > -rw--- 1 postgres postgres 16777216 Sep 29 11:06 > 00010BCF0080 > -rw--- 1 postgres postgres 16777216 Sep 29 12:05 > 00010BCF0081 > -rw--- 1 postgres postgres 16777216 Sep 29 11:28 > 00010BCF0082 > -rw--- 1 postgres postgres 16777216 Sep 29 11:06 > 00010BCF0083 > ... > 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. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On 2017-09-29 05:31:51, "Alexander Korotkov" wrote: On Thu, Sep 28, 2017 at 10:52 PM, chenhj wrote: On 2017-09-29 00:43:18,"Alexander Korotkov" wrote: On Thu, Sep 28, 2017 at 6:44 PM, chenhj wrote: On 2017-09-28 01:29:29,"Alexander Korotkov" 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 00010BCF0078 -rw--- 1 postgres postgres 16777216 Sep 29 14:05 00010BCF0079 -rw--- 1 postgres postgres 16777216 Sep 29 14:05 00010BCF007A -rw--- 1 postgres postgres 16777216 Sep 29 14:05 00010BCF007B -rw--- 1 postgres postgres 16777216 Sep 29 14:05 00010BCF007C -rw--- 1 postgres postgres 16777216 Sep 29 14:05 00010BCF007D -rw--- 1 postgres postgres 16777216 Sep 29 11:22 00010BCF007E //after this, there are about 4GB WALs for reuse -rw--- 1 postgres postgres 16777216 Sep 29 11:08 00010BCF007F -rw--- 1 postgres postgres 16777216 Sep 29 11:06 00010BCF0080 -rw--- 1 postgres postgres 16777216 Sep 29 12:05 00010BCF0081 -rw--- 1 postgres postgres 16777216 Sep 29 11:28 00010BCF0082 -rw--- 1 postgres postgres 16777216 Sep 29 11:06 00010BCF0083 ... - Best Regards, Chen Huajun
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On Thu, Sep 28, 2017 at 10:52 PM, chenhj wrote: > On 2017-09-29 00:43:18,"Alexander Korotkov" > wrote: > > On Thu, Sep 28, 2017 at 6:44 PM, chenhj wrote: > >> On 2017-09-28 01:29:29,"Alexander Korotkov" >> 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. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On 2017-09-29 00:43:18,"Alexander Korotkov" wrote: On Thu, Sep 28, 2017 at 6:44 PM, chenhj wrote: On 2017-09-28 01:29:29,"Alexander Korotkov" 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. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company Thanks for advice! I had modified it. - Best Regards, Chen Huajun pg_rewind_wal_copy_reduce_v5.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On Thu, Sep 28, 2017 at 6:44 PM, chenhj wrote: > On 2017-09-28 01:29:29,"Alexander Korotkov" > 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. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On 2017-09-28 01:29:29,"Alexander Korotkov" wrote: It appears that your patch conflicts with fc49e24f. Please, rebase it. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company Yes, i had rebased it, Please check the new patch. -- Best Regards, Chen Huajun pg_rewind_wal_copy_reduce_v4.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On Mon, Sep 25, 2017 at 6:26 PM, chenhj wrote: > On 2017-09-23 01:59:0, "Alexander Korotkov" > wrote: > > On Fri, Sep 22, 2017 at 7:16 PM, chenhj 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! > Great. I tried this patch. It applies cleanly, but doesn't compile. pg_rewind.c:310:36: error: too few arguments provided to function-like > macro invocation > XLByteToSeg(divergerec, startsegno); > ^ > ../../../src/include/access/xlog_internal.h:118:9: note: macro > 'XLByteToSeg' defined here > #define XLByteToSeg(xlrp, logSegNo, wal_segsz_bytes) \ > ^ > pg_rewind.c:310:2: error: use of undeclared identifier 'XLByteToSeg' > XLByteToSeg(divergerec, startsegno); > ^ > pg_rewind.c:311:89: error: too few arguments provided to function-like > macro invocation > XLogFileName(divergence_wal_filename, > targetHistory[lastcommontliIndex].tli, startsegno); > > ^ > ../../../src/include/access/xlog_internal.h:155:9: note: macro > 'XLogFileName' defined here > #define XLogFileName(fname, tli, logSegNo, wal_segsz_bytes) \ > ^ > pg_rewind.c:311:2: error: use of undeclared identifier 'XLogFileName' > XLogFileName(divergence_wal_filename, > targetHistory[lastcommontliIndex].tli, startsegno); > ^ > pg_rewind.c:312:34: error: too few arguments provided to function-like > macro invocation > XLByteToPrevSeg(endrec, endsegno); > ^ > ../../../src/include/access/xlog_internal.h:121:9: note: macro > 'XLByteToPrevSeg' defined here > #define XLByteToPrevSeg(xlrp, logSegNo, wal_segsz_bytes) \ > ^ > pg_rewind.c:312:2: error: use of undeclared identifier 'XLByteToPrevSeg' > XLByteToPrevSeg(endrec, endsegno); > ^ > pg_rewind.c:313:57: error: too few arguments provided to function-like > macro invocation > XLogFileName(last_source_wal_filename, endtli, endsegno); >^ > ../../../src/include/access/xlog_internal.h:155:9: note: macro > 'XLogFileName' defined here > #define XLogFileName(fname, tli, logSegNo, wal_segsz_bytes) \ > ^ > pg_rewind.c:313:2: error: use of undeclared identifier 'XLogFileName' > XLogFileName(last_source_wal_filename, endtli, endsegno); > ^ > 8 errors generated. It appears that your patch conflicts with fc49e24f. Please, rebase it. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On 2017-09-23 01:59:0, "Alexander Korotkov" wrote: On Fri, Sep 22, 2017 at 7:16 PM, chenhj 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! --- Regards, Chen Huajun pg_rewind_wal_copy_reduce_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On Fri, Sep 22, 2017 at 7:16 PM, chenhj 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. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
Hi This is the new pacth with TAP test and use Macro XLOGDIR. And i had add this patch to the commitfest, https://commitfest.postgresql.org/15/1302/ -- Best Regards, Chen Huajun pg_rewind_wal_copy_reduce_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
At 2017-09-17 08:33:33, "Michael Paquier" wrote: >On Sun, Sep 17, 2017 at 3:19 AM, Alexander Korotkov > wrote: >> Hi! >> >> On Sat, Sep 16, 2017 at 5:56 PM, chenhj wrote: >>> >>> 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. >> >> >> Looks like cool optimization for me. Please, add this patch to the next >> commitfest. > >Agreed. > >> Do you think this patch should modify pg_rewind tap tests too? It would be >> nice to make WAL files fetching more covered by tap tests. In particular, >> new tests may generate more WAL files and make sure that pg_rewind fetches >> only required files among them. > >This looks mandatory to me. Using pg_switch_wal() and a minimum amount >of WAL generated you could just make the set of WAL segments skipped >minimal data. > >I have not checked in details, but I think that the positions where >you are applying the filters are using the right approach. > >! !(strncmp(path, "pg_wal", 6) == 0 && IsXLogFileName(path + 7) && >Please use XLOGDIR here. >-- >Michael > Thanks, I will use XLOGDIR and add TAP tests later. -- Chen Huajun
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
On Sun, Sep 17, 2017 at 3:19 AM, Alexander Korotkov wrote: > Hi! > > On Sat, Sep 16, 2017 at 5:56 PM, chenhj wrote: >> >> 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. > > > Looks like cool optimization for me. Please, add this patch to the next > commitfest. Agreed. > Do you think this patch should modify pg_rewind tap tests too? It would be > nice to make WAL files fetching more covered by tap tests. In particular, > new tests may generate more WAL files and make sure that pg_rewind fetches > only required files among them. This looks mandatory to me. Using pg_switch_wal() and a minimum amount of WAL generated you could just make the set of WAL segments skipped minimal data. I have not checked in details, but I think that the positions where you are applying the filters are using the right approach. ! !(strncmp(path, "pg_wal", 6) == 0 && IsXLogFileName(path + 7) && Please use XLOGDIR here. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH]make pg_rewind to not copy useless WAL files
Hi! On Sat, Sep 16, 2017 at 5:56 PM, chenhj wrote: > 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. > Looks like cool optimization for me. Please, add this patch to the next commitfest. Do you think this patch should modify pg_rewind tap tests too? It would be nice to make WAL files fetching more covered by tap tests. In particular, new tests may generate more WAL files and make sure that pg_rewind fetches only required files among them. -- Alexander Korotkov Postgres Professional: http://www.postgrespro.com The Russian Postgres Company