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-03-01T21:14:35Z
Lists: pgsql-hackers 
On Wed, Feb 24, 2021 at 1:55 PM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
> [ new patches ]

Regarding 0001:

There seem to be whitespace-only changes to the comment for select_loop().

I wonder if the ParallelSlotsSetupOneDB()/ParallelSlotsSetupMinimal()
changes could be simpler. First idea: Suppose you had
ParallelSlotsSetup(numslots) that just creates the slot array with 0
connections, and then ParallelSlotsAdopt(slots, conn, cparams) if you
want to make it own an existing connection. That seems like it might
be cleaner. Second idea: Why not get rid of ParallelSlotsSetupOneDB()
altogether, and just let ParallelSlotsGetIdle() connect the other
slots as required? Preconnecting all slots before we do anything is
good because ... of what?

I also wonder if things might be simplified by introducing a wrapper
object, e.g. ParallelSlotArray. Suppose a ParallelSlotArray stores the
number of slots (num_slots), the array of actual PGconn objects, and
the ConnParams to be used for new connections, and the initcmd to be
used for new connections. Maybe also the progname. This seems like it
would avoid a bunch of repeated parameter passing: you could just
create the ParallelSlotArray with the right contents, and then pass it
around everywhere, instead of having to keep passing the same stuff
in. If you want to switch to connecting to a different DB, you tweak
the ConnParams - maybe using an accessor function - and the system
figures the rest out.

I wonder if it's really useful to generalize this to a point of caring
about all the ConnParams fields, too. Like, if you only provide
ParallelSlotUpdateDB(slotarray, dbname), then that's the only field
that can change so you don't need to care about the others. And maybe
you also don't really need to keep the ConnParams fields in every
slot, either. Like, couldn't you just say something like: if
(strcmp(PQdb(conn) , slotarray->cparams->dbname) != 0) { wrong DB,
can't reuse without a reconnect }? I know sometimes a dbname is really
a whole connection string, but perhaps we could try to fix that by
using PQconninfoParse() in the right place, so that what ends up in
the cparams is just the db name, not a whole connection string.

This is just based on a relatively short amount of time spent studying
the patch, so I might well be off-base here. What do you think?

-- 
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.