Thread: We need to support ForeignRecheck for late row locking, don't we?
While working on the issue "Foreign join pushdown vs EvalPlanQual", I happened to notice odd behaviors of late row locking in FDWs. An example will be shown below using the attached postgres_fdw patch, which is basically the same as [1], but I changed its isolation level to READ COMMITTED and modified it so that the output parameter "updated" of RefetchForeignRow is updated accordingly. [Create an environment] mydatabase=# create table mytable (a int); mydatabase=# insert into mytable values (1); postgres=# create extension postgres_fdw; postgres=# create server myserver foreign data wrapper postgres_fdw options (dbname 'mydatabase'); postgres=# create user mapping for current_user server myserver; postgres=# create foreign table ftable (a int) server myserver options (table_name 'mytable'); [Run concurrent transactions that behave oddly] mydatabase=# begin; mydatabase=# update mytable set a = a + 1; postgres=# select * from ftable where a = 1 for update; mydatabase=# commit; (After the commit, the following result will be shown in the terminal for the postgres database. Note that the result doesn't satify a = 1.) a --- 2 (1 row) I think the reason for that is because we don't check pushed-down quals inside an EPQ testing even if what was fetched by RefetchForeignRow was an updated version of the tuple rather than the same version previously obtained. So, to fix this, I'd like to propose that pushed-down quals be checked in ForeignRecheck. Comments welcome! Best regards, Etsuro Fujita [1] http://www.postgresql.org/message-id/16016.1431455074@sss.pgh.pa.us
Attachment
On 2015/07/22 19:10, Etsuro Fujita wrote: > While working on the issue "Foreign join pushdown vs EvalPlanQual", I > happened to notice odd behaviors of late row locking in FDWs. > I think the reason for that is because we don't check pushed-down quals > inside an EPQ testing even if what was fetched by RefetchForeignRow was > an updated version of the tuple rather than the same version previously > obtained. So, to fix this, I'd like to propose that pushed-down quals > be checked in ForeignRecheck. Attached is a patch for that. * I've modified ForeignRecheck so as to check pushed-down quals whether doing late locking or early locking. I think we could probably make ForeignRecheck do so only when doing late locking, but I'm not sure it's worth complicating the code. * I've made the above change only for simple foreign table scans that have scanrelid > 0 and fdw_scan_tlist = NIL. As for simple foreign table scans that have scanrelid > 0 and *fdw_scan_tlist is non-NIL*, I think we are under discussion in another thread I started. Will update as necessary. * Sorry, I've not fully updated comments and docs yet. Will update. I'd be happy if I could get feedback earlier. Best regards, Etsuro Fujita
Attachment
Fujita-san, > On 2015/07/22 19:10, Etsuro Fujita wrote: > > While working on the issue "Foreign join pushdown vs EvalPlanQual", I > > happened to notice odd behaviors of late row locking in FDWs. > > > I think the reason for that is because we don't check pushed-down quals > > inside an EPQ testing even if what was fetched by RefetchForeignRow was > > an updated version of the tuple rather than the same version previously > > obtained. So, to fix this, I'd like to propose that pushed-down quals > > be checked in ForeignRecheck. > > Attached is a patch for that. > > * I've modified ForeignRecheck so as to check pushed-down quals whether > doing late locking or early locking. I think we could probably make > ForeignRecheck do so only when doing late locking, but I'm not sure it's > worth complicating the code. > > * I've made the above change only for simple foreign table scans that > have scanrelid > 0 and fdw_scan_tlist = NIL. As for simple foreign > table scans that have scanrelid > 0 and *fdw_scan_tlist is non-NIL*, I > think we are under discussion in another thread I started. Will update > as necessary. > > * Sorry, I've not fully updated comments and docs yet. Will update. > > I'd be happy if I could get feedback earlier. > Isn't it an option to put a new callback in ForeignRecheck? FDW driver knows its private data structure includes expression node that was pushed down to the remote side. So, it seems to me the best way to consult FDW driver whether the supplied tuple should be visible according to the pushed down qualifier. More or less, this fix need a new interface contract around EvalPlanQual logic. It is better to give FDW driver more flexibility of its private data structure and the way to process recheck logic, rather than special purpose variable. If FDW driver managed pushed-down expression in its own format, requirement to pushedDownQual makes them to have qualifier redundantly. The callback approach does not have such kind of concern. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei <kaigai@ak.jp.nec.com>
KaiGai-san, On 2015/07/24 23:51, Kouhei Kaigai wrote: >> On 2015/07/22 19:10, Etsuro Fujita wrote: >>> While working on the issue "Foreign join pushdown vs EvalPlanQual", I >>> happened to notice odd behaviors of late row locking in FDWs. >> >>> I think the reason for that is because we don't check pushed-down quals >>> inside an EPQ testing even if what was fetched by RefetchForeignRow was >>> an updated version of the tuple rather than the same version previously >>> obtained. So, to fix this, I'd like to propose that pushed-down quals >>> be checked in ForeignRecheck. >> * I've modified ForeignRecheck so as to check pushed-down quals whether >> doing late locking or early locking. > Isn't it an option to put a new callback in ForeignRecheck? > > FDW driver knows its private data structure includes expression node > that was pushed down to the remote side. So, it seems to me the best > way to consult FDW driver whether the supplied tuple should be visible > according to the pushed down qualifier. > > More or less, this fix need a new interface contract around EvalPlanQual > logic. It is better to give FDW driver more flexibility of its private > data structure and the way to process recheck logic, rather than special > purpose variable. > > If FDW driver managed pushed-down expression in its own format, requirement > to pushedDownQual makes them to have qualifier redundantly. > The callback approach does not have such kind of concern. That might be an idea, but is there any performance disadvantage as discussed in [1]?; it looks like that that needs to perform another remote query to see if the supplied tuple satisfies the pushed-down quals during EPQ testing. Best regards, Etsuro Fujita [1] http://www.postgresql.org/message-id/5590ED5C.2040200@lab.ntt.co.jp
> On 2015/07/24 23:51, Kouhei Kaigai wrote: > >> On 2015/07/22 19:10, Etsuro Fujita wrote: > >>> While working on the issue "Foreign join pushdown vs EvalPlanQual", I > >>> happened to notice odd behaviors of late row locking in FDWs. > >> > >>> I think the reason for that is because we don't check pushed-down quals > >>> inside an EPQ testing even if what was fetched by RefetchForeignRow was > >>> an updated version of the tuple rather than the same version previously > >>> obtained. So, to fix this, I'd like to propose that pushed-down quals > >>> be checked in ForeignRecheck. > > >> * I've modified ForeignRecheck so as to check pushed-down quals whether > >> doing late locking or early locking. > > > Isn't it an option to put a new callback in ForeignRecheck? > > > > FDW driver knows its private data structure includes expression node > > that was pushed down to the remote side. So, it seems to me the best > > way to consult FDW driver whether the supplied tuple should be visible > > according to the pushed down qualifier. > > > > More or less, this fix need a new interface contract around EvalPlanQual > > logic. It is better to give FDW driver more flexibility of its private > > data structure and the way to process recheck logic, rather than special > > purpose variable. > > > > If FDW driver managed pushed-down expression in its own format, requirement > > to pushedDownQual makes them to have qualifier redundantly. > > The callback approach does not have such kind of concern. > > That might be an idea, but is there any performance disadvantage as > discussed in [1]?; it looks like that that needs to perform another > remote query to see if the supplied tuple satisfies the pushed-down > quals during EPQ testing. > I expect the callback of ForeignRecheck runs ExecQual() towards the qualifier expression pushed-down but saved on the private data of ForeignScanState. It does not need to kick another remote query (unless FDW driver is not designed), so performance disadvantage is none or quite limited. Also, let's assume the case when scanrelid == 0 (join pushdown). It is easy to put special code path if scanrelid == 0, that implies ScanState is either ForeignScan or CustomScan. If ForeignRecheck (= recheckMtd) is called instead of the if- block below of the Assert() on ExecScanFetch, FDW driver will be able to put its own special code path to run alternative sub-plan. How this alternative sub-plan works? It walks down the sub-plan tree that is typically consists of NestLoop + ForeignScan for example, then ExecScanFetch() is called again towards ScanState with scanrelid > 0 at that time. Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei <kaigai@ak.jp.nec.com>
On 2015/07/27 18:16, Kouhei Kaigai wrote: >> On 2015/07/24 23:51, Kouhei Kaigai wrote: >>>> On 2015/07/22 19:10, Etsuro Fujita wrote: >>>>> While working on the issue "Foreign join pushdown vs EvalPlanQual", I >>>>> happened to notice odd behaviors of late row locking in FDWs. >>>> >>>>> I think the reason for that is because we don't check pushed-down quals >>>>> inside an EPQ testing even if what was fetched by RefetchForeignRow was >>>>> an updated version of the tuple rather than the same version previously >>>>> obtained. So, to fix this, I'd like to propose that pushed-down quals >>>>> be checked in ForeignRecheck. >> >>>> * I've modified ForeignRecheck so as to check pushed-down quals whether >>>> doing late locking or early locking. >> >>> Isn't it an option to put a new callback in ForeignRecheck? >>> >>> FDW driver knows its private data structure includes expression node >>> that was pushed down to the remote side. So, it seems to me the best >>> way to consult FDW driver whether the supplied tuple should be visible >>> according to the pushed down qualifier. >>> >>> More or less, this fix need a new interface contract around EvalPlanQual >>> logic. It is better to give FDW driver more flexibility of its private >>> data structure and the way to process recheck logic, rather than special >>> purpose variable. >>> >>> If FDW driver managed pushed-down expression in its own format, requirement >>> to pushedDownQual makes them to have qualifier redundantly. >>> The callback approach does not have such kind of concern. >> >> That might be an idea, but is there any performance disadvantage as >> discussed in [1]?; it looks like that that needs to perform another >> remote query to see if the supplied tuple satisfies the pushed-down >> quals during EPQ testing. >> > I expect the callback of ForeignRecheck runs ExecQual() towards > the qualifier expression pushed-down but saved on the private data > of ForeignScanState. It does not need to kick another remote query > (unless FDW driver is not designed), so performance disadvantage is > none or quite limited. The advantages look not clear to me. I think the callback approach would be a good idea if FDWs were able to do the recheck more efficiently in their own ways than the core, for example. Best regards, Etsuro Fujita