Re: elog(DEBUG2 in SpinLocked section.

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>, masao(dot)fujii(at)oss(dot)nttdata(dot)com, amit(dot)kapila16(at)gmail(dot)com, pasim(at)vmware(dot)com, pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: elog(DEBUG2 in SpinLocked section.
Date: 2020-06-03 05:27:51
Message-ID: 972559.1591162071@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I wrote:
> Ugh, that is just horrid. I experimented with the attached patch
> but it did not find any other problems.

It occurred to me to add NotHoldingSpinLock() into palloc and
friends, and look what I found in copy_replication_slot:

SpinLockAcquire(&s->mutex);
src_islogical = SlotIsLogical(s);
src_restart_lsn = s->data.restart_lsn;
temporary = s->data.persistency == RS_TEMPORARY;
plugin = logical_slot ? pstrdup(NameStr(s->data.plugin)) : NULL;
SpinLockRelease(&s->mutex);

That is not gonna do, of course. And there is another pstrdup
inside another spinlock section a bit further down in the same
function. Also, pg_get_replication_slots has a couple of
namecpy() calls inside a spinlock, which is maybe less dangerous
than palloc() but it's still willful disregard of the project coding
rule about "only straight-line code inside a spinlock".

I'm inclined to think that memcpy'ing the ReplicationSlot struct
into a local variable might be the best way, replacing all the
piecemeal copying these stanzas are doing right now. memcpy() of
a fixed amount of data isn't quite straight-line code perhaps,
but it has a well-defined runtime and zero chance of throwing an
error, which are the two properties we should be most urgently
concerned about.

regards, tom lane

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2020-06-03 05:47:48 Re: elog(DEBUG2 in SpinLocked section.
Previous Message Michael Paquier 2020-06-03 05:26:07 Re: OpenSSL 3.0.0 compatibility