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