Fwd: Keywords in pg_hba.conf should be field-specific

On 16 June 2011 00:22, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> I try to apply your patch, but it is finished with some failed hinks.
>
> Please, can you refresh your patch

Hi Pavel,

Thanks for taking a look.  I have attached v2 of the patch, as against
current HEAD.  I've also added the new patch to the CF app.  I look
forward to your comments.

Cheers,
BJ

Excerpts from Brendan Jurd's message of vie jun 17 19:31:41 -0400 2011:
> On 16 June 2011 00:22, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> > I try to apply your patch, but it is finished with some failed hinks.
> >
> > Please, can you refresh your patch
>
> Hi Pavel,
>
> Thanks for taking a look.  I have attached v2 of the patch, as against
> current HEAD.  I've also added the new patch to the CF app.  I look
> forward to your comments.

Is this really a WIP patch? I'm playing a bit with it currently, seems
fairly sane.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 18 June 2011 13:43, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> Is this really a WIP patch?  I'm playing a bit with it currently, seems
> fairly sane.
>

In this case, the WIP designation is meant to convey "warning: only
casual testing has beeen done". I tried it out with various
permutations of pg_hba.conf, and it worked as advertised in those
tests, but I have not made any attempt to formulate a more rigorous
testing regimen.

In particular I haven't tested that the more exotic authentication
methods still work properly, and I can't recall whether I tested
recursive file inclusion and group membership.

Is that a wrongful use of the WIP designation?

Cheers,
BJ

Hello Brendan,

I checked your patch, it is applied cleanly and I don't see any mayor
problem. This patch does all what is expected.

I have two minor comments

a) you don't use macro "token_matches" consistently

func: parse_hba_line

<------>if (strcmp(token->string, "local") == 0)

should be
if (token_is_keyword(token, "local"))
...

I don't see any sense when somebody use a quotes there.

b) probably you can simplify a memory management using own two
persistent memory context - and you can swap it. Then functions like
free_hba_record, clean_hba_list, free_lines should be removed.

Regards

Pavel Stehule

2011/6/18 Brendan Jurd <direvus(at)gmail(dot)com>:
> On 18 June 2011 13:43, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
>> Is this really a WIP patch?  I'm playing a bit with it currently, seems
>> fairly sane.
>>
>
> In this case, the WIP designation is meant to convey "warning: only
> casual testing has beeen done".  I tried it out with various
> permutations of pg_hba.conf, and it worked as advertised in those
> tests, but I have not made any attempt to formulate a more rigorous
> testing regimen.
>
> In particular I haven't tested that the more exotic authentication
> methods still work properly, and I can't recall whether I tested
> recursive file inclusion and group membership.
>
> Is that a wrongful use of the WIP designation?
>
> Cheers,
> BJ
>

sorry

> a) you don't use macro "token_matches" consistently

should be

a) you don't use macro "token_is_keyword" consistently

it should be used for all keywords

Regards

Pavel Stehule

Excerpts from Pavel Stehule's message of lun jun 20 11:34:25 -0400 2011:

> b) probably you can simplify a memory management using own two
> persistent memory context - and you can swap it. Then functions like
> free_hba_record, clean_hba_list, free_lines should be removed.

Yeah, I reworked the patch with something like that over the weekend.
Not all of it though. I'll send an updated version shortly.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of lun jun 20 12:19:37 -0400 2011:
> Excerpts from Pavel Stehule's message of lun jun 20 11:34:25 -0400 2011:
>
> > b) probably you can simplify a memory management using own two
> > persistent memory context - and you can swap it. Then functions like
> > free_hba_record, clean_hba_list, free_lines should be removed.
>
> Yeah, I reworked the patch with something like that over the weekend.
> Not all of it though. I'll send an updated version shortly.

Here it is, please let me know what you think. I took the liberty of
cleaning up some things that were clearly historical leftovers.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 21 June 2011 06:06, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Alvaro Herrera's message of lun jun 20 12:19:37 -0400 2011:
>> Excerpts from Pavel Stehule's message of lun jun 20 11:34:25 -0400 2011:
>>
>> > b) probably you can simplify a memory management using own two
>> > persistent memory context - and you can swap it. Then functions like
>> > free_hba_record, clean_hba_list, free_lines should be removed.
>>
>> Yeah, I reworked the patch with something like that over the weekend.
>> Not all of it though.  I'll send an updated version shortly.
>
> Here it is, please let me know what you think.  I took the liberty of
> cleaning up some things that were clearly historical leftovers.
>

Okay, yeah, the MemoryContext approach is far more elegant than what I
had. Boy was I ever barking up the wrong tree.

Cheers,
BJ

Excerpts from Brendan Jurd's message of lun jun 20 20:06:39 -0400 2011:
> On 21 June 2011 06:06, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> > Excerpts from Alvaro Herrera's message of lun jun 20 12:19:37 -0400 2011:
> >> Excerpts from Pavel Stehule's message of lun jun 20 11:34:25 -0400 2011:
> >>
> >> > b) probably you can simplify a memory management using own two
> >> > persistent memory context - and you can swap it. Then functions like
> >> > free_hba_record, clean_hba_list, free_lines should be removed.
> >>
> >> Yeah, I reworked the patch with something like that over the weekend.
> >> Not all of it though.  I'll send an updated version shortly.
> >
> > Here it is, please let me know what you think.  I took the liberty of
> > cleaning up some things that were clearly historical leftovers.
> >
>
> Okay, yeah, the MemoryContext approach is far more elegant than what I
> had. Boy was I ever barking up the wrong tree.

Eh, whoever wrote the original code was barking up the same tree, so I
don't blame you for following the well-trodden path.

I realize I took out most of the fun of this patch from you, but -- are
you still planning to do some more exhaustive testing of it? I checked
some funny scenarios (including include files and groups) but it's not
all that unlikely that I missed some cases. I also didn't check any
auth method other than ident, md5 and trust, 'cause I don't have what's
required for anything else. (It's a pity that the regression tests
don't exercise anything else.)

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 21 June 2011 11:11, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> I realize I took out most of the fun of this patch from you, but -- are
> you still planning to do some more exhaustive testing of it?  I checked
> some funny scenarios (including include files and groups) but it's not
> all that unlikely that I missed some cases.  I also didn't check any
> auth method other than ident, md5 and trust, 'cause I don't have what's
> required for anything else.  (It's a pity that the regression tests
> don't exercise anything else.)

I've pretty much tested all the things I can think to test, and I'm in
the same boat as you re auth methods. If there are bugs, I think they
will be of the obscure kind, that only reveal themselves under
peculiar circumstances.

Cheers,
BJ

2011/6/20 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Alvaro Herrera's message of lun jun 20 12:19:37 -0400 2011:
>> Excerpts from Pavel Stehule's message of lun jun 20 11:34:25 -0400 2011:
>>
>> > b) probably you can simplify a memory management using own two
>> > persistent memory context - and you can swap it. Then functions like
>> > free_hba_record, clean_hba_list, free_lines should be removed.
>>
>> Yeah, I reworked the patch with something like that over the weekend.
>> Not all of it though.  I'll send an updated version shortly.
>
> Here it is, please let me know what you think.  I took the liberty of
> cleaning up some things that were clearly historical leftovers.
>

I have one question. I can't find any rules for work with tokens, etc,
where is quotes allowed and disallowed?

I don't see any other issues.

Regards

Pavel Stehule

> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

On 21 June 2011 13:51, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> I have one question. I can't find any rules for work with tokens, etc,
> where is quotes allowed and disallowed?
>
> I don't see any other issues.

I'm not sure I understand your question, but quotes are allowed
anywhere and they always act to remove any special meaning the token
might otherwise have had. It's much like quoting a column name in
SQL.

I didn't change any of the hba parsing rules, so escaping and whatnot
should all work the way it did before. The only difference should be
that after parsing, keywords will only be evaluated for fields where
they matter.

Cheers,
BJ

2011/6/21 Brendan Jurd <direvus(at)gmail(dot)com>:
> On 21 June 2011 13:51, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> I have one question. I can't find any rules for work with tokens, etc,
>> where is quotes allowed and disallowed?
>>
>> I don't see any other issues.
>
> I'm not sure I understand your question, but quotes are allowed
> anywhere and they always act to remove any special meaning the token
> might otherwise have had.  It's much like quoting a column name in
> SQL.

I don't understand to using a macro

#define token_is_keyword(t, k) (!t->quoted && strcmp(t->string, k) == 0)

because you disallowed a quoting?

Regards

Pavel

>
> I didn't change any of the hba parsing rules, so escaping and whatnot
> should all work the way it did before.  The only difference should be
> that after parsing, keywords will only be evaluated for fields where
> they matter.
>
> Cheers,
> BJ
>

On 21 June 2011 14:34, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> I don't understand to using a macro
>
> #define token_is_keyword(t, k)  (!t->quoted && strcmp(t->string, k) == 0)
>
> because you disallowed a quoting?

Well, a token can only be treated as a special keyword if it is unquoted.

As an example, in the 'database' field, the bare token 'replication'
is a keyword meaning the pseudo-database for streaming rep. Whereas
the quoted token "replication" would mean a real database which is
called 'replication'.

Likewise, the bare token 'all' in the username field is a keyword --
it matches any username. Whereas the quoted token "all" would only
match a user named 'all'.

Therefore, token_is_keyword only returns true where the token matches
the given string as is also unquoted.

Does that make sense?

Cheers,
BJ

2011/6/21 Brendan Jurd <direvus(at)gmail(dot)com>:
> On 21 June 2011 14:34, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
>> I don't understand to using a macro
>>
>> #define token_is_keyword(t, k)  (!t->quoted && strcmp(t->string, k) == 0)
>>
>> because you disallowed a quoting?
>
> Well, a token can only be treated as a special keyword if it is unquoted.
>
> As an example, in the 'database' field, the bare token 'replication'
> is a keyword meaning the pseudo-database for streaming rep.  Whereas
> the quoted token "replication" would mean a real database which is
> called 'replication'.
>
> Likewise, the bare token 'all' in the username field is a keyword --
> it matches any username.  Whereas the quoted token "all" would only
> match a user named 'all'.
>
> Therefore, token_is_keyword only returns true where the token matches
> the given string as is also unquoted.
>
> Does that make sense?

yes - it has a sense. Quoting changes sense from keyword to literal.
But then I see a significant inconsistency - every know keywords
should be only tokens.

else if (strcmp(token, "pamservice") == 0)
- {
- REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
- parsedline->pamservice = pstrdup(c);
- }

because >>pamservice<< - is known keyword, but 'pamservice' is some
literal without any mean. You should to use a makro token_is_keyword
more often.

??

Regards

Pavel

>
> Cheers,
> BJ
>

Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:

> yes - it has a sense. Quoting changes sense from keyword to literal.
> But then I see a significant inconsistency - every know keywords
> should be only tokens.
>
> else if (strcmp(token, "pamservice") == 0)
> - {
> - REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
> - parsedline->pamservice = pstrdup(c);
> - }
>
> because >>pamservice<< - is known keyword, but 'pamservice' is some
> literal without any mean. You should to use a makro token_is_keyword
> more often.

Yeah, I wondered about this too (same with auth types, i.e. do we accept
quoted "hostssl" and so on or should that by rejected?). I opted for
leaving it alone, but maybe this needs to be fixed. (Now that I think
about it, what we should do first is verify whether it works with quotes
in the unpatched code).

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2011/6/21 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
>
>> yes - it has a sense. Quoting changes sense from keyword to literal.
>> But then I see a significant inconsistency - every know keywords
>> should be only tokens.
>>
>>         else if (strcmp(token, "pamservice") == 0)
>> -             {
>> -                 REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
>> -                 parsedline->pamservice = pstrdup(c);
>> -             }
>>
>> because >>pamservice<< - is known keyword, but 'pamservice' is some
>> literal without any mean. You should to use a makro token_is_keyword
>> more often.
>
> Yeah, I wondered about this too (same with auth types, i.e. do we accept
> quoted "hostssl" and so on or should that by rejected?).  I opted for
> leaving it alone, but maybe this needs to be fixed.  (Now that I think
> about it, what we should do first is verify whether it works with quotes
> in the unpatched code).
>

It's question about compatibility - sure. But a description inside
pg_hba.conf speaks cleanly - quoting means a lost of original
semantic.

And if we allow a quoting somewhere, then I can't imagine a
description - "somewhere quoting means a string literal, somewhere
have not impact?"

Regards

Pavel Stehule

> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

Excerpts from Pavel Stehule's message of mar jun 21 10:04:26 -0400 2011:
> 2011/6/21 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> > Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
> >
> >> yes - it has a sense. Quoting changes sense from keyword to literal.
> >> But then I see a significant inconsistency - every know keywords
> >> should be only tokens.
> >>
> >>         else if (strcmp(token, "pamservice") == 0)
> >> -             {
> >> -                 REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
> >> -                 parsedline->pamservice = pstrdup(c);
> >> -             }
> >>
> >> because >>pamservice<< - is known keyword, but 'pamservice' is some
> >> literal without any mean. You should to use a makro token_is_keyword
> >> more often.
> >
> > Yeah, I wondered about this too (same with auth types, i.e. do we accept
> > quoted "hostssl" and so on or should that by rejected?).  I opted for
> > leaving it alone, but maybe this needs to be fixed.  (Now that I think
> > about it, what we should do first is verify whether it works with quotes
> > in the unpatched code).

I tested it and it works: This line

"local" @dbs +b "trust"

is accepted and it works in the unpatched code. I don't think we want
to break people's existing pg_hba.conf files for no reason. I doubt
that many people are using pg_hba.conf tokens with quotes, mind you, but
there might be some ...

In any case, if people here thinks we should tighten this, it's easy to
do on top of this patch by changing the strcmp() calls to
token_is_keyword, as you say. Let's not burden this patch with the
responsibility of doing so, because that's likely to get it punted.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2011/6/21 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Pavel Stehule's message of mar jun 21 10:04:26 -0400 2011:
>> 2011/6/21 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
>> > Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
>> >
>> >> yes - it has a sense. Quoting changes sense from keyword to literal.
>> >> But then I see a significant inconsistency - every know keywords
>> >> should be only tokens.
>> >>
>> >>         else if (strcmp(token, "pamservice") == 0)
>> >> -             {
>> >> -                 REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
>> >> -                 parsedline->pamservice = pstrdup(c);
>> >> -             }
>> >>
>> >> because >>pamservice<< - is known keyword, but 'pamservice' is some
>> >> literal without any mean. You should to use a makro token_is_keyword
>> >> more often.
>> >
>> > Yeah, I wondered about this too (same with auth types, i.e. do we accept
>> > quoted "hostssl" and so on or should that by rejected?).  I opted for
>> > leaving it alone, but maybe this needs to be fixed.  (Now that I think
>> > about it, what we should do first is verify whether it works with quotes
>> > in the unpatched code).
>
> I tested it and it works: This line
>
> "local" @dbs +b "trust"
>
> is accepted and it works in the unpatched code.  I don't think we want
> to break people's existing pg_hba.conf files for no reason.  I doubt
> that many people are using pg_hba.conf tokens with quotes, mind you, but
> there might be some ...
>
> In any case, if people here thinks we should tighten this, it's easy to
> do on top of this patch by changing the strcmp() calls to
> token_is_keyword, as you say.  Let's not burden this patch with the
> responsibility of doing so, because that's likely to get it punted.

It is time to discuss about it.

I thinking so current behave is strange and should be fixed - it
doesn't respect a description stored in pg_hba.conf. I agree, so this
will have impact on compatibility, but pg_hba is config file so this
impact is not too hard. The cleaning now can carry a benefit in
future, when pg_hba can be more complex.

Regards

Pavel

On Tue, Jun 21, 2011 at 10:15:50AM -0400, Alvaro Herrera wrote:
> Excerpts from Pavel Stehule's message of mar jun 21 10:04:26 -0400 2011:
> > 2011/6/21 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> > > Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
> > >
> > >> yes - it has a sense. Quoting changes sense from keyword to literal.
> > >> But then I see a significant inconsistency - every know keywords
> > >> should be only tokens.
> > >>
> > >>         else if (strcmp(token, "pamservice") == 0)
> > >> -             {
> > >> -                 REQUIRE_AUTH_OPTION(uaPAM, "pamservice", "pam");
> > >> -                 parsedline->pamservice = pstrdup(c);
> > >> -             }
> > >>
> > >> because >>pamservice<< - is known keyword, but 'pamservice' is some
> > >> literal without any mean. You should to use a makro token_is_keyword
> > >> more often.
> > >
> > > Yeah, I wondered about this too (same with auth types, i.e. do we accept
> > > quoted "hostssl" and so on or should that by rejected?).  I opted for
> > > leaving it alone, but maybe this needs to be fixed.  (Now that I think
> > > about it, what we should do first is verify whether it works with quotes
> > > in the unpatched code).
>
> I tested it and it works: This line
>
> "local" @dbs +b "trust"
>
> is accepted and it works in the unpatched code. I don't think we want
> to break people's existing pg_hba.conf files for no reason. I doubt
> that many people are using pg_hba.conf tokens with quotes, mind you, but
> there might be some ...
>
> In any case, if people here thinks we should tighten this, it's easy to
> do on top of this patch by changing the strcmp() calls to
> token_is_keyword, as you say. Let's not burden this patch with the
> responsibility of doing so, because that's likely to get it punted.

Hmm, would it be possible to add some deprecation warnings for this case
without making the code too messy? Perhaps with a macro
"token_should_be_keyword". That's the usual path to tightening syntax.

Ross
--
Ross Reedstrom, Ph.D. reedstrm(at)rice(dot)edu
Systems Engineer & Admin, Research Scientist phone: 713-348-6166
Connexions http://cnx.org fax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E F888 D3AE 810E 88F0 BEDE

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
>> because >>pamservice<< - is known keyword, but 'pamservice' is some
>> literal without any mean. You should to use a makro token_is_keyword
>> more often.

> Yeah, I wondered about this too (same with auth types, i.e. do we accept
> quoted "hostssl" and so on or should that by rejected?). I opted for
> leaving it alone, but maybe this needs to be fixed. (Now that I think
> about it, what we should do first is verify whether it works with quotes
> in the unpatched code).

AFAICS, this is only important in places where the syntax allows either
a keyword or an identifier. If only a keyword is possible, there is no
value in rejecting it because it's quoted. And, when you do the test,
I think you'll find that it would be breaking hba files that used to
work (though admittedly, it's doubtful that there are any such in the
field).

regards, tom lane

2011/6/21 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
>>> because >>pamservice<< - is known keyword, but 'pamservice' is some
>>> literal without any mean. You should to use a makro token_is_keyword
>>> more often.
>
>> Yeah, I wondered about this too (same with auth types, i.e. do we accept
>> quoted "hostssl" and so on or should that by rejected?).  I opted for
>> leaving it alone, but maybe this needs to be fixed.  (Now that I think
>> about it, what we should do first is verify whether it works with quotes
>> in the unpatched code).
>
> AFAICS, this is only important in places where the syntax allows either
> a keyword or an identifier.  If only a keyword is possible, there is no
> value in rejecting it because it's quoted.  And, when you do the test,
> I think you'll find that it would be breaking hba files that used to
> work (though admittedly, it's doubtful that there are any such in the
> field).

It should be better documented. I don't think so this is good
solution, but this is not too important.

regards

Pavel

>
>                        regards, tom lane
>

Excerpts from Pavel Stehule's message of mar jun 21 11:04:11 -0400 2011:
> 2011/6/21 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:

> > AFAICS, this is only important in places where the syntax allows either
> > a keyword or an identifier.  If only a keyword is possible, there is no
> > value in rejecting it because it's quoted.  And, when you do the test,
> > I think you'll find that it would be breaking hba files that used to
> > work (though admittedly, it's doubtful that there are any such in the
> > field).
>
> It should be better documented. I don't think so this is good
> solution, but this is not too important.

On the contrary -- we should support it but not document it. I mean,
what good would that do? If someone is so silly to uselessly quote
keywords, let them do it, but let's not encourage it.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2011/6/21 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Pavel Stehule's message of mar jun 21 11:04:11 -0400 2011:
>> 2011/6/21 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>
>> > AFAICS, this is only important in places where the syntax allows either
>> > a keyword or an identifier.  If only a keyword is possible, there is no
>> > value in rejecting it because it's quoted.  And, when you do the test,
>> > I think you'll find that it would be breaking hba files that used to
>> > work (though admittedly, it's doubtful that there are any such in the
>> > field).
>>
>> It should be better documented. I don't think so this is good
>> solution, but this is not too important.
>
> On the contrary -- we should support it but not document it.  I mean,
> what good would that do?  If someone is so silly to uselessly quote
> keywords, let them do it, but let's not encourage it.

it is argument too :)

It has not good solution - one break compatibility, second is strange
and undocumented :(

Actually I don't remember a issues about pg_hba.conf - probably 99%
users work with default configuration, so we can leave this file in
current state.

I am thinking so a notice in pg_hba.conf can be redesigned - almost
all people don't read it, but if someone read it, then he needs a
correct information - in sense, so on quotes works only where literal
or known literal can be entered.

Regards

Pavel Stehule

>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

On Jun 21, 2011, at 12:41 PM, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> On the contrary -- we should support it but not document it.

+1.

...Robert

On 22 June 2011 00:47, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>> Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
>>> because >>pamservice<< - is known keyword, but 'pamservice' is some
>>> literal without any mean. You should to use a makro token_is_keyword
>>> more often.
>
>> Yeah, I wondered about this too (same with auth types, i.e. do we accept
>> quoted "hostssl" and so on or should that by rejected?).  I opted for
>> leaving it alone, but maybe this needs to be fixed.  (Now that I think
>> about it, what we should do first is verify whether it works with quotes
>> in the unpatched code).
>
> AFAICS, this is only important in places where the syntax allows either
> a keyword or an identifier.  If only a keyword is possible, there is no
> value in rejecting it because it's quoted.  And, when you do the test,
> I think you'll find that it would be breaking hba files that used to
> work (though admittedly, it's doubtful that there are any such in the
> field).

Yes, I agree, and this was my thinking when I came up against this
while writing the original patch. It doesn't help to treat "hostssl"
differently than hostssl, because quoted identifiers are meaningless
in that context.

Cheers,
BJ

2011/6/22 Brendan Jurd <direvus(at)gmail(dot)com>:
> On 22 June 2011 00:47, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
>>> Excerpts from Pavel Stehule's message of mar jun 21 00:59:44 -0400 2011:
>>>> because >>pamservice<< - is known keyword, but 'pamservice' is some
>>>> literal without any mean. You should to use a makro token_is_keyword
>>>> more often.
>>
>>> Yeah, I wondered about this too (same with auth types, i.e. do we accept
>>> quoted "hostssl" and so on or should that by rejected?).  I opted for
>>> leaving it alone, but maybe this needs to be fixed.  (Now that I think
>>> about it, what we should do first is verify whether it works with quotes
>>> in the unpatched code).
>>
>> AFAICS, this is only important in places where the syntax allows either
>> a keyword or an identifier.  If only a keyword is possible, there is no
>> value in rejecting it because it's quoted.  And, when you do the test,
>> I think you'll find that it would be breaking hba files that used to
>> work (though admittedly, it's doubtful that there are any such in the
>> field).
>
> Yes, I agree, and this was my thinking when I came up against this
> while writing the original patch.  It doesn't help to treat "hostssl"
> differently than hostssl, because quoted identifiers are meaningless
> in that context.

ook, now is clean, so this is majority opinion.

Please, can you send a final patch.

Regards

Pavel

>
> Cheers,
> BJ
>

On 22 June 2011 14:01, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> ook, now is clean, so this is majority opinion.
>
> Please, can you send a final patch.

I don't have any further changes to add to Alvaro's version 3, which
is already up on the CF app.

Cheers,
BJ

Excerpts from Brendan Jurd's message of mié jun 22 00:12:38 -0400 2011:
> On 22 June 2011 14:01, Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> wrote:
> > ook, now is clean, so this is majority opinion.
> >
> > Please, can you send a final patch.
>
> I don't have any further changes to add to Alvaro's version 3, which
> is already up on the CF app.

I have touched next_token() and next_token_expand() a bit more, because
it seemed to me that they could be simplified further (the bit about
returning the comma in the token, instead of being a boolean return,
seemed strange). Please let me know what you think.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

On 24 June 2011 03:48, Alvaro Herrera <alvherre(at)commandprompt(dot)com> wrote:
> I have touched next_token() and next_token_expand() a bit more, because
> it seemed to me that they could be simplified further (the bit about
> returning the comma in the token, instead of being a boolean return,
> seemed strange).  Please let me know what you think.

Cool. I didn't like the way it returned the comma either, but I
thought it would be best if I kept the changes in my patch to a
minimum.

Cheers,
BJ

Hello

>
> I have touched next_token() and next_token_expand() a bit more, because
> it seemed to me that they could be simplified further (the bit about
> returning the comma in the token, instead of being a boolean return,
> seemed strange).  Please let me know what you think.
>

I am thinking, so it is ok.

I am not sure, if load_ident is correct. In load_hba you clean a
previous context after successful processing. In load_ident you clean
data on start. Is it ok?

Regards

Pavel

Excerpts from Pavel Stehule's message of dom jun 26 13:10:13 -0400 2011:
> Hello
>
> >
> > I have touched next_token() and next_token_expand() a bit more, because
> > it seemed to me that they could be simplified further (the bit about
> > returning the comma in the token, instead of being a boolean return,
> > seemed strange).  Please let me know what you think.
>
> I am thinking, so it is ok.

Thanks, I have pushed it. Brendan: thanks for the patch.

> I am not sure, if load_ident is correct. In load_hba you clean a
> previous context after successful processing. In load_ident you clean
> data on start. Is it ok?

Yeah, they are a bit inconsistent. I am uninclined to mess with it,
though. Right now it seems to me that the only way it could fail is if
you hit an out-of-memory or a similar problem. And if you hit that at
postmaster startup ... well ... I guess you have A Problem.

If somebody wants to submit a patch to fix that, be my guest.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2011/6/28 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Pavel Stehule's message of dom jun 26 13:10:13 -0400 2011:
>> Hello
>>
>> >
>> > I have touched next_token() and next_token_expand() a bit more, because
>> > it seemed to me that they could be simplified further (the bit about
>> > returning the comma in the token, instead of being a boolean return,
>> > seemed strange).  Please let me know what you think.
>>
>> I am thinking, so it is ok.
>
> Thanks, I have pushed it.  Brendan: thanks for the patch.
>
>> I am not sure, if load_ident is correct. In load_hba you clean a
>> previous context after successful processing. In load_ident you clean
>> data on start. Is it ok?
>
> Yeah, they are a bit inconsistent.  I am uninclined to mess with it,
> though.  Right now it seems to me that the only way it could fail is if
> you hit an out-of-memory or a similar problem.  And if you hit that at
> postmaster startup ... well ... I guess you have A Problem.

there should be a format (syntax) error. If somebody breaks a pg_ident
and will do a reload, then all ident mapping is lost.

This is not related to topic or doesn't modify current behave, so
should not be repaired now

Pavel

>
> If somebody wants to submit a patch to fix that, be my guest.
>
> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

Excerpts from Pavel Stehule's message of mar jun 28 16:19:02 -0400 2011:

> there should be a format (syntax) error. If somebody breaks a pg_ident
> and will do a reload, then all ident mapping is lost.

No, the file is not actually parsed until the auth verification runs.
The incorrect tokens are happily stored in memory by the tokeniser. In
my view that's the wrong way to do it, but changing it seems to be a
lot of work (I didn't try).

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2011/6/28 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Pavel Stehule's message of mar jun 28 16:19:02 -0400 2011:
>
>> there should be a format (syntax) error. If somebody breaks a pg_ident
>> and will do a reload, then all ident mapping is lost.
>
> No, the file is not actually parsed until the auth verification runs.
> The incorrect tokens are happily stored in memory by the tokeniser.  In
> my view that's the wrong way to do it, but changing it seems to be a
> lot of work (I didn't try).
>

ok

> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>