Re: Excessive CPU usage in StandbyReleaseLocks()

Adding Simon and David R.

On Wed, Jun 20, 2018 at 5:44 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Hence, I have a modest proposal: use elog(LOG) followed by Assert(false),
> so that debug builds report such problems loudly but production builds
> do not. We've done that before, see e.g. load_relcache_init_file around
> relcache.c:5718.

Done. (This elog(LOG) isn't new BTW, I merely moved it.)

On Tue, Jun 19, 2018 at 6:30 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2018-06-19 17:43:42 +1200, Thomas Munro wrote:
>> + RecoveryLockLists = hash_create("RecoveryLockLists",
>> + 4096,
>> + &hash_ctl,
>
> Isn't that initial size needlessly big?

Changed to 64.

>> - RecoveryLockList = lappend(RecoveryLockList, newlock);
>> + entry->locks = lappend(entry->locks, newlock);
>
> Gotta be careful with lappend et al - it's really easy to leak memory
> without explicit context usage. Have you checked that we don't here?

The list and contents are explicitly freed when xids end and the locks
are released (because xid committed/aborted, or older than known
oldest running transaction, or we release all on promotion). The
existing code seems to assume that TopMemoryContext is current (or
some context that'll last all the way until our promotion), which
seems OK to me. TopMemoryContext is inherited from PostgresMain() and
any redo handler that changes it without restoring it would be buggy,
and errors are FATAL here.

I had missed one small thing though: I should call hash_destroy() in
ShutdownRecoveryTransactionEnvironment() to free the hash table itself
on promotion. Fixed.

Why does StandbyReleaseLocks() handle an invalid xid by removing all
locks (if (!TransactionIdIsValid(xid) || lock->xid == xid)) in master?

V2 attached.

--
Thomas Munro
http://www.enterprisedb.com