Hi,
On 10/18/22 7:51 AM, Michael Paquier wrote:
> On Mon, Oct 17, 2022 at 07:56:02PM +0200, Drouvot, Bertrand wrote:
>> On 10/14/22 7:30 AM, Michael Paquier wrote:
>>> This approach would not stick with
>>> pg_ident.conf though, as we validate the fields in each line when we
>>> put our hands on ident_user and after the base validation of a line
>>> (number of fields, etc.).
>>
>> I'm not sure to get the issue here with the proposed approach and
>> pg_ident.conf.
>
> My point is about parse_ident_line(), where we need to be careful in
> the order of the operations. The macros IDENT_MULTI_VALUE() and
> IDENT_FIELD_ABSENT() need to be applied on all the fields first, and
> the regex computation needs to be last. Your patch had a subtile
> issue here, as users may get errors on the computed regex before the
> ordering of the fields as the computation was used *before* the "Get
> the PG rolename token" part of the logic.
Gotcha, thanks! I was wondering if we shouldn't add a comment about that
and I see that you've added one in v2, thanks!
BTW, what about adding a new TAP test (dedicated patch) to test the
behavior in case of errors during the regexes compilation in
pg_ident.conf and pg_hba.conf (not done yet)? (we could add it once this
patch series is done).
> While putting my hands on that, I was also wondering whether we should
> have the error string generated after compilation within the internal
> regcomp() routine, but that would require more arguments to
> pg_regcomp() (as of file name, line number, **err_string), and that
> looks more invasive than necessary. Perhaps the follow-up steps will
> prove me wrong, though :)
I've had the same thought (and that was what the previous global patch
was doing). I'm tempted to think that the follow-steps will prove you
right ;-) (specially if at the end those will be the same error messages
for databases and roles).
>
> A last thing is the introduction of a free() routine for AuthTokens,
> to minimize the number of places where we haev pg_regfree(). The gain
> is minimal, but that looks more consistent with the execution and
> compilation paths.
Agree, that looks better.
I had a look at your v2, did a few tests and it looks good to me.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
