Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 02/28/2016 07:45 PM, Michael Paquier wrote:
> On Mon, Feb 29, 2016 at 3:50 AM, Joe Conway  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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Mon, Feb 29, 2016 at 3:50 AM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 02/28/2016 05:16 AM, Michael Paquier wrote:
> +Returns information about current controldata file state.
> s/controldata/control data?
> 
> +
> + 
> +  
> +   Column Name
> +   Data Type
> +  
> + 
> +
> 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

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index c0b94bc..4b5ee81 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT collation for ('foo' COLLATE "de_
*** 16703,16708 
--- 16703,17064 
  
 
  
+
+ The functions shown in 
+ print information initialized during initdb, such
+ as the catalog version. They also show information about write-ahead
+ logging and checkpoint processing. This information is cluster-wide,
+ and not specific to any one database. They provide most of the same
+ information, from the same source, as
+ , although in a form better suited
+ to SQL functions.
+
+ 
+
+ Control Data Functions
+ 
+  
+   Name Return Type Description
+  
+ 
+  
+   
+
+ pg_control_checkpoint
+ pg_control_checkpoint()
+
+record
+
+ Returns information about current checkpoint state.
+
+   
+ 
+   
+
+ pg_control_system
+ pg_control_system()
+
+record
+
+ Returns information about current controldata file state.
+
+   
+ 
+   
+
+ pg_control_init
+ pg_control_init()
+
+record
+
+ Returns information about cluster initialization state.
+
+   
+ 
+   
+
+ pg_control_recovery
+ pg_control_recovery()
+
+record
+
+ Returns information about recovery state.
+
+   
+ 
+  
+ 
+
+ 
+
+ pg_control_checkpoint returns a record, shown in
+ 
+
+ 
+
+ pg_control_checkpoint Columns
+ 
+  
+   
+Column Name
+Data Type
+   
+  
+ 
+  
+ 
+   
+checkpoint_location
+pg_lsn
+   
+ 
+   
+prior_location
+pg_lsn
+   
+ 
+   
+redo_location
+pg_lsn
+   
+ 
+   
+redo_wal_file
+text
+   
+ 
+   
+timeline_id
+integer
+   
+ 
+   
+prev_timeline_id
+integer
+   
+ 
+   
+full_page_writes
+boolean
+   
+ 
+   
+next_xid
+text
+   
+ 
+   
+next_oid
+oid
+   
+ 
+   
+next_multixact_id
+xid
+   
+ 
+   
+next_multi_offset
+xid
+   
+ 
+   
+oldest_xid
+  

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Sun, Feb 28, 2016 at 9:33 AM, Joe Conway  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.
>
>> +   
>> +pg_checkpoint_state
>> +   
>> +   
>> +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.
>> +   
>> 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?

+
+ 
+  
+   Column Name
+   Data Type
+  
+ 
+
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


-- 
Sent via 

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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.

> +   
> +pg_checkpoint_state
> +   
> +   
> +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.
> +   
> 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
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index c0b94bc..4b5ee81 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT collation for ('foo' COLLATE "de_
*** 16703,16708 
--- 16703,17064 
  
 
  
+
+ The functions shown in 
+ print information initialized during initdb, such
+ as the catalog version. They also show information about write-ahead
+ logging and checkpoint processing. This information is cluster-wide,
+ and not specific to any one database. They provide most of the same
+ information, from the same source, as
+ , although in a form better suited
+ to SQL functions.
+
+ 
+
+ Control Data Functions
+ 
+  
+   Name Return Type Description
+  
+ 
+  
+   
+
+ pg_control_checkpoint
+ pg_control_checkpoint()
+
+record
+
+ Returns information about current checkpoint state.
+
+   
+ 
+   
+
+ pg_control_system
+ pg_control_system()
+
+record
+
+ Returns information about current controldata file state.
+
+   
+ 
+   
+
+ pg_control_init
+ pg_control_init()
+
+record
+
+ Returns information about cluster initialization state.
+
+   
+ 
+   
+
+ pg_control_recovery
+ pg_control_recovery()
+
+record
+
+ Returns information about recovery state.
+
+   
+ 
+  
+ 
+
+ 
+
+ pg_control_checkpoint returns 

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Sat, Feb 20, 2016 at 12:12 PM, Joe Conway  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  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.

+   
+pg_checkpoint_state
+   
+   
+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.
+   
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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  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| 00010001
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, 

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Fri, Feb 19, 2016 at 11:53 AM, Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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  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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Andres Freund]

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  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

On 2/17/16 9:08 PM, Michael Paquier wrote:
> On Thu, Feb 18, 2016 at 11:02 AM, Peter Eisentraut  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?).


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Thu, Feb 18, 2016 at 11:02 AM, Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 02/17/2016 03:34 PM, Josh berkus wrote:
> On 02/17/2016 03:02 PM, Tom Lane wrote:
>> Joe Conway  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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Josh berkus]

On 02/17/2016 03:02 PM, Tom Lane wrote:

Joe Conway  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)


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Tom Lane]

Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Andrew Dunstan]

On 02/17/2016 05:14 PM, Tom Lane wrote:

Peter Eisentraut  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



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Josh berkus]

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)


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 02/17/2016 02:14 PM, Tom Lane wrote:
> Peter Eisentraut  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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Tom Lane]

Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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?



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Wed, Feb 17, 2016 at 10:13 AM, Michael Paquier
 wrote:
> On Tue, Feb 16, 2016 at 11:36 PM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Tue, Feb 16, 2016 at 11:36 PM, Joe Conway  wrote:
> Thanks!

OK. I think I'm good now. Thanks for the quick update.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 02/15/2016 11:20 PM, Michael Paquier wrote:
> Here are just a couple of cosmetic comments.

> Missing markup  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  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
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 412c845..7b71768 100644
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
***
*** 7350,7355 
--- 7350,7360 
   
  
   
+   pg_config
+   compile-time configuration parameters
+  
+ 
+  
pg_cursors
open cursors
   
***
*** 7609,7614 
--- 7614,7668 

   
  
+  
+   pg_config
+ 
+   
+pg_config
+   
+ 
+   
+The view pg_config 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
+ PostgreSQL Client
+Application. There is a System Information Function
+() called pg_config
+which underlies this view.
+   
+ 
+   
+pg_config Columns
+
+ 
+  
+   Name
+   Type
+   Description
+  
+ 
+ 
+ 
+  
+   name
+   text
+   The parameter name
+  
+ 
+  
+   setting
+   text
+   The parameter value
+  
+ 
+
+   
+ 
+  
+ 
   
pg_cursors
  
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index f9eea76..0e17ca3 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT * FROM pg_ls_dir('.') WITH ORDINA
*** 15003,15008 
--- 15003,15014 

  

+pg_config()
+setof record
+get list of compile-time configure variable names and their values
+   
+ 
+   
 pg_is_other_temp_schema(oid)
 boolean
 is schema another session's temporary schema?
*** SET search_path TO schema
  
 
+ pg_config
+
+ 
+
+ pg_config returns a set of records describing 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. The name column contains the
+ parameter name. The setting column contains the
+ parameter value. It provides the same basic information as the
+  PostgreSQL Client
+ Application. There is also a  system
+ view.
+
+ 
+
  version
 
  
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 923fe58..abf9a70 100644
*** a/src/backend/catalog/system_views.sql
--- 

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Tue, Feb 16, 2016 at 5:29 AM, Joe Conway  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 pg_config 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  PostgreSQL Client
+   Application. There is a System Information Function
Missing markup  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 01/20/2016 08:08 PM, Michael Paquier wrote:
> On Wed, Jan 20, 2016 at 2:32 AM, Joe Conway  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
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 412c845..6c50f79 100644
*** a/doc/src/sgml/catalogs.sgml
--- b/doc/src/sgml/catalogs.sgml
***
*** 7350,7355 
--- 7350,7360 
   
  
   
+   pg_config
+   compile-time configuration parameters
+  
+ 
+  
pg_cursors
open cursors
   
***
*** 7609,7614 
--- 7614,7667 

   
  
+  
+   pg_config
+ 
+   
+pg_config
+   
+ 
+   
+The view pg_config 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  PostgreSQL Client
+Application. There is a System Information Function
+() called pg_config
+which underlies this view.
+   
+ 
+   
+pg_config Columns
+
+ 
+  
+   Name
+   Type
+   Description
+  
+ 
+ 
+ 
+  
+   name
+   text
+   The parameter name
+  
+ 
+  
+   setting
+   text
+   The parameter value
+  
+ 
+
+   
+ 
+  
+ 
   
pg_cursors
  
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index f9eea76..29c36d2 100644
*** a/doc/src/sgml/func.sgml
--- b/doc/src/sgml/func.sgml
*** SELECT * FROM pg_ls_dir('.') WITH ORDINA
*** 15003,15008 
--- 15003,15014 

  

+pg_config()
+setof record
+get list of compile-time configure variable names and their values
+   
+ 
+   
 pg_is_other_temp_schema(oid)
 boolean
 is schema another session's temporary schema?
*** SET search_path TO schema
  
 
+ pg_config
+
+ 
+
+ pg_config returns a set of records describing 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. The
+ name column contains the parameter name.
+ The setting column contains the parameter value. It
+ provides the same basic information as the
+  PostgreSQL Client Application. There
+ is also a  system view.
+
+ 
+
  version
 
  
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 923fe58..abf9a70 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*** CREATE VIEW pg_timezone_abbrevs AS
*** 433,438 
--- 433,444 
  CREATE VIEW pg_timezone_names AS
  SELECT * FROM pg_timezone_names();
  
+ CREATE VIEW pg_config AS
+ SELECT * FROM pg_config();
+ 
+ REVOKE ALL on pg_config FROM PUBLIC;
+ REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC;
+ 
  -- Statistics views
  
  CREATE VIEW pg_stat_all_tables AS
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index 7889101..a0c82c1 100644
*** 

Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)

[Joe Conway]

On 02/11/2016 04:59 PM, Michael Paquier wrote:
> On Fri, Feb 12, 2016 at 9:18 AM, 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.
> 
> 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



signature.asc
Description: OpenPGP digital signature

Re: NextXID format change (was Re: [HACKERS] 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  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)

[Michael Paquier]

On Sat, Feb 13, 2016 at 7:54 AM, Bruce Momjian  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: NextXID format change (was Re: [HACKERS] 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  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  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)

[Michael Paquier]

On Fri, Feb 12, 2016 at 9:18 AM, Bruce Momjian  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  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)

[Joe Conway]

