On Thu, Aug 25, 2022 at 4:41 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > Yeah, I'd lean against back-patching. This is the sort of behavioral > change that users tend not to like finding in minor releases.
Here's a small patch. Despite the small size of the patch, there are a couple of debatable points here: 1. Should we have a code comment? I feel it isn't necessary, because there's a comment just a few lines earlier saying "Look up the role OIDs and do permissions checks" and that seems like sufficient justification for what follows. 2. What about the error message? Personally, I'm not very excited about "permission denied to whatever" as a way to phrase an error message. It doesn't sound like particularly good grammar to me. But it's the phrasing we use elsewhere, so I guess we should do the same here. 3. Should we have a test case? We are extremely thin on test cases for NOINHERIT behavior, it seems, and testing this one thing when we don't test anything else seems relatively useless. Also, privileges.sql is a giant mess. It's a 1700+ line test file that tests many fairly unrelated things. I am inclined to think that this file badly needs to be split up into a bunch of smaller files, because it's practically unmaintainable as is. For instance, the stuff at the top of the file is testing a bunch of things about role privileges, but then check some stuff about leakproof functions before coming back to test stuff about groups, which logically probably belongs with the role privileges stuff. Perhaps a reasonable starting split would be something like: - Privileges on roles. - Privileges on relations. - Privileges on other kinds of objects. - Default privileges. - Security barriers and leakproof functions. - Security-restricted operations. Some of those files might be fairly small initially, but they might be get bigger later, especially because it'd be a heck of a lot easier to add new test cases if you didn't have to worry that some change you make is going to break a test 1000 lines down in the file. -- Robert Haas EDB: http://www.enterprisedb.com
alterdefaultprivs-v1.patch
Description: Binary data
