Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

Add pooler personality configuration on pgbouncer.ini template #29

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

angeloskyratzakos
Copy link

Enabling some of the pooler configuration parameters.
I have also added poolMode under the poolerPersonality, so this might be a breaking change.
I can leave it as is, if there is any issue.
Thanks

Copy link
Collaborator

@matth-boise matth-boise left a comment

Choose a reason for hiding this comment

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

thanks for the changes! please see feedback. and i apologize for the delay

@@ -8,7 +8,14 @@ nodeAffinity: {} # optionally define nodeAffinity
tolerations: [] # optionally define tolerations
nodeSelector: {} # optionally define nodeSelector

poolMode: session
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you leave the poolMode=session line - but comment it out and add comment? e.g.,
# poolMode: session # please use poolerPersonality.poolMode

Copy link
Author

Choose a reason for hiding this comment

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

Nice idea will do it like that

Copy link
Collaborator

Choose a reason for hiding this comment

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

@angeloskyratzakos it's been a while - sorry! it would be great to get your changes in. could you make this last change - to re-add the poolMode line but commented out?

# serverResetQuery: "DISCARD ALL"
# serverResetQueryAlways: 0
# ignoreStartupParameters: ""
# serverCheckQuery: "SELECT 1;"
Copy link
Collaborator

Choose a reason for hiding this comment

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

the default in pgbouncer_1_8_1 is select 1. why did you change it?

Copy link
Author

Choose a reason for hiding this comment

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

You are right. I was following the pgbouncer's config

server_reset_query = {{ .Values.poolerPersonality.serverResetQuery | default "DISCARD ALL" }}
server_reset_query_always = {{ .Values.poolerPersonality.serverResetQueryAlways | default 0 }}
ignore_startup_parameters = {{ .Values.poolerPersonality.ignoreStartupParameters | default "" }}
server_check_query = {{ .Values.poolerPersonality.serverCheckQuery | default "SELECT 1;" }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

the default in pgbouncer_1_8_1 is select 1. why did you change it?

Copy link
Author

Choose a reason for hiding this comment

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

You're right, I will update it. I was following pgbouncer's config page

pgbouncer/templates/_pgbouncer.ini.tpl Show resolved Hide resolved
;server_check_query = select 1
;server_check_delay = 30
;application_name_add_host = 0
pool_mode = {{ .Values.poolerPersonality.poolMode | default "session" }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

in order to preserve backwards compatibility for chart users that may be doing custom overrides (through a -f overrides.yaml file or by pulling in pgbouncer as a requirements chart), i recommend changing this to use .Values.poolMode if it is in the dictionary. could you change this line to

pool_mode = {{ .Values.poolMode | default .Values.poolerPersonality.poolMode | default "session" }}

Copy link
Author

Choose a reason for hiding this comment

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

Nice trick! I didn't know that one. Thanks for the suggestion!

pgbouncer/values.yaml Show resolved Hide resolved

connectionLimits:
maxClientConn: 200
defaultPoolSize: 200
minPoolSize: 15
reservePoolSize: 25
reservePoolTimeout: 5
# maxDbConnections: 0 # zero means unlimited
# maxUserConnections: 0 # zero means unlimited
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above (since you didn't add templating for max_user_connections)

Copy link
Author

Choose a reason for hiding this comment

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

I have added for it as well please check line 81

Copy link
Author

@angeloskyratzakos angeloskyratzakos left a comment

Choose a reason for hiding this comment

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

@matth-boise Thank you very much for your time spending on my PR, appreciate it. I'm sorry for the delay, I will push the fixes right now.

@@ -8,7 +8,14 @@ nodeAffinity: {} # optionally define nodeAffinity
tolerations: [] # optionally define tolerations
nodeSelector: {} # optionally define nodeSelector

poolMode: session
Copy link
Author

Choose a reason for hiding this comment

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

Nice idea will do it like that

# serverResetQuery: "DISCARD ALL"
# serverResetQueryAlways: 0
# ignoreStartupParameters: ""
# serverCheckQuery: "SELECT 1;"
Copy link
Author

Choose a reason for hiding this comment

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

You are right. I was following the pgbouncer's config

pgbouncer/values.yaml Show resolved Hide resolved

connectionLimits:
maxClientConn: 200
defaultPoolSize: 200
minPoolSize: 15
reservePoolSize: 25
reservePoolTimeout: 5
# maxDbConnections: 0 # zero means unlimited
# maxUserConnections: 0 # zero means unlimited
Copy link
Author

Choose a reason for hiding this comment

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

I have added for it as well please check line 81

server_reset_query = {{ .Values.poolerPersonality.serverResetQuery | default "DISCARD ALL" }}
server_reset_query_always = {{ .Values.poolerPersonality.serverResetQueryAlways | default 0 }}
ignore_startup_parameters = {{ .Values.poolerPersonality.ignoreStartupParameters | default "" }}
server_check_query = {{ .Values.poolerPersonality.serverCheckQuery | default "SELECT 1;" }}
Copy link
Author

Choose a reason for hiding this comment

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

You're right, I will update it. I was following pgbouncer's config page

pgbouncer/templates/_pgbouncer.ini.tpl Show resolved Hide resolved
;server_check_query = select 1
;server_check_delay = 30
;application_name_add_host = 0
pool_mode = {{ .Values.poolerPersonality.poolMode | default "session" }}
Copy link
Author

Choose a reason for hiding this comment

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

Nice trick! I didn't know that one. Thanks for the suggestion!

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

Successfully merging this pull request may close these issues.

2 participants