From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Heikki Linnakangas <hlinnaka(at)iki(dot)fi> |
Cc: | vignesh C <vignesh21(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Melanie Plageman <melanieplageman(at)gmail(dot)com>, Yura Sokolov <y(dot)sokolov(at)postgrespro(dot)ru>, Robert Haas <robertmhaas(at)gmail(dot)com>, David Rowley <dgrowleyml(at)gmail(dot)com> |
Subject: | Re: refactoring relation extension and BufferAlloc(), faster COPY |
Date: | 2023-02-11 22:20:14 |
Message-ID: | 20230211222014.tewxr36cululbhhk@awork3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
On 2023-02-10 18:38:50 +0200, Heikki Linnakangas wrote:
> I'll continue reviewing this, but here's some feedback on the first two
> patches:
>
> v2-0001-aio-Add-some-error-checking-around-pinning.patch:
>
> I wonder if the extra assertion in LockBufHdr() is worth the overhead. It
> won't add anything without assertions, of course, but still. No objections
> if you think it's worth it.
It's so easy to get confused about local/non-local buffers, that I think it is
useful. I think we really need to consider cleaning up the separation
further. Having half the code for local buffers in bufmgr.c and the other half
in localbuf.c, without a scheme that I can recognize, is not a good scheme.
It bothers me somewhat ConditionalLockBufferForCleanup() silently accepts
multiple pins by the current backend. That's the right thing for
e.g. heap_page_prune_opt(), but for something like lazy_scan_heap() it's
not. And yes, I did encounter a bug hidden by that when making vacuumlazy use
AIO as part of that patchset. That's why I made BufferCheckOneLocalPin()
externally visible.
> v2-0002-hio-Release-extension-lock-before-initializing-pa.patch:
>
> Looks as far as it goes. It's a bit silly that we use RBM_ZERO_AND_LOCK,
> which zeroes the page, and then we call PageInit to zero the page again.
> RBM_ZERO_AND_LOCK only zeroes the page if it wasn't in the buffer cache
> previously, but with P_NEW, that is always true.
It is quite silly, and it shows up noticably in profiles. The zeroing is
definitely needed in other places calling PageInit(), though. I suspect we
should have a PageInitZeroed() or such, that asserts the page is zero, but
otherwise skips it.
Seems independent enough from this series, that I'd probably tackle it
separately? If you prefer, I'm ok with adding a patch to this series instead,
though.
Greetings,
Andres Freund
From | Date | Subject | |
---|---|---|---|
Next Message | Andres Freund | 2023-02-11 22:25:06 | Re: refactoring relation extension and BufferAlloc(), faster COPY |
Previous Message | Andres Freund | 2023-02-11 22:04:13 | Re: refactoring relation extension and BufferAlloc(), faster COPY |