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

Hi,

On 2022-03-22 21:57:51 +0530, Bharath Rupireddy wrote:
> On Sat, Mar 19, 2022 at 5:18 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2022-03-17 13:25:35 +0530, Bharath Rupireddy wrote:
> > > +--
> > > +-- pg_get_raw_wal_record()
> >
> > What is raw about the function?
>
> It right now gives data starting from the output of XLogReadRecord
> upto XLogRecGetTotalLen(xlogreader); length. Given that XLogReadRecord
> returns a pointer to the decoded record's header, I'm not sure it's
> the right choice. Actually, this function's intention(not an immediate
> use-case though), is to feed the WAL record to another function and
> then, say, repair a corrupted page given a base data page.
>
> As I said upthread, I'm open to removing this function for now, when a
> realistic need comes we can add it back. It also raised some concerns
> around the security and permissions. Thoughts?

I'm ok with having it with appropriate permissions, I just don't like the
name.

> > Why "CALLED ON NULL INPUT"? It doesn't make sense to call the function with a
> > NULL lsn, does it? Also, that's the default, why is it restated here?
>
> pg_get_wal_records_info needed that option (if end_lsn being the
> default, providing WAL info upto the end of WAL). Also, we can emit
> better error message ("invalid WAL start LSN") instead of generic one.
> I wanted to keep error message and code same across all the functions
> hence CALLED ON NULL INPUT option for pg_get_raw_wal_record.

I think it should be strict if it behaves strict. I fail to see what
consistency in error messages is worth here. And I'd probably just create two
different functions for begin and begin & end LSN and mark those as strict as
well.

> > > +REVOKE EXECUTE ON FUNCTION pg_get_wal_record_info(pg_lsn) FROM PUBLIC;
> > > +GRANT EXECUTE ON FUNCTION pg_get_wal_record_info(pg_lsn) TO pg_monitor;
> >
> > I don't think it's appropriate for pg_monitor to see all the data in the WAL.
>
> How about pg_read_server_files or some other?

That seems more appropriate.

> > > +-- pg_get_wal_stats()
> >
> > This seems like an exceedingly expensive way to compute this. Not just because
> > of doing the grouping, window etc, but also because it's serializing the
> > "data" field from pg_get_wal_records_info() just to never use it. With any
> > appreciatable amount of data the return value pg_get_wal_records_info() will
> > be serialized into a on-disk tuplestore.
> >
> > This is probably close to an order of magnitude slower than pg_waldump
> > --stats. Which imo renders this largely useless.
>
> Yeah that's true. Do you suggest having pg_get_wal_stats() a
> c-function like in v8 patch [1]?

Yes.

> SEe some numbers at [2] with pg_get_wal_stats using
> pg_get_wal_records_info and pg_get_wal_records_info as a direct
> c-function like in v8 patch [1]. A direct c-function always fares
> better (84 msec vs 1400msec).

That indeed makes the view as is pretty much useless. And it'd probably be
worse in a workload with longer records / many FPIs.

> > > +void
> > > +_PG_init(void)
> >
> > > +void
> > > +_PG_fini(void)
> >
> > Why have this stuff if it's not used?
>
> I kept it as a placeholder for future code additions, for instance
> test_decoding.c and ssl_passphrase_func.c has empty _PG_init(),
> _PG_fini(). If okay, I can mention there like "placeholder for now",
> otherwise I can remove it.

That's not comparable, the test_decoding case has it as a placeholder because
it serves as a template to create further output plugins. Something not the
case here. So please remove.

> > > + for (block_id = 0; block_id <= record->max_block_id; block_id++)
> > > + {
> >
> > To me duplicating this much code from waldump seems like a bad idea from a
> > maintainability POV.
>
> Even if we were to put the above code from pg_walinspect and
> pg_waldump into, say, walutils.c or some other existing file, there we
> had to make if (pg_walinspect) appendStringInfo else if (pg_waldump)
> printf() sort of thing, isn't it clumsy?

Why is that needed? Just use the stringinfo in both places? You're outputting
the exact same thing in both places right now. There's already a stringinfo in
XLogDumpDisplayRecord() these days (there wasn't back when pg_xlogddump was
written), so you could just convert at least the relevant printfs in
XLogDumpDisplayRecord().

> Also, unnecessary if
> conditions need to be executed for every record. For maintainability,
> I added a note in pg_walinspect.c and pg_waldump.c to consider fixing
> things in both places (of course this might sound dumbest way of doing
> it, IMHO, it's sensible, given the if(pg_walinspect)-else
> if(pg_waldump) sorts of code that we need to do in the common
> functions). Thoughts?

IMO we shouldn't merge this with as much duplication as there is right now,
the notes don't change that it's a PITA to maintain.

> Yeah. It is to handle some edge cases to print the WAL upto end_lsn
> and avoid waiting in read_local_xlog_page. I will change it.
>
> Actually, there's an open point as specified in [3]. Any thoughts on it?

Seems more user-friendly to wait - it's otherwise hard for a user to know what
LSN to put in.

Greetings,

Andres Freund