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