postgres_fdw: Obsolete comments in GetConnection()

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