Re: Patch proposal: New hooks in the connection path

From: "Drouvot, Bertrand" <bdrouvot(at)amazon(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Joe Conway <mail(at)joeconway(dot)com>
Cc: Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, Nathan Bossart <nathandbossart(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: Patch proposal: New hooks in the connection path
Date: 2022-07-12 12:58:27
Message-ID: a1558d12-c1c4-0fe5-f8a5-2b6c2294e55f@amazon.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

On 7/11/22 8:18 AM, Drouvot, Bertrand wrote:
> Hi,
>
> On 7/7/22 10:10 PM, Tom Lane wrote:
>> Joe Conway <mail(at)joeconway(dot)com> writes:
>>> It isn't clear to me if having a hook in the timeout handler is a
>>> nonstarter -- perhaps a comment with suitable warning for prospective
>>> extension authors is enough? Anyone else want to weigh in on this issue
>>> specifically?
>> It doesn't seem like a great place for a hook, because the list of stuff
>> you could safely do there would be mighty short, possibly the empty set.
>> Write to shared memory?  Not too safe.  Write to a file?  Even less.
>> Write to local memory?  Pointless, because we're about to _exit(1).
>> Pretty much anything I can think of that you'd want to do is something
>> we've already decided the core code can't safely do, and putting it
>> in a hook won't make it safer.
>>
>> If someone wants to argue for this hook, I'd like to see a credible
>> example of a *safe* use-case, keeping in mind the points raised in
>> the comments in BackendInitialize and process_startup_packet_die.
>
> The use case would be to increment a counter in shared memory (or most
> probably within an hash table) to reflect the number of times a
> startup packet timeout occurred.
>
> Reading the comments in/related to BackendInitialize() I understand
> that's definitely not safe to write in shared memory for the
> EXEC_BACKEND case, but wouldn't it be safe for the non EXEC_BACKEND case?
>
> BTW, it makes me realize that the hook being fired in the bad startup
> packet case:
>
>         /*
>          * Stop here if it was bad or a cancel packet.
> ProcessStartupPacket
>          * already did any appropriate error reporting.
>          */
>         if (status != STATUS_OK)
> +       {
> +               if (FailedConnection_hook)
> +                       (*FailedConnection_hook)
> (FCET_BAD_STARTUP_PACKET, port);
>                 proc_exit(0);
> +       }
>
> is not safe for the EXEC_BACKEND case.
>
What about the idea to trigger the hook for the STARTUP PACKET TIMEOUT
and BAD STARTUP PACKET only for the non EXEC_BACKEND/Windows cases?

I'm tempted to think it's better to have some cases where one could
benefit from the hook as opposed to none.

Thoughts?

Regards,

--

Bertrand Drouvot
Amazon Web Services: https://aws.amazon.com

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Önder Kalacı 2022-07-12 13:36:45 [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher
Previous Message John Naylor 2022-07-12 12:31:56 Re: proposal: Allocate work_mem From Pool