Thread: concerns around pg_lsn

concerns around pg_lsn

From
Jeevan Ladhe
Date:

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
Attachment

Re: concerns around pg_lsn

From
Michael Paquier
Date:
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

Attachment

Re: concerns around pg_lsn

From
Jeevan Ladhe
Date:
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 (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

From
Robert Haas
Date:
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



Re: concerns around pg_lsn

From
Michael Paquier
Date:
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

Attachment

Re: concerns around pg_lsn

From
Jeevan Ladhe
Date:


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

Re: concerns around pg_lsn

From
Michael Paquier
Date:
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

Attachment

Re: concerns around pg_lsn

From
Craig Ringer
Date:
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

From
Jeevan Ladhe
Date:
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


Re: concerns around pg_lsn

From
Jeevan Ladhe
Date:
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

From
Michael Paquier
Date:
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

Attachment

Re: concerns around pg_lsn

From
Jeevan Ladhe
Date:
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 
Attachment

Re: concerns around pg_lsn

From
Michael Paquier
Date:
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

Attachment

Re: concerns around pg_lsn

From
Jeevan Ladhe
Date:
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

Re: concerns around pg_lsn

From
Michael Paquier
Date:
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

Attachment

Re: concerns around pg_lsn

From
Jeevan Ladhe
Date:
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
Attachment

Re: concerns around pg_lsn

From
Alvaro Herrera
Date:
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



Re: concerns around pg_lsn

From
Michael Paquier
Date:
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

Attachment

Re: concerns around pg_lsn

From
Jeevan Ladhe
Date:
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 
Attachment

Re: concerns around pg_lsn

From
Michael Paquier
Date:
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

Attachment

Re: concerns around pg_lsn

From
Jeevan Ladhe
Date:
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