superuser() shortcuts
All,
We, as a community, have gotten flak from time-to-time about the
superuser role. We also tend to wish to avoid unnecessary
optimization as it complicates the code base and makes folks reviewing
the code wonder at the exceptions.
As such, I wonder at a few of superuser() checks we have today
which appear to be entirely superfluous, specifically:
replication/logical/logicalfuncs.c
check_permissions()
My 2c about this function is that it should be completely removed
and the place where it's checked replaced with just the
'has_rolreplication' call and error. It's only called in one
place and it'd be a simple one-liner anyway. As for
has_rolreplication, I don't understand why it's in miscinit.c when
the rest of the has_* set is in acl.c.
replication/slotfuncs.c - more or less the same
commands/alter.c
AlterObjectOwner_internal()
There's a shortcut here for superuser() that appears entirely
redundant as the immediately following 'has_privs_of_role()' will
return true for all superuser, as will the later
check_is_member_of_role() call, and the pg_namespace_aclcheck will
also return true. Perhaps I'm missing something, but why isn't
this superuser() check completely redundant and possible not ideal
(what if Anum_name is valid but NULL after all..?).
commands/tablecmds.c
ATExecChangeOwner()
The superuser check here looks to just be avoiding extra
permission checks, but that could change and we might eventually
end up in a situation similar to above where other checks are
happening (possibly to avoid a crash) but don't end up happenning
for superuser by mistake. I don't feel like table owner changes
happen so often that we need to avoid a couple extra function
calls and so I would recommend ripping out the explicit
superuser() check here.
commands/typecmds.c
AlterTypeOwner()
More-or-less the same as above.
commands/foreigncmds.c
AlterForeignServerOwner_internal()
Ditto.
Removing these design patterns may also help to avoid ending up with
more of them in the future as folks copy and/or crib off of what we've
already done to implement their features...
Thanks!
Stephen
Stephen,
We, as a community, have gotten flak from time-to-time about the
superuser role. We also tend to wish to avoid unnecessary
optimization as it complicates the code base and makes folks reviewing
the code wonder at the exceptions.
I have attached a patch for consideration/discussion that addresses these
items. I have based it off of current master (0c013e0). I have done some
cursory testing including check-world with success.
My 2c about this function is that it should be completely removed
and the place where it's checked replaced with just the
'has_rolreplication' call and error. It's only called in one
place and it'd be a simple one-liner anyway. As for
has_rolreplication, I don't understand why it's in miscinit.c when
the rest of the has_* set is in acl.c.
With all the other pg_has_* functions being found in acl.c I agree that it
would seem odd that this (or any other related function) would be found
elsewhere. Since aclchk.c also has a couple of functions that follow the
pattern of "has_<priv>_privilege", I moved this function there, for now, as
well as modified it to include superuser checks as they were not previously
there. The only related function I found in acl.c was "has_rolinherit"
which is a static function. :-/ There is also a static function
"has_rolcatupdate" in aclchk.c. I would agree that acl.c (or aclchk.c?)
would be a more appropriate location for these functions, though in some of
the above cases, it might require exposing them (I'm not sure that I see
any harm in doing so).
Perhaps refactoring to consolidate all of these in either acl.c or aclchk.c
with the following pattern might be a step in the right direction?
* has_createrole_privilege
* has_bypassrls_privilege
* has_inherit_privilege
* has_catupdate_privilege
* has_replication_privilege
* has_???_privilege
In either case, I think following a "convention" on the naming of these
functions (unless there is semantic reason not to) would also help to
reduce any confusion or head scratching.
Removing these design patterns may also help to avoid ending up with
more of them in the future as folks copy and/or crib off of what we've
already done to implement their features...
I agree.
-Adam
--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Attachments:
superuser-cleanup-shortcuts_10-1-2014.patchtext/x-patch; charset=US-ASCII; name=superuser-cleanup-shortcuts_10-1-2014.patchDownload+216-209
Adam,
* Brightwell, Adam (adam.brightwell@crunchydatasolutions.com) wrote:
We, as a community, have gotten flak from time-to-time about the
superuser role. We also tend to wish to avoid unnecessary
optimization as it complicates the code base and makes folks reviewing
the code wonder at the exceptions.I have attached a patch for consideration/discussion that addresses these
items. I have based it off of current master (0c013e0). I have done some
cursory testing including check-world with success.
Thanks! Please add it to the next commitfest.
With all the other pg_has_* functions being found in acl.c I agree that it
would seem odd that this (or any other related function) would be found
elsewhere. Since aclchk.c also has a couple of functions that follow the
pattern of "has_<priv>_privilege", I moved this function there, for now, as
well as modified it to include superuser checks as they were not previously
there. The only related function I found in acl.c was "has_rolinherit"
which is a static function. :-/ There is also a static function
"has_rolcatupdate" in aclchk.c. I would agree that acl.c (or aclchk.c?)
would be a more appropriate location for these functions, though in some of
the above cases, it might require exposing them (I'm not sure that I see
any harm in doing so).
I don't think has_rolinherit or has_rolcatupdate really need to move and
it seems unlikely that they'd be needed from elsewhere.. Is there a
reason you think they'd need to be exposed? I've not looked at the
patch at all though, perhaps that makes it clear.
Perhaps refactoring to consolidate all of these in either acl.c or aclchk.c
with the following pattern might be a step in the right direction?* has_createrole_privilege
* has_bypassrls_privilege
These are already in the right place, right?
* has_inherit_privilege
* has_catupdate_privilege
These probably don't need to move as they're only used in the .c files
that they're defined in (unless there's a reason that needs to change).
* has_replication_privilege
This is what I was on about, so I agree that it should be created. ;)
* has_???_privilege
Right, other things might be 'has_backup_privilege', for things like
pg_start/stop_backup and friends.
In either case, I think following a "convention" on the naming of these
functions (unless there is semantic reason not to) would also help to
reduce any confusion or head scratching.
Agreed.
Thanks!
Stephen
Stephen,
Thanks! Please add it to the next commitfest.
Sounds good. I'll update the patch and add accordingly.
I don't think has_rolinherit or has_rolcatupdate really need to move and
it seems unlikely that they'd be needed from elsewhere.. Is there a
reason you think they'd need to be exposed? I've not looked at the
patch at all though, perhaps that makes it clear.
There is no reason to expose them (at this point in time) other than
consolidation.
* has_createrole_privilege
* has_bypassrls_privilege
These are already in the right place, right?
If aclchk.c is the right place, then yes. :-)
* has_inherit_privilege
* has_catupdate_privilegeThese probably don't need to move as they're only used in the .c files
that they're defined in (unless there's a reason that needs to change).
Correct, though, I don't see any reason for them to move other than
attempting to consolidate them.
* has_???_privilege
Right, other things might be 'has_backup_privilege', for things like
pg_start/stop_backup and friends.
Correct.
-Adam
--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
All,
Thanks! Please add it to the next commitfest.
Sounds good. I'll update the patch and add accordingly.
Attached is an updated patch.
-Adam
--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Attachments:
superuser-cleanup-shortcuts_10-3-2014.patchtext/x-patch; charset=US-ASCII; name=superuser-cleanup-shortcuts_10-3-2014.patchDownload+240-240
Brightwell, Adam wrote:
All,
Thanks! Please add it to the next commitfest.
Sounds good. I'll update the patch and add accordingly.
Attached is an updated patch.
I noticed something strange while perusing this patch, but the issue
predates the patch. Some messages say "must be superuser or replication
role to foo", but our longstanding practice is to say "permission denied
to foo". Why do we have this inconsistency? Should we remove it? If
we do want to keep the old wording this patch should use "permission
denied to" in the places that it touches.
Other than that, since we already agreed that it's something we want,
the only comment I have about this patch is an empty line in variable
declarations here which should be removed:
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c new file mode 100644 index c9a9baf..ed89b23 *** a/src/backend/commands/alter.c
--- 807,848 ---- bool *nulls; bool *replaces;! AclObjectKind aclkind = get_object_aclkind(classId);
!
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro,
I noticed something strange while perusing this patch, but the issue
predates the patch. Some messages say "must be superuser or replication
role to foo", but our longstanding practice is to say "permission denied
to foo". Why do we have this inconsistency? Should we remove it? If
we do want to keep the old wording this patch should use "permission
denied to" in the places that it touches.
If we were to make it consistent and use the old wording, what do you
think about providing an "errhint" as well?
Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot:
errmsg - "permission denied to create physical replication slot"
errhint - "You must be superuser or replication role to use replication slots."
-Adam
--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I noticed something strange while perusing this patch, but the issue
predates the patch. Some messages say "must be superuser or replication
role to foo", but our longstanding practice is to say "permission denied
to foo". Why do we have this inconsistency? Should we remove it? If
we do want to keep the old wording this patch should use "permission
denied to" in the places that it touches.If we were to make it consistent and use the old wording, what do you
think about providing an "errhint" as well?Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot:
errmsg - "permission denied to create physical replication slot"
errhint - "You must be superuser or replication role to use replication slots."
As I started looking at this, there are multiple other places where
these types of error messages occur (opclasscmds.c, user.c,
postinit.c, miscinit.c are just a few), not just around the changes in
this patch. If we change them in one place, wouldn't it be best to
change them in the rest? If that is the case, I'm afraid that might
distract from the purpose of this patch. Perhaps, if we want to
change them, then that should be submitted as a separate patch?
-Adam
--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Brightwell, Adam wrote:
If we were to make it consistent and use the old wording, what do you
think about providing an "errhint" as well?Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot:
errmsg - "permission denied to create physical replication slot"
errhint - "You must be superuser or replication role to use replication slots."
Sure.
As I started looking at this, there are multiple other places where
these types of error messages occur (opclasscmds.c, user.c,
postinit.c, miscinit.c are just a few), not just around the changes in
this patch. If we change them in one place, wouldn't it be best to
change them in the rest? If that is the case, I'm afraid that might
distract from the purpose of this patch. Perhaps, if we want to
change them, then that should be submitted as a separate patch?
Yeah. I'm just saying that maybe this patch should adopt whatever
wording we agree to, not that we need to change other places. On the
other hand, since so many other places have adopted the different
wording, maybe there's a reason for it and if so, does anybody know what
it is. But I have to say that it does look inconsistent to me.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 10/23/14, 6:23 PM, Alvaro Herrera wrote:
Brightwell, Adam wrote:
If we were to make it consistent and use the old wording, what do you
think about providing an "errhint" as well?Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot:
errmsg - "permission denied to create physical replication slot"
errhint - "You must be superuser or replication role to use replication slots."Sure.
As I started looking at this, there are multiple other places where
these types of error messages occur (opclasscmds.c, user.c,
postinit.c, miscinit.c are just a few), not just around the changes in
this patch. If we change them in one place, wouldn't it be best to
change them in the rest? If that is the case, I'm afraid that might
distract from the purpose of this patch. Perhaps, if we want to
change them, then that should be submitted as a separate patch?Yeah. I'm just saying that maybe this patch should adopt whatever
wording we agree to, not that we need to change other places. On the
other hand, since so many other places have adopted the different
wording, maybe there's a reason for it and if so, does anybody know what
it is. But I have to say that it does look inconsistent to me.
Keep in mind that originally the ONLY special permissions "thing" we had was rolsuper. Now we also have replication, maybe some others, and there's discussion of adding a special "backup role".
In other words, the situation is a lot more complex than it used to be. So if cleanup is done here, it would be best to try and account for this new stuff.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Brightwell, Adam wrote:
If we were to make it consistent and use the old wording, what do you
think about providing an "errhint" as well?Perhaps for example in slotfuncs.c#pg_create_physical_replication_stot:
errmsg - "permission denied to create physical replication slot"
errhint - "You must be superuser or replication role to use replication slots."Sure.
Sounds reasonable to me and looks to be in-line with wording elsewhere.
As I started looking at this, there are multiple other places where
these types of error messages occur (opclasscmds.c, user.c,
postinit.c, miscinit.c are just a few), not just around the changes in
this patch. If we change them in one place, wouldn't it be best to
change them in the rest? If that is the case, I'm afraid that might
distract from the purpose of this patch. Perhaps, if we want to
change them, then that should be submitted as a separate patch?Yeah. I'm just saying that maybe this patch should adopt whatever
wording we agree to, not that we need to change other places. On the
other hand, since so many other places have adopted the different
wording, maybe there's a reason for it and if so, does anybody know what
it is. But I have to say that it does look inconsistent to me.
I agree that this patch should adopt the wording agreed to above and
that it's 'more similar' to most of the usage in the backend. As for
the general question, I'd suggest we add to the error message policy
that more-or-less anything with errcode(ERRCODE_INSUFFICIENT_PRIVILEGE)
have errmsg("permission denied to X") with any comments about 'must be a
superuser' or similar relegated to errhint(). We could add that as a
TODO and have more junior developers review these cases and come up with
patches (or argue for cases where they feel deviation is warranted).
Either way, that larger rework isn't for this patch and since you seem
happy with it (modulo the change above), I'm going to make that change,
do my own review, and move foward with it unless someone else wants to
speak up.
Thanks!
Stephen
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
As I started looking at this, there are multiple other places where
these types of error messages occur (opclasscmds.c, user.c,
postinit.c, miscinit.c are just a few), not just around the changes in
this patch. If we change them in one place, wouldn't it be best to
change them in the rest? If that is the case, I'm afraid that might
distract from the purpose of this patch. Perhaps, if we want to
change them, then that should be submitted as a separate patch?Yeah. I'm just saying that maybe this patch should adopt whatever
wording we agree to, not that we need to change other places. On the
other hand, since so many other places have adopted the different
wording, maybe there's a reason for it and if so, does anybody know what
it is. But I have to say that it does look inconsistent to me.
Updated patch attached. Comments welcome.
Thanks!
Stephen
Attachments:
superuser_cleanup.patchtext/x-diff; charset=us-asciiDownload+264-263
All,
* Stephen Frost (sfrost@snowman.net) wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
As I started looking at this, there are multiple other places where
these types of error messages occur (opclasscmds.c, user.c,
postinit.c, miscinit.c are just a few), not just around the changes in
this patch. If we change them in one place, wouldn't it be best to
change them in the rest? If that is the case, I'm afraid that might
distract from the purpose of this patch. Perhaps, if we want to
change them, then that should be submitted as a separate patch?Yeah. I'm just saying that maybe this patch should adopt whatever
wording we agree to, not that we need to change other places. On the
other hand, since so many other places have adopted the different
wording, maybe there's a reason for it and if so, does anybody know what
it is. But I have to say that it does look inconsistent to me.Updated patch attached. Comments welcome.
Looking over this again, I had another thought about it- given that this
changes the error messages returned for replication slots, which are new
in 9.4, should it be back-patched to 9.4? Otherwise we'll put 9.4
out and then immediately change these error messages in 9.5.
That said, it seems likely we'll be doing a more thorough review and
update of error messages for 9.5 (if others agree with my up-thread
proposal), such that these changes would be minor additional ones.
Thoughts? I don't have a preference either way, which makes me lean
towards not messing with 9.4, but wanted to bring it up.
Thanks!
Stephen
On 2014-10-28 09:43:35 -0400, Stephen Frost wrote:
All,
* Stephen Frost (sfrost@snowman.net) wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
As I started looking at this, there are multiple other places where
these types of error messages occur (opclasscmds.c, user.c,
postinit.c, miscinit.c are just a few), not just around the changes in
this patch. If we change them in one place, wouldn't it be best to
change them in the rest? If that is the case, I'm afraid that might
distract from the purpose of this patch. Perhaps, if we want to
change them, then that should be submitted as a separate patch?Yeah. I'm just saying that maybe this patch should adopt whatever
wording we agree to, not that we need to change other places. On the
other hand, since so many other places have adopted the different
wording, maybe there's a reason for it and if so, does anybody know what
it is. But I have to say that it does look inconsistent to me.Updated patch attached. Comments welcome.
Looking over this again, I had another thought about it- given that this
changes the error messages returned for replication slots, which are new
in 9.4, should it be back-patched to 9.4? Otherwise we'll put 9.4
out and then immediately change these error messages in 9.5.
-1.
For one I'm less than convinced that the new messages are an
improvement. They seem to be more verbose without a corresponding
improvement in clarity.
For another I don't see any need to rush this into 9.4.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Andres Freund (andres@2ndquadrant.com) wrote:
For one I'm less than convinced that the new messages are an
improvement. They seem to be more verbose without a corresponding
improvement in clarity.
The goal with the changes is to improve consistency of messaging. These
messages are not at all consistent today. Personally, I like the idea
of being clear in the main errmsg() that these are permission denied
errors, but we could go the other way and change all the existing
messages which say 'permission denied' to only say 'you have to be
superuser or have X' instead and expect folks to realize it's a
permission denied error from the SQL error code..
For another I don't see any need to rush this into 9.4.
Ok.
Thanks!
Stephen
On 10/27/14 11:40 AM, Stephen Frost wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
As I started looking at this, there are multiple other places where
these types of error messages occur (opclasscmds.c, user.c,
postinit.c, miscinit.c are just a few), not just around the changes in
this patch. If we change them in one place, wouldn't it be best to
change them in the rest? If that is the case, I'm afraid that might
distract from the purpose of this patch. Perhaps, if we want to
change them, then that should be submitted as a separate patch?Yeah. I'm just saying that maybe this patch should adopt whatever
wording we agree to, not that we need to change other places. On the
other hand, since so many other places have adopted the different
wording, maybe there's a reason for it and if so, does anybody know what
it is. But I have to say that it does look inconsistent to me.Updated patch attached. Comments welcome.
The changes in
src/backend/commands/alter.c
src/backend/commands/foreigncmds.c
src/backend/commands/tablecmds.c
src/backend/commands/typecmds.c
are OK, because the superuser() calls are redundant, and cleaning this
up is clearly in line with planned future work.
I suggest moving the rest of the changes into separate patches.
The error message wording changes might well be worth considering, but
there are tons more "must be $someone to do $something" error messages,
and changing two of them isn't making anything more or less consistent.
The ha*_something_privilege() changes are also not very consistent.
We already have have_createrole_privilege(), which does include a
superuser check, and you add has_replication_privilege() with a
superuser check, but has_catupdate_privilege() and
has_inherit_privilege() don't include a superuser check. That's clearly
a mess.
Btw., why rename have_createrole_privilege()?
Also, your patch has spaces between tabs. Check for whitespace errors
with git.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thanks for looking at this patch.
I suggest moving the rest of the changes into separate patches.
Hmmm... perhaps the following?
* superuser-cleanup - contains above mentioned superuser shortcuts only.
* has_privilege-cleanup - contains has_*_priviledge cleanup only.
Would that also require a separate commitfest entry?
The ha*_something_privilege() changes are also not very consistent.
We already have have_createrole_privilege(), which does include a
superuser check, and you add has_replication_privilege() with a
superuser check, but has_catupdate_privilege() and
has_inherit_privilege() don't include a superuser check. That's clearly
a mess.
Good catch. Though, according to the documentation, not even superuser is
allowed to bypass CATUPDATE.
http://www.postgresql.org/docs/9.4/static/catalog-pg-authid.html.
However, I can't think of a reason why "inherit" wouldn't need the
superuser check. Obviously superuser is considered a member of every role,
but is there a reason that a superuser would not be allowed to bypass
this? I only ask because it did not have a check previously, so I figure
there might have been a good reason for it?
Btw., why rename have_createrole_privilege()?
Well, actually it wasn't necessarily a rename. It was a removal of that
function all together as all it did was simply return the result of
"has_createrole_privilege". That seemed rather redundant and unnecessary,
IMO.
Also, your patch has spaces between tabs. Check for whitespace errors
with git.
Yikes.
-Adam
--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
Attached is two separate patches to address previous
comments/recommendations:
* superuser-cleanup-shortcuts_11-5-2014.patch
* has_privilege-cleanup_11-5-2014.patch
-Adam
--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
On 11/5/14 5:10 PM, Adam Brightwell wrote:
Attached is two separate patches to address previous
comments/recommendations:* superuser-cleanup-shortcuts_11-5-2014.patch
Seeing that the regression tests had to be changed in this patch
indicates that there is a change of functionality, even though this
patch was previously discussed as merely an internal cleanup. So either
there is a user-facing change, which would need to be documented (or at
least mentioned, discussed, and dismissed as minor), or there is a fault
in the tests, which should be fixed first independently.
Which makes me wonder whether the other changes are indeed without
effect or just not covered by tests.
* has_privilege-cleanup_11-5-2014.patch
I don't really see the merit of this patch. A "cleanup" patch that adds
more code than it removes isn't really a cleanup. If you want to make
the APIs consistent, then let's make a patch for that. If you want to
make the error messages consistent, then let's make a patch for that.
There is other work going on replacing these role attributes with
something more general, so maybe this cleanup should be done as part of
that other work.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2014-11-05 17:10:17 -0500, Adam Brightwell wrote:
Attached is two separate patches to address previous
comments/recommendations:* superuser-cleanup-shortcuts_11-5-2014.patch
* has_privilege-cleanup_11-5-2014.patch-Adam
--
Adam Brightwell - adam.brightwell@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com
diff --git a/contrib/test_decoding/expected/permissions.out b/contrib/test_decoding/expected/permissions.out new file mode 100644 index 212fd1d..f011955 *** a/contrib/test_decoding/expected/permissions.out --- b/contrib/test_decoding/expected/permissions.out *************** RESET ROLE; *** 54,66 **** -- plain user *can't* can control replication SET ROLE lr_normal; SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding'); ! ERROR: must be superuser or replication role to use replication slots INSERT INTO lr_test VALUES('lr_superuser_init'); ERROR: permission denied for relation lr_test SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1'); ! ERROR: must be superuser or replication role to use replication slots SELECT pg_drop_replication_slot('regression_slot'); ! ERROR: must be superuser or replication role to use replication slots RESET ROLE; -- replication users can drop superuser created slots SET ROLE lr_superuser; --- 54,69 ---- -- plain user *can't* can control replication SET ROLE lr_normal; SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding'); ! ERROR: permission denied to use replication slots ! HINT: You must be superuser or replication role to use replication slots. INSERT INTO lr_test VALUES('lr_superuser_init'); ERROR: permission denied for relation lr_test SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1'); ! ERROR: permission denied to use replication slots ! HINT: You must be superuser or replication role to use replication slots. SELECT pg_drop_replication_slot('regression_slot'); ! ERROR: permission denied to use replication slots ! HINT: You must be superuser or replication role to use replication slots. RESET ROLE; -- replication users can drop superuser created slots SET ROLE lr_superuser;
I still think this change makes the error message more verbose, without
any win in clarity.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
