RE: [Patch] add new parameter to pg_replication_origin_session_setup

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