Re: [HACKERS] Dynamic Shared Memory stuff
On Wed, Apr 9, 2014 at 9:20 PM, Robert Haas wrote: > On Wed, Apr 9, 2014 at 7:41 AM, Amit Kapila wrote: >> I am just not sure whether it is okay to rearrange the code and call >> GetLastError() only if returned handle is Invalid (NULL) or try to look >> for more info. > > Well, I don't know either. You wrote the Windows portion of this > code, so you'll have to decide what's best. If the best practice in > this area is to only call GetLastError() if the handle isn't valid, > then that's probably what we should do, too. But I can't speak to > what the best practice is. I have checked that other place in code also check handle to decide if API has failed. Refer function PGSharedMemoryIsInUse(). So I think fix to call GetLastError() after checking handle is okay. Attached patch fixes this issue. After patch, the server shows below log which is exactly what is expected from test_shm_mq LOG: registering background worker "test_shm_mq" LOG: starting background worker process "test_shm_mq" LOG: worker process: test_shm_mq (PID 4888) exited with exit code 1 LOG: unregistering background worker "test_shm_mq" LOG: registering background worker "test_shm_mq" LOG: starting background worker process "test_shm_mq" LOG: worker process: test_shm_mq (PID 3128) exited with exit code 1 LOG: unregistering background worker "test_shm_mq" With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com fix_dsm_invalid_errcode_issue.patch Description: Binary data -- 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] PostgreSQL in Windows console and Ctrl-C
On Fri, Apr 11, 2014 at 4:42 PM, Andrew Dunstan wrote: > On 04/11/2014 01:35 AM, Amit Kapila wrote: >> I don't think this is a complete fix, for example what about platform >> where >> _CreateRestrictedToken() is not supported. For Example, the current >> proposed fix will not work for below case: >> >> if (_CreateRestrictedToken == NULL) >> { >> /* >> * NT4 doesn't have CreateRestrictedToken, so just call ordinary >> * CreateProcess >> */ > Are we really supporting NT4 any more? Even XP is about to be at end of > support from Microsoft. In Docs, it is mentioned as Windows (Win2000 SP4 and later). Now what shall we do with this part of code, shall we keep it as it is and just fix in other part of code or shall we remove this part of code? Another thing to decide about this fix is that whether it is okay to fix it for CTRL+C and leave the problem open for CTRL+BREAK? (The current option used (CREATE_NEW_PROCESS_GROUP) will handle only CTRL+C). With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- 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] Negative Transition Aggregate Functions (WIP)
On Apr11, 2014, at 19:42 , Tom Lane wrote: > Florian Pflug writes: >> Yes, the idea had come up at some point during the review discussion. I >> agree that it's only worthwhile if it works for state type internal - though >> I think there ought to be a way to allow that. We could for example simply >> decree that the initfunc's first parameter must be of type internal if it's >> return type is, and then just pass NULL for that parameter. > > I had thought about that, but it doesn't really work since it'd be > violating the strictness spec of the function. Only if we insist on passing SQL NULL, not if we passed an non-NULL value that happens to be (char*)0. Or we could simply require the initfunc to be non-strict in the case of state type internal. best regards, Florian Pflug -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Problem with txid_snapshot_in/out() functionality
Hackers, the Slony team has been getting seldom reports of a problem with the txid_snapshot data type. The symptom is that a txid_snapshot on output lists the same txid multiple times in the xip list part of the external representation. This string is later rejected by txid_snapshot_in() when trying to determine if a particular txid is visible in that snapshot using the txid_visible_in_snapshot() function. I was not yet able to reproduce this problem in a lab environment. It might be related to subtransactions and/or two phase commit (at least one user is using both of them). The reported PostgreSQL version involved in that case was 9.1. At this point I would find it extremely helpful to "sanitize" the external representation in txid_snapshot_out() while emitting some NOTICE level logging when this actually happens. I am aware that this does amount to a functional change for a back release, but considering that the _out() generated external representation of an existing binary datum won't pass the type's _in() function, I argue that such change is warranted. Especially since this problem could possibly corrupt a dump. Comments? Jan -- Jan Wieck Senior Software Engineer http://slony.info -- 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] Problem with displaying "wide" tables in psql
Yeah, I think I agree. I'm pretty happy to commit it without old-ascii doing anything special. It looks to me like old-ascii just isn't very useful for a single column output (like expanded mode is implicitly). Maybe that needs to be fixed but then it needs to be fixed for non expanded mode as well. I don't think this new output is very useful dive it just displays the old-ascii info for the header cells. The header cells never wrap and often have a lot of empty lines so it just looks like noise. I do think the earlier change today was important. It looks great now with the wrap and newline indicators in ascii and Unicode mode. Fwiw I've switched my .psqlrc to use Unicode and wrapped=auto be default. It makes psql way more usable. It's even better than my old technique of running it in Emacs and scrolling left and right. I'll take a look at it this evening and will add regression tests (and probably more comments too!) and commit. I want to try adding Unicode regression tests but I'm not sure what the state of the art is for Unicode fallback behaviour on the build farm. I think there are some existing tests we can copy iirc. -- greg
Re: [HACKERS] Problem with displaying "wide" tables in psql
There were no support for wrapping and old-ascii line style in expanded mode at all. But now they are. 2014-04-11 21:58 GMT+04:00 Alvaro Herrera : > Sergey Muraviov wrote: > > I hope that I realized old-ascii logic correctly. > > I don't know what you changed here, but I don't think we need to > preserve old-ascii behavior in the new code, in any way. If you're > mimicking something obsolete and the end result of the new feature is > not great, perhaps that should be rethought. > > Can you please provide sample output for the previous version compared > to this new version? What changed? > > > -- > Álvaro Herrerahttp://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services > -- Best regards, Sergey Muraviov \pset expanded on \pset border 2 \pset columns 20 \pset format wrapped \pset linestyle unicode postgres=# select array_to_string(array_agg(repeat('x',n)),E'\n') as "x postgres"# y", array_to_string(array_agg(repeat('yy',10-n)),E'\n') as "x postgres"# y postgres"# z" from (select generate_series(1,10)) as t(n); ┌─[ RECORD 1 ]─┐ │ x↵│ x ↵│ │ y │ xx ↵│ │ │ xxx ↵│ │ │ ↵│ │ │ x ↵│ │ │ xx ↵│ │ │ xxx ↵│ │ │ ↵│ │ │ x ↵│ │ │ xx │ │ x↵│ …│ │ y↵│…yy ↵│ │ z │ …│ │ │…↵│ │ │ …│ │ │…yy ↵│ │ │ ↵│ │ │ yy ↵│ │ │ ↵│ │ │ yy ↵│ │ │ ↵│ │ │ yy ↵│ │ │ │ └───┴──┘ postgres=# \pset linestyle ascii Line style (linestyle) is ascii. postgres=# select array_to_string(array_agg(repeat('x',n)),E'\n') as "x y", array_to_string(array_agg(repeat('yy',10-n)),E'\n') as "x y z" from (select generate_series(1,10)) as t(n); +-[ RECORD 1 ]-+ | x+| x +| | y | xx +| | | xxx +| | | +| | | x +| | | xx +| | | xxx +| | | +| | | x +| | | xx | | x+| .| | y+|.yy +| | z | .| | |.+| | | .| | |.yy +| | | +| | | yy +| | | +| | | yy +| | | +| | | yy +| | | | +---+--+ postgres=# \pset linestyle old-ascii Line style (linestyle) is old-ascii. postgres=# select array_to_string(array_agg(repeat('x',n)),E'\n') as "x y", array_to_string(array_agg(repeat('yy',10-n)),E'\n') as "x y z" from (select generate_series(1,10)) as t(n); +-[ RECORD 1 ]-+ | x | x| |+y : xx | | : xxx | | : | | : x| | : xx | | : xxx | | : | | : x| | : xx | | x | | |+y ; yy | |+z : | | ; | | : | | ; yy | | : | | : yy | | : | | : yy | | : | | : yy | | : | +---+--+ -- 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] Problem with displaying "wide" tables in psql
Sergey Muraviov wrote: > I hope that I realized old-ascii logic correctly. I don't know what you changed here, but I don't think we need to preserve old-ascii behavior in the new code, in any way. If you're mimicking something obsolete and the end result of the new feature is not great, perhaps that should be rethought. Can you please provide sample output for the previous version compared to this new version? What changed? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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] [PATCH] Negative Transition Aggregate Functions (WIP)
Florian Pflug writes: > Yes, the idea had come up at some point during the review discussion. I > agree that it's only worthwhile if it works for state type internal - though > I think there ought to be a way to allow that. We could for example simply > decree that the initfunc's first parameter must be of type internal if it's > return type is, and then just pass NULL for that parameter. I had thought about that, but it doesn't really work since it'd be violating the strictness spec of the function. 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] Negative Transition Aggregate Functions (WIP)
Dean Rasheed writes: > On 10 April 2014 19:54, Tom Lane wrote: >> So if we go with that terminology, perhaps these names for the >> new CREATE AGGREGATE parameters: >> >> initfuncapplies to plain aggregation, mutually exclusive with >> initcond >> msfunc (or just mfunc?) forward transition for moving-agg mode >> mifunc inverse transition for moving-agg mode >> mstype state datatype for moving-agg mode >> msspace space estimate for mstype >> mfinalfunc final function for moving-agg mode >> minitfunc "firsttrans" for moving-agg mode >> minitcond mutually exclusive with minitfunc > Yeah, those work for me. > I think I prefer "mfunc" to "msfunc", but perhaps that's just my > natural aversion to the "ms" prefix :-) Meh. We've got mstype, and I don't think leaving out the "s" there feels right. > Also, perhaps "minvfunc" rather than "mifunc" because "i" by itself > could mean "initial". Good point. So with initfuncs out of the picture, we have new CREATE AGGREGATE parameter names msfunc forward transition for moving-agg mode minvfuncinverse transition for moving-agg mode mfinalfunc final function for moving-agg mode mstype state datatype for moving-agg mode msspace space estimate for mstype minitcond initial state value for moving-agg mode and new pg_aggregate columns aggmtransfn | regproc | not null aggminvtransfn| regproc | not null aggmfinalfn | regproc | not null aggmtranstype | oid | not null aggmtransspace| integer | not null aggminitval | text | It's a bit unfortunate that the catalog column names aren't quite on the same page as CREATE AGGREGATE, but it doesn't seem like a good idea to try to fix that now. 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] Negative Transition Aggregate Functions (WIP)
On Apr11, 2014, at 19:04 , Tom Lane wrote: > It strikes me that your second point > >> 2) It allows strict aggregates whose state type and input type aren't >> binary coercible to return NULL if all inputs were NULL without any >> special coding. As it stands today, this doesn't work without some >> kind of counter in the state, because the final function otherwise >> won't know if there were non-NULL inputs or not. Aggregates whose state >> and input types are binary coercible get around that by setting the >> initial value to NULL while *still* being strict, and relying on the >> state replacement behaviour of the aggregate machinery. > > could be addressed by the initfunc idea, but I'm still not sufficiently > excited by that one. Given the problem with internal-type transition > values, I think this could only win if there are cases where it would let > us use a regular SQL type instead of internal for the transition value; > and I'm not sure that there are many/any such cases. Yes, the idea had come up at some point during the review discussion. I agree that it's only worthwhile if it works for state type internal - though I think there ought to be a way to allow that. We could for example simply decree that the initfunc's first parameter must be of type internal if it's return type is, and then just pass NULL for that parameter. What I like about the initfunc idea is that it also naturally extends to ordered-set aggregates, I think it'd be very useful for some possible ordered-set aggregates to received their direct arguments beforehand and not afterwards. But that all seems largely orthogonal to the invtrans patch. best regards, Florian Pflug -- 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] Negative Transition Aggregate Functions (WIP)
Florian Pflug writes: > On Apr11, 2014, at 17:09 , Tom Lane wrote: >> Basically, this comes down to a design judgment call, and my judgment >> is differing from yours. In the absence of opinions from others, >> I'm going to do it my way. > Ok. Are you going to do the necessary changes, or shall I? I'm happy to > leave it to you, but if you lack the time I can try to find some. Nah, I'm happy to do the work, since it's me insisting on changing it. >> Tell me about the special case here again? Should we revisit the issue? > ... > I don't feel too strongly either way on this one - I initially implemented the > exception because I noticed that the NULL handling of some aggregates was > broken otherwise, and it seemed simpler to fix this in one place than going > over all the aggregates separately. OTOH, when I wrote the docs, I noticed > how hard it was to describe the behaviour accurately, which made me like it > less and less. And Dean wasn't happy with it at all, so that finally settled > it. Yeah, if you can't document the design nicely, it's probably not a good idea :-(. Thanks for the summary. It strikes me that your second point > 2) It allows strict aggregates whose state type and input type aren't > binary coercible to return NULL if all inputs were NULL without any > special coding. As it stands today, this doesn't work without some > kind of counter in the state, because the final function otherwise > won't know if there were non-NULL inputs or not. Aggregates whose state > and input types are binary coercible get around that by setting the > initial value to NULL while *still* being strict, and relying on the > state replacement behaviour of the aggregate machinery. could be addressed by the initfunc idea, but I'm still not sufficiently excited by that one. Given the problem with internal-type transition values, I think this could only win if there are cases where it would let us use a regular SQL type instead of internal for the transition value; and I'm not sure that there are many/any such cases. 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] Problem with displaying "wide" tables in psql
I hope that I realized old-ascii logic correctly. 2014-04-11 19:10 GMT+04:00 Greg Stark : > Looks good. > > It's still not doing the old-ascii column dividers but to be honest > I'm not sure what the intended behaviour of old-ascii is. I've noticed > old-ascii only displays the line markers for dividing lines, not the > final one. That seems pretty useless and maybe it's why we switched to > the new "ascii" mode, I don't recall. > > This is the way old-ascii displays when two columns are wrapping -- it > seems pretty confused to me. The headers are displaying newline > markers from the ascii style instead of : indicators in the dividing > line. The : and ; markers are only displayed for the first column, not > the second one. > > Maybe since the : and ; markers aren't shown for the final column that > means the expanded mode doesn't have to do anything since the column > is only ever the final column. > > > ++-+ > | x |x| > |+y |+ y| > |+z |+ z| > ++-+ > | x | xxx | > | xx ; | > | xxx: xxx | > | ; xxx | > | x : xxx | > | xx ; xx | > | xxx: xxx | > | ; x | > | x : xxx | > | xx : xx | > | xxx: x | > | : | > | x : xxx | > | xx : xx | > | xxx: x | > | : | > | x : xxx | > | xx : xx | > | xx : x | > | x : | > | xx : xxx | > | xx : xx | > | xx : x | > | xxx: | > | xx : | > | : | > | xx : | > | x : | > | xx : | > | xx | > | xx | > | xxx | > ++-+ > -- Best regards, Sergey Muraviov From 69d845f05864a5d07a90ee20e10a5cb09b78d1d3 Mon Sep 17 00:00:00 2001 From: Sergey Muraviov Date: Fri, 11 Apr 2014 20:55:17 +0400 Subject: [PATCH] Support for old-ascii mode --- src/bin/psql/print.c | 144 +++ 1 file changed, 122 insertions(+), 22 deletions(-) diff --git a/src/bin/psql/print.c b/src/bin/psql/print.c index 79fc43e..2c58cb5 100644 --- a/src/bin/psql/print.c +++ b/src/bin/psql/print.c @@ -1234,13 +1234,56 @@ print_aligned_vertical(const printTableContent *cont, FILE *fout) fprintf(fout, "%s\n", cont->title); } + if (cont->opt->format == PRINT_WRAPPED) + { + int output_columns = 0; + + /* + * Choose target output width: \pset columns, or $COLUMNS, or ioctl + */ + if (cont->opt->columns > 0) + output_columns = cont->opt->columns; + else + { + if (cont->opt->env_columns > 0) +output_columns = cont->opt->env_columns; +#ifdef TIOCGWINSZ + else + { +struct winsize screen_size; + +if (ioctl(fileno(stdout), TIOCGWINSZ, &screen_size) != -1) + output_columns = screen_size.ws_col; + } +#endif + } + + output_columns -= hwidth; + + if (opt_border == 0) + output_columns -= 1; + else + { + output_columns -= 3; /* -+- */ + + if (opt_border > 1) +output_columns -= 4; /* +--+ */ + } + + if (output_columns > 0 && dwidth > output_columns) + dwidth = output_columns; + } + /* print records */ for (i = 0, ptr = cont->cells; *ptr; i++, ptr++) { printTextRule pos; - int line_count, + int dline, + hline, dcomplete, - hcomplete; + hcomplete, + offset, + chars_to_output; if (cancel_pressed) break; @@ -1270,48 +1313,105 @@ print_aligned_vertical(const printTableContent *cont, FILE *fout) pg_wcsformat((const unsigned char *) *ptr, strlen(*ptr), encoding, dlineptr, dheight); - line_count = 0; + dline = hline = 0; dcomplete = hcomplete = 0; + offset = 0; + chars_to_output = dlineptr[dline].width; while (!dcomplete || !hcomplete) { + /* Left border */ if (opt_border == 2) -fprintf(fout, "%s ", dformat->leftvrule); +fputs(dformat->leftvrule, fout); + + /* Header */ if (!hcomplete) { -fprintf(fout, "%-s%*s", hlineptr[line_count].ptr, - hwidth - hlineptr[line_count].width, ""); +int swidth = hwidth - hlineptr[hline].width - 1; +fputs(hline? format->header_n
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
On Apr11, 2014, at 17:09 , Tom Lane wrote: > Basically, this comes down to a design judgment call, and my judgment > is differing from yours. In the absence of opinions from others, > I'm going to do it my way. Ok. Are you going to do the necessary changes, or shall I? I'm happy to leave it to you, but if you lack the time I can try to find some. >> ... SUM(int4) wouldn't need >> *any* extra state at all to be invertible, if it weren't for those pesky >> issues surrounding NULL handling. In fact, an earlier version of the >> invtrans patch *did* use int4_sum as SUM(int4)'s transfer functions! The >> only reason this doesn't work nowadays is that Dean didn't like the >> forward-nonstrict-but-inverse-strict special case that made this work. > > Tell me about the special case here again? Should we revisit the issue? My original coding allows the combination of non-strict forward with strict reverse transition functions. The combination behaved like a strict aggregate regarding NULL handling - i.e., neither the forward nor the reverse transition function received NULL inputs. But if the initial state was NULL, the forward transition function *would* be called with that NULL state value upon seeing the first non-NULL input. In the window case, the aggregation machinery would also take care to reset the state type to it's initial value when it removed the last non-NULL input from the aggregation state (just like it does for strict aggregates today). This had two advantages 1) First, it allows strict aggregates to use state type internal. As it stands now, aggregates with state type internal must implement their own NULL handling, even if they behave exactly like most standard aggregates do, namely ignore NULLS and return NULL only if there were no non-NULL inputs. 2) It allows strict aggregates whose state type and input type aren't binary coercible to return NULL if all inputs were NULL without any special coding. As it stands today, this doesn't work without some kind of counter in the state, because the final function otherwise won't know if there were non-NULL inputs or not. Aggregates whose state and input types are binary coercible get around that by setting the initial value to NULL while *still* being strict, and relying on the state replacement behaviour of the aggregate machinery. It, however, also has a few disadvantages 3) It means that one needs to look at the inverse transition function's strictness setting even if that function is never used. 4) It feels a bit hacky. (2) is what affects SUM(int4). The only reason it track the number of inputs is to be able to return NULL instead of 0 if no inputs remain. One idea to fix (3) and (4) was *explicitly* flagging aggregates as NULL-handling or NULL-ignoring, and also as what one might call "weakly strict", i.e. as returning NULL exactly if there were no non-NULL inputs. There are various variations of that theme possible - one flag for each behaviour, or simply a single "common behaviour" flag. In the end, I decided not to pursue that, mostly because the aggregates affected by that issued turned out to be relatively easy to fix. For the ones with state type internal, I simply added a counter, and I made SUM(int4) use AVG's transition function. I don't feel too strongly either way on this one - I initially implemented the exception because I noticed that the NULL handling of some aggregates was broken otherwise, and it seemed simpler to fix this in one place than going over all the aggregates separately. OTOH, when I wrote the docs, I noticed how hard it was to describe the behaviour accurately, which made me like it less and less. And Dean wasn't happy with it at all, so that finally settled it. best regards, Florian Pflug -- 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] Problem with displaying "wide" tables in psql
Looks good. It's still not doing the old-ascii column dividers but to be honest I'm not sure what the intended behaviour of old-ascii is. I've noticed old-ascii only displays the line markers for dividing lines, not the final one. That seems pretty useless and maybe it's why we switched to the new "ascii" mode, I don't recall. This is the way old-ascii displays when two columns are wrapping -- it seems pretty confused to me. The headers are displaying newline markers from the ascii style instead of : indicators in the dividing line. The : and ; markers are only displayed for the first column, not the second one. Maybe since the : and ; markers aren't shown for the final column that means the expanded mode doesn't have to do anything since the column is only ever the final column. ++-+ | x |x| |+y |+ y| |+z |+ z| ++-+ | x | xxx | | xx ; | | xxx: xxx | | ; xxx | | x : xxx | | xx ; xx | | xxx: xxx | | ; x | | x : xxx | | xx : xx | | xxx: x | | : | | x : xxx | | xx : xx | | xxx: x | | : | | x : xxx | | xx : xx | | xx : x | | x : | | xx : xxx | | xx : xx | | xx : x | | xxx: | | xx : | | : | | xx : | | x : | | xx : | | xx | | xx | | xxx | ++-+ stark=# select array_to_string(array_agg(repeat('x',x)),E'\n') as "x y z" from generate_series(1,20) as x(x); ┌─[ RECORD 1 ]─┐ │ x↵│ x ↵│ │ y↵│ xx ↵│ │ z │ xxx ↵│ │ │ ↵│ │ │ x ↵│ │ │ xx ↵│ │ │ xxx ↵│ │ │ ↵│ │ │ x ↵│ │ │ xx ↵│ │ │ xxx ↵│ │ │ ↵│ │ │ …│ │ │…x ↵│ │ │ …│ │ │…xx ↵│ │ │ …│ │ │…xxx ↵│ │ │ …│ │ │…↵│ │ │ …│ │ │…x ↵│ │ │ …│ │ │…xx ↵│ │ │ …│ │ │…xxx ↵│ │ │ …│ │ │… │ └───┴──┘ stark=# \pset linestyle ascii Line style (linestyle) is ascii. stark=# select array_to_string(array_agg(repeat('x',x)),E'\n') as "x y z" from generate_series(1,20) as x(x); stark"***# stark"***# stark-***# +-[ RECORD 1 ]-+ | x+| x +| | y+| xx +| | z | xxx +| | | +| | | x +| | | xx +| | | xxx +| | | +| | | x +| | | xx +| | | xxx +| | | +| | | .| | |.x +| | | .| | |.xx +| | | .| | |.xxx +| | | .| | |.+| | | .| | |.x +| | | .| | |.xx +| | | .| | |.xxx +| | | .| | |. | +---+--+ stark=# \pset linestyle old-ascii Line style (linestyle) is old-ascii. stark=# select array_to_string(array_agg(repeat('x',x)),E'\n') as "x y z" from generate_series(1,20) as x(x); stark"# stark"# stark-# +-[ RECORD 1 ]-+ | x | x| | y | xx | | z | xxx | | | | | | x| | | xx | | | xxx | | | | | | x| | | xx | | | xxx | | | | | | | | | x| | | | | | xx | | | | | | xxx | | | | | | | | | | | | x| | | | | | xx | | | | | | xxx | | | | | | | +---+--+ stark=# \pset expanded off Expanded display (expanded) is off. stark=# \pset columns 40 Target width (columns) is 40. stark=# select array_to_string(array_agg(repeat('x',x)),E'\n') as "x y z",array_to_string(array_agg(repeat(
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
Florian Pflug writes: > Which is why I feel that having two separate sets of transition functions > and state types solves the wrong problem. If we find a way to prevent > transition functions from being called directly, we'll shave a few cycles > of quite a few existing aggregates, invertible or not. If we find a way > around the initfunc-for-internal-statetype problem you discovered, the > implementation would get much simpler, since we could then make nearly > all of them strict. And this would again shave off a few cycles - for lots > of NULL inputs, the effect could even be large. Since neither of those latter things seems likely to happen, I don't find this argument convincing at all. Even if they did happen, they would represent an incremental improvement that would be equally useful with either one or two sfuncs. Basically, this comes down to a design judgment call, and my judgment is differing from yours. In the absence of opinions from others, I'm going to do it my way. However, quite independently of how many sfuncs there are, I'm interested about this: > ... SUM(int4) wouldn't need > *any* extra state at all to be invertible, if it weren't for those pesky > issues surrounding NULL handling. In fact, an earlier version of the > invtrans patch *did* use int4_sum as SUM(int4)'s transfer functions! The > only reason this doesn't work nowadays is that Dean didn't like the > forward-nonstrict-but-inverse-strict special case that made this work. Tell me about the special case here again? Should we revisit the issue? 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
[HACKERS] Signaling of waiting for a cleanup lock?
Hi, VACUUM sometimes waits synchronously for a cleanup lock on a heap page. Sometimes for a long time. Without reporting it externally. Rather confusing ;). Since we only take cleanup locks around vacuum, how about we report at least in pgstat that we're waiting? At the moment, there's really no way to know if that's what's happening. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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] WIP patch (v2) for updatable security barrier views
Stephen Frost writes: > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: >> Am I right in thinking that the "locking gotcha" only happens if you >> create a security_barrier view conaining a "SELECT ... FOR UPDATE"? If >> so, that seems like rather a niche case - not that that means we >> shouldn't warn people about it. > Hmm, the 'gotcha' I was referring to was the issue discussed upthread > around rows getting locked to be updated which didn't pass all the quals > (they passed the security_barrier view's, but not the user-supplied > ones), which could happen during a normal 'update' against a > security_barrier view, right? I didn't think that would require the > view definition to be 'FOR UPDATE'; if that's required then it would > seem like we're actually doing what the user expects based on their view > definition.. Yeah, the point of the "gotcha" is that a FOR UPDATE specified *outside* a security-barrier view would act as though it had appeared *inside* the view, since it effectively gets pushed down even though outer quals don't. 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] [feature] cached index to speed up specific queries on extremely large data sets
On Fri, Apr 11, 2014 at 2:12 PM, Michael Paquier wrote: > On Fri, Apr 11, 2014 at 9:53 PM, lkcl . wrote: >>> section: https://wiki.postgresql.org/wiki/Materialized_Views. > When updating a materialized view, or refreshing it, you need as well > to be aware that an exclusive lock is taken on it during the refresh > in 9.3, so the materialized view cannot be accessed for read queries. ok. as long as the storage of data (in the underlying table) is not adversely affected, that would be fine. as this is the hackers list and this turns out to be more a user question i'll leave it for now. >> awesome. uhhh, well that was easy *lol*. once i am paid, whom do i >> send the payment to for the fast response and incredibly valuable >> information? :) [this is a serious question!] > This can be helpful: > http://www.postgresql.org/about/donate/ thank you 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] [feature] cached index to speed up specific queries on extremely large data sets
On Fri, Apr 11, 2014 at 9:53 PM, lkcl . wrote: > On Fri, Apr 11, 2014 at 1:26 PM, Heikki Linnakangas > wrote: >> On 04/11/2014 03:20 PM, lkcl . wrote: >>> >>> so i had an idea. there already exists the concept of indexes. there >>> already exists the concept of "cached queries". question: would it be >>> practical to*merge* those two concepts such that specific queries >>> could be*updated* as new records are added, such that when the query >>> >>> is called again it answers basically pretty much immediately? let us >>> assume that performance degradation on "update" (given that indexes >>> already exist and are required to be updated) is acceptable. >> >> >> I think you just described materialized views. > > ... well... dang :) > > http://tech.jonathangardner.net/wiki/PostgreSQL/Materialized_Views > > ok so definitely not the snapshot materialised views, but yes! the > eager materialised views, definitely. > >> The built-in materialized >> views in PostgreSQL are not updated immediately as the tables are modified, > > ... but that would probably be enough. > >> but it's entirely possible to roll your own using views and triggers. There >> are a few links on the PostgreSQL wiki, in the "Versions before 9.3" >> section: https://wiki.postgresql.org/wiki/Materialized_Views. When updating a materialized view, or refreshing it, you need as well to be aware that an exclusive lock is taken on it during the refresh in 9.3, so the materialized view cannot be accessed for read queries. > awesome. uhhh, well that was easy *lol*. once i am paid, whom do i > send the payment to for the fast response and incredibly valuable > information? :) [this is a serious question!] This can be helpful: http://www.postgresql.org/about/donate/ -- 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] [feature] cached index to speed up specific queries on extremely large data sets
On Fri, Apr 11, 2014 at 1:26 PM, Heikki Linnakangas wrote: > On 04/11/2014 03:20 PM, lkcl . wrote: >> >> so i had an idea. there already exists the concept of indexes. there >> already exists the concept of "cached queries". question: would it be >> practical to*merge* those two concepts such that specific queries >> could be*updated* as new records are added, such that when the query >> >> is called again it answers basically pretty much immediately? let us >> assume that performance degradation on "update" (given that indexes >> already exist and are required to be updated) is acceptable. > > > I think you just described materialized views. ... well... dang :) http://tech.jonathangardner.net/wiki/PostgreSQL/Materialized_Views ok so definitely not the snapshot materialised views, but yes! the eager materialised views, definitely. > The built-in materialized > views in PostgreSQL are not updated immediately as the tables are modified, ... but that would probably be enough. > but it's entirely possible to roll your own using views and triggers. There > are a few links on the PostgreSQL wiki, in the "Versions before 9.3" > section: https://wiki.postgresql.org/wiki/Materialized_Views. awesome. uhhh, well that was easy *lol*. once i am paid, whom do i send the payment to for the fast response and incredibly valuable information? :) [this is a serious question!] l. -- 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] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of < 0
On Fri, Apr 11, 2014 at 10:03:08AM +0530, Amit Kapila wrote: > On Fri, Apr 11, 2014 at 10:00 AM, Amit Kapila wrote: > > On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian wrote: > >> On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote: > > > >> Ah, yes, good point. This is going to require backpatching then. > > > > I also think so. > > > >>> I think it's better to use check like below, just for matter of > >>> consistency with other place > >>> if (sock == INVALID_SOCKET) > >> > >> Agreed. That is how I have coded the patch. > > > > Sorry, I didn't checked the latest patch before that comment. > > > > I verified that your last patch is fine. Regression test also went fine. > > I have noticed small thing which I forgot to mention in previous mail. > I think below added extra line is not required. > > int > PQsocket(const PGconn *conn) > { > + Yes, I saw that yesterday and fixed it. I also did a dry run of backpatching and only 8.4 had conflicts, so I think we are good there. (This is like the readdir() fix all over again.) Once this is applied I will work on changing the libpq socket type to use portable pgsocket, but I am not planning to backpatch that unless we find a bug. -- Bruce Momjian http://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- 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] WIP patch (v2) for updatable security barrier views
* Craig Ringer (cr...@2ndquadrant.com) wrote: > > Hmm, the 'gotcha' I was referring to was the issue discussed upthread > > around rows getting locked to be updated which didn't pass all the quals > > (they passed the security_barrier view's, but not the user-supplied > > ones), which could happen during a normal 'update' against a > > security_barrier view, right? I didn't think that would require the > > view definition to be 'FOR UPDATE'; > > It doesn't require the view to be defined FOR UPDATE. Ok, great, glad I got that correct. :) > I'll try to write an isolstiontester case to donstrate this on the weekend. Great, thanks. I'll take a stab at writing up the 'gotcha' note tonight or tomorrow. Thanks again, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [feature] cached index to speed up specific queries on extremely large data sets
On 04/11/2014 03:20 PM, lkcl . wrote: so i had an idea. there already exists the concept of indexes. there already exists the concept of "cached queries". question: would it be practical to*merge* those two concepts such that specific queries could be*updated* as new records are added, such that when the query is called again it answers basically pretty much immediately? let us assume that performance degradation on "update" (given that indexes already exist and are required to be updated) is acceptable. I think you just described materialized views. The built-in materialized views in PostgreSQL are not updated immediately as the tables are modified, but it's entirely possible to roll your own using views and triggers. There are a few links on the PostgreSQL wiki, in the "Versions before 9.3" section: https://wiki.postgresql.org/wiki/Materialized_Views. - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [feature] cached index to speed up specific queries on extremely large data sets
hi folks, please cc me direct on responses as i am subscribed on digest. i've been asked to look at how to deal with around 7 billion records (appx 30 columns, appx data size total 1k) and this might have to be in a single system (i will need to Have Words with the client about that). the data is read-only and an arbitrary number of additional tables may be created to "manage" the data. records come in at a rate of around 25 million per day, the 7 billion records is based on the assumption of keeping one month's worth of data around. analysis of this data needs to be done across the entire set: i.e. it may not be subdivided into isolated tables (by day for example). i am therefore um rather concerned about efficiency, even just from the perspective of using 2nd normalised form and not doing JOINs against other tables. so i had an idea. there already exists the concept of indexes. there already exists the concept of "cached queries". question: would it be practical to *merge* those two concepts such that specific queries could be *updated* as new records are added, such that when the query is called again it answers basically pretty much immediately? let us assume that performance degradation on "update" (given that indexes already exist and are required to be updated) is acceptable. the only practical way (without digging into postgresql's c code) to do this at a higher level would be in effect to abandon the advantages of the postgresql query optimisation engine and *reimplement* it in a high-level language, subdividing the data into smaller (more manageable) tables, using yet more tables to store intermediate results of a previous query then somehow managing to stitch together a new response based on newer packets. it would be a complete nightmare to both implement and maintain. second question then based on whether the first is practical: is there anyone who would be willing (assuming it can be arranged) to engage in a contract to implement the required functionality? thanks, l. -- 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] WIP patch (v2) for updatable security barrier views
(Sorry if this breaks the thread history; on mobile) > > Am I right in thinking that the "locking gotcha" only happens if you > > create a security_barrier view conaining a "SELECT ... FOR UPDATE"? If > > so, that seems like rather a niche case - not that that means we > > shouldn't warn people about it. > > Hmm, the 'gotcha' I was referring to was the issue discussed upthread > around rows getting locked to be updated which didn't pass all the quals > (they passed the security_barrier view's, but not the user-supplied > ones), which could happen during a normal 'update' against a > security_barrier view, right? I didn't think that would require the > view definition to be 'FOR UPDATE'; It doesn't require the view to be defined FOR UPDATE. I'll try to write an isolstiontester case to donstrate this on the weekend. -- 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] PostgreSQL in Windows console and Ctrl-C
On 2014-04-11 07:12:50 -0400, Andrew Dunstan wrote: > > On 04/11/2014 01:35 AM, Amit Kapila wrote: > >On Fri, Apr 11, 2014 at 3:14 AM, Bruce Momjian wrote: > >>Can someone with Windows expertise comment on whether this should be > >>applied? > >I don't think this is a complete fix, for example what about platform where > >_CreateRestrictedToken() is not supported. For Example, the current > >proposed fix will not work for below case: > > > >if (_CreateRestrictedToken == NULL) > >{ > >/* > >* NT4 doesn't have CreateRestrictedToken, so just call ordinary > >* CreateProcess > >*/ > > > Are we really supporting NT4 any more? Even XP is about to be at end of > support from Microsoft. I seem to recall that win2k was already desupported? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, 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] PostgreSQL in Windows console and Ctrl-C
On 04/11/2014 01:35 AM, Amit Kapila wrote: On Fri, Apr 11, 2014 at 3:14 AM, Bruce Momjian wrote: Can someone with Windows expertise comment on whether this should be applied? I don't think this is a complete fix, for example what about platform where _CreateRestrictedToken() is not supported. For Example, the current proposed fix will not work for below case: if (_CreateRestrictedToken == NULL) { /* * NT4 doesn't have CreateRestrictedToken, so just call ordinary * CreateProcess */ Are we really supporting NT4 any more? Even XP is about to be at end of support from Microsoft. cheers andrew -- 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] WIP patch (v2) for updatable security barrier views
Dean, * Dean Rasheed (dean.a.rash...@gmail.com) wrote: > On 11 April 2014 04:04, Stephen Frost wrote: > > which changes it to "if a relation was changed to a subquery, it had > > better be because it's got securityQuals that we're dealing with". My > > general thinking here is that we'd be better off with the Assert() > > firing during some later development which changes things in this area > > than skipping the change because there aren't any securityQuals and then > > expecting everything to be fine with the subquery actually being a > > relation.. > > > > Yes, I think that makes sense, and it seems like a sensible check. Ok, I'll change that. > > I could see flipping that around too, to check if there are > > securityQuals and then Assert() on the change from relation to subquery- > > after all, if there are securityQuals then it *must* be a subquery, > > right? > > > > No, because a relation with securityQuals is only changed to a > subquery if it is actually used in the main query (see the check in > expand_security_quals). During the normal course of events when > working with an inheritance hierarchy, there are RTEs for other > children that aren't referred to in the main query for the current > inheritance child, and those RTEs are not expanded into subqueries. Ah, yes, makes sense. > For now though, the comments are correct, and it can only happen with > securityQuals. Adding an Assert() to that effect might be a bit fiddly > though, because the information required isn't available on the local > Node being processed, so I think it would have to add and maintain an > additional flag on the mutator context. I'm not sure that it's worth > it. Yeah, I was afraid that might be the case. No problem. > > Also, it seems like there should be a check_stack_depth() call here now? > > > > Hm, perhaps. I don't see any such checks in the rewriter though, and I > wouldn't expect the call depth here to get any deeper than it had > previously done there. Hmm, ok. I'll think on that one. > > That covers more-or-less everything outside of prepsecurity.c itself. > > I'm planning to review that tomorrow night. In general, I'm pretty > > happy with the shape of this. The wiki and earlier discussions were > > quite useful. My one complaint about this is that it feels like a few > > more comments and more documentation updates would be warrented; and in > > particular we need to make note of the locking "gotcha" in the docs. > > That's not a "solution", of course, but since we know about it we should > > probably make sure users are aware. > > Am I right in thinking that the "locking gotcha" only happens if you > create a security_barrier view conaining a "SELECT ... FOR UPDATE"? If > so, that seems like rather a niche case - not that that means we > shouldn't warn people about it. Hmm, the 'gotcha' I was referring to was the issue discussed upthread around rows getting locked to be updated which didn't pass all the quals (they passed the security_barrier view's, but not the user-supplied ones), which could happen during a normal 'update' against a security_barrier view, right? I didn't think that would require the view definition to be 'FOR UPDATE'; if that's required then it would seem like we're actually doing what the user expects based on their view definition.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Minor improvements in alter_table.sgml
(2014/04/09 12:03), Etsuro Fujita wrote: (2014/04/09 1:23), Robert Haas wrote: On Tue, Apr 8, 2014 at 5:05 AM, Etsuro Fujita wrote: Attached is a patch to improve the manual page for the ALTER TABLE command. Do we really need to add a section for "type_name" when we already have a section for "OF type_name"? I think that the section for "type_name" would be necessary as that in chapter "Parameters", not in chapter "Description", which includes the section for "OF type_name". constraint_name is also used for adding a constraint using an index. So it could not only be a constraint to alter, validate, or drop, but also a new constraint name to be added. I overlooked that. > Honestly, how much value is there in even having a section for this? Do we really want to document constraint_name as "name of an existing constraint, or the name of a new constraint to be added"? It would be accurate, then, but it also doesn't really tell you anything you didn't know already. You have a point there, but I feel odd about the documentation as is, because some are well written (eg, column_name) and some are not (eg, constraint_name). So, if there are no objections, I'd like to update the patch. Attached is an updated version of the patch. Thanks, Best regards, Etsuro Fujita diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml index 0b08f83..c16fc19 100644 --- a/doc/src/sgml/ref/alter_table.sgml +++ b/doc/src/sgml/ref/alter_table.sgml @@ -582,8 +582,8 @@ ALTER TABLE [ IF EXISTS ] name OWNER - This form changes the owner of the table, sequence, or view to the - specified user. + This form changes the owner of the table, sequence, view, materialized view, + or foreign table to the specified user. @@ -625,8 +625,9 @@ ALTER TABLE [ IF EXISTS ] name The RENAME forms change the name of a table - (or an index, sequence, or view), the name of an individual column in - a table, or the name of a constraint of the table. There is no effect on the stored data. + (or an index, sequence, view, materialized view, or foreign table), the name + of an individual column in a table, or the name of a constraint of the table. + There is no effect on the stored data. @@ -708,38 +709,47 @@ ALTER TABLE [ IF EXISTS ] name - new_name + constraint_name -New name for the table. +Name of a new or existing constraint. - type + new_constraint_name -Data type of the new column, or new data type for an existing -column. +New name for an existing constraint. - table_constraint + new_name -New table constraint for the table. +New name for the table. - constraint_name + data_type -Name of an existing constraint to drop. +Data type of the new column, or new data type for an existing +column. + + + + + + table_constraint + + +New table constraint for the table. @@ -799,6 +809,15 @@ ALTER TABLE [ IF EXISTS ] name + rewrite_rule_name + + +Name of a single rewrite rule to disable or enable. + + + + + index_name @@ -836,6 +855,15 @@ ALTER TABLE [ IF EXISTS ] name + type_name + + +The name of a composite type. + + + + + new_owner -- 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] Negative Transition Aggregate Functions (WIP)
On 10 April 2014 22:52, Tom Lane wrote: > Dean Rasheed writes: >> I was imagining that firsttrans would only be passed the first value >> to be aggregated, not any previous state, and that it would be illegal >> to specify both an initcond and a firsttrans function. > >> The forward transition function would only be called for values after >> the first, by which point the state would be non-null, and so it could >> be made strict in most cases. The same would apply to the invertible >> transition functions, so they wouldn't have to do null counting, which >> in turn would make their state types simpler. > > I put together a very fast proof-of-concept patch for this (attached). > It has a valid execution path for an aggregate with initfunc, but I didn't > bother writing the CREATE AGGREGATE support yet. I made sum(int4) work > as you suggest, marking the transfn strict and ripping out int4_sum's > internal support for null inputs. The result seems to be about a 4% or so > improvement in the overall aggregation speed, for a simple "SELECT > sum(int4col) FROM table" query. So from a performance standpoint this > seems only marginally worth doing. The real problem is not that 4% isn't > worth the trouble, it's that AFAICS the only built-in aggregates that > can benefit are sum(int2) and sum(int4). So that looks like a rather > narrow use-case. > > You had suggested upthread that we could use this idea to make the > transition functions strict for aggregates using "internal" transition > datatypes, but that does not work because the initfunc would violate > the safety rule that a function returning internal must take at least > one internal-type argument. That puts a pretty strong damper on the > usefulness of the approach, given how many internal-transtype aggregates > we have (and the moving-aggregate case is not going to be better is it?) > Ah, that's disappointing. If it can't be made to work for aggregates with internal state types, then I think it loses most of it's value, and I don't think it will be of much more use in the moving aggregate case either. > So at this point I'm feeling unexcited about the initfunc idea. > Unless it does something really good for the moving-aggregate case, > I think we should drop it. > Agreed. Thanks for prototyping it though. Regards, Dean -- 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] Using indices for UNION.
Hello. Thank you for the attentive comments. > I wrote: > > I still think this stuff mostly needs to be thrown away and rewritten > > in Path-creation style, but that's a long-term project. In the meantime > > this seems like a relatively small increment of complexity, so maybe it's > > worth applying. I'm concerned about the method for building a new > > DISTINCT clause though; the best that can be said for that is that it's > > a crude hack, and I'm less than convinced that there are no cases where > > it'll dump core. > > OK, after still more time thinking about it, here's the issues with that > way of generating a DISTINCT clause corresponding to the UNION: > > 1. Calling transformDistinctClause with a NULL pstate seems pretty unsafe. > It might accidentally fail to fail, today, but it's surely fragile. > It's violating the API contract not only of transformDistinctClause > itself (which is not documented as accepting a NULL pstate) but of > addTargetToGroupList (q.v.). Hmm I'm ashamed to have missed addTargetToGroupList. You are right about that. I saw only parser_errposition to field the NULL.. It should be fixed anyway. > A minimum requirement before I'd accept a > patch that did that is that it extended the header comments of those > functions to specify under what circumstances a NULL pstate can be passed. > However, that's not the direction to go, because of #2. > > 2. This approach potentially changes the semantics of the UNION. (This is > only important for a datatype that has more than one btree equality > operator, but let's posit that.) transformDistinctClause, as per its > header comment, allows the outer ORDER BY to influence which equality > operator it picks for DISTINCT. However, in the case of > "(SELECT ... UNION SELECT ...) ORDER BY ...", the UNION semantics were > chosen without reference to the ORDER BY, If my understanding is correct, the query is equivalent to it without the parens. The ORDER BY belongs to the topmost UNION on both cases. Actually planner compailns about "(SELECT...UNION SELECT ... ORDER BY) ORDER BY" that "multiple ORDER BY clauses not allowed" with/without this patch. > so it's possible that the > equality operators cited in the SetOperationStmt's groupClauses list are > not what you'd get from applying transformDistinctClause as the patch does. This patch flattenes and the SetOperationStmt was put out along with its groupClause. But the groupClauses is originally generated from targetlist in transformSetOperationTree. And the new DistinctClause is generated also from the same targetlist consulting to sortClause. They are not in the same order but it doesn't matter. Is it wrong? If it would make some trouble, I could add an check it out or count it on making the DistinctClauses.. Finally, explain (costs off) select a, b from c11 union select a, b from c12 order by b limit 1; |QUERY PLAN | - | Limit |-> Sort | Sort Key: c11.b | -> HashAggregate |Group Key: c11.b, c11.a |-> Append | -> Seq Scan on c11 | -> Seq Scan on c12 This HashAggregate does DISTINCT which was performed by using Sort-Unique without this patch, and yields the same result, I believe. > It is not okay for the planner to override the parser's choice of > semantics like that. As described above, as far as I saw, itself seems to be a problem. > Now I'm fairly sure that planner.c is expecting that the query's > distinctClause be a superset of the sortClause if any (cf comments for > SortGroupClause in parsenodes.h), so it wouldn't be okay to just blindly > build a distinctClause from topop->groupClauses. Yes, it could be but counting the sortClause into distinctClause is crucial factor for getting rid of unnecessary sorting. > I think what you need > to do is check that topop->groupClauses is compatible with the sortClause > if any (skipping the flattening transformation if not) and then build a > distinctClause by extending the sort clause list with any missing items > from topop->groupClauses. Ah, yes I agree as I described above. > So this is sort of like what > transformDistinctClause does, but different in detail, and the failure > case is to not do the transformation, rather than ereport'ing. (See also > generate_setop_grouplist, which you could almost use except that it's not > expecting to have to merge with a sortClause list.) Thank you. I'll follow this advise. > So this is all doable enough, but you're probably going to end up with > a new distinctClause-generating function that's at least twice the size > of the patch's former net code addition. So the question is whether it's > worth the trouble, considering that all this code has (I hope) a life > expectancy of no more than one or two more release cycles. Hmm. Since this is a kind of local optimization, some future drastic re
Re: [HACKERS] WIP patch (v2) for updatable security barrier views
On 11 April 2014 04:04, Stephen Frost wrote: > Dean, Craig, all, > > * Dean Rasheed (dean.a.rash...@gmail.com) wrote: >> This is reflected in the change to the regression test output where, >> in one of the tests, the ctids for the table to update are no longer >> coming from the same table. I think a better approach is to push down >> the rowmark into the subquery so that any locking required applies to >> the pushed down RTE --- see the attached patch. > > I'm working through this patch and came across a few places where I > wanted to ask questions (as much for my own edification as questioning > the actual implementation). Also, feel free to point out if I'm > bringing up something which has already been discussed. I've been > trying to follow the discussion but it's been a while and my memory may > have faded. > Thanks for looking at this. > While in the planner, we need to address the case of a child RTE which > has been transformed from a relation to a subquery. That all makes > perfect sense, but I'm wondering if it'd be better to change this > conditional: > > ! if (rte1->rtekind == RTE_RELATION && > ! rte1->securityQuals != NIL && > ! rte2->rtekind == RTE_SUBQUERY) > > which essentially says "if a relation was changed to a subquery *and* > it has security quals then we need to update the entry" into one like > this: > > ! if (rte1->rtekind == RTE_RELATION && > ! rte2->rtekind == RTE_SUBQUERY) > ! { > ! Assert(rte1->securityQuals != NIL); > ! ... > > which changes it to "if a relation was changed to a subquery, it had > better be because it's got securityQuals that we're dealing with". My > general thinking here is that we'd be better off with the Assert() > firing during some later development which changes things in this area > than skipping the change because there aren't any securityQuals and then > expecting everything to be fine with the subquery actually being a > relation.. > Yes, I think that makes sense, and it seems like a sensible check. > I could see flipping that around too, to check if there are > securityQuals and then Assert() on the change from relation to subquery- > after all, if there are securityQuals then it *must* be a subquery, > right? > No, because a relation with securityQuals is only changed to a subquery if it is actually used in the main query (see the check in expand_security_quals). During the normal course of events when working with an inheritance hierarchy, there are RTEs for other children that aren't referred to in the main query for the current inheritance child, and those RTEs are not expanded into subqueries. > A similar case exists in prepunion.c where we're checking if we should > recurse while in adjust_appendrel_attrs_mutator()- the check exists as > > ! if (IsA(node, Query)) > > (... which used to be an Assert(!IsA(node, Query)) ...) > > but the comment is then quite clear that we should only be doing this in > the security-barrier case; perhaps we should Assert() there to that > effect? It'd certainly make me feel a bit better about removing the two > Asserts() which were there; presumably we had to also remove the > Assert(!IsA(node, SubLink)) ? > When I originally wrote those comments, I thought that this change would only apply to securityQuals. Later, following a seemingly unrelated bug involving UPDATEs containing LATERAL references to the target relation (which is now disallowed), I thought that this change might help with that case too, if such UPDATEs were to be allowed again in the future (http://www.postgresql.org/message-id/CAEZATCXpOsF5wZ1XXWQur7G5M52=mwzuaqye9b0rgqhxvw3...@mail.gmail.com). For now though, the comments are correct, and it can only happen with securityQuals. Adding an Assert() to that effect might be a bit fiddly though, because the information required isn't available on the local Node being processed, so I think it would have to add and maintain an additional flag on the mutator context. I'm not sure that it's worth it. > Also, it seems like there should be a check_stack_depth() call here now? > Hm, perhaps. I don't see any such checks in the rewriter though, and I wouldn't expect the call depth here to get any deeper than it had previously done there. > That covers more-or-less everything outside of prepsecurity.c itself. > I'm planning to review that tomorrow night. In general, I'm pretty > happy with the shape of this. The wiki and earlier discussions were > quite useful. My one complaint about this is that it feels like a few > more comments and more documentation updates would be warrented; and in > particular we need to make note of the locking "gotcha" in the docs. > That's not a "solution", of course, but since we know about it we should > probably make sure users are aware. > Am I right in thinking that the "locking gotcha" only happens if you create a security_barr
Re: [HACKERS] Problem with displaying "wide" tables in psql
Hi. I've done some corrections for printing "newline" and "wrap" indicators. Please review the attached patch. 2014-04-11 0:14 GMT+04:00 Sergey Muraviov : > Hi. > > Thanks for your tests. > > I've fixed problem with headers, but got new one with data. > I'll try to solve it tomorrow. > > > 2014-04-10 18:45 GMT+04:00 Greg Stark : > > Ok, So I've hacked on this a bit. Below is a test case showing the >> problems I've found. >> >> 1) It isn't using the "newline" and "wrap" indicators or dividing lines. >> >> 2) The header is not being displayed properly when it contains a newline. >> >> I can hack in the newline and wrap indicators but the header >> formatting requires reworking the logic a bit. The header and data >> need to be stepped through in parallel rather than having a loop to >> handle the wrapping within the handling of a single line. I don't >> really have time for that today but if you can get to it that would be >> fine, >> > > > > -- > Best regards, > Sergey Muraviov > -- Best regards, Sergey Muraviov From 5e0f44994d04a81523920a78d3a35603e919170c Mon Sep 17 00:00:00 2001 From: Sergey Muraviov Date: Fri, 11 Apr 2014 11:03:41 +0400 Subject: [PATCH] Using "newline" and "wrap" indicators --- src/bin/psql/print.c | 130 +++ 1 file changed, 110 insertions(+), 20 deletions(-) diff --git a/src/bin/psql/print.c b/src/bin/psql/print.c index 79fc43e..6463162 100644 --- a/src/bin/psql/print.c +++ b/src/bin/psql/print.c @@ -1234,13 +1234,56 @@ print_aligned_vertical(const printTableContent *cont, FILE *fout) fprintf(fout, "%s\n", cont->title); } + if (cont->opt->format == PRINT_WRAPPED) + { + int output_columns = 0; + + /* + * Choose target output width: \pset columns, or $COLUMNS, or ioctl + */ + if (cont->opt->columns > 0) + output_columns = cont->opt->columns; + else + { + if (cont->opt->env_columns > 0) +output_columns = cont->opt->env_columns; +#ifdef TIOCGWINSZ + else + { +struct winsize screen_size; + +if (ioctl(fileno(stdout), TIOCGWINSZ, &screen_size) != -1) + output_columns = screen_size.ws_col; + } +#endif + } + + output_columns -= hwidth; + + if (opt_border == 0) + output_columns -= 1; + else + { + output_columns -= 3; /* -+- */ + + if (opt_border > 1) +output_columns -= 4; /* +--+ */ + } + + if (output_columns > 0 && dwidth > output_columns) + dwidth = output_columns; + } + /* print records */ for (i = 0, ptr = cont->cells; *ptr; i++, ptr++) { printTextRule pos; - int line_count, + int dline, + hline, dcomplete, - hcomplete; + hcomplete, + offset, + chars_to_output; if (cancel_pressed) break; @@ -1270,48 +1313,95 @@ print_aligned_vertical(const printTableContent *cont, FILE *fout) pg_wcsformat((const unsigned char *) *ptr, strlen(*ptr), encoding, dlineptr, dheight); - line_count = 0; + dline = hline = 0; dcomplete = hcomplete = 0; + offset = 0; + chars_to_output = dlineptr[dline].width; while (!dcomplete || !hcomplete) { + /* Left border */ if (opt_border == 2) fprintf(fout, "%s ", dformat->leftvrule); + + /* Header */ if (!hcomplete) { -fprintf(fout, "%-s%*s", hlineptr[line_count].ptr, - hwidth - hlineptr[line_count].width, ""); +int swidth = hwidth - hlineptr[hline].width - 1; +fprintf(fout, "%-s", hlineptr[hline].ptr); +if (swidth > 0) /* spacer */ + fprintf(fout, "%*s", swidth, ""); -if (!hlineptr[line_count + 1].ptr) +if (!hlineptr[hline + 1].ptr) +{ + fputs(" ", fout); hcomplete = 1; +} +else +{ + fputs(format->nl_right, fout); + hline++; +} } else -fprintf(fout, "%*s", hwidth, ""); +fprintf(fout, "%*s", hwidth + 1, ""); + /* Separator */ if (opt_border > 0) -fprintf(fout, " %s ", dformat->midvrule); - else -fputc(' ', fout); +fprintf(fout, "%s", dformat->midvrule); + /* Data */ if (!dcomplete) { -if (opt_border < 2) - fprintf(fout, "%s\n", dlineptr[line_count].ptr); +int target_width, + bytes_to_output, + swidth; + +fputs(!dcomplete && !offset? " ": format->wrap_left, fout); + +target_width = dwidth; +bytes_to_output = strlen_max_width(dlineptr[dline].ptr + offset, + &target_width, encoding); +fputnbytes(fout, (char *)(dlineptr[dline].ptr + offset), + bytes_to_output); + +chars_to_output -= target_width; +offset += bytes_to_output; + +/* spacer */ +swidth = dwidth - target_width; +if (swidth > 0) + fprintf(fout, "%*s", swidth, ""); + +if (!chars_to_output) +{ + if (!dlineptr[dline + 1].ptr) + { + fputs(" ", fout); + dcomplete = 1; + } + else + { + fputs(format->nl_right, fout); + dline++; + offset = 0; + chars_to_output = dlineptr[dline].width; + } +} else - fprintf(fout, "%-s%*s %s\n", dlineptr[li