On Friday, November 25, 2022 5:43 AM Peter Smith <smithpb2250@gmail.com> wrote:
> 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.
Hi, thank you for sharing this info.
I searched the codes where I could feel the merits to add "ms"
at the end of the variable names.
Adding the unites would help to calculate or convert some time related values.
In this patch there is only a couple of functions, like maybe_delay_apply()
or for conversion of time, parse_subscription_options.
I feel changing just a couple of structures might be awkward,
while changing all internal structures is too much. So, I keep the names
as those were after some modifications shared in [1].
If you have any better idea, please let me know.
[1] -
https://www.postgresql.org/message-id/TYCPR01MB83730C23CB7D29E57368BECDEDE29%40TYCPR01MB8373.jpnprd01.prod.outlook.com
Best Regards,
Takamichi Osumi
