On Wed, Oct 15, 2025 at 12:20 PM Peter Eisentraut <[email protected]> wrote: > > - backwards and forwards compatibility (we don't ever break old > > libpqs, but new libpqs can add new options safely) > > It might be worth elaborating exactly how this would be solved. If I > look through my dotfiles history, this kind of thing has been a > perennial problem. I don't have any specific ideas right now -- other > than perhaps "ignore unknown parameters", which is surely not without > problems. Depending on what we'd settle on here, that might inform > whether it's feasible to stick this into pg_service.conf or whether it > needs to be a separate thing.
Good timing. Here's a patchset that experiments with putting it all into pg_service.conf. = Roadmap = Patches 0001-0003 are small tweaks to make my life easier. I can pull them out separately if there's interest. 0004 implements a defaults section: [my-default-section] +=defaults sslmode=verify-full gssencmode=disable [other-service] ... 0005 implements forwards compatibility by marking specific defaults as ignorable: [default] +=defaults require_auth=scram-sha-256 channel_binding=require ?incredible_scram_feature=require 0006 implements PGNODEFAULTS to allow our test suites (and anything else) to turn off the new handling. This prevents a broken ~/.pg_service.conf from interfering with our tests. (A different way of solving that could be to point PGSERVICE to a blank file. I kind of liked the "turn it off" switch, though.) = Thoughts = I wanted to keep INI compatibility. I found a few clients that run pg_service.conf through a generic INI parser, which seems like an entirely reasonable thing to do. Going back to my earlier argument against environment variables, if we make people use this tool to get themselves out of a compatibility problem we introduce, and it then causes other existing parts of the Postgres ecosystem to break, I wouldn't feel great about that. (I could see an argument that breaking those clients would make it obvious that they can't apply the new defaults section correctly. But our old libpqs won't be able to apply the defaults section either, and we're presumably not going to accept breaking old libpqs.) I wanted to avoid stomping on existing service names. I could have gone the Windows route and generated a GUID or something, but instead I've allowed the user to choose any name they want for this section. They then mark it with the (maybe-too-cute?) magic string of "+=defaults", which 1) will cause earlier libpqs to fail if they accidentally try to select the section as a service, 2) is INI parseable (the key is "+"), and 3) kind of suggests what the section is meant to do: add these settings to the defaults. I've tried to avoid too much unbearable confusion by requiring that a defaults service be at the top of the file and have its marker first in the section. One subtlety that I hadn't considered was how the user and system files interact with one another. I want user defaults to override system defaults, if they are in conflict. But user services completely replace system services of the same name, so the code needs to keep the two behaviors separate. An emergent feature popped out of this while I was implementing the tests. You can now choose a default service, and the effect is that any settings listed there take precedence over the envvars. "Superdefaults." This is fragile, though -- setting a different service gets rid of those rather than merging them -- and I was idly wondering if that was something that could/should be made into its own first-class concept. The ability to ignore specific options was inspired by the ability of an ssh_config to IgnoreUnknown. Maybe you don't care if a nice-to-have option is ignored by older libpqs, but you maybe want to fail immediately if some security-critical option can't be honored (or if you just made a typo), and I think we should assume the latter. This feature would let you mark them accordingly. I'm not sure if all this is better than an architecture where the defaults and services are contained in different files. Since the syntax and behavior of the two types of sections is explicitly different, maybe combining them would be unnecessarily confusing for users? --Jacob
From 49691072e220f240f392d9bc6863469f846a09b7 Mon Sep 17 00:00:00 2001 From: Jacob Champion <[email protected]> Date: Tue, 7 Oct 2025 10:54:29 -0700 Subject: [PATCH 1/6] libpq: Simplify newline handling in t/006_service CRLF translation will already be handled by text mode; we don't need to add our own. --- src/interfaces/libpq/t/006_service.pl | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl index 797e6232b8f..432a7290c65 100644 --- a/src/interfaces/libpq/t/006_service.pl +++ b/src/interfaces/libpq/t/006_service.pl @@ -22,18 +22,14 @@ $dummy_node->init; my $td = PostgreSQL::Test::Utils::tempdir; -# Windows vs non-Windows: CRLF vs LF for the file's newline, relying on -# the fact that libpq uses fgets() when reading the lines of a service file. -my $newline = $windows_os ? "\r\n" : "\n"; - # Create the set of service files used in the tests. # File that includes a valid service name, and uses a decomposed connection # string for its contents, split on spaces. my $srvfile_valid = "$td/pg_service_valid.conf"; -append_to_file($srvfile_valid, "[my_srv]" . $newline); +append_to_file($srvfile_valid, "[my_srv]\n"); foreach my $param (split(/\s+/, $node->connstr)) { - append_to_file($srvfile_valid, $param . $newline); + append_to_file($srvfile_valid, $param . "\n"); } # File defined with no contents, used as default value for PGSERVICEFILE, @@ -51,14 +47,13 @@ my $srvfile_missing = "$td/pg_service_missing.conf"; my $srvfile_nested = "$td/pg_service_nested.conf"; copy($srvfile_valid, $srvfile_nested) or die "Could not copy $srvfile_valid to $srvfile_nested: $!"; -append_to_file($srvfile_nested, 'service=invalid_srv' . $newline); +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 . $newline); +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 -- 2.34.1
From 0132a4e8fddad82b0f4876f4ea42b7de142356ba Mon Sep 17 00:00:00 2001 From: Jacob Champion <[email protected]> Date: Tue, 7 Oct 2025 15:19:58 -0700 Subject: [PATCH 2/6] libpq: Pin sectionless-keyword behavior in pg_service.conf We've always ignored lines before the first named section of pg_service.conf. Pin that behavior in the tests so that it doesn't regress by accident. --- src/interfaces/libpq/t/006_service.pl | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl index 432a7290c65..9c692739511 100644 --- a/src/interfaces/libpq/t/006_service.pl +++ b/src/interfaces/libpq/t/006_service.pl @@ -26,7 +26,15 @@ my $td = PostgreSQL::Test::Utils::tempdir; # File that includes a valid service name, and uses a decomposed connection # string for its contents, split on spaces. my $srvfile_valid = "$td/pg_service_valid.conf"; -append_to_file($srvfile_valid, "[my_srv]\n"); +append_to_file( + $srvfile_valid, qq{ +# Settings without a section are, historically, ignored. +host=256.256.256.256 +port=1 +unknown-setting=1 + +[my_srv] +}); foreach my $param (split(/\s+/, $node->connstr)) { append_to_file($srvfile_valid, $param . "\n"); -- 2.34.1
From 20276a07bc1b7befa8c594ea2751f6497f18b70a Mon Sep 17 00:00:00 2001 From: Jacob Champion <[email protected]> Date: Tue, 7 Oct 2025 15:07:51 -0700 Subject: [PATCH 3/6] libpq: Improve unknown-keyword message for pg_service.conf Being told there's a syntax error in a file is a bit confusing if the real problem is that the option name wasn't recognized. --- src/interfaces/libpq/fe-connect.c | 3 ++- src/interfaces/libpq/t/006_service.pl | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index a3d12931fff..204f15787c9 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -6153,7 +6153,8 @@ parseServiceFile(const char *serviceFile, if (!found_keyword) { libpq_append_error(errorMessage, - "syntax error in service file \"%s\", line %d", + "unknown keyword \"%s\" in service file \"%s\", line %d", + key, serviceFile, linenr); result = 3; diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl index 9c692739511..6c1e7ec34ec 100644 --- a/src/interfaces/libpq/t/006_service.pl +++ b/src/interfaces/libpq/t/006_service.pl @@ -57,6 +57,14 @@ copy($srvfile_valid, $srvfile_nested) or die "Could not copy $srvfile_valid to $srvfile_nested: $!"; append_to_file($srvfile_nested, "service=invalid_srv\n"); +# File with an unknown setting. +my $srvfile_bad_keyword = "$td/pg_service_bad_keyword.conf"; +append_to_file( + $srvfile_bad_keyword, qq{ +[my_srv] +bad-unknown-setting=1 +}); + # Service file with nested "servicefile" defined. my $srvfile_nested_2 = "$td/pg_service_nested_2.conf"; copy($srvfile_valid, $srvfile_nested_2) @@ -182,6 +190,17 @@ local $ENV{PGSERVICEFILE} = "$srvfile_empty"; ); } +# Check behavior with unknown keywords. +{ + local $ENV{PGSERVICEFILE} = $srvfile_bad_keyword; + + $dummy_node->connect_fails( + 'service=my_srv', + 'connection with unknown connection option in service', + expected_stderr => + qr/unknown keyword "bad-unknown-setting" in service file/); +} + # Properly escape backslashes in the path, to ensure the generation of # correct connection strings. my $srvfile_win_cared = $srvfile_valid; -- 2.34.1
From 7f18db5f8cf1156a760dba948efc62838ac5d19c Mon Sep 17 00:00:00 2001 From: Jacob Champion <[email protected]> Date: Tue, 7 Oct 2025 16:15:48 -0700 Subject: [PATCH 4/6] WIP: pg_service: implement defaults section Default connection options may now be overridden by creating a section at the top of a pg_service.conf file with the following header: [any-arbitrary-section-name] +=defaults host=... System-level defaults (from the service file residing in PGSYSCONFDIR) and user-level defaults (from the PGSERVICEFILE) are merged, with the user-level options taking precedence. With this change, connection options fall back in order of decreasing precedence: 1) Connection string 2) User service 2b) System service, but only if no user service was found 3) Environment variable 4) User default 5) System default 6) Compile-time default Some code complexity here is due to the fact that the service name in use may depend on the defaults. The new tests make use of this to ensure that Test::Cluster environment variables are overridden as needed. --- src/interfaces/libpq/fe-connect.c | 251 +++++++++++++++++++++----- src/interfaces/libpq/t/006_service.pl | 93 ++++++++++ 2 files changed, 303 insertions(+), 41 deletions(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 204f15787c9..4fce5f393e1 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -493,7 +493,8 @@ static PQconninfoOption *conninfo_find(PQconninfoOption *connOptions, const char *keyword); static void defaultNoticeReceiver(void *arg, const PGresult *res); static void defaultNoticeProcessor(void *arg, const char *message); -static int parseServiceInfo(PQconninfoOption *options, +static int parseServiceInfo(PQconninfoOption *defaults, + PQconninfoOption *options, PQExpBuffer errorMessage); static int parseServiceFile(const char *serviceFile, const char *service, @@ -5916,7 +5917,8 @@ ldapServiceLookup(const char *purl, PQconninfoOption *options, #endif /* USE_LDAP */ /* - * parseServiceInfo: if a service name has been given, look it up and absorb + * parseServiceInfo: Parse any dynamic defaults from the service files. + * Additionally, if a service name has been given, look it up and absorb * connection options from it into *options. * * Returns 0 on success, nonzero on failure. On failure, if errorMessage @@ -5926,11 +5928,15 @@ ldapServiceLookup(const char *purl, PQconninfoOption *options, * a null PQExpBuffer pointer.) */ static int -parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage) +parseServiceInfo(PQconninfoOption *defaults, PQconninfoOption *options, + PQExpBuffer errorMessage) { const char *service = conninfo_getval(options, "service"); const char *service_fname = conninfo_getval(options, "servicefile"); - char serviceFile[MAXPGPATH]; + char userServiceFile[MAXPGPATH]; + char systemServiceFile[MAXPGPATH]; + bool have_user_file = false; + bool have_system_file = false; char *env; bool group_found = false; int status; @@ -5939,64 +5945,114 @@ parseServiceInfo(PQconninfoOption *options, PQExpBuffer errorMessage) /* * We have to special-case the environment variable PGSERVICE here, since * this is and should be called before inserting environment defaults for - * other connection options. + * other connection options, and it takes precedence over any default + * service defined in the files. */ if (service == NULL) service = getenv("PGSERVICE"); - /* If no service name given, nothing to do */ - if (service == NULL) - return 0; - /* * First, try the "servicefile" option in connection string. Then, try * the PGSERVICEFILE environment variable. Finally, check * ~/.pg_service.conf (if that exists). */ if (service_fname != NULL) - strlcpy(serviceFile, service_fname, sizeof(serviceFile)); + strlcpy(userServiceFile, service_fname, sizeof(userServiceFile)); else if ((env = getenv("PGSERVICEFILE")) != NULL) - strlcpy(serviceFile, env, sizeof(serviceFile)); + strlcpy(userServiceFile, env, sizeof(userServiceFile)); else { char homedir[MAXPGPATH]; if (!pqGetHomeDirectory(homedir, sizeof(homedir))) goto next_file; - snprintf(serviceFile, MAXPGPATH, "%s/%s", homedir, ".pg_service.conf"); - if (stat(serviceFile, &stat_buf) != 0) + snprintf(userServiceFile, MAXPGPATH, "%s/%s", homedir, ".pg_service.conf"); + if (stat(userServiceFile, &stat_buf) != 0) goto next_file; } - status = parseServiceFile(serviceFile, service, options, errorMessage, &group_found); - if (group_found || status != 0) + /* + * Pull defaults out of the user file first, if one exists. They take + * precedence over any defaults in the system file. + */ + status = parseServiceFile(userServiceFile, NULL, defaults, errorMessage, &group_found); + if (status != 0) return status; + have_user_file = true; + next_file: /* * This could be used by any application so we can't use the binary * location to find our config files. */ - snprintf(serviceFile, MAXPGPATH, "%s/pg_service.conf", + snprintf(systemServiceFile, MAXPGPATH, "%s/pg_service.conf", getenv("PGSYSCONFDIR") ? getenv("PGSYSCONFDIR") : SYSCONFDIR); - if (stat(serviceFile, &stat_buf) != 0) + if (stat(systemServiceFile, &stat_buf) != 0) goto last_file; - status = parseServiceFile(serviceFile, service, options, errorMessage, &group_found); + /* Fill in system defaults for any options not given in the user file. */ + status = parseServiceFile(systemServiceFile, NULL, defaults, errorMessage, &group_found); if (status != 0) return status; + have_system_file = true; + last_file: - if (!group_found) + + /* + * At this point, we've filled in any dynamic defaults. We can make one + * last attempt at a service name if none was given so far. + */ + if (service == NULL) { - libpq_append_error(errorMessage, "definition of service \"%s\" not found", service); - return 3; + service = conninfo_getval(defaults, "service"); + if (service == NULL) + return 0; /* nothing left to do */ } - return 0; + /* + * We have a service name. Try to find it first in the user file, then in + * the system file. + * + * Note the difference when compared to defaults! The default sections in + * the user and system files are *merged*, with the user file taking + * precedence. But if a named user service is found, any identically named + * system service is *ignored*. + */ + if (have_user_file) + { + status = parseServiceFile(userServiceFile, service, options, errorMessage, &group_found); + if (group_found || status != 0) + return status; + } + + if (have_system_file) + { + status = parseServiceFile(systemServiceFile, service, options, errorMessage, &group_found); + if (group_found || status != 0) + return status; + } + + libpq_append_error(errorMessage, "definition of service \"%s\" not found", service); + return 3; } +/* + * Fills in option fallback values from a service file. + * + * When service is not NULL: The file is searched for the named service section. + * If one is found, we make sure it's not marked as the default section (which + * is an error) and then parse it. + * + * When service is NULL: The default section is parsed if one exists at the + * start of the file. Later sections are also checked to ensure that they are + * not incorrectly marked as defaults. + * + * Already-set options are never overwritten. *group_found is set to true if a + * matching section was found, and false otherwise. + */ static int parseServiceFile(const char *serviceFile, const char *service, @@ -6010,6 +6066,8 @@ parseServiceFile(const char *serviceFile, FILE *f; char *line; char buf[1024]; + unsigned int section_num = 0; + unsigned int option_num = 0; *group_found = false; @@ -6052,13 +6110,17 @@ parseServiceFile(const char *serviceFile, /* Check for right groupname */ if (line[0] == '[') { - if (*group_found) + if (service != NULL && *group_found) { /* end of desired group reached; return success */ goto exit; } - if (strncmp(line + 1, service, strlen(service)) == 0 && + section_num++; + option_num = 0; + + if (service != NULL && + strncmp(line + 1, service, strlen(service)) == 0 && line[strlen(service) + 1] == ']') *group_found = true; else @@ -6066,6 +6128,8 @@ parseServiceFile(const char *serviceFile, } else { + option_num++; + if (*group_found) { /* @@ -6076,7 +6140,12 @@ parseServiceFile(const char *serviceFile, bool found_keyword; #ifdef USE_LDAP - if (strncmp(line, "ldap", 4) == 0) + + /* + * LDAP lookup is allowed only in service definitions, not the + * defaults section. + */ + if (service != NULL && strncmp(line, "ldap", 4) == 0) { int rc = ldapServiceLookup(line, options, errorMessage); @@ -6108,7 +6177,11 @@ parseServiceFile(const char *serviceFile, } *val++ = '\0'; - if (strcmp(key, "service") == 0) + /* + * A default service setting may be specified, but they're not + * allowed to be nested inside other services. + */ + if (service != NULL && strcmp(key, "service") == 0) { libpq_append_error(errorMessage, "nested \"service\" specifications not supported in service file \"%s\", line %d", @@ -6118,6 +6191,7 @@ parseServiceFile(const char *serviceFile, goto exit; } + /* Nested servicefile settings are never allowed. */ if (strcmp(key, "servicefile") == 0) { libpq_append_error(errorMessage, @@ -6151,15 +6225,64 @@ parseServiceFile(const char *serviceFile, } if (!found_keyword) + { + /* + * "unknown keyword" is unhelpful, if the actual problem + * is that the user has tried to select the default + * section. + */ + if (service != NULL + && strcmp(key, "+") == 0 + && strcmp(val, "defaults") == 0) + libpq_append_error(errorMessage, + "default section [%s] may not be named as a service in file \"%s\", line %d", + service, + serviceFile, + linenr); + else + libpq_append_error(errorMessage, + "unknown keyword \"%s\" in service file \"%s\", line %d", + key, + serviceFile, + linenr); + result = 3; + goto exit; + } + } + else if (section_num > 0 && option_num == 1) + { + /* + * Check for the default marker. We allow only the first + * section in the file to be the default section, and it must + * contain the marker "+=defaults" as its first key/value + * pair. + * + * We don't currently allow whitespace around keys and values, + * so we can perform a single strcmp(). + */ + if (strcmp(line, "+=defaults") != 0) + continue; + + /* + * Avoid user confusion and complain if the default marker is + * anywhere other than the first section. + */ + if (section_num != 1) { libpq_append_error(errorMessage, - "unknown keyword \"%s\" in service file \"%s\", line %d", - key, + "only the first section may be marked default in service file \"%s\", line %d", serviceFile, linenr); result = 3; goto exit; } + + /* + * If we're not looking for a service, parse this defaults + * section. + */ + if (service == NULL) + *group_found = true; } } } @@ -6171,7 +6294,7 @@ exit: * 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) + if (service != NULL && *group_found && result == 0) { for (i = 0; options[i].keyword; i++) { @@ -6666,23 +6789,40 @@ static bool conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage) { PQconninfoOption *option; + PQconninfoOption *defaults; + PQconninfoOption *defopt; + PQExpBufferData unused = {0}; PQconninfoOption *sslmode_default = NULL, *sslrootcert = NULL; char *tmp; + bool success = false; /* - * If there's a service spec, use it to obtain any not-explicitly-given - * parameters. Ignore error if no error message buffer is passed because - * there is no way to pass back the failure message. + * Create a parallel set of options to hold dynamic defaults. + * Unfortunately this requires constructing a throwaway error buffer if + * the caller didn't provide one. */ - if (parseServiceInfo(options, errorMessage) != 0 && errorMessage) + if (errorMessage == NULL) + termPQExpBuffer(&unused); /* make buffer operations no-ops */ + + defaults = conninfo_init(errorMessage ? errorMessage : &unused); + if (defaults == NULL) return false; + /* + * Fill in any dynamic defaults, and if there's a service spec, use it to + * obtain any not-explicitly-given parameters. Ignore error if no error + * message buffer is passed because there is no way to pass back the + * failure message. + */ + if (parseServiceInfo(defaults, options, errorMessage) != 0 && errorMessage) + goto cleanup; + /* * Get the fallback resources for parameters not specified in the conninfo * string nor the service. */ - for (option = options; option->keyword != NULL; option++) + for (option = options, defopt = defaults; option->keyword != NULL; option++, defopt++) { if (strcmp(option->keyword, "sslrootcert") == 0) sslrootcert = option; /* save for later */ @@ -6702,7 +6842,7 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage) { if (errorMessage) libpq_append_error(errorMessage, "out of memory"); - return false; + goto cleanup; } continue; } @@ -6725,7 +6865,7 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage) { if (errorMessage) libpq_append_error(errorMessage, "out of memory"); - return false; + goto cleanup; } continue; } @@ -6739,8 +6879,33 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage) } /* - * No environment variable specified or the variable isn't set - try - * compiled-in default + * No environment variable specified or the variable isn't set. First + * try a dynamic default from pg_service. + * + * We iterate over the defaults array in lockstep to avoid a lookup + * here, so check that the option order has remained static. + */ + if (option->keyword != defopt->keyword) + { + if (errorMessage) + libpq_append_error(errorMessage, + "defaults array is out of order"); + goto cleanup; + } + else if (defopt->val != NULL) + { + option->val = strdup(defopt->val); + if (!option->val) + { + if (errorMessage) + libpq_append_error(errorMessage, "out of memory"); + goto cleanup; + } + continue; + } + + /* + * Fall back to the compiled-in default. */ if (option->compiled != NULL) { @@ -6749,7 +6914,7 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage) { if (errorMessage) libpq_append_error(errorMessage, "out of memory"); - return false; + goto cleanup; } continue; } @@ -6784,12 +6949,16 @@ conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage) { if (errorMessage) libpq_append_error(errorMessage, "out of memory"); - return false; + goto cleanup; } } } - return true; + success = true; + +cleanup: + PQconninfoFree(defaults); + return success; } /* diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl index 6c1e7ec34ec..0826add30fd 100644 --- a/src/interfaces/libpq/t/006_service.pl +++ b/src/interfaces/libpq/t/006_service.pl @@ -118,6 +118,99 @@ local $ENV{PGSERVICEFILE} = "$srvfile_empty"; qr/definition of service "undefined-service" not found/); } +{ + # Check handling of the defaults section. + # + # pg_service_defaults.conf contains the same parameters as srvfile_valid, + # but it uses a default section to select the service automatically. (Use of + # a service remains necessary, to take precedence over Test::Cluster's + # automatic envvars.) + my $srvfile_defaults = "$td/pg_service_defaults.conf"; + append_to_file( + $srvfile_defaults, qq{ +# Settings before the default section must be ignored. +host=256.256.256.256 +port=1 +unknown-setting=1 + +[default] ++=defaults +service=my_srv +options=-O + +[my_srv] +}); + foreach my $param (split(/\s+/, $node->connstr)) + { + append_to_file($srvfile_defaults, $param . "\n"); + } + + local $ENV{PGSERVICEFILE} = $srvfile_defaults; + $dummy_node->connect_ok( + '', + 'connection with dynamic defaults in PGSERVICEFILE', + sql => 'SHOW allow_system_table_mods', + expected_stdout => qr/on/); + + # TODO is it really okay that postgres:// means whatever the environment + # says it means...? + $dummy_node->connect_ok( + 'postgres://', + 'connection with empty URI, dynamic defaults, and PGSERVICEFILE', + sql => 'SHOW allow_system_table_mods', + expected_stdout => qr/on/); + + $dummy_node->connect_fails( + 'service=default', + 'default sections may not be selected via connection parameter', + expected_stderr => + qr/default section \[default\] may not be named as a service/); + + { + local $ENV{PGSERVICE} = 'default'; + $dummy_node->connect_fails( + '', + 'default sections may not be selected via PGSERVICE', + expected_stderr => + qr/default section \[default\] may not be named as a service/); + } + + # A file containing more than one default section is rejected. + my $srvfile_too_many_defaults = "$td/pg_service_too_many_defaults.conf"; + copy($srvfile_defaults, $srvfile_too_many_defaults) + or die + "Could not copy $srvfile_defaults to $srvfile_too_many_defaults: $!"; + append_to_file( + $srvfile_too_many_defaults, qq{ +[default] ++=defaults +}); + + local $ENV{PGSERVICEFILE} = $srvfile_too_many_defaults; + $dummy_node->connect_fails( + '', + 'service files may not contain more than one default section', + expected_stderr => qr/only the first section may be marked default/); + + # A default section may not come after the first service section. + my $srvfile_defaults_after_service = + "$td/pg_service_defaults_after_service.conf"; + copy($srvfile_valid, $srvfile_defaults_after_service) + or die + "Could not copy $srvfile_valid to $srvfile_defaults_after_service: $!"; + append_to_file( + $srvfile_defaults_after_service, qq{ +[default] ++=defaults +}); + + local $ENV{PGSERVICEFILE} = $srvfile_defaults_after_service; + $dummy_node->connect_fails( + '', + 'defaults section must be first in the file', + expected_stderr => qr/only the first section may be marked default/); +} + # Checks case of incorrect service file. { local $ENV{PGSERVICEFILE} = $srvfile_missing; -- 2.34.1
From 3b3b5e9b7ad709a03c5b704ed74b9bfcf007380c Mon Sep 17 00:00:00 2001 From: Jacob Champion <[email protected]> Date: Fri, 10 Oct 2025 11:53:49 -0700 Subject: [PATCH 5/6] WIP: pg_service: implement forwards compatibility To allow defaults from multiple major versions of libpq to coexist in the PGSERVICEFILE, add the ability to ignore unknown keywords in the defaults section by prefixing them with '?': [my-defaults-section] +=defaults ?amazing_pg30_feature=on sslrootcert=system sslmode=verify-full ... --- src/interfaces/libpq/fe-connect.c | 13 ++++++++++++- src/interfaces/libpq/t/006_service.pl | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 4fce5f393e1..4fbaf4727ef 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -6138,6 +6138,7 @@ parseServiceFile(const char *serviceFile, char *key, *val; bool found_keyword; + bool skip_unknown = false; #ifdef USE_LDAP @@ -6177,6 +6178,16 @@ parseServiceFile(const char *serviceFile, } *val++ = '\0'; + /* + * Inside a defaults section, unknown options may be marked as + * skippable by the user for forwards compatibility purposes. + */ + if (service == NULL && key[0] == '?') + { + skip_unknown = true; + key++; + } + /* * A default service setting may be specified, but they're not * allowed to be nested inside other services. @@ -6224,7 +6235,7 @@ parseServiceFile(const char *serviceFile, } } - if (!found_keyword) + if (!found_keyword && !skip_unknown) { /* * "unknown keyword" is unhelpful, if the actual problem diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl index 0826add30fd..973035aac9b 100644 --- a/src/interfaces/libpq/t/006_service.pl +++ b/src/interfaces/libpq/t/006_service.pl @@ -137,6 +137,7 @@ unknown-setting=1 +=defaults service=my_srv options=-O +?unknown-setting=1 # should be ignored [my_srv] }); -- 2.34.1
From 7ccf7d7074ce34b61835d3c74acba853c9a394d3 Mon Sep 17 00:00:00 2001 From: Jacob Champion <[email protected]> Date: Fri, 10 Oct 2025 15:56:02 -0700 Subject: [PATCH 6/6] WIP: pg_service: implement PGNODEFAULTS Tests need to ensure that they have full control of the default connection state. While PGSYSCONFDIR was already redirected as needed, the user's config file would still be consulted when PGSERVICEFILE was empty. Add a PGNODEFAULTS envvar to turn off the previous feature, and make use of it in pg_regress and Test::Cluster connections. --- src/interfaces/libpq/fe-connect.c | 33 +++++++++++++++++--------- src/interfaces/libpq/t/006_service.pl | 10 ++++++++ src/test/perl/PostgreSQL/Test/Utils.pm | 3 +++ src/test/regress/pg_regress.c | 9 +++++++ 4 files changed, 44 insertions(+), 11 deletions(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index 4fbaf4727ef..000001de05d 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -5935,6 +5935,7 @@ parseServiceInfo(PQconninfoOption *defaults, PQconninfoOption *options, const char *service_fname = conninfo_getval(options, "servicefile"); char userServiceFile[MAXPGPATH]; char systemServiceFile[MAXPGPATH]; + bool load_defaults = true; bool have_user_file = false; bool have_system_file = false; char *env; @@ -5951,6 +5952,10 @@ parseServiceInfo(PQconninfoOption *defaults, PQconninfoOption *options, if (service == NULL) service = getenv("PGSERVICE"); + /* PGNODEFAULTS=1 disables the use of defaults sections. */ + if ((env = getenv("PGNODEFAULTS")) != NULL && strcmp(env, "1") == 0) + load_defaults = false; + /* * First, try the "servicefile" option in connection string. Then, try * the PGSERVICEFILE environment variable. Finally, check @@ -5971,13 +5976,16 @@ parseServiceInfo(PQconninfoOption *defaults, PQconninfoOption *options, goto next_file; } - /* - * Pull defaults out of the user file first, if one exists. They take - * precedence over any defaults in the system file. - */ - status = parseServiceFile(userServiceFile, NULL, defaults, errorMessage, &group_found); - if (status != 0) - return status; + if (load_defaults) + { + /* + * Pull defaults out of the user file first, if one exists. They take + * precedence over any defaults in the system file. + */ + status = parseServiceFile(userServiceFile, NULL, defaults, errorMessage, &group_found); + if (status != 0) + return status; + } have_user_file = true; @@ -5992,10 +6000,13 @@ next_file: if (stat(systemServiceFile, &stat_buf) != 0) goto last_file; - /* Fill in system defaults for any options not given in the user file. */ - status = parseServiceFile(systemServiceFile, NULL, defaults, errorMessage, &group_found); - if (status != 0) - return status; + if (load_defaults) + { + /* Fill in system defaults for any options not given in the user file. */ + status = parseServiceFile(systemServiceFile, NULL, defaults, errorMessage, &group_found); + if (status != 0) + return status; + } have_system_file = true; diff --git a/src/interfaces/libpq/t/006_service.pl b/src/interfaces/libpq/t/006_service.pl index 973035aac9b..cbb0b2d1c46 100644 --- a/src/interfaces/libpq/t/006_service.pl +++ b/src/interfaces/libpq/t/006_service.pl @@ -146,6 +146,8 @@ options=-O append_to_file($srvfile_defaults, $param . "\n"); } + # Test::Utils will have disabled dynamic defaults. + local $ENV{PGNODEFAULTS} = "0"; local $ENV{PGSERVICEFILE} = $srvfile_defaults; $dummy_node->connect_ok( '', @@ -210,6 +212,14 @@ options=-O '', 'defaults section must be first in the file', expected_stderr => qr/only the first section may be marked default/); + + { + # Re-enable PGNODEFAULTS and check that we can continue using the + # working service, even though the defaults section is broken. + local $ENV{PGNODEFAULTS} = "1"; + $dummy_node->connect_ok('service=my_srv', + 'connection with bad defaults section, but PGNODEFAULTS=1'); + } } # Checks case of incorrect service file. diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm index 85d36a3171e..3f27f315a4d 100644 --- a/src/test/perl/PostgreSQL/Test/Utils.pm +++ b/src/test/perl/PostgreSQL/Test/Utils.pm @@ -111,6 +111,9 @@ BEGIN $ENV{LC_NUMERIC} = 'C'; setlocale(LC_ALL, ""); + # Disable any defaults coming from pg_service.conf. + $ENV{PGNODEFAULTS} = "1"; + # This list should be kept in sync with pg_regress.c. my @envkeys = qw ( PGCHANNELBINDING diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c index 61c035a3983..c8b424a8336 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -728,6 +728,15 @@ initialize_environment(void) */ setenv("PGAPPNAME", "pg_regress", 1); + /* + * Disable any defaults coming from pg_service.conf, which would thwart our + * unsetenv()s below. + * + * TODO this would need to be documented for installcheck: only environment + * variables can be used to point to the system under test + */ + setenv("PGNODEFAULTS", "1", 1); + /* * Set variables that the test scripts may need to refer to. */ -- 2.34.1
