Re: concerns around pg_lsn
> > Thanks. Committed after applying some tweaks to it. I have noticed > that you forgot numeric_int4_opt_error() in the set. Oops. Thanks for the commit, Michael. Regards, Jeevan Ladhe
Re: concerns around pg_lsn
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 signature.asc Description: PGP signature
Re: concerns around pg_lsn
On Sun, Aug 4, 2019 at 12:13 PM Michael Paquier 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 0001-Make-have_error-initialization-more-defensive-v3.patch Description: Binary data
Re: concerns around pg_lsn
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 signature.asc Description: PGP signature
Re: concerns around pg_lsn
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: concerns around pg_lsn
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 0001-Make-have_error-initialization-more-defensive-v2.patch Description: Binary data
Re: concerns around pg_lsn
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
signature.asc
Description: PGP signature
Re: concerns around pg_lsn
Hi Michael,
On Thu, Aug 1, 2019 at 1:51 PM Michael Paquier 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
Re: concerns around pg_lsn
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
signature.asc
Description: PGP signature
Re: concerns around pg_lsn
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 0001-Make-the-have_error-flag-initialization-more-defensi.patch Description: Binary data
Re: concerns around pg_lsn
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 signature.asc Description: PGP signature
Re: concerns around pg_lsn
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
Re: concerns around pg_lsn
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 boolhave_error = false;
980
981 (void) float8in_internal_opt_error(tmp,
982NULL,
983"double
precision",
984tmp,
985_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,
999jb->val.string.len);
1000 boolhave_error = false;
1001
1002 val = float8in_internal_opt_error(tmp,
1003 NULL,
1004 "double
precision",
1005 tmp,
1006 _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 =
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
Re: concerns around pg_lsn
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
Re: concerns around pg_lsn
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 signature.asc Description: PGP signature
Re: concerns around pg_lsn
On Tue, Jul 30, 2019 at 6:06 PM Robert Haas wrote: > On Tue, Jul 30, 2019 at 4:52 AM Jeevan Ladhe > 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
Re: concerns around pg_lsn
On Tue, Jul 30, 2019 at 02:22:30PM +0530, Jeevan Ladhe wrote: > On Tue, Jul 30, 2019 at 9:42 AM Michael Paquier 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 signature.asc Description: PGP signature
Re: concerns around pg_lsn
On Tue, Jul 30, 2019 at 4:52 AM Jeevan Ladhe 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
Re: concerns around pg_lsn
Hi Michael, Thanks for your inputs, really appreciate. On Tue, Jul 30, 2019 at 9:42 AM Michael Paquier 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 > (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. 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
Re: concerns around pg_lsn
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 signature.asc Description: PGP signature
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
fix_have_error_flag.patch
Description: Binary data
mark_lsn_0_invalid.patch
Description: Binary data
