Thread: 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 */
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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.
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