Re: concerns around pg_lsn

Hi Michael,

On Thu, Aug 1, 2019 at 1:51 PM Michael Paquier <michael(at)paquier(dot)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