Horton

Re: new heapcheck contrib module

Mark Dilger <mark.dilger@enterprisedb.com>

View thread View on postgr.es
From: Mark Dilger <mark.dilger@enterprisedb.com>
To: Robert Haas <robertmhaas@gmail.com>, Andres Freund <andres@anarazel.de>
Cc: Dilip Kumar <dilipbalaut@gmail.com>, Peter Geoghegan <pg@bowt.ie>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2020-07-31T15:51:50Z
Lists: pgsql-hackers 

> On Jul 31, 2020, at 5:02 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> On Thu, Jul 30, 2020 at 9:38 PM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
>>> On Jul 30, 2020, at 5:53 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> On Thu, Jul 30, 2020 at 6:10 PM Mark Dilger
>> Since I can't think of a plausible concrete example of corruption which would elicit the problem I was worrying about, I'll withdraw the argument.  But that leaves me wondering about a comment that Andres made upthread:
>> 
>>> On Apr 20, 2020, at 12:42 PM, Andres Freund <andres@anarazel.de> wrote:
>> 
>>> I don't think random interspersed uses of CLogTruncationLock are a good
>>> idea. If you move to only checking visibility after tuple fits into
>>> [relfrozenxid, nextXid), then you don't need to take any locks here, as
>>> long as a lock against vacuum is taken (which I think this should do
>>> anyway).
> 
> The version of the patch I'm looking at doesn't seem to mention
> CLogTruncationLock at all, so I'm confused about the comment. But what
> it is that you are wondering about exactly?

In earlier versions of the patch, I was guarding (perhaps unnecessarily) against clog truncation, (perhaps incorrectly) by taking the CLogTruncationLock (aka XactTruncationLock.) .  I thought Andres was arguing that such locks were not necessary "as long as a lock against vacuum is taken".  That's what motivated me to remove the clog locking business and put in the ShareUpdateExclusive lock.  I don't want to remove the ShareUpdateExclusive lock from the patch without perhaps a clarification from Andres on the subject.  His recent reply upthread seems to still support the idea that some kind of protection is required:

> I think it's not at all ok to look in the procarray or clog for xids
> that are older than what you're announcing you may read. IOW I don't
> think it's OK to just ignore the problem, or try to work around it by
> holding XactTruncationLock.

I don't understand that paragraph fully, in particular the part about "than what you're announcing you may read", since the cached value of relfrozenxid is not announced; we're just assuming that as long as vacuum cannot advance it during our scan, that we should be safe checking whether xids newer than that value (and not in the future) were committed.

Andres?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

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.