Re: [HACKERS] [PATCH] Negative Transition Aggregate Functions (WIP)

2014-04-10 Thread David Rowley
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)

2014-04-10 Thread David Rowley
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

2014-04-10 Thread sachin kotwal

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

2014-04-10 Thread Heikki Linnakangas

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 Thread Etsuro Fujita
(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]

2014-04-10 Thread Rajeev rastogi
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

2014-04-10 Thread Heikki Linnakangas
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)

2014-04-10 Thread Rajeev rastogi
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

2014-04-10 Thread Gregory Smith

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

2014-04-10 Thread Bruce Momjian
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

2014-04-10 Thread Olivier Lalonde
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.

2014-04-10 Thread Tom Lane
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

2014-04-10 Thread Andrew Dunstan


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

2014-04-10 Thread Craig Ringer
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

2014-04-10 Thread Alexander Korotkov
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

2014-04-10 Thread k...@rice.edu
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)

2014-04-10 Thread Tom Lane
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

2014-04-10 Thread Greg Stark
Ok, So I've hacked on this a bit. Below is a test case showing the
problems I've found.

1) It isn't using the newline and wrap indicators or dividing lines.

2) The header is not being displayed properly when it contains a newline.

I can hack in the newline and wrap indicators but the header
formatting requires reworking the logic a bit. The header and data
need to be stepped through in parallel rather than having a loop to
handle the wrapping within the handling of a single line. I don't
really have time for that today but if you can get to it that would be
fine,
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

2014-04-10 Thread Fabrízio de Royes Mello
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

2014-04-10 Thread Robert Haas
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

2014-04-10 Thread Bruce Momjian
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

2014-04-10 Thread Simon Riggs
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

2014-04-10 Thread Greg Stark
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

2014-04-10 Thread Bruce Momjian
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)

2014-04-10 Thread Dean Rasheed
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)

2014-04-10 Thread Tom Lane
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)

2014-04-10 Thread Dean Rasheed
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)

2014-04-10 Thread Tom Lane
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

2014-04-10 Thread Alexander Korotkov
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)

2014-04-10 Thread Dean Rasheed
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

2014-04-10 Thread Sergey Muraviov
Hi.

Thanks for your tests.

I've fixed problem with headers, but got new one with data.
I'll try to solve it tomorrow.


2014-04-10 18:45 GMT+04:00 Greg Stark 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

2014-04-10 Thread Heikki Linnakangas

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

2014-04-10 Thread Bruce Momjian
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)

2014-04-10 Thread Florian Pflug
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

2014-04-10 Thread Bruce Momjian

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)

2014-04-10 Thread Tom Lane
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)

2014-04-10 Thread Florian Pflug
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)

2014-04-10 Thread Tom Lane
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)

2014-04-10 Thread Florian Pflug
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

2014-04-10 Thread Bruce Momjian
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)

2014-04-10 Thread Tom Lane
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

2014-04-10 Thread Bruce Momjian
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

2014-04-10 Thread Tom Lane
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

2014-04-10 Thread Bruce Momjian
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)

2014-04-10 Thread Florian Pflug
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

2014-04-10 Thread Sachin D. Kotwal
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

2014-04-10 Thread Haribabu Kommi
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

2014-04-10 Thread Bruce Momjian
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

2014-04-10 Thread Haribabu Kommi
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 Thread Etsuro Fujita
(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

2014-04-10 Thread Stephen Frost
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

2014-04-10 Thread Leon Smith
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

2014-04-10 Thread Amit Kapila
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

2014-04-10 Thread Amit Kapila
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.

2014-04-10 Thread Kyotaro HORIGUCHI
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.

2014-04-10 Thread Kyotaro HORIGUCHI
# 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

2014-04-10 Thread Amit Kapila
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