Re: [HACKERS] Notice lock waits

[Haribabu Kommi]

On Sat, Aug 6, 2016 at 3:00 AM, Jeff Janes  wrote:

> One time too many, I ran some minor change using psql on a production
> server and was wondering why it was taking so much longer than it did
> on the test server.  Only to discover, after messing around with
> opening new windows and running queries against pg_stat_activity and
> pg_locks and so on, that it was waiting for a lock.
>
> So I created a new guc, notice_lock_waits, which acts like
> log_lock_waits but sends the message as NOTICE so it will show up on
> interactive connections like psql.
>
> I turn it on in my .psqlrc, as it doesn't make much sense for me to
> turn it on in non-interactive sessions.
>
> A general facility for promoting selected LOG messages to NOTICE would
> be nice, but I don't know how to design or implement that.  This is
> much easier, and I find it quite useful.
>
> I have it PGC_SUSET because it does send some tiny amount of
> information about the blocking process (the PID) to the blocked
> process.  That is probably too paranoid, because the PID can be seen
> by anyone in the pg_locks table anyway.
>
> Do you think this is useful and generally implemented in the correct
> way?  If so, I'll try to write some sgml documentation for it.
>


Providing the details of lock wait to the client is good. I fell this
message
is useful for the cases where User/administrator is trying to perform some
SQL operations.

I also feel that, adding a GUC variable for these logs to show it to user
may not be good. Changing the existing GUC may be better.

I am not sure whether it really beneficial in providing all LOG as NOTICE
messages with a generic framework, it may be unnecessary overhead
for some users, I am not 100% sure.

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Showing parallel status in \df+

[Rushabh Lathia]

On Thu, Sep 29, 2016 at 12:07 AM, Pavel Stehule 
wrote:

> Hi
>
> 2016-09-28 18:57 GMT+02:00 Tom Lane :
>
>> Pavel Stehule  writes:
>> > 2016-09-28 16:03 GMT+02:00 Tom Lane :
>> >> I propose to push my current patch (ie, move PL function
>> >> source code to \df+ footers), and we can use it in HEAD for awhile
>> >> and see what we think.  We can alway improve or revert it later.
>>
>> > I had some objection to format of source code - it should be full source
>> > code, not just header and body.
>>
>> That would be redundant with stuff that's in the main part of the \df
>> display.  I really don't need to see the argument types twice, for
>> instance.
>>
>
> I am sorry, I disagree. Proposed form is hard readable. Is not possible to
> simply copy/paste.
>
> I cannot to imagine any use case for proposed format.
>
>
I just did testing on Tom's patch - which show pl source code as a footer
(show-pl-source-code-as-a-footer.patch). I am sorry, but I agree with Paval,
its is hard readable - and its not adding any simplification on what we have
now.

Pavel Stehule  writes:
> We are in cycle because prosrc field is used for two independent features
-
> and then it can be hard to find a agreement.

> I thought pretty much everyone was on board with the idea of keeping
> prosrc in \df+ for internal/C-language functions (and then probably
> renaming the column, since it isn't actually source code in that case).
>The argument is over what to do for PL functions, which is only one use
> case not two

Thinking more, I am good for keeping prosrc in \df+ for internal/C-language
functions (with changed column name). and then \sf will be used to
get the source code for PL, SQL, language.


Regards
>
> Pavel
>
>
>>
>> regards, tom lane
>>
>
>


-- 
Rushabh Lathia
www.EnterpriseDB.com

Re: [HACKERS] IF (NOT) EXISTS in psql-completion

[Kyotaro HORIGUCHI]

Hello,

At Wed, 28 Sep 2016 12:49:25 -0400, Robert Haas  wrote 
in 
> This patch hasn't been updated in over a week and we're just about out
> of time for this CommitFest, so I've marked it "Returned with
> Feedback" for now.  If it gets updated, it can be resubmitted for the
> next CommitFest.

Thanks, I will do it.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] asynchronous execution

[Kyotaro HORIGUCHI]

Hello, thank you for the comment.

At Wed, 28 Sep 2016 10:00:08 +0530, Amit Khandekar  
wrote in 
> On 24 September 2016 at 06:39, Robert Haas  wrote:
> > Since Kyotaro Horiguchi found that my previous design had a
> > system-wide performance impact due to the ExecProcNode changes, I
> > decided to take a different approach here: I created an async
> > infrastructure where both the requestor and the requestee have to be
> > specifically modified to support parallelism, and then modified Append
> > and ForeignScan to cooperate using the new interface.  Hopefully that
> > means that anything other than those two nodes will suffer no
> > performance impact.  Of course, it might have other problems
> 
> I see that the reason why you re-designed the asynchronous execution
> implementation is because the earlier implementation showed
> performance degradation in local sequential and local parallel scans.
> But I checked that the ExecProcNode() changes were not that
> significant as to cause the degradation.

The basic thought is that we don't allow degradation of as small
as around one percent for simple cases in exchange for this
feature (or similar ones).

Very simple case of SeqScan runs through a very short path, on
where prediction failure penalties of CPU by few branches results
in visible impact. I avoided that by using likely/unlikly but
more fundamental measure is preferable.

> It will not call ExecAsyncWaitForNode() unless that node
> supports asynchronism.

That's true, but it takes a certain amount of CPU cycle to decide
call it or not. The small bit of time is the issue in focus now.

> Do you feel there is anywhere else in
> the implementation that is really causing this degrade ? That
> previous implementation has some issues, but they seemed
> solvable. We could resolve the plan state recursion issue by
> explicitly making sure the same plan state does not get called
> again while it is already executing.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Sample configuration files

[Vik Fearing]

On 09/29/2016 05:55 AM, Michael Paquier wrote:
> On Thu, Sep 29, 2016 at 2:25 AM, Robert Haas  wrote:
>> So, anyone else have an opinion, pro or con?
> 
> Going through this thread, I'd vote -1. This is a documentation effort
> mainly, and installing those files has zero effect if they are not
> loaded via include_if_exists or include in postgresql.conf.

Just the other day, I needed this patch yet again but had to go look up
the documentation instead.

I wonder if it would be a good idea to have a postgresql.conf.d
directory that postgresql.conf would include_dir by default.  These
could then live in there and all I would have had to do is uncomment the
values I wanted.

This patch doesn't do that, of course, but I could easily write a patch
that does.  Would that go over better with the -1ers?
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support


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

Re: [HACKERS] New SQL counter statistics view (pg_stat_sql)

[Haribabu Kommi]

On Thu, Sep 22, 2016 at 3:05 AM, Alvaro Herrera 
wrote:

> Peter Eisentraut wrote:
>
> > How about having the tag not be a column name but a row entry.  So you'd
> > do something like
> >
> > SELECT * FROM pg_stat_sql WHERE tag = 'ALTER VIEW';
> >
> > That way, we don't have to keep updating (and re-debating) this when new
> > command types or subtypes are added.  And queries written for future
> > versions will not fail when run against old servers.
>
> Yeah, good idea.
>

Yes, Having it as a row entry is good.



> Let's also discuss the interface from the stats collector.  Currently we
> have some 20 new SQL functions, all alike, each loading the whole data
> and returning a single counter, and then the view invokes each function
> separately.  That doesn't seem great to me.  How about having a single C
> function that returns the whole thing as a SRF instead, and the view is
> just a single function invocation -- something like pg_lock_status
> filling pg_locks in one go.
>
> Another consideration is that the present patch lumps together all ALTER
> cases in a single counter.  This isn't great, but at the same time we
> don't want to bloat the stat files by having hundreds of counters per
> database, do we?


Currently, The SQL stats is a fixed size counter to track the all the ALTER
cases as single counter. So while sending the stats from the backend to
stats collector at the end of the transaction, the cost is same, because of
it's fixed size. This approach adds overhead to send and read the stats
is minimal.

With the following approach, I feel it is possible to support the counter at
command tag level.

Add a Global and local Hash to keep track of the counters by using the
command tag as the key, this hash table increases dynamically whenever
a new type of SQL command gets executed. The Local Hash data is passed
to stats collector whenever the transaction gets committed.

The problem I am thinking is that, Sending data from Hash and populating
the Hash from stats file for all the command tags adds some overhead.


Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] asynchronous execution

[Kyotaro HORIGUCHI]

Sorry for delayed response, I'll have enough time from now and
address this.

At Fri, 23 Sep 2016 21:09:03 -0400, Robert Haas  wrote 
in 
> Well, I promised to post this, so here it is.  It's not really working
> all that well at this point, and it's definitely not doing anything
> that interesting, but you can see the outline of what I have in mind.
> Since Kyotaro Horiguchi found that my previous design had a
> system-wide performance impact due to the ExecProcNode changes, I
> decided to take a different approach here: I created an async
> infrastructure where both the requestor and the requestee have to be
> specifically modified to support parallelism, and then modified Append
> and ForeignScan to cooperate using the new interface.  Hopefully that
> means that anything other than those two nodes will suffer no
> performance impact.  Of course, it might have other problems
> 
> Some notes:
> 
> - EvalPlanQual rechecks are broken.
> - EXPLAIN ANALYZE instrumentation is broken.
> - ExecReScanAppend is broken, because the async stuff needs some way
> of canceling an async request and I didn't invent anything like that
> yet.
> - The postgres_fdw changes pretend to be async but aren't actually.
> It's just a demo of (part of) the interface at this point.
> - The postgres_fdw changes also report all pg-fdw paths as
> async-capable, but actually the direct-modify ones aren't, so the
> regression tests fail.
> - Errors in the executor can leak the WaitEventSet.  Probably we need
> to modify ResourceOwners to be able to own WaitEventSets.
> - There are probably other bugs, too.
> 
> Whee!
> 
> Note that I've tried to solve the re-entrancy problems by (1) putting
> all of the event loop's state inside the EState rather than in local
> variables and (2) having the function that is called to report arrival
> of a result be thoroughly different than the function that is used to
> return a tuple to a synchronous caller.
> 
> Comments welcome, if you're feeling brave enough to look at anything
> this half-baked.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Thu, Sep 29, 2016 at 11:12:11AM +1300, Thomas Munro wrote:
> On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro
>  wrote:
> > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro
> >  wrote:
> >>
> >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter  wrote:
> >> >
> >> > [training_wheels_004.patch]
> >>
> >> [review]
> 
> Ping.

Please find attached the next revision.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
diff --git a/contrib/Makefile b/contrib/Makefile
index 25263c0..4bd456f 100644
--- a/contrib/Makefile
+++ b/contrib/Makefile
@@ -40,6 +40,7 @@ SUBDIRS = \
pgstattuple \
pg_visibility   \
postgres_fdw\
+   require_where   \
seg \
spi \
tablefunc   \
diff --git a/contrib/require_where/Makefile b/contrib/require_where/Makefile
new file mode 100644
index 000..0cf3663
--- /dev/null
+++ b/contrib/require_where/Makefile
@@ -0,0 +1,17 @@
+# contrib/require_where/Makefile
+
+MODULE_big = require_where
+OBJS = require_where.o
+
+PGFILEDESC = 'require_where - require DELETE and/or UPDATE to have a WHERE 
clause'
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS = $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = contrib/require_where
+top_builddir = ../..
+include $(top_builddir)/src/Makefile.global
+include $(top_builddir)/contrib/contrib-global.mk
+endif
diff --git a/contrib/require_where/data/test_require_where.data 
b/contrib/require_where/data/test_require_where.data
new file mode 100644
index 000..d4a29d8
--- /dev/null
+++ b/contrib/require_where/data/test_require_where.data
@@ -0,0 +1,16 @@
+Four
+score
+and
+seven
+years
+ago
+our
+fathers
+brought
+forth
+on
+this
+continent
+a
+new
+nation
diff --git a/contrib/require_where/expected/require_where.out 
b/contrib/require_where/expected/require_where.out
new file mode 100644
index 000..0876e13
--- /dev/null
+++ b/contrib/require_where/expected/require_where.out
@@ -0,0 +1,12 @@
+LOAD
+CREATE TABLE
+COPY 16
+UPDATE 16
+SET
+psql:sql/require_where.sql:17: ERROR:  UPDATE requires a WHERE clause
+HINT:  To update all rows, use "WHERE true" or similar.
+SET
+psql:sql/require_where.sql:21: ERROR:  DELETE requires a WHERE clause
+HINT:  To delete all rows, use "WHERE true" or similar.
+SET
+DELETE 16
diff --git a/contrib/require_where/require_where.c 
b/contrib/require_where/require_where.c
new file mode 100644
index 000..27cbc25
--- /dev/null
+++ b/contrib/require_where/require_where.c
@@ -0,0 +1,92 @@
+/*
+ * --
+ *
+ * require_where.c
+ *
+ * Copyright (C) 2016, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ * contrib/require_where/require_where.c
+ *
+ * --
+ */
+#include "postgres.h"
+
+#include "fmgr.h"
+
+#include "parser/analyze.h"
+
+#include "utils/elog.h"
+#include "utils/guc.h"
+
+PG_MODULE_MAGIC;
+
+void   _PG_init(void);
+void   _PG_fini(void);
+
+static post_parse_analyze_hook_type original_post_parse_analyze_hook = 
NULL;
+static boolrequire_where_delete = false;
+static boolrequire_where_update = false;
+
+static void
+require_where_check(ParseState *pstate, Query *query)
+{
+
+   if (require_where_delete && query->commandType == CMD_DELETE)
+   {
+   Assert(query->jointree != NULL);
+   if (query->jointree->quals == NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("DELETE requires a WHERE clause 
when require_where.delete is set to on"),
+errhint("To delete all rows, use 
\"WHERE true\" or similar.")));
+   }
+
+   if (require_where_update && query->commandType == CMD_UPDATE)
+   {
+   Assert(query->jointree != NULL);
+   if (query->jointree->quals == NULL)
+   ereport(ERROR,
+   (errcode(ERRCODE_SYNTAX_ERROR),
+errmsg("UPDATE requires a WHERE clause 
when require_where.delete is set to on"),
+errhint("To update all rows, use 
\"WHERE true\" or similar.")));
+   }
+
+   if (original_post_parse_analyze_hook != NULL)
+   (*original_post_parse_analyze_hook) (pstate, query);
+}
+
+void
+_PG_init(void)
+{
+   DefineCustomBoolVariable("require_where.delete",
+"Require every DELETE 
statement to have a WHERE clause.",
+  

Re: [HACKERS] Handling dropped attributes in pglogical_proto

[Petr Jelinek]

On 29/09/16 05:33, Michael Paquier wrote:
> On Wed, Sep 28, 2016 at 11:25 PM, Konstantin Knizhnik
>  wrote:
>> But if table was just altered and some attribute was removed from the table,
>> then rel->natts can be greater than natts.
> 
> This is part of pglogical, so you may want to reply on the dedicated
> thread or send directly a patch to them. By the way, this code path
> may need to care as well about attisdropped. It is never good not to
> check for it.
> 

Agreed this does not belong to hackers and should reported on github.

But just as note the rel variable is not Relation, it's
PGLogicalRelation which gets populated by relation message from the
upstream so if you observe the behavior mentioned in the original email,
you are probably doing something unexpected there (Konstantin is not
using vanilla pglogical). Also the attmap should never contain
attisdropped attributes.

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


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

Re: [HACKERS] Sample configuration files

[Michael Paquier]

On Thu, Sep 29, 2016 at 2:25 AM, Robert Haas  wrote:
> So, anyone else have an opinion, pro or con?

Going through this thread, I'd vote -1. This is a documentation effort
mainly, and installing those files has zero effect if they are not
loaded via include_if_exists or include in postgresql.conf.
-- 
Michael


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

Re: [HACKERS] WAL consistency check facility

[Michael Paquier]

On Fri, Sep 16, 2016 at 10:36 PM, Michael Paquier
 wrote:
> On Fri, Sep 16, 2016 at 10:30 PM, Robert Haas  wrote:
>> I don't think you have the right to tell Kuntal that he has to move
>> the patch to the next CommitFest because there are unspecified things
>> about the current version you don't like.  If you don't have time to
>> review further, that's your call, but he can leave the patch as Needs
>> Review and see if someone else has time.
>
> No complain from here if done this way. I don't mean any offense :)

Seeing nothing happening, I have moved the patch to next CF as there
is a new version, but no reviews for it.
-- 
Michael


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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[Michael Paquier]

On Wed, Sep 28, 2016 at 8:55 PM, Michael Paquier
 wrote:
>> Our b64_encode routine does use whitespace, so we can't use it as is for
>> SCRAM. As the patch stands, we might never output anything long enough to
>> create linefeeds, but let's be tidy. The base64 implementation is about 100
>> lines of code, so perhaps we should just leave src/backend/utils/encode.c
>> alone, and make a new copy of the base64 routines in src/common.
>
> OK, I'll refresh that tomorrow with the rest. Thanks for the commit to
> extend password_encryption.

OK, so after more chatting with Heikki, here is a list of TODO items
and a summary of the state of things:
- base64 encoding routines should drop whitespace (' ', \r, \t), and
it would be better to just copy those from the backend's encode.c to
src/common/. No need to move escape and binary things, nor touch
backend's base64 routines.
- No need to move sha1.c to src/common/. Better to just get sha2.c
into src/common/ as we aim at SCRAM-SHA-256.
- random() called in the client is no good. We need something better here.
- The error handling needs to be reworked and should follow the
protocol presented by RFC5802, by sending back e= messages. This needs
a bit of work, not much I think though as the infra is in place in the
core patch.
- Let's discard the md5-or-scram optional thing in pg_hba.conf. This
complicates the error handling protocol.

I am marking this patch as returned with feedback for current CF and
will post a new set soon, moving it to the next CF once I have the new
set of patches ready for posting.
-- 
Michael


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

Re: [HACKERS] Handling dropped attributes in pglogical_proto

[Michael Paquier]

On Wed, Sep 28, 2016 at 11:25 PM, Konstantin Knizhnik
 wrote:
> But if table was just altered and some attribute was removed from the table,
> then rel->natts can be greater than natts.

This is part of pglogical, so you may want to reply on the dedicated
thread or send directly a patch to them. By the way, this code path
may need to care as well about attisdropped. It is never good not to
check for it.
-- 
Michael


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

Re: [HACKERS] Add support for restrictive RLS policies

[Craig Ringer]

On 27 September 2016 at 15:15, Jeevan Chalke
 wrote:
> Hello Stephen,
>
> On Tue, Sep 27, 2016 at 12:57 AM, Stephen Frost  wrote:
>>
>> Jeevan,
>>
>> * Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
>> > I have started reviewing this patch and here are couple of points I have
>> > observed so far:
>> >
>> > 1. Patch applies cleanly
>> > 2. make / make install / initdb all good.
>> > 3. make check (regression) FAILED. (Attached diff file for reference).
>>
>> I've re-based my patch on top of current head and still don't see the
>> failures which you are getting during the regression tests.  Is it
>> possible you were doing the tests without a full rebuild of the source
>> tree..?
>>
>> Can you provide details of your build/test environment and the full
>> regression before and after output?
>
>
> I still get same failures with latest sources and with new patch. Here are
> few details of my setup. Let me know if I missed any.
>
> $ uname -a
> Linux centos7 3.10.0-327.28.3.el7.x86_64 #1 SMP Thu Aug 18 19:05:49 UTC 2016
> x86_64 x86_64 x86_64 GNU/Linux
>
> HEAD at
> commit 51c3e9fade76c12e4aa37bffdf800bbf74fb3fb1
>
> configure switches:
> ./configure --with-openssl --with-tcl --with-perl --with-python
> --with-ossp-uuid --with-ldap --with-pam --with-zlib --with-pgport=5432
> --enable-depend --enable-debug --enable-cassert --prefix=`pwd`/install
> CFLAGS="-g -O0"

I suggest:

git reset --hard
git clean -fdx
ccache -C

then re-apply patch and re-check.

I've had a couple of issues recently caused by ccache doing something
funky :( but haven't been able to track it down yet.


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


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

Re: [HACKERS] [GENERAL] C++ port of Postgres

[Thomas Munro]

On Mon, Sep 26, 2016 at 10:57 AM, Thomas Munro
 wrote:
> On Thu, Sep 1, 2016 at 1:41 AM, Peter Eisentraut
>  wrote:
>>
>> [trimmed cc list because of big attachments]
>>
>> On 8/16/16 4:22 PM, Jim Nasby wrote:
>> > Joy, do you have an idea what a *minimally invasive* patch for C++
>> > support would look like? That's certainly the first step here.
>>
>> I developed a minimally invasive patch for C++ support a few years ago
>> shortly after I wrote that blog post.  Since there appears to have been
>> some interest here now, I have updated that and split it up into logical
>> chunks.
>>
>> So here you go.
>
>
> I looked at a random selection of these patches this morning.

And this morning I looked at the rest of them.

> 0004-Fix-LDFLAGS-test-for-C.patch

Makes sense.

> 0005-Add-test-for-Wmissing-prototypes.patch

This does seem to follow the example of how we test for support for
other warning flags.

> 0006-Remove-unnecessary-prototypes.patch

Looks OK.

> 0007-Fix-incorrect-type-cast.patch

  /* array of check flags, reported to consistentFn */
- bool   *entryRes;
+ GinTernaryValue *entryRes;

Right.  That would be pretty dodgy even in C if we ever use stdbool.h,
because sizeof(_Bool) is implementation defined.  The
interchangeability relies on bool and GinTernaryValue both being
typedefs for 'char'.  (Not to mention the dangerous contradictions
possible with bools obtained that way: 'b == false || b == true' can
be false, which I guess has been thought about already and is off
topic here.)

I wonder if the following bit of gin.h should be more nuanced: maybe
it's OK to convert between bool and GinTernaryValue, but it's
definitely not OK to cast between pointers types?  Or maybe we should
have a function/macro to convert between the types explicitly and not
encourage people to consider them convertible.

  /*
   * A ternary value used by tri-consistent functions.
   *
   * For convenience, this is compatible with booleans. A boolean can be
   * safely cast to a GinTernaryValue.
   */
  typedef char GinTernaryValue;

> 0008-Add-necessary-type-cast.patch

Maybe instead of this:

- gcv.check = check;
+ gcv.check = (GinTernaryValue *) check;

... it would be better to do this?

-bool   *check = (bool *) PG_GETARG_POINTER(0);
+GinTernaryValue *check = (GinTernaryValue *) PG_GETARG_POINTER(0);

> 0009-Rename-some-typedefs-to-avoid-name-conflicts.patch

I don't know if it's a relevant precedent or not, but I noticed that
fdwapi.h, amapi.h and tsmapi.h used the convention that function
pointer types are named XXX_function, and then the members of a struct
behaving as a kind of vtable are named XXX.

> 0010-Reorder-some-things.patch

> Forward declarations of static variables are not possible in C++, so
> move the full definition before its use.

Right.

> 0011-Add-missing-fields-in-struct-initializations.patch

I don't undestand why this is necessary, unless you're explicitly
choosing to enable a warning like missing-field-initializers for C++
but not for C.  Implicit zero-initialisation of trailing missing
initialisers is a feature, not a bug.  Also I noticed that 0013 (or a
proper solution to the keyword collision problem) is needed before
this one.

> 0012-Separate-enum-from-struct.patch

Right.

> 0013-Avoid-C-key-words.patch

> This is not a long-term solution, because C header files that are
> included somewhere might have C++ awareness and will break if the key
> word is defined away.  But this shows the list of words that would have
> to be renamed around the code.

Right, let's rename them all directly.

> 0015-Fix-function-prototypes-for-C.patch

I wonder if (perhaps in some later later patch) walkers should take
const pointers and mutators non-const.  That may require propagating
constness around some more places.

> 0017-Don-t-define-bool-in-C.patch

Check.

> 0018-Change-TimeoutId-from-enum-to-integer.patch

This works, but I feel like we're losing something valuable if we
convert all our enums to ints just because some tiny bit of code
somewhere wants to loop over them.  Maybe we should we keep enums like
this, and do the necessary casting in the small number of places that
do int-like-stuff with them?  Like so:

diff --git a/src/backend/utils/misc/timeout.c b/src/backend/utils/misc/timeout.c
index 7171a7c..cc5b2c4 100644
--- a/src/backend/utils/misc/timeout.c
+++ b/src/backend/utils/misc/timeout.c
@@ -348,7 +348,7 @@ InitializeTimeouts(void)

for (i = 0; i < MAX_TIMEOUTS; i++)
{
-   all_timeouts[i].index = i;
+   all_timeouts[i].index = (TimeoutId) i;
all_timeouts[i].indicator = false;
all_timeouts[i].timeout_handler = NULL;
all_timeouts[i].start_time = 0;
@@ -379,7 +379,8 @@ RegisterTimeout(TimeoutId id, timeout_handler_proc handler)
if (id >= USER_TIMEOUT)
{
/* Allocate a user-defined timeout reason */
-   for (id = USER_TIMEOUT; id < MAX_TIMEOU

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Tom Lane]

Peter Eisentraut  writes:
> On 9/28/16 6:13 PM, Robert Haas wrote:
>> Christoph/Debian:
>> log_line_prefix = '%t [%p-%l] %q%u@%d '
>> Peter:
>> log_line_prefix = '%t [%p]: [%l] %qapp=%a '

> ...
> I don't know why it wants that "-1" there, and I'm actually not sure
> what the point of %l is in practice.  Those are separate issues that are
> having their own lively discussions at times.  I could drop the [%l]
> from my proposal if that causes concerns.

+1 for dropping %l --- seems to me that its main result is to add useless
bytes to the log.  Surely if you need to match up lines from the same
process, that's not that hard as long as %p is in there.

I'd also vote for dropping "app=" out of the regression test version;
again, that seems like basically dead weight.

regards, tom lane


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

Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[Michael Paquier]

On Thu, Sep 29, 2016 at 7:45 AM, David Steele  wrote:
> OK, I've done functional testing and this patch seems to work as
> specified (including the caveat noted above).  Some comments:

Thanks!

> * [PATCH 1/3] hs-checkpoints-v12-1
>
> +++ b/src/backend/access/transam/xlog.c
> +* Taking a lock is as well necessary to prevent potential torn reads
> +* on some platforms.
>
> How about, "Taking a lock is also necessary..."
>
> +   LWLockAcquire(&WALInsertLocks[i].l.lock, LW_EXCLUSIVE);
>
> That's a lot of exclusive locks and that would seem to have performance
> implications.  It seems to me this is going to be a hard one to
> benchmark because the regression (if any) would only be seen under heavy
> load on a very large system.
>
> In general I agree with the other comments that this could end up being
> a problem.  On the other hand, since the additional locks are only taken
> at checkpoint or archive_timeout it may not be that big a deal.

Yes, I did some tests on my laptop a couple of months back, that has 4
cores. After reducing NUM_XLOGINSERT_LOCKS from 8 to 4 to increase
contention and performing a bunch of INSERT using 4 clients on 4
different relations I could not catch a difference.. Autovacuum was
disabled to eliminate any noise. I tried checkpoint_segments at 30s to
see its effects, as well as larger values to see the impact with the
standby snapshot taken by the bgwriter. Other thoughts are welcome.

> +++ b/src/backend/access/transam/xloginsert.c * Should this record
> include the replication origin if one is set up?
>
> Outdated comment from XLogIncludeOrigin().

Fixed. I added as well some comments on top of XLogSetFlags to mention
what are the flags that can be used. I didn't think that it was
necessary to add an assertion here. Also, I noticed that the comment
on top of XLogInsertRecord mentioned those flags but was incorrect.

> * [PATCH 2/3] hs-checkpoints-v12-2
>
> +++ b/src/backend/postmaster/checkpointer.c
> +   /* OK, it's time to switch */
> +   elog(LOG, "Request XLog Switch");
>
> LOG level seems a bit much here, perhaps DEBUG1?

That's from Horiguchi-san's patch, and those would be definitely
better as DEBUG1 by looking at it. Now and in order to keep things
simple I think that we had better discard this patch for now. I was
planning to come back to this thing anyway once we are done with the
first problem.

> * [PATCH 3/3] hs-checkpoints-v12-3
>
> +* switch segment only when any substantial progress have 
> made from
> +* reasons will cause last_xlog_switch_lsn stay behind but it 
> doesn't
>
> How about, "Switch segment only when substantial progress has been made
> after the last segment was switched by a timeout.  Segment switching for
> other reasons..."
>
> +++ b/src/backend/access/transam/xlog.c
> +   elog(LOG, "Not a forced or shutdown checkpoint: progress_lsn 
> %X/%X,
> ckpt %X/%X",
> +   elog(LOG, "Checkpoint is skipped");
> +   elog(LOG, "snapshot taken by checkpoint %X/%X",
>
> Same for the above, seems like it would just be noise for most users.
>
> +++ b/src/backend/postmaster/bgwriter.c
> +   elog(LOG, "snapshot taken by bgwriter %X/%X",
>
> Ditto.

The original patch was developed to ease debugging, and I chose LOG to
not be polluted with a bunch of DEBUG1 entries :)

Now we can do something, as follows:
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8450,6 +8450,8 @@ CreateCheckPoint(int flags)
{
if (progress_lsn == ControlFile->checkPoint)
{
+   if (log_checkpoints)
+   ereport(LOG, "checkpoint skipped");
WALInsertLockRelease();
LWLockRelease(CheckpointLock);
END_CRIT_SECTION();
Letting users know that the checkpoint has been skipped sounds like a
good idea. Perhaps that's better if squashed with the first patch.

> I don't see any unintended consequences in this patch but it doesn't
> mean there aren't any.  I'm definitely concerned by the exclusive locks
> but it may turn out they do not actually represent a bottleneck.

That's a hard to see a difference. Perhaps I didn't try hard enough..

Well for now attached are two patches, that could just be squashed into one.
-- 
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f95fdb8..e87caa6 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8450,6 +8450,8 @@ CreateCheckPoint(int flags)
 	{
 		if (progress_lsn == ControlFile->checkPoint)
 		{
+			if (log_checkpoints)
+ereport(LOG, "checkpoint skipped");
 			WALInsertLockRelease();
 			LWLockRelease(CheckpointLock);
 			END_CRIT_SECTION();
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index b019bc1..ac40731 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/ba

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Peter Eisentraut]

On 9/28/16 6:13 PM, Robert Haas wrote:
> Christoph/Debian:
> log_line_prefix = '%t [%p-%l] %q%u@%d '
> Peter:
> log_line_prefix = '%t [%p]: [%l] %qapp=%a '

I'm aware of two existing guidelines on log line formats: syslog and
pgbadger.  Syslog output looks like this:

Sep 28 00:58:56 hostname syslogd[46]: some text here

pgbadger by default asks for this:

log_line_prefix = '%t [%p]: [%l-1] user=%u,db=%d,app=%a,client=%h '

I don't know why it wants that "-1" there, and I'm actually not sure
what the point of %l is in practice.  Those are separate issues that are
having their own lively discussions at times.  I could drop the [%l]
from my proposal if that causes concerns.

On balance, I think my proposal is more in line with existing
wide-spread conventions.

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


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Peter Eisentraut]

On 9/28/16 6:07 PM, Alvaro Herrera wrote:
> Adopting a default prefix is a different question.

A default prefix would require different settings for syslog, plain
text, and possibly some of the other variants.  I'm all in favor of
figuring that out, but it needs more work.

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


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

Re: [HACKERS] PATCH: Exclude additional directories in pg_basebackup

[Peter Eisentraut]

On 9/28/16 2:45 AM, Michael Paquier wrote:
> After all that fixed, I have moved the patch to "Ready for Committer".
> Please use the updated patch though.

Committed after some cosmetic changes.

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


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

Re: [HACKERS] pg_basebackup creates a corrupt file for pg_stat_tmp and pg_replslot on a backup location

[Peter Eisentraut]

On 9/25/16 8:06 AM, Ashutosh Sharma wrote:
> Hi Peter,
> 
>> I just wanted to update you, I have taken this commit fest entry patch
>> to review because I think it will be addresses as part of "Exclude
>> additional directories in pg_basebackup", which I'm also reviewing.
>> Therefore, I'm not actually planning on discussing this patch further.
>> Please correct me if this assessment does not match your expectations.
> 
> Thanks for the update. I am absolutely OK with it. I feel it would be
> a good idea to review "Exclude additional directories in
> pg_basebackup" which also addresses the issue reported by me.

That has been committed.

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


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

Re: [HACKERS] Tracking wait event for latches

[Michael Paquier]

On Wed, Sep 28, 2016 at 9:45 PM, Robert Haas  wrote:
> On Wed, Sep 28, 2016 at 8:38 AM, Michael Paquier
>  wrote:
>> So should I change back the patch to have only one argument for the
>> eventId, and guess classId from it?
>
> Why would you need to guess?

Incorrect wording from me perhaps? i just meant that processing needs
to know what is the classId coming for a specific eventId.

> But, yes, I think one argument is much preferable.

OK. Here is the wanted patch. I have reduced the routines of WaitLatch
& friends to use only one argument, and added what is the classId
associated with a given eventId in an array of multiple fields, giving
something like that:
+ const struct wait_event_entry WaitEventEntries[] = {
+   /* Activity */
+   {WAIT_ACTIVITY, "ArchiverMain"},
[...]

I have cleaned up as well the inclusions of pgstat.h that I added
previously. Patch is attached.
-- 
Michael
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 8ca1c1c..9265e00 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -496,7 +496,8 @@ pgfdw_get_result(PGconn *conn, const char *query)
 			wc = WaitLatchOrSocket(MyLatch,
    WL_LATCH_SET | WL_SOCKET_READABLE,
    PQsocket(conn),
-   -1L);
+   -1L,
+   WE_EXTENSION);
 			ResetLatch(MyLatch);
 
 			CHECK_FOR_INTERRUPTS();
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index f400785..bb975c1 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -679,6 +679,42 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
   buffer in question.
  
 
+
+ 
+  Activity: The server process is idle.  This is used by
+  system processes waiting for activity in their main processing loop.
+  wait_event will identify the specific wait point.
+ 
+
+
+ 
+  Extension: The server process is waiting for activity
+  in an extension module.  This category is useful for modules to
+  track custom waiting points.
+ 
+
+
+ 
+  Client: The server process is waiting for some activity
+  on a socket from user applications, and that the server expects
+  something to happen that is independent from its internal processes.
+  wait_event will identify the specific wait point.
+ 
+
+
+ 
+  IPC: The server process is waiting for some activity
+  from another process in the server.  wait_event will
+  identify the specific wait point.
+ 
+
+
+ 
+  Timeout: The server process is waiting for a timeout
+  to expire.  wait_event will identify the specific wait
+  point.
+ 
+

   
  
@@ -1085,6 +1121,143 @@ postgres   27093  0.0  0.0  30096  2752 ?Ss   11:34   0:00 postgres: ser
  BufferPin
  Waiting to acquire a pin on a buffer.
 
+
+ Activity
+ ArchiverMain
+ Waiting in main loop of the archiver process.
+
+
+ AutoVacuumMain
+ Waiting in main loop of autovacuum launcher process.
+
+
+ BgWriterHibernate
+ Waiting in background writer process, hibernating.
+
+
+ BgWriterMain
+ Waiting in main loop of background writer process background worker.
+
+
+ CheckpointerMain
+ Waiting in main loop of checkpointer process.
+
+
+ PgStatMain
+ Waiting in main loop of the statistics collector process.
+
+
+ RecoveryWalAll
+ Waiting for WAL from any kind of source (local, archive or stream) at recovery.
+
+
+ RecoveryWalStream
+ Waiting for WAL from a stream at recovery.
+
+
+ SysLoggerMain
+ Waiting in main loop of syslogger process.
+
+
+ WalReceiverMain
+ Waiting in main loop of WAL receiver process.
+
+
+ WalSenderMain
+ Waiting in main loop of WAL sender process.
+
+
+ WalWriterMain
+ Waiting in main loop of WAL writer process.
+
+
+ Client
+ SecureRead
+ Waiting to read data from a secure connection.
+
+
+ SecureWrite
+ Waiting to write data to a secure connection.
+
+
+ SSLOpenServer
+ Waiting for SSL while attempting connection.
+
+
+ WalReceiverWaitStart
+ Waiting for startup process to send initial data for streaming replication.
+
+
+ WalSenderWaitForWAL
+ Waiting for WAL to be flushed in WAL sender process.
+
+
+  

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[Tomas Vondra]

On 09/29/2016 01:59 AM, Robert Haas wrote:

On Wed, Sep 28, 2016 at 6:45 PM, Tomas Vondra
 wrote:

So, is 300 too little? I don't think so, because Dilip saw some benefit from
that. Or what scale factor do we think is needed to reproduce the benefit?
My machine has 256GB of ram, so I can easily go up to 15000 and still keep
everything in RAM. But is it worth it?


Dunno. But it might be worth a test or two at, say, 5000, just to
see if that makes any difference.



OK, I have some benchmarks to run on that machine, but I'll do a few 
tests with scale 5000 - probably sometime next week. I don't think the 
delay matters very much, as it's clear the patch will end up with RwF in 
this CF round.



I feel like we must be missing something here.  If Dilip is seeing
huge speedups and you're seeing nothing, something is different, and
we don't know what it is.  Even if the test case is artificial, it
ought to be the same when one of you runs it as when the other runs
it.  Right?



Yes, definitely - we're missing something important, I think. One 
difference is that Dilip is using longer runs, but I don't think that's 
a problem (as I demonstrated how stable the results are).


I wonder what CPU model is Dilip using - I know it's x86, but not which 
generation it is. I'm using E5-4620 v1 Xeon, perhaps Dilip is using a 
newer model and it makes a difference (although that seems unlikely).


regards

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


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

Re: [HACKERS] Hash Indexes

[Robert Haas]

On Wed, Sep 28, 2016 at 3:04 PM, Robert Haas  wrote:
> I'll write another email with my thoughts about the rest of the patch.

I think that the README changes for this patch need a fairly large
amount of additional work.  Here are a few things I notice:

- The confusion between buckets and pages hasn't been completely
cleared up.  If you read the beginning of the README, the terminology
is clearly set forth.  It says:

>> A hash index consists of two or more "buckets", into which tuples are placed 
>> whenever their hash key maps to the bucket number.  Each bucket in the hash 
>> index comprises one or more index pages.  The bucket's first page is 
>> permanently assigned to it when the bucket is created. Additional pages, 
>> called "overflow pages", are added if the bucket receives too many tuples to 
>> fit in the primary bucket page."

But later on, you say:

>> Scan will take a lock in shared mode on the primary bucket or on one of the 
>> overflow page.

So the correct terminology here would be "primary bucket page" not
"primary bucket".

- In addition, notice that there are two English errors in this
sentence: the word "the" needs to be added to the beginning of the
sentence, and the last word needs to be "pages" rather than "page".
There are a considerable number of similar minor errors; if you can't
fix them, I'll make a pass over it and clean it up.

- The whole "lock definitions" section seems to me to be pretty loose
and imprecise about what is happening.  For example, it uses the term
"split-in-progress" without first defining it.  The sentence quoted
above says that scans take a lock in shared mode either on the primary
page or on one of the overflow pages, but it's not to document code by
saying that it will do either A or B without explaining which one!  In
fact, I think that a scan will take a content lock first on the
primary bucket page and then on each overflow page in sequence,
retaining a pin on the primary buffer page throughout the scan.  So it
is not one or the other but both in a particular sequence, and that
can and should be explained.

Another problem with this section is that even when it's precise about
what is going on, it's probably duplicating what is or should be in
the following sections where the algorithms for each operation are
explained.  In the original wording, this section explains what each
lock protects, and then the following sections explain the algorithms
in the context of those definitions.  Now, this section contains a
sketch of the algorithm, and then the following sections lay it out
again in more detail.  The question of what each lock protects has
been lost.  Here's an attempt at some text to replace what you have
here:

===
Concurrency control for hash indexes is provided using buffer content
locks, buffer pins, and cleanup locks.   Here as elsewhere in
PostgreSQL, cleanup lock means that we hold an exclusive lock on the
buffer and have observed at some point after acquiring the lock that
we hold the only pin on that buffer.  For hash indexes, a cleanup lock
on a primary bucket page represents the right to perform an arbitrary
reorganization of the entire bucket, while a cleanup lock on an
overflow page represents the right to perform a reorganization of just
that page.  Therefore, scans retain a pin on both the primary bucket
page and the overflow page they are currently scanning, if any.
Splitting a bucket requires a cleanup lock on both the old and new
primary bucket pages.  VACUUM therefore takes a cleanup lock on every
bucket page in turn order to remove tuples.  It can also remove tuples
copied to a new bucket by any previous split operation, because the
cleanup lock taken on the primary bucket page guarantees that no scans
which started prior to the most recent split can still be in progress.
After cleaning each page individually, it attempts to take a cleanup
lock on the primary bucket page in order to "squeeze" the bucket down
to the minimum possible number of pages.
===

As I was looking at the old text regarding deadlock risk, I realized
what may be a serious problem.  Suppose process A is performing a scan
of some hash index.  While the scan is suspended, it attempts to take
a lock and is blocked by process B.  Process B, meanwhile, is running
VACUUM on that hash index.  Eventually, it will do
LockBufferForCleanup() on the hash bucket on which process A holds a
buffer pin, resulting in an undetected deadlock. In the current
coding, A would hold a heavyweight lock and B would attempt to acquire
a conflicting heavyweight lock, and the deadlock detector would kill
one of them.  This patch probably breaks that.  I notice that that's
the only place where we attempt to acquire a buffer cleanup lock
unconditionally; every place else, we acquire the lock conditionally,
so there's no deadlock risk.  Once we resolve this problem, the
paragraph about deadlock risk in this section should be revised to
explain whatever solution we come up with.

By the 

Re: [HACKERS] Transaction user id through logical decoding

[Craig Ringer]

On 28 Sep. 2016 17:50, "valeriof"  wrote:
>
> Hi all,
> I'm developing a custom plugin to stream Postgres CDC changes to my client
> application. One of the info the application needs is the user id of the
> user who executed a certain transaction. I can see we have access to other
> transaction info (xid, lsn, changed data) but apparently the user id is
not
> available.
> Does anyone know if it is possible to extract this info in any way?

It is not recorded in WAL so it isn't possible as-is.

Also you can't assume a tx is all done by one user id. SET ROLE, SECURITY
DEFINER, etc. Even the session user can change during a tx (which IMO a
defect).

You have a couple of options. You could patch pg to add an option to xlog
user id with heap and heap2 rmgr writes, but I doubt it'd have much chance
of getting into core. You'd need to work out how to tell when the new info
was there too.

You could add a new rmgr that logs use is at tx start and whenever it
changes. Doing this robustly could be interesting but I think it'd have
more chance. 10.0 at the earliest though.

You could use a FOR EACH ROW trigger added to each table to xlog a logical
wal message (9.6 only) with the user id changing the row. Maybe optimise by
keeping a cache with the last id logged and only log again if it changes.
Care here is needed for cleanup at xact end, rolled back subxact handling
etc.

(If you don't care about handling the corner cases you could use a FOR EACH
STATEMENT trigger instead.)

You could use a special table in an extension schema that you insert rows
into to record the user who performed an action. Using a before trigger.
Delete the row as soon as you insert it since you only care about the wal
record. Then when decoding inserts examine the affected table oid. If it's
your special table, save the stored user id in output plugin state instead
of sending it to the peer as a normal insert. BDR has some things similar
to this for its handling of ddl replication, TRUNCATE, and global sequence
voting that you could take a look at; see bdr_output.c and bdr_apply.c .

> Thanks,
> Valerio
>
>
>
> --
> View this message in context:
http://postgresql.nabble.com/Transaction-user-id-through-logical-decoding-tp5923261.html
> Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[Robert Haas]

On Wed, Sep 28, 2016 at 6:45 PM, Tomas Vondra
 wrote:
> So, is 300 too little? I don't think so, because Dilip saw some benefit from
> that. Or what scale factor do we think is needed to reproduce the benefit?
> My machine has 256GB of ram, so I can easily go up to 15000 and still keep
> everything in RAM. But is it worth it?

Dunno.  But it might be worth a test or two at, say, 5000, just to see
if that makes any difference.

I feel like we must be missing something here.  If Dilip is seeing
huge speedups and you're seeing nothing, something is different, and
we don't know what it is.  Even if the test case is artificial, it
ought to be the same when one of you runs it as when the other runs
it.  Right?

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


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

Re: [HACKERS] less expensive pg_buffercache on big shmem

[Ivan Kartyshov]

Hello everyone, patch was rebased.

Thank you Tomas for your reviewing this patch and for your valuable 
comments.


From the very beginning we had the misunderstanding with the naming of 
meethods.


> It'd be really useful if you could provide actual numbers, explain what
> metrics you compare and how. I'm sure it makes sense to you but it's
> utterly confusing for everyone else, and it also makes it impossible to
> reproduce the benchmark.

I test it as I wrote earlier, I run it a several times collecting TPS in 
each series, and calculate average value.


> Secondly, I see this bit added to the loop over buffers:
>
> if (bufHdr->tag.forkNum == -1)
> {
> fctx->record[i].blocknum = InvalidBlockNumber;
> continue;
> }
>
> and I have no idea why this is needed (when it was not needed before).

This helps to skip not used bufferpages. It is valuable on big and  not 
warmup shared memory.


On 09/02/2016 12:01 PM, Robert Haas wrote:

I think we certainly want to lock the buffer header, because otherwise
we might get a torn read of the buffer tag, which doesn't seem good.
But it's not obvious to me that there's any point in taking the lock
on the buffer mapping partition; I'm thinking that doesn't really do
anything unless we lock them all, and we all seem to agree that's
going too far.


Replace consistent method with semiconsistent (that lock buffer headers 
without partition lock). Made some additional fixes thanks to reviewers.



--
Ivan Kartyshov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
diff --git a/contrib/pg_buffercache/Makefile b/contrib/pg_buffercache/Makefile
index 497dbeb..9668d84 100644
--- a/contrib/pg_buffercache/Makefile
+++ b/contrib/pg_buffercache/Makefile
@@ -4,8 +4,9 @@ MODULE_big = pg_buffercache
 OBJS = pg_buffercache_pages.o $(WIN32RES)
 
 EXTENSION = pg_buffercache
-DATA = pg_buffercache--1.2.sql pg_buffercache--1.1--1.2.sql \
-	pg_buffercache--1.0--1.1.sql pg_buffercache--unpackaged--1.0.sql
+DATA = pg_buffercache--1.3.sql pg_buffercache--1.2--1.3.sql \
+	pg_buffercache--1.1--1.2.sql pg_buffercache--1.0--1.1.sql \
+	pg_buffercache--unpackaged--1.0.sql
 PGFILEDESC = "pg_buffercache - monitoring of shared buffer cache in real-time"
 
 ifdef USE_PGXS
diff --git a/contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql b/contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql
new file mode 100644
index 000..5935326
--- /dev/null
+++ b/contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql
@@ -0,0 +1,6 @@
+/* contrib/pg_buffercache/pg_buffercache--1.2--1.3.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pg_buffercache(text) UPDATE TO '1.3'" to load this file. \quit
+
+ALTER FUNCTION pg_buffercache_pages() PARALLEL SAFE;
diff --git a/contrib/pg_buffercache/pg_buffercache--1.2.sql b/contrib/pg_buffercache/pg_buffercache--1.3.sql
similarity index 88%
rename from contrib/pg_buffercache/pg_buffercache--1.2.sql
rename to contrib/pg_buffercache/pg_buffercache--1.3.sql
index 6ee5d84..6b3f69e 100644
--- a/contrib/pg_buffercache/pg_buffercache--1.2.sql
+++ b/contrib/pg_buffercache/pg_buffercache--1.3.sql
@@ -1,4 +1,4 @@
-/* contrib/pg_buffercache/pg_buffercache--1.2.sql */
+/* contrib/pg_buffercache/pg_buffercache--1.3.sql */
 
 -- complain if script is sourced in psql, rather than via CREATE EXTENSION
 \echo Use "CREATE EXTENSION pg_buffercache" to load this file. \quit
@@ -18,4 +18,4 @@ CREATE VIEW pg_buffercache AS
 
 -- Don't want these to be available to public.
 REVOKE ALL ON FUNCTION pg_buffercache_pages() FROM PUBLIC;
-REVOKE ALL ON pg_buffercache FROM PUBLIC;
+REVOKE ALL ON pg_buffercache FROM PUBLIC;
\ No newline at end of file
diff --git a/contrib/pg_buffercache/pg_buffercache.control b/contrib/pg_buffercache/pg_buffercache.control
index a4d664f..8c060ae 100644
--- a/contrib/pg_buffercache/pg_buffercache.control
+++ b/contrib/pg_buffercache/pg_buffercache.control
@@ -1,5 +1,5 @@
 # pg_buffercache extension
 comment = 'examine the shared buffer cache'
-default_version = '1.2'
+default_version = '1.3'
 module_pathname = '$libdir/pg_buffercache'
 relocatable = true
diff --git a/contrib/pg_buffercache/pg_buffercache_pages.c b/contrib/pg_buffercache/pg_buffercache_pages.c
index da13bde..b6d2efc 100644
--- a/contrib/pg_buffercache/pg_buffercache_pages.c
+++ b/contrib/pg_buffercache/pg_buffercache_pages.c
@@ -136,15 +136,6 @@ pg_buffercache_pages(PG_FUNCTION_ARGS)
 		MemoryContextSwitchTo(oldcontext);
 
 		/*
-		 * To get a consistent picture of the buffer state, we must lock all
-		 * partitions of the buffer map.  Needless to say, this is horrible
-		 * for concurrency.  Must grab locks in increasing order to avoid
-		 * possible deadlocks.
-		 */
-		for (i = 0; i < NUM_BUFFER_PARTITIONS; i++)
-			LWLockAcquire(BufMappingPartitionLockByIndex(i), LW_SHARED);
-
-		/*
 		 * Scan through all the buffers, saving the relevant fields in the
 		 * fctx->record structure.
 		 */
@

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Jeff Janes]

On Wed, Sep 28, 2016 at 10:48 AM, Robert Haas  wrote:

> On Sun, Sep 25, 2016 at 3:28 PM, Tomas Vondra
>  wrote:
> > Sure, that would be useful.
> >
> > I think it would be useful to make repository of such data sets, so that
> > patch authors & reviewers can get a reasonable collection of data sets if
> > needed, instead of scrambling every time. Opinions?
>
> In theory, great idea.  In practice, I suspect the problem will be
> that nobody will know what the use case for a particular data set was
> supposed to be, and therefore it'll become a collection of files
> nobody knows what to do with.
>

We would have to store them together with the example queries they pertain
to, somehow, rather than just a loose assemblage of files.  Best case is
probably to have a generator for the data, rather than data itself, when
possible. What would be the best technology to host such a thing?

Cheers,

Jeff

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[Tomas Vondra]

On 09/28/2016 05:39 PM, Robert Haas wrote:

On Tue, Sep 27, 2016 at 5:15 PM, Tomas Vondra
 wrote:

So, I got the results from 3.10.101 (only the pgbench data), and it looks
like this:

 3.10.101   1  8 16 32 64128192

 granular-locking2582  18492  33416  49583  53759  53572  51295
 no-content-lock 2580  18666  33860  49976  54382  54012  51549
 group-update2635  18877  33806  49525  54787  54117  51718
 master  2630  18783  33630  49451  54104  53199  50497

So 3.10.101 performs even better tnan 3.2.80 (and much better than 4.5.5),
and there's no sign any of the patches making a difference.


I'm sure that you mentioned this upthread somewhere, but I can't
immediately find it.  What scale factor are you testing here?



300, the same scale factor as Dilip.



It strikes me that the larger the scale factor, the more
CLogControlLock contention we expect to have.  We'll pretty much do
one CLOG access per update, and the more rows there are, the more
chance there is that the next update hits an "old" row that hasn't
been updated in a long time.  So a larger scale factor also
increases the number of active CLOG pages and, presumably therefore,
the amount of CLOG paging activity.

>

So, is 300 too little? I don't think so, because Dilip saw some benefit 
from that. Or what scale factor do we think is needed to reproduce the 
benefit? My machine has 256GB of ram, so I can easily go up to 15000 and 
still keep everything in RAM. But is it worth it?


regards

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


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

Re: [HACKERS] Fix checkpoint skip logic on idle systems by tracking LSN progress

[David Steele]

On 9/28/16 3:35 AM, Michael Paquier wrote:
> On Wed, Sep 28, 2016 at 6:12 AM, David Steele  wrote:
>> I tried the attached patch set and noticed an interesting behavior. With
>> archive_timeout=5 whenever I made a change I would get a WAL segment within
>> a few seconds as expected then another one would follow a few minutes later.
> 
> That's intentional. We may be able to make XLOG_SWITCH records as not
> updating the progress LSN, but I wanted to tackle that as a separate
> patch once we got the basics done correctly, which is still what I
> think this patch is doing. I should have been more precise upthread:
> this patch makes the handling of checkpoint skip logic correct for
> only standby snapshots, not segment switches, and puts the infra to
> handle other things.

OK, I've done functional testing and this patch seems to work as
specified (including the caveat noted above).  Some comments:

* [PATCH 1/3] hs-checkpoints-v12-1

+++ b/src/backend/access/transam/xlog.c
+* Taking a lock is as well necessary to prevent potential torn reads
+* on some platforms.

How about, "Taking a lock is also necessary..."

+   LWLockAcquire(&WALInsertLocks[i].l.lock, LW_EXCLUSIVE);

That's a lot of exclusive locks and that would seem to have performance
implications.  It seems to me this is going to be a hard one to
benchmark because the regression (if any) would only be seen under heavy
load on a very large system.

In general I agree with the other comments that this could end up being
a problem.  On the other hand, since the additional locks are only taken
at checkpoint or archive_timeout it may not be that big a deal.

+++ b/src/backend/access/transam/xloginsert.c * Should this record
include the replication origin if one is set up?

Outdated comment from XLogIncludeOrigin().

* [PATCH 2/3] hs-checkpoints-v12-2

+++ b/src/backend/postmaster/checkpointer.c
+   /* OK, it's time to switch */
+   elog(LOG, "Request XLog Switch");

LOG level seems a bit much here, perhaps DEBUG1?

* [PATCH 3/3] hs-checkpoints-v12-3

+* switch segment only when any substantial progress have made 
from
+* reasons will cause last_xlog_switch_lsn stay behind but it 
doesn't

How about, "Switch segment only when substantial progress has been made
after the last segment was switched by a timeout.  Segment switching for
other reasons..."

+++ b/src/backend/access/transam/xlog.c
+   elog(LOG, "Not a forced or shutdown checkpoint: progress_lsn 
%X/%X,
ckpt %X/%X",
+   elog(LOG, "Checkpoint is skipped");
+   elog(LOG, "snapshot taken by checkpoint %X/%X",

Same for the above, seems like it would just be noise for most users.

+++ b/src/backend/postmaster/bgwriter.c
+   elog(LOG, "snapshot taken by bgwriter %X/%X",

Ditto.

I don't see any unintended consequences in this patch but it doesn't
mean there aren't any.  I'm definitely concerned by the exclusive locks
but it may turn out they do not actually represent a bottleneck.

This does seem like the kind of patch that should get committed very
early in the release cycle to allow maximum time for regression testing.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Perhaps we should first try to get a consensus on the regression test
>> use-case.

> I thought Peter's suggestion for regression test drivers was a good one
> and I see no reason to block that.  Why do you (Tom) object so strongly
> against having a different one on buildfarm than elsewhere?  I'd rather
> have buildfarm adopt the new suggestion than having buildfarm drive the
> new stuff.

Well, my point is only that if you can't convince Andrew to sync the
buildfarm's choices with whatever your proposal is, then you haven't
got consensus.

regards, tom lane


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

Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Thu, Sep 29, 2016 at 11:12:11AM +1300, Thomas Munro wrote:
> On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro
>  wrote:
> > On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro
> >  wrote:
> >>
> >> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter  wrote:
> >> >
> >> > [training_wheels_004.patch]
> >>
> >> [review]
> 
> Ping.

I'll have another revision out as soon as I get some more test cases.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Thomas Munro]

On Mon, Sep 26, 2016 at 5:11 PM, Thomas Munro
 wrote:
> On Mon, Sep 26, 2016 at 1:18 PM, Thomas Munro
>  wrote:
>>
>> On Mon, Sep 19, 2016 at 4:02 PM, David Fetter  wrote:
>> >
>> > [training_wheels_004.patch]
>>
>> [review]

Ping.

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


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Robert Haas]

On Wed, Sep 28, 2016 at 6:07 PM, Alvaro Herrera
 wrote:
> I thought Peter's suggestion for regression test drivers was a good one
> and I see no reason to block that.  Why do you (Tom) object so strongly
> against having a different one on buildfarm than elsewhere?  I'd rather
> have buildfarm adopt the new suggestion than having buildfarm drive the
> new stuff.
>
> Adopting a default prefix is a different question.  For one thing IMHO
> it should not have %a (application name).  Christoph's suggestion
> (Debian's default) seemed good.

Yeah, I like Cristoph's suggestion fine.  It meets my criteria of
"includes timestamp and PID" and overall seems reasonable.   If we
adopted that across the board, it wouldn't be too much different from
what Peter proposed for the regression test.  Just to compare.

Christoph/Debian:
log_line_prefix = '%t [%p-%l] %q%u@%d '
Peter:
log_line_prefix = '%t [%p]: [%l] %qapp=%a '

So Peter's got %p and %l separated by "]: [" whereas Christoph has
them separated only by a dash.  Presumably that's minor.  Then they've
both got %q.  After that, Christoph has %u@%d, which seems reasonable
for an actual system, and Peter's got app=%a, which is better for the
regression tests because the user name will depend on the UNIX
username of the person running the tests.

So how about we adopt both suggestions, except changing Peter's to '%t
[%p-%l] %qapp=%a ' so that they are a bit more similar?  I bet that
would make more people happier than it would make less happy.

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


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

Re: [HACKERS] compiler warning read_objtype_from_string()

[Alvaro Herrera]

Tom Lane wrote:

> I do not think you should assume that the compiler is smart enough to
> deduce that, nor that all compilers even know ereport(ERROR) doesn't
> return.  Personally I don't see the point of the "type" variable at
> all, anyway.  I would have written this as
> 
> [code]

Makes sense.  I will patch it that way.

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


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Alvaro Herrera]

Tom Lane wrote:
> Robert Haas  writes:
> > On Thu, Sep 15, 2016 at 5:18 PM, Robert Haas  wrote:
> >> On Sat, Aug 27, 2016 at 3:59 PM, Tom Lane  wrote:
> >>> I think the odds of getting to something that everyone would agree on
> >>> are nil, so I'm not excited about getting into that particular
> >>> bikeshed-painting discussion.  Look at the amount of trouble we're
> >>> having converging on a default for the regression tests, which are
> >>> a far narrower use-case than "everybody".
> 
> >> Well, practically anything that includes a PID and the timestamp is
> >> going to be an improvement over the status quo.  Just because we can't
> >> all agree on what would be perfect does not mean that we can't do
> >> better than what we've got now.  +1 for trying.
> 
> > Is there any chance we can move forward here, or is this effort doomed for 
> > now?
> 
> It seemed like nobody wanted to try to push this forward, and it will take
> somebody actively pushing, IMO, for something to happen.
> 
> Perhaps we should first try to get a consensus on the regression test
> use-case.

I thought Peter's suggestion for regression test drivers was a good one
and I see no reason to block that.  Why do you (Tom) object so strongly
against having a different one on buildfarm than elsewhere?  I'd rather
have buildfarm adopt the new suggestion than having buildfarm drive the
new stuff.

Adopting a default prefix is a different question.  For one thing IMHO
it should not have %a (application name).  Christoph's suggestion
(Debian's default) seemed good.

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


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

Re: [HACKERS] compiler warning read_objtype_from_string()

[Tom Lane]

Alvaro Herrera  writes:
> Peter Eisentraut wrote:
>> I'm getting the following compiler warning (using nondefault
>> optimization options):
>> objectaddress.c: In function 'read_objtype_from_string':
>> objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
>> function [-Werror=maybe-uninitialized]
>> return type;

> Umm.  I think it can only be uninitialized if we fall out of the end of
> the array, in which case we're supposed to throw the ERROR and never
> return.  Is that not working?

I do not think you should assume that the compiler is smart enough to
deduce that, nor that all compilers even know ereport(ERROR) doesn't
return.  Personally I don't see the point of the "type" variable at
all, anyway.  I would have written this as

inti;

for (i = 0; i < lengthof(ObjectTypeMap); i++)
{
if (strcmp(ObjectTypeMap[i].tm_name, objtype) == 0)
return ObjectTypeMap[i].tm_type;
}
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
 errmsg("unrecognized object type \"%s\"", objtype)));
return -1;/* keep compiler quiet */

regards, tom lane


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

Re: [HACKERS] Showing parallel status in \df+

[Tom Lane]

Pavel Stehule  writes:
> We are in cycle because prosrc field is used for two independent features -
> and then it can be hard to find a agreement.

I thought pretty much everyone was on board with the idea of keeping
prosrc in \df+ for internal/C-language functions (and then probably
renaming the column, since it isn't actually source code in that case).
The argument is over what to do for PL functions, which is only one use
case not two.

regards, tom lane


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

Re: [HACKERS] kqueue

[Thomas Munro]

On Thu, Sep 29, 2016 at 9:09 AM, Keith Fiske  wrote:
> On Thu, Sep 15, 2016 at 11:11 PM, Thomas Munro
>  wrote:
>> Ok, here's a version tweaked to use EVFILT_PROC for postmaster death
>> detection instead of the pipe, as Tom Lane suggested in another
>> thread[1].
>>
>> [...]
>
> Ran benchmarks on unaltered 96rc1 again just to be safe. Those are first.
> Decided to throw a 32 process test in there as well to see if there's
> anything going on between 4 and 64

Thanks!  A summary:

┌──┬─┬───┬┬───┐
│   code   │ clients │  average  │ standard_deviation │  median   │
├──┼─┼───┼┼───┤
│ 9.6rc1   │   1 │ 25704.923 │108.766 │ 25731.006 │
│ 9.6rc1   │   4 │ 94032.889 │322.562 │ 94123.436 │
│ 9.6rc1   │  32 │ 86647.401 │ 33.616 │ 86664.849 │
│ 9.6rc1   │  64 │ 79360.680 │   1217.453 │ 79941.243 │
│ 9.6rc1/kqueue-v6 │   1 │ 24569.683 │   1433.339 │ 25146.434 │
│ 9.6rc1/kqueue-v6 │   4 │ 93435.450 │ 50.214 │ 93442.716 │
│ 9.6rc1/kqueue-v6 │  32 │ 88000.328 │135.143 │ 87891.856 │
│ 9.6rc1/kqueue-v6 │  64 │ 71726.034 │   4784.794 │ 72271.146 │
└──┴─┴───┴┴───┘

┌─┬───┬───┬──┐
│ clients │ unpatched │  patched  │  percent_change  │
├─┼───┼───┼──┤
│   1 │ 25731.006 │ 25146.434 │ -2.271858317548874692000 │
│   4 │ 94123.436 │ 93442.716 │ -0.72322051651408051 │
│  32 │ 86664.849 │ 87891.856 │  1.415807001521458833000 │
│  64 │ 79941.243 │ 72271.146 │ -9.594668173973727179000 │
└─┴───┴───┴──┘

The variation in the patched 64 client numbers is quite large, ranging
from ~66.5k to ~79.5k.  The highest number matched the unpatched
numbers which ranged 77.9k to 80k.  I wonder if that is noise and we
need to run longer (in which case the best outcome might be 'this
patch is neutral on FreeBSD'), or if something the patch does is doing
is causing that (for example maybe EVFILT_PROC proc filters causes
contention on the process table lock).

Matteo's results with the v6 patch on a low end NetBSD machine were
not good.  But the report at [1] implies that larger NetBSD and
OpenBSD systems have terrible problems with the
poll-postmaster-alive-pipe approach, which this EVFILT_PROC approach
would seem to address pretty well.

It's difficult to draw any conclusions at this point.

[1] https://www.postgresql.org/message-id/flat/20160915135755.GC19008%40genua.de

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

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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Tom Lane]

Robert Haas  writes:
> On Thu, Sep 15, 2016 at 5:18 PM, Robert Haas  wrote:
>> On Sat, Aug 27, 2016 at 3:59 PM, Tom Lane  wrote:
>>> I think the odds of getting to something that everyone would agree on
>>> are nil, so I'm not excited about getting into that particular
>>> bikeshed-painting discussion.  Look at the amount of trouble we're
>>> having converging on a default for the regression tests, which are
>>> a far narrower use-case than "everybody".

>> Well, practically anything that includes a PID and the timestamp is
>> going to be an improvement over the status quo.  Just because we can't
>> all agree on what would be perfect does not mean that we can't do
>> better than what we've got now.  +1 for trying.

> Is there any chance we can move forward here, or is this effort doomed for 
> now?

It seemed like nobody wanted to try to push this forward, and it will take
somebody actively pushing, IMO, for something to happen.

Perhaps we should first try to get a consensus on the regression test
use-case.

regards, tom lane


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

Re: [HACKERS] Bug in to_timestamp().

[Tom Lane]

Artur Zakirov  writes:
> - now DCH_cache_getnew() is called after parse_format(). Because now 
> parse_format() can raise an error and in the next attempt 
> DCH_cache_search() could return broken cache entry.

I started looking at your 0001-to-timestamp-format-checking-v4.patch
and this point immediately jumped out at me.  Currently the code relies
... without any documentation ... on no elog being thrown out of
parse_format().  That's at the very least trouble waiting to happen.
There's a hack to deal with errors from within the NUMDesc_prepare
subroutine, but it's a pretty ugly and underdocumented hack.  And what
you had here was randomly different from that solution, too.

After a bit of thought it seemed to me that a much cleaner fix is to add
a "valid" flag to the cache entries, which we can leave clear until we
have finished parsing the new format string.  That avoids adding extra
data copying as you suggested, removes the need for PG_TRY, and just
generally seems cleaner and more bulletproof.

I've pushed a patch that does it that way.  The 0001 patch will need
to be rebased over that (might just require removal of some hunks,
not sure).

I also pushed 0002-to-timestamp-validation-v2.patch with some revisions
(it'd broken acceptance of BC dates, among other things, but I think
I fixed everything).

Since you told us earlier that you'd be on vacation through the end of
September, I'm assuming that nothing more will happen on this patch during
this commitfest, so I will mark the CF entry Returned With Feedback.

regards, tom lane


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

Re: [HACKERS] Floating point comparison inconsistencies of the geometric types

[Kevin Grittner]

On Wed, Sep 28, 2016 at 2:04 PM, Kevin Grittner  wrote:

> Will take a look and post again.

I am moving this patch to the next CF.  You'll be hearing from me
sometime after this CF is closed.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] kqueue

[Keith Fiske]

On Thu, Sep 15, 2016 at 11:11 PM, Thomas Munro <
thomas.mu...@enterprisedb.com> wrote:

> On Thu, Sep 15, 2016 at 11:04 AM, Thomas Munro
>  wrote:
> > On Thu, Sep 15, 2016 at 10:48 AM, Keith Fiske  wrote:
> >> Thomas Munro brought up in #postgresql on freenode needing someone to
> test a
> >> patch on a larger FreeBSD server. I've got a pretty decent machine
> (3.1Ghz
> >> Quad Core Xeon E3-1220V3, 16GB ECC RAM, ZFS mirror on WD Red HDD) so
> offered
> >> to give it a try.
> >>
> >> Bench setup was:
> >> pgbench -i -s 100 -d postgres
> >>
> >> I ran this against 96rc1 instead of HEAD like most of the others in this
> >> thread seem to have done. Not sure if that makes a difference and can
> re-run
> >> if needed.
> >> With higher concurrency, this seems to cause decreased performance. You
> can
> >> tell which of the runs is the kqueue patch by looking at the path to
> >> pgbench.
> >
> > Thanks Keith.  So to summarise, you saw no change with 1 client, but
> > with 4 clients you saw a significant drop in performance (~93K TPS ->
> > ~80K TPS), and a smaller drop for 64 clients (~72 TPS -> ~68K TPS).
> > These results seem to be a nail in the coffin for this patch for now.
> >
> > Thanks to everyone who tested.  I might be back in a later commitfest
> > if I can figure out why and how to fix it.
>
> Ok, here's a version tweaked to use EVFILT_PROC for postmaster death
> detection instead of the pipe, as Tom Lane suggested in another
> thread[1].
>
> The pipe still exists and is used for PostmasterIsAlive(), and also
> for the race case where kevent discovers that the PID doesn't exist
> when you try to add it (presumably it died already, but we want to
> defer the report of that until you call EventSetWait, so in that case
> we stick the traditional pipe into the kqueue set as before so that
> it'll fire a readable-because-EOF event then).
>
> Still no change measurable on my laptop.  Keith, would you be able to
> test this on your rig and see if it sucks any less than the last one?
>
> [1] https://www.postgresql.org/message-id/13774.1473972000%40sss.pgh.pa.us
>
> --
> Thomas Munro
> http://www.enterprisedb.com
>


Ran benchmarks on unaltered 96rc1 again just to be safe. Those are first.
Decided to throw a 32 process test in there as well to see if there's
anything going on between 4 and 64

~/pgsql96rc1/bin/pgbench -i -s 100 -d pgbench -p 5496

[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 1 -c 1 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 1
number of threads: 1
duration: 60 s
number of transactions actually processed: 1543809
latency average: 0.039 ms
tps = 25729.749474 (including connections establishing)
tps = 25731.006414 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 1 -c 1 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 1
number of threads: 1
duration: 60 s
number of transactions actually processed: 1548340
latency average: 0.039 ms
tps = 25796.928387 (including connections establishing)
tps = 25798.275891 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 1 -c 1 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 1
number of threads: 1
duration: 60 s
number of transactions actually processed: 1535072
latency average: 0.039 ms
tps = 25584.182830 (including connections establishing)
tps = 25585.487246 (excluding connections establishing)

[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 4 -c 4 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 4
number of threads: 4
duration: 60 s
number of transactions actually processed: 5621013
latency average: 0.043 ms
tps = 93668.594248 (including connections establishing)
tps = 93674.730914 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 4 -c 4 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 4
number of threads: 4
duration: 60 s
number of transactions actually processed: 5659929
latency average: 0.042 ms
tps = 94293.572928 (including connections establishing)
tps = 94300.500395 (excluding connections establishing)
[keith@corpus ~]$ /home/keith/pgsql96rc1/bin/pgbench -T 60 -j 4 -c 4 -M
prepared -S -p 5496 pgbench
starting vacuum...end.
transaction type: 
scaling factor: 100
query mode: prepared
number of clients: 4
number of threads: 4
duration: 60 s
number of transactions actually processed: 5649572
latency average: 0.042 ms
tps = 94115.854165 (including connections establishing)
tps = 94123.436211 (excluding connections establishing)

[keith@corpus ~]$ /home/keith/pgsql96

Re: [HACKERS] Showing parallel status in \df+

[Pavel Stehule]

2016-09-28 21:59 GMT+02:00 Alvaro Herrera :

> Pavel Stehule wrote:
>
> > I am sorry, I disagree. Proposed form is hard readable. Is not possible
> to
> > simply copy/paste.
>
> Why do you care?  You can use \sf if you want to copy&paste the
> function code.
>

I know so I can use \sf. But I don't see any sense to have less readable
output of any psql command.


>
> > I cannot to imagine any use case for proposed format.
>
> My vote (which was not counted by Stephen) was to remove it from \df+
> altogether.  I stand by that.  People who are used to seeing the output
> in \df+ will wonder "where the heck did it go" and eventually figure it
> out, at which point it's no longer a problem.  We're not breaking
> anyone's scripts, that's for sure.
>

I prefer removing before proposed solution with proposed format.

We are in cycle because prosrc field is used for two independent features -
and then it can be hard to find a agreement.

Name of function in dll is some different than PL function body. But it is
stored and displayed in one field - and it is impossible do it well.

Regards

Pavel


>
> If we're not removing it, I +0 support the option of moving it to
> footers.  I'm -1 on doing nothing.
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] ICU integration

[Thomas Munro]

On Fri, Sep 23, 2016 at 6:27 PM, Thomas Munro
 wrote:
> On Wed, Aug 31, 2016 at 2:46 PM, Peter Eisentraut
>  wrote:
>> Here is a patch I've been working on to allow the use of ICU for sorting
>> and other locale things.
>
> This is very interesting work, and it's great to see some development
> in this area.  I've been peripherally involved in more than one
> collation-change-broke-my-data incident over the years.  I took the
> patch for a quick spin today.  Here are a couple of initial
> observations.

This seems like a solid start, but there are unresolved questions
about both high level goals (versioning strategy etc) and also some
technical details with this WIP patch.  It looks like several people
have an interest and ideas in this area, but clearly there isn't going
to be a committable patch in the next 48 hours.  So I will set this to
'Returned with Feedback' for now.  If you think you'll have a new
patch for the next CF then it looks like you can still 'Move to Next
CF' from 'Returned with Feedback' state if appropriate.  Thanks!

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


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

Re: [HACKERS] Showing parallel status in \df+

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Pavel Stehule wrote:
> > I cannot to imagine any use case for proposed format.
> 
> My vote (which was not counted by Stephen) was to remove it from \df+

Oh, sorry about that, not sure how I missed it. :/

> altogether.  I stand by that.  People who are used to seeing the output
> in \df+ will wonder "where the heck did it go" and eventually figure it
> out, at which point it's no longer a problem.  We're not breaking
> anyone's scripts, that's for sure.
> 
> If we're not removing it, I +0 support the option of moving it to
> footers.  I'm -1 on doing nothing.

This is more-or-less the same position that I have.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Showing parallel status in \df+

[Alvaro Herrera]

Pavel Stehule wrote:

> I am sorry, I disagree. Proposed form is hard readable. Is not possible to
> simply copy/paste.

Why do you care?  You can use \sf if you want to copy&paste the
function code.

> I cannot to imagine any use case for proposed format.

My vote (which was not counted by Stephen) was to remove it from \df+
altogether.  I stand by that.  People who are used to seeing the output
in \df+ will wonder "where the heck did it go" and eventually figure it
out, at which point it's no longer a problem.  We're not breaking
anyone's scripts, that's for sure.

If we're not removing it, I +0 support the option of moving it to
footers.  I'm -1 on doing nothing.

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


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

Re: [HACKERS] Add support for restrictive RLS policies

[Stephen Frost]

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Stephen, the typo "awseome" in the tests is a bit distracting.  Can you
> please fix it?

Done.

> I think you should use braces here, not parens:

Fixed.

> I don't think this paragraph is right -- you should call out each of the
> values PERMISSIVE and RESTRICTIVE (in upper case) instead.  Also note
> typos "Alternativly" and "visibillity".

Done.

> I dislike the "AND"d and "OR"d spelling of those terms.  Currently they
> only appear in comments within rowsecurity.c (of your authorship too, I
> imagine).  I think it'd be better to find actual words for those
> actions.

Reworded to not attempt to use AND and OR as verbs.  Additionally, a
patch is also included to remove those from the comments in
rowsecurity.c.  There are a few other places where we have "OR'd" in the
code base, but I didn't think it made sense to change those as part of
this effort.

* Jeevan Chalke (jeevan.cha...@enterprisedb.com) wrote:
> With this patch, pg_policy catalog now has seven columns, however
> Natts_pg_policy is still set to 6. It should be updated to 7 now.
> Doing this regression seems OK.

Ah, certainly interesting that it only caused incorrect behavior and not
a crash (and no incorrect behavior even on my system, at least with the
regression tests and other testing I've done).

Fixed.

> 1. In documentation, we should put both permissive as well as restrictive in
> the header like permissive|restrictive. 

I'm not sure which place in the documentation you are referring to
here..?  [ AS { PERMISSIVE | RESTRICTIVE } ] was added to the CREATE
POLICY synopsis documentation.

> 2. "If the policy is a "permissive" or "restrictive" policy." seems broken
> as
> sentence starts with "If" and there is no other part to it. Will it be
> better
> to say "Specifies whether the policy is a "permissive" or "restrictive"
> policy."?

Rewrote this to be clearer, I hope.

> 3. " .. a policy can instead by "restrictive""
> Do you mean "instead be" here?

This was also rewritten.

> 4. It will be good if we have an example for this in section
> "5.7. Row Security Policies"

I haven't added one yet, but will plan to do so.

> 5. AS ( PERMISSIVE | RESTRICTIVE )
> should be '{' and '}' instead of parenthesis.

Fixed.

> 6. I think following changes are irrelevant for this patch.
> Should be submitted as separate patch if required.

As mentioned, this is tab-completion for the new options which this
patch introduces.

> 7. Natts_pg_policy should be updated to 7 now.

Fixed.

> 8. In following error, $2 and @2 should be used to correctly display the
> option and location.

Fixed.

> I think adding negative test to test this error should be added in
> regression.

Done.

> 9. Need to update following comments in gram.y to reflect new changes.

Done.

> 10. ALTER POLICY has no changes for this. Any reason why that is not
> considered here.

As mentioned, I don't see a use-case for it currently.

> 11. Will it be better to use boolean for polpermissive in _policyInfo?
> And then set that appropriately while getting the policies. So that later we
> only need to test the boolean avoiding string comparison.

Done.

> 12. Since PERMISSIVE is default, we should dump only "RESTRICTIVE" when
> appropriate, like other default cases.

Done, for this and the other defaults.

> 13. Since PERMISSIVE is default, do we need changes like below?
> -\QCREATE POLICY p1 ON test_table FOR ALL TO PUBLIC \E
> +\QCREATE POLICY p1 ON test_table AS PERMISSIVE FOR ALL TO
> PUBLIC \E

Updated to reflect what pg_dump now produces.

> 14. While displaying policy details in permissionsList, per syntax, we
> should
> display (RESTRICT) before the command option. Also will it be better to use
> (RESTRICTIVE) instead of (RESTRICT)?

Fixed.

> 15. Similarly in describeOneTableDetails() too, can we have RESTRICTIVE
> after
> policy name and before command option ?
> If we do that then changes related to adding "POLICY" followed by
> "RESTRICTIVE"
> will be straight forward.

Fixed.

> 16. It be good to have test-coverage for permissionsList,
> describeOneTableDetails and dump-restore changes. Please add those.

Done.

> 17. In pg_policies view, we need to add details related to PERMISSIVE and
> RESTRICTIVE. Please do so. Also add test for it.

Done.

> 18. Fix typos pointed earlier by Alvera.

Done.

Updated patch attached.

Thanks!

Stephen
From 020871cddd3c7187bd55a52673cae0af17a95246 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Thu, 1 Sep 2016 02:11:30 -0400
Subject: [PATCH 1/2] Add support for restrictive RLS policies

We have had support for restrictive RLS policies since 9.5, but they
were only available through extensions which use the appropriate hooks.
This adds support into the grammer, catalog, psql and pg_dump for
restrictive RLS policies, thus reducing the cases where an extension is
necessary.
---
 doc/src/sgml/ref/create_policy.sgml   |  28 
 src/backend/catalog/system_views.

Re: [HACKERS] Hash Indexes

[Robert Haas]

On Wed, Sep 28, 2016 at 3:06 PM, Andres Freund  wrote:
> On 2016-09-28 15:04:30 -0400, Robert Haas wrote:
>> Andres already
>> stated that he things working on btree-over-hash would be more
>> beneficial than fixing hash, but at this point it seems like he's the
>> only one who takes that position.
>
> Note that I did *NOT* take that position. I was saying that I think we
> should evaluate whether that's not a better approach, doing some simple
> performance comparisons.

OK, sorry.  I evidently misunderstood your position, for which I apologize.

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


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

Re: [HACKERS] Hash Indexes

[Andres Freund]

On 2016-09-28 15:04:30 -0400, Robert Haas wrote:
> Andres already
> stated that he things working on btree-over-hash would be more
> beneficial than fixing hash, but at this point it seems like he's the
> only one who takes that position.

Note that I did *NOT* take that position. I was saying that I think we
should evaluate whether that's not a better approach, doing some simple
performance comparisons.

Greetings,

Andres Freund


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

Re: [HACKERS] Password identifiers, protocol aging and SCRAM protocol

[Stephen Frost]

Heikki, Michael, Magnus,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, Sep 27, 2016 at 10:42 PM, Heikki Linnakangas  wrote:
> > The libpq-side is not. Just calling random() won't do. We haven't needed for
> > random numbers in libpq before, but now we do. Is the pgcrypto solution
> > portable enough that we can use it in libpq?
> 
> Do you think that urandom would be enough then? The last time I took a
> look at that, I saw urandom on all modern platforms even those ones:
> OpenBSD, NetBSD, Solaris, SunOS. For Windows the CryptGen stuff would
> be nice enough I guess..

Magnus had been working on a patch that, as I recall, he thought was
portable and I believe could be used on both sides.

Magnus, would what you were working on be helpful here...?

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Hash Indexes

[Robert Haas]

On Tue, Sep 27, 2016 at 3:06 PM, Jesper Pedersen
 wrote:
> I have been running various tests, and applications with this patch together
> with the WAL v5 patch [1].
>
> As I havn't seen any failures and doesn't currently have additional feedback
> I'm moving this patch to "Ready for Committer" for their feedback.

Cool!  Thanks for reviewing.

Amit, can you please split the buffer manager changes in this patch
into a separate patch?  I think those changes can be committed first
and then we can try to deal with the rest of it.  Instead of adding
ConditionalLockBufferShared, I think we should add an "int mode"
argument to the existing ConditionalLockBuffer() function.  That way
is more consistent with LockBuffer().  It means an API break for any
third-party code that's calling this function, but that doesn't seem
like a big problem.  There are only 10 callers of
ConditionalLockBuffer() in our source tree and only one of those is in
contrib, so probably there isn't much third-party code that will be
affected by this, and I think it's worth it for the long-term
cleanliness.

As for CheckBufferForCleanup, I think that looks OK, but: (1) please
add an Assert() that we hold an exclusive lock on the buffer, using
LWLockHeldByMeInMode; and (2) I think we should rename it to something
like IsBufferCleanupOK.  Then, when it's used, it reads like English:
if (IsBufferCleanupOK(buf)) { /* clean up the buffer */ }.

I'll write another email with my thoughts about the rest of the patch.
For the record, Amit and I have had extensive discussions about this
effort off-list, and as Amit noted in his original post, the design is
based on suggestions which I previously posted to the list suggesting
how the issues with hash indexes might get fixed.  Therefore, I don't
expect to have too many basic disagreements regarding the design of
the patch; if anyone else does, please speak up.  Andres already
stated that he things working on btree-over-hash would be more
beneficial than fixing hash, but at this point it seems like he's the
only one who takes that position.  Even if we accept that working on
the hash AM is a reasonable thing to do, it doesn't follow that the
design Amit has adopted here is ideal.  I think it's reasonably good,
but that's only to be expected considering that I drafted the original
version of it and have been involved in subsequent discussions;
someone else might dislike something that I thought was OK, and any
such opinions certainly deserve a fair hearing.  To be clear, It's
been a long time since I've looked at any of the actual code in this
patch and I have at no point studied it deeply, so I expect that I may
find a fair number of things that I'm not happy with in detail, and
I'll write those up along with any design-level concerns that I do
have.  This should in no way forestall review from anyone else who
wants to get involved.

Thanks,

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


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

Re: [HACKERS] Floating point comparison inconsistencies of the geometric types

[Kevin Grittner]

On Wed, Sep 28, 2016 at 12:02 PM, Emre Hasegeli  wrote:

>> `make check` finds differences per the attached.  Please
>> investigate why the regression tests are failing and what the
>> appropriate response is.
>
> I fixed the first one and workaround the second with COLLATE "C".  I
> have how my changes caused this regression.
>
> "select_views" test runs "SELECT name, #thepath FROM iexit ORDER BY 1,
> 2" and expects to get rows in this order:
>
>>  I- 580Ramp |8
>>  I- 580/I-680  Ramp |2
>
> With the collation on my laptop, this is actually true:
>
>> regression=# select 'I- 580/I-680  Ramp' < 'I- 580   
>>  Ramp';
>>  ?column?
>> --
>>  t
>> (1 row)
>
> However, on the Linux server, I am testing it is not:
>
>> regression=# select 'I- 580Ramp' < 'I- 580/I-680 
>>  Ramp';
>>  ?column?
>> --
>>  f
>> (1 row)
>
> Do you know how it is not failing on the master?

Well, those two results are not contradictory -- notice that you
switched the order of the values in the comparison.  I don't think
you've really found the explanation yet.

>> [discussing inline static functions compared to macros for min()/max(), etc.]
>> I suspect that they will be as fast or faster, and they eliminate
>> the hazard of multiple evaluation, where a programmer might not be
>> aware of the multiple evaluation or of some side-effect of an
>> argument.
>
> I reworked the the patches to use inline functions and fixed the
> problems I found.  The new versions are attached.

Will take a look and post again.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


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

Re: [HACKERS] Binary I/O for isn extension

[Robert Haas]

On Wed, Sep 28, 2016 at 2:05 PM, Shay Rojansky  wrote:
> Sorry about this, I just haven't had a free moment (and it's definitely not
> very high priority...)

No issues, just cleaning house.

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


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

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Robert Haas]

On Wed, Sep 28, 2016 at 2:11 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, Sep 22, 2016 at 1:37 PM, Tom Lane  wrote:
>>> OK, I'll think about how to do that more efficiently.  The smaller
>>> incremental improvement isn't surprising, because in this example the
>>> index would still be 90-something MB if it had no free space at all,
>>> so there's going to be decreasing returns from any additional work
>>> to avoid wasted free space.  But if we can do it cheaply, this does
>>> suggest that using pages in order by free space is of value.
>
>> Tom, are you planning to do something about this patch yet this
>> CommitFest, or leave it until later?
>
> I doubt I will get to it this week, so let's mark it RWF for this fest.

OK, done.  Thanks for the reply.

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


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Christoph Berg]

Re: Robert Haas 2016-09-28 

> > Well, practically anything that includes a PID and the timestamp is
> > going to be an improvement over the status quo.  Just because we can't
> > all agree on what would be perfect does not mean that we can't do
> > better than what we've got now.  +1 for trying.
> 
> Is there any chance we can move forward here, or is this effort doomed for 
> now?

IMHO it would make sense. Maybe we should collect a few suggestions,
and then take a poll?

Christoph


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

Re: [HACKERS] Showing parallel status in \df+

[Pavel Stehule]

Hi

2016-09-28 18:57 GMT+02:00 Tom Lane :

> Pavel Stehule  writes:
> > 2016-09-28 16:03 GMT+02:00 Tom Lane :
> >> I propose to push my current patch (ie, move PL function
> >> source code to \df+ footers), and we can use it in HEAD for awhile
> >> and see what we think.  We can alway improve or revert it later.
>
> > I had some objection to format of source code - it should be full source
> > code, not just header and body.
>
> That would be redundant with stuff that's in the main part of the \df
> display.  I really don't need to see the argument types twice, for
> instance.
>

I am sorry, I disagree. Proposed form is hard readable. Is not possible to
simply copy/paste.

I cannot to imagine any use case for proposed format.

Regards

Pavel


>
> regards, tom lane
>

Re: [HACKERS] should xlog_outdesc modify its argument?

[Mark Dilger]

> On Sep 27, 2016, at 11:25 PM, Heikki Linnakangas  wrote:
> 
> On 09/28/2016 02:35 AM, Mark Dilger wrote:
>> The function
>> 
>>  static void xlog_outdesc(StringInfo buf, XLogReaderState *record);
>> 
>> in src/backend/access/transam/xlog.c is called by XLogInsertRecord,
>> and after returning a string describing an XLogRecord, it clears the
>> state data in its XLogReaderState argument.  That mixes the read-only
>> semantics of "give me a string that describes this argument" and the
>> read-write semantics of "clear out the value in this argument".
> 
> I don't see where the "clears the state data" is happening. Can you elaborate?

My apologies.  At the bottom of the function, it calls through the function 
pointer

RmgrTable[rmid].rm_desc(buf, record);

which is set up to call various *_desc functions.  I must have chased through
those function pointers incorrectly, as I can't find the problem now that I am
reviewing all those functions.

Sorry for the noise,

Mark Dilger



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

Re: [HACKERS] psql casts aspersions on server reliability

[Petr Jelinek]

On 28/09/16 17:13, David Steele wrote:
> On 9/28/16 10:22 AM, Robert Haas wrote:
>> On Wed, Sep 28, 2016 at 9:14 AM, Tom Lane  wrote:
>>> Robert Haas  writes:
 psql tends to do things like this:
 rhaas=# select * from pg_stat_activity;
 FATAL:  terminating connection due to administrator command
 server closed the connection unexpectedly
 This probably means the server terminated abnormally
 before or while processing the request.
>>>
 Basically everything psql has to say about this is a lie:
>>>
>>> I cannot get terribly excited about this.  What you seem to be proposing
>>> is that psql try to intuit the reason for connection closure from the
>>> last error message it got, but that seems likely to lead to worse lies
>>> than printing a boilerplate message.
>>>
>>> I could go along with just dropping the last sentence ("This probably...")
>>> if the last error we got was FATAL level.  I don't find "unexpectedly"
>>> to be problematic here: from the point of view of psql, and probably
>>> of its user, the shutdown *was* unexpected.
>>
>> I don't care very much whether we try to intuit the reason for
>> connection closure or not; it could be done, but I don't feel that it
>> has to be done.  My bigger point is that currently psql speculates
>> that the reason for *every* connection closure is abnormal server
>> termination, which is actually a very rare event.
>>
>> It may have been common when that message was added.
>> 1a17447be1186fdd36391c58a2a0209f613d89c4 changed the wording this
>> message in 2001, and the original message seems to date to
>> 011ee13131f6fa2f6dbafd3827b70d051cb28f64 in 1996.  And my guess is at
>> that time the server probably did just roll over and die with some
>> regularity.  But today it usually doesn't.  It's neither helpful nor
>> good PR for libpq to guess that the most likely cause of a server
>> disconnection is server unreliability.
>>
>> I have seen actual instances of customers getting upset by this
>> message even though the server had been shut down quite cleanly.  The
>> message got into a logfile and induced minor panic.  Fortunately, I
>> have not seen this happen lately.
> 
> +1 for making this error message less frightening.  I have also had to
> explain it away on occasion.
> 

+1 I've seen this being misleading way too often.

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


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

Re: [HACKERS] Tuplesort merge pre-reading

[Heikki Linnakangas]

On 09/28/2016 07:11 PM, Peter Geoghegan wrote:

On Wed, Sep 28, 2016 at 5:04 PM, Heikki Linnakangas  wrote:

Not sure that I understand. I agree that each merge pass tends to use
roughly the same number of tapes, but the distribution of real runs on
tapes is quite unbalanced in earlier merge passes (due to dummy runs).
It looks like you're always using batch memory, even for non-final
merges. Won't that fail to be in balance much of the time because of
the lopsided distribution of runs? Tapes have an uneven amount of real
data in earlier merge passes.



How does the distribution of the runs on the tapes matter?


The exact details are not really relevant to this discussion (I think
it's confusing that we simply say "Target Fibonacci run counts",
FWIW), but the simple fact that it can be quite uneven is.


Well, I claim that the fact that the distribution of runs is uneven, 
does not matter. Can you explain why you think it does?



This is why I never pursued batch memory for non-final merges. Isn't
that what you're doing here? You're pretty much always setting
"state->batchUsed = true".


Yep. As the patch stands, we wouldn't really need batchUsed, as we know 
that it's always true when merging, and false otherwise. But I kept it, 
as it seems like that might not always be true - we might use batch 
memory when building the initial runs, for example - and because it 
seems nice to have an explicit flag for it, for readability and 
debugging purposes.



I'm basically repeating myself here, but: I think it's incorrect that
LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
generally, it is questionable that it is called in such a high level
routine, rather than the start of a specific merge pass -- I said so a
couple of times already).



You can't release the tape buffer at the end of a pass, because the buffer
of a tape will already be filled with data from the next run on the same
tape.


Okay, but can't you just not use batch memory for non-final merges,
per my initial approach? That seems far cleaner.


Why? I don't see why the final merge should behave differently from the 
non-final ones.


- Heikki



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

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Tom Lane]

Robert Haas  writes:
> On Thu, Sep 22, 2016 at 1:37 PM, Tom Lane  wrote:
>> OK, I'll think about how to do that more efficiently.  The smaller
>> incremental improvement isn't surprising, because in this example the
>> index would still be 90-something MB if it had no free space at all,
>> so there's going to be decreasing returns from any additional work
>> to avoid wasted free space.  But if we can do it cheaply, this does
>> suggest that using pages in order by free space is of value.

> Tom, are you planning to do something about this patch yet this
> CommitFest, or leave it until later?

I doubt I will get to it this week, so let's mark it RWF for this fest.

regards, tom lane


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

Re: [HACKERS] Binary I/O for isn extension

[Shay Rojansky]

Sorry about this, I just haven't had a free moment (and it's definitely not
very high priority...)

On Wed, Sep 28, 2016 at 5:04 PM, Robert Haas  wrote:

> On Mon, Aug 22, 2016 at 8:14 AM, Fabien COELHO 
> wrote:
> > Hello Shay,
> >> Attached is a new version of the patch, adding an upgrade script and the
> >> rest of it. Note that because, as Fabien noted, there's doesn't seem to
> be
> >> a way to add send/receive functions with ALTER TYPE, I did that by
> >> updating
> >> pg_type directly - hope that's OK.
> >
> > This patch does not apply anymore, because there as been an update in
> > between to mark relevant contrib functions as "parallel".
> >
> > Could you update the patch?
>
> So, it's been over a month since this request, and there doesn't seem
> to be an update to this patch.  The CommitFest is over in 2 days, so
> I've marked this "Returned with Feedback".  Shay, please feel free to
> resubmit for the next CommitFest.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

[HACKERS] Batches, error handling and transaction in the protocol

[Shay Rojansky]

Hi everyone, I'd appreciate some guidance on an issue that's been raised
with Npgsql, input from other driver writers would be especially helpful.

Npgsql currently supports batching (or pipelining) to avoid roundtrips, and
sends a Sync message only at the end of the batch (so
Parse1/Bind1/Describe1/Execute1/Parse2/Bind2/Describe2/Execute2/Sync). The
reasoning is that if the first statement in the batch fails, the others
shouldn't be processed. This seems to be the standard approach (the
proposed patch for libpq seems to do the same).

At the same time, if the batch doesn't occur within an explicit transaction
(i.e. after BEGIN), it is automatically wrapped in an implicit transaction,
with Sync committing it. This can, for example, provoke deadlocks if two
batches try to update the same rows in reverse order. The problem is that
the user didn't request a transaction in any way - they're just using
batching to avoid roundtrips and their intention is to be in autocommit
mode.

One possible solution for this would be to insert a Sync after every
execute in the batch, rather than a single Sync at the very end. This would
make batches work the same as unbatched statements, and would resolve the
deadlocks. However, behavior in case of error would be problematic:
PostgreSQL would continue executing later messages if earlier ones failed,
Npgsql would have to deal with multiple errors, etc.

More generally speaking, the protocol appears to couple two different
things which may be unrelated. On the one hand, we have a protocol sync
mechanism for error recovery (skip until Sync). One the other hand, we have
an implicit transaction for extended query messages until that same Sync.
It seems valid to want to have error recovery without an implicit
transaction, but this doesn't seem supported by the current protocol (I
could add a note for v4).

Finally, to give more context, a Microsoft developer ran into this while
running ASP.NET benchmarks over Npgsql and its Entity Framework Core ORM
provider. One of EFCore's great new features is that it batches database
updates into a single roundtrip, but this triggered deadlocks. Whereas in
many cases it's OK to tell users to solve the deadlocks by properly
ordering their statements, when an ORM is creating the batch it's a more
difficult proposition.

Thanks for any thoughts or guidance!

Shay

[HACKERS] CommitFest wrap-up

[Robert Haas]

As some of you probably noticed, I just made a sweep through
everything that was marked "Waiting on Author" in the CommitFest and
hadn't been updated in the last couple of days.  Most of those I
marked as "Returned with Feedback", but some of them got some other
status, one I committed, and a few I just sent a ping of some sort to
the thread.

With that cleanup, things now look like this:

Needs review: 46. Waiting on Author: 21. Ready for Committer: 18.
Committed: 94. Moved to next CF: 1. Rejected: 12. Returned with
Feedback: 27. Total: 219.

There is obviously a good bit of stuff that has been marked "Ready for
Committer"; it would be good if committers could take a look at those
and see if they agree that a commit might be possible without undue
effort.

There is also a lot of stuff that is still in a "Needs Review" state.
I suspect a good amount of that stuff has actually had some review,
and if somebody wants to help, it would be great to go through those
entries and change the status of any of them that are not actually
waiting for review - i.e. if they have been reviewed and are awaiting
an update, mark them as "Waiting on Author".  This will help us
separate the things that still really deserve a look from the stuff
that has already had one.

The things that haven't had any review yet should get a review if
that's at all possible.

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


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

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Robert Haas]

On Thu, Sep 22, 2016 at 1:37 PM, Tom Lane  wrote:
> OK, I'll think about how to do that more efficiently.  The smaller
> incremental improvement isn't surprising, because in this example the
> index would still be 90-something MB if it had no free space at all,
> so there's going to be decreasing returns from any additional work
> to avoid wasted free space.  But if we can do it cheaply, this does
> suggest that using pages in order by free space is of value.

Tom, are you planning to do something about this patch yet this
CommitFest, or leave it until later?

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


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

Re: [HACKERS] Better tracking of free space during SP-GiST index build

[Robert Haas]

On Sun, Sep 25, 2016 at 3:28 PM, Tomas Vondra
 wrote:
> Sure, that would be useful.
>
> I think it would be useful to make repository of such data sets, so that
> patch authors & reviewers can get a reasonable collection of data sets if
> needed, instead of scrambling every time. Opinions?

In theory, great idea.  In practice, I suspect the problem will be
that nobody will know what the use case for a particular data set was
supposed to be, and therefore it'll become a collection of files
nobody knows what to do with.

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


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

Re: [HACKERS] less expensive pg_buffercache on big shmem

[Robert Haas]

On Tue, Sep 20, 2016 at 7:43 PM, Tomas Vondra
 wrote:
> On 09/02/2016 11:01 AM, Robert Haas wrote:
>>
>> On Fri, Sep 2, 2016 at 8:49 AM, Andres Freund  wrote:
>>>
>>> On 2016-09-02 08:31:42 +0530, Robert Haas wrote:

 I wonder whether we ought to just switch from the consistent method to
 the semiconsistent method and call it good.
>>>
>>>
>>> +1. I think, before long, we're going to have to switch away from having
>>> locks & partitions in the first place. So I don't see a problem relaxing
>>> this. It's not like that consistency really buys you anything...  I'd
>>> even consider not using any locks.
>>
>> I think we certainly want to lock the buffer header, because otherwise
>> we might get a torn read of the buffer tag, which doesn't seem good.
>> But it's not obvious to me that there's any point in taking the lock
>> on the buffer mapping partition; I'm thinking that doesn't really do
>> anything unless we lock them all, and we all seem to agree that's
>> going too far.
>
> +1 from me to only locking the buffer headers. IMHO that's perfectly fine
> for the purpose of this extension.

So, I think we have agreement on the way forward here, but what we
don't have is a committable patch.  I'm willing to commit one before
the end of this CommitFest if somebody produces one RSN; otherwise,
this is going to have to go into the "Returned with Feedback" bucket.

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


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

Re: [HACKERS] Set log_line_prefix and application name in test drivers

[Robert Haas]

On Thu, Sep 15, 2016 at 5:18 PM, Robert Haas  wrote:
> On Sat, Aug 27, 2016 at 3:59 PM, Tom Lane  wrote:
>> Christoph Berg  writes:
>>> I've always been wondering why we don't set a log_line_prefix by
>>> default.
>>
>> I think the odds of getting to something that everyone would agree on
>> are nil, so I'm not excited about getting into that particular
>> bikeshed-painting discussion.  Look at the amount of trouble we're
>> having converging on a default for the regression tests, which are
>> a far narrower use-case than "everybody".
>
> Well, practically anything that includes a PID and the timestamp is
> going to be an improvement over the status quo.  Just because we can't
> all agree on what would be perfect does not mean that we can't do
> better than what we've got now.  +1 for trying.

Is there any chance we can move forward here, or is this effort doomed for now?

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


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

Re: [HACKERS] PassDownLimitBound for ForeignScan/CustomScan

[Robert Haas]

On Tue, Sep 13, 2016 at 9:07 PM, Kouhei Kaigai  wrote:
> It is because of just my time pressure around the patch submission days.
> I'll try to enhance postgres_fdw as a usage of this run-time optimization.

Time has (pretty much) expired for this CommitFest.  In any case, this
will amount to a whole new patch, not just a rework of the current
one.  So I'm going to mark this "Rejected" in the CommitFest, and I
suggest you start a new thread for the proposed approach if you get a
chance to work on it.

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


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

Re: [HACKERS] identity columns

[Robert Haas]

On Mon, Sep 12, 2016 at 5:02 PM, Peter Eisentraut
 wrote:
> Thank you for this extensive testing.  I will work on getting the bugs
> fixed.

It looks like the patch has not been updated; since the CommitFest is
(hopefully) wrapping up, I am marking this "Returned with Feedback"
for now.

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


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

Re: [HACKERS] pgbench more operators & functions

[Jeevan Ladhe]

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 looks good to me now.
Passing this to committer.

The new status of this patch is: Ready for Committer

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

Re: [HACKERS] Index Onlys Scan for expressions

[Robert Haas]

On Thu, Sep 8, 2016 at 2:58 PM, Vladimir Sitnikov
 wrote:
> Ildar> Could you please try the patch and tell if it works for you?
>
> I've tested patch6 against recent head. The patch applies with no problems.
>
> The previous case (filter on top of i-o-s) is fixed. Great work.
>
> Here are the test cases and results:
> https://gist.github.com/vlsi/008e18e18b609fcaaec53d9cc210b7e2
>
> However, it looks there are issues when accessing non-indexed columns.
> The error is "ERROR: variable not found in subplan target list"
> The case is 02_case2_fails.sql (see the gist link above)
>
> The essence of the case is "create index on substr(vc, 1, 128)"
> and assume that majority of the rows have length(vc)<128.
> Under that conditions, it would be nice to do index-only-scan
> and filter (like in my previous case), but detect "long" rows
> and do additional recheck for them.

Based on this report, this patch appears to have bugs that would
preclude committing it, so I'm marking it "Returned with Feedback" for
this CommitFest, which is due to end shortly.  Ildar, please feel free
to resubmit once you've updated the patch.

FWIW, I think this is a good effort and hope to see it move forward.

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


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

Re: [HACKERS] Change error code for hstore syntax error

[Sherrylyn Branchaw]

Since no revised patch has been forthcoming and the CommitFest is due
to end shortly, I've marked this "Returned with Feedback".  Sherrylyn,
please feel free to update the patch and resubmit to the next
CommitFest.

Will do, Robert, and many thanks to Marko for the feedback. I apologize for
the delay; I had surgery two days ago and will get back to this as soon as
possible.

Sherrylyn

Re: [HACKERS] Sample configuration files

[Robert Haas]

On Thu, Sep 8, 2016 at 8:52 AM, Tom Lane  wrote:
> Vik Fearing  writes:
>> I noticed that this patch has been marked Waiting on Author with no
>> comment.  Peter, what more should I be doing right now while waiting for
>> Martín's review?
>
> FWIW, I agree with the upthread misgivings about whether this is actually
> a useful effort.  Even if we installed the sample config files somewhere
> (something there is not consensus for AFAICT), they would not actually
> *do* anything useful as standalone files.  I suppose you are imagining
> that people would either manually concatenate them onto postgresql.conf
> or insert an include directive for them into postgresql.conf, but neither
> of those things sound pleasant or maintainable.
>
> Moreover, it's not clear why anyone would do that at all in the age of
> ALTER SYSTEM SET.
>
> I suggest that it'd be more fruitful to view this as a documentation
> effort; that is, in each contrib module's SGML documentation file provide
> a standardized section listing all its parameters and their default
> settings.  That would be something that could be copied-and-pasted from
> into either an editor window on postgresql.conf for the old guard, or
> an ALTER SYSTEM SET command for the new.

So, tallying up the votes, one person has spoken in favor of this
(Martín Marqués) and two against it (Tom Lane and Robert Haas).  One
presumes the author is also in favor, so that's a 2-2 tie.  That's not
exactly a consensus against this effort, but it's not a ringing
endorsement, either.  It's hard for me to imagine anything getting
committed here unless some more people think it's a good idea.

So, anyone else have an opinion, pro or con?

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


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

Re: [HACKERS] Truncating/vacuuming relations on full tablespaces

[Robert Haas]

On Thu, Sep 8, 2016 at 2:46 AM, Haribabu Kommi  wrote:
> Patch needs rebase, it is failing to apply on latest master.
> And also there are some pending comment fix from Robert.

It's been almost three weeks and this hasn't been updated, so I think
it's pretty clear that it should be marked "Returned with Feedback" at
this point.  I'll go do that.  Asif, if you update the patch, you can
resubmit for the next CommitFest.  Please make sure that all review
comments already given are addressed in your next revision so that
reviewers don't waste time giving you the same comments again.

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


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

Re: [HACKERS] [PATCH] add option to pg_dumpall to exclude tables from the dump

[Robert Haas]

On Tue, Sep 6, 2016 at 9:37 PM, Gerdan Rezende dos Santos
 wrote:
> After review, I realized that there is a call to the function:
> doShellQuoting (pgdumpopts, OPTARG), which no longer seems to exist ...
> After understand the code, I saw that the call is appendShellString
> (pgdumpopts, OPTARG).
>
> Follow the patches already with the necessary corrections.

This doesn't seem to take into account the discussion between Tom Lane
and Jim Nasby about how this feature should work.

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


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

Re: [HACKERS] pg_basebackup stream xlog to tar

[Magnus Hagander]

On Sep 28, 2016 19:11, "Robert Haas"  wrote:
>
> On Mon, Sep 5, 2016 at 4:01 AM, Michael Paquier
>  wrote:
> > [ review comments ]
>
> This thread has been sitting idle for more than 3 weeks, so I'm
> marking it "Returned with Feedback" in the CommitFest application.
> Magnus, Michael's latest round of comments seem pretty trivial, so
> perhaps you want to just fix whichever of them seem to you to have
> merit and commit without waiting for the next CommitFest.  Or, you can
> resubmit for the next CommitFest if you think it needs more review.
> But the CommitFest is just about over so it's time to clean out old
> entries, one way or the other.

Yeah, understood. I was planning to get back to it this week, but failed to
find the time. I'll still have some hope about later this week, but most
likely not until the next.

/Magnus

Re: [HACKERS] Proposal for changes to recovery.conf API

[Robert Haas]

On Tue, Sep 6, 2016 at 10:11 AM, David Steele  wrote:
> On 9/6/16 8:07 AM, Robert Haas wrote:
>> On Wed, Aug 31, 2016 at 9:45 PM, Simon Riggs 
>> wrote:
>>> Related cleanup
>>> * Promotion signal file is now called "promote.trigger" rather than
>>> just "promote"
>>> * Remove user configurable "trigger_file" mechanism - use
>>> "promote.trigger" for all cases
>>
>>
>> I'm in favor of this.  I don't think that it's very hard for authors
>> of backup tools to adapt to this new world, and I don't see that
>> allowing configurability here does anything other than create more
>> cases to worry about.
>
> +1 from a backup tool author.

It's time to wrap up this CommitFest, and this thread doesn't seem to
contain anything that looks like a committable patch.  So, I'm marking
this "Returned with Feedback".  I hope that the fact that there's been
no discussion for the last three weeks doesn't mean this effort is
dead; I would like very much to see it move forward.

Thanks,

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


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

Re: [HACKERS] pg_basebackup stream xlog to tar

[Robert Haas]

On Mon, Sep 5, 2016 at 4:01 AM, Michael Paquier
 wrote:
> [ review comments ]

This thread has been sitting idle for more than 3 weeks, so I'm
marking it "Returned with Feedback" in the CommitFest application.
Magnus, Michael's latest round of comments seem pretty trivial, so
perhaps you want to just fix whichever of them seem to you to have
merit and commit without waiting for the next CommitFest.  Or, you can
resubmit for the next CommitFest if you think it needs more review.
But the CommitFest is just about over so it's time to clean out old
entries, one way or the other.

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


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

Re: [HACKERS] Change error code for hstore syntax error

[Robert Haas]

On Sun, Sep 4, 2016 at 7:15 PM, Marko Tiikkaja  wrote:
> On 2016-05-09 19:42, Sherrylyn Branchaw wrote:
>>
>> I'm attaching a revised patch; please let me know if there are any other
>> issues before I submit to the commitfest.
>
> I think this is mostly good, but these two should be changed:
>
>   errmsg("unexpected end of string: \"%s\"", state->begin)
>   errmsg("syntax error at position %d: \"%s\"", ...)
>
> Right now, aside from the error code, these two look like they're reporting
> about an error in the SQL statement itself, and not in an input value for a
> type.  I think they should look more like this:
>
>   errmsg("invalid input syntax for type hstore: \"%s\"", string),
>   errdetail("Unexpected end of input.")
>
> If possible, it might also make sense to provide more information than
> "unexpected end of string".  For example: what character were you expecting
> to find, or what were you scanning?  I didn't look too closely what exactly
> could be done here.  I'll leave that part to you.

Since no revised patch has been forthcoming and the CommitFest is due
to end shortly, I've marked this "Returned with Feedback".  Sherrylyn,
please feel free to update the patch and resubmit to the next
CommitFest.

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


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

Re: [HACKERS] Binary I/O for isn extension

[Robert Haas]

On Mon, Aug 22, 2016 at 8:14 AM, Fabien COELHO  wrote:
> Hello Shay,
>> Attached is a new version of the patch, adding an upgrade script and the
>> rest of it. Note that because, as Fabien noted, there's doesn't seem to be
>> a way to add send/receive functions with ALTER TYPE, I did that by
>> updating
>> pg_type directly - hope that's OK.
>
> This patch does not apply anymore, because there as been an update in
> between to mark relevant contrib functions as "parallel".
>
> Could you update the patch?

So, it's been over a month since this request, and there doesn't seem
to be an update to this patch.  The CommitFest is over in 2 days, so
I've marked this "Returned with Feedback".  Shay, please feel free to
resubmit for the next CommitFest.

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


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

Re: [HACKERS] [PATCH] SortSupport for macaddr type

[Robert Haas]

On Wed, Sep 14, 2016 at 6:14 AM, Julien Rouhaud
 wrote:
> On 26/08/2016 19:44, Brandur wrote:
>> Hello,
> Hello,
>
>> I've attached a patch to add SortSupport for Postgres' macaddr which has the
>> effect of improving the performance of sorting operations for the type. The
>> strategy that I employ is very similar to that for UUID, which is to create
>> abbreviated keys by packing as many bytes from the MAC address as possible 
>> into
>> Datums, and then performing fast unsigned integer comparisons while sorting.
>>
>> I ran some informal local benchmarks, and for cardinality greater than 100k
>> rows, I see a speed up on `CREATE INDEX` of roughly 25% to 65%. (For those
>> interested, I put a few more numbers into a small report here [2].)
>>
>
> That's a nice improvement!
>
>> Admittedly, this is not quite as useful as speeding up sorting on a more 
>> common
>> data type like TEXT or UUID, but the change still seems like a useful
>> performance improvement. I largely wrote it as an exercise to familiarize
>> myself with the Postgres codebase.
>>
>> I'll add an entry into the current commitfest as suggested by the Postgres 
>> Wiki
>> and follow up here with a link.
>>
>> Thanks, and if anyone has feedback or other thoughts, let me know!
>>
>
> I just reviewed your patch.  It applies and compiles cleanly, and the
> abbrev feature works as intended.  There's not much to say since this is
> heavily inspired on the uuid SortSupport. The only really specific part
> is in the abbrev_converter function, and I don't see any issue with it.
>
> I have a few trivial comments:
>
> * you used macaddr_cmp_internal() function name, for uuid the same
> function is named uuid_internal_cmp().  Using the same naming pattern is
> probably better.
>
> * the function comment on macaddr_abbrev_convert() doesn't mention
> specific little-endian handling
>
> * "There will be two bytes of zero padding on the least significant end"
>
> "least significant bits" would be better
>
> * This patch will trigger quite a lot modifications after a pgindent
> run.  Could you try to run pgindent on mac.c before sending an updated
> patch?

Since it's been two weeks and this patch hasn't been updated in
response to this review, I have marked it "Returned with Feedback" in
the CommitFest.  If it is updated, it can be resubmitted for the next
CommitFest.

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


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

Re: [HACKERS] hstore: add hstore_length function

[Robert Haas]

On Wed, Jun 8, 2016 at 10:44 AM, Robert Haas  wrote:
> On Mon, Jun 6, 2016 at 7:57 PM, Korbin Hoffman  wrote:
>> With regards to your second point- I've been maintaining consistency
>> with the rest of the hstore module. Hstore's _size is internally
>> stored as a uint, but all uses of HS_COUNT across the feature end up
>> stored in a signed int. I could only find (grep) a few occurrences of
>> PG_RETURN_UINT32 across the entire codebase, and none in the hstore
>> module. If there's strong consensus for change, though, I'm happy to
>> do so.
>
> The PG_RETURN_BLAH macro chosen should match the declared return type
> of that function.  So if your function, for example, returns int4 (or
> integer, which is the same thing), PG_RETURN_INT32 is correct.
>
> There are no built-in SQL datatypes for unsigned integers, which is
> why you did not find many uses of PG_RETURN_UINT32 in the code base.

Since this patch was never updated in response to this review, I am
marking it "Returned with Feedback" in this CommitFest.  If it is
updated, it can be resubmitted to a future CommitFest.

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


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

Re: [HACKERS] Showing parallel status in \df+

[Tom Lane]

Pavel Stehule  writes:
> 2016-09-28 16:03 GMT+02:00 Tom Lane :
>> I propose to push my current patch (ie, move PL function
>> source code to \df+ footers), and we can use it in HEAD for awhile
>> and see what we think.  We can alway improve or revert it later.

> I had some objection to format of source code - it should be full source
> code, not just header and body.

That would be redundant with stuff that's in the main part of the \df
display.  I really don't need to see the argument types twice, for instance.

regards, tom lane


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

Re: [HACKERS] IF (NOT) EXISTS in psql-completion

[Robert Haas]

On Tue, Sep 20, 2016 at 3:50 AM, Michael Paquier
 wrote:
> On Mon, Sep 19, 2016 at 6:11 PM, Pavel Stehule  
> wrote:
>> I am thinking so commit's description should be inside README
>
> Horiguchi-san, your patch has some whitespace issues, you may want to
> get a run with git diff --check. Here are some things I have spotted:
> src/bin/psql/tab-complete.c:1074: trailing whitespace.
> +"MATERIALIZED VIEW",
> src/bin/psql/tab-complete.c:2621: trailing whitespace.
> +   COMPLETE_WITH_QUERY(Query_for_list_of_roles,
>
> This set of patches is making psql tab completion move into a better
> shape, particularly with 0001 that removes the legendary huge if-elif
> and just the routine return immediately in case of a keyword match.
> Things could be a little bit more shortened by for example not doing
> the refactoring of the tab macros because they are just needed in
> tab-complete.c. The other patches introduce further improvements for
> the existing infrastructure, but that's a lot of things just for
> adding IF [NOT] EXISTS to be honest.
>
> Testing a bit, I have noticed that for example trying to after typing
> "create table if", if I attempt to do a tab completion "not exists"
> does not show up. I suspect that the other commands are failing at
> that as well.

This patch hasn't been updated in over a week and we're just about out
of time for this CommitFest, so I've marked it "Returned with
Feedback" for now.  If it gets updated, it can be resubmitted for the
next CommitFest.

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


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

Re: [HACKERS] Issue with bgworker, SPI and pgstat_report_stat

[Robert Haas]

On Sat, Sep 3, 2016 at 12:29 AM, Michael Paquier
 wrote:
> On Sat, Sep 3, 2016 at 10:02 AM, Tomas Vondra
>  wrote:
>> In any case, I think adding the pgstat_report_stat() into worker_spi seems
>> like a reasonable (and backpatchable) fix.
>
> Doing just that sounds reasonable seen from here. I am wondering also
> if it would not be worth mentioning in the documentation of the
> bgworkers that users trying to emulate somewhat the behavior of a
> backend should look at PostgresMain(). The code in itself is full of
> hints as well.

Everybody seems happy with this fix for a first step, so I've
committed it and back-patched it to 9.3.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: pg_ctl: Detect current standby state from pg_control

[Peter Eisentraut]

On 9/28/16 12:44 AM, Michael Paquier wrote:
> On Tue, Sep 27, 2016 at 9:55 AM, Michael Paquier
>  wrote:
>> > Seems overcomplicated to me. How about returning the control file all
>> > the time and let the caller pfree the result? You could then use
>> > crc_ok in pg_ctl.c's get_control_dbstate() to do the decision-making.
> In short I would just go with the attached and call it a day.

Pushed that way.  Thanks!

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


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

Re: [HACKERS] compiler warning read_objtype_from_string()

[Alvaro Herrera]

Peter Eisentraut wrote:
> I'm getting the following compiler warning (using nondefault
> optimization options):
> 
> objectaddress.c: In function 'read_objtype_from_string':
> objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
> function [-Werror=maybe-uninitialized]
>   return type;

Umm.  I think it can only be uninitialized if we fall out of the end of
the array, in which case we're supposed to throw the ERROR and never
return.  Is that not working?

> The comment for the function says
> 
>  * Return ObjectType for the given object type as given by
>  * getObjectTypeDescription; if no valid ObjectType code exists, but it's a
>  * possible output type from getObjectTypeDescription, return -1.
> 
> But the claim that it can return -1 does not seem supported by the code.

Actually, it is -- but the -1 value comes from the ObjectType array.
Perhaps the comment should state that explicitely.

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


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

Re: [HACKERS] "Some tests to cover hash_index"

[Robert Haas]

On Tue, Sep 20, 2016 at 2:26 PM, Alvaro Herrera
 wrote:
> Why not use generate_series() queries to insert the appropriate number
> of tuples, instead of a handful of INSERT lines each time?  Since each
> insert is a separate transaction, that would probably be faster.
>
> Why do you have a plpgsql function just to create a cursor?  Wouldn't it
> be simpler to create the cursor in an SQL statement?

This patch hasn't been updated in over a week, so I'm marking it
Returned with Feedback.  I think this is a good effort and I hope
something committable will come from it, but with 2 days left it's not
going to happen this CF.

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


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

Re: [HACKERS] Tuplesort merge pre-reading

[Peter Geoghegan]

On Wed, Sep 28, 2016 at 5:11 PM, Peter Geoghegan  wrote:
> This is why I never pursued batch memory for non-final merges. Isn't
> that what you're doing here? You're pretty much always setting
> "state->batchUsed = true".

Wait. I guess you feel you have to, since it wouldn't be okay to use
almost no memory per tape on non-final merges.

You're able to throw out so much code here in large part because you
give almost all memory over to logtape.c (e.g., you don't manage each
tape's share of "slots" anymore -- better to give everything to
logtape.c). So, with your patch, you would actually only have one
caller tuple in memory at once per tape for a merge that doesn't use
batch memory (if you made it so that a merge *could* avoid the use of
batch memory, as I suggest).

In summary, under your scheme, the "batchUsed" variable contains a
tautological value, since you cannot sensibly not use batch memory.
(This is even true with !state->tuples callers).

Do I have that right? If so, this seems rather awkward. Hmm.

-- 
Peter Geoghegan


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

Re: [HACKERS] Tuplesort merge pre-reading

[Peter Geoghegan]

On Wed, Sep 28, 2016 at 5:04 PM, Heikki Linnakangas  wrote:
>> Not sure that I understand. I agree that each merge pass tends to use
>> roughly the same number of tapes, but the distribution of real runs on
>> tapes is quite unbalanced in earlier merge passes (due to dummy runs).
>> It looks like you're always using batch memory, even for non-final
>> merges. Won't that fail to be in balance much of the time because of
>> the lopsided distribution of runs? Tapes have an uneven amount of real
>> data in earlier merge passes.
>
>
> How does the distribution of the runs on the tapes matter?

The exact details are not really relevant to this discussion (I think
it's confusing that we simply say "Target Fibonacci run counts",
FWIW), but the simple fact that it can be quite uneven is.

This is why I never pursued batch memory for non-final merges. Isn't
that what you're doing here? You're pretty much always setting
"state->batchUsed = true".

>> I'm basically repeating myself here, but: I think it's incorrect that
>> LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
>> generally, it is questionable that it is called in such a high level
>> routine, rather than the start of a specific merge pass -- I said so a
>> couple of times already).
>
>
> You can't release the tape buffer at the end of a pass, because the buffer
> of a tape will already be filled with data from the next run on the same
> tape.

Okay, but can't you just not use batch memory for non-final merges,
per my initial approach? That seems far cleaner.

-- 
Peter Geoghegan


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

Re: [HACKERS] Tuplesort merge pre-reading

[Heikki Linnakangas]

On 09/28/2016 06:05 PM, Peter Geoghegan wrote:

On Thu, Sep 15, 2016 at 9:51 PM, Heikki Linnakangas  wrote:

I don't think it makes much difference in practice, because most merge
passes use all, or almost all, of the available tapes. BTW, I think the
polyphase algorithm prefers to do all the merges that don't use all tapes
upfront, so that the last final merge always uses all the tapes. I'm not
100% sure about that, but that's my understanding of the algorithm, and
that's what I've seen in my testing.


Not sure that I understand. I agree that each merge pass tends to use
roughly the same number of tapes, but the distribution of real runs on
tapes is quite unbalanced in earlier merge passes (due to dummy runs).
It looks like you're always using batch memory, even for non-final
merges. Won't that fail to be in balance much of the time because of
the lopsided distribution of runs? Tapes have an uneven amount of real
data in earlier merge passes.


How does the distribution of the runs on the tapes matter?


+   usedBlocks = 0;
+   for (tapenum = 0; tapenum < state->maxTapes; tapenum++)
+   {
+   int64   numBlocks = blocksPerTape + (tapenum < remainder ? 1 : 0);
+
+   if (numBlocks > MaxAllocSize / BLCKSZ)
+   numBlocks = MaxAllocSize / BLCKSZ;
+   LogicalTapeAssignReadBufferSize(state->tapeset, tapenum,
+   numBlocks * BLCKSZ);
+   usedBlocks += numBlocks;
+   }
+   USEMEM(state, usedBlocks * BLCKSZ);


I'm basically repeating myself here, but: I think it's incorrect that
LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
generally, it is questionable that it is called in such a high level
routine, rather than the start of a specific merge pass -- I said so a
couple of times already).


You can't release the tape buffer at the end of a pass, because the 
buffer of a tape will already be filled with data from the next run on 
the same tape.


- Heikki



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

Re: [HACKERS] Cache Hash Index meta page.

[Mithun Cy]

On Tue, Sep 27, 2016 at 1:53 AM, Jeff Janes  wrote:
 > I think that this needs to be updated again for v8 of concurrent and v5
of wal

Adding the rebased patch over [1] + [2]

[1] Concurrent Hash index.

[2] Wal for hash index.


-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


cache_hash_index_metapage_onAmit_05_03_with_wall.patch
Description: Binary data

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

[HACKERS] compiler warning read_objtype_from_string()

[Peter Eisentraut]

I'm getting the following compiler warning (using nondefault
optimization options):

objectaddress.c: In function 'read_objtype_from_string':
objectaddress.c:2309:9: error: 'type' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
  return type;

The comment for the function says

 * Return ObjectType for the given object type as given by
 * getObjectTypeDescription; if no valid ObjectType code exists, but it's a
 * possible output type from getObjectTypeDescription, return -1.

But the claim that it can return -1 does not seem supported by the code.

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


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

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[Robert Haas]

On Tue, Sep 27, 2016 at 5:15 PM, Tomas Vondra
 wrote:
> So, I got the results from 3.10.101 (only the pgbench data), and it looks
> like this:
>
>  3.10.101   1  8 16 32 64128192
> 
>  granular-locking2582  18492  33416  49583  53759  53572  51295
>  no-content-lock 2580  18666  33860  49976  54382  54012  51549
>  group-update2635  18877  33806  49525  54787  54117  51718
>  master  2630  18783  33630  49451  54104  53199  50497
>
> So 3.10.101 performs even better tnan 3.2.80 (and much better than 4.5.5),
> and there's no sign any of the patches making a difference.

I'm sure that you mentioned this upthread somewhere, but I can't
immediately find it.  What scale factor are you testing here?

It strikes me that the larger the scale factor, the more
CLogControlLock contention we expect to have.  We'll pretty much do
one CLOG access per update, and the more rows there are, the more
chance there is that the next update hits an "old" row that hasn't
been updated in a long time.  So a larger scale factor also increases
the number of active CLOG pages and, presumably therefore, the amount
of CLOG paging activity.

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


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

Re: [HACKERS] Remove superuser() checks from pgstattuple

[Stephen Frost]

Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> On Tue, Sep 27, 2016 at 4:43 AM, Stephen Frost  wrote:
> > * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
> >> This is now being obsoleted by the later idea of allowing base installs
> >> from a chain of upgrade scripts.  But if your upgrade scripts contain
> >> ALTER TABLE commands, you will probably still want to write base install
> >> scripts that do a fresh CREATE TABLE instead.
> >
> > I've updated the patch to remove the new base version script and to rely
> > on the changes made by Tom to install the 1.4 version and then upgrade
> > to 1.5.
> >
> > Based on my testing, it appears to all work correctly.
> 
> Same conclusion from here.

Fantastic, thanks for the testing!

> > Updated (much smaller) patch attached.
> 
> I had a look at it, and it is doing the work it claims to do. So I am
> marking it as "Ready for Committer".

Great.  I'm going to go over the whole patch closely again and will push
it soon.

Thanks again!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] assert violation in logical messages serialization

[Robert Haas]

On Tue, Sep 27, 2016 at 10:45 AM, Stas Kelvich  wrote:
> During processing of logical messages in ReorderBufferSerializeChange()
> pointer to ondisk structure isn’t updated after possible reorder buffer 
> realloc by
> ReorderBufferSerializeReserve().
>
> Actual reason spotted by Konstantin Knizhnik.

