Re: [HACKERS] brin_summarize_new_values error checking

[Fujii Masao]

On Wed, Jan 27, 2016 at 11:56 AM, Fujii Masao  wrote:
> On Mon, Jan 25, 2016 at 4:03 PM, Jeff Janes  wrote:
>> In reviewing one of my patches[1], Fujii-san has pointed out that I
>> didn't include checks for being in recovery, or for working on another
>> backend's temporary index.
>>
>> I think that brin_summarize_new_values in 9.5.0 commits those same
>> sins. In its case, I don't think those are critical, as they just
>> result in getting less specific error messages that one might hope
>> for, rather than something worse like a panic.
>>
>> But still, we might want to address them.
>
> Agreed to add those checks.

Attached patch does this.

Regards,

-- 
Fujii Masao
*** a/src/backend/access/brin/brin.c
--- b/src/backend/access/brin/brin.c
***
*** 795,800  brin_summarize_new_values(PG_FUNCTION_ARGS)
--- 795,806 
  	Relation	heapRel;
  	double		numSummarized = 0;
  
+ 	if (RecoveryInProgress())
+ 		ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+  errmsg("recovery is in progress"),
+  errhint("BRIN page ranges cannot be summarized during recovery.")));
+ 
  	/*
  	 * We must lock table before index to avoid deadlocks.  However, if the
  	 * passed indexoid isn't an index then IndexGetRelation() will fail.
***
*** 817,822  brin_summarize_new_values(PG_FUNCTION_ARGS)
--- 823,838 
   errmsg("\"%s\" is not a BRIN index",
  		RelationGetRelationName(indexRel;
  
+ 	/*
+ 	 * Reject attempts to read non-local temporary relations; we would be
+ 	 * likely to get wrong data since we have no visibility into the owning
+ 	 * session's local buffers.
+ 	 */
+ 	if (RELATION_IS_OTHER_TEMP(indexRel))
+ 		ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ 			   errmsg("cannot access temporary indexes of other sessions")));
+ 
  	/* User must own the index (comparable to privileges needed for VACUUM) */
  	if (!pg_class_ownercheck(indexoid, GetUserId()))
  		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,

-- 
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: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Amit Kapila]

On Tue, Apr 19, 2016 at 8:44 PM, Robert Haas  wrote:
>
> On Tue, Apr 19, 2016 at 11:11 AM, Kevin Grittner 
wrote:
> > On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila 
wrote:
>
> >> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
> >> takes EXCLUSIVE LWLock which seems to be a probable reason for a
performance
> >> regression.  Now, here the question is do we need to acquire that lock
if
> >> xmin is not changed since the last time value of
> >> oldSnapshotControl->latest_xmin is updated or xmin is lesser than
equal to
> >> oldSnapshotControl->latest_xmin?
> >> If we don't need it for above cases, I think it can address the
performance
> >> regression to a good degree for read-only workloads when the feature is
> >> enabled.
> >
> > Thanks, Amit -- I think something along those lines is the right
> > solution to the scaling issues when the feature is enabled.  For
> > now I'm focusing on the back-patching issues and the performance
> > regression when the feature is disabled, but I'll shift focus to
> > this once the "killer" issues are in hand.
>
> Maybe Amit could try his idea in parallel.
>

Okay, will look into it.


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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Fujii Masao]

On Sun, Apr 17, 2016 at 1:22 AM, Magnus Hagander  wrote:
> On Wed, Apr 13, 2016 at 4:07 AM, Noah Misch  wrote:
>>
>> On Tue, Apr 12, 2016 at 10:08:23PM +0200, Magnus Hagander wrote:
>> > On Tue, Apr 12, 2016 at 8:39 AM, Noah Misch  wrote:
>> > > On Mon, Apr 11, 2016 at 11:22:27AM +0200, Magnus Hagander wrote:
>> > > > Well, if we *don't* do the rewrite before we release it, then we
>> > > > have to
>> > > > instead put information about the new version of the functions into
>> > > > the
>> > > old
>> > > > structure I think.
>> > > >
>> > > > So I think it's an open issue.
>> > >
>> > > Works for me...
>> > >
>> > > [This is a generic notification.]
>> > >
>> > > The above-described topic is currently a PostgreSQL 9.6 open item.
>> > > Magnus,
>> > > since you committed the patch believed to have created it, you own
>> > > this
>> > > open
>> > > item.  If that responsibility lies elsewhere, please let us know whose
>> > > responsibility it is to fix this.  Since new open items may be
>> > > discovered
>> > > at
>> > > any time and I want to plan to have them all fixed well in advance of
>> > > the
>> > > ship
>> > > date, I will appreciate your efforts toward speedy resolution.  Please
>> > > present, within 72 hours, a plan to fix the defect within seven days
>> > > of
>> > > this
>> > > message.  Thanks.
>> > >
>> >
>> > I won't have time to do the bigger rewrite/reordeirng by then, but I can
>> > certainly commit to having the smaller updates done to cover the new
>> > functionality in less than a week. If nothing else, that'll be something
>> > for me to do on the flight over to pgconf.us.
>>
>> Thanks for that plan; it sounds good.
>
>
> Here's a suggested patch.
>
> There is some duplication between the non-exclusive and exclusive backup
> sections, but I wanted to make sure that each set of instructions can just
> be followed top-to-bottom.
>
> I've also removed some tips that aren't really necessary as part of the
> step-by-step instructions in order to keep things from exploding in size.
>
> Finally, I've changed references to "backup dump" to just be "backup",
> because it's confusing to call them something with dumps in when it's not
> pg_dump. Enough that I got partially confused myself while editing...
>
> Comments?

+Low level base backups can be made in a non-exclusive or an exclusive
+way. The non-exclusive method is recommended and the exclusive one will
+at some point be deprecated and removed.

I don't object to add a non-exclusive mode of low level backup,
but I disagree to mark an exclusive backup as deprecated at least
until we can alleviate some pains that a non-exclusive mode causes.

One example of the pain, in a non-exclusive backup, we need to keep
the IDLE connection which was used to execute pg_start_backup(),
until the end of backup. Of course a backup can take a very
long time. In this case the IDLE connection also needs to remain
for such a long time. If it's accidentally terminated (e.g., because
of IDLE connection), the backup fails and needs to be taken again
from the beginning.

Another pain in a non-exclusive backup is to have to execute both
pg_start_backup() and pg_stop_backup() on the same connection.
Please imagine the case where psql is used to execute those two
backup functions (I believe that there are many users who do this).
For example,

psql -c "SELECT pg_start_backup()"
rsync, cp, tar, storage backup, or something
psql -c "SELECT pg_stop_backup()"

A non-exclusive backup breaks the above very simple steps because
two backup functions are executed on different connections.
So, how should we modify the steps for a non-exclusive backup?
Basically we need to pause psql after pg_start_backup(), signal it
to resume after the copy of database cluster is taken, and make
it execute pg_stop_backup(). I'm afraid that the backup script
will be complicated because of this pain of non-exclusive backup.

+ The pg_stop_backup will return one row with three
+ values. The second of these fields should be written to a file named
+ backup_label in the root directory of the backup. The
+ third field should be written to a file named
+ tablespace_map unless the field is empty.

How should we write those two values to different files when
we execute pg_stop_backup() via psql? Whole output of
pg_stop_backup() should be written to a transient file and
it should be filtered and written to different two files by
using some Linux commands? This also seems to make the backup
script more complicated.

Regards,

-- 
Fujii Masao


-- 
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] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Wed, Apr 20, 2016 at 1:39 PM, Noah Misch  wrote:
> On Tue, Apr 19, 2016 at 02:42:24AM -0300, Alvaro Herrera wrote:
>> Michael Paquier wrote:
>> > On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
>> >  wrote:
>> > > Now, I have produced two patches:
>> > > - 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
>> > > __crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
>> > > hack, though I am coming to think that this may be the approach that
>> > > would us the less harm, and that's closer to what is done for VS 2012
>> > > and 2013.
>> > > - 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
>> > > GetLocaleInfoEx, this requires us to lower a bit the support grid for
>> > > Windows, basically that's throwing support for XP if compilation is
>> > > done with VS 2015.
>> > > Based on my tests, both are working with short and long local names,
>> > > testing via initdb --locale.
>> >
>> > The first patch is actually not what I wanted to send. Here are the
>> > correct ones...
>>
>> This thread seems to have stalled.  I thought we were going to consider
>> these patches for 9.6.
>
> Committers have given this thread's patches a generous level of consideration.
> At this point, if $you wouldn't back-patch them to at least 9.5, they don't
> belong in 9.6.  However, a back-patch to 9.3 does seem fair, assuming the
> final patch looks anything like the current proposals.

Er, the change is rather located and fully controlled by _MSC_VER >=
1900, so this represents no risk for existing compilations on Windows,
don't you agree? With HEAD, code compilation would just fail btw
knowing how locales are broken, so it would just not work. That said,
support for new VS versions have been backpatched to the last stable
version at least, now that would be 9.5. There is indeed no written
policy stating what to do precisely in this case, but it seems to me
that there is no point in being overly protective as well..

>> Should we simply push them to see what the
>> buildfarm thinks?
>
> No.  The thread has been getting suitable test reports for a few weeks now.
> If it were not, better to make the enhancement wait as long as necessary than
> to use the buildfarm that way.  Buildfarm results wouldn't even be pertinent;
> they would merely tell us whether the patch broke non-VS 2015 compilers.

Well, they could push them, the results won't really matter and
existing machines won't be impacted, as no buildfarm machine is using
_MSC_VER >= 1900 as of now. Petr has one ready though as mentioned
upthread.
-- 
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] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Michael Paquier wrote:


On Wed, Apr 20, 2016 at 1:48 AM, Christian Ullrich  wrote:

Both patches behave exactly the same in this test. Of the 102 remaining
locales, I get an unexpected codepage for just four:

- kk: Expected 1251, used 1252
- kk-KZ: Expected 1251, used 1252
- sr: Expected 1251, used 1250
- uk: Expected 1251, used 1252

I suspect that "sr" is a bug in MSDN: 1250 is Eastern European (Latin), 1251
is Cyrillic, and "sr" alone is listed as Latin. So either the script or the
codepage are likely wrong.


Does VS2013 or older behave the same way for those locales? The patch
using __crt_locale_data_public on VS2015 should work more or less
similarly to VS2012~2013.


