Dear Amit,
>
> Few comments:
> 1. +step "s0_compare" {
> + SELECT s0.lsn < s1.lsn
> + FROM local_lsn_store as s0, local_lsn_store as s1
> + WHERE s0.session = 0 AND s1.session = 1;
> +}
>
> This appears to be a bit tricky to compare the values. Doing a
> sequential scan won't guarantee the order of rows' appearance. Can't
> we somehow get the two rows ordered by session_id and compare their
> values?
I considered another way to use the CTE for session 0. How do you feel?
> 2.
> + else if (candidate_state->acquired_by != acquired_by)
> + {
> + if (initialized)
> + candidate_state->roident = InvalidRepOriginId;
> +
> elog(ERROR, "could not find replication state slot for replication
> origin with OID %u which was acquired by %d",
> node, acquired_by);
> + }
>
> This doesn't appear neat. Instead, how about checking this case before
> setting current_state as shown in attached. If we do that, we
> shouldn't even need new variables like current_state and initialized.
Your approach cannot work when the specified origin is not used yet after the
instance starts. In this case the origin has not exist in the replication_states
yet and new slot is initialized.
Per current understanding, two ERRORs are needed to avoid adding new variables;
first one is in the loop, and second one is in session_replication_state == NULL
case. Latter one indicates the case that origin is inactive but PID is specified
so different error message can be set.
> Additionally, as shown in attached, it is better to make this a
> user-facing error by using ereport.
Indeed, elog() were replaced with ereport().
> 3. Merge all patches as I don't see the need to do any backpatch here.
Sure.
Attached patch includes all changes. Thought?
Best regards,
Hayato Kuroda
FUJITSU LIMITED
