Re: Review: compact fsync request queue on overflow

On Mon, Jan 17, 2011 at 1:43 PM, Chris Browne <cbbrowne(at)acm(dot)org> wrote:
>      (I observe that it wasn't all that obvious that "hash_search()"
>      *adds* elements that are missing.  I got confused and went
>      looking for "hash_add() or similar.  It's permissible to say "dumb
>      Chris".)

I didn't invent that API. It is a little strange, but you get the hang of it.

>      Question: Is there any further cleanup needed for the entries
>      that got "dropped" out of BGWriterShmem->requests?  It seems
>      not, but a leak seems conceivable.

They're just holding integers, so what's to clean up?

>      - Concurrent access...
>
>        Is there anything that can write extra elements to
>        BGWriterShmem->requests while this is running?
>
>        I wonder if we need to have any sort of lock surrounding this?

It's called with the BgWriterCommLock already held, and there's an
assertion to this effect as well.

>      With higher shared memory, I couldn't readily induce compaction,
>      which is probably a concurrency matter of not having enough volume
>      of concurrent work going on.

You can do it artificially by attaching gdb to the bgwriter.

>      In principle, the only case where it should worsen performance
>      is if the amount of time required to:
>        - Set up a hash table
>        - Insert an entry for each buffer
>        - Walk the skip_slot array, shortening the request queue
>          for each duplicate found
>      exceeded the amount of time required to do the duplicate fsync()
>      requests.
>
>      That cost should be mighty low.  It would be interesting to
>      instrument CompactBgwriterRequestQueue() to see how long it runs.
>
>      But note that this cost is also spread into a direction where it
>      likely wouldn't matter, as it is typically invoked by the
>      background writer process, so this would frequently not be paid
>      by "on-line" active processes.

This is not correct. The background writer only ever does
AbsorbFsyncRequests(); this would substitute (to some degree) in cases
where the background writer fell down on the job.

> bgwriter.c: In function 'CompactBgwriterRequestQueue':
> bgwriter.c:1142: warning: 'num_skipped' may be used uninitialized in this function

OK, I can fix that.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company