Dear Amit,
Thank you for reviewing! PSA new version.
> 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;
OK, both of them were fixed.
> > > 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?
For extracting value from the string, strtoul() is used.
This is an important point.
```
delay_val = strtoul(strVal(defel->arg), &endptr, 10);
```
If user specifies min_send_delay as '-1', the value is read as a bit string
'0xFFFFFFFFFFFFFFFF', and it is interpreted as PG_UINT64_MAX. After that such a
strange value is rejected by the part you copied. I have tested the case and it has
correctly rejected.
```
postgres=# START_REPLICATION SLOT "sub" LOGICAL 0/0 (min_send_delay '-1');
ERROR: min_send_delay "-1" out of range
CONTEXT: slot "sub", output plugin "pgoutput", in the startup callback
```
> + has been finished. However, there is a possibility that the table
> + status written in <link
> linkend="catalog-pg-subscription-rel"><structname>pg_subscription_rel</stru
> ctname></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</stru
> ctname></link>
> could be delayed in getting to the "ready" state, and also two-phase
> (if specified) could be delayed in getting to "enabled"."?
Oh, my Visual Studio Code did not detect the strange character.
And reworded accordingly.
Additionally, I modified the commit message to describe more clearly the reason
why the do not allow combination of min_send_delay and streaming = parallel.
Best Regards,
Hayato Kuroda
FUJITSU LIMITED
