Re: Combine pg_walinspect till_end_of_wal functions with others - Mailing list pgsql-hackers

On Wed, Mar 01, 2023 at 08:30:00PM +0530, Bharath Rupireddy wrote:
> On Wed, Mar 1, 2023 at 1:00 PM Bharath Rupireddy
> <bharath.rupireddyforpostgres@gmail.com> wrote:
> >
> > In a recent discussion [1], Michael Paquier asked if we can combine
> > pg_walinspect till_end_of_wal functions with other functions
> > pg_get_wal_records_info and pg_get_wal_stats. The code currently looks
> > much duplicated and the number of functions that pg_walinspect exposes
> > to the users is bloated. The point was that the till_end_of_wal
> > functions determine the end LSN and everything else that they do is
> > the same as their counterpart functions. Well, the idea then was to
> > keep things simple, not clutter the APIs, have better and consistent
> > user-inputted end_lsn validations at the cost of usability and code
> > redundancy. However, now I tend to agree with the feedback received.

+1, especially since I really don't like the use of "till" in the function
names.

> > I'm attaching a patch doing the $subject with the following behavior:
> > 1. If start_lsn is NULL, error out/return NULL.

Maybe naive and unrelated question, but is that really helpful?  If for some
reason I want to see information about *all available WAL*, I have to manually
dig for a suitable LSN.  The same action with pg_waldump is easier as I just
need to use the oldest available WAL that's present on disk.

> > Another idea is to convert till_end_of_wal flavors to SQL-only
> > functions and remove the c code from pg_walinspect.c. However, I
> > prefer $subject and completely remove till_end_of_wal flavors for
> > better usability in the long term.

I agree that using default arguments is a way better API.

Nitpicking:

Maybe we could group the kept unused exported C function at the end of the
file?

Also:

/*
- * Get info and data of all WAL records from start LSN till end of WAL.
+ * NB: This function does nothing and stays here for backward compatibility.
+ * Without it, the extension fails to install.
  *
- * This function emits an error if a future start i.e. WAL LSN the database
- * system doesn't know about is specified.
+ * Try using pg_get_wal_records_info() for the same till_end_of_wal
+ * functionaility.
  */
 Datum
 pg_get_wal_records_info_till_end_of_wal(PG_FUNCTION_ARGS)
 {
-   XLogRecPtr  start_lsn;
-   XLogRecPtr  end_lsn = InvalidXLogRecPtr;
-
-   start_lsn = PG_GETARG_LSN(0);
-
-   end_lsn = ValidateInputLSNs(true, start_lsn, end_lsn);
-
-   GetWALRecordsInfo(fcinfo, start_lsn, end_lsn);
-
-   PG_RETURN_VOID();
+   PG_RETURN_NULL();
 }

I don't like much this chunk (same for the other kept function).  Apart from
the obvious typo in "functionaility", I don't think that the comment is really
accurate.

Also, are we actually helping users if we simply return NULL there?  It's quite
possible that people will start to use the new shared lib while still having
the 1.1 SQL definition of the extension installed.  In that case, they will
simply retrieve a NULL row and may spend some time wondering why until they
eventually realize that their only option is to upgrade the extension first and
then use another function.  Why not make their life easier and explicity raise
a suitable error at the SQL level if users try to use those functions?