Re: Parallel Hash take II

On Wed, Nov 8, 2017 at 10:32 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Nov 7, 2017 at 4:01 PM, Andres Freund <andres(at)anarazel(dot)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