Found small issue with OUT params

Tony Caduto <tony_caduto@amsoftwaredesign.com> writes:

If I call it like this:
select * from FIND_USER_SOCKET_BYNAME('juser');
I would expect to get back 1 value with the name of the OUT param
(OUT_SOCKET_ADDRESS).
However it comes back with the name of the function

This is intentional, for compatibility with the pre-existing behavior
with functions in FROM. A function that isn't returning a record is
effectively declared as
FROM foo(...) AS foo(foo)
while a function that does return a record type gives you
FROM foo(...) AS foo(col1, col2)

regards, tom lane

Tom Lane wrote:

Tony Caduto <tony_caduto@amsoftwaredesign.com> writes:

If I call it like this:
select * from FIND_USER_SOCKET_BYNAME('juser');
I would expect to get back 1 value with the name of the OUT param
(OUT_SOCKET_ADDRESS).
However it comes back with the name of the function

This is intentional, for compatibility with the pre-existing behavior
with functions in FROM. A function that isn't returning a record is
effectively declared as
FROM foo(...) AS foo(foo)
while a function that does return a record type gives you
FROM foo(...) AS foo(col1, col2)

regards, tom lane

Tom,
Please don't take this the wrong way, but don't you think even if a
single param is declared as OUT it should return the name of the OUT param?

If the function has no OUT params and uses the return keyword it should
return the name of the function, if it has one or many out params it
should return even a single column as the name of the OUT param.

It seems inconsistant that just because I have one OUT param declared it
does not return the name I specified for that param.

Isn't it possible to detect that the function has a single OUT param
declared and if a OUT param is declared return that name?

I am bringing this up because people coming over from Oracle or MS SQL
server will notice something like this.

Thanks,

Tony Caduto

Tony Caduto <tony_caduto@amsoftwaredesign.com> writes:

Please don't take this the wrong way, but don't you think even if a
single param is declared as OUT it should return the name of the OUT param?

Not really, because "create function foo (in x int, out y float)" is
supposed to have the same external behavior as "create function foo
(in x int) returns float". I agree it's a bit of a judgment call, but
I do not see a case for changing it.

regards, tom lane

Tom Lane wrote:

Tony Caduto <tony_caduto@amsoftwaredesign.com> writes:

Please don't take this the wrong way, but don't you think even if a
single param is declared as OUT it should return the name of the OUT param?

Not really, because "create function foo (in x int, out y float)" is
supposed to have the same external behavior as "create function foo
(in x int) returns float". I agree it's a bit of a judgment call, but
I do not see a case for changing it.

I am agreeing with the poster that use of OUT should always print the
out parameter name. Is there a downside to doing that? Seems it gives
people an option.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Tom Lane wrote:

Tony Caduto <tony_caduto@amsoftwaredesign.com> writes:

Please don't take this the wrong way, but don't you think even if a
single param is declared as OUT it should return the name of the OUT param?

Not really, because "create function foo (in x int, out y float)" is
supposed to have the same external behavior as "create function foo
(in x int) returns float". I agree it's a bit of a judgment call, but
I do not see a case for changing it.

regards, tom lane

Hi Tom,
I understand where you are coming from, but I really think it should be
changed because that is how every other DB I know of works with a single
OUT param.

I was recently porting a fairly large application from
Firebird/Interbase and I had a bunch of functions that had one output
param, and in the win32 application that I was also moving over, it was
expecting the name of the OUT param, not the name of the function, So
either I change every single instance of the client code to now use the
function name or I add another dummy OUT param so my app does not have
to be modified.

The biggest reason to change this behavior is for porting from other
Databases so client code does not need to be needlessly modifed.

The new IN/OUT/INOUT params are sweet, and aside from this one issue, it
made porting the Firebird procs super easy.

I know I don't have much pull with development, but I think it should be
changed for the 8.1 release.

Thanks,

Tony

On 9/29/05, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Tony Caduto <tony_caduto@amsoftwaredesign.com> writes:

