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

From Takamichi Osumi (Fujitsu)
Subject RE: Time delayed LR (WAS Re: logical replication restrictions)
Date
Msg-id TYCPR01MB8373923C567963CF849A59DEEDFF9@TYCPR01MB8373.jpnprd01.prod.outlook.com
Whole thread Raw
In response to Re: Time delayed LR (WAS Re: logical replication restrictions)  (shveta malik <shveta.malik@gmail.com>)
List pgsql-hackers
On Tuesday, January 3, 2023 8:22 PM shveta malik <shveta.malik@gmail.com> wrote:
> Please find a few minor comments.
Thanks for your review !

> 1.
> +      diffms = TimestampDifferenceMilliseconds(GetCurrentTimestamp(),
> +
> 
>    TimestampTzPlusMilliseconds(ts, MySubscription->minapplydelay));  on
> unix, above code looks unaligned (copied from unix)
> 
> 2. same with:
> +      interval = DatumGetIntervalP(DirectFunctionCall3(interval_in,
> +
> 
>    CStringGetDatum(val),
> +
> 
>    ObjectIdGetDatum(InvalidOid),
> +
> 
>    Int32GetDatum(-1)));
> perhaps due to tabs?
Those patches indentation look OK. I checked them
by pgindent and less command described in [1]. So, I didn't change those.


> 2. comment not clear:
> +      * During the time delayed replication, avoid reporting the suspended
> +      * latest LSN are already flushed and written, to the publisher.
You are right. I fixed this part to make it clearer.
Could you please check ?

> 3.
> +      * Call send_feedback() to prevent the publisher from exiting by
> +      * timeout during the delay, when wal_receiver_status_interval is
> +      * available. The WALs for this delayed transaction is neither
> +      * written nor flushed yet, Thus, we don't make the latest LSN
> +      * overwrite those positions of the update message for this delay.
> 
>  yet, Thus, we -->  yet, thus, we/   yet. Thus, we
Yeah, you are right. But, I have removed the last sentence, because the last one
explains some internals of send_feedback(). I judged that it would be awkward
to describe it in maybe_delay_apply(). Now this part has become concise.

> 4.
> +      /* Adds portion time (in ms) to the previous result. */
> +      ms = interval->time / INT64CONST(1000);
> Is interval->time always in micro-seconds here?
Yeah, it seems so. Some internal codes indicate it. Kindly have a look at functions
such as make_interval() and interval2itm().

Please have a look at the latest patch v12 in [2].

[1] - https://www.postgresql.org/docs/current/source-format.html
[2] -
https://www.postgresql.org/message-id/TYCPR01MB837340F78F4A16F542589195EDFF9%40TYCPR01MB8373.jpnprd01.prod.outlook.com


Best Regards,
    Takamichi Osumi


pgsql-hackers by date:

Previous
From: Dean Rasheed
Date:
Subject: Re: Bug in check for unreachable MERGE WHEN clauses
Next
From: Tom Lane
Date:
Subject: Re: [PATCH] random_normal function