On Fri, Sep 24, 2021 at 5:53 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Tuesday, September 21, 2021 12:53 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
> >
> > I've attached the updated version patches. Please review them.
>
> Thanks for updating the patch,
> here are a few comments on the v14-0001 patch.
Thank you for the comments!
>
> 1)
> + hash_ctl.keysize = sizeof(Oid);
> + hash_ctl.entrysize = sizeof(SubscriptionRelState);
> + not_ready_rels_htab = hash_create("not ready relations in subscription",
> + 64,
> + &hash_ctl,
> + HASH_ELEM |
HASH_BLOBS);
> +
>
> ISTM we can pass list_length(not_ready_rels_list) as the nelem to hash_create.
As Amit pointed out, it seems not necessary to build a temporary hash
table for this purpose.
>
> 2)
>
> + /*
> + * Search for all the dead subscriptions and error entries in stats
> + * hashtable and tell the stats collector to drop them.
> + */
> + if (subscriptionHash)
> + {
> ...
> + HTAB *htab;
> +
>
> It seems we already delacre a "HTAB *htab;" in function pgstat_vacuum_stat(),
> can we use the existing htab here ?
Right. Will remove it.
>
>
> 3)
>
> PGSTAT_MTYPE_RESETREPLSLOTCOUNTER,
> + PGSTAT_MTYPE_SUBSCRIPTIONERR,
> + PGSTAT_MTYPE_SUBSCRIPTIONERRRESET,
> + PGSTAT_MTYPE_SUBSCRIPTIONERRPURGE,
> + PGSTAT_MTYPE_SUBSCRIPTIONPURGE,
> PGSTAT_MTYPE_AUTOVAC_START,
>
> Can we append these values at the end of the Enum struct which won't affect the
> other Enum values.
Yes, I'll move them to the end of the Enum struct.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
