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

[bitnami/pgbouncer] log to stdout only by default #68905

Merged
merged 1 commit into from
Jul 9, 2024
Merged

[bitnami/pgbouncer] log to stdout only by default #68905

merged 1 commit into from
Jul 9, 2024

Conversation

taraspos
Copy link
Contributor

@taraspos taraspos commented Jul 5, 2024

Description of the change

Add option to disable logging to file by setting PGBOUNCER_LOG_FILE=disable.
Change default to log to stdout only (instead of stdout and file) by default.

Benefits

pgbouncer is running in the foreground (without the -d flag) mode so logs are written to stdout anyway.

When running on Kubernetes/ECS/etc this is enough and having same information in a logfile is unnecessary. From my experience this logfile can grow quite large and having option to disable can fix potential disk space problems.

logfile definition from pgbouncer config documentation:

Generic settings

logfile
Specifies the log file. For daemonization (-d), either this or syslog need to be set.

The log file is kept open, so after rotation, kill -HUP or on console RELOAD; should be done. On Windows, the service must be stopped and started.

Note that setting logfile does not by itself turn off logging to stderr. Use the command-line option -q or -d for that.

Default: not set

Possible drawbacks

N/A

Applicable issues

Additional information

pgbouncer.ini with default PGBOUNCER_LOG_FILE value
[databases]
postgres=host=postgresql port=5432 dbname=postgres

[pgbouncer]
listen_port=6432
listen_addr=0.0.0.0
unix_socket_dir=/tmp/
unix_socket_mode=0777
auth_file=/opt/bitnami/pgbouncer/conf/userlist.txt
auth_type=scram-sha-256
pidfile=/opt/bitnami/pgbouncer/tmp/pgbouncer.pid
admin_users=postgres
client_tls_sslmode=disable
server_tls_sslmode=disable
ignore_startup_parameters=extra_float_digits
stats_period=60
server_round_robin=0
server_fast_close=0
pgbouncer.ini with PGBOUNCER_LOG_FILE=/tmp/test
[databases]
postgres=host=postgresql port=5432 dbname=postgres

[pgbouncer]
listen_port=6432
listen_addr=0.0.0.0
unix_socket_dir=/tmp/
unix_socket_mode=0777
auth_file=/opt/bitnami/pgbouncer/conf/userlist.txt
auth_type=scram-sha-256
pidfile=/opt/bitnami/pgbouncer/tmp/pgbouncer.pid
logfile=/tmp/test
admin_users=postgres
client_tls_sslmode=disable
server_tls_sslmode=disable
ignore_startup_parameters=extra_float_digits
stats_period=60
server_round_robin=0
server_fast_close=0

@github-actions github-actions bot added pgbouncer triage Triage is needed labels Jul 5, 2024
@github-actions github-actions bot requested a review from javsalgar July 5, 2024 09:54
Copy link

@lancedikson lancedikson left a comment

Choose a reason for hiding this comment

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

Would be nice to have this feature! LGTM! 👍

@carrodher carrodher added verify Execute verification workflow for these changes in-progress labels Jul 8, 2024
@github-actions github-actions bot removed the triage Triage is needed label Jul 8, 2024
@github-actions github-actions bot removed the request for review from javsalgar July 8, 2024 06:51
@github-actions github-actions bot requested a review from migruiz4 July 8, 2024 06:51
@migruiz4
Copy link
Member

migruiz4 commented Jul 9, 2024

Hi @taraspos,

Thank you very much for your contribution! Although, I would like to request some changes.

Because the best practice for containers would be to only log to stdout, to prevent filling the container filesystem with unnecessary files, I would like to make it the default behavior.

To do so, I would like to request the following changes:

  • Remove the extra logic from libpgbouncer.sh
  • Change the default value at pgbouncer-env.sh:
- export PGBOUNCER_LOG_FILE="${PGBOUNCER_LOG_FILE:-${PGBOUNCER_LOG_DIR}/pgbouncer.log}"
+ export PGBOUNCER_LOG_FILE="${PGBOUNCER_LOG_FILE:-}"
  • Update the README.md with the new value and description for the variable PGBOUNCER_LOG_FILE:
- | `PGBOUNCER_LOG_FILE`                  | PgBouncer log file.                                                                                                                                                                        | `${PGBOUNCER_LOG_DIR}/pgbouncer.log` |
+ | `PGBOUNCER_LOG_FILE`                  | If set, log PgBouncer output to log file in addition to stdout.                                                                                                                            | `nil`                                |

@taraspos
Copy link
Contributor Author

taraspos commented Jul 9, 2024

@migruiz4 all done!

Copy link
Member

@migruiz4 migruiz4 left a comment

Choose a reason for hiding this comment

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

Thank you very much!

@taraspos taraspos changed the title [bitnami/pgbouncer] add option to disable logfile [bitnami/pgbouncer] log to stdout only by default Jul 9, 2024
@taraspos
Copy link
Contributor Author

taraspos commented Jul 9, 2024

@migruiz4 thank you!
Is this ready to be merged now?

@migruiz4 migruiz4 merged commit 7f8a148 into bitnami:main Jul 9, 2024
10 checks passed
@lancedikson
Copy link

May I ask what the ETA for the release is? :)

@migruiz4
Copy link
Member

migruiz4 commented Jul 9, 2024

I triggered a new release now, so in about 30 more minutes or so a new PR should be submitted, tested, and merged automatically if everything goes well.
Shortly after the image should be available in Dockerhub as well.

@taraspos
Copy link
Contributor Author

taraspos commented Jul 9, 2024

@lancedikson
Copy link

Thanks! 💪

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pgbouncer solved verify Execute verification workflow for these changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bitnami/pgbouncer] Option to disable writing logs to a file
5 participants