Message ID | 20191128111542.5132-1-p.debruijn@unilogic.nl |
---|---|
State | Rejected |
Headers | show |
Series | package/postgresql: fix filesystem naming consistency | expand |
On Thu, 28 Nov 2019 12:15:42 +0100 Pascal de Bruijn <p.debruijn@unilogic.nl> wrote: > Currently the service is called postgresql, but other filesystem > references are called pgsql, which is inconsistent and confusing. > > Given that at least Debian uses postgresql in the filesystems > as well I would suggest moving the filesystem reference to align > with the service name as opposed to the other way around. > > Signed-off-by: Pascal de Bruijn <p.debruijn@unilogic.nl> > --- > package/postgresql/Config.in | 2 +- > package/postgresql/S50postgresql | 8 ++++---- > package/postgresql/postgresql.mk | 4 ++-- > package/postgresql/postgresql.service | 6 +++--- > 4 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/package/postgresql/Config.in b/package/postgresql/Config.in > index e548d3c..2f677da 100644 > --- a/package/postgresql/Config.in > +++ b/package/postgresql/Config.in > @@ -11,7 +11,7 @@ config BR2_PACKAGE_POSTGRESQL > database system. > > Enable the readline package to gain readline support in > - pgsql (the command line interpreter), which offers > + psql (the command line interpreter), which offers This doesn't seem to be related to the commit. Also, is the command line tool really called psql ? Thanks! Thomas
From: Thomas Petazzoni <thomas.petazzoni@bootlin.com> To: Pascal de Bruijn <p.debruijn@unilogic.nl> Cc: <buildroot@busybox.net> Sent: 11/28/2019 4:36 PM Subject: Re: [Buildroot] [PATCH] package/postgresql: fix filesystem naming consistency On Thu, 28 Nov 2019 12:15:42 +0100 Pascal de Bruijn <p.debruijn@unilogic.nl> wrote: > Currently the service is called postgresql, but other filesystem > references are called pgsql, which is inconsistent and confusing. > > Given that at least Debian uses postgresql in the filesystems > as well I would suggest moving the filesystem reference to align > with the service name as opposed to the other way around. > > Signed-off-by: Pascal de Bruijn <p.debruijn@unilogic.nl> > --- > package/postgresql/Config.in | 2 +- > package/postgresql/S50postgresql | 8 ++++---- > package/postgresql/postgresql.mk | 4 ++-- > package/postgresql/postgresql.service | 6 +++--- > 4 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/package/postgresql/Config.in b/package/postgresql/Config.in > index e548d3c..2f677da 100644 > --- a/package/postgresql/Config.in > +++ b/package/postgresql/Config.in > @@ -11,7 +11,7 @@ config BR2_PACKAGE_POSTGRESQL > database system. > > Enable the readline package to gain readline support in > - pgsql (the command line interpreter), which offers > + psql (the command line interpreter), which offers This doesn't seem to be related to the commit. Also, is the command line tool really called psql ? Yes, it's not directly related, but just a minor typo fix. The command is indeed called psql. Regards, Pascal de Bruijn
>>>>> "Pascal" == Pascal de Bruijn <p.debruijn@unilogic.nl> writes: Hi, >> - pgsql (the command line interpreter), which offers >> + psql (the command line interpreter), which offers > This doesn't seem to be related to the commit. Also, is the command > line tool really called psql ? > Yes, it's not directly related, but just a minor typo fix. > The command is indeed called psql. Ok, could you send a separate patch just for that typo so we can apply it to master? The path change looks like next material to me.
Hello Pascal, On Thu, 28 Nov 2019 12:15:42 +0100, Pascal de Bruijn <p.debruijn@unilogic.nl> wrote: > Currently the service is called postgresql, but other filesystem > references are called pgsql, which is inconsistent and confusing. > > Given that at least Debian uses postgresql in the filesystems > as well I would suggest moving the filesystem reference to align > with the service name as opposed to the other way around. > > Signed-off-by: Pascal de Bruijn <p.debruijn@unilogic.nl> > --- > package/postgresql/Config.in | 2 +- > package/postgresql/S50postgresql | 8 ++++---- > package/postgresql/postgresql.mk | 4 ++-- > package/postgresql/postgresql.service | 6 +++--- > 4 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/package/postgresql/Config.in b/package/postgresql/Config.in > index e548d3c..2f677da 100644 > --- a/package/postgresql/Config.in > +++ b/package/postgresql/Config.in > @@ -11,7 +11,7 @@ config BR2_PACKAGE_POSTGRESQL > database system. > > Enable the readline package to gain readline support in > - pgsql (the command line interpreter), which offers > + psql (the command line interpreter), which offers +1 for fixing this typo (which dates back to my initial submission from 2014) in a separate patch... > command history and edit functions. > > Enable the zlib package to gain builtin compression for > diff --git a/package/postgresql/S50postgresql b/package/postgresql/S50postgresql > index 1ece4fc..8d7ad34 100644 > --- a/package/postgresql/S50postgresql > +++ b/package/postgresql/S50postgresql > @@ -5,20 +5,20 @@ > > umask 077 > > -if [ ! -f /var/lib/pgsql/PG_VERSION ]; then > +if [ ! -f /var/lib/postgresql/PG_VERSION ]; then > echo "Initializing postgresql data base..." > - su - postgres -c '/usr/bin/pg_ctl initdb -D /var/lib/pgsql' > + su - postgres -c '/usr/bin/pg_ctl initdb -D /var/lib/postgresql' -1 for this change as /var/lib/pgsql is the suggested/common default location, see e.g. [1], and is saving 5 characters at many location from the script ;-) Regards, Peter [1] https://www.postgresql.org/docs/12/storage-file-layout.html > echo "done" > fi > > start() { > printf "Starting postgresql: " > - su - postgres -c '/usr/bin/pg_ctl start -w -D /var/lib/pgsql -l logfile' > + su - postgres -c '/usr/bin/pg_ctl start -w -D /var/lib/postgresql -l logfile' > echo "OK" > } > stop() { > printf "Stopping postgresql: " > - su - postgres -c '/usr/bin/pg_ctl stop -D /var/lib/pgsql -m fast' > + su - postgres -c '/usr/bin/pg_ctl stop -D /var/lib/postgresql -m fast' > echo "OK" > } > restart() { > diff --git a/package/postgresql/postgresql.mk b/package/postgresql/postgresql.mk > index 858cd69..5a4ffe6 100644 > --- a/package/postgresql/postgresql.mk > +++ b/package/postgresql/postgresql.mk > @@ -102,11 +102,11 @@ endif > POSTGRESQL_CONF_ENV += CFLAGS="$(POSTGRESQL_CFLAGS)" > > define POSTGRESQL_USERS > - postgres -1 postgres -1 * /var/lib/pgsql /bin/sh - PostgreSQL Server > + postgres -1 postgres -1 * /var/lib/postgresql /bin/sh - PostgreSQL Server > endef > > define POSTGRESQL_INSTALL_TARGET_FIXUP > - $(INSTALL) -dm 0700 $(TARGET_DIR)/var/lib/pgsql > + $(INSTALL) -dm 0700 $(TARGET_DIR)/var/lib/postgresql > $(RM) -rf $(TARGET_DIR)/usr/lib/postgresql/pgxs > endef > > diff --git a/package/postgresql/postgresql.service b/package/postgresql/postgresql.service > index 53e6f84..7698558 100644 > --- a/package/postgresql/postgresql.service > +++ b/package/postgresql/postgresql.service > @@ -15,10 +15,10 @@ Group=postgres > StandardOutput=syslog > StandardError=syslog > SyslogIdentifier=postgres > -PIDFile=/var/lib/pgsql/postmaster.pid > +PIDFile=/var/lib/postgresql/postmaster.pid > > -ExecStartPre=/bin/sh -c "if [ ! -f /var/lib/pgsql/PG_VERSION ]; then /usr/bin/pg_ctl initdb -D /var/lib/pgsql; fi" > -ExecStart=/usr/bin/postgres -D /var/lib/pgsql > +ExecStartPre=/bin/sh -c "if [ ! -f /var/lib/postgresql/PG_VERSION ]; then /usr/bin/pg_ctl initdb -D /var/lib/postgresql; fi" > +ExecStart=/usr/bin/postgres -D /var/lib/postgresql > ExecReload=/usr/bin/kill -HUP $MAINPID > KillMode=mixed > KillSignal=SIGINT
From: Peter Seiderer <ps.report@gmx.net> To: Pascal de Bruijn <p.debruijn@unilogic.nl> Cc: <buildroot@busybox.net> Sent: 11/28/2019 10:38 PM Subject: Re: [Buildroot] [PATCH] package/postgresql: fix filesystem naming consistency Hello Pascal, On Thu, 28 Nov 2019 12:15:42 +0100, Pascal de Bruijn <p.debruijn@unilogic.nl> wrote: > Currently the service is called postgresql, but other filesystem > references are called pgsql, which is inconsistent and confusing. > > Given that at least Debian uses postgresql in the filesystems > as well I would suggest moving the filesystem reference to align > with the service name as opposed to the other way around. > > Signed-off-by: Pascal de Bruijn <p.debruijn@unilogic.nl> > --- > package/postgresql/Config.in | 2 +- > package/postgresql/S50postgresql | 8 ++++---- > package/postgresql/postgresql.mk | 4 ++-- > package/postgresql/postgresql.service | 6 +++--- > 4 files changed, 10 insertions(+), 10 deletions(-) > > diff --git a/package/postgresql/Config.in b/package/postgresql/Config.in > index e548d3c..2f677da 100644 > --- a/package/postgresql/Config.in > +++ b/package/postgresql/Config.in > @@ -11,7 +11,7 @@ config BR2_PACKAGE_POSTGRESQL > database system. > > Enable the readline package to gain readline support in > - pgsql (the command line interpreter), which offers > + psql (the command line interpreter), which offers +1 for fixing this typo (which dates back to my initial submission from 2014) in a separate patch... > command history and edit functions. > > Enable the zlib package to gain builtin compression for > diff --git a/package/postgresql/S50postgresql b/package/postgresql/S50postgresql > index 1ece4fc..8d7ad34 100644 > --- a/package/postgresql/S50postgresql > +++ b/package/postgresql/S50postgresql > @@ -5,20 +5,20 @@ > > umask 077 > > -if [ ! -f /var/lib/pgsql/PG_VERSION ]; then > +if [ ! -f /var/lib/postgresql/PG_VERSION ]; then > echo "Initializing postgresql data base..." > - su - postgres -c '/usr/bin/pg_ctl initdb -D /var/lib/pgsql' > + su - postgres -c '/usr/bin/pg_ctl initdb -D /var/lib/postgresql' -1 for this change as /var/lib/pgsql is the suggested/common default location, see e.g. [1], and is saving 5 characters at many location from the script ;-) This comes down to: Do you want to stick with an upstream recommendation even if it's ill considered... Given this is a trivially avoided inconsistency. The whole point of having consistent naming between service and related directories is that you don't even have to look in the documentation where things are. On a sidenote, PostgreSQL does look for /etc/postgresql/postgresql.conf regardless and not /etc/pgsql/pgsql.conf, so it's a bit of mess really... As far as I can tell Debian choose to fix this, and RedHat seems to have stuck with the upstream inconsistency as well. So ultimately it's a bit of philosophical choice... Regards, Pascal de Bruijn
On 29/11/2019 09:32, Pascal de Bruijn wrote: > From: Peter Seiderer <ps.report@gmx.net> [snip] >> -if [ ! -f /var/lib/pgsql/PG_VERSION ]; then >> +if [ ! -f /var/lib/postgresql/PG_VERSION ]; then >> echo "Initializing postgresql data base..." >> - su - postgres -c '/usr/bin/pg_ctl initdb -D /var/lib/pgsql' >> + su - postgres -c '/usr/bin/pg_ctl initdb -D /var/lib/postgresql' > > -1 for this change as /var/lib/pgsql is the suggested/common default location, see e.g. [1], > and is saving 5 characters at many location from the script ;-) More important than whether it is suggested by upstream, though: changing it now breaks existing installations. Indeed, anyone who has installed a system with existing current buildroot will have the databases in /var/ib/pgsql. After this change, upgrading the system to newer buildroot will result in the databases no longer being found and instead new databases are created... So, at least a "legacy" symlink pgsql->postgresql must be created. But even that is a bit iffy, because it's likely that the databases are not directly in the readonly rootfs, but symlinked to a (writeable) data partition. So the symlink must be created at runtime. But even then, you can't know if the user put the whole of /var/lib on the writeable partition, or just /var/lib/pgsql... > This comes down to: > Do you want to stick with an upstream recommendation even if it's ill considered... > Given this is a trivially avoided inconsistency. It was trivially avoidable 5 years ago. Now, it is no longer trivial. > The whole point of having consistent naming between service and related directories is that you don't even have to look in the documentation where things are. > > > > On a sidenote, PostgreSQL does look for /etc/postgresql/postgresql.conf regardless and not /etc/pgsql/pgsql.conf, so it's a bit of mess really... If these are the only reasons for changing the name, then I don't think it's worth the bother of breaking existing systems. Regards, Arnout > > > As far as I can tell Debian choose to fix this, and RedHat seems to have stuck with the upstream inconsistency as well. > > > So ultimately it's a bit of philosophical choice... > > > > Regards, > Pascal de Bruijn > > > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes: Hello, >> The whole point of having consistent naming between service and >> related directories is that you don't even have to look in the >> documentation where things are. >> >> On a sidenote, PostgreSQL does look for >> /etc/postgresql/postgresql.conf regardless and not >> /etc/pgsql/pgsql.conf, so it's a bit of mess really... > If these are the only reasons for changing the name, then I don't think it's > worth the bother of breaking existing systems. Agreed.
diff --git a/package/postgresql/Config.in b/package/postgresql/Config.in index e548d3c..2f677da 100644 --- a/package/postgresql/Config.in +++ b/package/postgresql/Config.in @@ -11,7 +11,7 @@ config BR2_PACKAGE_POSTGRESQL database system. Enable the readline package to gain readline support in - pgsql (the command line interpreter), which offers + psql (the command line interpreter), which offers command history and edit functions. Enable the zlib package to gain builtin compression for diff --git a/package/postgresql/S50postgresql b/package/postgresql/S50postgresql index 1ece4fc..8d7ad34 100644 --- a/package/postgresql/S50postgresql +++ b/package/postgresql/S50postgresql @@ -5,20 +5,20 @@ umask 077 -if [ ! -f /var/lib/pgsql/PG_VERSION ]; then +if [ ! -f /var/lib/postgresql/PG_VERSION ]; then echo "Initializing postgresql data base..." - su - postgres -c '/usr/bin/pg_ctl initdb -D /var/lib/pgsql' + su - postgres -c '/usr/bin/pg_ctl initdb -D /var/lib/postgresql' echo "done" fi start() { printf "Starting postgresql: " - su - postgres -c '/usr/bin/pg_ctl start -w -D /var/lib/pgsql -l logfile' + su - postgres -c '/usr/bin/pg_ctl start -w -D /var/lib/postgresql -l logfile' echo "OK" } stop() { printf "Stopping postgresql: " - su - postgres -c '/usr/bin/pg_ctl stop -D /var/lib/pgsql -m fast' + su - postgres -c '/usr/bin/pg_ctl stop -D /var/lib/postgresql -m fast' echo "OK" } restart() { diff --git a/package/postgresql/postgresql.mk b/package/postgresql/postgresql.mk index 858cd69..5a4ffe6 100644 --- a/package/postgresql/postgresql.mk +++ b/package/postgresql/postgresql.mk @@ -102,11 +102,11 @@ endif POSTGRESQL_CONF_ENV += CFLAGS="$(POSTGRESQL_CFLAGS)" define POSTGRESQL_USERS - postgres -1 postgres -1 * /var/lib/pgsql /bin/sh - PostgreSQL Server + postgres -1 postgres -1 * /var/lib/postgresql /bin/sh - PostgreSQL Server endef define POSTGRESQL_INSTALL_TARGET_FIXUP - $(INSTALL) -dm 0700 $(TARGET_DIR)/var/lib/pgsql + $(INSTALL) -dm 0700 $(TARGET_DIR)/var/lib/postgresql $(RM) -rf $(TARGET_DIR)/usr/lib/postgresql/pgxs endef diff --git a/package/postgresql/postgresql.service b/package/postgresql/postgresql.service index 53e6f84..7698558 100644 --- a/package/postgresql/postgresql.service +++ b/package/postgresql/postgresql.service @@ -15,10 +15,10 @@ Group=postgres StandardOutput=syslog StandardError=syslog SyslogIdentifier=postgres -PIDFile=/var/lib/pgsql/postmaster.pid +PIDFile=/var/lib/postgresql/postmaster.pid -ExecStartPre=/bin/sh -c "if [ ! -f /var/lib/pgsql/PG_VERSION ]; then /usr/bin/pg_ctl initdb -D /var/lib/pgsql; fi" -ExecStart=/usr/bin/postgres -D /var/lib/pgsql +ExecStartPre=/bin/sh -c "if [ ! -f /var/lib/postgresql/PG_VERSION ]; then /usr/bin/pg_ctl initdb -D /var/lib/postgresql; fi" +ExecStart=/usr/bin/postgres -D /var/lib/postgresql ExecReload=/usr/bin/kill -HUP $MAINPID KillMode=mixed KillSignal=SIGINT
Currently the service is called postgresql, but other filesystem references are called pgsql, which is inconsistent and confusing. Given that at least Debian uses postgresql in the filesystems as well I would suggest moving the filesystem reference to align with the service name as opposed to the other way around. Signed-off-by: Pascal de Bruijn <p.debruijn@unilogic.nl> --- package/postgresql/Config.in | 2 +- package/postgresql/S50postgresql | 8 ++++---- package/postgresql/postgresql.mk | 4 ++-- package/postgresql/postgresql.service | 6 +++--- 4 files changed, 10 insertions(+), 10 deletions(-)