Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Jul 23, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7988bb0
Fix error when trying to drop columns from temp tables.
Jul 18, 2024
a483195
Fix the source of the incorrect systable scan lookup where objsubid w…
Jul 18, 2024
ec4ac30
Make pg_depend lookup more specific based on number of supplied keys.
Jul 18, 2024
5735993
Clean up code logic.
Jul 18, 2024
2e7a359
Update code comment.
Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/backend/catalog/dependency.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1391,8 +1391,14 @@ deleteOneObject(const ObjectAddress *object, Relation *depRel, int flags)
DeleteSecurityLabel(object);
DeleteInitPrivs(object);

// Delete from ENR - noop if not found from ENR
ENRDropEntry(object->objectId);
/*
* If objectSubId != 0, then this is a column. There are no ENR entries
* for individual columns, so skip ENRDropEntry in this case (or else we
* will delete the entire table instead of just the column). Note that this
* is a no-op if the objectId is not found from ENR.
*/
if (object->objectSubId == 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@lejaokri lejaokri Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use InvalidOid instead of 0 in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ENRDropEntry(object->objectId);

/*
* CommandCounterIncrement here to ensure that preceding changes are all
Expand Down
10 changes: 6 additions & 4 deletions src/backend/utils/misc/queryenvironment.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,25 +435,27 @@ bool ENRgetSystableScan(Relation rel, Oid indexId, int nkeys, ScanKey key, List
* Search through the entire ENR relation list for everything
* that has a relation (non-recursive) to this object.
* If indexId is DependDependerIndexId, we try to mimic
* SELECT * FROM pg_depend WHERE classid=v1 AND objid=v2
* SELECT * FROM pg_depend WHERE classid=v1 AND objid=v2 (AND objsubid = v3 if applicable)
* Otherwise if it is DependReferenceIndexId we try to mimic
* SELECT * FROM pg_depend WHERE refclassid=v1 AND refobjid=v2
* SELECT * FROM pg_depend WHERE refclassid=v1 AND refobjid=v2 (AND refobjsubid = v3 if applicable)
* 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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
*tuplist = list_insert_nth(*tuplist, index++, lfirst(lc));
*tuplist_flags |= SYSSCAN_ENR_NEEDFREE;
found = true;
}
else if (indexId == DependReferenceIndexId &&
tup->refclassid == (Oid)v1 &&
tup->refobjid == (Oid)v2)
tup->refobjid == (Oid)v2 &&
(nkeys == 2 || tup->refobjsubid == (int32)v3))
Comment on lines +457 to +458
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. good catch!

{
*tuplist = list_insert_nth(*tuplist, index++, lfirst(lc));
*tuplist_flags |= SYSSCAN_ENR_NEEDFREE;
Expand Down
Loading