Обсуждение: 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

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

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