Allow file inclusion in pg_hba and pg_ident files
Hi,
I recently had to work on some internal tool which, among other things, has to
merge pg_hba.conf and pg_ident.conf files between an existing (possibly
already updated) instance and some external repository.
My biggest concern is that any issue in either the external repository content
or the tool could leave the instance entirely unreachable. There are some
protection attemps to avoid that, but really there isn't any practical
possibility if one wants to make sure that some of the entries are left alone
or not hidden no matter what.
To address that, I'd like to propose the possibility to include files in hba
and ident configuration files. This was already discussed in the past, and in
my understanding this is mostly wanted, while some people expressed concerned
on a use case that wouldn't rely on thousands of entries.
In my case, there shouldn't be more than a dozen generated pg_hba/pg_ident
lines. All I want is to have my main hba (and ident) files do something like:
include hba_forbid_non_ssl.conf
include hba_superuser.conf
include hba_replication.conf
include hba_application_generated.conf
So that the tool has a way to make sure that it won't prevent dba login or
replication, or allow unsecure connections, and can simply rewrite its target
file rather than merging it, sorting it with weird rules and deciding which
lines are ok to be removed and which one aren't.
I'm attaching a patchset that implements $SUBJECT:
0001 adds a new pg_ident_file_mappings view, which is basically the same as
pg_hba_file_rules view but for mappings. It's probably already useful, for
instance if you need to tweak some regexp.
0002 adds a new "include" directive to both auth files, which will include the
given file at this exact position. I chose to do this is in the tokenization
rather than the parsing, as it makes things simpler in general, and also allows
a single code path for both files. It also adds 2 new columns to the
pg_hba_file_rules and pg_ident_file_mappings views: file_name and
(rule|mapping)_number, so it's possible to identify where a rule is coming from
and more importantly which has the priority over the over. Note that I only
added "include", but maybe people would want something like include_dir or
include_if_exists too.
Both patches have updated documentation, but no test yet. If there's an
agreement on the feature, I plan to add some TAP test for it.
Finally I also added 0003, which is a POC for a new pg_hba_matches() function,
that can help DBA to understand why their configuration isn't working as they
expect. This only to start the discussion on that topic, the code is for now
really hackish, as I don't know how much this is wanted and/or if some other
behavior would be better, and there's also no documentation or test. The
function for now only takes an optional inet (null means unix socket), the
target role and an optional ssl flag and returns the file, line and raw line
matching if any, or null. For instance:
=# select * from pg_hba_matches('127.0.0.1'::inet, 'postgres');
-[ RECORD 1 ]-----------------------------------------------------------------
file_name | /tmp/pgbuild/toto.conf
line_num | 5
raw_line | host all all 127.0.0.1/32 trust
I'm wondering for instance if it would be better to (optionally?) keep
iterating over the lines and print all lines that would have matched and in
which order, or if the function should return the parsed line information
rather than the raw line, although it could make the output worse if using huge
secondary auth files. I'm also not sure if the various possible flags (ssl,
gss) should be explicitly set or if the function should try various
permutations on its own.
Note that this function only works against the current configuration files, not
the one currently active. As far as I can see PostgresMain gets rid of
PostmasterContext, which is where they currently live, so they don't exist
anymore in the backends. To make it work we would have to always keep those in
each backend, but also reload them along with the main config file on SIGHUP.
Except on Windows (and -DEXEC_BACKEND), as the current version of auth files
are always used anyway. So would it make sense to add the possibility to use
the loaded files instead, given the required extra cost and the fact that it
wouldn't make sense on Windows? If yes, I guess that we should also allow that
in the pg_hba / pg_ident views.
While at it, I noticed this comment added in de16ab72388:
The <filename>pg_hba.conf</filename> file is read on start-up and when
the main server process receives a
<systemitem>SIGHUP</systemitem><indexterm><primary>SIGHUP</primary></indexterm>
signal. If you edit the file on an
active system, you will need to signal the postmaster
(using <literal>pg_ctl reload</literal>, calling the SQL function
<function>pg_reload_conf()</function>, or using <literal>kill
-HUP</literal>) to make it re-read the file.
[...]
The preceding statement is not true on Microsoft Windows: there, any
changes in the <filename>pg_hba.conf</filename> file are immediately
applied by subsequent new connections.
I also find this comment a bit misleading. Wouldn't it be better to have a
similar statement for pg_ident, or at least a note that it applies to both
files?
On Wed, Feb 23, 2022 at 12:59:59PM +0800, Julien Rouhaud wrote:
To address that, I'd like to propose the possibility to include files in hba
and ident configuration files. This was already discussed in the past, and in
my understanding this is mostly wanted, while some people expressed concerned
on a use case that wouldn't rely on thousands of entries.
+1, I think this would be very useful.
0001 adds a new pg_ident_file_mappings view, which is basically the same as
pg_hba_file_rules view but for mappings. It's probably already useful, for
instance if you need to tweak some regexp.
This seems reasonable.
Finally I also added 0003, which is a POC for a new pg_hba_matches() function,
that can help DBA to understand why their configuration isn't working as they
expect. This only to start the discussion on that topic, the code is for now
really hackish, as I don't know how much this is wanted and/or if some other
behavior would be better, and there's also no documentation or test. The
function for now only takes an optional inet (null means unix socket), the
target role and an optional ssl flag and returns the file, line and raw line
matching if any, or null. For instance:
I think another use-case for this is testing updates to your configuration
files. For example, I could ensure that hba_forbid_non_ssl.conf wasn't
accidentally reverted as part of an unrelated change.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Wed, Feb 23, 2022 at 09:44:58AM -0800, Nathan Bossart wrote:
On Wed, Feb 23, 2022 at 12:59:59PM +0800, Julien Rouhaud wrote:
0001 adds a new pg_ident_file_mappings view, which is basically the same as
pg_hba_file_rules view but for mappings. It's probably already useful, for
instance if you need to tweak some regexp.This seems reasonable.
Interesting. One can note that hba.c is already large, and this makes
the file larger. I'd like to think that it would be better to move
all the code related to the SQL functions for pg_hba.conf and such to
a new hbafuncs.c under adt/. Would that make sense?
--
Michael
Hi,
On Sat, Feb 26, 2022 at 03:04:43PM +0900, Michael Paquier wrote:
On Wed, Feb 23, 2022 at 09:44:58AM -0800, Nathan Bossart wrote:
On Wed, Feb 23, 2022 at 12:59:59PM +0800, Julien Rouhaud wrote:
0001 adds a new pg_ident_file_mappings view, which is basically the same as
pg_hba_file_rules view but for mappings. It's probably already useful, for
instance if you need to tweak some regexp.This seems reasonable.
Interesting. One can note that hba.c is already large, and this makes
the file larger. I'd like to think that it would be better to move
all the code related to the SQL functions for pg_hba.conf and such to
a new hbafuncs.c under adt/. Would that make sense?
I'm fine with it. Assuming that you meant to move also the underlying
functions that goes with it (fill_hba_line and such), that would end up
removing about 15% of hba.c (after applying 0001, 0002 and 0003).
Note that in order to do so we would need to expose quite a lot more about hba
internals, like tokenize_file() and parse_hba_line(), along with structs
HbaToken and TokenizedLine.
On Sat, Feb 26, 2022 at 02:27:15PM +0800, Julien Rouhaud wrote:
I'm fine with it. Assuming that you meant to move also the underlying
functions that goes with it (fill_hba_line and such), that would end up
removing about 15% of hba.c (after applying 0001, 0002 and 0003).
Cool. Thanks for the number.
Note that in order to do so we would need to expose quite a lot more about hba
internals, like tokenize_file() and parse_hba_line(), along with structs
HbaToken and TokenizedLine.
I'd be rather fine with exposing all that in the shape of a clean
split, renaming some of those structures and/or function with an
Hba-like prefix, for consistency.
--
Michael
On Sat, Feb 26, 2022 at 03:36:19PM +0900, Michael Paquier wrote:
On Sat, Feb 26, 2022 at 02:27:15PM +0800, Julien Rouhaud wrote:
Note that in order to do so we would need to expose quite a lot more about hba
internals, like tokenize_file() and parse_hba_line(), along with structs
HbaToken and TokenizedLine.I'd be rather fine with exposing all that in the shape of a clean
split, renaming some of those structures and/or function with an
Hba-like prefix, for consistency.
Of course. I was thinking using "auth" for something that's common to pg_hba
and pg_ident (like e.g. TokenizeAuthFile()), and otherwise keep the current
hba/ident prefix.
Unless someone object or suggest better naming in the next few days I will take
care of that.
I would also welcome some opinion on the points I mentioned about 0002.
On Sat, Feb 26, 2022 at 02:50:33PM +0800, Julien Rouhaud wrote:
Of course. I was thinking using "auth" for something that's common to pg_hba
and pg_ident (like e.g. TokenizeAuthFile()), and otherwise keep the current
hba/ident prefix.
Okay, thanks.
Unless someone object or suggest better naming in the next few days I will take
care of that.
I don't have an opinion to share about 0002 and 0003 yet, but 0001
seems like a good idea on its own.
--
Michael
Hi,
On Wed, Feb 23, 2022 at 09:44:58AM -0800, Nathan Bossart wrote:
Finally I also added 0003, which is a POC for a new pg_hba_matches() function,
that can help DBA to understand why their configuration isn't working as they
expect. This only to start the discussion on that topic, the code is for now
really hackish, as I don't know how much this is wanted and/or if some other
behavior would be better, and there's also no documentation or test. The
function for now only takes an optional inet (null means unix socket), the
target role and an optional ssl flag and returns the file, line and raw line
matching if any, or null. For instance:I think another use-case for this is testing updates to your configuration
files. For example, I could ensure that hba_forbid_non_ssl.conf wasn't
accidentally reverted as part of an unrelated change.
Indeed, that function could really be helpful in many scenario. Note that this
isn't my idea but Magnus idea, which he mentioned quite a long time ago.
Hi,
On Mon, Feb 28, 2022 at 05:01:07PM +0900, Michael Paquier wrote:
On Sat, Feb 26, 2022 at 02:50:33PM +0800, Julien Rouhaud wrote:
Of course. I was thinking using "auth" for something that's common to pg_hba
and pg_ident (like e.g. TokenizeAuthFile()), and otherwise keep the current
hba/ident prefix.Okay, thanks.
Done in attached v2. I did the split in a separate commit, as the diff is
otherwise unreadable. While at it I also fixed a few minor issues (I missed a
MemoryContextDelete, and now avoid relying on inet_net_pton which apparently
doesn't exist in cygwin).
Attachments:
v2-0001-Extract-view-processing-code-from-hba.c.patchtext/plain; charset=us-asciiDownload+2067-2036
v2-0002-Add-a-pg_ident_file_mappings-view.patchtext/plain; charset=us-asciiDownload+412-97
v2-0003-Allow-file-inclusion-in-pg_hba-and-pg_ident-files.patchtext/plain; charset=us-asciiDownload+395-156
v2-0004-POC-Add-a-pg_hba_matches-function.patchtext/plain; charset=us-asciiDownload+154-1
On Mon, Feb 28, 2022 at 07:42:17PM +0800, Julien Rouhaud wrote:
Done in attached v2. I did the split in a separate commit, as the diff is
otherwise unreadable. While at it I also fixed a few minor issues (I missed a
MemoryContextDelete, and now avoid relying on inet_net_pton which apparently
doesn't exist in cygwin).
Hmm. The diffs of 0001 are really hard to read. Do you know why this
is happening? Is that because some code has been moved around? I
have been doing a comparison of all the routines showing up in the
diffs, to note that the contents of load_hba(), load_ident(),
hba_getauthmethod() & friends are actually the same, but this makes
the change history harder to follow. Moving around fill_hba_line()
and fill_hba_view() should be enough, indeed.
+#include "utils/guc.h"
+//#include "utils/tuplestore.h"
Ditto.
+ /* Build a tuple descriptor for our result type */
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");Worth noting that I was planning to apply a patch from Melanie
Plageman to simplify the creation of tupledesc and tuplestores for
set-returning functions like this one, so this would cut a bit of code
here. This is not directly related to your patch, though, that's my
business :)
Well, as of 0002, one thing that makes things harder to follow is that
parse_ident_line() is moved at a different place in hba.c, one slight
difference being err_msg to store the error message in the token
line.. But shouldn't the extension of parse_ident_line() with its
elevel be included in 0001? Or, well, it could just be done in its
own patch to make for a cleaner history, so as 0002 could be shaped as
two commits itself.
Also, it seems to me that we'd better have some TAP tests for that to
make sure of its contents? One place would be src/test/auth/.
Another place where we make use of user mapping is the krb5 tests but
these are run in a limited fashion in the buildfarm. We also set some
mappings for SSPI on Windows all the time, so we'd better be careful
about that and paint some $windows_os in the tests when looking at the
output of the view.
+-- We expect no user mapping in this test
+select count(*) = 0 as ok from pg_ident_file_mappings;It could be possible to do installcheck on an instance that has user
mappings meaning that this had better be ">= 0", no? Does this pass
on Windows where pg_regress sets some mappings for SSPI when creating
one or more roles?
--
Michael
Hi,
On Tue, Mar 01, 2022 at 04:45:48PM +0900, Michael Paquier wrote:
Hmm. The diffs of 0001 are really hard to read. Do you know why this
is happening? Is that because some code has been moved around?
Yes, I followed the file convention to put the static functions first and then
the exposed functions, and git-diff makes a terrible mess out of it :(
I
have been doing a comparison of all the routines showing up in the
diffs, to note that the contents of load_hba(), load_ident(),
hba_getauthmethod() & friends are actually the same, but this makes
the change history harder to follow. Moving around fill_hba_line()
and fill_hba_view() should be enough, indeed.
There's no functional change apart from exposing some functions and moving some
in another file, so I though it's still ok to keep some consistency. There
isn't much changes backpatched in that file, so it shouldn't create more
maintenance burden than simply splitting the file.
If you prefer to interleave static and non static function I can change it.
+#include "utils/guc.h"
+//#include "utils/tuplestore.h"
Yes I noticed this one this morning. I didn't want to send a new patch version
just for that, but I already fixed it locally.
+ /* Build a tuple descriptor for our result type */ + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) + elog(ERROR, "return type must be a row type");Worth noting that I was planning to apply a patch from Melanie
Plageman to simplify the creation of tupledesc and tuplestores for
set-returning functions like this one, so this would cut a bit of code
here. This is not directly related to your patch, though, that's my
business :)
Yes I'm aware of that thread. I will be happy to change the patch to use
MakeFuncResultTuplestore() as soon as it lands. Thanks for the notice though.
Well, as of 0002, one thing that makes things harder to follow is that
parse_ident_line() is moved at a different place in hba.c, one slight
difference being err_msg to store the error message in the token
line.. But shouldn't the extension of parse_ident_line() with its
elevel be included in 0001?
No, because 0001 is unrelated. The changes you mentioned (exposing the
function and adding the error reporting) are only needed for the new view
introduced in 0002, thus not part of 0001.
Or, well, it could just be done in its own patch to make for a cleaner
history, so as 0002 could be shaped as two commits itself.
It seems strange to me to add a commit (or include in a commit) just to make a
single function exposed while nothing needs it, but I can do it this way if you
prefer.
Also, it seems to me that we'd better have some TAP tests for that to
make sure of its contents?
As I mentioned in my initial email, I intentionally didn't add any test in the
patchset yet, except the exact same coverage for the new view as there's for
pg_hba_file_rules. Ideally I'd like to add tests only once, to cover both 002
and 0003. But I don't want to waste time for that right now, especially since
no one seems to be interested in 0003.
+-- We expect no user mapping in this test +select count(*) = 0 as ok from pg_ident_file_mappings;It could be possible to do installcheck on an instance that has user
mappings meaning that this had better be ">= 0", no?
I thought about it, and supposed it would bring a bit more value with the test
like that. I can change it if you prefer.
Does this pass
on Windows where pg_regress sets some mappings for SSPI when creating
one or more roles?
According to CI and cfbot yes. E.g.
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/37/3558.
Note that the failed runs are the warning I mentioned for mingw32 and the POC
0004, which is now fixed.
On Tue, Mar 01, 2022 at 05:19:50PM +0800, Julien Rouhaud wrote:
On Tue, Mar 01, 2022 at 04:45:48PM +0900, Michael Paquier wrote:
Hmm. The diffs of 0001 are really hard to read. Do you know why this
is happening? Is that because some code has been moved around?Yes, I followed the file convention to put the static functions first and then
the exposed functions, and git-diff makes a terrible mess out of it :(
I'd like to think that not doing such a thing would be more helpful in
this case. As the diffs show, anyone is going to have a hard time to
figure out if there are any differences in any of those routines, and
if these are the origin of a different problem. A second thing is
that this is going to make back-patching unnecessarily harder.
There's no functional change apart from exposing some functions and moving some
in another file, so I though it's still ok to keep some consistency. There
isn't much changes backpatched in that file, so it shouldn't create more
maintenance burden than simply splitting the file.
A lot of files do that already. History clarity matters most IMO.
As I mentioned in my initial email, I intentionally didn't add any test in the
patchset yet, except the exact same coverage for the new view as there's for
pg_hba_file_rules. Ideally I'd like to add tests only once, to cover both 002
and 0003. But I don't want to waste time for that right now, especially since
no one seems to be interested in 0003.
But that would be helpful for 0002. I think that we should have a bit
more coverage in this area. pg_hba_file_rules could gain in coverage,
additionally, but this is unrelated to what you are proposing here..
Does this pass
on Windows where pg_regress sets some mappings for SSPI when creating
one or more roles?According to CI and cfbot yes. E.g.
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/37/3558.
Note that the failed runs are the warning I mentioned for mingw32 and the POC
0004, which is now fixed.
Interesting, I would not have expected that. I may poke at that more
seriously.
--
Michael
Hi,
The cfbot says that the patch doesn't apply anymore, so here's a v3 with the
changes mentioned below.
On Tue, Mar 01, 2022 at 05:19:50PM +0800, Julien Rouhaud wrote:
If you prefer to interleave static and non static function I can change it.
Change the split to not reorder functions.
+#include "utils/guc.h"
+//#include "utils/tuplestore.h"Yes I noticed this one this morning. I didn't want to send a new patch version
just for that, but I already fixed it locally.
Included.
Yes I'm aware of that thread. I will be happy to change the patch to use
MakeFuncResultTuplestore() as soon as it lands. Thanks for the notice though.
Done, with the new SetSingleFuncCall().
It could be possible to do installcheck on an instance that has user
mappings meaning that this had better be ">= 0", no?I thought about it, and supposed it would bring a bit more value with the test
like that. I can change it if you prefer.
Change this way.
Attachments:
v3-0003-Allow-file-inclusion-in-pg_hba-and-pg_ident-files.patchtext/plain; charset=us-asciiDownload+314-80
v3-0004-POC-Add-a-pg_hba_matches-function.patchtext/plain; charset=us-asciiDownload+154-1
v3-0001-Extract-view-processing-code-from-hba.c.patchtext/plain; charset=us-asciiDownload+475-443
v3-0002-Add-a-pg_ident_file_mappings-view.patchtext/plain; charset=us-asciiDownload+302-17
Hi hackers,
The cfbot says that the patch doesn't apply anymore, so here's a v3 with
the
changes mentioned below.
I came across this thread while looking for the patches that need review.
The v3-0001 patch LGTM.
Since v3-0002 adds a new view and alters pg_proc.dat shouldn't it also
increase CATALOG_VERSION_NO? Not sure if we generally do this in the
patches or expect the committer to make the change manually.
Same question regarding v3-0003. Other than that the patch looks OK, but
doesn't seem to add any tests for the new functionality. Do you think it
would be possible to test-cover the file inclusion? Personally I don't
think it's that critical to have these particular tests, but if they can be
added, I think we should do so.
I didn't review v3-0004 since it's marked as PoC and seems to be a separate
feature that is not targeting PG15. I suggest excluding it from the
patchset in order to keep the focus. Consider creating a new thread and a
new CF entry after we deal with v3-0001...v3-0003.
All in all, the patchset seems to be in good shape and I don't have
anything but some little nitpicks. It passes `make installcheck` and I
verified manually that the file inclusion 1) works 2) write proper error
messages to the logfile when the included file doesn't exist or has wrong
permissions.
--
Best regards,
Aleksander Alekseev
Hi hackers,
It passes `make installcheck` ...
`make installcheck-world`, of course. Sorry for the confusion.
--
Best regards,
Aleksander Alekseev
Hi,
On Tue, Mar 22, 2022 at 03:21:20PM +0300, Aleksander Alekseev wrote:
The v3-0001 patch LGTM.
Since v3-0002 adds a new view and alters pg_proc.dat shouldn't it also
increase CATALOG_VERSION_NO? Not sure if we generally do this in the
patches or expect the committer to make the change manually.
It's better to no include any catversion bump, otherwise the patches will
rot very fast. Author can mention the need for a catversion bump in the
commit message, and I usually do so but I apparently forgot.
Same question regarding v3-0003. Other than that the patch looks OK, but
doesn't seem to add any tests for the new functionality. Do you think it
would be possible to test-cover the file inclusion? Personally I don't
think it's that critical to have these particular tests, but if they can be
added, I think we should do so.
Yes, as I mentioned in the first email I'm willing to add test coverage. But
this will require TAP tests, and it's likely going to be a bit annoying to make
sure it has decent coverage and works on all platforms, so I'd rather do it
once I know there's at least some basic agreement on the feature and/or the
approach. Unfortunately, until now there was no feedback on that part despite
the activity on the thread. In my experience it's a good sign that this will
get rejected soon, so I still didn't write tests.
I didn't review v3-0004 since it's marked as PoC and seems to be a separate
feature that is not targeting PG15. I suggest excluding it from the
patchset in order to keep the focus. Consider creating a new thread and a
new CF entry after we deal with v3-0001...v3-0003.
This feature was discussed in some old thread when the file inclusion was first
discussed almost a decade ago, and in my understanding it was part of the "must
have" (along with 0002) in order to accept the feature. That's why I added
it, since I'm also willing to work of that if needed, whether before or after
the file inclusion thing.
All in all, the patchset seems to be in good shape and I don't have
anything but some little nitpicks. It passes `make installcheck` and I
verified manually that the file inclusion 1) works 2) write proper error
messages to the logfile when the included file doesn't exist or has wrong
permissions.
Thanks!
On Tue, Mar 22, 2022 at 09:38:00PM +0800, Julien Rouhaud wrote:
On Tue, Mar 22, 2022 at 03:21:20PM +0300, Aleksander Alekseev wrote:
Since v3-0002 adds a new view and alters pg_proc.dat shouldn't it also
increase CATALOG_VERSION_NO? Not sure if we generally do this in the
patches or expect the committer to make the change manually.It's better to no include any catversion bump, otherwise the patches will
rot very fast. Author can mention the need for a catversion bump in the
commit message, and I usually do so but I apparently forgot.
Yeah, committers take care of that. You would just expose yourself to
more noise in the CF bot for no gain, as a catversion bump is useful
after a patch has been merged so as as users are able to know when a
cluster needs to be pg_upgrade'd or initdb'd because the catalog
created and run are incompatible.
All in all, the patchset seems to be in good shape and I don't have
anything but some little nitpicks. It passes `make installcheck` and I
verified manually that the file inclusion 1) works 2) write proper error
messages to the logfile when the included file doesn't exist or has wrong
permissions.Thanks!
Pushing forward with 0001 by the end of the CF is the part that has no
controversy IMO, and I have no objections to it. Now, after looking
at this part, I found a few things, as of:
- HbaToken, the set of elements in the lists of TokenizedAuthLine, is
a weird to use as this layer gets used by both pg_hba.conf and
pg_indent.conf before transforming them into each HbaLine and
IdentLine. While making this part of the internals exposed, I think
that we'd better rename that to AuthToken at least. This impacts the
names of some routines internal to hba.c to copy and create
AuthTokens.
- s/gethba_options/get_hba_options/, to be consistent with
fill_hba_view() and other things.
- The comment at the top of tokenize_auth_file() needed a refresh.
That's mostly cosmetic, and the rest of the code moved is identical.
So at the end this part looks rather commitable to me.
I have not been able to test 0002 in details, but it looks rather
rather sane to me at quick glance, and it is simple. The argument
about more TAP tests applies to it, though, even if there is one SQL
test to check the function execution. It is probably better to not
consider 0003 and 0004 for this CF.
--
Michael
Attachments:
v4-0001-Extract-view-processing-code-from-hba.c.patchtext/x-diff; charset=us-asciiDownload+511-471
Hi,
On Wed, Mar 23, 2022 at 11:03:46AM +0900, Michael Paquier wrote:
Pushing forward with 0001 by the end of the CF is the part that has no
controversy IMO, and I have no objections to it. Now, after looking
at this part, I found a few things, as of:
- HbaToken, the set of elements in the lists of TokenizedAuthLine, is
a weird to use as this layer gets used by both pg_hba.conf and
pg_indent.conf before transforming them into each HbaLine and
IdentLine. While making this part of the internals exposed, I think
that we'd better rename that to AuthToken at least. This impacts the
names of some routines internal to hba.c to copy and create
AuthTokens.
Yeah, I thought about it but didn't rename it given your concerns about git
history. I'm fine either way.
- s/gethba_options/get_hba_options/, to be consistent with
fill_hba_view() and other things.
- The comment at the top of tokenize_auth_file() needed a refresh.That's mostly cosmetic, and the rest of the code moved is identical.
So at the end this part looks rather commitable to me.
Looks good to me, thanks.
I have not been able to test 0002 in details, but it looks rather
rather sane to me at quick glance, and it is simple. The argument
about more TAP tests applies to it, though, even if there is one SQL
test to check the function execution. It is probably better to not
consider 0003 and 0004 for this CF.
No objection to moving 0003 and 0004 to the next commitfest.
On Wed, Mar 23, 2022 at 10:16:34AM +0800, Julien Rouhaud wrote:
Yeah, I thought about it but didn't rename it given your concerns about git
history. I'm fine either way.
Oh, OK. The amount of diffs created by 0001 is still fine to grab
even with the struct rename, so let's stick with it.
--
Michael
On Wed, Mar 23, 2022 at 01:14:02PM +0900, Michael Paquier wrote:
On Wed, Mar 23, 2022 at 10:16:34AM +0800, Julien Rouhaud wrote:
Yeah, I thought about it but didn't rename it given your concerns about git
history. I'm fine either way.Oh, OK. The amount of diffs created by 0001 is still fine to grab
even with the struct rename, so let's stick with it.
And so, this first part has been applied as of d4781d8. I'll try to
look at 0002 shortly.
--
Michael
