Simplify if/else logic of walsender CreateReplicationSlot
Hi,
While reviewing another patch I was looking at the walsender's static
function CreateReplicationSlot
I found that the current logic seemed to have some unnecessary if/else
checking which can be simplified.
~~
To summarise:
CURRENT
if (cmd->kind == REPLICATION_KIND_PHYSICAL)
{
...
}
else
{
...
}
if (cmd->kind == REPLICATION_KIND_LOGICAL)
{
...
}
else if (cmd->kind == REPLICATION_KIND_PHYSICAL && reserve_wal)
{
...
}
SUGGESTION
if (cmd->kind == REPLICATION_KIND_PHYSICAL)
{
...
if (reserve_wal)
{
...
}
}
else /* REPLICATION_KIND_LOGICAL */
{
...
}
~~~
PSA a small patch for making this change.
(I ran make check-world after this change and it was successful)
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-Simplify-if-else-logic-of-walsender-CreateReplica.patchapplication/octet-stream; name=v1-0001-Simplify-if-else-logic-of-walsender-CreateReplica.patchDownload
From 4c786078b29d2f7ccb563761acb5fe423585c7de Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Mon, 20 Nov 2023 17:52:48 +1100
Subject: [PATCH v1] Simplify if/else logic of walsender CreateReplicationSlot
---
src/backend/replication/walsender.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index e250b05..3bc9c82 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1061,9 +1061,25 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
ReplicationSlotCreate(cmd->slotname, false,
cmd->temporary ? RS_TEMPORARY : RS_PERSISTENT,
false);
+
+ if (reserve_wal)
+ {
+ ReplicationSlotReserveWal();
+
+ ReplicationSlotMarkDirty();
+
+ /* Write this slot to disk if it's a permanent one. */
+ if (!cmd->temporary)
+ ReplicationSlotSave();
+ }
}
else
{
+ LogicalDecodingContext *ctx;
+ bool need_full_snapshot = false;
+
+ Assert(cmd->kind == REPLICATION_KIND_LOGICAL);
+
CheckLogicalDecodingRequirements();
/*
@@ -1076,12 +1092,6 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
ReplicationSlotCreate(cmd->slotname, true,
cmd->temporary ? RS_TEMPORARY : RS_EPHEMERAL,
two_phase);
- }
-
- if (cmd->kind == REPLICATION_KIND_LOGICAL)
- {
- LogicalDecodingContext *ctx;
- bool need_full_snapshot = false;
/*
* Do options check early so that we can bail before calling the
@@ -1175,16 +1185,6 @@ CreateReplicationSlot(CreateReplicationSlotCmd *cmd)
if (!cmd->temporary)
ReplicationSlotPersist();
}
- else if (cmd->kind == REPLICATION_KIND_PHYSICAL && reserve_wal)
- {
- ReplicationSlotReserveWal();
-
- ReplicationSlotMarkDirty();
-
- /* Write this slot to disk if it's a permanent one. */
- if (!cmd->temporary)
- ReplicationSlotSave();
- }
snprintf(xloc, sizeof(xloc), "%X/%X",
LSN_FORMAT_ARGS(MyReplicationSlot->data.confirmed_flush));
--
1.8.3.1
On Mon, Nov 20, 2023 at 06:01:42PM +1100, Peter Smith wrote:
While reviewing another patch I was looking at the walsender's static
function CreateReplicationSlotI found that the current logic seemed to have some unnecessary if/else
checking which can be simplified.
Good idea. What you are suggesting here improves the readability of
this code, so +1.
--
Michael
On Mon, Nov 20, 2023 at 05:07:38PM +0900, Michael Paquier wrote:
Good idea. What you are suggesting here improves the readability of
this code, so +1.
And applied this one, thanks!
--
Michael
On Tue, Nov 21, 2023 at 3:57 PM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Nov 20, 2023 at 05:07:38PM +0900, Michael Paquier wrote:
Good idea. What you are suggesting here improves the readability of
this code, so +1.And applied this one, thanks!
Thanks for pushing.
======
Kind Regards,
Peter Smith.
Fujitsu Australia