Re: Odd behavior in foreign table modification (Was: Re: Optimization for updating foreign tables in Postgres FDW) - Mailing list pgsql-hackers

On 2016/01/20 3:42, Robert Haas wrote:
> On Tue, Jan 19, 2016 at 1:59 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.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