Re: [HACKERS] Parallel Hash take II - Mailing list pgsql-hackers

On Wed, Nov 8, 2017 at 10:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Nov 7, 2017 at 4:01 PM, Andres Freund <andres@anarazel.de> wrote:
>> diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
>> index 4c35ccf65eb..8b91d5a6ebe 100644
>> --- a/src/backend/utils/resowner/resowner.c
>> +++ b/src/backend/utils/resowner/resowner.c
>> @@ -528,16 +528,6 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
>>                                 PrintRelCacheLeakWarning(res);
>>                         RelationClose(res);
>>                 }
>> -
>> -               /* Ditto for dynamic shared memory segments */
>> -               while (ResourceArrayGetAny(&(owner->dsmarr), &foundres))
>> -               {
>> -                       dsm_segment *res = (dsm_segment *) DatumGetPointer(foundres);
>> -
>> -                       if (isCommit)
>> -                               PrintDSMLeakWarning(res);
>> -                       dsm_detach(res);
>> -               }
>>         }
>>         else if (phase == RESOURCE_RELEASE_LOCKS)
>>         {
>> @@ -654,6 +644,16 @@ ResourceOwnerReleaseInternal(ResourceOwner owner,
>>                                 PrintFileLeakWarning(res);
>>                         FileClose(res);
>>                 }
>> +
>> +               /* Ditto for dynamic shared memory segments */
>> +               while (ResourceArrayGetAny(&(owner->dsmarr), &foundres))
>> +               {
>> +                       dsm_segment *res = (dsm_segment *) DatumGetPointer(foundres);
>> +
>> +                       if (isCommit)
>> +                               PrintDSMLeakWarning(res);
>> +                       dsm_detach(res);
>> +               }
>>         }
>>
>> Is that entirely unproblematic? Are there any DSM callbacks that rely on
>> locks still being held? Please split this part into a separate commit
>> with such analysis.
>
> FWIW, I think this change is a really good idea (I recommended it to
> Thomas at some stage, I think).

Yeah, it was Robert's suggestion; I thought I needed *something* like
this but was hesitant for the niggling reason that Andres mentions:
what if someone somewhere (including code outside our source tree)
depends on this ordering because of unlocking etc?

At that time I thought that my clean-up logic wasn't going to work on
Windows without this reordering, because we were potentially closing
file handles after unlinking the files, and I was under the impression
that Windows wouldn't like that.  Since then I've learned that Windows
does actually allow it, but only if all file handles were opened with
the FILE_SHARE_DELETE flag.  We always do that (see src/port/open.c),
so in fact this change is probably not needed for my patch set (theory
not tested).  I will put it in a separate patch as requested by
Andres, because it's generally a good idea anyway for the reasons that
Robert explained (ie you probably always want to clean up memory last,
since it might contain the meta-data/locks/control objects/whatever
you'll need to clean up anything else).

-- 
Thomas Munro
http://www.enterprisedb.com


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