Re: query cancel issues in contrib/dblink

Hi,

contrib/dblink seems to have no treatments for query cancels.
It causes the following issues:

(1) Users need to wait for completion of remote query.
Requests for query cancel won't be delivered to remote servers.

(2) PGresult objects will be memory leak. The result is not released
when query is cancelled; it is released only when dblink function
is called max_calls times.

They are long standing issues (not only in 8.4),
but I hope we will fix them to make dblink more robust.

For (1), asynchronous libpq functions should be used instead of blocking
ones, and wait for the remote query using a loop with CHECK_FOR_INTERRUPTS().
For (2), we might need to store PGresult not only in funcctx->user_fctx
but also in a global list, and free all results in XactCallback if remain.

Comments welcome.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

On Thu, Jun 25, 2009 at 10:41 PM, Itagaki
Takahiro<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> Hi,
>
> contrib/dblink seems to have no treatments for query cancels.
> It causes the following issues:
>
> (1) Users need to wait for completion of remote query.
>    Requests for query cancel won't be delivered to remote servers.
>
> (2) PGresult objects will be memory leak. The result is not released
>    when query is cancelled; it is released only when dblink function
>    is called max_calls times.
>
> They are long standing issues (not only in 8.4),
> but I hope we will fix them to make dblink more robust.
>
> For (1), asynchronous libpq functions should be used instead of blocking
> ones, and wait for the remote query using a loop with CHECK_FOR_INTERRUPTS().

How would you structure this loop exactly?

merlin

Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:

> Takahiro<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> > contrib/dblink seems to have no treatments for query cancels.
> > (1) Users need to wait for completion of remote query.
> > (2) PGresult objects will be memory leak.

Here is a patch to fix the issues. I hope the fixes will be ported
to older versions if possible.

(1) is fixed by using non-blocking APIs in libpq. I think we should
always use non-blocking APIs even if the dblink function itself is
a blocking-function.

(2) is fixed by RegisterXactCallback(AtEOXact_dblink). However, there
might be any better solutions -- for example, ResourceOwner framework.

> > For (1), asynchronous libpq functions should be used instead of blocking
> > ones, and wait for the remote query using a loop with CHECK_FOR_INTERRUPTS().
>
> How would you structure this loop exactly?

Please check execute_query() and wait_for_result() in the patch.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Joe, Itagaki,

Can you provide an update on this patch? Joe, you were going to
review and possibly commit it. Itagaki, did you have a new version?
Are there any outstanding issues?

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Can you provide an update on this patch? Joe, you were going to
> review and possibly commit it. Itagaki, did you have a new version?
> Are there any outstanding issues?

Thanks for your reviewing.
Here is an updated version. I fixed some bugs:

* Fix connection leak on cancel. In the previous patch, running
transactions are canceled, but the temporary connection was leaked.
* Discard all pending results on cancel handler. I implemented
PQexec-compatible behavior with async APIs.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

Itagaki Takahiro wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
>> Can you provide an update on this patch? Joe, you were going to
>> review and possibly commit it. Itagaki, did you have a new version?
>> Are there any outstanding issues?
>
> Thanks for your reviewing.
> Here is an updated version. I fixed some bugs:
>
> * Fix connection leak on cancel. In the previous patch, running
> transactions are canceled, but the temporary connection was leaked.
> * Discard all pending results on cancel handler. I implemented
> PQexec-compatible behavior with async APIs.

Thanks for the update. I am planning to review, but my time will be
extremely limited for the next two weeks. I might find some time this
coming weekend, and will do what I can, but after that it will be 9/28
(after my son's wedding on 9/27) before I get time to look closely.

Joe

What happened to this patch?

---------------------------------------------------------------------------

Itagaki Takahiro wrote:
>
> Merlin Moncure <mmoncure(at)gmail(dot)com> wrote:
>
> > Takahiro<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> > > contrib/dblink seems to have no treatments for query cancels.
> > > (1) Users need to wait for completion of remote query.
> > > (2) PGresult objects will be memory leak.
>
> Here is a patch to fix the issues. I hope the fixes will be ported
> to older versions if possible.
>
> (1) is fixed by using non-blocking APIs in libpq. I think we should
> always use non-blocking APIs even if the dblink function itself is
> a blocking-function.
>
> (2) is fixed by RegisterXactCallback(AtEOXact_dblink). However, there
> might be any better solutions -- for example, ResourceOwner framework.
>
>
> > > For (1), asynchronous libpq functions should be used instead of blocking
> > > ones, and wait for the remote query using a loop with CHECK_FOR_INTERRUPTS().
> >
> > How would you structure this loop exactly?
>
> Please check execute_query() and wait_for_result() in the patch.
>
> Regards,
> ---
> ITAGAKI Takahiro
> NTT Open Source Software Center
>

[ Attachment, skipping... ]

>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +

On Wed, Feb 24, 2010 at 2:33 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> What happened to this patch?

I'm pretty sure it's the same as this:

https://commitfest.postgresql.org/action/patch_view?id=263

...Robert

Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> I'm pretty sure it's the same as this:
> https://commitfest.postgresql.org/action/patch_view?id=263

Yes, (2) are resolved with the patch with a different implementation.

> (2) is fixed by RegisterXactCallback(AtEOXact_dblink). However, there
> might be any better solutions -- for example, ResourceOwner framework.

(1) still exists, but we had a consensus that we don't have to fix it
because we have async functions.

> (1) is fixed by using non-blocking APIs in libpq. I think we should
> always use non-blocking APIs even if the dblink function itself is
> a blocking-function.

Regards,
---
Takahiro Itagaki
NTT Open Source Software Center