useless LIMIT_OPTION_DEFAULT
Hi, all
By reading the codes, I found that we process limit option as LIMIT_OPTION_WITH_TIES when using WITH TIES
and all others are LIMIT_OPTION_COUNT by commit 357889eb17bb9c9336c4f324ceb1651da616fe57.
And check actual limit node in limit_needed().
There is no need to maintain a useless default limit enum.
I remove it and have an install check to verify.
Are there any considerations behind this?
Shall we remove it for clear as it’s not actually the default option.
Zhang Mingli
www.hashdata.xyz
Attachments:
v1-0001-Remove-useless-LIMIT_OPTION_DEFAULT.patchapplication/octet-streamDownload
From 81e8224d6853ec9b603d622f29d95c0e4e3e7aac Mon Sep 17 00:00:00 2001
From: Zhang Mingli <avamingli@gmail.com>
Date: Thu, 14 Dec 2023 08:58:05 +0800
Subject: [PATCH] Remove useless LIMIT_OPTION_DEFAULT.
We process limit option as LIMIT_OPTION_WITH_TIES when using WITH TIES.
All others are LIMIT_OPTION_COUNT.
And check actual limit node in limit_needed().
There is no need to maintain a useless default limit enum.
Authored-by: Zhang Mingli avamingli@gmail.com
---
src/backend/parser/gram.y | 2 +-
src/include/nodes/nodes.h | 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index f16bbd3cdd..63f172e175 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -18461,7 +18461,7 @@ insertSelectOptions(SelectStmt *stmt,
parser_errposition(exprLocation(limitClause->limitCount))));
stmt->limitCount = limitClause->limitCount;
}
- if (limitClause && limitClause->limitOption != LIMIT_OPTION_DEFAULT)
+ if (limitClause)
{
if (stmt->limitOption)
ereport(ERROR,
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 4c32682e4c..6e776a8aa6 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -439,8 +439,7 @@ typedef enum OnConflictAction
typedef enum LimitOption
{
LIMIT_OPTION_COUNT, /* FETCH FIRST... ONLY */
- LIMIT_OPTION_WITH_TIES, /* FETCH FIRST... WITH TIES */
- LIMIT_OPTION_DEFAULT, /* No limit present */
+ LIMIT_OPTION_WITH_TIES /* FETCH FIRST... WITH TIES */
} LimitOption;
#endif /* NODES_H */
--
2.34.1
Import Notes
Reply to msg id not found: b4f097ce-6422-49d7-888a-2e0a7a18f5f2@SparkReference msg id not found: b4f097ce-6422-49d7-888a-2e0a7a18f5f2@Spark
Zhang Mingli <zmlpostgres@gmail.com> writes:
By reading the codes, I found that we process limit option as LIMIT_OPTION_WITH_TIES when using WITH TIES
and all others are LIMIT_OPTION_COUNT by commit 357889eb17bb9c9336c4f324ceb1651da616fe57.
And check actual limit node in limit_needed().
There is no need to maintain a useless default limit enum.
I agree, that looks pretty useless. Our normal convention for
representing not having any LIMIT clause would be to not create
a SelectLimit node at all. I don't see why allowing a second
representation is a good idea, especially when the code is not
in fact doing that.
git blame shows that this came in with 357889eb1. Alvaro,
Surafel, do you want to argue for keeping things as-is?
regards, tom lane
On 2023-Dec-14, Tom Lane wrote:
Zhang Mingli <zmlpostgres@gmail.com> writes:
By reading the codes, I found that we process limit option as LIMIT_OPTION_WITH_TIES when using WITH TIES
and all others are LIMIT_OPTION_COUNT by commit 357889eb17bb9c9336c4f324ceb1651da616fe57.
And check actual limit node in limit_needed().
There is no need to maintain a useless default limit enum.I agree, that looks pretty useless. Our normal convention for
representing not having any LIMIT clause would be to not create
a SelectLimit node at all. I don't see why allowing a second
representation is a good idea, especially when the code is not
in fact doing that.git blame shows that this came in with 357889eb1. Alvaro,
Surafel, do you want to argue for keeping things as-is?
I looked at the history of this. That enum member first appeared as a
result of your review at [1]/messages/by-id/3277.1567722389@sss.pgh.pa.us, and the accompanying code at that time did
use it a few times. The problem is that I later ([2]/messages/by-id/20191125203442.GA30191@alvherre.pgsql) proposed to rewrite
that code and remove most of the uses of that enum member, but failed to
remove it completely.
So, I think we're OK to remove it. I'm going to push Zhang's patch
soon unless there are objections.
[1]: /messages/by-id/3277.1567722389@sss.pgh.pa.us
[2]: /messages/by-id/20191125203442.GA30191@alvherre.pgsql
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"Doing what he did amounts to sticking his fingers under the hood of the
implementation; if he gets his fingers burnt, it's his problem." (Tom Lane)