Horton

Thread

Commits

  1. Fix instance of elog() called while holding a spinlock

  2. Don't call palloc() while holding a spinlock, either.

  3. Don't call elog() while holding spinlock.

  1. elog(DEBUG2 in SpinLocked section.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-06-02T07:15:18Z

    Hello.

I noticed that UpdateSpillStats calls "elog(DEBUG2" within
SpinLockAcquire section on MyWalSnd.  The lock doesn't protect rb and
in the first place the rb cannot be modified during the function is
running.

It should be out of the lock section.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  2. Re: elog(DEBUG2 in SpinLocked section.

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-06-02T08:35:48Z

    
On 2020/06/02 16:15, Kyotaro Horiguchi wrote:
> Hello.
> 
> I noticed that UpdateSpillStats calls "elog(DEBUG2" within
> SpinLockAcquire section on MyWalSnd.  The lock doesn't protect rb and
> in the first place the rb cannot be modified during the function is
> running.
> 
> It should be out of the lock section.

Thanks for the patch! It looks good to me.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  3. Re: elog(DEBUG2 in SpinLocked section.

    Amit Kapila <amit.kapila16@gmail.com> — 2020-06-02T08:42:37Z

    On Tue, Jun 2, 2020 at 2:05 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> On 2020/06/02 16:15, Kyotaro Horiguchi wrote:
> > Hello.
> >
> > I noticed that UpdateSpillStats calls "elog(DEBUG2" within
> > SpinLockAcquire section on MyWalSnd.  The lock doesn't protect rb and
> > in the first place the rb cannot be modified during the function is
> > running.
> >
> > It should be out of the lock section.

Right.

>
> Thanks for the patch! It looks good to me.
>

The patch looks good to me as well.  I will push this unless Fujii-San
wants to do it.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  4. Re: elog(DEBUG2 in SpinLocked section.

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-06-02T10:24:16Z

    
On 2020/06/02 17:42, Amit Kapila wrote:
> On Tue, Jun 2, 2020 at 2:05 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>> On 2020/06/02 16:15, Kyotaro Horiguchi wrote:
>>> Hello.
>>>
>>> I noticed that UpdateSpillStats calls "elog(DEBUG2" within
>>> SpinLockAcquire section on MyWalSnd.  The lock doesn't protect rb and
>>> in the first place the rb cannot be modified during the function is
>>> running.
>>>
>>> It should be out of the lock section.
> 
> Right.
> 
>>
>> Thanks for the patch! It looks good to me.
>>
> 
> The patch looks good to me as well.  I will push this unless Fujii-San
> wants to do it.

Thanks! I pushed the patch.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

  5. Re: elog(DEBUG2 in SpinLocked section.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-06-03T00:18:19Z

    At Tue, 2 Jun 2020 19:24:16 +0900, Fujii Masao <masao.fujii@oss.nttdata.com> wrote in 
> Thanks! I pushed the patch.

Thanks to all!

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  6. Re: elog(DEBUG2 in SpinLocked section.

    Michael Paquier <michael@paquier.xyz> — 2020-06-03T03:05:10Z

    On Wed, Jun 03, 2020 at 09:18:19AM +0900, Kyotaro Horiguchi wrote:
> Thanks to all!

Indeed, this was incorrect.  And you may not have noticed, but we have
a second instance of that in LogicalIncreaseRestartDecodingForSlot()
that goes down to 9.4 and b89e151.  I used a dirty-still-efficient
hack to detect that, and that's the only instance I have spotted.

I am not sure if that's worth worrying a back-patch, but we should
really address that at least on HEAD.  Attached is an extra patch to
close the loop.
--
Michael

  7. Re: elog(DEBUG2 in SpinLocked section.

    Amit Kapila <amit.kapila16@gmail.com> — 2020-06-03T03:22:08Z

    On Wed, Jun 3, 2020 at 8:35 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jun 03, 2020 at 09:18:19AM +0900, Kyotaro Horiguchi wrote:
> > Thanks to all!
>
> Indeed, this was incorrect.
>

Do you mean to say correct?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  8. Re: elog(DEBUG2 in SpinLocked section.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-03T04:36:34Z

    Michael Paquier <michael@paquier.xyz> writes:
> Indeed, this was incorrect.  And you may not have noticed, but we have
> a second instance of that in LogicalIncreaseRestartDecodingForSlot()
> that goes down to 9.4 and b89e151.  I used a dirty-still-efficient
> hack to detect that, and that's the only instance I have spotted.

Ugh, that is just horrid.  I experimented with the attached patch
but it did not find any other problems.  Still, that only proves
something about code paths that are taken during check-world, and
we know that our test coverage is not very good :-(.

Should we think about adding automated detection of this type of
mistake?  I don't like the attached as-is because of the #include
footprint expansion, but maybe we can find a better way.

> I am not sure if that's worth worrying a back-patch, but we should
> really address that at least on HEAD.

It's actually worse in the back branches, because elog() did not have
a good short-circuit path like ereport() does.  +1 for back-patch.

			regards, tom lane

  9. Re: elog(DEBUG2 in SpinLocked section.

    Michael Paquier <michael@paquier.xyz> — 2020-06-03T04:41:21Z

    On Wed, Jun 03, 2020 at 08:52:08AM +0530, Amit Kapila wrote:
> Do you mean to say correct?

Nope, I really meant that the code before caa3c42 is incorrect, and I
am glad that it got fixed.  Sorry if that sounded confusing.
--
Michael

  10. Re: elog(DEBUG2 in SpinLocked section.

    Michael Paquier <michael@paquier.xyz> — 2020-06-03T04:48:59Z

    On Wed, Jun 03, 2020 at 12:36:34AM -0400, Tom Lane wrote:
> Ugh, that is just horrid.  I experimented with the attached patch
> but it did not find any other problems.

Oh.  I can see the same "ifndef FRONTEND" logic all around the place
as I did on my local branch :)

> Still, that only proves something about code paths that are taken
> during check-world, and we know that our test coverage is not very
> good :-(.

Yeah.  Not perfect, still we are getting better at it with the years.
I am fine to take care of a backpatch, but I'll wait first a bit to
see if others have any comments.
--
Michael

  11. Re: elog(DEBUG2 in SpinLocked section.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-03T05:27:51Z

    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

  12. Re: elog(DEBUG2 in SpinLocked section.

    Michael Paquier <michael@paquier.xyz> — 2020-06-03T05:47:48Z

    On Wed, Jun 03, 2020 at 01:27:51AM -0400, Tom Lane wrote:
> 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.

+1.  And I guess that you are already on that?  ;)
--
Michael

  13. Re: elog(DEBUG2 in SpinLocked section.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-03T05:54:23Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jun 03, 2020 at 01:27:51AM -0400, Tom Lane wrote:
>> 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.

> +1.  And I guess that you are already on that?  ;)

I'll work on it tomorrow ... it's getting late here.

			regards, tom lane

  14. Re: elog(DEBUG2 in SpinLocked section.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-03T06:00:53Z

    ... and InvalidateObsoleteReplicationSlots(), too.

I am detecting a pattern here.

			regards, tom lane

  15. Re: elog(DEBUG2 in SpinLocked section.

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> — 2020-06-03T06:18:19Z

    At Wed, 03 Jun 2020 02:00:53 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in 
> ... and InvalidateObsoleteReplicationSlots(), too.
> 
> I am detecting a pattern here.

I looked through 224 locations where SpinLockAcquire and found some.

LogicalIncreaseRestartDecodingForSlot is spotted by Michael.
pg_get_replication_slots has some namecpy as Tom pointed out.
copy_replication_slot has pstrdup as Tom pointed out.
InvalidateObsoleteReplicationSlots has pstrdup as Tom poineed out.

I found another instance of pstrdup, but found some string copy functions.

CreateInitDecodingContext has StrNCpy (up to NAMEDATALEN = 64 bytes).
RequestXLogStreaming has strlcpy (up to MAXCONNINFO = 1024 bytes).
SaveSlotToPath has memcpy on ReplicationSlotOnDisk (176 bytes).
WalReceiverMain has strlcpy(MAXCONINFO + NAMEDATALEN) and memset of MAXCONNINFO.
pg_stat_get_wal_receiver has strlcpy (NAMEDATALEN + NI_MAXHOST(1025) + MAXCONNINFO).

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

  16. Re: elog(DEBUG2 in SpinLocked section.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-03T18:35:29Z

    Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> I looked through 224 locations where SpinLockAcquire and found some.

Yeah, I made a similar scan and arrived at about the same conclusions.
I think that the memcpy and strlcpy calls are fine; at least, we've got
to transport data somehow and it's not apparent why those aren't OK ways
to do it.  The one use of StrNCpy is annoying from a cosmetic standpoint
(mainly because it's Not Like Anywhere Else) but I'm not sure it's worth
changing.

The condition-variable code has a boatload of spinlocked calls of the
proclist functions in proclist.h.  All of those are straight-line code
so they're okay performance wise, but I wonder if we shouldn't add a
comment to that header pointing out that its functions must not throw
errors.

The only other thing I remain concerned about is some instances of atomic
operations inside spinlocks, which I started a separate thread about [1].

			regards, tom lane

[1] https://www.postgresql.org/message-id/1141819.1591208385%40sss.pgh.pa.us

  17. Re: elog(DEBUG2 in SpinLocked section.

    Michael Paquier <michael@paquier.xyz> — 2020-06-04T01:43:56Z

    On Wed, Jun 03, 2020 at 12:36:34AM -0400, Tom Lane wrote:
> Should we think about adding automated detection of this type of
> mistake?  I don't like the attached as-is because of the #include
> footprint expansion, but maybe we can find a better way.

I think that this one first boils down to the FRONTEND dependency in
those headers.  Or in short, spin.h may get loaded by the frontend but
we have a backend-only API, no?

> It's actually worse in the back branches, because elog() did not have
> a good short-circuit path like ereport() does.  +1 for back-patch.

Thanks, got that fixed down to 9.5.
--
Michael

  18. Re: elog(DEBUG2 in SpinLocked section.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-04T01:57:33Z

    Michael Paquier <michael@paquier.xyz> writes:
> On Wed, Jun 03, 2020 at 12:36:34AM -0400, Tom Lane wrote:
>> Should we think about adding automated detection of this type of
>> mistake?  I don't like the attached as-is because of the #include
>> footprint expansion, but maybe we can find a better way.

> I think that this one first boils down to the FRONTEND dependency in
> those headers.  Or in short, spin.h may get loaded by the frontend but
> we have a backend-only API, no?

I think the #include bloat comes from wanting to declare the global
state variable as "slock_t *".  We could give up on that and write
something like this in a central place like c.h:

#if defined(USE_ASSERT_CHECKING) && !defined(FRONTEND)
extern void *held_spinlock;
#define NotHoldingSpinLock() Assert(held_spinlock == NULL)
#else
#define NotHoldingSpinLock() ((void) 0)
#endif

Then throwing NotHoldingSpinLock() into relevant places costs
nothing new include-wise.

			regards, tom lane

  19. Re: elog(DEBUG2 in SpinLocked section.

    Robert Haas <robertmhaas@gmail.com> — 2020-06-09T17:46:28Z

    On Wed, Jun 3, 2020 at 12:36 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Should we think about adding automated detection of this type of
> mistake?  I don't like the attached as-is because of the #include
> footprint expansion, but maybe we can find a better way.

I think it would be an excellent idea.

Removing some of these spinlocks and replacing them with LWLocks might
also be worth considering.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  20. Re: elog(DEBUG2 in SpinLocked section.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-09T17:59:44Z

    Robert Haas <robertmhaas@gmail.com> writes:
> Removing some of these spinlocks and replacing them with LWLocks might
> also be worth considering.

When I went through the existing spinlock stanzas, the only thing that
really made me acutely uncomfortable was the chunk in pg_stat_statement's
pgss_store(), lines 1386..1438 in HEAD.  In the first place, that's
pushing the notion of "short straight-line code" well beyond reasonable
bounds.  Other processes could waste a fair amount of time spinning while
the lock holder does all this arithmetic; not to mention the risk of
exhausting one's CPU time-slice partway through.  In the second place,
a chunk of code this large could well allow people to make modifications
without noticing that they're inside a spinlock, allowing future coding
violations to sneak in.

Not sure what we want to do about it though.  An LWLock per pgss entry
probably isn't gonna do.  Perhaps we could take a cue from your old
hack with multiplexed spinlocks, and map the pgss entries onto some
fixed-size pool of LWLocks, figuring that the odds of false conflicts
are small as long as the pool is bigger than MaxBackends.

			regards, tom lane

  21. Re: elog(DEBUG2 in SpinLocked section.

    Robert Haas <robertmhaas@gmail.com> — 2020-06-09T19:20:08Z

    On Tue, Jun 9, 2020 at 1:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > Removing some of these spinlocks and replacing them with LWLocks might
> > also be worth considering.
>
> When I went through the existing spinlock stanzas, the only thing that
> really made me acutely uncomfortable was the chunk in pg_stat_statement's
> pgss_store(), lines 1386..1438 in HEAD.  In the first place, that's
> pushing the notion of "short straight-line code" well beyond reasonable
> bounds.  Other processes could waste a fair amount of time spinning while
> the lock holder does all this arithmetic; not to mention the risk of
> exhausting one's CPU time-slice partway through.  In the second place,
> a chunk of code this large could well allow people to make modifications
> without noticing that they're inside a spinlock, allowing future coding
> violations to sneak in.
>
> Not sure what we want to do about it though.  An LWLock per pgss entry
> probably isn't gonna do.  Perhaps we could take a cue from your old
> hack with multiplexed spinlocks, and map the pgss entries onto some
> fixed-size pool of LWLocks, figuring that the odds of false conflicts
> are small as long as the pool is bigger than MaxBackends.

I mean, what would be wrong with having an LWLock per pgss entry? If
you're worried about efficiency, it's no longer the case that an
LWLock uses a spinlock internally, so there's not the old problem of
doubling (plus contention) the number of atomic operations by using an
LWLock. If you're worried about space, an LWLock is only 16 bytes, and
the slock_t that we'd be replacing is currently at the end of the
struct so presumably followed by some padding.

I suspect that these days many of the places we're using spinlocks are
buying little of any value on the efficiency side, but making any
high-contention scenarios way worse. Plus, unlike LWLocks, they're not
instrumented with wait events, so you can't even find out that you've
got contention there without breaking out 'perf', not exactly a great
thing to have to do in a production environments.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  22. Re: elog(DEBUG2 in SpinLocked section.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-09T23:24:15Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, Jun 9, 2020 at 1:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> When I went through the existing spinlock stanzas, the only thing that
>> really made me acutely uncomfortable was the chunk in pg_stat_statement's
>> pgss_store(), lines 1386..1438 in HEAD.

> I mean, what would be wrong with having an LWLock per pgss entry?

Hmm, maybe nothing.  I'm accustomed to thinking of them as being
significantly more expensive than spinlocks, but maybe we've narrowed
the gap enough that that's not such a problem.

			regards, tom lane

  23. Re: elog(DEBUG2 in SpinLocked section.

    Andres Freund <andres@anarazel.de> — 2020-06-09T23:54:15Z

    Hi,

On 2020-06-09 19:24:15 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Tue, Jun 9, 2020 at 1:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> When I went through the existing spinlock stanzas, the only thing that
> >> really made me acutely uncomfortable was the chunk in pg_stat_statement's
> >> pgss_store(), lines 1386..1438 in HEAD.
> 
> > I mean, what would be wrong with having an LWLock per pgss entry?

+1

> Hmm, maybe nothing.  I'm accustomed to thinking of them as being
> significantly more expensive than spinlocks, but maybe we've narrowed
> the gap enough that that's not such a problem.

They do add a few cycles (IIRC ~30 or so, last time I measured a
specific scenario) of latency to acquisition, but it's not a large
amount. The only case where acquisition is noticably slower, in my
experiments, is when there's "just the right amount" of
contention. There spinning instead of entering the kernel can be good.

I've mused about adding a small amount of spinning to lwlock acquisition
before. But so far working on reducing contention seemed the better
route.


Funnily enough lwlock *release*, even when there are no waiters, has a
somewhat noticable performance difference on x86 (and other TSO
platforms) compared to spinlock release. For spinlock release we can
just use a plain write and a compiler barrier, whereas lwlock release
needs to use an atomic operation.

I think that's hard, but not impossible, to avoid for an userspace
reader-writer lock.


It would be a nice experiment to make spinlocks a legacy wrapper around
rwlocks. I think if we added 2-3 optimizations (optimize for
exclusive-only locks, short amount of spinning, possibly inline
functions for "fast path" acquisitions/release) that'd be better for
nearly all situations. And in the situations where it's not, the loss
would be pretty darn small.

Greetings,

Andres Freund

  24. Re: elog(DEBUG2 in SpinLocked section.

    Andres Freund <andres@anarazel.de> — 2020-06-10T00:12:54Z

    Hi,

On 2020-06-09 15:20:08 -0400, Robert Haas wrote:
> If you're worried about space, an LWLock is only 16 bytes, and the
> slock_t that we'd be replacing is currently at the end of the struct
> so presumably followed by some padding.

I don't think the size is worth of concern in this case, and I'm not
sure there's any current case where it's really worth spending effort
reducing size. But if there is: It seems possible to reduce the size.

struct LWLock {
        uint16                     tranche;              /*     0     2 */

        /* XXX 2 bytes hole, try to pack */

        pg_atomic_uint32           state;                /*     4     4 */
        proclist_head              waiters;              /*     8     8 */

        /* size: 16, cachelines: 1, members: 3 */
        /* sum members: 14, holes: 1, sum holes: 2 */
        /* last cacheline: 16 bytes */
};

First, we could remove the tranche from the lwlock, and instead perform
more work when we need to know it. Which is only when we're going to
sleep, so it'd be ok if it's not that much work. Perhaps we could even
defer determining the tranche to the the *read* side of the wait event
(presumably that'd require making the pgstat side a bit more
complicated).

Second, it seems like it should be doable to reduce the size of the
waiters list. We e.g. could have a separate 'array of wait lists' array
in shared memory, which gets assigned to an lwlock whenever a backend
wants to wait for an lwlock. The number of processes waiting for lwlocks
is clearly limited by MAX_BACKENDS / 2^18-1 backends waiting, so one 4
byte integer pointing to a wait list obviously would suffice.

But again, I'm not sure the current size a real problem anywhere.

Greetings,

Andres Freund

  25. Re: elog(DEBUG2 in SpinLocked section.

    Robert Haas <robertmhaas@gmail.com> — 2020-06-10T14:45:37Z

    On Tue, Jun 9, 2020 at 8:12 PM Andres Freund <andres@anarazel.de> wrote:
> I don't think the size is worth of concern in this case, and I'm not
> sure there's any current case where it's really worth spending effort
> reducing size. But if there is: It seems possible to reduce the size.

Yeah, I don't think it's very important.

> First, we could remove the tranche from the lwlock, and instead perform
> more work when we need to know it. Which is only when we're going to
> sleep, so it'd be ok if it's not that much work. Perhaps we could even
> defer determining the tranche to the the *read* side of the wait event
> (presumably that'd require making the pgstat side a bit more
> complicated).
>
> Second, it seems like it should be doable to reduce the size of the
> waiters list. We e.g. could have a separate 'array of wait lists' array
> in shared memory, which gets assigned to an lwlock whenever a backend
> wants to wait for an lwlock. The number of processes waiting for lwlocks
> is clearly limited by MAX_BACKENDS / 2^18-1 backends waiting, so one 4
> byte integer pointing to a wait list obviously would suffice.
>
> But again, I'm not sure the current size a real problem anywhere.

Honestly, both of these sound more painful than it's worth. We're not
likely to have enough LWLocks that using 16 bytes for each one rather
than 8 is a major problem. With regard to the first of these ideas,
bear in mind that the LWLock might be in a DSM segment that the reader
doesn't have mapped.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  26. Re: elog(DEBUG2 in SpinLocked section.

    Andres Freund <andres@anarazel.de> — 2020-06-16T23:31:05Z

    Hi,

On 2020-06-03 00:36:34 -0400, Tom Lane wrote:
> Should we think about adding automated detection of this type of
> mistake?  I don't like the attached as-is because of the #include
> footprint expansion, but maybe we can find a better way.

I experimented with making the compiler warn about about some of these
kinds of mistakes without needing full test coverage:

I was able to get clang to warn about things like using palloc in signal
handlers, or using palloc while holding a spinlock. Which would be
great, except that it doesn't warn when there's an un-annotated
intermediary function. Even when that function is in the same TU.

Here's my attempt: https://godbolt.org/z/xfa6Es

It does detect things like
    spinlock_lock();
    example_alloc(17);
    spinlock_unlock();

<source>:49:2: warning: cannot call function 'example_alloc' while mutex 'holding_spinlock' is held [-Wthread-safety-analysis]

        example_alloc(17);

        ^

which isn't too bad.

Does anybody think this would be useful even if it doesn't detect the
more complicated cases?

Greetings,

Andres Freund

  27. Re: elog(DEBUG2 in SpinLocked section.

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-06-16T23:46:29Z

    Andres Freund <andres@anarazel.de> writes:
> I experimented with making the compiler warn about about some of these
> kinds of mistakes without needing full test coverage:

> I was able to get clang to warn about things like using palloc in signal
> handlers, or using palloc while holding a spinlock. Which would be
> great, except that it doesn't warn when there's an un-annotated
> intermediary function. Even when that function is in the same TU.

Hm.  Couldn't we make "calling an un-annotated function" be a violation
in itself?  Certainly in the case of spinlocks, what we want is pretty
nearly a total ban on calling anything at all.  I wouldn't cry too hard
about having a similar policy for signal handlers.  (The postmaster's
handlers would have to be an exception for now.)

			regards, tom lane

  28. Re: elog(DEBUG2 in SpinLocked section.

    Andres Freund <andres@anarazel.de> — 2020-06-17T00:27:36Z

    Hi,

On 2020-06-16 19:46:29 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I experimented with making the compiler warn about about some of these
> > kinds of mistakes without needing full test coverage:
>
> > I was able to get clang to warn about things like using palloc in signal
> > handlers, or using palloc while holding a spinlock. Which would be
> > great, except that it doesn't warn when there's an un-annotated
> > intermediary function. Even when that function is in the same TU.
>
> Hm.  Couldn't we make "calling an un-annotated function" be a violation
> in itself?

I don't see a way to do that with these annotations, unfortunately.

https://clang.llvm.org/docs/ThreadSafetyAnalysis.html
https://clang.llvm.org/docs/AttributeReference.html#acquire-capability-acquire-shared-capability


> Certainly in the case of spinlocks, what we want is pretty
> nearly a total ban on calling anything at all.  I wouldn't cry too hard
> about having a similar policy for signal handlers.

It'd be interesting to try and see how invasive that'd be, if it were
possible to enforce. But...

Greetings,

Andres Freund