Identical results for unmodified master built with 2013.

--
Christian




--
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] VS 2015 support in src/tools/msvc

[Noah Misch]

On Tue, Apr 19, 2016 at 02:42:24AM -0300, Alvaro Herrera wrote:
> Michael Paquier wrote:
> > On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
> >  wrote:
> > > Now, I have produced two patches:
> > > - 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
> > > __crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
> > > hack, though I am coming to think that this may be the approach that
> > > would us the less harm, and that's closer to what is done for VS 2012
> > > and 2013.
> > > - 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
> > > GetLocaleInfoEx, this requires us to lower a bit the support grid for
> > > Windows, basically that's throwing support for XP if compilation is
> > > done with VS 2015.
> > > Based on my tests, both are working with short and long local names,
> > > testing via initdb --locale.
> > 
> > The first patch is actually not what I wanted to send. Here are the
> > correct ones...
> 
> This thread seems to have stalled.  I thought we were going to consider
> these patches for 9.6.

Committers have given this thread's patches a generous level of consideration.
At this point, if $you wouldn't back-patch them to at least 9.5, they don't
belong in 9.6.  However, a back-patch to 9.3 does seem fair, assuming the
final patch looks anything like the current proposals.

> Should we simply push them to see what the
> buildfarm thinks?

No.  The thread has been getting suitable test reports for a few weeks now.
If it were not, better to make the enhancement wait as long as necessary than
to use the buildfarm that way.  Buildfarm results wouldn't even be pertinent;
they would merely tell us whether the patch broke non-VS 2015 compilers.

nm


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

[Masahiko Sawada]

On Mon, Apr 18, 2016 at 2:15 PM, Kyotaro HORIGUCHI
 wrote:
> At Fri, 15 Apr 2016 17:36:57 +0900, Masahiko Sawada  
> wrote in 
>> >> How about if we do all the parsing stuff in temporary context and then 
>> >> copy
>> >> the results using TopMemoryContext?  I don't think it will be a leak in
>> >> TopMemoryContext, because next time we try to check/assign s_s_names, it
>> >> will free the previous result.
>> >
>> > I agree with you. A temporary context for the parser seems
>> > reasonable. TopMemoryContext is created very early in main() so
>> > palloc on it is effectively the same with malloc.
>> > One problem is that only the top memory block is assumed to be
>> > free()'d, not pfree()'d by guc_set_extra. It makes this quite
>> > ugly..
>> >
>> > Maybe we shouldn't use the extra for this purpose.
>> >
>> > Thoughts?
>> >
>>
>> How about if check_hook just parses parameter in
>> CurrentMemoryContext(i.g., T_AllocSetContext), and then the
>> assign_hook copies syncrep_parse_result to TopMemoryContext.
>> Because syncrep_parse_result is a global variable, these hooks can see it.
>
> Hmm. Somewhat uneasy but should work. The attached patch does it.
>
>> Here are some comments.
>>
>> -SyncRepUpdateConfig(void)
>> +SyncRepFreeConfig(SyncRepConfigData *config, bool itself, MemoryContext cxt)
>>
>> Sorry, it's my bad. itself variables is no longer needed because
>> SyncRepFreeConfig is called by only one function.
>>
>> -void
>> -SyncRepFreeConfig(SyncRepConfigData *config)
>> +SyncRepConfigData *
>> +SyncRepCopyConfig(SyncRepConfigData *oldconfig, MemoryContext targetcxt)
>>
>> I'm not sure targetcxt argument is necessary.
>
> Yes, these are just for signalling so removal doesn't harm.
>

Thank you for updating the patch.

Here are some comments.

+   Assert(syncrep_parse_result == NULL);
+

Why do we need Assert at this point?
It's possible that syncrep_parse_result is not NULL after setting
s_s_names by ALTER SYSTEM.

+   /*
+* this memory block will be freed as a part of the
memory contxt for
+* config file processing.
+*/

s/contxt/context/

Regards,

--
Masahiko Sawada


-- 
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: Remove regress-python3-mangle.mk

[Noah Misch]

On Tue, Apr 19, 2016 at 06:31:34PM +0300, Yury Zhuravlev wrote:
> Now we generate tests for plpython3 of plpython2 tests. I think we should
> write independently 2 test suite.
> Why is that bad:
> 1. Differences between python2 and python3 more than can be solved by
> regexps. And these differences are growing.

Considering we have 2756 lines of Python test SQL and just thirteen lines of
sed to mangle them, the current method is scaling nicely.

> 2. We must be careful to write tests, so as not to break the converter. And
> we can not verify the possibility python3.

I may not understand that second sentence.  We have multiple buildfarm members
verifying the python2 case and multiple members verifying the python3 case.

> 3. Because we use sed we do not tests for plpython3 under Windows. And I
> have trouble with CMake too.

Even if removing regress-python3-mangle.mk happened to be good for PL/Python,
we need build systems flexible enough to implement steps like it without a
struggle.  Our current build systems, the src/tools/msvc system and the GNU
make system, have that flexibility.  We could port regress-python3-mangle.mk
to Perl and run it on Windows; it just hasn't been a priority.

> Yes, we will have 2 times more similar code but you need to relates to the
> python2 and python3 as the different languages.

The PL/Python implementation does not view them as different languages; the
bulk of its code is the same for both python2 and python3.  We'd want almost
the same test cases in both suites.  Maintaining a single, adaptable test
suite is cheaper than forking that suite.

nm


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

Re: [HACKERS] pg_dump dump catalog ACLs

[Noah Misch]

On Sun, Apr 17, 2016 at 11:02:28PM -0400, Noah Misch wrote:
> On Tue, Apr 05, 2016 at 05:50:18PM -0400, Stephen Frost wrote:
> > I'll be doing more testing, review and clean-up (there are some
> > whitespace only changes in the later patches that really should be
> > included in the earlier patches where the code was actually changed)
> > tonight with plans to push this tomorrow night.

> (2) pg_dump queries:
> 
> > @@ -14187,18 +14869,65 @@ dumpTable(Archive *fout, TableInfo *tbinfo)
> 
> > + "FROM 
> > pg_catalog.pg_attribute at "
> > +  "JOIN pg_catalog.pg_class c ON 
> > (at.attrelid = c.oid) "
> > + "LEFT JOIN 
> > pg_init_privs pip ON "
> 
> Since pg_attribute and pg_class require schema qualification here, so does
> pg_init_privs.  Likewise elsewhere in the patch's pg_dump changes.
> 
> 
> (3) pg_dumpall became much slower around the time of these commits.  On one
> machine (POWER7 3.55 GHz), a pg_dumpall just after initdb slowed from 0.25s at
> commit 6c268df^ to 4.0s at commit 7a54270.  On a slower machine (Opteron
> 1210), pg_dumpall now takes 19s against such a fresh cluster.

[This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Stephen,
since you committed the patch believed to have created it, you own this open
item.  If that responsibility lies elsewhere, please let us know whose
responsibility it is to fix this.  Since new open items may be discovered at
any time and I want to plan to have them all fixed well in advance of the ship
date, I will appreciate your efforts toward speedy resolution.  Please
present, within 72 hours, a plan to fix the defect within seven days of this
message.  Thanks.


-- 
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] Timeline following for logical slots

[Noah Misch]

On Sun, Apr 17, 2016 at 10:01:36PM +0800, Craig Ringer wrote:
> I made an unfortunate oversight in the logical decoding timeline following
> code: it doesn't work for logical decoding from the walsender, because I
> left the glue code required out of the final cut of the patch.

> Subject: [PATCH] Enable logical timeline following in the walsender
> 
> ---
>  src/backend/access/transam/xlogutils.c |  7 +++
>  src/backend/replication/walsender.c| 11 +++
>  src/include/access/xlogreader.h|  3 +++
>  src/include/access/xlogutils.h |  2 ++
>  4 files changed, 15 insertions(+), 8 deletions(-)

[This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Alvaro,
since you committed the patch believed to have created it, you own this open
item.  If that responsibility lies elsewhere, please let us know whose
responsibility it is to fix this.  Since new open items may be discovered at
any time and I want to plan to have them all fixed well in advance of the ship
date, I will appreciate your efforts toward speedy resolution.  Please
present, within 72 hours, a plan to fix the defect within seven days of this
message.  Thanks.


-- 
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] EXPLAIN VERBOSE with parallel Aggregate

[Noah Misch]

On Sun, Apr 17, 2016 at 10:22:24AM -0400, Tom Lane wrote:
> David Rowley  writes:
> > On 16 April 2016 at 04:27, Tom Lane  wrote:
> >> +1 for the latter, if we can do it conveniently.  I think exposing
> >> the names of the aggregate implementation functions would be very
> >> user-unfriendly, as nobody but us hackers knows what those are.
> 
> > It does not really seem all that convenient to do this. It also seems
> > a bit strange to me to have a parent node report a column which does
> > not exist in any nodes descending from it. Remember that the combine
> > Aggref does not have the same ->args as its corresponding partial
> > Aggref. It's not all that clear to me if there is any nice way to do
> > have this work the way you'd like. If we were to just call
> > get_rule_expr() on the first arg of the combine aggregate node, it
> > would re-print the PARTIAL keyword again.
> 
> This suggests to me that the parsetree representation for partial
> aggregates was badly chosen.  If EXPLAIN can't recognize them, then
> neither can anything else, and we may soon find needs for telling
> the difference that are not just cosmetic.  Maybe we need more
> fields in Aggref.

[This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If that responsibility lies elsewhere, please let us know whose
responsibility it is to fix this.  Since new open items may be discovered at
any time and I want to plan to have them all fixed well in advance of the ship
date, I will appreciate your efforts toward speedy resolution.  Please
present, within 72 hours, a plan to fix the defect within seven days of this
message.  Thanks.


-- 
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] Memory leak in GIN index build

[Marc Cousin]

I had the possibility to perform tests on 9.5, and can confirm the
memory leak I was seeing is solved with the patch (and that's great :) )

Regards

Marc



On 18/04/2016 17:53, Julien Rouhaud wrote:
> On 18/04/2016 16:33, Tom Lane wrote:
>> I poked at this over the weekend, and got more unhappy the more I poked.
>> Aside from the memory leakage issue, there are multiple coding-rule
>> violations besides the one you noted about scope of the critical sections.
>> One example is that in the page-split path for an inner page, we modify
>> the contents of childbuf long before getting into the critical section.
>> The number of out-of-date comments was depressingly large as well.
>>
>> I ended up deciding that the most reasonable fix was along the lines of
>> my first alternative above.  In the attached draft patches, the
>> "placeToPage" method is split into two methods, "beginPlaceToPage" and
>> "execPlaceToPage", where the second method is what's supposed to happen
>> inside the critical section for a non-page-splitting update.  Nothing
>> that's done inside beginPlaceToPage is critical.
>>
>> I've tested this to the extent of verifying that it passes make
>> check-world and stops the memory leak in your test case, but it could use
>> more eyeballs on it.
>>
> Thanks! I also started working on it but it was very far from being
> complete (and already much more ugly...).
>
> I couldn't find any problem in the patch.
>
> I wonder if asserting being in a critical section would be valuable in
> the *execPlaceToPage functions, or that (leaf->walinfolen > 0) in
> dataExecPlaceToPageLeaf(), since it's filled far from this function.
>
>> Attached are draft patches against HEAD and 9.5 (they're the same except
>> for BufferGetPage calls).  I think we'd also want to back-patch to 9.4,
>> but that will take considerable additional work because of the XLOG API
>> rewrite that happened in 9.5.  I also noted that some of the coding-rule
>> violations seem to be new in 9.5, so the problems may be less severe in
>> 9.4 --- the memory leak definitely exists there, though.
>>



-- 
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] Postgres 9.6 scariest patch tournament

[David Fetter]

On Tue, Apr 19, 2016 at 04:06:55PM -0400, Tom Lane wrote:
> Alvaro Herrera  writes:
> > Peter Geoghegan wrote:
> >> I would have appreciated more scope to say how confident I am in
> >> my prediction, and how scary in absolute terms I consider the
> >> scariest patches to be.
> 
> > It was purposefully ambiguous.  Maybe it should have been stated
> > explicitely.
> 
> I was thinking about complaining that "scariest" and "most bugs" are
> not the same thing.  Features you can turn off are not very scary,
> even if they're full of bugs (cough ... parallel query ... cough),
> because we could just ship 'em disabled by default until there's
> more reason to trust them.  What I find scary is patches that can
> break existing use-cases with no simple workaround.  I'm not sure
> which one to vote for yet.

There's space on the ballot for up to three, and it appears to be a
ranked choice or similar preference system.

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


-- 
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] Updated backup APIs for non-exclusive backups

[Noah Misch]

On Sat, Apr 16, 2016 at 06:22:47PM +0200, Magnus Hagander wrote:
> > On Tue, Apr 12, 2016 at 10:08:23PM +0200, Magnus Hagander wrote:
> > > I won't have time to do the bigger rewrite/reordeirng by then, but I can
> > > certainly commit to having the smaller updates done to cover the new
> > > functionality in less than a week.

> There is some duplication between the non-exclusive and exclusive backup
> sections, but I wanted to make sure that each set of instructions can just
> be followed top-to-bottom.
> 
> I've also removed some tips that aren't really necessary as part of the
> step-by-step instructions in order to keep things from exploding in size.
> 
> Finally, I've changed references to "backup dump" to just be "backup",
> because it's confusing to call them something with dumps in when it's not
> pg_dump. Enough that I got partially confused myself while editing...
> 
> Comments?

I scanned this briefly, and it looks reasonable.  I recommend committing it
forthwith.

> *** a/doc/src/sgml/backup.sgml
> --- b/doc/src/sgml/backup.sgml
> ***
> *** 818,823  test ! -f /mnt/server/archivedir/000100A90065 
> && cp pg_xlog/
> --- 818,838 
>   simple. It is very important that these steps are executed in
>   sequence, and that the success of a step is verified before
>   proceeding to the next step.
> +
> +
> + Low level base backups can be made in a non-exclusive or an exclusive
> + way. The non-exclusive method is recommended and the exclusive one will
> + at some point be deprecated and removed.

"I will deprecate X at some point" has the same effect as "I deprecate X now."
If you have no doubt you want to deprecate it, I advise plainer phrasing like,
"The exclusive method is deprecated and will eventually be removed."  That is
to say, just deprecate it right now.  If you have doubts, omit deprecation.


-- 
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] VS 2015 support in src/tools/msvc

[Michael Paquier]

On Wed, Apr 20, 2016 at 1:48 AM, Christian Ullrich  wrote:
> Both patches behave exactly the same in this test. Of the 102 remaining
> locales, I get an unexpected codepage for just four:
>
> - kk: Expected 1251, used 1252
> - kk-KZ: Expected 1251, used 1252
> - sr: Expected 1251, used 1250
> - uk: Expected 1251, used 1252
>
> I suspect that "sr" is a bug in MSDN: 1250 is Eastern European (Latin), 1251
> is Cyrillic, and "sr" alone is listed as Latin. So either the script or the
> codepage are likely wrong.

Does VS2013 or older behave the same way for those locales? The patch
using __crt_locale_data_public on VS2015 should work more or less
similarly to VS2012~2013.
-- 
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] Postgres 9.6 scariest patch tournament

[Peter Geoghegan]

On Tue, Apr 19, 2016 at 12:37 PM, Alvaro Herrera
 wrote:
> This guy reads my mind.  Where's my tinfoil hat?

Heh. Well, I'm generally not in favor of communicating concerns
without an obligation to defend them, but it could work well in tiny
doses. Offering hackers a low-risk way to take a position greatly
reduces the "knew-it-all-along" effect. We may then be more accurate
in assessing our own ability to anticipate problems.

There is very simple Malthusian logic [1] that explains why we'll
usually be wrong, which is:

Why are hackers bad at anticipating where bugs will be? Because if
they weren't, then there wouldn't be any bugs.

Please don't interpret my remarks as showing flippancy about bugs.
(The same should be said about the whole "scary patches" poll,
actually.)

>> I would have appreciated more scope to say how confident I am in my
>> prediction, and how scary in absolute terms I consider the scariest
>> patches to be.
>
> It was purposefully ambiguous.  Maybe it should have been stated
> explicitely.

I voted, and my vote probably just slightly reinforced the
conventional wisdom about where to look for problems -- it was not a
vote for parallel query, since I agree with Tom's assessment of the
risks there. I think you can probably guess what I voted for.

I wouldn't have expressed a similar sentiment on this list, because
that would probably turn out to be just jumping on the bandwagon.
There is a good chance that the patch will be totally fine in the end,
anyway. It was probably very carefully reviewed, precisely because it
touches critical parts of the system. And, it works in a way that
generalizes from an existing well-tested mechanism.

My vote represented "I certainly hope this patch has no bugs in it"
this time around. Next time, it might be "this patch almost certainly
has lots of undiscovered bugs", which might well be an original
insight for the release team if it's in my area of expertise (chances
are good that those bugs are not critically important if it gets to
that). Rarely, the message will be "I'm deeply concerned about the
*lasting* repercussions of having merged this patch". And so, yes, I
think that we might want to be clearer about looking for nuances like
that.

[1] http://www.scottaaronson.com/blog/?p=418
-- 
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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Tom Lane]

Dean Rasheed  writes:
> On 19 April 2016 at 14:38, Tom Lane  wrote:
>> Yeah, what I was thinking of printing is something like
>> 
>> asind(x),
>> asind(x) IN (-90,-30,0,30,90) AS asind_exact,
>> ...
>> 
>> with extra_float_digits=3.

> OK, that sounds like it would be a useful improvement to the tests.

Pushed.  Peter, what results do you get from these tests on your
problematic machine?

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] Postgres 9.6 scariest patch tournament

[Tom Lane]

Alvaro Herrera  writes:
> Peter Geoghegan wrote:
>> I would have appreciated more scope to say how confident I am in my
>> prediction, and how scary in absolute terms I consider the scariest
>> patches to be.

> It was purposefully ambiguous.  Maybe it should have been stated
> explicitely.

I was thinking about complaining that "scariest" and "most bugs" are
not the same thing.  Features you can turn off are not very scary,
even if they're full of bugs (cough ... parallel query ... cough),
because we could just ship 'em disabled by default until there's more
reason to trust them.  What I find scary is patches that can break
existing use-cases with no simple workaround.  I'm not sure which one
to vote for yet.

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] Postgres 9.6 scariest patch tournament

[Alvaro Herrera]

Peter Geoghegan wrote:
> On Mon, Apr 18, 2016 at 1:22 PM, Josh berkus  wrote:
> > We should send the owner of the scariest patch something as a prize.
> > Maybe a plastic skeleton or something ...
> 
> I think it was a good idea to call it the scariest patch rather than
> something more severe sounding. Having the poll only be half-serious
> is a good way to avoid self-censorship, and emphasizes that we're
> concerned about bugs that cause serious instability to the system as a
> whole. We're less concerned about the overall number of bugs in any
> given patch.

This guy reads my mind.  Where's my tinfoil hat?

> I would have appreciated more scope to say how confident I am in my
> prediction, and how scary in absolute terms I consider the scariest
> patches to be.

It was purposefully ambiguous.  Maybe it should have been stated
explicitely.

-- 
Á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] Postgres 9.6 scariest patch tournament

[Peter Geoghegan]

On Mon, Apr 18, 2016 at 1:22 PM, Josh berkus  wrote:
> We should send the owner of the scariest patch something as a prize.
> Maybe a plastic skeleton or something ...

I think it was a good idea to call it the scariest patch rather than
something more severe sounding. Having the poll only be half-serious
is a good way to avoid self-censorship, and emphasizes that we're
concerned about bugs that cause serious instability to the system as a
whole. We're less concerned about the overall number of bugs in any
given patch.

I would have appreciated more scope to say how confident I am in my
prediction, and how scary in absolute terms I consider the scariest
patches to be.

-- 
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] Reducing the size of BufferTag & remodeling forks

[Alvaro Herrera]

Andres Freund wrote:

> I've actually changed course a bit and I'm trying something different: A
> two level structure. One hashtable that maps (RelFileNode, ForkNumber)
> to a 'open relation' data structure, and from there a radix tree over
> just the block number. To avoid having to look up in the hashtable
> frequently there's a pointer in RelationData to a fork's radix tree.

Is this going anywhere, or did you drop the subject altogether?

-- 
Á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] Parser extensions (maybe for 10?)

[Pavel Stehule]

2016-04-19 12:49 GMT+02:00 Simon Riggs :

> On 12 April 2016 at 06:51, Tom Lane  wrote:
>
>> Craig Ringer  writes:
>> > The other area where there's room for extension without throwing out the
>> > whole thing and rebuilding is handling of new top-level statements. We
>> can
>> > probably dispatch the statement text to a sub-parser provided by an
>> > extension that registers interest in that statement name when we
>> attempt to
>> > parse it and fail. Even then I'm pretty sure it won't be possible to do
>> so
>> > while still allowing multi-statements. I wish we didn't support
>> > multi-statements, but we're fairly stuck with them.
>>
>> Well, as I said, I've been there and done that.  Things get sticky
>> when you notice that those "new top-level statements" would like to
>> contain sub-clauses (e.g. arithmetic expressions) that should be defined
>> by the core grammar.  And maybe the extension would also like to
>> define additions to the expression grammar, requiring a recursive
>> callback into the extension.  It gets very messy very fast.
>
>
> As Tom says, we can't easily break it down into multiple co-operating
> pieces, so lets forget that as unworkable.
>
> What is possible is a whole new grammar... for example if we imagine
>
>  SET client_language_path = 'foo, postgresql'
>
> Works similar to search_path, but not userset. We try to parse incoming
> statements against the foo parser first, if that fails we try postgresql.
> The default setting would be simply 'postgresql', so no match -> syntax
> error.
>

The idea is good. I don't understand to name "client_language_path" - it is
not clean - a) this is server side feature, b) we use term "language" for
PL, so any other term will be better.


>
> We could make that easier by making the postgresql parser a plugin itself.
> So to produce a new one you just copy the files, modify them as needed then
> insert a new record into pg_language as an extension.
>
> --
> Simon Riggshttp://www.2ndQuadrant.com/
> 
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] snapshot too old, configured by time

[Alvaro Herrera]

Kevin Grittner wrote:
> On Tue, Apr 19, 2016 at 11:02 AM, Alvaro Herrera
>  wrote:

> > Robert Haas wrote:
> >
> >> That wouldn't have fixed my problem, which involved rebasing a patch.
> >
> > True.  I note that it's possible to munge a patch mechanically to sort
> > out this situation.
> 
> I admit it is possible.  I'm becoming more convinced with each post
> that it's the wrong approach.  I feel like I have been in the
> modern version of an Æsop fable here:
> 
> http://www.bartleby.com/17/1/62.html

LOL.

Well, it seems I'm outvoted.  I leave you to do with your donkey as you
please.

-- 
Á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] snapshot too old, configured by time

[Kevin Grittner]

On Tue, Apr 19, 2016 at 11:02 AM, Alvaro Herrera
 wrote:
> Andres Freund wrote:
>> On 2016-04-19 12:03:22 -0300, Alvaro Herrera wrote:
>
 Since this change to BufferGetPage() has caused severe back-patch
 pain for at least two committers so far, I will revert that (very
 recent) change to this patch later today unless I hear an
 objections.
>>>
>>> I vote for back-patching a no-op change instead, as discussed elsewhere.
>>
>> What about Tom's argument that that'd be problematic for external code?
>
> Kevin offered to code it in a way that maintains ABI and API
> compatibility with some trickery.

I pointed out that it would be possible to do so, but specifically
said I wasn't arguing for that.  We would need to create a new name
for what BufferGetPage() does on master, and have that call the old
BufferGetPage() on back-branches.  That seems pretty ugly.

I tend to think that the original approach, while it puts the
burden on coders to recognize when TestForOldSnapshot() must be
called, is no more onerous than many existing issues coders much
worry about -- like whether to add something to outfuncs.c, as an
example.  I have been skeptical of the nanny approach all along,
and after seeing the impact of having it in the tree for a few
days, I really am inclined to pull back and put this on the same
footing as the other things hackers need to learn and tend to as
they code.

> Robert Haas wrote:
>
>> That wouldn't have fixed my problem, which involved rebasing a patch.
>
> True.  I note that it's possible to munge a patch mechanically to sort
> out this situation.

I admit it is possible.  I'm becoming more convinced with each post
that it's the wrong approach.  I feel like I have been in the
modern version of an Æsop fable here:

http://www.bartleby.com/17/1/62.html

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


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

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Alvaro Herrera wrote:


Michael Paquier wrote:

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
 wrote:

Now, I have produced two patches:
- 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
__crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
hack, though I am coming to think that this may be the approach that
would us the less harm, and that's closer to what is done for VS 2012
and 2013.
- 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
GetLocaleInfoEx, this requires us to lower a bit the support grid for
Windows, basically that's throwing support for XP if compilation is
done with VS 2015.
Based on my tests, both are working with short and long local names,
testing via initdb --locale.


The first patch is actually not what I wanted to send. Here are the
correct ones...


This thread seems to have stalled.  I thought we were going to consider
these patches for 9.6.  Should we simply push them to see what the
buildfarm thinks?  Has anyone other than Michael actually tested it in
VS2015?


I built them both, and for lack of a better idea, ran initdb with all 
(short) locales in the Vista list 
(https://msdn.microsoft.com/en-us/goglobal/bb896001.aspx, second column) 
whose ANSI codepage is not either 0 or 1252. The former probably means 
"no such thing", the latter is my own default which I excluded to detect 
cases where it falls back to that.


Both patches behave exactly the same in this test. Of the 102 remaining 
locales, I get an unexpected codepage for just four:


- kk: Expected 1251, used 1252
- kk-KZ: Expected 1251, used 1252
- sr: Expected 1251, used 1250
- uk: Expected 1251, used 1252

I suspect that "sr" is a bug in MSDN: 1250 is Eastern European (Latin), 
1251 is Cyrillic, and "sr" alone is listed as Latin. So either the 
script or the codepage are likely wrong.


--
Christian



--
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] snapshot too old, configured by time

[Alvaro Herrera]

Andres Freund wrote:
> On 2016-04-19 12:03:22 -0300, Alvaro Herrera wrote:

> > > Since this change to BufferGetPage() has caused severe back-patch
> > > pain for at least two committers so far, I will revert that (very
> > > recent) change to this patch later today unless I hear an
> > > objections.
> > 
> > I vote for back-patching a no-op change instead, as discussed elsewhere.
> 
> What about Tom's argument that that'd be problematic for external code?

Kevin offered to code it in a way that maintains ABI and API
compatibility with some trickery.


Robert Haas wrote:

> That wouldn't have fixed my problem, which involved rebasing a patch.

True.  I note that it's possible to munge a patch mechanically to sort
out this situation.

> I really think it's also a bad precedent to back-patch things into
> older branches that are not themselves bug fixes.  Users count on us
> not to destabilize older branches, and that means being minimalist
> about what we put into them.

Well, this wouldn't change the inner working of the code at all, only
how it looks, so it wouldn't affect users.  I grant that it would affect
developers of forks.

-- 
Á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] Parser extensions (maybe for 10?)

[Stas Kelvich]

> On 12 Apr 2016, at 07:36, Arcadiy Ivanov  wrote:
> 
> [ 
>   DISTRIBUTE BY { REPLICATION | ROUNDROBIN | { [HASH | MODULO ] ( column_name 
> ) } } |
>   DISTRIBUTED { { BY ( column_name ) } | { RANDOMLY } |
>   DISTSTYLE { EVEN | KEY | ALL } DISTKEY ( column_name )
> ]
> [ TO { GROUP groupname | NODE ( nodename [, ... ] ) } ]

Less invasive way to achieve same is to use WITH parameter
that already exists in CREATE TABLE, CREATE INDEX, etc.

Like that:

create table foo(id int) with(distributed_by=‘id’, nodes=’node1, node2’);

That’s easier to allow extensions to define custom parameters for WITH, than
to extend parser.

-- 
Stas Kelvich
Postgres Professional: http://www.postgrespro.com
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

[HACKERS] Proposal: Remove regress-python3-mangle.mk

[Yury Zhuravlev]

Hello.
Now we generate tests for plpython3 of plpython2 tests. I think we should 
write independently 2 test suite.

Why is that bad:
1. Differences between python2 and python3 more than can be solved by 
regexps. And these differences are growing.
2. We must be careful to write tests, so as not to break the converter. And 
we can not verify the possibility python3.
3. Because we use sed we do not tests for plpython3 under Windows. And I 
have trouble with CMake too. 

Yes, we will have 2 times more similar code but you need to relates to the 
python2 and python3 as the different languages.


Thank you for attention.
--
Yury Zhuravlev
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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Dean Rasheed]

On 19 April 2016 at 14:38, Tom Lane  wrote:
> Yeah, what I was thinking of printing is something like
>
> asind(x),
> asind(x) IN (-90,-30,0,30,90) AS asind_exact,
> ...
>
> with extra_float_digits=3.  The point of this is not necessarily to give
> any extra information, though it might, but for failures to be more easily
> interpretable.  If I'd forgotten how the test worked just a few months
> after committing it, how likely is it that some random user faced with a
> similar failure would understand what they were seeing?
>
> Also, though I agree that it might not help much to know whether the
> output is 45.0001 or 44., our thoughts would
> be trending in quite a different direction if it turns out that the
> output is radically wrong, or even a NaN.  The existing test cannot
> exclude that possibility.
>

OK, that sounds like it would be a useful improvement to the tests.

Regards,
Dean


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Kevin Grittner]

On Tue, Apr 19, 2016 at 10:14 AM, Robert Haas  wrote:
> On Tue, Apr 19, 2016 at 11:11 AM, Kevin Grittner  wrote:
>> On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila  wrote:
>>> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund  wrote:

 On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
 > That is more controversial than the potential ~2% regression for
 > old_snapshot_threshold=-1.  Alvaro[2] and Robert[3] are okay releasing
 > that way, and Andres[4] is not.

 FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
 a clear proposal on the table how to solve the scalability issue around
 MaintainOldSnapshotTimeMapping().
>>>
>>> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
>>> takes EXCLUSIVE LWLock which seems to be a probable reason for a performance
>>> regression.  Now, here the question is do we need to acquire that lock if
>>> xmin is not changed since the last time value of
>>> oldSnapshotControl->latest_xmin is updated or xmin is lesser than equal to
>>> oldSnapshotControl->latest_xmin?
>>> If we don't need it for above cases, I think it can address the performance
>>> regression to a good degree for read-only workloads when the feature is
>>> enabled.
>>
>> Thanks, Amit -- I think something along those lines is the right
>> solution to the scaling issues when the feature is enabled.  For
>> now I'm focusing on the back-patching issues and the performance
>> regression when the feature is disabled, but I'll shift focus to
>> this once the "killer" issues are in hand.
>
> Maybe Amit could try his idea in parallel.

That would be great!

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


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Robert Haas]

On Tue, Apr 19, 2016 at 11:11 AM, Kevin Grittner  wrote:
> On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila  wrote:
>> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund  wrote:
>>>
>>> On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
>>> > That is more controversial than the potential ~2% regression for
>>> > old_snapshot_threshold=-1.  Alvaro[2] and Robert[3] are okay releasing
>>> > that way, and Andres[4] is not.
>>>
>>> FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
>>> a clear proposal on the table how to solve the scalability issue around
>>> MaintainOldSnapshotTimeMapping().
>>
>> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
>> takes EXCLUSIVE LWLock which seems to be a probable reason for a performance
>> regression.  Now, here the question is do we need to acquire that lock if
>> xmin is not changed since the last time value of
>> oldSnapshotControl->latest_xmin is updated or xmin is lesser than equal to
>> oldSnapshotControl->latest_xmin?
>> If we don't need it for above cases, I think it can address the performance
>> regression to a good degree for read-only workloads when the feature is
>> enabled.
>
> Thanks, Amit -- I think something along those lines is the right
> solution to the scaling issues when the feature is enabled.  For
> now I'm focusing on the back-patching issues and the performance
> regression when the feature is disabled, but I'll shift focus to
> this once the "killer" issues are in hand.

Maybe Amit could try his idea in parallel.

-- 
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] snapshot too old, configured by time

[Robert Haas]

On Tue, Apr 19, 2016 at 11:03 AM, Alvaro Herrera
 wrote:
> Kevin Grittner wrote:
>> On Tue, Apr 19, 2016 at 6:38 AM, Robert Haas  wrote:
>>
>> > The right thing to do about that is just change it back to the
>> > way Kevin had it originally.
>>
>> Since this change to BufferGetPage() has caused severe back-patch
>> pain for at least two committers so far, I will revert that (very
>> recent) change to this patch later today unless I hear an
>> objections.
>
> I vote for back-patching a no-op change instead, as discussed elsewhere.

That wouldn't have fixed my problem, which involved rebasing a patch.
I really think it's also a bad precedent to back-patch things into
older branches that are not themselves bug fixes.  Users count on us
not to destabilize older branches, and that means being minimalist
about what we put into them.

-- 
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: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Kevin Grittner]

On Tue, Apr 19, 2016 at 9:57 AM, Amit Kapila  wrote:
> On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund  wrote:
>>
>> On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
>> > That is more controversial than the potential ~2% regression for
>> > old_snapshot_threshold=-1.  Alvaro[2] and Robert[3] are okay releasing
>> > that way, and Andres[4] is not.
>>
>> FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
>> a clear proposal on the table how to solve the scalability issue around
>> MaintainOldSnapshotTimeMapping().
>
> It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
> takes EXCLUSIVE LWLock which seems to be a probable reason for a performance
> regression.  Now, here the question is do we need to acquire that lock if
> xmin is not changed since the last time value of
> oldSnapshotControl->latest_xmin is updated or xmin is lesser than equal to
> oldSnapshotControl->latest_xmin?
> If we don't need it for above cases, I think it can address the performance
> regression to a good degree for read-only workloads when the feature is
> enabled.

Thanks, Amit -- I think something along those lines is the right
solution to the scaling issues when the feature is enabled.  For
now I'm focusing on the back-patching issues and the performance
regression when the feature is disabled, but I'll shift focus to
this once the "killer" issues are in hand.

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


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

Re: [HACKERS] snapshot too old, configured by time

[Andres Freund]

On 2016-04-19 12:03:22 -0300, Alvaro Herrera wrote:
> Kevin Grittner wrote:
> > On Tue, Apr 19, 2016 at 6:38 AM, Robert Haas  wrote:
> > 
> > > The right thing to do about that is just change it back to the
> > > way Kevin had it originally.
> > 
> > Since this change to BufferGetPage() has caused severe back-patch
> > pain for at least two committers so far, I will revert that (very
> > recent) change to this patch later today unless I hear an
> > objections.
> 
> I vote for back-patching a no-op change instead, as discussed elsewhere.

What about Tom's argument that that'd be problematic for external code?


-- 
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] snapshot too old, configured by time

[Alvaro Herrera]

Kevin Grittner wrote:
> On Tue, Apr 19, 2016 at 6:38 AM, Robert Haas  wrote:
> 
> > The right thing to do about that is just change it back to the
> > way Kevin had it originally.
> 
> Since this change to BufferGetPage() has caused severe back-patch
> pain for at least two committers so far, I will revert that (very
> recent) change to this patch later today unless I hear an
> objections.

I vote for back-patching a no-op change instead, as discussed elsewhere.

-- 
Á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] Re: [COMMITTERS] pgsql: Avoid extra locks in GetSnapshotData if old_snapshot_threshold

[Amit Kapila]

On Sun, Apr 17, 2016 at 2:26 AM, Andres Freund  wrote:
>
> On 2016-04-16 16:44:52 -0400, Noah Misch wrote:
> > That is more controversial than the potential ~2% regression for
> > old_snapshot_threshold=-1.  Alvaro[2] and Robert[3] are okay releasing
> > that way, and Andres[4] is not.
>
> FWIW, I could be kinda convinced that it's temporarily ok, if there'd be
> a clear proposal on the table how to solve the scalability issue around
> MaintainOldSnapshotTimeMapping().
>

It seems that for read-only workloads, MaintainOldSnapshotTimeMapping()
takes EXCLUSIVE LWLock which seems to be a probable reason for a
performance regression.  Now, here the question is do we need to acquire
that lock if xmin is not changed since the last time value of
oldSnapshotControl->latest_xmin is updated or xmin is lesser than equal to
oldSnapshotControl->latest_xmin?
If we don't need it for above cases, I think it can address the performance
regression to a good degree for read-only workloads when the feature is
enabled.


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

Re: [HACKERS] Parser extensions (maybe for 10?)

[Aleksander Alekseev]

> On 2016-04-19 12:20:03 +0300, Aleksander Alekseev wrote:
> > Can we guarantee that extensions don't conflict? In fact we can
> > since we already do it. If all tests pass there is no conflict.
> 
> How does that follow? Even if you were to test all possible extensions
> together - obviously not possible - how do passing tests prove the
> grammar to be conflict free?

Do we currently test that all existing extensions work together? No.
And in fact it doesn't matter whether they work together or not. What
matters that concrete subset of extensions chosen by given user work
together. We don't guarantee that extensions are bug free either. In
fact I'm quite sure there are many bugs in PostgreSQL extensions and
PostgreSQL itself. But if `make check` pass probably extension doesn't
have more bugs than usual. Why syntax extension should suddenly be an
exception of these rules?

Also I would like to remind that suggested approach is only about
syntax sugar. The resulting desugared query would be the same as usual.
If it's invalid we just discard it.


For the record - I'm not telling that this SQL extending feature should
necessarily be implemented. Frankly I'm personally quite against it.
I can't think of any real cases when it would be very useful and I don't
think that this feature is worth an effort, not mentioning further
support. All I'm telling is that it could be done using methods that are
well-known for decades.

-- 
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] Declarative partitioning

[Amit Langote]

Hi Idlar, Alexander,

On Tue, Apr 19, 2016 at 11:26 PM, Alexander Korotkov
 wrote:
> On Tue, Apr 19, 2016 at 4:57 PM, Ildar Musin  wrote:
>>
>> Thanks for your new patch! I've tried it and discovered some strange
>> behavior for partitioning by composite key. Here is an example of my setup:
>>
>> create table test(a int, b int) partition by range (a, b);
>> create table test_1 partition of test for values start (0, 0) end (100,
>> 100);
>> create table test_2 partition of test for values start (100, 100) end
>> (200, 200);
>> create table test_3 partition of test for values start (200, 200) end
>> (300, 300);
>>
>> It's alright so far. But if we try to insert record in which attribute 'a'
>> belongs to one partition and attribute 'b' belongs to another then record
>> will be inserted in the first one:
>>
>> insert into test(a, b) values (150, 50);
>>
>> select tableoid::regclass, * from test;
>>  tableoid |  a  | b
>> --+-+
>>  test_2   | 150 | 50
>> (1 row)
>
>
> That's how composite keys work. First subkey is checked. If it's equal then
> second subkey is checked and so on.
>
> # SELECT (100, 100) < (150, 50), (150, 50) < (200, 200);
>  ?column? | ?column?
> --+--
>  t| t
> (1 row)

Yes.

> Another question is that it might be NOT what users expect from that.  From
> the syntax side it very looks like defining something boxes regions for two
> keys which could be replacement for subpartitioning.  But it isn't so.

Need to check why query with qual b < 100 behaves the way it does.
Something's going wrong there with the constraints (partition
predicates) that are being generated internally (as mentioned before,
still driven by constraint exclusion using the constraints generated
on-the-fly).

As for the composite range partition bounds in Ildar's example, it's
as if the second value in the key never determines the fate of a row
going into some partition, therefore no constraints should have been
generated for column b of the key.  I'm afraid that's not the case as
per the latest patch.  Will fix.

Thanks a lot for trying it out and the report.

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] snapshot too old, configured by time

[Kevin Grittner]

On Tue, Apr 19, 2016 at 6:38 AM, Robert Haas  wrote:

> The right thing to do about that is just change it back to the
> way Kevin had it originally.

Since this change to BufferGetPage() has caused severe back-patch
pain for at least two committers so far, I will revert that (very
recent) change to this patch later today unless I hear an
objections.

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


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

Re: [HACKERS] Declarative partitioning

[Alexander Korotkov]

On Tue, Apr 19, 2016 at 4:57 PM, Ildar Musin  wrote:

> On 15.04.2016 07:35, Amit Langote wrote:
>
>> Thanks a lot for the comments. The patch set changed quite a bit since
>> the last version. Once the CF entry was marked returned with feedback on
>> March 22, I held off sending the new version at all. Perhaps, it would have
>> been OK. Anyway here it is, if you are interested. I will create an entry
>> in CF 2016-09 for the same. Also, see below replies to you individual
>> comments.
>>
>
> Thanks for your new patch! I've tried it and discovered some strange
> behavior for partitioning by composite key. Here is an example of my setup:
>
> create table test(a int, b int) partition by range (a, b);
> create table test_1 partition of test for values start (0, 0) end (100,
> 100);
> create table test_2 partition of test for values start (100, 100) end
> (200, 200);
> create table test_3 partition of test for values start (200, 200) end
> (300, 300);
>
> It's alright so far. But if we try to insert record in which attribute 'a'
> belongs to one partition and attribute 'b' belongs to another then record
> will be inserted in the first one:
>
> insert into test(a, b) values (150, 50);
>
> select tableoid::regclass, * from test;
>  tableoid |  a  | b
> --+-+
>  test_2   | 150 | 50
> (1 row)


That's how composite keys work. First subkey is checked. If it's equal then
second subkey is checked and so on.

# SELECT (100, 100) < (150, 50), (150, 50) < (200, 200);
 ?column? | ?column?
--+--
 t| t
(1 row)

Another question is that it might be NOT what users expect from that.  From
the syntax side it very looks like defining something boxes regions for two
keys which could be replacement for subpartitioning.  But it isn't so.

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

[HACKERS] Avoid parallel full and right join paths.

[Mithun Cy]

Tests:
create table mytab(x int,x1 char(9),x2 varchar(9));
create table mytab1(y int,y1 char(9),y2 varchar(9));
insert into mytab values (generate_series(1,5),'aa','aaa');
insert into mytab1 values (generate_series(1,1),'aa','aaa');
insert into mytab values (generate_series(1,50),'aa','aaa');
insert into mytab values (generate_series(1,50),'aa','aaa');
analyze mytab;
analyze mytab1;
vacuum mytab;
vacuum mytab1;

set max_parallel_degree=0;
SET
df=# SELECT count(*) FROM mytab RIGHT OUTER JOIN mytab1
ON mytab.x = mytab1.y;
count
---
3
(1 row)

# set max_parallel_degree=5;
SET
df=# SELECT count(*) FROM mytab RIGHT OUTER JOIN mytab1
ON mytab.x = mytab1.y;
count
---
39089
(1 row)

Casue:
==
Normal plan
==
explain SELECT count(*) FROM mytab RIGHT OUTER JOIN mytab1
ON mytab.x = mytab1.y;postgres-#
QUERY PLAN
--
Aggregate (cost=21682.71..21682.72 rows=1 width=8)
-> Hash Right Join (cost=289.00..21629.07 rows=21457 width=0)
Hash Cond: (mytab.x = mytab1.y)
-> Seq Scan on mytab (cost=0.00..17188.00 rows=105 width=4)
-> Hash (cost=164.00..164.00 rows=1 width=4)
-> Seq Scan on mytab1 (cost=0.00..164.00 rows=1 width=4)
=

Parallel plan.
==
explain SELECT count(*) FROM mytab RIGHT OUTER JOIN mytab1
ON mytab.x = mytab1.y;postgres-#
QUERY PLAN
---
Finalize Aggregate (cost=14135.88..14135.89 rows=1 width=8)
-> Gather (cost=14135.67..14135.88 rows=2 width=8)
Number of Workers: 2
-> Partial Aggregate (cost=13135.67..13135.68 rows=1 width=8)
-> Hash Right Join (cost=289.00..13082.02 rows=21457 width=0)
Hash Cond: (mytab.x = mytab1.y)
-> Parallel Seq Scan on mytab (cost=0.00..11063.00 rows=437500 width=4)
-> Hash (cost=164.00..164.00 rows=1 width=4)
-> Seq Scan on mytab1 (cost=0.00..164.00 rows=1 width=4)


As above Right and Full join paths cannot be parallel as they can produce
false null extended rows because outer table is partial path and not
completely visible.
Adding a patch to fix same.

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


avoid_parallel_full_right_join.patch
Description: Binary data

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

Re: [HACKERS] Parser extensions (maybe for 10?)

[Andres Freund]

On 2016-04-19 12:20:03 +0300, Aleksander Alekseev wrote:
> Can we guarantee that extensions don't conflict? In fact we can since
> we already do it. If all tests pass there is no conflict.

How does that follow? Even if you were to test all possible extensions
together - obviously not possible - how do passing tests prove the
grammar to be conflict free?

Andres


-- 
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] Parser extensions (maybe for 10?)

[Andres Freund]

On 2016-04-19 15:32:07 +0300, Aleksander Alekseev wrote:
> > As Tom says, we can't easily break it down into multiple co-operating
> > pieces, so lets forget that as unworkable.
> 
> I'm sorry but didn't I just demonstrate the opposite?

I doubt it.

> If so it's very
> easy to prove - give a counterexample. As I understand approach I
> described handles cases named by Tom just fine. In fact the idea of
> transforming ASTs (a.k.a metaprogramming) is successfully used by
> programmers for about 50 years now.
> 
> (As a side note - I'm not a native English speaker but I believe such
> type of logic is known as "argument from authority".)

And the above is called an ad-hominem.


-- 
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] Declarative partitioning

[Ildar Musin]

Hi Amit,

On 15.04.2016 07:35, Amit Langote wrote:
Thanks a lot for the comments. The patch set changed quite a bit since 
the last version. Once the CF entry was marked returned with feedback 
on March 22, I held off sending the new version at all. Perhaps, it 
would have been OK. Anyway here it is, if you are interested. I will 
create an entry in CF 2016-09 for the same. Also, see below replies to 
you individual comments.


Thanks for your new patch! I've tried it and discovered some strange 
behavior for partitioning by composite key. Here is an example of my setup:


create table test(a int, b int) partition by range (a, b);
create table test_1 partition of test for values start (0, 0) end (100, 
100);
create table test_2 partition of test for values start (100, 100) end 
(200, 200);
create table test_3 partition of test for values start (200, 200) end 
(300, 300);


It's alright so far. But if we try to insert record in which attribute 
'a' belongs to one partition and attribute 'b' belongs to another then 
record will be inserted in the first one:


insert into test(a, b) values (150, 50);

select tableoid::regclass, * from test;
 tableoid |  a  | b
--+-+
 test_2   | 150 | 50
(1 row)

I think there should be an error because value for 'b' violates range 
constraint for test_2. Now if we query data from 'test' and add filter b 
< 100, then planner will exclude partitions 'test_2' (which actually 
contains inserted row) and 'test_3' and return nothing:


select * from test where b < 100;
 a | b
---+---
(0 rows)

explain (costs off) select * from test where b < 100;
QUERY PLAN
---
 Append
   ->  Seq Scan on test
 Filter: (b < 100)
   ->  Seq Scan on test_1
 Filter: (b < 100)
(5 rows)

--
Ildar Musin
i.mu...@postgrespro.ru



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

Re: [HACKERS] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Tom Lane]

Dean Rasheed  writes:
> On 19 April 2016 at 05:16, Noah Misch  wrote:
>> On Mon, Apr 18, 2016 at 11:56:55PM -0400, Tom Lane wrote:
>>> Hm?  The expected answer is exact (30, 45, or whatever) in each case.
>>> If we get some residual low-order digits then it's a failure, so we don't
>>> need to worry about whether it's the same failure everywhere.

>> Does something forbid snprintf implementations from printing '45'::float8 as
>> 45.0001 under extra_float_digits=3?

> I'm not sure it's really worth having the test output something like
> 45.0001 since that extra detail doesn't really seem
> particularly useful beyond the fact that the result wasn't exactly 45.
> Also you'd have to be careful how you modified the test, since it's
> possible that 45.0001 might be printed as '45' even under
> extra_float_digits=3 and so there'd be a risk of the test passing when
> it ought to fail, unless you also printed something else out to
> indicate exactness.

Yeah, what I was thinking of printing is something like

asind(x),
asind(x) IN (-90,-30,0,30,90) AS asind_exact,
...

with extra_float_digits=3.  The point of this is not necessarily to give
any extra information, though it might, but for failures to be more easily
interpretable.  If I'd forgotten how the test worked just a few months
after committing it, how likely is it that some random user faced with a
similar failure would understand what they were seeing?

Also, though I agree that it might not help much to know whether the
output is 45.0001 or 44., our thoughts would
be trending in quite a different direction if it turns out that the
output is radically wrong, or even a NaN.  The existing test cannot
exclude that possibility.

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] Postgres 9.6 scariest patch tournament

[Chapman Flack]

On 04/18/2016 04:22 PM, Josh berkus wrote:
> 
> We should send the owner of the scariest patch something as a prize.
> Maybe a plastic skeleton or something ...

A mouse.

-Chap



-- 
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] Parser extensions (maybe for 10?)

[Aleksander Alekseev]

> As Tom says, we can't easily break it down into multiple co-operating
> pieces, so lets forget that as unworkable.

I'm sorry but didn't I just demonstrate the opposite? If so it's very
easy to prove - give a counterexample. As I understand approach I
described handles cases named by Tom just fine. In fact the idea of
transforming ASTs (a.k.a metaprogramming) is successfully used by
programmers for about 50 years now.

(As a side note - I'm not a native English speaker but I believe such
type of logic is known as "argument from authority".)

> What is possible is a whole new grammar... for example if we imagine
> 
>  SET client_language_path = 'foo, postgresql'
> 
> Works similar to search_path, but not userset. We try to parse
> incoming statements against the foo parser first, if that fails we
> try postgresql. The default setting would be simply 'postgresql', so
> no match -> syntax error.
> 
> We could make that easier by making the postgresql parser a plugin
> itself. So to produce a new one you just copy the files, modify them
> as needed then insert a new record into pg_language as an extension.
> 

I think its not an extension but a replacement of a grammar. This
approach implies that every extension implements a parser from scratch.
Not sure if anyone will do it in practice to change SQL syntax a little
bit.

I'm not telling that such a feature will be completely worthless. But
why not to make a step further and not to implement plugable protocols?
E.g. make PostgreSQL compatible with MySQL and/or MongoDB? Or maybe
implement SQL dialect that forbids implicit type conversion. Or add
build-in connection pooling mechanism. I wonder though if all of this
could already be implemented as an extension without any changes in
PostgreSQL core. 

-- 
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] Parser extensions (maybe for 10?)

[Oleg Bartunov]

On Tue, Apr 19, 2016 at 1:49 PM, Simon Riggs  wrote:

> On 12 April 2016 at 06:51, Tom Lane  wrote:
>
>> Craig Ringer  writes:
>> > The other area where there's room for extension without throwing out the
>> > whole thing and rebuilding is handling of new top-level statements. We
>> can
>> > probably dispatch the statement text to a sub-parser provided by an
>> > extension that registers interest in that statement name when we
>> attempt to
>> > parse it and fail. Even then I'm pretty sure it won't be possible to do
>> so
>> > while still allowing multi-statements. I wish we didn't support
>> > multi-statements, but we're fairly stuck with them.
>>
>> Well, as I said, I've been there and done that.  Things get sticky
>> when you notice that those "new top-level statements" would like to
>> contain sub-clauses (e.g. arithmetic expressions) that should be defined
>> by the core grammar.  And maybe the extension would also like to
>> define additions to the expression grammar, requiring a recursive
>> callback into the extension.  It gets very messy very fast.
>
>
> As Tom says, we can't easily break it down into multiple co-operating
> pieces, so lets forget that as unworkable.
>
> What is possible is a whole new grammar... for example if we imagine
>
>  SET client_language_path = 'foo, postgresql'
>
> Works similar to search_path, but not userset. We try to parse incoming
> statements against the foo parser first, if that fails we try postgresql.
> The default setting would be simply 'postgresql', so no match -> syntax
> error.
>
>
that's interesting. I'd add parse_error_handler, which actually processes
syntax error.

SET client_language_path = 'foo, postgresql, parse_error_handler'


> We could make that easier by making the postgresql parser a plugin itself.
> So to produce a new one you just copy the files, modify them as needed then
> insert a new record into pg_language as an extension.
>
> --
> Simon Riggshttp://www.2ndQuadrant.com/
> 
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/19/2016 01:47 AM, Michael Paquier wrote:

On Tue, Apr 19, 2016 at 2:42 PM, Alvaro Herrera
 wrote:

Michael Paquier wrote:

On Mon, Apr 11, 2016 at 3:25 PM, Michael Paquier
 wrote:

Now, I have produced two patches:
- 0001-Support-for-VS-2015-locale-hack.patch, which makes use of
__crt_locale_data_public in ucrt/corecrt.h. This is definitely an ugly
hack, though I am coming to think that this may be the approach that
would us the less harm, and that's closer to what is done for VS 2012
and 2013.
- 0001-Support-for-VS-2015-getlocaleinfoex.patch, which make use of
GetLocaleInfoEx, this requires us to lower a bit the support grid for
Windows, basically that's throwing support for XP if compilation is
done with VS 2015.
Based on my tests, both are working with short and long local names,
testing via initdb --locale.

The first patch is actually not what I wanted to send. Here are the
correct ones...

This thread seems to have stalled.  I thought we were going to consider
these patches for 9.6.  Should we simply push them to see what the
buildfarm thinks?

We need to choose first which one we'd like to have, it would be great
to get some input from Andrew or even Magnus here who I thought would
get the last word.



trying to make some more time to spend on this. I hope to in the next 
day or two.


cheers

andrew




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

Re: [HACKERS] snapshot too old, configured by time

[Robert Haas]

On Tue, Apr 19, 2016 at 1:23 AM, Michael Paquier
 wrote:
> On Tue, Apr 19, 2016 at 3:14 AM, Tom Lane  wrote:
>> Or in short: this is a whole lot further than I'm prepared to go to
>> satisfy one customer with a badly-designed application.  And from what
>> I can tell from the Feb 2015 discussion, that's what this has been
>> written for.
>
> This holds true. I imagine that a lot of people at least on this list
> have already spent some time in tracking down long-running
> transactions in someone's application and actually tuned the
> application so as the bloat gets reduced and things perform better for
> other transactions taking a shorter time. Without the need of this
> feature.

So I don't want to be too vociferous in defending a feature that (a)
was written by a colleague and (b) obviously isn't perfect, but I will
point out that:

1. There was a surprising amount of positive reaction when Kevin first
proposed this.  I expected a lot more people to say this kind of thing
at the beginning, when Kevin first brought this up, but in fact a
number of people wrote into say they'd really like to have this.
Those positive reaction shouldn't be forgotten just because those
people aren't wading into a discussion about the merits of adding
arguments to BufferGetPage.

2. Without this feature, you can kill sessions or transactions to
control bloat, but this feature is properly thought of as a way to
avoid bloat *without* killing sessions or transactions.  You can let
the session live, without having it generate bloat, just so long as it
doesn't try to touch any data that has been recently modified.  We
have no other feature in PostgreSQL that does something like that.

At the moment, what I see happening is that Tom, the one person who
has hated this feature since the beginning, still hates it, and we're
on the verge of asking Kevin to revert it because (1) Tom hates it and
(2) Kevin changed the BufferGetPage stuff in the way that Alvaro
requested.  I think that's not quite fair.  If we want to demand that
this feature be reverted because it causes a performance loss even
when turned off, I get that.  If we think that it's badly implemented,
fine, I get that, too.  But asking somebody to revert a patch because
the author adjusted things to match what the reviewer wanted is not
fair.  The right thing to do about that is just change it back to the
way Kevin had it originally.

-- 
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] Parser extensions (maybe for 10?)

[Simon Riggs]

On 12 April 2016 at 06:51, Tom Lane  wrote:

> Craig Ringer  writes:
> > The other area where there's room for extension without throwing out the
> > whole thing and rebuilding is handling of new top-level statements. We
> can
> > probably dispatch the statement text to a sub-parser provided by an
> > extension that registers interest in that statement name when we attempt
> to
> > parse it and fail. Even then I'm pretty sure it won't be possible to do
> so
> > while still allowing multi-statements. I wish we didn't support
> > multi-statements, but we're fairly stuck with them.
>
> Well, as I said, I've been there and done that.  Things get sticky
> when you notice that those "new top-level statements" would like to
> contain sub-clauses (e.g. arithmetic expressions) that should be defined
> by the core grammar.  And maybe the extension would also like to
> define additions to the expression grammar, requiring a recursive
> callback into the extension.  It gets very messy very fast.


As Tom says, we can't easily break it down into multiple co-operating
pieces, so lets forget that as unworkable.

What is possible is a whole new grammar... for example if we imagine

 SET client_language_path = 'foo, postgresql'

Works similar to search_path, but not userset. We try to parse incoming
statements against the foo parser first, if that fails we try postgresql.
The default setting would be simply 'postgresql', so no match -> syntax
error.

We could make that easier by making the postgresql parser a plugin itself.
So to produce a new one you just copy the files, modify them as needed then
insert a new record into pg_language as an extension.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Coverage report

[Aleksander Alekseev]

Hello, Alvaro.

> Some people have already heard about this.

Yes, we did :) Nice job!

> * Should we run something other than "make check-world"  As far as I
> know, that covers all or almost all the tests we have; are there
> things that we should have and are not running?  If so, how do we go
> about enabling them?

I'm personally quite concerned that currently everything that is in .h
files is considered a public API and thus should not be changed, or some
third-party extensions can break. Can we run `make check` on some (or
maybe all?) extensions from PGXN? This way we could guarantee that if
changing API breaks some extensions, this are extensions that actually
nobody uses or knows about.

Not mentioning that in this case we will check code coverage of these
extensions too.

> * buildfarm doesn't run make check-world for reasons of its own.
> Maybe we should run exactly what a typical buildfarm animal runs?
> That way, the coverage report would be closer to what we're actually
> verifying.

How about something like `make check; make -C contrib check` ?

> * Should we keep historical reports to study how numbers change in
> time? Maybe save one report a week or something like that?

Yes, I think we should. One report a day and a history for a week or two
should be enough.

> * Any other comments?

I noticed that output of genhtml looks like this:

```
Writing directory view page.
Overall coverage rate:
  lines..: 63.2% (179603 of 284239 lines)
  functions..: 69.5% (10550 of 15183 functions)
```

Maybe we should send an e-mail to pgsql-hackers@ automatically if these
numbers drops.

-- 
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] OS scheduler bugs affecting high-concurrency contention

[Andrea Suisani]

Hi,

On 04/16/2016 04:15 PM, Kevin Grittner wrote:

There is a paper that any one interested in performance at high
concurrency, especially in Linux, should read[1].  While doing
other work, a group of researchers saw behavior that they suspected
was due to scheduler bugs in Linux.  There were no tools that made
proving that practical, so they developed such a tool set and used
it to find four bugs in the Linux kernel which were introduced in
these releases, have not yet been fixed, and have this following
maximum impact when running NAS benchmarks, based on running with
and without the researchers' fixes for the bugs:

2.6.32:  22%
2,6.38:  13x
3.9: 27x
3.19:   138x

That's right -- one of these OS scheduler bugs in production
versions of Linux can make one of NASA's benchmarks run for 138
times as long as it does without the bug.  I don't feel that I can
interpret the results of any high-concurrency benchmarks in a
meaningful way without knowing which of these bugs were present in
the OS used for the benchmark.  Just as an example, it is helpful
to know that the benchmarks Andres presented were run on 3.16, so
it would have three of these OS bugs affecting results, but not the
most severe one.  I encourage you to read the paper an draw your
own conclusions.

Anyway, please don't confuse this thread with the one on the
"snapshot too old" patch -- I am still working on that and will
post results there when they are ready.



Thanks for the link, appreciated.

On slightly related topic, Jens Axboe proposed a patchset [1]
to improve the performance of background buffered writeback.

On Lwn.net an article about the issue at hand has been recently published [2].

Maybe this work could somewhat solve the problem experienced by PostgreSQL users
while checkpoint process flushes all pending changes to disk and recycles the
transaction logs.

--
Andrea Suisani
suis...@opinioni.net
Demetra opinioni.net srl

[1] "[PATCHSET v3][RFC] Make background writeback not suck"
http://thread.gmane.org/gmane.linux.kernel/2186732

[2] "Toward less-annoying background writeback"
https://lwn.net/SubscriberLink/682582/93d9e5b6bed03a32/




--
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] Parser extensions (maybe for 10?)

[Aleksander Alekseev]

