Stop the search once replication origin is found

On Mon, Nov 20, 2023 at 2:36 PM Antonin Houska <ah@cybertec.at> wrote:

Although it's not performance-critical, I think it just makes sense to break
the loop in replorigin_session_setup() as soon as we've found the origin.

Your proposal sounds reasonable to me.

--
With Regards,
Amit Kapila.

On Mon, Nov 20, 2023 at 4:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 20, 2023 at 2:36 PM Antonin Houska <ah@cybertec.at> wrote:

Although it's not performance-critical, I think it just makes sense to break
the loop in replorigin_session_setup() as soon as we've found the origin.

Your proposal sounds reasonable to me.

Pushed, thanks for the patch!

--
With Regards,
Amit Kapila.

On Wed, Nov 22, 2023 at 7:49 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 20, 2023 at 4:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

On Mon, Nov 20, 2023 at 2:36 PM Antonin Houska <ah@cybertec.at> wrote:

Although it's not performance-critical, I think it just makes sense to break
the loop in replorigin_session_setup() as soon as we've found the origin.

Your proposal sounds reasonable to me.

Pushed, thanks for the patch!

--

Hi,

While reviewing the replorigin_session_setup() fix [1]/messages/by-id/2694.1700471273@antos that was pushed
yesterday, I saw that some nearby code in that same function might
benefit from some refactoring.

I'm not sure if you want to modify it or not, but FWIW I think the
code can be tidied by making the following changes:

~~~

1.
else if (curstate->acquired_by != 0 && acquired_by == 0)
{
ereport(ERROR,
(errcode(ERRCODE_OBJECT_IN_USE),
errmsg("replication origin with ID %d is already
active for PID %d",
curstate->roident, curstate->acquired_by)));
}

1a. AFAICT the above code doesn't need to be else/if

1b. The brackets are unnecessary for a single statement.

~~~

2.
if (session_replication_state == NULL && free_slot == -1)
ereport(ERROR,
(errcode(ERRCODE_CONFIGURATION_LIMIT_EXCEEDED),
errmsg("could not find free replication state slot for replication
origin with ID %d",
node),
errhint("Increase max_replication_slots and try again.")));
else if (session_replication_state == NULL)
{
/* initialize new slot */
session_replication_state = &replication_states[free_slot];
Assert(session_replication_state->remote_lsn == InvalidXLogRecPtr);
Assert(session_replication_state->local_lsn == InvalidXLogRecPtr);
session_replication_state->roident = node;
}

The above code can be improved by combining within a single check for
session_replication_state NULL.

~~~

3.
There are some unnecessary double-blank lines.

~~~

4.
/* ok, found slot */
session_replication_state = curstate;
break;

QUESTION: That 'session_replication_state' is a global variable, but
there is more validation logic that comes *after* this assignment
which might decide there was some problem and cause an ereport or
elog. In practice, maybe it makes no difference, but it did seem
slightly dubious to me to assign to a global before determining
everything is OK. Thoughts?

~~~

Anyway, PSA a patch for the 1-3 above.

======
[1]: /messages/by-id/2694.1700471273@antos

Kind Regards,
Peter Smith.
Fujitsu Australia