Reviewer's Guide by Sourcery

This PR refactors Postgres query handling to use cursors only when necessary, specifically when the query pagination limit is not set or exceeds the cursor batch size. It introduces a new function RunQueryForSchemaFn that conditionally applies cursors, improving performance by avoiding unnecessary cursor creation for small queries. The changes also include the replacement of direct calls to RunCursorQueryForSchemaFn with the new RunQueryForSchemaFn in multiple files.

Updated class diagram for cursorSession

classDiagram
    class cursorSession {
        -id string
        -tx *postgres.Tx
        -close func()
    }

File-Level Changes

Images are ready for the commit at 37461f4.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-437-g37461f4181.

Codecov Report

Attention: Patch coverage is 52.11268% with 34 lines in your changes missing coverage. Please review.

Project coverage is 48.95%. Comparing base (d174ead) to head (37461f4).
Report is 46 commits behind head on master.

@@            Coverage Diff             @@
##           master   #14939      +/-   ##
==========================================
- Coverage   48.96%   48.95%   -0.02%     
==========================================
  Files        2550     2551       +1     
  Lines      187228   187411     +183     
==========================================
+ Hits        91677    91747      +70     
- Misses      88301    88404     +103     
- Partials     7250     7260      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

How did you test this? I'm not sure CI is sufficient as I would imagine by and large the CI queries and flows would come in under the cursor threshold. I would think a new unit test would probably be valid as well since we are creating a new path.

@dashrews78 I have changed functions. Based on @janisz tests, using a cursor is expensive, and we don't want to keep it everywhere. I didn't add a test because there is no if anymore. All common functions are primarily tested via store functions. It will be the same with RunQueryForSchemaFn when we start using it. Please take a look.

@janisz, can you please check this PR again?

@dashrews78 I have changed functions. Based on @janisz tests, using a cursor is expensive, and we don't want to keep it everywhere. I didn't add a test because there is no if anymore. All common functions are primarily tested via store functions. It will be the same with RunQueryForSchemaFn when we start using it. Please take a look.

@janisz, can you please check this PR again?

I will be honest, these changes have been on blast and I have not had the time to keep up due to critical feature priority. So forgive me if the questions I'm about to ask have been addressed elsewhere.

Regarding the "cursor is expensive" statement, there are 2 sides to that equation. Central expense and Postgres expense. Have we looked at how these changes impact consumption of resources on Postgres? Do we have quantification of the cursor expense some place? What type of load have these tests been performed? What set of objects and queries were primarily used?

From my tests on alerts, cursor is 20 to 30% slower than without it.
I have not checked the impact on the postgres yet. We will summarize it in one place and decide if we need cursors or not.

Regarding the "cursor is expensive" statement, there are 2 sides to that equation. Central expense and Postgres expense. Have we looked at how these changes impact consumption of resources on Postgres? Do we have quantification of the cursor expense some place? What type of load have these tests been performed? What set of objects and queries were primarily used?

I think we are not getting it worse as we keep Walk with cursor and GetBy... without it. Eventually we can discuss if we should keep cursor and maybe merge Walk with GetByQuery or even GetMany (if we don't need missing indices) but for now I think migration to a callback give us more performance.

Regarding the "cursor is expensive" statement, there are 2 sides to that equation. Central expense and Postgres expense. Have we looked at how these changes impact consumption of resources on Postgres? Do we have quantification of the cursor expense some place? What type of load have these tests been performed? What set of objects and queries were primarily used?

I think we are not getting it worse as we keep Walk with cursor and GetBy... without it. Eventually we can discuss if we should keep cursor and maybe merge Walk with GetByQuery or even GetMany (if we don't need missing indices) but for now I think migration to a callback give us more performance.

For posterity, I just want to point out that I don't think any of that actually answered my questions.