Thread: [HACKERS] Autovacuum launcher occurs error when cancelled by SIGINT
Hi, As I report in another thread[1], I found the autovacuum launcher occurs the following error in PG 10 when this received SIGINT. I can repuroduce this by pg_cancel_backend or `kill -2 <pid of autovacuum laucher>`. 2017-06-21 13:56:07.010 JST [32483] ERROR: canceling statement due to user request 2017-06-21 13:56:08.022 JST [32483] ERROR: can't attach the same segment more than once 2017-06-21 13:56:09.034 JST [32483] ERROR: can't attach the same segment more than once 2017-06-21 13:56:10.045 JST [32483] ERROR: can't attach the same segment more than once ... This errors continue until this process is terminated or the server is restarted. When SIGINT is issued, the process exits from the main loop and returns to sigsetjmp, and calls dsa_attach() before entering into the loop again, this causes the error. We can fix it by calling dsa_attach() before sigsetjmp. Attached is the patch. Regards, [1] https://www.postgresql.org/message-id/20170621205657.61d90605.nagata%40sraoss.co.jp -- Yugo Nagata <nagata@sraoss.co.jp> -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Wed, Jun 21, 2017 at 5:45 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote:
> Hi,
>
> As I report in another thread[1], I found the autovacuum launcher occurs
> the following error in PG 10 when this received SIGINT. I can repuroduce
> this by pg_cancel_backend or `kill -2 <pid of autovacuum laucher>`.
>
> 2017-06-21 13:56:07.010 JST [32483] ERROR: canceling statement due to user request
> 2017-06-21 13:56:08.022 JST [32483] ERROR: can't attach the same segment more than once
> 2017-06-21 13:56:09.034 JST [32483] ERROR: can't attach the same segment more than once
> 2017-06-21 13:56:10.045 JST [32483] ERROR: can't attach the same segment more than once
> ...
>
> This errors continue until this process is terminated or the server is restarted.
>
> When SIGINT is issued, the process exits from the main loop and returns
> to sigsetjmp, and calls dsa_attach() before entering into the loop again,
> this causes the error.
>
> We can fix it by calling dsa_attach() before sigsetjmp. Attached is the patch.
>
I think we can just check dsm_find_mapping() to check whether the dsm
handle is already attached. Something like,
}
- else
+ else if(!dsm_find_mapping(AutoVacuumShmem->av_dsa_handle)) { AutoVacuumDSA =
dsa_attach(AutoVacuumShmem->av_dsa_handle); dsa_pin_mapping(AutoVacuumDSA);
Thoughts?
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
On Wed, Jun 21, 2017 at 6:50 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> I think we can just check dsm_find_mapping() to check whether the dsm
> handle is already attached. Something like,
>
> }
> - else
> + else if(!dsm_find_mapping(AutoVacuumShmem->av_dsa_handle))
> {
> AutoVacuumDSA = dsa_attach(AutoVacuumShmem->av_dsa_handle);
> dsa_pin_mapping(AutoVacuumDSA);
>
> Thoughts?
IMHO, It's not a good idea to use DSM call to verify the DSA handle.
--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com
On Wed, Jun 21, 2017 at 7:07 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote:
> On Wed, Jun 21, 2017 at 6:50 PM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> I think we can just check dsm_find_mapping() to check whether the dsm
>> handle is already attached. Something like,
>>
>> }
>> - else
>> + else if(!dsm_find_mapping(AutoVacuumShmem->av_dsa_handle))
>> {
>> AutoVacuumDSA = dsa_attach(AutoVacuumShmem->av_dsa_handle);
>> dsa_pin_mapping(AutoVacuumDSA);
>>
>> Thoughts?
>
> IMHO, It's not a good idea to use DSM call to verify the DSA handle.
>
Okay. Is there any particular scenario you've in mind where this may fail?
--
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com
On Wed, Jun 21, 2017 at 7:47 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote: >> IMHO, It's not a good idea to use DSM call to verify the DSA handle. >> > Okay. Is there any particular scenario you've in mind where this may fail? It's not about failure, but about the abstraction. When we are using the DSA we should not directly access the DSM which is under DSA. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
On Wed, Jun 21, 2017 at 7:52 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: > On Wed, Jun 21, 2017 at 7:47 PM, Kuntal Ghosh > <kuntalghosh.2007@gmail.com> wrote: >>> IMHO, It's not a good idea to use DSM call to verify the DSA handle. >>> >> Okay. Is there any particular scenario you've in mind where this may fail? > > It's not about failure, but about the abstraction. When we are using > the DSA we should not directly access the DSM which is under DSA. > Okay. I thought that I've found at least one usage of dsm_find_mapping() in the code. :-) But, I've some more doubts. 1. When should we use dsm_find_mapping()? (The first few lines of dsm_attach is same as dsm_find_mapping().) 2. As a user of dsa, how should we check whether my dsa handle is already attached? I guess this is required because, if a user tries to re-attach a dsa handle, it's punishing the user by throwing an error and the user wants to avoid such errors. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
On Wed, Jun 21, 2017 at 9:15 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > This errors continue until this process is terminated or the server is restarted. > > When SIGINT is issued, the process exits from the main loop and returns > to sigsetjmp, and calls dsa_attach() before entering into the loop again, > this causes the error. > > We can fix it by calling dsa_attach() before sigsetjmp. Attached is the patch. Your fix looks like a bad idea to me. If the shared memory area does not exist after an exception occurred the process should be able to re-attach to the shared memory area if it exists or create a new one if that's not the case. That should not be a one-time execution. -- Michael
On Thu, Jun 22, 2017 at 1:29 AM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote: > On Wed, Jun 21, 2017 at 7:52 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >> On Wed, Jun 21, 2017 at 7:47 PM, Kuntal Ghosh >> <kuntalghosh.2007@gmail.com> wrote: >>>> IMHO, It's not a good idea to use DSM call to verify the DSA handle. >>>> >>> Okay. Is there any particular scenario you've in mind where this may fail? >> >> It's not about failure, but about the abstraction. When we are using >> the DSA we should not directly access the DSM which is under DSA. >> > Okay. I thought that I've found at least one usage of > dsm_find_mapping() in the code. :-) > > But, I've some more doubts. > 1. When should we use dsm_find_mapping()? (The first few lines of > dsm_attach is same as dsm_find_mapping().) > 2. As a user of dsa, how should we check whether my dsa handle is > already attached? I guess this is required because, if a user tries to > re-attach a dsa handle, it's punishing the user by throwing an error > and the user wants to avoid such errors. From a logical point of view, there is nothing preventing the use of dsm_find_mapping() on a DSA handle, still the API layering looks wrong if you want to check for an existing mapping. So why not defining a new API, like dsa_find_mapping() that just wraps dsm_find_mapping() but has its own error handling? This would offer more flexibility for the future. -- Michael
On Thu, 22 Jun 2017 13:12:48 +0900 Michael Paquier <michael.paquier@gmail.com> wrote: > On Wed, Jun 21, 2017 at 9:15 PM, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > This errors continue until this process is terminated or the server is restarted. > > > > When SIGINT is issued, the process exits from the main loop and returns > > to sigsetjmp, and calls dsa_attach() before entering into the loop again, > > this causes the error. > > > > We can fix it by calling dsa_attach() before sigsetjmp. Attached is the patch. > > Your fix looks like a bad idea to me. If the shared memory area does > not exist after an exception occurred the process should be able to > re-attach to the shared memory area if it exists or create a new one > if that's not the case. That should not be a one-time execution. Thank you for your comment. I overlooked it and now I understand it. > -- > Michael -- Yugo Nagata <nagata@sraoss.co.jp>
On Thu, Jun 22, 2017 at 9:48 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Thu, Jun 22, 2017 at 1:29 AM, Kuntal Ghosh > <kuntalghosh.2007@gmail.com> wrote: >> On Wed, Jun 21, 2017 at 7:52 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >>> On Wed, Jun 21, 2017 at 7:47 PM, Kuntal Ghosh >>> <kuntalghosh.2007@gmail.com> wrote: >>>>> IMHO, It's not a good idea to use DSM call to verify the DSA handle. >>>>> >>>> Okay. Is there any particular scenario you've in mind where this may fail? >>> >>> It's not about failure, but about the abstraction. When we are using >>> the DSA we should not directly access the DSM which is under DSA. >>> >> Okay. I thought that I've found at least one usage of >> dsm_find_mapping() in the code. :-) >> >> But, I've some more doubts. >> 1. When should we use dsm_find_mapping()? (The first few lines of >> dsm_attach is same as dsm_find_mapping().) >> 2. As a user of dsa, how should we check whether my dsa handle is >> already attached? I guess this is required because, if a user tries to >> re-attach a dsa handle, it's punishing the user by throwing an error >> and the user wants to avoid such errors. > > From a logical point of view, there is nothing preventing the use of > dsm_find_mapping() on a DSA handle, still the API layering looks wrong > if you want to check for an existing mapping. So why not defining a > new API, like dsa_find_mapping() that just wraps dsm_find_mapping() > but has its own error handling? This would offer more flexibility for > the future. Yeah. That sounds reasonable. Or, dsa_attach can throw error conditionally. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com
On Thu, Jun 22, 2017 at 2:44 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote: > On Thu, Jun 22, 2017 at 9:48 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Thu, Jun 22, 2017 at 1:29 AM, Kuntal Ghosh >> <kuntalghosh.2007@gmail.com> wrote: >>> But, I've some more doubts. >>> 1. When should we use dsm_find_mapping()? (The first few lines of >>> dsm_attach is same as dsm_find_mapping().) >>> 2. As a user of dsa, how should we check whether my dsa handle is >>> already attached? I guess this is required because, if a user tries to >>> re-attach a dsa handle, it's punishing the user by throwing an error >>> and the user wants to avoid such errors. >> >> From a logical point of view, there is nothing preventing the use of >> dsm_find_mapping() on a DSA handle, still the API layering looks wrong >> if you want to check for an existing mapping. So why not defining a >> new API, like dsa_find_mapping() that just wraps dsm_find_mapping() >> but has its own error handling? This would offer more flexibility for >> the future. > > Yeah. That sounds reasonable. Or, dsa_attach can throw error conditionally. Maybe, let's see what Robert and Thomas have to tell on the matter as they wrote that code. My take on the matter is that the DSA API should remain close to its parent. By the way, this is a new issue in Postgres 10 as this code has been introduced by 7526e10. So I have added an open item with Álvaro as owner. -- Michael
On Thu, Jun 22, 2017 at 6:10 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Jun 22, 2017 at 2:44 PM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> On Thu, Jun 22, 2017 at 9:48 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>>> On Thu, Jun 22, 2017 at 1:29 AM, Kuntal Ghosh
>>> <kuntalghosh.2007@gmail.com> wrote:
>>>> But, I've some more doubts.
>>>> 1. When should we use dsm_find_mapping()? (The first few lines of
>>>> dsm_attach is same as dsm_find_mapping().)
>>>> 2. As a user of dsa, how should we check whether my dsa handle is
>>>> already attached? I guess this is required because, if a user tries to
>>>> re-attach a dsa handle, it's punishing the user by throwing an error
>>>> and the user wants to avoid such errors.
>>>
>>> From a logical point of view, there is nothing preventing the use of
>>> dsm_find_mapping() on a DSA handle, still the API layering looks wrong
>>> if you want to check for an existing mapping. So why not defining a
>>> new API, like dsa_find_mapping() that just wraps dsm_find_mapping()
>>> but has its own error handling? This would offer more flexibility for
>>> the future.
>>
>> Yeah. That sounds reasonable. Or, dsa_attach can throw error conditionally.
>
> Maybe, let's see what Robert and Thomas have to tell on the matter as
> they wrote that code. My take on the matter is that the DSA API should
> remain close to its parent. By the way, this is a new issue in
> Postgres 10 as this code has been introduced by 7526e10. So I have
> added an open item with Álvaro as owner.
Hmm. So the problem here is that AutoVacLauncherMain assumes that
there are only two possibilities: (1) there is no handle published in
shmem yet, so we should create a DSA area and publish the handle, and
(2) there is a handle published in shmem so we should attach to it.
But there is a another possiblity: (3) there is a handle published in
shmem, but we are already attached to it (because we've restarted our
main loop after SIGINT).
The suggestion so far was to check if we're already attached to that
segment (making use of the implementation detail that a DSA handle is
also a DSM segment handle), but don't we know if we're already
attached by checking our AutoVacuumDSA variable? Like this:
AutoVacuumShmem->av_workitems = InvalidDsaPointer; LWLockRelease(AutovacuumLock); }
- else
+ else if (AutoVacuumDSA == NULL) { AutoVacuumDSA =
dsa_attach(AutoVacuumShmem->av_dsa_handle);
--
Thomas Munro
http://www.enterprisedb.com
Thomas Munro wrote: > Hmm. So the problem here is that AutoVacLauncherMain assumes that > there are only two possibilities: (1) there is no handle published in > shmem yet, so we should create a DSA area and publish the handle, and > (2) there is a handle published in shmem so we should attach to it. > But there is a another possiblity: (3) there is a handle published in > shmem, but we are already attached to it (because we've restarted our > main loop after SIGINT). > > The suggestion so far was to check if we're already attached to that > segment (making use of the implementation detail that a DSA handle is > also a DSM segment handle), but don't we know if we're already > attached by checking our AutoVacuumDSA variable? This seems like a good fix to me. Pushed this way, with an additional comment. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Yugo Nagata wrote: > Hi, > > As I report in another thread[1], I found the autovacuum launcher occurs > the following error in PG 10 when this received SIGINT. I can repuroduce > this by pg_cancel_backend or `kill -2 <pid of autovacuum laucher>`. Thanks for the report, BTW! -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Jun 22, 2017 at 4:29 AM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote: > On Wed, Jun 21, 2017 at 7:52 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >> On Wed, Jun 21, 2017 at 7:47 PM, Kuntal Ghosh >> <kuntalghosh.2007@gmail.com> wrote: >>>> IMHO, It's not a good idea to use DSM call to verify the DSA handle. >>>> >>> Okay. Is there any particular scenario you've in mind where this may fail? >> >> It's not about failure, but about the abstraction. When we are using >> the DSA we should not directly access the DSM which is under DSA. >> > Okay. I thought that I've found at least one usage of > dsm_find_mapping() in the code. :-) > > But, I've some more doubts. > 1. When should we use dsm_find_mapping()? (The first few lines of > dsm_attach is same as dsm_find_mapping().) > 2. As a user of dsa, how should we check whether my dsa handle is > already attached? I guess this is required because, if a user tries to > re-attach a dsa handle, it's punishing the user by throwing an error > and the user wants to avoid such errors. I thought about this when designing the DSA API. I couldn't think of any good reason to provide an 'am-I-already-attached?' function equivalent to dsm_find_mapping. It seemed to me that the client code shouldn't ever be in any doubt about whether it's attached, and that wilfully or absent-mindedly throwing away dsa_area pointers and having to ask for them again doesn't seem like a very good design. I suspect the same applies to dsm_find_mapping, and I don't see any callers in the source tree or indeed anywhere on the internet (based on a quick Google search). But I could be missing something. -- Thomas Munro http://www.enterprisedb.com
Thomas Munro wrote: > I thought about this when designing the DSA API. I couldn't think of > any good reason to provide an 'am-I-already-attached?' function > equivalent to dsm_find_mapping. It seemed to me that the client code > shouldn't ever be in any doubt about whether it's attached, and that > wilfully or absent-mindedly throwing away dsa_area pointers and having > to ask for them again doesn't seem like a very good design. I suspect > the same applies to dsm_find_mapping, and I don't see any callers in > the source tree or indeed anywhere on the internet (based on a quick > Google search). But I could be missing something. I think such an API call is potentially useful for things like assertion checks, if nothing else. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jun 23, 2017 at 7:02 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Thomas Munro wrote: >> I thought about this when designing the DSA API. I couldn't think of >> any good reason to provide an 'am-I-already-attached?' function >> equivalent to dsm_find_mapping. It seemed to me that the client code >> shouldn't ever be in any doubt about whether it's attached, and that >> wilfully or absent-mindedly throwing away dsa_area pointers and having >> to ask for them again doesn't seem like a very good design. I suspect >> the same applies to dsm_find_mapping, and I don't see any callers in >> the source tree or indeed anywhere on the internet (based on a quick >> Google search). But I could be missing something. I don't think that's completely exact. dsm_attach() uses at its duplicates dsm_find_mapping() to see if a segment is already attached. So dsm_attach could be refactored to directly use dsm_find_mapping(). > I think such an API call is potentially useful for things like assertion > checks, if nothing else. Indeed, that's useful here as well. -- Michael
On Thu, 22 Jun 2017 13:55:26 -0400 Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Yugo Nagata wrote: > > Hi, > > > > As I report in another thread[1], I found the autovacuum launcher occurs > > the following error in PG 10 when this received SIGINT. I can repuroduce > > this by pg_cancel_backend or `kill -2 <pid of autovacuum laucher>`. > > Thanks for the report, BTW! Thank you for fixing it! > > -- > Álvaro Herrera https://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Yugo Nagata <nagata@sraoss.co.jp>
On Fri, Jun 23, 2017 at 3:01 AM, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > On Thu, Jun 22, 2017 at 4:29 AM, Kuntal Ghosh > <kuntalghosh.2007@gmail.com> wrote: >> On Wed, Jun 21, 2017 at 7:52 PM, Dilip Kumar <dilipbalaut@gmail.com> wrote: >>> On Wed, Jun 21, 2017 at 7:47 PM, Kuntal Ghosh >>> <kuntalghosh.2007@gmail.com> wrote: >>>>> IMHO, It's not a good idea to use DSM call to verify the DSA handle. >>>>> >>>> Okay. Is there any particular scenario you've in mind where this may fail? >>> >>> It's not about failure, but about the abstraction. When we are using >>> the DSA we should not directly access the DSM which is under DSA. >>> >> Okay. I thought that I've found at least one usage of >> dsm_find_mapping() in the code. :-) >> >> But, I've some more doubts. >> 1. When should we use dsm_find_mapping()? (The first few lines of >> dsm_attach is same as dsm_find_mapping().) >> 2. As a user of dsa, how should we check whether my dsa handle is >> already attached? I guess this is required because, if a user tries to >> re-attach a dsa handle, it's punishing the user by throwing an error >> and the user wants to avoid such errors. > > I thought about this when designing the DSA API. I couldn't think of > any good reason to provide an 'am-I-already-attached?' function > equivalent to dsm_find_mapping. It seemed to me that the client code > shouldn't ever be in any doubt about whether it's attached, and that > wilfully or absent-mindedly throwing away dsa_area pointers and having > to ask for them again doesn't seem like a very good design. I suspect > the same applies to dsm_find_mapping, and I don't see any callers in > the source tree or indeed anywhere on the internet (based on a quick > Google search). But I could be missing something. > Thanks a lot for the clarification. -- Thanks & Regards, Kuntal Ghosh EnterpriseDB: http://www.enterprisedb.com