Thread: Re: [COMMITTERS] pgsql: In pg_dump, include pg_catalog and extension ACLs, if changed

Stephen Frost <sfrost@snowman.net> writes:
> In pg_dump, include pg_catalog and extension ACLs, if changed

This patch added a regression test step that creates some roles
and doesn't drop them again.  This is unacceptable, because

(1) it breaks the ability to do "make installcheck" more than once.

(2) it leaves roles lying around in a production installation,
if installcheck is used there.

And it doesn't even adhere to the convention we've agreed to about
naming test roles regress_something.  OK, it's not as dangerously
broken that way as rowsecurity.sql, which is (still) creating roles
named "alice" and "bob", but it's not acceptable like this.

It'd be possible to fix (1) by adding "drop if exists", but I think the
whole thing is wrongheaded due to (2).  Perhaps the needs of the test
could be met by granting/revoking some rights explicitly to current_user
(ie, the test superuser)?  That wouldn't have any real effects on a
superuser, but it would provide some grist for testing the behavior.
Or you could test this behavior in some other setting than the core
regression tests; perhaps in a TAP test for pg_dump.
        regards, tom lane

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> It'd be possible to fix (1) by adding "drop if exists", but I think the
> whole thing is wrongheaded due to (2).  Perhaps the needs of the test
> could be met by granting/revoking some rights explicitly to current_user
> (ie, the test superuser)?  That wouldn't have any real effects on a
> superuser, but it would provide some grist for testing the behavior.

Sure, I can do that.

> Or you could test this behavior in some other setting than the core
> regression tests; perhaps in a TAP test for pg_dump.

I have to admit that I'm not terrible familiar with setting that up, but
will take a look at it.  This is what I'd really like to have as we
have far too little testing of pg_dump in the test suite.

Thanks!

Stephen

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> broken that way as rowsecurity.sql, which is (still) creating roles
> named "alice" and "bob", but it's not acceptable like this.

Attached is a patch to fix all of the role usage in rowsecurity.sql
(I believe, feel free to let me know if there's anything else).  In
particular, all of the roles are changed to begin with 'regress_', and
they are all now created with NOLOGIN.

I'll plan to push this in the next day or so.

I'll also work up a patch for the rest of the CREATE USER/ROLE usage in
the core regression tests.

This will result in a bit of not-strictly-necessary code churn, but
having consistency in the code base really is valuable where we have an
agreed upon policy as to what all the tests should be doing, for new
(and even old) developers to follow.

Comments and concerns welcome, of course.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> broken that way as rowsecurity.sql, which is (still) creating roles
>> named "alice" and "bob", but it's not acceptable like this.

> Attached is a patch to fix all of the role usage in rowsecurity.sql
> (I believe, feel free to let me know if there's anything else).  In
> particular, all of the roles are changed to begin with 'regress_', and
> they are all now created with NOLOGIN.

+1 for the general idea, although there's something to be said for
using names like 'regress_alice' and 'regress_bob' in this context.
'regress_user1' and 'regress_user2' are awfully gray and same-y,
and therefore a bit error-prone IMO.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> broken that way as rowsecurity.sql, which is (still) creating roles
> >> named "alice" and "bob", but it's not acceptable like this.
>
> > Attached is a patch to fix all of the role usage in rowsecurity.sql
> > (I believe, feel free to let me know if there's anything else).  In
> > particular, all of the roles are changed to begin with 'regress_', and
> > they are all now created with NOLOGIN.
>
> +1 for the general idea, although there's something to be said for
> using names like 'regress_alice' and 'regress_bob' in this context.
> 'regress_user1' and 'regress_user2' are awfully gray and same-y,
> and therefore a bit error-prone IMO.

Fair enough.

I'll adjust the ending '_userX' to be actual names along the lines of
'alice', 'bob', 'carol', 'dave', 'eve', 'frank', etc.

(List pulled from https://en.wikipedia.org/wiki/Alice_and_Bob).

Thanks!

Stephen