Dear Peter, Thank you for reviewing! PSA new version.
> ====== > Commit Message > > 1. > Discussion: > https://postgr.es/m/CAB-JLwYOYwL=XTyAXKiH5CtM_Vm8KjKh7aaitCKvmCh4r > zr...@mail.gmail.com > > tmp > > ~ > > What's that "tmp" doing there? A typo? Removed. It was a typo. I used `git rebase` command to combining the local commits, but the commit message seemed to be remained. > ====== > doc/src/sgml/catalogs.sgml > > 2. > + <row> > + <entry role="catalog_table_entry"><para role="column_definition"> > + <structfield>subminapplydelay</structfield> <type>int4</type> > + </para> > + <para> > + The minimum delay (ms) for applying changes. > + </para></entry> > + </row> > > For consistency remove the period (.) because the other > single-sentence descriptions on this page do not have one. I have also confirmed and agreed. Fixed. > ====== > src/backend/commands/subscriptioncmds.c > > 3. AlterSubscription > + errmsg("cannot set parallel streaming mode for subscription with %s", > + "min_apply_delay")); > > Since there are no translator considerations here why not write it like this: > > errmsg("cannot set parallel streaming mode for subscription with > min_apply_delay") Fixed. > ~~~ > > 4. AlterSubscription > + errmsg("cannot set %s for subscription in parallel streaming mode", > + "min_apply_delay")); > > Since there are no translator considerations here why not write it like this: > > errmsg("cannot set min_apply_delay for subscription in parallel streaming > mode") Fixed. > ~~~ > > 5. > +defGetMinApplyDelay(DefElem *def) > +{ > + char *input_string; > + int result; > + const char *hintmsg; > + > + input_string = defGetString(def); > + > + /* > + * Parse given string as parameter which has millisecond unit > + */ > + if (!parse_int(input_string, &result, GUC_UNIT_MS, &hintmsg)) > + ereport(ERROR, > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("invalid value for parameter \"%s\": \"%s\"", > + "min_apply_delay", input_string), > + hintmsg ? errhint("%s", _(hintmsg)) : 0)); > + > + /* > + * Check both the lower boundary for the valid min_apply_delay range and > + * the upper boundary as the safeguard for some platforms where INT_MAX is > + * wider than int32 respectively. Although parse_int() has confirmed that > + * the result is less than or equal to INT_MAX, the value will be stored > + * in a catalog column of int32. > + */ > + if (result < 0 || result > PG_INT32_MAX) > + ereport(ERROR, > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("%d ms is outside the valid range for parameter \"%s\" (%d .. %d)", > + result, > + "min_apply_delay", > + 0, PG_INT32_MAX))); > + > + return result; > +} > > 5a. > Since there are no translator considerations here why not write the > first error like: > > errmsg("invalid value for parameter \"min_apply_delay\": \"%s\"", > input_string) > > ~ > > 5b. > Since there are no translator considerations here why not write the > second error like: > > errmsg("%d ms is outside the valid range for parameter > \"min_apply_delay\" (%d .. %d)", > result, 0, PG_INT32_MAX)) Both of you said were fixed. Best Regards, Hayato Kuroda FUJITSU LIMITED > -----Original Message----- > From: Peter Smith <smithpb2...@gmail.com> > Sent: Tuesday, February 7, 2023 9:33 AM > To: Osumi, Takamichi/大墨 昂道 <osumi.takami...@fujitsu.com> > Cc: Amit Kapila <amit.kapil...@gmail.com>; Shi, Yu/侍 雨 > <shiy.f...@fujitsu.com>; Kyotaro Horiguchi <horikyota....@gmail.com>; > vignes...@gmail.com; Kuroda, Hayato/黒田 隼人 > <kuroda.hay...@fujitsu.com>; shveta.ma...@gmail.com; dilipbal...@gmail.com; > eu...@eulerto.com; m.melihmu...@gmail.com; and...@anarazel.de; > mar...@f10.com.br; pgsql-hack...@postgresql.org > Subject: Re: Time delayed LR (WAS Re: logical replication restrictions) > > Here are my review comments for v29-0001. > > ====== > Commit Message > > 1. > Discussion: > https://postgr.es/m/CAB-JLwYOYwL=XTyAXKiH5CtM_Vm8KjKh7aaitCKvmCh4r > zr...@mail.gmail.com > > tmp > > ~ > > What's that "tmp" doing there? A typo? > > ====== > doc/src/sgml/catalogs.sgml > > 2. > + <row> > + <entry role="catalog_table_entry"><para role="column_definition"> > + <structfield>subminapplydelay</structfield> <type>int4</type> > + </para> > + <para> > + The minimum delay (ms) for applying changes. > + </para></entry> > + </row> > > For consistency remove the period (.) because the other > single-sentence descriptions on this page do not have one. > > ====== > src/backend/commands/subscriptioncmds.c > > 3. AlterSubscription > + errmsg("cannot set parallel streaming mode for subscription with %s", > + "min_apply_delay")); > > Since there are no translator considerations here why not write it like this: > > errmsg("cannot set parallel streaming mode for subscription with > min_apply_delay") > > ~~~ > > 4. AlterSubscription > + errmsg("cannot set %s for subscription in parallel streaming mode", > + "min_apply_delay")); > > Since there are no translator considerations here why not write it like this: > > errmsg("cannot set min_apply_delay for subscription in parallel streaming > mode") > > ~~~ > > 5. > +defGetMinApplyDelay(DefElem *def) > +{ > + char *input_string; > + int result; > + const char *hintmsg; > + > + input_string = defGetString(def); > + > + /* > + * Parse given string as parameter which has millisecond unit > + */ > + if (!parse_int(input_string, &result, GUC_UNIT_MS, &hintmsg)) > + ereport(ERROR, > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("invalid value for parameter \"%s\": \"%s\"", > + "min_apply_delay", input_string), > + hintmsg ? errhint("%s", _(hintmsg)) : 0)); > + > + /* > + * Check both the lower boundary for the valid min_apply_delay range and > + * the upper boundary as the safeguard for some platforms where INT_MAX is > + * wider than int32 respectively. Although parse_int() has confirmed that > + * the result is less than or equal to INT_MAX, the value will be stored > + * in a catalog column of int32. > + */ > + if (result < 0 || result > PG_INT32_MAX) > + ereport(ERROR, > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("%d ms is outside the valid range for parameter \"%s\" (%d .. %d)", > + result, > + "min_apply_delay", > + 0, PG_INT32_MAX))); > + > + return result; > +} > > 5a. > Since there are no translator considerations here why not write the > first error like: > > errmsg("invalid value for parameter \"min_apply_delay\": \"%s\"", > input_string) > > ~ > > 5b. > Since there are no translator considerations here why not write the > second error like: > > errmsg("%d ms is outside the valid range for parameter > \"min_apply_delay\" (%d .. %d)", > result, 0, PG_INT32_MAX)) > > ------ > Kind Regards, > Peter Smith. > Fujitsu Australia >
v30-0001-Time-delayed-logical-replication-subscriber.patch
Description: v30-0001-Time-delayed-logical-replication-subscriber.patch
