Re: Parallel Seq Scan - Mailing list pgsql-hackers
| From | Amit Kapila |
|---|---|
| Subject | Re: Parallel Seq Scan |
| Date | |
| Msg-id | CAA4eK1JHtDM1yMnCSCb2G2zVuXFsu1YAsg_v=3KL++vwwvgxWA@mail.gmail.com Whole thread Raw |
| In response to | Re: Parallel Seq Scan (Kouhei Kaigai <kaigai@ak.jp.nec.com>) |
| List | pgsql-hackers |
On Wed, Sep 23, 2015 at 6:42 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote:
>
> > > On Thu, Sep 17, 2015 at 4:44 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> > To verify the patch, I have done 2 things, first I have added elog to
> > the newly supported read funcs and then in planner, I have used
> > nodeToString and stringToNode on planned_stmt and then used the
> > newly generated planned_stmt for further execution. After making these
> > changes, I have ran make check-world and ensures that it covers all the
> > newly added nodes.
> >
> > Note, that as we don't populate funcid's in expressions during read, the
> > same has to be updated by traversing the tree and updating in different
> > expressions based on node type. Attached patch (read_funcs_test_v1)
> > contains the changes required for testing the patch. I am not very sure
> > about what do about some of the ForeignScan fields (fdw_private) in order
> > to update the funcid as the data in those expressions could be FDW specific.
> > This is anyway for test, so doesn't matter much, but the same will be
> > required to support read of ForeignScan node by worker.
> >
> Because of interface contract, it is role of FDW driver to put nodes which
> are safe to copyObject on fdw_exprs and fdw_private field. Unless FDW driver
> does not violate, fdw_exprs and fdw_private shall be reproduced on the worker
> side. (Of course, we cannot guarantee nobody has local pointer on private
> field...)
> Sorry, I cannot understand the sentence of funcid population. It seems to me
> funcid is displayed as-is, and _readFuncExpr() does nothing special...?
>
>
> > > On Thu, Sep 17, 2015 at 4:44 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> > To verify the patch, I have done 2 things, first I have added elog to
> > the newly supported read funcs and then in planner, I have used
> > nodeToString and stringToNode on planned_stmt and then used the
> > newly generated planned_stmt for further execution. After making these
> > changes, I have ran make check-world and ensures that it covers all the
> > newly added nodes.
> >
> > Note, that as we don't populate funcid's in expressions during read, the
> > same has to be updated by traversing the tree and updating in different
> > expressions based on node type. Attached patch (read_funcs_test_v1)
> > contains the changes required for testing the patch. I am not very sure
> > about what do about some of the ForeignScan fields (fdw_private) in order
> > to update the funcid as the data in those expressions could be FDW specific.
> > This is anyway for test, so doesn't matter much, but the same will be
> > required to support read of ForeignScan node by worker.
> >
> Because of interface contract, it is role of FDW driver to put nodes which
> are safe to copyObject on fdw_exprs and fdw_private field. Unless FDW driver
> does not violate, fdw_exprs and fdw_private shall be reproduced on the worker
> side. (Of course, we cannot guarantee nobody has local pointer on private
> field...)
> Sorry, I cannot understand the sentence of funcid population. It seems to me
> funcid is displayed as-is, and _readFuncExpr() does nothing special...?
>
Here I am referring to operator's funcid (refer function _readOpExpr()). It is
hard-coded to InvalidOid during read. Currently fix_opfuncids() is used to
fill the funcid for expressions, now I am not sure if it can be used for
fdw_private data (I have tried it, but it was failing, may be it is not at all
required to fix any funcid for it) or other fields in Foreign Scan.
I have observed that in out functions, we output fdw_private field which
indicates that ideally we should be able to read it.
>
> Robert Haas said:
> > 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.
> >
> Like this?
> https://github.com/kaigai/sepgsql/blob/readfuncs/src/backend/nodes/readfuncs.c#L133
>
> I think outfuncs.c also have similar macro to centralize the format of array.
> Actually, most of boolean array are displayed using booltostr(), however, only
> _outMergeJoin() uses "%d" format to display boolean as integer.
> It is a bit inconsistent manner.
>
Yes, I have also noticed the same and thought of sending a patch which I
> Robert Haas said:
> > 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.
> >
> Like this?
> https://github.com/kaigai/sepgsql/blob/readfuncs/src/backend/nodes/readfuncs.c#L133
>
> I think outfuncs.c also have similar macro to centralize the format of array.
> Actually, most of boolean array are displayed using booltostr(), however, only
> _outMergeJoin() uses "%d" format to display boolean as integer.
> It is a bit inconsistent manner.
>
Yes, I have also noticed the same and thought of sending a patch which I
have done just before sending this mail.
pgsql-hackers by date: