COPY planning
I noticed that COPY calls planner() (this was introduced in 85188ab88).
I think it should be calling pg_plan_query() instead. The latter is a
very thin wrapper around the former which simply adds a couple of
logging entries, DTrace hooks for start/end, and a debugging cross-check
for plan node copying.
I came across this because I was considering adding some code to
pg_plan_query, so I would have needed to essentially duplicate it in the
COPY path, which seemed bad. (I have since abandoned the idea, but this
seems a reasonable thing to change nonetheless.)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index e98f0fe..94b2f8f 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1414,7 +1414,7 @@ BeginCopy(bool is_from,
Assert(query->utilityStmt == NULL); /* plan the query */
- plan = planner(query, 0, NULL);
+ plan = pg_plan_query(query, 0, NULL); /*
* With row level security and a user using "COPY relation TO", we
--
�lvaro Herrera 33.5S 70.5W
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
I noticed that COPY calls planner() (this was introduced in 85188ab88).
I think it should be calling pg_plan_query() instead.
+1 --- AFAICS, this is the *only* place that is going directly to
planner() without going through pg_plan_query(); other utility
functions such as CREATE TABLE AS do the latter.
As far as the patch goes, do copy.c's #include's need adjustment?
I'm wondering if optimizer/planner.h could be removed, in particular.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
I noticed that COPY calls planner() (this was introduced in 85188ab88).
I think it should be calling pg_plan_query() instead.+1 --- AFAICS, this is the *only* place that is going directly to
planner() without going through pg_plan_query(); other utility
functions such as CREATE TABLE AS do the latter.As far as the patch goes, do copy.c's #include's need adjustment?
I'm wondering if optimizer/planner.h could be removed, in particular.
BeginCopyFrom still uses expression_planner(), at least..
Thanks!
Stephen
Stephen Frost wrote:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
I noticed that COPY calls planner() (this was introduced in 85188ab88).
I think it should be calling pg_plan_query() instead.+1 --- AFAICS, this is the *only* place that is going directly to
planner() without going through pg_plan_query(); other utility
functions such as CREATE TABLE AS do the latter.
Yeah, exactly. Thus, pushed.
As far as the patch goes, do copy.c's #include's need adjustment?
I'm wondering if optimizer/planner.h could be removed, in particular.BeginCopyFrom still uses expression_planner(), at least..
Yup. However I notice that there are a few other callers of
expression_planner() that do not involve the optimizer for anything
else. Maybe it makes sense to have a separate header file that's just
#include "nodes/primnodes.h"
extern Expr *expression_planner(Expr *expr);
extern bool plan_cluster_use_sort(Oid tableOid, Oid indexOid);
Seems it could be used by a large percentage of files currently
including planner.h.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
Yup. However I notice that there are a few other callers of
expression_planner() that do not involve the optimizer for anything
else. Maybe it makes sense to have a separate header file that's just
#include "nodes/primnodes.h"
extern Expr *expression_planner(Expr *expr);
extern bool plan_cluster_use_sort(Oid tableOid, Oid indexOid);
Seems it could be used by a large percentage of files currently
including planner.h.
Hmm. I'd be a bit inclined to define such a file as "planner's
externally visible API", which would mean it should also include
planner() itself, and maybe also eval_const_expressions --- are
there external uses of that?
If we wanted to get rid of external uses of clauses.h, we'd probably
want to move some things like contain_mutable_functions() to nodeFuncs.c,
rather than put them into this hypothetical new header.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers