Re: Combine pg_walinspect till_end_of_wal functions with others

On Mon, Mar 6, 2023 at 2:22 PM Julien Rouhaud <rjuju123(at)gmail(dot)com> wrote:
>
> > > 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.

Are you saying that the pg_walinspect functions should figure out the
oldest available WAL file and LSN, and start from there if start_lsn
specified as NULL or invalid? Note that pg_waldump requires either
explicit startlsn and/or startseg (WAL file name), it can't search for
the oldest WAL file available and start from there automatically.

If the user wants to figure it out, they can do something like below:

ostgres=# select * from pg_ls_waldir() order by name;
name | size | modification
--------------------------+----------+------------------------
000000010000000000000001 | 16777216 | 2023-03-06 14:54:55+00
000000010000000000000002 | 16777216 | 2023-03-06 14:54:55+00

If we try to make these functions figure out the oldest WAl file and
start from there, then it'll unnecessarily complicate the APIs and
functions. If we still think we need a better function for the users
to figure out the oldest WAL file, perhaps, add a SQL-only
view/function to pg_walinspect that returns "select name from
pg_ls_waldir() order by name limit 1;", but honestly, that's so
trivial.

> > > 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.

Thanks. Yes, that's true.

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

Will do.

> 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.
>
> 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.

Can you be more specific what's inaccurate about the comment?

> 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?

I thought about it initially, but wanted to avoid more errors. An
error would make them use the new version easily. I will change it
that way.

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com