Re: Some doubious code in pgstat.c

On Thu, Nov 5, 2020 at 11:18 AM Kyotaro Horiguchi
<horikyota(dot)ntt(at)gmail(dot)com> wrote:
>
> At Wed, 4 Nov 2020 22:49:57 +0900, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote in
> > On Wed, Nov 4, 2020 at 6:49 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > On Wed, Nov 4, 2020 at 2:25 PM Kyotaro Horiguchi
> > > <horikyota(dot)ntt(at)gmail(dot)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/