On Wed, Sep 23, 2015 at 5:42 AM, Robert Haas <
robertmhaas@gmail.com> wrote:
>
> 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.
>
Updated.
> Doesn't _readCommonPlan() leak?
I have tried that way to keep the code simple with a view that this will
not be read in long-lived memory context, however your suggestion
below makes sense and can avoid it.
> 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.
>
Changed as per suggestion.
> Instead of passing the Plan down by casting, how about passing
> &local_node->plan? And similarly for scans and joins.
>
Changed as per suggestion.
> readAttrNumberCols uses sizeof(Oid) instead of sizeof(AttrNumber).
>
Fixed.
> 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.
>
As discussed below this is required and I haven't changed it.
> 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.
>
Changed as per suggestion.
> 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?
>
I have kept them in order they appear in nodes.h (that way it seems easier
to keep track if anything is missed), however if you want I can reorder them
as per your suggestion.
Note - Test is changed to verify just the output of readfuncs with changes in
planner. I have removed elog related changes in readfuncs, as during last
test, I have verified that make check-world covers all types of nodes that
are added by patch.