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+19-25
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