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

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

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

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

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