Skip to content

Fix spurious global xmin > xmin error on rollback - #1890

Open
gfphoenix78 wants to merge 1 commit into
apache:mainfrom
gfphoenix78:fix-xid-error
Open

Fix spurious global xmin > xmin error on rollback#1890
gfphoenix78 wants to merge 1 commit into
apache:mainfrom
gfphoenix78:fix-xid-error

Conversation

@gfphoenix78

Copy link
Copy Markdown
Contributor

GetSnapshotData() on a QE computes xmin under ProcArrayLock but reads/advances DistributedLogShared->oldestXmin afterwards, in a different lock domain. In that window a concurrent backend can advance the shared oldestXmin past this snapshot's xmin: when the transaction whose local xid equals xmin aborts, its distributed-log entry keeps distribXid == 0, so DistributedLog_AdvanceOldestXmin()'s forward scan skips over it instead of stopping, bumping oldestXmin beyond xmin. The resulting globalxmin > xmin then tripped a hard elog(ERROR) and failed the query (seen intermittently in the partition_prune regression test).

The aborted xid is invisible to this snapshot anyway, and the visibility horizon only needs to be a valid lower bound, so a horizon at our own xmin loses nothing. Clamp globalxmin to xmin instead of erroring -- always safe, and consistent with the clamp already done in DistributedLog_GetOldestXmin().

Fixes #ISSUE_Number

What does this PR do?

Type of Change

  • Bug fix (non-breaking change)
  • New feature (non-breaking change)
  • Breaking change (fix or feature with breaking changes)
  • Documentation update

Breaking Changes

Test Plan

  • Unit tests added/updated
  • Integration tests added/updated
  • Passed make installcheck
  • Passed make -C src/test installcheck-cbdb-parallel

Impact

Performance:

User-facing changes:

Dependencies:

Checklist

Additional Context

CI Skip Instructions


GetSnapshotData() on a QE computes xmin under ProcArrayLock but
reads/advances DistributedLogShared->oldestXmin afterwards, in a
different lock domain.  In that window a concurrent backend can advance
the shared oldestXmin past this snapshot's xmin: when the transaction
whose local xid equals xmin aborts, its distributed-log entry keeps
distribXid == 0, so DistributedLog_AdvanceOldestXmin()'s forward scan
skips over it instead of stopping, bumping oldestXmin beyond xmin.  The
resulting globalxmin > xmin then tripped a hard elog(ERROR) and failed
the query (seen intermittently in the partition_prune regression test).

The aborted xid is invisible to this snapshot anyway, and the
visibility horizon only needs to be a valid lower bound, so a horizon
at our own xmin loses nothing.  Clamp globalxmin to xmin instead of
erroring -- always safe, and consistent with the clamp already done in
DistributedLog_GetOldestXmin().
@gfphoenix78

Copy link
Copy Markdown
Contributor Author

The problem is:

gpadmin=# create temp table ttmp(a int);
ERROR:  global xmin (1076) is higher than transaction xmin (1075) (procarray.c:3298)  (seg0 127.0.0.1:7002 pid=2466718) (procarray.c:3298)
LINE 1: create temp table ttmp(a int);
                          ^

repro:

-- prepare
drop table if exists t1;

create table t1(a int);

-- T1
1: begin;
1: insert into t1 select i from generate_series(1,10)i;
-- to see the local xid on segments
1: select gp_segment_id, xmin, * from t1;

-- T2, will trigger the error
2: set optimizer=off; -- invoke the QE backend on segments
-- gdb attach to one of the QEs and break after release the ProcArrayLock
-- gdb -p xxxx
-- > b procarray.c:3303
-- > c

-- the following sql will be break when running GetSnapshotData
2: create temp table ttmp(a int);

-- rollback T1
1: rollback;

-- running SQL that advance DistributedLogShared.oldestXmin
1: insert into t1 select i from generate_series(1,10)i;

-- gdb: the QE of T2 should be still hanged in GetSnapshotData
-- quit the gdb now
-- > quit

-- now, the above SQL to create temp table will fail

gdb break the function GetSnapshotData at:

	/*
	 * GP: In computing Globals, also take distributed snapshots into
	 * account.
	 */
	if (TransactionIdPrecedes(xmin, globalxmin))
		globalxmin = xmin;

	if (!IS_QUERY_DISPATCHER())

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants