Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Invent pg_hba_file_rules view to show the content of pg_hba.conf.
- de16ab723888 10.0 landed
Restructure hba.c to replace 3 parallel lists with single list of structs.
- 350cb921ae2c 10.0 landed

pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-08-15T11:17:21Z

Hi All,

While working on pg_hba_lookup function that can be used to lookup for an client
authentication that can be matched for given input parameters, Tom raised some
concrete use case issues in the following mail [1]. In this same
thread, he raised
some advantages of having a view similar like pg_file_settings view
for pg_hba.conf
also.

Here I attached a patch that implements the pg_hba_file_settings view
that displays
all the rows in pg_hba.conf. In case if any error exists in the
authentication rule, the
corresponding error is displayed similar like pg_file_settings.

This view can be used to verify whether there exists any problems or
not in the pg_hba.conf
before it reloads into the system. This view cannot be used to check
similar like
pg_hba_lookup function to find out which rule maps to the
corresponding input connection.

comments?

[1] - https://www.postgresql.org/message-id/28434.1468246200%40sss.pgh.pa.us

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Simon Riggs <simon@2ndquadrant.com> — 2016-09-03T15:44:54Z

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

> comments?

This looks like a good feature contribution, thanks.

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

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

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

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

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

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

Thanks

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

Re: pg_hba_file_settings view patch

Christoph Berg <myon@debian.org> — 2016-09-04T11:46:03Z

Re: Simon Riggs 2016-09-03 <CANP8+jLFDeCJ7YWuzSodKXD85oNy2Kxa40U5GRnry6ms9Mz+5A@mail.gmail.com>
> pg_hba_file_settings seems a clumsy name. I'd prefer pg_hba_settings,
> since that name could live longer than the concept of pg_hba.conf,
> which seems likely to become part of ALTER SYSTEM in future, so we
> wouldn't really want the word "file" in there.

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

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

Christoph

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-09-05T07:09:53Z

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

> On 15 August 2016 at 12:17, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
>
> > comments?
>
> This looks like a good feature contribution, thanks.
>
> At present the patch doesn't apply cleanly, please rebase.
>

Rebased patch is attached.

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

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

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

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

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

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

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

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

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

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2016-10-03T04:51:50Z

On Mon, Sep 5, 2016 at 4:09 PM, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
> On Sun, Sep 4, 2016 at 1:44 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On 15 August 2016 at 12:17, Haribabu Kommi <kommi.haribabu@gmail.com>
>> wrote:
>>
>> > comments?
>>
>> This looks like a good feature contribution, thanks.
>>
>> At present the patch doesn't apply cleanly, please rebase.
>
>
> Rebased patch is attached.

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

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

Re: pg_hba_file_settings view patch

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2016-10-03T06:25:56Z

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

I guess for ability to use filtering like:

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

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

-- 
Best regards,
Vitaly Burovoy

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2016-10-03T06:46:33Z

On Mon, Oct 3, 2016 at 3:25 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com> wrote:
> I guess for ability to use filtering like:
>
> SELECT * FROM pg_hba_rules WHERE options->>radiusserver LIKE '%.example.com';
>
> I think it would be harder if options is an array of strings...

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

Re: pg_hba_file_settings view patch

Vitaly Burovoy <vitaly.burovoy@gmail.com> — 2016-10-03T07:53:09Z

On 10/2/16, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Mon, Oct 3, 2016 at 3:25 PM, Vitaly Burovoy <vitaly.burovoy@gmail.com>
> wrote:
>> I guess for ability to use filtering like:
>>
>> SELECT * FROM pg_hba_rules WHERE options->>radiusserver LIKE
>> '%.example.com';
>>
>> I think it would be harder if options is an array of strings...
>
> With unnest() and a matching pattern, not that hard but..

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

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

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

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

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

-- 
Best regards,
Vitaly Burovoy

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-10-25T06:00:47Z

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

> On Mon, Sep 5, 2016 at 4:09 PM, Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
> > On Sun, Sep 4, 2016 at 1:44 AM, Simon Riggs <simon@2ndquadrant.com>
> wrote:
> >> On 15 August 2016 at 12:17, Haribabu Kommi <kommi.haribabu@gmail.com>
> >> wrote:
> >>
> >> > comments?
> >>
> >> This looks like a good feature contribution, thanks.
> >>
> >> At present the patch doesn't apply cleanly, please rebase.
> >
> >
> > Rebased patch is attached.
>
> Moved to next CF as there is a patch and no reviews.
>
> +       push_jsonb_string_key(&parseState, "map");
> +       push_jsonb_string_value(&parseState, hba->usermap);
> [...]
> +    <row>
> +     <entry><structfield>options</structfield></entry>
> +     <entry><type>jsonb</type></entry>
> +     <entry>Configuration options set for authentication method</entry>
> +    </row>
> Why is it an advantage to use jsonb here instead of a simple array
> made of name=value? If they were nested I'd see a case for it but it
> seems to me that as presented this is just an overkill. In short, I
> think that this patch needs a bit of rework, so I am marking it as
> returned with feedback.
>


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

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

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-10-25T07:23:29Z

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

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

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

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

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

Re: pg_hba_file_settings view patch

Robert Haas <robertmhaas@gmail.com> — 2016-10-26T19:07:45Z

On Tue, Oct 25, 2016 at 3:23 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Haribabu Kommi wrote:
>> On Mon, Oct 3, 2016 at 3:51 PM, Michael Paquier <michael.paquier@gmail.com>
>> wrote:
>
>> Yes, I agree that adding these JSONB utility functions for this view
>> is an overkill, but I thought that these are may be useful for some
>> users if it is a JSONB type instead of array.
>
> Peter Eisentraut said he'd like JSON better:
> https://www.postgresql.org/message-id/5547DB0A.2020904@gmx.net
> I asked twice about the use of JSON, suggesting an array instead:
> https://www.postgresql.org/message-id/20151204163147.GZ2763@alvherre.pgsql
> https://www.postgresql.org/message-id/20160201215714.GA98800@alvherre.pgsql
>
> I now think that we should figure out what it is that we want before we
> continue to request it to be changed over and over.

That sounds like a good idea.  :-)

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

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

Re: pg_hba_file_settings view patch

Tom Lane <tgl@sss.pgh.pa.us> — 2016-10-26T19:24:53Z

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

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

			regards, tom lane

Re: pg_hba_file_settings view patch

Josh Berkus <josh@agliodbs.com> — 2016-10-26T19:54:22Z

On 10/26/2016 12:24 PM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> FWIW, I'm -1 on using JSON here.  I don't believe that we should start
>> using JSON all over the place just because we can.  If we do that,
>> we'll end up with a mishmash of styles, and maybe look silly when JSON
>> is replaced by the new and much better SDGJHSDR format.
> 
> I concur.  JSON isn't a core datatype and I don't want to see it treated
> as one.  We should redesign this view so that it doesn't rely on anything
> more advanced than arrays.

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

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

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)

Re: pg_hba_file_settings view patch

Joshua D. Drake <jd@commandprompt.com> — 2016-10-26T20:04:15Z

On 10/26/2016 12:54 PM, Josh Berkus wrote:
> On 10/26/2016 12:24 PM, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> FWIW, I'm -1 on using JSON here.  I don't believe that we should start
>>> using JSON all over the place just because we can.  If we do that,
>>> we'll end up with a mishmash of styles, and maybe look silly when JSON
>>> is replaced by the new and much better SDGJHSDR format.
>>
>> I concur.  JSON isn't a core datatype and I don't want to see it treated
>> as one.  We should redesign this view so that it doesn't rely on anything
>> more advanced than arrays.
>
> Huh?  Sure it is.   Ships in PostgreSQL-core.
>
> I mean, I'm not particularly in favor of using JSON for this (arrays
> seem OK), but that seems like an invalid reason not to.

-1 to JSON for this.

JD

>


-- 
Command Prompt, Inc.                  http://the.postgres.company/
                         +1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.

Re: pg_hba_file_settings view patch

Tom Lane <tgl@sss.pgh.pa.us> — 2016-10-26T20:11:58Z

Josh Berkus <josh@agliodbs.com> writes:
> On 10/26/2016 12:24 PM, Tom Lane wrote:
>> I concur.  JSON isn't a core datatype and I don't want to see it treated
>> as one.  We should redesign this view so that it doesn't rely on anything
>> more advanced than arrays.

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

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

			regards, tom lane

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2016-10-27T00:33:10Z

On Thu, Oct 27, 2016 at 5:11 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Josh Berkus <josh@agliodbs.com> writes:
>> On 10/26/2016 12:24 PM, Tom Lane wrote:
>>> I concur.  JSON isn't a core datatype and I don't want to see it treated
>>> as one.  We should redesign this view so that it doesn't rely on anything
>>> more advanced than arrays.
>
>> Huh?  Sure it is.   Ships in PostgreSQL-core.
>
> To my way of thinking it's a nonstandard extension.  The fact that we
> chose to package it in core and not as an extension doesn't alter the
> fact that it's peripheral to the system and nothing else depends on it.
> I'd like to keep things that way.  I wouldn't want any core-system
> functionality to start depending on the geometric types, either.

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

Re: pg_hba_file_settings view patch

Greg Stark <stark@mit.edu> — 2016-10-27T06:37:30Z

On Wed, Oct 26, 2016 at 11:04 PM, Joshua D. Drake <jd@commandprompt.com> wrote:
> On 10/26/2016 12:54 PM, Josh Berkus wrote:
>> I mean, I'm not particularly in favor of using JSON for this (arrays
>> seem OK), but that seems like an invalid reason not to.
>
> -1 to JSON for this.

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

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

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

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

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

-- 
greg

Re: pg_hba_file_settings view patch

Alvaro Herrera <alvherre@2ndquadrant.com> — 2016-10-27T17:17:07Z

Greg Stark wrote:

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

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

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

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-10-28T05:55:51Z

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

> Greg Stark wrote:
>
> > The fundamental problem is that the pga_hba.conf file has some bits of
> > complex structure that aren't easily captured by linear arrays. The
> > problem I struggled with most was the keywords like "all", "samerole",
> > and "replication". A simple array of text makes it awkward to
> > distinguish those keywords from the quoted text values with the same
> > content. And then there are the ldap options which naturally would be
> > a data type like json or htab.
>
> Hmm I thought we had decided that such keywords would live in separate
> arrays, i.e. you have one array for plain names and another array for
> keyword stuff.  Then it's not ambiguous anymore.



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


Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-07T03:36:18Z

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

>
>
> On Fri, Oct 28, 2016 at 4:17 AM, Alvaro Herrera <alvherre@2ndquadrant.com>
> wrote:
>
>> Greg Stark wrote:
>>
>> > The fundamental problem is that the pga_hba.conf file has some bits of
>> > complex structure that aren't easily captured by linear arrays. The
>> > problem I struggled with most was the keywords like "all", "samerole",
>> > and "replication". A simple array of text makes it awkward to
>> > distinguish those keywords from the quoted text values with the same
>> > content. And then there are the ldap options which naturally would be
>> > a data type like json or htab.
>>
>> Hmm I thought we had decided that such keywords would live in separate
>> arrays, i.e. you have one array for plain names and another array for
>> keyword stuff.  Then it's not ambiguous anymore.
>
>
>
> Thanks for all your opinions. Here I attached updated patch with the change
> in column datatype from JSONB to TEXT array. Rest of the code changes
> are same to the earlier patch.
>

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

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2016-11-07T04:36:45Z

On Mon, Nov 7, 2016 at 12:36 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> The added regression test fails for the cases where the server is loaded
> with
> different pg_hba.conf rules during installcheck verification. Updated patch
> is
> attached with removing those tests.

That's not a full review as I just glanced at this patch a couple of seconds...

 #include "utils/guc.h"
+#include "utils/jsonb.h"
 #include "utils/lsyscache.h"
You don't need to include this header when using arrays.

Implementing a test case is possible as well using the TAP
infrastructure. You may want to look at it and help folks testing the
patch more easily with a set of configurations in pg_hba.conf that
cover a maximum of code paths in your patch.
-- 
Michael

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-10T06:10:54Z

On Mon, Nov 7, 2016 at 3:36 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Mon, Nov 7, 2016 at 12:36 PM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
> > The added regression test fails for the cases where the server is loaded
> > with
> > different pg_hba.conf rules during installcheck verification. Updated
> patch
> > is
> > attached with removing those tests.
>
> That's not a full review as I just glanced at this patch a couple of
> seconds...
>
>  #include "utils/guc.h"
> +#include "utils/jsonb.h"
>  #include "utils/lsyscache.h"
> You don't need to include this header when using arrays.
>

Thanks for the review. Fixed in the updated patch with
additional error messages are also added.


> Implementing a test case is possible as well using the TAP
> infrastructure. You may want to look at it and help folks testing the
> patch more easily with a set of configurations in pg_hba.conf that
> cover a maximum of code paths in your patch.
>

Added a tap test under src/test folder to cover maximum code changes.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2016-11-16T11:10:17Z

make check run with this patch shows server crashes. regression.out
attached. I have run make check after a clean build, tried building it
after running configure, but the problem is always reproducible. Do
you see this problem?

Also the patch has a white space error.
git diff --check
src/backend/utils/init/postinit.c:729: space before tab in indent.
+       /*

On Thu, Nov 10, 2016 at 11:40 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
>
>
> On Mon, Nov 7, 2016 at 3:36 PM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Mon, Nov 7, 2016 at 12:36 PM, Haribabu Kommi
>> <kommi.haribabu@gmail.com> wrote:
>> > The added regression test fails for the cases where the server is loaded
>> > with
>> > different pg_hba.conf rules during installcheck verification. Updated
>> > patch
>> > is
>> > attached with removing those tests.
>>
>> That's not a full review as I just glanced at this patch a couple of
>> seconds...
>>
>>  #include "utils/guc.h"
>> +#include "utils/jsonb.h"
>>  #include "utils/lsyscache.h"
>> You don't need to include this header when using arrays.
>
>
> Thanks for the review. Fixed in the updated patch with
> additional error messages are also added.
>
>>
>> Implementing a test case is possible as well using the TAP
>> infrastructure. You may want to look at it and help folks testing the
>> patch more easily with a set of configurations in pg_hba.conf that
>> cover a maximum of code paths in your patch.
>
>
> Added a tap test under src/test folder to cover maximum code changes.
>
> Regards,
> Hari Babu
> Fujitsu Australia
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: pg_hba_file_settings view patch

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2016-11-17T11:13:27Z

On Wed, Nov 16, 2016 at 4:40 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> make check run with this patch shows server crashes. regression.out
> attached. I have run make check after a clean build, tried building it
> after running configure, but the problem is always reproducible. Do
> you see this problem?
>
> Also the patch has a white space error.
> git diff --check
> src/backend/utils/init/postinit.c:729: space before tab in indent.
> +       /*
>
I looked at the patch in some more details and here are some more comments
1. In catalog.sgml, the sentence "If the configuration file contains any errors
..." looks redundant, as description of "error" field says so. Removed it in
the attached patch. In that example, you might want to provide pg_hba.conf
contents to help understand the view output.

2. I think the view will be useful, if loading the file did not have the
desired effects, whether because of SIGHUP or a fresh start. So, may be the
sentence "for diagnosing problems if a SIGHUP signal did not have the desired
effects.", should be changed to be more generic e.g. ... if loading file did
not have ... .

3. Something wrong with the indentation, at least not how pg_indent would indent
the variable names.
+typedef struct LookupHbaLineCxt
+{
+    MemoryContext memcxt;
+    TupleDesc    tupdesc;
+    Tuplestorestate *tuple_store;
+} LookupHbaLineCxt;

+static void lookup_hba_line_callback(void *context, int lineno,
HbaLine *hba, const char *err_msg);
Overflows 80 character limit.

in parse_hba_line()
-        ereport(LOG,
+        ereport(level,
                 (errcode(ERRCODE_CONFIG_FILE_ERROR),
                  errmsg("invalid connection type \"%s\"",
                         token->string),
                  errcontext("line %d of configuration file \"%s\"",
                             line_num, HbaFileName)));
+        *err_msg = pstrdup(_("invalid connection type"));

Is the difference between error reported to error log and that in the view
intentional? That brings to another question. Everywhere, in similar code, the
patch adds a line *err_msg = pstrdup() or psprinf() and copies the arguements
to errmsg(). Someone modifying the error message has to duplicate the changes.
Since the code is just few lines away, it may not be hard to duplicate the
changes, but still that's a maintenance burder. Isn't there a way to compute
the message once and use it twice? show_all_file_settings() used for
pg_file_settings also has similar problem, so may be it's an accepted practice.
There are multiple instances of such a difference, but may be the invalid value
can be found out from the value of the referenced field (which will be part of
the view). So, may be it's ok. But that not true with the difference below.
gai_strerror() may not be obvious from the referenced field.
-                ereport(LOG,
+                ereport(level,
                         (errcode(ERRCODE_CONFIG_FILE_ERROR),
                          errmsg("invalid IP address \"%s\": %s",
                                 str, gai_strerror(ret)),
                          errcontext("line %d of configuration file \"%s\"",
                                     line_num, HbaFileName)));
                 if (gai_result)
                     pg_freeaddrinfo_all(hints.ai_family, gai_result);
+                *err_msg = pstrdup(_("invalid IP address"));

4.
+    if (!rsi || !IsA(rsi, ReturnSetInfo) ||
+        (rsi->allowedModes & SFRM_Materialize) == 0)
+        ereport(ERROR,
+                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                 errmsg("set-valued function called in context that
cannot accept a set")));
show_all_file_settings(), a function similar to this one, splits the condition
above into two and throws different error message for each of them.
    /* Check to see if caller supports us returning a tuplestore */
    if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
        ereport(ERROR,
                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                 errmsg("set-valued function called in context that
cannot accept a set")));
    if (!(rsinfo->allowedModes & SFRM_Materialize))
        ereport(ERROR,
                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
                 errmsg("materialize mode required, but it is not " \
                        "allowed in this context")));
Why is this difference?

5. May be you want to rename "type" attribute to "connection_type" to be
explicit.

6. The attribute names "keyword_database" and "keyword_user" do not seem to be
appropriate. They do not look like keywords as such. They are more like
synonyms or collection (value replication is an exception). May be you want to
rename those as "database_keyword" or "user_keyword" similar to the naming
convention of token_is_a_database_keyword(). I agree that the usage
can not be described in a single phrase correctly, and pg_hba.conf
documentation too doesn't help much. Similarly for keyword_address.

7. Also, each of the fields, database, user, address can contain multiple
values in pg_hba.conf. So may be corresponding attributes should be named as
plural rather than singular.

8. If any of the parsed lines has an error parse_hba_line() returns a NULL
line. In order to avoid memory leak, load_hba() runs this function in a memory
context, which is destroyed when an error is encountered. This also destroys
any previous lines which were parsed correctly. IIUC, the patch uses the
callback to save the contents of those lines in a different context, so that an
error wouldn't destroy those. But using a callback doesn't seem like a good way
to do this. Furthermore the code assumes that if callback is provided the error
level is always DEBUG3 or the caller doesn't want to update the saved
authentication rules etc. If in future someone wants to add another callback
function but doesn't want error level to be DEBUG3 or still wants to update the
saved rules, we will need conditional statements based on the value of
callback. That doesn't seems to be something which should be done with
callbacks. I don't think that's flexible. A better design may be to let
load_hba() accept errorlevel, and flag indicating whether to ignore errors as
an argument and return a list of parsed lines. If there is an error and the
flag indicates not to ignore the error, we destroy the memory context and
return NIL. The list can be then used to update the saved hba rules or to
process further (e.g. to feed the function hba_rules()). hbacxt also can an
OUTPUT arguemnt to the function or an argument passed in by the caller.

9. I am not able to understand, why does this patch need changes to
load_ident(). Sorry, if I have missed any previous discussion on this topic.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-18T06:53:34Z

On Thu, Nov 17, 2016 at 10:13 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> On Wed, Nov 16, 2016 at 4:40 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
> > make check run with this patch shows server crashes. regression.out
> > attached. I have run make check after a clean build, tried building it
> > after running configure, but the problem is always reproducible. Do
> > you see this problem?
>

Thanks for reviewing the patch.

No. I am not able to reproduce this problem.
make check works fine in my system.

From the regression.out file, the crash occurred in select_parallel.out,
I don't think this patch has any affect on that test.

> Also the patch has a white space error.
> > git diff --check
> > src/backend/utils/init/postinit.c:729: space before tab in indent.
> > +       /*
> >
>

corrected.


> I looked at the patch in some more details and here are some more comments
> 1. In catalog.sgml, the sentence "If the configuration file contains any
> errors
> ..." looks redundant, as description of "error" field says so. Removed it
> in
> the attached patch. In that example, you might want to provide pg_hba.conf
> contents to help understand the view output.
>

updated details, but not exactly what you said. check it once.

2. I think the view will be useful, if loading the file did not have the
> desired effects, whether because of SIGHUP or a fresh start. So, may be the
> sentence "for diagnosing problems if a SIGHUP signal did not have the
> desired
> effects.", should be changed to be more generic e.g. ... if loading file
> did
> not have ... .
>

changed.


> 3. Something wrong with the indentation, at least not how pg_indent would
> indent
> the variable names.
> +typedef struct LookupHbaLineCxt
> +{
> +    MemoryContext memcxt;
> +    TupleDesc    tupdesc;
> +    Tuplestorestate *tuple_store;
> +} LookupHbaLineCxt;
>

corrected.


> +static void lookup_hba_line_callback(void *context, int lineno,
> HbaLine *hba, const char *err_msg);
> Overflows 80 character limit.
>

corrected.


> in parse_hba_line()
> -        ereport(LOG,
> +        ereport(level,
>                  (errcode(ERRCODE_CONFIG_FILE_ERROR),
>                   errmsg("invalid connection type \"%s\"",
>                          token->string),
>                   errcontext("line %d of configuration file \"%s\"",
>                              line_num, HbaFileName)));
> +        *err_msg = pstrdup(_("invalid connection type"));
>
> Is the difference between error reported to error log and that in the view
> intentional? That brings to another question. Everywhere, in similar code,
> the
> patch adds a line *err_msg = pstrdup() or psprinf() and copies the
> arguements
> to errmsg(). Someone modifying the error message has to duplicate the
> changes.
> Since the code is just few lines away, it may not be hard to duplicate the
> changes, but still that's a maintenance burder. Isn't there a way to
> compute
> the message once and use it twice? show_all_file_settings() used for
> pg_file_settings also has similar problem, so may be it's an accepted
> practice.
> There are multiple instances of such a difference, but may be the invalid
> value
> can be found out from the value of the referenced field (which will be
> part of
> the view). So, may be it's ok. But that not true with the difference below.
> gai_strerror() may not be obvious from the referenced field.
> -                ereport(LOG,
> +                ereport(level,
>                          (errcode(ERRCODE_CONFIG_FILE_ERROR),
>                           errmsg("invalid IP address \"%s\": %s",
>                                  str, gai_strerror(ret)),
>                           errcontext("line %d of configuration file
> \"%s\"",
>                                      line_num, HbaFileName)));
>                  if (gai_result)
>                      pg_freeaddrinfo_all(hints.ai_family, gai_result);
> +                *err_msg = pstrdup(_("invalid IP address"));
>

Reused the error string once, as in this patch it chances in many places
compared
to pg_file_settings, so I tend to reuse it.


> 4.
> +    if (!rsi || !IsA(rsi, ReturnSetInfo) ||
> +        (rsi->allowedModes & SFRM_Materialize) == 0)
> +        ereport(ERROR,
> +                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                 errmsg("set-valued function called in context that
> cannot accept a set")));
> show_all_file_settings(), a function similar to this one, splits the
> condition
> above into two and throws different error message for each of them.
>     /* Check to see if caller supports us returning a tuplestore */
>     if (rsinfo == NULL || !IsA(rsinfo, ReturnSetInfo))
>         ereport(ERROR,
>                 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                  errmsg("set-valued function called in context that
> cannot accept a set")));
>     if (!(rsinfo->allowedModes & SFRM_Materialize))
>         ereport(ERROR,
>                 (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>                  errmsg("materialize mode required, but it is not " \
>                         "allowed in this context")));
> Why is this difference?
>

changed according to show_all_file_settings() function.


> 5. May be you want to rename "type" attribute to "connection_type" to be
> explicit.
>

"type" is the keyword that is mentioned in the pg_hba.conf, I feel it is
better
if this view is in sync with that. If others feel the same, I can change.


> 6. The attribute names "keyword_database" and "keyword_user" do not seem
> to be
> appropriate. They do not look like keywords as such. They are more like
> synonyms or collection (value replication is an exception). May be you
> want to
> rename those as "database_keyword" or "user_keyword" similar to the naming
> convention of token_is_a_database_keyword(). I agree that the usage
> can not be described in a single phrase correctly, and pg_hba.conf
> documentation too doesn't help much. Similarly for keyword_address.
>

Changed.


> 7. Also, each of the fields, database, user, address can contain multiple
> values in pg_hba.conf. So may be corresponding attributes should be named
> as
> plural rather than singular.
>

Same answer as to the question - 5.


> 8. If any of the parsed lines has an error parse_hba_line() returns a NULL
> line. In order to avoid memory leak, load_hba() runs this function in a
> memory
> context, which is destroyed when an error is encountered. This also
> destroys
> any previous lines which were parsed correctly. IIUC, the patch uses the
> callback to save the contents of those lines in a different context, so
> that an
> error wouldn't destroy those. But using a callback doesn't seem like a
> good way
> to do this. Furthermore the code assumes that if callback is provided the
> error
> level is always DEBUG3 or the caller doesn't want to update the saved
> authentication rules etc. If in future someone wants to add another
> callback
> function but doesn't want error level to be DEBUG3 or still wants to
> update the
> saved rules, we will need conditional statements based on the value of
> callback. That doesn't seems to be something which should be done with
> callbacks. I don't think that's flexible. A better design may be to let
> load_hba() accept errorlevel, and flag indicating whether to ignore errors
> as
> an argument and return a list of parsed lines. If there is an error and the
> flag indicates not to ignore the error, we destroy the memory context and
> return NIL. The list can be then used to update the saved hba rules or to
> process further (e.g. to feed the function hba_rules()). hbacxt also can an
> OUTPUT arguemnt to the function or an argument passed in by the caller.
>

hba_rules() function cannot operate on final parsed hba lines, because it
has
to store the error that is present in the line, that can be obtained only
during
the parse stage.

The hba rules are needed only for the authentication purpose and those
shouldn't
be stored in the individual backend. Because of this reason after every
operation
the parsed rules are cleared.

Added a flag to pass the log level.


> 9. I am not able to understand, why does this patch need changes to
> load_ident(). Sorry, if I have missed any previous discussion on this
> topic.


Earlier, the pg_hba.conf is loaded into the Postmastercontext, this is
causing
problems for this patch. So In the patch it is changed into
currentmemorycontext
and deleted the data at the end. The similar change is carried out for ident
functionality, because of this reason, it is shown in this patch. May be I
can
separate that into an individual patch.

Updated patch is attached.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2016-11-22T10:46:49Z

On Fri, Nov 18, 2016 at 12:23 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
>
>
> On Thu, Nov 17, 2016 at 10:13 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>
>> On Wed, Nov 16, 2016 at 4:40 PM, Ashutosh Bapat
>> <ashutosh.bapat@enterprisedb.com> wrote:
>> > make check run with this patch shows server crashes. regression.out
>> > attached. I have run make check after a clean build, tried building it
>> > after running configure, but the problem is always reproducible. Do
>> > you see this problem?
>
>
> Thanks for reviewing the patch.
>
> No. I am not able to reproduce this problem.
> make check works fine in my system.

It could be because of some un-initialised variable, which is
initialized appropriately by default on your machine but not on my
machine. I first applied your pg_hba_rules... patch, ran regression.
It didn't crash. then I applied patch for discard_hba... and it
started crashing. Does that give you any clue? Here's regression.out
file for make installcheck. Here is error log snippet that shows a
SIGSEGV there.
2016-11-22 15:47:11.939 IST [86206] LOG:  worker process: parallel
worker for PID 86779 (PID 86780) was terminated by signal 11:
Segmentation fault
2016-11-22 15:47:11.939 IST [86206] LOG:  terminating any other active
server processes
2016-11-22 15:47:11.939 IST [86779] WARNING:  terminating connection
because of crash of another server process
2016-11-22 15:47:11.939 IST [86779] DETAIL:  The postmaster has
commanded this server process to roll back the current transaction and
exit, because another server process exited abnormally and possibly
corrupted shared memory.

Applying those patches in any order doesn't matter.

>
> From the regression.out file, the crash occurred in select_parallel.out,
> I don't think this patch has any affect on that test.

The changes in postinit.c may have that impact. Just a guess though. I
haven't debugged the crash myself.


>>
>> I looked at the patch in some more details and here are some more comments
>> 1. In catalog.sgml, the sentence "If the configuration file contains any
>> errors
>> ..." looks redundant, as description of "error" field says so. Removed it
>> in
>> the attached patch. In that example, you might want to provide pg_hba.conf
>> contents to help understand the view output.
>
>
> updated details, but not exactly what you said. check it once.
>



>
>>
>> in parse_hba_line()
>> -        ereport(LOG,
>> +        ereport(level,
>>                  (errcode(ERRCODE_CONFIG_FILE_ERROR),
>>                   errmsg("invalid connection type \"%s\"",
>>                          token->string),
>>                   errcontext("line %d of configuration file \"%s\"",
>>                              line_num, HbaFileName)));
>> +        *err_msg = pstrdup(_("invalid connection type"));
>>
>> Is the difference between error reported to error log and that in the view
>> intentional? That brings to another question. Everywhere, in similar code,
>> the
>> patch adds a line *err_msg = pstrdup() or psprinf() and copies the
>> arguements
>> to errmsg(). Someone modifying the error message has to duplicate the
>> changes.
>> Since the code is just few lines away, it may not be hard to duplicate the
>> changes, but still that's a maintenance burder. Isn't there a way to
>> compute
>> the message once and use it twice? show_all_file_settings() used for
>> pg_file_settings also has similar problem, so may be it's an accepted
>> practice.
>> There are multiple instances of such a difference, but may be the invalid
>> value
>> can be found out from the value of the referenced field (which will be
>> part of
>> the view). So, may be it's ok. But that not true with the difference
>> below.
>> gai_strerror() may not be obvious from the referenced field.
>> -                ereport(LOG,
>> +                ereport(level,
>>                          (errcode(ERRCODE_CONFIG_FILE_ERROR),
>>                           errmsg("invalid IP address \"%s\": %s",
>>                                  str, gai_strerror(ret)),
>>                           errcontext("line %d of configuration file
>> \"%s\"",
>>                                      line_num, HbaFileName)));
>>                  if (gai_result)
>>                      pg_freeaddrinfo_all(hints.ai_family, gai_result);
>> +                *err_msg = pstrdup(_("invalid IP address"));
>
>
> Reused the error string once, as in this patch it chances in many places
> compared
> to pg_file_settings, so I tend to reuse it.

Thanks. Although the new change might affect the way we translate the
messages in other languages. I am not sure. So, I will leave it for
someone with more knowledge to review.


>
>>
>> 5. May be you want to rename "type" attribute to "connection_type" to be
>> explicit.
>
>
> "type" is the keyword that is mentioned in the pg_hba.conf, I feel it is
> better
> if this view is in sync with that. If others feel the same, I can change.

Ok.


>
>>
>> 7. Also, each of the fields, database, user, address can contain multiple
>> values in pg_hba.conf. So may be corresponding attributes should be named
>> as
>> plural rather than singular.
>
>
> Same answer as to the question - 5.

Ok.

>
>>
>> 8. If any of the parsed lines has an error parse_hba_line() returns a NULL
>> line. In order to avoid memory leak, load_hba() runs this function in a
>> memory
>> context, which is destroyed when an error is encountered. This also
>> destroys
>> any previous lines which were parsed correctly. IIUC, the patch uses the
>> callback to save the contents of those lines in a different context, so
>> that an
>> error wouldn't destroy those. But using a callback doesn't seem like a
>> good way
>> to do this. Furthermore the code assumes that if callback is provided the
>> error
>> level is always DEBUG3 or the caller doesn't want to update the saved
>> authentication rules etc. If in future someone wants to add another
>> callback
>> function but doesn't want error level to be DEBUG3 or still wants to
>> update the
>> saved rules, we will need conditional statements based on the value of
>> callback. That doesn't seems to be something which should be done with
>> callbacks. I don't think that's flexible. A better design may be to let
>> load_hba() accept errorlevel, and flag indicating whether to ignore errors
>> as
>> an argument and return a list of parsed lines. If there is an error and
>> the
>> flag indicates not to ignore the error, we destroy the memory context and
>> return NIL. The list can be then used to update the saved hba rules or to
>> process further (e.g. to feed the function hba_rules()). hbacxt also can
>> an
>> OUTPUT arguemnt to the function or an argument passed in by the caller.
>
>
> hba_rules() function cannot operate on final parsed hba lines, because it
> has
> to store the error that is present in the line, that can be obtained only
> during
> the parse stage.
>
> The hba rules are needed only for the authentication purpose and those
> shouldn't
> be stored in the individual backend. Because of this reason after every
> operation
> the parsed rules are cleared.
>
> Added a flag to pass the log level.

    /*
+    * If callback function is available, then don't update the saved
+    * authentication rules.
+    */
+   if (callback)
+   {
+       MemoryContextDelete(linecxt);
+       MemoryContextSwitchTo(oldcxt);
+       MemoryContextDelete(hbacxt);
+       return true;
+   }

this still remains problematic, in case another user of load_hba wants
to pass a callback but wants to update the saved rules. Usually,
callbacks are used when the decision to modify certain logic is far
away in time and code from the actual place where the changes are to
be applied, e.g. FDW callbacks. But here we that's not the case. Is
there any precedence in code for something like this.

What we may want to do, is separate the logic of reading the hba rules
in a list and the logic to update existing rules in two different
functions e.g read_hba() and load_hba(). hba_rules() can use
read_hba() with ignore errors flag to get the list of hba lines. It
can then use this list to create tuples to be returned in hba_rules().
load_hba() will read_hba() with memory reset on error flag (same
boolean) to read the list of hba lines and update the saved rules if
there's no error. err_msg can be either a field in HbaLine, which will
be used only by hba_rules() OR read_hba() could return list of
err_msgs as a pass by ref argument.

>
>>
>> 9. I am not able to understand, why does this patch need changes to
>> load_ident(). Sorry, if I have missed any previous discussion on this
>> topic.
>
>
> Earlier, the pg_hba.conf is loaded into the Postmastercontext, this is
> causing
> problems for this patch. So In the patch it is changed into
> currentmemorycontext
> and deleted the data at the end. The similar change is carried out for ident
> functionality, because of this reason, it is shown in this patch. May be I
> can
> separate that into an individual patch.

I think we need to include the hba related changes in this patch and
indent related changes should be moved to the other patch.


-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2016-11-29T09:40:20Z

On Tue, Nov 22, 2016 at 9:46 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

>
> It could be because of some un-initialised variable, which is
> initialized appropriately by default on your machine but not on my
> machine. I first applied your pg_hba_rules... patch, ran regression.
> It didn't crash. then I applied patch for discard_hba... and it
> started crashing. Does that give you any clue? Here's regression.out
> file for make installcheck. Here is error log snippet that shows a
> SIGSEGV there.
> 2016-11-22 15:47:11.939 IST [86206] LOG:  worker process: parallel
> worker for PID 86779 (PID 86780) was terminated by signal 11:
> Segmentation fault
> 2016-11-22 15:47:11.939 IST [86206] LOG:  terminating any other active
> server processes
> 2016-11-22 15:47:11.939 IST [86779] WARNING:  terminating connection
> because of crash of another server process
> 2016-11-22 15:47:11.939 IST [86779] DETAIL:  The postmaster has
> commanded this server process to roll back the current transaction and
> exit, because another server process exited abnormally and possibly
> corrupted shared memory.
>
> Applying those patches in any order doesn't matter.
>

I am not able to reproduce the crash both in debug and release mode
builds with both check and installcheck options.

Can you please share the back trace of the crash, so that it will be helpful
for me to locate the problem.


Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2016-11-29T10:15:19Z

Here's backtrace and some debugging information
Program terminated with signal 11, Segmentation fault.
#0  0x00000000007f96cd in shm_mq_sendv (mqh=0x121e998,
iov=0x7ffc9b7b79f0, iovcnt=2, nowait=1 '\001') at shm_mq.c:357
357        Assert(mq->mq_sender == MyProc);
(gdb) where
#0  0x00000000007f96cd in shm_mq_sendv (mqh=0x121e998,
iov=0x7ffc9b7b79f0, iovcnt=2, nowait=1 '\001') at shm_mq.c:357
#1  0x00000000006d8387 in mq_putmessage (msgtype=88 'X', s=0x0, len=0)
at pqmq.c:165
#2  0x0000000000515147 in ParallelWorkerMain (main_arg=141900502) at
parallel.c:1120
#3  0x0000000000783063 in StartBackgroundWorker () at bgworker.c:726
#4  0x0000000000795b77 in do_start_bgworker (rw=0x1216f00) at postmaster.c:5535
#5  0x0000000000795e4f in maybe_start_bgworker () at postmaster.c:5710
#6  0x0000000000794eb3 in sigusr1_handler (postgres_signal_arg=10) at
postmaster.c:4959
#7  <signal handler called>
#8  0x00002b005933a693 in select () from /lib/x86_64-linux-gnu/libc.so.6
#9  0x0000000000790720 in ServerLoop () at postmaster.c:1665
#10 0x000000000078fe76 in PostmasterMain (argc=8, argv=0x11eef40) at
postmaster.c:1309
#11 0x00000000006d8f1d in main (argc=8, argv=0x11eef40) at main.c:228
(gdb) p mq->mq_sender
Cannot access memory at address 0x6b636568635f707d
(gdb) p mq
$1 = (shm_mq *) 0x6b636568635f706d

Looking at this, I am wondering, how could that happen with your
patches. But every time I have tried to apply your patches and run
regression, I get this crash. Just now I tried the patches on a all
new repository and reproduced the crash.

On Tue, Nov 29, 2016 at 3:10 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
>
>
> On Tue, Nov 22, 2016 at 9:46 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>
>>
>> It could be because of some un-initialised variable, which is
>> initialized appropriately by default on your machine but not on my
>> machine. I first applied your pg_hba_rules... patch, ran regression.
>> It didn't crash. then I applied patch for discard_hba... and it
>> started crashing. Does that give you any clue? Here's regression.out
>> file for make installcheck. Here is error log snippet that shows a
>> SIGSEGV there.
>> 2016-11-22 15:47:11.939 IST [86206] LOG:  worker process: parallel
>> worker for PID 86779 (PID 86780) was terminated by signal 11:
>> Segmentation fault
>> 2016-11-22 15:47:11.939 IST [86206] LOG:  terminating any other active
>> server processes
>> 2016-11-22 15:47:11.939 IST [86779] WARNING:  terminating connection
>> because of crash of another server process
>> 2016-11-22 15:47:11.939 IST [86779] DETAIL:  The postmaster has
>> commanded this server process to roll back the current transaction and
>> exit, because another server process exited abnormally and possibly
>> corrupted shared memory.
>>
>> Applying those patches in any order doesn't matter.
>
>
> I am not able to reproduce the crash both in debug and release mode
> builds with both check and installcheck options.
>
> Can you please share the back trace of the crash, so that it will be helpful
> for me to locate the problem.
>
>
> Regards,
> Hari Babu
> Fujitsu Australia



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-04T03:54:16Z

On Tue, Nov 29, 2016 at 9:15 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> Here's backtrace and some debugging information
> Program terminated with signal 11, Segmentation fault.
> #0  0x00000000007f96cd in shm_mq_sendv (mqh=0x121e998,
> iov=0x7ffc9b7b79f0, iovcnt=2, nowait=1 '\001') at shm_mq.c:357
> 357        Assert(mq->mq_sender == MyProc);
> (gdb) where
> #0  0x00000000007f96cd in shm_mq_sendv (mqh=0x121e998,
> iov=0x7ffc9b7b79f0, iovcnt=2, nowait=1 '\001') at shm_mq.c:357
> #1  0x00000000006d8387 in mq_putmessage (msgtype=88 'X', s=0x0, len=0)
> at pqmq.c:165
> #2  0x0000000000515147 in ParallelWorkerMain (main_arg=141900502) at
> parallel.c:1120
> #3  0x0000000000783063 in StartBackgroundWorker () at bgworker.c:726
> #4  0x0000000000795b77 in do_start_bgworker (rw=0x1216f00) at
> postmaster.c:5535
> #5  0x0000000000795e4f in maybe_start_bgworker () at postmaster.c:5710
> #6  0x0000000000794eb3 in sigusr1_handler (postgres_signal_arg=10) at
> postmaster.c:4959
> #7  <signal handler called>
> #8  0x00002b005933a693 in select () from /lib/x86_64-linux-gnu/libc.so.6
> #9  0x0000000000790720 in ServerLoop () at postmaster.c:1665
> #10 0x000000000078fe76 in PostmasterMain (argc=8, argv=0x11eef40) at
> postmaster.c:1309
> #11 0x00000000006d8f1d in main (argc=8, argv=0x11eef40) at main.c:228
> (gdb) p mq->mq_sender
> Cannot access memory at address 0x6b636568635f707d
> (gdb) p mq
> $1 = (shm_mq *) 0x6b636568635f706d
>
> Looking at this, I am wondering, how could that happen with your
> patches. But every time I have tried to apply your patches and run
> regression, I get this crash. Just now I tried the patches on a all
> new repository and reproduced the crash.


I am also able to reproduce the crash once, but I didn't find the
reason why I leads to crash if I change the loading of hba and ident
files under currentmemory context instead of postmaster context.


>> Reused the error string once, as in this patch it chances in many places
>> compared
>> to pg_file_settings, so I tend to reuse it.
>
>Thanks. Although the new change might affect the way we translate the
>messages in other languages. I am not sure. So, I will leave it for
>someone with more knowledge to review.

There is no problem to the translation, because i kept those messages
under _(), so translations will pick those messages.

>What we may want to do, is separate the logic of reading the hba rules
>in a list and the logic to update existing rules in two different
>functions e.g read_hba() and load_hba(). hba_rules() can use
>read_hba() with ignore errors flag to get the list of hba lines. It
>can then use this list to create tuples to be returned in hba_rules().
>load_hba() will read_hba() with memory reset on error flag (same
>boolean) to read the list of hba lines and update the saved rules if
>there's no error. err_msg can be either a field in HbaLine, which will
>be used only by hba_rules() OR read_hba() could return list of
>err_msgs as a pass by ref argument.

Because of the above context problem, I just needs some part of the
code to read the pg_hba.conf under current memory context, so changed
the logic into a separate function to read the hba rules under currentmemory
context.

Latest patch is attached.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Simon Riggs <simon@2ndquadrant.com> — 2017-01-04T20:10:45Z

On 4 January 2017 at 03:54, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:

> Latest patch is attached.

The "method" column should be called "auth" or "auth_method" or "authentication"

I think we should have some tests, but I'll hear your views on that.
Perhaps we can include a test/sample pg_hba.conf for use in tests.

Since we've had crashes, I suggest running the test 10000 times and
checks for leaks and crashes.

If its safe we can move towards commit. Thanks

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

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-05T04:58:58Z

On Thu, Jan 5, 2017 at 5:10 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On 4 January 2017 at 03:54, Haribabu Kommi <kommi.haribabu@gmail.com> wrote:
>
>> Latest patch is attached.
>
> The "method" column should be called "auth" or "auth_method" or "authentication"
>
> I think we should have some tests, but I'll hear your views on that.
> Perhaps we can include a test/sample pg_hba.conf for use in tests.
>
> Since we've had crashes, I suggest running the test 10000 times and
> checks for leaks and crashes.
>
> If its safe we can move towards commit. Thanks

Could you hold on a bit to commit that? I'd like to look at it in more
details. At quick glance, there is for example no need to use
CreateTemplateTupleDesc and list the columns both in pg_proc.h and the
C routine itself. And memset() can be used in fill_hba_line for the
error code path.
-- 
Michael

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-09T00:45:55Z

On Tue, Nov 29, 2016 at 9:15 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> Here's backtrace and some debugging information
> Program terminated with signal 11, Segmentation fault.
> #0  0x00000000007f96cd in shm_mq_sendv (mqh=0x121e998,
> iov=0x7ffc9b7b79f0, iovcnt=2, nowait=1 '\001') at shm_mq.c:357
> 357        Assert(mq->mq_sender == MyProc);
> (gdb) where
> #0  0x00000000007f96cd in shm_mq_sendv (mqh=0x121e998,
> iov=0x7ffc9b7b79f0, iovcnt=2, nowait=1 '\001') at shm_mq.c:357
> #1  0x00000000006d8387 in mq_putmessage (msgtype=88 'X', s=0x0, len=0)
> at pqmq.c:165
> #2  0x0000000000515147 in ParallelWorkerMain (main_arg=141900502) at
> parallel.c:1120
> #3  0x0000000000783063 in StartBackgroundWorker () at bgworker.c:726
> #4  0x0000000000795b77 in do_start_bgworker (rw=0x1216f00) at
> postmaster.c:5535
> #5  0x0000000000795e4f in maybe_start_bgworker () at postmaster.c:5710
> #6  0x0000000000794eb3 in sigusr1_handler (postgres_signal_arg=10) at
> postmaster.c:4959
> #7  <signal handler called>
> #8  0x00002b005933a693 in select () from /lib/x86_64-linux-gnu/libc.so.6
> #9  0x0000000000790720 in ServerLoop () at postmaster.c:1665
> #10 0x000000000078fe76 in PostmasterMain (argc=8, argv=0x11eef40) at
> postmaster.c:1309
> #11 0x00000000006d8f1d in main (argc=8, argv=0x11eef40) at main.c:228
> (gdb) p mq->mq_sender
> Cannot access memory at address 0x6b636568635f707d
> (gdb) p mq
> $1 = (shm_mq *) 0x6b636568635f706d
>

I found the reason to the crash. This is because of new discard_hba() call
that
is added in InitPostgres after authentication.

The PostmasterContext is deleted and set it to NULL for all children
processes
except normal backend process. But because of addition of discard_hba()
function
call in InitPostgres, the parsed_hba_context is checked for NULL and freed.
For
all other childrens except normal backend, this pointer is not NULL and it
leads to
freeing of some other memory and that leads to the crash of the parallel
worker.

The freeing of parsed_hba_context memory is required only for normal backend
processes after authentication, so moved the discard_hba() function call
into the
if block solved the problem.

But anyway the logic of reading hba rules is changed for pg_hba_rules view,
so
this patch is not required anyway. Just for reference I attached updated
patch.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-10T07:35:13Z

On Thu, Jan 5, 2017 at 1:58 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Could you hold on a bit to commit that? I'd like to look at it in more
> details. At quick glance, there is for example no need to use
> CreateTemplateTupleDesc and list the columns both in pg_proc.h and the
> C routine itself. And memset() can be used in fill_hba_line for the
> error code path.

And here we go.

+<programlisting>
+postgres=# select * from pg_hba_rules;
[... large example ...]
+
+</programlisting>
It would be nice to reduce the width of this example. That's not going
to be friendly with the generated html.

+       switch (hba->conntype)
+       {
+           case ctLocal:
+               values[index] = CStringGetTextDatum("local");
+               break;
+           case ctHost:
+               values[index] = CStringGetTextDatum("host");
+               break;
+           case ctHostSSL:
+               values[index] = CStringGetTextDatum("hostssl");
+               break;
+           case ctHostNoSSL:
+               values[index] = CStringGetTextDatum("hostnossl");
+               break;
+           default:
+               elog(ERROR, "unexpected connection type in parsed HBA entry");
+               break;
+       }
Here let's remove the break clause and let compilers catch problem
when they show up.

+   if (hba->pamservice)
+   {
+       initStringInfo(&str);
+       appendStringInfoString(&str, "pamservice=");
+       appendStringInfoString(&str, hba->pamservice);
+       options[noptions++] = CStringGetTextDatum(str.data);
+   }
There is a bunch of code duplication here. I think that it would make
more sense to encapsulate that into a routine, at least let's use
appendStringInfo and let's group the two calls together.

+/* LDAP supports 10 currently, keep this well above the most any
method needs */
+#define MAX_OPTIONS 12
Er, why? There is an assert already, that should be enough.

=# \d pg_hba_rules
               View "pg_catalog.pg_hba_rules"
      Column      |  Type   | Collation | Nullable | Default
------------------+---------+-----------+----------+---------
 line_number      | integer |           |          |
 type             | text    |           |          |
 keyword_database | text[]  |           |          |
 database         | text[]  |           |          |
 keyword_user     | text[]  |           |          |
 user_name        | text[]  |           |          |
 keyword_address  | text    |           |          |
 address          | inet    |           |          |
 netmask          | inet    |           |          |
 hostname         | text    |           |          |
 method           | text    |           |          |
 options          | text[]  |           |          |
 error            | text    |           |          |
keyword_database and database map actually to the same thing if you
refer to a raw pg_hba.conf file because they have the same meaning for
user. You could simplify the view and just remove keyword_database,
keyword_user and keyword_address. This would simplify your patch as
well with all hte mumbo-jumbo to see if a string is a dedicated
keyword or not. In its most simple shape pg_hba_rules should show to
the user as an exact map of the entries of the raw file.

I have copied the example file of pg_hba.conf, reloaded it:
https://www.postgresql.org/docs/devel/static/auth-pg-hba-conf.html
And then the output result gets corrupted by showing up free()'d results:
null   | null    | \x7F\x7F\x7F\x7F\x7F

+   if (err_msg)
+   {
+       /* type */
+       index++;
+       nulls[index] = true;
[... long sequence ...]
Please let's use MemSet here and remove this large chunk of code...

+   if (!superuser())
+       ereport(ERROR,
+               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+                (errmsg("must be superuser to view pg_hba.conf settings"))));
Access to the function is already revoked, so what's the point of this
superuser check?

+   tupdesc = CreateTemplateTupleDesc(NUM_PG_HBA_LOOKUP_ATTS, false);
+   TupleDescInitEntry(tupdesc, (AttrNumber) 1, "line_number",
+                      INT4OID, -1, 0);
There is no need to list all the columns here. You can just use
get_call_result_type() and be done with it as the types and columns
names are already listed in the pg_proc entry of the new function.
-- 
Michael

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-17T01:19:40Z

On Tue, Jan 10, 2017 at 6:35 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Thu, Jan 5, 2017 at 1:58 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> > Could you hold on a bit to commit that? I'd like to look at it in more
> > details. At quick glance, there is for example no need to use
> > CreateTemplateTupleDesc and list the columns both in pg_proc.h and the
> > C routine itself. And memset() can be used in fill_hba_line for the
> > error code path.
>
> And here we go.
>

Thanks for the review.


> +<programlisting>
> +postgres=# select * from pg_hba_rules;
> [... large example ...]
> +
> +</programlisting>
> It would be nice to reduce the width of this example. That's not going
> to be friendly with the generated html.
>

Added a small example.

+       switch (hba->conntype)
> +       {
> +           case ctLocal:
> +               values[index] = CStringGetTextDatum("local");
> +               break;
> +           case ctHost:
> +               values[index] = CStringGetTextDatum("host");
> +               break;
> +           case ctHostSSL:
> +               values[index] = CStringGetTextDatum("hostssl");
> +               break;
> +           case ctHostNoSSL:
> +               values[index] = CStringGetTextDatum("hostnossl");
> +               break;
> +           default:
> +               elog(ERROR, "unexpected connection type in parsed HBA
> entry");
> +               break;
> +       }
> Here let's remove the break clause and let compilers catch problem
> when they show up.
>

Removed.


> +   if (hba->pamservice)
> +   {
> +       initStringInfo(&str);
> +       appendStringInfoString(&str, "pamservice=");
> +       appendStringInfoString(&str, hba->pamservice);
> +       options[noptions++] = CStringGetTextDatum(str.data);
> +   }
> There is a bunch of code duplication here. I think that it would make
> more sense to encapsulate that into a routine, at least let's use
> appendStringInfo and let's group the two calls together.
>

Use a new function to reduce the repeated lines of code.


> +/* LDAP supports 10 currently, keep this well above the most any
> method needs */
> +#define MAX_OPTIONS 12
> Er, why? There is an assert already, that should be enough.
>

Which Assert? This macro is used to verify that the maximum number
of authentication options that are possible for a single hba line.



> =# \d pg_hba_rules
>                View "pg_catalog.pg_hba_rules"
>       Column      |  Type   | Collation | Nullable | Default
> ------------------+---------+-----------+----------+---------
>  line_number      | integer |           |          |
>  type             | text    |           |          |
>  keyword_database | text[]  |           |          |
>  database         | text[]  |           |          |
>  keyword_user     | text[]  |           |          |
>  user_name        | text[]  |           |          |
>  keyword_address  | text    |           |          |
>  address          | inet    |           |          |
>  netmask          | inet    |           |          |
>  hostname         | text    |           |          |
>  method           | text    |           |          |
>  options          | text[]  |           |          |
>  error            | text    |           |          |
> keyword_database and database map actually to the same thing if you
> refer to a raw pg_hba.conf file because they have the same meaning for
> user. You could simplify the view and just remove keyword_database,
> keyword_user and keyword_address. This would simplify your patch as
> well with all hte mumbo-jumbo to see if a string is a dedicated
> keyword or not. In its most simple shape pg_hba_rules should show to
> the user as an exact map of the entries of the raw file.
>

I removed keyword_database and keyword_user columns where the data
in those columns can easily represent with the database and user columns.
But for address filed can contains keywords such as "same host" and etc and
also a hostname also. Because of this reason, this filed is converted into
3 columns in the view.

I have copied the example file of pg_hba.conf, reloaded it:
> https://www.postgresql.org/docs/devel/static/auth-pg-hba-conf.html
> And then the output result gets corrupted by showing up free()'d results:
> null   | null    | \x7F\x7F\x7F\x7F\x7F
>

There was a problem in resetting the error string, working with attached
patch.


> +   if (err_msg)
> +   {
> +       /* type */
> +       index++;
> +       nulls[index] = true;
> [... long sequence ...]
> Please let's use MemSet here and remove this large chunk of code..
>

Done.


> +   if (!superuser())
> +       ereport(ERROR,
> +               (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +                (errmsg("must be superuser to view pg_hba.conf
> settings"))));
> Access to the function is already revoked, so what's the point of this
> superuser check?
>

Removed.


>
> +   tupdesc = CreateTemplateTupleDesc(NUM_PG_HBA_LOOKUP_ATTS, false);
> +   TupleDescInitEntry(tupdesc, (AttrNumber) 1, "line_number",
> +                      INT4OID, -1, 0);
> There is no need to list all the columns here. You can just use
> get_call_result_type() and be done with it as the types and columns
> names are already listed in the pg_proc entry of the new function.]


Done.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-17T06:24:21Z

On Tue, Jan 17, 2017 at 10:19 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Tue, Jan 10, 2017 at 6:35 PM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>> +/* LDAP supports 10 currently, keep this well above the most any
>> method needs */
>> +#define MAX_OPTIONS 12
>> Er, why? There is an assert already, that should be enough.
>
>
> Which Assert? This macro is used to verify that the maximum number
> of authentication options that are possible for a single hba line.

That one:
+   Assert(noptions <= MAX_OPTIONS);
+   if (noptions)
+       return PointerGetDatum(
+               construct_array(options, noptions, TEXTOID, -1, false, 'i'));

>> =# \d pg_hba_rules
>>                View "pg_catalog.pg_hba_rules"
>>       Column      |  Type   | Collation | Nullable | Default
>> ------------------+---------+-----------+----------+---------
>>  line_number      | integer |           |          |
>>  type             | text    |           |          |
>>  keyword_database | text[]  |           |          |
>>  database         | text[]  |           |          |
>>  keyword_user     | text[]  |           |          |
>>  user_name        | text[]  |           |          |
>>  keyword_address  | text    |           |          |
>>  address          | inet    |           |          |
>>  netmask          | inet    |           |          |
>>  hostname         | text    |           |          |
>>  method           | text    |           |          |
>>  options          | text[]  |           |          |
>>  error            | text    |           |          |
>> keyword_database and database map actually to the same thing if you
>> refer to a raw pg_hba.conf file because they have the same meaning for
>> user. You could simplify the view and just remove keyword_database,
>> keyword_user and keyword_address. This would simplify your patch as
>> well with all hte mumbo-jumbo to see if a string is a dedicated
>> keyword or not. In its most simple shape pg_hba_rules should show to
>> the user as an exact map of the entries of the raw file.
>
> I removed keyword_database and keyword_user columns where the data
> in those columns can easily represent with the database and user columns.
> But for address filed can contains keywords such as "same host" and etc and
> also a hostname also. Because of this reason, this field is converted into
> 3 columns in the view.

Hm. We could as well consider cidr and use just one column. But still,
the use of inet as a data type in a system view looks like a wrong
choice to me. Or we could actually just use text... Opinions from
others are welcome here of course.

>> I have copied the example file of pg_hba.conf, reloaded it:
>> https://www.postgresql.org/docs/devel/static/auth-pg-hba-conf.html
>> And then the output result gets corrupted by showing up free()'d results:
>> null   | null    | \x7F\x7F\x7F\x7F\x7F
>
> There was a problem in resetting the error string, working with attached
> patch.

Thanks. Now that works.

> Updated patch attached.

This begins to look good. I have found a couple of minor issues.

+  <para>
+   The <structname>pg_hba_rules</structname> view can be read only by
+   superusers.
+  </para>
This is not true anymore.

+     <entry>
+      Line number within client authentication configuration file
+      the current value was set at
+     </entry>
I'd tune that without a past sentence. Saying just pg_hba.conf would
be fine perhaps?

+    <row>
+     <entry><structfield>database</structfield></entry>
+     <entry><structfield>text[]</structfield></entry>
+     <entry>List of database name</entry>
+    </row>
This should be plural, database nameS.

+     <entry>
+      List of keyword address names,
+      name can be all, samehost and samenet
+     </entry>
Phrasing looks weird to me, what about "List of keyword address names,
whose values can be all, samehost or samenet", with <literal> markups.

+postgres=# select line_number, type, database, user_name, auth_method
from pg_hba_rules;
Nit: this could be upper-cased.

+static Datum
+getauthmethod(UserAuth auth_method)
+{
+ ...
+       default:
+           elog(ERROR, "unexpected authentication method in parsed HBA entry");
+           break;
+   }
I think that you should also remove the default clause here to catchup
errors at compilation.

+       switch (hba->conntype)
+       {
+           case ctLocal:
+               values[index] = CStringGetTextDatum("local");
+               break;
+           case ctHost:
+               values[index] = CStringGetTextDatum("host");
+               break;
+           case ctHostSSL:
+               values[index] = CStringGetTextDatum("hostssl");
+               break;
+           case ctHostNoSSL:
+               values[index] = CStringGetTextDatum("hostnossl");
+               break;
+           default:
+               elog(ERROR, "unexpected connection type in parsed HBA entry");
+       }
You could go without the default clause here as well.
-- 
Michael

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-18T07:11:50Z

On Tue, Jan 17, 2017 at 5:24 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Tue, Jan 17, 2017 at 10:19 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
> > On Tue, Jan 10, 2017 at 6:35 PM, Michael Paquier <
> michael.paquier@gmail.com>
> > wrote:
> >> +/* LDAP supports 10 currently, keep this well above the most any
> >> method needs */
> >> +#define MAX_OPTIONS 12
> >> Er, why? There is an assert already, that should be enough.
> >
> >
> > Which Assert? This macro is used to verify that the maximum number
> > of authentication options that are possible for a single hba line.
>
> That one:
> +   Assert(noptions <= MAX_OPTIONS);
> +   if (noptions)
> +       return PointerGetDatum(
> +               construct_array(options, noptions, TEXTOID, -1, false,
> 'i'));


Sorry, I didn't clearly understand of your comment. The MAX_OPTIONS
macro is used to fill the Datum array to generate the options text array
data.


> >> =# \d pg_hba_rules
> >>                View "pg_catalog.pg_hba_rules"
> >>       Column      |  Type   | Collation | Nullable | Default
> >> ------------------+---------+-----------+----------+---------
> >>  line_number      | integer |           |          |
> >>  type             | text    |           |          |
> >>  keyword_database | text[]  |           |          |
> >>  database         | text[]  |           |          |
> >>  keyword_user     | text[]  |           |          |
> >>  user_name        | text[]  |           |          |
> >>  keyword_address  | text    |           |          |
> >>  address          | inet    |           |          |
> >>  netmask          | inet    |           |          |
> >>  hostname         | text    |           |          |
> >>  method           | text    |           |          |
> >>  options          | text[]  |           |          |
> >>  error            | text    |           |          |
> >> keyword_database and database map actually to the same thing if you
> >> refer to a raw pg_hba.conf file because they have the same meaning for
> >> user. You could simplify the view and just remove keyword_database,
> >> keyword_user and keyword_address. This would simplify your patch as
> >> well with all hte mumbo-jumbo to see if a string is a dedicated
> >> keyword or not. In its most simple shape pg_hba_rules should show to
> >> the user as an exact map of the entries of the raw file.
> >
> > I removed keyword_database and keyword_user columns where the data
> > in those columns can easily represent with the database and user columns.
> > But for address filed can contains keywords such as "same host" and etc
> and
> > also a hostname also. Because of this reason, this field is converted
> into
> > 3 columns in the view.
>
> Hm. We could as well consider cidr and use just one column. But still,
> the use of inet as a data type in a system view looks like a wrong
> choice to me. Or we could actually just use text... Opinions from
> others are welcome here of course.
>

Changed to text datatype and merged address, keyword_address and hostname
into address column. The netmask is the extra column in the view.


> > Updated patch attached.
>
> This begins to look good. I have found a couple of minor issues.
>
> +  <para>
> +   The <structname>pg_hba_rules</structname> view can be read only by
> +   superusers.
> +  </para>
> This is not true anymore.
>

removed.

+     <entry>
> +      Line number within client authentication configuration file
> +      the current value was set at
> +     </entry>
> I'd tune that without a past sentence. Saying just pg_hba.conf would
> be fine perhaps?
>

changed to - "Line number of the client authentication rule in pg_hba.conf
file"


> +    <row>
> +     <entry><structfield>database</structfield></entry>
> +     <entry><structfield>text[]</structfield></entry>
> +     <entry>List of database name</entry>
> +    </row>
> This should be plural, database nameS.
>

corrected.

+     <entry>
> +      List of keyword address names,
> +      name can be all, samehost and samenet
> +     </entry>
> Phrasing looks weird to me, what about "List of keyword address names,
> whose values can be all, samehost or samenet", with <literal> markups.
>

corrected.

+postgres=# select line_number, type, database, user_name, auth_method
> from pg_hba_rules;
> Nit: this could be upper-cased.
>

corrected.

+static Datum
> +getauthmethod(UserAuth auth_method)
> +{
> + ...
> +       default:
> +           elog(ERROR, "unexpected authentication method in parsed HBA
> entry");
> +           break;
> +   }
> I think that you should also remove the default clause here to catchup
> errors at compilation.
>

removed.


> +       switch (hba->conntype)
> +       {
> +           case ctLocal:
> +               values[index] = CStringGetTextDatum("local");
> +               break;
> +           case ctHost:
> +               values[index] = CStringGetTextDatum("host");
> +               break;
> +           case ctHostSSL:
> +               values[index] = CStringGetTextDatum("hostssl");
> +               break;
> +           case ctHostNoSSL:
> +               values[index] = CStringGetTextDatum("hostnossl");
> +               break;
> +           default:
> +               elog(ERROR, "unexpected connection type in parsed HBA
> entry");
> +       }
> You could go without the default clause here as well.
>

removed.

updated patch attached.
Added tap tests patch also attached.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-19T05:08:10Z

On Wed, Jan 18, 2017 at 4:11 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> updated patch attached.

Thanks for the new version.

> Added tap tests patch also attached.

This begins to look really nice. I am having fun torturing it :)

Here are I think my last comments:

+   linecxt = tokenize_file(HbaFileName, file, &hba_lines,
&hba_line_nums, &hba_raw_lines);
+   FreeFile(file);
tokenize_file can leave on ERROR, in which case the file descriptor
would leak. You much likely need a
PG_END_ENSURE_ERROR_CLEANUP/PG_ENSURE_ERROR_CLEANUP block here with a
callback to FreeFile() if an error is caught.

+     <entry>
+      ADDRESS specifies the set of hosts the record matches.
+      It can be a host name, or it is made up of an IP address
+      or keywords such as (<literal>all</literal>,
+      <literal>samehost</literal> and <literal>samenet</literal>).
+     </entry>
Why is that upper-case?

+     <entry>
+      If not null, an error message indicating why this
+      rule could not be loaded
+     </entry>
Need a dot here, that's a sentence.

src/test/regress/expected/rules.out needs to be refreshed, regression
tests are failing.
-- 
Michael

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-19T07:25:57Z

On Thu, Jan 19, 2017 at 4:08 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Wed, Jan 18, 2017 at 4:11 PM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
> > updated patch attached.
>
> Thanks for the new version.
>
> > Added tap tests patch also attached.
>
> This begins to look really nice. I am having fun torturing it :)
>

Thanks for the review.

Here are I think my last comments:
>
> +   linecxt = tokenize_file(HbaFileName, file, &hba_lines,
> &hba_line_nums, &hba_raw_lines);
> +   FreeFile(file);
> tokenize_file can leave on ERROR, in which case the file descriptor
> would leak. You much likely need a
> PG_END_ENSURE_ERROR_CLEANUP/PG_ENSURE_ERROR_CLEANUP block here with a
> callback to FreeFile() if an error is caught.
>

Added the cleanup mechanism. But the tokenize_file() function call
present in many places, But in one flow still it is possible to have
file descriptor leak because of pg_hba_rules view. Because of this
reason, added the cleanup everywhere.


> +     <entry>
> +      ADDRESS specifies the set of hosts the record matches.
> +      It can be a host name, or it is made up of an IP address
> +      or keywords such as (<literal>all</literal>,
> +      <literal>samehost</literal> and <literal>samenet</literal>).
> +     </entry>
> Why is that upper-case?
>

Corrected.

+     <entry>
> +      If not null, an error message indicating why this
> +      rule could not be loaded
> +     </entry>
> Need a dot here, that's a sentence.
>

updated.

src/test/regress/expected/rules.out needs to be refreshed, regression
> tests are failing.
>

Corrected.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-19T07:56:45Z

On Thu, Jan 19, 2017 at 4:25 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> Added the cleanup mechanism. But the tokenize_file() function call
> present in many places, But in one flow still it is possible to have
> file descriptor leak because of pg_hba_rules view. Because of this
> reason, added the cleanup everywhere.

Oops, sorry. Actually you don't need that. AllocateFile() registers
the fd opened with the sub-transactions it is involved with... So if
there is an ERROR nothing leaks.
-- 
Michael

Re: pg_hba_file_settings view patch

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-01-19T12:28:08Z

On Thu, Jan 19, 2017 at 1:26 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Thu, Jan 19, 2017 at 4:25 PM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> Added the cleanup mechanism. But the tokenize_file() function call
>> present in many places, But in one flow still it is possible to have
>> file descriptor leak because of pg_hba_rules view. Because of this
>> reason, added the cleanup everywhere.
>
> Oops, sorry. Actually you don't need that. AllocateFile() registers
> the fd opened with the sub-transactions it is involved with... So if
> there is an ERROR nothing leaks.

I agree. If we need any fix, it should be a separate patch.

The patch is in much better shape than previous versions. Thanks for
working on it.

Here are some more review comments.
'indicates' should be used instead of 'indicating'
+  <para>
+   If the configuration file contains any problems,
<structfield>error</structfield> field
+   indicating the problem of that rule. Following is the sample
output of the view.
+  </para>
The first sentence may be rewritten as
<structfield>error</structfield> field, if not NULL, describes problem in
the rule on the line <structfield>line_number</structfield>.

Instead of showing same values like {all}, trust on multiple lines, you may
show an example with different values on different lines.
+<screen>
+ line_number | type  | database | user_name | auth_method
+-------------+-------+----------+-----------+-------------
+          84 | local | {all}    | {all}     | trust
+          86 | host  | {all}    | {all}     | trust
+          88 | host  | {all}    | {all}     | trust
+(3 rows)
+</screen>

getauthmethod() deparses the authentication tokens parsed in parse_hba_line()
starting with /* Get the authentication method */. There is less chance that
those tokens would be changed later, but we might need adjustments when new
methods are added or method names are changed. Instead, we might want to create
an array of token where nth token indicates auth_method = n. The code block in
parse_hba_line() can be changed to look up this array and assign index of the
token if found to auth_method. Token which are enabled by compiler flags will
be part of the array only when that flag is enabled, otherwise they will be
NULL.
#ifdef ENABLE_GSS
        parsedline->auth_method = uaGSS;
#else
        unsupauth = "gss";
#endif
If we do that getauthmethod() simply fetches the token by referencing array
with auth_method as index, with some special handling for uaImplicitReject.
This will take away any future maintenance needed. Something similar can be
done to conntype.

This is not going to help in binary without CASSERT i.e. for most users, if
they provide more than 12 options, albeit resulting in an error. Please convert
this into an elog() or another error that hba parser throws.
+    Assert(noptions <= MAX_OPTIONS);
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-19T12:32:56Z

On Thu, Jan 19, 2017 at 9:28 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Thu, Jan 19, 2017 at 1:26 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Thu, Jan 19, 2017 at 4:25 PM, Haribabu Kommi
>> <kommi.haribabu@gmail.com> wrote:
>>> Added the cleanup mechanism. But the tokenize_file() function call
>>> present in many places, But in one flow still it is possible to have
>>> file descriptor leak because of pg_hba_rules view. Because of this
>>> reason, added the cleanup everywhere.
>>
>> Oops, sorry. Actually you don't need that. AllocateFile() registers
>> the fd opened with the sub-transactions it is involved with... So if
>> there is an ERROR nothing leaks.
>
> I agree. If we need any fix, it should be a separate patch.

It happens that no fix is needed here. That was some useless fuss. Sorry.
-- 
Michael

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-20T01:56:17Z

On Thu, Jan 19, 2017 at 11:28 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> On Thu, Jan 19, 2017 at 1:26 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> > On Thu, Jan 19, 2017 at 4:25 PM, Haribabu Kommi
> > <kommi.haribabu@gmail.com> wrote:
> >> Added the cleanup mechanism. But the tokenize_file() function call
> >> present in many places, But in one flow still it is possible to have
> >> file descriptor leak because of pg_hba_rules view. Because of this
> >> reason, added the cleanup everywhere.
> >
> > Oops, sorry. Actually you don't need that. AllocateFile() registers
> > the fd opened with the sub-transactions it is involved with... So if
> > there is an ERROR nothing leaks.
>
> I agree. If we need any fix, it should be a separate patch.
>
> The patch is in much better shape than previous versions. Thanks for
> working on it.
>

Thanks for the review.

Here are some more review comments.
> 'indicates' should be used instead of 'indicating'
> +  <para>
> +   If the configuration file contains any problems,
> <structfield>error</structfield> field
> +   indicating the problem of that rule. Following is the sample
> output of the view.
> +  </para>
> The first sentence may be rewritten as
> <structfield>error</structfield> field, if not NULL, describes problem in
> the rule on the line <structfield>line_number</structfield>.
>

Changed accordingly.


> Instead of showing same values like {all}, trust on multiple lines, you may
> show an example with different values on different lines.
> +<screen>
> + line_number | type  | database | user_name | auth_method
> +-------------+-------+----------+-----------+-------------
> +          84 | local | {all}    | {all}     | trust
> +          86 | host  | {all}    | {all}     | trust
> +          88 | host  | {all}    | {all}     | trust
> +(3 rows)
> +</screen>
>

Added more rows with different options.

getauthmethod() deparses the authentication tokens parsed in
> parse_hba_line()
> starting with /* Get the authentication method */. There is less chance
> that
> those tokens would be changed later, but we might need adjustments when new
> methods are added or method names are changed. Instead, we might want to
> create
> an array of token where nth token indicates auth_method = n. The code
> block in
> parse_hba_line() can be changed to look up this array and assign index of
> the
> token if found to auth_method. Token which are enabled by compiler flags
> will
> be part of the array only when that flag is enabled, otherwise they will be
> NULL.
> #ifdef ENABLE_GSS
>         parsedline->auth_method = uaGSS;
> #else
>         unsupauth = "gss";
> #endif
> If we do that getauthmethod() simply fetches the token by referencing array
> with auth_method as index, with some special handling for uaImplicitReject.
> This will take away any future maintenance needed. Something similar can be
> done to conntype.
>

Thanks for the improvement suggestion.
I am thinking of whether is it really required, as because we rarely change,
the name of authentication option that is already exposed and also added new
options can easily found by the compiler in case if it is missed to add.



> This is not going to help in binary without CASSERT i.e. for most users, if
> they provide more than 12 options, albeit resulting in an error. Please
> convert
> this into an elog() or another error that hba parser throws.
> +    Assert(noptions <= MAX_OPTIONS);


No. In case if user provides more than 12 options that are invalid, during
the parsing
itself, it identifies that it is an invalid option and error string is
stored in error filed.

The Assert case can be hit only, when the user added to new options to
display
to the user through view but not updating the macro to the max number of
options
then, it can lead to that assert.

Updated patch attached including reverting of file leak changes.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-20T07:16:25Z

On Fri, Jan 20, 2017 at 10:56 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> The Assert case can be hit only, when the user added to new options to
> display
> to the user through view but not updating the macro to the max number of
> options then, it can lead to that assert.
>
> Updated patch attached including reverting of file leak changes.

OK, thanks for the new version. I am marking this version as ready for
committer.
-- 
Michael

Re: pg_hba_file_settings view patch

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-01-20T07:22:51Z

On Fri, Jan 20, 2017 at 12:46 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Fri, Jan 20, 2017 at 10:56 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> The Assert case can be hit only, when the user added to new options to
>> display
>> to the user through view but not updating the macro to the max number of
>> options then, it can lead to that assert.
>>
>> Updated patch attached including reverting of file leak changes.
>
> OK, thanks for the new version. I am marking this version as ready for
> committer.

I do intend to make a pass ASAP.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: pg_hba_file_settings view patch

Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-20T21:01:37Z

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> [ pg_hba_rules_10.patch ]

I took a quick look over this.

* I'm not exactly convinced that the way you approached the error message
reporting, ie duplicating the logged message, is good.  In particular
this results in localizing the strings reported in pg_hba_rules.error,
which is exactly opposite to the decision we reached for the
pg_file_settings view.  What's the reasoning for deciding that this
view should contain localized strings?  (More generally, we found in
the pg_file_settings view that we didn't always want to use exactly
the same string that was logged, anyway.)

* Also, there seems to be a lot of ereports remaining unconverted,
eg the "authentication file token too long" error.  One of the things
we wanted pg_file_settings to be able to do was finger pretty much any
mistake in the config file, including syntax errors.  It seems like
it'd be a shame if pg_hba_rules is unable to help with that.  You
should be able to fill in line number and error even if the line is
too mangled to be able to populate the other fields sanely.

* While we're on the comparison to pg_file_settings ... pg_hba_rules
is not the view name I'd guess if I guessed one based on that precedent.
I don't have a better suggestion offhand, but this name seems weirdly
inconsistent.

* I think "memcxt" as a field name is pretty unhelpful if you suppose
it just means "memory context", and outright misleading if you guess
it's, say, the context the tuplestore is in.  Perhaps call it "tmpcxt"
and add a comment like "Short-lived context, reset after each line".
The other fields of FillHbaLineCxt could do with comments too.

* ... although really, you've gone way overboard with temp contexts
here.  I don't think it's necessary to have a per-line context at all;
you could just do all the work in the single temp context that fill_hba
calls hbacxt, and drop it all at end of function, because no matter what
you'll be eating O(file size) space, and we're just quibbling over the
size of the multiplier.  Also, if you're concerned with reclaiming space
before end of query, aren't you leaking the tokenize_file output data?

* getauthmethod() might be better replaced with an array.  And doesn't it
produce uninitialized-variable warnings for you?

* It seems a little weird that fill_hba_auth_opt isn't inserting the "="
between name and value.  And should it be using psprintf?  It's the
only use of StringInfo in this file, so it looks a bit out of place.
Actually, I wonder if you wouldn't be better off replacing it with a
coding style like

	options[noptions++] =
	    CStringGetTextDatum(psprintf("ldapport=%d", hba->ldapport));

which seems more readable and more flexible.

* "MAX_OPTIONS" is, uh, mighty generic.  Maybe "MAX_HBA_OPTIONS"?
And the comment for it doesn't actually tell you what it is.

* NUM_PG_HBA_LOOKUP_ATTS seems like it ought to match the name of the
view, ie NUM_PG_HBA_RULES_ATTS if that doesn't get renamed.

* Usually we just write "if (listvar)" or "if (listvar != NIL)" rather
than "if (list_length(listvar) != 0)".  list_length() overspecifies what
you need to test.  This isn't as critical as it was back in the day when
list_length() cost O(N), but still the former is much more common project
style.

* Why is AllocateFile() failure only an ereport(LOG) in fill_hba()?
>From the user's viewpoint he'll get an empty view with no visible
reason.  Probably ereport(ERROR) is more sensible.  You could imagine
trying to show the error in the view, but that seems like more work
than the case warrants.

* Seems like the FillHbaLineCxt variable could just be a local struct
in hba_rules(), and dispense with one palloc/pfree cycle.

* I'm not really on board with patches modifying pgindent/typedefs.list
retail.  To my mind that file represents the typedefs used the last
time we pgindent'd the whole tree, and if you want an up-to-date list
you should ask the buildfarm.  Otherwise there's just too much confusion
stemming from the fact that not everybody updates it when patching.

My own workflow for reindenting patches goes more like
curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o my-typedefs.list
... manually edit my-typedefs.list to add any new typedefs from patch ...
pgindent --typedefs=my-typedefs.list target-files

			regards, tom lane

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-23T08:13:55Z

On Sat, Jan 21, 2017 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> > [ pg_hba_rules_10.patch ]
>
> I took a quick look over this.
>

Thanks for the review.


> * I'm not exactly convinced that the way you approached the error message
> reporting, ie duplicating the logged message, is good.  In particular
> this results in localizing the strings reported in pg_hba_rules.error,
> which is exactly opposite to the decision we reached for the
> pg_file_settings view.  What's the reasoning for deciding that this
> view should contain localized strings?  (More generally, we found in
> the pg_file_settings view that we didn't always want to use exactly
> the same string that was logged, anyway.)
>

Actually there is no particular reason to display the localized strings,
Just thought that it may be useful to the user if it get displayed in their
own language. And also doing this way will reduce the error message
duplicate in the code that is used for display in the view and writing it
into the log file.


> * Also, there seems to be a lot of ereports remaining unconverted,
> eg the "authentication file token too long" error.  One of the things
> we wanted pg_file_settings to be able to do was finger pretty much any
> mistake in the config file, including syntax errors.  It seems like
> it'd be a shame if pg_hba_rules is unable to help with that.  You
> should be able to fill in line number and error even if the line is
> too mangled to be able to populate the other fields sanely.
>

The two errors that are missed are, "could not open secondary
authentication file"
and "authentication file token too long" errors. For these two cases, the
server
is not throwing any error, it just logs the message and continues. Is it
fine to add
these these two cases as errors in the view?


> * While we're on the comparison to pg_file_settings ... pg_hba_rules
> is not the view name I'd guess if I guessed one based on that precedent.
> I don't have a better suggestion offhand, but this name seems weirdly
> inconsistent.
>

People are suggested to use "rules" instead of "settings", as the entries
in the pg_hba.conf are used as rules to control the client authentication
mechanism.


> * I think "memcxt" as a field name is pretty unhelpful if you suppose
> it just means "memory context", and outright misleading if you guess
> it's, say, the context the tuplestore is in.  Perhaps call it "tmpcxt"
> and add a comment like "Short-lived context, reset after each line".
> The other fields of FillHbaLineCxt could do with comments too.
>
> * ... although really, you've gone way overboard with temp contexts
> here.  I don't think it's necessary to have a per-line context at all;
> you could just do all the work in the single temp context that fill_hba
> calls hbacxt, and drop it all at end of function, because no matter what
> you'll be eating O(file size) space, and we're just quibbling over the
> size of the multiplier.  Also, if you're concerned with reclaiming space
> before end of query, aren't you leaking the tokenize_file output data?
>

Removed the temp context and done everything in a single context.


>
> * getauthmethod() might be better replaced with an array.  And doesn't it
> produce uninitialized-variable warnings for you?
>

No, i am not getting any warnings.
Changed to a static array.


>
> * It seems a little weird that fill_hba_auth_opt isn't inserting the "="
> between name and value.  And should it be using psprintf?  It's the
> only use of StringInfo in this file, so it looks a bit out of place.
> Actually, I wonder if you wouldn't be better off replacing it with a
> coding style like
>
>         options[noptions++] =
>             CStringGetTextDatum(psprintf("ldapport=%d", hba->ldapport));
>
> which seems more readable and more flexible.
>

Corrected accordingly.


>
> * "MAX_OPTIONS" is, uh, mighty generic.  Maybe "MAX_HBA_OPTIONS"?
> And the comment for it doesn't actually tell you what it is.
>

Updated.


>
> * NUM_PG_HBA_LOOKUP_ATTS seems like it ought to match the name of the
> view, ie NUM_PG_HBA_RULES_ATTS if that doesn't get renamed.
>

Updated to the current name.


> * Usually we just write "if (listvar)" or "if (listvar != NIL)" rather
> than "if (list_length(listvar) != 0)".  list_length() overspecifies what
> you need to test.  This isn't as critical as it was back in the day when
> list_length() cost O(N), but still the former is much more common project
> style.
>

Corrected.

* Why is AllocateFile() failure only an ereport(LOG) in fill_hba()?
> From the user's viewpoint he'll get an empty view with no visible
> reason.  Probably ereport(ERROR) is more sensible.  You could imagine
> trying to show the error in the view, but that seems like more work
> than the case warrants.
>

Corrected.


> * Seems like the FillHbaLineCxt variable could just be a local struct
> in hba_rules(), and dispense with one palloc/pfree cycle.
>

Removed the FillHbaLineCxt structure itself, after removing the
memory context variable, it just have two variables, directly passed
them as an arguments.


> * I'm not really on board with patches modifying pgindent/typedefs.list
> retail.  To my mind that file represents the typedefs used the last
> time we pgindent'd the whole tree, and if you want an up-to-date list
> you should ask the buildfarm.  Otherwise there's just too much confusion
> stemming from the fact that not everybody updates it when patching.
>
> My own workflow for reindenting patches goes more like
> curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o
> my-typedefs.list
> ... manually edit my-typedefs.list to add any new typedefs from patch ...
> pgindent --typedefs=my-typedefs.list target-files
>

Ok. Thanks for the information. I followed the above steps for the
indentation.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-24T07:17:12Z

On Mon, Jan 23, 2017 at 5:13 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Sat, Jan 21, 2017 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> * I'm not exactly convinced that the way you approached the error message
>> reporting, ie duplicating the logged message, is good.  In particular
>> this results in localizing the strings reported in pg_hba_rules.error,
>> which is exactly opposite to the decision we reached for the
>> pg_file_settings view.  What's the reasoning for deciding that this
>> view should contain localized strings?  (More generally, we found in
>> the pg_file_settings view that we didn't always want to use exactly
>> the same string that was logged, anyway.)
>
> Actually there is no particular reason to display the localized strings,
> Just thought that it may be useful to the user if it get displayed in their
> own language. And also doing this way will reduce the error message
> duplicate in the code that is used for display in the view and writing it
> into the log file.

Perhaps consistency would not hurt and something like
record_config_file_error() could be done to save the error parsing
error. What's actually the problem with localized strings exposed in a
system view? Encoding conflicts?

>> * Also, there seems to be a lot of ereports remaining unconverted,
>> eg the "authentication file token too long" error.  One of the things
>> we wanted pg_file_settings to be able to do was finger pretty much any
>> mistake in the config file, including syntax errors.  It seems like
>> it'd be a shame if pg_hba_rules is unable to help with that.  You
>> should be able to fill in line number and error even if the line is
>> too mangled to be able to populate the other fields sanely.
>
> The two errors that are missed are, "could not open secondary authentication
> file"
> and "authentication file token too long" errors. For these two cases, the
> server
> is not throwing any error, it just logs the message and continues. Is it
> fine to add
> these these two cases as errors in the view?

Missed those ones during the initial review... It would be a good idea
to include them to track problems.
-- 
Michael

Re: pg_hba_file_settings view patch

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-01-24T14:19:57Z

On Mon, Jan 23, 2017 at 1:43 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
>
>
> On Sat, Jan 21, 2017 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
>> > [ pg_hba_rules_10.patch ]
>>
>> I took a quick look over this.
>
>
> Thanks for the review.
>
>>
>> * I'm not exactly convinced that the way you approached the error message
>> reporting, ie duplicating the logged message, is good.  In particular
>> this results in localizing the strings reported in pg_hba_rules.error,
>> which is exactly opposite to the decision we reached for the
>> pg_file_settings view.  What's the reasoning for deciding that this
>> view should contain localized strings?  (More generally, we found in
>> the pg_file_settings view that we didn't always want to use exactly
>> the same string that was logged, anyway.)
>
>
> Actually there is no particular reason to display the localized strings,
> Just thought that it may be useful to the user if it get displayed in their
> own language. And also doing this way will reduce the error message
> duplicate in the code that is used for display in the view and writing it
> into the log file.
>

Would it be better, if we could parse each HBA line within
PG_TRY()/PG_CATCH() and read errmsg from errordata stack in
PG_CATCH()? We do that only when errcode is ERRCODE_CONFIG_FILE_ERROR,
PG_THROWing otherwise. That's probably a bad idea but wanted to put it
out as it came to me. It would eliminate a lot of changes in this
patch.


>> * getauthmethod() might be better replaced with an array.  And doesn't it
>> produce uninitialized-variable warnings for you?
>
>
> No, i am not getting any warnings.
> Changed to a static array.

Thanks. Probably we should update parse_hba_line() to keep it in sync
with the array. But that may be a separate add-on patch.

Rest of the patch looks good to me.
-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-25T01:04:58Z

On Tue, Jan 24, 2017 at 11:19 PM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Mon, Jan 23, 2017 at 1:43 PM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>>
>>
>> On Sat, Jan 21, 2017 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>
>>> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
>>> > [ pg_hba_rules_10.patch ]
>>>
>>> I took a quick look over this.
>>
>>
>> Thanks for the review.
>>
>>>
>>> * I'm not exactly convinced that the way you approached the error message
>>> reporting, ie duplicating the logged message, is good.  In particular
>>> this results in localizing the strings reported in pg_hba_rules.error,
>>> which is exactly opposite to the decision we reached for the
>>> pg_file_settings view.  What's the reasoning for deciding that this
>>> view should contain localized strings?  (More generally, we found in
>>> the pg_file_settings view that we didn't always want to use exactly
>>> the same string that was logged, anyway.)
>>
>>
>> Actually there is no particular reason to display the localized strings,
>> Just thought that it may be useful to the user if it get displayed in their
>> own language. And also doing this way will reduce the error message
>> duplicate in the code that is used for display in the view and writing it
>> into the log file.
>>
>
> Would it be better, if we could parse each HBA line within
> PG_TRY()/PG_CATCH() and read errmsg from errordata stack in
> PG_CATCH()? We do that only when errcode is ERRCODE_CONFIG_FILE_ERROR,
> PG_THROWing otherwise. That's probably a bad idea but wanted to put it
> out as it came to me. It would eliminate a lot of changes in this
> patch.

It still needs to save the error message string somewhere. So I am not
sure that it would save much patch size.
-- 
Michael

Re: pg_hba_file_settings view patch

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-01-25T03:50:41Z

On Wed, Jan 25, 2017 at 6:34 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Jan 24, 2017 at 11:19 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> On Mon, Jan 23, 2017 at 1:43 PM, Haribabu Kommi
>> <kommi.haribabu@gmail.com> wrote:
>>>
>>>
>>> On Sat, Jan 21, 2017 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>>>
>>>> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
>>>> > [ pg_hba_rules_10.patch ]
>>>>
>>>> I took a quick look over this.
>>>
>>>
>>> Thanks for the review.
>>>
>>>>
>>>> * I'm not exactly convinced that the way you approached the error message
>>>> reporting, ie duplicating the logged message, is good.  In particular
>>>> this results in localizing the strings reported in pg_hba_rules.error,
>>>> which is exactly opposite to the decision we reached for the
>>>> pg_file_settings view.  What's the reasoning for deciding that this
>>>> view should contain localized strings?  (More generally, we found in
>>>> the pg_file_settings view that we didn't always want to use exactly
>>>> the same string that was logged, anyway.)
>>>
>>>
>>> Actually there is no particular reason to display the localized strings,
>>> Just thought that it may be useful to the user if it get displayed in their
>>> own language. And also doing this way will reduce the error message
>>> duplicate in the code that is used for display in the view and writing it
>>> into the log file.
>>>
>>
>> Would it be better, if we could parse each HBA line within
>> PG_TRY()/PG_CATCH() and read errmsg from errordata stack in
>> PG_CATCH()? We do that only when errcode is ERRCODE_CONFIG_FILE_ERROR,
>> PG_THROWing otherwise. That's probably a bad idea but wanted to put it
>> out as it came to me. It would eliminate a lot of changes in this
>> patch.
>
> It still needs to save the error message string somewhere. So I am not
> sure that it would save much patch size.

My understanding is that ereport (and some other calls included in
that statement) call saves it on errordata stack before jumping to the
handler.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-25T04:28:29Z

On Wed, Jan 25, 2017 at 2:50 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

> On Wed, Jan 25, 2017 at 6:34 AM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
> > On Tue, Jan 24, 2017 at 11:19 PM, Ashutosh Bapat
> > <ashutosh.bapat@enterprisedb.com> wrote:
> >> On Mon, Jan 23, 2017 at 1:43 PM, Haribabu Kommi
> >> <kommi.haribabu@gmail.com> wrote:
> >>>
> >>>
> >>> On Sat, Jan 21, 2017 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >>>>
> >>>> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> >>>> > [ pg_hba_rules_10.patch ]
> >>>>
> >>>> I took a quick look over this.
> >>>
> >>>
> >>> Thanks for the review.
> >>>
> >>>>
> >>>> * I'm not exactly convinced that the way you approached the error
> message
> >>>> reporting, ie duplicating the logged message, is good.  In particular
> >>>> this results in localizing the strings reported in pg_hba_rules.error,
> >>>> which is exactly opposite to the decision we reached for the
> >>>> pg_file_settings view.  What's the reasoning for deciding that this
> >>>> view should contain localized strings?  (More generally, we found in
> >>>> the pg_file_settings view that we didn't always want to use exactly
> >>>> the same string that was logged, anyway.)
> >>>
> >>>
> >>> Actually there is no particular reason to display the localized
> strings,
> >>> Just thought that it may be useful to the user if it get displayed in
> their
> >>> own language. And also doing this way will reduce the error message
> >>> duplicate in the code that is used for display in the view and writing
> it
> >>> into the log file.
> >>>
> >>
> >> Would it be better, if we could parse each HBA line within
> >> PG_TRY()/PG_CATCH() and read errmsg from errordata stack in
> >> PG_CATCH()? We do that only when errcode is ERRCODE_CONFIG_FILE_ERROR,
> >> PG_THROWing otherwise. That's probably a bad idea but wanted to put it
> >> out as it came to me. It would eliminate a lot of changes in this
> >> patch.
> >
> > It still needs to save the error message string somewhere. So I am not
> > sure that it would save much patch size.
>
> My understanding is that ereport (and some other calls included in
> that statement) call saves it on errordata stack before jumping to the
> handler.


All the ereport messages of level are LOG, because of this reason, because
of this reason even if we use the TRY/CATCH, it doesn't work.  As the
messages gets printed to the logfile and continue to process the next
statement.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-25T04:46:37Z

On Tue, Jan 24, 2017 at 6:17 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Mon, Jan 23, 2017 at 5:13 PM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
> > On Sat, Jan 21, 2017 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> * I'm not exactly convinced that the way you approached the error
> message
> >> reporting, ie duplicating the logged message, is good.  In particular
> >> this results in localizing the strings reported in pg_hba_rules.error,
> >> which is exactly opposite to the decision we reached for the
> >> pg_file_settings view.  What's the reasoning for deciding that this
> >> view should contain localized strings?  (More generally, we found in
> >> the pg_file_settings view that we didn't always want to use exactly
> >> the same string that was logged, anyway.)
> >
> > Actually there is no particular reason to display the localized strings,
> > Just thought that it may be useful to the user if it get displayed in
> their
> > own language. And also doing this way will reduce the error message
> > duplicate in the code that is used for display in the view and writing it
> > into the log file.
>
> Perhaps consistency would not hurt and something like
> record_config_file_error() could be done to save the error parsing
> error. What's actually the problem with localized strings exposed in a
> system view? Encoding conflicts?
>
> >> * Also, there seems to be a lot of ereports remaining unconverted,
> >> eg the "authentication file token too long" error.  One of the things
> >> we wanted pg_file_settings to be able to do was finger pretty much any
> >> mistake in the config file, including syntax errors.  It seems like
> >> it'd be a shame if pg_hba_rules is unable to help with that.  You
> >> should be able to fill in line number and error even if the line is
> >> too mangled to be able to populate the other fields sanely.
> >
> > The two errors that are missed are, "could not open secondary
> authentication
> > file"
> > and "authentication file token too long" errors. For these two cases, the
> > server
> > is not throwing any error, it just logs the message and continues. Is it
> > fine to add
> > these these two cases as errors in the view?
>
> Missed those ones during the initial review... It would be a good idea
> to include them to track problems.
>

The above mentioned two error logs that occur in the tokenize_file function.
Currently during the loading of pg_hba.conf file, it just logs the above two
problems and continue to load the file.

Currently, I added the errors for the cases, where the server will stop
proceeding
because of these errors. Those are mostly in parse_hba_line function.

To enhance error reporting of failures in tokenize_file also, the
tokenize_file should
return errors along with line_nums and those lines should be ignored in
processing
the parse_hba_line function. To do that, the tokenize_file should return
whenever
it encounters above those two errors only in pg_hba_rules case, but not for
normal
scenario.

Is it fine to proceed with the changes?

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2017-01-25T04:47:17Z

On Wed, Jan 25, 2017 at 9:58 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
>
>
> On Wed, Jan 25, 2017 at 2:50 PM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>>
>> On Wed, Jan 25, 2017 at 6:34 AM, Michael Paquier
>> <michael.paquier@gmail.com> wrote:
>> > On Tue, Jan 24, 2017 at 11:19 PM, Ashutosh Bapat
>> > <ashutosh.bapat@enterprisedb.com> wrote:
>> >> On Mon, Jan 23, 2017 at 1:43 PM, Haribabu Kommi
>> >> <kommi.haribabu@gmail.com> wrote:
>> >>>
>> >>>
>> >>> On Sat, Jan 21, 2017 at 8:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> >>>>
>> >>>> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
>> >>>> > [ pg_hba_rules_10.patch ]
>> >>>>
>> >>>> I took a quick look over this.
>> >>>
>> >>>
>> >>> Thanks for the review.
>> >>>
>> >>>>
>> >>>> * I'm not exactly convinced that the way you approached the error
>> >>>> message
>> >>>> reporting, ie duplicating the logged message, is good.  In particular
>> >>>> this results in localizing the strings reported in
>> >>>> pg_hba_rules.error,
>> >>>> which is exactly opposite to the decision we reached for the
>> >>>> pg_file_settings view.  What's the reasoning for deciding that this
>> >>>> view should contain localized strings?  (More generally, we found in
>> >>>> the pg_file_settings view that we didn't always want to use exactly
>> >>>> the same string that was logged, anyway.)
>> >>>
>> >>>
>> >>> Actually there is no particular reason to display the localized
>> >>> strings,
>> >>> Just thought that it may be useful to the user if it get displayed in
>> >>> their
>> >>> own language. And also doing this way will reduce the error message
>> >>> duplicate in the code that is used for display in the view and writing
>> >>> it
>> >>> into the log file.
>> >>>
>> >>
>> >> Would it be better, if we could parse each HBA line within
>> >> PG_TRY()/PG_CATCH() and read errmsg from errordata stack in
>> >> PG_CATCH()? We do that only when errcode is ERRCODE_CONFIG_FILE_ERROR,
>> >> PG_THROWing otherwise. That's probably a bad idea but wanted to put it
>> >> out as it came to me. It would eliminate a lot of changes in this
>> >> patch.
>> >
>> > It still needs to save the error message string somewhere. So I am not
>> > sure that it would save much patch size.
>>
>> My understanding is that ereport (and some other calls included in
>> that statement) call saves it on errordata stack before jumping to the
>> handler.
>
>
> All the ereport messages of level are LOG, because of this reason, because
> of this reason even if we use the TRY/CATCH, it doesn't work.  As the
> messages gets printed to the logfile and continue to process the next
> statement.

Right. Sorry for missing to mention about this change in the patch.
Originally the messages are at level ERROR so TRY/CATCH will be able
to catch it. We will need to somehow then turn ERROR to LOG and
re-throw it. I haven't tried it myself though.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: pg_hba_file_settings view patch

Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-25T17:32:18Z

Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> On Wed, Jan 25, 2017 at 9:58 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
>> All the ereport messages of level are LOG, because of this reason, because
>> of this reason even if we use the TRY/CATCH, it doesn't work.  As the
>> messages gets printed to the logfile and continue to process the next
>> statement.

> Right. Sorry for missing to mention about this change in the patch.
> Originally the messages are at level ERROR so TRY/CATCH will be able
> to catch it. We will need to somehow then turn ERROR to LOG and
> re-throw it. I haven't tried it myself though.

I do not think throwing/catching errors is a good idea here.  It will mean
that the view can't report more than one mistake per run, and it will
create a significant difference in the parsing code's control flow between
"normal" and "read for view" modes, which is a recipe for bugs.  Also,
it's different from the way things are done for the pg_file_settings view.
For the sake of future developers, I think we should make this work as
much like that view as we can.

The way I'd be inclined to make the individual reporting changes is like

             if (!EnableSSL)
+            {
-               ereport(LOG,
+               ereport(elevel,
                         (errcode(ERRCODE_CONFIG_FILE_ERROR),
                errmsg("hostssl record cannot match because SSL is disabled"),
                          errhint("Set ssl = on in postgresql.conf."),
                          errcontext("line %d of configuration file \"%s\"",
                                     line_num, HbaFileName)));
+                *err_msg = pstrdup("hostssl record cannot match because SSL is disabled");
+            }

which is considerably less invasive and hence easier to review, and
supports reporting different text in the view than appears in the log,
should we need that.  It seems likely also that we could drop the pstrdup
in the case of constant strings (we'd still need psprintf if we want to
insert values into the view messages), which would make this way cheaper
than what's in the patch now.

			regards, tom lane

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-26T02:17:41Z

On Thu, Jan 26, 2017 at 2:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The way I'd be inclined to make the individual reporting changes is like
>
>              if (!EnableSSL)
> +            {
> -               ereport(LOG,
> +               ereport(elevel,
>                          (errcode(ERRCODE_CONFIG_FILE_ERROR),
>                 errmsg("hostssl record cannot match because SSL is disabled"),
>                           errhint("Set ssl = on in postgresql.conf."),
>                           errcontext("line %d of configuration file \"%s\"",
>                                      line_num, HbaFileName)));
> +                *err_msg = pstrdup("hostssl record cannot match because SSL is disabled");
> +            }
>
> which is considerably less invasive and hence easier to review, and
> supports reporting different text in the view than appears in the log,
> should we need that.  It seems likely also that we could drop the pstrdup
> in the case of constant strings (we'd still need psprintf if we want to
> insert values into the view messages), which would make this way cheaper
> than what's in the patch now.

I don't really understand the argument about readability of the patch
as what Haribabu has proposed is simply to avoid a duplicate of the
strings and the diffs of the patch are really clear. For the sake of
not translating the strings sent back to the system view though I can
buy it.
-- 
Michael

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-26T12:45:22Z

On Thu, Jan 26, 2017 at 4:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> writes:
> > On Wed, Jan 25, 2017 at 9:58 AM, Haribabu Kommi
> > <kommi.haribabu@gmail.com> wrote:
> >> All the ereport messages of level are LOG, because of this reason,
> because
> >> of this reason even if we use the TRY/CATCH, it doesn't work.  As the
> >> messages gets printed to the logfile and continue to process the next
> >> statement.
>
> > Right. Sorry for missing to mention about this change in the patch.
> > Originally the messages are at level ERROR so TRY/CATCH will be able
> > to catch it. We will need to somehow then turn ERROR to LOG and
> > re-throw it. I haven't tried it myself though.
>
> I do not think throwing/catching errors is a good idea here.  It will mean
> that the view can't report more than one mistake per run, and it will
> create a significant difference in the parsing code's control flow between
> "normal" and "read for view" modes, which is a recipe for bugs.  Also,
> it's different from the way things are done for the pg_file_settings view.
> For the sake of future developers, I think we should make this work as
> much like that view as we can.
>
> The way I'd be inclined to make the individual reporting changes is like
>
>              if (!EnableSSL)
> +            {
> -               ereport(LOG,
> +               ereport(elevel,
>                          (errcode(ERRCODE_CONFIG_FILE_ERROR),
>                 errmsg("hostssl record cannot match because SSL is
> disabled"),
>                           errhint("Set ssl = on in postgresql.conf."),
>                           errcontext("line %d of configuration file
> \"%s\"",
>                                      line_num, HbaFileName)));
> +                *err_msg = pstrdup("hostssl record cannot match because
> SSL is disabled");
> +            }
>
> which is considerably less invasive and hence easier to review, and
> supports reporting different text in the view than appears in the log,
> should we need that.  It seems likely also that we could drop the pstrdup
> in the case of constant strings (we'd still need psprintf if we want to
> insert values into the view messages), which would make this way cheaper
> than what's in the patch now.
>

Updated patch attached as per the above modifications.

This patch currently doesn't have the code for reporting the two log
messages
that can occur in tokenize_file function. To support the same, I am
thinking of
changing line_nums list to line_info list that can contain both line number
and
the error message that occurred during the tokenize. This list data is used
to identify whether that line is having any error or not before parsing
that hba
line, and directly report that line as error in the view.

Any comments/suggestions in proceeding with that implementation.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-26T14:36:31Z

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> This patch currently doesn't have the code for reporting the two log
> messages that can occur in tokenize_file function. To support the same,
> I am thinking of changing line_nums list to line_info list that can
> contain both line number and the error message that occurred during the
> tokenize. This list data is used to identify whether that line is having
> any error or not before parsing that hba line, and directly report that
> line as error in the view.

Yeah, perhaps.  tokenize_file() has pushed the return-parallel-lists
notion to the limit of sanity already.  It would make more sense to
change it to return a single list containing one struct per line,
which would include the token list, raw line text, and line number.

It might make sense to proceed by writing a separate patch that just
refactors the existing code to have an API like that, and then revise
this patch to add an error message field to the per-line struct.  Or
maybe that's just extra work, not sure.

			regards, tom lane

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-27T02:26:58Z

On Thu, Jan 26, 2017 at 11:36 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
>> This patch currently doesn't have the code for reporting the two log
>> messages that can occur in tokenize_file function. To support the same,
>> I am thinking of changing line_nums list to line_info list that can
>> contain both line number and the error message that occurred during the
>> tokenize. This list data is used to identify whether that line is having
>> any error or not before parsing that hba line, and directly report that
>> line as error in the view.
>
> Yeah, perhaps.  tokenize_file() has pushed the return-parallel-lists
> notion to the limit of sanity already.  It would make more sense to
> change it to return a single list containing one struct per line,
> which would include the token list, raw line text, and line number.
>
> It might make sense to proceed by writing a separate patch that just
> refactors the existing code to have an API like that, and then revise
> this patch to add an error message field to the per-line struct.  Or
> maybe that's just extra work, not sure.

Beginning with a cleaner state the feature implementation would likely
facilitate the restructuring work of pg_hba_rules and its overall
size, so doing the refactoring work first would make the most sense.
-- 
Michael

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-27T15:35:48Z

On Fri, Jan 27, 2017 at 1:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> > This patch currently doesn't have the code for reporting the two log
> > messages that can occur in tokenize_file function. To support the same,
> > I am thinking of changing line_nums list to line_info list that can
> > contain both line number and the error message that occurred during the
> > tokenize. This list data is used to identify whether that line is having
> > any error or not before parsing that hba line, and directly report that
> > line as error in the view.
>
> Yeah, perhaps.  tokenize_file() has pushed the return-parallel-lists
> notion to the limit of sanity already.  It would make more sense to
> change it to return a single list containing one struct per line,
> which would include the token list, raw line text, and line number.
>
> It might make sense to proceed by writing a separate patch that just
> refactors the existing code to have an API like that, and then revise
> this patch to add an error message field to the per-line struct.  Or
> maybe that's just extra work, not sure.
>

Here I attached tokenize_file refactor patch to return single linked list
that contains a structure and rebased pg_hba_rules patch on top it
by adding an error message to that structure to hold the errors occurred
during tokenization..

I came up with TokenizedLline as a structure name that works with all
configuration files and member names (I hope). If it needs any better
names please let me know.

Updated patches are attached.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-27T18:47:47Z

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> On Fri, Jan 27, 2017 at 1:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> It might make sense to proceed by writing a separate patch that just
>> refactors the existing code to have an API like that, and then revise
>> this patch to add an error message field to the per-line struct.  Or
>> maybe that's just extra work, not sure.

> Here I attached tokenize_file refactor patch to return single linked list
> that contains a structure and rebased pg_hba_rules patch on top it
> by adding an error message to that structure to hold the errors occurred
> during tokenization..

I pushed the first patch with some revisions.  You had the TokenizedLine
struct containing something that was still a three-level-nesting list,
whereas it only needs to be two levels, and you hadn't updated any of
the comments that the patch falsified.  Also, I figured we might as well
pass the TokenizedLine struct as-is to parse_hba_line and
parse_ident_line, because that was going to be the next step anyway
so they could pass back error messages.

Off to look at the second patch ...

			regards, tom lane

Re: pg_hba_file_settings view patch

Robert Haas <robertmhaas@gmail.com> — 2017-01-27T20:58:58Z

On Fri, Jan 20, 2017 at 4:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> * I'm not really on board with patches modifying pgindent/typedefs.list
> retail.  To my mind that file represents the typedefs used the last
> time we pgindent'd the whole tree, and if you want an up-to-date list
> you should ask the buildfarm.  Otherwise there's just too much confusion
> stemming from the fact that not everybody updates it when patching.
>
> My own workflow for reindenting patches goes more like
> curl https://buildfarm.postgresql.org/cgi-bin/typedefs.pl -o my-typedefs.list
> ... manually edit my-typedefs.list to add any new typedefs from patch ...
> pgindent --typedefs=my-typedefs.list target-files

Andres and I -- among others -- have been patching typedefs.list
retail for a while now.  I think it makes life a lot easier.

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

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-27T22:48:10Z

On Sat, Jan 28, 2017 at 5:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> > On Fri, Jan 27, 2017 at 1:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> It might make sense to proceed by writing a separate patch that just
> >> refactors the existing code to have an API like that, and then revise
> >> this patch to add an error message field to the per-line struct.  Or
> >> maybe that's just extra work, not sure.
>
> > Here I attached tokenize_file refactor patch to return single linked list
> > that contains a structure and rebased pg_hba_rules patch on top it
> > by adding an error message to that structure to hold the errors occurred
> > during tokenization..
>
> I pushed the first patch with some revisions.  You had the TokenizedLine
> struct containing something that was still a three-level-nesting list,
> whereas it only needs to be two levels, and you hadn't updated any of
> the comments that the patch falsified.  Also, I figured we might as well
> pass the TokenizedLine struct as-is to parse_hba_line and
> parse_ident_line, because that was going to be the next step anyway
> so they could pass back error messages.
>

sorry for missing to update comments. I also thought of reducing the list
level after sending the patch.

Off to look at the second patch ...
>

Used TokenizeLine->err_msg variable only to return the error message
from parse_hba_line.

Attached a rebased patch on the latest master hopefully.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-27T23:28:06Z

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> [ pg_hba_rules_13.patch ]

I spent awhile hacking on this, and made a lot of things better, but
I'm still very unhappy about the state of the comments.  You changed
the APIs of a bunch of functions, often into fairly subtle things,
and you did not touch even one of their API-specification comments.
As an example, next_token() now needs something like

"On error, log a message at ereport level elevel and set *err_msg to
an error string.  Note that the return value might be either true or
false after an error; *err_msg must be checked to determine that.
Hence, *err_msg had better be NULL on entry, or you won't be able
to tell."

Having to write such a thing might even convince you that you should
try a little harder to make the behavior less confusing.  Just adding
arguments, and not changing the result-value specification, is not
necessarily the best way to do this.

I haven't looked at the docs yet.

I'm still not very happy about the choice of view name ...

			regards, tom lane

Re: pg_hba_file_settings view patch

Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-28T16:55:32Z

I wrote:
> I'm still not very happy about the choice of view name ...

After looking over this thread again, I think that we should go with
pg_file_hba_rules or perhaps pg_hba_file_rules.  I see that options
like that were discussed and rejected earlier, but I feel the arguments
against were based on false premises.  I think we need "file" in the
name because:

1. It makes the analogy to the pg_file_settings view clearer.

2. It emphasizes that what you see in the view is the contents of
the disk files, not necessarily the active rules.

3. It leaves the door open to use "pg_hba_rules" as the name of some
future view that *does* show the active rules, analogously to pg_settings
which does show the active GUC settings.

I realize that there's no very convenient way to implement a true
active-auth-rules view right now, but it's not hard to see how that
could be fixed if we were motivated to do so.  One simple way would
be for the postmaster, any time it had successfully loaded the hba
file, to write out some representation of the parsed data into an
"active auth rules" file.  I doubt anyone would bother if the only
application were an active-rules view, but there's at least one
other reason to do this, which is that we could make the HBA stuff
work the same on Windows as it does elsewhere.  Right now, since
new EXEC_BACKEND backends must read the HBA files for themselves,
Windows does not have the property that pg_hba.conf is read only
at SIGHUP --- break the file with some fat-fingered editing, and
new connections begin to fail instantly.  But if new backends
always read a postmaster-written file, then the behavior would be
the same as it is on Unix.

			regards, tom lane

Re: pg_hba_file_settings view patch

Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-28T22:18:48Z

I wrote:
> I spent awhile hacking on this, and made a lot of things better, but
> I'm still very unhappy about the state of the comments.

I made another pass over this, working on the comments and the docs,
and changing the view name to "pg_hba_file_rules".  I think this version
is committable if people are satisfied with that name.

One loose end is what to do about testing.  I did not much like the
proposed TAP tests.  We could just put "select count(*) > 0 from
pg_hba_file_rules" into the main regression tests, which would provide
some code coverage there, if not very much guarantee that what the view
outputs is sane.

			regards, tom lane

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-30T02:20:30Z

On Sun, Jan 29, 2017 at 9:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I wrote:
> > I spent awhile hacking on this, and made a lot of things better, but
> > I'm still very unhappy about the state of the comments.
>
> I made another pass over this, working on the comments and the docs,
> and changing the view name to "pg_hba_file_rules".  I think this version
> is committable if people are satisfied with that name.
>

Thanks for working on the patch. I am fine with the "pg_hba_file_rules"
name. I have to improve in writing better comments after checking the
attached patch. I will improve the comments in further patch submissions
to community.

> One loose end is what to do about testing.  I did not much like the
> proposed TAP tests.  We could just put "select count(*) > 0 from
> pg_hba_file_rules" into the main regression tests, which would provide
> some code coverage there, if not very much guarantee that what the view
> outputs is sane.
>

I added the test in main regression test to the patch which you shared based
on the mail of creating separate tests for system views in [1]. The
attached
needs to be applied on top the patch shared in [1].

[1] - https://www.postgresql.org/message-id/19359.1485723741%40sss.pgh.pa.us

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-30T06:18:48Z

On Mon, Jan 30, 2017 at 11:20 AM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
> On Sun, Jan 29, 2017 at 9:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> tgl wrote:
>> > I spent awhile hacking on this, and made a lot of things better, but
>> > I'm still very unhappy about the state of the comments.
>>
>> I made another pass over this, working on the comments and the docs,
>> and changing the view name to "pg_hba_file_rules".  I think this version
>> is committable if people are satisfied with that name.

(catching up with this thread as a lot has happened.)

> Thanks for working on the patch. I am fine with the "pg_hba_file_rules"
> name. I have to improve in writing better comments after checking the
> attached patch. I will improve the comments in further patch submissions
> to community.

No objections here.

+/*
+ * The following character array represents the names of the authentication
+ * methods that are supported by PostgreSQL.
+ *
+ * Note: keep this in sync with the UserAuth enum in hba.h.
+ */
+static const char *const UserAuthName[] =
+{
+   "reject",
+   "implicit reject",          /* Not a user-visible option */
+   "trust",
+   "ident",
+   "password",
+   "md5",
+   "gss",
+   "sspi",
+   "pam",
+   "bsd",
+   "ldap",
+   "cert",
+   "radius",
+   "peer"
+};
Perhaps this could use a StaticAssertStmt()? Say something like that:
#define USER_AUTH_LAST uaPeer
StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
    "UserAuthName must include all user authentication names");

Any updates could easily be forgotten.
-- 
Michael

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-30T12:51:06Z

On Mon, Jan 30, 2017 at 5:18 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

> On Mon, Jan 30, 2017 at 11:20 AM, Haribabu Kommi
> <kommi.haribabu@gmail.com> wrote:
> > On Sun, Jan 29, 2017 at 9:18 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> tgl wrote:
> >> > I spent awhile hacking on this, and made a lot of things better, but
> >> > I'm still very unhappy about the state of the comments.
> >>
> >> I made another pass over this, working on the comments and the docs,
> >> and changing the view name to "pg_hba_file_rules".  I think this version
> >> is committable if people are satisfied with that name.
>
> (catching up with this thread as a lot has happened.)
>
> > Thanks for working on the patch. I am fine with the "pg_hba_file_rules"
> > name. I have to improve in writing better comments after checking the
> > attached patch. I will improve the comments in further patch submissions
> > to community.
>
> No objections here.
>
> +/*
> + * The following character array represents the names of the
> authentication
> + * methods that are supported by PostgreSQL.
> + *
> + * Note: keep this in sync with the UserAuth enum in hba.h.
> + */
> +static const char *const UserAuthName[] =
> +{
> +   "reject",
> +   "implicit reject",          /* Not a user-visible option */
> +   "trust",
> +   "ident",
> +   "password",
> +   "md5",
> +   "gss",
> +   "sspi",
> +   "pam",
> +   "bsd",
> +   "ldap",
> +   "cert",
> +   "radius",
> +   "peer"
> +};
> Perhaps this could use a StaticAssertStmt()? Say something like that:
> #define USER_AUTH_LAST uaPeer
> StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
>     "UserAuthName must include all user authentication names");
>
> Any updates could easily be forgotten.
>

Thanks for the review. Added the static assert statement.

Updated patch attached.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Tom Lane <tgl@sss.pgh.pa.us> — 2017-01-30T23:04:27Z

Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> On Mon, Jan 30, 2017 at 5:18 PM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>> #define USER_AUTH_LAST uaPeer
>> StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
>> "UserAuthName must include all user authentication names");

> Thanks for the review. Added the static assert statement.

This isn't exactly bulletproof, since somebody could add another enum
value and forget to update the macro.  Still, it's better than nothing.
I tried to make it a shade more idiot-proof by putting the #define
physically inside the enum list --- you'd have to really have blinders
on to not notice it there.  (Not that people haven't made equally silly
mistakes :-(.)

Pushed with that adjustment.  Thanks for working on this!

			regards, tom lane

Re: pg_hba_file_settings view patch

Haribabu Kommi <kommi.haribabu@gmail.com> — 2017-01-31T03:55:20Z

On Tue, Jan 31, 2017 at 10:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
> > On Mon, Jan 30, 2017 at 5:18 PM, Michael Paquier <
> michael.paquier@gmail.com>
> > wrote:
> >> #define USER_AUTH_LAST uaPeer
> >> StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
> >> "UserAuthName must include all user authentication names");
>
> > Thanks for the review. Added the static assert statement.
>
> This isn't exactly bulletproof, since somebody could add another enum
> value and forget to update the macro.  Still, it's better than nothing.
> I tried to make it a shade more idiot-proof by putting the #define
> physically inside the enum list --- you'd have to really have blinders
> on to not notice it there.  (Not that people haven't made equally silly
> mistakes :-(.)
>
> Pushed with that adjustment.  Thanks for working on this!
>

Thanks for your support.

Regards,
Hari Babu
Fujitsu Australia

Re: pg_hba_file_settings view patch

Michael Paquier <michael.paquier@gmail.com> — 2017-01-31T04:17:43Z

On Tue, Jan 31, 2017 at 12:55 PM, Haribabu Kommi
<kommi.haribabu@gmail.com> wrote:
>
>
> On Tue, Jan 31, 2017 at 10:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Haribabu Kommi <kommi.haribabu@gmail.com> writes:
>> > On Mon, Jan 30, 2017 at 5:18 PM, Michael Paquier
>> > <michael.paquier@gmail.com>
>> > wrote:
>> >> #define USER_AUTH_LAST uaPeer
>> >> StaticAssertStmt(lengthof(UserAuthName) == USER_AUTH_LAST + 1,
>> >> "UserAuthName must include all user authentication names");
>>
>> > Thanks for the review. Added the static assert statement.
>>
>> This isn't exactly bulletproof, since somebody could add another enum
>> value and forget to update the macro.  Still, it's better than nothing.
>> I tried to make it a shade more idiot-proof by putting the #define
>> physically inside the enum list --- you'd have to really have blinders
>> on to not notice it there.  (Not that people haven't made equally silly
>> mistakes :-(.)
>>
>> Pushed with that adjustment.  Thanks for working on this!
>
>
> Thanks for your support.

The modifications looks fine for me, thanks for adding the assertion.
-- 
Michael