Обсуждение: 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.

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.

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

nitpick: It looks like there's an extra set of parentheses around

errmsg().

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