Re: Autovacuum launcher occurs error when cancelled by SIGINT

On Thu, Jun 22, 2017 at 6:10 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Jun 22, 2017 at 2:44 PM, Kuntal Ghosh
> <kuntalghosh(dot)2007(at)gmail(dot)com> wrote:
>> On Thu, Jun 22, 2017 at 9:48 AM, Michael Paquier
>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>> On Thu, Jun 22, 2017 at 1:29 AM, Kuntal Ghosh
>>> <kuntalghosh(dot)2007(at)gmail(dot)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