Re: Simplify backend terminate and wait logic in postgres_fdw test

On Fri, Apr 9, 2021 at 7:29 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Fri, Apr 09, 2021 at 06:53:21AM +0530, Bharath Rupireddy wrote:
> > I didn't think of the warning cases, my bad. How about using SET
> > client_min_messages = 'ERROR'; before we call
> > pg_wait_for_backend_termination? We can only depend on the return
> > value of pg_wait_for_backend_termination, when true we can exit. This
> > way the buildfarm will not see warnings. Thoughts?
>
> You could do that, but I would also bet that this is going to get
> forgotten in the future if this gets extended in more SQL tests that
> are output-sensitive, in or out of core. Honestly, I can get behind a
> warning in pg_wait_for_backend_termination() to inform that the
> process poked at is not a PostgreSQL one, because it offers new and
> useful information to the user. But, and my apologies for sounding a
> bit noisy, I really don't get why pg_wait_until_termination() has any
> need to do that. From what I can see, it provides the following
> information:
> - A PID, that we already know from the caller or just from
> pg_stat_activity.
> - A timeout, already known as well.
> - The fact that the process did not terminate, information given by
> the "false" status, only used in this case.
>
> So there is no new information here to the user, only a duplicate of
> what's already known to the caller of this function. I see more
> advantages in removing this WARNING rather than keeping it.

IMO it does make sense to provide a warning for a bool returning
function, if there are multiple situations in which the function
returns false. This will give clear information as to why the false is
returned.

pg_terminate_backend: false is returned 1) when the process with given
pid is not a backend(warning "PID %d is not a PostgreSQL server
process") 2) if the kill() fails(warning "could not send signal to
process %d: %m") 3) if the timeout is specified and the backend is not
terminated within it(warning "backend with PID %d did not terminate
within %lld milliseconds").
pg_cancel_backend: false is returned 1) when the process with the
given pid is not a backend 2) if the kill() fails.
pg_wait_for_backend_termination: false is returned 1) when the process
with a given pid is not a backend 2) the backend is not terminated
within the timeout.

If we ensure that all the above functions return false in only one
situation and error in all other situations, then removing warnings
makes sense.

Having said above, there seems to be a reason for issuing a warning
and returning false instead of error, that is the callers can just
call these functions in a loop until they return true. See the below
comments:
/*
* This is just a warning so a loop-through-resultset will not abort
* if one backend terminated on its own during the run.
*/
/* Again, just a warning to allow loops */

I would like to keep the behaviour of these functions as-is.

> You could do that, but I would also bet that this is going to get
> forgotten in the future if this gets extended in more SQL tests that
> are output-sensitive, in or out of core

On the above point of hackers (wanting to use these functions in more
SQL tests) forgetting that the functions pg_terminate_backend,
pg_cancel_backend, pg_wait_for_backend_termination issue a warning in
some cases which might pollute the tests if used without suppressing
these warnings, I feel it is best left to the patch implementers and
the reviewers. On our part, we mentioned that the functions
pg_terminate_backend and pg_wait_for_backend_termination would emit a
warning on timeout "On timeout a warning is emitted and
<literal>false</literal> is returned"

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com