Subject: RE: pg_rewind with --write-recovery-conf option doesn't write dbname to primary_conninfo value.

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 3 Feb 2025 05:50:32 +0000

Dear Sawada-san,

> I think it's a good idea to support it at least on HEAD. I've attached
> a patch for that.

+1. I've confirmed that pg_rewind and -R can't output dbname for now,
and your patch allows to do it.

Few comments for your patch.

1.

pg_basebackup.sgml has below description. I feel this can be ported to
pg_rewind.sgml as well.

```
The dbname will be recorded only if the dbname was
specified explicitly in the connection string or <link linkend="libpq-envars">
environment variable</link>.
```

2.
I'm not sure whether recovery_gen.h/c is a correct place for the exported function
GetDbnameFromConnectionOptions(). The function itself does not handle
postgresql.cuto.conf file.
I seeked other header files and felt that connect_utils.h might be.

```
/*-------------------------------------------------------------------------
 *
 * Facilities for frontend code to connect to and disconnect from databases.
```

Another idea is to change the third API to accept the whole connection string, and
it extracts dbname from it. In this approach we can make GetDbnameFromConnectionOptions()
to static function - which does not feel strange for me.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Peter Smith <[email-masked]>
Date: Mon, 3 Feb 2025 16:59:37 +1100

> I seeked other header files and felt that connect_utils.h might be.

Hi Sawada-san, h

Here are a few more minor comments in addition to what Kuroda-San
already posted.

======
typo in patch name /primary_conninfo/primary_connifo/

======
Commit message

no details.
bad link.

======
src/fe_utils/recovery_gen.c

1.
static char *
FindDbnameInConnParams(PQconninfoOption *conn_opts)

There is a missing forward declaration of this function. Better to add
it for consistency because the other static function has one.

~~~

2.
+static char *
+FindDbnameInConnParams(PQconninfoOption *conn_opts)
+{
+ PQconninfoOption *conn_opt;
+
+ for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++)
+ {
+ if (strcmp(conn_opt->keyword, "dbname") == 0 &&
+ conn_opt->val != NULL && conn_opt->val[0] != '\0')
+ return pg_strdup(conn_opt->val);
+ }
+ return NULL;
+}

2a.
I know you copied this, but it seems a bit strange that this function
is named "Params" when everything about it including its parameter and
comments and the caller talks about "_opts" or "Options"

~

2b.
I know you copied this, but normally 'conn_opt' might have been
written as a for-loop variable.

~~~

3.
+/*
+ * GetDbnameFromConnectionOptions
+ *
+ * This is a special purpose function to retrieve the dbname from either the
+ * 'connstr' specified by the caller or from the environment variables.
+ *
+ * Returns NULL, if dbname is not specified by the user in the above
+ * mentioned connection options.
+ */

What does "in the above mentioned connection options" mean? In the
original function comment where this was copied from there was an
extra sentence ("We follow ... from various connection options.") so
this had more context, but now that the other sentence is removed
maybe "above mentioned connection options" looks like it also needs
rewording.

======
Kind Regards,
Peter Smith.
Fujitsu Australia

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 3 Feb 2025 11:32:28 -0800

> Here are a few more minor comments in addition to what Kuroda-San

Thank you for reviewing the patch.

> typo in patch name /primary_conninfo/primary_connifo/

Will fix.

> bad link.

Yeah, I cannot add the discussion link before sending the patch.

> it for consistency because the other static function has one.

Will fix.

> comments and the caller talks about "_opts" or "Options"

Yes, it seems clearer to use ConnOpts instead.

> written as a for-loop variable.

Fill fix.

> maybe "above mentioned connection options" looks like it also needs

Agreed, will fix.

Regards,

-- 
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 3 Feb 2025 12:36:21 -0800

> Few comments for your patch.

Thank you for reviewing the patch!

Agreed, will fix.

> I seeked other header files and felt that connect_utils.h might be.

But this function neither connects to nor disconnects from databases, either.

> to static function - which does not feel strange for me.

I'm concerned that it reduces the usability; users (or existing
extensions) would need to construct the whole connection string just
to pass the database name. If we want to avoid exposing
GetDbnameFromConnectionOptions(), I'd introduce another exposed
function for that, say GenerateRecoveryConfigWithConnStr().

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 11 Feb 2025 15:56:04 -0800

> function for that, say GenerateRecoveryConfigWithConnStr().

I've attached the updated patch. I address all comments I got so far
and added a small regression test.

It makes sense to me that we move GetDbnameFromConnectionOptions() to
recovery_gen.c since this function is currently used only with
GenerateRecoveryConfig() which is defined in the same file. If we find
a more appropriate place, we can move it later. Feedback is very
welcome.

Regards,

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 12 Feb 2025 05:36:01 +0000

Dear Sawada-san,

Thanks for updating the patch!

> a more appropriate place, we can move it later. Feedback is very

I considered your idea that adding new API, but it seemed for me to have less
benefit. Also, I do not know better place for the declaration now. Overall, the
patch looks good to me.

Best regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Masahiko Sawada <[email-masked]>
Date: Tue, 11 Mar 2025 16:26:15 -0700

> benefit. Also, I do not know better place for the declaration now. Overall, the

I'm going to push the v2 patch, barring any objections and further comments.

Regards,

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 12 Mar 2025 18:05:02 -0700

> I'm going to push the v2 patch, barring any objections and further comments.

Pushed.