public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/1] add .gitignore for basebackup_to_shell 13+ messages / 7 participants [nested] [flat]
* [PATCH 1/1] add .gitignore for basebackup_to_shell @ 2022-03-30 22:28 Nathan Bossart <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Nathan Bossart @ 2022-03-30 22:28 UTC (permalink / raw) --- contrib/basebackup_to_shell/.gitignore | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 contrib/basebackup_to_shell/.gitignore diff --git a/contrib/basebackup_to_shell/.gitignore b/contrib/basebackup_to_shell/.gitignore new file mode 100644 index 0000000000..5dcb3ff972 --- /dev/null +++ b/contrib/basebackup_to_shell/.gitignore @@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ -- 2.25.1 --3V7upXqbjpZ4EhLz-- ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-20 14:26 Robert Haas <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Robert Haas @ 2024-02-20 14:26 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Ian Lawrence Barwick <[email protected]>; pgsql-hackers On Tue, Feb 20, 2024 at 5:58 PM Hayato Kuroda (Fujitsu) <[email protected]> wrote: > > Seems weird to me. You don't use dbname=replication to ask for a > > replication connection, so why would we ever end up with that > > anywhere? And especially in only one of two such closely related > > cases? > > Just FYI - here is an extreme case. And note that I have applied proposed patch. > > When `pg_basebackup -D data_N2 -R` is used: > ``` > primary_conninfo = 'user=hayato ... dbname=hayato ... > ``` > > But when `pg_basebackup -d "" -D data_N2 -R` is used: > ``` > primary_conninfo = 'user=hayato ... dbname=replication > ``` It seems like maybe somebody should look into why this is happening, and perhaps fix it. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-21 03:04 Kyotaro Horiguchi <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 4 replies; 13+ messages in thread From: Kyotaro Horiguchi @ 2024-02-21 03:04 UTC (permalink / raw) To: [email protected]; +Cc: [email protected]; [email protected]; pgsql-hackers At Tue, 20 Feb 2024 19:56:10 +0530, Robert Haas <[email protected]> wrote in > It seems like maybe somebody should look into why this is happening, > and perhaps fix it. GetConnection()@streamutil.c wants to ensure conninfo has a fallback database name ("replication"). However, the function seems to be ignoring the case where neither dbname nor connection string is given, which is the first case Kuroda-san raised. The second case is the intended behavior of the function. > /* pg_recvlogical uses dbname only; others use connection_string only. */ > Assert(dbname == NULL || connection_string == NULL); And the function incorrectly assumes that the connection string requires "dbname=replication". > * Merge the connection info inputs given in form of connection string, > * options and default values (dbname=replication, replication=true, etc.) But the name is a pseudo database name only used by pg_hba.conf (maybe) , which cannot be used as an actual database name (without quotation marks, or unless it is actually created). The function should not add the fallback database name because the connection string for physical replication connection doesn't require the dbname parameter. (attached patch) About the proposed patch, pg_basebackup cannot verify the validity of the dbname. It could be problematic. Although I haven't looked the original thread, it seems that the dbname is used only by pg_sync_replication_slots(). If it is true, couldn't we make the SQL function require a database name to make a connection, instead of requiring it in physical-replication conninfo? regards. -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c index 56d1b15951..da63a04de4 100644 --- a/src/bin/pg_basebackup/streamutil.c +++ b/src/bin/pg_basebackup/streamutil.c @@ -78,7 +78,7 @@ GetConnection(void) /* * Merge the connection info inputs given in form of connection string, - * options and default values (dbname=replication, replication=true, etc.) + * options and default values (replication=true, etc.) */ i = 0; if (connection_string) @@ -96,14 +96,6 @@ GetConnection(void) keywords = pg_malloc0((argcount + 1) * sizeof(*keywords)); values = pg_malloc0((argcount + 1) * sizeof(*values)); - /* - * Set dbname here already, so it can be overridden by a dbname in the - * connection string. - */ - keywords[i] = "dbname"; - values[i] = "replication"; - i++; - for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++) { if (conn_opt->val != NULL && conn_opt->val[0] != '\0') Attachments: [text/plain] do_not_set_fallback_dbname.txt (969B, ../../[email protected]/2-do_not_set_fallback_dbname.txt) download | inline diff: diff --git a/src/bin/pg_basebackup/streamutil.c b/src/bin/pg_basebackup/streamutil.c index 56d1b15951..da63a04de4 100644 --- a/src/bin/pg_basebackup/streamutil.c +++ b/src/bin/pg_basebackup/streamutil.c @@ -78,7 +78,7 @@ GetConnection(void) /* * Merge the connection info inputs given in form of connection string, - * options and default values (dbname=replication, replication=true, etc.) + * options and default values (replication=true, etc.) */ i = 0; if (connection_string) @@ -96,14 +96,6 @@ GetConnection(void) keywords = pg_malloc0((argcount + 1) * sizeof(*keywords)); values = pg_malloc0((argcount + 1) * sizeof(*values)); - /* - * Set dbname here already, so it can be overridden by a dbname in the - * connection string. - */ - keywords[i] = "dbname"; - values[i] = "replication"; - i++; - for (conn_opt = conn_opts; conn_opt->keyword != NULL; conn_opt++) { if (conn_opt->val != NULL && conn_opt->val[0] != '\0') ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-21 03:37 Ajin Cherian <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 3 siblings, 0 replies; 13+ messages in thread From: Ajin Cherian @ 2024-02-21 03:37 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers On Wed, Feb 21, 2024 at 2:04 PM Kyotaro Horiguchi <[email protected]> wrote: > > About the proposed patch, pg_basebackup cannot verify the validity of > the dbname. It could be problematic. > > Although I haven't looked the original thread, it seems that the > dbname is used only by pg_sync_replication_slots(). If it is true, > couldn't we make the SQL function require a database name to make a > connection, instead of requiring it in physical-replication conninfo? > > regards. > > -- > Kyotaro Horiguchi > NTT Open Source Software Center > I agree. If the intention is to meet the new requirement of the sync-slot patch which requires a dbname in the primary_conninfo, then pseudo dbnames will not work, whether it be the username or just "replication". I feel if the user does not specify dbname explicitly in pg_basebackup it should be left blank in the generated primary_conninfo string as well. regards, Ajin Cherian Fujitsu Australia ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-21 03:41 Ajin Cherian <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 3 siblings, 0 replies; 13+ messages in thread From: Ajin Cherian @ 2024-02-21 03:41 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers On Wed, Feb 21, 2024 at 2:04 PM Kyotaro Horiguchi <[email protected]> wrote: > > Although I haven't looked the original thread, it seems that the > dbname is used only by pg_sync_replication_slots(). If it is true, > couldn't we make the SQL function require a database name to make a > connection, instead of requiring it in physical-replication conninfo? > > > In the original thread, the intention is to not just provide this functionality using the function pg_sync_replication_slots(), but provide a GUC option on standbys to sync logical replication slots periodically even without calling that function. This requires connecting to a database. regards, Ajin Cherian Fujitsu Australia ^ permalink raw reply [nested|flat] 13+ messages in thread
* RE: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-21 05:08 Hayato Kuroda (Fujitsu) <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 3 siblings, 1 reply; 13+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2024-02-21 05:08 UTC (permalink / raw) To: 'Kyotaro Horiguchi' <[email protected]>; +Cc: [email protected] <[email protected]>; pgsql-hackers; [email protected] <[email protected]> Dear Horiguchi-san, > GetConnection()@streamutil.c wants to ensure conninfo has a fallback > database name ("replication"). However, the function seems to be > ignoring the case where neither dbname nor connection string is given, > which is the first case Kuroda-san raised. The second case is the > intended behavior of the function. > > > /* pg_recvlogical uses dbname only; others use connection_string only. > */ > > Assert(dbname == NULL || connection_string == NULL); > > And the function incorrectly assumes that the connection string > requires "dbname=replication". > > > * Merge the connection info inputs given in form of connection string, > > * options and default values (dbname=replication, replication=true, > etc.) > > But the name is a pseudo database name only used by pg_hba.conf > (maybe) , which cannot be used as an actual database name (without > quotation marks, or unless it is actually created). The function > should not add the fallback database name because the connection > string for physical replication connection doesn't require the dbname > parameter. (attached patch) I was also missing, but the requirement was that the dbname should be included only when the dbname option was explicitly specified [1]. Even mine and yours cannot handle like that. Libpq function PQconnectdbParams()->pqConnectOptions2() fills all the parameter to PGconn, at that time the information whether it is intentionally specified or not is discarded. Then, GenerateRecoveryConfig() would just write down all the connection parameters from PGconn, they cannot recognize. One approach is that based on Horiguchi-san's one and initial patch, we can avoid writing when the dbname is same as the username. [1]: https://www.postgresql.org/message-id/CAA4eK1KH1d1J5giPMZVOtMe0iqncf1CpNwkBKoYAmXdC-kEGZg%40mail.gma... Best Regards, Hayato Kuroda FUJITSU LIMITED https://www.fujitsu.com/ ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-21 05:28 Ajin Cherian <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Ajin Cherian @ 2024-02-21 05:28 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected] <[email protected]>; pgsql-hackers; [email protected] <[email protected]> On Wed, Feb 21, 2024 at 4:09 PM Hayato Kuroda (Fujitsu) < [email protected]> wrote: > Dear Horiguchi-san, > > > GetConnection()@streamutil.c wants to ensure conninfo has a fallback > > database name ("replication"). However, the function seems to be > > ignoring the case where neither dbname nor connection string is given, > > which is the first case Kuroda-san raised. The second case is the > > intended behavior of the function. > > > > > /* pg_recvlogical uses dbname only; others use connection_string > only. > > */ > > > Assert(dbname == NULL || connection_string == NULL); > > > > And the function incorrectly assumes that the connection string > > requires "dbname=replication". > > > > > * Merge the connection info inputs given in form of connection > string, > > > * options and default values (dbname=replication, > replication=true, > > etc.) > > > > But the name is a pseudo database name only used by pg_hba.conf > > (maybe) , which cannot be used as an actual database name (without > > quotation marks, or unless it is actually created). The function > > should not add the fallback database name because the connection > > string for physical replication connection doesn't require the dbname > > parameter. (attached patch) > > I was also missing, but the requirement was that the dbname should be > included > only when the dbname option was explicitly specified [1]. Even mine and > yours > cannot handle like that. Libpq function > PQconnectdbParams()->pqConnectOptions2() > fills all the parameter to PGconn, at that time the information whether it > is > intentionally specified or not is discarded. Then, > GenerateRecoveryConfig() would > just write down all the connection parameters from PGconn, they cannot > recognize. > > Well, one option is that if a default dbname should be written to the configuration file, then "postgres' is a better option than "replication" or "username" as the default option, at least a db of that name exists. regards, Ajin Cherian Fujitsu Australia ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-23 06:47 Amit Kapila <[email protected]> parent: Kyotaro Horiguchi <[email protected]> 3 siblings, 1 reply; 13+ messages in thread From: Amit Kapila @ 2024-02-23 06:47 UTC (permalink / raw) To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; [email protected]; [email protected]; pgsql-hackers On Wed, Feb 21, 2024 at 8:34 AM Kyotaro Horiguchi <[email protected]> wrote: > > At Tue, 20 Feb 2024 19:56:10 +0530, Robert Haas <[email protected]> wrote in > > It seems like maybe somebody should look into why this is happening, > > and perhaps fix it. > > GetConnection()@streamutil.c wants to ensure conninfo has a fallback > database name ("replication"). However, the function seems to be > ignoring the case where neither dbname nor connection string is given, > which is the first case Kuroda-san raised. The second case is the > intended behavior of the function. > > > /* pg_recvlogical uses dbname only; others use connection_string only. */ > > Assert(dbname == NULL || connection_string == NULL); > > And the function incorrectly assumes that the connection string > requires "dbname=replication". > > > * Merge the connection info inputs given in form of connection string, > > * options and default values (dbname=replication, replication=true, etc.) > > But the name is a pseudo database name only used by pg_hba.conf > (maybe) , which cannot be used as an actual database name (without > quotation marks, or unless it is actually created). The function > should not add the fallback database name because the connection > string for physical replication connection doesn't require the dbname > parameter. (attached patch) > We do append dbname=replication even in libpqrcv_connect for .pgpass lookup as mentioned in comments. See below: libpqrcv_connect() { .... else { /* * The database name is ignored by the server in replication mode, * but specify "replication" for .pgpass lookup. */ keys[++i] = "dbname"; vals[i] = "replication"; } ... } I think as part of this effort we should just add dbname to primary_conninfo written in postgresql.auto.conf file. As above, the question is valid whether we should do it just for 17 or for prior versions. Let's discuss more on that. I am not sure of the use case for versions before 17 but commit cca97ce6a665 mentioned that some middleware or proxies might however need to know the dbname to make the correct routing decision for the connection. Does that apply here as well? If so, we can do it and update the docs, otherwise, let's do it just for 17. -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 13+ messages in thread
* RE: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-27 08:30 Hayato Kuroda (Fujitsu) <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2024-02-27 08:30 UTC (permalink / raw) To: 'Amit Kapila' <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers; Kyotaro Horiguchi <[email protected]> Dear Amit, > We do append dbname=replication even in libpqrcv_connect for .pgpass > lookup as mentioned in comments. See below: > libpqrcv_connect() > { > .... > else > { > /* > * The database name is ignored by the server in replication mode, > * but specify "replication" for .pgpass lookup. > */ > keys[++i] = "dbname"; > vals[i] = "replication"; > } > ... > } OK. So we must add the value for the authorization, right? I think it should be described even in GetConnection(). > I think as part of this effort we should just add dbname to > primary_conninfo written in postgresql.auto.conf file. As above, the > question is valid whether we should do it just for 17 or for prior > versions. Let's discuss more on that. I am not sure of the use case > for versions before 17 but commit cca97ce6a665 mentioned that some > middleware or proxies might however need to know the dbname to make > the correct routing decision for the connection. Does that apply here > as well? If so, we can do it and update the docs, otherwise, let's do > it just for 17. Hmm, I might lose your requirements again. So, we must satisfy all of below ones, right? 1) add {"dbname", "replication"} key-value pair to look up .pgpass file correctly. 2) If the -R is given, output dbname=xxx value to be used by slotsync worker. 3) If the dbname is not given in the connection string, the same string as username must be used to keep the libpq connection rule. 4) No need to add dbname=replication pair PSA the patch for implementing it. It is basically same as Ian's one. However, this patch still cannot satisfy the condition 3). `pg_basebackup -D data_N2 -d "user=postgres" -R` -> dbname would not be appeared in primary_conninfo. This is because `if (connection_string)` case in GetConnection() explicy override a dbname to "replication". I've tried to add a dummy entry {"dbname", NULL} pair before the overriding, but it is no-op. Because The replacement of the dbname in pqConnectOptions2() would be done only for the valid (=lastly specified) connection options. Best Regards, Hayato Kuroda FUJITSU LIMITED https://www.fujitsu.com/ Attachments: [application/octet-stream] pg_basebackup-write-dbname.v0002.patch (1.6K, ../../TYCPR01MB1207779B65625AAEC975B5EC0F5592@TYCPR01MB12077.jpnprd01.prod.outlook.com/2-pg_basebackup-write-dbname.v0002.patch) download | inline diff: diff --git a/src/fe_utils/recovery_gen.c b/src/fe_utils/recovery_gen.c index 2585f11939..c3eb1a59a0 100644 --- a/src/fe_utils/recovery_gen.c +++ b/src/fe_utils/recovery_gen.c @@ -49,12 +49,20 @@ GenerateRecoveryConfig(PGconn *pgconn, char *replication_slot) { /* Omit empty settings and those libpqwalreceiver overrides. */ if (strcmp(opt->keyword, "replication") == 0 || - strcmp(opt->keyword, "dbname") == 0 || strcmp(opt->keyword, "fallback_application_name") == 0 || (opt->val == NULL) || (opt->val != NULL && opt->val[0] == '\0')) continue; + /* + * Omit "dbname" if the special value "replication" is specified, or + * the server version is 16 and earlier. + */ + if (strcmp(opt->keyword, "dbname") == 0 && + (strcmp(opt->val, "replication") == 0 || + PQserverVersion(pgconn) < MINIMUM_VERSION_FOR_DBNAME_PARAM)) + continue; + /* Separate key-value pairs with spaces */ if (conninfo_buf.len != 0) appendPQExpBufferChar(&conninfo_buf, ' '); diff --git a/src/include/fe_utils/recovery_gen.h b/src/include/fe_utils/recovery_gen.h index ca2c4800d0..740d90c9d8 100644 --- a/src/include/fe_utils/recovery_gen.h +++ b/src/include/fe_utils/recovery_gen.h @@ -20,6 +20,12 @@ */ #define MINIMUM_VERSION_FOR_RECOVERY_GUC 120000 +/* + * from version 17, there is a use-case for adding the dbname parameter + * to the generated "primary_conninfo" value + */ +#define MINIMUM_VERSION_FOR_DBNAME_PARAM 170000 + extern PQExpBuffer GenerateRecoveryConfig(PGconn *pgconn, char *replication_slot); extern void WriteRecoveryConfig(PGconn *pgconn, char *target_dir, ^ permalink raw reply [nested|flat] 13+ messages in thread
* RE: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-02-27 08:37 Hayato Kuroda (Fujitsu) <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Hayato Kuroda (Fujitsu) @ 2024-02-27 08:37 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; 'Amit Kapila' <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers; Kyotaro Horiguchi <[email protected]> > PSA the patch for implementing it. It is basically same as Ian's one. > However, this patch still cannot satisfy the condition 3). > > `pg_basebackup -D data_N2 -d "user=postgres" -R` > -> dbname would not be appeared in primary_conninfo. > > This is because `if (connection_string)` case in GetConnection() explicy override > a dbname to "replication". I've tried to add a dummy entry {"dbname", NULL} pair > before the overriding, but it is no-op. Because The replacement of the dbname in > pqConnectOptions2() would be done only for the valid (=lastly specified) > connection options. Oh, this patch missed the straightforward case: pg_basebackup -D data_N2 -d "user=postgres dbname=replication" -R -> dbname would not be appeared in primary_conninfo. So I think it cannot be applied as-is. Sorry for sharing the bad item. Best Regards, Hayato Kuroda FUJITSU LIMITED https://www.fujitsu.com/ ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-03-11 11:45 Amit Kapila <[email protected]> parent: Hayato Kuroda (Fujitsu) <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: Amit Kapila @ 2024-03-11 11:45 UTC (permalink / raw) To: Hayato Kuroda (Fujitsu) <[email protected]>; +Cc: [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers; Kyotaro Horiguchi <[email protected]> On Tue, Feb 27, 2024 at 2:07 PM Hayato Kuroda (Fujitsu) <[email protected]> wrote: > > > PSA the patch for implementing it. It is basically same as Ian's one. > > However, this patch still cannot satisfy the condition 3). > > > > `pg_basebackup -D data_N2 -d "user=postgres" -R` > > -> dbname would not be appeared in primary_conninfo. > > > > This is because `if (connection_string)` case in GetConnection() explicy override > > a dbname to "replication". I've tried to add a dummy entry {"dbname", NULL} pair > > before the overriding, but it is no-op. Because The replacement of the dbname in > > pqConnectOptions2() would be done only for the valid (=lastly specified) > > connection options. > > Oh, this patch missed the straightforward case: > > pg_basebackup -D data_N2 -d "user=postgres dbname=replication" -R > -> dbname would not be appeared in primary_conninfo. > > So I think it cannot be applied as-is. Sorry for sharing the bad item. > Can you please share the patch that can be considered for review? -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-03-12 11:42 vignesh C <[email protected]> parent: Amit Kapila <[email protected]> 0 siblings, 1 reply; 13+ messages in thread From: vignesh C @ 2024-03-12 11:42 UTC (permalink / raw) To: Amit Kapila <[email protected]>; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers; Kyotaro Horiguchi <[email protected]> On Mon, 11 Mar 2024 at 17:16, Amit Kapila <[email protected]> wrote: > > On Tue, Feb 27, 2024 at 2:07 PM Hayato Kuroda (Fujitsu) > <[email protected]> wrote: > > > > > PSA the patch for implementing it. It is basically same as Ian's one. > > > However, this patch still cannot satisfy the condition 3). > > > > > > `pg_basebackup -D data_N2 -d "user=postgres" -R` > > > -> dbname would not be appeared in primary_conninfo. > > > > > > This is because `if (connection_string)` case in GetConnection() explicy override > > > a dbname to "replication". I've tried to add a dummy entry {"dbname", NULL} pair > > > before the overriding, but it is no-op. Because The replacement of the dbname in > > > pqConnectOptions2() would be done only for the valid (=lastly specified) > > > connection options. > > > > Oh, this patch missed the straightforward case: > > > > pg_basebackup -D data_N2 -d "user=postgres dbname=replication" -R > > -> dbname would not be appeared in primary_conninfo. > > > > So I think it cannot be applied as-is. Sorry for sharing the bad item. > > > > Can you please share the patch that can be considered for review? Here is a patch with few changes: a) Removed the version check based on discussion from [1] b) updated the documentation. I have tested various scenarios with the attached patch, the dbname that is written in postgresql.auto.conf for each of the cases with pg_basebackup is given below: 1) ./pg_basebackup -U vignesh -R -> primary_conninfo = "dbname=vignesh" (In this case primary_conninfo will have dbname as username specified) 2) ./pg_basebackup -D data -R -> primary_conninfo = "dbname=vignesh" (In this case primary_conninfo will have the dbname as username (which is the same as the OS user while setting defaults)) 3) ./pg_basebackup -d "user=vignesh" -D data -R -> primary_conninfo = "dbname=replication" (In this case primary_conninfo will have dbname as replication which is the default value from GetConnection as connection string is specified) 4) ./pg_basebackup -d "user=vignesh dbname=postgres" -D data -R -> primary_conninfo = "dbname=postgres" (In this case primary_conninfo will have the dbname as the dbname specified) 5) ./pg_basebackup -d "" -D data -R -> primary_conninfo = "dbname=replication" (In this case primary_conninfo will have dbname as replication which is the default value from GetConnection as connection string is specified) I have mentioned the reasons as to why dbname is being set to replication or username in each of the cases. How about replacing these values in postgresql.auto.conf manually. [1] - https://www.postgresql.org/message-id/CAGECzQTh9oB3nu98DsHMpRaVaqXPDRgTDEikY82OAKYF0%3DhVMA%40mail.g... Regards, Vignesh Attachments: [text/x-patch] pg_basebackup-write-dbname.v0003.patch (1.4K, ../../CALDaNm0tr-t_L+anY1+qseAajpdE1H-cFQzcPzXoRJWUjvOdXg@mail.gmail.com/2-pg_basebackup-write-dbname.v0003.patch) download | inline diff: diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml index 88c689e725..1a6980d1b0 100644 --- a/doc/src/sgml/ref/pg_basebackup.sgml +++ b/doc/src/sgml/ref/pg_basebackup.sgml @@ -243,7 +243,10 @@ PostgreSQL documentation The <filename>postgresql.auto.conf</filename> file will record the connection settings and, if specified, the replication slot that <application>pg_basebackup</application> is using, so that - streaming replication will use the same settings later on. + a) synchronization of logical replication slots on the primary to the + hot_standby from <link linkend="pg-sync-replication-slots"> + <function>pg_sync_replication_slots</function></link> or slot sync worker + and b) streaming replication will use the same settings later on. </para> </listitem> diff --git a/src/fe_utils/recovery_gen.c b/src/fe_utils/recovery_gen.c index 2585f11939..cb37703ab9 100644 --- a/src/fe_utils/recovery_gen.c +++ b/src/fe_utils/recovery_gen.c @@ -49,7 +49,6 @@ GenerateRecoveryConfig(PGconn *pgconn, char *replication_slot) { /* Omit empty settings and those libpqwalreceiver overrides. */ if (strcmp(opt->keyword, "replication") == 0 || - strcmp(opt->keyword, "dbname") == 0 || strcmp(opt->keyword, "fallback_application_name") == 0 || (opt->val == NULL) || (opt->val != NULL && opt->val[0] == '\0')) ^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Have pg_basebackup write "dbname" in "primary_conninfo"? @ 2024-03-13 11:28 Amit Kapila <[email protected]> parent: vignesh C <[email protected]> 0 siblings, 0 replies; 13+ messages in thread From: Amit Kapila @ 2024-03-13 11:28 UTC (permalink / raw) To: vignesh C <[email protected]>; +Cc: Hayato Kuroda (Fujitsu) <[email protected]>; [email protected] <[email protected]>; [email protected] <[email protected]>; pgsql-hackers; Kyotaro Horiguchi <[email protected]> On Tue, Mar 12, 2024 at 5:13 PM vignesh C <[email protected]> wrote: > > On Mon, 11 Mar 2024 at 17:16, Amit Kapila <[email protected]> wrote: > > > > On Tue, Feb 27, 2024 at 2:07 PM Hayato Kuroda (Fujitsu) > > <[email protected]> wrote: > > > > > > > PSA the patch for implementing it. It is basically same as Ian's one. > > > > However, this patch still cannot satisfy the condition 3). > > > > > > > > `pg_basebackup -D data_N2 -d "user=postgres" -R` > > > > -> dbname would not be appeared in primary_conninfo. > > > > > > > > This is because `if (connection_string)` case in GetConnection() explicy override > > > > a dbname to "replication". I've tried to add a dummy entry {"dbname", NULL} pair > > > > before the overriding, but it is no-op. Because The replacement of the dbname in > > > > pqConnectOptions2() would be done only for the valid (=lastly specified) > > > > connection options. > > > > > > Oh, this patch missed the straightforward case: > > > > > > pg_basebackup -D data_N2 -d "user=postgres dbname=replication" -R > > > -> dbname would not be appeared in primary_conninfo. > > > > > > So I think it cannot be applied as-is. Sorry for sharing the bad item. > > > > > > > Can you please share the patch that can be considered for review? > > Here is a patch with few changes: a) Removed the version check based > on discussion from [1] b) updated the documentation. > I have tested various scenarios with the attached patch, the dbname > that is written in postgresql.auto.conf for each of the cases with > pg_basebackup is given below: > 1) ./pg_basebackup -U vignesh -R > -> primary_conninfo = "dbname=vignesh" (In this case primary_conninfo > will have dbname as username specified) > 2) ./pg_basebackup -D data -R > -> primary_conninfo = "dbname=vignesh" (In this case primary_conninfo > will have the dbname as username (which is the same as the OS user > while setting defaults)) > 3) ./pg_basebackup -d "user=vignesh" -D data -R > -> primary_conninfo = "dbname=replication" (In this case > primary_conninfo will have dbname as replication which is the default > value from GetConnection as connection string is specified) > 4) ./pg_basebackup -d "user=vignesh dbname=postgres" -D data -R > -> primary_conninfo = "dbname=postgres" (In this case primary_conninfo > will have the dbname as the dbname specified) > 5) ./pg_basebackup -d "" -D data -R > -> primary_conninfo = "dbname=replication" (In this case > primary_conninfo will have dbname as replication which is the default > value from GetConnection as connection string is specified) > > I have mentioned the reasons as to why dbname is being set to > replication or username in each of the cases. > IIUC, the dbname is already allowed in connstring for pg_basebackup by commit cca97ce6a6 and with this patch we are just writing it in postgresql.auto.conf if -R option is used to write recovery info. This will help slot syncworker to automatically connect with database without user manually specifying the dbname and replication connection, the same will still be ignored. I don't see any problem with this. Does anyone else see any problem? The <filename>postgresql.auto.conf</filename> file will record the connection settings and, if specified, the replication slot that <application>pg_basebackup</application> is using, so that - streaming replication will use the same settings later on. + a) synchronization of logical replication slots on the primary to the + hot_standby from <link linkend="pg-sync-replication-slots"> + <function>pg_sync_replication_slots</function></link> or slot sync worker + and b) streaming replication will use the same settings later on. We can simply modify the last line as: ".. streaming replication or logical replication slots synchronization will use the same settings later on." Additionally, you can give a link reference to [1]. [1] - https://www.postgresql.org/docs/devel/logicaldecoding-explanation.html#LOGICALDECODING-REPLICATION-S... -- With Regards, Amit Kapila. ^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2024-03-13 11:28 UTC | newest] Thread overview: 13+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2022-03-30 22:28 [PATCH 1/1] add .gitignore for basebackup_to_shell Nathan Bossart <[email protected]> 2024-02-20 14:26 Re: Have pg_basebackup write "dbname" in "primary_conninfo"? Robert Haas <[email protected]> 2024-02-21 03:04 ` Re: Have pg_basebackup write "dbname" in "primary_conninfo"? Kyotaro Horiguchi <[email protected]> 2024-02-21 03:37 ` Re: Have pg_basebackup write "dbname" in "primary_conninfo"? Ajin Cherian <[email protected]> 2024-02-21 03:41 ` Re: Have pg_basebackup write "dbname" in "primary_conninfo"? Ajin Cherian <[email protected]> 2024-02-21 05:08 ` RE: Have pg_basebackup write "dbname" in "primary_conninfo"? Hayato Kuroda (Fujitsu) <[email protected]> 2024-02-21 05:28 ` Re: Have pg_basebackup write "dbname" in "primary_conninfo"? Ajin Cherian <[email protected]> 2024-02-23 06:47 ` Re: Have pg_basebackup write "dbname" in "primary_conninfo"? Amit Kapila <[email protected]> 2024-02-27 08:30 ` RE: Have pg_basebackup write "dbname" in "primary_conninfo"? Hayato Kuroda (Fujitsu) <[email protected]> 2024-02-27 08:37 ` RE: Have pg_basebackup write "dbname" in "primary_conninfo"? Hayato Kuroda (Fujitsu) <[email protected]> 2024-03-11 11:45 ` Re: Have pg_basebackup write "dbname" in "primary_conninfo"? Amit Kapila <[email protected]> 2024-03-12 11:42 ` Re: Have pg_basebackup write "dbname" in "primary_conninfo"? vignesh C <[email protected]> 2024-03-13 11:28 ` Re: Have pg_basebackup write "dbname" in "primary_conninfo"? Amit Kapila <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox