On Thu, Feb 23, 2012 at 3:09 AM, Peter Geoghegan <pe...@2ndquadrant.com> wrote:
>> These have all been changed in the usual manner to support one new
>> field, the queryId, on the toplevel Plan and Query nodes. The queryId
>> is 64-bit field copied from queries to plans to tie together plans "to
>> be used by plugins" (quoth the source) as they flow through the
>> system. pg_stat_statements fills them with the 64-bit hash of the
>> query text -- a reasonable functionality to possess, I think, but the
>> abstraction seems iffy: plugins cannot compose well if they all need
>> to use the queryId for different reasons. Somehow I get the feeling
>> that this field can be avoided or its use case can be satisfied in a
>> more satisfying way.
>
> Maybe the answer here is to have a simple mechanism for modules to
> claim the exclusive right to be able to set that flag?
As I see it, the queryId as the most contentious internals change in
the patch, as queryId is not really an opaque id tracking a query's
progression, but rather a bit of extra space for query jumbles, and
query jumbles only, as far as I can tell: thus, not really a generally
useful backend service, but rather specifically for pg_stat_statements
-- that may not take such a special field off the table (more
opinionated people will probably comment on that), but it's a pretty
awkward kind of inclusion.
I'm posting an experimental patch in having the backend define state
whose only guarantee is to be equal between a PlannedStmt and a Query
node that derived it, and its effect on pg_stat_statements. (the
latter not included in this email, but available via git[0]) The
exact mechanism I use is pretty questionable, but my skimming of the
entry points of the code suggests it might work: I use the pointer to
the Query node from the PlannedStmt, which is only unique as long as
the Query (head) node remains allocated while the PlannedStmt is also
alive. A big if, but able to be verified with assertions in debug
mode ("does the pointer see poisoned memory?") and low overhead
presuming one had to allocate memory already. However, the
abstraction is as such that if the key generation needs to change
compliant consumers should be fine.
At ExecutorFinish (already hookable) all NodeKeys remembered by an
extension should be invalidated, as that memory is free and ready to
be used again.
As Query node availability from the PlannedStmt is not part of the
node for a reason, it is rendered as an opaque uintptr_t, and hidden
behind a type that only is intended to define equality and
"definedness" (!= NULL is the implementation).
The clear penalty is that the extension must be able to map from the
arbitrary, backend-assigned queryId (which I have renamed nodeKey just
to make it easier to discuss) to its own internal value it cares
about: the query jumble. I used a very simple, fixed-size association
array with a small free-space location optimization, taking advantage
of the property in pg_stat_statements can simply not process a query
if it doesn't want to (but should process most of them so one can get
a good sense of what is going on).
A credible(?) alternative I have thought of is to instead expose a
memory context for use by extensions on the interesting nodes; in
doing so extensions can request space and store whatever they need.
The part that I feel might be tricky is figuring out a reasonable and
appropriate time to free that 'extension-context' and what context
that context should live under. But I'd be wiling to try it with
haste if it sounds a lot more credible or useful than what I've got.
--
fdr
[0]: https://github.com/fdr/postgres/tree/wrench-pgss
From c3ad77375062cfbeee7d4ce7e0fe274a5db76453 Mon Sep 17 00:00:00 2001
From: Daniel Farina <dan...@heroku.com>
Date: Fri, 24 Feb 2012 01:31:54 -0800
Subject: [PATCH] Introduce NodeKey as a service to extensions in the backend
Signed-off-by: Daniel Farina <dan...@heroku.com>
---
src/backend/nodes/copyfuncs.c | 4 ++--
src/backend/nodes/outfuncs.c | 6 ------
src/backend/nodes/readfuncs.c | 7 +++----
src/backend/optimizer/plan/planner.c | 2 +-
src/backend/parser/analyze.c | 2 +-
src/include/nodes/parsenodes.h | 4 ++--
src/include/nodes/plannodes.h | 4 +++-
src/include/nodes/primnodes.h | 14 ++++++++++++++
8 files changed, 26 insertions(+), 17 deletions(-)
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***************
*** 91,97 **** _copyPlannedStmt(const PlannedStmt *from)
COPY_NODE_FIELD(relationOids);
COPY_NODE_FIELD(invalItems);
COPY_SCALAR_FIELD(nParamExec);
! COPY_SCALAR_FIELD(queryId);
return newnode;
}
--- 91,97 ----
COPY_NODE_FIELD(relationOids);
COPY_NODE_FIELD(invalItems);
COPY_SCALAR_FIELD(nParamExec);
! COPY_SCALAR_FIELD(nodeKey);
return newnode;
}
***************
*** 2415,2421 **** _copyQuery(const Query *from)
COPY_SCALAR_FIELD(commandType);
COPY_SCALAR_FIELD(querySource);
! COPY_SCALAR_FIELD(queryId);
COPY_SCALAR_FIELD(canSetTag);
COPY_NODE_FIELD(utilityStmt);
COPY_SCALAR_FIELD(resultRelation);
--- 2415,2421 ----
COPY_SCALAR_FIELD(commandType);
COPY_SCALAR_FIELD(querySource);
! COPY_SCALAR_FIELD(nodeKey);
COPY_SCALAR_FIELD(canSetTag);
COPY_NODE_FIELD(utilityStmt);
COPY_SCALAR_FIELD(resultRelation);
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***************
*** 81,90 ****
#define WRITE_LOCATION_FIELD(fldname) \
appendStringInfo(str, " :" CppAsString(fldname) " %d", node->fldname)
- /* Write a query id field */
- #define WRITE_QUERYID_FIELD(fldname) \
- ((void) 0)
-
/* Write a Node field */
#define WRITE_NODE_FIELD(fldname) \
(appendStringInfo(str, " :" CppAsString(fldname) " "), \
--- 81,86 ----
***************
*** 259,265 **** _outPlannedStmt(StringInfo str, const PlannedStmt *node)
WRITE_NODE_FIELD(relationOids);
WRITE_NODE_FIELD(invalItems);
WRITE_INT_FIELD(nParamExec);
- WRITE_QUERYID_FIELD(queryId);
}
/*
--- 255,260 ----
***************
*** 2164,2170 **** _outQuery(StringInfo str, const Query *node)
WRITE_ENUM_FIELD(commandType, CmdType);
WRITE_ENUM_FIELD(querySource, QuerySource);
- WRITE_QUERYID_FIELD(query_id);
WRITE_BOOL_FIELD(canSetTag);
/*
--- 2159,2164 ----
*** a/src/backend/nodes/readfuncs.c
--- b/src/backend/nodes/readfuncs.c
***************
*** 116,124 ****
token = pg_strtok(&length); /* get field value */ \
local_node->fldname = -1 /* set field to "unknown" */
- /* NOOP */
- #define READ_QUERYID_FIELD(fldname) \
- ((void) 0)
/* Read a Node field */
#define READ_NODE_FIELD(fldname) \
token = pg_strtok(&length); /* skip :fldname */ \
--- 116,121 ----
***************
*** 206,212 **** _readQuery(void)
READ_ENUM_FIELD(commandType, CmdType);
READ_ENUM_FIELD(querySource, QuerySource);
- READ_QUERYID_FIELD(query_id);
READ_BOOL_FIELD(canSetTag);
READ_NODE_FIELD(utilityStmt);
READ_INT_FIELD(resultRelation);
--- 203,208 ----
***************
*** 234,239 **** _readQuery(void)
--- 230,238 ----
READ_NODE_FIELD(setOperations);
READ_NODE_FIELD(constraintDeps);
+ /* Set up the unique but arbitrary nodeKey */
+ local_node->nodeKey = (NodeKey) local_node;
+
READ_DONE();
}
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
***************
*** 240,246 **** standard_planner(Query *parse, int cursorOptions, ParamListInfo boundParams)
result->relationOids = glob->relationOids;
result->invalItems = glob->invalItems;
result->nParamExec = list_length(glob->paramlist);
! result->queryId = parse->queryId;
return result;
}
--- 240,246 ----
result->relationOids = glob->relationOids;
result->invalItems = glob->invalItems;
result->nParamExec = list_length(glob->paramlist);
! result->nodeKey = parse->nodeKey;
return result;
}
*** a/src/backend/parser/analyze.c
--- b/src/backend/parser/analyze.c
***************
*** 245,250 **** transformStmt(ParseState *pstate, Node *parseTree)
--- 245,251 ----
/* Mark as original query until we learn differently */
result->querySource = QSRC_ORIGINAL;
result->canSetTag = true;
+ result->nodeKey = (NodeKey) result;
return result;
}
***************
*** 905,911 **** transformSelectStmt(ParseState *pstate, SelectStmt *stmt)
ListCell *l;
qry->commandType = CMD_SELECT;
- qry->queryId = 0;
/* process the WITH clause independently of all else */
if (stmt->withClause)
--- 906,911 ----
*** a/src/include/nodes/parsenodes.h
--- b/src/include/nodes/parsenodes.h
***************
*** 103,110 **** typedef struct Query
QuerySource querySource; /* where did I come from? */
! uint32 queryId; /* query identifier that can be set by plugins.
! * Will be copied to resulting PlannedStmt. */
bool canSetTag; /* do I set the command result tag? */
--- 103,110 ----
QuerySource querySource; /* where did I come from? */
! NodeKey nodeKey; /* query identifier so hooks can associate
! * queries to plans */
bool canSetTag; /* do I set the command result tag? */
*** a/src/include/nodes/plannodes.h
--- b/src/include/nodes/plannodes.h
***************
*** 68,74 **** typedef struct PlannedStmt
int nParamExec; /* number of PARAM_EXEC Params used */
! uint32 queryId; /* query identifier carried from query tree */
} PlannedStmt;
/* macro for fetching the Plan associated with a SubPlan node */
--- 68,76 ----
int nParamExec; /* number of PARAM_EXEC Params used */
! NodeKey nodeKey; /* Possesses a value that can be used to
! * uniquely identify the source of this plan
! * within one backend. */
} PlannedStmt;
/* macro for fetching the Plan associated with a SubPlan node */
*** a/src/include/nodes/primnodes.h
--- b/src/include/nodes/primnodes.h
***************
*** 1266,1269 **** typedef struct FromExpr
--- 1266,1283 ----
Node *quals; /* qualifiers on join, if any */
} FromExpr;
+ /*
+ * NodeKey - Associate nodes together throughout query parse and planning
+ *
+ * This is currently implemented as intptr_t to piggyback on memory allocation
+ * as a way to derive unique, unambiguous keys within a backend with low
+ * overhead. This means that given this implementation of NodeKey that the
+ * target of the pointer deriving the NodeKey *must* remain allocated, or it
+ * risks being reused.
+ *
+ */
+ typedef uintptr_t NodeKey;
+
+ #define NodeKeyDefined(nk) ((nk) != (NodeKey) NULL)
+
#endif /* PRIMNODES_H */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
