Re: 2018-03 Commitfest Summary (Andres #1)

[Pavel Stehule]

2018-03-04 8:29 GMT+01:00 Craig Ringer :

> On 4 March 2018 at 14:58, Pavel Stehule  wrote:
>>
>>
>> 2018-03-04 3:09 GMT+01:00 Craig Ringer :
>>
>>> On 3 March 2018 at 17:56, Fabien COELHO  wrote:
>>>

 The (trivial) big picture is to allow client-side expressions in psql
 (which as a \if:-) by reusing pgbench expression engine, so that one could
 write things like

   \let i :j + 12 * :k

 or

   \if :VERSION_NUM < 14


>>> I still haven't really grasped why this isn't done by embedding a
>>> client-side scripting language interpreter, giving us vastly greater
>>> capabilities with only the maintenance of the glue instead of writing our
>>> own ad-hoc scripting tool. Something confine-able like JavaScript or Lua.
>>>
>>
>> I am primary a psql user, so I'll talk about psql. I don't need there
>> more functionality, than has C macros. Not more. So \if :VERSION_NUM < x is
>> good enough. && || operators are nice to have
>>
>> For this I don't to join any VM. It is overkill. What scripting
>> functionality we can do in psql now, and probably in long future
>>
>> a) setting prompt
>> b) some deployment - version checks
>> c) implementation of some simple regress scenarios
>>
>> Can be different if we start new rich TUI client based on ncurses, new
>> features - but it is different story.
>>
>> More - implementation of simple expression evaluation in psql doesn't
>> break later integration of some VM. Maybe it prepare a way for it.
>>
>> I can imagine the psql command \lua anylua expression, with possibility
>> to call any lua function with possibility to iterate over SQL result, show
>> any result, and read, write from psql variables - prefer Lua against
>> JavaScript, but same can be done there too.
>>
>> But this not against to this patch. This patch is not too big, too
>> complex, too hard to maintain - and some simple today issues can be done
>> simple
>>
>>
> Fine by me so long as it remains a manageable scope, rather than
> incrementally turning into some horror scripting language.
>

It is Postgres development limited by commitfest cycles - step by step. I
can imagine to grow this scripting possibilities - but programming any
non-trivial task in \xxx commands is everything, but not nice and friendly.
This is natural and practical limit. So the scope is limited to expression
evaluation. Nothing more. Maybe, if there will be a agreement and spirit,
we can talk about some new concepts for psql, pgbench to allow some more
customization or some modernization. Personally, I am almost happy with
psql, when I wrote pspg :). We can get some inspiration from pgcli
https://github.com/dbcli/pgcli. These people does good work - but due
python it is slower in visualisation. After this commitfest I'll start
discussion, what is missing in psql.

Maybe integration of some VM can reduce lot of code inside psql and can
help with better autocomplete implementation - but it is pretty long story,
and it means direct dependecy on selected VM.

Regards

Pavel

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

>

Re: 2018-03 Commitfest Summary (Andres #1)

[Michael Paquier]

On Sat, Mar 03, 2018 at 05:13:25PM -0800, Andres Freund wrote:
> Reviewing whether the implementation is good enough *does* use
> resources.  Our scarcest resource isn't patch contributions, it's
> dealing with review and maintenance.

This is true.  One thing that patch authors tend to easily forget is
that a patch merged into the development branch means in no way that the
work is done, it is only the beginning.  Even if it is the committer's
responsibility to maintain a feature because by committing he accepts to
take the maintenance load, the author should also help with things as
the person who knows the pushed code as much as the committer himself.
The more patches pushed, the more maintenance load.  Careful peer review
is critical in being able to measure if a feature is going to cost much
in maintenance or not in the years following its commit.

> A lot of contributors, including serial ones, don't even remotely put in
> as much resources reviewing other people's patches as they use up in
> reviewer and committer bandwidth.  You certainly have contributed more
> patches than you've reviewed for example.  That fundamentally can't
> scale, unless some individual contribute way more review resources than
> they use up, and that's not something many people afford nor want.

This problem has existed for years, and has existed since I managed my
first commit fest.  In my opinion, it is easier to give value in a
company to new and shiny features than to bug tasks, so people tend to
give priority to features over maintenance, because they give more value
to their own career as new features are mainly seen as individual
achievements, while maintenance is a collective achievement, as this
involves most of the time fixing a problem that somebody else
introduced.  That's sad I think, maintenance should be given more value
as this is in the area of teamwork-related metrics.
--
Michael


signature.asc
Description: PGP signature

Re: 2018-03 Commitfest Summary (Andres #1)

[Craig Ringer]

On 4 March 2018 at 14:58, Pavel Stehule  wrote:
>
>
> 2018-03-04 3:09 GMT+01:00 Craig Ringer :
>
>> On 3 March 2018 at 17:56, Fabien COELHO  wrote:
>>
>>>
>>> The (trivial) big picture is to allow client-side expressions in psql
>>> (which as a \if:-) by reusing pgbench expression engine, so that one could
>>> write things like
>>>
>>>   \let i :j + 12 * :k
>>>
>>> or
>>>
>>>   \if :VERSION_NUM < 14
>>>
>>>
>> I still haven't really grasped why this isn't done by embedding a
>> client-side scripting language interpreter, giving us vastly greater
>> capabilities with only the maintenance of the glue instead of writing our
>> own ad-hoc scripting tool. Something confine-able like JavaScript or Lua.
>>
>
> I am primary a psql user, so I'll talk about psql. I don't need there more
> functionality, than has C macros. Not more. So \if :VERSION_NUM < x is good
> enough. && || operators are nice to have
>
> For this I don't to join any VM. It is overkill. What scripting
> functionality we can do in psql now, and probably in long future
>
> a) setting prompt
> b) some deployment - version checks
> c) implementation of some simple regress scenarios
>
> Can be different if we start new rich TUI client based on ncurses, new
> features - but it is different story.
>
> More - implementation of simple expression evaluation in psql doesn't
> break later integration of some VM. Maybe it prepare a way for it.
>
> I can imagine the psql command \lua anylua expression, with possibility to
> call any lua function with possibility to iterate over SQL result, show any
> result, and read, write from psql variables - prefer Lua against
> JavaScript, but same can be done there too.
>
> But this not against to this patch. This patch is not too big, too
> complex, too hard to maintain - and some simple today issues can be done
> simple
>
>
Fine by me so long as it remains a manageable scope, rather than
incrementally turning into some horror scripting language.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: 2018-03 Commitfest Summary (Andres #1)

[Pavel Stehule]

2018-03-04 3:09 GMT+01:00 Craig Ringer :

> On 3 March 2018 at 17:56, Fabien COELHO  wrote:
>
>>
>> The (trivial) big picture is to allow client-side expressions in psql
>> (which as a \if:-) by reusing pgbench expression engine, so that one could
>> write things like
>>
>>   \let i :j + 12 * :k
>>
>> or
>>
>>   \if :VERSION_NUM < 14
>>
>>
> I still haven't really grasped why this isn't done by embedding a
> client-side scripting language interpreter, giving us vastly greater
> capabilities with only the maintenance of the glue instead of writing our
> own ad-hoc scripting tool. Something confine-able like JavaScript or Lua.
>

I am primary a psql user, so I'll talk about psql. I don't need there more
functionality, than has C macros. Not more. So \if :VERSION_NUM < x is good
enough. && || operators are nice to have

For this I don't to join any VM. It is overkill. What scripting
functionality we can do in psql now, and probably in long future

a) setting prompt
b) some deployment - version checks
c) implementation of some simple regress scenarios

Can be different if we start new rich TUI client based on ncurses, new
features - but it is different story.

More - implementation of simple expression evaluation in psql doesn't break
later integration of some VM. Maybe it prepare a way for it.

I can imagine the psql command \lua anylua expression, with possibility to
call any lua function with possibility to iterate over SQL result, show any
result, and read, write from psql variables - prefer Lua against
JavaScript, but same can be done there too.

But this not against to this patch. This patch is not too big, too complex,
too hard to maintain - and some simple today issues can be done simple

Regards

Pavel



>
> --
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>

Re: [HACKERS] Removing LEFT JOINs in more cases

[David Rowley]

On 10 January 2018 at 08:44, Tom Lane  wrote:
> select distinct nextval('foo') from a left join b ...
>
> The presence of the DISTINCT again doesn't excuse changing how often
> nextval() gets called.
>
> I kinda doubt this list of counterexamples is exhaustive, either;
> it's just what occurred to me in five or ten minutes thought.
> So maybe you can make this idea work, but you need to think much
> harder about what the counterexamples are.

While working on the cases where the join removal should be disallowed
I discovered that the existing code is not too careful about this
either:

drop table if exists t1;

create table t1 (a int);
insert into t1 values(1);

create or replace function notice(pn int) returns int as $$
begin
raise notice '%', pn;
return pn;
end;
$$ volatile language plpgsql;

create unique index t1_a_uidx on t1(a);

explain (costs off, analyze, timing off, summary off)
select t1.a from t1 left join t1 t2 on t1.a = t2.a and notice(t2.a) = t1.a;
   QUERY PLAN

 Seq Scan on t1 (actual rows=1 loops=1)
(1 row)

drop index t1_a_uidx; -- drop the index to disallow left join removal.

explain (costs off, analyze, timing off, summary off)
select t1.a from t1 left join t1 t2 on t1.a = t2.a and notice(t2.a) = t1.a;
NOTICE:  1
QUERY PLAN
--
 Nested Loop Left Join (actual rows=1 loops=1)
   Join Filter: ((t1.a = t2.a) AND (notice(t2.a) = t1.a))
   ->  Seq Scan on t1 (actual rows=1 loops=1)
   ->  Seq Scan on t1 t2 (actual rows=1 loops=1)
(4 rows)

Should this be fixed? or is this case somehow not worth worrying about?

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: GSOC 2018 ideas

[Charles Cui]

Hi Aleksander,

   Went through the documents listed by you, and they are helpful!
It seems the main purpose of extension pg_protobuf is to parse
a protobuf struct and return the decoded field. May I ask how these kinds
of extensions are used in postgreSQL (or in other words, the scenarios to
use these plugins)?


Thanks Charles!

2018-03-02 21:11 GMT-08:00 Charles Cui :

> Got it, Aleksander! Will study these documents carefully!
>
> 2018-02-26 4:21 GMT-08:00 Aleksander Alekseev :
>
>> Hello Charles,
>>
>> > I saw PostgreSQL is selected in GSOC 2018 and pretty interested in the
>> > ideas of thrift data types support that proposed by you. So, I want to
>> > prepare for a proposal based on this idea.
>>
>> Glad you are interested in this project!
>>
>> > Can I have more detailed information of what documents or code that I
>> > need to understand?
>>
>> I would recommend the following documents and code:
>>
>> * Source code of pg_protobuf
>>   https://github.com/afiskon/pg_protobuf
>> * "Writing Postgres Extensions" tutorial series by Manuel Kniep
>>   http://big-elephants.com/2015-10/writing-postgres-extensions-part-i/
>> * "So you want to make an extension?" talk by Keith Fiske
>>   http://slides.keithf4.com/extension_dev/#/
>> * Apache Thrift official website
>>   https://thrift.apache.org/
>> * Also a great explanation of the Thrift format can be found in the
>>   book "Designing Data-Intensive Applications" by Martin Kleppmann
>>   http://dataintensive.net/
>>
>> > Also, if this idea is allocated to other student (or in other worlds,
>> > you prefer some student to work on it), do let me know, so that I can
>> > pick some other project in PostgreSQL. Any comments or suggestions are
>> > welcomed!
>>
>> To my best knowledge currently there are no other students interested in
>> this particular work.
>>
>> --
>> Best regards,
>> Aleksander Alekseev
>>
>
>

Re: select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?

[Thomas Munro]

On Sun, Mar 4, 2018 at 4:37 PM, Thomas Munro
 wrote:
> Could it be that a concurrency bug causes tuples to be lost on the
> tuple queue, and also sometimes causes X (terminate) messages to be
> lost from the error queue, so that the worker appears to go away
> unexpectedly?

Could shm_mq_detach_internal() need a pg_write_barrier() before it
writes mq_detached = true, to make sure that anyone who observes that
can also see the most recent increase of mq_bytes_written?

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Rewriting the test of pg_upgrade as a TAP test - take two

[Andrew Dunstan]

On Sun, Mar 4, 2018 at 12:42 AM, Peter Eisentraut
 wrote:
> On 1/26/18 03:00, Michael Paquier wrote:
>> As promised on a recent thread, here is a second tentative to switch
>> pg_upgrade's test.sh into a TAP infrastructure.
>
> AFAICT, this still has the same problem as the previous take, namely
> that adding a TAP test suite to the pg_upgrade subdirectory will end up
> with the build farm client running the pg_upgrade tests twice.  What we
> likely need here is an update to the build farm client in conjunction
> with this.
>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Something like this should do the trick.

diff --git a/PGBuild/Modules/TestUpgrade.pm b/PGBuild/Modules/TestUpgrade.pm
index 8406d27..05859f9 100644
--- a/PGBuild/Modules/TestUpgrade.pm
+++ b/PGBuild/Modules/TestUpgrade.pm
@@ -50,6 +50,11 @@ sub setup
 "overly long build root $buildroot will cause upgrade problems - try
something shorter than 46 chars"
   if (length($buildroot) > 46);

+   # don't run module if builtin test is enabled.
+my $using_tap_tests = $conf->using_msvc ? $conf->{config}->{tap_tests} :
+ grep {$_ eq '--enable-tap-tests'} @{$conf->{config}} ;
+   return if $using_tap_tests && -d
"$buildroot/$branch/pgsql/src/bin/pg_upgrade/t";
+
 # could even set up several of these (e.g. for different branches)
 my $self  = {
 buildroot => $buildroot,


cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?

[Thomas Munro]

On Sun, Mar 4, 2018 at 4:17 PM, Tomas Vondra
 wrote:
> On 03/04/2018 04:11 AM, Thomas Munro wrote:
>> On Sun, Mar 4, 2018 at 4:07 PM, Tomas Vondra
>>  wrote:
>>> ! ERROR:  lost connection to parallel worker
>>
>> That sounds like the new defences from 
>> 2badb5afb89cd569500ef7c3b23c7a9d11718f2f.
>
> Yeah. But I wonder why the worker fails at all, or how to find that.

Could it be that a concurrency bug causes tuples to be lost on the
tuple queue, and also sometimes causes X (terminate) messages to be
lost from the error queue, so that the worker appears to go away
unexpectedly?

-- 
Thomas Munro
http://www.enterprisedb.com

Re: select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?

[Tomas Vondra]

On 03/04/2018 04:11 AM, Thomas Munro wrote:
> On Sun, Mar 4, 2018 at 4:07 PM, Tomas Vondra
>  wrote:
>> I've started "make check" with parallel_schedule tweaked to contain many
>> select_parallel runs, and so far I've seen a couple of failures like
>> this (about 10 failures out of 1500 runs):
>>
>>   select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and
>> tenk2.thousand=0;
>> ! ERROR:  lost connection to parallel worker
>>
>> I have no idea why the worker fails (no segfaults in dmesg, nothing in
>> posgres log), or if it's related to the issue discussed here at all.
> 
> That sounds like the new defences from 
> 2badb5afb89cd569500ef7c3b23c7a9d11718f2f.
> 

Yeah. But I wonder why the worker fails at all, or how to find that.


-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?

[Thomas Munro]

On Sun, Mar 4, 2018 at 4:07 PM, Tomas Vondra
 wrote:
> I've started "make check" with parallel_schedule tweaked to contain many
> select_parallel runs, and so far I've seen a couple of failures like
> this (about 10 failures out of 1500 runs):
>
>   select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and
> tenk2.thousand=0;
> ! ERROR:  lost connection to parallel worker
>
> I have no idea why the worker fails (no segfaults in dmesg, nothing in
> posgres log), or if it's related to the issue discussed here at all.

That sounds like the new defences from 2badb5afb89cd569500ef7c3b23c7a9d11718f2f.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?

[Tomas Vondra]

On 03/04/2018 03:40 AM, Andres Freund wrote:
> 
> 
> On March 3, 2018 6:36:51 PM PST, Tomas Vondra  
> wrote:
>> On 03/04/2018 03:20 AM, Thomas Munro wrote:
>>> Hi,
>>>
>>> I saw a one-off failure like this:
>>>
>>>   QUERY PLAN
>>>  
>> --
>>>Aggregate (actual rows=1 loops=1)
>>> !->  Nested Loop (actual rows=98000 loops=1)
>>>->  Seq Scan on tenk2 (actual rows=10 loops=1)
>>>  Filter: (thousand = 0)
>>>  Rows Removed by Filter: 9990
>>> !  ->  Gather (actual rows=9800 loops=10)
>>>  Workers Planned: 4
>>>  Workers Launched: 4
>>>  ->  Parallel Seq Scan on tenk1 (actual rows=1960
>> loops=50)
>>> --- 485,495 
>>>   QUERY PLAN
>>>  
>> --
>>>Aggregate (actual rows=1 loops=1)
>>> !->  Nested Loop (actual rows=97984 loops=1)
>>>->  Seq Scan on tenk2 (actual rows=10 loops=1)
>>>  Filter: (thousand = 0)
>>>  Rows Removed by Filter: 9990
>>> !  ->  Gather (actual rows=9798 loops=10)
>>>  Workers Planned: 4
>>>  Workers Launched: 4
>>>  ->  Parallel Seq Scan on tenk1 (actual rows=1960
>> loops=50)
>>>
>>>
>>> Two tuples apparently went missing.
>>>
>>> Similar failures on the build farm:
>>>
>>>
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=okapi=2018-03-03%2020%3A15%3A01
>>>
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=locust=2018-03-03%2018%3A13%3A32
>>>
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog=2018-03-03%2017%3A55%3A11
>>>
>>> Could this be related to commit
>>> 34db06ef9a1d7f36391c64293bf1e0ce44a33915 or commit
>>> 497171d3e2aaeea3b30d710b4e368645ad07ae43?
>>>
>>
>> I think the same failure (or at least very similar plan diff) was
>> already mentioned here:
>>
>> https://www.postgresql.org/message-id/17385.1520018...@sss.pgh.pa.us
>>
>> So I guess someone else already noticed, but I don't see the cause
>> identified in that thread.
> 
> Robert and I started discussing it a bit over IM. No conclusion. Robert tried 
> to reproduce locally, including disabling atomics, without luck.
> 
> Can anybody reproduce locally?
> 

I've started "make check" with parallel_schedule tweaked to contain many
select_parallel runs, and so far I've seen a couple of failures like
this (about 10 failures out of 1500 runs):

  select count(*) from tenk1, tenk2 where tenk1.hundred > 1 and
tenk2.thousand=0;
! ERROR:  lost connection to parallel worker

I have no idea why the worker fails (no segfaults in dmesg, nothing in
posgres log), or if it's related to the issue discussed here at all.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?

[Thomas Munro]

On Sun, Mar 4, 2018 at 3:48 PM, Thomas Munro
 wrote:
> I've seen it several times on Travis CI.  (So I would normally have
> been able to tell you about this problem before the was committed,
> except that the email thread was too long and the mail archive app
> cuts long threads off!)

(Correction.  It wasn't too long.  That was something else.  In this
case the Commitfest entry had two threads registered and my bot is too
stupid to find the interesting one.)

-- 
Thomas Munro
http://www.enterprisedb.com

Re: select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?

[Thomas Munro]

On Sun, Mar 4, 2018 at 3:40 PM, Andres Freund  wrote:
> On March 3, 2018 6:36:51 PM PST, Tomas Vondra  
> wrote:
>>On 03/04/2018 03:20 AM, Thomas Munro wrote:
>>> Hi,
>>>
>>> I saw a one-off failure like this:
>>>
>>>   QUERY PLAN
>>>
>>--
>>>Aggregate (actual rows=1 loops=1)
>>> !->  Nested Loop (actual rows=98000 loops=1)
>>>->  Seq Scan on tenk2 (actual rows=10 loops=1)
>>>  Filter: (thousand = 0)
>>>  Rows Removed by Filter: 9990
>>> !  ->  Gather (actual rows=9800 loops=10)
>>>  Workers Planned: 4
>>>  Workers Launched: 4
>>>  ->  Parallel Seq Scan on tenk1 (actual rows=1960
>>loops=50)
>>> --- 485,495 
>>>   QUERY PLAN
>>>
>>--
>>>Aggregate (actual rows=1 loops=1)
>>> !->  Nested Loop (actual rows=97984 loops=1)
>>>->  Seq Scan on tenk2 (actual rows=10 loops=1)
>>>  Filter: (thousand = 0)
>>>  Rows Removed by Filter: 9990
>>> !  ->  Gather (actual rows=9798 loops=10)
>>>  Workers Planned: 4
>>>  Workers Launched: 4
>>>  ->  Parallel Seq Scan on tenk1 (actual rows=1960
>>loops=50)
>>>
>>>
>>> Two tuples apparently went missing.
>>>
>>> Similar failures on the build farm:
>>>
>>>
>>https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=okapi=2018-03-03%2020%3A15%3A01
>>>
>>https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=locust=2018-03-03%2018%3A13%3A32
>>>
>>https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog=2018-03-03%2017%3A55%3A11
>>>
>>> Could this be related to commit
>>> 34db06ef9a1d7f36391c64293bf1e0ce44a33915 or commit
>>> 497171d3e2aaeea3b30d710b4e368645ad07ae43?
>>>
>>
>>I think the same failure (or at least very similar plan diff) was
>>already mentioned here:
>>
>>https://www.postgresql.org/message-id/17385.1520018...@sss.pgh.pa.us
>>
>>So I guess someone else already noticed, but I don't see the cause
>>identified in that thread.

Oh.  Sorry, I didn't recognise that as the same thing, from the title.
Doesn't seem to be related to number of workers launched at all... it
looks more like the tuple queue is misbehaving.  Though I haven't got
any proof of anything yet.

> Robert and I started discussing it a bit over IM. No conclusion. Robert tried 
> to reproduce locally, including disabling atomics, without luck.
>
> Can anybody reproduce locally?

I've seen it several times on Travis CI.  (So I would normally have
been able to tell you about this problem before the was committed,
except that the email thread was too long and the mail archive app
cuts long threads off!)  Will try on some different kind of computers
that I have local control off...  I suspect (knowing how we run it on
Travis CI) that being way overloaded might be helpful...

-- 
Thomas Munro
http://www.enterprisedb.com

Re: select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?

[Andres Freund]

On March 3, 2018 6:36:51 PM PST, Tomas Vondra  
wrote:
>On 03/04/2018 03:20 AM, Thomas Munro wrote:
>> Hi,
>> 
>> I saw a one-off failure like this:
>> 
>>   QUERY PLAN
>>  
>--
>>Aggregate (actual rows=1 loops=1)
>> !->  Nested Loop (actual rows=98000 loops=1)
>>->  Seq Scan on tenk2 (actual rows=10 loops=1)
>>  Filter: (thousand = 0)
>>  Rows Removed by Filter: 9990
>> !  ->  Gather (actual rows=9800 loops=10)
>>  Workers Planned: 4
>>  Workers Launched: 4
>>  ->  Parallel Seq Scan on tenk1 (actual rows=1960
>loops=50)
>> --- 485,495 
>>   QUERY PLAN
>>  
>--
>>Aggregate (actual rows=1 loops=1)
>> !->  Nested Loop (actual rows=97984 loops=1)
>>->  Seq Scan on tenk2 (actual rows=10 loops=1)
>>  Filter: (thousand = 0)
>>  Rows Removed by Filter: 9990
>> !  ->  Gather (actual rows=9798 loops=10)
>>  Workers Planned: 4
>>  Workers Launched: 4
>>  ->  Parallel Seq Scan on tenk1 (actual rows=1960
>loops=50)
>> 
>> 
>> Two tuples apparently went missing.
>> 
>> Similar failures on the build farm:
>> 
>>
>https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=okapi=2018-03-03%2020%3A15%3A01
>>
>https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=locust=2018-03-03%2018%3A13%3A32
>>
>https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog=2018-03-03%2017%3A55%3A11
>> 
>> Could this be related to commit
>> 34db06ef9a1d7f36391c64293bf1e0ce44a33915 or commit
>> 497171d3e2aaeea3b30d710b4e368645ad07ae43?
>> 
>
>I think the same failure (or at least very similar plan diff) was
>already mentioned here:
>
>https://www.postgresql.org/message-id/17385.1520018...@sss.pgh.pa.us
>
>So I guess someone else already noticed, but I don't see the cause
>identified in that thread.

Robert and I started discussing it a bit over IM. No conclusion. Robert tried 
to reproduce locally, including disabling atomics, without luck.

Can anybody reproduce locally?


Andres

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

Re: select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?

[Tomas Vondra]

On 03/04/2018 03:20 AM, Thomas Munro wrote:
> Hi,
> 
> I saw a one-off failure like this:
> 
>   QUERY PLAN
>   --
>Aggregate (actual rows=1 loops=1)
> !->  Nested Loop (actual rows=98000 loops=1)
>->  Seq Scan on tenk2 (actual rows=10 loops=1)
>  Filter: (thousand = 0)
>  Rows Removed by Filter: 9990
> !  ->  Gather (actual rows=9800 loops=10)
>  Workers Planned: 4
>  Workers Launched: 4
>  ->  Parallel Seq Scan on tenk1 (actual rows=1960 loops=50)
> --- 485,495 
>   QUERY PLAN
>   --
>Aggregate (actual rows=1 loops=1)
> !->  Nested Loop (actual rows=97984 loops=1)
>->  Seq Scan on tenk2 (actual rows=10 loops=1)
>  Filter: (thousand = 0)
>  Rows Removed by Filter: 9990
> !  ->  Gather (actual rows=9798 loops=10)
>  Workers Planned: 4
>  Workers Launched: 4
>  ->  Parallel Seq Scan on tenk1 (actual rows=1960 loops=50)
> 
> 
> Two tuples apparently went missing.
> 
> Similar failures on the build farm:
> 
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=okapi=2018-03-03%2020%3A15%3A01
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=locust=2018-03-03%2018%3A13%3A32
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog=2018-03-03%2017%3A55%3A11
> 
> Could this be related to commit
> 34db06ef9a1d7f36391c64293bf1e0ce44a33915 or commit
> 497171d3e2aaeea3b30d710b4e368645ad07ae43?
> 

I think the same failure (or at least very similar plan diff) was
already mentioned here:

https://www.postgresql.org/message-id/17385.1520018...@sss.pgh.pa.us

So I guess someone else already noticed, but I don't see the cause
identified in that thread.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

select_parallel test failure: gather sometimes losing tuples (maybe during rescans)?

[Thomas Munro]

Hi,

I saw a one-off failure like this:

  QUERY PLAN
  --
   Aggregate (actual rows=1 loops=1)
!->  Nested Loop (actual rows=98000 loops=1)
   ->  Seq Scan on tenk2 (actual rows=10 loops=1)
 Filter: (thousand = 0)
 Rows Removed by Filter: 9990
!  ->  Gather (actual rows=9800 loops=10)
 Workers Planned: 4
 Workers Launched: 4
 ->  Parallel Seq Scan on tenk1 (actual rows=1960 loops=50)
--- 485,495 
  QUERY PLAN
  --
   Aggregate (actual rows=1 loops=1)
!->  Nested Loop (actual rows=97984 loops=1)
   ->  Seq Scan on tenk2 (actual rows=10 loops=1)
 Filter: (thousand = 0)
 Rows Removed by Filter: 9990
!  ->  Gather (actual rows=9798 loops=10)
 Workers Planned: 4
 Workers Launched: 4
 ->  Parallel Seq Scan on tenk1 (actual rows=1960 loops=50)


Two tuples apparently went missing.

Similar failures on the build farm:

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=okapi=2018-03-03%2020%3A15%3A01
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=locust=2018-03-03%2018%3A13%3A32
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog=2018-03-03%2017%3A55%3A11

Could this be related to commit
34db06ef9a1d7f36391c64293bf1e0ce44a33915 or commit
497171d3e2aaeea3b30d710b4e368645ad07ae43?

-- 
Thomas Munro
http://www.enterprisedb.com

Re: [HACKERS] user-defined numeric data types triggering ERROR: unsupported type

[Tomas Vondra]

On 03/04/2018 02:37 AM, Tom Lane wrote:
> Tomas Vondra  writes:
>> Here is v2 of the fix. It does handle all the convert_to_scalar calls
>> for various data types, just like the numeric one did in v1 with the
>> exception of bytea.
> 
> Pushed with some adjustments.
> 

Thanks. I see I forgot to tweak a call in btree_gist - sorry about that.

>> The bytea case is fixed by checking that the boundary values are
>> varlenas. This seems better than checking for BYTEAOID explicitly, which
>> would fail for custom varlena-based types. At first I've been thinking
>> there might be issues when the data types has mismatching ordering, but
>> I don't think the patch makes it any worse.
> 
> I didn't like this one bit.  It's unlike all the other cases, which accept
> only specific type OIDs, and there's no good reason to assume that a
> bytea-vs-something-else comparison operator would have bytea-like
> semantics.  So I think it's better to punt, pending the invention of an
> API to let the operator supply its own convert_to_scalar logic.
> 

OK, understood. That's the "mismatching ordering" I was wondering about.

>> I've also added a bunch of regression tests, checking each case. The
>> bytea test it should cause segfault on master, of course.
> 
> I was kind of underwhelmed with these test cases, too, so I didn't
> commit them.  But they were good for proving that the bytea bug
> wasn't hypothetical :-)

Underwhelmed in what sense? Should the tests be constructed in some
other way, or do you think it's something that doesn't need the tests?


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: 2018-03 Commitfest Summary (Andres #1)

[Craig Ringer]

On 3 March 2018 at 17:56, Fabien COELHO  wrote:

>
> The (trivial) big picture is to allow client-side expressions in psql
> (which as a \if:-) by reusing pgbench expression engine, so that one could
> write things like
>
>   \let i :j + 12 * :k
>
> or
>
>   \if :VERSION_NUM < 14
>
>
I still haven't really grasped why this isn't done by embedding a
client-side scripting language interpreter, giving us vastly greater
capabilities with only the maintenance of the glue instead of writing our
own ad-hoc scripting tool. Something confine-able like JavaScript or Lua.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: 2018-03 Commitfest Summary (Andres #1)

[Craig Ringer]

On 2 March 2018 at 17:47, Fabien COELHO  wrote:


>
> For instance, I used extensively tps throttling, latencies and timeouts
> measures when developping and testing the checkpointer sorting & throttling
> patch.
>

I have to admit, I've found tps throttling and latency measurement useful
when working with logical replication. It's really handy to find a stable,
sustainable throughput on master at which a replica can keep up.

PostgreSQL is about more than raw TPS. Users care about latency. Things we
change affect latency. New index tricks like batching updates; sync commit
changes for standby consistency, etc.

That's not a reason to throw anything and everything into pgbench. But
there's value to more than measuring raw tps.

Also, I'm not the one doing the work.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: 2018-03 Commitfest Summary (Andres #1)

[Craig Ringer]

On 2 March 2018 at 02:37, Tom Lane  wrote:

> Andres Freund  writes:
> > On 2018-03-01 14:09:02 +0100, Fabien COELHO wrote:
> >>> A bit concerned that we're turning pgbench into a kitchen sink.
>
> >> I do not understand "kitchen sink" expression in this context, and your
> >> general concerns about pgbench in various comments in your message.
>
> > We're adding a lot of stuff to pgbench that only a few people
> > use. There's a lot of duplication with similar parts of code in other
> > parts of the codebase. pgbench in my opinion is a tool to facilitate
> > postgres development, not a goal in itself.
>
> FWIW, I share Andres' concern that pgbench is being extended far past
> what anyone has shown a need for.  If we had infinite resources this
> wouldn't be a big problem, but it's eating into limited committer hours
> and I'm not really convinced that we're getting adequate return.


I have similar worries about us growing an ad-hoc scripting language in
psql.

-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: [HACKERS] plpgsql - additional extra checks

[Tomas Vondra]

On 03/02/2018 10:30 PM, Pavel Stehule wrote:
> Hi
> 
> 2018-03-01 21:14 GMT+01:00 David Steele  >:
> 
> Hi Pavel,
> 
> On 1/7/18 3:31 AM, Pavel Stehule wrote:
> >
> >     There, now it's in the correct Waiting for Author state. :)
> >
> > thank you for comments. All should be fixed in attached patch
> 
> This patch no longer applies (and the conflicts do not look trivial).
> Can you provide a rebased patch?
> 
> $ git apply -3 ../other/plpgsql-extra-check-180107.patch
> error: patch failed: src/pl/plpgsql/src/pl_exec.c:5944
> Falling back to three-way merge...
> Applied patch to 'src/pl/plpgsql/src/pl_exec.c' with conflicts.
> U src/pl/plpgsql/src/pl_exec.c
> 
> Marked as Waiting on Author.
> 
> 
> I am sending updated code. It reflects Tom's changes - now, the rec is
> used as row type too, so the checks must be on two places. With this
> update is related one change. When result is empty, then the extra
> checks doesn't work - PLpgSQL runtime doesn't pass necessary tupledesc.
> But usually, when result is empty, then there are not problems with
> missing values, because every value is NULL.
> 

I've looked at this patch today, and in general it seems in fairly good
shape - I don't really see any major issues in it that would mean it
can't be promoted to RFC soon.

A couple of comments though:

1) I think the docs are OK, but there are a couple of keywords that
should be wrapped in  or  tags, otherwise the
formatting will be incorrect.

I've done that in the attached patch, as it's easier than listing which
keywords/where etc. I haven't wrapped the lines, though, to make it
easier to see the difference in meld or similar tools.


2) The does does a bunch of checks of log level, in the form

if (too_many_rows_level >= WARNING)

which is perhaps a bit too verbose, because the default value of that
variable is 0. So

if (too_many_rows_level)

would be enough, and it makes the checks a bit shorter. Again, this is
done in the attached patch.


3) There is a couple of typos in the comments, like "stric_" instead of
"strict_" and so on. Again, fixed in the patch, along with slightly
rewording a bunch of comments like

/* no source for destination column */

instead of

/* there are no data */

and so on.


4) I have also reworded the text of the two checks. Firstly, I've replaced

query returned more than one row

with

SELECT INTO query returned more than one row

which I think provides additional useful context to the user.

I've also replaced

Number of evaluated fields does not match expected.

with

Number of source and target fields in assignment does not match.

because the original text seems a bit cumbersome to me. It might be
useful to also include the expected/actual number of fields, to provide
a bit more context. That's valuable particularly for WARNING messages,
which do not include information about line numbers (or even function
name). So anything that helps to locate the query (of possibly many in
that function) is valuable.


Stephen: I see you're listed as reviewer on this patch - do you see an
issue blocking this patch from getting RFC? I see you did a review in
January, but Pavel seems to have resolved the issues you identified.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 1657ec3..ebdd0be 100644
--- a/doc/src/sgml/plpgsql.sgml
+++ b/doc/src/sgml/plpgsql.sgml
@@ -5027,12 +5027,12 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$
   

 Some PL/PgSQL commands allow assigning values
-to more than one variable at a time, such as SELECT INTO.  Typically,
+to more than one variable at a time, such as SELECT INTO.  Typically,
 the number of target variables and the number of source variables should
-match, though PL/PgSQL will use NULL for
+match, though PL/PgSQL will use NULL for
 missing values and extra variables are ignored.  Enabling this check
-will cause PL/PgSQL to throw a WARNING or
-ERROR whenever the number of target variables and the number of source
+will cause PL/PgSQL to throw a WARNING or
+ERROR whenever the number of target variables and the number of source
 variables are different.

   
@@ -5044,7 +5044,7 @@ a_output := a_output || $$ if v_$$ || referrer_keys.kind || $$ like '$$

 Enabling this check will cause PL/PgSQL to
 check if a given query returns more than one row when an
-INTO clause is used.  As an INTO statement will only
+INTO clause is used.  As an INTO statement will only
 ever use one row, having a query return multiple rows is generally
 either inefficient and/or 

Re: [HACKERS] user-defined numeric data types triggering ERROR: unsupported type

[Tom Lane]

Tomas Vondra  writes:
> Here is v2 of the fix. It does handle all the convert_to_scalar calls
> for various data types, just like the numeric one did in v1 with the
> exception of bytea.

Pushed with some adjustments.

> The bytea case is fixed by checking that the boundary values are
> varlenas. This seems better than checking for BYTEAOID explicitly, which
> would fail for custom varlena-based types. At first I've been thinking
> there might be issues when the data types has mismatching ordering, but
> I don't think the patch makes it any worse.

I didn't like this one bit.  It's unlike all the other cases, which accept
only specific type OIDs, and there's no good reason to assume that a
bytea-vs-something-else comparison operator would have bytea-like
semantics.  So I think it's better to punt, pending the invention of an
API to let the operator supply its own convert_to_scalar logic.

> I've also added a bunch of regression tests, checking each case. The
> bytea test it should cause segfault on master, of course.

I was kind of underwhelmed with these test cases, too, so I didn't
commit them.  But they were good for proving that the bytea bug
wasn't hypothetical :-)

regards, tom lane

Re: 2018-03 Commitfest Summary (Andres #1)

[Andres Freund]

Hi,

On 2018-03-03 10:36:18 +0100, Fabien COELHO wrote:
> Why would committers prevent pgbench to be used to reproduce this kind of
> load?

The goal of *discussing* whether a feature is worth the cost is
obviously not to deprive users of features. I find that is a fairly
absurd, and frankly insulting, ascription of motives.


I didn't "veto" the patch or anything, nor did Tom. I wondered whether
we're adding more cost than overall gains.  We have very few people that
actually show up when there are bugs and fix them, and adding more code
tends to make maintenance harder.


> The point of pgbench is to be able to test different kind of
> loads/scenarii... Accepting such features, if the implementation is
> good enough, should be no brainer, and alas it is not.

Reviewing whether the implementation is good enough *does* use
resources.  Our scarcest resource isn't patch contributions, it's
dealing with review and maintenance.


A lot of contributors, including serial ones, don't even remotely put in
as much resources reviewing other people's patches as they use up in
reviewer and committer bandwidth.  You certainly have contributed more
patches than you've reviewed for example.  That fundamentally can't
scale, unless some individual contribute way more review resources than
they use up, and that's not something many people afford nor want.

And while possibly not universally seen that way, in my opinion, and I'm
not alone seeing things that way, contributors that contribute more
review resources than they "use", are granted more latitude in what they
want to do because they help the project scale.


Note that pgbench code does add work, even if one is not using the new
features. As you know, I was working on performance and robustness
improvements, and to make sure they are and stay correct I was
attempting to compile postgres with -fsanitize=overflow - which fails
because pgbench isn't overflow safe. I reported that, but you didn't
follow up with fixes.


> Note that I'd wish that at least the ready-for-committer bug-fixes would be
> processed by committers when tagged as ready in a timely fashion (say under
> 1 CF), which is not currently the case.

Yes, we're not fast enough to integrate fixes. That's largely because
there's few committers that are active. I fail to see how that is an
argument to integrate *more* code that needs fixes.


I entirely agree that not getting ones patches can be very
demoralizing. But unless somebody manages to find a few seasoned
developers and pays them to focus on review and maintenance, I don't see
how that is going to change.  And given scant resources, we need to
prioritize.

Greetings,

Andres Freund

Re: WIP: BRIN multi-range indexes

[Tomas Vondra]

Hi,

Attached is a patch version fixing breakage due to pg_proc changes
commited in fd1a421fe661.

On 03/02/2018 05:08 AM, Tom Lane wrote:
> Andres Freund  writes:
>> On 2018-02-25 01:30:47 +0100, Tomas Vondra wrote:
>>> Note: Currently, this only works with float8-based data types.
>>> Supporting additional data types is not a big issue, but will
>>> require extending the opclass with "subtract" operator (used to
>>> compute distance between values when merging ranges).
> 
>> Based on Tom's past stances I'm a bit doubtful he'd be happy with 
>> such a restriction. Note that something similar-ish also has come 
>> up in 0a459cec96.
> 
>> I kinda wonder if there's any way to not have two similar but not 
>> equal types of logic here?
> 

I don't think it's very similar to what 0a459cec96 is doing. It's true
both deal with ranges of values, but that's about it - I don't see how
this patch could reuse some bits from 0a459cec96.

To elaborate, 0a459cec96 only really needs to know "does this value fall
into this range" while this patch needs to compare ranges by length.
That is, given a bunch of ranges (summary of values for a section of a
table), it needs to decide which ranges to merge - and it picks the
ranges with the smallest gap.

So for example with ranges [1,10], [15,20], [30,200], [250,300] it would
merge [1,10] and [15,20] because the gap between them is only 5, which
is shorter than the other gaps. This is used when the summary for a
range of pages gets "full" (the patch only keeps up to 32 ranges or so).

Not sure how I could reuse 0a459cec96 to do this.

> Hm. I wonder what the patch intends to do with subtraction overflow, 
> or infinities, or NaNs. Just as with the RANGE patch, it does not 
> seem to me that failure is really an acceptable option. Indexes are 
> supposed to be able to index whatever the column datatype can store.
> 

I've been thinking about this after looking at 0a459cec96, and I don't
think this patch has the same issues. One reason is that just like the
original minmax opclass, it does not really mess with the data it
stores. It only does min/max on the values, and stores that, so if there
was NaN or Infinity, it will index NaN or Infinity.

The subtraction is used only to decide which ranges to merge first, and
if the subtraction returns Infinity/NaN the ranges will be considered
very distant and merged last. Which is pretty much the desired behavior,
because it means -Infinity, Infinity and NaN will be keps as individual
"points" as long as possible.

Perhaps there is some other danger/thinko here, that I don't see?


The one overflow issue I found in the patch is that the numeric
"distance" function does this:

d = DirectFunctionCall2(numeric_sub, a2, a1);   /* a2 - a1 */

PG_RETURN_FLOAT8(DirectFunctionCall1(numeric_float8, d));

which can overflow, of course. But that is not fatal - the index may get
inefficient due to non-optimal merging of ranges, but it will still
return correct results. But I think this can be easily improved by
passing not only the two values, but also minimum and maximum, and use
that to normalize the values to [0,1].


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


0001-Pass-all-keys-to-BRIN-consistent-function-at-once.patch.gz
Description: application/gzip


0002-Move-IS-NOT-NULL-checks-to-bringetbitmap.patch.gz
Description: application/gzip


0003-BRIN-bloom-indexes.patch.gz
Description: application/gzip


0004-BRIN-multi-range-minmax-indexes.patch.gz
Description: application/gzip

Re: PATCH: pgbench - option to build using ppoll() for larger connection counts

[Andres Freund]

On 2018-03-01 11:30:39 +0100, Fabien COELHO wrote:
> 
> > > -#ifdef HAVE_SYS_SELECT_H
> > > +#ifdef PGBENCH_USE_SELECT/* force use of 
> > > select(2)? */
> > > +#undef HAVE_PPOLL
> > > +#endif
> > > +#ifdef HAVE_PPOLL
> > > +#include 
> > > +#elif defined(HAVE_SYS_SELECT_H)
> > > +#define POLL_USING_SELECT
> > 
> > (random thing noticed while going through patches)
> > 
> > It strikes me as a bad idea to undefine configure selected
> > symbols. Postgres header might rely on them. It also strikes me as
> > entirely unnecessary here.
> 
> Yes, I though about this one but let it pass. Indeed, it would be sufficient
> to not load "poll.h" when select is forced, without undefining the configure
> setting.

I've marked the CF entry waiting on author.

Greetings,

Andres Freund

Re: BUG #14941: Vacuum crashes

[Andres Freund]

On 2018-01-11 08:14:42 +0900, Michael Paquier wrote:
> On Wed, Jan 10, 2018 at 05:26:43PM +, Bossart, Nathan wrote:
> > Perhaps any logging changes for VACOPT_NOWAIT could be handled in a
> > separate thread.  For now, I've updated 0003 to remove the logging
> > changes.
> 
> Thanks. I am marking those as ready for committer, you are providing the
> set patch patch which offer the most consistent experience.

I was working on committing 0002 and 0003, when I noticed that the
second patch doesn't actually fully works.  NOWAIT does what it says on
the tin iff the table is locked with a lower lock level than access
exclusive.  But if AEL is used, the command is blocked in

static List *
expand_vacuum_rel(VacuumRelation *vrel)
...
/*
 * We transiently take AccessShareLock to protect the syscache 
lookup
 * below, as well as find_all_inheritors's expectation that the 
caller
 * holds some lock on the starting relation.
 */
relid = RangeVarGetRelid(vrel->relation, AccessShareLock, 
false);

ISTM has been added after the patches initially were proposed. See
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=19de0ab23ccba12567c18640f00b49f01471018d

I'm a bit disappointed that the tests didn't catch this, they're clearly
not fully there. They definitely should test the AEL case, as
demonstrated here.


Independent of that, I'm also concerned that NOWAIT isn't implemented
consistently with other commands. Aren't we erroring out in other uses
of NOWAIT?  ISTM a more appropriate keyword would have been SKIP
LOCKED.  I think the behaviour makes sense, but I'd rename the internal
flag and the grammar to use SKIP LOCKED.

Lightly edited patches attached. Please preserve commit messages while
fixing these issues.

Greetings,

Andres Freund
>From 3a59265b8ae9837a16aed76773d638f7392a395b Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Sat, 3 Mar 2018 15:47:53 -0800
Subject: [PATCH v5 1/2] Add parenthesized options syntax for ANALYZE.

This is analogous to the syntax allowed for VACUUM. This allows us to
avoid making new options reserved keywords and makes it easier to
allow arbitrary argument order. Oh, and it's consistent with the other
commands, too.

Author: Nathan Bossart
Reviewed-By: Michael Paquier, Masahiko Sawada
Discussion: https://postgr.es/m/d3fc73e2-9b1a-4db4-8180-55f57d116...@amazon.com
---
 doc/src/sgml/ref/analyze.sgml| 14 +-
 src/backend/parser/gram.y| 17 +
 src/test/regress/expected/vacuum.out |  7 +++
 src/test/regress/sql/vacuum.sql  |  4 
 4 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/ref/analyze.sgml b/doc/src/sgml/ref/analyze.sgml
index 83b07a03003..10b3a9a6733 100644
--- a/doc/src/sgml/ref/analyze.sgml
+++ b/doc/src/sgml/ref/analyze.sgml
@@ -21,9 +21,14 @@ PostgreSQL documentation
 
  
 
+ANALYZE [ ( option [, ...] ) ] [ table_and_columns [, ...] ]
 ANALYZE [ VERBOSE ] [ table_and_columns [, ...] ]
 
-where table_and_columns is:
+where option can be one of:
+
+VERBOSE
+
+and table_and_columns is:
 
 table_name [ ( column_name [, ...] ) ]
 
@@ -49,6 +54,13 @@ ANALYZE [ VERBOSE ] [ table_and_columns
+
+  
+   When the option list is surrounded by parentheses, the options can be
+   written in any order.  The parenthesized syntax was added in
+   PostgreSQL 11;  the unparenthesized syntax
+   is deprecated.
+  
  
 
  
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index d99f2be2c97..1d7317c6eac 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -306,6 +306,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
 
 %type 	opt_lock lock_type cast_context
 %type 	vacuum_option_list vacuum_option_elem
+analyze_option_list analyze_option_elem
 %type 	opt_or_replace
 opt_grant_grant_option opt_grant_admin_option
 opt_nowait opt_if_exists opt_with_data
@@ -10548,6 +10549,22 @@ AnalyzeStmt: analyze_keyword opt_verbose opt_vacuum_relation_list
 	n->rels = $3;
 	$$ = (Node *)n;
 }
+			| analyze_keyword '(' analyze_option_list ')' opt_vacuum_relation_list
+{
+	VacuumStmt *n = makeNode(VacuumStmt);
+	n->options = VACOPT_ANALYZE | $3;
+	n->rels = $5;
+	$$ = (Node *) n;
+}
+		;
+
+analyze_option_list:
+			analyze_option_elem{ $$ = $1; }
+			| analyze_option_list ',' analyze_option_elem	{ $$ = $1 | $3; }
+		;
+
+analyze_option_elem:
+			VERBOSE{ $$ = VACOPT_VERBOSE; }
 		;
 
 analyze_keyword:
diff --git a/src/test/regress/expected/vacuum.out b/src/test/regress/expected/vacuum.out
index c440c7ea58f..d66e2aa3b70 100644
--- a/src/test/regress/expected/vacuum.out
+++ b/src/test/regress/expected/vacuum.out
@@ -112,6 +112,13 @@ ANALYZE vactst, does_not_exist, vacparted;
 ERROR:  relation "does_not_exist" does not exist
 ANALYZE vactst (i), vacparted 

Re: [HACKERS] GUC for cleanup indexes threshold.

[Alexander Korotkov]

On Fri, Mar 2, 2018 at 10:53 AM, Masahiko Sawada 
wrote:

> > 2) In the append-only case, index statistics can lag indefinitely.
>
> The original proposal proposed a new GUC that specifies a fraction of
> the modified pages to trigger a cleanup indexes.


Regarding original proposal, I didn't get what exactly it's intended to be.
You're checking if vacuumed_pages >= nblocks * vacuum_cleanup_index_scale.
But vacuumed_pages is the variable which could be incremented when
no indexes exist on the table.  When indexes are present, this variable is
always
zero.  I can assume, that it's intended to compare number of pages where
at least one tuple is deleted to nblocks * vacuum_cleanup_index_scale.
But that is also not an option for us, because we're going to optimize the
case when exactly zero tuples is deleted by vacuum.

The thing I'm going to propose is to add estimated number of tuples in
table to IndexVacuumInfo.  Then B-tree can memorize that number of tuples
when last time index was scanned in the meta-page.  If pass value
is differs from the value in meta-page too much, then cleanup is forced.

Any better ideas?


--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: perltidy version

[Tom Lane]

Magnus Hagander  writes:
> Ah yeah, if I apply that one first, the diff from using 20140328 is much
> smaller. Attached is that one, which means the difference between the two
> perltidy versions.

I'm hardly a Perl guru, so I'm not going to opine on whether these
changes are for the better or worse.  They're definitely not very
extensive, though.  If the folks here who do hack Perl a lot think
the 20140328 output is better, I'm fine with switching.

regards, tom lane

Re: [HACKERS] PATCH: multivariate histograms and MCV lists

[Tomas Vondra]

An updated patch version, fixing the breakage caused by fd1a421fe6
twiddling with pg_proc.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


0001-Keep-information-about-already-estimated-clauses.patch.gz
Description: application/gzip


0002-multivariate-MCV-lists.patch.gz
Description: application/gzip


0003-multivariate-histograms.patch.gz
Description: application/gzip

Re: Disabling src/test/[ssl|ldap] when not building with SSL/LDAP support

[Michael Paquier]

On Sat, Mar 03, 2018 at 08:58:18AM -0500, Peter Eisentraut wrote:
> On 3/2/18 03:23, Michael Paquier wrote:
>> That's a better idea.  I have reworked that in 0001.  What do you think?
>> This has the same effect as diag(), which shows information directly to
>> the screen, and that's the behavior I was looking for.
> 
> I ended up using plan skip_all, which seems to be intended for this
> purpose, according to the documentation.

Yes, that's what I got as well first but...

>> I have concluded about using the most simple name: PG_TEST_EXTRA.  A
>> documentation attempt is added as well.  This is unfortunately close to
>> EXTRA_TESTS.  It would be tempting to directly reuse EXTRA_TESTS as
>> well, however this is used only for the core regression tests now, so a
>> separated variable seems adapted to me.
> 
> Committed.  Very nice.

Thanks for the commit, Peter, PG_TEST_EXTRA is already set is my
environment.
--
Michael


signature.asc
Description: PGP signature

Re: autovacuum: change priority of the vacuumed tables

[Tomas Vondra]

On 03/03/2018 10:21 PM, Jim Nasby wrote:
> On 3/3/18 2:53 PM, Tomas Vondra wrote:
>> That largely depends on what knobs would be exposed. I'm against adding
>> some low-level knobs that perhaps 1% of the users will know how to tune,
>> and the rest will set it incorrectly. Some high-level options that would
>> specify the workload type might work, but I have no idea about details.
> 
> Not knowing about details is why we've been stuck here for years: it's
> not terribly obvious how to create a scheduler that is going to work in
> all situations. Current autovac is great for 80% of situations, but it
> simply doesn't handle the remaining 20% by itself. Once you're pushing
> your IO limits you *have* to start scheduling manual vacuums for any
> critical tables.
> 
> At least if we exposed some low level ability to control autovac
> workers then others could create tools to improve the situation.
> Currently that's not possible because manual vacuum lacks features
> that autovac has.
> 

I have my doubts about both points - usefulness of low-level controls
and viability of tools built on them.

Firstly, my hunch is that if we knew what low-level controls to expose,
it would pretty much how to implement the tool internally. Exposing
something just because you home someone will find a use for that seems
like a dead-end to me. So, which controls would you expose?

Second, all the statistics used to decide which tables need vacuuming
are already exposed, and we have things like bgworkers etc. So you could
go and write a custom autovacuum today - copy the autovacuum code, tweak
the scheduling, done. Yet no such tool emerged yet. Why is that?

>>> One fairly simple option would be to simply replace the logic
>>> that currently builds a worker's table list with running a query
>>> via SPI. That would allow for prioritizing important tables. It
>>> could also reduce the problem of workers getting "stuck" on a ton
>>> of large tables by taking into consideration the total number of
>>> pages/tuples a list contains.
>>>
>> I don't see why SPI would be needed to do that, i.e. why couldn't
>> we implement such prioritization with the current approach. Another
>> thing
> 
> Sure, it's just a SMOC. But most of the issue here is actually a
> query problem. I suspect that the current code would actually shrink
> if converted to SPI. In any case, I'm not wed to that idea.
> 

I disagree this a "query problem" - it certainly is not the case that
simply prioritizing the tables differently will make a difference. Or
more precisely, it certainly does not solve the autovacuum issues I'm
thinking about. I have no idea which issues are you trying to solve,
because you haven't really described those.

>> is I really doubt prioritizing "important tables" is an good solution,
>> as it does not really guarantee anything.
> 
> If by "important" you mean small tables with high update rates,
> prioritizing those actually would help as long as you have free workers.
> By itself it doesn't gain all that much though.
> 

Which is why I mentioned we could have separate pools of autovacuum
workers - one for regular tables, one for "important" ones.

>>> A more fine-grained approach would be to have workers make a new 
>>> selection after every vacuum they complete. That would provide
>>> the ultimate in control, since you'd be able to see exactly what
>>> all the other workers are doing.
>> That was proposed earlier in this thread, and the issue is it may
>> starve all the other tables when the "important" tables need
>> cleanup all the time.
> 
> There's plenty of other ways to shoot yourself in the foot in that 
> regard already. We can always have safeguards in place if we get too 
> close to wrap-around, just like we currently do.

I haven't mentioned wraparound at all.

My point is that if you entirely ignore some tables because "important"
ones need constant cleanup (saturating the total autovacuum capacity),
then you'll end up with extreme bloat in those other tables. And then
you will need to do more work to clean them up, which will likely cause
delays in cleaning up the important ones.


FWIW I find this discussion rather too hand-wavy, because I have no idea
what controls would you like to expose, etc. If you have an idea, please
write a patch or at least a proposal explaining the details.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: autovacuum: change priority of the vacuumed tables

[Jim Nasby]

On 3/3/18 2:53 PM, Tomas Vondra wrote:

That largely depends on what knobs would be exposed. I'm against adding
some low-level knobs that perhaps 1% of the users will know how to tune,
and the rest will set it incorrectly. Some high-level options that would
specify the workload type might work, but I have no idea about details.


Not knowing about details is why we've been stuck here for years: it's 
not terribly obvious how to create a scheduler that is going to work in 
all situations. Current autovac is great for 80% of situations, but it 
simply doesn't handle the remaining 20% by itself. Once you're pushing 
your IO limits you *have* to start scheduling manual vacuums for any 
critical tables.


At least if we exposed some low level ability to control autovac workers 
then others could create tools to improve the situation. Currently 
that's not possible because manual vacuum lacks features that autovac has.



