Skip to content
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

fix: Ensure Postgres queries are committed or autocommit is used #5039

Merged

Conversation

TomSteenbergen
Copy link
Contributor

What this PR does / why we need it:

We are using psycopg3 in case we use Postgres for the online store. If you create connections with psycopg3 using a context manager (i.e. a with ... block), it will automatically commit the transaction once you exit the context. However, in feast we are manually creating (and in the case of connection pools, putting) connections, so we need to ensure that we also explicitly call commit after firing queries to the database.

The commit statements were missing for (async) read methods of the PostgresOnlineStore and the teardown method. For the read statements, this wasn't an issue as we just do a SELECT. However, it does add warning statements and quite a bit of latency as transactions needed to be rolled back.

Hence, for those read methods, I introduced an optional autocommit to the _get_conn and _get_conn_async methods that defaults to False. We are setting it to True in our read methods, so that prevents us from having to call commit and further optimizes the performance. In my local tests, online_read_async performance improved by 20-30% when calling explicitly commit, and 40-70% when enabling autocommit (depending on feature view).

Which issue(s) this PR fixes:

Fixes #5038

Signed-off-by: TomSteenbergen <[email protected]>
Signed-off-by: TomSteenbergen <[email protected]>
Copy link
Member

@franciscojavierarceo franciscojavierarceo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this!

@franciscojavierarceo franciscojavierarceo merged commit 46f8d7a into feast-dev:master Feb 11, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Postgres Online store does not always manage transactions gracefully
2 participants