Re: Combine pg_walinspect till_end_of_wal functions with others

On Tue, Mar 14, 2023 at 5:02 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> So let's be clean and drop these slots to keep the tests
> self-contained. pg_walinspect in REL_15_STABLE gets that right, IMV,
> and that's no different from the role cleanup, as one example.

Hm, added replication slot drop back.

> > As hackers we know that these functions have been removed and how to
> > achieve till_end_of_wal with the other functions. I noticed that
> > you've removed my changes (see below) from the docs that were saying
> > how to get info/stats till_end_of_wal. That leaves end users confused
> > as to how they can achieve till_end_of_wal functionality. All users
> > can't look for commit history/message but they can easily read the
> > docs. I prefer to have the following (did so in the attached v7) and
> > get rid of the above note if you don't feel strongly about it.
> >
> > + If a future <replaceable>end_lsn</replaceable>
> > + (i.e. the LSN server doesn't know about) is specified, it returns
> > + informaton till end of WAL.
>
> FWIW, I don't see a strong need for that, because this documents a
> behavior where we would not fail. And FWIW, it just feel natural to
> me because the process stops the scan up to where it can. In short,
> it should be enough for the docs to mention the error patterns,
> nothing else.

My thoughts are simple here - how would one (an end user, not me and
not you) figure out how to get info/stats till the end of WAL? I'm
sure it would be difficult to find that out without looking at the
code or commit history. Search for till end of WAL behaviour with new
version will be more given the 1.0 version has explicit functions to
do that. IMO, there's no harm in being explicit in how to achieve till
end of WAL functionality around in the docs.

> > 3. I wanted COUNT(*) >= 0 for successful function execution to be
> > COUNT(*) >= 1 so that we will check for at least the functions
> > returning 1 record. And failures to be SELECT * FROM. This was my
> > intention but I don't see that in this patch or in the previous
> > test-refactoring commit. I added that in the attached v7 patch again.
> > Also, made test comments conssitent.
>
> Noticed that as well, still it feels to me that these had better be
> separated from the rest, and be in their own patch, perhaps *after*
> the main patch discussed on this thread, or just moved into their own
> threads. If a commit finishes with a list of bullet points referring
> to a list of completely different things than the subject, there may
> be a problem. In this v7, we have:
> - Change the behavior of the functions for end LSNs, tweaking the
> tests to do so.
> - Adjust more comments and formats in the tests.
> - Adjust some tests to be pickier with detection of generated WAL
> records.
> - Remove the drop slot calls.
> But what we need to care most here is the first point.

I get it. I divided the patches to 0001 and 0002 with 0001 focussing
on the change of behaviour around future end LSNs, dropping till end
of WAL functions and tests tweakings related to it. 0002 has all other
tests tidy up things.

Please find the attached v8 patch set for further review.

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