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(dot)munro(at)enterprisedb(dot)com> wrote:
> > On Wed, Jun 27, 2018 at 1:13 PM, Thomas Munro
> > <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
> >> On Wed, Jun 27, 2018 at 12:16 PM, Thomas Munro
> >> <thomas(dot)munro(at)enterprisedb(dot)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