On Tue Nov 4, 2025 at 9:16 PM -03, David Rowley wrote:
> On Wed, 5 Nov 2025 at 08:51, Matheus Alcantara <matheusssilv97@gmail.com> wrote:
>>
>> On Mon Nov 3, 2025 at 7:47 PM -03, David Rowley wrote:
>> > Are you sure you've not got something else in your branch? It applies
>> > ok here, and the CFbot isn't complaining either. CFBot's is based on
>> > cf8be0225, which is 2 commits before the one you're trying, but
>> > src/test/regress/expected/aggregates.out hasn't been changed since
>> > 2025-10-07.
>> >
>> Yes, my branch is clean, I even tried to apply on a cleaned git clone
>> but it is still failling to apply, very strange. I've added the cfbot
>> remote and cherry picked your commit and this works. I'll investigate
>> later why I'm not able to apply your patch directly.
>
> Did you look at: git diff origin/master..master ?
> I've certainly accidentally periodically committed to my local master
> which I ended up doing: git reset --hard origin/master to fix
>
Yes, I ran git reset before trying to apply the v2 and it still had
conflicts, very strange. Anyway the v3 applied clean on my environment
now.
>> The code seems good to me, I don't have too many comments, I'm just not
>> sure if we should keep the #ifdef NOT_USED block but I'm not totally
>> against it. I'm +1 for the idea.
>
> Thanks for the review. I might not have been clear that I had only
> intended the NOT_USED part as an example for during the review period.
> I'd never intended it going any further.
>
Ok, it make sense now, thanks for making it clear.
> I've attached a version with the NOT_USED part removed (and a bunch of
> #includes I forgot to remove). The only other change was a minor
> revision to some comments.
>
Thanks, it looks cleaner.
> The primary concern I have now is when in planning that we do this
> Aggref simplification. Maybe I shouldn't be too concerned about that
> as there doesn't seem to be a current reason not to put it where it
> is. If someone comes up with a reason to do it later in planning at
> some point in the future, we can consider moving it then. That sort of
> excludes extensions with aggregates that want to have a
> SupportRequestSimplifyAggref support function that might need the
> processing done later in planning, but that just feels like a
> situation that's unlikely to arise.
>
I think it's ok to leave where it is implemented now and it make sense
to me. The SupportRequestSimplifyAggref is similar with
SupportRequestSimplify which is used by simplify_function() that is
called at eval_const_expressions_mutator(), the simplify_aggref() is
also called at the same function, so it seems to be consistent.
--
Matheus Alcantara
EDB: http://www.enterprisedb.com
