Обсуждение: Return pg_control from pg_backup_stop().

Hackers,

This is greatly simplified implementation of the patch proposed in [1] 
and hopefully it addresses the concerns expressed there. Since the 
implementation is quite different it seemed like a new thread was 
appropriate, especially since the old thread title would be very 
misleading regarding the new functionality.

The basic idea is to harden recovery by returning a copy of pg_control 
from pg_backup_stop() that has a flag set to prevent recovery if the 
backup_label file is missing. Instead of backup software copying 
pg_control from PGDATA, it stores an updated version that is returned 
from pg_backup_stop(). This is better for the following reasons:

* The user can no longer remove backup_label and get what looks like a 
successful recovery (while almost certainly causing corruption). If 
backup_label is removed the cluster will not start. The user may try 
pg_resetwal, but that tool makes it pretty clear that corruption will 
result from its use.

* We don't need to worry about backup software seeing a torn copy of 
pg_control, since Postgres can safely read it out of memory and provide 
a valid copy via pg_backup_stop(). This solves torn reads without 
needing to write pg_control via a temp file, which may affect 
performance on a standby.

* For backup from standby, we no longer need to instruct the backup 
software to copy pg_control last. In fact the backup software should not 
copy pg_control from PGDATA at all.

These changes have no impact on current backup software and they are 
free to use the pg_control available from pg_stop_backup() or continue 
to use pg_control from PGDATA. Of course they will miss the benefits of 
getting a consistent copy of pg_control and the backup_label checking, 
but will be no worse off than before.

I'll register this in the July CF.

Regards,
-David

[1] 
https://www.postgresql.org/message-id/2daf8adc-8db7-4204-a7f2-a7e94e2bfa4b@pgmasters.net

On 1/24/25 13:43, David Steele wrote:
> 
> Rebased and improved a comment and an error.
Rebased to fix breakage caused by the split of func.sgml in 4e23c9e.

Regards,
-David

On 8/7/25 05:30, David Steele wrote:
> On 1/24/25 13:43, David Steele wrote:
>>
>> Rebased and improved a comment and an error.
> Rebased to fix breakage caused by the split of func.sgml in 4e23c9e.


Rebased to implement simplification added by "Simplify creation of 
built-in functions with default arguments" (759b03b2).

Regards,
-David

On 2/20/26 10:10, David Steele wrote:
> On 8/7/25 05:30, David Steele wrote:
>> On 1/24/25 13:43, David Steele wrote:
>>>
>>> Rebased and improved a comment and an error.
>> Rebased to fix breakage caused by the split of func.sgml in 4e23c9e.
> 
> 
> Rebased to implement simplification added by "Simplify creation of 
> built-in functions with default arguments" (759b03b2).

With the patches this time!

Regards,
-David

On 2/20/26 12:47, David Steele wrote:
> On 2/20/26 10:10, David Steele wrote:
>> On 8/7/25 05:30, David Steele wrote:
>>> On 1/24/25 13:43, David Steele wrote:
>>>>
>>>> Rebased and improved a comment and an error.
>>> Rebased to fix breakage caused by the split of func.sgml in 4e23c9e.
>>
>>
>> Rebased to implement simplification added by "Simplify creation of 
>> built-in functions with default arguments" (759b03b2).

Rebased on "Simplify creation of built-in functions with non-default 
ACLs." (f95d73ed).

Regards,
-David

On Mon, Mar 16, 2026 at 10:16:58PM -0700, Haibo Yan wrote:
> I have not read the code yet, so this may already be answered there,
> but I had a question about the proposal itself. This patch protects
> against a missing backup_label, but what about a wrong one? If a
> user restores a backup_label file from a different backup, the
> existence check alone would not detect that. Do we need some
> consistency check between the returned pg_control copy and the
> backup_label contents, or is the intended scope here limited to the
> “missing file” case only?

Please note that we use bottom-posting on the lists, as of:
https://en.wikipedia.org/wiki/Posting_style#Bottom-posting
--
Michael

On 3/17/26 12:16, Haibo Yan wrote:

> I have not read the code yet, so this may already be answered there, but 
> I had a question about the proposal itself. This patch protects against 
> a missing backup_label, but what about a wrong one? If a user restores a 
> backup_label file from a different backup, the existence check alone 
> would not detect that. Do we need some consistency check between the 
> returned pg_control copy and the backup_label contents, or is the 
> intended scope here limited to the “missing file” case only?

Thank you for having a look!

