8000 Rework handling of subtransactions in 2PC recovery · postgrespro/postgres_cluster@49e9281 · GitHub
[go: up one dir, main page]
Skip to content

Commit 49e9281

Browse files
simonat2ndQuadrantsimonat2ndQuadrant
committed
Rework handling of subtransactions in 2PC recovery
The bug fixed by 0874d4f caused us to question and rework the handling of subtransactions in 2PC during and at end of recovery. Patch adds checks and tests to ensure no further bugs. This effectively removes the temporary measure put in place by 546c13e. Author: Simon Riggs Reviewed-by: Tom Lane, Michael Paquier Discussion: http://postgr.es/m/CANP8+j+vvXmruL_i2buvdhMeVv5TQu0Hm2+C5N+kdVwHJuor8w@mail.gmail.com
1 parent 0352c15 commit 49e9281

File tree

7 files changed

+56
-55
lines changed
  • src
    • backend
      • access/transam
  • storage/ipc
  • include/access

    • 7 files changed

    +56
    -55
    lines changed

    ‎src/backend/access/transam/subtrans.c‎

    Lines changed: 22 additions & 10 deletions
    Original file line numberDiff line numberDiff line change
    @@ -68,32 +68,35 @@ static bool SubTransPagePrecedes(int page1, int page2);
    6868

    6969
    /*
    7070
    * Record the parent of a subtransaction in the subtrans log.
    71-
    *
    72-
    * In some cases we may need to overwrite an existing value.
    7371
    */
    7472
    void
    75-
    SubTransSetParent(TransactionId xid, TransactionId parent, bool overwriteOK)
    73+
    SubTransSetParent(TransactionId xid, TransactionId parent)
    7674
    {
    7775
    int pageno = TransactionIdToPage(xid);
    7876
    int entryno = TransactionIdToEntry(xid);
    7977
    int slotno;
    8078
    TransactionId *ptr;
    8179

    8280
    Assert(TransactionIdIsValid(parent));
    81+
    Assert(TransactionIdFollows(xid, parent));
    8382

    8483
    LWLockAcquire(SubtransControlLock, LW_EXCLUSIVE);
    8584

    8685
    slotno = SimpleLruReadPage(SubTransCtl, pageno, true, xid);
    8786
    ptr = (TransactionId *) SubTransCtl->shared->page_buffer[slotno];
    8887
    ptr += entryno;
    8988

    90-
    /* Current state should be 0 */
    91-
    Assert(*ptr == InvalidTransactionId ||
    92-
    (*ptr == parent && overwriteOK));
    93-
    94-
    *ptr = parent;
    95-
    96-
    SubTransCtl->shared->page_dirty[slotno] = true;
    89+
    /*
    90+
    * It's possible we'll try to set the parent xid multiple times
    91+
    * but we shouldn't ever be changing the xid from one valid xid
    92+
    * to another valid xid, which would corrupt the data structure.
    93+
    */
    94+
    if (*ptr != parent)
    95+
    {
    96+
    Assert(*ptr == InvalidTransactionId);
    97+
    *ptr = parent;
    98+
    SubTransCtl->shared->page_dirty[slotno] = true;
    99+
    }
    97100

    98101
    LWLockRelease(SubtransControlLock);
    99102
    }
    @@ -157,6 +160,15 @@ SubTransGetTopmostTransaction(TransactionId xid)
    157160
    if (TransactionIdPrecedes(parentXid, TransactionXmin))
    158161
    break;
    159162
    parentXid = SubTransGetParent(parentXid);
    163+
    164+
    /*
    165+
    * By convention the parent xid gets allocated first, so should
    166+
    * always precede the child xid. Anything else points to a corrupted
    167+
    * data structure that could lead to an infinite loop, so exit.
    168+
    */
    169+
    if (!TransactionIdPrecedes(parentXid, previousXid))
    170+
    elog(ERROR, "pg_subtrans contains invalid entry: xid %u points to parent xid %u",
    171+
    previousXid, parentXid);
    160172
    }
    161173

    162174
    Assert(TransactionIdIsValid(previousXid));

    ‎src/backend/access/transam/twophase.c‎

    Lines changed: 28 additions & 39 deletions
    Original file line numberDiff line numberDiff line change
    @@ -221,8 +221,7 @@ static void RemoveGXact(GlobalTransaction gxact);
    221221
    static void XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len);
    222222
    static char *ProcessTwoPhaseBuffer(TransactionId xid,
    223223
    XLogRecPtr prepare_start_lsn,
    224-
    bool fromdisk, bool overwriteOK, bool setParent,
    225-
    bool setNextXid);
    224+
    bool fromdisk, bool setParent, bool setNextXid);
    226225
    static void MarkAsPreparingGuts(GlobalTransaction gxact, TransactionId xid,
    227226
    const char *gid, TimestampTz prepared_at, Oid owner,
    228227
    Oid databaseid);
    @@ -1743,8 +1742,7 @@ restoreTwoPhaseData(void)
    17431742
    xid = (TransactionId) strtoul(clde->d_name, NULL, 16);
    17441743

    17451744
    buf = ProcessTwoPhaseBuffer(xid, InvalidXLogRecPtr,
    1746-
    true, false, false,
    1747-
    false);
    1745+
    true, false, false);
    17481746
    if (buf == NULL)
    17491747
    continue;
    17501748

    @@ -1804,8 +1802,7 @@ PrescanPreparedTransactions(TransactionId **xids_p, int *nxids_p)
    18041802

    18051803
    buf = ProcessTwoPhaseBuffer(xid,
    18061804
    gxact->prepare_start_lsn,
    1807-
    gxact->ondisk, false, false,
    1808-
    true);
    1805+
    gxact->ondisk, false, true);
    18091806

    18101807
    if (buf == NULL)
    18111808
    continue;
    @@ -1858,12 +1855,12 @@ PrescanPreparedTransactions(TransactionId **xids_p, int *nxids_p)
    18581855
    * This is never called at the end of recovery - we use
    18591856
    * RecoverPreparedTransactions() at that point.
    18601857
    *
    1861-
    * Currently we simply call SubTransSetParent() for any subxids of prepared
    1862-
    * transactions. If overwriteOK is true, it's OK if some XIDs have already
    1863-
    * been marked in pg_subtrans.
    1858+
    * The lack of calls to SubTransSetParent() calls here is by design;
    1859+
    * those calls are made by RecoverPreparedTransactions() at the end of recovery
    1860+
    * for those xacts that need this.
    18641861
    */
    18651862
    void
    1866-
    StandbyRecoverPreparedTransactions(bool overwriteOK)
    1863+
    StandbyRecoverPreparedTransactions(void)
    18671864
    {
    18681865
    int i;
    18691866

    @@ -1880,8 +1877,7 @@ StandbyRecoverPreparedTransactions(bool overwriteOK)
    18801877

    18811878
    buf = ProcessTwoPhaseBuffer(xid,
    18821879
    gxact->prepare_start_lsn,
    1883-
    gxact->ondisk, overwriteOK, true,
    1884-
    false);
    1880+
    gxact->ondisk, false, false);
    18851881
    if (buf != NULL)
    18861882
    pfree(buf);
    18871883
    }
    @@ -1895,6 +1891,13 @@ StandbyRecoverPreparedTransactions(bool overwriteOK)
    18951891
    * each prepared transaction (reacquire locks, etc).
    18961892
    *
    18971893
    * This is run during database startup.
    1894+
    *
    1895+
    * At the end of recovery the way we take snapshots will change. We now need
    1896+
    * to mark all running transactions with their full SubTransSetParent() in A3E2 fo
    1897+
    * to allow normal snapshots to work correctly if snapshots overflow.
    1898+
    * We do this here because by definition prepared transactions are the only
    1899+
    * type of write transaction still running, so this is necessary and
    1900+
    * complete.
    18981901
    */
    18991902
    void
    19001903
    RecoverPreparedTransactions(void)
    @@ -1913,15 +1916,21 @@ RecoverPreparedTransactions(void)
    19131916
    TwoPhaseFileHeader *hdr;
    19141917
    TransactionId *subxids;
    19151918
    const char *gid;
    1916-
    bool overwriteOK = false;
    1917-
    int i;
    19181919

    19191920
    xid = gxact->xid;
    19201921

    1922+
    /*
    1923+
    * Reconstruct subtrans state for the transaction --- needed
    1924+
    * because pg_subtrans is not preserved over a restart. Note that
    1925+
    * we are linking all the subtransactions directly to the
    1926+
    * top-level XID; there may originally have been a more complex
    1927+
    * hierarchy, but there's no need to restore that exactly.
    1928+
    * It's possible that SubTransSetParent has been set before, if
    1929+
    * the prepared transaction generated xid assignment records.
    1930+
    */
    19211931
    buf = ProcessTwoPhaseBuffer(xid,
    19221932
    gxact->prepare_start_lsn,
    1923-
    gxact->ondisk, false, false,
    1924-
    false);
    1933+
    gxact->ondisk, true, false);
    19251934
    if (buf == NULL)
    19261935
    continue;
    19271936

    @@ -1939,25 +1948,6 @@ RecoverPreparedTransactions(void)
    19391948
    bufptr += MAXALIGN(hdr->nabortrels * sizeof(RelFileNode));
    19401949
    bufptr += MAXALIGN(hdr->ninvalmsgs * sizeof(SharedInvalidationMessage));
    19411950

    1942-
    /*
    1943-
    * It's possible that SubTransSetParent has been set before, if
    1944-
    * the prepared transaction generated xid assignment records. Test
    1945-
    * here must match one used in AssignTransactionId().
    1946-
    */
    1947-
    if (InHotStandby && (hdr->nsubxacts >= PGPROC_MAX_CACHED_SUBXIDS ||
    1948-
    XLogLogicalInfoActive()))
    1949-
    overwriteOK = true;
    1950-
    1951-
    /*
    1952-
    * Reconstruct subtrans state for the transaction --- needed
    1953-
    * because pg_subtrans is not preserved over a restart. Note that
    1954-
    * we are linking all the subtransactions directly to the
    1955-
    * top-level XID; there may originally have been a more complex
    1956-
    * hierarchy, but there's no need to restore that exactly.
    1957-
    */
    1958-
    for (i = 0; i < hdr->nsubxacts; i++)
    1959-
    SubTransSetParent(subxids[i], xid, true);
    1960-
    19611951
    /*
    19621952
    * Recreate its GXACT and dummy PGPROC. But, check whether
    19631953
    * it was added in redo and already has a shmem entry for
    @@ -2006,16 +1996,15 @@ RecoverPreparedTransactions(void)
    20061996
    * Given a transaction id, read it either from disk or read it directly
    20071997
    * via shmem xlog record pointer using the provided "prepare_start_lsn".
    20081998
    *
    2009-
    * If setParent is true, then use the overwriteOK parameter to set up
    2010-
    * subtransaction parent linkages.
    1999+
    * If setParent is true, set up subtransaction parent linkages.
    20112000
    *
    20122001
    * If setNextXid is true, set ShmemVariableCache->nextXid to the newest
    20132002
    * value scanned.
    20142003
    */
    20152004
    static char *
    20162005
    ProcessTwoPhaseBuffer(TransactionId xid,
    20172006
    XLogRecPtr prepare_start_lsn,
    2018-
    bool fromdisk, bool overwriteOK,
    2007+
    bool fromdisk,
    20192008
    bool setParent, bool setNextXid)
    20202009
    {
    20212010
    TransactionId origNextXid = ShmemVariableCache->nextXid;
    @@ -2142,7 +2131,7 @@ ProcessTwoPhaseBuffer(TransactionId xid,
    21422131
    }
    21432132

    21442133
    if (setParent)
    2145-
    SubTransSetParent(subxid, xid, overwriteOK);
    2134+
    SubTransSetParent(subxid, xid);
    21462135
    }
    21472136

    21482137
    return buf;

    ‎src/backend/access/transam/xact.c‎

    Lines changed: 1 addition & 1 deletion
    Original file line numberDiff line numberDiff line change
    @@ -559,7 +559,7 @@ AssignTransactionId(TransactionState s)
    559559
    XactTopTransactionId = s->transactionId;
    560560

    561561
    if (isSubXact)
    562-
    SubTransSetParent(s->transactionId, s->parent->transactionId, false);
    562+
    SubTransSetParent(s->transactionId, s->parent->transactionId);
    563563

    564564
    /*
    565565
    * If it's a top-level transaction, the predicate locking system needs to

    ‎src/backend/access/transam/xlog.c‎

    Lines changed: 2 additions & 2 deletions
    Original file line numberDiff line numberDiff line change
    @@ -6930,7 +6930,7 @@ StartupXLOG(void)
    69306930

    69316931
    ProcArrayApplyRecoveryInfo(&running);
    69326932

    6933-
    StandbyRecoverPreparedTransactions(false);
    6933+
    StandbyRecoverPreparedTransactions();
    69346934
    }
    69356935
    }
    69366936

    @@ -9692,7 +9692,7 @@ xlog_redo(XLogReaderState *record)
    96929692

    96939693
    ProcArrayApplyRecoveryInfo(&running);
    96949694

    9695-
    StandbyRecoverPreparedTransactions(true);
    9695+
    StandbyRecoverPreparedTransactions();
    96969696
    }
    96979697

    96989698
    /* ControlFile->checkPointCopy always tracks the latest ckpt XID */

    ‎src/backend/storage/ipc/procarray.c‎

    Lines changed: 1 addition & 1 deletion
    Original file line numberDiff line numberDiff line change
    @@ -943,7 +943,7 @@ ProcArrayApplyXidAssignment(TransactionId topxid,
    943943
    * have attempted to SubTransSetParent().
    944944
    */
    945945
    for (i = 0; i < nsubxids; i++)
    946-
    SubTransSetParent(subxids[i], topxid, false);
    946+
    SubTransSetParent(subxids[i], topxid);
    947947

    948948
    /* KnownAssignedXids isn't maintained yet, so we're done for now */
    949949
    if (standbyState == STANDBY_INITIALIZED)

    ‎src/include/access/subtrans.h‎

    Lines changed: 1 addition & 1 deletion
    Original file line numberDiff line numberDiff line change
    @@ -14,7 +14,7 @@
    1414
    /* Number of SLRU buffers to use for subtrans */
    1515
    #define NUM_SUBTRANS_BUFFERS 32
    1616

    17-
    extern void SubTransSetParent(TransactionId xid, TransactionId parent, bool overwriteOK);
    17+
    extern void SubTransSetParent(TransactionId xid, TransactionId parent);
    1818
    extern TransactionId SubTransGetParent(TransactionId xid);
    1919
    extern TransactionId SubTransGetTopmostTransaction(TransactionId xid);
    2020

    ‎src/include/access/twophase.h‎

    Lines changed: 1 addition & 1 deletion
    Original file line numberDiff line numberDiff line change
    @@ -46,7 +46,7 @@ extern bool StandbyTransactionIdIsPrepared(TransactionId xid);
    4646

    4747
    extern TransactionId PrescanPreparedTransactions(TransactionId **xids_p,
    4848
    int *nxids_p);
    49-
    extern void StandbyRecoverPreparedTransactions(bool overwriteOK);
    49+
    extern void StandbyRecoverPreparedTransactions(void);
    5050
    extern void RecoverPreparedTransactions(void);
    5151

    5252
    extern void CheckPointTwoPhase(XLogRecPtr redo_horizon);

    0 commit comments

    Comments
     (0)
    0