Hello!
IIUC the regression test test_pg_dump [1] fails, see the attached
regression.diffs:
diff -U3
/Users/test/Work/postgrespro/src/test/modules/test_pg_dump/expected/test_pg_dump.out
/Users/test/Work/postgrespro/build/testrun/test_pg_dump-running/regress/results/test_pg_dump.out
---
/User
=?UTF-8?B?0JXQs9C+0YAg0KfQuNC90LTRj9GB0LrQuNC9?= writes:
> This query does not expect that test database may already contain some
> information about custom user that ran test_pg_dump-running.
I'm perfectly content to reject this as being an abuse of the test
case. Our TAP tests are built on th
>On Thu, Jun 20, 2024 at 3:43PM Hannu Krosing < han...@google.com > wrote:
>> Still it would be nice to have some public support for users of
>> non-managed PostgreSQL databases as well
>+1.
>
>--
>Robert Haas
>EDB: http://www.enterprisedb.com
Hello! I have recently been researching postgres bu
On Thu, Jun 20, 2024 at 3:43 PM Hannu Krosing wrote:
> Still it would be nice to have some public support for users of
> non-managed PostgreSQL databases as well
+1.
--
Robert Haas
EDB: http://www.enterprisedb.com
It does happen with some regularity.
At least one large cloud database provider I know of saw this more
than once a month until the mitigations were integrated in the major
version upgrade process.
It is possible that making database upgrades easier via better
automation is what made this turn up
Hannu Krosing writes:
> Or perhaps we should still also patch pg_dump to ignore the aclentries
> which refer to roles that do not exist in the database ?
I didn't want to do that before, and I still don't. Given that this
issue has existed since pg_init_privs was invented (9.6) without
prior rep
On Thu, Jun 20, 2024 at 2:25 PM Tom Lane wrote:
> Hannu Krosing writes:
> > Or perhaps we should still also patch pg_dump to ignore the aclentries
> > which refer to roles that do not exist in the database ?
>
> I didn't want to do that before, and I still don't. Given that this
> issue has exis
Or perhaps we should still also patch pg_dump to ignore the aclentries
which refer to roles that do not exist in the database ?
On Thu, Jun 20, 2024 at 7:41 PM Hannu Krosing wrote:
>
> Then maybe we should put a query / function in the release notes to
> clean up the existing mess.
>
> Thinking o
Then maybe we should put a query / function in the release notes to
clean up the existing mess.
Thinking of it we should do it anyway, as the patch only prevents new
messiness from happening and does not fix existing issues.
I could share a query to update the pg_init_privs with non-existent
role
Hannu Krosing writes:
> Is there anything that could be back-patched with reasonable effort ?
Afraid not. The whole thing is dependent on pg_shdepend entries
that won't exist in older branches.
regards, tom lane
Hi Tom,
Is there anything that could be back-patched with reasonable effort ?
--
Hannu
On Mon, Jun 17, 2024 at 6:37 PM Daniel Gustafsson wrote:
>
> > On 17 Jun 2024, at 16:56, Tom Lane wrote:
> > Daniel Gustafsson writes:
>
> >> I wonder if this will break any tools/scripts in prod which reli
> On 17 Jun 2024, at 16:56, Tom Lane wrote:
> Daniel Gustafsson writes:
>> I wonder if this will break any tools/scripts in prod which relies on the
>> previous (faulty) behaviour. It will be interesting to see if anything shows
>> up on -bugs. Off the cuff it seems like a good idea judging by
Daniel Gustafsson writes:
> On 15 Jun 2024, at 01:46, Tom Lane wrote:
>> The core change is to install SHARED_DEPENDENCY_INITACL entries in
>> pg_shdepend for all references to non-pinned roles in pg_init_privs,
>> whether they are the object's owner or not. Doing that ensures that
>> we can't d
> On 15 Jun 2024, at 01:46, Tom Lane wrote:
>
> Robert Haas writes:
>> I think the only thing we absolutely have to fix here is the dangling
>> ACL references.
>
> Here's a draft patch that takes care of Hannu's example, and I think
> it fixes other potential dangling-reference scenarios too.
>
I wrote:
> The semantics I've implemented on top of that are:
> * ALTER OWNER does not touch pg_init_privs entries.
> * REASSIGN OWNED replaces pg_init_privs references with the new
> role, whether the references are as grantor or grantee.
> * DROP OWNED removes pg_init_privs mentions of the doomed
Robert Haas writes:
> I think the only thing we absolutely have to fix here is the dangling
> ACL references.
Here's a draft patch that takes care of Hannu's example, and I think
it fixes other potential dangling-reference scenarios too.
I'm not sure whether I like the direction this is going, b
On Fri, May 24, 2024 at 4:00 PM Tom Lane wrote:
> > Oh! That does seem like it would make what I said wrong, but how would
> > it even know who the original owner was? Shouldn't we be recreating
> > the object with the owner it had at dump time?
>
> Keep in mind that the whole point here is for th
On Tue, May 28, 2024 at 9:06 PM Hannu Krosing wrote:
> We should definitely also fix pg_dump, likely just checking that the
> role exists when generating REVOKE commands (may be a good practice
> for other cases too so instead of casting to ::regrole do the actual
> join)
>
> ## here is the fix f
Hi Daniel,
pg_upgrade is just one important user of pg_dump which is the one that
generates REVOKE for a non-existent role.
We should definitely also fix pg_dump, likely just checking that the
role exists when generating REVOKE commands (may be a good practice
for other cases too so instead of ca
> On 26 May 2024, at 23:25, Tom Lane wrote:
>
> Hannu Krosing writes:
>> Attached is a minimal patch to allow missing roles in REVOKE command
>
> FTR, I think this is a very bad idea.
Agreed, this is papering over a bug. If we are worried about pg_upgrade it
would be better to add a check to
Hannu Krosing writes:
> Attached is a minimal patch to allow missing roles in REVOKE command
FTR, I think this is a very bad idea.
It might be OK if we added some kind of IF EXISTS option,
but I'm not eager about that concept either.
The right thing here is to fix the backend so that pg_dump do
Attached is a minimal patch to allow missing roles in REVOKE command
This should fix the pg_upgrade issue and also a case where somebody
has dropped a role you are trying to revoke privileges from :
smalltest=# create table revoketest();
CREATE TABLE
smalltest=# revoke select on revoketest from b
On Sat, May 25, 2024 at 4:48 PM Tom Lane wrote:
>
> Hannu Krosing writes:
> > Having an pg_init_privs entry referencing a non-existing user is
> > certainly of no practical use.
>
> Sure, that's not up for debate. What I think we're discussing
> right now is
>
> 1. What other cases are badly han
Hannu Krosing writes:
> Having an pg_init_privs entry referencing a non-existing user is
> certainly of no practical use.
Sure, that's not up for debate. What I think we're discussing
right now is
1. What other cases are badly handled by the pg_init_privs
mechanisms.
2. How much of that is pra
On Fri, May 24, 2024 at 10:00 PM Tom Lane wrote:
>
> Robert Haas writes:
> > On Fri, May 24, 2024 at 2:57 PM Tom Lane wrote:
> >> Doesn't seem right to me. That will give pg_dump the wrong idea
> >> of what the initial privileges actually were, and I don't see how
> >> it can construct correct
Robert Haas writes:
> On Fri, May 24, 2024 at 2:57 PM Tom Lane wrote:
>> Doesn't seem right to me. That will give pg_dump the wrong idea
>> of what the initial privileges actually were, and I don't see how
>> it can construct correct delta GRANT/REVOKE on the basis of false
>> information. Duri
On Fri, May 24, 2024 at 2:57 PM Tom Lane wrote:
> Doesn't seem right to me. That will give pg_dump the wrong idea
> of what the initial privileges actually were, and I don't see how
> it can construct correct delta GRANT/REVOKE on the basis of false
> information. During the dump reload, the ext
Robert Haas writes:
> On Fri, May 24, 2024 at 11:59 AM Tom Lane wrote:
>> So this goal seems to
>> mean that neither ALTER OWNER nor REASSIGN OWNED should touch
>> pg_init_privs at all, as that would break its function of recording
>> a historical state. Only DROP OWNED should get rid of pg_init
On Fri, May 24, 2024 at 11:59 AM Tom Lane wrote:
> Thinking about this some more: the point of pg_init_privs is to record
> an object's privileges as they stood at the end of CREATE EXTENSION
> (or extension update), with the goal that pg_dump should be able to
> compute the delta between that and
Daniel Gustafsson writes:
> On 24 May 2024, at 16:20, Tom Lane wrote:
>> Another point: shdepReassignOwned explicitly does not touch grants
>> or default ACLs. It feels like the same should be true of
>> pg_init_privs entries,
> Agreed, I can't see why pg_init_privs should be treated differentl
> On 24 May 2024, at 16:20, Tom Lane wrote:
> I've tentatively concluded that I shouldn't have modeled
> SHARED_DEPENDENCY_INITACL so closely on SHARED_DEPENDENCY_ACL,
> in particular the decision that we don't need such an entry if
> there's also SHARED_DEPENDENCY_OWNER.
+1, in light of this
Daniel Gustafsson writes:
> I had a look, but I didn't beat you to a fix since it's not immediately clear
> to me how this should work for REASSING OWNED (DROP OWNED seems a simpler
> case). Should REASSIGN OWNED alter the rows in pg_shdepend matching init
> privs
> from SHARED_DEPENDENCY_OWNER
> On 24 May 2024, at 01:01, Tom Lane wrote:
>
> Hannu Krosing writes:
>> While the 'DROP OWNED BY fails to clean out pg_init_privs grants'
>> issue is now fixed,we have a similar issue with REASSIGN OWNED BY that
>> is still there:
>
> Ugh, how embarrassing. I'll take a look tomorrow, if no on
Hannu Krosing writes:
> While the 'DROP OWNED BY fails to clean out pg_init_privs grants'
> issue is now fixed,we have a similar issue with REASSIGN OWNED BY that
> is still there:
Ugh, how embarrassing. I'll take a look tomorrow, if no one
beats me to it.
regards, tom l
While the 'DROP OWNED BY fails to clean out pg_init_privs grants'
issue is now fixed,we have a similar issue with REASSIGN OWNED BY that
is still there:
Tested on fresh git checkout om May 20th
test=# create user privtestuser superuser;
CREATE ROLE
test=# set role privtestuser;
SET
test=# create
On Monday, April 29, 2024, Tom Lane wrote:
> "David G. Johnston" writes:
> > My solution to this was to rely on the fact that the bootstrap superuser
> is
> > assigned OID 10 regardless of its name.
>
> Yeah, I wrote it that way to start with too, but reconsidered
> because
>
> (1) I don't like
"David G. Johnston" writes:
> My solution to this was to rely on the fact that the bootstrap superuser is
> assigned OID 10 regardless of its name.
Yeah, I wrote it that way to start with too, but reconsidered
because
(1) I don't like hard-coding numeric OIDs. We can avoid that in C
code but it
On Monday, April 29, 2024, Tom Lane wrote:
> Daniel Gustafsson writes:
> >> On 28 Apr 2024, at 20:52, Tom Lane wrote:
>
>
> >> This is of course not bulletproof: with a sufficiently weird
> >> bootstrap superuser name, we could get false matches to parts
> >> of "regress_dump_test_role" or to p
I wrote:
> Pushed, thanks for reviewing!
Argh, I forgot I'd meant to push b0c5b215d first not second.
Oh well, it was only neatnik-ism that made me want to see
those other animals fail --- and a lot of the buildfarm is
red right now for $other_reasons anyway.
regards, tom
Daniel Gustafsson writes:
> On 29 Apr 2024, at 21:15, Tom Lane wrote:
>> v3 attached also has a bit more work on code comments.
> LGTM.
Pushed, thanks for reviewing!
regards, tom lane
> On 29 Apr 2024, at 21:15, Tom Lane wrote:
> It occurred to me to use "aclexplode" to expand the initprivs, and
> then we can substitute names with simple equality tests. The test
> query is a bit more complicated, but I feel better about it.
Nice, I didn't even remember that function existed.
Daniel Gustafsson writes:
>> On 28 Apr 2024, at 20:52, Tom Lane wrote:
>> ... It's a little bit
>> nasty to look at the ACL column of pg_init_privs, because that text
>> involves the bootstrap superuser's name which is site-dependent.
>> What I did to try to make the test stable is
>> replace(ini
> On 28 Apr 2024, at 20:52, Tom Lane wrote:
>
> I wrote:
>> Here's a draft patch that attacks that. It seems to fix the
>> problem with test_pg_dump: no dangling pg_init_privs grants
>> are left behind.
Reading this I can't find any sharp edges, and I prefer your changes to
recordExtensionInitP
I wrote:
> Here's a draft patch that attacks that. It seems to fix the
> problem with test_pg_dump: no dangling pg_init_privs grants
> are left behind.
Here's a v2 that attempts to add some queries to test_pg_dump.sql
to provide visual verification that pg_shdepend and pg_init_privs
are updated c
I wrote:
> A bigger problem though is that I think you are addressing the
> original complaint from the older thread, which while it's a fine
> thing to fix seems orthogonal to the failure we're seeing in the
> buildfarm. The buildfarm's problem is not that we're recording
> incorrect pg_init_priv
Daniel Gustafsson writes:
> On 21 Apr 2024, at 23:08, Tom Lane wrote:
>> So the meson animals are not running the test that sets up the
>> problematic data.
> I took a look at this, reading code and the linked thread. My gut feeling is
> that Stephen is right in that the underlying bug is these
> On 21 Apr 2024, at 23:08, Tom Lane wrote:
>
> Daniel Gustafsson writes:
>>> On 6 Apr 2024, at 01:10, Tom Lane wrote:
>>> (So now I'm wondering why *only* copperhead has shown this so far.
>>> Are our other cross-version-upgrade testing animals AWOL?)
>
>> Clicking around searching for Xversi
> On 21 Apr 2024, at 23:08, Tom Lane wrote:
> ... were you going to look at it? I can take a whack if it's too far down
> your priority list.
Yeah, I’m working on a patchset right now.
./daniel
Daniel Gustafsson writes:
>> On 6 Apr 2024, at 01:10, Tom Lane wrote:
>> (So now I'm wondering why *only* copperhead has shown this so far.
>> Are our other cross-version-upgrade testing animals AWOL?)
> Clicking around searching for Xversion animals I didn't spot any, but without
> access to th
> On 6 Apr 2024, at 01:10, Tom Lane wrote:
> (So now I'm wondering why *only* copperhead has shown this so far.
> Are our other cross-version-upgrade testing animals AWOL?)
Clicking around searching for Xversion animals I didn't spot any, but without
access to the database it's nontrivial to kno
Noah Misch writes:
> On Fri, Apr 05, 2024 at 07:10:59PM -0400, Tom Lane wrote:
>> The fact that the DROP ROLE added by 936e3fa37 succeeded indicates
>> that these role references weren't captured in pg_shdepend.
>> I imagine that we also lack code that would allow DROP OWNED BY to
>> follow up on
On Fri, Apr 05, 2024 at 07:10:59PM -0400, Tom Lane wrote:
> I wondered why buildfarm member copperhead has started to fail
> xversion-upgrade-HEAD-HEAD tests. I soon reproduced the problem here:
>
> pg_restore: creating ACL "regress_pg_dump_schema.TYPE "test_type""
> pg_restore: while PROCESSING
52 matches
Mail list logo
