Re: Parallel Seq Scan - Mailing list pgsql-hackers

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?
>