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

PR #1001: Postgres Connection Pooling Control #1015

Merged
merged 3 commits into from
May 3, 2021
Merged

PR #1001: Postgres Connection Pooling Control #1015

merged 3 commits into from
May 3, 2021

Conversation

dylanrhysscott
Copy link
Contributor

@dylanrhysscott dylanrhysscott commented Mar 17, 2021

This closes #1001

It appears Boundary requires at least 2 connections to start correctly presumably a connection for a controller and one for the worker? If this was set to 1 Boundary just appears to hang and server never starts so I have added a check to throw an error if 1 is specified. Setting this value to 0 will allow Boundary to open as many connections as required. Testing this using current Boundary release we see 3 idle connections from one controller:

 process_id | username | database_name | client_address | application_name |         backend_start         | state  |         state_change
------------+----------+---------------+----------------+------------------+-------------------------------+--------+-------------------------------
         30 |          |               |                |                  | 2021-03-17 23:13:53.71606+00  |        |
         32 | postgres |               |                |                  | 2021-03-17 23:13:53.716876+00 |        |
        167 | postgres | postgres      |                | psql             | 2021-03-17 23:47:49.24453+00  | active | 2021-03-17 23:48:59.766412+00
        173 | postgres | postgres      | 172.17.0.1     |                  | 2021-03-17 23:48:55.642789+00 | idle   | 2021-03-17 23:48:55.652682+00
        174 | postgres | postgres      | 172.17.0.1     |                  | 2021-03-17 23:48:55.658425+00 | idle   | 2021-03-17 23:48:55.700322+00
        175 | postgres | postgres      | 172.17.0.1     |                  | 2021-03-17 23:48:55.670138+00 | idle   | 2021-03-17 23:48:58.000375+00
         28 |          |               |                |                  | 2021-03-17 23:13:53.715683+00 |        |
         27 |          |               |                |                  | 2021-03-17 23:13:53.71553+00  |        |
         29 |          |               |                |                  | 2021-03-17 23:13:53.715879+00 |        |

Setting the following config

controller {
    name = "boundary-controller"
    description = "A boundary controller"
    database {
       ....
        max_open_connections = 2
    }
}

The number of active connections is reduced

 process_id | username | database_name | client_address | application_name |         backend_start         | state  |         state_change
------------+----------+---------------+----------------+------------------+-------------------------------+--------+-------------------------------
         30 |          |               |                |                  | 2021-03-17 23:13:53.71606+00  |        |
         32 | postgres |               |                |                  | 2021-03-17 23:13:53.716876+00 |        |
        167 | postgres | postgres      |                | psql             | 2021-03-17 23:47:49.24453+00  | active | 2021-03-17 23:51:08.237552+00
        180 | postgres | postgres      | 172.17.0.1     |                  | 2021-03-17 23:51:01.977899+00 | idle   | 2021-03-17 23:51:01.987556+00
        181 | postgres | postgres      | 172.17.0.1     |                  | 2021-03-17 23:51:01.991211+00 | idle   | 2021-03-17 23:51:06.271325+00
         28 |          |               |                |                  | 2021-03-17 23:13:53.715683+00 |        |
         27 |          |               |                |                  | 2021-03-17 23:13:53.71553+00  |        |
         29 |          |               |                |                  | 2021-03-17 23:13:53.715879+00 |        |

Setting the following config

controller {
    name = "boundary-controller"
    description = "A boundary controller"
    database {
       ....
        max_open_connections = 1
    }
}

Boundary correctly throws an error

➜  bin git:(postgres-conn-pool) ./boundary server -config config.hcl
Error connecting to database: unable to create db object with dialect postgres: max_open_connections must be unlimited by setting 0 or at least 2
➜  bin git:(postgres-conn-pool)

@hashicorp-cla
Copy link

hashicorp-cla commented Mar 17, 2021

CLA assistant check
All committers have signed the CLA.

@jefferai
Copy link
Member

I don't think this is a bad addition to have but I'd be interested in knowing why two connections are required.

@jefferai
Copy link
Member

To put it another way, an even better solution would be to accept this patch but not error on 1 connection :-)

@dylanrhysscott
Copy link
Contributor Author

Thanks @jefferai going to take another look at this and see if I can get some logs out of the DB driver for an explanation - Would be good to be able to minimise the number of connections for limited environments such as the ones provided by a managed Postgres option

@dylanrhysscott
Copy link
Contributor Author

dylanrhysscott commented Mar 25, 2021

@jefferai Okay so I think I have traced the issue by littering logs everywhere for debugging - https://github.com/hashicorp/boundary/blob/main/internal/kms/repository_root_key.go#L141 this function call is not returning - I suspect it might have something to do with the reader used in list() function - Wondering if Gorm is creating a couple of connections for each emit of the read to run reads and writes concurrently? Boundary uses a reader and writer here https://github.com/hashicorp/boundary/blob/main/internal/kms/repository.go#L25. I found this in an article

Assuming subsequent statements use the same connection. Run two statements one after another and they’re likely to run on two different connections. Run LOCK TABLES tbl1 WRITE followed by SELECT * FROM tbl1 and you’re likely to block and wait. If you need a guarantee of a single statement being used, you need to use a sql.Tx.

Which suggests two different transactions (ie a read + write) would require independent connections and may explain the blocking problem

@dylanrhysscott
Copy link
Contributor Author

@jefferai Any suggestions on how to proceed with this? Wondering if its a GORM bug more so than Boundary. Is it okay to set minimum required to 2?

@jefferai
Copy link
Member

I'm milestoning this for 0.2.1 -- you may well be right in your assessment but we want to also dig into it and verify it, regarding the reason 1 connection doesn't work. Don't worry -- we want this feature! We just haven't been able to get to it in the runup to 0.2.0...sorry about that.

@jefferai jefferai added this to the 0.2.1 milestone Apr 14, 2021
@dylanrhysscott
Copy link
Contributor Author

Hi @jefferai Thanks for getting back to me :) No worries I'm curious as well as to why one connection does not work. In theory it should I do certainly suspect this will be a GORM limitation more than a Boundary one. We may just have to set the min connections to 2 and open an issue upstream for GORM - Let me know if you need any more tests in the meantime!

Copy link
Collaborator

@jimlambrt jimlambrt left a comment

Choose a reason for hiding this comment

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

Can you update the docs, now that we have a reason for the required two connections?

website/content/docs/configuration/controller.mdx Outdated Show resolved Hide resolved
@dylanrhysscott
Copy link
Contributor Author

Changes made and re sync'd with main due to failing CI script

@jimlambrt
Copy link
Collaborator

@dylanrhysscott -> ty for the contribution!

@jimlambrt jimlambrt merged commit 4ae5003 into hashicorp:main May 3, 2021
@dylanrhysscott dylanrhysscott deleted the postgres-conn-pool branch May 8, 2021 14:33
hugoghx pushed a commit that referenced this pull request Dec 6, 2024
* test(e2e): Add test for minio

* CR

* test(e2e): Add session recording test with oRemoteCommand (#1032)

* test(e2e): Add session recording test using oRemoteCommand

* CR: Use io.Discard

* test(e2e): Add session recording control master test (#1035)

* test(e2e): Add session recording test with ssh certificate injection (#1033)

* test(e2e): Add test for certificate injection with session recording

* CR: Add test cases to use sign endpoint

* test(e2e): Add session recording test with scp (#1036)

* test(e2e): Add scp test for session recording

* CR: Math
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Postgres Connection Pooling Control
4 participants