Обсуждение: libpq 9.4 requires /etc/passwd?
Hi,
I've got several reports that postfix's pgsql lookup tables are broken
with 9.4's libpq5, while 9.3's libpq5 works just fine. The error
message looks like this:
Jan 10 00:11:40 lehmann postfix/trivial-rewrite[29960]: warning: connect to pgsql server localhost:5432: out of
memory?
Jan 10 00:11:40 lehmann postfix/trivial-rewrite[29960]: warning: pgsql:/etc/postfix/pgsqltest lookup error for "*"
The "out of memory?" message comes from PQsetdbLogin and
PQerrorMessage in postfix-2.11.3/src/global/dict_pgsql.c:
if ((host->db = PQsetdbLogin(host->name, host->port, NULL, NULL, dbname, username,
password))== NULL || PQstatus(host->db) != CONNECTION_OK) { msg_warn("connect to pgsql server %s: %s",
host->hostname, PQerrorMessage(host->db));
There are two ways around the problem on the postfix side: not running
the postfix service chrooted, or using a "proxy" map which effectively
does the lookup also from outside the chroot.
Debian bug https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=756627
mentions another solution: creation of an /etc/passwd file inside the
postfix chroot.
libpq wants the user home directory to find .pgpass and
.pg_service.conf files, but apparently the behavior to require the
existence of the passwd file (or nss equivalent) is new in 9.4.
I've tried to make sense of the fe-connect.c code to find the issue
but couldn't spot it. Can someone explain what's going on there? Is
this a bug in libpq? (It's certainly a regression.)
Christoph
--
cb@df7cb.de | http://www.df7cb.de/
Christoph Berg <cb@df7cb.de> writes:
> libpq wants the user home directory to find .pgpass and
> .pg_service.conf files, but apparently the behavior to require the
> existence of the passwd file (or nss equivalent) is new in 9.4.
There is demonstrably no direct reference to /etc/passwd in the PG code.
It's possible that we've added some new expectation that $HOME can be
identified, but a quick look through the code shows no such checks that
don't look like they've been there for some time.
Are the complainants doing anything that would result in SSL certificate
checking? More generally, it'd be useful to see an exact example of
what are the connection parameters and environment that result in a
failure.
regards, tom lane
I wrote:
> Christoph Berg <cb@df7cb.de> writes:
>> libpq wants the user home directory to find .pgpass and
>> .pg_service.conf files, but apparently the behavior to require the
>> existence of the passwd file (or nss equivalent) is new in 9.4.
> There is demonstrably no direct reference to /etc/passwd in the PG code.
> It's possible that we've added some new expectation that $HOME can be
> identified, but a quick look through the code shows no such checks that
> don't look like they've been there for some time.
Hmm ... actually, I'll bet it's not $HOME that's at issue, but the name
of the user. Commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 turned
failure of pg_fe_getauthname() into a hard connection failure, whereas
previously it was harmless as long as the caller provided a username.
I wonder if we shouldn't just revert that commit in toto. Yeah,
identifying an out-of-memory error might be useful, but this cure
seems a lot worse than the disease. What's more, this coding reports
*any* pg_fe_getauthname failure as "out of memory", which is much worse
than useless.
Alternatively, maybe don't try to resolve username this way until
we've found that the caller isn't providing any username.
regards, tom lane
On Fri, Jan 9, 2015 at 06:42:19PM -0500, Tom Lane wrote:
> Christoph Berg <cb@df7cb.de> writes:
> > libpq wants the user home directory to find .pgpass and
> > .pg_service.conf files, but apparently the behavior to require the
> > existence of the passwd file (or nss equivalent) is new in 9.4.
>
> There is demonstrably no direct reference to /etc/passwd in the PG code.
> It's possible that we've added some new expectation that $HOME can be
> identified, but a quick look through the code shows no such checks that
> don't look like they've been there for some time.
>
> Are the complainants doing anything that would result in SSL certificate
> checking? More generally, it'd be useful to see an exact example of
> what are the connection parameters and environment that result in a
> failure.
I see similar error strings, but nothing that ends with a question mark:
"out of memory?". However, I wonder if the crux of the problem is that
they are running in a chroot environment where the user id can't be
looked up.
Looking at libpq's pg_fe_getauthname(), I see that if it can't get the
OS user name of the effective user, it assumes it failed and returns
NULL:
/* * We document PQconndefaults() to return NULL for a memory allocation * failure. We don't have an API
toreturn a user name lookup failure, so * we just assume it always succeeds. */#ifdef WIN32 if
(GetUserName(username,&namesize)) name = username;#else if (pqGetpwuid(geteuid(), &pwdstr, pwdbuf,
sizeof(pwdbuf),&pw) == 0) name = pw->pw_name;#endif authn = name ? strdup(name) : NULL;
and conninfo_add_defaults() assumes a pg_fe_getauthname() failure is a
memory allocation failure:
option->val = pg_fe_getauthname();if (!option->val){ if (errorMessage) printfPQExpBuffer(errorMessage,
libpq_gettext("out of memory\n")); return false;
It was my 9.4 commit that changed this:
commit a4c8f14364c27508233f8a31ac4b10a4c90235a9Author: Bruce Momjian <bruce@momjian.us>Date: Thu Mar 20 11:48:31 2014
-0400 libpq: pass a memory allocation failure error up to PQconndefaults() Previously user name memory allocation
failureswere ignored and the default user name set to NULL.
I am thinking we have to now assume that user name lookups can fail for
other reasons. I am unclear how this worked in the past though.
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ Everyone has their own god. +
On Fri, Jan 9, 2015 at 06:57:02PM -0500, Tom Lane wrote:
> I wrote:
> > Christoph Berg <cb@df7cb.de> writes:
> >> libpq wants the user home directory to find .pgpass and
> >> .pg_service.conf files, but apparently the behavior to require the
> >> existence of the passwd file (or nss equivalent) is new in 9.4.
>
> > There is demonstrably no direct reference to /etc/passwd in the PG code.
> > It's possible that we've added some new expectation that $HOME can be
> > identified, but a quick look through the code shows no such checks that
> > don't look like they've been there for some time.
>
> Hmm ... actually, I'll bet it's not $HOME that's at issue, but the name
> of the user. Commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 turned
> failure of pg_fe_getauthname() into a hard connection failure, whereas
> previously it was harmless as long as the caller provided a username.
>
> I wonder if we shouldn't just revert that commit in toto. Yeah,
> identifying an out-of-memory error might be useful, but this cure
> seems a lot worse than the disease. What's more, this coding reports
> *any* pg_fe_getauthname failure as "out of memory", which is much worse
> than useless.
>
> Alternatively, maybe don't try to resolve username this way until
> we've found that the caller isn't providing any username.
Seems we both found it at the same time. Here is the thread were we
discussed it, and you were concerned about the memory allocation failure
too:
http://www.postgresql.org/message-id/flat/20140320154905.GC7711@momjian.us#20140320154905.GC7711@momjian.us
In particular, it appears to me that if the strdup in pg_fe_getauthname
fails, we'll just let that pass without comment, which is inconsistent
with all the other out-of-memory cases in conninfo_add_defaults.
(I wonder whether any code downstream of this supposes that we always
have a value for the "user" option. It's a pretty safe bet that the
case is hardly ever tested.)
I have developed the attached patch which documents why the user name
lookup might fail, and sets the failure string to "". It preserves the
memory allocation failure behavior.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Вложения
Bruce Momjian <bruce@momjian.us> writes:
> On Fri, Jan 9, 2015 at 06:57:02PM -0500, Tom Lane wrote:
>> Hmm ... actually, I'll bet it's not $HOME that's at issue, but the name
>> of the user. Commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 turned
>> failure of pg_fe_getauthname() into a hard connection failure, whereas
>> previously it was harmless as long as the caller provided a username.
>>
>> I wonder if we shouldn't just revert that commit in toto. Yeah,
>> identifying an out-of-memory error might be useful, but this cure
>> seems a lot worse than the disease. What's more, this coding reports
>> *any* pg_fe_getauthname failure as "out of memory", which is much worse
>> than useless.
>>
>> Alternatively, maybe don't try to resolve username this way until
>> we've found that the caller isn't providing any username.
> I have developed the attached patch which documents why the user name
> lookup might fail, and sets the failure string to "". It preserves the
> memory allocation failure behavior.
I'm unimpressed with that patch. It does nothing to fix the fundamental
problem, which is that error handling in this area is many bricks shy of
a load. And to the extent that it addresses Christoph's complaint, it
does so only in a roundabout and utterly undocumented way.
I think we need to suck it up and fix pg_fe_getauthname to have proper
error reporting, which in turns means fixing pqGetpwuid (since the API
for that is incapable of distinguishing "no such user" from "error during
lookup"). Accordingly, I propose the attached patch instead. Some
notes:
* According to Microsoft's documentation, Windows' GetUserName() does not
set errno (it would be mildly astonishing if it did...). The existing
error-handling code in src/common/username.c is therefore wrong too.
* port.h seems to think it should declare pqGetpwuid() if __CYGWIN__,
but noplace else agrees with that.
* I made pg_fe_getauthname's Windows username[] array 256 bytes for
consistency with username.c, where some actual research seems to have
been expended on how large it ought to be.
* pqGetpwuid thinks there was once a four-argument spec for getpwuid_r.
That must have been in the late bronze age, because even in 1992's Single
Unix Spec it's five arguments. And the SUS is our standard benchmark
for Unix portability. So I think we could rip that out, along with the
corresponding configure test, at least in HEAD. I've not done so here
though.
In principle, changing the API specs for pg_fe_getauthname and pqGetpwuid
should be safe because we don't officially export those functions. In
practice, on platforms that don't honor the export restriction list it's
theoretically possible that somebody is calling those from third-party
code. So what I propose we do with this is patch HEAD and 9.4 only.
We need to do *something* in 9.4 to address Christoph's complaint, and
that branch is new enough that we can probably get away with changing
officially-unsupported APIs. The lack of other field complaints makes
me okay with not trying to fix these issues further back.
Comments?
regards, tom lane
diff --git a/src/common/username.c b/src/common/username.c
index ee5ef1c..ac9a898 100644
*** a/src/common/username.c
--- b/src/common/username.c
*************** get_user_name(char **errstr)
*** 58,64 ****
if (!GetUserName(username, &len))
{
! *errstr = psprintf(_("user name lookup failure: %s"), strerror(errno));
return NULL;
}
--- 58,65 ----
if (!GetUserName(username, &len))
{
! *errstr = psprintf(_("user name lookup failure: error code %lu"),
! GetLastError());
return NULL;
}
diff --git a/src/include/port.h b/src/include/port.h
index 1d53e4e..26d7fcd 100644
*** a/src/include/port.h
--- b/src/include/port.h
*************** extern void srandom(unsigned int seed);
*** 433,439 ****
/* thread.h */
extern char *pqStrerror(int errnum, char *strerrbuf, size_t buflen);
! #if !defined(WIN32) || defined(__CYGWIN__)
extern int pqGetpwuid(uid_t uid, struct passwd * resultbuf, char *buffer,
size_t buflen, struct passwd ** result);
#endif
--- 433,439 ----
/* thread.h */
extern char *pqStrerror(int errnum, char *strerrbuf, size_t buflen);
! #ifndef WIN32
extern int pqGetpwuid(uid_t uid, struct passwd * resultbuf, char *buffer,
size_t buflen, struct passwd ** result);
#endif
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
index 179793e..5a6a1e4 100644
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*************** pg_fe_sendauth(AuthRequest areq, PGconn
*** 714,735 ****
/*
* pg_fe_getauthname
*
! * Returns a pointer to dynamic space containing whatever name the user
! * has authenticated to the system. If there is an error, return NULL.
*/
char *
! pg_fe_getauthname(void)
{
const char *name = NULL;
- char *authn;
#ifdef WIN32
! char username[128];
DWORD namesize = sizeof(username) - 1;
#else
char pwdbuf[BUFSIZ];
struct passwd pwdstr;
struct passwd *pw = NULL;
#endif
/*
--- 714,738 ----
/*
* pg_fe_getauthname
*
! * Returns a pointer to malloc'd space containing whatever name the user
! * has authenticated to the system. If there is an error, return NULL,
! * and put a suitable error message in *errorMessage if that's not NULL.
*/
char *
! pg_fe_getauthname(PQExpBuffer errorMessage)
{
+ char *result = NULL;
const char *name = NULL;
#ifdef WIN32
! char username[256];
DWORD namesize = sizeof(username) - 1;
#else
+ uid_t user_id = geteuid();
char pwdbuf[BUFSIZ];
struct passwd pwdstr;
struct passwd *pw = NULL;
+ int pwerr;
#endif
/*
*************** pg_fe_getauthname(void)
*** 741,764 ****
*/
pglock_thread();
- /*
- * We document PQconndefaults() to return NULL for a memory allocation
- * failure. We don't have an API to return a user name lookup failure, so
- * we just assume it always succeeds.
- */
#ifdef WIN32
if (GetUserName(username, &namesize))
name = username;
#else
! if (pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pw) == 0)
name = pw->pw_name;
#endif
! authn = name ? strdup(name) : NULL;
pgunlock_thread();
! return authn;
}
--- 744,785 ----
*/
pglock_thread();
#ifdef WIN32
if (GetUserName(username, &namesize))
name = username;
+ else if (errorMessage)
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("user name lookup failure: error code %lu\n"),
+ GetLastError());
#else
! pwerr = pqGetpwuid(user_id, &pwdstr, pwdbuf, sizeof(pwdbuf), &pw);
! if (pw != NULL)
name = pw->pw_name;
+ else if (errorMessage)
+ {
+ if (pwerr != 0)
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("could not look up local user ID %d: %s\n"),
+ (int) user_id,
+ pqStrerror(pwerr, pwdbuf, sizeof(pwdbuf)));
+ else
+ printfPQExpBuffer(errorMessage,
+ libpq_gettext("local user with ID %d does not exist\n"),
+ (int) user_id);
+ }
#endif
! if (name)
! {
! result = strdup(name);
! if (result == NULL && errorMessage)
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
! }
pgunlock_thread();
! return result;
}
diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
index 59b6c16..8d35767 100644
*** a/src/interfaces/libpq/fe-auth.h
--- b/src/interfaces/libpq/fe-auth.h
***************
*** 19,24 ****
extern int pg_fe_sendauth(AuthRequest areq, PGconn *conn);
! extern char *pg_fe_getauthname(void);
#endif /* FE_AUTH_H */
--- 19,24 ----
extern int pg_fe_sendauth(AuthRequest areq, PGconn *conn);
! extern char *pg_fe_getauthname(PQExpBuffer errorMessage);
#endif /* FE_AUTH_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index b2f556c..25961b1 100644
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** static bool
*** 765,774 ****
connectOptions2(PGconn *conn)
{
/*
* If database name was not given, default it to equal user name
*/
! if ((conn->dbName == NULL || conn->dbName[0] == '\0')
! && conn->pguser != NULL)
{
if (conn->dbName)
free(conn->dbName);
--- 765,790 ----
connectOptions2(PGconn *conn)
{
/*
+ * If user name was not given, fetch it. (Most likely, the fetch will
+ * fail, since the only way we get here is if pg_fe_getauthname() failed
+ * during conninfo_add_defaults(). But now we want an error message.)
+ */
+ if (conn->pguser == NULL || conn->pguser[0] == '\0')
+ {
+ if (conn->pguser)
+ free(conn->pguser);
+ conn->pguser = pg_fe_getauthname(&conn->errorMessage);
+ if (!conn->pguser)
+ {
+ conn->status = CONNECTION_BAD;
+ return false;
+ }
+ }
+
+ /*
* If database name was not given, default it to equal user name
*/
! if (conn->dbName == NULL || conn->dbName[0] == '\0')
{
if (conn->dbName)
free(conn->dbName);
*************** keep_going: /* We will come back to
*** 1967,1972 ****
--- 1983,1989 ----
char pwdbuf[BUFSIZ];
struct passwd pass_buf;
struct passwd *pass;
+ int passerr;
uid_t uid;
gid_t gid;
*************** keep_going: /* We will come back to
*** 1987,1999 ****
goto error_return;
}
! pqGetpwuid(uid, &pass_buf, pwdbuf, sizeof(pwdbuf), &pass);
!
if (pass == NULL)
{
! appendPQExpBuffer(&conn->errorMessage,
! libpq_gettext("local user with ID %d does not exist\n"),
! (int) uid);
goto error_return;
}
--- 2004,2021 ----
goto error_return;
}
! passerr = pqGetpwuid(uid, &pass_buf, pwdbuf, sizeof(pwdbuf), &pass);
if (pass == NULL)
{
! if (passerr != 0)
! appendPQExpBuffer(&conn->errorMessage,
! libpq_gettext("could not look up local user ID %d: %s\n"),
! (int) uid,
! pqStrerror(passerr, sebuf, sizeof(sebuf)));
! else
! appendPQExpBuffer(&conn->errorMessage,
! libpq_gettext("local user with ID %d does not exist\n"),
! (int) uid);
goto error_return;
}
*************** conninfo_add_defaults(PQconninfoOption *
*** 4605,4622 ****
}
/*
! * Special handling for "user" option
*/
if (strcmp(option->keyword, "user") == 0)
{
! option->val = pg_fe_getauthname();
! if (!option->val)
! {
! if (errorMessage)
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
! return false;
! }
continue;
}
}
--- 4627,4641 ----
}
/*
! * Special handling for "user" option. Note that if pg_fe_getauthname
! * fails, we just leave the value as NULL; there's no need for this to
! * be an error condition if the caller provides a user name. The only
! * reason we do this now at all is so that callers of PQconndefaults
! * will see a correct default (barring error, of course).
*/
if (strcmp(option->keyword, "user") == 0)
{
! option->val = pg_fe_getauthname(NULL);
continue;
}
}
*************** pqGetHomeDirectory(char *buf, int bufsiz
*** 5843,5849 ****
struct passwd pwdstr;
struct passwd *pwd = NULL;
! if (pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd) != 0)
return false;
strlcpy(buf, pwd->pw_dir, bufsize);
return true;
--- 5862,5869 ----
struct passwd pwdstr;
struct passwd *pwd = NULL;
! (void) pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd);
! if (pwd == NULL)
return false;
strlcpy(buf, pwd->pw_dir, bufsize);
return true;
diff --git a/src/port/path.c b/src/port/path.c
index e8faac3..d0f72df 100644
*** a/src/port/path.c
--- b/src/port/path.c
*************** get_home_path(char *ret_path)
*** 777,783 ****
struct passwd pwdstr;
struct passwd *pwd = NULL;
! if (pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd) != 0)
return false;
strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
return true;
--- 777,784 ----
struct passwd pwdstr;
struct passwd *pwd = NULL;
! (void) pqGetpwuid(geteuid(), &pwdstr, pwdbuf, sizeof(pwdbuf), &pwd);
! if (pwd == NULL)
return false;
strlcpy(ret_path, pwd->pw_dir, MAXPGPATH);
return true;
diff --git a/src/port/thread.c b/src/port/thread.c
index 1568803..aab7451 100644
*** a/src/port/thread.c
--- b/src/port/thread.c
*************** pqStrerror(int errnum, char *strerrbuf,
*** 83,88 ****
--- 83,94 ----
/*
* Wrapper around getpwuid() or getpwuid_r() to mimic POSIX getpwuid_r()
* behaviour, if it is not available or required.
+ *
+ * Per POSIX, the possible cases are:
+ * success: returns zero, *result is non-NULL
+ * uid not found: returns zero, *result is NULL
+ * error during lookup: returns an errno code, *result is NULL
+ * (caller should *not* assume that the errno variable is set)
*/
#ifndef WIN32
int
*************** pqGetpwuid(uid_t uid, struct passwd * re
*** 93,114 ****
#ifdef GETPWUID_R_5ARG
/* POSIX version */
! getpwuid_r(uid, resultbuf, buffer, buflen, result);
#else
/*
* Early POSIX draft of getpwuid_r() returns 'struct passwd *'.
* getpwuid_r(uid, resultbuf, buffer, buflen)
*/
*result = getpwuid_r(uid, resultbuf, buffer, buflen);
#endif
#else
-
/* no getpwuid_r() available, just use getpwuid() */
*result = getpwuid(uid);
#endif
-
- return (*result == NULL) ? -1 : 0;
}
#endif
--- 99,123 ----
#ifdef GETPWUID_R_5ARG
/* POSIX version */
! return getpwuid_r(uid, resultbuf, buffer, buflen, result);
#else
/*
* Early POSIX draft of getpwuid_r() returns 'struct passwd *'.
* getpwuid_r(uid, resultbuf, buffer, buflen)
*/
+ errno = 0;
*result = getpwuid_r(uid, resultbuf, buffer, buflen);
+ /* paranoia: ensure we return zero on success */
+ return (*result == NULL) ? errno : 0;
#endif
#else
/* no getpwuid_r() available, just use getpwuid() */
+ errno = 0;
*result = getpwuid(uid);
+ /* paranoia: ensure we return zero on success */
+ return (*result == NULL) ? errno : 0;
#endif
}
#endif
On Fri, Jan 09, 2015 at 06:57:02PM -0500, Tom Lane wrote: > Commit a4c8f14364c27508233f8a31ac4b10a4c90235a9 turned > failure of pg_fe_getauthname() into a hard connection failure, whereas > previously it was harmless as long as the caller provided a username. > > I wonder if we shouldn't just revert that commit in toto. Yeah, > identifying an out-of-memory error might be useful, but this cure > seems a lot worse than the disease. What's more, this coding reports > *any* pg_fe_getauthname failure as "out of memory", which is much worse > than useless. +1 for reverting it as the next step > Alternatively, maybe don't try to resolve username this way until > we've found that the caller isn't providing any username. and for subsequently pursuing this.
One other point here: I realized while testing my patch that it's actually
impossible to provoke the failure mode Christoph is unhappy about in psql.
You can only see it in an application that uses PQsetdb/PQsetdbLogin,
which of course psql doesn't anymore. The reason is that in the PQconnect
family of functions, conninfo_add_defaults() is only applied after
filling in all available caller-supplied parameters, so if the user has
in one way or another specified the role name to use, we never invoke
pg_fe_getauthname() at all. It's only called if we have to use the
default role name, and in that context of course a hard failure is
appropriate. But in the PQsetdbLogin code path, we do pg_fe_getauthname
first and then overwrite (some) values from the parameters, so a failure
during conninfo_add_defaults() is premature.
This is a tad disturbing, because we are not using and therefore not
testing PQsetdb/PQsetdbLogin anywhere, which means that any failure modes
that path acquires that don't exist in the PQconnect code path are
guaranteed to go undetected in our testing. Now, I rather doubt that
we'd have found the problem with doesn't-handle-lack-of-/etc/passwd-well
even if we had been testing that code path, but we certainly won't find
problems in paths we don't test.
Not entirely sure what to do about this, but I predict this won't be
the last complaint unless we find some way to improve test coverage
in this area. Or perhaps we could turn PQsetdbLogin into a ***very***
thin wrapper around PQconnectdbParams?
regards, tom lane
Re: Tom Lane 2015-01-10 <22432.1420915326@sss.pgh.pa.us> > So what I propose we do with this is patch HEAD and 9.4 only. > We need to do *something* in 9.4 to address Christoph's complaint, and > that branch is new enough that we can probably get away with changing > officially-unsupported APIs. The lack of other field complaints makes > me okay with not trying to fix these issues further back. The problem isn't present in 9.3 and earlier (at least with postfix-pgsql), so there's no need to go back further. As for the number of complaints, I've received two independent reports on IRC, and upon googling the problem had been seen as early as in July [1] and August [2]. All four reports are for postfix-pgsql on Debian, but that's probably just because we pushed 9.4 into the next-release branch very early. (And I wish someone had told me about the problem, instead of only reporting it for postfix...) [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=756627 [2] https://workaround.org/comment/3415#comment-3415 Christoph -- cb@df7cb.de | http://www.df7cb.de/
Christoph Berg <cb@df7cb.de> writes:
> Re: Tom Lane 2015-01-10 <22432.1420915326@sss.pgh.pa.us>
>> So what I propose we do with this is patch HEAD and 9.4 only.
>> We need to do *something* in 9.4 to address Christoph's complaint, and
>> that branch is new enough that we can probably get away with changing
>> officially-unsupported APIs. The lack of other field complaints makes
>> me okay with not trying to fix these issues further back.
> The problem isn't present in 9.3 and earlier (at least with
> postfix-pgsql), so there's no need to go back further.
Right, the specific issue you're complaining of is new in 9.4. The
general issue that failures in /etc/passwd lookups aren't well reported
has been there more or less forever, but your immediate point is that no
such failure should be reported to the user at all if he's supplied a
login name to use.
It looks like the bad handling of Windows' GetUserName() failures has
been there a long time too, but given the lack of field reports I don't
feel a need to back-patch that very far either.
regards, tom lane
On Sat, Jan 10, 2015 at 02:02:54PM -0500, Tom Lane wrote: > Not entirely sure what to do about this, but I predict this won't be > the last complaint unless we find some way to improve test coverage > in this area. Or perhaps we could turn PQsetdbLogin into a > ***very*** thin wrapper around PQconnectdbParams? +1 for this. Having a single point of truth here would be a big win. Cheers, David. -- David Fetter <david@fetter.org> http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fetter@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Christoph Berg <cb@df7cb.de> writes:
> Re: Tom Lane 2015-01-10 <22432.1420915326@sss.pgh.pa.us>
>> So what I propose we do with this is patch HEAD and 9.4 only.
>> We need to do *something* in 9.4 to address Christoph's complaint, and
>> that branch is new enough that we can probably get away with changing
>> officially-unsupported APIs. The lack of other field complaints makes
>> me okay with not trying to fix these issues further back.
> The problem isn't present in 9.3 and earlier (at least with
> postfix-pgsql), so there's no need to go back further.
I've committed a fix for this in HEAD and 9.4.
regards, tom lane
Re: Tom Lane 2015-01-11 <13609.1420998872@sss.pgh.pa.us> > > The problem isn't present in 9.3 and earlier (at least with > > postfix-pgsql), so there's no need to go back further. > > I've committed a fix for this in HEAD and 9.4. I've just tested with the HEAD libpq and the issue is fixed. Thanks! In the first iteration of the test the database was still down and I got this error message: Jan 11 19:08:53 lehmann postfix/trivial-rewrite[3453]: warning: connect to pgsql server localhost:5432: could not connectto server: Connection refused??Is the server running on host "localhost" (::1) and accepting??TCP/IP connections onport 5432??could not connect to server: Connection refused??Is the server running on host "localhost" (127.0.0.1) and accepting??TCP/IPconnections on port 5432?? The ?? are probably postfix and/or syslog messing with newlines or the like. At first I was confused that the PG part of the error message is duplicated, but then I figured that's two different "localhost" addresses, so all is fine. Christoph -- cb@df7cb.de | http://www.df7cb.de/