Re: Parallel Seq Scan

On Wed, Sep 23, 2015 at 5:42 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>
> On Tue, Sep 22, 2015 at 3:21 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com