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
