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

pgdb: cover transactions with inTransaction function to simplify error handling #833

Merged
merged 33 commits into from
Jul 8, 2022

Conversation

rumyantseva
Copy link
Contributor

@rumyantseva rumyantseva commented Jul 4, 2022

Description

Note: This PR might be hard to review, as a lot of lines of the pgdb package were changed. Let me know how I can help you with reviewing this PR.

There is no special issue for this fix, but it's done as a part of ref #372.

inTransaction

As now we use transactions in many places, handling potential errors (when a transaction begins, commits or rollbacks) takes quite a lot of lines of code (:D) and requires some accuracy.

To simplify this approach, this PR offers an inTransaction function to "wrap" any function with a transaction.

Querier

pgxtype.Querier is introduced instead of pgx.Tx to unify function signatures.

Dealing with errors

As some of the pgdb errors might be wrapped, the comparison of errors was refactored to errors.Is and errors.As instead of ==.

Not in scope

This PR only offers a refactoring for the existing transactions, adding new transactions is not in the scope of this PR.

Readiness checklist

  • I set assignee, reviewers, labels, project and sprint.
  • I added tests for new functionality or bugfixes.
  • I ran task all, and it passed.
  • I added/updated documentation for exported and unexported functions, variables, types, etc.
  • I checked complex documentation rendering with bin/godoc -index -play -timestamps -http=127.0.0.1:6060.

@rumyantseva rumyantseva changed the title pgPool: don't log errors when there are no errors pgdb: don't log errors when there are no errors Jul 4, 2022
@codecov
Copy link

codecov bot commented Jul 4, 2022

Codecov Report

Merging #833 (8e003c9) into main (22dcc23) will increase coverage by 0.26%.
The diff coverage is 64.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #833      +/-   ##
==========================================
+ Coverage   59.10%   59.37%   +0.26%     
==========================================
  Files         218      218              
  Lines        9855     9833      -22     
==========================================
+ Hits         5825     5838      +13     
+ Misses       3320     3296      -24     
+ Partials      710      699      -11     
Impacted Files Coverage Δ
internal/util/testutil/pg.go 40.74% <0.00%> (ø)
internal/handlers/pg/pgdb/pool.go 64.25% <63.88%> (+6.37%) ⬆️
internal/handlers/pg/pgdb/settings.go 37.20% <73.68%> (ø)
internal/handlers/pg/msg_create.go 69.35% <75.00%> (ø)
internal/handlers/pg/msg_drop.go 61.76% <100.00%> (ø)
internal/handlers/pg/msg_dropdatabase.go 62.06% <100.00%> (ø)
internal/handlers/pg/msg_dbstats.go 65.11% <0.00%> (-6.98%) ⬇️
internal/util/version/version.go 55.00% <0.00%> (-5.00%) ⬇️
Flag Coverage Δ
integration 61.79% <61.32%> (+0.24%) ⬆️
mongodb 18.92% <7.54%> (+0.12%) ⬆️
pg 61.72% <61.32%> (+0.24%) ⬆️
unit 24.99% <34.43%> (+0.50%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@rumyantseva rumyantseva self-assigned this Jul 4, 2022
@rumyantseva rumyantseva added the code/bug Some user-visible feature works incorrectly label Jul 4, 2022
@rumyantseva rumyantseva marked this pull request as ready for review July 4, 2022 11:42
@rumyantseva rumyantseva requested a review from AlekSi as a code owner July 4, 2022 11:42
@rumyantseva
Copy link
Contributor Author

@AlekSi @w84thesun please take a look at this quick fix. Let's not log errors if there are no errors (otherwise, the logs look quite confusing).

Copy link
Contributor

@w84thesun w84thesun left a comment

Choose a reason for hiding this comment

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

I think inTransaction method makes it difficult to read the code, as you need to keep it in mind when you read other methods code. Maybe introducing function like finishTx(tx pgx.Tx, err error, method string) would be enough?

@rumyantseva rumyantseva changed the title pgdb: don't log errors when there are no errors pgdb: cover transactions with inTransaction function to simplify error handling Jul 5, 2022
@AlekSi AlekSi linked an issue Jul 7, 2022 that may be closed by this pull request
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
@AlekSi
Copy link
Member

AlekSi commented Jul 7, 2022

#844 was merged, please resolve merge conflicts and replace a few more == with errors.Is

internal/handlers/pg/msg_create.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
internal/handlers/pg/pgdb/pool.go Outdated Show resolved Hide resolved
Copy link
Contributor

@w84thesun w84thesun left a comment

Choose a reason for hiding this comment

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

LGTM.

@AlekSi AlekSi enabled auto-merge (squash) July 8, 2022 12:23
@AlekSi AlekSi disabled auto-merge July 8, 2022 12:23
@AlekSi AlekSi enabled auto-merge (squash) July 8, 2022 12:24
@AlekSi AlekSi added this to the v0.5.0 milestone Jul 8, 2022
@AlekSi AlekSi merged commit 850bf6c into FerretDB:main Jul 8, 2022
@rumyantseva rumyantseva deleted the fix-logging-for-pool branch August 8, 2022 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code/bug Some user-visible feature works incorrectly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Optimize documents fetching / filtering
4 participants