DROP OWNED BY is broken on master branch.

Started by Rushabh Lathiaover 3 years ago7 messages

rushabh.lathia@gmail.com

over 3 years ago

1 attachment(s)

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

diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index f2f227f..6134fe3 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -1411,8 +1411,6 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
 												sdepForm->objid);
 						break;
 					}
-					/* FALLTHROUGH */
-				case SHARED_DEPENDENCY_OWNER:
 					/* Save it for deletion below */
 					obj.classId = sdepForm->classid;
 					obj.objectId = sdepForm->objid;
@@ -1426,6 +1424,24 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
 					}
 					add_exact_object_address(&obj, deleteobjs);
 					break;
+				case SHARED_DEPENDENCY_OWNER:
+					/* If a local object, save it for deletion below */
+					if (sdepForm->dbid == MyDatabaseId)
+					{
+						/* Save it for deletion below */
+						obj.classId = sdepForm->classid;
+						obj.objectId = sdepForm->objid;
+						obj.objectSubId = sdepForm->objsubid;
+						/* as above */
+						AcquireDeletionLock(&obj, 0);
+						if (!systable_recheck_tuple(scan, tuple))
+						{
+							ReleaseDeletionLock(&obj);
+							break;
+						}
+						add_exact_object_address(&obj, deleteobjs);
+					}
+					break;
 			}
 		}

Michael Paquier

michael@paquier.xyz

over 3 years ago

In reply to: Rushabh Lathia (#1)

Re: DROP OWNED BY is broken on master branch.

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

Robert Haas

robertmhaas@gmail.com

over 3 years ago

In reply to: Rushabh Lathia (#1)

1 attachment(s)

Re: DROP OWNED BY is broken on master branch.

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

From 07af327b0f646cf946dde6ea7f7fcaed31613426 Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Mon, 26 Sep 2022 14:14:54 -0400
Subject: [PATCH] Fix bug in DROP OWNED BY.

Commit 6566133c5f52771198aca07ed18f84519fac1be7 broke the case where
the role passed to DROP OWNED BY owns a database.

Report by Rushabh Lathia, who also provided a patch, but this patch
takes a slightly different approach to fixing the problem.

Discussion: http://postgr.es/m/CAGPqQf2vO+nbo=3yAdZ8v26Rbug7bY4YjPaPLZx=L1NZ9-CC3w@mail.gmail.com
---
 src/backend/catalog/pg_shdepend.c | 30 ++++++++++++++++++++----------
 src/bin/scripts/t/020_createdb.pl | 18 ++++++++++++++++++
 2 files changed, 38 insertions(+), 10 deletions(-)

diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index f2f227f887..e359b2bd14 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -1412,19 +1412,29 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
 						break;
 					}
 					/* FALLTHROUGH */
+
 				case SHARED_DEPENDENCY_OWNER:
-					/* Save it for deletion below */
-					obj.classId = sdepForm->classid;
-					obj.objectId = sdepForm->objid;
-					obj.objectSubId = sdepForm->objsubid;
-					/* as above */
-					AcquireDeletionLock(&obj, 0);
-					if (!systable_recheck_tuple(scan, tuple))
+					/*
+					 * Save it for deletion below, if it's a local object or a
+					 * role grant. Other shared objects, such as databases,
+					 * should not be removed here.
+					 */
+					if (sdepForm->dbid == SHARED_DEPENDENCY_OWNER ||
+						sdepForm->classid == AuthMemRelationId)
 					{
-						ReleaseDeletionLock(&obj);
-						break;
+						/* Save it for deletion below */
+						obj.classId = sdepForm->classid;
+						obj.objectId = sdepForm->objid;
+						obj.objectSubId = sdepForm->objsubid;
+						/* as above */
+						AcquireDeletionLock(&obj, 0);
+						if (!systable_recheck_tuple(scan, tuple))
+						{
+							ReleaseDeletionLock(&obj);
+							break;
+						}
+						add_exact_object_address(&obj, deleteobjs);
 					}
-					add_exact_object_address(&obj, deleteobjs);
 					break;
 			}
 		}
diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
index 2e712f4fe9..a74bf3b0d8 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -158,4 +158,22 @@ $node->issues_sql_like(
 	qr/statement: CREATE DATABASE foobar7 STRATEGY file_copy TEMPLATE foobar2/,
 	'create database with FILE_COPY strategy');
 
+# Create database owned by role_foobar.
+$node->issues_sql_like(
+	[ 'createdb', '-T', 'foobar2', '-O', 'role_foobar', 'foobar8' ],
+	qr/statement: CREATE DATABASE foobar8 OWNER role_foobar TEMPLATE foobar2/,
+	'create database with owner role_foobar');
+($ret, $stdout, $stderr) = $node->psql(
+	'foobar2',
+	'DROP OWNED BY role_foobar;',
+	on_error_die => 1,
+);
+ok($ret == 0, "DROP OWNED BY role_foobar");
+($ret, $stdout, $stderr) = $node->psql(
+	'foobar2',
+	'DROP DATABASE foobar8;',
+	on_error_die => 1,
+);
+ok($ret == 0, "DROP DATABASE foobar8");
+
 done_testing();
-- 
2.24.3 (Apple Git-128)

Rushabh Lathia

rushabh.lathia@gmail.com

over 3 years ago

In reply to: Robert Haas (#3)

Re: DROP OWNED BY is broken on master branch.

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

Robert Haas

robertmhaas@gmail.com

over 3 years ago

In reply to: Rushabh Lathia (#4)

1 attachment(s)

Re: DROP OWNED BY is broken on master branch.

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

From 5a5111b7dbf92176678ee229cdb072ad22cad50c Mon Sep 17 00:00:00 2001
From: Robert Haas <rhaas@postgresql.org>
Date: Tue, 27 Sep 2022 09:57:54 -0400
Subject: [PATCH v2] Fix bug in DROP OWNED BY.

Commit 6566133c5f52771198aca07ed18f84519fac1be7 broke the case where
the role passed to DROP OWNED BY owns a database.

Report by Rushabh Lathia, who also provided a patch, but this patch
takes a slightly different approach to fixing the problem.

Discussion: http://postgr.es/m/CAGPqQf2vO+nbo=3yAdZ8v26Rbug7bY4YjPaPLZx=L1NZ9-CC3w@mail.gmail.com
---
 src/backend/catalog/pg_shdepend.c | 29 +++++++++++++++++++----------
 src/bin/scripts/t/020_createdb.pl | 18 ++++++++++++++++++
 2 files changed, 37 insertions(+), 10 deletions(-)

diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c
index f2f227f887..bc26bf1ef5 100644
--- a/src/backend/catalog/pg_shdepend.c
+++ b/src/backend/catalog/pg_shdepend.c
@@ -1412,19 +1412,28 @@ shdepDropOwned(List *roleids, DropBehavior behavior)
 						break;
 					}
 					/* FALLTHROUGH */
+
 				case SHARED_DEPENDENCY_OWNER:
-					/* Save it for deletion below */
-					obj.classId = sdepForm->classid;
-					obj.objectId = sdepForm->objid;
-					obj.objectSubId = sdepForm->objsubid;
-					/* as above */
-					AcquireDeletionLock(&obj, 0);
-					if (!systable_recheck_tuple(scan, tuple))
+					/*
+					 * Save it for deletion below, if it's a local object or a
+					 * role grant. Other shared objects, such as databases,
+					 * should not be removed here.
+					 */
+					if (sdepForm->dbid == MyDatabaseId ||
+						sdepForm->classid == AuthMemRelationId)
 					{
-						ReleaseDeletionLock(&obj);
-						break;
+						obj.classId = sdepForm->classid;
+						obj.objectId = sdepForm->objid;
+						obj.objectSubId = sdepForm->objsubid;
+						/* as above */
+						AcquireDeletionLock(&obj, 0);
+						if (!systable_recheck_tuple(scan, tuple))
+						{
+							ReleaseDeletionLock(&obj);
+							break;
+						}
+						add_exact_object_address(&obj, deleteobjs);
 					}
-					add_exact_object_address(&obj, deleteobjs);
 					break;
 			}
 		}
diff --git a/src/bin/scripts/t/020_createdb.pl b/src/bin/scripts/t/020_createdb.pl
index 2e712f4fe9..a74bf3b0d8 100644
--- a/src/bin/scripts/t/020_createdb.pl
+++ b/src/bin/scripts/t/020_createdb.pl
@@ -158,4 +158,22 @@ $node->issues_sql_like(
 	qr/statement: CREATE DATABASE foobar7 STRATEGY file_copy TEMPLATE foobar2/,
 	'create database with FILE_COPY strategy');
 
+# Create database owned by role_foobar.
+$node->issues_sql_like(
+	[ 'createdb', '-T', 'foobar2', '-O', 'role_foobar', 'foobar8' ],
+	qr/statement: CREATE DATABASE foobar8 OWNER role_foobar TEMPLATE foobar2/,
+	'create database with owner role_foobar');
+($ret, $stdout, $stderr) = $node->psql(
+	'foobar2',
+	'DROP OWNED BY role_foobar;',
+	on_error_die => 1,
+);
+ok($ret == 0, "DROP OWNED BY role_foobar");
+($ret, $stdout, $stderr) = $node->psql(
+	'foobar2',
+	'DROP DATABASE foobar8;',
+	on_error_die => 1,
+);
+ok($ret == 0, "DROP DATABASE foobar8");
+
 done_testing();
-- 
2.24.3 (Apple Git-128)

Rushabh Lathia

rushabh.lathia@gmail.com

over 3 years ago

In reply to: Robert Haas (#5)

Re: DROP OWNED BY is broken on master branch.

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

Robert Haas

robertmhaas@gmail.com

over 3 years ago

In reply to: Rushabh Lathia (#6)

Re: DROP OWNED BY is broken on master branch.

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