public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 5/8] add special pg_brin_bloom_summary data type 5+ messages / 5 participants [nested] [flat]
* [PATCH 5/8] add special pg_brin_bloom_summary data type @ 2020-08-06 15:56 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Tomas Vondra @ 2020-08-06 15:56 UTC (permalink / raw) --- src/backend/access/brin/brin_bloom.c | 91 ++++++++++++++++++++++- src/include/catalog/pg_proc.dat | 14 ++++ src/include/catalog/pg_type.dat | 7 ++ src/test/regress/expected/type_sanity.out | 7 +- 4 files changed, 115 insertions(+), 4 deletions(-) diff --git a/src/backend/access/brin/brin_bloom.c b/src/backend/access/brin/brin_bloom.c index b119e4e264..e7ca100821 100644 --- a/src/backend/access/brin/brin_bloom.c +++ b/src/backend/access/brin/brin_bloom.c @@ -647,7 +647,7 @@ brin_bloom_opcinfo(PG_FUNCTION_ARGS) result->oi_regular_nulls = true; result->oi_opaque = (BloomOpaque *) MAXALIGN((char *) result + SizeofBrinOpcInfo(1)); - result->oi_typcache[0] = lookup_type_cache(BYTEAOID, 0); + result->oi_typcache[0] = lookup_type_cache(BRINBLOOMSUMMARYOID, 0); PG_RETURN_POINTER(result); } @@ -978,3 +978,92 @@ brin_bloom_options(PG_FUNCTION_ARGS) PG_RETURN_VOID(); } + +/* + * brin_bloom_summary_in + * - input routine for type brin_bloom_summary. + * + * brin_bloom_summary is only used internally to represent summaries + * in BRIN bloom indexes, so it has no operations of its own, and we + * disallow input too. + */ +Datum +brin_bloom_summary_in(PG_FUNCTION_ARGS) +{ + /* + * brin_bloom_summary stores the data in binary form and parsing + * text input is not needed, so disallow this. + */ + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot accept a value of type %s", "pg_brin_bloom_summary"))); + + PG_RETURN_VOID(); /* keep compiler quiet */ +} + + +/* + * brin_bloom_summary_out + * - output routine for type brin_bloom_summary. + * + * BRIN bloom summaries are serialized into a bytea value, but we want + * to output something nicer humans can understand. + */ +Datum +brin_bloom_summary_out(PG_FUNCTION_ARGS) +{ + BloomFilter *filter; + StringInfoData str; + + initStringInfo(&str); + appendStringInfoChar(&str, '{'); + + /* + * XXX not sure the detoasting is necessary (probably not, this + * can only be in an index). + */ + filter = (BloomFilter *) PG_DETOAST_DATUM(PG_GETARG_BYTEA_PP(0)); + + if (BLOOM_IS_HASHED(filter)) + { + appendStringInfo(&str, "mode: hashed nhashes: %u nbits: %u nbits_set: %u", + filter->nhashes, filter->nbits, filter->nbits_set); + } + else + { + appendStringInfo(&str, "mode: sorted nvalues: %u nsorted: %u", + filter->nvalues, filter->nsorted); + /* TODO include the sorted/unsorted values */ + } + + appendStringInfoChar(&str, '}'); + + PG_RETURN_CSTRING(str.data); +} + +/* + * brin_bloom_summary_recv + * - binary input routine for type brin_bloom_summary. + */ +Datum +brin_bloom_summary_recv(PG_FUNCTION_ARGS) +{ + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot accept a value of type %s", "pg_brin_bloom_summary"))); + + PG_RETURN_VOID(); /* keep compiler quiet */ +} + +/* + * brin_bloom_summary_send + * - binary output routine for type brin_bloom_summary. + * + * BRIN bloom summaries are serialized in a bytea value (although the + * type is named differently), so let's just send that. + */ +Datum +brin_bloom_summary_send(PG_FUNCTION_ARGS) +{ + return byteasend(fcinfo); +} diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 5336aa7b9c..da4c9c6202 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -10975,3 +10975,17 @@ prosrc => 'unicode_is_normalized' }, ] + +{ oid => '9035', descr => 'I/O', + proname => 'brin_bloom_summary_in', prorettype => 'pg_brin_bloom_summary', + proargtypes => 'cstring', prosrc => 'brin_bloom_summary_in' }, +{ oid => '9036', descr => 'I/O', + proname => 'brin_bloom_summary_out', prorettype => 'cstring', + proargtypes => 'pg_brin_bloom_summary', prosrc => 'brin_bloom_summary_out' }, +{ oid => '9037', descr => 'I/O', + proname => 'brin_bloom_summary_recv', provolatile => 's', + prorettype => 'pg_brin_bloom_summary', proargtypes => 'internal', + prosrc => 'brin_bloom_summary_recv' }, +{ oid => '9038', descr => 'I/O', + proname => 'brin_bloom_summary_send', provolatile => 's', prorettype => 'bytea', + proargtypes => 'pg_brin_bloom_summary', prosrc => 'brin_bloom_summary_send' }, diff --git a/src/include/catalog/pg_type.dat b/src/include/catalog/pg_type.dat index b2cec07416..a41c2e5418 100644 --- a/src/include/catalog/pg_type.dat +++ b/src/include/catalog/pg_type.dat @@ -631,3 +631,10 @@ typalign => 'd', typstorage => 'x' }, ] + +{ oid => '9034', oid_symbol => 'BRINBLOOMSUMMARYOID', + descr => 'BRIN bloom summary', + typname => 'pg_brin_bloom_summary', typlen => '-1', typbyval => 'f', typcategory => 'S', + typinput => 'brin_bloom_summary_in', typoutput => 'brin_bloom_summary_out', + typreceive => 'brin_bloom_summary_recv', typsend => 'brin_bloom_summary_send', + typalign => 'i', typstorage => 'x', typcollation => 'default' }, diff --git a/src/test/regress/expected/type_sanity.out b/src/test/regress/expected/type_sanity.out index 274130e706..97bf9797de 100644 --- a/src/test/regress/expected/type_sanity.out +++ b/src/test/regress/expected/type_sanity.out @@ -67,13 +67,14 @@ WHERE p1.typtype not in ('c','d','p') AND p1.typname NOT LIKE E'\\_%' (SELECT 1 FROM pg_type as p2 WHERE p2.typname = ('_' || p1.typname)::name AND p2.typelem = p1.oid and p1.typarray = p2.oid); - oid | typname -------+----------------- + oid | typname +------+----------------------- 194 | pg_node_tree 3361 | pg_ndistinct 3402 | pg_dependencies 5017 | pg_mcv_list -(4 rows) + 9034 | pg_brin_bloom_summary +(5 rows) -- Make sure typarray points to a varlena array type of our own base SELECT p1.oid, p1.typname as basetype, p2.typname as arraytype, -- 2.25.4 --jsivyprvf2oxfjz3 Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="0006-BRIN-multi-range-minmax-indexes-20200807.patch" ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age? @ 2023-04-14 12:30 Robert Haas <[email protected]> 2023-04-14 12:33 ` Re: Should we remove vacuum_defer_cleanup_age? Daniel Gustafsson <[email protected]> 2023-04-14 13:46 ` Re: Should we remove vacuum_defer_cleanup_age? Jonathan S. Katz <[email protected]> 0 siblings, 2 replies; 5+ messages in thread From: Robert Haas @ 2023-04-14 12:30 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; +Cc: Jonathan S. Katz <[email protected]>; Amit Kapila <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On Thu, Apr 13, 2023 at 11:06 PM Laurenz Albe <[email protected]> wrote: > I am not against this in principle, but I know that there are people using > this parameter; see the discussion linked in > > https://postgr.es/m/[email protected] > > I can't say if they have a good use case for that parameter or not. Yeah, I feel similarly. Actually, personally I have no evidence, not even an anecdote, suggesting that this parameter is in use, but I'm a bit skeptical of any consensus of the form "no one is using X," because there sure are a lot of people running PostgreSQL and they sure do a lot of things. Some more justifiably than others, but often people have surprisingly good excuses for doing stuff that sounds bizarre when you first hear about it, and it doesn't seem totally impossible that somebody could have found a way to get value out of this. However, I suspect that there aren't many such people, and I think the setting is a kludge, so I support removing it. Maybe we'll find out that we ought to add something else instead, like a limited delimited in time rather than in XIDs. Or maybe the existing facilities are good enough. But as Peter rightly says, XID age is likely a poor proxy for whatever people really care about, so I don't think continuing to have a setting that works like that is a good plan. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age? 2023-04-14 12:30 Re: Should we remove vacuum_defer_cleanup_age? Robert Haas <[email protected]> @ 2023-04-14 12:33 ` Daniel Gustafsson <[email protected]> 1 sibling, 0 replies; 5+ messages in thread From: Daniel Gustafsson @ 2023-04-14 12:33 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Laurenz Albe <[email protected]>; Jonathan S. Katz <[email protected]>; Amit Kapila <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers > On 14 Apr 2023, at 14:30, Robert Haas <[email protected]> wrote: > ..as Peter rightly says, XID age is likely a poor proxy for > whatever people really care about, so I don't think continuing to have > a setting that works like that is a good plan. Agreed, and removing it is likely to be a good vehicle for figuring out what we should have instead (if anything). -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age? 2023-04-14 12:30 Re: Should we remove vacuum_defer_cleanup_age? Robert Haas <[email protected]> @ 2023-04-14 13:46 ` Jonathan S. Katz <[email protected]> 2023-04-14 15:25 ` Re: Should we remove vacuum_defer_cleanup_age? Greg Stark <[email protected]> 1 sibling, 1 reply; 5+ messages in thread From: Jonathan S. Katz @ 2023-04-14 13:46 UTC (permalink / raw) To: Robert Haas <[email protected]>; Laurenz Albe <[email protected]>; +Cc: Amit Kapila <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On 4/14/23 8:30 AM, Robert Haas wrote: > On Thu, Apr 13, 2023 at 11:06 PM Laurenz Albe <[email protected]> wrote: >> I am not against this in principle, but I know that there are people using >> this parameter; see the discussion linked in >> >> https://postgr.es/m/[email protected] >> >> I can't say if they have a good use case for that parameter or not. > > Yeah, I feel similarly. Actually, personally I have no evidence, not > even an anecdote, suggesting that this parameter is in use, but I'm a > bit skeptical of any consensus of the form "no one is using X," > because there sure are a lot of people running PostgreSQL and they > sure do a lot of things. Some more justifiably than others, but often > people have surprisingly good excuses for doing stuff that sounds > bizarre when you first hear about it, and it doesn't seem totally > impossible that somebody could have found a way to get value out of > this. Let me restate [1] in a different way. Using a large enough dataset, I did qualitatively look at overall usage of both "vacuum_defer_cleanup_age" and compared to "hot_standby_feedback", given you can use both to accomplish similar outcomes. The usage of "vacuum_defer_cleanup_age" was really minimal, in fact approaching "0", whereas "hot_standby_feedback" had significant adoption. I'm not saying that "we should remove a setting just because it's not used" or that it may not have utility -- I'm saying that we can remove the setting given: 1. We know that using this setting incorrectly (which can be done fairly easily) can cause significant issues 2. There's another setting that can accomplish similar goals that's much safer 3. The setting itself is not widely used It's the combination of all 3 that led to my conclusion. If it were just (1), I'd lean more strongly towards trying to fix it first. > However, I suspect that there aren't many such people, and I think the > setting is a kludge, so I support removing it. Maybe we'll find out > that we ought to add something else instead, like a limited delimited > in time rather than in XIDs. Or maybe the existing facilities are good > enough. But as Peter rightly says, XID age is likely a poor proxy for > whatever people really care about, so I don't think continuing to have > a setting that works like that is a good plan. That seems like a good eventual outcome. Thanks, Jonathan [1] https://www.postgresql.org/message-id/bf42784f-4d57-0a3d-1a06-ffac1a09318c%40postgresql.org Attachments: [application/pgp-signature] OpenPGP_signature (840B, ../../[email protected]/2-OpenPGP_signature) download ^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: Should we remove vacuum_defer_cleanup_age? 2023-04-14 12:30 Re: Should we remove vacuum_defer_cleanup_age? Robert Haas <[email protected]> 2023-04-14 13:46 ` Re: Should we remove vacuum_defer_cleanup_age? Jonathan S. Katz <[email protected]> @ 2023-04-14 15:25 ` Greg Stark <[email protected]> 0 siblings, 0 replies; 5+ messages in thread From: Greg Stark @ 2023-04-14 15:25 UTC (permalink / raw) To: Jonathan S. Katz <[email protected]>; +Cc: Robert Haas <[email protected]>; Laurenz Albe <[email protected]>; Amit Kapila <[email protected]>; Andres Freund <[email protected]>; Justin Pryzby <[email protected]>; Alvaro Herrera <[email protected]>; pgsql-hackers On Fri, 14 Apr 2023 at 09:47, Jonathan S. Katz <[email protected]> wrote: > > Let me restate [1] in a different way. > > Using a large enough dataset, I did qualitatively look at overall usage > of both "vacuum_defer_cleanup_age" and compared to > "hot_standby_feedback", given you can use both to accomplish similar > outcomes. I assume people would use hot_standby_feedback if they have streaming replication. The main use cases for vacuum_defer_cleanup_age would be if you're replaying WAL files. That may sound archaic but there are plenty of circumstances where your standby may not have network access to your primary at all or not want to be replaying continuously. I wonder whether your dataset is self-selecting sites that have streaming replication. That does seem like the more common usage pattern. Systems using wal files are more likely to be things like data warehouses, offline analytics systems, etc. They may not even be well known in the same organization that runs the online operations -- in my experience they're often run by marketing or sales organizations or in some cases infosec teams and consume data from lots of sources. The main reason to use wal archive replay is often to provide the isolation so that the operations team don't need to worry about the impact on production and that makes it easy to forget these even exist. -- greg ^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2023-04-14 15:25 UTC | newest] Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2020-08-06 15:56 [PATCH 5/8] add special pg_brin_bloom_summary data type Tomas Vondra <[email protected]> 2023-04-14 12:30 Re: Should we remove vacuum_defer_cleanup_age? Robert Haas <[email protected]> 2023-04-14 12:33 ` Re: Should we remove vacuum_defer_cleanup_age? Daniel Gustafsson <[email protected]> 2023-04-14 13:46 ` Re: Should we remove vacuum_defer_cleanup_age? Jonathan S. Katz <[email protected]> 2023-04-14 15:25 ` Re: Should we remove vacuum_defer_cleanup_age? Greg Stark <[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