Re: EvalPlanQual behaves oddly for FDW queries involving system columns

On 2015/03/12 13:35, Tom Lane wrote:
> I don't like the execMain.c changes much at all. They look somewhat
> like they're intended to allow foreign tables to adopt a different
> locking strategy,

I didn't intend such a thing. My intention is, foreign tables have
markType = ROW_MARK_COPY as ever, but I might not have correctly
understood what you pointed out. Could you elaborate on that?

> The changes are not all consistent
> either, eg this:
>
> ! if (erm->relation &&
> ! erm->relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE)
>
> is not necessary if this Assert is accurate:
>
> ! if (erm->relation)
> ! {
> ! Assert(erm->relation->rd_rel->relkind == RELKIND_FOREIGN_TABLE);

I modified InitPlan so that relations get opened for foreign tables
as well as local tables. So, I think the change would be necessary.

> I don't see the need for the change in nodeForeignscan.c. If the FDW has
> returned a physical tuple, it can fix that for itself, while if it has
> returned a virtual tuple, the ctid will be garbage in any case.

If you leave nodeForeignscan.c unchanged, file_fdw would cause the
problem that I pointed out upthread. Here is an example.

[Create a test environment]

postgres=# create foreign table foo (a int) server file_server options
(filename '/home/pgsql/foo.csv', format 'csv');
CREATE FOREIGN TABLE
postgres=# select tableoid, ctid, * from foo;
tableoid | ctid | a
----------+----------------+---
16459 | (4294967295,0) | 1
(1 row)

postgres=# create table tab (a int, b int);
CREATE TABLE
postgres=# insert into tab values (1, 1);
INSERT 0 1

[Run concurrent transactions]

In terminal1:
postgres=# begin;
BEGIN
postgres=# update tab set b = b * 2;
UPDATE 1

In terminal2:
postgres=# select foo.tableoid, foo.ctid, foo.* from foo, tab where
foo.a = tab.a for update;

In terminal1:
postgres=# commit;
COMMIT

In terminal2:(When committing the UPDATE query in terminal1, psql in
terminal2 will display the result for the SELECT-FOR-UPDATE query as
shown below.)
tableoid | ctid | a
----------+-------+---
16459 | (0,0) | 1
(1 row)

Note the value of the ctid has changed!

Rather than changing nodeForeignscan.c, it might be better to change
heap_form_tuple to set the t_ctid of a formed tuple to be invalid.

Thanks for the review!

Best regards,
Etsuro Fujita