Re: RFC: Logging plan of the running query
On 2023-06-06 03:26, James Coleman wrote: On Mon, Jun 5, 2023 at 4:30 AM torikoshia wrote: On 2023-06-03 02:51, James Coleman wrote: > Hello, > > Thanks for working on this patch! Sure thing! I'm *very interested* in seeing this available, and I think it paves the way for some additional features later on... > On Thu, Dec 8, 2022 at 12:10 AM torikoshia ... > To put it positively: we believe that, for example, catalog accesses > inside CHECK_FOR_INTERRUPTS() -- assuming that the CFI call is inside > an existing valid transaction/query state, as it would be for this > patch -- are safe. If there were problems, then those problems are > likely bugs we already have in other CFI cases. Thanks a lot for the discussion and sharing it! I really appreciate it. BTW I'm not sure whether all the CFI are called in valid transaction, do you think we should check each of them? I kicked off the regressions tests with a call to ProcessLogQueryPlanInterrupt() in every single CHECK_FOR_INTERRUPTS() call. Several hours and 52 GB of logs later I have confirmed that (with the attached revision) at the very least the regression test suite can't trigger any kind of failures regardless of when we trigger this. The existing code in the patch for only running the explain when there's an active query handling that. Thanks for the testing! I've attached v27. The important change here in 0001 is that it guarantees the interrupt handler is re-entrant, since that was a bug exposed by my testing. I've also included 0002 which is only meant for testing (it attempts to log in the plan in every CHECK_FOR_INTERRUPTS() call). When SIGINT is sent during ProcessLogQueryPlanInterrupt(), ProcessLogQueryPlanInterruptActive can remain true. After that, pg_log_query_plan() does nothing but just returns. AFAIU, v26 logs plan for each pg_log_query_plan() even when another pg_log_query_plan() is running, but it doesn't cause errors or critical problem. Considering the problem solved and introduced by v27, I think v26 might be fine. How do you think? Regards, James -- Regards, -- Atsushi Torikoshi NTT DATA CORPORATION
Re: Remove WindowClause PARTITION BY items belonging to redundant pathkeys
On Fri, 9 Jun 2023 at 20:57, Richard Guo wrote: > > On Fri, Jun 9, 2023 at 8:13 AM David Rowley wrote: >> It might be possible to make adjustments in nodeWindowAgg.c to have >> the equality checks come out as true when there is no ORDER BY. >> update_frameheadpos() is one location that would need to be adjusted. >> It would need further study to ensure we don't accidentally break >> anything. I've not done that study, so won't be adjusting the patch >> for now. > > > I'm also not sure if doing that is safe in all cases. Hmm, do you think > we can instead check wc->frameOptions to see if it is the RANGE OFFSET > case in make_pathkeys_for_window(), and decide to not remove or remove > redundant ORDER BY items according to whether it is or not RANGE OFFSET? I think ideally, we'd not have to add special cases to the planner to disable the optimisation for certain cases. I'd much rather see adjustments in the executor to handle cases where we've removed ORDER BY columns (e.g adjust update_frameheadpos() to assume rows are equal when there are no order by columns.) That of course would require that there are no cases where removing ORDER BY columns would change the actual query results. I can't currently think of any reason why the results would change, but I'm not overly familiar with the RANGE option, so I'd need to spend a bit longer looking at it than I have done so far to feel confident in making the patch process ORDER BY columns too. I'm ok with just doing the PARTITION BY stuff as step one. The ORDER BY stuff is more complex and risky which seems like a good reason to tackle separately. David
Re: Let's make PostgreSQL multi-threaded
On Sat, Jun 10, 2023 at 11:32 PM Hannu Krosing wrote: > > On Mon, Jun 5, 2023 at 4:52 PM Heikki Linnakangas wrote: > > > > If there are no major objections, I'm going to update the developer FAQ, > > removing the excuses there for why we don't use threads [1]. > > I think it is not wise to start the wholesale removal of the objections there. > > But I think it is worthwhile to revisit the section about threads and > maybe split out the historic part which is no more true, and provide > both pros and cons for these. > > I started with this short summary from the discussion in this thread, > feel free to expand, argue, fix :) > * is current excuse > -- is counterargument or ack > > As an example, threads are not yet used instead of multiple processes > for backends because: > * Historically, threads were poorly supported and buggy. > -- yes they were, not relevant now when threads are well-supported and > non-buggy > > * An error in one backend can corrupt other backends if they're > threads within a single process > -- still valid for silent corruption > -- for detected crash - yes, but we are restarting all backends in > case of crash anyway. > > * Speed improvements using threads are small compared to the remaining > backend startup time. > -- we now have some measurements that show significant performance > improvements not related to startup time > > * The backend code would be more complex. > -- this is still the case > -- even more worrisome is that all extensions also need to be rewritten > -- and many incompatibilities will be silent and take potentially years to > find > > * Terminating backend processes allows the OS to cleanly and quickly > free all resources, protecting against memory and file descriptor > leaks and making backend shutdown cheaper and faster > -- still true > > * Debugging threaded programs is much harder than debugging worker > processes, and core dumps are much less useful > -- this was countered by claiming that > -- by now we have reasonable debugger support for threads > -- there is no direct debugger support for debugging the exact > system set up like PostgreSQL processes + shared memory > > * Sharing of read-only executable mappings and the use of > shared_buffers means processes, like threads, are very memory > efficient > -- this seems to say that the current process model is as good as threads ? > -- there were a few counterarguments > -- per-backend virtual memory mapping can add up to significant > amount of extra RAM usage > -- the discussion did not yet touch various per-backend caches > (pg_catalog cache, statement cache) which are arguably easier to > implement in threaded model > -- TLB reload at each process switch is expensive and would be > mostly avoided in case of threads I think it is worth mentioning that parallel worker infrastructure will be simplified with threaded models e.g. 'parallel query', and 'parallel vacuum'. -- Regards, Dilip Kumar EnterpriseDB: http://www.enterprisedb.com
Re: Wrong results from Parallel Hash Full Join
Michael Paquier writes: > This has fallen through the cracks and conchuela has failed again > today, so I went ahead and applied the fix on HEAD. Thanks! Thanks! I'd intended to push that but it didn't get to the top of the to-do queue yet. (I'm still kind of wondering why only conchuela has failed to date.) regards, tom lane
Re: Wrong results from Parallel Hash Full Join
On Wed, Jun 07, 2023 at 05:16:12PM -0400, Melanie Plageman wrote: > On Fri, May 19, 2023 at 8:05 PM Tom Lane wrote: >> Considering that this is a parallel plan, I don't think there's any >> mystery about why an ORDER-BY-less query might have unstable output >> order; the only mystery is why more of the buildfarm hasn't failed. >> Can we just add "ORDER BY t1.id" to this query? It looks like you >> get the same PHJ plan, although now underneath Sort/Gather Merge. > > Yes, this was an oversight on my part. Attached is the patch that does > just what you suggested. Confirmed that adding an ORDER BY adds a Sort node between a Gather Merge and a Parallel Hash Full Join, not removing coverage. This has fallen through the cracks and conchuela has failed again today, so I went ahead and applied the fix on HEAD. Thanks! -- Michael signature.asc Description: PGP signature
Re: ERROR: wrong varnullingrels (b 3) (expected (b)) for Var 2/1
On Sat, Jun 10, 2023 at 12:08 AM Tom Lane wrote: > Richard Guo writes: > > We can identify in which form of identity 3 the plan is built up by > > checking the relids of the B/C join's outer rel. If it's in the first > > form, the outer rel's relids must contain the A/B join. Otherwise it > > should only contain B's relid. So I'm considering that maybe we can > > adjust the nulling bitmap for nestloop parameters according to that. > > Attached is a patch for that. Does this make sense? > > Hmm. I don't really want to do it in identify_current_nestloop_params > because that gets applied to all nestloop params, so it seems like > that risks masking bugs of other kinds. I'd rather do it in > process_subquery_nestloop_params, which we know is only applied to > subquery LATERAL references. So more or less as attached. Yeah, that makes sense. process_subquery_nestloop_params is a better place to do this adjustments. +1 to v2 patch. Thanks Richard
Re: Parallelize correlated subqueries that execute within each worker
On Tue, Jun 6, 2023 at 4:36 AM Richard Guo wrote: > > > On Mon, Jan 23, 2023 at 10:00 PM James Coleman wrote: >> >> Which this patch we do in fact now see (as expected) rels with >> non-empty lateral_relids showing up in generate_[useful_]gather_paths. >> And the partial paths can now have non-empty required outer rels. I'm >> not able to come up with a plan that would actually be caught by those >> checks; I theorize that because of the few places we actually call >> generate_[useful_]gather_paths we are in practice already excluding >> those, but for now I've left these as a conditional rather than an >> assertion because it seems like the kind of guard we'd want to ensure >> those methods are safe. > > > I'm trying to understand this part. AFAICS we will not create partial > paths for a rel, base or join, if it has lateral references. So it > seems to me that in generate_[useful_]gather_paths after we've checked > that there are partial paths, the checks for lateral_relids are not > necessary because lateral_relids should always be empty in this case. > Maybe I'm missing something. At first I was thinking "isn't the point of the patch to generate partial paths for rels with lateral references" given what I'd written back in January, but I added "Assert(bms_is_empty(required_outer));" to both of those functions and the assertion never fails running the tests (including my newly parallelizable queries). I'm almost positive I'd checked this back in January (not only had I'd explicitly written that I'd confirmed we had non-empty lateral_relids there, but also it was the entire based of the alternate approach to the patch), but...I can't go back to 5 months ago and remember what I'd done. Ah! Your comment about "after we've checked that there are partial paths" triggered a thought. I think originally I'd had the "bms_is_subset(required_outer, rel->relids)" check first in these functions. And indeed if I run the tests with that the assertion moved to above the partial paths check, I get failures in generate_useful_gather_paths specifically. Mystery solved! > And while trying the v9 patch I came across a crash with the query > below. > > set min_parallel_table_scan_size to 0; > set parallel_setup_cost to 0; > set parallel_tuple_cost to 0; > > explain (costs off) > select * from pg_description t1 where objoid in > (select objoid from pg_description t2 where t2.description = > t1.description); >QUERY PLAN > > Seq Scan on pg_description t1 >Filter: (SubPlan 1) >SubPlan 1 > -> Gather >Workers Planned: 2 >-> Parallel Seq Scan on pg_description t2 > Filter: (description = t1.description) > (7 rows) > > select * from pg_description t1 where objoid in > (select objoid from pg_description t2 where t2.description = > t1.description); > WARNING: terminating connection because of crash of another server process > > Seems something is wrong when extracting the argument from the Param in > parallel worker. With what I'm trying to change I don't think this plan should ever be generated since it means we'd have to pass a param from the outer seq scan into the parallel subplan, which we can't do (currently). I've attached the full backtrace to the email, but as you hinted at the parallel worker is trying to get the param (in this case detoasting it), but the param doesn't exist on the worker, so it seg faults. Looking at this further I think there's an existing test case that exposes the misplanning here (the one right under the comment "Parallel Append is not to be used when the subpath depends on the outer param" in select_parallel.sql), but it doesn't seg fault because the param is an integer, doesn't need to be detoasted, and therefore (I think) we skate by (but probably with wrong results in depending on the dataset). Interestingly this is one of the existing test queries my original patch approach didn't change, so this gives me something specific to work with improving the path. Thanks for testing this and bringing this to my attention! BTW are you by any chance testing on ARM macOS? I reproduced the issue there, but for some reason I did not reproduce the error (and the plan wasn't parallelized) when I tested this on linux. Perhaps I missed setting something up; it seems odd. > BTW another rebase is needed as it no longer applies to HEAD. Apologies; I'd rebased, but hadn't updated the thread. See attached for an updated series (albeit still broken on your test query). Thanks, James * thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0) * frame #0: 0x00010449cab0 postgres`toast_raw_datum_size(value=0) at detoast.c:550:6 frame #1: 0x000104cf9438 postgres`texteq(fcinfo=0x000112809c80) at varlena.c:1645:10 frame #2: 0x0001047ca7ac postgres`ExecInterpExpr(state=0x0001128097e0,
Re: make_ctags: use -I option to ignore pg_node_attr macro
Hi Sawada-san, > In my Mac environment where non-Exuberant ctags and emacs 28.2 are > installed, the generated etags file cannot be loaded by emacs due to > file format error. The generated TAGS file is: > > % head -10 TAGS > > ) / > sizeof(BlockNumber)sizeof(BlockNumber)117,3750 > my > @newa newa395,10443 > > variadic array[1,2]:array[1,2]56,1803 > > variadic array[]::inarray[]::i72,2331 > > variadic array[array64,2111 > > variadic array[array68, > > variadic array[array76,2441 > (2 * (2 53,1353 > my $fn fn387,10147 > startblock 101,4876 > > Since the etags files consist of multiple sections[1] we cannot sort > the generated etags file. With the attached patch, make_etags (with > non-Exuberant ctags) generates a correct format etags file and it > works: > > % head -10 TAGS > > /Users/masahiko/pgsql/source/postgresql/GNUmakefile,1187 > subdir 7,56 > top_builddir 8,65 > docs:docs13,167 > world-contrib-recurse:world-contrib-recurse19,273 > world-bin-contrib-recurse:world-bin-contrib-recurse24,394 > html man:html man26,444 > install-docs:install-docs29,474 > install-world-contrib-recurse:install-world-contrib-recurse35,604 > > BTW regarding the following comment, as far as I can read the > Wikipedia page for ctags[1], Exuberant ctags file doesn't have a > header section. > > # Exuberant tags has a header that we cannot sort in with the other entries > # so we skip the sort step > # Why are we sorting this? I guess some tag implementation need this, > # particularly for append mode. bjm 2012-02-24 > if [ ! "$IS_EXUBERANT" ] > > Instead, the page says that sorting non-Exuberant tags file allows for > faster searching on of the tags file. I've fixed the comment > accordingly too. > > Regards, > > [1] https://en.wikipedia.org/wiki/Ctags#Etags_2 Sorry for late reply and thanks for the patch! I have confirmed the error with make_etags on my Mac (emacs 28.1 + non-Exuberant ctags), and the error is fixed by your patch. Also I have confirmed the patch does not affect make_etags on my Linux (emacs 26.3 + Exuberant ctags). I will push the fix to REL_15_STABLE and master branch if there's no objection. Best reagards, -- Tatsuo Ishii SRA OSS LLC English: http://www.sraoss.co.jp/index_en/ Japanese:http://www.sraoss.co.jp
RE: Support logical replication of DDLs
On Thur, Jun 8, 2023 20:02 PM shveta malik wrote:
> Thank You Vignesh for handling (a), Ajin for handling (b), Shi-san and
> Hou-san for contributing in (c).
>
> The new changes are in patch 0001, 0002, 0005 and 0008.
Thanks for updating the patch set.
Here are some comments:
===
For 0002 patch.
1. The typo atop the function EventTriggerTableInitWrite.
```
+/*
+ * Fire table_init_rewrite triggers.
+ */
+void
+EventTriggerTableInitWrite(Node *real_create, ObjectAddress address)
```
s/table_init_rewrite/table_init_write
~~~
2. The new process for "SCT_CreateTableAs" in the function
pg_event_trigger_ddl_commands.
With the event trigger created in
test_ddl_deparse_regress/sql/test_ddl_deparse.sql, when creating the table that
already exists with `CreateTableAs` command, an error is raised like below:
```
postgres=# CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class
WHERE relkind = 'r';
postgres=# CREATE TABLE IF NOT EXISTS as_select1 AS SELECT * FROM pg_class
WHERE relkind = 'r';
NOTICE: relation "as_select1" already exists, skipping
ERROR: unrecognized object class: 0
CONTEXT: PL/pgSQL function test_ddl_deparse() line 6 at FOR over SELECT rows
```
It seems that we could check cmd->d.ctas.real_create in the function
pg_event_trigger_ddl_commands and return NULL in this case.
===
For 0004 patch.
3. The command tags that are not supported for deparsing in the tests.
```
FOR r IN SELECT * FROM pg_event_trigger_ddl_commands()
-- Some TABLE commands generate sequence-related commands, also
deparse them.
WHERE command_tag in ('ALTER FOREIGN TABLE', 'ALTER TABLE',
'CREATE FOREIGN
TABLE', 'CREATE TABLE',
'CREATE TABLE AS',
'DROP FOREIGN TABLE',
'DROP TABLE', 'ALTER
SEQUENCE',
'CREATE SEQUENCE',
'DROP SEQUENCE')
```
Since foreign table is not supported yet in the current patch set, it seems that
we need to remove "FOREIGN TABLE" related command tag. If so, I think the
following three files need to be modified:
- test_ddl_deparse_regress/sql/test_ddl_deparse.sql
- test_ddl_deparse_regress/t/001_compare_dumped_results.pl
- test_ddl_deparse_regress/t/002_regress_tests.pl
~~~
4. The different test items between meson and Makefile.
It seems that we should keep the same SQL files and the same order of SQL files
in test_ddl_deparse_regress/meson.build and test_ddl_deparse_regress/Makefile.
===
For 0004 && 0008 patches.
5. The test cases in the test file
test_ddl_deparse_regress/t/001_compare_dumped_results.pl.
```
# load test cases from the regression tests
-my @regress_tests = split /\s+/, $ENV{REGRESS};
+#my @regress_tests = split /\s+/, $ENV{REGRESS};
+my @regress_tests = ("create_type", "create_schema", "create_rule",
"create_index");
```
I think @regress_tests should include all SQL files, instead of just four. BTW,
the old way (using `split /\s+/, $ENV{REGRESS}`) doesn't work in meson.
Regards,
Wang wei
Re: v16 fails to build w/ Visual Studio 2015
On Wed, Jun 07, 2023 at 03:04:16PM -0700, Noah Misch wrote: > On Wed, Jun 07, 2023 at 11:34:09PM +0200, Peter Eisentraut wrote: >> Apparently, nobody has used it between Sat Jul 9 08:52:19 2022 and now? One week close enough. I have run checks on VS 2015 back when working on 6203583, but I don't have this environment at hand anymore. > Essentially. I assume you're referring to commit 964d01a "Automatically > generate node support functions". I bet it actually broke a few days later, > at ff33a8c "Remove artificial restrictions on which node types have out/read > funcs." Note that the last version-dependent checks of _MSC_VER have been removed in the commit I am mentioning above, so the gain in removing VS 2015 is marginal. Even less once src/tools/msvc/ gets removed. But perhaps it makes a few things easier with meson in mind? I don't think that's a reason enough to officially remove support for VS 2015 on 17~ and let it be for v16, though. It seems like my old Windows env was one bug in the Matrix, and I've moved one to newer versions already. -- Michael signature.asc Description: PGP signature
Re: v16 fails to build w/ Visual Studio 2015
On Wed, Jun 07, 2023 at 11:35:34PM +0200, Peter Eisentraut wrote: > I kind of like the style where there is only one return at the end, because > it makes it easier to inject debugging code that inspects the return value. I kind of disagree here, the previous style is a bit ugly-ish, with the code generated by gen_node_support.pl being dependent on this local call because it is necessary to know about return_value: - if (false) - ; #include "readfuncs.switch.c" So +1 for what's proposed. -- Michael signature.asc Description: PGP signature
Re: [PATCH] hstore: Fix parsing on Mac OS X: isspace() is locale specific
On Tue, Jun 06, 2023 at 10:16:09AM -0400, Evan Jones wrote: > I did a quick look at the places found with "git grep isspace" yesterday. I > agree with the comment from commit 9ae2661: "I've left alone isspace() > calls in places that aren't really expecting any non-ASCII input > characters, such as float8in()." There are a number of other calls where I > think it would likely be safe, and possibly even a good idea, to replace > isspace() with scanner_isspace(). However, I couldn't find any where I > could cause a bug like the one I hit in hstore parsing. Yes, I agree with this feeling. Like 9ae2661, I can't get really excited about plastering more of that, especially if it were for timezone value input or dictionary options. One area with a new isspace() since 2017 is multirangetypes.c, but it is just a copy of rangetypes.c. > Original mailing list post for commit 9ae2661 in case it is helpful for > others: https://www.postgresql.org/message-id/10129.1495302...@sss.pgh.pa.us I have reproduced the original problem reported on macOS 13.4, which is close to the top of what's available. Passing to pg_regress some options to use something else than UTF-8 leads to a failure in the tests, so we need a split like fussyztrmatch to test that: REGRESS_OPTS='--encoding=SQL_ASCII --no-locale' make check An other error pattern without a split could be found on Windows, as of: select E'key\u0105=>value'::hstore; - hstore -- - "keyÄ…"=>"value" -(1 row) - +ERROR: character with byte sequence 0xc4 0x85 in encoding "UTF8" has no equivalent in encoding "WIN1252" +LINE 1: select E'key\u0105=>value'::hstore; We don't do that for unaccent, actually, leading to similar failures.. I'll launch a separate thread about that shortly. With that fixed, the fix has been applied and backpatched. Thanks for the report, Evan! -- Michael signature.asc Description: PGP signature
Re: [PATCH] pg_regress.c: Fix "make check" on Mac OS X: Pass DYLD_LIBRARY_PATH
On Tue, Jun 06, 2023 at 09:48:24PM -0400, Tom Lane wrote: > configure with --enable-tap-tests, then do "make check-world". > > Also, adding certain additional feature arguments such as > --with-python enables more test cases. We aren't going to be > super excited about a patch that doesn't handle all of them. There is a bit more to this story. Mainly, see PG_TEST_EXTRA here: https://www.postgresql.org/docs/devel/regress-run.html -- Michael signature.asc Description: PGP signature
Re: check_strxfrm_bug()
On Mon, Apr 17, 2023 at 8:00 AM Thomas Munro wrote: > On Sun, Dec 18, 2022 at 10:27 AM Thomas Munro wrote: > > With my garbage collector hat on, that made me wonder if there was > > some more potential cleanup here: could we require locale_t yet? The > > last straggler systems on our target OS list to add the POSIX locale_t > > stuff were Solaris 11.4 (2018) and OpenBSD 6.2 (2018). Apparently > > it's still too soon: we have two EOL'd OSes in the farm that are older > > than that. But here's an interesting fact about wrasse, assuming its > > host is gcc211: it looks like it can't even apply further OS updates > > because the hardware[1] is so old that Solaris doesn't support it > > anymore[2]. > > For the record, now the OpenBSD machines have been upgraded, so now > "wrasse" is the last relevant computer on earth with no POSIX > locale_t. Unfortunately there is no reason to think it's going to go > away soon, so I'm just noting this fact here as a reminder for when it > eventually does... Since talk of server threads erupted again, I just wanted to note that this system locale API stuff would be on the long list of micro-obstacles. You'd *have* to use the locale_t-based interfaces (or come up with replacements using a big ugly lock to serialise all access to the process-global locale, or allow only ICU locale support in that build, but those seem like strange lengths to go to just to support a dead version of Solaris). There are still at least a couple of functions that lack XXX_l variants in the standard: mbstowcs() and wcstombs() (though we use the non-standard _l variants if we find them in ), but that's OK because we use uselocale() and not setlocale(), because uselocale() is required to be thread-local. The use of setlocale() to set up the per-backend/per-database default locale would have to be replaced with uselocale(). In other words, I think wrasse would not be coming with us on this hypothetical quest.
Re: Do we want a hashset type?
On Sun, Jun 11, 2023, at 17:03, Tomas Vondra wrote: > On 6/11/23 12:26, Joel Jacobson wrote: >> I think there are some good arguments that speaks in favour of including it >> in core: ... > > I agree with all of that, but ... > > It's just past feature freeze, so the earliest release this could appear > in is 17, about 15 months away. > > Once stuff gets added to core, it's tied to the release cycle, so no new > features in between. > > Presumably people would like to use the extension in the release they > already use, without backporting. > > Postgres is extensible for a reason, exactly so that we don't need to > have everything in core. Interesting, I've never thought about this one before: What if something is deemed to be fundamental and therefore qualify for core inclusion, and at the same time is suitable to be made an extension. Would it be possible to ship such extension as pre-installed? What was the json/jsonb story, was it ever an extension before being included in core? /Joel
Re: Do we want a hashset type?
On Sun, Jun 11, 2023, at 16:58, Andrew Dunstan wrote: >>On 2023-06-11 Su 06:26, Joel Jacobson wrote: >>Perhaps "set" would have been a better name, since the use of hash functions >>from an end-user perspective is >>implementation details, but we cannot use >>that word since it's a reserved keyword, hence "hashset". > >Rather than use "hashset", which as you say is based on an implementation >detail, I would prefer something like >"integer_set" - what it's a set of. Apologies for the confusion previously. Upon further reflection, I recognize that the term "hash" in "hashset" extends beyond mere representation of implementation. It imparts key information about performance characteristics as well as functionality inherent to the set. In hindsight, "hashset" does emerge as the most suitable terminology. /Joel
Re: Do we want a hashset type?
On 6/11/23 12:26, Joel Jacobson wrote: > On Sat, Jun 10, 2023, at 22:26, Tomas Vondra wrote: >> On 6/10/23 17:46, Andrew Dunstan wrote: >>> >>> Maybe you can post a full patch as well as incremental? >>> >> >> I wonder if we should keep discussing this extension here, considering >> it's going to be out of core (at least for now). Not sure how many >> pgsql-hackers are interested in this, so maybe we should just move it to >> github PRs or something ... > > I think there are some good arguments that speaks in favour of including it > in core: > > 1. It's a fundamental data structure. Perhaps "set" would have been a better > name, > since the use of hash functions from an end-user perspective is implementation > details, but we cannot use that word since it's a reserved keyword, hence > "hashset". > > 2. The addition of SQL/PGQ in SQL:2023 is evidence of a general perceived need > among SQL users to evaluate graph queries. Even if a future implementation of > SQL/PGQ > would mean users wouldn't need to deal with the hashset type directly, the > same > type could hopefully be used, in part or in whole, under the hood by the > future > SQL/PGQ implementation. If low-level functionality is useful on its own, I > think it's > a benefit of exposing it to users, since it allows system testing of the > component > in isolation, even if it's primarily gonna be used as a smaller part of a > larger more > high-level component. > > 3. I think there is a general need for hashset, experienced by myself, Andrew > and > I would guess lots of others users. The general pattern that will be improved > is > when you currently would do array_agg(DISTINCT ...) > probably there are other situations too, since it's a fundamental data > structure. > I agree with all of that, but ... It's just past feature freeze, so the earliest release this could appear in is 17, about 15 months away. Once stuff gets added to core, it's tied to the release cycle, so no new features in between. Presumably people would like to use the extension in the release they already use, without backporting. Postgres is extensible for a reason, exactly so that we don't need to have everything in core. > On Sat, Jun 10, 2023, at 22:12, Tomas Vondra wrote: 3) support for other types (now it only works with int32) >> I think we should decide what types we want/need to support, and add one >> or two types early. Otherwise we'll have code / on-disk format making >> various assumptions about the type length etc. >> >> I have no idea what types people use as node IDs - is it likely we'll >> need to support types passed by reference / varlena types? Or can we >> just assume it's int/bigint? > > I think we should just support data types that would be sensible > to use as a PRIMARY KEY in a fully normalised data model, > which I believe would only include "int", "bigint" and "uuid". > OK, makes sense. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Do we want a hashset type?
On 2023-06-11 Su 06:26, Joel Jacobson wrote: On Sat, Jun 10, 2023, at 22:26, Tomas Vondra wrote: On 6/10/23 17:46, Andrew Dunstan wrote: Maybe you can post a full patch as well as incremental? I wonder if we should keep discussing this extension here, considering it's going to be out of core (at least for now). Not sure how many pgsql-hackers are interested in this, so maybe we should just move it to github PRs or something ... I think there are some good arguments that speaks in favour of including it in core: 1. It's a fundamental data structure. That's reason enough IMNSHO. Perhaps "set" would have been a better name, since the use of hash functions from an end-user perspective is implementation details, but we cannot use that word since it's a reserved keyword, hence "hashset". Rather than use "hashset", which as you say is based on an implementation detail, I would prefer something like "integer_set" - what it's a set of. cheers andrew -- Andrew Dunstan EDB:https://www.enterprisedb.com
Should heapam_estimate_rel_size consider fillfactor?
Hi,
While testing some stuff, I noticed heapam_estimate_rel_size (or rather
table_block_relation_estimate_size it calls) ignores fillfactor, so that
for a table without statistics it ends up with reltuple estimate much
higher than reality. For example, with fillfactor=10 the estimate is
about 10x higher.
I ran into this while doing some tests with hash indexes, where I use
fillfactor to make the table look bigger (as if the tuples were wider),
and I ran into this:
drop table hash_test;
create table hash_test (a int, b text) with (fillfactor=10);
insert into hash_test select 1 + ((i - 1) / 1), md5(i::text)
from generate_series(1, 100) s(i);
-- analyze hash_test;
create index hash_test_idx on hash_test using hash (a);
select pg_size_pretty(pg_relation_size('hash_test_idx'));
If you run it like this (without the analyze), the index will be 339MB.
With the analyze, it's 47MB.
This only happens if there are no relpages/reltuples statistics yet, in
which case table_block_relation_estimate_size estimates density from
tuple width etc.
So it seems the easiest "fix" is to do ANALYZE before creating the index
(and not after it, as I had in my scripts). But I wonder how many people
fail to realize this - it sure took me a while to realize the indexes
are too large and even longer what is causing it. I wouldn't be very
surprised if many people had bloated hash indexes after bulk loads.
So maybe we should make table_block_relation_estimate_size smarter to
also consider the fillfactor in the "no statistics" branch, per the
attached patch.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Companydiff --git a/src/backend/access/table/tableam.c b/src/backend/access/table/tableam.c
index a5e6c92f35..3a26846f01 100644
--- a/src/backend/access/table/tableam.c
+++ b/src/backend/access/table/tableam.c
@@ -737,11 +737,19 @@ table_block_relation_estimate_size(Relation rel, int32 *attr_widths,
* and (c) different table AMs might use different padding schemes.
*/
int32 tuple_width;
+ int fillfactor;
+
+ /*
+ * Without reltuples/relpages, we also need to consider fillfactor.
+ * The other branch considers it implicitly by calculating density
+ * from actual relpages/reltuples statistics.
+ */
+ fillfactor = RelationGetFillFactor(rel, HEAP_DEFAULT_FILLFACTOR);
tuple_width = get_rel_data_width(rel, attr_widths);
tuple_width += overhead_bytes_per_tuple;
/* note: integer division is intentional here */
- density = usable_bytes_per_page / tuple_width;
+ density = (usable_bytes_per_page * fillfactor / 100) / tuple_width;
}
*tuples = rint(density * (double) curpages);
Re: Do we want a hashset type?
On Sat, Jun 10, 2023, at 22:26, Tomas Vondra wrote: > On 6/10/23 17:46, Andrew Dunstan wrote: >> >> Maybe you can post a full patch as well as incremental? >> > > I wonder if we should keep discussing this extension here, considering > it's going to be out of core (at least for now). Not sure how many > pgsql-hackers are interested in this, so maybe we should just move it to > github PRs or something ... I think there are some good arguments that speaks in favour of including it in core: 1. It's a fundamental data structure. Perhaps "set" would have been a better name, since the use of hash functions from an end-user perspective is implementation details, but we cannot use that word since it's a reserved keyword, hence "hashset". 2. The addition of SQL/PGQ in SQL:2023 is evidence of a general perceived need among SQL users to evaluate graph queries. Even if a future implementation of SQL/PGQ would mean users wouldn't need to deal with the hashset type directly, the same type could hopefully be used, in part or in whole, under the hood by the future SQL/PGQ implementation. If low-level functionality is useful on its own, I think it's a benefit of exposing it to users, since it allows system testing of the component in isolation, even if it's primarily gonna be used as a smaller part of a larger more high-level component. 3. I think there is a general need for hashset, experienced by myself, Andrew and I would guess lots of others users. The general pattern that will be improved is when you currently would do array_agg(DISTINCT ...) probably there are other situations too, since it's a fundamental data structure. On Sat, Jun 10, 2023, at 22:12, Tomas Vondra wrote: >>> 3) support for other types (now it only works with int32) > I think we should decide what types we want/need to support, and add one > or two types early. Otherwise we'll have code / on-disk format making > various assumptions about the type length etc. > > I have no idea what types people use as node IDs - is it likely we'll > need to support types passed by reference / varlena types? Or can we > just assume it's int/bigint? I think we should just support data types that would be sensible to use as a PRIMARY KEY in a fully normalised data model, which I believe would only include "int", "bigint" and "uuid". /Joel
