Add session setting support for specs #381

mkleen · 2024-08-27T12:53:30Z

This adds support for session settings for specs. The session settings will be applied to each connection before executing the spec.

This adds support for session settings for the various specs formats. These session settings will be applied by the http/pg client before the spec is executed.

mkleen · 2024-08-28T06:40:49Z

tests/test_integration.py

@@ -63,7 +63,7 @@ def parse(self, string, name='<string>'):
 class SourceBuildTest(TestCase):

    def test_build_from_branch(self):
-        self.assertIsNotNone(get_crate('4.1'))
+        self.assertIsNotNone(get_crate('5.8'))


Gradle based builds are failing on the ci machines.

mkleen · 2024-08-28T06:46:17Z

@mfussenegger Ready for review!

cr8/clients.py

mfussenegger · 2024-08-28T09:19:43Z

cr8/clients.py

            conn = aiohttp.TCPConnector(**self._connector_params)
            self.__session = session = aiohttp.ClientSession(connector=conn)
+            for setting, value in self.session_settings.items():
+                payload = {'stmt': f'set {setting}={value}'}
+                await _exec(
+                    session,
+                    next(self.urls),
+                    dumps(payload, cls=CrateJsonEncoder)
+                )


TCPConnector afaik internally does some pooling, so this will have the opposite problem of the asyncpg variant in that it only ever gets called once per session initialization instead of once per connection.
It's probably necessary to subclass the TCPConnector to customize _create_connection.

Running the spec mentioned above shows:

cr> select stmt, count(*) from sys.jobs_log group by stmt order by 2 desc ; +----------------------------------+----------+ | stmt | count(*) | +----------------------------------+----------+ | select count(*) from sys.summits | 1000 | | set timezone=UTC | 2 | | set application_name=my_app | 2 | | ANALYZE | 1 | +----------------------------------+----------+

Why did you mark this as resolved, there are no changes for this?

It's resolved, see the results.

The settings were set for multiple clients.

Do i miss something here ? What should be the expected output for the spec with concurrency 10 ?

It should run set .. for each connection it establishes.

With concurrency = 10 I'd expect there to be more than 1 call in the sys.jobs_log output.

You can also verify how many connections it made via:

cr> select connections from sys.nodes limit 10; +-----------------------------------------------------------------------------------------------------------+ | connections | +-----------------------------------------------------------------------------------------------------------+ | {"http": {"open": 1, "total": 12}, "psql": {"open": 0, "total": 0}, "transport": {"open": 0, "total": 0}} | +-----------------------------------------------------------------------------------------------------------+

The numbers should add up

mkleen · 2024-08-29T07:30:31Z

@mfussenegger Thanks for the feedback. Here are the results after the last changes.

test.toml:

[session_settings]
application_name = 'my_app'
timezone = 'UTC'

[[queries]]
name = "test"
statement = "select count(*) from sys.summits"
concurrency = 1
iterations = 1000

cr8 run-spec test.toml localhost:4200

cr> select stmt, count(*) from sys.jobs_log group by stmt order by 2 desc;
+----------------------------------+----------+
| stmt                             | count(*) |
+----------------------------------+----------+
| select count(*) from sys.summits |     1000 |
| select version from sys.nodes    |        1 |
| set timezone=UTC                 |        1 |
| set application_name=my_app      |        1 |
| ANALYZE                          |        1 |
+----------------------------------+----------+

cr8 run-spec test.toml asyncpg://localhost:5432

cr> select stmt, count(*) from sys.jobs_log group by stmt order by 2 desc;
+----------------------------------+----------+
| stmt                             | count(*) |
+----------------------------------+----------+
| select count(*) from sys.summits |     1000 |
| select version from sys.nodes    |        1 |
| set timezone=UTC                 |        1 |
| set application_name=my_app      |        1 |
| ANALYZE                          |        1 |
+----------------------------------+----------+

test.toml:

[session_settings]
application_name = 'my_app'
timezone = 'UTC'

[[queries]]
name = "test"
statement = "select count(*) from sys.summits"
concurrency = 10
iterations = 1000

