Skip to content

Commit a1c6923

Browse files
tglsfdctglsfdc
committed
Adjust things so that the query_string of a cached plan and the sourceText of
a portal are never NULL, but reliably provide the source text of the query. It turns out that there was only one place that was really taking a short-cut, which was the 'EXECUTE' utility statement. That doesn't seem like a sufficiently critical performance hotspot to justify not offering a guarantee of validity of the portal source text. Fix it to copy the source text over from the cached plan. Add Asserts in the places that set up cached plans and portals to reject null source strings, and simplify a bunch of places that formerly needed to guard against nulls. There may be a few places that cons up statements for execution without having any source text at all; I found one such in ConvertTriggerToFK(). It seems sufficient to inject a phony source string in such a case, for instance ProcessUtility((Node *) atstmt, "(generated ALTER TABLE ADD FOREIGN KEY command)", NULL, false, None_Receiver, NULL); We should take a second look at the usage of debug_query_string, particularly the recently added current_query() SQL function. ITAGAKI Takahiro and Tom Lane
1 parent 6cc88f0 commit a1c6923

File tree

11 files changed

+66
-65
lines changed

11 files changed

+66
-65
lines changed

‎src/backend/commands/portalcmds.c

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
*
1515
*
1616
* IDENTIFICATION
17-
* $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.74 2008/05/12 20:01:59 alvherre Exp $
17+
* $PostgreSQL: pgsql/src/backend/commands/portalcmds.c,v 1.75 2008/07/18 20:26:06 tgl Exp $
1818
*
1919
*-------------------------------------------------------------------------
2020
*/
@@ -68,7 +68,7 @@ PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
6868
RequireTransactionChain(isTopLevel, "DECLARE CURSOR");
6969

7070
/*
71-
* Create a portal and copy the plan into its memory context.
71+
* Create a portal and copy the plan and queryString into its memory.
7272
*/
7373
portal = CreatePortal(cstmt->portalname, false, false);
7474

@@ -77,8 +77,7 @@ PerformCursorOpen(PlannedStmt *stmt, ParamListInfo params,
7777
stmt = copyObject(stmt);
7878
stmt->utilityStmt = NULL; /* make it look like plain SELECT */
7979

80-
if (queryString) /* copy the source text too for safety */
81-
queryString = pstrdup(queryString);
80+
queryString = pstrdup(queryString);
8281

8382
PortalDefineQuery(portal,
8483
NULL,

‎src/backend/commands/prepare.c

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
* Copyright (c) 2002-2008, PostgreSQL Global Development Group
1111
*
1212
* IDENTIFICATION
13-
* $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.87 2008/05/12 20:01:59 alvherre Exp $
13+
* $PostgreSQL: pgsql/src/backend/commands/prepare.c,v 1.88 2008/07/18 20:26:06 tgl Exp $
1414
*
1515
*-------------------------------------------------------------------------
1616
*/
@@ -174,6 +174,7 @@ ExecuteQuery(ExecuteStmt *stmt, const char *queryString,
174174
ParamListInfo paramLI = NULL;
175175
EState *estate = NULL;
176176
Portal portal;
177+
char *query_string;
177178

178179
/* Look it up in the hash table */
179180
entry = FetchPreparedStatement(stmt->name, true);
@@ -203,6 +204,10 @@ ExecuteQuery(ExecuteStmt *stmt, const char *queryString,
203204
/* Don't display the portal in pg_cursors, it is for internal use only */
204205
portal->visible = false;
205206

207+
/* Copy the plan's saved query string into the portal's memory */
208+
query_string = MemoryContextStrdup(PortalGetHeapMemory(portal),
209+
entry->plansource->query_string);
210+
206211
/*
207212
* For CREATE TABLE / AS EXECUTE, we must make a copy of the stored query
208213
* so that we can modify its destination (yech, but this has always been
@@ -249,13 +254,9 @@ ExecuteQuery(ExecuteStmt *stmt, const char *queryString,
249254
plan_list = cplan->stmt_list;
250255
}
251256

252-
/*
253-
* Note: we don't bother to copy the source query string into the portal.
254-
* Any errors it might be useful for will already have been reported.
255-
*/
256257
PortalDefineQuery(portal,
257258
NULL,
258-
NULL,
259+
query_string,
259260
entry->plansource->commandTag,
260261
plan_list,
261262
cplan);
@@ -777,12 +778,7 @@ pg_prepared_statement(PG_FUNCTION_ARGS)
777778
MemSet(nulls, 0, sizeof(nulls));
778779

779780
values[0] = CStringGetTextDatum(prep_stmt->stmt_name);
780-
781-
if (prep_stmt->plansource->query_string == NULL)
782-
nulls[1] = true;
783-
else
784-
values[1] = CStringGetTextDatum(prep_stmt->plansource->query_string);
785-
781+
values[1] = CStringGetTextDatum(prep_stmt->plansource->query_string);
786782
values[2] = TimestampTzGetDatum(prep_stmt->prepare_time);
787783
values[3] = build_regtype_array(prep_stmt->plansource->param_types,
788784
prep_stmt->plansource->num_params);

‎src/backend/commands/trigger.c

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
99
* IDENTIFICATION
10-
* $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.235 2008/07/13 20:45:47 tgl Exp $
10+
* $PostgreSQL: pgsql/src/backend/commands/trigger.c,v 1.236 2008/07/18 20:26:06 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -728,7 +728,8 @@ ConvertTriggerToFK(CreateTrigStmt *stmt, Oid funcoid)
728728

729729
/* ... and execute it */
730730
ProcessUtility((Node *) atstmt,
731-
NULL, NULL, false, None_Receiver, NULL);
731+
"(generated ALTER TABLE ADD FOREIGN KEY command)",
732+
NULL, false, None_Receiver, NULL);
732733

733734
/* Remove the matched item from the list */
734735
info_list = list_delete_ptr(info_list, info);

‎src/backend/executor/spi.c

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.196 2008/06/01 17:32:48 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/executor/spi.c,v 1.197 2008/07/18 20:26:06 tgl Exp $
1212
*
1313
*-------------------------------------------------------------------------
1414
*/
@@ -1057,10 +1057,8 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
10571057
*/
10581058
oldcontext = MemoryContextSwitchTo(PortalGetHeapMemory(portal));
10591059

1060-
/* Copy the plan's query string, if available, into the portal */
1061-
query_string = plansource->query_string;
1062-
if (query_string)
1063-
query_string = pstrdup(query_string);
1060+
/* Copy the plan's query string into the portal */
1061+
query_string = pstrdup(plansource->query_string);
10641062

10651063
/* If the plan has parameters, copy them into the portal */
10661064
if (plan->nargs > 0)

‎src/backend/parser/analyze.c

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
* Portions Copyright (c) 1996-2008, PostgreSQL Global Development Group
1818
* Portions Copyright (c) 1994, Regents of the University of California
1919
*
20-
* $PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.372 2008/06/19 00:46:04 alvherre Exp $
20+
* $PostgreSQL: pgsql/src/backend/parser/analyze.c,v 1.373 2008/07/18 20:26:06 tgl Exp $
2121
*
2222
*-------------------------------------------------------------------------
2323
*/
@@ -72,9 +72,6 @@ static bool check_parameter_resolution_walker(Node *node,
7272
* parse_analyze
7373
* Analyze a raw parse tree and transform it to Query form.
7474
*
75-
* If available, pass the source text from which the raw parse tree was
76-
* generated; it's OK to pass NULL if this is not available.
77-
*
7875
* Optionally, information about $n parameter types can be supplied.
7976
* References to $n indexes not defined by paramTypes[] are disallowed.
8077
*
@@ -89,6 +86,8 @@ parse_analyze(Node *parseTree, const char *sourceText,
8986
ParseState *pstate = make_parsestate(NULL);
9087
Query *query;
9188

89+
Assert(sourceText != NULL); /* required as of 8.4 */
90+
9291
pstate->p_sourcetext = sourceText;
9392
pstate->p_paramtypes = paramTypes;
9493
pstate->p_numparams = numParams;
@@ -115,6 +114,8 @@ parse_analyze_varparams(Node *parseTree, const char *sourceText,
115114
ParseState *pstate = make_parsestate(NULL);
116115
Query *query;
117116

117+
Assert(sourceText != NULL); /* required as of 8.4 */
118+
118119
pstate->p_sourcetext = sourceText;
119120
pstate->p_paramtypes = *paramTypes;
120121
pstate->p_numparams = *numParams;

‎src/backend/tcop/postgres.c

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.553 2008/05/15 00:17:40 tgl Exp $
11+
* $PostgreSQL: pgsql/src/backend/tcop/postgres.c,v 1.554 2008/07/18 20:26:06 tgl Exp $
1212
*
1313
* NOTES
1414
* this is the "main" module of the postgres backend and
@@ -1388,9 +1388,9 @@ exec_bind_message(StringInfo input_message)
13881388
/*
13891389
* Report query to various monitoring facilities.
13901390
*/
1391-
debug_query_string = psrc->query_string ? psrc->query_string : "<BIND>";
1391+
debug_query_string = psrc->query_string;
13921392

1393-
pgstat_report_activity(debug_query_string);
1393+
pgstat_report_activity(psrc->query_string);
13941394

13951395
set_ps_display("BIND", false);
13961396

@@ -1466,10 +1466,8 @@ exec_bind_message(StringInfo input_message)
14661466
*/
14671467
oldContext = MemoryContextSwitchTo(PortalGetHeapMemory(portal));
14681468

1469-
/* Copy the plan's query string, if available, into the portal */
1470-
query_string = psrc->query_string;
1471-
if (query_string)
1472-
query_string = pstrdup(query_string);
1469+
/* Copy the plan's query string into the portal */
1470+
query_string = pstrdup(psrc->query_string);
14731471

14741472
/* Likewise make a copy of the statement name, unless it's unnamed */
14751473
if (stmt_name[0])
@@ -1714,7 +1712,7 @@ exec_bind_message(StringInfo input_message)
17141712
*stmt_name ? stmt_name : "<unnamed>",
17151713
*portal_name ? "/" : "",
17161714
*portal_name ? portal_name : "",
1717-
psrc->query_string ? psrc->query_string : "<source not stored>"),
1715+
psrc->query_string),
17181716
errhidestmt(true),
17191717
errdetail_params(params)));
17201718
break;
@@ -1780,7 +1778,7 @@ exec_execute_message(const char *portal_name, long max_rows)
17801778
*/
17811779
if (is_xact_command)
17821780
{
1783-
sourceText = portal->sourceText ? pstrdup(portal->sourceText) : NULL;
1781+
sourceText = pstrdup(portal->sourceText);
17841782
if (portal->prepStmtName)
17851783
prepStmtName = pstrdup(portal->prepStmtName);
17861784
else
@@ -1805,9 +1803,9 @@ exec_execute_message(const char *portal_name, long max_rows)
18051803
/*
18061804
* Report query to various monitoring facilities.
18071805
*/
1808-
debug_query_string = sourceText ? sourceText : "<EXECUTE>";
1806+
debug_query_string = sourceText;
18091807

1810-
pgstat_report_activity(debug_query_string);
1808+
pgstat_report_activity(sourceText);
18111809

18121810
set_ps_display(portal->commandTag, false);
18131811

@@ -1840,15 +1838,14 @@ exec_execute_message(const char *portal_name, long max_rows)
18401838
if (check_log_statement(portal->stmts))
18411839
{
18421840
ereport(LOG,
1843-
(errmsg("%s %s%s%s%s%s",
1841+
(errmsg("%s %s%s%s: %s",
18441842
execute_is_fetch ?
18451843
_("execute fetch from") :
18461844
_("execute"),
18471845
prepStmtName,
18481846
*portal_name ? "/" : "",
18491847
*portal_name ? portal_name : "",
1850-
sourceText ? ": " : "",
1851-
sourceText ? sourceText : ""),
1848+
sourceText),
18521849
errhidestmt(true),
18531850
errdetail_params(portalParams)));
18541851
was_logged = true;
@@ -1924,16 +1921,15 @@ exec_execute_message(const char *portal_name, long max_rows)
19241921
break;
19251922
case 2:
19261923
ereport(LOG,
1927-
(errmsg("duration: %s ms %s %s%s%s%s%s",
1924+
(errmsg("duration: %s ms %s %s%s%s: %s",
19281925
msec_str,
19291926
execute_is_fetch ?
19301927
_("execute fetch from") :
19311928
_("execute"),
19321929
prepStmtName,
19331930
*portal_name ? "/" : "",
19341931
*portal_name ? portal_name : "",
1935-
sourceText ? ": " : "",
1936-
sourceText ? sourceText : ""),
1932+
sourceText),
19371933
errhidestmt(true),
19381934
errdetail_params(portalParams)));
19391935
break;
@@ -2049,7 +2045,7 @@ errdetail_execute(List *raw_parsetree_list)
20492045
PreparedStatement *pstmt;
20502046

20512047
pstmt = FetchPreparedStatement(stmt->name, false);
2052-
if (pstmt && pstmt->plansource->query_string)
2048+
if (pstmt)
20532049
{
20542050
errdetail("prepare: %s", pstmt->plansource->query_string);
20552051
return 0;

‎src/backend/tcop/utility.c

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@
1010
*
1111
*
1212
* IDENTIFICATION
13-
* $PostgreSQL: pgsql/src/backend/tcop/utility.c,v 1.294 2008/06/15 01:25:54 tgl Exp $
13+
* $PostgreSQL: pgsql/src/backend/tcop/utility.c,v 1.295 2008/07/18 20:26:06 tgl Exp $
1414
*
1515
*-------------------------------------------------------------------------
1616
*/
@@ -218,13 +218,17 @@ check_xact_readonly(Node *parsetree)
218218
* general utility function invoker
219219
*
220220
* parsetree: the parse tree for the utility statement
221-
* queryString: original source text of command (NULL if not available)
221+
* queryString: original source text of command
222222
* params: parameters to use during execution
223223
* isTopLevel: true if executing a "top level" (interactively issued) command
224224
* dest: where to send results
225225
* completionTag: points to a buffer of size COMPLETION_TAG_BUFSIZE
226226
* in which to store a command completion status string.
227227
*
228+
* Notes: as of PG 8.4, caller MUST supply a queryString; it is not
229+
* allowed anymore to pass NULL. (If you really don't have source text,
230+
* you can pass a constant string, perhaps "(query not available)".)
231+
*
228232
* completionTag is only set nonempty if we want to return a nondefault status.
229233
*
230234
* completionTag may be NULL if caller doesn't want a status string.
@@ -237,6 +241,8 @@ ProcessUtility(Node *parsetree,
237241
DestReceiver *dest,
238242
char *completionTag)
239243
{
244+
Assert(queryString != NULL); /* required as of 8.4 */
245+
240246
check_xact_readonly(parsetree);
241247

242248
if (completionTag)

‎src/backend/utils/cache/plancache.c

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
* Portions Copyright (c) 1994, Regents of the University of California
3434
*
3535
* IDENTIFICATION
36-
* $PostgreSQL: pgsql/src/backend/utils/cache/plancache.c,v 1.18 2008/05/12 20:02:02 alvherre Exp $
36+
* $PostgreSQL: pgsql/src/backend/utils/cache/plancache.c,v 1.19 2008/07/18 20:26:06 tgl Exp $
3737
*
3838
*-------------------------------------------------------------------------
3939
*/
@@ -104,8 +104,7 @@ InitPlanCache(void)
104104
* about all that we do here is copy it into permanent storage.
105105
*
106106
* raw_parse_tree: output of raw_parser()
107-
* query_string: original query text (can be NULL if not available, but
108-
* that is discouraged because it degrades error message quality)
107+
* query_string: original query text (as of PG 8.4, must not be NULL)
109108
* commandTag: compile-time-constant tag for query, or NULL if empty query
110109
* param_types: array of parameter type OIDs, or NULL if none
111110
* num_params: number of parameters
@@ -130,6 +129,8 @@ CreateCachedPlan(Node *raw_parse_tree,
130129
MemoryContext source_context;
131130
MemoryContext oldcxt;
132131

132+
Assert(query_string != NULL); /* required as of 8.4 */
133+
133134
/*
134135
* Make a dedicated memory context for the CachedPlanSource and its
135136
* subsidiary data. We expect it can be pretty small.
@@ -152,7 +153,7 @@ CreateCachedPlan(Node *raw_parse_tree,
152153
oldcxt = MemoryContextSwitchTo(source_context);
153154
plansource = (CachedPlanSource *) palloc(sizeof(CachedPlanSource));
154155
plansource->raw_parse_tree = copyObject(raw_parse_tree);
155-
plansource->query_string = query_string ? pstrdup(query_string) : NULL;
156+
plansource->query_string = pstrdup(query_string);
156157
plansource->commandTag = commandTag; /* no copying needed */
157158
if (num_params > 0)
158159
{
@@ -228,6 +229,8 @@ FastCreateCachedPlan(Node *raw_parse_tree,
228229
OverrideSearchPath *search_path;
229230
MemoryContext oldcxt;
230231

232+
Assert(query_string != NULL); /* required as of 8.4 */
233+
231234
/*
232235
* Fetch current search_path into given context, but do any recalculation
233236
* work required in caller's context.

0 commit comments

Comments
 (0)