Обсуждение: GetSubscriptionRelations declares too many scan keys

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

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

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.

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

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.

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

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

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

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

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