Re: Odd system-column handling in postgres_fdw join pushdown patch

On Thu, Mar 17, 2016 at 4:30 PM, Etsuro Fujita <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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