Thread
Commits
Same data as JSON:
GET /api/v1/messages/:b64id/commits
the thread's linked commits as JSON, with link sources.
API reference →
-
Add pg_buffercache_evict() function for testing.
- 13453eedd3f6 17.0 landed
-
Extension Enhancement: Buffer Invalidation in pg_buffercache
Palak Chaturvedi <chaturvedipalak1911@gmail.com> — 2023-06-30T10:46:50Z
I hope this email finds you well. I am excited to share that I have extended the functionality of the `pg_buffercache` extension by implementing buffer invalidation capability, as requested by some PostgreSQL contributors for improved testing scenarios. This marks my first time submitting a patch to pgsql-hackers, and I am eager to receive your expert feedback on the changes made. Your insights are invaluable, and any review or comments you provide will be greatly appreciated. The primary objective of this enhancement is to enable explicit buffer invalidation within the `pg_buffercache` extension. By doing so, we can simulate scenarios where buffers are invalidated and observe the resulting behavior in PostgreSQL. As part of this patch, a new function or mechanism has been introduced to facilitate buffer invalidation. I would like to hear your thoughts on whether this approach provides a good user interface for this functionality. Additionally, I seek your evaluation of the buffer locking protocol employed in the extension to ensure its correctness and efficiency. Please note that I plan to add comprehensive documentation once the details of this enhancement are agreed upon. This documentation will serve as a valuable resource for users and contributors alike. I believe that your expertise will help uncover any potential issues and opportunities for further improvement. I have attached the patch file to this email for your convenience. Your valuable time and consideration in reviewing this extension are sincerely appreciated. Thank you for your continued support and guidance. I am looking forward to your feedback and collaboration in enhancing the PostgreSQL ecosystem. The working of the extension: 1. Creating the extension pg_buffercache and then call select query on a table and note the buffer to be cleared. pgbench=# create extension pg_buffercache; CREATE EXTENSION pgbench=# select count(*) from pgbench_accounts; count -------- 100000 (1 row) pgbench=# SELECT * FROM pg_buffercache WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); bufferid | relfilenode | reltablespace | reldatabase | relforknumber | relblocknumber | isdirty | usagecount | pinning_backends ----------+-------------+---------------+-------------+---------------+----------------+---------+------------+------------------ 233 | 16397 | 1663 | 16384 | 0 | 0 | f | 1 | 0 234 | 16397 | 1663 | 16384 | 0 | 1 | f | 1 | 0 235 | 16397 | 1663 | 16384 | 0 | 2 | f | 1 | 0 236 | 16397 | 1663 | 16384 | 0 | 3 | f | 1 | 0 237 | 16397 | 1663 | 16384 | 0 | 4 | f | 1 | 0 2. Clearing a single buffer by entering the bufferid. pgbench=# SELECT count(*) FROM pg_buffercache WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); count ------- 1660 (1 row) pgbench=# select pg_buffercache_invalidate(233); pg_buffercache_invalidate --------------------------- t (1 row) pgbench=# SELECT count(*) FROM pg_buffercache WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); count ------- 1659 (1 row) 3. Clearing the entire buffer for a relation using the function. pgbench=# SELECT count(*) FROM pg_buffercache WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); count ------- 1659 (1 row) pgbench=# select count(pg_buffercache_invalidate(bufferid)) from pg_buffercache where relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); count ------- 1659 (1 row) pgbench=# SELECT count(*) FROM pg_buffercache WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); count ------- 0 (1 row) Best regards, Palak -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Thomas Munro <thomas.munro@gmail.com> — 2023-06-30T22:09:12Z
On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi <chaturvedipalak1911@gmail.com> wrote: > pgbench=# select count(pg_buffercache_invalidate(bufferid)) from > pg_buffercache where relfilenode = > pg_relation_filenode('pgbench_accounts'::regclass); Hi Palak, Thanks for working on this! I think this will be very useful for testing existing workloads but also for testing future work on prefetching with AIO (and DIO), work on putting SLRUs (or anything else) into the buffer pool, nearby proposals for caching buffer mapping information, etc etc. Palak and I talked about this idea a bit last week (stimulated by a recent thread[1], but the topic has certainly come up before), and we discussed some different ways one could specify which pages are dropped. For example, perhaps the pg_prewarm extension could have an 'unwarm' option instead. I personally thought the buffer ID-based approach was quite good because it's extremely simple, while giving the user the full power of SQL to say which buffers. Half a table? Visibility map? Everything? Root page of an index? I think that's probably better than something that requires more code and complication but is less flexible in the end. It feels like the right level of rawness for something primarily of interest to hackers and advanced users. I don't think it matters that there is a window between selecting a buffer ID and invalidating it, for the intended use cases. That's my vote, anyway, let's see if others have other ideas... We also talked a bit about how one might control the kernel page cache in more fine-grained ways for testing purposes, but it seems like the pgfincore project has that covered with its pgfadvise_willneed() and pgfadvise_dontneed(). IMHO that project could use more page-oriented operations (instead of just counts and coarse grains operations) but that's something that could be material for patches to send to the extension maintainers. This work, in contrast, is more tangled up with bufmgr.c internals, so it feels like this feature belongs in a core contrib module. Some initial thoughts on the patch: I wonder if we should include a simple exercise in contrib/pg_buffercache/sql/pg_buffercache.sql. One problem is that it's not guaranteed to succeed in general. It doesn't wait for pins to go away, and it doesn't retry cleaning dirty buffers after one attempt, it just returns false, which I think is probably the right approach, but it makes the behaviour too non-deterministic for simple tests. Perhaps it's enough to include an exercise where we call it a few times to hit a couple of cases, but not verify what effect it has. It should be restricted by role, but I wonder which role it should be. Testing for superuser is now out of fashion. Where the Makefile mentions 1.4--1.5.sql, the meson.build file needs to do the same. That's because PostgreSQL is currently in transition from autoconf/gmake to meson/ninja[2], so for now we have to maintain both build systems. That's why it fails to build in some CI tasks[3]. You can enable CI in your own GitHub account if you want to run test builds on several operating systems, see [4] for info. [1] https://www.postgresql.org/message-id/flat/CAFSGpE3y_oMK1uHhcHxGxBxs%2BKrjMMdGrE%2B6HHOu0vttVET0UQ%40mail.gmail.com [2] https://wiki.postgresql.org/wiki/Meson [3] http://cfbot.cputube.org/palak-chaturvedi.html [4] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/ci/README;hb=HEAD -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Palak Chaturvedi <chaturvedipalak1911@gmail.com> — 2023-07-03T08:26:29Z
Hi Thomas, Thank you for your suggestions. I have added the sql in the meson build as well. On Sat, 1 Jul 2023 at 03:39, Thomas Munro <thomas.munro@gmail.com> wrote: > > On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi > <chaturvedipalak1911@gmail.com> wrote: > > pgbench=# select count(pg_buffercache_invalidate(bufferid)) from > > pg_buffercache where relfilenode = > > pg_relation_filenode('pgbench_accounts'::regclass); > > Hi Palak, > > Thanks for working on this! I think this will be very useful for > testing existing workloads but also for testing future work on > prefetching with AIO (and DIO), work on putting SLRUs (or anything > else) into the buffer pool, nearby proposals for caching buffer > mapping information, etc etc. > > Palak and I talked about this idea a bit last week (stimulated by a > recent thread[1], but the topic has certainly come up before), and we > discussed some different ways one could specify which pages are > dropped. For example, perhaps the pg_prewarm extension could have an > 'unwarm' option instead. I personally thought the buffer ID-based > approach was quite good because it's extremely simple, while giving > the user the full power of SQL to say which buffers. Half a table? > Visibility map? Everything? Root page of an index? I think that's > probably better than something that requires more code and > complication but is less flexible in the end. It feels like the right > level of rawness for something primarily of interest to hackers and > advanced users. I don't think it matters that there is a window > between selecting a buffer ID and invalidating it, for the intended > use cases. That's my vote, anyway, let's see if others have other > ideas... > > We also talked a bit about how one might control the kernel page cache > in more fine-grained ways for testing purposes, but it seems like the > pgfincore project has that covered with its pgfadvise_willneed() and > pgfadvise_dontneed(). IMHO that project could use more page-oriented > operations (instead of just counts and coarse grains operations) but > that's something that could be material for patches to send to the > extension maintainers. This work, in contrast, is more tangled up > with bufmgr.c internals, so it feels like this feature belongs in a > core contrib module. > > Some initial thoughts on the patch: > > I wonder if we should include a simple exercise in > contrib/pg_buffercache/sql/pg_buffercache.sql. One problem is that > it's not guaranteed to succeed in general. It doesn't wait for pins > to go away, and it doesn't retry cleaning dirty buffers after one > attempt, it just returns false, which I think is probably the right > approach, but it makes the behaviour too non-deterministic for simple > tests. Perhaps it's enough to include an exercise where we call it a > few times to hit a couple of cases, but not verify what effect it has. > > It should be restricted by role, but I wonder which role it should be. > Testing for superuser is now out of fashion. > > Where the Makefile mentions 1.4--1.5.sql, the meson.build file needs > to do the same. That's because PostgreSQL is currently in transition > from autoconf/gmake to meson/ninja[2], so for now we have to maintain > both build systems. That's why it fails to build in some CI tasks[3]. > You can enable CI in your own GitHub account if you want to run test > builds on several operating systems, see [4] for info. > > [1] https://www.postgresql.org/message-id/flat/CAFSGpE3y_oMK1uHhcHxGxBxs%2BKrjMMdGrE%2B6HHOu0vttVET0UQ%40mail.gmail.com > [2] https://wiki.postgresql.org/wiki/Meson > [3] http://cfbot.cputube.org/palak-chaturvedi.html > [4] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/ci/README;hb=HEAD -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
jian he <jian.universality@gmail.com> — 2023-07-03T15:46:26Z
On Mon, Jul 3, 2023 at 4:26 PM Palak Chaturvedi <chaturvedipalak1911@gmail.com> wrote: > > Hi Thomas, > Thank you for your suggestions. I have added the sql in the meson > build as well. > > On Sat, 1 Jul 2023 at 03:39, Thomas Munro <thomas.munro@gmail.com> wrote: > > > > On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi > > <chaturvedipalak1911@gmail.com> wrote: > > > pgbench=# select count(pg_buffercache_invalidate(bufferid)) from > > > pg_buffercache where relfilenode = > > > pg_relation_filenode('pgbench_accounts'::regclass); > > > > Hi Palak, > > > > Thanks for working on this! I think this will be very useful for > > testing existing workloads but also for testing future work on > > prefetching with AIO (and DIO), work on putting SLRUs (or anything > > else) into the buffer pool, nearby proposals for caching buffer > > mapping information, etc etc. > > > > Palak and I talked about this idea a bit last week (stimulated by a > > recent thread[1], but the topic has certainly come up before), and we > > discussed some different ways one could specify which pages are > > dropped. For example, perhaps the pg_prewarm extension could have an > > 'unwarm' option instead. I personally thought the buffer ID-based > > approach was quite good because it's extremely simple, while giving > > the user the full power of SQL to say which buffers. Half a table? > > Visibility map? Everything? Root page of an index? I think that's > > probably better than something that requires more code and > > complication but is less flexible in the end. It feels like the right > > level of rawness for something primarily of interest to hackers and > > advanced users. I don't think it matters that there is a window > > between selecting a buffer ID and invalidating it, for the intended > > use cases. That's my vote, anyway, let's see if others have other > > ideas... > > > > We also talked a bit about how one might control the kernel page cache > > in more fine-grained ways for testing purposes, but it seems like the > > pgfincore project has that covered with its pgfadvise_willneed() and > > pgfadvise_dontneed(). IMHO that project could use more page-oriented > > operations (instead of just counts and coarse grains operations) but > > that's something that could be material for patches to send to the > > extension maintainers. This work, in contrast, is more tangled up > > with bufmgr.c internals, so it feels like this feature belongs in a > > core contrib module. > > > > Some initial thoughts on the patch: > > > > I wonder if we should include a simple exercise in > > contrib/pg_buffercache/sql/pg_buffercache.sql. One problem is that > > it's not guaranteed to succeed in general. It doesn't wait for pins > > to go away, and it doesn't retry cleaning dirty buffers after one > > attempt, it just returns false, which I think is probably the right > > approach, but it makes the behaviour too non-deterministic for simple > > tests. Perhaps it's enough to include an exercise where we call it a > > few times to hit a couple of cases, but not verify what effect it has. > > > > It should be restricted by role, but I wonder which role it should be. > > Testing for superuser is now out of fashion. > > > > Where the Makefile mentions 1.4--1.5.sql, the meson.build file needs > > to do the same. That's because PostgreSQL is currently in transition > > from autoconf/gmake to meson/ninja[2], so for now we have to maintain > > both build systems. That's why it fails to build in some CI tasks[3]. > > You can enable CI in your own GitHub account if you want to run test > > builds on several operating systems, see [4] for info. > > > > [1] https://www.postgresql.org/message-id/flat/CAFSGpE3y_oMK1uHhcHxGxBxs%2BKrjMMdGrE%2B6HHOu0vttVET0UQ%40mail.gmail.com > > [2] https://wiki.postgresql.org/wiki/Meson > > [3] http://cfbot.cputube.org/palak-chaturvedi.html > > [4] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/ci/README;hb=HEAD newbie question: quote from: https://www.interdb.jp/pg/pgsql08.html > > Pinned: When the corresponding buffer pool slot stores a page and any PostgreSQL processes are accessing the page (i.e. refcount and usage_count are greater than or equal to 1), the state of this buffer descriptor is pinned. > Unpinned: When the corresponding buffer pool slot stores a page but no PostgreSQL processes are accessing the page (i.e. usage_count is greater than or equal to 1, but refcount is 0), the state of this buffer descriptor is unpinned. So do you need to check BUF_STATE_GET_REFCOUNT(buf_state) and BUF_STATE_GET_USAGECOUNT(state)? -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Palak Chaturvedi <chaturvedipalak1911@gmail.com> — 2023-07-04T06:08:04Z
hi, I don't think we need to check the usage count. Because we are clearing all the buffers that are not pinned. Checking the usage count is for buffer replacement since we are not replacing it does not matter. On Mon, 3 Jul 2023 at 21:16, jian he <jian.universality@gmail.com> wrote: > > On Mon, Jul 3, 2023 at 4:26 PM Palak Chaturvedi > <chaturvedipalak1911@gmail.com> wrote: > > > > Hi Thomas, > > Thank you for your suggestions. I have added the sql in the meson > > build as well. > > > > On Sat, 1 Jul 2023 at 03:39, Thomas Munro <thomas.munro@gmail.com> wrote: > > > > > > On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi > > > <chaturvedipalak1911@gmail.com> wrote: > > > > pgbench=# select count(pg_buffercache_invalidate(bufferid)) from > > > > pg_buffercache where relfilenode = > > > > pg_relation_filenode('pgbench_accounts'::regclass); > > > > > > Hi Palak, > > > > > > Thanks for working on this! I think this will be very useful for > > > testing existing workloads but also for testing future work on > > > prefetching with AIO (and DIO), work on putting SLRUs (or anything > > > else) into the buffer pool, nearby proposals for caching buffer > > > mapping information, etc etc. > > > > > > Palak and I talked about this idea a bit last week (stimulated by a > > > recent thread[1], but the topic has certainly come up before), and we > > > discussed some different ways one could specify which pages are > > > dropped. For example, perhaps the pg_prewarm extension could have an > > > 'unwarm' option instead. I personally thought the buffer ID-based > > > approach was quite good because it's extremely simple, while giving > > > the user the full power of SQL to say which buffers. Half a table? > > > Visibility map? Everything? Root page of an index? I think that's > > > probably better than something that requires more code and > > > complication but is less flexible in the end. It feels like the right > > > level of rawness for something primarily of interest to hackers and > > > advanced users. I don't think it matters that there is a window > > > between selecting a buffer ID and invalidating it, for the intended > > > use cases. That's my vote, anyway, let's see if others have other > > > ideas... > > > > > > We also talked a bit about how one might control the kernel page cache > > > in more fine-grained ways for testing purposes, but it seems like the > > > pgfincore project has that covered with its pgfadvise_willneed() and > > > pgfadvise_dontneed(). IMHO that project could use more page-oriented > > > operations (instead of just counts and coarse grains operations) but > > > that's something that could be material for patches to send to the > > > extension maintainers. This work, in contrast, is more tangled up > > > with bufmgr.c internals, so it feels like this feature belongs in a > > > core contrib module. > > > > > > Some initial thoughts on the patch: > > > > > > I wonder if we should include a simple exercise in > > > contrib/pg_buffercache/sql/pg_buffercache.sql. One problem is that > > > it's not guaranteed to succeed in general. It doesn't wait for pins > > > to go away, and it doesn't retry cleaning dirty buffers after one > > > attempt, it just returns false, which I think is probably the right > > > approach, but it makes the behaviour too non-deterministic for simple > > > tests. Perhaps it's enough to include an exercise where we call it a > > > few times to hit a couple of cases, but not verify what effect it has. > > > > > > It should be restricted by role, but I wonder which role it should be. > > > Testing for superuser is now out of fashion. > > > > > > Where the Makefile mentions 1.4--1.5.sql, the meson.build file needs > > > to do the same. That's because PostgreSQL is currently in transition > > > from autoconf/gmake to meson/ninja[2], so for now we have to maintain > > > both build systems. That's why it fails to build in some CI tasks[3]. > > > You can enable CI in your own GitHub account if you want to run test > > > builds on several operating systems, see [4] for info. > > > > > > [1] https://www.postgresql.org/message-id/flat/CAFSGpE3y_oMK1uHhcHxGxBxs%2BKrjMMdGrE%2B6HHOu0vttVET0UQ%40mail.gmail.com > > > [2] https://wiki.postgresql.org/wiki/Meson > > > [3] http://cfbot.cputube.org/palak-chaturvedi.html > > > [4] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/ci/README;hb=HEAD > > newbie question: > quote from: https://www.interdb.jp/pg/pgsql08.html > > > > Pinned: When the corresponding buffer pool slot stores a page and any PostgreSQL processes are accessing the page (i.e. refcount and usage_count are greater than or equal to 1), the state of this buffer descriptor is pinned. > > Unpinned: When the corresponding buffer pool slot stores a page but no PostgreSQL processes are accessing the page (i.e. usage_count is greater than or equal to 1, but refcount is 0), the state of this buffer descriptor is unpinned. > > > So do you need to check BUF_STATE_GET_REFCOUNT(buf_state) and > BUF_STATE_GET_USAGECOUNT(state)? -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Japin Li <japinli@hotmail.com> — 2023-07-04T08:50:33Z
On Mon, 03 Jul 2023 at 16:26, Palak Chaturvedi <chaturvedipalak1911@gmail.com> wrote: > Hi Thomas, > Thank you for your suggestions. I have added the sql in the meson > build as well. > > On Sat, 1 Jul 2023 at 03:39, Thomas Munro <thomas.munro@gmail.com> wrote: >> >> On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi >> <chaturvedipalak1911@gmail.com> wrote: >> > pgbench=# select count(pg_buffercache_invalidate(bufferid)) from >> > pg_buffercache where relfilenode = >> > pg_relation_filenode('pgbench_accounts'::regclass); >> >> Hi Palak, >> >> Thanks for working on this! I think this will be very useful for >> testing existing workloads but also for testing future work on >> prefetching with AIO (and DIO), work on putting SLRUs (or anything >> else) into the buffer pool, nearby proposals for caching buffer >> mapping information, etc etc. >> >> Palak and I talked about this idea a bit last week (stimulated by a >> recent thread[1], but the topic has certainly come up before), and we >> discussed some different ways one could specify which pages are >> dropped. For example, perhaps the pg_prewarm extension could have an >> 'unwarm' option instead. I personally thought the buffer ID-based >> approach was quite good because it's extremely simple, while giving >> the user the full power of SQL to say which buffers. Half a table? >> Visibility map? Everything? Root page of an index? I think that's >> probably better than something that requires more code and >> complication but is less flexible in the end. It feels like the right >> level of rawness for something primarily of interest to hackers and >> advanced users. I don't think it matters that there is a window >> between selecting a buffer ID and invalidating it, for the intended >> use cases. That's my vote, anyway, let's see if others have other >> ideas... >> >> We also talked a bit about how one might control the kernel page cache >> in more fine-grained ways for testing purposes, but it seems like the >> pgfincore project has that covered with its pgfadvise_willneed() and >> pgfadvise_dontneed(). IMHO that project could use more page-oriented >> operations (instead of just counts and coarse grains operations) but >> that's something that could be material for patches to send to the >> extension maintainers. This work, in contrast, is more tangled up >> with bufmgr.c internals, so it feels like this feature belongs in a >> core contrib module. >> >> Some initial thoughts on the patch: >> >> I wonder if we should include a simple exercise in >> contrib/pg_buffercache/sql/pg_buffercache.sql. One problem is that >> it's not guaranteed to succeed in general. It doesn't wait for pins >> to go away, and it doesn't retry cleaning dirty buffers after one >> attempt, it just returns false, which I think is probably the right >> approach, but it makes the behaviour too non-deterministic for simple >> tests. Perhaps it's enough to include an exercise where we call it a >> few times to hit a couple of cases, but not verify what effect it has. >> >> It should be restricted by role, but I wonder which role it should be. >> Testing for superuser is now out of fashion. >> >> Where the Makefile mentions 1.4--1.5.sql, the meson.build file needs >> to do the same. That's because PostgreSQL is currently in transition >> from autoconf/gmake to meson/ninja[2], so for now we have to maintain >> both build systems. That's why it fails to build in some CI tasks[3]. >> You can enable CI in your own GitHub account if you want to run test >> builds on several operating systems, see [4] for info. >> >> [1] https://www.postgresql.org/message-id/flat/CAFSGpE3y_oMK1uHhcHxGxBxs%2BKrjMMdGrE%2B6HHOu0vttVET0UQ%40mail.gmail.com >> [2] https://wiki.postgresql.org/wiki/Meson >> [3] http://cfbot.cputube.org/palak-chaturvedi.html >> [4] https://git.postgresql.org/gitweb/?p=postgresql.git;a=blob_plain;f=src/tools/ci/README;hb=HEAD I think, zero is not a valid buffer identifier. See src/include/storage/buf.h. + bufnum = PG_GETARG_INT32(0); + if (bufnum < 0 || bufnum > NBuffers) + { + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("buffernum is not valid"))); + + } If we use SELECT pg_buffercache_invalidate(0), it will crash. -- Regrads, Japin Li. -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
jian he <jian.universality@gmail.com> — 2023-07-04T09:00:00Z
the following will also crash. no idea why. begin; select count(*) from onek; select relpages from pg_class where relname = 'onek'; --queryA SELECT count(*) FROM pg_buffercache WHERE relfilenode = pg_relation_filenode('onek'::regclass); --queryB insert into onek values(default); select count(pg_buffercache_invalidate(bufferid)) from pg_buffercache where relfilenode = pg_relation_filenode('onek'::regclass); --------------------------------- queryA returns 35, queryB returns 37. ---------------------------------- crash info: test_dev=*# insert into onek values(default); INSERT 0 1 test_dev=*# select count(pg_buffercache_invalidate(bufferid)) from pg_buffercache where relfilenode = pg_relation_filenode('onek'::regclass); TRAP: failed Assert("resarr->nitems < resarr->maxitems"), File: "../../Desktop/pg_sources/main/postgres/src/backend/utils/resowner/resowner.c", Line: 275, PID: 1533312 postgres: jian test_dev [local] SELECT(ExceptionalCondition+0xa1)[0x55fc8f8d14e1] postgres: jian test_dev [local] SELECT(+0x9e7ab3)[0x55fc8f915ab3] postgres: jian test_dev [local] SELECT(ResourceOwnerRememberBuffer+0x1d)[0x55fc8f91696d] postgres: jian test_dev [local] SELECT(+0x78ab17)[0x55fc8f6b8b17] postgres: jian test_dev [local] SELECT(TryInvalidateBuffer+0x6d)[0x55fc8f6c507d] /home/jian/postgres/pg16_test/lib/pg_buffercache.so(pg_buffercache_invalidate+0x3d)[0x7f2361837abd] postgres: jian test_dev [local] SELECT(+0x57eebc)[0x55fc8f4acebc] postgres: jian test_dev [local] SELECT(ExecInterpExprStillValid+0x3c)[0x55fc8f4a6e2c] postgres: jian test_dev [local] SELECT(+0x5a0f16)[0x55fc8f4cef16] postgres: jian test_dev [local] SELECT(+0x5a3588)[0x55fc8f4d1588] postgres: jian test_dev [local] SELECT(+0x58f747)[0x55fc8f4bd747] postgres: jian test_dev [local] SELECT(standard_ExecutorRun+0x1f0)[0x55fc8f4b29f0] postgres: jian test_dev [local] SELECT(ExecutorRun+0x46)[0x55fc8f4b2d16] postgres: jian test_dev [local] SELECT(+0x7eb3b0)[0x55fc8f7193b0] postgres: jian test_dev [local] SELECT(PortalRun+0x1eb)[0x55fc8f71b7ab] postgres: jian test_dev [local] SELECT(+0x7e8cf4)[0x55fc8f716cf4] postgres: jian test_dev [local] SELECT(PostgresMain+0x134f)[0x55fc8f71869f] postgres: jian test_dev [local] SELECT(+0x70f80c)[0x55fc8f63d80c] postgres: jian test_dev [local] SELECT(PostmasterMain+0x1758)[0x55fc8f63f278] postgres: jian test_dev [local] SELECT(main+0x27e)[0x55fc8f27067e] /lib/x86_64-linux-gnu/libc.so.6(+0x29d90)[0x7f2361629d90] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80)[0x7f2361629e40] postgres: jian test_dev [local] SELECT(_start+0x25)[0x55fc8f272bb5] 2023-07-04 16:56:13.088 CST [1532822] LOG: server process (PID 1533312) was terminated by signal 6: Aborted 2023-07-04 16:56:13.088 CST [1532822] DETAIL: Failed process was running: select count(pg_buffercache_invalidate(bufferid)) from pg_buffercache where relfilenode = pg_relation_filenode('onek'::regclass); 2023-07-04 16:56:13.088 CST [1532822] LOG: terminating any other active server processes server closed the connection unexpectedly This probably means the server terminated abnormally before or while processing the request. The connection to the server was lost. Attempting reset: 2023-07-04 16:56:13.091 CST [1533381] FATAL: the database system is in recovery mode Failed. The connection to the server was lost. Attempting reset: Failed. -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Japin Li <japinli@hotmail.com> — 2023-07-04T09:45:47Z
On Tue, 04 Jul 2023 at 17:00, jian he <jian.universality@gmail.com> wrote: > the following will also crash. no idea why. > begin; > select count(*) from onek; > select relpages from pg_class where relname = 'onek'; --queryA > > SELECT count(*) FROM pg_buffercache WHERE relfilenode = > pg_relation_filenode('onek'::regclass); --queryB > > insert into onek values(default); > > select count(pg_buffercache_invalidate(bufferid)) from > pg_buffercache where relfilenode = > pg_relation_filenode('onek'::regclass); > > --------------------------------- > queryA returns 35, queryB returns 37. > ---------------------------------- > crash info: > test_dev=*# insert into onek values(default); > INSERT 0 1 > test_dev=*# select count(pg_buffercache_invalidate(bufferid)) from > pg_buffercache where relfilenode = > pg_relation_filenode('onek'::regclass); > TRAP: failed Assert("resarr->nitems < resarr->maxitems"), File: > "../../Desktop/pg_sources/main/postgres/src/backend/utils/resowner/resowner.c", > Line: 275, PID: 1533312 According to the comments of ResourceArrayAdd(), the caller must have previously done ResourceArrayEnlarge(). I tried to call ResourceOwnerEnlargeBuffers() before PinBuffer_Locked(), so it can avoid this crash. if ((buf_state & BM_DIRTY) == BM_DIRTY) { + /* make sure we can handle the pin */ + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); + /* * Try once to flush the dirty buffer. */ PinBuffer_Locked(bufHdr); -- Regrads, Japin Li. -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
jian he <jian.universality@gmail.com> — 2023-07-04T11:53:39Z
On Tue, Jul 4, 2023 at 5:45 PM Japin Li <japinli@hotmail.com> wrote: > > On Tue, 04 Jul 2023 at 17:00, jian he <jian.universality@gmail.com> wrote: > > the following will also crash. no idea why. > > begin; > > select count(*) from onek; > > select relpages from pg_class where relname = 'onek'; --queryA > > > > SELECT count(*) FROM pg_buffercache WHERE relfilenode = > > pg_relation_filenode('onek'::regclass); --queryB > > > > insert into onek values(default); > > > > select count(pg_buffercache_invalidate(bufferid)) from > > pg_buffercache where relfilenode = > > pg_relation_filenode('onek'::regclass); > > > > --------------------------------- > > queryA returns 35, queryB returns 37. > > ---------------------------------- > > crash info: > > test_dev=*# insert into onek values(default); > > INSERT 0 1 > > test_dev=*# select count(pg_buffercache_invalidate(bufferid)) from > > pg_buffercache where relfilenode = > > pg_relation_filenode('onek'::regclass); > > TRAP: failed Assert("resarr->nitems < resarr->maxitems"), File: > > > "../../Desktop/pg_sources/main/postgres/src/backend/utils/resowner/resowner.c", > > Line: 275, PID: 1533312 > > According to the comments of ResourceArrayAdd(), the caller must have > previously > done ResourceArrayEnlarge(). I tried to call ResourceOwnerEnlargeBuffers() > before > PinBuffer_Locked(), so it can avoid this crash. > > if ((buf_state & BM_DIRTY) == BM_DIRTY) > { > + /* make sure we can handle the pin */ > + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); > + > /* > * Try once to flush the dirty buffer. > */ > PinBuffer_Locked(bufHdr); > > -- > Regrads, > Japin Li. > thanks. tested flush pg_catalog, public schema, now, both works as pitched. -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
jian he <jian.universality@gmail.com> — 2023-07-05T01:14:59Z
On Sat, Jul 1, 2023 at 6:09 AM Thomas Munro <thomas.munro@gmail.com> wrote: > > > It should be restricted by role, but I wonder which role it should be. > Testing for superuser is now out of fashion. > as pg_buffercache/pg_buffercache--1.2--1.3.sql. You need pg_maintain privilege to use pg_buffercache. The following query works on a single user. Obviously you need a role who can gain pg_monitor privilege. begin; create role test login nosuperuser; grant select, insert on onek to test; grant pg_monitor to test; set role test; select count(*) from onek; insert into onek values(default); (SELECT count(*) FROM pg_buffercache WHERE relfilenode = pg_relation_filenode('onek'::regclass)) except ( select count(pg_buffercache_invalidate(bufferid)) from pg_buffercache where relfilenode = pg_relation_filenode('onek'::regclass) ); rollback; -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Nitin Jadhav <nitinjadhavpostgres@gmail.com> — 2023-07-05T12:23:04Z
+1 for the idea. It's going to be more useful to test and understand the buffer management of PostgreSQL and it can be used to explicitly free up the buffers if there are any such requirements. I had a quick look over the patch. Following are the comments. First, The TryInvalidateBuffer() tries to flush the buffer if it is dirty and then tries to invalidate it if it meets the requirement. Instead of directly doing this can we provide an option to the caller to mention whether to invalidate the dirty buffers or not. For example, TryInvalidateBuffer(Buffer bufnum, bool force), if the force is set to FALSE, then ignore invalidating dirty buffers. Otherwise, flush the dirty buffer and try to invalidate. Second, In TryInvalidateBuffer(), it first checks if the reference count is greater than zero and then checks for dirty buffers. Will there be a scenario where the buffer is dirty and its reference count is zero? Can you please provide more information on this or adjust the code accordingly. > +/* > +Try Invalidating a buffer using bufnum. > +If the buffer is invalid, the function returns false. > +The function checks for dirty buffer and flushes the dirty buffer before invalidating. > +If the buffer is still dirty it returns false. > +*/ > +bool The star(*) and space are missing here. Please refer to the style of function comments and change accordingly. Thanks & Regards, Nitin Jadhav On Fri, Jun 30, 2023 at 4:17 PM Palak Chaturvedi <chaturvedipalak1911@gmail.com> wrote: > > I hope this email finds you well. I am excited to share that I have > extended the functionality of the `pg_buffercache` extension by > implementing buffer invalidation capability, as requested by some > PostgreSQL contributors for improved testing scenarios. > > This marks my first time submitting a patch to pgsql-hackers, and I am > eager to receive your expert feedback on the changes made. Your > insights are invaluable, and any review or comments you provide will > be greatly appreciated. > > The primary objective of this enhancement is to enable explicit buffer > invalidation within the `pg_buffercache` extension. By doing so, we > can simulate scenarios where buffers are invalidated and observe the > resulting behavior in PostgreSQL. > > As part of this patch, a new function or mechanism has been introduced > to facilitate buffer invalidation. I would like to hear your thoughts > on whether this approach provides a good user interface for this > functionality. Additionally, I seek your evaluation of the buffer > locking protocol employed in the extension to ensure its correctness > and efficiency. > > Please note that I plan to add comprehensive documentation once the > details of this enhancement are agreed upon. This documentation will > serve as a valuable resource for users and contributors alike. I > believe that your expertise will help uncover any potential issues and > opportunities for further improvement. > > I have attached the patch file to this email for your convenience. > Your valuable time and consideration in reviewing this extension are > sincerely appreciated. > > Thank you for your continued support and guidance. I am looking > forward to your feedback and collaboration in enhancing the PostgreSQL > ecosystem. > > The working of the extension: > > 1. Creating the extension pg_buffercache and then call select query on > a table and note the buffer to be cleared. > pgbench=# create extension pg_buffercache; > CREATE EXTENSION > pgbench=# select count(*) from pgbench_accounts; > count > -------- > 100000 > (1 row) > > pgbench=# SELECT * > FROM pg_buffercache > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > bufferid | relfilenode | reltablespace | reldatabase | relforknumber > | relblocknumber | isdirty | usagecount | pinning_backends > ----------+-------------+---------------+-------------+---------------+----------------+---------+------------+------------------ > 233 | 16397 | 1663 | 16384 | 0 > | 0 | f | 1 | 0 > 234 | 16397 | 1663 | 16384 | 0 > | 1 | f | 1 | 0 > 235 | 16397 | 1663 | 16384 | 0 > | 2 | f | 1 | 0 > 236 | 16397 | 1663 | 16384 | 0 > | 3 | f | 1 | 0 > 237 | 16397 | 1663 | 16384 | 0 > | 4 | f | 1 | 0 > > > 2. Clearing a single buffer by entering the bufferid. > pgbench=# SELECT count(*) > FROM pg_buffercache > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > count > ------- > 1660 > (1 row) > > pgbench=# select pg_buffercache_invalidate(233); > pg_buffercache_invalidate > --------------------------- > t > (1 row) > > pgbench=# SELECT count(*) > FROM pg_buffercache > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > count > ------- > 1659 > (1 row) > > 3. Clearing the entire buffer for a relation using the function. > pgbench=# SELECT count(*) > FROM pg_buffercache > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > count > ------- > 1659 > (1 row) > > pgbench=# select count(pg_buffercache_invalidate(bufferid)) from > pg_buffercache where relfilenode = > pg_relation_filenode('pgbench_accounts'::regclass); > count > ------- > 1659 > (1 row) > > pgbench=# SELECT count(*) > FROM pg_buffercache > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > count > ------- > 0 > (1 row) > > > Best regards, > Palak -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Palak Chaturvedi <chaturvedipalak1911@gmail.com> — 2023-07-11T12:38:56Z
Hey Nitin, >Will >there be a scenario where the buffer is dirty and its reference count >is zero? There might be a buffer that has been dirtied but is not pinned or being used currently by a process. So checking the refcount and then dirty buffers helps. >First, The TryInvalidateBuffer() tries to flush the buffer if it is dirty and then tries to invalidate it if it meets the requirement. Instead of directly doing this can we provide an option to the caller to mention whether to invalidate the dirty buffers or not. Yes that can be implemented with a default value of force. Will implement it in the next patch. On Wed, 5 Jul 2023 at 17:53, Nitin Jadhav <nitinjadhavpostgres@gmail.com> wrote: > > +1 for the idea. It's going to be more useful to test and understand > the buffer management of PostgreSQL and it can be used to explicitly > free up the buffers if there are any such requirements. > > I had a quick look over the patch. Following are the comments. > > First, The TryInvalidateBuffer() tries to flush the buffer if it is > dirty and then tries to invalidate it if it meets the requirement. > Instead of directly doing this can we provide an option to the caller > to mention whether to invalidate the dirty buffers or not. For > example, TryInvalidateBuffer(Buffer bufnum, bool force), if the force > is set to FALSE, then ignore invalidating dirty buffers. Otherwise, > flush the dirty buffer and try to invalidate. > > Second, In TryInvalidateBuffer(), it first checks if the reference > count is greater than zero and then checks for dirty buffers. Will > there be a scenario where the buffer is dirty and its reference count > is zero? Can you please provide more information on this or adjust the > code accordingly. > > > +/* > > +Try Invalidating a buffer using bufnum. > > +If the buffer is invalid, the function returns false. > > +The function checks for dirty buffer and flushes the dirty buffer before invalidating. > > +If the buffer is still dirty it returns false. > > +*/ > > +bool > > The star(*) and space are missing here. Please refer to the style of > function comments and change accordingly. > > Thanks & Regards, > Nitin Jadhav > > On Fri, Jun 30, 2023 at 4:17 PM Palak Chaturvedi > <chaturvedipalak1911@gmail.com> wrote: > > > > I hope this email finds you well. I am excited to share that I have > > extended the functionality of the `pg_buffercache` extension by > > implementing buffer invalidation capability, as requested by some > > PostgreSQL contributors for improved testing scenarios. > > > > This marks my first time submitting a patch to pgsql-hackers, and I am > > eager to receive your expert feedback on the changes made. Your > > insights are invaluable, and any review or comments you provide will > > be greatly appreciated. > > > > The primary objective of this enhancement is to enable explicit buffer > > invalidation within the `pg_buffercache` extension. By doing so, we > > can simulate scenarios where buffers are invalidated and observe the > > resulting behavior in PostgreSQL. > > > > As part of this patch, a new function or mechanism has been introduced > > to facilitate buffer invalidation. I would like to hear your thoughts > > on whether this approach provides a good user interface for this > > functionality. Additionally, I seek your evaluation of the buffer > > locking protocol employed in the extension to ensure its correctness > > and efficiency. > > > > Please note that I plan to add comprehensive documentation once the > > details of this enhancement are agreed upon. This documentation will > > serve as a valuable resource for users and contributors alike. I > > believe that your expertise will help uncover any potential issues and > > opportunities for further improvement. > > > > I have attached the patch file to this email for your convenience. > > Your valuable time and consideration in reviewing this extension are > > sincerely appreciated. > > > > Thank you for your continued support and guidance. I am looking > > forward to your feedback and collaboration in enhancing the PostgreSQL > > ecosystem. > > > > The working of the extension: > > > > 1. Creating the extension pg_buffercache and then call select query on > > a table and note the buffer to be cleared. > > pgbench=# create extension pg_buffercache; > > CREATE EXTENSION > > pgbench=# select count(*) from pgbench_accounts; > > count > > -------- > > 100000 > > (1 row) > > > > pgbench=# SELECT * > > FROM pg_buffercache > > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > > bufferid | relfilenode | reltablespace | reldatabase | relforknumber > > | relblocknumber | isdirty | usagecount | pinning_backends > > ----------+-------------+---------------+-------------+---------------+----------------+---------+------------+------------------ > > 233 | 16397 | 1663 | 16384 | 0 > > | 0 | f | 1 | 0 > > 234 | 16397 | 1663 | 16384 | 0 > > | 1 | f | 1 | 0 > > 235 | 16397 | 1663 | 16384 | 0 > > | 2 | f | 1 | 0 > > 236 | 16397 | 1663 | 16384 | 0 > > | 3 | f | 1 | 0 > > 237 | 16397 | 1663 | 16384 | 0 > > | 4 | f | 1 | 0 > > > > > > 2. Clearing a single buffer by entering the bufferid. > > pgbench=# SELECT count(*) > > FROM pg_buffercache > > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > > count > > ------- > > 1660 > > (1 row) > > > > pgbench=# select pg_buffercache_invalidate(233); > > pg_buffercache_invalidate > > --------------------------- > > t > > (1 row) > > > > pgbench=# SELECT count(*) > > FROM pg_buffercache > > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > > count > > ------- > > 1659 > > (1 row) > > > > 3. Clearing the entire buffer for a relation using the function. > > pgbench=# SELECT count(*) > > FROM pg_buffercache > > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > > count > > ------- > > 1659 > > (1 row) > > > > pgbench=# select count(pg_buffercache_invalidate(bufferid)) from > > pg_buffercache where relfilenode = > > pg_relation_filenode('pgbench_accounts'::regclass); > > count > > ------- > > 1659 > > (1 row) > > > > pgbench=# SELECT count(*) > > FROM pg_buffercache > > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > > count > > ------- > > 0 > > (1 row) > > > > > > Best regards, > > Palak -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Palak Chaturvedi <chaturvedipalak1911@gmail.com> — 2023-07-11T13:09:36Z
Can you please review the new patch of the extension with implemented force variable. On Tue, 11 Jul 2023 at 18:08, Palak Chaturvedi <chaturvedipalak1911@gmail.com> wrote: > > Hey Nitin, > >Will > >there be a scenario where the buffer is dirty and its reference count > >is zero? > There might be a buffer that has been dirtied but is not pinned or > being used currently by a process. So checking the refcount and then > dirty buffers helps. > >First, The TryInvalidateBuffer() tries to flush the buffer if it is > dirty and then tries to invalidate it if it meets the requirement. > Instead of directly doing this can we provide an option to the caller > to mention whether to invalidate the dirty buffers or not. > Yes that can be implemented with a default value of force. Will > implement it in the next patch. > > On Wed, 5 Jul 2023 at 17:53, Nitin Jadhav <nitinjadhavpostgres@gmail.com> wrote: > > > > +1 for the idea. It's going to be more useful to test and understand > > the buffer management of PostgreSQL and it can be used to explicitly > > free up the buffers if there are any such requirements. > > > > I had a quick look over the patch. Following are the comments. > > > > First, The TryInvalidateBuffer() tries to flush the buffer if it is > > dirty and then tries to invalidate it if it meets the requirement. > > Instead of directly doing this can we provide an option to the caller > > to mention whether to invalidate the dirty buffers or not. For > > example, TryInvalidateBuffer(Buffer bufnum, bool force), if the force > > is set to FALSE, then ignore invalidating dirty buffers. Otherwise, > > flush the dirty buffer and try to invalidate. > > > > Second, In TryInvalidateBuffer(), it first checks if the reference > > count is greater than zero and then checks for dirty buffers. Will > > there be a scenario where the buffer is dirty and its reference count > > is zero? Can you please provide more information on this or adjust the > > code accordingly. > > > > > +/* > > > +Try Invalidating a buffer using bufnum. > > > +If the buffer is invalid, the function returns false. > > > +The function checks for dirty buffer and flushes the dirty buffer before invalidating. > > > +If the buffer is still dirty it returns false. > > > +*/ > > > +bool > > > > The star(*) and space are missing here. Please refer to the style of > > function comments and change accordingly. > > > > Thanks & Regards, > > Nitin Jadhav > > > > On Fri, Jun 30, 2023 at 4:17 PM Palak Chaturvedi > > <chaturvedipalak1911@gmail.com> wrote: > > > > > > I hope this email finds you well. I am excited to share that I have > > > extended the functionality of the `pg_buffercache` extension by > > > implementing buffer invalidation capability, as requested by some > > > PostgreSQL contributors for improved testing scenarios. > > > > > > This marks my first time submitting a patch to pgsql-hackers, and I am > > > eager to receive your expert feedback on the changes made. Your > > > insights are invaluable, and any review or comments you provide will > > > be greatly appreciated. > > > > > > The primary objective of this enhancement is to enable explicit buffer > > > invalidation within the `pg_buffercache` extension. By doing so, we > > > can simulate scenarios where buffers are invalidated and observe the > > > resulting behavior in PostgreSQL. > > > > > > As part of this patch, a new function or mechanism has been introduced > > > to facilitate buffer invalidation. I would like to hear your thoughts > > > on whether this approach provides a good user interface for this > > > functionality. Additionally, I seek your evaluation of the buffer > > > locking protocol employed in the extension to ensure its correctness > > > and efficiency. > > > > > > Please note that I plan to add comprehensive documentation once the > > > details of this enhancement are agreed upon. This documentation will > > > serve as a valuable resource for users and contributors alike. I > > > believe that your expertise will help uncover any potential issues and > > > opportunities for further improvement. > > > > > > I have attached the patch file to this email for your convenience. > > > Your valuable time and consideration in reviewing this extension are > > > sincerely appreciated. > > > > > > Thank you for your continued support and guidance. I am looking > > > forward to your feedback and collaboration in enhancing the PostgreSQL > > > ecosystem. > > > > > > The working of the extension: > > > > > > 1. Creating the extension pg_buffercache and then call select query on > > > a table and note the buffer to be cleared. > > > pgbench=# create extension pg_buffercache; > > > CREATE EXTENSION > > > pgbench=# select count(*) from pgbench_accounts; > > > count > > > -------- > > > 100000 > > > (1 row) > > > > > > pgbench=# SELECT * > > > FROM pg_buffercache > > > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > > > bufferid | relfilenode | reltablespace | reldatabase | relforknumber > > > | relblocknumber | isdirty | usagecount | pinning_backends > > > ----------+-------------+---------------+-------------+---------------+----------------+---------+------------+------------------ > > > 233 | 16397 | 1663 | 16384 | 0 > > > | 0 | f | 1 | 0 > > > 234 | 16397 | 1663 | 16384 | 0 > > > | 1 | f | 1 | 0 > > > 235 | 16397 | 1663 | 16384 | 0 > > > | 2 | f | 1 | 0 > > > 236 | 16397 | 1663 | 16384 | 0 > > > | 3 | f | 1 | 0 > > > 237 | 16397 | 1663 | 16384 | 0 > > > | 4 | f | 1 | 0 > > > > > > > > > 2. Clearing a single buffer by entering the bufferid. > > > pgbench=# SELECT count(*) > > > FROM pg_buffercache > > > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > > > count > > > ------- > > > 1660 > > > (1 row) > > > > > > pgbench=# select pg_buffercache_invalidate(233); > > > pg_buffercache_invalidate > > > --------------------------- > > > t > > > (1 row) > > > > > > pgbench=# SELECT count(*) > > > FROM pg_buffercache > > > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > > > count > > > ------- > > > 1659 > > > (1 row) > > > > > > 3. Clearing the entire buffer for a relation using the function. > > > pgbench=# SELECT count(*) > > > FROM pg_buffercache > > > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > > > count > > > ------- > > > 1659 > > > (1 row) > > > > > > pgbench=# select count(pg_buffercache_invalidate(bufferid)) from > > > pg_buffercache where relfilenode = > > > pg_relation_filenode('pgbench_accounts'::regclass); > > > count > > > ------- > > > 1659 > > > (1 row) > > > > > > pgbench=# SELECT count(*) > > > FROM pg_buffercache > > > WHERE relfilenode = pg_relation_filenode('pgbench_accounts'::regclass); > > > count > > > ------- > > > 0 > > > (1 row) > > > > > > > > > Best regards, > > > Palak -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Andres Freund <andres@anarazel.de> — 2023-07-19T00:45:51Z
Hi, I wanted this feature a couple times before... On 2023-07-03 13:56:29 +0530, Palak Chaturvedi wrote: > +PG_FUNCTION_INFO_V1(pg_buffercache_invalidate); > +Datum > +pg_buffercache_invalidate(PG_FUNCTION_ARGS) I don't think "invalidating" is the right terminology. Note that we already have InvalidateBuffer() - but it's something we can't allow users to do, as it throws away dirty buffer contents (it's used for things like dropping a table). How about using "discarding" for this functionality? Using the buffer ID as the identifier doesn't seem great, because what that buffer is used for, could have changed since the buffer ID has been acquired (via the pg_buffercache view presumably)? My suspicion is that the usual usecase for this would be to drop all buffers that can be dropped? > + if (bufnum < 0 || bufnum > NBuffers) > + { > + ereport(ERROR, > + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), > + errmsg("buffernum is not valid"))); > + > + } > + > + result = TryInvalidateBuffer(bufnum); > + PG_RETURN_BOOL(result); > +} I think this should be restricted to superuser by default (by revoking permissions from PUBLIC). We allow normal users to use pg_prewarm(...), true - but we perform an ACL check on the relation, so it can only be used for relations you have access too. This function could be used to affect performance of other users quite substantially. > +/* > +Try Invalidating a buffer using bufnum. > +If the buffer is invalid, the function returns false. > +The function checks for dirty buffer and flushes the dirty buffer before invalidating. > +If the buffer is still dirty it returns false. > +*/ > +bool > +TryInvalidateBuffer(Buffer bufnum) > +{ > + BufferDesc *bufHdr = GetBufferDescriptor(bufnum - 1); > + uint32 buf_state; > + > + ReservePrivateRefCountEntry(); > + > + buf_state = LockBufHdr(bufHdr); > + if ((buf_state & BM_VALID) == BM_VALID) > + { > + /* > + * The buffer is pinned therefore cannot invalidate. > + */ > + if (BUF_STATE_GET_REFCOUNT(buf_state) > 0) > + { > + UnlockBufHdr(bufHdr, buf_state); > + return false; > + } > + if ((buf_state & BM_DIRTY) == BM_DIRTY) > + { > + /* > + * Try once to flush the dirty buffer. > + */ > + PinBuffer_Locked(bufHdr); > + LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED); > + FlushBuffer(bufHdr, NULL, IOOBJECT_RELATION, IOCONTEXT_NORMAL); > + LWLockRelease(BufferDescriptorGetContentLock(bufHdr)); > + UnpinBuffer(bufHdr); > + buf_state = LockBufHdr(bufHdr); > + if (BUF_STATE_GET_REFCOUNT(buf_state) > 0) > + { > + UnlockBufHdr(bufHdr, buf_state); > + return false; > + } > + > + /* > + * If its dirty again or not valid anymore give up. > + */ > + > + if ((buf_state & (BM_DIRTY | BM_VALID)) != (BM_VALID)) > + { > + UnlockBufHdr(bufHdr, buf_state); > + return false; > + } > + > + } > + > + InvalidateBuffer(bufHdr); I'm wary of using InvalidateBuffer() here, it's typically used for different purposes, including throwing valid contents away. That seems a bit scary. I think you should be able to just use InvalidateVictimBuffer()? Greetings, Andres Freund -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Thomas Munro <thomas.munro@gmail.com> — 2023-07-19T01:26:30Z
On Wed, Jul 19, 2023 at 12:45 PM Andres Freund <andres@anarazel.de> wrote: > I don't think "invalidating" is the right terminology. Note that we already > have InvalidateBuffer() - but it's something we can't allow users to do, as it > throws away dirty buffer contents (it's used for things like dropping a > table). > > How about using "discarding" for this functionality? +1 > Using the buffer ID as the identifier doesn't seem great, because what that > buffer is used for, could have changed since the buffer ID has been acquired > (via the pg_buffercache view presumably)? > > My suspicion is that the usual usecase for this would be to drop all buffers > that can be dropped? Well the idea was to be able to drop less than everything. Instead of having to bike-shed what the user interface should look like to specify what subset of everything you want to drop, you can just write SQL queries (mostly likely involving the pg_buffercache view, indeed). It's true that buffer IDs can change underneath your feet between SELECT and discard, but the whole concept is inherently racy like that. Suppose we instead had pg_unwarm('my_table') or whatever instead. Immediately after it runs and before it even returns, some blocks of my_table can finish up coming back into the pool. It's also interesting to be able to kick individual pages out when testing code that caches buffers IDs for ReadRecentBuffer(), and other buffer-pool work. Hence desire to not try to be clever at all here, and just come up with the absolute bare minimum thing that can kick buffers out by ID and leave the rest up to hackers/experts who are willing and able to write queries to supply them. You can still drop everything that can be dropped -- generate_series. Or whatever you want. -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Cary Huang <cary.huang@highgo.ca> — 2023-07-28T21:25:04Z
Hello I had a look at the patch and tested it on CI bot, it compiles and tests fine both autoconf and meson. I noticed that the v2 patch contains the v1 patch file as well. Not sure if intended but put there my accident. > I don't think "invalidating" is the right terminology. Note that we already > have InvalidateBuffer() - but it's something we can't allow users to do, as it > throws away dirty buffer contents (it's used for things like dropping a > table). > > How about using "discarding" for this functionality? I think "invalidating" is the right terminology here, it is exactly what the feature is doing, it tries to invalidate a buffer ID by calling InvalidateBuffer() routine inside buffer manager and calls FlushBuffer() before invalidating if marked dirty. The problem here is that InvalidateBuffer() could be dangerous because it allows a user to invalidate buffer that may have data in other tables not owned by the current user, I think it all comes down to the purpose of this feature. Based on the description in this email thread, I feel like this feature should be categorized as a developer-only feature, to be used by PG developer to experiment and observe some development works by invalidating one more more specific buffers..... If this is the case, it may be helpful to add a "DEVELOPER_OPTIONS" in GUC, which allows or disallows the TryInvalidateBuffer() to run or to return error if user does not have this developer option enabled. If the purpose of this feature is for general users, then it would make sense to have something like pg_unwarm (exactly opposite of pg_prewarm) that takes table name (instead of buffer ID) and drop all buffers associated with that table name. There will be permission checks as well so a user cannot pg_unwarm a table owned by someone else. User in this case won't be able to invalidate a particular buffer, but he/she should not have to as a regular user anyway. thanks! Cary Huang ------------- HighGo Software Inc. (Canada) cary.huang@highgo.ca www.highgo.ca
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Palak Chaturvedi <chaturvedipalak1911@gmail.com> — 2023-08-01T04:38:52Z
Hii, Thanks for your feedback. We have decided to add a role for the extension to solve that problem. And concerning to pg_unwarm table I think we can create a new function to do that but I think a general user would not require to clear a table from buffercache. We can use bufferid and where statements to do the same if a superuser/(specific role) requests it. Thanks. On Sat, 29 Jul 2023 at 02:55, Cary Huang <cary.huang@highgo.ca> wrote: > > Hello > > I had a look at the patch and tested it on CI bot, it compiles and tests fine both autoconf and meson. I noticed that the v2 patch contains the v1 patch file as well. Not sure if intended but put there my accident. > > > I don't think "invalidating" is the right terminology. Note that we already > > have InvalidateBuffer() - but it's something we can't allow users to do, as it > > throws away dirty buffer contents (it's used for things like dropping a > > table). > > > > How about using "discarding" for this functionality? > > I think "invalidating" is the right terminology here, it is exactly what the feature is doing, it tries to invalidate a buffer ID by calling InvalidateBuffer() routine inside buffer manager and calls FlushBuffer() before invalidating if marked dirty. > > The problem here is that InvalidateBuffer() could be dangerous because it allows a user to invalidate buffer that may have data in other tables not owned by the current user, > > I think it all comes down to the purpose of this feature. Based on the description in this email thread, I feel like this feature should be categorized as a developer-only feature, to be used by PG developer to experiment and observe some development works by invalidating one more more specific buffers..... If this is the case, it may be helpful to add a "DEVELOPER_OPTIONS" in GUC, which allows or disallows the TryInvalidateBuffer() to run or to return error if user does not have this developer option enabled. > > If the purpose of this feature is for general users, then it would make sense to have something like pg_unwarm (exactly opposite of pg_prewarm) that takes table name (instead of buffer ID) and drop all buffers associated with that table name. There will be permission checks as well so a user cannot pg_unwarm a table owned by someone else. User in this case won't be able to invalidate a particular buffer, but he/she should not have to as a regular user anyway. > > thanks! > > Cary Huang > ------------- > HighGo Software Inc. (Canada) > cary.huang@highgo.ca > www.highgo.ca >
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Cedric Villemain <cedric.villemain+pgsql@abcsql.com> — 2023-11-22T10:04:59Z
Le 01/07/2023 à 00:09, Thomas Munro a écrit : > On Fri, Jun 30, 2023 at 10:47 PM Palak Chaturvedi > <chaturvedipalak1911@gmail.com> wrote: > We also talked a bit about how one might control the kernel page cache > in more fine-grained ways for testing purposes, but it seems like the > pgfincore project has that covered with its pgfadvise_willneed() and > pgfadvise_dontneed(). IMHO that project could use more page-oriented > operations (instead of just counts and coarse grains operations) but > that's something that could be material for patches to send to the > extension maintainers. This work, in contrast, is more tangled up > with bufmgr.c internals, so it feels like this feature belongs in a > core contrib module. Precisely what pgfincore is doing/offering already. Happy to propose to postgresql tree if there are interest. Next step for pgfincore is to add cachestat() syscall and evaluates benefits for PostgreSQL cost estimators of this new call. Here an example to achieve the warm/unwarm, each bit is a PostgreSQL page, so here we warm cache with the first 3 and remove the last 3 from cache (system cache, not shared buffers). -- Loading and Unloading cedric=# select * from pgfadvise_loader('pgbench_accounts', 0, true, true, B'111000'); relpath | os_page_size | os_pages_free | pages_loaded | pages_unloaded ------------------+--------------+---------------+--------------+---------------- base/11874/16447 | 4096 | 408376 | 3 | 3 --- Cédric Villemain +33 (0)6 20 30 22 52 https://Data-Bene.io PostgreSQL Expertise, Support, Training, R&D -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Cedric Villemain <cedric.villemain+pgsql@abcsql.com> — 2024-01-03T16:25:03Z
Hi Palak, I did a quick review of the patch: +CREATE FUNCTION pg_buffercache_invalidate(IN int, IN bool default true) +RETURNS bool +AS 'MODULE_PATHNAME', 'pg_buffercache_invalidate' +LANGUAGE C PARALLEL SAFE; --> Not enforced anywhere, but you can also add a comment to the function, for end users... +PG_FUNCTION_INFO_V1(pg_buffercache_invalidate); +Datum +pg_buffercache_invalidate(PG_FUNCTION_ARGS) +{ + Buffer bufnum; "Buffer blocknum" is not correct in this context I believe. Buffer is when you have to manage Local buffer too (negative number). Here uint32 is probably the good choice at the end, as used in pg_buffercache in other places. Also in this extension bufferid is used, not buffernum. + bufnum = PG_GETARG_INT32(0); + if (bufnum <= 0 || bufnum > NBuffers) maybe have a look at pageinspect and its PG_GETARG_UINT32. + { + ereport(ERROR, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("buffernum is not valid"))); https://www.postgresql.org/docs/16/error-style-guide.html let me think that message like 'buffernum is not valid' can be enhanced: out of range, cannot be negative or exceed number of shared buffers.... ? Maybe add the value to the message. + + } + + /* + * Check whether to force invalidate the dirty buffer. The default value of force is true. + */ + + force = PG_GETARG_BOOL(1); I think you also need to test PG_ARGISNULL with force parameter. +/* + * Try Invalidating a buffer using bufnum. + * If the buffer is invalid, the function returns false. + * The function checks for dirty buffer and flushes the dirty buffer before invalidating. + * If the buffer is still dirty it returns false. + */ +bool +TryInvalidateBuffer(Buffer bufnum, bool force) +{ + BufferDesc *bufHdr = GetBufferDescriptor(bufnum - 1); this is not safe, GetBufferDescriptor() accepts uint, but can receive negative here. Use uint32 and bufferid. + uint32 buf_state; + ReservePrivateRefCountEntry(); + + buf_state = LockBufHdr(bufHdr); + if ((buf_state & BM_VALID) == BM_VALID) + { + /* + * The buffer is pinned therefore cannot invalidate. + */ + if (BUF_STATE_GET_REFCOUNT(buf_state) > 0) + { + UnlockBufHdr(bufHdr, buf_state); + return false; + } + if ((buf_state & BM_DIRTY) == BM_DIRTY) + { + /* + * If the buffer is dirty and the user has not asked to clear the dirty buffer return false. + * Otherwise clear the dirty buffer. + */ + if(!force){ + return false; probably need to unlockbuffer here too. + } + /* + * Try once to flush the dirty buffer. + */ + ResourceOwnerEnlargeBuffers(CurrentResourceOwner); + PinBuffer_Locked(bufHdr); + LWLockAcquire(BufferDescriptorGetContentLock(bufHdr), LW_SHARED); + FlushBuffer(bufHdr, NULL, IOOBJECT_RELATION, IOCONTEXT_NORMAL); + LWLockRelease(BufferDescriptorGetContentLock(bufHdr)); + UnpinBuffer(bufHdr); I am unsure of this area (the code is correct, but I wonder why there is no static code for this part -from pin to unpin- in PostgreSQL), and maybe better to go with FlushOneBuffer() ? Also it is probably required to account for the shared buffer eviction in some pg_stat* view or table. Not sure how disk syncing is handled after this sequence nor if it's important ? + buf_state = LockBufHdr(bufHdr); + if (BUF_STATE_GET_REFCOUNT(buf_state) > 0) + { + UnlockBufHdr(bufHdr, buf_state); + return false; + } + + /* + * If its dirty again or not valid anymore give up. + */ + + if ((buf_state & (BM_DIRTY | BM_VALID)) != (BM_VALID)) + { + UnlockBufHdr(bufHdr, buf_state); + return false; + } + + } + + InvalidateBuffer(bufHdr); + return true; + } + else + { + UnlockBufHdr(bufHdr, buf_state); + return false; + } Maybe safe to remove the else {} ... Maybe more tempting to start the big if with the following instead less nested...): + if ((buf_state & BM_VALID) != BM_VALID) + { + UnlockBufHdr(bufHdr, buf_state); + return false; + } Doc and test are absent. --- Cédric Villemain +33 (0)6 20 30 22 52 https://Data-Bene.io PostgreSQL Expertise, Support, Training, R&D -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Jim Nasby <jim.nasby@gmail.com> — 2024-01-03T23:15:11Z
<!DOCTYPE html> <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> <div class="moz-cite-prefix">On 1/3/24 10:25 AM, Cédric Villemain wrote:<br> </div> <blockquote type="cite" cite="mid:8b8c709a-c7cc-4965-8296-64f549c27501@abcsql.com">Hi Palak, <br> <br> I did a quick review of the patch: <br> <br> +CREATE FUNCTION pg_buffercache_invalidate(IN int, IN bool default true) <br> +RETURNS bool <br> +AS 'MODULE_PATHNAME', 'pg_buffercache_invalidate' <br> +LANGUAGE C PARALLEL SAFE; <br> <br> --> Not enforced anywhere, but you can also add a comment to the function, for end users... <br> </blockquote> <p>The arguments should also have names...</p> <blockquote type="cite" cite="mid:8b8c709a-c7cc-4965-8296-64f549c27501@abcsql.com"><br> + force = PG_GETARG_BOOL(1); <br> <br> I think you also need to test PG_ARGISNULL with force parameter. <br> </blockquote> Actually, that's true for the first argument as well. Or, just mark the function as STRICT.<br> <pre class="moz-signature" cols="72">-- Jim Nasby, Data Architect, Austin TX</pre> </body> </html> -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Cedric Villemain <cedric.villemain+pgsql@abcsql.com> — 2024-01-14T13:36:26Z
Hi Palak, there is currently even more interest in your patch as it should help building tests for on-going development around cache/read management/effects. Do you expect to be able to follow-up in the coming future ? Thank you, Cédric On 04/01/2024 00:15, Jim Nasby wrote: > On 1/3/24 10:25 AM, Cédric Villemain wrote: >> Hi Palak, >> >> I did a quick review of the patch: >> >> +CREATE FUNCTION pg_buffercache_invalidate(IN int, IN bool default true) >> +RETURNS bool >> +AS 'MODULE_PATHNAME', 'pg_buffercache_invalidate' >> +LANGUAGE C PARALLEL SAFE; >> >> --> Not enforced anywhere, but you can also add a comment to the >> function, for end users... > > The arguments should also have names... > >> >> + force = PG_GETARG_BOOL(1); >> >> I think you also need to test PG_ARGISNULL with force parameter. > Actually, that's true for the first argument as well. Or, just mark the > function as STRICT. > > -- > Jim Nasby, Data Architect, Austin TX > -- --- Cédric Villemain +33 (0)6 20 30 22 52 https://Data-Bene.io PostgreSQL Expertise, Support, Training, R&D
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Thomas Munro <thomas.munro@gmail.com> — 2024-02-26T22:41:23Z
[Sorry to those who received this message twice -- the first time got bounced by the list because of a defunct email address in the CC list.] Here is a rebase of Palak's v2 patch. I didn't change anything except for the required resource manager API change, a pgindent run, and removal of a stray file, and there is still some feedback to be addressed before we can get this in, but I wanted to fix the bitrot and re-open this CF item because this is very useful work. It's essential for testing the prefetching-related stuff happening in various other threads, where you want to be able to get the buffer pool into various interesting states.
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Maxim Orlov <orlovmg@gmail.com> — 2024-03-07T17:20:11Z
Quite an interesting patch, in my opinion. I've decided to work on it a bit, did some refactoring (sorry) and add basic tests. Also, I try to take into account as much as possible notes on the patch, mentioned by Cédric Villemain. > and maybe better to go with FlushOneBuffer() ? It's a good idea, but I'm not sure at the moment. I'll try to dig some deeper into it. At least, FlushOneBuffer does not work for a local buffers. So, we have to decide whatever pg_buffercache_invalidate should or should not work for local buffers. For now, I don't see why it should not. Maybe I miss something? -- Best regards, Maxim Orlov.
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Thomas Munro <thomas.munro@gmail.com> — 2024-04-04T03:22:17Z
On Fri, Mar 8, 2024 at 6:20 AM Maxim Orlov <orlovmg@gmail.com> wrote: > Quite an interesting patch, in my opinion. I've decided to work on it a bit, did some refactoring (sorry) and add > basic tests. Also, I try to take into account as much as possible notes on the patch, mentioned by Cédric Villemain. Thanks! Unfortunately I don't think it's possible to include a regression test that looks at the output, because it'd be non-deterministic. Any other backend could pin or dirty the buffer you try to evict, changing the behaviour. > > and maybe better to go with FlushOneBuffer() ? > It's a good idea, but I'm not sure at the moment. I'll try to dig some deeper into it. At least, FlushOneBuffer does > not work for a local buffers. So, we have to decide whatever pg_buffercache_invalidate should or should not > work for local buffers. For now, I don't see why it should not. Maybe I miss something? I think it's OK to ignore local buffers for now. pg_buffercache generally doesn't support/show them so I don't feel inclined to support them for this. I removed a few traces of local support. It didn't seem appropriate to use the pg_monitor role for this, so I made it superuser-only. I don't think it makes much sense to use this on any kind of production system so I don't think we need a new role for it, and existing roles don't seem too appropriate. pageinspect et al use the same approach. I added a VOLATILE qualifier to the function. I added some documentation. I changed the name to pg_buffercache_evict(). I got rid of the 'force' flag which was used to say 'I only want to evict this buffer it is clean'. I don't really see the point in that, we might as well keep it simple. You could filter buffers on "isdirty" if you want. I added comments to scare anyone off using EvictBuffer() for anything much, and marking it as something for developer convenience. (I am aware of an experimental patch that uses this same function as part of a buffer pool resizing operation, but that has other infrastructure to make that safe and would adjust those remarks accordingly.) I wondered whether it should really be testing for BM_TAG_VALID rather than BM_VALID. Arguably, but it doesn't seem important for now. The distinction would arise if someone had tried to read in a buffer, got an I/O error and abandoned ship, leaving a buffer with a valid tag but not valid contents. Anyone who tries to ReadBuffer() it will then try to read it again, but in the meantime this function won't be able to evict it (it'll just return false). Doesn't seem that obvious to me that this obscure case needs to be handled. That doesn't happen *during* a non-error case, because then it's pinned and we already return false in this code for pins. I contemplated whether InvalidateBuffer() or InvalidateVictimBuffer() would be better here and realised that Andres's intuition was probably right when he suggested the latter up-thread. It is designed with the right sort of arbitrary concurrent activity in mind, where the former assumes things about locking and dropping, which could get us into trouble if not now maybe in the future. I ran the following diabolical buffer blaster loop while repeatedly running installcheck: do $$ begin loop perform pg_buffercache_evict(bufferid) from pg_buffercache where random() <= 0.25; end loop; End; $$; The only ill-effect was a hot laptop. Thoughts, objections, etc? Very simple example of use: create or replace function uncache_relation(name text) returns boolean begin atomic; select bool_and(pg_buffercache_evict(bufferid)) from pg_buffercache where reldatabase = (select oid from pg_database where datname = current_database()) and relfilenode = pg_relation_filenode(name); end; More interesting for those of us hacking on streaming I/O stuff was the ability to evict just parts of things and see how the I/O merging and I/O depth react. -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Thomas Munro <thomas.munro@gmail.com> — 2024-04-06T23:07:58Z
On second thoughts, I think the original "invalidate" terminology was fine, no need to invent a new term. I thought of a better name for the bufmgr.c function though: InvalidateUnpinnedBuffer(). That name seemed better to me after I festooned it with warnings about why exactly it's inherently racy and only for testing use. I suppose someone could propose an additional function pg_buffercache_invalidate(db, tbspc, rel, fork, blocknum) that would be slightly better in the sense that it couldn't accidentally evict some innocent block that happened to replace the real target just before it runs, but I don't think it matters much for this purpose and it would still be racy on return (vacuum decides to load your block back in) so I don't think it's worth bothering with. So this is the version I plan to commit.
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Melanie Plageman <melanieplageman@gmail.com> — 2024-04-07T23:53:22Z
On Sat, Apr 6, 2024 at 7:08 PM Thomas Munro <thomas.munro@gmail.com> wrote: > > On second thoughts, I think the original "invalidate" terminology was > fine, no need to invent a new term. > > I thought of a better name for the bufmgr.c function though: > InvalidateUnpinnedBuffer(). That name seemed better to me after I > festooned it with warnings about why exactly it's inherently racy and > only for testing use. > > I suppose someone could propose an additional function > pg_buffercache_invalidate(db, tbspc, rel, fork, blocknum) that would > be slightly better in the sense that it couldn't accidentally evict > some innocent block that happened to replace the real target just > before it runs, but I don't think it matters much for this purpose and > it would still be racy on return (vacuum decides to load your block > back in) so I don't think it's worth bothering with. > > So this is the version I plan to commit. I've reviewed v6. I think you should mention in the docs that it only works for shared buffers -- so specifically not buffers containing blocks of temp tables. In the function pg_buffercache_invalidate(), why not use the BufferIsValid() function? - if (buf < 1 || buf > NBuffers) + if (!BufferIsValid(buf) || buf > NBuffers) I thought the below would be more clear for the comment above InvalidateUnpinnedBuffer(). - * Returns true if the buffer was valid and it has now been made invalid. - * Returns false if the wasn't valid, or it couldn't be evicted due to a pin, - * or if the buffer becomes dirty again while we're trying to write it out. + * Returns true if the buffer was valid and has now been made invalid. Returns + * false if it wasn't valid, if it couldn't be evicted due to a pin, or if the + * buffer becomes dirty again while we're trying to write it out. Some of that probably applies for the docs too (i.e. you have some similar wording in the docs). There is actually one typo in your version, so even if you don't adopt my suggestion, you should fix that typo. I didn't notice anything else out of place. I tried it and it worked as expected. I'm excited to have this feature! I didn't read through this whole thread, but was there any talk of adding other functions to let me invalidate a bunch of buffers at once or even some options -- like invalidate every 3rd buffer or whatever? (Not the concern of this patch, but just wondering because that would be a useful future enhancement IMO). - Melanie
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Andres Freund <andres@anarazel.de> — 2024-04-08T00:10:13Z
Hi, On 2024-04-07 11:07:58 +1200, Thomas Munro wrote: > I thought of a better name for the bufmgr.c function though: > InvalidateUnpinnedBuffer(). That name seemed better to me after I > festooned it with warnings about why exactly it's inherently racy and > only for testing use. I still dislike that, fwiw, due to the naming similarity to InvalidateBuffer(), which throws away dirty buffer contents too. Which obviously isn't acceptable from "userspace". I'd just name it pg_buffercache_evict() - given that the commit message's first paragraph uses "it is useful to be able to evict arbitrary blocks" that seems to describe things at least as well as "invalidate"? Greetings, Andres Freund
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Thomas Munro <thomas.munro@gmail.com> — 2024-04-08T04:30:34Z
On Mon, Apr 8, 2024 at 12:10 PM Andres Freund <andres@anarazel.de> wrote: > On 2024-04-07 11:07:58 +1200, Thomas Munro wrote: > > I thought of a better name for the bufmgr.c function though: > > InvalidateUnpinnedBuffer(). That name seemed better to me after I > > festooned it with warnings about why exactly it's inherently racy and > > only for testing use. > > I still dislike that, fwiw, due to the naming similarity to > InvalidateBuffer(), which throws away dirty buffer contents too. Which > obviously isn't acceptable from "userspace". I'd just name it > pg_buffercache_evict() - given that the commit message's first paragraph uses > "it is useful to be able to evict arbitrary blocks" that seems to describe > things at least as well as "invalidate"? Alright, sold. I'll go with EvictUnpinnedBuffer() in bufmgr.c and pg_buffercache_evict() in the contrib module.
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Thomas Munro <thomas.munro@gmail.com> — 2024-04-08T05:02:58Z
On Mon, Apr 8, 2024 at 11:53 AM Melanie Plageman <melanieplageman@gmail.com> wrote: > I've reviewed v6. I think you should mention in the docs that it only > works for shared buffers -- so specifically not buffers containing > blocks of temp tables. Thanks for looking! The whole pg_buffercache extension is for working with shared buffers only, as mentioned at the top. I have tried to improve that paragraph though, as it only mentioned examining them. > In the function pg_buffercache_invalidate(), why not use the > BufferIsValid() function? > > - if (buf < 1 || buf > NBuffers) > + if (!BufferIsValid(buf) || buf > NBuffers) It doesn't check the range (it has assertions, not errors). > I thought the below would be more clear for the comment above > InvalidateUnpinnedBuffer(). > > - * Returns true if the buffer was valid and it has now been made invalid. > - * Returns false if the wasn't valid, or it couldn't be evicted due to a pin, > - * or if the buffer becomes dirty again while we're trying to write it out. > + * Returns true if the buffer was valid and has now been made invalid. Returns > + * false if it wasn't valid, if it couldn't be evicted due to a pin, or if the > + * buffer becomes dirty again while we're trying to write it out. Fixed. > Some of that probably applies for the docs too (i.e. you have some > similar wording in the docs). There is actually one typo in your > version, so even if you don't adopt my suggestion, you should fix that > typo. Yeah, thanks, improved similarly there. > I didn't notice anything else out of place. I tried it and it worked > as expected. I'm excited to have this feature! Thanks! > I didn't read through this whole thread, but was there any talk of > adding other functions to let me invalidate a bunch of buffers at once > or even some options -- like invalidate every 3rd buffer or whatever? > (Not the concern of this patch, but just wondering because that would > be a useful future enhancement IMO). TBH I tried to resist people steering in that direction because you can also just define a SQL function to do that built on this, and if you had specialised functions they'd never be quite right. IMHO we succeeded in minimising the engineering and maximising flexibility, 'cause it's for hackers. Crude, but already able to express a wide range of stuff by punting the problem to SQL. Thanks to Palak for the patch. Pushed.
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Maksim Milyutin <milyutinma@gmail.com> — 2024-04-14T18:16:15Z
On 07.04.2024 02:07, Thomas Munro wrote: > So this is the version I plan to commit. > > +bool > +EvictUnpinnedBuffer(Buffer buf) > +{ > ... > + /* This will return false if it becomes dirty or someone else pins it. */ > + result = InvalidateVictimBuffer(desc); > + > + UnpinBuffer(desc); > + > + return result; > +} Hi, Thomas! Should not we call at the end the StrategyFreeBuffer() function to add target buffer to freelist and not miss it after invalidation? -- Best regards, Maksim Milyutin -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Maksim Milyutin <milyutinma@gmail.com> — 2024-04-29T18:47:41Z
On 14.04.2024 21:16, Maksim Milyutin wrote: > On 07.04.2024 02:07, Thomas Munro wrote: > >> So this is the version I plan to commit. >> >> +bool >> +EvictUnpinnedBuffer(Buffer buf) >> +{ >> ... >> + /* This will return false if it becomes dirty or someone else pins it. */ >> + result = InvalidateVictimBuffer(desc); >> + >> + UnpinBuffer(desc); >> + >> + return result; >> +} > > > Hi, Thomas! > > Should not we call at the end the StrategyFreeBuffer() function to add > target buffer to freelist and not miss it after invalidation? > Hello everyone! Please take a look at this issue, current implementation of EvictUnpinnedBuffer() IMO is erroneous - evicted buffers are lost permanently and will not be reused again -- Best regards, Maksim Milyutin -
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Thomas Munro <thomas.munro@gmail.com> — 2024-04-29T19:17:34Z
On Tue, Apr 30, 2024 at 6:47 AM Maksim Milyutin <milyutinma@gmail.com> wrote: >> Should not we call at the end the StrategyFreeBuffer() function to add target buffer to freelist and not miss it after invalidation? > Please take a look at this issue, current implementation of EvictUnpinnedBuffer() IMO is erroneous - evicted buffers are lost permanently and will not be reused again Hi Maksim, Oops, thanks, will fix.
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Thomas Munro <thomas.munro@gmail.com> — 2024-04-29T20:59:15Z
On Tue, Apr 30, 2024 at 7:17 AM Thomas Munro <thomas.munro@gmail.com> wrote: > On Tue, Apr 30, 2024 at 6:47 AM Maksim Milyutin <milyutinma@gmail.com> wrote: > >> Should not we call at the end the StrategyFreeBuffer() function to add target buffer to freelist and not miss it after invalidation? > > > Please take a look at this issue, current implementation of EvictUnpinnedBuffer() IMO is erroneous - evicted buffers are lost permanently and will not be reused again I don't think that's true: it is not lost permanently, it'll be found by the regular clock hand. Perhaps it should be put on the freelist so it can be found again quickly, but I'm not sure that's a bug, is it? If it were true, even basic testing eg select count(pg_buffercache_evict(bufferid)) from pg_buffercache would leave the system non-functional, but it doesn't, the usual CLOCK algorithm just does its thing.
-
Re: Extension Enhancement: Buffer Invalidation in pg_buffercache
Maksim Milyutin <milyutinma@gmail.com> — 2024-04-30T06:59:07Z
On 29.04.2024 23:59, Thomas Munro wrote: > On Tue, Apr 30, 2024 at 7:17 AM Thomas Munro<thomas.munro@gmail.com> wrote: >> On Tue, Apr 30, 2024 at 6:47 AM Maksim Milyutin<milyutinma@gmail.com> wrote: >>>> Should not we call at the end the StrategyFreeBuffer() function to add target buffer to freelist and not miss it after invalidation? >>> Please take a look at this issue, current implementation of EvictUnpinnedBuffer() IMO is erroneous - evicted buffers are lost permanently and will not be reused again > I don't think that's true: it is not lost permanently, it'll be found > by the regular clock hand. Perhaps it should be put on the freelist > so it can be found again quickly, but I'm not sure that's a bug, is > it? Yeah, you are right. Thanks for clarification. CLOCK algorithm will reuse it eventually but being of evicted cleared buffer in freelist might greatly restrict the time of buffer allocation when all others buffers were in use. -- Best regards, Maksim Milyutin