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.

Sign up for GitHub

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

Jump to bottom

Enable cursor support for PostgreSQL and SQLite #2864

Merged
merged 27 commits into from
Jun 27, 2023
Merged

Enable cursor support for PostgreSQL and SQLite #2864

merged 27 commits into from
Jun 27, 2023

Conversation

AlekSi
Copy link
Member

@AlekSi AlekSi commented Jun 20, 2023 •
edited
Loading

Description

This PR completely removes the testing option for enabling and disabling cursors; they are always enabled now.

For everyone to review, hence do not merge PRs that should not be merged .

Readiness checklist

  • I added/updated unit tests.
  • I added/updated integration/compatibility tests.
  • I added/updated comments and checked rendering.
  • I made spot refactorings.
  • I updated user documentation.
  • I ran task all, and it passed.
  • I ensured that PR title is good enough for the changelog.
  • (for maintainers only) I set Reviewers (@FerretDB/core), Labels, Project and project's Sprint fields.
  • I marked all done items in this checklist.

@AlekSi AlekSi added the code/chore Code maintenance improvements label Jun 20, 2023
@AlekSi AlekSi added this to the Next milestone Jun 20, 2023
@AlekSi AlekSi self-assigned this Jun 20, 2023
@codecov
Copy link

codecov bot commented Jun 20, 2023 •
edited
Loading

Codecov Report

Merging #2864 (a207031) into main (9355acc) will increase coverage by 0.12%.
The diff coverage is 65.85%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2864      +/-   ##
==========================================
+ Coverage   63.62%   63.75%   +0.12%     
==========================================
  Files         451      451              
  Lines       23583    23701     +118     
==========================================
+ Hits        15005    15110     +105     
- Misses       7610     7623      +13     
  Partials      968      968
Impacted Files Coverage Δ
integration/setup/listener.go 61.74% <ø> (-0.26%) ⬇️
integration/setup/setup.go 69.78% <ø> (ø)
internal/backends/collection.go 0.00% <ø> (ø)
internal/backends/sqlite/backend.go 0.00% <0.00%> (ø)
internal/backends/sqlite/query_iterator.go 0.00% <0.00%> (ø)
internal/handlers/common/count.go 100.00% <ø> (ø)
internal/handlers/common/find.go 100.00% <ø> (ø)
internal/handlers/common/insert.go 100.00% <ø> (ø)
internal/handlers/registry/pg.go 100.00% <ø> (ø)
internal/handlers/registry/registry.go 55.55% <ø> (ø)
... and 18 more

... and 1 file with indirect coverage changes

Flag Coverage Δ
integration 57.05% <65.04%> (+0.16%) ⬆️
mongodb 4.37% <0.00%> (-0.03%) ⬇️
pg 56.97% <65.04%> (+0.16%) ⬆️
shard-1 42.22% <59.34%> (+0.23%) ⬆️
shard-2 45.34% <62.19%> (+0.09%) ⬆️
shard-3 40.94% <63.41%> (+0.31%) ⬆️
unit 24.29% <18.29%> (+0.03%) ⬆️

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

@AlekSi AlekSi added not ready Issues that are not ready to be worked on; PRs that should skip CI do not merge PRs that should not be merged and removed not ready Issues that are not ready to be worked on; PRs that should skip CI labels Jun 20, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 23, 2023

@AlekSi this pull request has merge conflicts.

@mergify mergify bot added conflict PRs that have merge conflicts and removed conflict PRs that have merge conflicts labels Jun 23, 2023
This was referenced Jun 25, 2023
Closed
Closed
@mergify
Copy link
Contributor

mergify bot commented Jun 26, 2023

@AlekSi this pull request has merge conflicts.

@mergify mergify bot removed the conflict PRs that have merge conflicts label Jun 26, 2023
@AlekSi AlekSi changed the title Add basic cursor support for SQLite Enable cursor support for PostgreSQL and SQLite Jun 26, 2023
@AlekSi AlekSi added code/feature Some user-visible feature is not implemented yet and removed code/chore Code maintenance improvements labels Jun 26, 2023
@AlekSi AlekSi marked this pull request as ready for review June 26, 2023 12:42
@AlekSi AlekSi requested a review from a team as a code owner June 26, 2023 12:42
@AlekSi AlekSi requested review from w84thesun and rumyantseva June 26, 2023 12:42
@AlekSi AlekSi enabled auto-merge (squash) June 26, 2023 12:42
@AlekSi AlekSi requested review from a team, chilagrow and noisersup June 26, 2023 12:42
@AlekSi AlekSi mentioned this pull request Jun 26, 2023
Closed
w84thesun
w84thesun approved these changes Jun 26, 2023
View reviewed changes
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.

It would be nice to have cursors enabled! Asked a question about query iterator changes.

internal/backends/sqlite/query_iterator.go Outdated Show resolved Hide resolved
chilagrow
chilagrow approved these changes Jun 27, 2023
View reviewed changes
Copy link
Member

@chilagrow chilagrow left a comment

Choose a reason for hiding this comment

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

You are right about the complexity added by cursor.

Overall looks great. We have to watch out on error handling when cursor is involved.

internal/clientconn/cursor/cursor.go Show resolved Hide resolved
internal/handlers/pg/msg_aggregate.go Show resolved Hide resolved
internal/handlers/pg/msg_find.go Show resolved Hide resolved
@AlekSi AlekSi removed the do not merge PRs that should not be merged label Jun 27, 2023
@AlekSi AlekSi merged commit 4768f47 into FerretDB:main Jun 27, 2023
@AlekSi AlekSi deleted the sqlite-cursors branch June 27, 2023 06:36
@chilagrow chilagrow mentioned this pull request Jun 27, 2023
Merged
9 tasks
@noisersup noisersup mentioned this pull request Jun 27, 2023
Closed
@adetunjii adetunjii mentioned this pull request Sep 12, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chilagrow chilagrow chilagrow approved these changes

@w84thesun w84thesun w84thesun approved these changes

@rumyantseva rumyantseva Awaiting requested review from rumyantseva rumyantseva is a code owner automatically assigned from FerretDB/leads

@noisersup noisersup Awaiting requested review from noisersup noisersup was automatically assigned from FerretDB/core

Assignees

@AlekSi AlekSi

Labels
code/feature Some user-visible feature is not implemented yet
Projects
Current Engineering
Archived in project
Milestone
v1.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@AlekSi @w84thesun @chilagrow