Horton

Re: new heapcheck contrib module

Robert Haas <robertmhaas@gmail.com>

View thread View on postgr.es
From: Robert Haas <robertmhaas@gmail.com>
To: Mark Dilger <mark.dilger@enterprisedb.com>
Cc: Thomas Munro <thomas.munro@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, Peter Geoghegan <pg@bowt.ie>, "Andrey M. Borodin" <x4mmm@yandex-team.ru>, Stephen Frost <sfrost@snowman.net>, Michael Paquier <michael@paquier.xyz>, Amul Sul <sulamul@gmail.com>, Dilip Kumar <dilipbalaut@gmail.com>, Andres Freund <andres@anarazel.de>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2021-02-03T22:03:14Z
Lists: pgsql-hackers 
On Tue, Feb 2, 2021 at 6:10 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote:
> 0001 -- no changes

Committed.

> 0002 -- fixing omissions in @pgfeutilsfiles in file src/tools/msvc/Mkvcbuild.pm

Here are a few minor cosmetic issues with this patch:

- connect_utils.c lacks a file header comment.
- Some or perhaps all of the other file header comments need an update for 2021.
- There's bogus hunks in the diff for string_utils.c.

I think the rest of this looks good. I spent a long time puzzling over
whether consumeQueryResult() and processQueryResult() needed to be
moved, but then I realized that this patch actually makes them into
static functions inside parallel_slot.c, rather than public functions
as they were before. I like that. The only reason those functions need
to be moved at all is so that the scripts_parallel/parallel_slot stuff
can continue to do its thing, so this is actually a better way of
grouping things together than what we have now.

> 0003 -- no changes

I think it would be better if there were no handler by default, and
failing to set one leads to an assertion failure when we get to the
point where one would be called.

I don't think I understand the point of renaming processQueryResult
and consumeQueryResult. Isn't that just code churn for its own sake?

PGresultHandler seems too generic. How about ParallelSlotHandler or
ParallelSlotResultHandler?

I'm somewhat inclined to propose s/ParallelSlot/ConnectionSlot/g but I
guess it's better not to get sucked into renaming things.

It's a little strange that we end up with mutators to set the slot's
handler and handler context when we elsewhere feel free to monkey with
a slot's connection directly, but it's not a perfect world and I can't
think of anything I'd like better.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Commits

  1. Add pg_amcheck, a CLI for contrib/amcheck.

  2. Refactor and generalize the ParallelSlot machinery.

  3. Generalize parallel slot result handling.

  4. Move some code from src/bin/scripts to src/fe_utils to permit reuse.

  5. Factor pattern-construction logic out of processSQLNamePattern.

  6. Doc: clean up verify_heapam() documentation.

  7. Fix more portability issues in new amcheck code.

  8. Fix portability issues in new amcheck test.

  9. Try to avoid a compiler warning about using fxid uninitialized.

  10. Extend amcheck to check heap pages.

  11. Adjust walsender usage of xlogreader, simplify APIs

  12. Improve checking of child pages in contrib/amcheck.

  13. Sanitize line pointers within contrib/amcheck.

  14. Fix possible sorting error when aborting use of abbreviated keys.