On 01/19/2016 07:04 PM, Michael Paquier wrote:
> On Wed, Jan 20, 2016 at 11:41 AM, Alvaro Herrera
>  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
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index b694dea..2dcbfbd 100644
*** a/src/backend/access/rmgrdesc/xlogdesc.c
--- b/src/backend/access/rmgrdesc/xlogdesc.c
*** xlog_desc(StringInfo buf, XLogReaderStat
*** 43,49 
  		CheckPoint *checkpoint = (CheckPoint *) rec;
  
  		appendStringInfo(buf, "redo %X/%X; "
! 		 "tli %u; prev tli %u; fpw %s; xid %u/%u; oid %u; multi %u; offset %u; "
  		 "oldest xid %u in DB %u; oldest multi %u in DB %u; "
  		 "oldest/newest commit timestamp xid: %u/%u; "
  		 "oldest running xid %u; %s",
--- 43,49 
  		CheckPoint *checkpoint = (CheckPoint *) rec;
  
  		appendStringInfo(buf, "redo %X/%X; "
! 		 "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
  		 "oldest xid %u in DB %u; oldest multi %u in DB %u; "
  		 "oldest/newest commit timestamp xid: %u/%u; "
  		 "oldest running xid %u; %s",
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7d5d493..ee87e1b 100644
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
*** StartupXLOG(void)
*** 6284,6290 
    (uint32) (checkPoint.redo >> 32), (uint32) checkPoint.redo,
  	wasShutdown ? "TRUE" : "FALSE")));
  	ereport(DEBUG1,
! 			(errmsg_internal("next transaction ID: %u/%u; next OID: %u",
  	checkPoint.nextXidEpoch, checkPoint.nextXid,
  	checkPoint.nextOid)));
  	ereport(DEBUG1,
--- 6284,6290 
    (uint32) (checkPoint.redo >> 32), (uint32) checkPoint.redo,
  	wasShutdown ? "TRUE" : "FALSE")));
  	ereport(DEBUG1,
! 			(errmsg_internal("next transaction ID: %u:%u; next OID: %u",
  	checkPoint.nextXidEpoch, checkPoint.nextXid,
  	checkPoint.nextOid)));
  	ereport(DEBUG1,
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index e7e072f..5dd2dbc 100644
*** a/src/bin/pg_controldata/pg_controldata.c
--- b/src/bin/pg_controldata/pg_controldata.c
*** main(int argc, char *argv[])
*** 252,258 
  		   ControlFile.checkPointCopy.PrevTimeLineID);
  	printf(_("Latest checkpoint's full_page_writes: %s\n"),
  		   ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
! 	printf(_("Latest checkpoint's NextXID:  %u/%u\n"),
  		   ControlFile.checkPointCopy.nextXidEpoch,
  		   ControlFile.checkPointCopy.nextXid);
  	printf(_("Latest checkpoint's NextOID:  %u\n"),
--- 252,258 
  		   ControlFile.checkPointCopy.PrevTimeLineID);
  	printf(_("Latest checkpoint's full_page_writes: %s\n"),
  		   ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
! 	printf(_("Latest checkpoint's NextXID:  %u:%u\n"),
  		   ControlFile.checkPointCopy.nextXidEpoch,
  		   ControlFile.checkPointCopy.nextXid);
  	printf(_("Latest checkpoint's NextOID:  %u\n"),
diff --git a/src/bin/pg_resetxlog/pg_resetxlog.c b/src/bin/pg_resetxlog/pg_resetxlog.c
index ca706a5..525b82b 100644
*** a/src/bin/pg_resetxlog/pg_resetxlog.c
--- 

Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)

[Michael Paquier]

On Wed, Feb 10, 2016 at 9:57 AM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Thu, Jan 21, 2016 at 1:08 PM, Michael Paquier
 wrote:
> On Wed, Jan 20, 2016 at 2:32 AM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Wed, Jan 20, 2016 at 2:32 AM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: NextXID format change (was Re: [HACKERS] 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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)

[Michael Paquier]

On Wed, Jan 20, 2016 at 11:41 AM, Alvaro Herrera
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

NextXID format change (was Re: [HACKERS] exposing pg_controldata and pg_config as functions)

[Joe Conway]

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
diff --git a/src/backend/access/rmgrdesc/xlogdesc.c b/src/backend/access/rmgrdesc/xlogdesc.c
index b694dea..2dcbfbd 100644
*** a/src/backend/access/rmgrdesc/xlogdesc.c
--- b/src/backend/access/rmgrdesc/xlogdesc.c
*** xlog_desc(StringInfo buf, XLogReaderStat
*** 43,49 
  		CheckPoint *checkpoint = (CheckPoint *) rec;
  
  		appendStringInfo(buf, "redo %X/%X; "
! 		 "tli %u; prev tli %u; fpw %s; xid %u/%u; oid %u; multi %u; offset %u; "
  		 "oldest xid %u in DB %u; oldest multi %u in DB %u; "
  		 "oldest/newest commit timestamp xid: %u/%u; "
  		 "oldest running xid %u; %s",
--- 43,49 
  		CheckPoint *checkpoint = (CheckPoint *) rec;
  
  		appendStringInfo(buf, "redo %X/%X; "
! 		 "tli %u; prev tli %u; fpw %s; xid %u:%u; oid %u; multi %u; offset %u; "
  		 "oldest xid %u in DB %u; oldest multi %u in DB %u; "
  		 "oldest/newest commit timestamp xid: %u/%u; "
  		 "oldest running xid %u; %s",
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7d5d493..ee87e1b 100644
*** a/src/backend/access/transam/xlog.c
--- b/src/backend/access/transam/xlog.c
*** StartupXLOG(void)
*** 6284,6290 
    (uint32) (checkPoint.redo >> 32), (uint32) checkPoint.redo,
  	wasShutdown ? "TRUE" : "FALSE")));
  	ereport(DEBUG1,
! 			(errmsg_internal("next transaction ID: %u/%u; next OID: %u",
  	checkPoint.nextXidEpoch, checkPoint.nextXid,
  	checkPoint.nextOid)));
  	ereport(DEBUG1,
--- 6284,6290 
    (uint32) (checkPoint.redo >> 32), (uint32) checkPoint.redo,
  	wasShutdown ? "TRUE" : "FALSE")));
  	ereport(DEBUG1,
! 			(errmsg_internal("next transaction ID: %u:%u; next OID: %u",
  	checkPoint.nextXidEpoch, checkPoint.nextXid,
  	checkPoint.nextOid)));
  	ereport(DEBUG1,
diff --git a/src/bin/pg_controldata/pg_controldata.c b/src/bin/pg_controldata/pg_controldata.c
index e7e072f..5dd2dbc 100644
*** a/src/bin/pg_controldata/pg_controldata.c
--- b/src/bin/pg_controldata/pg_controldata.c
*** main(int argc, char *argv[])
*** 252,258 
  		   ControlFile.checkPointCopy.PrevTimeLineID);
  	printf(_("Latest checkpoint's full_page_writes: %s\n"),
  		   ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
! 	printf(_("Latest checkpoint's NextXID:  %u/%u\n"),
  		   ControlFile.checkPointCopy.nextXidEpoch,
  		   ControlFile.checkPointCopy.nextXid);
  	printf(_("Latest checkpoint's NextOID:  %u\n"),
--- 252,258 
  		   ControlFile.checkPointCopy.PrevTimeLineID);
  	printf(_("Latest checkpoint's full_page_writes: %s\n"),
  		   ControlFile.checkPointCopy.fullPageWrites ? _("on") : _("off"));
! 	printf(_("Latest checkpoint's NextXID:  %u:%u\n"),
  		   ControlFile.checkPointCopy.nextXidEpoch,
  		   ControlFile.checkPointCopy.nextXid);
  	printf(_("Latest checkpoint's NextOID:  %u\n"),
diff --git a/src/bin/pg_resetxlog/pg_resetxlog.c b/src/bin/pg_resetxlog/pg_resetxlog.c
index ca706a5..525b82b 100644
*** a/src/bin/pg_resetxlog/pg_resetxlog.c
--- b/src/bin/pg_resetxlog/pg_resetxlog.c
*** PrintControlValues(bool guessed)
*** 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"),
  		   ControlFile.checkPointCopy.nextXidEpoch,
  		   ControlFile.checkPointCopy.nextXid);
  	printf(_("Latest checkpoint's NextOID:  %u\n"),
--- 646,652 
  		   ControlFile.checkPointCopy.ThisTimeLineID);
  	printf(_("Latest 

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Bruce Momjian]

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  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +
diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
new file mode 100644
index 2def729..34e194c
*** a/src/bin/pg_upgrade/controldata.c
--- b/src/bin/pg_upgrade/controldata.c
*** get_control_data(ClusterInfo *cluster, b
*** 197,207 
  			p++;/* remove ':' char */
  			cluster->controldata.chkpnt_nxtepoch = str2uint(p);
  
! 			p = strchr(p, '/');
  			if (p == NULL || strlen(p) <= 1)
  pg_fatal("%d: controldata retrieval problem\n", __LINE__);
  
! 			p++;/* remove '/' char */
  			cluster->controldata.chkpnt_nxtxid = str2uint(p);
  			got_xid = true;
  		}
--- 197,214 
  			p++;/* remove ':' char */
  			cluster->controldata.chkpnt_nxtepoch = str2uint(p);
  
! 			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;
! 
  			if (p == NULL || strlen(p) <= 1)
  pg_fatal("%d: controldata retrieval problem\n", __LINE__);
  
! 			p++;/* remove '/' or ':' char */
  			cluster->controldata.chkpnt_nxtxid = str2uint(p);
  			got_xid = true;
  		}

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Robert Haas]

On Mon, Jan 18, 2016 at 7:42 PM, Michael Paquier
 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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 923fe58..abf9a70 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*** CREATE VIEW pg_timezone_abbrevs AS
*** 433,438 
--- 433,444 
  CREATE VIEW pg_timezone_names AS
  SELECT * FROM pg_timezone_names();
  
+ CREATE VIEW pg_config AS
+ SELECT * FROM pg_config();
+ 
+ REVOKE ALL on pg_config FROM PUBLIC;
+ REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC;
+ 
  -- Statistics views
  
  CREATE VIEW pg_stat_all_tables AS
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index 7889101..a0c82c1 100644
*** a/src/backend/utils/misc/Makefile
--- b/src/backend/utils/misc/Makefile
*** include $(top_builddir)/src/Makefile.glo
*** 14,21 
  
  override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
  
! OBJS = guc.o help_config.o pg_rusage.o ps_status.o rls.o \
!sampling.o superuser.o timeout.o tzparser.o
  
  # This location might depend on the installation directories. Therefore
  # we can't subsitute it into pg_config.h.
--- 14,21 
  
  override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
  
! OBJS = guc.o help_config.o pg_config.o pg_rusage.o \
!ps_status.o rls.o sampling.o superuser.o timeout.o tzparser.o
  
  # This location might depend on the installation directories. Therefore
  # we can't subsitute it into pg_config.h.
diff --git a/src/backend/utils/misc/pg_config.c b/src/backend/utils/misc/pg_config.c
index ...d86460e .
*** a/src/backend/utils/misc/pg_config.c
--- b/src/backend/utils/misc/pg_config.c
***
*** 0 
--- 1,102 
+ /*-
+  *
+  * pg_config.c
+  *		Expose same output as pg_config except as an SRF
+  *
+  * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
+  * Portions Copyright (c) 1994, Regents of the University of California
+  *
+  * IDENTIFICATION
+  *	  src/backend/utils/misc/pg_config.c
+  *
+  */
+ 
+ #include "postgres.h"
+ 
+ #include "funcapi.h"
+ #include "miscadmin.h"
+ #include "catalog/pg_type.h"
+ #include "common/config_info.h"
+ #include "utils/builtins.h"
+ #include "utils/elog.h"
+ #include "port.h"
+ 
+ Datum
+ pg_config(PG_FUNCTION_ARGS)
+ {
+ 	ReturnSetInfo	   *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+ 	Tuplestorestate	   *tupstore;
+ 	HeapTuple			tuple;
+ 	TupleDesc			tupdesc;
+ 	AttInMetadata	   *attinmeta;
+ 	MemoryContext		per_query_ctx;
+ 	MemoryContext		oldcontext;
+ 	configdata		   *ConfigData;
+ 	size_tconfigdata_len;
+ 	char			   *values[2];
+ 	int	i = 0;
+ 
+ 	/* check to see if caller supports us returning a tuplestore */
+ 	if (!rsinfo || !(rsinfo->allowedModes & SFRM_Materialize))
+ 		ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+  errmsg("materialize mode required, but it is not "
+ 		"allowed in this context")));
+ 
+ 	per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
+ 	

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Andres Freund]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 923fe58..abf9a70 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*** CREATE VIEW pg_timezone_abbrevs AS
*** 433,438 
--- 433,444 
  CREATE VIEW pg_timezone_names AS
  SELECT * FROM pg_timezone_names();
  
+ CREATE VIEW pg_config AS
+ SELECT * FROM pg_config();
+ 
+ REVOKE ALL on pg_config FROM PUBLIC;
+ REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC;
+ 
  -- Statistics views
  
  CREATE VIEW pg_stat_all_tables AS
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index 7889101..a0c82c1 100644
*** a/src/backend/utils/misc/Makefile
--- b/src/backend/utils/misc/Makefile
*** include $(top_builddir)/src/Makefile.glo
*** 14,21 
  
  override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
  
! OBJS = guc.o help_config.o pg_rusage.o ps_status.o rls.o \
!sampling.o superuser.o timeout.o tzparser.o
  
  # This location might depend on the installation directories. Therefore
  # we can't subsitute it into pg_config.h.
--- 14,21 
  
  override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
  
! OBJS = guc.o help_config.o pg_config.o pg_rusage.o \
!ps_status.o rls.o sampling.o superuser.o timeout.o tzparser.o
  
  # This location might depend on the installation directories. Therefore
  # we can't subsitute it into pg_config.h.
diff --git a/src/backend/utils/misc/pg_config.c b/src/backend/utils/misc/pg_config.c
index ...05ee67b .
*** a/src/backend/utils/misc/pg_config.c
--- b/src/backend/utils/misc/pg_config.c
***
*** 0 
--- 1,102 
+ /*-
+  *
+  * pg_config.c
+  *		Expose same output as pg_config except as an SRF
+  *
+  * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
+  * Portions Copyright (c) 1994, Regents of the University of California
+  *
+  * IDENTIFICATION
+  *	  src/backend/utils/misc/pg_config.c
+  *
+  */
+ 
+ #include "postgres.h"
+ 
+ #include "funcapi.h"
+ #include "miscadmin.h"
+ #include "catalog/pg_type.h"
+ #include "common/config_info.h"
+ #include "utils/builtins.h"
+ #include "utils/elog.h"
+ #include "port.h"
+ 
+ Datum
+ pg_config(PG_FUNCTION_ARGS)
+ {
+ 	ReturnSetInfo	   *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+ 	Tuplestorestate	   *tupstore;
+ 	HeapTuple			tuple;
+ 	TupleDesc			tupdesc;
+ 	AttInMetadata	   *attinmeta;
+ 	MemoryContext		per_query_ctx;
+ 	MemoryContext		oldcontext;
+ 	configdata		   *ConfigData;
+ 	size_tconfigdata_len;
+ 	char			   *values[2];
+ 	int	i = 0;
+ 
+ 	/* check to see if caller supports us returning a tuplestore */
+ 	if (!rsinfo || !(rsinfo->allowedModes & SFRM_Materialize))
+ 		ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+  errmsg("materialize mode required, but it is not "
+ 		"allowed in this context")));
+ 
+ 	per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
+ 	oldcontext = MemoryContextSwitchTo(per_query_ctx);
+ 
+ 	/* get the requested return tuple description */
+ 	tupdesc = CreateTupleDescCopy(rsinfo->expectedDesc);
+ 
+ 	/*
+ 	 * Check to make sure we have a reasonable tuple descriptor
+ 	 */
+ 	if (tupdesc->natts != 2 ||
+ 		tupdesc->attrs[0]->atttypid != TEXTOID ||
+ 		tupdesc->attrs[1]->atttypid != TEXTOID)
+ 		ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+  errmsg("query-specified return tuple and "
+ 		"function return type are not compatible")));
+ 
+ 	/* OK to use it */
+ 	attinmeta = TupleDescGetAttInMetadata(tupdesc);
+ 
+ 	/* let the caller know we're sending back a tuplestore */
+ 	rsinfo->returnMode = SFRM_Materialize;
+ 
+ 	/* initialize our tuplestore */
+ 	tupstore = tuplestore_begin_heap(true, false, work_mem);
+ 
+ 	ConfigData = get_configdata(my_exec_path, _len);
+ 	for (i = 0; i < configdata_len; i++)
+ 	{
+ 		values[0] = ConfigData[i].name;
+ 		values[1] = ConfigData[i].setting;
+ 
+ 		tuple = BuildTupleFromCStrings(attinmeta, values);
+ 		tuplestore_puttuple(tupstore, tuple);
+ 	}
+ 	
+ 	/*
+ 	 * no longer need the tuple descriptor reference created by
+ 	 * TupleDescGetAttInMetadata()
+ 	 */
+ 	ReleaseTupleDesc(tupdesc);
+ 
+ 	tuplestore_donestoring(tupstore);
+ 	rsinfo->setResult = tupstore;
+ 
+ 	/*
+ 	 * SFRM_Materialize mode expects us to return a NULL Datum. The actual
+ 	 * tuples are in our tuplestore and passed back through
+ 	 * rsinfo->setResult. rsinfo->setDesc is set to the tuple description
+ 	 * that we actually used to build our tuples with, so the caller can
+ 	 * verify we did what it was expecting.
+ 	 */
+ 	rsinfo->setDesc = tupdesc;
+ 	

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Tue, Jan 19, 2016 at 6:55 AM, Robert Haas  wrote:
> On Mon, Jan 18, 2016 at 4:43 AM, Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 923fe58..abf9a70 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*** CREATE VIEW pg_timezone_abbrevs AS
*** 433,438 
--- 433,444 
  CREATE VIEW pg_timezone_names AS
  SELECT * FROM pg_timezone_names();
  
+ CREATE VIEW pg_config AS
+ SELECT * FROM pg_config();
+ 
+ REVOKE ALL on pg_config FROM PUBLIC;
+ REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC;
+ 
  -- Statistics views
  
  CREATE VIEW pg_stat_all_tables AS
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index 7889101..a0c82c1 100644
*** a/src/backend/utils/misc/Makefile
--- b/src/backend/utils/misc/Makefile
*** include $(top_builddir)/src/Makefile.glo
*** 14,21 
  
  override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
  
! OBJS = guc.o help_config.o pg_rusage.o ps_status.o rls.o \
!sampling.o superuser.o timeout.o tzparser.o
  
  # This location might depend on the installation directories. Therefore
  # we can't subsitute it into pg_config.h.
--- 14,21 
  
  override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
  
! OBJS = guc.o help_config.o pg_config.o pg_rusage.o \
!ps_status.o rls.o sampling.o superuser.o timeout.o tzparser.o
  
  # This location might depend on the installation directories. Therefore
  # we can't subsitute it into pg_config.h.
