Horton

Thread

Commits

  1. Upgrade documentation connected with shared_preload_libraries et al.

  2. Don't downcase entries within shared_preload_libraries et al.

  1. PATCH: Don't downcase filepath/filename while loading libraries

    QL Zhuo <zhuo.dev@gmail.com> — 2017-06-16T02:04:12Z

    I just put this line in my postgresql.conf:

```
shared_preload_libraries = '/Path/Contains/UpCaseWords/an_ext.so'
```

Then the server couldn't start. It tried to load the file
"/path/contains/upcasewords/an_ext.so" and failed.

After few digging, I found there's a wrong use of `SplitIdentifierString`
in function `load_libraries` in /src/backend/utils/init/miscinit.c, and the
attached patch fixes it.


--
This email address (zhuo.dev<at>gmail.com) is only for development affairs,
e.g. mail list, please mail to zhuo<at>hexoasis.com or zhuoql<at>zoho.com
for other purpose.

ZHUO QL (KDr2), http://kdr2.com

  2. Re: PATCH: Don't downcase filepath/filename while loading libraries

    Michael Paquier <michael.paquier@gmail.com> — 2017-06-16T02:34:34Z

    On Fri, Jun 16, 2017 at 11:04 AM, QL Zhuo <zhuo.dev@gmail.com> wrote:
> I just put this line in my postgresql.conf:
>
> ```
> shared_preload_libraries = '/Path/Contains/UpCaseWords/an_ext.so'
> ```
>
> Then the server couldn't start. It tried to load the file
> "/path/contains/upcasewords/an_ext.so" and failed.
>
> After few digging, I found there's a wrong use of `SplitIdentifierString` in
> function `load_libraries` in /src/backend/utils/init/miscinit.c, and the
> attached patch fixes it.

That's a good catch. All the other callers of SplitIdentifierString()
don't handle a list of directories. This requires a back-patch.
-- 
Michael

  3. Re: PATCH: Don't downcase filepath/filename while loading libraries

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-16T04:01:02Z

    Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, Jun 16, 2017 at 11:04 AM, QL Zhuo <zhuo.dev@gmail.com> wrote:
>> After few digging, I found there's a wrong use of `SplitIdentifierString` in
>> function `load_libraries` in /src/backend/utils/init/miscinit.c, and the
>> attached patch fixes it.

> That's a good catch. All the other callers of SplitIdentifierString()
> don't handle a list of directories. This requires a back-patch.

(1) As is, I think the patch leaks memory.  SplitDirectoriesString's
API is not identical to SplitIdentifierString's.

(2) My inclination would be not to back-patch.  This change could break
configurations that worked before, and the lack of prior complaints
says that not many people are having a problem with it.

			regards, tom lane

  4. Re: PATCH: Don't downcase filepath/filename while loading libraries

    Michael Paquier <michael.paquier@gmail.com> — 2017-06-16T04:18:01Z

    On Fri, Jun 16, 2017 at 1:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> (2) My inclination would be not to back-patch.  This change could break
> configurations that worked before, and the lack of prior complaints
> says that not many people are having a problem with it.

That's fourteen years without complains, still I cannot imagine any
cases where it would be a problem as people who would have faced this
problem but not reported it have likely just enforced the FS to handle
case-insensitivity for paths. Or they just relied on the default: on
Windows the default is to be case-insensitive, as much as OSX. So the
proposed patch handles things better with case-sensitive paths,
without really impacting users with case-insensive FS.

I see the point of not back-patching the change though.
-- 
Michael

  5. Re: PATCH: Don't downcase filepath/filename while loading libraries

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-16T04:27:33Z

    Michael Paquier <michael.paquier@gmail.com> writes:
> On Fri, Jun 16, 2017 at 1:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> (2) My inclination would be not to back-patch.  This change could break
>> configurations that worked before, and the lack of prior complaints
>> says that not many people are having a problem with it.

> That's fourteen years without complains, still I cannot imagine any
> cases where it would be a problem as people who would have faced this
> problem but not reported it have likely just enforced the FS to handle
> case-insensitivity for paths.

Well, it's not just about case sensitivity.  The comment for
SplitDirectoriesString points out that it deals with embedded spaces
differently, and it also applies canonicalize_path().  I'm too tired
to think hard about what that part might mean for compatibility, but
it probably isn't nothing.

Anyway, I agree that this is an appropriate change for HEAD.  Just
not convinced that we should shove it into minor releases.

			regards, tom lane

  6. PATCH: Don't downcase filepath/filename while loading libraries

    David G. Johnston <david.g.johnston@gmail.com> — 2017-06-16T04:39:39Z

    On Thursday, June 15, 2017, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michael Paquier <michael.paquier@gmail.com> writes:
> > On Fri, Jun 16, 2017 at 1:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> (2) My inclination would be not to back-patch.  This change could break
> >> configurations that worked before, and the lack of prior complaints
> >> says that not many people are having a problem with it.
>
> > That's fourteen years without complains, still I cannot imagine any
> > cases where it would be a problem as people who would have faced this
> > problem but not reported it have likely just enforced the FS to handle
> > case-insensitivity for paths.
>
> Well, it's not just about case sensitivity.  The comment for
> SplitDirectoriesString points out that it deals with embedded spaces
> differently, and it also applies canonicalize_path().  I'm too tired
> to think hard about what that part might mean for compatibility, but
> it probably isn't nothing.
>
> Anyway, I agree that this is an appropriate change for HEAD.  Just
> not convinced that we should shove it into minor releases.
>
>
If it's downcasing then careless use of mixed case when the true path on a
case sensitive filesystem is all lower case (not unlikely at all given
default packaging for deb and rpm packages, iirc) would indeed be a
problem.  Those paths are accidentally working right now.  I vote for no
back-patch.

David J.

  7. Re: PATCH: Don't downcase filepath/filename while loading libraries

    QL Zhuo <zhuo.dev@gmail.com> — 2017-06-16T04:43:57Z

    "That's fourteen years without complains", so maybe not to back-patch is
good choice, until someone complains this :)

And, the attached new patch fixes the memory leaks.

--
This email address (zhuo.dev<at>gmail.com) is only for development affairs,
e.g. mail list, please mail to zhuo<at>hexoasis.com or zhuoql<at>zoho.com
for other purpose.

ZHUO QL (KDr2), http://kdr2.com

On Fri, Jun 16, 2017 at 12:27 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michael Paquier <michael.paquier@gmail.com> writes:
> > On Fri, Jun 16, 2017 at 1:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> (2) My inclination would be not to back-patch.  This change could break
> >> configurations that worked before, and the lack of prior complaints
> >> says that not many people are having a problem with it.
>
> > That's fourteen years without complains, still I cannot imagine any
> > cases where it would be a problem as people who would have faced this
> > problem but not reported it have likely just enforced the FS to handle
> > case-insensitivity for paths.
>
> Well, it's not just about case sensitivity.  The comment for
> SplitDirectoriesString points out that it deals with embedded spaces
> differently, and it also applies canonicalize_path().  I'm too tired
> to think hard about what that part might mean for compatibility, but
> it probably isn't nothing.
>
> Anyway, I agree that this is an appropriate change for HEAD.  Just
> not convinced that we should shove it into minor releases.
>
>                         regards, tom lane
>

  8. Re: PATCH: Don't downcase filepath/filename while loading libraries

    Tom Lane <tgl@sss.pgh.pa.us> — 2017-06-20T17:43:53Z

    QL Zhuo <zhuo.dev@gmail.com> writes:
> And, the attached new patch fixes the memory leaks.

Pushed with minor adjustments --- mostly, getting rid of the now-redundant
canonicalize_path() call.  I also updated the documentation.

I notice the documentation formerly said "All library names are converted
to lower case unless double-quoted", which means that formally this isn't
a bug at all, but operating-as-designed-and-documented.  Still, I agree
it's an improvement.

			regards, tom lane