Обсуждение: [HACKERS] PATCH: Don't downcase filepath/filename while loading libraries
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.```
shared_preload_libraries = '/Path/Contains/UpCaseWords/an_ext.so'
```
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
Вложения
Re: [HACKERS] PATCH: Don't downcase filepath/filename while loading libraries
От
Michael Paquier
Дата:
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
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
Re: [HACKERS] PATCH: Don't downcase filepath/filename while loading libraries
От
Michael Paquier
Дата:
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
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
[HACKERS] PATCH: Don't downcase filepath/filename while loading libraries
От
"David G. Johnston"
Дата:
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.
"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.
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
Вложения
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