cr8 run-spec test.toml localhost:4200

cr> select stmt, count(*) from sys.jobs_log group by stmt order by 2 desc;
+----------------------------------+----------+
| stmt                             | count(*) |
+----------------------------------+----------+
| select count(*) from sys.summits |     1000 |
| select version from sys.nodes    |        1 |
| set timezone=UTC                 |        1 |
| set application_name=my_app      |        1 |
| ANALYZE                          |        1 |
+----------------------------------+----------+

cr8 run-spec test.toml asyncpg://localhost:5432

cr> select stmt, count(*) from sys.jobs_log group by stmt order by 2 desc;
+----------------------------------+----------+
| stmt                             | count(*) |
+----------------------------------+----------+
| select count(*) from sys.summits |     1000 |
| set timezone=UTC                 |      120 |
| set application_name=my_app      |      120 |
| select version from sys.nodes    |        1 |
| ANALYZE                          |        1 |
+----------------------------------+----------+

Note: The asyncpg client sets with concurrency 10, 12 pools with 10 connections, therefore each session setting is only applied once.

mkleen · 2024-09-03T14:35:00Z

cr8/engine.py

+            for setting, value in self.session_settings.items():
+                stmt = f'set {setting}={value}'
+                statements = itertools.repeat((stmt, ()), self.concurrency)
+                aio.run_many(f, statements, self.concurrency, self.concurrency)


This works. it's not so elegant. It runs in parallel with the set statement n times for n connections. I tried various approaches but this is straight forward.

@mfussenegger Do you have any opinion on this approach? I think this needs some cleanup, but would you be ok with this solution?

Besides breaking encapsulation I think this might become unrealiable with a higher concurrency. The first set statements can finish before laters are triggered - which means a connection could get re-used instead of creating a new one for each set

I think I'd rather have a minimal extra pool implementation in the client on top of the session, and setting limit=1 on the underlying connector to ensure there is no double pooling.

ok, makes sense. Thanks for the feedback.

I think I'd rather have a minimal extra pool implementation in the client on top of the session, and setting limit=1 on the underlying connector to ensure there is no double pooling.

Do you mean a pool with multiple instances of TCPConnector?

This would mean I would need to create also multiple ClientSessions, one for each TCPConnector and pool them too.

A custom Connector where I maintain the connections is not an option, because I need to have a ClientSession to run HTTP operations on top of this.

I can try the option of using multiple ClientSessions each having it's own TCPConnector with limit 1 and see how the performance is.

mkleen · 2024-09-04T09:04:18Z

@mfussenegger Here is a naive pool implementation for the http client. Surprisingly this improves throughput on the spec:

cr8 run-spec test.toml localhost:4200

master:

## Running Query:
   Name: test
   Statement:
     select count(*) from sys.summits
   Concurrency: 10
   Iterations: 1000
100%|██████████| 1000/1000 [00:00<00:00, 3174.79 requests/s]
Runtime (in ms):
    mean:    0.240 ± 0.031
    min/max: 0.123 → 6.382

with pool:

## Running Query:
   Name: test
   Statement:
     select count(*) from sys.summits
   Concurrency: 10
   Iterations: 1000
100%|██████████| 1000/1000 [00:00<00:00, 4150.77 requests/s]
Runtime (in ms):
    mean:    0.193 ± 0.021
    min/max: 0.103 → 6.738

mkleen · 2024-09-04T09:07:24Z

However, on higher concurrency (100) this is very inefficient.

mkleen · 2024-09-04T09:10:49Z

I don't know how to proceed here. I think it's easier not to deal with this on the client level and instead create a user and set a session default.

mkleen · 2024-09-05T09:03:25Z

What do you think about dropping the support for session settings for the HTTP client and using the asyncpg client for the benchmarks?

mfussenegger · 2024-09-09T08:52:20Z

Added a fixup for the pooling - as far as I can tell it should be working as intended now

mkleen · 2024-09-09T08:54:14Z

Added a fixup for the pooling - as far as I can tell it should be working as intended now

Thanks for the help. Appreciated!

mkleen marked this pull request as ready for review August 27, 2024 12:54

