On Fri, Nov 11, 2022 at 5:52 PM Maxim Orlov <orlovmg@gmail.com> wrote:
>
> Hi!
>
> I've watched over the patch and consider it useful. Applies without conflicts. The functionality of the patch itself
is
> meets declared functionality.
Thanks for reviewing.
> But, in my view, some improvements may be proposed. We should be more, let's say, cautious (or a paranoid if you
wish),
> in pg_walfile_offset_lsn while dealing with user input values. What I mean by that is:
> - to init input vars of sscanf, i.e. tli, log andseg;
> - check the return value of sscanf call;
> - check filename max length.
IsXLogFileName() will take care of this. Also, I've added a new inline
function XLogIdFromFileName() that parses file name and returns log
and seg along with XLogSegNo and timeline id. This new function avoids
an extra sscanf() call as well.
> Another question arises for me: is this function can be called during recovery? If not, we should ereport about this,
shouldwe?
It's just a utility function and doesn't depend on any of the server's
current state (unlike pg_walfile_name()), hence it doesn't matter if
this function is called during recovery.
> And one last note here: pg_walfile_offset_lsn is accepting NULL values and return NULL in this case. From a
theoretical
> point of view, this is perfectly fine. Actually, I think this is exactly how it supposed to be, but I'm not sure if
thereare no other opinions here.
These functions are marked as 'STRICT', meaning a null is returned,
without even calling the function, if any of the input parameters is
null. I think we can keep the behaviour the same as its friends.
On Fri, Nov 11, 2022 at 11:05 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>
Thanks for reviewing.
> Hmm, in 0002, why not return the timeline from the LSN too? It seems a
> waste not to have it.
Yeah, that actually makes sense. We might be tempted to return segno
too, but it's not something that we emit to the user elsewhere,
whereas we emit timeline id.
> + ereport(ERROR,
> + (errcode(ERRCODE_NUMERIC_VALUE_OUT_OF_RANGE),
> + errmsg("\"offset\" must not be negative or greater than or "
> + "equal to WAL segment size")));
>
> I don't think the word offset should be in quotes; and please don't cut
> the line. So I propose
>
> errmsg("offset must not be negative or greater than or equal to the WAL segment size")));
Changed.
While on this, I noticed that the pg_walfile_name_offset() isn't
covered in tests. I took an opportunity and added a simple test case
along with pg_walfile_offset_lsn().
I'm attaching the v3 patch set for further review.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
