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

Set Max Connection Limit as Env Variable #70

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AtulRajput01
Copy link

@AtulRajput01 AtulRajput01 commented Jun 13, 2023

This PR addresses issue #18 add a new environment variable, POSTGRES_MAX_CONNECTIONS, to the docker-compose.yml file in order to configure the maximum number of database connections for the TimescaleDB service. By setting the value to 10, we can increase the maximum allowed connections to improve scalability and accommodate higher traffic.

@ChakshuGautam
Copy link
Collaborator

Hey @AtulRajput01 are we using PG or POSTGRES as the prefix for all these?

@AtulRajput01
Copy link
Author

@ChakshuGautam yes sir, we are using 'POSTGRES' as the prefix for all these variables.

Copy link
Collaborator

@jayanth-kumar-morem jayanth-kumar-morem left a comment

Choose a reason for hiding this comment

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

Hi @AtulRajput01 ,

To ensure this feature works as expected, I recommend adding a smoke test to verify the maximum number of database connections. Here's a rough idea of how you could approach this:

  1. Start the Docker services with the new environment variable set.
  2. Connect to the PostgreSQL database and check the current max_connections setting using the SQL command SHOW max_connections;. This should return the value set in the POSTGRES_MAX_CONNECTIONS environment variable.
  3. Try to open more connections than the limit set. PostgreSQL should start refusing new connections once the limit is reached.

This test will help us confirm that the POSTGRES_MAX_CONNECTIONS environment variable is correctly setting the maximum number of database connections.

@AtulRajput01
Copy link
Author

AtulRajput01 commented Jun 15, 2023

I have made the changes accordingly. I have updated the value of POSTGRES_MAX_CONNECTIONS 10 to 100.

Screenshot from 2023-06-15 18-56-14

Copy link
Collaborator

@jayanth-kumar-morem jayanth-kumar-morem 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 add the test to the workflow?

@AtulRajput01
Copy link
Author

Can you add the test to the workflow?

hi @jayanth-kumar-morem , Added a smoke test to verify the maximum number of database connections

Copy link
Collaborator

@jayanth-kumar-morem jayanth-kumar-morem left a comment

Choose a reason for hiding this comment

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

You haven't rebased your updates on the new changes on the repo. Please rebase it properly and here's a sample on how it needs to be tested.

        PGPASSWORD=$POSTGRES_PASSWORD psql -h localhost -U $POSTGRES_USER -d postgres -c "SHOW max_connections;"
        if [ "$(PGPASSWORD=$POSTGRES_PASSWORD psql -h localhost -U $POSTGRES_USER -d postgres -c "SHOW max_connections;" | grep -oP '(?<=row )[0-9]+')" -ne "$POSTGRES_MAX_CONNECTIONS" ]; then
          echo "Max connections is not set correctly!"
          exit 1
        fi

Copy link
Collaborator

@jayanth-kumar-morem jayanth-kumar-morem left a comment

Choose a reason for hiding this comment

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

  1. squash multiple similar commits
  2. Default max connections in Postgresql is 100. Please change it to let's say 10 in the docker-compose and smoke tests. If the smoke tests pass when we pass 10 from docker-compose only, we can verify that we are able to modify the max connections from the env variable you added.

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

Successfully merging this pull request may close these issues.

3 participants