Thread: AlterSubscription_refresh "wrconn" wrong variable?
While reviewing some logical replication code I stumbled across a variable usage that looks suspicious to me. Note that the AlterSubscription_refresh function (unlike other functions in the subscriptioncmds.c) is using the global variable "wrconn" instead of a local stack variable of the same name. I was unable to think of any good reason why it would be deliberately doing this, so my guess is that it is simply an accidental mistake that has gone unnoticed because the compiler was silently equally happy just using the global var. Apparently, this is not causing any reported problems because it seems like the code has been this way for ~4 years [1]. Even so, it doesn't look intentional to me and I felt that there may be unknown consequences (e.g. resource leakage?) of just blatting over the global var. So, PSA a small patch to make this AlterSubscription_refresh function use a stack variable consistent with the other nearby functions. Thoughts? ------ [1] https://github.com/postgres/postgres/commit/7c4f52409a8c7d85ed169bbbc1f6092274d03920# Kind Regards, Peter Smith. Fujitsu Australia
Attachment
On Tue, May 4, 2021 at 5:00 AM Peter Smith <smithpb2250@gmail.com> wrote: > > While reviewing some logical replication code I stumbled across a > variable usage that looks suspicious to me. > > Note that the AlterSubscription_refresh function (unlike other > functions in the subscriptioncmds.c) is using the global variable > "wrconn" instead of a local stack variable of the same name. I was > unable to think of any good reason why it would be deliberately doing > this, so my guess is that it is simply an accidental mistake that has > gone unnoticed because the compiler was silently equally happy just > using the global var. > > Apparently, this is not causing any reported problems because it seems > like the code has been this way for ~4 years [1]. > > Even so, it doesn't look intentional to me and I felt that there may > be unknown consequences (e.g. resource leakage?) of just blatting over > the global var. So, PSA a small patch to make this > AlterSubscription_refresh function use a stack variable consistent > with the other nearby functions. > > Thoughts? +1. It looks like the global variable wrconn defined/declared in worker_internal.h/worker.c, is for logical apply/table sync worker and it doesn't make sense to use it for CREATE/ALTER subscription refresh code that runs on a backend. And I couldn't think of any unknown consequences/resource leakage, because that global variable is being used by different processes which have their own copy. And, the patch basically looks good to me, except a bit of rewording the commit message to something like "Use local variable wrconn in AlterSubscription_refresh instead of global a variable with the same name which is meant to be used for logical apply/table sync worker. Having the wrconn global variable in AlterSubscription_refresh doesn't cause any real issue as such but it keeps the code in subscriptioncmds.c inconsistent with other functions which use a local variable named wrconn." or some other better wording? Regression tests were passed on my dev system with the patch. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
Hi, On 2021-05-04 09:29:42 +1000, Peter Smith wrote: > While reviewing some logical replication code I stumbled across a > variable usage that looks suspicious to me. > Note that the AlterSubscription_refresh function (unlike other > functions in the subscriptioncmds.c) is using the global variable > "wrconn" instead of a local stack variable of the same name. I was > unable to think of any good reason why it would be deliberately doing > this, so my guess is that it is simply an accidental mistake that has > gone unnoticed because the compiler was silently equally happy just > using the global var. > Apparently, this is not causing any reported problems because it seems > like the code has been this way for ~4 years [1]. This sounded vaguely familiar. After a bit of searching I found that's because I debugged a crash related to it: https://www.postgresql.org/message-id/20201111215820.qihhrz7fayu6myfi%40alap3.anarazel.de Peter? Greetings, Andres Freund
On Tue, May 4, 2021 at 1:56 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Tue, May 4, 2021 at 5:00 AM Peter Smith <smithpb2250@gmail.com> wrote: > > > > While reviewing some logical replication code I stumbled across a > > variable usage that looks suspicious to me. > > > > Note that the AlterSubscription_refresh function (unlike other > > functions in the subscriptioncmds.c) is using the global variable > > "wrconn" instead of a local stack variable of the same name. I was > > unable to think of any good reason why it would be deliberately doing > > this, so my guess is that it is simply an accidental mistake that has > > gone unnoticed because the compiler was silently equally happy just > > using the global var. > > > > Apparently, this is not causing any reported problems because it seems > > like the code has been this way for ~4 years [1]. > > > > Even so, it doesn't look intentional to me and I felt that there may > > be unknown consequences (e.g. resource leakage?) of just blatting over > > the global var. So, PSA a small patch to make this > > AlterSubscription_refresh function use a stack variable consistent > > with the other nearby functions. > > > > Thoughts? > > +1. It looks like the global variable wrconn defined/declared in > worker_internal.h/worker.c, is for logical apply/table sync worker and > it doesn't make sense to use it for CREATE/ALTER subscription refresh > code that runs on a backend. And I couldn't think of any unknown > consequences/resource leakage, because that global variable is being > used by different processes which have their own copy. > > And, the patch basically looks good to me, except a bit of rewording > the commit message to something like "Use local variable wrconn in > AlterSubscription_refresh instead of global a variable with the same > name which is meant to be used for logical apply/table sync worker. > Having the wrconn global variable in AlterSubscription_refresh doesn't > cause any real issue as such but it keeps the code in > subscriptioncmds.c inconsistent with other functions which use a local > variable named wrconn." or some other better wording? > > Regression tests were passed on my dev system with the patch. > Thanks for your feedback. I can post another patch (or same patch with an improved commit comment) later, but I will just wait a day first in case there is more information to say about it. e.g. my suspicion that there would be "consequences" seems to have come to fruition after all [1] although I never would have thought of that tricky trigger / refresh scenario. ------ [1] https://www.postgresql.org/message-id/20210504043149.vg4w66vuh4qjrbph%40alap3.anarazel.de Kind Regards, Peter Smith. Fujitsu Australia
On Tue, May 4, 2021 at 2:31 PM Andres Freund <andres@anarazel.de> wrote: > > Hi, > > On 2021-05-04 09:29:42 +1000, Peter Smith wrote: > > While reviewing some logical replication code I stumbled across a > > variable usage that looks suspicious to me. > > > Note that the AlterSubscription_refresh function (unlike other > > functions in the subscriptioncmds.c) is using the global variable > > "wrconn" instead of a local stack variable of the same name. I was > > unable to think of any good reason why it would be deliberately doing > > this, so my guess is that it is simply an accidental mistake that has > > gone unnoticed because the compiler was silently equally happy just > > using the global var. > > > Apparently, this is not causing any reported problems because it seems > > like the code has been this way for ~4 years [1]. > > This sounded vaguely familiar. After a bit of searching I found that's > because I debugged a crash related to it: > https://www.postgresql.org/message-id/20201111215820.qihhrz7fayu6myfi%40alap3.anarazel.de > Oh! No wonder it sounded familiar. It looks like I've just re-discovered the identical problem 5 months after your post. ------ Kind Regards, Peter Smith. Fujitsu Australia
PSA v2 of this patch - it has the same content, but an improved commit comment. I have also added a commitfest entry, https://commitfest.postgresql.org/33/3109/ ------ Kind Regards, Peter Smith Fujitsu Australia
Attachment
Peter Smith <smithpb2250@gmail.com> writes:
> This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making
itconsistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
> The global wrconn is only meant to be used for logical apply/tablesync worker.
> Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is still
posslbleif the apply worker ever manages to do a subscription refresh. e.g. see [1]
Hm. I would actually place the blame for this on whoever thought
it was okay to name a global variable something as generic as
"wrconn". Let's rename that while we're at it, say to something
like "tablesync_wrconn" (feel free to bikeshed).
regards, tom lane
On Wed, May 5, 2021 at 8:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2250@gmail.com> writes: > > This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making itconsistent with other functions in subscriptioncmds.c (e.g. DropSubscription). > > The global wrconn is only meant to be used for logical apply/tablesync worker. > > Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is stillposslble if the apply worker ever manages to do a subscription refresh. e.g. see [1] > > Hm. I would actually place the blame for this on whoever thought > it was okay to name a global variable something as generic as > "wrconn". Let's rename that while we're at it, say to something > like "tablesync_wrconn" (feel free to bikeshed). I don't think "tablesync_wrconn" is the right name, because wrconn is also being used in logical replication apply worker. So something like "apply_worker_wrconn" would be more meaningful. With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
On Wed, May 5, 2021 at 3:20 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Wed, May 5, 2021 at 8:05 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Peter Smith <smithpb2250@gmail.com> writes: > > > This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, makingit consistent with other functions in subscriptioncmds.c (e.g. DropSubscription). > > > The global wrconn is only meant to be used for logical apply/tablesync worker. > > > Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is stillposslble if the apply worker ever manages to do a subscription refresh. e.g. see [1] > > > > Hm. I would actually place the blame for this on whoever thought > > it was okay to name a global variable something as generic as > > "wrconn". Let's rename that while we're at it, say to something > > like "tablesync_wrconn" (feel free to bikeshed). > OK, I am happy to change this but firstly just need some consensus on the new name to use. I hope to avoid changing it, and then changing it 5 more times. > I don't think "tablesync_wrconn" is the right name, because wrconn is > also being used in logical replication apply worker. So something like > "apply_worker_wrconn" would be more meaningful. > Yes. that is better except I wonder if "apply_worker_wrconn" might seem unusual when used by the tablesync worker. My suggestion is "lrep_worker_wrconn" which seems ok for both apply / tablesyn workers. ------ Kind Regards, Peter Smith. Fujitsu Australia
On Wed, May 5, 2021 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Smith <smithpb2250@gmail.com> writes: > > This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name, making itconsistent with other functions in subscriptioncmds.c (e.g. DropSubscription). > > The global wrconn is only meant to be used for logical apply/tablesync worker. > > Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is stillposslble if the apply worker ever manages to do a subscription refresh. e.g. see [1] > > Hm. I would actually place the blame for this on whoever thought > it was okay to name a global variable something as generic as > "wrconn". Let's rename that while we're at it, say to something > like "tablesync_wrconn" (feel free to bikeshed). PSA v3 of the patch. Same as before, but now also renames the global variable from "wrconn" to "lrep_worker_wrconn". ------ Kind Regards, Peter Smith Fujitsu Australia
Attachment
On Thu, 06 May 2021 at 17:08, Peter Smith <smithpb2250@gmail.com> wrote:
> On Wed, May 5, 2021 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Peter Smith <smithpb2250@gmail.com> writes:
>> > This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name,
makingit consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
>> > The global wrconn is only meant to be used for logical apply/tablesync worker.
>> > Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is
stillposslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]
>>
>> Hm. I would actually place the blame for this on whoever thought
>> it was okay to name a global variable something as generic as
>> "wrconn". Let's rename that while we're at it, say to something
>> like "tablesync_wrconn" (feel free to bikeshed).
>
> PSA v3 of the patch. Same as before, but now also renames the global
> variable from "wrconn" to "lrep_worker_wrconn".
>
Thanks for updating patch. I'm confused why we move the walrcv_connect() out of
PG_TRY() block?
+ /* Try to connect to the publisher. */
+ wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
+ if (!wrconn)
+ ereport(ERROR,
+ (errmsg("could not connect to the publisher: %s", err)));
+
PG_TRY();
{
- /* Try to connect to the publisher. */
- wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
- if (!wrconn)
- ereport(ERROR,
- (errmsg("could not connect to the publisher: %s", err)));
-
/* Get the table list from publisher. */
pubrel_names = fetch_table_list(wrconn, sub->publications);
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Thu, May 6, 2021 at 7:18 PM Japin Li <japinli@hotmail.com> wrote:
>
>
> On Thu, 06 May 2021 at 17:08, Peter Smith <smithpb2250@gmail.com> wrote:
> > On Wed, May 5, 2021 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>
> >> Peter Smith <smithpb2250@gmail.com> writes:
> >> > This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name,
makingit consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
> >> > The global wrconn is only meant to be used for logical apply/tablesync worker.
> >> > Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is
stillposslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]
> >>
> >> Hm. I would actually place the blame for this on whoever thought
> >> it was okay to name a global variable something as generic as
> >> "wrconn". Let's rename that while we're at it, say to something
> >> like "tablesync_wrconn" (feel free to bikeshed).
> >
> > PSA v3 of the patch. Same as before, but now also renames the global
> > variable from "wrconn" to "lrep_worker_wrconn".
> >
>
> Thanks for updating patch. I'm confused why we move the walrcv_connect() out of
> PG_TRY() block?
> + /* Try to connect to the publisher. */
> + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> + if (!wrconn)
> + ereport(ERROR,
> + (errmsg("could not connect to the publisher: %s", err)));
> +
> PG_TRY();
> {
> - /* Try to connect to the publisher. */
> - wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> - if (!wrconn)
> - ereport(ERROR,
> - (errmsg("could not connect to the publisher: %s", err)));
> -
> /* Get the table list from publisher. */
> pubrel_names = fetch_table_list(wrconn, sub->publications);
Thanks for your review. Reason for moving that out of the PG_TRY are:
a) It makes code now consistent with other functions using wrconn. See
CreateSubscription, DropSubscription etc
b) It means don't need the wrconn NULL check anymore in the PG_FINALLY
so it simplifies the disconnect.
------
Kind Regards,
Peter Smith.
Fujitsu Australia
On Thu, 06 May 2021 at 17:30, Peter Smith <smithpb2250@gmail.com> wrote:
> On Thu, May 6, 2021 at 7:18 PM Japin Li <japinli@hotmail.com> wrote:
>>
>>
>> On Thu, 06 May 2021 at 17:08, Peter Smith <smithpb2250@gmail.com> wrote:
>> > On Wed, May 5, 2021 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >>
>> >> Peter Smith <smithpb2250@gmail.com> writes:
>> >> > This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name,
makingit consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
>> >> > The global wrconn is only meant to be used for logical apply/tablesync worker.
>> >> > Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm is
stillposslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]
>> >>
>> >> Hm. I would actually place the blame for this on whoever thought
>> >> it was okay to name a global variable something as generic as
>> >> "wrconn". Let's rename that while we're at it, say to something
>> >> like "tablesync_wrconn" (feel free to bikeshed).
>> >
>> > PSA v3 of the patch. Same as before, but now also renames the global
>> > variable from "wrconn" to "lrep_worker_wrconn".
>> >
>>
>> Thanks for updating patch. I'm confused why we move the walrcv_connect() out of
>> PG_TRY() block?
>> + /* Try to connect to the publisher. */
>> + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
>> + if (!wrconn)
>> + ereport(ERROR,
>> + (errmsg("could not connect to the publisher: %s", err)));
>> +
>> PG_TRY();
>> {
>> - /* Try to connect to the publisher. */
>> - wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
>> - if (!wrconn)
>> - ereport(ERROR,
>> - (errmsg("could not connect to the publisher: %s", err)));
>> -
>> /* Get the table list from publisher. */
>> pubrel_names = fetch_table_list(wrconn, sub->publications);
>
> Thanks for your review. Reason for moving that out of the PG_TRY are:
>
> a) It makes code now consistent with other functions using wrconn. See
> CreateSubscription, DropSubscription etc
>
> b) It means don't need the wrconn NULL check anymore in the PG_FINALLY
> so it simplifies the disconnect.
>
Thanks for your explanation!
--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.
On Thu, May 6, 2021 at 3:00 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> On Thu, May 6, 2021 at 7:18 PM Japin Li <japinli@hotmail.com> wrote:
> >
> >
> > On Thu, 06 May 2021 at 17:08, Peter Smith <smithpb2250@gmail.com> wrote:
> > > On Wed, May 5, 2021 at 12:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >>
> > >> Peter Smith <smithpb2250@gmail.com> writes:
> > >> > This patch replaces the global "wrconn" in AlterSubscription_refresh with a local variable of the same name,
makingit consistent with other functions in subscriptioncmds.c (e.g. DropSubscription).
> > >> > The global wrconn is only meant to be used for logical apply/tablesync worker.
> > >> > Using the global/incorrect wrconn in AlterSubscription_refresh doesn't normally cause any problems, but harm
isstill posslble if the apply worker ever manages to do a subscription refresh. e.g. see [1]
> > >>
> > >> Hm. I would actually place the blame for this on whoever thought
> > >> it was okay to name a global variable something as generic as
> > >> "wrconn". Let's rename that while we're at it, say to something
> > >> like "tablesync_wrconn" (feel free to bikeshed).
> > >
> > > PSA v3 of the patch. Same as before, but now also renames the global
> > > variable from "wrconn" to "lrep_worker_wrconn".
> > >
> >
> > Thanks for updating patch. I'm confused why we move the walrcv_connect() out of
> > PG_TRY() block?
> > + /* Try to connect to the publisher. */
> > + wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> > + if (!wrconn)
> > + ereport(ERROR,
> > + (errmsg("could not connect to the publisher: %s", err)));
> > +
> > PG_TRY();
> > {
> > - /* Try to connect to the publisher. */
> > - wrconn = walrcv_connect(sub->conninfo, true, sub->name, &err);
> > - if (!wrconn)
> > - ereport(ERROR,
> > - (errmsg("could not connect to the publisher: %s", err)));
> > -
> > /* Get the table list from publisher. */
> > pubrel_names = fetch_table_list(wrconn, sub->publications);
>
> Thanks for your review. Reason for moving that out of the PG_TRY are:
>
> a) It makes code now consistent with other functions using wrconn. See
> CreateSubscription, DropSubscription etc
>
> b) It means don't need the wrconn NULL check anymore in the PG_FINALLY
> so it simplifies the disconnect.
And even if any error occurs after the connection is established and
while libpqrcv_PQexec is being done in libpqrcv_connect, we reach
PG_FINALLY() block to disconnect the connection, so no connection leak
can occur.
Patch looks good to me except for the comments in the commit message:
1) it crosses 80 char limit 2) a typo : "posslble"
Please add it to the current commitfest if not done already so that we
don't lose track of it and the patch gets a chance to be tested.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
On 2021-May-06, Peter Smith wrote: > PSA v3 of the patch. Same as before, but now also renames the global > variable from "wrconn" to "lrep_worker_wrconn". I think there are two patches here -- the changes to AlterSubscription_refresh are a backpatchable bugfix, and the rest of it can just be applied to master. In my mind we make a bit of a distinction for global variables by using CamelCase rather than undercore_separated_words. There are plenty that violate that "rule" of course, but ISTM that makes them stand more and it's less likely we've made this mistake. So I would name the variable LogRepWALRcvConn or something like that. My €0.02. -- Álvaro Herrera Valdivia, Chile "Entristecido, Wutra (canción de Las Barreras) echa a Freyr a rodar y a nosotros al mar"
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2021-May-06, Peter Smith wrote:
>> PSA v3 of the patch. Same as before, but now also renames the global
>> variable from "wrconn" to "lrep_worker_wrconn".
> I think there are two patches here -- the changes to
> AlterSubscription_refresh are a backpatchable bugfix, and the rest of it
> can just be applied to master.
The rename of that variable is just cosmetic, true, but I'd still be
inclined to back-patch it. If we don't do so then I'm afraid that
future back-patched fixes might be bitten by the same confusion,
possibly introducing new real bugs.
Having said that, keeping the two aspects in separate patches might
ease review and testing.
> In my mind we make a bit of a distinction for global variables by using
> CamelCase rather than undercore_separated_words.
I think it's about 50/50, TBH. I'd stick with whichever style is
being used in nearby code.
regards, tom lane
On Fri, May 7, 2021 at 7:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > On 2021-May-06, Peter Smith wrote: > >> PSA v3 of the patch. Same as before, but now also renames the global > >> variable from "wrconn" to "lrep_worker_wrconn". > > > I think there are two patches here -- the changes to > > AlterSubscription_refresh are a backpatchable bugfix, and the rest of it > > can just be applied to master. > > The rename of that variable is just cosmetic, true, but I'd still be > inclined to back-patch it. If we don't do so then I'm afraid that > future back-patched fixes might be bitten by the same confusion, > possibly introducing new real bugs. > > Having said that, keeping the two aspects in separate patches might > ease review and testing. Done. > > > In my mind we make a bit of a distinction for global variables by using > > CamelCase rather than undercore_separated_words. > > I think it's about 50/50, TBH. I'd stick with whichever style is > being used in nearby code. > The nearby code was a random mixture of Camels and Snakes, so instead of flipping a coin I went with the suggestion from Alvaro. ~~ PSA v4 of the patch. 0001 - Fixes the AlterSubscription_refresh as before. 0002 - Renames the global var "wrconn" -> "LogRepWorkerWalRcvConn" as suggested. ------ Kind Regards, Peter Smith Fujitsu Australia
Attachment
On Fri, May 7, 2021 at 6:09 PM Peter Smith <smithpb2250@gmail.com> wrote: > > On Fri, May 7, 2021 at 7:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > > > On 2021-May-06, Peter Smith wrote: > > >> PSA v3 of the patch. Same as before, but now also renames the global > > >> variable from "wrconn" to "lrep_worker_wrconn". > > > > > I think there are two patches here -- the changes to > > > AlterSubscription_refresh are a backpatchable bugfix, and the rest of it > > > can just be applied to master. > > > > The rename of that variable is just cosmetic, true, but I'd still be > > inclined to back-patch it. If we don't do so then I'm afraid that > > future back-patched fixes might be bitten by the same confusion, > > possibly introducing new real bugs. > > > > Having said that, keeping the two aspects in separate patches might > > ease review and testing. > > Done. > > > > > > In my mind we make a bit of a distinction for global variables by using > > > CamelCase rather than undercore_separated_words. > > > > I think it's about 50/50, TBH. I'd stick with whichever style is > > being used in nearby code. > > > > The nearby code was a random mixture of Camels and Snakes, so instead > of flipping a coin I went with the suggestion from Alvaro. > > ~~ > > PSA v4 of the patch. > > 0001 - Fixes the AlterSubscription_refresh as before. > 0002 - Renames the global var "wrconn" -> "LogRepWorkerWalRcvConn" as suggested. > It seems that the 0001 part of this patch was pushed in the weekend [1]. Thanks! But what about the 0002 part? If there is no immediate plan to push that also then I will post a v5 just to stop the cfbot complaining. -------- [1] https://github.com/postgres/postgres/commit/4e8c0f1a0d0d095a749a329a216c88a340a455b6 KInd Regards, Peter Smith Fujitsu Australia
On Mon, May 10, 2021 at 7:50 AM Peter Smith <smithpb2250@gmail.com> wrote: > > 0001 - Fixes the AlterSubscription_refresh as before. > > 0002 - Renames the global var "wrconn" -> "LogRepWorkerWalRcvConn" as suggested. > > It seems that the 0001 part of this patch was pushed in the weekend [1]. Thanks! > > But what about the 0002 part? If there is no immediate plan to push > that also then I will post a v5 just to stop the cfbot complaining. I think the 0002 patch can be posted here, if it looks good, it can be made "Ready For Committer". With Regards, Bharath Rupireddy. EnterpriseDB: http://www.enterprisedb.com
PSA v5 of the patch. It is the same as v4 but with the v4-0001 part omitted because that was already pushed. (reposted to keep cfbot happy). ------ Kind Regards, Peter Smith Fujitsu Australia
Attachment
On 2021-May-10, Peter Smith wrote: > PSA v5 of the patch. It is the same as v4 but with the v4-0001 part > omitted because that was already pushed. I made a few whitespace adjustments on Friday that I didn't get time to push, so I left the whole set to after the minors are finalized this week. I'll get them pushed on Wednesday or so. (The back branches have a few conflicts, on every release, but I see no reason to post those and it'd upset the cfbot). -- Álvaro Herrera Valdivia, Chile
Attachment
On 2021-May-10, Peter Smith wrote: > PSA v5 of the patch. It is the same as v4 but with the v4-0001 part > omitted because that was already pushed. Thanks, I have pushed this. -- Álvaro Herrera 39°49'30"S 73°17'W