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
