Re: [HACKERS] Better way of dealing with pgstat wait timeout during buildfarm runs?

[Andres Freund]

On 2015-01-21 22:43:03 -0500, Matt Kelly wrote:
> >
> > Sure, but nobody who is not a developer is going to care about that.
> > A typical user who sees "pgstat wait timeout", or doesn't, isn't going
> > to be able to make anything at all out of that.
> 
> 
> As a user, I wholeheartedly disagree.

Note that that the change we discussed wasn't removing the message. It
was changing the log level from WARNING to LOG. Which means the change
is not going to the client anymore, but still to the server log (perhaps
surprisingly, the likelihood for the latter actually increases).

> I think the warning is incredibly valuable.  Along those lines I'd also
> love to see a pg_stat_snapshot_timestamp() for monitoring code to use to
> determine if its using a stale snapshot, as well as helping to smooth
> graphs of the statistics that are based on highly granular snapshotting.

I can see that being useful.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Parallel Seq Scan

[Amit Langote]

On 22-01-2015 PM 02:30, Amit Kapila wrote:
>> Perhaps you are aware or you've postponed working on it, but I see that
>> a plan executing in a worker does not know about instrumentation.
> 
> I have deferred it until other main parts are stabilised/reviewed.  Once
> that is done, we can take a call what is best we can do for instrumentation.
> Thom has reported the same as well upthread.
> 

Ah, I missed Thom's report.

>> Note the "Rows Removed by Filter". I guess the difference may be
>> because, all the rows filtered by workers were not accounted for. I'm
>> not quite sure, but since exec_worker_stmt goes the Portal way,
>> QueryDesc.instrument_options remains unset and hence no instrumentation
>> opportunities in a worker backend. One option may be to pass
>> instrument_options down to worker_stmt?
>>
> 
> I think there is more to it, master backend need to process that information
> as well.
> 

I see.

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Thu, Jan 22, 2015 at 10:44 AM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:
>
> On 21-01-2015 PM 09:43, Amit Kapila wrote:
> > On Wed, Jan 21, 2015 at 4:31 PM, Amit Langote 
> > wrote:
> >> On Wednesday, January 21, 2015, Amit Kapila 
> > wrote:
> >>>
> >>>
> >>> Does it happen only when parallel_seqscan_degree >
max_worker_processes?
> >>
> >>
> >>  I have max_worker_processes set to the default of 8 while
> > parallel_seqscan_degree is 4. So, this may be a case different from
Thom's.
> >>
> >
> > I think this is due to reason that memory for forming
> > tuple in master backend is retained for longer time which
> > is causing this statement to take much longer time than
> > required.  I have fixed the other issue as well reported by
> > you in attached patch.
> >
>
> Thanks for fixing.
>
> > I think this patch is still not completely ready for general
> > purpose testing, however it could be helpful if we can run
> > some tests to see in what kind of scenario's it gives benefit
> > like in the test you are doing if rather than increasing
> > seq_page_cost, you should add an expensive WHERE condition
> > so that it should automatically select parallel plan. I think it is
better
> > to change one of the new parameter's (parallel_setup_cost,
> > parallel_startup_cost and cpu_tuple_comm_cost) if you want
> > your statement to use parallel plan, like in your example if
> > you would have reduced cpu_tuple_comm_cost, it would have
> > selected parallel plan, that way we can get some feedback about
> > what should be the appropriate default values for the newly added
> > parameters.  I am already planing to do some tests in that regard,
> > however if I get some feedback from other's that would be helpful.
> >
> >
>
> Perhaps you are aware or you've postponed working on it, but I see that
> a plan executing in a worker does not know about instrumentation.

I have deferred it until other main parts are stabilised/reviewed.  Once
that is done, we can take a call what is best we can do for instrumentation.
Thom has reported the same as well upthread.

> Note the "Rows Removed by Filter". I guess the difference may be
> because, all the rows filtered by workers were not accounted for. I'm
> not quite sure, but since exec_worker_stmt goes the Portal way,
> QueryDesc.instrument_options remains unset and hence no instrumentation
> opportunities in a worker backend. One option may be to pass
> instrument_options down to worker_stmt?
>

I think there is more to it, master backend need to process that information
as well.

> By the way, 17s and 6s compare really well in favor of parallel seqscan
> above, :)
>

That sounds interesting.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Parallel Seq Scan

[Amit Langote]

On 21-01-2015 PM 09:43, Amit Kapila wrote:
> On Wed, Jan 21, 2015 at 4:31 PM, Amit Langote 
> wrote:
>> On Wednesday, January 21, 2015, Amit Kapila 
> wrote:
>>>
>>>
>>> Does it happen only when parallel_seqscan_degree > max_worker_processes?
>>
>>
>>  I have max_worker_processes set to the default of 8 while
> parallel_seqscan_degree is 4. So, this may be a case different from Thom's.
>>
> 
> I think this is due to reason that memory for forming
> tuple in master backend is retained for longer time which
> is causing this statement to take much longer time than
> required.  I have fixed the other issue as well reported by
> you in attached patch.
> 

Thanks for fixing.

> I think this patch is still not completely ready for general
> purpose testing, however it could be helpful if we can run
> some tests to see in what kind of scenario's it gives benefit
> like in the test you are doing if rather than increasing
> seq_page_cost, you should add an expensive WHERE condition
> so that it should automatically select parallel plan. I think it is better
> to change one of the new parameter's (parallel_setup_cost,
> parallel_startup_cost and cpu_tuple_comm_cost) if you want
> your statement to use parallel plan, like in your example if
> you would have reduced cpu_tuple_comm_cost, it would have
> selected parallel plan, that way we can get some feedback about
> what should be the appropriate default values for the newly added
> parameters.  I am already planing to do some tests in that regard,
> however if I get some feedback from other's that would be helpful.
> 
> 

Perhaps you are aware or you've postponed working on it, but I see that
a plan executing in a worker does not know about instrumentation. It
results in the EXPLAIN ANALYZE showing incorrect figures. For example
compare the normal seqscan and parallel seqscan below:

postgres=# EXPLAIN ANALYZE SELECT * FROM test WHERE sqrt(a) < 3456 AND
md5(a::text) LIKE 'ac%';
  QUERY PLAN

---
 Seq Scan on test  (cost=0.00..310228.52 rows=16120 width=4) (actual
time=0.497..17062.436 rows=39028 loops=1)
   Filter: ((sqrt((a)::double precision) < 3456::double precision) AND
(md5((a)::text) ~~ 'ac%'::text))
   Rows Removed by Filter: 9960972
 Planning time: 0.206 ms
 Execution time: 17378.413 ms
(5 rows)

postgres=# EXPLAIN ANALYZE SELECT * FROM test WHERE sqrt(a) < 3456 AND
md5(a::text) LIKE 'ac%';
  QUERY PLAN

---
 Parallel Seq Scan on test  (cost=0.00..255486.08 rows=16120 width=4)
(actual time=7.329..4906.981 rows=39028 loops=1)
   Filter: ((sqrt((a)::double precision) < 3456::double precision) AND
(md5((a)::text) ~~ 'ac%'::text))
   Rows Removed by Filter: 1992710
   Number of Workers: 4
   Number of Blocks Per Worker: 8849
 Planning time: 0.137 ms
 Execution time: 6077.782 ms
(7 rows)

Note the "Rows Removed by Filter". I guess the difference may be
because, all the rows filtered by workers were not accounted for. I'm
not quite sure, but since exec_worker_stmt goes the Portal way,
QueryDesc.instrument_options remains unset and hence no instrumentation
opportunities in a worker backend. One option may be to pass
instrument_options down to worker_stmt?

By the way, 17s and 6s compare really well in favor of parallel seqscan
above, :)

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Thu, Jan 22, 2015 at 6:37 AM, Kouhei Kaigai  wrote:
>
> (Please point out me if my understanding is incorrect.)
>
> What happen if dynamic background worker process tries to reference
temporary
> tables? Because buffer of temporary table blocks are allocated on private
> address space, its recent status is not visible to other process unless
it is
> not flushed to the storage every time.
>
> Do we need to prohibit create_parallelscan_paths() to generate a path when
> target relation is temporary one?
>

Yes, we need to prohibit parallel scans on temporary relations.  Will fix.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Better way of dealing with pgstat wait timeout during buildfarm runs?

[Matt Kelly]

>
> Sure, but nobody who is not a developer is going to care about that.
> A typical user who sees "pgstat wait timeout", or doesn't, isn't going
> to be able to make anything at all out of that.


As a user, I wholeheartedly disagree.

That warning helped me massively in diagnosing an unhealthy database server
in the past at TripAdvisor (i.e. high end server class box, not a raspberry
pie).  I have realtime monitoring that looks at pg_stat_database at regular
intervals particularly for the velocity of change of xact_commit and
xact_rollback columns, similar to how check_postgres does it.
https://github.com/bucardo/check_postgres/blob/master/check_postgres.pl#L4234

When one of those servers was unhealthy, it stopped reporting statistics
for 30 seconds+ at a time.  My dashboard which polled far more frequently
than that indicated the server was normally processing 0 tps with
intermittent spikes. I went directly onto the server and sampled
pg_stat_database.  That warning was the only thing that directly indicated
that the statistics collector was not to be trusted.  It obviously was a
victim of what was going on in the server, but its pretty important to know
when your methods for measuring server health are lying to you.  The spiky
TPS at first glance appears like some sort of live lock, not just that the
server is overloaded.

Now, I know: 0 change in stats = collector broken.  Rereading the docks,

 Also, the collector itself emits a new report at most once per
> PGSTAT_STAT_INTERVAL milliseconds (500 ms unless altered while building
> the server).


Without context this merely reads: "We sleep for 500ms, plus the time to
write the file, plus whenever the OS decides to enforce the timer
interrupt... so like 550-650ms."  It doesn't read, "When server is
unhealthy, but _still_ serving queries, the stats collector might not be
able to keep up and will just stop reporting stats all together."

I think the warning is incredibly valuable.  Along those lines I'd also
love to see a pg_stat_snapshot_timestamp() for monitoring code to use to
determine if its using a stale snapshot, as well as helping to smooth
graphs of the statistics that are based on highly granular snapshotting.

- Matt Kelly

Re: [HACKERS] TODO : Allow parallel cores to be used by vacuumdb [ WIP ]

[Amit Kapila]

On Thu, Jan 22, 2015 at 8:22 AM, Alvaro Herrera 
wrote:
>
> Amit Kapila wrote:
> > On Wed, Jan 21, 2015 at 8:51 PM, Alvaro Herrera <
alvhe...@2ndquadrant.com>
> > wrote:
> > >
> > > I didn't understand the coding in GetQueryResult(); why do we check
the
> > > result status of the last returned result only?  It seems simpler to
me
> > > to check it inside the loop, but maybe there's a reason you didn't do
it
> > > like that?
> > >
> > > Also, what is the reason we were ignoring those errors only in
> > > "completedb" mode?  It doesn't seem like it would cause any harm if we
> > > did it in all cases.  That means we can just not have the "completeDb"
> > > flag at all.
> >
> > IIRC it is done to match the existing behaviour where such errors are
> > ignored we use this utility to vacuum database.
>
> I think that's fine, but we should do it always, not just in
> whole-database mode.
>
> I've been hacking this patch today BTW; hope to have something to show
> tomorrow.
>

Great!


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] TODO : Allow parallel cores to be used by vacuumdb [ WIP ]

[Alvaro Herrera]

Amit Kapila wrote:
> On Wed, Jan 21, 2015 at 8:51 PM, Alvaro Herrera 
> wrote:
> >
> > I didn't understand the coding in GetQueryResult(); why do we check the
> > result status of the last returned result only?  It seems simpler to me
> > to check it inside the loop, but maybe there's a reason you didn't do it
> > like that?
> >
> > Also, what is the reason we were ignoring those errors only in
> > "completedb" mode?  It doesn't seem like it would cause any harm if we
> > did it in all cases.  That means we can just not have the "completeDb"
> > flag at all.
> 
> IIRC it is done to match the existing behaviour where such errors are
> ignored we use this utility to vacuum database.

I think that's fine, but we should do it always, not just in
whole-database mode.

I've been hacking this patch today BTW; hope to have something to show
tomorrow.

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] TODO : Allow parallel cores to be used by vacuumdb [ WIP ]

[Amit Kapila]

On Wed, Jan 21, 2015 at 8:51 PM, Alvaro Herrera 
wrote:
>
> I didn't understand the coding in GetQueryResult(); why do we check the
> result status of the last returned result only?  It seems simpler to me
> to check it inside the loop, but maybe there's a reason you didn't do it
> like that?
>
> Also, what is the reason we were ignoring those errors only in
> "completedb" mode?  It doesn't seem like it would cause any harm if we
> did it in all cases.  That means we can just not have the "completeDb"
> flag at all.
>

IIRC it is done to match the existing behaviour where such errors are
ignored we use this utility to vacuum database.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Merging postgresql.conf and postgresql.auto.conf

[Amit Kapila]

On Wed, Jan 21, 2015 at 9:43 PM, Sawada Masahiko 
wrote:
>
> On Wed, Jan 21, 2015 at 3:38 PM, Amit Kapila 
wrote:
> > On Tue, Jan 20, 2015 at 9:38 PM, Robert Haas 
wrote:
> >
> >
> > The reason why "sourcefile" and "sourceline" are not sufficient is that
> > they can only give the information about the setting in last file it is
> > present.  Assume max_connections (or any other setting) is available
> > in both postgresql.conf and postgresql.auto.conf, then it will display
> > the information about the setting in postgresql.auto.conf, so now user
> > might not be able to decide whether that is the setting he want to
retain
> > unless he knows the information about setting in postgresql.conf.
> >
> > Now as I have suggested upthread, that we can have a new view
> > pg_file_settings which will display information about settings even
> > when there exists multiple entries for the same in different files.
> >
> > I think adding such information to existing view pg_settings would
> > be difficult as the same code is used for show commands which
> > we don't want to change.
> >
>
> I think this new view is updated only when postmaster received SIGHUP
> or is started.
> And we can have new function like pg_update_file_setting() which
> updates this view.
>

If that is doable without much complication, then it might not be bad
idea to just add additional columns to existing view (pg_settings).  I
think you can once evaluate the details like what additional columns
(other than what pg_settings has) are required and how you want to
update them. After doing so further discussion could be more meaningful.



With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] New CF app deployment

[Michael Paquier]

On Thu, Jan 22, 2015 at 10:47 AM, Josh Berkus  wrote:
> Now that I'm back on this side of the Pacific, is there any additional
> data entry/cleanup which needs doing?
An extra look would be worth it. Magnus or I may have missed patch
entries between the old and new apps.
My2c.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] New CF app deployment

[Josh Berkus]

Magnus,

Now that I'm back on this side of the Pacific, is there any additional
data entry/cleanup which needs doing?

-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.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] Parallel Seq Scan

[Kouhei Kaigai]

> On Wed, Jan 21, 2015 at 4:31 PM, Amit Langote 
> wrote:
> > On Wednesday, January 21, 2015, Amit Kapila 
> wrote:
> >>
> >>
> >> Does it happen only when parallel_seqscan_degree > max_worker_processes?
> >
> >
> >  I have max_worker_processes set to the default of 8 while
> parallel_seqscan_degree is 4. So, this may be a case different from Thom's.
> >
> 
> I think this is due to reason that memory for forming tuple in master backend
> is retained for longer time which is causing this statement to take much
> longer time than required.  I have fixed the other issue as well reported
> by you in attached patch.
> 
> I think this patch is still not completely ready for general purpose testing,
> however it could be helpful if we can run some tests to see in what kind
> of scenario's it gives benefit like in the test you are doing if rather
> than increasing seq_page_cost, you should add an expensive WHERE condition
> so that it should automatically select parallel plan. I think it is better
> to change one of the new parameter's (parallel_setup_cost,
> parallel_startup_cost and cpu_tuple_comm_cost) if you want your statement
> to use parallel plan, like in your example if you would have reduced
> cpu_tuple_comm_cost, it would have selected parallel plan, that way we can
> get some feedback about what should be the appropriate default values for
> the newly added parameters.  I am already planing to do some tests in that
> regard, however if I get some feedback from other's that would be helpful.
> 
(Please point out me if my understanding is incorrect.)

What happen if dynamic background worker process tries to reference temporary
tables? Because buffer of temporary table blocks are allocated on private
address space, its recent status is not visible to other process unless it is
not flushed to the storage every time.

Do we need to prohibit create_parallelscan_paths() to generate a path when
target relation is temporary one?

Thanks,
--
NEC OSS Promotion Center / PG-Strom Project
KaiGai Kohei 

-- 
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] pgaudit - an auditing extension for PostgreSQL

[Stephen Frost]

* Jim Nasby (jim.na...@bluetreble.com) wrote:
> On 1/21/15 5:38 PM, Stephen Frost wrote:
> >Being startup-only won't help if the user is a superuser.
> 
> Crap, I thought postgresql.auto.conf was handled as an #include and therefore 
> you could still preempt it via postgresql.conf

It's not just that..  Having superuser access should really be
considered equivilant to having a shell as the unix user that postgres
is running as.

> >If this is being done for every execution of a query then I agree- SQL
> >or plpgsql probably wouldn't be fast enough.  That doesn't mean it makes
> >sense to have pgaudit support calling a C function, it simply means that
> >we need to find another way to configure auditing (which is what I think
> >I've done...).
> 
> I'm still nervous about overloading this onto the roles system; I think it 
> will end up being very easy to accidentally break. But if others think it'll 
> work then I guess I'm just being paranoid.

Break in which way..?  If you're saying "it'll be easy for a user to
misconfigure" then I might agree with you- but documentation and
examples can help to address that.  If you're worried that future PG
hacking will break it, well, I tend to doubt the GRANT piece is the area
of concern there- the recent development work is really around event
triggers and adding new object classes; the GRANT components have been
reasonably stable for the past few years.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL

[Jim Nasby]

On 1/21/15 5:38 PM, Stephen Frost wrote:

* Jim Nasby (jim.na...@bluetreble.com) wrote:

On 1/20/15 9:01 PM, Stephen Frost wrote:

* Jim Nasby (jim.na...@bluetreble.com) wrote:

+1. In particular I'm very concerned with the idea of doing this via roles, 
because that would make it trivial for any superuser to disable auditing. The 
only good option I could see to provide this kind of flexibility would be 
allowing the user to provide a function that accepts role, object, etc and make 
return a boolean. The performance of that would presumably suck with anything 
but a C function, but we could provide some C functions to handle simple cases.

Superusers will be able to bypass, trivially, anything that's done in
the process space of PG.  The only possible exception to that being an
SELinux or similar solution, but I don't think that's what you were
getting at.


Not if the GUC was startup-only. That would allow someone with OS access to the 
server to prevent a Postgres superuser from disabling it.


That is not accurate.

Being startup-only won't help if the user is a superuser.


Crap, I thought postgresql.auto.conf was handled as an #include and therefore 
you could still preempt it via postgresql.conf


I certainly don't think having the user provide a C function to specify
what should be audited as making any sense- if they can do that, they
can use the same hooks pgaudit is using and skip the middle-man.  As for
the performance concern you raise, I actually don't buy into it at all.
It's not like we worry about the performance of checking permissions on
objects in general and, for my part, I like to think that's because it's
pretty darn quick already.


I was only mentioning C because of performance concerns. If SQL or plpgsql is 
fast enough then there's no need.


If this is being done for every execution of a query then I agree- SQL
or plpgsql probably wouldn't be fast enough.  That doesn't mean it makes
sense to have pgaudit support calling a C function, it simply means that
we need to find another way to configure auditing (which is what I think
I've done...).


I'm still nervous about overloading this onto the roles system; I think it will 
end up being very easy to accidentally break. But if others think it'll work 
then I guess I'm just being paranoid.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.com


--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] jsonb, unicode escapes and escaped backslashes

[Andrew Dunstan]

The following case has just been brought to my attention (look at the 
differing number of backslashes):


   andrew=# select jsonb '"\\u"';
  jsonb
   --
 "\u"
   (1 row)

   andrew=# select jsonb '"\u"';
  jsonb
   --
 "\u"
   (1 row)

   andrew=# select json '"\u"';
   json
   --
 "\u"
   (1 row)

   andrew=# select json '"\\u"';
   json
   ---
 "\\u"
   (1 row)

The problem is that jsonb uses the parsed, unescaped value of the 
string, while json does not. when the string parser sees the input with 
the 2 backslashes, it outputs a single backslash, and then it encounters 
the remaining chareacters and emits them as is, resulting in a token of 
'\u'. When it encounters the input with one backslash, it recognizes 
a unicode escape, and because it's for u+ emits '\u'. All other 
unicode escapes are resolved, so the only abiguity on input concerns 
this case.


Things get worse, though. On output, '\uabcd' for any four hex digits is 
recognized as a unicode escape, and thus the backslash is not escaped, 
so that we get:


   andrew=# select jsonb '"\\uabcd"';
  jsonb
   --
 "\uabcd"
   (1 row)


We could probably fix this fairly easily for non- U+ cases by having 
jsonb_to_cstring use a different escape_json routine.


But it's a mess, sadly, and I'm not sure what a good fix for the U+ 
case would look like. Maybe we should detect such input and emit a 
warning of ambiguity? It's likely to be rare enough, but clearly not as 
rare as we'd like, since this is a report from the field.


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] pgaudit - an auditing extension for PostgreSQL

[Stephen Frost]

* Jim Nasby (jim.na...@bluetreble.com) wrote:
> On 1/20/15 9:01 PM, Stephen Frost wrote:
> >* Jim Nasby (jim.na...@bluetreble.com) wrote:
> >>>+1. In particular I'm very concerned with the idea of doing this via 
> >>>roles, because that would make it trivial for any superuser to disable 
> >>>auditing. The only good option I could see to provide this kind of 
> >>>flexibility would be allowing the user to provide a function that accepts 
> >>>role, object, etc and make return a boolean. The performance of that would 
> >>>presumably suck with anything but a C function, but we could provide some 
> >>>C functions to handle simple cases.
> >Superusers will be able to bypass, trivially, anything that's done in
> >the process space of PG.  The only possible exception to that being an
> >SELinux or similar solution, but I don't think that's what you were
> >getting at.
> 
> Not if the GUC was startup-only. That would allow someone with OS access to 
> the server to prevent a Postgres superuser from disabling it.

That is not accurate.

Being startup-only won't help if the user is a superuser.

> >I certainly don't think having the user provide a C function to specify
> >what should be audited as making any sense- if they can do that, they
> >can use the same hooks pgaudit is using and skip the middle-man.  As for
> >the performance concern you raise, I actually don't buy into it at all.
> >It's not like we worry about the performance of checking permissions on
> >objects in general and, for my part, I like to think that's because it's
> >pretty darn quick already.
> 
> I was only mentioning C because of performance concerns. If SQL or plpgsql is 
> fast enough then there's no need.

If this is being done for every execution of a query then I agree- SQL
or plpgsql probably wouldn't be fast enough.  That doesn't mean it makes
sense to have pgaudit support calling a C function, it simply means that
we need to find another way to configure auditing (which is what I think
I've done...).

Thanks,

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL

[Jim Nasby]

On 1/20/15 9:01 PM, Stephen Frost wrote:

* Jim Nasby (jim.na...@bluetreble.com) wrote:

>+1. In particular I'm very concerned with the idea of doing this via roles, 
because that would make it trivial for any superuser to disable auditing. The only 
good option I could see to provide this kind of flexibility would be allowing the 
user to provide a function that accepts role, object, etc and make return a 
boolean. The performance of that would presumably suck with anything but a C 
function, but we could provide some C functions to handle simple cases.

Superusers will be able to bypass, trivially, anything that's done in
the process space of PG.  The only possible exception to that being an
SELinux or similar solution, but I don't think that's what you were
getting at.


Not if the GUC was startup-only. That would allow someone with OS access to the 
server to prevent a Postgres superuser from disabling it.


I certainly don't think having the user provide a C function to specify
what should be audited as making any sense- if they can do that, they
can use the same hooks pgaudit is using and skip the middle-man.  As for
the performance concern you raise, I actually don't buy into it at all.
It's not like we worry about the performance of checking permissions on
objects in general and, for my part, I like to think that's because it's
pretty darn quick already.


I was only mentioning C because of performance concerns. If SQL or plpgsql is 
fast enough then there's no need.
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.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] Temporal features in PostgreSQL

[Bruce Momjian]

On Fri, Jan 16, 2015 at 09:58:22AM -0700, pe...@vanroose.be wrote:
> What's the current status of this topic?
> Has someone worked on temporal tables for PostgreSQL since 2012 ?
> 
> I'm giving a presentation on Fosdem later this month in Brussels, on the
> topic of temporal tables, and would like to give all possibly relevant
> information to the audience!

I have not heard of anyone working on this.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] B-Tree support function number 3 (strxfrm() optimization)

[Andrew Gierth]

> "Peter" == Peter Geoghegan  writes:

 Peter> Okay, then. I concede the point: We should support the datum
 Peter> case as you outline, since it is simpler than any
 Peter> alternative. It probably won't even be necessary to formalize
 Peter> the idea that finished abbreviated keys must be pass-by-value
 Peter> (at least not for the benefit of this functionality); if someone
 Peter> writes an opclass that generates pass-by-reference abbreviated
 Peter> keys (I think that might actually make sense, although I'm being
 Peter> imaginative), it simply won't work for the datum sort case,
 Peter> which is probably fine.

I don't see why a by-reference abbreviated key would be any more of an
issue for the datum sorter than for anything else. In either case you'd
just get excess memory usage (any memory allocated by the abbreviation
function for the result won't be charged against work_mem and won't be
freed until the sort ends).

What matters for the datum sorter (and not for the others) is that we
not try and abbreviate a by-value type (since we only have an allocated
copy of the value if it was by-reference); this is handled in the code
by just not asking for abbreviation in such cases.

 Peter> Are you going to submit this to the final commitfest? I'll
 Peter> review it if you do.

I'll post a cleaned-up version after the existing issues are fixed.

-- 
Andrew (irc:RhodiumToad)


-- 
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] B-Tree support function number 3 (strxfrm() optimization)

[Peter Geoghegan]

On Wed, Jan 21, 2015 at 2:11 PM, Peter Geoghegan  wrote:
> Okay, then. I concede the point: We should support the datum case as
> you outline, since it is simpler than any alternative. It probably
> won't even be necessary to formalize the idea that finished
> abbreviated keys must be pass-by-value (at least not for the benefit
> of this functionality); if someone writes an opclass that generates
> pass-by-reference abbreviated keys (I think that might actually make
> sense, although I'm being imaginative), it simply won't work for the
> datum sort case, which is probably fine.

I mean that a restriction formally preventing use of abbreviation with
pass-by-value types isn't necessary. That was something that I thought
we'd have to document as a restriction (for the benefit of your datum
sort patch), without considering that it could simply be skipped by
only considering state->datumTypeByVal (which is what you've proposed
here).

This requirement is much less likely than wanting to create
pass-by-value abbreviated keys for a pass-by-value datatype (which, as
I go into above, seems at least possible). This seems like a very
insignificant restriction, not worth formalizing or even mentioning in
code comments.

-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] proposal: plpgsql - Assert statement

[Jim Nasby]

On 1/21/15 3:10 PM, Pavel Stehule wrote:


is there some agreement on this behave of ASSERT statement?

I would to assign last patch to next commitfest.

Possible changes:

* I would to simplify a behave of evaluating of message expression - probably I 
disallow NULL there.


Well, the only thing I could see you doing there is throwing a different error 
if the hint is null. I don't see that as an improvement. I'd just leave it 
as-is.


* GUC enable_asserts will be supported


That would be good. Would that allow for enabling/disabling on a per-function 
basis too?


* a assert exception should not be handled by PLpgSQL handler -- like CANCEL 
exception


+1
--
Jim Nasby, Data Architect, Blue Treble Consulting
Data in Trouble? Get it in Treble! http://BlueTreble.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] B-Tree support function number 3 (strxfrm() optimization)

[Peter Geoghegan]

On Wed, Jan 21, 2015 at 4:44 AM, Andrew Gierth
 wrote:
> Now, I follow this general principle that someone who is not doing the
> work should never say "X is easy" to someone who _is_ doing it, unless
> they're prepared to at least outline the solution on request or
> otherwise contribute.  So see the attached patch (which I will concede
> could probably do with more comments, it's a quick hack intended for
> illustration) and tell me what you think is missing that would make it a
> complicated problem.

Okay, then. I concede the point: We should support the datum case as
you outline, since it is simpler than any alternative. It probably
won't even be necessary to formalize the idea that finished
abbreviated keys must be pass-by-value (at least not for the benefit
of this functionality); if someone writes an opclass that generates
pass-by-reference abbreviated keys (I think that might actually make
sense, although I'm being imaginative), it simply won't work for the
datum sort case, which is probably fine.

Are you going to submit this to the final commitfest? I'll review it if you do.
-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] proposal: plpgsql - Assert statement

[Pavel Stehule]

Hi all

is there some agreement on this behave of ASSERT statement?

I would to assign last patch to next commitfest.

Possible changes:

* I would to simplify a behave of evaluating of message expression -
probably I disallow NULL there.
* GUC enable_asserts will be supported
* a assert exception should not be handled by PLpgSQL handler -- like
CANCEL exception

Regards

Pavel



2014-12-14 19:54 GMT+01:00 Pavel Stehule :

>
>
> 2014-12-14 18:57 GMT+01:00 Alvaro Herrera :
>>
>> Pavel Stehule wrote:
>> > Hi
>> >
>> > any comments to last proposal and patch?
>>
>> WTH is that pstrdup("hint is null") thing?  Debugging leftover?
>>
>
> No, only not well error message. I propose a syntax ASSERT
> boolean_expression [, text expression ] -- text expression is >>hint<< for
> assertion debugging
>
> This text expression should not be null when it is used. I am not sure,
> what is well behave - so when assertions fails and text expression is null,
> then I use message "hint is null" as hint.
>
> Regards
>
> Pavel
>
>
>>
>> --
>> Álvaro Herrerahttp://www.2ndQuadrant.com/
>> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>>
>

Re: [HACKERS] New CF app: changing email sender

[Magnus Hagander]

On Tue, Jan 20, 2015 at 1:44 AM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> Hello, the new app's clean looking gets my favor and the
> integrated operation will do good for me:)
>
> By the way, I got the following notice when I tried to "Add
> comment" on the new app.
>
> "Note!: This form will generate an email to the public
>  mailinglist pgsql-hackers, with sender set to
>  horiguchi.kyot...@oss.ntt.co.jp!"
>
> Hmm. The mail address indeed *was* mine but is now obsolete, so
> that the email might bounce. But I haven't find how to change it
> within the app itself, and the PostgreSQL community account page.
>
> https://www.postgresql.org/account/
>
> Could you tell me how do I change the sender address?
>

Hi!

I've just pushed a change to the main website which now lets you change the
email address there. That meas you can go to
https://www.postgresql.org/account/profile/ and change the email. After you
have confirmed the change (an email will be sent to your new address when
you change it), the the new one should be valid on the cf app (if it
doesn't show up, log out of the cf app and back in, and it should show up).

I will look into Andrews issue of being able to have multiple email
addresses soon as well - but one feature per evening :)

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] proposal: lock_time for pg_stat_database

[Pavel Stehule]

Hi

2015-01-16 20:33 GMT+01:00 Jim Nasby :

> On 1/16/15 12:30 PM, Pavel Stehule wrote:
>
>>
>>
>> 2015-01-16 19:24 GMT+01:00 Pavel Stehule > >:
>>
>>
>>
>> 2015-01-16 19:06 GMT+01:00 Jim Nasby > >:
>>
>> On 1/16/15 11:35 AM, Pavel Stehule wrote:
>>
>>
>>
>> 2015-01-16 18:23 GMT+01:00 Jim Nasby <
>> jim.na...@bluetreble.com  > Jim.Nasby@bluetreble.__com >>:
>>
>>  On 1/16/15 11:00 AM, Pavel Stehule wrote:
>>
>>  Hi all,
>>
>>  some time ago, I proposed a lock time measurement
>> related to query. A main issue was a method, how to show this information.
>> Today proposal is little bit simpler, but still useful. We can show a total
>> lock time per database in pg_stat_database statistics. High number can be
>> signal about lock issues.
>>
>>
>>  Would this not use the existing stats mechanisms? If so,
>> couldn't we do this per table? (I realize that won't handle all cases; we'd
>> still need a "lock_time_other" somewhere).
>>
>>
>>
>> it can use a current existing stats mechanisms
>>
>> I afraid so isn't possible to assign waiting time to table -
>> because it depends on order
>>
>>
>> Huh? Order of what?
>>
>>
>> when you have a SELECT FROM T1, T2 and T1 is locked for t1, and T2 is
>> locked for t2 -- but if t2 < t1 then t2 is not important -- so what I have
>> to cont as lock time for T1 and T2?
>>
>
> If that select is waiting on a lock on t2, then it's waiting on that lock
> on that table. It doesn't matter who else has the lock.
>
>   Also, what do you mean by 'lock'? Heavyweight? We
>> already have some visibility there. What I wish we had was some way to know
>> if we're spending a lot of time in a particular non-heavy lock. Actually
>> measuring time probably wouldn't make sense but we might be able to count
>> how often we fail initial acquisition or something.
>>
>>
>> now, when I am thinking about it, lock_time is not good name
>> - maybe "waiting lock time" (lock time should not be interesting, waiting
>> is interesting) - it can be divided to some more categories - in GoodData
>> we use Heavyweight, pages, and others categories.
>>
>>
>> So do you see this somehow encompassing locks other than
>> heavyweight locks? Because I think that's the biggest need here. Basically,
>> something akin to TRACE_POSTGRESQL_LWLOCK_WAIT___START() that doesn't
>> depend on dtrace.
>>
>>
>> For these global statistics I see as important a common total waiting
>> time for locks - we can use a more detailed granularity but I am not sure,
>> if a common statistics are best tool.
>>
>
> Locks may be global, but what you're waiting for a lock on certainly
> isn't. It's almost always a lock either on a table or a row in a table. Of
> course this does mean you can't just blindly report that you're blocked on
> some XID; that doesn't tell anyone anything.
>
>  My motivations is - look to statistics -- and I can see ... lot of
>> rollbacks -- issue, lot of deadlocks -- issue, lot of waiting time -- issue
>> too. It is tool for people without possibility to use dtrace and similar
>> tools and for everyday usage - simple check if locks are not a issue (or if
>> locking is stable).
>>
>
> Meh. SELECT sum(state_change) FROM pg_stat_activity WHERE waiting is just
> about as useful. Or just turn on lock logging.
>
> If you really want to add it at the database level I'm not opposed (so
> long as it leaves the door open for more granular locking later), but I
> can't really get excited about it either.
>
>  and this proposal has sense only for heavyweight locks - because others
>> locks are everywhere
>>
>
> So what if they're everywhere? Right now if you're spending a lot of time
> waiting for LWLocks you have no way to know what's going on unless you
> happen to have dtrace. Obviously we're not going to something like issue a
> stats update every time we attempt to acquire an LWLock, but that doesn't
> mean we can't keep some counters on the locks and periodically report that.



I was wrong - probably is possible to attach lock waiting time per table

Regards

Pavel


>
> --
> Jim Nasby, Data Architect, Blue Treble Consulting
> Data in Trouble? Get it in Treble! http://BlueTreble.com
>

[HACKERS] Windows buildfarm animals are still not happy with abbreviated keys patch

[Peter Geoghegan]

Even following Robert's disabling of abbreviated keys on Windows,
buildfarm animals hamerkop, brolga, currawong and bowerbird remain
unhappy, with failing regression tests for "collate" and sometimes
(but not always) "aggregates". Some of these only use the C locale.

I think that "aggregates" does not fail for brolga because it's built
without "locale_t" support (not sure how to interpret MSVC "configure"
output, but I think that the other animals do have such support).  So
maybe this code being executed within btsortsupport_worker(), for the
C locale on Windows is at issue (note that abbreviation is still
disabled):

tss->locale = pg_newlocale_from_collation(collid);

That doesn't explain the other problems, though.
-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pg_stat_statements vs escape_string_warning

[Tom Lane]

Peter Geoghegan  writes:
> On Wed, Jan 21, 2015 at 10:14 AM, Tom Lane  wrote:
>> This isn't a back-patchable bug fix, but given the lack of prior
>> complaints, maybe it doesn't matter.  Alternatively, we could back-patch
>> only the addition of escape_string_warning to the struct: that would fit
>> into padding space in the struct so that there would be no ABI risk.

> I think that this is a good idea, but I see very little reason to
> back-patch. I'm not aware that the "padding space" argument has been
> used for something like this before.

Oh, we definitely *have* done that kind of thing in the past, when there
was sufficient motivation.  But I'm not sure there's sufficient motivation
here.

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] pg_stat_statements vs escape_string_warning

[Peter Geoghegan]

On Wed, Jan 21, 2015 at 10:14 AM, Tom Lane  wrote:
> What I'm inclined to do about this is add an escape_string_warning bool
> field to struct core_yy_extra_type, which would be copied from the GUC
> variable by scanner_init(); then check_string_escape_warning() would
> consult that field instead of consulting the GUC directly.  It would
> then be possible for fill_in_constant_lengths to reset that field after
> calling scanner_init so that no warnings appear during its reparse.

I've noticed this too, and found it annoying.

> As a matter of cleanliness I'm inclined to do the same with
> backslash_quote and standard_conforming_strings, so that callers of the
> core lexer are not tied to using the prevailing GUC settings but can
> control all these things.

+1

> This isn't a back-patchable bug fix, but given the lack of prior
> complaints, maybe it doesn't matter.  Alternatively, we could back-patch
> only the addition of escape_string_warning to the struct: that would fit
> into padding space in the struct so that there would be no ABI risk.
>
> Comments, objections?

I think that this is a good idea, but I see very little reason to
back-patch. I'm not aware that the "padding space" argument has been
used for something like this before.
-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pg_stat_statements vs escape_string_warning

[Tom Lane]

I happened to notice that if you run the regression tests with
pg_stat_statements installed, you will often (not always) get
a failure that looks like this:

*** src/test/regress/expected/plpgsql.out Tue Jan 20 12:01:52 2015
--- src/test/regress/results/plpgsql.out Wed Jan 21 12:43:19 2015
***
*** 4672,4677 
--- 4672,4683 
  HINT:  Use the escape string syntax for backslashes, e.g., E'\\'.
  QUERY:  SELECT 'foo\\bar\041baz'
  CONTEXT:  PL/pgSQL function strtest() line 4 at RETURN
+ WARNING:  nonstandard use of \\ in a string literal
+ LINE 1: SELECT 'foo\\bar\041baz'
+^
+ HINT:  Use the escape string syntax for backslashes, e.g., E'\\'.
+ QUERY:  SELECT 'foo\\bar\041baz'
+ CONTEXT:  PL/pgSQL function strtest() line 4 at RETURN
 strtest
  -
   foo\bar!baz

That is, we're getting an extra copy of the "escape string warning"
message.  Investigation disclosed that the reason is that 
pg_stat_statements' pgss_post_parse_analyze hook re-runs the lexer
over the query string (see fill_in_constant_lengths()), so that you
get an extra instance of any side-effects in the lexer.

This is kind of annoying, especially since it's nondeterministic ---
if there's already a pg_stat_statements entry matching "SELECT ?" then
you don't see the extra warning.

What I'm inclined to do about this is add an escape_string_warning bool
field to struct core_yy_extra_type, which would be copied from the GUC
variable by scanner_init(); then check_string_escape_warning() would
consult that field instead of consulting the GUC directly.  It would
then be possible for fill_in_constant_lengths to reset that field after
calling scanner_init so that no warnings appear during its reparse.

As a matter of cleanliness I'm inclined to do the same with
backslash_quote and standard_conforming_strings, so that callers of the
core lexer are not tied to using the prevailing GUC settings but can
control all these things.

This isn't a back-patchable bug fix, but given the lack of prior
complaints, maybe it doesn't matter.  Alternatively, we could back-patch
only the addition of escape_string_warning to the struct: that would fit
into padding space in the struct so that there would be no ABI risk.

Comments, objections?

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] moving from contrib to bin

[Bruce Momjian]

On Sat, Jan 17, 2015 at 02:08:34PM +0100, Andres Freund wrote:
> 7) Are we sure that the authors in the affected contrib modules are ok
>with their authorship notice being removed? I don't think Ants, Bruce
>or Simon have a problem with that, but ...

I am fine.  It means others can be blamed.  ;-)

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] moving from contrib to bin

[Bruce Momjian]

On Sat, Jan 17, 2015 at 01:16:18PM +0100, Andres Freund wrote:
> Hi,
> 
> FWIW, I find it rather annoying if people attach patchsets as
> tarballs. That makes it impossible to look at them in the mailreader
> since I really don't have anything reasonable to go on to teach it to
> treat it as a set of patches.
> 
> I'd also like to see patches that primarily move code around as git diff
> -M -C style diffs (can also be passed to format-patch). That will show
> the file move and then additionally the changes that have been made in
> addition to the rename. There's no sane way the current diffs can be
> reviewed without applying them to a tree.

FYI, the .gitconfig setting is 'renames':

[diff]
renames = copies

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

  + Everyone has their own god. +


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Add min and max execute statement time in pg_stat_statement

[Arne Scheffer]

Andrew Dunstan schrieb am 2015-01-21:

> On 01/21/2015 11:21 AM, Arne Scheffer wrote:



> >Why is it a bad thing to call the column "stddev_samp" analog to the
> >aggregate function or make a note in the documentation, that the
> >sample stddev is used to compute the solution?


> I think you are making a mountain out of a molehill, frankly. These
> stats are not intended as anything other than a pretty indication of
> the
> shape, to see if they are significantly influenced by outliers. For
> any
> significantly large sample size the difference will be negligible.

You're right, I maybe exaggerated the statistics part a bit.
I wanted to help, because the patch is of interest for us.
I will try to keep focus in the future.


> But I will add a note to the documentation, that seems reasonable.


*happy*

Thx

Arne



-- 
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] Add min and max execute statement time in pg_stat_statement

[Arne Scheffer]

On Wed, 21 Jan 2015, Andrew Dunstan wrote:



On 01/21/2015 09:27 AM, Arne Scheffer wrote:

Sorry, corrected second try because of copy&paste mistakes:
VlG-Arne


Comments appreciated.
Definition var_samp = Sum of squared differences /n-1
Definition stddev_samp = sqrt(var_samp)
Example N=4
1.) Sum of squared differences
   1_4Sum(Xi-XM4)²
=
2.) adding nothing
   1_4Sum(Xi-XM4)²
  +0
  +0
  +0
=
3.) nothing changed
  1_4Sum(Xi-XM4)²
  +(-1_3Sum(Xi-XM3)²+1_3Sum(Xi-XM3)²)
  +(-1_2Sum(Xi-XM2)²+1_2Sum(Xi-XM2)²)
  +(-1_1Sum(Xi-XM1)²+1_1Sum(Xi-XM1)²)
=
4.) parts reordered
   (1_4Sum(Xi-XM4)²-1_3Sum(Xi-XM3)²)
  +(1_3Sum(Xi-XM3)²-1_2Sum(Xi-XM2)²)
  +(1_2Sum(Xi-XM2)²-1_1Sum(Xi-XM1)²)
  +1_1Sum(X1-XM1)²
=
5.)
   (X4-XM4)(X4-XM3)
+ (X3-XM3)(X3-XM2)
+ (X2-XM2)(X2-XM1)
+ (X1-XM1)²
=
6.) XM1=X1 => There it is - The iteration part of Welfords Algorithm
(in
reverse order)
   (X4-XM4)(X4-XM3)
+ (X3-XM3)(X3-XM2)
+ (X2-XM2)(X2-X1)
+ 0
The missing piece is 4.) to 5.)
it's algebra, look at e.g.:
http://jonisalonen.com/2013/deriving-welfords-method-for-computing-variance/







I have no idea what you are saying here.


I'm sorry for that statistics stuff, 
my attempt was only to visualize in detail 
the mathematical reason for 
the iterating part of Welfords algorithm

being computing the current sum of squared differences in every step

- therefore it's in my opinion better to call the variable sum_of_squared_diffs
  (every statistician will be confused bei "sum_of_variances",
   because:  sample variance = sum_of_squared_diffs / n-1,
   have a look at Mr. Cooks explanation)

- therefore deviding by n-1 is the unbiased estimator by definition.
  (have a look at Mr. Cooks explanation)

- therefore I suggested (as a minor nomenclature issue) to call the 
column/description
  stdev_samp (PostgreSQL-nomenclature) / sample_ to indicate that 
information.
  (have a look at the PostgreSQL aggregate functions, it's doing that the same 
way)



Here are comments in email to me from the author of 
 regarding the divisor 
used:


  My code is using the unbiased form of the sample variance, dividing
  by n-1.



I am relieved, now we are at least two persons saying that. :-)
Insert into the commonly known definition


Definition stddev_samp = sqrt(var_samp)


from above, and it's exactly my point.

Maybe I should add that in the code comments. Otherwise, I don't think we 
need a change.


Huh?

Why is it a bad thing to call the column "stddev_samp" analog to the
aggregate function or make a note in the documentation, 
that the sample stddev is used to compute the solution?


I really think it not a good strategy having the user to make a test or dive
into the source code to determine the divisor used.

E.g. David expected stdev_pop, so there is a need for documentation for cases 
with a small sample.

VlG-Arne

--
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] Add min and max execute statement time in pg_stat_statement

[Andrew Dunstan]

On 01/21/2015 11:21 AM, Arne Scheffer wrote:




Why is it a bad thing to call the column "stddev_samp" analog to the
aggregate function or make a note in the documentation, that the 
sample stddev is used to compute the solution?



I think you are making a mountain out of a molehill, frankly. These 
stats are not intended as anything other than a pretty indication of the 
shape, to see if they are significantly influenced by outliers. For any 
significantly large sample size the difference will be negligible.


But I will add a note to the documentation, that seems reasonable.

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] Additional role attributes && superuser review

[Adam Brightwell]

All,

>
> I'm slightly mystified as to how including the word "online" helps
> here.  It's unlikely that there will be an offline_backup permission,
> because if the system is off-line, SQL-level permissions are
> irrelevant.


After re-reading through this thread is seems like EXCLUSIVEBACKUP
(proposed by Magnus) seemed to be a potentially acceptable alternative.


Relevant post:
http://www.postgresql.org/message-id/cabuevez7bz0r85vut4rvxx0jkpih8hp8toqzgvpufl0kvcv...@mail.gmail.com

We need to separate the logical backups (pg_dump) from the physical ones
(start/stop+filesystem and pg_basebackup). We might also need to separate
the two different ways of doing physical backups.

Personalyl I think using the DUMP name makes that a lot more clear. Maybe
we need to avoid using BACKUP alone as well, to make sure it doesn't go the
other way - using BASEBACKUP and EXCLUSIVEBACKUP for those two different
ones perhaps?



Relevant post:
http://www.postgresql.org/message-id/20141231144610.gs3...@tamriel.snowman.net

I think I'm coming around to the EXCLUSIVEBACKUP option, per the
discussion with Magnus.  I don't particularly like LOGBACKUP and don't
think it really makes sense, while PHYSBACKUP seems like it'd cover what
REPLICATION already does.


Thoughts?

-Adam

--
Adam Brightwell - adam.brightw...@crunchydatasolutions.com
Database Engineer - www.crunchydatasolutions.com

Re: [HACKERS] Merging postgresql.conf and postgresql.auto.conf

[Sawada Masahiko]

On Wed, Jan 21, 2015 at 3:38 PM, Amit Kapila  wrote:
> On Tue, Jan 20, 2015 at 9:38 PM, Robert Haas  wrote:
>
> Okay and I was also not in favour of this approach.
>

Okay I agree with this.

>
> The reason why "sourcefile" and "sourceline" are not sufficient is that
> they can only give the information about the setting in last file it is
> present.  Assume max_connections (or any other setting) is available
> in both postgresql.conf and postgresql.auto.conf, then it will display
> the information about the setting in postgresql.auto.conf, so now user
> might not be able to decide whether that is the setting he want to retain
> unless he knows the information about setting in postgresql.conf.
>
> Now as I have suggested upthread, that we can have a new view
> pg_file_settings which will display information about settings even
> when there exists multiple entries for the same in different files.
>
> I think adding such information to existing view pg_settings would
> be difficult as the same code is used for show commands which
> we don't want to change.
>

I think this new view is updated only when postmaster received SIGHUP
or is started.
And we can have new function like pg_update_file_setting() which
updates this view.

Regards,

---
Sawada Masahiko


-- 
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] TODO : Allow parallel cores to be used by vacuumdb [ WIP ]

[Alvaro Herrera]

I didn't understand the coding in GetQueryResult(); why do we check the
result status of the last returned result only?  It seems simpler to me
to check it inside the loop, but maybe there's a reason you didn't do it
like that?

Also, what is the reason we were ignoring those errors only in
"completedb" mode?  It doesn't seem like it would cause any harm if we
did it in all cases.  That means we can just not have the "completeDb"
flag at all.

Finally, I think it's better to report the "missing relation" error,
even if we're going to return true to continue processing other tables.
That makes the situation clearer to the user.

So the function would end up looking like this:

/*
 * GetQueryResult
 *
 * Process the query result.  Returns true if there's no error, false
 * otherwise -- but errors about trying to vacuum a missing relation are
 * ignored.
 */
static bool
GetQueryResult(PGconn *conn, errorOptions *erropts)
{
PGresult*result = NULL;

SetCancelConn(conn);
while ((result = PQgetResult(conn)) != NULL)
{
/*
 * If errors are found, report them.  Errors about a missing 
table are
 * harmless so we continue processing, but die for other errors.
 */
if (PQresultStatus(result) != PGRES_COMMAND_OK)
{
char *sqlState = PQresultErrorField(result, 
PG_DIAG_SQLSTATE);

fprintf(stderr, _("%s: vacuuming of database \"%s\" 
failed: %s"),
erropts->progname, erropts->dbname, 
PQerrorMessage(conn));

if (sqlState && strcmp(sqlState, 
ERRCODE_UNDEFINED_TABLE) != 0)
{
PQclear(result);
return false;
}
}

PQclear(result);
}
ResetCancelConn();

return true;
}



-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Add min and max execute statement time in pg_stat_statement

[Andrew Dunstan]

On 01/21/2015 09:27 AM, Arne Scheffer wrote:

Sorry, corrected second try because of copy&paste mistakes:
VlG-Arne


Comments appreciated.
Definition var_samp = Sum of squared differences /n-1
Definition stddev_samp = sqrt(var_samp)
Example N=4
1.) Sum of squared differences
   1_4Sum(Xi-XM4)²
=
2.) adding nothing
   1_4Sum(Xi-XM4)²
  +0
  +0
  +0
=
3.) nothing changed
  1_4Sum(Xi-XM4)²
  +(-1_3Sum(Xi-XM3)²+1_3Sum(Xi-XM3)²)
  +(-1_2Sum(Xi-XM2)²+1_2Sum(Xi-XM2)²)
  +(-1_1Sum(Xi-XM1)²+1_1Sum(Xi-XM1)²)
=
4.) parts reordered
   (1_4Sum(Xi-XM4)²-1_3Sum(Xi-XM3)²)
  +(1_3Sum(Xi-XM3)²-1_2Sum(Xi-XM2)²)
  +(1_2Sum(Xi-XM2)²-1_1Sum(Xi-XM1)²)
  +1_1Sum(X1-XM1)²
=
5.)
   (X4-XM4)(X4-XM3)
+ (X3-XM3)(X3-XM2)
+ (X2-XM2)(X2-XM1)
+ (X1-XM1)²
=
6.) XM1=X1 => There it is - The iteration part of Welfords Algorithm
(in
reverse order)
   (X4-XM4)(X4-XM3)
+ (X3-XM3)(X3-XM2)
+ (X2-XM2)(X2-X1)
+ 0
The missing piece is 4.) to 5.)
it's algebra, look at e.g.:
http://jonisalonen.com/2013/deriving-welfords-method-for-computing-variance/







I have no idea what you are saying here.

Here are comments in email to me from the author of 
 regarding the divisor 
used:


   My code is using the unbiased form of the sample variance, dividing
   by n-1.

   It's usually not worthwhile to make a distinction between a sample
   and a population because the "population" is often itself a sample.
   For example, if you could measure the height of everyone on earth at
   one instance, that's the entire population, but it's still a sample
   from all who have lived and who ever will live.

   Also, for large n, there's hardly any difference between 1/n and
   1/(n-1).


Maybe I should add that in the code comments. Otherwise, I don't think 
we need a change.



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] Add min and max execute statement time in pg_stat_statement

[Arne Scheffer]

Sorry, corrected second try because of copy&paste mistakes:
VlG-Arne

> Comments appreciated.

> Definition var_samp = Sum of squared differences /n-1
> Definition stddev_samp = sqrt(var_samp)

> Example N=4

> 1.) Sum of squared differences
>   1_4Sum(Xi-XM4)²
> =
> 2.) adding nothing
>   1_4Sum(Xi-XM4)²
>  +0
>  +0
>  +0
> =
> 3.) nothing changed
>  1_4Sum(Xi-XM4)²
>  +(-1_3Sum(Xi-XM3)²+1_3Sum(Xi-XM3)²)
>  +(-1_2Sum(Xi-XM2)²+1_2Sum(Xi-XM2)²)
>  +(-1_1Sum(Xi-XM1)²+1_1Sum(Xi-XM1)²)

> =
> 4.) parts reordered
>   (1_4Sum(Xi-XM4)²-1_3Sum(Xi-XM3)²)
>  +(1_3Sum(Xi-XM3)²-1_2Sum(Xi-XM2)²)
>  +(1_2Sum(Xi-XM2)²-1_1Sum(Xi-XM1)²)
>  +1_1Sum(X1-XM1)²
> =
> 5.)
>   (X4-XM4)(X4-XM3)
> + (X3-XM3)(X3-XM2)
> + (X2-XM2)(X2-XM1)
> + (X1-XM1)²
> =
> 6.) XM1=X1 => There it is - The iteration part of Welfords Algorithm
> (in
> reverse order)
>   (X4-XM4)(X4-XM3)
> + (X3-XM3)(X3-XM2)
> + (X2-XM2)(X2-X1)
> + 0

> The missing piece is 4.) to 5.)
> it's algebra, look at e.g.:
> http://jonisalonen.com/2013/deriving-welfords-method-for-computing-variance/




-- 
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] Add min and max execute statement time in pg_stat_statement

[Arne Scheffer]

> >>I don't understand. I'm following pretty exactly the calculations
> >>stated
> >>at ;

> >>I'm not a statistician. Perhaps others who are more literate in

Maybe I'm mistaken here,
but I think, the algorithm is not that complicated.
I try to explain it further:

Comments appreciated.

Definition var_samp = Sum of squared differences /n-1
Definition stddev_samp = sqrt(var_samp)

Example N=4

1.) Sum of squared differences
  1_4Sum(Xi-XM4)²
=
2.) adding nothing
  1_4Sum(Xi-XM4)²
 +0
 +0
 +0
=
3.) nothing changed
 1_4Sum(Xi-XM4)²
 +(-1_3Sum(Xi-XM3)²+1_3Sum(Xi-XM3)²)
 +(-1_2Sum(Xi-XM2)²+1_2Sum(Xi-XM3)²)
 +(-1_1Sum(Xi-XM2)²+1_1Sum(Xi-XM3)²)

