elog(DEBUG2 in SpinLocked section.

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

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

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

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

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

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

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

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

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

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

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

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

... and InvalidateObsoleteReplicationSlots(), too.

I am detecting a pattern here.

regards, tom lane

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

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]/messages/by-id/1141819.1591208385@sss.pgh.pa.us.

regards, tom lane

[1]: /messages/by-id/1141819.1591208385@sss.pgh.pa.us

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

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

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

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