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