On Thu, Nov 5, 2020 at 11:18 AM Kyotaro Horiguchi <horikyota....@gmail.com> wrote: > > At Wed, 4 Nov 2020 22:49:57 +0900, Masahiko Sawada <sawada.m...@gmail.com> > wrote in > > On Wed, Nov 4, 2020 at 6:49 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > > > On Wed, Nov 4, 2020 at 2:25 PM Kyotaro Horiguchi > > > <horikyota....@gmail.com> wrote: > > > > > > > > Hello. > > > > > > > > While updating a patch, I noticed that the replication slot stats > > > > patch (9868167500) put some somewhat doubious codes. > > > > > > > > In pgstat_recv_replslot, an assertion like the following exists: > > > > > > > > > idx = pgstat_replslot_index(msg->m_slotname, !msg->m_drop); > > > > .. > > > > > Assert(idx >= 0 && idx < max_replication_slots); > > > > > > > > But the idx should be 0..(max_replication_slots - 1). > > > > > > > > > > Right. > > > > > > > > > > > In the same function the following code assumes that the given "char > > > > *name" has the length of NAMEDATALEN. It actually is, but that > > > > assumption seems a bit bogus. I think it should use strlcpy instead. > > > > > > > > > > Agreed. > > > > +1 > > > > The commit uses memcpy in the same way in other places too, for > > instance in pgstat_report_replslot_drop(). Should we fix all of them? > > Absolutely. the same is seen at several places. Please find the > attached. > > As another issue, just replace memcpy with strlcpy makes compiler > complain of type mismatch, as the first paramter to memcpy had an > needless "&" operator. I removed it in this patch. > > (&msg.m_slotname is a "char (*)[NAMEDATALEN]", not a "char *".) >
The patch looks good to me. Regards, -- Masahiko Sawada EnterpriseDB: https://www.enterprisedb.com/
