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(at)mail(dot)gmail(dot)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(at)gmail(dot)com>
> Sent: Tuesday, February 7, 2023 9:33 AM
> To: Osumi, Takamichi/大墨 昂道 <osumi(dot)takamichi(at)fujitsu(dot)com>
> Cc: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>; Shi, Yu/侍 雨
> <shiy(dot)fnst(at)fujitsu(dot)com>; Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com>;
> vignesh21(at)gmail(dot)com; Kuroda, Hayato/黒田 隼人
> <kuroda(dot)hayato(at)fujitsu(dot)com>; shveta(dot)malik(at)gmail(dot)com; dilipbalaut(at)gmail(dot)com;
> euler(at)eulerto(dot)com; m(dot)melihmutlu(at)gmail(dot)com; andres(at)anarazel(dot)de;
> marcos(at)f10(dot)com(dot)br; pgsql-hackers(at)postgresql(dot)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(at)mail(dot)gmail(dot)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
>