On 1/13/21 3:43 PM, Tomas Vondra wrote:
>
> ...
>
> 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).
>
FWIW the attached v8 patch does this - most of the conditions are moved
to the GetModifyBatchSize() callback. I've removed the check for the
BatchInsert callback, though - the FDW knows whether it supports that,
and it seems a bit pointless at the moment as there are no other batch
callbacks. Maybe we should add an Assert somewhere, though?
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
