Re: concerns around pg_lsn

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