Re: Return pg_control from pg_backup_stop().

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