On 2020-09-09 11:45, Andrey V. Lepikhov wrote:
On 9/8/20 8:34 PM, Alexey Kondratov wrote:
On 2020-09-08 17:00, Amit Langote wrote:
<a.kondra...@postgrespro.ru> wrote:
On 2020-09-08 10:34, Amit Langote wrote:
Another ambiguous part of the refactoring was in changing
InitResultRelInfo() arguments:
@@ -1278,6 +1280,7 @@ InitResultRelInfo(ResultRelInfo
*resultRelInfo,
Relation resultRelationDesc,
Index resultRelationIndex,
Relation partition_root,
+ bool use_multi_insert,
int instrument_options)
Why do we need to pass this use_multi_insert flag here? Would it be
better to set resultRelInfo->ri_usesMultiInsert in the
InitResultRelInfo() unconditionally like it is done for
ri_usesFdwDirectModify? And after that it will be up to the caller
whether to use multi-insert or not based on their own circumstances.
Otherwise now we have a flag to indicate that we want to check for
another flag, while this check doesn't look costly.
Hmm, I think having two flags seems confusing and bug prone,
especially if you consider partitions. For example, if a partition's
ri_usesMultiInsert is true, but CopyFrom()'s local flag is false,
then
execPartition.c: ExecInitPartitionInfo() would wrongly perform
BeginForeignCopy() based on only ri_usesMultiInsert, because it
wouldn't know CopyFrom()'s local flag. Am I missing something?
No, you're right. If someone want to share a state and use
ResultRelInfo (RRI) for that purpose, then it's fine, but CopyFrom()
may simply override RRI->ri_usesMultiInsert if needed and pass this
RRI further.
This is how it's done for RRI->ri_usesFdwDirectModify.
InitResultRelInfo() initializes it to false and then
ExecInitModifyTable() changes the flag if needed.
Probably this is just a matter of personal choice, but for me the
current implementation with additional argument in InitResultRelInfo()
doesn't look completely right. Maybe because a caller now should pass
an additional argument (as false) even if it doesn't care about
ri_usesMultiInsert at all. It also adds additional complexity and
feels like abstractions leaking.
I didn't feel what the problem was and prepared a patch version
according to Alexey's suggestion (see Alternate.patch).
Yes, that's very close to what I've meant.
+ leaf_part_rri->ri_usesMultiInsert = (leaf_part_rri->ri_usesMultiInsert
&&
+ rootResultRelInfo->ri_usesMultiInsert) ? true : false;
This could be just:
+ leaf_part_rri->ri_usesMultiInsert = (leaf_part_rri->ri_usesMultiInsert
&&
+ rootResultRelInfo->ri_usesMultiInsert);
This does not seem very convenient and will lead to errors in the
future. So, I agree with Amit.
And InitResultRelInfo() may set ri_usesMultiInsert to false by default,
since it's used only by COPY now. Then you won't need this in several
places:
+ resultRelInfo->ri_usesMultiInsert = false;
While the logic of turning multi-insert on with all the validations
required could be factored out of InitResultRelInfo() to a separate
routine.
Anyway, I don't insist at all and think it's fine to stick to the
original v7's logic.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
