date:20160315

On 3/8/16 9:12 PM, Andreas Karlsson wrote:
> I have one nitpick: why is one of the variables "true" while the other
> is "on" in the example? I think both should be "on".
> 
> #syslog_sequence_numbers = true
> #syslog_split_lines = on
> 
> Another possible improvement would be to change "Split messages sent to
> syslog." to something more verbose like "Split messages sent to syslog,
> by lines and to fit in 1024 bytes.".

Updated patches with your suggestions.  I also renamed
syslog_split_lines to syslog_split_messages, which I think is more accurate.


From 70bacecba46eb38c02c43957c2f1812faf5684df Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 26 Feb 2016 22:34:30 -0500
Subject: [PATCH 1/2] Add syslog_sequence_numbers parameter

---
 doc/src/sgml/config.sgml  | 28 +++
 src/backend/utils/error/elog.c| 12 ++--
 src/backend/utils/misc/guc.c  | 10 ++
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 src/include/utils/elog.h  |  1 +
 5 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6c73fb4..bbe87ce 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4305,6 +4305,34 @@ Where To Log

   
 
+  
+   syslog_sequence_numbers (boolean)
+
+ syslog_sequence_numbers configuration parameter
+
+   
+
+   
+
+ When logging to syslog and this is on (the
+ default), then each message will be prefixed by an increasing
+ sequence number (such as [2]).  This circumvents
+ the --- last message repeated N times --- suppression
+ that many syslog implementations perform by default.  In more modern
+ syslog implementations, repeat message suppression can be configured
+ (for example, $RepeatedMsgReduction
+ in rsyslog), so this might not be
+ necessary.  Also, you could turn this off if you actually want to
+ suppress repeated messages.
+
+
+
+ This parameter can only be set in the postgresql.conf
+ file or on the server command line.
+
+  
+ 
+
  
   event_source (string)
   
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index 5b7554b..88421c7 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -106,6 +106,7 @@ int			Log_error_verbosity = PGERROR_VERBOSE;
 char	   *Log_line_prefix = NULL;		/* format for extra log line info */
 int			Log_destination = LOG_DESTINATION_STDERR;
 char	   *Log_destination_string = NULL;
+bool		syslog_sequence_numbers = true;
 
 #ifdef HAVE_SYSLOG
 
@@ -2018,7 +2019,11 @@ write_syslog(int level, const char *line)
 
 			chunk_nr++;
 
-			syslog(level, "[%lu-%d] %s", seq, chunk_nr, buf);
+			if (syslog_sequence_numbers)
+syslog(level, "[%lu-%d] %s", seq, chunk_nr, buf);
+			else
+syslog(level, "[%d] %s", chunk_nr, buf);
+
 			line += buflen;
 			len -= buflen;
 		}
@@ -2026,7 +2031,10 @@ write_syslog(int level, const char *line)
 	else
 	{
 		/* message short enough */
-		syslog(level, "[%lu] %s", seq, line);
+		if (syslog_sequence_numbers)
+			syslog(level, "[%lu] %s", seq, line);
+		else
+			syslog(level, "%s", line);
 	}
 }
 #endif   /* HAVE_SYSLOG */
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index f0d4ec1..3ef432a 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -1632,6 +1632,16 @@ static struct config_bool ConfigureNamesBool[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"syslog_sequence_numbers", PGC_SIGHUP, LOGGING_WHERE,
+			gettext_noop("Add sequence number to syslog messags to avoid duplicate suppression."),
+			NULL
+		},
+		_sequence_numbers,
+		true,
+		NULL, NULL, NULL
+	},
+
 	/* End-of-list marker */
 	{
 		{NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index ee3d378..b72ea6d 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -358,6 +358,7 @@
 # These are relevant when logging to syslog:
 #syslog_facility = 'LOCAL0'
 #syslog_ident = 'postgres'
+#syslog_sequence_numbers = on
 
 # This is only relevant when logging to eventlog (win32):
 #event_source = 'PostgreSQL'
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 7d338dd..e245b2e 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -397,6 +397,7 @@ extern int	Log_error_verbosity;
 extern char *Log_line_prefix;
 extern int	Log_destination;
 extern char *Log_destination_string;
+extern bool syslog_sequence_numbers;
 
 /* Log destination bitmap */
 #define LOG_DESTINATION_STDERR	 1
-- 
2.7.3

From 66f367f37d3e3d67977ccace5aa3bb9bb8f947f6 Mon Sep 17 00:00:00 2001
From: Peter

Re: [HACKERS] multivariate statistics v14

2016-03-15 Thread Tatsuo Ishii

> Instead of simply multiplying the ndistinct estimate with selecticity,
> we instead use the formula for the expected number of distinct values
> observed in 'k' rows when there are 'd' distinct values in the bin
> 
> d * (1 - ((d - 1) / d)^k)
> 
> This is 'with replacements' which seems appropriate for the use, and it
> mostly assumes uniform distribution of the distinct values. So if the
> distribution is not uniform (e.g. there are very frequent groups) this
> may be less accurate than the current algorithm in some cases, giving
> over-estimates. But that's probably better than OOM.
> ---
>  src/backend/utils/adt/selfuncs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/backend/utils/adt/selfuncs.c 
> b/src/backend/utils/adt/selfuncs.c
> index f8d39aa..6eceedf 100644
> --- a/src/backend/utils/adt/selfuncs.c
> +++ b/src/backend/utils/adt/selfuncs.c
> @@ -3466,7 +3466,7 @@ estimate_num_groups(PlannerInfo *root, List 
> *groupExprs, double input_rows,
>   /*
>* Multiply by restriction selectivity.
>*/
> - reldistinct *= rel->rows / rel->tuples;
> + reldistinct = reldistinct * (1 - powl((reldistinct - 1) 
> / reldistinct,rel->rows));

Why do you change "*=" style? I see no reason to change this.

reldistinct *= 1 - powl((reldistinct - 1) / 
reldistinct, rel->rows);

Looks better to me because it's shorter and cleaner.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


-- 
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] Relaxing SSL key permission checks

On 3/10/16 9:20 PM, Peter Eisentraut wrote:
> On 3/4/16 3:55 PM, Alvaro Herrera wrote:
>> * it failed to check for S_IXUSR, so permissions 0700 were okay, in
>> contradiction with what the error message indicates.  This is a
>> preexisting bug actually.  Do we want to fix it by preventing a
>> user-executable file (possibly breaking compability with existing
>> executable key files), or do we want to document what the restriction
>> really is?
> 
> I think we should not check for S_IXUSR.  There is no reason for doing that.
> 
> I can imagine that key files are sometimes copied around using USB
> drives with FAT file systems or other means of that sort where
> permissions can scrambled.  While I hate gratuitous executable bits as
> much as the next person, insisting here would just create annoyances in
> practice.

I'm happy with this patch except this minor point.  Any final comments?



-- 
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

On 2/5/16 3:09 AM, Kyotaro HORIGUCHI wrote:
> I considered how to make tab-completion robust for syntactical
> noises, in other words, optional words in syntax. Typically "IF
> (NOT) EXISTS", UNIQUE and TEMPORARY are words that don't affect
> further completion.

To repeat the question I raised in the previous commit fest about tab
completion: Why do you want tab completion for IF NOT EXISTS?  When you
tab complete, the completion mechanism will show you whether the item in
question exists.  What is the use case?

-- 
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] insufficient qualification of some objects in dump files

On 2/26/16 1:30 AM, Tom Lane wrote:
> As the patch is presented, I agree with Peter that it does not really
> need a format number bump.  The question that has to be answered is
> whether this solution is good enough?  You could not trust it for
> automated processing of tags --- it's easy to think of cases in which the
> schema/object name separation would be ambiguous.  So is the tag really
> "strictly for human consumption"?  I'm not sure about that.

Well what are those tags for?  They are not used by pg_restore, so they
are for users.  My understanding is that the tags help in editing a TOC
list for use by pg_restore.  What pg_restore actually reads are the
OIDs, but the tags are there so users can edit the files.  The tags can
also be used for ad hoc automatic processing.  They are not sufficiently
delimited and escaped for robustness in all cases, but it can be done if
you control the inputs and know what to expect.  But this is the same
problem before and after my patch.

Both of these cases are helped by my patch, and both of these cases were
pretty broken (for the object classes in question) before my patch.

-- 
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] Choosing parallel_degree

On Tue, Mar 15, 2016 at 8:45 PM, David Rowley
 wrote:
> On 16 March 2016 at 13:26, Julien Rouhaud  wrote:
>> On 15/03/2016 21:12, Robert Haas wrote:
>>> I thought about this a bit more.  There are a couple of easy things we
>>> could do here.
>>>
>>> The 1000-page threshold could be made into a GUC.
>>>
>>> We could add a per-table reloption for parallel-degree that would
>>> override the calculation.
>>>
>>> Neither of those things is very smart, but they'd probably both help
>>> some people.  If someone is able to produce a patch for either or both
>>> of these things *quickly*, we could possibly try to squeeze it into
>>> 9.6 as a cleanup of work already done.
>>>
>>
>> I'm not too familiar with parallel planning, but I tried to implement
>> both in attached patch. I didn't put much effort into the
>> parallel_threshold GUC documentation, because I didn't really see a good
>> way to explain it. I'd e happy to improve it if needed. Also, to make
>> this parameter easier to tune for users, perhaps we could divide the
>> default value by 3 and use it as is in the first iteration in
>> create_parallel_path() ?
>>
>> Also, global max_parallel_degree still needs to be at least 1 for the
>> per table value to be considered.
>
> Thanks for working on this. I've only skimmed the patch so far, but
> will try to look more closely later.
>
> This did get me wondering why we have the parallel_threshold at all,
> and not just allow the parallel_setup_cost to make parallel plans look
> less favourable for smaller relations. I assume that this is so that
> we don't burden the planner with the overhead of generating parallel
> paths for smaller relations?

Right.  And, also, we need some heuristic for judging how many workers
to deploy.  parallel_setup_cost is of no use in making that decision.

-- 
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] Parallel Aggregate

On Tue, Mar 15, 2016 at 9:23 PM, David Rowley
 wrote:
>>> Should I update the patch to use the method you describe?
>>
>> Well, my feeling is that is going to make this a lot smaller and
>> simpler, so I think so.  But if you disagree strongly, let's discuss
>> further.
>
> Not strongly. It means that Aggref will need another field to store
> the transtype and/or the serialtype (for the follow-on patches in
> Combining Aggregates thread)
> The only other issue which I don't think I've addressed yet is target
> list width estimates. Probably that can just pay attention to the
> aggpartial flag too, and if I fix that then likely the Aggref needs to
> carry around a aggtransspace field too, which we really only need to
> populate when the Aggref is in partial mode... it would be wasteful to
> bother looking that up from the catalogs if we're never going to use
> the Aggref in partial mode, and it seems weird to leave it as zero and
> only populate it when we need it. So... if I put on my object oriented
> hat and think about this My brain says that Aggref should inherit
> from PartialAggref and that's what I have now... or at least some
> C variant. So I really do think it's cleaner and easier to understand
> by keeping the ParialAggref.
>
> I see on looking at exprType() that we do have other node types which
> conditionally return a different type, but seeing that does not fill
> me with joy.

I don't think I'd be objecting if you made PartialAggref a real
alternative to Aggref.  But that's not what you've got here.  A
PartialAggref is just a wrapper around an underlying Aggref that
changes the interpretation of it - and I think that's not a good idea.
If you want to have Aggref and PartialAggref as truly parallel node
types, that seems cool, and possibly better than what you've got here
now.  Alternatively, Aggref can do everything.  But I don't think we
should go with this wrapper concept.

-- 
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] WAL logging problem in 9.4.3?

2016-03-15 Thread Kyotaro HORIGUCHI

Thank you for the comment.

I understand that this is not an issue in a hurry so don't bother
to reply.

At Tue, 15 Mar 2016 18:21:34 +0100, Michael Paquier  
wrote in

Re: [HACKERS] pam auth - add rhost item

On 3/10/16 8:11 AM, Grzegorz Sampolski wrote:
> In attchment new patch with updated documentation and with small change
> to coding style as you suggested.

This patch seems fine.  I'm not sure about the name "pamusedns" for the
option, since we use the OS resolver, which might not actually use DNS.
 Maybe something like "pam_use_hostname"?

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

Re: [HACKERS][PROPOSAL] Covering + unique indexes.

2016-03-15 Thread Peter Geoghegan

On Mon, Mar 14, 2016 at 8:43 PM, Peter Geoghegan  wrote:
> Does this work with amcheck? Maybe it works with bt_index_check(), but
> not bt_index_parent_check()? I think that you need to make sure that
> _bt_compare() knows about this, too. That's because it isn't good
> enough to let a truncated internal IndexTuple compare equal to a
> scankey when non-truncated attributes are equal. I think you need to
> have an imaginary "minus infinity" attribute past the first
> non-truncated attribute (i.e. "minus infinity value" for the first
> *truncated* attribute). That way, the downlinks will always be lower
> bounds when the non-"included"/truncated attributes are involved. This
> seems necessary. No?

Actually, I now think I got this slightly wrong.

What's at issue is this (from nbtree README):

"""
Lehman and Yao assume that the key range for a subtree S is described
by Ki < v <= Ki+1 where Ki and Ki+1 are the adjacent keys in the parent
page.  This does not work for nonunique keys (for example, if we have
enough equal keys to spread across several leaf pages, there *must* be
some equal bounding keys in the first level up).  Therefore we assume
Ki <= v <= Ki+1 instead.  A search that finds exact equality to a
bounding key in an upper tree level must descend to the left of that
key to ensure it finds any equal keys in the preceding page.

"""

Today, nbtree needs to check the page to the left of an equal internal
page downlink child anyway. That isn't hard-coded into _bt_compare(),
though. If it was, it would be a "positive infinity" attribute, not
"negative infinity" as I incorrectly said. This is because the equal
IndexTuples might easily not begin exactly at the beginning of the
downlink's child page (often, we should begin in the left page
instead, by following the previous downlink in the parent instead --
just in case).

Any kind of "infinity" attribute probably isn't necessary for your
patch today, since, as referenced in the README extract above, our
slightly non-standard L does this in _bt_binsrch():

 /*
  * At this point we have high == low, but be careful: they could point
  * past the last slot on the page.
  *
  * On a leaf page, we always return the first key >= scan key (resp. >
  * scan key), which could be the last slot + 1.
  */
 if (P_ISLEAF(opaque))
 return low;

However, I think it's still a good idea to have a special integer in
the IndexTuple explicitly indicating the attribute at which the
"suffix" is truncated, even if the "suffix truncation" happens at a
consistent point based on an index in your patch. That will change in
the future, and we should be prepared.

Even though I was partially mistaken, clearly it still wasn't okay to
even try to compare non-existent attributes in internal pages, since
that segfaulted. So a (mostly imaginary) "positive infinity" attribute
can still exist, initially just to make _bt_compare() not crash. This
attribute number (stored in "itup->t_tid.ip_posid") effectively tells
the binary search code to look at the child to the left of the
compared downlink (not the downlink child itself), even though that's
already going to happen per the code above. So, thinking about it once
more (uh, sorry), _bt_compare() has to "indicate equality"/return 0,
*despite* being *logically* a "positive infinity" comparison from a
higher level, in order to let the code above to handle it instead, so
it isn't handled more than once. Also, not sure if less common
"nextkey == true" case needs some further consideration (lets forget
that detail for time being, though). Phew!

So, as I said, _bt_binsrch() and/or _bt_compare() can be fixed to make
sure that the scan arrives on the correct leaf page (the first leaf
page that an matching IndexTuple could be on). What then, though? What
about leaf pages, that *do* have the extra attributes ("INCLUDING"
attributes) represented in their tuples, and *don't* "return
OffsetNumberPrev(low)" at the end of _bt_binsrch() (they do the
P_LEAF() thing quoted above)? Are they safe? Remember:

* For nextkey=false (cmpval=1), the loop invariant is: all slots before
* 'low' are < scan key, all slots at or after 'high' are >= scan key.

I think this means that you need to be very careful about leaf pages, too.

Speculative insertion (used by UPSERT) may not even have the extra
attributes, during the precheck that starts from within
check_exclusion_or_unique_constraint() -- it needs to start from the
very start at the leaf level, without regard for the particular
details of the non-constrained extra columns. I see that you take the
number of attributes a new way, so that ultimately _bt_compare()'s
"keysz" argument only includes "full" attributes for cases like the
UPSERT one. That seems okay. However, what about the case where a
tuple is inserted? We need to do unique enforcement on the one hand,
but need to start at the right place for insertion on the other hand.
Is it okay that

Re: [HACKERS] Parallel Aggregate

On 16 March 2016 at 14:08, Robert Haas  wrote:
> On Tue, Mar 15, 2016 at 8:55 PM, David Rowley
>  wrote:
>> On 16 March 2016 at 13:42, Robert Haas  wrote:
>>> On Tue, Mar 15, 2016 at 8:04 PM, David Rowley
>>>  wrote:
 On 16 March 2016 at 12:58, Robert Haas  wrote:
> ...and why would one be true and the other false?

 One would be the combine aggregate (having aggpartial = false), and
 the one in the subnode would be the partial aggregate (having
 aggpartial = true)
 Notice in create_grouping_paths() I build a partial aggregate version
 of the PathTarget named partial_group_target, this one goes into the
 partial agg node, and Gather node. In this case the aggpartial will be
 set differently for the Aggrefs in each of the two PathTargets, so it
 would not be possible in setrefs.c to find the correct target list
 entry in the subnode by using equal(). It'll just end up triggering
 the elog(ERROR, "Aggref not found in subplan target list"); error.
>>>
>>> OK, I get it now.  I still don't like it very much.  There's no
>>> ironclad requirement that we use equal() here as opposed to some
>>> bespoke comparison function with the exact semantics we need, and ISTM
>>> that getting rid of PartialAggref would shrink this patch down quite a
>>> bit.
>>
>> Well that might work. I'd not thought of doing it that way. The only
>> issue that I can foresee with that is that when new fields are added
>> to Aggref in the future, we might miss updating that custom comparison
>> function to include them.
>
> That's true, but it doesn't seem like that big a deal.  A code comment
> in the Aggref definition seems like sufficient insurance against such
> a mistake.
>
>> Should I update the patch to use the method you describe?
>
> Well, my feeling is that is going to make this a lot smaller and
> simpler, so I think so.  But if you disagree strongly, let's discuss
> further.

Not strongly. It means that Aggref will need another field to store
the transtype and/or the serialtype (for the follow-on patches in
Combining Aggregates thread)
The only other issue which I don't think I've addressed yet is target
list width estimates. Probably that can just pay attention to the
aggpartial flag too, and if I fix that then likely the Aggref needs to
carry around a aggtransspace field too, which we really only need to
populate when the Aggref is in partial mode... it would be wasteful to
bother looking that up from the catalogs if we're never going to use
the Aggref in partial mode, and it seems weird to leave it as zero and
only populate it when we need it. So... if I put on my object oriented
hat and think about this My brain says that Aggref should inherit
from PartialAggref and that's what I have now... or at least some
C variant. So I really do think it's cleaner and easier to understand
by keeping the ParialAggref.

I see on looking at exprType() that we do have other node types which
conditionally return a different type, but seeing that does not fill
me with joy.

-- 
 David Rowley   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] Support for N synchronous standby servers - take 2

2016-03-15 Thread Thomas Munro


 Synchronous replication offers the ability to confirm that all changes
-made by a transaction have been transferred to one synchronous standby
-server. This extends the standard level of durability
+made by a transaction have been transferred to one or more
synchronous standby
+server. This extends that standard level of durability
 offered by a transaction commit. This level of protection is referred
-to as 2-safe replication in computer science theory.
+to as group-safe replication in computer science theory.


A message on the -general list today pointed me to some earlier
discussion[1] which quoted and referenced definitions of these
academic terms[2].  I think the above documentation should say:

"This level of protection is referred to as 2-safe replication in
computer science literature when synchronous_commit is
set to on, and group-1-safe (group-safe and 1-safe) when
synchronous_commit is set to remote_write."

By my reading, the situation doesn't actually change with this patch.
It doesn't matter whether you need 1 or 42 synchronous standbys to
make a quorum: 2-safe means durable (fsync) on all of them,
group-1-safe means durable on one server and received (implied by
remote_write) by all of them.

I think we should be using those definitions because Gray's earlier
definition of 2-safe from Transaction Processing 12.6.3 doesn't really
fit:  It can optionally mean remote receipt or remote durable storage,
but it doesn't wait if the 'backup' is down, so it's not the same type
of guarantee.  (He also has 'very safe' which might describe our
syncrep, I'm not sure.)

[1] 
http://www.postgresql.org/message-id/603c8f070812132142n5408e7ddk899e83cddd4cb...@mail.gmail.com
[2] http://infoscience.epfl.ch/record/33053/files/EPFL_TH2577.pdf page 76

On Thu, Mar 10, 2016 at 11:21 PM, Fujii Masao  wrote:
> On Fri, Mar 4, 2016 at 3:40 AM, Masahiko Sawada  wrote:
>> On Thu, Mar 3, 2016 at 11:30 PM, Masahiko Sawada  
>> wrote:
>>> Hi,
>>>
>>> Thank you so much for reviewing this patch!
>>>
>>> All review comments regarding document and comment are fixed.
>>> Attached latest v14 patch.
>>>
 This accepts 'abc^Id' as a name, which is wrong behavior (but
 such appliction names are not allowed anyway. If you assume so,
 I'd like to see a comment for that.).
>>>
>>> 'abc^Id' is accepted as application_name, no?
>>> postgres(1)=# set application_name to 'abc^Id';
>>> SET
>>> postgres(1)=# show application_name ;
>>>  application_name
>>> --
>>>  abc^Id
>>> (1 row)
>>>
 addlit_xd_string(char *ytext) and addlitchar_xd_string(unsigned
 char ychar) requires differnt character types. Is there any reason
 for that?
>>>
>>> Because addlit_xd_string() is for adding string(char *) to xd_string,
>>> OTOH addlit_xd_char() is for adding just one character to xd_string.
>>>
 I personally don't like addlit*string() things for such simple
 syntax but itself is acceptble enough for me. However it uses
 StringInfo to hold double-quoted names, which pallocs 1024 bytes
 of memory chunk for every double-quoted name. The chunks are
 finally stacked up left uncollected until the current
 memorycontext is deleted or reset (It is deleted just after
 finishing config file processing). Addition to that, setting
 s_s_names runs the parser twice. It seems to me too greedy and
 seems that static char [NAMEDATALEN] is enough using the v12 way
 without palloc/repalloc.
>>>
>>> I though that length of group name could be more than NAMEDATALEN, so
>>> I use StringInfo.
>>> Is it not necessary?
>>>
 I found that the name SyncGroupName.wait_num is not
 instinctive. How about sync_num, sync_member_num or
 sync_standby_num? If the last is preferable, .members also should
 be .standbys .
>>>
>>> Thanks, sync_num is preferable to me.
>>>
>>> ===
 I am quite uncomfortable with the existence of
 WanSnd.sync_standby_priority. It represented the pirority in the
 old linear s_s_names format but nested groups or even
 single-level quarum list obviously doesn't fit it. Can we get rid
 of sync_standby_priority, even though we realize atmost
 n-priority for now?
>>>
>>> We could get rid of sync_standby_priority.
>>> But if so, we will not be able to see the next sync standby in
>>> pg_stat_replication system view.
>>> Regarding each node priority, I was thinking that standbys in quorum
>>> list have same priority, and in nested group each standbys are given
>>> the priority starting from 1.
>>>
>>> ===
 The function SyncRepGetSyncedLsnsUsingPriority doesn't seem to
 have specific code for every prioritizing method (which are
 priority, quorum, nested and so). Is there any reson to use it as
 a callback of SyncGroupNode?
>>>
>>> The reason why the current code is so is that current code is for only
>>> priority method

Re: [HACKERS] Parallel Aggregate

On Tue, Mar 15, 2016 at 8:55 PM, David Rowley
 wrote:
> On 16 March 2016 at 13:42, Robert Haas  wrote:
>> On Tue, Mar 15, 2016 at 8:04 PM, David Rowley
>>  wrote:
>>> On 16 March 2016 at 12:58, Robert Haas  wrote:
 ...and why would one be true and the other false?
>>>
>>> One would be the combine aggregate (having aggpartial = false), and
>>> the one in the subnode would be the partial aggregate (having
>>> aggpartial = true)
>>> Notice in create_grouping_paths() I build a partial aggregate version
>>> of the PathTarget named partial_group_target, this one goes into the
>>> partial agg node, and Gather node. In this case the aggpartial will be
>>> set differently for the Aggrefs in each of the two PathTargets, so it
>>> would not be possible in setrefs.c to find the correct target list
>>> entry in the subnode by using equal(). It'll just end up triggering
>>> the elog(ERROR, "Aggref not found in subplan target list"); error.
>>
>> OK, I get it now.  I still don't like it very much.  There's no
>> ironclad requirement that we use equal() here as opposed to some
>> bespoke comparison function with the exact semantics we need, and ISTM
>> that getting rid of PartialAggref would shrink this patch down quite a
>> bit.
>
> Well that might work. I'd not thought of doing it that way. The only
> issue that I can foresee with that is that when new fields are added
> to Aggref in the future, we might miss updating that custom comparison
> function to include them.

That's true, but it doesn't seem like that big a deal.  A code comment
in the Aggref definition seems like sufficient insurance against such
a mistake.

> Should I update the patch to use the method you describe?

Well, my feeling is that is going to make this a lot smaller and
simpler, so I think so.  But if you disagree strongly, let's discuss
further.

-- 
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] [PROPOSAL] VACUUM Progress Checker.

2016-03-15 Thread Amit Langote

On 2016/03/16 2:33, Robert Haas wrote:
> On Tue, Mar 15, 2016 at 1:16 AM, Amit Langote
>  wrote:
>> On 2016/03/15 3:41, Robert Haas wrote:
>>> Well, I think you need to study the index AMs and figure this out.
>>
>> OK.  I tried to put calls to the callback in appropriate places, but
>> couldn't get the resulting progress numbers to look sane.  So I ended up
>> concluding that any attempt to do so is futile unless I analyze each AM's
>> vacuum code carefully to be able to determine in advance the max bound on
>> the count of blocks that the callback will report.  Anyway, as you
>> suggest, we can improve it later.
> 
> I don't think there is any way to bound that, because new blocks can
> get added to the index concurrently, and we might end up needing to
> scan them.  Reporting the number of blocks scanned can still be
> useful, though - any chance you can just implement that part of it?

Do you mean the changes I made to index bulk delete API for this purpose
in last few versions of this patch?  With it, I have observed that
reported scanned blocks count (that is incremented for every
ReadBufferExtended() call across a index vacuum cycle using the new
callback) can be non-deterministic.  Would there be an accompanying
index_blocks_total (pg_indexes_size()-based) in the view, as well?

>>> But I think for starters you should write a patch that reports the 
>>> following:
>>>
>>> 1. phase
>>> 2. number of heap blocks scanned
>>> 3. number of heap blocks vacuumed
>>> 4. number of completed index vac cycles
>>> 5. number of dead tuples collected since the last index vac cycle
>>> 6. number of dead tuples that we can store before needing to perform
>>> an index vac cycle
>>>
>>> All of that should be pretty straightforward, and then we'd have
>>> something we can ship.  We can add the detailed index reporting later,
>>> when we get to it, perhaps for 9.7.
>>
>> OK, I agree with this plan.  Attached updated patch implements this.
> 
> Sorta.  Committed after renaming what you called heap blocks vacuumed
> back to heap blocks scanned, adding heap blocks vacuumed, removing the
> overall progress meter which I don't believe will be anything close to
> accurate, fixing some stylistic stuff, arranging to update multiple
> counters automatically where it could otherwise produce confusion,
> moving the new view near similar ones in the file, reformatting it to
> follow the style of the rest of the file, exposing the counter
> #defines via a header file in case extensions want to use them, and
> overhauling and substantially expanding the documentation.

Thanks a lot for committing this.  The committed version is much better.
Some of the things therein should really have been part of the final
*submitted* patch; I will try to improve in that area in my submissions
henceforth.

Thanks,
Amit




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

Re: [HACKERS] Parallel Aggregate

On 16 March 2016 at 13:42, Robert Haas  wrote:
> On Tue, Mar 15, 2016 at 8:04 PM, David Rowley
>  wrote:
>> On 16 March 2016 at 12:58, Robert Haas  wrote:
>>> ...and why would one be true and the other false?
>>
>> One would be the combine aggregate (having aggpartial = false), and
>> the one in the subnode would be the partial aggregate (having
>> aggpartial = true)
>> Notice in create_grouping_paths() I build a partial aggregate version
>> of the PathTarget named partial_group_target, this one goes into the
>> partial agg node, and Gather node. In this case the aggpartial will be
>> set differently for the Aggrefs in each of the two PathTargets, so it
>> would not be possible in setrefs.c to find the correct target list
>> entry in the subnode by using equal(). It'll just end up triggering
>> the elog(ERROR, "Aggref not found in subplan target list"); error.
>
> OK, I get it now.  I still don't like it very much.  There's no
> ironclad requirement that we use equal() here as opposed to some
> bespoke comparison function with the exact semantics we need, and ISTM
> that getting rid of PartialAggref would shrink this patch down quite a
> bit.

Well that might work. I'd not thought of doing it that way. The only
issue that I can foresee with that is that when new fields are added
to Aggref in the future, we might miss updating that custom comparison
function to include them.

Should I update the patch to use the method you describe?

-- 
 David Rowley   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] Parallel Aggregate

On Tue, Mar 15, 2016 at 8:04 PM, David Rowley
 wrote:
> On 16 March 2016 at 12:58, Robert Haas  wrote:
>> On Tue, Mar 15, 2016 at 6:55 PM, David Rowley
>>  wrote:
>>> On 16 March 2016 at 11:00, Robert Haas  wrote:
 I don't see why we would need to leave aggpartial out of the equals()
 check.  I must be missing something.
>>>
>>> See fix_combine_agg_expr_mutator()
>>>
>>> This piece of code:
>>>
>>> /*
>>> * Aggrefs for partial aggregates are wrapped up in a PartialAggref,
>>> * we need to look into the PartialAggref to find the Aggref within.
>>> */
>>> foreach(lc, context->subplan_itlist->tlist)
>>> {
>>> PartialAggref *paggref;
>>> tle = (TargetEntry *) lfirst(lc);
>>> paggref = (PartialAggref *) tle->expr;
>>>
>>> if (IsA(paggref, PartialAggref) &&
>>> equal(paggref->aggref, aggref))
>>> break;
>>> }
>>>
>>> if equals() compared the aggpartial then this code would fail to find
>>> the Aggref in the subnode due to the aggpartial field being true on
>>> one and false on the other Aggref.
>>
>> ...and why would one be true and the other false?
>
> One would be the combine aggregate (having aggpartial = false), and
> the one in the subnode would be the partial aggregate (having
> aggpartial = true)
> Notice in create_grouping_paths() I build a partial aggregate version
> of the PathTarget named partial_group_target, this one goes into the
> partial agg node, and Gather node. In this case the aggpartial will be
> set differently for the Aggrefs in each of the two PathTargets, so it
> would not be possible in setrefs.c to find the correct target list
> entry in the subnode by using equal(). It'll just end up triggering
> the elog(ERROR, "Aggref not found in subplan target list"); error.

OK, I get it now.  I still don't like it very much.  There's no
ironclad requirement that we use equal() here as opposed to some
bespoke comparison function with the exact semantics we need, and ISTM
that getting rid of PartialAggref would shrink this patch down quite a
bit.

-- 
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] Choosing parallel_degree

On 16 March 2016 at 13:26, Julien Rouhaud  wrote:
> On 15/03/2016 21:12, Robert Haas wrote:
>> I thought about this a bit more.  There are a couple of easy things we
>> could do here.
>>
>> The 1000-page threshold could be made into a GUC.
>>
>> We could add a per-table reloption for parallel-degree that would
>> override the calculation.
>>
>> Neither of those things is very smart, but they'd probably both help
>> some people.  If someone is able to produce a patch for either or both
>> of these things *quickly*, we could possibly try to squeeze it into
>> 9.6 as a cleanup of work already done.
>>
>
> I'm not too familiar with parallel planning, but I tried to implement
> both in attached patch. I didn't put much effort into the
> parallel_threshold GUC documentation, because I didn't really see a good
> way to explain it. I'd e happy to improve it if needed. Also, to make
> this parameter easier to tune for users, perhaps we could divide the
> default value by 3 and use it as is in the first iteration in
> create_parallel_path() ?
>
> Also, global max_parallel_degree still needs to be at least 1 for the
> per table value to be considered.

Thanks for working on this. I've only skimmed the patch so far, but
will try to look more closely later.

This did get me wondering why we have the parallel_threshold at all,
and not just allow the parallel_setup_cost to make parallel plans look
less favourable for smaller relations. I assume that this is so that
we don't burden the planner with the overhead of generating parallel
paths for smaller relations?

-- 
 David Rowley   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] Choosing parallel_degree

2016-03-15 Thread Julien Rouhaud

On 15/03/2016 21:12, Robert Haas wrote:
> On Mon, Mar 14, 2016 at 9:25 PM, David Rowley
>  wrote:
>> Over in [1] James mentioned about wanting more to be able to have more
>> influence over the partial path's parallel_degree decision.  At risk
>> of a discussion on that hijacking the parallel aggregate thread, I
>> thought I'd start this for anyone who would want to discuss making
>> changes to that.
>>
>> I've attached a simple C program which shows the parallel_degree which
>> will be chosen at the moment. For now it's based on the size of the
>> base relation. Perhaps that will need to be rethought later, perhaps
>> based on costs. But I just don't think it's something for 9.6.
> 
> I thought about this a bit more.  There are a couple of easy things we
> could do here.
> 
> The 1000-page threshold could be made into a GUC.
> 
> We could add a per-table reloption for parallel-degree that would
> override the calculation.
> 
> Neither of those things is very smart, but they'd probably both help
> some people.  If someone is able to produce a patch for either or both
> of these things *quickly*, we could possibly try to squeeze it into
> 9.6 as a cleanup of work already done.
> 

I'm not too familiar with parallel planning, but I tried to implement
both in attached patch. I didn't put much effort into the
parallel_threshold GUC documentation, because I didn't really see a good
way to explain it. I'd e happy to improve it if needed. Also, to make
this parameter easier to tune for users, perhaps we could divide the
default value by 3 and use it as is in the first iteration in
create_parallel_path() ?

Also, global max_parallel_degree still needs to be at least 1 for the
per table value to be considered.

-- 
Julien Rouhaud
http://dalibo.com - http://dalibo.org
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6c73fb4..472f3d5 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3912,6 +3912,22 @@ SELECT * FROM parent WHERE key = 2400;
   
  
 
+ 
+  parallel_threshold (integer)
+  
+   parallel_threshold configuration 
parameter
+  
+  
+  
+   
+The planner will decide to use parallel plans for relation that are at
+least larger than this parameter. If you lower this setting, the 
planner
+will use more parallel workers.
+   
+
+  
+ 
+
  
   force_parallel_mode (enum)
   
diff --git a/doc/src/sgml/ref/create_table.sgml 
b/doc/src/sgml/ref/create_table.sgml
index cd234db..be9db15 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -909,6 +909,15 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | 
UNLOGGED ] TABLE [ IF NOT EXI

 

+max_parallel_degree (integer)
+
+ 
+  Per-table value for  parameter.
+ 
+
+   
+
+   
 autovacuum_enabled, 
toast.autovacuum_enabled (boolean)
 
  
diff --git a/src/backend/access/common/reloptions.c 
b/src/backend/access/common/reloptions.c
index ea0755a..0b06650 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -267,6 +267,15 @@ static relopt_int intRelOpts[] =
0, 0, 0
 #endif
},
+   {
+   {
+   "max_parallel_degree",
+   "Maximum number of parallel processes per executor node 
for this relation.",
+   RELOPT_KIND_HEAP,
+   AccessExclusiveLock
+   },
+   -1, 1, INT_MAX
+   },
 
/* list terminator */
{{NULL}}
@@ -1291,7 +1300,9 @@ default_reloptions(Datum reloptions, bool validate, 
relopt_kind kind)
{"autovacuum_analyze_scale_factor", RELOPT_TYPE_REAL,
offsetof(StdRdOptions, autovacuum) +offsetof(AutoVacOpts, 
analyze_scale_factor)},
{"user_catalog_table", RELOPT_TYPE_BOOL,
-   offsetof(StdRdOptions, user_catalog_table)}
+   offsetof(StdRdOptions, user_catalog_table)},
+   {"max_parallel_degree", RELOPT_TYPE_INT,
+   offsetof(StdRdOptions, max_parallel_degree)}
};
 
options = parseRelOptions(reloptions, validate, kind, );
diff --git a/src/backend/optimizer/path/allpaths.c 
b/src/backend/optimizer/path/allpaths.c
index 4f60b85..2570619 100644
--- a/src/backend/optimizer/path/allpaths.c
+++ b/src/backend/optimizer/path/allpaths.c
@@ -659,8 +659,12 @@ set_plain_rel_pathlist(PlannerInfo *root, RelOptInfo *rel, 
RangeTblEntry *rte)
 static void
 create_parallel_paths(PlannerInfo *root, RelOptInfo *rel)
 {
-   int parallel_threshold = 1000;
+   int p_threshold = parallel_threshold;
int parallel_degree = 1;
+   int p_max_degree = max_parallel_degree;
+
+   if (rel->max_parallel_degree != -1)
+   p_max_degree = rel->max_parallel_degree;

Re: [HACKERS] plpgsql - DECLARE - cannot to use %TYPE or %ROWTYPE for composite types

2016-03-15 Thread Tom Lane

Pavel Stehule  writes:
>> Robert Haas  writes:
>>> That's not a dumb idea.  I think %TYPE is an Oracle-ism, and it
>>> doesn't seem to have been their best-ever design decision.

> Using %TYPE has sense in PostgreSQL too.

It's certainly useful functionality; the question is whether this
particular syntax is an appropriate base for extended features.

As I see it, what we're talking about here could be called type operators:
given a type name or some other kind of SQL expression, produce the name
of a related type.  The existing things of that sort are %TYPE and []
(we don't really implement [] as a type operator, but a user could
reasonably think of it as one).  This patch proposes to make %TYPE and []
composable into a single operator, and then it proposes to add ELEMENT OF
as a different operator; and these things are only implemented in plpgsql.

My concern is basically that I don't want to stop there.  I think we want
more type operators in future, such as the rowtype-related operators
I sketched upthread; and I think we will want these operators anywhere
that you can write a type name.

Now, in the core grammar we have [] which can be attached to any type
name, and we have %TYPE but it only works in very limited contexts.
There's a fundamental problem with extending %TYPE to be used anywhere
a type name can: consider

select 'foo'::x%type from t;

It's ambiguous whether this is an invocation of %TYPE syntax or whether %
is meant to be a regular operator and TYPE the name of a variable.  Now,
we could remove that ambiguity by promoting TYPE to be a fully reserved
word (it is unreserved today).  But that's not very palatable, and even
if we did reserve TYPE, I think we'd still need a lexer kluge to convert
%TYPE into a single token, else bison will have lookahead problems.
That sort of kluge is ugly, costs performance, and tends to have
unforeseen side-effects.

So my opinion is that rather than extending %TYPE, we need a new syntax
that is capable of being used in more general contexts.

There's another problem with the proposal as given: it adds a prefix
type operator (ELEMENT OF) where before we only had postfix ones.
That means there's an ambiguity about which one binds tighter.  This is
not a big deal right now, since there'd be little point in combining
ELEMENT OF and [] in the same operation, but it's going to create a mess
when we try to add additional type operators.  You're going to need to
allow parentheses to control binding order.  I also find it unsightly
that the prefix operator looks so little like the postfix operators
syntactically, even though they do very similar sorts of things.

In short there basically isn't much to like about these syntax details.

I also do not like adding the feature to plpgsql first.  At best, that's
going to be code we throw away when we implement the same functionality
in the core's typename parser.  At worst, we'll have a permanent
incompatibility because we find we can't make the core parser use exactly
the same syntax.  (For example, it's possible we'd find out we have to
make ELEMENT a fully-reserved word in order to use this ELEMENT OF syntax.
Or maybe it's fine; but until we've tried to cram it into the Typename
production, we won't know.  I'm a bit suspicious of expecting it to be
fine, though, since AFAICS this patch breaks the ability to use "element"
as a plain type name in a plpgsql variable declaration.  Handwritten
parsing code like this tends to be full of such gotchas.)

In short, I think we should reject this implementation and instead try
to implement the type operators we want in the core grammar's Typename
production, from which plpgsql will pick it up automatically.  That is
going to require some other syntax than this.  As I said, I'm not
particularly pushing the function-like syntax I wrote upthread; but
I want to see something that is capable of supporting all those features
and can be extended later if we think of other type operators we want.

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] [WIP] speeding up GIN build with parallel workers

2016-03-15 Thread Constantin S. Pan

On Mon, 14 Mar 2016 08:42:26 -0400
David Steele  wrote:

> On 2/18/16 10:10 AM, Constantin S. Pan wrote:
> > On Wed, 17 Feb 2016 23:01:47 +0300
> > Oleg Bartunov  wrote:
> >
> >> My feedback is (Mac OS X 10.11.3)
> >>
> >> set gin_parallel_workers=2;
> >> create index message_body_idx on messages using gin(body_tsvector);
> >> LOG:  worker process: parallel worker for PID 5689 (PID 6906) was
> >> terminated by signal 11: Segmentation fault
> >
> > Fixed this, try the new patch. The bug was in incorrect handling
> > of some GIN categories.
> 
> Oleg, it looks like Constantin has updated to patch to address the
> issue you were seeing.  Do you have time to retest and review?
> 
> Thanks,

Actually, there was some progress since. The patch is
attached.

1. Added another GUC parameter for changing the amount of
shared memory for parallel GIN workers.

2. Changed the way results are merged. It uses shared memory
message queue now.

3. Tested on some real data (GIN index on email message body
tsvectors). Here are the timings for different values of
'gin_shared_mem' and 'gin_parallel_workers' on a 4-CPU
machine. Seems 'gin_shared_mem' has no visible effect.

wnum mem(MB) time(s)
   0  16 247
   1  16 256
   2  16 126
   4  16  89
   0  32 247
   1  32 270
   2  32 123
   4  32  92
   0  64 254
   1  64 272
   2  64 123
   4  64  88
   0 128 250
   1 128 263
   2 128 126
   4 128  85
   0 256 247
   1 256 269
   2 256 130
   4 256  88
   0 512 257
   1 512 275
   2 512 129
   4 512  92
   01024 255
   11024 273
   21024 130
   41024  90

On Wed, 17 Feb 2016 12:26:05 -0800
Peter Geoghegan  wrote:

> On Wed, Feb 17, 2016 at 7:55 AM, Constantin S. Pan 
> wrote:
> > 4. Hit the 8x speedup limit. Made some analysis of the reasons (see
> > the attached plot or the data file).  
> 
> Did you actually compare this to the master branch? I wouldn't like to
> assume that the one worker case was equivalent. Obviously that's the
> really interesting baseline.

Compared with the master branch. The case of 0 workers is
indeed equivalent to the master branch.

Regards,
Constantindiff --git a/src/backend/access/gin/gininsert.c b/src/backend/access/gin/gininsert.c
index cd21e0e..ff267b3 100644
--- a/src/backend/access/gin/gininsert.c
+++ b/src/backend/access/gin/gininsert.c
@@ -16,14 +16,21 @@
 
 #include "access/gin_private.h"
 #include "access/xloginsert.h"
+#include "access/parallel.h"
+#include "access/xact.h"
 #include "catalog/index.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
 #include "storage/smgr.h"
 #include "storage/indexfsm.h"
+#include "storage/spin.h"
+#include "utils/datum.h"
 #include "utils/memutils.h"
 #include "utils/rel.h"
 
+/* GUC parameter */
+int gin_parallel_workers = 0;
+int gin_shared_mem = 0;
 
 typedef struct
 {
@@ -35,7 +42,6 @@ typedef struct
 	BuildAccumulator accum;
 } GinBuildState;
 
-
 /*
  * Adds array of item pointers to tuple's posting list, or
  * creates posting tree and tuple pointing to tree in case
@@ -265,6 +271,169 @@ ginHeapTupleBulkInsert(GinBuildState *buildstate, OffsetNumber attnum,
 	MemoryContextReset(buildstate->funcCtx);
 }
 
+#define KEY_TASK 42
+#define KEY_SHM_ORIGIN 43
+#define KEY_SHM_PER_WORKER 44
+#define GIN_MAX_WORKERS 24
+#define GIN_BLOCKS_PER_WORKER 4
+
+/*
+ * The shmem message structure:
+ * Entry, Key, List
+ */
+
+typedef struct {
+	GinNullCategory category;
+	OffsetNumber attnum;
+	int nlist;
+} GinShmemEntry;
+
+typedef struct {
+	void *mq;
+	shm_mq_handle *mqh;
+	bool end_of_tree;
+	bool end_of_forest;
+
+	void *msg_body;
+	Size msg_len;
+	Datum key;
+	int skipkey;
+} WorkerResult;
+
+/*
+ * This structure describes the GIN build task for the parallel workers. We use
+ * OIDs here because workers are separate processes and pointers may become
+ * meaningless for them. The "lock" is used to protect the "scanned" and
+ * "reltuples" fields as the workers modify them.
+ */
+typedef struct {
+	int		to_scan;
+	int		scanned;
+	slock_t	lock;
+	Oid		heap_oid;
+	Oid		index_oid;
+	double	reltuples;
+	WorkerResult results[GIN_MAX_WORKERS];
+} PGinBuildTask;
+
+static volatile PGinBuildTask *task;
+
+static shm_mq *mq;
+static shm_mq_handle *mqh;
+
+static void ginDumpEntry(GinState *ginstate,
+		 volatile WorkerResult *r,
+		 OffsetNumber attnum,
+		 Datum key,
+		 GinNullCategory category,
+		 ItemPointerData *list,
+		 int nlist)
+{
+	int keylen, listlen;
+
+	bool isnull;
+	Form_pg_attribute att;
+	GinShmemEntry e;
+
+	// The message consists of 2 or 3 parts. iovec allows us to send them as
+	// one message though the parts are located at unrelated addresses.
+	shm_mq_iovec iov[3];
+	int iovlen = 0;
+
+	char *buf = NULL;
+
+	e.category =

Re: [HACKERS] Idle In Transaction Session Timeout, revived

2016-03-15 Thread Vik Fearing

On 03/08/2016 10:42 PM, Robert Haas wrote:
> On Sun, Jan 31, 2016 at 8:33 AM, Vik Fearing  wrote:
>> Attached is a rebased and revised version of my
>> idle_in_transaction_session_timeout patch from last year.
>>
>> This version does not suffer the problems the old one did where it would
>> jump out of SSL code thanks to Andres' patch in commit
>> 4f85fde8eb860f263384fffdca660e16e77c7f76.
>>
>> The basic idea is if a session remains idle in a transaction for longer
>> than the configured time, that connection will be dropped thus releasing
>> the connection slot and any locks that may have been held by the broken
>> client.
>>
>> Added to the March commitfest.

Attached is version 6 of this patch.

> I see this patch has been marked Ready for Committer despite the lack
> of any really substantive review.  Generally, I think it looks good.
> But I have a couple of questions/comments:
> 
> - I really wonder if the decision to ignore sessions that are idle in
> transaction (aborted) should revisited.  Consider this:
> 
> rhaas=# begin;
> BEGIN
> rhaas=# lock table pg_class;
> LOCK TABLE
> rhaas=# savepoint a;
> SAVEPOINT
> rhaas=# select 1/0;
> ERROR:  division by zero

Revisited.  All idle transactions are now affected, even aborted ones.
I had not thought about subtransactions.

> - I wonder if the documentation should mention potential advantages
> related to holding back xmin.

I guess I kind of punted on this in the new patch.  I briefly mention it
and then link to the routine-vacuuming docs.  I can reword that if
necessary.

> - What's the right order of events in PostgresMain?  Right now the
> patch disables the timeout after checking for interrupts and clearing
> DoingCommandRead, but maybe it would be better to disable the timeout
> first, so that we can't have it happen that start to execute the
> command and then, in medias res, bomb out because of the idle timeout.
> Then again, maybe you had some compelling reason for doing it this
> way, in which case we should document that in the comments.

There is no better reason for putting it there than "it seemed like a
good idea at the time".  I've looked into it a bit more, and I don't see
any danger of having it there, but I can certainly move it if you think
I should.

> - It would be nice if you reviewed someone else's patch in turn.

I have been reviewing other, small patches.  I am signed up for several
in this commitfest that I will hopefully get to shortly, and I have
reviewed others in recent fests where I had no patch of my own.

I may be playing on the penny slots, but I'm still putting my coins in.

> I'm attaching a lightly-edited version of your patch.

I have incorporated your changes.

I also changed the name IdleInTransactionTimeoutSessionPending to the
thinko-free IdleInTransactionSessionTimeoutPending.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 6c73fb4..aaa3a71 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -6063,6 +6063,27 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;
   
  
 
+ 
+  idle_in_transaction_session_timeout (integer)
+  
+   idle_in_transaction_session_timeout configuration parameter
+  
+  
+  
+   
+   Terminate any session with an open transaction that has been idle for
+   longer than the specified duration in milliseconds. This allows any
+   locks to be released and the connection slot to be reused. In particular,
+   it allows tuples visible only to this transaction to be vacuumed.  See
+for more details about this.
+   
+   
+   The default value of 0 means that sessions which are idle in transaction
+   will will not be terminated.
+   
+  
+ 
+
  
   vacuum_freeze_table_age (integer)
   
diff --git a/doc/src/sgml/mvcc.sgml b/doc/src/sgml/mvcc.sgml
index dd3c775..6352e12 100644
--- a/doc/src/sgml/mvcc.sgml
+++ b/doc/src/sgml/mvcc.sgml
@@ -725,7 +725,9 @@ ERROR:  could not serialize access due to read/write dependencies among transact
  
   
Don't leave connections dangling idle in transaction
-   longer than necessary.
+   longer than necessary.  The configuration parameter
+may be used to
+   automatically disconnect lingering sessions.
   
  
  
diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c
index 74ef419..a66e07b 100644
--- a/src/backend/storage/lmgr/proc.c
+++ b/src/backend/storage/lmgr/proc.c
@@ -58,6 +58,7 @@
 int			DeadlockTimeout = 1000;
 int			StatementTimeout = 0;
 int			LockTimeout = 0;
+int			IdleInTransactionSessionTimeout = 0;
 bool		log_lock_waits = false;
 
 /* Pointer to this process's PGPROC and PGXACT structs, if any */
diff --git a/src/backend/tcop/postgres.c

Re: [HACKERS] Parallel Aggregate

On 16 March 2016 at 12:58, Robert Haas  wrote:
> On Tue, Mar 15, 2016 at 6:55 PM, David Rowley
>  wrote:
>> On 16 March 2016 at 11:00, Robert Haas  wrote:
>>> I don't see why we would need to leave aggpartial out of the equals()
>>> check.  I must be missing something.
>>
>> See fix_combine_agg_expr_mutator()
>>
>> This piece of code:
>>
>> /*
>> * Aggrefs for partial aggregates are wrapped up in a PartialAggref,
>> * we need to look into the PartialAggref to find the Aggref within.
>> */
>> foreach(lc, context->subplan_itlist->tlist)
>> {
>> PartialAggref *paggref;
>> tle = (TargetEntry *) lfirst(lc);
>> paggref = (PartialAggref *) tle->expr;
>>
>> if (IsA(paggref, PartialAggref) &&
>> equal(paggref->aggref, aggref))
>> break;
>> }
>>
>> if equals() compared the aggpartial then this code would fail to find
>> the Aggref in the subnode due to the aggpartial field being true on
>> one and false on the other Aggref.
>
> ...and why would one be true and the other false?

One would be the combine aggregate (having aggpartial = false), and
the one in the subnode would be the partial aggregate (having
aggpartial = true)
Notice in create_grouping_paths() I build a partial aggregate version
of the PathTarget named partial_group_target, this one goes into the
partial agg node, and Gather node. In this case the aggpartial will be
set differently for the Aggrefs in each of the two PathTargets, so it
would not be possible in setrefs.c to find the correct target list
entry in the subnode by using equal(). It'll just end up triggering
the elog(ERROR, "Aggref not found in subplan target list"); error.

-- 
 David Rowley   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] Parallel Aggregate

On Tue, Mar 15, 2016 at 6:55 PM, David Rowley
 wrote:
> On 16 March 2016 at 11:00, Robert Haas  wrote:
>> I don't see why we would need to leave aggpartial out of the equals()
>> check.  I must be missing something.
>
> See fix_combine_agg_expr_mutator()
>
> This piece of code:
>
> /*
> * Aggrefs for partial aggregates are wrapped up in a PartialAggref,
> * we need to look into the PartialAggref to find the Aggref within.
> */
> foreach(lc, context->subplan_itlist->tlist)
> {
> PartialAggref *paggref;
> tle = (TargetEntry *) lfirst(lc);
> paggref = (PartialAggref *) tle->expr;
>
> if (IsA(paggref, PartialAggref) &&
> equal(paggref->aggref, aggref))
> break;
> }
>
> if equals() compared the aggpartial then this code would fail to find
> the Aggref in the subnode due to the aggpartial field being true on
> one and false on the other Aggref.

...and why would one be true and the other false?

-- 
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] NOT LIKE index support

2016-03-15 Thread Andreas Karlsson


On 03/15/2016 11:01 PM, Arjen Nienhuis wrote:

I noticed index support for NOT LIKE is missing. Is there a special
reason for that, or would a patch be accepted?

A use case would be:

... WHERE url NOT LIKE 'http%'

Or

... WHERE path NOT LIKE '/%'


My guess is the lack of many compelling use cases for such a feature. 
Indexes are generally only useful for expressions with a high 
selectivity (i.e. where you match a small percentage of the rows in the 
table), and in most cases NOT LIKE would match more than half of the 
rows in the table causing the query planner to prefer doing a sequential 
scan. And your examples above seem like they would match most rows in 
the table, making an index scan rarely worth it.


We do not have support for indexing the <> operator either for btree 
indexes.


Andreas



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

Re: [HACKERS] Parallel Aggregate

On 16 March 2016 at 11:00, Robert Haas  wrote:
> I don't see why we would need to leave aggpartial out of the equals()
> check.  I must be missing something.

See fix_combine_agg_expr_mutator()

This piece of code:

/*
* Aggrefs for partial aggregates are wrapped up in a PartialAggref,
* we need to look into the PartialAggref to find the Aggref within.
*/
foreach(lc, context->subplan_itlist->tlist)
{
PartialAggref *paggref;
tle = (TargetEntry *) lfirst(lc);
paggref = (PartialAggref *) tle->expr;

if (IsA(paggref, PartialAggref) &&
equal(paggref->aggref, aggref))
break;
}

if equals() compared the aggpartial then this code would fail to find
the Aggref in the subnode due to the aggpartial field being true on
one and false on the other Aggref.

-- 
 David Rowley   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] Minor bug affecting ON CONFLICT lock wait log messages

2016-03-15 Thread Peter Geoghegan

On Tue, Mar 15, 2016 at 6:18 AM, Stephen Frost  wrote:
> Agreed.  I'm going to play with it a bit more but barring objections,
> I'll commit and back-patch Peter's patch.

Thanks for taking care of this, Stephen.


-- 
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] Fix typos in comments

On Tue, Mar 15, 2016 at 5:48 PM, Oskari Saarenmaa  wrote:
> Attached a patch to fix a bunch of typos in comments.

Committed.

-- 
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

[HACKERS] NOT LIKE index support

2016-03-15 Thread Arjen Nienhuis

I noticed index support for NOT LIKE is missing. Is there a special reason
for that, or would a patch be accepted?

A use case would be:

... WHERE url NOT LIKE 'http%'

Or

... WHERE path NOT LIKE '/%'

Re: [HACKERS] Parallel Aggregate

On Tue, Mar 15, 2016 at 5:50 PM, David Rowley
 wrote:
>> I still think that's solving the problem the wrong way.  Why can't
>> exprType(), when applied to the Aggref, do something like this?
>>
>> {
>> Aggref *aref = (Aggref *) expr;
>> if (aref->aggpartial)
>> return aref->aggtranstype;
>> else
>> return aref->aggtype;
>> }
>>
>> The obvious answer is "well, because those fields don't exist in
>> Aggref".  But shouldn't they?  From here, it looks like PartialAggref
>> is a cheap hack around not having whacked Aggref around hard for
>> partial aggregation.
>
> We could do it that way if we left the aggpartial field out of the
> equals() check, but I think we go at length to not do that. Just look
> at what's done for all of the location fields. In any case if we did
> that then it might not actually be what we want all of the time...
> Perhaps in some cases we'd want equals() to return false when the
> aggpartial does not match, and in other cases we'd want it to return
> true. There's no way to control that behaviour, so to get around the
> setrefs.c problem I created the wrapper node type, which I happen to
> think is quite clean. Just see Tom's comments about Haribabu's temp
> fix for the problem where he put some hacks into the equals for aggref
> in [1].

I don't see why we would need to leave aggpartial out of the equals()
check.  I must be missing something.

 I don't see where this applies has_parallel_hazard or anything
 comparable to the aggregate functions.  I think it needs to do that.
>>>
>>> Not sure what you mean here.
>>
>> If the aggregate isn't parallel-safe, you can't do this optimization.
>> For example, imagine an aggregate function written in PL/pgsql that
>> for some reason writes data to a side table.  It's
>> has_parallel_hazard's job to check the parallel-safety properties of
>> the functions used in the query.
>
> Sorry, I do know what you mean by that. I might have been wrong to
> assume that the parallelModeOK check did this. I will dig into how
> that is set exactly.

Hmm, sorry, I wasn't very accurate, there.  The parallelModeOK check
will handle indeed the case where there are parallel-unsafe functions,
but it will not handle the case where there are parallel-restricted
functions.  In that latter case, the query can still use parallelism
someplace, but the parallel-restricted functions cannot be executed
beneath the Gather.

-- 
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 stats per script & other stuff

Fabien COELHO wrote:

> >If somebody specifies thousands of -f switches, they will waste a few
> >bytes with each, but I'm hardly concerned about a few dozen kilobytes
> >there ...
> 
> Ok, so you prefer a memory leak. I hate it on principle.

I don't "prefer" memory leaks -- I prefer interfaces that make sense.
Speaking of which, I don't think the arrangement in your patch really
does.  I know I suggested it, but now that I look again, it turns out I
chose badly and you implemented a bad idea, so can we go back and fix
it, please?

What I now think should really happen is that the current sql_scripts
array, currently under an anonymous struct, should be a typedef, say
ParsedScript, and get a new member for the weight; process_file and
process_builtin return a ParsedScript.  The weight and Command ** should
not be part of script_t at all.  In fact, with ParsedScript I don't
think we need to give a name to the anon struct used for builtin
scripts.  Rename the current sql_scripts.name to "desc", to mirror what
is actually put in there from the builtin array struct.  Make addScript
receive a ParsedScript and weight, fill in the weight into the struct,
and put it to the array after sanity-checking.  (I'm OK with keeping
"name" instead of renaming to "desc", if that change becomes too
invasive.)

No need for N_BUILTIN; we can use lengthof(builtin_script) instead.

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

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

Re: [HACKERS] Parallel Aggregate

On 16 March 2016 at 09:23, Robert Haas  wrote:
> On Mon, Mar 14, 2016 at 7:56 PM, David Rowley
>  wrote:
>> A comment does explain this, but perhaps it's not good enough, so I've
>> rewritten it to become.
>>
>> /*
>>  * PartialAggref
>>  *
>>  * When partial aggregation is required in a plan, the nodes from the partial
>>  * aggregate node, up until the finalize aggregate node must pass the 
>> partially
>>  * aggregated states up the plan tree. In regards to target list construction
>>  * in setrefs.c, this requires that exprType() returns the state's type 
>> rather
>>  * than the final aggregate value's type, and since exprType() for Aggref is
>>  * coded to return the aggtype, this is not correct for us. We can't fix this
>>  * by going around modifying the Aggref to change it's return type as 
>> setrefs.c
>>  * requires searching for that Aggref using equals() which compares all 
>> fields
>>  * in Aggref, and changing the aggtype would cause such a comparison to fail.
>>  * To get around this problem we wrap the Aggref up in a PartialAggref, this
>>  * allows exprType() to return the correct type and we can handle a
>>  * PartialAggref in setrefs.c by just peeking inside the PartialAggref to 
>> check
>>  * the underlying Aggref. The PartialAggref lives as long as executor 
>> start-up,
>>  * where it's removed and replaced with it's underlying Aggref.
>>  */
>> typedef struct PartialAggref
>>
>> does that help explain it better?
>
> I still think that's solving the problem the wrong way.  Why can't
> exprType(), when applied to the Aggref, do something like this?
>
> {
> Aggref *aref = (Aggref *) expr;
> if (aref->aggpartial)
> return aref->aggtranstype;
> else
> return aref->aggtype;
> }
>
> The obvious answer is "well, because those fields don't exist in
> Aggref".  But shouldn't they?  From here, it looks like PartialAggref
> is a cheap hack around not having whacked Aggref around hard for
> partial aggregation.

We could do it that way if we left the aggpartial field out of the
equals() check, but I think we go at length to not do that. Just look
at what's done for all of the location fields. In any case if we did
that then it might not actually be what we want all of the time...
Perhaps in some cases we'd want equals() to return false when the
aggpartial does not match, and in other cases we'd want it to return
true. There's no way to control that behaviour, so to get around the
setrefs.c problem I created the wrapper node type, which I happen to
think is quite clean. Just see Tom's comments about Haribabu's temp
fix for the problem where he put some hacks into the equals for aggref
in [1].

>>> I don't see where this applies has_parallel_hazard or anything
>>> comparable to the aggregate functions.  I think it needs to do that.
>>
>> Not sure what you mean here.
>
> If the aggregate isn't parallel-safe, you can't do this optimization.
> For example, imagine an aggregate function written in PL/pgsql that
> for some reason writes data to a side table.  It's
> has_parallel_hazard's job to check the parallel-safety properties of
> the functions used in the query.

Sorry, I do know what you mean by that. I might have been wrong to
assume that the parallelModeOK check did this. I will dig into how
that is set exactly.

>>> +   /* XXX +1 ? do we expect the main process to actually do real work? 
>>> */
>>> +   numPartialGroups = Min(numGroups, subpath->rows) *
>>> +   (subpath->parallel_degree + 
>>> 1);
>>> I'd leave out the + 1, but I don't think it matters much.
>>
>> Actually I meant to ask you about this. I see that subpath->rows is
>> divided by the Path's parallel_degree, but it seems the main process
>> does some work too, so this is why I added + 1, as during my tests
>> using a query which produces 10 groups, and had 4 workers, I noticed
>> that Gather was getting 50 groups back, rather than 40, I assumed this
>> is because the main process is helping too, but from my reading of the
>> parallel query threads I believe this is because the Gather, instead
>> of sitting around idle tries to do a bit of work too, if it appears
>> that nothing else is happening quickly enough. I should probably go
>> read nodeGather.c to learn that though.
>
> Yes, the main process does do some work, but less and less as the
> query gets more complicated.  See comments in cost_seqscan().

Thanks

[1] http://www.postgresql.org/message-id/10158.1457329...@sss.pgh.pa.us

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


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

[HACKERS] Fix typos in comments

2016-03-15 Thread Oskari Saarenmaa


Attached a patch to fix a bunch of typos in comments.

/ Oskari
>From 1effab7d75c3ac08257c637d8662b4c8b3fdc750 Mon Sep 17 00:00:00 2001
From: Oskari Saarenmaa 
Date: Tue, 15 Mar 2016 23:45:26 +0200
Subject: [PATCH] misc typofixes in comments

---
 contrib/pgcrypto/sha1.h| 2 +-
 contrib/sepgsql/label.c| 2 +-
 doc/src/sgml/sources.sgml  | 2 +-
 src/backend/access/transam/twophase.c  | 2 +-
 src/backend/libpq/auth.c   | 4 ++--
 src/backend/optimizer/util/relnode.c   | 2 +-
 src/backend/parser/parse_target.c  | 2 +-
 src/backend/storage/buffer/bufmgr.c| 2 +-
 src/backend/storage/ipc/dsm_impl.c | 2 +-
 src/backend/storage/ipc/procarray.c| 2 +-
 src/backend/storage/ipc/shm_mq.c   | 2 +-
 src/backend/utils/adt/json.c   | 2 +-
 src/backend/utils/adt/windowfuncs.c| 2 +-
 src/backend/utils/misc/guc.c   | 4 ++--
 src/bin/pg_dump/compress_io.c  | 2 +-
 src/bin/pg_dump/parallel.c | 2 +-
 src/bin/pg_upgrade/option.c| 2 +-
 src/bin/pgbench/pgbench.c  | 2 +-
 src/include/storage/shm_toc.h  | 2 +-
 src/interfaces/ecpg/ecpglib/execute.c  | 2 +-
 src/interfaces/ecpg/preproc/parse.pl   | 2 +-
 src/interfaces/ecpg/preproc/type.c | 2 +-
 src/test/regress/expected/inherit.out  | 4 ++--
 src/test/regress/expected/replica_identity.out | 2 +-
 src/test/regress/sql/inherit.sql   | 4 ++--
 src/test/regress/sql/replica_identity.sql  | 2 +-
 26 files changed, 30 insertions(+), 30 deletions(-)

diff --git a/contrib/pgcrypto/sha1.h b/contrib/pgcrypto/sha1.h
index 5532ca1..2f61e45 100644
--- a/contrib/pgcrypto/sha1.h
+++ b/contrib/pgcrypto/sha1.h
@@ -63,7 +63,7 @@ extern void sha1_pad(struct sha1_ctxt *);
 extern void sha1_loop(struct sha1_ctxt *, const uint8 *, size_t);
 extern void sha1_result(struct sha1_ctxt *, uint8 *);
 
-/* compatibilty with other SHA1 source codes */
+/* compatibility with other SHA1 source codes */
 typedef struct sha1_ctxt SHA1_CTX;
 
 #define SHA1Init(x)		sha1_init((x))
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 3e32f1b..e12a0e8 100644
--- a/contrib/sepgsql/label.c
+++ b/contrib/sepgsql/label.c
@@ -161,7 +161,7 @@ sepgsql_set_client_label(const char *new_label)
 /*
  * sepgsql_xact_callback
  *
- * A callback routine of transaction commit/abort/prepare.  Commmit or abort
+ * A callback routine of transaction commit/abort/prepare.  Commit or abort
  * changes in the client_label_pending list.
  */
 static void
diff --git a/doc/src/sgml/sources.sgml b/doc/src/sgml/sources.sgml
index fcb3e40..614defa 100644
--- a/doc/src/sgml/sources.sgml
+++ b/doc/src/sgml/sources.sgml
@@ -860,7 +860,7 @@ BETTER: unrecognized node type: 42
  Code in PostgreSQL should only rely on language
  features available in the C89 standard. That means a conforming
  C89 compiler has to be able to compile postgres, at least aside
- from a few platform dependant pieces. Features from later
+ from a few platform dependent pieces. Features from later
  revision of the C standard or compiler specific features can be
  used, if a fallback is provided.
 
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index c4fd9ef..e7234c8 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1613,7 +1613,7 @@ CheckPointTwoPhase(XLogRecPtr redo_horizon)
 	 * transaction manager isn't active.
 	 *
 	 * It's also possible to move I/O out of the lock, but on
-	 * every error we should check whether somebody commited our
+	 * every error we should check whether somebody committed our
 	 * transaction in different backend. Let's leave this optimisation
 	 * for future, if somebody will spot that this place cause
 	 * bottleneck.
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 57c2f48..7f1ae8c 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -838,7 +838,7 @@ pg_GSS_recvauth(Port *port)
 
 	/*
 	 * Loop through GSSAPI message exchange. This exchange can consist of
-	 * multiple messags sent in both directions. First message is always from
+	 * multiple messages sent in both directions. First message is always from
 	 * the client. All messages from client to server are password packets
 	 * (type 'p').
 	 */
@@ -1078,7 +1078,7 @@ pg_SSPI_recvauth(Port *port)
 
 	/*
 	 * Loop through SSPI message exchange. This exchange can consist of
-	 * multiple messags sent in both directions. First message is always from
+	 * multiple messages sent in both directions. First message is always from
 	 * the client. All messages from client to server are password packets
 	 * (type 'p').
 	 */
diff --git

Re: [HACKERS] Combining Aggregates

On 16 March 2016 at 06:39, Tomas Vondra  wrote:
> After looking at the parallel aggregate patch, I also looked at this one, as
> it's naturally related. Sadly I haven't found any issue that I could nag
> about ;-) The patch seems well baked, as it was in the oven for quite a long
> time already.

Thanks for looking at this.

> The one concern I do have is that it only adds (de)serialize functions for
> SUM(numeric) and AVG(numeric). I think it's reasonable not to include that
> into the patch, but it will look a bit silly if that's all that gets into
> 9.6.

Yes me too, so I spent several hours yesterday writing all of the
combine functions and serialisation/deserialisation that are required
for all of SUM(), AVG() STDDEV*(). I also noticed that I had missed
using some existing functions for bool_and() and bool_or() so I added
those to pg_aggregate.h. I'm just chasing down a crash bug on
HAVE_INT128 enabled builds, so should be posting a patch quite soon. I
didn't touch the FLOAT4 and FLOAT8 aggregates as I believe Haribabu
has a patch for that over on the parallel aggregate thread. I've not
looked at it in detail yet.

If Haribabu's patch does all that's required for the numerical
aggregates for floating point types then the status of covered
aggregates is (in order of pg_aggregate.h):

* AVG() complete coverage
* SUM() complete coverage
* MAX() complete coverage
* MIN() complete coverage
* COUNT() complete coverage
* STDDEV + friends complete coverage
* regr_*,covar_pop,covar_samp,corr not touched these.
* bool*() complete coverage
* bitwise aggs. complete coverage
* Remaining are not touched. I see diminishing returns with making
these parallel for now. I think I might not be worth pushing myself
any harder to make these ones work.

Does what I have done + floating point aggs from Haribabu seem
reasonable for 9.6?

> I think it would be good to actually document which aggregates support
> parallel execution, and which don't. Currently the docs don't mention this
> at all, and it's tricky for regular users to deduce that as it's related to
> the type of the internal state and not to the input types. An example of
> that is the SUM(bigint) example mentioned above.

I agree. I will look for a suitable place.

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

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

[HACKERS] Show dropped users' backends in pg_stat_activity

2016-03-15 Thread Oskari Saarenmaa

I was looking into some issues we recently had when dropping db users 
and was surprised to see that dropped users' sessions and transactions 
continue to work after the role is dropped.


Since dropping a role requires dropping all grants it has (using DROP 
OWNED BY ...) the dropped role can't start new transactions that do a 
whole lot unless there are objects with access granted to PUBLIC, but 
any running transactions remain running and can write to the database. 
They can also hold locks which interfere with other backends without 
showing up in most activity or lock monitoring tools as they won't 
appear in pg_stat_activity.


IMO any open sessions for a dropped user should be automatically 
terminated when the role is dropped, but that would probably be a bigger 
change so attached a proposed patch for using left joins in 
pg_stat_activity and pg_stat_replication to show activity by dropped roles.


/ Oskari
>From 4632aa09fe82d80e378123ca46fdf8aecdda795f Mon Sep 17 00:00:00 2001
From: Oskari Saarenmaa 
Date: Mon, 14 Mar 2016 18:34:24 +0200
Subject: [PATCH] pg_stat_activity: display backends for dropped roles

---
 src/backend/catalog/system_views.sql | 13 ++---
 src/test/regress/expected/rules.out  | 14 ++
 2 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 84aa061..e0b583e 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -642,9 +642,9 @@ CREATE VIEW pg_stat_activity AS
 S.backend_xid,
 s.backend_xmin,
 S.query
-FROM pg_database D, pg_stat_get_activity(NULL) AS S, pg_authid U
-WHERE S.datid = D.oid AND
-S.usesysid = U.oid;
+FROM pg_stat_get_activity(NULL) AS S
+JOIN pg_database AS D ON (S.datid = D.oid)
+LEFT JOIN pg_authid AS U ON (S.usesysid = U.oid);
 
 CREATE VIEW pg_stat_replication AS
 SELECT
@@ -664,10 +664,9 @@ CREATE VIEW pg_stat_replication AS
 W.replay_location,
 W.sync_priority,
 W.sync_state
-FROM pg_stat_get_activity(NULL) AS S, pg_authid U,
-pg_stat_get_wal_senders() AS W
-WHERE S.usesysid = U.oid AND
-S.pid = W.pid;
+FROM pg_stat_get_activity(NULL) AS S
+JOIN pg_stat_get_wal_senders() AS W ON (S.pid = W.pid)
+LEFT JOIN pg_authid AS U ON (S.usesysid = U.oid);
 
 CREATE VIEW pg_stat_wal_receiver AS
 SELECT
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 22ea06c..54d7a7b 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1656,10 +1656,9 @@ pg_stat_activity| SELECT s.datid,
 s.backend_xid,
 s.backend_xmin,
 s.query
-   FROM pg_database d,
-pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, ssl, sslversion, sslcipher, sslbits, sslcompression, sslclientdn),
-pg_authid u
-  WHERE ((s.datid = d.oid) AND (s.usesysid = u.oid));
+   FROM ((pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, ssl, sslversion, sslcipher, sslbits, sslcompression, sslclientdn)
+ JOIN pg_database d ON ((s.datid = d.oid)))
+ LEFT JOIN pg_authid u ON ((s.usesysid = u.oid)));
 pg_stat_all_indexes| SELECT c.oid AS relid,
 i.oid AS indexrelid,
 n.nspname AS schemaname,
@@ -1763,10 +1762,9 @@ pg_stat_replication| SELECT s.pid,
 w.replay_location,
 w.sync_priority,
 w.sync_state
-   FROM pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, ssl, sslversion, sslcipher, sslbits, sslcompression, sslclientdn),
-pg_authid u,
-pg_stat_get_wal_senders() w(pid, state, sent_location, write_location, flush_location, replay_location, sync_priority, sync_state)
-  WHERE ((s.usesysid = u.oid) AND (s.pid = w.pid));
+   FROM ((pg_stat_get_activity(NULL::integer) s(datid, pid, usesysid, application_name, state, query, wait_event_type, wait_event, xact_start, query_start, backend_start, state_change, client_addr, client_hostname, client_port, backend_xid, backend_xmin, ssl, sslversion, sslcipher, sslbits, sslcompression, sslclientdn)
+ JOIN pg_stat_get_wal_senders() w(pid, state, sent_location, write_location, flush_location, replay_location, sync_priority, sync_state) ON ((s.pid = w.pid)))
+ LEFT JOIN pg_authid u ON ((s.usesysid = u.oid)));
 pg_stat_ssl| SELECT s.pid,

Re: [HACKERS] Soliciting Feedback on Improving Server-Side Programming Documentation

On Tue, Mar 15, 2016 at 4:38 PM, Álvaro Hernández Tortosa 
wrote:

>
> I started a similar thread with probably similar concerns:
> http://www.postgresql.org/message-id/56d1a6aa.6080...@8kdata.com
>
> I believe this effort should be done. I added to my TODO list to
> compile a list of used functions in a selection of picked extensions to use
> that as a starting point of an "API".
>
> Regards,
>
> Álvaro
>
>
Clearly we have the same goal in mind. I don't know how I missed seeing
your thread.

Re: [HACKERS] Missing conversion error handling in postgres_fdw

On Tue, Mar 15, 2016 at 4:06 AM, Etsuro Fujita
 wrote:
> I noticed that this in make_tuple_from_result_row does conversion error
> handling only for the ordinary-column case (ie, errpos.cur_attno is set
> for that case, but not for the ctid case).
>
> /* convert value to internal representation */
> if (i > 0)
> {
> /* ordinary column */
> Assert(i <= tupdesc->natts);
> nulls[i - 1] = (valstr == NULL);
> /* Apply the input function even to nulls, to support domains */
> errpos.cur_attno = i;
> values[i - 1] = InputFunctionCall(>attinfuncs[i - 1],
>   valstr,
>   attinmeta->attioparams[i - 1],
>   attinmeta->atttypmods[i - 1]);
> errpos.cur_attno = 0;
> }
> else if (i == SelfItemPointerAttributeNumber)
> {
> /* ctid --- note we ignore any other system column in result */
> if (valstr != NULL)
> {
> Datum   datum;
>
> datum = DirectFunctionCall1(tidin, CStringGetDatum(valstr));
> ctid = (ItemPointer) DatumGetPointer(datum);
> }
> }
>
> I think errpos.cur_attno should be set for the ctid case as well.
> Attached is a patch for that.

Hmm, I'd say you are right. Committed.

-- 
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] Re: [COMMITTERS] pgsql: Only try to push down foreign joins if the user mapping OIDs mat

On Tue, Mar 15, 2016 at 6:44 AM, Ashutosh Bapat
 wrote:
> Here's patch which fixes the issue using Robert's idea.

Please at least check your patches with 'git diff --check' before
submitting them.  And where it's not too much trouble, pgindent them.
Or at least make them look something like what pgindent would have
produced, instead of having the line lengths be all over the place.

 -- change the session user to view_owner and execute the statement. Because of
 -- change in session user, the plan should get invalidated and created again.
--- While creating the plan, it should throw error since there is no
user mapping
--- available for view_owner.
+-- The join will not be pushed down since the joining relations do not have a
+-- valid user mapping.

Now what's going on here?  It seems to me that either postgres_fdw
requires a user mapping (in which case this ought to fail) or it
doesn't (in which case this ought to push the join down).  I don't
understand how working but not pushing the join down can ever be the
right behavior.

-- 
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] RFC: replace pg_stat_activity.waiting with something more descriptive

Robert Haas wrote:
> On Tue, Mar 15, 2016 at 10:41 AM, Thom Brown  wrote:
> >> It turns out that I hate the fact that the Wait Event Name column is
> >> effectively in a random order.  If a user sees a message, and goes to
> >> look up the value in the wait_event description table, they either
> >> have to search with their browser/PDF viewer, or scan down the list
> >> looking for the item they're looking for, not knowing how far down it
> >> will be.  The same goes for wait event type.

> Hmm, I'm not sure this is a good idea.  I don't think it's crazy to
> report the locks in the order they are defined in the source code;
> many people will be familiar with that order, and it might make the
> list easier to maintain.  On the other hand, I'm also not sure this is
> a bad idea.  Alphabetical order is a widely-used standard.  So, I'm
> going to abstain from any strong position here and ask what other
> people think of Thom's proposed change.

I think using implementation order is crazy.  +1 for alphabetical.  If
this really makes devs' lives more difficult (and I disagree that it
does), let's reorder the items in the source code too.

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


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

Re: [HACKERS] Background Processes and reporting

2016-03-15 Thread Vladimir Borodin


> 15 марта 2016 г., в 19:57, Oleg Bartunov  написал(а):
> 
> 
> 
> On Tue, Mar 15, 2016 at 7:43 PM, Alexander Korotkov 
> > wrote:
> On Tue, Mar 15, 2016 at 12:57 AM, Robert Haas  > wrote:
> On Mon, Mar 14, 2016 at 4:42 PM, Andres Freund  > wrote:
> > On 2016-03-14 16:16:43 -0400, Robert Haas wrote:
> >> > I have already shown [0, 1] the overhead of measuring timings in linux on
> >> > representative workload. AFAIK, these tests were the only one that showed
> >> > any numbers. All other statements about terrible performance have been 
> >> > and
> >> > remain unconfirmed.
> >>
> >> Of course, those numbers are substantial regressions which would
> >> likely make it impractical to turn this on on a heavily-loaded
> >> production system.
> >
> > A lot of people operating production systems are fine with trading a <=
> > 10% impact for more insight into the system; especially if that
> > configuration can be changed without a restart.  I know a lot of systems
> > that use pg_stat_statements, track_io_timing = on, etc; just to get
> > that. In fact there's people running perf more or less continuously in
> > production environments; just to get more insight.
> >
> > I think it's important to get as much information out there without
> > performance overhead, so it can be enabled by default. But I don't think
> > it makes sense to not allow features in that cannot be enabled by
> > default, *if* we tried to make them cheap enough beforehand.
> 
> Hmm, OK.  I would have expected you to be on the other side of this
> question, so maybe I'm all wet.  One point I am concerned about is
> that, right now, we have only a handful of types of wait events.  I'm
> very interested in seeing us add more, like I/O and client wait.  So
> any overhead we pay here is likely to eventually be paid in a lot of
> places - thus it had better be extremely small.
> 
> OK. Let's start to produce light, not heat.
> 
> As I get we have two features which we suspect to introduce overhead:
> 1) Recording parameters of wait events which requires some kind of 
> synchronization protocol.
> 2) Recording time of wait events because time measurements might be expensive 
> on some platforms.
> 
> Simultaneously there are machines and workloads where both of these features 
> doesn't produce measurable overhead.  And, we're talking not about toy 
> databases. Vladimir is DBA from Yandex which is in TOP-20 (by traffic) 
> internet companies in the world.  They do run both of this features in 
> production highload database without noticing any overhead of them. 
> 
> It would be great progress, if we decide that we could add both of these 
> features controlled by GUC (off by default).
> 
> enable_waits_statistics ?
>  
> 
> If we decide so, then let's start working on this. At first, we should 
> construct list of machines and workloads for testing. Each list of machines 
> and workloads would be not comprehensive. But let's find something that would 
> be enough for testing of GUC controlled, off by default features.  Then we 
> can turn our conversation from theoretical thoughts to particular benchmarks 
> which would be objective and convincing to everybody. 
> 
> Vladimir, could you provide a test suite, so other people could measure 
> overhead on their machines ?

I have somehow described it here [0]. Since the majority of concerns were 
around LWLocks, the plan was to reconstruct a workload under heavy LWLocks 
pressure. This can easily be done even with pgbench in two following scenarios:
1. Put all the data in shared buffers and on tmpfs and run read/write 
test. Contention would be around ProcArrayLock.
2. Put all the data in RAM but not all in shared buffers and run 
read-only test. Contention would be around buffer manager.

IMHO, these two tests are good to be representative and not depend much on 
hardware.

[0] 
http://www.postgresql.org/message-id/eee78e40-0e48-411a-9f90-cf9339da9...@simply.name

> 
> 
>  
> 
> Otherwise, let's just add these features to the list of unwanted 
> functionality and close this question.
> 
> --
> Alexander Korotkov
> Postgres Professional: http://www.postgrespro.com 
> 
> The Russian Postgres Company 


--
May the force be with you…
https://simply.name

Re: [HACKERS] Soliciting Feedback on Improving Server-Side Programming Documentation

2016-03-15 Thread Álvaro Hernández Tortosa



I started a similar thread with probably similar concerns: 
http://www.postgresql.org/message-id/56d1a6aa.6080...@8kdata.com


I believe this effort should be done. I added to my TODO list to 
compile a list of used functions in a selection of picked extensions to 
use that as a starting point of an "API".


Regards,

Álvaro


--
Álvaro Hernández Tortosa


---
8Kdata




On 15/03/16 13:02, Corey Huinker wrote:
Over the past few months, I've been familiarizing myself with postgres 
server side programming in C.


My attempts to educate myself were slow and halting. The existing 
server side programming documentation has some examples, but those 
examples didn't show me how do what I wanted to do, and my 
research-via-google was highly circular, almost always pointing back 
to the documentation I had already found lacking, or a copy of it.


Most of what I have learned I have culled from asking people on IRC, 
or bugging people I've met through user groups and PgConf. In all 
cases, people have been extremely helpful. However, this method is 
inefficient, because we're using two people's time, one of whom has to 
tolerate my incessant questions and slow learning pace.


Furthermore, the helpful suggestions I received boiled down to:
1. The function/macro/var you're looking for is PG_FOO, git grep PG_FOO
2. Look in blah.c which does something like what you're trying to do
3. The comments in blah.h do a good job of listing and explaining this 
macro or that


#1 git grep is a helpful reflex for discovering examples on my own, 
but it requires that I have a term to search on in the first place, 
and too often I don't know what I don't know.


#2 is the gold standard in terms of correctness (the code had to have 
worked at least up to the last checkin date), and in terms of 
discoverability it often gave me names of new macros to search for, 
coding patterns, etc. However, I was always left with the questions: 
How would I have figured this out on my own? How is the next person 
going to figure it out? Why doesn't anybody document this?


#3 Often answers the last question in #2: It *is* documented, but that 
documentation is not easily discoverable by conventional means.


So what I'd like to do is migrate some of the helpful information in 
the header files into pages of web searchable documentation, and also 
to revamp the existing documentation to be more relevant.


Along the way, I collected a list of things I wished I'd had from the 
start:


  * A list of all the GETARG_* macros. It would have been especially
great if this were in table form:   Your Parameter Is A / Use This
Macro / Which Gives This Result Type / Working example.
  * A list/table of the DatumGet* macros. I'm aware that many of them
overlap/duplicate others. That'd be good to know too.
  * The table at
http://www.postgresql.org/docs/9.1/static/errcodes-appendix.html
has the numeric codes and PL/PGSQL constants enumerated. It'd be
nice if it had the C #define as well
  * The SPI documentation mentions most/all of the SPI functions, but
I couldn't find documentation on the SPI variables like
SPI_processed and SPI_tuptable.
  * Examples and explanation of how PG_TRY()/PG_CATCH work. How to add
context callbacks.
  * Direct Function Calls
  * A comparison of the two modes of writing SRF functions
(Materialize vs multi-call)
  * Less explanation of how to do write V0-style functions. That was
called the "old style" back in version 7.1. Why is that
information up front in the documentation when so much else is
sequestered in header files?

Some of these things may seem obvious/trivial to you. I would argue 
that they're only obvious in retrospect, and the more obvious-to-you 
things we robustly document, the quicker we accumulate programmers who 
are capable of agreeing that it's obvious, and that's good for the 
community.


I'm aware that some of these APIs change frequently. In those cases, I 
suggest that we make note of that on the same page.


Because I'm still going through the learning curve, I'm probably the 
least qualified to write the actual documentation. However, I have a 
clear memory of what was hard to learn and I have the motivation to 
make it easier on the next person. That makes me a good focal point 
for gathering, formatting, and submitting the documentation in patch 
form. I'm volunteering to do so. What I need from you is:


  * Citations of existing documentation in header files that
could/should be exposed in our more formal documentation.
  * Explanations of any of the things above, which I can then reformat
into proposed documentation.
  * A willingness to review the proposed new documentation
  * Reasoned explanations for why this is a fool's errand

You supply the expertise, I'll write the patch.

Thanks in advance.

Re: [HACKERS] [PATCH v6] GSSAPI encryption support

2016-03-15 Thread Robbie Harwood

Stephen Frost  writes:

> Robbie,
>
> * Robbie Harwood (rharw...@redhat.com) wrote:
>> Michael Paquier  writes:
>> > -   maj_stat = gss_accept_sec_context(
>> > - _stat,
>> > +   maj_stat = gss_accept_sec_context(_stat,
>> >
>> > This is just noise.
>> 
>> You're not wrong, though I do think it makes the code more readable by
>> enforcing style and this is a "cleanup" commit.  I'll take it out if it
>> bothers you.
>
> First, thanks much for working on this, I've been following along the
> discussions and hope to be able to help move it to commit, once it's
> ready.
>
> Secondly, generally, speaking, we prefer that 'cleanup' type changes
> (particularly whitespace-only ones) are in independent commits which are
> marked as just whitespace/indentation changes.  We have a number of
> organizations which follow our code changes and it makes it more
> difficult on them to include whitespace/indentation changes with code
> changes.

Thanks for the clarification.  I'll be sure to take it out!


signature.asc
Description: PGP signature

Re: FW: [HACKERS] [NOVICE] WHERE clause not used when index is used

On 3/15/16 2:28 PM, Jernigan, Kevin wrote:
> I recently joined the product management team for AWS RDS Postgres
> (after years at Oracle in their database team), and we are very
> interested in confirming (or not) that the fix for the problem below
> will be included in 9.5.2, and in the community’s plans (likely date)
> for releasing 9.5.2.

The patch was reverted in the 9.5 branch, so assuming that that is the
end of this investigation (which it appears to be), then it will be part
of the 9.5.2 release.

> Is there an email list other than hackers where we can follow
> discussions on release plans for 9.5.2 (and future releases)?

This is a good list to follow to know about release schedules.



-- 
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] RFC: replace pg_stat_activity.waiting with something more descriptive

On Tue, Mar 15, 2016 at 10:41 AM, Thom Brown  wrote:
>> It turns out that I hate the fact that the Wait Event Name column is
>> effectively in a random order.  If a user sees a message, and goes to
>> look up the value in the wait_event description table, they either
>> have to search with their browser/PDF viewer, or scan down the list
>> looking for the item they're looking for, not knowing how far down it
>> will be.  The same goes for wait event type.
>>
>> I've attached a patch to sort the list by wait event type and then
>> wait event name.  It also corrects minor SGML indenting issues.
>
> Let's try that again, this time without duplicating a row, and omitting 
> another.

Hmm, I'm not sure this is a good idea.  I don't think it's crazy to
report the locks in the order they are defined in the source code;
many people will be familiar with that order, and it might make the
list easier to maintain.  On the other hand, I'm also not sure this is
a bad idea.  Alphabetical order is a widely-used standard.  So, I'm
going to abstain from any strong position here and ask what other
people think of Thom's proposed change.

-- 
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] RFC: replace pg_stat_activity.waiting with something more descriptive

On Tue, Mar 15, 2016 at 9:17 AM, Thomas Reiss  wrote:
> Here's a small docpatch to fix two typos in the new documentation.

Thanks, committed.

-- 
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] Small patch: fix warnings during compilation on FreeBSD

2016-03-15 Thread Aleksander Alekseev

> Please verify that the committed version solves your problem on
> FreeBSD.

I confirm this patch solves a problem.

> I've checked this on my OS X box, which turns out to have the
> interesting property that xlocale.h declares wcstombs_l(), but only
> if you previously included stdlib.h ... wtf?

Same on FreeBSD.

-- 
Best regards,
Aleksander Alekseev
http://eax.me/


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

Re: [HACKERS] Parallel Aggregate

On Mon, Mar 14, 2016 at 7:56 PM, David Rowley
 wrote:
>> More generally, why are we inventing PartialAggref
>> instead of reusing Aggref?  The code comments seem to contain no
>> indication as to why we shouldn't need all the same details for
>> PartialAggref that we do for Aggref, instead of only a small subset of
>> them.  Hmm... actually, it looks like PartialAggref is intended to
>> wrap Aggref, but that seems like a weird design.  Why not make Aggref
>> itself DTRT?   There's not enough explanation in the patch of what is
>> going on here and why.
>
> A comment does explain this, but perhaps it's not good enough, so I've
> rewritten it to become.
>
> /*
>  * PartialAggref
>  *
>  * When partial aggregation is required in a plan, the nodes from the partial
>  * aggregate node, up until the finalize aggregate node must pass the 
> partially
>  * aggregated states up the plan tree. In regards to target list construction
>  * in setrefs.c, this requires that exprType() returns the state's type rather
>  * than the final aggregate value's type, and since exprType() for Aggref is
>  * coded to return the aggtype, this is not correct for us. We can't fix this
>  * by going around modifying the Aggref to change it's return type as 
> setrefs.c
>  * requires searching for that Aggref using equals() which compares all fields
>  * in Aggref, and changing the aggtype would cause such a comparison to fail.
>  * To get around this problem we wrap the Aggref up in a PartialAggref, this
>  * allows exprType() to return the correct type and we can handle a
>  * PartialAggref in setrefs.c by just peeking inside the PartialAggref to 
> check
>  * the underlying Aggref. The PartialAggref lives as long as executor 
> start-up,
>  * where it's removed and replaced with it's underlying Aggref.
>  */
> typedef struct PartialAggref
>
> does that help explain it better?

I still think that's solving the problem the wrong way.  Why can't
exprType(), when applied to the Aggref, do something like this?

{
Aggref *aref = (Aggref *) expr;
if (aref->aggpartial)
return aref->aggtranstype;
else
return aref->aggtype;
}

The obvious answer is "well, because those fields don't exist in
Aggref".  But shouldn't they?  From here, it looks like PartialAggref
is a cheap hack around not having whacked Aggref around hard for
partial aggregation.

>> I don't see where this applies has_parallel_hazard or anything
>> comparable to the aggregate functions.  I think it needs to do that.
>
> Not sure what you mean here.

If the aggregate isn't parallel-safe, you can't do this optimization.
For example, imagine an aggregate function written in PL/pgsql that
for some reason writes data to a side table.  It's
has_parallel_hazard's job to check the parallel-safety properties of
the functions used in the query.

>> +   /* XXX +1 ? do we expect the main process to actually do real work? 
>> */
>> +   numPartialGroups = Min(numGroups, subpath->rows) *
>> +   (subpath->parallel_degree + 
>> 1);
>> I'd leave out the + 1, but I don't think it matters much.
>
> Actually I meant to ask you about this. I see that subpath->rows is
> divided by the Path's parallel_degree, but it seems the main process
> does some work too, so this is why I added + 1, as during my tests
> using a query which produces 10 groups, and had 4 workers, I noticed
> that Gather was getting 50 groups back, rather than 40, I assumed this
> is because the main process is helping too, but from my reading of the
> parallel query threads I believe this is because the Gather, instead
> of sitting around idle tries to do a bit of work too, if it appears
> that nothing else is happening quickly enough. I should probably go
> read nodeGather.c to learn that though.

Yes, the main process does do some work, but less and less as the
query gets more complicated.  See comments in cost_seqscan().

-- 
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] Choosing parallel_degree

On Mon, Mar 14, 2016 at 9:25 PM, David Rowley
 wrote:
> Over in [1] James mentioned about wanting more to be able to have more
> influence over the partial path's parallel_degree decision.  At risk
> of a discussion on that hijacking the parallel aggregate thread, I
> thought I'd start this for anyone who would want to discuss making
> changes to that.
>
> I've attached a simple C program which shows the parallel_degree which
> will be chosen at the moment. For now it's based on the size of the
> base relation. Perhaps that will need to be rethought later, perhaps
> based on costs. But I just don't think it's something for 9.6.

I thought about this a bit more.  There are a couple of easy things we
could do here.

The 1000-page threshold could be made into a GUC.

We could add a per-table reloption for parallel-degree that would
override the calculation.

Neither of those things is very smart, but they'd probably both help
some people.  If someone is able to produce a patch for either or both
of these things *quickly*, we could possibly try to squeeze it into
9.6 as a cleanup of work already done.

-- 
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 v6] GSSAPI encryption support

2016-03-15 Thread Stephen Frost

Robbie,

* Robbie Harwood (rharw...@redhat.com) wrote:
> Michael Paquier  writes:
> > -   maj_stat = gss_accept_sec_context(
> > - _stat,
> > +   maj_stat = gss_accept_sec_context(_stat,
> >
> > This is just noise.
> 
> You're not wrong, though I do think it makes the code more readable by
> enforcing style and this is a "cleanup" commit.  I'll take it out if it
> bothers you.

First, thanks much for working on this, I've been following along the
discussions and hope to be able to help move it to commit, once it's
ready.

Secondly, generally, speaking, we prefer that 'cleanup' type changes
(particularly whitespace-only ones) are in independent commits which are
marked as just whitespace/indentation changes.  We have a number of
organizations which follow our code changes and it makes it more
difficult on them to include whitespace/indentation changes with code
changes.

Thanks!

Stephen

signature.asc
Description: Digital signature

Re: [HACKERS] [NOVICE] WHERE clause not used when index is used

2016-03-15 Thread Peter Geoghegan

On Thu, Mar 3, 2016 at 2:07 AM, Simon Riggs  wrote:
> Later, I will add the tests we discovered here to index scans, so that
> further optimization work is more easily possible.

Please do.

I would like to start testing the B-Tree code more exhaustively by
adding a test suite to amcheck. This test suite would indirectly test
external sorting, B-Tree page deletion, edge-cases with very large
IndexTuples, etc.

Ideas for good areas of the B-Tree code to add tests for are welcome.

-- 
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] [PATCH v6] GSSAPI encryption support

2016-03-15 Thread Robbie Harwood

Michael Paquier  writes:

> On Tue, Mar 15, 2016 at 3:12 PM, David Steele  wrote:
>> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>>> Here's yet another version of GSSAPI encryption support.
>>
>> This looks far more stable than last versions, cool to see the
>> progress. pgbench -C does not complain on my side so that's a good
>> thing. This is not yet a detailed review, there are a couple of
>> things that I find strange in what you did and are potential subject
>> to bugs, but I need a bit of time to let that mature a bit. This is
>> not something yet committable, but I really like the direction that
>> the patch is taking.

Thanks!  I must admit a preference for receiving all feedback at once
(reduces back-and-forth overhead), but if you feel there are still
design-type problems then that's very reasonable.  (I also admit to
feeling the pressure of feature freeze in less than a month.)

> For now, regarding 0002:
> /*
> -* Flush message so client will see it, except for AUTH_REQ_OK, which need
> -* not be sent until we are ready for queries.
> +* In most cases, we do not need to send AUTH_REQ_OK until we are ready
> +* for queries, but if we are doing GSSAPI encryption that request must go
> +* out now.
>  */
> -   if (areq != AUTH_REQ_OK)
> -   pq_flush();
> +   pq_flush();
> Er, this sends unconditionally the message without caring about the
> protocol, and so this is incorrect, no?

My impression from reading the old version of the comment above it was
that on other protocols it could go either way.  I think last time I
made it conditional; I can do so again if it is desired.  It's certainly
not /incorrect/ to send it immediately; there's just a question of
performance by minimizing the number of writes as far as I can tell.

> +#ifdef ENABLE_GSS
> +   if (conn->gss->buf.data)
> +   pfree(conn->gss->buf.data);
> +   if (conn->gss->writebuf.data)
> +   pfree(conn->gss->writebuf.data);
> +#endif
> You should use resetStringInfo here.

That will leak since resetStringInfo() doesn't free the underlying
representation.

>> OK, everything seems to be working fine with a 9.6 client and server so
>> next I tested older clients and I got this error:
>>
>> $ /usr/lib/postgresql/9.1/bin/psql -h localhost \
>>   -U vagr...@pgmasters.net postgres
>> psql: FATAL:  GSSAPI did no provide required flags
>>
>> There's a small typo in the error message, BTW.

Thanks, will fix.  I forgot that MIT doesn't provide GSS_C_REPLAY_FLAG
and GSS_C_SEQUENCE_FLAG by default.  Removing those from auth.c should
temporarily resolve the problem, which is what I'll do in the next
version.  (Tested with MIT krb5.)

On the subject of older code, I realized (one of those wake up in the
middle of the night-type moments) that the old server has a potential
problem with new clients now in that we try to decrypt the "help I don't
recognize connection parameter gss_encrypt" error message.  v8 will have
some sort of a fix, though I don't know what yet.  The only thing I've
come up with so far is to have the client decrypt check the first time
through for packets beginning with 'E'.  Pretty much anything I can do
will end up being a poor substitute for being at the protocol layer
anyway.

> And in 0003, the previous error is caused by that:
> +   target_flags = GSS_C_MUTUAL_FLAG | GSS_C_REPLAY_FLAG |
> +   GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG;
> +   if ((gflags & target_flags) != target_flags)
> +   {
> +   ereport(FATAL, (errmsg("GSSAPI did no provide required flags")));
> +   return STATUS_ERROR;
> +   }
> Yeah, this is a recipe for protocol incompatibility and here the
> connection context is not yet fully defined I believe. We need to be
> careful.

Nope, it's done.  This is happening immediately prior to username
checks.  By the time we exit the do/while the context is fully
complete.

> -   maj_stat = gss_accept_sec_context(
> - _stat,
> +   maj_stat = gss_accept_sec_context(_stat,
>
> This is just noise.

You're not wrong, though I do think it makes the code more readable by
enforcing style and this is a "cleanup" commit.  I'll take it out if it
bothers you.

signature.asc
Description: PGP signature

Re: [HACKERS] Default Roles

2016-03-15 Thread Stephen Frost

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Feb 29, 2016 at 10:02 PM, Stephen Frost  wrote:
> > Attached is a stripped-down version of the default roles patch which
> > includes only the 'pg_signal_backend' default role.  This provides the
> > framework and structure for other default roles to be added and formally
> > reserves the 'pg_' role namespace.  This is split into two patches, the
> > first to formally reserve 'pg_', the second to add the new default role.
> >
> > Will add to the March commitfest for review.
> 
> Here is a review of the first patch:
> 
> +   if (!IsA(node, RoleSpec))
> +   elog(ERROR, "invalid node type %d", node->type);
> 
> That looks strange.  Why not just Assert(IsA(node, RoleSpec))?

Sure, that works, done.

> +
> +   return;
> 
> Useless return.

Removed.

> @@ -673,6 +673,7 @@ dumpRoles(PGconn *conn)
>  "pg_catalog.shobj_description(oid,
> 'pg_authid') as rolcomment, "
>   "rolname =
> current_user AS is_current_user "
>   "FROM pg_authid "
> + "WHERE rolname !~ '^pg_' "
>   "ORDER BY 2");
> else if (server_version >= 90100)
> printfPQExpBuffer(buf,
> @@ -895,6 +896,7 @@ dumpRoleMembership(PGconn *conn)
>"LEFT JOIN pg_authid ur on
> ur.oid = a.roleid "
>"LEFT JOIN pg_authid um on
> um.oid = a.member "
>"LEFT JOIN pg_authid ug on
> ug.oid = a.grantor "
> +  "WHERE NOT (ur.rolname ~
> '^pg_' AND um.rolname ~ '^pg_')"
>"ORDER BY 1,2,3");
> 
> If I'm reading this correctly, when dumping a 9.5 server, we'll
> silently drop any roles whose names start with pg_ from the dump, and
> hope that doesn't break anything.  When dumping a 9.4 or older server,
> we'll include those roles and hope that they miraculously restore on
> the new server.  I'm thinking neither of those approaches is going to
> work out, and the difference between them seems totally unprincipled.

That needed to be updated to be 9.6 and 9.5, of course.

Further, you make a good point that 9.6's pg_dumpall should really
always exclude any roles which start with 'pg_', throwing a warning if
it finds them.

Note that pg_upgrade won't proceed with an upgrade of a system that has
any 'pg_' roles.

> @@ -631,7 +637,8 @@ check_is_install_user(ClusterInfo *cluster)
> res = executeQueryOrDie(conn,
> "SELECT rolsuper, oid 
> "
> "FROM
> pg_catalog.pg_roles "
> -   "WHERE rolname
> = current_user");
> +   "WHERE rolname
> = current_user "
> +   "AND rolname
> !~ '^pg_'");
> 
> /*
>  * We only allow the install user in the new cluster (see comment 
> below)
> @@ -647,7 +654,8 @@ check_is_install_user(ClusterInfo *cluster)
> 
> res = executeQueryOrDie(conn,
> "SELECT COUNT(*) "
> -   "FROM
> pg_catalog.pg_roles ");
> +   "FROM
> pg_catalog.pg_roles "
> +   "WHERE rolname
> !~ '^pg_'");
> 
> if (PQntuples(res) != 1)
> pg_fatal("could not determine the number of users\n");
> 
> What bad thing would happen without these changes that would be
> avoided with these changes?  I can't think of anything.

This function ("check_is_install_user") is simply checking that the user
we are logged in as is the 'install' user and that there aren't any
other users in the destination cluster.  The initial check is perhaps a
bit paranoid- it shouldn't be possible for a 'pg_' role to be the one
which pg_upgrade has logged into the cluster with as none of the 'pg_'
roles have the 'login' privilege.

For the second check, pg_upgrade expects a pristine cluster to perform
the binary upgrade into, which includes checking that there aren't any
roles besides the 'install' role in the destination cluster.  Since
default roles are created at initdb time, the destination cluster *will*
have other roles in it besides the install role, but only roles whose
names start with 'pg_', and those are ok because they're from initdb.

Updated (and rebased) patch attached.

Thanks for the review!

Stephen
From 090994ca5a9ae0c64f8752b43801141582f31af7 Mon Sep 17 00:00:00 2001
From: Stephen Frost 
Date: Mon, 29 Feb 2016 21:27:46

Re: [HACKERS] [PATCH] Supporting +-Infinity values by to_timestamp(float8)

On 3/4/16 2:56 PM, Vitaly Burovoy wrote:

> On 3/4/16, Anastasia Lubennikova  wrote:
>
>> I think that you should update documentation. At least description of
>> epoch on this page:
>> http://www.postgresql.org/docs/devel/static/functions-datetime.html
> 
> Thank you very much for pointing where it is located (I saw only
> "to_timestamp(TEXT, TEXT)").
> I'll think how to update it.

Vitaly, have you decided how to update this yet?

>> 3. (nitpicking) I don't sure about "4STAMPS" suffix. "4" is nice
>> abbreviation, but it seems slightly confusing to me.
> 
> It doesn't matter for me what it is called, it is short enough and
> reflects a type on which it is applied.
> What would the best name be for it?

Anastasia, any suggestions for a better name, or just leave it as is?

I'm not in favor of the "4", either.  I think I would prefer
JULIAN_MAXYEAR_STAMP.

-- 
-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] proposal: function parse_ident

2016-03-15 Thread Pavel Stehule

2016-03-14 17:39 GMT+01:00 Teodor Sigaev :

> I afraid so I cannot to fix this inconsistency (if this is inconsistency -
>> the
>> binary values are same) - the parameter of function is raw string with
>> processed
>> escape codes, and I have not any information about original escape
>> sequences.
>> When you enter octet value, and I show it as hex value, then there should
>> be
>> difference. Buy I have not information about your input (octet or hex). I
>> have
>> the original string of SQL identifier inside parser, executor, but I have
>> not
>> original string of function parameter inside function (not without pretty
>> complex and long code).
>>
> Ok, agree
>
>
>> I am trying describe it in doc (I am sorry for my less level English) in
>> new
>> patch. Fixed duplicated oid too.
>>
> Edited a bit + fix some typos and remove unneeded headers, patch attached
>
> Sorry, I can't find all corner-cases at once, but:
> SELECT parse_ident(E'"c".X XX');
> ERROR:  identifier contains disallowed characters: "\"c"
>

I'll check it tomorrow

Thank you

Pavel


>
> Error message wrongly points to the reason of error.
>
>
>
>
>
> --
> Teodor Sigaev   E-mail: teo...@sigaev.ru
>WWW:
> http://www.sigaev.ru/
>

Re: [HACKERS] pgbench stats per script & other stuff

On 3/4/16 1:53 PM, Fabien COELHO wrote:

>>> That is why the "fs" variable in process_file is declared "static",
>>> and why
>>> I wrote "some hidden awkwarness".
>>>
>>> I did want to avoid a malloc because then who would free the struct?
>>> addScript cannot to it systematically because builtins are static. Or it
>>> would have to create an on purpose struct, but I then that would be more
>>> awkwarness, and malloc/free to pass arguments between functions is not
>>> efficient nor very elegant.
>>>
>>> So the "static" option looked like the simplest & most elegant version.
>>
>> Surely that trick breaks if you have more than one -f switch, no?  Oh, I
>> see what you're doing: you only use the command list, which is
>> allocated, so it doesn't matter that the rest of the struct changes
>> later.
> 
> The two fields that matter (desc and commands) are really copied into
> sql_scripts, so what stays in the is overriden if used another time.
> 
>> I'm not concerned about freeing the struct; what's the problem with it
>> surviving until the program terminates?
> 
> It is not referenced anywhere so it is a memory leak.
> 
>> If somebody specifies thousands of -f switches, they will waste a few
>> bytes with each, but I'm hardly concerned about a few dozen kilobytes
>> there ...
> 
> Ok, so you prefer a memory leak. I hate it on principle.
> 
> Here is a v23 with a memory leak anyway.

Álvaro, it looks like you've been both reviewer and committer on this
work for some time.

The latest patch seems to address you final concern.  Can I mark it
"ready for committer"?

-- 
-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] Idle In Transaction Session Timeout, revived

2016-03-15 Thread Andres Freund

On 2016-03-15 14:21:34 -0400, Robert Haas wrote:
> On Tue, Mar 8, 2016 at 6:08 PM, Andres Freund  wrote:
> > On 2016-03-08 16:42:37 -0500, Robert Haas wrote:
> >> - I really wonder if the decision to ignore sessions that are idle in
> >> transaction (aborted) should revisited.  Consider this:
> >>
> >> rhaas=# begin;
> >> BEGIN
> >> rhaas=# lock table pg_class;
> >> LOCK TABLE
> >> rhaas=# savepoint a;
> >> SAVEPOINT
> >> rhaas=# select 1/0;
> >> ERROR:  division by zero
> >
> > Probably only if the toplevel transaction is also aborted. Might not be
> > entirely trivial to determine.
> 
> Yes, that would be one way to do it - or just ignore whether it's
> aborted or not and make the timeout always apply.  That seems pretty
> reasonable, too, because a transaction that's idle in transaction and
> aborted could easily be one that the client has forgotten about, even
> if it's not hanging onto any resources other than a connection slot.
> And, if it turns out that the client didn't forget about it, well,
> they don't lose anything by retrying the transaction on a new
> connection anyway.

I'm fine with both.

Andres


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

[HACKERS] Re: pg_hba_lookup function to get all matching pg_hba.conf entries

On 3/3/16 12:16 AM, Haribabu Kommi wrote:
> On Fri, Feb 5, 2016 at 2:29 PM, Haribabu Kommi  
> wrote:
>>
>> This patch needs to be applied on top discard_hba_and_ident_cxt patch
>> that is posted earlier.
> 
> Here I attached a re-based patch to the latest head with inclusion of
> discard_hba_ident_cxt patch for easier review as a single patch.

Alex, Scott, do you have an idea of when you'll be able to review this
new version?

It applies with a minor conflict (caused by pg_control_* commit):

$ git apply -3 ../other/pg_hba_lookup_poc_v13.patch
error: patch failed: src/include/utils/builtins.h:1151
Falling back to three-way merge...
Applied patch to 'src/include/utils/builtins.h' with conflicts.
U src/include/utils/builtins.h

Thanks,
-- 
-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] Idle In Transaction Session Timeout, revived

On Tue, Mar 8, 2016 at 6:08 PM, Andres Freund  wrote:
> On 2016-03-08 16:42:37 -0500, Robert Haas wrote:
>> - I really wonder if the decision to ignore sessions that are idle in
>> transaction (aborted) should revisited.  Consider this:
>>
>> rhaas=# begin;
>> BEGIN
>> rhaas=# lock table pg_class;
>> LOCK TABLE
>> rhaas=# savepoint a;
>> SAVEPOINT
>> rhaas=# select 1/0;
>> ERROR:  division by zero
>
> Probably only if the toplevel transaction is also aborted. Might not be
> entirely trivial to determine.

Yes, that would be one way to do it - or just ignore whether it's
aborted or not and make the timeout always apply.  That seems pretty
reasonable, too, because a transaction that's idle in transaction and
aborted could easily be one that the client has forgotten about, even
if it's not hanging onto any resources other than a connection slot.
And, if it turns out that the client didn't forget about it, well,
they don't lose anything by retrying the transaction on a new
connection anyway.

On a procedural note, I'm happy to push this patch through to commit
if it gets updated in the near future.  If not, we should mark it
Returned with Feedback and Vik can resubmit for 9.7.  Tempus fugit.

-- 
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

[HACKERS] Re: [PATCH] fix DROP OPERATOR to reset links to itself on commutator and negator

Hi Kevin,

On 3/1/16 11:08 AM, Roma Sokolov wrote:
>> On 27 Feb 2016, at 03:46, Euler Taveira  wrote:
>> Because it is not a common practice to test catalog dependency on
>> separate tests (AFAICS initial catalogs are tested with oidjoins.sql).
>> Also, your test case is too huge for such a small use case. 
> 
> It seems oidjoins.sql is automatically generated and contains tests only for
> initial database state. On the other hand, there are tests for CREATE OPERATOR
> and ALTER OPERATOR, so it seems reasonable to me to have separate DROP 
> OPERATOR
> test, or to move all operator related testing to one file. This is however
> clearly outside of the scope of this patch, so in v3 I've simplified tests 
> using
> queries from oidjoins.sql.

You've signed up to review this patch, do you have an idea of when you
might be able to do the review?

Thanks,
-- 
-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] IF (NOT) EXISTS in psql-completion

On 3/15/16 1:42 PM, Robert Haas wrote:
> On Fri, Feb 26, 2016 at 2:37 AM, Kyotaro HORIGUCHI
>  wrote:
>> Hello, this is the new version of this patch.
> 
> The CommitFest entry for this patch is messed up.  It shows no author,
> even though I'm pretty sure that a patch has to have one by
> definition.
> 
> https://commitfest.postgresql.org/9/518/
> 
> Also, this patch was listed as Waiting on Author, but it's been
> updated since it was last reviewed, so I switched it back to Needs
> Review.  Can someone please review it and, if appropriate, mark it
> Ready for Committer?

Author has been set to Kyotaro Horiguchi.

-- 
-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] ALTER TABLE behind-the-scenes effects' CONTEXT

On 1/30/16 10:52 AM, Marko Tiikkaja wrote:
> On 2016-01-21 04:17, Simon Riggs wrote:
>> Marko, I was/am waiting for an updated patch. Could you comment please?
> 
> Sorry, I've not found time to work on this recently.
> 
> Thanks for everyone's comments so far.  I'll move this to the next CF
> and try and get an updated patch done in time for that one.

There's been no activity on this thread for while and no new patch was
submitted for the CF.

I have marked it "returned with feedback".

-- 
-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] silent data loss with ext4 / all current versions

On 3/15/16 10:39 AM, Michael Paquier wrote:

> On Thu, Mar 10, 2016 at 4:25 AM, Andres Freund wrote:
> 
>> Note that we currently have some frontend programs with the equivalent
>> problem. Most importantly receivelog.c (pg_basebackup/pg_recveivexlog)
>> are missing pretty much the same directory fsyncs.  And at least for
>> pg_recvxlog it's critical, especially now that receivexlog support
>> syncrep.  I've not done anything about that; there's pretty much no
>> chance to share backend code with the frontend in the back-branches.
> 
> Yeah, true. We definitely need to do something for that, even for HEAD
> it seems like an overkill to have something in for example src/common
> to allow frontends to have something if the fix is localized
> (pg_rewind may use something else), and it would be nice to finish
> wrapping that for the next minor release, so I propose the attached
> patches.

I noticed this when reviewing the pg_receive_xlog refactor and was going
to submit a patch after the CF.  It didn't occur to me to piggyback on
this work but I think it makes sense.

+1 from me for fixing this in pg_receivexlog and back-patching.

-- 
-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] Soliciting Feedback on Improving Server-Side Programming Documentation

On Tue, Mar 15, 2016 at 1:35 PM, Joshua D. Drake 
wrote:

> On 03/15/2016 10:30 AM, Corey Huinker wrote:
>
>>
>> On Tue, Mar 15, 2016 at 1:19 PM, Shulgin, Oleksandr
>> >
>> wrote:
>>
>> There's also a good deal of README files in the source tree, so I
>> would add:
>>
>> 4. find src -name 'README*'
>>
>>
>> That too. But README's don't show up (easily) in a google search, so
>> they elude discovery. We should want to make discovery easy to the
>> uninitiated.
>>
>
> I don't think anyone is arguing with you. I think we are trying to point
> you to sources for your project.
>
>
I didn't mean to imply that anyone was arguing. All responses so far have
been positive.

Re: [HACKERS] silent data loss with ext4 / all current versions

2016-03-15 Thread Andres Freund

On 2016-03-15 15:39:50 +0100, Michael Paquier wrote:
> I have finally been able to spend some time reviewing what you pushed
> on back-branches, and things are in correct shape I think. One small
> issue that I have is that for EXEC_BACKEND builds, in
> write_nondefault_variables we still use one instance of rename().

Correctly so afaics, because write_nondefault_variables is by definition
non-durable. We write that stuff at every start / SIGHUP. Adding an
fsync there would be an unnecessary slowdown.  I don't think it's good
policy adding fsync for stuff that definitely doesn't need it.


> Yeah, true. We definitely need to do something for that, even for HEAD
> it seems like an overkill to have something in for example src/common
> to allow frontends to have something if the fix is localized
> (pg_rewind may use something else), and it would be nice to finish
> wrapping that for the next minor release, so I propose the attached
> patches. At the same time, I think that adminpack had better be fixed
> as well, so there are actually three patches in this series, things
> that I shaped thinking about a backpatch btw, particularly for 0002.

I'm doubtful about "fixing" adminpack. We don't know how it's used, and
this could *seriously* increase its overhead for something potentially
used at a high rate.


>  /*
> + * Wrapper of rename() similar to the backend version with the same function
> + * name aimed at making the renaming durable on disk. Note that this version
> + * does not fsync the old file before the rename as all the code paths 
> leading
> + * to this function are already doing this operation. The new file is also
> + * normally not present on disk before the renaming so there is no need to
> + * bother about it.
> + */
> +static int
> +durable_rename(const char *oldfile, const char *newfile)
> +{
> + int fd;
> + charparentpath[MAXPGPATH];
> +
> + if (rename(oldfile, newfile) != 0)
> + {
> + /* durable_rename produced a log entry */
> + fprintf(stderr, _("%s: could not rename file \"%s\": %s\n"),
> + progname, current_walfile_name, 
> strerror(errno));
> + return -1;
> + }
> +
> + /*
> +  * To guarantee renaming of the file is persistent, fsync the file with 
> its
> +  * new name, and its containing directory.
> +  */
> + fd = open(newfile, O_RDWR | PG_BINARY, S_IRUSR | S_IWUSR);

Why is S_IRUSR | S_IWUSR specified here?


Are you working on a fix for pg_rewind? Let's go with initdb -S in a
first iteration, then we can, if somebody is interest enough, work on
making this nicer in master.

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] Add numeric_trim(numeric)

On Wed, Jan 27, 2016 at 7:09 PM, Alvaro Herrera
 wrote:
>> Here's a patch for the second function suggested in 5643125e.1030...@joh.to.
>
> I think this patch got useful feedback but we never saw a followup
> version posted.  I closed it as returned-with-feedback.  Feel free to
> submit a new version for the 2016-03 commitfest.

This was switched back to Waiting on Author despite not having been
updated.  I have switched it back to Returned with Feedback.  It's
very difficult to focus on the patches in a CommitFest that have a
chance of actually being ready to commit when the list is cluttered
with entries that have not even been updated.

-- 
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] IF (NOT) EXISTS in psql-completion

On Fri, Feb 26, 2016 at 2:37 AM, Kyotaro HORIGUCHI
 wrote:
> Hello, this is the new version of this patch.

The CommitFest entry for this patch is messed up.  It shows no author,
even though I'm pretty sure that a patch has to have one by
definition.

https://commitfest.postgresql.org/9/518/

Also, this patch was listed as Waiting on Author, but it's been
updated since it was last reviewed, so I switched it back to Needs
Review.  Can someone please review it and, if appropriate, mark it
Ready for Committer?

-- 
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] Combining Aggregates

2016-03-15 Thread Tomas Vondra

Hi,

On 03/14/2016 05:45 AM, David Rowley wrote:

On 14 March 2016 at 15:20, David Rowley wrote:

Current patch:
I've now updated the patch to base it on top of the parallel aggregate
patch in [2]. To apply the attached, you must apply [2] first!

...

[2]
http://www.postgresql.org/message-id/cakjs1f9tr-+9akwz1xuhunejz7gkkfo45b+4fcnj8dkrdzy...@mail.gmail.com

The attached fixes a small thinko in apply_partialaggref_nodes().

After looking at the parallel aggregate patch, I also looked at this
one, as it's naturally related. Sadly I haven't found any issue that I
could nag about ;-) The patch seems well baked, as it was in the oven
for quite a long time already.

The one concern I do have is that it only adds (de)serialize functions
for SUM(numeric) and AVG(numeric). I think it's reasonable not to
include that into the patch, but it will look a bit silly if that's all
that gets into 9.6.

It's true plenty of aggregates is supported out of the box, as they use
fixed-length data types for the aggregate state. But imagine users happy
that their aggregate queries get parallelized, only to find out that
sum(int8) disables that :-/

So I think we should try to support as many of the aggregates using
internal as possible - it seems quite straight-forward to do that at
least for the aggregates using the same internal representation (avg,
sum, var_samp, var_pop, variance, ...). That's 26 aggregates out of the
45 with aggtranstype=INTERNAL.

For the remaining aggregates (jsonb_*. json_*, string_agg, array_agg,
percentile_) it's probably more complicated to serialize the state, and
maybe even impossible to do that correctly (e.g. string_agg accumulates
the strings in the order as received, but partial paths would build
private lists - not sure if this can actually happen in practice).

I think it would be good to actually document which aggregates support
parallel execution, and which don't. Currently the docs don't mention
this at all, and it's tricky for regular users to deduce that as it's
related to the type of the internal state and not to the input types. An
example of that is the SUM(bigint) example mentioned above.

That's actually the one thing I think the patch is missing.

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] Password identifiers, protocol aging and SCRAM protocol

Hi Michael,

On 3/14/16 7:07 PM, Michael Paquier wrote:

> On Mon, Mar 14, 2016 at 5:06 PM, Michael Paquier  
> wrote:
>
>> On Mon, Mar 14, 2016 at 4:32 PM, David Steele  wrote:
>>
>>> Could you provide an updated set of patches for review?  Meanwhile I am
>>> marking this as "waiting for author".
>>
>> Sure. I'll provide them shortly with all the comments addressed. Up to
>> now I just had a couple of comments about docs and whitespaces, so I
>> didn't really bother sending a new set, but this meritates a rebase.
> 
> And here they are. I have addressed the documentation and the
> whitespaces reported up to now at the same time.

For this first review I would like to focus on the user visible changes
introduced in 0001-0002.

First I created two new users with each type of supported verifier:

postgres=# create user test with password 'test';
CREATE ROLE
postgres=# create user testu with unencrypted password 'testu'
   valid until '2017-01-01';
CREATE ROLE

1) I see that rolvaliduntil is still in pg_authid:

postgres=# select oid, rolname, rolvaliduntil from pg_authid;

  oid  | rolname | rolvaliduntil
---+-+
10 | vagrant |
 16387 | test|
 16388 | testu   | 2017-01-01 00:00:00+00

I think that's OK if we now define it to be "role validity" (it's still
password validity in the patched docs).  I would also like to see a
validuntil column in pg_auth_verifiers so we can track password
expiration for each verifier separately.  For now I think it's enough to
copy the same validity both places since there can only be one verifier.

2) I don't think the column naming in pg_auth_verifiers is consistent
with other catalogs:

postgres=# select * from pg_auth_verifiers;

 roleid | verimet |   verival
+-+-
  16387 | m   | md505a671c66aefea124cc08b76ea6d30bb
  16388 | p   | testu

System catalogs generally use a 3 character prefix so I would expect the
columns to be (if we pick avr as a prefix):

avrrole
avrmethod
avrverifier
avrvaliduntil

I'm not a big fan in abbreviating too much so you can see I've expanded
the names a bit.

3) rolpassword is still in pg_shadow even though it is not useful anymore:

postgres=# select usename, passwd, valuntil from pg_shadow;

 usename |  passwd  |valuntil
-+--+
 vagrant |  |
 test|  |
 testu   |  | 2017-01-01 00:00:00+00

If anyone is actually using this column in a meaningful way they are in
for a nasty surprise when trying use the value in passwd as a verifier.
 I would prefer to drop the column entirely and produce a clear error.

Perhaps a better option would be to drop pg_shadow entirely since it
seems to have no further purpose in life.

Thanks,
-- 
-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] [WIP] Effective storage of duplicates in B-tree index.

2016-03-15 Thread Anastasia Lubennikova


14.03.2016 16:02, David Steele:

Hi Anastasia,

On 2/18/16 12:29 PM, Anastasia Lubennikova wrote:

18.02.2016 20:18, Anastasia Lubennikova:

04.02.2016 20:16, Peter Geoghegan:

On Fri, Jan 29, 2016 at 8:50 AM, Anastasia Lubennikova
  wrote:

I fixed it in the new version (attached).


Thank you for the review.
At last, there is a new patch version 3.0. After some refactoring it
looks much better.
I described all details of the compression in this document
https://goo.gl/50O8Q0 (the same text without pictures is attached in
btc_readme_1.0.txt).
Consider it as a rough copy of readme. It contains some notes about
tricky moments of implementation and questions about future work.
Please don't hesitate to comment it.


Sorry, previous patch was dirty. Hotfix is attached.


This looks like an extremely valuable optimization for btree indexes 
but unfortunately it is not getting a lot of attention. It still 
applies cleanly for anyone interested in reviewing.




Thank you for attention.
I would be indebted to all reviewers, who can just try this patch on 
real data and workload (except WAL for now).

B-tree needs very much testing.

It's not clear to me that you answered all of Peter's questions in 
[1].  I understand that you've provided a README but it may not be 
clear if the answers are in there (and where).


I described in README all the points Peter asked.
But I see that it'd be better to answer directly.
Thanks for reminding, I'll do it tomorrow.


Also, at the end of the README it says:

13. Xlog. TODO.

Does that mean the patch is not yet complete?


Yes, you're right.
Frankly speaking, I supposed that someone will help me with that stuff,
but now I almost completed it. I'll send updated patch in the next letter.

I'm still doubtful about some patch details. I mentioned them in readme 
(bold type).

But they are mostly about future improvements.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres 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] Soliciting Feedback on Improving Server-Side Programming Documentation

2016-03-15 Thread Joshua D. Drake


On 03/15/2016 10:30 AM, Corey Huinker wrote:


On Tue, Mar 15, 2016 at 1:19 PM, Shulgin, Oleksandr
> wrote:

There's also a good deal of README files in the source tree, so I
would add:

4. find src -name 'README*'


That too. But README's don't show up (easily) in a google search, so
they elude discovery. We should want to make discovery easy to the
uninitiated.


I don't think anyone is arguing with you. I think we are trying to point 
you to sources for your project.


JD

--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


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

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

On Tue, Mar 15, 2016 at 1:16 AM, Amit Langote
 wrote:
> On 2016/03/15 3:41, Robert Haas wrote:
>> On Sat, Mar 12, 2016 at 7:49 AM, Amit Langote  
>> wrote:
>>> Instead, the attached patch adds a IndexBulkDeleteProgressCallback
>>> which AMs should call for every block that's read (say, right before a
>>> call to ReadBufferExtended) as part of a given vacuum run. The
>>> callback with help of some bookkeeping state can count each block and
>>> report to pgstat_progress API. Now, I am not sure if all AMs read 1..N
>>> blocks for every vacuum or if it's possible that some blocks are read
>>> more than once in single vacuum, etc.  IOW, some AM's processing may
>>> be non-linear and counting blocks 1..N (where N is reported total
>>> index blocks) may not be possible.  However, this is the best I could
>>> think of as doing what we are trying to do here. Maybe index AM
>>> experts can chime in on that.
>>>
>>> Thoughts?
>>
>> Well, I think you need to study the index AMs and figure this out.
>
> OK.  I tried to put calls to the callback in appropriate places, but
> couldn't get the resulting progress numbers to look sane.  So I ended up
> concluding that any attempt to do so is futile unless I analyze each AM's
> vacuum code carefully to be able to determine in advance the max bound on
> the count of blocks that the callback will report.  Anyway, as you
> suggest, we can improve it later.

I don't think there is any way to bound that, because new blocks can
get added to the index concurrently, and we might end up needing to
scan them.  Reporting the number of blocks scanned can still be
useful, though - any chance you can just implement that part of it?

>> But I think for starters you should write a patch that reports the following:
>>
>> 1. phase
>> 2. number of heap blocks scanned
>> 3. number of heap blocks vacuumed
>> 4. number of completed index vac cycles
>> 5. number of dead tuples collected since the last index vac cycle
>> 6. number of dead tuples that we can store before needing to perform
>> an index vac cycle
>>
>> All of that should be pretty straightforward, and then we'd have
>> something we can ship.  We can add the detailed index reporting later,
>> when we get to it, perhaps for 9.7.
>
> OK, I agree with this plan.  Attached updated patch implements this.

Sorta.  Committed after renaming what you called heap blocks vacuumed
back to heap blocks scanned, adding heap blocks vacuumed, removing the
overall progress meter which I don't believe will be anything close to
accurate, fixing some stylistic stuff, arranging to update multiple
counters automatically where it could otherwise produce confusion,
moving the new view near similar ones in the file, reformatting it to
follow the style of the rest of the file, exposing the counter
#defines via a header file in case extensions want to use them, and
overhauling and substantially expanding the documentation.

-- 
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] Small patch: fix warnings during compilation on FreeBSD

2016-03-15 Thread Tom Lane

Aleksander Alekseev  writes:
> OK, I'm not an expert in Autotools but this patch (see attachment) seems
> to solve a problem.

I fooled around with this some.  I felt originally that it should use
AC_CHECK_DECL, but that turns out not to work because AC_CHECK_DECL
has caching behavior built-in and so the second call did nothing.
However, we should still adopt the probe methodology it uses rather
than inventing our own; basically that's

#ifndef wcstombs_l
(void) wcstombs_l;
#endif

Also, after reviewing the bidding a bit more it seems likely to me
that wcstombs_l() might be declared in  on some platforms;
which would be problematic for this test as written if 
pulls in .  So the right way to make the comparison is to
determine whether stdlib.h+locale.h+xlocale.h succeeds where
stdlib.h+locale.h fails.

I've checked this on my OS X box, which turns out to have the interesting
property that xlocale.h declares wcstombs_l(), but only if you previously
included stdlib.h ... wtf?  But anyway that behavior is part of the
motivation for leaving stdlib.h in the test.

Please verify that the committed version solves your problem on FreeBSD.

> Please note that these changes:
> ... were generated but `autoreconf -iv`. I was not sure what to do
> about them. Eventually I decided to keep them. Still these changes could
> be safely discarded.

Yeah, it's not uncommon for platforms to carry local patches in their
autoconf packages, which results in changes like these.  We make a point
of generating our configure using unmodified GNU autoconf installations,
so as to avoid dependencies on which platform a committer was using to
run autoconf.

regards, tom lane

diff --git a/config/c-library.m4 b/config/c-library.m4
index 1d28c45..50d068d 100644
*** a/config/c-library.m4
--- b/config/c-library.m4
*** fi
*** 316,319 
  if test "$pgac_cv_type_locale_t" = 'yes (in xlocale.h)'; then
