Re: odd buildfarm failure - "pg_ctl: control file appears to be corrupt"

Greetings,

(Adding David Steele into the CC on this one...)

* Thomas Munro (thomas(dot)munro(at)gmail(dot)com) wrote:
> This is a frustrating thread, because despite the last patch solving
> most of the problems we discussed, it doesn't address the
> low-level-backup procedure in a nice way. We'd have to tell users
> they have to flock that file, or add a new step "pg_controldata --raw
> > pg_control", which seems weird when they already have a connection
> to the server.

flock'ing the file strikes me as dangerous to ask external tools to do
due to the chances of breaking the running system if they don't do it
right. I'm generally open to the idea of having the backup tool have to
do more complicated work to be correct but that just seems likely to
cause issues. Also- haven't looked yet, but I'm not sure that's even
possible if your backup tool is running as a user who only has read
access to the data directory? I don't want us to give up on that
feature.

> Maybe it just doesn't matter if eg the pg_controldata program can
> spuriously fail if pointed at a running system, and I was being too
> dogmatic trying to fix even that. Maybe we should just focus on
> fixing backups. Even there, I am beginning to suspect we are solving
> this problem in the wrong place when a higher level change could
> simplify the problem away.

For a running system.. perhaps pg_controldata should be connecting to
the database and calling functions there? Or just complain that the
system is online and tell the user to do that?

> Idea for future research: Perhaps pg_backup_stop()'s label-file
> output should include the control file image (suitably encoded)? Then
> the recovery-from-label code could completely ignore the existing
> control file, and overwrite it using that copy. It's already
> partially ignoring it, by using the label file's checkpoint LSN
> instead of the control file's. Perhaps the captured copy could
> include the correct LSN already, simplifying that code, and the low
> level backup procedure would not need any additional steps or caveats.
> No more atomicity problem for low-level-backups... but probably not
> something we would back-patch, for such a rare failure mode.

I like this general direction and wonder if we could possibly even push
a bit harder on it: have the backup_label include the control file's
contents in some form that is understood and then tell tools to *not*
copy the running pg_control file ... and maybe even complain if a
pg_control file exists when we detect that backup_label has the control
file's image. We've certainly had problems in the past where people
would just nuke the backup_label file, even though they were restoring
from a backup, because they couldn't start the system up since their
restore command wasn't set up properly or their WAL archive was missing.

Being able to get rid of the control file being in the backup at all
would make it harder for someone to get to a corrupted-but-running
system and that seems like it's a good thing.

> Here's a new minimal patch that solves only the bugs in basebackup +
> the simple SQL-facing functions that read the control file, by simply
> acquiring ControlFileLock in the obvious places. This should be
> simple enough for back-patching?

I don't particularly like fixing this in a way that only works for
pg_basebackup and means that the users of other backup tools don't have
a solution to this problem. What are we supposed to tell users of
pgBackRest when they see this fix for pg_basebackup in the next set of
minor releases and they ask us if we've addressed this risk?

We might be able to accept the 're-read on CRC failure' approach, if it
were being used for pg_controldata and we documented that external
tools and especially backup tools using the low-level API are required
to check the CRC and to re-read on failure if accessing a running
system.

While it's not ideal, maybe we could get away with changing the contents
of the backup_label as part of a back-patch? The documentation, at
least on a quick look, says to copy the second field from pg_backup_stop
into a backup_label file but doesn't say anything about what those
contents are or if they can change. That would at least address the
concern of backups ending up with a corrupt pg_control file and not
being able to be restored, even if tools aren't updated to verify the
CRC or similar. Of course, that's a fair bit of code churn for a
backpatch, which I certainly understand as being risky. If we can't
back-patch that, it might still be the way to go moving forward, while
also telling tools to check the CRC. (I'm not going to try to figure
out some back-patchable pretend solution for this for shell scripts that
pretend to be able to backup running PG databases; this issue is
probably the least of their issues anyway...)

A couple of interesting notes on this though- pgBackRest doesn't only
read the pg_control file at backup time, we also check it at
archive_command time, to make sure that the system ID and version that
are in the control file match up with the information in the WAL file
we're getting ready to archive and that those match with the system ID
and version of the repo/stanza into which we are pushing the WAL file.
We do read the control file on the replica but that isn't the one we
actually push into the repo as part of a backup- that's always the one
we read from the primary (we don't currently support 'backup just from
the replica').

Coming out of our discussion regarding this, we're likely to move
forward on the check-CRC-and-re-read approach for the next pgBackRest
release. If PG provides a better solution for us to use, great, but
given that this has been shown to happen, we're not intending to wait
around for PG to provide us with a better fix.

Thanks,

Stephen