[HACKERS] Is the unfair lwlock behavior intended?
Hello, I encountered a strange behavior of lightweight lock in PostgreSQL 9.2. That appears to apply to 9.6, too, as far as I examine the code. Could you tell me if the behavior is intended or needs fix? Simply put, the unfair behavior is that waiters for exclusive mode are overtaken by share-mode lockers who arrive later. PROBLEM Under a heavy read/write workload on a big machine with dozens of CPUs and hundreds of GBs of RAM, psql sometimes took more than 30 seconds to connect to the database (and actually, it failed to connect due to our connect_timeout setting.) The backend corresponding to the psql was waiting to acquire exclusive mode lock on ProcArrayLock. Some other backends took more than 10 seconds to commit their transactions, waiting for exclusive mode lock on ProcArrayLock. At that time, many backend processes (I forgot the number) were acquiring and releasing share mode lock on ProcArrayLock, most of which were from TransactionIsInProgress(). CAUSE Going into the 9.2 code, I realized that those who request share mode don't pay attention to the wait queue. That is, if some processes hold share mode lock and someone is waiting for exclusive mode in the wait queue, other processes who come later can get share mode overtaking those who are already waiting. If many processes repeatedly request share mode, the waiters can't get exclusive mode for a long time. Is this intentional, or should we make the later share-lockers if someone is in the wait queue? Regards Takayuki Tsunakawa -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LSN as a recovery target
On Mon, May 23, 2016 at 6:25 PM, Craig Ringer wrote: > On 24 May 2016 at 09:12, Michael Paquier wrote: >> >> Hi all, >> >> Today somebody has pointed me out that it could be interesting to be >> able to recovery up to a given LSN position. One argument behind that >> was to allow a maximum of things to recover up to the point where a >> relation block got corrupted by a specific record because of a broken >> segment. So that would be simply having recovery_target_lsn in >> recovery.conf similar to what we have now. >> Thoughts? >> > > Sounds useful, if somewhat niche. I've needed that in the past, and just > zeroed the WAL segment after the end of the target record. Not super > user-friendly. Yeah, that's really something that covers only a narrow case, though if we don't have it when we need it we're limited to some hacks. Perhaps people who have the advanced level to use such a thing have the level to use hacks anyway.. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Batch/pipelining support for libpq
Craig Ringer writes:
> On 24 May 2016 at 00:00, Michael Paquier wrote:
>> Did you consider the use of simple_list.c instead of introducing a new
>> mimic as PGcommandQueueEntry? It would be cool avoiding adding new
>> list emulations on frontends.
> I'd have to extend simple_list to add a generic object version, like
> struct my_list_elem
> {
> PG_SIMPLE_LIST_ATTRS;
> mytype mycol;
> myothertype myothercol;
> }
> Objections?
That doesn't look exactly "generic".
> I could add a void* version that's a simple clone of the string version,
> but having to malloc both a list cell and its contents separately is
> annoying.
I'd be okay with a void* version, but I'm not sure about this.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Batch/pipelining support for libpq
On 24 May 2016 at 00:00, Michael Paquier wrote:
>
> Did you consider the use of simple_list.c instead of introducing a new
> mimic as PGcommandQueueEntry? It would be cool avoiding adding new
> list emulations on frontends.
>
I'd have to extend simple_list to add a generic object version, like
struct my_list_elem
{
PG_SIMPLE_LIST_ATTRS;
mytype mycol;
myothertype myothercol;
}
Objections?
I could add a void* version that's a simple clone of the string version,
but having to malloc both a list cell and its contents separately is
annoying.
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] BTREE_BUILD_STATS build is broken
On Mon, May 23, 2016 at 8:09 PM, Tom Lane wrote: > Does it appear to compile without that? It does. The only thing that's absent is the pgrminclude directive, which is of course just a C comment. > (More generally, is there a better answer for that problem?) My unpublished parallel B-Tree index build patch will move everything to do with index builds into nbtsort.c. So, I will more than likely eventually propose that everything in question live there. I think that's a better approach in general, because the current high-level coordination from nbtree.c (e.g. how spools are initialized there) seems a little contrived. A single entry point for nbtsort.c works better. Short term, I guess the best solution is to just have a pgrminclude directive in both files. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Inheritance
On 24 May 2016 at 00:05, Merlin Moncure wrote: > > This feature was very much a product of the time, at the height of the > "Object Relational" fad. The trend for postgres has been in the exact > opposite direction, towards the SQL standard. Further complicating > matters, inheritance has been repurposed to be the foundation for > table partitioning, making heavy changes problematic. > Indeed. I find it notable that no popular ORM has bothered adopting PostgreSQL's inheritance features, and instead just use big left joins or repeated SELECTs to implement parent/child relationships, with foreign keys enforcing constraints. I consider inheritance mostly useless without the ability to have UNIQUE indexes that span a parent relation and all its children. You can use them for partitioning only by sacrificing a bunch of integrity protection or creating messy chains of FKs between individual partitions. I'd rather like to quietly deprecate inheritance and eventually remove it once we have real partitioning and some time has passed... -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] BTREE_BUILD_STATS build is broken
Peter Geoghegan writes: > If BTREE_BUILD_STATS needs a "tcopprot.h pgrminclude ignore" within > nbtree.c, then ISTM that the similar include directive within > nbtsort.c ought to receive the same treatment. Does it appear to compile without that? (More generally, is there a better answer for that problem?) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Batch/pipelining support for libpq
On 24 May 2016 at 00:00, Michael Paquier wrote: > On Mon, May 23, 2016 at 8:50 AM, Andres Freund wrote: > > > yay^2. > > I'll follow this mood. Yeha. BTW, I've publushed the HTML-ified SGML docs to http://2ndquadrant.github.io/postgres/libpq-batch-mode.html as a preview. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] Typo in 001_initdb.pl
* Michael Paquier (michael.paqu...@gmail.com) wrote: > On Mon, May 23, 2016 at 5:05 PM, Tom Lane wrote: > > Michael Paquier writes: > >> I just bumped into the following typo for $subject: > >> - 'role names cannot being with "pg_"'); > >> + 'role names cannot begin with "pg_"'); > > > > Pushed, thanks. > > Thanks. Thanks from me also, I had been planning to fix this, but got caught up with something else this evening. Stephen signature.asc Description: Digital signature
Re: [HACKERS] LSN as a recovery target
On 24 May 2016 at 09:12, Michael Paquier wrote: > Hi all, > > Today somebody has pointed me out that it could be interesting to be > able to recovery up to a given LSN position. One argument behind that > was to allow a maximum of things to recover up to the point where a > relation block got corrupted by a specific record because of a broken > segment. So that would be simply having recovery_target_lsn in > recovery.conf similar to what we have now. > Thoughts? > > Sounds useful, if somewhat niche. I've needed that in the past, and just zeroed the WAL segment after the end of the target record. Not super user-friendly. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] PATCH: Batch/pipelining support for libpq
On 24 May 2016 at 00:00, Michael Paquier wrote: > On Mon, May 23, 2016 at 8:50 AM, Andres Freund > wrote: > >> This should be very useful for optimising FDWs, Postgres-XC, etc. > > > > And optimizing normal clients. > > > > Not easy, but I'd be very curious how much psql's performance improves > > when replaying a .sql style dump, and replaying from a !tty fd, after > > hacking it up to use the batch API. > I didn't, but agree it'd be interesting. So would pg_restore, for that matter, though its use of COPY for the bulk of its work means it wouldn't make tons of difference. I think it'd be safe to enable it automatically in psql's --single-transaction mode. It's also safe to send anything after an explicit BEGIN and until the next COMMIT as a batch from libpq, and since it parses the SQL enough to detect statement boundaries already that shouldn't be too hard to handle. However: psql is synchronous, using the PQexec family of blocking calls. It's all fairly well contained in SendQuery and PSQLexec, but making it use batching still require restructuring those to use the asynchronous nonblocking API and append the query to a pending-list, plus the addition of a select() loop to handle results and dispatch more work. MainLoop() isn't structured around a select or poll, it loops over gets. So while it'd be interesting to see what difference batching made the changes to make psql use it would be a bit more invasive. Far from a rewrite, but to avoid lots of code duplication it'd have to change everything to use nonblocking mode and a select loop, which is a big change for such a core tool. This is a bit of a side-project and I've got to get back to "real work" so I don't expect to do a proper patch for psql any time soon. I'd rather not try to build too much on this until it's seen some review and I know the API won't need a drastic rewrite anyway. I'll see if I can do a hacked-up version one evening to see what it does for performance though. Did you consider the use of simple_list.c instead of introducing a new > mimic as PGcommandQueueEntry? It would be cool avoiding adding new > list emulations on frontends. > Nope. I didn't realise it was there; I've done very little on the C client and library side so far. So thanks, I'll update it accordingly. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] Typo in 001_initdb.pl
On Mon, May 23, 2016 at 5:05 PM, Tom Lane wrote: > Michael Paquier writes: >> I just bumped into the following typo for $subject: >> - 'role names cannot being with "pg_"'); >> + 'role names cannot begin with "pg_"'); > > Pushed, thanks. Thanks. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] LSN as a recovery target
Hi all, Today somebody has pointed me out that it could be interesting to be able to recovery up to a given LSN position. One argument behind that was to allow a maximum of things to recover up to the point where a relation block got corrupted by a specific record because of a broken segment. So that would be simply having recovery_target_lsn in recovery.conf similar to what we have now. Thoughts? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BTREE_BUILD_STATS build is broken
On Mon, May 23, 2016 at 4:41 PM, Tom Lane wrote: > Pushed, thanks. Thanks. If BTREE_BUILD_STATS needs a "tcopprot.h pgrminclude ignore" within nbtree.c, then ISTM that the similar include directive within nbtsort.c ought to receive the same treatment. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo in 001_initdb.pl
Michael Paquier writes: > I just bumped into the following typo for $subject: > - 'role names cannot being with "pg_"'); > + 'role names cannot begin with "pg_"'); Pushed, thanks. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] BTREE_BUILD_STATS build is broken
Peter Geoghegan writes: > The attached patch fixes the BTREE_BUILD_STATS build. Looks like > 65c5fcd353a859da9e61bfb2b92a99f12937de3b broke this. That commit was > made back in January, so no backpatch is required. Pushed, thanks. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
On Mon, May 23, 2016 at 4:27 PM, Tom Lane wrote: >> You also have to be aware of the new thing, which a lot of hackers >> will not be, if awareness of COPY_PARSE_PLAN_TREES is anything to go >> by. > > I doubt that anybody ever enables that in manual builds anyway. > The important thing is to have at least one buildfarm animal using it, > which there soon will be. I manually enable it sometimes. And, I've pointed out bugs that that would have caught to other hackers during review a couple of times. That's my view. I'm not going to make a fuss about it. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
Peter Geoghegan writes: > On Mon, May 23, 2016 at 4:11 PM, Tom Lane wrote: >> And after further thought, I decided that that was penny-wise and >> pound-foolish; it's more readable if the #define is just an independent >> pg_config_manual.h entry. The only work it'd save is the need to update >> a buildfarm animal or two to add the new #define, which is not exactly >> a huge cost. > You also have to be aware of the new thing, which a lot of hackers > will not be, if awareness of COPY_PARSE_PLAN_TREES is anything to go > by. I doubt that anybody ever enables that in manual builds anyway. The important thing is to have at least one buildfarm animal using it, which there soon will be. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
On Mon, May 23, 2016 at 4:11 PM, Tom Lane wrote: > And after further thought, I decided that that was penny-wise and > pound-foolish; it's more readable if the #define is just an independent > pg_config_manual.h entry. The only work it'd save is the need to update > a buildfarm animal or two to add the new #define, which is not exactly > a huge cost. You also have to be aware of the new thing, which a lot of hackers will not be, if awareness of COPY_PARSE_PLAN_TREES is anything to go by. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
I wrote: > Peter Geoghegan writes: >>> If that sounds like a plausible choke-point, the next question is what >>> to use to enable the debug test. I propose "#ifdef COPY_PARSE_PLAN_TREES" >>> since that enables similar sanity checking for other parts of >>> backend/nodes/, and we do have at least one buildfarm member using it. >> That's what I was thinking, too. No need to keep it separate. > After cogitating, I did it as attached just for readability's sake. And after further thought, I decided that that was penny-wise and pound-foolish; it's more readable if the #define is just an independent pg_config_manual.h entry. The only work it'd save is the need to update a buildfarm animal or two to add the new #define, which is not exactly a huge cost. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Inheritance
Tom Lane wrote: > My feeling about it is that we need to provide a partitioning feature > that doesn't rely on the current notion of inheritance at all. We've > heard from multiple users who want to use large numbers of partitions, > enough that simply having a separate relcache entry for each partition > would be a performance problem, never mind the current approach to > planning queries over inheritance trees. So the partitions need to be > objects much simpler than full-fledged tables. Sorry to hijack the thread, but I agree on this, and I'm worried that the patch being floated for partitioning may paint us on a corner from which it may be difficult to get out. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Inheritance
Joe Conway writes: > But then again, maybe we need to start with a clear notion of what > problems people are trying to solve when they use partitions. At least > some of the historic reasons are no longer valid. That's true. Just because people want to have a gazillion partitions doesn't necessarily mean it's a good design that we need to support well. Some investigation would be a smart use of time. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Inheritance
On 05/23/2016 03:05 PM, Tom Lane wrote: > Jim Nasby writes: >> I don't see why partitioning complicates fixing these issues. ISTM it's >> the exact same complaint for both inheritance and partitioning. > > My feeling about it is that we need to provide a partitioning feature > that doesn't rely on the current notion of inheritance at all. We've > heard from multiple users who want to use large numbers of partitions, > enough that simply having a separate relcache entry for each partition > would be a performance problem, never mind the current approach to > planning queries over inheritance trees. So the partitions need to be > objects much simpler than full-fledged tables. I wonder if it wouldn't make sense to define a partition as a list of segments within a single table that represent the partition? But then again, maybe we need to start with a clear notion of what problems people are trying to solve when they use partitions. At least some of the historic reasons are no longer valid. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Inheritance
Jim Nasby writes: > On 5/23/16 11:05 AM, Merlin Moncure wrote: >> This feature was very much a product of the time, at the height of the >> "Object Relational" fad. The trend for postgres has been in the exact >> opposite direction, towards the SQL standard. Further complicating >> matters, inheritance has been repurposed to be the foundation for >> table partitioning, making heavy changes problematic. > I don't see why partitioning complicates fixing these issues. ISTM it's > the exact same complaint for both inheritance and partitioning. My feeling about it is that we need to provide a partitioning feature that doesn't rely on the current notion of inheritance at all. We've heard from multiple users who want to use large numbers of partitions, enough that simply having a separate relcache entry for each partition would be a performance problem, never mind the current approach to planning queries over inheritance trees. So the partitions need to be objects much simpler than full-fledged tables. If we had that, and encouraged people to migrate simple partitioning use-cases to it, that might take off enough pressure that we could afford to consider more-complicated inheritance schemes rather than treating inheritance as an unfortunate legacy design. But we're some years away from being able to do that. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On 05/23/2016 02:37 PM, David G. Johnston wrote: > But then I don't get Joe's point - if its an implementation detail why > should it matter if rewriting the SRF-in-tlist to be laterals changes > execution from a serial to an interleaved implementation. Plus, Joe's > claim: "the capability to pipeline results is still only available in > the target list", and yours above are at odds since you claim the > rewritten behavior is the same today. Is there a disconnect in > knowledge or are you talking about different things? Unless there have been recent changes which I missed, ValuePerCall SRFs are still run to completion in one go, when executed in the FROM clause, but they project one-row-at-a-time in the target list. If your SRF returns many-many rows, the problem with the former case is that the entire thing has to be materialized in memory. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Inheritance
On 5/23/16 11:05 AM, Merlin Moncure wrote: Postgres doesn't work that way, and the documentation disclaims this: "Note: Although inheritance is frequently useful, it has not been integrated with unique constraints or foreign keys, which limits its usefulness. See Section 5.8 for more detail." Personally, I don't think this will ever be fixed. The reason why it doesn't work is due to some foundational implementation decisions that would have to be revisited. If the complaint is really about FKs/UNIQUE (and really AFAIK it's only UNIQUE that's the problem), then I agree: it should be addressed. It's a major impediment to partitioning (and generic inheritance). This feature was very much a product of the time, at the height of the "Object Relational" fad. The trend for postgres has been in the exact opposite direction, towards the SQL standard. Further complicating matters, inheritance has been repurposed to be the foundation for table partitioning, making heavy changes problematic. I don't see why partitioning complicates fixing these issues. ISTM it's the exact same complaint for both inheritance and partitioning. I also disagree about PK:PK FK's between a bunch of completely independent tables being a good way to model this stuff. It doubles the complexity of every query against a child table and doesn't perform nearly as well, because your data locality goes down the tubes. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
Peter Geoghegan writes:
> What I meant is that I think naively adding the choke-point for a
> top-level SelectStmt would do the job (although perhaps we could do
> better). I wasn't suggesting that we'd avoid recursing from there;
> only that we could reasonably avoid recursing from someplace else
> (e.g. InsertStmt) in the hope of finding a SelectStmt to test.
> Offhand, I don't imagine that that would offer better coverage.
I think it would. The attached patch shows nearly 150 failures in
the current regression tests. If I remove "case T_InsertStmt" that
drops to two failures: there are two cases in with.sgml that almost
manage to exercise the bug, but not quite because they are not
WITH RECURSIVE. That doesn't leave me with any warm feeling about
being able to find other similar oversights if we apply this testing
only to top-level SelectStmts.
>> If that sounds like a plausible choke-point, the next question is what
>> to use to enable the debug test. I propose "#ifdef COPY_PARSE_PLAN_TREES"
>> since that enables similar sanity checking for other parts of
>> backend/nodes/, and we do have at least one buildfarm member using it.
> That's what I was thinking, too. No need to keep it separate.
After cogitating, I did it as attached just for readability's sake.
regards, tom lane
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 7bc5be1..92f3276 100644
*** a/src/backend/nodes/nodeFuncs.c
--- b/src/backend/nodes/nodeFuncs.c
*** query_or_expression_tree_mutator(Node *n
*** 2991,2998
* Unlike expression_tree_walker, there is no special rule about query
* boundaries: we descend to everything that's possibly interesting.
*
! * Currently, the node type coverage extends to SelectStmt and everything
! * that could appear under it, but not other statement types.
*/
bool
raw_expression_tree_walker(Node *node,
--- 2991,3000
* Unlike expression_tree_walker, there is no special rule about query
* boundaries: we descend to everything that's possibly interesting.
*
! * Currently, the node type coverage here extends only to DML statements
! * (SELECT/INSERT/UPDATE/DELETE) and nodes that can appear in them, because
! * this is used mainly during analysis of CTEs, and only DML statements can
! * appear in CTEs.
*/
bool
raw_expression_tree_walker(Node *node,
diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c
index 7d2fedf..5979e76 100644
*** a/src/backend/parser/analyze.c
--- b/src/backend/parser/analyze.c
***
*** 45,50
--- 45,55
#include "utils/rel.h"
+ /* Use RAW_EXPRESSION_COVERAGE_TEST in COPY_PARSE_PLAN_TREES builds */
+ #ifdef COPY_PARSE_PLAN_TREES
+ #define RAW_EXPRESSION_COVERAGE_TEST
+ #endif
+
/* Hook for plugins to get control at end of parse analysis */
post_parse_analyze_hook_type post_parse_analyze_hook = NULL;
*** static Query *transformCreateTableAsStmt
*** 74,79
--- 79,87
CreateTableAsStmt *stmt);
static void transformLockingClause(ParseState *pstate, Query *qry,
LockingClause *lc, bool pushedDown);
+ #ifdef RAW_EXPRESSION_COVERAGE_TEST
+ static bool test_raw_expression_coverage(Node *node, void *context);
+ #endif
/*
*** transformStmt(ParseState *pstate, Node *
*** 220,225
--- 228,252
{
Query *result;
+ /*
+ * We apply RAW_EXPRESSION_COVERAGE_TEST testing to basic DML statements;
+ * we can't just run it on everything because raw_expression_tree_walker()
+ * doesn't claim to handle utility statements.
+ */
+ #ifdef RAW_EXPRESSION_COVERAGE_TEST
+ switch (nodeTag(parseTree))
+ {
+ case T_SelectStmt:
+ case T_InsertStmt:
+ case T_UpdateStmt:
+ case T_DeleteStmt:
+ (void) test_raw_expression_coverage(parseTree, NULL);
+ break;
+ default:
+ break;
+ }
+ #endif /* RAW_EXPRESSION_COVERAGE_TEST */
+
switch (nodeTag(parseTree))
{
/*
*** applyLockingClause(Query *qry, Index rti
*** 2713,2715
--- 2740,2764
rc->pushedDown = pushedDown;
qry->rowMarks = lappend(qry->rowMarks, rc);
}
+
+ /*
+ * Coverage testing for raw_expression_tree_walker().
+ *
+ * When enabled, we run raw_expression_tree_walker() over every DML statement
+ * submitted to parse analysis. Without this provision, that function is only
+ * applied in limited cases involving CTEs, and we don't really want to have
+ * to test everything inside as well as outside a CTE.
+ */
+ #ifdef RAW_EXPRESSION_COVERAGE_TEST
+
+ static bool
+ test_raw_expression_coverage(Node *node, void *context)
+ {
+ if (node == NULL)
+ return false;
+ return raw_expression_tree_walker(node,
+ test_raw_expression_coverage,
+ context);
+ }
+
+ #endif /* RAW_EXPRESSION_COVERAGE_TEST */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgre
Re: [HACKERS] Calling json_* functions with JSONB data
On Mon, May 23, 2016 at 5:38 PM, Jim Nasby wrote: > On 5/23/16 11:55 AM, Peter van Hardenberg wrote: > >> Fortunately, this seems quite easy to resolve by taking advantage of our >> ability to add json_*(jsonb) form of the functions. >> > > Another issue no one has mentioned is functions that return JSON/JSONB. > IMO those should NOT be overloaded, because that will make it very easy to > accidentally change from one type to the other without meaning to. Actually, by definition they cannot be overloaded. A function's signature is derived from its input types only. http://www.postgresql.org/docs/devel/static/sql-createfunction.html """ The name of the new function must not match any existing function with the same input argument types in the same schema. However, functions of different argument types can share a name (this is called overloading). """ Admittedly the absence of "output" is not emphasized but overloading in (most?) languages (small sample size for personal knowledge) is subject to the same restriction. David J.
Re: [HACKERS] Calling json_* functions with JSONB data
On 5/23/16 11:55 AM, Peter van Hardenberg wrote: Fortunately, this seems quite easy to resolve by taking advantage of our ability to add json_*(jsonb) form of the functions. Another issue no one has mentioned is functions that return JSON/JSONB. IMO those should NOT be overloaded, because that will make it very easy to accidentally change from one type to the other without meaning to. -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 4:42 PM, Tom Lane wrote: > "David G. Johnston" writes: > > On Mon, May 23, 2016 at 4:24 PM, Alvaro Herrera < > alvhe...@2ndquadrant.com> > > wrote: > >> Ah, so that's what "pipeline results" mean! I hadn't gotten that. I > >> agree; Abhijit had a patch or a plan for this, a long time ago ... > > > Is this sidebar strictly an implementation detail, not user visible? > > Hmm. It could be visible in the sense that the execution of multiple > functions in one ROWS FROM() construct could be interleaved, while > (I think) the current implementation runs each one to completion > serially. But if you're writing code that assumes that, I think you > should not be very surprised when we break it. In any case, that > would not affect the proposed translation for SRFs-in-tlist, since > those have that behavior today. > Thanks Sounds like "zipper results" would be a better term for it...but, yes, if that's the general context it falls into implementation from my perspective. But then I don't get Joe's point - if its an implementation detail why should it matter if rewriting the SRF-in-tlist to be laterals changes execution from a serial to an interleaved implementation. Plus, Joe's claim: "the capability to pipeline results is still only available in the target list", and yours above are at odds since you claim the rewritten behavior is the same today. Is there a disconnect in knowledge or are you talking about different things? David J.
Re: [HACKERS] Calling json_* functions with JSONB data
On Mon, May 23, 2016 at 4:37 PM, Tom Lane wrote:
> Peter van Hardenberg writes:
> > Great question, Marko. If you can point me towards an example I'll take a
> > look, but I'll proceed with the current understanding and suggestions and
> > see what people have to say.
>
> I believe Marko's just complaining about the case for unknown-type
> arguments, for example:
>
> regression=# select json_array_length('[1,2,3]');
> json_array_length
> ---
> 3
> (1 row)
>
> The parser has no trouble resolving this because there is only one
> json_array_length(); but if there were two, it would fail to make a
> determination of which one you meant.
>
> AFAICS the only way to fix that would be to introduce some preference
> between the two types. For example, we could move both 'json' and 'jsonb'
> into their own typcategory ('J' is unused...) and then mark 'jsonb' as
> the preferred type in that category. This would require a fair amount of
> experimentation to determine if it upsets any cases that work conveniently
> today; but right offhand I don't see any fatal problems with such an idea.
>
> regards, tom lane
>
I
guess the relevant point in the documentation is the parenthetical
sentence:
(The processing functions consider the last value as the operative one.)
http://www.postgresql.org/docs/9.5/static/datatype-json.html
Which normalizes the behaviors of jsonb and json as they pass through one
of these functions. Though only the multi-key is noted which means
white-space (immaterial) and key-order (potentially material) behaviors
differ; though the later coming through the function unscathed is not
something that user's should be relying upon. Specifically I'm thinking of
the behavior that "json_each(...)" would exhibit.
David J.
Re: [HACKERS] Calling json_* functions with JSONB data
I'll look into it, thanks for the explanation.
On Mon, May 23, 2016 at 1:37 PM, Tom Lane wrote:
> Peter van Hardenberg writes:
> > Great question, Marko. If you can point me towards an example I'll take a
> > look, but I'll proceed with the current understanding and suggestions and
> > see what people have to say.
>
> I believe Marko's just complaining about the case for unknown-type
> arguments, for example:
>
> regression=# select json_array_length('[1,2,3]');
> json_array_length
> ---
> 3
> (1 row)
>
> The parser has no trouble resolving this because there is only one
> json_array_length(); but if there were two, it would fail to make a
> determination of which one you meant.
>
> AFAICS the only way to fix that would be to introduce some preference
> between the two types. For example, we could move both 'json' and 'jsonb'
> into their own typcategory ('J' is unused...) and then mark 'jsonb' as
> the preferred type in that category. This would require a fair amount of
> experimentation to determine if it upsets any cases that work conveniently
> today; but right offhand I don't see any fatal problems with such an idea.
>
> regards, tom lane
>
--
Peter van Hardenberg
San Francisco, California
"Everything was beautiful, and nothing hurt."—Kurt Vonnegut
[HACKERS] BTREE_BUILD_STATS build is broken
The attached patch fixes the BTREE_BUILD_STATS build. Looks like 65c5fcd353a859da9e61bfb2b92a99f12937de3b broke this. That commit was made back in January, so no backpatch is required. -- Peter Geoghegan From 06d2150ff20dfa3e6fbc579a9e41ff3f6487 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Mon, 23 May 2016 13:55:34 -0700 Subject: [PATCH] Fix BTREE_BUILD_STATS build Commit 65c5fcd353a859da9e61bfb2b92a99f12937de3b broke this by removing a header include directive that is conditionally required. Add that back to nbtree.c. --- src/backend/access/nbtree/nbtree.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/backend/access/nbtree/nbtree.c b/src/backend/access/nbtree/nbtree.c index 013394c..ff6bedf 100644 --- a/src/backend/access/nbtree/nbtree.c +++ b/src/backend/access/nbtree/nbtree.c @@ -27,6 +27,7 @@ #include "storage/ipc.h" #include "storage/lmgr.h" #include "storage/smgr.h" +#include "tcop/tcopprot.h" #include "utils/index_selfuncs.h" #include "utils/memutils.h" -- 2.7.4 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
On Mon, May 23, 2016 at 1:55 PM, Tom Lane wrote: > Um, I think not --- you need the case cited by the OP, namely an INSERT > ON CONFLICT in a CTE in a SelectStmt. If we'd had any of those in the > regression tests, we'd have found the bug, but we don't; and it's not > an obvious combination to try. We would have found it if there were > a reason to run raw_expression_tree_walker() on bare InsertStmts, > but currently there is not. I don't follow. If we had coverage of that in the regression tests, then that would have shown the bug, of course. It would have shown the bug without any new debugging infrastructure being required (or being enabled). What I meant is that I think naively adding the choke-point for a top-level SelectStmt would do the job (although perhaps we could do better). I wasn't suggesting that we'd avoid recursing from there; only that we could reasonably avoid recursing from someplace else (e.g. InsertStmt) in the hope of finding a SelectStmt to test. Offhand, I don't imagine that that would offer better coverage. > If that sounds like a plausible choke-point, the next question is what > to use to enable the debug test. I propose "#ifdef COPY_PARSE_PLAN_TREES" > since that enables similar sanity checking for other parts of > backend/nodes/, and we do have at least one buildfarm member using it. That's what I was thinking, too. No need to keep it separate. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
Peter Geoghegan writes: > On Mon, May 23, 2016 at 12:48 PM, Tom Lane wrote: >> Also, related to this complaint though not this patch, it's disturbing >> that this oversight wasn't detected long ago. My first thought was to add >> some conditionally-compiled debugging code that just performs a no-op >> traverse of every raw parse tree produced by the grammar. However that >> doesn't work because raw_expression_tree_walker doesn't promise to support >> everything, only DML statements. Thoughts? > Why couldn't the debug code be executed conditionally, too? Since > raw_expression_tree_walker() promises to work for "SelectStmt and > everything that could appear under it", I don't think it would be > difficult to find a choke point for that. Perhaps there is some > subtlety I've missed, since what I propose seems too obvious. FWIW, I > don't think it would much matter if this debug code was not reliably > executed for every possible SelectStmt. Just limiting it to top-level > SelectStmts would have easily caught this bug. Um, I think not --- you need the case cited by the OP, namely an INSERT ON CONFLICT in a CTE in a SelectStmt. If we'd had any of those in the regression tests, we'd have found the bug, but we don't; and it's not an obvious combination to try. We would have found it if there were a reason to run raw_expression_tree_walker() on bare InsertStmts, but currently there is not. Possibly we could get adequate coverage by inserting the debug code into the first four switch cases in transformStmt(). If that sounds like a plausible choke-point, the next question is what to use to enable the debug test. I propose "#ifdef COPY_PARSE_PLAN_TREES" since that enables similar sanity checking for other parts of backend/nodes/, and we do have at least one buildfarm member using it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
On Mon, May 23, 2016 at 12:48 PM, Tom Lane wrote: >> I saw no reason to avoid the extra cycles. A noticeable omission has a >> cost: it gets noticed. Given this code path is likely to hardly ever >> be hit, this mechanical approach seemed best. That's all. > > I agree that performance isn't much of a concern, but code bloat and > inconsistency with other cases are valid concerns. That function does > not recurse into name lists in, for example, the A_Expr and FuncCall > cases. Okay. Noted. > Also, related to this complaint though not this patch, it's disturbing > that this oversight wasn't detected long ago. My first thought was to add > some conditionally-compiled debugging code that just performs a no-op > traverse of every raw parse tree produced by the grammar. However that > doesn't work because raw_expression_tree_walker doesn't promise to support > everything, only DML statements. Thoughts? Why couldn't the debug code be executed conditionally, too? Since raw_expression_tree_walker() promises to work for "SelectStmt and everything that could appear under it", I don't think it would be difficult to find a choke point for that. Perhaps there is some subtlety I've missed, since what I propose seems too obvious. FWIW, I don't think it would much matter if this debug code was not reliably executed for every possible SelectStmt. Just limiting it to top-level SelectStmts would have easily caught this bug. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
"David G. Johnston" writes: > On Mon, May 23, 2016 at 4:24 PM, Alvaro Herrera > wrote: >> Ah, so that's what "pipeline results" mean! I hadn't gotten that. I >> agree; Abhijit had a patch or a plan for this, a long time ago ... > âIs this sidebar strictly an implementation detail, not user visible? Hmm. It could be visible in the sense that the execution of multiple functions in one ROWS FROM() construct could be interleaved, while (I think) the current implementation runs each one to completion serially. But if you're writing code that assumes that, I think you should not be very surprised when we break it. In any case, that would not affect the proposed translation for SRFs-in-tlist, since those have that behavior today. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Calling json_* functions with JSONB data
Peter van Hardenberg writes:
> Great question, Marko. If you can point me towards an example I'll take a
> look, but I'll proceed with the current understanding and suggestions and
> see what people have to say.
I believe Marko's just complaining about the case for unknown-type
arguments, for example:
regression=# select json_array_length('[1,2,3]');
json_array_length
---
3
(1 row)
The parser has no trouble resolving this because there is only one
json_array_length(); but if there were two, it would fail to make a
determination of which one you meant.
AFAICS the only way to fix that would be to introduce some preference
between the two types. For example, we could move both 'json' and 'jsonb'
into their own typcategory ('J' is unused...) and then mark 'jsonb' as
the preferred type in that category. This would require a fair amount of
experimentation to determine if it upsets any cases that work conveniently
today; but right offhand I don't see any fatal problems with such an idea.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 4:24 PM, Alvaro Herrera wrote: > Tom Lane wrote: > > Joe Conway writes: > > > > I'll also note that, unless I missed something, we also have to > consider > > > that the capability to pipeline results is still only available in the > > > target list. > > > > Yes, we would definitely want to improve nodeFunctionscan.c to perform > > better for ValuePerCall SRFs. But that has value independently of this. > > Ah, so that's what "pipeline results" mean! I hadn't gotten that. I > agree; Abhijit had a patch or a plan for this, a long time ago ... > > Is this sidebar strictly an implementation detail, not user visible? David J.
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 4:15 PM, Tom Lane wrote: > Merlin Moncure writes: > > On Mon, May 23, 2016 at 2:13 PM, David Fetter wrote: > >> On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote: > >>> +1 on removing LCM. > > >> As a green field project, that would make total sense. As a thing > >> decades in, it's not clear to me that that would break less stuff or > >> break it worse than simply disallowing SRFs in the target list, which > >> has been rejected on bugward-compatibility grounds. I suspect it > >> would be even worse because disallowing SRFs in target lists would at > >> least be obvious and localized when it broke code. > > > If I'm reading this correctly, it sounds to me like you are making the > > case that removing target list SRF completely would somehow cause less > > breakage than say, rewriting it to a LATERAL based implementation for > > example. With more than a little forbearance, let's just say I don't > > agree. > > We should consider single and multiple SRFs in a targetlist as distinct > use-cases; only the latter has got weird properties. > > There are several things we could potentially do with multiple SRFs in > the same targetlist. In increasing order of backwards compatibility and > effort required: > > 1. Throw error if there's more than one SRF. > > 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...). This would > have the same behavior as before if the SRFs all return the same number > of rows, and otherwise would behave differently. > > 3. Rewrite into some other construct that still ends up as a FunctionScan > RTE node, but has the old LCM behavior if the SRFs produce different > numbers of rows. (Perhaps we would not need to expose this construct > as something directly SQL-visible.) > > It's certainly arguable that the common use-cases for SRF-in-tlist > don't have more than one SRF per tlist, and thus that implementing #1 > would be an appropriate amount of effort. It's worth noting also that > the LCM behavior has been repeatedly reported as a bug, and therefore > that if we do #3 we'll be expending very substantial effort to be > literally bug-compatible with ancient behavior that no one in the > current development group thinks is well-designed. As far as #2 goes, > it would have the advantage that code depending on the same-number-of- > rows case would continue to work as before. David has a point that it > would silently break application code that's actually depending on the > LCM behavior, but how much of that is there likely to be, really? > > [ reflects a bit... ] I guess there is room for an option 2-and-a-half: > > 2.5. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...), but decorate > the FunctionScan RTE to tell the executor to throw an error if the SRFs > don't all return the same number of rows, rather than silently > null-padding. This would have the same behavior as before for the sane > case, and would be very not-silent about cases where apps actually invoked > the LCM behavior. Again, we wouldn't necessarily have to expose such an > option at the SQL level. (Though it strikes me that such a restriction > could have value in its own right, analogous to the STRICT options that > we've invented in some other places to allow insisting on the expected > numbers of rows being returned. ROWS FROM STRICT (srf1(), srf2(), ...), > anybody?) > I'd let the engineers decide between 1, 2.5, and 3 - but if we accomplish our goals while implementing #3 I'd say that would be the best outcome for the community as whole. We don't have the luxury of providing a safe-usage mode where people writing new queries get the error but pre-existing queries are considered OK. We will have to rely upon education and deal with the occasional bug report but our long-time customers, even if only a minority would be affected, will appreciate the effort taken to not break code that has been working for a long time. The minority is likely small enough to at least make options 1 and 2.5 viable though I'd think we make an effort to avoid #1. David J.
Re: [HACKERS] Changed SRF in targetlist handling
Tom Lane wrote: > Joe Conway writes: > > I'll also note that, unless I missed something, we also have to consider > > that the capability to pipeline results is still only available in the > > target list. > > Yes, we would definitely want to improve nodeFunctionscan.c to perform > better for ValuePerCall SRFs. But that has value independently of this. Ah, so that's what "pipeline results" mean! I hadn't gotten that. I agree; Abhijit had a patch or a plan for this, a long time ago ... -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
Joe Conway writes: > I would be in favor of rewriting it to a LATERAL, but that would not be > backwards compatible entirely either IIUC. It could be made so, I think, but it may be more trouble than it's worth; see my previous message. > I'll also note that, unless I missed something, we also have to consider > that the capability to pipeline results is still only available in the > target list. Yes, we would definitely want to improve nodeFunctionscan.c to perform better for ValuePerCall SRFs. But that has value independently of this. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 4:05 PM, David Fetter wrote: > On Mon, May 23, 2016 at 02:39:54PM -0500, Merlin Moncure wrote: > > On Mon, May 23, 2016 at 2:13 PM, David Fetter wrote: > > > On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote: > > >> On Mon, May 23, 2016 at 12:10 PM, Tom Lane wrote: > > >> > Andres Freund writes: > > >> >> discussing executor performance with a number of people at pgcon, > > >> >> several hackers - me included - complained about the additional > > >> >> complexity, both code and runtime, required to handle SRFs in the > target > > >> >> list. > > >> > > > >> > Yeah, this has been an annoyance for a long time. > > >> > > > >> >> One idea I circulated was to fix that by interjecting a special > executor > > >> >> node to process SRF containing targetlists (reusing Result > possibly?). > > >> >> That'd allow to remove the isDone argument from > ExecEval*/ExecProject* > > >> >> and get rid of ps_TupFromTlist which is fairly ugly. > > >> > > > >> > Would that not lead to, in effect, duplicating all of execQual.c? > The new > > >> > executor node would still have to be prepared to process all > expression > > >> > node types. > > >> > > > >> >> Robert suggested - IIRC mentioning previous on-list discussion - to > > >> >> instead rewrite targetlist SRFs into lateral joins. My gut feeling > is > > >> >> that that'd be a larger undertaking, with significant semantics > changes. > > >> > > > >> > Yes, this was discussed on-list awhile back (I see David found a > reference > > >> > already). I think it's feasible, although we'd first have to agree > > >> > whether we want to remain bug-compatible with the old > > >> > least-common-multiple-of-the-periods behavior. I would vote for > not, > > >> > but it's certainly a debatable thing. > > >> > > >> +1 on removing LCM. > > > > > > As a green field project, that would make total sense. As a thing > > > decades in, it's not clear to me that that would break less stuff or > > > break it worse than simply disallowing SRFs in the target list, which > > > has been rejected on bugward-compatibility grounds. I suspect it > > > would be even worse because disallowing SRFs in target lists would at > > > least be obvious and localized when it broke code. > > > > If I'm reading this correctly, it sounds to me like you are making the > > case that removing target list SRF completely would somehow cause less > > breakage than say, rewriting it to a LATERAL based implementation for > > example. > > Yes. > > Making SRFs in target lists throw an error is a thing that will be > pretty straightforward to deal with in extant code bases, whatever > size of pain in the neck it might be. The line of code that caused > the error would be very clear, and the fix would be very obvious. > > Making their behavior different in some way that throws no warnings is > guaranteed to cause subtle and hard to track bugs in extant code > bases. I'm advocating that if a presently allowed SRF-in-target-list is allowed to remain it executes using the same semantics it has today. In all other cases, including LCM, if the present behavior is undesirable to maintain we throw an error. I'd hope that such an error can be written in such a way as to name the offending function or functions. If the user of a complex query doesn't want to expend the effort to locate the specific instance of SRF that is in violation they will still have the option to rewrite all of their uses in that particular query. David J.
Re: [HACKERS] Calling json_* functions with JSONB data
Great question, Marko. If you can point me towards an example I'll take a look, but I'll proceed with the current understanding and suggestions and see what people have to say. On Mon, May 23, 2016 at 10:47 AM, Marko Tiikkaja wrote: > On 2016-05-23 18:55, Peter van Hardenberg wrote: > >> I talked this over with Andrew who had no objections and suggested I float >> it on the list before writing a patch. Looks pretty straightforward, just >> a >> few new data rows in pg_proc.h. >> >> Anyone have any concerns or suggestions? >> > > What about cases like json_whatever($1) which previously worked but will > now be ambiguous? (Or will they somehow not be ambiguous?) > > > .m > -- Peter van Hardenberg San Francisco, California "Everything was beautiful, and nothing hurt."—Kurt Vonnegut
Re: [HACKERS] Changed SRF in targetlist handling
Merlin Moncure writes: > On Mon, May 23, 2016 at 2:13 PM, David Fetter wrote: >> On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote: >>> +1 on removing LCM. >> As a green field project, that would make total sense. As a thing >> decades in, it's not clear to me that that would break less stuff or >> break it worse than simply disallowing SRFs in the target list, which >> has been rejected on bugward-compatibility grounds. I suspect it >> would be even worse because disallowing SRFs in target lists would at >> least be obvious and localized when it broke code. > If I'm reading this correctly, it sounds to me like you are making the > case that removing target list SRF completely would somehow cause less > breakage than say, rewriting it to a LATERAL based implementation for > example. With more than a little forbearance, let's just say I don't > agree. We should consider single and multiple SRFs in a targetlist as distinct use-cases; only the latter has got weird properties. There are several things we could potentially do with multiple SRFs in the same targetlist. In increasing order of backwards compatibility and effort required: 1. Throw error if there's more than one SRF. 2. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...). This would have the same behavior as before if the SRFs all return the same number of rows, and otherwise would behave differently. 3. Rewrite into some other construct that still ends up as a FunctionScan RTE node, but has the old LCM behavior if the SRFs produce different numbers of rows. (Perhaps we would not need to expose this construct as something directly SQL-visible.) It's certainly arguable that the common use-cases for SRF-in-tlist don't have more than one SRF per tlist, and thus that implementing #1 would be an appropriate amount of effort. It's worth noting also that the LCM behavior has been repeatedly reported as a bug, and therefore that if we do #3 we'll be expending very substantial effort to be literally bug-compatible with ancient behavior that no one in the current development group thinks is well-designed. As far as #2 goes, it would have the advantage that code depending on the same-number-of- rows case would continue to work as before. David has a point that it would silently break application code that's actually depending on the LCM behavior, but how much of that is there likely to be, really? [ reflects a bit... ] I guess there is room for an option 2-and-a-half: 2.5. Rewrite into LATERAL ROWS FROM (srf1(), srf2(), ...), but decorate the FunctionScan RTE to tell the executor to throw an error if the SRFs don't all return the same number of rows, rather than silently null-padding. This would have the same behavior as before for the sane case, and would be very not-silent about cases where apps actually invoked the LCM behavior. Again, we wouldn't necessarily have to expose such an option at the SQL level. (Though it strikes me that such a restriction could have value in its own right, analogous to the STRICT options that we've invented in some other places to allow insisting on the expected numbers of rows being returned. ROWS FROM STRICT (srf1(), srf2(), ...), anybody?) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 02:39:54PM -0500, Merlin Moncure wrote: > On Mon, May 23, 2016 at 2:13 PM, David Fetter wrote: > > On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote: > >> On Mon, May 23, 2016 at 12:10 PM, Tom Lane wrote: > >> > Andres Freund writes: > >> >> discussing executor performance with a number of people at pgcon, > >> >> several hackers - me included - complained about the additional > >> >> complexity, both code and runtime, required to handle SRFs in the target > >> >> list. > >> > > >> > Yeah, this has been an annoyance for a long time. > >> > > >> >> One idea I circulated was to fix that by interjecting a special executor > >> >> node to process SRF containing targetlists (reusing Result possibly?). > >> >> That'd allow to remove the isDone argument from ExecEval*/ExecProject* > >> >> and get rid of ps_TupFromTlist which is fairly ugly. > >> > > >> > Would that not lead to, in effect, duplicating all of execQual.c? The > >> > new > >> > executor node would still have to be prepared to process all expression > >> > node types. > >> > > >> >> Robert suggested - IIRC mentioning previous on-list discussion - to > >> >> instead rewrite targetlist SRFs into lateral joins. My gut feeling is > >> >> that that'd be a larger undertaking, with significant semantics changes. > >> > > >> > Yes, this was discussed on-list awhile back (I see David found a > >> > reference > >> > already). I think it's feasible, although we'd first have to agree > >> > whether we want to remain bug-compatible with the old > >> > least-common-multiple-of-the-periods behavior. I would vote for not, > >> > but it's certainly a debatable thing. > >> > >> +1 on removing LCM. > > > > As a green field project, that would make total sense. As a thing > > decades in, it's not clear to me that that would break less stuff or > > break it worse than simply disallowing SRFs in the target list, which > > has been rejected on bugward-compatibility grounds. I suspect it > > would be even worse because disallowing SRFs in target lists would at > > least be obvious and localized when it broke code. > > If I'm reading this correctly, it sounds to me like you are making the > case that removing target list SRF completely would somehow cause less > breakage than say, rewriting it to a LATERAL based implementation for > example. Yes. Making SRFs in target lists throw an error is a thing that will be pretty straightforward to deal with in extant code bases, whatever size of pain in the neck it might be. The line of code that caused the error would be very clear, and the fix would be very obvious. Making their behavior different in some way that throws no warnings is guaranteed to cause subtle and hard to track bugs in extant code bases. We lost not a few existing users when we caused similar knock-ons in 8.3 by removing automated casts to text. I am no longer advocating for removing the functionality. I am just pointing out that the knock-on effects of changing the functionality may well cause more pain than the ones from removing it entirely. > With more than a little forbearance, let's just say I don't agree. If you'd be so kind as to explain your reasons, I think we'd all benefit. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On 05/23/2016 12:39 PM, Merlin Moncure wrote: > On Mon, May 23, 2016 at 2:13 PM, David Fetter wrote: >> On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote: >>> On Mon, May 23, 2016 at 12:10 PM, Tom Lane wrote: Andres Freund writes: > discussing executor performance with a number of people at pgcon, > several hackers - me included - complained about the additional > complexity, both code and runtime, required to handle SRFs in the target > list. Yeah, this has been an annoyance for a long time. > One idea I circulated was to fix that by interjecting a special executor > node to process SRF containing targetlists (reusing Result possibly?). > That'd allow to remove the isDone argument from ExecEval*/ExecProject* > and get rid of ps_TupFromTlist which is fairly ugly. Would that not lead to, in effect, duplicating all of execQual.c? The new executor node would still have to be prepared to process all expression node types. > Robert suggested - IIRC mentioning previous on-list discussion - to > instead rewrite targetlist SRFs into lateral joins. My gut feeling is > that that'd be a larger undertaking, with significant semantics changes. Yes, this was discussed on-list awhile back (I see David found a reference already). I think it's feasible, although we'd first have to agree whether we want to remain bug-compatible with the old least-common-multiple-of-the-periods behavior. I would vote for not, but it's certainly a debatable thing. >>> >>> +1 on removing LCM. >> >> As a green field project, that would make total sense. As a thing >> decades in, it's not clear to me that that would break less stuff or >> break it worse than simply disallowing SRFs in the target list, which >> has been rejected on bugward-compatibility grounds. I suspect it >> would be even worse because disallowing SRFs in target lists would at >> least be obvious and localized when it broke code. > > If I'm reading this correctly, it sounds to me like you are making the > case that removing target list SRF completely would somehow cause less > breakage than say, rewriting it to a LATERAL based implementation for > example. With more than a little forbearance, let's just say I don't > agree. I'm not necessarily saying that we should totally remove target list SRFs, but I will point out it has been deprecated ever since SRFs were first introduced: http://www.postgresql.org/docs/7.3/static/xfunc-sql.html "Currently, functions returning sets may also be called in the target list of a SELECT query. For each row that the SELECT generates by itself, the function returning set is invoked, and an output row is generated for each element of the function's result set. Note, however, that this capability is deprecated and may be removed in future releases." I would be in favor of rewriting it to a LATERAL, but that would not be backwards compatible entirely either IIUC. I'll also note that, unless I missed something, we also have to consider that the capability to pipeline results is still only available in the target list. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
Peter Geoghegan writes: > On Mon, May 23, 2016 at 12:22 PM, Tom Lane wrote: >> It seems unlikely to me that recursing into the name lists is helpful >> here: those are not going to contain any data that is interpretable >> without context. Did you have a reason to do that? > I saw no reason to avoid the extra cycles. A noticeable omission has a > cost: it gets noticed. Given this code path is likely to hardly ever > be hit, this mechanical approach seemed best. That's all. I agree that performance isn't much of a concern, but code bloat and inconsistency with other cases are valid concerns. That function does not recurse into name lists in, for example, the A_Expr and FuncCall cases. Also, related to this complaint though not this patch, it's disturbing that this oversight wasn't detected long ago. My first thought was to add some conditionally-compiled debugging code that just performs a no-op traverse of every raw parse tree produced by the grammar. However that doesn't work because raw_expression_tree_walker doesn't promise to support everything, only DML statements. Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 2:13 PM, David Fetter wrote: > On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote: >> On Mon, May 23, 2016 at 12:10 PM, Tom Lane wrote: >> > Andres Freund writes: >> >> discussing executor performance with a number of people at pgcon, >> >> several hackers - me included - complained about the additional >> >> complexity, both code and runtime, required to handle SRFs in the target >> >> list. >> > >> > Yeah, this has been an annoyance for a long time. >> > >> >> One idea I circulated was to fix that by interjecting a special executor >> >> node to process SRF containing targetlists (reusing Result possibly?). >> >> That'd allow to remove the isDone argument from ExecEval*/ExecProject* >> >> and get rid of ps_TupFromTlist which is fairly ugly. >> > >> > Would that not lead to, in effect, duplicating all of execQual.c? The new >> > executor node would still have to be prepared to process all expression >> > node types. >> > >> >> Robert suggested - IIRC mentioning previous on-list discussion - to >> >> instead rewrite targetlist SRFs into lateral joins. My gut feeling is >> >> that that'd be a larger undertaking, with significant semantics changes. >> > >> > Yes, this was discussed on-list awhile back (I see David found a reference >> > already). I think it's feasible, although we'd first have to agree >> > whether we want to remain bug-compatible with the old >> > least-common-multiple-of-the-periods behavior. I would vote for not, >> > but it's certainly a debatable thing. >> >> +1 on removing LCM. > > As a green field project, that would make total sense. As a thing > decades in, it's not clear to me that that would break less stuff or > break it worse than simply disallowing SRFs in the target list, which > has been rejected on bugward-compatibility grounds. I suspect it > would be even worse because disallowing SRFs in target lists would at > least be obvious and localized when it broke code. If I'm reading this correctly, it sounds to me like you are making the case that removing target list SRF completely would somehow cause less breakage than say, rewriting it to a LATERAL based implementation for example. With more than a little forbearance, let's just say I don't agree. merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
On Mon, May 23, 2016 at 12:22 PM, Tom Lane wrote: > It seems unlikely to me that recursing into the name lists is helpful > here: those are not going to contain any data that is interpretable > without context. Did you have a reason to do that? I saw no reason to avoid the extra cycles. A noticeable omission has a cost: it gets noticed. Given this code path is likely to hardly ever be hit, this mechanical approach seemed best. That's all. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
Peter Geoghegan writes: > Attached patch fixes this issue by adding the appropriate > raw_expression_tree_walker handler. This isn't the first quasi-utility > node to need such a handler, so the fix is simple. It seems unlikely to me that recursing into the name lists is helpful here: those are not going to contain any data that is interpretable without context. Did you have a reason to do that? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 01:28:11PM -0500, Merlin Moncure wrote: > On Mon, May 23, 2016 at 12:10 PM, Tom Lane wrote: > > Andres Freund writes: > >> discussing executor performance with a number of people at pgcon, > >> several hackers - me included - complained about the additional > >> complexity, both code and runtime, required to handle SRFs in the target > >> list. > > > > Yeah, this has been an annoyance for a long time. > > > >> One idea I circulated was to fix that by interjecting a special executor > >> node to process SRF containing targetlists (reusing Result possibly?). > >> That'd allow to remove the isDone argument from ExecEval*/ExecProject* > >> and get rid of ps_TupFromTlist which is fairly ugly. > > > > Would that not lead to, in effect, duplicating all of execQual.c? The new > > executor node would still have to be prepared to process all expression > > node types. > > > >> Robert suggested - IIRC mentioning previous on-list discussion - to > >> instead rewrite targetlist SRFs into lateral joins. My gut feeling is > >> that that'd be a larger undertaking, with significant semantics changes. > > > > Yes, this was discussed on-list awhile back (I see David found a reference > > already). I think it's feasible, although we'd first have to agree > > whether we want to remain bug-compatible with the old > > least-common-multiple-of-the-periods behavior. I would vote for not, > > but it's certainly a debatable thing. > > +1 on removing LCM. As a green field project, that would make total sense. As a thing decades in, it's not clear to me that that would break less stuff or break it worse than simply disallowing SRFs in the target list, which has been rejected on bugward-compatibility grounds. I suspect it would be even worse because disallowing SRFs in target lists would at least be obvious and localized when it broke code. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #14153: Unrecognized node type error when upsert is present in recursive CTE
On Sat, May 21, 2016 at 5:03 PM, Peter Geoghegan wrote:
> On Sat, May 21, 2016 at 4:28 PM, wrote:
>> ERROR: XX000: unrecognized node type: 920
>> LOCATION: raw_expression_tree_walker, nodeFuncs.c:3410
>>
>> I expected the query run successfully and return one row with 'a'.
>
> This is clearly a bug.
>
> 920 is IndexElem, which can appear in a raw parse tree in 9.5, due to
> ON CONFLICT's use of inference reusing what was previously only used
> for CREATE INDEX. (It does not make it into the post-parse analysis
> tree, though).
Attached patch fixes this issue by adding the appropriate
raw_expression_tree_walker handler. This isn't the first quasi-utility
node to need such a handler, so the fix is simple.
--
Peter Geoghegan
From 9610fb53586fccee1f6bb361607706e73d8a1cba Mon Sep 17 00:00:00 2001
From: Peter Geoghegan
Date: Mon, 23 May 2016 11:17:16 -0700
Subject: [PATCH] Add IndexElem to raw_expression_tree_walker()
INSERT ... ON CONFLICT unique index inference reused IndexElem nodes
within its raw parse tree. IndexElems were previously only used for
CREATE INDEX. There was never an IndexElem handler within
raw_expression_tree_walker(), presumably because of the assumption that
IndexElem was used exclusively by utility statements, and as such
required no handler. That assumption is now clearly obsolete. This can
be shown easily by executing a statement with a WITH RECURSIVE CTE that
contains INSERT ... ON CONFLICT ... RETURNING.
Per bug #14153 from Thomas Alton. Backpatch to 9.5, where ON CONFLICT
was added.
---
src/backend/nodes/nodeFuncs.c | 12
1 file changed, 12 insertions(+)
diff --git a/src/backend/nodes/nodeFuncs.c b/src/backend/nodes/nodeFuncs.c
index 7bc5be1..5e03efe 100644
--- a/src/backend/nodes/nodeFuncs.c
+++ b/src/backend/nodes/nodeFuncs.c
@@ -3370,6 +3370,18 @@ raw_expression_tree_walker(Node *node,
/* for now, constraints are ignored */
}
break;
+ case T_IndexElem:
+ {
+IndexElem *indelem = (IndexElem *) node;
+
+if (walker(indelem->expr, context))
+ return true;
+if (walker(indelem->collation, context))
+ return true;
+if (walker(indelem->opclass, context))
+ return true;
+ }
+ break;
case T_GroupingSet:
return walker(((GroupingSet *) node)->content, context);
case T_LockingClause:
--
2.7.4
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 1:44 PM, David Fetter wrote: > On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote: > > David Fetter writes: > > > On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote: > > >> This seems a bridge too far to me. It's just way too common to do > > >> "select generate_series(1,n)". We could tell people they have to > > >> rewrite to "select * from generate_series(1,n)", but it would be far > > >> more polite to do that for them. > > > > > How about making "TABLE generate_series(1,n)" work? It's even > > > shorter in exchange for some cognitive load. > > > > No thanks --- the word after TABLE ought to be a table name, not some > > arbitrary expression. That's way too much mess to save one keystroke. > > It's not just about saving a keystroke. This change would go with > removing the ability to do SRFs in the target list of a SELECT > query. > If you want to make an argument for doing this regardless of the target list SRF change by all means - but it does absolutely nothing to mitigate the breakage that would result if we choose this path. David J.
Re: [HACKERS] Calling json_* functions with JSONB data
Marko Tiikkaja writes: > On 2016-05-23 18:55, Peter van Hardenberg wrote: >> Anyone have any concerns or suggestions? > What about cases like json_whatever($1) which previously worked but > will now be ambiguous? (Or will they somehow not be ambiguous?) Good point, that would have to be looked into. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 12:10 PM, Tom Lane wrote: > Andres Freund writes: >> discussing executor performance with a number of people at pgcon, >> several hackers - me included - complained about the additional >> complexity, both code and runtime, required to handle SRFs in the target >> list. > > Yeah, this has been an annoyance for a long time. > >> One idea I circulated was to fix that by interjecting a special executor >> node to process SRF containing targetlists (reusing Result possibly?). >> That'd allow to remove the isDone argument from ExecEval*/ExecProject* >> and get rid of ps_TupFromTlist which is fairly ugly. > > Would that not lead to, in effect, duplicating all of execQual.c? The new > executor node would still have to be prepared to process all expression > node types. > >> Robert suggested - IIRC mentioning previous on-list discussion - to >> instead rewrite targetlist SRFs into lateral joins. My gut feeling is >> that that'd be a larger undertaking, with significant semantics changes. > > Yes, this was discussed on-list awhile back (I see David found a reference > already). I think it's feasible, although we'd first have to agree > whether we want to remain bug-compatible with the old > least-common-multiple-of-the-periods behavior. I would vote for not, > but it's certainly a debatable thing. +1 on removing LCM. The behavior of multiple targetlist SRF is so bizarre that it's incredible to believe anyone would reasonably expect it to work that way. Agree also that casual sane usage of target list SRF is incredibly common via generate_series() and unnest() etc is exceptionally common...better not to break those cases without a better justification than code simplicity. merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
David Fetter writes: > On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote: >> David Fetter writes: >>> How about making "TABLE generate_series(1,n)" work? It's even >>> shorter in exchange for some cognitive load. >> No thanks --- the word after TABLE ought to be a table name, not some >> arbitrary expression. That's way too much mess to save one keystroke. > It's not just about saving a keystroke. This change would go with > removing the ability to do SRFs in the target list of a SELECT > query. I guess you did not understand that I was rejecting doing that. Telling people they have to modify existing code that does this and works fine is exactly what I felt we can't do. We might be able to blow off complicated cases, but I think simpler cases are too common in the field. I'm on board with fixing things so that the *implementation* doesn't support SRF-in-tlist. But we can't just remove it from the language. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] It's seems that the function "do_text_output_multiline" does not suit for format "line1\nline2\n...lineN".
David Rowley writes: > On 20 May 2016 at 19:13, Hao Lee wrote: >> Today, I am do some works on adding some customized featues to PostgreSQL >> 9.6 beta1. But, when i do some output to psql using the fuction >> "do_text_output_multiline" with the string just like mentioned in mail >> tilte, such as "this is a\ntest for\nnew blank.". the PostgreSQL may lead to >> corruption in this function, and i debugged it that found this function can >> not dealt with the boundaries properly. The original function code as : > Thanks for reporting this. It does seem pretty broken. I guess we've > only gotten away with this due to EXPLAIN output lines always having a > \n at the end of them, but we should fix this. Agreed. > Your proposed fix looks a little bit confused. You could have just > removed the eol += len; as testing if (eol) in the else will never be > true as that else is only being hit because eol is NULL. I think really the right fix is "eol = text + len" rather than modifying the loop condition. Almost certainly, that is what the original coder intended, but typo'd the statement and nobody ever noticed. > I shuffled things around in there a bit and came up with the attached fix. I didn't like this version because it duplicated the string-conversion code, which admittedly is only one line, but not a very simple line. I pushed something based on "eol = text + len" instead. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Calling json_* functions with JSONB data
On 2016-05-23 18:55, Peter van Hardenberg wrote: I talked this over with Andrew who had no objections and suggested I float it on the list before writing a patch. Looks pretty straightforward, just a few new data rows in pg_proc.h. Anyone have any concerns or suggestions? What about cases like json_whatever($1) which previously worked but will now be ambiguous? (Or will they somehow not be ambiguous?) .m -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel pg_dump's error reporting doesn't work worth squat
I wrote: >> ... the pg_dump process has crashed with a SIGPIPE without printing >> any message whatsoever, and without coming anywhere near finishing the >> dump. > Attached is a proposed patch for this. It reverts exit_horribly() to > what it used to be pre-9.3, ie just print the message on stderr and die. > The master now notices child failure by observing EOF on the status pipe. > While that works automatically on Unix, we have to explicitly close the > child side of the pipe on Windows (could someone check that that works?). > I also fixed the parent side to ignore SIGPIPE earlier, so that it won't > just die if it was in the midst of sending to the child. Now that we're all back from PGCon ... does anyone wish to review this? Or have an objection to treating it as a bug fix and patching all branches back to 9.3? > BTW, after having spent the afternoon staring at it, I will assert with > confidence that pg_dump/parallel.c is an embarrassment to the project. I've done a bit of work on a cosmetic cleanup patch, but don't have anything to show yet. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 01:36:57PM -0400, Tom Lane wrote: > David Fetter writes: > > On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote: > >> This seems a bridge too far to me. It's just way too common to do > >> "select generate_series(1,n)". We could tell people they have to > >> rewrite to "select * from generate_series(1,n)", but it would be far > >> more polite to do that for them. > > > How about making "TABLE generate_series(1,n)" work? It's even > > shorter in exchange for some cognitive load. > > No thanks --- the word after TABLE ought to be a table name, not some > arbitrary expression. That's way too much mess to save one keystroke. It's not just about saving a keystroke. This change would go with removing the ability to do SRFs in the target list of a SELECT query. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
David Fetter writes: > On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote: >> This seems a bridge too far to me. It's just way too common to do >> "select generate_series(1,n)". We could tell people they have to >> rewrite to "select * from generate_series(1,n)", but it would be far >> more polite to do that for them. > How about making "TABLE generate_series(1,n)" work? It's even > shorter in exchange for some cognitive load. No thanks --- the word after TABLE ought to be a table name, not some arbitrary expression. That's way too much mess to save one keystroke. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Changed SRF in targetlist handling
On Mon, May 23, 2016 at 01:10:29PM -0400, Tom Lane wrote: > This seems a bridge too far to me. It's just way too common to do > "select generate_series(1,n)". We could tell people they have to > rewrite to "select * from generate_series(1,n)", but it would be far > more polite to do that for them. How about making "TABLE generate_series(1,n)" work? It's even shorter in exchange for some cognitive load. Cheers, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Calling json_* functions with JSONB data
Peter van Hardenberg writes: > I talked this over with Andrew who had no objections and suggested I float > it on the list before writing a patch. Looks pretty straightforward, just a > few new data rows in pg_proc.h. I think you might find that you need to add new C function entry points to keep opr_sanity.sql from complaining. That's still pretty easy though. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Calling json_* functions with JSONB data
On Mon, May 23, 2016 at 11:14 AM, David G. Johnston <
david.g.johns...@gmail.com> wrote:
> On Mon, May 23, 2016 at 12:55 PM, Peter van Hardenberg wrote:
>
>> Hi there,
>>
>> I noticed it was very easy to accidentally call the json_* form of JSON
>> manipulation functions with jsonb data as input. This is pretty
>> sub-optimal, since it involves rendering the jsonb then reparsing it and
>> calling the json_* form of the function.
>>
>> Fortunately, this seems quite easy to resolve by taking advantage of our
>> ability to add json_*(jsonb) form of the functions.
>>
>> I talked this over with Andrew who had no objections and suggested I
>> float it on the list before writing a patch. Looks pretty straightforward,
>> just a few new data rows in pg_proc.h.
>>
>> Anyone have any concerns or suggestions?
>>
>>
> Please provide an example of what you are talking about.
>
> SELECT json_array_length('[1,2]'::jsonb)
> ERROR: function json_array_length(jsonb) does not exist
>
> -- The function name is "jsonb_array_length"; and there is no implicit
> cast between the two.
>
He is saying that he accidentally calls json_array_length() instead of
jsonb_array_length()
and that it is an annoying usability problem. It happens to me too and I
agree it would be better if you could just call json_array_length()
regardless if the type is JSON or JSONB. If there is some significant
functionality difference from the user's perspective then having separate
"json_" and "jsonb_" functions makes sense, but in your example there is
not.
Re: [HACKERS] Parallel safety tagging of extension functions
Alvaro Herrera writes: > Tom Lane wrote: >> You'd have to alter the index opfamily to disconnect the function from it, >> drop/recreate the function, then re-add it to the opfamily. Kind of icky, >> but probably better than the alternatives. > What happens if the upgraded database contains indexes using those > opfamilies? I suppose getting such indexes dropped because of ALTER > EXTENSION UPDATE is not very nice. Sure, that's why we mustn't drop and recreate the whole opfamily. But we can do ALTER OPERATOR FAMILY ... DROP ... without causing dependent indexes to be dropped. Semi-bad things would happen if someone tried to access such an index partway through; but as long as the extension upgrade script itself doesn't do that, it should be okay. We run extension scripts as single transactions so the change should appear atomic. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Calling json_* functions with JSONB data
On Mon, May 23, 2016 at 12:55 PM, Peter van Hardenberg wrote:
> Hi there,
>
> I noticed it was very easy to accidentally call the json_* form of JSON
> manipulation functions with jsonb data as input. This is pretty
> sub-optimal, since it involves rendering the jsonb then reparsing it and
> calling the json_* form of the function.
>
> Fortunately, this seems quite easy to resolve by taking advantage of our
> ability to add json_*(jsonb) form of the functions.
>
> I talked this over with Andrew who had no objections and suggested I float
> it on the list before writing a patch. Looks pretty straightforward, just a
> few new data rows in pg_proc.h.
>
> Anyone have any concerns or suggestions?
>
>
Please provide an example of what you are talking about.
SELECT json_array_length('[1,2]'::jsonb)
ERROR: function json_array_length(jsonb) does not exist
-- The function name is "jsonb_array_length"; and there is no implicit cast
between the two.
David J.
[HACKERS] Calling json_* functions with JSONB data
Hi there, I noticed it was very easy to accidentally call the json_* form of JSON manipulation functions with jsonb data as input. This is pretty sub-optimal, since it involves rendering the jsonb then reparsing it and calling the json_* form of the function. Fortunately, this seems quite easy to resolve by taking advantage of our ability to add json_*(jsonb) form of the functions. I talked this over with Andrew who had no objections and suggested I float it on the list before writing a patch. Looks pretty straightforward, just a few new data rows in pg_proc.h. Anyone have any concerns or suggestions? -p -- Peter van Hardenberg San Francisco, California "Everything was beautiful, and nothing hurt."—Kurt Vonnegut
Re: [HACKERS] Changed SRF in targetlist handling
Andres Freund writes: > discussing executor performance with a number of people at pgcon, > several hackers - me included - complained about the additional > complexity, both code and runtime, required to handle SRFs in the target > list. Yeah, this has been an annoyance for a long time. > One idea I circulated was to fix that by interjecting a special executor > node to process SRF containing targetlists (reusing Result possibly?). > That'd allow to remove the isDone argument from ExecEval*/ExecProject* > and get rid of ps_TupFromTlist which is fairly ugly. Would that not lead to, in effect, duplicating all of execQual.c? The new executor node would still have to be prepared to process all expression node types. > Robert suggested - IIRC mentioning previous on-list discussion - to > instead rewrite targetlist SRFs into lateral joins. My gut feeling is > that that'd be a larger undertaking, with significant semantics changes. Yes, this was discussed on-list awhile back (I see David found a reference already). I think it's feasible, although we'd first have to agree whether we want to remain bug-compatible with the old least-common-multiple-of-the-periods behavior. I would vote for not, but it's certainly a debatable thing. > If we accept bigger semantical changes, I'm inclined to instead just get > rid of targetlist SRFs in total; they're really weird and not needed > anymore. This seems a bridge too far to me. It's just way too common to do "select generate_series(1,n)". We could tell people they have to rewrite to "select * from generate_series(1,n)", but it would be far more polite to do that for them. > One issue with removing targetlist SRFs is that they're currently > considerably faster than SRFs in FROM: I suspect that depends greatly on your test case. But in any case we could put more effort into optimizing nodeFunctionscan. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel safety tagging of extension functions
Tom Lane wrote: > Michael Paquier writes: > > On Sat, May 21, 2016 at 6:16 PM, Andreas Karlsson wrote: > >> My immediate thought is first doing an UPDATE of pg_proc and then updating > >> the catcache with CREATE OR REPLACE with the new arguments. Does that work? > >> Is there a less ugly way to accomplish this? > > > Isn't it better to just drop and recreate the function? pageinspect > > did so for example for heap_page_items in 1.4 to update its OUT > > arguments. > > You'd have to alter the index opfamily to disconnect the function from it, > drop/recreate the function, then re-add it to the opfamily. Kind of icky, > but probably better than the alternatives. What happens if the upgraded database contains indexes using those opfamilies? I suppose getting such indexes dropped because of ALTER EXTENSION UPDATE is not very nice. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls
On Mon, May 23, 2016 at 12:01 PM, Jeff Davis wrote: > On Fri, May 20, 2016 at 1:41 PM, David G. Johnston > wrote: > > How does the relatively new FILTER clause play into this, if at all? > > My interpretation of the standard is that FILTER is not allowable for > a window function, and IGNORE|RESPECT NULLS is not allowable for an > ordinary aggregate. > > So if we support IGNORE|RESPECT NULLS for anything other than a window > function, we have to come up with our own semantics. > > > We already have "STRICT" for deciding whether a function processes nulls. > > Wouldn't this need to exist on the "CREATE AGGREGATE" > > STRICT defines behavior at DDL time. I was suggesting that we might > want a DDL-time flag to indicate whether a function can make use of > the query-time IGNORE|RESPECT NULLS option. In other words, most > functions wouldn't know what to do with IGNORE|RESPECT NULLS, but > perhaps some would if we allowed them the option. > > Perhaps I didn't understand your point? > The "this" in the quote doesn't refer to STRICT but rather the non-existence feature that we are talking about. I am trying to make the point that the DDL syntax for "RESPECT|IGNORE NULLS" should be on the "CREATE AGGREGATE" page and not the "CREATE FUNCTION" page. As far as a plain function cares its only responsibility with respect to NULLs is defined by the STRICT property. As this behavior only manifests in aggregate situations the corresponding property should be defined there as well. David J.
Re: [HACKERS] Latent cache flush hazard in RelationInitIndexAccessInfo
Michael Paquier writes: > On Fri, Sep 11, 2015 at 11:29 AM, Robert Haas wrote: >> On Wed, Sep 9, 2015 at 3:00 PM, Tom Lane wrote: >>> The specific reason there's a problem is that there's a disconnect between >>> RelationClearRelation's test for whether a relcache entry has valid index >>> info (it checks whether rd_indexcxt != NULL) and the order of operations >>> in RelationInitIndexAccessInfo (creating the index context is not the >>> first thing it does). Given that if RelationClearRelation thinks the >>> info is valid, what it does is call RelationReloadIndexInfo which needs >>> rd_index to be valid, I'm thinking the best fix is to leave the order of >>> operations in RelationInitIndexAccessInfo alone and instead make the >>> "relcache entry has index info" check be "rd_index != NULL". There might >>> need to be some additional work to keep RelationReloadIndexInfo from >>> setting rd_isvalid = true too soon. >> Doesn't seem like a bad thing to clean up. > Doesn't sound bad to me either to reinforce things on HEAD... It's > been months since this has been posted, are you working on a patch? This dropped off my radar screen somehow. (I have a feeling I fixed it in Salesforce's code and forgot to transfer the fix to community.) Given that this is just latent for the community's purposes, it seems probably inappropriate to fix it post-beta1. I'm inclined to let it slide till 9.7^H^H^H10 development opens. Thoughts? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [sqlsmith] PANIC: failed to add BRIN tuple
Andreas Seltenreich wrote: > There was one instance of this PANIC when testing with the regression db > of master at 50e5315. > > , > | WARNING: specified item offset is too large > | PANIC: failed to add BRIN tuple > | server closed the connection unexpectedly > ` > > It is reproducible with the query below on this instance only. Hm, so this is an over-eager check. As I understand, what seems to have happened is that the page was filled with up to 10 tuples, then tuples 0-8 were removed, probably moved to other pages. When this update runs, it needs to update the remaining tuple, which is a normal thing for BRIN to do. But BRIN doesn't want the item number to change, because it's referenced from the "revmap"; so it removes the item and then wants to insert it again. But bufpage.c is not accustomed to having callers want to put items beyond what's the last currently used item plus one, so it raises a warning and returns without doing the insert. This drives BRIN crazy. I tried simply removing the "return InvalidOffsetNumber" line from that block (so that it still throws the warning but it does execute the index insert), and everything seems to behave correctly. I suppose a simple fix would be to add a flag to PageAddItem() and skip this block in that case, but that would break the ABI in 9.5. I'm not real sure what's a good fix yet. Maybe a new routine specific to the needs of BRIN is called for. I would also like to come up with a way to have this scenario be tested by a new regression test. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel safety tagging of extension functions
Michael Paquier writes: > On Sat, May 21, 2016 at 6:16 PM, Andreas Karlsson wrote: >> My immediate thought is first doing an UPDATE of pg_proc and then updating >> the catcache with CREATE OR REPLACE with the new arguments. Does that work? >> Is there a less ugly way to accomplish this? > Isn't it better to just drop and recreate the function? pageinspect > did so for example for heap_page_items in 1.4 to update its OUT > arguments. You'd have to alter the index opfamily to disconnect the function from it, drop/recreate the function, then re-add it to the opfamily. Kind of icky, but probably better than the alternatives. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Inheritance
On Mon, May 23, 2016 at 10:21 AM, Jim Nasby wrote: > On 5/22/16 1:37 AM, Jan Johansson wrote: >> >> - Allow single (behavior) inheritance (model here is quite a few modern >> languages, such as C#, D, ...) >> - Allow multiple declarative inheritance (interface like, the >> inheritance almost works like this today though) >> >> If, with these restrictions (or maybe only the first), do you think that >> it will simplify implementation and make it more feature complete? > > I think you'll need to be a bit more specific to elicit a response. What > exactly are you proposing to change? I would guess OP is complaining about what everyone complains about. What people want is for tables to have a base set of shared columns by a varying set of derived type dependent columns. Constraints on the shared columns will be enforced on all the derived columns. Postgres doesn't work that way, and the documentation disclaims this: "Note: Although inheritance is frequently useful, it has not been integrated with unique constraints or foreign keys, which limits its usefulness. See Section 5.8 for more detail." Personally, I don't think this will ever be fixed. The reason why it doesn't work is due to some foundational implementation decisions that would have to be revisited. This feature was very much a product of the time, at the height of the "Object Relational" fad. The trend for postgres has been in the exact opposite direction, towards the SQL standard. Further complicating matters, inheritance has been repurposed to be the foundation for table partitioning, making heavy changes problematic. The classic SQL approach to the problem, establishing a base table plus a type and derived tables with a pkey:pkey link isn't a very bad one from a data modelling perspective and serialization to the application is increasingly going to be handled by json going forward as opposed to hacking the postgres type system. This really reduces the value proposition of heavy changes to the inheritance features. If there was consensus on that point, I suppose the way forward is some documentation restructuring, starting with removing the increasingly baroque tutorial (trivia: the tutorial was the old manual at one point). merlin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Request for Patch Feedback: Lag & Lead Window Functions Can Ignore Nulls
On Fri, May 20, 2016 at 1:41 PM, David G. Johnston wrote: > How does the relatively new FILTER clause play into this, if at all? My interpretation of the standard is that FILTER is not allowable for a window function, and IGNORE|RESPECT NULLS is not allowable for an ordinary aggregate. So if we support IGNORE|RESPECT NULLS for anything other than a window function, we have to come up with our own semantics. > We already have "STRICT" for deciding whether a function processes nulls. > Wouldn't this need to exist on the "CREATE AGGREGATE" STRICT defines behavior at DDL time. I was suggesting that we might want a DDL-time flag to indicate whether a function can make use of the query-time IGNORE|RESPECT NULLS option. In other words, most functions wouldn't know what to do with IGNORE|RESPECT NULLS, but perhaps some would if we allowed them the option. Perhaps I didn't understand your point? Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Batch/pipelining support for libpq
On Mon, May 23, 2016 at 8:50 AM, Andres Freund wrote: > On 2016-05-23 17:19:09 +0800, Craig Ringer wrote: >> Following on from the foreign table batch inserts thread[1], here's a patch >> to add support for pipelining queries into asynchronous batches in libpq. > > Yay! >> I'm measuring 300x (not %) performance improvements doing batches on >> servers over the Internet, so this seems pretty worthwhile. It turned out >> to be way less invasive than I expected too. > > yay^2. I'll follow this mood. Yeha. >> (I intentionally didn't add any way for clients to annotate each work-item >> in a batch with their own private data. I think that'd be really useful and >> would make implementing clients easier, but should be a separate patch). >> >> This should be very useful for optimising FDWs, Postgres-XC, etc. > > And optimizing normal clients. > > Not easy, but I'd be very curious how much psql's performance improves > when replaying a .sql style dump, and replaying from a !tty fd, after > hacking it up to use the batch API. Did you consider the use of simple_list.c instead of introducing a new mimic as PGcommandQueueEntry? It would be cool avoiding adding new list emulations on frontends. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Batch/pipelining support for libpq
Hi, On 2016-05-23 17:19:09 +0800, Craig Ringer wrote: > Hi all > Following on from the foreign table batch inserts thread[1], here's a patch > to add support for pipelining queries into asynchronous batches in libpq. Yay! > I'm measuring 300x (not %) performance improvements doing batches on > servers over the Internet, so this seems pretty worthwhile. It turned out > to be way less invasive than I expected too. yay^2. > (I intentionally didn't add any way for clients to annotate each work-item > in a batch with their own private data. I think that'd be really useful and > would make implementing clients easier, but should be a separate patch). > > This should be very useful for optimising FDWs, Postgres-XC, etc. And optimizing normal clients. Not easy, but I'd be very curious how much psql's performance improves when replaying a .sql style dump, and replaying from a !tty fd, after hacking it up to use the batch API. - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Inheritance
On 5/22/16 1:37 AM, Jan Johansson wrote: - Allow single (behavior) inheritance (model here is quite a few modern languages, such as C#, D, ...) - Allow multiple declarative inheritance (interface like, the inheritance almost works like this today though) If, with these restrictions (or maybe only the first), do you think that it will simplify implementation and make it more feature complete? I think you'll need to be a bit more specific to elicit a response. What exactly are you proposing to change? -- Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX Experts in Analytics, Data Architecture and PostgreSQL Data in Trouble? Get it in Treble! http://BlueTreble.com 855-TREBLE2 (855-873-2532) mobile: 512-569-9461 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [sqlsmith] Failed assertions on parallel worker shutdown
Amit Kapila writes: > Earlier problems were due to the reason that some unsafe/restricted > expressions were pushed below Gather node as part of target list whereas in > the plan6, it seems some unsafe node is pushed below Gather node. It will > be helpful if you can share the offending query? plan6 corresponds to this query: select pg_catalog.anyarray_out( cast((select most_common_vals from pg_catalog.pg_stats limit 1 offset 41) as anyarray)) as c0 from public.quad_point_tbl as ref_0 where ref_0.p ~= ref_0.p; regards, Andreas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [sqlsmith] Failed assertions on parallel worker shutdown
On Mon, May 23, 2016 at 2:19 PM, Andreas Seltenreich
wrote:
>
> I wrote:
> >> There's another class of parallel worker core dumps when testing master
> >> with sqlsmith. In these cases, the following assertion fails for all
> >> workers simulataneously:
> >>
> >> TRAP: FailedAssertion("!(mqh->mqh_partial_bytes <= nbytes)", File:
"shm_mq.c", Line: 386)
> >
> > I no longer observe these after applying these two patches by Amit
> > Kapila
>
> I spoke too soon: These still occur with the patches applied, but with
> much lower probability. (one core dump per 20e6 random queries instead
> of 1e6).
>
> Most of the collected plans look inconspicuous to me now, except for one
> that again had a subplan below a gather node (plan6).
>
This problem looks different from the previous problems you have reported.
Earlier problems were due to the reason that some unsafe/restricted
expressions were pushed below Gather node as part of target list whereas in
the plan6, it seems some unsafe node is pushed below Gather node. It will
be helpful if you can share the offending query?
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] foreign table batch inserts
On 20 May 2016 at 23:18, Craig Ringer wrote: > On 20 May 2016 at 15:35, Craig Ringer wrote: > > >> >> You can, however, omit Sync from between messages and send a series of >> protocol messages, like >> >> Parse/Bind/Execute/Bind/Execute/Bind/Execute/Sync >> >> to avoid round-trip overheads. >> >> > I implemented what I think is a pretty solid proof of concept of this for > kicks this evening. Attached, including basic test program. Patch attached. > The performance difference over higher latency links is huge, see below. > I finished it off and submitted it. http://www.postgresql.org/message-id/flat/CAMsr+YFUjJytRyV4J-16bEoiZyH=4nj+sQ7JP9ajwz=b4dm...@mail.gmail.com#CAMsr+YFUjJytRyV4J-16bEoiZyH=4nj+sQ7JP9ajwz=b4dm...@mail.gmail.com https://commitfest.postgresql.org/10/634/ I'll use the other thread for the patch from now on. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
[HACKERS] PATCH: Batch/pipelining support for libpq
Hi all
Following on from the foreign table batch inserts thread[1], here's a patch
to add support for pipelining queries into asynchronous batches in libpq.
Attached, and also available at
https://github.com/2ndQuadrant/postgres/tree/dev/libpq-async-batch (subject
to rebasing and force pushes).
It's cleaned up over the draft I posted on that thread and has error
recovery implemented. I've written and included the SGML docs for it. The
test program is now pretty comprehensive, more so than for anything else in
libpq anyway. I'll submit it to the next CF as a 9.7/10.0 candidate.
I'm measuring 300x (not %) performance improvements doing batches on
servers over the Internet, so this seems pretty worthwhile. It turned out
to be way less invasive than I expected too.
(I intentionally didn't add any way for clients to annotate each work-item
in a batch with their own private data. I think that'd be really useful and
would make implementing clients easier, but should be a separate patch).
This should be very useful for optimising FDWs, Postgres-XC, etc.
[1]
http://www.postgresql.org/message-id/CAMsr+YFgDUiJ37DEfPRk8WDBuZ58psdAYJd8iNFSaGxtw=w...@mail.gmail.com
--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
From 711a6951f57d84309bd2d7477a145ee3412b7967 Mon Sep 17 00:00:00 2001
From: Craig Ringer
Date: Fri, 20 May 2016 12:45:18 +0800
Subject: [PATCH] Pipelining (batch) support for libpq
Allow libpq clients to avoid excessive round trips by pipelining multiple
commands into batches. A sync is only sent at the end of a batch. Commands
in a batch succeed or fail together.
Includes a test program in src/test/examples/testlibpqbatch.c
---
doc/src/sgml/libpq.sgml | 478 +
src/interfaces/libpq/exports.txt|7 +
src/interfaces/libpq/fe-connect.c | 16 +
src/interfaces/libpq/fe-exec.c | 572 +++-
src/interfaces/libpq/fe-protocol2.c |6 +
src/interfaces/libpq/fe-protocol3.c | 17 +-
src/interfaces/libpq/libpq-fe.h | 13 +-
src/interfaces/libpq/libpq-int.h| 37 +-
src/test/examples/.gitignore|1 +
src/test/examples/Makefile |2 +-
src/test/examples/testlibpqbatch.c | 1254 +++
11 files changed, 2360 insertions(+), 43 deletions(-)
create mode 100644 src/test/examples/testlibpqbatch.c
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 3829a14..d6e896f 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -4568,6 +4568,484 @@ int PQflush(PGconn *conn);
+
+ Batch mode and query pipelining
+
+
+ libpq
+ batch mode
+
+
+
+ libpq
+ pipelining
+
+
+
+ libpq supports queueing up mulitiple queries into
+ a pipeline to be executed as a batch on the server. Batching queries allows
+ applications to avoid a client/server round-trip after each query to get
+ the results before issuing the next query.
+
+
+
+ An example of batch use may be found in the source distribution in
+ src/test/examples/libpqbatch.c.
+
+
+
+ When to use batching
+
+
+Much like asynchronous query mode, there is no performance disadvantage to
+using batching and pipelining. It somewhat increased client application
+complexity and extra caution is required to prevent client/server network
+deadlocks, but can offer considerable performance improvements.
+
+
+
+Batching is most useful when the server is distant, i.e. network latency
+("ping time") is high, and when many small operations are being performed in
+rapid sequence. There is usually less benefit in using batches when each
+query takes many multiples of the client/server round-trip time to execute.
+A 100-statement operation run on a server 300ms round-trip-time away would take
+30 seconds in network latency alone without batching; with batching it may spend
+as little as 0.3s waiting for results from the server.
+
+
+
+Use batches when your application does lots of small
+INSERT, UPDATE and
+DELETE operations that can't easily be transformed into
+operations on sets or into a
+COPY operation.
+
+
+
+Batching less useful when information from one operation is required by the
+client before it knows enough to send the next operation. The client must
+introduce a synchronisation point and wait for a full client/server
+round-trip to get the results it needs. However, it's often possible to
+adjust the client design to exchange the required information server-side.
+Read-modify-write cycles are especially good candidates; for example:
+
+ BEGIN;
+ SELECT x FROM mytable WHERE id = 42 FOR UPDATE;
+ -- result: x=2
+ -- client adds 1 to x:
+ UPDATE mytable SET x = 3 WHERE id = 42;
+ COMMIT;
+
+could be much more efficiently done with:
+
+ UPDATE mytable SET x = x + 1;
+
Re: [HACKERS] [sqlsmith] Failed assertions on parallel worker shutdown
I wrote:
>> There's another class of parallel worker core dumps when testing master
>> with sqlsmith. In these cases, the following assertion fails for all
>> workers simulataneously:
>>
>> TRAP: FailedAssertion("!(mqh->mqh_partial_bytes <= nbytes)", File:
>> "shm_mq.c", Line: 386)
>
> I no longer observe these after applying these two patches by Amit
> Kapila
I spoke too soon: These still occur with the patches applied, but with
much lower probability. (one core dump per 20e6 random queries instead
of 1e6).
Most of the collected plans look inconspicuous to me now, except for one
that again had a subplan below a gather node (plan6). Tarball of all
collected plans attached.
regards,
Andreas
plans.tar.gz
Description: application/gzip
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
