Re: [PATCH]Fix pg_xact corruption from subtransaction abort after subcommit

Hello!

Bryan Green <dbryan(dot)green(at)gmail(dot)com> writes:
> ...
>     BEGIN;
>     DO $$
>     BEGIN
>         BEGIN
>             INSERT INTO t VALUES (1);   -- subtransaction acquires an XID
>         EXCEPTION WHEN OTHERS THEN
>             NULL;                       -- swallow the commit-time error
>         END;
>     END $$;
>     COMMIT;                             -- crashes here
>
>     TRAP: failed Assert("curval == 0 || ... || curval == status"),
>           File: "clog.c", Line: 702
>         TransactionIdSetStatusBit
>         TransactionIdSetTreeStatus
>         TransactionIdCommitTree
>         RecordTransactionCommit
>
> The fix is small and stays in xact.c.  AtSubCommit_childXids() records the
> parent's child count before it appends, and AbortSubTransaction() restores
> that count when the subtransaction aborts after the transfer.  The entries
> we added are the tail of the parent's array, so restoring the length drops
> exactly them; the grandchildren revert to aborted, which is correct because
> the whole subtree is rolling back.
>
> I confirmed on current master that the reproduction crashes without the
> patch and commits cleanly with it, that the aborted row is gone, that a
> committed sibling savepoint survives, and that the regression tests pass.

Probably this requires to have this test in the patch, reproducing this
issue it's not easy. Using a test that use the callback or an injection
point, don't know if this it's expects to be backported, if that's not
the case probably an injection point plus a regression will be enough.

Related to the code, you added this:

@@ -250,6 +252,7 @@ static TransactionStateData TopTransactionStateData = {
.state = TRANS_DEFAULT,
.blockState = TBLOCK_DEFAULT,
.topXidLogged = false,
+ .savedParentNChildXids = -1,
};

Why not initialize in StartTransaction() alongside with the other
transaction states ?

I was able to confirm this error, but it requires a test that targets
this specific case, that's why I think that the test should be included
here.

Regards,
--
Jonathan Gonzalez V.
EDB
https://www.enterprisedb.com