On Tue, Sep 22, 2015 at 3:21 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> Attached patch (read_funcs_v1.patch) contains support for all the plan
> and other nodes (like SubPlan which could be required for worker) except
> CustomScan node.
It looks like you need to update the top-of-file comment for outfuncs.c.
Doesn't _readCommonPlan() leak? I think we should avoid that.
_readCommonScan() and _readJoin() are worse: they leak multiple
objects. It should be simple enough to avoid this: just have your
helper function take a Plan * as argument and then use
READ_TEMP_LOCALS() rather than READ_LOCALS(). Then the caller can use
READ_LOCALS, call the helper to fill in all the Plan fields, and then
read the other stuff itself.
Instead of passing the Plan down by casting, how about passing
&local_node->plan? And similarly for scans and joins.
readAttrNumberCols uses sizeof(Oid) instead of sizeof(AttrNumber).
I still don't understand why we need to handle PlanInvalItem.
Actually, come to think of it, I'm not sure we need PlannedStmt
either. Let's leave those out; they seem like trouble.
I think it would be worth doing something like this:
#define READ_ATTRNUMBER_ARRAY(fldname, len) \ token = pg_strtok(&length); \ local_node->fldname =
readAttrNumberCols(len);
And similarly for READ_OID_ARRAY, READ_BOOL_ARRAY, READ_INT_ARRAY.
In general these routines are in the same order as plannodes.h, which
is good. But _readNestLoopParam is out of place. Can we move it just
after _readNestLoop?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
