Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply - Mailing list pgsql-hackers

From vignesh C
Subject Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply
Date
Msg-id CALDaNm3PL6-XpvOnqSw3BjvVcPFgvXt32XAsP62b1jgGiVODGw@mail.gmail.com
Whole thread Raw
In response to Re: Issue in postgres_fdw causing unnecessary wait for cancel request reply  (vignesh C <vignesh21@gmail.com>)
List pgsql-hackers
On Thu, 11 Jan 2024 at 20:00, vignesh C <vignesh21@gmail.com> wrote:
>
> On Thu, 13 Apr 2023 at 23:34, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> >
> >
> >
> > On 2023/04/13 11:00, Kyotaro Horiguchi wrote:
> > > Agreed, it seems to be a leftover when we moved to parse_int_param()
> > > in that area.
> >
> > It looks like there was an oversight in commit e7a2217978. I've attached a patch (0002) that updates
PQconnectPoll()to use parse_int_param() for parsing the keepalives parameter.
 
> >
> > As this change is not directly related to the bug fix, it may not be necessary to back-patch it to the stable
versions,I think. Thought?
 
> >
> >
> > >> To clarify, are you suggesting that PQgetCancel() should
> > >> only parse the parameters for TCP connections
> > >> if cancel->raddr.addr.ss_family != AF_UNIX?
> > >> If so, I think that's a good idea.
> > >
> > > You're right. I used connip in the diff because I thought it provided
> > > the same condition, but in a simpler way.
> >
> > I made a modification to the 0001 patch. It will now allow PQgetCancel() to parse and interpret TCP connection
parametersonly when the connection is not made through a Unix-domain socket.
 
> >
> >
> > > However, I notcied that PQgetCancel() doesn't set errbuf.. So, I'm
> > > fine with your proposal.
> >
> > Ok.
> >
> >
> > >> I think it is important to inform the user when an error
> > >> occurs and a cancel request cannot be sent, as this information
> > >> can help them identify the cause of the problem (such as
> > >> setting an overly large value for the keepalives parameter).
> > >
> > > Although I view it as an internal error, I agree with emitting some
> > > error messages in that situation.
> >
> > Ok.
>
> I have changed the status of the patch to "Waiting on Author" as all
> the issues are not addressed. Feel free to address them and change the
> status accordingly.

The patch which you submitted has been awaiting your attention for
quite some time now.  As such, we have moved it to "Returned with
Feedback" and removed it from the reviewing queue. Depending on
timing, this may be reversible.  Kindly address the feedback you have
received, and resubmit the patch to the next CommitFest.

Regards,
Vignesh



pgsql-hackers by date:

Previous
From: vignesh C
Date:
Subject: Re: Assertion failure in SnapBuildInitialSnapshot()
Next
From: vignesh C
Date:
Subject: Re: Should the archiver process always make sure that the timeline history files exist in the archive?