Skip to content

Commit

Permalink
Refactor SaveOidAssignments and RestoreOidAssignments logic.
Browse files Browse the repository at this point in the history
The two functions SaveOidAssignments and RestoreOidAssignments should
come together, before some procedures that do not want to touch the
global vars (dispatch_oids or preassigned_oids), we need to first save
the oid assignments, and then do the job, finally restore oid
assignments. A typical usage should be as below:
   List *l = SaveOidAssignments();
   do_the_job();
   RestoreOidAssignments(l);

The global var dispatch_oids is only used on QD, and the global var
preassigned_oids is only used on QEs. They are both Lists, in a
specific memorycontext, normally the memorycontext will be reset at
the end of transaction. Greenplum's MPP architecture need to make some
OIDs consistent among coordinator and segments (like table OIDs). The
oid assignments are generated on QD and then dispatched to QEs. A
single SQL might involve sever dispatch events, for example, there are
some functions involving SQLs and these functions are evaluated during
planning stage before we dispatch the final Utility plan. We do not
want to the dispatches during plannign stage to touch oid assignments.

Another subtle case that the pair of functions are useful is that
subtransaction abort will lead to reset  of the oid assignments memory
context. Subtransaction abort might happen for UDF with exception
handling and nothing to do with the main statement needed to
dispatch. That is why we deep copy the content to CurrentMemoryContext
and reset oid assignment context during SaveOidAssignments and bring
everything back during RestoreOidAssignments.

This commit adds the two functions before eval_constant_expressions()
to make sure the procedure does not touch oid assignments.

Fix issue: greenplum-db/gpdb#14465.
  • Loading branch information
kainwen committed Nov 15, 2022
1 parent 9871927 commit b66fc3c
Show file tree
Hide file tree
Showing 6 changed files with 119 additions and 3 deletions.
83 changes: 82 additions & 1 deletion src/backend/catalog/oid_dispatch.c
Original file line number Diff line number Diff line change
Expand Up @@ -193,10 +193,91 @@ ClearOidAssignmentsOnCommit(void)
preserve_oids_on_commit = false;
}

/*
* Comments for SaveOidAssignments and RestoreOidAssignments
* The two functions should come together, before some procedures
* that do not want to touch the global vars (dispatch_oids or preassigned_oids),
* we need to first save the oid assignments, and then do the job, finally
* restore oid assignments. A typical usage should be as below:
* List *l = SaveOidAssignments();
* do_the_job();
* RestoreOidAssignments(l);
*
* The global var dispatch_oids is only used on QD, and the global
* var preassigned_oids is only used on QEs. They are both Lists,
* in a specific memorycontext, normally the memorycontext will be
* reset at the end of transaction.
*
* Greenplum's MPP architecture need to make some OIDs consistent
* among coordinator and segments (like table OIDs). The oid assignments
* are generated on QD and then dispatched to QEs. A single SQL might
* involve sever dispatch events, for example, there are some functions
* involving SQLs and these functions are evaluated during planning stage
* before we dispatch the final Utility plan. We do not want to the dispatches
* during plannign stage to touch oid assignments.
*
* Another subtle case that the pair of functions are useful is that
* subtransaction abort will lead to reset of the oid assignments memory context.
* Subtransaction abort might happen for UDF with exception handling and nothing
* to do with the main statement needed to dispatch. That is why we deep copy
* the content to CurrentMemoryContext and reset oid assignment context during
* SaveOidAssignments and bring everything back during RestoreOidAssignments.
*
* Note: these two functions only do memory related operations when the gloabl
* vars are not empty.
*/
List *
SaveOidAssignments(void)
{
List *l = NIL;
List *src = NIL;

if (Gp_role == GP_ROLE_DISPATCH)
{
src = dispatch_oids;
dispatch_oids = NIL;
}
else if (Gp_role == GP_ROLE_EXECUTE)
{
src = preassigned_oids;
preassigned_oids = NIL;
}
else
return NIL;

if (src == NIL)
return NIL;

Assert(CurrentMemoryContext != get_oids_context());

l = copyObject(src);
MemoryContextReset(get_oids_context());
return l;
}

void
RestoreOidAssignments(List *oid_assignments)
{
dispatch_oids = oid_assignments;
MemoryContext old;
List **target;

if (oid_assignments == NIL)
return;

if (Gp_role == GP_ROLE_DISPATCH)
target = &dispatch_oids;
else if (Gp_role == GP_ROLE_EXECUTE)
target = &preassigned_oids;
else
return;

Assert(CurrentMemoryContext != get_oids_context());

old = MemoryContextSwitchTo(get_oids_context());
*target = copyObject(oid_assignments);
MemoryContextSwitchTo(old);

list_free_deep(oid_assignments);
}

/* ----------------------------------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion src/backend/commands/matview.c
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ refresh_matview_datafill(DestReceiver *dest, Query *query,
*
* See Github Issue for details: https://github.com/greenplum-db/gpdb/issues/11956
*/
List *saved_dispatch_oids = GetAssignedOidsForDispatch();
List *saved_dispatch_oids = SaveOidAssignments();

/* Lock and rewrite, using a copy to preserve the original query. */
copied_query = copyObject(query);
Expand Down
9 changes: 8 additions & 1 deletion src/backend/optimizer/util/clauses.c
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
#include "postgres.h"

#include "access/htup_details.h"
#include "catalog/oid_dispatch.h"
#include "catalog/pg_aggregate.h"
#include "catalog/pg_class.h"
#include "catalog/pg_language.h"
Expand Down Expand Up @@ -2478,6 +2479,8 @@ Node *
eval_const_expressions(PlannerInfo *root, Node *node)
{
eval_const_expressions_context context;
Node *result;
List *saved_oid_assignments;

if (root)
context.boundParams = root->glob->boundParams; /* bound Params */
Expand All @@ -2492,7 +2495,11 @@ eval_const_expressions(PlannerInfo *root, Node *node)
context.max_size = 0;
context.eval_stable_functions = should_eval_stable_functions(root);

return eval_const_expressions_mutator(node, &context);
saved_oid_assignments = SaveOidAssignments();
result = eval_const_expressions_mutator(node, &context);
RestoreOidAssignments(saved_oid_assignments);

return result;
}

/*--------------------
Expand Down
1 change: 1 addition & 0 deletions src/include/catalog/oid_dispatch.h
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@ extern void RememberPreassignedIndexNameForChildIndex(Oid parentIdxOid, Oid chil
/* Functions used in master and QE nodes */
extern void PreserveOidAssignmentsOnCommit(void);
extern void ClearOidAssignmentsOnCommit(void);
extern List * SaveOidAssignments(void);
extern void RestoreOidAssignments(List *oid_assignments);

/* Functions used in binary upgrade */
Expand Down
14 changes: 14 additions & 0 deletions src/test/regress/expected/oid_consistency.out
Original file line number Diff line number Diff line change
Expand Up @@ -797,3 +797,17 @@ select verify('trigger_oid');
1
(1 row)

-- Case for Issue: https://github.com/greenplum-db/gpdb/issues/14465
create function func_fail_14465(int) returns int
immutable language plpgsql as $$
begin
perform unwanted_grant();
raise warning 'owned';
return 1;
exception when others then
return 2;
end$$;
create materialized view mv_14465 as select 1 as c;
NOTICE: Table doesn't have 'DISTRIBUTED BY' clause -- Using column(s) named 'c' as the Greenplum Database data distribution key for this table.
HINT: The 'DISTRIBUTED BY' clause determines the distribution of data. Make sure column(s) chosen are the optimal data distribution key to minimize skew.
create index on mv_14465 (c) where func_fail_14465(1) > 0;
13 changes: 13 additions & 0 deletions src/test/regress/sql/oid_consistency.sql
Original file line number Diff line number Diff line change
Expand Up @@ -434,3 +434,16 @@ $$ language plpgsql no sql;
create trigger troid_trigger after insert on trigger_oid for each row execute procedure trig_func();

select verify('trigger_oid');

-- Case for Issue: https://github.com/greenplum-db/gpdb/issues/14465
create function func_fail_14465(int) returns int
immutable language plpgsql as $$
begin
perform unwanted_grant();
raise warning 'owned';
return 1;
exception when others then
return 2;
end$$;
create materialized view mv_14465 as select 1 as c;
create index on mv_14465 (c) where func_fail_14465(1) > 0;

0 comments on commit b66fc3c

Please sign in to comment.