Please don't take this the wrong way, but don't you think even if a
single param is declared as OUT it should return the name of the OUT param?

Not really, because "create function foo (in x int, out y float)" is
supposed to have the same external behavior as "create function foo
(in x int) returns float". I agree it's a bit of a judgment call, but
I do not see a case for changing it.

Just my $0.02, but that seems inconsistent. In my mind, the
difference between functions with OUT params and functions that return
a RECORD (or a specific rowtype) is syntactic sugar. I'm pretty sure
that this was used to explain the implementation when it was being
discussed, in fact.

Using that logic, a functions with one OUT param would be the same as
a function returning a rowtype with only one column, and the one
column in such a rowtype certainly has a name of it's own.

--
Mike Rylander
mrylander@gmail.com
GPLS -- PINES Development
Database Developer
http://open-ils.org

Mike Rylander <mrylander@gmail.com> writes:

Using that logic, a functions with one OUT param would be the same as
a function returning a rowtype with only one column,

But it's not (and no, I don't want to make it so, because the overhead
for the useless record result would be significant).

regards, tom lane

Tom Lane wrote:

Mike Rylander <mrylander@gmail.com> writes:

Using that logic, a functions with one OUT param would be the same as
a function returning a rowtype with only one column,

But it's not (and no, I don't want to make it so, because the overhead
for the useless record result would be significant).

regards, tom lane

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

Tom,
I hardly think the overhead would be significant on modern processors, I
don't think the majority of users are running on Pentium 90s.( I am
assuming you mean a performance overhead)

The whole point is the current behavior is inconsistent and not expected
and should be changed to be inline with the way other DB systems work.
What is the point of even allowing a single OUT param then? You might
as well just raise a error and tell the user that a single OUT param is
not allowed.
8.1 is going to bring even more users over from systems like Firebird,
MS SQL and even Oracle, and all of these allow a single OUT param and it
returns the name of the OUT param, not the name of the function. Like I
said before this behavior is going to make it more difficult to port
applications from other systems.

How difficult can it be to check if the function has a single OUT param
as compared to the old way of using RETURN?

Sorry if I am being a pain in the you know what, but I really think I am
correct on this matter.

Thanks,

Tony

On Fri, Sep 30, 2005 at 10:20:34AM -0500, Tony Caduto wrote:

Tom,
I hardly think the overhead would be significant on modern processors, I
don't think the majority of users are running on Pentium 90s.( I am
assuming you mean a performance overhead)

Um, please read the documention. Returning a tuple is *significantly*
more expensive than returning a single value. You have to get the tuple
descriptor, allocate memory for the tuple, fill in all the fields with
your data... For a single value you just return it.

See here for all the details, you really don't want to do it if you
don't need to.

http://www.postgresql.org/docs/8.0/interactive/xfunc-c.html#AEN30497

Now, you could fudge the parser to automatically alter the name of the
value in the function but I'm have no idea how hard that would be...
--
Martijn van Oosterhout <kleptog@svana.org> http://svana.org/kleptog/

Martijn van Oosterhout wrote:

On Fri, Sep 30, 2005 at 10:20:34AM -0500, Tony Caduto wrote:

Tom,
I hardly think the overhead would be significant on modern processors, I
don't think the majority of users are running on Pentium 90s.( I am
assuming you mean a performance overhead)

Um, please read the documention. Returning a tuple is *significantly*
more expensive than returning a single value. You have to get the tuple
descriptor, allocate memory for the tuple, fill in all the fields with
your data... For a single value you just return it.

See here for all the details, you really don't want to do it if you
don't need to.

http://www.postgresql.org/docs/8.0/interactive/xfunc-c.html#AEN30497

Now, you could fudge the parser to automatically alter the name of the
value in the function but I'm have no idea how hard that would be...

So you might notice little performance hit bringing back a million rows,
and most of these type of single OUT params functions only return one
row/value anyway.
There would be zero perceivable difference in performance regardless of
the extra overhead for a single value/row.

As a application developer, I don't care about tuples etc, I just want
it to work as expected without having to
resort to hacks like creating a second OUT param that is not used,
otherwise I would have to change a lot of client code where ever the OUT
param is refernced by name instead of position and that is done a lot
because the position is more likely to change than the name.

The bottom line(regardless of any overhead or if I read the docs about
returning a tuple) is that if you have a OUT param it should return that
name, not the name of the function, period.

Thanks,

Tony

So you might notice little performance hit bringing back a million rows,
and most of these type of single OUT params functions only return one
row/value anyway.
There would be zero perceivable difference in performance regardless of
the extra overhead for a single value/row.

Sounds like we need a test case... up for it?

Sincerely,

Joshua D. Drake

--
Your PostgreSQL solutions company - Command Prompt, Inc. 1.800.492.2240
PostgreSQL Replication, Consulting, Custom Programming, 24x7 support
Managed Services, Shared and Dedicated Hosting
Co-Authors: plPHP, plPerlNG - http://www.commandprompt.com/

On Friday 30 September 2005 11:49, Martijn van Oosterhout wrote:

On Fri, Sep 30, 2005 at 10:20:34AM -0500, Tony Caduto wrote:

Tom,
I hardly think the overhead would be significant on modern processors, I
don't think the majority of users are running on Pentium 90s.( I am
assuming you mean a performance overhead)

Um, please read the documention. Returning a tuple is *significantly*
more expensive than returning a single value. You have to get the tuple
descriptor, allocate memory for the tuple, fill in all the fields with
your data... For a single value you just return it.

ISTM it is better for us to be consistent with the visible behavior than to
have two different behaviors for out param functions just so one can be
faster. That way if people are concerned about the speed difference, they
can rewrite the function without an out param... afaict, as it stands now
you've given them no choice and are forcing them to handle two different
scenarios.

--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL

On Fri, Sep 30, 2005 at 10:53:22AM -0700, Joshua D. Drake wrote:

So you might notice little performance hit bringing back a million rows,
and most of these type of single OUT params functions only return one
row/value anyway.
There would be zero perceivable difference in performance regardless of
the extra overhead for a single value/row.

Sounds like we need a test case... up for it?

If there is a performance difference my vote is that we bite the bullet
for 8.1 and accept the performance hit rather than settle for
sub-optimal behavior. Much easier to fix the performance penalty down
the road than to fix the behavior.
--
Jim C. Nasby, Sr. Engineering Consultant jnasby@pervasive.com
Pervasive Software http://pervasive.com work: 512-231-6117
vcard: http://jim.nasby.net/pervasive.vcf cell: 512-569-9461

Jim C. Nasby wrote:

On Fri, Sep 30, 2005 at 10:53:22AM -0700, Joshua D. Drake wrote:

So you might notice little performance hit bringing back a million rows,
and most of these type of single OUT params functions only return one
row/value anyway.
There would be zero perceivable difference in performance regardless of
the extra overhead for a single value/row.

Sounds like we need a test case... up for it?

If there is a performance difference my vote is that we bite the bullet
for 8.1 and accept the performance hit rather than settle for
sub-optimal behavior. Much easier to fix the performance penalty down
the road than to fix the behavior.

Yes, it seems this is the concensus. I have added it to the open items
list:

have a single OUT parameter return the parameter name, not function name

---------------------------------------------------------------------------

PostgreSQL 8.1 Open Items
=========================

Current version at http://candle.pha.pa.us/cgi-bin/pgopenitems or
from http://www.postgresql.org/developer/beta.

Bugs
----
fix pg_dump --clean for roles
fix foreign trigger timing issue
fix ALTER SCHEMA RENAME for sequence name binding, or remove
improve spinlock performance
fix semantic issues of granted permissions in roles
fix pgxs for spaces in file names
have a single OUT parameter return the parameter name, not function name

