public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 1/1] resownerbench 5+ messages / 4 participants [nested] [flat]
* [PATCH 1/1] resownerbench @ 2020-11-11 22:40 Heikki Linnakangas <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Heikki Linnakangas @ 2020-11-11 22:40 UTC (permalink / raw) --- contrib/resownerbench/Makefile | 17 ++ contrib/resownerbench/resownerbench--1.0.sql | 14 ++ contrib/resownerbench/resownerbench.c | 154 +++++++++++++++++++ contrib/resownerbench/resownerbench.control | 6 + contrib/resownerbench/snaptest.sql | 37 +++++ 5 files changed, 228 insertions(+) create mode 100644 contrib/resownerbench/Makefile create mode 100644 contrib/resownerbench/resownerbench--1.0.sql create mode 100644 contrib/resownerbench/resownerbench.c create mode 100644 contrib/resownerbench/resownerbench.control create mode 100644 contrib/resownerbench/snaptest.sql diff --git a/contrib/resownerbench/Makefile b/contrib/resownerbench/Makefile new file mode 100644 index 00000000000..9b0e1cfee1a --- /dev/null +++ b/contrib/resownerbench/Makefile @@ -0,0 +1,17 @@ +MODULE_big = resownerbench +OBJS = resownerbench.o + +EXTENSION = resownerbench +DATA = resownerbench--1.0.sql +PGFILEDESC = "resownerbench - benchmark for ResourceOwners" + +ifdef USE_PGXS +PG_CONFIG = pg_config +PGXS := $(shell $(PG_CONFIG) --pgxs) +include $(PGXS) +else +subdir = contrib/resownerbench +top_builddir = ../.. +include $(top_builddir)/src/Makefile.global +include $(top_srcdir)/contrib/contrib-global.mk +endif diff --git a/contrib/resownerbench/resownerbench--1.0.sql b/contrib/resownerbench/resownerbench--1.0.sql new file mode 100644 index 00000000000..d29182f5982 --- /dev/null +++ b/contrib/resownerbench/resownerbench--1.0.sql @@ -0,0 +1,14 @@ +/* contrib/resownerbench/resownerbench--1.0.sql */ + +-- complain if script is sourced in psql, rather than via CREATE EXTENSION +\echo Use "CREATE EXTENSION resownerbench" to load this file. \quit + +CREATE FUNCTION snapshotbench_lifo(numkeep int, numsnaps int, numiters int) +RETURNS double precision +AS 'MODULE_PATHNAME', 'snapshotbench_lifo' +LANGUAGE C STRICT VOLATILE; + +CREATE FUNCTION snapshotbench_fifo(numkeep int, numsnaps int, numiters int) +RETURNS double precision +AS 'MODULE_PATHNAME', 'snapshotbench_fifo' +LANGUAGE C STRICT VOLATILE; diff --git a/contrib/resownerbench/resownerbench.c b/contrib/resownerbench/resownerbench.c new file mode 100644 index 00000000000..acfb6c39199 --- /dev/null +++ b/contrib/resownerbench/resownerbench.c @@ -0,0 +1,154 @@ +#include "postgres.h" + +#include "catalog/pg_type.h" +#include "catalog/pg_statistic.h" +#include "executor/spi.h" +#include "funcapi.h" +#include "libpq/pqsignal.h" +#include "utils/catcache.h" +#include "utils/syscache.h" +#include "utils/timestamp.h" +#include "utils/snapmgr.h" + +PG_MODULE_MAGIC; + +PG_FUNCTION_INFO_V1(snapshotbench_lifo); +PG_FUNCTION_INFO_V1(snapshotbench_fifo); + +/* + * ResourceOwner Performance test, using RegisterSnapshot(). + * + * This takes three parameters: numkeep, numsnaps, numiters. + * + * First, we register 'numkeep' snapshots. They are kept registed + * until the end of the test. Then, we repeatedly register and + * unregister 'numsnaps - numkeep' additional snapshots, repeating + * 'numiters' times. All the register/unregister calls are made in + * LIFO order. + * + * Returns the time spent, in milliseconds. + * + * The idea is to test the performance of ResourceOwnerRemember() + * and ReourceOwnerForget() operations, under different regimes. + * + * In the old implementation, if 'numsnaps' is small enough, all + * the entries fit in the resource owner's small array (it can + * hold 64 entries). + * + * In the new implementation, the array is much smaller, only 8 + * entries, but it's used together with the hash table so that + * we stay in the "array regime" as long as 'numsnaps - numkeep' + * is smaller than 8 entries. + * + * 'numiters' can be adjusted to adjust the overall runtime to be + * suitable long. + */ +Datum +snapshotbench_lifo(PG_FUNCTION_ARGS) +{ + int numkeep = PG_GETARG_INT32(0); + int numsnaps = PG_GETARG_INT32(1); + int numiters = PG_GETARG_INT32(2); + int i; + instr_time start, + duration; + Snapshot lsnap; + Snapshot *rs; + int numregistered = 0; + + rs = palloc(Max(numsnaps, numkeep) * sizeof(Snapshot)); + + lsnap = GetLatestSnapshot(); + + PG_SETMASK(&BlockSig); + INSTR_TIME_SET_CURRENT(start); + + while (numregistered < numkeep) + { + rs[numregistered] = RegisterSnapshot(lsnap); + numregistered++; + } + + for (i = 0 ; i < numiters; i++) + { + while (numregistered < numsnaps) + { + rs[numregistered] = RegisterSnapshot(lsnap); + numregistered++; + } + + while (numregistered > numkeep) + { + numregistered--; + UnregisterSnapshot(rs[numregistered]); + } + } + + while (numregistered > 0) + { + numregistered--; + UnregisterSnapshot(rs[numregistered]); + } + + INSTR_TIME_SET_CURRENT(duration); + INSTR_TIME_SUBTRACT(duration, start); + PG_SETMASK(&UnBlockSig); + + PG_RETURN_FLOAT8(INSTR_TIME_GET_MILLISEC(duration)); +}; + + +/* + * Same, but do the register/unregister operations in + * FIFO order. + */ +Datum +snapshotbench_fifo(PG_FUNCTION_ARGS) +{ + int numkeep = PG_GETARG_INT32(0); + int numsnaps = PG_GETARG_INT32(1); + int numiters = PG_GETARG_INT32(2); + int i, + j; + instr_time start, + duration; + Snapshot lsnap; + Snapshot *rs; + int numregistered = 0; + + rs = palloc(Max(numsnaps, numkeep) * sizeof(Snapshot)); + + lsnap = GetLatestSnapshot(); + + PG_SETMASK(&BlockSig); + INSTR_TIME_SET_CURRENT(start); + + while (numregistered < numkeep) + { + rs[numregistered] = RegisterSnapshot(lsnap); + numregistered++; + } + + for (i = 0 ; i < numiters; i++) + { + while (numregistered < numsnaps) + { + rs[numregistered] = RegisterSnapshot(lsnap); + numregistered++; + } + + for (j = numkeep; j < numregistered; j++) + UnregisterSnapshot(rs[j]); + numregistered = numkeep; + } + + for (j = 0; j < numregistered; j++) + UnregisterSnapshot(rs[j]); + numregistered = numkeep; + + INSTR_TIME_SET_CURRENT(duration); + INSTR_TIME_SUBTRACT(duration, start); + PG_SETMASK(&UnBlockSig); + + PG_RETURN_FLOAT8(INSTR_TIME_GET_MILLISEC(duration)); +}; diff --git a/contrib/resownerbench/resownerbench.control b/contrib/resownerbench/resownerbench.control new file mode 100644 index 00000000000..ada88b8eed8 --- /dev/null +++ b/contrib/resownerbench/resownerbench.control @@ -0,0 +1,6 @@ +# resownerbench + +comment = 'benchmark for ResourceOwners' +default_version = '1.0' +module_pathname = '$libdir/resownerbench' +relocatable = true diff --git a/contrib/resownerbench/snaptest.sql b/contrib/resownerbench/snaptest.sql new file mode 100644 index 00000000000..18c54e13fc9 --- /dev/null +++ b/contrib/resownerbench/snaptest.sql @@ -0,0 +1,37 @@ +-- +-- Performance test RegisterSnapshot/UnregisterSnapshot. +-- +select numkeep, numsnaps, + -- numiters, + -- round(lifo_time_ms) as lifo_total_time_ms, + -- round(fifo_time_ms) as fifo_total_time_ms, + round((lifo_time_ms::numeric / (numkeep + (numsnaps - numkeep) * numiters)) * 1000000, 1) as lifo_time_ns, + round((fifo_time_ms::numeric / (numkeep + (numsnaps - numkeep) * numiters)) * 1000000, 1) as fifo_time_ns +from +(values (0, 1, 10000000), + (0, 5, 2000000), + (0, 10, 1000000), + (0, 60, 100000), + (0, 70, 100000), + (0, 100, 100000), + (0, 1000, 10000), + (0, 10000, 1000), + +-- These tests keep 9 snapshots registered across the iterations. That +-- exceeds the size of the little array in the patch, so this exercises +-- the hash lookups. Without the patch, these still fit in the array +-- (it's 64 entries without the patch) + (9, 10, 10000000), + (9, 100, 100000), + (9, 1000, 10000), + (9, 10000, 1000), + +-- These exceed the 64 entry array even without the patch, so these fall +-- in the hash table regime with and without the patch. + (65, 70, 1000000), + (65, 100, 100000), + (65, 1000, 10000), + (65, 10000, 1000) +) AS params (numkeep, numsnaps, numiters), +lateral snapshotbench_lifo(numkeep, numsnaps, numiters) as lifo_time_ms, +lateral snapshotbench_fifo(numkeep, numsnaps, numiters) as fifo_time_ms; -- 2.29.2 --------------1104342304EC6341F5D7C60C-- ^ permalink raw reply [nested|flat] 5+ messages in thread
* Inconsistency in vacuum behavior @ 2023-01-16 08:18 Alexander Pyhalov <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Alexander Pyhalov @ 2023-01-16 08:18 UTC (permalink / raw) To: [email protected] Hi. We've run regress isolation tests on partitioned tables and found interesting VACUUM behavior. I'm not sure, if it's intended. In the following example, partitioned tables and regular tables behave differently: CREATE TABLE vacuum_tab (a int) PARTITION BY HASH (a); CREATE TABLE vacuum_tab_1 PARTITION OF vacuum_tab FOR VALUES WITH (MODULUS 2, REMAINDER 0); CREATE TABLE vacuum_tab_2 PARTITION OF vacuum_tab FOR VALUES WITH (MODULUS 2, REMAINDER 1); CREATE ROLE regress_vacuum_conflict; In the first session: begin; LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; In the second: SET ROLE regress_vacuum_conflict; VACUUM vacuum_tab; WARNING: permission denied to vacuum "vacuum_tab", skipping it <---- hangs here, trying to lock vacuum_tab_1 In non-partitioned case second session exits after emitting warning. In partitioned case, it hangs, trying to get locks. This is due to the fact that in expand_vacuum_rel() we skip parent table if vacuum_is_permitted_for_relation(), but don't perform such check for its child. The check will be performed later in vacuum_rel(), but after vacuum_open_relation(), which leads to hang in the second session. Is it intended? Why don't we perform vacuum_is_permitted_for_relation() check for inheritors in expand_vacuum_rel()? -- Best regards, Alexander Pyhalov, Postgres Professional ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Inconsistency in vacuum behavior @ 2023-01-16 13:48 Nikita Malakhov <[email protected]> parent: Alexander Pyhalov <[email protected]> 1 sibling, 1 reply; 5+ messages in thread From: Nikita Malakhov @ 2023-01-16 13:48 UTC (permalink / raw) To: Alexander Pyhalov <[email protected]>; +Cc: [email protected] Hi! I've checked this expand_vacuum_rel() and made a quick fix for this.Here's the result of the test: postgres@postgres=# set role regress_vacuum_conflict; SET Time: 0.369 ms postgres@postgres=> vacuum vacuum_tab; WARNING: permission denied to vacuum "vacuum_tab", skipping it WARNING: permission denied to vacuum "vacuum_tab_1", skipping it WARNING: permission denied to vacuum "vacuum_tab_2", skipping it VACUUM Time: 0.936 ms postgres@postgres=> Looks like it's a subject for a patch. On Mon, Jan 16, 2023 at 11:18 AM Alexander Pyhalov <[email protected]> wrote: > Hi. > > We've run regress isolation tests on partitioned tables and found > interesting VACUUM behavior. I'm not sure, if it's intended. > > In the following example, partitioned tables and regular tables behave > differently: > > CREATE TABLE vacuum_tab (a int) PARTITION BY HASH (a); > CREATE TABLE vacuum_tab_1 PARTITION OF vacuum_tab FOR VALUES WITH > (MODULUS 2, REMAINDER 0); > CREATE TABLE vacuum_tab_2 PARTITION OF vacuum_tab FOR VALUES WITH > (MODULUS 2, REMAINDER 1); > CREATE ROLE regress_vacuum_conflict; > > In the first session: > > begin; > LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; > > In the second: > SET ROLE regress_vacuum_conflict; > VACUUM vacuum_tab; > WARNING: permission denied to vacuum "vacuum_tab", skipping it <---- > hangs here, trying to lock vacuum_tab_1 > > In non-partitioned case second session exits after emitting warning. In > partitioned case, it hangs, trying to get locks. > This is due to the fact that in expand_vacuum_rel() we skip parent table > if vacuum_is_permitted_for_relation(), but don't perform such check for > its child. > The check will be performed later in vacuum_rel(), but after > vacuum_open_relation(), which leads to hang in the second session. > > Is it intended? Why don't we perform vacuum_is_permitted_for_relation() > check for inheritors in expand_vacuum_rel()? > > -- > Best regards, > Alexander Pyhalov, > Postgres Professional > > > -- Regards, Nikita Malakhov Postgres Professional https://postgrespro.ru/ ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Inconsistency in vacuum behavior @ 2023-01-16 14:26 Nikita Malakhov <[email protected]> parent: Nikita Malakhov <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Nikita Malakhov @ 2023-01-16 14:26 UTC (permalink / raw) To: Alexander Pyhalov <[email protected]>; +Cc: [email protected] Hi! Here's the patch that fixes this case, please check it out. The patch adds vacuum_is_permitted_for_relation() check before adding partition relation to the vacuum list, and if permission is denied the relation is not added, so it is not passed to vacuum_rel() and there are no try to acquire the lock. Cheers! On Mon, Jan 16, 2023 at 4:48 PM Nikita Malakhov <[email protected]> wrote: > Hi! > > I've checked this expand_vacuum_rel() and made a quick fix for this.Here's > the result of the test: > > postgres@postgres=# set role regress_vacuum_conflict; > SET > Time: 0.369 ms > postgres@postgres=> vacuum vacuum_tab; > WARNING: permission denied to vacuum "vacuum_tab", skipping it > WARNING: permission denied to vacuum "vacuum_tab_1", skipping it > WARNING: permission denied to vacuum "vacuum_tab_2", skipping it > VACUUM > Time: 0.936 ms > postgres@postgres=> > > Looks like it's a subject for a patch. > > On Mon, Jan 16, 2023 at 11:18 AM Alexander Pyhalov < > [email protected]> wrote: > >> Hi. >> >> We've run regress isolation tests on partitioned tables and found >> interesting VACUUM behavior. I'm not sure, if it's intended. >> >> In the following example, partitioned tables and regular tables behave >> differently: >> >> CREATE TABLE vacuum_tab (a int) PARTITION BY HASH (a); >> CREATE TABLE vacuum_tab_1 PARTITION OF vacuum_tab FOR VALUES WITH >> (MODULUS 2, REMAINDER 0); >> CREATE TABLE vacuum_tab_2 PARTITION OF vacuum_tab FOR VALUES WITH >> (MODULUS 2, REMAINDER 1); >> CREATE ROLE regress_vacuum_conflict; >> >> In the first session: >> >> begin; >> LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; >> >> In the second: >> SET ROLE regress_vacuum_conflict; >> VACUUM vacuum_tab; >> WARNING: permission denied to vacuum "vacuum_tab", skipping it <---- >> hangs here, trying to lock vacuum_tab_1 >> >> In non-partitioned case second session exits after emitting warning. In >> partitioned case, it hangs, trying to get locks. >> This is due to the fact that in expand_vacuum_rel() we skip parent table >> if vacuum_is_permitted_for_relation(), but don't perform such check for >> its child. >> The check will be performed later in vacuum_rel(), but after >> vacuum_open_relation(), which leads to hang in the second session. >> >> Is it intended? Why don't we perform vacuum_is_permitted_for_relation() >> check for inheritors in expand_vacuum_rel()? >> >> -- >> Best regards, >> Alexander Pyhalov, >> Postgres Professional >> >> >> > > -- > Regards, > Nikita Malakhov > Postgres Professional > https://postgrespro.ru/ > -- Regards, Nikita Malakhov Postgres Professional https://postgrespro.ru/ Attachments: [application/octet-stream] 0001_expand_vac_rel_part_chk_v1.patch (3.0K, ../../CAN-LCVPR3qYtvnwrG5Ci=m0hA3ct6icD69k7bCni7Uhb0bud0w@mail.gmail.com/3-0001_expand_vac_rel_part_chk_v1.patch) download | inline diff: From 48667c560fcc1d8c37b118f684a129db01f0d346 Mon Sep 17 00:00:00 2001 From: Nikita Malakhov <[email protected]> Date: Mon, 16 Jan 2023 17:14:07 +0300 Subject: [PATCH] The following test case found by PostgresPro team: In the following example, partitioned tables and regular tables behave differently: CREATE TABLE vacuum_tab (a int) PARTITION BY HASH (a); CREATE TABLE vacuum_tab_1 PARTITION OF vacuum_tab FOR VALUES WITH (MODULUS 2, REMAINDER 0); CREATE TABLE vacuum_tab_2 PARTITION OF vacuum_tab FOR VALUES WITH (MODULUS 2, REMAINDER 1); CREATE ROLE regress_vacuum_conflict; In the first session: begin; LOCK vacuum_tab IN SHARE UPDATE EXCLUSIVE MODE; In the second: SET ROLE regress_vacuum_conflict; VACUUM vacuum_tab; WARNING: permission denied to vacuum "vacuum_tab", skipping it <---- hangs here, trying to lock vacuum_tab_1 In non-partitioned case second session exits after emitting warning. In partitioned case, it hangs, trying to get locks. This is due to the fact that in expand_vacuum_rel() we skip parent table if vacuum_is_permitted_for_relation(), but don't perform such check for its child. The fix adds vacuum_is_permitted_for_relation() check before adding partition relation to the vacuum list, and while performing vacuum this emits warning that permission for vacuum is denied, and goes on without trying to acquire the lock. --- src/backend/commands/vacuum.c | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c index c4ed7efce3..99f6e334ee 100644 --- a/src/backend/commands/vacuum.c +++ b/src/backend/commands/vacuum.c @@ -849,7 +849,9 @@ expand_vacuum_rel(VacuumRelation *vrel, int options) foreach(part_lc, part_oids) { - Oid part_oid = lfirst_oid(part_lc); + Oid part_oid = lfirst_oid(part_lc); + HeapTuple part_tuple; + Form_pg_class part_classForm; if (part_oid == relid) continue; /* ignore original table */ @@ -859,11 +861,26 @@ expand_vacuum_rel(VacuumRelation *vrel, int options) * complain about failure to open one of these relations * later. */ - oldcontext = MemoryContextSwitchTo(vac_context); - vacrels = lappend(vacrels, makeVacuumRelation(NULL, - part_oid, - vrel->va_cols)); - MemoryContextSwitchTo(oldcontext); + + /* + * Check partition relations for vacuum permit. Do not add + * them to the list if vacuum is not permitted. + */ + part_tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(part_oid)); + if (HeapTupleIsValid(part_tuple)) + { + part_classForm = (Form_pg_class) GETSTRUCT(part_tuple); + + if (vacuum_is_permitted_for_relation(part_oid, part_classForm, options)) + { + oldcontext = MemoryContextSwitchTo(vac_context); + vacrels = lappend(vacrels, makeVacuumRelation(NULL, + part_oid, + vrel->va_cols)); + MemoryContextSwitchTo(oldcontext); + } + } + ReleaseSysCache(part_tuple); } } -- 2.25.1 ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Inconsistency in vacuum behavior @ 2023-01-21 01:12 Nathan Bossart <[email protected]> parent: Alexander Pyhalov <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Nathan Bossart @ 2023-01-21 01:12 UTC (permalink / raw) To: Alexander Pyhalov <[email protected]>; +Cc: [email protected] On Mon, Jan 16, 2023 at 11:18:08AM +0300, Alexander Pyhalov wrote: > Is it intended? Why don't we perform vacuum_is_permitted_for_relation() > check for inheritors in expand_vacuum_rel()? Since no lock is held on the partition, the calls to functions like object_ownercheck() and pg_class_aclcheck() in vacuum_is_permitted_for_relation() will produce cache lookup ERRORs if the relation is concurrently dropped. -- Nathan Bossart Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-01-21 01:12 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-11-11 22:40 [PATCH 1/1] resownerbench Heikki Linnakangas <[email protected]> 2023-01-16 08:18 Inconsistency in vacuum behavior Alexander Pyhalov <[email protected]> 2023-01-16 13:48 ` Re: Inconsistency in vacuum behavior Nikita Malakhov <[email protected]> 2023-01-16 14:26 ` Re: Inconsistency in vacuum behavior Nikita Malakhov <[email protected]> 2023-01-21 01:12 ` Re: Inconsistency in vacuum behavior Nathan Bossart <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox