On 10/24/14, 6:17 PM, Jim Nasby wrote:
- Does anyone have a tangible suggestion for how to reduce the code
duplication in patch #6?
Between execute_sql_string() and tcop/exec_simple_query()? Is there stuff in
exec_simple that's not safe for bgwriter? I'm not seeing why we can't use
exec_simple. :/
There are some differences if you compare them closely.
Without doing a deep dive, I'm guessing that most of the extra stuff would be
safe to re-use; it just wouldn't affect execute_sql_string. Obviously we could
add a boolean to exec_simple_query for the case when it's being used by a
bgwriter. Though, it seems like the biggest differences have to do with logging
Here's the differences I see:
- Disallowing transaction commands
- Logging
- What memory context is used (could we just use that differently in a
pg_backend backend?)
- Portal output format
- What to do with the output of intermediate commands (surely there's other
places we need to handle that? plpgsql maybe?)
I think all of those except logging could be handled by a function serving both
exec_simple_query and execute_sql_command that accepts a few booleans (or maybe
just one to indicate the caller) and some if's. At least I don't think it'd be
too bad, without actually writing it.
I'm not sure what to do about logging. If the original backend has logging
turned on, is it that horrible to do the same logging as exec_simple_query
would do?
Another option would be factoring out parts of exec_simple_query; the for loop
over the parse tree might be a good candidate. But I suspect that would be
uglier than a separate support function.
I do feel uncomfortable with the amount of duplication there is right now
though...
I took a stab at this by refactoring the guts of exec_simple_query (patch
attached) into a new function called exec_query_string (also attached in raw
form). As indicated it needs a bit more work. In particular, how it's dealing
with excluding transactional commands is rather ugly. Why do we need to do that
in pg_background?
Andres was concerned about the performance impact of doing this. I tested this
by doing
for i in {1..999999}; do echo 'SELECT 1;' >> test.sql; done
and then
time psql -f test.sql > /dev/nul
It appears there may be a ~1% performance hit, but my laptop isn't repeatable
enough to be sure. I did try manually in-lining exec_query_string to see if it
was the function call causing the issue; it didn't seem to make a difference.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com
Author: Jim Nasby <jim.na...@bluetreble.com>
Date: Thu, 30 Oct 2014 20:25:34 -0500
Move the bulk of exec_simple_query into exec_query_string() so that
pg_backend can also make use of it.
I’m not thrilled about the name, and I’m not sure tcopprot.h is the
right place to expose this. Also note the XXX comments.
---
contrib/pg_background/pg_background.c | 141 ++--------------------------------
src/backend/tcop/postgres.c | 83 ++++++++++++++++----
src/include/tcop/tcopprot.h | 7 ++
3 files changed, 80 insertions(+), 151 deletions(-)
diff --git a/contrib/pg_background/pg_background.c
b/contrib/pg_background/pg_background.c
index a566ffb..075ecd8 100644
--- a/contrib/pg_background/pg_background.c
+++ b/contrib/pg_background/pg_background.c
@@ -817,10 +817,6 @@ pg_background_worker_main(Datum main_arg)
static void
execute_sql_string(const char *sql)
{
- List *raw_parsetree_list;
- ListCell *lc1;
- bool isTopLevel;
- int commands_remaining;
MemoryContext parsecontext;
MemoryContext oldcontext;
@@ -839,139 +835,16 @@ execute_sql_string(const char *sql)
ALLOCSET_DEFAULT_MINSIZE,
ALLOCSET_DEFAULT_INITSIZE,
ALLOCSET_DEFAULT_MAXSIZE);
- oldcontext = MemoryContextSwitchTo(parsecontext);
- raw_parsetree_list = pg_parse_query(sql);
- commands_remaining = list_length(raw_parsetree_list);
- isTopLevel = commands_remaining == 1;
- MemoryContextSwitchTo(oldcontext);
/*
- * Do parse analysis, rule rewrite, planning, and execution for each raw
- * parsetree. We must fully execute each query before beginning parse
- * analysis on the next one, since there may be interdependencies.
+ * Do the real work
*/
- foreach(lc1, raw_parsetree_list)
- {
- Node *parsetree = (Node *) lfirst(lc1);
- const char *commandTag;
- char completionTag[COMPLETION_TAG_BUFSIZE];
- List *querytree_list,
- *plantree_list;
- bool snapshot_set = false;
- Portal portal;
- DestReceiver *receiver;
- int16 format = 1;
-
- /*
- * We don't allow transaction-control commands like COMMIT and
ABORT
- * here. The entire SQL statement is executed as a single
transaction
- * which commits if no errors are encountered.
- */
- if (IsA(parsetree, TransactionStmt))
- ereport(ERROR,
- (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("transaction control statements
are not allowed in pg_background")));
-
- /*
- * Get the command name for use in status display (it also
becomes the
- * default completion tag, down inside PortalRun). Set
ps_status and
- * do any special start-of-SQL-command processing needed by the
- * destination.
- */
- commandTag = CreateCommandTag(parsetree);
- set_ps_display(commandTag, false);
- BeginCommand(commandTag, DestNone);
-
- /* Set up a snapshot if parse analysis/planning will need one.
*/
- if (analyze_requires_snapshot(parsetree))
- {
- PushActiveSnapshot(GetTransactionSnapshot());
- snapshot_set = true;
- }
-
- /*
- * OK to analyze, rewrite, and plan this query.
- *
- * As with parsing, we need to make sure this data outlives the
- * transaction, because of the possibility that the statement
might
- * perform internal transaction control.
- */
- oldcontext = MemoryContextSwitchTo(parsecontext);
- querytree_list = pg_analyze_and_rewrite(parsetree, sql, NULL,
0);
- plantree_list = pg_plan_queries(querytree_list, 0, NULL);
-
- /* Done with the snapshot used for parsing/planning */
- if (snapshot_set)
- PopActiveSnapshot();
-
- /* If we got a cancel signal in analysis or planning, quit */
- CHECK_FOR_INTERRUPTS();
-
- /*
- * Execute the query using the unnamed portal.
- */
- portal = CreatePortal("", true, true);
- /* Don't display the portal in pg_cursors */
- portal->visible = false;
- PortalDefineQuery(portal, NULL, sql, commandTag, plantree_list,
NULL);
- PortalStart(portal, NULL, 0, InvalidSnapshot);
-
- /* We always use binary format, for efficiency. */
- PortalSetResultFormat(portal, 1, &format);
-
- /*
- * Tuples returned by any command other than the last are simply
- * discarded; but those returned by the last (or only) command
are
- * redirected to the shared memory queue we're using for
communication
- * with the launching backend. If the launching backend is gone
or has
- * detached us, these messages will just get dropped on the
floor.
- */
- --commands_remaining;
- if (commands_remaining > 0)
- receiver = CreateDestReceiver(DestNone);
- else
- {
- receiver = CreateDestReceiver(DestRemote);
- SetRemoteDestReceiverParams(receiver, portal);
- }
-
- /*
- * Only once the portal and destreceiver have been established
can
- * we return to the transaction context. All that stuff needs
to
- * survive an internal commit inside PortalRun!
- */
- MemoryContextSwitchTo(oldcontext);
-
- /* Here's where we actually execute the command. */
- (void) PortalRun(portal, FETCH_ALL, isTopLevel, receiver,
receiver,
- completionTag);
-
- /* Clean up the receiver. */
- (*receiver->rDestroy) (receiver);
-
- /* Clean up the portal. */
- PortalDrop(portal, false);
-
- /*
- * If this is the last parsetree, close down transaction
statement
- * before reporting CommandComplete. Otherwise, we need a
- * CommandCounterIncrement.
- */
- if (lnext(lc1) == NULL)
- finish_xact_command();
- else
- CommandCounterIncrement();
-
- /*
- * Send a CommandComplete message even if we suppressed the
query
- * results. The user backend will report the command tags in
the
- * absence of any true query results.
- */
- EndCommand(completionTag, DestRemote);
- }
-
- /* Make sure there's not still a transaction open. */
- finish_xact_command();
+ exec_query_string(query_string,
+ DestRemote,
+ &parsecontext,
+ true, /* allow_transactions */
+ true /* last_result_only */
+ );
}
/*
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index e44f5fa..3b998fc 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -823,13 +823,7 @@ static void
exec_simple_query(const char *query_string)
{
CommandDest dest = whereToSendOutput;
- MemoryContext oldcontext;
- List *parsetree_list;
- ListCell *parsetree_item;
bool save_log_statement_stats = log_statement_stats;
- bool was_logged = false;
- bool isTopLevel;
- char msec_str[32];
/*
@@ -849,6 +843,46 @@ exec_simple_query(const char *query_string)
ResetUsage();
/*
+ * Do the real work
+ */
+ exec_query_string(query_string,
+ dest,
+ MessageContext,
+ true, /* allow_transactions */
+ false /* last_result_only */
+ );
+
+ if (save_log_statement_stats)
+ ShowUsage("QUERY STATISTICS");
+
+ TRACE_POSTGRESQL_QUERY_DONE(query_string);
+
+ debug_query_string = NULL;
+}
+
+/*
+ * exec_query_string
+ *
+ * This is the guts of exec_simply_query, but is also used by
+ * execute_sql_string in contrib/pg_background
+ */
+void
+exec_query_string(const char *query_string,
+ CommandDest dest, /* Where to send output */
+ MemoryContext parsecontext, /* Context we
should use to parse query_string */
+ bool allow_transactions, /* Should we allow
transaction statements? */
+ bool last_result_only /* If true, drop all
command output except for the last command */
+ )
+{
+ MemoryContext oldcontext;
+ List *parsetree_list;
+ ListCell *parsetree_item;
+ bool was_logged = false;
+ bool isTopLevel;
+ char msec_str[32];
+ int commands_remaining;
+
+ /*
* Start up a transaction command. All queries generated by the
* query_string will be in this same command block, *unless* we find a
* BEGIN/COMMIT/ABORT statement; we have to force a new xact command
after
@@ -875,6 +909,7 @@ exec_simple_query(const char *query_string)
* we are in aborted transaction state!)
*/
parsetree_list = pg_parse_query(query_string);
+ commands_remaining = list_length(parsetree_list);
/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
@@ -915,6 +950,13 @@ exec_simple_query(const char *query_string)
DestReceiver *receiver;
int16 format;
+ if (!allow_transactions && IsA(parsetree, TransactionStmt))
+ /* XXX this needs to be more flexible */
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("transaction control statements
are not allowed in pg_background")));
+
+
/*
* Get the command name for use in status display (it also
becomes the
* default completion tag, down inside PortalRun). Set
ps_status and
@@ -925,6 +967,7 @@ exec_simple_query(const char *query_string)
set_ps_display(commandTag, false);
+ /* XXX what should we do here in the case of last_result_only =
true? */
BeginCommand(commandTag, dest);
/*
@@ -944,7 +987,8 @@ exec_simple_query(const char *query_string)
errdetail_abort()));
/* Make sure we are in a transaction command */
- start_xact_command();
+ if (allow_transactions)
+ start_xact_command();
/* If we got a cancel signal in parsing or prior command, quit
*/
CHECK_FOR_INTERRUPTS();
@@ -1027,10 +1071,19 @@ exec_simple_query(const char *query_string)
/*
* Now we can create the destination receiver object.
+ *
+ * If we sending the last result only, throw output away unless
it's
+ * the last command.
*/
- receiver = CreateDestReceiver(dest);
- if (dest == DestRemote)
- SetRemoteDestReceiverParams(receiver, portal);
+ --commands_remaining;
+ if (last_result_only && commands_remaining > 0)
+ receiver = CreateDestReceiver(DestNone);
+ else
+ {
+ receiver = CreateDestReceiver(dest);
+ if (dest == DestRemote)
+ SetRemoteDestReceiverParams(receiver, portal);
+ }
/*
* Switch back to transaction context for execution.
@@ -1055,7 +1108,9 @@ exec_simple_query(const char *query_string)
{
/*
* If this was a transaction control statement, commit
it. We will
- * start a new xact command for the next command (if
any).
+ * start a new xact command for the next command (if
any). Note
+ * that if allow_transactions is false then we should
never get to
+ * this point.
*/
finish_xact_command();
}
@@ -1121,12 +1176,6 @@ exec_simple_query(const char *query_string)
break;
}
- if (save_log_statement_stats)
- ShowUsage("QUERY STATISTICS");
-
- TRACE_POSTGRESQL_QUERY_DONE(query_string);
-
- debug_query_string = NULL;
}
/*
diff --git a/src/include/tcop/tcopprot.h b/src/include/tcop/tcopprot.h
index fd3df58..39c6a39 100644
--- a/src/include/tcop/tcopprot.h
+++ b/src/include/tcop/tcopprot.h
@@ -86,4 +86,11 @@ extern bool set_plan_disabling_options(const char *arg,
GucContext context,
GucSource source);
extern const char *get_stats_option_name(const char *arg);
+extern void exec_query_string(const char *query_string,
+ CommandDest dest, /* Where to send output */
+ MemoryContext parsecontext, /* Context we
should use to parse query_string */
+ bool allow_transactions, /* Should we allow
transaction statements? */
+ bool last_result_only /* If true, drop all
command output except for the last command */
+ );
+
#endif /* TCOPPROT_H */
--
2.1.2
/*
* exec_query_string
*
* This is the guts of exec_simply_query, but is also used by
* execute_sql_string in contrib/pg_background
*/
void
exec_query_string(const char *query_string,
CommandDest dest, /* Where to send output */
MemoryContext parsecontext, /* Context we
should use to parse query_string */
bool allow_transactions, /* Should we allow
transaction statements? */
bool last_result_only /* If true, drop all
command output except for the last command */
)
{
MemoryContext oldcontext;
List *parsetree_list;
ListCell *parsetree_item;
bool was_logged = false;
bool isTopLevel;
char msec_str[32];
int commands_remaining;
/*
* Start up a transaction command. All queries generated by the
* query_string will be in this same command block, *unless* we find a
* BEGIN/COMMIT/ABORT statement; we have to force a new xact command
after
* one of those, else bad things will happen in xact.c. (Note that this
* will normally change current memory context.)
*/
start_xact_command();
/*
* Zap any pre-existing unnamed statement. (While not strictly
necessary,
* it seems best to define simple-Query mode as if it used the unnamed
* statement and portal; this ensures we recover any storage used by
prior
* unnamed operations.)
*/
drop_unnamed_stmt();
/*
* Switch to appropriate context for constructing parsetrees.
*/
oldcontext = MemoryContextSwitchTo(MessageContext);
/*
* Do basic parsing of the query or queries (this should be safe even if
* we are in aborted transaction state!)
*/
parsetree_list = pg_parse_query(query_string);
commands_remaining = list_length(parsetree_list);
/* Log immediately if dictated by log_statement */
if (check_log_statement(parsetree_list))
{
ereport(LOG,
(errmsg("statement: %s", query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));
was_logged = true;
}
/*
* Switch back to transaction context to enter the loop.
*/
MemoryContextSwitchTo(oldcontext);
/*
* We'll tell PortalRun it's a top-level command iff there's exactly one
* raw parsetree. If more than one, it's effectively a transaction
block
* and we want PreventTransactionChain to reject unsafe commands. (Note:
* we're assuming that query rewrite cannot add commands that are
* significant to PreventTransactionChain.)
*/
isTopLevel = (list_length(parsetree_list) == 1);
/*
* Run through the raw parsetree(s) and process each one.
*/
foreach(parsetree_item, parsetree_list)
{
Node *parsetree = (Node *) lfirst(parsetree_item);
bool snapshot_set = false;
const char *commandTag;
char completionTag[COMPLETION_TAG_BUFSIZE];
List *querytree_list,
*plantree_list;
Portal portal;
DestReceiver *receiver;
int16 format;
if (!allow_transactions && IsA(parsetree, TransactionStmt))
/* XXX this needs to be more flexible */
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("transaction control statements
are not allowed in pg_background")));
/*
* Get the command name for use in status display (it also
becomes the
* default completion tag, down inside PortalRun). Set
ps_status and
* do any special start-of-SQL-command processing needed by the
* destination.
*/
commandTag = CreateCommandTag(parsetree);
set_ps_display(commandTag, false);
/* XXX what should we do here in the case of last_result_only =
true? */
BeginCommand(commandTag, dest);
/*
* If we are in an aborted transaction, reject all commands
except
* COMMIT/ABORT. It is important that this test occur before
we try
* to do parse analysis, rewrite, or planning, since all those
phases
* try to do database accesses, which may fail in abort state.
(It
* might be safe to allow some additional utility commands in
this
* state, but not many...)
*/
if (IsAbortedTransactionBlockState() &&
!IsTransactionExitStmt(parsetree))
ereport(ERROR,
(errcode(ERRCODE_IN_FAILED_SQL_TRANSACTION),
errmsg("current transaction is
aborted, "
"commands ignored until end
of transaction block"),
errdetail_abort()));
/* Make sure we are in a transaction command */
if (allow_transactions)
start_xact_command();
/* If we got a cancel signal in parsing or prior command, quit
*/
CHECK_FOR_INTERRUPTS();
/*
* Set up a snapshot if parse analysis/planning will need one.
*/
if (analyze_requires_snapshot(parsetree))
{
PushActiveSnapshot(GetTransactionSnapshot());
snapshot_set = true;
}
/*
* OK to analyze, rewrite, and plan this query.
*
* Switch to appropriate context for constructing querytrees
(again,
* these must outlive the execution context).
*/
oldcontext = MemoryContextSwitchTo(MessageContext);
querytree_list = pg_analyze_and_rewrite(parsetree, query_string,
NULL, 0);
plantree_list = pg_plan_queries(querytree_list, 0, NULL);
/* Done with the snapshot used for parsing/planning */
if (snapshot_set)
PopActiveSnapshot();
/* If we got a cancel signal in analysis or planning, quit */
CHECK_FOR_INTERRUPTS();
/*
* Create unnamed portal to run the query or queries in. If
there
* already is one, silently drop it.
*/
portal = CreatePortal("", true, true);
/* Don't display the portal in pg_cursors */
portal->visible = false;
/*
* We don't have to copy anything into the portal, because
everything
* we are passing here is in MessageContext, which will outlive
the
* portal anyway.
*/
PortalDefineQuery(portal,
NULL,
query_string,
commandTag,
plantree_list,
NULL);
/*
* Start the portal. No parameters here.
*/
PortalStart(portal, NULL, 0, InvalidSnapshot);
/*
* Select the appropriate output format: text unless we are
doing a
* FETCH from a binary cursor. (Pretty grotty to have to do
this here
* --- but it avoids grottiness in other places. Ah, the joys
of
* backward compatibility...)
*/
format = 0; /* TEXT is default */
if (IsA(parsetree, FetchStmt))
{
FetchStmt *stmt = (FetchStmt *) parsetree;
if (!stmt->ismove)
{
Portal fportal =
GetPortalByName(stmt->portalname);
if (PortalIsValid(fportal) &&
(fportal->cursorOptions &
CURSOR_OPT_BINARY))
format = 1; /* BINARY */
}
}
PortalSetResultFormat(portal, 1, &format);
/*
* Now we can create the destination receiver object.
*
* If we sending the last result only, throw output away unless
it's
* the last command.
*/
--commands_remaining;
if (last_result_only && commands_remaining > 0)
receiver = CreateDestReceiver(DestNone);
else
{
receiver = CreateDestReceiver(dest);
if (dest == DestRemote)
SetRemoteDestReceiverParams(receiver, portal);
}
/*
* Switch back to transaction context for execution.
*/
MemoryContextSwitchTo(oldcontext);
/*
* Run the portal to completion, and then drop it (and the
receiver).
*/
(void) PortalRun(portal,
FETCH_ALL,
isTopLevel,
receiver,
receiver,
completionTag);
(*receiver->rDestroy) (receiver);
PortalDrop(portal, false);
if (IsA(parsetree, TransactionStmt))
{
/*
* If this was a transaction control statement, commit
it. We will
* start a new xact command for the next command (if
any). Note
* that if allow_transactions is false then we should
never get to
* this point.
*/
finish_xact_command();
}
else if (lnext(parsetree_item) == NULL)
{
/*
* If this is the last parsetree of the query string,
close down
* transaction statement before reporting
command-complete. This
* is so that any end-of-transaction errors are
reported before
* the command-complete message is issued, to avoid
confusing
* clients who will expect either a command-complete
message or an
* error, not one and then the other. But for
compatibility with
* historical Postgres behavior, we do not force a
transaction
* boundary between queries appearing in a single query
string.
*/
finish_xact_command();
}
else
{
/*
* We need a CommandCounterIncrement after every query,
except
* those that start or end a transaction block.
*/
CommandCounterIncrement();
}
/*
* Tell client that we're done with this query. Note we emit
exactly
* one EndCommand report for each raw parsetree, thus one for
each SQL
* command the client sent, regardless of rewriting. (But a
command
* aborted by error will not send an EndCommand report at all.)
*/
EndCommand(completionTag, dest);
} /* end loop
over parsetrees */
/*
* Close down transaction statement, if one is open.
*/
finish_xact_command();
/*
* If there were no parsetrees, return EmptyQueryResponse message.
*/
if (!parsetree_list)
NullCommand(dest);
/*
* Emit duration logging if appropriate.
*/
switch (check_log_duration(msec_str, was_logged))
{
case 1:
ereport(LOG,
(errmsg("duration: %s ms", msec_str),
errhidestmt(true)));
break;
case 2:
ereport(LOG,
(errmsg("duration: %s ms statement:
%s",
msec_str, query_string),
errhidestmt(true),
errdetail_execute(parsetree_list)));
break;
}
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