One fairly simple option would be to simply replace the logic that
currently builds a worker's table list with running a query via SPI.
That would allow for prioritizing important tables. It could also reduce
the problem of workers getting "stuck" on a ton of large tables by
taking into consideration the total number of pages/tuples a list contains.


I don't see why SPI would be needed to do that, i.e. why couldn't we
implement such prioritization with the current approach. Another thing


Sure, it's just a SMOC. But most of the issue here is actually a query 
problem. I suspect that the current code would actually shrink if 
converted to SPI. In any case, I'm not wed to that idea.



is I really doubt prioritizing "important tables" is an good solution,
as it does not really guarantee anything.


If by "important" you mean small tables with high update rates, 
prioritizing those actually would help as long as you have free workers. 
By itself it doesn't gain all that much though.



A more fine-grained approach would be to have workers make a new
selection after every vacuum they complete. That would provide the
ultimate in control, since you'd be able to see exactly what all the
other workers are doing.

That was proposed earlier in this thread, and the issue is it may starve
all the other tables when the "important" tables need cleanup all the time.


There's plenty of other ways to shoot yourself in the foot in that 
regard already. We can always have safeguards in place if we get too 
close to wrap-around, just like we currently do.

--
Jim Nasby, Chief Data Architect, Austin TX
OpenSCG http://OpenSCG.com

Re: autovacuum: change priority of the vacuumed tables

[Tomas Vondra]

On 03/03/2018 08:32 PM, Jim Nasby wrote:
> On 2/19/18 10:00 AM, Tomas Vondra wrote:
>> So I don't think this is a very promising approach, unfortunately.
>>
>> What I think might work is having a separate pool of autovac workers,
>> dedicated to these high-priority tables. That still would not guarantee
>> the high-priority tables are vacuumed immediately, but at least that
>> they are not stuck in the worker queue behind low-priority ones.
>>
>> I wonder if we could detect tables with high update/delete activity and
>> promote them to high-priority automatically. The reasoning is that by
>> delaying the cleanup for those tables would result in significantly more
>> bloat than for those with low update/delete activity.
> 
> I've looked at this stuff in the past, and I think that the first step
> in trying to improve autovacuum needs to be allowing for a much more
> granular means of controlling worker table selection, and exposing that
> ability. There are simply too many different scenarios to try and
> account for to try and make a single policy that will satisfy everyone.
> Just as a simple example, OLTP databases (especially with small queue
> tables) have very different vacuum needs than data warehouses.
> 

That largely depends on what knobs would be exposed. I'm against adding
some low-level knobs that perhaps 1% of the users will know how to tune,
and the rest will set it incorrectly. Some high-level options that would
specify the workload type might work, but I have no idea about details.

> One fairly simple option would be to simply replace the logic that
> currently builds a worker's table list with running a query via SPI.
> That would allow for prioritizing important tables. It could also reduce
> the problem of workers getting "stuck" on a ton of large tables by
> taking into consideration the total number of pages/tuples a list contains.
> 

I don't see why SPI would be needed to do that, i.e. why couldn't we
implement such prioritization with the current approach. Another thing
is I really doubt prioritizing "important tables" is an good solution,
as it does not really guarantee anything.

> A more fine-grained approach would be to have workers make a new
> selection after every vacuum they complete. That would provide the
> ultimate in control, since you'd be able to see exactly what all the
> other workers are doing.

That was proposed earlier in this thread, and the issue is it may starve
all the other tables when the "important" tables need cleanup all the time.

regards
-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

use of getcwd(3)/chdir(2) during path resolution (exec.c)

[Petar Bogdanovic]

Hi,

When trying to deduct their absolute, symlink-free path from argv0,
pg_ctl, postgres (and probably others) use:

 a) getcwd(3) in find_my_exec in order to complement a relative path
containing at least one directory separator

 b) getcwd(3) and chdir(2) in resolve_symlinks in order to prune
symlinks from given absolute path

In both cases, the utility/daemon will fail (and exit) if the current
directory is either not readable or not searchable, even though said
utility/daemon has permission to access each component of its absolute
path in argv0.

That scenario is not uncommon:

$ id
uid=1000(petar)

$ pwd
/home/petar

$ ls -lad .
drwx--  5 petar  petar  512 Mar  2 23:41 .

$ sudo -u pgsql /usr/bin/pg_ctl start (...)
could not identify current directory: Permission denied
could not identify current directory: Permission denied
could not identify current directory: Permission denied
The program "postgres" is needed by pg_ctl but was not found in the same 
directory as "pg_ctl".
Check your installation.


There is an easy workaround for (a):  Only do getcwd if argv0 is not an
absolute path.  The variable cwd is not relevant for absolute paths.

(b) is not as straightforward;  resolve_symlinks only trusts getcwd to
provide symlink-free paths (see comment for rationale) so, during path
resolution, it will change the current directory one or more times and
finally, before returing, chdir back to the original wd.

The last step is, however, not possible if the original wd is not read-
and/or searchable.

In the patch below, path resolution is skipped if getcwd returns EACCES.
The other option would have been ignoring the initial getcwd / final
chdir and just keeping the most recent wd, which, at that point, could
be pretty much any component of the absolute path.  That seemed too
unpredictable.

This was tested with pgsql94 but that part of exec.c doesn't seem to
have changed in any more recent version.

Thanks,
Petar


--- src/common/exec.c.orig
+++ src/common/exec.c
@@ -103,45 +103,45 @@
 
 /*
  * find_my_exec -- find an absolute path to a valid executable
  *
  * argv0 is the name passed on the command line
  * retpath is the output area (must be of size MAXPGPATH)
  * Returns 0 if OK, -1 if error.
  *
  * The reason we have to work so hard to find an absolute path is that
  * on some platforms we can't do dynamic loading unless we know the
  * executable's location.  Also, we need a full path not a relative
  * path because we will later change working directory.  Finally, we want
  * a true path not a symlink location, so that we can locate other files
  * that are part of our installation relative to the executable.
  */
 int
 find_my_exec(const char *argv0, char *retpath)
 {
charcwd[MAXPGPATH],
test_path[MAXPGPATH];
char   *path;
 
-   if (!getcwd(cwd, MAXPGPATH))
+   if (!is_absolute_path(argv0) && !getcwd(cwd, MAXPGPATH))
{
log_error(_("could not identify current directory: %s"),
  strerror(errno));
return -1;
}
 
/*
 * If argv0 contains a separator, then PATH wasn't used.
 */
if (first_dir_separator(argv0) != NULL)
{
if (is_absolute_path(argv0))
StrNCpy(retpath, argv0, MAXPGPATH);
else
join_path_components(retpath, cwd, argv0);
canonicalize_path(retpath);
 
if (validate_exec(retpath) == 0)
return resolve_symlinks(retpath);
 
log_error(_("invalid binary \"%s\""), retpath);
return -1;
@@ -219,44 +219,50 @@
 resolve_symlinks(char *path)
 {
 #ifdef HAVE_READLINK
struct stat buf;
charorig_wd[MAXPGPATH],
link_buf[MAXPGPATH];
char   *fname;
 
/*
 * To resolve a symlink properly, we have to chdir into its directory 
and
 * then chdir to where the symlink points; otherwise we may fail to
 * resolve relative links correctly (consider cases involving mount
 * points, for example).  After following the final symlink, we use
 * getcwd() to figure out where the heck we're at.
 *
 * One might think we could skip all this if path doesn't point to a
 * symlink to start with, but that's wrong.  We also want to get rid of
 * any directory symlinks that are present in the given path. We expect
 * getcwd() to give us an accurate, symlink-free path.
 */
if (!getcwd(orig_wd, MAXPGPATH))
{
+   if (errno == EACCES)
+   {
+   log_error(_("access to current directory denied, "
+   "skipping path resolution of \"%s\""), 
path);
+  

Re: JIT compiling with LLVM v11

[Andres Freund]

Hi,

On 2018-03-03 09:37:35 -0500, Peter Eisentraut wrote:
> On 3/2/18 19:29, Andres Freund wrote:
> >> Using my standard set of CC=gcc-7 and CXX=g++-7, the build fails with
> >>
> >> g++-7: error: unrecognized command line option '-stdlib=libc++'
> 
> > It's actually already filtered, I just added -std*, because of selecting
> > the c++ standard, I guess I need to filter more aggressively.  This is
> > fairly fairly annoying.
> 
> I see you already filter llvm-config --cflags by picking only -I and -D.
>  Why not do the same for --cxxflags?  Any other options that we need
> like -f* should be discovered using the normal
> does-the-compiler-support-this-option tests.

Well, some -f obtions are ABI / behaviour influencing.  You can't, to my
knowledge, mix/match code build with -fno-rtti with code built with it
(influences symbol names).  LLVM builds without rtti by default, but a
lot of distros enable it...

I narrowed the filter to -std= (from -std), which should take care of
the -stdlib bit. I also dropped -fno-exceptions being copied since that
should not conflict.


> >> It seems that it was intended that way anyway, since llvmjit.h contains
> >> its own provisions for extern C.
> > 
> > Hrmpf, yea, I broke that the third time now.  I'm actually inclined to
> > add an appropriate #ifdef ... #error so it's not repeated, what do you
> > think?
> 
> Not sure.  Why not just move the line and not move it again? ;-)

Heh, done ;). Let's see how long it takes...


> > Does putting an
> > override COMPILER = $(CXX) $(CFLAGS)
> > 
> > into src/backend/jit/llvm/Makefile work?  It does force the use of CXX
> > for all important platforms if I see it correctly. Verified that it
> > works on linux.
> 
> Your latest HEAD builds out of the box for me now using the system compiler.

Cool.


> >> configure didn't find any of the LLVMOrc* symbols it was looking for.
> >> Is that a problem?  They seem to be for some debugging support.
> > 
> > That's not a problem, except that the symbols won't be registered with
> > the debugger, which is a bit annoying for backtraces. I tried to have
> > configure throw errors in cases llvm is too old or such.
> 
> Where does one get those then?  I have LLVM 5.0.1.  Is there something
> even newer?

I've submitted them upstream, but they're not yet released.


> > Hm, I'll switch them on in the development branch. Independent of the
> > final decision that's definitely the right thing for now.  The "full
> > capability" of the patchset is used if you turn on these three GUCs:
> > 
> > -c jit_expressions=1
> > -c jit_tuple_deforming=1
> > -c jit_perform_inlining=1
> 
> The last one doesn't seem to exist anymore.

Yup, as discussed in the earlier reply to you, I decided it's not
particularly useful to have.  As also threatened in that reply, I've
switched the defaults so you shouldn't have to change them anymore.


> If I turn on either of the first two, then make installcheck fails.  See
> attached diff.

Hm, so there's definitely something going on here that I don't yet
understand. I've pushed something that I've a slight hunch about
(dropping the dots from the symbol names, some tooling doesn't seem to
like that).

I'd to rebase to fix a few issues, but I've left the changes made since
the last push as separate commits.

Could you run something like:
regression[18425][1]=# set jit_above_cost = 0;
SET
regression[18425][1]=# set client_min_messages=debug2;
SET
regression[18425][1]=# SELECT pg_jit_available();
DEBUG:  0: probing availability of llvm for JIT at 
/home/andres/build/postgres/dev-assert/install/lib/llvmjit.so
LOCATION:  provider_init, jit.c:83
DEBUG:  0: successfully loaded LLVM in current session
LOCATION:  provider_init, jit.c:107
DEBUG:  0: time to opt: 0.001s
LOCATION:  llvm_compile_module, llvmjit.c:435
DEBUG:  0: time to emit: 0.014s
LOCATION:  llvm_compile_module, llvmjit.c:481
┌──┐
│ pg_jit_available │
├──┤
│ t│
└──┘
(1 row)

regression[18425][1]=# select now();
DEBUG:  0: time to opt: 0.001s
LOCATION:  llvm_compile_module, llvmjit.c:435
DEBUG:  0: time to emit: 0.008s
LOCATION:  llvm_compile_module, llvmjit.c:481
┌───┐
│  now  │
├───┤
│ 2018-03-03 11:33:13.776947-08 │
└───┘
(1 row)

regression[18425][1]=# SET jit_dump_bitcode = 1;
SET
regression[18425][1]=# select now();
DEBUG:  0: time to opt: 0.002s
LOCATION:  llvm_compile_module, llvmjit.c:435
DEBUG:  0: time to emit: 0.018s
LOCATION:  llvm_compile_module, llvmjit.c:481
┌───┐
│  now  │
├───┤
│ 2018-03-03 11:33:23.508875-08 │
└───┘
(1 row)


The last command should have dumped something into your data directory,
even if it failed like your regression test output showed. Could attach
the two files something 

Re: pgbench - allow to specify scale as a size

[Peter Eisentraut]

On 2/20/18 05:06, Fabien COELHO wrote:
>> Now the overhead is really 60-65%. Although the specification is 
>> unambiguous, 
>> but we still need some maths to know whether it fits in buffers or memory... 
>> The point of Karel regression is to take this into account.
>>
>> Also, whether this option would be more admissible to Tom is still an open 
>> question. Tom?
> 
> Here is a version with this approach: the documentation talks about 
> "actual data size, without overheads", and points out that storage 
> overheads are typically an additional 65%.

I think when deciding on a size for a test database for benchmarking,
you want to size it relative to RAM or other storage layers.  So a
feature that allows you to create a database of size N but it's actually
not going to be anywhere near N seems pretty useless for that.

(Also, we have, for better or worse, settled on a convention for byte
unit prefixes in guc.c.  Let's not introduce another one.)

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: autovacuum: change priority of the vacuumed tables

[Jim Nasby]

On 2/19/18 10:00 AM, Tomas Vondra wrote:

So I don't think this is a very promising approach, unfortunately.

What I think might work is having a separate pool of autovac workers,
dedicated to these high-priority tables. That still would not guarantee
the high-priority tables are vacuumed immediately, but at least that
they are not stuck in the worker queue behind low-priority ones.

I wonder if we could detect tables with high update/delete activity and
promote them to high-priority automatically. The reasoning is that by
delaying the cleanup for those tables would result in significantly more
bloat than for those with low update/delete activity.


I've looked at this stuff in the past, and I think that the first step 
in trying to improve autovacuum needs to be allowing for a much more 
granular means of controlling worker table selection, and exposing that 
ability. There are simply too many different scenarios to try and 
account for to try and make a single policy that will satisfy everyone. 
Just as a simple example, OLTP databases (especially with small queue 
tables) have very different vacuum needs than data warehouses.


One fairly simple option would be to simply replace the logic that 
currently builds a worker's table list with running a query via SPI. 
That would allow for prioritizing important tables. It could also reduce 
the problem of workers getting "stuck" on a ton of large tables by 
taking into consideration the total number of pages/tuples a list contains.


A more fine-grained approach would be to have workers make a new 
selection after every vacuum they complete. That would provide the 
ultimate in control, since you'd be able to see exactly what all the 
other workers are doing.

--
Jim Nasby, Chief Data Architect, Austin TX
OpenSCG http://OpenSCG.com

Re: Function to track shmem reinit time

[Justin Pryzby]

On Sat, Mar 03, 2018 at 01:00:52PM -0500, Peter Eisentraut wrote:
> On 2/28/18 07:11, Anastasia Lubennikova wrote:
> > Currently, if the 'restart_after_crash' option is on, postgres will just
> > restart.
> > And the only way to know that it happened is to regularly parse logfile
> > or monitor it, catching restart messages. This approach is really
> > inconvenient for
> > users, who have gigabytes of logs.
> 
> I find this premise a bit dubious.  Why have a log file if it's too big
> to find anything in it?  Server crashes aren't the only thing people are
> interested in.  So we'll need a function for "last $anything".

I think one can tell if it's crashed recently by comparing start time of parent
postmaster and its main children (I'm going to go put this in place for myself
now).

ts=# SELECT backend_type, backend_start, pg_postmaster_start_time(), 
backend_start-pg_postmaster_start_time() FROM pg_stat_activity ORDER BY 
backend_start LIMIT 1;
backend_type | backend_start |   
pg_postmaster_start_time|?column? 
-+---+---+-
 autovacuum launcher | 2018-03-02 00:21:11.604468-03 | 2018-03-02 
00:12:46.757642-03 | 00:08:24.846826

pryzbyj@pryzbyj:~$ ps -O lstart -upostgres # on a separate server with 9.4 
which doen't have backend_type, add in v10
  PID  STARTED S TTY  TIME COMMAND
12982 Mon Feb 19 06:03:13 2018 S ?00:00:33 
/usr/lib/postgresql/9.4/bin/postgres -D /var/lib/postgresql/9.4/main -c 
config_file=/etc/postgresql/9.4/main/postgresql.conf
12997 Mon Feb 19 06:03:14 2018 S ?00:00:00 postgres: checkpointer 
process 
 
12998 Mon Feb 19 06:03:14 2018 S ?00:00:08 postgres: writer process 

   
12999 Mon Feb 19 06:03:14 2018 S ?00:00:08 postgres: wal writer process 

   
13000 Mon Feb 19 06:03:14 2018 S ?00:00:19 postgres: autovacuum 
launcher process
   
13001 Mon Feb 19 06:03:14 2018 S ?00:00:45 postgres: stats collector 
process 
  

pryzbyj@pryzbyj:~$ sudo kill -SEGV 12997
pryzbyj@pryzbyj:~$ ps -O lstart -upostgres
  PID  STARTED S TTY  TIME COMMAND
 8644 Sat Mar  3 12:05:34 2018 S ?00:00:00 postgres: checkpointer 
process 
 
 8645 Sat Mar  3 12:05:34 2018 S ?00:00:00 postgres: writer process 

   
 8646 Sat Mar  3 12:05:34 2018 S ?00:00:00 postgres: wal writer process 

   
 8647 Sat Mar  3 12:05:34 2018 S ?00:00:00 postgres: autovacuum 
launcher process
   
 8648 Sat Mar  3 12:05:34 2018 S ?00:00:00 postgres: stats collector 
process 
  
12982 Mon Feb 19 06:03:13 2018 S ?00:00:33 
/usr/lib/postgresql/9.4/bin/postgres -D /var/lib/postgresql/9.4/main -c 
config_file=/etc/postgresql/9.4/main/postgresql.conf

Justin

Re: [PATCH] Add support for ON UPDATE/DELETE actions on ALTER CONSTRAINT

[Peter Eisentraut]

On 2/20/18 10:10, Matheus de Oliveira wrote:
> Besides that, there is a another change in this patch on current ALTER
> CONSTRAINT about deferrability options. Previously, if the user did
> ALTER CONSTRAINT without specifying an option on deferrable or
> initdeferred, it was implied the default options, so this:
> 
>     ALTER TABLE tbl
>     ALTER CONSTRAINT con_name;
> 
> Was equivalent to:
> 
>     ALTER TABLE tbl
>     ALTER CONSTRAINT con_name NOT DEFERRABLE INITIALLY IMMEDIATE;

Oh, that seems wrong.  Probably, it shouldn't even accept that syntax
with an empty options list, let alone reset options that are not
mentioned.  Can you prepare a separate patch for this issue?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Online enabling of checksums

[Magnus Hagander]

On Sat, Mar 3, 2018 at 5:17 PM, Tomas Vondra 
wrote:

>
>
> On 03/03/2018 05:08 PM, Magnus Hagander wrote:
> >
> >
> > On Sat, Mar 3, 2018 at 5:06 PM, Tomas Vondra
> > >
> wrote:
> >
> > On 03/03/2018 01:38 PM, Robert Haas wrote:
> > > On Sat, Mar 3, 2018 at 7:32 AM, Robert Haas  > wrote:
> > >> On Fri, Mar 2, 2018 at 6:26 PM, Tomas Vondra
> > >>  2ndquadrant.com>>
> > wrote:
> > >>> Hmmm, OK. So we need to have a valid checksum on a page, disable
> > >>> checksums, set some hint bits on the page (which won't be
> > >>> WAL-logged), enable checksums again and still get a valid
> > >>> checksum even with the new hint bits? That's possible, albeit
> > >>> unlikely.
> > >>
> > >> No, the problem is if - as is much more likely - the checksum is
> > >> not still valid.
> > >
> > > Hmm, on second thought ... maybe I didn't think this through
> > > carefully enough. If the checksum matches on the master by chance,
> > > and the page is the same on the standby, then we're fine, right?
> It's
> > > a weird accident, but nothing is actually broken. The failure
> > > scenario is where the standby has a version of the page with a bad
> > > checksum, but the master has a good checksum. So for example:
> > > checksums disabled, master modifies the page (which is replicated),
> > > master sets some hint bits (coincidentally making the checksum
> > > match), now we try to turn checksums on and don't re-replicate the
> > > page because the checksum already looks correct.
> > >
> >
> > Yeah. Doesn't that pretty much mean we can't skip any pages that have
> > correct checksum, because we can't rely on standby having the same
> page
> > data? That is, this block in ProcessSingleRelationFork:
> >
> >   /*
> >* If checksum was not set or was invalid, mark the buffer as dirty
> >* and force a full page write. If the checksum was already valid,
> we
> >* can leave it since we know that any other process writing the
> >* buffer will update the checksum.
> >*/
> >   if (checksum != pagehdr->pd_checksum)
> >   {
> >   START_CRIT_SECTION();
> >   MarkBufferDirty(buf);
> >   log_newpage_buffer(buf, false);
> >   END_CRIT_SECTION();
> >   }
> >
> > That would mean this optimization - only doing the write when the
> > checksum does not match - is broken.
> >
> >
> > Yes. I think that was the conclusion of this, as posted
> > in https://www.postgresql.org/message-id/CABUevExDZu__
> 5KweT8fr3Ox45YcuvTDEEu%3DaDpGBT8Sk0RQE_g%40mail.gmail.com
> > :)
> >
>
> Oh, right. I did have a "deja vu" feeling, when writing that. Good that
> I came to the same conclusion, though.
>
> >
> > If that's the case, it probably makes restarts/resume more expensive,
> > because this optimization was why after restart the already processed
> > data was only read (and the checksums verified) but not written.
> >
> >
> > Yes, it definitely does. It's not a dealbreaker, but it's certainly
> > a bit painful not to be able to resume as cheap.
> >
>
> Yeah. It probably makes the more elaborate resuming more valuable, but I
> still think it's not a "must have" for PG11.
>
>
Attached is a rebased patch which removes this optimization, updates the
pg_proc entry for the new format, and changes pg_verify_checksums to use -r
instead of -o for relfilenode.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 00fc364c0a..bf6f694640 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -8541,7 +8541,8 @@ LOG:  CleanUpLock: deleting: lock(0xb7acd844) id(24688,24696,0,0,0,1)
 or hide corruption, or other serious problems.  However, it may allow
 you to get past the error and retrieve undamaged tuples that might still be
 present in the table if the block header is still sane. If the header is
-corrupt an error will be reported even if this option is enabled. The
+corrupt an error will be reported even if this option is enabled. This
+option can only be enabled when data checksums are enabled. The
 default setting is off, and it can only be changed by a superuser.

   
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 2f59af25a6..a011ea1d8f 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -19481,6 +19481,71 @@ postgres=# SELECT * FROM pg_walfile_name_offset(pg_stop_backup());
 
   
 
+  
+   Data Checksum Functions
+
+   
+The functions shown in  can
+be used to enable 

Re: [HACKERS] pg_upgrade to clusters with a different WAL segment size

[Peter Eisentraut]

On 2/7/18 13:34, Bossart, Nathan wrote:
> Here is a patch to allow users to change the WAL segment size of a cluster 
> with
> pg_resetwal.  Like the '--wal-segize' option in initdb, the new '-s' option
> accepts segment sizes in megabytes.

Or how about we call the new option, um, --wal-segsize?

> The first is a division-by-zero error caused when the control data must be
> guessed.  If the control data is damaged, WalSegSz will not be set to the
> guessed WAL segment size, resulting in an error during XLogFromFileName().  
> The
> attached patch resolves this issue by ensuring WalSegSz is set to a valid 
> value
> after either reading or guessing the control values.

I noticed this issue in pg_controldata too.  Maybe that should be
combined in a separate patch.

> Note that some segment size changes will cause old WAL file names to be 
> reused.
> The new documentation for '-s' contains a note warning of this side effect.  I
> considered a couple of strategies to prevent this, but I chose not to include
> any in the patch based on previous correspondence in this thread.  If file 
> name
> overlap is an issue, users can already use the '-l' option to set a safe WAL
> starting address.

The patch "Configurable file mode mask" contains the beginning of a test
suite for pg_resetwal.  It would be great if we could get a test case
for this new functionality implemented.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pg_get_functiondef forgets about most GUC_LIST_INPUT GUCs

[Arthur Zakirov]

On Wed, Feb 21, 2018 at 04:35:50PM +0900, Michael Paquier wrote:
> You have an excellent point here, and I did not consider it.
> For pg_dump and pg_dumpall, something like what I described in
> https://www.postgresql.org/message-id/20180112012440.ga2...@paquier.xyz
> to extend pg_settings so as GUC types are visible via SQL could make
> sense, now it is really a lot for just being able to quote parameters
> correctly.  On top of that, what I suggested previously would not work
> reliably except if we have pg_get_functiondef load the library
> associated to a given parameter.  Even in this case there could
> perfectly be a custom parameter from another plugin and not a parameter
> associated to the function language itself.

In my opinion GetConfigOptionFlags() can be used for core and plpgsql GUCs.
A variable should not be quoted if it has GUC_LIST_INPUT flag or it is
plpgsql.extra_warnings or plpgsql.extra_errors.

I'm not sure that it is good to raise an error when the variable isn't
found, because without the patch the error isn't raised. But maybe better
to raise it to notify a user that there is a wrong variable. In this case we
may not raise the error if variable name contains
GUC_QUALIFIER_SEPARATOR.

> It seems to me that this brings us back to a best-effort for the backend
> and the frontend by maintaining a list of hardcoded parameter names,
> which could be refined a maximum by considering lists for in-core
> extensions and perhaps the most famous extensions around :(

It seems for frontend maintaining a hardcoded list is the only way.
Agree that extending pg_settings for that could be too much.


I've searched extensions in GitHub with GUC_LIST_INPUT variables. There
are couple of them:
- https://github.com/ohmu/pgmemcache
- https://github.com/marconeperes/pgBRTypes
And some not very fresh:
- https://github.com/witblitz/pldotnet
- https://github.com/ohmu/pgloggingfilter
- https://github.com/wangyuehong/pggearman
- https://github.com/siavashg/pgredis

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Re: [HACKERS] user-defined numeric data types triggering ERROR: unsupported type

[Tomas Vondra]

Hi,

Here is v2 of the fix. It does handle all the convert_to_scalar calls
for various data types, just like the numeric one did in v1 with the
exception of bytea.

The bytea case is fixed by checking that the boundary values are
varlenas. This seems better than checking for BYTEAOID explicitly, which
would fail for custom varlena-based types. At first I've been thinking
there might be issues when the data types has mismatching ordering, but
I don't think the patch makes it any worse.

I've also added a bunch of regression tests, checking each case. The
bytea test it should cause segfault on master, of course.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/utils/adt/network.c b/src/backend/utils/adt/network.c
index aac7621..34cf336 100644
--- a/src/backend/utils/adt/network.c
+++ b/src/backend/utils/adt/network.c
@@ -904,7 +904,7 @@ inet_merge(PG_FUNCTION_ARGS)
  * involving network types.
  */
 double
-convert_network_to_scalar(Datum value, Oid typid)
+convert_network_to_scalar(Datum value, Oid typid, bool *unknown_type)
 {
 	switch (typid)
 	{
@@ -960,7 +960,7 @@ convert_network_to_scalar(Datum value, Oid typid)
 	 * Can't get here unless someone tries to use scalarineqsel() on an
 	 * operator with one network and one non-network operand.
 	 */
-	elog(ERROR, "unsupported type: %u", typid);
+	*unknown_type = true;
 	return 0;
 }
 
diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c
index fcc8323..b210541 100644
--- a/src/backend/utils/adt/selfuncs.c
+++ b/src/backend/utils/adt/selfuncs.c
@@ -176,7 +176,7 @@ static bool estimate_multivariate_ndistinct(PlannerInfo *root,
 static bool convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
   Datum lobound, Datum hibound, Oid boundstypid,
   double *scaledlobound, double *scaledhibound);
-static double convert_numeric_to_scalar(Datum value, Oid typid);
+static double convert_numeric_to_scalar(Datum value, Oid typid, bool *unknown_type);
 static void convert_string_to_scalar(char *value,
 		 double *scaledvalue,
 		 char *lobound,
@@ -193,8 +193,9 @@ static double convert_one_string_to_scalar(char *value,
 			 int rangelo, int rangehi);
 static double convert_one_bytea_to_scalar(unsigned char *value, int valuelen,
 			int rangelo, int rangehi);
-static char *convert_string_datum(Datum value, Oid typid);
-static double convert_timevalue_to_scalar(Datum value, Oid typid);
+static char *convert_string_datum(Datum value, Oid typid, bool *unknown_type);
+static double convert_timevalue_to_scalar(Datum value, Oid typid,
+			bool *unknown_type);
 static void examine_simple_variable(PlannerInfo *root, Var *var,
 		VariableStatData *vardata);
 static bool get_variable_range(PlannerInfo *root, VariableStatData *vardata,
@@ -4071,10 +4072,18 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
 		case REGDICTIONARYOID:
 		case REGROLEOID:
 		case REGNAMESPACEOID:
-			*scaledvalue = convert_numeric_to_scalar(value, valuetypid);
-			*scaledlobound = convert_numeric_to_scalar(lobound, boundstypid);
-			*scaledhibound = convert_numeric_to_scalar(hibound, boundstypid);
-			return true;
+			{
+bool	unknown_type = false;
+
+*scaledvalue = convert_numeric_to_scalar(value, valuetypid,
+		 _type);
+*scaledlobound = convert_numeric_to_scalar(lobound, boundstypid,
+		   _type);
+*scaledhibound = convert_numeric_to_scalar(hibound, boundstypid,
+		   _type);
+
+return (!unknown_type);
+			}
 
 			/*
 			 * Built-in string types
@@ -4085,9 +4094,18 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
 		case TEXTOID:
 		case NAMEOID:
 			{
-char	   *valstr = convert_string_datum(value, valuetypid);
-char	   *lostr = convert_string_datum(lobound, boundstypid);
-char	   *histr = convert_string_datum(hibound, boundstypid);
+bool	unknown_type = false;
+
+char   *valstr = convert_string_datum(value, valuetypid,
+	  _type);
+char   *lostr = convert_string_datum(lobound, boundstypid,
+	 _type);
+char   *histr = convert_string_datum(hibound, boundstypid,
+	 _type);
+
+/* bail out if any of the values is not a string */
+if (unknown_type)
+	return false;
 
 convert_string_to_scalar(valstr, scaledvalue,
 		 lostr, scaledlobound,
@@ -4103,6 +4121,18 @@ convert_to_scalar(Datum value, Oid valuetypid, double *scaledvalue,
 			 */
 		case BYTEAOID:
 			{
+TypeCacheEntry *typcache;
+
+/*
+ * Ensure parameters passed to convert_bytea_to_scalar are
+ * all varlena. We're dealing with bytea values, but this
+ * seems like a reasonable way to handle custom types.
+ */
+typcache = lookup_type_cache(boundstypid, 0);
+
+if (typcache->typlen != -1)
+	return false;
+
 convert_bytea_to_scalar(value, 

Re: perltidy version

[Magnus Hagander]

On Fri, Mar 2, 2018 at 4:49 PM, Tom Lane  wrote:

> Magnus Hagander  writes:
> > On Fri, Mar 2, 2018 at 3:53 PM, Tom Lane  wrote:
> >> +1.  We're not that far away from it being time to run
> pgindent/perltidy,
> >> so now would be a good time to consider whether we like a newer
> version's
> >> result better.
>
> > For example, Debian ships with 20140328, which produces the attached
> diff.
> > I'm not sure if we want to go to whatever is a "common version on most
> > platforms" today, or just "whatever is latest" if we do upgrade. AFAICT
> > RHEL 7 seems to be on 20121207, RHEL 6 on 20090616. And in Ubuntu, 14.04
> > has 20120701, 16.04 has 20140328, and current devel has 20140328. In
> > general there seems to be very little overlap there, except Debian and
> > Ubuntu covers the same versions.
>
> > (Note that this diff is against HEAD -- it's possible a perltidy run with
> > the current version would also generate a diff, I have not compared them
> to
> > each other)
>
> Yeah, perltidy 20090616 already produces a pretty substantial diff on
> HEAD; attached.
>

Ah yeah, if I apply that one first, the diff from using 20140328 is much
smaller. Attached is that one, which means the difference between the two
perltidy versions.

-- 
 Magnus Hagander
 Me: https://www.hagander.net/ 
 Work: https://www.redpill-linpro.com/ 
diff --git a/doc/src/sgml/mk_feature_tables.pl b/doc/src/sgml/mk_feature_tables.pl
index 9b111b8b40..476e50e66d 100644
--- a/doc/src/sgml/mk_feature_tables.pl
+++ b/doc/src/sgml/mk_feature_tables.pl
@@ -38,8 +38,8 @@ while (<$feat>)
 
 	$is_supported eq $yesno || next;
 
-	$feature_name=~ s///g;
+	$feature_name =~ s///g;
 	$subfeature_name =~ s///g;
 
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index f80f718480..9a43222dc7 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -295,7 +295,7 @@ foreach my $catname (@{ $catalogs->{names} })
 	# Omit the oid column if the catalog doesn't have them
 	next
 	  if $table->{without_oids}
-		  && $attr->{name} eq 'oid';
+	  && $attr->{name} eq 'oid';
 
 	morph_row_for_pgattr(\%row, $schema, $attr, 1);
 	print_bki_insert(\%row, @attnames);
@@ -422,11 +422,11 @@ sub morph_row_for_pgattr
 		# compare DefineAttr in bootstrap.c. oidvector and
 		# int2vector are also treated as not-nullable.
 		$row->{attnotnull} =
-		$type->{typname} eq 'oidvector'   ? 't'
-		  : $type->{typname} eq 'int2vector'  ? 't'
-		  : $type->{typlen}  eq 'NAMEDATALEN' ? 't'
-		  : $type->{typlen} > 0 ? 't'
-		  :   'f';
+		$type->{typname} eq 'oidvector'  ? 't'
+		  : $type->{typname} eq 'int2vector' ? 't'
+		  : $type->{typlen} eq 'NAMEDATALEN' ? 't'
+		  : $type->{typlen} > 0  ? 't'
+		  :'f';
 	}
 	else
 	{
diff --git a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
index 97b0a07f40..31c12d567f 100644
--- a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
+++ b/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl
@@ -75,8 +75,8 @@ sub run_check
 	create_files();
 
 	command_ok(
-		[   'pg_archivecleanup', '-x', '.gz', $tempdir, $walfiles[2] . $suffix
-		],
+		[   'pg_archivecleanup', '-x', '.gz', $tempdir,
+			$walfiles[2] . $suffix ],
 		"$test_name: runs");
 
 	ok(!-f "$tempdir/$walfiles[0]",
diff --git a/src/bin/pg_basebackup/t/010_pg_basebackup.pl b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
index 18ca77f952..791d16e6a3 100644
--- a/src/bin/pg_basebackup/t/010_pg_basebackup.pl
+++ b/src/bin/pg_basebackup/t/010_pg_basebackup.pl
@@ -190,7 +190,7 @@ SKIP:
 -l "$tempdir/backup1/pg_tblspc/$_"
   and readlink "$tempdir/backup1/pg_tblspc/$_" eq
   "$tempdir/tbackup/tblspc1"
-			  } readdir($dh)),
+			} readdir($dh)),
 		"tablespace symlink was updated");
 	closedir $dh;
 
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index e84693d319..f3a8b16a88 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -374,8 +374,8 @@ my @errors = (
 	# SQL
 	[   'sql syntax error',
 		0,
-		[   qr{ERROR:  syntax error}, qr{prepared statement .* does not exist}
-		],
+		[   qr{ERROR:  syntax error},
+			qr{prepared statement .* does not exist} ],
 		q{-- SQL syntax error
 SELECT 1 + ;
 } ],
diff --git a/src/include/catalog/duplicate_oids b/src/include/catalog/duplicate_oids
index 7342d618ed..945fcf2b1d 100755
--- a/src/include/catalog/duplicate_oids
+++ b/src/include/catalog/duplicate_oids
@@ -15,11 +15,11 @@ while (<>)
 	next if /^CATALOG\(.*BKI_BOOTSTRAP/;
 	next
 	  unless /^DATA\(insert *OID *= *(\d+)/
-		  || /^CATALOG\([^,]*, *(\d+).*BKI_ROWTYPE_OID\((\d+)\)/
-		  || /^CATALOG\([^,]*, *(\d+)/
-		  || 

Re: Function to track shmem reinit time

[Peter Eisentraut]

On 2/28/18 07:11, Anastasia Lubennikova wrote:
> Currently, if the 'restart_after_crash' option is on, postgres will just
> restart.
> And the only way to know that it happened is to regularly parse logfile
> or monitor it, catching restart messages. This approach is really
> inconvenient for
> users, who have gigabytes of logs.

I find this premise a bit dubious.  Why have a log file if it's too big
to find anything in it?  Server crashes aren't the only thing people are
interested in.  So we'll need a function for "last $anything".

> This new function can be periodiacally called by a monitoring agent, and,
> if /shmem_init_time/ doesn't match /pg_postmaster_start_time,/
> we know that server crashed-restarted, and also know the exact time, when.

If we want to do something like this, I'd rather have a name that does
not expose implementation details.  If we restructure shared memory
management in the future, the name won't apply anymore.  Call it "last
crash" or "last reinit" or something like that.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [PATCH] Minor fixes for reloptions tests

[Peter Eisentraut]

On 1/22/18 15:06, Nikolay Shaplov wrote:
> I have a small fixes for already committed reloption test patch
> (https://www.postgresql.org/message-id/2615372.orqtEn8VGB@x200m)
> 
> First one is missing tab symbol where it should be.
> 
> The second is a comment. "The OIDS option is not stored" is not quite 
> correct. 
> They are stored, but not as reloption. I've added this notion there...

committed

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: pgbench randomness initialization

[Chapman Flack]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:tested, passed

The patch 8 works and addresses the things I noticed earlier.

It needs s/explicitely/explicitly/ in the docs.

The parsing of the seed involves matters of taste, I guess: if it were a signed 
int, then
sscanf's built-in %i would do everything those three explicit hex/octal/decimal 
branches
do, but there's no unsigned version of %i. Then there's strtoul(..., base=0), 
which accepts
the same choice of bases, but there's no unsigned-int-width version of that. 
Maybe it
would still look cleaner to use strtoul and just check that the result fits in 
unsigned int?
As I began, it comes down to taste ... this code does work.

I am not sure about the "idem for :random_seed" part: Does this mean that a 
value
could be given with -Drandom_seed on the command line, and become the value
of :random_seed, possibly different from the value given to --random-seed?
Is that intended? (Perhaps it is; I'm merely asking.)

-Chap

The new status of this patch is: Waiting on Author

Re: WIP: a way forward on bootstrap data

[Tom Lane]

John Naylor  writes:
> On 3/3/18, Tom Lane  wrote:
>> * In 0006, I'm not very pleased with the introduction of
>> "Makefile.headers".

> I wasn't happy with it either, but I couldn't get it to build
> otherwise. The sticking point was the symlinks in
> $(builddir)/src/include/catalog.  $(MAKE) -C catalog doesn't handle
> that. The makefile in /src/common relies on the backend makefile to
> know what to invoke for a given header. IIRC, relpath.c includes
> pg_tablespace.h, which now requires pg_tablespace_d.h to be built.

I'm not following.  AFAICS, what you put in src/common/Makefile was just

+.PHONY: generated-headers
+
+generated-headers:
+   $(MAKE) -C ../backend generated-headers

which doesn't appear to care whether backend/Makefile knows anything
about specific generated headers or not.  I think all we need to do
is consider that the *_d.h files ought to be built as another consequence
of invoking the generated-headers target.

BTW, there's already a submake-generated-headers target in
Makefile.global, which you should use in preference to rolling your own.

regards, tom lane

Re: WIP: a way forward on bootstrap data

[Tom Lane]

John Naylor  writes:
> I've attached a patch that takes care of these cleanups so they don't
> clutter the patch set.

Pushed.  I made a couple of cosmetic changes in genbki.pl.

regards, tom lane

Re: Online enabling of checksums

[Tomas Vondra]

On 03/03/2018 05:08 PM, Magnus Hagander wrote:
> 
> 
> On Sat, Mar 3, 2018 at 5:06 PM, Tomas Vondra
> > wrote:
> 
> On 03/03/2018 01:38 PM, Robert Haas wrote:
> > On Sat, Mar 3, 2018 at 7:32 AM, Robert Haas  > wrote:
> >> On Fri, Mar 2, 2018 at 6:26 PM, Tomas Vondra
> >> >
> wrote:
> >>> Hmmm, OK. So we need to have a valid checksum on a page, disable
> >>> checksums, set some hint bits on the page (which won't be
> >>> WAL-logged), enable checksums again and still get a valid
> >>> checksum even with the new hint bits? That's possible, albeit
> >>> unlikely.
> >>
> >> No, the problem is if - as is much more likely - the checksum is
> >> not still valid.
> >
> > Hmm, on second thought ... maybe I didn't think this through
> > carefully enough. If the checksum matches on the master by chance,
> > and the page is the same on the standby, then we're fine, right? It's
> > a weird accident, but nothing is actually broken. The failure
> > scenario is where the standby has a version of the page with a bad
> > checksum, but the master has a good checksum. So for example:
> > checksums disabled, master modifies the page (which is replicated),
> > master sets some hint bits (coincidentally making the checksum
> > match), now we try to turn checksums on and don't re-replicate the
> > page because the checksum already looks correct.
> >
> 
> Yeah. Doesn't that pretty much mean we can't skip any pages that have
> correct checksum, because we can't rely on standby having the same page
> data? That is, this block in ProcessSingleRelationFork:
> 
>   /*
>    * If checksum was not set or was invalid, mark the buffer as dirty
>    * and force a full page write. If the checksum was already valid, we
>    * can leave it since we know that any other process writing the
>    * buffer will update the checksum.
>    */
>   if (checksum != pagehdr->pd_checksum)
>   {
>       START_CRIT_SECTION();
>       MarkBufferDirty(buf);
>       log_newpage_buffer(buf, false);
>       END_CRIT_SECTION();
>   }
> 
> That would mean this optimization - only doing the write when the
> checksum does not match - is broken.
> 
> 
> Yes. I think that was the conclusion of this, as posted
> in 
> https://www.postgresql.org/message-id/CABUevExDZu__5KweT8fr3Ox45YcuvTDEEu%3DaDpGBT8Sk0RQE_g%40mail.gmail.com
> :)
> 

Oh, right. I did have a "deja vu" feeling, when writing that. Good that
I came to the same conclusion, though.

> 
> If that's the case, it probably makes restarts/resume more expensive,
> because this optimization was why after restart the already processed
> data was only read (and the checksums verified) but not written.
> 
> 
> Yes, it definitely does. It's not a dealbreaker, but it's certainly
> a bit painful not to be able to resume as cheap.
> 

Yeah. It probably makes the more elaborate resuming more valuable, but I
still think it's not a "must have" for PG11.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Online enabling of checksums

[Magnus Hagander]

On Sat, Mar 3, 2018 at 5:06 PM, Tomas Vondra 
wrote:

> On 03/03/2018 01:38 PM, Robert Haas wrote:
> > On Sat, Mar 3, 2018 at 7:32 AM, Robert Haas 
> wrote:
> >> On Fri, Mar 2, 2018 at 6:26 PM, Tomas Vondra
> >>  wrote:
> >>> Hmmm, OK. So we need to have a valid checksum on a page, disable
> >>> checksums, set some hint bits on the page (which won't be
> >>> WAL-logged), enable checksums again and still get a valid
> >>> checksum even with the new hint bits? That's possible, albeit
> >>> unlikely.
> >>
> >> No, the problem is if - as is much more likely - the checksum is
> >> not still valid.
> >
> > Hmm, on second thought ... maybe I didn't think this through
> > carefully enough. If the checksum matches on the master by chance,
> > and the page is the same on the standby, then we're fine, right? It's
> > a weird accident, but nothing is actually broken. The failure
> > scenario is where the standby has a version of the page with a bad
> > checksum, but the master has a good checksum. So for example:
> > checksums disabled, master modifies the page (which is replicated),
> > master sets some hint bits (coincidentally making the checksum
> > match), now we try to turn checksums on and don't re-replicate the
> > page because the checksum already looks correct.
> >
>
> Yeah. Doesn't that pretty much mean we can't skip any pages that have
> correct checksum, because we can't rely on standby having the same page
> data? That is, this block in ProcessSingleRelationFork:
>
>   /*
>* If checksum was not set or was invalid, mark the buffer as dirty
>* and force a full page write. If the checksum was already valid, we
>* can leave it since we know that any other process writing the
>* buffer will update the checksum.
>*/
>   if (checksum != pagehdr->pd_checksum)
>   {
>   START_CRIT_SECTION();
>   MarkBufferDirty(buf);
>   log_newpage_buffer(buf, false);
>   END_CRIT_SECTION();
>   }
>
> That would mean this optimization - only doing the write when the
> checksum does not match - is broken.
>

Yes. I think that was the conclusion of this, as posted in
https://www.postgresql.org/message-id/CABUevExDZu__5KweT8fr3Ox45YcuvTDEEu%3DaDpGBT8Sk0RQE_g%40mail.gmail.com
:)


If that's the case, it probably makes restarts/resume more expensive,
> because this optimization was why after restart the already processed
> data was only read (and the checksums verified) but not written.
>
>
Yes, it definitely does. It's not a dealbreaker, but it's certainly a bit
painful not to be able to resume as cheap.


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

Re: Online enabling of checksums

[Tomas Vondra]

On 03/03/2018 01:38 PM, Robert Haas wrote:
> On Sat, Mar 3, 2018 at 7:32 AM, Robert Haas  wrote:
>> On Fri, Mar 2, 2018 at 6:26 PM, Tomas Vondra
>>  wrote:
>>> Hmmm, OK. So we need to have a valid checksum on a page, disable 
>>> checksums, set some hint bits on the page (which won't be
>>> WAL-logged), enable checksums again and still get a valid
>>> checksum even with the new hint bits? That's possible, albeit
>>> unlikely.
>>
>> No, the problem is if - as is much more likely - the checksum is
>> not still valid.
> 
> Hmm, on second thought ... maybe I didn't think this through
> carefully enough. If the checksum matches on the master by chance,
> and the page is the same on the standby, then we're fine, right? It's
> a weird accident, but nothing is actually broken. The failure
> scenario is where the standby has a version of the page with a bad
> checksum, but the master has a good checksum. So for example:
> checksums disabled, master modifies the page (which is replicated),
> master sets some hint bits (coincidentally making the checksum
> match), now we try to turn checksums on and don't re-replicate the
> page because the checksum already looks correct.
> 

Yeah. Doesn't that pretty much mean we can't skip any pages that have
correct checksum, because we can't rely on standby having the same page
data? That is, this block in ProcessSingleRelationFork:

  /*
   * If checksum was not set or was invalid, mark the buffer as dirty
   * and force a full page write. If the checksum was already valid, we
   * can leave it since we know that any other process writing the
   * buffer will update the checksum.
   */
  if (checksum != pagehdr->pd_checksum)
  {
  START_CRIT_SECTION();
  MarkBufferDirty(buf);
  log_newpage_buffer(buf, false);
  END_CRIT_SECTION();
  }

That would mean this optimization - only doing the write when the
checksum does not match - is broken.

If that's the case, it probably makes restarts/resume more expensive,
because this optimization was why after restart the already processed
data was only read (and the checksums verified) but not written.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: line_perp() (?-|) is broken.

[Tom Lane]

Emre Hasegeli  writes:
>> Possibly this objection is pointless, because I'm not at all sure that
>> the existing code is careful about how it uses FPeq/FPzero; perhaps
>> we're applying EPSILON to all manner of numbers that don't have units
>> of the coordinate space.  But this won't help make it better.

> The existing code was probably paying attention to this particular
> one, but it fails to apply EPSILON meaningfully to many other places.
> For example lseg_parallel() and lseg_perp() applies it 2 times to the
> same input.  First point_sl() compares x coordinates with FPeq(), and
> then the returned slopes are compared again with FPeq().

Yeah, comparing a slope to EPSILON sure feels wrong :-(

regards, tom lane

Re: line_perp() (?-|) is broken.

[Emre Hasegeli]

> However, for either patch, I'm a bit concerned about using FPzero()
> on the inner product result.  To the extent that the EPSILON bound
> has any useful meaning at all, it needs to mean a maximum difference
> between two coordinate values.  The inner product has units of coordinates
> squared, so it seems like EPSILON isn't an appropriate threshold there.

In this case, I suggest this:

>if (FPzero(l1->A))
>PG_RETURN_BOOL(FPzero(l2->B));
>if (FPzero(l2->A))
>PG_RETURN_BOOL(FPzero(l1->B));
>if (FPzero(l1->B))
>PG_RETURN_BOOL(FPzero(l2->A));
>if (FPzero(l2->B))
>PG_RETURN_BOOL(FPzero(l1->A));
>
>PG_RETURN_BOOL(FPeq((l1->A * l2->A) / (l1->B * l2->B), -1.0));

> Possibly this objection is pointless, because I'm not at all sure that
> the existing code is careful about how it uses FPeq/FPzero; perhaps
> we're applying EPSILON to all manner of numbers that don't have units
> of the coordinate space.  But this won't help make it better.

The existing code was probably paying attention to this particular
one, but it fails to apply EPSILON meaningfully to many other places.
For example lseg_parallel() and lseg_perp() applies it 2 times to the
same input.  First point_sl() compares x coordinates with FPeq(), and
then the returned slopes are compared again with FPeq().

Re: [HACKERS] GSOC'17 project introduction: Parallel COPY execution with errors handling

[Tom Lane]

Peter Eisentraut  writes:
> On 3/3/18 00:48, Tom Lane wrote:
>> I don't think that can possibly work.  It would only be safe if, between
>> the thrower and the catcher, there were no other levels of control
>> operating according to the normal error-handling rules.  But input
>> functions certainly cannot assume that they are only called by COPY,
>> so how could they safely throw a "soft error"?

> That assumes that throwing a soft error in a context that does not
> handle it specially is not safe.  I'd imagine in such situations the
> soft error just behaves like a normal exception.

My point is that neither the thrower of the error, nor the catcher of it,
can possibly guarantee that there were no levels of control in between
that were expecting their resource leakages to be cleaned up by normal
error recovery.  As a counterexample consider the chain of control

COPY -> domain_in -> SQL function in CHECK -> int4in

If int4in throws a "soft" error, and COPY catches it and skips
error cleanup, we've likely got problems, depending on what the SQL
function did.  This could even break domain_in, although probably any
such effort would've taken care to harden domain_in against the issue.
But the possibility of SQL or PL functions having done nearly-arbitrary
things in between means that most of the backend is at hazard.

You could imagine making something like this work, but it's gonna take
very invasive and bug-inducing changes to our error cleanup rules,
affecting a ton of places that have no connection to the goal.

regards, tom lane

Re: WIP: a way forward on bootstrap data

[Tom Lane]

John Naylor  writes:
> On 3/3/18, Tom Lane  wrote:
>> * I'm a little disturbed by the fact that 0002 has to "re-doublequote
>> values that are macros expanded by initdb.c".  I see that there are only
>> a small number of affected places, so maybe it's not really worth working
>> harder, but I worry that something might get missed.  Is there any way to
>> include this consideration in the automated conversion, or at least to
>> verify that we found all the places to quote?  Or, seeing that 0004 seems
>> to be introducing some quoting-related hacks to genbki.pl anyway, maybe
>> we could take care of the issue there?

> The quoting hacks are really to keep the postgres.bki diff as small as
> possible (attached). The simplest and most air-tight way to address
> your concern would be to double-quote everything when writing the bki
> file. That could be done last as a follow-up.

Oh, if you're cross-checking by diff'ing the produced .bki file, then
that's sufficient to address my concern here.  No need to do more.

regards, tom lane

Re: 2018-03 Commitfest Summary (Andres #1)

[Tels]

Moin,

On Fri, March 2, 2018 2:48 pm, Andres Freund wrote:
> Hi,
>
> On 2018-03-02 11:06:12 +0100, Fabien COELHO wrote:
>> A lot of patches do not even get a review: no immediate interest or more
>> often no ressources currently available, patch stays put, I'm fine with
>> that.
>
> Well, even if that's the case that's not free of cost to transport them
> from fest to fest. Going through them continually isn't free. At some
> point we just gotta decide it's an undesirable feature.

Erm, I don't think that from A: "There are not enough reviewer capacities"
necessarily follows B: "This is an undesirable feature."

That PG doesn't have enough reviewers doesn't mean users/authors want or
need features - it just means there are not enough people who are capable
of doing a review, or they haven't been found yet, or worse, turned away
by the current process.

Sure, sometimes a lack of interest just means a lack of interest - but not
every user of PG reads or follows the -hackers list or does (or can) even
contribute to PG.

And, the most strongest point to me is: Somebody showed soo much interest
they got up and wrote a patch. Even if nobody reviewed it, that is already
a high hurdle cleared, isn't it?

>> Now, some happy patches actually get reviews and are switched to ready,
>> which shows that somebody saw enough interest in them to spend some time
>> to
>> improve them.
>>
>> If committers ditch these reviewed patches on weak ground (eg "I do not
>> need
>> this feature so nobody should need it"), it is both in contradiction
>> with
>> the fact that someone took the time to review it, and is highly
>> demotivating
>> for people who do participate to the reviewing process and contribute to
>> hopefully improve these patches, because the reviewing time just goes to
>> the
>> drain in the end even when the patch is okay.
>
> The consequence of this appears to be that we should integrate
> everything that anybody decided worthy enough to review.

And likewise, I don't think the reverse follows, either.

[snipabit]
>> So for me killing ready patches in the end of the process and on weak
>> ground
>> can only make the process worse. The project is shooting itself in the
>> foot,
>> and you cannot complain later that there is not enough reviewers.

That would also be my opinion.

> How would you want it to work otherwise? Merge everything that somebody
> found review worthy? Have everything immediately reviewed by committers?
> Neither of those seem realistic.

True, but the process could probably be streamlined quite a bit. As an
outsider, I do wonder why sometimes long time periods go by without any
action - but when then a deadline comes, the action is taken suddenly
without the involved parties having had the time to respond first (I mean
"again", of course they could have responded earlier. But you know how it
is in a busy world, and with PG being a "side-project").

That is unsatisfactory for the authors (who either rebase patches
needlessly, or find their patch not getting a review because it has
bitrotted again) or reviewers (because they find patches have bitrotted)
and everyone else (because even the simplest features or problem-solving
fixes can take easily a year or two, if they don't get rejected outright).

Maybe an idea would be to send automatic notifications about patches that
need rebasing, or upcoming deadlines like starting commit fests?

In addition, clear rules and well-formulated project goals would help a lot.

Also, the discussion about "needs of the project" vs. the "needs of the
users" [0] should be separate from the "what do we do about the lack of
manpower" discussion.

Because if you argue what you want with the what you have, you usually end
up with nothing in both departments.

Best regards,

Tels

[0]: E.g. how much is it worth to have a clean, pristine state of source
code vs. having some features and fixes for the most pressing items in a
somewhat timely manner? I do think that there should be at least different
levels between PG core and utilities like pgbench.

Re: All Taxi Services need Index Clustered Heap Append

[David Rowley]

On 3 March 2018 at 05:30, Darafei "Komяpa" Praliaskouski  
wrote:
> Our options were:
>
>  - partitioning. Not entirely trivial when your id is uuid. To get visible
> gains, we need to make sure each driver gets their own partition. That would
> leave us with 50 000(+) tables, and rumors say that in that's what is done
> in some bigger taxi service, and relcache then eats up all the RAM and
> system OOMs.

It's a good job someone invented HASH partitioning then.

It would be interesting to hear how your benchmarks go using current
master + the faster partition pruning patchset [1].  Currently, HASH
partitioning does exist in master, just there's no partition pruning
for the non-matching partitions, which is why you need [1].

I think trying with something like 500-1000 partitions might be a good
place to start.

[1] 
https://www.postgresql.org/message-id/0f96dd16-f5d5-7301-4ddf-858d41a6c...@lab.ntt.co.jp

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: PATCH: logical_work_mem and logical streaming of large in-progress transactions

[Tomas Vondra]

On 03/03/2018 06:19 AM, Erik Rijkers wrote:
> On 2018-03-03 01:55, Tomas Vondra wrote:
>> Hi there,
>>
>> attached is an updated patch fixing all the reported issues (a bit more
>> about those below).
> 
> Hi,
> 
> 0007-Track-statistics-for-streaming-spilling.patch  won't apply.  All
> the other patches apply ok.
> 
> patch complaints with:
> 
> patching file doc/src/sgml/monitoring.sgml
> patching file src/backend/catalog/system_views.sql
> Hunk #1 succeeded at 734 (offset 2 lines).
> patching file src/backend/replication/logical/reorderbuffer.c
> patching file src/backend/replication/walsender.c
> patching file src/include/catalog/pg_proc.h
> Hunk #1 FAILED at 2903.
> 1 out of 1 hunk FAILED -- saving rejects to file
> src/include/catalog/pg_proc.h.rej
> patching file src/include/replication/reorderbuffer.h
> patching file src/include/replication/walsender_private.h
> patching file src/test/regress/expected/rules.out
> Hunk #1 succeeded at 1861 (offset 2 lines).
> 
> Attached the produced reject file.
> 

Yeah, that's due to fd1a421fe66 which changed columns in pg_proc.h.
Attached is a rebased patch, fixing this.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


0001-Introduce-logical_work_mem-to-limit-ReorderBuffer.patch.gz
Description: application/gzip


0002-Immediatel-WAL-log-assignments.patch.gz
Description: application/gzip


0003-Issue-individual-invalidations-with-wal_level-logica.patch.gz
Description: application/gzip


0004-Extend-the-output-plugin-API-with-stream-methods.patch.gz
Description: application/gzip


0005-Implement-streaming-mode-in-ReorderBuffer.patch.gz
Description: application/gzip


0006-Add-support-for-streaming-to-built-in-replication.patch.gz
Description: application/gzip


0007-Track-statistics-for-streaming-spilling.patch.gz
Description: application/gzip


0008-BUGFIX-make-sure-subxact-is-marked-as-is_known_as_su.patch.gz
Description: application/gzip


0009-BUGFIX-set-final_lsn-for-subxacts-before-cleanup.patch.gz
Description: application/gzip

Re: TAP test module - PostgresClient

[Peter Eisentraut]

On 3/1/18 23:39, Michael Paquier wrote:
> On Thu, Mar 01, 2018 at 02:27:13AM -0800, Andres Freund wrote:
>> If I understand correctly there's been no progress on this since, and
>> there'd definitely need to be major work to get something we can agree
>> upon. Doesn't seem v11 material. I think we should mark this as returned
>> with feedback.  Arguments against?
> 
> Agreed with your position.  The TAP tests rely on IPC::Run as a pillar
> of its infrastructure.  I think that if we need a base API to do such
> capabilities we ought to prioritize what we can do with it first instead
> of trying to reinvent the wheel as this patch proposes in such a
> complicated way.

I haven't seen any explanation for a problem this is solving.  The
original submission contained a sample test case, by I don't see why
that couldn't be done with the existing infrastructure.

Patch closed for now.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GSOC'17 project introduction: Parallel COPY execution with errors handling

[Pavel Stehule]

2018-03-03 15:02 GMT+01:00 Peter Eisentraut <
peter.eisentr...@2ndquadrant.com>:

> On 3/3/18 00:48, Tom Lane wrote:
> > I don't think that can possibly work.  It would only be safe if, between
> > the thrower and the catcher, there were no other levels of control
> > operating according to the normal error-handling rules.  But input
> > functions certainly cannot assume that they are only called by COPY,
> > so how could they safely throw a "soft error"?
>
> That assumes that throwing a soft error in a context that does not
> handle it specially is not safe.  I'd imagine in such situations the
> soft error just behaves like a normal exception.
>

This soft error solves what? If somebody use fault tolerant COPY, then he
will not be satisfied, when only format errors will be ignored. Any
constraints, maybe trigger errors should be ignored too.

But is true, so some other databases raises soft error on format issues,
and doesn't raise a exception. Isn't better to use some alternative
algorithm like

 under subtransaction try to insert block of 1000 lines. When some fails do
rollback and try to import per block of 100 lines, and try to find wrong
line?

or another way - for this specific case reduce the cost of subtransaction?
This is special case, subtransaction under one command.

Regards

Pavel



>
> --
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: Rewriting the test of pg_upgrade as a TAP test - take two

[Peter Eisentraut]

On 1/26/18 03:00, Michael Paquier wrote:
> As promised on a recent thread, here is a second tentative to switch
> pg_upgrade's test.sh into a TAP infrastructure.

AFAICT, this still has the same problem as the previous take, namely
that adding a TAP test suite to the pg_upgrade subdirectory will end up
with the build farm client running the pg_upgrade tests twice.  What we
likely need here is an update to the build farm client in conjunction
with this.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] GSOC'17 project introduction: Parallel COPY execution with errors handling

[Peter Eisentraut]

On 3/3/18 00:48, Tom Lane wrote:
> I don't think that can possibly work.  It would only be safe if, between
> the thrower and the catcher, there were no other levels of control
> operating according to the normal error-handling rules.  But input
> functions certainly cannot assume that they are only called by COPY,
> so how could they safely throw a "soft error"?

That assumes that throwing a soft error in a context that does not
handle it specially is not safe.  I'd imagine in such situations the
soft error just behaves like a normal exception.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Disabling src/test/[ssl|ldap] when not building with SSL/LDAP support

[Peter Eisentraut]

On 3/2/18 03:23, Michael Paquier wrote:
> That's a better idea.  I have reworked that in 0001.  What do you think?
> This has the same effect as diag(), which shows information directly to
> the screen, and that's the behavior I was looking for.

I ended up using plan skip_all, which seems to be intended for this
purpose, according to the documentation.

> I have concluded about using the most simple name: PG_TEST_EXTRA.  A
> documentation attempt is added as well.  This is unfortunately close to
> EXTRA_TESTS.  It would be tempting to directly reuse EXTRA_TESTS as
> well, however this is used only for the core regression tests now, so a
> separated variable seems adapted to me.

Committed.  Very nice.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Contention preventing locking

[Amit Kapila]

On Thu, Mar 1, 2018 at 1:22 PM, Konstantin Knizhnik
 wrote:
>
> On 28.02.2018 16:32, Amit Kapila wrote:
>>
>> On Mon, Feb 26, 2018 at 8:26 PM, Konstantin Knizhnik
>>  wrote:
>
>
> Yes, but two notices:
> 1. Tuple lock is used inside heap_* functions. But not in EvalPlanQualFetch
> where transaction lock is also used.
> 2. Tuple lock is hold until the end of update, not until commit of the
> transaction. So other transaction can receive conrol before this transaction
> is completed. And contention still takes place.
> Contention is reduced and performance is increased only if locks (either
> tuple lock, either xid lock) are hold until the end of transaction.
> Unfortunately it may lead to deadlock.
>
> My last attempt to reduce contention was to replace shared lock with
> exclusive in XactLockTableWait and removing unlock from this function. So
> only one transaction can get xact lock and will will hold it until the end
> of transaction. Also tuple lock seems to be not needed in this case. It
> shows better performance on pgrw test but on YCSB benchmark with workload A
> (50% of updates) performance was even worser than with vanilla postgres. But
> was is wost of all - there are deadlocks in pgbench tests.
>
>> I think in this whole process backends may need to wait multiple times
>> either on tuple lock or xact lock.  It seems the reason for these
>> waits is that we immediately release the tuple lock (acquired by
>> heap_acquire_tuplock) once the transaction on which we were waiting is
>> finished.  AFAICU, the reason for releasing the tuple lock immediately
>> instead of at end of the transaction is that we don't want to
>> accumulate too many locks as that can lead to the unbounded use of
>> shared memory.  How about if we release the tuple lock at end of the
>> transaction unless the transaction acquires more than a certain
>> threshold (say 10 or 50) of such locks in which case we will fall back
>> to current strategy?
>>
> Certainly, I have tested such version. Unfortunately it doesn't help. Tuple
> lock is using tuple TID. But once transaction has made the update, new
> version of tuple will be produced with different TID and all new
> transactions will see this version, so them will not notice this lock at
> all.
>

Sure, but out of all new transaction again the only one transaction
will allow to update it and among new waiters, only one should get
access to it.  The situation should be better than when all the
waiters attempt to lock and update the tuple with same CTID.


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

idea - custom menu

[Pavel Stehule]

Hi

I am looking on project https://github.com/NikolayS/postgres_dba

Nice idea, and perfect publicity of typical PostgreSQL maintenance task.

We can allow to call some external applications on some key press (not
command) and evaluate their output. mc has F2 for User menu. It can works
well with pdmenu or some similar. Some simple menu can be done on we lines
with ncurses.

It can be substitution of some custom commands.

Comments, notes?

Regards

Pavel

Re: Online enabling of checksums

[Robert Haas]

On Sat, Mar 3, 2018 at 7:32 AM, Robert Haas  wrote:
> On Fri, Mar 2, 2018 at 6:26 PM, Tomas Vondra
>  wrote:
>> Hmmm, OK. So we need to have a valid checksum on a page, disable
>> checksums, set some hint bits on the page (which won't be WAL-logged),
>> enable checksums again and still get a valid checksum even with the new
>> hint bits? That's possible, albeit unlikely.
>
> No, the problem is if - as is much more likely - the checksum is not
> still valid.

Hmm, on second thought ... maybe I didn't think this through carefully
enough.  If the checksum matches on the master by chance, and the page
is the same on the standby, then we're fine, right?  It's a weird
accident, but nothing is actually broken.  The failure scenario is
where the standby has a version of the page with a bad checksum, but
the master has a good checksum.  So for example: checksums disabled,
master modifies the page (which is replicated), master sets some hint
bits (coincidentally making the checksum match), now we try to turn
checksums on and don't re-replicate the page because the checksum
already looks correct.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

[WIP] Document update for Logical Replication security

[Shinoda, Noriyoshi]

Hi, Hackers, 

The attached patch adds the following information to the document on Logical 
Replication.
About the requirement of connection role of Logical Replication, written in 
31.7 of the manual is as follows.
--
The role used for the replication connection must have the REPLICATION 
attribute.
--
However, the Logical Replication connection role also requires the LOGIN 
attribute.
And, for initial snapshots of Logical Replication, the connection role requires 
SELECT privilege on the replication target table, but it is not described in 
the manual.

Regards,

Noriyoshi Shinoda



logical_replication_doc.patch
Description: logical_replication_doc.patch

Re: Desirability of client-side expressions in psql?

[Pavel Stehule]

2018-03-03 11:35 GMT+01:00 Fabien COELHO :

>
> Hello devs,
>
> This is a discussion without actual patch intended for pg12, to be added
> to CF 2018-09. The expected end result is either "returned with feedback",
> meaning proceed to send some big patch(es), or "rejected", meaning the
> project does not want this, no point in submitting something.
>
> Client "psql" has an "\if" which can test a boolean value and has
> ":"-prefixed variables, including special presets such as ":VERSION_NUM"
> and ":SERVER_VERSION_NUM".
>
> The features are already usable because one can do server-side expressions
> (if connected), which is a little cumbersome and ugly but nevertheless
> functional, eg:
>
>   SELECT :VERSION_NUM = :SERVER_VERSION_NUM AS "same_version" \gset
>   \if :same_version
> ...
>
> However, when the "\if" patch was discussed, there was the underlying idea
> to extend psql so as to add client-side expression. That would allow things
> like:
>
>   \let i 
>   \if :VERSION_NUM = :SERVER_VERSION_NUM
> ...
>
> Before eventually starting on this subject with a loose objective of
> targeting 12.0, I would like to ascertain, especially from committers, but
> also from others, that:
>
> (1) the objective is desirable (i.e. avoid ending with "we do not want
> this feature on principle, the cost-benefit balance is not good
> enough").
>
> (2) maybe have a feedback on the proposed changes (not necessarily
> distinct patches, opinions are welcome), which would be to:
> (a) extend pgbench expressions so that they can handle what
> psql can do (eg variable-exists test which are available in psql)
> (b) maybe do some preliminary refactoring (eg create
> "pgbench/expression.c", "pgbench/variable.c")
> (c) move the pgbench expression engine to fe-utils
> (lexer, parser, execution...),
> (d) do some renaming (eg some "feex" prefix for "front-end
> expressions" to the various functions & types?),
> (e) abstract pgbench and psql variables so that they can be used
> transparently by expressions (i.e. some API alignment)
> (f) connect the engine to "psql"
> (g) create a shared documentation about these expressions,
> referenced from both psql and pgbench documentations.
> (h) provide non-regression tests on psql side as well.
>
> The overall transformation would be quite large (about 2000 lines moved
> around) but "quite" simple (it is moving an existing, working and tested
> feature to allow using it elsewhere), not a lot of new code per se.
>

I understand the request of some simple expression evaluation for pgbench
and conditional execution for psql. Because syntax is same, then share code
is really good idea. Lexer, parser, variable processing should be moved to
fe-utils, other implemented on place. We don't need all commands of pgbench
in psql, and we don't need interactive loop and psql commands in pgbench.
But the syntax of input commands is same on both environments, and all on
this level can be shared via some library. Some shared commands can be
implemented in other library, and called from final positions.

Using some simple expressions evaluations is much more simple, then
integration some full functional VM like lua, Python - and still good
enough.

I have not the feedback from psql users about missing strong integrated
language. What is current weak place of psql is tab complete and readline
multiline editor. The pgcli is better - and second hand, everything else is
better in psql.

Regards

Pavel

>
> --
> Fabien.
>
>

Re: Online enabling of checksums

[Robert Haas]

On Fri, Mar 2, 2018 at 6:26 PM, Tomas Vondra
 wrote:
> Hmmm, OK. So we need to have a valid checksum on a page, disable
> checksums, set some hint bits on the page (which won't be WAL-logged),
> enable checksums again and still get a valid checksum even with the new
> hint bits? That's possible, albeit unlikely.

No, the problem is if - as is much more likely - the checksum is not
still valid.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] GSoC 2017: weekly progress reports (week 4) and patch for hash index

[Amit Kapila]

On Fri, Mar 2, 2018 at 9:27 AM, Thomas Munro
 wrote:
> On Fri, Mar 2, 2018 at 3:57 PM, Andres Freund  wrote:
>> Based on this sub-thread this patch's status of 'needs review' doesn't
>> quite seem accurate and 'waiting on author' and then 'returned with
>> feedback' would be more fitting?
>
> I personally think this patch is really close to RFC.  Shubham has
> fulfilled the project requirement, it's a tidy and short patch, it has
> tests.  I think we really just need to verify that the split case
> works correctly.
>
> Hmm.  I notice that this calls PredicateLockPageSplit() after both
> calls to _hash_splitbucket() (the one in _hash_finish_split() and the
> one in _hash_expandtable()) instead of doing it inside that function,
> and that _hash_splitbucket() unlocks bucket_nbuf before returning.
> What if someone else accesses bucket_nbuf between
> LockBuffer(bucket_nbuf, BUFFER_LOCK_UNLOCK) and
> PredicateLockPageSplit()?  Doesn't that mean that another session can
> read a newly created page and miss a predicate lock that is about to
> be transferred to it?
>

Yes.  I think you are primarily worried about if there is an insert on
new bucket from another session as scans will anyway take the
predicate lock, right?

>  If that is indeed a race, could it be fixed by
> calling PredicateLockPageSplit() at the start of _hash_splitbucket()
> instead?
>

Yes, but I think it would be better if we call this once we are sure
that at least one tuple from the old bucket has been transferred
(consider if all tuples in the old bucket are dead).  Apart from this,
I think this patch has missed handling the cases where we scan the
buckets when the split is in progress.  In such cases, we scan both
old and new bucket, so I think we need to ensure that we take
PredicateLock on both the buckets during such scans.

> Could we get a few days to mull over this and Shubham's other patches?
>

I would also like to see this patch going in v11.  So, I can try to
finish the remaining review comments, if Shubham is not able to spare
time and you can help with the review.  I am also okay to review if
anyone else other than me can handle the remaining points.

>  It'd be really great to get some of these into 11.
>

+1.

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

Re: WIP: a way forward on bootstrap data

[John Naylor]

I wrote:

> I'll submit a
> preliminary patch soon to get some of those items out of the way.

I've attached a patch that takes care of these cleanups so they don't
clutter the patch set.

-John Naylor
From d97a8b2e5fa4977e656d1aca7ee7bf1289ecbd40 Mon Sep 17 00:00:00 2001
From: John Naylor 
Date: Sat, 3 Mar 2018 16:56:10 +0700
Subject: [PATCH] Some non-functional cleanups in preparation for the upcoming
 bootstrap data conversion:

Arrange pg_tablespace.h OID symbols so they are immediately after the relevant
DATA() line.

Separate out the pg_attribute logic of genbki.pl into its own function
and skip checking if the data is defined. This both narrows and shortens
the data writing loop of the script.

Correct spelling of some macros.
---
 src/backend/catalog/aclchk.c  |   6 +-
 src/backend/catalog/genbki.pl | 272 ++
 src/include/catalog/pg_init_privs.h   |   2 +-
 src/include/catalog/pg_tablespace.h   |   3 +-
 src/interfaces/ecpg/ecpglib/execute.c |   2 +-
 src/interfaces/ecpg/ecpglib/pg_type.h |   2 +-
 6 files changed, 149 insertions(+), 138 deletions(-)

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 3f2c629..0648539 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -5969,8 +5969,8 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
 			MemSet(nulls, false, sizeof(nulls));
 			MemSet(replace, false, sizeof(replace));
 
-			values[Anum_pg_init_privs_privs - 1] = PointerGetDatum(new_acl);
-			replace[Anum_pg_init_privs_privs - 1] = true;
+			values[Anum_pg_init_privs_initprivs - 1] = PointerGetDatum(new_acl);
+			replace[Anum_pg_init_privs_initprivs - 1] = true;
 
 			oldtuple = heap_modify_tuple(oldtuple, RelationGetDescr(relation),
 		 values, nulls, replace);
@@ -6007,7 +6007,7 @@ recordExtensionInitPrivWorker(Oid objoid, Oid classoid, int objsubid, Acl *new_a
 			values[Anum_pg_init_privs_privtype - 1] =
 CharGetDatum(INITPRIVS_EXTENSION);
 
-			values[Anum_pg_init_privs_privs - 1] = PointerGetDatum(new_acl);
+			values[Anum_pg_init_privs_initprivs - 1] = PointerGetDatum(new_acl);
 
 			tuple = heap_form_tuple(RelationGetDescr(relation), values, nulls);
 
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index ed90a02..8d740c3 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -156,154 +156,85 @@ foreach my $catname (@{ $catalogs->{names} })
 		print $bki "open $catname\n";
 	}
 
-	if (defined $catalog->{data})
+	# For pg_attribute.h, we generate data entries ourselves.
+	# NB: pg_type.h must come before pg_attribute.h in the input list
+	# of catalog names, since we use info from pg_type.h here.
+	if ($catname eq 'pg_attribute')
 	{
+		gen_pg_attribute($schema);
+	}
 
-		# Ordinary catalog with DATA line(s)
-		foreach my $row (@{ $catalog->{data} })
-		{
-
-			# Split line into tokens without interpreting their meaning.
-			my %bki_values;
-			@bki_values{@attnames} =
-			  Catalog::SplitDataLine($row->{bki_values});
-
-			# Perform required substitutions on fields
-			foreach my $column (@$schema)
-			{
-my $attname = $column->{name};
-my $atttype = $column->{type};
-
-# Substitute constant values we acquired above.
-# (It's intentional that this can apply to parts of a field).
-$bki_values{$attname} =~ s/\bPGUID\b/$BOOTSTRAP_SUPERUSERID/g;
-$bki_values{$attname} =~ s/\bPGNSP\b/$PG_CATALOG_NAMESPACE/g;
-
-# Replace regproc columns' values with OIDs.
-# If we don't have a unique value to substitute,
-# just do nothing (regprocin will complain).
-if ($atttype eq 'regproc')
-{
-	my $procoid = $regprocoids{ $bki_values{$attname} };
-	$bki_values{$attname} = $procoid
-	  if defined($procoid) && $procoid ne 'MULTIPLE';
-}
-			}
+	# Ordinary catalog with DATA line(s)
+	foreach my $row (@{ $catalog->{data} })
+	{
 
-			# Save pg_proc oids for use in later regproc substitutions.
-			# This relies on the order we process the files in!
-			if ($catname eq 'pg_proc')
-			{
-if (defined($regprocoids{ $bki_values{proname} }))
-{
-	$regprocoids{ $bki_values{proname} } = 'MULTIPLE';
-}
-else
-{
-	$regprocoids{ $bki_values{proname} } = $row->{oid};
-}
-			}
+		# Split line into tokens without interpreting their meaning.
+		my %bki_values;
+		@bki_values{@attnames} =
+		  Catalog::SplitDataLine($row->{bki_values});
 
-			# Save pg_type info for pg_attribute processing below
-			if ($catname eq 'pg_type')
+		# Perform required substitutions on fields
+		foreach my $column (@$schema)
+		{
+			my $attname = $column->{name};
+			my $atttype = $column->{type};
+
+			# Substitute constant values we acquired above.
+			# (It's intentional that this can apply to parts of a field).
+			$bki_values{$attname} =~ s/\bPGUID\b/$BOOTSTRAP_SUPERUSERID/g;
+			$bki_values{$attname} =~ 

Desirability of client-side expressions in psql?

[Fabien COELHO]

Hello devs,

This is a discussion without actual patch intended for pg12, to be added 
to CF 2018-09. The expected end result is either "returned with feedback", 
meaning proceed to send some big patch(es), or "rejected", meaning the 
project does not want this, no point in submitting something.


Client "psql" has an "\if" which can test a boolean value and has 
":"-prefixed variables, including special presets such as ":VERSION_NUM" 
and ":SERVER_VERSION_NUM".


The features are already usable because one can do server-side expressions 
(if connected), which is a little cumbersome and ugly but nevertheless 
functional, eg:


  SELECT :VERSION_NUM = :SERVER_VERSION_NUM AS "same_version" \gset
  \if :same_version
...

However, when the "\if" patch was discussed, there was the underlying idea 
to extend psql so as to add client-side expression. That would allow 
things like:


  \let i 
  \if :VERSION_NUM = :SERVER_VERSION_NUM
...

Before eventually starting on this subject with a loose objective of 
targeting 12.0, I would like to ascertain, especially from committers, but 
also from others, that:


(1) the objective is desirable (i.e. avoid ending with "we do not want
this feature on principle, the cost-benefit balance is not good
enough").

(2) maybe have a feedback on the proposed changes (not necessarily
distinct patches, opinions are welcome), which would be to:
(a) extend pgbench expressions so that they can handle what
psql can do (eg variable-exists test which are available in psql)
(b) maybe do some preliminary refactoring (eg create
"pgbench/expression.c", "pgbench/variable.c")
(c) move the pgbench expression engine to fe-utils
(lexer, parser, execution...),
(d) do some renaming (eg some "feex" prefix for "front-end
expressions" to the various functions & types?),
(e) abstract pgbench and psql variables so that they can be used
transparently by expressions (i.e. some API alignment)
(f) connect the engine to "psql"
(g) create a shared documentation about these expressions,
referenced from both psql and pgbench documentations.
(h) provide non-regression tests on psql side as well.

The overall transformation would be quite large (about 2000 lines moved 
around) but "quite" simple (it is moving an existing, working and tested 
feature to allow using it elsewhere), not a lot of new code per se.


--
Fabien.

Re: public schema default ACL

[Joe Conway]

On 03/03/2018 01:56 AM, Noah Misch wrote:
> Commit 5770172 ("Document security implications of search_path and the public
> schema.") is largely a workaround for the fact that the boot_val of
> search_path contains "public" while template0 gets "GRANT CREATE, USAGE ON
> SCHEMA public TO PUBLIC".  It's like having world-writable /usr/bin.  The
> security team opted not to change that in released branches, but we thought to
> revisit it later.  I propose, for v11, switching to "GRANT USAGE ON SCHEMA
> public TO PUBLIC" (omit CREATE).  Concerns?

+1. Doing this, or even revoking everything for schema public from
PUBLIC, is already common enough and good practice.

> If we do that alone, databases reaching v11 via dump/reload or pg_upgrade will
> get the new default ACL if they had not changed the ACL of schema public.  If
> they had GRANTed or REVOKEd on schema public, pg_dump will recreate the
> resulting ACL.  This is the standard pg_dump behavior for ACLs on system
> objects.  I think that's okay for the public schema, too, and I like
> preserving that usual rule.  However, if we wanted to minimize upgrade-time
> surprises, we could make pg_dump include GRANT for schema public
> unconditionally.  That way, the default ACL change would apply to new
> databases only.  Does anyone want to argue for that?

What about a pg_dump option to do that and then a big note in the
release notes telling people why they might want to use it?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: STATISTICS retained in CREATE TABLE ... LIKE (INCLUDING ALL)?

[David Rowley]

I've attached a rebased patch which fixes up the conflicts caused by 8b08f7d.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


create_table_like_statistics_v4.patch
Description: Binary data

Re: 2018-03 Commitfest Summary (Andres #1)

[Fabien COELHO]

Hello Andres,


For instance, I used extensively tps throttling, latencies and timeouts
measures when developping and testing the checkpointer sorting & throttling
patch.


That doesn't say that much about proposed feature additions, we didn't
say that feature isn't useful?


Sure.

The point I am trying to make is that adding capabilities to pgbench 
enables new kind of performance tests, and how useful they are cannot be 
foreseen from the starting point. Moreover, the usability increases when 
these capabilities can be combined.


For the latency & timeout measures, my initial point was to check the 
state of postgres for a performance subject I'm interested in, with the 
idea that people put too much emphasis on tps and not enough on latency. I 
knew there were some issues there, but I had no idea how terrible it was 
before I was able to get detailed measures. So the actual process was add 
capabilities (a lot of argumentation because people do not see the 
point...), then use them to collect data, spot a larger than expected 
problem and then try to fix it.



I can stop doing both if the project decides that improving pgbench
capabilities is against its interest.


That's not what we said. There's a difference between "we do not want to
improve pgbench" and "the cost/benefit balance seems to have shifted
over time, and the marginal benefit of proposed features isn't that
high".


I'm probably exagerating a bit for the sake of argument, but I still think 
that the project is over conservative about pgbench feature set, which 
IMHO is "nearly there", but not quite, so as to be useful for running 
actual benchmarks against postgres.


I've been pushing for a consistent set of basic features. Pgbench got 
boolean expressions, but they cannot be used in a conditional (eh, no \if, 
low marginal benefit) and they cannot test anything related to the data 
(no \gset, low marginal benefit). The current result is a half-baked 
inconsistent tool, which is just too bad.



As pgbench patches can stay ready-to-committers for half a dozen CF, I'm not
sure the strain on the committer time is that heavy:-)


That's just plain wrong. Even patches that linger cost time and attention.


Hmmm. A few seconds per month to move it to the next CF? A few seconds per 
month so skip these few "ready" patches while reading the very long list 
cluttered with a hundred "needs review" items anyway?



[...] I'm exactly of the opposite opinion. Submitting things out of 
context, without seeing at least drafts of later patches, is a lot more 
work and doesn't allow to see the big picture.


Hmmm.

The (trivial) big picture is to allow client-side expressions in psql 
(which as a \if:-) by reusing pgbench expression engine, so that one could 
write things like


  \let i :j + 12 * :k

or

  \if :VERSION_NUM < 14

In a psql script. For this purpose, the expression engine must somehow 
support the existing syntax, including testing whether a variable exists, 
hence the small 30-lines (including doc & tests) patch submission.


Obviously I should check first to get at least a committer's agreement 
that the feature is desirable: I have no issue with having a patch 
rejected after a long process because of bad programming and/or bad 
design, but if the thing is bared from the beginning there is no point in 
trying.


So I'm doing things one (slow) step at a time, especially as each time 
I've submitted patches which were doing more than one thing I was asked 
to disentangle features and restructuring.


There's a difference between maintaining a set of patches in a queue, 
nicely split up, and submitting them entirely independently.


Sure. I had a bad experience before on that subject, but I may be 
masochistic enough to try again:-)


--
Fabien.

Re: 2018-03 Commitfest Summary (Andres #1)

[Fabien COELHO]

Hello Peter,


On the "adequate return" point, my opinion is that currently pgbench is just
below the feature set needed to be generally usable, so not improving it is
a self-fullfilling ensurance that it will not be used further. Once the
"right" feature set is reached (for me, at least extracting query output
into variables, having conditionals, possibly a few more functions if some
benches use them), whether it would be actually more widely used by both
developers and users is an open question.


FWIW, I think that pgbench would become a lot more usable if someone
maintained a toolset for managing pgbench. Something similar to Greg
Smith's pgbench-tools project, but with additional features for
instrumenting the server. There would be a lot of value in integrating
it with third party tooling, such as perf and BCC, and in making it
easy for non-experts to run relevant, representative tests.

Things like the rate limiting and alternative distributions were
sorely needed, but there are diminishing returns. It's pretty clear to
me that much of the remaining low hanging fruit is outside of pgbench
itself.


It happens that I might start something on the line of what you are 
suggesting above.


However there is a minimal set of features needed in pgbench itself, 
especially on the scripting side (functions, variables, conditions, error 
handling... which are currently work in progress). I do not think that it 
would be make any sense to re-implement all detailed data collection, load 
throttling, client & thread handling outside pgbench, just because there 
is a missing basic feature such as a particular hash function or a stupid 
\if on the result of a query to implement a simple benchmark.


None of the more recent pgbench enhancements seem to make it easier to 
use.


I agree that "easier to use" is a worthy objective, and that its answer is 
probably partly outside pgbench (although there could be parts inside, eg 
json/CSV/... outputs to help collect performance data for instance).


--
Fabien.

Re: [HACKERS] Another oddity in handling of WCO constraints in postgres_fdw

[Arthur Zakirov]

Hello,

On Wed, Feb 28, 2018 at 05:22:42PM +0900, Etsuro Fujita wrote:
> I rebased the patch over HEAD and revised comments/docs a little bit. Please
> find attached a new version of the patch.

I've reviewed the patch.

The code is good, clear and it is pretty small. There are documentation
fixes and additional regression tests.

Unfortunately the patch is outdated and it needs rebasing. Outdated
files are regression tests files.

After rebasing regression tests they pass.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Re: Parallel Aggregates for string_agg and array_agg

[David Rowley]

I've attached a rebased patch which fixes the conflicts caused by fd1a421.

-- 
 David Rowley   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services


combinefn_for_string_and_array_aggs_v4.patch
Description: Binary data

Re: 2018-03 Commitfest Summary (Andres #1)

[Fabien COELHO]

Hello,

[...] The consequence of this appears to be that we should integrate 
everything that anybody decided worthy enough to review. That just 
doesn't make sense, we can't maintain the project that way, nor will the 
design be even remotely coherent.


Sure. The pgbench capabilities we are discussing are consistent, though, 
which is why I'm arguing.



Sure it's a balancing act, but nobody denies that.


Yep. I'm arguing on the balance.


So for me killing ready patches in the end of the process and on weak ground
can only make the process worse. The project is shooting itself in the foot,
and you cannot complain later that there is not enough reviewers.


How would you want it to work otherwise? Merge everything that somebody
found review worthy?


No, but I think that there should be stronger technical/design/... reject 
arguments than the over-conservatism shown on some patches.


I'll take as an example the pgbench hash functions patch about which you 
seemed to show some reservation: it adds a few hash functions, a necessary 
feature to reproduce a YCSB load (Yahoo cloud service benchmark), together 
with zipfian distributions (already accepted, thanks).


Why would committers prevent pgbench to be used to reproduce this kind of 
load? The point of pgbench is to be able to test different kind of 
loads/scenarii... Accepting such features, if the implementation is good 
enough, should be no brainer, and alas it is not.


Have everything immediately reviewed by committers? Neither of those 
seem realistic.


Sure. I'm aiming at an ambitious "eventually":-)

Note that I'd wish that at least the ready-for-committer bug-fixes would 
be processed by committers when tagged as ready in a timely fashion (say 
under 1 CF), which is not currently the case.


--
Fabien.

Re: WIP: a way forward on bootstrap data

[John Naylor]

Thanks for taking a look.

On 3/3/18, Tom Lane  wrote:
> John Naylor  writes:
>> Version 8, rebased against 76b6aa41f41d.
>
> I took a preliminary look through this, without yet attempting to execute
> the script against HEAD.  I have a few thoughts:
>
> * I'm inclined not to commit the conversion scripts to the repo.  I doubt
> there are third parties out there with a need for them, and if they do
> need them they can get 'em out of this thread in the mailing list
> archives.  (If anyone has a different opinion about that, speak up!)

If no one feels strongly otherwise, I'll just attach the conversion
script along with the other patches for next version. To be clear, the
rewrite script is intended be committed, both to enforce formatting
and as a springboard for bulk editing. Now, whether that belongs in
/src/include/catalog or /src/tools is debatable.

> * I'm a little disturbed by the fact that 0002 has to "re-doublequote
> values that are macros expanded by initdb.c".  I see that there are only
> a small number of affected places, so maybe it's not really worth working
> harder, but I worry that something might get missed.  Is there any way to
> include this consideration in the automated conversion, or at least to
> verify that we found all the places to quote?  Or, seeing that 0004 seems
> to be introducing some quoting-related hacks to genbki.pl anyway, maybe
> we could take care of the issue there?

The quoting hacks are really to keep the postgres.bki diff as small as
possible (attached). The simplest and most air-tight way to address
your concern would be to double-quote everything when writing the bki
file. That could be done last as a follow-up.

> * I notice you have a few "preliminary cleanup" changes here and there
> in the series, such as fixing the inconsistent spelling of
> Anum_pg_init_privs_initprivs.  These could be applied before we start
> the main conversion process, and I'm inclined to do that since we could
> get 'em out of the way early.  Ideally, the main conversion would only
> touch the header files and related scripts/makefiles.
...
> * In 0003, I'd recommend leaving the re-indentation to happen in the next
> perltidy run (assuming perltidy would fix that, which I think is true but
> I might be wrong).  It's just creating more review work to do it here.
> In any case, the patch summary line is pretty misleading since it's
> *not* just reindenting, but also refactoring genbki.pl.  (BTW, if that
> refactoring would work on the script as it is, maybe that's another
> thing we could do early?  The more we can do before "flag day", the
> better IMO.)

I tested perltidy 20090616 and it handles it fine. I'll submit a
preliminary patch soon to get some of those items out of the way.

> * In 0006, I'm not very pleased with the introduction of
> "Makefile.headers".  I'd keep those macros where they are in
> catalog/Makefile.  backend/Makefile doesn't need to know about that,
> especially since it's doing an unconditional invocation of
> catalog/Makefile anyway.  It could just do something like
>
> submake-schemapg:
>   $(MAKE) -C catalog generated-headers
>
> and leave it to catalog/Makefile to know what needs to happen for
> both schemapg.h and the other generated files.

I wasn't happy with it either, but I couldn't get it to build
otherwise. The sticking point was the symlinks in
$(builddir)/src/include/catalog.  $(MAKE) -C catalog doesn't handle
that. The makefile in /src/common relies on the backend makefile to
know what to invoke for a given header. IIRC, relpath.c includes
pg_tablespace.h, which now requires pg_tablespace_d.h to be built.

Perhaps /src/common/Makefile could invoke the catalog makefile
directly, and the pg_*_d.h headers could be written to
$(builddir)/src/include/catalog directly? I'll hack on it some more.

> Overall, though, this is looking pretty promising.
>
>   regards, tom lane
>

Glad to hear it.

-John Naylor
--- postgres.bki.orig   2018-02-25 15:58:50.082261557 +0700
+++ postgres.bki.noquotes   2018-02-25 16:12:39.967498748 +0700
@@ -5651,9 +5651,9 @@
  lanacl = aclitem[]
  )
 open pg_language
-insert OID = 12 ( "internal" 10 f f 0 0 2246 _null_ )
-insert OID = 13 ( "c" 10 f f 0 0 2247 _null_ )
-insert OID = 14 ( "sql" 10 f t 0 0 2248 _null_ )
+insert OID = 12 ( internal 10 f f 0 0 2246 _null_ )
+insert OID = 13 ( c 10 f f 0 0 2247 _null_ )
+insert OID = 14 ( sql 10 f t 0 0 2248 _null_ )
 close pg_language
 create pg_largeobject_metadata 2995
  (
@@ -5753,8 +5753,8 @@
 insert ( 2798 n 0 2796 - 2796 - - - - - f f r r 2799 27 0 0 0 _null_ _null_ )
 insert ( 3527 n 0 3524 - 3524 - - - - - f f r r 3518 3500 0 0 0 _null_ _null_ )
 insert ( 3565 n 0 3563 - 3563 - - - - - f f r r 1203 869 0 0 0 _null_ _null_ )
-insert ( 2147 n 0 2804 - 463 - - 2804 3547 - f f r r 0 20 0 20 0 "0" "0" )
-insert ( 2803 n 0 1219 - 463 - - 1219 3546 - f f r r 0 20 0 20 0 "0" "0" )
+insert ( 2147 n 0 2804 - 463 - -