=
4.) parts reordered
  (1_4Sum(Xi-XM4)²-1_3Sum(Xi-XM3)²)
 +(1_3Sum(Xi-XM3)²-1_2Sum(Xi-XM2)²)
 +(1_2Sum(Xi-XM2)²-1_1Sum(Xi-XM2)²)
 +1_1Sum(X1-XM1)²
=
5.)
  (X4-XM4)(X4-XM3)
+ (X3-XM3)(X3-XM2)
+ (X2-XM2)(X2-XM1)
+ (X1-XM1)²
=
6.) XM1=X1 => There it is - The iteration part of Welfords Algorithm (in
reverse order)
  (X4-XM4)(X4-XM3)
+ (X3-XM3)(X3-XM2)
+ (X2-XM2)(X2-X1)
+ 0

The missing piece is 4.) to 5.)
it's algebra, look at e.g.:
http://jonisalonen.com/2013/deriving-welfords-method-for-computing-variance/

> Thanks. Still not quite sure what to do, though :-) I guess in the
> end we want the answer to come up with similar results to the builtin
> stddev SQL function. I'll try to set up a test program, to see if we do.

If you want to go this way:
Maybe this is one of the very few times, you have to use a small sample
;-)

VlG-Arne


> 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


-- 
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] Add min and max execute statement time in pg_stat_statement

[Arne Scheffer]

David G Johnston schrieb am 2015-01-21:
> Andrew Dunstan wrote
> > On 01/20/2015 01:26 PM, Arne Scheffer wrote:

> >> And a very minor aspect:
> >> The term "standard deviation" in your code stands for
> >> (corrected) sample standard deviation, I think,
> >> because you devide by n-1 instead of n to keep the
> >> estimator unbiased.
> >> How about mentioning the prefix "sample"
> >> to indicate this beiing the estimator?


> > I don't understand. I'm following pretty exactly the calculations
> > stated
> > at ;


> > I'm not a statistician. Perhaps others who are more literate in
> > statistics can comment on this paragraph.

> I'm largely in the same boat as Andrew but...

> I take it that Arne is referring to:

> http://en.wikipedia.org/wiki/Bessel's_correction

Yes, it is.

> but the mere presence of an (n-1) divisor does not mean that is what
> is
> happening.  In this particular situation I believe the (n-1) simply
> is a
> necessary part of the recurrence formula and not any attempt to
> correct for
> sampling bias when estimating a population's variance.

That's wrong, it's applied in the end to the sum of squared differences
and therefore per definition the corrected sample standard deviation
estimator.

> In fact, as
> far as
> the database knows, the values provided to this function do represent
> an
> entire population and such a correction would be unnecessary.  I

That would probably be an exotic assumption in a working database
and it is not, what is computed here!

> guess it
> boils down to whether "future" queries are considered part of the
> population
> or whether the population changes upon each query being run and thus
> we are
> calculating the ever-changing population variance.

Yes, indeed correct.
And exactly to avoid that misunderstanding, I suggested to
use the "sample" term.
To speak in Postgresql terms; applied in Andrews/Welfords algorithm
is stddev_samp(le), not stddev_pop(ulation).
Therefore stddev in Postgres is only kept for historical reasons, look at
http://www.postgresql.org/docs/9.4/static/functions-aggregate.html
Table 9-43.

VlG-Arne

> Note point 3 in
> the
> linked Wikipedia article.





> David J.



> --
> View this message in context:
> http://postgresql.nabble.com/Add-min-and-max-execute-statement-time-in-pg-stat-statement-tp5774989p5834805.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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] parallel mode and parallel contexts

[Amit Kapila]

On Wed, Jan 21, 2015 at 6:35 PM, Robert Haas  wrote:
>
> On Wed, Jan 21, 2015 at 2:11 AM, Amit Kapila 
wrote:
> > On Tue, Jan 20, 2015 at 9:52 PM, Robert Haas 
wrote:
> >> On Tue, Jan 20, 2015 at 9:41 AM, Amit Kapila 
> >> wrote:
> >> > It seems [WaitForBackgroundWorkerShutdown] has possibility to wait
> >> > forever.
> >> > Assume one of the worker is not able to start (not able to attach
> >> > to shared memory or some other reason), then status returned by
> >> > GetBackgroundWorkerPid() will be BGWH_NOT_YET_STARTED
> >> > and after that it can wait forever in WaitLatch.
> >>
> >> I don't think that's right.  The status only remains
> >> BGWH_NOT_YET_STARTED until the postmaster forks the child process.
> >
> > I think the control flow can reach the above location before
> > postmaster could fork all the workers.  Consider a case that
> > we have launched all workers during ExecutorStart phase
> > and then before postmaster could start all workers an error
> > occurs in master backend and then it try to Abort the transaction
> > and destroy the parallel context, at that moment it will get the
> > above status and wait forever in above code.
> >
> > I am able to reproduce above scenario with debugger by using
> > parallel_seqscan patch.
>
> Hmm.  Well, if you can reproduce it, there clearly must be a bug.  But
> I'm not quite sure where.  What should happen in that case is that the
> process that started the worker has to wait for the postmaster to
> actually start it,

Okay, I think this should solve the issue, also it should be done
before call of TerminateBackgroundWorker().


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] parallel mode and parallel contexts

[Robert Haas]

On Wed, Jan 21, 2015 at 2:11 AM, Amit Kapila  wrote:
> On Tue, Jan 20, 2015 at 9:52 PM, Robert Haas  wrote:
>> On Tue, Jan 20, 2015 at 9:41 AM, Amit Kapila 
>> wrote:
>> > It seems [WaitForBackgroundWorkerShutdown] has possibility to wait
>> > forever.
>> > Assume one of the worker is not able to start (not able to attach
>> > to shared memory or some other reason), then status returned by
>> > GetBackgroundWorkerPid() will be BGWH_NOT_YET_STARTED
>> > and after that it can wait forever in WaitLatch.
>>
>> I don't think that's right.  The status only remains
>> BGWH_NOT_YET_STARTED until the postmaster forks the child process.
>
> I think the control flow can reach the above location before
> postmaster could fork all the workers.  Consider a case that
> we have launched all workers during ExecutorStart phase
> and then before postmaster could start all workers an error
> occurs in master backend and then it try to Abort the transaction
> and destroy the parallel context, at that moment it will get the
> above status and wait forever in above code.
>
> I am able to reproduce above scenario with debugger by using
> parallel_seqscan patch.

Hmm.  Well, if you can reproduce it, there clearly must be a bug.  But
I'm not quite sure where.  What should happen in that case is that the
process that started the worker has to wait for the postmaster to
actually start it, and then after that for the new process to die, and
then it should return.

-- 
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] B-Tree support function number 3 (strxfrm() optimization)

[Andrew Gierth]

> "Peter" == Peter Geoghegan  writes:

 Peter> Basically, the intersection of the datum sort case with
 Peter> abbreviated keys seems complicated.

Not to me. To me it seems completely trivial.

Now, I follow this general principle that someone who is not doing the
work should never say "X is easy" to someone who _is_ doing it, unless
they're prepared to at least outline the solution on request or
otherwise contribute.  So see the attached patch (which I will concede
could probably do with more comments, it's a quick hack intended for
illustration) and tell me what you think is missing that would make it a
complicated problem.

 Peter> I tended to think that the solution was to force a heaptuple
 Peter> sort instead (where abbreviation naturally can be used),

This seems completely wrong - why should the caller have to worry about
this implementation detail? The caller shouldn't have to know about what
types or what circumstances might or might not benefit from
abbreviation.

-- 
Andrew (irc:RhodiumToad)


diff --git a/src/backend/utils/sort/tuplesort.c b/src/backend/utils/sort/tuplesort.c
index b6e302f..0dbb277 100644
--- a/src/backend/utils/sort/tuplesort.c
+++ b/src/backend/utils/sort/tuplesort.c
@@ -901,30 +901,34 @@ tuplesort_begin_datum(Oid datumType, Oid sortOperator, Oid sortCollation,
 	state->copytup = copytup_datum;
 	state->writetup = writetup_datum;
 	state->readtup = readtup_datum;
+	state->abbrevNext = 10;
 
 	state->datumType = datumType;
 
-	/* Prepare SortSupport data */
-	state->onlyKey = (SortSupport) palloc0(sizeof(SortSupportData));
-
-	state->onlyKey->ssup_cxt = CurrentMemoryContext;
-	state->onlyKey->ssup_collation = sortCollation;
-	state->onlyKey->ssup_nulls_first = nullsFirstFlag;
-	/*
-	 * Conversion to abbreviated representation infeasible in the Datum case.
-	 * It must be possible to subsequently fetch original datum values within
-	 * tuplesort_getdatum(), which would require special-case preservation of
-	 * original values.
-	 */
-	state->onlyKey->abbreviate = false;
-
-	PrepareSortSupportFromOrderingOp(sortOperator, state->onlyKey);
-
 	/* lookup necessary attributes of the datum type */
 	get_typlenbyval(datumType, &typlen, &typbyval);
 	state->datumTypeLen = typlen;
 	state->datumTypeByVal = typbyval;
 
+	/* Prepare SortSupport data */
+	state->sortKeys = (SortSupport) palloc0(sizeof(SortSupportData));
+
+	state->sortKeys->ssup_cxt = CurrentMemoryContext;
+	state->sortKeys->ssup_collation = sortCollation;
+	state->sortKeys->ssup_nulls_first = nullsFirstFlag;
+	state->sortKeys->abbreviate = !typbyval;
+
+	PrepareSortSupportFromOrderingOp(sortOperator, state->sortKeys);
+
+	/*
+	 * The "onlyKey" optimization cannot be used with abbreviated keys, since
+	 * tie-breaker comparisons may be required.  Typically, the optimization is
+	 * only of value to pass-by-value types anyway, whereas abbreviated keys
+	 * are typically only of value to pass-by-reference types.
+	 */
+	if (!state->sortKeys->abbrev_converter)
+		state->onlyKey = state->sortKeys;
+
 	MemoryContextSwitchTo(oldcontext);
 
 	return state;
@@ -1318,10 +1322,43 @@ tuplesort_putdatum(Tuplesortstate *state, Datum val, bool isNull)
 	}
 	else
 	{
-		stup.datum1 = datumCopy(val, false, state->datumTypeLen);
+		Datum	original = datumCopy(val, false, state->datumTypeLen);
 		stup.isnull1 = false;
-		stup.tuple = DatumGetPointer(stup.datum1);
+		stup.tuple = DatumGetPointer(original);
 		USEMEM(state, GetMemoryChunkSpace(stup.tuple));
+
+		if (!state->sortKeys->abbrev_converter)
+		{
+			stup.datum1 = original;
+		}
+		else if (!consider_abort_common(state))
+		{
+			/* Store abbreviated key representation */
+			stup.datum1 = state->sortKeys->abbrev_converter(original,
+			state->sortKeys);
+		}
+		else
+		{
+			/* Abort abbreviation */
+			int		i;
+
+			stup.datum1 = original;
+
+			/*
+			 * Set state to be consistent with never trying abbreviation.
+			 *
+			 * Alter datum1 representation in already-copied tuples, so as to
+			 * ensure a consistent representation (current tuple was just handled).
+			 * Note that we rely on all tuples copied so far actually being
+			 * contained within memtuples array.
+			 */
+			for (i = 0; i < state->memtupcount; i++)
+			{
+SortTuple *mtup = &state->memtuples[i];
+
+mtup->datum1 = PointerGetDatum(mtup->tuple);
+			}
+		}
 	}
 
 	puttuple_common(state, &stup);
