Re: Odd behavior in foreign table modification (Was: Re: Optimization for updating foreign tables in Postgres FDW)

On 2016/01/20 3:42, Robert Haas wrote:
> On Tue, Jan 19, 2016 at 1:59 AM, Etsuro Fujita
> <fujita(dot)etsuro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>>>>> I've run into an issue:
>>>>>
>>>>> *# UPDATE master_customers SET id = 22 WHERE id = 16 RETURNING
>>>>> tableoid::regclass;
>>>>> ERROR:
>>>>> CONTEXT: Remote SQL command: UPDATE public.customers SET id = 22
>>>>> WHERE ((id = 16)) RETURNING NULL

>>> While working on this, I noticed that the existing postgres_fdw system
>>> shows similar behavior, so I changed the subject.
>>>
>>> IIUC, the reason for that is when the local query specifies "RETURNING
>>> tableoid::regclass", the FDW has fmstate->has_returning=false while the
>>> remote query executed at ModifyTable has "RETURNING NULL", as shown in
>>> the above example; that would cause an abnormal exit in executing the
>>> remote query in postgresExecForeignUpdate, since that the FDW would get
>>> PGRES_TUPLES_OK as a result of the query while the FDW would think that
>>> the right result to get should be PGRES_COMMAND_OK, from the flag
>>> fmstate->has_returning=false.
>>>
>>> Attached is a patch to fix that.

>> I added this to the next CF.
>>
>> https://commitfest.postgresql.org/9/483/

> Uggh, what a mess. How about passing an additional boolean
> "is_returning" to deparseTargetList()? If false, then
> deparseTargetList() behaves as now. If false, then
> deparseTargetList() doesn't append anything at all if there are no
> columns to return, instead of (as at present) adding NULL. On the
> other hand, if there are columns to return, then it appends "
> RETURNING " before the first column. Then, deparseReturningList could
> skip adding RETURNING itself, and just pass true to
> deparseTargetList(). The advantage of this approach is that we don't
> end up with two copies of the code that have to stay synchronized -

Thanks for the review! I think that is important.

> the decision is made inside deparseTargetList(), and
> deparseReturningList() accepts the results.

My concern about that is that would make the code in deparseTargetList()
complicated.

Essentially, I think your propossal needs a two-pass algorithm for
deparseTargetList; (1) create an integer List of the columns being
retrieved from the given attrs_used (getRetrievedAttrs()), and (2) print
those columns (printRetrievedAttrs()). How about sharing those two
functions between deparseTargetList and deparseReturningList?:

* In deparseTargetList, perform getRetrievedAttrs(). If
getRetrievedAttrs()!=NIL, perform printRetrievedAttrs(). Otherwise,
print NULL.
* In deparseReturningList, perform getRetrievedAttrs() before adding
RETURNING. If getRetrievedAttrs()!=NIL, print RETURNING and perform
printRetrievedAttrs(). Otherwise, do nothing.

Best regards,
Etsuro Fujita