From cbe71bac54ea2cdd0d352a91bda85746bf32ef72 Mon Sep 17 00:00:00 2001 From: Michael Paquier Date: Mon, 19 Oct 2026 07:35:06 +0900 Subject: [PATCH] libpq: Revert the connection option "servicefile" The following commits are reverted: * 092f3c63efc6, libpq: Add "servicefile" connection option * 6b1c4d326b06, psql: Add variable SERVICEFILE * 90354030b8fc, libpq: Fix "servicefile" after fallback to system service file After more review, PGSERVICEFILE is sufficient for the purpose of specifying a per-session service file. Some details around this feature will need more attention before being actually usable, if we ever get down to it. One independent piece that has been committed during the v19 development cycle is df286a5b830a, which is kept in the tree as that was an unrelated change. Reported-by: Sehrope Sarkuni Discussion: https://postgr.es/m/CAH7T-aq+U6GDPO8mr3Gga3KzxGpAE=BSLb38x+R8fvWjzF-H8w@mail.gmail.com Backpatch-through: 19 --- doc/src/sgml/libpq.sgml | 24 +++----- doc/src/sgml/ref/psql-ref.sgml | 9 --- src/bin/psql/command.c | 7 --- src/interfaces/libpq/fe-connect.c | 66 ++------------------ src/interfaces/libpq/libpq-int.h | 2 - src/interfaces/libpq/t/006_service.pl | 86 +-------------------------- 6 files changed, 13 insertions(+), 181 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index a1e82fea215..e17f18b9ea4 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -2320,19 +2320,6 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname - - servicefile - - - This option specifies the name of the per-user connection service file - (see ). - Defaults to ~/.pg_service.conf, or - %APPDATA%\postgresql\.pg_service.conf on - Microsoft Windows. - - - - target_session_attrs @@ -9170,8 +9157,12 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough) PGSERVICEFILE - PGSERVICEFILE behaves the same as the - connection parameter. + PGSERVICEFILE specifies the name of the per-user + connection service file + (see ). + Defaults to ~/.pg_service.conf, or + %APPDATA%\postgresql\.pg_service.conf on + Microsoft Windows. @@ -9612,8 +9603,7 @@ myEventProc(PGEventId evtId, void *evtInfo, void *passThrough) On Microsoft Windows, it is named %APPDATA%\postgresql\.pg_service.conf (where %APPDATA% refers to the Application Data subdirectory - in the user's profile). A different file name can be specified using the - servicefile key word in a libpq connection string or by + in the user's profile). A different file name can be specified by setting the environment variable PGSERVICEFILE. The system-wide file is named pg_service.conf. By default it is sought in the etc directory diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index a08c9332384..6c437cc5ac0 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -4688,15 +4688,6 @@ bar - - SERVICEFILE - - - The service file name, if applicable. - - - - SHELL_ERROR diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index f130687d7ed..f5fe9d2fb9e 100644 --- a/src/bin/psql/command.c +++ b/src/bin/psql/command.c @@ -4587,7 +4587,6 @@ SyncVariables(void) char vbuf[32]; const char *server_version; char *service_name; - char *service_file; /* get stuff from connection */ pset.encoding = PQclientEncoding(pset.db); @@ -4607,11 +4606,6 @@ SyncVariables(void) if (service_name) pg_free(service_name); - service_file = get_conninfo_value("servicefile"); - SetVariable(pset.vars, "SERVICEFILE", service_file); - if (service_file) - pg_free(service_file); - /* this bit should match connection_warnings(): */ /* Try to get full text form of version, might include "devel" etc */ server_version = PQparameterStatus(pset.db, "server_version"); @@ -4641,7 +4635,6 @@ UnsyncVariables(void) { SetVariable(pset.vars, "DBNAME", NULL); SetVariable(pset.vars, "SERVICE", NULL); - SetVariable(pset.vars, "SERVICEFILE", NULL); SetVariable(pset.vars, "USER", NULL); SetVariable(pset.vars, "HOST", NULL); SetVariable(pset.vars, "PORT", NULL); diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index ad8ae163699..357a67b47aa 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -202,10 +202,6 @@ static const internalPQconninfoOption PQconninfoOptions[] = { "Database-Service", "", 20, offsetof(struct pg_conn, pgservice)}, - {"servicefile", "PGSERVICEFILE", NULL, NULL, - "Database-Service-File", "", 64, - offsetof(struct pg_conn, pgservicefile)}, - {"user", "PGUSER", NULL, NULL, "Database-User", "", 20, offsetof(struct pg_conn, pguser)}, @@ -5081,7 +5077,6 @@ freePGconn(PGconn *conn) free(conn->dbName); free(conn->replication); free(conn->pgservice); - free(conn->pgservicefile); free(conn->pguser); if (conn->pgpass) { @@ -5959,7 +5954,6 @@ static int parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage) { const char *service = conninfo_getval(options, "service"); - const char *service_fname = conninfo_getval(options, "servicefile"); char serviceFile[MAXPGPATH]; char *env; bool group_found = false; @@ -5979,13 +5973,10 @@ parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage) return 0; /* - * First, try the "servicefile" option in connection string. Then, try - * the PGSERVICEFILE environment variable. Finally, check - * ~/.pg_service.conf (if that exists). + * Try PGSERVICEFILE if specified, else try ~/.pg_service.conf (if that + * exists). */ - if (service_fname != NULL) - strlcpy(serviceFile, service_fname, sizeof(serviceFile)); - else if ((env = getenv("PGSERVICEFILE")) != NULL) + if ((env = getenv("PGSERVICEFILE")) != NULL) strlcpy(serviceFile, env, sizeof(serviceFile)); else { @@ -6017,18 +6008,6 @@ next_file: if (status != 0) return status; - /* Update servicefile to the file that actually supplied the service */ - if (group_found && service_fname != NULL && - conninfo_storeval(options, "servicefile", serviceFile, - errorMessage, false, false) == NULL) - { - /* - * conninfo_storeval already set an error message, that could be only - * an OOM. - */ - return 3; - } - last_file: if (!group_found) { @@ -6153,17 +6132,7 @@ parseServiceFile(const char *serviceFile, if (strcmp(key, "service") == 0) { libpq_append_error(errorMessage, - "nested \"service\" specifications not supported in service file \"%s\", line %d", - serviceFile, - linenr); - result = 3; - goto exit; - } - - if (strcmp(key, "servicefile") == 0) - { - libpq_append_error(errorMessage, - "nested \"servicefile\" specifications not supported in service file \"%s\", line %d", + "nested service specifications not supported in service file \"%s\", line %d", serviceFile, linenr); result = 3; @@ -6206,33 +6175,6 @@ parseServiceFile(const char *serviceFile, } exit: - - /* - * If a service has been successfully found, set the "servicefile" option - * if not already set. This matters when we use a default service file or - * PGSERVICEFILE, where we want to be able track the value. - */ - if (*group_found && result == 0) - { - for (i = 0; options[i].keyword; i++) - { - if (strcmp(options[i].keyword, "servicefile") != 0) - continue; - - /* If value is already set, nothing to do */ - if (options[i].val != NULL) - break; - - options[i].val = strdup(serviceFile); - if (options[i].val == NULL) - { - libpq_append_error(errorMessage, "out of memory"); - result = 3; - } - break; - } - } - fclose(f); return result; diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 61b9d486d29..750dfab2d40 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -392,8 +392,6 @@ struct pg_conn char *dbName; /* database name */ char *replication; /* connect as the replication standby? */ char *pgservice; /* Postgres service, if any */ - char *pgservicefile; /* path to a service file containing - * service(s) */ char *pguser; /* Postgres username and password, if any */ char *pgpass; char *pgpassfile; /* path to a file containing password(s) */ diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl index 8e29880731f..be8a9ad0531 100644 --- a/src/interfaces/libpq/t/006_service.pl +++ b/src/interfaces/libpq/t/006_service.pl @@ -49,12 +49,6 @@ copy($srvfile_valid, $srvfile_nested) or die "Could not copy $srvfile_valid to $srvfile_nested: $!"; append_to_file($srvfile_nested, "service=invalid_srv\n"); -# Service file with nested "servicefile" defined. -my $srvfile_nested_2 = "$td/pg_service_nested_2.conf"; -copy($srvfile_valid, $srvfile_nested_2) - or die "Could not copy $srvfile_valid to $srvfile_nested_2: $!"; -append_to_file($srvfile_nested_2, 'servicefile=' . $srvfile_default . "\n"); - # Set the fallback directory lookup of the service file to the temporary # directory of this test. PGSYSCONFDIR is used if the service file # defined in PGSERVICEFILE cannot be found, or when a service file is @@ -143,14 +137,6 @@ local $ENV{PGSERVICEFILE} = "$srvfile_empty"; sql => "SELECT 'connect2_3'", expected_stdout => qr/connect2_3/); - my $srvfile_empty_win_cared = $srvfile_empty; - $srvfile_empty_win_cared =~ s/\\/\\\\/g; - $dummy_node->connect_ok( - q{service=my_srv servicefile='} . $srvfile_empty_win_cared . q{'}, - 'SERVICEFILE updated when service is found in default pg_service.conf', - sql => '\echo :SERVICEFILE', - expected_stdout => qr/^\Q$srvfile_default\E$/); - local $ENV{PGSERVICE} = 'undefined-service'; $dummy_node->connect_fails( '', @@ -168,77 +154,9 @@ local $ENV{PGSERVICEFILE} = "$srvfile_empty"; $dummy_node->connect_fails( 'service=my_srv', - 'connection with "service" in nested service file', + 'connection with nested service file', expected_stderr => - qr/nested "service" specifications not supported in service file/); - - local $ENV{PGSERVICEFILE} = $srvfile_nested_2; - - $dummy_node->connect_fails( - 'service=my_srv', - 'connection with "servicefile" in nested service file', - expected_stderr => - qr/nested "servicefile" specifications not supported in service file/ - ); -} - -# Properly escape backslashes in the path, to ensure the generation of -# correct connection strings. -my $srvfile_win_cared = $srvfile_valid; -$srvfile_win_cared =~ s/\\/\\\\/g; - -# Checks that the "servicefile" option works as expected -{ - $dummy_node->connect_ok( - q{service=my_srv servicefile='} . $srvfile_win_cared . q{'}, - 'connection with valid servicefile in connection string', - sql => "SELECT 'connect3_1'", - expected_stdout => qr/connect3_1/); - - # Encode slashes and backslash - my $encoded_srvfile = $srvfile_valid =~ s{([\\/])}{ - $1 eq '/' ? '%2F' : '%5C' - }ger; - - # Additionally encode a colon in servicefile path of Windows - $encoded_srvfile =~ s/:/%3A/g; - - $dummy_node->connect_ok( - 'postgresql:///?service=my_srv&servicefile=' . $encoded_srvfile, - 'connection with valid servicefile in URI', - sql => "SELECT 'connect3_2'", - expected_stdout => qr/connect3_2/); - - local $ENV{PGSERVICE} = 'my_srv'; - $dummy_node->connect_ok( - q{servicefile='} . $srvfile_win_cared . q{'}, - 'connection with PGSERVICE and servicefile in connection string', - sql => "SELECT 'connect3_3'", - expected_stdout => qr/connect3_3/); - - $dummy_node->connect_ok( - 'postgresql://?servicefile=' . $encoded_srvfile, - 'connection with PGSERVICE and servicefile in URI', - sql => "SELECT 'connect3_4'", - expected_stdout => qr/connect3_4/); -} - -# Check that the "servicefile" option takes priority over the PGSERVICEFILE -# environment variable. -{ - local $ENV{PGSERVICEFILE} = 'non-existent-file.conf'; - - $dummy_node->connect_fails( - 'service=my_srv', - 'connection with invalid PGSERVICEFILE', - expected_stderr => - qr/service file "non-existent-file\.conf" not found/); - - $dummy_node->connect_ok( - q{service=my_srv servicefile='} . $srvfile_win_cared . q{'}, - 'connection with both servicefile and PGSERVICEFILE', - sql => "SELECT 'connect4_1'", - expected_stdout => qr/connect4_1/); + qr/nested service specifications not supported in service file/); } $node->teardown_node;