Bug 214663 - databases/postgresqlXX-server: Add FIB support to startup script
Summary: databases/postgresqlXX-server: Add FIB support to startup script
Status: Closed Feedback Timeout
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Michael Gmelin
URL:
Keywords: feature, needs-qa, patch
Depends on:
Blocks:
 
Reported: 2016-11-19 10:33 UTC by Ryder Dain
Modified: 2020-04-28 14:30 UTC (History)
5 users (show)

See Also:
koobs: maintainer-feedback? (pgsql)


Attachments
suggested patch for rc.d script (396 bytes, patch)
2016-11-19 10:33 UTC, Ryder Dain
no flags Details | Diff
More robust patch for using setfib (untested) (1.78 KB, patch)
2016-11-19 11:24 UTC, Michael Gmelin
no flags Details | Diff
Patch for all version of postgresqlXX-server, bump revision (10.01 KB, patch)
2016-11-20 11:44 UTC, Michael Gmelin
grembo: maintainer-approval? (scrappy)
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryder Dain 2016-11-19 10:33:26 UTC
Created attachment 177179 [details]
suggested patch for rc.d script

When restarting postgresql server via rc scripts, the server's process inherits the fib of the caller, ignoring any settings in /etc/rc.conf. During maintenance, this can cause headaches when simply restarting the database brings it up with the incorrect routing table.

This is likely due to the rc script's use of 'su -l ${postgresql_user} -c "exec ${command} ..." in the script.

A simple fix is to insert "setfib ${postgresql_fib}" in the exec statment ; when no fib is configured, this will default to 0 in any case without any error. This is provided by the patch below.

Caveat: this would break port installations on any OS prior to a base where /usr/sbin/setfib is not in use (but this was tested and found present on amd64 versions as early as 7.2 – may still want to modify to account for early OS edge cases rather than calling setfib always implicitly)
Comment 1 Michael Gmelin freebsd_committer 2016-11-19 10:42:30 UTC
This will break if no FIBs are used.

See here for a working example (documenting the option and adding pre_cmd to modify $command):

https://svnweb.freebsd.org/ports/head/devel/phabricator/files/phd.in?revision=366900&view=markup
Comment 2 Michael Gmelin freebsd_committer 2016-11-19 11:24:56 UTC
Created attachment 177180 [details]
More robust patch for using setfib (untested)

@Ryder: Could you try this patch? (untested)
Comment 3 Kubilay Kocak freebsd_committer freebsd_triage 2016-11-19 11:27:53 UTC
Should this feature/support be added to all postgresqlXX-server ports?
Comment 4 Michael Gmelin freebsd_committer 2016-11-19 11:33:09 UTC
(In reply to Kubilay Kocak from comment #3)

That would make sense.

I also feel like that it would be better to get rid of postgresql_command() and use the standard functions from /etc/rc.subr for setting _user, _fib etc., but I'm not using the port myself and I didn't check if that would lead to ill side-effects, therefore this more conservative approach.
Comment 5 Ryder Dain 2016-11-19 12:50:47 UTC
Thanks for catching my patch's error. It's why I shouldn't do this on Saturday mornings.

Attempted your patch as-is against a fresh portsnap, but there was an error:

$ sudo patch < ~/postrc.patch 
Hmm...  Looks like a unified diff to me...
The text leading up to this was:
--------------------------
|--- files/postgresql.in.orig    2016-11-19 12:03:22.581531987 +0100
|+++ files/postgresql.in 2016-11-19 12:22:09.606409987 +0100
--------------------------
Patching file files/postgresql.in using Plan A...
Hunk #1 succeeded at 14.
Hunk #2 succeeded at 38.
Hunk #3 failed at 64.
Hunk #4 succeeded at 106.
1 out of 4 hunks failed--saving rejects to files/postgresql.in.rej
done
$ sudo cat files/postgresql.in.rej 
@@ -62,6 +64,7 @@
        eval postgresql_data="\${postgresql_${profile}_data:-${postgresql_data}}"
        eval postgresql_flags="\${postgresql_${profile}_flags:-${postgresql_flags}}"
        eval postgresql_initdb_flags="\${postgresql_${profile}_initdb_flags:-${postgresql_initdb_flags}}"
+       eval postgresql_fib="\${postgresql_${profile}_fib:-${postgresql_fib}}"
    fi
 else
    if [ "x${postgresql_profiles}" != "x" -a "x$1" != "x" ]; then
Comment 6 Michael Gmelin freebsd_committer 2016-11-20 11:44:03 UTC
Created attachment 177204 [details]
Patch for all version of postgresqlXX-server, bump revision

@ryder Please find attached a new version of the patch that covers all versions of postgresqlXX (patch was generated against r426569 of HEAD)
Comment 7 Ryder Dain 2016-11-21 13:46:51 UTC
Checks out.
Comment 8 Walter Schwarzenfeld freebsd_triage 2018-01-17 07:33:11 UTC
Any news here?
Comment 9 Michael Gmelin freebsd_committer 2018-01-22 23:25:11 UTC
@w.schwarzenfeld@utanet.at Definitely still required

@pgsql Is there anything you can do about it?
Comment 10 Palle Girgensohn freebsd_committer 2018-01-23 11:17:53 UTC
Sorry for leaving this one around for so long. 

What is special with PostgreSQL that makes it require special attention WRT FIBs? Is it the exec call? that should perhaps be changed rather than the suggested solution? Otherwise, shouldn't all daemons have some similar FIB support?

Palle
Comment 11 Michael Gmelin freebsd_committer 2018-01-23 11:40:55 UTC
It's about its own special calls of exec (combined with su), overloading of start/restart/..., providing a special initdb target and the fact that it supports profiles (also compare with www/apache24's handling of the matter).

More simple daemons (those that don't replace start, restart etc.) are handled automatically by /etc/rc.subr, but if you role your own thing (overload commands and pass them into pg_ctl) you have to manage fibs yourself at the moment.

One could think of adding some support in /etc/rc.subr that allows overloading of start etc. more easily, so one can my setting the user, fib etc. while still running their own commands. Also, profiles could become a standardized feature, but all of this is outside of the scope of this PR.
Comment 12 Michael Gmelin freebsd_committer 2018-02-15 00:46:48 UTC
@Palle Any updates?
Comment 13 Palle Girgensohn freebsd_committer 2018-02-26 08:36:54 UTC
Hi,


Perhaps the su -l ... -c 'exec ...' is not the best way to start postgresql either way. I'll review this and see if there is simpler way, or else commit this suggestion.

Palle
Comment 14 Michael Gmelin freebsd_committer 2019-03-28 02:19:37 UTC
(In reply to Palle Girgensohn from comment #13)

Was this ever applied?
Comment 15 Palle Girgensohn freebsd_committer 2019-03-28 08:50:52 UTC
(In reply to Michael Gmelin from comment #14)

No, sadly not. Thanks for reminding me.

Palle
Comment 16 Michael Gmelin freebsd_committer 2019-09-03 14:28:52 UTC
(In reply to Palle Girgensohn from comment #15)

Any chance to fix this before the PRs third anniversary? ;)
Comment 17 Michael Gmelin freebsd_committer 2020-04-14 09:53:10 UTC
ping