Re: Parallel Seq Scan

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.

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