Re: pg_walinspect - a new extension to get raw WAL data and WAL stats

On Wed, Feb 16, 2022 at 1:01 AM Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> On Mon, Feb 14, 2022 at 8:32 PM Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
> >
> > Here are few comments:
>
> Thanks for reviewing the patches.
>
> > +/*
> > + * Verify the authenticity of the given raw WAL record.
> > + */
> > +Datum
> > +pg_verify_raw_wal_record(PG_FUNCTION_ARGS)
> > +{
> >
> >
> > Do we really need this function? I see that whenever the record is
> > read, we verify it. So could there be a scenario where any of these
> > functions would return an invalid WAL record?
>
> Yes, this function can be useful. Imagine a case where raw WAL records
> are fetched from one server using pg_get_wal_record_info and sent over
> the network to another server (for fixing some of the corrupted data
> pages or for whatever reasons), using pg_verify_raw_wal_record one can
> verify authenticity.
>

I don't think that's the use case of this patch. Unless there is some
other valid reason, I would suggest you remove it.

> > Should we add a function that returns the pointer to the first and
> > probably the last WAL record in the WAL segment? This would help users
> > to inspect the wal records in the entire wal segment if they wish to
> > do so.
>
> Good point. One can do this already with pg_get_wal_records_info and
> pg_walfile_name_offset. Usually, the LSN format itself can give an
> idea about the WAL file it is in.
>
> postgres=# select lsn, pg_walfile_name_offset(lsn) from
> pg_get_wal_records_info('0/5000000', '0/5FFFFFF') order by lsn asc
> limit 1;
> lsn | pg_walfile_name_offset
> -----------+-------------------------------
> 0/5000038 | (000000010000000000000005,56)
> (1 row)
>
> postgres=# select lsn, pg_walfile_name_offset(lsn) from
> pg_get_wal_records_info('0/5000000', '0/5FFFFFF') order by lsn desc
> limit 1;
> lsn | pg_walfile_name_offset
> -----------+-------------------------------------
> 0/5FFFFC0 | (000000010000000000000005,16777152)
> (1 row)
>

The workaround you are suggesting is not very user friendly and FYKI
pg_wal_records_info simply hangs at times when we specify the higher
and lower limit of lsn in a wal file.

To make things easier for the end users I would suggest we add a
function that can return a valid first and last lsn in a walfile. The
output of this function can be used to inspect the wal records in the
entire wal file if they wish to do so and I am sure they will. So, it
should be something like this:

select first_valid_lsn, last_valid_lsn from
pg_get_first_last_valid_wal_record('wal-segment-name');

And above function can directly be used with pg_get_wal_records_info() like

select pg_get_wal_records_info(pg_get_first_last_valid_wal_record('wal-segment'));

I think this is a pretty basic ASK that we expect to be present in the
module like this.

> > +PG_FUNCTION_INFO_V1(pg_get_raw_wal_record);
> > +PG_FUNCTION_INFO_V1(pg_get_first_valid_wal_record_lsn);
> > +PG_FUNCTION_INFO_V1(pg_verify_raw_wal_record);
> > +PG_FUNCTION_INFO_V1(pg_get_wal_record_info);
> > +PG_FUNCTION_INFO_V1(pg_get_wal_records_info);
> >
> > I think we should allow all these functions to be executed in wait and
> > *nowait* mode. If a user specifies nowait mode, the function should
> > return if no WAL data is present, rather than waiting for new WAL data
> > to become available, default behaviour could be anything you like.
>
> Currently, pg_walinspect uses read_local_xlog_page which waits in the
> while(1) loop if a future LSN is specified. As read_local_xlog_page is
> an implementation of XLogPageReadCB, which doesn't have a wait/nowait
> parameter, if we really need a wait/nowait mode behaviour, we need to
> do extra things(either add a backend-level global wait variable, set
> before XLogReadRecord, if set, read_local_xlog_page can just exit
> without waiting and reset after the XLogReadRecord or add an extra
> bool wait variable to XLogReaderState and use it in
> read_local_xlog_page).
>

I am not asking to do any changes in the backend code. Please check -
how pg_waldump does this when a user requests to stop once the endptr
has reached. If not for all functions at least for a few functions we
can do this if it is doable.

>
> > +Datum
> > +pg_get_wal_records_info(PG_FUNCTION_ARGS)
> > +{
> > +#define PG_GET_WAL_RECORDS_INFO_COLS 10
> >
> >
> > We could probably have another variant of this function that would
> > work even if the end pointer is not specified, in which case the
> > default end pointer would be the last WAL record in the WAL segment.
> > Currently it mandates the use of an end pointer which slightly reduces
> > flexibility.
>
> Last WAL record in the WAL segment may not be of much use(one can
> figure out the last valid WAL record in a wal file as mentioned
> above), but the WAL records info till the latest current flush LSN of
> the server would be a useful functionality. But that too, can be found
> using something like "select lsn, prev_lsn, resource_manager from
> pg_get_wal_records_info('0/8099568', pg_current_wal_lsn());"
>

What if a user wants to inspect all the valid wal records from a
startptr (startlsn) and he doesn't know the endptr? Why should he/she
be mandated to get the endptr and supply it to this function? I don't
think we should force users to do that. I think this is again a very
basic ASK that can be done in this version itself. It is not at all
any advanced thing that we can think of doing in the future.

> > +
> > +/*
> > + * Get the first valid raw WAL record lsn.
> > + */
> > +Datum
> > +pg_get_first_valid_wal_record_lsn(PG_FUNCTION_ARGS)
> >
> >
> > I think this function should return a pointer to the nearest valid WAL
> > record which can be the previous WAL record to the LSN entered by the
> > user or the next WAL record. If a user unknowingly enters an lsn that
> > does not exist then in such cases we should probably return the lsn of
> > the previous WAL record instead of hanging or waiting for the new WAL
> > record to arrive.
>
> Is it useful?

It is useful in the same way as returning the next valid wal pointer
is. Why should a user wait for the next valid wal pointer to be
available instead the function should identify the previous valid wal
record and return it and put an appropriate message to the user.

If there's a strong reason, how about naming
> pg_get_next_valid_wal_record_lsn returning the next valid wal record
> LSN and pg_get_previous_valid_wal_record_lsn returning the previous
> valid wal record LSN ? If you think having two functions is too much,
> then, how about pg_get_first_valid_wal_record_lsn returning both the
> next valid wal record LSN and its previous wal record LSN?
>

The latter one looks better.

--
With Regards,
Ashutosh Sharma.