Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add tests for low-level PGLZ [de]compression routines

  2. Fix heap-buffer-overflow in pglz_decompress() on corrupt input.

  1. test_compression, test module for low-level compression APIs (for 2b5ba2a0a141)

    Michael Paquier <michael@paquier.xyz> — 2026-04-13T00:37:55Z

    Hi all,
(Andrew in CC.)

While reading Andrew's commit 2b5ba2a0a141, I was a bit sad to not see
tests for these problems with pglz, applied with the fix down to v14.
Relying on fuzzing is not really cool, because these consume resources
and they may not even hit the correct target, and we want a maximum of
deterministic tests.

And then, I got reminded that one of my pet plugins does something
close to that (used that around 9.5 for some FPW compression
benchmarks):
https://github.com/michaelpq/pg_plugins/tree/main/compress_test

With this infrastructure already at hand, implementing the
problematic tests with corrupted varlenas was a matter of minutes,
leading me to the attached patch (bonus points for check_comprete and
rawsize).

I would like to apply that down to v14, like the previous commit that
has fixed these cases with pglz.  That should come in handy in case
more bugs pop in this area of the code, especially with more
compression methods in mind.

Any objections and/or comments about that?
--
Michael

  2. Re: test_compression, test module for low-level compression APIs (for 2b5ba2a0a141)

    Andres Freund <andres@anarazel.de> — 2026-04-13T02:20:43Z

    Hi,

On 2026-04-13 09:37:55 +0900, Michael Paquier wrote:
> With this infrastructure already at hand, implementing the
> problematic tests with corrupted varlenas was a matter of minutes,
> leading me to the attached patch (bonus points for check_comprete and
> rawsize)

I think it doesn't scale to have a whole postgres cluster for a test that
takes a few milliseconds.  The amount of IO one run of all of postgres' tests
is doing is getting unmangeable, and lots of clusters that are used for a a
second or two that are immediately destroyed contributes substantially to
that.

One PG_TEST_NOCLEAN=1 run with meson ends up with a 33GB testrun/ directory.
And that's without even counting all the pg_regress tests, because there's no
convenient way to disable that.  On a smaller machine much of that will be
written to disk due to cache/memory pressure.

There's really no reason for something like this to be a test doing tests via
SQL from what I can tell.

If it does not to be via SSL, can we please start to find a way to combine
tiny stuff like this?  We're working hard at making our tests grow
unsustainable.

Greetings,

Andres Freund

  3. Re: test_compression, test module for low-level compression APIs (for 2b5ba2a0a141)

    Michael Paquier <michael@paquier.xyz> — 2026-04-13T03:52:59Z

    On Sun, Apr 12, 2026 at 10:20:43PM -0400, Andres Freund wrote:
> There's really no reason for something like this to be a test doing tests via
> SQL from what I can tell.
> 
> If it does not to be via SSL, can we please start to find a way to combine
> tiny stuff like this?  We're working hard at making our tests grow
> unsustainable.

If we care about `make check` rather than `make installcheck`, it
seems to me that a solution already exists in the shape of C function
called through the main regression test suite.  And for the specific
case of this thread, I could live with two new functions in regress.c
that are then called in compression.sql.

Would this idea work for you when it comes to this proposal?
--
Michael

  4. Re: test_compression, test module for low-level compression APIs (for 2b5ba2a0a141)

    Andres Freund <andres@anarazel.de> — 2026-04-13T14:08:16Z

    Hi,

On 2026-04-13 12:52:59 +0900, Michael Paquier wrote:
> On Sun, Apr 12, 2026 at 10:20:43PM -0400, Andres Freund wrote:
> > There's really no reason for something like this to be a test doing tests via
> > SQL from what I can tell.
> > 
> > If it does not to be via SSL, can we please start to find a way to combine
> > tiny stuff like this?  We're working hard at making our tests grow
> > unsustainable.
> 
> If we care about `make check` rather than `make installcheck`

I don't really see the difference WRT that in this case? installcheck still
has regress.so available, no?


> it seems to me that a solution already exists in the shape of C function
> called through the main regression test suite.

I guess that'd work.  I don't really see the point in calling these via SQL,
tbh, but if that's easier for you, using a regress.c helper woul do the trick.


> Would this idea work for you when it comes to this proposal?

