concerns around pg_lsn
Hi all,
While reviewing some code around pg_lsn_in() I came across a couple of
(potential?) issues:
1.
Commit 21f428eb moves lsn conversion functionality from pg_lsn_in() to a new
function pg_lsn_in_internal(). It takes two parameters the lsn string and a
pointer to a boolean (*have_error) to indicate if there was an error while
converting string format to XLogRecPtr.
pg_lsn_in_internal() only sets the *have_error to 'true' if there is an
error,
but leaves it for the caller to make sure it was passed by initializing as
'false'. Currently it is only getting called from pg_lsn_in() and
timestamptz_in()
where it has been taken care that the flag is set to false before making the
call. But I think in general it opens the door for unpredictable bugs if
pg_lsn_in_internal() gets called from other locations in future (if need
maybe) and by mistake, it just checks the return value of the flag without
setting it to false before making a call.
I am attaching a patch that makes sure that *have_error is set to false in
pg_lsn_in_internal() itself, rather than being caller dependent.
Also, I think there might be callers who may not care if there had been an
error
while converting and just ok to use InvalidXLogRecPtr against return value,
and
may pass just a NULL boolean pointer to this function, but for now, I have
left
that untouched. Maybe just adding an Assert would improve the situation for
time being.
I have attached a patch (fix_have_error_flag.patch) to take care of above.
2.
I happened to peep in test case pg_lsn.sql, and I started exploring the
macros
around lsn.
Following macros:
{code}
/*
* Zero is used indicate an invalid pointer. Bootstrap skips the first
possible
* WAL segment, initializing the first WAL page at WAL segment size, so no
XLOG
* record can begin at zero.
*/
#define InvalidXLogRecPtr 0
#define XLogRecPtrIsInvalid(r) ((r) == InvalidXLogRecPtr)
{code}
IIUC, in the comment above we clearly want to mark 0 as an invalid lsn (also
further IIUC the comment states - lsn would start from (walSegSize + 1)).
Given
this, should not it be invalid to allow "0/0" as the value of type pg_lsn,
or
for that matter any number < walSegSize?
There is a test scenario in test case pg_lsn.sql which tests insertion of
"0/0"
in a table having a pg_lsn column. I think this is contradictory to the
comment.
I am not sure of thought behind this and might be wrong while making the
above
assumption. But, I tried to look around a bit in hackers emails and could
not
locate any related discussion.
I have attached a patch (mark_lsn_0_invalid.patch) that makes above changes.
Thoughts?
Regards,
Jeevan Ladhe
Attachments:
fix_have_error_flag.patchapplication/octet-stream; name=fix_have_error_flag.patchDownload
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index b4c6c23..90e9b8c 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -34,6 +34,9 @@ pg_lsn_in_internal(const char *str, bool *have_error)
off;
XLogRecPtr result;
+ Assert(have_error != NULL);
+ *have_error = false;
+
/* Sanity check input format. */
len1 = strspn(str, "0123456789abcdefABCDEF");
if (len1 < 1 || len1 > MAXPG_LSNCOMPONENT || str[len1] != '/')
@@ -61,7 +64,7 @@ pg_lsn_in(PG_FUNCTION_ARGS)
{
char *str = PG_GETARG_CSTRING(0);
XLogRecPtr result;
- bool have_error = false;
+ bool have_error;
result = pg_lsn_in_internal(str, &have_error);
if (have_error)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fc46360..ce179fc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11677,7 +11677,7 @@ check_recovery_target_lsn(char **newval, void **extra, GucSource source)
{
XLogRecPtr lsn;
XLogRecPtr *myextra;
- bool have_error = false;
+ bool have_error;
lsn = pg_lsn_in_internal(*newval, &have_error);
if (have_error)
mark_lsn_0_invalid.patchapplication/octet-stream; name=mark_lsn_0_invalid.patchDownload
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index b4c6c23..d755da3 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -53,6 +53,9 @@ pg_lsn_in_internal(const char *str, bool *have_error)
off = (uint32) strtoul(str + len1 + 1, NULL, 16);
result = ((uint64) id << 32) | off;
+ if (XLogRecPtrIsInvalid(result))
+ *have_error = true;
+
return result;
}
diff --git a/src/test/regress/expected/pg_lsn.out b/src/test/regress/expected/pg_lsn.out
index 64d41df..d0ef436 100644
--- a/src/test/regress/expected/pg_lsn.out
+++ b/src/test/regress/expected/pg_lsn.out
@@ -4,6 +4,10 @@
CREATE TABLE PG_LSN_TBL (f1 pg_lsn);
-- Largest and smallest input
INSERT INTO PG_LSN_TBL VALUES ('0/0');
+ERROR: invalid input syntax for type pg_lsn: "0/0"
+LINE 1: INSERT INTO PG_LSN_TBL VALUES ('0/0');
+ ^
+INSERT INTO PG_LSN_TBL VALUES ('ABC/DEF');
INSERT INTO PG_LSN_TBL VALUES ('FFFFFFFF/FFFFFFFF');
-- Incorrect input
INSERT INTO PG_LSN_TBL VALUES ('G/0');
@@ -28,9 +32,9 @@ LINE 1: INSERT INTO PG_LSN_TBL VALUES ('/ABCD');
^
-- Min/Max aggregation
SELECT MIN(f1), MAX(f1) FROM PG_LSN_TBL;
- min | max
------+-------------------
- 0/0 | FFFFFFFF/FFFFFFFF
+ min | max
+---------+-------------------
+ ABC/DEF | FFFFFFFF/FFFFFFFF
(1 row)
DROP TABLE PG_LSN_TBL;
diff --git a/src/test/regress/sql/pg_lsn.sql b/src/test/regress/sql/pg_lsn.sql
index 2c143c8..391d0b8 100644
--- a/src/test/regress/sql/pg_lsn.sql
+++ b/src/test/regress/sql/pg_lsn.sql
@@ -6,6 +6,7 @@ CREATE TABLE PG_LSN_TBL (f1 pg_lsn);
-- Largest and smallest input
INSERT INTO PG_LSN_TBL VALUES ('0/0');
+INSERT INTO PG_LSN_TBL VALUES ('ABC/DEF');
INSERT INTO PG_LSN_TBL VALUES ('FFFFFFFF/FFFFFFFF');
-- Incorrect input
On Mon, Jul 29, 2019 at 10:55:29PM +0530, Jeevan Ladhe wrote:
I am attaching a patch that makes sure that *have_error is set to false in
pg_lsn_in_internal() itself, rather than being caller dependent.
Agreed about making the code more defensive as you do. I would keep
the initialization in check_recovery_target_lsn and pg_lsn_in_internal
though. That does not hurt and makes the code easier to understand,
aka we don't expect an error by default in those paths.
IIUC, in the comment above we clearly want to mark 0 as an invalid lsn (also
further IIUC the comment states - lsn would start from (walSegSize + 1)).
Given this, should not it be invalid to allow "0/0" as the value of
type pg_lsn, or for that matter any number < walSegSize?
You can rely on "0/0" as a base point to calculate the offset in a
segment, so my guess is that we could break applications by generating
an error. Please note that the behavior is much older than the
introduction of pg_lsn, as the original parsing logic has been removed
in 6f289c2b with validate_xlog_location() in xlogfuncs.c.
--
Michael
Hi Michael,
Thanks for your inputs, really appreciate.
On Tue, Jul 30, 2019 at 9:42 AM Michael Paquier <michael@paquier.xyz> wrote:
On Mon, Jul 29, 2019 at 10:55:29PM +0530, Jeevan Ladhe wrote:
I am attaching a patch that makes sure that *have_error is set to false
in
pg_lsn_in_internal() itself, rather than being caller dependent.
Agreed about making the code more defensive as you do. I would keep
the initialization in check_recovery_target_lsn and pg_lsn_in_internal
though. That does not hurt and makes the code easier to understand,
aka we don't expect an error by default in those paths.
Sure, understood. I am ok with this.
IIUC, in the comment above we clearly want to mark 0 as an invalid lsn
(alsofurther IIUC the comment states - lsn would start from (walSegSize + 1)).
Given this, should not it be invalid to allow "0/0" as the value of
type pg_lsn, or for that matter any number < walSegSize?You can rely on "0/0" as a base point to calculate the offset in a
segment, so my guess is that we could break applications by generating
an error.
Agree that it may break the applications.
Please note that the behavior is much older than the
introduction of pg_lsn, as the original parsing logic has been removed
in 6f289c2b with validate_xlog_location() in xlogfuncs.c.
My only concern was something that we internally treat as invalid, why do
we allow, that as a valid value for that type. While I am not trying to
reinvent the wheel here, I am trying to understand if there had been any
idea behind this and I am missing it.
Regards,
Jeevan Ladhe
On Tue, Jul 30, 2019 at 4:52 AM Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:
My only concern was something that we internally treat as invalid, why do
we allow, that as a valid value for that type. While I am not trying to
reinvent the wheel here, I am trying to understand if there had been any
idea behind this and I am missing it.
Well, the word "invalid" can mean more than one thing. Something can
be valid or invalid depending on context. I can't have -2 dollars in
my wallet, but I could have -2 dollars in my bank account, because the
bank will allow me to pay out slightly more money than I actually have
on the idea that I will pay them back later (and with interest!). So
as an amount of money in my wallet, -2 is invalid, but as an amount of
money in my bank account, it is valid.
0/0 is not a valid LSN in the sense that (in current releases) we
never write a WAL record there, but it's OK to compute with it.
Subtracting '0/0'::pg_lsn seems useful as a way to convert an LSN to
an absolute byte-index in the WAL stream.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Jul 30, 2019 at 02:22:30PM +0530, Jeevan Ladhe wrote:
On Tue, Jul 30, 2019 at 9:42 AM Michael Paquier <michael@paquier.xyz> wrote:
Agreed about making the code more defensive as you do. I would keep
the initialization in check_recovery_target_lsn and pg_lsn_in_internal
though. That does not hurt and makes the code easier to understand,
aka we don't expect an error by default in those paths.Sure, understood. I am ok with this.
I am adding Peter Eisentraut in CC as 21f428e is his commit. I think
that the first patch is a good idea, so I would be fine to apply it,
but let's see the original committer's opinion first.
--
Michael
On Tue, Jul 30, 2019 at 6:06 PM Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jul 30, 2019 at 4:52 AM Jeevan Ladhe
<jeevan.ladhe@enterprisedb.com> wrote:My only concern was something that we internally treat as invalid, why do
we allow, that as a valid value for that type. While I am not trying to
reinvent the wheel here, I am trying to understand if there had been any
idea behind this and I am missing it.Well, the word "invalid" can mean more than one thing. Something can
be valid or invalid depending on context. I can't have -2 dollars in
my wallet, but I could have -2 dollars in my bank account, because the
bank will allow me to pay out slightly more money than I actually have
on the idea that I will pay them back later (and with interest!). So
as an amount of money in my wallet, -2 is invalid, but as an amount of
money in my bank account, it is valid.0/0 is not a valid LSN in the sense that (in current releases) we
never write a WAL record there, but it's OK to compute with it.
Subtracting '0/0'::pg_lsn seems useful as a way to convert an LSN to
an absolute byte-index in the WAL stream.
Thanks Robert for such a nice and detailed explanation.
I now understand why LSN '0/0' can still be useful.
Regards,
Jeevan Ladhe
On Wed, Jul 31, 2019 at 09:51:30AM +0900, Michael Paquier wrote:
I am adding Peter Eisentraut in CC as 21f428e is his commit. I think
that the first patch is a good idea, so I would be fine to apply it,
but let's see the original committer's opinion first.
On further review of the first patch, I think that it could be a good
idea to apply the same safeguards within float8in_internal_opt_error.
Jeevan, what do you think?
--
Michael
On the topic of pg_lsn, I recently noticed that there's no
operator(+)(pg_lsn,bigint) nor is there an operator(-)(pg_lsn,bigint) so
you can't compute offsets easily. We don't have a cast between pg_lsn and
bigint because we don't expose an unsigned bigint type in SQL, so you can't
work around it that way.
I may be missing the obvious, but I suggest (and will follow with a patch
for) adding + and - operators for computing offsets. I was considering an
age() function for it too, but I think it's best to force the user to be
clear about what current LSN they want to compare with so I'll skip that.
--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise
Hi Michael,
On further review of the first patch, I think that it could be a good
idea to apply the same safeguards within float8in_internal_opt_error.
Jeevan, what do you think?
Sure, agree, it makes sense to address float8in_internal_opt_error(),
there might be more occurrences of such instances in other functions
as well. I think if we agree, as and when encounter them while touching
those areas we should fix them.
What is more dangerous with float8in_internal_opt_error() is, it has
the have_error flag, which is never ever set or used in that function.
Further
more risks are - the callers of this function e.g.
executeItemOptUnwrapTarget()
are passing a non-null pointer to it(default set to false) and expect to
throw
an error if it sees some error during float8in_internal_opt_error(), *but*
float8in_internal_opt_error() has actually never touched the have_error
flag.
So, in this case it is fine because the flag was set to false, if it was not
set, then the garbage value would always result in true and keep on throwing
an error!
Here is relevant code from function executeItemOptUnwrapTarget():
{code}
975 if (jb->type == jbvNumeric)
976 {
977 char *tmp =
DatumGetCString(DirectFunctionCall1(numeric_out,
978
NumericGetDatum(jb->val.numeric)));
979 bool have_error = false;
980
981 (void) float8in_internal_opt_error(tmp,
982 NULL,
983 "double
precision",
984 tmp,
985 &have_error);
986
987 if (have_error)
988 RETURN_ERROR(ereport(ERROR,
989
(errcode(ERRCODE_NON_NUMERIC_JSON_ITEM),
990 errmsg("jsonpath item
method .%s() can only be applied to a numeric value",
991
jspOperationName(jsp->type)))));
992 res = jperOk;
993 }
994 else if (jb->type == jbvString)
995 {
996 /* cast string as double */
997 double val;
998 char *tmp = pnstrdup(jb->val.string.val,
999 jb->val.string.len);
1000 bool have_error = false;
1001
1002 val = float8in_internal_opt_error(tmp,
1003 NULL,
1004 "double
precision",
1005 tmp,
1006 &have_error);
1007
1008 if (have_error || isinf(val))
1009 RETURN_ERROR(ereport(ERROR,
1010
(errcode(ERRCODE_NON_NUMERIC_JSON_ITEM),
1011 errmsg("jsonpath item
method .%s() can only be applied to a numeric value",
1012
jspOperationName(jsp->type)))));
1013
1014 jb = &jbv;
1015 jb->type = jbvNumeric;
1016 jb->val.numeric =
DatumGetNumeric(DirectFunctionCall1(float8_numeric,
1017
Float8GetDatum(val)));
1018 res = jperOk;
1019 }
{code}
I will further check if by mistake any further commits have removed
references
to assignments from float8in_internal_opt_error(), evaluate it, and set out
a
patch.
This is one of the reason, I was saying it can be taken as a good practice
to
let the function who is accepting an out parameter sets the value for sure
to
some or other value.
Regards,
Jeevan Ladhe
Hi Michael,
What is more dangerous with float8in_internal_opt_error() is, it has
the have_error flag, which is never ever set or used in that function.
Further
more risks are - the callers of this function e.g.
executeItemOptUnwrapTarget()
are passing a non-null pointer to it(default set to false) and expect to
throw
an error if it sees some error during float8in_internal_opt_error(), *but*
float8in_internal_opt_error() has actually never touched the have_error
flag.
My bad, I see there's this macro call in float8in_internal_opt_error() and
that
set the flag:
{code}
#define RETURN_ERROR(throw_error) \
do { \
if (have_error) { \
*have_error = true; \
return 0.0; \
} else { \
throw_error; \
} \
} while (0)
{code}
My patch on way, thanks.
Regards,
Jeevan Ladhe
On Thu, Aug 01, 2019 at 11:14:32AM +0530, Jeevan Ladhe wrote:
Sure, agree, it makes sense to address float8in_internal_opt_error(),
there might be more occurrences of such instances in other functions
as well. I think if we agree, as and when encounter them while touching
those areas we should fix them.
I have spotted a third area within make_result_opt_error in numeric.c
which could gain readability by initializing have_error if the pointer
is defined.
I will further check if by mistake any further commits have removed
references to assignments from float8in_internal_opt_error(),
evaluate it, and set out a patch.
Thanks, Jeevan!
--
Michael
Hi Michael,
I will further check if by mistake any further commits have removed
references to assignments from float8in_internal_opt_error(),
evaluate it, and set out a patch.Thanks, Jeevan!
Here is a patch that takes care of addressing the flag issue including
pg_lsn_in_internal() and others.
I have further also fixed couple of other functions,
numeric_div_opt_error() and
numeric_mod_opt_error() which are basically callers of
make_result_opt_error().
Kindly do let me know if you have any comments.
Regards,
Jeevan Ladhe
Attachments:
0001-Make-the-have_error-flag-initialization-more-defensi.patchapplication/octet-stream; name=0001-Make-the-have_error-flag-initialization-more-defensi.patchDownload
From 199432ea2258707fac4c22bcda509f3c4d5177ae Mon Sep 17 00:00:00 2001
From: Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
Date: Thu, 1 Aug 2019 11:55:30 +0530
Subject: [PATCH] Make the have_error flag initialization more defensive.
Basically get rid of the dependency to have caller
initalize the flag.
This patch addresses such occurrences in following functions:
pg_lsn_in_internal(), float8in_internal_opt_error(),
numeric_mod_opt_error(), numeric_div_opt_error() and
numeric_mod_opt_error().
Jeevan Ladhe
---
src/backend/utils/adt/float.c | 3 +++
src/backend/utils/adt/jsonpath_exec.c | 6 +++---
src/backend/utils/adt/numeric.c | 9 +++++++++
src/backend/utils/adt/pg_lsn.c | 13 ++++++++++---
src/backend/utils/misc/guc.c | 2 +-
5 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 7540ca2..d40dc6c 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -376,6 +376,9 @@ float8in_internal_opt_error(char *num, char **endptr_p,
double val;
char *endptr;
+ if (have_error)
+ *have_error = false;
+
/* skip leading whitespace */
while (*num != '\0' && isspace((unsigned char) *num))
num++;
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index 293d6da..a0ead39 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -976,7 +976,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
{
char *tmp = DatumGetCString(DirectFunctionCall1(numeric_out,
NumericGetDatum(jb->val.numeric)));
- bool have_error = false;
+ bool have_error;
(void) float8in_internal_opt_error(tmp,
NULL,
@@ -997,7 +997,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
double val;
char *tmp = pnstrdup(jb->val.string.val,
jb->val.string.len);
- bool have_error = false;
+ bool have_error;
val = float8in_internal_opt_error(tmp,
NULL,
@@ -1521,7 +1521,7 @@ executeBinaryArithmExpr(JsonPathExecContext *cxt, JsonPathItem *jsp,
}
else
{
- bool error = false;
+ bool error;
res = func(lval->val.numeric, rval->val.numeric, &error);
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 201784b..7b0874b 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -2605,6 +2605,9 @@ numeric_div_opt_error(Numeric num1, Numeric num2, bool *have_error)
Numeric res;
int rscale;
+ if(have_error)
+ *have_error = false;
+
/*
* Handle NaN
*/
@@ -2721,6 +2724,9 @@ numeric_mod_opt_error(Numeric num1, Numeric num2, bool *have_error)
NumericVar arg2;
NumericVar result;
+ if (have_error)
+ *have_error = false;
+
if (NUMERIC_IS_NAN(num1) || NUMERIC_IS_NAN(num2))
return make_result(&const_nan);
@@ -6249,6 +6255,9 @@ make_result_opt_error(const NumericVar *var, bool *have_error)
int n;
Size len;
+ if (have_error)
+ *have_error = false;
+
if (sign == NUMERIC_NAN)
{
result = (Numeric) palloc(NUMERIC_HDRSZ_SHORT);
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index b4c6c23..3a3490f 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -34,17 +34,24 @@ pg_lsn_in_internal(const char *str, bool *have_error)
off;
XLogRecPtr result;
+ if (have_error)
+ *have_error = false;
+
/* Sanity check input format. */
len1 = strspn(str, "0123456789abcdefABCDEF");
if (len1 < 1 || len1 > MAXPG_LSNCOMPONENT || str[len1] != '/')
{
- *have_error = true;
+ if (have_error)
+ *have_error = true;
+
return InvalidXLogRecPtr;
}
len2 = strspn(str + len1 + 1, "0123456789abcdefABCDEF");
if (len2 < 1 || len2 > MAXPG_LSNCOMPONENT || str[len1 + 1 + len2] != '\0')
{
- *have_error = true;
+ if (have_error)
+ *have_error = true;
+
return InvalidXLogRecPtr;
}
@@ -61,7 +68,7 @@ pg_lsn_in(PG_FUNCTION_ARGS)
{
char *str = PG_GETARG_CSTRING(0);
XLogRecPtr result;
- bool have_error = false;
+ bool have_error;
result = pg_lsn_in_internal(str, &have_error);
if (have_error)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fc46360..ce179fc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11677,7 +11677,7 @@ check_recovery_target_lsn(char **newval, void **extra, GucSource source)
{
XLogRecPtr lsn;
XLogRecPtr *myextra;
- bool have_error = false;
+ bool have_error;
lsn = pg_lsn_in_internal(*newval, &have_error);
if (have_error)
--
2.7.4
On Thu, Aug 01, 2019 at 12:39:26PM +0530, Jeevan Ladhe wrote:
Here is a patch that takes care of addressing the flag issue including
pg_lsn_in_internal() and others.
Your original patch for pg_lsn_in_internal() was right IMO, and the
new one is not. In the numeric and float code paths, we have this
kind of pattern:
if (have_error)
{
*have_error = true;
return;
}
else
elog(ERROR, "Boom. Show is over.");
But the pg_lsn.c portion does not have that. have_error cannot be
NULL or the caller may fall into the trap of setting it to NULL and
miss some errors at parsing-time. So I think that keeping the
assertion on (have_error != NULL) is necessary.
--
Michael
Hi Michael,
On Thu, Aug 1, 2019 at 1:51 PM Michael Paquier <michael@paquier.xyz> wrote:
On Thu, Aug 01, 2019 at 12:39:26PM +0530, Jeevan Ladhe wrote:
Here is a patch that takes care of addressing the flag issue including
pg_lsn_in_internal() and others.Your original patch for pg_lsn_in_internal() was right IMO, and the
new one is not. In the numeric and float code paths, we have this
kind of pattern:
if (have_error)
{
*have_error = true;
return;
}
else
elog(ERROR, "Boom. Show is over.");But the pg_lsn.c portion does not have that. have_error cannot be
NULL or the caller may fall into the trap of setting it to NULL and
miss some errors at parsing-time. So I think that keeping the
assertion on (have_error != NULL) is necessary.
Thanks for your concern.
In pg_lsn_in_internal() changes, the caller will get the invalid lsn
in case there are errors:
{code}
if (len1 < 1 || len1 > MAXPG_LSNCOMPONENT || str[len1] != '/')
{
if (have_error)
*have_error = true;
return InvalidXLogRecPtr;
}
{code}
The only thing is that, if the caller cares about the error during
the parsing or not. For some callers just making sure if the given
string was valid lsn or not might be ok, and the return value
'InvalidXLogRecPtr' will tell that. That caller may not unnecessary
declare the flag and pass a pointer to it.
Regards,
Jeevan Ladhe
On Thu, Aug 01, 2019 at 02:10:08PM +0530, Jeevan Ladhe wrote:
The only thing is that, if the caller cares about the error during
the parsing or not.
That's where the root of the problem is. We should really make things
so as the caller of this routine cares about errors. With your patch
a caller could do pg_lsn_in_internal('G/G', NULL), and then get
InvalidXLogRecPtr which is plain wrong. It is true that a caller may
not care about the error, but the idea is to make callers *think*
about the error case when they implement something and decide if it is
valid or not. The float and numeric code paths do that, not pg_lsn
with this patch. It would actually be fine to move ereport(ERROR)
from pg_lsn_in to pg_lsn_in_internal and trigger these if have_error
is NULL, but that means a duplication and the code is simple.
--
Michael
Sure Michael, in the attached patch I have reverted the checks from
pg_lsn_in_internal() and added Assert() per my original patch.
Regards,
Jeevan Ladhe
Attachments:
0001-Make-have_error-initialization-more-defensive-v2.patchapplication/octet-stream; name=0001-Make-have_error-initialization-more-defensive-v2.patchDownload
From 5cae464a6291ddcc6302a381701221cfd2ebcd1e Mon Sep 17 00:00:00 2001
From: Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
Date: Sun, 4 Aug 2019 09:08:20 +0530
Subject: [PATCH] Make have_error initialization more defensive v2
Basically get rid of the dependency to have caller
initalize the flag.
This patch addresses such occurrences in following functions:
pg_lsn_in_internal(), float8in_internal_opt_error(),
numeric_mod_opt_error(), numeric_div_opt_error() and
numeric_mod_opt_error().
Jeevan Ladhe
---
src/backend/utils/adt/float.c | 3 +++
src/backend/utils/adt/jsonpath_exec.c | 6 +++---
src/backend/utils/adt/numeric.c | 9 +++++++++
src/backend/utils/adt/pg_lsn.c | 5 ++++-
src/backend/utils/misc/guc.c | 2 +-
5 files changed, 20 insertions(+), 5 deletions(-)
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 7540ca2..d40dc6c 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -376,6 +376,9 @@ float8in_internal_opt_error(char *num, char **endptr_p,
double val;
char *endptr;
+ if (have_error)
+ *have_error = false;
+
/* skip leading whitespace */
while (*num != '\0' && isspace((unsigned char) *num))
num++;
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index 293d6da..a0ead39 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -976,7 +976,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
{
char *tmp = DatumGetCString(DirectFunctionCall1(numeric_out,
NumericGetDatum(jb->val.numeric)));
- bool have_error = false;
+ bool have_error;
(void) float8in_internal_opt_error(tmp,
NULL,
@@ -997,7 +997,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
double val;
char *tmp = pnstrdup(jb->val.string.val,
jb->val.string.len);
- bool have_error = false;
+ bool have_error;
val = float8in_internal_opt_error(tmp,
NULL,
@@ -1521,7 +1521,7 @@ executeBinaryArithmExpr(JsonPathExecContext *cxt, JsonPathItem *jsp,
}
else
{
- bool error = false;
+ bool error;
res = func(lval->val.numeric, rval->val.numeric, &error);
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 201784b..7b0874b 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -2605,6 +2605,9 @@ numeric_div_opt_error(Numeric num1, Numeric num2, bool *have_error)
Numeric res;
int rscale;
+ if(have_error)
+ *have_error = false;
+
/*
* Handle NaN
*/
@@ -2721,6 +2724,9 @@ numeric_mod_opt_error(Numeric num1, Numeric num2, bool *have_error)
NumericVar arg2;
NumericVar result;
+ if (have_error)
+ *have_error = false;
+
if (NUMERIC_IS_NAN(num1) || NUMERIC_IS_NAN(num2))
return make_result(&const_nan);
@@ -6249,6 +6255,9 @@ make_result_opt_error(const NumericVar *var, bool *have_error)
int n;
Size len;
+ if (have_error)
+ *have_error = false;
+
if (sign == NUMERIC_NAN)
{
result = (Numeric) palloc(NUMERIC_HDRSZ_SHORT);
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index b4c6c23..90e9b8c 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -34,6 +34,9 @@ pg_lsn_in_internal(const char *str, bool *have_error)
off;
XLogRecPtr result;
+ Assert(have_error != NULL);
+ *have_error = false;
+
/* Sanity check input format. */
len1 = strspn(str, "0123456789abcdefABCDEF");
if (len1 < 1 || len1 > MAXPG_LSNCOMPONENT || str[len1] != '/')
@@ -61,7 +64,7 @@ pg_lsn_in(PG_FUNCTION_ARGS)
{
char *str = PG_GETARG_CSTRING(0);
XLogRecPtr result;
- bool have_error = false;
+ bool have_error;
result = pg_lsn_in_internal(str, &have_error);
if (have_error)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fc46360..ce179fc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11677,7 +11677,7 @@ check_recovery_target_lsn(char **newval, void **extra, GucSource source)
{
XLogRecPtr lsn;
XLogRecPtr *myextra;
- bool have_error = false;
+ bool have_error;
lsn = pg_lsn_in_internal(*newval, &have_error);
if (have_error)
--
2.7.4
On 2019-Aug-04, Jeevan Ladhe wrote:
Sure Michael, in the attached patch I have reverted the checks from
pg_lsn_in_internal() and added Assert() per my original patch.
Can we please change the macro definition so that have_error is one of
the arguments? Having the variable be used inside the macro definition
but not appear literally in the call is quite confusing.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Aug 03, 2019 at 11:57:01PM -0400, Alvaro Herrera wrote:
Can we please change the macro definition so that have_error is one of
the arguments? Having the variable be used inside the macro definition
but not appear literally in the call is quite confusing.
Good idea. This needs some changes only in float.c.
--
Michael
On Sun, Aug 4, 2019 at 12:13 PM Michael Paquier <michael@paquier.xyz> wrote:
On Sat, Aug 03, 2019 at 11:57:01PM -0400, Alvaro Herrera wrote:
Can we please change the macro definition so that have_error is one of
the arguments? Having the variable be used inside the macro definition
but not appear literally in the call is quite confusing.
Can't agree more. This is where I also got confused initially and thought
the flag is unused.
Good idea. This needs some changes only in float.c.
Please find attached patch with the changes to RETURN_ERROR and
it's references in float.c
Regards,
Jeevan Ladhe
Attachments:
0001-Make-have_error-initialization-more-defensive-v3.patchapplication/octet-stream; name=0001-Make-have_error-initialization-more-defensive-v3.patchDownload
From 0e20d90ab016b7ac926feba765f62d8b64017d50 Mon Sep 17 00:00:00 2001
From: Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>
Date: Sun, 4 Aug 2019 09:08:20 +0530
Subject: [PATCH] Make have_error initialization more defensive v3
Basically get rid of the dependency to have caller
initalize the flag.
This patch addresses such occurrences in following functions:
pg_lsn_in_internal(), float8in_internal_opt_error(),
numeric_mod_opt_error(), numeric_div_opt_error() and
numeric_mod_opt_error().
Also, add have_error flag to macro RETURN_ERROR to make the
references more readable in float.c
Jeevan Ladhe
---
src/backend/utils/adt/float.c | 44 ++++++++++++++++++-----------------
src/backend/utils/adt/jsonpath_exec.c | 6 ++---
src/backend/utils/adt/numeric.c | 9 +++++++
src/backend/utils/adt/pg_lsn.c | 5 +++-
src/backend/utils/misc/guc.c | 2 +-
5 files changed, 40 insertions(+), 26 deletions(-)
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index 7540ca2..219a0bb 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -337,10 +337,10 @@ float8in(PG_FUNCTION_ARGS)
}
/* Convenience macro: set *have_error flag (if provided) or throw error */
-#define RETURN_ERROR(throw_error) \
+#define RETURN_ERROR(have_error, throw_error) \
do { \
if (have_error) { \
- *have_error = true; \
+ *(have_error) = true; \
return 0.0; \
} else { \
throw_error; \
@@ -376,6 +376,9 @@ float8in_internal_opt_error(char *num, char **endptr_p,
double val;
char *endptr;
+ if (have_error)
+ *have_error = false;
+
/* skip leading whitespace */
while (*num != '\0' && isspace((unsigned char) *num))
num++;
@@ -385,10 +388,10 @@ float8in_internal_opt_error(char *num, char **endptr_p,
* strtod() on different platforms.
*/
if (*num == '\0')
- RETURN_ERROR(ereport(ERROR,
- (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
- errmsg("invalid input syntax for type %s: \"%s\"",
- type_name, orig_string))));
+ RETURN_ERROR(have_error, ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("invalid input syntax for type %s: \"%s\"",
+ type_name, orig_string))));
errno = 0;
val = strtod(num, &endptr);
@@ -461,19 +464,18 @@ float8in_internal_opt_error(char *num, char **endptr_p,
char *errnumber = pstrdup(num);
errnumber[endptr - num] = '\0';
- RETURN_ERROR(ereport(ERROR,
- (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
- errmsg("\"%s\" is out of range for "
- "type double precision",
- errnumber))));
+ RETURN_ERROR(have_error, ereport(ERROR,
+ (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
+ errmsg("\"%s\" is out of range for type double precision",
+ errnumber))));
}
}
else
- RETURN_ERROR(ereport(ERROR,
- (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
- errmsg("invalid input syntax for type "
- "%s: \"%s\"",
- type_name, orig_string))));
+ RETURN_ERROR(have_error, ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("invalid input syntax for type "
+ "%s: \"%s\"",
+ type_name, orig_string))));
}
#ifdef HAVE_BUGGY_SOLARIS_STRTOD
else
@@ -496,11 +498,11 @@ float8in_internal_opt_error(char *num, char **endptr_p,
if (endptr_p)
*endptr_p = endptr;
else if (*endptr != '\0')
- RETURN_ERROR(ereport(ERROR,
- (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
- errmsg("invalid input syntax for type "
- "%s: \"%s\"",
- type_name, orig_string))));
+ RETURN_ERROR(have_error, ereport(ERROR,
+ (errcode(ERRCODE_INVALID_TEXT_REPRESENTATION),
+ errmsg("invalid input syntax for type "
+ "%s: \"%s\"",
+ type_name, orig_string))));
return val;
}
diff --git a/src/backend/utils/adt/jsonpath_exec.c b/src/backend/utils/adt/jsonpath_exec.c
index 293d6da..a0ead39 100644
--- a/src/backend/utils/adt/jsonpath_exec.c
+++ b/src/backend/utils/adt/jsonpath_exec.c
@@ -976,7 +976,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
{
char *tmp = DatumGetCString(DirectFunctionCall1(numeric_out,
NumericGetDatum(jb->val.numeric)));
- bool have_error = false;
+ bool have_error;
(void) float8in_internal_opt_error(tmp,
NULL,
@@ -997,7 +997,7 @@ executeItemOptUnwrapTarget(JsonPathExecContext *cxt, JsonPathItem *jsp,
double val;
char *tmp = pnstrdup(jb->val.string.val,
jb->val.string.len);
- bool have_error = false;
+ bool have_error;
val = float8in_internal_opt_error(tmp,
NULL,
@@ -1521,7 +1521,7 @@ executeBinaryArithmExpr(JsonPathExecContext *cxt, JsonPathItem *jsp,
}
else
{
- bool error = false;
+ bool error;
res = func(lval->val.numeric, rval->val.numeric, &error);
diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index 201784b..7b0874b 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -2605,6 +2605,9 @@ numeric_div_opt_error(Numeric num1, Numeric num2, bool *have_error)
Numeric res;
int rscale;
+ if(have_error)
+ *have_error = false;
+
/*
* Handle NaN
*/
@@ -2721,6 +2724,9 @@ numeric_mod_opt_error(Numeric num1, Numeric num2, bool *have_error)
NumericVar arg2;
NumericVar result;
+ if (have_error)
+ *have_error = false;
+
if (NUMERIC_IS_NAN(num1) || NUMERIC_IS_NAN(num2))
return make_result(&const_nan);
@@ -6249,6 +6255,9 @@ make_result_opt_error(const NumericVar *var, bool *have_error)
int n;
Size len;
+ if (have_error)
+ *have_error = false;
+
if (sign == NUMERIC_NAN)
{
result = (Numeric) palloc(NUMERIC_HDRSZ_SHORT);
diff --git a/src/backend/utils/adt/pg_lsn.c b/src/backend/utils/adt/pg_lsn.c
index b4c6c23..90e9b8c 100644
--- a/src/backend/utils/adt/pg_lsn.c
+++ b/src/backend/utils/adt/pg_lsn.c
@@ -34,6 +34,9 @@ pg_lsn_in_internal(const char *str, bool *have_error)
off;
XLogRecPtr result;
+ Assert(have_error != NULL);
+ *have_error = false;
+
/* Sanity check input format. */
len1 = strspn(str, "0123456789abcdefABCDEF");
if (len1 < 1 || len1 > MAXPG_LSNCOMPONENT || str[len1] != '/')
@@ -61,7 +64,7 @@ pg_lsn_in(PG_FUNCTION_ARGS)
{
char *str = PG_GETARG_CSTRING(0);
XLogRecPtr result;
- bool have_error = false;
+ bool have_error;
result = pg_lsn_in_internal(str, &have_error);
if (have_error)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fc46360..ce179fc 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -11677,7 +11677,7 @@ check_recovery_target_lsn(char **newval, void **extra, GucSource source)
{
XLogRecPtr lsn;
XLogRecPtr *myextra;
- bool have_error = false;
+ bool have_error;
lsn = pg_lsn_in_internal(*newval, &have_error);
if (have_error)
--
2.7.4
On Mon, Aug 05, 2019 at 09:15:02AM +0530, Jeevan Ladhe wrote:
Please find attached patch with the changes to RETURN_ERROR and
it's references in float.c
Thanks. Committed after applying some tweaks to it. I have noticed
that you forgot numeric_int4_opt_error() in the set.
--
Michael