Questionable ping logic in LogicalRepApplyLoop
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?
In fact, looking closer, it appears to me that
(1) The "ping_sent = false" at line 2124 is also dead code, because
ping_sent could never be anything but false at this point;
(2) The "if (!ping_sent)" at line 2274 is also dead code, because
ping_sent is still never anything but false at that point.
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.
regards, tom lane
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
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
... oh look! commit 3f60f690fac1 moved last_recv_timestamp without
realizing that ping_sent had to get the same treatment.
Hah, I wondered if something like that had happened, but I didn't
get around to excavating in the git history yet. Thanks for doing so.
Will push a fix later.
regards, tom lane