AC_DEFINE(LOCALE_T_IN_XLOCALE, 1,
  [Define to 1 if `locale_t' requires .])
! fi])])# PGAC_HEADER_XLOCALE
--- 316,349 
  if test "$pgac_cv_type_locale_t" = 'yes (in xlocale.h)'; then
AC_DEFINE(LOCALE_T_IN_XLOCALE, 1,
  [Define to 1 if `locale_t' requires .])
! fi])# PGAC_TYPE_LOCALE_T
! 
! 
! # PGAC_FUNC_WCSTOMBS_L
! # 
! # Try to find a declaration for wcstombs_l().  It might be in stdlib.h
! # (following the POSIX requirement for wcstombs()), or in locale.h, or in
! # xlocale.h.  If it's in the latter, define WCSTOMBS_L_IN_XLOCALE.
! #
! AC_DEFUN([PGAC_FUNC_WCSTOMBS_L],
! [AC_CACHE_CHECK([for wcstombs_l declaration], pgac_cv_func_wcstombs_l,
! [AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
! [#include 
! #include ],
! [#ifndef wcstombs_l
! (void) wcstombs_l;
! #endif])],
! [pgac_cv_func_wcstombs_l='yes'],
! [AC_COMPILE_IFELSE([AC_LANG_PROGRAM(
! [#include 
! #include 
! #include ],
! [#ifndef wcstombs_l
! (void) wcstombs_l;
! #endif])],
! [pgac_cv_func_wcstombs_l='yes (in xlocale.h)'],
! [pgac_cv_func_wcstombs_l='no'])])])
! if test "$pgac_cv_func_wcstombs_l" = 'yes (in xlocale.h)'; then
!   AC_DEFINE(WCSTOMBS_L_IN_XLOCALE, 1,
! [Define to 1 if `wcstombs_l' requires .])
! fi])# PGAC_FUNC_WCSTOMBS_L
diff --git a/configure b/configure
index 08cff23..a45be67 100755
*** a/configure
--- b/configure
*** $as_echo "#define GETTIMEOFDAY_1ARG 1" >
*** 12364,12369 
--- 12364,12422 
  
  fi
  
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for wcstombs_l declaration" >&5
+ $as_echo_n "checking for wcstombs_l declaration... " >&6; }
+ if ${pgac_cv_func_wcstombs_l+:} false; then :
+   $as_echo_n "(cached) " >&6
+ else
+   cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+ /* end confdefs.h.  */
+ #include 
+ #include 
+ int
+ main ()
+ {
+ #ifndef wcstombs_l
+ (void) wcstombs_l;
+ #endif
+   ;
+   return 0;
+ }
+ _ACEOF
+ if ac_fn_c_try_compile "$LINENO"; then :
+   pgac_cv_func_wcstombs_l='yes'
+ else
+   cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+ /* end confdefs.h.  */
+ #include 
+ #include 
+ #include 
+ int
+ main ()
+ {
+ #ifndef wcstombs_l
+ (void) wcstombs_l;
+ #endif
+   ;
+   return 0;
+ }
+ _ACEOF
+ if ac_fn_c_try_compile "$LINENO"; then :
+   pgac_cv_func_wcstombs_l='yes (in xlocale.h)'
+ else
+   pgac_cv_func_wcstombs_l='no'
+ fi
+ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ fi
+ rm -f core conftest.err conftest.$ac_objext conftest.$ac_ext
+ fi
+ { $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_func_wcstombs_l" >&5
+ $as_echo "$pgac_cv_func_wcstombs_l" >&6; }
+ if test "$pgac_cv_func_wcstombs_l" = 'yes (in xlocale.h)'; then
+ 
+ $as_echo "#define WCSTOMBS_L_IN_XLOCALE 1" >>confdefs.h
+ 
+ fi
  
  # Some versions of libedit contain strlcpy(), setproctitle(), and other
  # symbols that that library has no business exposing to the world.  Pending
diff --git a/configure.in b/configure.in
index 0b7dd97..c298926 100644
*** a/configure.in
--- b/configure.in
*** fi
***

Re: [HACKERS] Soliciting Feedback on Improving Server-Side Programming Documentation

On Tue, Mar 15, 2016 at 1:19 PM, Shulgin, Oleksandr <
oleksandr.shul...@zalando.de> wrote:

> There's also a good deal of README files in the source tree, so I would
> add:
>
> 4. find src -name 'README*'
>

That too. But README's don't show up (easily) in a google search, so they
elude discovery. We should want to make discovery easy to the uninitiated.

Re: [HACKERS] Soliciting Feedback on Improving Server-Side Programming Documentation

>
>
> I think this is all great. You may find some automated assistance from
> doxygen.postgresql.org .
>
> Sincerely,
>
> JD


doxygen is great as far as it goes, but it does a great job of separating
function definition from the comment explaining the function, so I have to
drill into the raw source anyway.

Also, doxygen isn't very helpful with macros, and a lot of functions in the
internals are actually macros.

Re: [HACKERS] WAL logging problem in 9.4.3?

On Fri, Mar 11, 2016 at 9:32 AM, Kyotaro HORIGUCHI
 wrote:
> At Fri, 19 Feb 2016 22:27:00 +0900, Michael Paquier 
>  wrote in 
> 
>> > Worth noting that this patch does not address the problem with index
>> > relations when a TRUNCATE is used in the same transaction as its
>
> Focusing this issue, what we should do is somehow building empty
> index just after a index truncation. The attached patch does the
> following things to fix this.
>
> - make index_build use ambuildempty when the relation on which
>   the index will be built is apparently empty. That is, when the
>   relation has no block.
> - add one parameter "persistent" to ambuildempty(). It behaves as
>   before if the parameter is false. If not, it creates an empty
>   index on MAIN_FORK and emits logs even if wal_level is minimal.

Hm. It seems to me that this patch is just a bandaid for the real
problem which is that we should not TRUNCATE the underlying index
relations when the TRUNCATE optimization is running. In short I would
let the empty routines in AM code paths alone, and just continue using
them for the generation of INIT_FORKNUM with unlogged relations. Your
patch is not something backpatchable anyway I think.

> The new parameter 'persistent' would be better be forknum because
> it actually represents the persistency of the index to be
> created. But I'm out of time now..

I actually have some users running with wal_level to minimal, even if
I don't think they use this optimization, we had better fix even index
relations at the same time as table relations.. I'll try to get some
time once the patch review storm goes down a little, except if someone
beats me to it first.
-- 
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] Soliciting Feedback on Improving Server-Side Programming Documentation

2016-03-15 Thread Shulgin, Oleksandr

On Tue, Mar 15, 2016 at 6:02 PM, Corey Huinker 
wrote:

> Over the past few months, I've been familiarizing myself with postgres
> server side programming in C.
>
> My attempts to educate myself were slow and halting. The existing server
> side programming documentation has some examples, but those examples didn't
> show me how do what I wanted to do, and my research-via-google was highly
> circular, almost always pointing back to the documentation I had already
> found lacking, or a copy of it.
>
> Most of what I have learned I have culled from asking people on IRC, or
> bugging people I've met through user groups and PgConf. In all cases,
> people have been extremely helpful. However, this method is inefficient,
> because we're using two people's time, one of whom has to tolerate my
> incessant questions and slow learning pace.
>
> Furthermore, the helpful suggestions I received boiled down to:
> 1. The function/macro/var you're looking for is PG_FOO, git grep PG_FOO
> 2. Look in blah.c which does something like what you're trying to do
> 3. The comments in blah.h do a good job of listing and explaining this
> macro or that
>

There's also a good deal of README files in the source tree, so I would add:

4. find src -name 'README*'

[HACKERS] Re: BUG #13685: Archiving while idle every archive_timeout with wal_level hot_standby

On Mon, Mar 14, 2016 at 6:46 PM, David Steele  wrote:
> On 2/24/16 12:40 AM, Michael Paquier wrote:
>
>> This has the merit to be clear, thanks for the input. Whatever the
>> approach taken at the end we have two candidates:
>> - Extend XLogInsert() with an extra argument for flags (Andres)
>> - Introduce XLogInsertExtended with this extra argument and let
>> XLogInsert() in peace (Robert and I).
>> Actually, I lied, there was still something I could do for this
>> thread: attached are two patches implementing both approaches as
>> respectively a-1 and a-2. Patch b is the set of logs I used for the
>> tests to show with a low checkpoint_timeout that checkpoints are
>> getting correctly skipped on an idle system.
>
>
> Unfortunately neither A nor B apply anymore.
>
> However, since the patches can still be read through I wonder if Robert or
> Andres would care to opine on whether A or B is better now that they can see
> the full implementation?
>
> For my 2c I'm happy with XLogInsertExtended() since it seems to be a rare
> use case where flags are required.  This can always be refactored in the
> future if/when the use of flags spreads.
>
> I think it would be good to make a decision on this before asking Michael to
> rebase.

That's a bit embarrassing, the last versions should be able to apply
cleanly as there have not been changes in this area of the code
lately... But... I did a mistake when generating the patches by
diff'ing them from an incorrect commit number... This explains why
they exploded in size, so attached are the corrected rebased versions.
Too many patches I guess.. And both of them are attached by the way.
-- 
Michael
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index d7973bc..a2b4aff 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -8343,8 +8343,12 @@ CreateCheckPoint(int flags)
 	if ((flags & (CHECKPOINT_IS_SHUTDOWN | CHECKPOINT_END_OF_RECOVERY |
   CHECKPOINT_FORCE)) == 0)
 	{
+		elog(LOG, "Not a forced or shutdown checkpoint: progress_lsn %X/%X, ckpt %X/%X",
+			 (uint32) (progress_lsn >> 32), (uint32) progress_lsn,
+			 (uint32) (ControlFile->checkPoint >> 32), (uint32) ControlFile->checkPoint);
 		if (progress_lsn == ControlFile->checkPoint)
 		{
+			elog(LOG, "Checkpoint is skipped");
 			WALInsertLockRelease();
 			LWLockRelease(CheckpointLock);
 			END_CRIT_SECTION();
@@ -8511,7 +8515,11 @@ CreateCheckPoint(int flags)
 	 * recovery we don't need to write running xact data.
 	 */
 	if (!shutdown && XLogStandbyInfoActive())
-		LogStandbySnapshot();
+	{
+		XLogRecPtr lsn = LogStandbySnapshot();
+		elog(LOG, "snapshot taken by checkpoint %X/%X",
+			 (uint32) (lsn >> 32), (uint32) lsn);
+	}
 
 	START_CRIT_SECTION();
 
diff --git a/src/backend/postmaster/bgwriter.c b/src/backend/postmaster/bgwriter.c
index 79cfd7b..082e589 100644
--- a/src/backend/postmaster/bgwriter.c
+++ b/src/backend/postmaster/bgwriter.c
@@ -333,7 +333,9 @@ BackgroundWriterMain(void)
 GetLastCheckpointRecPtr() < current_progress_lsn &&
 last_progress_lsn < current_progress_lsn)
 			{
-(void) LogStandbySnapshot();
+XLogRecPtr lsn = LogStandbySnapshot();
+elog(LOG, "snapshot taken by bgwriter %X/%X",
+	 (uint32) (lsn >> 32), (uint32) lsn);
 last_snapshot_ts = now;
 last_progress_lsn = current_progress_lsn;
 			}
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index c740952..22ebba5 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -634,7 +634,7 @@ brinbuild(Relation heap, Relation index, IndexInfo *indexInfo)
 		XLogRegisterData((char *) , SizeOfBrinCreateIdx);
 		XLogRegisterBuffer(0, meta, REGBUF_WILL_INIT);
 
-		recptr = XLogInsert(RM_BRIN_ID, XLOG_BRIN_CREATE_INDEX);
+		recptr = XLogInsert(RM_BRIN_ID, XLOG_BRIN_CREATE_INDEX, 0);
 
 		page = BufferGetPage(meta);
 		PageSetLSN(page, recptr);
diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.c
index d0ca485..98fcc2c 100644
--- a/src/backend/access/brin/brin_pageops.c
+++ b/src/backend/access/brin/brin_pageops.c
@@ -199,7 +199,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
 			XLogRegisterBuffer(0, oldbuf, REGBUF_STANDARD);
 			XLogRegisterBufData(0, (char *) newtup, newsz);
 
-			recptr = XLogInsert(RM_BRIN_ID, info);
+			recptr = XLogInsert(RM_BRIN_ID, info, 0);
 
 			PageSetLSN(oldpage, recptr);
 		}
@@ -294,7 +294,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
 			/* old page */
 			XLogRegisterBuffer(2, oldbuf, REGBUF_STANDARD);
 
-			recptr = XLogInsert(RM_BRIN_ID, info);
+			recptr = XLogInsert(RM_BRIN_ID, info, 0);
 
 			PageSetLSN(oldpage, recptr);
 			PageSetLSN(newpage, recptr);
@@ -444,7 +444,7 @@ brin_doinsert(Relation idxrel, BlockNumber pagesPerRange,
 
 		XLogRegisterBuffer(1, revmapbuf, 0);
 
-		recptr = XLogInsert(RM_BRIN_ID, info);
+		recptr = XLogInsert(RM_BRIN_ID, info, 0);

Re: [HACKERS] Soliciting Feedback on Improving Server-Side Programming Documentation

2016-03-15 Thread Joshua D. Drake


On 03/15/2016 10:02 AM, Corey Huinker wrote:


Some of these things may seem obvious/trivial to you. I would argue that
they're only obvious in retrospect, and the more obvious-to-you things
we robustly document, the quicker we accumulate programmers who are
capable of agreeing that it's obvious, and that's good for the community.

I'm aware that some of these APIs change frequently. In those cases, I
suggest that we make note of that on the same page.


I think this is all great. You may find some automated assistance from 
doxygen.postgresql.org .


Sincerely,

JD


--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


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

[HACKERS] Soliciting Feedback on Improving Server-Side Programming Documentation

Over the past few months, I've been familiarizing myself with postgres
server side programming in C.

My attempts to educate myself were slow and halting. The existing server
side programming documentation has some examples, but those examples didn't
show me how do what I wanted to do, and my research-via-google was highly
circular, almost always pointing back to the documentation I had already
found lacking, or a copy of it.

Most of what I have learned I have culled from asking people on IRC, or
bugging people I've met through user groups and PgConf. In all cases,
people have been extremely helpful. However, this method is inefficient,
because we're using two people's time, one of whom has to tolerate my
incessant questions and slow learning pace.

Furthermore, the helpful suggestions I received boiled down to:
1. The function/macro/var you're looking for is PG_FOO, git grep PG_FOO
2. Look in blah.c which does something like what you're trying to do
3. The comments in blah.h do a good job of listing and explaining this
macro or that

#1 git grep is a helpful reflex for discovering examples on my own, but it
requires that I have a term to search on in the first place, and too often
I don't know what I don't know.

#2 is the gold standard in terms of correctness (the code had to have
worked at least up to the last checkin date), and in terms of
discoverability it often gave me names of new macros to search for, coding
patterns, etc. However, I was always left with the questions: How would I
have figured this out on my own? How is the next person going to figure it
out? Why doesn't anybody document this?

#3 Often answers the last question in #2: It *is* documented, but that
documentation is not easily discoverable by conventional means.

So what I'd like to do is migrate some of the helpful information in the
header files into pages of web searchable documentation, and also to revamp
the existing documentation to be more relevant.

Along the way, I collected a list of things I wished I'd had from the start:

   - A list of all the GETARG_* macros. It would have been especially great
   if this were in table form:   Your Parameter Is A / Use This Macro / Which
   Gives This Result Type / Working example.
   - A list/table of the DatumGet* macros. I'm aware that many of them
   overlap/duplicate others. That'd be good to know too.
   - The table at
   http://www.postgresql.org/docs/9.1/static/errcodes-appendix.html has the
   numeric codes and PL/PGSQL constants enumerated. It'd be nice if it had the
   C #define as well
   - The SPI documentation mentions most/all of the SPI functions, but I
   couldn't find documentation on the SPI variables like SPI_processed and
   SPI_tuptable.
   - Examples and explanation of how PG_TRY()/PG_CATCH work. How to add
   context callbacks.
   - Direct Function Calls
   - A comparison of the two modes of writing SRF functions (Materialize vs
   multi-call)
   - Less explanation of how to do write V0-style functions. That was
   called the "old style" back in version 7.1. Why is that information up
   front in the documentation when so much else is sequestered in header files?

Some of these things may seem obvious/trivial to you. I would argue that
they're only obvious in retrospect, and the more obvious-to-you things we
robustly document, the quicker we accumulate programmers who are capable of
agreeing that it's obvious, and that's good for the community.

I'm aware that some of these APIs change frequently. In those cases, I
suggest that we make note of that on the same page.

Because I'm still going through the learning curve, I'm probably the least
qualified to write the actual documentation. However, I have a clear memory
of what was hard to learn and I have the motivation to make it easier on
the next person. That makes me a good focal point for gathering,
formatting, and submitting the documentation in patch form. I'm
volunteering to do so. What I need from you is:

   - Citations of existing documentation in header files that could/should
   be exposed in our more formal documentation.
   - Explanations of any of the things above, which I can then reformat
   into proposed documentation.
   - A willingness to review the proposed new documentation
   - Reasoned explanations for why this is a fool's errand

You supply the expertise, I'll write the patch.

Thanks in advance.

Re: [HACKERS] Small patch: fix warnings during compilation on FreeBSD

Aleksander Alekseev wrote:

> Please note that these changes:
> 
> ```
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T off_t) 1 << 31) << 31) - 1 + (((off_t) 1 <<
> 31) << 31))
> ```
> 
> ... were generated but `autoreconf -iv`. I was not sure what to do
> about them. Eventually I decided to keep them. Still these changes could
> be safely discarded.

I have noticed these while messing with configure.in too.  These are
generated by an autoconf as patched by Debian; see
http://bugs.debian.org/742780
This is said to fix undefined behavior when off_t is 32 bits.

They are not present in stock GNU autoconf 2.69 nor are in autoconf's
git repo.  I think we should continue to use the output of stock,
unpatched autoconf.

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

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

Re: [HACKERS] Background Processes and reporting

2016-03-15 Thread Oleg Bartunov

On Tue, Mar 15, 2016 at 7:43 PM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Tue, Mar 15, 2016 at 12:57 AM, Robert Haas 
> wrote:
>
>> On Mon, Mar 14, 2016 at 4:42 PM, Andres Freund 
>> wrote:
>> > On 2016-03-14 16:16:43 -0400, Robert Haas wrote:
>> >> > I have already shown [0, 1] the overhead of measuring timings in
>> linux on
>> >> > representative workload. AFAIK, these tests were the only one that
>> showed
>> >> > any numbers. All other statements about terrible performance have
>> been and
>> >> > remain unconfirmed.
>> >>
>> >> Of course, those numbers are substantial regressions which would
>> >> likely make it impractical to turn this on on a heavily-loaded
>> >> production system.
>> >
>> > A lot of people operating production systems are fine with trading a <=
>> > 10% impact for more insight into the system; especially if that
>> > configuration can be changed without a restart.  I know a lot of systems
>> > that use pg_stat_statements, track_io_timing = on, etc; just to get
>> > that. In fact there's people running perf more or less continuously in
>> > production environments; just to get more insight.
>> >
>> > I think it's important to get as much information out there without
>> > performance overhead, so it can be enabled by default. But I don't think
>> > it makes sense to not allow features in that cannot be enabled by
>> > default, *if* we tried to make them cheap enough beforehand.
>>
>> Hmm, OK.  I would have expected you to be on the other side of this
>> question, so maybe I'm all wet.  One point I am concerned about is
>> that, right now, we have only a handful of types of wait events.  I'm
>> very interested in seeing us add more, like I/O and client wait.  So
>> any overhead we pay here is likely to eventually be paid in a lot of
>> places - thus it had better be extremely small.
>>
>
> OK. Let's start to produce light, not heat.
>
> As I get we have two features which we suspect to introduce overhead:
> 1) Recording parameters of wait events which requires some kind of
> synchronization protocol.
> 2) Recording time of wait events because time measurements might be
> expensive on some platforms.
>
> Simultaneously there are machines and workloads where both of these
> features doesn't produce measurable overhead.  And, we're talking not about
> toy databases. Vladimir is DBA from Yandex which is in TOP-20 (by traffic)
> internet companies in the world.  They do run both of this features in
> production highload database without noticing any overhead of them.
>

> It would be great progress, if we decide that we could add both of these
> features controlled by GUC (off by default).
>

enable_waits_statistics ?


>
> If we decide so, then let's start working on this. At first, we should
> construct list of machines and workloads for testing. Each list of machines
> and workloads would be not comprehensive. But let's find something that
> would be enough for testing of GUC controlled, off by default features.
> Then we can turn our conversation from theoretical thoughts to particular
> benchmarks which would be objective and convincing to everybody.
>

Vladimir, could you provide a test suite, so other people could measure
overhead on their machines ?




>
> Otherwise, let's just add these features to the list of unwanted
> functionality and close this question.
>
> --
> Alexander Korotkov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>

Re: [HACKERS] [PATCH v6] GSSAPI encryption support

On Tue, Mar 15, 2016 at 3:12 PM, David Steele  wrote:
> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>> Here's yet another version of GSSAPI encryption support.

This looks far more stable than last versions, cool to see the
progress. pgbench -C does not complain on my side so that's a good
thing. This is not yet a detailed review, there are a couple of things
that I find strange in what you did and are potential subject to bugs,
but I need a bit of time to let that mature a bit. This is not
something yet committable, but I really like the direction that the
patch is taking.

For now, regarding 0002:
/*
-* Flush message so client will see it, except for AUTH_REQ_OK, which need
-* not be sent until we are ready for queries.
+* In most cases, we do not need to send AUTH_REQ_OK until we are ready
+* for queries, but if we are doing GSSAPI encryption that request must go
+* out now.
 */
-   if (areq != AUTH_REQ_OK)
-   pq_flush();
+   pq_flush();
Er, this sends unconditionally the message without caring about the
protocol, and so this is incorrect, no?

+#ifdef ENABLE_GSS
+   if (conn->gss->buf.data)
+   pfree(conn->gss->buf.data);
+   if (conn->gss->writebuf.data)
+   pfree(conn->gss->writebuf.data);
+#endif
You should use resetStringInfo here.

> OK, everything seems to be working fine with a 9.6 client and server so
> next I tested older clients and I got this error:
>
> $ /usr/lib/postgresql/9.1/bin/psql -h localhost \
>   -U vagr...@pgmasters.net postgres
> psql: FATAL:  GSSAPI did no provide required flags
>
> There's a small typo in the error message, BTW.

And in 0003, the previous error is caused by that:
+   target_flags = GSS_C_MUTUAL_FLAG | GSS_C_REPLAY_FLAG |
+   GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG;
+   if ((gflags & target_flags) != target_flags)
+   {
+   ereport(FATAL, (errmsg("GSSAPI did no provide required flags")));
+   return STATUS_ERROR;
+   }
Yeah, this is a recipe for protocol incompatibility and here the
connection context is not yet fully defined I believe. We need to be
careful.

-   maj_stat = gss_accept_sec_context(
- _stat,
+   maj_stat = gss_accept_sec_context(_stat,
This is just noise.
-- 
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] Background Processes and reporting

2016-03-15 Thread Alexander Korotkov

On Tue, Mar 15, 2016 at 12:57 AM, Robert Haas  wrote:

> On Mon, Mar 14, 2016 at 4:42 PM, Andres Freund  wrote:
> > On 2016-03-14 16:16:43 -0400, Robert Haas wrote:
> >> > I have already shown [0, 1] the overhead of measuring timings in
> linux on
> >> > representative workload. AFAIK, these tests were the only one that
> showed
> >> > any numbers. All other statements about terrible performance have
> been and
> >> > remain unconfirmed.
> >>
> >> Of course, those numbers are substantial regressions which would
> >> likely make it impractical to turn this on on a heavily-loaded
> >> production system.
> >
> > A lot of people operating production systems are fine with trading a <=
> > 10% impact for more insight into the system; especially if that
> > configuration can be changed without a restart.  I know a lot of systems
> > that use pg_stat_statements, track_io_timing = on, etc; just to get
> > that. In fact there's people running perf more or less continuously in
> > production environments; just to get more insight.
> >
> > I think it's important to get as much information out there without
> > performance overhead, so it can be enabled by default. But I don't think
> > it makes sense to not allow features in that cannot be enabled by
> > default, *if* we tried to make them cheap enough beforehand.
>
> Hmm, OK.  I would have expected you to be on the other side of this
> question, so maybe I'm all wet.  One point I am concerned about is
> that, right now, we have only a handful of types of wait events.  I'm
> very interested in seeing us add more, like I/O and client wait.  So
> any overhead we pay here is likely to eventually be paid in a lot of
> places - thus it had better be extremely small.
>

OK. Let's start to produce light, not heat.

As I get we have two features which we suspect to introduce overhead:
1) Recording parameters of wait events which requires some kind of
synchronization protocol.
2) Recording time of wait events because time measurements might be
expensive on some platforms.

Simultaneously there are machines and workloads where both of these
features doesn't produce measurable overhead.  And, we're talking not about
toy databases. Vladimir is DBA from Yandex which is in TOP-20 (by traffic)
internet companies in the world.  They do run both of this features in
production highload database without noticing any overhead of them.

It would be great progress, if we decide that we could add both of these
features controlled by GUC (off by default).

If we decide so, then let's start working on this. At first, we should
construct list of machines and workloads for testing. Each list of machines
and workloads would be not comprehensive. But let's find something that
would be enough for testing of GUC controlled, off by default features.
Then we can turn our conversation from theoretical thoughts to particular
benchmarks which would be objective and convincing to everybody.

Otherwise, let's just add these features to the list of unwanted
functionality and close this question.

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

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

On Tue, Mar 15, 2016 at 3:46 PM, Valery Popov wrote:
> make installcheck-world failed on several contrib modules:
> dblink, file_fdw, hstore, pgcrypto, pgstattuple, postgres_fdw, tablefunc.
> The tests results are attached.
> Documentation looks good.
> Where may be a problem with make check-world and make installcheck-world
> results?

I cannot reproduce this, and my guess is that the binaries of those
contrib/ modules are not up to date for the installed instance of
Postgres you are running the tests on. Particularly I find this
portion doubtful:

  SELECT avg(normal_rand)::int FROM normal_rand(100, 250, 0.2);
! server closed the connection unexpectedly
! This probably means the server terminated abnormally
! before or while processing the request.
! connection to server was lost

The set of patches I am proposing here does not go through those code
paths, and this is likely an aggregate failure.
-- 
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] More stable query plans via more predictable column statistics

2016-03-15 Thread Shulgin, Oleksandr

On Wed, Mar 9, 2016 at 5:28 PM, Tom Lane  wrote:

> "Shulgin, Oleksandr"  writes:
> > Yes, I now recall that my actual concern was that sample_cnt may
> calculate
> > to 0 due to the latest condition above, but that also implies track_cnt
> ==
> > 0, and then we have a for loop there which will not run at all due to
> this,
> > so I figured we can avoid calculating avgcount and running the loop
> > altogether with that check.  I'm not opposed to changing the condition if
> > that makes the code easier to understand (or dropping it altogether if
> > calculating 0/0 is believed to be harmless anyway).
>
> Avoiding intentional zero divides is good.  It might happen to work
> conveniently on your machine, but I wouldn't think it's portable.
>

Tom,

Thank you for volunteering to review this patch!

Are you waiting on me to produce an updated version with more comments
about NULL-handling in the distinct estimator, or do you have something
cooking already?

--
Regards,
Alex

Re: [HACKERS] Add schema-qualified relnames in constraint error messages.

2016-03-15 Thread Shulgin, Oleksandr

On Mon, Mar 14, 2016 at 7:55 PM, Tom Lane  wrote:

> "Shulgin, Oleksandr"  writes:
> > What I dislike about this POC is all the disruption in libpq, to be
> > honest.
>
> Yeah, I don't much like that either.  But I don't think we can avoid
> some refactoring there; as designed, conversion of an error message into
> user-visible form is too tightly tied to receipt of the message.
>

True.  Attached is a v2 which addresses all of the points raised earlier I
believe.

We still extract the message building part of code from
pqGetErrorNotice3(), but the user-facing change is much more sane now: just
added a new function PQresultVerboseErrorMessage().

> It would be much neater if we could form the verbose message every
> > time and let the client decide where to cut it.  Maybe a bit "too clever"
> > would be to put a \0 char between short message and it's verbose
> > continuation.  The client could then reach the verbose part like this
> > (assuming that libpq did put a verbose part there): msg + strlen(msg) +
> 1.
>
> Blech :-(
>

:-)  That would not work either way, I've just noticed that SQLLEVEL is put
at the start of the message in verbose mode, but not by default.

Thinking about it, though, it seems to me that we could get away with
> always performing both styles of conversion and sticking both strings
> into the PGresult.  That would eat a little more space but not much.
> Then we just need to add API to let the application get at both forms.
>

This is what the v2 basically implements, now complete with help,
tab-complete and documentation changes.  I don't think we can add a usual
simple regression test here reliably, due to LOCATION field which might be
subject to unexpected line number changes.  But do we really need one?

--
Regards,
Alex
From d8d6a65da57d8e14eac4f614d31b19d52f8176d9 Mon Sep 17 00:00:00 2001
From: Oleksandr Shulgin 
Date: Wed, 6 Jan 2016 14:58:17 +0100
Subject: [PATCH] Add \errverbose psql command and support in libpq

For every error we build both the default and verbose error message,
then store both in PGresult.  The client can then retrieve the verbose
message using the new exported function PGresultVerboseErrorMessage().

In psql we store the verbose error message in pset (if any) for display
when requested by the user with \errverbose.
---
 doc/src/sgml/libpq.sgml |  40 -
 src/bin/psql/command.c  |   9 ++
 src/bin/psql/common.c   |   5 ++
 src/bin/psql/help.c |   1 +
 src/bin/psql/settings.h |   2 +
 src/bin/psql/startup.c  |   1 +
 src/bin/psql/tab-complete.c |   2 +-
 src/interfaces/libpq/exports.txt|   1 +
 src/interfaces/libpq/fe-exec.c  |  29 ++-
 src/interfaces/libpq/fe-protocol3.c | 166 +---
 src/interfaces/libpq/libpq-fe.h |   1 +
 src/interfaces/libpq/libpq-int.h|   3 +-
 12 files changed, 181 insertions(+), 79 deletions(-)

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 2328d8f..8b9544f 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -2691,6 +2691,38 @@ char *PQresultErrorMessage(const PGresult *res);
   
  
 
+ 
+  
+   PQresultVerboseErrorMessage
+   
+PQresultVerboseErrorMessage
+   
+  
+
+  
+   
+Returns the most verbose error message associated with the
+command, or an empty string if there was no error.
+
+char *PQresultVerboseErrorMessage(const PGresult *res);
+
+If there was an error, the returned string will include a trailing
+newline.  The caller should not free the result directly. It will
+be freed when the associated PGresult handle is
+passed to PQclear.
+   
+
+   
+ If error verbosity is already set to the maximum available level,
+ this will return exactly the same error message
+ as PQresultErrorMessage. Otherwise it can be
+ useful to retrieve a more detailed error message without running
+ the failed query again (while increasing the error verbosity
+ level).
+   
+  
+ 
+
  
   PQresultErrorFieldPQresultErrorField
   
@@ -5579,9 +5611,11 @@ PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity);
   this will normally fit on a single line.  The default mode produces
   messages that include the above plus any detail, hint, or context
   fields (these might span multiple lines).  The VERBOSE
-  mode includes all available fields.  Changing the verbosity does not
-  affect the messages available from already-existing
-  PGresult objects, only subsequently-created ones.
+  mode includes all available fields.  It is still possible to
+  retrieve the most verbose error message from an
+  existing PGresult object (without changing the
+  current verbosity

Re: [HACKERS] [PATCH] Integer overflow in timestamp[tz]_part() and date/time boundaries check

2016-03-15 Thread Mark Dilger


> On Mar 15, 2016, at 8:35 AM, Mark Dilger  wrote:
> 
> 
>> On Mar 14, 2016, at 5:12 PM, Vitaly Burovoy  wrote:
>> 
>> On 3/14/16, Mark Dilger  wrote:
>>> The first thing I notice about this patch is that
>>> src/include/datatype/timestamp.h
>>> has some #defines that are brittle.  The #defines have comments explaining
>>> their logic, but I'd rather embed that in the #define directly:
>>> 
>>> This:
>>> 
>>> +#ifdef HAVE_INT64_TIMESTAMP
>>> +#define MIN_TIMESTAMP  INT64CONST(-2118134880)
>>> +/* == (0 - POSTGRES_EPOCH_JDATE) * 86400 * USECS_PER_SEC */
>>> +#define MAX_TIMESTAMP  INT64CONST(92233713312)
>>> +/* == (JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 86400 * USECS_PER_SEC
>>> */
>>> +#else
>>> +#define MIN_TIMESTAMP  -211813488000.0
>>> +/* == (0 - POSTGRES_EPOCH_JDATE) * 86400 */
>>> +#define MAX_TIMESTAMP  9223371331200.0
>>> +/* == (JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 86400 */
>>> +#endif
>>> 
>>> Could be written as:
>>> 
>>> #ifdef HAVE_INT64_TIMESTAMP
>>> #define MIN_TIMESTAMP
>>> ((INT64CONST(0) - POSTGRES_EPOCH_JDATE) * USECS_PER_DAY)
>>> #define MAX_TIMESTAMP
>>> ((JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * USECS_PER_DAY)
>>> #else
>>> #define MIN_TIMESTAMP
>>> ((INT64CONST(0) - POSTGRES_EPOCH_JDATE) * SECS_PER_DAY)
>>> #define MAX_TIMESTAMP
>>> ((JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * SECS_PER_DAY)
>>> #endif
>>> 
>>> I assume modern compilers would convert these to the same constants at
>>> compile-time,
>> 
>> Firstly, Postgres is compiling not only by modern compilers.
> 
> Do you have an example of a compiler that will not do this constant folding
> at compile time?
> 
>>> rather than impose a runtime penalty.
>> 
>> Secondary, It is hard to write it correctly obeying Postgres' coding
>> standard (e.g. 80-columns border) and making it clear: it is not so
>> visual difference between USECS_PER_DAY and SECS_PER_DAY in the
>> expressions above (but it is vivid in comments in the file).
> 
> Hmm.  I think using USECS_PER_DAY is perfectly clear, but that is a personal
> opinion.  I don't see any way to argue if you don't see it that way.
> 
>> Also a
>> questions raise "Why is INT64CONST(0) necessary in `#else' block"
>> (whereas `float' is necessary there)
> 
> You appear to be correct.  The second half should be defined in terms of
> float.  I compiled on a system with int64 support, so I didn't notice. Thanks
> for catching that.
> 
>> or "Why is INT64CONST set for
>> MIN_TIMESTAMP, but not for MAX_TIMESTAMP?" (JULIAN_MAX4STAMPS is _not_
>> int64).
> 
> I was only casting the zero to int64.  That doesn't seem necessary, so it can
> be removed.  Both MIN_TIMESTAMP and MAX_TIMESTAMP were defined
> in terms of USECS_PER_DAY, which itself is defined in terms of INT64CONST,
> so I believe they both work out to be an int64 constant.
> 
>> The file is full of constants. For example, JULIAN_MAX and
>> SECS_PER_DAY are one of them.
> 
> JULIAN_MAXYEAR and JULIAN_MAXYEAR4STAMPS appear to be magic
> numbers with no explanation.  I would not point to them as examples to be
> followed.
> 
>> I would leave it as is.
>> 
>>> The #defines would be less brittle in
>>> the event, for example, that the postgres epoch were ever changed.
>> 
>> I don't think it is real, and even in such case all constants are
>> collected together in the file and will be found and changed at once.
> 
> I agree that they would be found at once.  I disagree that the example
> is not real, as I have changed the postgres epoch myself in some builds,
> to be able to use int32 timestamps on small devices.
> 
> Regards,
> Mark Dilger

I forgot to mention that I am not trying to block this commit.  These were just
my observations about the patch, for your consideration.



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

[HACKERS] PostgreSQL 9.6 Feature Freeze - April 8, 2016

At the recent PostgreSQL developer meeting in Brussels, a consensus
was reached that an early beta, leading to an on-time release, would
be very desirable.  In particular, it was suggested that we should
attempt to release PostgreSQL 9.6beta1 in May.  The release management
team has determined that this will require a feature freeze in early
April.  Accordingly, the release management has decided that all
feature patches destined for PostgreSQL 9.6 must be committed no later
than April 8, 2016.  Any patch not committed prior to 2016-04-09
00:00:00 GMT may not be committed to PostgreSQL 9.6 unless (a) it is a
bug fix, (b) it represents essential cleanup of a previously-committed
patch, or (c) the release management team has approved an extension to
the deadline for that particular patch.

As of the time when feature freeze goes into effect, all patches
remaining in the CommitFest will be either moved to the next
CommitFest, if they are as of that time in a state of Needs Review or
Ready for Committer; or marked as Returned with Feedback, if they are
in a state of Waiting on Author.  The release management team
encourages patch authors and reviewers, the CommitFest manager (David
Steele), and all other community members to keep the status of each
patch in the CommitFest accurate throughout the CommitFest, and
particularly as the feature freeze deadline approaches.

The release management team anticipates approving exceptions to the
feature freeze deadline only if the following criteria are met: (1)
the extension is requested by a committer who actually intends to
commit the patch before the extension lapses; (2) the release
management team believes that the patch has sufficient consensus to
proceed with it; (3) the release management team believes that
accepting the patch will not destabilize the tree or otherwise
compromise the PostgreSQL community's ability to get a beta out the
door on schedule; and (4) the proposed extension does not extend
beyond April 15, 2016.

--
Robert Haas
PostgreSQL 9.6 Release Management Team


-- 
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] Weighted Stats

2016-03-15 Thread David Fetter

On Fri, Jan 08, 2016 at 04:37:36PM +1100, Haribabu Kommi wrote:
> On Mon, Dec 21, 2015 at 1:50 PM, David Fetter  wrote:
> > On Sun, Dec 20, 2015 at 06:13:33PM -0600, Jim Nasby wrote:
> >> On 11/2/15 5:46 PM, David Fetter wrote:
> >> >I'd like to add weighted statistics to PostgreSQL
> >>
> >> Anything happen with this? If community isn't interested, ISTM it'd be good
> >> to put this in PGXN.
> >
> > I think it's already in PGXN as an extension, and I'll get another
> > version out this early this week, as it involves mostly adding some
> > tests.
> >
> > I'll do the float8 ones for core this week, too, and unless there's a
> > really great reason to do more data types on the first pass, it should
> > be in committable shape.
> 
> I reviewed the patch, following are my observations.
> 
> 1. +   precision, numeric, or interval
> 
> with interval type it is giving problem. As interval data type is not 
> supported,
> so remove it in the list of supported inputs.

Done.

> 
> 2. +float8_weighted_avg(PG_FUNCTION_ARGS)
> 
> It will be helpful, if you provide some information as a function header,
> how the weighted average is calculated similar like other weighted functions.

Done.

> 3. + transvalues = check_float8_array(transarray,
> "float8_weighted_stddev_accum", 4);
> 
> The second parameter to check_float8_array should be "float8_weighted_accum".

Done.

> 4. There is an OID conflict of 4066 with latest master code.

Fixed.

> 5.+ A += newvalW * ( newvalX - transvalues[2] ) / W;
> + CHECKFLOATVAL(A, isinf(newvalW) || isinf(newvalX - transvalues[2])
> || isinf(1.0/W), true);
> 
> + Q += newvalW * (newvalX - transvalues[2]) * (newvalX - A);
> + CHECKFLOATVAL(A, isinf(newvalX -  transvalues[3]) || isinf(newvalX -
> A) || isinf(1.0/W), true);
> 
> 
> Is the need of calculation also needs to be passed to CHECKFLOATVAL?
> Just passing
> the variables involved in the calculation isn't enough? If expressions
> are required then
> it should be something as follows?
> 
> CHECKFLOATVAL(A, isinf(transvalues[2]) || isinf(newvalW) ||
> isinf(newvalX - transvalues[2]) || isinf(1.0/W), true);
> 
> CHECKFLOATVAL(Q, isinf(transvalues[3]) || isinf(newvalX -
> transvalues[2]) || isinf(newvalX - A) || isinf(1.0/W), true);

Done.

Please find attached a patch that uses the float8 version to cover the
numeric types.

Cheers,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david.fet...@gmail.com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 000489d..09ada7e 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -12645,6 +12645,29 @@ NULL baz(3 rows)
  
   

+weighted_average
+   
+   
+weighted_avg
+   
+   weighted_avg(value 
expression, weight 
expression)
+  
+  
+   smallint, int,
+   bigint, real, double
+   precision, numeric, or interval
+  
+  
+   numeric for any integer-type argument,
+   double precision for a floating-point argument,
+   otherwise the same as the argument data type
+  
+  the average (arithmetic mean) of all input values, weighted by 
the input weights
+ 
+
+ 
+  
+   
 bit_and

bit_and(expression)
@@ -13288,6 +13311,29 @@ SELECT xmlagg(x) FROM (SELECT x FROM test ORDER BY y 
DESC) AS tab;
  
   

+weighted standard deviation
+population
+   
+   
+weighted_stddev_pop
+   
+   weighted_stddev_pop(value 
expression, weight 
expression)
+  
+  
+   smallint, int,
+   bigint, real, double
+   precision, or numeric
+  
+  
+   double precision for floating-point arguments,
+   otherwise numeric
+  
+  weighted population standard deviation of the input values
+ 
+
+ 
+  
+   
 standard deviation
 sample

@@ -13311,6 +13357,29 @@ SELECT xmlagg(x) FROM (SELECT x FROM test ORDER BY y 
DESC) AS tab;
  
   

+weighted standard deviation
+sample
+   
+   
+weighted_stddev_samp
+   
+   weighted_stddev_samp(value 
expression, weight 
expression)
+  
+  
+   smallint, int,
+   bigint, real, double
+   precision, or numeric
+  
+  
+   double precision for floating-point arguments,
+   otherwise numeric
+  
+  weighted sample standard deviation of the input values
+ 
+
+ 
+  
+   
 variance

variance(expression)
diff --git a/src/backend/utils/adt/float.c b/src/backend/utils/adt/float.c
index d4e5d55..c03f712 100644
--- a/src/backend/utils/adt/float.c
+++ b/src/backend/utils/adt/float.c
@@ -2364,6 +2364,7 @@ setseed(PG_FUNCTION_ARGS)
  * float8_accum

Re: [HACKERS] [PATCH] Integer overflow in timestamp[tz]_part() and date/time boundaries check

2016-03-15 Thread Mark Dilger


> On Mar 14, 2016, at 5:12 PM, Vitaly Burovoy  wrote:
> 
> On 3/14/16, Mark Dilger  wrote:
>> The first thing I notice about this patch is that
>> src/include/datatype/timestamp.h
>> has some #defines that are brittle.  The #defines have comments explaining
>> their logic, but I'd rather embed that in the #define directly:
>> 
>> This:
>> 
>> +#ifdef HAVE_INT64_TIMESTAMP
>> +#define MIN_TIMESTAMP  INT64CONST(-2118134880)
>> +/* == (0 - POSTGRES_EPOCH_JDATE) * 86400 * USECS_PER_SEC */
>> +#define MAX_TIMESTAMP  INT64CONST(92233713312)
>> +/* == (JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 86400 * USECS_PER_SEC
>> */
>> +#else
>> +#define MIN_TIMESTAMP  -211813488000.0
>> +/* == (0 - POSTGRES_EPOCH_JDATE) * 86400 */
>> +#define MAX_TIMESTAMP  9223371331200.0
>> +/* == (JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * 86400 */
>> +#endif
>> 
>> Could be written as:
>> 
>> #ifdef HAVE_INT64_TIMESTAMP
>> #define MIN_TIMESTAMP
>> ((INT64CONST(0) - POSTGRES_EPOCH_JDATE) * USECS_PER_DAY)
>> #define MAX_TIMESTAMP
>> ((JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * USECS_PER_DAY)
>> #else
>> #define MIN_TIMESTAMP
>> ((INT64CONST(0) - POSTGRES_EPOCH_JDATE) * SECS_PER_DAY)
>> #define MAX_TIMESTAMP
>> ((JULIAN_MAX4STAMPS - POSTGRES_EPOCH_JDATE) * SECS_PER_DAY)
>> #endif
>> 
>> I assume modern compilers would convert these to the same constants at
>> compile-time,
> 
> Firstly, Postgres is compiling not only by modern compilers.

Do you have an example of a compiler that will not do this constant folding
at compile time?

>> rather than impose a runtime penalty.
> 
> Secondary, It is hard to write it correctly obeying Postgres' coding
> standard (e.g. 80-columns border) and making it clear: it is not so
> visual difference between USECS_PER_DAY and SECS_PER_DAY in the
> expressions above (but it is vivid in comments in the file).

Hmm.  I think using USECS_PER_DAY is perfectly clear, but that is a personal
opinion.  I don't see any way to argue if you don't see it that way.

> Also a
> questions raise "Why is INT64CONST(0) necessary in `#else' block"
> (whereas `float' is necessary there)

You appear to be correct.  The second half should be defined in terms of
float.  I compiled on a system with int64 support, so I didn't notice. Thanks
for catching that.

> or "Why is INT64CONST set for
> MIN_TIMESTAMP, but not for MAX_TIMESTAMP?" (JULIAN_MAX4STAMPS is _not_
> int64).

I was only casting the zero to int64.  That doesn't seem necessary, so it can
be removed.  Both MIN_TIMESTAMP and MAX_TIMESTAMP were defined
in terms of USECS_PER_DAY, which itself is defined in terms of INT64CONST,
so I believe they both work out to be an int64 constant.

> The file is full of constants. For example, JULIAN_MAX and
> SECS_PER_DAY are one of them.

JULIAN_MAXYEAR and JULIAN_MAXYEAR4STAMPS appear to be magic
numbers with no explanation.  I would not point to them as examples to be
followed.

> I would leave it as is.
> 
>> The #defines would be less brittle in
>> the event, for example, that the postgres epoch were ever changed.
> 
> I don't think it is real, and even in such case all constants are
> collected together in the file and will be found and changed at once.

I agree that they would be found at once.  I disagree that the example
is not real, as I have changed the postgres epoch myself in some builds,
to be able to use int32 timestamps on small devices.

Regards,
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] Minor bug affecting ON CONFLICT lock wait log messages

2016-03-15 Thread Stephen Frost

* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote:
> Stephen Frost wrote:
> > * Julien Rouhaud (julien.rouh...@dalibo.com) wrote:
> 
> > XLTW_InsertIndexUnique is used when building a unique index, but this is
> > just a check, and more to the point, it's actually a re-check of what
> > we're doing in nbinsert.c where we're already using XLTW_InsertIndex.
> > 
> > We wouldn't want to end up returning different error messages for the
> > same command under the same conditions just based, which is what we'd
> > potentially end up doing if we used XLTW_InsertIndexUnique here.
> 
> Perhaps we need a new value in that enum, so that the context message is
> specific to the situation at hand?

Maybe, but that would require a new message and new translation and just
generally doesn't seem like something we'd want to back-patch for a
bugfix.  As such, if someone's interested, they could certainly propose
it to go into HEAD, but I'm not going to look at making those kinds of
changes for this bugfix.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages

Stephen Frost wrote:
> * Julien Rouhaud (julien.rouh...@dalibo.com) wrote:

> XLTW_InsertIndexUnique is used when building a unique index, but this is
> just a check, and more to the point, it's actually a re-check of what
> we're doing in nbinsert.c where we're already using XLTW_InsertIndex.
> 
> We wouldn't want to end up returning different error messages for the
> same command under the same conditions just based, which is what we'd
> potentially end up doing if we used XLTW_InsertIndexUnique here.

Perhaps we need a new value in that enum, so that the context message is
specific to the situation at hand?

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


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

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