Feedback-ID: id4a34324:Fastmail
Date: Thu, 26 Mar 2026 16:26:33 -0400
From: Andres Freund <andres@anarazel.de>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Sami Imseih <samimseih@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Thomas Munro <thomas.munro@gmail.com>
Subject: Re: dshash_find_or_insert vs. OOM
Message-ID: <gq55lyolnqm4yv7texawu7sr64btcczktu7hd4byl6zarxkibv@pb6ajailtqop>
References: 
 <CA+TgmoaJwUukUZGu7_yL74oMTQQz2=zqucMhF9+9xBmSC5us1w@mail.gmail.com>
 <CAA5RZ0vNVdBnm_TSCR5K6fT7yc7-jQuneNUYFxFgS7hQnn62sA@mail.gmail.com>
 <CA+TgmoZ3cd+wh1vz8gB_iD9zxZX558xUERaPm7G-M8wa7p9GNg@mail.gmail.com>
 <CAA5RZ0tWjsthTod0ODPEW4noNrSvY+gRWTMxiQ89-yY-k92B8Q@mail.gmail.com>
 <CAA5RZ0uhz2uEtUV1KqoCHKQY4U3pgB6N1n-oR-gy8TPUoenQSA@mail.gmail.com>
 <CA+TgmoYmzRbNjx8sbm_0Hk-VtLBtJPe=juOUSFkm7ge4Xy7hYw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CA+TgmoYmzRbNjx8sbm_0Hk-VtLBtJPe=juOUSFkm7ge4Xy7hYw@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/gq55lyolnqm4yv7texawu7sr64btcczktu7hd4byl6zarxkibv%40pb6ajailtqop>
Precedence: bulk

Hi,

On 2026-03-25 13:55:02 -0400, Robert Haas wrote:
> So the patch definitely has some value: it adds coverage of 60+ lines
> of code. On the other hand, it still feels to me like we'd be far more
> likely to notice new dshash bugs based on its existing usages than
> because of this. If I were modifying dshash, I wouldn't run this test
> suite first; I'd run the regression tests first. And the patch does
> have a cost: it's 334 lines of code that will have to be maintained
> going forward. I don't know if that's worth it. I feel like we're a
> lot more likely to break dshash behavior in some complicated
> concurrency scenario than we are to break it in a test like this. And,
> if somebody modifies dshash_destory() or dshash_dump(), they just need
> to test the code at least once before committing to get essentially
> the same benefit we'd get from this, which you would hope people would
> do anyway. None of this is to say that this patch is a horrible idea,
> but I'm also not entirely convinced that it is an excellent idea, so I
> would like to hear some other opinions.
> 
> Of course, there's also the fact that this patch is quite similar to
> some other test_* modules that we already have, like test_dsa or
> test_bitmapset. So maybe those are good and this is also good. But I'm
> not sure. The good news is, if we do want this, it probably doesn't
> need much more work to be committable.

I think tests like this do have value and I'd definitely run them first while
hacking on code related to dshash, rather than relying on the regression tests
or such.  E.g. having test_aio was invaluable to being able to get AIO into a
stable state.  When hacking on something with complicated edge cases I'd just
add a test for it, making development faster as well as ensuring the
complicated case continues to work into the future.

However, creating its own test module for small parts of the codebase doesn't
quite make sense to me. A pretty decent chunk of the test is just boilerplate
to add a new module, and every new test module requires its own cluster, which
adds a fair bit of runtime overhead, particularly on windows.  I think
test_dsa, test_dsm_registry, test_dshash should just be one combined test
module, they're testing quite closely related code.

Greetings,

Andres Freund