Re: Speedup of relation deletes during recovery

On Thu, Jun 28, 2018 at 3:23 AM, Andres Freund <andres(at)anarazel(dot)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(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.

OK, so what about the attached patch?

Regards,

--
Fujii Masao