Re: Speedup of relation deletes during recovery

From: Andres Freund <andres(at)anarazel(dot)de>
To: Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
Cc: Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Robert Haas <robertmhaas(at)gmail(dot)com>
Subject: Re: Speedup of relation deletes during recovery
Date: 2018-06-27 18:23:37
Message-ID: 20180627182337.ajgzmyd37msrfk46@alap3.anarazel.de
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Daniel Gustafsson 2018-06-27 18:57:43 Re: [HACKERS] Support for Secure Transport SSL library on macOS as OpenSSL alternative
Previous Message Fujii Masao 2018-06-27 18:21:51 Re: Speedup of relation deletes during recovery