Re: Writable foreign tables: how to identify rows

Kohei KaiGai <kaigai@kaigai.gr.jp> writes:
> 2013/3/6 Tom Lane <tgl@sss.pgh.pa.us>:
>> I think if we're going to support magic row identifiers, they need to
>> be actual system columns, complete with negative attnums and entries
>> in pg_attribute.

> Sorry, -1 for me.

> The proposed design tried to kill two birds with one stone.
> The reason why the new GetForeignRelWidth() can reserve multiple slot
> for pseudo-columns is, that intends to push-down complex calculation in
> target-list to the remote computing resource.

[ shrug... ]  That's just pie in the sky: there is no such capability
in the submitted patch, nor is it close to being implementable.  When
weighing that against the very real probability that this patch won't
get into 9.3 at all, I have no problem tossing that overboard to try to
get to something committable.

The larger issue here is that the patch is confusing the set of
possibly-computed columns that could be returned by a scan node with
the catalog-specified set of columns for a table.  I don't think that
changing the (representation of the) latter on the fly within the
planner is a tenable approach.  You've had to put in an unreasonable
number of crude hacks already to make that sort-of work, but I have
exactly zero confidence that you've hacked everyplace that would have
to change.  I also have no confidence that there aren't unfixable
problems where the same TupleDesc would need to be in both states
to satisfy the expectations of different bits of code.  (An example
of the risks here is that, IIRC, parts of the planner use the relation's
TupleDesc as a guide to what "relname.*" means.  So I'm pretty
suspicious that the existing patch breaks behavior for whole-row Vars
in some cases.)  Really the idea is a bit broken in this form anyway,
because if we did have a plan that involved calculating "(x-y)^2" at
the scan level, we'd want that expression to be represented using Vars
for x and y, not some made-up Var whose meaning is not apparent from
the system catalogs.

Also, the right way to deal with this desire is to teach the planner in
general, not just FDWs, about pushing expensive calculations down the
plan tree --- basically, resurrecting Joe Hellerstein's thesis work,
which we ripped out more than ten years ago.  I don't think there's that
much that would need to change about the planner's data structures, but
deciding where is cheapest to evaluate an expression is a pretty hard
problem.  Trying to handle that locally within FDWs is doomed to failure
IMO.

So my intention is to get rid of GetForeignRelWidth() and make use of
the existing ctid system column for returning remote TIDs in
postgres_fdw.  (On review I notice that we're creating ctid columns
for foreign tables already, so we don't even need the proposed hook
to let FDWs control that; though we will certainly want one in future
if we are to support non-TID magic row identifiers.)

If you find that unacceptable, I'm quite willing to mark this patch
Returned With Feedback and get on with dealing with some of the other
forty-odd patches in the CF queue.
        regards, tom lane