@@ -1887,9 +1924,9 @@ tuplesort_getdatum(Tuplesortstate *state, bool forward,
 	else
 	{
 		if (should_free)
-			*val = stup.datum1;
+			*val = PointerGetDatum(stup.tuple);
 		else
-			*val = datumCopy(stup.datum1, false, state->datumTypeLen);
+			*val = datumCopy(PointerGetDatum(stup.tuple), false, state->datumTypeLen);
 		*isNull = false;
 	}
 
@@ -3712,9 +3749,22 @@ readtup_index(Tuplesortstate *state, SortTuple *stup,
 static int
 comparetup_datum(const SortTuple *a, const SortTuple *b, Tuplesortstate *state)
 {
-	return ApplySortComparator(a->datum1, a->isnull1,
-			   b->datum1, b->isnu

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Wed, Jan 21, 2015 at 4:31 PM, Amit Langote 
wrote:
> On Wednesday, January 21, 2015, Amit Kapila 
wrote:
>>
>>
>> Does it happen only when parallel_seqscan_degree > max_worker_processes?
>
>
>  I have max_worker_processes set to the default of 8 while
parallel_seqscan_degree is 4. So, this may be a case different from Thom's.
>

I think this is due to reason that memory for forming
tuple in master backend is retained for longer time which
is causing this statement to take much longer time than
required.  I have fixed the other issue as well reported by
you in attached patch.

I think this patch is still not completely ready for general
purpose testing, however it could be helpful if we can run
some tests to see in what kind of scenario's it gives benefit
like in the test you are doing if rather than increasing
seq_page_cost, you should add an expensive WHERE condition
so that it should automatically select parallel plan. I think it is better
to change one of the new parameter's (parallel_setup_cost,
parallel_startup_cost and cpu_tuple_comm_cost) if you want
your statement to use parallel plan, like in your example if
you would have reduced cpu_tuple_comm_cost, it would have
selected parallel plan, that way we can get some feedback about
what should be the appropriate default values for the newly added
parameters.  I am already planing to do some tests in that regard,
however if I get some feedback from other's that would be helpful.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


parallel_seqscan_v5.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] Re: [COMMITTERS] pgsql: Disable -faggressive-loop-optimizations in gcc 4.8+ for pre-9.2

[Alvaro Herrera]

Bernd Helmle wrote:
> 
> 
> --On 20. Januar 2015 17:15:01 +0100 Andres Freund 
> wrote:
> 
> >I personally think that being able to at least compile/make check old
> >versions a bit longer is a good idea.
> 
> +1 from me for this idea.

Already done yesterday :-)

Thanks,

-- 
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL

[Stephen Frost]

* Abhijit Menon-Sen (a...@2ndquadrant.com) wrote:
> At 2015-01-20 21:47:02 -0500, sfr...@snowman.net wrote:
> > Review the opening of this email though and consider that we could
> > look at "what privileges has the audit role granted to the current
> > role?" as defining what is to be audited.
> 
> Right, I understand now how that would work. I'll try to find time to
> (a) implement this, (b) remove the backwards-compatibility code, and
> (c) split up the USE_DEPARSE_FUNCTIONS stuff.

Great!  Thanks!

> > > For example, what if I want to see all the tables created and
> > > dropped by a particular user?
> > 
> > I hadn't been intending to address that with this scheme, but I think
> > we have that by looking for privilege grants where the audit role is
> > the grantee and the role-to-be-audited the grantor.
> 
> For CREATE, yes, with a bit of extra ACL-checking code in the utility
> hook; but I don't think we'll get very far without the ability to log
> ALTER/DROP too. :-) So there has to be some alternative mechanism for
> that, and I'm hoping Robert (or anyone!) has something in mind.

ALTER/DROP can be logged based on the USAGE privilege for the schema.
We can't differentiate those cases but we can at least handle them as a
group.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Parallel Seq Scan

[Amit Langote]

On Wednesday, January 21, 2015, Amit Kapila  wrote:

> On Wed, Jan 21, 2015 at 12:47 PM, Amit Langote <
> langote_amit...@lab.ntt.co.jp
> > wrote:
> >
> > On 20-01-2015 PM 11:29, Amit Kapila wrote:
> > > Note - I have yet to handle the new node types introduced at some
> > > of the places and need to verify prepared queries and some other
> > > things, however I think it will be good if I can get some feedback
> > > at current stage.
> > >
> >
> > I got an assertion failure:
> >
> > In src/backend/executor/execTuples.c: ExecStoreTuple()
> >
> > /* passing shouldFree=true for a tuple on a disk page is not sane */
> > Assert(BufferIsValid(buffer) ? (!shouldFree) : true);
> >
>
> Good Catch!
> The reason is that while master backend is scanning from a heap
> page, if it finds another tuple/tuples's from shared memory message
> queue it will process those tuples first and in such a scenario, the scan
> descriptor will still have reference to buffer which it is using from
> scanning
> from heap.  Your proposed fix will work.
>
> > After fixing this, the assertion failure seems to be gone though I
> > observed the blocked (CPU maxed out) state as reported elsewhere by Thom
> > Brown.
> >
>
> Does it happen only when parallel_seqscan_degree > max_worker_processes?
>

 I have max_worker_processes set to the default of 8 while
parallel_seqscan_degree is 4. So, this may be a case different from Thom's.

Thanks,
Amit

Re: [HACKERS] Parallel Seq Scan

[Amit Kapila]

On Wed, Jan 21, 2015 at 12:47 PM, Amit Langote <
langote_amit...@lab.ntt.co.jp> wrote:
>
> On 20-01-2015 PM 11:29, Amit Kapila wrote:
> > Note - I have yet to handle the new node types introduced at some
> > of the places and need to verify prepared queries and some other
> > things, however I think it will be good if I can get some feedback
> > at current stage.
> >
>
> I got an assertion failure:
>
> In src/backend/executor/execTuples.c: ExecStoreTuple()
>
> /* passing shouldFree=true for a tuple on a disk page is not sane */
> Assert(BufferIsValid(buffer) ? (!shouldFree) : true);
>

Good Catch!
The reason is that while master backend is scanning from a heap
page, if it finds another tuple/tuples's from shared memory message
queue it will process those tuples first and in such a scenario, the scan
descriptor will still have reference to buffer which it is using from
scanning
from heap.  Your proposed fix will work.

> After fixing this, the assertion failure seems to be gone though I
> observed the blocked (CPU maxed out) state as reported elsewhere by Thom
> Brown.
>

Does it happen only when parallel_seqscan_degree > max_worker_processes?


Thanks for checking the patch.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Partitioning: issues/ideas (Was: Re: [HACKERS] On partitioning)

[Amit Langote]

On 21-01-2015 AM 01:42, Robert Haas wrote:
> On Mon, Jan 19, 2015 at 8:48 PM, Amit Langote
>  wrote:
 Specifically, do we regard a partitions as pg_inherits children of its
 partitioning parent?
>>>
>>> I don't think this is totally an all-or-nothing decision.  I think
>>> everyone is agreed that we need to not break things that work today --
>>> e.g. Merge Append.  What that implies for pg_inherits isn't altogether
>>> clear.
>>
>> One point is that an implementation may end up establishing the
>> parent-partition hierarchy somewhere other than (or in addition to)
>> pg_inherits. One intention would be to avoid tying partitioning scheme
>> to certain inheritance features that use pg_inherits. For example,
>> consider call sites of find_all_inheritors(). One notable example is
>> Append/MergeAppend which would be of interest to partitioning. We would
>> want to reuse that part of the infrastructure but we could might as well
>> write an equivalent, say find_all_partitions() which scans something
>> other than pg_inherits to get all partitions.
> 
> IMHO, there's little reason to avoid putting pg_inherits entries in
> for the partitions, and then this just works.  We can find other ways
> to make it work if that turns out to be better, but if we don't have
> one, there's no reason to complicate things.
> 

Ok, I will go forward and stick to pg_inherits approach for now. Perhaps
the concerns I am expressing have other solutions that don't require
abandoning pg_inherits approach altogether.

>> Agree that some concrete idea of internal representation should help
>> guide the catalog structure. If we are going to cache the partitioning
>> info in relcache (which we most definitely will), then we should try to
>> make sure to consider the scenario of having a lot of partitioned tables
>> with a lot of individual partitions. It looks like it would be similar
>> to a scenarios where there are a lot of inheritance hierarchies. But,
>> availability of partitioning feature would definitely cause these
>> numbers to grow larger. Perhaps this is an important point driving this
>> discussion.
> 
> Yeah, it would be good if the costs of supporting, say, 1000
> partitions were negligible.
> 
>> A primary question for me about partition-pruning is when do we do it?
>> Should we model it after relation_excluded_by_constraints() and hence
>> totally plan-time? But, the tone of the discussion is that we postpone
>> partition-pruning to execution-time and hence my perhaps misdirected
>> attempts to inject it into some executor machinery.
> 
> It's useful to prune partitions at plan time, because then you only
> have to do the work once.  But sometimes you don't know enough to do
> it at plan time, so it's useful to do it at execution time, too.
> Then, you can do it differently for every tuple based on the actual
> value you have.  There's no point in doing 999 unnecessary relation
> scans if we can tell which partition the actual run-time value must be
> in.  But I think execution-time pruning can be a follow-on patch.  If
> you don't restrict the scope of the first patch as much as possible,
> you're not going to have much luck getting this committed.
> 

Ok, I will limit myself to focusing on following things at the moment:

* Provide syntax in CREATE TABLE to declare partition key
* Provide syntax in CREATE TABLE to declare a table as partition of a
partitioned table and values it contains
* Arrange to have partition key and values stored in appropriate
catalogs (existing or new)
* Arrange to cache partitioning info of partitioned tables in relcache

Thanks,
Amit



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Re: [COMMITTERS] pgsql: Disable -faggressive-loop-optimizations in gcc 4.8+ for pre-9.2

[Bernd Helmle]

--On 20. Januar 2015 17:15:01 +0100 Andres Freund  
wrote:



I personally think that being able to at least compile/make check old
versions a bit longer is a good idea.


+1 from me for this idea.

--
Thanks

Bernd


--
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] Dereferenced pointers checked as NULL in btree_utils_var.c

[Heikki Linnakangas]

On 01/21/2015 07:14 AM, Michael Paquier wrote:

Also,
looking at the code of gist, gbt_var_same is called through
gistKeyIsEQ, which is used for an insertion or for a split. The point
is that when doing an insertion, a call to gistgetadjusted is done and
we look if one of the keys is NULL. If one of them is, code does not
go through gistKeyIsEQ, so the NULL checks do not seem necessary in
gbt_var_same.


I think you are confusing NULL pointers with an SQL NULL. 
gistgetadjusted checks that it's not an SQL NULL (!oldisnull[i]), but it 
does not check if it's a NULL pointer 
(DatumGetPointer(oldentries[i].key) != NULL). The case we're worried 
about is that the value is not an SQL NULL, i.e. isnull flag is false, 
but the Datum is a NULL pointer.


Nevertheless, looking at the code, I don't that a NULL pointer can ever 
be passed to the same-method, for any of the built-in or contrib 
opclasses, but it's very confusing because some functions check for 
that. My proof goes like this:


1. The key value passed as argument must've been originally formed by 
the compress, union, or picksplit methods.


1.1. Some compress methods do nothing, and just return the passed-in 
key, which comes from the table and cannot be a NULL pointer (the 
compress method is never called for SQL NULLs). Other compress methods 
don't check for a NULL pointer, and would crash if there was one. 
gist_poly_compress() and gist_circle_compress() do check for a NULL, but 
they only return a NULL key if the input key is NULL, which cannot 
happen because the input comes from a table. So I believe the 
NULL-checks in those functions are dead code, and none of the compress 
methods ever return a NULL key.


1.2. None of the union methods return a NULL pointer (nor expect one as 
input).


1.3. None of the picksplit methods return a NULL pointer. They all 
return one of the original values, or one formed with the union method. 
The picksplit method can return a NULL pointer in the spl_ldatum or 
spl_rdatum field, in the degenerate case that it puts all keys on the 
same halve. However, the caller, gistUserPickSplit checks for that and 
immediately overwrites spl_ldatum and spl_rdatum with sane values in 
that case.



I don't understand what this sentence in the documentation on the 
compress method means:



Depending on your needs, you could also need to care about
compressing NULL values in there, storing for example (Datum) 0 like
gist_circle_compress does.


The compress method is never called for NULLs, so the above is nonsense. 
I think it should be removed, as well as the checks in 
gist_circle_compress and gist_poly_compress. According to git history, 
the check in gist_circle_compress been there ever since the module was 
imported into contrib/rtree_gist, in 2001. The documentation was added 
later:


commit a0a3883dd977d6618899ccd14258a0696912a9d2
Author: Tom Lane 
Date:   Fri Jun 12 19:48:53 2009 +

Improve documentation about GiST opclass support functions.
Dimitri Fontaine

I'm guessing that Tom added that sentence (it was not in the patch that 
Dimitri submitted originally) just because there was that check in the 
existing function, without realizing that the check was in fact dead code.


- 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] B-Tree support function number 3 (strxfrm() optimization)

[Andrew Gierth]

> "Peter" == Peter Geoghegan  writes:

 Peter> You'll probably prefer the attached. This patch works by
 Peter> disabling abbreviation, but only after writing out runs, with
 Peter> the final merge left to go. That way, it doesn't matter when
 Peter> abbreviated keys are not read back from disk (or regenerated).

This seems tolerable to me for a quick fix. The merits of storing the
abbreviation vs. re-abbreviating on input can be studied later.

 Peter> I believe this bug was missed because it only occurs when there
 Peter> are multiple runs, and not in the common case where there is one
 Peter> big initial run that is found already sorted when we reach
 Peter> mergeruns().

Ah, yes, there is an optimization for the one-run case which bypasses
all further comparisons, hiding the problem.

-- 
Andrew (irc:RhodiumToad)


-- 
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] Error check always bypassed in tablefunc.c

[Michael Paquier]

On Tue, Jan 20, 2015 at 4:05 PM, Michael Paquier
 wrote:
> On Tue, Jan 20, 2015 at 8:47 AM, Michael Paquier
>  wrote:
>> On Mon, Jan 19, 2015 at 11:06 PM, Joe Conway  wrote:
>>> -BEGIN PGP SIGNED MESSAGE-
>>> Hash: SHA1
>>>
>>> On 01/19/2015 08:16 AM, Alvaro Herrera wrote:
 Haven't looked at this patch, but I wonder if it would be better
 to replace the innards of connectby with a rewrite of the query to
 use standard WITH queries.  Maybe we can remove a couple hundred
 lines from tablefunc.c?
>>>
>>> Seems like a good idea -- connectby is really obsolete for quite a
>>> while now other than as an SRF example. I guess we only keep it around
>>> for backwards compatibility?
>> For master, yes we could brush up things a bit. Now do we really do
>> the same for back-branches? I would think that the answer there is
>> something close to the patch I sent.
>
> So, using a WITH RECURSIVE, here is a query equivalent to what connectby does:
> [...]
> Thoughts?
Looking at this stuff, actually I do not think that it is possible for
now to support orderby_fld at the same level with WITH RECURSIVE in
connectby because we need to reorder the items of the base table
within the 2nd query of the UNION ALL to give something like that and
WITH RECURSIVE does not support ORDER BY (and mutual recursion between
WITH items).

Another thing to note is that WITH RECURSIVE weakens the infinite
recursion detection. I don't think it would be good to weaken that...
Attached is a result of this random hacking, resulting in some cleanup
btw:
 1 file changed, 110 insertions(+), 264 deletions(-)
Regards,
-- 
Michael
diff --git a/contrib/tablefunc/tablefunc.c b/contrib/tablefunc/tablefunc.c
index 3388fab..e7c3674 100644
--- a/contrib/tablefunc/tablefunc.c
+++ b/contrib/tablefunc/tablefunc.c
@@ -54,7 +54,6 @@ static Tuplestorestate *get_crosstab_tuplestore(char *sql,
 		bool randomAccess);
 static void validateConnectbyTupleDesc(TupleDesc tupdesc, bool show_branch, bool show_serial);
 static bool compatCrosstabTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
-static bool compatConnectbyTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2);
 static void get_normal_pair(float8 *x1, float8 *x2);
 static Tuplestorestate *connectby(char *relname,
 		  char *key_fld,
@@ -68,21 +67,6 @@ static Tuplestorestate *connectby(char *relname,
 		  MemoryContext per_query_ctx,
 		  bool randomAccess,
 		  AttInMetadata *attinmeta);
-static Tuplestorestate *build_tuplestore_recursively(char *key_fld,
-			 char *parent_key_fld,
-			 char *relname,
-			 char *orderby_fld,
-			 char *branch_delim,
-			 char *start_with,
-			 char *branch,
-			 int level,
-			 int *serial,
-			 int max_depth,
-			 bool show_branch,
-			 bool show_serial,
-			 MemoryContext per_query_ctx,
-			 AttInMetadata *attinmeta,
-			 Tuplestorestate *tupstore);
 
 typedef struct
 {
@@ -1161,14 +1145,16 @@ connectby(char *relname,
 	Tuplestorestate *tupstore = NULL;
 	int			ret;
 	MemoryContext oldcontext;
-
-	int			serial = 1;
+	StringInfoData inner_sql, outer_sql, orderby_sql;
+	char	  **values;
+	int			num_cols;
 
 	/* Connect to SPI manager */
 	if ((ret = SPI_connect()) < 0)
 		/* internal error */
 		elog(ERROR, "connectby: SPI_connect returned %d", ret);
 
+
 	/* switch to longer term context to create the tuple store */
 	oldcontext = MemoryContextSwitchTo(per_query_ctx);
 
@@ -1177,244 +1163,137 @@ connectby(char *relname,
 
 	MemoryContextSwitchTo(oldcontext);
 
-	/* now go get the whole tree */
-	tupstore = build_tuplestore_recursively(key_fld,
-			parent_key_fld,
-			relname,
-			orderby_fld,
-			branch_delim,
-			start_with,
-			start_with, /* current_branch */
-			0,	/* initial level is 0 */
-			&serial,	/* initial serial is 1 */
-			max_depth,
-			show_branch,
-			show_serial,
-			per_query_ctx,
-			attinmeta,
-			tupstore);
-
-	SPI_finish();
-
-	return tupstore;
-}
-
-static Tuplestorestate *
-build_tuplestore_recursively(char *key_fld,
-			 char *parent_key_fld,
-			 char *relname,
-			 char *orderby_fld,
-			 char *branch_delim,
-			 char *start_with,
-			 char *branch,
-			 int level,
-			 int *serial,
-			 int max_depth,
-			 bool show_branch,
-			 bool show_serial,
-			 MemoryContext per_query_ctx,
-			 AttInMetadata *attinmeta,
-			 Tuplestorestate *tupstore)
-{
-	TupleDesc	tupdesc = attinmeta->tupdesc;
-	int			ret;
-	int			proc;
-	int			serial_column;
-	StringInfoData sql;
-	char	  **values;
-	char	   *current_key;
-	char	   *current_key_parent;
-	char		current_level[INT32_STRLEN];
-	char		serial_str[INT32_STRLEN];
-	char	   *current_branch;
-	HeapTuple	tuple;
-
-	if (max_depth > 0 && level > max_depth)
-		return tupstore;
-
-	initStringInfo(&sql);
-
-	/* Build initial sql statement */
-	if (!show_serial)
-	{
-		appendS

Re: [HACKERS] pgaudit - an auditing extension for PostgreSQL

[Abhijit Menon-Sen]

[After a discussion on IRC with Stephen…]

At 2015-01-20 21:47:02 -0500, sfr...@snowman.net wrote:
>
> Review the opening of this email though and consider that we could
> look at "what privileges has the audit role granted to the current
> role?" as defining what is to be audited.

Right, I understand now how that would work. I'll try to find time to
(a) implement this, (b) remove the backwards-compatibility code, and
(c) split up the USE_DEPARSE_FUNCTIONS stuff.

> > For example, what if I want to see all the tables created and
> > dropped by a particular user?
> 
> I hadn't been intending to address that with this scheme, but I think
> we have that by looking for privilege grants where the audit role is
> the grantee and the role-to-be-audited the grantor.

For CREATE, yes, with a bit of extra ACL-checking code in the utility
hook; but I don't think we'll get very far without the ability to log
ALTER/DROP too. :-) So there has to be some alternative mechanism for
that, and I'm hoping Robert (or anyone!) has something in mind.

> Well, I was primairly digging for someone to say "yes, the object
> access stuff will be reworked to be based on event triggers, thus
> removing the need for the object access bits".

(This doesn't entirely make sense to me, but it's tangential anyway, so
I won't comment on it for now.)

-- Abhijit


-- 
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: decreasing memory needlessly consumed by array_agg

[Jeff Davis]

On Tue, 2015-01-20 at 23:37 +0100, Tomas Vondra wrote:
> Tom's message where he points that out is here:
> http://www.postgresql.org/message-id/20707.1396372...@sss.pgh.pa.us

That message also says:

"I think a patch that stood a chance of getting committed would need to
detect whether the aggregate was being called in simple or grouped
contexts, and apply different behaviors in the two cases."

I take that as an objection to any patch which does not distinguish
between the grouped and ungrouped aggregate cases, which includes your
patch.

I don't agree with that objection (or perhaps I don't understand it);
but given the strong words above, I need to get some kind of response
before I can consider committing your patch.

> I generally agree that having two API 'facets' with different behavior
> is slightly awkward and assymetric, but I wouldn't call that ugly.

Right, your words are more precise (and polite). My apologies.

> I
> actually modified both APIs initially, but I think Ali is right that not
> breaking the existing API (and keeping the original behavior in that
> case) is better. We can break it any time we want in the future, but
> it's impossible to "unbreak it" ;-)

We can't break the old API, and I'm not suggesting that we do. I was
hoping to find some alternative.

Regards,
Jeff Davis




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers