Horton

Thread

Commits

  1. Simplify one use of ScanKey in pg_subscription.c

  2. Logical replication support for initial data copy

  1. GetSubscriptionRelations declares too many scan keys

    Peter Smith <smithpb2250@gmail.com> — 2021-05-10T07:06:11Z

    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

  2. Re: GetSubscriptionRelations declares too many scan keys

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2021-05-10T08:09:31Z

    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

  3. Re: GetSubscriptionRelations declares too many scan keys

    Julien Rouhaud <rjuju123@gmail.com> — 2021-05-10T08:56:40Z

    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.

  4. Re: GetSubscriptionRelations declares too many scan keys

    Peter Smith <smithpb2250@gmail.com> — 2021-05-10T09:09:29Z

    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

  5. Re: GetSubscriptionRelations declares too many scan keys

    Julien Rouhaud <rjuju123@gmail.com> — 2021-05-10T09:16:53Z

    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.

  6. Re: GetSubscriptionRelations declares too many scan keys

    Dilip Kumar <dilipbalaut@gmail.com> — 2021-05-10T09:38:56Z

    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

  7. Re: GetSubscriptionRelations declares too many scan keys

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-05-10T14:14:08Z

    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

  8. Re: GetSubscriptionRelations declares too many scan keys

    Michael Paquier <michael@paquier.xyz> — 2021-05-11T07:50:46Z

    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

  9. Re: GetSubscriptionRelations declares too many scan keys

    Michael Paquier <michael@paquier.xyz> — 2021-05-12T07:52:42Z

    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

  10. Re: GetSubscriptionRelations declares too many scan keys

    Peter Smith <smithpb2250@gmail.com> — 2021-05-12T09:35:19Z

    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