Odd system-column handling in postgres_fdw join pushdown patch

On Thu, Mar 17, 2016 at 4:30 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:

Hi,

I noticed that the postgres_fdw join pushdown patch retrieves system
columns other than ctid (and oid) from the remote server as shown in the
example:

postgres=# explain verbose select foo.tableoid, foo.xmin, foo.cmin,
foo.xmax, foo.cmax, foo.* from foo, bar where foo.a = bar.a;

QUERY PLAN

--------------------------------------------------------------------------------------------------------------------------------------------------------
--------
Foreign Scan (cost=100.00..102.09 rows=2 width=28)
Output: foo.tableoid, foo.xmin, foo.cmin, foo.xmax, foo.cmax, foo.a,
foo.b
Relations: (public.foo) INNER JOIN (public.bar)
Remote SQL: SELECT r1.tableoid, r1.xmin, r1.cmin, r1.xmax, r1.cmax,
r1.a, r1.b FROM (public.foo r1 INNER JOIN public.bar r2 ON (TRUE)) WHERE
((r1.a =
r2.a))
(4 rows)

Thanks for catching the bug and producing a patch.

BUT: we don't make any effort to ensure that local and remote values
match, so system columns other than ctid and oid should not be retrieved
from the remote server. So, I'd like to propose: (1) when tableoids are
requested from the remote server, postgres_fdw sets valid values for
them locally, instead (core should support that?) and

If we are disabling join pushdown when the targetlist has other system
columns, shouldn't we treat tableoid in the same fashion. We should disable
join pushdown when tableoid is requested?

I agree that we might want to do this in core instead of FDW specific core.
That way we avoid each FDW implementing its own solution. Ultimately, all
that needs to be done to push OID of the foreign table in place of tableoid
column. The core code can do that. It already does that for the base
tables.

(2) when any of
xmins, xmaxs, cmins, and cmaxs are requested, postgres_fdw gives up
pushing down foreign joins. (We might be able to set appropriate values
for them locally the same way as for tableoids, but I'm not sure it's
worth complicating the code.) I think that would be probably OK,
because users wouldn't retrieve any such columns in practice.

In that patch you have set pushdown_safe to false for the base relation
fetching system columns. But pushdown_safe = false means that that relation
is not safe to push down. A base foreign relation is always safe to push
down, so should have pushdown_safe = true always. Instead, I suggest having
a separate boolean has_unshippable_syscols (or something with similar name)
in PgFdwRelationInfo, which is set to true in such case. In
foreign_join_ok, we return false (thus not pushing down the join), if any
of the joining relation has that attribute set. By default this member is
false.

Even for a base table those values are rather random, although they are not
fetched from the foreign server. Instead of not pushing the join down, we
should push the join down without fetching those attributes. While
constructing the query, don't include these system attributes in SELECT
clause and don't include corresponding positions in retrieved_attributes
list. That means those attributes won't be set while fetching the row from
the foreign server and will have garbage values in corresponding places. I
guess that would work.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2016/03/17 22:15, Ashutosh Bapat wrote:

On Thu, Mar 17, 2016 at 4:30 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:

BUT: we don't make any effort to ensure that local and remote values
match, so system columns other than ctid and oid should not be retrieved
from the remote server. So, I'd like to propose: (1) when tableoids are
requested from the remote server, postgres_fdw sets valid values for
them locally, instead (core should support that?) and

If we are disabling join pushdown when the targetlist has other system
columns, shouldn't we treat tableoid in the same fashion. We should
disable join pushdown when tableoid is requested?

That seems a bit too restrictive to me.

I agree that we might want to do this in core instead of FDW specific
core. That way we avoid each FDW implementing its own solution.
Ultimately, all that needs to be done to push OID of the foreign table
in place of tableoid column. The core code can do that. It already does
that for the base tables.

OK, I'll try to modify the patch so that the core does that work.

(2) when any of
xmins, xmaxs, cmins, and cmaxs are requested, postgres_fdw gives up
pushing down foreign joins. (We might be able to set appropriate values
for them locally the same way as for tableoids, but I'm not sure it's
worth complicating the code.) I think that would be probably OK,
because users wouldn't retrieve any such columns in practice.

In that patch you have set pushdown_safe to false for the base relation
fetching system columns. But pushdown_safe = false means that that
relation is not safe to push down. A base foreign relation is always
safe to push down, so should have pushdown_safe = true always. Instead,
I suggest having a separate boolean has_unshippable_syscols (or
something with similar name) in PgFdwRelationInfo, which is set to true
in such case. In foreign_join_ok, we return false (thus not pushing down
the join), if any of the joining relation has that attribute set. By
default this member is false.

Maybe I'm missing something, but why do you consider that base foreign
tables need always be safe to push down? IIUC, the pushdown_safe flag
is used only for foreign_join_ok, so I think that a base foreign table
needs not necessarily be safe to push down.

Even for a base table those values are rather random, although they are
not fetched from the foreign server. Instead of not pushing the join
down, we should push the join down without fetching those attributes.
While constructing the query, don't include these system attributes in
SELECT clause and don't include corresponding positions in
retrieved_attributes list. That means those attributes won't be set
while fetching the row from the foreign server and will have garbage
values in corresponding places. I guess that would work.

That might be an idea, but as I said above, users wouldn't specify any
system columns other than tableoid and ctid (and oid) in their queries,
in practice, so I'm not sure it's worth complicating the code.

Best regards,
Etsuro Fujita

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

(2) when any of

xmins, xmaxs, cmins, and cmaxs are requested, postgres_fdw gives up
pushing down foreign joins. (We might be able to set appropriate
values
for them locally the same way as for tableoids, but I'm not sure it's
worth complicating the code.) I think that would be probably OK,
because users wouldn't retrieve any such columns in practice.

In that patch you have set pushdown_safe to false for the base relation

fetching system columns. But pushdown_safe = false means that that
relation is not safe to push down. A base foreign relation is always
safe to push down, so should have pushdown_safe = true always. Instead,
I suggest having a separate boolean has_unshippable_syscols (or
something with similar name) in PgFdwRelationInfo, which is set to true
in such case. In foreign_join_ok, we return false (thus not pushing down
the join), if any of the joining relation has that attribute set. By
default this member is false.

Maybe I'm missing something, but why do you consider that base foreign
tables need always be safe to push down? IIUC, the pushdown_safe flag is
used only for foreign_join_ok, so I think that a base foreign table needs
not necessarily be safe to push down.

A base foreign table "always" fetches data from the foreign server, so it
"has to be" always safe to push down. pushdown_safe flag is designated to
tell whether the relation corresponding to PgFdwRelationInfo where this
flag is set is safe to push down.Right now it's only used for joins but in
future it would be used for any push down of higher operations. It seems
very much possible after the upper pathification changes. We can not have a
query sent to the foreign server for a relation, when pushdown_safe is
false for that. Your patch does that for foreign base relation which try to
fetch system columns.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Thu, Mar 17, 2016 at 7:00 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

BUT: we don't make any effort to ensure that local and remote values
match, so system columns other than ctid and oid should not be retrieved
from the remote server.

I agree.

So, I'd like to propose: (1) when tableoids are
requested from the remote server, postgres_fdw sets valid values for
them locally, instead (core should support that?)

Sure.

and (2) when any of
xmins, xmaxs, cmins, and cmaxs are requested, postgres_fdw gives up
pushing down foreign joins. (We might be able to set appropriate values
for them locally the same way as for tableoids, but I'm not sure it's
worth complicating the code.) I think that would be probably OK,
because users wouldn't retrieve any such columns in practice.

Now that seems like the wrong reaction. I mean, aren't these just
going to be 0 or something? Refusing to push the join down seems
strange.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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

