Re: Proposal: allow database-specific role memberships

Thanks for the feedback.

I have attached an alternate version of the v5 patch that incorporates the
suggested changes to the documentation and DRYs up some of the acl.c code
for comparison. As for the databaseOid / InvalidOid parameter, I'm open to
any suggestions for how to make that even cleaner, but am currently at a
loss as to how that would look.

CI is showing a failure to run pg_dump on just the Linux - Debian Bullseye
job (https://cirrus-ci.com/task/5265343722553344). Does anyone have any
ideas as to where I should look in order to debug that?

Kenaniah

On Fri, Jan 21, 2022 at 3:04 PM David G. Johnston <
david(dot)g(dot)johnston(at)gmail(dot)com> wrote:

> On Fri, Jan 21, 2022 at 3:12 PM Kenaniah Cerny <kenaniah(at)gmail(dot)com> wrote:
>
>> The latest rebased version of the patch is attached.
>>
>
> As I was just reminded, we tend to avoid specifying specific PostgreSQL
> versions in our documentation. We just say what the current version does.
> Here, the note sentences at lines 62 and 63 don't follow documentation
> norms on that score and should just be removed. The last two sentences
> belong in the main description body, not a note. Thus the whole note goes
> away.
>
> I don't think I really appreciated the value this feature would have when
> combined with the predefined roles like pg_read_all_data and
> pg_write_all_data.
>
> I suppose I don't really appreciate the warning about SUPERUSER, etc...or
> at least why this warning is somehow specific to the per-database version
> of role membership. If this warning is desirable it should be worded to
> apply to role membership in general - and possibly proposed as a separate
> patch for consideration.
>
> I didn't dive deeply but I think we now have at three places in the acl.c
> code where after setting memlist from the system cache we perform nearly
> identical for loops to generate the final roles_list. Possibly this needs
> a refactor first so that you can introduce the per-database stuff more
> succinctly. Basically, the vast majority of this commit is just adding
> InvalidOid and databaseOid all other the place - with a few minor code
> changes to accommodate the new arguments. The acl.c code should try and be
> made done the same after post-refactor.
>
> David J.
>
>