Horton

Thread

Commits

  1. Remove code duplication for permission checks with replication slots

  2. Re-implement the ereport() macro using __VA_ARGS__.

  1. Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-09-11T08:28:42Z

    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.

  2. Re: Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c

    Euler Taveira <euler@eulerto.com> — 2021-09-12T16:46:35Z

    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/

  3. Re: Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c

    Nathan Bossart <bossartn@amazon.com> — 2021-09-12T23:02:49Z

    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

  4. Re: Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c

    Euler Taveira <euler@eulerto.com> — 2021-09-13T01:14:36Z

    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/

  5. Re: Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c

    Michael Paquier <michael@paquier.xyz> — 2021-09-13T02:37:38Z

    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

  6. Re: Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-09-13T03:17:16Z

    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.

  7. Re: Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-09-13T03:21:18Z

    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.

  8. Re: Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c

    Michael Paquier <michael@paquier.xyz> — 2021-09-14T01:23:48Z

    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

  9. Re: Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2021-09-14T15:57:47Z

    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).

  10. Re: Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c

    Michael Paquier <michael@paquier.xyz> — 2021-09-14T22:18:48Z

    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