On Fri, Nov 25, 2022 at 2:15 AM Takamichi Osumi (Fujitsu) <osumi.takami...@fujitsu.com> wrote: > > On Wednesday, October 5, 2022 6:42 PM Peter Smith <smithpb2...@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