Re: Two pg_rewind patches (auto generate recovery conf and ensure clean shutdown)

On 03.10.2019 6:07, Michael Paquier wrote:
> On Wed, Oct 02, 2019 at 08:28:09PM +0300, Alexey Kondratov wrote:
>> I've directly followed your guess and tried to elaborate pg_rewind test
>> cases and... It seems I've caught a few bugs:
>>
>> 1) --dry-run actually wasn't completely 'dry'. It did update target
>> controlfile, which could cause repetitive pg_rewind calls to fail after
>> dry-run ones.
> I have just paid attention to this thread, but this is a bug which
> goes down to 12 actually so let's treat it independently of the rest.
> The control file was not written thanks to the safeguards in
> write_target_range() in past versions, but the recent refactoring
> around control file handling broke that promise. Another thing which
> is not completely exact is the progress reporting which should be
> reported even if the dry-run mode runs. That's less critical, but
> let's make things consistent.

I also thought about v12, though didn't check whether it's affected.

> Patch 0001 also forgot that recovery.conf should not be written either
> when no rewind is needed.

Yes, definitely, I forgot this code path, thanks.

> I have reworked your first patch as per the attached. What do you
> think about it? The part with the control file needs to go down to
> v12, and I would likely split that into two commits on HEAD: one for
> the control file and a second for the recovery.conf portion with the
> fix for --no-ensure-shutdown to keep a cleaner history.

It looks fine for me excepting the progress reporting part. It now adds
PG_CONTROL_FILE_SIZE to fetch_done. However, I cannot find that control
file is either included into filemap and fetch_size or counted during
calculate_totals(). Maybe I've missed something, but now it looks like
we report something that wasn't planned for progress reporting, doesn't it?

> + # Check that incompatible options error out.
> + command_fails(
> + [
> + 'pg_rewind', "--debug",
> + "--source-pgdata=$standby_pgdata",
> + "--target-pgdata=$master_pgdata", "-R",
> + "--no-ensure-shutdown"
> + ],
> + 'pg_rewind local with -R');
> Incompatible options had better be checked within a separate perl
> script? We generally do that for the other binaries.

Yes, it makes sense. I've reworked the patch with tests and added a
couple of extra cases.

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company