Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c
Hi,
We have two static check_permissions functions (one in slotfuncs.c
another in logicalfuncs.c) with the same name and same code for
checking the privileges for using replication slots. Why can't we have
a single function CheckReplicationSlotPermissions in slot.c? This way,
we can get rid of redundant code. Attaching a patch for it.
Thoughts?
Regards,
Bharath Rupireddy.
Attachments:
v1-0001-replication-slots-remove-duplicate-code-for-check.patchapplication/octet-stream; name=v1-0001-replication-slots-remove-duplicate-code-for-check.patchDownload
From ac43dd7661fe68c5d9ed2f56ed22917afea7dc75 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 11 Sep 2021 08:05:04 +0000
Subject: [PATCH v1] replication slots: remove duplicate code for
check_permissions
Remove two static check_permissions functions with the same name
and same code for checking the privileges for using replication
slots. This patch adds a single function CheckReplicationSlotPermissions
in slot.c. This way, the patch get rid of redundant code.
---
.../replication/logical/logicalfuncs.c | 11 +----------
src/backend/replication/slot.c | 12 ++++++++++++
src/backend/replication/slotfuncs.c | 19 +++++--------------
src/include/replication/slot.h | 1 +
4 files changed, 19 insertions(+), 24 deletions(-)
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.c
index 1f38c5b33e..df454b1518 100644
--- a/src/backend/replication/logical/logicalfuncs.c
+++ b/src/backend/replication/logical/logicalfuncs.c
@@ -95,15 +95,6 @@ LogicalOutputWrite(LogicalDecodingContext *ctx, XLogRecPtr lsn, TransactionId xi
p->returned_rows++;
}
-static void
-check_permissions(void)
-{
- if (!superuser() && !has_rolreplication(GetUserId()))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or replication role to use replication slots")));
-}
-
/*
* Helper function for the various SQL callable logical decoding functions.
*/
@@ -124,7 +115,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
List *options = NIL;
DecodingOutputState *p;
- check_permissions();
+ CheckReplicationSlotPermissions();
CheckLogicalDecodingRequirements();
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 33e9acab4a..7905050e68 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1059,6 +1059,18 @@ CheckSlotRequirements(void)
errmsg("replication slots can only be used if wal_level >= replica")));
}
+/*
+ * Check whether the user has privilege to use replication slots.
+ */
+void
+CheckReplicationSlotPermissions(void)
+{
+ if (!superuser() && !has_rolreplication(GetUserId()))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser or replication role to use replication slots"))));
+}
+
/*
* Reserve WAL for the currently active slot.
*
diff --git a/src/backend/replication/slotfuncs.c b/src/backend/replication/slotfuncs.c
index 31e74d3832..ecbd959cdc 100644
--- a/src/backend/replication/slotfuncs.c
+++ b/src/backend/replication/slotfuncs.c
@@ -25,15 +25,6 @@
#include "utils/pg_lsn.h"
#include "utils/resowner.h"
-static void
-check_permissions(void)
-{
- if (!superuser() && !has_rolreplication(GetUserId()))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must be superuser or replication role to use replication slots")));
-}
-
/*
* Helper function for creating a new physical replication slot with
* given arguments. Note that this function doesn't release the created
@@ -85,7 +76,7 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- check_permissions();
+ CheckReplicationSlotPermissions();
CheckSlotRequirements();
@@ -188,7 +179,7 @@ pg_create_logical_replication_slot(PG_FUNCTION_ARGS)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- check_permissions();
+ CheckReplicationSlotPermissions();
CheckLogicalDecodingRequirements();
@@ -224,7 +215,7 @@ pg_drop_replication_slot(PG_FUNCTION_ARGS)
{
Name name = PG_GETARG_NAME(0);
- check_permissions();
+ CheckReplicationSlotPermissions();
CheckSlotRequirements();
@@ -619,7 +610,7 @@ pg_replication_slot_advance(PG_FUNCTION_ARGS)
Assert(!MyReplicationSlot);
- check_permissions();
+ CheckReplicationSlotPermissions();
if (XLogRecPtrIsInvalid(moveto))
ereport(ERROR,
@@ -718,7 +709,7 @@ copy_replication_slot(FunctionCallInfo fcinfo, bool logical_slot)
if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
elog(ERROR, "return type must be a row type");
- check_permissions();
+ CheckReplicationSlotPermissions();
if (logical_slot)
CheckLogicalDecodingRequirements();
diff --git a/src/include/replication/slot.h b/src/include/replication/slot.h
index e32fb85db8..f46215a7f9 100644
--- a/src/include/replication/slot.h
+++ b/src/include/replication/slot.h
@@ -222,5 +222,6 @@ extern void StartupReplicationSlots(void);
extern void CheckPointReplicationSlots(void);
extern void CheckSlotRequirements(void);
+extern void CheckReplicationSlotPermissions(void);
#endif /* SLOT_H */
--
2.25.1
On Sat, Sep 11, 2021, at 5:28 AM, Bharath Rupireddy wrote:
We have two static check_permissions functions (one in slotfuncs.c
another in logicalfuncs.c) with the same name and same code for
checking the privileges for using replication slots. Why can't we have
a single function CheckReplicationSlotPermissions in slot.c? This way,
we can get rid of redundant code. Attaching a patch for it.
Good catch! Your patch looks good to me.
--
Euler Taveira
EDB https://www.enterprisedb.com/
On 9/11/21, 1:31 AM, "Bharath Rupireddy" <bharath.rupireddyforpostgres@gmail.com> wrote:
We have two static check_permissions functions (one in slotfuncs.c
another in logicalfuncs.c) with the same name and same code for
checking the privileges for using replication slots. Why can't we have
a single function CheckReplicationSlotPermissions in slot.c? This way,
we can get rid of redundant code. Attaching a patch for it.
+1
+/*
+ * Check whether the user has privilege to use replication slots.
+ */
+void
+CheckReplicationSlotPermissions(void)
+{
+ if (!superuser() && !has_rolreplication(GetUserId()))
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser or replication role to use replication slots"))));
+}nitpick: It looks like there's an extra set of parentheses around
errmsg().
Nathan
On Sun, Sep 12, 2021, at 8:02 PM, Bossart, Nathan wrote:
nitpick: It looks like there's an extra set of parentheses around
errmsg().
Indeed. Even the requirement for extra parenthesis around auxiliary function
calls was removed in v12 (e3a87b4991cc2d00b7a3082abb54c5f12baedfd1).
--
Euler Taveira
EDB https://www.enterprisedb.com/
On Sun, Sep 12, 2021 at 10:14:36PM -0300, Euler Taveira wrote:
On Sun, Sep 12, 2021, at 8:02 PM, Bossart, Nathan wrote:
nitpick: It looks like there's an extra set of parentheses around
errmsg().Indeed. Even the requirement for extra parenthesis around auxiliary function
calls was removed in v12 (e3a87b4991cc2d00b7a3082abb54c5f12baedfd1).
Yes. The patch makes sense. I am not seeing any other places that
could be grouped, so that looks fine as-is.
--
Michael
On Mon, Sep 13, 2021 at 6:45 AM Euler Taveira <euler@eulerto.com> wrote:
On Sun, Sep 12, 2021, at 8:02 PM, Bossart, Nathan wrote:
nitpick: It looks like there's an extra set of parentheses around
errmsg().Indeed. Even the requirement for extra parenthesis around auxiliary function
calls was removed in v12 (e3a87b4991cc2d00b7a3082abb54c5f12baedfd1).
The same commit says that the new code can be written in any way.
Having said that, I will leave it to the committer to take a call on
whether or not to remove the extra parenthesis.
"
While new code can be written either way, code intended to be
back-patched will need to use extra parens for awhile yet.
"
Regards,
Bharath Rupireddy.
On Mon, Sep 13, 2021 at 8:07 AM Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Sep 12, 2021 at 10:14:36PM -0300, Euler Taveira wrote:
On Sun, Sep 12, 2021, at 8:02 PM, Bossart, Nathan wrote:
nitpick: It looks like there's an extra set of parentheses around
errmsg().Indeed. Even the requirement for extra parenthesis around auxiliary function
calls was removed in v12 (e3a87b4991cc2d00b7a3082abb54c5f12baedfd1).Yes. The patch makes sense. I am not seeing any other places that
could be grouped, so that looks fine as-is.
Thanks all for taking a look at the patch. Here's the CF entry -
https://commitfest.postgresql.org/35/3319/
Regards,
Bharath Rupireddy.
On Mon, Sep 13, 2021 at 08:51:18AM +0530, Bharath Rupireddy wrote:
On Mon, Sep 13, 2021 at 8:07 AM Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Sep 12, 2021 at 10:14:36PM -0300, Euler Taveira wrote:
On Sun, Sep 12, 2021, at 8:02 PM, Bossart, Nathan wrote:
nitpick: It looks like there's an extra set of parentheses around
errmsg().Indeed. Even the requirement for extra parenthesis around auxiliary function
calls was removed in v12 (e3a87b4991cc2d00b7a3082abb54c5f12baedfd1).
Applied. Not using those extra parenthesis is the most common
pattern, so tweaked this way.
--
Michael
On 2021-Sep-14, Michael Paquier wrote:
On Mon, Sep 13, 2021 at 08:51:18AM +0530, Bharath Rupireddy wrote:
On Mon, Sep 13, 2021 at 8:07 AM Michael Paquier <michael@paquier.xyz> wrote:
On Sun, Sep 12, 2021 at 10:14:36PM -0300, Euler Taveira wrote:
On Sun, Sep 12, 2021, at 8:02 PM, Bossart, Nathan wrote:
nitpick: It looks like there's an extra set of parentheses around
errmsg().Indeed. Even the requirement for extra parenthesis around auxiliary function
calls was removed in v12 (e3a87b4991cc2d00b7a3082abb54c5f12baedfd1).Applied. Not using those extra parenthesis is the most common
pattern, so tweaked this way.
The parentheses that commit e3a87b4991cc removed the requirement for are
those that the committed code still has, starting at the errcode() line.
The ones in errmsg() were redundant and have never been necessary.
--
Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
"El sabio habla porque tiene algo que decir;
el tonto, porque tiene que decir algo" (Platon).
On Tue, Sep 14, 2021 at 12:57:47PM -0300, Alvaro Herrera wrote:
The parentheses that commit e3a87b4991cc removed the requirement for are
those that the committed code still has, starting at the errcode() line.
The ones in errmsg() were redundant and have never been necessary.
Indeed, thanks!
--
Michael