public inbox for [email protected]
help / color / mirror / Atom feedRe: Major Version Upgrade failure due to orphan roles entries in catalog
13+ messages / 2 participants
[nested] [flat]
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
@ 2026-02-25 17:33 Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Robert Haas @ 2026-02-25 17:33 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
On Wed, Feb 25, 2026 at 11:50 AM Tom Lane <[email protected]> wrote:
> I don't entirely agree that it's "business as usual": somebody screwed
> up to get the database into that state. I'm not here to apportion
> blame for that between the user and the database, but I do think it's
> appropriate for pg_dumpall to complain that it's facing invalid data.
>
> If you're good with pg_dumpall producing a warning and then emitting
> the GRANT with no grantor clause, I will go make that happen.
Well, I guess I don't really see why there should be a warning. I
mean, if you're not here to apportion blame between the user and
database, then allow me to step in: in v15-, it's entirely the
database's fault. It does absolutely zilch to keep that field valid.
As I showed in the earlier example, all the user has to do is create a
role, do something, and drop the role, and that field is no longer
valid. That's about the simplest scenario imaginable, and I don't
think any user would expect it to produce a corrupted database, and I
think the status quo ante prior to this commit was that we did not
consider that to be a corrupted database.
If that feels like the wrong answer to you, I suggest that the reason
is that you're not used to PostgreSQL code being as horrifically bad
as this code was prior to v16. I don't know of anywhere else in the
database where we store an OID and do absolutely nothing to ensure its
validity. Sure, we've found some holes over the years where we didn't
do enough, especially around concurrency, and we've probably all seen
examples of TOAST corruption where a value can't be de-TOASTed due to
some inconsistency. But those are examples of where we had had some
guards in place and they weren't as thorough as they needed to be, or
where the user voided the warranty by doing something stupid, or where
the storage manhandled the data. But before
6566133c5f52771198aca07ed18f84519fac1be7, we didn't just have
inadequate machinery in this area: we had none. I remember being
absolutely gobsmacked at this discovery that it had been this way for
years and apparently nobody was too fussed about it. It seemed to me
then and it seems to me now that such handling was way below our usual
quality standards, even for older code when we weren't as strict as we
are today. But that's nevertheless how it was.
Now, if you go and do as you propose here, and adjust the code so that
the grant is dumped but a warning is produced, my fear is that someone
upgrading from v15- to v16+ will see that warning and think that there
is a problem with their database that needs fixing. But, except for
the fact that they should really upgrade to a newer and better
version, that's not really true. They're just running a version where
no care was put into making that field valid, and so sometimes it
isn't. I suppose that is OK in the end: when such a user files a
support ticket with $EMPLOYER or any other company, somebody can
simply tell that user that the warning requires no corrective action
and can be safely ignored. But of course, they'll have to know that
this is the case, and not everyone will have read this discussion.
Moreover, we'll emit essentially the same warning for the member case,
where the warning does point to a problem that someone might want to
think about correcting, and exactly the same warning against a v16+
database where it indicates that something has actually gone wrong. I
don't know why we would want to do that instead of what I proposed,
but if you're dead-set on it, I can live with it: it will at least
mean people can upgrade without having perfectly good role grants
disappear into the ether.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
@ 2026-02-25 17:39 ` Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2026-02-25 17:39 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
Robert Haas <[email protected]> writes:
> On Wed, Feb 25, 2026 at 11:50 AM Tom Lane <[email protected]> wrote:
>> If you're good with pg_dumpall producing a warning and then emitting
>> the GRANT with no grantor clause, I will go make that happen.
> Well, I guess I don't really see why there should be a warning.
Because the result of the restore will not match how things were
in the source database? True, we do not have any way to make them
match, but that doesn't mean that pg_dumpall has fulfilled all
expectations.
> Now, if you go and do as you propose here, and adjust the code so that
> the grant is dumped but a warning is produced, my fear is that someone
> upgrading from v15- to v16+ will see that warning and think that there
> is a problem with their database that needs fixing.
On the other hand, if we produce no warning and yet the restored DB
is unlike the original, that could also be cause for concern.
> Moreover, we'll emit essentially the same warning for the member case,
> where the warning does point to a problem that someone might want to
> think about correcting, and exactly the same warning against a v16+
> database where it indicates that something has actually gone wrong.
That's a fair point, but maybe it could be addressed by phrasing the
message differently for the different cases.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
@ 2026-02-25 18:02 ` Robert Haas <[email protected]>
2026-02-25 18:30 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Robert Haas @ 2026-02-25 18:02 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
On Wed, Feb 25, 2026 at 12:39 PM Tom Lane <[email protected]> wrote:
> Because the result of the restore will not match how things were
> in the source database? True, we do not have any way to make them
> match, but that doesn't mean that pg_dumpall has fulfilled all
> expectations.
That's a fair point. On the flip side, the grantor in v15- is
basically a comment. From v16, it behaves in the way we'd normally
expect: a role grant depends on the grantor in the same way that a
table grant depends on the grantor in the table ACL, just with a
different catalog representation. In v15-, it has no functional
impact.
> > Moreover, we'll emit essentially the same warning for the member case,
> > where the warning does point to a problem that someone might want to
> > think about correcting, and exactly the same warning against a v16+
> > database where it indicates that something has actually gone wrong.
>
> That's a fair point, but maybe it could be addressed by phrasing the
> message differently for the different cases.
I like that idea. I think the biggest danger here for users is
actually that we have to convert the pre-v16 "it's a comment" catalog
state into a v16+ "it's a real thing that works like the rest of the
authorization system" catalog state, and there's no way to do that
perfectly because we're starting from a busted catalog representation.
It seems reasonable to somehow let people know that whatever we're
doing is an approximation. If we can make the "invalid grantor from
pre-v16" sound like "don't panic but we have to patch some stuff up so
you can upgrade" and the "invalid grantor for v16+" and "invalid
member" cases sound like "uh oh, it seems like something got
corrupted" then I'm entirely happy with that state of affairs
(assuming we also resume dumping the GRANT, of course).
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
@ 2026-02-25 18:30 ` Tom Lane <[email protected]>
2026-02-25 18:47 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2026-02-25 18:30 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
Robert Haas <[email protected]> writes:
> On Wed, Feb 25, 2026 at 12:39 PM Tom Lane <[email protected]> wrote:
>>> Moreover, we'll emit essentially the same warning for the member case,
>>> where the warning does point to a problem that someone might want to
>>> think about correcting, and exactly the same warning against a v16+
>>> database where it indicates that something has actually gone wrong.
>> That's a fair point, but maybe it could be addressed by phrasing the
>> message differently for the different cases.
> I like that idea.
OK, so we need to pick wordings. Right now we use this wording for
all three cases:
/* translator: %s represents a numeric role OID */
pg_log_warning("found orphaned pg_auth_members entry for role %s",
PQgetvalue(res, start, i_roleid));
I don't really love that wording for any of these cases, because
(a) "orphaned" isn't a word we use much, and (b) conflating the
role, member, and grantor doesn't seem helpful. How about
something like
missing role:
"ignoring role grant for missing role with OID nnn"
missing member:
"ignoring role grant to missing role with OID nnn"
missing grantor, source version >= 16:
"role grant of R1 to R2 was granted by missing role with OID nnn"
with detail
"We'll dump the GRANT without a GRANTED BY clause, but this shouldn't happen."
missing grantor, source version < 16:
"role grant of R1 to R2 was granted by missing role with OID nnn"
with detail
"This state isn't unusual. We'll dump the GRANT without a GRANTED BY clause."
(We have pg_log_warning_detail back to v16, so it's okay to rely on
a detail message.) I feel like this could use more word-smithing,
but it's covering more or less the right ground IMO.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:30 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
@ 2026-02-25 18:47 ` Robert Haas <[email protected]>
2026-02-25 18:58 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Robert Haas @ 2026-02-25 18:47 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
On Wed, Feb 25, 2026 at 1:30 PM Tom Lane <[email protected]> wrote:
> missing grantor, source version >= 16:
>
> "role grant of R1 to R2 was granted by missing role with OID nnn"
> with detail
> "We'll dump the GRANT without a GRANTED BY clause, but this shouldn't happen."
>
> missing grantor, source version < 16:
>
> "role grant of R1 to R2 was granted by missing role with OID nnn"
> with detail
> "This state isn't unusual. We'll dump the GRANT without a GRANTED BY clause."
>
> (We have pg_log_warning_detail back to v16, so it's okay to rely on
> a detail message.) I feel like this could use more word-smithing,
> but it's covering more or less the right ground IMO.
Yeah, I agree both that it's directionally correct and that more
word-smithing might help. I feel like your main message is better than
your detail, but I'm not sure exactly what would be an improvement.
Sometimes I find it helpful to play with the message level and the
primary message wording as a way of indicating the gravity, e.g.
info: dumping grant of role \"%s\" to role \"%s\" without GRANTED BY,
because role with OID %u no longer exists
vs.
warning: grant of role \"%s\" to \"%s\" has invalid grantor OID %u
detail: this grant will be dumped without GRANTED BY
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:30 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:47 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
@ 2026-02-25 18:58 ` Tom Lane <[email protected]>
2026-02-27 21:22 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2026-02-25 18:58 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
Robert Haas <[email protected]> writes:
> Sometimes I find it helpful to play with the message level and the
> primary message wording as a way of indicating the gravity, e.g.
> info: dumping grant of role \"%s\" to role \"%s\" without GRANTED BY,
> because role with OID %u no longer exists
> vs.
> warning: grant of role \"%s\" to \"%s\" has invalid grantor OID %u
> detail: this grant will be dumped without GRANTED BY
I'd be content to use those wordings (except I think our style
guide wants detail to be punctuated like a sentence). I'll wait
a day or so to see if anyone has a better idea, though.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:30 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:47 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:58 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
@ 2026-02-27 21:22 ` Tom Lane <[email protected]>
2026-02-27 21:28 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2026-02-27 21:22 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
I wrote:
> I'd be content to use those wordings (except I think our style
> guide wants detail to be punctuated like a sentence). I'll wait
> a day or so to see if anyone has a better idea, though.
When I looked at the code more closely, I realized that it already
doesn't dump grantors when dumping from pre-v16. I'm inclined to
think that is an overreaction to the possible unreliability of the
data (and from your comment upthread you might agree). But given
the lack of complaints about it, let's leave that as-is for now.
The immediate problem is that the code is allowing a null grantor to
suppress the GRANT altogether, *even if it's not going to use the
grantor*. Clearly that's silly. But if we're not going to use
the grantor, let's skip trying to fetch it, which means we also
don't need the info-level variant of the message.
So I end with the attached draft patch.
regards, tom lane
#text/x-diff; name="v1-clean-up-dumpRoleMembership.patch" [v1-clean-up-dumpRoleMembership.patch] /home/tgl/pgsql/v1-clean-up-dumpRoleMembership.patch
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:30 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:47 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:58 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:22 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
@ 2026-02-27 21:28 ` Tom Lane <[email protected]>
2026-02-27 21:42 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2026-02-27 21:28 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
I wrote:
> So I end with the attached draft patch.
Sigh, this time with it really attached.
regards, tom lane
Attachments:
[text/x-diff] v1-clean-up-dumpRoleMembership.patch (4.0K, 2-v1-clean-up-dumpRoleMembership.patch)
download | inline diff:
diff --git a/src/bin/pg_dump/pg_dumpall.c b/src/bin/pg_dump/pg_dumpall.c
index 65f8e3a41f1..3062636a2ce 100644
--- a/src/bin/pg_dump/pg_dumpall.c
+++ b/src/bin/pg_dump/pg_dumpall.c
@@ -1292,7 +1292,7 @@ dumpRoleMembership(PGconn *conn)
* that no longer exist. If we find such cases, print a warning and skip
* the entry.
*/
- dump_grantors = (PQserverVersion(conn) >= 160000);
+ dump_grantors = (server_version >= 160000);
/*
* Previous versions of PostgreSQL also did not have grant-level options.
@@ -1357,7 +1357,7 @@ dumpRoleMembership(PGconn *conn)
if (PQgetisnull(res, start, i_role))
{
/* translator: %s represents a numeric role OID */
- pg_log_warning("found orphaned pg_auth_members entry for role %s",
+ pg_log_warning("ignoring role grant for missing role with OID %s",
PQgetvalue(res, start, i_roleid));
break;
}
@@ -1374,6 +1374,11 @@ dumpRoleMembership(PGconn *conn)
remaining = end - start;
done = pg_malloc0_array(bool, remaining);
+
+ /*
+ * We use a hashtable to track the member names that have been granted
+ * admin option. Usually a hashtable is overkill, but sometimes not.
+ */
ht = rolename_create(remaining, NULL);
/*
@@ -1401,50 +1406,56 @@ dumpRoleMembership(PGconn *conn)
for (i = start; i < end; ++i)
{
char *member;
- char *admin_option;
char *grantorid;
- char *grantor;
+ char *grantor = NULL;
+ bool dump_grantor = dump_grantors;
char *set_option = "true";
+ char *admin_option;
bool found;
/* If we already did this grant, don't do it again. */
if (done[i - start])
continue;
- /* Complain about, then ignore, entries with orphaned OIDs. */
+ /* Complain about, then ignore, entries for unknown members. */
if (PQgetisnull(res, i, i_member))
{
/* translator: %s represents a numeric role OID */
- pg_log_warning("found orphaned pg_auth_members entry for role %s",
+ pg_log_warning("ignoring role grant to missing role with OID %s",
PQgetvalue(res, i, i_memberid));
done[i - start] = true;
--remaining;
continue;
}
- if (PQgetisnull(res, i, i_grantor))
+ member = PQgetvalue(res, i, i_member);
+
+ /* If the grantor is unknown, complain and dump without it. */
+ grantorid = PQgetvalue(res, i, i_grantorid);
+ if (dump_grantor)
{
- /* translator: %s represents a numeric role OID */
- pg_log_warning("found orphaned pg_auth_members entry for role %s",
- PQgetvalue(res, i, i_grantorid));
- done[i - start] = true;
- --remaining;
- continue;
+ if (PQgetisnull(res, i, i_grantor))
+ {
+ /* translator: %s represents a numeric role OID */
+ pg_log_warning("grant of role \"%s\" to \"%s\" has invalid grantor OID %s",
+ role, member, grantorid);
+ pg_log_warning_detail("This grant will be dumped without GRANTED BY.");
+ dump_grantor = false;
+ }
+ else
+ grantor = PQgetvalue(res, i, i_grantor);
}
- member = PQgetvalue(res, i, i_member);
- grantor = PQgetvalue(res, i, i_grantor);
- grantorid = PQgetvalue(res, i, i_grantorid);
admin_option = PQgetvalue(res, i, i_admin_option);
if (dump_grant_options)
set_option = PQgetvalue(res, i, i_set_option);
/*
- * If we're not dumping grantors or if the grantor is the
+ * If we're not dumping the grantor or if the grantor is the
* bootstrap superuser, it's fine to dump this now. Otherwise,
* it's got to be someone who has already been granted ADMIN
* OPTION.
*/
- if (dump_grantors &&
+ if (dump_grantor &&
atooid(grantorid) != BOOTSTRAP_SUPERUSERID &&
rolename_lookup(ht, grantor) == NULL)
continue;
@@ -1486,7 +1497,7 @@ dumpRoleMembership(PGconn *conn)
}
if (optbuf->data[0] != '\0')
appendPQExpBuffer(querybuf, " WITH %s", optbuf->data);
- if (dump_grantors)
+ if (dump_grantor)
appendPQExpBuffer(querybuf, " GRANTED BY %s", fmtId(grantor));
appendPQExpBuffer(querybuf, ";\n");
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:30 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:47 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:58 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:22 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:28 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
@ 2026-02-27 21:42 ` Robert Haas <[email protected]>
2026-02-27 21:52 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Robert Haas @ 2026-02-27 21:42 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
On Fri, Feb 27, 2026 at 4:28 PM Tom Lane <[email protected]> wrote:
> I wrote:
> > So I end with the attached draft patch.
>
> Sigh, this time with it really attached.
I suggest that having both a variable called dump_grantor and one
called dump_grantors is a little bit subtle, but other than that this
looks good on a quick read-through.
Regarding this point:
> I'm inclined to
> think that is an overreaction to the possible unreliability of the
> data (and from your comment upthread you might agree).
I think this is my code, so I certainly believed I had the right idea
at the time, but we could revisit that. One thing to keep in mind is
that in v15-, regardless of the notional grantor, in effect all grants
are independent of the existence of any other user. In v16+, they form
a tree structure, with grants depending on their grantors. So, when
upgrading from v15- to v16+, we have to end up with a valid tree
structure, but there's absolutely no reason to think that we already
have one. If we don't, it must be better to discard the grantor than
to fail the dump-and-restore altogether. Note that it's perfectly
possible not to have a tree structure, e.g. because we have circular
grants, even if all the roles involved still exist and have existed
continuously. I believe my thought process at the time was there
wasn't really any reason to imagine that whatever we were upgrading
from v15- was intended as a tree structure, and therefore it made
logical sense to treat it as though all of those grants directly
emanated from the superuser, i.e. the tree is just 1 level deep. Now,
that does mean losing the grantor information, but we're arguably
preserving the semantics, since any it reproduces the v15- state where
any of those roles are free to be dropped (assuming no other
blockers). Anyway, again, we can rethink that, but if we do it without
repairing any structural "tree defects," we'll end up with a dump that
we can't reload on v16+.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:30 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:47 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:58 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:22 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:28 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:42 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
@ 2026-02-27 21:52 ` Tom Lane <[email protected]>
2026-03-02 14:19 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2026-02-27 21:52 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
Robert Haas <[email protected]> writes:
> On Fri, Feb 27, 2026 at 4:28 PM Tom Lane <[email protected]> wrote:
>> Sigh, this time with it really attached.
> I suggest that having both a variable called dump_grantor and one
> called dump_grantors is a little bit subtle, but other than that this
> looks good on a quick read-through.
Fair ... do you have a suggestion for less confusing names?
I considered naming the new variable "dump_this_grantor", but thought
it was longer without being more helpful ... but maybe you disagree.
> Regarding this point:
>> I'm inclined to
>> think that is an overreaction to the possible unreliability of the
>> data (and from your comment upthread you might agree).
> I think this is my code, so I certainly believed I had the right idea
> at the time, but we could revisit that. One thing to keep in mind is
> that in v15-, regardless of the notional grantor, in effect all grants
> are independent of the existence of any other user. In v16+, they form
> a tree structure, with grants depending on their grantors. So, when
> upgrading from v15- to v16+, we have to end up with a valid tree
> structure, but there's absolutely no reason to think that we already
> have one.
Yeah, that is certainly a hazard we'd have to worry about. As I said,
I'm content to leave it as-is for now.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:30 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:47 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:58 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:22 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:28 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:42 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-27 21:52 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
@ 2026-03-02 14:19 ` Robert Haas <[email protected]>
2026-03-02 16:17 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Robert Haas @ 2026-03-02 14:19 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
On Fri, Feb 27, 2026 at 4:52 PM Tom Lane <[email protected]> wrote:
> > I suggest that having both a variable called dump_grantor and one
> > called dump_grantors is a little bit subtle, but other than that this
> > looks good on a quick read-through.
>
> Fair ... do you have a suggestion for less confusing names?
> I considered naming the new variable "dump_this_grantor", but thought
> it was longer without being more helpful ... but maybe you disagree.
Personally, I'd find the extra verbosity helpful, but I don't care
that much if you see it otherwise.
> > I think this is my code, so I certainly believed I had the right idea
> > at the time, but we could revisit that. One thing to keep in mind is
> > that in v15-, regardless of the notional grantor, in effect all grants
> > are independent of the existence of any other user. In v16+, they form
> > a tree structure, with grants depending on their grantors. So, when
> > upgrading from v15- to v16+, we have to end up with a valid tree
> > structure, but there's absolutely no reason to think that we already
> > have one.
>
> Yeah, that is certainly a hazard we'd have to worry about. As I said,
> I'm content to leave it as-is for now.
Yeah, sure. I was just mentioning it in case you were planning to
pursue that in a separate thread.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:30 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:47 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:58 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:22 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:28 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:42 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-27 21:52 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-03-02 14:19 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
@ 2026-03-02 16:17 ` Tom Lane <[email protected]>
2026-03-04 14:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
0 siblings, 1 reply; 13+ messages in thread
From: Tom Lane @ 2026-03-02 16:17 UTC (permalink / raw)
To: Robert Haas <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
Robert Haas <[email protected]> writes:
> On Fri, Feb 27, 2026 at 4:52 PM Tom Lane <[email protected]> wrote:
>> Fair ... do you have a suggestion for less confusing names?
>> I considered naming the new variable "dump_this_grantor", but thought
>> it was longer without being more helpful ... but maybe you disagree.
> Personally, I'd find the extra verbosity helpful, but I don't care
> that much if you see it otherwise.
If you think that's better, I'm happy to make it so. An author is
seldom the best judge of readability of his code.
Pushed like that.
regards, tom lane
^ permalink raw reply [nested|flat] 13+ messages in thread
* Re: Major Version Upgrade failure due to orphan roles entries in catalog
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:02 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:30 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-25 18:47 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 18:58 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:22 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:28 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-02-27 21:42 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-27 21:52 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
2026-03-02 14:19 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-03-02 16:17 ` Re: Major Version Upgrade failure due to orphan roles entries in catalog Tom Lane <[email protected]>
@ 2026-03-04 14:39 ` Robert Haas <[email protected]>
0 siblings, 0 replies; 13+ messages in thread
From: Robert Haas @ 2026-03-04 14:39 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; Virender Singla <[email protected]>; [email protected]; Aniket Jha <[email protected]>
On Mon, Mar 2, 2026 at 11:17 AM Tom Lane <[email protected]> wrote:
> Pushed like that.
Thanks!
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 13+ messages in thread
end of thread, other threads:[~2026-03-04 14:39 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2026-02-25 17:33 Re: Major Version Upgrade failure due to orphan roles entries in catalog Robert Haas <[email protected]>
2026-02-25 17:39 ` Tom Lane <[email protected]>
2026-02-25 18:02 ` Robert Haas <[email protected]>
2026-02-25 18:30 ` Tom Lane <[email protected]>
2026-02-25 18:47 ` Robert Haas <[email protected]>
2026-02-25 18:58 ` Tom Lane <[email protected]>
2026-02-27 21:22 ` Tom Lane <[email protected]>
2026-02-27 21:28 ` Tom Lane <[email protected]>
2026-02-27 21:42 ` Robert Haas <[email protected]>
2026-02-27 21:52 ` Tom Lane <[email protected]>
2026-03-02 14:19 ` Robert Haas <[email protected]>
2026-03-02 16:17 ` Tom Lane <[email protected]>
2026-03-04 14:39 ` Robert Haas <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox