Hi Thomas,
On 7/26/23 06:06, Thomas Munro wrote:
> While chatting to Robert and Andres about all this, a new idea came
> up. Or, rather, one of the first ideas that was initially rejected,
> now resurrected to try out a suggestion of Andres’s on how to
> de-pessimise it. Unfortunately, it also suffers from Windows-specific
> problems that I originally mentioned at the top of this thread but
> had since repressed. Arrrghgh.
>
> First, the good news:
>
> We could write out a whole new control file, and durable_rename() it
> into place. We don’t want to do that in general, because we don’t
> want to slow down UpdateMinRecoveryPoint(). The new concept is to do
> that only if a backup is in progress. That requires a bit of
> interlocking with backup start/stop (ie when runningBackups is
> changing in shmem, we don’t want to overlap with UpdateControlFile()'s
> decision on how to do it). Here is a patch to try that out. No more
> weasel wording needed for the docs; basebackup and low-level file
> system backup should always see an atomic control file (and
> occasionally also copy a harmless pg_control.tmp file). Then we only
> need the gross retry-until-stable hack for front-end programs.
I like the approach in these patches better than the last patch set. My
only concern would be possible performance regression on standbys (when
doing backup from standby) since pg_control can be written very
frequently to update min recovery point.
I've made a first pass through the patches and they look generally
reasonable (and back patch-able).
One thing:
+ sendFileWithContent(sink, XLOG_CONTROL_FILE,
+ (char *) control_file, sizeof(*control_file),
+ &manifest);
I wonder if we should pad pg_control out to 8k so it remains the same
size as now? Postgres doesn't care, but might look odd to users, and is
arguably a change in behavior that should not be back patched.
> And the bad news:
Provided we can reasonably address the Windows issues this seems to be
the way to go.
Regards,
-David