Hi Michael, > On Wed, Sep 11, 2024 at 04:07:06PM +0300, Aleksander Alekseev wrote: > > Commit 4ed8f0913bfd introduced long SLRU file names. The proposed > > patch removes SlruCtl->long_segment_names flag and makes SLRU always > > use long file names. This simplifies both the code and the API. > > Corresponding changes to pg_upgrade are included. > > That's leaner, indeed. > > > One drawback I see is that technically SLRU is an exposed API and > > changing it may affect third-party code. I'm not sure if we should > > seriously worry about this. Firstly, the change is trivial and > > secondly, it's not clear whether such third-party code even exists (we > > broke this API just recently in 4ed8f0913bfd and no one complained). > > Any third-party code using custom SLRUs would need to take care of > handling their upgrade path outside pg_upgrade. Not sure there are > any of them, TBH, but let's see. > > > I didn't include any tests for the new pg_upgrade code. To my > > knowledge we test it manually, with buildfarm members and during > > alpha- and beta-testing periods. Please let me know if you think there > > should be a corresponding TAP test. > > Removing the old API means that it is impossible to test a move from > short to long file names. That's OK by me to rely on the pg_upgrade > paths in the buildfarm code. We have a few of them.
Thanks for the feedback. > There is one thing I am wondering, here, though, which is to think > harder about a validity check at the end of 002_pg_upgrade.pl to make > sure that all the SLRU use long file names after running the tests. > That would mean thinking about a mechanism to list all of them from a > backend, rather than hardcode a list of them. Perhaps that's not > worth it, just dropping an idea in the bucket of ideas. I would guess > in the shape of a catalog that's able to represent at SQL level all > the SLRUs that exist in a backend. Hmm... IMO it would be a rather niche facility to maintain in PG core. At least I'm not aware of cases when a DBA wanted to list initialized SLRUs. Would it be convenient for core / extensions developers? Creating a breakpoint on SimpleLruInit() or adding a temporary elog() sounds simpler to me. It wouldn't hurt re-checking the segment file names in the TAP test but this would mean hardcoding catalog names which as I understand you want to avoid. With high probability PG wouldn't start if the corresponding piece of pg_upgrade is wrong (I checked more than once :). So I'm not entirely sure if it's worth the effort, but let's see what others think. -- Best regards, Aleksander Alekseev
