Обсуждение: exposing pg_controldata and pg_config as functions
Is there any significant interest in either of these?
Josh Berkus tells me that he would like pg_controldata information, and
I was a bit interested in pg_config information, for this reason: I had
a report of someone who had configured using --with-libxml but the xml
tests actually returned the results that are expected without xml being
configured. The regression tests thus passed, but should not have. It
occurred to me that if we had a test like
select pg_config('configure') ~ '--with-libxml' as has_xml;
in the xml tests then this failure mode would be detected.
cheers
andrew
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/20/2015 06:59 AM, Andrew Dunstan wrote:
> Is there any significant interest in either of these?
>
> Josh Berkus tells me that he would like pg_controldata information,
> and I was a bit interested in pg_config information, for this
> reason: I had a report of someone who had configured using
> --with-libxml but the xml tests actually returned the results that
> are expected without xml being configured. The regression tests
> thus passed, but should not have. It occurred to me that if we had
> a test like
>
> select pg_config('configure') ~ '--with-libxml' as has_xml;
>
> in the xml tests then this failure mode would be detected.
I've found use for them both in the past. A fair amount of bit-rot has
set it no doubt, and these were quick and dirty to begin with, but I
have these hacks from a while back:
https://github.com/jconway/pg_config
https://github.com/jconway/pg_controldata
- --
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBAgAGBQJV1epAAAoJEDfy90M199hlDjkP/AjAMtF4Q4rwfy5CsqA1rCgX
E/hLoTxExwU11nS2Q6IxC1unsXCDQrr2lkutKsw5Ybo78O7pMvR39GqShQ6ItaOr
xLxkYmxU1pEC4MAzZ6TR7RCAiP5SOgDEC3X6ArBqc0zub6FrnuYI3zv8eIgcTWqT
cFan4vCHYZnWUp3KQ0sOpSl4I/5jeW3AwrfTlnwskC8NwpP0Oa0DiXXTtXoRUYZI
CaWUsV9FgfzGvhyQCJpwcldEU9TprU24U09CpIVzSmw6Q9eQBHO4k+nT/Xw3BRjH
LxPM7gH9LweQOJhzP3J8agrJqbnSntayPZKG9ZsqMvC/8Ly+mlIO/X4cuEYpKO94
De9jO+aly6NhUdCpOdM6cZdqsTggXExaafl61wazYBUcLWotBnL9I1E9n59fm+yu
wgec7vdWIzZn6FYr+Ox2sgOBbxXVs3l/FLTCkoUgsZWRvlEL/naePr5TxMJMyqpo
pt15r1WRd4KwDEN4qxYHrzOab/T7QG1RS9Qr2v0GMPvQp4lIXgCq2aJJXy+iCDPE
lifDpHipk39h0r0RN377UFmfW3Z8DNLj0UQpuw+bOXtFLZpcA4WQdg64qXBaUU26
7icScC7+PpEr+HialFyA8lbDb9EVRrUaJ6CJajrGy8iuH/vpME2+40sgFTvavZtk
a0mfnPIdWJjzkldZGZ23
=hbBg
-----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/20/2015 07:54 AM, Joe Conway wrote:
> On 08/20/2015 06:59 AM, Andrew Dunstan wrote:
>> I was a bit interested in pg_config information, for this reason:
>> I had a report of someone who had configured using --with-libxml
>> but the xml tests actually returned the results that are expected
>> without xml being configured. The regression tests thus passed,
>> but should not have. It occurred to me that if we had a test
>> like
>
>> select pg_config('configure') ~ '--with-libxml' as has_xml;
>
>> in the xml tests then this failure mode would be detected.
>
> I've found use for them both in the past. A fair amount of bit-rot
> has set it no doubt, and these were quick and dirty to begin with,
> but I have these hacks from a while back:
>
> https://github.com/jconway/pg_config
The attached implements pg_config as a backend SRF and a matching
system view. A few notes:
1) The syntax is a bit different than what Andrew proposed:
8<----------------
select setting ~ '--with-libxml' as has_xml
from pg_config
where name = 'CONFIGURE';
has_xml
- ---------
t
(1 row)
8<----------------
In particular note that the name values are all upper case to be
consistent with pg_config, and at least currently there is no version
of the function which accepts a name as an argument (didn't seem
worthwhile to me).
2) No docs or related regression test yet. I will do that if there is
enough interest in this getting committed. So far no one except Andrew
and I have chimed in.
3) Requires a catalog version bump (not in posted diff)
4) The static function cleanup_path() was borrowed from
src/bin/pg_config/pg_config.c
It is a small and stable function (no change since 2010 AFAICS), so
maybe not worth the effort, but I was wondering if it should be moved
to src/common somewhere and shared.
I will add this to the next commitfest. Comments/feedback encouraged.
Joe
- --
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBAgAGBQJV2PykAAoJEDfy90M199hlQW0P/1fLCtFe50wFanleOxo41aki
yR8uG5vUZCLosx7lYd4+LyeE2g+bfs+fK6XgL1qIafI0zyxQSAU8TtjsIPQjjsvU
rNn1MWRrlOLEfOMMzbJPo01w5wzLhBvFzrYQ8vVtvf+T2PzjbU1hTMOcmaeXv6If
jYv0KQDgPBk/VPZ0D7MI4nYXVzNSInDLD7TGEpoTQwZ0oqvZwScSXc933isoULB4
4isael+g6mQJNoPz+OQEhUSoC922mrGs12SarfHJiUqJs1/NleClRRZ/9llCBnb2
3+zW6cb4XNh8aVP33zTtCsbrio206VjumWUYMNs546+qChormBOnYtZiIVRNRnPk
z4x/vxuhXVndDp1VnE5V5mRiW3B8ABliBf1Bcnf/Z+Gxi84LaZVtmL2hJrmn7voT
EZsQn/gmpB6ThHKbOl3t060fGZ/RAPDUwOWoYUIVcohOQqxK/iIka0bFM5cnuXO0
8oJ7CFkPSW7kBPs3uPO4Psf/jzrfaK3b/ZfitoV77sMQiVCABlR3a8khw+cPBrok
av/1afnGfz6qSxsV8sAyKUmRZkLDtmT01GUHCuujof1PQ3tD8zVsQWI3r51UcGB3
tFKvvy9koTHEunqkU6yQrCWNOEzHpGXEa1RIV33Ywgh0deKVEU5EbfJF5iIHBgOy
dYf2PHbYW7F1RSqKnZIa
=A2+X
-----END PGP SIGNATURE-----
Вложения
On Sun, Aug 23, 2015 at 7:50 AM, Joe Conway <mail@joeconway.com> wrote: > 1) The syntax is a bit different than what Andrew proposed: > > 8<---------------- > select setting ~ '--with-libxml' as has_xml > from pg_config > where name = 'CONFIGURE'; > has_xml > - --------- > t > (1 row) > 8<---------------- > > In particular note that the name values are all upper case to be > consistent with pg_config, and at least currently there is no version > of the function which accepts a name as an argument (didn't seem > worthwhile to me). Compatibility by default with the binary pg_config makes sense, users could just wrap an SQL with lower() or upper() if needed. > 2) No docs or related regression test yet. I will do that if there is > enough interest in this getting committed. So far no one except Andrew > and I have chimed in. I think that's a good thing to have, now I have concerns about making this data readable for non-superusers. Cloud deployments of Postgres are logically going to block the access of this view. > 4) The static function cleanup_path() was borrowed from > > src/bin/pg_config/pg_config.c cleanup_path is perhaps a candidate for src/port/path.c? > > It is a small and stable function (no change since 2010 AFAICS), so > maybe not worth the effort, but I was wondering if it should be moved > to src/common somewhere and shared. > > I will add this to the next commitfest. Comments/feedback encouraged. + Datum pg_config(PG_FUNCTION_ARGS); + + PG_FUNCTION_INFO_V1(pg_config); The declaration of the function is not needed, PG_FUNCTION_INFO_V1 takes care of it. Regards, -- Michael
On 08/23/2015 08:58 PM, Michael Paquier wrote: >> 2) No docs or related regression test yet. I will do that if there is >> enough interest in this getting committed. So far no one except Andrew >> and I have chimed in. > I think that's a good thing to have, now I have concerns about making > this data readable for non-superusers. Cloud deployments of Postgres > are logically going to block the access of this view. I don't think it exposes any information of great security value. > + Datum pg_config(PG_FUNCTION_ARGS); > + > + PG_FUNCTION_INFO_V1(pg_config); > > The declaration of the function is not needed, PG_FUNCTION_INFO_V1 > takes care of it. Umm, we shouldn't be using PG_FUNCTION_INFO_V1 in backend code at all, IIRC. cheers andrew
Andrew Dunstan <andrew@dunslane.net> writes:
> On 08/23/2015 08:58 PM, Michael Paquier wrote:
>> I think that's a good thing to have, now I have concerns about making
>> this data readable for non-superusers. Cloud deployments of Postgres
>> are logically going to block the access of this view.
> I don't think it exposes any information of great security value.
We just had that kerfuffle about whether WAL compression posed a security
risk; doesn't that imply that at least the data relevant to WAL position
has to be unreadable by non-superusers?
regards, tom lane
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 08/24/2015 06:50 AM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 08/23/2015 08:58 PM, Michael Paquier wrote: >>> I think that's a good thing to have, now I have concerns about >>> making this data readable for non-superusers. Cloud deployments >>> of Postgres are logically going to block the access of this >>> view. > >> I don't think it exposes any information of great security >> value. > > We just had that kerfuffle about whether WAL compression posed a > security risk; doesn't that imply that at least the data relevant > to WAL position has to be unreadable by non-superusers? So pg_config might be fully unrestricted, but pg_controldata might need certain rows filtered based on superuser status? Do you think those rows should be present but redacted, or completely filtered out? Joe - -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJV2z3SAAoJEDfy90M199hlgmwP/iPI4gJAM00b1mWiPHYSEMjQ pdVgPkFgfGQKyTizo7rEv1nJTQI3J9aUD7hvqYvPGlSOum0xei17fiRUIKnfqGgZ 7aSuhc97gZ7U5LvDsClovEUDEon+RIibZAYHKnKv2qYDwO/ZvfdFFQNi9TV0eREi QrEYafNo3/PWqJtrJoqhXaXyXsZ33FKtaaesQZJXvUUkTaE42eviq0cPiz2lHEsq szlGBnPkBS3qthAusApetAobZH9OymL4yl1BWwmBl3d2nEvQ4OVFGWo195It4XyQ 98bMzXse0PvBuKkcKrlTjxPdtR9UE/2FHojh7VLaj+JQeCGjehXNuogGPr7XHNSu cbCvIWsxW7Vz1liwFxY9I7Aui6/4X/oPehrct4CqaihqoztP1JrkQpVJDBYWwAhH Q/sRe8gUY8AWQHQljt9nuZvXmEYBnFbSf8tWVZ3/yhU1fK9dcl9B5doIHwKQXXtW +BHx4mOX5gcSRvGQFkJO0auE3Y9dvfUtpV4xDC57OHekgKA+rZw/HtElwKIhgrHI QoCd9PpJdG3UngX7ffsRuhJIhTUCSOKA2AIdceRyH4UgtqtHLzSU1tom3XMcQD+f mJvlKMwSvqh2Qmd/ZiNhgN4APkGk1AmH26hMMhI9HIrAIghkmPDfssLxYcBgJyDd lt8dJLQDnaddFLuvdQww =KZVU -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 08/24/2015 04:38 AM, Andrew Dunstan wrote: > On 08/23/2015 08:58 PM, Michael Paquier wrote: >> + Datum pg_config(PG_FUNCTION_ARGS); + + >> PG_FUNCTION_INFO_V1(pg_config); >> >> The declaration of the function is not needed, >> PG_FUNCTION_INFO_V1 takes care of it. > > Umm, we shouldn't be using PG_FUNCTION_INFO_V1 in backend code at > all, IIRC. Right -- those lines are a cut-n-pasteo from the original pg_config as an extension code. Will fix. Thanks, Joe - -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJV20DqAAoJEDfy90M199hlA9wP/RtnahsLzmbsXfPssTGUfdHu nuiF5Sgpqn5/tMNakNVr/ACBiSZQeFUf3FQNRzyoOK6zMXCox4HbSFKi4u0UxpYV CZZgIKByf4xaHjaZGpnY5dTteBQXdRv3Dp85hhmVbDHbO80+7e7zf0BI5QrUm14E Nhv6PJn6NMBm/GJrvm76+lDt7DJWYygi/3Jupn/aQNpPCZ5bHP+e4e/NC2FMtW3y Knm+KN5YEA0IWZKnM0s9kIfYeI9PE2tsF3jpdw8U7BTzziLf/6yTVXlJ/5xj0CfU 1kubgcZTp5UhDOWD7RuRQ6WUzbye/Yd/+9C9SNYltidZY7tnlbRyb0J6QerXNakM tM+eXAbroXnfAZulq8YJO8nFPviXh6Y4F1pEXtpVJIuLTu9NXEDhRLPAzsSXciCa yQuq98L2UmzpSr9i5ETMmjb7mUPcS9/IR/FQldNgP1/ARY2CTyL6hbdCJH8QieVo plEUaYPz4QbKTyF/OZsuamDSpqun412Zs/LRgF5kQhIcI1Q0z9SJ4GwQUZb/bwQm c0ztQnW8AKtzBgGVCYoJSKd4bD5w/Qtv8WdoZJzXnu3GOvq/laS+kCaBcYl3N7Rf dwDKpYmWitmyIT0THzhdCiJ38rMgq/JjmhCJQiMJJxvvsadl/mPKUO6k4ZUMNw6O BKrHf/JETN/Wnqd1IPqq =ohKC -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 08/24/2015 08:52 AM, Joe Conway wrote:
> On 08/24/2015 06:50 AM, Tom Lane wrote:
>> Andrew Dunstan <andrew@dunslane.net> writes:
>>> On 08/23/2015 08:58 PM, Michael Paquier wrote:
>>>> I think that's a good thing to have, now I have concerns
>>>> about making this data readable for non-superusers. Cloud
>>>> deployments of Postgres are logically going to block the
>>>> access of this view.
>
>>> I don't think it exposes any information of great security
>>> value.
>
>> We just had that kerfuffle about whether WAL compression posed a
>> security risk; doesn't that imply that at least the data
>> relevant to WAL position has to be unreadable by non-superusers?
>
> So pg_config might be fully unrestricted, but pg_controldata might
> need certain rows filtered based on superuser status? Do you think
> those rows should be present but redacted, or completely filtered
> out?
Here is the next installment on this. It addresses most of the
previous comments, but does not yet address the issue raised here by Tom
.
Changes:
1.) pg_controldata() function and pg_controldata view added
2.) cleanup_path() moved to port/path.c
3.) extra PG_FUNCTION_INFO_V1() noise removed
Issues needing comment:
a.) Which items need hiding from non-superusers and should the value be
redacted or the entire result set row be suppressed?
b.) There is a difference in the formatting of timestamps between the
pg_controldata binary and the builtin function. To see it do:
diff -c <(pg_controldata) \
<(psql -qAt -c "select rpad(name || ':', 38) || setting
from pg_controldata")
What I see is:
pg_controldata
! pg_control last modified: Tue 25 Aug 2015 08:10:42 PM PDT
pg_controldata()
! pg_control last modified: Tue Aug 25 20:10:42 2015
Does it matter?
c.) There is some common code between pg_controldata.c in bin and
pg_controldata.c in backend/utils/misc. Specifically the string
definitions in printf statements match those in ControlData[], and
dbState() and wal_level_str() are in both places. Any opinions
on consolidating them in src/common somewhere?
d.) Still no docs yet - will get to it eventually.
Thanks,
Joe
- --
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.22 (GNU/Linux)
iQIcBAEBAgAGBQJV3TNnAAoJEDfy90M199hl0OsQAIyTr0hqxwjrGenDpnS4qE8u
UJWVeCpqehFIobxcJ0TICTaQX835fzPGJIiTUwI1Dmz9sgjipvSG1wBmD4bRT93X
WO4e/+Yr5onZ9vNVXlUswPK2kuzehImhmzMSnJ6KDXKkfw2MOZmz56wBb3yIB3lq
K44FDukZ01w9RCGM8H5B/MPNMHIqfBB4wdlKARJZhqeUyPvTJ71iMiqeE77v3AIH
JLmW6kRw8c3NVu/Wa+GVz4FGjIZKR5oazlFYfDTeHXrxV8NIDUFNrKikAW1ScdVK
qSPVjFxoUlbX4W2dd1L1ciGeq83DktYbdKtpZZScQGXwhuq7Y1fHZQwzlxlraB/c
UiqNdxmi7IeUdOIncsKPDmjs7C5yeNj1CRnWHTAQRW98RM42A3TvT2Qlkxm0CVLQ
lZjFVOOMIf4pXYQv6PfiicO6QWYTUSXCa891s/10H2xkS/sMK1yHz3DWSZxVdDdI
dbh5gie/GFro1nwWd8gjkn5KCe917GDBAUBn+QE5TgUPnRhserq6FQBSyVXfZtOQ
o6nRM8vuv9Y06CRoeIgagtDWxippl0OAw442wHyme/PBQZ2842PW8GNNqw+B1HWz
Ir0V5FiZdLLQipwiKT152+8OsOa/NU6wxGFuJr8It/4471h3jU5dxuHO+wQqMDEb
xCn6ebwZaa9oSjHFrfy3
=oMOO
-----END PGP SIGNATURE-----
Вложения
* Joe Conway (mail@joeconway.com) wrote:
> Issues needing comment:
> a.) Which items need hiding from non-superusers and should the value be
> redacted or the entire result set row be suppressed?
I'm of the opinion that we need to at least redact it and that what we
should do is simply suppress the entire result set until we provide a
way for administrators to manage who can access it (eg: default roles,
this one would fall under 'pg_monitor', imo).
> b.) There is a difference in the formatting of timestamps between the
> pg_controldata binary and the builtin function. To see it do:
>
> diff -c <(pg_controldata) \
> <(psql -qAt -c "select rpad(name || ':', 38) || setting
> from pg_controldata")
>
> What I see is:
>
> pg_controldata
> ! pg_control last modified: Tue 25 Aug 2015 08:10:42 PM PDT
> pg_controldata()
> ! pg_control last modified: Tue Aug 25 20:10:42 2015
>
> Does it matter?
I don't think we can help that, can we? At the least, the
pg_controldata() output should match what the GUCs and whatnot tell us
to do, but the pg_controldata file needs to include things like the
timezone. In the end, I don't believe we need to make them match and
trying to would just make things ugly.
> c.) There is some common code between pg_controldata.c in bin and
> pg_controldata.c in backend/utils/misc. Specifically the string
> definitions in printf statements match those in ControlData[], and
> dbState() and wal_level_str() are in both places. Any opinions
> on consolidating them in src/common somewhere?
Haven't got any great ideas about exactly how to consolidate them, but I
do think it'd be good to do so..
Thanks!
Stephen
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 08/26/2015 06:33 AM, Stephen Frost wrote: > * Joe Conway (mail@joeconway.com) wrote: >> Issues needing comment: a.) Which items need hiding from >> non-superusers and should the value be redacted or the entire >> result set row be suppressed? > > I'm of the opinion that we need to at least redact it and that what > we should do is simply suppress the entire result set until we > provide a way for administrators to manage who can access it (eg: > default roles, this one would fall under 'pg_monitor', imo). Whatever it is it would have to be available during initdb. And in any case I'm no closer to knowing which rows to hide/redact/suppress other than WAL position. Possibly the thing to do for now would be to revoke public from these? Joe - -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJV3c3nAAoJEDfy90M199hlMnYQAJliTc7bJTCndMQ0emN6xV55 DqODtxABxh3kqPmWcvSO08dSZ5yHpCKYgIm8OmRIpfDUwNID1uBXsO5pRd1XVzLr 42OmQ9QauAZ9+f/Rea668U/zgzhnIJXdVsFfAmum516UoR3W1rqW5ggPKgN5YDhC 9Z6ikL1fs6l6l1yrvaefbepS1FLx6wDplhctN+hbEdHw9gwAf67fv7ncaPZ4BRyc hogL4mXoz0fFQz7RDvnR2g0uu17k3imbwzqGiyJwH4+9cfnNLWrBXupKwC06ufWF t3cLh4lLTUhx/2amB0qKMQp1MgVs6r70f5ciFTWvaO0nro0wSGHnIsnqFDOfnv2X kctZreHs7gDAFXWM4Qp45oxTHy6Lfce75IvDfZGZ3y8NOhEHZDqJs6VIdOgCu4h0 RkJE/RrRz7ZtMAhyokxWMZvffYRutLPbXAUvg6TBeDVy1T7SKoQK81IBz/Nkd+Bm WkB/EFklUZw/B2HnDpXRV3tdjAzMAJw22bQi0Y7515K25w7NC2nquzX1eBMGmaqe yDf/gobFg601E9WMjaNoxMGy3Niigk46UsQLGT7RJ/ciojY1gGQh/qd4b1BeJpM0 kRmj0Jsyn0cO8hs6h7jBNBVJjlBhr9ijd4tWaZAk9XqLExPPmGunhcoOMf6ttmvy 533U1P2OKyGBZZissMd4 =dlGD -----END PGP SIGNATURE-----
* Joe Conway (mail@joeconway.com) wrote:
> On 08/26/2015 06:33 AM, Stephen Frost wrote:
> > * Joe Conway (mail@joeconway.com) wrote:
> >> Issues needing comment: a.) Which items need hiding from
> >> non-superusers and should the value be redacted or the entire
> >> result set row be suppressed?
> >
> > I'm of the opinion that we need to at least redact it and that what
> > we should do is simply suppress the entire result set until we
> > provide a way for administrators to manage who can access it (eg:
> > default roles, this one would fall under 'pg_monitor', imo).
>
> Whatever it is it would have to be available during initdb. And in any
> case I'm no closer to knowing which rows to hide/redact/suppress other
> than WAL position. Possibly the thing to do for now would be to revoke
> public from these?
That was my thinking- revoke public from them. The default roles, based
on the last patch anyway, are available at initdb time and when
system_views.sql is run.
Thanks!
Stehpen
On 8/20/15 9:59 AM, Andrew Dunstan wrote:
> The regression tests thus passed, but should not have. It occurred to me
> that if we had a test like
>
> select pg_config('configure') ~ '--with-libxml' as has_xml;
>
> in the xml tests then this failure mode would be detected.
This particular case could probably be addressed in a less roundabout
way by enhancing the mapping mechanism in pg_regress.
On 8/24/15 9:50 AM, Tom Lane wrote: > Andrew Dunstan <andrew@dunslane.net> writes: >> On 08/23/2015 08:58 PM, Michael Paquier wrote: >>> I think that's a good thing to have, now I have concerns about making >>> this data readable for non-superusers. Cloud deployments of Postgres >>> are logically going to block the access of this view. > >> I don't think it exposes any information of great security value. > > We just had that kerfuffle about whether WAL compression posed a security > risk; doesn't that imply that at least the data relevant to WAL position > has to be unreadable by non-superusers? We already have functions that expose the current (or recent, or interesting) WAL position, so any new ones should probably follow the existing ones. Or possibly we don't need any new ones, because we already have enough?
On 8/25/15 11:32 PM, Joe Conway wrote: > 1.) pg_controldata() function and pg_controldata view added I don't think dumping out whatever pg_controldata happens to print as a bunch of text fields is very sophisticated. We have functionality to compute with WAL positions, for example, and they won't be of much use if this is going to be all text. Also, the GUC settings tracked in pg_control can already be viewed using normal mechanisms, so we don't need a second way to see them. The fact that some of this is stored in pg_control and some is not is really an implementation detail. We should be thinking of ways to expose specific useful information in useful ways, not just dump out everything we can find. Ultimately, I think we would like to move away from people parsing textual pg_controldata output, but this proposal is not moving very far in that direction.
On Mon, Aug 31, 2015 at 9:47 PM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 8/25/15 11:32 PM, Joe Conway wrote: >> 1.) pg_controldata() function and pg_controldata view added > > I don't think dumping out whatever pg_controldata happens to print as a > bunch of text fields is very sophisticated. We have functionality to > compute with WAL positions, for example, and they won't be of much use > if this is going to be all text. > > Also, the GUC settings tracked in pg_control can already be viewed using > normal mechanisms, so we don't need a second way to see them. > > The fact that some of this is stored in pg_control and some is not is > really an implementation detail. We should be thinking of ways to > expose specific useful information in useful ways, not just dump out > everything we can find. Ultimately, I think we would like to move away > from people parsing textual pg_controldata output, but this proposal is > not moving very far in that direction. The nice thing about dumping the information as text is that you can return every value in the same universal format: text. There's a lot to like about that. But I'm not sure I like the idea of adding a server dependency on the ability to exec pg_controldata. That seems like it could be unreliable at best, and a security vulnerability at worst. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> writes:
> But I'm not sure I like the idea of adding a server dependency on the
> ability to exec pg_controldata. That seems like it could be
> unreliable at best, and a security vulnerability at worst.
I hadn't been paying attention --- the proposed patch actually depends on
exec'ing pg_controldata? That's horrid! There is no expectation that
that's installed.
regards, tom lane
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 09/02/2015 05:25 PM, Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> But I'm not sure I like the idea of adding a server dependency on >> the ability to exec pg_controldata. That seems like it could be >> unreliable at best, and a security vulnerability at worst. > > I hadn't been paying attention --- the proposed patch actually > depends on exec'ing pg_controldata? That's horrid! There is no > expectation that that's installed. No it doesn't. I'm confused :-/ Joe - -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJV52qoAAoJEDfy90M199hltvwP/3MfUQfPPglhBuY1V3CTzHu9 kTw5tNuGI/244Yc11wLtV07W+3QWXzCNY/fL1JCW5ns51mTfZKfkskNWD0O9fIex gvK4p/3Z+y344qsdDlzbzw0A/PU05UCq1UlgXCF6nyQJW6cZfaCckbEpZWVK/uV7 aYokIqnIiilWaPu224b6jBOukK7oizEjXevdFBafLbetLJHMx+9k8LMbpPdieAm/ RSk17+N77WQ2zTFHcdz8U1MYAbaokmv155s1vUFgrqOUJGc0r6K+vImKgxOjbbmg pv2jf7vvUwwjUy7f2iPhWJAKfGCV1m9ovWaXsMYqcF55JwSzvP8B2htUtM4Lr1qF SsWO7e36bLoH++yAGfKp7oZIhA9r6SR6cwEoCvso3immZ2zhOzbRcw4tI4pE9fhB P/mEbKFF5BsGHjeslB8RrMQG68DxEwPkaafH4mc1QjKiXNfWPH9ci+pgfSLVphJq gn+ZuPrReIFhQKyMchcvZVVWJd9Dt02D2lsIzUfBWGXwOTLFVikD6BC6siy5KWmy xuEGLEfts9E7gPD3qPXxNuY7TCvb+L7R+1C9/M5diiV7rerMUocH/RqrPP6nXHTc BdfJhzOfU+H+Kt0nbdE8Vjw3BOKT6nqT0kc+le+F/Q1h2XLB63KhaOkFzVW73Rfd JRRqkyks+eVgEn2I4OKm =OAms -----END PGP SIGNATURE-----
Tom Lane wrote: > Robert Haas <robertmhaas@gmail.com> writes: > > But I'm not sure I like the idea of adding a server dependency on the > > ability to exec pg_controldata. That seems like it could be > > unreliable at best, and a security vulnerability at worst. > > I hadn't been paying attention --- the proposed patch actually depends on > exec'ing pg_controldata? That's horrid! There is no expectation that > that's installed. No, it doesn't. For the pg_controldata output it processes the pg_control file directly, and for pg_config it relies on compile-time CPPFLAGS. I think trying to duplicate the exact strings isn't too nice an interface. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 09/02/2015 02:34 PM, Alvaro Herrera wrote: > Tom Lane wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> But I'm not sure I like the idea of adding a server dependency on the >>> ability to exec pg_controldata. That seems like it could be >>> unreliable at best, and a security vulnerability at worst. >> >> I hadn't been paying attention --- the proposed patch actually depends on >> exec'ing pg_controldata? That's horrid! There is no expectation that >> that's installed. > > No, it doesn't. For the pg_controldata output it processes the > pg_control file directly, and for pg_config it relies on compile-time > CPPFLAGS. > > I think trying to duplicate the exact strings isn't too nice an > interface. Well, for pg_controldata, no, but what else would you do for pg_config? -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
Josh Berkus wrote: > On 09/02/2015 02:34 PM, Alvaro Herrera wrote: > > I think trying to duplicate the exact strings isn't too nice an > > interface. > > Well, for pg_controldata, no, but what else would you do for pg_config? I was primarily looking at pg_controldata, so we agree there. As for pg_config, I'm confused about its usefulness -- which of these lines are useful in the SQL interface? Anyway, I don't see anything better than a couple of text columns for this case. BINDIR = /home/alvherre/Code/pgsql/install/master/bin DOCDIR = /home/alvherre/Code/pgsql/install/master/share/doc HTMLDIR = /home/alvherre/Code/pgsql/install/master/share/doc INCLUDEDIR = /home/alvherre/Code/pgsql/install/master/include PKGINCLUDEDIR = /home/alvherre/Code/pgsql/install/master/include INCLUDEDIR-SERVER = /home/alvherre/Code/pgsql/install/master/include/server LIBDIR = /home/alvherre/Code/pgsql/install/master/lib PKGLIBDIR = /home/alvherre/Code/pgsql/install/master/lib LOCALEDIR = /home/alvherre/Code/pgsql/install/master/share/locale MANDIR = /home/alvherre/Code/pgsql/install/master/share/man SHAREDIR = /home/alvherre/Code/pgsql/install/master/share SYSCONFDIR = /home/alvherre/Code/pgsql/install/master/etc PGXS = /home/alvherre/Code/pgsql/install/master/lib/pgxs/src/makefiles/pgxs.mk CONFIGURE = '--enable-debug' '--enable-depend' '--enable-cassert' '--enable-nls' '--cache-file=/home/alvherre/tmp/pgconfig.master.cache''--enable-thread-safety' '--with-python' '--with-perl' '--with-tcl''--with-openssl' '--with-libxml' '--with-tclconfig=/usr/lib/tcl8.6' '--enable-tap-tests' '--prefix=/pgsql/install/master''--with-pgport=55432' CC = gcc CPPFLAGS = -D_GNU_SOURCE -I/usr/include/libxml2 CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g -O2 CFLAGS_SL = -fpic LDFLAGS = -L../../../src/common -Wl,--as-needed -Wl,-rpath,'/pgsql/install/master/lib',--enable-new-dtags LDFLAGS_EX = LDFLAGS_SL = LIBS = -lpgcommon -lpgport -lxml2 -lssl -lcrypto -lz -lreadline -lrt -lcrypt -ldl -lm VERSION = PostgreSQL 9.6devel -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Sep 2, 2015 at 5:31 PM, Joe Conway <mail@joeconway.com> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 09/02/2015 05:25 PM, Tom Lane wrote: >> Robert Haas <robertmhaas@gmail.com> writes: >>> But I'm not sure I like the idea of adding a server dependency on >>> the ability to exec pg_controldata. That seems like it could be >>> unreliable at best, and a security vulnerability at worst. >> >> I hadn't been paying attention --- the proposed patch actually >> depends on exec'ing pg_controldata? That's horrid! There is no >> expectation that that's installed. > > No it doesn't. I'm confused :-/ No, I'm confused. Sorry. Somehow I misread your patch. Pay no attention to that man behind the curtain. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 09/02/2015 02:54 PM, Alvaro Herrera wrote: > Josh Berkus wrote: >> On 09/02/2015 02:34 PM, Alvaro Herrera wrote: >>> I think trying to duplicate the exact strings isn't too nice an >>> interface. >> >> Well, for pg_controldata, no, but what else would you do for pg_config? > > I was primarily looking at pg_controldata, so we agree there. > > As for pg_config, I'm confused about its usefulness -- which of these > lines are useful in the SQL interface? Anyway, I don't see anything > better than a couple of text columns for this case. There are production environments where even the superuser has no direct, local, command line access on production database servers (short of intentional hacking, which would be frowned upon at best), and the only interface for getting information from postgres is via a database connection. So to the extent pg_config and pg_controldata command line binaries are useful, so is the ability to get the same output via SQL. Given that, my own feeling is that if we provide a SQL interface at all, it ought to be pretty much the exact same output as the command line programs produce. To the extent that we want specific pg_controldata output in non-text form, we should identify which items those are and provide individual functions for them. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 9/6/15 3:34 PM, Joe Conway wrote: > On 09/02/2015 02:54 PM, Alvaro Herrera wrote: >> Josh Berkus wrote: >>> On 09/02/2015 02:34 PM, Alvaro Herrera wrote: >>>> I think trying to duplicate the exact strings isn't too nice an >>>> interface. >>> >>> Well, for pg_controldata, no, but what else would you do for pg_config? >> >> I was primarily looking at pg_controldata, so we agree there. >> >> As for pg_config, I'm confused about its usefulness -- which of these >> lines are useful in the SQL interface? Anyway, I don't see anything >> better than a couple of text columns for this case. > > There are production environments where even the superuser has no > direct, local, command line access on production database servers But then they also have no use for the information that pg_config prints out. > and the > only interface for getting information from postgres is via a database > connection. So to the extent pg_config and pg_controldata command line > binaries are useful, so is the ability to get the same output via SQL. > > Given that, my own feeling is that if we provide a SQL interface at all, > it ought to be pretty much the exact same output as the command line > programs produce. That argument makes no sense to me. Again, we need to think about what actual use there is for this information. Just because the information exists somewhere, but you can't access it, doesn't mean we just need to copy it around.
On 09/06/2015 11:17 PM, Peter Eisentraut wrote: > On 9/6/15 3:34 PM, Joe Conway wrote: >> On 09/02/2015 02:54 PM, Alvaro Herrera wrote: >>> Josh Berkus wrote: >>>> On 09/02/2015 02:34 PM, Alvaro Herrera wrote: >>>>> I think trying to duplicate the exact strings isn't too nice an >>>>> interface. >>>> Well, for pg_controldata, no, but what else would you do for pg_config? >>> I was primarily looking at pg_controldata, so we agree there. >>> >>> As for pg_config, I'm confused about its usefulness -- which of these >>> lines are useful in the SQL interface? Anyway, I don't see anything >>> better than a couple of text columns for this case. >> There are production environments where even the superuser has no >> direct, local, command line access on production database servers > But then they also have no use for the information that pg_config prints > out. > >> and the >> only interface for getting information from postgres is via a database >> connection. So to the extent pg_config and pg_controldata command line >> binaries are useful, so is the ability to get the same output via SQL. >> >> Given that, my own feeling is that if we provide a SQL interface at all, >> it ought to be pretty much the exact same output as the command line >> programs produce. > That argument makes no sense to me. > > Again, we need to think about what actual use there is for this > information. Just because the information exists somewhere, but you > can't access it, doesn't mean we just need to copy it around. I already gave a use case that you dismissed in favour of a vague solution that we don't actually have. You seem to be the only person objecting to this proposal. cheers andrew
Andrew Dunstan wrote: > I already gave a use case that you dismissed in favour of a vague solution > that we don't actually have. You seem to be the only person objecting to > this proposal. I think that use case would be better served by a completely different interface -- some way to query the server, "does this installation support feature X?" What you proposed, using a regexp to look for --enable-xml in the pg_config --configure output, doesn't look all that nice to me. For instance, we already have sql_features.txt, which is exposed as a table in the docs. It's not quite the same thing (because what you want is not the same as conformance to the SQL standard), but I think a system view that has a list of features and a boolean flag for each one is more practical (though admittedly it's more work to implement.) Another alternative which I think is simpler is a read-only GUC, which we already have for a number of compile-time properties. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 09/06/2015 12:34 PM, Joe Conway wrote: > To the extent that we want specific pg_controldata output in non-text > form, we should identify which items those are and provide individual > functions for them. Well, I think it's pretty simple, let's take it down: # function pg_control_control_data() returning INT, TSTZ pg_control version number: 942 pg_control last modified: Thu 20 Aug 2015 10:05:33 AM PDT #function pg_catversion() returning BIGINT Catalog version number: 201409291 # have function for this, no? Database system identifier: 6102142380557650900 # not relevant, if we can connect, it's running Database cluster state: shut down # Do we have functions for all of the below? # if not I suggest virtual table pg_checkpoint_status # returning the below 17 columns # that would be useful even if we have some of them Latest checkpoint location: 0/1A1BF178 Prior checkpoint location: 0/1A1BF0D8 Latest checkpoint's REDO location: 0/1A1BF178 Latest checkpoint's REDO WAL file: 00000001000000000000001A Latest checkpoint's TimeLineID: 1 Latest checkpoint's PrevTimeLineID: 1 Latest checkpoint's full_page_writes: on Latest checkpoint's NextXID: 0/2038 Latest checkpoint's NextOID: 19684 Latest checkpoint's NextMultiXactId: 1 Latest checkpoint's NextMultiOffset: 0 Latest checkpoint's oldestXID: 711 Latest checkpoint's oldestXID's DB: 1 Latest checkpoint's oldestActiveXID: 0 Latest checkpoint's oldestMultiXid: 1 Latest checkpoint's oldestMulti's DB: 1 Time of latest checkpoint: Thu 20 Aug 2015 10:05:33 AM PDT # Not in any way useful Fake LSN counter for unlogged rels: 0/1 # add another system view, # pg_recovery_state, holding the # below 5 columns Minimum recovery ending location: 0/0 Min recovery ending loc's timeline: 0 Backup start location: 0/0 Backup end location: 0/0 End-of-backup record required: no # duplicates system settings, not needed Current wal_level setting: logical Current wal_log_hints setting: off Current max_connections setting: 100 Current max_worker_processes setting: 8 Current max_prepared_xacts setting: 0 Current max_locks_per_xact setting: 64 Maximum data alignment: 8 Database block size: 8192 # do we have the below anywhere else? # this is somewhat duplicative of config info Blocks per segment of large relation: 131072 WAL block size: 8192 Bytes per WAL segment: 16777216 Maximum length of identifiers: 64 Maximum columns in an index: 32 Maximum size of a TOAST chunk: 1996 Size of a large-object chunk: 2048 Date/time type storage: 64-bit integers Float4 argument passing: by value Float8 argument passing: by value # return INT function pg_data_page_checksum_version() Data page checksum version: 0 -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com
On Sun, Sep 6, 2015 at 3:34 PM, Joe Conway <mail@joeconway.com> wrote: > On 09/02/2015 02:54 PM, Alvaro Herrera wrote: >> Josh Berkus wrote: >>> On 09/02/2015 02:34 PM, Alvaro Herrera wrote: >>>> I think trying to duplicate the exact strings isn't too nice an >>>> interface. >>> >>> Well, for pg_controldata, no, but what else would you do for pg_config? >> >> I was primarily looking at pg_controldata, so we agree there. >> >> As for pg_config, I'm confused about its usefulness -- which of these >> lines are useful in the SQL interface? Anyway, I don't see anything >> better than a couple of text columns for this case. > > There are production environments where even the superuser has no > direct, local, command line access on production database servers (short > of intentional hacking, which would be frowned upon at best), and the > only interface for getting information from postgres is via a database > connection. So to the extent pg_config and pg_controldata command line > binaries are useful, so is the ability to get the same output via SQL. I don't buy that argument as far as pg_config is concerned. That's mostly providing local pathnames. If you don't have command-line access to the box where the server is running, you not only can't use that information, but you probably aren't really entitled to it. I see exposing the pg_controldata information as reasonable because that's actually facts about the database cluster, rather than the system on which it is running. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 9/7/15 7:32 PM, Alvaro Herrera wrote: > Andrew Dunstan wrote: > >> I already gave a use case that you dismissed in favour of a vague solution >> that we don't actually have. You seem to be the only person objecting to >> this proposal. > > I think that use case would be better served by a completely different > interface -- some way to query the server, "does this installation > support feature X?" What you proposed, using a regexp to look for > --enable-xml in the pg_config --configure output, doesn't look all that > nice to me. Agreed.
On 9/7/15 7:21 PM, Andrew Dunstan wrote: > I already gave a use case that you dismissed in favour of a vague > solution that we don't actually have. But that's also the only use case so far, which seems a little thin.
On 09/08/2015 04:21 PM, Peter Eisentraut wrote: > On 9/7/15 7:32 PM, Alvaro Herrera wrote: >> Andrew Dunstan wrote: >> >>> I already gave a use case that you dismissed in favour of a vague solution >>> that we don't actually have. You seem to be the only person objecting to >>> this proposal. >> I think that use case would be better served by a completely different >> interface -- some way to query the server, "does this installation >> support feature X?" What you proposed, using a regexp to look for >> --enable-xml in the pg_config --configure output, doesn't look all that >> nice to me. > Agreed. > > The problem is that at least this user's system had something odd about it. so that I wouldn't entirely trust the output of select is_supported from information_schema.sql_features where feature_name = 'XML type'; to reflect the config. I also have cases where clients don't want to give me superuser access, and sometimes not even shell access, and it could well be useful to me to be able to say to them "OK, you need to make sure that this file in this location has this entry". cheers andrew
On 9/8/15 4:56 PM, Andrew Dunstan wrote:
> The problem is that at least this user's system had something odd about
> it. so that I wouldn't entirely trust the output of
>
> select is_supported
> from information_schema.sql_features
> where feature_name = 'XML type';
>
> to reflect the config.
This should be a built-in function, not dependent on the state of the
catalogs, like pg_build_option('xml') returns boolean.
> I also have cases where clients don't want to give me superuser access,
> and sometimes not even shell access, and it could well be useful to me
> to be able to say to them "OK, you need to make sure that this file in
> this location has this entry".
Not sure what this has to do with this.
On 2015-08-20 09:59:25 -0400, Andrew Dunstan wrote:
> Is there any significant interest in either of these?
>
> Josh Berkus tells me that he would like pg_controldata information, and I
> was a bit interested in pg_config information, for this reason: I had a
> report of someone who had configured using --with-libxml but the xml tests
> actually returned the results that are expected without xml being
> configured. The regression tests thus passed, but should not have. It
> occurred to me that if we had a test like
>
> select pg_config('configure') ~ '--with-libxml' as has_xml;
>
> in the xml tests then this failure mode would be detected.
On my reading of the thread there seems to be a tenative agreement that
pg_controldata is useful and still controversy around pg_config. Can we
split committing this?
Greetings,
Andres Freund
> [2015082503-pgconfig_controldata.diff] I tried to build this, and the patch applies cleanly but then a ld error emerges: (The first four lines (about gram.y) are standard warnings; the error starts from the ld line) In file included from gram.y:14908:0: scan.c: In function ‘yy_try_NUL_trans’: scan.c:10307:23: warning: unused variable ‘yyg’ [-Wunused-variable] struct yyguts_t * yyg = (struct yyguts_t*)yyscanner;/* This var may be unused depending upon options. */ ^ /usr/bin/ld: Dwarf Error: found dwarf version '4', this reader only handles version 2 information. utils/fmgrtab.o:(.rodata+0x19f78): undefined reference to `_null_' utils/fmgrtab.o:(.rodata+0x1a078): undefined reference to `_null_' collect2: error: ld returned 1 exit status make[2]: *** [postgres] Error 1 make[1]: *** [all-backend-recurse] Error 2 make: *** [all-src-recurse] Error 2 The configure was: ./configure \ --prefix=/var/data1/pg_stuff/pg_installations/pgsql.controldata \ --with-pgport=6594 \ --bindir=/var/data1/pg_stuff/pg_installations/pgsql.controldata/bin.fast \ --libdir=/var/data1/pg_stuff/pg_installations/pgsql.controldata/lib.fast \ --sysconfdir=/var/data1/pg_stuff/pg_installations/pgsql.controldata/etc \ --quiet --enable-depend --with-perl --with-python --with-openssl --with-libxml \ --with-extra-version=_controldata_20151030_1432_c5057b2b3481 --enable-tap-tests Thanks, Erik Rijkers
On 10/30/2015 06:53 AM, Erik Rijkers wrote: >> [2015082503-pgconfig_controldata.diff] > > I tried to build this, and the patch applies cleanly but then a ld error > emerges: I'm sure the patch would need to be rebased, but the last thread died with significant complaints and questions about what was the correct approach. I therefore never even bothered submitting my latest patch version. I'll try to pick this back up in the next week and see if I can come up with something we can get a consensus on... Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Erik Rijkers wrote: > utils/fmgrtab.o:(.rodata+0x19f78): undefined reference to `_null_' > utils/fmgrtab.o:(.rodata+0x1a078): undefined reference to `_null_' The fix for this is to add a parallel-safe flag to new pg_proc.h lines. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Sep 17, 2015 at 7:12 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-08-20 09:59:25 -0400, Andrew Dunstan wrote:
>> Is there any significant interest in either of these?
>>
>> Josh Berkus tells me that he would like pg_controldata information, and I
>> was a bit interested in pg_config information, for this reason: I had a
>> report of someone who had configured using --with-libxml but the xml tests
>> actually returned the results that are expected without xml being
>> configured. The regression tests thus passed, but should not have. It
>> occurred to me that if we had a test like
>>
>> select pg_config('configure') ~ '--with-libxml' as has_xml;
>>
>> in the xml tests then this failure mode would be detected.
>
> On my reading of the thread there seems to be a tentative agreement that
> pg_controldata is useful and still controversy around pg_config. Can we
> split committing this?
Yeah, the last version of the patch dates of August, and there is
visibly agreement that the information of pg_controldata provided at
SQL level is useful while the data of pg_config is proving to be less
interesting for remote users. Could the patch be rebased and split as
suggested above?
--
Michael
On Wed, Dec 9, 2015 at 9:18 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Sep 17, 2015 at 7:12 AM, Andres Freund <andres@anarazel.de> wrote:
>> On 2015-08-20 09:59:25 -0400, Andrew Dunstan wrote:
>>> Is there any significant interest in either of these?
>>>
>>> Josh Berkus tells me that he would like pg_controldata information, and I
>>> was a bit interested in pg_config information, for this reason: I had a
>>> report of someone who had configured using --with-libxml but the xml tests
>>> actually returned the results that are expected without xml being
>>> configured. The regression tests thus passed, but should not have. It
>>> occurred to me that if we had a test like
>>>
>>> select pg_config('configure') ~ '--with-libxml' as has_xml;
>>>
>>> in the xml tests then this failure mode would be detected.
>>
>> On my reading of the thread there seems to be a tentative agreement that
>> pg_controldata is useful and still controversy around pg_config. Can we
>> split committing this?
>
> Yeah, the last version of the patch dates of August, and there is
> visibly agreement that the information of pg_controldata provided at
> SQL level is useful while the data of pg_config is proving to be less
> interesting for remote users. Could the patch be rebased and split as
> suggested above?
I am marking this patch as returned with feedback, there is not much activity...
--
Michael
On 12/23/2015 05:45 AM, Michael Paquier wrote: >> Yeah, the last version of the patch dates of August, and there is >> visibly agreement that the information of pg_controldata provided at >> SQL level is useful while the data of pg_config is proving to be less >> interesting for remote users. Could the patch be rebased and split as >> suggested above? > > I am marking this patch as returned with feedback, there is not much activity... I just dusted this off yesterday finally. Anyway, based on the discussions I plan to: 1) split it into two separate patches, one for pg_config and one for pg_controldata. 2) Change the pg_controldata to be a bunch of separate functions as suggested by Josh Berkus rather than one SRF. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Thu, Dec 24, 2015 at 2:08 AM, Joe Conway <mail@joeconway.com> wrote: > On 12/23/2015 05:45 AM, Michael Paquier wrote: >>> Yeah, the last version of the patch dates of August, and there is >>> visibly agreement that the information of pg_controldata provided at >>> SQL level is useful while the data of pg_config is proving to be less >>> interesting for remote users. Could the patch be rebased and split as >>> suggested above? >> >> I am marking this patch as returned with feedback, there is not much activity... > > I just dusted this off yesterday finally. Anyway, based on the > discussions I plan to: > > 1) split it into two separate patches, one for pg_config and one for > pg_controldata. > 2) Change the pg_controldata to be a bunch of separate functions as > suggested by Josh Berkus rather than one SRF. This looks like a plan, thanks! -- Michael
On 12/23/2015 04:37 PM, Michael Paquier wrote: > On Thu, Dec 24, 2015 at 2:08 AM, Joe Conway <mail@joeconway.com> wrote: >> On 12/23/2015 05:45 AM, Michael Paquier wrote: >>>> Yeah, the last version of the patch dates of August, and there is >>>> visibly agreement that the information of pg_controldata provided at >>>> SQL level is useful while the data of pg_config is proving to be less >>>> interesting for remote users. Could the patch be rebased and split as >>>> suggested above? >>> >>> I am marking this patch as returned with feedback, there is not much activity... >> >> I just dusted this off yesterday finally. Anyway, based on the >> discussions I plan to: >> >> 1) split it into two separate patches, one for pg_config and one for >> pg_controldata. >> 2) Change the pg_controldata to be a bunch of separate functions as >> suggested by Josh Berkus rather than one SRF. > > This looks like a plan, thanks! First installment -- pg_config function/view as a separate patch, rebased to current master. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
On 12/23/2015 04:37 PM, Michael Paquier wrote:
> On Thu, Dec 24, 2015 at 2:08 AM, Joe Conway <mail@joeconway.com> wrote:
>> 2) Change the pg_controldata to be a bunch of separate functions as
>> suggested by Josh Berkus rather than one SRF.
>
> This looks like a plan, thanks!
As discussed, a completely revamped and split off pg_controldata patch.
Below are the details for those interested.
Comments please.
Joe
===============
What this patch does:
---------------
1) Change NextXID output format from "%u/%u" to "%u:%u"
(see recent hackers thread)
2) Refactor bin/pg_controldata (there should be no visible change to
pg_controldata output)
3) Adds new functions, more or less in line with previous discussions:
* pg_checkpoint_state()
* pg_controldata_state()
* pg_recovery_state()
* pg_init_state()
===============
Missing (TODO once agreement on the above is reached):
---------------
a) documentation
b) catversion bump
c) regression tests
===============
New function detail and sample output:
---------------
postgres=# \x
Expanded display is on.
postgres=# \df pg_*_state
List of functions
-[ RECORD 1 ]-------+----------------------------------------------
Schema | pg_catalog
Name | pg_checkpoint_state
Result data type | record
Argument data types | OUT checkpoint_location pg_lsn,
| OUT prior_location pg_lsn,
| OUT redo_location pg_lsn,
| OUT redo_wal_file text,
| OUT timeline_id integer,
| OUT prev_timeline_id integer,
| OUT full_page_writes boolean,
| OUT next_xid text,
| OUT next_oid oid,
| OUT next_multixact_id xid,
| OUT next_multi_offset xid,
| OUT oldest_xid xid,
| OUT oldest_xid_dbid oid,
| OUT oldest_active_xid xid,
| OUT oldest_multi_xid xid,
| OUT oldest_multi_dbid oid,
| OUT oldest_commit_ts_xid xid,
| OUT newest_commit_ts_xid xid,
| OUT checkpoint_time timestamp with time zone
Type | normal
-[ RECORD 2 ]-------+----------------------------------------------
Schema | pg_catalog
Name | pg_controldata_state
Result data type | record
Argument data types | OUT pg_control_version integer,
| OUT catalog_version_no integer,
| OUT system_identifier bigint,
| OUT pg_control_last_modified
| timestamp with time zone
Type | normal
-[ RECORD 3 ]-------+----------------------------------------------
Schema | pg_catalog
Name | pg_init_state
Result data type | record
Argument data types | OUT max_data_alignment integer,
| OUT database_block_size integer,
| OUT blocks_per_segment integer,
| OUT wal_block_size integer,
| OUT bytes_per_wal_segment integer,
| OUT max_identifier_length integer,
| OUT max_index_columns integer,
| OUT max_toast_chunk_size integer,
| OUT large_object_chunk_size integer,
| OUT bigint_timestamps boolean,
| OUT float4_pass_by_value boolean,
| OUT float8_pass_by_value boolean,
| OUT data_page_checksum_version integer
Type | normal
-[ RECORD 4 ]-------+----------------------------------------------
Schema | pg_catalog
Name | pg_recovery_state
Result data type | record
Argument data types | OUT min_recovery_end_location pg_lsn,
| OUT min_recovery_end_timeline integer,
| OUT backup_start_location pg_lsn,
| OUT backup_end_location pg_lsn,
| OUT end_of_backup_record_required boolean
Type | normal
postgres=# select * from pg_controldata_state();
-[ RECORD 1 ]------------+-----------------------
pg_control_version | 942
catalog_version_no | 201511071
system_identifier | 6233852631805477166
pg_control_last_modified | 2015-12-29 15:32:09-08
postgres=# select * from pg_checkpoint_state();
-[ RECORD 1 ]--------+-------------------------
checkpoint_location | 0/14E8C38
prior_location | 0/14D6340
redo_location | 0/14E8C38
redo_wal_file | 000000010000000000000001
timeline_id | 1
prev_timeline_id | 1
full_page_writes | t
next_xid | 0:574
next_oid | 12407
next_multixact_id | 1
next_multi_offset | 0
oldest_xid | 565
oldest_xid_dbid | 1
oldest_active_xid | 0
oldest_multi_xid | 1
oldest_multi_dbid | 1
oldest_commit_ts_xid | 0
newest_commit_ts_xid | 0
checkpoint_time | 2015-12-29 15:32:02-08
postgres=# select * from pg_recovery_state();
-[ RECORD 1 ]-----------------+----
min_recovery_end_location | 0/0
min_recovery_end_timeline | 0
backup_start_location | 0/0
backup_end_location | 0/0
end_of_backup_record_required | f
postgres=# select * from pg_init_state();
-[ RECORD 1 ]--------------+---------
max_data_alignment | 8
database_block_size | 8192
blocks_per_segment | 131072
wal_block_size | 8192
bytes_per_wal_segment | 16777216
max_identifier_length | 64
max_index_columns | 32
max_toast_chunk_size | 1996
large_object_chunk_size | 2048
bigint_timestamps | t
float4_pass_by_value | t
float8_pass_by_value | t
data_page_checksum_version | 0
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Вложения
On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway <mail@joeconway.com> wrote:
> 1) Change NextXID output format from "%u/%u" to "%u:%u"
> (see recent hackers thread)
! printf(_("Latest checkpoint's NextXID: %u/%u\n"), ControlFile.checkPointCopy.nextXidEpoch,
ControlFile.checkPointCopy.nextXid); printf(_("Latest checkpoint's NextOID: %u\n"),
--- 646,652 ---- ControlFile.checkPointCopy.ThisTimeLineID); printf(_("Latest checkpoint's
full_page_writes:%s\n"), ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
! printf(_("Latest checkpoint's NextXID: %u:%u\n"),
This should be definitely a separate patch.
> 2) Refactor bin/pg_controldata (there should be no visible change to
> pg_controldata output)
> 3) Adds new functions, more or less in line with previous discussions:
> * pg_checkpoint_state()
> * pg_controldata_state()
> * pg_recovery_state()
> * pg_init_state()
Taking the opposite direction of Josh upthread, why is this split
actually necessary? Isn't the idea to provide a SQL interface of what
pg_controldata shows? If this split proves to be useful, shouldn't we
do it as well for pg_controldata?
> ===============
> Missing (TODO once agreement on the above is reached):
> ---------------
> a) documentation
This would be good to have.
> b) catversion bump
That's committer work.
> c) regression tests
Hm, what would be the value of those tests? I think we could live
without for simple functions like that honestly.
I think that those functions should be superuser-only. They provide
information about the system globally.
--
Michael
On Sun, Dec 27, 2015 at 5:39 AM, Joe Conway <mail@joeconway.com> wrote: > First installment -- pg_config function/view as a separate patch, > rebased to current master. Documentation would be good to have. ! # don't include subdirectory-path-dependent -I and -L switches ! STD_CPPFLAGS := $(filter-out -I$(top_srcdir)/src/include -I$(top_builddir)/src/include,$(CPPFLAGS)) ! STD_LDFLAGS := $(filter-out -L$(top_builddir)/src/port,$(LDFLAGS)) ! override CPPFLAGS += -DVAL_CONFIGURE="\"$(configure_args)\"" ! override CPPFLAGS += -DVAL_CC="\"$(CC)\"" ! override CPPFLAGS += -DVAL_CPPFLAGS="\"$(STD_CPPFLAGS)\"" ! override CPPFLAGS += -DVAL_CFLAGS="\"$(CFLAGS)\"" ! override CPPFLAGS += -DVAL_CFLAGS_SL="\"$(CFLAGS_SL)\"" ! override CPPFLAGS += -DVAL_LDFLAGS="\"$(STD_LDFLAGS)\"" ! override CPPFLAGS += -DVAL_LDFLAGS_EX="\"$(LDFLAGS_EX)\"" ! override CPPFLAGS += -DVAL_LDFLAGS_SL="\"$(LDFLAGS_SL)\"" ! override CPPFLAGS += -DVAL_LIBS="\"$(LIBS)\"" This duplication from src/bin/pg_config is a bad idea. Couldn't we do something in src/common instead that sets up values at compilation time in a routine (perhaps set of routines) available for both the frontend and backend? -- Michael
On Sat, Jan 16, 2016 at 11:07 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Sun, Dec 27, 2015 at 5:39 AM, Joe Conway <mail@joeconway.com> wrote: >> First installment -- pg_config function/view as a separate patch, >> rebased to current master. > > Documentation would be good to have. > > ! # don't include subdirectory-path-dependent -I and -L switches > ! STD_CPPFLAGS := $(filter-out -I$(top_srcdir)/src/include > -I$(top_builddir)/src/include,$(CPPFLAGS)) > ! STD_LDFLAGS := $(filter-out -L$(top_builddir)/src/port,$(LDFLAGS)) > ! override CPPFLAGS += -DVAL_CONFIGURE="\"$(configure_args)\"" > ! override CPPFLAGS += -DVAL_CC="\"$(CC)\"" > ! override CPPFLAGS += -DVAL_CPPFLAGS="\"$(STD_CPPFLAGS)\"" > ! override CPPFLAGS += -DVAL_CFLAGS="\"$(CFLAGS)\"" > ! override CPPFLAGS += -DVAL_CFLAGS_SL="\"$(CFLAGS_SL)\"" > ! override CPPFLAGS += -DVAL_LDFLAGS="\"$(STD_LDFLAGS)\"" > ! override CPPFLAGS += -DVAL_LDFLAGS_EX="\"$(LDFLAGS_EX)\"" > ! override CPPFLAGS += -DVAL_LDFLAGS_SL="\"$(LDFLAGS_SL)\"" > ! override CPPFLAGS += -DVAL_LIBS="\"$(LIBS)\"" > This duplication from src/bin/pg_config is a bad idea. Couldn't we do > something in src/common instead that sets up values at compilation > time in a routine (perhaps set of routines) available for both the > frontend and backend? Just forgot to mention that those new functions should be superuser-only. -- Michael
On Jan 16, 2016, at 9:08 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > Just forgot to mention that those new functions should be superuser-only. I think nobody should ever say this without explaining why. Superuser restrictions are necessary in some cases, but the fewerof them we have, the better. ...Robert
On Sun, Jan 17, 2016 at 12:27 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Jan 16, 2016, at 9:08 AM, Michael Paquier <michael.paquier@gmail.com> wrote: >> Just forgot to mention that those new functions should be superuser-only. > > I think nobody should ever say this without explaining why. Superuser restrictions are necessary in some cases, but thefewer of them we have, the better. The pg_config functions are giving away information about the system itself, isn't that potentially sensible? The pg_controdata ones show up information about checkpoint, recovery etc. There are a couple of fields that could be made completely visible, like the information defined when running initdb or how the code is compiled like block size (not the system ID), but we surely do not want to give away checkpoint and recovery information. -- Michael
On January 17, 2016 12:46:36 AM GMT+01:00, Michael Paquier <michael.paquier@gmail.com> wrote: , but we surely do not want to give away >checkpoint and recovery information. Why is that? A lot of that information is available anyway? --- Please excuse brevity and formatting - I am writing this on my mobile phone.
On 01/16/2016 06:02 AM, Michael Paquier wrote:
> On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway <mail@joeconway.com> wrote:
>> 1) Change NextXID output format from "%u/%u" to "%u:%u"
>> (see recent hackers thread)
>
> ! printf(_("Latest checkpoint's NextXID: %u/%u\n"),
> ControlFile.checkPointCopy.nextXidEpoch,
> ControlFile.checkPointCopy.nextXid);
> printf(_("Latest checkpoint's NextOID: %u\n"),
> --- 646,652 ----
> ControlFile.checkPointCopy.ThisTimeLineID);
> printf(_("Latest checkpoint's full_page_writes: %s\n"),
> ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
> ! printf(_("Latest checkpoint's NextXID: %u:%u\n"),
> This should be definitely a separate patch.
Ok. Notwithstanding Simon's reply, there seems to be consensus that this
is the way to go. Will commit it this way unless some additional
objections surface in the next day or so.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
On 01/16/2016 06:07 AM, Michael Paquier wrote: > On Sun, Dec 27, 2015 at 5:39 AM, Joe Conway <mail@joeconway.com> wrote: >> First installment -- pg_config function/view as a separate patch, >> rebased to current master. > > Documentation would be good to have. I'm definitely happy to write the docs, but earlier it was not clear that there was enough support for this patch at all, and I don't want to waste cycles writing docs for a feature that ultimately does not get committed. What's the current feel for whether this feature in general is a good idea or bad? > ! # don't include subdirectory-path-dependent -I and -L switches > ! STD_CPPFLAGS := $(filter-out -I$(top_srcdir)/src/include > -I$(top_builddir)/src/include,$(CPPFLAGS)) > ! STD_LDFLAGS := $(filter-out -L$(top_builddir)/src/port,$(LDFLAGS)) > ! override CPPFLAGS += -DVAL_CONFIGURE="\"$(configure_args)\"" > ! override CPPFLAGS += -DVAL_CC="\"$(CC)\"" > ! override CPPFLAGS += -DVAL_CPPFLAGS="\"$(STD_CPPFLAGS)\"" > ! override CPPFLAGS += -DVAL_CFLAGS="\"$(CFLAGS)\"" > ! override CPPFLAGS += -DVAL_CFLAGS_SL="\"$(CFLAGS_SL)\"" > ! override CPPFLAGS += -DVAL_LDFLAGS="\"$(STD_LDFLAGS)\"" > ! override CPPFLAGS += -DVAL_LDFLAGS_EX="\"$(LDFLAGS_EX)\"" > ! override CPPFLAGS += -DVAL_LDFLAGS_SL="\"$(LDFLAGS_SL)\"" > ! override CPPFLAGS += -DVAL_LIBS="\"$(LIBS)\"" > This duplication from src/bin/pg_config is a bad idea. Couldn't we do > something in src/common instead that sets up values at compilation > time in a routine (perhaps set of routines) available for both the > frontend and backend? Will take a look at it. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 01/16/2016 06:02 AM, Michael Paquier wrote: > On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway <mail@joeconway.com> wrote: >> 3) Adds new functions, more or less in line with previous discussions: >> * pg_checkpoint_state() >> * pg_controldata_state() >> * pg_recovery_state() >> * pg_init_state() > > Taking the opposite direction of Josh upthread, why is this split > actually necessary? Isn't the idea to provide a SQL interface of what > pg_controldata shows? If this split proves to be useful, shouldn't we > do it as well for pg_controldata? The last discussion moved strongly in the direction of separate functions, and that being different from pg_controldata was not a bad thing. That said, I'm still of the opinion that there are legitimate reasons to want the command line pg_controldata and the SQL functions to produce equivalent, if not identical, results. I just wish we could get a clear consensus one way or the other. >> =============== >> Missing (TODO once agreement on the above is reached): >> --------------- >> a) documentation > > This would be good to have. Sure, once we have settled on a direction. >> b) catversion bump > > That's committer work. I know, but since I will be the committer if this thing ever gets that far, I wanted to point out that I had not forgotten that little detail ;-) >> c) regression tests > > Hm, what would be the value of those tests? I think we could live > without for simple functions like that honestly. Works for me. > I think that those functions should be superuser-only. They provide > information about the system globally. The tail of this thread seems to be headed away from this direction. I'll take another look and propose something concrete. Thanks for the comments! Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On Sun, Jan 17, 2016 at 8:48 AM, Andres Freund <andres@anarazel.de> wrote: > On January 17, 2016 12:46:36 AM GMT+01:00, Michael Paquier <michael.paquier@gmail.com> wrote: > , but we surely do not want to give away >>checkpoint and recovery information. > > Why is that? A lot of that information is available anyway? We are trying to hide away from non-superusers WAL-related information in system views and system function, that's my point to do the same here. For the data of pg_control, it seems to me that this can give away to any authorized users hints regarding the way Postgres is built, perhaps letting people know for example which Linux distribution is used and which flavor of Postgres is used (we already give away some information with version() but that's different than the libraries this is linking to), so an attacker may be able to take advantage of that to do attacks on potentially outdated packages? And I would think that many users are actually going to revoke the access of those functions to public if we are going to make them world-visible. It is easier as well to restrict things first, and then relax if necessary, than the opposite as well. -- Michael
On 2016-01-18 10:18:34 +0900, Michael Paquier wrote: > We are trying to hide away from non-superusers WAL-related information > in system views and system function, that's my point to do the same > here. We are? pg_current_xlog_insert_location(), pg_current_xlog_location(), pg_is_xlog_replay_paused(), pg_stat_bgwriter ... are all non-superuser? > For the data of pg_control, it seems to me that this can give > away to any authorized users hints regarding the way Postgres is > built, perhaps letting people know for example which Linux > distribution is used and which flavor of Postgres is used (we already > give away some information with version() but that's different than > the libraries this is linking to), so an attacker may be able to take > advantage of that to do attacks on potentially outdated packages? And > I would think that many users are actually going to revoke the access > of those functions to public if we are going to make them > world-visible. It is easier as well to restrict things first, and then > relax if necessary, than the opposite as well. Meh, that seems pretty far into pseudo security arguments. Greetings, Andres Freund
On Sun, Jan 17, 2016 at 02:24:46PM -0800, Joe Conway wrote:
> On 01/16/2016 06:02 AM, Michael Paquier wrote:
> > On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway <mail@joeconway.com> wrote:
> >> 1) Change NextXID output format from "%u/%u" to "%u:%u"
> >> (see recent hackers thread)
> >
> > ! printf(_("Latest checkpoint's NextXID: %u/%u\n"),
> > ControlFile.checkPointCopy.nextXidEpoch,
> > ControlFile.checkPointCopy.nextXid);
> > printf(_("Latest checkpoint's NextOID: %u\n"),
> > --- 646,652 ----
> > ControlFile.checkPointCopy.ThisTimeLineID);
> > printf(_("Latest checkpoint's full_page_writes: %s\n"),
> > ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
> > ! printf(_("Latest checkpoint's NextXID: %u:%u\n"),
> > This should be definitely a separate patch.
>
> Ok. Notwithstanding Simon's reply, there seems to be consensus that this
> is the way to go. Will commit it this way unless some additional
> objections surface in the next day or so.
FYI, this slash-colon change will break pg_upgrade unless it is patched.
Dp you want a patch from me?
-- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB
http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +
On Mon, Jan 18, 2016 at 4:43 AM, Andres Freund <andres@anarazel.de> wrote: > On 2016-01-18 10:18:34 +0900, Michael Paquier wrote: >> We are trying to hide away from non-superusers WAL-related information >> in system views and system function, that's my point to do the same >> here. > > We are? pg_current_xlog_insert_location(), pg_current_xlog_location(), > pg_is_xlog_replay_paused(), pg_stat_bgwriter ... are all non-superuser? Yeah. There's certainly no need for the WAL positions reported by pg_controldata to be any more restricted than other functions that give away information about WAL position. We had some discussion about restricting WAL position information in general due to possible information leakage, and if we do that, then perhaps this should be similarly restricted. Presumably vulnerabilities here would be harder to exploit because the values change much less frequently, so if you are trying to learn something the rate at which you can glean information will be much lower. But maybe we should put the same restrictions on all of it. >> For the data of pg_control, it seems to me that this can give >> away to any authorized users hints regarding the way Postgres is >> built, perhaps letting people know for example which Linux >> distribution is used and which flavor of Postgres is used (we already >> give away some information with version() but that's different than >> the libraries this is linking to), so an attacker may be able to take >> advantage of that to do attacks on potentially outdated packages? And >> I would think that many users are actually going to revoke the access >> of those functions to public if we are going to make them >> world-visible. It is easier as well to restrict things first, and then >> relax if necessary, than the opposite as well. > > Meh, that seems pretty far into pseudo security arguments. Yeah, I really don't see anything in the pg_controldata output that looks sensitive. The WAL locations are the closest of anything, AFAICS. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 01/18/2016 01:47 PM, Bruce Momjian wrote:
> On Sun, Jan 17, 2016 at 02:24:46PM -0800, Joe Conway wrote:
>> On 01/16/2016 06:02 AM, Michael Paquier wrote:
>>> On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway <mail@joeconway.com> wrote:
>>>> 1) Change NextXID output format from "%u/%u" to "%u:%u"
>>>> (see recent hackers thread)
>>>
>>> ! printf(_("Latest checkpoint's NextXID: %u/%u\n"),
>>> ControlFile.checkPointCopy.nextXidEpoch,
>>> ControlFile.checkPointCopy.nextXid);
>>> printf(_("Latest checkpoint's NextOID: %u\n"),
>>> --- 646,652 ----
>>> ControlFile.checkPointCopy.ThisTimeLineID);
>>> printf(_("Latest checkpoint's full_page_writes: %s\n"),
>>> ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
>>> ! printf(_("Latest checkpoint's NextXID: %u:%u\n"),
>>> This should be definitely a separate patch.
>>
>> Ok. Notwithstanding Simon's reply, there seems to be consensus that this
>> is the way to go. Will commit it this way unless some additional
>> objections surface in the next day or so.
>
> FYI, this slash-colon change will break pg_upgrade unless it is patched.
> Dp you want a patch from me?
Didn't realize that -- yes please.
Thanks,
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
* Robert Haas (robertmhaas@gmail.com) wrote: > On Mon, Jan 18, 2016 at 4:43 AM, Andres Freund <andres@anarazel.de> wrote: > > Meh, that seems pretty far into pseudo security arguments. > > Yeah, I really don't see anything in the pg_controldata output that > looks sensitive. The WAL locations are the closest of anything, > AFAICS. Heikki already showed how the WAL location information could be exploited if compression is enabled. I believe that's something we should care about and fix in one way or another (my initial approach was using defualt roles, but using the ACL system and starting out w/ no rights granted to that function also works). Thanks! Stephen
On January 18, 2016 11:10:35 PM GMT+01:00, Stephen Frost <sfrost@snowman.net> wrote: >* Robert Haas (robertmhaas@gmail.com) wrote: >> On Mon, Jan 18, 2016 at 4:43 AM, Andres Freund <andres@anarazel.de> >wrote: >> > Meh, that seems pretty far into pseudo security arguments. >> >> Yeah, I really don't see anything in the pg_controldata output that >> looks sensitive. The WAL locations are the closest of anything, >> AFAICS. > >Heikki already showed how the WAL location information could be >exploited if compression is enabled. > >I believe that's something we should care about and fix in one way or >another (my initial approach was using defualt roles, but using the ACL >system and starting out w/ no rights granted to that function also >works). Sure. But it's pointless to make things more complicated when there's functions providing equivalent information already. --- Please excuse brevity and formatting - I am writing this on my mobile phone.
On 01/17/2016 02:29 PM, Joe Conway wrote: >> Documentation would be good to have. > > I'm definitely happy to write the docs, but earlier it was not clear > that there was enough support for this patch at all, and I don't want to > waste cycles writing docs for a feature that ultimately does not get > committed. What's the current feel for whether this feature in general > is a good idea or bad? Thoughts anyone? >> ! # don't include subdirectory-path-dependent -I and -L switches >> ! STD_CPPFLAGS := $(filter-out -I$(top_srcdir)/src/include >> -I$(top_builddir)/src/include,$(CPPFLAGS)) >> ! STD_LDFLAGS := $(filter-out -L$(top_builddir)/src/port,$(LDFLAGS)) >> ! override CPPFLAGS += -DVAL_CONFIGURE="\"$(configure_args)\"" >> ! override CPPFLAGS += -DVAL_CC="\"$(CC)\"" >> ! override CPPFLAGS += -DVAL_CPPFLAGS="\"$(STD_CPPFLAGS)\"" >> ! override CPPFLAGS += -DVAL_CFLAGS="\"$(CFLAGS)\"" >> ! override CPPFLAGS += -DVAL_CFLAGS_SL="\"$(CFLAGS_SL)\"" >> ! override CPPFLAGS += -DVAL_LDFLAGS="\"$(STD_LDFLAGS)\"" >> ! override CPPFLAGS += -DVAL_LDFLAGS_EX="\"$(LDFLAGS_EX)\"" >> ! override CPPFLAGS += -DVAL_LDFLAGS_SL="\"$(LDFLAGS_SL)\"" >> ! override CPPFLAGS += -DVAL_LIBS="\"$(LIBS)\"" >> This duplication from src/bin/pg_config is a bad idea. Couldn't we do >> something in src/common instead that sets up values at compilation >> time in a routine (perhaps set of routines) available for both the >> frontend and backend? > > Will take a look at it. Please see the attached. Duplication removed. Thanks, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
On Tue, Jan 19, 2016 at 6:55 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, Jan 18, 2016 at 4:43 AM, Andres Freund <andres@anarazel.de> wrote: >> On 2016-01-18 10:18:34 +0900, Michael Paquier wrote: >>> We are trying to hide away from non-superusers WAL-related information >>> in system views and system function, that's my point to do the same >>> here. >> >> We are? pg_current_xlog_insert_location(), pg_current_xlog_location(), >> pg_is_xlog_replay_paused(), pg_stat_bgwriter ... are all non-superuser? > > Yeah. There's certainly no need for the WAL positions reported by > pg_controldata to be any more restricted than other functions that > give away information about WAL position. We had some discussion > about restricting WAL position information in general due to possible > information leakage, and if we do that, then perhaps this should be > similarly restricted. Presumably vulnerabilities here would be harder > to exploit because the values change much less frequently, so if you > are trying to learn something the rate at which you can glean > information will be much lower. But maybe we should put the same > restrictions on all of it. Well, we can still use REVOKE on those functions, so it is not like a user cannot restrict the access to this information. The current situation makes it hard for both us and the user to figure out if an instance is considered as secure or not, so things are unbalanced. Perhaps the best answer is to add a documentation section to tell people how to harden their database after initdb'ing it, with different sections aimed at hardening different things, one being the WAL information, and mention as well in those docs which hardening action covers what. Stephen mentioned that some time ago, that would still be good. >>> For the data of pg_control, it seems to me that this can give >>> away to any authorized users hints regarding the way Postgres is >>> built, perhaps letting people know for example which Linux >>> distribution is used and which flavor of Postgres is used (we already >>> give away some information with version() but that's different than >>> the libraries this is linking to), so an attacker may be able to take >>> advantage of that to do attacks on potentially outdated packages? And >>> I would think that many users are actually going to revoke the access >>> of those functions to public if we are going to make them >>> world-visible. It is easier as well to restrict things first, and then >>> relax if necessary, than the opposite as well. >> >> Meh, that seems pretty far into pseudo security arguments. > > Yeah, I really don't see anything in the pg_controldata output that > looks sensitive. The WAL locations are the closest of anything, > AFAICS. The system identifier perhaps? I honestly don't have on top of my head a way to exploit this information but leaking that at SQL level seems sensible: that's a unique identifier of a Postgres instance used when setting up a cluster after all. -- Michael
fOn Mon, Jan 18, 2016 at 01:54:02PM -0800, Joe Conway wrote:
> On 01/18/2016 01:47 PM, Bruce Momjian wrote:
> > On Sun, Jan 17, 2016 at 02:24:46PM -0800, Joe Conway wrote:
> >> On 01/16/2016 06:02 AM, Michael Paquier wrote:
> >>> On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway <mail@joeconway.com> wrote:
> >>>> 1) Change NextXID output format from "%u/%u" to "%u:%u"
> >>>> (see recent hackers thread)
> >>>
> >>> ! printf(_("Latest checkpoint's NextXID: %u/%u\n"),
> >>> ControlFile.checkPointCopy.nextXidEpoch,
> >>> ControlFile.checkPointCopy.nextXid);
> >>> printf(_("Latest checkpoint's NextOID: %u\n"),
> >>> --- 646,652 ----
> >>> ControlFile.checkPointCopy.ThisTimeLineID);
> >>> printf(_("Latest checkpoint's full_page_writes: %s\n"),
> >>> ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
> >>> ! printf(_("Latest checkpoint's NextXID: %u:%u\n"),
> >>> This should be definitely a separate patch.
> >>
> >> Ok. Notwithstanding Simon's reply, there seems to be consensus that this
> >> is the way to go. Will commit it this way unless some additional
> >> objections surface in the next day or so.
> >
> > FYI, this slash-colon change will break pg_upgrade unless it is patched.
> > Dp you want a patch from me?
>
> Didn't realize that -- yes please.
Sure, attached, and it would be applied only to head, where you change
pg_controldata. pg_upgrade has to read the old and new cluster's
pg_controldata. We could get more sophisticated by checking the catalog
version number where the format was changed, but that doesn't seem worth
it, and is overly complex because we get the catalog version number from
pg_controldata, so you would be adding a dependency in ordering of the
pg_controldata entries.
I can test all suppored Postgres versions with pg_upgrade once you apply
the patch, but I think it will be fine.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +
Вложения
On 01/18/2016 04:16 PM, Joe Conway wrote: > Please see the attached. Duplication removed. Actually please see this version instead. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
On Tue, Jan 19, 2016 at 11:08 AM, Joe Conway <mail@joeconway.com> wrote:
> On 01/18/2016 04:16 PM, Joe Conway wrote:
>> Please see the attached. Duplication removed.
>
> Actually please see this version instead.
Thanks for the new patch.
+ tuplestore_puttuple(tupstore, tuple);
+ }
+
+ /*
+ * no longer need the tuple descriptor reference created by
The patch has some whitespaces.
+REVOKE ALL on pg_config FROM PUBLIC;
+REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC;
I guess that this portion is still under debate :)
make[1]: Nothing to be done for `all'.
make -C ../backend submake-errcodes
make[2]: *** No rule to make target `config_info.o', needed by
`libpgcommon.a'. Stop.
make[2]: *** Waiting for unfinished jobs....
The patch is visibly forgetting to include config_info.c, which should
be part of src/common.
/*
+ * This function cleans up the paths for use with either cmd.exe or Msys
+ * on Windows. We need them to use filenames without spaces, for which a
+ * short filename is the safest equivalent, eg:
+ * C:/Progra~1/
+ */
+void
+cleanup_path(char *path)
+{
Perhaps this refactoring would be useful as a separate patch?
--
Michael
On Tue, Jan 19, 2016 at 1:49 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Jan 19, 2016 at 11:08 AM, Joe Conway <mail@joeconway.com> wrote:
>> On 01/18/2016 04:16 PM, Joe Conway wrote:
>>> Please see the attached. Duplication removed.
>>
>> Actually please see this version instead.
>
> Thanks for the new patch.
>
> + tuplestore_puttuple(tupstore, tuple);
> + }
> +
> + /*
> + * no longer need the tuple descriptor reference created by
> The patch has some whitespaces.
>
> +REVOKE ALL on pg_config FROM PUBLIC;
> +REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC;
> I guess that this portion is still under debate :)
>
> make[1]: Nothing to be done for `all'.
> make -C ../backend submake-errcodes
> make[2]: *** No rule to make target `config_info.o', needed by
> `libpgcommon.a'. Stop.
> make[2]: *** Waiting for unfinished jobs....
> The patch is visibly forgetting to include config_info.c, which should
> be part of src/common.
>
> /*
> + * This function cleans up the paths for use with either cmd.exe or Msys
> + * on Windows. We need them to use filenames without spaces, for which a
> + * short filename is the safest equivalent, eg:
> + * C:/Progra~1/
> + */
> +void
> +cleanup_path(char *path)
> +{
> Perhaps this refactoring would be useful as a separate patch?
You need as well to update @pgcommonallfiles in Mkvcbuild.pm or the
compilation with MSVC is going to fail.
--
Michael
On Mon, Jan 18, 2016 at 07:50:12PM -0500, Bruce Momjian wrote:
> > >>>> 1) Change NextXID output format from "%u/%u" to "%u:%u"
> > >>>> (see recent hackers thread)
> > >>>
> > >>> ! printf(_("Latest checkpoint's NextXID: %u/%u\n"),
> > >>> ControlFile.checkPointCopy.nextXidEpoch,
> > >>> ControlFile.checkPointCopy.nextXid);
> > >>> printf(_("Latest checkpoint's NextOID: %u\n"),
> > >>> --- 646,652 ----
> > >>> ControlFile.checkPointCopy.ThisTimeLineID);
> > >>> printf(_("Latest checkpoint's full_page_writes: %s\n"),
> > >>> ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
> > >>> ! printf(_("Latest checkpoint's NextXID: %u:%u\n"),
> > >>> This should be definitely a separate patch.
> > >>
> > >> Ok. Notwithstanding Simon's reply, there seems to be consensus that this
> > >> is the way to go. Will commit it this way unless some additional
> > >> objections surface in the next day or so.
> > >
> > > FYI, this slash-colon change will break pg_upgrade unless it is patched.
> > > Dp you want a patch from me?
> >
> > Didn't realize that -- yes please.
>
> Sure, attached, and it would be applied only to head, where you change
> pg_controldata. pg_upgrade has to read the old and new cluster's
> pg_controldata. We could get more sophisticated by checking the catalog
> version number where the format was changed, but that doesn't seem worth
> it, and is overly complex because we get the catalog version number from
> pg_controldata, so you would be adding a dependency in ordering of the
> pg_controldata entries.
Sorry, please use the attached patch instead, now tested with your
changes.
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +
Вложения
On Mon, Jan 18, 2016 at 7:42 PM, Michael Paquier <michael.paquier@gmail.com> wrote: >> Yeah, I really don't see anything in the pg_controldata output that >> looks sensitive. The WAL locations are the closest of anything, >> AFAICS. > > The system identifier perhaps? I honestly don't have on top of my head > a way to exploit this information but leaking that at SQL level seems > sensible: that's a unique identifier of a Postgres instance used when > setting up a cluster after all. I think you are confusing useful information with security-sensitive information. The system identifier may be useful, but if you can't use it to compromise something, it's not security-sensitive. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 01/18/2016 08:51 PM, Michael Paquier wrote:
> On Tue, Jan 19, 2016 at 1:49 PM, Michael Paquier
>> + }
>> +
>> + /*
>> + * no longer need the tuple descriptor reference created by
>> The patch has some whitespaces.
I take it you mean a line with only whitespace? Fixed.
>> +REVOKE ALL on pg_config FROM PUBLIC;
>> +REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC;
>> I guess that this portion is still under debate :)
Left in for the moment.
>> make[1]: Nothing to be done for `all'.
>> make -C ../backend submake-errcodes
>> make[2]: *** No rule to make target `config_info.o', needed by
>> `libpgcommon.a'. Stop.
>> make[2]: *** Waiting for unfinished jobs....
>> The patch is visibly forgetting to include config_info.c, which should
>> be part of src/common.
Confounded git! ;-)
Fixed.
>> /*
>> + * This function cleans up the paths for use with either cmd.exe or Msys
>> + * on Windows. We need them to use filenames without spaces, for which a
>> + * short filename is the safest equivalent, eg:
>> + * C:/Progra~1/
>> + */
>> +void
>> +cleanup_path(char *path)
>> +{
>> Perhaps this refactoring would be useful as a separate patch?
It doesn't seem worth doing on its own, and it is required by the rest
of this patch. I'd say keep it here, but I'll break it out if you think
it important enough.
> You need as well to update @pgcommonallfiles in Mkvcbuild.pm or the
> compilation with MSVC is going to fail.
Good catch -- done.
The only things I know of still lacking is:
1) Documentation
2) Decision on REVOKE ... FROM PUBLIC
I'm assuming by the lack of complainants that there is enough support
for the feature (conceptually at least) that it is worthwhile for me to
write the docs. Will do that over the next couple of days or so.
Thanks for the code reviews!
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Вложения
On 01/19/2016 09:02 AM, Bruce Momjian wrote: >>>>> Ok. Notwithstanding Simon's reply, there seems to be consensus that this >>>>> is the way to go. Will commit it this way unless some additional >>>>> objections surface in the next day or so. >>>> >>>> FYI, this slash-colon change will break pg_upgrade unless it is patched. >>>> Dp you want a patch from me? >>> >>> Didn't realize that -- yes please. >> >> Sure, attached, and it would be applied only to head, where you change >> pg_controldata. pg_upgrade has to read the old and new cluster's >> pg_controldata. We could get more sophisticated by checking the catalog >> version number where the format was changed, but that doesn't seem worth >> it, and is overly complex because we get the catalog version number from >> pg_controldata, so you would be adding a dependency in ordering of the >> pg_controldata entries. > > Sorry, please use the attached patch instead, now tested with your > changes. The attached includes Bruce's change, plus I found two additional sites that appear to need the same change. The xlog.c change is just a DEBUG message, so not a big deal. I'm less certain if the xlogdesc.c change might create some fallout. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
Re: NextXID format change (was Re: exposing pg_controldata and pg_config as functions)
От
Alvaro Herrera
Дата:
Joe Conway wrote: > The attached includes Bruce's change, plus I found two additional sites > that appear to need the same change. The xlog.c change is just a DEBUG > message, so not a big deal. I'm less certain if the xlogdesc.c change > might create some fallout. Hm, pg_xlogdump links the rmgrdesc files, so perhaps you might need to adjust expected test output for it. Not really sure. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: NextXID format change (was Re: exposing pg_controldata and pg_config as functions)
От
Michael Paquier
Дата:
On Wed, Jan 20, 2016 at 11:41 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Joe Conway wrote: > >> The attached includes Bruce's change, plus I found two additional sites >> that appear to need the same change. The xlog.c change is just a DEBUG >> message, so not a big deal. I'm less certain if the xlogdesc.c change >> might create some fallout. > > Hm, pg_xlogdump links the rmgrdesc files, so perhaps you might need to > adjust expected test output for it. Not really sure. We don't depend on this output format in any tests AFAIK, at least check-world is not complaining here and pg_xlogdump has no dedicated tests. There may be some utility in the outside world doing some manipulation of the string generated for this record, but that's not worth worrying about anyway. Patch looks fine, I have not spotted any other places that need a refresh. -- Michael
On Wed, Jan 20, 2016 at 2:32 AM, Joe Conway <mail@joeconway.com> wrote:
> The only things I know of still lacking is:
> 1) Documentation
> 2) Decision on REVOKE ... FROM PUBLIC
Yep, regarding 2) I am the only one actually making noise to protect
this information by default, against at least 2 committers :)
> I'm assuming by the lack of complainants that there is enough support
> for the feature (conceptually at least) that it is worthwhile for me to
> write the docs. Will do that over the next couple of days or so.
I would think so as well.
+typedef struct configdata
+{
+ char *name;
+ char *setting;
+} configdata;
For a better analogy to ControlFileData, this could be renamed ConfigData?
-OBJS_COMMON = exec.o pg_lzcompress.o pgfnames.o psprintf.o relpath.o \
- rmtree.o string.o username.o wait_error.o
+# don't include subdirectory-path-dependent -I and -L switches
+STD_CPPFLAGS := $(filter-out -I$(top_srcdir)/src/include
-I$(top_builddir)/src/include,$(CPPFLAGS))
+STD_LDFLAGS := $(filter-out -L$(top_builddir)/src/port,$(LDFLAGS))
The point of the move to src/common is to remove the duplication in
src/bin/pg_config/Makefile.
--- /dev/null
+++ b/src/common/config_info.c
[...]
+#include "postgres.h"
+
+#include "miscadmin.h"
+#include "common/config_info.h"
All the files in src/common should begin their include declarations with that:
#ifndef FRONTEND
#include "postgres.h"
#else
#include "postgres_fe.h"
#endif
+configdata *
+get_configdata(char *my_exec_path, size_t *configdata_len)
+{
It may be good to mention that the result is palloc'd and that caller
may need to free it if necessary. It does not matter in the two code
paths of this patch, but it may matter for other users calling that.
MSVC compilation is working correctly here.
--
Michael
On Thu, Jan 21, 2016 at 1:08 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Jan 20, 2016 at 2:32 AM, Joe Conway <mail@joeconway.com> wrote:
>> The only things I know of still lacking is:
>> 1) Documentation
>> 2) Decision on REVOKE ... FROM PUBLIC
>
> Yep, regarding 2) I am the only one actually making noise to protect
> this information by default, against at least 2 committers :)
>
>> I'm assuming by the lack of complainants that there is enough support
>> for the feature (conceptually at least) that it is worthwhile for me to
>> write the docs. Will do that over the next couple of days or so.
>
> I would think so as well.
>
> +typedef struct configdata
> +{
> + char *name;
> + char *setting;
> +} configdata;
> For a better analogy to ControlFileData, this could be renamed ConfigData?
>
> -OBJS_COMMON = exec.o pg_lzcompress.o pgfnames.o psprintf.o relpath.o \
> - rmtree.o string.o username.o wait_error.o
> +# don't include subdirectory-path-dependent -I and -L switches
> +STD_CPPFLAGS := $(filter-out -I$(top_srcdir)/src/include
> -I$(top_builddir)/src/include,$(CPPFLAGS))
> +STD_LDFLAGS := $(filter-out -L$(top_builddir)/src/port,$(LDFLAGS))
> The point of the move to src/common is to remove the duplication in
> src/bin/pg_config/Makefile.
>
> --- /dev/null
> +++ b/src/common/config_info.c
> [...]
> +#include "postgres.h"
> +
> +#include "miscadmin.h"
> +#include "common/config_info.h"
> All the files in src/common should begin their include declarations with that:
> #ifndef FRONTEND
> #include "postgres.h"
> #else
> #include "postgres_fe.h"
> #endif
>
> +configdata *
> +get_configdata(char *my_exec_path, size_t *configdata_len)
> +{
> It may be good to mention that the result is palloc'd and that caller
> may need to free it if necessary. It does not matter in the two code
> paths of this patch, but it may matter for other users calling that.
>
> MSVC compilation is working correctly here.
I am marking this patch as returned with feedback for now, not all the
issues have been fixed yet, and there are still no docs (the
conclusion being that people would like to have this stuff, right?).
Feel free to move it to the next CF should a new version be written.
--
Michael
On 01/19/2016 07:04 PM, Michael Paquier wrote: > On Wed, Jan 20, 2016 at 11:41 AM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: >> Joe Conway wrote: >> >>> The attached includes Bruce's change, plus I found two additional sites >>> that appear to need the same change. The xlog.c change is just a DEBUG >>> message, so not a big deal. I'm less certain if the xlogdesc.c change >>> might create some fallout. >> >> Hm, pg_xlogdump links the rmgrdesc files, so perhaps you might need to >> adjust expected test output for it. Not really sure. > > We don't depend on this output format in any tests AFAIK, at least > check-world is not complaining here and pg_xlogdump has no dedicated > tests. There may be some utility in the outside world doing some > manipulation of the string generated for this record, but that's not > worth worrying about anyway. > > Patch looks fine, I have not spotted any other places that need a refresh. I'll commit the attached tomorrow if there are no other concerns voiced. In the spirit of the dev meeting discussion, I am trying to use the commit message template discussed. Something like: -- email subject limit ----------------------------------------- Change delimiter used for display of NextXID NextXID has been rendered in the form of a pg_lsn even though it really is not. This can cause confusion, so change the format from %u/%u to %u:%u, per discussion on hackers. Complaint by me, patch by me and Bruce, reviewed by Michael Paquier and Alvaro. Applied to HEAD only. Reported-by: Joe Conway Author: Joe Conway, Bruce Momjian Reviewed-by: Michael Paquier, Alvaro Herrera Tested-by: Michael Paquier Backpatch-through: master -- email subject limit ----------------------------------------- That does look pretty redundant though. Thoughts? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
Re: NextXID format change (was Re: exposing pg_controldata and pg_config as functions)
От
Michael Paquier
Дата:
On Wed, Feb 10, 2016 at 9:57 AM, Joe Conway <mail@joeconway.com> wrote: > I'll commit the attached tomorrow if there are no other concerns voiced. Just a nitpick regarding this block: + if (strchr(p, '/') != NULL) + p = strchr(p, '/'); + /* delimiter changed from '/' to ':' in 9.6 */ + else if (GET_MAJOR_VERSION(cluster->major_version) >= 906) + p = strchr(p, ':'); + else + p = NULL; Changing it as follows would save some instructions because there is no need to call strchr an extra time: if (GET_MAJOR_VERSION(cluster->major_version) >= 906) p = strchr(p, ':'); else p = strchr(p, '/'); > In the spirit of the dev meeting discussion, I am trying to use the > commit message template discussed. Something like: > > -- email subject limit ----------------------------------------- > Change delimiter used for display of NextXID > > NextXID has been rendered in the form of a pg_lsn even though it > really is not. This can cause confusion, so change the format from > %u/%u to %u:%u, per discussion on hackers. > > Complaint by me, patch by me and Bruce, reviewed by Michael Paquier > and Alvaro. Applied to HEAD only. > > Reported-by: Joe Conway > Author: Joe Conway, Bruce Momjian > Reviewed-by: Michael Paquier, Alvaro Herrera > Tested-by: Michael Paquier > Backpatch-through: master > That does look pretty redundant though. Thoughts? Removing Tested-by would be fine here I guess, testing and review are overlapping concepts for this patch. -- Michael
Re: NextXID format change (was Re: exposing pg_controldata and pg_config as functions)
От
Bruce Momjian
Дата:
On Wed, Feb 10, 2016 at 10:23:41AM +0900, Michael Paquier wrote: > On Wed, Feb 10, 2016 at 9:57 AM, Joe Conway <mail@joeconway.com> wrote: > > I'll commit the attached tomorrow if there are no other concerns voiced. > > Just a nitpick regarding this block: > + if (strchr(p, '/') != NULL) > + p = strchr(p, '/'); > + /* delimiter changed from '/' to ':' in 9.6 */ > + else if (GET_MAJOR_VERSION(cluster->major_version) >= 906) > + p = strchr(p, ':'); > + else > + p = NULL; > Changing it as follows would save some instructions because there is > no need to call strchr an extra time: > if (GET_MAJOR_VERSION(cluster->major_version) >= 906) > p = strchr(p, ':'); > else > p = strchr(p, '/'); No, that is not an improvement --- see my previous comment: > We could get more sophisticated by checking the catalog version number > where the format was changed, but that doesn't seem worth it, and is > overly complex because we get the catalog version number from > pg_controldata, so you would be adding a dependency in ordering of the > pg_controldata entries. By testing for '906', you prevent users from using pg_upgrade to go from one catalog version of 9.6 to a later one. Few people may want to do it, but it should work. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription +
Re: NextXID format change (was Re: exposing pg_controldata and pg_config as functions)
От
Michael Paquier
Дата:
On Fri, Feb 12, 2016 at 9:18 AM, Bruce Momjian <bruce@momjian.us> wrote: > On Wed, Feb 10, 2016 at 10:23:41AM +0900, Michael Paquier wrote: >> On Wed, Feb 10, 2016 at 9:57 AM, Joe Conway <mail@joeconway.com> wrote: >> > I'll commit the attached tomorrow if there are no other concerns voiced. >> >> Just a nitpick regarding this block: >> + if (strchr(p, '/') != NULL) >> + p = strchr(p, '/'); >> + /* delimiter changed from '/' to ':' in 9.6 */ >> + else if (GET_MAJOR_VERSION(cluster->major_version) >= 906) >> + p = strchr(p, ':'); >> + else >> + p = NULL; >> Changing it as follows would save some instructions because there is >> no need to call strchr an extra time: >> if (GET_MAJOR_VERSION(cluster->major_version) >= 906) >> p = strchr(p, ':'); >> else >> p = strchr(p, '/'); > > No, that is not an improvement --- see my previous comment: > >> We could get more sophisticated by checking the catalog version number >> where the format was changed, but that doesn't seem worth it, and is >> overly complex because we get the catalog version number from >> pg_controldata, so you would be adding a dependency in ordering of the >> pg_controldata entries. > > By testing for '906', you prevent users from using pg_upgrade to go from > one catalog version of 9.6 to a later one. Few people may want to do > it, but it should work. OK, I see now. I did not consider the case where people would like to get upgrade from a dev version of 9.6 to the latest 9.6 version, just the upgrade from a previous major version <= 9.5. Thanks for reminding that pg_upgrade needs to support that. -- Michael
On 02/11/2016 04:59 PM, Michael Paquier wrote: > On Fri, Feb 12, 2016 at 9:18 AM, Bruce Momjian <bruce@momjian.us> wrote: >> No, that is not an improvement --- see my previous comment: >> >>> We could get more sophisticated by checking the catalog version number >>> where the format was changed, but that doesn't seem worth it, and is >>> overly complex because we get the catalog version number from >>> pg_controldata, so you would be adding a dependency in ordering of the >>> pg_controldata entries. >> >> By testing for '906', you prevent users from using pg_upgrade to go from >> one catalog version of 9.6 to a later one. Few people may want to do >> it, but it should work. > > OK, I see now. I did not consider the case where people would like to > get upgrade from a dev version of 9.6 to the latest 9.6 version, just > the upgrade from a previous major version <= 9.5. Thanks for reminding > that pg_upgrade needs to support that. Pushed with Bruce's original patch included. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: NextXID format change (was Re: exposing pg_controldata and pg_config as functions)
От
Bruce Momjian
Дата:
On Thu, Feb 11, 2016 at 07:18:46PM -0500, Bruce Momjian wrote: > No, that is not an improvement --- see my previous comment: > > > We could get more sophisticated by checking the catalog version number > > where the format was changed, but that doesn't seem worth it, and is > > overly complex because we get the catalog version number from > > pg_controldata, so you would be adding a dependency in ordering of the > > pg_controldata entries. > > By testing for '906', you prevent users from using pg_upgrade to go from > one catalog version of 9.6 to a later one. Few people may want to do > it, but it should work. C comment added explaining why we do this. -- Bruce Momjian <bruce@momjian.us> http://momjian.us EnterpriseDB http://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Roman grave inscription +
Re: NextXID format change (was Re: exposing pg_controldata and pg_config as functions)
От
Michael Paquier
Дата:
On Sat, Feb 13, 2016 at 7:54 AM, Bruce Momjian <bruce@momjian.us> wrote: > On Thu, Feb 11, 2016 at 07:18:46PM -0500, Bruce Momjian wrote: >> No, that is not an improvement --- see my previous comment: >> >> > We could get more sophisticated by checking the catalog version number >> > where the format was changed, but that doesn't seem worth it, and is >> > overly complex because we get the catalog version number from >> > pg_controldata, so you would be adding a dependency in ordering of the >> > pg_controldata entries. >> >> By testing for '906', you prevent users from using pg_upgrade to go from >> one catalog version of 9.6 to a later one. Few people may want to do >> it, but it should work. > > C comment added explaining why we do this. Thanks for the addition. -- Michael
On 01/20/2016 08:08 PM, Michael Paquier wrote:
> On Wed, Jan 20, 2016 at 2:32 AM, Joe Conway <mail@joeconway.com> wrote:
>> The only things I know of still lacking is:
>> 1) Documentation
Done and included in the attached.
>> 2) Decision on REVOKE ... FROM PUBLIC
>
> Yep, regarding 2) I am the only one actually making noise to protect
> this information by default, against at least 2 committers :)
I plan to commit this way -- if the decision is made to remove the two
REVOKEs it can always be done later, but I see no problem with it.
> +typedef struct configdata
> +{
> + char *name;
> + char *setting;
> +} configdata;
> For a better analogy to ControlFileData, this could be renamed ConfigData?
Well I was already using ConfigData as the variable name, but after some
review it seems better your way, so I made the struct ConfigData and the
variable configdata.
> The point of the move to src/common is to remove the duplication in
> src/bin/pg_config/Makefile.
check
> All the files in src/common should begin their include declarations with that:
> #ifndef FRONTEND
> #include "postgres.h"
> #else
> #include "postgres_fe.h"
> #endif
check
> +configdata *
> +get_configdata(char *my_exec_path, size_t *configdata_len)
> +{
> It may be good to mention that the result is palloc'd and that caller
> may need to free it if necessary. It does not matter in the two code
> paths of this patch, but it may matter for other users calling that.
check
I believe this takes care of all open issues with this, so I plan to
commit it as attached in a day or two. Thanks for your reviews and comments!
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Вложения
On Tue, Feb 16, 2016 at 5:29 AM, Joe Conway <mail@joeconway.com> wrote: > I believe this takes care of all open issues with this, so I plan to > commit it as attached in a day or two. Thanks for your reviews and comments! Here are just a couple of cosmetic comments. + The view <structname>pg_config</structname> describes the + compile-time configuration parameters of the currently installed + version of PostgreSQL. It is intended, for example, to be used by + software packages that want to interface to PostgreSQL to facilitate + finding the required header files and libraries. It provides the same + basic information as the <xref linkend="app-pgconfig"> PostgreSQL Client + Application. There is a System Information Function Missing markup <productname></> around PostgreSQL. + Application. There is a System Information Function Why is using upper-case characters necessary here? This could just say system function. The paragraph in func.sgml is a copy-paste of the one in catalogs.sgml. We may want to remove the duplication. + /* let the caller know we're sending back a tuplestore */ + rsinfo->returnMode = SFRM_Materialize; I guess one can recognize your style here for SRF functions :) @@ -61,7 +74,7 @@ libpgcommon_srv.a: $(OBJS_SRV)# a hack that might fail someday if there is a *_srv.o without a# corresponding*.o, but it works for now.%_srv.o: %.c %.o - $(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@ + $(CC) $(CFLAGS) $(subst -DFRONTEND ,, $(CPPFLAGS)) -c $< -o $@ Diff noise? --- /dev/null +++ b/src/common/config_info.c [...] + * IDENTIFICATION + * src/common/controldata_utils.c This is incorrect. + * IDENTIFICATION + * src/backend/utils/misc/pg_config.c + * + */ I am nitpicking here but this header block should have a long "----------------" at its bottom. -- Michael
On 02/15/2016 11:20 PM, Michael Paquier wrote: > Here are just a couple of cosmetic comments. > Missing markup <productname></> around PostgreSQL. Added, but I'll note that there are tons of locations in the docs where we do not do that. Maybe they should all be made consistent. > + Application. There is a System Information Function > Why is using upper-case characters necessary here? This could just say > system function. It is capitalized because it refers to a section in the docs. I followed it with <xref linkend="functions-info"> as well, so it ends up reading: "There is a System Information Function (Section 9.25) called pg_config which underlies this view." I guess I could be convinced to lower case it, but I thought this looked better. > The paragraph in func.sgml is a copy-paste of the one in > catalogs.sgml. We may want to remove the duplication. The paragraphs are mostly copy-paste, but slightly different (toward the end). There is both a function and a system view -- why would we not want a description in both places? > + /* let the caller know we're sending back a tuplestore */ > + rsinfo->returnMode = SFRM_Materialize; > I guess one can recognize your style here for SRF functions :) Old habits die hard ;-) > @@ -61,7 +74,7 @@ libpgcommon_srv.a: $(OBJS_SRV) > # a hack that might fail someday if there is a *_srv.o without a > # corresponding *.o, but it works for now. > %_srv.o: %.c %.o > - $(CC) $(CFLAGS) $(subst -DFRONTEND,, $(CPPFLAGS)) -c $< -o $@ > + $(CC) $(CFLAGS) $(subst -DFRONTEND ,, $(CPPFLAGS)) -c $< -o $@ > Diff noise? No, intentional. The original version leaves a white space at the beginning of CPPFLAGS after removing -DFRONTEND. > --- /dev/null > +++ b/src/common/config_info.c > [...] > + * IDENTIFICATION > + * src/common/controldata_utils.c > This is incorrect. Right -- found and fixed several of these with Alvaro's help after posting. Also fixed Copyright dates (s/2015/2016/) on the new files. > + * IDENTIFICATION > + * src/backend/utils/misc/pg_config.c > + * > + */ > I am nitpicking here but this header block should have a long > "----------------" at its bottom. Fair enough -- fixed. Thanks! Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
On Tue, Feb 16, 2016 at 11:36 PM, Joe Conway <mail@joeconway.com> wrote: > Thanks! OK. I think I'm good now. Thanks for the quick update. -- Michael
On Wed, Feb 17, 2016 at 10:13 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Feb 16, 2016 at 11:36 PM, Joe Conway <mail@joeconway.com> wrote: >> Thanks! > > OK. I think I'm good now. Thanks for the quick update. Actually, having second-thoughts on the matter, why is is that necessary to document the function pg_config()? The functions wrapping a system view just have the view documented, take for example pg_show_all_file_settings, pg_show_all_settings, pg_stat_get_wal_receiver, etc. -- Michael
On 02/17/2016 02:32 AM, Michael Paquier wrote: > Actually, having second-thoughts on the matter, why is is that > necessary to document the function pg_config()? The functions wrapping > a system view just have the view documented, take for example > pg_show_all_file_settings, pg_show_all_settings, > pg_stat_get_wal_receiver, etc. Ok, removed the documentation on the function pg_config() and pushed. Included bumped catversion. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 1/31/16 7:34 AM, Michael Paquier wrote: > I am marking this patch as returned with feedback for now, not all the > issues have been fixed yet, and there are still no docs (the > conclusion being that people would like to have this stuff, right?). > Feel free to move it to the next CF should a new version be written. I think we still don't have a real use case for this feature, and a couple of points against it.
On 2/17/16 12:15 PM, Joe Conway wrote: > On 02/17/2016 02:32 AM, Michael Paquier wrote: >> Actually, having second-thoughts on the matter, why is is that >> necessary to document the function pg_config()? The functions wrapping >> a system view just have the view documented, take for example >> pg_show_all_file_settings, pg_show_all_settings, >> pg_stat_get_wal_receiver, etc. > > Ok, removed the documentation on the function pg_config() and pushed. I still have my serious doubts about this, especially not even requiring superuser access for this information. Could someone explain why we need this?
Peter Eisentraut <peter_e@gmx.net> writes:
> On 2/17/16 12:15 PM, Joe Conway wrote:
>> Ok, removed the documentation on the function pg_config() and pushed.
> I still have my serious doubts about this, especially not even requiring
> superuser access for this information. Could someone explain why we
> need this?
I thought we'd agreed on requiring superuser access for this function.
I concur that letting just anyone see the config data is inappropriate.
regards, tom lane
On 02/17/2016 02:14 PM, Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.net> writes: >> On 2/17/16 12:15 PM, Joe Conway wrote: >>> Ok, removed the documentation on the function pg_config() and pushed. > >> I still have my serious doubts about this, especially not even requiring >> superuser access for this information. Could someone explain why we >> need this? > > I thought we'd agreed on requiring superuser access for this function. > I concur that letting just anyone see the config data is inappropriate. It does not let anyone see config data out of the box: + CREATE VIEW pg_config AS + SELECT * FROM pg_config(); + + REVOKE ALL on pg_config FROM PUBLIC; + REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC; + But it does not have an explicit superuser check. I can add that if that's the consensus. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 02/17/2016 01:31 PM, Peter Eisentraut wrote: > On 1/31/16 7:34 AM, Michael Paquier wrote: >> I am marking this patch as returned with feedback for now, not all the >> issues have been fixed yet, and there are still no docs (the >> conclusion being that people would like to have this stuff, right?). >> Feel free to move it to the next CF should a new version be written. > > I think we still don't have a real use case for this feature, and a > couple of points against it. I have a use-case for this feature, at part of it containerized PostgreSQL. Right now, there is certain diagnostic information (like timeline) which is exposed ONLY in pg_controldata. That leaves no reasonable way to expose this information in an API. (and yes, we have a bigger issue with stuff which is only in pg_log, but one thing at a time) -- -- Josh Berkus Red Hat OSAS (any opinions are my own)
On 02/17/2016 05:14 PM, Tom Lane wrote: > Peter Eisentraut <peter_e@gmx.net> writes: >> On 2/17/16 12:15 PM, Joe Conway wrote: >>> Ok, removed the documentation on the function pg_config() and pushed. >> I still have my serious doubts about this, especially not even requiring >> superuser access for this information. Could someone explain why we >> need this? > I thought we'd agreed on requiring superuser access for this function. > I concur that letting just anyone see the config data is inappropriate. > > I'm in favor, and don't really want to rehearse the argument. But I think I can live quite happily with it being superuser only. It's pretty hard to argue that exposing it to a superuser creates much risk, ISTM. cheers andrew
Joe Conway <mail@joeconway.com> writes:
> On 02/17/2016 02:14 PM, Tom Lane wrote:
>> I thought we'd agreed on requiring superuser access for this function.
>> I concur that letting just anyone see the config data is inappropriate.
> It does not let anyone see config data out of the box:
> + CREATE VIEW pg_config AS
> + SELECT * FROM pg_config();
> +
> + REVOKE ALL on pg_config FROM PUBLIC;
> + REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC;
Ah, that's fine. I'd looked for a superuser() check and not seen one,
but letting the SQL permissions system handle it seems good enough.
regards, tom lane
On 02/17/2016 03:02 PM, Tom Lane wrote: > Joe Conway <mail@joeconway.com> writes: >> On 02/17/2016 02:14 PM, Tom Lane wrote: >>> I thought we'd agreed on requiring superuser access for this function. >>> I concur that letting just anyone see the config data is inappropriate. > >> It does not let anyone see config data out of the box: > >> + CREATE VIEW pg_config AS >> + SELECT * FROM pg_config(); >> + >> + REVOKE ALL on pg_config FROM PUBLIC; >> + REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC; > > Ah, that's fine. I'd looked for a superuser() check and not seen one, > but letting the SQL permissions system handle it seems good enough. What I like about this is that if I want to expose it to a non-superuser, I can just do a GRANT instead of needing to write a security definer view. -- -- Josh Berkus Red Hat OSAS (any opinions are my own)
On 02/17/2016 03:34 PM, Josh berkus wrote: > On 02/17/2016 03:02 PM, Tom Lane wrote: >> Joe Conway <mail@joeconway.com> writes: >>> On 02/17/2016 02:14 PM, Tom Lane wrote: >>>> I thought we'd agreed on requiring superuser access for this function. >>>> I concur that letting just anyone see the config data is inappropriate. >> >>> It does not let anyone see config data out of the box: >> >>> + CREATE VIEW pg_config AS >>> + SELECT * FROM pg_config(); >>> + >>> + REVOKE ALL on pg_config FROM PUBLIC; >>> + REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC; >> >> Ah, that's fine. I'd looked for a superuser() check and not seen one, >> but letting the SQL permissions system handle it seems good enough. > > What I like about this is that if I want to expose it to a > non-superuser, I can just do a GRANT instead of needing to write a > security definer view. Which was my reason for doing it this way, although that GRANT will not get preserved by pg_dump currently. Stephen Frost is working on a patch to change/fix that though (see the "Additional role attributes && superuser review" thread), which I believe he intends to get done RSN and into 9.6. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 2/17/16 5:20 PM, Josh berkus wrote: > I have a use-case for this feature, at part of it containerized > PostgreSQL. Right now, there is certain diagnostic information (like > timeline) which is exposed ONLY in pg_controldata. I'm talking about the pg_config() function, not pg_controldata.
On Thu, Feb 18, 2016 at 11:02 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 2/17/16 5:20 PM, Josh berkus wrote: >> I have a use-case for this feature, at part of it containerized >> PostgreSQL. Right now, there is certain diagnostic information (like >> timeline) which is exposed ONLY in pg_controldata. > > I'm talking about the pg_config() function, not pg_controldata. Andrew has mentioned a use case he had at the beginning of this thread to enhance a bit the regression tests related to libxml. -- Michael
On 2/17/16 9:08 PM, Michael Paquier wrote: > On Thu, Feb 18, 2016 at 11:02 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >> On 2/17/16 5:20 PM, Josh berkus wrote: >>> I have a use-case for this feature, at part of it containerized >>> PostgreSQL. Right now, there is certain diagnostic information (like >>> timeline) which is exposed ONLY in pg_controldata. >> >> I'm talking about the pg_config() function, not pg_controldata. > > Andrew has mentioned a use case he had at the beginning of this thread > to enhance a bit the regression tests related to libxml. While that idea was useful, I think we had concluded that there are better ways to do this and that this way probably wouldn't even work (Windows?).
On 2016-02-17 21:19:08 -0500, Peter Eisentraut wrote: > On 2/17/16 9:08 PM, Michael Paquier wrote: > > On Thu, Feb 18, 2016 at 11:02 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > >> On 2/17/16 5:20 PM, Josh berkus wrote: > >>> I have a use-case for this feature, at part of it containerized > >>> PostgreSQL. Right now, there is certain diagnostic information (like > >>> timeline) which is exposed ONLY in pg_controldata. > >> > >> I'm talking about the pg_config() function, not pg_controldata. > > > > Andrew has mentioned a use case he had at the beginning of this thread > > to enhance a bit the regression tests related to libxml. > > While that idea was useful, I think we had concluded that there are > better ways to do this and that this way probably wouldn't even work > (Windows?). I don't understand why you're so opposed to this. Several people said that they're interested in this information in the current discussion and it has been requested repeatedly over the years. For superusers you can already hack access, but it's darn ugly. Greetings, Andres Freund
On 02/18/2016 12:30 AM, Andres Freund wrote: > On 2016-02-17 21:19:08 -0500, Peter Eisentraut wrote: >> On 2/17/16 9:08 PM, Michael Paquier wrote: >>> On Thu, Feb 18, 2016 at 11:02 AM, Peter Eisentraut <peter_e@gmx.net> wrote: >>>> On 2/17/16 5:20 PM, Josh berkus wrote: >>>>> I have a use-case for this feature, at part of it containerized >>>>> PostgreSQL. Right now, there is certain diagnostic information (like >>>>> timeline) which is exposed ONLY in pg_controldata. >>>> >>>> I'm talking about the pg_config() function, not pg_controldata. >>> >>> Andrew has mentioned a use case he had at the beginning of this thread >>> to enhance a bit the regression tests related to libxml. >> >> While that idea was useful, I think we had concluded that there are >> better ways to do this and that this way probably wouldn't even work >> (Windows?). Just to be clear, AFAIK there is no issue with the committed changes on Windows. If there is please provide a concrete example that we can discuss. > I don't understand why you're so opposed to this. Several people said > that they're interested in this information in the current discussion > and it has been requested repeatedly over the years. For superusers you > can already hack access, but it's darn ugly. Exactly. -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
On 2/18/16 3:30 AM, Andres Freund wrote: > I don't understand why you're so opposed to this. Several people said > that they're interested in this information in the current discussion > and it has been requested repeatedly over the years. I think no one except Andrew Dunstan has requested this, and his use case is disputed. Everyone else is either confusing this with the pg_controldata part or is just transitively claiming that someone else wanted it. I don't have a problem with the implementation, but I don't understand what this feature is meant for.
On 2/18/16 12:20 PM, Joe Conway wrote: > Just to be clear, AFAIK there is no issue with the committed changes on > Windows. If there is please provide a concrete example that we can discuss. Originally it was mentioned that this feature could be used in the regression tests by making certain tests conditional on configure options. Which presumably won't work if the build was on Windows. I don't doubt that your code actually works on Windows.
On Fri, Feb 19, 2016 at 11:53 AM, Peter Eisentraut <peter_e@gmx.net> wrote: > On 2/18/16 12:20 PM, Joe Conway wrote: >> Just to be clear, AFAIK there is no issue with the committed changes on >> Windows. If there is please provide a concrete example that we can discuss. > > Originally it was mentioned that this feature could be used in the > regression tests by making certain tests conditional on configure > options. Which presumably won't work if the build was on Windows. MSVC code passes VAL_CONFIGURE to pg_config.h by calling GetFakeConfigure() and make the output of pg_config consistent with when ./configure is used. So for CONFIGURE I see no issues. Things like CPPFLAGS or LIBS though become listed as "not recorded" with this change so the output of pg_config is more verbose when MSVC is used. This still seems an acceptable trade-off even after reviewing this patch to make this information available on the backend. And it seems as well that this would become set, at least partially, when using cmake build instead of the MSVC cruft in src/tools/msvc. -- Michael
On 01/17/2016 04:10 PM, Joe Conway wrote: > On 01/16/2016 06:02 AM, Michael Paquier wrote: >> On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway <mail@joeconway.com> wrote: >>> 3) Adds new functions, more or less in line with previous discussions: >>> * pg_checkpoint_state() >>> * pg_controldata_state() >>> * pg_recovery_state() >>> * pg_init_state() >> >> Taking the opposite direction of Josh upthread, why is this split >> actually necessary? Isn't the idea to provide a SQL interface of what >> pg_controldata shows? If this split proves to be useful, shouldn't we >> do it as well for pg_controldata? > > The last discussion moved strongly in the direction of separate > functions, and that being different from pg_controldata was not a bad > thing. That said, I'm still of the opinion that there are legitimate > reasons to want the command line pg_controldata and the SQL functions to > produce equivalent, if not identical, results. I just wish we could get > a clear consensus one way or the other. I've assumed that we are sticking with the separate functions. As such, here is a rebased patch, with documentation and other fixes such as Copyright year, Mkvcbuild support, and some cruft removal. >> I think that those functions should be superuser-only. They provide >> information about the system globally. > > The tail of this thread seems to be headed away from this direction. > I'll take another look and propose something concrete. I've looked at existing functions that seem similar, and as far as I can see none are superuser-only. I'm certainly happy to make them so if that's the consensus, but currently they are wide open. Opinions? For convenience in answering that question, here is what information is included in the output of each function (\df so you can see the data types, plus SELECT output for a more readable example): 8<------------------------- postgres=# \x Expanded display is on. postgres=# \df pg_checkpoint_state Name | pg_checkpoint_state Result data type | record Argument data types | OUT checkpoint_location pg_lsn, OUT prior_location pg_lsn, OUT redo_location pg_lsn, OUT redo_wal_file text, OUT timeline_id integer, OUT prev_timeline_id integer, OUT full_page_writes boolean, OUT next_xid text, OUT next_oid oid, OUT next_multixact_id xid, OUT next_multi_offset xid, OUT oldest_xid xid, OUT oldest_xid_dbid oid, OUT oldest_active_xid xid, OUT oldest_multi_xid xid, OUT oldest_multi_dbid oid, OUT oldest_commit_ts_xid xid, OUT newest_commit_ts_xid xid, OUT checkpoint_time timestamp with time zone postgres=# select * from pg_checkpoint_state(); -[ RECORD 1 ]--------+------------------------- checkpoint_location | 0/14CD368 prior_location | 0/14CD0D0 redo_location | 0/14CD368 redo_wal_file | 000000010000000000000001 timeline_id | 1 prev_timeline_id | 1 full_page_writes | t next_xid | 0:576 next_oid | 12415 next_multixact_id | 1 next_multi_offset | 0 oldest_xid | 568 oldest_xid_dbid | 1 oldest_active_xid | 0 oldest_multi_xid | 1 oldest_multi_dbid | 1 oldest_commit_ts_xid | 0 newest_commit_ts_xid | 0 checkpoint_time | 2016-02-19 18:44:51-08 postgres=# \df pg_controldata_state Name | pg_controldata_state Result data type | record Argument data types | OUT pg_control_version integer, OUT catalog_version_no integer, OUT system_identifier bigint, OUT pg_control_last_modified timestamp with time zone postgres=# select * from pg_controldata_state(); -[ RECORD 1 ]------------+----------------------- pg_control_version | 942 catalog_version_no | 201602171 system_identifier | 6253198751269127743 pg_control_last_modified | 2016-02-19 18:44:58-08 postgres=# \df pg_init_state Name | pg_init_state Result data type | record Argument data types | OUT max_data_alignment integer, OUT database_block_size integer, OUT blocks_per_segment integer, OUT wal_block_size integer, OUT bytes_per_wal_segment integer, OUT max_identifier_length integer, OUT max_index_columns integer, OUT max_toast_chunk_size integer, OUT large_object_chunk_size integer, OUT bigint_timestamps boolean, OUT float4_pass_by_value boolean, OUT float8_pass_by_value boolean, OUT data_page_checksum_version integer postgres=# select * from pg_init_state(); -[ RECORD 1 ]--------------+--------- max_data_alignment | 8 database_block_size | 8192 blocks_per_segment | 131072 wal_block_size | 8192 bytes_per_wal_segment | 16777216 max_identifier_length | 64 max_index_columns | 32 max_toast_chunk_size | 1996 large_object_chunk_size | 2048 bigint_timestamps | t float4_pass_by_value | t float8_pass_by_value | t data_page_checksum_version | 0 postgres=# \df pg_recovery_state Result data type | record Argument data types | OUT min_recovery_end_location pg_lsn, OUT min_recovery_end_timeline integer, OUT backup_start_location pg_lsn, OUT backup_end_location pg_lsn, OUT end_of_backup_record_required boolean postgres=# select * from pg_recovery_state(); -[ RECORD 1 ]-----------------+---- min_recovery_end_location | 0/0 min_recovery_end_timeline | 0 backup_start_location | 0/0 backup_end_location | 0/0 end_of_backup_record_required | f 8<------------------------- Is there general consensus that we want this feature, and that we want it in this form? Any other comments? Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Вложения
On Sat, Feb 20, 2016 at 12:12 PM, Joe Conway <mail@joeconway.com> wrote:
> On 01/17/2016 04:10 PM, Joe Conway wrote:
>> On 01/16/2016 06:02 AM, Michael Paquier wrote:
>>> On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway <mail@joeconway.com> wrote:
>>>> 3) Adds new functions, more or less in line with previous discussions:
>>>> * pg_checkpoint_state()
>>>> * pg_controldata_state()
>>>> * pg_recovery_state()
>>>> * pg_init_state()
>>>
>>> Taking the opposite direction of Josh upthread, why is this split
>>> actually necessary? Isn't the idea to provide a SQL interface of what
>>> pg_controldata shows? If this split proves to be useful, shouldn't we
>>> do it as well for pg_controldata?
>>
>> The last discussion moved strongly in the direction of separate
>> functions, and that being different from pg_controldata was not a bad
>> thing. That said, I'm still of the opinion that there are legitimate
>> reasons to want the command line pg_controldata and the SQL functions to
>> produce equivalent, if not identical, results. I just wish we could get
>> a clear consensus one way or the other.
>
> I've assumed that we are sticking with the separate functions. As such,
> here is a rebased patch, with documentation and other fixes such as
> Copyright year, Mkvcbuild support, and some cruft removal.
Looking again at this thread I guess that this is consensus, based on
the proposal from Josh and seeing no other ideas around. Another idea
would be to group all the fields that into a single function
pg_control_data().
> Is there general consensus that we want this feature, and that we want
> it in this form? Any other comments?
I had a look at this patch.
+ <indexterm>
+ <primary>pg_checkpoint_state</primary>
+ </indexterm>
+ <para>
+ <function>pg_checkpoint_state</> returns a record containing
+ checkpoint_location, prior_location, redo_location, redo_wal_file,
+ timeline_id, prev_timeline_id, full_page_writes, next_xid, next_oid,
+ next_multixact_id, next_multi_offset, oldest_xid, oldest_xid_dbid,
+ oldest_active_xid, oldest_multi_xid, oldest_multi_dbid,
+ oldest_commit_ts_xid, newest_commit_ts_xid, and checkpoint_time.
+ </para>
This is bit unreadable. The only entry in the documentation that
adopts a similar style is pg_stat_file, and with six fields that feels
as being enough. I would suggest using a table instead with the type
of the field and its name.
Regarding the naming of the functions, I think that it would be good
to get something consistent with the concept of those being "Control
Data functions" by having them share the same prefix, say pg_control_
- pg_control_checkpoint
- pg_control_init
- pg_control_system
- pg_control_recovery
+ snprintf (controldata_name, CONTROLDATANAME_LEN,
+ "%s:", controldata[i].name);
Nitpick: extra space.
+static const char *const controldata_names[] =
+{
+ gettext_noop("pg_control version number"),
+ gettext_noop("Catalog version number"),
+ gettext_noop("Database system identifier"),
Is this complication really necessary? Those identifiers are used only
in the frontend and the footprint of this patch on pg_controldata is
really large. What I think we should do is have in src/common the
following set of routines that work directly on ControlFileData:
- checkControlFile, to perform basic sanity checks on the control file
(CRC, see for example pg_rewind.c)
- getControlFile(dataDir), that simply returns a palloc'd
ControlFileData to the caller after looking at global/pg_control.
pg_rewind could perhaps make use of the one to check the control file
CRC, to fetch ControlFileData there is some parallel logic for the
source server if it is either remote or local so it would be better to
not use getControlFile in this case.
--
Michael
On 02/21/2016 05:30 AM, Michael Paquier wrote:
> Looking again at this thread I guess that this is consensus, based on
> the proposal from Josh and seeing no other ideas around. Another idea
> would be to group all the fields that into a single function
> pg_control_data().
I think a single function would be ridiculously wide. I like the four
separate functions better if we're going to do it this way at all.
> + <indexterm>
> + <primary>pg_checkpoint_state</primary>
> + </indexterm>
> + <para>
> + <function>pg_checkpoint_state</> returns a record containing
> + checkpoint_location, prior_location, redo_location, redo_wal_file,
> + timeline_id, prev_timeline_id, full_page_writes, next_xid, next_oid,
> + next_multixact_id, next_multi_offset, oldest_xid, oldest_xid_dbid,
> + oldest_active_xid, oldest_multi_xid, oldest_multi_dbid,
> + oldest_commit_ts_xid, newest_commit_ts_xid, and checkpoint_time.
> + </para>
> This is bit unreadable. The only entry in the documentation that
> adopts a similar style is pg_stat_file, and with six fields that feels
> as being enough. I would suggest using a table instead with the type
> of the field and its name.
Ok, changed to your suggestion.
> Regarding the naming of the functions, I think that it would be good
> to get something consistent with the concept of those being "Control
> Data functions" by having them share the same prefix, say pg_control_
> - pg_control_checkpoint
> - pg_control_init
> - pg_control_system
> - pg_control_recovery
No issues -- changed.
> + snprintf (controldata_name, CONTROLDATANAME_LEN,
> + "%s:", controldata[i].name);
> Nitpick: extra space.
I didn't understand this comment but it is moot now anyway...
> +static const char *const controldata_names[] =
> +{
> + gettext_noop("pg_control version number"),
> + gettext_noop("Catalog version number"),
> + gettext_noop("Database system identifier"),
> Is this complication really necessary? Those identifiers are used only
> in the frontend and the footprint of this patch on pg_controldata is
> really large. What I think we should do is have in src/common the
> following set of routines that work directly on ControlFileData:
> - checkControlFile, to perform basic sanity checks on the control file
> (CRC, see for example pg_rewind.c)
> - getControlFile(dataDir), that simply returns a palloc'd
> ControlFileData to the caller after looking at global/pg_control.
> pg_rewind could perhaps make use of the one to check the control file
> CRC, to fetch ControlFileData there is some parallel logic for the
> source server if it is either remote or local so it would be better to
> not use getControlFile in this case.
I agree with the assessment that much of what had been moved based on
the original pg_controladata() SRF no longer needs to move. This version
only puts get_controlfile() into src/common, since that is the bit that
is still shared. If checkControlFile() or something similar is useful
for pg_rewind or some other extension, I'd say that should be a separate
patch.
Oh, and the entire thing is now rebased against a git pull from a few
hours ago. I moved this to the upcoming commitfest too, although I think
it is pretty well ready to go.
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Вложения
On Sun, Feb 28, 2016 at 9:33 AM, Joe Conway <mail@joeconway.com> wrote:
> On 02/21/2016 05:30 AM, Michael Paquier wrote:
>> Looking again at this thread I guess that this is consensus, based on
>> the proposal from Josh and seeing no other ideas around. Another idea
>> would be to group all the fields that into a single function
>> pg_control_data().
>
> I think a single function would be ridiculously wide. I like the four
> separate functions better if we're going to do it this way at all.
>
>> + <indexterm>
>> + <primary>pg_checkpoint_state</primary>
>> + </indexterm>
>> + <para>
>> + <function>pg_checkpoint_state</> returns a record containing
>> + checkpoint_location, prior_location, redo_location, redo_wal_file,
>> + timeline_id, prev_timeline_id, full_page_writes, next_xid, next_oid,
>> + next_multixact_id, next_multi_offset, oldest_xid, oldest_xid_dbid,
>> + oldest_active_xid, oldest_multi_xid, oldest_multi_dbid,
>> + oldest_commit_ts_xid, newest_commit_ts_xid, and checkpoint_time.
>> + </para>
>> This is bit unreadable. The only entry in the documentation that
>> adopts a similar style is pg_stat_file, and with six fields that feels
>> as being enough. I would suggest using a table instead with the type
>> of the field and its name.
>
> Ok, changed to your suggestion.
>
>
>> Regarding the naming of the functions, I think that it would be good
>> to get something consistent with the concept of those being "Control
>> Data functions" by having them share the same prefix, say pg_control_
>> - pg_control_checkpoint
>> - pg_control_init
>> - pg_control_system
>> - pg_control_recovery
>
> No issues -- changed.
>
>> + snprintf (controldata_name, CONTROLDATANAME_LEN,
>> + "%s:", controldata[i].name);
>> Nitpick: extra space.
>
> I didn't understand this comment but it is moot now anyway...
>
>> +static const char *const controldata_names[] =
>> +{
>> + gettext_noop("pg_control version number"),
>> + gettext_noop("Catalog version number"),
>> + gettext_noop("Database system identifier"),
>> Is this complication really necessary? Those identifiers are used only
>> in the frontend and the footprint of this patch on pg_controldata is
>> really large. What I think we should do is have in src/common the
>> following set of routines that work directly on ControlFileData:
>> - checkControlFile, to perform basic sanity checks on the control file
>> (CRC, see for example pg_rewind.c)
>> - getControlFile(dataDir), that simply returns a palloc'd
>> ControlFileData to the caller after looking at global/pg_control.
>> pg_rewind could perhaps make use of the one to check the control file
>> CRC, to fetch ControlFileData there is some parallel logic for the
>> source server if it is either remote or local so it would be better to
>> not use getControlFile in this case.
>
> I agree with the assessment that much of what had been moved based on
> the original pg_controladata() SRF no longer needs to move. This version
> only puts get_controlfile() into src/common, since that is the bit that
> is still shared. If checkControlFile() or something similar is useful
> for pg_rewind or some other extension, I'd say that should be a separate
> patch.
>
> Oh, and the entire thing is now rebased against a git pull from a few
> hours ago. I moved this to the upcoming commitfest too, although I think
> it is pretty well ready to go.
Thanks for the updated version.
+ Returns information about current controldata file state.
s/controldata/control data?
+ <tgroup cols="2">
+ <thead>
+ <row>
+ <entry>Column Name</entry>
+ <entry>Data Type</entry>
+ </row>
+ </thead>
+
Having a description of each field would be nice.
+ * pg_controldata.c
+ * Expose select pg_controldata output, except via SQL functions
I am not much getting the meaning of this sentence. What about the following:
"Set of routines exposing the contents of the control data file in a
set of SQL functions".
+ /* Populate the values and null arrays */
+ values[0] = LSNGetDatum(ControlFile->checkPoint);
+ nulls[0] = false;
+
+ values[1] = LSNGetDatum(ControlFile->prevCheckPoint);
+ nulls[1] = false;
Instead of setting each flag of nulls[] one by one, just calling once
MemSet would be fine. Any method is fine though.
+ /* get a copy of the control file */
+ ControlFile = get_controlfile(DataDir, progname);
Some whitespaces here. git diff is showing in red here.
+ if (ControlFile->pg_control_version % 65536 == 0 &&
+ ControlFile->pg_control_version / 65536 != 0)
+ elog(ERROR, _("byte ordering mismatch"));
You may want to put this check directly in get_controlfile(). it is
repeated 4 times in the backend, and has an equivalent in
pg_controldata.
our @pgcommonallfiles = qw(
- config_info.c exec.c pg_lzcompress.c pgfnames.c psprintf.c
+ config_info.c controldata_utils.c exec.c pg_lzcompress.c pgfnames.c relpath.c rmtree.c string.c username.c
wait_error.c);
"psprintf.c" has been removed from this list. This causes the build
with MSVC to fail.
--
Michael
On 02/28/2016 05:16 AM, Michael Paquier wrote:
> + Returns information about current controldata file state.
> s/controldata/control data?
>
> + <tgroup cols="2">
> + <thead>
> + <row>
> + <entry>Column Name</entry>
> + <entry>Data Type</entry>
> + </row>
> + </thead>
> +
> Having a description of each field would be nice.
Might be nice, but the pg_controldata section of the docs does not have
any description either, and I don't feel compelled to improve on that
just for the sake of this patch. I'll put it on my TODO to improve both
at some point though.
> + * pg_controldata.c
> + * Expose select pg_controldata output, except via SQL functions
> I am not much getting the meaning of this sentence. What about the following:
> "Set of routines exposing the contents of the control data file in a
> set of SQL functions".
Ok, reworded to something similar.
> + /* Populate the values and null arrays */
> + values[0] = LSNGetDatum(ControlFile->checkPoint);
> + nulls[0] = false;
> +
> + values[1] = LSNGetDatum(ControlFile->prevCheckPoint);
> + nulls[1] = false;
> Instead of setting each flag of nulls[] one by one, just calling once
> MemSet would be fine. Any method is fine though.
I prefer the current style and I believe it is more consistent
with what is done elsewhere. In any case this is not exactly a
performance critical path.
> + /* get a copy of the control file */
> + ControlFile = get_controlfile(DataDir, progname);
> Some whitespaces here. git diff is showing in red here.
fixed
> + if (ControlFile->pg_control_version % 65536 == 0 &&
> + ControlFile->pg_control_version / 65536 != 0)
> + elog(ERROR, _("byte ordering mismatch"));
> You may want to put this check directly in get_controlfile(). it is
> repeated 4 times in the backend, and has an equivalent in
> pg_controldata.
makes sense - done
> our @pgcommonallfiles = qw(
> - config_info.c exec.c pg_lzcompress.c pgfnames.c psprintf.c
> + config_info.c controldata_utils.c exec.c pg_lzcompress.c pgfnames.c
> relpath.c rmtree.c string.c username.c wait_error.c);
> "psprintf.c" has been removed from this list. This causes the build
> with MSVC to fail.
good catch -- fixed
If there are no other complaints or comments, I will commit the attached
sometime this coming week (the the requisite catversion bump).
Thanks!
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Вложения
On Mon, Feb 29, 2016 at 3:50 AM, Joe Conway <mail@joeconway.com> wrote: > If there are no other complaints or comments, I will commit the attached > sometime this coming week (the the requisite catversion bump). Thanks for the updated patch, I have nothing else to say on my side. The new version has fixed the MSVC build, I just double-checked it. -- Michael
On 02/28/2016 07:45 PM, Michael Paquier wrote: > On Mon, Feb 29, 2016 at 3:50 AM, Joe Conway <mail@joeconway.com> wrote: >> If there are no other complaints or comments, I will commit the attached >> sometime this coming week (the the requisite catversion bump). > > Thanks for the updated patch, I have nothing else to say on my side. > The new version has fixed the MSVC build, I just double-checked it. Pushed with catversion bump. Thanks, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development