On 2020-Sep-04, Tom Lane wrote: > While playing around with clang's scan-build I noticed this warning: > > worker.c:2281:7: warning: Value stored to 'ping_sent' is never read > ping_sent = true; > ^ ~~~~ > > At first I thought it was a harmless unnecessary update, but looking > closer I wonder whether it isn't telling us there is a logic bug here. > Specifically, I wonder why the "ping_sent" variable is local to the > loop starting at line 2084, rather than having the same lifespan as > "last_recv_timestamp". Do we really want to forget that we sent a > ping anytime we have to wait for more data?
Ah ... maybe this bug is the reason why the bug fixed by 470687b4a5bb did not affect logical replication. > In short, we could remove the ping_sent variable entirely without > changing this code's behavior. I'm not 100% sure what semantics > it's trying to achieve, but I don't think it's achieving them. I imagine that moving the variable one block-scope outwards (together with last_recv_timestamp) is what was intended. ... oh look! commit 3f60f690fac1 moved last_recv_timestamp without realizing that ping_sent had to get the same treatment. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
