Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-06-12 Thread Michael Paquier
On Thu, Jun 11, 2020 at 10:16:50AM -0400, Stephen Frost wrote: > Uh, doesn't this pull these functions out of libpgcommon, where they > might have been being used by utilities outside of our client tools? This stuff is new to 13, and we are still in beta so it is fine in my opinion to still

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-06-11 Thread Stephen Frost
Greetings, * Michael Paquier (mich...@paquier.xyz) wrote: > On Wed, Jun 10, 2020 at 01:23:37PM +0900, Michael Paquier wrote: > > Okay. After sleeping on it, it looks like would be better to move > > this new fe_archive.c to src/fe_utils/. I'll try to do that tomorrow, > > and added an open item

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-06-11 Thread Michael Paquier
On Wed, Jun 10, 2020 at 01:23:37PM +0900, Michael Paquier wrote: > Okay. After sleeping on it, it looks like would be better to move > this new fe_archive.c to src/fe_utils/. I'll try to do that tomorrow, > and added an open item so as we don't forget about it. Applied this part now as of

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-06-09 Thread Michael Paquier
On Mon, Jun 08, 2020 at 02:16:32PM +0300, Alexey Kondratov wrote: > Do not like it either. Having the same naming and a guarantee that this code > is always compiled separately looks more convenient for me. > > [1] >

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-06-08 Thread Michael Paquier
On Mon, Jun 08, 2020 at 02:16:32PM +0300, Alexey Kondratov wrote: > BTW, most of 'common' is a really common code with only four exceptions > like logging.c, which is frontend-only. Is it there for historical > reasons only or something else?" > > Personally, I would prefer that everything in the

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-06-08 Thread Alexey Kondratov
On 2020-06-08 09:14, Michael Paquier wrote: On Sun, Jun 07, 2020 at 10:05:03PM +0300, Alexander Korotkov wrote: On Sat, Jun 6, 2020 at 8:49 PM Peter Eisentraut wrote: Why is fe_archive.c in src/common/ and not in src/fe_utils/? It is not "common" to frontend and backend. Yep, this seems

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-06-08 Thread Michael Paquier
On Sun, Jun 07, 2020 at 10:05:03PM +0300, Alexander Korotkov wrote: > On Sat, Jun 6, 2020 at 8:49 PM Peter Eisentraut > wrote: >> Why is fe_archive.c in src/common/ and not in src/fe_utils/? It is not >> "common" to frontend and backend. > > Yep, this seems wrong to me. I can propose following

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-06-07 Thread Alexander Korotkov
Hi! On Sat, Jun 6, 2020 at 8:49 PM Peter Eisentraut wrote: > > On 2020-03-31 08:48, Michael Paquier wrote: > > On Mon, Mar 30, 2020 at 05:00:01PM +0300, Alexey Kondratov wrote: > >> What do think about adding following sanity check into xlogarchive.c? > >> > >> +#ifdef FRONTEND > >> +#error

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-06-06 Thread Peter Eisentraut
On 2020-03-31 08:48, Michael Paquier wrote: On Mon, Mar 30, 2020 at 05:00:01PM +0300, Alexey Kondratov wrote: What do think about adding following sanity check into xlogarchive.c? +#ifdef FRONTEND +#error "This file is not expected to be compiled for frontend code" +#endif It would prevent

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-04-01 Thread Alexey Kondratov
On 2020-04-01 05:19, Michael Paquier wrote: On Tue, Mar 31, 2020 at 03:48:21PM +0900, Michael Paquier wrote: Thanks, committed 0001 after fixing the order of the headers. One patch left. And committed now 0002, meaning that we are officially done. Thanks Alexey for your patience. Thanks

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-31 Thread Michael Paquier
On Tue, Mar 31, 2020 at 03:48:21PM +0900, Michael Paquier wrote: > Thanks, committed 0001 after fixing the order of the headers. One > patch left. And committed now 0002, meaning that we are officially done. Thanks Alexey for your patience. -- Michael signature.asc Description: PGP signature

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-31 Thread Michael Paquier
On Mon, Mar 30, 2020 at 05:00:01PM +0300, Alexey Kondratov wrote: > What do think about adding following sanity check into xlogarchive.c? > > +#ifdef FRONTEND > +#error "This file is not expected to be compiled for frontend code" > +#endif > > It would prevent someone from adding typedefs and

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-30 Thread Alexey Kondratov
On 2020-03-30 04:56, Michael Paquier wrote: On Fri, Mar 27, 2020 at 07:47:29AM +0900, Michael Paquier wrote: On Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote: I don't think any such cleanup should hamper the patch at hand anyway. I don't think either, so I would do the split

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-30 Thread Alexander Korotkov
On Mon, Mar 30, 2020 at 4:57 AM Michael Paquier wrote: > On Fri, Mar 27, 2020 at 07:47:29AM +0900, Michael Paquier wrote: > > On Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote: > >> I don't think any such cleanup should hamper the patch at hand anyway. > > > > I don't think either,

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-29 Thread Michael Paquier
On Fri, Mar 27, 2020 at 07:47:29AM +0900, Michael Paquier wrote: > On Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote: >> I don't think any such cleanup should hamper the patch at hand anyway. > > I don't think either, so I would do the split for the archive routines > at least. It

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-27 Thread Michael Paquier
On Fri, Mar 27, 2020 at 12:24:19AM +0300, Alexey Kondratov wrote: > The block of function declarations for xlogarchive.c inside xlog_internal.h > looks a bit dangling already, since all other functions and variables > defined/initialized in xlog.c. That way, it looks good to me to move it >

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-26 Thread Michael Paquier
On Thu, Mar 26, 2020 at 06:56:36PM -0300, Alvaro Herrera wrote: > Uh, is XLOGDIR the only reason to include xlog_internal.h? Maybe it > would be easier to have a routine in xlog.c that returns the path? > There's a few functions in xlog.c that could use it, as well. Yep, that's all. We could

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-26 Thread Alvaro Herrera
On 2020-Mar-26, Michael Paquier wrote: > I was looking at this patch again today and I am rather fine with the > existing semantics. Still I don't like much to name the frontend-side > routine FrontendRestoreArchivedFile() and use a different name than > the backend counterpart because we have

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-26 Thread Alexey Kondratov
On 2020-03-26 10:34, Michael Paquier wrote: On Tue, Mar 24, 2020 at 12:22:16PM +0900, Michael Paquier wrote: Thanks Alvaro and Alexander. 0001 has been applied as of e09ad07. Now for 0002, let's see about it later. Attached is a rebased version, with no actual changes. I was looking at this

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-26 Thread Michael Paquier
On Tue, Mar 24, 2020 at 12:22:16PM +0900, Michael Paquier wrote: > Thanks Alvaro and Alexander. 0001 has been applied as of e09ad07. > Now for 0002, let's see about it later. Attached is a rebased > version, with no actual changes. I was looking at this patch again today and I am rather fine

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-23 Thread Michael Paquier
On Mon, Mar 23, 2020 at 07:17:13PM +0300, Alexander Korotkov wrote: > On Mon, Mar 23, 2020 at 6:16 PM Alvaro Herrera > wrote: >> Hi, I didn't realize that this was waiting on me. It looks good to me >> -- I'd just remove the comment on the function prototype in archiver.h, >> which is not

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-23 Thread Alexander Korotkov
On Mon, Mar 23, 2020 at 6:16 PM Alvaro Herrera wrote: > On 2020-Mar-23, Alexander Korotkov wrote: > > On Fri, Mar 13, 2020 at 3:18 AM Michael Paquier wrote: > > > No issues with that either. Are you fine with the updated version > > > attached for 0001? > > > > I think patchset is almost ready

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-23 Thread Alvaro Herrera
On 2020-Mar-23, Alexander Korotkov wrote: > Dear Alvaro, > > On Fri, Mar 13, 2020 at 3:18 AM Michael Paquier wrote: > > No issues with that either. Are you fine with the updated version > > attached for 0001? > > I think patchset is almost ready for commit. Michael is waiting for > your

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-23 Thread Alexander Korotkov
Dear Alvaro, On Fri, Mar 13, 2020 at 3:18 AM Michael Paquier wrote: > No issues with that either. Are you fine with the updated version > attached for 0001? I think patchset is almost ready for commit. Michael is waiting for your answer on whether you're fine with current shape of 0001.

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-12 Thread Michael Paquier
On Thu, Mar 12, 2020 at 12:50:17PM -0300, Alvaro Herrera wrote: > Thanks, looks good. I don't think we *need* the MAXPGPATH restriction > really -- I was thinking in a StringInfo kind of approach where you just > append the stuff you need without having to think about the buffer > length. Oh,

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-12 Thread Alvaro Herrera
On 2020-Mar-11, Michael Paquier wrote: > On Tue, Mar 10, 2020 at 12:39:53PM -0300, Alvaro Herrera wrote: > > Another option is to return the command as a palloc'ed string (per > > psprintf), instead of using a caller-stack-allocated variable. Passing > > the buffer len is widely used, but more

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-12 Thread Alexey Kondratov
On 12.03.2020 07:39, Michael Paquier wrote: I'd like to commit the refactoring piece in 0001 tomorrow, then let's move on with the rest as of 0002. If more comments and docs are needed for archive.c, let's continue discussing that. I just went through the both patches and realized that I

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-11 Thread Michael Paquier
On Wed, Mar 11, 2020 at 12:27:03PM +0900, Michael Paquier wrote: >> Also, I think the function comment could stand some more detailing. > > What kind of additional information would you like to add on top of > what the updated version attached does? I have been working more on that, and attached

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-10 Thread Michael Paquier
On Tue, Mar 10, 2020 at 12:39:53PM -0300, Alvaro Herrera wrote: > Another option is to return the command as a palloc'ed string (per > psprintf), instead of using a caller-stack-allocated variable. Passing > the buffer len is widely used, but more error prone (and I think getting > this one wrong

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-10 Thread Alvaro Herrera
On 2020-Mar-10, Michael Paquier wrote: > On Tue, Mar 10, 2020 at 01:05:40PM +0300, Alexander Korotkov wrote: > > Two options seem reasonable to me in this case. The first is to pass > > length as additional argument as you did. The second option is to > > make argument a pointer to fixed-size

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-10 Thread Alexander Korotkov
On Tue, Mar 10, 2020 at 3:44 PM Michael Paquier wrote: > On Tue, Mar 10, 2020 at 01:05:40PM +0300, Alexander Korotkov wrote: > > Two options seem reasonable to me in this case. The first is to pass > > length as additional argument as you did. The second option is to > > make argument a pointer

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-10 Thread Michael Paquier
On Tue, Mar 10, 2020 at 01:05:40PM +0300, Alexander Korotkov wrote: > Two options seem reasonable to me in this case. The first is to pass > length as additional argument as you did. The second option is to > make argument a pointer to fixed-size array as following. > > extern bool

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-10 Thread Alexander Korotkov
On Tue, Mar 10, 2020 at 7:28 AM Michael Paquier wrote: > On Mon, Mar 09, 2020 at 03:38:29PM +0530, Kuntal Ghosh wrote: > > That's a good suggestion. But, it's unlikely that a caller would pass > > something longer than MAXPGPATH and we indeed use that value a lot in > > the code. IMHO, it looks

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-09 Thread Michael Paquier
On Mon, Mar 09, 2020 at 03:38:29PM +0530, Kuntal Ghosh wrote: > That's a good suggestion. But, it's unlikely that a caller would pass > something longer than MAXPGPATH and we indeed use that value a lot in > the code. IMHO, it looks okay to me to have that assumption here as > well. Well, a more

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-09 Thread Kuntal Ghosh
On Mon, Mar 9, 2020 at 1:46 PM Michael Paquier wrote: > > On Fri, Mar 06, 2020 at 05:22:16PM +0900, Michael Paquier wrote: > > Thanks for the suggestion. Avoiding dead code makes sense as well > > here. I'll think about this stuff a bit more first. > > Okay, after pondering more on that, here

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-09 Thread Michael Paquier
On Fri, Mar 06, 2020 at 05:22:16PM +0900, Michael Paquier wrote: > Thanks for the suggestion. Avoiding dead code makes sense as well > here. I'll think about this stuff a bit more first. Okay, after pondering more on that, here is a first cut with a patch refactoring restore_command build to

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-06 Thread Michael Paquier
On Thu, Mar 05, 2020 at 11:09:06PM -0300, Alvaro Herrera wrote: > Hmm, doesn't the CF bot already validate the MSVC build? > > Splitting in two seems all right, but I think one commit that introduces > dead code is not great. It may make more sense to have one commit for > common/archive.c, and

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-05 Thread Alvaro Herrera
On 2020-Mar-06, Michael Paquier wrote: > I was also thinking to split the patch into two pieces: > - Introduction of common/archive.c and common/fe_archive.c (the former > is used by xlogarchive.c and the latter only by pg_rewind). The > latter is dead code without the second patch, but this

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-05 Thread Michael Paquier
On Thu, Mar 05, 2020 at 07:52:24PM +0300, Alexey Kondratov wrote: > OK, I was still having in mind pg_rewind as the only one user of this > routine. Now it is a part of the common and I could imagine a hypothetical > tool that is polling the archive and waiting for a specific WAL segment to >

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-05 Thread Alexey Kondratov
On 05.03.2020 09:24, Michael Paquier wrote: On Wed, Mar 04, 2020 at 08:14:20PM +0300, Alexey Kondratov wrote: - I did not actually get why you don't check for a missing command when using wait_result_is_any_signal. In this case I'd think that it is better to exit immediately as follow-up calls

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-04 Thread Michael Paquier
On Wed, Mar 04, 2020 at 08:14:20PM +0300, Alexey Kondratov wrote: > On 04.03.2020 10:45, Michael Paquier wrote: > - *        Functions for finding and validating executable files > + *        Functions for finding and validating from executables files > > There is probably something missing here.

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-04 Thread Alexey Kondratov
On 04.03.2020 10:45, Michael Paquier wrote: On Mon, Mar 02, 2020 at 08:59:49PM +0300, Alexey Kondratov wrote: All other remarks look clear for me, so I fix them in the next patch version, thanks. Already done as per the attached, with a new routine named getRestoreCommand() and more done.

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-03 Thread Michael Paquier
On Mon, Mar 02, 2020 at 08:59:49PM +0300, Alexey Kondratov wrote: > OK, sounds reasonable, but just to be clear. I will remove only a > possibility to bypass this sanity check (with 0), but leave expectedSize > argument intact. We still need it, since pg_rewind takes WalSegSz from > ControlFile

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-02 Thread Alexey Kondratov
On 2020-03-02 07:53, Michael Paquier wrote: + * For fixed-size files, the caller may pass the expected size as an + * additional crosscheck on successful recovery. If the file size is not + * known, set expectedSize = 0. + */ +int +RestoreArchivedWALFile(const char *path, const char

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-03-01 Thread Michael Paquier
On Fri, Feb 28, 2020 at 03:37:47PM +0300, Alexey Kondratov wrote: > I would prefer to keep it, since there are plenty of similar comments near > Asserts and elogs all over the Postgres. Otherwise it may look like a valid > error state. It may be obvious now, but for someone who is not aware of >

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-02-28 Thread Alexey Kondratov
On 2020-02-28 09:43, Michael Paquier wrote: On Thu, Feb 27, 2020 at 06:29:34PM +0300, Alexey Kondratov wrote: On 2020-02-27 16:41, Alexey Kondratov wrote: > > New version of the patch is attached. Thanks again for your review. > Last patch (v18) got a conflict with one of today commits

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-02-27 Thread Michael Paquier
On Thu, Feb 27, 2020 at 06:29:34PM +0300, Alexey Kondratov wrote: > On 2020-02-27 16:41, Alexey Kondratov wrote: > > > > New version of the patch is attached. Thanks again for your review. > > > > Last patch (v18) got a conflict with one of today commits (05d8449e73). > Rebased version is

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-02-27 Thread Alexey Kondratov
On 2020-02-27 16:41, Alexey Kondratov wrote: New version of the patch is attached. Thanks again for your review. Last patch (v18) got a conflict with one of today commits (05d8449e73). Rebased version is attached. -- Alexey Kondratov Postgres Professional https://www.postgrespro.com The

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-02-27 Thread Alexey Kondratov
On 2020-02-27 04:52, Michael Paquier wrote: On Thu, Feb 27, 2020 at 12:43:55AM +0300, Alexander Korotkov wrote: Regarding text split change, it was made by pgindent. I didn't notice it belongs to unchanged part of code. Sure, we shouldn't include this into the patch. I have read through v17

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-02-26 Thread Michael Paquier
On Thu, Feb 27, 2020 at 12:43:55AM +0300, Alexander Korotkov wrote: > Regarding text split change, it was made by pgindent. I didn't notice > it belongs to unchanged part of code. Sure, we shouldn't include this > into the patch. I have read through v17 (not tested, sorry), and spotted a couple

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-02-26 Thread Alexander Korotkov
On Wed, Feb 26, 2020 at 11:45 PM Alexey Kondratov wrote: > On 2020-02-26 22:03, Alexander Korotkov wrote: > > On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov > > wrote: > >> > >> I think usage of chmod() deserves comment. As I get default > >> permissions are sufficient for work, but we need

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-02-26 Thread Alexey Kondratov
On 2020-02-26 22:03, Alexander Korotkov wrote: On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov wrote: I think usage of chmod() deserves comment. As I get default permissions are sufficient for work, but we need to set them to satisfy 'check PGDATA permissions' test. I've added this

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-02-26 Thread Alexander Korotkov
On Tue, Feb 25, 2020 at 1:48 PM Alexander Korotkov wrote: > > I think usage of chmod() deserves comment. As I get default > permissions are sufficient for work, but we need to set them to > satisfy 'check PGDATA permissions' test. I've added this comment myself. I've also fixes some

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-02-25 Thread Alexander Korotkov
On Sat, Feb 1, 2020 at 2:16 AM Alexey Kondratov wrote: > New version is attached. Do you have any other comments or objections? Now patch looks much better. Thanks to Michael Paquier for review. I've just revised commit message reflecting we've removed one of the new options. But I have

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-01-31 Thread Alexey Kondratov
On 2020-01-24 08:50, Michael Paquier wrote: On Wed, Jan 22, 2020 at 12:55:30AM +0300, Alexander Korotkov wrote: On Sun, Jan 19, 2020 at 1:24 PM Michael Paquier wrote: +static int +RestoreArchivedWALFile(const char *path, const char *xlogfname, + off_t expectedSize, const

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-01-23 Thread Michael Paquier
On Wed, Jan 22, 2020 at 12:55:30AM +0300, Alexander Korotkov wrote: > Thank you for your review! > The revised patch is attached. Thanks for the new version. > On Sun, Jan 19, 2020 at 1:24 PM Michael Paquier wrote: >> +static int >> +RestoreArchivedWALFile(const char *path, const char

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-01-21 Thread Alexander Korotkov
Hi, Michael! Thank you for your review! The revised patch is attached. On Sun, Jan 19, 2020 at 1:24 PM Michael Paquier wrote: > On Sat, Jan 18, 2020 at 06:21:22PM +0300, Alexander Korotkov wrote: > > I made some minor cleanup. In particular, I've to fix usage of terms > > "WAL" and "WALs".

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-01-19 Thread Tom Lane
Alvaro Herrera writes: > On 2020-Jan-19, Michael Paquier wrote: >> +use File::Glob ':bsd_glob'; >> +use File::Path qw(remove_tree make_path); >> +use File::Spec::Functions qw(catdir catfile); >> Is this compatible with our minimum perl requirements for the TAP >> tests? > I *think* :bsd_glob

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-01-19 Thread Alvaro Herrera
On 2020-Jan-19, Michael Paquier wrote: > So using WAL to tell about a WAL segment file is wrong, WALs is not a > term that actually exists. I agree. > So, in my opinion, it is fine to use "WAL file", "WAL segment" or even > "WAL segment file". Agreed with these three terms -- "WAL file" seems

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-01-19 Thread Michael Paquier
On Sat, Jan 18, 2020 at 06:21:22PM +0300, Alexander Korotkov wrote: > I made some minor cleanup. In particular, I've to fix usage of terms > "WAL" and "WALs". This patch sometimes use term "WAL" to specify > single WAL file and term "WALs" to specify multiple WAL files. But > WAL stands for

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2020-01-18 Thread Alexander Korotkov
Hi! On Tue, Dec 3, 2019 at 12:41 PM Alexey Kondratov wrote: > On 01.12.2019 5:57, Michael Paquier wrote: > > On Thu, Sep 26, 2019 at 03:08:22PM +0300, Alexey Kondratov wrote: > >> As Alvaro correctly pointed in the nearby thread [1], we've got an > >> interference regarding -R command line

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-12-03 Thread Alexey Kondratov
On 01.12.2019 5:57, Michael Paquier wrote: On Thu, Sep 26, 2019 at 03:08:22PM +0300, Alexey Kondratov wrote: As Alvaro correctly pointed in the nearby thread [1], we've got an interference regarding -R command line argument. I agree that it's a good idea to reserve -R for recovery configuration

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-11-30 Thread Michael Paquier
On Thu, Sep 26, 2019 at 03:08:22PM +0300, Alexey Kondratov wrote: > As Alvaro correctly pointed in the nearby thread [1], we've got an > interference regarding -R command line argument. I agree that it's a good > idea to reserve -R for recovery configuration write to be consistent with >

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-09-26 Thread Alexey Kondratov
On 01.08.2019 19:53, Alexey Kondratov wrote: On 26.07.2019 20:43, Liudmila Mantrova wrote: On a more general note, I wonder if everyone is happy with the --using-postgresql-conf option name, or we should continue searching for a narrower term. Unfortunately, I don't have any better

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-08-01 Thread Alexey Kondratov
On 26.07.2019 20:43, Liudmila Mantrova wrote: I would like to suggest a couple of changes to docs and comments, please see the attachment. The "...or fetched on startup" part also seems wrong here, but it's not a part of your patch, so I'm going to ask about it on psql-docs separately.

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-07-26 Thread Liudmila Mantrova
On 7/1/19 5:20 PM, Alexey Kondratov wrote: Hi Thomas, On 01.07.2019 15:02, Thomas Munro wrote: Hi Alexey, This no longer applies.  Since the Commitfest is starting now, could you please rebase it? Thank you for a reminder. Rebased version of the patch is attached. I've also modified my

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-07-01 Thread Alexey Kondratov
Hi Thomas, On 01.07.2019 15:02, Thomas Munro wrote: Hi Alexey, This no longer applies. Since the Commitfest is starting now, could you please rebase it? Thank you for a reminder. Rebased version of the patch is attached. I've also modified my logging code in order to obey new unified

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-07-01 Thread Thomas Munro
On Thu, Mar 28, 2019 at 6:49 AM Alexey Kondratov wrote: > Updated version of patch is attached. Hi Alexey, This no longer applies. Since the Commitfest is starting now, could you please rebase it? Thanks, -- Thomas Munro https://enterprisedb.com

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-03-27 Thread Alexey Kondratov
On 26.03.2019 11:19, Michael Paquier wrote: + * This is a simplified and adapted to frontend version of + * RestoreArchivedFile function from transam/xlogarchive.c + */ +static int +RestoreArchivedWAL(const char *path, const char *xlogfname, I don't think that we should have duplicates for that,

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-03-26 Thread Michael Paquier
On Wed, Mar 20, 2019 at 01:55:51PM +0800, Andrey Borodin wrote: > I'm a bit confused by by console output routines. E.g. in > pg_rewind's main() you call pg_fatal()s, and printf(), and pg_log() > with various levels. Shouldn't we use all the pg_* functions? pg_fatal() would exit immediately, and

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-03-19 Thread Andrey Borodin
Hi! > 7 марта 2019 г., в 20:27, Alexey Kondratov > написал(а): > > I'm a bit confused by by console output routines. E.g. in pg_rewind's main() you call pg_fatal()s, and printf(), and pg_log() with various levels. Shouldn't we use all the pg_* functions? But most of this printing usages

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-03-07 Thread Alexey Kondratov
On 07.03.2019 10:26, David Steele wrote: On 3/6/19 5:38 PM, Andrey Borodin wrote: The new patch is much smaller (less than 400 lines) and works as advertised. There's a typo "retreive" there. Ough, corrected this in three different places. Not my word, definitely. Thanks! These lines

Re: Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-03-06 Thread David Steele
On 3/6/19 5:38 PM, Andrey Borodin wrote: 20 февр. 2019 г., в 17:06, Alexey Kondratov написал(а): The new patch is much smaller (less than 400 lines) and works as advertised. There's a typo "retreive" there. These lines look a little suspicious: char

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-03-06 Thread Andrey Borodin
Hi! > 20 февр. 2019 г., в 17:06, Alexey Kondratov > написал(а): > >> >> I will work out this one with postgres -C and come back till the next >> commitfest. I found that something similar is already used in pg_ctl and >> there is a mechanism for finding valid executables in exec.c. So it

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-02-20 Thread Alexey Kondratov
Hi, I will work out this one with postgres -C and come back till the next commitfest. I found that something similar is already used in pg_ctl and there is a mechanism for finding valid executables in exec.c. So it does not seem to be a big deal at the first sight. I have reworked the

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-02-18 Thread Alexey Kondratov
On 18.02.2019 19:49, Alvaro Herrera wrote: On 16.02.2019 6:41, Andres Freund wrote: It sounds like a seriously bad idea to use a different parser for pg_rewind. Why don't you just use postgres for it? As in /path/to/postgres -D /path/to/datadir/ -C shared_buffers ? Eh, this is what I

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-02-18 Thread Alvaro Herrera
On 2019-Feb-18, Andres Freund wrote: > Hi, > > On 2019-02-18 16:26:55 +0300, Alexey Kondratov wrote: > > Hi Andres, > > > > Thank you for your feedback. > > > > On 16.02.2019 6:41, Andres Freund wrote: > > > It sounds like a seriously bad idea to use a different parser for > > > pg_rewind.

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-02-18 Thread Andres Freund
Hi, On 2019-02-18 16:26:55 +0300, Alexey Kondratov wrote: > Hi Andres, > > Thank you for your feedback. > > On 16.02.2019 6:41, Andres Freund wrote: > > It sounds like a seriously bad idea to use a different parser for > > pg_rewind. Why don't you just use postgres for it? As in > >

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-02-18 Thread Alexey Kondratov
Hi Andres, Thank you for your feedback. On 16.02.2019 6:41, Andres Freund wrote: It sounds like a seriously bad idea to use a different parser for pg_rewind. Why don't you just use postgres for it? As in /path/to/postgres -D /path/to/datadir/ -C shared_buffers ? Initially, when I started

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-02-15 Thread Andres Freund
Hi, On 2019-02-08 18:30:18 +0300, Alexey Kondratov wrote: > From 99c6d94f37a797400d41545a271ff111b92e9361 Mon Sep 17 00:00:00 2001 > From: Alexey Kondratov > Date: Fri, 21 Dec 2018 14:00:30 +0300 > Subject: [PATCH] pg_rewind: options to use restore_command from > postgresql.conf or command

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-02-15 Thread Andres Freund
Hi, On 2018-11-07 12:58:11 +0300, Alexey Kondratov wrote: > On 30.10.2018 06:01, Michael Paquier wrote: > > - Reusing the GUC parser is something I would avoid as well. Not worth > > the complexity. > > Yes, I don't like it either. I will try to make guc-file.l frontend safe. It sounds like a

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-02-11 Thread Alexey Kondratov
Hi! On 09.02.2019 14:31, Andrey Borodin wrote: Here's a typo in postgreslq.conf + fprintf(stderr, _("%s: option -r/--use-postgresql-conf is specified, but postgreslq.conf is absent in the target directory\n"), Fixed, thanks. I do not attach new version of the patch for

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-02-09 Thread Andrey Borodin
Hi! Thanks for the next version! > 8 февр. 2019 г., в 18:30, Alexey Kondratov > написал(а): > > On 21.01.2019 23:50, a.kondra...@postgrespro.ru wrote: >> Thank you for the review! I have updated the patch according to your >> comments and remarks. Please, find new version attached. > > During

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-02-08 Thread Alexey Kondratov
On 21.01.2019 23:50, a.kondra...@postgrespro.ru wrote: Thank you for the review! I have updated the patch according to your comments and remarks. Please, find new version attached. During the self-reviewing of the code and tests, I discovered some problems with build on Windows. New version

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-01-21 Thread a . kondratov
Hi Andrey, Thank you for the review! I have updated the patch according to your comments and remarks. Please, find new version attached. On 2019-01-07 12:12, Andrey Borodin wrote: Here are some my notes: 1. RestoreArchivedWAL() shares a lot of code with RestoreArchivedFile(). Is it

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2019-01-07 Thread Andrey Borodin
Hi! Thanks for working on this feature, I believe it solves actual problem of HA systems. > 30 окт. 2018 г., в 8:01, Michael Paquier написал(а): > > Another thing I am wondering is: do we actually need something complex? > What we want to know is what data is necessary to build the file map,

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-12-26 Thread Alexey Kondratov
Greetings, - Reusing the GUC parser is something I would avoid as well.  Not worth the complexity. Yes, I don't like it either. I will try to make guc-file.l frontend safe. Any success with that? I looked into it and found that currently guc-file.c is built as part of guc.c, so it seems

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-12-24 Thread Alexey Kondratov
Hi Dmitry, On 30.11.2018 19:04, Dmitry Dolgov wrote: Just to confirm, patch still can be applied without conflicts, and pass all the tests. Also I like the original motivation for the feature, sounds pretty useful. For now I'm moving it to the next CF. Thanks, although I have slightly updated

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-11-30 Thread Dmitry Dolgov
> On Wed, Nov 7, 2018 at 10:58 AM Alexey Kondratov > wrote: > > On 30.10.2018 06:01, Michael Paquier wrote: > > > On Mon, Oct 29, 2018 at 12:09:21PM +0300, Alexey Kondratov wrote: > >> Currently in the patch, with dry-run option (-n) pg_rewind only fetches > >> missing WALs to be able to build

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-11-07 Thread Alexey Kondratov
On 30.10.2018 06:01, Michael Paquier wrote: On Mon, Oct 29, 2018 at 12:09:21PM +0300, Alexey Kondratov wrote: Currently in the patch, with dry-run option (-n) pg_rewind only fetches missing WALs to be able to build file map, while doesn't touch any data files. So I guess it behaves exactly as

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-10-29 Thread Michael Paquier
On Mon, Oct 29, 2018 at 12:09:21PM +0300, Alexey Kondratov wrote: > Currently in the patch, with dry-run option (-n) pg_rewind only fetches > missing WALs to be able to build file map, while doesn't touch any data > files. So I guess it behaves exactly as you described and we do not need a >

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-10-29 Thread Alexey Kondratov
Something that we could think about is directly to provide a command to pg_rewind via command line. In my patch I added this option too. One can pass restore_command via -R option, e.g.: pg_rewind -P --target-pgdata=/path/to/master/pg_data --source-pgdata=/path/to/standby/pg_data -R 'cp

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-10-29 Thread Alexey Kondratov
Hi Andrey, Will you add this patch to CF? I'm going to review it. Best regards, Andrey Borodin Here it is https://commitfest.postgresql.org/20/1849/ -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-10-29 Thread Michael Paquier
On Mon, Oct 22, 2018 at 02:19:07PM -0300, Alvaro Herrera wrote: > Hmm, I remember we had a project to have a new postmaster option that > would report the value of some GUC option, so instead of parsing the > file in the frontend, you'd invoke the backend to do the parsing. But I > don't know

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-10-29 Thread Andrey Borodin
Hi, Alexey! > 25 окт. 2018 г., в 17:37, Alexey Kondratov > написал(а): Will you add this patch to CF? I'm going to review it. Best regards, Andrey Borodin

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-10-25 Thread Alexey Kondratov
On 22.10.2018 20:19, Alvaro Herrera wrote: I didn't actually try patch yet, but the idea seems interesting. Will you add it to the commitfest? I am willing to add it to the November commitfest, but I have some concerns regarding frontend version of GUC parser. Probably, it is possible to

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-10-22 Thread Alvaro Herrera
On 2018-Oct-22, Alexey Kondratov wrote: > > I didn't actually try patch yet, but the idea seems interesting. Will > > you add it to the commitfest? > I am willing to add it to the November commitfest, but I have some concerns > regarding frontend version of GUC parser. Probably, it is possible to

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-10-22 Thread Alexey Kondratov
Hi Andrey, Thank you for your reply. I think it is better to load restore_command from recovery.conf. Yes, it seems to be the most native way. That's why I needed this rewritten (mostly copy-pasted) frontend-safe version of parser (guc-file.l). I didn't actually try patch yet, but the idea

Re: [Patch] pg_rewind: options to use restore_command from recovery.conf or command line

2018-10-22 Thread Andrey Borodin
Hi, Alexey! > 19 окт. 2018 г., в 22:49, Alexey Kondratov > написал(а): > I expect, that it will be a good idea to allow pg_rewind to look for a > restore_command > +1 Normally you do not expect huge progress on failed master. But you still can get a lot of WAL if you have network partition