Prevent gp_tablespace_segment_location() from executing on entrydb QE #13287

interma · 2022-03-21T12:08:11Z

gp_tablespace_segment_location() has its own dispatching logic, so they do not work on entrydb QE since we do not support dispatching on it. These are other functions like it, we fix them before by giving a warning to user, example: #11178.

gp_tablespace_segment_location() is defined in src/backend/catalog/system_views.sql, it has been fixed in master (with more details and repro scenario): #13075: changed it definition in system_views.sql.

But it cannot port to 6X directly: modify system_views.sql breaks the catalog interface, so we need handle it in c code.
This PR does it and refactors the previous similar logic.

Here are some reminders before you submit the pull request

Add tests for the change
Document changes
Communicate in the mailing list if needed
Pass make installcheck
Review a PR in return to support the community

interma · 2022-03-23T01:12:58Z

src/backend/optimizer/util/pathnode.c

+		;
+	}
+
+	if (!strcmp(proname, "gp_tablespace_segment_location"))


compare the function name one by one is not effective here, but I think it's acceptable:

there are not many such functions

only exec_location == PROEXECLOCATION_ALL_SEGMENTS and run on entrydb does this check

kainwen

I think the banned list logic might be added in init_fcache. Please spike it.

kainwen · 2022-03-23T02:29:20Z

src/backend/optimizer/util/pathnode.c

+static bool
+function_not_run_entrydb(const char *proname, Oid funcid)
+{
+	// check by oid (if it has a fixed oid)


/* */ style comments.

kainwen · 2022-03-23T02:33:20Z

src/backend/optimizer/util/pathnode.c

+	}
+
+	// check by func name
+	if (!strcmp(proname, "gp_tablespace_segment_location"))


We cannot compare via text name. In theory, user can define a UDF has the same name but in different schema.

So please use funcid, this is the identifier.

kainwen · 2022-03-23T02:33:37Z

src/backend/optimizer/util/pathnode.c

@@ -2793,7 +2814,45 @@ create_functionscan_path(PlannerInfo *root, RelOptInfo *rel,
 		}
 	}
 	else
+	{
+		// Gp_role == GP_ROLE_EXECUTE


Comment style and extra blank line.

kainwen · 2022-03-23T02:37:02Z

src/backend/optimizer/util/pathnode.c

+				const char *proname = NameStr(procform->proname);;
+				char this_exec_location = func_exec_location(funcid);
+
+				if (GpIdentity.segindex == MASTER_CONTENT_ID &&


create_function_scan is called during planning stage. Most of the case, GpIdentity.segindex == MASTER_CONTENT_ID will be true. Here we are creating plan not running a function. Thsi is not correct place to add the logic.

interma · 2022-03-23T03:13:41Z

thanks Zhenghua, I will fix them later.

interma · 2022-03-30T01:04:29Z

thanks zhenghua for review!

interma · 2022-03-30T02:26:31Z

refactor the code: integrated Junfeng's #11178 logic in my PR.

kainwen · 2022-04-12T11:01:14Z

src/backend/executor/execQual.c

+	if (retvalue)
+		return retvalue;
+
+	char exec_location = func_exec_location(foid);


These two lines 1487 & 1488 seem redundant to me.

My thoughts:
so far, these functions (need to check by name) are not too many, and all of them arePROEXECLOCATION_ALL_SEGMENTS, using the if condition first can boost a little performance (avoid every function to go to strcmp later).

I will add some comments in code.

My thoughts: so far, these functions (need to check by name) are not too many, and all of them arePROEXECLOCATION_ALL_SEGMENTS, using the if condition first can boost a little performance (avoid every function to go to strcmp later).

I will add some comments in code.

But the logic is a bit confusing. We want to disallow a function's call by name, but here we'd try to fetch another field to filter out first.

Also func_exec_location will search the syscache and I do not think the perf is changed.

no problem, I will fix it.

kainwen · 2022-04-12T11:02:00Z

src/backend/optimizer/util/pathnode.c

@@ -2793,7 +2793,9 @@ create_functionscan_path(PlannerInfo *root, RelOptInfo *rel,
 		}
 	}
 	else
+	{


This seems not related to the issue, it is better not to change.

kainwen · 2022-04-12T11:02:43Z

src/backend/executor/execQual.c

+ * So they do not work on entrydb since we do not support dispatching
+ * from entry-db currently.
+ */
+static bool


Maybe one word on why some are checked via OID and some are by text.

kainwen

@interma
I have added some more minor comments. Please address them before merging. Thanks.

interma · 2022-04-12T11:12:33Z

got it, will fix them later, thanks zhenghua!

interma · 2022-04-14T07:51:01Z

merged, thanks zhenghua for review it!

…greenplum-db#13287) gp_tablespace_segment_location() has its own dispatching logic, so they do not work on entrydb QE since we do not support dispatching on it. These are other functions like it, we fix them before by giving a warning to user, example: greenplum-db#11178. gp_tablespace_segment_location() is defined in src/backend/catalog/system_views.sql, it has been fixed in master (with more details and repro scenario): greenplum-db#13075: changed it definition in system_views.sql. But it cannot port to 6X directly: modify system_views.sql breaks the catalog interface, so we need handle it in c code. This PR does it and refactors the previous similar logic.

try to fix 6x

078b9a6

interma added the version: 6X_STABLE label Mar 21, 2022

add forbid func code

40c6bb0

interma commented Mar 23, 2022

View reviewed changes

add test cases

2b66c54

interma changed the title ~~[WIP] Prevent gp_tablespace_segment_location() from executing on entrydb QE~~ Prevent gp_tablespace_segment_location() from executing on entrydb QE Mar 23, 2022

interma requested a review from kainwen March 23, 2022 01:54

kainwen requested changes Mar 23, 2022

View reviewed changes

interma added 2 commits March 29, 2022 19:46

move the judge logic to init_fcache()

a7c9e09

fix test out file

6814629

kainwen approved these changes Mar 29, 2022

View reviewed changes

refactor and cleanup code

4ba2948

kainwen reviewed Apr 12, 2022

View reviewed changes

interma added 2 commits April 13, 2022 09:26

fix code review issues

009caf4

fix code review 2

ae410da

interma merged commit 4bc90ea into greenplum-db:6X_STABLE Apr 14, 2022

Sasasu mentioned this pull request May 7, 2022

6X: dispatch temporary namespace ID to all Gangs. #13213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent gp_tablespace_segment_location() from executing on entrydb QE #13287

Prevent gp_tablespace_segment_location() from executing on entrydb QE #13287

interma commented Mar 21, 2022 •

edited

interma Mar 23, 2022

kainwen left a comment

kainwen Mar 23, 2022

kainwen Mar 23, 2022

kainwen Mar 23, 2022

kainwen Mar 23, 2022

interma commented Mar 23, 2022

interma commented Mar 30, 2022

interma commented Mar 30, 2022

kainwen Apr 12, 2022

interma Apr 13, 2022 •

edited

kainwen Apr 14, 2022

interma Apr 14, 2022

kainwen Apr 12, 2022

interma Apr 13, 2022

kainwen Apr 12, 2022

interma Apr 13, 2022

kainwen left a comment

interma commented Apr 12, 2022

interma commented Apr 14, 2022

Prevent gp_tablespace_segment_location() from executing on entrydb QE #13287

Prevent gp_tablespace_segment_location() from executing on entrydb QE #13287

Conversation

interma commented Mar 21, 2022 • edited

Here are some reminders before you submit the pull request

Choose a reason for hiding this comment

kainwen left a 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

interma commented Mar 23, 2022

interma commented Mar 30, 2022

interma commented Mar 30, 2022

Choose a reason for hiding this comment

interma Apr 13, 2022 • edited

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

kainwen left a comment

Choose a reason for hiding this comment

interma commented Apr 12, 2022

interma commented Apr 14, 2022

interma commented Mar 21, 2022 •

edited

interma Apr 13, 2022 •

edited