Re: Some doubious code in pgstat.c

Masahiko Sawada <sawada.mshk@gmail.com>

From: Masahiko Sawada <sawada.mshk@gmail.com>
To: Amit Kapila <amit.kapila16@gmail.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2020-11-04T13:49:57Z
Lists: pgsql-hackers
On Wed, Nov 4, 2020 at 6:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Nov 4, 2020 at 2:25 PM Kyotaro Horiguchi
> <horikyota.ntt@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?

Regards,

-- 
Masahiko Sawada
EnterpriseDB:  https://www.enterprisedb.com/



Commits

  1. Use strlcpy instead of memcpy for copying the slot name in pgstat.c.

  2. Rearrange pgstat_bestart() to avoid failures within its critical section.