Thread: GetSubscriptionRelations declares too many scan keys

GetSubscriptionRelations declares too many scan keys

From
Peter Smith
Date:
The function GetSubscriptionRelations was declaring ScanKeyData
skey[2]; but actually
only uses 1 scan key. It seems like the code was cut/paste from other
nearby functions
which really are using 2 keys.

PSA a trivial patch to declare the correct number of keys for this function.

------
Kind Regards,
Peter Smith.
Fujitsu Australia

Attachment

Re: GetSubscriptionRelations declares too many scan keys

From
Bharath Rupireddy
Date:
On Mon, May 10, 2021 at 12:36 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> The function GetSubscriptionRelations was declaring ScanKeyData
> skey[2]; but actually
> only uses 1 scan key. It seems like the code was cut/paste from other
> nearby functions
> which really are using 2 keys.
>
> PSA a trivial patch to declare the correct number of keys for this function.

+1 for the change. It looks like a cut/paste type introduced by the
commit 7c4f52409a.

A comment on the patch: why do we need to declare an array of 1
element ScanKeyData skey[1];? Instead, can we just do ScanKeyData
skey;?

With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com



Re: GetSubscriptionRelations declares too many scan keys

From
Julien Rouhaud
Date:
On Mon, May 10, 2021 at 01:39:31PM +0530, Bharath Rupireddy wrote:
> On Mon, May 10, 2021 at 12:36 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > The function GetSubscriptionRelations was declaring ScanKeyData
> > skey[2]; but actually
> > only uses 1 scan key. It seems like the code was cut/paste from other
> > nearby functions
> > which really are using 2 keys.
> >
> > PSA a trivial patch to declare the correct number of keys for this function.
> 
> +1 for the change. It looks like a cut/paste type introduced by the
> commit 7c4f52409a.
> 
> A comment on the patch: why do we need to declare an array of 1
> element ScanKeyData skey[1];? Instead, can we just do ScanKeyData
> skey;?

+1, there are already many places where it's done this way if there's only 1
key.



Re: GetSubscriptionRelations declares too many scan keys

From
Peter Smith
Date:
On Mon, May 10, 2021 at 6:09 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
>
> On Mon, May 10, 2021 at 12:36 PM Peter Smith <smithpb2250@gmail.com> wrote:
> >
> > The function GetSubscriptionRelations was declaring ScanKeyData
> > skey[2]; but actually
> > only uses 1 scan key. It seems like the code was cut/paste from other
> > nearby functions
> > which really are using 2 keys.
> >
> > PSA a trivial patch to declare the correct number of keys for this function.
>
> +1 for the change. It looks like a cut/paste type introduced by the
> commit 7c4f52409a.
>
> A comment on the patch: why do we need to declare an array of 1
> element ScanKeyData skey[1];? Instead, can we just do ScanKeyData
> skey;?

IMO declaring skey[1] is better because then the code can share the
same pattern as every other ScanData skey[n] code.

Please search PG source code for "ScanData skey[1];" - there are
dozens of precedents where other people felt the same as me for
declaring single keys.

--------
Kind Regards,
Peter Smith.
Fujitsu Australia



Re: GetSubscriptionRelations declares too many scan keys

From
Julien Rouhaud
Date:
On Mon, May 10, 2021 at 07:09:29PM +1000, Peter Smith wrote:
> On Mon, May 10, 2021 at 6:09 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > On Mon, May 10, 2021 at 12:36 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > >
> > > The function GetSubscriptionRelations was declaring ScanKeyData
> > > skey[2]; but actually
> > > only uses 1 scan key. It seems like the code was cut/paste from other
> > > nearby functions
> > > which really are using 2 keys.
> > >
> > > PSA a trivial patch to declare the correct number of keys for this function.
> >
> > +1 for the change. It looks like a cut/paste type introduced by the
> > commit 7c4f52409a.
> >
> > A comment on the patch: why do we need to declare an array of 1
> > element ScanKeyData skey[1];? Instead, can we just do ScanKeyData
> > skey;?
> 
> IMO declaring skey[1] is better because then the code can share the
> same pattern as every other ScanData skey[n] code.
> 
> Please search PG source code for "ScanData skey[1];" - there are
> dozens of precedents where other people felt the same as me for
> declaring single keys.