mkleen force-pushed the mkleen/session_settings branch from 7c8cf23 to abd54a2 Compare August 27, 2024 13:45

mkleen marked this pull request as draft August 27, 2024 13:53

mkleen force-pushed the mkleen/session_settings branch from 5c413c3 to 333108a Compare August 27, 2024 16:09

mkleen marked this pull request as ready for review August 27, 2024 16:11

mkleen force-pushed the mkleen/session_settings branch 2 times, most recently from fb33c98 to ab00a48 Compare August 27, 2024 20:24

Add session setting support for specs

09110ba

This adds support for session settings for the various specs formats. These session settings will be applied by the http/pg client before the spec is executed.

mkleen force-pushed the mkleen/session_settings branch from ab00a48 to 09110ba Compare August 27, 2024 20:32

mkleen commented Aug 28, 2024

View reviewed changes

mkleen mentioned this pull request Aug 28, 2024

Enable lookup-joins for join benchmarks based on session settings crate/crate-benchmarks#323

Merged

2 tasks

mfussenegger reviewed Aug 28, 2024

View reviewed changes

mkleen added 2 commits August 28, 2024 12:29

Make session setting only initialized once for each connection

ae612d4

Remove session settings from non-benchmark connections

ab385e0

mkleen requested a review from mfussenegger August 29, 2024 07:34

mkleen commented Sep 3, 2024

View reviewed changes

Initialize every connection of http client with session settings

2d008ae

mkleen force-pushed the mkleen/session_settings branch from 50b02a3 to 2d008ae Compare September 3, 2024 16:44

Add simple pool to http client

c4c6942

clean up

e341947

mkleen force-pushed the mkleen/session_settings branch from 54d5fbc to e341947 Compare September 5, 2024 10:07

mfussenegger force-pushed the mkleen/session_settings branch from b742916 to 08ce745 Compare September 9, 2024 08:40

fixup! clean up

1dcdcd3

mfussenegger force-pushed the mkleen/session_settings branch from 08ce745 to 1dcdcd3 Compare September 9, 2024 08:45

mfussenegger merged commit d910d77 into mfussenegger:master Sep 9, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add session setting support for specs #381

Add session setting support for specs #381

mkleen commented Aug 27, 2024 •

edited

Loading

mkleen Aug 28, 2024

mkleen commented Aug 28, 2024

mfussenegger Aug 28, 2024

mfussenegger Sep 2, 2024

mkleen Sep 2, 2024

mkleen Sep 2, 2024

mkleen Sep 2, 2024 •

edited

Loading

mfussenegger Sep 2, 2024

mkleen commented Aug 29, 2024 •

edited

Loading

mkleen Sep 3, 2024 •

edited

Loading

mkleen Sep 3, 2024

mfussenegger Sep 3, 2024

mkleen Sep 3, 2024

mkleen Sep 4, 2024

mkleen Sep 4, 2024

mkleen Sep 4, 2024

mkleen Sep 4, 2024

mkleen commented Sep 4, 2024

mkleen commented Sep 4, 2024 •

edited

Loading

mkleen commented Sep 4, 2024

mkleen commented Sep 5, 2024

mfussenegger commented Sep 9, 2024

mkleen commented Sep 9, 2024

Add session setting support for specs #381

Add session setting support for specs #381

Conversation

mkleen commented Aug 27, 2024 • edited Loading

Choose a reason for hiding this comment

mkleen commented Aug 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkleen Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkleen commented Aug 29, 2024 • edited Loading

mkleen Sep 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkleen commented Sep 4, 2024

mkleen commented Sep 4, 2024 • edited Loading

mkleen commented Sep 4, 2024

mkleen commented Sep 5, 2024

mfussenegger commented Sep 9, 2024

mkleen commented Sep 9, 2024

mkleen commented Aug 27, 2024 •

edited

Loading

mkleen Sep 2, 2024 •

edited

Loading

mkleen commented Aug 29, 2024 •

edited

Loading

mkleen Sep 3, 2024 •

edited

Loading

mkleen commented Sep 4, 2024 •

edited

Loading