Re: POC: postgres_fdw insert batching

On 1/13/21 10:15 AM, Amit Langote wrote:
> Hi Tomas, Tsunakawa-san,
> 
> Thanks for your work on this.
> 
> On Tue, Jan 12, 2021 at 11:06 AM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>> AFAICS the discussions about making this use COPY and/or libpq
>> pipelining (neither of which is committed yet) ended with the conclusion
>> that those changes are somewhat independent, and that it's worth getting
>> this committed in the current form. Barring objections, I'll push this
>> within the next couple days.
> 
> I was trying this out today (been meaning to do so for a while) and
> noticed that this fails when there are AFTER ROW triggers on the
> foreign table.  Here's an example:
> 
> create extension postgres_fdw ;
> create server lb foreign data wrapper postgres_fdw ;
> create user mapping for current_user server lb;
> create table p (a numeric primary key);
> create foreign table fp (a int) server lb options (table_name 'p');
> create function print_row () returns trigger as $$ begin raise notice
> '%', new; return null; end; $$ language plpgsql;
> create trigger after_insert_trig after insert on fp for each row
> execute function print_row();
> insert into fp select generate_series (1, 10);
> <crashes>
> 
> Apparently, the new code seems to assume that batching wouldn't be
> active when the original query contains RETURNING clause but some
> parts fail to account for the case where RETURNING is added to the
> query to retrieve the tuple to pass to the AFTER TRIGGER.
> Specifically, the Assert in the following block in
> execute_foreign_modify() is problematic:
> 
>     /* Check number of rows affected, and fetch RETURNING tuple if any */
>     if (fmstate->has_returning)
>     {
>         Assert(*numSlots == 1);
>         n_rows = PQntuples(res);
>         if (n_rows > 0)
>             store_returning_result(fmstate, slots[0], res);
>     }
> 

Thanks for the report. Yeah, I think there's a missing check in
ExecInsert. Adding

  (!resultRelInfo->ri_TrigDesc->trig_insert_after_row)

solves this. But now I'm wondering if this is the wrong place to make
this decision. I mean, why should we make the decision here, when the
decision whether to have a RETURNING clause is made in postgres_fdw in
deparseReturningList? We don't really know what the other FDWs will do,
for example.

So I think we should just move all of this into GetModifyBatchSize. We
can start with ri_BatchSize = 0. And then do

  if (resultRelInfo->ri_BatchSize == 0)
    resultRelInfo->ri_BatchSize =
      resultRelInfo->ri_FdwRoutine->GetModifyBatchSize(resultRelInfo);

  if (resultRelInfo->ri_BatchSize > 1)
  {
    ... do batching ...
  }

The GetModifyBatchSize would always return value > 0, so either 1 (no
batching) or >1 (batching).


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company