Remove backwards compat ugliness in snapbuild.c.

In 955a684e040 we fixed a bug in initial snapshot creation. In the
course of which several members of struct SnapBuild were obsoleted. As
SnapBuild is serialized to disk we couldn't change the memory layout.

Unfortunately I subsequently forgot about removing the backward compat
gunk, but luckily Heikki just reminded me.

This commit bumps SNAPBUILD_VERSION, therefore breaking existing
slots (which is fine in a major release).

Author: Andres Freund
Reminded-By: Heikki Linnakangas <[email protected]>
Discussion: https://postgr.es/m/c94be044-818f-15e3-1ad3-7a7ae2dfed0a@iki.fi

diff --git a/src/backend/replication/logical/snapbuild.c b/src/backend/replication/logical/snapbuild.c
index e903e561afc4219446784df778bf957746f95499..752cf2d7dbceab681c2622f3a492acf48bb551b3 100644 (file)
--- a/src/backend/replication/logical/snapbuild.c
+++ b/src/backend/replication/logical/snapbuild.c
@@ -189,24 +189,11 @@ struct SnapBuild
    ReorderBuffer *reorder;
 
    /*
-    * Outdated: This struct isn't used for its original purpose anymore, but
-    * can't be removed / changed in a minor version, because it's stored
-    * on-disk.
+    * TransactionId at which the next phase of initial snapshot building will
+    * happen. InvalidTransactionId if not known (i.e. SNAPBUILD_START), or
+    * when no next phase necessary (SNAPBUILD_CONSISTENT).
     */
-   struct
-   {
-       /*
-        * NB: This field is misused, until a major version can break on-disk
-        * compatibility. See SnapBuildNextPhaseAt() /
-        * SnapBuildStartNextPhaseAt().
-        */
-       TransactionId was_xmin;
-       TransactionId was_xmax;
-
-       size_t      was_xcnt;   /* number of used xip entries */
-       size_t      was_xcnt_space; /* allocated size of xip */
-       TransactionId *was_xip; /* running xacts array, xidComparator-sorted */
-   }           was_running;
+   TransactionId next_phase_at;
 
    /*
     * Array of transactions which could have catalog changes that committed
@@ -272,34 +259,6 @@ static void SnapBuildWaitSnapshot(xl_running_xacts *running, TransactionId cutof
 static void SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn);
 static bool SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn);
 
-/*
- * Return TransactionId after which the next phase of initial snapshot
- * building will happen.
- */
-static inline TransactionId
-SnapBuildNextPhaseAt(SnapBuild *builder)
-{
-   /*
-    * For backward compatibility reasons this has to be stored in the wrongly
-    * named field.  Will be fixed in next major version.
-    */
-   return builder->was_running.was_xmax;
-}
-
-/*
- * Set TransactionId after which the next phase of initial snapshot building
- * will happen.
- */
-static inline void
-SnapBuildStartNextPhaseAt(SnapBuild *builder, TransactionId at)
-{
-   /*
-    * For backward compatibility reasons this has to be stored in the wrongly
-    * named field.  Will be fixed in next major version.
-    */
-   builder->was_running.was_xmax = at;
-}
-
 /*
  * Allocate a new snapshot builder.
  *
@@ -728,7 +687,7 @@ SnapBuildProcessChange(SnapBuild *builder, TransactionId xid, XLogRecPtr lsn)
     * we got into the SNAPBUILD_FULL_SNAPSHOT state.
     */
    if (builder->state < SNAPBUILD_CONSISTENT &&
-       TransactionIdPrecedes(xid, SnapBuildNextPhaseAt(builder)))
+       TransactionIdPrecedes(xid, builder->next_phase_at))
        return false;
 
    /*
@@ -945,7 +904,7 @@ SnapBuildCommitTxn(SnapBuild *builder, XLogRecPtr lsn, TransactionId xid,
     */
    if (builder->state == SNAPBUILD_START ||
        (builder->state == SNAPBUILD_BUILDING_SNAPSHOT &&
-        TransactionIdPrecedes(xid, SnapBuildNextPhaseAt(builder))))
+        TransactionIdPrecedes(xid, builder->next_phase_at)))
    {
        /* ensure that only commits after this are getting replayed */
        if (builder->start_decoding_at <= lsn)
@@ -1267,7 +1226,7 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn
        Assert(TransactionIdIsNormal(builder->xmax));
 
        builder->state = SNAPBUILD_CONSISTENT;
-       SnapBuildStartNextPhaseAt(builder, InvalidTransactionId);
+       builder->next_phase_at = InvalidTransactionId;
 
        ereport(LOG,
                (errmsg("logical decoding found consistent point at %X/%X",
@@ -1299,7 +1258,7 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn
    else if (builder->state == SNAPBUILD_START)
    {
        builder->state = SNAPBUILD_BUILDING_SNAPSHOT;
-       SnapBuildStartNextPhaseAt(builder, running->nextXid);
+       builder->next_phase_at = running->nextXid;
 
        /*
         * Start with an xmin/xmax that's correct for future, when all the
@@ -1331,11 +1290,11 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn
     * be decoded.  Switch to FULL_SNAPSHOT.
     */
    else if (builder->state == SNAPBUILD_BUILDING_SNAPSHOT &&
-            TransactionIdPrecedesOrEquals(SnapBuildNextPhaseAt(builder),
+            TransactionIdPrecedesOrEquals(builder->next_phase_at,
                                           running->oldestRunningXid))
    {
        builder->state = SNAPBUILD_FULL_SNAPSHOT;
-       SnapBuildStartNextPhaseAt(builder, running->nextXid);
+       builder->next_phase_at = running->nextXid;
 
        ereport(LOG,
                (errmsg("logical decoding found initial consistent point at %X/%X",
@@ -1356,11 +1315,11 @@ SnapBuildFindSnapshot(SnapBuild *builder, XLogRecPtr lsn, xl_running_xacts *runn
     * collected.  Switch to CONSISTENT.
     */
    else if (builder->state == SNAPBUILD_FULL_SNAPSHOT &&
-            TransactionIdPrecedesOrEquals(SnapBuildNextPhaseAt(builder),
+            TransactionIdPrecedesOrEquals(builder->next_phase_at,
                                           running->oldestRunningXid))
    {
        builder->state = SNAPBUILD_CONSISTENT;
-       SnapBuildStartNextPhaseAt(builder, InvalidTransactionId);
+       builder->next_phase_at = InvalidTransactionId;
 
        ereport(LOG,
                (errmsg("logical decoding found consistent point at %X/%X",
@@ -1463,7 +1422,7 @@ typedef struct SnapBuildOnDisk
    offsetof(SnapBuildOnDisk, version)
 
 #define SNAPBUILD_MAGIC 0x51A1E001
-#define SNAPBUILD_VERSION 2
+#define SNAPBUILD_VERSION 3
 
 /*
  * Store/Load a snapshot from disk, depending on the snapshot builder's state.
@@ -1508,6 +1467,9 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
    if (builder->state < SNAPBUILD_CONSISTENT)
        return;
 
+   /* consistent snapshots have no next phase */
+   Assert(builder->next_phase_at == InvalidTransactionId);
+
    /*
     * We identify snapshots by the LSN they are valid for. We don't need to
     * include timelines in the name as each LSN maps to exactly one timeline
@@ -1596,9 +1558,6 @@ SnapBuildSerialize(SnapBuild *builder, XLogRecPtr lsn)
                &ondisk->builder,
                sizeof(SnapBuild));
 
-   /* there shouldn't be any running xacts */
-   Assert(builder->was_running.was_xcnt == 0);
-
    /* copy committed xacts */
    sz = sizeof(TransactionId) * builder->committed.xcnt;
    memcpy(ondisk_c, builder->committed.xip, sz);
@@ -1801,34 +1760,6 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
    }
    COMP_CRC32C(checksum, &ondisk.builder, sizeof(SnapBuild));
 
-   /* restore running xacts (dead, but kept for backward compat) */
-   sz = sizeof(TransactionId) * ondisk.builder.was_running.was_xcnt_space;
-   ondisk.builder.was_running.was_xip =
-       MemoryContextAllocZero(builder->context, sz);
-   pgstat_report_wait_start(WAIT_EVENT_SNAPBUILD_READ);
-   readBytes = read(fd, ondisk.builder.was_running.was_xip, sz);
-   pgstat_report_wait_end();
-   if (readBytes != sz)
-   {
-       int         save_errno = errno;
-
-       CloseTransientFile(fd);
-
-       if (readBytes < 0)
-       {
-           errno = save_errno;
-           ereport(ERROR,
-                   (errcode_for_file_access(),
-                    errmsg("could not read file \"%s\": %m", path)));
-       }
-       else
-           ereport(ERROR,
-                   (errcode(ERRCODE_DATA_CORRUPTED),
-                    errmsg("could not read file \"%s\": read %d of %zu",
-                           path, readBytes, sz)));
-   }
-   COMP_CRC32C(checksum, ondisk.builder.was_running.was_xip, sz);
-
    /* restore committed xacts information */
    sz = sizeof(TransactionId) * ondisk.builder.committed.xcnt;
    ondisk.builder.committed.xip = MemoryContextAllocZero(builder->context, sz);
@@ -1890,6 +1821,8 @@ SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
    if (TransactionIdPrecedes(ondisk.builder.xmin, builder->initial_xmin_horizon))
        goto snapshot_not_interesting;
 
+   /* consistent snapshots have no next phase */
+   Assert(ondisk.builder.next_phase_at == InvalidTransactionId);
 
    /* ok, we think the snapshot is sensible, copy over everything important */
    builder->xmin = ondisk.builder.xmin;