RE: Time delayed LR (WAS Re: logical replication restrictions) - Mailing list pgsql-hackers

From Hayato Kuroda (Fujitsu)
Subject RE: Time delayed LR (WAS Re: logical replication restrictions)
Date
Msg-id TYAPR01MB5866982AD09BD79D2536675EF5AA9@TYAPR01MB5866.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Re: Time delayed LR (WAS Re: logical replication restrictions)  (Amit Kapila <amit.kapila16@gmail.com>)
Responses Re: Time delayed LR (WAS Re: logical replication restrictions)
RE: Time delayed LR (WAS Re: logical replication restrictions)
List pgsql-hackers
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


Attachment

pgsql-hackers by date:

Previous
From: Dean Rasheed
Date:
Subject: Re: Missing cases from SPI_result_code_string()
Next
From: Hugo Zhang
Date:
Subject: Unexpected abort at llvm::report_bad_alloc_error when load JIT library