Date: Mon, 25 Nov 2019 17:34:42 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Tomas Vondra <tomas.vondra@2ndquadrant.com>
Cc: Surafel Temesgen <surafel3000@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@postgresql.org>,
	Andrew Gierth <andrew@tao11.riddles.org.uk>
Subject: Re: FETCH FIRST clause WITH TIES option
Message-ID: <20191125203442.GA30191@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="X1bOJ3K7DJ5YkBrT"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20191111145649.GA7829@alvherre.pgsql>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk


--X1bOJ3K7DJ5YkBrT
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2019-Nov-11, Alvaro Herrera wrote:

> I'm not sure the proposed changes to gram.y are all that great, though.

Here's a proposed simplification of the gram.y changes.  There are two
things here:

1. cosmetic: we don't need the LimitClause struct; we can use just
   SelectLimit, and return that from limit_clause; that can be
   complemented using the offset_clause if there's any at select_limit
   level.

2. there's a gratuituous palloc() in opt_select_limit when there's no
   clause, that seems to be there just so that NULLs can be returned.
   That's one extra palloc for SELECTs parsed using one the affected
   productions ... it's not every single select, but it seems bad enough
   it's worth fixing.

I fixed #2 by just checking whether the return from opt_select_limit is
NULL.  ISTM it'd be better to pass the SelectLimit pointer to
insertSelectOptions instead (which is a static function in gram.y anyway
so there's no difficulty there).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--X1bOJ3K7DJ5YkBrT
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="with_ties_gram.patch"

diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index e776215155..f4304a45b9 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -135,13 +135,6 @@ typedef struct SelectLimit
 	LimitOption limitOption;
 } SelectLimit;
 
-/* Private struct for the result of limit_clause production */
-typedef struct LimitClause
-{
-	Node *limitCount;
-	LimitOption limitOption;
-} LimitClause;
-
 /* ConstraintAttributeSpec yields an integer bitmask of these flags: */
 #define CAS_NOT_DEFERRABLE			0x01
 #define CAS_DEFERRABLE				0x02
@@ -258,8 +251,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 	PartitionSpec		*partspec;
 	PartitionBoundSpec	*partboundspec;
 	RoleSpec			*rolespec;
-	struct SelectLimit	*SelectLimit;
-	struct LimitClause	*LimitClause;
+	struct SelectLimit	*selectlimit;
 }
 
 %type <node>	stmt schema_stmt
@@ -392,8 +384,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 %type <ival>	import_qualification_type
 %type <importqual> import_qualification
 %type <node>	vacuum_relation
-%type <SelectLimit> opt_select_limit select_limit
-%type <LimitClause> limit_clause
+%type <selectlimit> opt_select_limit select_limit limit_clause
 
 %type <list>	stmtblock stmtmulti
 				OptTableElementList TableElementList OptInherit definition
@@ -11343,8 +11334,9 @@ select_no_parens:
 			| select_clause opt_sort_clause for_locking_clause opt_select_limit
 				{
 					insertSelectOptions((SelectStmt *) $1, $2, $3,
-										($4)->limitOffset, ($4)->limitCount,
-										($4)->limitOption,
+										($4) ? ($4)->limitOffset : NULL,
+										($4) ? ($4)->limitCount : NULL,
+										($4) ? ($4)->limitOption : LIMIT_OPTION_DEFAULT,
 										NULL,
 										yyscanner);
 					$$ = $1;
@@ -11362,7 +11354,7 @@ select_no_parens:
 				{
 					insertSelectOptions((SelectStmt *) $2, NULL, NIL,
 										NULL, NULL,
-										LIMIT_OPTION_DEFAULT,$1,
+										LIMIT_OPTION_DEFAULT, $1,
 										yyscanner);
 					$$ = $2;
 				}
@@ -11370,15 +11362,16 @@ select_no_parens:
 				{
 					insertSelectOptions((SelectStmt *) $2, $3, NIL,
 										NULL, NULL,
-										LIMIT_OPTION_DEFAULT,$1,
+										LIMIT_OPTION_DEFAULT, $1,
 										yyscanner);
 					$$ = $2;
 				}
 			| with_clause select_clause opt_sort_clause for_locking_clause opt_select_limit
 				{
 					insertSelectOptions((SelectStmt *) $2, $3, $4,
-										($5)->limitOffset, ($5)->limitCount,
-										($5)->limitOption,
+										($5) ? ($5)->limitOffset : NULL,
+										($5) ? ($5)->limitCount : NULL,
+										($5) ? ($5)->limitOption : LIMIT_OPTION_DEFAULT,
 										$1,
 										yyscanner);
 					$$ = $2;
@@ -11683,27 +11676,17 @@ sortby:		a_expr USING qual_all_Op opt_nulls_order
 select_limit:
 			limit_clause offset_clause
 				{
-					SelectLimit *n = (SelectLimit *) palloc(sizeof(SelectLimit));
-					n->limitOffset = $2;
-					n->limitCount = ($1)->limitCount;
-					n->limitOption = ($1)->limitOption;
-					$$ = n;
+					$$ = $1;
+					($$)->limitOffset = $2;
 				}
 			| offset_clause limit_clause
 				{
-					SelectLimit *n = (SelectLimit *) palloc(sizeof(SelectLimit));
-					n->limitOffset = $1;
-					n->limitCount = ($2)->limitCount;
-					n->limitOption = ($2)->limitOption;
-					$$ = n;
+					$$ = $2;
+					($$)->limitOffset = $1;
 				}
 			| limit_clause
 				{
-					SelectLimit *n = (SelectLimit *) palloc(sizeof(SelectLimit));
-					n->limitOffset = NULL;
-					n->limitCount = ($1)->limitCount;
-					n->limitOption = ($1)->limitOption;
-					$$ = n;
+					$$ = $1;
 				}
 			| offset_clause
 				{
@@ -11717,20 +11700,14 @@ select_limit:
 
 opt_select_limit:
 			select_limit						{ $$ = $1; }
-			| /* EMPTY */
-				{
-					SelectLimit *n = (SelectLimit *) palloc(sizeof(SelectLimit));
-					n->limitOffset = NULL;
-					n->limitCount = NULL;
-					n->limitOption = LIMIT_OPTION_DEFAULT;
-					$$ = n;
-				}
+			| /* EMPTY */						{ $$ = NULL; }
 		;
 
 limit_clause:
 			LIMIT select_limit_value
 				{
-					LimitClause *n = (LimitClause *) palloc(sizeof(LimitClause));
+					SelectLimit *n = (SelectLimit *) palloc(sizeof(SelectLimit));
+					n->limitOffset = NULL;
 					n->limitCount = $2;
 					n->limitOption = LIMIT_OPTION_COUNT;
 					$$ = n;
@@ -11753,21 +11730,24 @@ limit_clause:
 			 */
 			| FETCH first_or_next select_fetch_first_value row_or_rows ONLY
 				{
-					LimitClause *n = (LimitClause *) palloc(sizeof(LimitClause));
+					SelectLimit *n = (SelectLimit *) palloc(sizeof(SelectLimit));
+					n->limitOffset = NULL;
 					n->limitCount = $3;
 					n->limitOption = LIMIT_OPTION_COUNT;
 					$$ = n;
 				}
 			| FETCH first_or_next select_fetch_first_value row_or_rows WITH TIES
 				{
-					LimitClause *n = (LimitClause *) palloc(sizeof(LimitClause));
+					SelectLimit *n = (SelectLimit *) palloc(sizeof(SelectLimit));
+					n->limitOffset = NULL;
 					n->limitCount = $3;
 					n->limitOption = LIMIT_OPTION_WITH_TIES;
 					$$ = n;
 				}
 			| FETCH first_or_next row_or_rows ONLY
 				{
-					LimitClause *n = (LimitClause *) palloc(sizeof(LimitClause));
+					SelectLimit *n = (SelectLimit *) palloc(sizeof(SelectLimit));
+					n->limitOffset = NULL;
 					n->limitCount = makeIntConst(1, -1);
 					n->limitOption = LIMIT_OPTION_COUNT;
 					$$ = n;

--X1bOJ3K7DJ5YkBrT--