[Update] PostgreSQL allow -D in rc.conf.local

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[Update] PostgreSQL allow -D in rc.conf.local

Adam Wolk-2
Hi ports@,

I'm attaching a small diff to allow picking a different postgresql
cluster (the directory you create with initdb) location within
/etc/rc.conf.local.

With the attached diff, one can now define the folder with:
postgresql_flags="-D /var/postgresql/testdata -w -l
/var/postgresql/logfile"

Changing the value after -D to point to any valid pg cluster.

The current implementation had the path hard-coded in the rc.d file with
no way to override it.

By using the existing daemon_flags and setting -D there we indeed
could start postgresql with a different data_directory as the last -D
passed to start takes priority. However since that flag was not used in
the remaining calls the rc script would fail to check, reload and stop a
postgresql instance started this way.

In order to allow setting a custom data_directory, we remove datadir
variable and set -D directly in daemon_flags then append ${daemon_flags}
to all invocations of pg_ctl ($rcexec)) in check, reload, start and stop
targets. The existing flags we use 'by default' which is -w and -l that
due to this change are now added to all invocations have the following
impact:

1. `-l` ignored in all targets except start, it sets the location of the
log file. adding it to other commands is a no-op and doesn't change
existing behavior.
2. `-w` affects the start and stop targets. No change in behavior for
the start target. For the stop target it's also a no-op as reading the
pg_ctl code - that's the default even if not specified on the command
line.

Since we got rid of ${datadir} we now have no way to check where is the
postmaster.pid that the start target attempts to remove. I investigated
this and came to the conclusion that removing the pidfile is not needed.

When starting, pg_ctl upon noticing that a postmaster.pid is already
present will issue a warning:

pg_ctl: another server might be running; trying to start server anyway

The pg_ctl code itself handles cases of a leftover pid:

https://github.com/postgres/postgres/blob/b8f2da0ac5a24f669c8d320c58646759b8fc69a5/src/bin/pg_ctl/pg_ctl.c#L583

So if the database starts up properly, the file will be overwritten but
a system administrator will be able to see a log entry indicating that a
dead pidfile was removed. Note that pg_ctl stop itself removes the
postmaster.pid so if it's there then likely the database was not stopped
properly (and might need investigation for recovery).

The removal of the postmaster.pid has been moved from post server stop
to server start 9 years ago in commit 1.3. It was present in stop since
revision 1.1 of the file.

I don't think the above changes require a port quirk. Worst case
scenario iff someone defined postgresql_flags in rc.conf.local their
database would now not start due to -D being likely not specified by the
user. However, this is not permanent (or any whatsoever) damage as the
user only needs to add -D /var/postgresql/data to his flags to regain
full functionality.

I do think that this could warrant an entry in current.html. The second
diff addresses this.

I also noticed that the postgresql_flags line is used verbatim in the
ENVIRONMENT section of rc.d(8), should I update that manual to include
the -D that is now the new default for flags? I assume that yes, so the
third diff covers that, however the extended line wraps in the manual
now (perhaps that is undesired?).

As always I'm looking for feedback and OK's :)

Regards,
Adam

Index: Makefile
===================================================================
RCS file: /cvs/ports/databases/postgresql/Makefile,v
retrieving revision 1.255
diff -u -p -r1.255 Makefile
--- Makefile 12 Aug 2019 16:40:40 -0000 1.255
+++ Makefile 17 Aug 2019 20:42:47 -0000
@@ -9,6 +9,7 @@ COMMENT-pg_upgrade=Support for upgrading
 
 VERSION= 11.5
 PREV_MAJOR= 10
+REVISION-server=1
 DISTNAME= postgresql-${VERSION}
 PKGNAME-main= postgresql-client-${VERSION}
 PKGNAME-server= postgresql-server-${VERSION}
Index: pkg/postgresql.rc
===================================================================
RCS file: /cvs/ports/databases/postgresql/pkg/postgresql.rc,v
retrieving revision 1.12
diff -u -p -r1.12 postgresql.rc
--- pkg/postgresql.rc 11 Jan 2018 19:27:01 -0000 1.12
+++ pkg/postgresql.rc 17 Aug 2019 20:42:47 -0000
@@ -2,10 +2,8 @@
 #
 # $OpenBSD: postgresql.rc,v 1.12 2018/01/11 19:27:01 rpe Exp $
 
-datadir="/var/postgresql/data"
-
 daemon="${TRUEPREFIX}/bin/pg_ctl"
-daemon_flags="-w -l /var/postgresql/logfile"
+daemon_flags="-D /var/postgresql/data -w -l /var/postgresql/logfile"
 daemon_user="_postgresql"
 
 . /etc/rc.d/rc.subr
@@ -13,21 +11,20 @@ daemon_user="_postgresql"
 rc_usercheck=NO
 
 rc_check() {
- ${rcexec} "${daemon} -D ${datadir} status"
+ ${rcexec} "${daemon} status ${daemon_flags}"
 }
 
 rc_reload() {
- ${rcexec} "${daemon} -D ${datadir} reload"
+ ${rcexec} "${daemon} reload ${daemon_flags}"
 }
 
 rc_start() {
- rm -f ${datadir}/postmaster.pid
- ${rcexec} "${daemon} -D ${datadir} start ${daemon_flags}"
+ ${rcexec} "${daemon} start ${daemon_flags}"
 }
 
 rc_stop() {
- ${rcexec} "${daemon} -D ${datadir} stop -m fast" || \
- ${rcexec} "${daemon} -D ${datadir} stop -m immediate"
+ ${rcexec} "${daemon} stop ${daemon_flags} -m fast" || \
+ ${rcexec} "${daemon} stop ${daemon_flags} -m immediate"
 }
 
 rc_cmd $1

Index: current.html
===================================================================
RCS file: /cvs/www/faq/current.html,v
retrieving revision 1.1006
diff -u -p -r1.1006 current.html
--- current.html 7 Aug 2019 10:34:25 -0000 1.1006
+++ current.html 17 Aug 2019 20:42:19 -0000
@@ -152,6 +152,21 @@ Instead disable the route evaluation pro
 <code>rde rib Loc-RIB no evaluate</code>.
 
 
+<h3 id="r20190817">2019/08/17 - [ports] Datadir added to default postgres flags</h3>
+
+Default <a href="https://man.openbsd.org/rc.d.8">rc.d(8)</a> flags
+for databases/postgresql have been changed from:
+<pre class="cmdbox">
+daemon_flags="-w -l /var/postgresql/logfile"
+</pre>
+to
+<pre class="cmdbox">
+daemon_flags="-D /var/postgresql/data -w -l /var/postgresql/logfile"
+</pre>
+if you defined <code>postgresql_flags</code> in <code>/etc/rc.conf.local</code>
+make sure that your entry includes <code>-D /var/postgresql/data</code>.
+
+
 <!--
      Two blank lines before new sections.
      New sentences start on new lines.

Index: rc.d.8
===================================================================
RCS file: /cvs/src/share/man/man8/rc.d.8,v
retrieving revision 1.34
diff -u -p -r1.34 rc.d.8
--- rc.d.8 27 Jul 2017 07:49:05 -0000 1.34
+++ rc.d.8 17 Aug 2019 20:21:28 -0000
@@ -137,13 +137,13 @@ with the name of the script.
 For example, postgres is managed through
 .Pa /etc/rc.d/postgresql :
 .Pp
-.Dl daemon_flags=-w -l /var/postgresql/logfile
+.Dl daemon_flags="-D /var/postgresql/data -w -l /var/postgresql/logfile"
 .Pp
 To override this and increase the debug log level (keeping the existing
 flags), define the following in
 .Xr rc.conf.local 8 :
 .Pp
-.Dl postgresql_flags=-w -l /var/postgresql/logfile -d 5
+.Dl postgresql_flags="-D /var/postgresql/data -w -l /var/postgresql/logfile -d 5"
 .Pp
 Each script may define its own defaults, as explained in
 .Xr rc.subr 8 .
Reply | Threaded
Open this post in threaded view
|

Re: [Update] PostgreSQL allow -D in rc.conf.local

Antoine Jacoutot-7
On Sat, Aug 17, 2019 at 10:55:11PM +0200, Adam Wolk wrote:

> Hi ports@,
>
> I'm attaching a small diff to allow picking a different postgresql
> cluster (the directory you create with initdb) location within
> /etc/rc.conf.local.
>
> With the attached diff, one can now define the folder with:
> postgresql_flags="-D /var/postgresql/testdata -w -l
> /var/postgresql/logfile"
>
> Changing the value after -D to point to any valid pg cluster.
>
> The current implementation had the path hard-coded in the rc.d file with
> no way to override it.
>
> By using the existing daemon_flags and setting -D there we indeed
> could start postgresql with a different data_directory as the last -D
> passed to start takes priority. However since that flag was not used in
> the remaining calls the rc script would fail to check, reload and stop a
> postgresql instance started this way.
>
> In order to allow setting a custom data_directory, we remove datadir
> variable and set -D directly in daemon_flags then append ${daemon_flags}
> to all invocations of pg_ctl ($rcexec)) in check, reload, start and stop
> targets. The existing flags we use 'by default' which is -w and -l that
> due to this change are now added to all invocations have the following
> impact:
>
> 1. `-l` ignored in all targets except start, it sets the location of the
> log file. adding it to other commands is a no-op and doesn't change
> existing behavior.
> 2. `-w` affects the start and stop targets. No change in behavior for
> the start target. For the stop target it's also a no-op as reading the
> pg_ctl code - that's the default even if not specified on the command
> line.
>
> Since we got rid of ${datadir} we now have no way to check where is the
> postmaster.pid that the start target attempts to remove. I investigated
> this and came to the conclusion that removing the pidfile is not needed.
>
> When starting, pg_ctl upon noticing that a postmaster.pid is already
> present will issue a warning:
>
> pg_ctl: another server might be running; trying to start server anyway
>
> The pg_ctl code itself handles cases of a leftover pid:
>
> https://github.com/postgres/postgres/blob/b8f2da0ac5a24f669c8d320c58646759b8fc69a5/src/bin/pg_ctl/pg_ctl.c#L583
>
> So if the database starts up properly, the file will be overwritten but
> a system administrator will be able to see a log entry indicating that a
> dead pidfile was removed. Note that pg_ctl stop itself removes the
> postmaster.pid so if it's there then likely the database was not stopped
> properly (and might need investigation for recovery).
>
> The removal of the postmaster.pid has been moved from post server stop
> to server start 9 years ago in commit 1.3. It was present in stop since
> revision 1.1 of the file.
>
> I don't think the above changes require a port quirk. Worst case
> scenario iff someone defined postgresql_flags in rc.conf.local their
> database would now not start due to -D being likely not specified by the
> user. However, this is not permanent (or any whatsoever) damage as the
> user only needs to add -D /var/postgresql/data to his flags to regain
> full functionality.
>
> I do think that this could warrant an entry in current.html. The second
> diff addresses this.
>
> I also noticed that the postgresql_flags line is used verbatim in the
> ENVIRONMENT section of rc.d(8), should I update that manual to include
> the -D that is now the new default for flags? I assume that yes, so the
> third diff covers that, however the extended line wraps in the manual
> now (perhaps that is undesired?).
>
> As always I'm looking for feedback and OK's :)

I like this.


>
> Regards,
> Adam

> Index: Makefile
> ===================================================================
> RCS file: /cvs/ports/databases/postgresql/Makefile,v
> retrieving revision 1.255
> diff -u -p -r1.255 Makefile
> --- Makefile 12 Aug 2019 16:40:40 -0000 1.255
> +++ Makefile 17 Aug 2019 20:42:47 -0000
> @@ -9,6 +9,7 @@ COMMENT-pg_upgrade=Support for upgrading
>  
>  VERSION= 11.5
>  PREV_MAJOR= 10
> +REVISION-server=1
>  DISTNAME= postgresql-${VERSION}
>  PKGNAME-main= postgresql-client-${VERSION}
>  PKGNAME-server= postgresql-server-${VERSION}
> Index: pkg/postgresql.rc
> ===================================================================
> RCS file: /cvs/ports/databases/postgresql/pkg/postgresql.rc,v
> retrieving revision 1.12
> diff -u -p -r1.12 postgresql.rc
> --- pkg/postgresql.rc 11 Jan 2018 19:27:01 -0000 1.12
> +++ pkg/postgresql.rc 17 Aug 2019 20:42:47 -0000
> @@ -2,10 +2,8 @@
>  #
>  # $OpenBSD: postgresql.rc,v 1.12 2018/01/11 19:27:01 rpe Exp $
>  
> -datadir="/var/postgresql/data"
> -
>  daemon="${TRUEPREFIX}/bin/pg_ctl"
> -daemon_flags="-w -l /var/postgresql/logfile"
> +daemon_flags="-D /var/postgresql/data -w -l /var/postgresql/logfile"
>  daemon_user="_postgresql"
>  
>  . /etc/rc.d/rc.subr
> @@ -13,21 +11,20 @@ daemon_user="_postgresql"
>  rc_usercheck=NO
>  
>  rc_check() {
> - ${rcexec} "${daemon} -D ${datadir} status"
> + ${rcexec} "${daemon} status ${daemon_flags}"
>  }
>  
>  rc_reload() {
> - ${rcexec} "${daemon} -D ${datadir} reload"
> + ${rcexec} "${daemon} reload ${daemon_flags}"
>  }
>  
>  rc_start() {
> - rm -f ${datadir}/postmaster.pid
> - ${rcexec} "${daemon} -D ${datadir} start ${daemon_flags}"
> + ${rcexec} "${daemon} start ${daemon_flags}"
>  }
>  
>  rc_stop() {
> - ${rcexec} "${daemon} -D ${datadir} stop -m fast" || \
> - ${rcexec} "${daemon} -D ${datadir} stop -m immediate"
> + ${rcexec} "${daemon} stop ${daemon_flags} -m fast" || \
> + ${rcexec} "${daemon} stop ${daemon_flags} -m immediate"
>  }
>  
>  rc_cmd $1

> Index: current.html
> ===================================================================
> RCS file: /cvs/www/faq/current.html,v
> retrieving revision 1.1006
> diff -u -p -r1.1006 current.html
> --- current.html 7 Aug 2019 10:34:25 -0000 1.1006
> +++ current.html 17 Aug 2019 20:42:19 -0000
> @@ -152,6 +152,21 @@ Instead disable the route evaluation pro
>  <code>rde rib Loc-RIB no evaluate</code>.
>  
>  
> +<h3 id="r20190817">2019/08/17 - [ports] Datadir added to default postgres flags</h3>
> +
> +Default <a href="https://man.openbsd.org/rc.d.8">rc.d(8)</a> flags
> +for databases/postgresql have been changed from:
> +<pre class="cmdbox">
> +daemon_flags="-w -l /var/postgresql/logfile"
> +</pre>
> +to
> +<pre class="cmdbox">
> +daemon_flags="-D /var/postgresql/data -w -l /var/postgresql/logfile"
> +</pre>
> +if you defined <code>postgresql_flags</code> in <code>/etc/rc.conf.local</code>
> +make sure that your entry includes <code>-D /var/postgresql/data</code>.
> +
> +
>  <!--
>       Two blank lines before new sections.
>       New sentences start on new lines.

> Index: rc.d.8
> ===================================================================
> RCS file: /cvs/src/share/man/man8/rc.d.8,v
> retrieving revision 1.34
> diff -u -p -r1.34 rc.d.8
> --- rc.d.8 27 Jul 2017 07:49:05 -0000 1.34
> +++ rc.d.8 17 Aug 2019 20:21:28 -0000
> @@ -137,13 +137,13 @@ with the name of the script.
>  For example, postgres is managed through
>  .Pa /etc/rc.d/postgresql :
>  .Pp
> -.Dl daemon_flags=-w -l /var/postgresql/logfile
> +.Dl daemon_flags="-D /var/postgresql/data -w -l /var/postgresql/logfile"
>  .Pp
>  To override this and increase the debug log level (keeping the existing
>  flags), define the following in
>  .Xr rc.conf.local 8 :
>  .Pp
> -.Dl postgresql_flags=-w -l /var/postgresql/logfile -d 5
> +.Dl postgresql_flags="-D /var/postgresql/data -w -l /var/postgresql/logfile -d 5"
>  .Pp
>  Each script may define its own defaults, as explained in
>  .Xr rc.subr 8 .


--
Antoine

Reply | Threaded
Open this post in threaded view
|

Re: [Update] PostgreSQL allow -D in rc.conf.local

Stuart Henderson
On 2019/08/18 08:51, Antoine Jacoutot wrote:

> On Sat, Aug 17, 2019 at 10:55:11PM +0200, Adam Wolk wrote:
> > Hi ports@,
> >
> > I'm attaching a small diff to allow picking a different postgresql
> > cluster (the directory you create with initdb) location within
> > /etc/rc.conf.local.
> >
> > With the attached diff, one can now define the folder with:
> > postgresql_flags="-D /var/postgresql/testdata -w -l
> > /var/postgresql/logfile"
[..snip..]

> >
> > I also noticed that the postgresql_flags line is used verbatim in the
> > ENVIRONMENT section of rc.d(8), should I update that manual to include
> > the -D that is now the new default for flags? I assume that yes, so the
> > third diff covers that, however the extended line wraps in the manual
> > now (perhaps that is undesired?).
> >
> > As always I'm looking for feedback and OK's :)
>
> I like this.

me too.

> > I do think that this could warrant an entry in current.html.

yes, I agree.

OK with all except one part,

> > Index: rc.d.8
> > ===================================================================
> > RCS file: /cvs/src/share/man/man8/rc.d.8,v
> > retrieving revision 1.34
> > diff -u -p -r1.34 rc.d.8
> > --- rc.d.8 27 Jul 2017 07:49:05 -0000 1.34
> > +++ rc.d.8 17 Aug 2019 20:21:28 -0000
> > @@ -137,13 +137,13 @@ with the name of the script.
> >  For example, postgres is managed through
> >  .Pa /etc/rc.d/postgresql :
> >  .Pp
> > -.Dl daemon_flags=-w -l /var/postgresql/logfile
> > +.Dl daemon_flags="-D /var/postgresql/data -w -l /var/postgresql/logfile"
> >  .Pp
> >  To override this and increase the debug log level (keeping the existing
> >  flags), define the following in
> >  .Xr rc.conf.local 8 :
> >  .Pp
> > -.Dl postgresql_flags=-w -l /var/postgresql/logfile -d 5
> > +.Dl postgresql_flags="-D /var/postgresql/data -w -l /var/postgresql/logfile -d 5"

I think this should be without quotes, it works either way but the
normal documented format is without, e.g. see the dhcpd_flags example
in rc.conf.local(5).

> >  .Pp
> >  Each script may define its own defaults, as explained in
> >  .Xr rc.subr 8 .

Reply | Threaded
Open this post in threaded view
|

Re: [Update] PostgreSQL allow -D in rc.conf.local

Pierre-Emmanuel Andre
Looks good for me.
Ok pea@

Thank you

Le 24 août 2019 22:54:41 GMT+02:00, Stuart Henderson <[hidden email]> a écrit :

>On 2019/08/18 08:51, Antoine Jacoutot wrote:
>> On Sat, Aug 17, 2019 at 10:55:11PM +0200, Adam Wolk wrote:
>> > Hi ports@,
>> >
>> > I'm attaching a small diff to allow picking a different postgresql
>> > cluster (the directory you create with initdb) location within
>> > /etc/rc.conf.local.
>> >
>> > With the attached diff, one can now define the folder with:
>> > postgresql_flags="-D /var/postgresql/testdata -w -l
>> > /var/postgresql/logfile"
>[..snip..]
>
>> >
>> > I also noticed that the postgresql_flags line is used verbatim in
>the
>> > ENVIRONMENT section of rc.d(8), should I update that manual to
>include
>> > the -D that is now the new default for flags? I assume that yes, so
>the
>> > third diff covers that, however the extended line wraps in the
>manual
>> > now (perhaps that is undesired?).
>> >
>> > As always I'm looking for feedback and OK's :)
>>
>> I like this.
>
>me too.
>
>> > I do think that this could warrant an entry in current.html.
>
>yes, I agree.
>
>OK with all except one part,
>
>> > Index: rc.d.8
>> > ===================================================================
>> > RCS file: /cvs/src/share/man/man8/rc.d.8,v
>> > retrieving revision 1.34
>> > diff -u -p -r1.34 rc.d.8
>> > --- rc.d.8 27 Jul 2017 07:49:05 -0000 1.34
>> > +++ rc.d.8 17 Aug 2019 20:21:28 -0000
>> > @@ -137,13 +137,13 @@ with the name of the script.
>> >  For example, postgres is managed through
>> >  .Pa /etc/rc.d/postgresql :
>> >  .Pp
>> > -.Dl daemon_flags=-w -l /var/postgresql/logfile
>> > +.Dl daemon_flags="-D /var/postgresql/data -w -l
>/var/postgresql/logfile"
>> >  .Pp
>> >  To override this and increase the debug log level (keeping the
>existing
>> >  flags), define the following in
>> >  .Xr rc.conf.local 8 :
>> >  .Pp
>> > -.Dl postgresql_flags=-w -l /var/postgresql/logfile -d 5
>> > +.Dl postgresql_flags="-D /var/postgresql/data -w -l
>/var/postgresql/logfile -d 5"
>
>I think this should be without quotes, it works either way but the
>normal documented format is without, e.g. see the dhcpd_flags example
>in rc.conf.local(5).
>
>> >  .Pp
>> >  Each script may define its own defaults, as explained in
>> >  .Xr rc.subr 8 .