On 2016/03/19 4:51, Robert Haas wrote:

On Thu, Mar 17, 2016 at 7:00 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

So, I'd like to propose: (1) when tableoids are
requested from the remote server, postgres_fdw sets valid values for
them locally, instead (core should support that?)

Sure.

and (2) when any of
xmins, xmaxs, cmins, and cmaxs are requested, postgres_fdw gives up
pushing down foreign joins. (We might be able to set appropriate values
for them locally the same way as for tableoids, but I'm not sure it's
worth complicating the code.) I think that would be probably OK,
because users wouldn't retrieve any such columns in practice.

Now that seems like the wrong reaction. I mean, aren't these just
going to be 0 or something? Refusing to push the join down seems
strange.

OK, I'll modify the patch so that the join is pushed down even if any of
xmins, xmaxs, cmins, and cmaxs are requested. Do you think which one
should set values for these as well as tableoids, postgres_fdw or core?

Best regards,
Etsuro Fujita

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

On Tue, Mar 22, 2016 at 8:03 AM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:

On 2016/03/19 4:51, Robert Haas wrote:

On Thu, Mar 17, 2016 at 7:00 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp> wrote:

So, I'd like to propose: (1) when tableoids are
requested from the remote server, postgres_fdw sets valid values for
them locally, instead (core should support that?)

Sure.

and (2) when any of

xmins, xmaxs, cmins, and cmaxs are requested, postgres_fdw gives up
pushing down foreign joins. (We might be able to set appropriate values
for them locally the same way as for tableoids, but I'm not sure it's
worth complicating the code.) I think that would be probably OK,
because users wouldn't retrieve any such columns in practice.

Now that seems like the wrong reaction. I mean, aren't these just

going to be 0 or something? Refusing to push the join down seems
strange.

OK, I'll modify the patch so that the join is pushed down even if any of
xmins, xmaxs, cmins, and cmaxs are requested. Do you think which one
should set values for these as well as tableoids, postgres_fdw or core?

Earlier in this mail chain, I suggested that the core should take care of
storing the values for these columns. But instead, I think, core should
provide functions which can be used by FDWs, if they want, to return values
for those columns. Something like Datum
get_syscol_value(RelOptInfo/Relation, attno). The function will return
Datum 0 for most of the columns and table's OID for tableoid. My 0.02.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2016/03/22 14:54, Ashutosh Bapat wrote:

On Tue, Mar 22, 2016 at 8:03 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:
OK, I'll modify the patch so that the join is pushed down even if
any of xmins, xmaxs, cmins, and cmaxs are requested. Do you think
which one should set values for these as well as tableoids,
postgres_fdw or core?

Earlier in this mail chain, I suggested that the core should take care
of storing the values for these columns. But instead, I think, core
should provide functions which can be used by FDWs, if they want, to
return values for those columns. Something like Datum
get_syscol_value(RelOptInfo/Relation, attno). The function will return
Datum 0 for most of the columns and table's OID for tableoid. My 0.02.

What I had in mind was (1) create_foreignscan_plan would create Lists
from the ForeignScan's fdw_scan_tlist: (a) indexes/OID values of
tableoids in fdw_scan_tlist, and (b) indexes of xids and cids in
fdw_scan_tlist, and then (2) ForeignNext would set the OID values for
the tableoid columns in the scan tuple, using the Lists (a), and
appropriate values (0 or something) for the xid and cid columns in the
scan tuple, using the List (b).

Best regards,
Etsuro Fujita

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

On Tue, Mar 22, 2016 at 5:05 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:

On 2016/03/22 14:54, Ashutosh Bapat wrote:

On Tue, Mar 22, 2016 at 8:03 AM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:
OK, I'll modify the patch so that the join is pushed down even if
any of xmins, xmaxs, cmins, and cmaxs are requested. Do you think
which one should set values for these as well as tableoids,
postgres_fdw or core?

Earlier in this mail chain, I suggested that the core should take care

of storing the values for these columns. But instead, I think, core
should provide functions which can be used by FDWs, if they want, to
return values for those columns. Something like Datum
get_syscol_value(RelOptInfo/Relation, attno). The function will return
Datum 0 for most of the columns and table's OID for tableoid. My 0.02.

What I had in mind was (1) create_foreignscan_plan would create Lists from
the ForeignScan's fdw_scan_tlist: (a) indexes/OID values of tableoids in
fdw_scan_tlist, and (b) indexes of xids and cids in fdw_scan_tlist, and
then (2) ForeignNext would set the OID values for the tableoid columns in
the scan tuple, using the Lists (a), and appropriate values (0 or
something) for the xid and cid columns in the scan tuple, using the List
(b).

Looks Ok to me, although, that way an FDW looses an ability to use its own
values for those columns, in case it wants to. For example, while using
postgres_fdw for sharding, it might help saving xmax, xmin, cmax, cmin from
the foreign server and use them while communicating with the foreign server.

Best regards,
Etsuro Fujita

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2016/03/22 21:10, Ashutosh Bapat wrote:

On Tue, Mar 22, 2016 at 5:05 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:
On 2016/03/22 14:54, Ashutosh Bapat wrote:
Earlier in this mail chain, I suggested that the core should
take care
of storing the values for these columns. But instead, I think, core
should provide functions which can be used by FDWs, if they want, to
return values for those columns. Something like Datum
get_syscol_value(RelOptInfo/Relation, attno). The function will
return
Datum 0 for most of the columns and table's OID for tableoid. My
0.02.

What I had in mind was (1) create_foreignscan_plan would create
Lists from the ForeignScan's fdw_scan_tlist: (a) indexes/OID values
of tableoids in fdw_scan_tlist, and (b) indexes of xids and cids in
fdw_scan_tlist, and then (2) ForeignNext would set the OID values
for the tableoid columns in the scan tuple, using the Lists (a), and
appropriate values (0 or something) for the xid and cid columns in
the scan tuple, using the List (b).

Looks Ok to me, although, that way an FDW looses an ability to use its
own values for those columns, in case it wants to. For example, while
using postgres_fdw for sharding, it might help saving xmax, xmin, cmax,
cmin from the foreign server and use them while communicating with the
foreign server.

Yeah, it might be the case.

On second thoughts, I changed my mind; I think it'd be better for the
FDW's to set values for tableoids, xids, and cids in the scan tuple.
The reason other than your suggestion is because expressions in
fdw_scan_tlist that contain such columns are not necessarily simple Vars
and because such expressions might be evaluated more efficiently by the
FDW than core. We assume in postgres_fdw that expressions in
fdw_scan_tlist are always simple Vars, though.

I'm not sure it's worth providing functions you suggested, because we
can't assume that columns in the scan tuple are always simple Var
columns, as I said above.

Best regards,
Etsuro Fujita

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

On Wed, Mar 23, 2016 at 8:20 AM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:

On 2016/03/22 21:10, Ashutosh Bapat wrote:

On Tue, Mar 22, 2016 at 5:05 PM, Etsuro Fujita
<fujita.etsuro@lab.ntt.co.jp <mailto:fujita.etsuro@lab.ntt.co.jp>> wrote:
On 2016/03/22 14:54, Ashutosh Bapat wrote:
Earlier in this mail chain, I suggested that the core should
take care
of storing the values for these columns. But instead, I think,
core
should provide functions which can be used by FDWs, if they want,
to
return values for those columns. Something like Datum
get_syscol_value(RelOptInfo/Relation, attno). The function will
return
Datum 0 for most of the columns and table's OID for tableoid. My
0.02.

What I had in mind was (1) create_foreignscan_plan would create

