Path separator

I've seen a couple of reports that the new SSL error messages on windows
look strange with paths the wrong way. For example:

root certificate file "C:\Documents and Settings\<SNIP>\Application
Data/postgresql/root.crt" does not exist.

The issue being the mix of forward and backwards slashes. Attached patch
should fix this.

Is this worth doing? Comments?

//Magnus

On Wed, Mar 18, 2009 at 9:24 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> I've seen a couple of reports that the new SSL error messages on windows
> look strange with paths the wrong way. For example:
>
> root certificate file "C:\Documents and Settings\<SNIP>\Application
> Data/postgresql/root.crt" does not exist.
>
> The issue being the mix of forward and backwards slashes. Attached patch
> should fix this.
>
> Is this worth doing? Comments?

Yes. I've seen a couple of complaints as well, and both thought the
error was actually the mixed slashes causing the problem, and *not*
the missing root.crt.

--
Dave Page
EnterpriseDB UK: http://www.enterprisedb.com

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> I've seen a couple of reports that the new SSL error messages on windows
> look strange with paths the wrong way. For example:

> root certificate file "C:\Documents and Settings\<SNIP>\Application
> Data/postgresql/root.crt" does not exist.

> The issue being the mix of forward and backwards slashes. Attached patch
> should fix this.

> Is this worth doing? Comments?

In view of the way that canonicalize_path() works, I can't help thinking
this is going in precisely the wrong direction.

Also, don't we already have a macro someplace for the platform's
preferred path separator?

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> I've seen a couple of reports that the new SSL error messages on windows
>> look strange with paths the wrong way. For example:
>
>> root certificate file "C:\Documents and Settings\<SNIP>\Application
>> Data/postgresql/root.crt" does not exist.
>
>> The issue being the mix of forward and backwards slashes. Attached patch
>> should fix this.
>
>> Is this worth doing? Comments?
>
> In view of the way that canonicalize_path() works, I can't help thinking
> this is going in precisely the wrong direction.

In a way, yes. But canonicalize_path() runs only in the backend, and
this is only in the frontend. I think the requirements on the frontend
are slightly different than the backend.

But the most important thing is to be consistent within the same path as
we report it I think, so we could switch all to forward slashes as well
if you think that's better.

>
> Also, don't we already have a macro someplace for the platform's
> preferred path separator?

I looked for a macro for it, didn't find it. It seems to be hardcoded.
We have macros for SYSTEM_QUOTE for example, but not for the path
separator AFAICF.

I just realized we have a make_native_path() function, I had completely
missed that one. So we could possibly use that instead. In the end it
does the same thing

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> In view of the way that canonicalize_path() works, I can't help thinking
>> this is going in precisely the wrong direction.

> In a way, yes. But canonicalize_path() runs only in the backend, and
> this is only in the frontend. I think the requirements on the frontend
> are slightly different than the backend.

Just for the record, canonicalize_path does work in the frontend;
we have uses of it in psql and pg_ctl. But we have previously decided
not to import path.c into libpq, which is where the present issue is,
so yes there is a problem with using it. The same objection applies to
make_native_path unfortunately.

> But the most important thing is to be consistent within the same path as
> we report it I think, so we could switch all to forward slashes as well
> if you think that's better.

What I'm concerned about is the prospect that if we do this here,
we're going to end up trying to do it all over the frontend code.
(And I'm not entirely convinced that it doesn't then propagate into
the backend, too, but even just the frontend code is bad enough.)

> I just realized we have a make_native_path() function, I had completely
> missed that one. So we could possibly use that instead. In the end it
> does the same thing

I'd definitely favor using make_native_path over hand-rolled code.
But I guess what I'm suggesting is that it'd be more consistent with
our previous choices to apply canonicalize_path instead.

The major stumbling block to doing either thing is not wishing to import
path.c into libpq. I think that the objection was partially code size
and partially namespace pollution (path.c doesn't use pg_ or similar
prefixes on its exported names). The latter is no longer a problem on
platforms that support exported-name filtering, but that isn't all of
them. Could we consider splitting path.c into two parts, that which we
want in libpq and that which we don't?

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Tom Lane wrote:
>>> In view of the way that canonicalize_path() works, I can't help thinking
>>> this is going in precisely the wrong direction.
>
>> In a way, yes. But canonicalize_path() runs only in the backend, and
>> this is only in the frontend. I think the requirements on the frontend
>> are slightly different than the backend.
>
> Just for the record, canonicalize_path does work in the frontend;
> we have uses of it in psql and pg_ctl. But we have previously decided
> not to import path.c into libpq, which is where the present issue is,
> so yes there is a problem with using it. The same objection applies to
> make_native_path unfortunately.

That's what I meant. And yeah, that's true about make_native_path as well.

Should we reconsider this decision?

>> But the most important thing is to be consistent within the same path as
>> we report it I think, so we could switch all to forward slashes as well
>> if you think that's better.
>
> What I'm concerned about is the prospect that if we do this here,
> we're going to end up trying to do it all over the frontend code.
> (And I'm not entirely convinced that it doesn't then propagate into
> the backend, too, but even just the frontend code is bad enough.)

True.

>> I just realized we have a make_native_path() function, I had completely
>> missed that one. So we could possibly use that instead. In the end it
>> does the same thing
>
> I'd definitely favor using make_native_path over hand-rolled code.
> But I guess what I'm suggesting is that it'd be more consistent with
> our previous choices to apply canonicalize_path instead.

I agree that this is probably the best way to do it.

> The major stumbling block to doing either thing is not wishing to import
> path.c into libpq. I think that the objection was partially code size
> and partially namespace pollution (path.c doesn't use pg_ or similar
> prefixes on its exported names). The latter is no longer a problem on
> platforms that support exported-name filtering, but that isn't all of
> them. Could we consider splitting path.c into two parts, that which we
> want in libpq and that which we don't?

On my system (linux), path.o is 5k. libpq.so is 157k. Is adding that
size *really* a problem?

Also, is there a chance that the linker is smart enough to actually
remove the parts of path.o that aren't used in libpq completely, if it's
not exported at all? (if the size does matter)

If it is, sure, we could split it apart. But fairly large parts of it
would be required by both. But I guess the number of symbols would be
quite a bit smaller.

Is it worth taking a look at exactly what the sizes end up being?
Shouldn't be all that much work...

//Magnus

Magnus Hagander wrote:
>> The major stumbling block to doing either thing is not wishing to import
>> path.c into libpq. I think that the objection was partially code size
>> and partially namespace pollution (path.c doesn't use pg_ or similar
>> prefixes on its exported names). The latter is no longer a problem on
>> platforms that support exported-name filtering, but that isn't all of
>> them. Could we consider splitting path.c into two parts, that which we
>> want in libpq and that which we don't?
>
> On my system (linux), path.o is 5k. libpq.so is 157k. Is adding that
> size *really* a problem?
>
> Also, is there a chance that the linker is smart enough to actually
> remove the parts of path.o that aren't used in libpq completely, if it's
> not exported at all? (if the size does matter)
>
> If it is, sure, we could split it apart. But fairly large parts of it
> would be required by both. But I guess the number of symbols would be
> quite a bit smaller.

Answering myself here: the filesize for the "frontend only" part is
about 2k on this system.

//Magnus

Magnus Hagander wrote:
> Magnus Hagander wrote:
>>> The major stumbling block to doing either thing is not wishing to import
>>> path.c into libpq. I think that the objection was partially code size
>>> and partially namespace pollution (path.c doesn't use pg_ or similar
>>> prefixes on its exported names). The latter is no longer a problem on
>>> platforms that support exported-name filtering, but that isn't all of
>>> them. Could we consider splitting path.c into two parts, that which we
>>> want in libpq and that which we don't?
>> On my system (linux), path.o is 5k. libpq.so is 157k. Is adding that
>> size *really* a problem?
>>
>> Also, is there a chance that the linker is smart enough to actually
>> remove the parts of path.o that aren't used in libpq completely, if it's
>> not exported at all? (if the size does matter)
>>
>> If it is, sure, we could split it apart. But fairly large parts of it
>> would be required by both. But I guess the number of symbols would be
>> quite a bit smaller.
>
> Answering myself here: the filesize for the "frontend only" part is
> about 2k on this system.

Long meeting, time for coding.. :-) Here's a rough patch. Is this about
what you had in mind?

//Magnus

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Tom Lane wrote:
>> The major stumbling block to doing either thing is not wishing to import
>> path.c into libpq. I think that the objection was partially code size
>> and partially namespace pollution (path.c doesn't use pg_ or similar
>> prefixes on its exported names). The latter is no longer a problem on
>> platforms that support exported-name filtering, but that isn't all of
>> them. Could we consider splitting path.c into two parts, that which we
>> want in libpq and that which we don't?

> On my system (linux), path.o is 5k. libpq.so is 157k. Is adding that
> size *really* a problem?

I'm more worried about the external dependencies pulled in by the
path-discovery stuff (geteuid for instance). None of that is code
that libpq needs at all.

> Also, is there a chance that the linker is smart enough to actually
> remove the parts of path.o that aren't used in libpq completely, if it's
> not exported at all? (if the size does matter)

The normal expectation is that .o files are the unit of linking. There
might be a platform or two that is smarter, but they are not the norm.

Since I'm the one who's hot about this, I'm willing to do the work.
Do you agree that canonicalize_path and make_native_path are all that
we want to push into libpq for now? If so, I'll rename them to
pg_..._path and push them into a separate source file.

regards, tom lane

I wrote:
> Since I'm the one who's hot about this, I'm willing to do the work.

Belay that ... I'll review your patch instead, later today sometime.

regards, tom lane

Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Answering myself here: the filesize for the "frontend only" part is
>> about 2k on this system.

> Long meeting, time for coding.. :-) Here's a rough patch. Is this about
> what you had in mind?

Hm, this seems to make the namespace pollution problem worse not better,
because of de-staticizing so many functions. I guess we could stick pg_
prefixes on all of them. That's a bit ugly; anybody have a better idea?

It might be that it'd be better to push a couple more of the simple
path-munging functions (like join_path_components) over into the new
file, so as to have a more logical division of responsibilities.
In my mind the two key areas here are "path syntax knowledge" and
"extracting absolute paths from environmental information". The second
part seems to be the one that doesn't belong on the frontend side.

regards, tom lane

Tom Lane wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> Answering myself here: the filesize for the "frontend only" part is
>>> about 2k on this system.
>
>> Long meeting, time for coding.. :-) Here's a rough patch. Is this about
>> what you had in mind?
>
> Hm, this seems to make the namespace pollution problem worse not better,
> because of de-staticizing so many functions. I guess we could stick pg_
> prefixes on all of them. That's a bit ugly; anybody have a better idea?

Not really.

> It might be that it'd be better to push a couple more of the simple
> path-munging functions (like join_path_components) over into the new
> file, so as to have a more logical division of responsibilities.
> In my mind the two key areas here are "path syntax knowledge" and
> "extracting absolute paths from environmental information". The second
> part seems to be the one that doesn't belong on the frontend side.

What would be the gain there? To be able to re-static-ify for example
skip_drive? Or just a nicer division?

We should probably also consider moving get_home_path() over to the
frontend one, and get rid of the copy that's in fe-connect.c.

//Magnus

I assume we never came to any conclusion on this.

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

Magnus Hagander wrote:
> Tom Lane wrote:
> > Magnus Hagander <magnus(at)hagander(dot)net> writes:
> >>> Answering myself here: the filesize for the "frontend only" part is
> >>> about 2k on this system.
> >
> >> Long meeting, time for coding.. :-) Here's a rough patch. Is this about
> >> what you had in mind?
> >
> > Hm, this seems to make the namespace pollution problem worse not better,
> > because of de-staticizing so many functions. I guess we could stick pg_
> > prefixes on all of them. That's a bit ugly; anybody have a better idea?
>
> Not really.
>
>
> > It might be that it'd be better to push a couple more of the simple
> > path-munging functions (like join_path_components) over into the new
> > file, so as to have a more logical division of responsibilities.
> > In my mind the two key areas here are "path syntax knowledge" and
> > "extracting absolute paths from environmental information". The second
> > part seems to be the one that doesn't belong on the frontend side.
>
> What would be the gain there? To be able to re-static-ify for example
> skip_drive? Or just a nicer division?
>
> We should probably also consider moving get_home_path() over to the
> frontend one, and get rid of the copy that's in fe-connect.c.
>
>
> //Magnus
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com
PG East: http://www.enterprisedb.com/community/nav-pg-east-2010.do
+ If your life is a hard drive, Christ can be your backup. +

yeah, we have a couple of patches that aren't good... And I kind of
lost track of it waiting for feedback on my last question.

It's not a must-fix, but it'd be good to get better messages eventually.

//Magnus

2010/2/26 Bruce Momjian <bruce(at)momjian(dot)us>:
>
> I assume we never came to any conclusion on this.
>
> ---------------------------------------------------------------------------
>
> Magnus Hagander wrote:
>> Tom Lane wrote:
>> > Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> >>> Answering myself here: the filesize for the "frontend only" part is
>> >>> about 2k on this system.
>> >
>> >> Long meeting, time for coding.. :-) Here's a rough patch. Is this about
>> >> what you had in mind?
>> >
>> > Hm, this seems to make the namespace pollution problem worse not better,
>> > because of de-staticizing so many functions.  I guess we could stick pg_
>> > prefixes on all of them.  That's a bit ugly; anybody have a better idea?
>>
>> Not really.
>>
>>
>> > It might be that it'd be better to push a couple more of the simple
>> > path-munging functions (like join_path_components) over into the new
>> > file, so as to have a more logical division of responsibilities.
>> > In my mind the two key areas here are "path syntax knowledge" and
>> > "extracting absolute paths from environmental information".  The second
>> > part seems to be the one that doesn't belong on the frontend side.
>>
>> What would be the gain there? To be able to re-static-ify for example
>> skip_drive? Or just a nicer division?
>>
>> We should probably also consider moving get_home_path() over to the
>> frontend one, and get rid of the copy that's in fe-connect.c.
>>
>>
>> //Magnus
>>
>>
>> --
>> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
>> To make changes to your subscription:
>> http://www.postgresql.org/mailpref/pgsql-hackers
>
> --
>  Bruce Momjian  <bruce(at)momjian(dot)us>        http://momjian.us
>  EnterpriseDB                             http://enterprisedb.com
>  PG East:  http://www.enterprisedb.com/community/nav-pg-east-2010.do
>  + If your life is a hard drive, Christ can be your backup. +
>

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/