Horton

Thread

Commits

  1. Add safeguards in LSN, numeric and float calculation for custom errors

  2. Don't call data type input functions in GUC check hooks

  1. concerns around pg_lsn

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-07-29T17:25:29Z

    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

  2. Re: concerns around pg_lsn

    Michael Paquier <michael@paquier.xyz> — 2019-07-30T04:12:49Z

    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

  3. Re: concerns around pg_lsn

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-07-30T08:52:30Z

    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

  4. Re: concerns around pg_lsn

    Robert Haas <robertmhaas@gmail.com> — 2019-07-30T12:36:45Z

    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

  5. Re: concerns around pg_lsn

    Michael Paquier <michael@paquier.xyz> — 2019-07-31T00:51:30Z

    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

  6. Re: concerns around pg_lsn

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-07-31T11:56:42Z

    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

  7. Re: concerns around pg_lsn

    Michael Paquier <michael@paquier.xyz> — 2019-08-01T00:46:42Z

    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

  8. Re: concerns around pg_lsn

    Craig Ringer <craig@2ndquadrant.com> — 2019-08-01T02:45:50Z

    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

  9. Re: concerns around pg_lsn

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-08-01T05:44:32Z

    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

  10. Re: concerns around pg_lsn

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-08-01T05:59:54Z

    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

  11. Re: concerns around pg_lsn

    Michael Paquier <michael@paquier.xyz> — 2019-08-01T06:01:33Z

    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

  12. Re: concerns around pg_lsn

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-08-01T07:09:26Z

    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

  13. Re: concerns around pg_lsn

    Michael Paquier <michael@paquier.xyz> — 2019-08-01T08:21:13Z

    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

  14. Re: concerns around pg_lsn

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-08-01T08:40:08Z

    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

  15. Re: concerns around pg_lsn

    Michael Paquier <michael@paquier.xyz> — 2019-08-01T09:12:29Z

    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

  16. Re: concerns around pg_lsn

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-08-04T03:41:09Z

    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

  17. Re: concerns around pg_lsn

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-08-04T03:57:01Z

    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

  18. Re: concerns around pg_lsn

    Michael Paquier <michael@paquier.xyz> — 2019-08-04T06:43:53Z

    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

  19. Re: concerns around pg_lsn

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-08-05T03:45:02Z

    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

  20. Re: concerns around pg_lsn

    Michael Paquier <michael@paquier.xyz> — 2019-08-05T06:36:32Z

    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

  21. Re: concerns around pg_lsn

    Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> — 2019-08-05T06:55:46Z

    >
> 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