Lists from the ForeignScan's fdw_scan_tlist: (a) indexes/OID values
of tableoids in fdw_scan_tlist, and (b) indexes of xids and cids in
fdw_scan_tlist, and then (2) ForeignNext would set the OID values
for the tableoid columns in the scan tuple, using the Lists (a), and
appropriate values (0 or something) for the xid and cid columns in
the scan tuple, using the List (b).

Looks Ok to me, although, that way an FDW looses an ability to use its

own values for those columns, in case it wants to. For example, while
using postgres_fdw for sharding, it might help saving xmax, xmin, cmax,
cmin from the foreign server and use them while communicating with the
foreign server.

Yeah, it might be the case.

On second thoughts, I changed my mind; I think it'd be better for the
FDW's to set values for tableoids, xids, and cids in the scan tuple. The
reason other than your suggestion is because expressions in fdw_scan_tlist
that contain such columns are not necessarily simple Vars and because such
expressions might be evaluated more efficiently by the FDW than core. We
assume in postgres_fdw that expressions in fdw_scan_tlist are always simple
Vars, though.

I'm not sure it's worth providing functions you suggested, because we
can't assume that columns in the scan tuple are always simple Var columns,
as I said above.

An FDW can choose not to use those functions, so I don't see a connection
between scan list having simple Vars and existence of those functions
(actually a single one). But having those function would minimize the code
that each FDW has to write, in case they want those functions. E.g. we have
to translate Var::varno to tableoid in case that's requested by pulling RTE
and then getting oid out from there. If that functionality is available in
the core, 1. the code is not duplicated 2. every FDW will get the same
tableoid. Similarly for the other columns.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2016/03/23 13:44, Ashutosh Bapat wrote:

An FDW can choose not to use those functions, so I don't see a
connection between scan list having simple Vars and existence of those
functions (actually a single one). But having those function would
minimize the code that each FDW has to write, in case they want those
functions. E.g. we have to translate Var::varno to tableoid in case
that's requested by pulling RTE and then getting oid out from there. If
that functionality is available in the core, 1. the code is not
duplicated 2. every FDW will get the same tableoid. Similarly for the
other columns.

OK. Then, I'd like to propose a function that would create interger
Lists of indexes of tableoids, xids and cids plus an OID List of these
tableoids, in a given fdw_scan_tlist, on the assumption that each
expression in the fdw_scan_tlist is a simple Var. I'd also like to
propose another function that would fill system columns using these
Lists when creating a scan tuple.

Best regards,
Etsuro Fujita

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

On Thu, Mar 24, 2016 at 9:31 AM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:

On 2016/03/23 13:44, Ashutosh Bapat wrote:

An FDW can choose not to use those functions, so I don't see a
connection between scan list having simple Vars and existence of those
functions (actually a single one). But having those function would
minimize the code that each FDW has to write, in case they want those
functions. E.g. we have to translate Var::varno to tableoid in case
that's requested by pulling RTE and then getting oid out from there. If
that functionality is available in the core, 1. the code is not
duplicated 2. every FDW will get the same tableoid. Similarly for the
other columns.

OK. Then, I'd like to propose a function that would create interger Lists
of indexes of tableoids, xids and cids plus

Ok,

an OID List of these tableoids,

I didn't get this.

in a given fdw_scan_tlist, on the assumption that each expression in the
fdw_scan_tlist is a simple Var.

I guess this is Ok. In fact, at least for now an expression involving any
of those columns is not pushable to the foreign server, as the expression
can not be evaluated there. So, if we come across such a case in further
pushdowns, we will need to have a different solution for pushing down such
target lists.

I'd also like to propose another function that would fill system columns
using these Lists when creating a scan tuple.

Ok.

I had imagined that the code to extract the above lists and filling the
values in scan tuple will be in FDW. We only provide a function to supply
those values. But what you propose might actually be much practical.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

A much simpler solution, that will work with postgres_fdw, might be to just
deparse these columns with whatever random values (except for tableoid)
they are expected to have in those places. Often these values can simply be
NULL or 0. For tableoid deparse it to 'oid value'::oid. Thus for a user
query

select t1.taleoid, t2.xmax, t1.c1, t2.c2 from t1 join t2 on (...) ... --
where t1 and t2 are foreign tables with same names on the foreign server.

the query sent to the foreign server would look like

select '15623'::oid, NULL, t1.c1, t2.c2 from t1 join t2 on (...) ... --
where '15623' is oid of t1 on local server.

This does spend more bandwidth than necessary and affect performance, here
is why the approach might be better,
1. It's not very common to request these system columns in a "join" query
involving foreign tables. Usually they will have user columns or ctid
(DMLs) but very rarely other system columns.

2. This allows expressions involving these system columns to be pushed
down, whenever we will start pushing them down in the targetlist.

3. The changes to the code are rather small. deparseColumnRef() will need
to produce the strings above instead of actual column names.

4. The approach will work with slight change, if and when, we need the
actual system column values from the foreign server. That time the above
function needs to deparse the column names instead of constant values.

Having to hardcode tableoid at the time of planning should be fine since
change in tableoid between planning and execution will trigger plan cache
invalidation. I haven't tried this though.

Sorry for bringing this solution late to the table.

On Thu, Mar 24, 2016 at 3:04 PM, Ashutosh Bapat <
ashutosh.bapat@enterprisedb.com> wrote:

On Thu, Mar 24, 2016 at 9:31 AM, Etsuro Fujita <
fujita.etsuro@lab.ntt.co.jp> wrote:

On 2016/03/23 13:44, Ashutosh Bapat wrote:

An FDW can choose not to use those functions, so I don't see a
connection between scan list having simple Vars and existence of those
functions (actually a single one). But having those function would
minimize the code that each FDW has to write, in case they want those
functions. E.g. we have to translate Var::varno to tableoid in case
that's requested by pulling RTE and then getting oid out from there. If
that functionality is available in the core, 1. the code is not
duplicated 2. every FDW will get the same tableoid. Similarly for the
other columns.

OK. Then, I'd like to propose a function that would create interger
Lists of indexes of tableoids, xids and cids plus

Ok,

an OID List of these tableoids,

I didn't get this.

in a given fdw_scan_tlist, on the assumption that each expression in the
fdw_scan_tlist is a simple Var.

I guess this is Ok. In fact, at least for now an expression involving any
of those columns is not pushable to the foreign server, as the expression
can not be evaluated there. So, if we come across such a case in further
pushdowns, we will need to have a different solution for pushing down such
target lists.

I'd also like to propose another function that would fill system columns
using these Lists when creating a scan tuple.

Ok.

I had imagined that the code to extract the above lists and filling the
values in scan tuple will be in FDW. We only provide a function to supply
those values. But what you propose might actually be much practical.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2016/03/25 13:37, Ashutosh Bapat wrote:

A much simpler solution, that will work with postgres_fdw, might be to
just deparse these columns with whatever random values (except for
tableoid) they are expected to have in those places. Often these values
can simply be NULL or 0. For tableoid deparse it to 'oid value'::oid.
Thus for a user query

select t1.taleoid, t2.xmax, t1.c1, t2.c2 from t1 join t2 on (...) ... --
where t1 and t2 are foreign tables with same names on the foreign server.

the query sent to the foreign server would look like

select '15623'::oid, NULL, t1.c1, t2.c2 from t1 join t2 on (...) ... --
where '15623' is oid of t1 on local server.

This does spend more bandwidth than necessary and affect performance,
here is why the approach might be better,
1. It's not very common to request these system columns in a "join"
query involving foreign tables. Usually they will have user columns or
ctid (DMLs) but very rarely other system columns.

That may be true for now, but once we implement pair-wise join for two
distributedly-partitioned tables in which we can push down pair-wise
foreign joins, tableoid would be used in many cases, to identify child
tables for rows to come from.

2. This allows expressions involving these system columns to be pushed
down, whenever we will start pushing them down in the targetlist.

3. The changes to the code are rather small. deparseColumnRef() will
need to produce the strings above instead of actual column names.

4. The approach will work with slight change, if and when, we need the
actual system column values from the foreign server. That time the above
function needs to deparse the column names instead of constant values.

As you pointed out, spending more bandwidth than necessary seems a bit
inefficient.

The approach that we discussed would minimize the code for the FDW
author to write, by providing the support functions you proposed. I'll
post a patch for that early next week. (It would also minimize the
patch to push down UPDATE/DELETE on a foreign join, proposed in [1]/messages/by-id/56D57C4A.9000500@lab.ntt.co.jp,
which has the same issue as for handling system columns in a RETURNING
clause in such pushed-down UPDATE/DELETE. So I'd like to propose that
approach as a common functionality.)

Sorry for bringing this solution late to the table.

No problem.

Best regards,
Etsuro Fujita

[1]: /messages/by-id/56D57C4A.9000500@lab.ntt.co.jp

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

On 2016/03/25 17:16, Etsuro Fujita wrote:

The approach that we discussed would minimize the code for the FDW
author to write, by providing the support functions you proposed. I'll
post a patch for that early next week.

I added two helper functions: GetFdwScanTupleExtraData and
FillFdwScanTupleSysAttrs. The FDW author could use the former to get
info about system attributes other than ctids and oids in fdw_scan_tlist
during BeginForeignScan, and the latter to set values for these system
attributes during IterateForeignScan (InvalidTransactionId for
xmins/xmaxs, InvalidCommandId for cmins/cmaxs, and valid values for
tableoids). Attached is a proposed patch for that. I also slightly
simplified the changes to make_tuple_from_result_row and
conversion_error_callback made by the postgres_fdw join pushdown patch.
What do you think about that?

Best regards,
Etsuro Fujita

On 2016/03/29 15:37, Etsuro Fujita wrote:

I added two helper functions: GetFdwScanTupleExtraData and
FillFdwScanTupleSysAttrs. The FDW author could use the former to get
info about system attributes other than ctids and oids in fdw_scan_tlist
during BeginForeignScan, and the latter to set values for these system
attributes during IterateForeignScan (InvalidTransactionId for
xmins/xmaxs, InvalidCommandId for cmins/cmaxs, and valid values for
tableoids). Attached is a proposed patch for that. I also slightly
simplified the changes to make_tuple_from_result_row and
conversion_error_callback made by the postgres_fdw join pushdown patch.
What do you think about that?

I revised comments a little bit. Attached is an updated version of the
patch. I think this issue should be fixed in advance of the PostgreSQL
9.6beta1 release.

Best regards,
Etsuro Fujita

With this patch, all instances of tableoid, cmin, cmax etc. will get a
non-NULL value irrespective of whether they appear on nullable side of the
join or not.

e.g. select t1.c1, t1.tableoid, t2.c1, t2.tableoid from ft4 t1 left join
ft5 t2 on (t1.c1 = t2.c1); run in contrib_regression gives output
c1 | tableoid | c1 | tableoid
-----+----------+----+----------
2 | 54282 | | 54285
4 | 54282 | | 54285
6 | 54282 | 6 | 54285
8 | 54282 | | 54285
10 | 54282 | | 54285
12 | 54282 | 12 | 54285

but the same query run on local tables select t1.c1, t1.tableoid, t2.c1,
t2.tableoid from "S 1"."T 3" t1 left join "S 1"."T 4" t2 on (t1.c1 =
t2.c1); gives output
c1 | tableoid | c1 | tableoid
-----+----------+----+----------
2 | 54258 | |
4 | 54258 | |
6 | 54258 | 6 | 54266
8 | 54258 | |
10 | 54258 | |
12 | 54258 | 12 | 54266

BTW, why do we want to set the column values with invalid values, and not
null? Wouldn't setting them NULL will be a better way?

On Tue, Apr 5, 2016 at 12:11 PM, Etsuro Fujita <fujita.etsuro@lab.ntt.co.jp>
wrote:

On 2016/03/29 15:37, Etsuro Fujita wrote:

I added two helper functions: GetFdwScanTupleExtraData and
FillFdwScanTupleSysAttrs. The FDW author could use the former to get
info about system attributes other than ctids and oids in fdw_scan_tlist
during BeginForeignScan, and the latter to set values for these system
attributes during IterateForeignScan (InvalidTransactionId for
xmins/xmaxs, InvalidCommandId for cmins/cmaxs, and valid values for
tableoids). Attached is a proposed patch for that. I also slightly
simplified the changes to make_tuple_from_result_row and
conversion_error_callback made by the postgres_fdw join pushdown patch.
What do you think about that?

I revised comments a little bit. Attached is an updated version of the
patch. I think this issue should be fixed in advance of the PostgreSQL
9.6beta1 release.

Best regards,
Etsuro Fujita

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Tue, Apr 05, 2016 at 03:41:00PM +0900, Etsuro Fujita wrote:

On 2016/03/29 15:37, Etsuro Fujita wrote:

I added two helper functions: GetFdwScanTupleExtraData and
FillFdwScanTupleSysAttrs. The FDW author could use the former to get
info about system attributes other than ctids and oids in fdw_scan_tlist
during BeginForeignScan, and the latter to set values for these system
attributes during IterateForeignScan (InvalidTransactionId for
xmins/xmaxs, InvalidCommandId for cmins/cmaxs, and valid values for
tableoids). Attached is a proposed patch for that. I also slightly
simplified the changes to make_tuple_from_result_row and
conversion_error_callback made by the postgres_fdw join pushdown patch.
What do you think about that?

I revised comments a little bit. Attached is an updated version of the
patch. I think this issue should be fixed in advance of the PostgreSQL
9.6beta1 release.

Of the foreign table columns affected here, I bet only tableoid sees
non-negligible use. Even tableoid is not very prominent, so I would not have
thought of this as a beta1 blocker. What about this bug makes pre-beta1
resolution especially valuable?

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

On Wed, Apr 06, 2016 at 01:14:34AM -0400, Noah Misch wrote:

On Tue, Apr 05, 2016 at 03:41:00PM +0900, Etsuro Fujita wrote:

On 2016/03/29 15:37, Etsuro Fujita wrote:

I added two helper functions: GetFdwScanTupleExtraData and
FillFdwScanTupleSysAttrs. The FDW author could use the former to get
info about system attributes other than ctids and oids in fdw_scan_tlist
during BeginForeignScan, and the latter to set values for these system
attributes during IterateForeignScan (InvalidTransactionId for
xmins/xmaxs, InvalidCommandId for cmins/cmaxs, and valid values for
tableoids). Attached is a proposed patch for that. I also slightly
simplified the changes to make_tuple_from_result_row and
conversion_error_callback made by the postgres_fdw join pushdown patch.
What do you think about that?

I revised comments a little bit. Attached is an updated version of the
patch. I think this issue should be fixed in advance of the PostgreSQL
9.6beta1 release.

Of the foreign table columns affected here, I bet only tableoid sees
non-negligible use. Even tableoid is not very prominent, so I would not have
thought of this as a beta1 blocker. What about this bug makes pre-beta1
resolution especially valuable?

This will probably get resolved earlier if it enters the process now as a non
beta blocker, compared to entering the process later as a beta blocker. I'm
taking the liberty of doing that:

[This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item. Robert,
since you committed the patch believed to have created it, you own this open
item. If that responsibility lies elsewhere, please let us know whose
responsibility it is to fix this. Since new open items may be discovered at
any time and I want to plan to have them all fixed well in advance of the ship
date, I will appreciate your efforts toward speedy resolution. Please
present, within 72 hours, a plan to fix the defect within seven days of this
message. Thanks.

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