Re: Providing catalog view to pg_hba.conf file - Patch submission

Hi,

In connection to my previous proposal about "providing catalog view to
pg_hba.conf file contents" , I have developed the attached patch .

[Current situation]

Currently, to view the pg_hba.conf file contents, DB admin has to access
the file from database server to read the settings. In case of huge and
multiple hba files, finding the appropriate hba rules which are loaded
will be difficult and take some time.

[What this Patch does]

Functionality of the attached patch is that it will provide a new view
"pg_hba_settings" to admin users. Public access to the view is
restricted. This view will display basic information about HBA setting
details of postgresql cluster. Information to be shown , is taken from
parsed hba lines and not directly read from pg_hba.conf files.
Documentation files are also updated to include details of this new view
under "Chapter 47.System Catalogs". Also , a new note is added in
"chapter 19.1 The pg_hba.conf File"

[Advantage]

Advantage of having this "pg_hba_settings" view is that the admin can
check, what hba rules are loaded in runtime via database connection
itself. And, thereby it will be easy and useful for admin to check all
the users with their privileges in a single view to manage them.

Thanks & Regards,

Vaishnavi

Fujitsu Australia

On Fri, Mar 14, 2014 at 6:30 AM, Prabakaran, Vaishnavi <
vaishnavip(at)fast(dot)au(dot)fujitsu(dot)com> wrote:

> Hi,
>
>
>
> In connection to my previous proposal about "providing catalog view to
> pg_hba.conf file contents" , I have developed the attached patch .
>
>
>
> [Current situation]
>
> Currently, to view the pg_hba.conf file contents, DB admin has to access
> the file from database server to read the settings. In case of huge and
> multiple hba files, finding the appropriate hba rules which are loaded will
> be difficult and take some time.
>
>
>
> [What this Patch does]
>
> Functionality of the attached patch is that it will provide a new view
> "pg_hba_settings" to admin users. Public access to the view is restricted.
> This view will display basic information about HBA setting details of
> postgresql cluster. Information to be shown , is taken from parsed hba
> lines and not directly read from pg_hba.conf files. Documentation files are
> also updated to include details of this new view under "Chapter 47.System
> Catalogs". Also , a new note is added in "chapter 19.1 The pg_hba.conf File"
>
>
>
> [Advantage]
>
> Advantage of having this "pg_hba_settings" view is that the admin can
> check, what hba rules are loaded in runtime via database connection itself.
> And, thereby it will be easy and useful for admin to check all the users
> with their privileges in a single view to manage them.
>
>
>
This looks like a useful feature, so make sure you register it on
https://commitfest.postgresql.org/action/commitfest_view?id=22.

I haven't looked at the actual code yet, btu I did notice one thing at a
very quick lookover at the docs - it seems to be completely ignoring the
key/value parameters given on a row, and stops reporting after the auth
method? That seems bad. And also, probably host/mask should be using the
inet style datatypes and not text?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

From: Magnus Hagander [mailto:magnus(at)hagander(dot)net]
Sent: Friday, 14 March 2014 9:33 PM
To: Prabakaran, Vaishnavi
Cc: PostgreSQL-development
Subject: Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission

On Fri, Mar 14, 2014 at 6:30 AM, Prabakaran, Vaishnavi <vaishnavip(at)fast(dot)au(dot)fujitsu(dot)com<mailto:vaishnavip(at)fast(dot)au(dot)fujitsu(dot)com>> wrote:
Hi,

In connection to my previous proposal about "providing catalog view to pg_hba.conf file contents" , I have developed the attached patch .

[Current situation]
Currently, to view the pg_hba.conf file contents, DB admin has to access the file from database server to read the settings. In case of huge and multiple hba files, finding the appropriate hba rules which are loaded will be difficult and take some time.

[What this Patch does]
Functionality of the attached patch is that it will provide a new view "pg_hba_settings" to admin users. Public access to the view is restricted. This view will display basic information about HBA setting details of postgresql cluster. Information to be shown , is taken from parsed hba lines and not directly read from pg_hba.conf files. Documentation files are also updated to include details of this new view under "Chapter 47.System Catalogs". Also , a new note is added in "chapter 19.1 The pg_hba.conf File"

[Advantage]
Advantage of having this "pg_hba_settings" view is that the admin can check, what hba rules are loaded in runtime via database connection itself. And, thereby it will be easy and useful for admin to check all the users with their privileges in a single view to manage them.

>This looks like a useful feature, so make sure you register it on https://commitfest.postgresql.org/action/commitfest_view?id=22.

>I haven't looked at the actual code yet, btu I did notice one thing at a very quick lookover at the docs - it seems to be completely ignoring the key/value parameters given on a row, and >stops reporting after the auth method? That seems bad. And also, probably host/mask should be using the inet style datatypes and not text?

Agree, am now working on including a new column "configuration_option" to display the key/value parameter set. I will send the updated patch once after adding new column.
Host/mask values are stored as sockaddr_storage structure in parsed_hba_lines, so I have used text datatype to display the hostname.

Thanks & Regards,
Vaishnavi
Fujitsu Australia
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Friday, Mar 14, 2014 at 9:33 PM, Maganus Hagander <magnus(at)hagander(dot)net<mailto:magnus(at)hagander(dot)net> > wrote:

>>Hi,
>>In connection to my previous proposal about "providing catalog view to pg_hba.conf file contents" , I have developed the attached patch .
>> [Current situation]
>>Currently, to view the pg_hba.conf file contents, DB admin has to access the file from database server to read the settings. In case of huge and multiple hba files, finding the appropriate hba rules which are loaded will be difficult and take some time.

>> [What this Patch does]
>>Functionality of the attached patch is that it will provide a new view "pg_hba_settings" to admin users. Public access to the view is restricted. This view will display basic information about HBA setting details of postgresql cluster. Information to be >>shown , is taken from parsed hba lines and not directly read from pg_hba.conf files. Documentation files are also updated to include details of this new view under "Chapter 47.System Catalogs". Also , a new note is added in "chapter 19.1 The >>pg_hba.conf File"
>> [Advantage]
>>Advantage of having this "pg_hba_settings" view is that the admin can check, what hba rules are loaded in runtime via database connection itself. And, thereby it will be easy and useful for admin to check all the users with their privileges in a single >>view to manage them.
>This looks like a useful feature, so make sure you register it on https://commitfest.postgresql.org/action/commitfest_view?id=22.
Sure, I will add it to commitfest.
>I haven't looked at the actual code yet, btu I did notice one thing at a very quick lookover at the docs - it seems to be completely ignoring the key/value parameters given on a row, and >stops reporting after the auth method? That seems bad. And also, >probably host/mask should be using the inet style datatypes and not text?

Added new column "configuration_option" to pg_hba_settings view to display the key/value parameter set. Attached the updated patch.

Thanks & Regards,
Vaishnavi
Fujitsu Australia

On Fri, Mar 14, 2014 at 12:30 AM, Prabakaran, Vaishnavi
<vaishnavip(at)fast(dot)au(dot)fujitsu(dot)com> wrote:
> Hi,
>
> In connection to my previous proposal about "providing catalog view to
> pg_hba.conf file contents" , I have developed the attached patch .
>
[...]
>
> [What this Patch does]
>
> Functionality of the attached patch is that it will provide a new view
> "pg_hba_settings" to admin users. Public access to the view is restricted.
> This view will display basic information about HBA setting details of
> postgresql cluster. Information to be shown , is taken from parsed hba
> lines and not directly read from pg_hba.conf files. Documentation files are
> also updated to include details of this new view under "Chapter 47.System
> Catalogs". Also , a new note is added in "chapter 19.1 The pg_hba.conf File"
>

A normal user can see all the info the view provide once you GRANT
permissions on it. How much info should a non-superuser see from this
view? currently a non-superuser can't see pg_hba info, now it can.

This function should be superuser only or only show info related for
current_user if it user is not superuser.

Also, i think you should use lowercase values just they are in
pg_hba.conf (ie: local not Local, host not Host, etc)

--
Jaime Casanova www.2ndQuadrant.com
Professional PostgreSQL: Soporte 24x7 y capacitación
Phone: +593 4 5107566 Cell: +593 987171157

Hi Vaishnavi.

In addition to Jaime's comments about the functionality, here are are
some comments about the code.

Well, they were supposed to be comments about the code, but it turns out
I have comments about the feature as well. We need to figure out what to
do about the database and user columns. Returning an array containing
e.g. "{all}" won't fly. We must distinguish between "all" and "{all}".

I don't have a good solution, other than returning two columns each: one
a string, and one an array, only one of them set for any given record.

> + int
> + GetNumHbaLines(void)
> + {

Please add a blank line before this.

> + /*
> + * Fetches the HbaLine corresponding to linenum variable.
> + * Fill the values required to build a tuple, of view pg_hba_settings, in values pointer.
> + */
> + void
> + GetHbaLineByNum(int linenum, const char **values)
> + {

I suggest naming this function GetHbaValuesByLinenum() and changing the
comment to "Fill in suitable values to build a tuple representing the
HbaLine corresponding to the given linenum".

Actually, maybe it should be hba_getvaluesbyline() for consistency with
the other functions in the file? In that case, the preceding function
should also be renamed to hba_getnumlines().

> + /* Retrieving connection type */
> + switch (hba->conntype)
> + {

The comment should be just "Connection type". Generally speaking, all of
the "Retrieving" comments should be changed similarly. Or just removed.

> + case ctLocal:
> + values[0] = pstrdup("Local");
> + break;

I agree with Jaime: this should be lowercase. And do you really need to
pstrdup() these strings?

> + appendStringInfoString(&str, "{");
> + foreach(dbcell, hba->databases)
> + {
> + tok = lfirst(dbcell);
> + appendStringInfoString(&str, tok->string);
> + appendStringInfoString(&str, ",");
> + }
> +
> + /*
> + * For any number of values inserted, separator at the end needs to be
> + * replaced by string terminator
> + */
> + if (str.len > 1)
> + {
> + str.data[str.len - 1] = '\0';
> + str.len -= 1;
> + appendStringInfoString(&str, "}");
> + values[1] = str.data;
> + }
> + else
> + values[1] = NULL;

I don't like this at all. I would write it something like this:

n = 0;

/* hba->conntype stuff */

n++;
if (list_length(hba->databases) != 0)
{
initStringInfo(&str);
appendStringInfoString(&str, "{");

foreach(cell, hba->databases)
{
tok = lfirst(cell);
appendStringInfoString(&str, tok->string);
appendStringInfoString(&str, ",");
}

str.data[str.len - 1] = '}';
values[n] = str.data;
}
else
values[n] = NULL;

Note the variable n instead of using 0/1/… indexes into values, and that
I moved the call to initStringInfo from the beginning of the function.

The same applies to the other cases below.

(But this is, of course, all subject to whatever solution is found to
the all/{all} problem.)

> /* Retrieving Socket address */
> switch (hba->addr.ss_family)
> {
> case AF_INET:
> len = sizeof(struct sockaddr_in);
> break;
> #ifdef HAVE_IPV6
> case AF_INET6:
> len = sizeof(struct sockaddr_in6);
> break;
> #endif
> default:
> len = sizeof(struct sockaddr_storage);
> break;
> }
> if (getnameinfo((const struct sockaddr *) & hba->addr, len, buffer, sizeof(buffer), NULL, 0, NI_NUMERICHOST) == 0)
> values[3] = pstrdup(buffer);
> else
> values[3] = NULL;

This should use pg_getnameinfo_all. You don't need the switch, just pass
in .salen. Use a buffer of NI_MAXHOST, not 256. Look for other calls to
pg_getnameinfo_all for examples. (Also split long lines.)

(Note: this pstrdup is OK.)

> /* Retrieving socket mask */
> switch (hba->mask.ss_family)
> {
> case AF_INET:

Same.

> + case ipCmpMask:
> + values[5] = pstrdup("Mask");
> + break;

Lowercase, no pstrdup.

> + case uaReject:
> + values[7] = pstrdup("Reject");
> + break;

Same.

> + if ((hba->usermap) && (hba->auth_method == uaIdent || hba->auth_method == uaPeer || hba->auth_method == uaCert || hba->auth_method == uaGSS || hba->auth_method == uaSSPI))

Split across lines.

> + appendStringInfoString(&str, pstrdup(hba->ldapserver));

No need for the many, many pstrdup()s.

> + if (str.len > 1)
> + {
> + str.data[str.len - 1] = '\0';
> + str.len -= 1;
> + appendStringInfoString(&str, "}");
> + values[8] = str.data;
> + }
> + else
> + values[8] = NULL;

This should become:

if (str.len != 0)
{
str.data[str.len - 1] = '}';
values[n] = str.data;
}
else
values[n] = NULL;

> + /* Retrieving linenumber */
> + snprintf(buffer, sizeof(buffer), "%d", hba->linenumber);
> + values[9] = pstrdup(buffer);

Use psprintf() and get rid of the static buffer.

> + /* stuff done only on the first call of the function */
> + if (SRF_IS_FIRSTCALL())
> + {
> + /* create a function context for cross-call persistence */
> + funcctx = SRF_FIRSTCALL_INIT();

This is not a comment about your patch, just a general complaint about
how everyone cargo-cults ALL of these comments for EVERY SINGLE SRF.
Isn't "if (SRF_IS_FIRSTCALL())" _perfectly_ clear, or is it just me?

I'd just as soon rip them all out of this patch.

> + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "connection_type",
> + TEXTOID, -1, 0);

(See comments below about column names.)

> + else
> + {
> + /* do when there is no more left */
> + SRF_RETURN_DONE(funcctx);
> + }

I'd just say SRF_RETURN_DONE(funcctx) at the end without the else block.

> + DATA(insert OID = 3206 ( pg_show_all_hba_settings PGNSP PGUID 12 1 1000 0 0 f f f f t t s 0 0 2249 "" "{25,1009,1009,25,25,25,25,25,1009,23}" "{o,o,o,o,o,o,o,o,o,o}" "{connection_type,databases,roles,socket_address,socket_mask,compare_method,hostname,authmethod,configuration_options,linenumber}" _null_ show_all_hba_settings _null_ _null_ _null_ ));
> + DESCR("SHOW ALL HBA settings as a function");

OID 3206 is now in use by the #>> operator, so you'll have to pick
another when you resubmit.

I went through the values one by one, and they look OK. But see comments
below about the column names.

The description should be "Return client authentication settings" or so.

> + <para>
> + The view <structname>pg_hba_settings</structname> provides access to
> + useful information about authentication details of Postgresql cluster.
> + For security reasons, access to this view is limited only to superusers. No update, insert or delete operations are allowed in this view.
> + </para>

I suggest:

The read-only <structname>pg_hba_settings</structname> view provides
access to the client authentication configuration from pg_hba.conf.
Access to this view is limited to superusers.

(So this addresses Jaime's comment: the view is already superuser-only.)

> + <entry><structfield>connection_type</structfield></entry>

I'd call this just "type" to match pg_hba.conf.

> + <entry><structfield>databases</structfield></entry>
> + <entry><type>text[]</type></entry>
> + <entry>List of database names</entry>

I'm really not a fan of returning an array with e.g. "{all}". But I'm
not sure what to do instead. I think the really right thing to do would
be to have two separate columns, one with "all", "sameuser", "samerole",
"replication", or empty; and the other an array of database names.

I'd like to hear what others think about this.

> + <entry><structfield>roles</structfield></entry>

Same problem.

> + <entry><structfield>socket_address</structfield></entry>

Just "address".

> + <entry><structfield>socket_mask</structfield></entry>

Just "mask".

> + <entry><structfield>authmethod</structfield></entry>

Just "method".

> + <entry><structfield>configuration_options</structfield></entry>

Just "options".

> + <entry><structfield>linenumber</structfield></entry>

Should be "line_number", I think.

> + <para>
> + Details of this file can be accessed from pg_hba_settings view.
> + See <xref linkend="view-pg-hba-settings"> for details.
> + </para>

I suggest "The contents of this file are reflected in the
pg_hba_settings view. See … for details."

Do you have the time to rework the patch along these lines and resubmit
for this commitfest (i.e. in the next few days, certainly sometime this
week)? If not, please let me know and I will move it to the next CF to
give you time to make the changes.

Thank you for your contribution. Although I've asked for a lot of
changes, I think the feature is certainly one we want.

-- Abhijit

At 2014-06-29 22:25:54 +0530, ams(at)2ndQuadrant(dot)com wrote:
>
> I think the really right thing to do would be to have two separate
> columns, one with "all", "sameuser", "samerole", "replication", or
> empty; and the other an array of database names.

After sleeping on it, I realised that the code would return '{all}' for
'all' in pg_hba.conf, but '{"all"}' for '"all"'. So it's not exactly
ambiguous, but I don't think it's especially useful for callers.

I think having two columns would work. The columns could be called
"database" and "database_list" and "user" and "user_list" respectively.

The database column may contain one of "all", "sameuser", "samegroup",
"replication", but if it's empty, database_list will contain an array of
database names. Then ("all", {}) and ("", {all}) are easily separated.
Likewise for user and user_list.

I've marked this patch "returned with feedback" and moved it to the
August CF after discussion with Vaishnavi.

-- Abhijit

On Mon, Jun 30, 2014 at 5:06 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com> wrote:
> I think having two columns would work. The columns could be called
> "database" and "database_list" and "user" and "user_list" respectively.
>
> The database column may contain one of "all", "sameuser", "samegroup",
> "replication", but if it's empty, database_list will contain an array of
> database names. Then ("all", {}) and ("", {all}) are easily separated.
> Likewise for user and user_list.

Thanks for the review.

I corrected all the review comments except the one to add two columns
as (database, database_list and user, user_list). I feel this may cause
some confusion to the users.

Here I attached the latest version of the patch.
I will add this patch to the next commitfest.

Regards,
Hari Babu
Fujitsu Australia

On 1/27/15 1:04 AM, Haribabu Kommi wrote:
> On Mon, Jun 30, 2014 at 5:06 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com> wrote:
>> I think having two columns would work. The columns could be called
>> "database" and "database_list" and "user" and "user_list" respectively.
>>
>> The database column may contain one of "all", "sameuser", "samegroup",
>> "replication", but if it's empty, database_list will contain an array of
>> database names. Then ("all", {}) and ("", {all}) are easily separated.
>> Likewise for user and user_list.
>
> Thanks for the review.
>
> I corrected all the review comments except the one to add two columns
> as (database, database_list and user, user_list). I feel this may cause
> some confusion to the users.
>
> Here I attached the latest version of the patch.
> I will add this patch to the next commitfest.

Apologies if this was covered, but why isn't the IP address an inet instead of text?

Also, what happens if someone reloads the config in the middle of running the SRF? ISTM it'd be better to do something like process all of parsed_hba_lines into a tuplestore. Obviously there's still a race condition there, but at least it's a lot smaller, and AFAIK no worse than the pg_stats views.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Wed, Jan 28, 2015 at 9:47 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> On 1/27/15 1:04 AM, Haribabu Kommi wrote:
>>
>> Here I attached the latest version of the patch.
>> I will add this patch to the next commitfest.
>
>
> Apologies if this was covered, but why isn't the IP address an inet instead
> of text?

Corrected the address type as inet instead of text. updated patch is attached.

> Also, what happens if someone reloads the config in the middle of running
> the SRF?

hba entries are reloaded only in postmaster process, not in every backend.
So there shouldn't be any problem with config file reload. Am i
missing something?

Regards,
Hari Babu
Fujitsu Australia

On Wed, Jan 28, 2015 at 4:46 AM, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
wrote:
>
> On Wed, Jan 28, 2015 at 9:47 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>
wrote:
> > On 1/27/15 1:04 AM, Haribabu Kommi wrote:
> >>
> >> Here I attached the latest version of the patch.
> >> I will add this patch to the next commitfest.
> >
> >
> > Apologies if this was covered, but why isn't the IP address an inet
instead
> > of text?
>
> Corrected the address type as inet instead of text. updated patch is
attached.
>
> > Also, what happens if someone reloads the config in the middle of
running
> > the SRF?
>
> hba entries are reloaded only in postmaster process, not in every backend.
> So there shouldn't be any problem with config file reload. Am i
> missing something?
>

I don't have any comment to disagree with the patch (idea and code).

But I'm thinking about this patch and would not be interesting to have a
FDW to manipulate the hba file? Imagine if we are able to manipulate the
HBA file using INSERT/UPDATE/DELETE.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

=?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabriziomello(at)gmail(dot)com> writes:
> But I'm thinking about this patch and would not be interesting to have a
> FDW to manipulate the hba file? Imagine if we are able to manipulate the
> HBA file using INSERT/UPDATE/DELETE.

Since the HBA file is fundamentally order-dependent, while SQL tables
are fundamentally not, that doesn't seem like a great API match. You
could probably brute-force something that would work, but it would very
much be a case of using a hammer to solve a screwdriver problem.

regards, tom lane

On 1/28/15 12:46 AM, Haribabu Kommi wrote:
>> >Also, what happens if someone reloads the config in the middle of running
>> >the SRF?
> hba entries are reloaded only in postmaster process, not in every backend.
> So there shouldn't be any problem with config file reload. Am i
> missing something?

Ahh, good point. That does raise another issue though... there might well be some confusion about that. I think people are used to the varieties of how settings come into effect that perhaps this isn't an issue with pg_settings, but it's probably worth at least mentioning in the docs for a pg_hba view.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Wed, Jan 28, 2015 at 5:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabriziomello(at)gmail(dot)com> writes:
> > But I'm thinking about this patch and would not be interesting to have a
> > FDW to manipulate the hba file? Imagine if we are able to manipulate the
> > HBA file using INSERT/UPDATE/DELETE.
>
> Since the HBA file is fundamentally order-dependent, while SQL tables
> are fundamentally not, that doesn't seem like a great API match. You
> could probably brute-force something that would work, but it would very
> much be a case of using a hammer to solve a screwdriver problem.
>

Maybe, but my intention is provide an easy way to edit HBA entries. With an
extension or API to edit HBA entries many developers of PostgreSQL tools
(ie. pgadmin, phppgadmin, etc) will be benefited.

Perhaps a fdw can't be the best choice, maybe a complete new SQL syntax to
manipulate HBA entries like we did with ALTER SYSTEM. It's just some
thoughts about it.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog: http://fabriziomello.github.io
>> Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello
>> Github: http://github.com/fabriziomello

On 1/29/15 6:19 AM, Fabrízio de Royes Mello wrote:
> On Wed, Jan 28, 2015 at 5:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us <mailto:tgl(at)sss(dot)pgh(dot)pa(dot)us>> wrote:
> >
> > =?UTF-8?Q?Fabr=C3=ADzio_de_Royes_Mello?= <fabriziomello(at)gmail(dot)com <mailto:fabriziomello(at)gmail(dot)com>> writes:
> > > But I'm thinking about this patch and would not be interesting to have a
> > > FDW to manipulate the hba file? Imagine if we are able to manipulate the
> > > HBA file using INSERT/UPDATE/DELETE.
> >
> > Since the HBA file is fundamentally order-dependent, while SQL tables
> > are fundamentally not, that doesn't seem like a great API match. You
> > could probably brute-force something that would work, but it would very
> > much be a case of using a hammer to solve a screwdriver problem.
> >
>
> Maybe, but my intention is provide an easy way to edit HBA entries. With an extension or API to edit HBA entries many developers of PostgreSQL tools (ie. pgadmin, phppgadmin, etc) will be benefited.
>
> Perhaps a fdw can't be the best choice, maybe a complete new SQL syntax to manipulate HBA entries like we did with ALTER SYSTEM. It's just some thoughts about it.

Aside from Tom's concern about sets not being a good way to handle this (which I agree with), the idea of "editing" pg_hba.conf via SQL raises all the problems that were brought up when ALTER SYSTEM was being developed. One of the big problems is a question of how you can safely modify a text file that's full of comments and what-not. You'd need to address those issues if you hope to modify pg_hba.conf via SQL.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Fri, Jan 30, 2015 at 3:16 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>
> On 1/29/15 6:19 AM, Fabrízio de Royes Mello wrote:
>>
>> Perhaps a fdw can't be the best choice, maybe a complete new SQL syntax
to manipulate HBA entries like we did with ALTER SYSTEM. It's just some
thoughts about it.
>
>
> Aside from Tom's concern about sets not being a good way to handle this
(which I agree with), the idea of "editing" pg_hba.conf via SQL raises all
the problems that were brought up when ALTER SYSTEM was being developed.
One of the big problems is a question of how you can safely modify a text
file that's full of comments and what-not. You'd need to address those
issues if you hope to modify pg_hba.conf via SQL.
>

I think the big problem you are mentioning can be resolved in
a similar way as we have done for ALTER SYSTEM which is
to have a separate file (.auto.conf) for settings done via
ALTER SYSTEM command, do you see any major problem
with that approach.

Here one thing that is different is the format of pg_hba.conf file
which is quite different from postgresql.conf file, it seems to me
if want to handle this via ALTER SYSTEM we need to have
additional/different syntax.

For Example,

ALTER SYSTEM AUTH TYPE = <type_value> DATABASE = <value> ...
or
ALTER SYSTEM AUTH ...

In second syntax, user can mention the whole record. Internally we
need to validate the syntax and values.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Thu, Jan 29, 2015 at 10:13 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> I think the big problem you are mentioning can be resolved in
> a similar way as we have done for ALTER SYSTEM which is
> to have a separate file (.auto.conf) for settings done via
> ALTER SYSTEM command, do you see any major problem
> with that approach.

Yes. The contents of postgresql.conf are only mildly order-dependent.
If you put the same setting in more than once, it matters which one is
last. Apart from that, though, it doesn't really matter:
wal_keep_segments=10 means the same thing if it occurs before
max_connections=401 that it means after that. The same is not true of
pg_hba.conf, where the order matters a lot. This makes merging two
files together much less feasible, and much more confusing.

You are also a lot more likely to lock yourself out of the database by
adjusting pg_hba.conf. You can do that by modifying postgresql.conf,
say by putting an invalid combination of parameters in there or
getting it to request more semaphores or more RAM than the system can
accommodate or changing listen_addresses to 127.0.0.1, but there are
lots of things that you can do that carry no such risk. This is much
less true with pg_hba.conf. Even if I had a feature that would let me
modify pg_hba.conf remotely, I'm not sure I'd be brave enough to use
it.

Overall, this seems to me like a can of worms better left unopened.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 1/29/15 9:13 PM, Amit Kapila wrote:
> > Aside from Tom's concern about sets not being a good way to handle
> this (which I agree with), the idea of "editing" pg_hba.conf via SQL
> raises all the problems that were brought up when ALTER SYSTEM was being
> developed. One of the big problems is a question of how you can safely
> modify a text file that's full of comments and what-not. You'd need to
> address those issues if you hope to modify pg_hba.conf via SQL.
> >
>
> I think the big problem you are mentioning can be resolved in
> a similar way as we have done for ALTER SYSTEM which is
> to have a separate file (.auto.conf) for settings done via
> ALTER SYSTEM command, do you see any major problem
> with that approach.

Yes I do. pg_hba.conf is completely depending on ordering, so there's no
way you can simply toss another file into the mix. It's bad enough that
we do that with postgresql.auto.conf, but at least that's a simple
over-ride. With HBA a single ALTER SYSTEM could activate (or deactivate)
a huge swath of pg_hba.conf. That makes for a system that's fragile, and
since it's security related, dangerous.

I could maybe see an interface where we allowed users to perform
line-level operations on pg_hba.conf via SQL: UPDATE line X, INSERT
BEFORE/AFTER line X, DELETE line X. At least that would preserve the
critical nature of rules ordering.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Fri, Jan 30, 2015 at 10:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Thu, Jan 29, 2015 at 10:13 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > I think the big problem you are mentioning can be resolved in
> > a similar way as we have done for ALTER SYSTEM which is
> > to have a separate file (.auto.conf) for settings done via
> > ALTER SYSTEM command, do you see any major problem
> > with that approach.
>
> Yes. The contents of postgresql.conf are only mildly order-dependent.
> If you put the same setting in more than once, it matters which one is
> last. Apart from that, though, it doesn't really matter:
> wal_keep_segments=10 means the same thing if it occurs before
> max_connections=401 that it means after that. The same is not true of
> pg_hba.conf, where the order matters a lot.

Do you mean to say that as authentication system uses just the
first record that matches to perform authentication, it could lead
to problems if an order is not maintained? Won't the same
set of problems can occur if user tries to that manually and do
it without proper care of such rules. Now the problem with
command is that user can't see the order in which entries are
being made, but it seems to me that we can provide a view or some
way to user so that the order of entries is visible and the same is
allowed to be manipulated via command.

> This makes merging two
> files together much less feasible, and much more confusing.
>
> You are also a lot more likely to lock yourself out of the database by
> adjusting pg_hba.conf.

I think that could be even possible via Alter User .. password, if the
password is changed then also kind of user can be locked out of
database, basically it is also part of authentication mechanism.

> Even if I had a feature that would let me
> modify pg_hba.conf remotely, I'm not sure I'd be brave enough to use
> it.
>

Okay, but how about via some server side utility or some other way with
which users don't need to manually edit the file?

It seems to be that some of the other databases like Oracle also provide
a way for users to operate of similar files via commands, although in a
different way [1].

> Overall, this seems to me like a can of worms better left unopened.

Sure, I can understand the dangers you want to highlight, however
OTOH it seems to me that providing some way to users with which
they can change things without manually editing file is a good move.

[1]
http://docs.oracle.com/cd/B28359_01/network.111/b28317/lsnrctl.htm#i553656

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 01/30/2015 10:01 PM, Amit Kapila wrote:
> On Fri, Jan 30, 2015 at 10:58 PM, Robert Haas <robertmhaas(at)gmail(dot)com
> <mailto:robertmhaas(at)gmail(dot)com>> wrote:
>> Yes. The contents of postgresql.conf are only mildly order-dependent.
>> If you put the same setting in more than once, it matters which one is
>> last. Apart from that, though, it doesn't really matter:
>> wal_keep_segments=10 means the same thing if it occurs before
>> max_connections=401 that it means after that. The same is not true of
>> pg_hba.conf, where the order matters a lot.
>
> Do you mean to say that as authentication system uses just the
> first record that matches to perform authentication, it could lead
> to problems if an order is not maintained? Won't the same
> set of problems can occur if user tries to that manually and do
> it without proper care of such rules. Now the problem with
> command is that user can't see the order in which entries are
> being made, but it seems to me that we can provide a view or some
> way to user so that the order of entries is visible and the same is
> allowed to be manipulated via command.

We *can*, yes. But the technical issues around that have not been
addressed. Certainly just making the new system view respond to
UPDATE/INSERT/DELETE would not be sufficient.

And then once we address the technical issues, we'll need to address the
security implications.

I think this is worth doing; there's some tremendous utility potential
in having a PostgresQL which can be 100% managed via port 5432,
especially for the emerging world of container-based hosting (Docker et.
al.). However, it's also going to be difficult.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Hi

I am sending a review of this patch.

1. We would this patch?

yes. It is a good idea - checking internal view is more comfortable and
faster than checking some (possibly longer) pg_hba.conf. There was no
objections.

2. Scope - does this patch, what we need?

yes. There was a discussion about altering pg_hba.conf from SQL, but we
don't need it now.

3. Patching, compilation

no warnings, no errors

4. Regress tests

test rules ... FAILED -- missing info about new view

My objections:

1. data type for "database" field should be array of name or array of text.

When name contains a comma, then this comma is not escaped

currently: {omega,my stupid extremly, name2,my stupid name}

expected: {"my stupid name",omega,"my stupid extremly, name2"}

Same issue I see in "options" field

2. Reload is not enough for content refresh - logout is necessary

I understand, why it is, but it is not very friendly, and can be very
stressful. It should to work with last reloaded data.

I have not too strong opinion on @1, but @2 should be fixed.

Regards

Pavel

2015-01-28 7:46 GMT+01:00 Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>:

> On Wed, Jan 28, 2015 at 9:47 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com>
> wrote:
> > On 1/27/15 1:04 AM, Haribabu Kommi wrote:
> >>
> >> Here I attached the latest version of the patch.
> >> I will add this patch to the next commitfest.
> >
> >
> > Apologies if this was covered, but why isn't the IP address an inet
> instead
> > of text?
>
> Corrected the address type as inet instead of text. updated patch is
> attached.
>
> > Also, what happens if someone reloads the config in the middle of running
> > the SRF?
>
> hba entries are reloaded only in postmaster process, not in every backend.
> So there shouldn't be any problem with config file reload. Am i
> missing something?
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

On Sat, Feb 7, 2015 at 8:26 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hi
>
> I am sending a review of this patch.

Thanks for the review. sorry for the delay.

> 4. Regress tests
>
> test rules ... FAILED -- missing info about new view

Thanks. Corrected.

> My objections:
>
> 1. data type for "database" field should be array of name or array of text.
>
> When name contains a comma, then this comma is not escaped
>
> currently: {omega,my stupid extremly, name2,my stupid name}
>
> expected: {"my stupid name",omega,"my stupid extremly, name2"}
>
> Same issue I see in "options" field

Changed including the user column also.

> 2. Reload is not enough for content refresh - logout is necessary
>
> I understand, why it is, but it is not very friendly, and can be very
> stressful. It should to work with last reloaded data.

At present the view data is populated from the variable "parsed_hba_lines"
which contains the already parsed lines of pg_hba.conf file.

During the reload, the hba entries are reloaded only in the postmaster process
and not in every backend, because of this reason the reloaded data is
not visible until
the session is disconnected and connected.

Instead of changing the SIGHUP behavior in the backend to load the hba entries
also, whenever the call is made to the view, the pg_hba.conf file is
parsed to show
the latest reloaded data in the view. This way it doesn't impact the
existing behavior
but it may impact the performance because of file load into memory every time.

Updated patch is attached. comments?

Regards,
Hari Babu
Fujitsu Australia

Hi

It looks well, I have only one objection.

I am not sure so function "hba_settings" should be in file guc.c - it has
zero relation to GUC.

Maybe hba.c file is better probably.

Other opinions?

2015-02-27 7:30 GMT+01:00 Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>:

> On Sat, Feb 7, 2015 at 8:26 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> > Hi
> >
> > I am sending a review of this patch.
>
> Thanks for the review. sorry for the delay.
>
> > 4. Regress tests
> >
> > test rules ... FAILED -- missing info about new view
>
> Thanks. Corrected.
>
> > My objections:
> >
> > 1. data type for "database" field should be array of name or array of
> text.
> >
> > When name contains a comma, then this comma is not escaped
> >
> > currently: {omega,my stupid extremly, name2,my stupid name}
> >
> > expected: {"my stupid name",omega,"my stupid extremly, name2"}
> >
> > Same issue I see in "options" field
>
> Changed including the user column also.
>
> > 2. Reload is not enough for content refresh - logout is necessary
> >
> > I understand, why it is, but it is not very friendly, and can be very
> > stressful. It should to work with last reloaded data.
>
> At present the view data is populated from the variable "parsed_hba_lines"
> which contains the already parsed lines of pg_hba.conf file.
>
> During the reload, the hba entries are reloaded only in the postmaster
> process
> and not in every backend, because of this reason the reloaded data is
> not visible until
> the session is disconnected and connected.
>
> Instead of changing the SIGHUP behavior in the backend to load the hba
> entries
> also, whenever the call is made to the view, the pg_hba.conf file is
> parsed to show
> the latest reloaded data in the view. This way it doesn't impact the
> existing behavior
> but it may impact the performance because of file load into memory every
> time.
>

your solution is ok, simply and clean. Performance for this view should not
be critical and every time will be faster than login as root and searching
pg_hba.conf

Regards

Pavel

>
> Updated patch is attached. comments?
>
> Regards,
> Hari Babu
> Fujitsu Australia
>

Hi,

On 28.1.2015 23:01, Jim Nasby wrote:
> On 1/28/15 12:46 AM, Haribabu Kommi wrote:
>>> >Also, what happens if someone reloads the config in the middle of
>>> running
>>> >the SRF?
>> hba entries are reloaded only in postmaster process, not in every
>> backend.
>> So there shouldn't be any problem with config file reload. Am i
>> missing something?
>
> Ahh, good point. That does raise another issue though... there might
> well be some confusion about that. I think people are used to the
> varieties of how settings come into effect that perhaps this isn't an
> issue with pg_settings, but it's probably worth at least mentioning in
> the docs for a pg_hba view.

I share this fear, and it's the main problem I have with this patch.

Currently, the patch always does load_hba() every time pg_hba_settings
is accessed, which loads the current contents of the config file into
the backend process, but the postmaster still uses the original one.
This makes it impossible to look at currently effective pg_hba.conf
contents. Damn confusing, I guess.

Also, I dare to say that having a system view that doesn't actually show
the system state (but contents of a config file that may not be loaded
yet), is wrong.

Granted, we don't modify pg_hba.conf all that often, and it's usually
followed by a SIGHUP to reload the contents, so both the backend and
postmaster should see the same stuff. But the cases when I'd use this
pg_hba_settings view usually turned out to be 'forgot to SIGHUP', so
this would not help, actually.

What I can imagine is keeping the original list (parsed by postmaster,
containing the effective pg_hba.conf contents), and keeping another list
of 'current contents' within backends. And having a 'reload' flag for
pg_hba_settings, determining which list to use.

pg_hba_settings(reload=false) -> old list (from postmaster)
pg_hba_settings(reload=true) -> new list (from backend)

The pg_hba_settings view should use 'reload=false' (i.e. show effective
contents of the hba).

The other feature that'd be cool to have is a debugging function on top
of the view, i.e. a function pg_hba_check(host, ip, db, user, pwd)
showing which hba rule matched. But that's certainly nontrivial.

--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

All,

* Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> On 28.1.2015 23:01, Jim Nasby wrote:
> > On 1/28/15 12:46 AM, Haribabu Kommi wrote:
> >>> >Also, what happens if someone reloads the config in the middle of
> >>> running
> >>> >the SRF?
> >> hba entries are reloaded only in postmaster process, not in every
> >> backend.
> >> So there shouldn't be any problem with config file reload. Am i
> >> missing something?
> >
> > Ahh, good point. That does raise another issue though... there might
> > well be some confusion about that. I think people are used to the
> > varieties of how settings come into effect that perhaps this isn't an
> > issue with pg_settings, but it's probably worth at least mentioning in
> > the docs for a pg_hba view.
>
> I share this fear, and it's the main problem I have with this patch.

Uh, yeah, agreed.

> Currently, the patch always does load_hba() every time pg_hba_settings
> is accessed, which loads the current contents of the config file into
> the backend process, but the postmaster still uses the original one.
> This makes it impossible to look at currently effective pg_hba.conf
> contents. Damn confusing, I guess.

Indeed. At a *minimum*, we'd need to have some way to say "what you're
seeing isn't what's actually being used".

> Also, I dare to say that having a system view that doesn't actually show
> the system state (but contents of a config file that may not be loaded
> yet), is wrong.

Right, we need to show what's currently active in PG-land, not just spit
back whatever the on-disk contents currently are.

> Granted, we don't modify pg_hba.conf all that often, and it's usually
> followed by a SIGHUP to reload the contents, so both the backend and
> postmaster should see the same stuff. But the cases when I'd use this
> pg_hba_settings view usually turned out to be 'forgot to SIGHUP', so
> this would not help, actually.

Agreed.

> What I can imagine is keeping the original list (parsed by postmaster,
> containing the effective pg_hba.conf contents), and keeping another list
> of 'current contents' within backends. And having a 'reload' flag for
> pg_hba_settings, determining which list to use.
>
> pg_hba_settings(reload=false) -> old list (from postmaster)
> pg_hba_settings(reload=true) -> new list (from backend)
>
> The pg_hba_settings view should use 'reload=false' (i.e. show effective
> contents of the hba).

I don't think we actually care what the "current contents" are from the
backend's point of view- after all, when does an individual backend ever
use the contents of pg_hba.conf after it's up and running? What would
make sense, to me at least, would be:

pg_hba_configured() -- spits back whatever the config file has
pg_hba_active() -- shows what the postmaster is using currently

Or something along those lines. Note that I'd want pg_hba_configured()
to throw an error if the config file can't be parsed (which would be
quite handy to make sure that you didn't goof up the config..). This,
combined with pg_reload_conf() would provide a good way to review
changes before putting them into place.

> The other feature that'd be cool to have is a debugging function on top
> of the view, i.e. a function pg_hba_check(host, ip, db, user, pwd)
> showing which hba rule matched. But that's certainly nontrivial.

I'm not sure that I see why, offhand, it'd be much more than trivial..

Thanks!

Stephen

2015-02-27 17:59 GMT+01:00 Stephen Frost <sfrost(at)snowman(dot)net>:

> All,
>
> * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> > On 28.1.2015 23:01, Jim Nasby wrote:
> > > On 1/28/15 12:46 AM, Haribabu Kommi wrote:
> > >>> >Also, what happens if someone reloads the config in the middle of
> > >>> running
> > >>> >the SRF?
> > >> hba entries are reloaded only in postmaster process, not in every
> > >> backend.
> > >> So there shouldn't be any problem with config file reload. Am i
> > >> missing something?
> > >
> > > Ahh, good point. That does raise another issue though... there might
> > > well be some confusion about that. I think people are used to the
> > > varieties of how settings come into effect that perhaps this isn't an
> > > issue with pg_settings, but it's probably worth at least mentioning in
> > > the docs for a pg_hba view.
> >
> > I share this fear, and it's the main problem I have with this patch.
>
> Uh, yeah, agreed.
>

yes, good notice. I was blind.

>
> > Currently, the patch always does load_hba() every time pg_hba_settings
> > is accessed, which loads the current contents of the config file into
> > the backend process, but the postmaster still uses the original one.
> > This makes it impossible to look at currently effective pg_hba.conf
> > contents. Damn confusing, I guess.
>
> Indeed. At a *minimum*, we'd need to have some way to say "what you're
> seeing isn't what's actually being used".
>
> > Also, I dare to say that having a system view that doesn't actually show
> > the system state (but contents of a config file that may not be loaded
> > yet), is wrong.
>
> Right, we need to show what's currently active in PG-land, not just spit
> back whatever the on-disk contents currently are.
>
> > Granted, we don't modify pg_hba.conf all that often, and it's usually
> > followed by a SIGHUP to reload the contents, so both the backend and
> > postmaster should see the same stuff. But the cases when I'd use this
> > pg_hba_settings view usually turned out to be 'forgot to SIGHUP', so
> > this would not help, actually.
>
> Agreed.
>
> > What I can imagine is keeping the original list (parsed by postmaster,
> > containing the effective pg_hba.conf contents), and keeping another list
> > of 'current contents' within backends. And having a 'reload' flag for
> > pg_hba_settings, determining which list to use.
> >
> > pg_hba_settings(reload=false) -> old list (from postmaster)
> > pg_hba_settings(reload=true) -> new list (from backend)
> >
> > The pg_hba_settings view should use 'reload=false' (i.e. show effective
> > contents of the hba).
>
> I don't think we actually care what the "current contents" are from the
> backend's point of view- after all, when does an individual backend ever
> use the contents of pg_hba.conf after it's up and running? What would
> make sense, to me at least, would be:
>
> pg_hba_configured() -- spits back whatever the config file has
> pg_hba_active() -- shows what the postmaster is using currently
>

I disagree and I dislike this direction. It is looks like over engineering.

* load every time is wrong, because you will see possibly not active data.

* ignore reload is a attack to mental health of our users.

It should to work like "pg_settings". I need to see "what is wrong in this
moment" in pg_hba.conf, not what was or what will be wrong.

We can load any config files via admin contrib module - so there is not
necessary repeat same functionality

Regards

Pavel

> Or something along those lines. Note that I'd want pg_hba_configured()
> to throw an error if the config file can't be parsed (which would be
> quite handy to make sure that you didn't goof up the config..). This,
> combined with pg_reload_conf() would provide a good way to review
> changes before putting them into place.
>
> > The other feature that'd be cool to have is a debugging function on top
> > of the view, i.e. a function pg_hba_check(host, ip, db, user, pwd)
> > showing which hba rule matched. But that's certainly nontrivial.
>
> I'm not sure that I see why, offhand, it'd be much more than trivial..
>
> Thanks!
>
> Stephen
>

On 27.2.2015 17:59, Stephen Frost wrote:
> All,
>
> * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
>>
>> The other feature that'd be cool to have is a debugging function
>> on top of the view, i.e. a function pg_hba_check(host, ip, db,
>> user, pwd) showing which hba rule matched. But that's certainly
>> nontrivial.
>
> I'm not sure that I see why, offhand, it'd be much more than trivial
> ...

From time to time I have to debug why are connection attempts failing,
and with moderately-sized pg_hba.conf files (e.g. on database servers
shared by multiple applications) that may be tricky. Identifying the
rule that matched (and rejected) the connection would be helpful.

But yes, that's non-trivial and out of scope of this patch.

--
Tomas Vondra http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Fri, Feb 27, 2015 at 12:48 PM, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com
> wrote:

> On 27.2.2015 17:59, Stephen Frost wrote:
> > All,
> >
> > * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> >>
> >> The other feature that'd be cool to have is a debugging function
> >> on top of the view, i.e. a function pg_hba_check(host, ip, db,
> >> user, pwd) showing which hba rule matched. But that's certainly
> >> nontrivial.
> >
> > I'm not sure that I see why, offhand, it'd be much more than trivial
> > ...
>
> From time to time I have to debug why are connection attempts failing,
> and with moderately-sized pg_hba.conf files (e.g. on database servers
> shared by multiple applications) that may be tricky. Identifying the
> rule that matched (and rejected) the connection would be helpful.
>

If you did actually get a rejected connection, you get that in the log (as
of 9.3, iirc). Such a function would make it possible to test it without
having failed first though :)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> 2015-02-27 17:59 GMT+01:00 Stephen Frost <sfrost(at)snowman(dot)net>:
> > I don't think we actually care what the "current contents" are from the
> > backend's point of view- after all, when does an individual backend ever
> > use the contents of pg_hba.conf after it's up and running? What would
> > make sense, to me at least, would be:
> >
> > pg_hba_configured() -- spits back whatever the config file has
> > pg_hba_active() -- shows what the postmaster is using currently
>
> I disagree and I dislike this direction. It is looks like over engineering.
>
> * load every time is wrong, because you will see possibly not active data.

That's the point of the two functions- one to give you what a reload
*now* would, and one to see what's currently active.

> * ignore reload is a attack to mental health of our users.

Huh?

> It should to work like "pg_settings". I need to see "what is wrong in this
> moment" in pg_hba.conf, not what was or what will be wrong.

That's what pg_hba_active() would be from above, yes.

> We can load any config files via admin contrib module - so there is not
> necessary repeat same functionality

That's hardly the same- I can't (easily, anyway) join the results of
pg_read() to pg_hba_active() and see what's different or the same.

Thanks,

Stephen

* Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> On 27.2.2015 17:59, Stephen Frost wrote:
> > All,
> >
> > * Tomas Vondra (tomas(dot)vondra(at)2ndquadrant(dot)com) wrote:
> >>
> >> The other feature that'd be cool to have is a debugging function
> >> on top of the view, i.e. a function pg_hba_check(host, ip, db,
> >> user, pwd) showing which hba rule matched. But that's certainly
> >> nontrivial.
> >
> > I'm not sure that I see why, offhand, it'd be much more than trivial
> > ...
>
> From time to time I have to debug why are connection attempts failing,
> and with moderately-sized pg_hba.conf files (e.g. on database servers
> shared by multiple applications) that may be tricky. Identifying the
> rule that matched (and rejected) the connection would be helpful.

To clarify, I was trying to say that writing that function didn't seem
very difficult to me. I definitely think that *having* that function
would be very useful.

> But yes, that's non-trivial and out of scope of this patch.

For my 2c, I view this as somewhat up to the author. I wouldn't
complain if it was included in a new version of this patch as I don't
think it'd add all that much complexity and it'd be very nice, but I
certainly think this patch could go in without that too.

Thanks!

Stephen

On 02/27/2015 10:35 AM, Stephen Frost wrote:
>> From time to time I have to debug why are connection attempts failing,
>> > and with moderately-sized pg_hba.conf files (e.g. on database servers
>> > shared by multiple applications) that may be tricky. Identifying the
>> > rule that matched (and rejected) the connection would be helpful.
> To clarify, I was trying to say that writing that function didn't seem
> very difficult to me. I definitely think that *having* that function
> would be very useful.
>
>> > But yes, that's non-trivial and out of scope of this patch.
> For my 2c, I view this as somewhat up to the author. I wouldn't
> complain if it was included in a new version of this patch as I don't
> think it'd add all that much complexity and it'd be very nice, but I
> certainly think this patch could go in without that too.

Also, a testing function could be written in userspace after this
feature is added. I can imagine how to write one as a UDF.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

2015-02-27 19:32 GMT+01:00 Stephen Frost <sfrost(at)snowman(dot)net>:

> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> > 2015-02-27 17:59 GMT+01:00 Stephen Frost <sfrost(at)snowman(dot)net>:
> > > I don't think we actually care what the "current contents" are from the
> > > backend's point of view- after all, when does an individual backend
> ever
> > > use the contents of pg_hba.conf after it's up and running? What would
> > > make sense, to me at least, would be:
> > >
> > > pg_hba_configured() -- spits back whatever the config file has
> > > pg_hba_active() -- shows what the postmaster is using currently
> >
> > I disagree and I dislike this direction. It is looks like over
> engineering.
> >
> > * load every time is wrong, because you will see possibly not active
> data.
>
> That's the point of the two functions- one to give you what a reload
> *now* would, and one to see what's currently active.
>

> > * ignore reload is a attack to mental health of our users.
>
> Huh?
>

this topic should be divided, please. One part - functions for loading
pg_hba and translating to some table. Can be two, can be one with one
parameter. It will be used probably by advanced user, and I am able to
accept it like you or Tomas proposed. Second part is the content of view
pg_hba_conf. It should be only one and should to view a active content. I
mean a content that is actively used - when other session is started. I am
strongly against the behave, when I have to close session to refresh a
content of this view (after reload) - and I am against to see there not
active content.

Regards

Pavel

>
> > It should to work like "pg_settings". I need to see "what is wrong in
> this
> > moment" in pg_hba.conf, not what was or what will be wrong.
>
> That's what pg_hba_active() would be from above, yes.
>
> > We can load any config files via admin contrib module - so there is not
> > necessary repeat same functionality
>
> That's hardly the same- I can't (easily, anyway) join the results of
> pg_read() to pg_hba_active() and see what's different or the same.
>
> Thanks,
>
> Stephen
>

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> this topic should be divided, please. One part - functions for loading
> pg_hba and translating to some table. Can be two, can be one with one
> parameter. It will be used probably by advanced user, and I am able to
> accept it like you or Tomas proposed.

I'm not sure why you'd need a parameter for the function. The function
could back a view too. In general, I think you're agreeing with me that
it'd be a useful capability to have.

> Second part is the content of view
> pg_hba_conf. It should be only one and should to view a active content. I
> mean a content that is actively used - when other session is started. I am
> strongly against the behave, when I have to close session to refresh a
> content of this view (after reload) - and I am against to see there not
> active content.

Right, we also need a view (or function, or both) which provides what
the *active* configuration of the running postmaster is. This is
exactly what I was proposing (or what I was intending to, at least) with
pg_hba_active, so, again, I think we're in agreement here.

Thanks,

Stephen

2015-02-27 20:55 GMT+01:00 Stephen Frost <sfrost(at)snowman(dot)net>:

> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> > this topic should be divided, please. One part - functions for loading
> > pg_hba and translating to some table. Can be two, can be one with one
> > parameter. It will be used probably by advanced user, and I am able to
> > accept it like you or Tomas proposed.
>
> I'm not sure why you'd need a parameter for the function. The function
> could back a view too. In general, I think you're agreeing with me that
> it'd be a useful capability to have.
>

I have not a strong idea about the best interface. There is not a precedent
probably. Can be some little bit strange to have more functions for
reading this config file. So there can be one function with parameter like
Tomas proposed. But it is detail - and any user, who use this functionality
should be able to work with any variant.

> > Second part is the content of view
> > pg_hba_conf. It should be only one and should to view a active content. I
> > mean a content that is actively used - when other session is started. I
> am
> > strongly against the behave, when I have to close session to refresh a
> > content of this view (after reload) - and I am against to see there not
> > active content.
>
> Right, we also need a view (or function, or both) which provides what
> the *active* configuration of the running postmaster is. This is
> exactly what I was proposing (or what I was intending to, at least) with
> pg_hba_active, so, again, I think we're in agreement here.
>

perfect :)

Regards

Pavel

>
> Thanks,
>
> Stephen
>

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Right, we also need a view (or function, or both) which provides what
> the *active* configuration of the running postmaster is. This is
> exactly what I was proposing (or what I was intending to, at least) with
> pg_hba_active, so, again, I think we're in agreement here.

I think that's going to be a lot harder than you realize, and it will have
undesirable security implications, in that whatever you do to expose the
postmaster's internal state to backends will also make it visible to other
onlookers; not to mention probably adding new failure modes.

There are also nontrivial semantic differences in this area between
Windows and other platforms (ie in an EXEC_BACKEND build the current file
contents *are* the active version). If you insist on two views you will
need to explain why/how they act differently on different platforms.

I think the proposed mechanism (ie read and return the current contents of
the file) is just fine, and we should stop there rather than engineering
this to death. We've survived twenty years with *no* feature of this
sort, how is it suddenly essential that we expose postmaster internal
state?

regards, tom lane

2015-02-27 22:26 GMT+01:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Right, we also need a view (or function, or both) which provides what
> > the *active* configuration of the running postmaster is. This is
> > exactly what I was proposing (or what I was intending to, at least) with
> > pg_hba_active, so, again, I think we're in agreement here.
>
> I think that's going to be a lot harder than you realize, and it will have
> undesirable security implications, in that whatever you do to expose the
> postmaster's internal state to backends will also make it visible to other
> onlookers; not to mention probably adding new failure modes.
>

we can do copy of pg_hba.conf somewhere when postmaster starts or when it
is reloaded.

Later, we can read this copy from child nodes.

Is it a possibility?

Regards

Pavel

>
> There are also nontrivial semantic differences in this area between
> Windows and other platforms (ie in an EXEC_BACKEND build the current file
> contents *are* the active version). If you insist on two views you will
> need to explain why/how they act differently on different platforms.
>
> I think the proposed mechanism (ie read and return the current contents of
> the file) is just fine, and we should stop there rather than engineering
> this to death. We've survived twenty years with *no* feature of this
> sort, how is it suddenly essential that we expose postmaster internal
> state?
>
> regards, tom lane
>

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Right, we also need a view (or function, or both) which provides what
> > the *active* configuration of the running postmaster is. This is
> > exactly what I was proposing (or what I was intending to, at least) with
> > pg_hba_active, so, again, I think we're in agreement here.
>
> I think that's going to be a lot harder than you realize, and it will have
> undesirable security implications, in that whatever you do to expose the
> postmaster's internal state to backends will also make it visible to other
> onlookers; not to mention probably adding new failure modes.

I had been considering what it'd take (and certainly appreciated that
it's not trivial to look at the postmaster post-fork) but had also been
thinking a simpler approach might be possible (one which doesn't involve
*directly* looking at the postmaster config)- we could probably
reasonably consider whatever the backend has currently is the same as
the active configuration, provided we reload the pg_hba.conf into the
backends when a sighup is sent, just as we do with postgresql.conf.

I understand that there may be objections to that on the basis that it's
work that's (other than for this case) basically useless, but then
again, it's not like we anticipate reloads happening with a high
frequency or that we expect loading these files to take all that long.

The original patch only went off of what was in place when the backend
was started from the postmaster and the later patch changed it to just
always show what was currently in the pg_hba.conf file, but what
everyone on this thread (except those worried more about the
implementation and less about the capability) expects and wants is
"pg_settings, but for pg_hba". The way we get that is to do it exactly
the same as how we handle pg_settings.

> I think the proposed mechanism (ie read and return the current contents of
> the file) is just fine, and we should stop there rather than engineering
> this to death. We've survived twenty years with *no* feature of this
> sort, how is it suddenly essential that we expose postmaster internal
> state?

Perhaps I'm missing something, but I really don't see it to be a huge
deal to just reload pg_hba.conf in the backends when a sighup is done,
which would provide pg_settings-like results (ignoring that you can set
GUCs directly in the backend, of course), with two ways through the
function which loads the file- one which actually updates the global
setting in the backend during a sighup, and one which provides the
results in variables passed in by the caller (or similar) and allows
returning the contents of the current pg_hba.conf as parsed in an SRF.

Thanks!

Stephen

Pavel,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> 2015-02-27 22:26 GMT+01:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> > Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > > Right, we also need a view (or function, or both) which provides what
> > > the *active* configuration of the running postmaster is. This is
> > > exactly what I was proposing (or what I was intending to, at least) with
> > > pg_hba_active, so, again, I think we're in agreement here.
> >
> > I think that's going to be a lot harder than you realize, and it will have
> > undesirable security implications, in that whatever you do to expose the
> > postmaster's internal state to backends will also make it visible to other
> > onlookers; not to mention probably adding new failure modes.
>
> we can do copy of pg_hba.conf somewhere when postmaster starts or when it
> is reloaded.

Please see my reply to Tom. There's no trivial way to reach into the
postmaster from a backend- but we do get a copy of whatever the
postmaster had when we forked, and the postmaster only reloads
pg_hba.conf on a sighup and that sighup is passed down to the children,
so we simply need to also reload the pg_hba.conf in the children when
they get a sighup.

That's how postgresql.conf is handled, which is what pg_settings is
based off of, and I believe is the behavior folks are really looking
for.

Thanks,

Stephen

On 02/27/2015 04:41 PM, Stephen Frost wrote:
>> we can do copy of pg_hba.conf somewhere when postmaster starts or when it
>> is reloaded.
>
> Please see my reply to Tom. There's no trivial way to reach into the
> postmaster from a backend- but we do get a copy of whatever the
> postmaster had when we forked, and the postmaster only reloads
> pg_hba.conf on a sighup and that sighup is passed down to the children,
> so we simply need to also reload the pg_hba.conf in the children when
> they get a sighup.
>
> That's how postgresql.conf is handled, which is what pg_settings is
> based off of, and I believe is the behavior folks are really looking
> for.

I thought the patch in question just implemented reading the file from
disk, and nothing else?

Speaking for my uses, I would rather have just that for 9.5 than wait
for something more sophisticated in 9.6.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> I understand that there may be objections to that on the basis that it's
> work that's (other than for this case) basically useless,

Got it in one.

I'm also not terribly happy about leaving security-relevant data sitting
around in backend memory 100% of the time. We have had bugs that exposed
backend memory contents for reading without also granting the ability to
execute arbitrary code, so I think doing this does represent a
quantifiable decrease in the security of pg_hba.conf.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > I understand that there may be objections to that on the basis that it's
> > work that's (other than for this case) basically useless,
>
> Got it in one.

Meh. It's hardly all that difficult and it's not useless if the user
wants to look at it.

> I'm also not terribly happy about leaving security-relevant data sitting
> around in backend memory 100% of the time. We have had bugs that exposed
> backend memory contents for reading without also granting the ability to
> execute arbitrary code, so I think doing this does represent a
> quantifiable decrease in the security of pg_hba.conf.

How is that any different from today? The only time it's not *already*
in backend memory is when the user has happened to go through and make a
change and used reload (instead of restart) and then it's not so much
that the security sensetive information isn't there, it's just out of
date.

I'm not entirely against the idea of changing how things are today, but
this argument simply doesn't apply to the current state of things.

Thanks!

Stephen

* Josh Berkus (josh(at)agliodbs(dot)com) wrote:
> On 02/27/2015 04:41 PM, Stephen Frost wrote:
> >> we can do copy of pg_hba.conf somewhere when postmaster starts or when it
> >> is reloaded.
> >
> > Please see my reply to Tom. There's no trivial way to reach into the
> > postmaster from a backend- but we do get a copy of whatever the
> > postmaster had when we forked, and the postmaster only reloads
> > pg_hba.conf on a sighup and that sighup is passed down to the children,
> > so we simply need to also reload the pg_hba.conf in the children when
> > they get a sighup.
> >
> > That's how postgresql.conf is handled, which is what pg_settings is
> > based off of, and I believe is the behavior folks are really looking
> > for.
>
> I thought the patch in question just implemented reading the file from
> disk, and nothing else?
>
> Speaking for my uses, I would rather have just that for 9.5 than wait
> for something more sophisticated in 9.6.

From my perspective, at least, the differences we're talking about are
not enough to raise this to a 9.5-vs-9.6 issue. I can see the use cases
for both (which is exactly why I suggested providing both). Having one
would be better than nothing, but I foretell lots of subsequent
complaints along the lines of "everything looks right according to
pg_hba_config, but I'm getting this error!!" Now, perhaps that's the
right approach to go for 9.5 since it'd more-or-less force our hand to
deal with it in 9.6 properly, but, personally, I'd be happier if we
moved forward with providing both because everyone agrees that it makes
sense rather than waiting to see if user complaints force our hand.

Thanks!

Stephen

2015-02-28 1:41 GMT+01:00 Stephen Frost <sfrost(at)snowman(dot)net>:

> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> > 2015-02-27 22:26 GMT+01:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> > > Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > > > Right, we also need a view (or function, or both) which provides what
> > > > the *active* configuration of the running postmaster is. This is
> > > > exactly what I was proposing (or what I was intending to, at least)
> with
> > > > pg_hba_active, so, again, I think we're in agreement here.
> > >
> > > I think that's going to be a lot harder than you realize, and it will
> have
> > > undesirable security implications, in that whatever you do to expose
> the
> > > postmaster's internal state to backends will also make it visible to
> other
> > > onlookers; not to mention probably adding new failure modes.
> >
> > we can do copy of pg_hba.conf somewhere when postmaster starts or when it
> > is reloaded.
>
> Please see my reply to Tom. There's no trivial way to reach into the
> postmaster from a backend- but we do get a copy of whatever the
> postmaster had when we forked, and the postmaster only reloads
> pg_hba.conf on a sighup and that sighup is passed down to the children,
> so we simply need to also reload the pg_hba.conf in the children when
> they get a sighup.
>
> That's how postgresql.conf is handled, which is what pg_settings is
> based off of, and I believe is the behavior folks are really looking
> for.
>

It has sense for me too.

Pavel

>
> Thanks,
>
> Stephen
>

2015-02-28 2:40 GMT+01:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > I understand that there may be objections to that on the basis that it's
> > work that's (other than for this case) basically useless,
>
> Got it in one.
>
> I'm also not terribly happy about leaving security-relevant data sitting
> around in backend memory 100% of the time. We have had bugs that exposed
> backend memory contents for reading without also granting the ability to
> execute arbitrary code, so I think doing this does represent a
> quantifiable decrease in the security of pg_hba.conf.
>

The Stephen's proposal changes nothing in security. These data are in
memory now. The only one difference is, so these data will be fresh.

Regards

Pavel

>
> regards, tom lane
>

2015-02-28 3:12 GMT+01:00 Stephen Frost <sfrost(at)snowman(dot)net>:

> * Josh Berkus (josh(at)agliodbs(dot)com) wrote:
> > On 02/27/2015 04:41 PM, Stephen Frost wrote:
> > >> we can do copy of pg_hba.conf somewhere when postmaster starts or
> when it
> > >> is reloaded.
> > >
> > > Please see my reply to Tom. There's no trivial way to reach into the
> > > postmaster from a backend- but we do get a copy of whatever the
> > > postmaster had when we forked, and the postmaster only reloads
> > > pg_hba.conf on a sighup and that sighup is passed down to the children,
> > > so we simply need to also reload the pg_hba.conf in the children when
> > > they get a sighup.
> > >
> > > That's how postgresql.conf is handled, which is what pg_settings is
> > > based off of, and I believe is the behavior folks are really looking
> > > for.
> >
> > I thought the patch in question just implemented reading the file from
> > disk, and nothing else?
> >
> > Speaking for my uses, I would rather have just that for 9.5 than wait
> > for something more sophisticated in 9.6.
>
> From my perspective, at least, the differences we're talking about are
> not enough to raise this to a 9.5-vs-9.6 issue. I can see the use cases
> for both (which is exactly why I suggested providing both). Having one
> would be better than nothing, but I foretell lots of subsequent
> complaints along the lines of "everything looks right according to
> pg_hba_config, but I'm getting this error!!" Now, perhaps that's the
> right approach to go for 9.5 since it'd more-or-less force our hand to
> deal with it in 9.6 properly, but, personally, I'd be happier if we
> moved forward with providing both because everyone agrees that it makes
> sense rather than waiting to see if user complaints force our hand.
>

+1

Probably we can implement simple load pg_hba.conf and tab transformation
early. There is a agreement and not any problem.

But if we start to implement some view, then it should be fully functional
without potential issues.

Regards

Pavel

>
> Thanks!
>
> Stephen
>

On Sat, Feb 28, 2015 at 11:41 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Pavel,
>
> * Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
>> 2015-02-27 22:26 GMT+01:00 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> > Stephen Frost <sfrost(at)snowman(dot)net> writes:
>> > > Right, we also need a view (or function, or both) which provides what
>> > > the *active* configuration of the running postmaster is. This is
>> > > exactly what I was proposing (or what I was intending to, at least) with
>> > > pg_hba_active, so, again, I think we're in agreement here.
>> >
>> > I think that's going to be a lot harder than you realize, and it will have
>> > undesirable security implications, in that whatever you do to expose the
>> > postmaster's internal state to backends will also make it visible to other
>> > onlookers; not to mention probably adding new failure modes.
>>
>> we can do copy of pg_hba.conf somewhere when postmaster starts or when it
>> is reloaded.
>
> Please see my reply to Tom. There's no trivial way to reach into the
> postmaster from a backend- but we do get a copy of whatever the
> postmaster had when we forked, and the postmaster only reloads
> pg_hba.conf on a sighup and that sighup is passed down to the children,
> so we simply need to also reload the pg_hba.conf in the children when
> they get a sighup.
>
> That's how postgresql.conf is handled, which is what pg_settings is
> based off of, and I believe is the behavior folks are really looking
> for.

Loading pg_hba.conf during SIGHUP in the backends will solve the
problem of displaying the
data which is not yet loaded. This change may produce a warning if it
fails to load pg_hba.conf in the backends.

Here I attached the updated patch. I didn't yet added the pg_hba_check function.
I feel it is better to be a separate patch. I can share the same later.

Regards,
Hari Babu
Fujitsu Australia

On Mon, Mar 2, 2015 at 6:36 AM, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
wrote:

> Loading pg_hba.conf during SIGHUP in the backends will solve the
> problem of displaying the
> data which is not yet loaded. This change may produce a warning if it
> fails to load pg_hba.conf in the backends.
>

This seems like the right strategy to me. It parallels pg_settings and
postgresql.conf which means one less surprising quirk for future developers.

An idle thought, in the long-term it seems like it would be better to have
postmaster have some shared memory where it dumps out a config data
structure that backends can all see. That might help with the
race-conditions we have now when reloading config data where different
backends could end up with different interpretations of the same config or
even see different configs if the files are being modified concurrently.
But I think that would have to be done very carefully so the postmaster
doesn't sacrifice any reliability.

--
greg

Greg,

* Greg Stark (stark(at)mit(dot)edu) wrote:
> On Mon, Mar 2, 2015 at 6:36 AM, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>
> wrote:
>
> > Loading pg_hba.conf during SIGHUP in the backends will solve the
> > problem of displaying the
> > data which is not yet loaded. This change may produce a warning if it
> > fails to load pg_hba.conf in the backends.
>
> This seems like the right strategy to me. It parallels pg_settings and
> postgresql.conf which means one less surprising quirk for future developers.

Agreed.

> An idle thought, in the long-term it seems like it would be better to have
> postmaster have some shared memory where it dumps out a config data
> structure that backends can all see. That might help with the
> race-conditions we have now when reloading config data where different
> backends could end up with different interpretations of the same config or
> even see different configs if the files are being modified concurrently.

I had considered this option also but threw it out almost as quickly as
it came to mind..

> But I think that would have to be done very carefully so the postmaster
> doesn't sacrifice any reliability.

Due to this. I'm not sure there's any trivial way to ensure that. I
suppose we could use mmap() and then mprotect() the region when we're
not reloading it in the postmaster, and make sure that it's PROT_READ
when we fork() as I *think* that would be preserved in the child,
until/unless the child changed it. That the child could change it is
certainly a concern, even if it requires a few extra steps, but it'd at
least be better than nothing, and if there's no code in the child which
ever calls mprotect, well, perhaps it'd be good enough.

Obviously, this would be a much larger change and we'd have to use a
lock-free structure to make sure that the children don't read something
inconsistent and avoid having to use a lock that could cause problems
for the postmaster.

Thanks!

Stephen

​So earlier someone commented that using lists list_nth() seemed odd and a
tuplestore might be better. In fact using lists this way is O(n^2). I've
done some quick tests and it doesn't start being a problem until about
10,000 lines which obviously isn't a terribly common way to use
pg_hba_settings. However we have in the past had people doing multi-tenant
clusters with hundreds or thousands of databases in a cluster complaining
about scalability of certain operations. It would be a shame to introduce a
new one.

It does seem annoying to use a tuplestore as IIRC the function scan node
also materializes the results in recent years. But at least it would scale
linearly.

On Mon, Mar 2, 2015 at 1:42 PM, Greg Stark <stark(at)mit(dot)edu> wrote:

> ​So earlier someone commented that using lists list_nth() seemed odd and a
> tuplestore might be better. In fact using lists this way is O(n^2). I've
> done some quick tests and it doesn't start being a problem until about
> 10,000 lines which obviously isn't a terribly common way to use
> pg_hba_settings. However we have in the past had people doing multi-tenant
> clusters with hundreds or thousands of databases in a cluster complaining
> about scalability of certain operations. It would be a shame to introduce a
> new one.
>
> It does seem annoying to use a tuplestore as IIRC the function scan node
> also materializes the results in recent years. But at least it would scale
> linearly.
>

So I didn't get the memo about SFRM_Materialize. Here's a rewrite of this
using that interface which seems to test ok up to 100k. At that point I
start running into memory errors on reading the HBA file so I guess that's
an indication that's large enough to stop worrying about it.

--
greg

On Mon, Mar 2, 2015 at 4:36 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
>
> So I didn't get the memo about SFRM_Materialize. Here's a rewrite of this using that interface which seems to test ok up to 100k. At that point I start running into memory errors on reading the HBA file so I guess that's an indication that's large enough to stop worrying about it.

Hm. I'm wondering why I'm getting out of memory errors now with just
25k lines in pg_hba.conf. It looks like the HbaLine struct is "only"
472 bytes so the list should only be occupying about 11MB. In fact
it's occupying about a gigabyte:

TopMemoryContext: 8192 total in 1 blocks; 5264 free (1 chunks); 2928 used
ident parser context: 0 total in 0 blocks; 0 free (0 chunks); 0 used
hba parser context: 956300288 total in 126 blocks; 1694136 free (82
chunks); 954606152 used

The lines in my pg_hba.conf don't have any databases or roles listed
so there shouldn't be any tokens taking up space either. I just copied
this line 25,000 times:
local all all trust

--
greg

Greg Stark wrote:

> Hm. I'm wondering why I'm getting out of memory errors now with just
> 25k lines in pg_hba.conf. It looks like the HbaLine struct is "only"
> 472 bytes so the list should only be occupying about 11MB. In fact
> it's occupying about a gigabyte:

Maybe it's leaking heavily while parsing ...

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

* Greg Stark (stark(at)mit(dot)edu) wrote:
> On Mon, Mar 2, 2015 at 4:36 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> >
> > So I didn't get the memo about SFRM_Materialize. Here's a rewrite of this using that interface which seems to test ok up to 100k. At that point I start running into memory errors on reading the HBA file so I guess that's an indication that's large enough to stop worrying about it.
>
> Hm. I'm wondering why I'm getting out of memory errors now with just
> 25k lines in pg_hba.conf. It looks like the HbaLine struct is "only"
> 472 bytes so the list should only be occupying about 11MB. In fact
> it's occupying about a gigabyte:

Uh, maybe because it's trying to allocate over 1GB and palloc() doesn't
support that?

Thanks,

Stephen

On Mon, Mar 2, 2015 at 7:42 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> Uh, maybe because it's trying to allocate over 1GB and palloc() doesn't
> support that?

Nobody's allocating anything that big. It's a list of 25,000 pointers
to 472-byte structs. That should add up to about 11MB. Instead the
memory context is a total of 954606152 bytes which is still under a
gigabyte and the database does start up. It drives my laptop to a
crawl and Autovacuum crashes when it tries to start but the postmaster
is mostly happy. That's about 38k per line or about 80x as much as it
should be taking.

The tokenizer is run in a separate memory context which gets reset
when this is done. Then during parsing it copies things from that
memory context over as needed to fill in the struct. In my example
pg_hba.c it shouldn't have to copy anything over though so even if
there's a bug there I don't see how it would cause this.

My best guess is that some functions called allocate some temporary
storage. I haven't spotted them yet though I put this aside when I
first sent my message. It seems like it would be safest to use a
temporary memory context throughout the whole process and only copy
things to the hba memory context manually.

--
greg

On Mon, Mar 2, 2015 at 7:51 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> Nobody's allocating anything that big. It's a list of 25,000 pointers
> to 472-byte structs. That should add up to about 11MB. Instead the
> memory context is a total of 954606152 bytes which is still under a
> gigabyte and the database does start up. It drives my laptop to a
> crawl and Autovacuum crashes when it tries to start but the postmaster
> is mostly happy. That's about 38k per line or about 80x as much as it
> should be taking.

Hm. Well it seems my laptop was just messed up from having run out of
memory. This seems to have affected both ext4fs and the shared memory
system in weird ways such that Postgres wasn't reading the new config
when I thought (and thought I had confirmed) it was.

Now that the filesystem is behaving properly Postgres seems to be
behaving more reasonably. It seems to be allocating between 1kB and
1.6kB per hba line including the rawline string, the list of databases
and roles which contain structs pointing to "all". That still seems
highish but not insane.

And now that my machine is behaving better here are the timings for
the new function using SFRM_Materialize:

::***# select count(*) from pg_hba_settings();
┌───────┐
│ count │
├───────┤
│ 10002 │
└───────┘
(1 row)

Time: 31.931 ms

...

::***# select count(*) from pg_hba_settings();
┌───────┐
│ count │
├───────┤
│ 80002 │
└───────┘
(1 row)

Time: 277.376 ms

And the total memory used for the 80k lines is about 83MB.
hba parser context: 83885056 total in 22 blocks; 1322232 free (13
chunks); 82562824 used

Using the n^2 approach it's:

::# select count(*) from pg_hba_settings;
┌───────┐
│ count │
├───────┤
│ 10002 │
└───────┘
(1 row)

Time: 595.397 ms

...

::***# select count(*) from pg_hba_settings;
┌───────┐
│ count │
├───────┤
│ 20002 │
└───────┘
(1 row)

Time: 2441.081 ms

--
greg

On Mon, Jun 30, 2014 at 8:06 AM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com> wrote:
> After sleeping on it, I realised that the code would return '{all}' for
> 'all' in pg_hba.conf, but '{"all"}' for '"all"'. So it's not exactly
> ambiguous, but I don't think it's especially useful for callers.

Hm. Nope, it doesn't. It just says {all} regardless of whether "all"
is quoted or not.

This makes sense, the rules for when to quote things in pg_hba and
when to quote things for arrays are separate. And we definitely don't
want to start adding quotes to every token that the parser noted was
quoted because then you'll start seeing things like {"\"all\""} and
{"\"database with space\""} which won't make it any easier to match
things.

I'm not sure adding a separate column is really the solution either.
You can have things like all,databasename (which is the keyword "all"
not a database "all). Or more likely something like sameuser,bob or
even things like replication,all.

I'm thinking leaving well enough alone is probably best. It's not
perfect but if a user does have a database named "all" or
"replication" or a user named "sameuser" or "samerole" then it's not
like pg_hba_settings crashes or anything, it just produces information
that's hard to interpret and the response might just be "don't do
that".

The only other option I was pondering was using a jsonb instead of an
array. That would give us more flexibility and we could have a json
array that contained strings and objects representing keywords. But
most hba files consist *mostly* of singleton keywords, so the result
might be kind of cumbersome.

--
greg

On 3/3/15 9:08 AM, Greg Stark wrote:
> On Mon, Jun 30, 2014 at 8:06 AM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com> wrote:
>> After sleeping on it, I realised that the code would return '{all}' for
>> 'all' in pg_hba.conf, but '{"all"}' for '"all"'. So it's not exactly
>> ambiguous, but I don't think it's especially useful for callers.
>
> Hm. Nope, it doesn't. It just says {all} regardless of whether "all"
> is quoted or not.
>
> This makes sense, the rules for when to quote things in pg_hba and
> when to quote things for arrays are separate. And we definitely don't
> want to start adding quotes to every token that the parser noted was
> quoted because then you'll start seeing things like {"\"all\""} and
> {"\"database with space\""} which won't make it any easier to match
> things.
>
> I'm not sure adding a separate column is really the solution either.
> You can have things like all,databasename (which is the keyword "all"
> not a database "all). Or more likely something like sameuser,bob or
> even things like replication,all.

What about a separate column that's just the text from pg_hba? Or is
that what you're opposed to?

FWIW, I'd say that having the individual array elements be correct is
more important than what the result of array_out is. That way you could
always do array_to_string(..., ', ') and get valid pg_hba output.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Tue, Mar 3, 2015 at 6:05 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:

> What about a separate column that's just the text from pg_hba? Or is that what you're opposed to?

I'm not sure what you mean by that. There's a rawline field we could
put somewhere but it contains the entire line.

> FWIW, I'd say that having the individual array elements be correct is more
> important than what the result of array_out is. That way you could always do
> array_to_string(..., ', ') and get valid pg_hba output.

Well I don't think you can get that without making the view less
useful for every other purpose.

Like, I would want to be able to do WHERE "user" @> array[?] or WHERE
database = array[?] or to join against a list of users or databases
somewhere else.

To do what you suggest would mean the tokens will need to be quoted
based on pg_hba.conf syntax requirements. That would mean I would need
to check each variable or join value against pg_hba.conf's quoting
requirements to compare with it. It seems more practical to have that
knowledge if you're actually going to generate a pg_hba.conf than to
pass around these quoted strings all the time.

On further review I've made a few more changes attached.

I think we should change the column names to "users" and "databases"
to be clear they're lists and also to avoid the "user" SQL reserved
word.

I removed the dependency on strlist_to_array which is in
objectaddress.c which isn't a very sensible dependency -- it does seem
like it would be handy to have a list-based version of construct_array
moved to arrayfuncs.c but for now it's not much more work to handle
these ourselves.

I changed the options to accumulate one big array instead of an array
of bunches of options. Previously you could only end up with a
singleton array with a comma-delimited string or a two element array
with one of those and map=.

I think the error if pg_hba fails to reload needs to be LOG. It would
be too unexpected to the user who isn't necessarily the one who issued
the SIGHUP to spontaneously get a warning.

I also removed the "mask" from local entries and made some of the
NULLS that shouldn't be possible to happen (unknown auth method or
connection method) actually throw errors.

--
greg

On 3/3/15 12:57 PM, Greg Stark wrote:
> On Tue, Mar 3, 2015 at 6:05 PM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>
>> What about a separate column that's just the text from pg_hba? Or is that what you're opposed to?
>
> I'm not sure what you mean by that. There's a rawline field we could
> put somewhere but it contains the entire line.

I mean have a field for each of user/databases that gives you valid
pg_hba.conf output. That would allow for cut & paste. But perhaps that's
just not a use case.

>> FWIW, I'd say that having the individual array elements be correct is more
>> important than what the result of array_out is. That way you could always do
>> array_to_string(..., ', ') and get valid pg_hba output.
>
> Well I don't think you can get that without making the view less
> useful for every other purpose.
>
> Like, I would want to be able to do WHERE "user" @> array[?] or WHERE
> database = array[?] or to join against a list of users or databases
> somewhere else.

I think we're screwed in that regard anyway, because of the special
constructs. You'd need different logic to handle things like +role and
sameuser. We might even end up painted in a corner where we can't change
it in the future because it'll break everyone's scripts.

> To do what you suggest would mean the tokens will need to be quoted
> based on pg_hba.conf syntax requirements. That would mean I would need
> to check each variable or join value against pg_hba.conf's quoting
> requirements to compare with it. It seems more practical to have that
> knowledge if you're actually going to generate a pg_hba.conf than to
> pass around these quoted strings all the time.

What about this:

- database_specials enum[] contains all occurrences of all, sameuser,
samerole and replication (or maybe it doesn't need to be an array)
- in_roles name[] is a list of all cases of +role_name, with the +
stripped off (I think the @ case is already handled before the SRF is
called??)
- role_specials enum[] handles all (enum[] for any future expansion)

Alternatively in_roles could just be combined with role_specials as long
as we preserve the +.

Any tokens that match the special conditions do not show up in
databases/users, and those fields become name[]. AFAIK that means the
quoting should be identical (at least looking at check_db() and
check_role()).

I can make these changes if you want.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com

On Wed, Mar 4, 2015 at 5:57 AM, Greg Stark <stark(at)mit(dot)edu> wrote:
> On further review I've made a few more changes attached.
>
> I think we should change the column names to "users" and "databases"
> to be clear they're lists and also to avoid the "user" SQL reserved
> word.
>
> I removed the dependency on strlist_to_array which is in
> objectaddress.c which isn't a very sensible dependency -- it does seem
> like it would be handy to have a list-based version of construct_array
> moved to arrayfuncs.c but for now it's not much more work to handle
> these ourselves.
>
> I changed the options to accumulate one big array instead of an array
> of bunches of options. Previously you could only end up with a
> singleton array with a comma-delimited string or a two element array
> with one of those and map=.

The changes are fine, this eliminates the unnecessary dependency on
objectaddress.

> I think the error if pg_hba fails to reload needs to be LOG. It would
> be too unexpected to the user who isn't necessarily the one who issued
> the SIGHUP to spontaneously get a warning.
>
> I also removed the "mask" from local entries and made some of the
> NULLS that shouldn't be possible to happen (unknown auth method or
> connection method) actually throw errors.

changes are fine. All the tests are passed.

Out of curiosity, regarding the result materialize code addition, Any
way the caller of "hba_settings" function
"ExecMakeTableFunctionResult" also stores the results in tuple_store.
Is there any advantage
doing it in hba_settings function rather than in the caller?

Regards,
Hari Babu
Fujitsu Australia

On Wed, Mar 4, 2015 at 12:17 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
> I can make these changes if you want.

Personally I'm just not convinced this is worth it. It makes the
catalogs harder for people to read and use and only benefits people
who have users named "all" or databases named "all", "sameuser", or
"samerole" which I can't really imagine would be anyone.

If this were going to be the infrastructure on which lots of tools
rested rather than just a read-only view that was mostly going to be
read by humans that might be different. Are you envisioning a tool
that would look at this view, offer a gui for users to make changes
based on that information, and build a new pg_hba.conf to replace it?

--
greg

On Wed, Mar 4, 2015 at 12:34 AM, Haribabu Kommi
<kommi(dot)haribabu(at)gmail(dot)com> wrote:
> Out of curiosity, regarding the result materialize code addition, Any
> way the caller of "hba_settings" function
> "ExecMakeTableFunctionResult" also stores the results in tuple_store.
> Is there any advantage
> doing it in hba_settings function rather than in the caller?

That might protect against concurrent pg_hba reloads, though I suspect
there's a CHECK_FOR_INTERRUPTS hanging out in that loop. We could
maybe put one in this loop and check if the parser memory context has
been reset.

But the immediate problem is that having the API that looked up a line
by line number and then calling it repeatedly with successive line
numbers was O(n^2). Each time it was called it had to walk down the
list from the head all over again. 1 + 2 + 3 + 4 + ... n = n(n+1)/2 or
O(n^2). This isn't performance critical but it would not have run in a
reasonable length of time for large pg_hba files.

And having to store the state between calls means the code is at least
as simple for the tuplestore, imho even simpler.

--
greg

On Wed, Mar 4, 2015 at 12:18 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> On Wed, Mar 4, 2015 at 12:34 AM, Haribabu Kommi
> <kommi(dot)haribabu(at)gmail(dot)com> wrote:
>> Out of curiosity, regarding the result materialize code addition, Any
>> way the caller of "hba_settings" function
>> "ExecMakeTableFunctionResult" also stores the results in tuple_store.
>> Is there any advantage
>> doing it in hba_settings function rather than in the caller?
>
> That might protect against concurrent pg_hba reloads, though I suspect
> there's a CHECK_FOR_INTERRUPTS hanging out in that loop. We could
> maybe put one in this loop and check if the parser memory context has
> been reset.

I feel there is no problem of current pg_hba reloads, because the
check_for_interrupts
doesn't reload the conf files. It will be done in the "postgresMain"
function once the
query finishes. Am I missing something?

+ foreach(line, parsed_hba_lines)

In the above for loop it is better to add "check_for_interrupts" to
avoid it looping
if the parsed_hba_lines are more.

> But the immediate problem is that having the API that looked up a line
> by line number and then calling it repeatedly with successive line
> numbers was O(n^2). Each time it was called it had to walk down the
> list from the head all over again. 1 + 2 + 3 + 4 + ... n = n(n+1)/2 or
> O(n^2). This isn't performance critical but it would not have run in a
> reasonable length of time for large pg_hba files.
>
> And having to store the state between calls means the code is at least
> as simple for the tuplestore, imho even simpler.

Got it. Thanks.

Regards,
Hari Babu
Fujitsu Australia

On 03/03/2015 05:07 PM, Greg Stark wrote:
> On Wed, Mar 4, 2015 at 12:17 AM, Jim Nasby <Jim(dot)Nasby(at)bluetreble(dot)com> wrote:
>> I can make these changes if you want.
>
> Personally I'm just not convinced this is worth it. It makes the
> catalogs harder for people to read and use and only benefits people
> who have users named "all" or databases named "all", "sameuser", or
> "samerole" which I can't really imagine would be anyone.
>
> If this were going to be the infrastructure on which lots of tools
> rested rather than just a read-only view that was mostly going to be
> read by humans that might be different. Are you envisioning a tool
> that would look at this view, offer a gui for users to make changes
> based on that information, and build a new pg_hba.conf to replace it?

I'm planning to write such a tool. However, I am not concerned about
weird name cases like the above.

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi
<kommi(dot)haribabu(at)gmail(dot)com> wrote:
> + foreach(line, parsed_hba_lines)
>
> In the above for loop it is better to add "check_for_interrupts" to
> avoid it looping
> if the parsed_hba_lines are more.

Updated patch is attached with the addition of check_for_interrupts in
the for loop.

Regards,
Hari Babu
Fujitsu Australia

On Wed, Mar 4, 2015 at 1:35 AM, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> wrote:
> I feel there is no problem of current pg_hba reloads, because the
> check_for_interrupts
> doesn't reload the conf files. It will be done in the "postgresMain"
> function once the
> query finishes. Am I missing something?

Ah, no I guess that's right. I even noticed that earlier but forgot.

--
greg

On 3/3/15 7:17 PM, Jim Nasby wrote:
> I think we're screwed in that regard anyway, because of the special
> constructs. You'd need different logic to handle things like +role and
> sameuser. We might even end up painted in a corner where we can't change
> it in the future because it'll break everyone's scripts.

Yeah, I'm getting worried about this. I think most people agree that
getting a peek at pg_hba.conf from within the server is useful, but
everyone seems to have quite different uses for it. Greg wants to join
against other catalog tables, Jim wants to reassemble a valid and
accurate pg_hba.conf, Josh wants to write an editing tool. Personally,
I'd like to see something as close to the actual file as possible.

If there were an obviously correct way to map the various special
constructs to the available SQL data types, then fine. But if there
isn't, then we shouldn't give a false overinterpretation. So I'd render
everything that's disputed as a plain text field. (Not even an array of
text.)

2015-03-04 22:41 GMT+01:00 Peter Eisentraut <peter_e(at)gmx(dot)net>:

> On 3/3/15 7:17 PM, Jim Nasby wrote:
> > I think we're screwed in that regard anyway, because of the special
> > constructs. You'd need different logic to handle things like +role and
> > sameuser. We might even end up painted in a corner where we can't change
> > it in the future because it'll break everyone's scripts.
>
> Yeah, I'm getting worried about this. I think most people agree that
> getting a peek at pg_hba.conf from within the server is useful, but
> everyone seems to have quite different uses for it. Greg wants to join
> against other catalog tables, Jim wants to reassemble a valid and
> accurate pg_hba.conf, Josh wants to write an editing tool. Personally,
> I'd like to see something as close to the actual file as possible.
>
> If there were an obviously correct way to map the various special
> constructs to the available SQL data types, then fine. But if there
> isn't, then we shouldn't give a false overinterpretation. So I'd render
> everything that's disputed as a plain text field. (Not even an array of
> text.)
>

I disagree with last note - arrays where is expected are valid. I don't see
any reason why anybody have to do parsing some informations from any table.

The face of pg_hba.conf in SQL space can be different than original file -
but all data should be clean (without necessity of additional parsing)

Regards

Pavel

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

On Wed, Mar 4, 2015 at 9:41 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> everyone seems to have quite different uses for it. Greg wants to join
> against other catalog tables, Jim wants to reassemble a valid and
> accurate pg_hba.conf, Josh wants to write an editing tool. Personally,
> I'd like to see something as close to the actual file as possible.

Well if you want to read the file as is you can do so using the file
reading functions which afaik were specifically intended for the
purpose of writing config editing tools.

Just to repeat, even if you're reassembling a valid and accurage
pg_hba.conf or writing an editing tool you can do that with what we
have today as long as there are no databases named "all", "sameuser",
or "samerole" and no roles named "all". That's something an editing
tool could check and provide a warning for to the user and refuse to
write a config file if it's the case and I doubt there are any such
users out there anyways.

Having five separate columns would work but I just think it's way more
complicated than necessary and burdens everyone who wants to use the
view less formally.

>
> If there were an obviously correct way to map the various special
> constructs to the available SQL data types, then fine. But if there
> isn't, then we shouldn't give a false overinterpretation. So I'd render
> everything that's disputed as a plain text field. (Not even an array of
> text.)

Joining against other catalog tables may be kind of exaggerated but if
I have data in an SQL view I certainly expect to be able to write
WHERE clauses to find the rows I want without having to do string
parsing. If it were a comma-delimited string I have to do something
like WHERE users LIKE '%,username,%' but then that's not good enough
since it may be the first or last and the username itself may contain
white-space or a comma etc etc.

I think knowing about the + prefix and the risk of tokens like "all"
and "sameuser" etc are pretty small compromises to make. But having to
know the quoting rules and write a tokenizer are too far.

--
greg

On 3/5/15 9:42 AM, Greg Stark wrote:
> Well if you want to read the file as is you can do so using the file
> reading functions which afaik were specifically intended for the
> purpose of writing config editing tools.

Sure, but those things are almost never installed by default, and I
don't want to install them if they provide easy write access, and they
don't help you find the actual file. This proposal is much nicer.

> Joining against other catalog tables may be kind of exaggerated but if
> I have data in an SQL view I certainly expect to be able to write
> WHERE clauses to find the rows I want without having to do string
> parsing. If it were a comma-delimited string I have to do something
> like WHERE users LIKE '%,username,%' but then that's not good enough
> since it may be the first or last and the username itself may contain
> white-space or a comma etc etc.

The point is, it should be one or the other (or both), not something in
the middle.

It's either a textual representation of the file or a semantic one. If
it's the latter, then all user names, group names, and special key words
need to be resolved in some way that they cannot be confused with
unfortunately named user names. And there needs to be a way that we can
add more in the future.

On Fri, Mar 6, 2015 at 3:11 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> The point is, it should be one or the other (or both), not something in
> the middle.
>
> It's either a textual representation of the file or a semantic one. If
> it's the latter, then all user names, group names, and special key words
> need to be resolved in some way that they cannot be confused with
> unfortunately named user names. And there needs to be a way that we can
> add more in the future.
>

I think what we have here is already a good semantic representation. It
doesn't handle all the corner cases but those corner cases are a) very
unlikely and b) easy to check for. A tool can check for any users starting
with + or named "all" or any databases called "sameuser" or "samerole". If
they exist then the view isn't good enough to reconstruct the raw file. But
they're very unlikely to exist, I've never heard of anyone with such things
and can't imagine why someone would make them.

The view as is is useful for everyone. Someone who wants to write a tool
can reconstruct the file as long as none of the problematic names exist in
the database. Someone who wants to run audits can run queries to see what
users have access to or check the permissions on a database and do simple
comparisons to check for whatever they want to check for.

If we change it to be 100% unambiguous then it will be cumbersome to work
with. The view will have 5 columns instead of 2 dedicated to users and
databases. It will be much harder for humans to read, and for people
writing code they'll have to check three columns for matches. It just seems
like a lot of busywork to make it less useful.

Attached are two copies of the view, one as is today and one with the
keywords and recursive elements broken out.

Also, fwiw as far as adding more in the future -- breaking the keywords out
into a separate column wouldn't interfere with that but if we want to
include any additional syntax like + then it will require additional
columns. That would actually be less flexible for future changes.

I'm inclined to commit this as is.

--
greg

On 3/4/15 1:34 AM, Haribabu Kommi wrote:
> On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi
> <kommi(dot)haribabu(at)gmail(dot)com> wrote:
>> + foreach(line, parsed_hba_lines)
>>
>> In the above for loop it is better to add "check_for_interrupts" to
>> avoid it looping
>> if the parsed_hba_lines are more.
>
> Updated patch is attached with the addition of check_for_interrupts in
> the for loop.

I tried out your latest patch. I like that it updates even in running
sessions when the file is reloaded.

The permission checking is faulty, because unprivileged users can
execute pg_hba_settings() directly.

Check the error messages against the style guide (especially
capitalization).

I don't like that there is a hard-coded limit of 16 options 5 pages away
from where it is actually used. That could be done better.

I'm not sure about the name "pg_hba_settings". Why not just "pg_hba" or
"pg_hba_conf" if you're going for a representation that is close to the
file (as opposed to pg_settings, which is really a lot more abstract
than any particular file).

I would put the line_number column first.

I continue to think that it is a serious mistake to stick special values
like 'all' and 'replication' into the arrays without additional
decoration. That makes this view useless or at least dangerous for
editing tools or tools that want to reassemble the original file.
Clearly at least one of those has to be a use case. Otherwise we can
just print out the physical lines without interpretation.

The "mask" field can go, because address is of type inet, which can
represent masks itself. (Or should it be cidr then? Who knows.) The
preferred visual representation of masks in pg_hba.conf has been
"address/mask" for a while now, so we should preserve that.
Additionally, you can then use the existing inet/cidr operations to do
things like checking whether some IP address is contained in an address
specification.

I can't tell from the documentation what the "compare_method" field is
supposed to do. I see it on the code, but that is not a natural
representation of pg_hba.conf. In fact, this just supports my earlier
statement. Why are special values in the address field special, but not
in the user or database fields?

uaImplicitReject is not a user-facing authentication method, so it
shouldn't be shown (or showable).

I would have expected options to be split into keys and values.

All that code to reassemble the options from the parsed struct
representation seems crazy to me. Surely, we could save the strings as
we parse them?

I can't make sense of the log message "pg_hba.conf not reloaded,
pg_hba_settings may show stale information". If load_hba() failed, the
information isn't stale, is it?

In any case, printing this to the server log seems kind of pointless,
because someone using the view is presumably doing so because they can't
or don't want to read the server log. The proper place might be a
warning when the view is actually called.

On Wed, Mar 11, 2015 at 1:32 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> I think what we have here is already a good semantic representation. It
> doesn't handle all the corner cases but those corner cases are a) very
> unlikely and b) easy to check for. A tool can check for any users starting
> with + or named "all" or any databases called "sameuser" or "samerole". If
> they exist then the view isn't good enough to reconstruct the raw file. But
> they're very unlikely to exist, I've never heard of anyone with such things
> and can't imagine why someone would make them.

-1. Like Peter, I think this is a bad plan. Somebody looking at the
view should be able to understand with 100% confidence, and without
additional parsing, what the semantics of the pg_hba.conf file are.
Saying "those cases are unlikely so we're not going to handle them" is
really selling ourselves short.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas wrote:
> On Wed, Mar 11, 2015 at 1:32 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> > I think what we have here is already a good semantic representation. It
> > doesn't handle all the corner cases but those corner cases are a) very
> > unlikely and b) easy to check for. A tool can check for any users starting
> > with + or named "all" or any databases called "sameuser" or "samerole". If
> > they exist then the view isn't good enough to reconstruct the raw file. But
> > they're very unlikely to exist, I've never heard of anyone with such things
> > and can't imagine why someone would make them.
>
> -1. Like Peter, I think this is a bad plan. Somebody looking at the
> view should be able to understand with 100% confidence, and without
> additional parsing, what the semantics of the pg_hba.conf file are.
> Saying "those cases are unlikely so we're not going to handle them" is
> really selling ourselves short.

+1 what Robert said. I think the additional "keyword" columns are a
good solution to the issue.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Mar 16, 2015 at 4:29 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> +1 what Robert said. I think the additional "keyword" columns are a
> good solution to the issue.
>

Huh. Well I disagree but obviously I'm in the minority. I'll put fix it up
accordingly today and post the resulting view output (which I expect will
look like the example in the previous message). I think it's cumbersome but
it shouldn't be hard to implement.

--
greg

On Mon, Mar 16, 2015 at 9:29 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
wrote:

> Robert Haas wrote:
> > On Wed, Mar 11, 2015 at 1:32 PM, Greg Stark <stark(at)mit(dot)edu> wrote:
> > > I think what we have here is already a good semantic representation. It
> > > doesn't handle all the corner cases but those corner cases are a) very
> > > unlikely and b) easy to check for. A tool can check for any users
> starting
> > > with + or named "all" or any databases called "sameuser" or
> "samerole". If
> > > they exist then the view isn't good enough to reconstruct the raw
> file. But
> > > they're very unlikely to exist, I've never heard of anyone with such
> things
> > > and can't imagine why someone would make them.
> >
> > -1. Like Peter, I think this is a bad plan. Somebody looking at the
> > view should be able to understand with 100% confidence, and without
> > additional parsing, what the semantics of the pg_hba.conf file are.
> > Saying "those cases are unlikely so we're not going to handle them" is
> > really selling ourselves short.
>
> +1 what Robert said. I think the additional "keyword" columns are a
> good solution to the issue.
>
>
​Why not just leave the double-quoting requirements intact. An unquoted
<any> or <sameuser> (etc) would represent the special keyword while the
quoted version would mean that the name is used literally.

​Have the: "​A separate file containing [database|user] names can be
specified by preceding the file name with @" possibilities been added to
the test suite?

I'm not totally opposed to using some other quoting symbol to represent
keywords as well - like "*" (e.g., *all*). Like Greg, I'm not to keen on
the idea of adding additional keyword columns.

*Logical View - Keyword Expansion*

​My other thought was to not use the keywords at all and simply resolve
their meaning​ to actual role/database names and list them explicitly.
That would be a true logical view and allow for simple user checking by
saying: 'username' = ANY(users); without the need for ([...] OR '*any*' =
ANY(users) or similar). If going that route I guess it would behoove us to
either incorporate a "physical" view of the actual file converted to a
table or to then maybe have some kind of "tags" fields with the special
values encoded should someone want to know how the user list was
generated. In effect, the "pg_hba" view in the original file but with
actual names of users and databases instead of empty arrays.

The "sameuser" => "all" pairing is a pure physical representation in that
instance. What I would do in a logical representation is have a single
record for each user that has a database of the same name in the current
database. However, because of that limitation you either would be
duplicating information by keeping "sameuser,all" or you would have to have
a separate view representing the physical view of the file. I think having
two views, one logical and one physical, would solve the two use cases
nicely without having to compromise or incorporate too much redundancy and
complexity.

Likewise, if we know the host ip address and subnet mask the keywords
"samehost" and "samenet" should be resolvable to actual values at the time
of viewing. Though that would add the complication of multi-network
listening...

I guess a full logical view is a bit much but if we keep the same quoting
mechanics as mandated by the file then there should be no ambiguity in
terms of label meaning and whomever is looking at this stuff has to
understand about the keywords and using quote to remove the special-ness
anyway.

David J.

On Mon, Mar 16, 2015 at 1:46 PM, David G. Johnston
<david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
> Why not just leave the double-quoting requirements intact. An unquoted
> <any> or <sameuser> (etc) would represent the special keyword while the
> quoted version would mean that the name is used literally.

That would be OK with me, I think.

> I'm not totally opposed to using some other quoting symbol to represent
> keywords as well - like "*" (e.g., *all*). Like Greg, I'm not to keen on
> the idea of adding additional keyword columns.

We probably should not use one quoting convention in the file and
another in the view.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Mar 16, 2015 at 5:46 PM, David G. Johnston <
david(dot)g(dot)johnston(at)gmail(dot)com> wrote:

> ​Why not just leave the double-quoting requirements intact. An unquoted
> <any> or <sameuser> (etc) would represent the special keyword while the
> quoted version would mean that the name is used literally.
>

For users that would be worse than not quoting. Then if they look up users
they can't say WHERE username =ANY (users). They would have to do
sumersaults like CASE WHEN username = 'all' then '"all"' =ANY (users) else
username =ALL (users).

The whole point of having a view should be that you don't need to know the
syntax rules for pg_hba.conf to interpret the data. If you do then you
might as well just write a parser and read the file.

--
greg

On Mon, Mar 16, 2015 at 11:11 AM, Greg Stark <stark(at)mit(dot)edu> wrote:

>
> On Mon, Mar 16, 2015 at 5:46 PM, David G. Johnston <
> david(dot)g(dot)johnston(at)gmail(dot)com> wrote:
>
>> ​Why not just leave the double-quoting requirements intact. An unquoted
>> <any> or <sameuser> (etc) would represent the special keyword while the
>> quoted version would mean that the name is used literally.
>>
>
> For users that would be worse than not quoting. Then if they look up users
> they can't say WHERE username =ANY (users). They would have to do
> sumersaults like CASE WHEN username = 'all' then '"all"' =ANY (users) else
> username =ALL (users).
>
> The whole point of having a view should be that you don't need to know the
> syntax rules for pg_hba.conf to interpret the data. If you do then you
> might as well just write a parser and read the file.
>
>
>
​Create a "pg_hba_user" type, and an implicit cast from text to that type,
so when you say: "WHERE 'any' = ANY(...)" the system does the syntax
conversion for you and the user doesn't have to, for the most part, be
aware of the special rules for quoting names. Otherwise I don't see how
you can meet the requirement to accommodate "any" as a valid user
identifier​

​without using two columns - one for keywords and one for users using the
quoting rules of PostgreSQL pg_role instead of using the, more restrictive,
rules of pg_hba.conf

​​
​
​In that case I would not leave the users column with an empty array when
"any" is specified but would incorporate all known roles into the value;
though maybe it is just noise during typical usage...it would likely be a
long field that would be useful for querying but not necessarily for
display.

​David J.​
​

On Fri, Mar 13, 2015 at 1:33 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 3/4/15 1:34 AM, Haribabu Kommi wrote:
>> On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi
>> <kommi(dot)haribabu(at)gmail(dot)com> wrote:
>>> + foreach(line, parsed_hba_lines)
>>>
>>> In the above for loop it is better to add "check_for_interrupts" to
>>> avoid it looping
>>> if the parsed_hba_lines are more.
>>
>> Updated patch is attached with the addition of check_for_interrupts in
>> the for loop.
>
> I tried out your latest patch. I like that it updates even in running
> sessions when the file is reloaded.

Thanks for the review. Sorry for late reply.

> The permission checking is faulty, because unprivileged users can
> execute pg_hba_settings() directly.

corrected.

> Check the error messages against the style guide (especially
> capitalization).

corrected.

> I don't like that there is a hard-coded limit of 16 options 5 pages away
> from where it is actually used. That could be done better.

changed to 12 instead of 16.

> I'm not sure about the name "pg_hba_settings". Why not just "pg_hba" or
> "pg_hba_conf" if you're going for a representation that is close to the
> file (as opposed to pg_settings, which is really a lot more abstract
> than any particular file).

changed to pg_hba_conf.

> I would put the line_number column first.

changed.

> I continue to think that it is a serious mistake to stick special values
> like 'all' and 'replication' into the arrays without additional
> decoration. That makes this view useless or at least dangerous for
> editing tools or tools that want to reassemble the original file.
> Clearly at least one of those has to be a use case. Otherwise we can
> just print out the physical lines without interpretation.

It is possible to provide more than one keyword for databases or users.
Is it fine to use the text array for keyword databases and keyword users.

> The "mask" field can go, because address is of type inet, which can
> represent masks itself. (Or should it be cidr then? Who knows.) The
> preferred visual representation of masks in pg_hba.conf has been
> "address/mask" for a while now, so we should preserve that.
> Additionally, you can then use the existing inet/cidr operations to do
> things like checking whether some IP address is contained in an address
> specification.

removed.

> I can't tell from the documentation what the "compare_method" field is
> supposed to do. I see it on the code, but that is not a natural
> representation of pg_hba.conf. In fact, this just supports my earlier
> statement. Why are special values in the address field special, but not
> in the user or database fields?
>
> uaImplicitReject is not a user-facing authentication method, so it
> shouldn't be shown (or showable).

removed.

> I would have expected options to be split into keys and values.
>
> All that code to reassemble the options from the parsed struct
> representation seems crazy to me. Surely, we could save the strings as
> we parse them?

I didn't get this point clearly. Can you explain it a bit more.

> I can't make sense of the log message "pg_hba.conf not reloaded,
> pg_hba_settings may show stale information". If load_hba() failed, the
> information isn't stale, is it?
>
> In any case, printing this to the server log seems kind of pointless,
> because someone using the view is presumably doing so because they can't
> or don't want to read the server log. The proper place might be a
> warning when the view is actually called.

Changed accordingly as if the reload fails, further selects on the
view through a warning
as view data may not be proper like below.

"There was some failure in reloading pg_hba.conf file. The pg_hba.conf
settings data may contains stale information"

Here I attached latest patch with the fixes other than keyword columns.
I will provide the patch with keyword columns and documentation changes later.

Regards,
Hari Babu
Fujitsu Australia

Hi

I checked this patch. I like the functionality and behave.

There is minor issue with outdated regress test

test rules ... FAILED

I have no objections.

Regards

Pavel

2015-03-27 9:23 GMT+01:00 Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>:

> On Fri, Mar 13, 2015 at 1:33 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > On 3/4/15 1:34 AM, Haribabu Kommi wrote:
> >> On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi
> >> <kommi(dot)haribabu(at)gmail(dot)com> wrote:
> >>> + foreach(line, parsed_hba_lines)
> >>>
> >>> In the above for loop it is better to add "check_for_interrupts" to
> >>> avoid it looping
> >>> if the parsed_hba_lines are more.
> >>
> >> Updated patch is attached with the addition of check_for_interrupts in
> >> the for loop.
> >
> > I tried out your latest patch. I like that it updates even in running
> > sessions when the file is reloaded.
>
> Thanks for the review. Sorry for late reply.
>
> > The permission checking is faulty, because unprivileged users can
> > execute pg_hba_settings() directly.
>
> corrected.
>
> > Check the error messages against the style guide (especially
> > capitalization).
>
> corrected.
>
> > I don't like that there is a hard-coded limit of 16 options 5 pages away
> > from where it is actually used. That could be done better.
>
> changed to 12 instead of 16.
>
> > I'm not sure about the name "pg_hba_settings". Why not just "pg_hba" or
> > "pg_hba_conf" if you're going for a representation that is close to the
> > file (as opposed to pg_settings, which is really a lot more abstract
> > than any particular file).
>
> changed to pg_hba_conf.
>
> > I would put the line_number column first.
>
> changed.
>
> > I continue to think that it is a serious mistake to stick special values
> > like 'all' and 'replication' into the arrays without additional
> > decoration. That makes this view useless or at least dangerous for
> > editing tools or tools that want to reassemble the original file.
> > Clearly at least one of those has to be a use case. Otherwise we can
> > just print out the physical lines without interpretation.
>
> It is possible to provide more than one keyword for databases or users.
> Is it fine to use the text array for keyword databases and keyword users.
>
> > The "mask" field can go, because address is of type inet, which can
> > represent masks itself. (Or should it be cidr then? Who knows.) The
> > preferred visual representation of masks in pg_hba.conf has been
> > "address/mask" for a while now, so we should preserve that.
> > Additionally, you can then use the existing inet/cidr operations to do
> > things like checking whether some IP address is contained in an address
> > specification.
>
> removed.
>
> > I can't tell from the documentation what the "compare_method" field is
> > supposed to do. I see it on the code, but that is not a natural
> > representation of pg_hba.conf. In fact, this just supports my earlier
> > statement. Why are special values in the address field special, but not
> > in the user or database fields?
> >
> > uaImplicitReject is not a user-facing authentication method, so it
> > shouldn't be shown (or showable).
>
> removed.
>
> > I would have expected options to be split into keys and values.
> >
> > All that code to reassemble the options from the parsed struct
> > representation seems crazy to me. Surely, we could save the strings as
> > we parse them?
>
> I didn't get this point clearly. Can you explain it a bit more.
>
> > I can't make sense of the log message "pg_hba.conf not reloaded,
> > pg_hba_settings may show stale information". If load_hba() failed, the
> > information isn't stale, is it?
> >
> > In any case, printing this to the server log seems kind of pointless,
> > because someone using the view is presumably doing so because they can't
> > or don't want to read the server log. The proper place might be a
> > warning when the view is actually called.
>
> Changed accordingly as if the reload fails, further selects on the
> view through a warning
> as view data may not be proper like below.
>
> "There was some failure in reloading pg_hba.conf file. The pg_hba.conf
> settings data may contains stale information"
>
> Here I attached latest patch with the fixes other than keyword columns.
> I will provide the patch with keyword columns and documentation changes
> later.
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>
>

On Mon, Mar 30, 2015 at 4:34 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hi
>
> I checked this patch. I like the functionality and behave.

Thanks for the review.

Here I attached updated patch with the following changes.

1. Addition of two new keyword columns

keyword_databases - The database name can be "all", "replication",
sameuser", "samerole" and "samegroup".
keyword_roles - The role can be "all" and a group name prefixed with "+".

The rest of the database and role names are treated as normal database
and role names.

2. Added the code changes to identify the names with quoted.

3. Updated documentation changes

4. Regression test is corrected.

Regards,
Hari Babu
Fujitsu Australia

Hi

2015-03-31 14:38 GMT+02:00 Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>:

> On Mon, Mar 30, 2015 at 4:34 AM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> > Hi
> >
> > I checked this patch. I like the functionality and behave.
>
> Thanks for the review.
>
> Here I attached updated patch with the following changes.
>
> 1. Addition of two new keyword columns

> keyword_databases - The database name can be "all", "replication",
> sameuser", "samerole" and "samegroup".
> keyword_roles - The role can be "all" and a group name prefixed with "+".
>

I am not very happy about name - but I have no better idea :( - maybe
"database_mask", "user_mask"

>
> The rest of the database and role names are treated as normal database
> and role names.
>
> 2. Added the code changes to identify the names with quoted.
>

Is it a good idea? Database's names are not quoted every else (in system
catalog). So now, we cannot to use join to this view.

postgres=# select (databases)[1] from pg_hba_conf ;
databases
-----------
"omega 2"

(4 rows)

postgres=# select datname from pg_database ;
datname
-----------
template1
template0
postgres
omega 2
(4 rows)

I dislike this - we know, so the name must be quoted in file (without it,
the file was incorrect). And if you need quotes, there is a function
quote_ident. If we use quotes elsewhere, then it should be ok, bot not now.
Please, remove it. More, it is not necessary, when you use a "keyword"
columns.

Regards

Pavel

>
> 3. Updated documentation changes
>
> 4. Regression test is corrected.
>
>
> Regards,
> Hari Babu
> Fujitsu Australia
>

On Sat, Apr 4, 2015 at 4:19 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> Hi
>
> 2015-03-31 14:38 GMT+02:00 Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>:
>>
>> keyword_databases - The database name can be "all", "replication",
>> sameuser", "samerole" and "samegroup".
>> keyword_roles - The role can be "all" and a group name prefixed with "+".
>
>
> I am not very happy about name - but I have no better idea :( - maybe
> "database_mask", "user_mask"
>

How about database_keywords and user_keywords as column names?

>>
>>
>> The rest of the database and role names are treated as normal database
>> and role names.
>>
>> 2. Added the code changes to identify the names with quoted.
>
>
> Is it a good idea? Database's names are not quoted every else (in system
> catalog). So now, we cannot to use join to this view.
>
> postgres=# select (databases)[1] from pg_hba_conf ;
> databases
> -----------
> "omega 2"
>
> (4 rows)
>
> postgres=# select datname from pg_database ;
> datname
> -----------
> template1
> template0
> postgres
> omega 2
> (4 rows)
>
> I dislike this - we know, so the name must be quoted in file (without it,
> the file was incorrect). And if you need quotes, there is a function
> quote_ident. If we use quotes elsewhere, then it should be ok, bot not now.
> Please, remove it. More, it is not necessary, when you use a "keyword"
> columns.

Thanks. The special handling of quotes for pg_hba_conf is not required.
I will keep the behaviour similar to the other catalog tables.
I will remove the quote support in the next version.

Regards,
Hari Babu
Fujitsu Australia

2015-04-04 15:29 GMT+02:00 Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>:

> On Sat, Apr 4, 2015 at 4:19 PM, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
> wrote:
> > Hi
> >
> > 2015-03-31 14:38 GMT+02:00 Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>:
> >>
> >> keyword_databases - The database name can be "all", "replication",
> >> sameuser", "samerole" and "samegroup".
> >> keyword_roles - The role can be "all" and a group name prefixed with
> "+".
> >
> >
> > I am not very happy about name - but I have no better idea :( - maybe
> > "database_mask", "user_mask"
> >
>
> How about database_keywords and user_keywords as column names?
>

I am thinking, it is better than keyword_databases - it is more logical.
But it is maybe question for mathematicians.

some other variant "database_filters" and "user_filters" or
"database_predicates" and "user_predicates"

but it is not terrible nice too

Regards

Pavel

>
> >>
> >>
> >> The rest of the database and role names are treated as normal database
> >> and role names.
> >>
> >> 2. Added the code changes to identify the names with quoted.
> >
> >
> > Is it a good idea? Database's names are not quoted every else (in system
> > catalog). So now, we cannot to use join to this view.
> >
> > postgres=# select (databases)[1] from pg_hba_conf ;
> > databases
> > -----------
> > "omega 2"
> >
> > (4 rows)
> >
> > postgres=# select datname from pg_database ;
> > datname
> > -----------
> > template1
> > template0
> > postgres
> > omega 2
> > (4 rows)
> >
> > I dislike this - we know, so the name must be quoted in file (without it,
> > the file was incorrect). And if you need quotes, there is a function
> > quote_ident. If we use quotes elsewhere, then it should be ok, bot not
> now.
> > Please, remove it. More, it is not necessary, when you use a "keyword"
> > columns.
>
> Thanks. The special handling of quotes for pg_hba_conf is not required.
> I will keep the behaviour similar to the other catalog tables.
> I will remove the quote support in the next version.
>
> Regards,
> Hari Babu
> Fujitsu Australia
>

I'm not sure what the best way to handle the hand-off from patch
contribution to reviewer/committer. If I start tweaking things then
you send in a new version it's actually more work to resolve the
conflicts. I think at this point it's easiest if I just take it from
here.

I'm puzzled about the change from pg_hba_settings to pg_hba_conf.
They're both ok but if pressed I would have preferred the original
pg_hba_settings since that's consistent with the pg_settings view.

There's no reason to quote tokens, that defeats the whole point of
breaking the keywords into a separate column. Also there's no point in
breaking out "all" but then still leaving "+foo" in the same column.
My version had that moved into a new column as well called
"recursive_roles". We could call that just "roles" or "groups" but I
was opting to the more explicit.

--
greg

On 2015-04-08 19:19:29 +0100, Greg Stark wrote:
> I'm not sure what the best way to handle the hand-off from patch
> contribution to reviewer/committer. If I start tweaking things then
> you send in a new version it's actually more work to resolve the
> conflicts. I think at this point it's easiest if I just take it from
> here.

Are you intending to commit this?

Greetings,

Andres Freund

On 5/1/15 12:33 PM, Andres Freund wrote:
> On 2015-04-08 19:19:29 +0100, Greg Stark wrote:
>> I'm not sure what the best way to handle the hand-off from patch
>> contribution to reviewer/committer. If I start tweaking things then
>> you send in a new version it's actually more work to resolve the
>> conflicts. I think at this point it's easiest if I just take it from
>> here.
>
> Are you intending to commit this?

It still looks quite dubious to me.

The more I test this, the more fond I grow of the idea of having this
information available in SQL. But I'm also growing more perplexed by
how this the file is mapped to a table. It just isn't a good match.

For instance: What is keyword_databases? Why is it an array? Same for
keyword_users. How can I know whether a given database or user matches
a keyword? What is compare_method? (Should perhaps be
keyword_address?) Why is compare method set to "mask" when a hostname
is set? (Column order is also a bit confusing here.) I'd also like
options to be jsonb instead of a text array.

Ultimately, I don't see how this is better than just showing the raw
file. I don't see a sensible way to compute answers to questions such
as "What is the authentication method for user X from IP address Y." If
I can't do that, what's the point of having a processed version?

On Tue, May 5, 2015 at 6:48 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 5/1/15 12:33 PM, Andres Freund wrote:
>> On 2015-04-08 19:19:29 +0100, Greg Stark wrote:
>>> I'm not sure what the best way to handle the hand-off from patch
>>> contribution to reviewer/committer. If I start tweaking things then
>>> you send in a new version it's actually more work to resolve the
>>> conflicts. I think at this point it's easiest if I just take it from
>>> here.
>>
>> Are you intending to commit this?
>
> It still looks quite dubious to me.
>
> The more I test this, the more fond I grow of the idea of having this
> information available in SQL. But I'm also growing more perplexed by
> how this the file is mapped to a table. It just isn't a good match.
>
> For instance: What is keyword_databases? Why is it an array? Same for
> keyword_users. How can I know whether a given database or user matches
> a keyword? What is compare_method? (Should perhaps be
> keyword_address?) Why is compare method set to "mask" when a hostname
> is set? (Column order is also a bit confusing here.) I'd also like
> options to be jsonb instead of a text array.

Thanks for your suggestion. I am not sure how to use jsonb here, i
will study the same
and provide a patch for the next version.

Regards,
Hari Babu
Fujitsu Australia

* Haribabu Kommi (kommi(dot)haribabu(at)gmail(dot)com) wrote:
> On Tue, May 5, 2015 at 6:48 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> > On 5/1/15 12:33 PM, Andres Freund wrote:
> >> On 2015-04-08 19:19:29 +0100, Greg Stark wrote:
> >>> I'm not sure what the best way to handle the hand-off from patch
> >>> contribution to reviewer/committer. If I start tweaking things then
> >>> you send in a new version it's actually more work to resolve the
> >>> conflicts. I think at this point it's easiest if I just take it from
> >>> here.
> >>
> >> Are you intending to commit this?
> >
> > It still looks quite dubious to me.
> >
> > The more I test this, the more fond I grow of the idea of having this
> > information available in SQL. But I'm also growing more perplexed by
> > how this the file is mapped to a table. It just isn't a good match.
> >
> > For instance: What is keyword_databases? Why is it an array? Same for
> > keyword_users. How can I know whether a given database or user matches
> > a keyword? What is compare_method? (Should perhaps be
> > keyword_address?) Why is compare method set to "mask" when a hostname
> > is set? (Column order is also a bit confusing here.) I'd also like
> > options to be jsonb instead of a text array.
>
> Thanks for your suggestion. I am not sure how to use jsonb here, i
> will study the same
> and provide a patch for the next version.

Regarding "next version"- are you referring to 9.6 and therefore we
should go ahead and bounce this to the next CF, or were you planning to
post a "next version" of the patch today?

This is certainly a capability which I'd like to see, though I share
Peter's concerns regarding the splitting up of the keywords rather than
keeping the same structure as what's in the actual pg_hba.conf. That
strikes me as confusing. It'd be neat if we were able to change
pg_hba.conf to make more sense and then perhaps the SQL version wouldn't
look so different but I don't think there's any way to do that.

I discussed the patch briefing with Greg over IM, who pointed out that
keeping things just exactly as they are in the config file would mean
implementing, essentially, a pg_hba.conf parser in SQL. I can
understand that perspective, but I don't think there's really much hope
in users being able to use this view directly without a lot of effort,
regardless. We need to provide a function which takes the arguments
that our pg_hba lookup does (database, user-to-login-as, maybe system
user for pg_ident checks, optionally an IP, etc) and then returns the
record that matches.

Apologies for not being able to provide more feedback earlier. I'll be
happy to help with all of the above and review the patch.

Independently, I'd love to see an SQL interface to pg_ident.conf too,
where, I expect anyway, it'll be a lot simpler, though I'm not sure that
it's very useful until we also have pg_hba.conf available through SQL.

Thanks!

Stephen

On Fri, May 15, 2015 at 11:24 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> * Haribabu Kommi (kommi(dot)haribabu(at)gmail(dot)com) wrote:
>> On Tue, May 5, 2015 at 6:48 AM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>> > It still looks quite dubious to me.
>> >
>> > The more I test this, the more fond I grow of the idea of having this
>> > information available in SQL. But I'm also growing more perplexed by
>> > how this the file is mapped to a table. It just isn't a good match.
>> >
>> > For instance: What is keyword_databases? Why is it an array? Same for
>> > keyword_users. How can I know whether a given database or user matches
>> > a keyword? What is compare_method? (Should perhaps be
>> > keyword_address?) Why is compare method set to "mask" when a hostname
>> > is set? (Column order is also a bit confusing here.) I'd also like
>> > options to be jsonb instead of a text array.
>>
>> Thanks for your suggestion. I am not sure how to use jsonb here, i
>> will study the same
>> and provide a patch for the next version.
>
> Regarding "next version"- are you referring to 9.6 and therefore we
> should go ahead and bounce this to the next CF, or were you planning to
> post a "next version" of the patch today?

Yes, for 9.6 version.

> This is certainly a capability which I'd like to see, though I share
> Peter's concerns regarding the splitting up of the keywords rather than
> keeping the same structure as what's in the actual pg_hba.conf. That
> strikes me as confusing. It'd be neat if we were able to change
> pg_hba.conf to make more sense and then perhaps the SQL version wouldn't
> look so different but I don't think there's any way to do that.
>
> I discussed the patch briefing with Greg over IM, who pointed out that
> keeping things just exactly as they are in the config file would mean
> implementing, essentially, a pg_hba.conf parser in SQL. I can
> understand that perspective, but I don't think there's really much hope
> in users being able to use this view directly without a lot of effort,
> regardless. We need to provide a function which takes the arguments
> that our pg_hba lookup does (database, user-to-login-as, maybe system
> user for pg_ident checks, optionally an IP, etc) and then returns the
> record that matches.

Thanks for details. I will try to come up with a view and a function
by considering all the above for the next commitfest.

> Apologies for not being able to provide more feedback earlier. I'll be
> happy to help with all of the above and review the patch.
>
> Independently, I'd love to see an SQL interface to pg_ident.conf too,
> where, I expect anyway, it'll be a lot simpler, though I'm not sure that
> it's very useful until we also have pg_hba.conf available through SQL.

Yes, Definitely I look into pg_ident also along with pg_hba.

Regards,
Hari Babu
Fujitsu Australia

On 05/16/2015 06:00 AM, Haribabu Kommi wrote:
>> >Regarding "next version"- are you referring to 9.6 and therefore we
>> >should go ahead and bounce this to the next CF, or were you planning to
>> >post a "next version" of the patch today?
> Yes, for 9.6 version.

No new patch emerged that could be reviewed in this commitfest (July),
so I've marked this as "Returned with feedback".

- Heikki