On Friday, November 29, 2024 6:35 PM Kuroda, Hayato/黒田 隼人
<kuroda.hay...@fujitsu.com> wrote:
>
> Dear Hou,
>
> Thanks for updating the patch! Here are my comments mainly for 0001.
Thanks for the comments!
>
> 02. maybe_advance_nonremovable_xid
>
> ```
> + case RCI_REQUEST_PUBLISHER_STATUS:
> + request_publisher_status(data);
> + break;
> ```
>
> I think the part is not reachable because the transit
> RCI_REQUEST_PUBLISHER_STATUS->RCI_WAIT_FOR_PUBLISHER_STATU
> S is done in get_candidate_xid()->request_publisher_status().
> Can we remove this?
I changed to call the maybe_advance_nonremovable_xid() after changing the phase
in get_candidate_xid/wait_for_publisher_status, so that the code is reachable.
>
>
> 05. request_publisher_status
>
> ```
> + if (!reply_message)
> + {
> + MemoryContext oldctx = MemoryContextSwitchTo(ApplyContext);
> +
> + reply_message = makeStringInfo();
> + MemoryContextSwitchTo(oldctx);
> + }
> + else
> + resetStringInfo(reply_message);
> ```
>
> Same lines exist in two functions: can we provide an inline function?
I personally feel these codes may not worth a separate function since it’s
simple.
So didn't change in this version.
>
> 06. wait_for_publisher_status
>
> ```
> + if (!FullTransactionIdIsValid(data->last_phase_at))
> + data->last_phase_at =
> FullTransactionIdFromEpochAndXid(data->remote_epoch,
> +
> + data->remote_nextxid);
> +
> ```
>
> Not sure, is there a possibility that data->last_phase_at is valid here? It is
> initialized just before transiting to RCI_WAIT_FOR_PUBLISHER_STATUS.
Oh. I think last_phase_at should be initialized only in the first phase. Fixed.
Other comments look good to me and have been addressed in V13.
Best Regards,
Hou zj
