Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
On Thu, Apr 10, 2014 at 9:55 AM, Tom Lane t...@sss.pgh.pa.us wrote: I'm pretty sure David Rowley did some benchmarking. The results should be in this thread somewhere I think, but they currently evade me... Maybe David can re-post, if he's following this... I saw benchmarks addressing window aggregation, but none looking for side-effects on plain aggregation. These ones maybe? http://www.postgresql.org/message-id/CAApHDvr_oSpvM-XXz43eCMX8n0EfshJ=9j+rxvgqcy91yr-...@mail.gmail.com I think it was only around SUM(numeric), and you'll need to scroll down a bit to find it as it's mixed in with a load of window agg tests. At the time it was the only forward transition function that I had added any overhead to, so I think it was the only one I bothered to test at the time. However, I do remember benchmarking the bool_and and bool_or changes after I rewrote the way they worked and also found the same as you... no difference, I just can't find the post with my results... Though it sounds like Tom found the same as me so I don't think it's worth me looking any more for them. Regards David Rowley
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
On Thu, Apr 10, 2014 at 9:55 AM, Tom Lane t...@sss.pgh.pa.us wrote: Florian Pflug f...@phlo.org writes: I was (and still am) not in favour of duplicating the whole quadruple of (state, initialvalue, transferfunction, finalfunction) because it seems excessive. In fact, I believed that doing this would probably be grounds for outright rejection of the patch, on the base of catalog bloat. And your initial response to this suggestion seemed to confirm this. Well, I think it's much more likely that causing a performance penalty for cases unrelated to window aggregates would lead to outright rejection :-(. The majority of our users probably don't ever use window functions, but for sure they've heard of SUM(). We can't penalize the non-window case. Expanding pg_aggregate from 10 columns (as per patch) to 14 (as per this suggestion) is a little annoying but it doesn't sound like a show stopper. It seems reasonable to assume that the extra initval would be NULL in most cases, so it's probably a net addition of 12 bytes per row. I also wouldn't imagine that the overhead of storing that would be too great... And are there really any databases out there that have 1000's of custom aggregate functions? I'm actually quite glad to see someone agrees with me on this. I think it opens up quite a bit of extra optimisation opportunities with things like MAX and MIN... In these cases we could be tracking the number of values of max found and reset it when we get a bigger value. That way we could report the inverse transition as successful if maxcount is still above 0 after the removal of a max value... Similar to how I implemented the inverse transition for sum(numeric). In fact doing it this way would mean that inverse transitions for sum(numeric) would never fail and retry. I just thought we had gotten to a stage of not requiring this due to the overheads being so low... I was quite surprised to see the count tracking account for 5% for sum int. What I don't quite understand yet is why we can't just create a new function for int inverse transitions instead of borrowing the inverse transition functions for avg...? Regards David Rowley
Re: [HACKERS] WAL replay bugs
I executed given steps many times to produce this bug. But still I unable to hit this bug. I used attached scripts to produce this bug. Can I get scripts to produce this bug? wal_replay_bug.sh http://postgresql.1045698.n5.nabble.com/file/n5799512/wal_replay_bug.sh - Thanks and Regards, Sachin Kotwal NTT-DATA-OSS Center (Pune) -- View this message in context: http://postgresql.1045698.n5.nabble.com/WAL-replay-bugs-tp5799053p5799512.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.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] WAL replay bugs
On 04/10/2014 10:52 AM, sachin kotwal wrote: I executed given steps many times to produce this bug. But still I unable to hit this bug. I used attached scripts to produce this bug. Can I get scripts to produce this bug? wal_replay_bug.sh http://postgresql.1045698.n5.nabble.com/file/n5799512/wal_replay_bug.sh Oh, I can't reproduce it using that script either. I must've used some variation of it, and posted wrong script. The attached seems to do the trick. I changed the INSERT statements slightly, so that all the new rows have the same key. Thanks for verifying this! - Heikki wal_replay_bug.sh Description: application/shellscript -- 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] Get more from indices.
(2014/04/10 0:08), Tom Lane wrote: Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp writes: Oops! I found a bug in this patch. The previous v8 patch missed the case that build_index_pathkeys() could build a partial pathkeys from the index tlist. TBH I think that's barely the tip of the iceberg of cases where this patch will get the wrong answer. Also, I don't see it doing anything to check the ordering of multiple index columns I think that the following code in index_pathkeys_are_extensible() would check the ordering: + if (!pathkeys_contained_in(pathkeys, root-query_pathkeys)) + return false; Also, what's with the success return before the loop: + if (list_length(pathkeys) == list_length(root-query_pathkeys)) + return true; At this point you haven't proven *anything at all* about whether the index columns have something to do with the query_pathkeys. I think that the two pathkeys would be proved to be equal, if the both conditions are satisfied. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS]
On 10 April 2014 11:18, Pavan Deolasee Wrote: I could think of few global variables like transaction properties related(i.e. read-only mode, isolation level etc). As I plan to keep transaction properties of autonomous transaction same as main transaction, so there is no need to have these global variables separately. Apart from this there are global variables like with-in transaction counters, GUC, xactStartTimeStamp. I think there is no need to maintain these variables also separately. They can continue from previous value for autonomous transaction also similar to as sub-transaction does. Hmm. Is that in line with what other databases do ? I would have preferred AT to run like a standalone transaction without any influence of the starting transaction, managing its own resources/locks/visibility/triggers etc. To me it seems it is not very useful to keep the transaction properties separate except the read-only properties (though oracle does not share any transaction properties). So we can have restriction that isolation and deferrable properties of main transaction will be inherited by autonomous transaction but read-only properties can be defined independently by autonomous transaction. Which looks to be fair restriction according to me. In order to keep read-only properties separate, there is already infrastructure in PG. Inside the structure TransactionStateData, there is variable prevXactReadOnly (entry-time xact r/o state), which can keep the parent transaction read only properties and XactReadOnly can be changed to current transaction properties. Moreover we can take this (transaction properties) as a feature enhancement also once a basic infrastructure is established, if acceptable to everyone. Autonomous transaction will not share resource/lock/visibility etc with main transaction. This has been already taken care in WIP patch. In-case of autonomous transaction, only specific global variables initialized are related to resources (similar to sub-transaction), which anyway gets stored in current transaction state. Please let me know if I am missing something or if you have some specific global variables related issue. No, I don't have any specific issues in mind. Mostly all such global state is managed through various AtStart/AtEOX and related routines. So a careful examination of all those routines will give a good idea what needs to be handled. You probably will require to write AtATStart/AtATEOX and similar routines to manage the state at AT start/commit/rollback. Sorry, I haven't looked at your WIP patch yet. For some of the resources, I have already written AtATStart/AtATEOX kind of routines in WIP patch. Comments/feedbacks/doubts are welcome. Thanks and Regards, Kumar Rajeev Rastogi
Re: [HACKERS] small typo about comment in xlog.c
On 04/10/2014 07:19 AM, Tomonari Katsumata wrote: Hi, I'm reading xlog.c, and I noticed a comment of do_pg_abort_backup is typo. ... 10247 * NB: This is only for aborting a non-exclusive backup that doesn't write 10248 * backup_label. A backup started with pg_stop_backup() needs to be finished 10249 * with pg_stop_backup(). ... I think A backup started with pg_stop_backup() should be A backup started with pg_start_backup(). This is a bug about source comment, so it's not big problem. But I want to fix the comment. See attached patch. Fixed, thanks. - Heikki -- 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] Autonomous Transaction (WIP)
On 10 April 2014 11:18, Pavan Deolasee Wrote: I could think of few global variables like transaction properties related(i.e. read-only mode, isolation level etc). As I plan to keep transaction properties of autonomous transaction same as main transaction, so there is no need to have these global variables separately. Apart from this there are global variables like with-in transaction counters, GUC, xactStartTimeStamp. I think there is no need to maintain these variables also separately. They can continue from previous value for autonomous transaction also similar to as sub-transaction does. Hmm. Is that in line with what other databases do ? I would have preferred AT to run like a standalone transaction without any influence of the starting transaction, managing its own resources/locks/visibility/triggers etc. To me it seems it is not very useful to keep the transaction properties separate except the read-only properties (though oracle does not share any transaction properties). So we can have restriction that isolation and deferrable properties of main transaction will be inherited by autonomous transaction but read-only properties can be defined independently by autonomous transaction. Which looks to be fair restriction according to me. In order to keep read-only properties separate, there is already infrastructure in PG. Inside the structure TransactionStateData, there is variable prevXactReadOnly (entry-time xact r/o state), which can keep the parent transaction read only properties and XactReadOnly can be changed to current transaction properties. Moreover we can take this (transaction properties) as a feature enhancement also once a basic infrastructure is established, if acceptable to everyone. Autonomous transaction will not share resource/lock/visibility etc with main transaction. This has been already taken care in WIP patch. In-case of autonomous transaction, only specific global variables initialized are related to resources (similar to sub-transaction), which anyway gets stored in current transaction state. Please let me know if I am missing something or if you have some specific global variables related issue. No, I don't have any specific issues in mind. Mostly all such global state is managed through various AtStart/AtEOX and related routines. So a careful examination of all those routines will give a good idea what needs to be handled. You probably will require to write AtATStart/AtATEOX and similar routines to manage the state at AT start/commit/rollback. Sorry, I haven't looked at your WIP patch yet. For some of the resources, I have already written AtATStart/AtATEOX kind of routines in WIP patch. Comments/feedbacks/doubts are welcome. Thanks and Regards, Kumar Rajeev Rastogi
Re: [HACKERS] WIP patch (v2) for updatable security barrier views
On 4/9/14 9:56 PM, Stephen Frost wrote: As for docs and testing, those are things we would certainly be better off with and may mean that this isn't able to make it into 9.4, which is fair, but I wouldn't toss it out solely due to that. We have a git repo with multiple worked out code examples, ones that have been in active testing for months now. It's private just to reduce mistakes as things are cleared for public consumption, but I (and Mark and Jeff here) can pull anything we like from it to submit to hackers. There's also a test case set from Yeb Havinga he used for his review. I expected to turn at least one of those into a Postgres regression test. The whole thing squealed to a stop when the regression tests Craig was working on were raising multiple serious questions. I didn't see much sense in bundling more boring, passing tests when the ones we already had seemed off--and no one was sure why. Now that Tom has given some guidance on the row locking weirdness, maybe it's time for me to start updating those into make check form. The documentation has been in a similar holding pattern. I have lots of resources to help document what does and doesn't work here to the quality expected in the manual. I just need a little more confidence about which feature set is commit worthy. The documentation that makes sense is very different if only updatable security barrier views is committed. -- Greg Smith greg.sm...@crunchydatasolutions.com Chief PostgreSQL Evangelist - http://crunchydatasolutions.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] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0
On Thu, Apr 10, 2014 at 11:05:49AM +0530, Amit Kapila wrote:
On Tue, Apr 8, 2014 at 11:32 PM, Bruce Momjian br...@momjian.us wrote:
On Sun, Apr 6, 2014 at 11:45:59AM +0530, Amit Kapila wrote:
In fact, this C program compiled by gcc on Debian issues no compiler
warnings and returns 'hello', showing that -1 and ~0 compare as equal:
int
main(int argc, char **argv)
{
int i;
unsigned int j;
i = -1;
j = ~0;
if (i == j)
printf(hello\n);
return 0;
}
I have add below code to check it's usage as per PG:
if (j 0)
printf(hello-1\n);
It doesn't print hello-1, which means that all the check's in code
for sock_desc 0 can have problem.
Ah, yes, good point. This is going to require backpatching then.
1.
int
pg_foreach_ifaddr(PgIfAddrCallback callback, void *cb_data)
sock = WSASocket(AF_INET, SOCK_DGRAM, 0, 0, 0, 0);
if (sock == SOCKET_ERROR)
Well, the actual problem here is that WSASocket() returns INVALID_SOCKET
per the documentation, not SOCKET_ERROR. I did not use PGINVALID_SOCKET
here because this is Windows-specific code, defining 'sock' as SOCKET.
We could have sock defined as pgsocket, but because this is Windows code
already, it doesn't seem wise to mix portability code in there.
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.
--
Bruce Momjian br...@momjian.ushttp://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
[HACKERS] Adding unsigned 256 bit integers
I was wondering if there would be any way to do the following in PostgreSQL: UPDATE cryptotable SET work = work + 'some big hexadecimal number' where work is an unsigned 256 bit integer. Right now my column is a character varying(64) column (hexadecimal representation of the number) but I would be happy to switch to another data type if it lets me do the operation above. If it's not possible with vanilla PostgreSQL, are there extensions that could help me? -- - Oli Olivier Lalonde http://www.syskall.com -- connect with me! Freelance web and Node.js engineer Skype: o-lalonde
Re: [HACKERS] Get more from indices.
Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: (2014/04/10 0:08), Tom Lane wrote: TBH I think that's barely the tip of the iceberg of cases where this patch will get the wrong answer. Also, I don't see it doing anything to check the ordering of multiple index columns I think that the following code in index_pathkeys_are_extensible() would check the ordering: + if (!pathkeys_contained_in(pathkeys, root-query_pathkeys)) + return false; Hm ... if you're relying on that, then what's the point of the new loop at all? 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] Adding unsigned 256 bit integers
On 04/10/2014 09:13 AM, Olivier Lalonde wrote: I was wondering if there would be any way to do the following in PostgreSQL: UPDATE cryptotable SET work = work + 'some big hexadecimal number' where work is an unsigned 256 bit integer. Right now my column is a character varying(64) column (hexadecimal representation of the number) but I would be happy to switch to another data type if it lets me do the operation above. If it's not possible with vanilla PostgreSQL, are there extensions that could help me? The numeric type allows numbers with huge numbers of digits. I've used it to calculate fibonacci numbers thousands of digits long. 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] Adding unsigned 256 bit integers
On 04/10/2014 09:13 PM, Olivier Lalonde wrote: I was wondering if there would be any way to do the following in PostgreSQL: UPDATE cryptotable SET work = work + 'some big hexadecimal number' For readers finding this in the archives, this question also appears here: http://dba.stackexchange.com/questions/62934/adding-unsigned-256-bit-integers-in-postgresql -- Craig Ringer 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
[HACKERS] Partial match fix for fast scan
Hi, GIN partial match appears to be broken after fast scan. Following simple test case raises assertion failure. create extension btree_gin; create table test as (select id, random() as val from generate_series(1,100) id); create index test_idx on test using gin (val); vacuum test; select * from test where val between 0.1 and 0.9; Attached patch fixes bugs in entryGetItem function. I would especially point that continue; checks while condition even if it's postfix while. That's why I surrounded tbm_iterate with another while. -- With best regards, Alexander Korotkov. ginfastscanfix.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] Adding unsigned 256 bit integers
On Thu, Apr 10, 2014 at 09:13:47PM +0800, Olivier Lalonde wrote: I was wondering if there would be any way to do the following in PostgreSQL: UPDATE cryptotable SET work = work + 'some big hexadecimal number' where work is an unsigned 256 bit integer. Right now my column is a character varying(64) column (hexadecimal representation of the number) but I would be happy to switch to another data type if it lets me do the operation above. If it's not possible with vanilla PostgreSQL, are there extensions that could help me? -- - Oli Olivier Lalonde http://www.syskall.com -- connect with me! Hi Olivier, Here are some sample pl/pgsql helper functions that I have written for other purposes. They use integers but can be adapted to use numeric. Regards, Ken --- CREATE OR REPLACE FUNCTION hex2dec(t text) RETURNS integer AS $$ DECLARE r RECORD; BEGIN FOR r IN EXECUTE 'SELECT x'''||t||'''::integer AS hex' LOOP RETURN r.hex; END LOOP; END $$ LANGUAGE plpgsql IMMUTABLE STRICT; --- --- CREATE OR REPLACE FUNCTION bytea2int ( in_string BYTEA ) RETURNS INTEGER AS $$ DECLARE b1 INTEGER := 0; b2 INTEGER := 0; b3 INTEGER := 0; b4 INTEGER := 0; out_int INTEGER := 0; BEGIN CASE OCTET_LENGTH(in_string) WHEN 1 THEN b4 := get_byte(in_string, 0); WHEN 2 THEN b3 := get_byte(in_string, 0); b4 := get_byte(in_string, 1); WHEN 3 THEN b2 := get_byte(in_string, 0); b3 := get_byte(in_string, 1); b4 := get_byte(in_string, 2); WHEN 4 THEN b1 := get_byte(in_string, 0); b2 := get_byte(in_string, 1); b3 := get_byte(in_string, 2); b4 := get_byte(in_string, 3); END CASE; out_int := (b1 24) + (b2 16) + (b3 8) + b4; RETURN(out_int); END; $$ LANGUAGE plpgsql IMMUTABLE; --- -- 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 dean.a.rash...@gmail.com writes: On 10 April 2014 01:13, Florian Pflug f...@phlo.org wrote: However, I still believe the best approach at this point is to just work on making int4_avg_accum faster. I still see no principal reason what it has to be noticeably slower - the only additional work it absolutely *has* to perform is *one* 64-bit increment. In the best case that would make sum() not noticeably slower than avg(), whereas using a firsttrans/initialfunction would potentially make both of them faster than they currently are, and not just in window queries. I'm still of the opinion that we should separate the transfn for invertible cases from the normal one, and allow for two separate state types. One of the things that helps with is the strictness consideration: you no longer have to have the same strictness setting for the plain and invertible forward transfns. This idea of a separate firsttrans function is interesting but perhaps orthogonal to the current patch. Also, I don't quite understand how it would work for aggregates with null initvalues; don't you end up with exactly the same conflict about how you can't mark the transfn strict? Or is the idea that firsttrans would *only* apply to aggregates with null initvalue, and so you wouldn't even pass the previous state value to 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] Problem with displaying wide tables in psql
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,
postgres=***# \pset
Border style (border) is 2.
Target width (columns) is 20.
Expanded display (expanded) is on.
Field separator (fieldsep) is |.
Default footer (footer) is on.
Output format (format) is wrapped.
Line style (linestyle) is unicode.
Null display (null) is .
Locale-adjusted numeric output (numericlocale) is off.
Pager (pager) usage is off.
Record separator (recordsep) is newline.
Table attributes (tableattr) unset.
Title (title) unset.
Tuples only (tuples_only) is off.
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 │ │
│ │ yy │
│ y │ │
│ │ │
│ 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 | |
| | yy |
| y | |
| | |
| z | |
| | yy |
| | |
| | yy |
| | |
| | yy |
| | |
| | yy |
| | |
+---+--+
postgres=***# \pset columns 30
Target width (columns) is 30.
postgres=***# \pset linestyle unicode
Line style (linestyle) is unicode.
postgres=***# \pset columns 30
Target width (columns) is 30.
postgres=***# \pset expanded off
Expanded display (expanded) is off.
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);
┌───┬┐
│ x↵│ x ↵│
│ y │ y ↵│
│ │ z│
├───┼┤
│ x↵│ yy…│
│ xx ↵│… ↵│
│ xxx ↵│ yy…│
│ ↵│…yy↵│
│ x↵│ yy↵│
│ xx ↵│ ↵│
│ xxx ↵│ yy↵│
│ ↵│ ↵│
│ x↵│ yy↵│
│ x…│ ↵│
│…x │ yy↵│
│ ││
└───┴┘
(1 row)
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);
+---++
| x+| x +|
| y | y +|
| | z|
+---++
| x+| yy.|
| xx +|. +|
| xxx +| yy.|
| +|.yy+|
| x+| yy+|
| xx +| +|
| xxx +| yy+|
| +| +|
| x+| yy+|
| x.| +|
|.x | 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] Partial match fix for fast scan
On Thu, Apr 10, 2014 at 11:09 AM, Alexander Korotkov aekorot...@gmail.comwrote: Hi, GIN partial match appears to be broken after fast scan. Following simple test case raises assertion failure. create extension btree_gin; create table test as (select id, random() as val from generate_series(1,100) id); create index test_idx on test using gin (val); vacuum test; select * from test where val between 0.1 and 0.9; Attached patch fixes bugs in entryGetItem function. I would especially point that continue; checks while condition even if it's postfix while. That's why I surrounded tbm_iterate with another while. Interesting... to me (using current master) your test case doesn't fail... fabrizio=# select * from test where val between 0.1 and 0.9; id |val +--- 1 | 0.554413774050772 2 | 0.767866868525743 3 | 0.601187175605446 ... But fail if I change the values of between clause: fabrizio=# select * from test where val between 0.1 and 0.19; ERROR: tuple offset out of range: 8080 Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog sobre TI: http://fabriziomello.blogspot.com Perfil Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello
Re: [HACKERS] psql \d+ and oid display
On Wed, Apr 9, 2014 at 11:42 AM, Bruce Momjian br...@momjian.us wrote: On Wed, Apr 9, 2014 at 09:27:11AM -0400, Robert Haas wrote: On Wed, Apr 9, 2014 at 1:02 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Well, that's sorta my concern. I mean, right now we've got people saying what the heck is a replica identity?. But, if the logical decoding stuff becomes popular, as I hope it will, that's going to be an important thing for people to adjust, and the information needs to be present in a clear and easily-understood way. I haven't studied the current code in detail so maybe it's fine. I just want to make sure we're not giving it second-class treatment solely on the basis that it's new and people aren't using it yet. I think the proposal is don't mention the property if it has the default value. That's not second-class status, as long as people who know what the property is understand that behavior. It's just conserving screen space. One thing that concerns me is that replica identity has a different default for system tables (NOTHING) than for other tables (DEFAULT). So when we say we're not going to display the default value, are we going to display it when it's not NOTHING, when it's not DEFAULT, or when it's not the actual default for that particular kind of table? We exclude pg_catalog from displaying Replica Identity due to this inconsistency. I assume this was desired because you can't replicate system tables. Is that true? The current test is: if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm') /* * No need to display default values; we already display a * REPLICA IDENTITY marker on indexes. */ tableinfo.relreplident != 'd' tableinfo.relreplident != 'i' strcmp(schemaname, pg_catalog) != 0) What might make more sense is this: if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm') /* * No need to display default values; we already display a * REPLICA IDENTITY marker on indexes. */ tableinfo.relreplident != 'i' ((strcmp(schemaname, pg_catalog) != 0 tableinfo.relreplident != 'd') || (strcmp(schemaname, pg_catalog) == 0 tableinfo.relreplident != 'n'))) Well, this is why I think we should just display it always. I don't think users are going to remember the exact algorithm for whether or not the line gets displayed, so you're just putting yourself in a situation where the \d+ output doesn't actually inform the user. If you want to leave it out when it's default and show the none line for catalog tables, that's OK by me too. But calling one line of output that displays important information clutter and only appears when the user explicitly requests verbose mode (with \d+ rather than \d) strikes me as as silly. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- 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] psql \d+ and oid display
On Thu, Apr 10, 2014 at 12:23:40PM -0400, Robert Haas wrote: What might make more sense is this: if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm') /* * No need to display default values; we already display a * REPLICA IDENTITY marker on indexes. */ tableinfo.relreplident != 'i' ((strcmp(schemaname, pg_catalog) != 0 tableinfo.relreplident != 'd') || (strcmp(schemaname, pg_catalog) == 0 tableinfo.relreplident != 'n'))) Well, this is why I think we should just display it always. I don't think users are going to remember the exact algorithm for whether or not the line gets displayed, so you're just putting yourself in a situation where the \d+ output doesn't actually inform the user. If you want to leave it out when it's default and show the none line for catalog tables, that's OK by me too. But calling one line of output that displays important information clutter and only appears when the user explicitly requests verbose mode (with \d+ rather than \d) strikes me as as silly. The issue is that none is the default for system tables and default is the default for user tables. Tom complained about the none for the pg_depend display. I am starting to think I am not going to make everyone happy and we just need to vote. Frankly, I think there are enough people who want this conditionally displayed that I don't even need a vote. -- Bruce Momjian br...@momjian.ushttp://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] Minor performance improvement in transition to external sort
On 6 February 2014 18:21, Jeff Janes jeff.ja...@gmail.com wrote: On Tue, Feb 4, 2014 at 2:22 PM, Jeremy Harris j...@wizmail.org wrote: The attached patch replaces the existing siftup method for heapify with a siftdown method. Tested with random integers it does 18% fewer compares and takes 10% less time for the heapify, over the work_mem range 1024 to 1048576. Thanks for working on this. +1 Your patch isn't linked properly from the CF manager though. If you like patches like this then there's a long(er) list of optimizations already proposed previously around sorting. It would be good to have someone work through them for external sorts. I believe Noah is working on parallel internal sort (as an aside). There's also an optimization possible for merge joins where we use the output of the first sort as an additional filter on the second sort. That can help when we're going to join two disjoint tables. -- Simon Riggs 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] psql \d+ and oid display
If it's conditional I think when it matches a guc is too hard for users to use. I think say nothing if oids are off and say something of their on would be fine. It would result in clutter for users which oids on by default but that's a non default setting. And the consequences of having oids on when they're intended to be off are much more likely to be missed our forgotten and need a reminder. If they're off when they need to be on you'll surely notice... -- greg On 10 Apr 2014 12:45, Bruce Momjian br...@momjian.us wrote: On Thu, Apr 10, 2014 at 12:23:40PM -0400, Robert Haas wrote: What might make more sense is this: if ((tableinfo.relkind == 'r' || tableinfo.relkind == 'm') /* * No need to display default values; we already display a * REPLICA IDENTITY marker on indexes. */ tableinfo.relreplident != 'i' ((strcmp(schemaname, pg_catalog) != 0 tableinfo.relreplident != 'd') || (strcmp(schemaname, pg_catalog) == 0 tableinfo.relreplident != 'n'))) Well, this is why I think we should just display it always. I don't think users are going to remember the exact algorithm for whether or not the line gets displayed, so you're just putting yourself in a situation where the \d+ output doesn't actually inform the user. If you want to leave it out when it's default and show the none line for catalog tables, that's OK by me too. But calling one line of output that displays important information clutter and only appears when the user explicitly requests verbose mode (with \d+ rather than \d) strikes me as as silly. The issue is that none is the default for system tables and default is the default for user tables. Tom complained about the none for the pg_depend display. I am starting to think I am not going to make everyone happy and we just need to vote. Frankly, I think there are enough people who want this conditionally displayed that I don't even need a vote. -- Bruce Momjian br...@momjian.ushttp://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] psql \d+ and oid display
On Thu, Apr 10, 2014 at 01:05:32PM -0400, Greg Stark wrote: If it's conditional I think when it matches a guc is too hard for users to use. Yes, we gave up on having the OID display match the GUC; we just display something if and only if it oids are present. Robert is talking about the Identity Replica display, which is new in 9.4 and should match how we display oids. Right now Identity Replica only displays something if the user changed the default. I think say nothing if oids are off and say something of their on would be fine. It would result in clutter for users which oids on by default but that's a non default setting. Yes, that is what the proposed patch does. -- Bruce Momjian br...@momjian.ushttp://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] [PATCH] Negative Transition Aggregate Functions (WIP)
On 10 April 2014 15:18, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 01:13, Florian Pflug f...@phlo.org wrote: However, I still believe the best approach at this point is to just work on making int4_avg_accum faster. I still see no principal reason what it has to be noticeably slower - the only additional work it absolutely *has* to perform is *one* 64-bit increment. In the best case that would make sum() not noticeably slower than avg(), whereas using a firsttrans/initialfunction would potentially make both of them faster than they currently are, and not just in window queries. I'm still of the opinion that we should separate the transfn for invertible cases from the normal one, and allow for two separate state types. One of the things that helps with is the strictness consideration: you no longer have to have the same strictness setting for the plain and invertible forward transfns. Yes, I can imagine that there would be some aggregates for which the plain forwards transition function would be simpler than the invertible one, with a simpler state type. You'd still be left with quite a large number of existing aggregates having non-strict plain transition functions, in addition to a bunch of new non-strict invertible transition functions that had to do null counting. This idea of a separate firsttrans function is interesting but perhaps orthogonal to the current patch. Also, I don't quite understand how it would work for aggregates with null initvalues; don't you end up with exactly the same conflict about how you can't mark the transfn strict? Or is the idea that firsttrans would *only* apply to aggregates with null initvalue, and so you wouldn't even pass the previous state value to it? 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. 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] [PATCH] Negative Transition Aggregate Functions (WIP)
Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 15:18, Tom Lane t...@sss.pgh.pa.us wrote: This idea of a separate firsttrans function is interesting but perhaps orthogonal to the current patch. Also, I don't quite understand how it would work for aggregates with null initvalues; don't you end up with exactly the same conflict about how you can't mark the transfn strict? Or is the idea that firsttrans would *only* apply to aggregates with null initvalue, and so you wouldn't even pass the previous state value to it? 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. Got it. So the existing behavior where we insert the first non-null value could be seen as a degenerate case in which the firsttrans function is an identity. 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. Makes sense to me. We need names for these things though. I don't think abbreviating to ffunc is a good idea because of the likelihood of confusion with the finalfunc (indeed I see the CREATE AGGREGATE ref page is already using ffunc as a short form for finalfunc). Maybe initfunc, which would parallel initcond? What about names for the invertible-aggregate infrastructure? I'm tempted to prefix inv to all the existing names, but then invsfunc means the alternate forward function ... can we use invifunc for the inverse transition function? Or maybe the prefix should be just i. 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 10 April 2014 19:04, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 15:18, Tom Lane t...@sss.pgh.pa.us wrote: This idea of a separate firsttrans function is interesting but perhaps orthogonal to the current patch. Also, I don't quite understand how it would work for aggregates with null initvalues; don't you end up with exactly the same conflict about how you can't mark the transfn strict? Or is the idea that firsttrans would *only* apply to aggregates with null initvalue, and so you wouldn't even pass the previous state value to it? 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. Got it. So the existing behavior where we insert the first non-null value could be seen as a degenerate case in which the firsttrans function is an identity. Right. 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. Makes sense to me. We need names for these things though. I don't think abbreviating to ffunc is a good idea because of the likelihood of confusion with the finalfunc (indeed I see the CREATE AGGREGATE ref page is already using ffunc as a short form for finalfunc). Maybe initfunc, which would parallel initcond? Yes, I was already thinking that initfunc is actually a much better name for that. What about names for the invertible-aggregate infrastructure? I'm tempted to prefix inv to all the existing names, but then invsfunc means the alternate forward function ... can we use invifunc for the inverse transition function? Or maybe the prefix should be just i. Hmm, I'm not a fan of any of those names. Perhaps win as a prefix to denote a sliding window? Or just m for moving aggregate. 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] [PATCH] Negative Transition Aggregate Functions (WIP)
Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 19:04, Tom Lane t...@sss.pgh.pa.us wrote: What about names for the invertible-aggregate infrastructure? I'm tempted to prefix inv to all the existing names, but then invsfunc means the alternate forward function ... can we use invifunc for the inverse transition function? Or maybe the prefix should be just i. Hmm, I'm not a fan of any of those names. Perhaps win as a prefix to denote a sliding window? Or just m for moving aggregate. Hmm ... moving aggregate is actually a pretty good name for this whole feature -- better than invertible aggregate anyway. I can feel a global-search-and-replace coming on. 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 That takes us up to 16 columns in pg_aggregate, but it's still not going to be a very voluminous catalog --- there's only 171 rows there today. So I'm not particularly concerned about space, and if there's a chance of squeezing out cycles, I think we should seize 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] Partial match fix for fast scan
On Thu, Apr 10, 2014 at 8:22 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Thu, Apr 10, 2014 at 11:09 AM, Alexander Korotkov aekorot...@gmail.com wrote: Hi, GIN partial match appears to be broken after fast scan. Following simple test case raises assertion failure. create extension btree_gin; create table test as (select id, random() as val from generate_series(1,100) id); create index test_idx on test using gin (val); vacuum test; select * from test where val between 0.1 and 0.9; Attached patch fixes bugs in entryGetItem function. I would especially point that continue; checks while condition even if it's postfix while. That's why I surrounded tbm_iterate with another while. Interesting... to me (using current master) your test case doesn't fail... fabrizio=# select * from test where val between 0.1 and 0.9; id |val +--- 1 | 0.554413774050772 2 | 0.767866868525743 3 | 0.601187175605446 ... But fail if I change the values of between clause: fabrizio=# select * from test where val between 0.1 and 0.19; ERROR: tuple offset out of range: 8080 It must be compiled with --enable-cassert to fail on assertion. -- With best regards, Alexander Korotkov.
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
On 10 April 2014 19:54, Tom Lane t...@sss.pgh.pa.us wrote: Dean Rasheed dean.a.rash...@gmail.com writes: On 10 April 2014 19:04, Tom Lane t...@sss.pgh.pa.us wrote: What about names for the invertible-aggregate infrastructure? I'm tempted to prefix inv to all the existing names, but then invsfunc means the alternate forward function ... can we use invifunc for the inverse transition function? Or maybe the prefix should be just i. Hmm, I'm not a fan of any of those names. Perhaps win as a prefix to denote a sliding window? Or just m for moving aggregate. Hmm ... moving aggregate is actually a pretty good name for this whole feature -- better than invertible aggregate anyway. I can feel a global-search-and-replace coming on. 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 :-) Also, perhaps minvfunc rather than mifunc because i by itself could mean initial. 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] Problem with displaying wide tables in psql
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 st...@mit.edu: 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
Re: [HACKERS] Partial match fix for fast scan
On 04/10/2014 10:00 PM, Alexander Korotkov wrote: On Thu, Apr 10, 2014 at 8:22 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: On Thu, Apr 10, 2014 at 11:09 AM, Alexander Korotkov aekorot...@gmail.com wrote: GIN partial match appears to be broken after fast scan. Following simple test case raises assertion failure. create extension btree_gin; create table test as (select id, random() as val from generate_series(1,100) id); create index test_idx on test using gin (val); vacuum test; select * from test where val between 0.1 and 0.9; Attached patch fixes bugs in entryGetItem function. I would especially point that continue; checks while condition even if it's postfix while. That's why I surrounded tbm_iterate with another while. Interesting... to me (using current master) your test case doesn't fail... fabrizio=# select * from test where val between 0.1 and 0.9; id |val +--- 1 | 0.554413774050772 2 | 0.767866868525743 3 | 0.601187175605446 ... But fail if I change the values of between clause: fabrizio=# select * from test where val between 0.1 and 0.19; ERROR: tuple offset out of range: 8080 It must be compiled with --enable-cassert to fail on assertion. I'm actually getting the tuple offset out of range with Fabrizio's query, even after your fix (not every time, run it a few times, launching a new connection each time). So there's another bug lurking there. The problem seems to be that even though we've checked in the innermost loop that not all of the items on the page are = advancePast, that situation can change later if advancePast is advanced. So on next invocation entryGetItem, the loop to skip past advancePast might read bogus offsets in the array. I pushed a patch, which includes Alexander's fix, and also fixes the second issue. - Heikki -- 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] trailing comment ghost-timing
On Mon, Mar 31, 2014 at 02:06:28PM -0400, Bruce Momjian wrote: Where are we on this? It seem odd that psql sends /* */ comments to the server, but not -- comments. Should this be documented or changed? I am confused why changing the behavior would affect the regression test output as -- and /* */ comments already appear, and it was stated that -- comments are already not sent to the server. Everyone agreed that suppressing \timing output for a PGRES_EMPTY_QUERY return result was not a good idea. I have applied the attached document patch to document that '--' comments are not passed to the server, while C-style block comments are. We can call this a feature. ;-) -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml new file mode 100644 index 5dce06a..85899d7 *** a/doc/src/sgml/ref/psql-ref.sgml --- b/doc/src/sgml/ref/psql-ref.sgml *** testdb=gt; *** 679,684 --- 679,690 xref linkend=SQL-LISTEN and xref linkend=SQL-NOTIFY. /para + + para + While C-style block comments are passed to the server for + processing and removal, SQL-standard comments are removed by + applicationpsql/application. + /para /refsect2 refsect2 id=APP-PSQL-meta-commands -- 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 Apr10, 2014, at 02:13 , Florian Pflug f...@phlo.org wrote: On Apr9, 2014, at 23:17 , Florian Pflug f...@phlo.org wrote: On Apr9, 2014, at 21:35 , Tom Lane t...@sss.pgh.pa.us wrote: A quick test says that avg(int4) is about five percent slower than sum(int4), so that's the kind of hit we'd be taking on non-windowed aggregations if we do it like this. That's rather surprising though. AFAICS, there's isn't much difference between the two transfer functions int4_sum and int4_avg_accum at all. The differences come down to (+ denoting things which ought to make int4_avg_accum slower compared to int4_sum, - denoting the opposite) 1. +) int4_avg_accum calls AggCheckCallContext 2. -) int4_sum checks if the state is NULL (it never is for int4_avg_accum) 3. +) int4_avg_accum uses ARR_HASNULL, ARR_SIZE, ARR_OVERHEAD_NONULLS to verify that the state is a 2-element array without NULL entries 4. -) int4_sum checks if the input is NULL I've done a bit of profiling on this (using Instruments.app on OSX). One thing I missed is that inv4_avg_accum also calls pg_detoast_datum - that calls comes from the PG_GETARG_ARRAYTYPE_P macro. Doing that is a bit silly, since we know that the datum cannot possibly be toasted I think (or if it was, nodeAgg.c should do the de-toasting). The profile also attributes a rather large percent of the total runtime of int4_avg_accum (around 30%) to the call of AggCheckCallContext(). Getting rid of that would help quite a few transition functions, invertible or not. That certainly seems doable - we'd need a way to mark functions as internal support functions, and prevent user-initiated calls of such functions. Transition functions marked that way could then safely scribble over their state arguments without having to consult AggCheckCallContext() first. ... However, I still believe the best approach at this point is to just work on making int4_avg_accum faster. I still see no principal reason what it has to be noticeably slower - the only additional work it absolutely *has* to perform is *one* 64-bit increment. I played with this a bit. Currently, int4_avg_accum invokes AggCheckCallContext every time, and also repeatedly checks whether the array has the required dimension - which, incidentally, is the only big difference between int4_avg_accum and int4_sum. To avoid that, I added a flags field to fcinfo which nodeAgg uses to tell transition functions whether they're called for the first time, or are being called with whatever state they themselves returned last time, i.e the n-th time. If the n-th time flag is set, int4_avg_accum simply retrieves the state with PG_GETARG_DATUM() instead of PG_GETARG_ARRAYTYPE_P(), relying on the fact that it never returns toasted datums itself, and also doesn't bother to validate the array size, for the same reason. If the flag is not set, it uses PG_GETARG_ARRAYTYPE_COPY_P and does validate the array size. In theory, it could further distinguish between a 1st call in an aggregation context (where the copy is unnecessary), and a user-initiated call (i.e. outside an aggregation). But that seems unnecessary - one additional copy per aggregation group seems unlikely to be a problem. With this in place, instruction-level profiling using Apple's Instruments.app shows that int4_avg_accum takes about 1.5% of the total runtime of a simple aggregation, while int4_sum takes about 1.2%. A (very rough) patch is attached. I haven't been able to repeat Tom's measurement which shows a 5% difference in performance between the invertible and the non-invertible versions of SUM(int4), so I cannot say if this removes that. But the profiling I've done would certainly indicate it should. best regards, Florian Pflug invtrans_sumint4_opt2.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
Can someone with Windows expertise comment on whether this should be
applied?
---
On Tue, Jan 7, 2014 at 12:44:33PM +0100, Christian Ullrich wrote:
Hello all,
when pg_ctl start is used to run PostgreSQL in a console window on
Windows, it runs in the background (it is terminated by closing the
window, but that is probably inevitable). There is one problem,
however: The first Ctrl-C in that window, no matter in which
situation, will cause the background postmaster to exit. If you,
say, ping something, and press Ctrl-C to stop ping, you probably
don't want the database to go away, too.
The reason is that Windows delivers the Ctrl-C event to all
processes using that console, not just to the foreground one.
Here's a patch to fix that. pg_ctl stop still works, and it has no
effect when running as a service, so it should be safe. It starts
the postmaster in a new process group (similar to calling setpgrp()
after fork()) that does not receive Ctrl-C events from the console
window.
--
Christian
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
new file mode 100644
index 50d4586..89a9544
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
*** CreateRestrictedProcess(char *cmd, PROCE
*** 1561,1566
--- 1561,1567
HANDLE restrictedToken;
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
SID_AND_ATTRIBUTES dropSids[2];
+ DWORD flags;
/* Functions loaded dynamically */
__CreateRestrictedToken _CreateRestrictedToken = NULL;
*** CreateRestrictedProcess(char *cmd, PROCE
*** 1636,1642
AddUserToTokenDacl(restrictedToken);
#endif
! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE,
CREATE_SUSPENDED, NULL, NULL, si, processInfo);
Kernel32Handle = LoadLibrary(KERNEL32.DLL);
if (Kernel32Handle != NULL)
--- 1637,1650
AddUserToTokenDacl(restrictedToken);
#endif
! flags = CREATE_SUSPENDED;
!
! /* Protect console process from Ctrl-C */
! if (!as_service) {
! flags |= CREATE_NEW_PROCESS_GROUP;
! }
!
! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE,
flags, NULL, NULL, si, processInfo);
Kernel32Handle = LoadLibrary(KERNEL32.DLL);
if (Kernel32Handle != NULL)
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Bruce Momjian br...@momjian.ushttp://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] [PATCH] Negative Transition Aggregate Functions (WIP)
Dean Rasheed dean.a.rash...@gmail.com 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?)
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.
regards, tom lane
diff --git a/src/backend/catalog/pg_aggregate.c b/src/backend/catalog/pg_aggregate.c
index fe6dc8a..1ca5c8f 100644
*** a/src/backend/catalog/pg_aggregate.c
--- b/src/backend/catalog/pg_aggregate.c
*** AggregateCreate(const char *aggName,
*** 390,395
--- 390,396
values[Anum_pg_aggregate_aggfnoid - 1] = ObjectIdGetDatum(procOid);
values[Anum_pg_aggregate_aggkind - 1] = CharGetDatum(aggKind);
values[Anum_pg_aggregate_aggnumdirectargs - 1] = Int16GetDatum(numDirectArgs);
+ values[Anum_pg_aggregate_agginitfn - 1] = ObjectIdGetDatum(InvalidOid);
values[Anum_pg_aggregate_aggtransfn - 1] = ObjectIdGetDatum(transfn);
values[Anum_pg_aggregate_aggfinalfn - 1] = ObjectIdGetDatum(finalfn);
values[Anum_pg_aggregate_aggsortop - 1] = ObjectIdGetDatum(sortop);
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 7e4bca5..2671c49 100644
*** a/src/backend/executor/nodeAgg.c
--- b/src/backend/executor/nodeAgg.c
*** typedef struct AggStatePerAggData
*** 152,157
--- 152,158
int numTransInputs;
/* Oids of transfer functions */
+ Oid initfn_oid; /* may be InvalidOid */
Oid transfn_oid;
Oid finalfn_oid; /* may be InvalidOid */
*** typedef struct AggStatePerAggData
*** 160,165
--- 161,167
* corresponding oid is not InvalidOid. Note in particular that fn_strict
* flags are kept here.
*/
+ FmgrInfo initfn;
FmgrInfo transfn;
FmgrInfo finalfn;
*** typedef struct AggHashEntryData
*** 296,301
--- 298,306
static void initialize_aggregates(AggState *aggstate,
AggStatePerAgg peragg,
AggStatePerGroup pergroup);
+ static void initialize_transition_value(AggState *aggstate,
+ AggStatePerAgg peraggstate,
+ AggStatePerGroup pergroupstate);
static void advance_transition_function(AggState *aggstate,
AggStatePerAgg peraggstate,
AggStatePerGroup pergroupstate);
*** initialize_aggregates(AggState *aggstate
*** 403,408
--- 408,498
}
/*
+ * Initialize the transition value upon reaching the first non-NULL input(s).
+ *
+ * We use this code when the initcond is NULL and the transfn is strict,
+ * which otherwise would mean the transition value can never become non-null.
+ * If an initfn has been provided, call it on the current input value(s);
+ * otherwise, take the current input value as the new transition value.
+ * (In the latter case, we already checked that this is okay datatype-wise.)
+ */
+ static void
+ initialize_transition_value(AggState *aggstate,
+ AggStatePerAgg peraggstate,
+ AggStatePerGroup pergroupstate)
+ {
+ FunctionCallInfo tfcinfo = peraggstate-transfn_fcinfo;
+ MemoryContext oldContext;
+
+ if (OidIsValid(peraggstate-initfn_oid))
+ {
+ FunctionCallInfoData fcinfo;
+ int numInitArgs;
+ Datum newVal;
+
+ /* We run the transition functions in per-input-tuple memory
Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)
On Apr10, 2014, at 21:34 , Dean Rasheed dean.a.rash...@gmail.com wrote:
On 10 April 2014 19:54, Tom Lane t...@sss.pgh.pa.us 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
I think I prefer mfunc to msfunc, but perhaps that's just my
natural aversion to the ms prefix :-)
Also, perhaps minvfunc rather than mifunc because i by itself
could mean initial.
I still think you're getting ahead of yourselves here. The number of
aggregates which benefit from this is tiny SUM(int2,int4) and maybe
BOOL_{AND,OR}. And in the SUM(int2,int4) case *only* on 64-bit archs -
for the others, the state type is already pass-by-ref.
I don't think we should be additional that much additional machinery
until it has been conclusively demonstrated that the performance
regression cannot be fixed any other way. Which, quite frankly, I
don't believe. Nothing in int4_avg_accum looks particularly expensive,
and the things that *do* seem to cost something certainly can be made
cheaper. c.f. the patch I just posted.
Another reason I'm so opposed to this is that inverse transition
functions might not be the last kind of transition functions we ever
add. For example, if we ever get ROLLUP/CUBE, we might want to have
a mergefunc which takes two aggregation states and combines them
into one. What do we do if we add those? Add yet a another set of
mergable transition functions? What about the various combinations
of invertible/non-invertible mergable/non-mergable that could result?
The opportunity cost seems pretty high here...
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 f...@phlo.org writes:
I still think you're getting ahead of yourselves here. The number of
aggregates which benefit from this is tiny SUM(int2,int4) and maybe
BOOL_{AND,OR}. And in the SUM(int2,int4) case *only* on 64-bit archs -
for the others, the state type is already pass-by-ref.
That argument is reasonable for the initfunc idea, but it doesn't apply
otherwise.
Another reason I'm so opposed to this is that inverse transition
functions might not be the last kind of transition functions we ever
add. For example, if we ever get ROLLUP/CUBE, we might want to have
a mergefunc which takes two aggregation states and combines them
into one. What do we do if we add those?
Make more pg_aggregate columns. What exactly do you think is either
easier or harder about such cases? Also, maybe I'm misremembering
the spec, but ROLLUP/CUBE wouldn't apply to window functions would
they? So if your argument is based on the assumption that these
features need to combine, I'm not sure it's true.
The bottom line for me is that it seems conceptually far cleaner to
make the moving-aggregate support be independent than to insist that
it share an stype and sfunc with the plain case.
Furthermore, I do not buy the argument that if we hack hard enough,
we can make the performance cost of forcing the sfunc to do double duty
negligible. In the first place, that argument is unsupported by much
evidence, and in the second place, it will certainly cost us complexity
to make the performance issue go away. Instead we can just design the
problem out, for nothing that I see as a serious drawback.
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 00:07 , Tom Lane t...@sss.pgh.pa.us wrote:
Florian Pflug f...@phlo.org writes:
I still think you're getting ahead of yourselves here. The number of
aggregates which benefit from this is tiny SUM(int2,int4) and maybe
BOOL_{AND,OR}. And in the SUM(int2,int4) case *only* on 64-bit archs -
for the others, the state type is already pass-by-ref.
That argument is reasonable for the initfunc idea, but it doesn't apply
otherwise.
Why not? AFAICS, the increase in cost comes from going from an
by-value to a by-reference state type. Once you're using a by-refence
type, you already pay the overhead of the additional dereferences, and
for calling AggCheckCallContext() or some equivalent.
Another reason I'm so opposed to this is that inverse transition
functions might not be the last kind of transition functions we ever
add. For example, if we ever get ROLLUP/CUBE, we might want to have
a mergefunc which takes two aggregation states and combines them
into one. What do we do if we add those?
Make more pg_aggregate columns. What exactly do you think is either
easier or harder about such cases? Also, maybe I'm misremembering
the spec, but ROLLUP/CUBE wouldn't apply to window functions would
they? So if your argument is based on the assumption that these
features need to combine, I'm not sure it's true.
Well, it was just an example - there might be other future extensions
which *do* need to combine. And we've been known to go beyond the spec
sometimes...
Furthermore, I do not buy the argument that if we hack hard enough,
we can make the performance cost of forcing the sfunc to do double duty
negligible. In the first place, that argument is unsupported by much
evidence, and in the second place, it will certainly cost us complexity
to make the performance issue go away. Instead we can just design the
problem out, for nothing that I see as a serious drawback.
My argument is that is costs us more complexity to duplicate everything
for the invertible case, *and* the result seems less flexible - not
from the POV of aggregate implementations, but from the POV of future
extensions.
As for evidence - have you looked at the patch I posted? I'd be very
interested to know if it removes the performance differences you saw.
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] psql \d+ and oid display
On Tue, Apr 1, 2014 at 10:45:29AM -0700, Jeff Janes wrote: I am suggesting it for at least some other things. I'm rather aggrieved that \d+ without argument shows you the size and the description/comment for every table, but \d+ foo does not show you the size and description/comment of the specific table you just asked for. Not so aggrieved that I wrote and submitted a patch, you might notice; but I'll get to it eventually if no one beats me to it. Agree, that is kind of odd. Should that be a TODO? -- Bruce Momjian br...@momjian.ushttp://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] [PATCH] Negative Transition Aggregate Functions (WIP)
Florian Pflug f...@phlo.org writes: My argument is that is costs us more complexity to duplicate everything for the invertible case, *and* the result seems less flexible - not from the POV of aggregate implementations, but from the POV of future extensions. [ shrug... ] You can argue against any feature whatsoever by claiming that it might possibly conflict with something we would wish to do sometime in the future. I think you need to have a much more concrete argument about specific issues this will cause in order to be convincing. For all we know about ROLLUP/CUBE implementation issues right now, doing this feature with separate implementations might make that easier not harder. (I note that the crux of my complaint right now is that we're asking sfuncs to serve two masters --- how's it going to be better when they have to serve three or four?) As for evidence - have you looked at the patch I posted? I'd be very interested to know if it removes the performance differences you saw. (1) You can't really prove the absence of a performance issue by showing that one specific aggregate doesn't have an issue. (2) These results (mine as well as yours) seem mighty platform-dependent, so the fact you don't see an issue doesn't mean someone else won't. (3) A new FunctionCallInfoData field just for this? Surely not. There's got to be a distributed cost to that (although I notice you didn't bother initializing the field most places, which is cheating). Now point 3 could be addressed by doing the signaling in some other way with the existing context field. But it's still the case that you're trying to band-aid a bad design. There's no good reason to make the sfunc do extra work to be invertible in contexts where we know, with certainty, that that work is useless. Especially not when we know that even a few instructions of extra work can be performance-significant. 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] psql \d+ and oid display
On Thu, Apr 10, 2014 at 01:10:35PM -0400, Bruce Momjian wrote: On Thu, Apr 10, 2014 at 01:05:32PM -0400, Greg Stark wrote: If it's conditional I think when it matches a guc is too hard for users to use. Yes, we gave up on having the OID display match the GUC; we just display something if and only if it oids are present. Robert is talking about the Identity Replica display, which is new in 9.4 and should match how we display oids. Right now Identity Replica only displays something if the user changed the default. I think say nothing if oids are off and say something of their on would be fine. It would result in clutter for users which oids on by default but that's a non default setting. Yes, that is what the proposed patch does. I talked to Robert via voice to understand his concerns. He clearly explained the complexity of how Replica Identity is set. I, of course, am concerned about user confusion in showing these values. What I did was develop the attached patch which clarifies the default for system and non-system tables, documents when psql displays it, and improves the display for pg_catalog tables that aren't equal to NONE. It also has changed the OID status to only display if it exists. One question that came up with Robert is whether OID status should appear for \d as well, now that is only shows up when present. I am hopeful this patch is closer to general agreement. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml new file mode 100644 index 0b08f83..ac6a4a4 *** a/doc/src/sgml/ref/alter_table.sgml --- b/doc/src/sgml/ref/alter_table.sgml *** ALTER TABLE [ IF EXISTS ] replaceable c *** 608,619 para This form changes the information which is written to the write-ahead log to identify rows which are updated or deleted. This option has no effect ! except when logical replication is in use. literalDEFAULT/ records the old values of the columns of the primary key, if any. literalUSING INDEX/ records the old values of the columns covered by the named index, which must be unique, not partial, not deferrable, and include only columns marked literalNOT NULL/. literalFULL/ records the old values of all columns in the row. literalNOTHING/ records no information about the old row. In all cases, no old values are logged unless at least one of the columns that would be logged differs between the old and new versions of the row. /para --- 608,621 para This form changes the information which is written to the write-ahead log to identify rows which are updated or deleted. This option has no effect ! except when logical replication is in use. literalDEFAULT/ ! (the default for non-system tables) records the old values of the columns of the primary key, if any. literalUSING INDEX/ records the old values of the columns covered by the named index, which must be unique, not partial, not deferrable, and include only columns marked literalNOT NULL/. literalFULL/ records the old values of all columns in the row. literalNOTHING/ records no information about the old row. + (This is the default for system tables.) In all cases, no old values are logged unless at least one of the columns that would be logged differs between the old and new versions of the row. /para diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml new file mode 100644 index 85899d7..0b91d45 *** a/doc/src/sgml/ref/psql-ref.sgml --- b/doc/src/sgml/ref/psql-ref.sgml *** testdb=gt; *** 951,957 The command form literal\d+/literal is identical, except that more information is displayed: any comments associated with the columns of the table are shown, as is the presence of OIDs in the ! table, the view definition if the relation is a view. /para para --- 951,959 The command form literal\d+/literal is identical, except that more information is displayed: any comments associated with the columns of the table are shown, as is the presence of OIDs in the ! table, the view definition if the relation is a view, a non-default ! link linkend=SQL-CREATETABLE-REPLICA-IDENTITYreplica ! identity/link setting. /para para diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c new file mode 100644 index d1447fe..0ff7950 *** a/src/bin/psql/describe.c --- b/src/bin/psql/describe.c *** describeOneTableDetails(const char *sche *** 2350,2357 * No need to display default values; we already display a *
Re: [HACKERS] psql \d+ and oid display
Bruce Momjian br...@momjian.us writes: It also has changed the OID status to only display if it exists. One question that came up with Robert is whether OID status should appear for \d as well, now that is only shows up when present. Yeah, I was wondering about that too. If part of the argument here is to make these two displays act more alike, it seems inconsistent that one is emitted by \d while the other only comes out with \d+. Of course, there are two ways to fix that: maybe the replica info also only belongs in \d+? 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] psql \d+ and oid display
On Thu, Apr 10, 2014 at 07:58:55PM -0400, Tom Lane wrote: Bruce Momjian br...@momjian.us writes: It also has changed the OID status to only display if it exists. One question that came up with Robert is whether OID status should appear for \d as well, now that is only shows up when present. Yeah, I was wondering about that too. If part of the argument here is to make these two displays act more alike, it seems inconsistent that one is emitted by \d while the other only comes out with \d+. Of course, there are two ways to fix that: maybe the replica info also only belongs in \d+? OK, I changed my patch to only show replica info for \d+. If we decide to change them to both display for \d, I will update it again. -- Bruce Momjian br...@momjian.ushttp://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] [PATCH] Negative Transition Aggregate Functions (WIP)
On Apr11, 2014, at 01:30 , Tom Lane t...@sss.pgh.pa.us wrote: Florian Pflug f...@phlo.org writes: As for evidence - have you looked at the patch I posted? I'd be very interested to know if it removes the performance differences you saw. (1) You can't really prove the absence of a performance issue by showing that one specific aggregate doesn't have an issue. I'm claiming that SUM(int4) is about as simple as it gets, so if the effect can be mitigated there, it can be mitigated everywhere. The more complex a forward-only transition function is, the less will and added if or two hurt. (2) These results (mine as well as yours) seem mighty platform-dependent, so the fact you don't see an issue doesn't mean someone else won't. Yeah, though *any* change - mine, the one your propose, and any other - has the potential to hurt some platform due to weird interactions (say, cache trashing). (3) A new FunctionCallInfoData field just for this? Surely not. There's got to be a distributed cost to that (although I notice you didn't bother initializing the field most places, which is cheating). I think the field doesn't actually increase the size of the structure at all - at least not if the bool before it has size 1 and the short following it is 2-byte aligned. Or at least that was why I made it a char, and added it at the place I did. But I might be missing something... Also, InitFunctionCallInfoData *does* initialize the flags to zero. Though maybe not everybody uses that - I didn't check, this was just a quick hack. Now point 3 could be addressed by doing the signaling in some other way with the existing context field. But it's still the case that you're trying to band-aid a bad design. There's no good reason to make the sfunc do extra work to be invertible in contexts where we know, with certainty, that that work is useless. This is the principal point where we disagree, I think. From my POV, the problem isn't invertibility here at all. Heck, 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. The way I see it, the main problem is the drop in performance that comes from using a pass-by-ref state type. Which IMHO happens because this *already* is a heavily band-aided design - all the state validation and if (AggCheckCallContext(,NULL)) stuff really works around the fact that transition functions *aren't* supposed to be user-called, yet they are, and must deal. 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. Compared to that, the benefit of having a completely separate set of transition functions and state types for the invertible case is much less tangible, IMHO. Especially not when we know that even a few instructions of extra work can be performance-significant. But where do we draw that line? nodeWindowAgg.c quite certainly wastes about as many cycles as int4_avg_accum does on various checks that are unnecessary unless in the non-sliding window case. Do we duplicate those functions too? 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] WAL replay bugs
On Thu, Apr 10, 2014 at 6:21 PM, Heikki Linnakangas hlinnakan...@vmware.com wrote: On 04/10/2014 10:52 AM, sachin kotwal wrote: I executed given steps many times to produce this bug. But still I unable to hit this bug. I used attached scripts to produce this bug. Can I get scripts to produce this bug? Oh, I can't reproduce it using that script either. I must've used some variation of it, and posted wrong script. The attached seems to do the trick. I changed the INSERT statements slightly, so that all the new rows have the same key. Thanks for verifying this! Thanks to explain the case to produce this bug. I am able to produce this bug by using latest scripts from last mail. I applied patch submitted for this bug and re-run the scripts. Now it is giving correct result. Thanks and Regards, Sachin Kotwal
Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C
On Fri, Apr 11, 2014 at 7:44 AM, Bruce Momjian br...@momjian.us wrote:
Can someone with Windows expertise comment on whether this should be
applied?
I tested the same in windows and it is working as specified.
The same background running server can be closed with ctrl+break command.
---
On Tue, Jan 7, 2014 at 12:44:33PM +0100, Christian Ullrich wrote:
Hello all,
when pg_ctl start is used to run PostgreSQL in a console window on
Windows, it runs in the background (it is terminated by closing the
window, but that is probably inevitable). There is one problem,
however: The first Ctrl-C in that window, no matter in which
situation, will cause the background postmaster to exit. If you,
say, ping something, and press Ctrl-C to stop ping, you probably
don't want the database to go away, too.
The reason is that Windows delivers the Ctrl-C event to all
processes using that console, not just to the foreground one.
Here's a patch to fix that. pg_ctl stop still works, and it has no
effect when running as a service, so it should be safe. It starts
the postmaster in a new process group (similar to calling setpgrp()
after fork()) that does not receive Ctrl-C events from the console
window.
--
Christian
diff --git a/src/bin/pg_ctl/pg_ctl.c b/src/bin/pg_ctl/pg_ctl.c
new file mode 100644
index 50d4586..89a9544
*** a/src/bin/pg_ctl/pg_ctl.c
--- b/src/bin/pg_ctl/pg_ctl.c
*** CreateRestrictedProcess(char *cmd, PROCE
*** 1561,1566
--- 1561,1567
HANDLE restrictedToken;
SID_IDENTIFIER_AUTHORITY NtAuthority = {SECURITY_NT_AUTHORITY};
SID_AND_ATTRIBUTES dropSids[2];
+ DWORD flags;
/* Functions loaded dynamically */
__CreateRestrictedToken _CreateRestrictedToken = NULL;
*** CreateRestrictedProcess(char *cmd, PROCE
*** 1636,1642
AddUserToTokenDacl(restrictedToken);
#endif
! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE,
CREATE_SUSPENDED, NULL, NULL, si, processInfo);
Kernel32Handle = LoadLibrary(KERNEL32.DLL);
if (Kernel32Handle != NULL)
--- 1637,1650
AddUserToTokenDacl(restrictedToken);
#endif
! flags = CREATE_SUSPENDED;
!
! /* Protect console process from Ctrl-C */
! if (!as_service) {
! flags |= CREATE_NEW_PROCESS_GROUP;
! }
!
! r = CreateProcessAsUser(restrictedToken, NULL, cmd, NULL, NULL, TRUE,
flags, NULL, NULL, si, processInfo);
Kernel32Handle = LoadLibrary(KERNEL32.DLL);
if (Kernel32Handle != NULL)
Regards,
Hari Babu
Fujitsu Australia
--
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 11:58:58AM +1000, Haribabu Kommi wrote: On Fri, Apr 11, 2014 at 7:44 AM, Bruce Momjian br...@momjian.us wrote: Can someone with Windows expertise comment on whether this should be applied? I tested the same in windows and it is working as specified. The same background running server can be closed with ctrl+break command. OK. If I apply this, are you able to test to see if the problem is fixed? -- Bruce Momjian br...@momjian.ushttp://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] PostgreSQL in Windows console and Ctrl-C
On Fri, Apr 11, 2014 at 12:12 PM, Bruce Momjian br...@momjian.us wrote: On Fri, Apr 11, 2014 at 11:58:58AM +1000, Haribabu Kommi wrote: On Fri, Apr 11, 2014 at 7:44 AM, Bruce Momjian br...@momjian.us wrote: Can someone with Windows expertise comment on whether this should be applied? I tested the same in windows and it is working as specified. The same background running server can be closed with ctrl+break command. OK. If I apply this, are you able to test to see if the problem is fixed? I already tested in HEAD by applying the attached patch in the earlier mail. with ctrl+c command the background process is not closed. But with ctrl+break command the same can be closed. if this behavior is fine, then we can apply patch. Regards, Hari Babu Fujitsu Australia -- 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] Get more from indices.
(2014/04/10 22:25), Tom Lane wrote: Etsuro Fujita fujita.ets...@lab.ntt.co.jp writes: (2014/04/10 0:08), Tom Lane wrote: TBH I think that's barely the tip of the iceberg of cases where this patch will get the wrong answer. Also, I don't see it doing anything to check the ordering of multiple index columns I think that the following code in index_pathkeys_are_extensible() would check the ordering: +if (!pathkeys_contained_in(pathkeys, root-query_pathkeys)) +return false; Hm ... if you're relying on that, then what's the point of the new loop at all? The point is that from the discussion [1], we allow the index pathkeys to be extended to query_pathkeys if each *remaining* pathkey in query_pathkey is a Var belonging to the indexed relation. The code is confusing, though. Sorry, that is my faults. Thanks, [1] http://www.postgresql.org/message-id/29637.1389064...@sss.pgh.pa.us Best regards, Etsuro Fujita -- 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, 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.
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..
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?
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)) ?
Also, it seems like there should be a check_stack_depth() call here now?
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.
Thanks,
Stephen
signature.asc
Description: Digital signature
Re: [HACKERS] Adding unsigned 256 bit integers
pgmp is also worth mentioning here, and it's likely to be more efficient than the numeric type or something you hack up yourself: http://pgmp.projects.pgfoundry.org/ Best, Leon On Thu, Apr 10, 2014 at 10:11 AM, k...@rice.edu k...@rice.edu wrote: On Thu, Apr 10, 2014 at 09:13:47PM +0800, Olivier Lalonde wrote: I was wondering if there would be any way to do the following in PostgreSQL: UPDATE cryptotable SET work = work + 'some big hexadecimal number' where work is an unsigned 256 bit integer. Right now my column is a character varying(64) column (hexadecimal representation of the number) but I would be happy to switch to another data type if it lets me do the operation above. If it's not possible with vanilla PostgreSQL, are there extensions that could help me? -- - Oli Olivier Lalonde http://www.syskall.com -- connect with me! Hi Olivier, Here are some sample pl/pgsql helper functions that I have written for other purposes. They use integers but can be adapted to use numeric. Regards, Ken --- CREATE OR REPLACE FUNCTION hex2dec(t text) RETURNS integer AS $$ DECLARE r RECORD; BEGIN FOR r IN EXECUTE 'SELECT x'''||t||'''::integer AS hex' LOOP RETURN r.hex; END LOOP; END $$ LANGUAGE plpgsql IMMUTABLE STRICT; --- --- CREATE OR REPLACE FUNCTION bytea2int ( in_string BYTEA ) RETURNS INTEGER AS $$ DECLARE b1 INTEGER := 0; b2 INTEGER := 0; b3 INTEGER := 0; b4 INTEGER := 0; out_int INTEGER := 0; BEGIN CASE OCTET_LENGTH(in_string) WHEN 1 THEN b4 := get_byte(in_string, 0); WHEN 2 THEN b3 := get_byte(in_string, 0); b4 := get_byte(in_string, 1); WHEN 3 THEN b2 := get_byte(in_string, 0); b3 := get_byte(in_string, 1); b4 := get_byte(in_string, 2); WHEN 4 THEN b1 := get_byte(in_string, 0); b2 := get_byte(in_string, 1); b3 := get_byte(in_string, 2); b4 := get_byte(in_string, 3); END CASE; out_int := (b1 24) + (b2 16) + (b3 8) + b4; RETURN(out_int); END; $$ LANGUAGE plpgsql IMMUTABLE; --- -- 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 Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian br...@momjian.us 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. 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] [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of 0
On Fri, Apr 11, 2014 at 10:00 AM, Amit Kapila amit.kapil...@gmail.com wrote:
On Thu, Apr 10, 2014 at 5:21 PM, Bruce Momjian br...@momjian.us 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)
{
+
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] Get more from indices.
Hi, sorry for the absense. I've been back. Attached is the patch following the discussion below. (2014/04/10 0:08), Tom Lane wrote: TBH I think that's barely the tip of the iceberg of cases where this patch will get the wrong answer. Also, I don't see it doing anything to check the ordering of multiple index columns I think that the following code in index_pathkeys_are_extensible() would check the ordering: + if (!pathkeys_contained_in(pathkeys, root-query_pathkeys)) + return false; Hm ... if you're relying on that, then what's the point of the new loop at all? The point is that from the discussion [1], we allow the index pathkeys to be extended to query_pathkeys if each *remaining* pathkey in query_pathkey is a Var belonging to the indexed relation. The code is confusing, though. Sorry, that is my faults. Hmm, I found that the iterations for the part that already checked by pathkeys_contained_in are not only needless but a bit heavy. And the loop seems a little verbose. I did for the patch, in index_pathkeys_are_extensible, - Avoiding duplicate check with pathkeys_contained_in. I put similar code to list_nth_cell since it is not exposed outside of list.c. - Add comment to clarify the purpose of the loop. - Simplify the check for the remaining keycolumns Thanks, [1] http://www.postgresql.org/message-id/29637.1389064...@sss.pgh.pa.us Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- 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] Get more from indices.
# Sorry for accidentialy sending the previous mail unfinished.
## ...and I seem to have bombed uncertain files off out of my
## home directory by accident, too :(
=
Hi, sorry for the absense. I've been back.
Thank you for continuing this discussion.
Attached is the patch following the discussion below.
(2014/04/10 0:08), Tom Lane wrote:
TBH I think that's barely the tip of the iceberg of cases where this
patch will get the wrong answer.
Also, I don't see it doing anything to check the ordering
of multiple index columns
I think that the following code in index_pathkeys_are_extensible() would
check the ordering:
+ if (!pathkeys_contained_in(pathkeys, root-query_pathkeys))
+ return false;
Hm ... if you're relying on that, then what's the point of the new loop
at all?
The point is that from the discussion [1], we allow the index pathkeys
to be extended to query_pathkeys if each *remaining* pathkey in
query_pathkey is a Var belonging to the indexed relation. The code is
confusing, though. Sorry, that is my faults.
Hmm, I found that the iterations for the part that already
checked by pathkeys_contained_in are not only useless but a bit
heavy. And the loop seems a little verbose. I did for the patch,
in index_pathkeys_are_extensible,
- Avoiding duplicate check with pathkeys_contained_in.
I put similar code to list_nth_cell since it is not exposed
outside of list.c.
- Add comment to clarify the purpose of the loop.
- Simplify the check for the remaining keycolumns
I think this makes some things clearer.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index bfb4b9f..ff5c88c 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -1790,6 +1790,7 @@ _outIndexOptInfo(StringInfo str, const IndexOptInfo *node)
WRITE_BOOL_FIELD(predOK);
WRITE_BOOL_FIELD(unique);
WRITE_BOOL_FIELD(immediate);
+ WRITE_BOOL_FIELD(allnotnull);
WRITE_BOOL_FIELD(hypothetical);
/* we don't bother with fields copied from the pg_am entry */
}
diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer/path/indxpath.c
index a912174..4376e95 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -951,8 +951,11 @@ build_index_paths(PlannerInfo *root, RelOptInfo *rel,
{
index_pathkeys = build_index_pathkeys(root, index,
ForwardScanDirection);
- useful_pathkeys = truncate_useless_pathkeys(root, rel,
- index_pathkeys);
+ if (index_pathkeys_are_extensible(root, index, index_pathkeys))
+ useful_pathkeys = root-query_pathkeys;
+ else
+ useful_pathkeys = truncate_useless_pathkeys(root, rel,
+ index_pathkeys);
orderbyclauses = NIL;
orderbyclausecols = NIL;
}
diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c
index 9179c61..5edca4f 100644
--- a/src/backend/optimizer/path/pathkeys.c
+++ b/src/backend/optimizer/path/pathkeys.c
@@ -502,6 +502,76 @@ build_index_pathkeys(PlannerInfo *root,
}
/*
+ * index_pathkeys_are_extensible
+ * Check whether the pathkeys are extensible to query_pathkeys.
+ */
+bool
+index_pathkeys_are_extensible(PlannerInfo *root,
+ IndexOptInfo *index,
+ List *pathkeys)
+{
+ bool result;
+ ListCell *lc1, *remain;
+
+ if (root-query_pathkeys == NIL || pathkeys == NIL)
+ return false;
+
+ /* This index is not suitable for pathkey extension */
+ if (!index-unique || !index-immediate || !index-allnotnull)
+ return false;
+
+ /* pathkeys is a prefixing proper subset of index tlist */
+ if (list_length(pathkeys) list_length(index-indextlist))
+ return false;
+
+ if (!pathkeys_contained_in(pathkeys, root-query_pathkeys))
+ return false;
+
+ if (list_length(pathkeys) == list_length(root-query_pathkeys))
+ return true;
+
+ Assert(list_length(root-query_pathkeys) list_length(pathkeys));
+
+ /*
+ * Check if all unchecked elements of query_patheys are simple Vars for
+ * the same relation.
+ */
+
+ /* list_nth_cell is not exposed publicly.. */
+ if (list_length(pathkeys) == list_length(root-query_pathkeys) - 1)
+ remain = list_tail(root-query_pathkeys);
+ else
+ {
+ int n = list_length(pathkeys);
+
+ remain = root-query_pathkeys-head;
+ while(n-- 0) remain = remain-next;
+ }
+
+ result = true;
+ for_each_cell(lc1, remain)
+ {
+ PathKey*pathkey = (PathKey *) lfirst(lc1);
+ ListCell *lc2;
+
+ foreach(lc2, pathkey-pk_eclass-ec_members)
+ {
+ EquivalenceMember *member = (EquivalenceMember *) lfirst(lc2);
+
+ if (!bms_equal(member-em_relids, index-rel-relids) ||
+!IsA(member-em_expr, Var))
+ {
+result = false;
+break;
+ }
+ }
+
+ if (!result) break;
+ }
+ return result;
+}
+
+/*
* build_expression_pathkey
* Build a pathkeys list that describes an ordering by a single expression
* using the given sort operator.
diff --git
Re: [HACKERS] PostgreSQL in Windows console and Ctrl-C
On Fri, Apr 11, 2014 at 3:14 AM, Bruce Momjian br...@momjian.us 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
*/
write_stderr(_(%s: WARNING: cannot create restricted tokens on this
platform\n), progname);
if (Advapi32Handle != NULL)
FreeLibrary(Advapi32Handle);
return CreateProcess(NULL, cmd, NULL, NULL, FALSE, 0, NULL, NULL, si,
processInfo);
}
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
