Hi,
On Wed, Aug 10, 2022 at 1:06 AM Zhihong Yu <z...@yugabyte.com> wrote:
> On Tue, Aug 9, 2022 at 4:45 AM Etsuro Fujita <etsuro.fuj...@gmail.com> wrote:
>> * When running AFTER ROW triggers in CopyMultiInsertBufferFlush(), the
>> patch uses the slots passed to ExecForeignBatchInsert(), not the ones
>> returned by the callback function, but I don't think that that is
>> always correct, as the documentation about the callback function says:
>>
>> The return value is an array of slots containing the data that was
>> actually inserted (this might differ from the data supplied, for
>> example as a result of trigger actions.)
>> The passed-in <literal>slots</literal> can be re-used for this purpose.
>>
>> postgres_fdw re-uses the passed-in slots, but other FDWs might not, so
>> I modified the patch to reference the returned slots when running the
>> AFTER ROW triggers.
I noticed that my explanation was not correct. Let me explain.
Before commit 82593b9a3, when batching into a view referencing a
postgres_fdw foreign table that has WCO constraints, postgres_fdw used
the passed-in slots to store the first tuple that was actually
inserted to the remote table. But that commit disabled batching in
that case, so postgres_fdw wouldn’t use the passed-in slots (until we
support batching when there are WCO constraints from the parent views
and/or AFTER ROW triggers on the foreign table).
> + /* If any rows were inserted, run AFTER ROW INSERT triggers. */
> ...
> + for (i = 0; i < inserted; i++)
> + {
> + TupleTableSlot *slot = rslots[i];
> ...
> + slot->tts_tableOid =
> + RelationGetRelid(resultRelInfo->ri_RelationDesc);
>
> It seems the return value of
> `RelationGetRelid(resultRelInfo->ri_RelationDesc)` can be stored in a
> variable outside the for loop.
> Inside the for loop, assign this variable to slot->tts_tableOid.
Actually, I did this to match the code in ExecBatchInsert(), but that
seems like a good idea, so I’ll update the patch as such in the next
version.
Thanks for reviewing!
Best regards,
Etsuro Fujita
