Re: xid wraparound danger due to INDEX_CLEANUP false

Hi,

On 2020-04-22 20:08:42 -0700, Peter Geoghegan wrote:
> I can get Valgrind to complain about it when the regression tests are
> run with the attached patch applied.

Nice! Have you checked how much of an incremental slowdown this causes?

> This patch is very rough -- it was just the first thing that I tried.
> I don't know how Valgrind remembers the status of shared memory
> regions across backends when they're marked with
> VALGRIND_MAKE_MEM_NOACCESS(). Even still, this idea looks promising. I
> should try to come up with a committable patch before too long.

IIRC valgrind doesn't at all share access markings across processes.

> The good news is that the error I showed is the only error that I see,
> at least with this rough patch + "make installcheck". It's possible
> that the patch isn't as effective as it could be, though. For one
> thing, it definitely won't detect incorrect buffer accesses where a
> pin is held but a buffer lock is not held. That seems possible, but a
> bit harder.

Given hint bits it seems fairly hard to make that a reliable check.

> +#ifdef USE_VALGRIND
> + if (!isLocalBuf)
> + {
> + Buffer b = BufferDescriptorGetBuffer(bufHdr);
> + VALGRIND_MAKE_MEM_DEFINED(BufferGetPage(b), BLCKSZ);
> + }
> +#endif

Hm. It's a bit annoying that we have to mark the contents defined. It'd
be kinda useful to be able to mark unused parts of pages as undefined
initially. But there's afaictl no way to just set/unset addressability,
while not touching definedness. So this is probably the best we can do
without adding a lot of complexity.

> if (isExtend)
> {
> /* new buffers are zero-filled */
> @@ -1039,6 +1047,12 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
> buf = GetBufferDescriptor(buf_id);
>
> valid = PinBuffer(buf, strategy);
> +#ifdef USE_VALGRIND
> + {
> + Buffer b = BufferDescriptorGetBuffer(buf);
> + VALGRIND_MAKE_MEM_DEFINED(BufferGetPage(b), BLCKSZ);
> + }
> +#endif

Why aren't we doing this in PinBuffer() and PinBuffer_Locked(), but at
their callsites?

> @@ -1633,6 +1653,12 @@ PinBuffer(BufferDesc *buf, BufferAccessStrategy strategy)
> buf_state))
> {
> result = (buf_state & BM_VALID) != 0;
> +#ifdef USE_VALGRIND
> + {
> + Buffer b = BufferDescriptorGetBuffer(buf);
> + VALGRIND_MAKE_MEM_DEFINED(BufferGetPage(b), BLCKSZ);
> + }
> +#endif
> break;
> }
> }

Oh, but we actually are doing it in PinBuffer() too?

> /*
> * Decrement the shared reference count.
> @@ -2007,6 +2039,12 @@ BufferSync(int flags)
> */
> if (pg_atomic_read_u32(&bufHdr->state) & BM_CHECKPOINT_NEEDED)
> {
> +#ifdef USE_VALGRIND
> + {
> + Buffer b = BufferDescriptorGetBuffer(bufHdr);
> + VALGRIND_MAKE_MEM_DEFINED(BufferGetPage(b), BLCKSZ);
> + }
> +#endif
> if (SyncOneBuffer(buf_id, false, &wb_context) & BUF_WRITTEN)
> {
> TRACE_POSTGRESQL_BUFFER_SYNC_WRITTEN(buf_id);

Shouldn't the pin we finally acquire in SyncOneBuffer() be sufficient?

> @@ -2730,6 +2768,12 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln)
> * Run PageGetLSN while holding header lock, since we don't have the
> * buffer locked exclusively in all cases.
> */
> +#ifdef USE_VALGRIND
> + {
> + Buffer b = BufferDescriptorGetBuffer(buf);
> + VALGRIND_MAKE_MEM_DEFINED(BufferGetPage(b), BLCKSZ);
> + }
> +#endif
> recptr = BufferGetLSN(buf);

This shouldn't be needed, as the caller ought to hold a pin:
*
* The caller must hold a pin on the buffer and have share-locked the
* buffer contents. (Note: a share-lock does not prevent updates of
* hint bits in the buffer, so the page could change while the write
* is in progress, but we assume that that will not invalidate the data
* written.)
*

Greetings,

Andres Freund