The goal here is only to check for a missing backup_label. The general 
problem is that PostgreSQL suggests that removing backup_label might be 
a good idea so the user does it:

If you are not restoring from a backup, try removing the file 
\"%s/backup_label\"

The user *could* copy a backup_label from another backup and there are 
ways we could detect that but I feel that should be material for a 
separate patch.

Regards,
-David

On Tue, Mar 17, 2026 at 11:50:29AM -0700, Haibo Yan wrote:
> Thank you for the clarification. I have now read the code, and
> overall it looks good to me. I only had one very small comment.

(Bottom-posting note from above, please be careful.)

> I was just wondering whether the following might be slightly clearer:
> ```
> memset(controlFile, 0, PG_CONTROL_FILE_SIZE);
> memcpy(controlFile, ControlFile, sizeof(ControlFileData));
> ```
>
> I do not think this is a real issue, though.

         {
             ControlFile->backupStartPoint = checkPoint.redo;
             ControlFile->backupEndRequired = backupEndRequired;
+            ControlFile->backupLabelRequired = false;

It sounds like it is going to be important to document the reason why
the flag is reset here (aka we don't need the backup_label file
anymore).

+backup_control_file(uint8_t *controlFile)
+{
+    ControlFileData *controlData = ((ControlFileData *)controlFile);
+
+    memset(controlFile + sizeof(ControlFileData), 0,
+           PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
+
+    LWLockAcquire(ControlFileLock, LW_SHARED);
+    memcpy(controlFile, ControlFile, sizeof(ControlFileData));
+    LWLockRelease(ControlFileLock);
+
+    controlData->backupLabelRequired = true;
+
+    INIT_CRC32C(controlData->crc);
+    COMP_CRC32C(controlData->crc, controlFile, offsetof(ControlFileData, crc));
+    FIN_CRC32C(controlData->crc);

I was wondering if we should have an assertion at least to cross-check
that the contents we store in shared memory never go out-of-sync with
the on-disk contents, in the shape of a USE_ASSERT_CHECKING block that
calls get_controlfile() and memcmp()'s the contents between shmem and
the on-disk file, while the LWLock is taken.  We ship the control file
last on purpose, one reason being backups taken from standbys, so that
may be sensible to do.

Another property of the new control file flag that is implied in the
implementation but not documented is that we should never check for
backupLabelRequired when a backup_label is gone.  Actually, the flag
is reset in InitWalRecovery() in the initial steps of recovery, and
the backup_label file is removed much later in StartupXLOG() just
*after* UpdateControlFile() to minimize the window where the contents
of the control file and the backup_label file is removed are
out-of-sync.  This window means that if we crash between the
completion of UpdateControlFile() and the durable_rename() we could
have a flag reset with a backup_label still around.  On restart,
recovery would fail, requiring a manual modification of the control
file, at least.  It sounds to me that this implementation detail
should be documented clearly.

Finally, here is a general opinion.  I like this patch, and it is
basically risk-free for base backups taken with the replication
protocol as we update the control file with the new flag set
on-the-fly.

Now, I am worried about backups that use pg_stop_backup().  Changing
backup APIs has always been a very sensitive area, and this is going
to require operators to update backup tools so as the control file
received as a result of pg_stop_backup() is copied, at the cost of
getting a failure if they don't do so.  I will *not* proceed with this
change without a clear approval from some more committers or senior
hackers that they like this change (approach previously suggested by
Andres, actually, for what I can see).  I am adding in CC a few
committers who have commented on this set of proposals and who have
touched the recovery code in the last few years, for awareness.
The timing is what it is, and we are at the end of a release cycle.
Let's see if we can reach a consensus of some kind.
--
Michael

On 3/18/26 08:43, Michael Paquier wrote:
> On Tue, Mar 17, 2026 at 11:50:29AM -0700, Haibo Yan wrote:
>> Thank you for the clarification. I have now read the code, and
>> overall it looks good to me. I only had one very small comment.
> 
> (Bottom-posting note from above, please be careful.)
> 
>> I was just wondering whether the following might be slightly clearer:
>> ```
>> memset(controlFile, 0, PG_CONTROL_FILE_SIZE);
>> memcpy(controlFile, ControlFile, sizeof(ControlFileData));
>> ```

Yeah, perhaps I am being too clever here. The reason why I like this 
pattern:

memset(controlFile + sizeof(ControlFileData), 0,
        PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));

...is that valgrind will complain if the ControlFileData part is not 
completely initialized later on.

But your version is what is generally used in the code so I'm fine with 
doing it that way.

>           {
>               ControlFile->backupStartPoint = checkPoint.redo;
>               ControlFile->backupEndRequired = backupEndRequired;
> +            ControlFile->backupLabelRequired = false;
> 
> It sounds like it is going to be important to document the reason why
> the flag is reset here (aka we don't need the backup_label file
> anymore).

Good point -- I'll add that in the next revision.

> +backup_control_file(uint8_t *controlFile)
> +{
> +    ControlFileData *controlData = ((ControlFileData *)controlFile);
> +
> +    memset(controlFile + sizeof(ControlFileData), 0,
> +           PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
> +
> +    LWLockAcquire(ControlFileLock, LW_SHARED);
> +    memcpy(controlFile, ControlFile, sizeof(ControlFileData));
> +    LWLockRelease(ControlFileLock);
> +
> +    controlData->backupLabelRequired = true;
> +
> +    INIT_CRC32C(controlData->crc);
> +    COMP_CRC32C(controlData->crc, controlFile, offsetof(ControlFileData, crc));
> +    FIN_CRC32C(controlData->crc);
> 
> I was wondering if we should have an assertion at least to cross-check
> that the contents we store in shared memory never go out-of-sync with
> the on-disk contents, in the shape of a USE_ASSERT_CHECKING block that
> calls get_controlfile() and memcmp()'s the contents between shmem and
> the on-disk file, while the LWLock is taken.  We ship the control file
> last on purpose, one reason being backups taken from standbys, so that
> may be sensible to do.

As far as I can see this should always be true -- I audited all the

LWLockAcquire(ControlFileLock, LW_EXCLUSIVE)

sections and the file is always saved once if is updated. Let me see if 
I can add this check without too much pain, e.g. an additional parameter.

> Another property of the new control file flag that is implied in the
> implementation but not documented is that we should never check for
> backupLabelRequired when a backup_label is gone.  

I'm not sure what you mean here? That's exactly when we do want to check 
as below:

/*
     * No backup_label file has been found if we are here. Error if the
     * control file requires backup_label.
     */
if (ControlFile->backupLabelRequired)
     ereport(FATAL,
             (errmsg("could not find backup_label required for recovery"),
                 errhint("backup_label must be present for recovery to 
proceed")));


 > Actually, the flag> is reset in InitWalRecovery() in the initial 
steps of recovery, and
> the backup_label file is removed much later in StartupXLOG() just
> *after* UpdateControlFile() to minimize the window where the contents
> of the control file and the backup_label file is removed are
> out-of-sync.  This window means that if we crash between the
> completion of UpdateControlFile() and the durable_rename() we could
> have a flag reset with a backup_label still around.  On restart,
> recovery would fail, requiring a manual modification of the control
> file, at least.  It sounds to me that this implementation detail
> should be documented clearly.

I'll test it but I don't think this is the case. If backup_label is 
present then we'll just update pg_control again as we do now.

When backup_label is present the value of backupLabelRequired does not 
matter so it just follows the prior logic.

> Finally, here is a general opinion.  I like this patch, and it is
> basically risk-free for base backups taken with the replication
> protocol as we update the control file with the new flag set
> on-the-fly.

Glad to hear it!

> Now, I am worried about backups that use pg_stop_backup().  Changing
> backup APIs has always been a very sensitive area, and this is going
> to require operators to update backup tools so as the control file
> received as a result of pg_stop_backup() is copied, at the cost of
> getting a failure if they don't do so.  

Using the pg_control copy from pg_backup_stop() is entirely optional and 
nothing is broken if vendors decide not to use it. This can be 
demonstrated by applying the 01 patch without 02. In that case the tests 
in 042_low_level_backup continue to run. You can also apply 01 and 02 
and revert the test changes in 042_low_level_backup and that works, too.

Vendors can use the new feature if they want the protection of 
backupLabelRequired and a guaranteed non-torn copy of pg_control but if 
they do nothing then nothing breaks.

> I will *not* proceed with this
> change without a clear approval from some more committers or senior
> hackers that they like this change (approach previously suggested by
> Andres, actually, for what I can see).  

You are correct and this was an omission on my part. If this gets to 
commit we'll definitely want to mention that this flag was Andres' 
suggestion.

> I am adding in CC a few
> committers who have commented on this set of proposals and who have
> touched the recovery code in the last few years, for awareness.
> The timing is what it is, and we are at the end of a release cycle.
> Let's see if we can reach a consensus of some kind.

Perfect. I'm looking forward to their input.

I'll hold off on a new patch version until we get some feedback since I 
don't think any of the requested changes are critical to the functionality.

Regards,
-David

On Wed, Mar 18, 2026 at 04:05:24AM +0000, David Steele wrote:
> On 3/18/26 08:43, Michael Paquier wrote:
>> I was wondering if we should have an assertion at least to cross-check
>> that the contents we store in shared memory never go out-of-sync with
>> the on-disk contents, in the shape of a USE_ASSERT_CHECKING block that
>> calls get_controlfile() and memcmp()'s the contents between shmem and
>> the on-disk file, while the LWLock is taken.  We ship the control file
>> last on purpose, one reason being backups taken from standbys, so that
>> may be sensible to do.
>
> As far as I can see this should always be true -- I audited all the
>
> LWLockAcquire(ControlFileLock, LW_EXCLUSIVE)
>
> sections and the file is always saved once if is updated. Let me see if I
> can add this check without too much pain, e.g. an additional parameter.

This matches with my reads of the code.  The attached check, that can
be applied on top of your patches, passes under check-world.

>> Another property of the new control file flag that is implied in the
>> implementation but not documented is that we should never check for
>> backupLabelRequired when a backup_label is gone.
>
> I'm not sure what you mean here? That's exactly when we do want to check as
> below:

Sorry for the confusion, I meant that "we should never check for
backupLabelRequired when we have a backup_label".

> Using the pg_control copy from pg_backup_stop() is entirely optional and
> nothing is broken if vendors decide not to use it. This can be demonstrated
> by applying the 01 patch without 02. In that case the tests in
> 042_low_level_backup continue to run. You can also apply 01 and 02 and
> revert the test changes in 042_low_level_backup and that works, too.

FWIW, after a second look I am actually wondering if 0002 is safe at
all.  The contents of the control file are fetched after we are done
with do_pg_backup_stop(), and there could be a bunch of activity that
happens between the end of do_pg_backup_stop() and the
backup_control_file() call, where the control file could be updated
more and interfere with the recovery startup for some of its fields?
GUC parameter updates that may touch the control file are one thing
popping into mind.
--
Michael

On 3/18/26 11:53, Michael Paquier wrote:
> On Wed, Mar 18, 2026 at 04:05:24AM +0000, David Steele wrote:
>> On 3/18/26 08:43, Michael Paquier wrote:
>>> I was wondering if we should have an assertion at least to cross-check
>>> that the contents we store in shared memory never go out-of-sync with
>>> the on-disk contents, in the shape of a USE_ASSERT_CHECKING block that
>>> calls get_controlfile() and memcmp()'s the contents between shmem and
>>> the on-disk file, while the LWLock is taken.  We ship the control file
>>> last on purpose, one reason being backups taken from standbys, so that
>>> may be sensible to do.
>>
>> As far as I can see this should always be true -- I audited all the
>>
>> LWLockAcquire(ControlFileLock, LW_EXCLUSIVE)
>>
>> sections and the file is always saved once if is updated. Let me see if I
>> can add this check without too much pain, e.g. an additional parameter.
> 
> This matches with my reads of the code.  The attached check, that can
> be applied on top of your patches, passes under check-world.

Looks good to me. I have integrated it into the attached patches. I just 
changed it to look at our copy instead of the global ControlData since 
that is the one we are going to save.

>> Using the pg_control copy from pg_backup_stop() is entirely optional and
>> nothing is broken if vendors decide not to use it. This can be demonstrated
>> by applying the 01 patch without 02. In that case the tests in
>> 042_low_level_backup continue to run. You can also apply 01 and 02 and
>> revert the test changes in 042_low_level_backup and that works, too.
> 
> FWIW, after a second look I am actually wondering if 0002 is safe at
> all.  The contents of the control file are fetched after we are done
> with do_pg_backup_stop(), and there could be a bunch of activity that
> happens between the end of do_pg_backup_stop() and the
> backup_control_file() call, where the control file could be updated
> more and interfere with the recovery startup for some of its fields?
> GUC parameter updates that may touch the control file are one thing
> popping into mind.

You are correct -- the copy of pg_control needs to happen before 
do_pg_backup_stop(). An older version of this patch saved pg_control in 
backup_state which made the prior location safe. However, I missed 
moving this code when I moved pg_control out of backup_state. Code 
review to the rescue.

I believe I have addressed all current review comments in the attached 
patches. I tested Postgres crashing right after pg_control is updated 
but before backup_label is renamed. It worked as expected on restart. I 
also manually removed backup_label before restarting and that worked as 
well. I wrote a comment documenting all that.

One thing I could do is note in the documentation that it is not 
strictly necessary to get pg_control from pg_backup_stop(). Right now it 
sounds like copying from disk is no longer an option -- but it is if you 
are willing to accept the possibility of pg_control being torn. But I'd 
hope most well-maintained backup software is taking care of that by now.

The problem with caveats in the docs is it can lead to confusion and 
getting pg_control from pg_backup_stop() is just a better idea in general.

Thoughts?

Regards,
-David

On Wed, Mar 18, 2026 at 07:35:47AM +0000, David Steele wrote:
> You are correct -- the copy of pg_control needs to happen before
> do_pg_backup_stop(). An older version of this patch saved pg_control in
> backup_state which made the prior location safe. However, I missed moving
> this code when I moved pg_control out of backup_state. Code review to the
> rescue.

Right.  I am wondering also if the final result would not be better
without 0002, actually, focusing only on the "simpler" base backup
case through the replication protocol, and you are making a good case
in mentioning it as not absolutely mandatory for base backups that are
taken through the SQL functions.  One could always tweak the flag
manually in the control file based on the contents taken from the data
folder.  That's more hairy than writing the entire file, for sure,
still possible.
--
Michael

On 3/18/26 15:26, Michael Paquier wrote:
> On Wed, Mar 18, 2026 at 07:35:47AM +0000, David Steele wrote:
>> You are correct -- the copy of pg_control needs to happen before
>> do_pg_backup_stop(). An older version of this patch saved pg_control in
>> backup_state which made the prior location safe. However, I missed moving
>> this code when I moved pg_control out of backup_state. Code review to the
>> rescue.
> 
> Right.  I am wondering also if the final result would not be better
> without 0002, actually, focusing only on the "simpler" base backup
> case through the replication protocol, and you are making a good case
> in mentioning it as not absolutely mandatory for base backups that are
> taken through the SQL functions.  One could always tweak the flag
> manually in the control file based on the contents taken from the data
> folder.  That's more hairy than writing the entire file, for sure,
> still possible.

Getting even 01 into PG19 would be a great outcome. This would solve the 
problem of torn pg_control and deleted backup labels for any backups 
made with pg_basebackup and that's going to cover a *lot* of cases.

Established third-party backup solutions that are not based on 
pg_basebackup are generally able to manipulate pg_control so that's not 
as much of a concern, perhaps. It does raise the barrier of entry for 
new backup software if they need to learn to read and validate 
pg_control to avoid a torn copy and set the flag. Patch 02 solves that 
problem in a general way so I still think it adds value for the 
ecosystem -- but we could always discuss that in the PG20 cycle.

Whatever gets committed for PG19 I'll write a followup patch to describe 
the hazards of reading pg_control and generally how to get a good copy. 
However, this will be complicated enough that the best answer will 
likely be to use pg_basebackup or some other reputable backup software. 
I don't love this -- I feel like the low-level interface should be 
usable with such hazards.

Regards,
-David

Hi Corey,

On 3/19/26 04:00, Corey Huinker wrote:
>     Whatever gets committed for PG19 I'll write a followup patch to
>     describe
>     the hazards of reading pg_control and generally how to get a good copy.
>     However, this will be complicated enough that the best answer will
>     likely be to use pg_basebackup or some other reputable backup software.
>     I don't love this -- I feel like the low-level interface should be
>     usable with such hazards.
> 
> Surya Poondla and I had decided on this patchset as a pair-reviewing 
> exercise. However, events have overtaken us, and several other people 
> have chimed in expressing the same concerns that we had observed but 
> hadn't yet completed our review. 

Thank you both for having a look!

 > All of the main concerns that we had > found up to this point have 
been addressed in the lastest patchset,
> except for the trivial observation that the ereport() uses the old style 
> and doesn't need the set of parens around (errmsg(), errhint()). 

Grep shows there are lots of messages with the new style but many more 
in the old style. Presumably they are only being updated as they are 
modified. Do you happen to know the commit or message thread where this 
policy was started? I've been searching but it is such a generic search 
term.

I've updated the message style in the new patches.

> Patches 
> apply clean, tests pass, test coverage seems sufficient, we're happy 
> with the wording of the documentation, in short there really isn't a 
> whole lot for us to add to the review, and for that reason we're 
> removing our names from the list of reviewers in the commitfest app.

It seems to me you've still done a review. Confirming what the other 
reviewers found is good info to have.

Regards,
-David