Thanks for the patch, and congratulations to Konstantin on the spot.
Committed and back-patched to 96.

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


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

Re: [HACKERS] PATCH: Exclude additional directories in pg_basebackup

[David Steele]

On 9/28/16 2:45 AM, Michael Paquier wrote:
> On Tue, Sep 27, 2016 at 11:27 PM, David Steele  wrote:
>> On 9/26/16 2:36 AM, Michael Paquier wrote:
>>
>>> Just a nit:
>>>  
>>> - postmaster.pid
>>> + postmaster.pid and postmaster.opts
>>>  
>>> Having one  block for each file would be better.
>>
>> OK, changed.
> 
> You did not actually change it :)

Oops, this was intended to mean that I changed:

>>> +const char *excludeFile[] =
>>> excludeFiles[]?

> +
> + backup_label and tablespace_map.  If these
> + files exist they belong to an exclusive backup and are not 
> applicable
> + to the base backup.
> +
> This is a bit confusing. When using pg_basebackup the files are
> ignored, right, but they are included in the tar stream so they are
> not excluded at the end. So we had better remove purely this
> paragraph. Similarly, postgresql.auto.conf.tmp is something that
> exists only for a short time frame. Not listing it directly is fine
> IMO.

OK, I agree that's confusing and probably not helpful to the user.

> +   /* If symlink, write it as a directory anyway */
> +#ifndef WIN32
> +   if (S_ISLNK(statbuf->st_mode))
> +#else
> +   if (pgwin32_is_junction(pathbuf))
> +#endif
> +
> +   statbuf->st_mode = S_IFDIR | S_IRWXU;
> Indentation here is confusing. Coverity would complain.

OK.

> +   /* Stat the file */
> +   snprintf(pathbuf, MAXPGPATH, "%s/%s", path, de->d_name);
> There is no need to put the stat() call this early in the loop as this
> is just used for re-creating empty directories.

OK.

> +if (strcmp(pathbuf + basepathlen + 1,
> +   excludeFiles[excludeIdx]) == 0)
> There is no need for complicated maths, you can just use de->d_name.

OK.

> pg_xlog has somewhat a similar treatment, but per the exception
> handling of archive_status it is better to leave its check as-is.

OK.

> The DEBUG1 entries are really useful for debugging, it would be nice
> to keep them in the final patch.

That was my thinking as well.  It's up to Peter, though.

> Thinking harder, your logic can be simplified. You could just do the 
> following:
> - Check for interrupts first
> - Look at the list of excluded files
> - Call lstat()
> - Check for excluded directories

I think setting excludeFound = false the second time is redundant since
it must be false at that point.  Am I missing something or are you just
being cautious in case code changes above?

> After all that fixed, I have moved the patch to "Ready for Committer".
> Please use the updated patch though.

The v6 patch looks good to me.

-- 
-David
da...@pgmasters.net


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

Re: [HACKERS] Tuplesort merge pre-reading

[Peter Geoghegan]

On Thu, Sep 15, 2016 at 9:51 PM, Heikki Linnakangas  wrote:
>> I still don't get why you're doing all of this within mergeruns() (the
>> beginning of when we start merging -- we merge all quicksorted runs),
>> rather than within beginmerge() (the beginning of one particular merge
>> pass, of which there are potentially more than one). As runs are
>> merged in a non-final merge pass, fewer tapes will remain active for
>> the next merge pass. It doesn't do to do all that up-front when we
>> have multiple merge passes, which will happen from time to time.
>
>
> Now that the pre-reading is done in logtape.c, it doesn't stop at a run
> boundary. For example, when we read the last 1 MB of the first run on a
> tape, and we're using a 10 MB read buffer, we will merrily also read the
> first 9 MB from the next run. You cannot un-read that data, even if the tape
> is inactive in the next merge pass.

I've had a chance to think about this some more. I did a flying review
of the same revision that I talk about here, but realized some more
things. First, I will do a recap.

> I don't think it makes much difference in practice, because most merge
> passes use all, or almost all, of the available tapes. BTW, I think the
> polyphase algorithm prefers to do all the merges that don't use all tapes
> upfront, so that the last final merge always uses all the tapes. I'm not
> 100% sure about that, but that's my understanding of the algorithm, and
> that's what I've seen in my testing.

Not sure that I understand. I agree that each merge pass tends to use
roughly the same number of tapes, but the distribution of real runs on
tapes is quite unbalanced in earlier merge passes (due to dummy runs).
It looks like you're always using batch memory, even for non-final
merges. Won't that fail to be in balance much of the time because of
the lopsided distribution of runs? Tapes have an uneven amount of real
data in earlier merge passes.

FWIW, polyphase merge actually doesn't distribute runs based on the
Fibonacci sequence (at least in Postgres). It uses a generalization of
the Fibonacci numbers [1] -- *which* generalization varies according
to merge order/maxTapes. IIRC, with Knuth's P == 6 (i.e. merge order
== 6), it's the "hexanacci" sequence.

The following code is from your latest version, posted on 2016-09-14,
within the high-level mergeruns() function (the function that takes
quicksorted runs, and produces final output following 1 or more merge
passes):

> +   usedBlocks = 0;
> +   for (tapenum = 0; tapenum < state->maxTapes; tapenum++)
> +   {
> +   int64   numBlocks = blocksPerTape + (tapenum < remainder ? 1 : 0);
> +
> +   if (numBlocks > MaxAllocSize / BLCKSZ)
> +   numBlocks = MaxAllocSize / BLCKSZ;
> +   LogicalTapeAssignReadBufferSize(state->tapeset, tapenum,
> +   numBlocks * BLCKSZ);
> +   usedBlocks += numBlocks;
> +   }
> +   USEMEM(state, usedBlocks * BLCKSZ);

I'm basically repeating myself here, but: I think it's incorrect that
LogicalTapeAssignReadBufferSize() is called so indiscriminately (more
generally, it is questionable that it is called in such a high level
routine, rather than the start of a specific merge pass -- I said so a
couple of times already).

It's weird that you change the meaning of maxTapes by reassigning in
advance of iterating through maxTapes like this. I should point out
again that the master branch mergebatch() function does roughly the
same thing as you're doing here as follows:

static void
mergebatch(Tuplesortstate *state, int64 spacePerTape)
{
int srcTape;

Assert(state->activeTapes > 0);
Assert(state->tuples);

/*
 * For the purposes of tuplesort's memory accounting, the batch allocation
 * is special, and regular memory accounting through USEMEM() calls is
 * abandoned (see mergeprereadone()).
 */
for (srcTape = 0; srcTape < state->maxTapes; srcTape++)
{
char   *mergetuples;

if (!state->mergeactive[srcTape])
continue;

/* Allocate buffer for each active tape */
mergetuples = MemoryContextAllocHuge(state->tuplecontext,
 spacePerTape);

/* Initialize state for tape */
state->mergetuples[srcTape] = mergetuples;
state->mergecurrent[srcTape] = mergetuples;
state->mergetail[srcTape] = mergetuples;
state->mergeoverflow[srcTape] = NULL;
}

state->batchUsed = true;
state->spacePerTape = spacePerTape;
}

Notably, this function:

* Works without altering the meaning of maxTapes. state->maxTapes is
Knuth's T, which is established very early and doesn't change with
polyphase merge (same applies to state->tapeRange). What does change
across merge passes is the number of *active* tapes. I don't think
it's necessary to change that in any way. I find it very confusing.
(Also, that you're using state->currentRun here at all seems bad, for
more or less 

Re: [HACKERS] psql casts aspersions on server reliability

[David Steele]

On 9/28/16 10:22 AM, Robert Haas wrote:
> On Wed, Sep 28, 2016 at 9:14 AM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> psql tends to do things like this:
>>> rhaas=# select * from pg_stat_activity;
>>> FATAL:  terminating connection due to administrator command
>>> server closed the connection unexpectedly
>>> This probably means the server terminated abnormally
>>> before or while processing the request.
>>
>>> Basically everything psql has to say about this is a lie:
>>
>> I cannot get terribly excited about this.  What you seem to be proposing
>> is that psql try to intuit the reason for connection closure from the
>> last error message it got, but that seems likely to lead to worse lies
>> than printing a boilerplate message.
>>
>> I could go along with just dropping the last sentence ("This probably...")
>> if the last error we got was FATAL level.  I don't find "unexpectedly"
>> to be problematic here: from the point of view of psql, and probably
>> of its user, the shutdown *was* unexpected.
> 
> I don't care very much whether we try to intuit the reason for
> connection closure or not; it could be done, but I don't feel that it
> has to be done.  My bigger point is that currently psql speculates
> that the reason for *every* connection closure is abnormal server
> termination, which is actually a very rare event.
> 
> It may have been common when that message was added.
> 1a17447be1186fdd36391c58a2a0209f613d89c4 changed the wording this
> message in 2001, and the original message seems to date to
> 011ee13131f6fa2f6dbafd3827b70d051cb28f64 in 1996.  And my guess is at
> that time the server probably did just roll over and die with some
> regularity.  But today it usually doesn't.  It's neither helpful nor
> good PR for libpq to guess that the most likely cause of a server
> disconnection is server unreliability.
> 
> I have seen actual instances of customers getting upset by this
> message even though the server had been shut down quite cleanly.  The
> message got into a logfile and induced minor panic.  Fortunately, I
> have not seen this happen lately.

+1 for making this error message less frightening.  I have also had to
explain it away on occasion.

-- 
-David
da...@pgmasters.net


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