EvalPlanQual behaves oddly for FDW queries involving system columns

classic Classic list List threaded Threaded
49 messages Options
123
Reply | Threaded
Open this post in threaded view
|

EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
Here is an example using postgres_fdw.

[Terminal 1]
postgres=# create table t (a int, b int);
CREATE TABLE
postgres=# insert into t values (1, 1);
INSERT 0 1
postgres=# begin;
BEGIN
postgres=# update t set b = b * 2;
UPDATE 1

[Terminal 2]
postgres=# create foreign table ft (a int) server loopback options
(table_name 'lbt');
CREATE FOREIGN TABLE
postgres=# insert into ft values (1);
INSERT 0 1
postgres=# select tableoid, ctid, * from ft;
 tableoid | ctid  | a
----------+-------+---
    25092 | (0,1) | 1
(1 row)

postgres=# select ft.tableoid, ft.ctid, ft.* from t, ft where t.a = ft.a
for update;

[Terminal 1]
postgres=# commit;
COMMIT

[Terminal 2]
postgres=# select ft.tableoid, ft.ctid, ft.* from t, ft where t.a = ft.a
for update;
 tableoid |      ctid      | a
----------+----------------+---
        0 | (4294967295,0) | 1
(1 row)

Note that tableoid and ctid have been changed!

I think the reason for that is because EvalPlanQualFetchRowMarks doesn't
properly set tableoid and ctid for foreign tables, IIUC.  I think this
should be fixed.  Please find attached a patch.  The patch slightly
relates to [1], so if it is reasonable, I'll update [1] on top of this.

[1] https://commitfest.postgresql.org/action/patch_view?id=1386

Best regards,
Etsuro Fujita


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

evalqualplan-v1.patch (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Alvaro Herrera-9
Etsuro Fujita wrote:

> ***************
> *** 817,826 **** InitPlan(QueryDesc *queryDesc, int eflags)
> --- 818,833 ----
>   break;
>   case ROW_MARK_COPY:
>   /* there's no real table here ... */
> + relkind = rt_fetch(rc->rti, rangeTable)->relkind;
> + if (relkind == RELKIND_FOREIGN_TABLE)
> + relid = getrelid(rc->rti, rangeTable);
> + else
> + relid = InvalidOid;
>   relation = NULL;
>   break;
>   default:
>   elog(ERROR, "unrecognized markType: %d", rc->markType);
> + relid = InvalidOid;
>   relation = NULL; /* keep compiler quiet */
>   break;
>   }

[ ... ]

> --- 2326,2342 ----
>  
>   /* build a temporary HeapTuple control structure */
>   tuple.t_len = HeapTupleHeaderGetDatumLength(td);
> ! /* if relid is valid, rel is a foreign table; set system columns */
> ! if (OidIsValid(erm->relid))
> ! {
> ! tuple.t_self = td->t_ctid;
> ! tuple.t_tableOid = erm->relid;
> ! }
> ! else
> ! {
> ! ItemPointerSetInvalid(&(tuple.t_self));
> ! tuple.t_tableOid = InvalidOid;
> ! }
>   tuple.t_data = td;
>  
>   /* copy and store tuple */

I find this arrangement confusing and unnecessary -- surely if you have
access to the ExecRowMark here, you could just obtain the relid with
RelationGetRelid instead of saving the OID beforehand?  And if you have
the Relation, you could just consult the relkind at that point instead
of relying on the relid being set or not as a flag to indicate whether
the rel is foreign.

I didn't look at anything else in the patch so I can't comment more on
it, but the change to ExecRowMark caught my attention.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
On 2015/01/16 1:24, Alvaro Herrera wrote:

> Etsuro Fujita wrote:
>> *** 817,826 **** InitPlan(QueryDesc *queryDesc, int eflags)
>> --- 818,833 ----
>>     break;
>>     case ROW_MARK_COPY:
>>     /* there's no real table here ... */
>> + relkind = rt_fetch(rc->rti, rangeTable)->relkind;
>> + if (relkind == RELKIND_FOREIGN_TABLE)
>> + relid = getrelid(rc->rti, rangeTable);
>> + else
>> + relid = InvalidOid;
>>     relation = NULL;
>>     break;

>> --- 2326,2342 ----
>>
>>     /* build a temporary HeapTuple control structure */
>>     tuple.t_len = HeapTupleHeaderGetDatumLength(td);
>> ! /* if relid is valid, rel is a foreign table; set system columns */
>> ! if (OidIsValid(erm->relid))
>> ! {
>> ! tuple.t_self = td->t_ctid;
>> ! tuple.t_tableOid = erm->relid;
>> ! }
>> ! else
>> ! {
>> ! ItemPointerSetInvalid(&(tuple.t_self));
>> ! tuple.t_tableOid = InvalidOid;
>> ! }
>>     tuple.t_data = td;

> I find this arrangement confusing and unnecessary -- surely if you have
> access to the ExecRowMark here, you could just obtain the relid with
> RelationGetRelid instead of saving the OID beforehand?

I don't think so because we don't have the Relation (ie, erm->relation =
NULL) here since InitPlan don't open/lock relations having markType =
ROW_MARK_COPY as shown above, which include foreign tables selected for
update/share.  But I noticed we should open/lock such foreign tables as
well in InitPlan because I think ExecOpenScanRelation is assuming that,
IIUC.

 > And if you have
> the Relation, you could just consult the relkind at that point instead
> of relying on the relid being set or not as a flag to indicate whether
> the rel is foreign.

Followed your revision.  Attached is an updated version of the patch.

Thanks for the comment!

Best regards,
Etsuro Fujita


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

EvalPlanQual-v2.patch (5K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
On 2015/01/19 17:10, Etsuro Fujita wrote:
> Attached is an updated version of the patch.

I'll add this to the next CF.

Best regards,
Etsuro Fujita


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Ashutosh Bapat
In reply to this post by Etsuro Fujita
Hi Fujita-san,
I am having some minor problems running this repro


On Thu, Jan 15, 2015 at 12:45 PM, Etsuro Fujita <[hidden email]> wrote:
Here is an example using postgres_fdw.

[Terminal 1]
postgres=# create table t (a int, b int);
CREATE TABLE
postgres=# insert into t values (1, 1);
INSERT 0 1
postgres=# begin;
BEGIN
postgres=# update t set b = b * 2;
UPDATE 1

[Terminal 2]
postgres=# create foreign table ft (a int) server loopback options
(table_name 'lbt');

There isn't any table "lbt" mentioned here. Do you mean "t" here?
 
CREATE FOREIGN TABLE
postgres=# insert into ft values (1);
INSERT 0 1
postgres=# select tableoid, ctid, * from ft;
 tableoid | ctid  | a
----------+-------+---
    25092 | (0,1) | 1
(1 row)

Shouldn't we see two values here one inserted in 't' and one in "ft"

postgres=# select ft.tableoid, ft.ctid, ft.* from t, ft where t.a = ft.a
for update;

[Terminal 1]
postgres=# commit;
COMMIT

[Terminal 2]
postgres=# select ft.tableoid, ft.ctid, ft.* from t, ft where t.a = ft.a
for update;
 tableoid |      ctid      | a
----------+----------------+---
        0 | (4294967295,0) | 1
(1 row)


Instead of this result, I got following error
ERROR:  could not serialize access due to concurrent update
CONTEXT:  Remote SQL command: SELECT a, ctid FROM public.t FOR UPDATE

Am I missing something while reproducing the problem?
 
Note that tableoid and ctid have been changed!

I think the reason for that is because EvalPlanQualFetchRowMarks doesn't
properly set tableoid and ctid for foreign tables, IIUC.  I think this
should be fixed.  Please find attached a patch.  The patch slightly
relates to [1], so if it is reasonable, I'll update [1] on top of this.

[1] https://commitfest.postgresql.org/action/patch_view?id=1386

Best regards,
Etsuro Fujita


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers




--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
Hi Ashutosh,

On 2015/02/03 16:44, Ashutosh Bapat wrote:
> I am having some minor problems running this repro

>     [Terminal 2]
>     postgres=# create foreign table ft (a int) server loopback options
>     (table_name 'lbt');

> There isn't any table "lbt" mentioned here. Do you mean "t" here?

Sorry, my explanation was not enough.  "lbt" means a foreign table named
"lbt" defined on a foreign server named "loopback".  It'd be defined eg,
in the following manner:

$ createdb efujita

efujita=# create table lbt (a int);
CREATE TABLE

postgres=# create server loopback foreign data wrapper postgres_fdw
options (dbname 'efujita');
CREATE SERVER
postgres=# create user mapping for current_user server loopback;
CREATE USER MAPPING
postgres=# create foreign table ft (a int) server loopback options
(table_name 'lbt');
CREATE FOREIGN TABLE

Thanks for the review!

Best regards,
Etsuro Fujita


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Ashutosh Bapat
Hi Fujita-san,
I tried reproducing the issue with the steps summarised.
Here's my setup
postgres=# \d ft
         Foreign table "public.ft"
 Column |  Type   | Modifiers | FDW Options
--------+---------+-----------+-------------
 a      | integer |           |
Server: loopback
FDW Options: (table_name 'lbt')

postgres=# \d lbt
      Table "public.lbt"
 Column |  Type   | Modifiers
--------+---------+-----------
 a      | integer |

The select (without for update) returns me two rows (one inserted in lbt and one in ft), whereas in your description there is only one row. For me, I am getting following error
postgres=# select ft.tableoid, ft.ctid, ft.* from lbt, ft where lbt.a = ft.a
for update;
ERROR:  could not serialize access due to concurrent update
CONTEXT:  Remote SQL command: SELECT a, ctid FROM public.lbt FOR UPDATE
postgres=#

after commit on terminal 1.

Am I missing something?
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
On 2015/03/09 16:02, Ashutosh Bapat wrote:
> I tried reproducing the issue with the steps summarised.

Thanks for the review!

> Here's my setup

Sorry, my explanation was not enough, but such was not my intention.
I'll re-summarize the steps below:

[Create a test environment]

$ createdb mydatabase
$ psql mydatabase
mydatabase=# create table mytable (a int);

$ psql postgres
postgres=# create extension postgres_fdw;
postgres=# create server loopback foreign data wrapper postgres_fdw
options (dbname 'mydatabase');
postgres=# create user mapping for current_user server loopback;
postgres=# create foreign table ftable (a int) server loopback options
(table_name 'mytable');
postgres=# insert into ftable values (1);
postgres=# create table ltable (a int, b int);
postgres=# insert into ltable values (1, 1);

[Run concurrent transactions]

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

In terminal2:
$ psql postgres
postgres=# select tableoid, ctid, * from ftable;
  tableoid | ctid  | a
----------+-------+---
     16394 | (0,1) | 1
(1 row)

postgres=# select f.tableoid, f.ctid, f.* from ftable f, ltable l where
f.a = l.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
----------+----------------+---
         0 | (4294967295,0) | 1
(1 row)

Best regards,
Etsuro Fujita


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Ashutosh Bapat
Now I can reproduce the problem.

Sanity
--------
Patch compiles cleanly and make check passes. The tests in file_fdw and postgres_fdw contrib modules pass.

The patch works as expected in the test case reported.

I have only one doubt.
In EvalPlanQualFetchRowMarks(). tuple->t_self is assigned from td->t_ctid. CTID or even t_self may be valid for foreign tables based on postgres_fdw but may not be valid for other FDWs. So, this assignment might put some garbage in t_self, rather we should set it to invalid as done prior to the patch. I might have missed some previous thread, we decided to go this route, so ignore the comment, in that case.

Apart from this, I do not have any comments here.

On Mon, Mar 9, 2015 at 4:05 PM, Etsuro Fujita <[hidden email]> wrote:
On 2015/03/09 16:02, Ashutosh Bapat wrote:
I tried reproducing the issue with the steps summarised.

Thanks for the review!

Here's my setup

Sorry, my explanation was not enough, but such was not my intention. I'll re-summarize the steps below:

[Create a test environment]

$ createdb mydatabase
$ psql mydatabase
mydatabase=# create table mytable (a int);

$ psql postgres
postgres=# create extension postgres_fdw;
postgres=# create server loopback foreign data wrapper postgres_fdw options (dbname 'mydatabase');
postgres=# create user mapping for current_user server loopback;
postgres=# create foreign table ftable (a int) server loopback options (table_name 'mytable');
postgres=# insert into ftable values (1);
postgres=# create table ltable (a int, b int);
postgres=# insert into ltable values (1, 1);

[Run concurrent transactions]

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

In terminal2:
$ psql postgres
postgres=# select tableoid, ctid, * from ftable;
 tableoid | ctid  | a
----------+-------+---
    16394 | (0,1) | 1
(1 row)

postgres=# select f.tableoid, f.ctid, f.* from ftable f, ltable l where f.a = l.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
----------+----------------+---
        0 | (4294967295,0) | 1
(1 row)

Best regards,
Etsuro Fujita



--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
On 2015/03/11 17:37, Ashutosh Bapat wrote:
> Now I can reproduce the problem.
>
> Sanity
> --------
> Patch compiles cleanly and make check passes. The tests in file_fdw and
> postgres_fdw contrib modules pass.
>
> The patch works as expected in the test case reported.

Thanks for the testing!

> I have only one doubt.
> In EvalPlanQualFetchRowMarks(). tuple->t_self is assigned from
> td->t_ctid. CTID or even t_self may be valid for foreign tables based on
> postgres_fdw but may not be valid for other FDWs. So, this assignment
> might put some garbage in t_self, rather we should set it to invalid as
> done prior to the patch. I might have missed some previous thread, we
> decided to go this route, so ignore the comment, in that case.

Good point.  As the following code and comment I added to ForeignNext, I
think that FDW authors should initialize the tup->t_data->t_ctid of each
scan tuple with its own TID.  If the authors do that, the t_self is
guaranteed to be assigned the right TID from the whole-row Var (ie,
td->t_ctid) in EvalPlanQualFetchRowMarks.

         /* We assume that t_ctid is initialized with its own TID */
         tup->t_self = tup->t_data->t_ctid;

IMHO, I'm not sure it's worth complicating the code as you mentioned.
(I don't know whether there are any discussions about this before.)

Note that file_fdw needs no treatment.  In that case, in ForeignNext,
the tup->t_data->t_ctid of each scan tuple is initialized with (0,0) (if
necessary), and then the t_self will be correctly assigned (0,0) throguh
the whole-row Var in EvalPlanQualFetchRowMarks.  So, no inconsistency!

> Apart from this, I do not have any comments here.

Thanks again.

Best regards,
Etsuro Fujita


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Ashutosh Bapat


On Wed, Mar 11, 2015 at 5:10 PM, Etsuro Fujita <[hidden email]> wrote:
On 2015/03/11 17:37, Ashutosh Bapat wrote:
Now I can reproduce the problem.

Sanity
--------
Patch compiles cleanly and make check passes. The tests in file_fdw and
postgres_fdw contrib modules pass.

The patch works as expected in the test case reported.

Thanks for the testing!

I have only one doubt.
In EvalPlanQualFetchRowMarks(). tuple->t_self is assigned from
td->t_ctid. CTID or even t_self may be valid for foreign tables based on
postgres_fdw but may not be valid for other FDWs. So, this assignment
might put some garbage in t_self, rather we should set it to invalid as
done prior to the patch. I might have missed some previous thread, we
decided to go this route, so ignore the comment, in that case.

Good point.  As the following code and comment I added to ForeignNext, I think that FDW authors should initialize the tup->t_data->t_ctid of each scan tuple with its own TID.  If the authors do that, the t_self is guaranteed to be assigned the right TID from the whole-row Var (ie, td->t_ctid) in EvalPlanQualFetchRowMarks.

        /* We assume that t_ctid is initialized with its own TID */
        tup->t_self = tup->t_data->t_ctid;

IMHO, I'm not sure it's worth complicating the code as you mentioned. (I don't know whether there are any discussions about this before.)

Note that file_fdw needs no treatment.  In that case, in ForeignNext, the tup->t_data->t_ctid of each scan tuple is initialized with (0,0) (if necessary), and then the t_self will be correctly assigned (0,0) throguh the whole-row Var in EvalPlanQualFetchRowMarks.  So, no inconsistency!


I will leave this issue for the committer to judge. Changed the status to "ready for committer".
 
Apart from this, I do not have any comments here.

Thanks again.

Best regards,
Etsuro Fujita



--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Tom Lane-2
Ashutosh Bapat <[hidden email]> writes:
> I will leave this issue for the committer to judge. Changed the status to
> "ready for committer".

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, but if so they belong in a different patch that
actually implements such a thing.  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 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.

                        regards, tom lane


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
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


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Tom Lane-2
Etsuro Fujita <[hidden email]> writes:
> 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?

I think the real fix as far as postgres_fdw is concerned is in fact
to let it adopt a different ROW_MARK strategy, since it has meaningful
ctid values.  However, that is not a one-size-fits-all answer.  The
fundamental problem I've got with this patch is that it's trying to
impose some one-size-fits-all assumptions on all FDWs about whether
ctids are meaningful; which is wrong, not to mention not backwards
compatible.

>> 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.

But that's self-inflicted damage, because in fact ctid correctly shows
as invalid in this case in HEAD, without this patch.

The tableoid problem can be fixed much less invasively as per the attached
patch.  I think that we should continue to assume that ctid is not
meaningful (and hence should read as (4294967295,0)) in FDWs that use
ROW_MARK_COPY, and press forward on fixing the locking issues for
postgres_fdw by letting it use ROW_MARK_REFERENCE or something close to
that.  That would also cause ctid to read properly for rows from
postgres_fdw.

                        regards, tom lane


diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 33b172b..5a1c3b3 100644
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
*************** EvalPlanQualFetchRowMarks(EPQState *epqs
*** 2438,2444 ****
  /* build a temporary HeapTuple control structure */
  tuple.t_len = HeapTupleHeaderGetDatumLength(td);
  ItemPointerSetInvalid(&(tuple.t_self));
! tuple.t_tableOid = InvalidOid;
  tuple.t_data = td;
 
  /* copy and store tuple */
--- 2438,2445 ----
  /* build a temporary HeapTuple control structure */
  tuple.t_len = HeapTupleHeaderGetDatumLength(td);
  ItemPointerSetInvalid(&(tuple.t_self));
! tuple.t_tableOid = getrelid(erm->rti,
! epqstate->estate->es_range_table);
  tuple.t_data = td;
 
  /* copy and store tuple */


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
On 2015/03/13 0:50, Tom Lane wrote:
> The tableoid problem can be fixed much less invasively as per the attached
> patch.  I think that we should continue to assume that ctid is not
> meaningful (and hence should read as (4294967295,0)) in FDWs that use
> ROW_MARK_COPY, and press forward on fixing the locking issues for
> postgres_fdw by letting it use ROW_MARK_REFERENCE or something close to
> that.  That would also cause ctid to read properly for rows from
> postgres_fdw.

OK, thanks!

BTW, what do you think about opening/locking foreign tables selected for
update at InitPlan, which the original patch does?  As I mentioned in
[1], ISTM that ExecOpenScanRelation called from ExecInitForeignScan is
assuming that.

Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/54BCBBF8.3020103@...


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
On 2015/03/13 11:46, Etsuro Fujita wrote:
> BTW, what do you think about opening/locking foreign tables selected for
> update at InitPlan, which the original patch does?  As I mentioned in
> [1], ISTM that ExecOpenScanRelation called from ExecInitForeignScan is
> assuming that.

> [1] http://www.postgresql.org/message-id/54BCBBF8.3020103@...

Let me explain further.  Here is the comment in ExecOpenScanRelation:

      * Determine the lock type we need.  First, scan to see if target
relation
      * is a result relation.  If not, check if it's a FOR UPDATE/FOR SHARE
      * relation.  In either of those cases, we got the lock already.

I think this is not true for foreign tables selected FOR UPDATE/SHARE,
which have markType = ROW_MARK_COPY, because such foreign tables don't
get opened/locked by InitPlan.  Then such foreign tables don't get
locked by neither of InitPlan nor ExecOpenScanRelation.  I think this is
a bug.  To fix it, I think we should open/lock such foreign tables at
InitPlan as the original patch does.

Best regards,
Etsuro Fujita


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Tom Lane-2
Etsuro Fujita <[hidden email]> writes:
> Let me explain further.  Here is the comment in ExecOpenScanRelation:

>       * Determine the lock type we need.  First, scan to see if target
> relation
>       * is a result relation.  If not, check if it's a FOR UPDATE/FOR SHARE
>       * relation.  In either of those cases, we got the lock already.

> I think this is not true for foreign tables selected FOR UPDATE/SHARE,
> which have markType = ROW_MARK_COPY, because such foreign tables don't
> get opened/locked by InitPlan.  Then such foreign tables don't get
> locked by neither of InitPlan nor ExecOpenScanRelation.  I think this is
> a bug.

You are right.  I think it may not matter in practice, but if the executor
is taking its own locks here then it should not overlook ROW_MARK_COPY
cases.

> To fix it, I think we should open/lock such foreign tables at
> InitPlan as the original patch does.

I still don't like that; InitPlan is not doing something that would
require physical table access.  The right thing is to fix
ExecOpenScanRelation's idea of whether InitPlan took a lock or not,
which I've now done.

                        regards, tom lane


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
On 2015/03/25 4:56, Tom Lane wrote:

> Etsuro Fujita <[hidden email]> writes:
>> Let me explain further.  Here is the comment in ExecOpenScanRelation:
>
>>        * Determine the lock type we need.  First, scan to see if target
>> relation
>>        * is a result relation.  If not, check if it's a FOR UPDATE/FOR SHARE
>>        * relation.  In either of those cases, we got the lock already.
>
>> I think this is not true for foreign tables selected FOR UPDATE/SHARE,
>> which have markType = ROW_MARK_COPY, because such foreign tables don't
>> get opened/locked by InitPlan.  Then such foreign tables don't get
>> locked by neither of InitPlan nor ExecOpenScanRelation.  I think this is
>> a bug.
>
> You are right.  I think it may not matter in practice, but if the executor
> is taking its own locks here then it should not overlook ROW_MARK_COPY
> cases.
>
>> To fix it, I think we should open/lock such foreign tables at
>> InitPlan as the original patch does.
>
> I still don't like that; InitPlan is not doing something that would
> require physical table access.  The right thing is to fix
> ExecOpenScanRelation's idea of whether InitPlan took a lock or not,
> which I've now done.

OK, thanks.

Best regards,
Etsuro Fujita


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Etsuro Fujita
In reply to this post by Tom Lane-2
On 2015/03/13 0:50, Tom Lane wrote:
> I think the real fix as far as postgres_fdw is concerned is in fact
> to let it adopt a different ROW_MARK strategy, since it has meaningful
> ctid values.  However, that is not a one-size-fits-all answer.

> The tableoid problem can be fixed much less invasively as per the attached
> patch.  I think that we should continue to assume that ctid is not
> meaningful (and hence should read as (4294967295,0)) in FDWs that use
> ROW_MARK_COPY, and press forward on fixing the locking issues for
> postgres_fdw by letting it use ROW_MARK_REFERENCE or something close to
> that.  That would also cause ctid to read properly for rows from
> postgres_fdw.

To support ROW_MARK_REFERENCE on (postgres_fdw) foreign tables, I'd like
to propose the following FDW APIs:

RowMarkType
GetForeignRowMarkType(Oid relid,
                       LockClauseStrength strength);

Decide which rowmark type to use for a foreign table (that has strength
= LCS_NONE), ie, ROW_MARK_REFERENCE or ROW_MARK_COPY.  (For now, the
second argument takes LCS_NONE only, but is intended to be used for the
possible extension to the other cases.)  This is called during
select_rowmark_type() in the planner.

void
BeginForeignFetch(EState *estate,
                   ExecRowMark *erm,
                   List *fdw_private,
                   int eflags);

Begin a remote fetch. This is called during InitPlan() in the executor.

HeapTuple
ExecForeignFetch(EState *estate,
                  ExecRowMark *erm,
                  ItemPointer tupleid);

Re-fetch the specified tuple.  This is called during
EvalPlanQualFetchRowMarks() in the executor.

void
EndForeignFetch(EState *estate,
                 ExecRowMark *erm);

End a remote fetch.  This is called during ExecEndPlan() in the executor.

And I'd also like to propose to add a table/server option,
row_mark_reference, to postgres_fdw.  When a user sets the option to
true for eg a foreign table, ROW_MARK_REFERENCE will be used for the
table, not ROW_MARK_COPY.

Attached is a WIP patch, which contains no docs/regression tests.

It'd be appreciated if anyone could send back any comments earlier.

Best regards,
Etsuro Fujita


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

EvalPlanQual-v3.patch (36K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: EvalPlanQual behaves oddly for FDW queries involving system columns

Tom Lane-2
Etsuro Fujita <[hidden email]> writes:
> To support ROW_MARK_REFERENCE on (postgres_fdw) foreign tables, I'd like
> to propose the following FDW APIs:

> RowMarkType
> GetForeignRowMarkType(Oid relid,
>                        LockClauseStrength strength);

> Decide which rowmark type to use for a foreign table (that has strength
> = LCS_NONE), ie, ROW_MARK_REFERENCE or ROW_MARK_COPY.  (For now, the
> second argument takes LCS_NONE only, but is intended to be used for the
> possible extension to the other cases.)  This is called during
> select_rowmark_type() in the planner.

Why would you restrict that to LCS_NONE?  Seems like the point is to give
the FDW control of the rowmark behavior, not only partial control.
(For the same reason I disagree with the error check in the patch that
restricts which ROW_MARK options this function can return.  If the FDW has
TIDs, seems like it could reasonably use whatever options tables can use.)

> void
> BeginForeignFetch(EState *estate,
>                    ExecRowMark *erm,
>                    List *fdw_private,
>                    int eflags);

> Begin a remote fetch. This is called during InitPlan() in the executor.

The begin/end functions seem like useless extra mechanism.  Why wouldn't
the FDW just handle this during its regular scan setup?  It could look to
see whether the foreign table is referenced by any ExecRowMarks (possibly
there's room to add some convenience functions to help with that).  What's
more, that design would make it simpler if the basic row fetch needs to be
modified.

> And I'd also like to propose to add a table/server option,
> row_mark_reference, to postgres_fdw.  When a user sets the option to
> true for eg a foreign table, ROW_MARK_REFERENCE will be used for the
> table, not ROW_MARK_COPY.

Why would we leave that in the hands of the user?  Hardly anybody is going
to know what to do with the option, or even that they should do something
with it.  It's basically only of value for debugging AFAICS, but if we
expose an option we're going to be stuck with supporting it forever.

                        regards, tom lane


--
Sent via pgsql-hackers mailing list ([hidden email])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
123