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
Attachments:
v1-0001-GetSubscriptionRelations-declare-only-1-scan-key.patchapplication/octet-stream; name=v1-0001-GetSubscriptionRelations-declare-only-1-scan-key.patchDownload
From 1c1444699507baf91edbe08d5bf0a5293cc53f55 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Mon, 10 May 2021 16:56:00 +1000
Subject: [PATCH v1] GetSubscriptionRelations declare only 1 scan key.
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.
---
src/backend/catalog/pg_subscription.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index 4039768..7db1f7d 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -461,19 +461,18 @@ GetSubscriptionRelations(Oid subid)
List *res = NIL;
Relation rel;
HeapTuple tup;
- int nkeys = 0;
- ScanKeyData skey[2];
+ ScanKeyData skey[1];
SysScanDesc scan;
rel = table_open(SubscriptionRelRelationId, AccessShareLock);
- ScanKeyInit(&skey[nkeys++],
+ ScanKeyInit(&skey[0],
Anum_pg_subscription_rel_srsubid,
BTEqualStrategyNumber, F_OIDEQ,
ObjectIdGetDatum(subid));
scan = systable_beginscan(rel, InvalidOid, false,
- NULL, nkeys, skey);
+ NULL, 1, skey);
while (HeapTupleIsValid(tup = systable_getnext(scan)))
{
--
1.8.3.1
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