Fix error when trying to drop columns from temp tables. #410

Sairakan · 2024-07-18T19:09:29Z

Description

Our original code for ENRgetSystableScan did not take into account the fact that pg_depend scans can use up to three keys. Fix the issue by making sure to use the third key if it is supplied.

Also fix the logic around performing the deletion so that we don't try to drop the entire ENR table when deleting columns.

Backport to PG15 as well.

Issues Resolved

BABEL-4912

Check List

Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is under the terms of the PostgreSQL license, and grant any person obtaining a copy of the contribution permission to relicense all or a portion of my contribution to the PostgreSQL License solely to contribute all or a portion of my contribution to the PostgreSQL open source project.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Jason Teng <[email protected]>

…as not being checked. Signed-off-by: Jason Teng <[email protected]>

Signed-off-by: Jason Teng <[email protected]>

timchang514 · 2024-07-18T21:01:28Z

src/backend/catalog/dependency.c

@@ -1392,7 +1392,8 @@ deleteOneObject(const ObjectAddress *object, Relation *depRel, int flags)
 	DeleteInitPrivs(object);

 	// Delete from ENR - noop if not found from ENR
-	ENRDropEntry(object->objectId);
+	if (object->objectSubId == 0)


so then how does an ENR entry with object->objectSubId != 0 get cleaned up?

There aren't any ENR entries for such objects - ENR entries are only created for the base table, which would have a subid of 0. This test case is to fix an erroneous case where we are trying to delete something that doesn't actually have an ENR entry at all.

nit: please add a comment why we explicitly check for objectSubId. I cant seem to find the testcase for this code change

Sorry, I worded my comment awkwardly - the overall fix requires this change in order to work. Without it, we will get an error when trying to ALTER TABLE DROP COLUMN, as the table will erroneously be deleted when we only wanted to drop one column. So in a sense, all of the test cases for the feature in babelfish-for-postgresql/babelfish_extensions#2753 exercise this code path.

This test case is to fix an erroneous case should have been This check is to fix an erroneous case.

I'll add a comment clarifying why we need this check.

timchang514 · 2024-07-18T21:06:57Z

src/backend/utils/misc/queryenvironment.c

 				* So we cannot return right away if there is a match.
 				*/
 				ListCell   *lc;
 				foreach(lc, enr->md.cattups[ENR_CATTUP_DEPEND]) {
 					Form_pg_depend tup = (Form_pg_depend) GETSTRUCT((HeapTuple) lfirst(lc));
 					if (indexId == DependDependerIndexId &&
 						tup->classid == (Oid)v1 &&
-						tup->objid == (Oid)v2)
+						tup->objid == (Oid)v2 &&
+						(nkeys == 2 || tup->objsubid == (int32)v3))


is it safe to just check if tup->objsubid == v3? since v3 if not set is 0, and tup->objsubid will be 0 for non-column lookups

That doesn't work - if the caller of this function only wants 2 keys, then they want all pg_depend entries that match the objid, no matter what the subid is - so we need to completely ignore the subid in that case.

forestkeeper · 2024-07-18T23:32:25Z

src/backend/catalog/dependency.c

@@ -1392,7 +1392,8 @@ deleteOneObject(const ObjectAddress *object, Relation *depRel, int flags)
 	DeleteInitPrivs(object);

 	// Delete from ENR - noop if not found from ENR
-	ENRDropEntry(object->objectId);
+	if (object->objectSubId == 0)


Should use InvalidOid instead of 0 in this case

objectSubId is not an Oid, it is an int32 (see the definition in pg_depend https://www.postgresql.org/docs/current/catalog-pg-depend.html or in the code for FormData_pg_depend). I don't think it would be appropriate to use InvalidOid when this is not an Oid value, this is an int value representing the column number represented in the pg_depend tuple (or 0 in the case that it is not used).

In all other places in the code, this value is explicitly compared to 0, e.g. https://github.com/postgres/postgres/blob/master/src/backend/catalog/dependency.c#L557

forestkeeper · 2024-07-18T23:33:44Z

src/backend/utils/misc/queryenvironment.c

 				* So we cannot return right away if there is a match.
 				*/
 				ListCell   *lc;
 				foreach(lc, enr->md.cattups[ENR_CATTUP_DEPEND]) {
 					Form_pg_depend tup = (Form_pg_depend) GETSTRUCT((HeapTuple) lfirst(lc));
 					if (indexId == DependDependerIndexId &&
 						tup->classid == (Oid)v1 &&
-						tup->objid == (Oid)v2)
+						tup->objid == (Oid)v2 &&
+						(nkeys == 2 || tup->objsubid == (int32)v3))


Why other have (Oid) to cast to Oid, while v3 is cast to (int32) , and v1,v2,v3 is really confusing in naming, should assign a more reasonable naming.

As above, this is cast to int32 in order to match the definition of the field in ForData_pg_depend. We use generic v1, v2, v3 naming as they are used to refer to the keys being passed into the function, which could be for any catalog table, not just pg_depend. Thus, the value they represent can be multiple different types depending on which catalog we are searching.

lejaokri · 2024-07-22T20:00:22Z

src/backend/utils/misc/queryenvironment.c

+						tup->refobjid == (Oid)v2 &&
+						(nkeys == 2 || tup->refobjsubid == (int32)v3))


I see. good catch!

Signed-off-by: Jason Teng <[email protected]>

…les.' (#2753) Tests for babelfish-for-postgresql/postgresql_modified_for_babelfish#410 Task: BABEL-4912 Signed-off-by: Jason Teng <[email protected]>

…r-postgresql#410) Our original code for ENRgetSystableScan did not take into account the fact that pg_depend scans can use up to three keys. Fix the issue by making sure to use the third key if it is supplied. Also fix the logic around performing the deletion so that we don't try to drop the entire ENR table when deleting columns. Backport to PG15 as well. Task: BABEL-4912 Signed-off-by: Jason Teng <[email protected]>

…les.' (babelfish-for-postgresql#2753) Tests for babelfish-for-postgresql/postgresql_modified_for_babelfish#410 Task: BABEL-4912 Signed-off-by: Jason Teng <[email protected]>

Our original code for ENRgetSystableScan did not take into account the fact that pg_depend scans can use up to three keys. Fix the issue by making sure to use the third key if it is supplied. Also fix the logic around performing the deletion so that we don't try to drop the entire ENR table when deleting columns. Backport to PG15 as well. Task: BABEL-4912 Signed-off-by: Jason Teng <[email protected]>

…les.' (#2753) (#2779) Tests for babelfish-for-postgresql/postgresql_modified_for_babelfish#410 Task: BABEL-4912 Signed-off-by: Jason Teng <[email protected]>

…les.' (babelfish-for-postgresql#2753) Tests for babelfish-for-postgresql/postgresql_modified_for_babelfish#410 Task: BABEL-4912 Signed-off-by: Jason Teng <[email protected]>

…r-postgresql#410) (babelfish-for-postgresql#414) Our original code for ENRgetSystableScan did not take into account the fact that pg_depend scans can use up to three keys. Fix the issue by making sure to use the third key if it is supplied. Also fix the logic around performing the deletion so that we don't try to drop the entire ENR table when deleting columns. Backport to PG15 as well. Task: BABEL-4912 Signed-off-by: Jason Teng <[email protected]>

…fish-for-postgresql#410) Our original code for ENRgetSystableScan did not take into account the fact that pg_depend scans can use up to three keys. Fix the issue by making sure to use the third key if it is supplied. Also fix the logic around performing the deletion so that we don't try to drop the entire ENR table when deleting columns. Backport to PG15 as well. Task: BABEL-4912 Signed-off-by: Jason Teng <[email protected]>

…les.' (babelfish-for-postgresql#2753) (babelfish-for-postgresql#2779) Tests for babelfish-for-postgresql/postgresql_modified_for_babelfish#410 Task: BABEL-4912 Signed-off-by: Jason Teng <[email protected]>

Jason Teng added 2 commits July 18, 2024 18:33

Fix error when trying to drop columns from temp tables.

7988bb0

Signed-off-by: Jason Teng <[email protected]>

Fix the source of the incorrect systable scan lookup where objsubid w…

a483195

…as not being checked. Signed-off-by: Jason Teng <[email protected]>

Sairakan mentioned this pull request Jul 18, 2024

Test changes for 'Fix error when trying to drop columns from temp tables.' babelfish-for-postgresql/babelfish_extensions#2753

Merged

1 task

Jason Teng added 2 commits July 18, 2024 19:59

Make pg_depend lookup more specific based on number of supplied keys.

ec4ac30

Signed-off-by: Jason Teng <[email protected]>

Clean up code logic.

5735993

Signed-off-by: Jason Teng <[email protected]>

timchang514 reviewed Jul 18, 2024

View reviewed changes

forestkeeper reviewed Jul 18, 2024

View reviewed changes

lejaokri reviewed Jul 22, 2024

View reviewed changes

Update code comment.

2e7a359

Signed-off-by: Jason Teng <[email protected]>

lejaokri approved these changes Jul 22, 2024

View reviewed changes

2jungkook merged commit 799ea9e into babelfish-for-postgresql:BABEL_4_X_DEV__PG_16_X Jul 23, 2024
2 checks passed

2jungkook deleted the jira-babel-4912 branch July 23, 2024 16:59

Sairakan mentioned this pull request Jul 24, 2024

Test changes for 'Fix error when trying to drop columns from temp tables.' babelfish-for-postgresql/babelfish_extensions#2779

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error when trying to drop columns from temp tables. #410

Fix error when trying to drop columns from temp tables. #410

Sairakan commented Jul 18, 2024 •

edited

Loading

timchang514 Jul 18, 2024

Sairakan Jul 18, 2024

lejaokri Jul 22, 2024 •

edited

Loading

Sairakan Jul 22, 2024

timchang514 Jul 18, 2024

Sairakan Jul 18, 2024

forestkeeper Jul 18, 2024

Sairakan Jul 18, 2024

Sairakan Jul 19, 2024

forestkeeper Jul 18, 2024

Sairakan Jul 18, 2024

lejaokri Jul 22, 2024

		tup->refobjid == (Oid)v2 &&
		(nkeys == 2 \|\| tup->refobjsubid == (int32)v3))

Fix error when trying to drop columns from temp tables. #410

Fix error when trying to drop columns from temp tables. #410

Conversation

Sairakan commented Jul 18, 2024 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lejaokri Jul 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sairakan commented Jul 18, 2024 •

edited

Loading

lejaokri Jul 22, 2024 •

edited

Loading