From: | Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> |
---|---|
To: | "k(dot)jamison(at)fujitsu(dot)com" <k(dot)jamison(at)fujitsu(dot)com> |
Cc: | "'pgsql-hackers(at)postgresql(dot)org'" <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [Patch] Optimize dropping of relation buffers using dlist |
Date: | 2019-11-05 15:34:30 |
Message-ID: | 20191105153430.ewyvysqnftlaammc@development |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi Kirk,
On Tue, Nov 05, 2019 at 09:58:22AM +0000, k(dot)jamison(at)fujitsu(dot)com wrote:
>Hi,
>
>
>> Another one that I'd need feedback of is the use of new dlist operations
>
>> for this cached buffer list. I did not use in this patch the existing
>
>> Postgres dlist architecture (ilist.h) because I want to save memory space
>
>> as much as possible especially when NBuffers become large. Both dlist_node
>
>> & dlist_head are 16 bytes. OTOH, two int pointers for this patch is 8 bytes.
>
>In cb_dlist_combine(), the code block below can impact performance
>especially for cases when the doubly linked list is long (IOW, many cached buffers).
> /* Point to the tail of main dlist */
> while (curr_main->next != CACHEDBLOCK_END_OF_LIST)
> curr_main = cb_dlist_next(curr_main);
>
>Attached is an improved version of the previous patch, which adds a pointer
>information of the TAIL field in order to speed up the abovementioned operation.
>I stored the tail field in the prev pointer of the head entry (maybe not a typical
>approach). A more typical one is by adding a tail field (int tail) to CachedBufferEnt,
>but I didn’t do that because as I mentioned in previous email I want to avoid
>using more memory as much as possible.
>The patch worked as intended and passed the tests.
>
>Any thoughts?
>
A couple of comments based on briefly looking at the patch.
1) I don't think you should / need to expose most of the ne stuff in
buf_internals.h. It's only used from buf_internals.c and having all
the various cb_dlist_* function in .h seems strange.
2) This adds another hashtable maintenance to BufferAlloc etc. but
you've only done tests / benchmark for the case this optimizes. I
think we need to see a benchmark for workload that allocates and
invalidates lot of buffers. A pgbench with a workload that fits into
RAM but not into shared buffers would be interesting.
3) I see this triggered a failure on cputube, in the commit_ts TAP test.
That's a bit strange, someone should investigate I guess.
https://travis-ci.org/postgresql-cfbot/postgresql/builds/607563900
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From | Date | Subject | |
---|---|---|---|
Next Message | Dilip Kumar | 2019-11-05 15:50:15 | Re: cost based vacuum (parallel) |
Previous Message | Andres Freund | 2019-11-05 15:19:28 | Re: cost based vacuum (parallel) |