RE: Time delayed LR (WAS Re: logical replication restrictions)

Dear Peter,

Thank you for reviewing! PSA new version.

> ======
> Commit Message
> 
> 1.
> Discussion:
> https://postgr.es/m/CAB-JLwYOYwL=XTyAXKiH5CtM_Vm8KjKh7aaitCKvmCh4r
> zr5pQ@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 <smithpb2250@gmail.com>
> Sent: Tuesday, February 7, 2023 9:33 AM
> To: Osumi, Takamichi/大墨 昂道 <osumi.takamichi@fujitsu.com>
> Cc: Amit Kapila <amit.kapila16@gmail.com>; Shi, Yu/侍 雨
> <shiy.fnst@fujitsu.com>; Kyotaro Horiguchi <horikyota.ntt@gmail.com>;
> vignesh21@gmail.com; Kuroda, Hayato/黒田 隼人
> <kuroda.hayato@fujitsu.com>; shveta.malik@gmail.com; dilipbalaut@gmail.com;
> euler@eulerto.com; m.melihmutlu@gmail.com; andres@anarazel.de;
> marcos@f10.com.br; pgsql-hackers@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
> zr5pQ@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
>