AFAICT there are 73 occurences vs 62 of the "Scandata skey;".  I don't think
there's a huge consensus for one over the other.



Re: GetSubscriptionRelations declares too many scan keys

From
Dilip Kumar
Date:
On Mon, May 10, 2021 at 2:46 PM Julien Rouhaud <rjuju123@gmail.com> wrote:
>
> On Mon, May 10, 2021 at 07:09:29PM +1000, Peter Smith wrote:
> > On Mon, May 10, 2021 at 6:09 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@gmail.com> wrote:
> > >
> > > On Mon, May 10, 2021 at 12:36 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > > >
> > > > The function GetSubscriptionRelations was declaring ScanKeyData
> > > > skey[2]; but actually
> > > > only uses 1 scan key. It seems like the code was cut/paste from other
> > > > nearby functions
> > > > which really are using 2 keys.
> > > >
> > > > PSA a trivial patch to declare the correct number of keys for this function.
> > >
> > > +1 for the change. It looks like a cut/paste type introduced by the
> > > commit 7c4f52409a.
> > >
> > > A comment on the patch: why do we need to declare an array of 1
> > > element ScanKeyData skey[1];? Instead, can we just do ScanKeyData
> > > skey;?
> >
> > IMO declaring skey[1] is better because then the code can share the
> > same pattern as every other ScanData skey[n] code.
> >
> > Please search PG source code for "ScanData skey[1];" - there are
> > dozens of precedents where other people felt the same as me for
> > declaring single keys.
>
> AFAICT there are 73 occurences vs 62 of the "Scandata skey;".  I don't think
> there's a huge consensus for one over the other.

I think both Scandata skey[1]; and Scandata skey; are used. But IMHO
using Scandata skey; looks better.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com



Re: GetSubscriptionRelations declares too many scan keys

From
Tom Lane
Date:
Julien Rouhaud <rjuju123@gmail.com> writes:
> On Mon, May 10, 2021 at 07:09:29PM +1000, Peter Smith wrote:
>> Please search PG source code for "ScanData skey[1];" - there are
>> dozens of precedents where other people felt the same as me for
>> declaring single keys.

> AFAICT there are 73 occurences vs 62 of the "Scandata skey;".  I don't think
> there's a huge consensus for one over the other.

Yeah, there's no real consensus about that.  But in this case there's
a strong reason to use skey[1]: it makes the patch a very safe one-liner.
To convert to the other pattern would require touching more code.

            regards, tom lane



Re: GetSubscriptionRelations declares too many scan keys

From
Michael Paquier
Date:
On Mon, May 10, 2021 at 10:14:08AM -0400, Tom Lane wrote:
> Yeah, there's no real consensus about that.  But in this case there's
> a strong reason to use skey[1]: it makes the patch a very safe one-liner.
> To convert to the other pattern would require touching more code.

FWIW, what Peter S has done looks fine to me, even if it is true that
CountDBSubscriptions() uses one scan key but does not use an array.

And that makes the code slightly easier to follow.
--
Michael

Attachment

Re: GetSubscriptionRelations declares too many scan keys

From
Michael Paquier
Date:
On Tue, May 11, 2021 at 04:50:46PM +0900, Michael Paquier wrote:
> And that makes the code slightly easier to follow.

Yeah, that's better this way, so applied.
--
Michael

Attachment

Re: GetSubscriptionRelations declares too many scan keys

From
Peter Smith
Date:
On Wed, May 12, 2021 at 5:52 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, May 11, 2021 at 04:50:46PM +0900, Michael Paquier wrote:
> > And that makes the code slightly easier to follow.
>
> Yeah, that's better this way, so applied.

Thanks!

------
Kind Regards,
Peter Smith.
Fujitsu Australia