hapostgres
Add support for running a pg_autoctl controlled pgbouncer instance
As per discussion in previous PR [#508].
Thank you in advance for the comments and discussion.
Implements pg_autoctl [enable|disable] pgbouncer [options]
Implements pg_autoctl create postgres [--enable-pgbouncer
Intention In a pg_autoctl managed environment, have a pgbouncer instance pointing to the current primary. The instance can run in any postgres node but not in a monitor node. The instance can start either when the postgres node does or at any other moment. The instance can stop at any moment without affecting the running node.
Architecture Process tree
In essence we want to run pgbouncer as a subprocess of pg_autoctl. It will be pg_autoctl's responsibility to manage the pgbouncer subprocess. In that effect, pg_autoctl launches a 'pgbouncer manager' process and a pgbouncer process under it. When running, the process tree may look like this:
pg_autoctl create postgres --pgdata ./data ---
\_ pg_autoctl: start/stop postgres
| \_ /postgres -D ./data -h *
| \_ postgres: logger
| \_ postgres: startup recovering 000000030000000000000003
| \_ postgres: checkpointer
| \_ postgres: background writer
| \_ postgres: stats collector
| \_ postgres: walreceiver streaming 0/305FB48
\_ pg_autoctl: node active
\_ pg_autoctl: pgbouncer manager
\_ pgbouncer config.ini
The top-level pg_autoctl process needs not to know about the pgbouncer process. It only needs to know about the 'pgbouncer manager' process. This is equivelant with relation between top level pg_autoctl, 'start/stop postgres' and postgres itself.
Pgbouncer manager
This service is responsible for starting,stoping,pausing and resuming pgbouncer. Also, it listens to notifications from the monitor node regarding the state of the nodes and upon a state change, it pauses pgbouncer, updates its running configuration (more about configuration in the section 'Configuration') and finally resumes pgbouncer.
The service will exit either when asked to from the top-level pg_autoctl process, or if pgbouncer fails. It is the top level's process responsibility to restart the pgbouncer manager.
Keeper
Intentionally, keeper does not track any state regarding both pgbouncer manager and pgbouncer itself. The rationale for it, is that pgbouncer should be considered a 'best effort' service. One should explicitly point that the service is required on every startup. If pgbouncer fails, one should not lose failover requirements of the database. Of course, one can enable/disable the pgbouncer service when one wishes.
Configuration
Pgbouncer is heavily configuration based. It is that no command line arguments will be used and everything will be explicitly written in the configuration file that is finally passed as an argument to pgbouncer.
When the user of pg_autoctl requires a pgbouncer service, then she has to point to a pgbouncer configuration file. At that point, pg_autoctl takes control of the configuration. It will only read the 'user' and 'pgbouncer' sections and ignore the 'database' section. Also, it will not read all the keys from those two sections, because the pgbouncer set up has to match the set up of the nodes.
Once the keys are read, then they are written to a special template file handled by pg_autoctl entirely. Any keys that are known to point to files, will be parsed and the contents of said files will be copied over to our template directory. Whenever a pgbouncer process needs to read the configuration, the template file will be copied over to a special run time configuration file. Then any run time keys will be added, eg. logfile location, pid file location, etc, along with the database section.
The database section contains a single entry only, pointing to the currently running primary node.
All configuration file management is implemented following the XDG Base Directory specification. See the structure ConfigFilePaths and pgbouncer_config{.c,.h} for details.
Intentionally no template files are cleaned up at process exit. The same applies for the runtime files although the last is more for debugging purposes at the moment and should be addressed in the future.
Implementation detail
Given the extent of the configuration options of pgbouncer, the structure that is holding all that info is best to not be kept in the stack. We are not responsible for validation of the options, yet it is easier to map the type of the options to either strings or integers. Since the ini api used in pg_auto_failover codebase initializes integer values to -1, a section has been added to skip integer keys with that value when writing ini files. In retrospect having -1 as the default value seems not a great choice as it is not uncommon for -1 to mean "explicitly disable something" as opposed to "use default value". It might be better to use INT_MIN for the default initializer of integers and skip that value if needed.
NOTE: At this point, not all keys are handled (e.g. ssl, auth_type etc) in order to get the full picture first straight before adding those trivial issues.
Supervisor
It was desired to use the already extensive supervisor infrastructure of pg_autoctl to run the pgbouncer manager process under. It was also desired to be able to start or stop the pbouncer manager process without affecting any other running core services, such as the database. Also, it was desired for the keeper service to not track any state regarding pgbouncer, because as explained above, a failing pgbouncer should not lead to any node stop.
For this, a new concept of 'enabled' and 'enableable' service is added. The supervisor can include services that can be enabled during startup or at any other point after the startup. Struct Service is expanded with two new members, 'enabled' to denote the current running state of service which defaults to yes, and 'canDisable' to denote if we can ever disable that service which defaults to 'no'. See supervisor.h for details. When Service is declared with the 'canDisable' flag to 'true' it is possible to enable it or disable it.
Two new signals have been added to handle 'enable' and 'disable' cases, 'USR1' and 'USR2' respectively. Upon receipt of an 'enable' signal, the supervisor goes round its services ring and looks for services that can be enabled, if found, it's start_service function is called and on successfull completion the service is marked as enabled. Upon receipt of a 'disable' signal, the supervisor goes round its services ring and looks for enabled services that can be disabled, if found then the supervisor sends the stop service signal to it and marks it as 'disabled'.
A service that is marked as 'canDisable', can also have any policy defined. If an enabled service fails the supervisor fails to restart it in case it is permanent, the supervisor should not stop all the other services running.
For pgbouncer manager, the policy is set as PERMANENT as it is desired for the service to keep running as long as possible.
Enable/Disable
It follows that pg_autoctl [enable|disable] pgbouncer [options] can be
implemented simply by writing the configuration template and signaling the
running node to enable or disable its services. If the command was 'enable', it
would be preferable and trivial to issue a 'select 1' command to pgbouncer to
ensure that it came up. This is left for a subsequent commit.
Signaling
At this point, the communication between pgbouncer manager and pgbouncer is happening with signals. It will be preferable for this communication to happen using psql commands. It is rather trivial but it adds quite a bit to the already large diff footprint. For this, it is left for a subsequent commit.
If a more fine-grained control over which services are enabled (since now is start everything/stop everything implementation) is desired, then signalling is not the way to go. It is acceptable for now that only one such service exists.
Hi @gkokolatos ; thanks for this PR and very detailed architecture presentation!
A first level reaction here, not a full review yet even though I did skim through the code changes:
-
[supervisor] I think we are missing a notion of static/dynamic services. The services that we have in current releases are all static, the pgbouncer service is dynamic: it needs to be enabled by the user. With such a notion we then can remove
canDisablein your implementation. -
[supervisor] Maybe we can manage two different arrays and a new API to register new services dynamically, rather than statically adding to the only array we have now and mark the service disabled. I'm thinking another static sized array in C, but with a dynamic count and a way to fill-in the next free slot with a dynamic service entry, if that makes sense?
-
[supervisor] I see that you have implemented SIGUSR1 and SIGUSR2 for enable/disable dynamic services. Thinking about how to do it now, I would think we need to register if the service is enabled or not in our pg_autoctl configuration file. Then certainly a SIGHUP (
pg_autoctl reload) that triggers updating our in-memory settings from the on-disk settings would allow to realise that the service is now enabled/disabled and react to that by either starting or stopping it? -
[pgbouncer setup] The current PR is quite big, and I wonder if there's a way to make it useful and still split it in smaller parts. Would it make sense for the first iteration to have a static handling of the
pgbouncer.inifile, without any of the user-provided template business, and only focus on the values we know how to maintain directly? We would always use port 6432, pooling_mode session, etc. That's just a starting point, a later PR can then add user provided templates for pgbouncer setup. -
[pgbouncer control] How much code are we talking about to switch from signals to pgbouncer commands over a Postgres connection? We should be able to re-use most of the
pgsql.cfacilities there, I suppose?
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, December 3, 2020 1:14 PM, Dimitri Fontaine [email protected] wrote:
Hi @gkokolatos ; thanks for this PR and very detailed architecture presentation!
Thank you for your initial comments and the direction. Is always such a pleasure!
A first level reaction here, not a full review yet even though I did skim through the code changes:
Of course. It will take a bit.
[supervisor] I think we are missing a notion of static/dynamic services. The services that we have in current releases are all static, the pgbouncer service is dynamic: it needs to be enabled by the user. With such a notion we then can remove canDisable in your implementation
You are correct. That is exactly the intention and direction. I went down that path to be honest but it was far more intrusive. So I chose the 'easy' approach for now. This ties with the second argument bellow.
[supervisor] Maybe we can manage two different arrays and a new API to register new services dynamically, rather than statically adding to the only array we have now and mark the service disabled. I'm thinking another static sized array in C, but with a dynamic count and a way to fill-in the next free slot with a dynamic service entry, if that makes sense?
To be frank, a fully dynamic array would seem like a better fit. The api could look like supervisor_init(), supervisor_add_service(), supervisor_remove_service(), supervisor_foreach_service(), supervisor_destroy(). It is obvious that ordering in the startup/shutdown sequence is important and this could be handled with a priority parameter to denote that order.
[supervisor] I see that you have implemented SIGUSR1 and SIGUSR2 for enable/disable dynamic services. Thinking about how to do it now, I would think we need to register if the service is enabled or not in our pg_autoctl configuration file. Then certainly a SIGHUP (pg_autoctl reload) that triggers updating our in-memory settings from the on-disk settings would allow to realise that the service is now enabled/disabled and react to that by either starting or stopping it?
I tried explicitly to avoid that for two reasons as I tried to explain in my not extremely clear comment. A reload might possibly affect a running Postgres instance and the intention was to explicitly not mess with anything else that Postgres might be doing. Second, it is possible for the pgbouncer service to fail even before it has gone under supervision, for example during the configuration management. One would not like to risk restarting a pg_autoctl service (which will read state) just because the pgbouncer service fails.
If you do not mind any of the two points above, I can certainly update the patch to react with reloads.
[pgbouncer setup] The current PR is quite big, and I wonder if there's a way to make it useful and still split it in smaller parts. Would it make sense for the first iteration to have a static handling of the pgbouncer.ini file, without any of the user-provided template business, and only focus on the values we know how to maintain directly? We would always use port 6432, pooling_mode session, etc. That's just a starting point, a later PR can then add user provided templates for pgbouncer setup.
I tried that but is not that trivial, especially because several files are needed, eg. auth_file and user_file. One way to split it up would be all the infrastructure, e.g. supervisor, ini_file, etc, included in one PR and another that adds the pgbouncer service. What do you think?
[pgbouncer control] How much code are we talking about to switch from signals to pgbouncer commands over a Postgres connection? We should be able to re-use most of the pgsql.c facilities there, I suppose?
Yes sir. That would be the best way to go, ca 200 lines of code, i.e. ca 10% of the current PR. I thought of adding this as a second PR once everything else has fallen to place. I can do it earlier of course.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.
- [supervisor] I think we are missing a notion of static/dynamic services. The services that we have in current releases are all static, the pgbouncer service is dynamic: it needs to be enabled by the user. With such a notion we then can remove canDisable in your implementation
You are correct. That is exactly the intention and direction. I went down that path to be honest but it was far more intrusive. So I chose the 'easy' approach for now. This ties with the second argument bellow.
I see. Well if we manage to split the PR in a meaningful way we might be able to make progress on that front and implement the supervisor refactoring in a single step, which I think would be best.
- [supervisor] Maybe we can manage two different arrays and a new API to register new services dynamically, rather than statically adding to the only array we have now and mark the service disabled. I'm thinking another static sized array in C, but with a dynamic count and a way to fill-in the next free slot with a dynamic service entry, if that makes sense?
To be frank, a fully dynamic array would seem like a better fit. The api could look like supervisor_init(), supervisor_add_service(), supervisor_remove_service(), supervisor_foreach_service(), supervisor_destroy(). It is obvious that ordering in the startup/shutdown sequence is important and this could be handled with a priority parameter to denote that order.
What I like most about static/dynamic array approach is that we artificially limit the changes in the existing code, and don't have to implement either priorities or service dependencies. Also it makes it quite clear that we may have different policies about error handling for static services compared to dynamic services. More on that later.
I like the API you sketch, it looks like what I had in mind. My current thinking would still be to introduce a intermediate level between Supervisor and its Services, with a static array passed at supervisor_start() like we do today, and then use the dynamic services API on-top of that.
I tried explicitly to avoid that for two reasons as I tried to explain in my not extremely clear comment. A reload might possibly affect a running Postgres instance and the intention was to explicitly not mess with anything else that Postgres might be doing. Second, it is possible for the pgbouncer service to fail even before it has gone under supervision, for example during the configuration management. One would not like to risk restarting a pg_autoctl service (which will read state) just because the pgbouncer service fails. If you do not mind any of the two points above, I can certainly update the patch to react with reloads.
I see, it's all about error handling policy there, then, and limiting the impact of pg_autoctl enable <service> for the rest of the running system. That's a fair point. Still, I think we could use a single signal in that case, say SIGUSR1, and communicate enable/disable in a single source, the configuration file. That way we can still implement things as usual: client-side, we check that things are in order, then edit the pg_autoctl configuration file, then signal the server-side pg_autoctl process that it needs to re-read its setup to apply changes. SIGHUP is used for Postgres and pg_autoctl related changes, SIGUSR1 could be used for “dynamic service(s)”.
This then encourages to have a [services] section in the keeper config, maybe such as the following:
[services]
pgbouncer = enabled
Would that make sense?
Then there's the whole question about error handling policy. At some point we need to decide if pgbouncer is part of the pg_autoctl “user contract” or just an added facility. If the application connection string is going to use the pgbouncer port always, then it seems fair to me to say that failure to run pgbouncer is the same as failure to run Postgres: the application can't connect to our service. At all.
The command pg_autoctl show uri --formation default still shows the Postgres port in the multi-host connection string. I think now is a good time for us to figure out what we want to show to our users when they enable pgbouncer. An extra optional connection string? Replace the connection string port numbers with those of pgbouncer?
Another option in the target architecture would be that when we finally add pg_autoctl create pgbouncer we then have the following setup: application → pgbouncer → pg_autoctl/pgbouncer → pg_autoctl/postgres. In that case the first-level pgbouncer setup would be dynamically edited to always point to the primary's pgbouncer, I suppose.
In any case, what I see is that when using pgbouncer and failing to actually run it, then as far as the application is concerned, the service is unavailable. At first glance, it looks like strong enough reason to impact the whole pg_autoctl node.
- [pgbouncer setup] The current PR is quite big, and I wonder if there's a way to make it useful and still split it in smaller parts. Would it make sense for the first iteration to have a static handling of the pgbouncer.ini file, without any of the user-provided template business, and only focus on the values we know how to maintain directly? We would always use port 6432, pooling_mode session, etc. That's just a starting point, a later PR can then add user provided templates for pgbouncer setup.
I tried that but is not that trivial, especially because several files are needed, eg. auth_file and user_file. One way to split it up would be all the infrastructure, e.g. supervisor, ini_file, etc, included in one PR and another that adds the pgbouncer service. What do you think?
We can generate the auth_file in the same way as done in https://github.com/pgbouncer/pgbouncer/blob/master/etc/mkauth.py ; and user_file could contain only the --username (or pg_autoctl config get postgresql.username, which has a default value in pg_setup_get_username() but we don't use that in the config get command; maybe we should always put something in the configuration file though, when we derive the value at init time).
Same for the target database. I am also wondering about changing --dbname to be a mandatory option at pg_autoctl create time, with a WARNing message when postgres is being used.
Anyway, what I'm saying is that I think we have enough information to generate a minimum working pgbouncer.ini setup and use that in our first PR, then improve on it to provide more flexibility to the users.
- [pgbouncer control] How much code are we talking about to switch from signals to pgbouncer commands over a Postgres connection? We should be able to re-use most of the pgsql.c facilities there, I suppose? Yes sir. That would be the best way to go, ca 200 lines of code, i.e. ca 10% of the current PR. I thought of adding this as a second PR once everything else has fallen to place. I can do it earlier of course.
Well, I'm not too sure about that one anymore. Can you remind me the benefits/advantage of using the pgbouncer console commands compare to using unix signals?
‐‐‐‐‐‐‐ Original Message ‐‐‐‐‐‐‐ On Thursday, December 3, 2020 2:52 PM, Dimitri Fontaine [email protected] wrote:
- [supervisor] I think we are missing a notion of static/dynamic services. The services that we have in current releases are all static, the pgbouncer service is dynamic: it needs to be enabled by the user. With such a notion we then can remove canDisable in your implementation
You are correct. That is exactly the intention and direction. I went down that path to be honest but it was far more intrusive. So I chose the 'easy' approach for now. This ties with the second argument bellow.
I see. Well if we manage to split the PR in a meaningful way we might be able to make progress on that front and implement the supervisor refactoring in a single step, which I think would be best.
Agreed.
- [supervisor] Maybe we can manage two different arrays and a new API to register new services dynamically, rather than statically adding to the only array we have now and mark the service disabled. I'm thinking another static sized array in C, but with a dynamic count and a way to fill-in the next free slot with a dynamic service entry, if that makes sense?
To be frank, a fully dynamic array would seem like a better fit. The api could look like supervisor_init(), supervisor_add_service(), supervisor_remove_service(), supervisor_foreach_service(), supervisor_destroy(). It is obvious that ordering in the startup/shutdown sequence is important and this could be handled with a priority parameter to denote that order.
What I like most about static/dynamic array approach is that we artificially limit the changes in the existing code, and don't have to implement either priorities or service dependencies. Also it makes it quite clear that we may have different policies about error handling for static services compared to dynamic services. More on that later.
Wait. Just to be completely clear. You are suggesting that:
- static services are implemented in a static and thus immutable array
- dynamic services are implemented in a dynamic and mutable array
- startup is handled as is now with the static array
- in supervisor loop we deal with any dynamic services required
Sure. That is more intrusive but makes perfect sense. It will require some extra decoupling in some functions in the supervisor (e.g. as proposed in this PR between restart service and handle the failure of said restart).
If I have not understood correctly your point, please elaborate a bit more.
I like the API you sketch, it looks like what I had in mind. My current thinking would still be to introduce a intermediate level between Supervisor and its Services, with a static array passed at supervisor_start() like we do today, and then use the dynamic services API on-top of that.
If I am correct above, then the api will be for the dynamic part only, no?
II tried explicitly to avoid that for two reasons as I tried to explain in my not extremely clear comment. A reload might possibly affect a running Postgres instance and the intention was to explicitly not mess with anything else that Postgres might be doing. Second, it is possible for the pgbouncer service to fail even before it has gone under supervision, for example during the configuration management. One would not like to risk restarting a pg_autoctl service (which will read state) just because the pgbouncer service fails. If you do not mind any of the two points above, I can certainly update the patch to react with reloads.
I see, it's all about error handling policy there, then, and limiting the impact of pg_autoctl enable
for the rest of the running system. That's a fair point. Still, I think we could use a single signal in that case, say SIGUSR1, and communicate enable/disable in a single source, the configuration file. That way we can still implement things as usual: client-side, we check that things are in order, then edit the pg_autoctl configuration file, then signal the server-side pg_autoctl process that it needs to re-read its setup to apply changes. SIGHUP is used for Postgres and pg_autoctl related changes, SIGUSR1 could be used for “dynamic service(s)”. This then encourages to have a [services] section in the keeper config, maybe such as the following:
[services]
pgbouncer
= enabled
Would that make sense?
Yes it does. Easy to do and I can write it so.
Then there's the whole question about error handling policy. At some point we need to decide if pgbouncer is part of the pg_autoctl “user contract” or just an added facility. If the application connection string is going to use the pgbouncer port always, then it seems fair to me to say that failure to run pgbouncer is the same as failure to run Postgres: the application can't connect to our service. At all.
This is correct. I will leave to you to decide for the user contract. I will add that it might be so that pgbouncer is not the only way to connect to the db. And it might be possible for one pgbouncer instance to fail but having all the other standalone instances running. I feel, that at least having the code to be able to handle 'soft' failing scenarios, where a service is permitted to fail without affecting anything else, will be beneficial and it does not add too much maintenance burden.
The command pg_autoctl show uri --formation default still shows the Postgres port in the multi-host connection string. I think now is a good time for us to figure out what we want to show to our users when they enable pgbouncer. An extra optional connection string? Replace the connection string port numbers with those of pgbouncer?
Excellent question. An extra optional connection string to pgbouncer will seem preferable. One should still be able to connect to the db regardless no?
Another option in the target architecture would be that when we finally add pg_autoctl create pgbouncer we then have the following setup: application → pgbouncer → pg_autoctl/pgbouncer → pg_autoctl/postgres. In that case the first-level pgbouncer setup would be dynamically edited to always point to the primary's pgbouncer, I suppose.
I am afraid I lost you here a bit. For pg_autoctl create pgbouncer I always assumed that it would be application -> pg_autoctl/pgbouncer -> pg_autoctl/postgres which point to the primary's Postgres. Why assume that the primary node runs with a pgbouncer enabled?
In any case, what I see is that when using pgbouncer and failing to actually run it, then as far as the application is concerned, the service is unavailable. At first glance, it looks like strong enough reason to impact the whole pg_autoctl node.
Not really, because any other pgbouncer node might be running just fine. In any case, I will be very happy to implement an everything or nothing policy if you wish.
- [pgbouncer setup] The current PR is quite big, and I wonder if there's a way to make it useful and still split it in smaller parts. Would it make sense for the first iteration to have a static handling of the pgbouncer.ini file, without any of the user-provided template business, and only focus on the values we know how to maintain directly? We would always use port 6432, pooling_mode session, etc. That's just a starting point, a later PR can then add user provided templates for pgbouncer setup.
I tried that but is not that trivial, especially because several files are needed, eg. auth_file and user_file. One way to split it up would be all the infrastructure, e.g. supervisor, ini_file, etc, included in one PR and another that adds the pgbouncer service. What do you think?
We can generate the auth_file in the same way as done in https://github.com/pgbouncer/pgbouncer/blob/master/etc/mkauth.py ; and user_file could contain only the --username (or pg_autoctl config get postgresql.username, which has a default value in pg_setup_get_username() but we don't use that in the config get command; maybe we should always put something in the configuration file though, when we derive the value at init time).
Same for the target database. I am also wondering about changing --dbname to be a mandatory option at pg_autoctl create time, with a WARNing message when postgres is being used.
Anyway, what I'm saying is that I think we have enough information to generate a minimum working pgbouncer.ini setup and use that in our first PR, then improve on it to provide more flexibility to the users.
Again I am terribly sorry but I do not follow very well.
As the PR stands, I think that the end state of how configuration should be managed is achieved, albeit with minor tweaks needed. We can maybe work around the size of the PR by splitting into five distinct parts, all of them needed for the end result. Those will be:
- Read pgbouncer supplied configuration into memory
- Build configuration pathnames and store them
- Write configuration from memory to template file
- Prepare runtime configuration parameters into memory
- Write runtime configuration file using template and memory
All those five steps will lead to the code currently presented in this PR but in smaller chunks. Would that help? It is very possible that I failed to understand your suggestion so excuse me for that.
- [pgbouncer control] How much code are we talking about to switch from signals to pgbouncer commands over a Postgres connection? We should be able to re-use most of the pgsql.c facilities there, I suppose? Yes sir. That would be the best way to go, ca 200 lines of code, i.e. ca 10% of the current PR. I thought of adding this as a second PR once everything else has fallen to place. I can do it earlier of course.
Well, I'm not too sure about that one anymore. Can you remind me the benefits/advantage of using the pgbouncer console commands compare to using unix signals?
The benefit of console commands is that you get a response and you know when and if the request succeeded. With signals there is only hope that things succeeded. It is a nice to have at this point.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.
@DimCitus
Hey, I started working on the two things we agreed upon, namely introducing dynamic services and introducing a [services] section in the keeper configuration. Both are great tasks and fun to work on. Having already spend about a day on those, I realise that they will require a bit more time to get completed. Also, Christmas vacations are upon us (15th of December for me).
So my question is, how can we possibly coordinate or collaborate best in order to move forward fast enough?
Thanks in advance and have a great weekend.
Wait. Just to be completely clear. You are suggesting that: * static services are implemented in a static and thus immutable array * dynamic services are implemented in a dynamic and mutable array * startup is handled as is now with the static array * in supervisor loop we deal with any dynamic services required Sure. That is more intrusive but makes perfect sense. It will require some extra decoupling in some functions in the supervisor (e.g. as proposed in this PR between restart service and handle the failure of said restart). If I have not understood correctly your point, please elaborate a bit more.
Static services, as of today, no changes, yes. Dynamic services: static array (compile time known dimensions) with dynamic content, so mutable, yes. We want to continue using the stack as our GC, so we don't malloc, we just build sizeable objects and fill them with the data we need.
Startup needs to be accommodated to also start enabled services from the get-go, I think.
If I am correct above, then the api will be for the dynamic part only, no?
Yes.
Then there's the whole question about error handling policy. At some point we need to decide if pgbouncer is part of the pg_autoctl “user contract” or just an added facility. If the application connection string is going to use the pgbouncer port always, then it seems fair to me to say that failure to run pgbouncer is the same as failure to run Postgres: the application can't connect to our service. At all.
This is correct. I will leave to you to decide for the user contract. I will add that it might be so that pgbouncer is not the only way to connect to the db. And it might be possible for one pgbouncer instance to fail but having all the other standalone instances running. I feel, that at least having the code to be able to handle 'soft' failing scenarios, where a service is permitted to fail without affecting anything else, will be beneficial and it does not add too much maintenance burden.
We might need to add a “proxyport” column to the nodes in the monitor so that we know how to generate an URI with the right port numbers when pgbouncer is enabled. Also you're right that pgbouncer failure modes are more complex than just 1:1 mapping with Postgres failure modes, because of proxying to another Postgres instance than the local one.
Let's begin with the “allowed to fail” model and play with that a little. I tend to work better with simple and wrong practice than with beautiful theories ;-)
Excellent question. An extra optional connection string to pgbouncer will seem preferable. One should still be able to connect to the db regardless no?
Well it depends on the pooling mode (session, transaction) and the work load. In some cases bypassing pgbouncer means Denial of Service or abouts. That's not the problem we are trying to solve here, though, I will admit to that.
Again I am terribly sorry but I do not follow very well. As the PR stands, I think that the end state of how configuration should be managed is achieved, albeit with minor tweaks needed. We can maybe work around the size of the PR by splitting into five distinct parts, all of them needed for the end result. Those will be: 1) Read pgbouncer supplied configuration into memory 2) Build configuration pathnames and store them 3) Write configuration from memory to template file 4) Prepare runtime configuration parameters into memory 5) Write runtime configuration file using template and memory All those five steps will lead to the code currently presented in this PR but in smaller chunks. Would that help? It is very possible that I failed to understand your suggestion so excuse me for that.
I think the way we handle configuration is complex in itself and should be better addressed in a follow-up PR. One thing I have in mind is to validate the user-provided template file, and that means starting a transient dynamic pgbouncer service from the “client-side” pg_autoctl process and running some pgbouncer queries and Postgres queries before accepting it. That's on-top of all you did already, and again, better discussed separately if at all possible.
Let's go with a static hard-coded template for now, that the users can't change, and offer more features later. Hopefully both features are going to land in the same release anyway.
The benefit of console commands is that you get a response and you know when and if the request succeeded. With signals there is only hope that things succeeded. It is a nice to have at this point.
Let's switch to commands instead of signals in the first PR then. I believe we could split some parts of pgsql.[ch] in a new module pgconn.[ch] so that we can re-use the connection handling facility with the retry bits, notification handling, etc, from the new pgbouncer.[ch] module, or even a more specialised file if needed.
Hey, I started working on the two things we agreed upon, namely introducing dynamic services and introducing a [services] section in the keeper configuration. Both are great tasks and fun to work on. Having already spend about a day on those, I realise that they will require a bit more time to get completed. Also, Christmas vacations are upon us (15th of December for me).
So my question is, how can we possibly coordinate or collaborate best in order to move forward fast enough?
We just released 1.4.1 which is a bug fix release for 1.4.0, with many fixes, as in https://github.com/citusdata/pg_auto_failover/releases/tag/v1.4.1. Our next release is scheduled to be 1.4.2 at this time (unless schema changes need to happen on the monitor, then it would be 1.5.0), and that's scheduled to happen end-of January: https://github.com/citusdata/pg_auto_failover/milestone/10.
Our plan is to switch to monthly releases with the new year. So if you miss the next release, the next one is going to happen just a month later. Do you want to target 1.4.2 version for pgbouncer support? did you have something else in mind with our ways to coordinate/collaborate? I am happy to find more time and other higher-bandwidth supports if needed!
A commit was added to address most of the comments received.
Please refer to commit for details.
Here I wish to outline that now:
Enable/disable is gone in favour of dynamic services A tighter integration with the keeper configuration has been added. Whether pgbouncer is enabled is a property of the keeper configuration. (and all that implies).
The rest is maintained the same. Still it is quite an intrusive commit in itself. IF it all seems like a way to go, I can try to split it the whole thing in three distinct parts for easier review.
- Introduction of dynamic services
- Pgbouncer configuration management
- Introduction of pgbouncer service
Thoughts?
@DimCitus Hi and a happy new year!
Anything I can do to help the review of the current PR?
Hi @gkokolatos ; Happy New Year! Thanks for the ping, I didn't realise you added so much to the current PR. Indeed, I think we are ready for a split and individual review of PRs:
- Dynamic Services
- pgbouncer service with a default hard-coded template configuration
- user-provided template file for pgbouncer
About Dynamic Services, I am not too sure about the API we have at the moment, it feels like it will need a couple more passes. It is strange to have a dynamic handler separated that way (I would probable hard-code that part), and it feels strange to hard-code the call to kill to disable a dynamic service, for instance (I would probably have a function where we handle the termination with a call to kill). Small things overall. I also happen to have another use-case for the Dynamic Services, so this might help review that PR separately and refine the API if needed.
I am wondering. @DimCitus Would this be included by default on pg_auto_failover managed nodes? Because we will be scripting the setup of pgbouncer for our nodes as per https://github.com/neuroforgede/pg_auto_failover_ansible/issues/20 . And I was wondering if this feature would interfere with that.
EDIT:
I re-read the conversation and it seems this would be an optional feature so nvm.
Any updates on when will this feature be merged ? We are looking forward to the feature of pg_autoctl controlled pgbouncer. Any update on it ?
Hi @MeydanOzeri and @zhangyahuai ; thanks for your interest in pg_auto_failover and in this feature to allow controlling the pgbouncer process directly within the pg_autoctl process tree. The sad reality is that no one is working on this idea at the moment, that I know of. The first step towards such a feature is some code infrastructure for what we named “dynamic services”. @gkokolatos has worked a lot on it, see PR https://github.com/citusdata/pg_auto_failover/pull/573. It's not been looked at for a long time now.
Who wants to contribute in that area?
I believe what should be done is the dynamic services infrastructure, and then a generic service that knows how to run a user-defined command when a node state changes or some other event happens that our LISTEN/NOTIFY protocol emits, and then we can embed a pgbouncer layer that uses the generic “hook” facility underneath in a user-friendly way.
