Thread: planstate_tree_walker oversight CustomScan
Hi, The planstate_tree_walker() oversight custom_ps of CustomScanState; that should be a list of underlying PlanState object if any. ExplainPreScanNode() treated ForeignScan and CustomScan in special way (it is sufficient for ExplainPreScanNode() purpose), thus, it didn't implement its recursive portion originally. The job of ExplainPreScanNode() is know all the relids involved in a particular subquery execution. On the other hands, fs_relids of ForeignScan and custom_relids of CustomScan informs a set of relids to be scanned by this Scan node without recursive, so it did not have recursive walks on the underlying sub-plans. However, planstate_tree_walker() will have different expectation. It is general walker routine, thus, it is natural users to expect the callback is also kicked towards the underlying planstate of CustomScan (and ForeignScan; once EPQ recheck gets solved). The attached patch adds support of CustomScan on the walker. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei <kaigai@ak.jp.nec.com>
Attachment
On Mon, Sep 21, 2015 at 9:54 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote: > The planstate_tree_walker() oversight custom_ps of CustomScanState; > that should be a list of underlying PlanState object if any. > > ExplainPreScanNode() treated ForeignScan and CustomScan in special > way (it is sufficient for ExplainPreScanNode() purpose), thus, it > didn't implement its recursive portion originally. > > The job of ExplainPreScanNode() is know all the relids involved > in a particular subquery execution. On the other hands, fs_relids > of ForeignScan and custom_relids of CustomScan informs a set of > relids to be scanned by this Scan node without recursive, so it > did not have recursive walks on the underlying sub-plans. > > However, planstate_tree_walker() will have different expectation. > It is general walker routine, thus, it is natural users to expect > the callback is also kicked towards the underlying planstate of > CustomScan (and ForeignScan; once EPQ recheck gets solved). > > The attached patch adds support of CustomScan on the walker. Do you need to add something to ExplainPreScanNode for this as well, like if (scanrelid != 0) *rels_used = bms_add_member(*rels_used, scanrelid)? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> -----Original Message----- > From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Robert Haas > Sent: Wednesday, September 23, 2015 10:15 AM > To: Kaigai Kouhei(海外 浩平) > Cc: pgsql-hackers@postgresql.org > Subject: Re: [HACKERS] planstate_tree_walker oversight CustomScan > > On Mon, Sep 21, 2015 at 9:54 AM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote: > > The planstate_tree_walker() oversight custom_ps of CustomScanState; > > that should be a list of underlying PlanState object if any. > > > > ExplainPreScanNode() treated ForeignScan and CustomScan in special > > way (it is sufficient for ExplainPreScanNode() purpose), thus, it > > didn't implement its recursive portion originally. > > > > The job of ExplainPreScanNode() is know all the relids involved > > in a particular subquery execution. On the other hands, fs_relids > > of ForeignScan and custom_relids of CustomScan informs a set of > > relids to be scanned by this Scan node without recursive, so it > > did not have recursive walks on the underlying sub-plans. > > > > However, planstate_tree_walker() will have different expectation. > > It is general walker routine, thus, it is natural users to expect > > the callback is also kicked towards the underlying planstate of > > CustomScan (and ForeignScan; once EPQ recheck gets solved). > > > > The attached patch adds support of CustomScan on the walker. > > Do you need to add something to ExplainPreScanNode for this as well, > like if (scanrelid != 0) *rels_used = bms_add_member(*rels_used, > scanrelid)? > The latest ExplainPreScanNode is sufficient. Regardless of scanrelid (even if it is zero), fs_relids and custom_relids shall be set properly to introduce which relations are represented by this ForeignScan and CustomScan node. So, additional planstate_tree_walker() call might be a bit redundant, but harmless. The reason why ForeignScan/CustomScan node have these bitmap is, we cannot guarantee they always have underlying scan node. For example, ForeignScan that kicks remote join query will not have local underlying scan node on the foreign tables to be involved. So, we had to inform ExplainPreScanNode which relids are represented by this Foreign/CustomScan node. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei <kaigai@ak.jp.nec.com>
On Tue, Sep 22, 2015 at 9:30 PM, Kouhei Kaigai <kaigai@ak.jp.nec.com> wrote: > The latest ExplainPreScanNode is sufficient. Regardless of scanrelid > (even if it is zero), fs_relids and custom_relids shall be set properly > to introduce which relations are represented by this ForeignScan and > CustomScan node. So, additional planstate_tree_walker() call might be > a bit redundant, but harmless. > > The reason why ForeignScan/CustomScan node have these bitmap is, we > cannot guarantee they always have underlying scan node. For example, > ForeignScan that kicks remote join query will not have local underlying > scan node on the foreign tables to be involved. > So, we had to inform ExplainPreScanNode which relids are represented > by this Foreign/CustomScan node. OK, I've just committed the patch the way it is for now, then. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company