pg_upgrade segfaults when given an invalid PGSERVICE value
If you try running pg_upgrade with the PGSERVICE environment variable
set to some invalid/non-existent service pg_upgrade segfaults
Program received signal SIGSEGV, Segmentation fault.
0x000000000040bdd1 in check_pghost_envvar () at server.c:304
304 for (option = start; option->keyword != NULL; option++)
(gdb) p start
$5 = (PQconninfoOption *) 0x0
PQconndefaults can return NULL if it has issues.
The attached patch prints a minimally useful error message. I don't a
good way of getting a more useful error message out of PQconndefaults()
I checked this against master but it was reported to me as a issue in 9.2
Steve
Attachments:
pg_upgrade_pgservice.difftext/x-patch; name=pg_upgrade_pgservice.diffDownload
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
index ed67759..f2e4d63 100644
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*************** check_pghost_envvar(void)
*** 300,306 ****
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
!
for (option = start; option->keyword != NULL; option++)
{
if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
--- 300,309 ----
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
! if (start == NULL)
! {
! pg_log(PG_FATAL,"can not get default connection options\n");
! }
for (option = start; option->keyword != NULL; option++)
{
if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
On Mon, Mar 18, 2013 at 12:08:09PM -0400, Steve Singer wrote:
If you try running pg_upgrade with the PGSERVICE environment
variable set to some invalid/non-existent service pg_upgrade
segfaultsProgram received signal SIGSEGV, Segmentation fault.
0x000000000040bdd1 in check_pghost_envvar () at server.c:304
304 for (option = start; option->keyword != NULL; option++)
(gdb) p start
$5 = (PQconninfoOption *) 0x0PQconndefaults can return NULL if it has issues.
The attached patch prints a minimally useful error message. I don't
a good way of getting a more useful error message out of
PQconndefaults()I checked this against master but it was reported to me as a issue in 9.2
Well, that's interesting. There is no mention of PQconndefaults()
returning NULL except for out of memory:
Returns a connection options array. This can be used to determine
all possible <function>PQconnectdb</function> options and their
current default values. The return value points to an array of
<structname>PQconninfoOption</structname> structures, which ends
with an entry having a null <structfield>keyword</> pointer. The
--> null pointer is returned if memory could not be allocated. Note that
the current default values (<structfield>val</structfield> fields)
will depend on environment variables and other context. Callers
must treat the connection options data as read-only.
Looking at libpq/fe-connect.c::PQconndefaults(), it calls
conninfo_add_defaults(), which has this:
/*
* If there's a service spec, use it to obtain any not-explicitly-given
* parameters.
*/
if (parseServiceInfo(options, errorMessage) != 0)
return false;
so it is clearly possible for PQconndefaults() to return NULL for
service file failures. The questions are:
* Is this what we want?
* Should we document this?
* Should we change this to just throw a warning?
Also, it seems pg_upgrade isn't the only utility that is confused:
contrib/postgres_fdw/option.c and contrib/dblink/dblink.c think
PQconndefaults() returning NULL means out of memory and report that
as the error string.
bin/scripts/pg_isready.c and contrib/pg_upgrade/server.c have no
check for NULL return.
libpq/test/uri-regress.c knows to throw a generic error message.
So, we have some decisions and work to do.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 13-03-18 09:17 PM, Bruce Momjian wrote:
On Mon, Mar 18, 2013 at 12:08:09PM -0400, Steve Singer wrote:
If you try running pg_upgrade with the PGSERVICE environment
variable set to some invalid/non-existent service pg_upgrade
segfaultsProgram received signal SIGSEGV, Segmentation fault.
0x000000000040bdd1 in check_pghost_envvar () at server.c:304
304 for (option = start; option->keyword != NULL; option++)
(gdb) p start
$5 = (PQconninfoOption *) 0x0PQconndefaults can return NULL if it has issues.
The attached patch prints a minimally useful error message. I don't
a good way of getting a more useful error message out of
PQconndefaults()I checked this against master but it was reported to me as a issue in 9.2
Well, that's interesting. There is no mention of PQconndefaults()
returning NULL except for out of memory:Returns a connection options array. This can be used to determine
all possible <function>PQconnectdb</function> options and their
current default values. The return value points to an array of
<structname>PQconninfoOption</structname> structures, which ends
with an entry having a null <structfield>keyword</> pointer. The
--> null pointer is returned if memory could not be allocated. Note that
the current default values (<structfield>val</structfield> fields)
will depend on environment variables and other context. Callers
must treat the connection options data as read-only.Looking at libpq/fe-connect.c::PQconndefaults(), it calls
conninfo_add_defaults(), which has this:/*
* If there's a service spec, use it to obtain any not-explicitly-given
* parameters.
*/
if (parseServiceInfo(options, errorMessage) != 0)
return false;so it is clearly possible for PQconndefaults() to return NULL for
service file failures. The questions are:* Is this what we want?
What other choices do we have? I don't think PQconndefaults() should
continue on as if PGSERVICE wasn't set in the environment after a
failure from parseServiceInfo.
* Should we document this?
Yes the documentation should indicate that PQconndefaults() can return
NULL for more than just memory failures.
* Should we change this to just throw a warning?
How would we communicate warnings from PQconndefaults() back to the caller?
Also, it seems pg_upgrade isn't the only utility that is confused:
contrib/postgres_fdw/option.c and contrib/dblink/dblink.c think
PQconndefaults() returning NULL means out of memory and report that
as the error string.bin/scripts/pg_isready.c and contrib/pg_upgrade/server.c have no
check for NULL return.libpq/test/uri-regress.c knows to throw a generic error message.
So, we have some decisions and work to do.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Mar 19, 2013 at 10:12:21AM -0400, Steve Singer wrote:
so it is clearly possible for PQconndefaults() to return NULL for
service file failures. The questions are:* Is this what we want?
What other choices do we have? I don't think PQconndefaults() should
continue on as if PGSERVICE wasn't set in the environment after a
failure from parseServiceInfo.
True. Ignoring a service specification seems wrong, and issuing a
warning message weak.
* Should we document this?
Yes the documentation should indicate that PQconndefaults() can
return NULL for more than just memory failures.
The attached patch fixes this. I am unclear about backpatching this as
it hits lot of code, is rare, and adds new translation strings. On the
other hand, it does crash the applications.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
Attachments:
libpq.difftext/x-diff; charset=us-asciiDownload
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
new file mode 100644
index bba0d2a..eca061f
*** a/contrib/dblink/dblink.c
--- b/contrib/dblink/dblink.c
*************** dblink_fdw_validator(PG_FUNCTION_ARGS)
*** 1947,1953 ****
if (!options) /* assume reason for failure is OOM */
ereport(ERROR,
(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
! errmsg("out of memory"),
errdetail("could not get libpq's default connection options")));
}
--- 1947,1953 ----
if (!options) /* assume reason for failure is OOM */
ereport(ERROR,
(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
! errmsg("out of memory or service name cannot be found"),
errdetail("could not get libpq's default connection options")));
}
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
new file mode 100644
index ed67759..cd246ce
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*************** check_pghost_envvar(void)
*** 300,306 ****
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
!
for (option = start; option->keyword != NULL; option++)
{
if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
--- 300,310 ----
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
! if (!start)
! pg_log(PG_FATAL,
! "out of memory or service name cannot be found\n"
! "could not get libpq's default connection options\n");
!
for (option = start; option->keyword != NULL; option++)
{
if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
new file mode 100644
index 123cb4f..d4890be
*** a/contrib/postgres_fdw/option.c
--- b/contrib/postgres_fdw/option.c
*************** InitPgFdwOptions(void)
*** 169,175 ****
if (!libpq_options) /* assume reason for failure is OOM */
ereport(ERROR,
(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
! errmsg("out of memory"),
errdetail("could not get libpq's default connection options")));
/* Count how many libpq options are available. */
--- 169,175 ----
if (!libpq_options) /* assume reason for failure is OOM */
ereport(ERROR,
(errcode(ERRCODE_FDW_OUT_OF_MEMORY),
! errmsg("out of memory or service name cannot be found"),
errdetail("could not get libpq's default connection options")));
/* Count how many libpq options are available. */
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
new file mode 100644
index 775d250..ceb873e
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*************** typedef struct
*** 481,487 ****
current default values. The return value points to an array of
<structname>PQconninfoOption</structname> structures, which ends
with an entry having a null <structfield>keyword</> pointer. The
! null pointer is returned if memory could not be allocated. Note that
the current default values (<structfield>val</structfield> fields)
will depend on environment variables and other context. Callers
must treat the connection options data as read-only.
--- 481,488 ----
current default values. The return value points to an array of
<structname>PQconninfoOption</structname> structures, which ends
with an entry having a null <structfield>keyword</> pointer. The
! null pointer is returned if memory could not be allocated or
! the specified connection service name cannot be found. Note that
the current default values (<structfield>val</structfield> fields)
will depend on environment variables and other context. Callers
must treat the connection options data as read-only.
diff --git a/src/bin/scripts/pg_isready.c b/src/bin/scripts/pg_isready.c
new file mode 100644
index 4ba257d..f02f4f9
*** a/src/bin/scripts/pg_isready.c
--- b/src/bin/scripts/pg_isready.c
*************** main(int argc, char **argv)
*** 126,131 ****
--- 126,138 ----
* Get the default host and port so we can display them in our output
*/
connect_options = PQconndefaults();
+ if (!connect_options)
+ {
+ fprintf(stderr, _("out of memory or service name cannot be found\n"
+ "could not get libpq's default connection options\n"));
+ exit(PQPING_NO_ATTEMPT);
+ }
+
conn_opt_ptr = connect_options;
while (conn_opt_ptr->keyword)
{
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index eea9c6b..60d0c8b
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** connectOptions2(PGconn *conn)
*** 850,856 ****
*
* Construct a default connection options array, which identifies all the
* available options and shows any default values that are available from the
! * environment etc. On error (eg out of memory), NULL is returned.
*
* Using this function, an application may determine all possible options
* and their current default values.
--- 850,857 ----
*
* Construct a default connection options array, which identifies all the
* available options and shows any default values that are available from the
! * environment etc. On error (out of memory or invalid service name),
! * NULL is returned.
*
* Using this function, an application may determine all possible options
* and their current default values.
Bruce Momjian <bruce@momjian.us> writes:
On Tue, Mar 19, 2013 at 10:12:21AM -0400, Steve Singer wrote:
* Should we document this?
Yes the documentation should indicate that PQconndefaults() can
return NULL for more than just memory failures.
The attached patch fixes this. I am unclear about backpatching this as
it hits lot of code, is rare, and adds new translation strings. On the
other hand, it does crash the applications.
I don't particularly care for hard-wiring knowledge that bad service
name is the only other reason for PQconndefaults to fail (even assuming
that that's true, a point not in evidence ATM). I care even less for
continuing to use ERRCODE_FDW_OUT_OF_MEMORY for errors clearly outside
its meaning.
I think we should either change PQconndefaults to *not* fail in this
circumstance, or find a way to return an error message.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Mar 20, 2013 at 12:30:32PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Tue, Mar 19, 2013 at 10:12:21AM -0400, Steve Singer wrote:
* Should we document this?
Yes the documentation should indicate that PQconndefaults() can
return NULL for more than just memory failures.The attached patch fixes this. I am unclear about backpatching this as
it hits lot of code, is rare, and adds new translation strings. On the
other hand, it does crash the applications.I don't particularly care for hard-wiring knowledge that bad service
name is the only other reason for PQconndefaults to fail (even assuming
that that's true, a point not in evidence ATM). I care even less for
continuing to use ERRCODE_FDW_OUT_OF_MEMORY for errors clearly outside
its meaning.
Yes, overloading that error code was bad.
I think we should either change PQconndefaults to *not* fail in this
circumstance, or find a way to return an error message.
Well, Steve Singer didn't like the idea of ignoring a service lookup
failure. What do others think? We can throw a warning, but there is no
way to know if the application allows the user to see it.
Adding a way to communicate the service failure reason to the user would
require a libpq API change, unless we go crazy and say -1 means service
failure, and assume -1 can't be a valid pointer.
Perhaps we need to remove the memory references and just report a
failure, and mention services as a possible cause.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Bruce Momjian <bruce@momjian.us> writes:
On Wed, Mar 20, 2013 at 12:30:32PM -0400, Tom Lane wrote:
I think we should either change PQconndefaults to *not* fail in this
circumstance, or find a way to return an error message.
Well, Steve Singer didn't like the idea of ignoring a service lookup
failure. What do others think? We can throw a warning, but there is no
way to know if the application allows the user to see it.
Short of changing PQconndefaults's API, it seems like the only
reasonable answer is to not fail *in the context of PQconndefaults*.
We could still fail for bad service name in a real connection operation
(where there is an opportunity to return an error message).
While this surely isn't the nicest answer, it doesn't seem totally
unreasonable to me. A bad service name indeed does not contribute
anything to the set of defaults available.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Mar 20, 2013 at 01:30:20PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Wed, Mar 20, 2013 at 12:30:32PM -0400, Tom Lane wrote:
I think we should either change PQconndefaults to *not* fail in this
circumstance, or find a way to return an error message.Well, Steve Singer didn't like the idea of ignoring a service lookup
failure. What do others think? We can throw a warning, but there is no
way to know if the application allows the user to see it.Short of changing PQconndefaults's API, it seems like the only
reasonable answer is to not fail *in the context of PQconndefaults*.
We could still fail for bad service name in a real connection operation
(where there is an opportunity to return an error message).
Yes, that is _a_ plan.
While this surely isn't the nicest answer, it doesn't seem totally
unreasonable to me. A bad service name indeed does not contribute
anything to the set of defaults available.
I think the concern is that the services file could easily change the
defaults that are used for connecting, though you could argue that the
real defaults for a bad service entry are properly returned.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 13-03-20 02:17 PM, Bruce Momjian wrote:
On Wed, Mar 20, 2013 at 01:30:20PM -0400, Tom Lane wrote:
While this surely isn't the nicest answer, it doesn't seem totally
unreasonable to me. A bad service name indeed does not contribute
anything to the set of defaults available.I think the concern is that the services file could easily change the
defaults that are used for connecting, though you could argue that the
real defaults for a bad service entry are properly returned.
Yes, my concern is that if I have a typo in the value of PGSERVICE I
don't want to end up getting connected a connection to localhost instead
of an error.
From a end-user expectations point of view I am okay with somehow
marking the structure returned by PQconndefaults in a way that the
connect calls will later fail.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Steve Singer <ssinger@ca.afilias.info> writes:
On 13-03-20 02:17 PM, Bruce Momjian wrote:
I think the concern is that the services file could easily change the
defaults that are used for connecting, though you could argue that the
real defaults for a bad service entry are properly returned.
Yes, my concern is that if I have a typo in the value of PGSERVICE I
don't want to end up getting connected a connection to localhost instead
of an error.
From a end-user expectations point of view I am okay with somehow
marking the structure returned by PQconndefaults in a way that the
connect calls will later fail.
Unless the program changes the value of PGSERVICE, surely all subsequent
connection attempts will fail for the same reason, regardless of what
PQconndefaults returns?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 13-03-20 05:54 PM, Tom Lane wrote:
Steve Singer <ssinger@ca.afilias.info> writes:
From a end-user expectations point of view I am okay with somehow
marking the structure returned by PQconndefaults in a way that the
connect calls will later fail.Unless the program changes the value of PGSERVICE, surely all subsequent
connection attempts will fail for the same reason, regardless of what
PQconndefaults returns?regards, tom lane
So your proposing we do something like the attached patch? Where we
change conninfo_add_defaults to ignore an invalid PGSERVICE if being
called by PQconndefaults() but keep the existing behaviour in other
contexts where it is actually being used to establish a connection?
In this case even if someone takes the result of PQconndefaults and uses
that to build connection options for a new connection it should fail
when it does the pgservice lookup when establishing the connection.
That sounds reasonable to me.
Steve
Attachments:
conn_defaults.difftext/x-patch; name=conn_defaults.diffDownload
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
index ed67759..c1d8d69 100644
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*************** check_pghost_envvar(void)
*** 300,306 ****
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
!
for (option = start; option->keyword != NULL; option++)
{
if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
--- 300,309 ----
/* Get valid libpq env vars from the PQconndefaults function */
start = PQconndefaults();
! if (start == NULL)
! {
! pg_log(PG_FATAL,"can not get default connection options out of memory\n");
! }
for (option = start; option->keyword != NULL; option++)
{
if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index eea9c6b..0a9a29e 100644
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** static PQconninfoOption *conninfo_array_
*** 347,353 ****
const char *const * values, PQExpBuffer errorMessage,
bool use_defaults, int expand_dbname);
static bool conninfo_add_defaults(PQconninfoOption *options,
! PQExpBuffer errorMessage);
static PQconninfoOption *conninfo_uri_parse(const char *uri,
PQExpBuffer errorMessage, bool use_defaults);
static bool conninfo_uri_parse_options(PQconninfoOption *options,
--- 347,354 ----
const char *const * values, PQExpBuffer errorMessage,
bool use_defaults, int expand_dbname);
static bool conninfo_add_defaults(PQconninfoOption *options,
! PQExpBuffer errorMessage,
! bool ignore_missing_service);
static PQconninfoOption *conninfo_uri_parse(const char *uri,
PQExpBuffer errorMessage, bool use_defaults);
static bool conninfo_uri_parse_options(PQconninfoOption *options,
*************** PQconndefaults(void)
*** 875,881 ****
connOptions = conninfo_init(&errorBuf);
if (connOptions != NULL)
{
! if (!conninfo_add_defaults(connOptions, &errorBuf))
{
PQconninfoFree(connOptions);
connOptions = NULL;
--- 876,882 ----
connOptions = conninfo_init(&errorBuf);
if (connOptions != NULL)
{
! if (!conninfo_add_defaults(connOptions, &errorBuf,true))
{
PQconninfoFree(connOptions);
connOptions = NULL;
*************** conninfo_parse(const char *conninfo, PQE
*** 4243,4249 ****
*/
if (use_defaults)
{
! if (!conninfo_add_defaults(options, errorMessage))
{
PQconninfoFree(options);
return NULL;
--- 4244,4250 ----
*/
if (use_defaults)
{
! if (!conninfo_add_defaults(options, errorMessage,false))
{
PQconninfoFree(options);
return NULL;
*************** conninfo_array_parse(const char *const *
*** 4395,4401 ****
*/
if (use_defaults)
{
! if (!conninfo_add_defaults(options, errorMessage))
{
PQconninfoFree(options);
return NULL;
--- 4396,4402 ----
*/
if (use_defaults)
{
! if (!conninfo_add_defaults(options, errorMessage,false))
{
PQconninfoFree(options);
return NULL;
*************** conninfo_array_parse(const char *const *
*** 4416,4422 ****
* error condition here --- we just leave the option's value as NULL.
*/
static bool
! conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
{
PQconninfoOption *option;
char *tmp;
--- 4417,4424 ----
* error condition here --- we just leave the option's value as NULL.
*/
static bool
! conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage,
! bool ignore_missing_service)
{
PQconninfoOption *option;
char *tmp;
*************** conninfo_add_defaults(PQconninfoOption *
*** 4425,4431 ****
* If there's a service spec, use it to obtain any not-explicitly-given
* parameters.
*/
! if (parseServiceInfo(options, errorMessage) != 0)
return false;
/*
--- 4427,4434 ----
* If there's a service spec, use it to obtain any not-explicitly-given
* parameters.
*/
! if (parseServiceInfo(options, errorMessage) != 0 &&
! ! ignore_missing_service)
return false;
/*
*************** conninfo_uri_parse(const char *uri, PQEx
*** 4511,4517 ****
*/
if (use_defaults)
{
! if (!conninfo_add_defaults(options, errorMessage))
{
PQconninfoFree(options);
return NULL;
--- 4514,4520 ----
*/
if (use_defaults)
{
! if (!conninfo_add_defaults(options, errorMessage,false))
{
PQconninfoFree(options);
return NULL;
On Mon, Mar 25, 2013 at 10:59:21AM -0400, Steve Singer wrote:
On 13-03-20 05:54 PM, Tom Lane wrote:
Steve Singer <ssinger@ca.afilias.info> writes:
From a end-user expectations point of view I am okay with somehow
marking the structure returned by PQconndefaults in a way that the
connect calls will later fail.Unless the program changes the value of PGSERVICE, surely all subsequent
connection attempts will fail for the same reason, regardless of what
PQconndefaults returns?regards, tom lane
So your proposing we do something like the attached patch? Where we
change conninfo_add_defaults to ignore an invalid PGSERVICE if being
called by PQconndefaults() but keep the existing behaviour in other
contexts where it is actually being used to establish a connection?In this case even if someone takes the result of PQconndefaults and
uses that to build connection options for a new connection it should
fail when it does the pgservice lookup when establishing the
connection. That sounds reasonable to me.
I was just about to look at this --- thanks for doing the legwork. Your
fix is for conninfo_add_defaults() to conditionally fail, and that is a
logical approach.
One big problem I see is that effectively we have made
conninfo_add_defaults() to conditionally fail based on a missing service
(ignore_missing_service), and I think that reinforced my feeling that
having PQconndefaults() not fail on a missing service is just an awkward
approach to the problem.
We are taking this approach because PQconndefaults() doesn't have an API
to return the error cause, while other API calls do. Returning true so
we can later report the right error from a later API call just feels
wrong.
However, I am also unclear about what alternative to suggest, except to
sprinkle a "possible service problem" message to all the call failure.
If we do want to the route of the patch, we should document this in the
docs and C code so it is clear why we went in this direction.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Bruce Momjian <bruce@momjian.us> writes:
We are taking this approach because PQconndefaults() doesn't have an API
to return the error cause, while other API calls do. Returning true so
we can later report the right error from a later API call just feels
wrong.
Well, plan B would be to invent a replacement function that does have
the ability to return an error message, but that seems like a lot of
work for a problem that's so marginal that it wasn't noticed till now.
(It's not so much creating the function that worries me, it's fixing
clients to use it.)
Plan C would be to redefine bogus value of PGSERVICE as not an error,
period.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Mar 25, 2013 at 07:07:42PM -0400, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
We are taking this approach because PQconndefaults() doesn't have an API
to return the error cause, while other API calls do. Returning true so
we can later report the right error from a later API call just feels
wrong.Well, plan B would be to invent a replacement function that does have
the ability to return an error message, but that seems like a lot of
work for a problem that's so marginal that it wasn't noticed till now.
(It's not so much creating the function that worries me, it's fixing
clients to use it.)Plan C would be to redefine bogus value of PGSERVICE as not an error,
period.
Given all of these poor options, is defining a PQconndefaults() as
perhaps out of memory or a service file problem really not better?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Bruce Momjian <bruce@momjian.us> writes:
On Mon, Mar 25, 2013 at 07:07:42PM -0400, Tom Lane wrote:
Well, plan B would be to invent a replacement function that does have
the ability to return an error message, but that seems like a lot of
work for a problem that's so marginal that it wasn't noticed till now.
(It's not so much creating the function that worries me, it's fixing
clients to use it.)Plan C would be to redefine bogus value of PGSERVICE as not an error,
period.
Given all of these poor options, is defining a PQconndefaults() as
perhaps out of memory or a service file problem really not better?
Uh ... no. In the first place, what evidence have you got that those
are (and will continue to be) the only two possible causes? In the
second place, this still requires changing every client of
PQconndefaults(), even if it's only to the extent of fixing their
error message texts. If we're going to do that, I'd rather ask them
to change to a more future-proof solution.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 13-03-26 12:40 AM, Tom Lane wrote:
Bruce Momjian <bruce@momjian.us> writes:
On Mon, Mar 25, 2013 at 07:07:42PM -0400, Tom Lane wrote:
Well, plan B would be to invent a replacement function that does have
the ability to return an error message, but that seems like a lot of
work for a problem that's so marginal that it wasn't noticed till now.
(It's not so much creating the function that worries me, it's fixing
clients to use it.)Plan C would be to redefine bogus value of PGSERVICE as not an error,
period.Given all of these poor options, is defining a PQconndefaults() as
perhaps out of memory or a service file problem really not better?Uh ... no. In the first place, what evidence have you got that those
are (and will continue to be) the only two possible causes? In the
second place, this still requires changing every client of
PQconndefaults(), even if it's only to the extent of fixing their
error message texts. If we're going to do that, I'd rather ask them
to change to a more future-proof solution.
So to summarise:
Plan A: The first patch I attached for pg_upgrade + documentation
changes, and changing the other places that call PQconndefaults() to
accept failures on either out of memory, or an invalid PGSERVICE
Plan B: Create a new function PQconndefaults2(char * errorBuffer) or
something similar that returned error information to the caller.
Plan C: PQconndefaults() just ignores an invalid service but connection
attempts fail because other callers of conninfo_add_defaults still pay
attention to connection failures. This is the second patch I sent.
Plan D: Service lookup failures are always ignored by
conninfo_add_defaults. If you attempt to connect with a bad PGSERVICE
set it will behave as if no PGSERVICE value was set. I don't think
anyone explicitly proposed this yet.
Plan 'D' is the only option that I'm opposed to, it will effect a lot
more applications then ones that call PQconndefaults() and I feel it
will confuse users.
I'm not convinced plan B is worth the effort of having to maintain two
versions of PQconndefaults() for a while to fix a corner case.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Mar 28, 2013 at 03:06:30PM -0400, Steve Singer wrote:
So to summarise:
Plan A: The first patch I attached for pg_upgrade + documentation
changes, and changing the other places that call PQconndefaults() to
accept failures on either out of memory, or an invalid PGSERVICEPlan B: Create a new function PQconndefaults2(char * errorBuffer) or
something similar that returned error information to the caller.Plan C: PQconndefaults() just ignores an invalid service but
connection attempts fail because other callers of
conninfo_add_defaults still pay attention to connection failures.
This is the second patch I sent.Plan D: Service lookup failures are always ignored by
conninfo_add_defaults. If you attempt to connect with a bad
PGSERVICE set it will behave as if no PGSERVICE value was set. I
don't think anyone explicitly proposed this yet.Plan 'D' is the only option that I'm opposed to, it will effect a
lot more applications then ones that call PQconndefaults() and I
feel it will confuse users.I'm not convinced plan B is worth the effort of having to maintain
two versions of PQconndefaults() for a while to fix a corner case.
Yep, that's pretty much it. I agree B is overkill, and D seems
dangerous. I think Tom likes C --- my only question is how many people
will be confused that C returns inaccurate information, because though
it returns connection options, it doesn't process the service file,
while forced processing of the service file for real connections throws
an error.
We might be fine with C, but it must be documented in the C code and
SGML docs.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ It's impossible for everything to be true. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Mar 28, 2013 at 03:06:30PM -0400, Steve Singer wrote:
So to summarise:
Plan A: The first patch I attached for pg_upgrade + documentation
changes, and changing the other places that call PQconndefaults() to
accept failures on either out of memory, or an invalid PGSERVICEPlan B: Create a new function PQconndefaults2(char * errorBuffer) or
something similar that returned error information to the caller.Plan C: PQconndefaults() just ignores an invalid service but
connection attempts fail because other callers of
conninfo_add_defaults still pay attention to connection failures.
This is the second patch I sent.Plan D: Service lookup failures are always ignored by
conninfo_add_defaults. If you attempt to connect with a bad
PGSERVICE set it will behave as if no PGSERVICE value was set. I
don't think anyone explicitly proposed this yet.Plan 'D' is the only option that I'm opposed to, it will effect a
lot more applications then ones that call PQconndefaults() and I
feel it will confuse users.I'm not convinced plan B is worth the effort of having to maintain
two versions of PQconndefaults() for a while to fix a corner case.
OK, I am back to looking at this issue from March. I went with option
C, patch attached, which seems to be the most popular. It is similar to
Steve's patch, but structured differently and includes a doc patch.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
Attachments:
service.difftext/x-diff; charset=us-asciiDownload
diff --git a/contrib/pg_upgrade/server.c b/contrib/pg_upgrade/server.c
new file mode 100644
index b75f553..7d2aa35
*** a/contrib/pg_upgrade/server.c
--- b/contrib/pg_upgrade/server.c
*************** check_pghost_envvar(void)
*** 325,330 ****
--- 325,333 ----
start = PQconndefaults();
+ if (!start)
+ pg_fatal("out of memory\n");
+
for (option = start; option->keyword != NULL; option++)
{
if (option->envvar && (strcmp(option->envvar, "PGHOST") == 0 ||
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
new file mode 100644
index 955f248..503a63a
*** a/doc/src/sgml/libpq.sgml
--- b/doc/src/sgml/libpq.sgml
*************** typedef struct
*** 483,489 ****
with an entry having a null <structfield>keyword</> pointer. The
null pointer is returned if memory could not be allocated. Note that
the current default values (<structfield>val</structfield> fields)
! will depend on environment variables and other context. Callers
must treat the connection options data as read-only.
</para>
--- 483,490 ----
with an entry having a null <structfield>keyword</> pointer. The
null pointer is returned if memory could not be allocated. Note that
the current default values (<structfield>val</structfield> fields)
! will depend on environment variables and other context. A
! missing or invalid service file will be silently ignored. Callers
must treat the connection options data as read-only.
</para>
diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c
new file mode 100644
index 975f795..9797140
*** a/src/interfaces/libpq/fe-auth.c
--- b/src/interfaces/libpq/fe-auth.c
*************** pg_fe_sendauth(AuthRequest areq, PGconn
*** 982,988 ****
* if there is an error, return NULL with an error message in errorMessage
*/
char *
! pg_fe_getauthname(PQExpBuffer errorMessage)
{
const char *name = NULL;
char *authn;
--- 982,988 ----
* if there is an error, return NULL with an error message in errorMessage
*/
char *
! pg_fe_getauthname(void)
{
const char *name = NULL;
char *authn;
diff --git a/src/interfaces/libpq/fe-auth.h b/src/interfaces/libpq/fe-auth.h
new file mode 100644
index bfddc68..25440b0
*** 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(PQExpBuffer errorMessage);
#endif /* FE_AUTH_H */
--- 19,24 ----
extern int pg_fe_sendauth(AuthRequest areq, PGconn *conn);
! extern char *pg_fe_getauthname(void);
#endif /* FE_AUTH_H */
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
new file mode 100644
index 8dd1a59..7ab4a9a
*** a/src/interfaces/libpq/fe-connect.c
--- b/src/interfaces/libpq/fe-connect.c
*************** PQconndefaults(void)
*** 875,881 ****
connOptions = conninfo_init(&errorBuf);
if (connOptions != NULL)
{
! if (!conninfo_add_defaults(connOptions, &errorBuf))
{
PQconninfoFree(connOptions);
connOptions = NULL;
--- 875,882 ----
connOptions = conninfo_init(&errorBuf);
if (connOptions != NULL)
{
! /* pass NULL errorBuf to ignore errors */
! if (!conninfo_add_defaults(connOptions, NULL))
{
PQconninfoFree(connOptions);
connOptions = NULL;
*************** conninfo_array_parse(const char *const *
*** 4412,4420 ****
*
* Defaults are obtained from a service file, environment variables, etc.
*
! * Returns TRUE if successful, otherwise FALSE; errorMessage is filled in
! * upon failure. Note that failure to locate a default value is not an
! * error condition here --- we just leave the option's value as NULL.
*/
static bool
conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
--- 4413,4422 ----
*
* Defaults are obtained from a service file, environment variables, etc.
*
! * Returns TRUE if successful, otherwise FALSE; errorMessage, if supplied,
! * is filled in upon failure. Note that failure to locate a default value
! * is not an error condition here --- we just leave the option's value as
! * NULL.
*/
static bool
conninfo_add_defaults(PQconninfoOption *options, PQExpBuffer errorMessage)
*************** conninfo_add_defaults(PQconninfoOption *
*** 4424,4432 ****
/*
* If there's a service spec, use it to obtain any not-explicitly-given
! * parameters.
*/
! if (parseServiceInfo(options, errorMessage) != 0)
return false;
/*
--- 4426,4435 ----
/*
* 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(options, errorMessage) != 0 && errorMessage)
return false;
/*
*************** conninfo_add_defaults(PQconninfoOption *
*** 4448,4455 ****
option->val = strdup(tmp);
if (!option->val)
{
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
return false;
}
continue;
--- 4451,4459 ----
option->val = strdup(tmp);
if (!option->val)
{
! if (errorMessage)
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
return false;
}
continue;
*************** conninfo_add_defaults(PQconninfoOption *
*** 4465,4472 ****
option->val = strdup(option->compiled);
if (!option->val)
{
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
return false;
}
continue;
--- 4469,4477 ----
option->val = strdup(option->compiled);
if (!option->val)
{
! if (errorMessage)
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
return false;
}
continue;
*************** conninfo_add_defaults(PQconninfoOption *
*** 4477,4483 ****
*/
if (strcmp(option->keyword, "user") == 0)
{
! option->val = pg_fe_getauthname(errorMessage);
continue;
}
}
--- 4482,4488 ----
*/
if (strcmp(option->keyword, "user") == 0)
{
! option->val = pg_fe_getauthname();
continue;
}
}
On Fri, Nov 29, 2013 at 04:14:00PM -0500, Bruce Momjian wrote:
On Thu, Mar 28, 2013 at 03:06:30PM -0400, Steve Singer wrote:
So to summarise:
Plan A: The first patch I attached for pg_upgrade + documentation
changes, and changing the other places that call PQconndefaults() to
accept failures on either out of memory, or an invalid PGSERVICEPlan B: Create a new function PQconndefaults2(char * errorBuffer) or
something similar that returned error information to the caller.Plan C: PQconndefaults() just ignores an invalid service but
connection attempts fail because other callers of
conninfo_add_defaults still pay attention to connection failures.
This is the second patch I sent.Plan D: Service lookup failures are always ignored by
conninfo_add_defaults. If you attempt to connect with a bad
PGSERVICE set it will behave as if no PGSERVICE value was set. I
don't think anyone explicitly proposed this yet.Plan 'D' is the only option that I'm opposed to, it will effect a
lot more applications then ones that call PQconndefaults() and I
feel it will confuse users.I'm not convinced plan B is worth the effort of having to maintain
two versions of PQconndefaults() for a while to fix a corner case.OK, I am back to looking at this issue from March. I went with option
C, patch attached, which seems to be the most popular. It is similar to
Steve's patch, but structured differently and includes a doc patch.
Patch applied.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ Everyone has their own god. +
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers