Thread: 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
Attachment
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
Attachment
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
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