Stop the search once replication origin is found
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
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index b0255ffd25..460e3dcc38 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -1144,6 +1144,7 @@ replorigin_session_setup(RepOriginId node, int acquired_by)
/* ok, found slot */
session_replication_state = curstate;
+ break;
}
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
Attachments:
v1-0001-replorigin_session_setup-refactoring.patchapplication/octet-stream; name=v1-0001-replorigin_session_setup-refactoring.patchDownload
From 2fa9bff99df0dc22054038aae9033a6b57055d9d Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Thu, 23 Nov 2023 09:20:34 +1100
Subject: [PATCH v1] replorigin_session_setup refactoring
---
src/backend/replication/logical/origin.c | 21 +++++++++------------
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/src/backend/replication/logical/origin.c b/src/backend/replication/logical/origin.c
index 460e3dc..9ca3fd7 100644
--- a/src/backend/replication/logical/origin.c
+++ b/src/backend/replication/logical/origin.c
@@ -1134,28 +1134,26 @@ replorigin_session_setup(RepOriginId node, int acquired_by)
if (curstate->roident != node)
continue;
- else if (curstate->acquired_by != 0 && acquired_by == 0)
- {
+ 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)));
- }
/* ok, found slot */
session_replication_state = curstate;
break;
}
-
- 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)
+ if (session_replication_state == NULL)
{
+ if (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.")));
+
/* initialize new slot */
session_replication_state = &replication_states[free_slot];
Assert(session_replication_state->remote_lsn == InvalidXLogRecPtr);
@@ -1163,7 +1161,6 @@ replorigin_session_setup(RepOriginId node, int acquired_by)
session_replication_state->roident = node;
}
-
Assert(session_replication_state->roident != InvalidRepOriginId);
if (acquired_by == 0)
--
1.8.3.1