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
