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

On Fri, Mar 14, 2014 at 12:30 AM, Prabakaran, Vaishnavi
<vaishnavip@fast.au.fujitsu.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
areallowed in this view. 
 
> +   </para>

I suggest:
   The read-only <structname>pg_hba_settings</structname> view provides   access to the client authentication
configurationfrom 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@2ndQuadrant.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@2ndquadrant.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@2ndquadrant.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
likeprocess all of parsed_hba_lines into a tuplestore. Obviously there's still a race condition there, but at least
it'sa 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.Nasby@bluetreble.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

<div dir="ltr"><div class="gmail_extra"><br />On Wed, Jan 28, 2015 at 4:46 AM, Haribabu Kommi <<a
href="mailto:kommi.haribabu@gmail.com">kommi.haribabu@gmail.com</a>>wrote:<br />><br />> On Wed, Jan 28, 2015
at9:47 AM, Jim Nasby <<a href="mailto:Jim.Nasby@bluetreble.com">Jim.Nasby@bluetreble.com</a>> wrote:<br />>
>On 1/27/15 1:04 AM, Haribabu Kommi wrote:<br />> >><br />> >> Here I attached the latest version
ofthe patch.<br />> >> I will add this patch to the next commitfest.<br />> ><br />> ><br />>
>Apologies if this was covered, but why isn't the IP address an inet instead<br />> > of text?<br />><br
/>>Corrected the address type as inet instead of text. updated patch is attached.<br />><br />> > Also,
whathappens if someone reloads the config in the middle of running<br />> > the SRF?<br />><br />> hba
entriesare reloaded only in postmaster process, not in every backend.<br />> So there shouldn't be any problem with
configfile reload. Am i<br />> missing something?<br />><br /><br /></div><div class="gmail_extra">I don't have
anycomment to disagree with the patch (idea and code).<br /></div><div class="gmail_extra"><br /></div><div
class="gmail_extra">ButI'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.<br /></div><div
class="gmail_extra"><br/></div><div class="gmail_extra">Regards,<br /></div><div class="gmail_extra"><br />--<br
/>Fabríziode Royes Mello<br />Consultoria/Coaching PostgreSQL<br />>> Timbira: <a
href="http://www.timbira.com.br">http://www.timbira.com.br</a><br/>>> Blog: <a
href="http://fabriziomello.github.io">http://fabriziomello.github.io</a><br/>>> Linkedin: <a
href="http://br.linkedin.com/in/fabriziomello">http://br.linkedin.com/in/fabriziomello</a><br/>>> Twitter: <a
href="http://twitter.com/fabriziomello">http://twitter.com/fabriziomello</a><br/>>> Github: <a
href="http://github.com/fabriziomello">http://github.com/fabriziomello</a></div></div>

Fabrízio de Royes Mello <fabriziomello@gmail.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
areused to the varieties of how settings come into effect that perhaps this isn't an issue with pg_settings, but it's
probablyworth 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 1/29/15 6:19 AM, Fabrízio de Royes Mello wrote:
> On Wed, Jan 28, 2015 at 5:27 PM, Tom Lane <tgl@sss.pgh.pa.us <mailto:tgl@sss.pgh.pa.us>> wrote:
>  >
>  > Fabrízio de Royes Mello <fabriziomello@gmail.com <mailto:fabriziomello@gmail.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
developersof 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
ALTERSYSTEM. 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.confvia SQL raises all the problems that were brought up when ALTER SYSTEM was being developed. One of the big
problemsis a question of how you can safely modify a text file that's full of comments and what-not. You'd need to
addressthose 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 Thu, Jan 29, 2015 at 10:13 PM, Amit Kapila <amit.kapila16@gmail.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 01/30/2015 10:01 PM, Amit Kapila wrote:
> On Fri, Jan 30, 2015 at 10:58 PM, Robert Haas <robertmhaas@gmail.com
> <mailto:robertmhaas@gmail.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

On Sat, Feb 7, 2015 at 8:26 PM, Pavel Stehule <pavel.stehule@gmail.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,

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.vondra@2ndquadrant.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

On 27.2.2015 17:59, Stephen Frost wrote:
> All,
> 
> * Tomas Vondra (tomas.vondra@2ndquadrant.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

* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> 2015-02-27 17:59 GMT+01:00 Stephen Frost <sfrost@snowman.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.vondra@2ndquadrant.com) wrote:
> On 27.2.2015 17:59, Stephen Frost wrote:
> > All,
> >
> > * Tomas Vondra (tomas.vondra@2ndquadrant.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

* Pavel Stehule (pavel.stehule@gmail.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

Stephen Frost <sfrost@snowman.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

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.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.stehule@gmail.com) wrote:
> 2015-02-27 22:26 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
> > Stephen Frost <sfrost@snowman.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@snowman.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@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.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@agliodbs.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

On Sat, Feb 28, 2015 at 11:41 AM, Stephen Frost <sfrost@snowman.net> wrote:
> Pavel,
>
> * Pavel Stehule (pavel.stehule@gmail.com) wrote:
>> 2015-02-27 22:26 GMT+01:00 Tom Lane <tgl@sss.pgh.pa.us>:
>> > Stephen Frost <sfrost@snowman.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

Greg,

* Greg Stark (stark@mit.edu) wrote:
> On Mon, Mar 2, 2015 at 6:36 AM, Haribabu Kommi <kommi.haribabu@gmail.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

On Mon, Mar 2, 2015 at 4:36 PM, Greg Stark <stark@mit.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
upto 100k. At that point I start running into memory errors on reading the HBA file so I guess that's an indication
that'slarge 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@mit.edu) wrote:
> On Mon, Mar 2, 2015 at 4:36 PM, Greg Stark <stark@mit.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
okup to 100k. At that point I start running into memory errors on reading the HBA file so I guess that's an indication
that'slarge 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@snowman.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@mit.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@2ndquadrant.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@2ndquadrant.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.Nasby@bluetreble.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.Nasby@bluetreble.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@mit.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.Nasby@bluetreble.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.haribabu@gmail.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@mit.edu> wrote:
> On Wed, Mar 4, 2015 at 12:34 AM, Haribabu Kommi
> <kommi.haribabu@gmail.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.Nasby@bluetreble.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.haribabu@gmail.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.haribabu@gmail.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.)

On Wed, Mar 4, 2015 at 9:41 PM, Peter Eisentraut <peter_e@gmx.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 3/4/15 1:34 AM, Haribabu Kommi wrote:
> On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi
> <kommi.haribabu@gmail.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@mit.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@mit.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 1:46 PM, David G. Johnston
<david.g.johnston@gmail.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 Fri, Mar 13, 2015 at 1:33 PM, Peter Eisentraut <peter_e@gmx.net> wrote:
> On 3/4/15 1:34 AM, Haribabu Kommi wrote:
>> On Wed, Mar 4, 2015 at 12:35 PM, Haribabu Kommi
>> <kommi.haribabu@gmail.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

On Mon, Mar 30, 2015 at 4:34 AM, Pavel Stehule <pavel.stehule@gmail.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

On Sat, Apr 4, 2015 at 4:19 PM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hi
>
> 2015-03-31 14:38 GMT+02:00 Haribabu Kommi <kommi.haribabu@gmail.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

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@gmx.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.haribabu@gmail.com) wrote:
> On Tue, May 5, 2015 at 6:48 AM, Peter Eisentraut <peter_e@gmx.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@snowman.net> wrote:
> * Haribabu Kommi (kommi.haribabu@gmail.com) wrote:
>> On Tue, May 5, 2015 at 6:48 AM, Peter Eisentraut <peter_e@gmx.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