Allow pool_size per user

[durdina]

Specifying pool_size is now possible in section [users] beside [databases] or as a global param. Possibility of specifying pool_size in user configuration was requested in the associated issue. Resolves #911.

Looking forward to discussing the implementation details or the issue ratioinale if needed.

Thanks,
Michal

Note: This is my first pull request for this project. I picked this issue as it was labeled as 'good-first-issue' and I wanted to get acquainted with the project.

Fixes #911
Partially fixes #166

[JelteF]

Why change this comment?

[durdina]

I thought it would be a good idea to fix also the documentation of pool_mode. It was slightly misleading as it is not necessarily true that the default is used when pool_mode is not overridden for user. There still can be a setting of pool_mode at db level in which case the default is not used.

The same is true for pool_size starting this PR so I thought I would make the documentation of the two params practically identical.

Also the config.md#L1260-L1261 for the same param is stating this fact correctly already.

[durdina]

HI @JelteF, thank you for looking into this PR.

Unfortunately, I had to revert this PR back to draft since as innocent as my changes look, they somehow managed to break test_peering.py (see the CI log). I am working on that actively, once resolved I'll also address your comments and resubmit.

Thanks

[durdina]

This is weird.

Even a dummy modification of pool_pool_size() breaks cancellation forwarding and causes test_peering.py to fail.

FAILED              [100%]
test/test_peering.py:41 (test_peering_without_own_index)
/media/durdina/workspace/courses/uni/MP/linux/pgbouncer/test/test_peering.py:50: in test_peering_without_own_index
    with pytest.raises(
E   Failed: DID NOT RAISE <class 'psycopg.errors.QueryCanceled'>
        _          = 0
        cancel     = <Future at 0x7ab3f67d4940 state=finished returned NoneType>
        cur        = <psycopg.Cursor [closed] [BAD] at 0x7ab3f67a40f0>
        peers      = {1: <test.utils.Bouncer object at 0x7ab3f67d6b60>,
 2: <test.utils.Bouncer object at 0x7ab3f67d73a0>,
 3: <test.utils.Bouncer object at 0x7ab3f67d7460>}
        pool       = <concurrent.futures.thread.ThreadPoolExecutor object at 0x7ab3f67d4d90>
        query      = <Future at 0x7ab3f67d46a0 state=finished returned Cursor>

It is reproducible though. I've created a patch that adds an additional branch to pool_pool_size() that does not even get executed yet it causes the test to be flaky as from that moment on the cancellation requests sometimes work sometimes not. It's a bit puzzling since the change seems too small to affect execution timing anyhow.

[durdina]

Patch mentioned in the previous comment:
Even_dummy_modification_of_pool_pool_size_causes_issue_in_test_peering_py.patch

[JelteF]

I can reproduce the error with your patch. But it goes away if I change the initial check to:

  if (pool->user && pool->user->connection_count > 999999999) // dummy condition that is never true

So I guess, pool->user can be NULL when this function is called. And not checking for that is causing some strange undefined behaviour.

[JelteF]

Yeah, this field can indeed be NULL according to the stru

	PgUser *user;			/* user logged in as, this field is NULL for peer pools */

[durdina]

Thanks for feedback. I've incorporated much of it, for the documentation I left an explanation.

[durdina]

Yeah, this field can indeed be NULL according to the stru

	PgUser *user;			/* user logged in as, this field is NULL for peer pools */

Also thanks for finding this one, I added logging inside pool_pool_size where I printed out pool->user and never got any visible error but I guess that's because pool->user was null only inside peer pgbouncer (peer id #2 or #3) which probably caused premature termination (Segmentation fault) which might be the reason why the cancellation requested was not successfully delivered to posgress process. The logs from pgbouncer peer id #2 were shorter on occurrences when the test failed but I don't recall them looking abnormally terminated though. I think the regular stopping sequence of closing connection was still there. Would have to check again.

[durdina]

Hi @JelteF, I've incorporated your comments and this PR is probably ready for merging. What do you think?

[DrSchmurge]

Hi mates!
When will this feature be available in a release? We are badly waiting for it :)

[durdina]

Hi @DrSchmurge,
I am sorry, I personally cannot tell when the next release is going to be. Official maintainers of this project would be better to assist here.

My advise would be picking this feature as a patch and applying it to the latest released version (which atm is 1.22.1) but I understand it might not be for everybody.

CC: @JelteF