Hi Michael, Thanks for your inputs, really appreciate.
On Tue, Jul 30, 2019 at 9:42 AM Michael Paquier <mich...@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
