Stop the search once replication origin is found
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t48758psql -h localhost -U postgresBuilt from patchset v4 (message #4), August 18, 2026 at 03:13 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t48758_4 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t48758_4 && git checkout t48758_4Patchset v4 (message #4) is on t48758_4
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.
--
Antonin Houska
Web: https://www.cybertec-postgresql.com
Attachments:
replorigin_session_setup_break.difftext/x-diffDownload+1-0
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