Re: Return pg_control from pg_backup_stop().

On 10/23/26 18:17, Heikki Linnakangas wrote:
> On 23/10/2026 15:42, David Steele wrote:
>> Patch 02 implements the flag to enforce the presence of backup_label
>> and credits Andres with the original idea. This is the bulk of the
>> prior patch.
> Makes sense at high level.
>
>> --- a/doc/src/sgml/backup.sgml
>> +++ b/doc/src/sgml/backup.sgml
>> @@ -1031,6 +1031,19 @@ SELECT * FROM pg_backup_stop(wait_for_archive
>> => true);
>>       vital to the backup working and must be written byte for byte
>> without
>>       modification, which may require opening the file in binary mode.
>>      </para>
>> +    <para>
>> +     Note that a backup made with the low-level API copies
>> +     <filename>pg_control</filename> from the running cluster, so it
>> does not
>> +     contain the flag that <xref linkend="app-pgbasebackup"/> sets to
>> make
>> +     <filename>backup_label</filename> mandatory for recovery. Removing
>> +     <filename>backup_label</filename> from such a backup will not
>> prevent the
>> +     cluster from starting; it will instead recover from the checkpoint
>> +     recorded in <filename>pg_control</filename>. Unless that happens
>> to be the
>> +     checkpoint the backup started from, recovery will begin too late
>> and
>> +     silently produce an inconsistent cluster.
>> +     Backup software using this API should set the flag itself if it
>> is able to
>> +     modify <filename>pg_control</filename>.
>> +    </para>
>>     </listitem>
>>     <listitem>
>>      <para>
>
> That's good advice, but the text feels awkward. Which flag do you need
> to set exactly? And maybe the text should be placed elsewhere; the
> copying of the pg_control file happened earlier already and you should
> probably modify it already when you copy it.

On second thought I think this should just be removed rather than moved.
Modifying pg_control is not something we talk about in the docs, so
there are no good examples, and I don't think we should start here.
Removed in v18.

>> +   The copy of <filename>pg_control</filename> stored in the backup
>> is flagged
>> +   to require the <filename>backup_label</filename> file for
>> recovery. If
>> +   <filename>backup_label</filename> is removed, recovery refuses to
>> start
>> +   rather than silently producing an inconsistent cluster. Restoring
>> +   <filename>backup_label</filename> to the root of the backup allows
>> +   recovery to proceed, and the flag is cleared once recovery has
>> read the
>> +   file. <xref linkend="app-pgresetwal"/> also clears the flag, but
>> discarding
>> +   the WAL bypasses the replay that recovery needs, leaving an
>> inconsistent
>> +   cluster.
>
> You shouldn't try to restore a backup_label file into an existing backup
> that's missing it, so that wording sounds dangerous.

Agreed, I updated this in v18 to recommend a fresh restored including
backup label. Who knows where a user might get a backup_label from
otherwise?

I also updated the related hint to suggest a new restore:

- errhint("Restore the backup_label file that was created during the
backup."));
+ errhint("Restore the backup again, including the backup_label
file."));

>>        defined as the result of <literal>pg_current_wal_insert_lsn()</
>> literal>
>>        when rewinding from a live source or the last checkpoint LSN when
>>        rewinding from a stopped source.
>> +      <filename>pg_control</filename> is also flagged to require
>> +      <filename>backup_label</filename>, so that removing that file
>> does not
>> +      allow the target to start up in an inconsistent state. The flag is
>> +      cleared once the target has processed <filename>backup_label</
>> filename>.
>
> That doesn't seem worth mentioning here; too much detail.

Agreed. Removed in v18.

>> @@ -604,6 +604,10 @@ InitWalRecovery(ControlFileData *ControlFile,
>> bool *wasShutdown_ptr,
>>                      ereport(FATAL,
>>                              errmsg("could not find redo location %X/
>> %08X referenced by checkpoint record at %X/%08X",
>>                                     LSN_FORMAT_ARGS(checkPoint.redo),
>> LSN_FORMAT_ARGS(CheckPointLoc)),
>> +                            ControlFile->backupLabelRequired ?
>> +                            errhint("Touch \"%s/recovery.signal\" or
>> \"%s/standby.signal\" and add required recovery options.\n"
>> +                                    "Do not remove \"%s/
>> backup_label\"; it is required to recover this backup.",
>> +                                    DataDir, DataDir, DataDir) :
>>                              errhint("If you are restoring from a
>> backup, touch \"%s/recovery.signal\" or \"%s/standby.signal\" and add
>> required recovery options.\n"
>>                                      "If you are not restoring from a
>> backup, try removing the file \"%s/backup_label\".\n"
>>                                      "Be careful: removing \"%s/
>> backup_label\" will result in a corrupt cluster if restoring from a
>> backup.",
>
> Umm, not new with his patch, but why do we still have that hint to
> remove backup_label? Is that ever the right thing to do? Why didn't we
> remove that hint when exclusive-mode backups were removed?

Unfortunately I believe there is still a use case for this hint. When
making a backup, e.g. via snapshot, it is still easier to write the
backup_label into PGDATA, make the snapshot, then remove it. If the
cluster crashes before the removal then the answer is to remove the
backup label, just as it was if exclusive backup crashed.

I'm not excited about the above use case but not sure we can discount it
entirely.

What we could do is document that the above use case should be
accomplished by copying backup_label to another file in PGDATA, e.g.
backup_label.snap, and then renaming it when the snapshot is restored. I
think this is reasonable given that recovery.signal/standby.signal still
needed to be created so there is some work to be done on the restore
side. Maybe then it would be OK to remove the hint?

That feels like the subject for a separate patch, though. I have added
it to my notes.

> Idea for further hardening: If you attempt to start up postgres and the
> backup_label is present, set the flag in the control file. That way,
> even if the backup tool doesn't modify the control file, it will be set
> on first startup attempt. If you delete the backup_label later, you
> still won't be able to start up into an inconsistent state. It won't
> protect you if you remove the backup_label before attempting to start
> up, but it'd catch some cases.
This would be dangerous if we accept that the above use case is valid.
In that case on restart the cluster would be bricked where in fact
removing the backup_label is the right thing to do.

Maybe we could set the flag when backup_label is present *and* either
standby.signal or recovery.signal. That works for the case above.

I originally had a patch that allowed backup software to get a non-torn
copy of pg_control with the flag set from pg_backup_stop(). If I can get
these patches committed I plan to reintroduce it. I think that is a
better answer for third-party software.

If we do decide to go with your idea instead (or in addition to), I'd
prefer to pursue it in a separate patch. I have added it to my notes.

Regards,
-David