Re: pg_stat_statements and "IN" conditions
> On Mon, Apr 15, 2024 at 06:09:29PM +0900, Sutou Kouhei wrote:
>
> Thanks. I'm not familiar with this code base but I've
> reviewed these patches because I'm interested in this
> feature too.
Thanks for the review! The commentaries for the first patch make sense
to me, will apply.
> 0003:
>
> > diff --git a/contrib/pg_stat_statements/pg_stat_statements.c
> > b/contrib/pg_stat_statements/pg_stat_statements.c
> > index d7841b51cc9..00eec30feb1 100644
> > --- a/contrib/pg_stat_statements/pg_stat_statements.c
> > +++ b/contrib/pg_stat_statements/pg_stat_statements.c
> > ...
> > @@ -2883,12 +2886,22 @@ generate_normalized_query(JumbleState *jstate,
> > const char *query,
> > /* The firsts merged constant */
> > else if (!skip)
> > {
> > + static const uint32 powers_of_ten[] = {
> > + 1, 10, 100,
> > + 1000, 1, 10,
> > + 100, 1000, 1,
> > + 10
> > + };
> > + int lower_merged = powers_of_ten[magnitude - 1];
> > + int upper_merged = powers_of_ten[magnitude];
>
> How about adding a reverse function of decimalLength32() to
> numutils.h and use it here?
I was pondering that at some point, but eventually decided to keep it
this way, because:
* the use case is quite specific, I can't image it's being used anywhere
else
* it would not be strictly reverse, as the transformation itself is not
reversible in the strict sense
> > - n_quer_loc += sprintf(norm_query + n_quer_loc, "...");
> > + n_quer_loc += sprintf(norm_query + n_quer_loc, "...
> > [%d-%d entries]",
> > + lower_merged,
> > upper_merged - 1);
>
> Do we still have enough space in norm_query for this change?
> It seems that norm_query expects up to 10 additional bytes
> per jstate->clocations[i].
As far as I understand there should be enough space, because we're going
to replace at least 10 constants with one merge record. But it's a good
point, this should be called out in the commentary explaining why 10
additional bytes are added.
> It seems that we can merge 0001, 0003 and 0004 to one patch.
> (Sorry. I haven't read all discussions yet. If we already
> discuss this, sorry for this noise.)
There is a certain disagreement about which portion of this feature
makes sense to go with first, thus I think keeping all options open is a
good idea. In the end a committer can squash the patches if needed.
Identify huge pages accessibility using madvise
Hi,
I would like to propose a small patch to address an annoying issue with
the way how PostgreSQL does fallback in case if "huge_pages = try" is
set. Here is how the problem looks like:
* PostgreSQL is starting on a machine with some huge pages available
* It tries to identify that fact and does mmap with MAP_HUGETLB, which
succeeds
* But it has a pleasure to run inside a cgroup with a hugetlb
controller and limits set to 0 (or anything less than PostgreSQL
needs)
* Under this circumstances PostgreSQL will proceed allocating huge
pages, but the first page fault will trigger SIGBUS
I've sketched out how to reproduce it with cgroup v1 and v2 in the
attached scripts.
This sounds like quite a rare combination of factors, but apparently
it's fairly easy to face this on K8s/OpenShift. There was a bug reported
some time ago [1] about this behaviour, and back then I was under the
impression it's a solved matter with nothing to do. Yet I still observe
this type of issues, the latest one not longer than a week ago.
After some research I found what looks to me like a relatively simple
way to address the problem. In Linux kernel 5.14 a new flag to madvise
was introduced that might be just what we need here. It's called
MADV_POPULATE_READ [2] and it tells kernel to populate page tables by
triggering read faults if required. One by-design feature of this flag
is to fail the madvise call in the situations like one above, giving an
opportunity to avoid SIGBUS.
I've outlined a patch to implement this approach and tested it on a
newish Linux kernel I've got lying around (6.9.0-rc1) -- no SIGBUS,
PostgreSQL does fallback to not use huge pages. The resulting change
seems to be small enough to justify addressing this small but annoying
issue. Any thoughts or commentaries about the proposal?
[1]:
https://www.postgresql.org/message-id/flat/HE1PR0701MB256920EEAA3B2A9C06249F339E110%40HE1PR0701MB2569.eurprd07.prod.outlook.com
[2]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4ca9b3859dac14bbef0c27d00667bb5b10917adb
>From 0001d43117dc5cad08fb0908a3e50a00c56f88d3 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 13 Apr 2024 11:31:46 +0200
Subject: [PATCH v1] Identify huge pages accesibility using madvise
Currently, PostgreSQL tries to figure out whether huge pages are
available, to fallback if "huge_pages = try" is set. There is an
annoying situation that this approach cannot handle, when there are huge
pages available, but they are restricted via cgroups. If this happens
and PostgreSQL is running inside a cgroup that limits on huge pages to
0, the allocation part with mmap would work, but the very first page
fault will return SIGBUS.
To handle this situation more gracefully, add madvise call with
MADV_POPULATE_READ flag if available (it was introduced in Linux kernel
5.14). This flag tells kernel to populate page tables by triggering read
faults if required, and in the situation described above it will fail,
giving PostgreSQL an opportunity to fallback and proceed without huge
pages. Note that it's not a side effect, but rather a designed behaviour [1].
[1]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4ca9b3859dac14bbef0c27d00667bb5b10917adb
---
src/backend/port/sysv_shmem.c | 30 --
1 file changed, 28 insertions(+), 2 deletions(-)
diff --git a/src/backend/port/sysv_shmem.c b/src/backend/port/sysv_shmem.c
index 1a6d8fa0fb..cbacf62066 100644
--- a/src/backend/port/sysv_shmem.c
+++ b/src/backend/port/sysv_shmem.c
@@ -600,7 +600,7 @@ CreateAnonymousSegment(Size *size)
{
Sizeallocsize = *size;
void *ptr = MAP_FAILED;
- int mmap_errno = 0;
+ int mmap_errno = 0, madv_errno = 0;
#ifndef MAP_HUGETLB
/* PGSharedMemoryCreate should have dealt with this case */
@@ -625,6 +625,28 @@ CreateAnonymousSegment(Size *size)
if (huge_pages == HUGE_PAGES_TRY && ptr == MAP_FAILED)
elog(DEBUG1, "mmap(%zu) with MAP_HUGETLB failed, huge
pages disabled: %m",
allocsize);
+
+#ifdef MADV_POPULATE_READ
+ /*
+* Verifying if huge pages are available is done in two steps:
first
+* mmap with MAP_HUGETLB, then madvise with MADV_POPULATE_READ.
For the
+* latter the MADV_POPULATE_READ flag will tell kernel to
populate page
+* tables by triggering read faults if required, revealing
potential
+* access issues that otherwise would result in SIGBUS.
+*
+* If mmap fails, no huge pages are available; if it does not,
there is
+* still possibility that huge pages are limited via cgroups. If
+* madvise fails, there are some huge pages, but we cannot
access them
+* due to cgroup
Re: broken JIT support on Fedora 40
> On Wed, Apr 10, 2024 at 12:43:23PM +1200, Thomas Munro wrote: > On Tue, Apr 9, 2024 at 10:05 PM Dmitry Dolgov <9erthali...@gmail.com> wrote: > > + /* In assertion builds, run the LLVM verify pass. */ > > +#ifdef USE_ASSERT_CHECKING > > + LLVMPassBuilderOptionsSetVerifyEach(options, true); > > +#endif > > Thanks, that seems nicer. I think the question is whether it will > slow down build farm/CI/local meson test runs to a degree that exceeds > its value. Another option would be to have some other opt-in macro, > like the existing #ifdef LLVM_PASS_DEBUG, for people who maintain > JIT-related stuff to turn on. Oh, I see. I'm afraid I don't have enough knowledge of the CI pipeline, but at least locally for me installcheck became only few percent slower with the verify pass. > Supposing we go with USE_ASSERT_CHECKING, I have another question: > > - const char *nm = "llvm.lifetime.end.p0i8"; > + const char *nm = "llvm.lifetime.end.p0"; > > Was that a mistake, or did the mangling rules change in some version? I'm not sure, but inclined to think it's the same as with noundef -- a mistake, which was revealed in some recent version of LLVM. From what I understand the suffix i8 indicates an overloaded argument of that type, which is probably not needed. At the same time I can't get this error from the verify pass with LLVM-12 or LLVM-15 (I have those at hand by accident). To make it even more confusing I've found a few similar examples in other projects, where this was really triggered by an issue in LLVM [1]. [1]: https://github.com/rust-lang/rust/issues/102738
Re: broken JIT support on Fedora 40
> On Tue, Apr 09, 2024 at 07:07:58PM +1200, Thomas Munro wrote: > On Sat, Apr 6, 2024 at 5:01 AM Dmitry Dolgov <9erthali...@gmail.com> wrote: > > > > Yep, I think this is it. I've spent some hours trying to understand why > > > > suddenly deform function has noundef ret attribute, when it shouldn't -- > > > > this explains it and the proposed change fixes the crash. One thing that > > > > is still not clear to me though is why this copied attribute doesn't > > > > show up in the bitcode dumped right before running inline pass (I've > > > > added this to troubleshoot the issue). > > > > > > One thing to consider in this context is indeed adding "verify" pass as > > > suggested in the PR, at least for the debugging configuration. Without > > > the fix > > > it immediately returns: > > > > > > Running analysis: VerifierAnalysis on deform_0_1 > > > Attribute 'noundef' applied to incompatible type! > > > > > > llvm error: Broken function found, compilation aborted! > > > > Here is what I have in mind. Interestingly enough, it also shows few > > more errors besides "noundef": > > > > Intrinsic name not mangled correctly for type arguments! Should be: > > llvm.lifetime.end.p0 > > ptr @llvm.lifetime.end.p0i8 > > > > It refers to the function from create_LifetimeEnd, not sure how > > important is this. > > Would it be too slow to run the verify pass always, in assertion > builds? Here's a patch for the original issue, and a patch to try > that idea + a fix for that other complaint it spits out. The latter > would only run for LLVM 17+, but that seems OK. Sounds like a good idea. About the overhead, I've done a quick test on the reproducer at hands, doing explain analyze in a tight loop and fetching "optimization" timinigs. It gives quite visible difference 96ms p99 with verify vs 46ms p99 without verify (and a rather low stddev, ~1.5ms). But I can imagine it's acceptable for a build with assertions. Btw, I've found there is a C-api for this exposed, which produces the same warnings for me. Maybe it would be even better this way: /** * Toggle adding the VerifierPass for the PassBuilder, ensuring all functions * inside the module is valid. */ void LLVMPassBuilderOptionsSetVerifyEach(LLVMPassBuilderOptionsRef Options, LLVMBool VerifyEach); + /* In assertion builds, run the LLVM verify pass. */ +#ifdef USE_ASSERT_CHECKING + LLVMPassBuilderOptionsSetVerifyEach(options, true); +#endif
Re: broken JIT support on Fedora 40
> On Fri, Apr 05, 2024 at 03:50:50PM +0200, Dmitry Dolgov wrote: > > On Fri, Apr 05, 2024 at 03:21:06PM +0200, Dmitry Dolgov wrote: > > > On Sat, Apr 06, 2024 at 02:00:38AM +1300, Thomas Munro wrote: > > > On Sun, Mar 31, 2024 at 12:49 PM Thomas Munro > > > wrote: > > > > https://github.com/llvm/llvm-project/pull/87093 > > > > > > Oh, with those clues, I think I might see... It is a bit strange that > > > we copy attributes from AttributeTemplate(), a function that returns > > > Datum, to our void deform function. It works (I mean doesn't crash) > > > if you just comment this line out: > > > > > > llvm_copy_attributes(AttributeTemplate, v_deform_fn); > > > > > > ... but I guess that disables inlining of the deform function? So > > > perhaps we just need to teach that thing not to try to copy the return > > > value's attributes, which also seems to work here: > > > > Yep, I think this is it. I've spent some hours trying to understand why > > suddenly deform function has noundef ret attribute, when it shouldn't -- > > this explains it and the proposed change fixes the crash. One thing that > > is still not clear to me though is why this copied attribute doesn't > > show up in the bitcode dumped right before running inline pass (I've > > added this to troubleshoot the issue). > > One thing to consider in this context is indeed adding "verify" pass as > suggested in the PR, at least for the debugging configuration. Without the fix > it immediately returns: > > Running analysis: VerifierAnalysis on deform_0_1 > Attribute 'noundef' applied to incompatible type! > > llvm error: Broken function found, compilation aborted! Here is what I have in mind. Interestingly enough, it also shows few more errors besides "noundef": Intrinsic name not mangled correctly for type arguments! Should be: llvm.lifetime.end.p0 ptr @llvm.lifetime.end.p0i8 It refers to the function from create_LifetimeEnd, not sure how important is this. >From 7a05bd48a4270998a08e63e07cdf1cb9932bfddd Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Fri, 5 Apr 2024 17:52:06 +0200 Subject: [PATCH v1] Add jit_verify_bitcode When passing LLVM IR through optimizer, for troubleshooting purposes it's useful to apply the "verify" pass as well. It can catch an invalid IR, if such was produced by mistake, and output a meaningful message about the error, e.g.: Attribute 'noundef' applied to incompatible type! ptr @deform_0_1 Control this via jit_verify_bitcode development guc. --- doc/src/sgml/config.sgml| 19 +++ src/backend/jit/jit.c | 1 + src/backend/jit/llvm/llvmjit.c | 11 --- src/backend/utils/misc/guc_tables.c | 11 +++ src/include/jit/jit.h | 2 +- 5 files changed, 40 insertions(+), 4 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 624518e0b01..5f07ae099f4 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -11931,6 +11931,25 @@ LOG: CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1) + + jit_verify_bitcode (boolean) + + jit_verify_bitcode configuration parameter + + + + +Adds a verify pass to the pipeline, when optimizing +generated LLVM IR. It helps to identify +cases when an invalid IR was generated due to errors, and is only +useful for working on the internals of the JIT implementation. +The default setting is off. +Only superusers and users with the appropriate SET +privilege can change this setting. + + + + jit_expressions (boolean) diff --git a/src/backend/jit/jit.c b/src/backend/jit/jit.c index 815b58f33c5..ad94e4b55ee 100644 --- a/src/backend/jit/jit.c +++ b/src/backend/jit/jit.c @@ -39,6 +39,7 @@ bool jit_tuple_deforming = true; double jit_above_cost = 10; double jit_inline_above_cost = 50; double jit_optimize_above_cost = 50; +bool jit_verify_bitcode = false; static JitProviderCallbacks provider; static bool provider_successfully_loaded = false; diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c index ec0fdd49324..a4b21abb83a 100644 --- a/src/backend/jit/llvm/llvmjit.c +++ b/src/backend/jit/llvm/llvmjit.c @@ -698,12 +698,17 @@ llvm_optimize_module(LLVMJitContext *context, LLVMModuleRef module) #else LLVMPassBuilderOptionsRef options; LLVMErrorRef err; - const char *passes; + const char *passes, *intermediate_passes; if (context->base.flags & PGJIT_OPT3) - passes = "default"; + intermediate_passes = "default"; else
Re: broken JIT support on Fedora 40
> On Fri, Apr 05, 2024 at 03:21:06PM +0200, Dmitry Dolgov wrote: > > On Sat, Apr 06, 2024 at 02:00:38AM +1300, Thomas Munro wrote: > > On Sun, Mar 31, 2024 at 12:49 PM Thomas Munro > > wrote: > > > https://github.com/llvm/llvm-project/pull/87093 > > > > Oh, with those clues, I think I might see... It is a bit strange that > > we copy attributes from AttributeTemplate(), a function that returns > > Datum, to our void deform function. It works (I mean doesn't crash) > > if you just comment this line out: > > > > llvm_copy_attributes(AttributeTemplate, v_deform_fn); > > > > ... but I guess that disables inlining of the deform function? So > > perhaps we just need to teach that thing not to try to copy the return > > value's attributes, which also seems to work here: > > Yep, I think this is it. I've spent some hours trying to understand why > suddenly deform function has noundef ret attribute, when it shouldn't -- > this explains it and the proposed change fixes the crash. One thing that > is still not clear to me though is why this copied attribute doesn't > show up in the bitcode dumped right before running inline pass (I've > added this to troubleshoot the issue). One thing to consider in this context is indeed adding "verify" pass as suggested in the PR, at least for the debugging configuration. Without the fix it immediately returns: Running analysis: VerifierAnalysis on deform_0_1 Attribute 'noundef' applied to incompatible type! llvm error: Broken function found, compilation aborted!
Re: broken JIT support on Fedora 40
> On Sat, Apr 06, 2024 at 02:00:38AM +1300, Thomas Munro wrote: > On Sun, Mar 31, 2024 at 12:49 PM Thomas Munro wrote: > > https://github.com/llvm/llvm-project/pull/87093 > > Oh, with those clues, I think I might see... It is a bit strange that > we copy attributes from AttributeTemplate(), a function that returns > Datum, to our void deform function. It works (I mean doesn't crash) > if you just comment this line out: > > llvm_copy_attributes(AttributeTemplate, v_deform_fn); > > ... but I guess that disables inlining of the deform function? So > perhaps we just need to teach that thing not to try to copy the return > value's attributes, which also seems to work here: Yep, I think this is it. I've spent some hours trying to understand why suddenly deform function has noundef ret attribute, when it shouldn't -- this explains it and the proposed change fixes the crash. One thing that is still not clear to me though is why this copied attribute doesn't show up in the bitcode dumped right before running inline pass (I've added this to troubleshoot the issue).
Re: pg_stat_statements and "IN" conditions
> On Wed, Mar 27, 2024 at 08:56:12AM +0900, Yasuo Honda wrote: > Thanks for the useful info. > > Ruby on Rails uses bigint as a default data type for the primary key > and prepared statements have been enabled by default for PostgreSQL. > I'm looking forward to these current patches being merged as a first > step and future versions of pg_stat_statements will support > normalizing bigint and prepared statements. Here is the rebased version. In the meantime I'm going to experiment with how to support more use cases in a way that will be more acceptable for the community. >From 21d6e88cc9594745e3b88938be1547aa526b2a29 Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Wed, 3 Apr 2024 20:02:51 +0200 Subject: [PATCH v19 1/4] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on the number of parameters, because every element of ArrayExpr is jumbled. In certain situations it's undesirable, especially if the list becomes too large. Make an array of Const expressions contribute only the first/last elements to the jumble hash. Allow to enable this behavior via the new pg_stat_statements parameter query_id_const_merge with the default value off. Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane, Michael Paquier, Sergei Kornilov, Alvaro Herrera, David Geier Tested-by: Chengxi Sun --- contrib/pg_stat_statements/Makefile | 2 +- .../pg_stat_statements/expected/merging.out | 167 ++ contrib/pg_stat_statements/meson.build| 1 + .../pg_stat_statements/pg_stat_statements.c | 62 ++- contrib/pg_stat_statements/sql/merging.sql| 58 ++ doc/src/sgml/pgstatstatements.sgml| 57 +- src/backend/nodes/gen_node_support.pl | 21 ++- src/backend/nodes/queryjumblefuncs.c | 100 ++- src/backend/postmaster/launch_backend.c | 3 + src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/nodes/primnodes.h | 2 +- src/include/nodes/queryjumble.h | 9 +- 12 files changed, 458 insertions(+), 25 deletions(-) create mode 100644 contrib/pg_stat_statements/expected/merging.out create mode 100644 contrib/pg_stat_statements/sql/merging.sql diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile index 414a30856e4..03a62d685f3 100644 --- a/contrib/pg_stat_statements/Makefile +++ b/contrib/pg_stat_statements/Makefile @@ -19,7 +19,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS)) REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf REGRESS = select dml cursors utility level_tracking planning \ - user_activity wal entry_timestamp cleanup oldextversions + user_activity wal entry_timestamp cleanup oldextversions merging # Disabled because these tests require "shared_preload_libraries=pg_stat_statements", # which typical installcheck users do not have (e.g. buildfarm clients). NO_INSTALLCHECK = 1 diff --git a/contrib/pg_stat_statements/expected/merging.out b/contrib/pg_stat_statements/expected/merging.out new file mode 100644 index 000..1e58283afe8 --- /dev/null +++ b/contrib/pg_stat_statements/expected/merging.out @@ -0,0 +1,167 @@ +-- +-- Const merging functionality +-- +CREATE EXTENSION pg_stat_statements; +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- No merging is performed, as a baseline result +SELECT pg_stat_statements_reset() IS NOT NULL AS t; + t +--- + t +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; +query| calls +-+--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) | 1 + SELECT pg_stat_statements_reset() IS NOT NULL AS t | 1 +(4 rows) + +-- Normal scenario, too many simple constants for an IN query +SET pg_stat_statements.query_id_const_merge = on; +SELECT pg_stat_statements_reset() IS NOT NULL AS t; + t +--- + t +(1 row) + +SELECT * FROM test_merge WHERE id IN (1); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements
Re: broken JIT support on Fedora 40
> On Sat, Mar 30, 2024 at 04:38:11PM +1300, Thomas Munro wrote:
> On Fri, Mar 22, 2024 at 7:15 AM Dmitry Dolgov <9erthali...@gmail.com> wrote:
> > > For verification, I've modified the deform.outblock to call LLVMBuildRet
> > > instead of LLVMBuildRetVoid and this seems to help -- inline and deform
> > > stages are still performed as before, but nothing crashes. But of course
> > > it doesn't sound right that inlining pass cannot process such code.
>
> Thanks for investigating and filing the issue. It doesn't seem to be
> moving yet. Do you want to share the LLVMBuildRet() workaround?
> Maybe we need to consider shipping something like that in the
> meantime?
Yeah, sorry, I'm a bit baffled about this situation myself. Yesterday
I've opened a one-line PR fix that should address the issue, maybe this
would help. In the meantime I've attached what did work for me as a
workaround -- it essentially just makes the deform function to return
some value. It's ugly, but since call site will ignore that, and it's
only one occasion where LLVMBuildRetVoid is used, maybe it's acceptable.
Give me a moment, I'm going to test this change more (waiting on
rebuilding LLVM, it takes quire a while on my machine :( ), then can
confirm that it works as expected on the latest version.
>From 83e37220cc3e8cb04b0d8a608240fdde4b003713 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 30 Mar 2024 17:45:38 +0100
Subject: [PATCH v1] Workaround for deform crashing in LLVM inliner
Deform function returns via the outblock that contains a "ret void"
instruction build with LLVMBuildRetVoid. This triggers a bug in the
latest LLVM 18.1, when the inliner pass is applied to the function [1].
Avoid the issue via introducing a fake return variable, which will be
ignored on the call site.
[1]: https://github.com/llvm/llvm-project/issues/86162
---
src/backend/jit/llvm/llvmjit_deform.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/jit/llvm/llvmjit_deform.c b/src/backend/jit/llvm/llvmjit_deform.c
index b07f8e7f756..7ebc7b6d731 100644
--- a/src/backend/jit/llvm/llvmjit_deform.c
+++ b/src/backend/jit/llvm/llvmjit_deform.c
@@ -761,6 +761,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
{
LLVMValueRef v_off = l_load(b, TypeSizeT, v_offp, "");
LLVMValueRef v_flags;
+ LLVMValueRef v_fake_ret = l_sizet_const(0);
LLVMBuildStore(b, l_int16_const(lc, natts), v_nvalidp);
v_off = LLVMBuildTrunc(b, v_off, LLVMInt32TypeInContext(lc), "");
@@ -768,7 +769,7 @@ slot_compile_deform(LLVMJitContext *context, TupleDesc desc,
v_flags = l_load(b, LLVMInt16TypeInContext(lc), v_flagsp, "tts_flags");
v_flags = LLVMBuildOr(b, v_flags, l_int16_const(lc, TTS_FLAG_SLOW), "");
LLVMBuildStore(b, v_flags, v_flagsp);
- LLVMBuildRetVoid(b);
+ LLVMBuildRet(b, v_fake_ret);
}
LLVMDisposeBuilder(b);
base-commit: 7eb9a8201890f3b208fd4c109a5b08bf139b692a
--
2.41.0
Re: pg_stat_statements and "IN" conditions
> On Tue, Mar 26, 2024 at 04:21:46PM +0900, Yasuo Honda wrote: > Yes. The script uses prepared statements because Ruby on Rails enables > prepared statements by default for PostgreSQL databases. > > Then I tested this branch > https://github.com/yahonda/postgres/tree/pg_stat_statements without > using prepared statements as follows and all of them do not normalize > in clause values. > > - Disabled prepared statements by setting `prepared_statements: false` > https://gist.github.com/yahonda/2c2d6ac7a955886a305750eecfd07c5e > > - Use ruby-pg > https://gist.github.com/yahonda/2f0efb11ae888d8f6b27a07e0b833fdf > > - Use psql > https://gist.github.com/yahonda/c830379b33d66a743aef159aa03d7e49 > > I do not know why even if I use psql, the query column at > pg_stat_sql_statement shows it is like a prepared statement "IN ($1, > $2)". It's a similar case: the column is defined as bigint, thus PostgreSQL has to wrap every constant expression in a function expression that converts its type to bigint. The current patch version doesn't try to reduce a FuncExpr into Const (event if the wrapped value is a Const), thus this array is not getting merged. If you replace bigint with an int, no type conversion would be required and merging logic will kick in. Again, the original version of the patch was able to handle this case, but it was stripped away to make the patch smaller in hope of moving forward. Anyway, thanks for reminding about how annoying the current handling of constant arrays can look like in practice!
Re: pg_stat_statements and "IN" conditions
> On Sun, Mar 24, 2024 at 11:36:38PM +0900, Yasuo Honda wrote: > Thanks for the information. I can apply these 4 patches from > 0eb23285a2 . I tested this branch from Ruby on Rails and it gets some > unexpected behavior from my point of view. > Setting pg_stat_statements.query_id_const_merge_threshold = 5 does not > normalize sql queries whose number of in clauses exceeds 5. > > Here are test steps. > https://gist.github.com/yahonda/825ffccc4dcb58aa60e12ce33d25cd45#expected-behavior > > It would be appreciated if I can get my understanding correct. >From what I understand out of the description this ruby script uses prepared statements, passing values as parameters, right? Unfortunately the current version of the patch doesn't handle that, it works with constants only [1]. The original incarnation of this feature was able to handle that, but the implementation was considered to be not suitable -- thus, to make some progress, it was left outside. The plan is, if everything goes fine at some point, to do a follow-up patch to handle Params and the rest. [1]: https://www.postgresql.org/message-id/20230211104707.grsicemegr7d3mgh%40erthalion.local
Re: pg_stat_statements and "IN" conditions
> On Sat, Mar 23, 2024 at 04:13:44PM +0900, Yasuo Honda wrote: > Hi, I'm interested in this feature. It looks like these patches have > some conflicts. > > http://cfbot.cputube.org/patch_47_2837.log > > Would you rebase these patches? Sure, I can rebase, give me a moment. If you don't want to wait, there is a base commit in the patch, against which it should be applied without issues, 0eb23285a2.
Re: broken JIT support on Fedora 40
> On Sun, Mar 17, 2024 at 09:02:08PM +0100, Dmitry Dolgov wrote: > > On Fri, Mar 15, 2024 at 01:54:38PM +1300, Thomas Munro wrote: > > For me it seems that the LLVMRunPasses() call, new in > > > > commit 76200e5ee469e4a9db5f9514b9d0c6a31b496bff > > Author: Thomas Munro > > Date: Wed Oct 18 22:15:54 2023 +1300 > > > > jit: Changes for LLVM 17. > > > > is reaching code that segfaults inside libLLVM, specifically in > > llvm::InlineFunction(llvm::CallBase&, llvm::InlineFunctionInfo&, bool, > > llvm::AAResults*, bool, llvm::Function*). First obvious question > > would be: is that NULL argument still acceptable? Perhaps it wants > > our LLVMTargetMachineRef there: > > > >err = LLVMRunPasses(module, passes, NULL, options); > > > > But then when we see what is does with that argument, it arrives at a > > place that apparently accepts nullptr. > > > > https://github.com/llvm/llvm-project/blob/6b2bab2839c7a379556a10287034bd55906d7094/llvm/lib/Passes/PassBuilderBindings.cpp#L56 > > https://github.com/llvm/llvm-project/blob/6b2bab2839c7a379556a10287034bd55906d7094/llvm/include/llvm/Passes/PassBuilder.h#L124 > > > > Hrmph. Might need an assertion build to learn more. I'll try to look > > again next week or so. > > Looks like I can reproduce this as well, libLLVM crashes when reaching > AddReturnAttributes inside InlineFunction, when trying to access > operands of the return instruction. I think, for whatever reason, the > latest LLVM doesn't like (i.e. do not expect this when performing > inlining pass) return instructions that do not have a return value, and > this is what happens in the outblock of deform function we generate > (slot_compile_deform). > > For verification, I've modified the deform.outblock to call LLVMBuildRet > instead of LLVMBuildRetVoid and this seems to help -- inline and deform > stages are still performed as before, but nothing crashes. But of course > it doesn't sound right that inlining pass cannot process such code. > Unfortunately I don't see any obvious change in the recent LLVM history > that would justify this outcome, might be a genuine bug, will > investigate further. I think I found the change that got it all started [1], the commit has a set of tags like 18.1.0-rc1 and is relatively recent. The message doesn't say anything related to the crash that we see, so I assume it's indeed a bug. I've opened an issue to confirm this understanding [2] (wow, issues were indeed moved to github since the last time I was touching LLVM), let's see what would be the response. [1]: https://github.com/llvm/llvm-project/commit/2da4960f20f7e5d88a68ce25636a895284dc66d8 [2]: https://github.com/llvm/llvm-project/issues/86162
Re: broken JIT support on Fedora 40
> On Fri, Mar 15, 2024 at 01:54:38PM +1300, Thomas Munro wrote: > For me it seems that the LLVMRunPasses() call, new in > > commit 76200e5ee469e4a9db5f9514b9d0c6a31b496bff > Author: Thomas Munro > Date: Wed Oct 18 22:15:54 2023 +1300 > > jit: Changes for LLVM 17. > > is reaching code that segfaults inside libLLVM, specifically in > llvm::InlineFunction(llvm::CallBase&, llvm::InlineFunctionInfo&, bool, > llvm::AAResults*, bool, llvm::Function*). First obvious question > would be: is that NULL argument still acceptable? Perhaps it wants > our LLVMTargetMachineRef there: > >err = LLVMRunPasses(module, passes, NULL, options); > > But then when we see what is does with that argument, it arrives at a > place that apparently accepts nullptr. > > https://github.com/llvm/llvm-project/blob/6b2bab2839c7a379556a10287034bd55906d7094/llvm/lib/Passes/PassBuilderBindings.cpp#L56 > https://github.com/llvm/llvm-project/blob/6b2bab2839c7a379556a10287034bd55906d7094/llvm/include/llvm/Passes/PassBuilder.h#L124 > > Hrmph. Might need an assertion build to learn more. I'll try to look > again next week or so. Looks like I can reproduce this as well, libLLVM crashes when reaching AddReturnAttributes inside InlineFunction, when trying to access operands of the return instruction. I think, for whatever reason, the latest LLVM doesn't like (i.e. do not expect this when performing inlining pass) return instructions that do not have a return value, and this is what happens in the outblock of deform function we generate (slot_compile_deform). For verification, I've modified the deform.outblock to call LLVMBuildRet instead of LLVMBuildRetVoid and this seems to help -- inline and deform stages are still performed as before, but nothing crashes. But of course it doesn't sound right that inlining pass cannot process such code. Unfortunately I don't see any obvious change in the recent LLVM history that would justify this outcome, might be a genuine bug, will investigate further.
Re: Schema variables - new implementation for Postgres 15
Yep, in this constellation the implementation holds much better (in terms of memory) in my create/let/drop testing. I've marked the CF item as ready for committer, but a note for anyone who would like to pick up it from here -- we're talking about first 5 patches here, up to the memory cleaning after DROP VARIABLE. It doesn't mean the rest is somehow not worth it, but I believe it's a good first step.
Re: Schema variables - new implementation for Postgres 15
> On Mon, Jan 29, 2024 at 08:57:42AM +0100, Pavel Stehule wrote: > Hi > > ne 28. 1. 2024 v 19:00 odesílatel Dmitry Dolgov <9erthali...@gmail.com> > napsal: > > > Thanks for the update, smaller patches looks promising. > > > > Off the list Pavel has mentioned that the first two patches contain a > > bare minimum for session variables, so I've reviewed them once more and > > suggest to concentrate on them first. I'm afraid the memory cleanup > > patch has to be added to the "bare minimum" set as well -- otherwise in > > my tests it was too easy to run out of memory via creating, assigning > > and dropping variables. Unfortunately one can't extract those three > > patches from the series and apply only them, the memory patch would have > > some conflicts. Can you maybe reshuffle the series to have those patches > > (1, 2 + 8) as first three? > > > > probably you need too > > 0006-function-pg_session_variables-for-cleaning-tests.patch and > 0007-DISCARD-VARIABLES.patch > > 6 is necessary for testing of cleaning Ok, let me take a look at those. Unless there are any objections, my plan would be to give it a final check and mark the CF item as ready for committer -- meaning the first 5 patches.
Re: Schema variables - new implementation for Postgres 15
> On Sun, Jan 28, 2024 at 08:34:40PM +0100, Pavel Stehule wrote:
> > * I've noticed an interesting result when a LET statement is used to
> > assign a
> > value without a subquery:
> >
> > create variable test as text;
> > -- returns NULL
> > select test;
> >
> > -- use repeat directly without a subquery
> > let test = repeat("test", 10);
> >
> > -- returns NULL
> > select test;
> >
> > I was expecting to see an error here, is this a correct behaviour?
> >
>
> what is strange on this result?
Never mind, I've got confused about the quotes here -- it was referring
to the variable content, not a string.
Re: Schema variables - new implementation for Postgres 15
Thanks for the update, smaller patches looks promising.
Off the list Pavel has mentioned that the first two patches contain a
bare minimum for session variables, so I've reviewed them once more and
suggest to concentrate on them first. I'm afraid the memory cleanup
patch has to be added to the "bare minimum" set as well -- otherwise in
my tests it was too easy to run out of memory via creating, assigning
and dropping variables. Unfortunately one can't extract those three
patches from the series and apply only them, the memory patch would have
some conflicts. Can you maybe reshuffle the series to have those patches
(1, 2 + 8) as first three?
If that's possible, my proposal would be to proceed with them first. To the
best of my knowledge they look good to me, except few minor details:
* The documentation says in a couple of places (ddl.sgml,
create_variable.sgml) that "Retrieving a session variable's value
returns either a NULL or a default value", but as far as I see the
default value feature is not implemented within first two patches.
* Similar with mentioning immutable session variables in plpgsql.sgml .
* Commentary to LookupVariable mentions a rowtype_only argument:
+/*
+ * Returns oid of session variable specified by possibly qualified
identifier.
+ *
+ * If not found, returns InvalidOid if missing_ok, else throws error.
+ * When rowtype_only argument is true the session variables of not
+ * composite types are ignored. This should to reduce possible
collisions.
+ */
+Oid
+LookupVariable(const char *nspname,
+ const char *varname,
+ bool missing_ok)
but the function doesn't have it.
* I've noticed an interesting result when a LET statement is used to assign a
value without a subquery:
create variable test as text;
-- returns NULL
select test;
-- use repeat directly without a subquery
let test = repeat("test", 10);
-- returns NULL
select test;
I was expecting to see an error here, is this a correct behaviour?
Re: pg_stat_statements and "IN" conditions
> On Mon, Jan 22, 2024 at 06:07:27PM +0100, Dmitry Dolgov wrote: > > Please notice that at the moment, it's not being tested at all because > > of a patch-apply failure -- that's what the little triangular symbol > > means. The rest of the display concerns the test results from the > > last successfully-applied patch version. (Perhaps that isn't a > > particularly great UI design.) > > > > If you click on the triangle you find out > > > > == Applying patches on top of PostgreSQL commit ID > > b0f0a9432d0b6f53634a96715f2666f6d4ea25a1 === > > === applying patch > > ./v17-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch > > patching file contrib/pg_stat_statements/Makefile > > Hunk #1 FAILED at 19. > > 1 out of 1 hunk FAILED -- saving rejects to file > > contrib/pg_stat_statements/Makefile.rej > > patching file contrib/pg_stat_statements/expected/merging.out > > patching file contrib/pg_stat_statements/meson.build > > Oh, I see, thanks. Give me a moment, will fix this. Here is it. >From ac8a7c93fbb72c469ca7128280f52024adb860ab Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Mon, 22 Jan 2024 21:11:15 +0100 Subject: [PATCH v18 1/4] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on the number of parameters, because every element of ArrayExpr is jumbled. In certain situations it's undesirable, especially if the list becomes too large. Make an array of Const expressions contribute only the first/last elements to the jumble hash. Allow to enable this behavior via the new pg_stat_statements parameter query_id_const_merge with the default value off. Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane, Michael Paquier, Sergei Kornilov, Alvaro Herrera, David Geier Tested-by: Chengxi Sun --- contrib/pg_stat_statements/Makefile | 2 +- .../pg_stat_statements/expected/merging.out | 167 ++ contrib/pg_stat_statements/meson.build| 1 + .../pg_stat_statements/pg_stat_statements.c | 62 ++- contrib/pg_stat_statements/sql/merging.sql| 58 ++ doc/src/sgml/pgstatstatements.sgml| 57 +- src/backend/nodes/gen_node_support.pl | 21 ++- src/backend/nodes/queryjumblefuncs.c | 100 ++- src/backend/postmaster/postmaster.c | 3 + src/backend/utils/misc/postgresql.conf.sample | 1 - src/include/nodes/primnodes.h | 2 +- src/include/nodes/queryjumble.h | 9 +- 12 files changed, 458 insertions(+), 25 deletions(-) create mode 100644 contrib/pg_stat_statements/expected/merging.out create mode 100644 contrib/pg_stat_statements/sql/merging.sql diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile index 414a30856e4..03a62d685f3 100644 --- a/contrib/pg_stat_statements/Makefile +++ b/contrib/pg_stat_statements/Makefile @@ -19,7 +19,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS)) REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf REGRESS = select dml cursors utility level_tracking planning \ - user_activity wal entry_timestamp cleanup oldextversions + user_activity wal entry_timestamp cleanup oldextversions merging # Disabled because these tests require "shared_preload_libraries=pg_stat_statements", # which typical installcheck users do not have (e.g. buildfarm clients). NO_INSTALLCHECK = 1 diff --git a/contrib/pg_stat_statements/expected/merging.out b/contrib/pg_stat_statements/expected/merging.out new file mode 100644 index 000..1e58283afe8 --- /dev/null +++ b/contrib/pg_stat_statements/expected/merging.out @@ -0,0 +1,167 @@ +-- +-- Const merging functionality +-- +CREATE EXTENSION pg_stat_statements; +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- No merging is performed, as a baseline result +SELECT pg_stat_statements_reset() IS NOT NULL AS t; + t +--- + t +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; +query| calls +-+--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1
Re: pg_stat_statements and "IN" conditions
> On Mon, Jan 22, 2024 at 11:35:22AM -0500, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: > >> On Mon, Jan 22, 2024 at 05:33:26PM +1100, Peter Smith wrote: > >> Hi, This patch has a CF status of "Needs Review" [1], but it seems > >> there was a CFbot test failure last time it was run [2]. Please have a > >> look and post an updated version if necessary. > >> > >> == > >> [1] https://commitfest.postgresql.org/46/2837/ > >> [2] https://cirrus-ci.com/task/6688578378399744 > > > It's the same failing pipeline Vignesh C was talking above. I've fixed > > the issue in the latest patch version, but looks like it wasn't picked > > up yet (from what I understand, the latest build for this CF is 8 weeks > > old). > > Please notice that at the moment, it's not being tested at all because > of a patch-apply failure -- that's what the little triangular symbol > means. The rest of the display concerns the test results from the > last successfully-applied patch version. (Perhaps that isn't a > particularly great UI design.) > > If you click on the triangle you find out > > == Applying patches on top of PostgreSQL commit ID > b0f0a9432d0b6f53634a96715f2666f6d4ea25a1 === > === applying patch > ./v17-0001-Prevent-jumbling-of-every-element-in-ArrayExpr.patch > patching file contrib/pg_stat_statements/Makefile > Hunk #1 FAILED at 19. > 1 out of 1 hunk FAILED -- saving rejects to file > contrib/pg_stat_statements/Makefile.rej > patching file contrib/pg_stat_statements/expected/merging.out > patching file contrib/pg_stat_statements/meson.build Oh, I see, thanks. Give me a moment, will fix this.
Re: pg_stat_statements and "IN" conditions
> On Mon, Jan 22, 2024 at 05:33:26PM +1100, Peter Smith wrote: > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > there was a CFbot test failure last time it was run [2]. Please have a > look and post an updated version if necessary. > > == > [1] https://commitfest.postgresql.org/46/2837/ > [2] https://cirrus-ci.com/task/6688578378399744 It's the same failing pipeline Vignesh C was talking above. I've fixed the issue in the latest patch version, but looks like it wasn't picked up yet (from what I understand, the latest build for this CF is 8 weeks old).
Re: pg_stat_statements and "IN" conditions
> On Mon, Jan 08, 2024 at 05:10:20PM +0100, Dmitry Dolgov wrote:
> > On Sat, Jan 06, 2024 at 09:04:54PM +0530, vignesh C wrote:
> >
> > CFBot shows documentation build has failed at [1] with:
> > [07:44:55.531] time make -s -j${BUILD_JOBS} -C doc
> > [07:44:57.987] postgres.sgml:572: element xref: validity error : IDREF
> > attribute linkend references an unknown ID
> > "guc-query-id-const-merge-threshold"
> > [07:44:58.179] make[2]: *** [Makefile:70: postgres-full.xml] Error 4
> > [07:44:58.179] make[2]: *** Deleting file 'postgres-full.xml'
> > [07:44:58.181] make[1]: *** [Makefile:8: all] Error 2
> > [07:44:58.182] make: *** [Makefile:16: all] Error 2
> >
> > [1] - https://cirrus-ci.com/task/6688578378399744
>
> Indeed, after moving the configuration option to pgss I forgot to update
> its reference in the docs. Thanks for noticing, will update soon.
Here is the fixed version.
>From 1b99ffd68de6e82d9bbc45c18153ef965a228e28 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 14 Oct 2023 15:00:48 +0200
Subject: [PATCH v17 1/4] Prevent jumbling of every element in ArrayExpr
pg_stat_statements produces multiple entries for queries like
SELECT something FROM table WHERE col IN (1, 2, 3, ...)
depending on the number of parameters, because every element of
ArrayExpr is jumbled. In certain situations it's undesirable, especially
if the list becomes too large.
Make an array of Const expressions contribute only the first/last
elements to the jumble hash. Allow to enable this behavior via the new
pg_stat_statements parameter query_id_const_merge with the default value off.
Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane,
Michael Paquier, Sergei Kornilov, Alvaro Herrera, David Geier
Tested-by: Chengxi Sun
---
contrib/pg_stat_statements/Makefile | 2 +-
.../pg_stat_statements/expected/merging.out | 167 ++
contrib/pg_stat_statements/meson.build| 1 +
.../pg_stat_statements/pg_stat_statements.c | 62 ++-
contrib/pg_stat_statements/sql/merging.sql| 58 ++
doc/src/sgml/pgstatstatements.sgml| 57 +-
src/backend/nodes/gen_node_support.pl | 21 ++-
src/backend/nodes/queryjumblefuncs.c | 100 ++-
src/backend/postmaster/postmaster.c | 3 +
src/backend/utils/misc/postgresql.conf.sample | 1 -
src/include/nodes/primnodes.h | 2 +-
src/include/nodes/queryjumble.h | 9 +-
12 files changed, 458 insertions(+), 25 deletions(-)
create mode 100644 contrib/pg_stat_statements/expected/merging.out
create mode 100644 contrib/pg_stat_statements/sql/merging.sql
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index eba4a95d91a..af731fc9a58 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -19,7 +19,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS))
REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
REGRESS = select dml cursors utility level_tracking planning \
- user_activity wal cleanup oldextversions
+ user_activity wal cleanup oldextversions merging
# Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
# which typical installcheck users do not have (e.g. buildfarm clients).
NO_INSTALLCHECK = 1
diff --git a/contrib/pg_stat_statements/expected/merging.out b/contrib/pg_stat_statements/expected/merging.out
new file mode 100644
index 000..f286c735a36
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/merging.out
@@ -0,0 +1,167 @@
+--
+-- Const merging functionality
+--
+CREATE EXTENSION pg_stat_statements;
+CREATE TABLE test_merge (id int, data int);
+-- IN queries
+-- No merging is performed, as a baseline result
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id | data
++--
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10);
+ id | data
++--
+(0 rows)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
+ id | data
++--
+(0 rows)
+
+SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C";
+query| calls
+-+---
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1
+ SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) | 1
+ SELECT pg_stat_statements_reset()
Re: pg_stat_statements and "IN" conditions
> On Sat, Jan 06, 2024 at 09:04:54PM +0530, vignesh C wrote:
>
> CFBot shows documentation build has failed at [1] with:
> [07:44:55.531] time make -s -j${BUILD_JOBS} -C doc
> [07:44:57.987] postgres.sgml:572: element xref: validity error : IDREF
> attribute linkend references an unknown ID
> "guc-query-id-const-merge-threshold"
> [07:44:58.179] make[2]: *** [Makefile:70: postgres-full.xml] Error 4
> [07:44:58.179] make[2]: *** Deleting file 'postgres-full.xml'
> [07:44:58.181] make[1]: *** [Makefile:8: all] Error 2
> [07:44:58.182] make: *** [Makefile:16: all] Error 2
>
> [1] - https://cirrus-ci.com/task/6688578378399744
Indeed, after moving the configuration option to pgss I forgot to update
its reference in the docs. Thanks for noticing, will update soon.
Re: [RFC] Clang plugin for catching suspicious typedef casting
> On Sun, Dec 03, 2023 at 07:02:55PM -0800, Peter Geoghegan wrote: > On Sun, Dec 3, 2023 at 6:31 AM Dmitry Dolgov <9erthali...@gmail.com> wrote: > > > Only advantage I see to using libclang is that you can run programs built > > > with libclang regardless of what your C compiler is. I typically use GCC. > > > > > > I think your idea of automating this kind of thing is great no matter how > > > it > > > is implemented. > > > > Yeah, absolutely. > > What is the difference between a clang plugin (or whatever this is), > and a custom clang-tidy check? Are they the same thing? Good question. Obviously one difference is that a clang plugin is more tightly integrated into the build process, where a custom clang-tidy check could be used independently. IIUC they also use different API, AST Consumer vs AST Matchers, but so far I have no idea what consequences does it have. Clang tooling page has this to say about choosing the right interface [1], where clang-tidy seems to fall into LibTooling cathegory: Clang Plugins allow you to run additional actions on the AST as part of a compilation. Plugins are dynamic libraries that are loaded at runtime by the compiler, and they’re easy to integrate into your build environment. Canonical examples of when to use Clang Plugins: * special lint-style warnings or errors for your project creating * additional build artifacts from a single compile step [...] LibTooling is a C++ interface aimed at writing standalone tools, as well as integrating into services that run clang tools. Canonical examples of when to use LibTooling: * a simple syntax checker * refactoring tools > I myself use clang-tidy through clangd. It has a huge number of > checks, plus some custom checks that are only used by certain open > source projects. > > An example of a check that seems like it would be interesting to > Postgres hackers: > > https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/bugprone/signal-handler.html > > An example of a custom clang-tidy check, used for the Linux Kernel: > > https://releases.llvm.org/17.0.1/tools/clang/tools/extra/docs/clang-tidy/checks/linuxkernel/must-use-errs.html Yeah, those look interesting, and might be even worth including independently of the topic in this thread. > Your idea of starting with a check that identifies when BlockNumber > and Buffer variables were confused seems like the right general idea > to me. It's just about impossible for this to be a false positive in practice, > which is important. But, I wonder, is there a way of accomplishing the > same thing without any custom code? Seems like the general idea of not > confusing two types like BlockNumber and Buffer might be a generic > one? Agree, the example in this patch could be generalized. > * A custom check that enforces coding standards within signal handlers > -- the generic one that I linked to might need to be customized, in > whatever way (don't use it myself). > > * A custom check that enforces a coding standard that applies within > critical sections. > > We already have an assertion that catches memory allocations made > within a critical section. It might make sense to have tooling that > can detect other kinds of definitely-unsafe things in critical > sections. For example, maybe it would be possible to detect when > XLogRegisterBufData() has been passed a pointer to something on the > stack that will go out of scope, in a way that leaves open the > possibility of reading random stuff from the stack once inside > XLogInsert(). AddressSanitizer's use-after-scope can detect that sort > of thing, though not reliably. Interesting ideas, thanks for sharing. I'm going to try implementing some of those. [1]: https://clang.llvm.org/docs/Tooling.html
Re: [RFC] Clang plugin for catching suspicious typedef casting
> On Fri, Dec 01, 2023 at 04:01:05PM -0600, Tristan Partin wrote: > On Fri Aug 4, 2023 at 5:47 AM CDT, Dmitry Dolgov wrote: > > > On Thu, Aug 03, 2023 at 12:23:52PM -0500, Tristan Partin wrote: > > > > > > This is the first I am learning about clang plugins. Interesting concept. > > > Did you give any thought to using libclang instead of a compiler plugin? I > > > am kind of doing similar work, but I went with libclang instead of a > > > plugin. > > > > Nope, never thought about trying libclang. From the clang documentation > > it seems a plugin is a suitable interface if one wants to: > > > > special lint-style warnings or errors for your project > > > > Which is what I was trying to achieve. Are there any advantages of > > libclang that you see? > > Only advantage I see to using libclang is that you can run programs built > with libclang regardless of what your C compiler is. I typically use GCC. > > I think your idea of automating this kind of thing is great no matter how it > is implemented. Yeah, absolutely. Sorry, haven't had a chance to follow up on the patch, but I'm planing to do so. I guess the important part, as Peter mentioned above in the thread, is to figure out more use cases which could be usefully augmented with such compile time checks. At the moment I don't have any other except BlockNumber vs Buffer, so I'm going to search through the hackers looking for more potential targets. If anyone got a great idea right away about where compile-time checks could be useful, feel free to share!
Re: Schema variables - new implementation for Postgres 15
> On Sun, Dec 03, 2023 at 06:04:12AM +0100, Pavel Stehule wrote: > > Thank you for your review. Next two weeks I'll not too much time to work > on this patch - I have to work on some commercial work, and the week is > Prague PgConf, so my reply will be slow. But after these events I'll > concentrate on this patch. No worries, it's fine. Have fun at PGConf!
Re: Schema variables - new implementation for Postgres 15
> On Sat, Nov 18, 2023 at 06:28:53PM +0100, Pavel Stehule wrote: > so 18. 11. 2023 v 15:54 odesílatel Dmitry Dolgov <9erthali...@gmail.com> > napsal: > > As a side note, I'm intended to go one more time through the first few > > patches introducing the basic functionality, and then mark it as ready > > in CF. I can't break the patch in testing since quite long time, and for > > most parts the changes make sense to me. > > I marked pg_session_variables function as PARALLEL RESTRICTED, and did > rebase So, after one week of uninterrupted evening reviews I've made it through the first four patches :) It's a decent job -- more than once, looking at the code, I thought I could construct a case when it's going to blow up, but everything was working just fine. Yet, I think the patch still has to be reshaped a bit before moving forward. I've got a couple proposals of different nature: high level changes (you probably won't like some of them, but I'm sure they're going to be useful), technical code-level improvements/comments, and few language changes. With those changes in mind I would be satisfied with the patch, and hopefully they would also make it easier for a potential committer to pick it up. # High level proposals * I would suggest reducing the scope of the patch as much as possible, and not just by trimming on the edges, but rather following Phileas Fogg's example with the steamboat Henrietta -- get rid of all non-essential parts. This will make this rather large patch more approachable for others. For that one can concentrate only on the first two patches plus the fourth one (memory cleanup after dropping variables), leaving DISCARD, ON TRANSACTION END, DEFAULT, IMMUTABLE for the follow-up in the future. Another thing in this context would be to evaluate plpgsql support for this feature. You know the use case better than me, how important it is? Is it an intrinsic part of the feature, or session variables could be still valuable enough even without plpgsql? From what I see postponing plgpsql will make everything about ~800 lines lighter (most likely more), and also allow to ignore couple of concerns about the implementation (about this later). * The new GUC session_variables_ambiguity_warning is definitely going to cause many objections, it's another knob to manage very subtle behaviour detail very few people will ever notice. I see the point behind warning about ambiguity, so probably it makes sense to bite the bullet and decide one way or another. The proposal is to warn always in potentially ambiguous situations, and if concerns are high about logging too much, maybe do the warning on lower logging levels. # Code-level observations * It feels a bit awkward to have varid assignment logic in a separate function, what about adding an argument with varid to CreateVariableDestReceiver? SetVariableDestReceiverVarid still could be used for CreateDestReceiver. /* * Initially create a DestReceiver object. */ DestReceiver * CreateVariableDestReceiver(void) /* * Set parameters for a VariableDestReceiver. * Should be called right after creating the DestReceiver. */ void SetVariableDestReceiverVarid(DestReceiver *self, Oid varid) * It's worth it to add a commentary here explaining why it's fine to use InvalidOid here: if (pstmt->commandType != CMD_UTILITY) - ExplainOnePlan(pstmt, into, es, query_string, paramLI, queryEnv, + ExplainOnePlan(pstmt, into, InvalidOid, es, query_string, paramLI, queryEnv, , (es->buffers ? : NULL)); My understanding is that since LetStmt is CMD_UTILITY, this branch will never be visited for a session variable. * IIUC this one is introduced to exclude session variables from the normal path with EXPR_KIND_UPDATE_TARGET: + EXPR_KIND_ASSIGN_VARIABLE, /* PL/pgSQL assignment target - disallow +* session variables */ But the name doesn't sound right, maybe longer EXPR_KIND_UPDATE_TARGET_NO_VARS is better? * I'm curious about this one, which exactly part does this change cover? @@ -4888,21 +4914,43 @@ substitute_actual_parameters_mutator(Node *node, - if (param->paramkind != PARAM_EXTERN) + if (param->paramkind != PARAM_EXTERN && + param->paramkind != PARAM_VARIABLE) elog(ERROR, "unexpected paramkind: %d", (int) param->paramkind); I've commented it out, but no tests were affected. * Does it mean there could be theoretically two LET statements at the same time with different command type, one CMD_UTILITY, one CMD_SELECT? Can it cause any issues? + /* +* Inside PL/pgSQL we don't want to execute LET statement as utility +* command, because it disallow to execute ex
Re: Schema variables - new implementation for Postgres 15
> On Sat, Nov 18, 2023 at 02:19:09PM +0100, Pavel Stehule wrote: > > Would it be a problem to make pg_session_variables inspect the catalog > > or something similar if needed? > > > > It can be very easy to build pg_session_variables based on iteration over > the system catalog. But I am not sure if we want it. pg_session_variables() > is designed to show the variables from session memory, and it is used for > testing. Originally it was named pg_debug_session_variables. If we iterate > over catalog, it means using locks, and it can have an impact on isolation > tests. I see, thanks for clarification. In the end one can check the catalog directly of course, is there any other value in this function except for debugging purposes? As a side note, I'm intended to go one more time through the first few patches introducing the basic functionality, and then mark it as ready in CF. I can't break the patch in testing since quite long time, and for most parts the changes make sense to me.
Re: Schema variables - new implementation for Postgres 15
> On Wed, Aug 23, 2023 at 04:02:44PM +0200, Pavel Stehule wrote: > NameListToString is already buildin function. Do you think NamesFromList? > > This is my oversight - there is just `+extern List *NamesFromList(List > *names); ` line, but sure - it should be in 0002 patch > > fixed now Right, thanks for fixing. I think there is a wrinkle with pg_session_variables function. It returns nothing if sessionvars hash table is empty, which has two consequences: * One might get confused about whether a variable is created, based on the information from the function. An expected behaviour, but could be considered a bad UX. =# CREATE VARIABLE var1 AS varchar; -- empty, is expected =# SELECT name, typname, can_select, can_update FROM pg_session_variables(); name | typname | can_select | can_update --+-++ (0 rows) -- but one can't create a variable =# CREATE VARIABLE var1 AS varchar; ERROR: 42710: session variable "var1" already exists LOCATION: create_variable, pg_variable.c:102 -- yet, suddenly after a select... =# SELECT var2; var2 -- NULL (1 row) -- ... it's not empty =# SELECT name, typname, can_select, can_update FROM pg_sessio n_variables(); name | typname | can_select | can_update --+---++ var2 | character varying | t | t (1 row) * Running a parallel query will end up returning an empty result even after accessing the variable. -- debug_parallel_query = 1 all the time =# CREATE VARIABLE var2 AS varchar; -- empty, is expected =# SELECT name, typname, can_select, can_update FROM pg_session_variables(); name | typname | can_select | can_update --+-++ (0 rows) -- but this time an access... SELECT var2; var2 -- NULL (1 row) -- or set... =# LET var2 = 'test'; -- doesn't change the result, it's still empty =# SELECT name, typname, can_select, can_update FROM pg_session_variables(); name | typname | can_select | can_update --+-++ (0 rows) Would it be a problem to make pg_session_variables inspect the catalog or something similar if needed?
Re: pg_stat_statements and "IN" conditions
> On Fri, Oct 27, 2023 at 05:02:44PM +0200, Dmitry Dolgov wrote:
> > On Thu, Oct 26, 2023 at 09:08:42AM +0900, Michael Paquier wrote:
> > typedef struct ArrayExpr
> > {
> > + pg_node_attr(custom_query_jumble)
> > +
> >
> > Hmm. I am not sure that this is the best approach
> > implementation-wise. Wouldn't it be better to invent a new
> > pg_node_attr (these can include parameters as well!), say
> > query_jumble_merge or query_jumble_agg_location that aggregates all
> > the parameters of a list to be considered as a single element. To put
> > it short, we could also apply the same property to other parts of a
> > parsed tree, and not only an ArrayExpr's list.
>
> Sounds like an interesting idea, something like:
>
> typedef struct ArrayExpr
> {
> ...
> List *elements pg_node_attr(query_jumble_merge);
>
> to replace simple JUMBLE_NODE(elements) with more elaborated logic.
>
> > /* GUC parameters */
> > extern PGDLLIMPORT int compute_query_id;
> > -
> > +extern PGDLLIMPORT bool query_id_const_merge;
> >
> > Not much a fan of this addition as well for an in-core GUC. I would
> > suggest pushing the GUC layer to pg_stat_statements, maintaining the
> > computation method to use as a field of JumbleState as I suspect that
> > this is something we should not enforce system-wide, but at
> > extension-level instead.
>
> I also do not particularly like an extra GUC here, but as far as I can
> tell to make it pg_stat_statements GUC only it has to be something
> similar to EnableQueryId (e.g. EnableQueryConstMerging), that will be
> called from pgss. Does this sound better?
For clarity, here is what I had in mind for those two points.
>From 760420fc4aeb96c20d99ae205ee57670d73dc27b Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 14 Oct 2023 15:00:48 +0200
Subject: [PATCH v16 1/4] Prevent jumbling of every element in ArrayExpr
pg_stat_statements produces multiple entries for queries like
SELECT something FROM table WHERE col IN (1, 2, 3, ...)
depending on the number of parameters, because every element of
ArrayExpr is jumbled. In certain situations it's undesirable, especially
if the list becomes too large.
Make an array of Const expressions contribute only the first/last
elements to the jumble hash. Allow to enable this behavior via the new
pg_stat_statements parameter query_id_const_merge with the default value off.
Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane,
Michael Paquier, Sergei Kornilov, Alvaro Herrera, David Geier
Tested-by: Chengxi Sun
---
contrib/pg_stat_statements/Makefile | 2 +-
.../pg_stat_statements/expected/merging.out | 167 ++
contrib/pg_stat_statements/meson.build| 1 +
.../pg_stat_statements/pg_stat_statements.c | 62 ++-
contrib/pg_stat_statements/sql/merging.sql| 58 ++
doc/src/sgml/pgstatstatements.sgml| 54 +-
src/backend/nodes/gen_node_support.pl | 21 ++-
src/backend/nodes/queryjumblefuncs.c | 100 ++-
src/backend/postmaster/postmaster.c | 3 +
src/backend/utils/misc/postgresql.conf.sample | 1 -
src/include/nodes/primnodes.h | 2 +-
src/include/nodes/queryjumble.h | 9 +-
12 files changed, 456 insertions(+), 24 deletions(-)
create mode 100644 contrib/pg_stat_statements/expected/merging.out
create mode 100644 contrib/pg_stat_statements/sql/merging.sql
diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile
index eba4a95d91a..af731fc9a58 100644
--- a/contrib/pg_stat_statements/Makefile
+++ b/contrib/pg_stat_statements/Makefile
@@ -19,7 +19,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS))
REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf
REGRESS = select dml cursors utility level_tracking planning \
- user_activity wal cleanup oldextversions
+ user_activity wal cleanup oldextversions merging
# Disabled because these tests require "shared_preload_libraries=pg_stat_statements",
# which typical installcheck users do not have (e.g. buildfarm clients).
NO_INSTALLCHECK = 1
diff --git a/contrib/pg_stat_statements/expected/merging.out b/contrib/pg_stat_statements/expected/merging.out
new file mode 100644
index 000..f286c735a36
--- /dev/null
+++ b/contrib/pg_stat_statements/expected/merging.out
@@ -0,0 +1,167 @@
+--
+-- Const merging functionality
+--
+CREATE EXTENSION pg_stat_statements;
+CREATE TABLE test_merge (id int, data int);
+-- IN queries
+-- No merging is performed, as a baseline result
+SELECT pg_stat_statements_reset();
+ pg_stat_statements_reset
+--
+
+(1 row)
+
+SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9);
+ id |
Re: pg_stat_statements and "IN" conditions
> On Thu, Oct 26, 2023 at 09:08:42AM +0900, Michael Paquier wrote:
> typedef struct ArrayExpr
> {
> + pg_node_attr(custom_query_jumble)
> +
>
> Hmm. I am not sure that this is the best approach
> implementation-wise. Wouldn't it be better to invent a new
> pg_node_attr (these can include parameters as well!), say
> query_jumble_merge or query_jumble_agg_location that aggregates all
> the parameters of a list to be considered as a single element. To put
> it short, we could also apply the same property to other parts of a
> parsed tree, and not only an ArrayExpr's list.
Sounds like an interesting idea, something like:
typedef struct ArrayExpr
{
...
List *elements pg_node_attr(query_jumble_merge);
to replace simple JUMBLE_NODE(elements) with more elaborated logic.
> /* GUC parameters */
> extern PGDLLIMPORT int compute_query_id;
> -
> +extern PGDLLIMPORT bool query_id_const_merge;
>
> Not much a fan of this addition as well for an in-core GUC. I would
> suggest pushing the GUC layer to pg_stat_statements, maintaining the
> computation method to use as a field of JumbleState as I suspect that
> this is something we should not enforce system-wide, but at
> extension-level instead.
I also do not particularly like an extra GUC here, but as far as I can
tell to make it pg_stat_statements GUC only it has to be something
similar to EnableQueryId (e.g. EnableQueryConstMerging), that will be
called from pgss. Does this sound better?
> + /*
> + * Indicates the constant represents the beginning or the end of a
> merged
> + * constants interval.
> + */
> + boolmerged;
>
> Not sure that this is the best thing to do either. Instead of this
> extra boolean flag, could it be simpler if we switch LocationLen so as
> we track the start position and the end position of a constant in a
> query string, so as we'd use one LocationLen for a whole set of Const
> nodes in an ArrayExpr? Perhaps this could just be a refactoring piece
> of its own?
Sounds interesting as well, but it seems to me there is a catch. I'll
try to elaborate, bear with me:
* if the start and the end positions of a constant means the first and the
last character representing it, we need the textual length of the
constant in the query to be able to construct such a LocationLen. The
lengths are calculated in pg_stat_statements later, not in JumbleQuery,
and it uses parser for that. Doing all of this in JumbleQuery doesn't
sound reasonable to me.
* if instead we talk about the start and the end positions in a
set of constants, that would mean locations of the first and the last
constants in the set, and everything seems fine. But for such
LocationLen to represent a single constant (not a set of constants), it
means that only the start position would be meaningful, the end position
will not be used.
The second approach is somewhat close to be simpler than the merge flag,
but assumes the ugliness for a single constant. What do you think about
this?
> + /*
> + * If the first expression is a constant, verify if the following
> elements
> + * are constants as well. If yes, the list is eligible for merging, and
> the
> + * order of magnitude need to be calculated.
> + */
> + if (IsA(firstExpr, Const))
> + {
> + foreach(temp, elements)
> + if (!IsA(lfirst(temp), Const))
> + return false;
>
> This path should be benchmarked, IMO.
I can do some benchmarking here, but of course it's going to be slower
than the baseline. The main idea behind the patch is to trade this
overhead for the benefits in the future while processing pgss records,
hoping that it's going to be worth it (and in those extreme cases I'm
trying to address it's definitely worth it).
Re: pg_stat_statements and "IN" conditions
> On Fri, Oct 13, 2023 at 03:35:19PM +0200, Dmitry Dolgov wrote: > > On Fri, Oct 13, 2023 at 05:07:00PM +0900, Michael Paquier wrote: > > Now, it doesn't mean that this approach with the "powers" will never > > happen, but based on the set of opinions I am gathering on this thread > > I would suggest to rework the patch as follows: > > - First implement an on/off switch that reduces the lists in IN and/or > > ANY to one parameter. Simply. > > - Second, refactor the powers routine. > > - Third, extend the on/off switch, or just implement a threshold with > > a second switch. > > Well, if it will help move this patch forward, why not. To clarify, I'm > going to split the current implementation into three patches, one for > each point you've mentioned. Here is what I had mind. The first patch implements the basic notion of merging, and I guess everyone agrees on its usefulness. The second and third implement merging into groups power of 10, which I still find useful as well. The last one adds a lower threshold for merging on top of that. My intentions are to get the first one in, ideally I would love to see the second and third applied as well. > > When it comes to my opinion, I am not seeing any objections to the > > feature as a whole, and I'm OK with the first step. I'm also OK to > > keep the door open for more improvements in controlling how these > > IN/ANY lists show up, but there could be more than just the number of > > items as parameter (say the query size, different behaviors depending > > on the number of clauses in queries, subquery context or CTEs/WITH, > > etc. just to name a few things coming in mind). > > Interesting point, but now it's my turn to have troubles imagining the > case, where list representation could be controlled depending on > something else than the number of elements in it. Do you have any > specific example in mind? In the current patch version I didn't add anything yet to address the question of having more parameters to tune constants merging. The main obstacle as I see it is that the information for that has to be collected when jumbling various query nodes. Anything except information about the ArrayExpr itself would have to be acquired when jumbling some other parts of the query, not directly related to the ArrayExpr. It seems to me this interdependency between otherwise unrelated nodes outweigh the value it brings, and would require some more elaborate (and more invasive for the purpose of this patch) mechanism to implement. >From 4367571d3d886a14690ba31ad0163d0d309a52a3 Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sat, 14 Oct 2023 15:00:48 +0200 Subject: [PATCH v15 1/4] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on the number of parameters, because every element of ArrayExpr is jumbled. In certain situations it's undesirable, especially if the list becomes too large. Make an array of Const expressions contribute only the first/last elements to the jumble hash. Allow to enable this behavior via the new GUC query_id_const_merge with the default value off. Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane, Michael Paquier, Sergei Kornilov, Alvaro Herrera, David Geier Tested-by: Chengxi Sun --- contrib/pg_stat_statements/Makefile | 2 +- .../pg_stat_statements/expected/merging.out | 167 ++ contrib/pg_stat_statements/meson.build| 1 + .../pg_stat_statements/pg_stat_statements.c | 34 +++- contrib/pg_stat_statements/sql/merging.sql| 58 ++ doc/src/sgml/config.sgml | 28 +++ doc/src/sgml/pgstatstatements.sgml| 25 ++- src/backend/nodes/gen_node_support.pl | 2 +- src/backend/nodes/queryjumblefuncs.c | 86 - src/backend/utils/misc/guc_tables.c | 10 ++ src/backend/utils/misc/postgresql.conf.sample | 2 +- src/include/nodes/primnodes.h | 2 + src/include/nodes/queryjumble.h | 9 +- 13 files changed, 406 insertions(+), 20 deletions(-) create mode 100644 contrib/pg_stat_statements/expected/merging.out create mode 100644 contrib/pg_stat_statements/sql/merging.sql diff --git a/contrib/pg_stat_statements/Makefile b/contrib/pg_stat_statements/Makefile index eba4a95d91a..af731fc9a58 100644 --- a/contrib/pg_stat_statements/Makefile +++ b/contrib/pg_stat_statements/Makefile @@ -19,7 +19,7 @@ LDFLAGS_SL += $(filter -lm, $(LIBS)) REGRESS_OPTS = --temp-config $(top_srcdir)/contrib/pg_stat_statements/pg_stat_statements.conf REGRESS = select dml cursors utility level_tracking planning \ - user_activity wal cleanup oldextversions + user_activity wal cleanup oldextversions merging # Di
Re: LLVM 16 (opaque pointers)
> On Fri, Oct 13, 2023 at 11:06:21AM +0200, Dmitry Dolgov wrote:
> > On Thu, Oct 12, 2023 at 04:31:20PM -0700, Andres Freund wrote:
> > I don't think the "function(no-op-function),no-op-module" bit does something
> > particularly useful?
>
> Right, looks like leftovers after verifying which passes were actually
> applied. My bad, could be removed.
>
> > I also don't think we should add the mem2reg pass outside of -O0 - running
> > it
> > after a real optimization pipeline doesn't seem useful and might even make
> > the
> > code worse? mem2reg is included in default (and obviously also in O3).
>
> My understanding was that while mem2reg is included everywhere above
> -O0, this set of passes won't hurt. But yeah, if you say it could
> degrade the final result, it's better to not do this. I'll update this
> part.
Here is what I had in mind (only this part in the second patch was changed).
>From 040121be6d150e8f18f154a64b409baef2b15ffb Mon Sep 17 00:00:00 2001
From: Thomas Munro
Date: Mon, 9 May 2022 08:27:47 +1200
Subject: [PATCH v4 1/2] jit: Support opaque pointers in LLVM 16.
Remove use of LLVMGetElementType() and provide the type of all pointers
to LLVMBuildXXX() functions when emitting IR, as required by modern LLVM
versions[1].
* For LLVM <= 14, we'll still use the old LLVMBuildXXX() functions.
* For LLVM == 15, we'll continue to do the same, explicitly opting
out of opaque pointer mode.
* For LLVM >= 16, we'll use the new LLVMBuildXXX2() functions that take
the extra type argument.
The difference is hidden behind some new IR emitting wrapper functions
l_load(), l_gep(), l_call() etc. The change is mostly mechanical,
except that at each site the correct type had to be provided.
In some places we needed to do some extra work to get functions types,
including some new wrappers for C++ APIs that are not yet exposed by in
LLVM's C API, and some new "example" functions in llvmjit_types.c
because it's no longer possible to start from the function pointer type
and ask for the function type.
Back-patch to 12, because it's a little tricker in 11 and we agreed not
to put the latest LLVM support into the upcoming final release of 11.
[1] https://llvm.org/docs/OpaquePointers.html
Reviewed-by: Dmitry Dolgov <9erthali...@gmail.com>
Reviewed-by: Ronan Dunklau
Reviewed-by: Andres Freund
Discussion: https://postgr.es/m/CA%2BhUKGKNX_%3Df%2B1C4r06WETKTq0G4Z_7q4L4Fxn5WWpMycDj9Fw%40mail.gmail.com
---
src/backend/jit/llvm/llvmjit.c| 59 ++--
src/backend/jit/llvm/llvmjit_deform.c | 119
src/backend/jit/llvm/llvmjit_expr.c | 401 --
src/backend/jit/llvm/llvmjit_types.c | 39 ++-
src/backend/jit/llvm/llvmjit_wrap.cpp | 12 +
src/backend/jit/llvm/meson.build | 2 +-
src/include/jit/llvmjit.h | 7 +
src/include/jit/llvmjit_emit.h| 106 +--
8 files changed, 481 insertions(+), 264 deletions(-)
diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index 4dfaf797432..1c8e1ab3a76 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -64,8 +64,10 @@ LLVMTypeRef TypeStorageBool;
LLVMTypeRef TypePGFunction;
LLVMTypeRef StructNullableDatum;
LLVMTypeRef StructHeapTupleData;
+LLVMTypeRef StructMinimalTupleData;
LLVMTypeRef StructTupleDescData;
LLVMTypeRef StructTupleTableSlot;
+LLVMTypeRef StructHeapTupleHeaderData;
LLVMTypeRef StructHeapTupleTableSlot;
LLVMTypeRef StructMinimalTupleTableSlot;
LLVMTypeRef StructMemoryContextData;
@@ -76,8 +78,11 @@ LLVMTypeRef StructExprState;
LLVMTypeRef StructAggState;
LLVMTypeRef StructAggStatePerGroupData;
LLVMTypeRef StructAggStatePerTransData;
+LLVMTypeRef StructPlanState;
LLVMValueRef AttributeTemplate;
+LLVMValueRef ExecEvalSubroutineTemplate;
+LLVMValueRef ExecEvalBoolSubroutineTemplate;
static LLVMModuleRef llvm_types_module = NULL;
@@ -451,11 +456,7 @@ llvm_pg_var_type(const char *varname)
if (!v_srcvar)
elog(ERROR, "variable %s not in llvmjit_types.c", varname);
- /* look at the contained type */
- typ = LLVMTypeOf(v_srcvar);
- Assert(typ != NULL && LLVMGetTypeKind(typ) == LLVMPointerTypeKind);
- typ = LLVMGetElementType(typ);
- Assert(typ != NULL);
+ typ = LLVMGlobalGetValueType(v_srcvar);
return typ;
}
@@ -467,12 +468,14 @@ llvm_pg_var_type(const char *varname)
LLVMTypeRef
llvm_pg_var_func_type(const char *varname)
{
- LLVMTypeRef typ = llvm_pg_var_type(varname);
+ LLVMValueRef v_srcvar;
+ LLVMTypeRef typ;
+
+ v_srcvar = LLVMGetNamedFunction(llvm_types_module, varname);
+ if (!v_srcvar)
+ elog(ERROR, "function %s not in llvmjit_types.c", varname);
- /* look at the contained type */
- Assert(LLVMGetTypeKind(typ) == LLVMPointerTypeKind);
- typ = LLVMGetElementType(typ);
- Assert(typ != NULL && LLVMGetTypeKind(typ) == LLVMFunctionTypeKind);
+ typ = LLVMG
Re: pg_stat_statements and "IN" conditions
> On Fri, Oct 13, 2023 at 05:07:00PM +0900, Michael Paquier wrote: > Now, it doesn't mean that this approach with the "powers" will never > happen, but based on the set of opinions I am gathering on this thread > I would suggest to rework the patch as follows: > - First implement an on/off switch that reduces the lists in IN and/or > ANY to one parameter. Simply. > - Second, refactor the powers routine. > - Third, extend the on/off switch, or just implement a threshold with > a second switch. Well, if it will help move this patch forward, why not. To clarify, I'm going to split the current implementation into three patches, one for each point you've mentioned. > When it comes to my opinion, I am not seeing any objections to the > feature as a whole, and I'm OK with the first step. I'm also OK to > keep the door open for more improvements in controlling how these > IN/ANY lists show up, but there could be more than just the number of > items as parameter (say the query size, different behaviors depending > on the number of clauses in queries, subquery context or CTEs/WITH, > etc. just to name a few things coming in mind). Interesting point, but now it's my turn to have troubles imagining the case, where list representation could be controlled depending on something else than the number of elements in it. Do you have any specific example in mind?
Re: LLVM 16 (opaque pointers)
> On Thu, Oct 12, 2023 at 04:31:20PM -0700, Andres Freund wrote:
> Hi,
>
> On 2023-10-11 21:59:50 +1300, Thomas Munro wrote:
> > +#else
> > + LLVMPassBuilderOptionsRef options;
> > + LLVMErrorRef err;
> > + int compile_optlevel;
> > + char *passes;
> > +
> > + if (context->base.flags & PGJIT_OPT3)
> > + compile_optlevel = 3;
> > + else
> > + compile_optlevel = 0;
> > +
> > + passes =
> > psprintf("default,mem2reg,function(no-op-function),no-op-module",
> > + compile_optlevel);
>
> I don't think the "function(no-op-function),no-op-module" bit does something
> particularly useful?
Right, looks like leftovers after verifying which passes were actually
applied. My bad, could be removed.
> I also don't think we should add the mem2reg pass outside of -O0 - running it
> after a real optimization pipeline doesn't seem useful and might even make the
> code worse? mem2reg is included in default (and obviously also in O3).
My understanding was that while mem2reg is included everywhere above
-O0, this set of passes won't hurt. But yeah, if you say it could
degrade the final result, it's better to not do this. I'll update this
part.
Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE
> On Wed, Sep 20, 2023 at 03:42:43PM +0200, David Geier wrote: > Another, not yet discussed, option I can see work is: > > 4. Allocate a fixed amount of memory for the instrumentation stats based on > MAX_PARALLEL_WORKER_LIMIT: MAX_PARALLEL_WORKER_LIMIT is 1024 and used as the > limit of the max_parallel_workers GUC. This way MAX_PARALLEL_WORKER_LIMIT * > sizeof(BitmapHeapScanInstrumentation) = 1024 * 8 = 8192 bytes would be > allocated. To cut this down in half we could additionally change the type of > lossy_pages and exact_pages from long to uint32. Only possibly needed memory > would have to get initialized, the remaining unused memory would remain > untouched to not waste cycles. I'm not sure that it would be acceptable -- if I understand correctly it would be 8192 bytes per parallel bitmap heap scan node, and could be noticeable in the worst case scenario with too many connections and too many such nodes in every query. I find the original approach with an offset not that bad, after all there is something similar going on in other places, e.g. parallel heap scan also has phs_snapshot_off (although the rest is fixed sized). My commentary above in the thread was mostly about the cosmetic side. Giving snapshot_and_stats a decent name and maybe even ditching the access functions, using instead only the offset field couple of times, and it would look better to me.
Re: [RFC] Add jit deform_counter
> On Fri, Sep 08, 2023 at 03:34:42PM +0200, Daniel Gustafsson wrote: > > On 5 Sep 2023, at 16:37, Daniel Gustafsson wrote: > > > I've gone over this version of the patch and I think it's ready to go in. > > I'm > > marking this Ready for Committer and will go ahead with it shortly barring > > any > > objections. > > Pushed, after another round of review with some minor fixes. Thanks!
Re: [RFC] Add jit deform_counter
> On Wed, Jul 19, 2023 at 05:18:29PM +0200, Dmitry Dolgov wrote:
> > On Tue, Jul 18, 2023, 3:32 PM Daniel Gustafsson wrote
> >> Here is the patch with the proposed variation.
> >
> > This version still leaves non-text EXPLAIN formats with timing which
> doesn't
> > add up. Below are JSON and XML examples:
>
> Good point. For the structured formats it should be represented via a nested
> level. I'll try to do this and other proposed changes as soon as I'll get
> back.
And here is it. The json version of EXPLAIN now looks like this:
"JIT": {
[...]
"Timing": {
"Generation": {
"Deform": 0.000,
"Total": 0.205
},
"Inlining": 0.065,
"Optimization": 2.465,
"Emission": 2.337,
"Total": 5.072
}
},
>From 692ec7fb6c8132626e3597b753510dd1648ebeed Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 11 Jun 2022 11:54:40 +0200
Subject: [PATCH v6 1/2] Add deform_counter
At the moment generation_counter includes time spent on both JITing
expressions and tuple deforming. Those are configured independently via
corresponding options (jit_expressions and jit_tuple_deforming), which
means there is no way to find out what fraction of time tuple deforming
alone is taking.
Add deform_counter dedicated to tuple deforming, which allows seeing
more directly the influence jit_tuple_deforming is having on the query.
---
doc/src/sgml/jit.sgml | 2 +-
src/backend/commands/explain.c | 14 +++---
src/backend/jit/jit.c | 1 +
src/backend/jit/llvm/llvmjit_expr.c | 6 ++
src/include/jit/jit.h | 3 +++
5 files changed, 22 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/jit.sgml b/doc/src/sgml/jit.sgml
index 998c972e8ba..1921557cb82 100644
--- a/doc/src/sgml/jit.sgml
+++ b/doc/src/sgml/jit.sgml
@@ -170,7 +170,7 @@ SET
JIT:
Functions: 3
Options: Inlining false, Optimization false, Expressions true, Deforming true
- Timing: Generation 1.259 ms, Inlining 0.000 ms, Optimization 0.797 ms, Emission 5.048 ms, Total 7.104 ms
+ Timing: Generation 1.259 ms (Deform 0.000 ms), Inlining 0.000 ms, Optimization 0.797 ms, Emission 5.048 ms, Total 7.104 ms
Execution Time: 7.416 ms
As visible here, JIT was used, but inlining and
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 8570b14f621..78cced83931 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -893,6 +893,7 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
/* calculate total time */
INSTR_TIME_SET_ZERO(total_time);
+ /* don't add deform_counter, it's included into generation_counter */
INSTR_TIME_ADD(total_time, ji->generation_counter);
INSTR_TIME_ADD(total_time, ji->inlining_counter);
INSTR_TIME_ADD(total_time, ji->optimization_counter);
@@ -914,14 +915,15 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
"Inlining", jit_flags & PGJIT_INLINE ? "true" : "false",
"Optimization", jit_flags & PGJIT_OPT3 ? "true" : "false",
"Expressions", jit_flags & PGJIT_EXPR ? "true" : "false",
- "Deforming", jit_flags & PGJIT_DEFORM ? "true" : "false");
+ "Deform", jit_flags & PGJIT_DEFORM ? "true" : "false");
if (es->analyze && es->timing)
{
ExplainIndentText(es);
appendStringInfo(es->str,
- "Timing: %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n",
+ "Timing: %s %.3f ms (%s %.3f ms), %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n",
"Generation", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->generation_counter),
+ "Deform", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->deform_counter),
"Inlining", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->inlining_counter),
"Optimization", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->optimization_counter),
"Emission", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->emission_counter),
@@ -945,9 +947,15 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
{
ExplainOpenGroup("Timing", "Timing", true, es);
- ExplainPropertyFloat("Generation", "ms",
+ ExplainOpenGroup("Generation", "Generation", true, es);
+ ExplainPropertyFloat("Deform", "ms",
+ 1000.0 * INSTR_TIME_GET_DOUBLE(ji->deform_counter),
+ 3, es);
+ ExplainPropertyFloat("Total", "ms",
1000.0 * INSTR_T
Re: Schema variables - new implementation for Postgres 15
> On Sat, Aug 12, 2023 at 09:28:19AM +0800, Julien Rouhaud wrote: > On Fri, Aug 11, 2023 at 05:55:26PM +0200, Dmitry Dolgov wrote: > > > > Another confusing example was this one at the end of set_session_variable: > > > > + /* > > +* XXX While unlikely, an error here is possible. It wouldn't > > leak memory > > +* as the allocated chunk has already been correctly assigned > > to the > > +* session variable, but would contradict this function > > contract, which is > > +* that this function should either succeed or leave the > > current value > > +* untouched. > > +*/ > > + elog(DEBUG1, "session variable \"%s.%s\" (oid:%u) has new > > value", > > + > > get_namespace_name(get_session_variable_namespace(svar->varid)), > > +get_session_variable_name(svar->varid), > > +svar->varid); > > > > It's not clear, which exactly error you're talking about, it's the last > > instruction in the function. > > FTR I think I'm the one that changed that. The error I was talking about is > elog() itself (in case of OOM for instance), or even one of the get_* call, if > running with log_level <= DEBUG1. It's clearly really unlikely but still > possible, thus this comment which also tries to explain why this elog() is not > done earlier. I see, thanks for clarification. Absolutely nitpicking, but the crucial "that's why this elog is not done earlier" is only assumed in the comment between the lines, not stated out loud :)
Re: Schema variables - new implementation for Postgres 15
> On Thu, Aug 03, 2023 at 08:15:13AM +0200, Pavel Stehule wrote:
> Hi
>
> fresh rebase
Thanks for continuing efforts. The new patch structure looks better to
me (although the boundary between patches 0001 and 0002 is somewhat
fuzzy, e.g. the function NameListToString is used already in the first
one, but defined in the second). Couple of commentaries along the way:
* Looks like it's common to use BKI_DEFAULT when defining catalog
entities, something like BKI_DEFAULT(-1) for typmod, BKI_DEFAULT(0) for
collation, etc. Does it make sense to put few default values into
pg_variable as well?
* The first patch contains:
diff --git a/src/backend/access/transam/xact.c
b/src/backend/access/transam/xact.c
@@ -2800,6 +2800,8 @@ AbortTransaction(void)
AtAbort_Portals();
smgrDoPendingSyncs(false, is_parallel_worker);
AtEOXact_LargeObject(false);
+
+ /* 'false' means it's abort */
AtAbort_Notify();
AtEOXact_RelationMap(false, is_parallel_worker);
AtAbort_Twophase();
What does the commentary refer to, is it needed?
* I see ExplainOneQuery got a new argument:
static void ExplainOneQuery(Query *query, int cursorOptions,
- IntoClause *into,
ExplainState *es,
+ IntoClause *into, Oid
targetvar, ExplainState *es,
const char *queryString, ParamListInfo params,
QueryEnvironment *queryEnv);
>From what I understand it represents a potential session variable to be
explained. Isn't it too specific for this interface, could it be put
somewhere else? To be honest, I don't have any suggestions myself, but
it feels a bit out of place here.
* Session variable validity logic is not always clear, at least to me,
producing following awkward pieces of code:
+ if (!svar->is_valid)
+ {
+ if (is_session_variable_valid(svar))
+ svar->is_valid = true;
I get it as there are two ways how a variable could be invalid?
* It's not always easy to follow which failure modes are taken care of. E.g.
+* Don't try to use possibly invalid data from svar. And we don't want
to
+* overwrite invalid svar immediately. The datumCopy can fail, and in
this
+* case, the stored value will be invalid still.
I couldn't find any similar precautions, how exactly datumCopy can fail,
are you referring to palloc/memcpy failures?
Another confusing example was this one at the end of set_session_variable:
+ /*
+* XXX While unlikely, an error here is possible. It wouldn't leak
memory
+* as the allocated chunk has already been correctly assigned to the
+* session variable, but would contradict this function contract, which
is
+* that this function should either succeed or leave the current value
+* untouched.
+*/
+ elog(DEBUG1, "session variable \"%s.%s\" (oid:%u) has new value",
+
get_namespace_name(get_session_variable_namespace(svar->varid)),
+get_session_variable_name(svar->varid),
+svar->varid);
It's not clear, which exactly error you're talking about, it's the last
instruction in the function.
Maybe it would be beneficial to have some overarching description, all
in one place, about how session variables implementation handles various
failures?
Re: [RFC] Clang plugin for catching suspicious typedef casting
> On Wed, Aug 09, 2023 at 03:23:32PM +0200, Peter Eisentraut wrote: > On 03.08.23 18:56, Dmitry Dolgov wrote: > > I would like to get your opinion, folks. Does it sound interesting > > enough for the community, is it worth it to pursue the idea? > > I think it's interesting, and doesn't look too invasive. > > Maybe we can come up with three or four interesting use cases and try to > implement them. BlockNumber vs. Buffer type checking is obviously a bit > marginal to get anyone super-excited, but it's a reasonable demo. > > Also, how stable is the plugin API? Would we need to keep updating these > plugins for each new clang version? >From the first glance the API itself seems to be stable -- I didn't find many breaking changes for plugins in the release notes over the last five releases. The git history for such definitions as ASTConsumer or RecursiveASTVisitor also doesn't seem to be very convoluted. But libreoffice example shows, that some updating would be necessary -- they've got about a dozen of places in the code that depend on the clang version. From what I see it's usually not directly related to the plugin API, and looks more like our opaque pointers issue.
Re: [RFC] Clang plugin for catching suspicious typedef casting
> On Thu, Aug 03, 2023 at 12:23:52PM -0500, Tristan Partin wrote: > > This is the first I am learning about clang plugins. Interesting concept. > Did you give any thought to using libclang instead of a compiler plugin? I > am kind of doing similar work, but I went with libclang instead of a plugin. Nope, never thought about trying libclang. From the clang documentation it seems a plugin is a suitable interface if one wants to: special lint-style warnings or errors for your project Which is what I was trying to achieve. Are there any advantages of libclang that you see?
[RFC] Clang plugin for catching suspicious typedef casting
Hi,
In the commit [1] one side change was to fix mixed up usage of
BlockNumber and Buffer variables. This was a one-off manual effort, and
indeed it hardly seems possible to get compilation warnings for such
code, as long as the underlying types could be converted in a standard
conforming way. As far as I see such conversions are acceptable and
used every now and then in the project, but they may hide some subtle
issues.
One interesting way I found to address this was to benefit from clang
plugin system [2]. A clang plugin allows you to run some frontend
actions when compiling code with clang, and this approach is used in
some large open source projects (e.g. I've got my inspiration largely
from libreoffice [3]). After a little experimenting I could teach such a
plugin to warn about situations similar to the one described above. What
it does is:
* It visits all the function call expressions
* Verify if the function arguments are defined via typedef
* Compare the actual argument with the function declaration
* Consult with the suppression rules to minimize false positives
In the end the plugin catches the error mentioned above, and we get
something like this:
ginget.c:143:41: warning: Typedef check: Expected 'BlockNumber' (aka
'unsigned int'),
got 'Buffer' (aka 'int') in PredicateLockPage
PredicateLockPage(btree->index, stack->buffer, snapshot);
Of course the plugin produces more warning, and I haven't checked all of
them yet -- some probably would have to be ignored as well. But in the
long run I'm pretty confident it's possible to fine tune this logic and
suppression rules to get minimum noise.
As I see it, there are advantages of using plugins in such way:
* Helps automatically detect some issues
* Other type of plugins could be useful to support large undertakings,
where a lot of code transformation is involved.
And of course disadvantages as well:
* It has to be fine-tuned to be useful
* It's compiler dependent, not clear how to always exercise it
I would like to get your opinion, folks. Does it sound interesting
enough for the community, is it worth it to pursue the idea?
Some final notes about infrastructure bits. Never mind cmake in there --
it was just for a quick start, I'm going to convert it to something
else. There are some changes needed to tell the compiler to actually
load the plugin, those of course could be done much better as well. I
didn't do anything with meson here, because it turns out meson tends to
enclose the plugin file with '-Wl,--start-group ... -Wl,--end-group' and
it breaks the plugin discovery.
[1]:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=126552c85c1cfb6ce6445159b8024cfa5631f33e
[2]: https://clang.llvm.org/docs/ClangPlugins.html
[3]: https://git.libreoffice.org/core/+/refs/heads/master/compilerplugins/clang/
>From 01e645f456f9476c60943f12d794465567f2d265 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Thu, 3 Aug 2023 15:51:26 +0200
Subject: [PATCH] Typedef-check clang plugin
Add a clang plugin to warn about suspicious type casting for types
defined via typedef, e.g. BlockNumber and Buffer.
---
configure | 36
configure.ac | 12 ++
src/tools/clang/typedef_check/CMakeLists.txt | 31 +++
.../clang/typedef_check/TypedefCheck.cpp | 183 ++
4 files changed, 262 insertions(+)
create mode 100644 src/tools/clang/typedef_check/CMakeLists.txt
create mode 100644 src/tools/clang/typedef_check/TypedefCheck.cpp
diff --git a/configure b/configure
index 2e518c8007d..71da30779de 100755
--- a/configure
+++ b/configure
@@ -767,6 +767,7 @@ enable_coverage
GENHTML
LCOV
GCOV
+enable_typedef_check
enable_debug
enable_rpath
default_port
@@ -835,6 +836,7 @@ enable_rpath
enable_spinlocks
enable_atomics
enable_debug
+enable_typedef_check
enable_profiling
enable_coverage
enable_dtrace
@@ -1528,6 +1530,7 @@ Optional Features:
--disable-spinlocks do not use spinlocks
--disable-atomics do not use atomic operations
--enable-debug build with debugging symbols (-g)
+ --enable-typedef-check build with typedef-check plugin
--enable-profiling build with profiling enabled
--enable-coverage build with coverage testing instrumentation
--enable-dtrace build with DTrace support
@@ -3344,6 +3347,34 @@ fi
+#
+# --enable-typedef-check adds clang typedef-check plugin
+#
+
+
+# Check whether --enable-typedef-check was given.
+if test "${enable_typedef_check+set}" = set; then :
+ enableval=$enable_typedef_check;
+ case $enableval in
+yes)
+ :
+ ;;
+no)
+ :
+ ;;
+*)
+ as_fn_error $? "no argument expected for --enable-typedef-check option" "$LINENO" 5
+ ;;
+ esac
+
+else
+ enable_typedef_check=no
+
+fi
+
+
+
+
#
# --enable-profiling enables gcc profiling
#
@@ -7828,6 +7859,11 @@ if test "$enable_debug" = yes && test
Re: [RFC] Add jit deform_counter
> On Tue, Jul 18, 2023, 3:32 PM Daniel Gustafsson wrote >> Here is the patch with the proposed variation. > > This version still leaves non-text EXPLAIN formats with timing which doesn't > add up. Below are JSON and XML examples: Good point. For the structured formats it should be represented via a nested level. I'll try to do this and other proposed changes as soon as I'll get back.
Re: pg_stat_statements and "IN" conditions
> On Mon, Jul 03, 2023 at 09:46:11PM -0700, Nathan Bossart wrote: Thanks for reviewing. > On Sun, Mar 19, 2023 at 01:27:34PM +0100, Dmitry Dolgov wrote: > > +If this parameter is on, two queries with an array will get the > > same > > +query identifier if the only difference between them is the number > > of > > +constants, both numbers is of the same order of magnitude and > > greater or > > +equal 10 (so the order of magnitude is greather than 1, it is not > > worth > > +the efforts otherwise). > > IMHO this adds way too much complexity to something that most users would > expect to be an on/off switch. This documentation is exclusively to be precise about how does it work. Users don't have to worry about all this, and pretty much turn it on/off, as you've described. I agree though, I could probably write this text a bit differently. > If I understand Álvaro's suggestion [0] correctly, he's saying that in > addition to allowing "on" and "off", it might be worth allowing > something like "powers" to yield roughly the behavior described above. > I don't think he's suggesting that this "powers" behavior should be > the only available option. Independently of what Álvaro was suggesting, I find the "powers" approach more suitable, because it answers my own concerns about the previous implementation. Having "on"/"off" values means we would have to scratch heads coming up with a one-size-fit-all default value, or to introduce another option for the actual cut-off threshold. I would like to avoid both of those options, that's why I went with "powers" only. > Also, it seems counterintuitive that queries with fewer than 10 > constants are not merged. Why? What would be your intuition using this feature? > In the interest of moving this patch forward, I would suggest making it a > simple on/off switch in 0002 and moving the "powers" functionality to a new > 0003 patch. I think separating out the core part of this feature might > help reviewers. As you can see, I got distracted by the complicated > threshold logic and ended up focusing my first round of review there. I would disagree. As I've described above, to me "powers" seems to be a better fit, and the complicated logic is in fact reusing one already existing function. Do those arguments sound convincing to you?
Re: Let's make PostgreSQL multi-threaded
> On Mon, Jun 05, 2023 at 06:43:54PM +0300, Heikki Linnakangas wrote: > On 05/06/2023 11:28, Tristan Partin wrote: > > > # Exposed PIDs > > > > > > We expose backend process PIDs to users in a few places. > > > pg_stat_activity.pid and pg_terminate_backend(), for example. They need > > > to be replaced, or we can assign a fake PID to each connection when > > > running in multi-threaded mode. > > > > Would it be possible to just transparently slot in the thread ID > > instead? > > Perhaps. It might break applications that use the PID directly with e.g. > 'kill ', though. I think things are getting more interesting if some external resource accounting like cgroups is taking place. From what I know cgroup v2 has only few controllers that allow threaded granularity, and memory or io controllers are not part of this list. Since Postgres is doing quite a lot of different things, sometimes it makes sense to put different limitations on different types of activity, e.g. to give more priority to a certain critical internal job on the account of slowing down backends. In the end it might be complicated or not possible to do that for individual threads. Such cases are probably not very important from the high level point of view, but could become an argument when deciding what should be a process and what should be a thread.
Re: LLVM 16 (opaque pointers)
> On Mon, May 22, 2023 at 03:38:44PM +1200, Thomas Munro wrote:
> Further changes are already needed for their "main" branch (LLVM
> 17-to-be), so this won't quite be enough to shut seawasp up. At a
> glance, we will need to change from the "old pass manager" API that
> has recently been vaporised[1]
> (llvm-c/Transforms/PassManagerBuilder.h) to the new one[2][3]
> (llvm-c/Transforms/PassBuilder.h), which I suspect/hope will be as
> simple as changing llvmjit.c to call LLVMRunPasses() with a string
> describing the passes we want in "opt -passes" format, instead of our
> code that calls LLVMAddFunctionInlingPass() etc. But that'll be a
> topic for another day, and another thread.
>
> [1]
> https://github.com/llvm/llvm-project/commit/0aac9a2875bad4f065367e4a6553fad78605f895
> [2] https://llvm.org/docs/NewPassManager.html
> [3] https://reviews.llvm.org/D102136
Thanks for tackling the topic! I've tested it with a couple of versions,
LLVM 12 that comes with my Gentoo box, LLVM 15 build from sources and
the modified version of patch adopted for LLVM 17 (build form sources as
well). In all three cases everything seems to be working fine.
Simple benchmarking with a query stolen from some other jit thread
(pgbench running single client with multiple unions of selects a-la
SELECT a, count(*), sum(b) FROM test WHERE c = 2 GROUP BY a) show some
slight performance differences, but nothing dramatic so far. LLVM 17
version produces the lowest latency, with faster generation, inlining
and optimization, but slower emission time. LLVM 12 version produces the
largest latencies with everything except emission timings being slower.
LLVM 15 is somewhere in between.
I'll continue reviewing and, for the records, attach adjustments I was
using for LLVM 17 (purely for testing, not taking into account other
versions), in case if I've missed something.
>From 33e39a376fdbcceb6d4e757f4a798b3922e7068c Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sun, 4 Jun 2023 11:10:41 +0200
Subject: [PATCH 2/2] LLVM 17
---
src/backend/jit/llvm/llvmjit.c| 73 ---
src/backend/jit/llvm/llvmjit_wrap.cpp | 4 ++
2 files changed, 25 insertions(+), 52 deletions(-)
diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index b83bc04ae3a..b701e167abd 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -18,6 +18,9 @@
#include
#include
#include
+#if LLVM_VERSION_MAJOR > 16
+#include
+#endif
#if LLVM_VERSION_MAJOR > 11
#include
#include
@@ -27,12 +30,14 @@
#endif
#include
#include
+#if LLVM_VERSION_MAJOR < 17
#include
#include
#include
#if LLVM_VERSION_MAJOR > 6
#include
#endif
+#endif
#include "jit/llvmjit.h"
#include "jit/llvmjit_emit.h"
@@ -559,69 +564,33 @@ llvm_function_reference(LLVMJitContext *context,
static void
llvm_optimize_module(LLVMJitContext *context, LLVMModuleRef module)
{
- LLVMPassManagerBuilderRef llvm_pmb;
- LLVMPassManagerRef llvm_mpm;
- LLVMPassManagerRef llvm_fpm;
- LLVMValueRef func;
+ LLVMPassBuilderOptionsRef options;
+ LLVMErrorRef err;
int compile_optlevel;
+ char *passes;
if (context->base.flags & PGJIT_OPT3)
compile_optlevel = 3;
else
compile_optlevel = 0;
- /*
- * Have to create a new pass manager builder every pass through, as the
- * inliner has some per-builder state. Otherwise one ends up only inlining
- * a function the first time though.
- */
- llvm_pmb = LLVMPassManagerBuilderCreate();
- LLVMPassManagerBuilderSetOptLevel(llvm_pmb, compile_optlevel);
- llvm_fpm = LLVMCreateFunctionPassManagerForModule(module);
+ passes = psprintf("default,mem2reg,function(no-op-function),no-op-module",
+ compile_optlevel);
- if (context->base.flags & PGJIT_OPT3)
- {
- /* TODO: Unscientifically determined threshold */
- LLVMPassManagerBuilderUseInlinerWithThreshold(llvm_pmb, 512);
- }
- else
- {
- /* we rely on mem2reg heavily, so emit even in the O0 case */
- LLVMAddPromoteMemoryToRegisterPass(llvm_fpm);
- }
+ options = LLVMCreatePassBuilderOptions();
- LLVMPassManagerBuilderPopulateFunctionPassManager(llvm_pmb, llvm_fpm);
+#ifdef LLVM_PASS_DEBUG
+ LLVMPassBuilderOptionsSetDebugLogging(options, 1);
+#endif
- /*
- * Do function level optimization. This could be moved to the point where
- * functions are emitted, to reduce memory usage a bit.
- */
- LLVMInitializeFunctionPassManager(llvm_fpm);
- for (func = LLVMGetFirstFunction(context->module);
- func != NULL;
- func = LLVMGetNextFunction(func))
- LLVMRunFunctionPassManager(llvm_fpm, func);
- LLVMFinalizeFunctionPassManager(llvm_fpm);
- LLVMDisposePassManager(llvm_fpm);
+ LLVMPassBuilderOptionsSetInlinerThreshold(options, 512);
- /*
- * Perform module level optimization. We do so even in the non-optimized
- * case, so always-inline functions etc get inlined. It's cheap enough.
- */
- llvm_mpm = LLVMCreatePassManager();
- LLVMPassManagerBuilderPopulateModulePassManager(llvm_pmb,
-
Re: xmlserialize bug - extra empty row at the end
> On Sun, Apr 23, 2023 at 02:02:17PM +0200, Jim Jones wrote:
> On 23.04.23 07:31, Pavel Stehule wrote:
> > Hi
> >
> > maybe I found a bug in xmlserialize
> >
> > SELECT xmlserialize(DOCUMENT '42'
> > AS varchar INDENT);
> >
> > (2023-04-23 07:27:53) postgres=# SELECT xmlserialize(DOCUMENT
> > '42' AS varchar INDENT);
> > ┌─┐
> > │ xmlserialize │
> > ╞═╡
> > │ ↵│
> > │ ↵│
> > │ 42↵│
> > │ ↵│
> > │ ↵│
> > │ │
> > └─┘
> > (1 row)
> >
> > Looks so there is an extra empty row.
> >
> > Regards
> >
> > Pavel
>
> Hi Pavel,
>
> Good catch! It looks like it comes directly from libxml2.
>
> xmlDocPtr doc = xmlReadDoc(BAD_CAST " x=\"y\">42", NULL, NULL, 0 );
> xmlBufferPtr buf = NULL;
> xmlSaveCtxtPtr ctxt = NULL;
>
> buf = xmlBufferCreate();
> ctxt = xmlSaveToBuffer(buf, NULL, XML_SAVE_NO_DECL | XML_SAVE_FORMAT);
>
> xmlSaveDoc(ctxt, doc);
> xmlSaveClose(ctxt);
>
> printf("'%s'",buf->content);
>
> ==>
>
> '
>
> 42
>
>
> '
It looks like this happens only if xml type is DOCUMENT, and thus
xmlSaveDoc is used to save the doc directly. I might be wrong, but after
a quick look at the corresponding libxml functionality it seems that a
new line is getting added if the element type is not XML_XINCLUDE_{START|END},
which is unfortunate if correct.
Re: [RFC] Add jit deform_counter
> On Fri, Mar 31, 2023 at 07:39:27PM +0200, Dmitry Dolgov wrote:
> > On Wed, Mar 29, 2023 at 01:50:37PM +1300, David Rowley wrote:
> > On Sun, 12 Jun 2022 at 21:14, Dmitry Dolgov <9erthali...@gmail.com> wrote:
> > > I've noticed that JIT performance counter generation_counter seems to
> > > include
> > > actions, relevant for both jit_expressions and jit_tuple_deforming
> > > options. It
> > > means one can't directly see what is the influence of jit_tuple_deforming
> > > alone, which would be helpful when adjusting JIT options. To make it
> > > better a
> > > new counter can be introduced, does it make sense?
> >
> > I'm not so sure about this idea. As of now, if I look at EXPLAIN
> > ANALYZE's JIT summary, the individual times add up to the total time.
> >
> > If we add this deform time, then that's no longer going to be true as
> > the "Generation" time includes the newly added deform time.
> >
> > master:
> > JIT:
> >Functions: 600
> >Options: Inlining false, Optimization false, Expressions true, Deforming
> > true
> >Timing: Generation 37.758 ms, Inlining 0.000 ms, Optimization 6.736
> > ms, Emission 172.244 ms, Total 216.738 ms
> >
> > 37.758 + 6.736 + 172.244 = 216.738
> >
> > I think if I was a DBA wondering why JIT was taking so long, I'd
> > probably either be very astonished or I'd report a bug if I noticed
> > that all the individual component JIT times didn't add up to the total
> > time.
> >
> > I don't think the solution is to subtract the deform time from the
> > generation time either.
> >
> > Can't users just get this by looking at EXPLAIN ANALYZE with and
> > without jit_tuple_deforming?
>
> It could be done this way, but then users need to know that tuple
> deforming is included into generation time (I've skimmed through the
> docs, there seems to be no direct statements about that, although it
> could be guessed). At the same time I don't think it's very
> user-friendly approach -- after all it could be the same for other
> timings, i.e. only one counter for all JIT operations present,
> expecting users to experiment how would it change if this or that option
> will be different.
>
> I agree about adding up to the total time though. What about changing
> the format to something like this?
>
>Options: Inlining false, Optimization false, Expressions true, Deforming
> true
>Timing: Generation 37.758 ms (Deforming 1.234 ms), Inlining 0.000 ms,
> Optimization 6.736 ms, Emission 172.244 ms, Total 216.738 ms
>
> This way it doesn't look like deforming timing is in the same category
> as others, but rather a part of another value.
Here is the patch with the proposed variation.
>From 963b9a31f2241cfff766544685709d813145f33a Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 11 Jun 2022 11:54:40 +0200
Subject: [PATCH v5 1/2] Add deform_counter
At the moment generation_counter includes time spent on both JITing
expressions and tuple deforming. Those are configured independently via
corresponding options (jit_expressions and jit_tuple_deforming), which
means there is no way to find out what fraction of time tuple deforming
alone is taking.
Add deform_counter dedicated to tuple deforming, which allows seeing
more directly the influence jit_tuple_deforming is having on the query.
---
src/backend/commands/explain.c | 7 ++-
src/backend/jit/jit.c | 1 +
src/backend/jit/llvm/llvmjit_expr.c | 6 ++
src/include/jit/jit.h | 3 +++
4 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 5334c503e1..a134411209 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -893,6 +893,7 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
/* calculate total time */
INSTR_TIME_SET_ZERO(total_time);
+ /* don't add deform_counter, it's included into generation_counter */
INSTR_TIME_ADD(total_time, ji->generation_counter);
INSTR_TIME_ADD(total_time, ji->inlining_counter);
INSTR_TIME_ADD(total_time, ji->optimization_counter);
@@ -920,8 +921,9 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
{
ExplainIndentText(es);
appendStringInfo(es->str,
- "Timing: %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n",
+ "Timing: %s %.3f ms (%s %.3f ms), %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n",
"Generation", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->generation_counter),
+ "Deforming", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->deform_count
Re: Schema variables - new implementation for Postgres 15
> On Tue, Mar 28, 2023 at 09:34:20PM +0200, Pavel Stehule wrote: > Hi > > > Talking about documentation I've noticed that the implementation > > contains few limitations, that are not mentioned in the docs. Examples > > are WITH queries: > > > > WITH x AS (LET public.svar = 100) SELECT * FROM x; > > ERROR: LET not supported in WITH query > > > > The LET statement doesn't support the RETURNING clause, so using inside > CTE does not make any sense. > > Do you have some tips, where this behaviour should be mentioned? Yeah, you're right, it's probably not worth adding. I usually find it a good idea to explicitly mention any limitations, but WITH docs are actually have one line about statements without the RETURNING clause, plus indeed for LET it makes even less sense. > > and using with set-returning functions (haven't found any related tests). > > > > There it is: > > +CREATE VARIABLE public.svar AS int; > +-- should be ok > +LET public.svar = generate_series(1, 1); > +-- should fail > +LET public.svar = generate_series(1, 2); > +ERROR: expression returned more than one row > +LET public.svar = generate_series(1, 0); > +ERROR: expression returned no rows > +DROP VARIABLE public.svar; Oh, interesting. I was looking for another error message from parse_func.c: set-returning functions are not allowed in LET assignment expression Is this one you've posted somehow different? > > Another small note is about this change in the rowsecurity: > > > > /* > > -* For SELECT, UPDATE and DELETE, add security quals to enforce > > the USING > > -* policies. These security quals control access to existing > > table rows. > > -* Restrictive policies are combined together using AND, and > > permissive > > -* policies are combined together using OR. > > +* For SELECT, LET, UPDATE and DELETE, add security quals to > > enforce the > > +* USING policies. These security quals control access to > > existing table > > +* rows. Restrictive policies are combined together using AND, and > > +* permissive policies are combined together using OR. > > */ > > > > From this commentary one may think that LET command supports row level > > security, but I don't see it being implemented. A wrong commentary? > > > > I don't think so. The row level security should be supported. I tested it > on example from doc: > > [...] > > (2023-03-28 21:32:33) postgres=# set role to t1role; > SET > (2023-03-28 21:32:40) postgres=# select * from accounts ; > ┌─┬─┬┐ > │ manager │ company │ contact_email │ > ╞═╪═╪╡ > │ t1role │ xxx │ t1r...@xxx.org │ > └─┴─┴┘ > (1 row) > > (2023-03-28 21:32:45) postgres=# let v = (select company from accounts); > LET > (2023-03-28 21:32:58) postgres=# select v; > ┌─┐ > │ v │ > ╞═╡ > │ xxx │ > └─┘ > (1 row) > > (2023-03-28 21:33:03) postgres=# set role to default; > SET > (2023-03-28 21:33:12) postgres=# set role to t2role; > SET > (2023-03-28 21:33:19) postgres=# select * from accounts ; > ┌─┬─┬┐ > │ manager │ company │ contact_email │ > ╞═╪═╪╡ > │ t2role │ yyy │ t2r...@yyy.org │ > └─┴─┴┘ > (1 row) > > (2023-03-28 21:33:22) postgres=# let v = (select company from accounts); > LET > (2023-03-28 21:33:26) postgres=# select v; > ┌─┐ > │ v │ > ╞═╡ > │ yyy │ > └─┘ > (1 row) Hm, but isn't the row level security enforced here on the select level, not when assigning some value via LET? Plus, it seems the comment originally refer to the command types (CMD_SELECT, etc), and there is no CMD_LET and no need for it, right? I'm just trying to understand if there was anything special done for session variables in this regard, and if not, the commentary change seems to be not needed (I know, I know, it's totally nitpicking).
Re: [RFC] Add jit deform_counter
> On Wed, Mar 29, 2023 at 01:50:37PM +1300, David Rowley wrote: > On Sun, 12 Jun 2022 at 21:14, Dmitry Dolgov <9erthali...@gmail.com> wrote: > > I've noticed that JIT performance counter generation_counter seems to > > include > > actions, relevant for both jit_expressions and jit_tuple_deforming options. > > It > > means one can't directly see what is the influence of jit_tuple_deforming > > alone, which would be helpful when adjusting JIT options. To make it better > > a > > new counter can be introduced, does it make sense? > > I'm not so sure about this idea. As of now, if I look at EXPLAIN > ANALYZE's JIT summary, the individual times add up to the total time. > > If we add this deform time, then that's no longer going to be true as > the "Generation" time includes the newly added deform time. > > master: > JIT: >Functions: 600 >Options: Inlining false, Optimization false, Expressions true, Deforming > true >Timing: Generation 37.758 ms, Inlining 0.000 ms, Optimization 6.736 > ms, Emission 172.244 ms, Total 216.738 ms > > 37.758 + 6.736 + 172.244 = 216.738 > > I think if I was a DBA wondering why JIT was taking so long, I'd > probably either be very astonished or I'd report a bug if I noticed > that all the individual component JIT times didn't add up to the total > time. > > I don't think the solution is to subtract the deform time from the > generation time either. > > Can't users just get this by looking at EXPLAIN ANALYZE with and > without jit_tuple_deforming? It could be done this way, but then users need to know that tuple deforming is included into generation time (I've skimmed through the docs, there seems to be no direct statements about that, although it could be guessed). At the same time I don't think it's very user-friendly approach -- after all it could be the same for other timings, i.e. only one counter for all JIT operations present, expecting users to experiment how would it change if this or that option will be different. I agree about adding up to the total time though. What about changing the format to something like this? Options: Inlining false, Optimization false, Expressions true, Deforming true Timing: Generation 37.758 ms (Deforming 1.234 ms), Inlining 0.000 ms, Optimization 6.736 ms, Emission 172.244 ms, Total 216.738 ms This way it doesn't look like deforming timing is in the same category as others, but rather a part of another value.
Re: Schema variables - new implementation for Postgres 15
> On Sun, Mar 26, 2023 at 07:32:05PM +0800, Julien Rouhaud wrote: > Hi, > > I just have a few minor wording improvements for the various comments / > documentation you quoted. Talking about documentation I've noticed that the implementation contains few limitations, that are not mentioned in the docs. Examples are WITH queries: WITH x AS (LET public.svar = 100) SELECT * FROM x; ERROR: LET not supported in WITH query and using with set-returning functions (haven't found any related tests). Another small note is about this change in the rowsecurity: /* -* For SELECT, UPDATE and DELETE, add security quals to enforce the USING -* policies. These security quals control access to existing table rows. -* Restrictive policies are combined together using AND, and permissive -* policies are combined together using OR. +* For SELECT, LET, UPDATE and DELETE, add security quals to enforce the +* USING policies. These security quals control access to existing table +* rows. Restrictive policies are combined together using AND, and +* permissive policies are combined together using OR. */ >From this commentary one may think that LET command supports row level security, but I don't see it being implemented. A wrong commentary?
Re: Schema variables - new implementation for Postgres 15
> On Fri, Mar 24, 2023 at 08:04:08AM +0100, Pavel Stehule wrote: > čt 23. 3. 2023 v 19:54 odesílatel Pavel Stehule > napsal: > > > čt 23. 3. 2023 v 16:33 odesílatel Peter Eisentraut < > > peter.eisentr...@enterprisedb.com> napsal: > > > >> The other issue is that by its nature this patch adds a lot of code in a > >> lot of places. Large patches are more likely to be successful if they > >> add a lot of code in one place or smaller amounts of code in a lot of > >> places. But this patch does both and it's just overwhelming. There is > >> so much new internal functionality and terminology. Variables can be > >> created, registered, initialized, stored, copied, prepared, set, freed, > >> removed, released, synced, dropped, and more. I don't know if anyone > >> has actually reviewed all that in detail. > >> > >> Has any effort been made to make this simpler, smaller, reduce scope, > >> refactoring, find commonalities with other features, try to manage the > >> complexity somehow? > >> > > I agree that this patch is large, but almost all code is simple. Complex > > code is "only" in 0002-session-variables.patch (113KB/438KB). > > > > Now, I have no idea how the functionality can be sensibly reduced or > > divided (no without significant performance loss). I see two difficult > > points in this code: > > > > 1. when to clean memory. The code implements cleaning very accurately - > > and this is unique in Postgres. Partially I implement some functionality of > > storage manager. Probably no code from Postgres can be reused, because > > there is not any support for global temporary objects. Cleaning based on > > sinval messages processing is difficult, but there is nothing else. The > > code is a little bit more complex, because there are three types of session > > variables: a) session variables, b) temp session variables, c) session > > variables with transaction scope. Maybe @c can be removed, and maybe we > > don't need to support not null default (this can simplify initialization). > > What do you think about it? > > > > 2. how to pass a variable's value to the executor. The implementation is > > based on extending the Param node, but it cannot reuse query params buffers > > and implements own. > > But it is hard to simplify code, because we want to support usage > > variables in queries, and usage in PL/pgSQL expressions too. And both are > > processed differently. > > > > Maybe I can divide the patch 0002-session-variables to three sections - > related to memory management, planning and execution? I agree, the patch scale is a bit overwhelming. It's worth noting that due to the nature of this change certain heavy lifting has to be done in any case, plus I've got an impression that some part of the patch are quite solid (although I haven't reviewed everything, did anyone achieve that milestone?). But still, it would be of great help to simplify the current implementation, and I'm afraid the only way of doing this is to make trades-off about functionality vs change size & complexity. Maybe instead splitting the patch into implementation components, it's possible to split it feature-by-feature, where every single patch would represent an independent (to a certain degree) functionality? I have in mind something like: catalog changes; base implementation; ACL support; xact actions implementation (on commit drop, etc); variables with default value; shadowing; etc. If such approach is possible, it will give us: flexibility to apply only a subset of the whole patch series; some understanding how much complexity is coming from each feature. What do you think about this idea? I also recall somewhere earlier in the thread Pavel has mentioned that a transactional version of session variables patch would be actually simpler, and he has plans to implement it later on. Is there another trade-off on the table we could think of, transactional vs non-transactional session variables?
Re: pg_stat_statements and "IN" conditions
> On Tue, Mar 14, 2023 at 08:04:32PM +0100, Dmitry Dolgov wrote:
> > On Tue, Mar 14, 2023 at 02:14:17PM -0400, Gregory Stark (as CFM) wrote:
> > So I was seeing that this patch needs a rebase according to cfbot.
>
> Yeah, folks are getting up to speed in with pgss improvements recently.
> Thanks for letting me know.
Following recent refactoring of pg_stat_statements tests, I've created a
new one for merging functionality in the patch. This should solve any
conflicts.
>From 4f8ad43c1f0547913e5da8fa97fe08bccf06c91d Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 17 Feb 2023 10:17:55 +0100
Subject: [PATCH v14 1/2] Reusable decimalLength functions
Move out decimalLength functions to reuse in the following patch.
---
src/backend/utils/adt/numutils.c | 50 +---
src/include/utils/numutils.h | 67
2 files changed, 68 insertions(+), 49 deletions(-)
create mode 100644 src/include/utils/numutils.h
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index 471fbb7ee6..df7418cce7 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,9 +18,8 @@
#include
#include
-#include "common/int.h"
#include "utils/builtins.h"
-#include "port/pg_bitutils.h"
+#include "utils/numutils.h"
/*
* A table of all two-digit numbers. This is used to speed up decimal digit
@@ -38,53 +37,6 @@ static const char DIGIT_TABLE[200] =
"80" "81" "82" "83" "84" "85" "86" "87" "88" "89"
"90" "91" "92" "93" "94" "95" "96" "97" "98" "99";
-/*
- * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
- */
-static inline int
-decimalLength32(const uint32 v)
-{
- int t;
- static const uint32 PowersOfTen[] = {
- 1, 10, 100,
- 1000, 1, 10,
- 100, 1000, 1,
- 10
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
-static inline int
-decimalLength64(const uint64 v)
-{
- int t;
- static const uint64 PowersOfTen[] = {
- UINT64CONST(1), UINT64CONST(10),
- UINT64CONST(100), UINT64CONST(1000),
- UINT64CONST(1), UINT64CONST(10),
- UINT64CONST(100), UINT64CONST(1000),
- UINT64CONST(1), UINT64CONST(10),
- UINT64CONST(100), UINT64CONST(1000),
- UINT64CONST(1), UINT64CONST(10),
- UINT64CONST(100), UINT64CONST(1000),
- UINT64CONST(1), UINT64CONST(10),
- UINT64CONST(100), UINT64CONST(1000)
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
- */
- t = (pg_leftmost_one_pos64(v) + 1) * 1233 / 4096;
- return t + (v >= PowersOfTen[t]);
-}
-
static const int8 hexlookup[128] = {
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
-1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1,
diff --git a/src/include/utils/numutils.h b/src/include/utils/numutils.h
new file mode 100644
index 00..876e64f2df
--- /dev/null
+++ b/src/include/utils/numutils.h
@@ -0,0 +1,67 @@
+/*-
+ *
+ * numutils.h
+ * Decimal length functions for numutils.c
+ *
+ *
+ * Portions Copyright (c) 1996-2023, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/utils/numutils.h
+ *
+ *-
+ */
+#ifndef NUMUTILS_H
+#define NUMUTILS_H
+
+#include "common/int.h"
+#include "port/pg_bitutils.h"
+
+/*
+ * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
+ */
+static inline int
+decimalLength32(const uint32 v)
+{
+ int t;
+ static const uint32 PowersOfTen[] = {
+ 1, 10, 100,
+ 1000, 1, 10,
+ 100, 1000, 1,
+ 10
+ };
+
+ /*
+ * Compute base-10 logarithm by dividing the base-2 logarithm by a
+ * good-enough approximation of the base-2 logarithm of 10
+ */
+ t = (pg_leftmost_one_pos32(v) + 1) * 1233 / 4096;
+ return t + (v >= PowersOfTen[t]);
+}
+
+static inline int
+decimalLength64(const uint64 v)
+{
+ int t;
+ static const uint64 PowersOfTen[] = {
+ UINT64CONST(1), UINT64CONST(10),
+ UINT64CONST(100), UINT64CONST(1000),
+ UINT64CONST(1), UINT64CONST(10),
+ UINT64CONST(100), UINT64CONST(1000),
+ UINT64CON
Re: Parallel Bitmap Heap Scan reports per-worker stats in EXPLAIN ANALYZE
> On Tue, Feb 21, 2023 at 01:02:35PM +0100, David Geier wrote: > Hi, > > On 1/20/23 09:34, David Geier wrote: > > EXPLAIN ANALYZE for parallel Bitmap Heap Scans currently only reports > > the number of heap blocks processed by the leader. It's missing the > > per-worker stats. The attached patch adds that functionality in the > > spirit of e.g. Sort or Memoize. Here is a simple test case and the > > EXPLAIN ANALYZE output with and without the patch: > > Attached is a rebased version of the patch. I would appreciate someone > taking a look. > > As background: the change doesn't come out of thin air. We repeatedly took > wrong conclusions in our query analysis because we assumed that the reported > block counts include the workers. > > If no one objects I would also register the patch at the commit fest. The > patch is passing cleanly on CI. Thanks for the patch. The idea sounds reasonable to me, but I have to admit snapshot_and_stats implementation looks awkward. Maybe it would be better to have a separate structure field for both stats and snapshot, which will be set to point to a corresponding place in the shared FAM e.g. when the worker is getting initialized? shm_toc_allocate mentions BUFFERALIGN to handle possibility of some atomic operations needing it, so I guess that would have to be an alignment in this case as well. Probably another option would be to allocate two separate pieces of shared memory, which resolves questions like proper alignment, but annoyingly will require an extra lookup and a new key.
Re: pg_stat_statements and "IN" conditions
> On Tue, Mar 14, 2023 at 02:14:17PM -0400, Gregory Stark (as CFM) wrote: > So I was seeing that this patch needs a rebase according to cfbot. Yeah, folks are getting up to speed in with pgss improvements recently. Thanks for letting me know. > However it looks like the review feedback you're looking for is more > of design questions. What jumbling is best to include in the feature > set and which is best to add in later patches. It sounds like you've > gotten conflicting feedback from initial reviews. > > It does sound like the patch is pretty mature and you're actively > responding to feedback so if you got more authoritative feedback it > might even be committable now. It's already been two years of being > rolled forward so it would be a shame to keep rolling it forward. You got it about right. There is a balance to strike between implementation, that would cover more useful cases, but has more dependencies (something like possibility of having multiple query id), and more minimalistic implementation that would actually be acceptable in the way it is now. But I haven't heard back from David about it, so I assume everybody is fine with the minimalistic approach. > Or is there some fatal problem that you're trying to work around and > still haven't found the magic combination that convinces any > committers this is something we want? In which case perhaps we set > this patch returned? I don't get that impression myself though. Nothing like this on my side, although I'm not good at conjuring committing powers of the nature.
Re: Schema variables - new implementation for Postgres 15
> On Wed, Mar 08, 2023 at 08:31:07AM +0100, Pavel Stehule wrote: > pá 3. 3. 2023 v 21:19 odesílatel Dmitry Dolgov <9erthali...@gmail.com> > napsal: > > > > On Tue, Feb 28, 2023 at 06:12:50AM +0100, Pavel Stehule wrote: > > > > > > fresh rebase > > > > I'm continuing to review, this time going through shadowing stuff in > > transformColumnRef, IdentifyVariable etc. Well, that's a lot of leg work > > for rather little outcome :) I guess all attempts to simplify this part > > weren't successful? > > > > Originally I wrote it in the strategy "reduce false alarms". But when I > think about it, it may not be good in this case. Usually the changes are > done first on some developer environment, and good practice is to disallow > same (possibly confusing) identifiers. So I am not against making this > warning more aggressive with some possibility of false alarms. With > blocking reduction of alarms the differences in regress was zero. So I > reduced this part. Great, thank you. > > Couple of questions to it. In IdentifyVariable in the branch handling > > two values the commentary says: > > > > /* > > * a.b can mean "schema"."variable" or "variable"."field", > > * Check both variants, and returns InvalidOid with > > * not_unique flag, when both interpretations are > > * possible. Second node can be star. In this case, the > > * only allowed possibility is "variable"."*". > > */ > > > > I read this as "variable"."*" is a valid combination, but the very next > > part of this condition says differently: > > > > > > > > > /* > > * Session variables doesn't support unboxing by star > > * syntax. But this syntax have to be calculated here, > > * because can come from non session variables related > > * expressions. > > */ > > Assert(IsA(field2, A_Star)); > > > > Is the first commentary not quite correct? > > > > I think it is correct, but maybe I was not good at describing this issue. > The sentence "Second node can be a star. In this case, the > the only allowed possibility is "variable"."*"." should be in the next > comment. > > In this part we process a list of identifiers, and we try to map these > identifiers to some semantics. The parser should ensure that > all fields of lists are strings or the last field is a star. In this case > the semantic "schema".* is nonsense, and the only possible semantic > is "variable".*. It is valid semantics, but unsupported now. Unboxing is > available by syntax (var).* > > I changed the comment Thanks. Just to clarify, by "unsupported" you mean unsupported in the current patch implementation right? From what I understand value unboxing could be done without parentheses in a non-top level select query. As a side note, I'm not sure if this branch is exercised in any tests. I've replaced returning InvalidOid with actually doing LookupVariable(NULL, a, true) in this case, and all the tests are still passing. > > Another question about how shadowing warning should work between > > namespaces. > > Let's say I've got two namespaces, public and test, both have a session > > variable with the same name, but only one has a table with the same name: > > > > -- in public > > create table test_agg(a int); > > create type for_test_agg as (a int); > > create variable test_agg for_test_agg; > > > > -- in test > > create type for_test_agg as (a int); > > create variable test_agg for_test_agg; > > > > Now if we will try to trigger the shadowing warning from public > > namespace, it would work differently: > > > > -- in public > > =# let test.test_agg.a = 10; > > =# let test_agg.a = 20; > > =# set session_variables_ambiguity_warning to on; > > > > -- note the value returned from the table > > =# select jsonb_agg(test_agg.a) from test_agg; > > WARNING: 42702: session variable "test_agg.a" is shadowed > > LINE 1: select jsonb_agg(test_agg.a) from test_agg; > > ^ > > DETAIL: Session variables can be shadowed by columns, routine's > > variables and routine's arguments with the same name. > > LOCATION: transformColumnRef, parse_expr.c:940 > > jsonb_agg > > --- > > [1] > > > &g
Re: Schema variables - new implementation for Postgres 15
> On Tue, Feb 28, 2023 at 06:12:50AM +0100, Pavel Stehule wrote: > > fresh rebase I'm continuing to review, this time going through shadowing stuff in transformColumnRef, IdentifyVariable etc. Well, that's a lot of leg work for rather little outcome :) I guess all attempts to simplify this part weren't successful? Couple of questions to it. In IdentifyVariable in the branch handling two values the commentary says: /* * a.b can mean "schema"."variable" or "variable"."field", * Check both variants, and returns InvalidOid with * not_unique flag, when both interpretations are * possible. Second node can be star. In this case, the * only allowed possibility is "variable"."*". */ I read this as "variable"."*" is a valid combination, but the very next part of this condition says differently: /* * Session variables doesn't support unboxing by star * syntax. But this syntax have to be calculated here, * because can come from non session variables related * expressions. */ Assert(IsA(field2, A_Star)); Is the first commentary not quite correct? Another question about how shadowing warning should work between namespaces. Let's say I've got two namespaces, public and test, both have a session variable with the same name, but only one has a table with the same name: -- in public create table test_agg(a int); create type for_test_agg as (a int); create variable test_agg for_test_agg; -- in test create type for_test_agg as (a int); create variable test_agg for_test_agg; Now if we will try to trigger the shadowing warning from public namespace, it would work differently: -- in public =# let test.test_agg.a = 10; =# let test_agg.a = 20; =# set session_variables_ambiguity_warning to on; -- note the value returned from the table =# select jsonb_agg(test_agg.a) from test_agg; WARNING: 42702: session variable "test_agg.a" is shadowed LINE 1: select jsonb_agg(test_agg.a) from test_agg; ^ DETAIL: Session variables can be shadowed by columns, routine's variables and routine's arguments with the same name. LOCATION: transformColumnRef, parse_expr.c:940 jsonb_agg --- [1] -- no warning, note the session variable value =# select jsonb_agg(test.test_agg.a) from test_agg; jsonb_agg --- [10] It happens because in the second scenario the logic inside transformColumnRef will not set up the node variable (there is no corresponding table in the "test" schema), and the following conditions covering session variables shadowing are depending on it. Is it supposed to be like this?
Re: pg_stat_statements and "IN" conditions
> On Thu, Feb 23, 2023 at 09:48:35AM +0100, David Geier wrote: > Hi, > > > > Seems like supporting only constants is a good starting > > > point. The only thing that is likely confusing for users is that NUMERICs > > > (and potentially constants of other types) are unsupported. Wouldn't it be > > > fairly simple to support them via something like the following? > > > > > > is_const(element) || (is_coercion(element) && > > > is_const(element->child)) > > It definitely makes sense to implement that, although I don't think it's > > going to be acceptable to do that via directly listing conditions an > > element has to satisfy. It probably has to be more flexible, sice we > > would like to extend it in the future. My plan is to address this in a > > follow-up patch, when the main mechanism is approved. Would you agree > > with this approach? > > I still think it's counterintuitive and I'm pretty sure people would even > report this as a bug because not knowing about the difference in internal > representation you would expect NUMERICs to work the same way other > constants work. If anything we would have to document it. > > Can't we do something pragmatic and have something like > IsMergableInElement() which for now only supports constants and later can be > extended? Or what exactly do you mean by "more flexible"? Here is how I see it (pls correct me if I'm wrong at any point). To support numerics as presented in the tests from this patch, we have to deal with FuncExpr (the function converting a value into a numeric). Having in mind only numerics, we would need to filter out any other FuncExpr (which already sounds dubious). Then we need to validate for example that the function is immutable and have constant arguments, which is already implemented in evaluate_function and is a part of eval_const_expression. There is nothing special about numerics at this point, and this approach leads us back to eval_const_expression to a certain degree. Do you see any other way? I'm thinking about Michael idea in this context, and want to see if it would be possible to make the mechanism more flexible using some node attributes. But I see it only as a follow-up step, not a prerequisite.
Re: pg_stat_statements and "IN" conditions
> On Thu, Feb 09, 2023 at 08:43:29PM +0100, Dmitry Dolgov wrote:
> > On Thu, Feb 09, 2023 at 06:26:51PM +0100, Alvaro Herrera wrote:
> > On 2023-Feb-09, Dmitry Dolgov wrote:
> >
> > > > On Thu, Feb 09, 2023 at 02:30:34PM +0100, Peter Eisentraut wrote:
> >
> > > > What is the point of making this a numeric setting? Either you want
> > > > to merge all values or you don't want to merge any values.
> > >
> > > At least in theory the definition of "too many constants" is different
> > > for different use cases and I see allowing to configure it as a way of
> > > reducing the level of surprise here.
> >
> > I was thinking about this a few days ago and I agree that we don't
> > necessarily want to make it just a boolean thing; we may want to make it
> > more complex. One trivial idea is to make it group entries in powers of
> > 10: for 0-9 elements, you get one entry, and 10-99 you get a different
> > one, and so on:
> >
> > # group everything in a single bucket
> > const_merge_threshold = true / yes / on
> >
> > # group 0-9, 10-99, 100-999, 1000-
> > const_merge_treshold = powers
> >
> > Ideally the value would be represented somehow in the query text. For
> > example
> >
> > query| calls
> > --+---
> > select * from test where i in ({... 0-9 entries ...})| 2
> > select * from test where i in ({... 10-99 entries ...}) | 1
> >
> > What do you think? The jumble would have to know how to reduce all
> > values within each power-of-ten group to one specific value, but I don't
> > think that should be particularly difficult.
>
> Yeah, it sounds appealing and conveniently addresses the question of
> losing the information about how many constants originally were there.
> Not sure if the example above would be the most natural way to represent
> it in the query text, but otherwise I'm going to try implementing this.
> Stay tuned.
It took me couple of evenings, here is what I've got:
* The representation is not that far away from your proposal, I've
settled on:
SELECT * FROM test_merge WHERE id IN (... [10-99 entries])
* To not reinvent the wheel, I've reused decimalLenght function from
numutils, hence one more patch to make it available to reuse.
* This approach resolves my concerns about letting people tuning
the behaviour of merging, as now it's possible to distinguish between
calls with different number of constants up to the power of 10. So
I've decided to simplify the configuration and make the guc boolean to
turn it off or on.
* To separate queries with constants falling into different ranges
(10-99, 100-999, etc), the order of magnitude is added into the jumble
hash.
* I've incorporated feedback from Sergei and David, as well as tried to
make comments and documentation more clear.
Any feedback is welcomed, thanks!
>From dbb4eab9f3efbcee2326278be6f70ff52685b2b0 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 17 Feb 2023 10:17:55 +0100
Subject: [PATCH v13 1/2] Reusable decimalLength functions
Move out decimalLength functions to reuse in the following patch.
---
src/backend/utils/adt/numutils.c | 50 +---
src/include/utils/numutils.h | 67
2 files changed, 68 insertions(+), 49 deletions(-)
create mode 100644 src/include/utils/numutils.h
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
index 471fbb7ee6..df7418cce7 100644
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -18,9 +18,8 @@
#include
#include
-#include "common/int.h"
#include "utils/builtins.h"
-#include "port/pg_bitutils.h"
+#include "utils/numutils.h"
/*
* A table of all two-digit numbers. This is used to speed up decimal digit
@@ -38,53 +37,6 @@ static const char DIGIT_TABLE[200] =
"80" "81" "82" "83" "84" "85" "86" "87" "88" "89"
"90" "91" "92" "93" "94" "95" "96" "97" "98" "99";
-/*
- * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
- */
-static inline int
-decimalLength32(const uint32 v)
-{
- int t;
- static const uint32 PowersOfTen[] = {
- 1, 10, 100,
- 1000, 1, 10,
- 100, 1000, 1,
- 10
- };
-
- /*
- * Compute base-10 logarithm by dividing the base-2 logarithm by a
- * good-enough approximation of the base-2 logarithm of 10
-
Re: pg_stat_statements and "IN" conditions
> On Wed, Feb 15, 2023 at 08:51:56AM +0100, David Geier wrote: > Hi, > > On 2/11/23 13:08, Dmitry Dolgov wrote: > > > On Sat, Feb 11, 2023 at 11:47:07AM +0100, Dmitry Dolgov wrote: > > > > > > The original version of the patch was doing all of this, i.e. handling > > > numerics, Param nodes, RTE_VALUES. The commentary about > > > find_const_walker in tests is referring to a part of that, that was > > > dealing with evaluation of expression to see if it could be reduced to a > > > constant. > > > > > > Unfortunately there was a significant push back from reviewers because > > > of those features. That's why I've reduced the patch to it's minimally > > > useful version, having in mind re-implementing them as follow-up patches > > > in the future. This is the reason as well why I left tests covering all > > > this missing functionality -- as breadcrumbs to already discovered > > > cases, important for the future extensions. > > I'd like to elaborate on this a bit and remind about the origins of the > > patch, as it's lost somewhere in the beginning of the thread. The idea > > is not pulled out of thin air, everything is coming from our attempts to > > improve one particular monitoring infrastructure in a real commercial > > setting. Every covered use case and test in the original proposal was a > > result of field trials, when some application-side library or ORM was > > responsible for gigabytes of data in pgss, chocking the monitoring agent. > > Thanks for the clarifications. I didn't mean to contend the usefulness of > the patch and I wasn't aware that you already jumped through the loops of > handling Param, etc. No worries, I just wanted to emphasize that we've already collected quite some number of use cases. > Seems like supporting only constants is a good starting > point. The only thing that is likely confusing for users is that NUMERICs > (and potentially constants of other types) are unsupported. Wouldn't it be > fairly simple to support them via something like the following? > > is_const(element) || (is_coercion(element) && is_const(element->child)) It definitely makes sense to implement that, although I don't think it's going to be acceptable to do that via directly listing conditions an element has to satisfy. It probably has to be more flexible, sice we would like to extend it in the future. My plan is to address this in a follow-up patch, when the main mechanism is approved. Would you agree with this approach?
Re: pg_stat_statements and "IN" conditions
> On Sat, Feb 11, 2023 at 11:47:07AM +0100, Dmitry Dolgov wrote: > > The original version of the patch was doing all of this, i.e. handling > numerics, Param nodes, RTE_VALUES. The commentary about > find_const_walker in tests is referring to a part of that, that was > dealing with evaluation of expression to see if it could be reduced to a > constant. > > Unfortunately there was a significant push back from reviewers because > of those features. That's why I've reduced the patch to it's minimally > useful version, having in mind re-implementing them as follow-up patches > in the future. This is the reason as well why I left tests covering all > this missing functionality -- as breadcrumbs to already discovered > cases, important for the future extensions. I'd like to elaborate on this a bit and remind about the origins of the patch, as it's lost somewhere in the beginning of the thread. The idea is not pulled out of thin air, everything is coming from our attempts to improve one particular monitoring infrastructure in a real commercial setting. Every covered use case and test in the original proposal was a result of field trials, when some application-side library or ORM was responsible for gigabytes of data in pgss, chocking the monitoring agent.
Re: pg_stat_statements and "IN" conditions
> On Sat, Feb 11, 2023 at 11:03:36AM +0100, David Geier wrote: > Hi, > > On 2/9/23 16:02, Dmitry Dolgov wrote: > > > Unfortunately, rebase is needed again due to recent changes in > > > queryjumblefuncs ( 9ba37b2cb6a174b37fc51d0649ef73e56eae27fc ) > I reviewed the last patch applied to some commit from Feb. 4th. Thanks for looking. Few quick answers about high-level questions below, the rest I'll incorporate in the new version. > - There's a comment about find_const_walker(). I cannot find that function > anywhere. What am I missing? > > [...] > > - Don't you intend to use the NUMERIC data column in SELECT * FROM > test_merge_numeric WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10)? Otherwise, > the test is identical to previous test cases and you're not checking for > what happens with NUMERICs which are wrapped in FuncExpr because of the > implicit coercion. > > - Don't we want to extend IsConstList() to allow a list of all implicitly > coerced constants? It's inconsistent that otherwise e.g. NUMERICs don't > work. > > [...] > > - Prepared statements are not supported as they contain INs with Param > instead of Const nodes. While less likely, I've seen applications that use > prepared statements in conjunction with queries generated through a UI which > ended up with tons of prepared queries with different number of elements in > the IN clause. Not necessarily something that must go into this patch but > maybe worth thinking about. The original version of the patch was doing all of this, i.e. handling numerics, Param nodes, RTE_VALUES. The commentary about find_const_walker in tests is referring to a part of that, that was dealing with evaluation of expression to see if it could be reduced to a constant. Unfortunately there was a significant push back from reviewers because of those features. That's why I've reduced the patch to it's minimally useful version, having in mind re-implementing them as follow-up patches in the future. This is the reason as well why I left tests covering all this missing functionality -- as breadcrumbs to already discovered cases, important for the future extensions. > - Why do we actually only want to merge constants? Why don't we ignore the > type of element in the IN and merge whatever is there? Is this because the > original jumbling logic as of now only has support for constants? > > - Ideally we would even remove duplicates. That would even improve > cardinality estimation but I guess we don't want to spend the cycles on > doing so in the planner? I believe these points are beyond the patch goals, as it's less clear (at least to me) if it's safe or desirable to do so.
Re: pg_stat_statements and "IN" conditions
> On Thu, Feb 09, 2023 at 06:26:51PM +0100, Alvaro Herrera wrote:
> On 2023-Feb-09, Dmitry Dolgov wrote:
>
> > > On Thu, Feb 09, 2023 at 02:30:34PM +0100, Peter Eisentraut wrote:
>
> > > What is the point of making this a numeric setting? Either you want
> > > to merge all values or you don't want to merge any values.
> >
> > At least in theory the definition of "too many constants" is different
> > for different use cases and I see allowing to configure it as a way of
> > reducing the level of surprise here.
>
> I was thinking about this a few days ago and I agree that we don't
> necessarily want to make it just a boolean thing; we may want to make it
> more complex. One trivial idea is to make it group entries in powers of
> 10: for 0-9 elements, you get one entry, and 10-99 you get a different
> one, and so on:
>
> # group everything in a single bucket
> const_merge_threshold = true / yes / on
>
> # group 0-9, 10-99, 100-999, 1000-
> const_merge_treshold = powers
>
> Ideally the value would be represented somehow in the query text. For
> example
>
> query| calls
> --+---
> select * from test where i in ({... 0-9 entries ...})| 2
> select * from test where i in ({... 10-99 entries ...}) | 1
>
> What do you think? The jumble would have to know how to reduce all
> values within each power-of-ten group to one specific value, but I don't
> think that should be particularly difficult.
Yeah, it sounds appealing and conveniently addresses the question of
losing the information about how many constants originally were there.
Not sure if the example above would be the most natural way to represent
it in the query text, but otherwise I'm going to try implementing this.
Stay tuned.
Re: pg_stat_statements and "IN" conditions
> On Thu, Feb 09, 2023 at 02:30:34PM +0100, Peter Eisentraut wrote: > On 07.02.23 21:14, Sergei Kornilov wrote: > > It seems a little strange to me that with const_merge_threshold = 1, such a > > test case gives the same result as with const_merge_threshold = 2 > > What is the point of making this a numeric setting? Either you want to > merge all values or you don't want to merge any values. At least in theory the definition of "too many constants" is different for different use cases and I see allowing to configure it as a way of reducing the level of surprise here. The main scenario for a numerical setting would be to distinguish between normal usage with just a handful of constants (and the user expecting to see them represented in pgss) and some sort of outliers with thousands of constants in a query (e.g. as a defence mechanism for the infrastructure working with those metrics). But I agree that it's not clear how much value is in that. Not having strong opinion about this I would be fine changing it to a boolean option (with an actual limit hidden internally) if everyone agrees it fits better.
Re: pg_stat_statements and "IN" conditions
> On Tue, Feb 07, 2023 at 11:14:52PM +0300, Sergei Kornilov wrote: > Hello! Thanks for reviewing. > Unfortunately, rebase is needed again due to recent changes in > queryjumblefuncs ( 9ba37b2cb6a174b37fc51d0649ef73e56eae27fc ) Yep, my favourite game, rebaseball. Will post a new version soon, after figuring out all the recent questions. > It seems a little strange to me that with const_merge_threshold = 1, such a > test case gives the same result as with const_merge_threshold = 2 > > select pg_stat_statements_reset(); > set const_merge_threshold to 1; > select * from test where i in (1,2,3); > select * from test where i in (1,2); > select * from test where i in (1); > select query, calls from pg_stat_statements order by query; > > query| calls > -+--- > select * from test where i in (...) | 2 > select * from test where i in ($1) | 1 > > Probably const_merge_threshold = 1 should produce only "i in (...)"? Well, it's not intentional, probably I need to be more careful with off-by-one. Although I agree to a certain extent with Peter questioning the value of having numerical option here, let me think about this. > const_merge_threshold is "the minimal length of an array" (more or equal) or > "array .. length is larger than" (not equals)? I think the documentation is > ambiguous in this regard. > > I also noticed a typo in guc_tables.c: "Sets the minimal numer of constants > in an array" -> number Yep, I'll rephrase the documentation.
Re: pg_stat_statements and "IN" conditions
> On Sun, Feb 05, 2023 at 11:02:32AM -0500, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: > > I'm thinking about this in the following way: the core jumbling logic is > > responsible for deriving locations based on the input expressions; in > > the case of merging we produce less locations; pgss have to represent > > the result only using locations and has to be able to differentiate > > simple locations and locations after merging. > > Uh ... why? ISTM you're just going to elide all inside the IN, > so why do you need more than a start and stop position? Exactly, start and stop positions. But if there would be no information that merging was applied, the following queries will look the same after jumbling, right? -- input query SELECT * FROM test_merge WHERE id IN (1, 2); -- jumbling result, two LocationLen, for values 1 and 2 SELECT * FROM test_merge WHERE id IN ($1, $2); -- input query SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); -- jumbling result, two LocationLen after merging, for values 1 and 10 SELECT * FROM test_merge WHERE id IN (...); -- without remembering about merging the result would be SELECT * FROM test_merge WHERE id IN ($1, $2);
Re: pg_stat_statements and "IN" conditions
> On Sun, Feb 05, 2023 at 10:30:25AM +0900, Michael Paquier wrote: > On Sat, Feb 04, 2023 at 06:08:41PM +0100, Dmitry Dolgov wrote: > > Here is the rebased version. To adapt to the latest changes, I've marked > > ArrayExpr with custom_query_jumble to implement this functionality, but > > tried to make the actual merge logic relatively independent. Otherwise, > > everything is the same. > > I was quickly looking at this patch, so these are rough impressions. > > + boolmerged; /* whether or not the location was marked as > + not contributing to jumble */ > > This part of the patch is a bit disturbing.. We have node attributes > to track if portions of a node should be ignored or have a location > marked, still this "merged" flag is used as an extension to track if a > location should be done or not. Is that a concept that had better be > controlled via a new node attribute? Good question. I need to think a bit more if it's possible to leverage node attributes mechanism, but at the moment I'm still inclined to extend LocationLen. The reason is that it doesn't exactly serve the tracking purpose, i.e. whether to capture a location (I have to update the code commentary), it helps differentiate cases when locations A and D are obtained from merging A B C D instead of just being A and D. I'm thinking about this in the following way: the core jumbling logic is responsible for deriving locations based on the input expressions; in the case of merging we produce less locations; pgss have to represent the result only using locations and has to be able to differentiate simple locations and locations after merging. > +-- > +-- Consts merging > +-- > +CREATE TABLE test_merge (id int, data int); > +-- IN queries > +-- No merging > > Would it be better to split this set of tests into a new file? FWIW, > I have a patch in baking process that refactors a bit the whole, > before being able to extend it so as we have more coverage for > normalized utility queries, as of now the query strings stored by > pg_stat_statements don't reflect that even if the jumbling computation > marks the location of the Const nodes included in utility statements > (partition bounds, queries of COPY, etc.). I should be able to send > that tomorrow, and my guess that you could take advantage of that > even for this thread. Sure, I'll take a look how I can benefit from your work, thanks.
Re: pg_stat_statements and "IN" conditions
> On Thu, Feb 02, 2023 at 04:05:54PM +0100, Dmitry Dolgov wrote: > > On Thu, Feb 02, 2023 at 03:07:27PM +0100, Alvaro Herrera wrote: > > This appears to have massive conflicts. Would you please rebase? > > Sure, I was already mentally preparing myself to do so in the view of > recent changes in query jumbling. Will post soon. Here is the rebased version. To adapt to the latest changes, I've marked ArrayExpr with custom_query_jumble to implement this functionality, but tried to make the actual merge logic relatively independent. Otherwise, everything is the same. >From e72f6b8990dace82667d46b3578062bee92af472 Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sun, 24 Jul 2022 11:43:25 +0200 Subject: [PATCH v12] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on number of parameters, because every element of ArrayExpr is jumbled. In certain situations it's undesirable, especially if the list becomes too large. Make Const expressions contribute nothing to the jumble hash if they're a part of an ArrayExpr, which length is larger than specified threshold. Allow to configure the threshold via the new GUC const_merge_threshold with the default value zero, which disables this feature. Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane Tested-by: Chengxi Sun --- .../expected/pg_stat_statements.out | 412 ++ .../pg_stat_statements/pg_stat_statements.c | 33 +- .../sql/pg_stat_statements.sql| 107 + doc/src/sgml/config.sgml | 26 ++ doc/src/sgml/pgstatstatements.sgml| 28 +- src/backend/nodes/gen_node_support.pl | 2 +- src/backend/nodes/queryjumblefuncs.c | 102 - src/backend/utils/misc/guc_tables.c | 13 + src/backend/utils/misc/postgresql.conf.sample | 2 +- src/include/nodes/primnodes.h | 2 + src/include/nodes/queryjumble.h | 5 +- 11 files changed, 712 insertions(+), 20 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index fb9ccd920f..9acdb55c9a 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -1142,4 +1142,416 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%'; 2 (1 row) +-- +-- Consts merging +-- +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- No merging +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | calls ++--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1 + SELECT pg_stat_statements_reset() | 1 + SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 +(7 rows) + +-- Normal +SET const_merge_threshold = 5; +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | calls ++--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3) | 1 + SELECT pg_stat_statements_reset() | 1 + SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 +(3 rows) + +SELECT * FROM test
Re: pg_stat_statements and "IN" conditions
> On Thu, Feb 02, 2023 at 03:07:27PM +0100, Alvaro Herrera wrote: > This appears to have massive conflicts. Would you please rebase? Sure, I was already mentally preparing myself to do so in the view of recent changes in query jumbling. Will post soon.
Re: pg_stat_statements and "IN" conditions
> On Sun, Jan 29, 2023 at 09:56:02AM -0300, Marcos Pegoraro wrote: > Em dom., 29 de jan. de 2023 às 09:24, Dmitry Dolgov <9erthali...@gmail.com> > escreveu: > > > > On Fri, Jan 27, 2023 at 08:15:29PM +0530, vignesh C wrote: > > > The patch does not apply on top of HEAD as in [1], please post a rebased > > patch: > > > > Thanks. I think this one should do the trick. > > > > There is a typo on DOC part > +and it's length is larger than const_merge_threshold > , > +then array elements will contribure nothing to the query > identifier. > +Thus the query will get the same identifier no matter how many > constants > > That "contribure" should be "contribute" Indeed, thanks for noticing. >From 1d980ef5f556c1684ea5c991965b2375bbdd139b Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sun, 24 Jul 2022 11:43:25 +0200 Subject: [PATCH v11] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on number of parameters, because every element of ArrayExpr is jumbled. In certain situations it's undesirable, especially if the list becomes too large. Make Const expressions contribute nothing to the jumble hash if they're a part of an ArrayExpr, which length is larger than specified threshold. Allow to configure the threshold via the new GUC const_merge_threshold with the default value zero, which disables this feature. Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane Tested-by: Chengxi Sun --- .../expected/pg_stat_statements.out | 412 ++ .../pg_stat_statements/pg_stat_statements.c | 33 +- .../sql/pg_stat_statements.sql| 107 + doc/src/sgml/config.sgml | 26 ++ doc/src/sgml/pgstatstatements.sgml| 28 +- src/backend/nodes/queryjumblefuncs.c | 105 - src/backend/utils/misc/guc_tables.c | 13 + src/backend/utils/misc/postgresql.conf.sample | 2 +- src/include/nodes/queryjumble.h | 5 +- 9 files changed, 712 insertions(+), 19 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 9ac5c87c3a..f18f34ae5b 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -1141,4 +1141,416 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%'; 2 (1 row) +-- +-- Consts merging +-- +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- No merging +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | calls ++--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1 + SELECT pg_stat_statements_reset() | 1 + SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 +(7 rows) + +-- Normal +SET const_merge_threshold = 5; +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | calls ++--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3) | 1 + SELECT pg_stat_statements_reset() | 1 + SELE
Re: pg_stat_statements and "IN" conditions
> On Fri, Jan 27, 2023 at 08:15:29PM +0530, vignesh C wrote: > The patch does not apply on top of HEAD as in [1], please post a rebased > patch: Thanks. I think this one should do the trick. >From 3c51561ddaecdbc82842fae4fab74cc33526f17c Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sun, 24 Jul 2022 11:43:25 +0200 Subject: [PATCH v10] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on number of parameters, because every element of ArrayExpr is jumbled. In certain situations it's undesirable, especially if the list becomes too large. Make Const expressions contribute nothing to the jumble hash if they're a part of an ArrayExpr, which length is larger than specified threshold. Allow to configure the threshold via the new GUC const_merge_threshold with the default value zero, which disables this feature. Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane Tested-by: Chengxi Sun --- .../expected/pg_stat_statements.out | 412 ++ .../pg_stat_statements/pg_stat_statements.c | 33 +- .../sql/pg_stat_statements.sql| 107 + doc/src/sgml/config.sgml | 26 ++ doc/src/sgml/pgstatstatements.sgml| 28 +- src/backend/nodes/queryjumblefuncs.c | 105 - src/backend/utils/misc/guc_tables.c | 13 + src/backend/utils/misc/postgresql.conf.sample | 2 +- src/include/nodes/queryjumble.h | 5 +- 9 files changed, 712 insertions(+), 19 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 9ac5c87c3a..f18f34ae5b 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -1141,4 +1141,416 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%'; 2 (1 row) +-- +-- Consts merging +-- +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- No merging +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | calls ++--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1 + SELECT pg_stat_statements_reset() | 1 + SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 +(7 rows) + +-- Normal +SET const_merge_threshold = 5; +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | calls ++--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3) | 1 + SELECT pg_stat_statements_reset() | 1 + SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 +(3 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; +
Re: Lazy JIT IR code generation to increase JIT speed with partitions
> On Fri, Jan 27, 2023 at 10:02:32AM +0100, David Geier wrote: > It's very curious as to why we didn't really see that when dumping the > bitcode. It seems like the bitcode is always different enough to not spot > that. As I've noted off-list, there was noticeable difference in the dumped bitcode, which I haven't noticed since we were talking mostly about differences between executions of the same query.
Re: Schema variables - new implementation for Postgres 15 (typo)
> On Mon, Jan 23, 2023 at 07:09:27PM +0100, Pavel Stehule wrote: > po 23. 1. 2023 v 15:25 odesílatel Dmitry Dolgov <9erthali...@gmail.com> > napsal: > > > > On Sun, Jan 22, 2023 at 07:47:07PM +0100, Pavel Stehule wrote: > > > pá 20. 1. 2023 v 21:35 odesílatel Dmitry Dolgov <9erthali...@gmail.com> > > > napsal: > > > > > > > * I think it was already mentioned in the thread, there seems to be > > not a > > > > single usage of CHECK_FOR_INTERRUPTS in session_variable.c . But some > > > > number > > > > of loops over the sessionvars are implemented, are they always going > > to > > > > be > > > > small enough to not make any troubles? > > > > > > > > > > The longest cycle is a cycle that rechecks actively used variables > > against > > > system catalog. No cycle depends on the content of variables. > > > > Right, but what about the cases with huge number of variables? Not > > saying it could be in any sense common, but possible to do. > > > > Now I tested 10K variables (with enabled assertions, without it is should > be faster) > > [...] > > I can be wrong, but from these numbers I don't think so these sync cycles > should to contain CHECK_FOR_INTERRUPTS > > What do you think? Well, there is always possibility someone will create more variables than any arbitrary limit we have tested for. But I see your point and don't have a strong opinion about this, so let's keep it as it is :) > > > > * sync_sessionvars_all explains why is it necessary to copy > > > > xact_recheck_varids: > > > > > > > > When we check the variables, the system cache can be > > > > invalidated, > > > > and xact_recheck_varids can be enhanced. > > > > > > > > I'm not quite following what the "enhancement" part is about? Is > > > > xact_recheck_varids could be somehow updated concurrently with the > > loop? > > > > > > > > > > Yes. pg_variable_cache_callback can be called when > > > is_session_variable_valid is executed. > > > > > > Maybe "extended" can be a better word instead of "enhanced"? I > > reformulated > > > this comment > > > > Yeah, "extended" sounds better. But I was mostly confused about the > > mechanism, if the cache callback can interrupt the execution at any > > moment when called, that would explain it. > > > > patch from yesterday has extended comment in this area :-) Thanks!
Re: Schema variables - new implementation for Postgres 15 (typo)
> On Sun, Jan 22, 2023 at 07:47:07PM +0100, Pavel Stehule wrote: > pá 20. 1. 2023 v 21:35 odesílatel Dmitry Dolgov <9erthali...@gmail.com> > napsal: > > > * I think it was already mentioned in the thread, there seems to be not a > > single usage of CHECK_FOR_INTERRUPTS in session_variable.c . But some > > number > > of loops over the sessionvars are implemented, are they always going to > > be > > small enough to not make any troubles? > > > > The longest cycle is a cycle that rechecks actively used variables against > system catalog. No cycle depends on the content of variables. Right, but what about the cases with huge number of variables? Not saying it could be in any sense common, but possible to do. > > * sync_sessionvars_all explains why is it necessary to copy > > xact_recheck_varids: > > > > When we check the variables, the system cache can be > > invalidated, > > and xact_recheck_varids can be enhanced. > > > > I'm not quite following what the "enhancement" part is about? Is > > xact_recheck_varids could be somehow updated concurrently with the loop? > > > > Yes. pg_variable_cache_callback can be called when > is_session_variable_valid is executed. > > Maybe "extended" can be a better word instead of "enhanced"? I reformulated > this comment Yeah, "extended" sounds better. But I was mostly confused about the mechanism, if the cache callback can interrupt the execution at any moment when called, that would explain it.
Re: Schema variables - new implementation for Postgres 15 (typo)
I've accumulated another collection of various questions and comments. As a side note I'm getting a good feeling about this patch, those part I've read so far looks good to me. * I've suddenly realized one could use pseudo types for variables, and it not always works. E.g.: =# create variable pseudo_array anyarray; =# select pseudo_array; pseudo_array -- NULL =# let pseudo_array = ARRAY[1, 2, 3]; ERROR: 42804: target session variable is of type anyarray but expression is of type integer[] LOCATION: svariableStartupReceiver, svariableReceiver.c:79 =# create variable pseudo_unknown unknown; =# select pseudo_unknown; ERROR: XX000: failed to find conversion function from unknown to text LOCATION: coerce_type, parse_coerce.c:542 Is it supposed to be like this, or something is missing? * I think it was already mentioned in the thread, there seems to be not a single usage of CHECK_FOR_INTERRUPTS in session_variable.c . But some number of loops over the sessionvars are implemented, are they always going to be small enough to not make any troubles? * sync_sessionvars_all explains why is it necessary to copy xact_recheck_varids: When we check the variables, the system cache can be invalidated, and xact_recheck_varids can be enhanced. I'm not quite following what the "enhancement" part is about? Is xact_recheck_varids could be somehow updated concurrently with the loop? * A small typo diff --git a/src/backend/commands/session_variable.c b/src/backend/commands/session_variable.c --- a/src/backend/commands/session_variable.c +++ b/src/backend/commands/session_variable.c @@ -485,7 +485,7 @@ sync_sessionvars_all(bool filter_lxid) /* * When we check the variables, the system cache can be invalidated, -* and xac_recheck_varids can be enhanced. We want to iterate +* and xact_recheck_varids can be enhanced. We want to iterate NOTE: The commentaries above were made based on the previous patch version, but it looks like those aspects were not changed.
Re: [RFC] Add jit deform_counter
> On Sun, Jan 08, 2023 at 09:06:33PM +0100, Pavel Stehule wrote:
> It is working although I am not sure if it is correctly
>
> when I run EXPLAIN ANALYZE for query `explain analyze select
> count(length(prosrc) > 0) from pg_proc;`
>
> I got plan and times
>
> ┌─┐
> │ QUERY PLAN
>│
> ╞═╡
> │ Aggregate (cost=154.10..154.11 rows=1 width=8) (actual
> time=134.450..134.451 rows=1 loops=1)
> │
> │ -> Seq Scan on pg_proc (cost=0.00..129.63 rows=3263 width=16) (actual
> time=0.013..0.287 rows=3266 loops=1) │
> │ Planning Time: 0.088 ms
> │
> │ JIT:
>│
> │ Functions: 3
>│
> │ Options: Inlining true, Optimization true, Expressions true, Deforming
> true │
> │ Timing: Generation 0.631 ms, Deforming 0.396 ms, Inlining 10.026 ms,
> Optimization 78.608 ms, Emission 44.915 ms, Total 134.181 ms │
> │ Execution Time: 135.173 ms
>│
> └─┘
> (8 rows)
>
> Deforming is 0.396ms
>
> When I run mentioned query, and when I look to pg_stat_statements table, I
> see different times
>
> deforming is about 10ms
>
> wal_bytes │ 0
> jit_functions │ 9
> jit_generation_time│ 1.90404099
> jit_deform_count │ 3
> jit_deform_time│ 36.395131
> jit_inlining_count │ 3
> jit_inlining_time │ 256.104205
> jit_optimization_count │ 3
> jit_optimization_time │ 132.4536130002
> jit_emission_count │ 3
> jit_emission_time │ 1.210633
>
> counts are correct, but times are strange - there is not consistency with
> values from EXPLAIN
>
> When I run this query on master, the values are correct
>
> jit_functions │ 6
> jit_generation_time│ 1.350521
> jit_inlining_count │ 2
> jit_inlining_time │ 24.0183823
> jit_optimization_count │ 2
> jit_optimization_time │ 173.405792
> jit_emission_count │ 2
> jit_emission_time │ 91.226655
> ┴───
>
> │ JIT:
> │
> │ Functions: 3
> │
> │ Options: Inlining true, Optimization true, Expressions true, Deforming
> true │
> │ Timing: Generation 0.636 ms, Inlining 9.309 ms, Optimization 89.653 ms,
> Emission 45.812 ms, Total 145.410 ms │
> │ Execution Time: 146.410 ms
> │
> └┘
Thanks for noticing. Similarly to the previous issue, the order of
columns was incorrect -- deform counters have to be the last columns in
the view.
>From 93d739e9258f79474df5644831a1f82fc97742dc Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 11 Jun 2022 11:54:40 +0200
Subject: [PATCH v4 1/2] Add deform_counter
At the moment generation_counter includes time spent on both JITing
expressions and tuple deforming. Those are configured independently via
corresponding options (jit_expressions and jit_tuple_deforming), which
means there is no way to find out what fraction of time tuple deforming
alone is taking.
Add deform_counter dedicated to tuple deforming, which allows seeing
more directly the influence jit_tuple_deforming is having on the query.
---
src/backend/commands/explain.c | 7 ++-
src/backend/jit/jit.c | 1 +
src/backend/jit/llvm/llvmjit_expr.c | 6 ++
src/include/jit/jit.h | 3 +++
4 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index f86983c660..2f23602a87 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -882,6 +882,7 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
/* calculate total time */
INSTR_TIME_SET_ZERO(total_time);
+ /* don't add deform_counter, it's included into generation_counter */
INSTR_TIME_ADD(total_time, ji->generation_counter);
INSTR_TIME_ADD(total_time, ji->inlining_counter);
INSTR_TIME_ADD(total_time, ji->optimization_counter);
@@ -909,8 +910,9 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
{
ExplainIndentText(es);
Re: [RFC] Add jit deform_counter
> On Sat, Jan 07, 2023 at 07:09:11PM +0100, Pavel Stehule wrote: > so 7. 1. 2023 v 16:48 odesílatel Dmitry Dolgov <9erthali...@gmail.com> > napsal: > > > > On Fri, Jan 06, 2023 at 09:42:09AM +0100, Pavel Stehule wrote: > > > The explain part is working, the part of pg_stat_statements doesn't > > > > > > set jit_above_cost to 10; > > > set jit_optimize_above_cost to 10; > > > set jit_inline_above_cost to 10; > > > > > > (2023-01-06 09:08:59) postgres=# explain analyze select > > > count(length(prosrc) > 0) from pg_proc; > > > > > ┌┐ > > > │ QUERY PLAN > > > │ > > > > > ╞╡ > > > │ Aggregate (cost=154.10..154.11 rows=1 width=8) (actual > > > time=132.320..132.321 rows=1 loops=1) > > │ > > > │ -> Seq Scan on pg_proc (cost=0.00..129.63 rows=3263 width=16) > > (actual > > > time=0.013..0.301 rows=3266 loops=1) │ > > > │ Planning Time: 0.070 ms > > > │ > > > │ JIT: > > > │ > > > │ Functions: 3 > > > │ > > > │ Options: Inlining true, Optimization true, Expressions true, > > Deforming > > > true │ > > > │ Timing: Generation 0.597 ms, Deforming 0.407 ms, Inlining 8.943 ms, > > > Optimization 79.403 ms, Emission 43.091 ms, Total 132.034 ms │ > > > │ Execution Time: 132.986 ms > > > │ > > > > > └┘ > > > (8 rows) > > > > > > I see the result of deforming in explain analyze, but related values in > > > pg_stat_statements are 0. > > > > I'm not sure why, but pgss jit metrics are always nulls for explain > > analyze queries. I have noticed this with surprise myself, when recently > > was reviewing the lazy jit patch, but haven't yet figure out what is the > > reason. Anyway, without "explain analyze" you'll get correct deforming > > numbers in pgss. > > > > It was really strange, because I tested the queries without EXPLAIN ANALYZE > too, and new columns were always zero on my comp. Other jit columns were > filled. But I didn't do a deeper investigation. Interesting. I've verified it once more with the query and the parameters you've posted, got the following: jit_functions | 3 jit_generation_time| 1.257522 jit_deform_count | 1 jit_deform_time| 10.381345 jit_inlining_count | 1 jit_inlining_time | 71.628168 jit_optimization_count | 1 jit_optimization_time | 48.146447 jit_emission_count | 1 jit_emission_time | 0.737822 Maybe there is anything else special about how you run it? Otherwise addressed the rest of commentaries, thanks. >From 93d739e9258f79474df5644831a1f82fc97742dc Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sat, 11 Jun 2022 11:54:40 +0200 Subject: [PATCH v3 1/2] Add deform_counter At the moment generation_counter includes time spent on both JITing expressions and tuple deforming. Those are configured independently via corresponding options (jit_expressions and jit_tuple_deforming), which means there is no way to find out what fraction of time tuple deforming alone is taking. Add deform_counter dedicated to tuple deforming, which allows seeing more directly the influence jit_tuple_deforming is having on the query. --- src/backend/commands/explain.c | 7 ++- src/backend/jit/jit.c | 1 + src/backend/jit/llvm/llvmjit_expr.c | 6 ++ src/include/jit/jit.h | 3 +++ 4 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index f86983c660..2f23602a87 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -882,6 +882,7 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji) /* calculate total time */ INSTR_TIME_SET_ZERO(total_time); + /* don't add deform_counter, it's included into genera
Re: [RFC] Add jit deform_counter
> On Fri, Jan 06, 2023 at 09:42:09AM +0100, Pavel Stehule wrote:
> The explain part is working, the part of pg_stat_statements doesn't
>
> set jit_above_cost to 10;
> set jit_optimize_above_cost to 10;
> set jit_inline_above_cost to 10;
>
> (2023-01-06 09:08:59) postgres=# explain analyze select
> count(length(prosrc) > 0) from pg_proc;
> ┌┐
> │ QUERY PLAN
> │
> ╞╡
> │ Aggregate (cost=154.10..154.11 rows=1 width=8) (actual
> time=132.320..132.321 rows=1 loops=1) │
> │ -> Seq Scan on pg_proc (cost=0.00..129.63 rows=3263 width=16) (actual
> time=0.013..0.301 rows=3266 loops=1) │
> │ Planning Time: 0.070 ms
> │
> │ JIT:
> │
> │ Functions: 3
> │
> │ Options: Inlining true, Optimization true, Expressions true, Deforming
> true │
> │ Timing: Generation 0.597 ms, Deforming 0.407 ms, Inlining 8.943 ms,
> Optimization 79.403 ms, Emission 43.091 ms, Total 132.034 ms │
> │ Execution Time: 132.986 ms
> │
> └┘
> (8 rows)
>
> I see the result of deforming in explain analyze, but related values in
> pg_stat_statements are 0.
I'm not sure why, but pgss jit metrics are always nulls for explain
analyze queries. I have noticed this with surprise myself, when recently
was reviewing the lazy jit patch, but haven't yet figure out what is the
reason. Anyway, without "explain analyze" you'll get correct deforming
numbers in pgss.
> Minimally, the values are assigned in wrong order
>
> + if (api_version >= PGSS_V1_11)
> + {
> + values[i++] = Float8GetDatumFast(tmp.jit_deform_time);
> + values[i++] = Int64GetDatumFast(tmp.jit_deform_count);
> + }
(facepalm) Yep, will fix the order.
> After reading the doc, I am confused what this metric means
>
> +
> +
> + jit_deform_count bigint
> +
> +
> + Number of times tuples have been deformed
> +
> +
> +
> +
> +
> + jit_deform_time double
> precision
> +
> +
> + Total time spent by the statement on deforming tuples, in
> milliseconds
> +
> +
>
> It is not clean so these times and these numbers are related just to the
> compilation of the deforming process, not by own deforming.
Good point, I need to formulate this more clearly.
Re: [RFC] Add jit deform_counter
> On Sun, Dec 25, 2022 at 06:55:02PM +0100, Pavel Stehule wrote:
> there are some problems with stability of regress tests
>
> http://cfbot.cputube.org/dmitry-dolgov.html
Looks like this small change predates moving to meson, the attached
version should help.
>From 1b19af29b4a71e008d9d4ac7ca39d06dc89d6237 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 11 Jun 2022 11:54:40 +0200
Subject: [PATCH v2 1/2] Add deform_counter
At the moment generation_counter includes time spent on both JITing
expressions and tuple deforming. Those are configured independently via
corresponding options (jit_expressions and jit_tuple_deforming), which
means there is no way to find out what fraction of time tuple deforming
alone is taking.
Add deform_counter dedicated to tuple deforming, which allows seeing
more directly the influence jit_tuple_deforming is having on the query.
---
src/backend/commands/explain.c | 7 ++-
src/backend/jit/jit.c | 1 +
src/backend/jit/llvm/llvmjit_expr.c | 6 ++
src/include/jit/jit.h | 3 +++
4 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index f86983c660..2f23602a87 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -882,6 +882,7 @@ ExplainPrintJIT(ExplainState *es, int jit_flags,
JitInstrumentation *ji)
/* calculate total time */
INSTR_TIME_SET_ZERO(total_time);
+ /* don't add deform_counter, it's included into generation_counter */
INSTR_TIME_ADD(total_time, ji->generation_counter);
INSTR_TIME_ADD(total_time, ji->inlining_counter);
INSTR_TIME_ADD(total_time, ji->optimization_counter);
@@ -909,8 +910,9 @@ ExplainPrintJIT(ExplainState *es, int jit_flags,
JitInstrumentation *ji)
{
ExplainIndentText(es);
appendStringInfo(es->str,
-"Timing: %s %.3f ms,
%s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n",
+"Timing: %s %.3f ms,
%s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n",
"Generation", 1000.0 *
INSTR_TIME_GET_DOUBLE(ji->generation_counter),
+"Deforming", 1000.0 *
INSTR_TIME_GET_DOUBLE(ji->deform_counter),
"Inlining", 1000.0 *
INSTR_TIME_GET_DOUBLE(ji->inlining_counter),
"Optimization", 1000.0
* INSTR_TIME_GET_DOUBLE(ji->optimization_counter),
"Emission", 1000.0 *
INSTR_TIME_GET_DOUBLE(ji->emission_counter),
@@ -937,6 +939,9 @@ ExplainPrintJIT(ExplainState *es, int jit_flags,
JitInstrumentation *ji)
ExplainPropertyFloat("Generation", "ms",
1000.0 *
INSTR_TIME_GET_DOUBLE(ji->generation_counter),
3, es);
+ ExplainPropertyFloat("Deforming", "ms",
+1000.0 *
INSTR_TIME_GET_DOUBLE(ji->deform_counter),
+3, es);
ExplainPropertyFloat("Inlining", "ms",
1000.0 *
INSTR_TIME_GET_DOUBLE(ji->inlining_counter),
3, es);
diff --git a/src/backend/jit/jit.c b/src/backend/jit/jit.c
index 91a6b2b63a..a6bdf03718 100644
--- a/src/backend/jit/jit.c
+++ b/src/backend/jit/jit.c
@@ -185,6 +185,7 @@ InstrJitAgg(JitInstrumentation *dst, JitInstrumentation
*add)
{
dst->created_functions += add->created_functions;
INSTR_TIME_ADD(dst->generation_counter, add->generation_counter);
+ INSTR_TIME_ADD(dst->deform_counter, add->deform_counter);
INSTR_TIME_ADD(dst->inlining_counter, add->inlining_counter);
INSTR_TIME_ADD(dst->optimization_counter, add->optimization_counter);
INSTR_TIME_ADD(dst->emission_counter, add->emission_counter);
diff --git a/src/backend/jit/llvm/llvmjit_expr.c
b/src/backend/jit/llvm/llvmjit_expr.c
index f114337f8e..1ad384f15e 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -121,7 +121,9 @@ llvm_compile_expr(ExprState *state)
LLVMValueRef v_aggnulls;
instr_time starttime;
+ instr_time deform_starttime;
instr_time endtime;
+ instr_time deform_endtime;
llvm_enter_fatal_on_oom();
@@ -315,10 +317,14 @@ llvm_compile_expr(ExprState *state)
*/
Re: Schema variables - new implementation for Postgres 15 (typo)
> On Thu, Dec 22, 2022 at 08:45:57PM +0100, Pavel Stehule wrote: > > From the first look it seems some major topics the discussion is evolving > > are about: > > > > * Validity of the use case. Seems to be quite convincingly addressed in > > [1] and > > [2]. > > > > * Complicated logic around invalidation, concurrent create/drop etc. (I > > guess > > the issue above is falling into the same category). > > > > * Concerns that session variables could repeat some problems of temporary > > tables. > > > > Why do you think so? The variable has no mvcc support - it is just stored > value with local visibility without mvcc support. There can be little bit > similar issues like with global temporary tables. Yeah, sorry for not being precise, I mean global temporary tables. This is not my analysis, I've simply picked up it was mentioned a couple of times here. The points above are not meant to serve as an objection against the patch, but rather to figure out if there are any gaps left to address and come up with some sort of plan with "committed" as a final destination.
Re: Schema variables - new implementation for Postgres 15 (typo)
Hi, I'm continuing review the patch slowly, and have one more issue plus one philosophical question. The issue have something to do with variables invalidation. Enabling debug_discard_caches = 1 (about which I've learned from this thread) and running this subset of the test suite: CREATE SCHEMA svartest; SET search_path = svartest; CREATE VARIABLE var3 AS int; CREATE OR REPLACE FUNCTION inc(int) RETURNS int AS $$ BEGIN LET svartest.var3 = COALESCE(svartest.var3 + $1, $1); RETURN var3; END; $$ LANGUAGE plpgsql; SELECT inc(1); SELECT inc(1); SELECT inc(1); crashes in my setup like this: #2 0x00b432d4 in ExceptionalCondition (conditionName=0xce9b99 "n >= 0 && n < list->length", fileName=0xce9c18 "list.c", lineNumber=770) at assert.c:66 #3 0x007d3acd in list_delete_nth_cell (list=0x18ab248, n=-3388) at list.c:770 #4 0x007d3b88 in list_delete_cell (list=0x18ab248, cell=0x18dc028) at list.c:842 #5 0x006bcb52 in sync_sessionvars_all (filter_lxid=true) at session_variable.c:524 #6 0x006bd4cb in SetSessionVariable (varid=16386, value=2, isNull=false) at session_variable.c:844 #7 0x006bd617 in SetSessionVariableWithSecurityCheck (varid=16386, value=2, isNull=false) at session_variable.c:885 #8 0x7f763b890698 in exec_stmt_let (estate=0x7ffcc6fd5190, stmt=0x18aa920) at pl_exec.c:5030 #9 0x7f763b88a746 in exec_stmts (estate=0x7ffcc6fd5190, stmts=0x180) at pl_exec.c:2116 #10 0x7f763b88a247 in exec_stmt_block (estate=0x7ffcc6fd5190, block=0x18aabf8) at pl_exec.c:1935 #11 0x7f763b889a49 in exec_toplevel_block (estate=0x7ffcc6fd5190, block=0x18aabf8) at pl_exec.c:1626 #12 0x7f763b8879df in plpgsql_exec_function (func=0x18781b0, fcinfo=0x18be110, simple_eval_estate=0x0, simple_eval_resowner=0x0, procedure_resowner=0x0, atomic=true) at pl_exec.c:615 #13 0x7f763b8a2320 in plpgsql_call_handler (fcinfo=0x18be110) at pl_handler.c:277 #14 0x00721716 in ExecInterpExpr (state=0x18bde28, econtext=0x18bd3d0, isnull=0x7ffcc6fd56d7) at execExprInterp.c:730 #15 0x00723642 in ExecInterpExprStillValid (state=0x18bde28, econtext=0x18bd3d0, isNull=0x7ffcc6fd56d7) at execExprInterp.c:1855 #16 0x0077a78b in ExecEvalExprSwitchContext (state=0x18bde28, econtext=0x18bd3d0, isNull=0x7ffcc6fd56d7) at ../../../src/include/executor/executor.h:344 #17 0x0077a7f4 in ExecProject (projInfo=0x18bde20) at ../../../src/include/executor/executor.h:378 #18 0x0077a9dc in ExecResult (pstate=0x18bd2c0) at nodeResult.c:136 #19 0x00738ea0 in ExecProcNodeFirst (node=0x18bd2c0) at execProcnode.c:464 #20 0x0072c6e3 in ExecProcNode (node=0x18bd2c0) at ../../../src/include/executor/executor.h:262 #21 0x0072f426 in ExecutePlan (estate=0x18bd098, planstate=0x18bd2c0, use_parallel_mode=false, operation=CMD_SELECT, sendTuples=true, numberTuples=0, direction=ForwardScanDirection, dest=0x18b3eb8, execute_once=true) at execMain.c:1691 #22 0x0072cf76 in standard_ExecutorRun (queryDesc=0x189c688, direction=ForwardScanDirection, count=0, execute_once=true) at execMain.c:423 #23 0x0072cdb3 in ExecutorRun (queryDesc=0x189c688, direction=ForwardScanDirection, count=0, execute_once=true) at execMain.c:367 #24 0x0099afdc in PortalRunSelect (portal=0x1866648, forward=true, count=0, dest=0x18b3eb8) at pquery.c:927 #25 0x0099ac99 in PortalRun (portal=0x1866648, count=9223372036854775807, isTopLevel=true, run_once=true, dest=0x18b3eb8, altdest=0x18b3eb8, qc=0x7ffcc6fd5a70) at pquery.c:771 #26 0x0099487d in exec_simple_query (query_string=0x17edcc8 "SELECT inc(1);") at postgres.c:1238 It seems that sync_sessionvars_all tries to remove a cell that either doesn't belong to the xact_recheck_varids or weird in some other way: +>>> p l - xact_recheck_varids->elements $81 = -3388 The second thing I wanted to ask about is a more strategical question. Does anyone have clear understanding where this patch is going? The thread is quite large, and it's hard to catch up with all the details -- it would be great if someone could summarize the current state of things, are there any outstanding design issues or not addressed concerns? >From the first look it seems some major topics the discussion is evolving are >about: * Validity of the use case. Seems to be quite convincingly addressed in [1] and [2]. * Complicated logic around invalidation, concurrent create/drop etc. (I guess the issue above is falling into the same category). * Concerns that session variables could repeat some problems of temporary tables. Is there anything else? [1]:
Re: Lazy JIT IR code generation to increase JIT speed with partitions
> On Thu, Jul 14, 2022 at 02:45:29PM +0200, David Geier wrote:
> On Mon, Jul 4, 2022 at 10:32 PM Andres Freund wrote:
> > On 2022-06-27 16:55:55 +0200, David Geier wrote:
> > > Indeed, the total JIT time increases the more modules are used. The
> > reason
> > > for this to happen is that the inlining pass loads and deserializes all
> > to
> > > be inlined modules (.bc files) from disk prior to inlining them via
> > > llvm::IRMover. There's already a cache for such modules in the code, but
> > it
> > > is currently unused. This is because llvm::IRMover takes the module to be
> > > inlined as std::unique_ptr. The by-value argument requires
> > > the source module to be moved, which means it cannot be reused
> > afterwards.
> > > The code is accounting for that by erasing the module from the cache
> > after
> > > inlining it, which in turns requires reloading the module next time a
> > > reference to it is encountered.
> > >
> > > Instead of each time loading and deserializing all to be inlined modules
> > > from disk, they can reside in the cache and instead be cloned via
> > > llvm::CloneModule() before they get inlined. Key to calling
> > > llvm::CloneModule() is fully deserializing the module upfront, instead of
> > > loading the module lazily. That is why I changed the call from
> > > LLVMGetBitcodeModuleInContext2() (which lazily loads the module via
> > > llvm::getOwningLazyBitcodeModule()) to LLVMParseBitCodeInContext2()
> > (which
> > > fully loads the module via llvm::parseBitcodeFile()). Beyond that it
> > seems
> > > like that prior to LLVM 13, cloning modules could fail with an assertion
> > > (not sure though if that would cause problems in a release build without
> > > assertions). Andres reported this problem back in the days here [1]. In
> > the
> > > meanwhile the issue got discussed in [2] and finally fixed for LLVM 13,
> > see
> > > [3].
> >
> > Unfortunately that doesn't work right now - that's where I had started. The
> > problem is that IRMover renames types. Which, in the case of cloned modules
> > unfortunately means that types used cloned modules are also renamed in the
> > "origin" module. Which then causes problems down the line, because parts of
> > the LLVM code match types by type names.
> >
> > That can then have the effect of drastically decreasing code generation
> > quality over time, because e.g. inlining never manages to find signatures
> > compatible.
Hi,
Thanks for the patch, looks quite interesting!
First to summarize things a bit: from what I understand there are two
suggestions on the table, one is about caching modules when doing
inlining, the second is about actual lazy jitting. Are those to tightly
coupled together, could they be split into two separate patches? It
would make it a bit easier to review and test.
I was playing with the caching part of the patch (still have to think
about the lazy jitting), which generally seems to be a good idea. From
what I see the main concern here is a chance that IRMover will rename
types, degrading performance of the generated code in long run. I have
to admit, I'm not fully sure mentioned LLVM 13 fix [1] completely
eliminates those concerns, somehow its description is formulated in not
very committing way ("don't know if this patch fixes ..., but it does
fix a few soundness issues that have crept in."). But I haven't found
any crashes or false asserts coming from the LLVM side (using v13),
running several rounds of regression tests with forced jitting, so a
point to the fix.
I would be curious to learn how Andres was troubleshooting type renaming
issues? Using LLVM 13 from packages, jitting the same query twice and
dumping the bitcode out showed some difference in types a-la
"%struct.ExprState.142" vs "%struct.ExprState.430" (from what I
understood, the whole thing is an identifier, including the number) with
the patch, but the very same changes are happening on the main branch as
well. Of course, I was inspecting bitcode only for certain queries, it
doesn't exclude that some other examples actually feature type renaming.
In general, it would be interesting to know how to do some sort of "smoke
tests" for the generated code, e.g. in case if LLVM has fixed this
particular issue, but they might reappear in the future?
I did few performance tests and got numbers similar to posted in the
thread, inlining time being impressively reduced (~10 times) as well as
(suspiciously) optimization time (~1.5 times). The approach was a bit
different though, I've run the sequence of example queries from the
thread using pgbench and checked jit counters from pgss.
Few small notes:
If caching of modules is safe from LLVM >= 13, I guess it should be
wrapped into a corresponding condition on LLVM_VERSION_MAJOR, right?
Why the assert about hasExternalLinkage was removed from the
llvmjit_inline.cpp?
For so much discussion about such a small change there is definitely not
enough commentaries in the code about dangers of cloning modules.
Re: Schema variables - new implementation for Postgres 15
> On Fri, Nov 04, 2022 at 03:17:18PM +0100, Pavel Stehule wrote: > > I've stumbled upon something that looks weird to me (inspired by the > > example from tests): > > > > =# create variable v2 as int; > > =# let v2 = 3; > > =# create view vv2 as select coalesce(v2, 0) + 1000 as result > > > > =# select * from vv2; > > result > > > > 1003 > > > > =# set force_parallel_mode to on; > > =# select * from vv2; > > result > > > > 1000 > > > > In the second select the actual work is done from a worker backend. > > Since values of session variables are stored in the backend local > > memory, it's not being shared with the worker and the value is not found > > in the hash map. Does this suppose to be like that? > > > > It looks like a bug, but parallel queries should be supported. > > The value of the variable is passed as parameter to the worker backend. But > probably somewhere the original reference was not replaced by parameter > > On Fri, Nov 04, 2022 at 10:17:13PM +0800, Julien Rouhaud wrote: > Hi, > > There's code to serialize and restore all used variables for parallel workers > (see code about PARAM_VARIABLE and queryDesc->num_session_variables / > queryDesc->plannedstmt->sessionVariables). I haven't reviewed that part yet, > but it's supposed to be working. Blind guess would be that it's missing > something in expression walker. I see, thanks. I'll take a deeper look, my initial assumption was due to the fact that in the worker case create_sessionvars_hashtables is getting called for every query.
Re: Schema variables - new implementation for Postgres 15
> On Fri, Nov 04, 2022 at 05:58:06AM +0100, Pavel Stehule wrote: > Hi > > fix clang warning I've stumbled upon something that looks weird to me (inspired by the example from tests): =# create variable v2 as int; =# let v2 = 3; =# create view vv2 as select coalesce(v2, 0) + 1000 as result =# select * from vv2; result 1003 =# set force_parallel_mode to on; =# select * from vv2; result 1000 In the second select the actual work is done from a worker backend. Since values of session variables are stored in the backend local memory, it's not being shared with the worker and the value is not found in the hash map. Does this suppose to be like that?
Re: Schema variables - new implementation for Postgres 15
> On Thu, Oct 13, 2022 at 07:41:32AM +0200, Pavel Stehule wrote:
> rebased with simplified code related to usage of pfree function
Thanks for the patch, great work!
I've got a couple of questions, although I haven't fully finished reviewing yet
(so more to come):
* I'm curious about ALTER VARIABLE. Current implementation allows altering only
the name, schema or the owner -- why not e.g. immutability?
* psql tab completion implementation mentions that CREATE VARIABLE could be
used inside CREATE SCHEMA:
/* CREATE VARIABLE --- is allowed inside CREATE SCHEMA, so use TailMatches
*/
/* Complete CREATE VARIABLE with AS */
else if (TailMatches("IMMUTABLE"))
Is that correct? It doesn't like it works, and from what I see it requires
some modifications in transformCreateSchemaStmt and schema_stmt.
* psql describe mentions the following:
/*
* Most functions in this file are content to print an empty table when
* there are no matching objects. We intentionally deviate from that
* here, but only in !quiet mode, for historical reasons.
*/
I guess it's taken from listTables, and the extended versions says "because
of the possibility that the user is confused about what the two pattern
arguments mean". Are those historical reasons apply to variables as well?
Re: pg_stat_statements and "IN" conditions
> On Sat, Sep 24, 2022 at 04:07:14PM +0200, Dmitry Dolgov wrote: > > On Fri, Sep 16, 2022 at 09:25:13PM +0300, Sergei Kornilov wrote: > > Hello! > > > > Unfortunately the patch needs another rebase due to the recent split of > > guc.c (0a20ff54f5e66158930d5328f89f087d4e9ab400) > > > > I'm reviewing a patch on top of a previous commit and noticed a failed test: > > > > # Failed test 'no parameters missing from postgresql.conf.sample' > > # at t/003_check_guc.pl line 82. > > # got: '1' > > # expected: '0' > > # Looks like you failed 1 test of 3. > > t/003_check_guc.pl .. > > > > The new option has not been added to the postgresql.conf.sample > > > > PS: I would also like to have such a feature. It's hard to increase > > pg_stat_statements.max or lose some entries just because some ORM sends > > requests with a different number of parameters. > > Thanks! I'll post the rebased version soon. And here it is. >From 327673290bfad8cebc00f9706a25d11034d2245d Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sun, 24 Jul 2022 11:43:25 +0200 Subject: [PATCH v9] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on number of parameters, because every element of ArrayExpr is jumbled. In certain situations it's undesirable, especially if the list becomes too large. Make Const expressions contribute nothing to the jumble hash if they're a part of an ArrayExpr, which length is larger than specified threshold. Allow to configure the threshold via the new GUC const_merge_threshold with the default value zero, which disables this feature. Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane Tested-by: Chengxi Sun --- .../expected/pg_stat_statements.out | 412 ++ .../pg_stat_statements/pg_stat_statements.c | 33 +- .../sql/pg_stat_statements.sql| 107 + doc/src/sgml/config.sgml | 26 ++ doc/src/sgml/pgstatstatements.sgml| 28 +- src/backend/utils/misc/guc_tables.c | 13 + src/backend/utils/misc/postgresql.conf.sample | 2 +- src/backend/utils/misc/queryjumble.c | 105 - src/include/utils/queryjumble.h | 5 +- 9 files changed, 712 insertions(+), 19 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index ff0166fb9d..858cf49e66 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -1102,4 +1102,416 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%'; 2 (1 row) +-- +-- Consts merging +-- +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- No merging +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | calls ++--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1 + SELECT pg_stat_statements_reset() | 1 + SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 +(7 rows) + +-- Normal +SET const_merge_threshold = 5; +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | c
Re: pg_stat_statements and "IN" conditions
> On Fri, Sep 16, 2022 at 09:25:13PM +0300, Sergei Kornilov wrote: > Hello! > > Unfortunately the patch needs another rebase due to the recent split of guc.c > (0a20ff54f5e66158930d5328f89f087d4e9ab400) > > I'm reviewing a patch on top of a previous commit and noticed a failed test: > > # Failed test 'no parameters missing from postgresql.conf.sample' > # at t/003_check_guc.pl line 82. > # got: '1' > # expected: '0' > # Looks like you failed 1 test of 3. > t/003_check_guc.pl .. > > The new option has not been added to the postgresql.conf.sample > > PS: I would also like to have such a feature. It's hard to increase > pg_stat_statements.max or lose some entries just because some ORM sends > requests with a different number of parameters. Thanks! I'll post the rebased version soon.
Re: pg_stat_statements and "IN" conditions
> On Sat, Mar 26, 2022 at 06:40:35PM +0100, Dmitry Dolgov wrote: > > On Mon, Mar 14, 2022 at 04:51:50PM +0100, Dmitry Dolgov wrote: > > > On Mon, Mar 14, 2022 at 11:38:23AM -0400, Tom Lane wrote: > > > Dmitry Dolgov <9erthali...@gmail.com> writes: > > > > On Mon, Mar 14, 2022 at 11:23:17AM -0400, Tom Lane wrote: > > > >> I do find it odd that the proposed patch doesn't cause the *entire* > > > >> list to be skipped over. That seems like extra complexity and > > > >> confusion > > > >> to no benefit. > > > > > > > That's a bit surprising for me, I haven't even thought that folks could > > > > think this is an odd behaviour. As I've mentioned above, the original > > > > idea was to give some clues about what was inside the collapsed array, > > > > but if everyone finds it unnecessary I can of course change it. > > > > > > But if what we're doing is skipping over an all-Consts list, then the > > > individual Consts would be elided from the pg_stat_statements entry > > > anyway, no? All that would remain is information about how many such > > > Consts there were, which is exactly the information you want to drop. > > > > Hm, yes, you're right. I guess I was thinking about this more like about > > shortening some text with ellipsis, but indeed no actual Consts will end > > up in the result anyway. Thanks for clarification, will modify the > > patch! > > Here is another iteration. Now the patch doesn't leave any trailing > Consts in the normalized query, and contains more documentation. I hope > it's getting better. Hi, Here is the rebased version, with no other changes. >From 5092a6914f1e55636bb8beed2251322cc0f1eec6 Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sun, 24 Jul 2022 11:43:25 +0200 Subject: [PATCH v8] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on number of parameters, because every element of ArrayExpr is jumbled. In certain situations it's undesirable, especially if the list becomes too large. Make Const expressions contribute nothing to the jumble hash if they're a part of an ArrayExpr, which length is larger than specified threshold. Allow to configure the threshold via the new GUC const_merge_threshold with the default value zero, which disables this feature. Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane Tested-by: Chengxi Sun --- .../expected/pg_stat_statements.out | 412 ++ .../pg_stat_statements/pg_stat_statements.c | 33 +- .../sql/pg_stat_statements.sql| 107 + doc/src/sgml/config.sgml | 26 ++ doc/src/sgml/pgstatstatements.sgml| 28 +- src/backend/utils/misc/guc.c | 13 + src/backend/utils/misc/queryjumble.c | 105 - src/include/utils/queryjumble.h | 5 +- 8 files changed, 711 insertions(+), 18 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index ff0166fb9d..858cf49e66 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -1102,4 +1102,416 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%'; 2 (1 row) +-- +-- Consts merging +-- +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- No merging +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | calls ++--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8
[RFC] Add jit deform_counter
Hi,
I've noticed that JIT performance counter generation_counter seems to include
actions, relevant for both jit_expressions and jit_tuple_deforming options. It
means one can't directly see what is the influence of jit_tuple_deforming
alone, which would be helpful when adjusting JIT options. To make it better a
new counter can be introduced, does it make sense?
>From bf4c137e5ec87e49be8b799424e2ca2ad0cf1b9c Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Sat, 11 Jun 2022 11:54:40 +0200
Subject: [RFC PATCH 1/2] Add deform_counter
At the moment generation_counter includes time spent on both JITing
expressions and tuple deforming. Those are configured independently via
corresponding options (jit_expressions and jit_tuple_deforming), which
means there is no way to find out what fraction of time tuple deforming
alone is taking.
Add deform_counter dedicated to tuple deforming, which allows seeing
more directly the influence jit_tuple_deforming is having on the query.
---
src/backend/commands/explain.c | 7 ++-
src/backend/jit/jit.c | 1 +
src/backend/jit/llvm/llvmjit_expr.c | 6 ++
src/include/jit/jit.h | 3 +++
4 files changed, 16 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index 5d1f7089da..e03a34726d 100644
--- a/src/backend/commands/explain.c
+++ b/src/backend/commands/explain.c
@@ -882,6 +882,7 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
/* calculate total time */
INSTR_TIME_SET_ZERO(total_time);
+ /* don't add deform_counter, it's included into generation_counter */
INSTR_TIME_ADD(total_time, ji->generation_counter);
INSTR_TIME_ADD(total_time, ji->inlining_counter);
INSTR_TIME_ADD(total_time, ji->optimization_counter);
@@ -909,8 +910,9 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
{
ExplainIndentText(es);
appendStringInfo(es->str,
- "Timing: %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n",
+ "Timing: %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms, %s %.3f ms\n",
"Generation", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->generation_counter),
+ "Deforming", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->deform_counter),
"Inlining", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->inlining_counter),
"Optimization", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->optimization_counter),
"Emission", 1000.0 * INSTR_TIME_GET_DOUBLE(ji->emission_counter),
@@ -937,6 +939,9 @@ ExplainPrintJIT(ExplainState *es, int jit_flags, JitInstrumentation *ji)
ExplainPropertyFloat("Generation", "ms",
1000.0 * INSTR_TIME_GET_DOUBLE(ji->generation_counter),
3, es);
+ ExplainPropertyFloat("Deforming", "ms",
+ 1000.0 * INSTR_TIME_GET_DOUBLE(ji->deform_counter),
+ 3, es);
ExplainPropertyFloat("Inlining", "ms",
1000.0 * INSTR_TIME_GET_DOUBLE(ji->inlining_counter),
3, es);
diff --git a/src/backend/jit/jit.c b/src/backend/jit/jit.c
index 18d168f1af..d57b33c088 100644
--- a/src/backend/jit/jit.c
+++ b/src/backend/jit/jit.c
@@ -185,6 +185,7 @@ InstrJitAgg(JitInstrumentation *dst, JitInstrumentation *add)
{
dst->created_functions += add->created_functions;
INSTR_TIME_ADD(dst->generation_counter, add->generation_counter);
+ INSTR_TIME_ADD(dst->deform_counter, add->deform_counter);
INSTR_TIME_ADD(dst->inlining_counter, add->inlining_counter);
INSTR_TIME_ADD(dst->optimization_counter, add->optimization_counter);
INSTR_TIME_ADD(dst->emission_counter, add->emission_counter);
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index b6b6512ef1..6e5f0c1ef0 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -121,7 +121,9 @@ llvm_compile_expr(ExprState *state)
LLVMValueRef v_aggnulls;
instr_time starttime;
+ instr_time deform_starttime;
instr_time endtime;
+ instr_time deform_endtime;
llvm_enter_fatal_on_oom();
@@ -315,10 +317,14 @@ llvm_compile_expr(ExprState *state)
*/
if (tts_ops && desc && (context->base.flags & PGJIT_DEFORM))
{
+ INSTR_TIME_SET_CURRENT(deform_starttime);
l_jit_deform =
slot_compile_deform(context, desc,
tts_ops,
op->d.fetch.last_var);
+ INSTR_TIME_SET_CURRENT(deform_endtime);
+ INSTR_TIME_ACCUM_DIFF(context->base.instr.deform_counter,
+ deform_endtime, deform_starttime);
}
if (l_jit_deform)
diff --git a/src/include/jit/jit.h b/src/include/jit/jit.h
index d194033209..c4f20ecbbd 100644
--- a/src/include/jit/jit.h
+++ b/src/include/jit/jit.h
@@ -32,6 +32,9 @@ typedef struct JitInstrumentation
/* accumulated time to generate code */
instr_time generation_counter;
+ /* accumulated time to deform tuples, included into generation_counter */
+ instr_time deform_counter;
+
/* accumulated time for inlining */
Re: Limiting memory allocation
> On Wed, May 18, 2022 at 04:49:24PM -0400, Joe Conway wrote: > On 5/18/22 16:20, Alvaro Herrera wrote: > > On 2022-May-18, Joe Conway wrote: > > > > > On 5/18/22 11:11, Alvaro Herrera wrote: > > > > > > Apparently, if the cgroup goes over the "high" limit, the processes are > > > > *throttled*. Then if the group goes over the "max" limit, OOM-killer is > > > > invoked. > > > > > You may be misinterpreting "throttle" in this context. From [1]: > > > > > > The memory.high boundary on the other hand can be set > > > much more conservatively. When hit, it throttles > > > allocations by forcing them into direct reclaim to > > > work off the excess, but it never invokes the OOM > > > killer. > > > > Well, that means the backend processes don't do their expected task > > (process some query) but instead they have to do "direct reclaim". I > > don't know what that is, but it sounds like we'd need to add > > Linux-specific code in order for this to fix anything. > > Postgres does not need to do anything. The kernel just does its thing (e.g. > clearing page cache or swapping out anon memory) more aggressively than > normal to clear up some space for the impending allocation. > > > And what would we do in such a situation anyway? Seems like our > > best hope would be to> get malloc() to return NULL and have the > > resulting transaction abort free enough memory that things in other > > backends can continue to run. > > With the right hooks an extension could detect the memory pressure in an OS > specific way and return null. > > > *If* there is a way to have cgroups make Postgres do that, then that > > would be useful enough. > > Memory accounting under cgroups (particularly v2) can provide the signal > needed for a Linux specific extension to do that. To elaborate a bit on this, Linux PSI feature (in the context of containers, cgroups v2 only) [1] would allow a userspace application to register a trigger on memory pressure exceeding some threshold. The pressure here is not exactly how much memory is allocated, but rather memory stall, and the whole machinery would involve polling -- but still sounds interesting in the context of this thread. [1]: https://www.kernel.org/doc/Documentation/accounting/psi.rst
Re: pg_stat_statements and "IN" conditions
> On Mon, Mar 14, 2022 at 04:51:50PM +0100, Dmitry Dolgov wrote: > > On Mon, Mar 14, 2022 at 11:38:23AM -0400, Tom Lane wrote: > > Dmitry Dolgov <9erthali...@gmail.com> writes: > > > On Mon, Mar 14, 2022 at 11:23:17AM -0400, Tom Lane wrote: > > >> I do find it odd that the proposed patch doesn't cause the *entire* > > >> list to be skipped over. That seems like extra complexity and confusion > > >> to no benefit. > > > > > That's a bit surprising for me, I haven't even thought that folks could > > > think this is an odd behaviour. As I've mentioned above, the original > > > idea was to give some clues about what was inside the collapsed array, > > > but if everyone finds it unnecessary I can of course change it. > > > > But if what we're doing is skipping over an all-Consts list, then the > > individual Consts would be elided from the pg_stat_statements entry > > anyway, no? All that would remain is information about how many such > > Consts there were, which is exactly the information you want to drop. > > Hm, yes, you're right. I guess I was thinking about this more like about > shortening some text with ellipsis, but indeed no actual Consts will end > up in the result anyway. Thanks for clarification, will modify the > patch! Here is another iteration. Now the patch doesn't leave any trailing Consts in the normalized query, and contains more documentation. I hope it's getting better. >From 8226635c221a659097deb6ea64626a587296ea60 Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sat, 12 Mar 2022 14:42:02 +0100 Subject: [PATCH v7] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on number of parameters, because every element of ArrayExpr is jumbled. In certain situations it's undesirable, especially if the list becomes too large. Make Const expressions contribute nothing to the jumble hash if they're a part of an ArrayExpr, which length is larger than specified threshold. Allow to configure the threshold via the new GUC const_merge_threshold with the default value zero, which disables this feature. Reviewed-by: Zhihong Yu, Sergey Dudoladov, Robert Haas, Tom Lane Tested-by: Chengxi Sun --- .../expected/pg_stat_statements.out | 412 ++ .../pg_stat_statements/pg_stat_statements.c | 33 +- .../sql/pg_stat_statements.sql| 107 + doc/src/sgml/config.sgml | 26 ++ doc/src/sgml/pgstatstatements.sgml| 28 +- src/backend/utils/misc/guc.c | 13 + src/backend/utils/misc/queryjumble.c | 105 - src/include/utils/queryjumble.h | 5 +- 8 files changed, 711 insertions(+), 18 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index e0abe34bb6..de3970e462 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -1077,4 +1077,416 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%'; 2 (1 row) +-- +-- Consts merging +-- +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- No merging +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | calls ++--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1 + SELECT pg_stat_statements_reset() | 1 + SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"
Re: MDAM techniques and Index Skip Scan patch
> On Wed, Mar 23, 2022 at 05:32:46PM -0400, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: > > On Tue, Mar 22, 2022 at 04:55:49PM -0400, Tom Lane wrote: > >> In short: I would throw out just about all the planner infrastructure > >> that's been proposed so far. It looks bulky, expensive, and > >> drastically undercommented, and I don't think it's buying us anything > >> of commensurate value. > > > Broadly speaking planner related changes proposed in the patch so far > > are: UniqueKey, taken from the neighbour thread about select distinct; > > list of uniquekeys to actually pass information about the specified > > loose scan prefix into nbtree; some verification logic to prevent > > applying skipping when it's not supported. I can imagine taking out > > UniqueKeys and passing loose scan prefix in some other form (the other > > parts seems to be essential) -- is that what you mean? > > My point is that for pure loose scans --- that is, just optimizing a scan, > not doing AM-based duplicate-row-elimination --- you do not need to pass > any new data to btree at all. It can infer what to do on the basis of the > set of index quals it's handed. > > The bigger picture here is that I think the reason this patch series has > failed to progress is that it's too scattershot. You need to pick a > minimum committable feature and get that done, and then you can move on > to the next part. I think the minimum committable feature is loose scans, > which will require a fair amount of work in access/nbtree/ but very little > new planner code, and will be highly useful in their own right even if we > never do anything more. > > In general I feel that the UniqueKey code is a solution looking for a > problem, and that treating it as the core of the patchset is a mistake. > We should be driving this work off of what nbtree needs to make progress, > and not building more infrastructure elsewhere than we have to. Maybe > we'll end up with something that looks like UniqueKeys, but I'm far from > convinced of that. I see. I'll need some thinking time about how it may look like (will probably return with more questions). The CF item could be set to RwF, what would you say, Jesper?
Re: MDAM techniques and Index Skip Scan patch
> On Tue, Mar 22, 2022 at 04:55:49PM -0400, Tom Lane wrote: > Peter Geoghegan writes: > > Like many difficult patches, the skip scan patch is not so much > > troubled by problems with the implementation as it is troubled by > > *ambiguity* about the design. Particularly concerning how skip scan > > meshes with existing designs, as well as future designs -- > > particularly designs for other MDAM techniques. I've started this > > thread to have a big picture conversation about how to think about > > these things. > > Peter asked me off-list to spend some time thinking about the overall > direction we ought to be pursuing here. I have done that, and here > are a few modest suggestions. Thanks. To make sure I understand your proposal better, I have a couple of questions: > In short: I would throw out just about all the planner infrastructure > that's been proposed so far. It looks bulky, expensive, and > drastically undercommented, and I don't think it's buying us anything > of commensurate value. Broadly speaking planner related changes proposed in the patch so far are: UniqueKey, taken from the neighbour thread about select distinct; list of uniquekeys to actually pass information about the specified loose scan prefix into nbtree; some verification logic to prevent applying skipping when it's not supported. I can imagine taking out UniqueKeys and passing loose scan prefix in some other form (the other parts seems to be essential) -- is that what you mean?
Re: MDAM techniques and Index Skip Scan patch
ruct Path List *pathkeys; /* sort ordering of path's output */ /* pathkeys is a List of PathKey nodes; see above */ + + List *uniquekeys; /* the unique keys, or NIL if none */ } Path; /* Macro for extracting a path's parameterization relids; beware double eval */ diff --git a/src/include/nodes/pg_list.h b/src/include/nodes/pg_list.h index 2cb9d1371d..4ac871fd16 100644 --- a/src/include/nodes/pg_list.h +++ b/src/include/nodes/pg_list.h @@ -567,6 +567,7 @@ extern pg_nodiscard List *list_delete_last(List *list); extern pg_nodiscard List *list_delete_first_n(List *list, int n); extern pg_nodiscard List *list_delete_nth_cell(List *list, int n); extern pg_nodiscard List *list_delete_cell(List *list, ListCell *cell); +extern bool list_is_subset(const List *members, const List *target); extern List *list_union(const List *list1, const List *list2); extern List *list_union_ptr(const List *list1, const List *list2); diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h index 620eeda2d6..bb6d730e93 100644 --- a/src/include/optimizer/pathnode.h +++ b/src/include/optimizer/pathnode.h @@ -27,6 +27,7 @@ extern int compare_fractional_path_costs(Path *path1, Path *path2, double fraction); extern void set_cheapest(RelOptInfo *parent_rel); extern void add_path(RelOptInfo *parent_rel, Path *new_path); +extern void add_unique_path(RelOptInfo *parent_rel, Path *new_path); extern bool add_path_precheck(RelOptInfo *parent_rel, Cost startup_cost, Cost total_cost, List *pathkeys, Relids required_outer); diff --git a/src/include/optimizer/paths.h b/src/include/optimizer/paths.h index 0c3a0b90c8..3dfa21adad 100644 --- a/src/include/optimizer/paths.h +++ b/src/include/optimizer/paths.h @@ -229,6 +229,9 @@ extern List *build_join_pathkeys(PlannerInfo *root, extern List *make_pathkeys_for_sortclauses(PlannerInfo *root, List *sortclauses, List *tlist); +extern List *make_pathkeys_for_uniquekeys(PlannerInfo *root, + List *sortclauses, + List *tlist); extern void initialize_mergeclause_eclasses(PlannerInfo *root, RestrictInfo *restrictinfo); extern void update_mergeclause_eclasses(PlannerInfo *root, @@ -255,4 +258,10 @@ extern PathKey *make_canonical_pathkey(PlannerInfo *root, extern void add_paths_to_append_rel(PlannerInfo *root, RelOptInfo *rel, List *live_childrels); +extern bool query_has_uniquekeys_for(PlannerInfo *root, + List *exprs, + bool allow_multinulls); + +extern List *build_uniquekeys(PlannerInfo *root, List *sortclauses); + #endif /* PATHS_H */ -- 2.32.0 >From 1f61de293ad1eef7e91971c4c26aab031ae205c0 Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sat, 8 Jan 2022 17:16:49 +0100 Subject: [PATCH v41 2/6] Index skip scan Allow IndexOnlyScan to skip duplicated tuples based on search key prefix (a trick also known as Index Skip Scan or Loose Index Scan, see in the wiki [1]). The idea is to avoid scanning all equal values of a key, as soon as a new value is found, restart the search by looking for a larger value. This approach is much faster when the index has many equal keys. Implemented via equipping IndexPath with indexskipprefix field and creating an extra IndexPath with such prefix if suitable unique expressions are present. On the execution size a new index am function amskip is introduced to provide index specific implementation for such skipping. To simplify potential amskip implementations, ExecSupportsBackwardScan now returns false in case if index skip scan is used, otherwise amskip has to deal with scroll cursor and be prepared to handle different advance/read directions. ExecSupportsBackwardScan may seem to have too big scope, but looks like now it used only together with cursorOptions checks for CURSOR_OPT_SCROLL. Original patch and design were proposed by Thomas Munro [2], revived and improved by Dmitry Dolgov and Jesper Pedersen. [1] https://wiki.postgresql.org/wiki/Loose_indexscan [2] https://www.postgresql.org/message-id/flat/CADLWmXXbTSBxP-MzJuPAYSsL_2f0iPm5VWPbCvDbVvfX93FKkw%40mail.gmail.com Author: Jesper Pedersen, Dmitry Dolgov Reviewed-by: Thomas Munro, David Rowley, Floris Van Nee, Kyotaro Horiguchi, Tomas Vondra, Peter Geoghegan --- contrib/bloom/blutils.c | 1 + doc/src/sgml/config.sgml | 15 ++ doc/src/sgml/indexam.sgml | 43 ++ doc/src/sgml/indices.sgml | 23 +++ src/backend/access/brin/brin.c| 1 + src/backend/access/gin/ginutil.c | 1 + src/backend/access/gist/gist.c| 1 + src/backend/access/hash/hash.c| 1 + src/backend/access/index/indexam.c| 16 ++ src/backend/access/spgist/spgutils.c | 1 + src/backend/commands/explain.c| 23 +++ src/backend/executor/execAmi.c
Re: pg_stat_statements and "IN" conditions
> On Mon, Mar 14, 2022 at 11:38:23AM -0400, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: > > On Mon, Mar 14, 2022 at 11:23:17AM -0400, Tom Lane wrote: > >> I do find it odd that the proposed patch doesn't cause the *entire* > >> list to be skipped over. That seems like extra complexity and confusion > >> to no benefit. > > > That's a bit surprising for me, I haven't even thought that folks could > > think this is an odd behaviour. As I've mentioned above, the original > > idea was to give some clues about what was inside the collapsed array, > > but if everyone finds it unnecessary I can of course change it. > > But if what we're doing is skipping over an all-Consts list, then the > individual Consts would be elided from the pg_stat_statements entry > anyway, no? All that would remain is information about how many such > Consts there were, which is exactly the information you want to drop. Hm, yes, you're right. I guess I was thinking about this more like about shortening some text with ellipsis, but indeed no actual Consts will end up in the result anyway. Thanks for clarification, will modify the patch!
Re: pg_stat_statements and "IN" conditions
> On Mon, Mar 14, 2022 at 11:23:17AM -0400, Tom Lane wrote: > Robert Haas writes: > > I do find it odd that the proposed patch doesn't cause the *entire* > list to be skipped over. That seems like extra complexity and confusion > to no benefit. That's a bit surprising for me, I haven't even thought that folks could think this is an odd behaviour. As I've mentioned above, the original idea was to give some clues about what was inside the collapsed array, but if everyone finds it unnecessary I can of course change it.
Re: pg_stat_statements and "IN" conditions
> On Mon, Mar 14, 2022 at 11:02:16AM -0400, Robert Haas wrote: > On Mon, Mar 14, 2022 at 10:57 AM Dmitry Dolgov <9erthali...@gmail.com> wrote: > > Well, yeah, the commit message is somewhat clumsy in this regard. It > > works almost in the way you've described, except if the list is all > > constants and long enough to satisfy the threshold then *first N > > elements (where N == threshold) will be jumbled -- to leave at least > > some traces of it in pgss. > > But that seems to me to be a thing we would not want. Why do you think > otherwise? Hm. Well, if the whole list would be not jumbled, the transformation would look like this, right? WHERE x in (1, 2, 3, 4, 5) -> WHERE x in (...) Leaving some number of original elements in place gives some clue for the reader about at least what type of data the array has contained. Which hopefully makes it a bit easier to identify even in the collapsed form: WHERE x in (1, 2, 3, 4, 5) -> WHERE x in (1, 2, ...) > > I'm not sure if I follow the last point. WHERE x in (1,3) and x = > > any(array[1,3]) are two different things for sure, but in which way are > > they going to be mixed together because of this change? My goal was to > > make only the following transformation, without leaving any uncertainty: > > > > WHERE x in (1, 2, 3, 4, 5) -> WHERE x in (1, 2, ...) > > WHERE x = any(array[1, 2, 3, 4, 5]) -> WHERE x = any(array[1, 2, ...]) > > I understand. I think it might be OK to transform both of those > things, but I don't think it's very clear either from the comments or > the nonexistent documentation that both of those cases are affected -- > and I think that needs to be clear. Not sure exactly how to do that, > just saying that we can't add behavior unless it will be clear to > users what the behavior is. Yep, got it.
Re: pg_stat_statements and "IN" conditions
> On Mon, Mar 14, 2022 at 10:17:57AM -0400, Robert Haas wrote: > On Sat, Mar 12, 2022 at 9:11 AM Dmitry Dolgov <9erthali...@gmail.com> wrote: > > Here is the limited version of list collapsing functionality, which > > doesn't utilize eval_const_expressions and ignores most of the stuff > > except ArrayExprs. Any thoughts/more suggestions? > > The proposed commit message says this commit intends to "Make Consts > contribute nothing to the jumble hash if they're part of a series and > at position further that specified threshold." I'm not sure whether > that's what the patch actually implements because I can't immediately > understand the new logic you've added, but I think if we did what that > sentence said then, supposing the threshold is set to 1, it would > result in producing the same hash for "x in (1,2)" that we do for "x > in (1,3)" but a different hash for "x in (2,3)" which does not sound > like what we want. What I would have thought we'd do is: if the list > is all constants and long enough to satisfy the threshold then nothing > in the list gets jumbled. Well, yeah, the commit message is somewhat clumsy in this regard. It works almost in the way you've described, except if the list is all constants and long enough to satisfy the threshold then *first N elements (where N == threshold) will be jumbled -- to leave at least some traces of it in pgss. > I'm a little surprised that there's not more context-awareness in this > code. It seems that it applies to every ArrayExpr found in the query, > which I think would extend to cases beyond something = IN(whatever). > In particular, any use of ARRAY[] in the query would be impacted. Now, > the comments seem to imply that's pretty intentional, but from the > user's point of view, WHERE x in (1,3) and x = any(array[1,3]) are two > different things. If anything like this is to be adopted, we certainly > need to be precise about exactly what it is doing and which cases are > covered. I'm not sure if I follow the last point. WHERE x in (1,3) and x = any(array[1,3]) are two different things for sure, but in which way are they going to be mixed together because of this change? My goal was to make only the following transformation, without leaving any uncertainty: WHERE x in (1, 2, 3, 4, 5) -> WHERE x in (1, 2, ...) WHERE x = any(array[1, 2, 3, 4, 5]) -> WHERE x = any(array[1, 2, ...]) > I thought of looking at the documentation to see whether you'd tried > to clarify this there, and found that you hadn't written any. > > In short, I think this patch is not really very close to being in > committable shape even if nobody were objecting to the concept. Sure, I'll add documentation. To be honest I'm not targeting PG15 with this, just want to make some progress. Thanks for the feedback, I'm glad to see it coming!
Re: pg_stat_statements and "IN" conditions
> On Thu, Mar 10, 2022 at 12:11:59PM -0500, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: > > New status: Waiting on Author > > > This seems incorrect, as the only feedback I've got was "this is a bad > > idea", and no reaction on follow-up questions. > > I changed the status because it seems to me there is no chance of > this being committed as-is. > > 1. I think an absolute prerequisite before we could even consider > changing the query jumbler rules this much is to do the work that was > put off when the jumbler was moved into core: that is, provide some > honest support for multiple query-ID generation methods being used at > the same time. Even if you successfully make a case for > pg_stat_statements to act this way, other consumers of query IDs > aren't going to be happy with it. > > 2. You haven't made a case for it. The original complaint was > about different lengths of IN lists not being treated as equivalent, > but this patch has decided to do I'm-not-even-sure-quite-what > about treating different Params as equivalent. Plus you're trying > to invoke eval_const_expressions in the jumbler; that is absolutely > Not OK, for both safety and semantic reasons. > > If you backed off to just treating ArrayExprs containing different > numbers of Consts as equivalent, maybe that'd be something we could > adopt without fixing point 1. I don't think anything that fuzzes the > treatment of Params can get away with that, though. Here is the limited version of list collapsing functionality, which doesn't utilize eval_const_expressions and ignores most of the stuff except ArrayExprs. Any thoughts/more suggestions? >From ce9f2ed2466d28dbbef3310383d84eba58e5791b Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Sat, 12 Mar 2022 14:42:02 +0100 Subject: [PATCH v6] Prevent jumbling of every element in ArrayExpr pg_stat_statements produces multiple entries for queries like SELECT something FROM table WHERE col IN (1, 2, 3, ...) depending on number of parameters, because every element of ArrayExpr is jumbled. Make Consts contribute nothing to the jumble hash if they're part of a series and at position further that specified threshold. Reviewed-by: Zhihong Yu, Sergey Dudoladov Tested-by: Chengxi Sun --- .../expected/pg_stat_statements.out | 412 ++ .../pg_stat_statements/pg_stat_statements.c | 26 +- .../sql/pg_stat_statements.sql| 107 + src/backend/utils/misc/guc.c | 13 + src/backend/utils/misc/queryjumble.c | 236 +- src/include/utils/queryjumble.h | 10 +- 6 files changed, 791 insertions(+), 13 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index e0abe34bb6..e05a6f565a 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -1077,4 +1077,416 @@ SELECT COUNT(*) FROM pg_stat_statements WHERE query LIKE '%SELECT GROUPING%'; 2 (1 row) +-- +-- Consts merging +-- +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- No merging +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9); + id | data ++-- +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8, 9, 10); + id | data ++-- +(0 rows) + +SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C"; + query | calls ++--- + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9) | 1 + SELECT * FROM test_merge WHERE id IN ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) | 1 + SELECT pg_stat_statements_reset() | 1 + SELECT query, calls FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 +(7 rows) + +-- Normal +SET const_merge_threshold = 5; +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +
