Re: Asynchronous MergeAppend

Hi.

Matheus Alcantara писал(а) 2025-11-18 00:09:
> On Sat Nov 15, 2025 at 7:57 AM -03, Alexander Pyhalov wrote:
>>> Here are some comments on my first look at the patches. I still don't
>>> have too much experience with the executor code but I hope that I can
>>> help with something.
>>>
>>> v5-0001-mark_async_capable-subpath-should-match-subplan.patch
>>>
>>> I don't have to much comments on this, perhaps we could have a commit
>>> message explaining the reason behind the change.
>>
>> I've expanded commit message. The issue is that mark_async_capable()
>> relies
>> on the fact that plan node type corresponds to path type. This is not
>> true when
>> (Merge)Append decides to insert Sort node.
>>
> Your explanation about why this change is needed that you've include on
> your first email sounds more clear IMHO. I would suggest the following
> for a commit message:
>     mark_async_capable() believes that path corresponds to plan. This
> is
>     not true when create_[merge_]append_plan() inserts sort node. In
>     this case mark_async_capable() can treat Sort plan node as some
>     other and crash. Fix this by handling the Sort node separately.
>
>     This is needed to make MergeAppend node async-capable that it will
>     be implemented in a next commit.
>
> What do you think?
>

Seems to be OK.

> I was reading the patch changes again and I have a minor point:
>
>                                 SubqueryScan *scan_plan = (SubqueryScan
> *) plan;
>
>                                 /*
> -                                * If the generated plan node includes
> a gating Result node,
> -                                * we can't execute it asynchronously.
> +                                * If the generated plan node includes
> a gating Result node or
> +                                * a Sort node, we can't execute it
> asynchronously.
>                                  */
> -                               if (IsA(plan, Result))
> +                               if (IsA(plan, Result) || IsA(plan,
> Sort))
>
> Shouldn't we cast the plan to SubqueryScan* after the IsA(...) check? I
> know this code has been before your changes but type casting before a
> IsA() check sounds a bit strange to me. Also perhaps we could add an
> Assert(IsA(plan, SubqueryScan)) after the IsA(...) check and before the
> type casting just for sanity?

Yes, checking for node not to be A and then using it as B seems to be
strange. But casting to another type and checking if node is of a
particular type before using seems to be rather common. It doesn't do
any harm if we don't actually refer to SubqueryScan fields.

Updated the first patch.


--
Best regards,
Alexander Pyhalov,
Postgres Professional