[#224] Prefill when doing a switch-to or primary change.

[fluca1978]

Whenever the primary host is changed, by means of an explicit
switch-to command or by a primary failure, the connection flushing
is activated. If possible, the prefill should be also restored on the
new server.
As suggested in
#225 (comment)
it would be nice to check if the specified new server is the same as
the old one (failure of the primary) or a different one (switch-to),
and in the case they are different the prefill is forced.

[jesperpedersen]

I think I like this direction a bit more.

Called the variable primary - and set it to -1, since we don't know what server really is. And, do the server != (unsigned char)primary check before the other prefill checks are done.

[jesperpedersen]

I would out in an explicit primary != -1 check as well...

[jesperpedersen]

You don't need to enter the https://github.com/agroal/pgagroal/blob/master/src/libpgagroal/pool.c#L848 statement if primary isn't valid, so

int primary = -1;

if (pgagroal_get_primary(&primary))
{
    pgagroal_log_debug("No primary defined");
}

if (primary != -1 && server != (unsigned char)primary)
{
  // Line 848 and onwards ...

}

[jesperpedersen]

Can you squash and force push ?

[jesperpedersen]

Ah, there is still a thing.

We don't need to call prefill(false) for an old flushed primary server... which is the "if (primary != -1 && ..." check.

Otherwise it would try and prefill to MIN_SIZE

[fluca1978]

Just to make it clear, you don't want to prefill at all if the primary is not set or the server has remained the same, right?
That makes me wondering: when it is fine to prefill to the MIN_VALUE?

[jesperpedersen]

This is only for the flush_server case, and yes - there should be no prefill (as MIN_SIZE could be > 0, and therefore cause connects to an invalid host).

There are a lot of cases where prefill(false) makes sense - f.ex. after validation of a connection fails or after idle timeout. This is why people need to consider MIN_SIZE carefully as it will tie up resources on the database cluster.

[fluca1978]

I'm not really comfortable with squashing a set of published commits, please check if it is fine now.

[jesperpedersen]

The prefill(true) will happen if primary != server

[jesperpedersen]

And, you can just do 2 levels

if (primary != -1 && ...
{
   // Existing code
  if (..

[fluca1978]

Sorry, I'm a little confused: if there is the need to prefill (number of users and limits not zero) we fork and issue a pgagroal_prefill in the child process, that then exits, while the parent process continues to memory destroy and exits too.
So what is the need to fork if not issuing a pgagroal_prefill?

[jesperpedersen]

There is no need. Hence the if (!fork()) should be the inner most one

[jesperpedersen]

I have to check something with uncrustify - otherwise I'll merge :)

[fluca1978]

Fine.
While reading the code I found a lot of places with code like the following:

if (prefill && config->number_of_users > 0 && config->number_of_limits > 0)
   {
      if (!fork())
      {
         pgagroal_prefill(false);
      }
   }

I was wondering why not to create a function that encapsulates all the above, something like pgagroal_fork_and_prefill_if_needed thcat will be safe to be called without having to check for limits, primary, and so on. Clearly this should be something unrelated to this pull request.

[jesperpedersen]

Yeah, it could be a static function within pool.c

[jesperpedersen]

Merged.

Thanks for your contribution !