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

On Tue, Feb 21, 2023 at 1:28 PM Hayato Kuroda (Fujitsu)
<kuroda.hayato@fujitsu.com> wrote:
>
> > doc/src/sgml/catalogs.sgml
> >
> > 4.
> > +     <row>
> > +      <entry role="catalog_table_entry"><para role="column_definition">
> > +       <structfield>subminsenddelay</structfield> <type>int4</type>
> > +      </para>
> > +      <para>
> > +       The minimum delay, in milliseconds, by the publisher to send changes
> > +      </para></entry>
> > +     </row>
> >
> > "by the publisher to send changes" --> "by the publisher before sending changes"
>
> As Amit said[1], there is a possibility to delay after sending delay. So I changed to
> "before sending COMMIT record". How do you think?
>

I think it would be better to say: "The minimum delay, in
milliseconds, by the publisher before sending all the changes". If you
agree then similar change is required in below comment as well:
+ /*
+ * The minimum delay, in milliseconds, by the publisher before sending
+ * COMMIT/PREPARE record.
+ */
+ int32 min_send_delay;
+

>
> > src/backend/replication/pgoutput/pgoutput.c
> >
> > 11.
> > + errno = 0;
> > + parsed = strtoul(strVal(defel->arg), &endptr, 10);
> > + if (errno != 0 || *endptr != '\0')
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("invalid min_send_delay")));
> > +
> > + if (parsed > PG_INT32_MAX)
> > + ereport(ERROR,
> > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> > + errmsg("min_send_delay \"%s\" out of range",
> > + strVal(defel->arg))));
> >
> > Should the validation be also checking/asserting no negative numbers,
> > or actually should the min_send_delay be defined as a uint32 in the
> > first place?
>
> I think you are right because min_apply_delay does not have related code.
> we must consider additional possibility that user may send START_REPLICATION
> by hand and it has minus value.
> Fixed.
>

Your reasoning for adding the additional check seems good to me but I
don't see it in the patch. The check as I see is as below:
+ if (delay_val > PG_INT32_MAX)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("min_send_delay \"%s\" out of range",
+ strVal(defel->arg))));

Am, I missing something, and the new check is at some other place?

+          has been finished. However, there is a possibility that the table
+          status written in <link
linkend="catalog-pg-subscription-rel"><structname>pg_subscription_rel</structname></link>
+          will be delayed in getting to "ready" state, and also two-phase
+          (if specified) will be delayed in getting to "enabled".
+         </para>

There appears to be a special value <0x00> after "ready". I think that
is added by mistake or probably you have used some editor which has
added this value. Can we slightly reword this to: "However, there is a
possibility that the table status updated in <link
linkend="catalog-pg-subscription-rel"><structname>pg_subscription_rel</structname></link>
could be delayed in getting to the "ready" state, and also two-phase
(if specified) could be delayed in getting to "enabled"."?

-- 
With Regards,
Amit Kapila.