Improve handling of corrupted two-phase state files at recovery

When a corrupted two-phase state file is found by WAL replay, be it for
crash recovery or archive recovery, then the file is simply skipped and
a WARNING is logged to the user, causing the transaction to be silently
lost.  Facing an on-disk WAL file which is corrupted is as likely to
happen as what is stored in WAL records, but WAL records are already
able to fail hard if there is a CRC mismatch.  On-disk two-phase state
files, on the contrary, are simply ignored if corrupted.  Note that when
restoring the initial two-phase data state at recovery, files newer than
the horizon XID are discarded hence no files present in pg_twophase/
should be torned and have been made durable by a previous checkpoint, so
recovery should never see any corrupted two-phase state file by design.

The situation got better since 978b2f6 which has added two-phase state
information directly in WAL instead of using on-disk files, so the risk
is limited to two-phase transactions which live across at least one
checkpoint for long periods.  Backups having legit two-phase state files
on-disk could also lose silently transactions when restored if things
get corrupted.

This behavior exists since two-phase commit has been introduced, no
back-patch is done for now per the lack of complaints about this
problem.

Author: Michael Paquier
Discussion: https://postgr.es/m/20180709050309.GM1467@paquier.xyz

diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 4aff6cf7f278965d73019f7cf9ddb3ea813994ee..c06bbe72646c1896f8db7b715dd0c03c30fedc3e 100644 (file)
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1207,10 +1207,12 @@ RegisterTwoPhaseRecord(TwoPhaseRmgrId rmid, uint16 info,
  * Read and validate the state file for xid.
  *
  * If it looks OK (has a valid magic number and CRC), return the palloc'd
- * contents of the file.  Otherwise return NULL.
+ * contents of the file, issuing an error when finding corrupted data.  If
+ * missing_ok is true, which indicates that missing files can be safely
+ * ignored, then return NULL.  This state can be reached when doing recovery.
  */
 static char *
-ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
+ReadTwoPhaseFile(TransactionId xid, bool missing_ok)
 {
    char        path[MAXPGPATH];
    char       *buf;
@@ -1227,11 +1229,12 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
    fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
    if (fd < 0)
    {
-       if (give_warnings)
-           ereport(WARNING,
-                   (errcode_for_file_access(),
-                    errmsg("could not open file \"%s\": %m", path)));
-       return NULL;
+       if (missing_ok && errno == ENOENT)
+           return NULL;
+
+       ereport(ERROR,
+               (errcode_for_file_access(),
+                errmsg("could not open file \"%s\": %m", path)));
    }
 
    /*
@@ -1241,35 +1244,27 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
     * even on a valid file.
     */
    if (fstat(fd, &stat))
-   {
-       int         save_errno = errno;
-
-       CloseTransientFile(fd);
-       if (give_warnings)
-       {
-           errno = save_errno;
-           ereport(WARNING,
-                   (errcode_for_file_access(),
-                    errmsg("could not stat file \"%s\": %m", path)));
-       }
-       return NULL;
-   }
+       ereport(ERROR,
+               (errcode_for_file_access(),
+                errmsg("could not stat file \"%s\": %m", path)));
 
    if (stat.st_size < (MAXALIGN(sizeof(TwoPhaseFileHeader)) +
                        MAXALIGN(sizeof(TwoPhaseRecordOnDisk)) +
                        sizeof(pg_crc32c)) ||
        stat.st_size > MaxAllocSize)
-   {
-       CloseTransientFile(fd);
-       return NULL;
-   }
+       ereport(ERROR,
+               (errcode(ERRCODE_DATA_CORRUPTED),
+                errmsg_plural("incorrect size of file \"%s\": %zu byte",
+                              "incorrect size of file \"%s\": %zu bytes",
+                              (Size) stat.st_size, path,
+                              (Size) stat.st_size)));
 
    crc_offset = stat.st_size - sizeof(pg_crc32c);
    if (crc_offset != MAXALIGN(crc_offset))
-   {
-       CloseTransientFile(fd);
-       return NULL;
-   }
+       ereport(ERROR,
+               (errcode(ERRCODE_DATA_CORRUPTED),
+                errmsg("incorrect alignment of CRC offset for file \"%s\"",
+                       path)));
 
    /*
     * OK, slurp in the file.
@@ -1280,37 +1275,31 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
    r = read(fd, buf, stat.st_size);
    if (r != stat.st_size)
    {
-       int         save_errno = errno;
-
-       pgstat_report_wait_end();
-       CloseTransientFile(fd);
-       if (give_warnings)
-       {
-           if (r < 0)
-           {
-               errno = save_errno;
-               ereport(WARNING,
-                       (errcode_for_file_access(),
-                        errmsg("could not read file \"%s\": %m", path)));
-           }
-           else
-               ereport(WARNING,
-                       (errmsg("could not read file \"%s\": read %d of %zu",
-                               path, r, (Size) stat.st_size)));
-       }
-       pfree(buf);
-       return NULL;
+       if (r < 0)
+           ereport(ERROR,
+                   (errcode_for_file_access(),
+                    errmsg("could not read file \"%s\": %m", path)));
+       else
+           ereport(ERROR,
+                   (errmsg("could not read file \"%s\": read %d of %zu",
+                           path, r, (Size) stat.st_size)));
    }
 
    pgstat_report_wait_end();
    CloseTransientFile(fd);
 
    hdr = (TwoPhaseFileHeader *) buf;
-   if (hdr->magic != TWOPHASE_MAGIC || hdr->total_len != stat.st_size)
-   {
-       pfree(buf);
-       return NULL;
-   }
+   if (hdr->magic != TWOPHASE_MAGIC)
+       ereport(ERROR,
+               (errcode(ERRCODE_DATA_CORRUPTED),
+                errmsg("invalid magic number stored in file \"%s\"",
+                       path)));
+
+   if (hdr->total_len != stat.st_size)
+       ereport(ERROR,
+               (errcode(ERRCODE_DATA_CORRUPTED),
+                errmsg("invalid size stored in file \"%s\"",
+                       path)));
 
    INIT_CRC32C(calc_crc);
    COMP_CRC32C(calc_crc, buf, crc_offset);
@@ -1319,10 +1308,10 @@ ReadTwoPhaseFile(TransactionId xid, bool give_warnings)
    file_crc = *((pg_crc32c *) (buf + crc_offset));
 
    if (!EQ_CRC32C(calc_crc, file_crc))
-   {
-       pfree(buf);
-       return NULL;
-   }
+       ereport(ERROR,
+               (errcode(ERRCODE_DATA_CORRUPTED),
+                errmsg("calculated CRC checksum does not match value stored in file \"%s\"",
+                       path)));
 
    return buf;
 }
@@ -1431,7 +1420,7 @@ StandbyTransactionIdIsPrepared(TransactionId xid)
        return false;           /* nothing to do */
 
    /* Read and validate file */
-   buf = ReadTwoPhaseFile(xid, false);
+   buf = ReadTwoPhaseFile(xid, true);
    if (buf == NULL)
        return false;
 
@@ -1479,7 +1468,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
     * to disk if for some reason they have lived for a long time.
     */
    if (gxact->ondisk)
-       buf = ReadTwoPhaseFile(xid, true);
+       buf = ReadTwoPhaseFile(xid, false);
    else
        XlogReadTwoPhaseData(gxact->prepare_start_lsn, &buf, NULL);
 
@@ -1874,6 +1863,10 @@ restoreTwoPhaseData(void)
  * write a WAL entry, and so there might be no evidence in WAL of those
  * subxact XIDs.
  *
+ * On corrupted two-phase files, fail immediately.  Keeping around broken
+ * entries and let replay continue causes harm on the system, and a new
+ * backup should be rolled in.
+ *
  * Our other responsibility is to determine and return the oldest valid XID
  * among the prepared xacts (if none, return ShmemVariableCache->nextXid).
  * This is needed to synchronize pg_subtrans startup properly.
@@ -2164,15 +2157,7 @@ ProcessTwoPhaseBuffer(TransactionId xid,
    if (fromdisk)
    {
        /* Read and validate file */
-       buf = ReadTwoPhaseFile(xid, true);
-       if (buf == NULL)
-       {
-           ereport(WARNING,
-                   (errmsg("removing corrupt two-phase state file for transaction %u",
-                           xid)));
-           RemoveTwoPhaseFile(xid, true);
-           return NULL;
-       }
+       buf = ReadTwoPhaseFile(xid, false);
    }
    else
    {
@@ -2185,21 +2170,15 @@ ProcessTwoPhaseBuffer(TransactionId xid,
    if (!TransactionIdEquals(hdr->xid, xid))
    {
        if (fromdisk)
-       {
-           ereport(WARNING,
-                   (errmsg("removing corrupt two-phase state file for transaction %u",
+           ereport(ERROR,
+                   (errcode(ERRCODE_DATA_CORRUPTED),
+                    errmsg("corrupted two-phase state file for transaction \"%u\"",
                            xid)));
-           RemoveTwoPhaseFile(xid, true);
-       }
        else
-       {
-           ereport(WARNING,
-                   (errmsg("removing corrupt two-phase state from memory for transaction %u",
+           ereport(ERROR,
+                   (errcode(ERRCODE_DATA_CORRUPTED),
+                    errmsg("corrupted two-phase state in memory for transaction \"%u\"",
                            xid)));
-           PrepareRedoRemove(xid, true);
-       }
-       pfree(buf);
-       return NULL;
    }
 
    /*