diff --git a/src/backend/utils/misc/pg_config.c b/src/backend/utils/misc/pg_config.c
index ...05ee67b .
*** a/src/backend/utils/misc/pg_config.c
--- b/src/backend/utils/misc/pg_config.c
***
*** 0 
--- 1,102 
+ /*-
+  *
+  * pg_config.c
+  *		Expose same output as pg_config except as an SRF
+  *
+  * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
+  * Portions Copyright (c) 1994, Regents of the University of California
+  *
+  * IDENTIFICATION
+  *	  src/backend/utils/misc/pg_config.c
+  *
+  */
+ 
+ #include "postgres.h"
+ 
+ #include "funcapi.h"
+ #include "miscadmin.h"
+ #include "catalog/pg_type.h"
+ #include "common/config_info.h"
+ #include "utils/builtins.h"
+ #include "utils/elog.h"
+ #include "port.h"
+ 
+ Datum
+ pg_config(PG_FUNCTION_ARGS)
+ {
+ 	ReturnSetInfo	   *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+ 	Tuplestorestate	   *tupstore;
+ 	HeapTuple			tuple;
+ 	TupleDesc			tupdesc;
+ 	AttInMetadata	   *attinmeta;
+ 	MemoryContext		per_query_ctx;
+ 	MemoryContext		oldcontext;
+ 	configdata		   *ConfigData;
+ 	size_tconfigdata_len;
+ 	char			   *values[2];
+ 	int	i = 0;
+ 
+ 	/* check to see if caller supports us returning a tuplestore */
+ 	if (!rsinfo || !(rsinfo->allowedModes & SFRM_Materialize))
+ 		ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+  errmsg("materialize mode required, but it is not "
+ 		"allowed in this context")));
+ 
+ 	per_query_ctx = rsinfo->econtext->ecxt_per_query_memory;
+ 	oldcontext = MemoryContextSwitchTo(per_query_ctx);
+ 
+ 	/* get the requested return tuple description */
+ 	tupdesc = CreateTupleDescCopy(rsinfo->expectedDesc);
+ 
+ 	/*
+ 	 * Check to make sure we have a reasonable tuple descriptor
+ 	 */
+ 	if (tupdesc->natts != 2 ||
+ 		tupdesc->attrs[0]->atttypid != 

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Bruce Momjian]

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  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  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +
diff --git a/src/bin/pg_upgrade/controldata.c b/src/bin/pg_upgrade/controldata.c
new file mode 100644
index 1f7b65e..aaaea7b
*** a/src/bin/pg_upgrade/controldata.c
--- b/src/bin/pg_upgrade/controldata.c
*** get_control_data(ClusterInfo *cluster, b
*** 198,203 
--- 198,206 
  			cluster->controldata.chkpnt_nxtepoch = str2uint(p);
  
  			p = strchr(p, '/');
+ 			/* delimiter changed from '/' to ':' in 9.6 */
+ 			if (p == NULL && GET_MAJOR_VERSION(cluster->major_version) >= 906)
+ p = strchr(p, ':');
  			if (p == NULL || strlen(p) <= 1)
  pg_fatal("%d: controldata retrieval problem\n", __LINE__);
  

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Tue, Jan 19, 2016 at 1:49 PM, Michael Paquier
 wrote:
> On Tue, Jan 19, 2016 at 11:08 AM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Tue, Jan 19, 2016 at 11:08 AM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Jan 18, 2016 at 4:43 AM, Andres Freund  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


signature.asc
Description: Digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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  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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Robert Haas]

On Mon, Jan 18, 2016 at 4:43 AM, Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Andres Freund]

On January 18, 2016 11:10:35 PM GMT+01:00, Stephen Frost  
wrote:
>* Robert Haas (robertmh...@gmail.com) wrote:
>> On Mon, Jan 18, 2016 at 4:43 AM, Andres Freund 
>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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Bruce Momjian]

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  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  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+ Roman grave inscription +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 01/16/2016 06:02 AM, Michael Paquier wrote:
> On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway  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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 01/16/2016 06:07 AM, Michael Paquier wrote:
> On Sun, Dec 27, 2015 at 5:39 AM, Joe Conway  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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Sun, Jan 17, 2016 at 8:48 AM, Andres Freund  wrote:
> On January 17, 2016 12:46:36 AM GMT+01:00, Michael Paquier 
>  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 01/16/2016 06:02 AM, Michael Paquier wrote:
> On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway  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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Sun, Dec 27, 2015 at 5:39 AM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Sat, Jan 16, 2016 at 11:07 PM, Michael Paquier
 wrote:
> On Sun, Dec 27, 2015 at 5:39 AM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Wed, Dec 30, 2015 at 9:08 AM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Andres Freund]

On January 17, 2016 12:46:36 AM GMT+01:00, Michael Paquier 
 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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Robert Haas]

On Jan 16, 2016, at 9:08 AM, Michael Paquier  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 fewer of them we have, the 
better.

...Robert

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 12/23/2015 04:37 PM, Michael Paquier wrote:
> On Thu, Dec 24, 2015 at 2:08 AM, Joe Conway  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| 00010001
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

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

On 12/23/2015 04:37 PM, Michael Paquier wrote:
> On Thu, Dec 24, 2015 at 2:08 AM, Joe Conway  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
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 536c805..a50ffe8 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*** CREATE VIEW pg_timezone_abbrevs AS
*** 433,438 
--- 433,444 
  CREATE VIEW pg_timezone_names AS
  SELECT * FROM pg_timezone_names();
  
+ CREATE VIEW pg_config AS
+ SELECT * FROM pg_config();
+ 
+ REVOKE ALL on pg_config FROM PUBLIC;
+ REVOKE EXECUTE ON FUNCTION pg_config() FROM PUBLIC;
+ 
  -- Statistics views
  
  CREATE VIEW pg_stat_all_tables AS
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index 7889101..e0015d8 100644
*** a/src/backend/utils/misc/Makefile
--- b/src/backend/utils/misc/Makefile
*** include $(top_builddir)/src/Makefile.glo
*** 14,21 
  
  override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
  
! OBJS = guc.o help_config.o pg_rusage.o ps_status.o rls.o \
!sampling.o superuser.o timeout.o tzparser.o
  
  # This location might depend on the installation directories. Therefore
  # we can't subsitute it into pg_config.h.
--- 14,34 
  
  override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
  
! # 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)\""
! 
! OBJS = guc.o help_config.o pg_config.o pg_rusage.o \
!ps_status.o rls.o sampling.o superuser.o timeout.o tzparser.o
  
  # This location might depend on the installation directories. Therefore
  # we can't subsitute it into pg_config.h.
diff --git a/src/backend/utils/misc/pg_config.c b/src/backend/utils/misc/pg_config.c
index ...e29a706 .
*** a/src/backend/utils/misc/pg_config.c
--- b/src/backend/utils/misc/pg_config.c
***
*** 0 
--- 1,266 
+ /*-
+  *
+  * pg_config.c
+  *		Expose same output as pg_config except as an SRF
+  *
+  * Portions Copyright (c) 1996-2015, PostgreSQL Global Development Group
+  * Portions Copyright (c) 1994, Regents of the University of California
+  *
+  * IDENTIFICATION
+  *	  src/backend/utils/misc/pg_config.c
+  *
+  */
+ 
+ #include "postgres.h"
+ 
+ #include "funcapi.h"
+ #include "miscadmin.h"
+ #include "catalog/pg_type.h"
+ #include "utils/builtins.h"
+ #include "utils/elog.h"
+ #include "port.h"
+ 
+ static void get_configdata(void);
+ 
+ #ifdef PGDLLIMPORT
+ /* Postgres global */
+ extern PGDLLIMPORT char my_exec_path[];
+ #else
+ /* Postgres global */
+ extern DLLIMPORT char my_exec_path[];
+ #endif /* PGDLLIMPORT */
+ 
+ struct configdata
+ {
+ 	char	   *name;
+ 	char	   *setting;
+ };
+ 
+ static struct configdata ConfigData[] =
+ {
+ 	{"BINDIR", NULL},
+ 	{"DOCDIR", NULL},
+ 	{"HTMLDIR", NULL},
+ 	{"INCLUDEDIR", NULL},
+ 	{"PKGINCLUDEDIR", NULL},
+ 	{"INCLUDEDIR-SERVER", NULL},
+ 	{"LIBDIR", NULL},
+ 	{"PKGLIBDIR", NULL},
+ 	{"LOCALEDIR", NULL},
+ 	{"MANDIR", NULL},
+ 	{"SHAREDIR", NULL},
+ 	{"SYSCONFDIR", NULL},
+ 	{"PGXS", NULL},
+ 	{"CONFIGURE", NULL},
+ 	{"CC", NULL},
+ 	{"CPPFLAGS", NULL},
+ 	{"CFLAGS", NULL},
+ 	{"CFLAGS_SL", NULL},
+ 	{"LDFLAGS", NULL},
+ 	{"LDFLAGS_EX", NULL},
+ 	{"LDFLAGS_SL", 

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Wed, Dec 9, 2015 at 9:18 PM, Michael Paquier
 wrote:
> On Thu, Sep 17, 2015 at 7:12 AM, Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Thu, Dec 24, 2015 at 2:08 AM, Joe Conway  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Michael Paquier]

On Thu, Sep 17, 2015 at 7:12 AM, Andres Freund  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Erik Rijkers]

[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







--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Andres Freund]

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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Andrew Dunstan]

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Robert Haas]

On Sun, Sep 6, 2015 at 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 (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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Josh Berkus]

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:0001001A
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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Andrew Dunstan]

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


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

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



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Robert Haas]

On Wed, Sep 2, 2015 at 5:31 PM, Joe Conway  wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> On 09/02/2015 05:25 PM, Tom Lane wrote:
>> Robert Haas  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Robert Haas]

On Mon, Aug 31, 2015 at 9:47 PM, Peter Eisentraut  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 09/02/2015 05:25 PM, Tom Lane wrote:
> Robert Haas  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-


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Alvaro Herrera]

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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Tom Lane]

Robert Haas  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Alvaro Herrera]

Tom Lane wrote:
> Robert Haas  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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Josh Berkus]

On 09/02/2015 02:34 PM, Alvaro Herrera wrote:
> Tom Lane wrote:
>> Robert Haas  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

On 8/24/15 9:50 AM, Tom Lane wrote:
> Andrew Dunstan  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?



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Peter Eisentraut]

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.



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 08/26/2015 06:33 AM, Stephen Frost wrote:
 * Joe Conway (m...@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-


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Stephen Frost]

* Joe Conway (m...@joeconway.com) wrote:
 On 08/26/2015 06:33 AM, Stephen Frost wrote:
  * Joe Conway (m...@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


signature.asc
Description: Digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Stephen Frost]

* Joe Conway (m...@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


signature.asc
Description: Digital signature

Re: [HACKERS] exposing pg_controldata and pg_config as functions

[Joe Conway]

-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 and...@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-
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index ccc030f..7b5db32 100644
*** a/src/backend/catalog/system_views.sql
--- b/src/backend/catalog/system_views.sql
*** CREATE VIEW pg_timezone_abbrevs AS
*** 425,430 
--- 425,436 
  CREATE VIEW pg_timezone_names AS
  SELECT * FROM pg_timezone_names();
  
+ CREATE VIEW pg_config AS
+ SELECT * FROM pg_config();
+ 
+ CREATE VIEW pg_controldata AS
+ SELECT * FROM pg_controldata();
+ 
  -- Statistics views
  
  CREATE VIEW pg_stat_all_tables AS
diff --git a/src/backend/utils/misc/Makefile b/src/backend/utils/misc/Makefile
index 7889101..13b60f7 100644
*** a/src/backend/utils/misc/Makefile
--- b/src/backend/utils/misc/Makefile
*** include $(top_builddir)/src/Makefile.glo
*** 14,21 
  
  override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
  
! OBJS = guc.o help_config.o pg_rusage.o ps_status.o rls.o \
!sampling.o superuser.o timeout.o tzparser.o
  
  # This location might depend on the installation directories. Therefore
  # we can't subsitute it into pg_config.h.
--- 14,34 
  
  override CPPFLAGS := -I. -I$(srcdir) $(CPPFLAGS)
  
! # 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 +=