On Wed, Nov 22, 2023 at 7:49 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Mon, Nov 20, 2023 at 4:36 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Mon, Nov 20, 2023 at 2:36 PM Antonin Houska <a...@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] 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] https://www.postgresql.org/message-id/flat/2694.1700471273%40antos Kind Regards, Peter Smith. Fujitsu Australia
v1-0001-replorigin_session_setup-refactoring.patch
Description: Binary data