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

warning about pid file during reload even if it is set to its default value #228

Closed
fluca1978 opened this issue May 6, 2022 · 0 comments
Assignees
Labels
bug Something isn't working

Comments

@fluca1978
Copy link
Collaborator

Running 08f1ca2 (version 1.5.0).

When issuing a configuration reload with pgagroal-cli reload the system displays a log notice about the PID file:

 INFO  configuration.c:2743 Restart required for pidfile - Existing /tmp/pgagraol.5432.pid New 

The problem is that the pid file is not indicated at all in the configuration, so it will get its default value, that is not changed during the reload action.
The problem seems to be that the function restart_string correctly checks even for null/empty strings, and in the new configuration (the one being reloaded) the pidfile is not initialized at all, so the message is triggered.

Either restart_string (and similar functions) should be made aware of which arguments can be not filled because they will be set later on, or the config->pidfile attribute should be set early on during pgagroal_read_configuration instead of when the attribute is going to be used (in main.c within create_pidfile).

@fluca1978 fluca1978 added the bug Something isn't working label May 6, 2022
fluca1978 added a commit to fluca1978/pgagroal that referenced this issue May 6, 2022
…loads.

When configuration is reloaded via `pgagroal-cli reload`, the system
checks all the parameters and warns the user about the need of a
restart if some particular parameters have changed.
The check for the `pidfile` parameter is done via `restart_string()`
that, in the case the pidfile has not been specified in the
configuration file, finds a value in the old configuration (the one
running) and no value at all in the new one.
To avoid the misbehaviour, `pgagroal_validate_configuration()` now
invokes a new function `pgagroal_init_pidfile_if_needed()` that sets
the default value of the `pidfile` in the configuration structure.
This way, if the parameter has its default value, no difference will
be spot and no warning message will be issued.

It is safe to make the pidfile default configuration as the last step
in `pgagroal_validate_configuration()` because this function should be
called whenever the configuration is read, and after all this ensures
the configuration is in a "valid" state. On the other hand, it is not
safe to do such configuration within `pgagroal_read_configuration()`
because the default pidfile is built on top of `unix_socket_dir`, that
at that time could be not set.

Close agroal#228
jesperpedersen pushed a commit that referenced this issue May 6, 2022
 reloads.

When configuration is reloaded via `pgagroal-cli reload`, the system
checks all the parameters and warns the user about the need of a
restart if some particular parameters have changed.
The check for the `pidfile` parameter is done via `restart_string()`
that, in the case the pidfile has not been specified in the
configuration file, finds a value in the old configuration (the one
running) and no value at all in the new one.
To avoid the misbehaviour, `pgagroal_validate_configuration()` now
invokes a new function `pgagroal_init_pidfile_if_needed()` that sets
the default value of the `pidfile` in the configuration structure.
This way, if the parameter has its default value, no difference will
be spot and no warning message will be issued.

It is safe to make the pidfile default configuration as the last step
in `pgagroal_validate_configuration()` because this function should be
called whenever the configuration is read, and after all this ensures
the configuration is in a "valid" state. On the other hand, it is not
safe to do such configuration within `pgagroal_read_configuration()`
because the default pidfile is built on top of `unix_socket_dir`, that
at that time could be not set.

Close #228
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant