Thread

1. Return pg_control from pg_backup_stop().

   David Steele <david@pgmasters.net> — 2024-05-17T02:46:49Z

   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

2. Re: Return pg_control from pg_backup_stop().

   Michael Paquier <michael@paquier.xyz> — 2024-10-02T07:15:55Z

   On Fri, May 17, 2024 at 12:46:49PM +1000, David Steele wrote:
   > 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.
   
   -        /* No backup_label file has been found if we are here. */
   +        /*
   +         * 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 succeed")));
   
   I thought that this had some similarities with my last fight in this
   area, where xlogrecovery.c would fail hard if there was a backup_label
   file but no signal files, but nope that's not the case:
   https://www.postgresql.org/message-id/flat/ZArVOMifjzE7f8W7%40paquier.xyz
   
   > 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().
   
   Hmm, okay.  There is also a slight impact for BASE_BACKUP, requiring
   basebackup_progress_wait_wal_archive() and do_pg_backup_stop() to be
   called earlier when sending the main data directory which is the last
   one in the list of tablespaces.  As far as I can see, this does not
   change the logic because do_pg_backup_stop() does not touch the
   control file, but it seems to me that we'd rather keep these two
   calls as they are now, and send the control file once we are out of
   the for loop that processes all the tablespaces.  That seems slightly
   cleaner to me, still I agree that both are the same things.
   
   Anyway, finishing do_pg_backup_stop() and then sending the control
   file is just a consequence of the implementation choice driven by the
   output required for the SQL function so as this is stored in the
   backup state to get it down to pg_backup_stop() in xlogfuncs.c, so we
   could also take one step back and forget about the SQL function,
   setting only the new flag when sending a BASE_BACKUP, or just not use
   the backupState to store this data as the exercise involves forcing
   one boolean and recalculate a CRC32.
   
   > * 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.
   
   We're talking about a 8kB file which has a size of 512B
   (PG_CONTROL_MAX_SAFE_SIZE) to avoid such issues.  So I'm not sure to
   see your point here?
   
   > * 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.
   
   Yep.  Not from PGDATA, but from the function.
   
   > 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.
   
   There is a large comment block in do_pg_backup_stop() around
   backup_stopped_in_recovery.  Perhaps it should be improved based on
   this patch.
   
   The main concern that I have over this patch is: who is actually going
   to use this extension of the SQL stop function?  Perhaps existing
   backup solutions are good enough risk vs reward is not worth it?  The
   label_file and the tablespace map are text, this is a series of bytes
   that has no visibility for the end-user unless checked on the
   client-side.  This adds a new step where backups would need to copy
   the control file to the data folder.
   --
   Michael
   

3. Re: Return pg_control from pg_backup_stop().

   David Steele <david@pgbackrest.org> — 2024-10-02T09:03:27Z

   On 10/2/24 10:11, Michael Paquier wrote:
   > On Fri, May 17, 2024 at 12:46:49PM +1000, David Steele wrote:
   > 
   >> 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().
   > 
   > Hmm, okay.  There is also a slight impact for BASE_BACKUP, requiring
   > basebackup_progress_wait_wal_archive() and do_pg_backup_stop() to be
   > called earlier when sending the main data directory which is the last
   > one in the list of tablespaces.  As far as I can see, this does not
   > change the logic because do_pg_backup_stop() does not touch the
   > control file, but it seems to me that we'd rather keep these two
   > calls as they are now, and send the control file once we are out of
   > the for loop that processes all the tablespaces.  That seems slightly
   > cleaner to me, still I agree that both are the same things.
   
   Sending pg_control later results in even more code churn because of how 
   tar files are terminated. I've updated it that way in v2 so you can see 
   what I mean. I don't have a strong preference, though, so if you prefer 
   the implementation in v2 then that's fine with me.
   
   > Anyway, finishing do_pg_backup_stop() and then sending the control
   > file is just a consequence of the implementation choice driven by the
   > output required for the SQL function so as this is stored in the
   > backup state to get it down to pg_backup_stop() in xlogfuncs.c, so we
   > could also take one step back and forget about the SQL function,
   > setting only the new flag when sending a BASE_BACKUP, or just not use
   > the backupState to store this data as the exercise involves forcing
   > one boolean and recalculate a CRC32.
   
   I can definitely see us making other updates to pg_control so I would 
   rather keep this logic centralized, even though it is not too 
   complicated at this point. Still, even 8 lines of code (as it is now) 
   seems better not to duplicate.
   
   >> * 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.
   > 
   > We're talking about a 8kB file which has a size of 512B
   > (PG_CONTROL_MAX_SAFE_SIZE) to avoid such issues.  So I'm not sure to
   > see your point here?
   
   Even at 512B it is possible to see tears in pg_control and they happen 
   in the build farm right now. In fact, this thread [1] trying to fix the 
   problem was what got me thinking about alternate solutions to preventing 
   tears in pg_control. Thomas' proposed fixes have not been committed to 
   my knowledge so the problem remains, but would be fixed by this commit.
   
   > There is a large comment block in do_pg_backup_stop() around
   > backup_stopped_in_recovery.  Perhaps it should be improved based on
   > this patch.
   
   I added a sentence to this comment block in v2.
   
   > The main concern that I have over this patch is: who is actually going
   > to use this extension of the SQL stop function?
   
   Primarily existing backup software, I would imagine. The idea is that it 
   would give them feature parity with pg_basebackup, rather than the new 
   protections being exclusive to pg_basebackup.
   
   > Perhaps existing
   > backup solutions are good enough risk vs reward is not worth it?  
   
   I'm not sure I see the risk here. Saving out pg_control is optional so 
   no changes to current software is required. Of course they miss the 
   benefit of the protection against tears and missing backup_label, but 
   that is a choice.
   
   Also, no matter what current backup solutions do, they cannot prevent a 
   user from removing backup_label after restore. This patch prevents 
   invalid recovery when that happens.
   
   > The
   > label_file and the tablespace map are text, this is a series of bytes
   > that has no visibility for the end-user unless checked on the
   > client-side.  This adds a new step where backups would need to copy
   > the control file to the data folder.
   
   Again, optional, but if I was able to manage these saves using the psql 
   interface in the TAP tests then I'd say it would be pretty easy for 
   anyone with a normal connection to Postgres. Also, we require users to 
   treat tabelspace_map and backup_label as binary so not too big a change 
   here.
   
   [1] 
   https://www.postgresql.org/message-id/CA%2BhUKG%2Bjig%2BQdBETj_ab%2B%2BVWSoJjbwT3sLNCnk0wFsY_6tRqoA%40mail.gmail.com

4. Re: Return pg_control from pg_backup_stop().

   Michael Paquier <michael@paquier.xyz> — 2024-10-03T04:45:43Z

   On Wed, Oct 02, 2024 at 09:03:27AM +0000, David Steele wrote:
   > On 10/2/24 10:11, Michael Paquier wrote:
   >> Hmm, okay.  There is also a slight impact for BASE_BACKUP, requiring
   >> basebackup_progress_wait_wal_archive() and do_pg_backup_stop() to be
   >> called earlier when sending the main data directory which is the last
   >> one in the list of tablespaces.  As far as I can see, this does not
   >> change the logic because do_pg_backup_stop() does not touch the
   >> control file, but it seems to me that we'd rather keep these two
   >> calls as they are now, and send the control file once we are out of
   >> the for loop that processes all the tablespaces.  That seems slightly
   >> cleaner to me, still I agree that both are the same things.
   > 
   > Sending pg_control later results in even more code churn because of how tar
   > files are terminated. I've updated it that way in v2 so you can see what I
   > mean. I don't have a strong preference, though, so if you prefer the
   > implementation in v2 then that's fine with me.
   
   It does not make much of a difference, indeed.
   
   >> Anyway, finishing do_pg_backup_stop() and then sending the control
   >> file is just a consequence of the implementation choice driven by the
   >> output required for the SQL function so as this is stored in the
   >> backup state to get it down to pg_backup_stop() in xlogfuncs.c, so we
   >> could also take one step back and forget about the SQL function,
   >> setting only the new flag when sending a BASE_BACKUP, or just not use
   >> the backupState to store this data as the exercise involves forcing
   >> one boolean and recalculate a CRC32.
   > 
   > I can definitely see us making other updates to pg_control so I would rather
   > keep this logic centralized, even though it is not too complicated at this
   > point. Still, even 8 lines of code (as it is now) seems better not to
   > duplicate.
   
   I was wondering if the field update should be hidden behind a macro
   that uses an offsetof() on ControlFileData, with the name of the field
   and a pointer to the value to update to.  If you include the CRC32
   calculation in that, that makes for less chunks of code when updating
   one field of the control file.
   
   The full CRC calculation could also be hidden inside a macro, as there
   are a couple of places where we do the same things, like pg_rewind.c,
   etc.
   
   >> We're talking about a 8kB file which has a size of 512B
   >> (PG_CONTROL_MAX_SAFE_SIZE) to avoid such issues.  So I'm not sure to
   >> see your point here?
   > 
   > Even at 512B it is possible to see tears in pg_control and they happen in
   > the build farm right now. In fact, this thread [1] trying to fix the problem
   > was what got me thinking about alternate solutions to preventing tears in
   > pg_control. Thomas' proposed fixes have not been committed to my knowledge
   > so the problem remains, but would be fixed by this commit.
   
   Ah, right.  That rings a bell.  Thomas has done some work with
   c558e6fd92ff and 63a582222c6b.  And we're still not taking the
   ControlFileLock while copying it over..  It looks like we should do it
   separately, and backpatch.  That's not something for this thread to
   worry about.
   
   >> Perhaps existing
   >> backup solutions are good enough risk vs reward is not worth it?
   > 
   > I'm not sure I see the risk here. Saving out pg_control is optional so no
   > changes to current software is required. Of course they miss the benefit of
   > the protection against tears and missing backup_label, but that is a choice.
   >
   > Again, optional, but if I was able to manage these saves using the psql
   > interface in the TAP tests then I'd say it would be pretty easy for anyone
   > with a normal connection to Postgres. Also, we require users to treat
   > tabelspace_map and backup_label as binary so not too big a change here.
   
   Maintenance cost for a limited user impact overall.  With incremental
   backups being a thing in v18 only available through the replication
   protocol, the SQL functions have less advantages these days.  My point
   would be to see this thread as a two-step process:
   1) Update the flag in the control file when sending it across in
   replication stream.
   2) Do the SQL function thing with the bytea for the control file, if
   necessary.
   
   1) is something that has more value than 2), IMO, because there is no
   need for a manual step when a backup is taken by the replication
   protocol.  Well, custom backup solutions that rely on the replication
   protocol to copy the data would need to make sure that they have a
   backup_label, but that's something they should do anyway and what this
   patch wants to protect users from.  The SQL part is optional IMO.  It
   can be done, but it has less impact overall and makes backups more
   complicated by requiring the manual copy of the control file. 
   --
   Michael
   

5. Re: Return pg_control from pg_backup_stop().

   David Steele <david@pgbackrest.org> — 2024-10-03T09:11:14Z

   On 10/3/24 07:45, Michael Paquier wrote:
   > On Wed, Oct 02, 2024 at 09:03:27AM +0000, David Steele wrote:
   >> On 10/2/24 10:11, Michael Paquier wrote:
   > 
   >> I can definitely see us making other updates to pg_control so I would rather
   >> keep this logic centralized, even though it is not too complicated at this
   >> point. Still, even 8 lines of code (as it is now) seems better not to
   >> duplicate.
   > 
   > I was wondering if the field update should be hidden behind a macro
   > that uses an offsetof() on ControlFileData, with the name of the field
   > and a pointer to the value to update to.  If you include the CRC32
   > calculation in that, that makes for less chunks of code when updating
   > one field of the control file.
   > 
   > The full CRC calculation could also be hidden inside a macro, as there
   > are a couple of places where we do the same things, like pg_rewind.c,
   > etc.
   
   This seems to be a different case than pg_rewind, especially since we 
   need a ControlFileLock. I think that is a bit much to do in a macro, so 
   I split the functionality out into a function instead. This simplifies 
   the logic in basebackup.c but has little impact elsewhere.
   
   >>> We're talking about a 8kB file which has a size of 512B
   >>> (PG_CONTROL_MAX_SAFE_SIZE) to avoid such issues.  So I'm not sure to
   >>> see your point here?
   >>
   >> Even at 512B it is possible to see tears in pg_control and they happen in
   >> the build farm right now. In fact, this thread [1] trying to fix the problem
   >> was what got me thinking about alternate solutions to preventing tears in
   >> pg_control. Thomas' proposed fixes have not been committed to my knowledge
   >> so the problem remains, but would be fixed by this commit.
   > 
   > Ah, right.  That rings a bell.  Thomas has done some work with
   > c558e6fd92ff and 63a582222c6b.  And we're still not taking the
   > ControlFileLock while copying it over..  It looks like we should do it
   > separately, and backpatch.  That's not something for this thread to
   > worry about.
   
   I'd be happy to adapt patch 01 to be back-patched (without the new flag) 
   if we decide it is a good idea. Just locking and making a copy of 
   pg_control is easy enough, but if we accept the backup_control_file() 
   function for new versions then we could keep that for the back patch to 
   reduce churn between versions.
   
   >>> Perhaps existing
   >>> backup solutions are good enough risk vs reward is not worth it?
   >>
   >> I'm not sure I see the risk here. Saving out pg_control is optional so no
   >> changes to current software is required. Of course they miss the benefit of
   >> the protection against tears and missing backup_label, but that is a choice.
   >>
   >> Again, optional, but if I was able to manage these saves using the psql
   >> interface in the TAP tests then I'd say it would be pretty easy for anyone
   >> with a normal connection to Postgres. Also, we require users to treat
   >> tabelspace_map and backup_label as binary so not too big a change here.
   > 
   > Maintenance cost for a limited user impact overall.  With incremental
   > backups being a thing in v18 only available through the replication
   > protocol, the SQL functions have less advantages these days.  My point
   > would be to see this thread as a two-step process:
   > 1) Update the flag in the control file when sending it across in
   > replication stream.
   > 2) Do the SQL function thing with the bytea for the control file, if
   > necessary.
   
   OK, I have split the patch into two parts along these lines.
   
   > 1) is something that has more value than 2), IMO, because there is no
   > need for a manual step when a backup is taken by the replication
   > protocol.  Well, custom backup solutions that rely on the replication
   > protocol to copy the data would need to make sure that they have a
   > backup_label, but that's something they should do anyway and what this
   > patch wants to protect users from.  The SQL part is optional IMO.  It
   > can be done, but it has less impact overall and makes backups more
   > complicated by requiring the manual copy of the control file.
   
   I don't think having incremental backup in pg_basebackup means alternate 
   backup solutions are going away or that we should deprecate the SQL 
   interface. If nothing else, third-party solutions need a way to get an 
   untorn copy of pg_control and in general I think the new flag will be 
   universally useful.
   
   Regards,
   -David

6. Re: Return pg_control from pg_backup_stop().

   David Steele <david@pgbackrest.org> — 2024-11-20T22:44:36Z

   On 10/3/24 05:11, David Steele wrote:
   > On 10/3/24 07:45, Michael Paquier wrote:
   > 
   >> 1) is something that has more value than 2), IMO, because there is no
   >> need for a manual step when a backup is taken by the replication
   >> protocol.  Well, custom backup solutions that rely on the replication
   >> protocol to copy the data would need to make sure that they have a
   >> backup_label, but that's something they should do anyway and what this
   >> patch wants to protect users from.  The SQL part is optional IMO.  It
   >> can be done, but it has less impact overall and makes backups more
   >> complicated by requiring the manual copy of the control file.
   > 
   > I don't think having incremental backup in pg_basebackup means alternate 
   > backup solutions are going away or that we should deprecate the SQL 
   > interface. If nothing else, third-party solutions need a way to get an 
   > untorn copy of pg_control and in general I think the new flag will be 
   > universally useful.
   
   I updated this patch to fix an issue with -fsanitize=alignment. I'm not 
   entirely happy with copying twice but not sure of another way to do it. 
   As far as I can see VARDATA() will not return aligned data on 64-bit 
   architectures.
   
   Regards,
   -David
   

7. Re: Return pg_control from pg_backup_stop().

   David Steele <david@pgbackrest.org> — 2025-01-24T18:43:02Z

   On 11/20/24 17:44, David Steele wrote:
   > On 10/3/24 05:11, David Steele wrote:
   >> On 10/3/24 07:45, Michael Paquier wrote:
   >>
   >>> 1) is something that has more value than 2), IMO, because there is no
   >>> need for a manual step when a backup is taken by the replication
   >>> protocol.  Well, custom backup solutions that rely on the replication
   >>> protocol to copy the data would need to make sure that they have a
   >>> backup_label, but that's something they should do anyway and what this
   >>> patch wants to protect users from.  The SQL part is optional IMO.  It
   >>> can be done, but it has less impact overall and makes backups more
   >>> complicated by requiring the manual copy of the control file.
   >>
   >> I don't think having incremental backup in pg_basebackup means 
   >> alternate backup solutions are going away or that we should deprecate 
   >> the SQL interface. If nothing else, third-party solutions need a way 
   >> to get an untorn copy of pg_control and in general I think the new 
   >> flag will be universally useful.
   > 
   > I updated this patch to fix an issue with -fsanitize=alignment. I'm not 
   > entirely happy with copying twice but not sure of another way to do it. 
   > As far as I can see VARDATA() will not return aligned data on 64-bit 
   > architectures.
   
   Rebased and improved a comment and an error.
   
   Regards,
   -David

8. Re: Return pg_control from pg_backup_stop().

   David Steele <david@pgbackrest.org> — 2025-08-06T22:30:50Z

   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

9. Re: Return pg_control from pg_backup_stop().

   David Steele <david@pgbackrest.org> — 2026-02-20T03:10:34Z

   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
   
   
   
   

10. Re: Return pg_control from pg_backup_stop().

    David Steele <david@pgbackrest.org> — 2026-02-20T05:47:26Z

    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

11. Re: Return pg_control from pg_backup_stop().

    David Steele <david@pgbackrest.org> — 2026-03-06T01:27:38Z

    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

12. Re: Return pg_control from pg_backup_stop().

    Haibo Yan <tristan.yim@gmail.com> — 2026-03-17T05:16:58Z

    Hi David
    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?
    Regards
    Haibo
    
    > On Mar 5, 2026, at 5:27 PM, David Steele <david@pgbackrest.org> wrote:
    > 
    > 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<pgcontrol-flag-v8-01-basebackup.patch><pgcontrol-flag-v8-02-sql.patch>
    
    

13. Re: Return pg_control from pg_backup_stop().

    Michael Paquier <michael@paquier.xyz> — 2026-03-17T05:51:12Z

    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
    

14. Re: Return pg_control from pg_backup_stop().

    David Steele <david@pgbackrest.org> — 2026-03-17T07:05:06Z

    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
    
    
    
    
    
    

15. Re: Return pg_control from pg_backup_stop().

    Haibo Yan <tristan.yim@gmail.com> — 2026-03-17T18:50:29Z

    Hi David,
    
    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.
    
    You currently have:
    ```
    memset(controlFile + sizeof(ControlFileData), 0,
           PG_CONTROL_FILE_SIZE - sizeof(ControlFileData));
    memcpy(controlFile, ControlFile, sizeof(ControlFileData));
    ``` 
    This is correct, since only the trailing bytes need to be zeroed before the copy.
    
    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.
    
    Thanks
    Haibo
    
    > On Mar 17, 2026, at 12:05 AM, David Steele <david@pgbackrest.org> wrote:
    > 
    > 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
    > 
    > 
    
    

16. Re: Return pg_control from pg_backup_stop().

    Michael Paquier <michael@paquier.xyz> — 2026-03-18T01:43:29Z

    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
    

17. Re: Return pg_control from pg_backup_stop().

    David Steele <david@pgbackrest.org> — 2026-03-18T04:05:24Z

    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
    
    
    
    

18. Re: Return pg_control from pg_backup_stop().

    Michael Paquier <michael@paquier.xyz> — 2026-03-18T04:53:51Z

    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
    

19. Re: Return pg_control from pg_backup_stop().

    David Steele <david@pgbackrest.org> — 2026-03-18T07:35:47Z

    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
    

20. Re: Return pg_control from pg_backup_stop().

    Michael Paquier <michael@paquier.xyz> — 2026-03-18T08:26:43Z

    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
    

21. Re: Return pg_control from pg_backup_stop().

    David Steele <david@pgbackrest.org> — 2026-03-18T12:26:48Z

    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
    
    
    
    

22. Re: Return pg_control from pg_backup_stop().

    Corey Huinker <corey.huinker@gmail.com> — 2026-03-18T21:00:34Z

    >
    > 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. 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()). 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.
    

23. Re: Return pg_control from pg_backup_stop().

    David Steele <david@pgbackrest.org> — 2026-03-19T02:55:58Z

    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

24. Re: Return pg_control from pg_backup_stop().

    Corey Huinker <corey.huinker@gmail.com> — 2026-03-19T04:20:40Z

    On Wed, Mar 18, 2026 at 10:56 PM David Steele <david@pgbackrest.org> wrote:
    
    > 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.
    
    
    That's always been my assumption. Not worth the churn.
    
    
    > 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 limited my git log -p to elog.h, and it seems it started with
    e3a87b4991cc back in 2020. The only reason I knew about it was that I used
    to do backports from v13 to unsupported versions, and the new style would
    cause the build to fail on an otherwise clean cherry pick.
    
    
    > It seems to me you've still done a review. Confirming what the other
    > reviewers found is good info to have.
    >
    
    Of a sort, yes, but our review doesn't touch the "is this a good idea"
    question, which has been by far the thing most in need of reviewing across
    the long discussion threads.
    

25. Re: Return pg_control from pg_backup_stop().

    David Steele <david@pgbackrest.org> — 2026-04-13T14:55:10Z

    On 3/18/26 19:26, David Steele wrote:
    > 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.
    
    I have withdrawn this patch. If anybody wants to pick it up in the 
    future I'll be happy to rebase it but I think two years is long enough 
    to maintain a patch that is not getting traction.
    
    We are left with the issue that pg_basebackup backups may contain a torn 
    copy of pg_control. At the least this should be documented.
    
    It would also be a good idea to document that utilizing the low-level 
    backup interface requires validating the checksum in pg_control to avoid 
    a torn copy. This is non-trivial but certainly doable.
    
    Regards,
    -David
    
    
    
    

26. Re: Return pg_control from pg_backup_stop().

    David Steele <david@pgbackrest.org> — 2026-06-30T05:15:19Z

    On 4/13/26 21:55, David Steele wrote:
    > 
    > I have withdrawn this patch. If anybody wants to pick it up in the 
    > future I'll be happy to rebase it but I think two years is long enough 
    > to maintain a patch that is not getting traction.
    I would like to revive this but focus on the first patch for now and 
    drop the second patch from consideration.
    
    The patch now implements only the new flag for pgcontrol and wires this 
    logic into basebackup. It has the additional benefit of guaranteeing 
    that the base backup contains a non-torn version of pgcontrol.
    
    I know everyone was really busy in March but now that things are a bit 
    calmer I'd like to revisit.
    
    Heikki, Robert, Andres, Fujii -- any objections or comments? I believe 
    Michael is on board with the feature (this part, at least) but he very 
    sensibly would like to have some consensus.
    
    Thanks,
    -David