Skip to content

Per connection trasnaction tracking and cleanup #130

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

Merged

Conversation

tonyalaribe
Copy link
Contributor

@tonyalaribe tonyalaribe commented Aug 17, 2025

I ran into an issue where the entire global transaction state is marked as TransactionState::Failed, causing every other transaction to fail until the server is restarted, due to having a single transaction state for the entire application.

When failing, all queries even from new connections return an error

postgres=> select name,project_id,timestamp from otel_logs_and_spans where project_id='00000000-0000-0000-0000-000000000000' and date='2025-08-07' limit 30;
ERROR:  current transaction is aborted, commands ignored until end of transaction block

This PR refactors transaction state management to be per-connection (like postgres) instead of global:

  • Per-connection isolation: Each connection now maintains its own transaction state using a HashMap<ConnectionId, ConnectionState> with the process ID as the unique identifier
  • Thread-safe access: Replaced Mutex with RwLock for better concurrent read performance
  • Automatic cleanup: Implemented opportunistic cleanup that removes stale connections (>1 hour inactive) every 100 state updates to prevent memory leaks

@tonyalaribe tonyalaribe reopened this Aug 17, 2025
@tonyalaribe tonyalaribe force-pushed the per-conn-transaction-tracking branch from 466892e to 79f9e63 Compare August 17, 2025 13:40
@tonyalaribe
Copy link
Contributor Author

This PR tries to track transaction state for each connection in a mark, indexed by the connection pid. But from my tests, all connections to datafusion-postgres & pgwire? seem to always be PID 0, and the Secret is also usually empty. Is the pgwire implementation incomplete?

@tonyalaribe
Copy link
Contributor Author

I'm not tracking connections based on a unique ID, which is a hash of pid + secret_key + socket_address. So it's not more unique and should offer better connection state isolation.

@tonyalaribe tonyalaribe force-pushed the per-conn-transaction-tracking branch from 86bc203 to c664f17 Compare August 17, 2025 22:24
@sunng87
Copy link
Member

sunng87 commented Aug 19, 2025

@tonyalaribe Thank you! This should fix previous issue with transaction state. This was a mistake made in previous big PR.

But actually we don't need to have transaction management at this level, it's already bound with ClientInfo in pgwire: https://github.com/sunng87/pgwire/blob/master/src/api/mod.rs#L60

To update the transaction state, check this example: https://github.com/sunng87/pgwire/blob/master/examples/transaction.rs#L57

I can merge this but in my recent refactor, I will remove this transaction states from datafusion-postgres completely.

@sunng87 sunng87 merged commit 0af480f into datafusion-contrib:master Aug 20, 2025
6 checks passed
@tonyalaribe
Copy link
Contributor Author

Thanks @sunng87. I think I'm happy with any solution you take. I agree that transaction management probably shouldn't be at that level.

Thanks for merging this, by the way. And thanks for this project.

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