On Wed, Apr 30, 2025 at 4:29 PM Tom Lane <t...@sss.pgh.pa.us> wrote: > However, I'd at least like to complain about the fact that > it breaks pg_dumpall, which is surely not expecting anything > but the default behavior. If for any reason the restore is > run under a non-default setting of createrole_self_grant, > there's a potential of creating role grants that were not > there in the source database. Admittedly the damage is > probably limited by the fact that it only applies if the > restoring user has CREATEROLE but not SUPERUSER, which > I imagine is a rare case. But don't we need to add > createrole_self_grant to the set of GUCs that pg_dump[all] > resets in the emitted SQL?
I spent some time on this today. As you might imagine, it's quite easy to make pg_dumpall emit SET createrole_self_grant = '', but doing so seems less useful than I had expected. I wonder if I'm missing something important here, so I thought I'd better inquire before proceeding further. As I see it, the core difficulty is that the output of pg_dumpall always contains a CREATE ROLE statement for every single role in the system, even the bootstrap superuser. For starters, that means that if you simply run initdb and create cluster #1, run initdb again and create cluster #2, dump the first and restore to the second, you will get an error, because the same bootstrap superuser will exist in both systems, so when you feed the output of pg_dumpall to psql, you end up trying to create a role that already exists. At this point, my head is already kind of exploding, because I thought we were pretty careful to try to make it so that pg_dump output can be restored without error even in the face of pre-existing objects like the public schema and the plpgsql language, but apparently we haven't applied the same principle to pg_dumpall.[1] But if, as you posit above, we were to try running the output of pg_dumpall through psql as a non-superuser, the problem is a whole lot worse. You can imagine a pg_dumpall feature that only tries to dump (on the source system) roles that the dumping user can administer, and only tries to recreate those roles on the target system, but we haven't got that feature, so we're going to try to recreate every single source role on the target system, including the bootstrap user and the non-superuser who is restoring the dump if they exist on the source side and any other superusers and any other users created by other CREATEROLE superusers and it seems to me that under any set of somewhat-reasonable assumptions you're going to expect a bunch of error messages to start showing up at this point. In short, trying to restore pg_dumpall output as a non-superuser appears to be an unsupported scenario, so the fact that we don't SET createrole_self_grant = '' to cater to it doesn't really seem like a bug to many any more. In fact, I think there's a decent argument that we ought to let the prevailing value of createrole_self_grant take effect in this scenario. One pretty likely scenario, at least as it seems to me, is that the user was superuser on the source system and is not superuser on the target system but wants to recreate the same set of roles. If they want to freely access objects owned by those roles as they could on the source system, then they're going to need self-grants, and we have no better clue than the value of createrole_self_grant to help us figure out whether they want that or not. To state the concern another way, if this is a bug, it should be possible to construct a test case that fails without the patch and passes with the patch. But it appears to me that the only way I could do that is if I programatically edit the dump. And that seems like cheating, because if we are talking about a scenario where the user is editing the dump, they can also add SET createrole_self_grant = '' if desired. I don't want to make it sound like I now hate the idea of doing as you proposed here, because I do see the point of nailing down critical GUCs that can affect the interpretation of SQL statements in places like pg_dumpall output, and maybe we should do that here ... kinda just in case? But I'm not altogether sure that's a sufficient justification, and at any rate I think we need to be clear on whether that *is* the justification or whether there's something more concrete that we're trying to make work. -- Robert Haas EDB: http://www.enterprisedb.com [1] Exception: When --binary-upgrade is used, we emit only ALTER ROLE and not CREATE ROLE for the bootstrap superuser. Why we think the error is only worth avoiding in the --binary-upgrade case is unclear to me.
