Обсуждение: 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 whendblink 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.takahiro@oss.ntt.co.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@gmail.com> wrote: > Takahiro<itagaki.takahiro@oss.ntt.co.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
anew version? Are there any outstanding issues?
Thanks,
Stephen
Stephen Frost <sfrost@snowman.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@snowman.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@gmail.com> wrote: > > > Takahiro<itagaki.takahiro@oss.ntt.co.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@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.comPG 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@momjian.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@gmail.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