Yes.


I think we need to combine about half the modules in src/test/modules, the
current course is absurd:

16:  37
17:  46
18:  49
dev: 62


[Almost] All of them create a new initdb'd cluster. ~50MB of writes for a
short test is insane.  Doing low-level unittests by doing inter-process
communication from psql to the server, marshalling everything through text, is
insane.

Greetings,

Andres Freund

  5. Re: test_compression, test module for low-level compression APIs (for 2b5ba2a0a141)

    Jacob Champion <jacob.champion@enterprisedb.com> — 2026-04-13T16:06:56Z

    On Mon, Apr 13, 2026 at 7:08 AM Andres Freund <andres@anarazel.de> wrote:
>
> Doing low-level unittests by doing inter-process
> communication from psql to the server, marshalling everything through text, is
> insane.

+1. (I'm more than happy to provide eyes/code/pairing/etc. for C unit
test infrastructure, since I've been pushing on that from the client
side recently.)

--Jacob

  6. Re: test_compression, test module for low-level compression APIs (for 2b5ba2a0a141)

    Michael Paquier <michael@paquier.xyz> — 2026-04-13T17:13:52Z

    On Mon, Apr 13, 2026 at 10:08:16AM -0400, Andres Freund wrote:
> I guess that'd work.  I don't really see the point in calling these via SQL,
> tbh, but if that's easier for you, using a regress.c helper woul do the trick.

Okay, thanks for the opinion.  It makes the addition of more tests
slightly easier as there is no need to rely on an extra wrapper of
what's in pg_lzcompress.h.  I have done that in the attached, with
additions to the main regression test suite.
--
Michael

  7. Re: test_compression, test module for low-level compression APIs (for 2b5ba2a0a141)

    Andres Freund <andres@anarazel.de> — 2026-04-13T18:50:31Z

    Hi,

On 2026-04-14 02:13:52 +0900, Michael Paquier wrote:
> +++ b/src/test/regress/parallel_schedule
> @@ -128,7 +128,7 @@ test: partition_merge partition_split partition_join partition_prune reloptions
>  # event_trigger depends on create_am and cannot run concurrently with
>  # any test that runs DDL
>  # oidjoins is read-only, though, and should run late for best coverage
> -test: oidjoins event_trigger
> +test: oidjoins event_trigger compression_pglz

The new test creates a function, so I don't think this will be safe?

Greetings,

Andres Freund

  8. Re: test_compression, test module for low-level compression APIs (for 2b5ba2a0a141)

    Michael Paquier <michael@paquier.xyz> — 2026-04-13T19:01:53Z

    On Mon, Apr 13, 2026 at 02:50:31PM -0400, Andres Freund wrote:
> Hi,
> 
> On 2026-04-14 02:13:52 +0900, Michael Paquier wrote:
> > +++ b/src/test/regress/parallel_schedule
>> @@ -128,7 +128,7 @@ test: partition_merge partition_split partition_join partition_prune reloptions
>>  # event_trigger depends on create_am and cannot run concurrently with
>>  # any test that runs DDL
>>  # oidjoins is read-only, though, and should run late for best coverage
>> -test: oidjoins event_trigger
>> +test: oidjoins event_trigger compression_pglz
> 
> The new test creates a function, so I don't think this will be safe?

Yep, that was a thinko.  The group for plancache still has room for
one test, as it can go up to 19, so we could move one test in the
group of partition_merge t the group of plancache, keeping all the
compression tests together.

Anyway, how about just creating a new group for all the compression
tests?  I have a patch set in preparation for zstd and TOAST, where I
would put the new test suite in this group.
--
Michael

  9. Re: test_compression, test module for low-level compression APIs (for 2b5ba2a0a141)

    Michael Paquier <michael@paquier.xyz> — 2026-04-14T21:16:07Z

    On Tue, Apr 14, 2026 at 04:01:53AM +0900, Michael Paquier wrote:
> Anyway, how about just creating a new group for all the compression
> tests?  I have a patch set in preparation for zstd and TOAST, where I
> would put the new test suite in this group.

At the end I have used a new group.  For branches older than v18, the
parallel groups close by had a lot more room, but I have given
priority to consistency with HEAD in parallel_schedule across the
board.
--
Michael