On Wed, Oct 6, 2021 at 8:39 PM Bharath Rupireddy <bharath.rupireddyforpostg...@gmail.com> wrote: > On Wed, Oct 6, 2021 at 4:55 PM Etsuro Fujita <etsuro.fuj...@gmail.com> wrote: > > On Wed, Oct 6, 2021 at 6:37 PM Bharath Rupireddy > > <bharath.rupireddyforpostg...@gmail.com> wrote: > > > +1 for rewording the comments. Here are my thoughts on the patch: > > > > > > 1) Just to be consistent(we are using this word in the error message, > > > and in other comments around there), how about > > > + * Determine whether to try to reestablish the connection. > > > instead of > > > + * Determine whether to try to remake the connection later. > > > > Actually, we use the word “remake” as well in comments in > > connection.c: e.g., “If the connection needs to be *remade* due to > > invalidation, disconnect as soon as we're out of all transactions.” in > > GetConnection(). But I don’t have a strong opinion about that, so > > I’ll change the word as proposed. > > Thanks. > > > > 2) Just to be consistent, how about > > > + * cases where we're starting new transaction (not subtransaction), > > > if a broken connection is > > > instead of > > > + * cases where we're out of all transactions, if a broken connection is > > > > Actually, I modified the comment to match existing comments like the > > one mentioned above. I think the patch would actually be more > > consistent. > > Okay. > > > > 3) IMO we don't need the word "later" here because we are immediately > > > reestablishing the connection, if it is decided to do so. > > > + * Determine whether to try to remake the connection later. > > > > Ok, I’ll drop the word “later”. > > Thanks.
Pushed after modifying the patch as such. Thanks for reviewing! Best regards, Etsuro Fujita
