Postgres Pro
Downloads
Home   >   mailing lists

Re: dropping datumSort field - Mailing list pgsql-hackers

From Zhihong Yu
Subject Re: dropping datumSort field
Date
Msg-id CALNJ-vT3dk1TMZm2Nsik7sHkD2QqMr-0pwLAFwnJ_wTYiTCBJw@mail.gmail.com
Whole thread Raw
In response to Re: dropping datumSort field  (Robert Haas <robertmhaas@gmail.com>)
Responses Re: dropping datumSort field
List pgsql-hackers
Tree view


On Tue, Aug 9, 2022 at 8:24 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Aug 9, 2022 at 11:16 AM Zhihong Yu <zyu@yugabyte.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

pgsql-hackers by date:

Previous
From: Junwang Zhao
Date:
Subject: remove useless comments
Next
From: Tom Lane
Date:
Subject: Re: remove useless comments