pg_hba_file_settings view patch

On 15 August 2016 at 12:17, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:

comments?

This looks like a good feature contribution, thanks.

At present the patch doesn't apply cleanly, please rebase.

The patch doesn't contain any tests, which means I can't see what the
output looks like, so I can't judge the exact usefulness of this
particular patch. ISTM likely that there will be some detailed points
to review in there somewhere.

Do we want line number, or priority order? i.e. do we want to count
comment lines or just main rule lines? I prefer latter.
Various other questions along those lines needed, once I can see the output.

What is push_jsonb_string_value() etc..?
If there is required infrastructure there is no explanation of why.
Suggest you explain and/or split into two.

pg_hba_file_settings seems a clumsy name. I'd prefer pg_hba_settings,
since that name could live longer than the concept of pg_hba.conf,
which seems likely to become part of ALTER SYSTEM in future, so we
wouldn't really want the word "file" in there.

I've not seen anything yet to make me think a commit for this wouldn't
happen once we've worked the detail.

Thanks

--
Simon Riggs http://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: Simon Riggs 2016-09-03 <CANP8+jLFDeCJ7YWuzSodKXD85oNy2Kxa40U5GRnry6ms9Mz+5A@mail.gmail.com>

pg_hba_file_settings seems a clumsy name. I'd prefer pg_hba_settings,
since that name could live longer than the concept of pg_hba.conf,
which seems likely to become part of ALTER SYSTEM in future, so we
wouldn't really want the word "file" in there.

IMHO "settings" is wrong here. "pg_file_settings" means "settings in
config file (that might not been applied yet)". The contents of
pg_hba.conf are not config settings, but there doesn't appear to be a
standard name for them - 19.1 calls them "records".

Given that the patch seems to report what's on disk, "pg_hba_file"
seems a good name to me. Even if ALTER SYSTEM should become able to
update the file, it'd still be a file. (If it were the actual running
config, I'd go for "pg_hba_rules".)

Christoph

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

On Sun, Sep 4, 2016 at 1:44 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 15 August 2016 at 12:17, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

comments?

This looks like a good feature contribution, thanks.

At present the patch doesn't apply cleanly, please rebase.

Rebased patch is attached.

The patch doesn't contain any tests, which means I can't see what the
output looks like, so I can't judge the exact usefulness of this
particular patch. ISTM likely that there will be some detailed points
to review in there somewhere.

Added a test in the regress and also in the docs.

Do we want line number, or priority order? i.e. do we want to count

comment lines or just main rule lines? I prefer latter.
Various other questions along those lines needed, once I can see the
output.

I preferred the line number that includes the comment lines also. This way
it
will be easy to edit the file if it contains any errors by directly going
to that line
number.

What is push_jsonb_string_value() etc..?
If there is required infrastructure there is no explanation of why.
Suggest you explain and/or split into two.

I added a JSONB type column to display the hba.conf options values.
To store the options data into JSONB format, currently i didn't find any
functions that are available to use in the core. So I added key/value
functions to add the data into JSONB object.

The functions related code is split into a different patch and attached.

pg_hba_file_settings seems a clumsy name. I'd prefer pg_hba_settings,
since that name could live longer than the concept of pg_hba.conf,
which seems likely to become part of ALTER SYSTEM in future, so we
wouldn't really want the word "file" in there.

Yes, I also agree that *file* is not required. The hba rules are not
available
in memory also in the backends. I changed the view name to pg_hba_rules
as per the other mail from Christoph.

Regards,
Hari Babu
Fujitsu Australia

On Mon, Sep 5, 2016 at 4:09 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:

On Sun, Sep 4, 2016 at 1:44 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

On 15 August 2016 at 12:17, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

comments?

This looks like a good feature contribution, thanks.

At present the patch doesn't apply cleanly, please rebase.

Rebased patch is attached.

Moved to next CF as there is a patch and no reviews.

+       push_jsonb_string_key(&parseState, "map");
+       push_jsonb_string_value(&parseState, hba->usermap);
[...]
+    <row>
+     <entry><structfield>options</structfield></entry>
+     <entry><type>jsonb</type></entry>
+     <entry>Configuration options set for authentication method</entry>
+    </row>
Why is it an advantage to use jsonb here instead of a simple array
made of name=value? If they were nested I'd see a case for it but it
seems to me that as presented this is just an overkill. In short, I
think that this patch needs a bit of rework, so I am marking it as
returned with feedback.
-- 
Michael

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

On 10/2/16, Michael Paquier <michael.paquier@gmail.com> wrote:

+       push_jsonb_string_key(&parseState, "map");
+       push_jsonb_string_value(&parseState, hba->usermap);
[...]
+    <row>
+     <entry><structfield>options</structfield></entry>
+     <entry><type>jsonb</type></entry>
+     <entry>Configuration options set for authentication method</entry>
+    </row>
Why is it an advantage to use jsonb here instead of a simple array
made of name=value? If they were nested I'd see a case for it but it
seems to me that as presented this is just an overkill.

I guess for ability to use filtering like:

SELECT * FROM pg_hba_rules WHERE options->>radiusserver LIKE '%.example.com';

I think it would be harder if options is an array of strings...

--
Best regards,
Vitaly Burovoy

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

On Mon, Oct 3, 2016 at 3:25 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:

I guess for ability to use filtering like:

SELECT * FROM pg_hba_rules WHERE options->>radiusserver LIKE '%.example.com';

I think it would be harder if options is an array of strings...

With unnest() and a matching pattern, not that hard but..
--
Michael

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

On 10/2/16, Michael Paquier <michael.paquier@gmail.com> wrote:

On Mon, Oct 3, 2016 at 3:25 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
wrote:

I guess for ability to use filtering like:

SELECT * FROM pg_hba_rules WHERE options->>radiusserver LIKE
'%.example.com';

I think it would be harder if options is an array of strings...

With unnest() and a matching pattern, not that hard but..

I'm not a user of that feature, and I don't know how pg_hba files look
like in really big companies...

But for me filtering is more complicated than just a single comparison.
What about more complex filtering --- several radiusserver and a user(s):

WHERE
options->>radiusserver = ANY(ARRAY['a.example.com', 'g.example.com'])
AND
options->>radiusidentifier = ANY(ARRAY['ID_a', 'ID_b', 'ID_c',
'ID_d', 'ID_e']) -- or even a subquery

Again, I don't know whether it will be widely used, but in case of
multiple param_name->param_value settings (column "options") I'd like
to see native key-value store rather than array of strings (according
to POLA).

I guess you're expecting "key=value" format as they are written in the
pg_hba file (and described in the doc), but sometimes they can be
parsed and output differs from exact pg_hba records (for instance look
at "ldapurl" parameter).

--
Best regards,
Vitaly Burovoy

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

On Mon, Oct 3, 2016 at 3:51 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Mon, Sep 5, 2016 at 4:09 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

On Sun, Sep 4, 2016 at 1:44 AM, Simon Riggs <simon@2ndquadrant.com>

wrote:

On 15 August 2016 at 12:17, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

comments?

This looks like a good feature contribution, thanks.

At present the patch doesn't apply cleanly, please rebase.

Rebased patch is attached.

Moved to next CF as there is a patch and no reviews.

+       push_jsonb_string_key(&parseState, "map");
+       push_jsonb_string_value(&parseState, hba->usermap);
[...]
+    <row>
+     <entry><structfield>options</structfield></entry>
+     <entry><type>jsonb</type></entry>
+     <entry>Configuration options set for authentication method</entry>
+    </row>
Why is it an advantage to use jsonb here instead of a simple array
made of name=value? If they were nested I'd see a case for it but it
seems to me that as presented this is just an overkill. In short, I
think that this patch needs a bit of rework, so I am marking it as
returned with feedback.

Yes, I agree that adding these JSONB utility functions for this view
is an overkill, but I thought that these are may be useful for some
users if it is a JSONB type instead of array.

If anyone else feel the same opinion, I can update the patch with
array datatype.

Regards,
Hari Babu
Fujitsu Australia

Haribabu Kommi wrote:

On Mon, Oct 3, 2016 at 3:51 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

Yes, I agree that adding these JSONB utility functions for this view
is an overkill, but I thought that these are may be useful for some
users if it is a JSONB type instead of array.

Peter Eisentraut said he'd like JSON better:
/messages/by-id/5547DB0A.2020904@gmx.net
I asked twice about the use of JSON, suggesting an array instead:
/messages/by-id/20151204163147.GZ2763@alvherre.pgsql
/messages/by-id/20160201215714.GA98800@alvherre.pgsql

I now think that we should figure out what it is that we want before we
continue to request it to be changed over and over.

--
�lvaro Herrera https://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

On Tue, Oct 25, 2016 at 3:23 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Haribabu Kommi wrote:

On Mon, Oct 3, 2016 at 3:51 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

Yes, I agree that adding these JSONB utility functions for this view
is an overkill, but I thought that these are may be useful for some
users if it is a JSONB type instead of array.

Peter Eisentraut said he'd like JSON better:
/messages/by-id/5547DB0A.2020904@gmx.net
I asked twice about the use of JSON, suggesting an array instead:
/messages/by-id/20151204163147.GZ2763@alvherre.pgsql
/messages/by-id/20160201215714.GA98800@alvherre.pgsql

I now think that we should figure out what it is that we want before we
continue to request it to be changed over and over.

That sounds like a good idea. :-)

FWIW, I'm -1 on using JSON here. I don't believe that we should start
using JSON all over the place just because we can. If we do that,
we'll end up with a mishmash of styles, and maybe look silly when JSON
is replaced by the new and much better SDGJHSDR format.

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

Robert Haas <robertmhaas@gmail.com> writes:

FWIW, I'm -1 on using JSON here. I don't believe that we should start
using JSON all over the place just because we can. If we do that,
we'll end up with a mishmash of styles, and maybe look silly when JSON
is replaced by the new and much better SDGJHSDR format.

I concur. JSON isn't a core datatype and I don't want to see it treated
as one. We should redesign this view so that it doesn't rely on anything
more advanced than arrays.

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

On 10/26/2016 12:24 PM, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

FWIW, I'm -1 on using JSON here. I don't believe that we should start
using JSON all over the place just because we can. If we do that,
we'll end up with a mishmash of styles, and maybe look silly when JSON
is replaced by the new and much better SDGJHSDR format.

I concur. JSON isn't a core datatype and I don't want to see it treated
as one. We should redesign this view so that it doesn't rely on anything
more advanced than arrays.

Huh? Sure it is. Ships in PostgreSQL-core.

I mean, I'm not particularly in favor of using JSON for this (arrays
seem OK), but that seems like an invalid reason not to.

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

On 10/26/2016 12:54 PM, Josh Berkus wrote:

On 10/26/2016 12:24 PM, Tom Lane wrote:

Robert Haas <robertmhaas@gmail.com> writes:

FWIW, I'm -1 on using JSON here. I don't believe that we should start
using JSON all over the place just because we can. If we do that,
we'll end up with a mishmash of styles, and maybe look silly when JSON
is replaced by the new and much better SDGJHSDR format.

I concur. JSON isn't a core datatype and I don't want to see it treated
as one. We should redesign this view so that it doesn't rely on anything
more advanced than arrays.

Huh? Sure it is. Ships in PostgreSQL-core.

I mean, I'm not particularly in favor of using JSON for this (arrays
seem OK), but that seems like an invalid reason not to.

-1 to JSON for this.

JD

--
Command Prompt, Inc. http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, 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

Josh Berkus <josh@agliodbs.com> writes:

On 10/26/2016 12:24 PM, Tom Lane wrote:

I concur. JSON isn't a core datatype and I don't want to see it treated
as one. We should redesign this view so that it doesn't rely on anything
more advanced than arrays.

Huh? Sure it is. Ships in PostgreSQL-core.

To my way of thinking it's a nonstandard extension. The fact that we
chose to package it in core and not as an extension doesn't alter the
fact that it's peripheral to the system and nothing else depends on it.
I'd like to keep things that way. I wouldn't want any core-system
functionality to start depending on the geometric types, either.

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

On Thu, Oct 27, 2016 at 5:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Josh Berkus <josh@agliodbs.com> writes:

On 10/26/2016 12:24 PM, Tom Lane wrote:

I concur. JSON isn't a core datatype and I don't want to see it treated
as one. We should redesign this view so that it doesn't rely on anything
more advanced than arrays.

Huh? Sure it is. Ships in PostgreSQL-core.

To my way of thinking it's a nonstandard extension. The fact that we
chose to package it in core and not as an extension doesn't alter the
fact that it's peripheral to the system and nothing else depends on it.
I'd like to keep things that way. I wouldn't want any core-system
functionality to start depending on the geometric types, either.

I got a similar opinion regarding this patch to be honest after
looking at it, seeing actually with a bad eye the use of fancy data
types that are not well-spread among the other catalog views and
functions. So -1 for JSON and +1 for arrays.
--
Michael

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

On Wed, Oct 26, 2016 at 11:04 PM, Joshua D. Drake <jd@commandprompt.com> wrote:

On 10/26/2016 12:54 PM, Josh Berkus wrote:

I mean, I'm not particularly in favor of using JSON for this (arrays
seem OK), but that seems like an invalid reason not to.

-1 to JSON for this.

Sigh. Well I tried to review this patch in a previous iteration so let
me give some context.

The fundamental problem is that the pga_hba.conf file has some bits of
complex structure that aren't easily captured by linear arrays. The
problem I struggled with most was the keywords like "all", "samerole",
and "replication". A simple array of text makes it awkward to
distinguish those keywords from the quoted text values with the same
content. And then there are the ldap options which naturally would be
a data type like json or htab.

Some people wanted to store strings like '"all"' with the quotes which
I thought was ugly and functionally less useful because it would be
hard to query and impossible to join against things like pg_users.
Others wanted to give up the idea of expanding the entries at all and
just have a single string for the whole line which I thought was
pointless -- you may as well just read the file then.

Personally my recommendation was to ignore the problem. Just have
arrays of text and document that if you have a real user by the name
"all" or "samerole" then the view cannot be interpreted accurately.
Tools like pgadmin which want to use the view could check for such
users and display a warning or error rather than inaccurate
information.

If there's any support for my recommendation I'm still happy to pick
up the patch again and commit it.

--
greg

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

Greg Stark wrote:

The fundamental problem is that the pga_hba.conf file has some bits of
complex structure that aren't easily captured by linear arrays. The
problem I struggled with most was the keywords like "all", "samerole",
and "replication". A simple array of text makes it awkward to
distinguish those keywords from the quoted text values with the same
content. And then there are the ldap options which naturally would be
a data type like json or htab.

Hmm I thought we had decided that such keywords would live in separate
arrays, i.e. you have one array for plain names and another array for
keyword stuff. Then it's not ambiguous anymore.

--
�lvaro Herrera https://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

On Fri, Oct 28, 2016 at 4:17 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Greg Stark wrote:

The fundamental problem is that the pga_hba.conf file has some bits of
complex structure that aren't easily captured by linear arrays. The
problem I struggled with most was the keywords like "all", "samerole",
and "replication". A simple array of text makes it awkward to
distinguish those keywords from the quoted text values with the same
content. And then there are the ldap options which naturally would be
a data type like json or htab.

Hmm I thought we had decided that such keywords would live in separate
arrays, i.e. you have one array for plain names and another array for
keyword stuff. Then it's not ambiguous anymore.

Thanks for all your opinions. Here I attached updated patch with the change
in column datatype from JSONB to TEXT array. Rest of the code changes
are same to the earlier patch.

Regards,
Hari Babu
Fujitsu Australia

On Fri, Oct 28, 2016 at 4:55 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
wrote:

On Fri, Oct 28, 2016 at 4:17 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
wrote:

Greg Stark wrote:

The fundamental problem is that the pga_hba.conf file has some bits of
complex structure that aren't easily captured by linear arrays. The
problem I struggled with most was the keywords like "all", "samerole",
and "replication". A simple array of text makes it awkward to
distinguish those keywords from the quoted text values with the same
content. And then there are the ldap options which naturally would be
a data type like json or htab.

Hmm I thought we had decided that such keywords would live in separate
arrays, i.e. you have one array for plain names and another array for
keyword stuff. Then it's not ambiguous anymore.

Thanks for all your opinions. Here I attached updated patch with the change
in column datatype from JSONB to TEXT array. Rest of the code changes
are same to the earlier patch.

The added regression test fails for the cases where the server is loaded
with
different pg_hba.conf rules during installcheck verification. Updated patch
is
attached with removing those tests.

Regards,
Hari Babu
Fujitsu Australia