Fix distributed snapshot race during commit - #1884
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hi, @hbswcsyzx welcome!🎊 Thanks for taking the effort to make our project better! 🙌 Keep making such awesome contributions!
There was a problem hiding this comment.
Pull request overview
This PR addresses a distributed snapshot visibility race during distributed transaction commit by changing the ordering of GXID/proc-array cleanup vs. QE commit notifications, and adds an isolation2 regression test to reproduce/guard the behavior (issue #1465). In addition, the PR includes a broad set of build/dependency/licensing/documentation updates that are not described in the PR’s stated “fix is contained in …” scope.
Changes:
- Adjust distributed transaction commit notification ordering so the coordinator clears the distributed GXID from the proc array before dispatching COMMIT/COMMIT PREPARED (retaining a local copy for retries/diagnostics).
- Add a new isolation2 test + expected output and register it in the isolation2 schedule for issue #1465.
- Update a variety of build scripts/configuration and add/adjust ASF licensing headers and related files.
Reviewed changes
Copilot reviewed 84 out of 98 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/backend/cdb/cdbtm.c | Clears proc-array GXID earlier in commit paths; retains GXID locally for later DTX commands/diagnostics. |
| src/include/cdb/cdbtm.h | Adds dtxGxid field to retain the QD GXID after proc-array cleanup. |
| src/test/isolation2/sql/issue1465_distributed_snapshot.sql | New isolation test reproducing the distributed snapshot/proc-array race. |
| src/test/isolation2/expected/issue1465_distributed_snapshot.out | Expected output for the new isolation test. |
| src/test/isolation2/isolation2_schedule | Registers the new issue1465 isolation test in the schedule. |
| src/include/crypto/bufenc.h | Adds ASF license header block to file header comment. |
| src/common/sm3.c | Adds ASF license header block. |
| SECURITY.md | Adds ASF license header comment block. |
| README.md | Adds ASF license header comment block. |
| README.apache.md | Adds ASF license header comment block. |
| CONTRIBUTING.md | Adds ASF license header comment block. |
| CODE_OF_CONDUCT.md | Adds ASF license header comment block. |
| deploy/README.md | Adds ASF license header comment block. |
| deploy/build/README.Ubuntu.bash | Deletes build helper script. |
| deploy/build/README.Rhel-Rocky.bash | Deletes build helper script. |
| deploy/build/README.md | Deletes build documentation. |
| deploy/build/README.macOS.md | Deletes build documentation. |
| deploy/build/README.macOS.bash | Deletes build helper script. |
| deploy/build/README.Linux.md | Deletes build documentation. |
| deploy/build/README.CentOS.bash | Deletes build helper script. |
| python-dependencies.txt | Bumps PyYAML pinned version to 5.4.1. |
| gpMgmt/bin/Makefile | Reworks python dependency handling to download sources at build time; adjusts pyyaml/mock build steps. |
| gpMgmt/bin/pythonSrc/ext/.gitkeep | Adds placeholder for pip/curl-downloaded Python source archives. |
| gpMgmt/bin/.gitignore | Ignores downloaded python source artifacts under pythonSrc/ext. |
| gpMgmt/test/README | Updates doc text to reflect behave now installed via pip instead of vendored docs. |
| gpMgmt/bin/gpshrink | Adds ASF license header block. |
| gpMgmt/bin/gpdemo | Adds ASF license header block. |
| gpMgmt/bin/generate-greenplum-path.sh | Adds “greenplum naming compatibility” notice block. |
| gpAux/gpdemo/gpdemo-defaults.sh | Adds ASF license header block. |
| gpAux/client/scripts/greenplum_clients_path.sh | Adds “greenplum naming compatibility” notice block. |
| gpAux/client/install/src/windows/CreatePackage.bat | Switches PyYAML installation method to pip-based install. |
| configure.ac | Updates version string; adds AX_COMPARE_VERSION; adds PAX and pythonsrc-ext dependency checks. |
| configure | Regenerated configure script reflecting configure.ac changes (includes PAX dependency/version checks). |
| config/ax_compare_version.m4 | Adds Autoconf Archive AX_COMPARE_VERSION macro file. |
| pom.xml | Updates project version string; adjusts RAT exclude list organization/entries. |
| NOTICE | Simplifies NOTICE contents (removes prior third-party attribution text). |
| LICENSE | Reorganizes/updates third-party license attribution sections. |
| licenses/LICENSE-setuptools.txt | Deletes bundled license text file. |
| licenses/LICENSE-pyyaml.txt | Deletes bundled license text file. |
| licenses/LICENSE-pygresql.txt | Deletes bundled license text file. |
| licenses/LICENSE-bsd3-psutil.txt | Deletes bundled license text file. |
| licenses/LICENSE-bsd2-mock.txt | Deletes bundled license text file. |
| licenses/LICENSE-bsd2-behave.txt | Deletes bundled license text file. |
| .gitmodules | Removes cpp-stub git submodule entry. |
| contrib/pgcrypto/sm4.h | Adds ASF license header block. |
| contrib/pax_storage/tools/gen_sql.c | Replaces // comment header with ASF-style block header. |
| contrib/pax_storage/src/cpp/storage/README_CTID_in_pax.md | Adds ASF license header comment block. |
| contrib/pax_storage/src/cpp/storage/proto/pax.proto | Adds ASF license header block comment. |
| contrib/pax_storage/src/cpp/storage/proto/micro_partition_stats.proto | Adds ASF license header block comment. |
| contrib/pax_storage/src/cpp/storage/pax_test.cc | Updates include path to new in-repo cpp-stub location. |
| contrib/pax_storage/src/cpp/storage/orc/orc_test.cc | Updates include path to new in-repo cpp-stub location. |
| contrib/pax_storage/src/cpp/storage/file_system_test.cc | Updates include path to new in-repo cpp-stub location. |
| contrib/pax_storage/src/cpp/pax_gtest.cc | Updates include path to new in-repo cpp-stub location. |
| contrib/pax_storage/src/cpp/cpp-stub/src/stub.h | Adds vendored cpp-stub header into repo tree. |
| contrib/pax_storage/src/cpp/cpp-stub/src/addr_pri.h | Adds vendored cpp-stub header into repo tree. |
| contrib/pax_storage/src/cpp/cpp-stub/src_win/addr_any.h | Adds vendored cpp-stub Windows header into repo tree. |
| contrib/pax_storage/src/cpp/cpp-stub/src_darwin/addr_any.h | Adds vendored cpp-stub darwin placeholder header into repo tree. |
| contrib/pax_storage/src/cpp/cpp-stub/README.md | Adds vendored cpp-stub README into repo tree. |
| contrib/pax_storage/src/cpp/cpp-stub/LICENSE | Updates cpp-stub LICENSE file contents/header. |
| contrib/pax_storage/src/cpp/cpp-stub/.gitignore | Adds cpp-stub local ignore patterns. |
| contrib/pax_storage/src/cpp/CMakeLists.txt | Adds ASF license header block. |
| contrib/pax_storage/src/cpp/cmake/pax.cmake | Adds ASF license header block. |
| contrib/pax_storage/src/cpp/cmake/pax_format.cmake | Adds ASF license header block. |
| contrib/pax_storage/src/api/python3/test/paxpy_test.py | Adds ASF license header block. |
| contrib/pax_storage/src/api/python3/setup.py | Adds ASF license header block. |
| contrib/pax_storage/src/api/python3/setup-debug.py | Adds ASF license header block. |
| contrib/pax_storage/src/api/python3/paxtype_cast.cc | Adds ASF license header block. |
| contrib/pax_storage/src/api/python3/paxpy_types.h | Adds ASF license header block. |
| contrib/pax_storage/src/api/python3/paxpy_modules.cc | Adds ASF license header block. |
| contrib/pax_storage/src/api/python3/paxpy_comm.h | Adds ASF license header block. |
| contrib/pax_storage/src/api/python3/paxfilereader_type.cc | Adds ASF license header block. |
| contrib/pax_storage/src/api/python3/paxfile_type.cc | Adds ASF license header block. |
| contrib/pax_storage/src/api/python3/CMakeLists.txt | Adds ASF license header block. |
| contrib/pax_storage/Makefile | Adds ASF license header block. |
| contrib/pax_storage/init_file | Adds ASF license header block. |
| contrib/pax_storage/FindDependencies.cmake | Adds ASF license header block. |
| contrib/pax_storage/doc/README.toast.md | Adds ASF license header comment block. |
| contrib/pax_storage/doc/README.md | Adds ASF license header comment block. |
| contrib/pax_storage/doc/README.format.md | Adds ASF license header comment block. |
| contrib/pax_storage/doc/README.filter.md | Adds ASF license header comment block. |
| contrib/pax_storage/doc/README.dev.md | Adds ASF license header comment block. |
| contrib/pax_storage/doc/README.clustering.md | Adds ASF license header comment block. |
| contrib/pax_storage/doc/README.catalog.md | Adds ASF license header comment block. |
| contrib/pax_storage/doc/performance.md | Adds ASF license header comment block. |
| contrib/pax_storage/CMakeLists.txt | Adds ASF license header block. |
| .github/ISSUE_TEMPLATE/config.yml | Adds ASF header block; changes blank_issues_enabled behavior. |
| .github/ISSUE_TEMPLATE/bug-report.yml | Adds ASF header block. |
| .github/DISCUSSION_TEMPLATE/proposal.yml | Adds ASF header block. |
| .github/DISCUSSION_TEMPLATE/ideas-feature-requests.yml | Adds ASF header block. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AC_MSG_CHECKING([CMake version]) | ||
| CMAKE_VERSION=`$CMAKE --version | head -n1 | sed 's/[^0-9]*\([0-9][0-9.]*\).*/\1/'` | ||
| AC_MSG_RESULT([$CMAKE_VERSION]) |
| { $as_echo "$as_me:${as_lineno-$LINENO}: checking CMake version" >&5 | ||
| $as_echo_n "checking CMake version... " >&6; } | ||
| CMAKE_VERSION=`$CMAKE --version | head -n1 | sed 's/^0-9*\(0-90-9.*\).*/\1/'` | ||
| { $as_echo "$as_me:${as_lineno-$LINENO}: result: $CMAKE_VERSION" >&5 | ||
| $as_echo "$CMAKE_VERSION" >&6; } |
| REM Install PyYAML using pip instead of extracting from tarball | ||
| pip3 install --target=%GPDB_INSTALL_PATH%\lib\python PyYAML==5.4.1 |
| <!-- | ||
| Licensed to the Apache Software Foundation (ASF) under one | ||
| or more contributor license agreements. See the NOTICE file | ||
| distributed with this work for additional information | ||
| regarding copyright ownership. The ASF licenses this file |
9440a55 to
8417815
Compare
|
Hi guy, thank you for your contribution, but the pr looks also have some problems as following: The current ordering maintains a critical invariant: if a distributed snapshot considers a gxid completed (gxid < xmax and not in inProgressXidArray), the transaction has already committed locally on every QE. This is what makes a distributed read atomic across segments. With this patch, a snapshot taken after ProcArrayEndGxact() but during the COMMIT PREPARED broadcast treats the gxid as committed while some QEs have not yet processed COMMIT PREPARED. On a QE, the distributed verdict only applies after the local xid is committed (the DistributedLog mapping is written at local commit); on a lagging QE the tuple falls back to local visibility rules, where the xid is still a prepared, in-progress transaction. Consequences:
2: It breaks checkpoint / crash recovery. ProcArrayEndGxact() also clears tmGxact->gxid and tmGxact->includeInCkpt. getDtxCheckPointInfo() relies on scanning the proc array for entries with includeInCkpt set to include committed-but-not-yet-forgotten distributed transactions in the checkpoint record, so that crash recovery (recoverTM()) can re-dispatch COMMIT PREPARED to the QEs (see the protocol comment near getDtxCheckPointInfo() in CreateCheckPoint()). Today this flag is cleared only after the FORGET record(doInsertForgetCommitted()). With this patch, if a checkpoint runs during the broadcast/retry window and the QD then crashes before the FORGET record is written, the committed transaction is missing from the checkpoint's committed-gxid list. Recovery will not re-deliver COMMIT PREPARED, and the prepared transactions on the QEs are treated as in-doubt and rolled back — a durably committed distributed transaction gets partially rolled back on the segments. Committed data is lost. |
8af710c to
daf0cef
Compare
Keep the QD GXID in the proc array through normal transaction cleanup and mark DTX commits while QE notifications are in progress. QD snapshot creation waits on the existing GXID lock until the coordinator and all QEs have a coherent commit view. Retain the existing checkpoint and FORGET COMMITTED lifecycle, and add deterministic isolation coverage for two-phase and one-phase commit paths. Fixes apache#1465
daf0cef to
8967aa4
Compare
Use pg_atomic_read_u64() when WaitForDtxCommit() inspects a DTX. The GXID is published through atomic_gxid, and a plain 64-bit read can tear on platforms where 64-bit accesses are not naturally atomic. Keep the snapshot wait aligned with the existing atomic GXID protocol. See: Issue#1465 <apache#1465>
|
Thanks for identifying these issues. They exposed a real flaw in the earlier The current fix preserves the original ordering. Before the first QE commit Therefore I also changed the wait path to use |
Fixes #1465
What does this PR do?
Fixes a distributed snapshot visibility race during distributed transaction
commit.
The coordinator keeps the distributed GXID in the proc array through the
existing transaction cleanup sequence. Immediately before the first QE commit
notification, it marks the transaction as
commitInProgresswhile holdingProcArrayLock. When a QD creates a distributed snapshot, it checks thesemarkers while holding the same lock. If a marked transaction is found, the QD
waits on the existing GXID heavyweight lock, then reacquires the proc-array
lock and builds the snapshot.
This preserves the required ordering:
progress.
notifications and normal QD cleanup have completed.
ProcArrayEndGxact(),includeInCkpt, the FORGET record, and crash-recoveryhandling keep their existing lifecycle and ordering.
The one-phase path is marked before
COMMIT ONE PHASEdispatch, and thetwo-phase path is marked before
COMMIT PREPAREDdispatch, including retries.WaitForDtxCommit()also reads the published GXID withpg_atomic_read_u64()to match the existing atomic GXID protocol.
Type of Change
Breaking Changes
None.
Test Plan
make installcheckmake -C src/test installcheck-cbdb-parallelTargeted validation was performed in Docker using a cluster built from this
branch:
issue1465_distributed_snapshotpassed: 2/2 isolation2 tests.the committed version for both reads using the same repeatable-read
snapshot.
blocked until the transaction completed, then returned the committed value.
crashed the QD during phase 2, and recovered by re-dispatching
COMMIT PREPARED; committed data was retained and no prepared transactions remainedon either QE.
2,700 writes and 18,948 reader transactions with correct results and no
remaining prepared transactions.
Impact
Performance:
Snapshot creation can wait during the existing QE commit-notification window.
The wait uses the existing local GXID lock and adds no network round trip.
Snapshots outside that window are unchanged.
User-facing changes:
This prevents distributed snapshots from observing mixed committed and
uncommitted versions of the same distributed update. There are no syntax,
catalog, or API changes.
Dependencies:
None.
Checklist
Additional Context
The earlier approach that removed the GXID from the proc array before QE
notification was superseded. The current patch does not remove or advertise a
GXID early; it adds a commit-in-progress marker and waits at QD distributed
snapshot creation. The code change is limited to the DTX/proc-array paths and
one isolation2 regression test.