rename es_epq_active to es_epqstate
Hi hackers,
While reviewing the ExecSeqScan optimizations patch[1]/messages/by-id/CA+HiwqF+oydVreYN3Xp7U6x_LKi9ZL+No2X6WUv8X_kN+yHSLA@mail.gmail.com, I found that
es_epq_active might not be well named, my intuition told me that this
is a boolean field because of the "active" suffix.
es_epq_active was introduced in 27cc7cd, in the original discussion[2]/messages/by-id/20190828030201.v5u76ty47mtw2efp@alap3.anarazel.de,
Tom and Andres discussed the field name "es_active_epq" a little bit,
let me quote some:
------- quoted content begin
Also I dislike the field name "es_active_epq", as what that suggests
to me is exactly backwards from the way you apparently are using it.
Maybe "es_parent_epq" instead? The comment for it is pretty opaque
as well, not to mention that it misspells EState.
I think what I was trying to signal was that EPQ is currently active
from the POV of executor nodes. Ought to have been es_epq_active, for
that, I guess. For me "if (estate->es_epq_active)" explains the meaning
of the check more than "if (estate->es_parent_epq)".
I went with es_epq_active, as I suggested in my earlier email, which
still seems accurate to me.
I really want to move consideration of es_ep_active to ExecInitNode()
time, rather than execution time. If we add an execScan helper, we can
have it set the corresponding executor node's ExecProcNode to
a) a function that performs qual checks and projection
b) a function that performs projection
c) the fetch method from the scan node
d) basically the current ExecScan, when es_epq_active
-------- quoted content end
ISTM Andres tend to use *es_epq_active* in a boolean way,
like `if (es_epq_active) then`, but in the code base, all its usages
follow pattern `if (es_epq_active == NULL) then`, so I propose to
change es_epq_active to es_epqstate.
[1]: /messages/by-id/CA+HiwqF+oydVreYN3Xp7U6x_LKi9ZL+No2X6WUv8X_kN+yHSLA@mail.gmail.com
[2]: /messages/by-id/20190828030201.v5u76ty47mtw2efp@alap3.anarazel.de
--
Regards
Junwang Zhao
Attachments:
v1-0001-rename-es_epq_active-to-es_epqstate.patchapplication/octet-stream; name=v1-0001-rename-es_epq_active-to-es_epqstate.patchDownload+22-23
Junwang Zhao <zhjwpku@gmail.com> writes:
ISTM Andres tend to use *es_epq_active* in a boolean way,
like `if (es_epq_active) then`, but in the code base, all its usages
follow pattern `if (es_epq_active == NULL) then`, so I propose to
change es_epq_active to es_epqstate.
While I didn't especially love "es_epq_active" at the time,
I don't see that "es_epqstate" is much of an improvement:
it's an extremely generic name that conveys little information.
And renaming it now, years later, seems to add little except
back-patching hazards. So I'd vote for leaving it alone.
regards, tom lane
On Sat, Jan 18, 2025 at 9:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Junwang Zhao <zhjwpku@gmail.com> writes:
ISTM Andres tend to use *es_epq_active* in a boolean way,
like `if (es_epq_active) then`, but in the code base, all its usages
follow pattern `if (es_epq_active == NULL) then`, so I propose to
change es_epq_active to es_epqstate.While I didn't especially love "es_epq_active" at the time,
I don't see that "es_epqstate" is much of an improvement:
it's an extremely generic name that conveys little information.
And renaming it now, years later, seems to add little except
back-patching hazards. So I'd vote for leaving it alone.regards, tom lane
Ok, let's keep it as is, thanks for the explanation.
--
Regards
Junwang Zhao