From: Tom Lane <tgl@sss.pgh.pa.us>
To: "Bossart, Nathan" <bossartn@amazon.com>
cc: Michael Paquier <michael@paquier.xyz>, Andres Freund <andres@anarazel.de>,
        Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
        "sulamul@gmail.com" <sulamul@gmail.com>,
        "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
Subject: Re: inefficient loop in StandbyReleaseLockList()
In-reply-to: <0108A03D-FF2B-4CBC-933B-E047CD975158@amazon.com>
References: <CD2F0E7F-9822-45EC-A411-AE56F14DEA9F@amazon.com>
 <CAAJ_b94_awY-VfbLbzxm4zNni3NXxG3wrSkSgnHyCDqHnT+eJg@mail.gmail.com>
 <20211028.155751.1898631306946946681.horikyota.ntt@gmail.com>
 <20211028220748.6ekrl74minwq75f4@alap3.anarazel.de>
 <20211028221424.6wtcse3qkbf2cmqo@alap3.anarazel.de>
 <560364.1635459891@sss.pgh.pa.us>
 <B4C04240-1AAC-4200-9636-BCC7984688D8@amazon.com>
 <565036.1635463448@sss.pgh.pa.us>
 <20211028235248.4kbhtcqn2hvjukbd@alap3.anarazel.de>
 <YXuaLlYJxkmwnTDl@paquier.xyz>
 <0108A03D-FF2B-4CBC-933B-E047CD975158@amazon.com>
Comments: In-reply-to "Bossart, Nathan" <bossartn@amazon.com>
	message dated "Fri, 29 Oct 2021 17:30:44 -0000"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1013796.1635709095.1@sss.pgh.pa.us>
Date: Sun, 31 Oct 2021 15:38:15 -0400
Message-ID: <1013797.1635709095@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1013797.1635709095%40sss.pgh.pa.us>
Precedence: bulk

"Bossart, Nathan" <bossartn@amazon.com> writes:
> On 10/28/21, 11:53 PM, "Michael Paquier" <michael@paquier.xyz> wrote:
>> Actually, as the list of recovery locks is saved in TopMemoryContext,
>> wouldn't it be better to keep a per-cell deletion of the list, which
>> would mean that we'd better do the operation in the reverse order to
>> make things faster with the new list implementation?  But that's what
>> Andres points at with CFIs in the middle of one list of the hash table
>> processed?

> Hm.  IIUC anything bad enough to cause the startup process to break
> out of the StandbyReleaseLockList() loop will also cause the entire
> process to be restarted.  I'm not seeing any risk of reusing a half-
> released lock list.  I might be misunderstanding the concern, though.

Yeah, there's no expectation that this data structure needs to be kept
consistent after an error; and I'm not real sure that the existing
code could claim to satisfy such a requirement if we did need it.
(There's at least a short window where the caller's hash table entry
will point at an already-freed List.)

Pushed the patch as given.  I've not yet reviewed other list_delete_first
callers, but I'll take a look.  (I seem to remember that I did survey
them while writing 1cff1b95a, but I evidently missed that this code
could be dealing with a list long enough to be problematic.)

			regards, tom lane