On Sun, Nov 15, 2009 at 14:27, Marko Tiikkaja
<marko.tiikk...@cs.helsinki.fi> wrote:
> I wrote:
>>
>> Attached is the latest version of this patch.
Find attached a incremental diff with the following changes:
-get rid of operation argument to InitResultRelInfo, its unused now
-add some asserts to make sure places we use subplanstate now that it
can be null
(*note* AFAICT its a cant happen, but it made me nervous hence the Asserts)
-remove unneeded plannodes.h includes
-minor whitespace fix
Other comments:
You have an "XXX we should probably update the snapshot a bit
differently". Any plans on that?
Thats quite a bit of new code in ExecutePlan, worth splitting into its
own function?
Also, after reading through the previous threads; it was not
immediately obvious that you dealt with
http://archives.postgresql.org/pgsql-hackers/2009-10/msg00566.php by
only allowing selects or values at the top level of with.
Find below the standard review boilerplate from
http://wiki.postgresql.org/wiki/Reviewing_a_Patch
Summary: looks ready for a commiter to me after above comments are addressed.
Submission review:
*Is the patch in context diff format?
Yes
* Does it apply cleanly to the current CVS HEAD?
Yes, with fuzz
* Does it include reasonable tests, necessary doc patches, etc?
Yes
Usability review:
Read what the patch is supposed to do, and consider:
* Does the patch actually implement that?
Yes
* Do we want that?
Yes
* Do we already have it?
No
* Does it follow SQL spec, or the community-agreed behavior?
Yes
* Does it include pg_dump support (if applicable)?
N/A
* Are there dangers?
No
* Have all the bases been covered?
All the ones I can see
Feature test:
Apply the patch, compile it and test:
* Does the feature work as advertised?
Yes
* Are there corner cases the author has failed to consider?
Not that I could trigger
* Are there any assertion failures or crashes?
No
o Review should be done with the configure options --enable-cassert
and --enable-debug turned on;
Yes
Performance review:
*Does the patch slow down simple tests:
No
*If it claims to improve performance, does it?
N/A
*Does it slow down other things
No
Coding review:
Read the changes to the code in detail and consider:
* Does it follow the project coding guidelines?
Yes
* Are there portability issues?
No
* Will it work on Windows/BSD etc?
Yes
* Are the comments sufficient and accurate?
Yes
* Does it do what it says, correctly?
Yes
* Does it produce compiler warnings?
No
* Can you make it crash?
No
Architecture review:
Consider the changes to the code in the context of the project as a whole:
* Is everything done in a way that fits together coherently with
other features/modules?
I think so.
* Are there interdependencies than can cause problems?
No
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***************
*** 925,931 **** ExecuteTruncate(TruncateStmt *stmt)
InitResultRelInfo(resultRelInfo,
rel,
0, /* dummy rangetable index */
- CMD_DELETE, /* don't need any index info */
false);
resultRelInfo++;
}
--- 925,930 ----
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***************
*** 665,671 **** InitPlan(QueryDesc *queryDesc, int eflags)
InitResultRelInfo(resultRelInfo,
resultRelation,
resultRelationIndex,
- operation,
estate->es_instrument);
resultRelInfo++;
}
--- 665,670 ----
***************
*** 857,863 **** void
InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- CmdType operation,
bool doInstrument)
{
/*
--- 856,861 ----
***************
*** 987,993 **** ExecGetTriggerResultRel(EState *estate, Oid relid)
InitResultRelInfo(rInfo,
rel,
0, /* dummy rangetable index */
- CMD_DELETE,
estate->es_instrument);
estate->es_trig_target_relations =
lappend(estate->es_trig_target_relations, rInfo);
--- 985,990 ----
*** a/src/backend/executor/nodeSubplan.c
--- b/src/backend/executor/nodeSubplan.c
***************
*** 667,672 **** ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
--- 667,673 ----
/* Link the SubPlanState to already-initialized subplan */
sstate->planstate = (PlanState *) list_nth(estate->es_subplanstates,
subplan->plan_id - 1);
+ Assert(sstate->planstate != NULL);
/* Initialize subexpressions */
sstate->testexpr = ExecInitExpr((Expr *) subplan->testexpr, parent);
*** a/src/backend/parser/parse_cte.c
--- b/src/backend/parser/parse_cte.c
***************
*** 18,24 ****
#include "nodes/nodeFuncs.h"
#include "parser/analyze.h"
#include "parser/parse_cte.h"
- #include "nodes/plannodes.h"
#include "utils/builtins.h"
--- 18,23 ----
*** a/src/backend/parser/parse_relation.c
--- b/src/backend/parser/parse_relation.c
***************
*** 24,30 ****
#include "funcapi.h"
#include "nodes/makefuncs.h"
#include "nodes/nodeFuncs.h"
- #include "nodes/plannodes.h"
#include "parser/parsetree.h"
#include "parser/parse_relation.h"
#include "parser/parse_type.h"
--- 24,29 ----
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
***************
*** 1842,1854 **** RewriteQuery(Query *parsetree, List *rewrite_events)
cte->ctequery = (Node *) lfirst(lc);
else
rewritten = lcons((void *) lfirst(lc), rewritten);
-
n++;
}
}
else
{
cte->ctequery = (Node *) linitial(newstuff);
rewritten = list_concat(rewritten,
list_delete_first(newstuff));
--- 1842,1855 ----
cte->ctequery = (Node *) lfirst(lc);
else
rewritten = lcons((void *) lfirst(lc), rewritten);
n++;
}
}
else
{
+ Assert(list_length(newstuff) == 1);
+
cte->ctequery = (Node *) linitial(newstuff);
rewritten = list_concat(rewritten,
list_delete_first(newstuff));
*** a/src/include/executor/executor.h
--- b/src/include/executor/executor.h
***************
*** 160,166 **** extern void ExecutorRewind(QueryDesc *queryDesc);
extern void InitResultRelInfo(ResultRelInfo *resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
- CmdType operation,
bool doInstrument);
extern ResultRelInfo *ExecGetTriggerResultRel(EState *estate, Oid relid);
extern bool ExecContextForcesOids(PlanState *planstate, bool *hasoids);
--- 160,165 ----
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
