Re: dropping datumSort field

On Tue, Aug 9, 2022 at 8:24 AM Robert Haas <robertmhaas(at)gmail(dot)com> wrote:

> On Tue, Aug 9, 2022 at 11:16 AM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
> > tupDesc is declared inside `if (!node->sort_Done)` block whereas the
> last reference to tupDesc is outside the if block.
>
> Yep.
>
> > I take your review comment and will go back to do more homework.
>
> The real point for me here is you haven't offered any reason to make
> this change. The structure member in question is basically free.
> Because of alignment padding it uses no more memory, and it makes the
> intent of the code clearer.
>
> Let's not change things just because we could.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com

I should have provided motivation for the patch.

I was aware of recent changes around ExprEvalStep. e.g.

Remove unused fields from ExprEvalStep

Though the datumSort field may be harmless for now, in the future, the
(auto) alignment padding may not work if more fields are added to the
struct.
I think we should always leave some room in struct's for future expansion.

As for making the intent of the code clearer, the datumSort field is only
used in one method.
My previous patch removed some comment which should have been shifted into
this method.
In my opinion, localizing the check in single method is easier to
understand than resorting to additional struct field.

Cheers