Questions
---------
cosider O_SYNC as default when O_DIRECT exists
/contrib move to pgfoundry
pgindent?
make sure bitmap scan optimizer settings are reasonable

Documentation
-------------

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On 9/30/05, Jim C. Nasby <jnasby@pervasive.com> wrote:

On Fri, Sep 30, 2005 at 10:53:22AM -0700, Joshua D. Drake wrote:

So you might notice little performance hit bringing back a million rows,
and most of these type of single OUT params functions only return one
row/value anyway.
There would be zero perceivable difference in performance regardless of
the extra overhead for a single value/row.

Sounds like we need a test case... up for it?

If there is a performance difference my vote is that we bite the bullet
for 8.1 and accept the performance hit rather than settle for
sub-optimal behavior. Much easier to fix the performance penalty down
the road than to fix the behavior.

What about just returning the single OUT value named by the parameter,
instead of special casing single-OUT functions? If I understand
correctly, Tom has just added a test to make single-OUT functions look
like RETURNS functions. If that were removed then we'd have what, at
least by counting the responses on this thread, seems to be the
desired (and expected) behaviour.

Or I could just be misunderstanding the implementation again.

--
Mike Rylander
mrylander@gmail.com
GPLS -- PINES Development
Database Developer
http://open-ils.org

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org on behalf of Bruce Momjian
Sent: Sat 10/1/2005 1:16 AM
To: Jim C. Nasby
Cc: Joshua D. Drake; Tony Caduto; pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] Found small issue with OUT params

fix pgxs for spaces in file names

I posted a patch for that.

http://archives.postgresql.org/pgsql-patches/2005-09/msg00137.php

Regards, Dave

Oh, good, I missed that patch.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org on behalf of Bruce Momjian
Sent: Sat 10/1/2005 1:16 AM
To: Jim C. Nasby
Cc: Joshua D. Drake; Tony Caduto; pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] Found small issue with OUT params

fix pgxs for spaces in file names

I posted a patch for that.

http://archives.postgresql.org/pgsql-patches/2005-09/msg00137.php

Regards, Dave

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

[ correcting the completely offtopic Subject ]

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

Before going down that path, I wanted to review the makefiles and see
whether quoting all occurrences of the path variables would be a
workable thing or not. That would be more desirable, if it's not a
maintenance nightmare, because it'd solve the problem everywhere instead
of only for Windows --- and there is a real risk on, say, Darwin.

One idea that comes to mind is to just quote the variables at point of
creation --- that is, write something like

override pkglibdir := "$(pkglibdir)"

in Makefile.global. I haven't tried this to see if it'd work or not ...
any comments? Would single or double quotes be most desirable?

regards, tom lane

Patch applied. Thanks.

---------------------------------------------------------------------------

Bruce Momjian wrote:

Oh, good, I missed that patch.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org on behalf of Bruce Momjian
Sent: Sat 10/1/2005 1:16 AM
To: Jim C. Nasby
Cc: Joshua D. Drake; Tony Caduto; pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] Found small issue with OUT params

fix pgxs for spaces in file names

I posted a patch for that.

http://archives.postgresql.org/pgsql-patches/2005-09/msg00137.php

Regards, Dave

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Index: src/bin/pg_config/pg_config.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_config/pg_config.c,v
retrieving revision 1.13
diff -u -r1.13 pg_config.c
--- src/bin/pg_config/pg_config.c	27 Sep 2005 17:39:33 -0000	1.13
+++ src/bin/pg_config/pg_config.c	28 Sep 2005 12:13:13 -0000
@@ -31,6 +31,63 @@

/*
+ * This function cleans up the paths for use with either cmd.exe or Msys
+ * on Windows. We need them to use double backslashes and filenames without
+ * spaces (for which a short filename is the safest equivalent) eg:
+ * C:\\Progra~1\\
+ *
+ * This can fail in 2 ways - if the path doesn't exist, or short names are
+ * disabled. In the first case, don't return any path. In the second case, 
+ * we leave the path in the long form. In this case, it does still seem to
+ * fix elements containing spaces which is all we actually need.
+ */
+static void
+cleanup_path(char *path)
+{
+#ifdef WIN32
+	int	x=0, y=0;
+	char	temp[MAXPGPATH];
+
+	if (GetShortPathName(path, path, MAXPGPATH - 1) == 0)
+	{
+		/* Ignore ERROR_INVALID_PARAMETER as it almost certainly 
+		 * means that short names are disabled
+		 */
+		if (GetLastError() != ERROR_INVALID_PARAMETER)
+		{
+			path[0] = '\0';
+			return;
+		}
+	}
+		
+
+	/* Replace '\' with '\\'. */
+	for (x = 0; x < strlen(path); x++)
+        {
+		if (path[x] == '/' || path[x] == '\\')
+		{
+			temp[y] = '\\';
+			y++;
+			temp[y] = '\\';
+		}
+		else
+		{
+			temp[y] = path[x];
+		}
+
+		y++;
+
+		/* Bail out if we're too close to MAXPGPATH */
+		if (y >= MAXPGPATH - 2)
+			break;
+	}
+	temp[y] = '\0';
+
+	strncpy(path, temp, MAXPGPATH - 1);
+#endif
+}
+
+
+/*
* For each piece of information known to pg_config, we define a subroutine
* to print it.  This is probably overkill, but it avoids code duplication
* and accidentally omitting items from the "all" display.
@@ -39,138 +96,152 @@
static void
show_bindir(bool all)
{
-	char		path[MAXPGPATH];
-	char	   *lastsep;
+	char	path[MAXPGPATH];
+	char	*lastsep;

if (all)
printf("BINDIR = ");
/* assume we are located in the bindir */
strcpy(path, mypath);
lastsep = strrchr(path, '/');
+
if (lastsep)
*lastsep = '\0';
+
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_docdir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("DOCDIR = ");
get_doc_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_includedir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("INCLUDEDIR = ");
get_include_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_pkgincludedir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("PKGINCLUDEDIR = ");
get_pkginclude_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_includedir_server(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("INCLUDEDIR-SERVER = ");
get_includeserver_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_libdir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("LIBDIR = ");
get_lib_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_pkglibdir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("PKGLIBDIR = ");
get_pkglib_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_localedir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("LOCALEDIR = ");
get_locale_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_mandir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("MANDIR = ");
get_man_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_sharedir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("SHAREDIR = ");
get_share_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_sysconfdir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("SYSCONFDIR = ");
get_etc_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_pgxs(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("PGXS = ");
get_pkglib_path(mypath, path);
strncat(path, "/pgxs/src/makefiles/pgxs.mk", MAXPGPATH - 1);
+ cleanup_path(path);
printf("%s\n", path);
}

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sorry, this is the same patch with the proper subject line.

---------------------------------------------------------------------------

Bruce Momjian wrote:

Patch applied. Thanks.

---------------------------------------------------------------------------

Bruce Momjian wrote:

Oh, good, I missed that patch.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

---------------------------------------------------------------------------

-----Original Message-----
From: pgsql-hackers-owner@postgresql.org on behalf of Bruce Momjian
Sent: Sat 10/1/2005 1:16 AM
To: Jim C. Nasby
Cc: Joshua D. Drake; Tony Caduto; pgsql-hackers@postgresql.org
Subject: Re: [HACKERS] Found small issue with OUT params

fix pgxs for spaces in file names

I posted a patch for that.

http://archives.postgresql.org/pgsql-patches/2005-09/msg00137.php

Regards, Dave

---------------------------(end of broadcast)---------------------------
TIP 4: Have you searched our list archives?

http://archives.postgresql.org

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Index: src/bin/pg_config/pg_config.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_config/pg_config.c,v
retrieving revision 1.13
diff -u -r1.13 pg_config.c
--- src/bin/pg_config/pg_config.c	27 Sep 2005 17:39:33 -0000	1.13
+++ src/bin/pg_config/pg_config.c	28 Sep 2005 12:13:13 -0000
@@ -31,6 +31,63 @@

/*
+ * This function cleans up the paths for use with either cmd.exe or Msys
+ * on Windows. We need them to use double backslashes and filenames without
+ * spaces (for which a short filename is the safest equivalent) eg:
+ * C:\\Progra~1\\
+ *
+ * This can fail in 2 ways - if the path doesn't exist, or short names are
+ * disabled. In the first case, don't return any path. In the second case, 
+ * we leave the path in the long form. In this case, it does still seem to
+ * fix elements containing spaces which is all we actually need.
+ */
+static void
+cleanup_path(char *path)
+{
+#ifdef WIN32
+	int	x=0, y=0;
+	char	temp[MAXPGPATH];
+
+	if (GetShortPathName(path, path, MAXPGPATH - 1) == 0)
+	{
+		/* Ignore ERROR_INVALID_PARAMETER as it almost certainly 
+		 * means that short names are disabled
+		 */
+		if (GetLastError() != ERROR_INVALID_PARAMETER)
+		{
+			path[0] = '\0';
+			return;
+		}
+	}
+		
+
+	/* Replace '\' with '\\'. */
+	for (x = 0; x < strlen(path); x++)
+        {
+		if (path[x] == '/' || path[x] == '\\')
+		{
+			temp[y] = '\\';
+			y++;
+			temp[y] = '\\';
+		}
+		else
+		{
+			temp[y] = path[x];
+		}
+
+		y++;
+
+		/* Bail out if we're too close to MAXPGPATH */
+		if (y >= MAXPGPATH - 2)
+			break;
+	}
+	temp[y] = '\0';
+
+	strncpy(path, temp, MAXPGPATH - 1);
+#endif
+}
+
+
+/*
* For each piece of information known to pg_config, we define a subroutine
* to print it.  This is probably overkill, but it avoids code duplication
* and accidentally omitting items from the "all" display.
@@ -39,138 +96,152 @@
static void
show_bindir(bool all)
{
-	char		path[MAXPGPATH];
-	char	   *lastsep;
+	char	path[MAXPGPATH];
+	char	*lastsep;

if (all)
printf("BINDIR = ");
/* assume we are located in the bindir */
strcpy(path, mypath);
lastsep = strrchr(path, '/');
+
if (lastsep)
*lastsep = '\0';
+
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_docdir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("DOCDIR = ");
get_doc_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_includedir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("INCLUDEDIR = ");
get_include_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_pkgincludedir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("PKGINCLUDEDIR = ");
get_pkginclude_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_includedir_server(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("INCLUDEDIR-SERVER = ");
get_includeserver_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_libdir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("LIBDIR = ");
get_lib_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_pkglibdir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("PKGLIBDIR = ");
get_pkglib_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_localedir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("LOCALEDIR = ");
get_locale_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_mandir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("MANDIR = ");
get_man_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_sharedir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("SHAREDIR = ");
get_share_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_sysconfdir(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("SYSCONFDIR = ");
get_etc_path(mypath, path);
+ cleanup_path(path);
printf("%s\n", path);
}

static void
show_pgxs(bool all)
{
- char path[MAXPGPATH];
+ char path[MAXPGPATH];

if (all)
printf("PGXS = ");
get_pkglib_path(mypath, path);
strncat(path, "/pgxs/src/makefiles/pgxs.mk", MAXPGPATH - 1);
+ cleanup_path(path);
printf("%s\n", path);
}

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

-- 
Bruce Momjian                        |  http://candle.pha.pa.us
pgman@candle.pha.pa.us               |  (610) 359-1001
+  If your life is a hard drive,     |  13 Roberts Road
+  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Sorry, this is the same patch with the proper subject line.

Why does this patch convert '\' to '\\' and not to '/' ?
AFAICS that does nothing except to make the code more fragile.

regards, tom lane