Re: Add recovery to pg_control and remove backup_label

On 11/10/23 00:37, Michael Paquier wrote:
> On Tue, Nov 07, 2023 at 05:20:27PM +0900, Michael Paquier wrote:
>> On Mon, Nov 06, 2023 at 05:39:02PM -0400, David Steele wrote:
>> I've retested today, and miss the failure.  I'll let you know if I see
>> this again.
> 
> I've done a few more dozen runs, and still nothing.  I am wondering
> what this disturbance was.

OK, hopefully it was just a blip.

>>> If we set these fields where backup_label was renamed, the logic would not
>>> be exactly the same since pg_control won't be updated until the next time
>>> through the loop. Since the fields should be updated before
>>> UpdateControlFile() I thought it made sense to keep all the updates
>>> together.
>>>
>>> Overall I think it is simpler, and we don't need to acquire a lock on
>>> ControlFile.
>>
>> What you are proposing is the same as what we already do for
>> backupEndRequired or backupStartPoint in the control file when
>> initializing recovery, so objection withdrawn.
>>
>>> Thomas included this change in his pg_basebackup changes so I did the same.
>>> Maybe wait a bit before we split this out? Seems like a pretty small
>>> change...
>>
>> Seems like a pretty good argument for refactoring that now, and let
>> any other patches rely on it.  Would you like to send a separate
>> patch?
> 
> The split still looks worth doing seen from here, so I am switching
> the patch as WoA for now.

This has been split out.

>>>> Actually, grouping backupStartPointTLI and minRecoveryPointTLI should
>>>> reduce more the size with some alignment magic, no?
>>>
>>> I thought about this, but it seemed to me that existing fields had been
>>> positioned to make the grouping logical rather than to optimize alignment,
>>> e.g. minRecoveryPointTLI. Ideally that would have been placed near
>>> backupEndRequired (or vice versa). But if the general opinion is to
>>> rearrange for alignment, I'm OK with that.
>>
>> I've not tested, but it looks like moving backupStartPointTLI after
>> backupEndPoint should shave 8 bytes, if you want to maintain a more
>> coherent group for the LSNs.

OK, I have moved backupStartPointTLI.

> +    * backupFromStandby indicates that the backup was taken on a standby. It is
> +    * require to initialize recovery and set to false afterwards.
> s/require/required/.

Fixed.

> The term "backup recovery", that we've never used in the tree until
> now as far as I know.  Perhaps this recovery method should just be
> referred as "recovery from backup"?

Well, "backup recovery" is less awkward, I think. For instance "backup 
recovery field" vs "recovery from backup field".

> By the way, there is another thing that this patch has forgotten: the
> SQL functions that display data from the control file.  Shouldn't
> pg_control_recovery() be extended with the new fields?  These fields
> may be less critical than the other ones related to recovery, but I
> suspect that showing them can become handy at least for debugging and
> monitoring purposes.

I guess that depends on whether we reset them or not (discussion below). 
Right now they would not be visible since by the time the user could log 
on they would be reset.

> Something in this area is that backupRecoveryRequired is the switch
> controlling if the fields set by the recovery initialization.  Could
> it be actually useful to leave the other fields as they are and only
> reset backupRecoveryRequired before the first control file update?
> This would leave a trace of the backup history directly in the control
> file.

Since the other recovery fields are cleared in ReachedEndOfBackup() this 
would be a change from what we do now.

None of these fields are ever visible (with the exception of 
minRecoveryPoint/TLI) since they are reset when the database becomes 
consistent and before logons are allowed. Viewing them with 
pg_controldata makes sense, but I'm not sure pg_control_recovery() does.

In fact, are backup_start_lsn, backup_end_lsn, and 
end_of_backup_record_required ever non-zero when logged onto Postgres? 
Maybe I'm missing something?

> What about pg_resetwal and RewriteControlFile()?  Shouldn't these
> recovery fields be reset as well?

Done.

> git diff --check is complaining a bit.

Fixed.

New patches attached based on eb81e8e790.

Regards,
-David