Thread: postgres_fdw: Obsolete comments in GetConnection()

While working on something else, I noticed $SUBJECT, which I should
have updated in commit 27e1f1456.  :-(  There are two places that need
to be updated, but in the first place the second one seemed a bit
redundant to me, because it says the same thing as the first one, and
is placed pretty close to the first one within 10 lines or so.  So I
rewrote the second one entirely into something much more simple like
the attached.

Best regards,
Etsuro Fujita

On Wed, Oct 6, 2021 at 2:35 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
> While working on something else, I noticed $SUBJECT, which I should
> have updated in commit 27e1f1456.  :-(  There are two places that need
> to be updated, but in the first place the second one seemed a bit
> redundant to me, because it says the same thing as the first one, and
> is placed pretty close to the first one within 10 lines or so.  So I
> rewrote the second one entirely into something much more simple like
> the attached.

+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.

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

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.
The word "later" here in the comment below makes sense but not in the
above comment.
+ * detected, we try to reestablish a new connection later.

Regards,
Bharath Rupireddy.

Hi Bharath,

On Wed, Oct 6, 2021 at 6:37 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@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.

> 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.

> 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!

Best regards,
Etsuro Fujita

On Wed, Oct 6, 2021 at 4:55 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
>
> Hi Bharath,
>
> On Wed, Oct 6, 2021 at 6:37 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@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.

Regards,
Bharath Rupireddy.

On Wed, Oct 6, 2021 at 8:39 PM Bharath Rupireddy
<bharath.rupireddyforpostgres@gmail.com> wrote:
> On Wed, Oct 6, 2021 at 4:55 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote:
> > On Wed, Oct 6, 2021 at 6:37 PM Bharath Rupireddy
> > <bharath.rupireddyforpostgres@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