Re: Speedup of relation deletes during recovery

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.

Greetings,

Andres Freund