timeline signedness

Started by Peter Eisentrautover 12 years ago5 messages
#1Peter Eisentraut
peter_e@gmx.net

WAL timelines are unsigned 32-bit integers everywhere, except the
replication parser (replication/repl_gram.y and
replication/repl_scanner.l) treats them as signed 32-bit integers. It's
obviously a corner case, but it would be prudent to be correct about
this. It should be easy to fix in those grammar files.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#2Peter Eisentraut
peter_e@gmx.net
In reply to: Peter Eisentraut (#1)
1 attachment(s)
Re: timeline signedness

On Wed, 2013-08-07 at 21:55 -0400, Peter Eisentraut wrote:

WAL timelines are unsigned 32-bit integers everywhere, except the
replication parser (replication/repl_gram.y and
replication/repl_scanner.l) treats them as signed 32-bit integers. It's
obviously a corner case, but it would be prudent to be correct about
this. It should be easy to fix in those grammar files.

Here is a patch to fix this.

Attachments:

timeline-unsigned.patchtext/x-patch; charset=UTF-8; name=timeline-unsigned.patchDownload
diff --git a/src/backend/replication/repl_gram.y b/src/backend/replication/repl_gram.y
index bce18b8..f465530 100644
--- a/src/backend/replication/repl_gram.y
+++ b/src/backend/replication/repl_gram.y
@@ -56,7 +56,7 @@ Node *replication_parse_result;
 %union {
 		char					*str;
 		bool					boolval;
-		int32					intval;
+		uint32					uintval;
 
 		XLogRecPtr				recptr;
 		Node					*node;
@@ -66,7 +66,7 @@ Node *replication_parse_result;
 
 /* Non-keyword tokens */
 %token <str> SCONST
-%token <intval> ICONST
+%token <uintval> UCONST
 %token <recptr> RECPTR
 
 /* Keyword tokens. */
@@ -85,7 +85,7 @@ Node *replication_parse_result;
 %type <node>	base_backup start_replication identify_system timeline_history
 %type <list>	base_backup_opt_list
 %type <defelt>	base_backup_opt
-%type <intval>	opt_timeline
+%type <uintval>	opt_timeline
 %%
 
 firstcmd: command opt_semicolon
@@ -175,14 +175,7 @@ start_replication:
 			;
 
 opt_timeline:
-			K_TIMELINE ICONST
-				{
-					if ($2 <= 0)
-						ereport(ERROR,
-								(errcode(ERRCODE_SYNTAX_ERROR),
-								 (errmsg("invalid timeline %d", $2))));
-					$$ = $2;
-				}
+			K_TIMELINE UCONST			{ $$ = $2; }
 				| /* nothing */			{ $$ = 0; }
 			;
 
@@ -190,15 +183,10 @@ opt_timeline:
  * TIMELINE_HISTORY %d
  */
 timeline_history:
-			K_TIMELINE_HISTORY ICONST
+			K_TIMELINE_HISTORY UCONST
 				{
 					TimeLineHistoryCmd *cmd;
 
-					if ($2 <= 0)
-						ereport(ERROR,
-								(errcode(ERRCODE_SYNTAX_ERROR),
-								 (errmsg("invalid timeline %d", $2))));
-
 					cmd = makeNode(TimeLineHistoryCmd);
 					cmd->timeline = $2;
 
diff --git a/src/backend/replication/repl_scanner.l b/src/backend/replication/repl_scanner.l
index b4743e6..3d930f1 100644
--- a/src/backend/replication/repl_scanner.l
+++ b/src/backend/replication/repl_scanner.l
@@ -83,8 +83,8 @@ TIMELINE_HISTORY	{ return K_TIMELINE_HISTORY; }
 " "				;
 
 {digit}+		{
-					yylval.intval = pg_atoi(yytext, sizeof(int32), 0);
-					return ICONST;
+					yylval.uintval = strtoul(yytext, NULL, 10);
+					return UCONST;
 				}
 
 {hexdigit}+\/{hexdigit}+		{
#3Magnus Hagander
magnus@hagander.net
In reply to: Peter Eisentraut (#2)
Re: timeline signedness

On Tue, Aug 13, 2013 at 1:31 PM, Peter Eisentraut <peter_e@gmx.net> wrote:

On Wed, 2013-08-07 at 21:55 -0400, Peter Eisentraut wrote:

WAL timelines are unsigned 32-bit integers everywhere, except the
replication parser (replication/repl_gram.y and
replication/repl_scanner.l) treats them as signed 32-bit integers. It's
obviously a corner case, but it would be prudent to be correct about
this. It should be easy to fix in those grammar files.

Here is a patch to fix this.

Looks fine to me, and looks like the correct thing to do.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Heikki Linnakangas
hlinnakangas@vmware.com
In reply to: Peter Eisentraut (#2)
Re: timeline signedness

On 13.08.2013 14:31, Peter Eisentraut wrote:

On Wed, 2013-08-07 at 21:55 -0400, Peter Eisentraut wrote:

WAL timelines are unsigned 32-bit integers everywhere, except the
replication parser (replication/repl_gram.y and
replication/repl_scanner.l) treats them as signed 32-bit integers. It's
obviously a corner case, but it would be prudent to be correct about
this. It should be easy to fix in those grammar files.

+1

Here is a patch to fix this.

If I'm reading this correctly, timeline 0 no longer throws an error with
this patch.

- Heikki

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Peter Eisentraut
peter_e@gmx.net
In reply to: Heikki Linnakangas (#4)
Re: timeline signedness

On Wed, 2013-08-14 at 16:20 +0300, Heikki Linnakangas wrote:

On 13.08.2013 14:31, Peter Eisentraut wrote:

On Wed, 2013-08-07 at 21:55 -0400, Peter Eisentraut wrote:

WAL timelines are unsigned 32-bit integers everywhere, except the
replication parser (replication/repl_gram.y and
replication/repl_scanner.l) treats them as signed 32-bit integers. It's
obviously a corner case, but it would be prudent to be correct about
this. It should be easy to fix in those grammar files.

+1

Here is a patch to fix this.

If I'm reading this correctly, timeline 0 no longer throws an error with
this patch.

Fixed that.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers