On Mon, Feb 3, 2025 at 12:36 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Sun, Feb 2, 2025 at 9:50 PM Hayato Kuroda (Fujitsu) > <kuroda.hay...@fujitsu.com> wrote: > > > > 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. > > Thank you for reviewing the 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>. > > ``` > > Agreed, will fix. > > > > > 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. > > ``` > > But this function neither connects to nor disconnects from databases, either. > > > > > 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. > > 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().
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, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
v2-0001-pg_rewind-Add-dbname-to-primary_conninfo-when-usi.patch
Description: Binary data
