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.
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
Attachments:
0001-Don-t-downcase-filepath-in-load_libraries.patchapplication/octet-stream; name=0001-Don-t-downcase-filepath-in-load_libraries.patchDownload
From a45dbc453fe501c8143b6f7d74541d2f3e3f8e9d Mon Sep 17 00:00:00 2001
From: KDr2 <zhuo.dev@gmail.com>
Date: Thu, 15 Jun 2017 06:28:29 +0800
Subject: [PATCH] Don't downcase filepath in load_libraries
---
src/backend/utils/init/miscinit.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 8d149bf..ff11650 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1436,7 +1436,7 @@ load_libraries(const char *libraries, const char *gucname, bool restricted)
rawstring = pstrdup(libraries);
/* Parse string into list of identifiers */
- if (!SplitIdentifierString(rawstring, ',', &elemlist))
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
{
/* syntax error in list */
pfree(rawstring);
--
2.1.4
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
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:
Show quoted text
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
Attachments:
0001-Don-t-downcase-filepath-in-load_libraries.patchapplication/octet-stream; name=0001-Don-t-downcase-filepath-in-load_libraries.patchDownload
From 057eaa10ce6ce1c79afc9be38f54e5fe1f350eed Mon Sep 17 00:00:00 2001
From: KDr2 <zhuo.dev@gmail.com>
Date: Thu, 15 Jun 2017 06:28:29 +0800
Subject: [PATCH] Don't downcase filepath in load_libraries
---
src/backend/utils/init/miscinit.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 8d149bf..d637af3 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1436,11 +1436,11 @@ load_libraries(const char *libraries, const char *gucname, bool restricted)
rawstring = pstrdup(libraries);
/* Parse string into list of identifiers */
- if (!SplitIdentifierString(rawstring, ',', &elemlist))
+ if (!SplitDirectoriesString(rawstring, ',', &elemlist))
{
/* syntax error in list */
pfree(rawstring);
- list_free(elemlist);
+ list_free_deep(elemlist);
ereport(LOG,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("invalid list syntax in parameter \"%s\"",
@@ -1471,7 +1471,7 @@ load_libraries(const char *libraries, const char *gucname, bool restricted)
}
pfree(rawstring);
- list_free(elemlist);
+ list_free_deep(elemlist);
}
/*
--
2.1.4
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers