New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[YSQL] Deadlock involving WaitForBackendsCatalogVersion #18711
Comments
The function
It is mentioned in the commit summary:
|
The purpose looks like we try to make a snapshot of the current sessions, but end up making a copy of references to those sessions in the |
Note
|
But how can we safely make a clone of I think we can fix the bug by
This assumes that
We can see that |
Thinking more, it's tricky and hard to ensure safety without locking mutex_. We can consider adding an |
Forget the deadlock: GetActiveTransactionList taking locks makes pg_stat_activity unusably slow. This kind of information could be pre-populated close to pgstat code instead of pgstat having to go all the way to the pggate layer to get the information on the fly. Worst is when a CREATE INDEX is running and someone selects from pg_stat_activity: that's going to hang for a while. |
For instance, master flag |
… session Summary: **Background** pg_stat_activity issues an RPC (GetActiveTransactionList) to the local t-server to fetch a mapping between currently active sessions and the transactions that these sessions are executing. At the local t-server, this RPC results in the execution of the following pseudo-code: ``` acquire exclusive pg_client_service_mutex make a shallow copy of active_sessions release exclusive pg_client_service_mutex for session_x in copied sessions: acquire exclusive per_session_lock for session_x read transaction info from session_x release exclusive per_session_lock for session_x ``` In a nutshell, the RPC seeks to acquire an exclusive lock on each of the sessions active at the t-server. This causes contention on a given session's lock whenever the session is performing active work concurrent to a a pg_stat_activity request. This manifests itself particularly during: # Long running DML operations (such as CreateTable/Index) # The WaitForBackendsCatalogVersion flow, where an exclusive lock is acquired on each of the active sessions at the t-server. In theory, any operation which acquires a per-session lock at the t-server for a non-trivial duration will experience contention from a concurrently running pg_stat_activity request. **Fix** This revision introduces a transaction cache at the t-server, which stores the mapping between active sessions and their transactions. The cache is written to, at the beginning and end of each transaction by the session initiating the transaction under an exclusive lock for the cache. The cache is read from, by the session servicing the pg_stat_activity request, under a shared lock for the cache. This alleviates the need for pg_stat_activity to take an exclusive lock on the session. Jira: DB-7616 Test Plan: ``` ./yb_build.sh --cxx-test pgwrapper_pg_stat_activity-test --gtest_filter PgStatActivityDelayTest.SlowDDLOperation ``` Reviewers: jason, myang, pjain, dmitry Reviewed By: myang Subscribers: smishra, ybase, yql, bogdan Differential Revision: https://phorge.dev.yugabyte.com/D28666
… lock on t-server session Summary: Original commit: f38b75f / D28666 **Background** pg_stat_activity issues an RPC (GetActiveTransactionList) to the local t-server to fetch a mapping between currently active sessions and the transactions that these sessions are executing. At the local t-server, this RPC results in the execution of the following pseudo-code: ``` acquire exclusive pg_client_service_mutex make a shallow copy of active_sessions release exclusive pg_client_service_mutex for session_x in copied sessions: acquire exclusive per_session_lock for session_x read transaction info from session_x release exclusive per_session_lock for session_x ``` In a nutshell, the RPC seeks to acquire an exclusive lock on each of the sessions active at the t-server. This causes contention on a given session's lock whenever the session is performing active work concurrent to a a pg_stat_activity request. This manifests itself particularly during: # Long running DML operations (such as CreateTable/Index) # The WaitForBackendsCatalogVersion flow, where an exclusive lock is acquired on each of the active sessions at the t-server. In theory, any operation which acquires a per-session lock at the t-server for a non-trivial duration will experience contention from a concurrently running pg_stat_activity request. **Fix** This revision introduces a transaction cache at the t-server, which stores the mapping between active sessions and their transactions. The cache is written to, at the beginning and end of each transaction by the session initiating the transaction under an exclusive lock for the cache. The cache is read from, by the session servicing the pg_stat_activity request, under a shared lock for the cache. This alleviates the need for pg_stat_activity to take an exclusive lock on the session. Jira: DB-7616 Test Plan: ``` ./yb_build.sh --cxx-test pgwrapper_pg_stat_activity-test --gtest_filter PgStatActivityDelayTest.SlowDDLOperation ``` Reviewers: jason, myang, pjain, dmitry Reviewed By: myang Subscribers: smishra, ybase, yql, bogdan Differential Revision: https://phorge.dev.yugabyte.com/D28923
…ock on t-server session Summary: Original commit: f38b75f / D28666 **Background** pg_stat_activity issues an RPC (GetActiveTransactionList) to the local t-server to fetch a mapping between currently active sessions and the transactions that these sessions are executing. At the local t-server, this RPC results in the execution of the following pseudo-code: ``` acquire exclusive pg_client_service_mutex make a shallow copy of active_sessions release exclusive pg_client_service_mutex for session_x in copied sessions: acquire exclusive per_session_lock for session_x read transaction info from session_x release exclusive per_session_lock for session_x ``` In a nutshell, the RPC seeks to acquire an exclusive lock on each of the sessions active at the t-server. This causes contention on a given session's lock whenever the session is performing active work concurrent to a a pg_stat_activity request. This manifests itself particularly during: # Long running DML operations (such as CreateTable/Index) # The WaitForBackendsCatalogVersion flow, where an exclusive lock is acquired on each of the active sessions at the t-server. In theory, any operation which acquires a per-session lock at the t-server for a non-trivial duration will experience contention from a concurrently running pg_stat_activity request. **Fix** This revision introduces a transaction cache at the t-server, which stores the mapping between active sessions and their transactions. The cache is written to, at the beginning and end of each transaction by the session initiating the transaction under an exclusive lock for the cache. The cache is read from, by the session servicing the pg_stat_activity request, under a shared lock for the cache. This alleviates the need for pg_stat_activity to take an exclusive lock on the session. Jira: DB-7616 Test Plan: ``` ./yb_build.sh --cxx-test pgwrapper_pg_stat_activity-test --gtest_filter PgStatActivityDelayTest.SlowDDLOperation ``` Reviewers: jason, myang, pjain, dmitry Reviewed By: myang Subscribers: bogdan, yql, ybase, smishra Differential Revision: https://phorge.dev.yugabyte.com/D28922
No others backports pending right? |
No more backports left, all done! 👍 |
Summary: This diff reverts changes made in context of the https://phorge.dev.yugabyte.com/D28666 diff. Instead of storing info related to session's transactions into separate map named `TransactionCache` such info is stored near the session itself in the `SessionInfo` structure. ``` struct SessionInfo { TxnAssignment txn_assignment; LockablePgClientSession session; }; ``` This structure is stored in the `PgClientServiceImpl::sessions_` field (instead of `LockablePgClientSessionPtr`). Jira: DB-7616 Test Plan: Jenkins run existing tests ``` ./yb_build.sh --cxx-test pgwrapper_pg_stat_activity-test --gtest_filter PgStatActivityDelayTest.SlowDDLOperation ./yb_build.sh --cxx-test pgwrapper_pg_stat_activity-test --gtest_filter PgStatActivityTest.AllBackendsTransaction ./yb_build.sh --cxx-test pgwrapper_pg_stat_activity-test --gtest_filter PgStatActivityTest.CurrentTransaction ./yb_build.sh --cxx-test pgwrapper_pg_stat_activity-test --gtest_filter PgStatActivityTest.DDLInsideDMLTransaction ``` Reviewers: sergei, myang, kramanathan Reviewed By: sergei, myang Subscribers: bogdan, yql, ybase Tags: #jenkins-ready Differential Revision: https://phorge.dev.yugabyte.com/D29109
Jira Link: DB-7616
Description
When I ran the test
PgIndexBackfillTest.PgStatProgressCreateIndexPhase
via commandThe test passed in master branch (commit dfc08e8) which does not have per-database catalog version mode. However, when I ran the test with per-database catalog version mode turned on by default: change the default value of
--FLAGS_TEST_enable_db_catalog_version_mode
to true, then the above test failed.After debugging, I found there is a potential deadlock scenario. Following are the detailed steps that lead to the deadlock:
(1)
The tablet server (ts-1) runs
WaitForBackendsCatalogVersion
(called by PGDefineIndex
function as part of create index workflow). There is a macro defined for this methodNote that this macro has
GetSession(req)
, which is defined asIn particular, it returns a
PgClientSessionLocker
object.The constructor of
PgClientSessionLocker
automatically locks thelockable
, in this case is the session object.Secondly, the session object will be locked until
method(req, resp, context)
completes.method
in this case isSo we know that
PgClientSession
is now locked and will not be released until its methodWaitForBackendsCatalogVersion
completes.(2)
Now let's consider how
client().WaitForYsqlBackendsCatalogVersion
is implemented and we will see the deadlock.So it is sending
WaitForYsqlBackendsCatalogVersion
RPC to master. At master side:It basically calls
YsqlBackendsManager::WaitForYsqlBackendsCatalogVersion
, where aBackendsCatalogVersionJob
is created:(3)
The
BackendsCatalogVersionJob
job is started viaBackendsCatalogVersionJob::Launch
, which in turn launches a task for each tablet server:BackendsCatalogVersionJob::LaunchTS
starts aBackendsCatalogVersionTS
task for every tablet server. This of course, includes ts-1 that we mentioned in step (1).The
BackendsCatalogVersionTS
followsRetryingTSRpcTask
work flow:It sends out RPC to a tablet server:
Note
WaitForYsqlBackendsCatalogVersionAsync
is called to send aWaitForYsqlBackendsCatalogVersion
RPC to ts-1.(4) Now back in ts-1:
How ts-1 handles
WaitForYsqlBackendsCatalogVersion
?It builds a query:
Then a connection is made to its local PG master to start a new PG backend.
Next,
pg_stat_activity
is defined as a view which involves functionpg_stat_get_activity
:It was found that function
pg_stat_get_activity
is not always called, depending on the other JOIN clauses defined above. But ifpg_stat_get_activity
is called, then we have a problem. Insidepg_stat_get_activity
it callsWe can see eventually it makes a RPC
GetActiveTransactionList
back to ts-1 itself.(5)
In
GetActiveTransactionList
The call
PgClientSessionLocker(session)
makes aPgClientSessionLocker
object, the constructor ofPgClientSessionLocker
automatically locks thelockable
, in this case is the same session object that has already been locked above in step (1). The deadlock!Warning: Please confirm that this issue does not contain any sensitive information
The text was updated successfully, but these errors were encountered: