Re: [HACKERS] pg_hba_file_settings view patch
От | Haribabu Kommi |
---|---|
Тема | Re: [HACKERS] pg_hba_file_settings view patch |
Дата | |
Msg-id | CAJrrPGdEg_BuC_21BMrM3bPimYKcmHp3ZZxaWMXnAOczaQdY0g@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [HACKERS] pg_hba_file_settings view patch (Michael Paquier <michael.paquier@gmail.com>) |
Ответы |
Re: [HACKERS] pg_hba_file_settings view patch
(Michael Paquier <michael.paquier@gmail.com>)
|
Список | pgsql-hackers |
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.
> also a hostname also. Because of this reason, this field is converted into>> =# \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
> 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.
updated patch attached.
Added tap tests patch also attached.
Regards,
Hari Babu
Fujitsu Australia
Вложения
В списке pgsql-hackers по дате отправления: