DROP OWNED BY is broken on master branch.
Hi All,
Consider the below test:
postgres@53130=#create role test WITH login createdb;
CREATE ROLE
postgres@53130=#\c - test
You are now connected to database "postgres" as user "test".
postgres@53150=#create database test;
CREATE DATABASE
postgres@53150=#\c - rushabh
You are now connected to database "postgres" as user "rushabh".
postgres@53162=#
postgres@53162=#
-- This was working before the below mentioned commit.
postgres@53162=#drop owned by test;
ERROR: global objects cannot be deleted by doDeletion
Commit 6566133c5f52771198aca07ed18f84519fac1be7 ensure that
pg_auth_members.grantor is always valid. This commit did changes
into shdepDropOwned() function and combined the SHARED_DEPENDENCY_ACL
and SHARED_DEPENDENCY_OWNER. In that process it removed condition for
local object in owner dependency.
case SHARED_DEPENDENCY_OWNER:
- /* If a local object, save it for deletion below */
- if (sdepForm->dbid == MyDatabaseId)
+ /* Save it for deletion below */Case ending up with above error because of the above removed condition.
Please find the attached patch which fixes the case.
Thanks,
Rushabh Lathia
www.EnterpriseDB.com
Attachments:
fix_shdepDropOwned.patchtext/x-patch; charset=US-ASCII; name=fix_shdepDropOwned.patchDownload+18-2
On Mon, Sep 26, 2022 at 01:13:53PM +0530, Rushabh Lathia wrote:
Please find the attached patch which fixes the case.
Could it be possible to stress this stuff in the regression tests?
There is a gap here. (I have not looked at what you are proposing.)
--
Michael
On Mon, Sep 26, 2022 at 3:44 AM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
Commit 6566133c5f52771198aca07ed18f84519fac1be7 ensure that
pg_auth_members.grantor is always valid. This commit did changes
into shdepDropOwned() function and combined the SHARED_DEPENDENCY_ACL
and SHARED_DEPENDENCY_OWNER. In that process it removed condition for
local object in owner dependency.case SHARED_DEPENDENCY_OWNER: - /* If a local object, save it for deletion below */ - if (sdepForm->dbid == MyDatabaseId) + /* Save it for deletion below */Case ending up with above error because of the above removed condition.
Please find the attached patch which fixes the case.
Thanks for the report. I think it would be preferable not to duplicate
the logic as your version does, though, so here's a slightly different
version that avoids that.
Per Michael's suggestion, I have also written a test case and included
it in this version.
Comments?
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
0001-Fix-bug-in-DROP-OWNED-BY.patchapplication/octet-stream; name=0001-Fix-bug-in-DROP-OWNED-BY.patchDownload+38-11
On Mon, Sep 26, 2022 at 11:46 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Sep 26, 2022 at 3:44 AM Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:Commit 6566133c5f52771198aca07ed18f84519fac1be7 ensure that
pg_auth_members.grantor is always valid. This commit did changes
into shdepDropOwned() function and combined the SHARED_DEPENDENCY_ACL
and SHARED_DEPENDENCY_OWNER. In that process it removed condition for
local object in owner dependency.case SHARED_DEPENDENCY_OWNER: - /* If a local object, save it for deletion below */ - if (sdepForm->dbid == MyDatabaseId) + /* Save it for deletion below */Case ending up with above error because of the above removed condition.
Please find the attached patch which fixes the case.
Thanks for the report. I think it would be preferable not to duplicate
the logic as your version does, though, so here's a slightly different
version that avoids that.
Yes, I was also thinking to avoid the duplicate logic but couldn't found
a way. I did the quick testing with the patch, and reported test is working
fine. But "make check" is failing with few failures.
Per Michael's suggestion, I have also written a test case and included
it in this version.
Thanks for this.
Comments?
--
Robert Haas
EDB: http://www.enterprisedb.com
--
Rushabh Lathia
On Tue, Sep 27, 2022 at 2:53 AM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
Yes, I was also thinking to avoid the duplicate logic but couldn't found
a way. I did the quick testing with the patch, and reported test is working
fine. But "make check" is failing with few failures.
Oh, woops. There was a dumb mistake in that version -- it was testing
sdepForm->dbid == SHARED_DEPENDENCY_OWNER, which is nonsense, instead
of sdepForm->dbid == MyDatabaseId. Here's a fixed version.
--
Robert Haas
EDB: http://www.enterprisedb.com
Attachments:
v2-0001-Fix-bug-in-DROP-OWNED-BY.patchapplication/octet-stream; name=v2-0001-Fix-bug-in-DROP-OWNED-BY.patchDownload+37-11
On Tue, Sep 27, 2022 at 7:34 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Sep 27, 2022 at 2:53 AM Rushabh Lathia <rushabh.lathia@gmail.com>
wrote:Yes, I was also thinking to avoid the duplicate logic but couldn't found
a way. I did the quick testing with the patch, and reported test isworking
fine. But "make check" is failing with few failures.
Oh, woops. There was a dumb mistake in that version -- it was testing
sdepForm->dbid == SHARED_DEPENDENCY_OWNER, which is nonsense, instead
of sdepForm->dbid == MyDatabaseId. Here's a fixed version.
This seems to fix the issue and in further testing I didn't find anything
else.
Thanks,
--
Robert Haas
EDB: http://www.enterprisedb.com
--
Rushabh Lathia
On Wed, Sep 28, 2022 at 8:21 AM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
On Tue, Sep 27, 2022 at 7:34 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Sep 27, 2022 at 2:53 AM Rushabh Lathia <rushabh.lathia@gmail.com> wrote:
Yes, I was also thinking to avoid the duplicate logic but couldn't found
a way. I did the quick testing with the patch, and reported test is working
fine. But "make check" is failing with few failures.Oh, woops. There was a dumb mistake in that version -- it was testing
sdepForm->dbid == SHARED_DEPENDENCY_OWNER, which is nonsense, instead
of sdepForm->dbid == MyDatabaseId. Here's a fixed version.This seems to fix the issue and in further testing I didn't find anything else.
OK, committed.
--
Robert Haas
EDB: http://www.enterprisedb.com