On Fri, Nov 25, 2022 at 2:15 AM Takamichi Osumi (Fujitsu)
<osumi.takamichi@fujitsu.com> wrote:
>
> On Wednesday, October 5, 2022 6:42 PM Peter Smith <smithpb2250@gmail.com> wrote:
...
>
> > ======
> >
> > 5. src/backend/commands/subscriptioncmds.c - SubOpts
> >
> > @@ -89,6 +91,7 @@ typedef struct SubOpts
> > bool disableonerr;
> > char *origin;
> > XLogRecPtr lsn;
> > + int64 min_apply_delay;
> > } SubOpts;
> >
> > I feel it would be better to be explicit about the storage units. So call this
> > member ‘min_apply_delay_ms’. E.g. then other code in
> > parse_subscription_options will be more natural when you are converting using
> > and assigning them to this member.
> I don't think we use such names including units explicitly.
> Could you please tell me a similar example for this ?
>
Regex search "\..*_ms[e\s]" finds some members where the unit is in
the member name.
e.g. delay_ms (see EnableTimeoutParams in timeout.h)
e.g. interval_in_ms (see timeout_paramsin timeout.c)
Regex search ".*_ms[e\s]" finds many local variables where the unit is
in the variable name
> > ======
> >
> > 16. src/include/catalog/pg_subscription.h
> >
> > + int64 subapplydelay; /* Replication apply delay */
> > +
> >
> > Consider renaming this as 'subapplydelayms' to make the units perfectly clear.
> Similar to the 5th comments, I can't find any examples for this.
> I'd like to keep it general, which makes me feel it is more aligned with
> existing codes.
>
As above.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
