On Fri, Oct 28, 2022 at 4:41 AM Andres Freund <and...@anarazel.de> wrote:
>
> On 2022-10-24 08:15:11 +0530, Bharath Rupireddy wrote:
>
>
> > + /* When we get SIGINT/SIGTERM, we exit */
> > + if (ready_to_exit)
> > + {
> > + /*
> > + * Try informing the server about our exit, but don't
> > wait around
> > + * or retry on failure.
> > + */
> > + (void) PQputCopyEnd(conn, NULL);
> > + (void) PQflush(conn);
> > + time_to_abort = ready_to_exit;
>
> This doesn't strike me as great - because the ready_to_exit isn't checked in
> the loop around StreamLogicalLog(), we'll reconnect if something else causes
> StreamLogicalLog() to return.Fixed. > Why do we need both time_to_abort and ready_to_exit? Intention to have ready_to_exit is to be able to distinguish between SIGINT/SIGTERM and aborting when endpos is reached so that necessary code is skipped/executed and proper logs are printed. > Perhaps worth noting that > time_to_abort is still an sig_atomic_t, but isn't modified in a signal > handler, which seems a bit unnecessarily confusing. time_to_abort is just a static variable, no? +static bool time_to_abort = false; +static volatile sig_atomic_t ready_to_exit = false; Please see the attached v3 patch. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
v3-0001-Fix-pg_recvlogical-error-message-upon-SIGINT-SIGT.patch
Description: Binary data
