Re: Expose JIT counters/timing in pg_stat_statements
> On Fri, Feb 25, 2022 at 04:19:27PM +0100, Magnus Hagander wrote: > On Fri, Feb 25, 2022 at 2:33 PM Julien Rouhaud wrote: > > > > Hi, > > > > On Fri, Feb 25, 2022 at 02:06:29PM +0100, Magnus Hagander wrote: > > > Here's a patch to add the sum of timings for JIT counters to > > > pg_stat_statements, as a way to follow-up on if JIT is doing a good or > > > a bad job in a configuration. > > > > +1, it seems like something quite useful. > > Given the amount of time often spent debugging JIT -- getting more > insight is going to make it easier to tune it instead of like what > unfortunately many people do and just turn it off.. Indeed, sounds convenient, although I wonder how exactly one would use it to tune JIT? I'm curious, because I got used to situations when one single long query takes much longer than expected due to JIT issues -- but it seems the target of this patch are situations when there are a lot of long queries using JIT, and it's easier to analyze them via pgss? > > > I decided to only store the total time for the timings, since there > > > are 4 different timings and storing max/min/etc for each one of them > > > would lead to a bit too much data. This can of course be reconsidered, > > > but I think that's a reasonable tradeoff. > > > > I think the cumulated total time is enough. Looking at the patch, I think > > we > > should also cumulate the number of time jit was triggered, and > > probably the same for each other main operation (optimization and inlining). > > Otherwise the values may be wrong and look artificially low. > > So just to be clear, you're basically thinking: > > jit_count = count of entries where jit_functions>0 > jit_functions = > jit_optimizatinos = count of entries where time spent on jit_optimizations > 0 One interesting not-very-relevant for the patch thing I've noticed while reading it, is that there seems to be no way to find out what fraction of time jit_tuple_deforming is taking alone, it's sort of merged together with jit_expressions in generation_counter.
Re: Commitfest 2022-03 Patch Triage Part 1a.i
> On Tue, Mar 01, 2022 at 11:16:36AM -0500, Greg Stark wrote: > Last November Daniel Gustafsson did a patch triage. It took him three > emails to get through the patches in the commitfest back then. Since > then we've had the November and the January commitfests so I was > interested to see how many of these patches had advanced > > I'm only part way through the first email but so far only two patches > have changed status -- and both to "Returned with feedback" :( > > So I'm going to post updates but I'm going to break it up into smaller > batches because otherwise it'll take me a month before I post > anything. Thanks for being proactive! > > 1741: Index Skip Scan > > = > > An often requested feature which has proven hard to reach consensus on an > > implementation for. The thread(s) have stalled since May, is there any > > hope of > > taking this further? Where do we go from here with this patch? > > "Often requested indeed! I would love to be able to stop explaining to > people that Postgres can't handle this case well. > > It seems there are multiple patch variants around and suggestions from > Heikki and Peter G about fundamental interface choices. It would be > nice to have a good summary from someone who is involved about what's > actually left unresolved. I'm going to leave a summary for this one here, if you don't mind. I believe the design commentary from Heikki about using index_rescan was more or less answered by Thomas, and having no follow up on that I'm assuming it was convincing enough. Peter G most recent suggestion about MDAM approach was interesting, but very general, not sure what to make of it in absence of any feedback on follow-up questions/proposed experimental changes. On top of that a correlated patch [1] that supposed to get some improvements for this feature on the planner side didn't get much feedback either. The idea is that the feature could be done in much better way, but the alternative proposal is still not there and I think doesn't even have a CF item. The current state of things is that I've managed to prepare much smaller and less invasive standalone version of the patch for review, leaving most questionable parts aside as optional. Overall it seems that the common agreement about the patch is "the design could be better", but no one have yet articulated in which way, or formulated what are the current issues. Having being through 19 CF the common ground for folks, who were involved into it, is that with no further feedback the CF item could be closed. Sad but true :( [1]: https://commitfest.postgresql.org/37/2433/
Re: Expose JIT counters/timing in pg_stat_statements
> On Mon, Mar 07, 2022 at 01:27:02PM +0100, Magnus Hagander wrote: > On Fri, Feb 25, 2022 at 5:40 PM Dmitry Dolgov <9erthali...@gmail.com> wrote: > > One interesting not-very-relevant for the patch thing I've noticed while > > reading it, is that there seems to be no way to find out what fraction > > of time jit_tuple_deforming is taking alone, it's sort of merged > > together with jit_expressions in generation_counter. > > That's missing att a deeper level though, right? We don't have it in > EXPLAIN ANALYZE either. So while I agree that's useful, I think that's > the job of another patch, and these two sets of counters should be the > same. Right, it's missing on the instrumentation level, I was just surprised to notice that.
Re: pg_stat_statements and "IN" conditions
> On Wed, Jan 05, 2022 at 10:11:11PM +0100, Dmitry Dolgov wrote: > > On Tue, Jan 04, 2022 at 06:02:43PM -0500, Tom Lane wrote: > > We can debate whether the rules proposed here are good for > > pg_stat_statements or not, but it seems inevitable that they will be a > > disaster for some other consumers of the query hash. > > Hm, which consumers do you mean here, potential extension? Isn't the > ability to use an external module to compute queryid make this situation > possible anyway? > > > do you really think that a query with two int > > parameters is equivalent to one with five float parameters for all > > query-identifying purposes? > > Nope, and it will be hard to figure this out no matter which approach > we're talking about, because it mostly depends on the context and type > of queries I guess. Instead, such functionality should allow some > reasonable configuration. To be clear, the use case I have in mind here > is not four or five, but rather a couple of hundreds constants where > chances that the whole construction was generated automatically by ORM > is higher than normal. > > > I can see the merits of allowing different numbers of IN elements > > to be considered equivalent for pg_stat_statements, but this patch > > seems to go far beyond that basic idea, and I fear the side-effects > > will be very bad. > > Not sure why it goes far beyond, but then there were two approaches > under consideration, as I've stated in the first message. I already > don't remember all the details, but another one was evolving around > doing similar things in a more limited fashion in transformAExprIn. The > problem would be then to carry the information, necessary to represent > the act of "merging" some number of queryids together. Any thoughts > here? > > The idea of keeping the original queryid untouched and add another type > of id instead sounds interesting, but it will add too much overhead for > a quite small use case I guess. ``` Thu, 10 Mar 2022 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.
Re: pg_stat_statements and "IN" conditions
> On Thu, Mar 10, 2022 at 12:32:08PM -0500, Robert Haas wrote: > On Thu, Mar 10, 2022 at 12:12 PM Tom Lane wrote: > > > 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. > > I think there are two separate points here, one about patch quality > and the other about whether the basic idea is good. I think the basic > idea is good. I do not contend that collapsing IN-lists of arbitrary > length is what everyone wants in all cases, but it seems entirely > reasonable to me to think that it is what some people want. So I would > say just make it a parameter and let people configure whichever > behavior they want. My bet is 95% of users would prefer to have it on, > but even if that's wildly wrong, having it as an optional behavior > hurts nobody. Let it be off by default and let those who want it flip > the toggle. On the code quality issue, I haven't read the patch but > your concerns sound well-founded to me from reading what you wrote. I have the same understanding, there is a toggle in the patch exactly for this purpose. To give a bit more context, the whole development was ORM-driven rather than pulled out of thin air -- people were complaining about huge generated queries that could be barely displayed in monitoring, I was trying to address it via collapsing the list where it was happening. In other words "I'm-not-even-sure-quite-what" part may be indeed too extensive, but was triggered by real world issues. Of course, I could get the implementation not quite right, e.g. I wasn't aware about dangers of using eval_const_expressions. But that's what the CF item and the corresponding discussion is for, I guess. Let me see what I could do to improve it.
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_statem
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 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 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: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: MDAM techniques and Index Skip Scan patch
mate like this */ @@ -1196,6 +1199,8 @@ typedef struct 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
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
> 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: Index Skip Scan (new UniqueKeys)
> On Mon, Nov 30, 2020 at 04:42:20PM +0200, Heikki Linnakangas wrote:
>
> I had a quick look at this patch. I haven't been following this thread, so
> sorry if I'm repeating old arguments, but here we go:
Thanks!
> - I'm surprised you need a new index AM function (amskip) for this. Can't
> you just restart the scan with index_rescan()? The btree AM can check if the
> new keys are on the same page, and optimize the rescan accordingly, like
> amskip does. That would speed up e.g. nested loop scans too, where the keys
> just happen to be clustered.
An interesting point. At the moment I'm not sure whether it's possible
to implement skipping via index_rescan or not, need to take a look. But
checking if the new keys are on the same page would introduce some
overhead I guess, wouldn't it be too invasive to add it into already
existing btree AM?
> - Does this optimization apply to bitmap index scans?
No, from what I understand it doesn't.
> - This logic in build_index_paths() is not correct:
>
> > + /*
> > +* Skip scan is not supported when there are qual conditions,
> > which are not
> > +* covered by index. The reason for that is that those
> > conditions are
> > +* evaluated later, already after skipping was applied.
> > +*
> > +* TODO: This implementation is too restrictive, and doesn't
> > allow e.g.
> > +* index expressions. For that we need to examine index_clauses
> > too.
> > +*/
> > + if (root->parse->jointree != NULL)
> > + {
> > + ListCell *lc;
> > +
> > + foreach(lc, (List *)root->parse->jointree->quals)
> > + {
> > + Node *expr, *qual = (Node *) lfirst(lc);
> > + Var *var;
> > + bool found = false;
> > +
> > + if (!is_opclause(qual))
> > + {
> > + not_empty_qual = true;
> > + break;
> > + }
> > +
> > + expr = get_leftop(qual);
> > +
> > + if (!IsA(expr, Var))
> > + {
> > + not_empty_qual = true;
> > + break;
> > + }
> > +
> > + var = (Var *) expr;
> > +
> > + for (int i = 0; i < index->ncolumns; i++)
> > + {
> > + if (index->indexkeys[i] ==
> > var->varattno)
> > + {
> > + found = true;
> > + break;
> > + }
> > + }
> > +
> > + if (!found)
> > + {
> > + not_empty_qual = true;
> > + break;
> > + }
> > + }
> > + }
>
> If you care whether the qual is evaluated by the index AM or not, you need
> to also check that the operator is indexable. Attached is a query that
> demonstrates that problem.
> ...
> Also, you should probably check that the index quals are in the operator
> family as that used for the DISTINCT.
Yes, good point, will change this in the next version.
> I'm actually a bit confused why we need this condition. The IndexScan
> executor node should call amskip() only after checking the additional quals,
> no?
This part I don't quite get, what exactly you mean by checking the
additional quals in the executor node? But at the end of the day this
condition was implemented exactly to address the described issue, which
was found later and added to the tests.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Mon, Nov 30, 2020 at 02:26:19PM +0100, Dmitry Dolgov wrote: > > On Mon, Nov 30, 2020 at 04:12:29PM +0300, Alexander Korotkov wrote: > > > > > > My first question is whether we're > > > > able to handle different subscript types differently. For instance, > > > > one day we could handle jsonpath subscripts for jsonb. And for sure, > > > > jsonpath subscripts are expected to be handled differently from text > > > > subscripts. I see we can distinguish types during in prepare and > > > > validate functions. But it seems there is no type information in > > > > fetch and assign functions. Should we add something like this to the > > > > SubscriptingRefState for future usage? > > > > > > > > Datum uppertypeoid[MAX_SUBSCRIPT_DEPTH]; > > > > Datum lowertypeoid[MAX_SUBSCRIPT_DEPTH]; > > > > > > Yes, makes sense. My original idea was that it could be done within the > > > jsonpath support patch itself, but at the same time providing these > > > fields into SubscriptingRefState will help other potential extensions. > > > > > > Having said that, maybe it would be even better to introduce a field > > > with an opaque structure for both SubscriptingRefState and > > > SubscriptingRef, where every implementation of custom subscripting can > > > store any necessary information? In case of jsonpath it could keep type > > > information acquired in prepare function, which would be then passed via > > > SubscriptingRefState down to the fetch/assign. > > > > The idea of an opaque field in SubscriptingRef structure is more > > attractive to me. Could you please implement it? > > Sure, doesn't seem to be that much work. The attached implementation should be enough I guess, as fetch/assign are executed in a child memory context of one where prepare does the stuff. >From 6f7d9589cc827dfb3b1d82a3e0e629b482e4c77a Mon Sep 17 00:00:00 2001 From: erthalion <9erthali...@gmail.com> Date: Thu, 31 Jan 2019 22:37:19 +0100 Subject: [PATCH v35 1/5] Base implementation of subscripting mechanism Introduce all the required machinery for generalizing subscripting operation for a different data types: * subscripting handler procedure, to set up a relation between data type and corresponding subscripting logic. * subscripting routines, that help generalize a subscripting logic, since it involves few stages, namely preparation (e.g. to figure out required types), validation (to check the input and return meaningful error message), fetch (executed when we extract a value using subscripting), assign (executed when we update a data type with a new value using subscripting). Without this it would be neccessary to introduce more new fields to pg_type, which would be too invasive. All ArrayRef related logic was removed and landed as a separate subscripting implementation in the following patch from this series. The rest of the code was rearranged, to e.g. store a type of assigned value for an assign operation. Reviewed-by: Tom Lane, Arthur Zakirov --- .../pg_stat_statements/pg_stat_statements.c | 1 + src/backend/catalog/heap.c| 6 +- src/backend/catalog/pg_type.c | 7 +- src/backend/commands/typecmds.c | 77 +- src/backend/executor/execExpr.c | 26 +--- src/backend/executor/execExprInterp.c | 124 +++ src/backend/nodes/copyfuncs.c | 2 + src/backend/nodes/equalfuncs.c| 2 + src/backend/nodes/outfuncs.c | 2 + src/backend/nodes/readfuncs.c | 2 + src/backend/parser/parse_expr.c | 54 --- src/backend/parser/parse_node.c | 141 -- src/backend/parser/parse_target.c | 88 +-- src/backend/utils/adt/ruleutils.c | 21 +-- src/backend/utils/cache/lsyscache.c | 23 +++ src/include/c.h | 2 + src/include/catalog/pg_type.h | 9 +- src/include/executor/execExpr.h | 15 +- src/include/nodes/primnodes.h | 8 + src/include/nodes/subscripting.h | 42 ++ src/include/parser/parse_node.h | 6 +- src/include/utils/lsyscache.h | 1 + 22 files changed, 333 insertions(+), 326 deletions(-) create mode 100644 src/include/nodes/subscripting.h diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c index 1eac9edaee..31ba120fb2 100644 --- a/contrib/pg_stat_statements/pg_stat_statements.c +++ b/contrib/pg_stat_statements/pg_stat_statements.c @@ -2800,6 +2800,7 @@ JumbleExpr(pgssJumbleState *jstate, Node *node) JumbleExpr(jsta
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Dec 02, 2020 at 12:58:51PM -0500, Tom Lane wrote: > So ... one of the things that's been worrying me about this patch > from day one is whether it would create a noticeable performance > penalty for existing use-cases. I did a small amount of experimentation > about that with the v35 patchset, and it didn't take long at all to > find that this: > > --- cut --- > create or replace function arraytest(n int) returns void as > $$ > declare > a int[]; > begin > a := array[1, 1]; > for i in 3..n loop > a[i] := a[i-1] - a[i-2]; > end loop; > end; > $$ > language plpgsql stable; > > \timing on > > select arraytest(1000); > --- cut --- > > is about 15% slower with the patch than with HEAD. I'm not sure > what an acceptable penalty might be, but 15% is certainly not it. > > I'm also not quite sure where the cost is going. It looks like > 0001+0002 aren't doing much to the executor except introducing > one level of subroutine call, which doesn't seem like it'd account > for that. I've tried to reproduce that, but get ~2-4% slowdown (with a pinned backend, no turbo etc). Are there any special steps I've probably missed? At the same time, I remember had conducted this sort of tests before when you and others raised the performance degradation question and the main part of the patch was already more or less stable. From what I remember the numbers back then were also rather small.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Dec 02, 2020 at 11:52:54AM -0500, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: > >> On Mon, Nov 30, 2020 at 02:26:19PM +0100, Dmitry Dolgov wrote: > >>> On Mon, Nov 30, 2020 at 04:12:29PM +0300, Alexander Korotkov wrote: > >>> The idea of an opaque field in SubscriptingRef structure is more > >>> attractive to me. Could you please implement it? > > >> Sure, doesn't seem to be that much work. > > I just happened to notice this bit. This idea is a complete nonstarter. > You cannot have an "opaque" field in a parsetree node, because then the > backend/nodes code has no idea what to do with it for > copy/compare/outfuncs/readfuncs. The patch seems to be of the opinion > that "do nothing" is adequate, which it completely isn't. > > Perhaps this is a good juncture at which to remind people that parse > tree nodes are read-only so far as the executor is concerned, so > storing something there only at execution time won't work either. Oh, right, stupid of me. Then I'll just stick with the original Alexanders suggestion.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Dec 02, 2020 at 01:20:10PM -0600, Justin Pryzby wrote: > On Wed, Dec 02, 2020 at 08:18:08PM +0100, Dmitry Dolgov wrote: > > > On Wed, Dec 02, 2020 at 12:58:51PM -0500, Tom Lane wrote: > > > So ... one of the things that's been worrying me about this patch > > > from day one is whether it would create a noticeable performance > > > penalty for existing use-cases. I did a small amount of experimentation > > > about that with the v35 patchset, and it didn't take long at all to > > > find that this: > > > --- cut --- > > > > > > is about 15% slower with the patch than with HEAD. I'm not sure > > > what an acceptable penalty might be, but 15% is certainly not it. > > > > > > I'm also not quite sure where the cost is going. It looks like > > > 0001+0002 aren't doing much to the executor except introducing > > > one level of subroutine call, which doesn't seem like it'd account > > > for that. > > > > I've tried to reproduce that, but get ~2-4% slowdown (with a pinned > > backend, no turbo etc). Are there any special steps I've probably > > missed? At the same time, I remember had conducted this sort of tests > > before when you and others raised the performance degradation question > > and the main part of the patch was already more or less stable. From > > what I remember the numbers back then were also rather small. > > Are you comparing with casserts (and therefor MEMORY_CONTEXT_CHECKING) > disabled? Yep, they're disabled.
Re: {CREATE INDEX, REINDEX} CONCURRENTLY improvements
> On Mon, Nov 30, 2020 at 04:54:39PM -0300, Alvaro Herrera wrote: > > In a previous thread [1], we added smarts so that processes running > CREATE INDEX CONCURRENTLY would not wait for each other. > > One is adding the same to REINDEX CONCURRENTLY. I've attached patch > 0002 here which does that. > > Why 0002, you ask? That's because preparatory patch 0001 simplifies the > ReindexRelationConcurrently somewhat by adding a struct to be used of > indexes that are going to be processed, instead of just a list of Oids. > This is a good change in itself because it let us get rid of duplicative > open/close of the index rels in order to obtain some info that's already > known at the start. Thanks! The patch looks pretty good to me, after reading it I only have a few minor comments/questions: * ReindexIndexInfo sounds a bit weird for me because of the repeating part, although I see there is already a similar ReindexIndexCallbackState so probably it's fine. * This one is mostly for me to understand. There are couple of places with a commentary that 'PROC_IN_SAFE_IC is not necessary, because the transaction only takes a snapshot to do some catalog manipulation'. But for some of them I don't immediately see in the relevant code anything related to snapshots. E.g. one in DefineIndex is followed by WaitForOlderSnapshots (which seems to only do waiting, not taking a snapshot), index_set_state_flags and CacheInvalidateRelcacheByRelid. Is taking a snapshot hidden somewhere there inside?
Re: Index Skip Scan (new UniqueKeys)
> On Tue, Dec 01, 2020 at 10:59:22PM +0200, Heikki Linnakangas wrote: > > > > - Does this optimization apply to bitmap index scans? > > > > No, from what I understand it doesn't. > > Would it be hard to add? Don't need to solve everything in the first > version of this, but I think in principle you could do the same > optimization for bitmap index scans, so if the current API can't do it, > that's maybe an indication that the API isn't quite right. I would expect it should not be hard as at the moment all parts seems relatively generic. But of course I need to check, while it seems no one had bitmap index scans in mind while developing this patch. > > > I'm actually a bit confused why we need this condition. The IndexScan > > > executor node should call amskip() only after checking the additional > > > quals, > > > no? > > > > This part I don't quite get, what exactly you mean by checking the > > additional quals in the executor node? But at the end of the day this > > condition was implemented exactly to address the described issue, which > > was found later and added to the tests. > > As I understand this, the executor logic goes like this: > > query: SELECT DISTINCT ON (a, b) a, b FROM foo where c like '%y%' and a > like 'a%' and b = 'b'; > > 1. Call index_beginscan, keys: a >= 'a', b = 'b' > > 2. Call index_getnext, which returns first row to the Index Scan node > > 3. Evaluates the qual "c like '%y%'" on the tuple. If it's false, goto step > 2 to get next tuple. > > 4. Return tuple to parent node > > 5. index_amskip(), to the next tuple with a > 'a'. Goto 2. > > The logic should work fine, even if there are quals that are not indexable, > like "c like '%y'" in the above example. So why doesn't it work? What am I > missing? To remind myself how it works I went through this sequence, and from what I understand the qual "c like '%y%'" is evaluated in this case in ExecQual, not after index_getnext_tid (and values returned after skipping are reported as filtered out). So when it comes to index_skip only quals on a & b were evaluated. Or did you mean something else? Another small detail is that in the current implementation there is no goto 2 in the last step. Originally it was like that, but since skipping return an exact position that we need there was something like "find a value, then do one step back so that index_getnext will find it". Unfortunately this stepping back part turns out to be a source of troubles, and getting rid of it even allowed to make code somewhat more concise. But of course I'm open for suggestions about improvements.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Dec 09, 2020 at 12:49:48PM -0500, Tom Lane wrote: > > I've pushed the core patch now. Thanks a lot! > The jsonb parts now have to be > rebased onto this design, which I'm assuming Dmitry will tackle Yes, I'm already on it, just couldn't keep up with the changes in this thread. > BTW, while reviewing the thread to write the commit message, > I was reminded of my concerns around the "is it a container" > business. As things stand, if type A has a typelem link to > type B, then the system supposes that A contains B physically; > this has implications for what's allowed in DDL, for example > (cf find_composite_type_dependencies() and other places). > We now have a feature whereby subscripting can yield a type > that is not contained in the source type in that sense. > I'd be happier if the "container" terminology were reserved for > that sort of physical containment, which means that I think a lot > of the commentary around SubscriptingRef is misleading. But I do > not have a better word to suggest offhand. Thoughts? I have only 'a composite'/'a compound' alternative in mind, but it's probably the same confusing as a container.
Re: pg_stat_statements and "IN" conditions
> On Wed, Dec 09, 2020 at 03:37:40AM +, Chengxi Sun wrote: > > The following review has been posted through the commitfest application: > make installcheck-world: tested, passed > Implements feature: tested, passed > Spec compliant: not tested > Documentation:not tested > > Hi, I did some test and it works well Thanks for testing!
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Dec 09, 2020 at 04:59:34PM -0500, Tom Lane wrote: > > 0001 adds the ability to attach a subscript handler to an existing > data type with ALTER TYPE. This is clearly going to be necessary > if we want extension types to be able to use this facility. The > only thing that I think might be controversial here is that I did > not add the ability to set pg_type.typelem. While that'd be easy > enough so far as ALTER TYPE is concerned, I'm not sure that we want > to encourage people to change it. The dependency rules mean that > the semantics of typelem aren't something you really want to change > after-the-fact on an existing type. Also, if we did allow it, any > existing SubscriptingRef.refelemtype values in stored views would > fail to be updated. I'm curious what could be the use case for setting pg_type.typelem for subscripting? I don't see this that much controversial, but maybe I'm missing something. > On Thu, Dec 10, 2020 at 05:37:20AM +0100, Pavel Stehule wrote: > st 9. 12. 2020 v 22:59 odesílatel Tom Lane napsal: > > > 0002 makes use of that to support subscripting of hstore. I'm not > > sure how much we care about that from a functionality standpoint, > > but it seems like it might be good to have a contrib module testing > > that extensions can use this. Also, I thought possibly an example > > showing what's basically the minimum possible amount of complexity > > would be good to have. If people like this, I'll finish it up (it > > lacks docs) and add it. > > > > +1 using subscripts for hstore is nice idea Yeah, I also find it's a good suggestion, the implementation seems fine as well. As a side note, I'm surprised hstore doesn't have any functionality to update values, except hstore_concat.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Fri, Dec 11, 2020 at 10:38:07AM -0500, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: > >> On Wed, Dec 09, 2020 at 04:59:34PM -0500, Tom Lane wrote: > >> 0001 adds the ability to attach a subscript handler to an existing > >> data type with ALTER TYPE. This is clearly going to be necessary > >> if we want extension types to be able to use this facility. The > >> only thing that I think might be controversial here is that I did > >> not add the ability to set pg_type.typelem. > > > I'm curious what could be the use case for setting pg_type.typelem for > > subscripting? I don't see this that much controversial, but maybe I'm > > missing something. > > If you want the result of subscripting to be "text" or some other built-in > type, then clearly there's no need to use typelem for that, you can just > refer to the standard OID macros. The potential use-case that I thought > of for setting typelem is where an extension defines types A and B and > would like subscripting of B to yield A. Installing A's OID as B.typelem > would save a catalog lookup during subscript parsing, and remove a bunch > of edge failure cases such as what happens if A gets renamed. However, > given the dependency behavior, this would also have the effect of "you > can't drop A without dropping B, and you can't modify A in any interesting > way either". That would be annoyingly restrictive if there weren't any > actual physical containment relationship. But on the other hand, maybe > it's acceptable and we just need to document it. > > The other issue is what about existing stored SubscriptingRef structs. > If our backs were to the wall I'd think about removing the refelemtype > field so there's no stored image of typelem that needs to be updated. > But that would incur an extra catalog lookup in array_exec_setup, so > I don't much like it. If we do add the ability to set typelem, I'd > prefer to just warn people to not change it once they've installed a > subscript handler. > > Anyway, between those two issues I'm about -0.1 on adding a way to alter > typelem. I won't fight hard if somebody wants it, but I'm inclined > to leave it out. Yes, makes sense. Thanks for the clarification. > On Wed, Dec 09, 2020 at 07:37:04PM +0100, Dmitry Dolgov wrote: > > On Wed, Dec 09, 2020 at 12:49:48PM -0500, Tom Lane wrote: > > > > The jsonb parts now have to be > > rebased onto this design, which I'm assuming Dmitry will tackle > > Yes, I'm already on it, just couldn't keep up with the changes in this > thread. While rebasing the jsonb patch I found out that the current subscripting assignment implementation in transformAssignmentIndirection always coerce the value to be assigned to the type which subscripting result suppose to have (refrestype). For arrays it's fine, since those two indeed must be the same, but for jsonb (and for hstore I guess too) the result of subscripting is always jsonb (well, text type) and the assigned value could be of some other type. This leads to assigning everything converted to text. Originally this coercion was done in the type specific code, so I hoped to put it into "transform" routine. Unfortunately "transform" is called before that (and could not be called later, because type information from sbsref is required) and all the other hooks are apparently too late. Probably the most straightforward solution here would be to add a new argument to transformAssignmentIndirection to signal if coercion needs to happen or not, and allow the type specific code to specify it via SubscriptingRef. Are there any better ideas?
Re: [HACKERS] [PATCH] Generic type subscripting
> On Thu, Dec 17, 2020 at 01:49:17PM -0500, Tom Lane wrote:
> Dmitry Dolgov <9erthali...@gmail.com> writes:
> > While rebasing the jsonb patch I found out that the current subscripting
> > assignment implementation in transformAssignmentIndirection always
> > coerce the value to be assigned to the type which subscripting result
> > suppose to have (refrestype). For arrays it's fine, since those two
> > indeed must be the same, but for jsonb (and for hstore I guess too) the
> > result of subscripting is always jsonb (well, text type) and the
> > assigned value could be of some other type. This leads to assigning
> > everything converted to text.
>
> So ... what's the problem with that? Seems like what you should put
> in and what you should get out should be the same type.
>
> We can certainly reconsider the API for the parsing hook if there's
> really a good reason for these to be different types, but it seems
> like that would just be encouraging poor design.
To be more specific, this is the current behaviour (an example from the
tests) and it doesn't seem right:
=# update test_jsonb_subscript
set test_json['a'] = 3 where id = 1;
UPDATE 1
=# select jsonb_typeof(test_json->'a')
from test_jsonb_subscript where id = 1;
jsonb_typeof
--
string
=# update test_jsonb_subscript
set test_json = jsonb_set(test_json, '{a}', '3') where id = 1;
UPDATE 1
=# select jsonb_typeof(test_json->'a')
from test_jsonb_subscript where id = 1;
jsonb_typeof
--
number
Re: [HACKERS] [PATCH] Generic type subscripting
> On Thu, Dec 17, 2020 at 03:29:35PM -0500, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: > > On Thu, Dec 17, 2020 at 01:49:17PM -0500, Tom Lane wrote: > >> We can certainly reconsider the API for the parsing hook if there's > >> really a good reason for these to be different types, but it seems > >> like that would just be encouraging poor design. > > > To be more specific, this is the current behaviour (an example from the > > tests) and it doesn't seem right: > > > =# update test_jsonb_subscript > >set test_json['a'] = 3 where id = 1; > > UPDATE 1 > > =# select jsonb_typeof(test_json->'a') > >from test_jsonb_subscript where id = 1; > > jsonb_typeof > > -- > > string > > > I'm rather inclined to think that the result of subscripting a > jsonb (and therefore also the required source type for assignment) > should be jsonb, not just text. In that case, something like > update ... set jsoncol['a'] = 3 > would fail, because there's no cast from integer to jsonb. You'd > have to write one of > update ... set jsoncol['a'] = '3' > update ... set jsoncol['a'] = '"3"' > to clarify how you wanted the input to be interpreted. > But that seems like a good thing, just as it is for jsonb_in. Yep, that makes sense, will go with this idea.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Fri, Dec 18, 2020 at 08:59:25PM +0100, Dmitry Dolgov wrote:
> > On Thu, Dec 17, 2020 at 03:29:35PM -0500, Tom Lane wrote:
> > Dmitry Dolgov <9erthali...@gmail.com> writes:
> > > On Thu, Dec 17, 2020 at 01:49:17PM -0500, Tom Lane wrote:
> > >> We can certainly reconsider the API for the parsing hook if there's
> > >> really a good reason for these to be different types, but it seems
> > >> like that would just be encouraging poor design.
> >
> > > To be more specific, this is the current behaviour (an example from the
> > > tests) and it doesn't seem right:
> >
> > > =# update test_jsonb_subscript
> > >set test_json['a'] = 3 where id = 1;
> > > UPDATE 1
> > > =# select jsonb_typeof(test_json->'a')
> > >from test_jsonb_subscript where id = 1;
> > > jsonb_typeof
> > > --
> > > string
> >
> >
> > I'm rather inclined to think that the result of subscripting a
> > jsonb (and therefore also the required source type for assignment)
> > should be jsonb, not just text. In that case, something like
> > update ... set jsoncol['a'] = 3
> > would fail, because there's no cast from integer to jsonb. You'd
> > have to write one of
> > update ... set jsoncol['a'] = '3'
> > update ... set jsoncol['a'] = '"3"'
> > to clarify how you wanted the input to be interpreted.
> > But that seems like a good thing, just as it is for jsonb_in.
>
> Yep, that makes sense, will go with this idea.
Here is the new version of jsonb subscripting rebased on the committed
infrastructure patch. I hope it will not introduce any confusion with
the previously posted patched in this thread (about alter type subscript
and hstore) as they are independent.
There are few differences from the previous version:
* No limit on number of subscripts for jsonb (as there is no intrinsic
limitation of this kind for jsonb).
* In case of assignment via subscript now it expects the replace value
to be of jsonb type.
* Similar to the implementation for arrays, if the source jsonb is NULL,
it will be replaced by an empty jsonb and the new value will be
assigned to it. This means:
=# select * from test_jsonb_subscript where id = 3;
id | test_json
+---
3 | NULL
=# update test_jsonb_subscript set test_json['a'] = '1' where id = 3;
UPDATE 1
=# select * from test_jsonb_subscript where id = 3;
id | test_json
+---
3 | {"a": 1}
and similar:
=# select * from test_jsonb_subscript where id = 3;
id | test_json
+---
3 | NULL
=# update test_jsonb_subscript set test_json[1] = '1' where id = 3;
UPDATE 1
=# select * from test_jsonb_subscript where id = 3;
id | test_json
+---
3 | {"1": 1}
The latter is probably a bit strange looking, but if there are any concerns
about this part (and in general about an assignment to jsonb which is NULL)
of the implementation it could be easily changed.
* There is nothing to address question about distinguishing a regular text
subscript and jsonpath in the patch yet. I guess the idea would be to save
the original subscript value type before coercing it into text and allow a
type specific code to convert it back. But I'll probably do it as a separate
patch when we finish with this one.
>From d2aab172a4e70af0684a937b99c426652231f456 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v38] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb of type object and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule
---
doc/src/sgml/json.sgml | 48 +
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 +++-
src/backend/utils/adt/jsonbsubs.c | 282
src/backend/utils/adt/jsonfuncs.c | 180 +-
sr
Re: [HACKERS] [PATCH] Generic type subscripting
> On Tue, Dec 22, 2020 at 12:19:26PM +0100, Pavel Stehule wrote:
>
> > Here is the new version of jsonb subscripting rebased on the committed
> > infrastructure patch. I hope it will not introduce any confusion with
> > the previously posted patched in this thread (about alter type subscript
> > and hstore) as they are independent.
> >
> > There are few differences from the previous version:
> >
> > * No limit on number of subscripts for jsonb (as there is no intrinsic
> > limitation of this kind for jsonb).
> >
> > * In case of assignment via subscript now it expects the replace value
> > to be of jsonb type.
> >
> > * Similar to the implementation for arrays, if the source jsonb is NULL,
> > it will be replaced by an empty jsonb and the new value will be
> > assigned to it. This means:
> >
> > =# select * from test_jsonb_subscript where id = 3;
> > id | test_json
> > +---
> > 3 | NULL
> >
> > =# update test_jsonb_subscript set test_json['a'] = '1' where id =
> > 3;
> > UPDATE 1
> >
> > =# select * from test_jsonb_subscript where id = 3;
> > id | test_json
> > +---
> > 3 | {"a": 1}
> >
> > and similar:
> >
> > =# select * from test_jsonb_subscript where id = 3;
> > id | test_json
> > +---
> > 3 | NULL
> >
> > =# update test_jsonb_subscript set test_json[1] = '1' where id = 3;
> > UPDATE 1
> >
> > =# select * from test_jsonb_subscript where id = 3;
> > id | test_json
> > +---
> > 3 | {"1": 1}
> >
> > The latter is probably a bit strange looking, but if there are any
> > concerns
> > about this part (and in general about an assignment to jsonb which is
> > NULL)
> > of the implementation it could be easily changed.
> >
>
> What is the possibility to make an array instead of a record?
>
> I expect behave like
>
> update x set test[1] = 10; --> "[10]";
> update x set test['1'] = 10; --> "{"1": 10}"
Yes, I also was thinking about this because such behaviour is more
natural. To implement this we need to provide possibility for type
specific code to remember original subscript expression type (something
like in the attached version), which could be also useful for the future
work on jsonpath. I'm just not sure if there are again some important
bits are missing in this idea, so if someone can take a look I would
appreciate. In case there are any issues, I would just suggest keep it
simple and return NULL.
>From dc7fc5fff7b1597861b950138e1084f4ac04e321 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v38] Subscripting for jsonb (extended with subscript type)
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment. The type of such empty jsonb would be
array if the first subscript was an integer, and object in all other
cases.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The patch also extends SubscriptingRef and SubscriptingRefState with
information about original types of subscript expressions. This could be
useful if type specific subscript implementation needs to distinguish
between different data types in subscripting.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule
---
doc/src/sgml/json.sgml | 48 +
src/backend/executor/execExpr.c | 21 +-
src/backend/nodes/copyfuncs.c | 2 +
src/backend/nodes/outfuncs.c| 2 +
src/backend/nodes/readfuncs.c | 2 +
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 ++-
src/backend/utils/adt/jsonbsubs.c | 299
src/backend/utils/adt/jsonfuncs.c | 180 +
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/executor/execExpr.h | 2 +
src/include/nodes/primnodes.h | 2 +
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 273 -
src/test/regress/sql/jsonb.sql | 84 +++-
16 files changed, 899 insertions(+), 106 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..cad7b02559 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,54 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
Re: [HACKERS] [PATCH] Generic type subscripting
> On Tue, Dec 22, 2020 at 11:57:13AM -0500, Tom Lane wrote:
> Dmitry Dolgov <9erthali...@gmail.com> writes:
> > On Tue, Dec 22, 2020 at 12:19:26PM +0100, Pavel Stehule wrote:
> >> I expect behave like
> >>
> >> update x set test[1] = 10; --> "[10]";
> >> update x set test['1'] = 10; --> "{"1": 10}"
>
> > Yes, I also was thinking about this because such behaviour is more
> > natural.
>
> I continue to feel that this is a fundamentally bad idea that will
> lead to much more pain than benefit. People are going to want to
> know why "test[1.0]" doesn't act like "test[1]". They are going
> to complain because "test[$1]" acts so much differently depending
> on whether they assigned a type to the $1 parameter or not. And
> they are going to bitch because dumping and reloading a rule causes
> it to do something different than it did before --- or at least we'd
> be at horrid risk of that; only if we hide the injected cast-to-text
> doesd the dumped rule look the way it needs to. Even then, the whole
> thing is critically dependent on the fact that integer-type constants
> are written and displayed differently from other constants, so it
> won't scale to any other type that someone might want to treat specially.
> So you're just leading datatype designers down a garden path that will be
> a dead end for many of them.
>
> IMO this isn't actually any saner than your previous iterations
> on the idea.
Ok. While I don't have any preferences here, we can disregard the last
posted patch (extended-with-subscript-type) and consider only
v38-0001-Subscripting-for-jsonb version.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Tue, Dec 22, 2020 at 02:21:22PM -0500, Tom Lane wrote:
> Pavel Stehule writes:
> > But maybe we try to design some that are designed already. Is there some
> > info about index specification in SQL/JSON?
>
> We do have precedent for this, it's the rules about resolving argument
> types for overloaded functions. But the conclusion that that precedent
> leads to is that we should check whether the subscript expression can
> be *implicitly* coerced to either integer or text, and fail if neither
> coercion or both coercions succeed. I'd be okay with that from a system
> design standpoint, but it's hard to say without trying it whether it
> will work out nicely from a usability standpoint. In a quick trial
> it seems it might be okay:
>
> regression=# create function mysub(int) returns text language sql
> regression-# as $$select 'int'$$;
> CREATE FUNCTION
> regression=# create function mysub(text) returns text language sql
> as $$select 'text'$$;
> CREATE FUNCTION
> regression=# select mysub(42);
> mysub
> ---
> int
> (1 row)
>
> regression=# select mysub('foo');
> mysub
> ---
> text
> (1 row)
>
> regression=# select mysub(42::bigint);
> ERROR: function mysub(bigint) does not exist
I'm not sure I completely follow and can't immediately find the relevant
code for overloaded functions, so I need to do a perception check.
Following this design in jsonb_subscripting_transform we try to coerce
the subscription expression to both integer and text (and maybe even to
jsonpath), and based on the result of which coercion has succeeded chose
different logic to handle it, right?
And just for me to understand. In the above example of the overloaded
function, with the integer we can coerce it only to text (since original
type of the expression is integer), and with the bigint it could be
coerced both to integer and text, that's why failure, isn't?
Re: pg_stat_statements and "IN" conditions
> On Wed, Nov 18, 2020 at 05:04:32PM +0100, Dmitry Dolgov wrote: > > On Wed, Aug 12, 2020 at 06:19:02PM +0200, Dmitry Dolgov wrote: > > > > I would like to start another thread to follow up on [1], mostly to bump up > > the > > topic. Just to remind, it's about how pg_stat_statements jumbling ArrayExpr > > in > > queries like: > > > > SELECT something FROM table WHERE col IN (1, 2, 3, ...) > > > > The current implementation produces different jumble hash for every > > different > > number of arguments for essentially the same query. Unfortunately a lot of > > ORMs > > like to generate these types of queries, which in turn leads to > > pg_stat_statements pollution. Ideally we want to prevent this and have only > > one > > record for such a query. > > > > As the result of [1] I've identified two highlighted approaches to improve > > this > > situation: > > > > * Reduce the generated ArrayExpr to an array Const immediately, in cases > > where > > all the inputs are Consts. > > > > * Make repeating Const to contribute nothing to the resulting hash. > > > > I've tried to prototype both approaches to find out pros/cons and be more > > specific. Attached patches could not be considered a completed piece of > > work, > > but they seem to work, mostly pass the tests and demonstrate the point. I > > would > > like to get some high level input about them and ideally make it clear what > > is > > the preferred solution to continue with. > > I've implemented the second approach mentioned above, this version was > tested on our test clusters for some time without visible issues. Will > create a CF item and would appreciate any feedback. After more testing I found couple of things that could be improved, namely in the presence of non-reducible constants one part of the query was not copied into the normalized version, and this approach also could be extended for Params. These are incorporated in the attached patch. >From a93824799eda63391989e8845393f0b773508e18 Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Tue, 17 Nov 2020 16:18:08 +0100 Subject: [PATCH v2] 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. Do the same for similar queries with VALUES as well. --- .../expected/pg_stat_statements.out | 657 +- .../pg_stat_statements/pg_stat_statements.c | 262 ++- .../sql/pg_stat_statements.sql| 129 3 files changed, 1034 insertions(+), 14 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 2a303a7f07..6978e37ca7 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -205,7 +205,7 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"; INSERT INTO test VALUES(generate_series($1, $2), $3) | 1 | 10 SELECT * FROM test ORDER BY a| 1 | 12 SELECT * FROM test WHERE a > $1 ORDER BY a | 2 |4 - SELECT * FROM test WHERE a IN ($1, $2, $3, $4, $5) | 1 |8 + SELECT * FROM test WHERE a IN ($1, $2, $3, $4, ...) | 1 |8 SELECT pg_stat_statements_reset()| 1 |1 SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 |0 UPDATE test SET b = $1 WHERE a = $2 | 6 |6 @@ -861,4 +861,659 @@ SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" | 1 | 0 |0 (6 rows) +-- +-- Consts merging +-- +SET pg_stat_statements.merge_threshold = 5; +CREATE TABLE test_merge (id int, data int); +-- IN queries +-- Normal +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 +-
Re: [HACKERS] [PATCH] Generic type subscripting
> On Sat, Dec 26, 2020 at 01:24:04PM -0500, Tom Lane wrote: > > In a case like jsonpath['...'], the initially UNKNOWN-type literal could > in theory be coerced to any of these types, so you'd have to resolve that > case manually. The overloaded-function code has an internal preference > that makes it choose TEXT if it has a choice of TEXT or some other target > type for an UNKNOWN input (cf parse_func.c starting about line 1150), but > if you ask can_coerce_type() it's going to say TRUE for all three cases. > > Roughly speaking, then, I think what you want to do is > > 1. If input type is UNKNOWNOID, choose result type TEXT. > > 2. Otherwise, apply can_coerce_type() to see if the input type can be > coerced to int4, text, or jsonpath. If it succeeds for none or more > than one of these, throw error. Otherwise choose the single successful > type. > > 3. Apply coerce_type() to coerce to the chosen result type. > > 4. At runtime, examine exprType() of the input to figure out what to do. Thanks, that was super useful. Following this suggestion I've made necessary adjustments for the patch. There is no jsonpath support, but this could be easily added on top.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Dec 30, 2020 at 02:45:12PM +0100, Dmitry Dolgov wrote:
> > On Sat, Dec 26, 2020 at 01:24:04PM -0500, Tom Lane wrote:
> >
> > In a case like jsonpath['...'], the initially UNKNOWN-type literal could
> > in theory be coerced to any of these types, so you'd have to resolve that
> > case manually. The overloaded-function code has an internal preference
> > that makes it choose TEXT if it has a choice of TEXT or some other target
> > type for an UNKNOWN input (cf parse_func.c starting about line 1150), but
> > if you ask can_coerce_type() it's going to say TRUE for all three cases.
> >
> > Roughly speaking, then, I think what you want to do is
> >
> > 1. If input type is UNKNOWNOID, choose result type TEXT.
> >
> > 2. Otherwise, apply can_coerce_type() to see if the input type can be
> > coerced to int4, text, or jsonpath. If it succeeds for none or more
> > than one of these, throw error. Otherwise choose the single successful
> > type.
> >
> > 3. Apply coerce_type() to coerce to the chosen result type.
> >
> > 4. At runtime, examine exprType() of the input to figure out what to do.
>
> Thanks, that was super useful. Following this suggestion I've made
> necessary adjustments for the patch. There is no jsonpath support, but
> this could be easily added on top.
And the forgotten patch itself.
>From 7e2fa3dc4a8b1f907a385451c6782f2dfdc743ab Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v39] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule
---
doc/src/sgml/json.sgml | 48
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 -
src/backend/utils/adt/jsonbsubs.c | 412
src/backend/utils/adt/jsonfuncs.c | 180 ++--
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +-
src/test/regress/sql/jsonb.sql | 84 +-
10 files changed, 981 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..100d1a60f4 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,54 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
+
+ jsonb Subscripting
+
+ jsonb data type supports array-style subscripting expressions
+ to extract or update particular elements. It's possible to use multiple
+ subscripting expressions to extract nested values. In this case, a chain of
+ subscripting expressions follows the same rules as the
+ path argument in jsonb_set function,
+ e.g. in case of arrays it is a 0-based operation or that negative integers
+ that appear in path count from the end of JSON arrays.
+ The result of subscripting expressions is always jsonb data type. An
+ example of subscripting syntax:
+
+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update value by key, note the single quotes - the assigned value
+-- needs to be of jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Select records using where clause with subscripting. Since the result of
+-- subscripting is jsonb and we basically want to compare two jsonb objects, we
+-- need to put the value in double quotes to be able to convert it to jsonb.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+
+
+ Subscripting for jsonb does not support slice expressions,
+ even if it contains an array.
+
+ In case if source jsonb is NULL, assignment
+ via subscripting will proceed as if it was an empty JSON object:
+
+-- If jsonb_field here is NULL, the result
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Dec 30, 2020 at 07:48:57PM +0100, Pavel Stehule wrote:
> st 30. 12. 2020 v 14:46 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> napsal:
>
> > > On Wed, Dec 30, 2020 at 02:45:12PM +0100, Dmitry Dolgov wrote:
> > > > On Sat, Dec 26, 2020 at 01:24:04PM -0500, Tom Lane wrote:
> > > >
> > > > In a case like jsonpath['...'], the initially UNKNOWN-type literal
> > could
> > > > in theory be coerced to any of these types, so you'd have to resolve
> > that
> > > > case manually. The overloaded-function code has an internal preference
> > > > that makes it choose TEXT if it has a choice of TEXT or some other
> > target
> > > > type for an UNKNOWN input (cf parse_func.c starting about line 1150),
> > but
> > > > if you ask can_coerce_type() it's going to say TRUE for all three
> > cases.
> > > >
> > > > Roughly speaking, then, I think what you want to do is
> > > >
> > > > 1. If input type is UNKNOWNOID, choose result type TEXT.
> > > >
> > > > 2. Otherwise, apply can_coerce_type() to see if the input type can be
> > > > coerced to int4, text, or jsonpath. If it succeeds for none or more
> > > > than one of these, throw error. Otherwise choose the single successful
> > > > type.
> > > >
> > > > 3. Apply coerce_type() to coerce to the chosen result type.
> > > >
> > > > 4. At runtime, examine exprType() of the input to figure out what to
> > do.
> > >
> > > Thanks, that was super useful. Following this suggestion I've made
> > > necessary adjustments for the patch. There is no jsonpath support, but
> > > this could be easily added on top.
> >
> > And the forgotten patch itself.
> >
>
> make check fails
Yeah, apparently I forgot to enable asserts back after the last
benchmarking discussion, and missed some of those. Will fix.
> 2. The index position was ignored.
>
> postgres=# update foo set a['a'][10] = '20';
> UPDATE 1
> postgres=# select * from foo;
> ┌─┐
> │ a │
> ╞═╡
> │ {"a": [20]} │
> └─┘
> (1 row)
I just realized I haven't included "filling the gaps" part, that's why
it works as before. Can add this too.
> 1. quietly ignored update
>
> postgres=# update foo set a['a'][10] = '20';
> UPDATE 1
> postgres=# select * from foo;
> ┌┐
> │ a │
> ╞╡
> │ {} │
> └┘
> (1 row)
This belongs to the original jsonb_set implementation. Although if we
started to change it anyway with "filling the gaps", maybe it's fine to
add one more flag to tune its behaviour in this case as well. I can
check how complicated that could be.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Dec 30, 2020 at 09:01:37PM +0100, Dmitry Dolgov wrote:
> > make check fails
>
> Yeah, apparently I forgot to enable asserts back after the last
> benchmarking discussion, and missed some of those. Will fix.
>
> > 2. The index position was ignored.
> >
> > postgres=# update foo set a['a'][10] = '20';
> > UPDATE 1
> > postgres=# select * from foo;
> > ┌─┐
> > │ a │
> > ╞═╡
> > │ {"a": [20]} │
> > └─┘
> > (1 row)
>
> I just realized I haven't included "filling the gaps" part, that's why
> it works as before. Can add this too.
>
> > 1. quietly ignored update
> >
> > postgres=# update foo set a['a'][10] = '20';
> > UPDATE 1
> > postgres=# select * from foo;
> > ┌┐
> > │ a │
> > ╞╡
> > │ {} │
> > └┘
> > (1 row)
>
> This belongs to the original jsonb_set implementation. Although if we
> started to change it anyway with "filling the gaps", maybe it's fine to
> add one more flag to tune its behaviour in this case as well. I can
> check how complicated that could be.
Here is what I had in mind. Assert issue in main patch is fixed (nothing
serious, it was just the rawscalar check for an empty jsonb created
during assignment), and the second patch contains all the bits with
"filling the gaps" including your suggestion about creating the whole
path if it's not present. The latter (creating the chain of empty
objects) I haven't tested that much, but if there are any issues or
concerns I guess it will not prevent the main patch from going forward.
>From c9143a620497dac5615c4de1d9349684e9af95b5 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v40 1/2] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule
---
doc/src/sgml/json.sgml | 48
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 -
src/backend/utils/adt/jsonbsubs.c | 413
src/backend/utils/adt/jsonfuncs.c | 180 ++--
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +-
src/test/regress/sql/jsonb.sql | 84 +-
10 files changed, 982 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..100d1a60f4 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,54 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
+
+ jsonb Subscripting
+
+ jsonb data type supports array-style subscripting expressions
+ to extract or update particular elements. It's possible to use multiple
+ subscripting expressions to extract nested values. In this case, a chain of
+ subscripting expressions follows the same rules as the
+ path argument in jsonb_set function,
+ e.g. in case of arrays it is a 0-based operation or that negative integers
+ that appear in path count from the end of JSON arrays.
+ The result of subscripting expressions is always jsonb data type. An
+ example of subscripting syntax:
+
+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update value by key, note the single quotes - the assigned value
+-- needs to be of jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Select records using where clause with subscripting. Since the result of
+-- subscripting is jsonb and we basically want to compare two jsonb objects, we
+-- need to put the value in double quotes to be able to convert it to jsonb.
+SELECT * FROM table_nam
Re: [HACKERS] [PATCH] Generic type subscripting
> On Thu, Dec 31, 2020 at 08:21:55PM +0100, Pavel Stehule wrote:
> čt 31. 12. 2020 v 15:27 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> napsal:
>
> the tests passed and filling gaps works well
>
> but creating empty objects doesn't work
>
> create table foo(a jsonb);
>
> insert into foo values('{}');
>
> postgres=# update foo set a['k'][1] = '20';
> UPDATE 1
> postgres=# select * from foo;
> ┌───┐
> │ a │
> ╞═══╡
> │ {"k": [null, 20]} │
> └───┘
> (1 row)
>
> it is ok
>
> postgres=# update foo set a['k3'][10] = '20';
> UPDATE 1
> postgres=# select * from foo;
> ┌───┐
> │ a │
> ╞═══╡
> │ {"k": [null, 20]} │
> └───┘
> (1 row)
>
> the second update was not successful
Right, it was working only if the source level is empty, thanks for
checking. I've found a bit more time and prepared more decent version
which covers all the cases I could come up with following the same
implementation logic. The first patch is the same though.
>From c9143a620497dac5615c4de1d9349684e9af95b5 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v41 1/2] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule
---
doc/src/sgml/json.sgml | 48
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 -
src/backend/utils/adt/jsonbsubs.c | 413
src/backend/utils/adt/jsonfuncs.c | 180 ++--
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +-
src/test/regress/sql/jsonb.sql | 84 +-
10 files changed, 982 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..100d1a60f4 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,54 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
+
+ jsonb Subscripting
+
+ jsonb data type supports array-style subscripting expressions
+ to extract or update particular elements. It's possible to use multiple
+ subscripting expressions to extract nested values. In this case, a chain of
+ subscripting expressions follows the same rules as the
+ path argument in jsonb_set function,
+ e.g. in case of arrays it is a 0-based operation or that negative integers
+ that appear in path count from the end of JSON arrays.
+ The result of subscripting expressions is always jsonb data type. An
+ example of subscripting syntax:
+
+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update value by key, note the single quotes - the assigned value
+-- needs to be of jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Select records using where clause with subscripting. Since the result of
+-- subscripting is jsonb and we basically want to compare two jsonb objects, we
+-- need to put the value in double quotes to be able to convert it to jsonb.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+
+
+ Subscripting for jsonb does not support slice expressions,
+ even if it contains an array.
+
+ In case if source jsonb is NULL, assignment
+ via subscripting will proceed as if it was an empty JSON object:
+
+-- If jsonb_field here is NULL, the result is {"a": 1}
+UPDATE table_name SET jsonb_field['a'] = '1';
+
+-- If jsonb_field here is NULL, the result is [1]
+UPDATE table_name SET jsonb_field[0] = '1';
+
+
+
+
+
Tr
Re: [HACKERS] [PATCH] Generic type subscripting
> On Sun, Jan 03, 2021 at 08:41:17PM +0100, Pavel Stehule wrote:
>
> probably some is wrong still
>
> create table foo(a jsonb);
> update foo set a['a'] = '10';
> update foo set a['b']['c'][1] = '10';
> update foo set a['b']['c'][10] = '10'
Thanks for noticing. Indeed, there was a subtle change of meaning for
'done' flag in setPath, which I haven't covered. Could you try this
version?
>From c9143a620497dac5615c4de1d9349684e9af95b5 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v42 1/2] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule
---
doc/src/sgml/json.sgml | 48
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 -
src/backend/utils/adt/jsonbsubs.c | 413
src/backend/utils/adt/jsonfuncs.c | 180 ++--
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +-
src/test/regress/sql/jsonb.sql | 84 +-
10 files changed, 982 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..100d1a60f4 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,54 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
+
+ jsonb Subscripting
+
+ jsonb data type supports array-style subscripting expressions
+ to extract or update particular elements. It's possible to use multiple
+ subscripting expressions to extract nested values. In this case, a chain of
+ subscripting expressions follows the same rules as the
+ path argument in jsonb_set function,
+ e.g. in case of arrays it is a 0-based operation or that negative integers
+ that appear in path count from the end of JSON arrays.
+ The result of subscripting expressions is always jsonb data type. An
+ example of subscripting syntax:
+
+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update value by key, note the single quotes - the assigned value
+-- needs to be of jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Select records using where clause with subscripting. Since the result of
+-- subscripting is jsonb and we basically want to compare two jsonb objects, we
+-- need to put the value in double quotes to be able to convert it to jsonb.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+
+
+ Subscripting for jsonb does not support slice expressions,
+ even if it contains an array.
+
+ In case if source jsonb is NULL, assignment
+ via subscripting will proceed as if it was an empty JSON object:
+
+-- If jsonb_field here is NULL, the result is {"a": 1}
+UPDATE table_name SET jsonb_field['a'] = '1';
+
+-- If jsonb_field here is NULL, the result is [1]
+UPDATE table_name SET jsonb_field[0] = '1';
+
+
+
+
+
Transforms
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 82732146d3..279ff15ade 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -50,6 +50,7 @@ OBJS = \
jsonb_op.o \
jsonb_util.o \
jsonfuncs.o \
+ jsonbsubs.o \
jsonpath.o \
jsonpath_exec.o \
jsonpath_gram.o \
diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index 4eeffa1424..41a1c1f9bb 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -68,18 +68,29 @@ static JsonbValue *pushJsonbValueScalar(JsonbParseState **pstate,
JsonbIteratorToken seq,
JsonbValue *scalarVal);
+JsonbValue *
+JsonbToJsonbValue(Jsonb *jsonb)
+{
+ JsonbValue *val = (JsonbValue *) palloc(sizeof(JsonbValue));
+
+ val->type = jbvBinary;
+ val->val.binary.data = &jsonb->root;
+ val->val.binary.len = VARSIZE(jsonb) - VARHDRSZ;
+
+ return val;
+}
+
/*
* Turn an in-memory JsonbValue into a Jsonb for on-disk storage.
*
- * There isn't a JsonbToJsonbValue(), because generally we find it more
- * convenient to directly iterate through the
Re: pg_stat_statements and "IN" conditions
> On Sat, Dec 26, 2020 at 08:53:28AM -0800, Zhihong Yu wrote:
> Hi,
> A few comments.
>
> + foreach(lc, (List *) expr)
> + {
> + Node * subExpr = (Node *) lfirst(lc);
> +
> + if (!IsA(subExpr, Const))
> + {
> + allConst = false;
> + break;
> + }
> + }
>
> It seems the above foreach loop (within foreach(temp, (List *) node)) can
> be preceded with a check that allConst is true. Otherwise the loop can be
> skipped.
Thanks for noticing. Now that I look at it closer I think it's the other
way around, the loop above checking constants for the first expression
is not really necessary.
> + if (currentExprIdx == pgss_merge_threshold - 1)
> + {
> + JumbleExpr(jstate, expr);
> +
> + /*
> +* A const expr is already found, so JumbleExpr must
> +* record it. Mark it as merged, it will be the
> first
> +* merged but still present in the statement query.
> +*/
> + Assert(jstate->clocations_count > 0);
> + jstate->clocations[jstate->clocations_count -
> 1].merged = true;
> + currentExprIdx++;
> + }
>
> The above snippet occurs a few times. Maybe extract into a helper method.
Originally I was hesitant to extract it was because it's quite small
part of the code. But now I've realized that the part relevant to lists
is not really correct, which makes those bits even more different, so I
think it makes sense to leave it like that. What do you think?
>From 35f3355e56462773263d31bebaf60fee6a71dca5 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Tue, 17 Nov 2020 16:18:08 +0100
Subject: [PATCH v3] 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. Do
the same for similar queries with VALUES as well.
---
.../expected/pg_stat_statements.out | 750 +-
.../pg_stat_statements/pg_stat_statements.c | 262 +-
.../sql/pg_stat_statements.sql| 139
3 files changed, 1137 insertions(+), 14 deletions(-)
diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out
index 2a303a7f07..4b5ed40bb2 100644
--- a/contrib/pg_stat_statements/expected/pg_stat_statements.out
+++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out
@@ -205,7 +205,7 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C";
INSERT INTO test VALUES(generate_series($1, $2), $3) | 1 | 10
SELECT * FROM test ORDER BY a| 1 | 12
SELECT * FROM test WHERE a > $1 ORDER BY a | 2 |4
- SELECT * FROM test WHERE a IN ($1, $2, $3, $4, $5) | 1 |8
+ SELECT * FROM test WHERE a IN ($1, $2, $3, $4, ...) | 1 |8
SELECT pg_stat_statements_reset()| 1 |1
SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 |0
UPDATE test SET b = $1 WHERE a = $2 | 6 |6
@@ -861,4 +861,752 @@ SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE
SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" | 1 | 0 |0
(6 rows)
+--
+-- Consts merging
+--
+SET pg_stat_statements.merge_threshold = 5;
+CREATE TABLE test_merge (id int, data int);
+-- IN queries
+-- Normal
+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 | dat
Re: [HACKERS] [PATCH] Generic type subscripting
> On Mon, Jan 04, 2021 at 06:56:17PM +0100, Pavel Stehule wrote:
> po 4. 1. 2021 v 14:58 odesílatel Dmitry Dolgov <9erthali...@gmail.com>
> napsal:
> postgres=# update foo set a['c']['c'][10] = '10';
> postgres=# update foo set a['c'][10][10] = '10';
Yeah, there was one clumsy memory allocation. On the way I've found and
fixed another issue with jsonb generation, right now I don't see any
other problems. But as my imagination, despite all the sci-fi I've read
this year, is apparently not so versatile, I'll rely on yours, could you
please check this version again?
>From c9143a620497dac5615c4de1d9349684e9af95b5 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v43 1/2] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule
---
doc/src/sgml/json.sgml | 48
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 -
src/backend/utils/adt/jsonbsubs.c | 413
src/backend/utils/adt/jsonfuncs.c | 180 ++--
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +-
src/test/regress/sql/jsonb.sql | 84 +-
10 files changed, 982 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..100d1a60f4 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,54 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
+
+ jsonb Subscripting
+
+ jsonb data type supports array-style subscripting expressions
+ to extract or update particular elements. It's possible to use multiple
+ subscripting expressions to extract nested values. In this case, a chain of
+ subscripting expressions follows the same rules as the
+ path argument in jsonb_set function,
+ e.g. in case of arrays it is a 0-based operation or that negative integers
+ that appear in path count from the end of JSON arrays.
+ The result of subscripting expressions is always jsonb data type. An
+ example of subscripting syntax:
+
+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update value by key, note the single quotes - the assigned value
+-- needs to be of jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Select records using where clause with subscripting. Since the result of
+-- subscripting is jsonb and we basically want to compare two jsonb objects, we
+-- need to put the value in double quotes to be able to convert it to jsonb.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+
+
+ Subscripting for jsonb does not support slice expressions,
+ even if it contains an array.
+
+ In case if source jsonb is NULL, assignment
+ via subscripting will proceed as if it was an empty JSON object:
+
+-- If jsonb_field here is NULL, the result is {"a": 1}
+UPDATE table_name SET jsonb_field['a'] = '1';
+
+-- If jsonb_field here is NULL, the result is [1]
+UPDATE table_name SET jsonb_field[0] = '1';
+
+
+
+
+
Transforms
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 82732146d3..279ff15ade 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -50,6 +50,7 @@ OBJS = \
jsonb_op.o \
jsonb_util.o \
jsonfuncs.o \
+ jsonbsubs.o \
jsonpath.o \
jsonpath_exec.o \
jsonpath_gram.o \
diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index 4eeffa1424..41a1c1f9bb 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -68,18 +68,29 @@ static JsonbValue *pushJsonbValueScal
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Jan 06, 2021 at 09:22:53PM +0100, Pavel Stehule wrote:
>
> this case should to raise exception - the value should be changed or error
> should be raised
>
> postgres=# insert into foo values('{}');
> postgres=# update foo set a['a'] = '100';
> postgres=# update foo set a['a'][1] = '-1';
> postgres=# select * from foo;
> ┌┐
> │ a │
> ╞╡
> │ {"a": 100} │
> └┘
I was expecting this question, as I've left this like that intentionally
because of two reasons:
* Opposite to other changes, to implement this one we need to introduce
a condition more interfering with normal processing, which raises
performance issues for already existing functionality in jsonb_set.
* I vaguely recall there was a similar discussion about jsonb_set with
the similar solution.
For the references what I mean I've attached the third patch, which does
this. My opinion would be to not consider it, but I'm fine leaving this
decision to committer.
>From c9143a620497dac5615c4de1d9349684e9af95b5 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v44 1/3] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule
---
doc/src/sgml/json.sgml | 48
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 -
src/backend/utils/adt/jsonbsubs.c | 413
src/backend/utils/adt/jsonfuncs.c | 180 ++--
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +-
src/test/regress/sql/jsonb.sql | 84 +-
10 files changed, 982 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..100d1a60f4 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,54 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
+
+ jsonb Subscripting
+
+ jsonb data type supports array-style subscripting expressions
+ to extract or update particular elements. It's possible to use multiple
+ subscripting expressions to extract nested values. In this case, a chain of
+ subscripting expressions follows the same rules as the
+ path argument in jsonb_set function,
+ e.g. in case of arrays it is a 0-based operation or that negative integers
+ that appear in path count from the end of JSON arrays.
+ The result of subscripting expressions is always jsonb data type. An
+ example of subscripting syntax:
+
+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update value by key, note the single quotes - the assigned value
+-- needs to be of jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Select records using where clause with subscripting. Since the result of
+-- subscripting is jsonb and we basically want to compare two jsonb objects, we
+-- need to put the value in double quotes to be able to convert it to jsonb.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+
+
+ Subscripting for jsonb does not support slice expressions,
+ even if it contains an array.
+
+ In case if source jsonb is NULL, assignment
+ via subscripting will proceed as if it was an empty JSON object:
+
+-- If jsonb_field here is NULL, the result is {"a": 1}
+UPDATE table_name SET jsonb_field['a'] = '1';
+
+-- If jsonb_field here is NULL, the result is [1]
+UPDATE table_name SET jsonb_field[0] = '1';
+
+
+
+
+
Transforms
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 82732146d3..279ff15ade 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -50,6 +50,7 @@ OBJS = \
jsonb_op.o \
jsonb_util.o \
jsonfuncs.o \
+ jsonbsubs.o \
jsonpath.o \
jsonpath_exec.o \
jsonpath_gram.o \
diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index 4eeffa1424..41a1c1f9bb 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/utils/adt/jsonb_util.c
@@ -
Re: Asynchronous and "direct" IO support for PostgreSQL.
> On Tue, Feb 23, 2021 at 02:03:44AM -0800, Andres Freund wrote: > > over the last ~year I spent a lot of time trying to figure out how we could > add AIO (asynchronous IO) and DIO (direct IO) support to postgres. While > there's still a *lot* of open questions, I think I now have a decent handle on > most of the bigger architectural questions. Thus this long email. > > Just to be clear: I don't expect the current to design to survive as-is. If > there's a few sentences below that sound a bit like describing the new world, > that's because they're from the README.md in the patch series... Thanks! > Comments? Questions? > > I plan to send separate emails about smaller chunks of this seperately - > the whole topic is just too big. In particular I plan to send something > around buffer locking / state management - it's a one of the core issues > around this imo. I'm curious about control knobs for this feature, it's somewhat related to the stats questions also discussed in this thread. I guess most important of those are max_aio_in_flight, io_max_concurrency etc, and they're going to be a hard limits, right? I'm curious if it makes sense to explore possibility to have these sort of "backpressure", e.g. if number of inflight requests is too large calculate inflight_limit a bit lower than possible (to avoid hard performance deterioration when the db is trying to do too much IO, and rather do it smooth). From what I remember io_uring does have something similar only for SQPOLL. Another similar question if this could be used for throttling of some overloaded workers in case of misconfigured clients or such?
Re: Asynchronous and "direct" IO support for PostgreSQL.
> On Wed, Feb 24, 2021 at 01:45:10PM -0800, Andres Freund wrote: > > > I'm curious if it makes sense > > to explore possibility to have these sort of "backpressure", e.g. if > > number of inflight requests is too large calculate inflight_limit a bit > > lower than possible (to avoid hard performance deterioration when the db > > is trying to do too much IO, and rather do it smooth). > > What I do think is needed and feasible (there's a bunch of TODOs in the > code about it already) is to be better at only utilizing deeper queues > when lower queues don't suffice. So we e.g. don't read ahead more than a > few blocks for a scan where the query is spending most of the time > "elsewhere. > > There's definitely also some need for a bit better global, instead of > per-backend, control over the number of IOs in flight. That's not too > hard to implement - the hardest probably is to avoid it becoming a > scalability issue. > > I think the area with the most need for improvement is figuring out how > we determine the queue depths for different things using IO. Don't > really want to end up with 30 parameters influencing what queue depth to > use for (vacuum, index builds, sequential scans, index scans, bitmap > heap scans, ...) - but they benefit from a deeper queue will differ > between places. Yeah, sounds like an interesting opportunity for improvements. I'm preparing few benchmarks to understand better how this all works, so will keep this in mind. > > From what I remember io_uring does have something similar only for > > SQPOLL. Another similar question if this could be used for throttling > > of some overloaded workers in case of misconfigured clients or such? > > You mean dynamically? Or just by setting the concurrency lower for > certain users? I think doing so dynamically is way too complicated for > now. But I'd expect configuring it on a per-user basis or such to be a > reasonable thing. That might require splitting it into two GUCs - one > SUSET one and a second one that's settable by any user, but can only > lower the depth. > > I think it'll be pretty useful to e.g. configure autovacuum to have a > low queue depth instead of using the current cost limiting. That way the > impact on the overall system is limitted, but it's not slowed down > unnecessarily as much. Yes, you got it right, not dynamically, but rather expose this to be configured on e.g. per-user basis.
Re: Keep notnullattrs in RelOptInfo (Was part of UniqueKey patch series)
> On Thu, Feb 18, 2021 at 08:58:13PM +0800, Andy Fan wrote:
Thanks for continuing work on this patch!
> On Tue, Feb 16, 2021 at 12:01 PM David Rowley wrote:
>
> > On Fri, 12 Feb 2021 at 15:18, Andy Fan wrote:
> > >
> > > On Fri, Feb 12, 2021 at 9:02 AM David Rowley
> > wrote:
> > >> The reason I don't really like this is that it really depends where
> > >> you want to use RelOptInfo.notnullattrs. If someone wants to use it
> > >> to optimise something before the base quals are evaluated then they
> > >> might be unhappy that they found some NULLs.
> > >>
> > >
> > > Do you mean the notnullattrs is not set correctly before the base quals
> > are
> > > evaluated? I think we have lots of data structures which are set just
> > after some
> > > stage. but notnullattrs is special because it is set at more than 1
> > stage. However
> > > I'm doubtful it is unacceptable, Some fields ever change their meaning
> > at different
> > > stages like Var->varno. If a user has a misunderstanding on it, it
> > probably will find it
> > > at the testing stage.
> >
> > You're maybe focusing too much on your use case for notnullattrs. It
> > only cares about NULLs in the result for each query level.
> >
> > thinks of an example...
> >
> > OK, let's say I decided that COUNT(*) is faster than COUNT(id) so
> > decided that I might like to write a patch which rewrite the query to
> > use COUNT(*) when it was certain that "id" could not contain NULLs.
> >
> > The query is:
> >
> > SELECT p.partid, p.partdesc,COUNT(s.saleid) FROM part p LEFT OUTER
> > JOIN sales s ON p.partid = s.partid GROUP BY p.partid;
> >
> > sale.saleid is marked as NOT NULL in pg_attribute. As the writer of
> > the patch, I checked the comment for notnullattrs and it says "Not
> > null attrs, start from -FirstLowInvalidHeapAttributeNumber", so I
> > should be ok to assume since sales.saleid is marked in notnullattrs
> > that I can rewrite the query?!
> >
> > The documentation about the RelOptInfo.notnullattrs needs to be clear
> > what exactly it means. I'm not saying your representation of how to
> > record NOT NULL in incorrect. I'm saying that you need to be clear
> > what exactly is being recorded in that field.
> >
> > If you want it to mean "attribute marked here cannot output NULL
> > values at this query level", then you should say something along those
> > lines.
> >
> > However, having said that, because this is a Bitmapset of
> > pg_attribute.attnums, it's only possible to record Vars from base
> > relations. It does not seem like you have any means to record
> > attributes that are normally NULLable, but cannot produce NULL values
> > due to a strict join qual.
> >
> > e.g: SELECT t.nullable FROM t INNER JOIN j ON t.nullable = j.something;
> >
> > I'd expect the RelOptInfo for t not to contain a bit for the
> > "nullable" column, but there's no way to record the fact that the join
> > RelOptInfo for {t,j} cannot produce a NULL for that column. It might
> > be quite useful to know that for the UniqueKeys patch.
> >
>
> I checked again and found I do miss the check on JoinExpr->quals. I have
> fixed it in v3 patch. Thanks for the review!
>
> In the attached v3, commit 1 is the real patch, and commit 2 is just add
> some logs to help local testing. notnull.sql/notnull.out is the test case
> for
> this patch, both commit 2 and notnull.* are not intended to be committed
> at last.
Just to clarify, this version of notnullattrs here is the latest one,
and another one from "UniqueKey on Partitioned table" thread should be
disregarded?
> Besides the above fix in v3, I changed the comments alongs the notnullattrs
> as below and added a true positive helper function is_var_nullable.
With "true positive" you mean it will always correctly say if a Var is
nullable or not? I'm not sure about this, but couldn't be there still
some cases when a Var belongs to nullable_baserels, but still has some
constraints preventing it from being nullable (e.g. a silly example when
the not nullable column belong to the table, and the query does full
join of this table on itself using this column)?
Is this function necessary for the following patches? I've got an
impression that the discussion in this thread was mostly evolving about
correct description when notnullattrs could be used, not making it
bullet proof.
> Bitmapset *notnullattrs;
It looks like RelOptInfo has its own out function _outRelOptInfo,
probably the notnullattrs should be also present there as BITMAPSET_FIELD?
As a side note, I've attached those two new threads to CF item [1],
hopefully it's correct.
[1]: https://commitfest.postgresql.org/32/2433/
Re: Keep notnullattrs in RelOptInfo (Was part of UniqueKey patch series)
> On Fri, Mar 05, 2021 at 10:22:45AM +0800, Andy Fan wrote: > > > I checked again and found I do miss the check on JoinExpr->quals. I have > > > fixed it in v3 patch. Thanks for the review! > > > > > > In the attached v3, commit 1 is the real patch, and commit 2 is just add > > > some logs to help local testing. notnull.sql/notnull.out is the test > > case > > > for > > > this patch, both commit 2 and notnull.* are not intended to be committed > > > at last. > > > > Just to clarify, this version of notnullattrs here is the latest one, > > and another one from "UniqueKey on Partitioned table" thread should be > > disregarded? > > > > Actually they are different sections for UniqueKey. Since I don't want to > mess > two topics in one place, I open another thread. The topic here is how to > represent > a not null attribute, which is a precondition for all UniqueKey stuff. The > thread > " UniqueKey on Partitioned table[1] " is talking about how to maintain the > UniqueKey on a partitioned table only. Sure, those two threads are addressing different topics. But [1] also includes the patch for notnullattrs (I guess it's the same as one of the older versions from this thread), so it would be good to specify which one should be used to avoid any confusion. > > I'm not sure about this, but couldn't be there still > > some cases when a Var belongs to nullable_baserels, but still has some > > constraints preventing it from being nullable (e.g. a silly example when > > the not nullable column belong to the table, and the query does full > > join of this table on itself using this column)? > > > > Do you say something like "SELECT * FROM t1 left join t2 on t1.a = t2.a > WHERE > t2.b = 3; "? In this case, the outer join will be reduced to inner join > at > reduce_outer_join stage, which means t2 will not be shown in > nullable_baserels. Nope, as I said it's a bit useless example of full self join t1 on itself. In this case not null column "a" will be considered as nullable, but following your description for is_var_nullable it's fine (although couple of commentaries to this function are clearly necessary). > > Is this function necessary for the following patches? I've got an > > impression that the discussion in this thread was mostly evolving about > > correct description when notnullattrs could be used, not making it > > bullet proof. > > > > Exactly, that is the blocker issue right now. I hope more authorities can > give > some suggestions to move on. Hm...why essentially a documentation question is the blocker? Or if you mean it's a question of the patch scope, are there any arguments for extending it?
Re: POC: GROUP BY optimization
> On Tue, Oct 27, 2020 at 09:19:51PM +0100, Tomas Vondra wrote: > On Mon, Oct 26, 2020 at 11:40:40AM +0100, Dmitry Dolgov wrote: > > > On Mon, Oct 26, 2020 at 01:28:59PM +0400, Pavel Borisov wrote: > > > > Thanks for your interest! FYI there is a new thread about this topic [1] > > > > with the next version of the patch and more commentaries (I've created > > > > it for visibility purposes, but probably it also created some confusion, > > > > sorry for that). > > > > > > > > Thanks! > > > > > > I made a very quick look at your updates and noticed that it is intended > > > to > > > be simple and some parts of the code are removed as they have little test > > > coverage. I'd propose vice versa to increase test coverage to enjoy more > > > precise cost calculation and probably partial grouping. > > > > > > Or maybe it's worth to benchmark both patches and then re-decide what we > > > want more to have a more complicated or a simpler version. > > > > > > Good to know that this feature is not stuck anymore and we have more than > > > one proposal. > > > Thanks! > > > > Just to clarify, the patch that I've posted in another thread mentioned > > above is not an alternative proposal, but a development of the same > > patch I had posted in this thread. As mentioned in [1], reduce of > > functionality is an attempt to reduce the scope, and as soon as the base > > functionality looks good enough it will be returned back. > > > > I find it hard to follow two similar threads trying to do the same (or > very similar) things in different ways. Is there any chance to join > forces and produce a single patch series merging the changes? With the > "basic" functionality at the beginning, then patches with the more > complex stuff. That's the usual way, I think. > > As I said in my response on the other thread [1], I think constructing > additional paths with alternative orderings of pathkeys is the right > approach. Otherwise we can't really deal with optimizations above the > place where we consider this optimization. > > That's essentially what I was trying in explain May 16 response [2] > when I actually said this: > >So I don't think there will be a single "interesting" grouping >pathkeys (i.e. root->group_pathkeys), but a collection of pathkeys. >And we'll need to build grouping paths for all of those, and leave >the planner to eventually pick the one giving us the cheapest plan. > > I wouldn't go as far as saying the approach in this patch (i.e. picking > one particular ordering) is doomed, but it's going to be very hard to > make it work reliably. Even if we get the costing *at this node* right, > who knows how it'll affect costing of the nodes above us? > > So if I can suggest something, I'd merge the two patches, adopting the > path-based approach. With the very basic functionality/costing in the > first patch, and the more advanced stuff in additional patches. > > Does that make sense? Yes, and from what I understand it's already what had happened in the newer thread [1]. To avoid any confusion, there are no "two patches" at least from my side, and what I've posted in [1] is the continuation of this work, but with path-based approach adopted and a bit less functionality (essentially I've dropped everything what was not covered with tests in the original patch). In case if I'm missing something and Pavel's proposal is significantly different from the original patch (if I understand correctly, at the moment the latest patch posted here is a rebase and adjusting the old patch to work with the latest changes in master, right?), then indeed they could be merged, but please in the newer thread [1]. [1]: https://www.postgresql.org/message-id/flat/CA%2Bq6zcW_4o2NC0zutLkOJPsFt80megSpX_dVRo6GK9PC-Jx_Ag%40mail.gmail.com
Re: remove spurious CREATE INDEX CONCURRENTLY wait
> On Thu, Aug 20, 2020 at 03:11:19PM +0900, Michael Paquier wrote: > On Wed, Aug 19, 2020 at 02:16:46PM -0400, Alvaro Herrera wrote: > > I did not set the flag in REINDEX CONCURRENTLY, but as I understand it > > can be done too, since in essence it's the same thing as a CIC from a > > snapshot management point of view. > > Yes, I see no problems for REINDEX CONCURRENTLY as well as long as > there are no predicates and expressions involved. The transactions > that should be patched are all started in ReindexRelationConcurrently. > The transaction of index_concurrently_swap() cannot set up that > though. Only thing to be careful is to make sure that safe_flag is > correct depending on the list of indexes worked on. Hi, After looking through the thread and reading the patch it seems good, and there are only few minor questions: * Doing the same for REINDEX CONCURRENTLY, which does make sense. In fact it's already mentioned in the commentaries as done, which a bit confusing. * Naming, to be more precise what suggested Michael: > Could we consider renaming vacuumFlags? With more flags associated to > a PGPROC entry that are not related to vacuum, the current naming > makes things confusing. Something like statusFlags could fit better > in the picture? which sounds reasonable, and similar one about flag name PROC_IN_SAFE_CIC - if it covers both CREATE INDEX/REINDEX CONCURRENTLY maybe just PROC_IN_SAFE_IC? Any plans about those questions? I can imagine that are the only missing parts.
Re: How to retain lesser paths at add_path()?
> On Tue, Jan 14, 2020 at 12:46:02AM +0900, Kohei KaiGai wrote: > The v2 patch is attached. > > This adds two dedicated lists on the RelOptInfo to preserve lesser paths > if extension required to retain the path-node to be removed in usual manner. > These lesser paths are kept in the separated list, so it never expand the > length > of pathlist and partial_pathlist. That was the arguable point in the > discussion > at the last October. > > The new hook is called just before the path-node removal operation, and > gives extension a chance for extra decision. > If extension considers the path-node to be removed can be used in the upper > path construction stage, they can return 'true' as a signal to preserve this > lesser path-node. > In case when same kind of path-node already exists in the preserved_pathlist > and the supplied lesser path-node is cheaper than the old one, extension can > remove the worse one arbitrarily to keep the length of preserved_pathlist. > (E.g, PG-Strom may need one GpuJoin path-node either pathlist or preserved- > pathlist for further opportunity of combined usage with GpuPreAgg path-node. > It just needs "the best GpuJoin path-node" somewhere, not two or more.) > > Because PostgreSQL core has no information which preserved path-node can > be removed, extensions that uses path_removal_decision_hook() has > responsibility > to keep the length of preserved_(partial_)pathlist reasonable. Hi, Thanks for the patch! I had a quick look at it and have a few questions: * What would be the exact point/hook at which an extension can use preserved pathlists? I guess it's important, since I can imagine it's important for one of the issues mentioned in the thread about such an extension have to re-do significant part of the calculations from add_path. * Do you have any benchmark results with some extension using this hook? The idea with another pathlist of "discarded" paths sounds like a lightweight solution, and indeed I've performed few tests with two workloads (simple queries, queries with joins of 10 tables) and the difference between master and patched versions is rather small (no stable difference for the former, couple of percent for the latter). But it's of course with an empty hook, so it would be good to see other benchmarks as well. * Does it make sense to something similar with add_path_precheck, which also in some situations excluding paths? * This part sounds dangerous for me: > Because PostgreSQL core has no information which preserved path-node can > be removed, extensions that uses path_removal_decision_hook() has > responsibility > to keep the length of preserved_(partial_)pathlist reasonable. since an extension can keep limited number of paths in the list, but then the same hook could be reused by another extension which will also try to limit such paths, but together they'll explode.
Re: remove spurious CREATE INDEX CONCURRENTLY wait
> On Tue, Nov 03, 2020 at 07:14:47PM +0100, Dmitry Dolgov wrote:
> > On Thu, Aug 20, 2020 at 03:11:19PM +0900, Michael Paquier wrote:
> > On Wed, Aug 19, 2020 at 02:16:46PM -0400, Alvaro Herrera wrote:
> > > I did not set the flag in REINDEX CONCURRENTLY, but as I understand it
> > > can be done too, since in essence it's the same thing as a CIC from a
> > > snapshot management point of view.
> >
> > Yes, I see no problems for REINDEX CONCURRENTLY as well as long as
> > there are no predicates and expressions involved. The transactions
> > that should be patched are all started in ReindexRelationConcurrently.
> > The transaction of index_concurrently_swap() cannot set up that
> > though. Only thing to be careful is to make sure that safe_flag is
> > correct depending on the list of indexes worked on.
>
> Hi,
>
> After looking through the thread and reading the patch it seems good,
> and there are only few minor questions:
>
> * Doing the same for REINDEX CONCURRENTLY, which does make sense. In
> fact it's already mentioned in the commentaries as done, which a bit
> confusing.
Just to give it a shot, would the attached change be enough?
>From d30d8acf91679985970334069ab7f1f8f7fc3ec5 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 4 Aug 2020 22:04:57 -0400
Subject: [PATCH v3] Avoid spurious CREATE INDEX CONCURRENTLY waits
---
src/backend/commands/indexcmds.c | 122 ++-
src/include/storage/proc.h | 6 +-
2 files changed, 124 insertions(+), 4 deletions(-)
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 75552c64ed..5019397d50 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,10 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
* lazy VACUUMs, because they won't be fazed by missing index entries
* either. (Manual ANALYZEs, however, can't be excluded because they
* might be within transactions that are going to do arbitrary operations
- * later.)
+ * later.) Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
+ * on indexes that are neither expressional nor partial are also safe to
+ * ignore, since we know that those processes won't examine any data
+ * outside the table they're indexing.
*
* Also, GetCurrentVirtualXIDs never reports our own vxid, so we need not
* check for that.
@@ -406,7 +409,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
VirtualTransactionId *old_snapshots;
old_snapshots = GetCurrentVirtualXIDs(limitXmin, true, false,
- PROC_IS_AUTOVACUUM | PROC_IN_VACUUM,
+ PROC_IS_AUTOVACUUM | PROC_IN_VACUUM
+ | PROC_IN_SAFE_IC,
&n_old_snapshots);
if (progress)
pgstat_progress_update_param(PROGRESS_WAITFOR_TOTAL, n_old_snapshots);
@@ -426,7 +430,8 @@ WaitForOlderSnapshots(TransactionId limitXmin, bool progress)
newer_snapshots = GetCurrentVirtualXIDs(limitXmin,
true, false,
- PROC_IS_AUTOVACUUM | PROC_IN_VACUUM,
+ PROC_IS_AUTOVACUUM | PROC_IN_VACUUM
+ | PROC_IN_SAFE_IC,
&n_newer_snapshots);
for (j = i; j < n_old_snapshots; j++)
{
@@ -519,6 +524,7 @@ DefineIndex(Oid relationId,
bool amcanorder;
amoptions_function amoptions;
bool partitioned;
+ bool safe_index;
Datum reloptions;
int16 *coloptions;
IndexInfo *indexInfo;
@@ -1045,6 +1051,17 @@ DefineIndex(Oid relationId,
}
}
+ /*
+ * When doing concurrent index builds, we can set a PGPROC flag to tell
+ * concurrent VACUUM, CREATE INDEX CONCURRENTLY and REINDEX CONCURRENTLY
+ * to ignore us when waiting for concurrent snapshots. That can only be
+ * done for indexes that don't execute any expressions. Determine that.
+ * (The flag is reset automatically at transaction end, so it must be
+ * set for each transaction.)
+ */
+ safe_index = indexInfo->ii_Expressions == NIL &&
+ indexInfo->ii_Predicate == NIL;
+
/*
* Report index creation if appropriate (delay this till after most of the
* error checks)
@@ -1431,6 +1448,15 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ {
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyProc->vacuumFlags |= PROC_IN_SAFE_IC;
+ ProcGlobal->vacuumFlags[MyProc->pgxactoff] = MyProc->vacuumFlags;
+ LWLockRelease(ProcArrayLock);
+ }
+
/*
* The index is now visible, so we can report the OID.
*/
@@ -1490,6 +1516,15 @@ DefineIndex(Oid relationId,
CommitTransactionCommand();
StartTransactionCommand();
+ /* Tell concurrent index builds to ignore us, if index qualifies */
+ if (safe_index)
+ {
+ LWLockAcquire(ProcArrayLock,
Re: remove spurious CREATE INDEX CONCURRENTLY wait
> On Mon, Nov 09, 2020 at 10:02:27PM -0500, Tom Lane wrote: > > Alvaro Herrera writes: > > Yeah ... it would be much better if we can make it use atomics instead. > > I was thinking more like "do we need any locking at all". > > Assuming that a proc's vacuumFlags can be set by only the process itself, > there's no write conflicts to worry about. On the read side, there's a > hazard that onlookers will not see the PROC_IN_SAFE_IC flag set; but > that's not any different from what the outcome would be if they looked > just before this stanza executes. And even if they don't see it, at worst > we lose the optimization being proposed. > > There is a question of whether it's important that both copies of the flag > appear to update atomically ... but that just begs the question "why in > heaven's name are there two copies?" Sounds right, but after reading the thread about GetSnapshotData scalability more thoroughly there seem to be an assumption that those copies have to be updated at the same time under the same lock, and claims that in some cases justification for correctness around not taking ProcArrayLock is too complicated, at least for now. Interesting enough, similar discussion happened about vaccumFlags before with the same conclusion that theoretically it's fine to update without holding the lock, but this assumption could change one day and it's better to avoid such risks. Having said that I believe it makes sense to continue with locking. Are there any other opinions? I'll try to benchmark it in the meantime.
Re: remove spurious CREATE INDEX CONCURRENTLY wait
> On Fri, Nov 13, 2020 at 09:25:40AM +0900, Michael Paquier wrote: > On Thu, Nov 12, 2020 at 04:36:32PM +0100, Dmitry Dolgov wrote: > > Interesting enough, similar discussion happened about vaccumFlags before > > with the same conclusion that theoretically it's fine to update without > > holding the lock, but this assumption could change one day and it's > > better to avoid such risks. Having said that I believe it makes sense to > > continue with locking. Are there any other opinions? I'll try to > > benchmark it in the meantime. > > Thanks for planning some benchmarking for this specific patch. I have > to admit that the possibility of switching vacuumFlags to use atomics > is very appealing in the long term, with or without considering this > patch, even if we had better be sure that this patch has no actual > effect on concurrency first if atomics are not used in worst-case > scenarios. I've tried first to test scenarios where GetSnapshotData produces significant lock contention and "reindex concurrently" implementation with locks interferes with it. The idea I had is to create a test function that constantly calls GetSnapshotData (perf indeed shows significant portion of time spent on contended lock), and clash it with a stream of "reindex concurrently" of an empty relation (which still reaches safe_index check). I guess it could be considered as an artificial extreme case. Measuring GetSnapshotData (or rather the surrounding wrapper, to distinguish calls from the test function from everything else) latency without reindex, with reindex and locks, with reindex without locks should produce different "modes" and comparing them we can make some conclusions. Latency histograms without reindex (nanoseconds): nsecs : count distribution 512 -> 1023 : 0|| 1024 -> 2047 : 10001209 || 2048 -> 4095 : 76936|| 4096 -> 8191 : 1468 || 8192 -> 16383 : 98 || 16384 -> 32767 : 39 || 32768 -> 65535 : 6|| The same with reindex without locks: nsecs : count distribution 512 -> 1023 : 0|| 1024 -> 2047 : 111345 || 2048 -> 4095 : 6997627 || 4096 -> 8191 : 18575|| 8192 -> 16383 : 586 || 16384 -> 32767 : 312 || 32768 -> 65535 : 18 || The same with reindex with locks: nsecs : count distribution 512 -> 1023 : 0|| 1024 -> 2047 : 59438|| 2048 -> 4095 : 6901187 || 4096 -> 8191 : 18584|| 8192 -> 16383 : 581 || 16384 -> 32767 : 280 || 32768 -> 65535 : 84 || Looks like with reindex without locks is indeed faster (there are mode samples in lower time section), but not particularly significant to the whole distribution, especially taking into account extremity of the test. I'll take a look at benchmarking of switching vacuumFlags to use atomics, but as it's probably a bit off topic I'm going to attach another version of the patch with locks and suggested changes. To which I have one question: > Michael Paquier writes: > I think that this should be in its own routine, and that we had better > document that this should be called just after starting a transaction, > with an assertion enforcing that. I'm not sure which exactly assertion condition do you mean? >From 07c4705a22e6cdd5717df46a974ce00a69fc901f Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Wed, 11 Nov 2020 15:19:48 +0100 Subject: [PATCH v4 1/2] Rename vaccumFlags to statusFlags With more flags associated to a PGPROC entry that are not related to vacuum (currently existing or planned), the name "statusFlags" describes its purpose better. --- src/backend/access/transam/twophase.c | 2 +-
Re: pg_stat_statements and "IN" conditions
> On Wed, Aug 12, 2020 at 06:19:02PM +0200, Dmitry Dolgov wrote: > > I would like to start another thread to follow up on [1], mostly to bump up > the > topic. Just to remind, it's about how pg_stat_statements jumbling ArrayExpr in > queries like: > > SELECT something FROM table WHERE col IN (1, 2, 3, ...) > > The current implementation produces different jumble hash for every different > number of arguments for essentially the same query. Unfortunately a lot of > ORMs > like to generate these types of queries, which in turn leads to > pg_stat_statements pollution. Ideally we want to prevent this and have only > one > record for such a query. > > As the result of [1] I've identified two highlighted approaches to improve > this > situation: > > * Reduce the generated ArrayExpr to an array Const immediately, in cases where > all the inputs are Consts. > > * Make repeating Const to contribute nothing to the resulting hash. > > I've tried to prototype both approaches to find out pros/cons and be more > specific. Attached patches could not be considered a completed piece of work, > but they seem to work, mostly pass the tests and demonstrate the point. I > would > like to get some high level input about them and ideally make it clear what is > the preferred solution to continue with. I've implemented the second approach mentioned above, this version was tested on our test clusters for some time without visible issues. Will create a CF item and would appreciate any feedback. >From ece03928184d88add6629f5eba3ccc9e4fa5e7b8 Mon Sep 17 00:00:00 2001 From: Dmitrii Dolgov <9erthali...@gmail.com> Date: Tue, 17 Nov 2020 16:18:08 +0100 Subject: [PATCH v1] 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. Do the same for similar queries with VALUES as well. --- .../expected/pg_stat_statements.out | 614 +- .../pg_stat_statements/pg_stat_statements.c | 208 +- .../sql/pg_stat_statements.sql| 115 3 files changed, 925 insertions(+), 12 deletions(-) diff --git a/contrib/pg_stat_statements/expected/pg_stat_statements.out b/contrib/pg_stat_statements/expected/pg_stat_statements.out index 2a303a7f07..9d0fe074ae 100644 --- a/contrib/pg_stat_statements/expected/pg_stat_statements.out +++ b/contrib/pg_stat_statements/expected/pg_stat_statements.out @@ -205,7 +205,7 @@ SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C"; INSERT INTO test VALUES(generate_series($1, $2), $3) | 1 | 10 SELECT * FROM test ORDER BY a| 1 | 12 SELECT * FROM test WHERE a > $1 ORDER BY a | 2 |4 - SELECT * FROM test WHERE a IN ($1, $2, $3, $4, $5) | 1 |8 + SELECT * FROM test WHERE a IN ($1, $2, $3, $4, ...) | 1 |8 SELECT pg_stat_statements_reset()| 1 |1 SELECT query, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" | 0 |0 UPDATE test SET b = $1 WHERE a = $2 | 6 |6 @@ -861,4 +861,616 @@ SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE SELECT query, plans, calls, rows FROM pg_stat_statements ORDER BY query COLLATE "C" | 1 | 0 |0 (6 rows) +-- +-- Consts merging +-- +SET pg_stat_statements.merge_threshold = 5; +CREATE TABLE test_merge (id int); +-- IN queries +-- Normal +SELECT pg_stat_statements_reset(); + pg_stat_statements_reset +-- + +(1 row) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3); + id + +(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 + +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7); + id + +(0 rows) + +SELECT * FROM test_merge WHERE id IN (1, 2, 3, 4, 5, 6, 7, 8); + id + +(0 rows) + +SELECT * FROM t
Re: [HACKERS] [PATCH] Generic type subscripting
> On Fri, Nov 27, 2020 at 12:13:48PM +0300, Alexander Korotkov wrote: > > Hi! > > I've started to review this patch. Thanks! > My first question is whether we're > able to handle different subscript types differently. For instance, > one day we could handle jsonpath subscripts for jsonb. And for sure, > jsonpath subscripts are expected to be handled differently from text > subscripts. I see we can distinguish types during in prepare and > validate functions. But it seems there is no type information in > fetch and assign functions. Should we add something like this to the > SubscriptingRefState for future usage? > > Datum uppertypeoid[MAX_SUBSCRIPT_DEPTH]; > Datum lowertypeoid[MAX_SUBSCRIPT_DEPTH]; Yes, makes sense. My original idea was that it could be done within the jsonpath support patch itself, but at the same time providing these fields into SubscriptingRefState will help other potential extensions. Having said that, maybe it would be even better to introduce a field with an opaque structure for both SubscriptingRefState and SubscriptingRef, where every implementation of custom subscripting can store any necessary information? In case of jsonpath it could keep type information acquired in prepare function, which would be then passed via SubscriptingRefState down to the fetch/assign.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Mon, Nov 30, 2020 at 04:12:29PM +0300, Alexander Korotkov wrote: > > > > My first question is whether we're > > > able to handle different subscript types differently. For instance, > > > one day we could handle jsonpath subscripts for jsonb. And for sure, > > > jsonpath subscripts are expected to be handled differently from text > > > subscripts. I see we can distinguish types during in prepare and > > > validate functions. But it seems there is no type information in > > > fetch and assign functions. Should we add something like this to the > > > SubscriptingRefState for future usage? > > > > > > Datum uppertypeoid[MAX_SUBSCRIPT_DEPTH]; > > > Datum lowertypeoid[MAX_SUBSCRIPT_DEPTH]; > > > > Yes, makes sense. My original idea was that it could be done within the > > jsonpath support patch itself, but at the same time providing these > > fields into SubscriptingRefState will help other potential extensions. > > > > Having said that, maybe it would be even better to introduce a field > > with an opaque structure for both SubscriptingRefState and > > SubscriptingRef, where every implementation of custom subscripting can > > store any necessary information? In case of jsonpath it could keep type > > information acquired in prepare function, which would be then passed via > > SubscriptingRefState down to the fetch/assign. > > The idea of an opaque field in SubscriptingRef structure is more > attractive to me. Could you please implement it? Sure, doesn't seem to be that much work.
Re: [PATCH] Identify LWLocks in tracepoints
> On Sat, Dec 19, 2020 at 01:00:01PM +0800, Craig Ringer wrote: > > The attached patch set follows on from the discussion in [1] "Add LWLock > blocker(s) information" by adding the actual LWLock* and the numeric > tranche ID to each LWLock related TRACE_POSTGRESQL_foo tracepoint. > > This does not provide complete information on blockers, because it's not > necessarily valid to compare any two LWLock* pointers between two process > address spaces. The locks could be in DSM segments, and those DSM segments > could be mapped at different addresses. > > I wasn't able to work out a sensible way to map a LWLock* to any sort of > (tranche-id, lock-index) because there's no requirement that locks in a > tranche be contiguous or known individually to the lmgr. > > Despite that, the patches improve the information available for LWLock > analysis significantly. Thanks for the patches, this could be indeed useful. I've looked through and haven't noticed any issues with either the tracepoint extensions or commentaries, except that I find it is not that clear how trance_id indicates a re-initialization here? /* Re-initialization of individual LWLocks is not permitted */ Assert(tranche_id >= NUM_INDIVIDUAL_LWLOCKS || !IsUnderPostmaster); > Patch 2 adds the tranche id and lock pointer for each trace hit. This makes > it possible to differentiate between individual locks within a tranche, and > (so long as they aren't tranches in a DSM segment) compare locks between > processes. That means you can do lock-order analysis etc, which was not > previously especially feasible. I'm curious in which kind of situations lock-order analysis could be helpful? > Traces also don't have to do userspace reads for the tranche name all > the time, so the trace can run with lower overhead. This one is also interesting. Just for me to clarify, wouldn't there be a bit of overhead anyway (due to switching from kernel context to user space when a tracepoint was hit) that will mask name read overhead? Or are there any available numbers about it?
Re: [HACKERS] [PATCH] Generic type subscripting
> On Tue, Jan 12, 2021 at 08:02:59PM +0100, Pavel Stehule wrote: > ne 10. 1. 2021 v 19:52 odesílatel Pavel Stehule > napsal: > > I tested behaviour and I didn't find anything other than the mentioned > issue. > > Now I can check this feature from plpgsql, and it is working. Because there > is no special support in plpgsql runtime, the update of jsonb is > significantly slower than in update of arrays, and looks so update of jsonb > has O(N2) cost. I don't think it is important at this moment - more > important is fact, so I didn't find any memory problems. Thanks for testing. Regarding updates when the structure doesn't match provided path as I've mentioned I don't have strong preferences, but on the second though probably more inclined for returning an error in this case. Since there are pros and cons for both suggestions, it could be decided by vote majority between no update (Dian) or an error (Pavel, me) options. Any +1 to one of the options from others? Other than that, since I've already posted the patch for returning an error option, it seems that the only thing left is to decide with which version to go.
Re: POC: Cleaning up orphaned files using undo logs
> On Fri, Dec 04, 2020 at 10:22:42AM +0100, Antonin Houska wrote:
> Amit Kapila wrote:
>
> > On Fri, Nov 13, 2020 at 6:02 PM Antonin Houska wrote:
> > >
> > > Amit Kapila wrote:
> > >
> > > > On Thu, Nov 12, 2020 at 2:45 PM Antonin Houska wrote:
> > > >
> > > > If you want to track at undo record level, then won't it lead to
> > > > performance overhead and probably additional WAL overhead considering
> > > > this action needs to be WAL-logged. I think recording at page-level
> > > > might be a better idea.
> > >
> > > I'm not worried about WAL because the undo execution needs to be
> > > WAL-logged
> > > anyway - see smgr_undo() in the 0005- part of the patch set. What needs
> > > to be
> > > evaluated regarding performance is the (exclusive) locking of the page
> > > that
> > > carries the progress information.
> > >
> >
> > That is just for one kind of smgr, think how you will do it for
> > something like zheap. Their idea is to collect all the undo records
> > (unless the undo for a transaction is very large) for one zheap-page
> > and apply them together, so maintaining the status at each undo record
> > level will surely lead to a large amount of additional WAL. See below
> > how and why we have decided to do it differently.
> >
> > > I'm still not sure whether this info should
> > > be on every page or only in the chunk header. In either case, we have a
> > > problem if there are two or more chunks created by different transactions
> > > on
> > > the same page, and if more than on of these transactions need to perform
> > > undo. I tend to believe that this should happen rarely though.
> > >
> >
> > I think we need to maintain this information at the transaction level
> > and need to update it after processing a few blocks, at least that is
> > what was decided and implemented earlier. We also need to update it
> > when the log is switched or all the actions of the transaction were
> > applied. The reasoning is that for short transactions it won't matter
> > and for larger transactions, it is good to update it after a few pages
> > to avoid WAL and locking overhead. Also, it is better if we collect
> > the undo in bulk, this is proved to be beneficial for large
> > transactions.
>
> Attached is what I originally did not include in the patch series, see the
> part 0012. I have no better idea so far. The progress information is stored in
> the chunk header.
>
> To avoid too frequent locking, maybe the UpdateLastAppliedRecord() function
> can be modified so it recognizes when it's necessary to update the progress
> info. Also the user (zheap) should think when it should call the function.
> Since I've included 0012 now as a prerequisite for discarding (0013),
> currently it's only necessary to update the progress at undo log chunk
> boundary.
>
> In this version of the patch series I wanted to publish the remaining ideas I
> haven't published yet.
Thanks for the updated patch. As I've mentioned off the list I'm slowly
looking through it with the intent to concentrate on undo progress
tracking. But before I will post anything I want to mention couple of
strange issues I see, otherwise I will forget for sure. Maybe it's
already known, but running several times 'make installcheck' against a
freshly build postgres with the patch applied from time to time I
observe various errors.
This one happens on a crash recovery, seems like
UndoRecordSetXLogBufData has usr_type = USRT_INVALID and is involved in
the replay process:
TRAP: FailedAssertion("page_offset + this_page_bytes <=
uph->ud_insertion_point", File: "undopage.c", Line: 300)
postgres: startup recovering
00010012(ExceptionalCondition+0xa1)[0x558b38b8a350]
postgres: startup recovering
00010012(UndoPageSkipOverwrite+0x0)[0x558b38761b7e]
postgres: startup recovering
00010012(UndoReplay+0xa1d)[0x558b38766f32]
postgres: startup recovering
00010012(XactUndoReplay+0x77)[0x558b38769281]
postgres: startup recovering
00010012(smgr_redo+0x1af)[0x558b387aa7bd]
This one is somewhat similar:
TRAP: FailedAssertion("page_offset >= SizeOfUndoPageHeaderData", File:
"undopage.c", Line: 287)
postgres: undo worker for database 36893
(ExceptionalCondition+0xa1)[0x5559c90f1350]
postgres: undo worker for database 36893
(UndoPageOverwrite+0xa6)[0x5559c8cc8ae3]
postgres: undo worker for database 36893
(UpdateLastAppliedRecord+0xbe)[0x5559c8ccd008]
postgres: undo worker for database 36893 (smgr_undo+0xa6)[0x5559c8d11989]
There are also here and there messages about not found undo files:
ERROR: cannot open undo segment file 'base/undo/08.02': No
such file or directory
WARNING: failed to undo transaction
I haven't found out the trigger yet, but got an impression that it
happens after create_table tests.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Thu, Jan 14, 2021 at 12:02:42PM -0500, Dian M Fay wrote:
> > Other than that, since I've already posted the patch for returning an
> > error option, it seems that the only thing left is to decide with which
> > version to go.
>
> The trigger issue (which I did verify) makes the "no update" option
> unworkable imo, JavaScript's behavior notwithstanding. But it should be
> called out very clearly in the documentation, since it does depart from
> what people more familiar with that behavior may expect. Here's a quick
> draft, based on your v44 patch:
>
>
> jsonb data type supports array-style subscripting expressions
> to extract or update particular elements. It's possible to use multiple
> subscripting expressions to extract nested values. In this case, a chain of
> subscripting expressions follows the same rules as the
> path argument in jsonb_set function,
> e.g. in case of arrays it is a 0-based operation or that negative integers
> that appear in path count from the end of JSON arrays.
> The result of subscripting expressions is always of the jsonb data type.
>
>
> UPDATE statements may use subscripting in the
> SET clause to modify jsonb values. Every
> affected value must conform to the path defined by the subscript(s). If the
> path cannot be followed to its end for any individual value (e.g.
> val['a']['b']['c'] where val['a'] or
> val['b'] is null, a string, or a number), an error is
> raised even if other values do conform.
>
>
> An example of subscripting syntax:
Yes, makes sense. I've incorporated your suggestion into the last patch,
thanks.
>From c9143a620497dac5615c4de1d9349684e9af95b5 Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v45 1/3] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule
---
doc/src/sgml/json.sgml | 48
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 -
src/backend/utils/adt/jsonbsubs.c | 413
src/backend/utils/adt/jsonfuncs.c | 180 ++--
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +-
src/test/regress/sql/jsonb.sql | 84 +-
10 files changed, 982 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..100d1a60f4 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,54 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
+
+ jsonb Subscripting
+
+ jsonb data type supports array-style subscripting expressions
+ to extract or update particular elements. It's possible to use multiple
+ subscripting expressions to extract nested values. In this case, a chain of
+ subscripting expressions follows the same rules as the
+ path argument in jsonb_set function,
+ e.g. in case of arrays it is a 0-based operation or that negative integers
+ that appear in path count from the end of JSON arrays.
+ The result of subscripting expressions is always jsonb data type. An
+ example of subscripting syntax:
+
+-- Extract value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update value by key, note the single quotes - the assigned value
+-- needs to be of jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Select records using where clause with subscripting. Since the result of
+-- subscripting is jsonb and we basically want to compare two jsonb objects, we
+-- need to put the value in double quotes to be able to convert it to jsonb.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+
+
+ Subscripting for jsonb does not support slice expressions,
+ even if it contains an array.
+
+ In case if source jsonb is NULL, assignment
+ via subscripting will proceed as if it was an empty JSON object:
+
+-- If jsonb_field here is NULL, the result is {"a": 1}
+UPDATE table_name SET jsonb_field['a'] = '1';
+
+-- If jsonb_field here is NULL, the result is [1]
Re: [HACKERS] [PATCH] Generic type subscripting
> On Tue Jan 19, 2021 at 1:42 PM EST, Pavel Stehule wrote:
>
> I found minor issues.
>
> Doc - missing tag
>
> and three whitespaces issues
>
> see attached patch
Thanks, I need to remember to not skipp doc building for testing process
even for such small changes. Hope now I didn't forget anything.
> On Wed, Jan 20, 2021 at 09:58:43AM -0500, Dian M Fay wrote:
> Here's a full editing pass on the documentation, with v45 and Pavel's
> doc-whitespaces-fix.patch applied. I also corrected a typo in one of the
> added hints.
Great! I've applied almost all of it, except:
+ A jsonb value will accept assignments to nonexistent subscript
+ paths as long as the nonexistent elements being traversed are all arrays.
Maybe I've misunderstood the intention, but there is no requirement
about arrays for creating such an empty path. I've formulated it as:
+ A jsonb value will accept assignments to nonexistent subscript
+ paths as long as the last existing path key is an object or an array.
>From a4037c651a0cfd2448f38b6c8c932b5a09136b0a Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v46 1/3] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule, Dian M Fay
---
doc/src/sgml/json.sgml | 51
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 -
src/backend/utils/adt/jsonbsubs.c | 413
src/backend/utils/adt/jsonfuncs.c | 180 ++--
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +-
src/test/regress/sql/jsonb.sql | 84 +-
10 files changed, 985 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..3ace5e444b 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,57 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
+
+ jsonb Subscripting
+
+ The jsonb data type supports array-style subscripting expressions
+ to extract and modify elements. Nested values can be indicated by chaining
+ subscripting expressions, following the same rules as the path
+ argument in the jsonb_set function. If a jsonb
+ value is an array, numeric subscripts start at zero, and negative integers count
+ backwards from the last element of the array. Slice expressions are not supported.
+ The result of a subscripting expression is always of the jsonb data type.
+
+
+
+ An example of subscripting syntax:
+
+
+-- Extract object value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested object value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract array element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update object value by key. Note the quotes around '1': the assigned
+-- value must be of the jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Filter records using a WHERE clause with subscripting. Since the result of
+-- subscripting is jsonb, the value we compare it against must also be jsonb.
+-- The double quotes make "value" also a valid jsonb string.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+
+
+ jsonb assignment via subscripting handles a few edge cases
+ differently from jsonb_set. When a source jsonb
+ is NULL, assignment via subscripting will proceed as if
+ it was an empty JSON object:
+
+
+-- Where jsonb_field was NULL, it is now {"a": 1}
+UPDATE table_name SET jsonb_field['a'] = '1';
+
+-- Where jsonb_field was NULL, it is now [1]
+UPDATE table_name SET jsonb_field[0] = '1';
+
+
+
+
+
Transforms
diff --git a/src/backend/utils/adt/Makefile b/src/backend/utils/adt/Makefile
index 82732146d3..279ff15ade 100644
--- a/src/backend/utils/adt/Makefile
+++ b/src/backend/utils/adt/Makefile
@@ -50,6 +50,7 @@ OBJS = \
jsonb_op.o \
jsonb_util.o \
jsonfuncs.o \
+ jsonbsubs.o \
jsonpath.o \
jsonpath_exec.o \
jsonpath_gram.o \
diff --git a/src/backend/utils/adt/jsonb_util.c b/src/backend/utils/adt/jsonb_util.c
index 4eeffa1424..41a1c1f9bb 100644
--- a/src/backend/utils/adt/jsonb_util.c
+++ b/src/backend/uti
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Jan 20, 2021 at 11:34:16AM -0500, Dian M Fay wrote:
> > Thanks, I need to remember to not skipp doc building for testing process
> > even for such small changes. Hope now I didn't forget anything.
> >
> > > On Wed, Jan 20, 2021 at 09:58:43AM -0500, Dian M Fay wrote:
> >
> > > Here's a full editing pass on the documentation, with v45 and Pavel's
> > > doc-whitespaces-fix.patch applied. I also corrected a typo in one of the
> > > added hints.
> >
> > Great! I've applied almost all of it, except:
> >
> > + A jsonb value will accept assignments to nonexistent
> > subscript
> > + paths as long as the nonexistent elements being traversed are all
> > arrays.
> >
> > Maybe I've misunderstood the intention, but there is no requirement
> > about arrays for creating such an empty path. I've formulated it as:
> >
> > + A jsonb value will accept assignments to nonexistent
> > subscript
> > + paths as long as the last existing path key is an object or an array.
>
> My intention there was to highlight the difference between:
>
> * SET obj['a']['b']['c'] = '"newvalue"'
> * SET arr[0][0][3] = '"newvalue"'
>
> obj has to conform to {"a": {"b": {...}}} in order to receive the
> assignment of the nested c. If it doesn't, that's the error case we
> discussed earlier. But arr can be null, [], and so on, and any missing
> structure [[[null, null, null, "newvalue"]]] will be created.
If arr is 'null', or any other scalar value, such subscripting will work
only one level deep because they represented internally as an array of
one element. If arr is '[]' the path will comply by definition. So it's
essentially the same as for objects with no particular difference. If
such a quirk about scalars being treated like arrays is bothering, we
could also bend it in this case as well (see the attached version).
>From a4037c651a0cfd2448f38b6c8c932b5a09136b0a Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v48 1/3] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule, Dian M Fay
---
doc/src/sgml/json.sgml | 51
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 -
src/backend/utils/adt/jsonbsubs.c | 413
src/backend/utils/adt/jsonfuncs.c | 180 ++--
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +-
src/test/regress/sql/jsonb.sql | 84 +-
10 files changed, 985 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..3ace5e444b 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,57 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
+
+ jsonb Subscripting
+
+ The jsonb data type supports array-style subscripting expressions
+ to extract and modify elements. Nested values can be indicated by chaining
+ subscripting expressions, following the same rules as the path
+ argument in the jsonb_set function. If a jsonb
+ value is an array, numeric subscripts start at zero, and negative integers count
+ backwards from the last element of the array. Slice expressions are not supported.
+ The result of a subscripting expression is always of the jsonb data type.
+
+
+
+ An example of subscripting syntax:
+
+
+-- Extract object value by key
+SELECT ('{"a": 1}'::jsonb)['a'];
+
+-- Extract nested object value by key path
+SELECT ('{"a": {"b": {"c": 1}}}'::jsonb)['a']['b']['c'];
+
+-- Extract array element by index
+SELECT ('[1, "2", null]'::jsonb)[1];
+
+-- Update object value by key. Note the quotes around '1': the assigned
+-- value must be of the jsonb type as well
+UPDATE table_name SET jsonb_field['key'] = '1';
+
+-- Filter records using a WHERE clause with subscripting. Since the result of
+-- subscripting is jsonb, the value we compare it against must also be jsonb.
+-- The double quotes make "value" also a valid jsonb string.
+SELECT * FROM table_name WHERE jsonb_field['key'] = '"value"';
+
+
+ jsonb assignment via subscripting handles a few edge cases
+ differently from jsonb_set. When a source jsonb
+ is NULL, assig
Re: [HACKERS] [PATCH] Generic type subscripting
> On Wed, Jan 20, 2021 at 11:37:32PM -0500, Dian M Fay wrote:
> On Wed Jan 20, 2021 at 2:08 PM EST, Dmitry Dolgov wrote:
> > > On Wed, Jan 20, 2021 at 11:34:16AM -0500, Dian M Fay wrote:
> > > > Thanks, I need to remember to not skipp doc building for testing process
> > > > even for such small changes. Hope now I didn't forget anything.
> > > >
> > > > > On Wed, Jan 20, 2021 at 09:58:43AM -0500, Dian M Fay wrote:
> > > >
> > > > > Here's a full editing pass on the documentation, with v45 and Pavel's
> > > > > doc-whitespaces-fix.patch applied. I also corrected a typo in one of
> > > > > the
> > > > > added hints.
> > > >
> > > > Great! I've applied almost all of it, except:
> > > >
> > > > + A jsonb value will accept assignments to nonexistent
> > > > subscript
> > > > + paths as long as the nonexistent elements being traversed are all
> > > > arrays.
> > > >
> > > > Maybe I've misunderstood the intention, but there is no requirement
> > > > about arrays for creating such an empty path. I've formulated it as:
> > > >
> > > > + A jsonb value will accept assignments to nonexistent
> > > > subscript
> > > > + paths as long as the last existing path key is an object or an array.
> > >
> > > My intention there was to highlight the difference between:
> > >
> > > * SET obj['a']['b']['c'] = '"newvalue"'
> > > * SET arr[0][0][3] = '"newvalue"'
> > >
> > > obj has to conform to {"a": {"b": {...}}} in order to receive the
> > > assignment of the nested c. If it doesn't, that's the error case we
> > > discussed earlier. But arr can be null, [], and so on, and any missing
> > > structure [[[null, null, null, "newvalue"]]] will be created.
> >
> > If arr is 'null', or any other scalar value, such subscripting will work
> > only one level deep because they represented internally as an array of
> > one element. If arr is '[]' the path will comply by definition. So it's
> > essentially the same as for objects with no particular difference. If
> > such a quirk about scalars being treated like arrays is bothering, we
> > could also bend it in this case as well (see the attached version).
>
> I missed that distinction in the original UPDATE paragraph too. Here's
> another revision based on v48.
Looks good, I've applied it, thanks.
>From a4037c651a0cfd2448f38b6c8c932b5a09136b0a Mon Sep 17 00:00:00 2001
From: Dmitrii Dolgov <9erthali...@gmail.com>
Date: Fri, 18 Dec 2020 17:19:51 +0100
Subject: [PATCH v49 1/3] Subscripting for jsonb
Subscripting implementation for jsonb. It does not support slices, does
not have a limit for number of subscripts and for assignment expects a
replace value to be of jsonb type. There is also one functional
difference in assignment via subscripting from jsonb_set, when an
original jsonb container is NULL, subscripting replaces it with an empty
jsonb and proceed with assignment.
For the sake of code reuse, some parts of jsonb functionality were
rearranged to allow use the same functions for jsonb_set and assign
subscripting operation.
The original idea belongs to Oleg Bartunov.
Reviewed-by: Tom Lane, Arthur Zakirov, Pavel Stehule, Dian M Fay
---
doc/src/sgml/json.sgml | 51
src/backend/utils/adt/Makefile | 1 +
src/backend/utils/adt/jsonb_util.c | 76 -
src/backend/utils/adt/jsonbsubs.c | 413
src/backend/utils/adt/jsonfuncs.c | 180 ++--
src/include/catalog/pg_proc.dat | 4 +
src/include/catalog/pg_type.dat | 3 +-
src/include/utils/jsonb.h | 6 +-
src/test/regress/expected/jsonb.out | 272 +-
src/test/regress/sql/jsonb.sql | 84 +-
10 files changed, 985 insertions(+), 105 deletions(-)
create mode 100644 src/backend/utils/adt/jsonbsubs.c
diff --git a/doc/src/sgml/json.sgml b/doc/src/sgml/json.sgml
index 5b9a5557a4..3ace5e444b 100644
--- a/doc/src/sgml/json.sgml
+++ b/doc/src/sgml/json.sgml
@@ -602,6 +602,57 @@ SELECT jdoc->'guid', jdoc->'name' FROM api WHERE jdoc @> '{"tags": ["qu
+
+ jsonb Subscripting
+
+ The jsonb data type supports array-style subscripting expressions
+ to extract and modify elements. Nested values can be indicated by chaining
+ subscripting expressions, following the same rules as the path
+ argument in the jsonb_set function. If a jsonb
+ value is an array
Re: Index Skip Scan (new UniqueKeys)
> On Thu, Jan 28, 2021 at 09:49:26PM +0900, Masahiko Sawada wrote: > Hi Dmitry, > > Status update for a commitfest entry. > > This patch entry has been "Waiting on Author" on CF app and the > discussion seems inactive from the last CF. Could you share the > current status of this patch? Heikki already sent review comments and > there was a discussion but the WoA status is correct? If it needs > reviews, please rebase the patches and set it to "Needs Reviews" on CF > app. If you're not working on this, I'm going to set it to "Returned > with Feedback", barring objections. Yes, I'm still on it. In fact, I've sketched up almost immediately couple of changes to address Heikki feedback, but was distracted by subscripting stuff. Will try to send new version of the patch soon.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Fri, Jan 29, 2021 at 7:01 PM Alexander Korotkov > wrote: > Pushed with minor cleanup. Thanks a lot! > On Sun, Jan 31, 2021 at 05:23:25PM -0500, Tom Lane wrote: > > thorntail seems unhappy: > > [From 7c5d57c...] > Fix portability issue in new jsonbsubs code. > > On machines where sizeof(Datum) > sizeof(Oid) (that is, any 64-bit > platform), the previous coding would compute a misaligned > workspace->index pointer if nupper is odd. Architectures where > misaligned access is a hard no-no would then fail. This appears > to explain why thorntail is unhappy but other buildfarm members > are not. Yeah, that was an unexpected issue, thanks! I assume few other failing buildfarm members are the same, as they show similar symptoms (e.g. mussurana or ibisbill).
Re: Commitfest 2021-11 closed
> On Fri, Dec 03, 2021 at 09:51:21AM +0100, Daniel Gustafsson wrote: > I've now closed the 2021-11 commitfest, ~36% of the patches were closed in > some > way (committed, returned with feedback, withdrawn or rejected) with 184 > patches > moved to the next CF. Impressive numbers, thank you!
Re: pg_stat_statements and "IN" conditions
> On Tue, Jan 04, 2022 at 06:02:43PM -0500, Tom Lane wrote: > We can debate whether the rules proposed here are good for > pg_stat_statements or not, but it seems inevitable that they will be a > disaster for some other consumers of the query hash. Hm, which consumers do you mean here, potential extension? Isn't the ability to use an external module to compute queryid make this situation possible anyway? > do you really think that a query with two int > parameters is equivalent to one with five float parameters for all > query-identifying purposes? Nope, and it will be hard to figure this out no matter which approach we're talking about, because it mostly depends on the context and type of queries I guess. Instead, such functionality should allow some reasonable configuration. To be clear, the use case I have in mind here is not four or five, but rather a couple of hundreds constants where chances that the whole construction was generated automatically by ORM is higher than normal. > I can see the merits of allowing different numbers of IN elements > to be considered equivalent for pg_stat_statements, but this patch > seems to go far beyond that basic idea, and I fear the side-effects > will be very bad. Not sure why it goes far beyond, but then there were two approaches under consideration, as I've stated in the first message. I already don't remember all the details, but another one was evolving around doing similar things in a more limited fashion in transformAExprIn. The problem would be then to carry the information, necessary to represent the act of "merging" some number of queryids together. Any thoughts here? The idea of keeping the original queryid untouched and add another type of id instead sounds interesting, but it will add too much overhead for a quite small use case I guess.
Re: Multiple Query IDs for a rewritten parse tree
> On Sat, Jan 08, 2022 at 07:49:59PM -0500, Tom Lane wrote: > > The idea I'd been vaguely thinking about is to allow attaching a list > of query-hash nodes to a Query, where each node would contain a "tag" > identifying the specific hash calculation method, and also the value > of the query's hash calculated according to that method. We could > probably get away with saying that all such hash values must be uint64. > The main difference from your function-OID idea, I think, is that > I'm envisioning the tags as being small integers with well-known > values, similarly to the way we manage stakind values in pg_statistic. > In this way, an extension that wants a hash that the core knows how > to calculate doesn't need its own copy of the code, and similarly > one extension could publish a calculation method for use by other > extensions. An extension that wants a slightly modified version of hash calculation implementation from the core would still need to copy everything. The core probably has to provide more than one (hash, method) pair to cover some basic needs.
Re: 2022-01 Commitfest
> On Wed, Jan 12, 2022 at 01:41:42PM +0800, Julien Rouhaud wrote: > Hi, > > The January commitfest should have started almost two weeks ago, but given > that > nothing happened until now I think that it's safe to assume that either > everyone forgot or no one wanted to volunteer. > > I'm therfore volunteering to manage this commitfest, although since it's > already quite late it's probably going to be a bit chaotic and a best effort, > but it's better than nothing. Much appreciated, thanks!
Re: MDAM techniques and Index Skip Scan patch
> On Thu, Jan 13, 2022 at 03:27:08PM +, Floris Van Nee wrote: > > > > Could you send a rebased version? In the meantime I will change the status > > on the cf app to Waiting on Author. > > Attached a rebased version. FYI, I've attached this thread to the CF item as an informational one, but as there are some patches posted here, folks may get confused. For those who have landed here with no context, I feel obliged to mention that now there are two alternative patch series posted under the same CF item: * the original one lives in [1], waiting for reviews since the last May * an alternative one posted here from Floris [1]: https://www.postgresql.org/message-id/flat/20200609102247.jdlatmfyeecg52fi@localhost
Re: Index Skip Scan (new UniqueKeys)
> On Wed, Mar 17, 2021 at 03:28:00AM +0100, Tomas Vondra wrote: > Hi, > > I took a look at the new patch series, focusing mostly on the uniquekeys > part. It'd be a bit tedious to explain all the review comments here, so > attached is a patch series with a "review" patch for some of the parts. Great, thanks. > Most of it is fairly small (corrections to comments etc.), I'll go over > the more serious part so that we can discuss it here. I'll keep it split > per parts of the original patch series. > I suggest looking for XXX and FIXME comments in all the review patches. > > > 0001 > > > > > 0002 > > In fact both 0001 & 0002 belong to another thread, which these days span [1], [2]. I've included them only because they happened to be a dependency for index skip scan following David suggestions, sorry if it's confusing. At the same time the author behind 0001 & 0002 is present in this thread as well, maybe Andy can answer these comments right here and better than me. > 0003 > > > Just some comments/whitespace. > > > 0004 > > > I wonder why we don't include this in explain TEXT format? Seems it > might make it harder to write regression tests for this? It's easier to > just check that we deduced the right unique key(s) than having to > construct an example where it actually changes the plan. Yeah, good point. I believe originally it was like that to not make explain too verbose for skip scans, but displaying prefix definitely could be helpful for testing, so will do this (and address other comments as well). [1]: https://www.postgresql.org/message-id/flat/caku4awpqjaqjwq2x-ar9g3+zhrzu1k8hnp7a+_mluov-n5a...@mail.gmail.com [2]: https://www.postgresql.org/message-id/flat/caku4awru35c9g3ce15jmvwh6b2hzf4hf7czukrsiktv7akr...@mail.gmail.com
Re: pg_stat_statements and "IN" conditions
> On Thu, Mar 18, 2021 at 09:38:09AM -0400, David Steele wrote: > On 1/5/21 10:51 AM, Zhihong Yu wrote: > > > > + int lastExprLenght = 0; > > > > Did you mean to name the variable lastExprLenghth ? > > > > w.r.t. extracting to helper method, the second and third > > if (currentExprIdx == pgss_merge_threshold - 1) blocks are similar. > > It is up to you whether to create the helper method. > > I am fine with the current formation. > > Dmitry, thoughts on this review? Oh, right. lastExprLenghth is obviously a typo, and as we agreed that the helper is not strictly necessary I wanted to wait a bit hoping for more feedback and eventually to post an accumulated patch. Doesn't make sense to post another version only to fix one typo :)
Re: UniqueKey on Partitioned table.
> On Sat, Feb 20, 2021 at 10:25:59AM +0800, Andy Fan wrote: > > The attached is a UnqiueKey with EquivalenceClass patch, I just complete the > single relation part and may have bugs. I just attached it here for design > review only. and the not-null-attrs is just v1 which we can continue > discussing on the original thread[2]. Thanks for the patch. After a short look through it I'm a bit confused and wanted to clarify, now uniquekeys list could contain both Expr and EquivalenceClass?
Re: Asynchronous and "direct" IO support for PostgreSQL.
Sorry for another late reply, finally found some time to formulate couple of thoughts. > On Thu, Feb 25, 2021 at 09:22:43AM +0100, Dmitry Dolgov wrote: > > On Wed, Feb 24, 2021 at 01:45:10PM -0800, Andres Freund wrote: > > > > > I'm curious if it makes sense > > > to explore possibility to have these sort of "backpressure", e.g. if > > > number of inflight requests is too large calculate inflight_limit a bit > > > lower than possible (to avoid hard performance deterioration when the db > > > is trying to do too much IO, and rather do it smooth). > > > > What I do think is needed and feasible (there's a bunch of TODOs in the > > code about it already) is to be better at only utilizing deeper queues > > when lower queues don't suffice. So we e.g. don't read ahead more than a > > few blocks for a scan where the query is spending most of the time > > "elsewhere. > > > > There's definitely also some need for a bit better global, instead of > > per-backend, control over the number of IOs in flight. That's not too > > hard to implement - the hardest probably is to avoid it becoming a > > scalability issue. > > > > I think the area with the most need for improvement is figuring out how > > we determine the queue depths for different things using IO. Don't > > really want to end up with 30 parameters influencing what queue depth to > > use for (vacuum, index builds, sequential scans, index scans, bitmap > > heap scans, ...) - but they benefit from a deeper queue will differ > > between places. Talking about parameters, from what I understand the actual number of queues (e.g. io_uring) created is specified by PGAIO_NUM_CONTEXTS, shouldn't it be configurable? Maybe in fact there should be not that many knobs after all - if the model assumes the storage has: * Some number of hardware queues, then the number of queues AIO implementation needs to use depends on it. For example, lowering number of contexts between different benchmark runs I could see that some of the hardware queues were significantly underutilized. Potentially there could be also such thing as too many contexts. * Certain bandwidth, then the submit batch size (io_max_concurrency or PGAIO_SUBMIT_BATCH_SIZE) depends on it. This will allow to distinguish attached storage with high bandwidth and high latency vs local storages. >From what I see max_aio_in_flight is used as a queue depth for contexts, which is workload dependent and not easy to figure out as you mentioned. To avoid having 30 different parameters maybe it's more feasible to introduce "shallow" and "deep" queues, where particular depth for those could be derived from depth of hardware queues. The question which activity should use which queue is not easy, but if I get it right from queuing theory (assuming IO producers are stationary processes and fixed IO latency from the storage) it depends on IO arrivals distribution in every particular case and this in turn could be roughly estimated for each type of activity. One can expect different IO arrivals distributions for e.g. a normal point-query backend and a checkpoint or vacuum process, no matter what are the other conditions (collecting those for few benchmark runs gives indeed pretty distinct distributions). If I understand correctly, those contexts defined by PGAIO_NUM_CONTEXTS are the main working horse, right? I'm asking because there is also something called local_ring, but it seems there are no IOs submitted into those. Assuming that contexts are a main way of submitting IO, it would be also interesting to explore isolated for different purposes contexts. I haven't finished yet my changes here to give any results, but at least doing some tests with fio show different latencies, when two io_urings are processing mixed read/writes vs isolated read or writes. On the side note, at the end of the day there are so many queues - application queue, io_uring, mq software queue, hardware queue - I'm really curious if it would amplify tail latencies. Another thing I've noticed is AIO implementation is much more significantly affected by side IO activity than synchronous one. E.g. AIO version tps drops from tens of thousands to a couple of hundreds just because of some kworker started to flush dirty buffers (especially with disabled writeback throttling), while synchronous version doesn't suffer that much. Not sure what to make of it. Btw, overall I've managed to get better numbers from AIO implementation on IO bounded test cases with local NVME device, but non IO bounded were mostly a bit slower - is it expected, or am I missing something? Interesting thing to note is that io_uring implementation apparently relaxed requirements for polling operations, now one needs to have only CAP_SYS_NICE capability, not CAP_SYS_ADMIN. I guess theoretically there are no issues using it within the current design?
Re: Improve handling of pg_stat_statements handling of bind "IN" variables
> On Thu, Oct 3, 2019 at 3:33 AM Pavel Trukhanov > wrote: > >> On Wed, Jun 26, 2019 at 11:10 PM Tom Lane wrote: >> >> Greg Stark writes: >> > Actually thinking about this for two more seconds the question is what it >> > would do with a query like >> > WHERE col = ANY '1,2,3'::integer[] >> > Or >> > WHERE col = ANY ARRAY[1,2,3] >> > Whichever the language binding that is failing to do parameterized queries >> > is doing to emulate them. >> >> Yeah, one interesting question is whether this is actually modeling >> what happens with real-world applications --- are they sending Consts, >> or Params? >> >> I resolutely dislike the idea of marking arrays that came from IN >> differently from other ones; that's just a hack and it's going to give >> rise to unexplainable behavioral differences for logically-equivalent >> queries. > > Thanks for your input. > > As for real-world applications – being a founder of a server monitoring saas > (okmeter) I have access to stats on hundreds of postgres installations. > > It shows that IN with a variable number of params is ~7 times more used than > ANY(array). Hi, I would like to do some archaeology and inquire about this thread, since unfortunately there was no patch presented as far as I see. IIUC the ideas suggested in this thread are evolving mostly about modifying parser: > On Fri, Jun 14, 2019 at 2:46 AM Tom Lane wrote: > > I do not think you need new expression infrastructure. IMO what's going on > here is that we're indulging in premature optimization in the parser. It > would be better from a structural standpoint if the output of parse analysis > were closer to what the user wrote, and then the business of separating Vars > from Consts and reducing the Consts to an array were handled in the planner's > expression preprocessing phase. > > So maybe what you should be thinking about is a preliminary patch that's > mostly in the nature of refactoring, to move that processing to where it > should be. > > Of course, life is never quite that simple; there are at least two > issues you'd have to think about. > > * The parsing phase is responsible for determining the semantics of > the query, in particular resolving the data types of the IN-list items > and choosing the comparison operators that will be used. The planner > is not allowed to rethink that. What I'm not clear about offhand is > whether the existing coding in parse analysis might lead to different > choices of data types/operators than a more straightforward approach > does. If so, we'd have to decide whether that's okay. > > * Postponing this work might make things slower overall, which wouldn't > matter too much for short IN-lists, but you can bet that people who > throw ten-thousand-entry IN-lists at us will notice. So you'd need to > keep an eye on efficiency and make sure you don't end up repeating > similar processing over and over. This puzzles me, since the original issue sounds like a "representation" problem, when we want to calculate jumble hash in a way that obviously repeating parameters or constants are hashed into one value. I see the point in ideas like this: >> One idea that comes to me after looking at the code involved is that >> it might be an improvement across-the-board if transformAExprIn were to >> reduce the generated ArrayExpr to an array Const immediately, in cases >> where all the inputs are Consts. That is going to happen anyway come >> plan time, so it'd have zero impact on semantics or query performance. >> Doing it earlier would cost nothing, and could even be a net win, by >> reducing per-parse-node overhead in places like the rewriter. >> >> The advantage for the problem at hand is that a Const that's an array >> of 2 elements is going to look the same as a Const that's any other >> number of elements so far as pg_stat_statements is concerned. >> >> This doesn't help of course in cases where the values aren't all >> Consts. Since we eliminated Vars already, the main practical case >> would be that they're Params, leading us back to the previous >> question of whether apps are binding queries with different numbers >> of parameter markers in an IN, and how hard pg_stat_statements should >> try to fuzz that if they are. >> >> Then, to Greg's point, there's a question of whether transformArrayExpr >> should do likewise, ie try to produce an array Const immediately. >> I'm a bit less excited about that, but consistency suggests that >> we should have it act the same as the IN case. Interestingly enough, something similar was already mentioned in [1]. But no one jumped into this, probably due to its relative complexity, lack of personal time resources or not clear way to handle Params (I'm actually not sure about the statistics for Consts vs Params myself and need to check this, but can easily imagine both could be an often problem). Another idea also was mentioned in [1]: > I wonder whether we could improve this by arranging things so that both > Consts and Pa
Re: Index Skip Scan (new UniqueKeys)
> On Tue, Jul 14, 2020 at 06:18:50PM +, Floris Van Nee wrote:
>
> Due to the other changes I made in
> create_distinct_paths/query_has_uniquekeys_for, it will choose a correct plan
> now, even without the EC_MUST_BE_REDUNDANT check though, so it's difficult to
> give an actual failing test case now. However, since all code filters out
> constant keys, I think uniqueness should do it too - otherwise you could get
> into problems later on. It's also more consistent. If you already know
> something is unique by just (b), it doesn't make sense to store that it's
> unique by (a,b). Now that I think of it, the best place to do this
> EC_MUST_BE_REDUNDANT check is probably inside make_pathkeys_for_uniquekeys,
> rather than build_uniquekeys though. It's probably good to move it there.
That would be my suggestion as well.
> > Along the lines I'm also curious about this part:
> >
> > - ListCell *k;
> > - List *exprs = NIL;
> > -
> > - foreach(k, ec->ec_members)
> > - {
> > - EquivalenceMember *mem = (EquivalenceMember *)
> > lfirst(k);
> > - exprs = lappend(exprs, mem->em_expr);
> > - }
> > -
> > - result = lappend(result, makeUniqueKey(exprs, false, false));
> > + EquivalenceMember *mem = (EquivalenceMember*)
> > +lfirst(list_head(ec->ec_members));
> >
> > I'm curious about this myself, maybe someone can clarify. It looks like
> > generaly speaking there could be more than one member (if not
> > ec_has_volatile), which "representing knowledge that multiple items are
> > effectively equal". Is this information is not interesting enough to
> > preserve it
> > in unique keys?
>
> Yeah, that's a good question. Hence my question about the choice for Expr
> rather than EquivalenceClass for the Unique Keys patch to Andy/David. When
> storing just Expr, it is rather difficult to check equivalence in joins for
> example. Suppose, later on we decide to add join support to the distinct skip
> scan. Consider a query like this:
> SELECT DISTINCT t1.a FROM t1 JOIN t2 ON t1.a=t2.a
> As far as my understanding goes (I didn't look into it in detail though), I
> think here the distinct_pathkey will have an EqClass {t1.a, t2.a}. That
> results in a UniqueKey with expr (t1.a) (because currently we only take the
> first Expr in the list to construct the UniqueKey). We could also construct
> *two* unique keys, one with Expr (t1.a) and one with Expr (t2.a), but I don't
> think that's the correct approach either, as it will explode when you have
> multiple pathkeys, each having multiple Expr inside their EqClasses.
One UniqueKey can have multiple corresponding expressions, which gives
us also possibility of having one unique key with (t1.a, t2.a) and it
looks now similar to EquivalenceClass.
> > > - the distinct_pathkeys may be NULL, even though there's a possibility for
> > skipping. But it wouldn't create the uniquekeys in this case. This makes the
> > planner not choose skip scans even though it could. For example in queries
> > that do SELECT DISTINCT ON (a) * FROM t1 WHERE a=1 ORDER BY a,b; Since a
> > is constant, it's eliminated from regular pathkeys.
> >
> > What would be the point of skipping if it's a constant?
>
> For the query: SELECT DISTINCT ON (a) * FROM t1 WHERE a=1 ORDER BY a,b;
> There may be 1000s of records with a=1. We're only interested in the first
> one though. The traditional non-skip approach would still scan all records
> with a=1. Skip would just fetch the first one with a=1 and then skip to the
> next prefix and stop the scan.
The idea behind this query sounds questionable to me, more transparent
would be to do this without distinct, skipping will actually do exactly
the same stuff just under another name. But if allowing skipping on
constants do not bring significant changes in the code probably it's
fine.
> > > - to combat the issues mentioned earlier, there's now a check in
> > build_index_paths that checks if the query_pathkeys matches the
> > useful_pathkeys. Note that we have to use the path keys here rather than
> > any of the unique keys. The unique keys are only Expr nodes - they do not
> > contain the necessary information about ordering. Due to elimination of
> > some constant path keys, we have to search the attributes of the index to
> > find the correct prefix to use in skipping.
> >
> > IIUC here you mean this function, right?
> >
> > + prefix = find_index_prefix_for_pathkey(root,
> > +
> > index,
> > +
> > BackwardScanDirection,
> > +
> > llast_node(PathKey,
> > +
> > root->distinct_pathkeys));
> >
> > Doesn't it duplicate the job already done in build_index_pathkeys by
> > building
> > those pathkeys again? If yes, probably it's possible to reuse
> > useful_pathkeys.
> > Not sure about unordered indexes, but looks like query_pathkeys should
> > also match in this case.
> >
>
> Yeah, there's definitely some double work there, but the actual impact may be
> limited - it doesn't actually allocate a new path key, but it looks
Re: Index Skip Scan (new UniqueKeys)
> On Tue, Jul 21, 2020 at 04:35:55PM -0700, Peter Geoghegan wrote: > > > Well, it's obviously wrong, thanks for noticing. What is necessary is to > > compare two index tuples, the start and the next one, to test if they're > > the same (in which case if I'm not mistaken probably we can compare item > > pointers). I've got this question when I was about to post a new version > > with changes to address feedback from Andy, now I'll combine them and > > send a cumulative patch. > > This sounds like approximately the same problem as the one that > _bt_killitems() has to deal with as of Postgres 13. This is handled in > a way that is admittedly pretty tricky, even though the code does not > need to be 100% certain that it's "the same" tuple. Deduplication kind > of makes that a fuzzy concept. In principle there could be one big > index tuple instead of 5 tuples, even though the logical contents of > the page have not been changed between the time we recording heap TIDs > in local and the time _bt_killitems() tried to match on those heap > TIDs to kill_prior_tuple-kill some index tuples -- a concurrent > deduplication pass could do that. Your code needs to be prepared for > stuff like that. > > Ultimately posting list tuples are just a matter of understanding the > on-disk representation -- a "Small Matter of Programming". Even > without deduplication there are potential hazards from the physical > deletion of LP_DEAD-marked tuples in _bt_vacuum_one_page() (which is > not code that runs in VACUUM, despite the name). Make sure that you > hold a buffer pin on the leaf page throughout, because you need to do > that to make sure that VACUUM cannot concurrently recycle heap TIDs. > If VACUUM *is* able to concurrently recycle heap TIDs then it'll be > subtly broken. _bt_killitems() is safe because it either holds on to a > pin or gives up when the LSN changes at all. (ISTM that your only > choice is to hold on to a leaf page pin, since you cannot just decide > to give up in the way that _bt_killitems() sometimes can.) I see, thanks for clarification. You're right, in this part of implementation there is no way to give up if LSN changes like _bt_killitems does. As far as I can see the leaf page is already pinned all the time between reading relevant tuples and comparing them, I only need to handle posting list tuples.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Fri, Jul 31, 2020 at 03:35:22PM -0400, Tom Lane wrote: > > I started to look through this again, and really found myself wondering > why we're going to all this work to invent what are fundamentally pretty > bogus "features". The thing that particularly sticks in my craw is the > 0005 patch, which tries to interpret a subscript of a JSON value as either > integer or text depending on, seemingly, the phase of the moon. I don't > think that will work. For example, with existing arrays you can do > something like arraycol['42'] and the unknown-type literal is properly > cast to an integer. The corresponding situation with a JSON subscript > would have no principled resolution. > > It doesn't help any that both coercion alternatives are attempted at > COERCION_ASSIGNMENT level, which makes it noticeably more likely that > they'll both succeed. But using ASSIGNMENT level is quite inappropriate > in any context where it's not 100% certain what the intended type is. > > The proposed commit message for 0005 claims that this is somehow improving > our standards compliance, but I see nothing in the SQL spec suggesting > that you can subscript a JSON value at all within the SQL language, so > I think that claim is just false. It's due to my lack of writing skills. As far as I can remember the discussion was about JSON path part of the standard, where one allowed to use float indexes with implementation-defined rounding or truncation. In this patch series I'm trying to think of JSON subscript as an equivalent for JSON path, hence this misleading description. Having said that, I guess the main motivation behind 0005 is performance improvements. Hopefully Nikita can provide more insights. Overall back when 0005 patch was suggested its implementation looked reasonable for me, but I'll review it again. > Maybe this could be salvaged by flushing 0005 in its current form and > having the jsonb subscript executor do something like "if the current > value-to-be-subscripted is a JSON array, then try to convert the textual > subscript value to an integer". Not sure about what the error handling > rules ought to be like, though. I'm fine with the idea of separating 0005 patch and potentially prusuing it as an independent item. Just need to rebase 0006, since Pavel mentioned that it's a reasonable change he would like to see in the final result.
Re: LSM tree for Postgres
> On Tue, Aug 04, 2020 at 11:22:13AM +0300, Konstantin Knizhnik wrote: > > Then I think about implementing ideas of LSM using standard Postgres > nbtree. > > We need two indexes: one small for fast inserts and another - big > (main) index. This top index is small enough to fit in memory so > inserts in this index are very fast. Periodically we will merge data > from top index to base index and truncate the top index. To prevent > blocking of inserts in the table while we are merging indexes we can > add ... on more index, which will be used during merge. > > So final architecture of Lsm3 is the following: two top indexes used > in cyclic way and one main index. When top index reaches some > threshold value we initiate merge with main index, done by bgworker > and switch to another top index. As far as merging indexes is done in > background, it doesn't affect insert speed. Unfortunately Postgres > Index AM has not bulk insert operation, so we have to perform normal > inserts. But inserted data is already sorted by key which should > improve access locality and partly solve random reads problem for base > index. > > Certainly to perform search in Lsm3 we have to make lookups in all > three indexes and merge search results. Thanks for sharing this! In fact this reminds me more of partitioned b-trees [1] (and more older [2]) rather than LSM as it is (although could be that the former was influenced by the latter). What could be interesting is that quite often in these and many other whitepapers (e.g. [3]) to address the lookup overhead the design includes bloom filters in one or another way to avoid searching not relevant part of an index. Tomas mentioned them in this thread as well (in the different context), probably the design suggested here could also benefit from it? [1]: Riegger Christian, Vincon Tobias, Petrov Ilia. Write-optimized indexing with partitioned b-trees. (2017). 296-300. 10.1145/3151759.3151814. [2]: Graefe Goetz. Write-Optimized B-Trees. (2004). 672-683. 10.1016/B978-012088469-8/50060-7. [3]: Huanchen Zhang, David G. Andersen, Andrew Pavlo, Michael Kaminsky, Lin Ma, and Rui Shen. Reducing the Storage Overhead of Main-Memory OLTP Databases with Hybrid Indexes. (2016). 1567–1581. 10.1145/2882903.2915222.
Re: [HACKERS] [PATCH] Generic type subscripting
> On Sun, Aug 02, 2020 at 12:50:12PM +0200, Pavel Stehule wrote:
> >
> > > Maybe this could be salvaged by flushing 0005 in its current form and
> > > having the jsonb subscript executor do something like "if the current
> > > value-to-be-subscripted is a JSON array, then try to convert the textual
> > > subscript value to an integer". Not sure about what the error handling
> > > rules ought to be like, though.
> >
> > I'm fine with the idea of separating 0005 patch and potentially prusuing
> > it as an independent item. Just need to rebase 0006, since Pavel
> > mentioned that it's a reasonable change he would like to see in the
> > final result.
> >
>
> +1
Here is what I had in mind. Worth noting that, as well as the original
patch, the attached implementation keeps the same behaviour for negative
indices. Also, I've removed a strange inconsistency one could notice
with the original implementation, when one extra gap was introduced when
we append something at the beginning of an array.
>From ed8036ffd1fd65f5779f408fd0a4080357b29df2 Mon Sep 17 00:00:00 2001
From: erthalion <9erthali...@gmail.com>
Date: Thu, 31 Jan 2019 22:37:19 +0100
Subject: [PATCH v33 1/5] Base implementation of subscripting mechanism
Introduce all the required machinery for generalizing subscripting
operation for a different data types:
* subscripting handler procedure, to set up a relation between data type
and corresponding subscripting logic.
* subscripting routines, that help generalize a subscripting logic,
since it involves few stages, namely preparation (e.g. to figure out
required types), validation (to check the input and return meaningful
error message), fetch (executed when we extract a value using
subscripting), assign (executed when we update a data type with a new
value using subscripting). Without this it would be neccessary to
introduce more new fields to pg_type, which would be too invasive.
All ArrayRef related logic was removed and landed as a separate
subscripting implementation in the following patch from this series. The
rest of the code was rearranged, to e.g. store a type of assigned value
for an assign operation.
Reviewed-by: Tom Lane, Arthur Zakirov
---
.../pg_stat_statements/pg_stat_statements.c | 1 +
src/backend/catalog/heap.c| 6 +-
src/backend/catalog/pg_type.c | 15 +-
src/backend/commands/typecmds.c | 77 +-
src/backend/executor/execExpr.c | 25 +---
src/backend/executor/execExprInterp.c | 124 +++
src/backend/nodes/copyfuncs.c | 2 +
src/backend/nodes/equalfuncs.c| 2 +
src/backend/nodes/outfuncs.c | 2 +
src/backend/nodes/readfuncs.c | 2 +
src/backend/parser/parse_expr.c | 54 ---
src/backend/parser/parse_node.c | 141 --
src/backend/parser/parse_target.c | 88 +--
src/backend/utils/adt/ruleutils.c | 21 +--
src/backend/utils/cache/lsyscache.c | 23 +++
src/include/c.h | 2 +
src/include/catalog/pg_type.h | 9 +-
src/include/executor/execExpr.h | 13 +-
src/include/nodes/primnodes.h | 6 +
src/include/nodes/subscripting.h | 42 ++
src/include/parser/parse_node.h | 6 +-
src/include/utils/lsyscache.h | 1 +
22 files changed, 336 insertions(+), 326 deletions(-)
create mode 100644 src/include/nodes/subscripting.h
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 14cad19afb..bf19507d32 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2793,6 +2793,7 @@ JumbleExpr(pgssJumbleState *jstate, Node *node)
JumbleExpr(jstate, (Node *) sbsref->reflowerindexpr);
JumbleExpr(jstate, (Node *) sbsref->refexpr);
JumbleExpr(jstate, (Node *) sbsref->refassgnexpr);
+APP_JUMB(sbsref->refnestedfunc);
}
break;
case T_FuncExpr:
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 3985326df6..911e2a1ffe 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1056,7 +1056,8 @@ AddNewRelationType(const char *typeName,
-1, /* typmod */
0, /* array dimensions for typBaseType */
false, /* Type NOT NULL */
- InvalidOid); /* rowtypes never have a collation */
+ InvalidOid, /* rowtypes never have a collation */
+ InvalidOid); /* typsubshandler - none */
}
/*
@@ -1335,7 +1336,8 @@ heap_create_with_catalog(const char *relname,
-1, /* typmod */
0, /* array dimensions for typBaseType */
false, /* Type NOT NULL */
- InvalidOid); /* rowtypes never have a collation */
+ InvalidOid, /* rowtypes never have a c
pg_stat_statements and "IN" conditions
Hi I would like to start another thread to follow up on [1], mostly to bump up the topic. Just to remind, it's about how pg_stat_statements jumbling ArrayExpr in queries like: SELECT something FROM table WHERE col IN (1, 2, 3, ...) The current implementation produces different jumble hash for every different number of arguments for essentially the same query. Unfortunately a lot of ORMs like to generate these types of queries, which in turn leads to pg_stat_statements pollution. Ideally we want to prevent this and have only one record for such a query. As the result of [1] I've identified two highlighted approaches to improve this situation: * Reduce the generated ArrayExpr to an array Const immediately, in cases where all the inputs are Consts. * Make repeating Const to contribute nothing to the resulting hash. I've tried to prototype both approaches to find out pros/cons and be more specific. Attached patches could not be considered a completed piece of work, but they seem to work, mostly pass the tests and demonstrate the point. I would like to get some high level input about them and ideally make it clear what is the preferred solution to continue with. # Reducing ArrayExpr to an array Const IIUC this requires producing a Const with ArrayType constvalue in transformAExprIn for ScalarArrayOpExpr. This could be a general improvement, since apparently it's being done later anyway. But it deals only with Const, which leaves more on the table, e.g. Params and other similar types of duplication we observe when repeating constants are wrapped into VALUES. Another point here is that it's quite possible this approach will still require corresponding changes in pg_stat_statements, since just preventing duplicates to show also loses the information. Ideally we also need to have some understanding how many elements are actually there and display it, e.g. in cases when there is just one outlier query that contains a huge IN list. # Contribute nothing to the hash I guess there could be multiple ways of doing this, but the first idea I had in mind is to skip jumbling when necessary. At the same time it can be implemented more centralized for different types of queries (although in the attached patch there are only Const & Values). In the simplest case we just identify sequence of constants of the same type, which just ignores any other cases when stuff is mixed. But I believe it's something that could be considered a rare corner case and it's better to start with the simplest solution. Having said that I believe the second approach of contributing nothing to the hash sounds more appealing, but would love to hear other opinions. [1]: https://www.postgresql.org/message-id/flat/CAF42k%3DJCfHMJtkAVXCzBn2XBxDC83xb4VhV7jU7enPnZ0CfEQQ%40mail.gmail.com 0001-Reduce-ArrayExpr-into-const-array.patch Description: Binary data 0001-Limit-jumbling-for-repeating-constants.patch Description: Binary data
Re: Autonomous database is coming to Postgres?
> On Fri, Aug 14, 2020 at 08:55:53AM -0400, Bruce Momjian wrote: > > On Thu, Aug 13, 2020 at 03:26:33AM +, tsunakawa.ta...@fujitsu.com wrote: > > Hello, > > > > I'm not sure if I should have posted this to pgsql-advocacy, but this is > > being developed so I posted here. > > Does anyone know if this development come to open source Postgres, or only > > to the cloud services of Microsoft and Google? > > (I wonder this will become another reason that Postgres won't incorporate > > optimizer hint feature.) > > > > Data systems that learn to be better > > http://news.mit.edu/2020/mit-data-systems-learn-be-better-tsunami-bao-0810 > > It seems interesting, but I don't know anyone working on this. Tim Kraska mentioned in twitter plans about releasing BAO as an open source project (PostgreSQL extension I guess?), but there seems to be no interaction with the community.
Re: pg_index.indisreplident and invalid indexes
> On Thu, Aug 27, 2020 at 11:57:21AM +0900, Michael Paquier wrote: > > I think that this problem is similar to indisclustered, and that we > had better set indisreplident to false when clearing indisvalid for an > index concurrently dropped. This would prevent problems with ALTER > TABLE of course, but also the relcache. > > Any objections to the attached? I am not sure that this is worth a > backpatch as that's unlikely going to be a problem in the field, so > I'd like to fix this issue only on HEAD. This exists since 9.4 and > the introduction of replica identities. Thanks for the patch. It sounds right, so no objections from me. But I wonder if something similar has to be done also for index_concurrently_swap function? /* * Mark the new index as valid, and the old index as invalid similarly to * what index_set_state_flags() does. */ newIndexForm->indisvalid = true; oldIndexForm->indisvalid = false; oldIndexForm->indisclustered = false;
Group by reordering optimization
Hi, Better late than never, to follow up on the original thread [1] I would like to continue the discussion with the another version of the patch for group by reordering optimization. To remind, it's about reordering of group by clauses to do sorting more efficiently. The patch is rebased and modified to address (at least partially) the suggestions about making it consider new additional paths instead of changing original ones. It is still pretty much proof-of-concept version though with many blind spots, but I wanted to start kicking it and post at least something, otherwise it will never happen. An incremental approach so to say. In many ways it still contains the original code from Teodor. Changes and notes: * Instead of changing the order directly, now patch creates another patch with modifier order of clauses. It does so for the normal sort as well as for incremental sort. The whole thing is done in two steps: first it finds a potentially better ordering taking into account number of groups, widths and comparison costs; afterwards this information is used to produce a cost estimation. This is implemented via a separate create_reordered_sort_path to not introduce too many changes, I couldn't find any better place. * Function get_func_cost was removed at some point, but unfortunately this patch was implemented before that, so it's still present there. * For simplicity I've removed support in create_partial_grouping_paths, since they were not covered by the existing tests anyway. * The costing part is pretty rudimentary and looks only at the first group. It's mostly hand crafted to pass the existing tests. The question about handling skewed data sets is not addressed yet. [1]: https://www.postgresql.org/message-id/flat/7c79e6a5-8597-74e8-0671-1c39d124c9d6%40sigaev.ru 0001-Group-by-optimization.patch Description: Binary data
Re: lastOverflowedXid does not handle transaction ID wraparound
> On Tue, Oct 12, 2021 at 09:53:22PM -0700, Stan Hu wrote: > > I described how PostgreSQL can enter into a suboverflow condition on > the replica under a number of conditions: > > 1. A long transaction starts. > 2. A single SAVEPOINT is issued. > 3. Many rows are updated on the primary, and the same rows are read > from the replica. > > I noticed that lastOverflowedXid doesn't get cleared even after all > subtransactions have been completed. On a replica, it only seems to be > updated via a XLOG_XACT_ASSIGNMENT, but no such message will be sent > if subtransactions halt. If the XID wraps around again and a long > transaction starts before lastOverflowedXid, the replica might > unnecessarily enter in the suboverflow condition again. Hi, that's an interesting finding, thanks for the investigation. I didn't reproduce it fully (haven't checked the wraparound part), but indeed lastOverflowedXid is not changing that often, only every PGPROC_MAX_CACHED_SUBXIDS subtransactions. I wonder what would be side effects of clearing it when the snapshot is not suboverfloved anymore?
Re: lastOverflowedXid does not handle transaction ID wraparound
> On Wed, Oct 20, 2021 at 04:00:35PM +0500, Andrey Borodin wrote: > > 17 окт. 2021 г., в 21:55, Dmitry Dolgov <9erthali...@gmail.com> написал(а): > > I wonder what would be side > > effects of clearing it when the snapshot is not suboverfloved anymore? > > I think we should just invalidate lastOverflowedXid on every > XLOG_RUNNING_XACTS if subxid_overflow == false. I can't find a reason not to > do so. >From what I understand that was actually the case, lastOverflowedXid was set to InvalidTransactionId in ProcArrayApplyRecoveryInfo if subxid_overflow wasn't set. Looks like 10b7c686e52a6d1bb has changed it, to what I didn't pay attention originally.
Re: MDAM techniques and Index Skip Scan patch
> On Thu, Oct 21, 2021 at 07:16:00PM -0700, Peter Geoghegan wrote: > > My general concern is that the skip scan patch may currently be > structured in a way that paints us into a corner, MDAM-wise. > > Note that the MDAM paper treats skipping a prefix of columns as a case > where the prefix is handled by pretending that there is a clause that > looks like this: "WHERE date between -inf AND +inf" -- which is not so > different from the original sales SQL query example that I have > highlighted. We don't tend to think of queries like this (like my > sales query) as in any way related to skip-scan, because we don't > imagine that there is any skipping going on. But maybe we should > recognize the similarities. To avoid potential problems with extensibility in this sense, the implementation needs to explicitly work with sets of disjoint intervals of values instead of simple prefix size, one set of intervals per scan key. An interesting idea, doesn't seem to be a big change in terms of the patch itself.
Re: Patch abstracts in the Commitfest app
> On Fri, Nov 12, 2021 at 03:36:43PM +0100, Daniel Gustafsson wrote: > > On 12 Nov 2021, at 15:24, Justin Pryzby wrote: > > > > On Fri, Nov 12, 2021 at 01:51:28PM +0100, Daniel Gustafsson wrote: > >> While reading through and working with the hundreds of patches in the CF > >> app a > >> small feature/process request struck me: it would be really helpful if the > >> patch had a brief abstract outlining what it aims to add or fix (or > >> summary, > >> description or something else; not sure what to call it). Basically a > >> two-sentence or so version of the email posting the patch to -hackers. > > > > This seems fine ; that purpose is partially served (and duplicated) by the > > patch commit messages (if used). > > That's the problem, many patches are in diff format and don't have commit > messages at all. There are also many entries with patchsets containing > multiple patches and thus commitmessages but the app will only link to one. Probably encouraging to use cover letters, generated for such patch series and linked by the CF app, would be a good idea?
Re: refactoring basebackup.c
> On Fri, Nov 05, 2021 at 11:50:01AM -0400, Robert Haas wrote: > On Tue, Nov 2, 2021 at 10:32 AM Robert Haas wrote: > > Meanwhile, I think it's probably OK for me to go ahead and commit > > 0001-0003 from my patches at this point, since it seems we have pretty > > good evidence that the abstraction basically works, and there doesn't > > seem to be any value in holding off and maybe having to do a bunch > > more rebasing. > > I went ahead and committed 0001 and 0002, but got nervous about > proceeding with 0003. Hi, I'm observing a strange issue which I can only relate to bef47ff85d where bbsink abstraction was introduced. The problem is about failing assertion when doing: DETAIL: Failed process was running: BASE_BACKUP ( LABEL 'pg_basebackup base backup', PROGRESS, WAIT 0, MAX_RATE 102400, MANIFEST 'yes') Walsender tries to send a backup manifest, but crashes on the trottling sink: #2 0x560857b551af in ExceptionalCondition (conditionName=0x560857d15d27 "sink->bbs_next != NULL", errorType=0x560857d15c23 "FailedAssertion", fileName=0x560857d15d15 "basebackup_sink.c", lineNumber=91) at assert.c:69 #3 0x560857918a94 in bbsink_forward_manifest_contents (sink=0x5608593f73f8, len=32768) at basebackup_sink.c:91 #4 0x560857918d68 in bbsink_throttle_manifest_contents (sink=0x5608593f7450, len=32768) at basebackup_throttle.c:125 #5 0x5608579186d0 in bbsink_manifest_contents (sink=0x5608593f7450, len=32768) at ../../../src/include/replication/basebackup_sink.h:240 #6 0x560857918b1b in bbsink_forward_manifest_contents (sink=0x5608593f74e8, len=32768) at basebackup_sink.c:94 #7 0x560857911edc in bbsink_manifest_contents (sink=0x5608593f74e8, len=32768) at ../../../src/include/replication/basebackup_sink.h:240 #8 0x5608579129f6 in SendBackupManifest (manifest=0x7ffdaea9d120, sink=0x5608593f74e8) at backup_manifest.c:373 Looking at the similar bbsink_throttle_archive_contents it's not clear why comments for both functions (archive and manifest throttling) say "pass archive contents to next sink", but only bbsink_throttle_manifest_contents does pass bbs_next into the bbsink_forward_manifest_contents. Is it supposed to be like that? Passing the same sink object instead the next one into bbsink_forward_manifest_contents seems to solve the problem in this case.
Re: Keep notnullattrs in RelOptInfo (Was part of UniqueKey patch series)
> On Wed, Jul 07, 2021 at 01:20:24PM +1200, David Rowley wrote: > On Wed, 7 Jul 2021 at 13:04, Andy Fan wrote: > > Looking forward to watching this change closely, thank you both David and > > Tom! > > But I still don't understand what the faults my way have , do you mind > > telling the > > details? > > The problem is that we don't need 6 different ways to determine if a > Var can be NULL or not. You're proposing to add a method using > Bitmapsets and Tom has some proposing ideas around tracking > nullability in Vars. We don't need both. > > It seems to me that having it in Var allows us to have a much finer > gradient about where exactly a Var can be NULL. > > For example: SELECT nullablecol FROM tab WHERE nullablecol = ; > > If the equality operator is strict then the nullablecol can be NULL in > the WHERE clause but not in the SELECT list. Tom's idea should allow > us to determine both of those things but your idea cannot tell them > apart, so, in theory at least, Tom's idea seems better to me. Hi, This patch still occupies some place in my head, so I would like to ask few questions to see where it's going: * From the last emails in this thread I gather that the main obstacle from the design side of things is functionality around figuring out if a Var could be NULL or not, and everyone is waiting for a counterproposal about how to do that better. Is that correct? * Is this thread only about notnullattrs field in RelOptInfo, or about the UniqueKeys patch series after all? The title indicates the first one, but the last posted patch series included everything as far as I can see. * Putting my archaeologist's hat on, I've tried to find out what this alternative proposal was about. The result findings are scattered through the archives -- which proves that it's a hard topic indeed -- and participants of this thread are probably more aware about them than I am. The most detailed handwaving I found in the thread [1], with an idea to introduce NullableVar wrapper created by parser, is that it? It makes more clear why such approach could be more beneficial than a new field in RelOptInfo. And if the thread is only about the notnullattrs, I guess it would be indeed enough to object. * Now, how essential is notnullattrs functionality for the UniqueKeys patch series? From what I understand, it's being used to set multi_nulls field of every UniqueKey to indicate whether this key could produce NULL or not (which means no guaranties about uniqueness could be provided). Is there a way to limit the scope of the patch series and introduce UniqueKeys without require multi_nulls at all, or (again, in some limited situations) fetch necessary information somehow on the fly e.g. only from catcache without introducing any new infrastructure? [1]: https://www.postgresql.org/message-id/25142.1580847861%40sss.pgh.pa.us
Re: [HACKERS] advanced partition matching algorithm for partition-wise join
> On 22 March 2018 at 14:18, Ashutosh Bapat > wrote: > On Thu, Mar 22, 2018 at 4:32 AM, Dmitry Dolgov <9erthali...@gmail.com> wrote: >>> On 12 March 2018 at 06:00, Ashutosh Bapat >>> wrote: >>> Thanks for the note. Here are rebased patches. >> >> Since I started to look at this patch, I can share few random notes (although >> it's not a complete review, I'm in the progress now), maybe it can be >> helpful. >> >> In `partition_range_bounds_merge` >> >> + if (!merged) >> + break; >> >> is a bit redundant I think, because every time `merged` set to false it >> followed by break. > > Yes, right now. May be I should turn it into Assert(merged); What do you > think? Thank you for reply. Yes, that sounds good. But actually you also mentioned another topic that bothers me about this patch. Different parts of the algorithm implementation (at least for functions that build maps of matching partitions) are quite dependent on each other in terms of shared state. At first glance in `partition_range_bounds_merge` we have about a dozen of variables of different mutability level, that affect the control flow: outer_lb_index inner_lb_index merged merged_index overlap merged_lb merged_ub finished_outer finished_inner ub_cmpval lb_cmpval inner_has_default outer_has_default jointype It looks a bit too much for me, and would require commentaries like "if you changed the logic here, also take a look there". But I'm not saying that I have any specific suggestions how to change it (although I'll definitely try to do so, at least to get some better understanding of the underlying algorithm). >> >> I've noticed that in some places `IS_PARTITIONED_REL` was replaced >> >> - if (!IS_PARTITIONED_REL(joinrel)) >> + if (joinrel->part_scheme == NULL) >> >> but I'm not quite follow why? Is it because `boundinfo` is not available >> anymore at this point? If so, maybe it makes sense to update the commentary >> for >> this macro and mention to not use for joinrel. > > > This is done in try_partitionwise_join(). As explained in the comment > > * Get the list of matching partitions to be joined along with the > * partition bounds of the join relation. Because of the restrictions > * imposed by partition matching algorithm, not every pair of joining > * relations for this join will be able to use partition-wise join. But > all > * those pairs which can use partition-wise join will produce the same > * partition bounds for the join relation. > > boundinfo for the join relation is built in this function. So, we > don't have join relation's partitioning information fully set up yet. > So we can't use IS_PARTITIONED_REL() there. joinrel->part_scheme if > set tells that the joining relations have matching partition schemes > and thus the join relation can possibly use partition-wise join > technique. If it's not set, then we can't use partition-wise join. > > But IS_PARTITIONED_REL() is still useful at a number of other places, > where it's known to encounter a RelOptInfo whose partitioning > properties are fully setup. So, I don't think we should change the > macro or the comments above it. Just to make myself clear, I wanted to suggest not to change the commentary for `IS_PARTITIONED_REL` significantly, but just add a sentence that you need to check if given relation is fully set up. Also, few more random notes (mostly related to readability, since I found some parts of the patch hard to read, but of course it's arguable). ``` PartitionRangeBound outer_lb; PartitionRangeBound outer_ub; PartitionRangeBound inner_lb; PartitionRangeBound inner_ub; PartitionRangeBound *merged_lb = NULL; PartitionRangeBound *merged_ub = NULL; ``` Maybe it would be better to not repeat the type here? Let's say: ``` PartitionRangeBound outer_lb, outer_ub, ... ``` It's just too long and distracting. ``` partition_range_bounds_merge(int partnatts, FmgrInfo *partsupfuncs, Oid *partcollations, PartitionBoundInfo outer_bi, int outer_nparts, PartitionBoundInfo inner_bi, int inner_nparts, JoinType jointype, List **outer_parts, List **inner_parts) ``` >From what I see in `partition.c` there are a lot functions that accept `partnatts`, `partcollations` only to pass it down to, e.g. `partition_rbound_cmp`. What do you think about introducing a data structure to keep these arguments, and pass only an instance of this structure instead?
json(b)_to_tsvector with numeric values
Hi,
We've just noticed, that current implementation of `json(b)_to_tsvector` can be
confusing sometimes, if the target document contains numeric values.
In this case
we just drop them, and only string values will contribute to the result:
select to_tsvector('english', '{"a": "The Fat Rats", "b": 123}'::jsonb);
to_tsvector
-
'fat':2 'rat':3
(1 row)
The result would be less surprising if all values, that can be converted to
string representation (so, strings and numeric values, nothing to do for null &
boolean), will take part in it:
select to_tsvector('english', '{"a": "The Fat Rats", "b": 123}'::jsonb);
to_tsvector
-
'123':5 'fat':2 'rat':3
(1 row)
Attached patch contains small fix that's necessary to get the described
behavior. This patch doesn't touch `ts_headline` though, because following the
same approach it would require changing the type of element in the resulting
json(b).
Any opinions about this suggestion? Can it be considered as a bug fix and
included into this release?
jsonb_to_tsvector_numeric_v1.patch
Description: Binary data
Re: json(b)_to_tsvector with numeric values
> On 2 April 2018 at 11:27, Arthur Zakirov wrote: > On Mon, Apr 02, 2018 at 11:41:12AM +0300, Oleg Bartunov wrote: >> On Mon, Apr 2, 2018 at 9:45 AM, Arthur Zakirov >> wrote: >> I found this bug, when working on presentation about FTS and it looked >> annoying, since it validates >> the consistency of FTS.I think this is a bug, which needs to be fixed, >> else inconsistency with existing full text search will be gets >> deeper. >> >> The fix looks trivial, but needs a review, of course. > > Oh, I understood. The code looks good, tests passed. But maybe it is > better to use NumericGetDatum() instead of PointerGetDatum()? Well, technically speaking they're the same, but yes, NumericGetDatum would be more precise. I've modified it in the attached patch. jsonb_to_tsvector_numeric_v2.patch Description: Binary data
Re: json(b)_to_tsvector with numeric values
> On 4 April 2018 at 11:52, Teodor Sigaev wrote: the consistency of FTS.I think this is a bug, which needs to be fixed, else inconsistency with existing full text search will be gets deeper. > > Hm, seems, it's useful feature, but I suggest to make separate function > jsonb_any_to_tsvector and add support for boolean too (if you know better > name for function, do not hide it). Changing behavior of existing function > is not obvious for users and, seems, should not backpatched. What do you think about having not a separate function, but a flag argument to the existing one (like `create` in `jsonb_set`), that will have false as default value? The result would be the same, but without an extra function with almost the same implementation.
typcategory for regconfig
Hi,
Does anyone know, why `typcategory` value for tsvector `regconfig` is
`TYPCATEGORY_NUMERIC`, but in all the tests it's being used in string format?
It's probably not a big deal, but in this thread [1] it prevents me from
adopting the nice solution with a boolean flag for `to_tsvector` function,
because Postgres can't distinguish between `to_tsvector(regconfig, text)` and
`to_tsvector(jsonb, boolean)` in the expression:
to_tsvector('english', 'some text')
If it's value would be `TYPCATEGORY_STRING`, then everything will be fine,
since a string type will win. Also, it doesn't break any existing tests, so I
wonder whether it should be like that or not?
1:
https://www.postgresql.org/message-id/flat/CA%2Bq6zcXJQbS1b4kJ_HeAOoOc%3DunfnOrUEL%3DKGgE32QKDww7d8g%40mail.gmail.com
Re: typcategory for regconfig
> On 5 April 2018 at 15:27, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: >> Does anyone know, why `typcategory` value for tsvector `regconfig` is >> `TYPCATEGORY_NUMERIC`, > > Because OID is. I think we need all the OID-alias types to be the same > category as OID, else we're likely to have issues with queries like Ok, I see, thanks. > I think you need to bite the bullet and just provide the flag in > the 3-argument case (regconfig,json[b],bool). Well, it's already like that. I have now: to_tsvector(json(b), boolean) to_tsvector(regconfig, json(b), boolean) and as I mentioned above the first one is conflicting with to_tsvector(regconfig, text).
Re: typcategory for regconfig
> On 5 April 2018 at 15:48, Tom Lane wrote: > Dmitry Dolgov <9erthali...@gmail.com> writes: >> On 5 April 2018 at 15:27, Tom Lane wrote: >>> I think you need to bite the bullet and just provide the flag in >>> the 3-argument case (regconfig,json[b],bool). > >> Well, it's already like that. I have now: > >> to_tsvector(json(b), boolean) >> to_tsvector(regconfig, json(b), boolean) > >> and as I mentioned above the first one is conflicting with >> to_tsvector(regconfig, text). > > Right. So you need to either drop that form, or consider doing > something other than add-a-bool. Maybe the alternate behavior > should have a different function name, instead of being selected > by an argument? Yep, I'll swallow my perfectionism and go with a new function.
Re: json(b)_to_tsvector with numeric values
> On 4 April 2018 at 16:09, Teodor Sigaev wrote:
>
>>> Hm, seems, it's useful feature, but I suggest to make separate function
>>> jsonb_any_to_tsvector and add support for boolean too (if you know better
>>> name for function, do not hide it). Changing behavior of existing
>>> function
>>> is not obvious for users and, seems, should not backpatched.
>>
>>
>> What do you think about having not a separate function, but a flag
>> argument to
>> the existing one (like `create` in `jsonb_set`), that will have false as
>> default value? The result would be the same, but without an extra function
>> with
>> almost the same implementation.
>
>
> tsvector jsonb_to_tsvector(jsonb[, bool]) ?
> Agreed. Second arg should be optional.
Unfortunately, this idea with a flag argument can't be implemented easily
(related discussion is here [1]). So I've modified the patch accordingly to
your original suggestion about having separate functions
`json(b)_all_to_tsvector`.
1:
https://www.postgresql.org/message-id/flat/CA%2Bq6zcVJ%2BWx%2B-%3DkkN5UC0T-LtsJWnx0g9S0xSnn3jUWkriufDA%40mail.gmail.com
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5abb1c4..895b60a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9696,6 +9696,18 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple
+ json(b)_all_to_tsvector( config regconfig , document json(b))
+
+tsvector
+
+ reduce each string, numeric or boolean value in the document to a tsvector,
+ and then concatenate those in document order to produce a single tsvector
+
+json_all_to_tsvector('english', '{"a": "The Fat Rats", "b": 123}'::json)
+'123':5 'fat':2 'rat':3
+
+
+
ts_delete
diff --git a/src/backend/tsearch/to_tsany.c b/src/backend/tsearch/to_tsany.c
index ea5947a..02c2b00 100644
--- a/src/backend/tsearch/to_tsany.c
+++ b/src/backend/tsearch/to_tsany.c
@@ -267,12 +267,12 @@ to_tsvector(PG_FUNCTION_ARGS)
PointerGetDatum(in)));
}
-Datum
-jsonb_to_tsvector_byid(PG_FUNCTION_ARGS)
+/*
+ * Worker function for jsonb(_all)_to_tsvector(_byid)
+ */
+static TSVector
+jsonb_to_tsvector_worker(Oid cfgId, Jsonb *jb, bool allTypes)
{
- Oid cfgId = PG_GETARG_OID(0);
- Jsonb *jb = PG_GETARG_JSONB_P(1);
- TSVector result;
TSVectorBuildState state;
ParsedText prs;
@@ -281,11 +281,24 @@ jsonb_to_tsvector_byid(PG_FUNCTION_ARGS)
state.prs = &prs;
state.cfgId = cfgId;
- iterate_jsonb_string_values(jb, &state, add_to_tsvector);
+ if (allTypes)
+ iterate_jsonb_all_values(jb, &state, add_to_tsvector);
+ else
+ iterate_jsonb_string_values(jb, &state, add_to_tsvector);
- PG_FREE_IF_COPY(jb, 1);
- result = make_tsvector(&prs);
+ return make_tsvector(&prs);
+}
+
+Datum
+jsonb_to_tsvector_byid(PG_FUNCTION_ARGS)
+{
+ Oid cfgId = PG_GETARG_OID(0);
+ Jsonb *jb = PG_GETARG_JSONB_P(1);
+ TSVector result;
+
+ result = jsonb_to_tsvector_worker(cfgId, jb, false);
+ PG_FREE_IF_COPY(jb, 1);
PG_RETURN_TSVECTOR(result);
}
@@ -295,19 +308,48 @@ jsonb_to_tsvector(PG_FUNCTION_ARGS)
{
Jsonb *jb = PG_GETARG_JSONB_P(0);
Oid cfgId;
+ TSVector result;
cfgId = getTSCurrentConfig(true);
- PG_RETURN_DATUM(DirectFunctionCall2(jsonb_to_tsvector_byid,
- ObjectIdGetDatum(cfgId),
- JsonbPGetDatum(jb)));
+ result = jsonb_to_tsvector_worker(cfgId, jb, false);
+ PG_FREE_IF_COPY(jb, 1);
+
+ PG_RETURN_TSVECTOR(result);
}
Datum
-json_to_tsvector_byid(PG_FUNCTION_ARGS)
+jsonb_all_to_tsvector_byid(PG_FUNCTION_ARGS)
{
Oid cfgId = PG_GETARG_OID(0);
- text *json = PG_GETARG_TEXT_P(1);
+ Jsonb *jb = PG_GETARG_JSONB_P(1);
TSVector result;
+
+ result = jsonb_to_tsvector_worker(cfgId, jb, true);
+ PG_FREE_IF_COPY(jb, 1);
+
+ PG_RETURN_TSVECTOR(result);
+}
+
+Datum
+jsonb_all_to_tsvector(PG_FUNCTION_ARGS)
+{
+ Jsonb *jb = PG_GETARG_JSONB_P(0);
+ Oid cfgId;
+ TSVector result;
+
+ cfgId = getTSCurrentConfig(true);
+ result = jsonb_to_tsvector_worker(cfgId, jb, true);
+ PG_FREE_IF_COPY(jb, 1);
+
+ PG_RETURN_TSVECTOR(result);
+}
+
+/*
+ * Worker function for json(_all)_to_tsvector(_byid)
+ */
+static TSVector
+json_to_tsvector_worker(Oid cfgId, text *json, bool allTypes)
+{
TSVectorBuildState state;
ParsedText prs;
@@ -316,11 +358,20 @@ json_to_tsvector_byid(PG_FUNCTION_ARGS)
state.prs = &prs;
state.cfgId = cfgId;
- iterate_json_string_values(json, &state, add_to_tsvector);
+ iterate_json_values(json, allTypes, &state, add_to_tsvector);
- PG_FREE_IF_COPY(json, 1);
+ return make_tsvector(&prs);
+}
+
+Datum
+json_to_tsvector_byid(PG_FUNCTION_ARGS)
+{
+ Oid cfgId = PG_GETARG_OID(0);
+ text *json = PG_GETARG_TEXT_P(1);
+ TSVector result;
- result = make_tsvector(&prs);
+ result = json_to_tsvector_worker(cfgId, json, false);
+ PG_FREE_IF_COPY(json, 1);
PG_RETURN_TSVECTOR(result);
}
@@ -330,11 +381,40 @@ json_to_tsvec
Re: json(b)_to_tsvector with numeric values
> On 6 April 2018 at 16:25, Teodor Sigaev wrote:
> 1) I don't like jsonb_all_to_tsvector too.. What if we will accept new
> variant to index? Let me suggest:
>
> tsvector jsonb_to_tsvector([regclass,] jsonb, text[])
>
> where text[] arg is actually a flags, array contains any combination of
> literals 'numeric', 'string', 'boolean' (and even 'key' to index keys_ to
> point which types should be indexed. More than it, may be, it should a jsonb
> type for possible improvements in future. For now, it shouldbe a jsonb array
> type with string elements described above, example:
>
> select jsonb_to_tsvector('{"a": "aaa in bbb ddd ccc", "b":123}',
> '["numeric", "boolean"]');
>
>
> Form jsonb_to_tsvector('...', '["string"]) is effectively the same as
> current to_tsvector(jsonb)
Thank you for the suggestion, this sounds appealing. But I have two questions:
* why it should be a jsonb array, not a regular array?
* it would introduce the idea of jsonb element type expressed in text format,
so "string", "numeric", "boolean" etc - are there any consequences of that?
As far as I understand so far there was only jsonb_typeof.
> 2) Now it fails, and I see something strange in resuling tsvector
Oh, sorry, stupid copy-paste mistake in the condition. Just for the records,
I've attached fixed version of the previous patch (without any changes about an
array instead of a boolean flag).
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 5abb1c4..895b60a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -9696,6 +9696,18 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple
+ json(b)_all_to_tsvector( config regconfig , document json(b))
+
+tsvector
+
+ reduce each string, numeric or boolean value in the document to a tsvector,
+ and then concatenate those in document order to produce a single tsvector
+
+json_all_to_tsvector('english', '{"a": "The Fat Rats", "b": 123}'::json)
+'123':5 'fat':2 'rat':3
+
+
+
ts_delete
diff --git a/src/backend/tsearch/to_tsany.c b/src/backend/tsearch/to_tsany.c
index ea5947a..02c2b00 100644
--- a/src/backend/tsearch/to_tsany.c
+++ b/src/backend/tsearch/to_tsany.c
@@ -267,12 +267,12 @@ to_tsvector(PG_FUNCTION_ARGS)
PointerGetDatum(in)));
}
-Datum
-jsonb_to_tsvector_byid(PG_FUNCTION_ARGS)
+/*
+ * Worker function for jsonb(_all)_to_tsvector(_byid)
+ */
+static TSVector
+jsonb_to_tsvector_worker(Oid cfgId, Jsonb *jb, bool allTypes)
{
- Oid cfgId = PG_GETARG_OID(0);
- Jsonb *jb = PG_GETARG_JSONB_P(1);
- TSVector result;
TSVectorBuildState state;
ParsedText prs;
@@ -281,11 +281,24 @@ jsonb_to_tsvector_byid(PG_FUNCTION_ARGS)
state.prs = &prs;
state.cfgId = cfgId;
- iterate_jsonb_string_values(jb, &state, add_to_tsvector);
+ if (allTypes)
+ iterate_jsonb_all_values(jb, &state, add_to_tsvector);
+ else
+ iterate_jsonb_string_values(jb, &state, add_to_tsvector);
- PG_FREE_IF_COPY(jb, 1);
- result = make_tsvector(&prs);
+ return make_tsvector(&prs);
+}
+
+Datum
+jsonb_to_tsvector_byid(PG_FUNCTION_ARGS)
+{
+ Oid cfgId = PG_GETARG_OID(0);
+ Jsonb *jb = PG_GETARG_JSONB_P(1);
+ TSVector result;
+
+ result = jsonb_to_tsvector_worker(cfgId, jb, false);
+ PG_FREE_IF_COPY(jb, 1);
PG_RETURN_TSVECTOR(result);
}
@@ -295,19 +308,48 @@ jsonb_to_tsvector(PG_FUNCTION_ARGS)
{
Jsonb *jb = PG_GETARG_JSONB_P(0);
Oid cfgId;
+ TSVector result;
cfgId = getTSCurrentConfig(true);
- PG_RETURN_DATUM(DirectFunctionCall2(jsonb_to_tsvector_byid,
- ObjectIdGetDatum(cfgId),
- JsonbPGetDatum(jb)));
+ result = jsonb_to_tsvector_worker(cfgId, jb, false);
+ PG_FREE_IF_COPY(jb, 1);
+
+ PG_RETURN_TSVECTOR(result);
}
Datum
-json_to_tsvector_byid(PG_FUNCTION_ARGS)
+jsonb_all_to_tsvector_byid(PG_FUNCTION_ARGS)
{
Oid cfgId = PG_GETARG_OID(0);
- text *json = PG_GETARG_TEXT_P(1);
+ Jsonb *jb = PG_GETARG_JSONB_P(1);
TSVector result;
+
+ result = jsonb_to_tsvector_worker(cfgId, jb, true);
+ PG_FREE_IF_COPY(jb, 1);
+
+ PG_RETURN_TSVECTOR(result);
+}
+
+Datum
+jsonb_all_to_tsvector(PG_FUNCTION_ARGS)
+{
+ Jsonb *jb = PG_GETARG_JSONB_P(0);
+ Oid cfgId;
+ TSVector result;
+
+ cfgId = getTSCurrentConfig(true);
+ result = jsonb_to_tsvector_worker(cfgId, jb, true);
+ PG_FREE_IF_COPY(jb, 1);
+
+ PG_RETURN_TSVECTOR(result);
+}
+
+/*
+ * Worker function for json(_all)_to_tsvector(_byid)
+ */
+static TSVector
+json_to_tsvector_worker(Oid cfgId, text *json, bool allTypes)
+{
TSVectorBuildState state;
ParsedText prs;
@@ -316,11 +358,20 @@ json_to_tsvector_byid(PG_FUNCTION_ARGS)
state.prs = &prs;
state.cfgId = cfgId;
- iterate_json_string_values(json, &state, add_to_tsvector);
+ iterate_json_values(json, allTypes, &state, add_to_tsvector);
- PG_FREE_IF_COPY(json, 1);
+ return make_tsvector(&prs)
Re: json(b)_to_tsvector with numeric values
> On 6 April 2018 at 18:55, Teodor Sigaev wrote:
>
>
> Dmitry Dolgov wrote:
>>>
>>> On 6 April 2018 at 16:25, Teodor Sigaev wrote:
>>> 1) I don't like jsonb_all_to_tsvector too.. What if we will accept new
>>> variant to index? Let me suggest:
>>>
>>> tsvector jsonb_to_tsvector([regclass,] jsonb, text[])
>>>
>>> where text[] arg is actually a flags, array contains any combination of
>>> literals 'numeric', 'string', 'boolean' (and even 'key' to index keys_ to
>>> point which types should be indexed. More than it, may be, it should a
>>> jsonb
>>> type for possible improvements in future. For now, it shouldbe a jsonb
>>> array
>>> type with string elements described above, example:
>>>
>>> select jsonb_to_tsvector('{"a": "aaa in bbb ddd ccc", "b":123}',
>>> '["numeric", "boolean"]');
>>>
>>>
>>> Form jsonb_to_tsvector('...', '["string"]) is effectively the same as
>>> current to_tsvector(jsonb)
>>
>>
>> Thank you for the suggestion, this sounds appealing. But I have two
>> questions:
>>
>> * why it should be a jsonb array, not a regular array?
>
> To simplify extension of this array in future, for example to add limitation
> of recursion level in converted jsonb, etc.
>
>
>>
>> * it would introduce the idea of jsonb element type expressed in text
>> format,
>>so "string", "numeric", "boolean" etc - are there any consequences of
>> that?
>>As far as I understand so far there was only jsonb_typeof.
>
> Good catch, jsonb_typeof strings are okay: "string", "number", "boolean"
> also "all", "key", "value"
>
> See workable sketch for parsing jsonb flags and new worker variant.
Yep, thanks for the sketch. Here is the new version of patch, does it look
close to what you have in mind?
jsonb_to_tsvector_numeric_v4.patch
Description: Binary data
Re: json(b)_to_tsvector with numeric values
> On 7 April 2018 at 17:09, Teodor Sigaev wrote: >>> See workable sketch for parsing jsonb flags and new worker variant. >> >> >> Yep, thanks for the sketch. Here is the new version of patch, does it look >> close to what you have in mind? > > > Patch looks good except error messaging, you took it directly from sketch > where I didn't spend time for it. Please, improve. elog() should be used > only for impossible error, whereas user input could contins mistakes. I assume what you mean is that for user input errors we need to use ereport. Indeed, thanks for noticing. I've replaced all elog except the last one, since it actually describes an impossible situation, when we started to read an array, but ended up having something else instead WJB_END_ARRAY. jsonb_to_tsvector_numeric_v5.patch Description: Binary data
Re: Index Skip Scan
> On Mon, Sep 2, 2019 at 3:28 PM Dmitry Dolgov <9erthali...@gmail.com> wrote: > > > On Wed, Aug 28, 2019 at 9:32 PM Floris Van Nee > > wrote: > > > > I'm afraid I did manage to find another incorrect query result though > > Yes, it's an example of what I was mentioning before, that the current > modified > implementation of `_bt_readpage` wouldn't work well in case of going between > pages. So far it seems that the only problem we can have is when previous and > next items located on a different pages. I've checked how this issue can be > avoided, I hope I will post a new version relatively soon. Here is the version in which stepping between the pages works better. It seems sufficient to fix the case you've mentioned before, but for that we need to propagate keepPrev logic through `_bt_steppage` & `_bt_readnextpage`, and I can't say I like this solution. I have an idea that maybe it would be simpler to teach the code after index_skip to not do `_bt_next` right after one skip happened before. It should immediately elliminate several hacks from index skip itself, so I'll try to pursue this idea. > On Wed, Sep 4, 2019 at 10:45 PM Alvaro Herrera > wrote: Thank you for checking it out! > Surely it isn't right to add members prefixed with "ioss_" to > struct IndexScanState. Yeah, sorry. I've incorporated IndexScan support originally only to show that it's possible (with some limitations), but after that forgot to clean up. Now those fields are renamed. > I'm surprised about this "FirstTupleEmitted" business. Wouldn't it make > more sense to implement index_skip() to return the first tuple if the > scan is just starting? (I know little about executor, apologies if this > is a stupid question.) I'm not entirely sure, which exactly part do you mean? Now the first tuple is returned by `_bt_first`, how would it help if index_skip will return it? > It would be good to get more knowledgeable people to review this patch. > It's clearly something we want, yet it's been there for a very long > time. Sure, that would be nice. v25-0001-Index-skip-scan.patch Description: Binary data
Re: [HACKERS] [PATCH] Generic type subscripting
> On Thu, Sep 12, 2019 at 3:58 AM Alvaro Herrera > wrote: > Can you please send an updated version? Sure, I'll send it in a few days.
