Re: proposal: new polymorphic types - commontype and commontypearray
pá 13. 3. 2020 v 23:42 odesílatel Tom Lane napsal: > Pavel Stehule writes: > > [ anycompatible-types-20191127.patch ] > > I'm starting to review this patch seriously. I've found some bugs and > things I didn't like, and the documentation certainly needs work, but > I think I can get it to a committable state before too much longer. > > What I want to talk about right now is some preliminary refactoring > that I'd like to do, as shown in the 0001 patch below. (0002 is the > rest of the patch as I currently have it.) There are two main things > in it: > > 1. Rearrange the macros in pseudotypes.c so that we don't have any > pure-boilerplate functions that aren't built by the macros. I don't > think this should be controversial, as it's not changing anything > functionally. > > 2. Refactor the function signature validation logic in pg_proc.c and > pg_aggregate.c to avoid having duplicate logic between those two. > I did that by creating new functions in parse_coerce.c (for lack of > a better place) that say whether a proposed result type or aggregate > transition type is valid given a particular set of declared input types. > The reason that this might be controversial is that it forces a slightly > less precise error detail message to be issued, since the call site that's > throwing the error doesn't know exactly which rule was being violated. > (For example, before there was a specific error message about anyrange > result requiring an anyrange input, and now there isn't.) > > I think this is all right, mainly because we'd probably end up with > less-precise messages anyway for the more complex rules that 0002 is > going to add. If anybody's really hot about it, we could complicate > the API, say by having the call sites pass in the primary error message > or by having the checking subroutines pass back an errdetail string. > > We definitely need to do *something* about that, because it's already > the case that pg_aggregate.c is out of step with pg_proc.c about > polymorphism rules --- it's not enforcing the anyrange rule. I think > there's probably no user-reachable bug in that, because an aggregate > is constrained by its implementation functions for which the rule > would be enforced, but it still seems not good. > Unfortunately the error message " A function returning "anyrange" must have at least one "anyrange" argument." will be missing. This prerequisite is not intuitive. Second question is if we need special rule for anyrange. Regards Pavel > Thoughts? > > regards, tom lane > >
Re: make check crashes on POWER8 machine
В Fri, 13 Mar 2020 10:56:15 -0400 Tom Lane пишет: > Victor Wagner writes: > > Justin Pryzby wrote: > >> On Fri, Mar 13, 2020 at 10:29:13AM +0300, Victor Wagner wrote: > >>> I've encountered a problem with Postgres on PowerPC machine. > > >> Is it related to > >> https://www.postgresql.org/message-id/20032.1570808731%40sss.pgh.pa.us > >> https://bugzilla.kernel.org/show_bug.cgi?id=205183 > > > I don't think so. At least I cannot see any signal handler-related > > stuff in the trace, but see lots of calls to stored procedure > > executor instead. > > Read the whole thread. We fixed the issue with recursion in the > postmaster (9abb2bfc0); but the intermittent failure in > infinite_recurse is exactly the same as what we've been seeing for a > long time in the buildfarm, and there is zero doubt that it's that > kernel bug. I've tried to cherry-pick commit 9abb2bfc8 into REL_12_STABLE and rerun make check in loop. Oops, on 543 run it segfaults with same symptoms as before. Here is link to new core and logs https://drive.google.com/file/d/1oF-0fKHKvFn6FaJ3u-v36p9W0EBAY9nb/view?usp=sharing I'll try to do this simple test (run make check repeatedly) with master. There is some time until end of weekend when this machine is non needed by anyone else, so I have time to couple of thousands runs. -- Victor Wagner
RE: Planning counters in pg_stat_statements (using pgss_store)
imai.yoshik...@fujitsu.com wrote
> On Thu, Mar 12, 2020 at 6:37 PM, Julien Rouhaud wrote:
>> On Thu, Mar 12, 2020 at 1:11 PM Marco Slot <
> marco.slot@
> > wrote:
>> > On Thu, Mar 12, 2020 at 11:31 AM Julien Rouhaud <
> rjuju123@
> >
>> wrote:
>> > > There's at least the current version of IVM patchset that lacks the
>> > > querytext. Looking at various extensions, I see that pg_background
>> > > and pglogical call pg_plan_query internally but shouldn't have any
>> > > issue providing the query text. But there's also citus extension,
>> > > which don't keep around the query string at least when distributing
>> > > plans, which makes sense since it's of no use and they're heavily
>> > > modifying the original Query. I think that citus folks opinion on
>> > > the subject would be useful, so I'm Cc-ing Marco.
>> >
>> > Most of the time we keep our Query * data structures in a form that
>> > can be deparsed back into a query string by a modified copy of
>> > ruleutils.c, so we could generate a correct query string if absolutely
>> > necessary, though there are performance-sensitive cases where we'd
>> > rather not have the deparsing overhead.
>>
>> Yes, deparsing is probably too expensive for that use case.
>>
>> > In case of INSERT..SELECT into a distributed table, we might call
>> > pg_plan_query on the SELECT part (Query *) and send the output into a
>> > DestReceiver that sends tuples into shards of the distributed table
>> > via COPY. The fact that SELECT does not show up in pg_stat_statements
>> > separately is generally fine because it's an implementation detail,
>> > and it would probably be a little confusing because the user never ran
>> > the SELECT query. Moreover, the call to pg_plan_query would already be
>> > reflected in the planning or execution time of the top-level query, so
>> > it would be double counted if it had its own entry.
>>
>> Well, surprising statements can already appears in pg_stat_statements
>> when
>> you use some psql features, or if you have triggers as those will run
>> additional
>> queries under the hood.
>>
>> The difference here is that since citus is a CustomNode, underlying calls
>> to
>> planner will be accounted for that node, and that's indeed annoying. I
>> can see
>> that citus is doing some calls to spi_exec or
>> Executor* (in ExecuteLocalTaskPlan), which could also trigger
>> pg_stat_statements, but I don't know if a queryid is present there.
>>
>> > Another case is when some of the shards turn out to be local to the
>> > server that handles the distributed query. In that case we plan the
>> > queries on those shards via pg_plan_query instead of deparsing and
>> > sending the query string to a remote server. It would be less
>> > confusing for these queries to show in pg_stat_statements, because the
>> > queries on the shards on remote servers will show up as well. However,
>> > this is a performance-sensitive code path where we'd rather avoid
>> > deparsing.
>>
>> Agreed.
>>
>> > In general, I'd prefer if there was no requirement to pass a correct
>> > query string. I'm ok with passing "SELECT 'citus_internal'" or just ""
>> > if that does not lead to issues. Passing NULL to signal that the
>> > planner call should not be tracked separately does seem a bit cleaner.
>>
>> That's very interesting feedback, thanks! I'm not a fan of giving a way
>> for
>> queries to say that they want to be ignored by pg_stat_statements, but
>> double
>> counting the planning counters seem even worse, so I'm +0.5 to accept
>> NULL
>> query string in the planner, incidentally making pgss ignore those.
>
> It is preferable that we can count various queries statistics as much as
> possible
> but if it causes overhead even when without using pg_stat_statements, we
> would
> not have to force them to set appropriate query_text.
> About settings a fixed string in query_text, I think it doesn't make much
> sense
> because users can't take any actions by seeing those queries' stats.
> Moreover, if
> we set a fixed string in query_text to avoid pg_stat_statement's errors,
> codes
> would be inexplicable for other developers who don't know there's such
> requirements.
> After all, I agree accepting NULL query string in the planner.
>
> I don't know it is useful but there are also codes that avoid an error
> when
> sourceText is NULL.
>
> executor_errposition(EState *estate, int location)
> {
> ...
> /* Can't do anything if source text is not available */
> if (estate == NULL || estate->es_sourceText == NULL)
> }
>
> --
> Yoshikazu Imai
Hello Imai,
My understanding of V5 patch, that checks for Non-Zero queryid,
manage properly case where sourceText is NULL.
A NULL sourceText means that there was no Parsing for the associated
query, if there was no Parsing, there is no queryid (queryId=0),
and no planning counters update.
It doesn't change pg_plan_query behaviour (no regression for Citus, IVM,
...),
and was tested with success for IVM.
If my understand
RE: Planning counters in pg_stat_statements (using pgss_store)
> I don't know it is useful but there are also codes that avoid an error when
> sourceText is NULL.
> executor_errposition(EState *estate, int location)
> {
> ...
>/* Can't do anything if source text is not available */
>if (estate == NULL || estate->es_sourceText == NULL)
> }
or maybe would you prefer to replace the Non-Zero queryid test
by Non-NULL sourcetext one ?
--
Sent from: https://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html
Re: make check crashes on POWER8 machine
Victor Wagner writes: > Tom Lane пишет: >> Read the whole thread. We fixed the issue with recursion in the >> postmaster (9abb2bfc0); but the intermittent failure in >> infinite_recurse is exactly the same as what we've been seeing for a >> long time in the buildfarm, and there is zero doubt that it's that >> kernel bug. > I've tried to cherry-pick commit 9abb2bfc8 into REL_12_STABLE and rerun > make check in loop. Oops, on 543 run it segfaults with same symptoms > as before. Unsurprising, because it's a kernel bug. Maybe you could try cherry-picking the patch proposed at kernel.org (see other thread). regards, tom lane
Re: proposal: new polymorphic types - commontype and commontypearray
Pavel Stehule writes: > pá 13. 3. 2020 v 23:42 odesílatel Tom Lane napsal: >> The reason that this might be controversial is that it forces a slightly >> less precise error detail message to be issued, since the call site that's >> throwing the error doesn't know exactly which rule was being violated. >> (For example, before there was a specific error message about anyrange >> result requiring an anyrange input, and now there isn't.) > Unfortunately the error message " A function returning "anyrange" must have > at least one "anyrange" argument." will be missing. Yeah, that's what I said. But does it really add anything beyond the proposed text "A function returning a polymorphic type must have at least one matching polymorphic argument"? I don't think it'd be terribly helpful to say "A function returning anyelement must have at least one anyelement, anyarray, anynonarray, anyenum, or anyrange argument", and for sure such an error message would be a pain to maintain. regards, tom lane
Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager
On Fri, Mar 13, 2020 at 7:02 PM Dilip Kumar wrote: > > Apart from that, I have also extended the solution for the page lock. > And, I have also broken down the 3rd patch in two parts for relation > extension and for the page lock. > Thanks, I have made a number of cosmetic changes and written appropriate commit messages for all patches. See the attached patch series and let me know your opinion? BTW, did you get a chance to test page locks by using the extension which I have posted above or by some other way? I think it is important to test page-lock related patches now. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com v7-0001-Assert-that-we-don-t-acquire-a-heavyweight-lock-on-a.patch Description: Binary data v7-0002-Add-assert-to-ensure-that-page-locks-don-t-participa.patch Description: Binary data v7-0003-Allow-relation-extension-lock-to-conflict-among-para.patch Description: Binary data v7-0004-Allow-page-lock-to-conflict-among-parallel-group-mem.patch Description: Binary data
Re: proposal: new polymorphic types - commontype and commontypearray
so 14. 3. 2020 v 14:26 odesílatel Tom Lane napsal: > Pavel Stehule writes: > > pá 13. 3. 2020 v 23:42 odesílatel Tom Lane napsal: > >> The reason that this might be controversial is that it forces a slightly > >> less precise error detail message to be issued, since the call site > that's > >> throwing the error doesn't know exactly which rule was being violated. > >> (For example, before there was a specific error message about anyrange > >> result requiring an anyrange input, and now there isn't.) > > > Unfortunately the error message " A function returning "anyrange" must > have > > at least one "anyrange" argument." will be missing. > > Yeah, that's what I said. But does it really add anything beyond the > proposed text "A function returning a polymorphic type must have at least > one matching polymorphic argument"? I don't think it'd be terribly > helpful to say "A function returning anyelement must have at least one > anyelement, anyarray, anynonarray, anyenum, or anyrange argument", and > for sure such an error message would be a pain to maintain. > The error message in your first patch is ok for all types without anyrange. A behave of this type is more strict and +/- different than from other polymorphic types. Pavel > regards, tom lane >
Re: RecoveryWalAll and RecoveryWalStream wait events
On 2020/02/19 21:46 Fujii Masao : >> I agree to the former, I think RecoveryWalInterval works well enough. >RecoveryWalInterval sounds confusing to me... IMHO as a user, I prefer RecoveryRetrieveRetryInterval because it's easy to understand this wait_event is related to the parameter 'wal_retrieve_retry_interval'. Also from the point of balance, the explanation of RecoveryRetrieveRetryInterval is lengthy, but I sometimes feel explanations of wait_events in the manual are so simple that it's hard to understand well. >Waiting when WAL data is not available from any kind of sources >(local, archive or stream) before trying again to retrieve WAL data, I think 'local' means pg_wal here, but the comment on WaitForWALToBecomeAvailable() says checking pg_wal in standby mode is 'not documented', so I'm a little bit worried that users may be confused. Regards, -- Torikoshi Atsushi
Re: PATCH: add support for IN and @> in functional-dependency statistics use
Hi, I've pushed the first part of the patch, adding ScalarArrayOpExpr as supported clause for functional dependencies, and then also doing the same for MCV lists. As discussed, I'm not going to do anything about the array containment clauses for PG13, that needs more discussion. I have a bunch of additional improvements for extended stats, discussed in [1]. I'll wait a bit for buildfarm and maybe some feedback before pushing those. [1] https://www.postgresql.org/message-id/flat/20200113230008.g67iyk4cs3xbnjju@development regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: backend type in log_line_prefix?
On Fri, Feb 21, 2020 at 10:09:38AM +0100, Peter Eisentraut wrote: > From 75ac8ed0c47801712eb2aa300d9cb29767d2e121 Mon Sep 17 00:00:00 2001 > From: Peter Eisentraut > Date: Thu, 20 Feb 2020 18:16:39 +0100 > Subject: [PATCH v2 3/4] Add backend type to csvlog and optionally > log_line_prefix > diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml > index c1128f89ec..206778b1c3 100644 > --- a/doc/src/sgml/config.sgml > +++ b/doc/src/sgml/config.sgml > @@ -6470,6 +6470,11 @@ What to Log characters are copied straight to the log line. Some escapes are only recognized by session processes, and will be treated as empty by background processes such as the main server process. Status ... Escape Effect Session only > Application name > yes > > + > + %b > + Backend process type > + yes => should say "no", it's not blank for background processes: > + > + if (MyProcPid == PostmasterPid) > + backend_type_str = "postmaster"; > + else if (MyBackendType == B_BG_WORKER) > + backend_type_str = > MyBgworkerEntry->bgw_type; > + else > + backend_type_str = > pgstat_get_backend_desc(MyBackendType); -- Justin
Re: [PATCH] Incremental sort (was: PoC: Partial sort)
On Fri, Mar 13, 2020 at 8:23 PM James Coleman wrote: > > On Friday, March 13, 2020, Tomas Vondra wrote: >> >> On Fri, Mar 13, 2020 at 04:31:16PM -0400, James Coleman wrote: >>> >>> On Fri, Mar 13, 2020 at 2:23 PM James Coleman wrote: On Tue, Mar 10, 2020 at 10:44 PM Tomas Vondra wrote: > 3) Most of the execution plans look reasonable, except that some of the > plans look like this: > > > QUERY PLAN >- > Limit > -> GroupAggregate > Group Key: t.a, t.b, t.c, t.d > -> Incremental Sort > Sort Key: t.a, t.b, t.c, t.d > Presorted Key: t.a, t.b, t.c > -> Incremental Sort > Sort Key: t.a, t.b, t.c > Presorted Key: t.a, t.b > -> Index Scan using t_a_b_idx on t >(10 rows) > > i.e. there are two incremental sorts on top of each other, with > different prefixes. But this this is not a new issue - it happens with > queries like this: > >SELECT a, b, c, d, count(*) FROM ( > SELECT * FROM t ORDER BY a, b, c >) foo GROUP BY a, b, c, d limit 1000; > > i.e. there's a subquery with a subset of pathkeys. Without incremental > sort the plan looks like this: > > QUERY PLAN >- > Limit > -> GroupAggregate > Group Key: t.a, t.b, t.c, t.d > -> Sort > Sort Key: t.a, t.b, t.c, t.d > -> Sort > Sort Key: t.a, t.b, t.c > -> Seq Scan on t >(8 rows) > > so essentially the same plan shape. What bugs me though is that there > seems to be some sort of memory leak, so that this query consumes > gigabytes os RAM before it gets killed by OOM. But the memory seems not > to be allocated in any memory context (at least MemoryContextStats don't > show anything like that), so I'm not sure what's going on. > > Reproducing it is fairly simple: > >CREATE TABLE t (a bigint, b bigint, c bigint, d bigint); >INSERT INTO t SELECT > 1000*random(), 1000*random(), 1000*random(), 1000*random() >FROM generate_series(1,1000) s(i); >CREATE INDEX idx ON t(a,b); >ANALYZE t; > >EXPLAIN ANALYZE SELECT a, b, c, d, count(*) >FROM (SELECT * FROM t ORDER BY a, b, c) foo GROUP BY a, b, c, d >LIMIT 100; While trying to reproduce this, instead of lots of memory usage, I got the attached assertion failure instead. >>> >>> >>> And, without the EXPLAIN ANALYZE was able to get this one, which will >>> probably be a lot more helpful. >>> >> >> Hmmm, I'll try reproducing it, but can you investigate the values in the >> Assert? I mean, it fails on this: >> >> Assert(total_allocated == context->mem_allocated); >> >> so can you get a core or attach to the process using gdb, and see what's >> the expected / total value? I've reproduced this on multiple machines (though all are Ubuntu or Debian derivatives...I don't think that's likely to matter). A core dump is ~150MB, so I've uploaded to Dropbox [1]. I didn't find an obvious first-level member of Tuplesortstate that was covered by either of the two blocks in the AllocSet (both are 8KB in size). James [1]: https://www.dropbox.com/s/jwndwp4634hzywk/aset_assertion_failure.core?dl=0
resolve_generic_type() is over-complex and under-correct
parse_coerce.c's resolve_generic_type() is written on the assumption
that it might be asked to infer any polymorphic type's concrete type
given any other polymorphic type's concrete type. This is overkill,
because the only call sites look like
anyelement_type = resolve_generic_type(ANYELEMENTOID,
anyarray_type,
ANYARRAYOID);
subtype = resolve_generic_type(ANYELEMENTOID,
anyrange_type,
ANYRANGEOID);
anyarray_type = resolve_generic_type(ANYARRAYOID,
anyelement_type,
ANYELEMENTOID);
(There are two occurrences of each of these in funcapi.c,
and nothing anywhere else.)
But that's a good thing, because resolve_generic_type() gets some
of the un-exercised cases wrong. Notably, it appears to believe
that anyrange is the same type variable as anyelement: if asked
to resolve anyarray from anyrange, it will produce the array over
the concrete range type, where it should produce the array over
the range's subtype.
Rather than fix this up, I'm inclined to just nuke resolve_generic_type()
altogether, and replace the call sites with direct uses of the underlying
lookups such as get_array_type(). I think this is simpler to understand
as well as being significantly less code.
I also noticed that there's an asymmetry in funcapi.c's usage pattern:
it can resolve anyelement from either anyarray or anyrange, but it
can only resolve anyarray from anyelement not anyrange. This results
in warts like so:
regression=# create function foo(x anyrange) returns anyarray language sql
as 'select array[lower(x),upper(x)]';
CREATE FUNCTION
regression=# select foo(int4range(6,9));
foo
---
{6,9}
(1 row)
-- so far so good, but let's try it with OUT parameters:
regression=# create function foo2(x anyrange, out lu anyarray, out ul anyarray)
language sql
as 'select array[lower(x),upper(x)], array[upper(x),lower(x)]';
CREATE FUNCTION
regression=# select * from foo2(int4range(6,9));
ERROR: cache lookup failed for type 0
So that's a bug that needs fixing in any case.
regards, tom lane
Re: [PATCH] Incremental sort (was: PoC: Partial sort)
On Sat, Mar 14, 2020 at 12:07 PM James Coleman wrote: > > On Fri, Mar 13, 2020 at 8:23 PM James Coleman wrote: > > > > On Friday, March 13, 2020, Tomas Vondra > > wrote: > >> > >> On Fri, Mar 13, 2020 at 04:31:16PM -0400, James Coleman wrote: > >>> > >>> On Fri, Mar 13, 2020 at 2:23 PM James Coleman wrote: > > > On Tue, Mar 10, 2020 at 10:44 PM Tomas Vondra > wrote: > > 3) Most of the execution plans look reasonable, except that some of the > > plans look like this: > > > > > > QUERY PLAN > >- > > Limit > > -> GroupAggregate > > Group Key: t.a, t.b, t.c, t.d > > -> Incremental Sort > > Sort Key: t.a, t.b, t.c, t.d > > Presorted Key: t.a, t.b, t.c > > -> Incremental Sort > > Sort Key: t.a, t.b, t.c > > Presorted Key: t.a, t.b > > -> Index Scan using t_a_b_idx on t > >(10 rows) > > > > i.e. there are two incremental sorts on top of each other, with > > different prefixes. But this this is not a new issue - it happens with > > queries like this: > > > >SELECT a, b, c, d, count(*) FROM ( > > SELECT * FROM t ORDER BY a, b, c > >) foo GROUP BY a, b, c, d limit 1000; > > > > i.e. there's a subquery with a subset of pathkeys. Without incremental > > sort the plan looks like this: > > > > QUERY PLAN > >- > > Limit > > -> GroupAggregate > > Group Key: t.a, t.b, t.c, t.d > > -> Sort > > Sort Key: t.a, t.b, t.c, t.d > > -> Sort > > Sort Key: t.a, t.b, t.c > > -> Seq Scan on t > >(8 rows) > > > > so essentially the same plan shape. What bugs me though is that there > > seems to be some sort of memory leak, so that this query consumes > > gigabytes os RAM before it gets killed by OOM. But the memory seems not > > to be allocated in any memory context (at least MemoryContextStats > > don't > > show anything like that), so I'm not sure what's going on. > > > > Reproducing it is fairly simple: > > > >CREATE TABLE t (a bigint, b bigint, c bigint, d bigint); > >INSERT INTO t SELECT > > 1000*random(), 1000*random(), 1000*random(), 1000*random() > >FROM generate_series(1,1000) s(i); > >CREATE INDEX idx ON t(a,b); > >ANALYZE t; > > > >EXPLAIN ANALYZE SELECT a, b, c, d, count(*) > >FROM (SELECT * FROM t ORDER BY a, b, c) foo GROUP BY a, b, c, d > >LIMIT 100; > > While trying to reproduce this, instead of lots of memory usage, I got > the attached assertion failure instead. > >>> > >>> > >>> And, without the EXPLAIN ANALYZE was able to get this one, which will > >>> probably be a lot more helpful. > >>> > >> > >> Hmmm, I'll try reproducing it, but can you investigate the values in the > >> Assert? I mean, it fails on this: > >> > >> Assert(total_allocated == context->mem_allocated); > >> > >> so can you get a core or attach to the process using gdb, and see what's > >> the expected / total value? > > I've reproduced this on multiple machines (though all are Ubuntu or > Debian derivatives...I don't think that's likely to matter). A core > dump is ~150MB, so I've uploaded to Dropbox [1]. > > I didn't find an obvious first-level member of Tuplesortstate that was > covered by either of the two blocks in the AllocSet (both are 8KB in > size). > > James > > [1]: > https://www.dropbox.com/s/jwndwp4634hzywk/aset_assertion_failure.core?dl=0 And...I think I might have found out the issue (though haven't proved it 100% yet or fixed it): The incremental sort node calls `tuplesort_puttupleslot`, which switches the memory context to `sortcontext`. It then calls `puttuple_common`. `puttuple_common` may then call `grow_memtuples` which reallocs space for `sortstate->memtuples`, but `memtuples` is elsewhere allocated in the memory context maincontext. I had earlier in this debugging process noticed that `sortcontext` was allocated in `maincontext`, which seemed conceptually odd if our goal is to reuse the sort state, and I also found a comment that needed to be changed relative to cleaning up the per-sort context (that talks about it freeing the sort state itself), but the `memtuples` array was in fact freed additionally at reset, so it seemed safe. Given this issue though, I think I'm going to go ahead and rework so that the `memtuples` array lies within the `sortcontext` instead. James
Re: [PATCH] Incremental sort (was: PoC: Partial sort)
On Sat, Mar 14, 2020 at 12:24 PM James Coleman wrote: > > On Sat, Mar 14, 2020 at 12:07 PM James Coleman wrote: > > > > On Fri, Mar 13, 2020 at 8:23 PM James Coleman wrote: > > > > > > On Friday, March 13, 2020, Tomas Vondra > > > wrote: > > >> > > >> On Fri, Mar 13, 2020 at 04:31:16PM -0400, James Coleman wrote: > > >>> > > >>> On Fri, Mar 13, 2020 at 2:23 PM James Coleman wrote: > > > > > > On Tue, Mar 10, 2020 at 10:44 PM Tomas Vondra > > wrote: > > > 3) Most of the execution plans look reasonable, except that some of > > > the > > > plans look like this: > > > > > > > > > QUERY PLAN > > >- > > > Limit > > > -> GroupAggregate > > > Group Key: t.a, t.b, t.c, t.d > > > -> Incremental Sort > > > Sort Key: t.a, t.b, t.c, t.d > > > Presorted Key: t.a, t.b, t.c > > > -> Incremental Sort > > > Sort Key: t.a, t.b, t.c > > > Presorted Key: t.a, t.b > > > -> Index Scan using t_a_b_idx on t > > >(10 rows) > > > > > > i.e. there are two incremental sorts on top of each other, with > > > different prefixes. But this this is not a new issue - it happens > > > with > > > queries like this: > > > > > >SELECT a, b, c, d, count(*) FROM ( > > > SELECT * FROM t ORDER BY a, b, c > > >) foo GROUP BY a, b, c, d limit 1000; > > > > > > i.e. there's a subquery with a subset of pathkeys. Without > > > incremental > > > sort the plan looks like this: > > > > > > QUERY PLAN > > >- > > > Limit > > > -> GroupAggregate > > > Group Key: t.a, t.b, t.c, t.d > > > -> Sort > > > Sort Key: t.a, t.b, t.c, t.d > > > -> Sort > > > Sort Key: t.a, t.b, t.c > > > -> Seq Scan on t > > >(8 rows) > > > > > > so essentially the same plan shape. What bugs me though is that there > > > seems to be some sort of memory leak, so that this query consumes > > > gigabytes os RAM before it gets killed by OOM. But the memory seems > > > not > > > to be allocated in any memory context (at least MemoryContextStats > > > don't > > > show anything like that), so I'm not sure what's going on. > > > > > > Reproducing it is fairly simple: > > > > > >CREATE TABLE t (a bigint, b bigint, c bigint, d bigint); > > >INSERT INTO t SELECT > > > 1000*random(), 1000*random(), 1000*random(), 1000*random() > > >FROM generate_series(1,1000) s(i); > > >CREATE INDEX idx ON t(a,b); > > >ANALYZE t; > > > > > >EXPLAIN ANALYZE SELECT a, b, c, d, count(*) > > >FROM (SELECT * FROM t ORDER BY a, b, c) foo GROUP BY a, b, c, d > > >LIMIT 100; > > > > While trying to reproduce this, instead of lots of memory usage, I got > > the attached assertion failure instead. > > >>> > > >>> > > >>> And, without the EXPLAIN ANALYZE was able to get this one, which will > > >>> probably be a lot more helpful. > > >>> > > >> > > >> Hmmm, I'll try reproducing it, but can you investigate the values in the > > >> Assert? I mean, it fails on this: > > >> > > >> Assert(total_allocated == context->mem_allocated); > > >> > > >> so can you get a core or attach to the process using gdb, and see what's > > >> the expected / total value? > > > > I've reproduced this on multiple machines (though all are Ubuntu or > > Debian derivatives...I don't think that's likely to matter). A core > > dump is ~150MB, so I've uploaded to Dropbox [1]. > > > > I didn't find an obvious first-level member of Tuplesortstate that was > > covered by either of the two blocks in the AllocSet (both are 8KB in > > size). > > > > James > > > > [1]: > > https://www.dropbox.com/s/jwndwp4634hzywk/aset_assertion_failure.core?dl=0 > > And...I think I might have found out the issue (though haven't proved > it 100% yet or fixed it): > > The incremental sort node calls `tuplesort_puttupleslot`, which > switches the memory context to `sortcontext`. It then calls > `puttuple_common`. `puttuple_common` may then call `grow_memtuples` > which reallocs space for `sortstate->memtuples`, but `memtuples` is > elsewhere allocated in the memory context maincontext. > > I had earlier in this debugging process noticed that `sortcontext` was > allocated in `maincontext`, which seemed conceptually odd if our goal > is to reuse the sort state, and I also found a comment that needed to > be changed relative to cl
Re: Additional improvements to extended statistics
On Fri, Mar 13, 2020 at 04:54:51PM +, Dean Rasheed wrote:
On Mon, 9 Mar 2020 at 00:06, Tomas Vondra wrote:
On Mon, Mar 09, 2020 at 01:01:57AM +0100, Tomas Vondra wrote:
>
>Attaches is an updated patch series
>with parts 0002 and 0003 adding tests demonstrating the issue and then
>fixing it (both shall be merged to 0001).
>
One day I won't forget to actually attach the files ...
0001-0003 look reasonable to me.
One minor point -- there are now 2 code blocks that are basically the
same, looping over a list of clauses, calling clause_selectivity() and
then applying the "s1 = s1 + s2 - s1 * s2" formula. Perhaps they could
be combined into a new function (clauselist_selectivity_simple_or(),
say). I guess it would need to be passed the initial starting
selectivity s1, but it ought to help reduce code duplication.
Attached is a patch series rebased on top of the current master, after
committing the ScalarArrayOpExpr enhancements. I've updated the OR patch
to get rid of the code duplication, and barring objections I'll get it
committed shortly together with the two parts improving test coverage.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From cbe6cf599e52fb2335345ddc6736b0139d113760 Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Thu, 5 Mar 2020 01:47:16 +0100
Subject: [PATCH 1/4] Improve test coverage for multi-column MCV lists
The regression tests for extended statistics were not testing a couple
of important cases for the MCV lists:
* IS NOT NULL clauses - We did have queries with IS NULL clauses, but
not the negative case.
* clauses with variable on the right - All the clauses had the Var on
the left, i.e. (Var op Const), so this adds (Const op Var) too.
* columns with fixed-length types passed by reference - All columns
were using either by-value or varlena types, so add a test with
UUID columns too. This matters for (de)serialization.
* NULL-only dimension - When one of the columns contains only NULL
values, we treat it a a special case during (de)serialization.
* arrays containing NULL - When the constant parameter contains NULL
value, we need to handle it correctly during estimation, for all
IN, ANY and ALL clauses.
Discussion:
https://www.postgresql.org/message-id/flat/13902317.Eha0YfKkKy%40pierred-pdoc
Author: Tomas Vondra
---
src/test/regress/expected/stats_ext.out | 237 +++-
src/test/regress/sql/stats_ext.sql | 102 +-
2 files changed, 335 insertions(+), 4 deletions(-)
diff --git a/src/test/regress/expected/stats_ext.out
b/src/test/regress/expected/stats_ext.out
index 9fa659c71d..2ef19c590a 100644
--- a/src/test/regress/expected/stats_ext.out
+++ b/src/test/regress/expected/stats_ext.out
@@ -785,18 +785,36 @@ SELECT * FROM check_estimated_rows('SELECT * FROM
mcv_lists WHERE a = 1 AND b =
1 | 50
(1 row)
+SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE 1 = a AND
''1'' = b');
+ estimated | actual
+---+
+ 1 | 50
+(1 row)
+
SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE a < 1 AND b
< ''1''');
estimated | actual
---+
1 | 50
(1 row)
+SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE 1 > a AND
''1'' > b');
+ estimated | actual
+---+
+ 1 | 50
+(1 row)
+
SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE a <= 0 AND b
<= ''0''');
estimated | actual
---+
1 | 50
(1 row)
+SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE 0 >= a AND
''0'' >= b');
+ estimated | actual
+---+
+ 1 | 50
+(1 row)
+
SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE a = 1 AND b
= ''1'' AND c = 1');
estimated | actual
---+
@@ -809,12 +827,24 @@ SELECT * FROM check_estimated_rows('SELECT * FROM
mcv_lists WHERE a < 5 AND b <
1 | 50
(1 row)
+SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE a < 5 AND
''1'' > b AND 5 > c');
+ estimated | actual
+---+
+ 1 | 50
+(1 row)
+
SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE a <= 4 AND b
<= ''0'' AND c <= 4');
estimated | actual
---+
1 | 50
(1 row)
+SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE 4 >= a AND
''0'' >= b AND 4 >= c');
+ estimated | actual
+---+
+ 1 | 50
+(1 row)
+
SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE a = 1 OR b =
''1'' OR c = 1');
estimated | actual
---+
@@ -833,30 +863,60 @@ SELECT * FROM check_estimated_rows('SELECT * FROM
mcv_lists WHERE a IN (1, 2, 51
8 |200
(1 row)
+SELECT * FROM check_estimated_rows('SELECT * FROM mcv_lists WHERE a IN (1, 2,
51, 52, N
Re: Planning counters in pg_stat_statements (using pgss_store)
On Sat, Mar 14, 2020 at 03:04:00AM -0700, legrand legrand wrote:
> imai.yoshik...@fujitsu.com wrote
> > On Thu, Mar 12, 2020 at 6:37 PM, Julien Rouhaud wrote:
> >> That's very interesting feedback, thanks! I'm not a fan of giving a way
> >> for
> >> queries to say that they want to be ignored by pg_stat_statements, but
> >> double
> >> counting the planning counters seem even worse, so I'm +0.5 to accept
> >> NULL
> >> query string in the planner, incidentally making pgss ignore those.
> >
> > It is preferable that we can count various queries statistics as much as
> > possible
> > but if it causes overhead even when without using pg_stat_statements, we
> > would
> > not have to force them to set appropriate query_text.
> > About settings a fixed string in query_text, I think it doesn't make much
> > sense
> > because users can't take any actions by seeing those queries' stats.
> > Moreover, if
> > we set a fixed string in query_text to avoid pg_stat_statement's errors,
> > codes
> > would be inexplicable for other developers who don't know there's such
> > requirements.
> > After all, I agree accepting NULL query string in the planner.
> >
> > I don't know it is useful but there are also codes that avoid an error
> > when
> > sourceText is NULL.
> >
> > executor_errposition(EState *estate, int location)
> > {
> > ...
> > /* Can't do anything if source text is not available */
> > if (estate == NULL || estate->es_sourceText == NULL)
I'm wondering if that's really possible. But pgss uses the QueryDesc, which
should always have a query text (since pgss relies on that).
> My understanding of V5 patch, that checks for Non-Zero queryid,
> manage properly case where sourceText is NULL.
>
> A NULL sourceText means that there was no Parsing for the associated
> query, if there was no Parsing, there is no queryid (queryId=0),
> and no planning counters update.
>
> It doesn't change pg_plan_query behaviour (no regression for Citus, IVM,
> ...),
> and was tested with success for IVM.
>
> If my understanding is wrong, then setting track_planning = false
> would still be the work arround for the very rare (no core) extension(s)
> that may hit the NULL query text assertion failure.
>
> What do you think about this ?
I don't think that's a correct assumption. I obviously didn't read all of
citus extension, but it looks like what's happening is that they get generate a
custom Query from the original one, with all the modification needed for
distributed execution and whatnot, which is then fed to the planner. I think
it's entirely mossible that the modified Query herits from a previously set
queryid, while still not really having a query text. And if citus doesn't do
that, it doesn't seem like an illegal use cuse anyway.
I'm instead attaching a v7 which removes the assert in pg_plan_query, and
modify pgss_planner_hook to also ignore queries without a query text, as this
seems the best option.
>From 8412a8792bbf230406af33e4c1a93b425ac983ab Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Thu, 28 Mar 2019 13:33:23 +0100
Subject: [PATCH v7 1/2] Pass query string to the planner
---
src/backend/commands/copy.c | 3 ++-
src/backend/commands/createas.c | 3 ++-
src/backend/commands/explain.c | 2 +-
src/backend/commands/extension.c | 2 +-
src/backend/commands/matview.c | 2 +-
src/backend/commands/portalcmds.c| 2 +-
src/backend/executor/functions.c | 1 +
src/backend/optimizer/plan/planner.c | 10 ++
src/backend/tcop/postgres.c | 13 -
src/backend/utils/cache/plancache.c | 3 ++-
src/include/optimizer/optimizer.h| 3 ++-
src/include/optimizer/planner.h | 4 +++-
src/include/tcop/tcopprot.h | 6 --
13 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index fbde9f88e7..efb1e0d03e 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -1580,7 +1580,8 @@ BeginCopy(ParseState *pstate,
}
/* plan the query */
- plan = pg_plan_query(query, CURSOR_OPT_PARALLEL_OK, NULL);
+ plan = pg_plan_query(query, pstate->p_sourcetext,
+ CURSOR_OPT_PARALLEL_OK, NULL);
/*
* With row level security and a user using "COPY relation TO", we
diff --git a/src/backend/commands/createas.c b/src/backend/commands/createas.c
index 3a5676fb39..9febdc5165 100644
--- a/src/backend/commands/createas.c
+++ b/src/backend/commands/createas.c
@@ -329,7 +329,8 @@ ExecCreateTableAs(ParseState *pstate, CreateTableAsStmt *stmt,
Assert(query->commandType == CMD_SELECT);
/* plan the query */
- plan = pg_plan_query(query, CURSOR_OPT_PARALLEL_OK, params);
+ plan = pg_plan_query(query, pstate->p_sourcetext,
+ CURSOR_OPT_PARALLEL_OK, params);
/*
* Use a snapshot with an updated command ID to ensure this query sees
diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c
index d901dc4a50..
Re: range_agg
On Fri, Mar 13, 2020 at 2:39 PM Alvaro Herrera wrote:
> Here's the rebased version.
>
> I just realized I didn't include the API change I proposed in
> https://postgr.es/m/20200306200343.GA625@alvherre.pgsql ...
Thanks for your help with this Alvaro!
I was just adding your changes to my own branch and I noticed your
v12-0001 has different parameter names here:
diff --git a/src/backend/utils/adt/multirangetypes.c
b/src/backend/utils/adt/multirangetypes.c
index f9dd0378cc..0c9afd5448 100644
--- a/src/backend/utils/adt/multirangetypes.c
+++ b/src/backend/utils/adt/multirangetypes.c
@@ -376,11 +375,11 @@ multirange_typanalyze(PG_FUNCTION_ARGS)
* pointer to a type cache entry.
*/
static MultirangeIOData *
-get_multirange_io_data(FunctionCallInfo fcinfo, Oid mltrngtypid,
IOFuncSelector func)
+get_multirange_io_data(FunctionCallInfo fcinfo, Oid rngtypid,
IOFuncSelector func)
{
MultirangeIOData *cache = (MultirangeIOData *) fcinfo->flinfo->fn_extra;
-if (cache == NULL || cache->typcache->type_id != mltrngtypid)
+if (cache == NULL || cache->typcache->type_id != rngtypid)
{
int16 typlen;
booltypbyval;
@@ -389,9 +388,9 @@ get_multirange_io_data(FunctionCallInfo fcinfo,
Oid mltrngtypid, IOFuncSelector
cache = (MultirangeIOData *)
MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
sizeof(MultirangeIOData));
-cache->typcache = lookup_type_cache(mltrngtypid,
TYPECACHE_MULTIRANGE_INFO);
+cache->typcache = lookup_type_cache(rngtypid,
TYPECACHE_MULTIRANGE_INFO);
if (cache->typcache->rngtype == NULL)
-elog(ERROR, "type %u is not a multirange type", mltrngtypid);
+elog(ERROR, "type %u is not a multirange type", rngtypid);
/* get_type_io_data does more than we need, but is convenient */
get_type_io_data(cache->typcache->rngtype->type_id,
I'm pretty sure mltrngtypid is the correct name here. Right? Let me
know if I'm missing something. :-)
Yours,
Paul
Re: Feature improvement: can we add queryId for pg_catalog.pg_stat_activity view?
On Tue, Mar 03, 2020 at 04:24:59PM +0100, Julien Rouhaud wrote:
>
> cfbot reports a failure since 2f9661311b (command completion tag
> change), so here's a rebased v6, no change otherwise.
Conflict with 8e8a0becb3 (Unify several ways to tracking backend type), thanks
again to cfbot, rebased v7 attached.
>From dda1ab659a44c9a6375ee05d249baa2ec552 Mon Sep 17 00:00:00 2001
From: Julien Rouhaud
Date: Mon, 18 Mar 2019 18:55:50 +0100
Subject: [PATCH v7 1/2] Expose queryid in pg_stat_activity and log_line_prefix
Similarly to other fields in pg_stat_activity, only the queryid from the top
level statements are exposed, and if the backends status isn't active then the
queryid from the last executed statements is displayed.
Also add a %Q placeholder to include the queryid in the log_line_prefix, which
will also only expose top level statements.
Author: Julien Rouhaud
Reviewed-by: Evgeny Efimkin, Michael Paquier
Discussion: https://postgr.es/m/CA+8PKvQnMfOE-c3YLRwxOsCYXQDyP8VXs6CDtMZp1V4=d4l...@mail.gmail.com
---
.../pg_stat_statements/pg_stat_statements.c | 179 --
doc/src/sgml/config.sgml | 9 +-
doc/src/sgml/monitoring.sgml | 12 ++
src/backend/catalog/system_views.sql | 1 +
src/backend/executor/execMain.c | 8 +
src/backend/executor/execParallel.c | 14 +-
src/backend/executor/nodeGather.c | 3 +-
src/backend/executor/nodeGatherMerge.c| 4 +-
src/backend/parser/analyze.c | 5 +
src/backend/postmaster/pgstat.c | 65 +++
src/backend/tcop/postgres.c | 5 +
src/backend/utils/adt/pgstatfuncs.c | 7 +-
src/backend/utils/error/elog.c| 10 +-
src/backend/utils/misc/postgresql.conf.sample | 1 +
src/include/catalog/pg_proc.dat | 6 +-
src/include/executor/execParallel.h | 3 +-
src/include/pgstat.h | 5 +
src/test/regress/expected/rules.out | 9 +-
18 files changed, 267 insertions(+), 79 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 20dc8c605b..2b3aa79cb6 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -112,6 +112,14 @@ static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
#define JUMBLE_SIZE1024 /* query serialization buffer size */
+/*
+ * Utility statements that pgss_ProcessUtility and pgss_post_parse_analyze
+ * ignores.
+ */
+#define PGSS_HANDLED_UTILITY(n) (!IsA(n, ExecuteStmt) && \
+ !IsA(n, PrepareStmt) && \
+ !IsA(n, DeallocateStmt))
+
/*
* Extension version number, for supporting older extension versions' objects
*/
@@ -308,7 +316,8 @@ static void pgss_ProcessUtility(PlannedStmt *pstmt, const char *queryString,
ProcessUtilityContext context, ParamListInfo params,
QueryEnvironment *queryEnv,
DestReceiver *dest, QueryCompletion *qc);
-static uint64 pgss_hash_string(const char *str, int len);
+static const char *pgss_clean_querytext(const char *query, int *location, int *len);
+static uint64 pgss_compute_utility_queryid(const char *query, int query_len);
static void pgss_store(const char *query, uint64 queryId,
int query_location, int query_len,
double total_time, uint64 rows,
@@ -792,16 +801,34 @@ pgss_post_parse_analyze(ParseState *pstate, Query *query)
return;
/*
- * Utility statements get queryId zero. We do this even in cases where
- * the statement contains an optimizable statement for which a queryId
- * could be derived (such as EXPLAIN or DECLARE CURSOR). For such cases,
- * runtime control will first go through ProcessUtility and then the
- * executor, and we don't want the executor hooks to do anything, since we
- * are already measuring the statement's costs at the utility level.
+ * We compute a queryId now so that it can get exported in out
+ * PgBackendStatus. pgss_ProcessUtility will later discard it to prevents
+ * double counting of optimizable statements that are directly contained in
+ * utility statements. Note that we don't compute a queryId for prepared
+ * statemets related utility, as those will inherit from the underlying
+ * statements's one (except DEALLOCATE which is entirely untracked).
*/
if (query->utilityStmt)
{
- query->queryId = UINT64CONST(0);
+ if (pgss_track_utility && PGSS_HANDLED_UTILITY(query->utilityStmt)
+ && pstate->p_sourcetext)
+ {
+ const char *querytext = pstate->p_sourcetext;
+ int query_location = query->stmt_location;
+ int query_len = query->stmt_len;
+
+ /*
+ * Confine our attention to the relevant part of the string, if the
+ * query is a portion of a multi-statement source string.
+ */
+ querytext = pgss_clean_querytext(pstate->p_sourcetext,
+ &query_location,
+ &q
Re: [PATCH] Incremental sort (was: PoC: Partial sort)
On Sat, Mar 14, 2020 at 12:36 PM James Coleman wrote: > > On Sat, Mar 14, 2020 at 12:24 PM James Coleman wrote: > > > > On Sat, Mar 14, 2020 at 12:07 PM James Coleman wrote: > > > > > > On Fri, Mar 13, 2020 at 8:23 PM James Coleman wrote: > > > > > > > > On Friday, March 13, 2020, Tomas Vondra > > > > wrote: > > > >> > > > >> On Fri, Mar 13, 2020 at 04:31:16PM -0400, James Coleman wrote: > > > >>> > > > >>> On Fri, Mar 13, 2020 at 2:23 PM James Coleman > > > >>> wrote: > > > > > > > > > On Tue, Mar 10, 2020 at 10:44 PM Tomas Vondra > > > wrote: > > > > 3) Most of the execution plans look reasonable, except that some > > > > of the > > > > plans look like this: > > > > > > > > > > > > QUERY PLAN > > > >- > > > > Limit > > > > -> GroupAggregate > > > > Group Key: t.a, t.b, t.c, t.d > > > > -> Incremental Sort > > > > Sort Key: t.a, t.b, t.c, t.d > > > > Presorted Key: t.a, t.b, t.c > > > > -> Incremental Sort > > > > Sort Key: t.a, t.b, t.c > > > > Presorted Key: t.a, t.b > > > > -> Index Scan using t_a_b_idx on t > > > >(10 rows) > > > > > > > > i.e. there are two incremental sorts on top of each other, with > > > > different prefixes. But this this is not a new issue - it happens > > > > with > > > > queries like this: > > > > > > > >SELECT a, b, c, d, count(*) FROM ( > > > > SELECT * FROM t ORDER BY a, b, c > > > >) foo GROUP BY a, b, c, d limit 1000; > > > > > > > > i.e. there's a subquery with a subset of pathkeys. Without > > > > incremental > > > > sort the plan looks like this: > > > > > > > > QUERY PLAN > > > >- > > > > Limit > > > > -> GroupAggregate > > > > Group Key: t.a, t.b, t.c, t.d > > > > -> Sort > > > > Sort Key: t.a, t.b, t.c, t.d > > > > -> Sort > > > > Sort Key: t.a, t.b, t.c > > > > -> Seq Scan on t > > > >(8 rows) > > > > > > > > so essentially the same plan shape. What bugs me though is that > > > > there > > > > seems to be some sort of memory leak, so that this query consumes > > > > gigabytes os RAM before it gets killed by OOM. But the memory > > > > seems not > > > > to be allocated in any memory context (at least MemoryContextStats > > > > don't > > > > show anything like that), so I'm not sure what's going on. > > > > > > > > Reproducing it is fairly simple: > > > > > > > >CREATE TABLE t (a bigint, b bigint, c bigint, d bigint); > > > >INSERT INTO t SELECT > > > > 1000*random(), 1000*random(), 1000*random(), 1000*random() > > > >FROM generate_series(1,1000) s(i); > > > >CREATE INDEX idx ON t(a,b); > > > >ANALYZE t; > > > > > > > >EXPLAIN ANALYZE SELECT a, b, c, d, count(*) > > > >FROM (SELECT * FROM t ORDER BY a, b, c) foo GROUP BY a, b, c, d > > > >LIMIT 100; > > > > > > While trying to reproduce this, instead of lots of memory usage, I > > > got > > > the attached assertion failure instead. > > > >>> > > > >>> > > > >>> And, without the EXPLAIN ANALYZE was able to get this one, which will > > > >>> probably be a lot more helpful. > > > >>> > > > >> > > > >> Hmmm, I'll try reproducing it, but can you investigate the values in > > > >> the > > > >> Assert? I mean, it fails on this: > > > >> > > > >> Assert(total_allocated == context->mem_allocated); > > > >> > > > >> so can you get a core or attach to the process using gdb, and see > > > >> what's > > > >> the expected / total value? > > > > > > I've reproduced this on multiple machines (though all are Ubuntu or > > > Debian derivatives...I don't think that's likely to matter). A core > > > dump is ~150MB, so I've uploaded to Dropbox [1]. > > > > > > I didn't find an obvious first-level member of Tuplesortstate that was > > > covered by either of the two blocks in the AllocSet (both are 8KB in > > > size). > > > > > > James > > > > > > [1]: > > > https://www.dropbox.com/s/jwndwp4634hzywk/aset_assertion_failure.core?dl=0 > > > > And...I think I might have found out the issue (though haven't proved > > it 100% yet or fixed it): > > > > The incremental sort node calls `tuplesort_puttupleslot`, which > > switches the memory context to `sortcontext`. It then calls > > `puttuple_common`. `puttuple_common` may then call `grow_memtuples` > > which reallocs space fo
Re: PATCH: add support for IN and @> in functional-dependency statistics use
Hi,
I realized there's one more thing that probably needs discussing.
Essentially, these two clause types are the same:
a IN (1, 2, 3)
(a = 1 OR a = 2 OR a = 3)
but with 8f321bd1 we only recognize the first one as compatible with
functional dependencies. It was always the case that we estimated those
two clauses a bit differently, but the differences were usually small.
But now that we recognize IN as compatible with dependencies, the
difference may be much larger, which bugs me a bit ...
So I wonder if we should recognize the special form of an OR clause,
with all Vars referencing to the same attribute etc. and treat this as
supported by functional dependencies - the attached patch does that.
MCV lists there's already no difference because OR clauses are
supported.
The question is whether we want to do this, and whether we should also
teach the per-column estimates to recognize this special case of IN
clause. That would allow producing exactly the same estimates even with
functional dependencies - recognizing the OR clause as supported gets us
only half-way there, because we still use estimates for each clause (and
those will be slightly different).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/statistics/dependencies.c
b/src/backend/statistics/dependencies.c
index 72dc1cd1bd..5f9b43bf7f 100644
--- a/src/backend/statistics/dependencies.c
+++ b/src/backend/statistics/dependencies.c
@@ -753,24 +753,27 @@ pg_dependencies_send(PG_FUNCTION_ARGS)
static bool
dependency_is_compatible_clause(Node *clause, Index relid, AttrNumber *attnum)
{
- RestrictInfo *rinfo = (RestrictInfo *) clause;
Var*var;
- if (!IsA(rinfo, RestrictInfo))
- return false;
+ if (IsA(clause, RestrictInfo))
+ {
+ RestrictInfo *rinfo = (RestrictInfo *) clause;
- /* Pseudoconstants are not interesting (they couldn't contain a Var) */
- if (rinfo->pseudoconstant)
- return false;
+ /* Pseudoconstants are not interesting (they couldn't contain a
Var) */
+ if (rinfo->pseudoconstant)
+ return false;
- /* Clauses referencing multiple, or no, varnos are incompatible */
- if (bms_membership(rinfo->clause_relids) != BMS_SINGLETON)
- return false;
+ /* Clauses referencing multiple, or no, varnos are incompatible
*/
+ if (bms_membership(rinfo->clause_relids) != BMS_SINGLETON)
+ return false;
- if (is_opclause(rinfo->clause))
+ clause = (Node *) rinfo->clause;
+ }
+
+ if (is_opclause(clause))
{
/* If it's an opclause, check for Var = Const or Const = Var. */
- OpExpr *expr = (OpExpr *) rinfo->clause;
+ OpExpr *expr = (OpExpr *) clause;
/* Only expressions with two arguments are candidates. */
if (list_length(expr->args) != 2)
@@ -801,10 +804,10 @@ dependency_is_compatible_clause(Node *clause, Index
relid, AttrNumber *attnum)
/* OK to proceed with checking "var" */
}
- else if (IsA(rinfo->clause, ScalarArrayOpExpr))
+ else if (IsA(clause, ScalarArrayOpExpr))
{
/* If it's an scalar array operator, check for Var IN Const. */
- ScalarArrayOpExpr *expr = (ScalarArrayOpExpr *)
rinfo->clause;
+ ScalarArrayOpExpr *expr = (ScalarArrayOpExpr *) clause;
/*
* Reject ALL() variant, we only care about ANY/IN.
@@ -839,13 +842,43 @@ dependency_is_compatible_clause(Node *clause, Index
relid, AttrNumber *attnum)
/* OK to proceed with checking "var" */
}
- else if (is_notclause(rinfo->clause))
+ else if (is_orclause(clause))
+ {
+ BoolExpr *expr = (BoolExpr *) clause;
+ ListCell *lc;
+
+ /* start with no attribute number */
+ *attnum = InvalidAttrNumber;
+
+ foreach(lc, expr->args)
+ {
+ AttrNumber clause_attnum;
+
+ /*
+* Had we found incompatible clause in the arguments,
treat the
+* whole clause as incompatible.
+*/
+ if (!dependency_is_compatible_clause((Node *)
lfirst(lc),
+
relid, &clause_attnum))
+ return false;
+
+ if (*attnum == InvalidAttrNumber)
+ *attnum = clause_attnum;
+
+ if (*attnum != clause_attnum)
+ return false;
+
Re: [PATCH] Connection time for \conninfo
Hi There! I forgot about that ... It passed a little time from my new pushed changes. So go ahead :) On Tue, Mar 10, 2020 at 3:15 PM Stephen Frost wrote: > > Greetings, > > * Tom Lane (t...@sss.pgh.pa.us) wrote: > > Stephen Frost writes: > > > Anyway, I don't anticipate having time to do anything with this patch > > > but I disagree that this is a "we don't want it" kind of thing, rather > > > we maybe want it, since someone cared enough to write a patch, but the > > > patch needs work and maybe we want it to look a bit different and be > > > better defined. > > > > I think Peter's primary argument was that this doesn't belong in > > \conninfo, which is about reporting the parameters required to > > establish the connection. We have kind of broken that already by > > cramming SSL and GSS encryption info into the results, but that > > doesn't mean it should become a kitchen-sink listing of anything > > anybody says they'd like to know. > > I could certainly agree with wanting to have a psql command that's "give > me what I need to connect", but that idea and what conninfo actually > returns are pretty distant from each other. For one thing, if I wanted > that from psql, I'd sure hope to get back something that I could > directly use when starting up a new psql session. > > > Anyway, I think your point is that maybe this should be RWF > > not Rejected, and I agree with that. > > Ok. > > > (I had not looked at the last version of the patch, but now that > > I have, I still don't like the fact that it has the client tracking > > session start time separately from what the server does. The small > > discrepancy that introduces is going to confuse somebody. I see > > that there's no documentation update either.) > > This worries me about as much as I worry that someone's going to be > confused by explain-analyze output vs. \timing. Yes, it happens, and > actually pretty often, but I wouldn't change how it works because > they're two different things, not to mention that if I'm going to be > impacted by the time being off on one of the systems, I'd at least like > to know when my client thought it connected relative to the clock on my > client. So if the path it set as RWF could push a extra work in there but in main point what be the address about that. Thanks to all for you feedback. Regards! -- Rodrigo Ramírez Norambuena http://www.rodrigoramirez.com/
Re: [PATCH] Incremental sort (was: PoC: Partial sort)
On Sat, Mar 14, 2020 at 02:41:09PM -0400, James Coleman wrote:
It looks like the issue is actually into the `tuplecontext`, which is
currently a child context of `sortcontext`:
#3 0x558cd153b565 in AllocSetCheck
(context=context@entry=0x558cd28e0b70) at aset.c:1573
1573Assert(total_allocated == context->mem_allocated);
(gdb) p total_allocated
$1 = 16384
(gdb) p context->mem_allocated
$2 = 8192
(gdb) p context->name
$3 = 0x558cd16c8ccd "Caller tuples"
I stuck in several more AllocSetCheck calls in aset.c and got the
attached backtrace.
I think the problem is pretty simple - tuplesort_reset does call
tuplesort_reset, which resets the sortcontext. But that *deletes* the
tuplecontext, so the state->tuplecontext gets stale. I'd haven't looked
into the exact details, but it clearly confuses the accouting.
The attached patch fixes the issue for me - I'm not claiming it's the
right fix, but it's the simplest thing I could think of. Maybe the
tuplesort_rest should work differently, not sure.
And it seems to resolve the memory leak too - I suspect we've freed the
context (so it was not part of the tree of contexts) but the struct was
still valid and we kept allocating memory in it - but it was invisible
to MemoryContextDump etc.
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/sort/tuplesort.c
b/src/backend/utils/sort/tuplesort.c
index c2bd38f39f..91c0189577 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -1419,6 +1419,10 @@ tuplesort_reset(Tuplesortstate *state)
state->slabFreeHead = NULL;
state->growmemtuples = true;
+ state->tuplecontext = AllocSetContextCreate(state->sortcontext,
+
"Caller tuples",
+
ALLOCSET_DEFAULT_SIZES);
+
if (state->memtupsize < INITIAL_MEMTUPSIZE)
{
if (state->memtuples)
Re: Fix comment for max_cached_tuplebufs definition
On Fri, Feb 7, 2020 at 02:49:14PM +0530, Kuntal Ghosh wrote:
> Hello Hackers,
>
> While working on some issue in logical decoding, I found some
> inconsistencies in the comment for defining max_cached_tuplebufs in
> reorderbuffer.c. It only exists till PG10 because after that the
> definition got removed by the generational memory allocator patch. The
> variable is defined as follows in reorderbuffer.c:
> static const Size max_cached_tuplebufs = 4096 * 2; /* ~8MB */
>
> And it gets compared with rb->nr_cached_tuplebufs in
> ReorderBufferReturnTupleBuf as follows:
> if (tuple->alloc_tuple_size == MaxHeapTupleSize &&
> rb->nr_cached_tuplebufs < max_cached_tuplebufs)
>
>{
> rb->nr_cached_tuplebufs++;
> }
>
> So, what this variable actually tracks is 4096 * 2 times
> MaxHeapTupleSize amount of memory which is approximately 64MB. I've
> attached a patch to modify the comment.
>
> But, I'm not sure whether the intention was to keep 8MB cache only. In
> that case, I can come up with another patch.
Yes, I see you are correct, since each tuplebuf is MaxHeapTupleSize.
Patch applied from PG 9.5 to PG 10. Thanks.
--
Bruce Momjian https://momjian.us
EnterpriseDB https://enterprisedb.com
+ As you are, so once was I. As I am, so you will be. +
+ Ancient Roman grave inscription +
Re: control max length of parameter values logged
On Thu, Mar 12, 2020 at 12:01:24PM -0400, Tom Lane wrote: > Alvaro Herrera writes: > > The reason I'm so hot about parameter truncation is that we've seen > > cases where customers' log files contain log lines many megabytes long > > because of gigantic parameters; UUID arrays with tens of thousands of > > entries, and such. Sometimes we see those in the normal "statement" > > line because $customer interpolates into the query literal; normally the > > "solution" is to move the params from interpolated into a parameter. > > But if we log all parameters whole, that workaround no longer works, so > > a way to clip is necessary. > > Agreed, it seems like there's a fairly compelling case for being > able to clip. > > > I'm okay with the default being not to clip anything. > > Also agreed. It's been like it is for a long time with not that > many complaints, so the case for changing the default behavior > seems a bit weak. > > Barring other opinions, I think we have consensus here on what > to do. Alexey, will you update your patch? I am sorry --- I am confused. Why are we truncating or allowing control of truncation of BIND parameter values, but have no such facility for queries. Do we assume queries are shorter than BIND parameters, or is it just that it is easier to trim BIND parameters than values embedded in non-EXECUTE queries. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: control max length of parameter values logged
Bruce Momjian writes: > I am sorry --- I am confused. Why are we truncating or allowing control > of truncation of BIND parameter values, but have no such facility for > queries. Do we assume queries are shorter than BIND parameters, or is > it just that it is easier to trim BIND parameters than values embedded > in non-EXECUTE queries. The cases that Alvaro was worried about were enormous values supplied via bind parameters. We haven't heard comparable complaints about the statement text. Also, from a security standpoint, the contents of the statement text are way more critical than the contents of an out-of-line parameter; you can't do SQL injection from the latter. So I think the audience for trimming would be a lot smaller for statement-text trimming. regards, tom lane
Re: Additional improvements to extended statistics
On Sat, Mar 14, 2020 at 05:56:10PM +0100, Tomas Vondra wrote:
...
Attached is a patch series rebased on top of the current master, after
committing the ScalarArrayOpExpr enhancements. I've updated the OR patch
to get rid of the code duplication, and barring objections I'll get it
committed shortly together with the two parts improving test coverage.
I've pushed the two patches improving test coverage for functional
dependencies and MCV lists, which seems mostly non-controversial. I'll
wait a bit more with the two patches actually changing behavior (rebased
version attached, to keep cputube happy).
regards
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From ed72b6da747673a8c848a4bb8676e0d5debf2c26 Mon Sep 17 00:00:00 2001
From: Tomas Vondra
Date: Sun, 8 Mar 2020 23:26:50 +0100
Subject: [PATCH 1/2] Improve estimation of OR clauses with extended statistics
Until now, OR clauses were estimated using extended statistics only when
the whole clause (all the arguments) are compatible. If even just one
argument was found to be incompatible, the whole clause was estimated
ignoring extended statistics.
This was considered acceptable, because the estimation errors are not
that bad for OR clauses. It may however be an issue when the OR clause
is more complex, e.g. when some of the arguments are AND clauses etc.
This relaxes this restriction, using similar logic as for AND clauses.
We first estimate as much as possible using extended statistics, and
then use the existing (s1 + s2 - s1 * s2) formula for the rest.
Discussion:
https://www.postgresql.org/message-id/flat/13902317.Eha0YfKkKy%40pierred-pdoc
Author: Tomas Vondra
Reviewed-by: Dean Rasheed
---
src/backend/optimizer/path/clausesel.c| 141 +++---
src/backend/statistics/extended_stats.c | 36 +++--
src/backend/statistics/mcv.c | 5 +-
src/include/optimizer/optimizer.h | 7 +
.../statistics/extended_stats_internal.h | 3 +-
src/include/statistics/statistics.h | 3 +-
src/test/regress/expected/stats_ext.out | 61 +++-
src/test/regress/sql/stats_ext.sql| 13 +-
8 files changed, 227 insertions(+), 42 deletions(-)
diff --git a/src/backend/optimizer/path/clausesel.c
b/src/backend/optimizer/path/clausesel.c
index a3ebe10592..2ae2e0cc70 100644
--- a/src/backend/optimizer/path/clausesel.c
+++ b/src/backend/optimizer/path/clausesel.c
@@ -92,7 +92,7 @@ clauselist_selectivity(PlannerInfo *root,
*/
s1 *= statext_clauselist_selectivity(root, clauses, varRelid,
jointype, sjinfo, rel,
-
&estimatedclauses);
+
&estimatedclauses, false);
}
/*
@@ -104,6 +104,62 @@ clauselist_selectivity(PlannerInfo *root,
estimatedclauses);
}
+/*
+ * clauselist_selectivity_or -
+ * Compute the selectivity of an implicitly-ORed list of boolean
+ * expression clauses. The list can be empty, in which case 0.0
+ * must be returned. List elements may be either RestrictInfos
+ * or bare expression clauses --- the former is preferred since
+ * it allows caching of results.
+ *
+ * See clause_selectivity() for the meaning of the additional parameters.
+ *
+ * The basic approach is to apply extended statistics first, on as many
+ * clauses as possible, in order to capture cross-column dependencies etc.
+ * The remaining clauses are then estimated using regular statistics tracked
+ * for individual columns. This is done by simply passing the clauses to
+ * clauselist_selectivity and then combining the selectivities using the
+ * regular formula (s1+s2 - s1*s2).
+ */
+static Selectivity
+clauselist_selectivity_or(PlannerInfo *root,
+ List *clauses,
+ int varRelid,
+ JoinType jointype,
+ SpecialJoinInfo *sjinfo)
+{
+ Selectivity s = 0.0;
+ RelOptInfo *rel;
+ Bitmapset *estimatedclauses = NULL;
+
+ /*
+* Determine if these clauses reference a single relation. If so, and
if
+* it has extended statistics, try to apply those.
+*/
+ rel = find_single_rel_for_clauses(root, clauses);
+ if (rel && rel->rtekind == RTE_RELATION && rel->statlist != NIL)
+ {
+ /*
+* Estimate as many clauses as possible using extended
statistics.
+*
+* 'estimatedclauses' tracks the
Re: [PATCH] Skip llvm bytecode generation if LLVM is missing
On Fri, 13 Mar 2020 at 15:04, Andres Freund wrote: > On 2020-03-13 14:08:12 +0800, Craig Ringer wrote: > > The alternative would be to detect a missing clang and emit a much more > > informative error than the current one that explicitly suggests retrying > > with > > > > make with_llvm=no > > > > or setting with_llvm=no in the environment. > > That, that, that's what I suggested upthread? > Yes, and I still don't like it. "with_llvm" is the actual already-working option. I'd rather make this not randomly explode for users, but failing that we can just hack the Makefile in the rpms for EL-7 (where it's a particular mess) and rely on an error message for other cases. -- Craig Ringer http://www.2ndQuadrant.com/ 2ndQuadrant - PostgreSQL Solutions for the Enterprise
Re: Additional improvements to extended statistics
On Sun, Mar 15, 2020 at 1:08 PM Tomas Vondra wrote: > On Sat, Mar 14, 2020 at 05:56:10PM +0100, Tomas Vondra wrote: > >Attached is a patch series rebased on top of the current master, after > >committing the ScalarArrayOpExpr enhancements. I've updated the OR patch > >to get rid of the code duplication, and barring objections I'll get it > >committed shortly together with the two parts improving test coverage. > > > > I've pushed the two patches improving test coverage for functional > dependencies and MCV lists, which seems mostly non-controversial. I'll > wait a bit more with the two patches actually changing behavior (rebased > version attached, to keep cputube happy). Some comment fixes: - /* Check if the expression the right shape (one Var, one Const) */ - if (!examine_clause_args(expr->args, &var, NULL, NULL)) + /* +* Check if the expression the right shape (one Var and one Const, +* or two Vars). +*/ Check if the expression "has" or "is of" the right shape. - * Attempts to match the arguments to either (Var op Const) or (Const op Var), - * possibly with a RelabelType on top. When the expression matches this form, - * returns true, otherwise returns false. + * Attempts to match the arguments to either (Var op Const) or (Const op Var) + * or (Var op Var), possibly with a RelabelType on top. When the expression + * matches this form, returns true, otherwise returns false. ... match the arguments to (Var op Const), (Const op Var) or (Var op Var), ... + /* +* Both variables have to be for the same relation (otherwise it's +* a join clause, and we don't deal with those yet. +*/ Missing close parenthesis. Stimulated by some bad plans involving JSON, I found my way to your WIP stats-on-expressions patch in this thread. Do I understand correctly that it will eventually also support single expressions, like CREATE STATISTICS t_distinct_abc (ndistinct) ON (my_jsonb_column->>'abc') FROM t? It looks like that would solve problems that otherwise require a generated column or an expression index just to get ndistinct.
Re: Additional improvements to extended statistics
On Sun, Mar 15, 2020 at 02:48:02PM +1300, Thomas Munro wrote: On Sun, Mar 15, 2020 at 1:08 PM Tomas Vondra wrote: On Sat, Mar 14, 2020 at 05:56:10PM +0100, Tomas Vondra wrote: >Attached is a patch series rebased on top of the current master, after >committing the ScalarArrayOpExpr enhancements. I've updated the OR patch >to get rid of the code duplication, and barring objections I'll get it >committed shortly together with the two parts improving test coverage. > I've pushed the two patches improving test coverage for functional dependencies and MCV lists, which seems mostly non-controversial. I'll wait a bit more with the two patches actually changing behavior (rebased version attached, to keep cputube happy). Some comment fixes: - /* Check if the expression the right shape (one Var, one Const) */ - if (!examine_clause_args(expr->args, &var, NULL, NULL)) + /* +* Check if the expression the right shape (one Var and one Const, +* or two Vars). +*/ Check if the expression "has" or "is of" the right shape. - * Attempts to match the arguments to either (Var op Const) or (Const op Var), - * possibly with a RelabelType on top. When the expression matches this form, - * returns true, otherwise returns false. + * Attempts to match the arguments to either (Var op Const) or (Const op Var) + * or (Var op Var), possibly with a RelabelType on top. When the expression + * matches this form, returns true, otherwise returns false. ... match the arguments to (Var op Const), (Const op Var) or (Var op Var), ... + /* +* Both variables have to be for the same relation (otherwise it's +* a join clause, and we don't deal with those yet. +*/ Missing close parenthesis. Thanks, I'll get this fixed. Stimulated by some bad plans involving JSON, I found my way to your WIP stats-on-expressions patch in this thread. Do I understand correctly that it will eventually also support single expressions, like CREATE STATISTICS t_distinct_abc (ndistinct) ON (my_jsonb_column->>'abc') FROM t? It looks like that would solve problems that otherwise require a generated column or an expression index just to get ndistinct. Yes, I think that's generally the plan. I was also thinking about inventing some sort of special JSON statistics (e.g. extracting paths from the JSONB and computing frequencies, or something like that). But stats on expressions are one of the things I'd like to do in PG14. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] Incremental sort (was: PoC: Partial sort)
On Fri, Mar 13, 2020 at 1:06 PM James Coleman wrote: > > On Thu, Mar 12, 2020 at 5:53 PM Alvaro Herrera > wrote: > > > > I gave this a very quick look; I don't claim to understand it or > > anything, but I thought these trivial cleanups worthwhile. The only > > non-cosmetic thing is changing order of arguments to the SOn_printf() > > calls in 0008; I think they are contrary to what the comment says. > > Yes, I think you're correct (re: 0008). > > They all look generally good to me, and are included in the attached > patch series. I just realized something about this (unsure if in Alvaro's or in my applying that) broke make check pretty decently (3 test files broken, also much slower, and the incremental sort test returns a lot of obviously broken results). I'll take a look tomorrow and hopefully get a fix (probably will reply to the more recent subthread's though). James
Re: truncating timestamps on arbitrary intervals
On Fri, Mar 13, 2020 at 7:48 PM Isaac Morland wrote:
>
> On Fri, 13 Mar 2020 at 03:13, John Naylor wrote:
>
>> - align weeks to start on Sunday
>> select date_trunc_interval('7 days'::interval, TIMESTAMP '2020-02-11
>> 01:01:01.0', TIMESTAMP '1900-01-02');
>> date_trunc_interval
>> -
>> 2020-02-09 00:00:00
>> (1 row)
>
>
> I'm confused by this. If my calendars are correct, both 1900-01-02 and
> 2020-02-11 are Tuesdays. So if the date being adjusted and the origin are
> both Tuesday, shouldn't the day part be left alone when truncating to 7 days?
Thanks for taking a look! The non-intuitive behavior you found is
because the patch shifts the timestamp before converting to the
internal pg_tm type. The pg_tm type stores day of the month, which is
used for the calculation. It's not counting the days since the origin.
Then the result is shifted back.
To get more logical behavior, perhaps the optional parameter is better
as an offset instead of an origin. Alternatively (or additionally),
the function could do the math on int64 timestamps directly.
> Also, I'd like to confirm that the default starting point for 7 day periods
> (weeks) is Monday, per ISO.
That's currently the behavior in the existing date_trunc function,
when passed the string 'week'. Given that keyword, it calculates the
week of the year.
When using the proposed function with arbitrary intervals, it uses day
of the month, as found in the pg_tm struct. It doesn't treat 7 days
differently then 5 or 10 without user input (origin or offset), since
there is nothing special about 7 day intervals as such internally. To
show the difference between date_trunc, and date_trunc_interval as
implemented in v5 with no origin:
select date_trunc('week', d), count(*) from generate_series(
'2020-02-01'::timestamp, '2020-03-31', '1 day') d group by 1 order by
1;
date_trunc | count
-+---
2020-01-27 00:00:00 | 2
2020-02-03 00:00:00 | 7
2020-02-10 00:00:00 | 7
2020-02-17 00:00:00 | 7
2020-02-24 00:00:00 | 7
2020-03-02 00:00:00 | 7
2020-03-09 00:00:00 | 7
2020-03-16 00:00:00 | 7
2020-03-23 00:00:00 | 7
2020-03-30 00:00:00 | 2
(10 rows)
select date_trunc_interval('7 days'::interval, d), count(*) from
generate_series( '2020-02-01'::timestamp, '2020-03-31', '1 day') d
group by 1 order by 1;
date_trunc_interval | count
-+---
2020-02-01 00:00:00 | 7
2020-02-08 00:00:00 | 7
2020-02-15 00:00:00 | 7
2020-02-22 00:00:00 | 7
2020-02-29 00:00:00 | 1
2020-03-01 00:00:00 | 7
2020-03-08 00:00:00 | 7
2020-03-15 00:00:00 | 7
2020-03-22 00:00:00 | 7
2020-03-29 00:00:00 | 3
(10 rows)
Resetting the day every month is counterintuitive if not broken, and
as I mentioned it might make more sense to use the int64 timestamp
directly, at least for intervals less than one month. I'll go look
into doing that.
--
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
