This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 696427 - Native systemd integration for PostgreSQL
Native systemd integration for PostgreSQL
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: postgresql (Show other bugs)
rawhide
Unspecified Unspecified
unspecified Severity unspecified
: ---
: ---
Assigned To: Tom Lane
Fedora Extras Quality Assurance
:
Depends On:
Blocks: SysVtoSystemd 560634
  Show dependency treegraph
 
Reported: 2011-04-13 23:29 EDT by Mathieu Bridon
Modified: 2011-08-22 11:11 EDT (History)
7 users (show)

See Also:
Fixed In Version: postgresql-9.0.4-8.fc16
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2011-08-22 11:11:31 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
First patch to apply: split initdb and upgrade out of the init script. (12.01 KB, patch)
2011-04-13 23:30 EDT, Mathieu Bridon
no flags Details | Diff
Second patch to apply: systemd native integration. (7.58 KB, patch)
2011-04-13 23:31 EDT, Mathieu Bridon
no flags Details | Diff
First patch to apply: split initdb and upgrade out of the init script. (11.99 KB, patch)
2011-06-10 03:18 EDT, Mathieu Bridon
no flags Details | Diff
Second patch to apply: systemd native integration. (7.66 KB, patch)
2011-06-10 03:19 EDT, Mathieu Bridon
no flags Details | Diff
Split initdb/upgrade features out of the init script. (11.99 KB, patch)
2011-07-05 22:42 EDT, Mathieu Bridon
no flags Details | Diff
[PATCH 2/2] Native systemd integration for the PostgreSQL server. (7.65 KB, patch)
2011-07-05 22:43 EDT, Mathieu Bridon
no flags Details | Diff
initilizing postgresql database systemd unit (537 bytes, text/plain)
2011-07-12 17:10 EDT, Jóhann B. Guðmundsson
no flags Details
Upgrading Postgresql Database systemd unit. (798 bytes, text/plain)
2011-07-12 17:17 EDT, Jóhann B. Guðmundsson
no flags Details
Standalone pgsql init unit (355 bytes, text/plain)
2011-07-12 19:38 EDT, Jóhann B. Guðmundsson
no flags Details
Standalone pgsql upgrade unit (556 bytes, text/plain)
2011-07-12 19:39 EDT, Jóhann B. Guðmundsson
no flags Details
/etc/sysconfig file (140 bytes, text/plain)
2011-07-12 19:41 EDT, Jóhann B. Guðmundsson
no flags Details
Add a setup service (3.33 KB, patch)
2011-08-18 03:36 EDT, Mathieu Bridon
no flags Details | Diff

  None (edit)
Description Mathieu Bridon 2011-04-13 23:29:47 EDT
I have been working on providing native systemd integration for PostgreSQL.

Attached are two patches that apply to both the master and f15 branches of the Fedora package in dist-git.

The changes made to the spec file follow the draft packaging guidelines for systemd-enabled services:
    - https://fedoraproject.org/wiki/TomCallaway/Systemd_Revised_Draft
    - https://fedoraproject.org/wiki/User:Toshio/Systemd_scriptlet_options

Those guidelines are still draft because they haven't been properly tested yet. I have spent most of yesterday testing the changes, my test results are available here:
    http://bochecha.fedorapeople.org/postgresql-sysv_to_systemd.txt

So from the packaging side, everything looks quite satisfying (one little detail, see the notes at the bottom of the page), unless I missed some testing scenarii.

From the PostgreSQL side, I believe I didn't miss anything when porting from the init script, but of course I might be wrong.

There is one little detail that I don't quite like. Those changes introduce 2 new rpmlint errors:
 postgresql.spec:486: E: hardcoded-library-path in $RPM_BUILD_ROOT/lib/systemd/system
 A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
 should be replaced by something like /%{_lib} or %{_libdir}.
 
 postgresql.spec:487: E: hardcoded-library-path in $RPM_BUILD_ROOT/lib/systemd/system/postgresql.service
 A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
 should be replaced by something like /%{_lib} or %{_libdir}.

I don't really know how to fix these yet. The location is right (those files are not arch specific) but it is kind of annoying to be polluting the rpmlint output like this. :-/
Comment 1 Mathieu Bridon 2011-04-13 23:30:45 EDT
Created attachment 491941 [details]
First patch to apply: split initdb and upgrade out of the init script.
Comment 2 Mathieu Bridon 2011-04-13 23:31:25 EDT
Created attachment 491942 [details]
Second patch to apply: systemd native integration.
Comment 3 Mathieu Bridon 2011-04-13 23:33:15 EDT
CC-ing Tom and Toshio as they were working on the draft guidelines.
Comment 4 Mathieu Bridon 2011-04-13 23:51:01 EDT
As asked by Toshio on IRC:
- I rebooted between almost every test (where it made sense) and checked the results were still what we expecte
- When I checked that the service had been restarted, I looked at the output of "systemctl status postgresql.service" to make sure that the reported PID was different
Comment 5 Mathieu Bridon 2011-05-04 02:49:43 EDT
I just saw Spot's email about the packaging guidelines for systemd having been approved. Tom (Lane), would you have time to review my patches?
Comment 6 Tom Lane 2011-05-04 09:53:40 EDT
Yeah, I was kind of waiting for that to happen before putting any effort into this area.  I'll get to it soon.
Comment 7 Mathieu Bridon 2011-06-10 03:18:33 EDT
Created attachment 504031 [details]
First patch to apply: split initdb and upgrade out of the init script.

Here is an updated version of the first patch that applies on the new Rawhide Git HEAD.
Comment 8 Mathieu Bridon 2011-06-10 03:19:37 EDT
Created attachment 504032 [details]
Second patch to apply: systemd native integration.

And here comes the second patch update for the Rawhide Git HEAD.
Comment 9 Mathieu Bridon 2011-07-05 22:42:11 EDT
Created attachment 511424 [details]
Split initdb/upgrade features out of the init script.

In the native systemd world, we won't have the initscript anymore.
To not lose the initdb and upgrade features that it provides, they
need to be accessible as standalone commands.

This commit just takes them out of the initscript and installs them
in %{_bindir}. The init script was also modified to ensure keeping
compatibility.
Comment 10 Mathieu Bridon 2011-07-05 22:43:25 EDT
Created attachment 511425 [details]
[PATCH 2/2] Native systemd integration for the PostgreSQL server.

This commit migrates postgresql-server from the management with legacy
sysvinit scripts to native systemd unit configuration.

A few changes were necessary, as per the new Fedora Packaging Guidelines:
    - https://fedoraproject.org/wiki/Packaging:Guidelines:Systemd
    - https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd
    - https://fedoraproject.org/wiki/Packaging:SysVInitScript#Initscripts_in_addition_to_systemd_unit_files

The former page says:
    Packages may also provide a SysV initscript file, but are not
    required to do so.
    [... snip ...]
    If present, the SysV initscript(s) must go into an optional
    subpackage, so as not to confuse sysadmins.

As such, the postgresql spec file now builds a -sysvinit subpackage,
containing only the SysV init script.

The same page shows the snippets to use for the post, preun and postun
scriptlets. Those snippets are now used in the spec file, in lieu of
the former SysV-related scriptlets.

A native systemd unit file for PostgreSQL was added.

Finally, scriptlets for triggerun and truggerpostun were added as
specified in the latter pages above, providing an upgrade path from
the previous versions of the package that were not handled natively
by systemd.
Comment 11 Mathieu Bridon 2011-07-05 22:46:44 EDT
I'm starting a monthly tradition of rebasing those patches on the current Rawhide HEAD. :)

Any idea when (or if!) they can be applied?

For what is worth, I have been using a PostgreSQL package rebuilt with those patches at $dayjob since I submitted them the first time, and we haven't run into any kind of issue so far. Everything is smooth, PostgreSQL is started at boot time as intended (we enabled it manually), apps can connect to it, etc...
Comment 12 Jóhann B. Guðmundsson 2011-07-06 13:40:05 EDT
Tom ping ? 

Mathieu you can add "713562" ( blocker bug ) to the blocks field apparently I have insufficient bz privileges to do so myself
Comment 13 Kostas Georgiou 2011-07-12 11:56:44 EDT
With the old sysv script it was possible to do ln -s /etc/init.d/postgresql /etc/init.d/pgfoo && echo -e "PGPORT=9999\nPGDATA=/srv/pgfoo" > /etc/sysconfig/pgsql/pgfoo

With the current patch this doesn't work anymore and if someone installs the sysv script running /etc/init.d/pgfoo initdb|upgrade since postgresql-dbops has hardcoded /etc/sysconfig/pgsql/postgresql it will act on the wrong database.

If systemd can't handle the old setup things will have to be documented at least.

I am not sure about the -c silent_mode=off -c log_destination=syslog option either, you don't want to override a sysadmin setting which for example uses csvlog to feed the output somehwere else?
Comment 14 Tom Lane 2011-07-12 15:39:12 EDT
(In reply to comment #13)
> If systemd can't handle the old setup things will have to be documented at least.

I gather that /etc/sysconfig files are deprecated with systemd, but do we really need to remove the feature immediately?  It seems easier to use than having to make custom edits when duplicating the unit file for multiple database servers.

> I am not sure about the -c silent_mode=off -c log_destination=syslog option
> either, you don't want to override a sysadmin setting which for example uses
> csvlog to feed the output somehwere else?

I don't see a problem with silent_mode = off, except that it's probably useless because nobody is likely to try to turn that on.  (Also, upstream just deleted that setting as no longer of interest, which means the script would stop working as of PG 9.2, which seems like a bigger hazard than someone being silly and turning silent_mode on.)

I agree that it is not the charter of this patch to change the database's default logging configuration, *especially* not in a non-overridable way, so there is zero chance I'll accept the proposed log_destination option.
Comment 15 Jóhann B. Guðmundsson 2011-07-12 16:01:04 EDT
(In reply to comment #14)
> (In reply to comment #13)
> > If systemd can't handle the old setup things will have to be documented at least.
> 
> I gather that /etc/sysconfig files are deprecated with systemd, but do we
> really need to remove the feature immediately?  It seems easier to use than
> having to make custom edits when duplicating the unit file for multiple
> database servers.

Hum I'm not aware of the /etc/syconfig files being deprecated and Mathieu can you attached the actual unit files for postgresql so I can have a look at them I dont have time to go through the whole patch just to take a look at how the unit file look like.
Comment 16 Jóhann B. Guðmundsson 2011-07-12 17:10:17 EDT
Created attachment 512518 [details]
initilizing postgresql database systemd unit

This unit takes care of creating the initial database if it does not exist and effectively disable itself ones it has successfully run once and will fail if the data directory exist so it should save to enable this on installs
Comment 17 Jóhann B. Guðmundsson 2011-07-12 17:17:51 EDT
Created attachment 512522 [details]
Upgrading Postgresql Database systemd unit.

You will need to adjust this one to fit your desired upgrade behaviour and trigger. 

That should take care of systemd-nize that split of his and I assume that rest of the init script he has converted correctly to native systemd units.
Comment 18 Tom Lane 2011-07-12 17:24:30 EDT
(In reply to comment #16)
> Created attachment 512518 [details]
> initilizing postgresql database systemd unit
> 
> This unit takes care of creating the initial database if it does not exist and
> effectively disable itself ones it has successfully run once and will fail if
> the data directory exist so it should save to enable this on installs

This doesn't strike me as a particularly good idea, either.  There's a reason why the initscript doesn't do an automatic initdb for you but requires it to be a separate manual action, and that is that we used to have automatic initdb and it ate more than one person's database.  I think it should still be a separate manual action in the systemd world.

It is *not* sufficient to test whether the data directory already exists/is empty.  The reason why not is that people sometimes put the data directory on a mounted volume.  All you need is for the volume to be a bit slow to come up (think NFS, here, though it could happen on some forms of local storage too).  Then the test sees the data directory as not present, starts initdb, and if the volume comes live partway through that, boom!  initdb is scribbling on your database.  Don't laugh, this happened to more than one person before we decided automatic initdb was too much of a foot-gun.  In the asynchronous systemd environment I think the risk of that sort of situation would be a lot greater, not less.
Comment 19 Jóhann B. Guðmundsson 2011-07-12 17:35:25 EDT
having the service follow the same order as the postgresql.service will with the Added Before=postgresq.service should take care of that I would think or heck we could even bind it to the startup of the postgresql.service and have that before line in it and not enable it on install by default and if the data directory does not exit when the end user starts postgresql it gets created for them if it does it will fail to start..
Comment 20 Jóhann B. Guðmundsson 2011-07-12 17:37:20 EDT
Btw the way how did you solve that with starting the daemon it self?
Comment 21 Tom Lane 2011-07-12 17:57:23 EDT
My point is basically that the risk/reward ratio of having automatic initdb (or automatic pg_upgrade, either) is not appealing.  You do a manual initdb once per installation, it's done, you do *not* want the system considering whether to do it again on every subsequent reboot.  It only has to make the wrong choice once to destroy your database.  Against this risk there is little to set except a few seconds of time saved, once, by having it be automatic.

Another point is that, since database servers aren't started by default (and won't ever be, per Fedora policy), the time at which you might be deciding that you do want to run Postgres is not system boot time.  So putting this action into systemd's purview isn't really helpful anyway.  There's necessarily some manual action taken to enable the server to be started ("chkconfig on" or equivalent) and you can deal with initdb at the same time.

The reason why it was part of the initscript was only that it made it easy to ensure that the same configuration information (particularly, database directory location and owning user) was fed into both the initdb action and subsequent server start actions.  I'm not sure how we preserve that ease-of-use property in the systemd world, if we kick initdb out of systemd's purview.  Possibly the thing to do is insist that there always be an /etc/sysconfig file and have the initdb script read that.
Comment 22 Jóhann B. Guðmundsson 2011-07-12 18:16:00 EDT
You can add EnvironmentFile=-/etc/sysconfig/$foo to the initdb and the upgrade service that's no problem. 

You can also just change the [Install] section to say WantedBy=postgresql.service so when the end user decides to start the postgresql.service those service will get run and you can also just remove the After= Before= and the [Install] section and just install those units and you got your self a complete standalone units that get run as the postgresql user and the end user just has to run systemctl start pgsql-initdb.service and systemctl start pgsql-upgrade.service when ever he chooses to do so...
Comment 23 Jóhann B. Guðmundsson 2011-07-12 19:38:31 EDT
Created attachment 512537 [details]
Standalone pgsql init unit
Comment 24 Jóhann B. Guðmundsson 2011-07-12 19:39:19 EDT
Created attachment 512538 [details]
Standalone pgsql upgrade unit
Comment 25 Jóhann B. Guðmundsson 2011-07-12 19:41:08 EDT
Created attachment 512539 [details]
/etc/sysconfig file

/etc/sysconfig/pgsql/pgsql to go along with the standalone units..
Comment 26 Mathieu Bridon 2011-07-12 22:50:42 EDT
(In reply to comment #13)
> With the old sysv script it was possible to do ln -s /etc/init.d/postgresql
> /etc/init.d/pgfoo && echo -e "PGPORT=9999\nPGDATA=/srv/pgfoo" >
> /etc/sysconfig/pgsql/pgfoo
> 
> With the current patch this doesn't work anymore

I do that at $dayjob with the current patch:
$ cp /lib/systemd/system/{postgresql,pgfoo}.service
$ cp /usr/bin/{postgresql,pgfoo}-dbops

Then in those two files I modify the name of the /etc/sysconfig file that is used.

I agree with you that it's slightly more cumbersome than what you are currently doing with the SysV script, but I hadn't realized it could automatically detect what sysconfig file to use, so I didn't port it over. :)

(In reply to comment #14)
> (In reply to comment #13)
> > If systemd can't handle the old setup things will have to be documented at least.
> 
> I gather that /etc/sysconfig files are deprecated with systemd, but do we
> really need to remove the feature immediately?

My patches don't remove it, the unit file contains:
# Kept for compatibility with the Fedora SysV initscript for PostgreSQL: it
# would read this file to override its default options. As such, this line must
# be kept *after* the above `Environment=` lines.
EnvironmentFile=-/etc/sysconfig/pgsql/postgresql

> > I am not sure about the -c silent_mode=off -c log_destination=syslog option
> > either, you don't want to override a sysadmin setting which for example uses
> > csvlog to feed the output somehwere else?
> 
> I don't see a problem with silent_mode = off, except that it's probably useless
> because nobody is likely to try to turn that on.  (Also, upstream just deleted
> that setting as no longer of interest, which means the script would stop
> working as of PG 9.2, which seems like a bigger hazard than someone being silly
> and turning silent_mode on.)

Ouch, I didn't know it had been removed upstream. :-/

So, to make it clear, I used silent_mode=off because ideally with systemd, services don't daemonize themselves.

In PG 9.2, what is the behavior if this setting has been removed? PG will always daemonize itself? Or it will never do it? Is that the same behavior as the default one (i.e without specifying silent_mode=) of the current PG in Rawhide?

If it always daemonizes itself, then we will have to use Type=Forking, which is a bit less "systemd-esque" but should work perfectly.

> I agree that it is not the charter of this patch to change the database's
> default logging configuration, *especially* not in a non-overridable way, so
> there is zero chance I'll accept the proposed log_destination option.

It is overridable:
$ cp /{lib,etc}/systemd/system/postgresql.service

Then modify the log_destination in this new file.

I'm not saying this is ideal, just that it is possible. Now it's still up to you to decide if that is an acceptable way of doing things. :)

(In reply to comment #15)
> I dont have time to go through the whole patch just to take a look at how the
> unit file look like.

So others should do more work because you don't have time to do a bit?

Note that my patch adds a new unit file, so if you click on "diff" next to the patch in Bugzilla, you will see the unit file in plain text.

So no, I won't attach one more file as you can already look at it perfectly (unless you are color-blind and the green background is preventing you from reading the file, in which case I will happily oblige to your request).

I also doubt it is appropriate for you to provide new unit files without having read what has already been submitted.

(In reply to comment #21)
> My point is basically that the risk/reward ratio of having automatic initdb (or
> automatic pg_upgrade, either) is not appealing.

I agree that it should absolutely not be automatic.

I would even go further: this has nothing to do with systemd, it is not a service and should not be a unit file.

It is a one time manual operation, for which the obvious solution is IMHO a command-line tool. (which is what I provided)

> Another point is that, since database servers aren't started by default (and
> won't ever be, per Fedora policy), the time at which you might be deciding that
> you do want to run Postgres is not system boot time.  So putting this action
> into systemd's purview isn't really helpful anyway.  There's necessarily some
> manual action taken to enable the server to be started ("chkconfig on" or
> equivalent) and you can deal with initdb at the same time.

And even then, it's easy to add the required script as %post of your kickstart file (I know, I'm doing it :)

> The reason why it was part of the initscript was only that it made it easy to
> ensure that the same configuration information (particularly, database
> directory location and owning user) was fed into both the initdb action and
> subsequent server start actions.  I'm not sure how we preserve that ease-of-use
> property in the systemd world, if we kick initdb out of systemd's purview. 

Isn't the solution I provided acceptable?

With my patch, you get two possibilities:
1. The admin doesn't want systemd just yet and still uses a "traditional" init system.
   As such, he installs the postgresql-server-sysvinit package, then runs:
    $ service postgresql initdb
   which calls /usr/bin/postgresql-dbops initdb
   Then he can enable/start the service with chkconfig/service.
2. The admin is perfectly happy to run a full-systemd server.
   As such, he ony installs the postgresql-server package, then runs:
    $ /usr/bin/postgresql-dbops initdb
   Then he can enable/start the service with systemctl

So in the "legacy" case, nothing changes, and for the bravest out there, there is the exact same number of steps as before, except the first step has moved to a standalone command (which can and probably should be documented in the release notes).

> Possibly the thing to do is insist that there always be an /etc/sysconfig file
> and have the initdb script read that.

The patches I provided will read the sysconfig file if it exists, or use their default values if it doesn't. I agree this is not ideal as it means the default values are written both in the unit file and the init/upgrade script.

Making the sysconfig file mandatory doesn't strike me as a good solution either, as this is a Fedora specific configuration file, so all the work we're doing here wouldn't be upstreamable. :(

However, we already have the sysconfig file anyway, so I can live with that solution if you prefer it.

----

So if I sum this all up, the current issues with my patches are:
- running other instances with different PGPORT/PGDATA got a bit more complicated (comment #13)
    => I'll try to see if I can cook up something simpler
- silent_mode=off is a bad idea
    => can you answer my questions about that above?
- log_destination seems inappropriate in the unit file
    => are you okay with the way this can be overridden or would you still reject the patch?
- make /etc/sysconfig file mandatory
    => would you prefer that?
Comment 27 Honza Horak 2011-07-26 08:39:50 EDT
(In reply to comment #0)
>  postgresql.spec:487: E: hardcoded-library-path in
> $RPM_BUILD_ROOT/lib/systemd/system/postgresql.service
>  A library path is hardcoded to one of the following paths: /lib, /usr/lib. It
>  should be replaced by something like /%{_lib} or %{_libdir}.
> 
> I don't really know how to fix these yet. The location is right (those files
> are not arch specific) but it is kind of annoying to be polluting the rpmlint
> output like this. :-/

If we use "BuildRequires: systemd-units" (which is so), we can use macro %{_unitdir}, which is interpreted as /lib/systemd/system.
Comment 28 Honza Horak 2011-07-26 09:04:08 EDT
(In reply to comment #26)
> (In reply to comment #13)
> > With the old sysv script it was possible to do ln -s /etc/init.d/postgresql
> > /etc/init.d/pgfoo && echo -e "PGPORT=9999\nPGDATA=/srv/pgfoo" >
> > /etc/sysconfig/pgsql/pgfoo
> > 
> > With the current patch this doesn't work anymore
> 
> I do that at $dayjob with the current patch:
> $ cp /lib/systemd/system/{postgresql,pgfoo}.service
> $ cp /usr/bin/{postgresql,pgfoo}-dbops
> 
> Then in those two files I modify the name of the /etc/sysconfig file that is
> used.
> 
> I agree with you that it's slightly more cumbersome than what you are currently
> doing with the SysV script, but I hadn't realized it could automatically detect
> what sysconfig file to use, so I didn't port it over. :)

Thanks to Jóhann and bug #698755, we can use unit name in EnvironmentFile= parameter as well as in ExecStart=. It is already included in Fedora Rawhide, but not in F15 (which doesn't matter).

e.g. if we use 
EnvironmentFile=/etc/sysconfig/pgsql/%p
it will be interpreted as 
"/etc/sysconfig/pgsql/postgresql" in "postgresql.service", 
"/etc/sysconfig/pgsql/postgresql-foo" in "postgresql-foo.service", etc. 

Using this we can do the same things like with former init script:

# cp /lib/systemd/system/postgresql.service /etc/systemd/system/pgfoo.service
# echo -e "PGPORT=9999\nPGDATA=/srv/pgfoo" > /etc/sysconfig/pgsql/pgfoo

and we're done (if we use the same concept in initdb/upgrade services).

If we use this approach, I'd recommend to make sysconfig files mandatory, because there will be probably some default configuration (doesn't matter where and how defined now) which would be used if a config file doesn't exist (user forget to create it). If sysconfig files aren't mandatory, a user would use this default config without noticing and would work with another database, than he thought.
Comment 29 Jóhann B. Guðmundsson 2011-07-26 09:11:24 EDT
(In reply to comment #28)
> (In reply to comment #26)
> > (In reply to comment #13)
<snip>
> If we use this approach, I'd recommend to make sysconfig files mandatory,
> because there will be probably some default configuration (doesn't matter where
> and how defined now) which would be used if a config file doesn't exist (user
> forget to create it). If sysconfig files aren't mandatory, a user would use
> this default config without noticing and would work with another database, than
> he thought.

You probably are looking to add this in the Unit section of the service file

ConditionPathExists=/etc/sysconfig/pgsql/%p

Then the service will fail to start if a %p matching sysconfig file does not exist
Comment 30 Honza Horak 2011-07-26 09:18:41 EDT
(In reply to comment #29)
> You probably are looking to add this in the Unit section of the service file
> 
> ConditionPathExists=/etc/sysconfig/pgsql/%p
> 
> Then the service will fail to start if a %p matching sysconfig file does not
> exist

This is not needed, if we don't use "-" in EnvironmentFile=, it will fail too.
Comment 31 Honza Horak 2011-07-26 09:25:15 EDT
(In reply to comment #26)
> So, to make it clear, I used silent_mode=off because ideally with systemd,
> services don't daemonize themselves.
> 
> In PG 9.2, what is the behavior if this setting has been removed? PG will
> always daemonize itself? Or it will never do it? Is that the same behavior as
> the default one (i.e without specifying silent_mode=) of the current PG in
> Rawhide?
> 
> If it always daemonizes itself, then we will have to use Type=Forking, which is
> a bit less "systemd-esque" but should work perfectly.
> 

silent_mode is off by default, there is no difference if we use silent_mode=off or not.

> I'm not saying this is ideal, just that it is possible. Now it's still up to
> you to decide if that is an acceptable way of doing things. :)

I don't actually understand why we should use log_destination in default unit file at all. I think we can use the same way of logging as it was used in former init script. We just need to separate some bash code and place it into ExecStartPre=. However, I haven't test this approach yet.

> I agree that it should absolutely not be automatic.
> 
> I would even go further: this has nothing to do with systemd, it is not a
> service and should not be a unit file.
> 
> It is a one time manual operation, for which the obvious solution is IMHO a
> command-line tool. (which is what I provided)

As I've understood, type "oneshot" in systemd is prepared exactly for these purposes and if we don't use [Install] section, service won't be able to be enabled. Then we can offer users very similar interface as it was offered by init script.
Comment 32 Honza Horak 2011-07-26 09:43:22 EDT
(In reply to comment #31)
> As I've understood, type "oneshot" in systemd is prepared exactly for these
> purposes and if we don't use [Install] section, service won't be able to be
> enabled. Then we can offer users very similar interface as it was offered by
> init script.

I think Jóhann's standalone units (attachment #512538 [details] and attachment #512537 [details]) are ok, but... if we need to use %p in the same way as in comment #28, user will copy them into /etc/systemd/system/ under names pg-foo-initdb.service. Then systemd will read /etc/sysconfig/pgsql/pgfoo-initdb instead of /etc/sysconfig/pgsql/pgfoo, which is wrong.

Because of that we should maybe use standalone scripts, kicked out of systemd's purview, as already suggested here. Then a user can either:
1) copy these scripts (and determine config file path in it)
or
2) just add a parameter to them:
# /usr/bin/postgresql-dbops initdb pgfoo

I think copying files with initdb code is needless duplicity, so I'd rather see #2.
Comment 33 Honza Horak 2011-07-26 09:54:44 EDT
Systemd offers one interesting option capable also for multiple instances of postgresql (read more about it in http://0pointer.de/public/systemd-man/systemd.unit.html).

We can create postgresql@.service unit file instead of postgresql.service and then use %i in ExecStart= and EnvironmentFile= arguments. A user then works with the specific instance always, e.g.:

systemctl start postgresql@foo.service
systemctl status postgresql@bar.service

I don't think we should support this feature by default, because majority of users probably use only a single instance and we'd need to use an instance always:

systemctl start postgresql@master.service
systemctl status postgresql@master.service

...which is not very comfortable.

But there can be a documentation and examples included in rpm.
Comment 34 Jóhann B. Guðmundsson 2011-07-26 10:11:18 EDT
Well I guess you could symbolic link that postgresql@master.service to postgresql.service which would make it transparent to the end user as in the end user would run systemctl start postgresql.service and then call systemctl start postgresql@foo.service and systemctl status postgresql@bar.service should he add more than one instance
Comment 35 Mathieu Bridon 2011-07-26 22:55:15 EDT
(In reply to comment #28)
> Thanks to Jóhann and bug #698755, we can use unit name in EnvironmentFile=
> parameter as well as in ExecStart=. It is already included in Fedora Rawhide,
> but not in F15 (which doesn't matter).
> 
> e.g. if we use 
> EnvironmentFile=/etc/sysconfig/pgsql/%p
> it will be interpreted as 
> "/etc/sysconfig/pgsql/postgresql" in "postgresql.service", 
> "/etc/sysconfig/pgsql/postgresql-foo" in "postgresql-foo.service", etc. 
> 
> Using this we can do the same things like with former init script:
> 
> # cp /lib/systemd/system/postgresql.service /etc/systemd/system/pgfoo.service
> # echo -e "PGPORT=9999\nPGDATA=/srv/pgfoo" > /etc/sysconfig/pgsql/pgfoo
> 
> and we're done (if we use the same concept in initdb/upgrade services).

Neat! I didn't know about %p, good catch.

> If we use this approach, I'd recommend to make sysconfig files mandatory,
> because there will be probably some default configuration (doesn't matter where
> and how defined now) which would be used if a config file doesn't exist (user
> forget to create it). If sysconfig files aren't mandatory, a user would use
> this default config without noticing and would work with another database, than
> he thought.

The major problem I think is that we need to share some config between the unit file and the dbops script (PGPORT and PGDATA).

Making sysconfig file mandatory seems like a good way to fix this problem now, but it should be thoroughly documented as a legitimate use, so that it isn't blindly removed in the future.

(In reply to comment #31)
> silent_mode is off by default, there is no difference if we use silent_mode=off
> or not.

Great, one less parameter to use in the unit file then. :)

> > I'm not saying this is ideal, just that it is possible. Now it's still up to
> > you to decide if that is an acceptable way of doing things. :)
> 
> I don't actually understand why we should use log_destination in default unit
> file at all.

During my initial testing I added it for a reason... which I obviously failed to remember or document. :-/

If that bothers people, let's just remove it.

> > I agree that it should absolutely not be automatic.
> > 
> > I would even go further: this has nothing to do with systemd, it is not a
> > service and should not be a unit file.
> > 
> > It is a one time manual operation, for which the obvious solution is IMHO a
> > command-line tool. (which is what I provided)
> 
> As I've understood, type "oneshot" in systemd is prepared exactly for these
> purposes and if we don't use [Install] section, service won't be able to be
> enabled. Then we can offer users very similar interface as it was offered by
> init script.

Yes, a oneshot service would work. I just don't see it as being a service operation. But that's not my call to make anyway, and we've already use a service for the SSH host keys creation anyway.
Comment 36 Tom Lane 2011-07-27 17:05:46 EDT
(In reply to comment #28)
> Thanks to Jóhann and bug #698755, we can use unit name in EnvironmentFile=
> parameter as well as in ExecStart=. It is already included in Fedora Rawhide,
> but not in F15 (which doesn't matter).
> 
> e.g. if we use 
> EnvironmentFile=/etc/sysconfig/pgsql/%p
> it will be interpreted as 
> "/etc/sysconfig/pgsql/postgresql" in "postgresql.service", 
> "/etc/sysconfig/pgsql/postgresql-foo" in "postgresql-foo.service", etc. 

This is sort of a cute idea, but the fact that it doesn't work in F15 is a big handicap for testing, at least for me.  (And even if Fedora policy prohibits us from dropping the updated RPMs into F15, you can bet that users will want to do it themselves, since systemd's sysv emulation is so crummy.)

What I'm currently thinking is that maybe we should give up on the /etc/sysconfig files, expect all these variables to be set in the service file, and have postgresql-dbops just grep them out of the service file.

BTW, I don't especially like the name "postgresql-dbops".  How does "postgresql-setup" sound?
Comment 37 Tom Lane 2011-07-27 19:55:50 EDT
I've committed this with some hacking, please test.
Comment 38 Fedora Update System 2011-07-27 19:58:10 EDT
postgresql-9.0.4-8.fc16 has been submitted as an update for Fedora 16.
https://admin.fedoraproject.org/updates/postgresql-9.0.4-8.fc16
Comment 39 Fedora Update System 2011-08-01 16:15:08 EDT
Package postgresql-9.0.4-8.fc16:
* should fix your issue,
* was pushed to the Fedora 16 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing postgresql-9.0.4-8.fc16'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/postgresql-9.0.4-8.fc16
then log in and leave karma (feedback).
Comment 40 Mathieu Bridon 2011-08-04 06:23:42 EDT
Just tried this on a Fedora 16 VM, and it seems to work nicely.

I haven't tested the upgrade from SysV init script to systemd unit file though, but looking at your commit it should work without any issue.

Thanks!
Comment 41 Mathieu Bridon 2011-08-18 03:36:45 EDT
Created attachment 518812 [details]
Add a setup service

(In reply to comment #35)
> (In reply to comment #28)
> > > I agree that it should absolutely not be automatic.
> > > 
> > > I would even go further: this has nothing to do with systemd, it is not a
> > > service and should not be a unit file.
> > > 
> > > It is a one time manual operation, for which the obvious solution is IMHO a
> > > command-line tool. (which is what I provided)
> > 
> > As I've understood, type "oneshot" in systemd is prepared exactly for these
> > purposes and if we don't use [Install] section, service won't be able to be
> > enabled. Then we can offer users very similar interface as it was offered by
> > init script.
> 
> Yes, a oneshot service would work. I just don't see it as being a service
> operation. But that's not my call to make anyway, and we've already use a
> service for the SSH host keys creation anyway.

So I've been thinking some more about this, and I completely changed my opinion.

Here at $dayjob, we make a RHEL- / Fedora-based OS with systemd, and I'm reusing the Fedora 16 PostgreSQL package. One thing we need is to do some setup operations at first boot, one of those being to initialize the PostgreSQL cluster.

And it's true that it is extremely handy to have a service file for that, and to enable it in the kickstart so that it runs at first boot.

What I ended up doing is a service that disables itself as part of ExecStartPost, so that it's only run once. Unless the admin wants to run it again of course, in which case he can start it manually or enable it again so it runs at next reboot.

I'm attaching the patch here, in case you want to add that to the Fedora package.
Comment 42 Honza Horak 2011-08-19 01:39:04 EDT
(In reply to comment #41)
> I'm attaching the patch here, in case you want to add that to the Fedora
> package.

The service file doesn't seem to be a bad idea, but I'd prefer not to install it by default.

OTOH, it's probably possible to add this file to the package with some comments how to install and use it, but only for voluntaries.
Comment 43 Fedora Update System 2011-08-22 11:11:25 EDT
postgresql-9.0.4-8.fc16 has been pushed to the Fedora 16 stable repository.  If problems still persist, please make note of it in this bug report.

Note You need to log in before you can comment on or make changes to this bug.