> no - it is not possible. Not with Bison parser - it cannot work with
> unknown syntax - so isn't possible implement one part by parser A, and
> second part by parser B.
> 
> But we can parsers P1 and P2. P1 knows string XX, P2 knows YY. Buildin
> parser (BP) knows SQL
> 
> We can have registered parsers P1, P2, BP.
> 
> for string SELECT
> 
> P1 fails,
> P2 fails,
> BP processes it
> 
> for string YY
> 
> P1 fails,
> P2 process it,
> BP is not called
> 
> But transformations can be allowed too (but it is slower)
> 
> for string 
> 
> P1 does transformation to YYY
> P2 does transformation to SELECT
> BP processes it

I look on this a little bit differently.

Current pipeline is approximately like this:

```
query string -> LEX -> [lexemes] -> SYNT -> QueryAST -> PLANNER
```

Or in Haskell-like notation:

```
lex :: String -> [Lexeme]
synt :: [Lexeme] -> AST
```

Its reasonably simple to extend a lexer. Lets say that AST type doesn't
change, i.e. extensions provide only syntax sugar. After desugaring
query transforms to old-good SELECT, UPDATE, procedures calls, etc. In
this case what extension does is actually:

```
type Parser = [Lexeme] -> AST
extendParser :: Parser -> Parser
```

Can we guarantee that extensions don't conflict? In fact we can since
we already do it. If all tests pass there is no conflict.

The only tricky part I see is that sometimes we want:

```
extendParser1 ( extendParser2 ( default ))
```

... and sometimes:

```
extendParser2 ( extendParser1 ( default ))
```

I don't think that order of extension will matter most of the time. But
we still should provide a mechanism to change this order. For instance,
contribs could provide a default priority of parser extension.
Extensions with higher priority are applied first. Also user can
resolve conflicts by manually overriding these priorities:

```
select pg_parser_extension_priorities();
select pg_override_parser_extension_priority('some_extension', 100500);
```

I think it should work.

Thought?

-- 
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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Dean Rasheed]

On 19 April 2016 at 05:16, Noah Misch  wrote:
> On Mon, Apr 18, 2016 at 11:56:55PM -0400, Tom Lane wrote:
>> Noah Misch  writes:
>> > On Mon, Apr 18, 2016 at 10:22:59PM -0400, Tom Lane wrote:
>> >> We could alternatively set extra_float_digits to its max value and hope
>> >> that off-by-one-in-the-last-place values would get printed as something
>> >> visibly different from the exact result.  I'm not sure I want to trust
>> >> that that works reliably; but maybe it would be worth printing the
>> >> result both ways, just to provide additional info when there's a failure.
>>
>> > We'd have an independent problem if extra_float_digits=3 prints the same
>> > digits for distinguishable float values, so I wouldn't mind relying on it 
>> > not
>> > to do that.  But can we expect the extra_float_digits=3 representation of
>> > those particular values to be the same for every implementation?
>>
>> Hm?  The expected answer is exact (30, 45, or whatever) in each case.
>> If we get some residual low-order digits then it's a failure, so we don't
>> need to worry about whether it's the same failure everywhere.
>
> Does something forbid snprintf implementations from printing '45'::float8 as
> 45.0001 under extra_float_digits=3?

I'm not sure it's really worth having the test output something like
45.0001 since that extra detail doesn't really seem
particularly useful beyond the fact that the result wasn't exactly 45.
Also you'd have to be careful how you modified the test, since it's
possible that 45.0001 might be printed as '45' even under
extra_float_digits=3 and so there'd be a risk of the test passing when
it ought to fail, unless you also printed something else out to
indicate exactness.

Thinking about the actual failure in this case (asind(0.5) not
returning exactly 30) a couple of theories spring to mind. One is that
the compiler is being more aggressive over function inlining, so
init_degree_constants() is being inlined, and then asin_0_5 is being
evaluated at compile time rather than runtime, giving a slightly
different result, as happened in the original version of this patch.
Another theory is that the compiler is performing an unsafe ordering
rearrangement and transforming (asin(x) / asin_0_5) * 30.0 into
asin(x) * (30.0 / asin_0_5). This might happen for example under
something like -funsafe-math-optimizations.

I did a search for other people encountering similar problems and I
came across the following quite interesting example in the Gnu
Scientific Library's implementation of log1p() --
https://github.com/ampl/gsl/blob/master/sys/log1p.c. The reason the
code is now written in that way is in response to this bug:
https://lists.gnu.org/archive/html/bug-gsl/2007-07/msg8.html with
an overly aggressive compiler.

So using that technique, we might try using a volatile local variable
to enforce the desired evaluation order, e.g.:

volatile double tmp;

tmp = asin(x) / asin_0_5;
return tmp * 30.0;

A similar trick could be used to protect against
init_degree_constants() being inlined, by writing it as

volatile double one_half = 0.5;

asin_0_5 = asin(one_half);

since then the compiler wouldn't be able to assume that one_half was
still equal to 0.5, and wouldn't be able to optimise away the runtime
evaluation of asin() in favour of a compile-time constant.

These are both somewhat unconventional uses of volatile, but I think
they stand a better chance of being more portable, and also more
future-proof against compilers that might in the future make more
advanced code inlining and rearrangement choices.

Of course this is all pure speculation at this stage, but it seems
like it's worth a try.

Regards,
Dean


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

Re: [HACKERS] OOM in libpq and infinite loop with getCopyStart()

[Michael Paquier]

On Mon, Apr 18, 2016 at 4:48 PM, Michael Paquier
 wrote:
> Another, perhaps, better idea is to add some more extra logic to
> pg_conn as follows:
> boolis_emergency;
> PGresult *emergency_result;
> boolis_emergency_consumed;
> So as when an OOM shows up, is_emergency is set to true. Then a first
> call of PQgetResult returns the PGresult with the OOM in
> emergency_result, setting is_emergency_consumed to true and switching
> is_emergency back to false. Then a second call to PQgetResult enforces
> the consumption of any results remaining without waiting for them, at
> the end returning NULL to the caller, resetting is_emergency_consumed
> to false. That's different compared to the extra failure
> PGASYNC_COPY_FAILED in the fact that it does not break PQisBusy().

So, in terms of code, it gives more or less the attached. I have
coupled the emergency_result with an enum flag emergency_status that
has 3 states:
- NONE, which is the default
- ENABLE, which is when an OOM or other error has been found in libpq.
Making the next call of PQgetResult return the emergency_result
- CONSUMED, set after PQgetResult is consuming emergency_result, to
return to caller NULL so as it can get out of any PQgetResult loop
expecting a result at the end of. Once NULL is returned, the flag is
switched back to NONE.
This works in the case of getCopyStart because the OOM failures are
happening before any messages are sent to server.

The checks for the emergency result are done in PQgetResult, so as any
upper-level routine gets the message. Tom, others, what do you think?
-- 
Michael
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 2621767..c5d7efd 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -1689,6 +1689,30 @@ PQgetResult(PGconn *conn)
 	if (!conn)
 		return NULL;
 
+	if (!conn->emergency_result)
+	{
+		conn->emergency_result = PQmakeEmptyPGresult(conn, PGRES_FATAL_ERROR);
+		if (!conn->emergency_result)
+			return NULL;
+		conn->emergency_status = PGEMERGENCY_NONE;
+	}
+
+	/*
+	 * Check for any error that could have happened with the client.
+	 * Do this check before parsing any new commands.
+	 */
+	switch (conn->emergency_status)
+	{
+		case PGEMERGENCY_NONE:
+			break;
+		case PGEMERGENCY_ENABLE:
+			conn->emergency_status = PGEMERGENCY_CONSUMED;
+			return conn->emergency_result;
+		case PGEMERGENCY_CONSUMED:
+			conn->emergency_status = PGEMERGENCY_NONE;
+			return NULL;
+	}
+
 	/* Parse any available data, if our state permits. */
 	parseInput(conn);
 
@@ -1698,6 +1722,22 @@ PQgetResult(PGconn *conn)
 		int			flushResult;
 
 		/*
+		 * Check for any error that could have happened with
+		 * the client.
+		 */
+		switch (conn->emergency_status)
+		{
+			case PGEMERGENCY_NONE:
+break;
+			case PGEMERGENCY_ENABLE:
+conn->emergency_status = PGEMERGENCY_CONSUMED;
+return conn->emergency_result;
+			case PGEMERGENCY_CONSUMED:
+conn->emergency_status = PGEMERGENCY_NONE;
+return NULL;
+		}
+
+		/*
 		 * If data remains unsent, send it.  Else we might be waiting for the
 		 * result of a command the backend hasn't even got yet.
 		 */
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 0b8c62f..6d99d12 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1453,7 +1453,7 @@ getCopyStart(PGconn *conn, ExecStatusType copytype)
 
 	result = PQmakeEmptyPGresult(conn, copytype);
 	if (!result)
-		goto failure;
+		goto oom_failure;
 
 	if (pqGetc(&conn->copy_is_binary, conn))
 		goto failure;
@@ -1469,7 +1469,7 @@ getCopyStart(PGconn *conn, ExecStatusType copytype)
 		result->attDescs = (PGresAttDesc *)
 			pqResultAlloc(result, nfields * sizeof(PGresAttDesc), TRUE);
 		if (!result->attDescs)
-			goto failure;
+			goto oom_failure;
 		MemSet(result->attDescs, 0, nfields * sizeof(PGresAttDesc));
 	}
 
@@ -1492,6 +1492,10 @@ getCopyStart(PGconn *conn, ExecStatusType copytype)
 	conn->result = result;
 	return 0;
 
+oom_failure:
+	conn->emergency_status = PGEMERGENCY_ENABLE;
+	printfPQExpBuffer(&conn->errorMessage,
+	  libpq_gettext("out of memory\n"));
 failure:
 	PQclear(result);
 	return EOF;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 1183323..e9dd167 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -223,6 +223,19 @@ typedef enum
 	PGASYNC_COPY_BOTH			/* Copy In/Out data transfer in progress */
 } PGAsyncStatusType;
 
+/*
+ * PGemergencyState defines the consumption status of emergency_result
+ * for clients-side error handling, particularly out-of-memory errors
+ * that could happen.
+ */
+typedef enum
+{
+	PGEMERGENCY_NONE,			/* emergency result allocated, not needed */
+	PGEMERGENCY_ENABLE,			/* ready to be consumed by client */
+	PGEMERGENCY_CONSUMED		/* consumed by client, next call querying
+ * for a result will get NULL. */
+} PGemergencyState;
+
 /* PGQueryC