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

On 2016/03/17 22:15, Ashutosh Bapat wrote:
> On Thu, Mar 17, 2016 at 4:30 PM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp <mailto:fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)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