Re: Speedup of relation deletes during recovery - Mailing list pgsql-hackers

On Thu, Jun 28, 2018 at 3:23 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2018-06-28 03:21:51 +0900, Fujii Masao wrote:
>> On Wed, Jun 27, 2018 at 10:44 AM, Thomas Munro
>> <thomas.munro@enterprisedb.com> wrote:
>> > On Wed, Jun 27, 2018 at 1:13 PM, Thomas Munro
>> > <thomas.munro@enterprisedb.com> wrote:
>> >> On Wed, Jun 27, 2018 at 12:16 PM, Thomas Munro
>> >> <thomas.munro@enterprisedb.com> wrote:
>> >>> I think we should take the hint in the comments and make it O(1)
>> >>> anyway.  See attached draft patch.
>> >>
>> >> Alternatively, here is a shorter and sweeter dlist version (I did the
>> >> open-coded one thinking of theoretical back-patchability).
>> >
>> > ... though, on second thoughts, the dlist version steam-rolls over the
>> > possibility that it might not be in the list (mentioned in the
>> > comments, though it's not immediately clear how that would happen).
>> >
>> > On further reflection, on the basis that it's the most conservative
>> > change, +1 for Fujii-san's close-in-reverse-order idea.  We should
>> > reconsider that data structure for 12
>>
>> +1
>>
>> Attached is v3 patch which I implemented close-in-reverse-order idea
>> on v2.
>>
>> Regards,
>>
>> --
>> Fujii Masao
>
>> --- a/src/backend/access/transam/twophase.c
>> +++ b/src/backend/access/transam/twophase.c
>> @@ -1457,6 +1457,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
>>       int                     ndelrels;
>>       SharedInvalidationMessage *invalmsgs;
>>       int                     i;
>> +     SMgrRelation *srels;
>>
>>       /*
>>        * Validate the GID, and lock the GXACT to ensure that two backends do not
>> @@ -1549,13 +1550,22 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
>>               delrels = abortrels;
>>               ndelrels = hdr->nabortrels;
>>       }
>> +
>> +     srels = palloc(sizeof(SMgrRelation) * ndelrels);
>>       for (i = 0; i < ndelrels; i++)
>> -     {
>> -             SMgrRelation srel = smgropen(delrels[i], InvalidBackendId);
>> +             srels[i] = smgropen(delrels[i], InvalidBackendId);
>>
>> -             smgrdounlink(srel, false);
>> -             smgrclose(srel);
>> -     }
>> +     smgrdounlinkall(srels, ndelrels, false);
>> +
>> +     /*
>> +      * Call smgrclose() in reverse order as when smgropen() is called.
>> +      * This trick enables remove_from_unowned_list() in smgrclose()
>> +      * to search the SMgrRelation from the unowned list,
>> +      * in O(1) performance.
>> +      */
>> +     for (i = ndelrels - 1; i >= 0; i--)
>> +             smgrclose(srels[i]);
>> +     pfree(srels);
>>
>>       /*
>>        * Handle cache invalidation messages.
>> --- a/src/backend/access/transam/xact.c
>> +++ b/src/backend/access/transam/xact.c
>> @@ -5618,6 +5618,8 @@ xact_redo_commit(xl_xact_parsed_commit *parsed,
>>       /* Make sure files supposed to be dropped are dropped */
>>       if (parsed->nrels > 0)
>>       {
>> +             SMgrRelation *srels;
>> +
>>               /*
>>                * First update minimum recovery point to cover this WAL record. Once
>>                * a relation is deleted, there's no going back. The buffer manager
>> @@ -5635,6 +5637,7 @@ xact_redo_commit(xl_xact_parsed_commit *parsed,
>>                */
>>               XLogFlush(lsn);
>>
>> +             srels = palloc(sizeof(SMgrRelation) * parsed->nrels);
>>               for (i = 0; i < parsed->nrels; i++)
>>               {
>>                       SMgrRelation srel = smgropen(parsed->xnodes[i], InvalidBackendId);
>> @@ -5642,9 +5645,20 @@ xact_redo_commit(xl_xact_parsed_commit *parsed,
>>
>>                       for (fork = 0; fork <= MAX_FORKNUM; fork++)
>>                               XLogDropRelation(parsed->xnodes[i], fork);
>> -                     smgrdounlink(srel, true);
>> -                     smgrclose(srel);
>> +                     srels[i] = srel;
>>               }
>> +
>> +             smgrdounlinkall(srels, parsed->nrels, true);
>> +
>> +             /*
>> +              * Call smgrclose() in reverse order as when smgropen() is called.
>> +              * This trick enables remove_from_unowned_list() in smgrclose()
>> +              * to search the SMgrRelation from the unowned list,
>> +              * in O(1) performance.
>> +              */
>> +             for (i = parsed->nrels - 1; i >= 0; i--)
>> +                     smgrclose(srels[i]);
>> +             pfree(srels);
>>       }
>>
>>       /*
>> @@ -5685,6 +5699,7 @@ xact_redo_abort(xl_xact_parsed_abort *parsed, TransactionId xid)
>>  {
>>       int                     i;
>>       TransactionId max_xid;
>> +     SMgrRelation *srels;
>>
>>       Assert(TransactionIdIsValid(xid));
>>
>> @@ -5748,6 +5763,7 @@ xact_redo_abort(xl_xact_parsed_abort *parsed, TransactionId xid)
>>       }
>>
>>       /* Make sure files supposed to be dropped are dropped */
>> +     srels = palloc(sizeof(SMgrRelation) * parsed->nrels);
>>       for (i = 0; i < parsed->nrels; i++)
>>       {
>>               SMgrRelation srel = smgropen(parsed->xnodes[i], InvalidBackendId);
>> @@ -5755,9 +5771,20 @@ xact_redo_abort(xl_xact_parsed_abort *parsed, TransactionId xid)
>>
>>               for (fork = 0; fork <= MAX_FORKNUM; fork++)
>>                       XLogDropRelation(parsed->xnodes[i], fork);
>> -             smgrdounlink(srel, true);
>> -             smgrclose(srel);
>> +             srels[i] = srel;
>>       }
>> +
>> +     smgrdounlinkall(srels, parsed->nrels, true);
>> +
>> +     /*
>> +      * Call smgrclose() in reverse order as when smgropen() is called.
>> +      * This trick enables remove_from_unowned_list() in smgrclose()
>> +      * to search the SMgrRelation from the unowned list,
>> +      * in O(1) performance.
>> +      */
>> +     for (i = parsed->nrels - 1; i >= 0; i--)
>> +             smgrclose(srels[i]);
>> +     pfree(srels);
>>  }
>>
>>  void
>
> Can you please move the repeated stanzy to a separate function?
> Repeating the same code and comment three times isn't good.

OK, so what about the attached patch?

Regards,

-- 
Fujii Masao