Re: [HACKERS] max_parallel_degree > 0 for 9.6 beta

[Amit Kapila]

On Fri, Apr 22, 2016 at 1:31 AM, Gavin Flower  wrote:

> On 22/04/16 06:07, Robert Haas wrote:
>
>> On Thu, Apr 21, 2016 at 1:48 PM, Tom Lane  wrote:
>>
>>> Robert Haas  writes:
>>>
 On Wed, Apr 20, 2016 at 2:28 PM, Tom Lane  wrote:

> Andres Freund  writes:
>
>> max_parallel_degree currently defaults to 0.  I think we should enable
>> it by default for at least the beta period. Otherwise we're primarily
>> going to get reports back after the release.
>>
> So, I suggest that the only sensible non-zero values here are probably
 "1" or "2", given a default pool of 8 worker processes system-wide.
 Andres told me yesterday he'd vote for "2".  Any other opinions?

>>> It has to be at least 2 for beta purposes, else you are not testing
>>> situations with more than one worker process at all, which would be
>>> rather a large omission no?
>>>
>> That's what Andres, thought, too.  From my point of view, the big
>> thing is to be using workers at all.  It is of course possible that
>> there could be some bugs where a single worker is not enough, but
>> there's a lot of types of bug where even one worker would probably
>> find the problem.  But I'm OK with changing the default to 2.
>>
>> I'm curious.
>
> Why not 4?


IIUC, the idea to change max_parallel_degree for beta is to catch any bugs
in parallelism code, not to do any performance testing of same.  So, I
think either 1 or 2 should be sufficient to hit the bugs if there are any.
Do you have any reason to think that we might miss some category of bugs if
we don't use higher max_parallel_degree?


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

Re: [HACKERS] Fix of doc for synchronous_standby_names.

[Kyotaro HORIGUCHI]

I came to think that both of you are misunderstanding how
synchronous standbys are choosed so I'd like to clarify the
behavior.

At Fri, 22 Apr 2016 11:09:28 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20160422.110928.18809311.horiguchi.kyot...@lab.ntt.co.jp>
> > > But this particular sentence seems to be talking
> > > about what's the case for any given slot.
> > 
> > Right, that's my reading also.

In SyncRepInitConfig, every walsender sets sync_standby_priority
by itself. The priority value is the index of its
application_name in s_s_names list (1 based).

When a walsender receives a feedback from walreceiver, it may
release backends waiting for certain LSN to be secured.

First, SyncRepGetSyncStandbys collects active standbys. Then it
loops from the hightest priority value to pick up all of the
active standbys for each priority value until all of the seats
are occupied. Then SyncRepOldestSyncRepPtr calculates the oldest
LSNs only among the standbys SyncRepGetSyncStandbys
returned. Finally, it releases backends using the LSNs.

In short, every 'slot' in s_s_names can corresponds to two or
more *synchronous* standbys.

The resulting behavior is as the following.

> I don't certainly understnd what the 'sync slot' means. If it
> means a name in a replication set description, that is, 'nameN'
> in the following setting of s_s_names.
> 
> '2(name1, name2, name3)'
> 
> There may be two or more duplicates even in the
> single-sync-age. But only one synchronous standby was allowed so
> any 'sync slot' may have at least one matching synchronous
> standby in the single-sync-age. This is what I see in the
> sentense. Is this wrong?
> 
> Now, we can have multiple synchronous standbys so, for example,
> if three standbys with the name 'name1', two of them are choosed
> as synchronous. This is a new behavior in the multi-sync-age and
> syncrep.c has been changed so as to do so.
> 
> For a supplemnet, the following case.
> 
> '5(name1, name2, name3)'
> 
> and the following standbys
> 
> (name1, name1, name2, name2, name3, name3)

This was a bad example,

(name1, name1, name2, name2, name2, name2, name3)

For this case, the followings are choosed as synchornous standby.

(name1, name1, name2, name2, name2)

Three of the four name2s are choosed but which name2s is an
implement matter.

> # However, 5 for three names causes a warning..


reegards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

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

[Michael Paquier]

On Fri, Apr 22, 2016 at 4:56 AM, Christian Ullrich  wrote:
> * From: Andrew Dunstan [mailto:and...@dunslane.net]
>
>> 4. The compiler complains about one of Microsoft's own header files -
>> essentially it dislikes the=is construct:
>>
>> typedef enum { ... };
>>
>> It would be nice to make it shut up about it.
>
> I doubt that's possible; the declaration *is* wrong after all. We could turn 
> off the warning:
>
> #pragma warning(push)
> #pragma warning(disable : 1234, or whatever the number is)
> #include 
> #pragma warning(pop)

Well, yes.. Even if that's not pretty that would not be the first one
caused by a VS header bug (float.c)..

>> 5. It also complains about us casting a pid_t to a HANDLE in
>> pg_basebackup.c. Not sure what to do about that.
>
> The thing that's being cast is not a PID, but a HANDLE to a process. pid_t is 
> a typedef for int (in port/win32.h), therefore is always 32 bits, while 
> HANDLE is actually void*. However, Microsoft guarantees that kernel32 HANDLEs 
> (this includes those to threads and processes) fit into 32 bits on AMD64.
>
> Source: 
> https://msdn.microsoft.com/en-us/library/windows/desktop/ee872017(v=vs.85).aspx,
>  third bullet point.
>
> So we can simply silence the warning by casting explicitly.

Yes, when casting things this way I think that a comment would be fine
in the code. We could do that as separate patches actually.
-- 
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] kqueue

[Andres Freund]

On 2016-04-21 14:25:06 -0400, Robert Haas wrote:
> On Thu, Apr 21, 2016 at 2:22 PM, Andres Freund  wrote:
> > On 2016-04-21 14:15:53 -0400, Robert Haas wrote:
> >> On Tue, Mar 29, 2016 at 7:53 PM, Thomas Munro
> >>  wrote:
> >> > On the WaitEventSet thread I posted a small patch to add kqueue
> >> > support[1].  Since then I peeked at how some other software[2]
> >> > interacts with kqueue and discovered that there are platforms
> >> > including NetBSD where kevent.udata is an intptr_t instead of a void
> >> > *.  Here's a version which should compile there.  Would any NetBSD
> >> > user be interested in testing this?  (An alternative would be to make
> >> > configure to test for this with some kind of AC_COMPILE_IFELSE
> >> > incantation but the steamroller cast is simpler.)
> >>
> >> Did you code this up blind or do you have a NetBSD machine yourself?
> >
> > RMT, what do you think, should we try to get this into 9.6? It's
> > feasible that the performance problem 98a64d0bd713c addressed is also
> > present on free/netbsd.
> 
> My personal opinion is that it would be a reasonable thing to do if
> somebody can demonstrate that it actually solves a real problem.
> Absent that, I don't think we should rush it in.

On linux you needed a 2 socket machine to demonstrate the problem, but
both old ones (my 2009 workstation) and new ones were sufficient. I'd be
surprised if the situation on freebsd is any better, except that you
might hit another scalability bottleneck earlier.

I doubt there's many real postgres instances operating on bigger
hardware on freebsd, with sufficient throughput to show the problem. So
I think the argument for including is more along trying to be "nice" to
more niche-y OSs.

I really don't have any opinion either way.

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

[Tom Lane]

Andrew Dunstan  writes:
> On 04/21/2016 05:15 PM, Tom Lane wrote:
>> Do the other contrib modules all pass?  I can't recall if seg was the
>> only one we'd left like this.

> Only seg fails.

As a crosscheck, I put some code into fmgr_c_validator() to log a message
when creating a V0 function with a pass-by-val return type.  (Pass-by-ref
is no problem, according to my hypothesis, since it necessarily means
the C function returns a pointer.)  I get these hits in core + contrib
regression tests:

core:
LOG:  version-0 function widget_in returns type widget
LOG:  version-0 function oldstyle_length returns type integer

contrib:
LOG:  version-0 function seg_over_left returns type boolean
LOG:  version-0 function seg_over_right returns type boolean
LOG:  version-0 function seg_left returns type boolean
LOG:  version-0 function seg_right returns type boolean
LOG:  version-0 function seg_lt returns type boolean
LOG:  version-0 function seg_le returns type boolean
LOG:  version-0 function seg_gt returns type boolean
LOG:  version-0 function seg_ge returns type boolean
LOG:  version-0 function seg_contains returns type boolean
LOG:  version-0 function seg_contained returns type boolean
LOG:  version-0 function seg_overlap returns type boolean
LOG:  version-0 function seg_same returns type boolean
LOG:  version-0 function seg_different returns type boolean
LOG:  version-0 function seg_cmp returns type integer
LOG:  version-0 function gseg_consistent returns type boolean
LOG:  version-0 function gseg_compress returns type internal
LOG:  version-0 function gseg_decompress returns type internal
LOG:  version-0 function gseg_penalty returns type internal
LOG:  version-0 function gseg_picksplit returns type internal
LOG:  version-0 function gseg_same returns type internal

The widget_in gripe is a false positive, caused by the fact that we don't
know the properties of type widget when widget_in is declared.  We also
need not worry about the functions that return "internal", since that's
defined to be pointer-sized.

If we assume that oldstyle functions returning integer are still okay,
which the success of the regression test case involving oldstyle_length()
seems to prove, then indeed seg's bool-returning functions are the only
hazard.

Note though that this test fails to cover any contrib modules that
lack regression tests, since they wouldn't have gotten loaded by
"make installcheck".

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

[Andrew Dunstan]

On 04/21/2016 05:15 PM, Tom Lane wrote:


IIRC, we had intentionally left contrib/seg using mostly V0 calling
convention as a canary to let us know when that broke.  It's a bit
astonishing that it lasted this long, maybe.  But it looks like we're
going to have to convert at least the bool-returning functions to V1
if we want it to work on VS 2015.

Do the other contrib modules all pass?  I can't recall if seg was the
only one we'd left like this.





Only seg fails.

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

[HACKERS] New 9.6 external sort guidance around temp_tablespaces and maintenance_work_mem

[Peter Geoghegan]

Postgres 9.6 saw the performance characteristics of sorting
significantly altered. While almost every external sort will see a
benefit without any change in configuration, there is a new scope for
DBAs to tune the system to better take advantage of the improved
implementation of external sorting. They should get guidance from us
on this to make it more likely that this actually happens, though.

I'd like to discuss where this guidance should be added, but first, a
little background.

Firstly, DBAs may get a significant, appreciable benefit from making
sure that temp_tablespaces puts temp files on a fast filesystem; this
would probably have been far less helpful prior to 9.6. It seems
reasonable to suppose that explicitly setting temp_tablespaces does
not happen all that often on production installations today, since
external sorts were stalled on memory access most of the time with the
old replacement selection approach. While memory stalls remain a big
cost in 9.6, the situation is much improved. External sorts may be
significantly I/O bound far more frequently in 9.6, especially during
merging (which is also optimized in 9.6), and especially during
merging for CREATE INDEX in particular. In the latter case, it's
likely that the system writes index tuples and WAL out as it reads
runs in, with everything on the same filesystem.

Modern filesystems are really good at read-ahead and write-behind. It
really is not at all unexpected for sequential I/O with fast *disks*
to be faster than random *memory* access [1], and so I really doubt
that there is any question that I/O will now matter more. I expect
that blocking on sequential I/O will become much more of a problem
when parallel sort is added, but it's still going to be a problem with
serial sorts on busy production systems servicing many clients. (With
only a single client, this is much less true, but that's seldom the
case.)

Secondly, with 9.6 it's roughly true that the more memory you can
spare for maintenance_work_mem and work_mem, the better. There are
some caveats that I won't rehash right now; the caveats are rather
limited, and DBAs can probably just pretend that the common, simple
intuition "more memory is better, until you OOM" is at last true. I
don't imagine more information (the caveats) is of much practical use.

A duality
=

Adding faster disks to get more sequential write bandwidth is a
practical measure available to many DBAs, which is more important for
sorting now. While not every user can afford to do so, it's nice that
some have the *option* of upgrading to get better performance. It need
not be expensive; we get plenty of benefit from cheaper SATA HDDs,
since, with care, only sequential I/O performance really matters. This
was not really the case in prior releases. At the same time, if you
can afford the memory, you should probably just increase
work_mem/maintenance_work_mem to get another performance benefit.
These two benefits will often be complementary. There may a feedback
loop that looks a bit like this:

Quicksort is cache oblivious, which implies that we may effectively
use more working memory, which implies longer runs, which implies
fewer merge passes, which implies that sequential I/O performance is
mostly where I/O costs are paid, which implies that you can manage I/O
costs by adding (consumer grade?) SATA HDDs configured in RAID0 for
temp files, which implies that sorts finish sooner, which implies that
in aggregate memory use is lower, which implies that you can afford to
set work_mem/maintenance_work_mem higher still, which implies ... .

This may seem facile, but consider the huge difference in costs I'm
focussing on. There is a huge difference between the cost of random
I/O and sequential I/O (when FS cache is not able to "amortize" the
random I/O), just as there is a huge difference between the cost of an
L1 cache access, and main memory access (a cache miss). So, if I can
be forgiven for using such a hand-wavey term, there is an interesting
duality here. We should avoid "the expensive variety" of I/O (random
I/O) wherever possible, and avoid "the expensive variety" of memory
access (involving a last-level CPU cache miss) as much as possible.
Certainly, the economics of modern hardware strongly support
increasing locality of access at every level. We're talking about
differences of perhaps several orders of magnitude.

I/O bound sorts in earlier releases
---

Perhaps someone has personally seen a DBA adding a new temp_tablespace
on a separate, faster filesystem. Perhaps this clearly improved
performance. I don't think that undermines my argument, though. This
*does* sometimes happen on prior versions of Postgres, but my guess is
that this is almost accidental:

* Increasing work_mem/maintenance_work_mem perversely makes external
sorts *slower* before 9.6. Iterative tuning of these settings on busy
production systems will therefore tend to guide the DBA to decrease

[HACKERS] Missing lookup in msvcr120 for pgwin32_putenv

[Michael Paquier]

Hi all,

While looking at e545281 I bumped into the following thing that has
visibly been forgotten since VS2013 support has been added:
--- a/src/port/win32env.c
+++ b/src/port/win32env.c
@@ -67,6 +67,9 @@ pgwin32_putenv(const char *envval)
"msvcr110", 0, NULL
},  /* Visual Studio 2012 */
{
+   "msvcr120", 0, NULL
+   },  /* Visual Studio 2013 */
+   {
NULL, 0, NULL
}
};

Attached is a patch. This should be backpatched to 9.3 where, if I
recall correctly, support for VS2013 has been added.
Regards,
-- 
Michael


vs-2013-putenv.patch
Description: application/download

-- 
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] Dead code in win32.h

[Michael Paquier]

On Fri, Apr 22, 2016 at 4:52 AM, Tom Lane  wrote:
> I wrote:
>> Perhaps I'm missing something, but if so, in what way is the
>> "#if _MSC_VER >= 1600" stanza not totally useless given the
>> immediately preceding macro redefinitions?
>
> Some rummaging in the git history says that that stanza did something
> when it was added (in 63876d3ba), but the later commit 73838b52 made
> it redundant by making the earlier set of definitions unconditional.
> Now it's merely confusing, so I'm going to take it out and repurpose
> the comment.

Going through that... That's indeed dead meat (found a bug on the way actually).
-- 
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] Should XLogInsert() be done only inside a critical section?

[Michael Paquier]

On Fri, Apr 22, 2016 at 5:18 AM, Alvaro Herrera
 wrote:
> Michael Paquier wrote:
>> On Thu, Apr 21, 2016 at 5:44 AM, Tom Lane  wrote:
>> > Anyway, I went through our tree and added START/END_CRIT_SECTION calls
>> > around all XLogInsert calls that could currently be reached without one;
>> > see attached.  Since this potentially breaks third-party code I would
>> > not propose back-patching it, but I think it's reasonable to propose
>> > applying it to HEAD.
>>
>> +1 for sanitizing those code paths this way. This patch looks sane to
>> me after having a look with some testing.
>>
>> --- a/src/backend/access/brin/brin.c
>> +++ b/src/backend/access/brin/brin.c
>> @@ -610,15 +610,12 @@ brinbuild(Relation heap, Relation index,
>> IndexInfo *indexInfo)
>> elog(ERROR, "index \"%s\" already contains data",
>>  RelationGetRelationName(index));
>>
>> -   /*
>> -* Critical section not required, because on error the creation of the
>> -* whole relation will be rolled back.
>> -*/
>> Perhaps Alvaro has a opinion to offer regarding this bit removed in brin.c?
>
> I vaguely recall copying this comment from elsewhere, but I didn't see
> any other such comment being removed by the patch; I probably copied
> something else which got slowly mutated into what's there today during
> development.
>
> if we're adding the critical section then the comment should
> certainly be removed too.

A scan of the code is showing me that there are 88 sections in the
code containing a comment referring to a critical section, actually a
little bit more because those two terms are sometimes broken between
two lines. With Tom's patch applied, I have found two inconsistencies.

In RecordTransactionAbort@xact.c, there is the following comment that
would need a refresh because XactLogAbortRecord logs a record:
/* XXX do we really need a critical section here? */
START_CRIT_SECTION();

The comment of RelationTruncate@storage.c referring to the use of
critical sections should be updated.

Looking at that the access code while going through the patch, perhaps
it would be good to add some assertions regarding the presence of a
critical section in some code paths of gin. For example
dataExecPlaceToPage and entryExecPlaceToPage should be invoked in a
critical section per their comments. heap_page_prune_execute,
spgPageIndexMultiDelete, spgFormDeadTuple could be as well candidates
for such changes. Surely that's a different, HEAD-only patch though.
-- 
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] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

On 2016/04/22 9:22, Michael Paquier wrote:

On Thu, Apr 21, 2016 at 11:53 PM, Robert Haas  wrote:

I have committed this patch.  Thanks for working on this.  Sorry for the delay.



Thanks for the push. open_item--;


Thank you, Robert and Michael.

Best regards,
Etsuro Fujita




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

Re: [HACKERS] Fix of doc for synchronous_standby_names.

[Kyotaro HORIGUCHI]

I'm not so confident on me but, please let me continue on this a
bit more for my understanding.

At Thu, 21 Apr 2016 13:33:23 -0400, Robert Haas  wrote 
in 
> On Thu, Apr 21, 2016 at 1:01 AM, Amit Langote
>  wrote:
> > ISTM, the sentence describes what happens in a *single instance* of
> > encountering duplicate (same name found in primary_conninfo of 2 or more
> > standbys).  It's still one name but which of the standbys claims the spot
> > (for that name) of being a synchronous standby with given priority is
> > indeterminate.

Yes. I read it as the same.

> > Now, there can be multiple instances of encountering duplicates,
> > each for a different sync slot. 

This is true from the past(call it single-sync-age). Not new for
multiple sync-standbys (call it multi-sync-age).

> > But this particular sentence seems to be talking
> > about what's the case for any given slot.
> 
> Right, that's my reading also.

I don't certainly understnd what the 'sync slot' means. If it
means a name in a replication set description, that is, 'nameN'
in the following setting of s_s_names.

'2(name1, name2, name3)'

There may be two or more duplicates even in the
single-sync-age. But only one synchronous standby was allowed so
any 'sync slot' may have at least one matching synchronous
standby in the single-sync-age. This is what I see in the
sentense. Is this wrong?

Now, we can have multiple synchronous standbys so, for example,
if three standbys with the name 'name1', two of them are choosed
as synchronous. This is a new behavior in the multi-sync-age and
syncrep.c has been changed so as to do so.

For a supplemnet, the following case.

'5(name1, name2, name3)'

and the following standbys

(name1, name1, name2, name2, name3, name3)

The following standbys are choosed as synchronous.

(name1, name1, name2, name2, name3)

# However, 5 for three names causes a warning..

Am I still wrong?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] pg_stat_activity crashes

[Kyotaro HORIGUCHI]

At Thu, 21 Apr 2016 14:05:28 -0400, Robert Haas  wrote 
in 
> >>> Here is PoC patch which fixes the problem. I am wondering if we should
> >>> raise warning in the pg_stat_get_backend_wait_event_type() and
> >>> pg_stat_get_backend_wait_event() like the pg_signal_backend() does
> >>> when proc is NULL instead of just returning NULL which is what this
> >>> patch does though.
> >>
> >> It still makes the two relevant columns in pg_stat_activity
> >> inconsistent each other since it reads the procarray entry twice
> >> without a lock on procarray.
> >>
> >> The attached patch adds pgstat_get_wait_event_info to read
> >> wait_event_info in more appropriate way. Then change
> >> pg_stat_get_wait_event(_type) to take the wait_event_info.
> >>
> >> Does this work for you?
> >
> > This is a hideously way of fixing this problem.  The whole point of
> > storing the wait event in a 4-byte integer is that we already assume
> > reads of 4 byte integers are atomic and thus no lock is needed.

A lock was already held in BackendPidGetProc(). Is it also
needless? If so, we should use BackendPidGetProcWithLock() instad
(the name seems a bit confusing, though).

What I did in the patch was just extending the lock duration
until reading the pointer proc. I didn't added any additional
lock.

> >  The
> > only thing we need to do is to prevent the value from being read
> > twice, and we already have precedent for how to prevent that in
> > freelist.c.

However, I don't have objections for the patch applied.


> That was intended to say "a hideously expensive way".

Thanks for the kind suppliment..

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] Disallow unique index on system columns

[Eric Ridge]

On Wed, Apr 20, 2016 at 9:24 PM Tom Lane  wrote:

> > SELECT FROM table WHERE my_func('table', ctid) ==> 'index condition'
>

> Um, why's the ctid important here, or perhaps more directly, what is
> it you're really trying to do?
>

This function is defined as my_func(regclass, tid) and simply returns the
tid value passed in.  The operator is defined as ==>(tid, text).

Behind the scenes, the AM is actually backed by Elasticsearch, and the
tuple's ctid value is used as the "_id" in ES.

When Postgres decides to plan a sequential scan (or filter) to answer WHERE
clause conditions that use the ==>(tid, text) operator the AM isn't
involved but I still need to use the remote Elasticsearch server to answer
that condition.

So I came up with this "creative" approach to provide enough context in the
query plan for me to figure out a) which table is being used and b) which
physical row is being evaluated in the seqscan or filter.

When the operator's procedure is called, I notice that it's the first time
I've seen the FuncExpr on the LHS, go query Elasticsearch with the text
query from the RHS, build a hashtable of the matching ctids and lookup the
LHS's value in the hashtable.  If it exists, the row matches.

There just didn't seem to be enough context in the FunctionCallInfo of the
the operator's procedure to figure this out without something in the query
that's basically statically determined at parse time.

I suspect what I should be doing for this particular problem is taking
advantage of the Custom Scan API, but I'm trying to support PG 9.3.

We weren't planning to do that.
>

Great!

eric

Re: [HACKERS] more parallel query documentation

[Amit Langote]

On 2016/04/15 12:02, Robert Haas wrote:
> As previously threatened, I have written some user documentation for
> parallel query.  I put it up here:
> 
> https://wiki.postgresql.org/wiki/Parallel_Query
> 
> This is not totally comprehensive and I can think of a few more
> details that could be added, but it's a pretty good high-level
> overview of what got into 9.6.  After this has gotten some feedback
> and polishing, I would like to add a version of this to the SGML
> documentation somewhere.  I am not sure where it would fit, but I
> think it's good to document stuff like this.

Looking at the "Function Labeling For Parallel Safety" section.  There is
a sentence:

"Functions must be marked PARALLEL UNSAFE if they write to the database,
access sequences, change the transaction state even temporarily (e.g. a
PL/pgsql function which establishes an EXCEPTION block to catch errors),
or make persistent changes to settings."

Then looking at the "postgres_fdw vs. force_parallel_mode on ppc" thread
[1], I wonder if a note on the lines of "or a function that creates *new*
connection(s) to remote server(s)" may be in order.  Overkill?

Thanks,
Amit

[1]
http://www.postgresql.org/message-id/CAEepm=1_saV7WJQWqgZfeNL954=nhtvcaoyuu6fxet01rm2...@mail.gmail.com




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

Re: [HACKERS] EXPLAIN VERBOSE with parallel Aggregate

[David Rowley]

On 21 April 2016 at 17:14, Tom Lane  wrote:
> David Rowley  writes:
>> I'd like to admit that I'm a bit confused as to why
>> generate_function_name(), when it already knows the funcid, bothers to
>> call func_get_detail(), which performs a search for the function based
>> on the name and argument types, to find the function, most likely with
>> the same funcid as the one which it already knew.
>
>> Could you explain why this has to happen?
>
> The point is exactly to find out whether a search for the function
> given only the name and argument types would find the same function,
> or a similar function in a different schema --- which would happen
> if that other schema is earlier in the search path than the desired
> one, or maybe the desired one isn't in search_path at all.  In such
> a case we must schema-qualify the function name in the printed
> expression.

Thanks. That makes more sense now.

> To some extent this is because ruleutils serves two masters.  We would
> probably not care that much about schema exactness for EXPLAIN's purposes,
> but we do care for dumping views and rules.

OK, so here's my thoughts. Currently, as mentioned above, I've
included a PARTIAL prefix for partial aggregates. This is
syntactically incorrect for the dumping of views etc, but that should
not matter as partial Aggrefs never come from the parser, they're only
perhaps generated later in the planner. Same goes for combine
aggregates too.

In the attached I'm proposing that we simply just use the
pg_proc.proname which belongs to the aggref->aggfnoid for combine
aggregates. This gets around the function not being found by
generate_function_name() and the namespace problem, that code should
never be executed when getting a view def, since we can't have combine
aggs there.

The attached still does not get the output into the way Robert would
have liked, but I still stand by my dislike to pretending the combine
aggregate is a normal aggregate. It's not all that clear if FILTER
should be displayed in the combine agg. Combine Aggs don't do FILTER.

This makes the output:

postgres=# explain verbose select avg(num) FILTER (WHERE num >
0),sum(num),count(*) from i;
QUERY PLAN
---
 Finalize Aggregate  (cost=13758.56..13758.57 rows=1 width=48)
   Output: avg((PARTIAL avg(num) FILTER (WHERE (num > 0,
sum((sum(num))), count(*)
   ->  Gather  (cost=13758.33..13758.54 rows=2 width=48)
 Output: (PARTIAL avg(num) FILTER (WHERE (num > 0))),
(sum(num)), (count(*))
 Workers Planned: 2
 ->  Partial Aggregate  (cost=12758.33..12758.34 rows=1 width=48)
   Output: PARTIAL avg(num) FILTER (WHERE (num > 0)),
sum(num), count(*)
   ->  Parallel Seq Scan on public.i  (cost=0.00..8591.67
rows=416667 width=4)
 Output: num

Comments welcome.


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


parallel_agg_explain_verbose_v3.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] Optimization for updating foreign tables in Postgres FDW

[Michael Paquier]

On Thu, Apr 21, 2016 at 11:53 PM, Robert Haas  wrote:
> On Thu, Apr 21, 2016 at 8:44 AM, Michael Paquier
>  wrote:
>> On Thu, Apr 21, 2016 at 5:22 PM, Etsuro Fujita
>>  wrote:
>>> Attached is an updated version of the patch, which modified Michael's
>>> version of the patch, as I proposed in [1] (see "Other changes:").  I
>>> modified comments for pgfdw_get_result/pgfdw_exec_query also, mainly because
>>> words like "non-blocking mode" there seems confusing (note that we have
>>> PQsetnonbloking).
>>
>> OK, so that is what I sent except that the comments mentioning PG_TRY
>> are moved to their correct places. That's fine for me. Thanks for
>> gathering everything in a single patch and correcting it.
>
> I have committed this patch.  Thanks for working on this.  Sorry for the 
> delay.

Thanks for the push. open_item--;
-- 
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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Tom Lane]

Peter Eisentraut  writes:
> On 04/19/2016 04:48 PM, Tom Lane wrote:
>> Pushed.  Peter, what results do you get from these tests on your
>> problematic machine?

> acosd(x),
> acosd(x) IN (0,60,90,120,180) AS acosd_exact
>  FROM (VALUES (-1), (-0.5), (0), (0.5), (1)) AS t(x);
> -  x   | asind | asind_exact | acosd | acosd_exact
> ---+---+-+---+-
> -   -1 |   -90 | t   |   180 | t
> - -0.5 |   -30 | t   |   120 | t
> -0 | 0 | t   |90 | t
> -  0.5 |30 | t   |60 | t
> -1 |90 | t   | 0 | t
> +  x   |asind | asind_exact | acosd | acosd_exact
> +--+--+-+---+-
> +   -1 |  -90 | t   |   180 | t
> + -0.5 | -29.9964 | f   |   120 | t
> +0 |0 | t   |90 | t
> +  0.5 |  29.9964 | f   |60 | t
> +1 |   90 | t   | 0 | t
>  (5 rows)

> This is the same under the default -O2 and under -O0.

Hm.  This seems to prove that we're not getting exactly 1.0 from
(asin(x) / asin_0_5) with x = 0.5, but I'm having a hard time guessing
why that might be so when all the other cases work.

Could you send along the assembler code generated by the compiler (-S
output) for float.c?  Maybe that would shed some light.  Probably the
-O0 version would be easier to read.

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] max_parallel_degree > 0 for 9.6 beta

[Tom Lane]

Robert Haas  writes:
> On Thu, Apr 21, 2016 at 4:01 PM, Gavin Flower
>  wrote:
>> Why not 4?  As most processors now have at least 4 physical cores, & surely
>> it be more likely to flush out race conditions.

> Because if we did that, then it's extremely likely that people would
> end up writing queries that are faster only if workers are present,
> and then not get any workers.

Is that because max_worker_processes is only 8 by default?  Maybe we
need to raise that, at least for beta purposes?

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] max_parallel_degree > 0 for 9.6 beta

[Robert Haas]

On Thu, Apr 21, 2016 at 4:01 PM, Gavin Flower
 wrote:
> On 22/04/16 06:07, Robert Haas wrote:
>>
>> On Thu, Apr 21, 2016 at 1:48 PM, Tom Lane  wrote:
>>>
>>> Robert Haas  writes:

 On Wed, Apr 20, 2016 at 2:28 PM, Tom Lane  wrote:
>
> Andres Freund  writes:
>>
>> max_parallel_degree currently defaults to 0.  I think we should enable
>> it by default for at least the beta period. Otherwise we're primarily
>> going to get reports back after the release.

 So, I suggest that the only sensible non-zero values here are probably
 "1" or "2", given a default pool of 8 worker processes system-wide.
 Andres told me yesterday he'd vote for "2".  Any other opinions?
>>>
>>> It has to be at least 2 for beta purposes, else you are not testing
>>> situations with more than one worker process at all, which would be
>>> rather a large omission no?
>>
>> That's what Andres, thought, too.  From my point of view, the big
>> thing is to be using workers at all.  It is of course possible that
>> there could be some bugs where a single worker is not enough, but
>> there's a lot of types of bug where even one worker would probably
>> find the problem.  But I'm OK with changing the default to 2.
>>
> I'm curious.
>
> Why not 4?  As most processors now have at least 4 physical cores, & surely
> it be more likely to flush out race conditions.

Because if we did that, then it's extremely likely that people would
end up writing queries that are faster only if workers are present,
and then not get any workers.

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


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

[HACKERS] Incomplete description for \t in psql documentation - should mention caption

[David G. Johnston]

​​http://www.postgresql.org/docs/9.5/interactive/app-psql.html

"""
\t
Toggles the display of output column name headings and row count footer.
This command is equivalent to \pset tuples_only and is provided for
convenience.
"""

Experience says that a table caption (i.e., \C) is also suppressed when the
option is used.

The documentation should be changed though I'd argue that there is some
merit for still showing the caption.

The exact combination I wanted to try and make work was:
\C 'Table Caption'
\x \t
SELECT * FROM tbl LIMIT 1

The desired output would have been:

Table Caption
col1   | 1
col2   | 2
col3   | 3

This is also a bit mis-leading: the \t suppresses column name headings -
unless viewed in expanded mode...in which case it suppresses the "ROW#"
block heading but leave the column name headings (which just happen to be
placed in rows) in place.  This is indeed the desired behavior it just
isn't precisely documented.

So:

\t
Toggles display of the row count footer.  In normal mode also suppresses
the column name headings.  In expanded mode suppresses the Record# block
header instead.  Also suppresses any caption that is set for the table.

I'm not sure how willing someone is to work these mechanics out - or the
desire to make them conditional on expanded versus table mode.  My
immediate needs can be readily solved by adding an additional column to my
output so that "type | Record Type" is the first column of the expanded
output.  Or just live with the redundant "-[ RECORD 1 ]-" header.

David J.

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

[Tom Lane]

Andrew Dunstan  writes:
> 5. most importantly, on my Release/X64 build, I am getting a regression 
> failure on contrib/seg. regression diffs attached. Until that's fixed 
> this isn't going anywhere.

I'll bet a nickel that this means MSVC has broken the ABI rules that
allowed old-style (version 0) C functions to limp along without changes.
seg_same() and the other comparison functions that are misbehaving are
declared at the C level to return "bool".  I think what's happening is
they are now setting only one byte in the return register, or perhaps
only the low-order word, leaving the high-order bits as trash.  But
fmgr_oldstyle is coded as though V0 functions return "char *", and it's
going to be putting that whole pointer value into the result Datum, and
if the value isn't entirely zero then DatumGetBool will consider it
"true".  So this theory predicts a lot of intended "false" results will
read as "true", which is what your regression diffs show.

IIRC, we had intentionally left contrib/seg using mostly V0 calling
convention as a canary to let us know when that broke.  It's a bit
astonishing that it lasted this long, maybe.  But it looks like we're
going to have to convert at least the bool-returning functions to V1
if we want it to work on VS 2015.

Do the other contrib modules all pass?  I can't recall if seg was the
only one we'd left like this.

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

[Kevin Grittner]

On Thu, Apr 21, 2016 at 2:10 PM, Ants Aasma  wrote:
> On Thu, Apr 21, 2016 at 5:16 PM, Kevin Grittner  wrote:

>> Could you provide enough to make that a self-contained
>> reproducible test case [?]

> [provided]

Thanks!  I have your test case running, and it is not immediately
clear why old rows are not being vacuumed away.  Will investigate.

> I'm too tired right now to chase this down myself. The mental
> toll that two small kids can take is pretty staggering.

Been there, done that; so I know just what you mean.  :-)  It is
rewarding though, eh?

--
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] Should XLogInsert() be done only inside a critical section?

[Alvaro Herrera]

Michael Paquier wrote:
> On Thu, Apr 21, 2016 at 5:44 AM, Tom Lane  wrote:
> > Anyway, I went through our tree and added START/END_CRIT_SECTION calls
> > around all XLogInsert calls that could currently be reached without one;
> > see attached.  Since this potentially breaks third-party code I would
> > not propose back-patching it, but I think it's reasonable to propose
> > applying it to HEAD.
> 
> +1 for sanitizing those code paths this way. This patch looks sane to
> me after having a look with some testing.
> 
> --- a/src/backend/access/brin/brin.c
> +++ b/src/backend/access/brin/brin.c
> @@ -610,15 +610,12 @@ brinbuild(Relation heap, Relation index,
> IndexInfo *indexInfo)
> elog(ERROR, "index \"%s\" already contains data",
>  RelationGetRelationName(index));
> 
> -   /*
> -* Critical section not required, because on error the creation of the
> -* whole relation will be rolled back.
> -*/
> Perhaps Alvaro has a opinion to offer regarding this bit removed in brin.c?

I vaguely recall copying this comment from elsewhere, but I didn't see
any other such comment being removed by the patch; I probably copied
something else which got slowly mutated into what's there today during
development.

if we're adding the critical section then the comment should
certainly be removed too.  

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


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

Re: [HACKERS] max_parallel_degree > 0 for 9.6 beta

[Gavin Flower]

On 22/04/16 06:07, Robert Haas wrote:

On Thu, Apr 21, 2016 at 1:48 PM, Tom Lane  wrote:

Robert Haas  writes:

On Wed, Apr 20, 2016 at 2:28 PM, Tom Lane  wrote:

Andres Freund  writes:

max_parallel_degree currently defaults to 0.  I think we should enable
it by default for at least the beta period. Otherwise we're primarily
going to get reports back after the release.

So, I suggest that the only sensible non-zero values here are probably
"1" or "2", given a default pool of 8 worker processes system-wide.
Andres told me yesterday he'd vote for "2".  Any other opinions?

It has to be at least 2 for beta purposes, else you are not testing
situations with more than one worker process at all, which would be
rather a large omission no?

That's what Andres, thought, too.  From my point of view, the big
thing is to be using workers at all.  It is of course possible that
there could be some bugs where a single worker is not enough, but
there's a lot of types of bug where even one worker would probably
find the problem.  But I'm OK with changing the default to 2.


I'm curious.

Why not 4?  As most processors now have at least 4 physical cores, & 
surely it be more likely to flush out race conditions.



Cheers,
Gavin



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

* From: Andrew Dunstan [mailto:and...@dunslane.net]

> 4. The compiler complains about one of Microsoft's own header files -
> essentially it dislikes the=is construct:
> 
> typedef enum { ... };
> 
> It would be nice to make it shut up about it.

I doubt that's possible; the declaration *is* wrong after all. We could turn 
off the warning:

#pragma warning(push)
#pragma warning(disable : 1234, or whatever the number is)
#include 
#pragma warning(pop)

> 5. It also complains about us casting a pid_t to a HANDLE in
> pg_basebackup.c. Not sure what to do about that.

The thing that's being cast is not a PID, but a HANDLE to a process. pid_t is a 
typedef for int (in port/win32.h), therefore is always 32 bits, while HANDLE is 
actually void*. However, Microsoft guarantees that kernel32 HANDLEs (this 
includes those to threads and processes) fit into 32 bits on AMD64.

Source: 
https://msdn.microsoft.com/en-us/library/windows/desktop/ee872017(v=vs.85).aspx,
 third bullet point.

So we can simply silence the warning by casting explicitly.

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

[Robert Haas]

On Thu, Apr 21, 2016 at 3:31 PM, Alvaro Herrera
 wrote:
> Robert Haas wrote:
>> On Thu, Apr 21, 2016 at 2:22 PM, Andres Freund  wrote:
>> > On 2016-04-21 14:15:53 -0400, Robert Haas wrote:
>> >> On Tue, Mar 29, 2016 at 7:53 PM, Thomas Munro
>> >>  wrote:
>> >> > On the WaitEventSet thread I posted a small patch to add kqueue
>> >> > support[1].  Since then I peeked at how some other software[2]
>> >> > interacts with kqueue and discovered that there are platforms
>> >> > including NetBSD where kevent.udata is an intptr_t instead of a void
>> >> > *.  Here's a version which should compile there.  Would any NetBSD
>> >> > user be interested in testing this?  (An alternative would be to make
>> >> > configure to test for this with some kind of AC_COMPILE_IFELSE
>> >> > incantation but the steamroller cast is simpler.)
>> >>
>> >> Did you code this up blind or do you have a NetBSD machine yourself?
>> >
>> > RMT, what do you think, should we try to get this into 9.6? It's
>> > feasible that the performance problem 98a64d0bd713c addressed is also
>> > present on free/netbsd.
>>
>> My personal opinion is that it would be a reasonable thing to do if
>> somebody can demonstrate that it actually solves a real problem.
>> Absent that, I don't think we should rush it in.
>
> My first question is whether there are platforms that use kqueue on
> which the WaitEventSet stuff proves to be a bottleneck.  I vaguely
> recall that MacOS X in particular doesn't scale terribly well for other
> reasons, and I don't know if anybody runs *BSD in large machines.
>
> On the other hand, there's plenty of hackers running their laptops on
> MacOS X these days, so presumably any platform dependent problem would
> be discovered quickly enough.  As for NetBSD, it seems mostly a fringe
> platform, doesn't it?  We would discover serious dependency problems
> quickly enough on the buildfarm ... except that the only netbsd
> buildfarm member hasn't reported in over two weeks.
>
> Am I mistaken in any of these points?
>
> (Our coverage of the BSD platforms leaves much to be desired FWIW.)

My impression is that the Linux problem only manifested itself on
large machines.  I might be wrong about that.  But if that's true,
then we might not see regressions on other platforms just because
people aren't running those operating systems on big enough hardware.

-- 
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] Dead code in win32.h

[Tom Lane]

I wrote:
> Perhaps I'm missing something, but if so, in what way is the
> "#if _MSC_VER >= 1600" stanza not totally useless given the
> immediately preceding macro redefinitions?

Some rummaging in the git history says that that stanza did something
when it was added (in 63876d3ba), but the later commit 73838b52 made
it redundant by making the earlier set of definitions unconditional.
Now it's merely confusing, so I'm going to take it out and repurpose
the comment.

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

[Peter Eisentraut]

On 04/19/2016 04:48 PM, Tom Lane wrote:
> Pushed.  Peter, what results do you get from these tests on your
> problematic machine?

acosd(x),
acosd(x) IN (0,60,90,120,180) AS acosd_exact
 FROM (VALUES (-1), (-0.5), (0), (0.5), (1)) AS t(x);
-  x   | asind | asind_exact | acosd | acosd_exact
---+---+-+---+-
-   -1 |   -90 | t   |   180 | t
- -0.5 |   -30 | t   |   120 | t
-0 | 0 | t   |90 | t
-  0.5 |30 | t   |60 | t
-1 |90 | t   | 0 | t
+  x   |asind | asind_exact | acosd | acosd_exact
+--+--+-+---+-
+   -1 |  -90 | t   |   180 | t
+ -0.5 | -29.9964 | f   |   120 | t
+0 |0 | t   |90 | t
+  0.5 |  29.9964 | f   |60 | t
+1 |   90 | t   | 0 | t
 (5 rows)

This is the same under the default -O2 and under -O0.



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

[Andrew Dunstan]

On 04/11/2016 03:47 AM, 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...





I think I prefer the less hacky solution.

Progress report:

1. My VS 2015 installations (I now have several) all generate solution 
file with:


   Microsoft Visual Studio Solution File, Format Version 12.00

so I propose to set that as the solution file version.

2. The logic in win32.h is a bit convoluted. I'm going to simplify it.

3. I'm going to define _WINSOCK_DEPRECATED_NO_WARNINGS to silence some 
compiler bleatings about the way we use the Winsock API


4. The compiler complains about one of Microsoft's own header files - 
essentially it dislikes the=is construct:


   typedef enum { ... };

It would be nice to make it shut up about it.

5. It also complains about us casting a pid_t to a HANDLE in 
pg_basebackup.c. Not sure what to do about that.


5. most importantly, on my Release/X64 build, I am getting a regression 
failure on contrib/seg. regression diffs attached. Until that's fixed 
this isn't going anywhere.


cheers

andrew


*** C:/prog/postgresql/contrib/seg/expected/seg.out Wed Apr  6 12:18:10 2016
--- C:/prog/postgresql/contrib/seg/results/seg.out  Thu Apr 21 11:34:26 2016
***
*** 444,450 
  SELECT '24 .. 33.20'::seg = '24 .. 33.21'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '24 .. 33.20'::seg != '24 .. 33.20'::seg AS bool;
--- 444,450 
  SELECT '24 .. 33.20'::seg = '24 .. 33.21'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '24 .. 33.20'::seg != '24 .. 33.20'::seg AS bool;
***
*** 556,562 
  SELECT '1'::seg &< '0'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '1'::seg &< '1'::seg AS bool;
--- 556,562 
  SELECT '1'::seg &< '0'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '1'::seg &< '1'::seg AS bool;
***
*** 574,580 
  SELECT '0 .. 1'::seg &< '0'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg &< '1'::seg AS bool;
--- 574,580 
  SELECT '0 .. 1'::seg &< '0'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '0 .. 1'::seg &< '1'::seg AS bool;
***
*** 592,598 
  SELECT '0 .. 1'::seg &< '0 .. 0.5'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg &< '0 .. 1'::seg AS bool;
--- 592,598 
  SELECT '0 .. 1'::seg &< '0 .. 0.5'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '0 .. 1'::seg &< '0 .. 1'::seg AS bool;
***
*** 624,630 
  SELECT '0'::seg &> '1'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '1'::seg &> '1'::seg AS bool;
--- 624,630 
  SELECT '0'::seg &> '1'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '1'::seg &> '1'::seg AS bool;
***
*** 692,704 
  SELECT '1'::seg << '0'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '1'::seg << '1'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '1'::seg << '2'::seg AS bool;
--- 692,704 
  SELECT '1'::seg << '0'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '1'::seg << '1'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '1'::seg << '2'::seg AS bool;
***
*** 710,722 
  SELECT '0 .. 1'::seg << '0'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '1'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '2'::seg AS bool;
--- 710,722 
  SELECT '0 .. 1'::seg << '0'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '0 .. 1'::seg << '1'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '0 .. 1'::seg << '2'::seg AS bool;
***
*** 728,752 
  SELECT '0 .. 1'::seg << '0 .. 0.5'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '0 .. 1'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '0 .. 2'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '1 .. 2'::seg AS bool;
   bool 
  --
!  f
  (1 row)
  
  SELECT '0 .. 1'::seg << '2 .. 3'::seg AS bool;
--- 728,752 
  SELECT '0 .. 1'::seg << '0 .. 0.5'::seg AS bool;
   bool 
  --
!  t
  (1 row)
  
  SELECT '0 .. 1'::seg << '0 .. 1'::seg AS bool;
   bool 
  

Re: [HACKERS] kqueue

[Alvaro Herrera]

Robert Haas wrote:
> On Thu, Apr 21, 2016 at 2:22 PM, Andres Freund  wrote:
> > On 2016-04-21 14:15:53 -0400, Robert Haas wrote:
> >> On Tue, Mar 29, 2016 at 7:53 PM, Thomas Munro
> >>  wrote:
> >> > On the WaitEventSet thread I posted a small patch to add kqueue
> >> > support[1].  Since then I peeked at how some other software[2]
> >> > interacts with kqueue and discovered that there are platforms
> >> > including NetBSD where kevent.udata is an intptr_t instead of a void
> >> > *.  Here's a version which should compile there.  Would any NetBSD
> >> > user be interested in testing this?  (An alternative would be to make
> >> > configure to test for this with some kind of AC_COMPILE_IFELSE
> >> > incantation but the steamroller cast is simpler.)
> >>
> >> Did you code this up blind or do you have a NetBSD machine yourself?
> >
> > RMT, what do you think, should we try to get this into 9.6? It's
> > feasible that the performance problem 98a64d0bd713c addressed is also
> > present on free/netbsd.
> 
> My personal opinion is that it would be a reasonable thing to do if
> somebody can demonstrate that it actually solves a real problem.
> Absent that, I don't think we should rush it in.

My first question is whether there are platforms that use kqueue on
which the WaitEventSet stuff proves to be a bottleneck.  I vaguely
recall that MacOS X in particular doesn't scale terribly well for other
reasons, and I don't know if anybody runs *BSD in large machines.

On the other hand, there's plenty of hackers running their laptops on
MacOS X these days, so presumably any platform dependent problem would
be discovered quickly enough.  As for NetBSD, it seems mostly a fringe
platform, doesn't it?  We would discover serious dependency problems
quickly enough on the buildfarm ... except that the only netbsd
buildfarm member hasn't reported in over two weeks.

Am I mistaken in any of these points?

(Our coverage of the BSD platforms leaves much to be desired FWIW.)

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

[HACKERS] Dead code in win32.h

[Tom Lane]

src/include/port/win32.h contains (starting around line 270):


/*
 * Supplement to .
 */
#undef EAGAIN
#undef EINTR
#define EINTR WSAEINTR
#define EAGAIN WSAEWOULDBLOCK
#undef EMSGSIZE
#define EMSGSIZE WSAEMSGSIZE
#undef EAFNOSUPPORT
#define EAFNOSUPPORT WSAEAFNOSUPPORT
#undef EWOULDBLOCK
#define EWOULDBLOCK WSAEWOULDBLOCK
#undef ECONNRESET
#define ECONNRESET WSAECONNRESET
#undef EINPROGRESS
#define EINPROGRESS WSAEINPROGRESS
#undef ENOBUFS
#define ENOBUFS WSAENOBUFS
#undef EPROTONOSUPPORT
#define EPROTONOSUPPORT WSAEPROTONOSUPPORT
#undef ECONNREFUSED
#define ECONNREFUSED WSAECONNREFUSED
#undef EBADFD
#define EBADFD WSAENOTSOCK
#undef EOPNOTSUPP
#define EOPNOTSUPP WSAEOPNOTSUPP

/*
 * For Microsoft Visual Studio 2010 and above we intentionally redefine
 * the regular Berkeley error constants and set them to the WSA constants.
 * Note that this will break if those constants are used for anything else
 * than Windows Sockets errors.
 */
#if _MSC_VER >= 1600
#pragma warning(disable:4005)
#define EMSGSIZE WSAEMSGSIZE
#define EAFNOSUPPORT WSAEAFNOSUPPORT
#define EWOULDBLOCK WSAEWOULDBLOCK
#define EPROTONOSUPPORT WSAEPROTONOSUPPORT
#define ECONNRESET WSAECONNRESET
#define EINPROGRESS WSAEINPROGRESS
#define ENOBUFS WSAENOBUFS
#define ECONNREFUSED WSAECONNREFUSED
#define EOPNOTSUPP WSAEOPNOTSUPP
#pragma warning(default:4005)
#endif


Perhaps I'm missing something, but if so, in what way is the
"#if _MSC_VER >= 1600" stanza not totally useless given the
immediately preceding macro redefinitions?  And doesn't the
comment above it apply just as much to the preceding definitions
(other than the claim that it only happens in newer MSVC versions)?

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

[Ants Aasma]

On Thu, Apr 21, 2016 at 5:16 PM, Kevin Grittner  wrote:
> On Wed, Apr 20, 2016 at 8:08 PM, Ants Aasma  wrote:
>
>> However, while checking out if my proof of concept patch actually
>> works I hit another issue. I couldn't get my test for the feature to
>> actually work. The test script I used is attached.
>
> Could you provide enough to make that a self-contained reproducible
> test case (i.e., that I don't need to infer or re-write any steps
> or guess how to call it)?  In previous cases people have given me
> where they felt that the feature wasn't working there have have
> been valid reasons for it to behave as it was (e.g., a transaction
> with a transaction ID and an xmin which prevented cleanup from
> advancing).  I'll be happy to look at your case and see whether
> it's another such case or some bug, but it seems a waste to reverse
> engineer or rewrite parts of the test case to do so.

Just to be sure I didn't have anything screwy in my build environment
I redid the test on a freshly installed Fedora 23 VM. Steps to
reproduce:

1. Build postgresql from git. I used ./configure --enable-debug
--enable-cassert --prefix=/home/ants/pg-master
2. Set up database:

cat << EOF > test-settings.conf
old_snapshot_threshold = 1min

logging_collector = on
log_directory = 'pg_log'
log_filename = 'postgresql.log'
log_line_prefix = '[%m] '
log_autovacuum_min_duration = 0
EOF

pg-master/bin/initdb data/
cat test-settings.conf >> data/postgresql.conf
pg-master/bin/pg_ctl -D data/ start
pg-master/bin/createdb

3. Install python-psycopg2 and get the test script from my earlier e-mail [1]
4. Run the test:

python test_oldsnapshot.py "host=/tmp"

5. Observe that the table keeps growing even after the old snapshot
threshold is exceeded and autovacuum has run. Autovacuum log shows 0
tuples removed.

Only the write workload has a xid assigned, the other two backends
only have snapshot held:

[ants@localhost ~]$ pg-master/bin/psql -c "SELECT application_name,
backend_xid, backend_xmin, NOW()-xact_start AS tx_age, state FROM
pg_stat_activity"
   application_name   | backend_xid | backend_xmin | tx_age  |
   state
--+-+--+-+-
 write-workload   |   95637 |  | 00:00:00.009314 | active
 long-unrelated-query | | 1806 | 00:04:33.914048 | active
 interfering-query| | 2444 | 00:04:32.910742 |
idle in transaction
 psql | |95637 | 00:00:00| active

Output from the test tool attached. After killing the test tool and
the long running query autovacuum cleans stuff as expected.

I'm too tired right now to chase this down myself. The mental toll
that two small kids can take is pretty staggering. But I might find
the time to fire up a debugger sometime tomorrow.

Regards,
Ants Aasma

[1] http://www.postgresql.org/message-id/attachment/43859/test_oldsnapshot.py
[ants@localhost ~]$ python test_oldsnapshot.py "host=/tmp"
[21:37:56]  Starting 1800s long query
[21:37:56]  old_snapshot_threshold = 1min
[21:37:56]  High throughput table size @ 0s. Size176kB Last vacuum None 
ago
[21:37:57]  Counted 1000 rows with max 1637 in high_throughput table
[21:38:06]  High throughput table size @10s. Size952kB Last vacuum None 
ago
[21:38:07]  Counted 1000 rows with max 1637 in high_throughput table
[21:38:16]  High throughput table size @20s. Size   1768kB Last vacuum None 
ago
[21:38:17]  Counted 1000 rows with max 1637 in high_throughput table
[21:38:26]  High throughput table size @30s. Size   2640kB Last vacuum None 
ago
[21:38:27]  Counted 1000 rows with max 1637 in high_throughput table
[21:38:36]  High throughput table size @40s. Size   3488kB Last vacuum None 
ago
[21:38:37]  Counted 1000 rows with max 1637 in high_throughput table
[21:38:46]  High throughput table size @50s. Size   4328kB Last vacuum 
0:00:02.339652 ago
[21:38:47]  Counted 1000 rows with max 1637 in high_throughput table
[21:38:56]  High throughput table size @60s. Size   4832kB Last vacuum 
0:00:12.342278 ago
[21:38:57]  Counted 1000 rows with max 1637 in high_throughput table
[21:39:06]  High throughput table size @70s. Size   4920kB Last vacuum 
0:00:22.425250 ago
[21:39:07]  Counted 1000 rows with max 1637 in high_throughput table
[21:39:16]  High throughput table size @80s. Size   5016kB Last vacuum 
0:00:32.431971 ago
[21:39:17]  Counted 1000 rows with max 1637 in high_throughput table
[21:39:26]  High throughput table size @90s. Size   5112kB Last vacuum 
0:00:42.431730 ago
[21:39:27]  Counted 1000 rows with max 1637 in high_throughput table
[21:39:36]  High throughput table size @   100s. Size   5200kB Last vacuum 
0:00:52.448369 ago
[21:39:37]  Counted 1000 rows with max 1637 in high_throughput table
[21:39:46]  High throughput table size @   110s. Size   5672kB Last 

Re: [HACKERS] Wire protocol change

[Tom Lane]

Tatsuo Ishii  writes:
> If we are going to change the wire protocol, it would be nice if parse
> complete message includes an identification of the previous parse
> message. Same thing can be said to bind complete, command complete and
> close complete.

> The identification could be an either sequence number assigned to the
> parse message or just the statement name (IMO sequence numbers are
> better since duplicated statement names could be possible).

> Background: in extended protocol, a reply to a message sent to server
> could be asynchronously returned.

Uh, what?  There's no asynchrony in the protocol --- messages are
processed and answered strictly in order.  I grant that certain types
of application structures might find it convenient if we numbered the
responses, but it seems like mostly a waste of network bandwidth.
The application should be able to count the results for itself.

In any case, if the server simply numbers the responses, how does that
help?  In the example you show, the ParseComplete messages might come
back with numbers 42 and 43 --- exactly how does that help the app
associate them with the commands it sent?

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] [COMMITTERS] pgsql: Inline initial comparisons in TestForOldSnapshot()

[Tom Lane]

Kevin Grittner  writes:
> No joy with that shot in the dark.

> Should I add PGDLLEXPORT to the declaration of
> old_snapshot_threshold.  That doesn't seem to be documented
> anywhere, or have any useful comments; but it looks like it *might*
> be the arcane incantation needed here.

PGDLLIMPORT is what's needed on any backend global variable that's to
be referenced by extensions.  I already pushed a fix before noticing
this thread.

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

[Andres Freund]

On 2016-04-19 15:44:36 -0300, Alvaro Herrera wrote:
> 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?

I've postponed working on it a bit, as it was becoming clearer that 9.6
wasn't a realistic target. I do plan to pick this up again once we're in
beta.

Any specific reason for asking?

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

[Robert Haas]

On Thu, Apr 21, 2016 at 2:22 PM, Andres Freund  wrote:
> On 2016-04-21 14:15:53 -0400, Robert Haas wrote:
>> On Tue, Mar 29, 2016 at 7:53 PM, Thomas Munro
>>  wrote:
>> > On the WaitEventSet thread I posted a small patch to add kqueue
>> > support[1].  Since then I peeked at how some other software[2]
>> > interacts with kqueue and discovered that there are platforms
>> > including NetBSD where kevent.udata is an intptr_t instead of a void
>> > *.  Here's a version which should compile there.  Would any NetBSD
>> > user be interested in testing this?  (An alternative would be to make
>> > configure to test for this with some kind of AC_COMPILE_IFELSE
>> > incantation but the steamroller cast is simpler.)
>>
>> Did you code this up blind or do you have a NetBSD machine yourself?
>
> RMT, what do you think, should we try to get this into 9.6? It's
> feasible that the performance problem 98a64d0bd713c addressed is also
> present on free/netbsd.

My personal opinion is that it would be a reasonable thing to do if
somebody can demonstrate that it actually solves a real problem.
Absent that, I don't think we should rush it in.

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

[Andres Freund]

On 2016-04-21 14:15:53 -0400, Robert Haas wrote:
> On Tue, Mar 29, 2016 at 7:53 PM, Thomas Munro
>  wrote:
> > On the WaitEventSet thread I posted a small patch to add kqueue
> > support[1].  Since then I peeked at how some other software[2]
> > interacts with kqueue and discovered that there are platforms
> > including NetBSD where kevent.udata is an intptr_t instead of a void
> > *.  Here's a version which should compile there.  Would any NetBSD
> > user be interested in testing this?  (An alternative would be to make
> > configure to test for this with some kind of AC_COMPILE_IFELSE
> > incantation but the steamroller cast is simpler.)
> 
> Did you code this up blind or do you have a NetBSD machine yourself?

RMT, what do you think, should we try to get this into 9.6? It's
feasible that the performance problem 98a64d0bd713c addressed is also
present on free/netbsd.

- 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] checkpoint_flush_after documentation inconsistency

[Andres Freund]

On 2016-04-18 14:33:28 +0900, Fujii Masao wrote:
> On Fri, Apr 15, 2016 at 6:56 PM, Magnus Hagander  wrote:
> > The documentation says that the default value is 128Kb on Linux, but the
> > code says it's 256Kb.
> >
> > Not sure which one is correct, but the other one should be updated :) I'm
> > guessing it's a copy/paste mistake in the docs, but since I'm not sure I'm
> > not just pushing a fix.
> 
> I think you're right.
> 
> I also found another several small problems regarding XXX_flush_after
> parameters.

Thanks for finding these, once I'm back home/office from pgconf.nyc
(tonight / tomorrow) I'll push a fix.

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

[Robert Haas]

On Tue, Mar 29, 2016 at 7:53 PM, Thomas Munro
 wrote:
> On the WaitEventSet thread I posted a small patch to add kqueue
> support[1].  Since then I peeked at how some other software[2]
> interacts with kqueue and discovered that there are platforms
> including NetBSD where kevent.udata is an intptr_t instead of a void
> *.  Here's a version which should compile there.  Would any NetBSD
> user be interested in testing this?  (An alternative would be to make
> configure to test for this with some kind of AC_COMPILE_IFELSE
> incantation but the steamroller cast is simpler.)

Did you code this up blind or do you have a NetBSD machine yourself?

-- 
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] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Robert Haas]

On Thu, Apr 21, 2016 at 9:45 AM, Andres Freund  wrote:
>> My attempts to test things were also singularly unrewarding.  Even
>> after messing with the filesystem in various ways, I couldn't figure
>> out exactly how this makes a difference.
>
> What do you mean by this? You couldn't reproduce the bug? Or not how to
> trigger such a test?

Let's see.  I think what i did is made _mdfd_getseg() error out if it
got to a segment that was not the right size and preceded the one it
was trying to reach.  But I could not hit that error.

-- 
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] max_parallel_degree > 0 for 9.6 beta

[Robert Haas]

On Thu, Apr 21, 2016 at 1:48 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Apr 20, 2016 at 2:28 PM, Tom Lane  wrote:
>>> Andres Freund  writes:
 max_parallel_degree currently defaults to 0.  I think we should enable
 it by default for at least the beta period. Otherwise we're primarily
 going to get reports back after the release.
>
>> So, I suggest that the only sensible non-zero values here are probably
>> "1" or "2", given a default pool of 8 worker processes system-wide.
>> Andres told me yesterday he'd vote for "2".  Any other opinions?
>
> It has to be at least 2 for beta purposes, else you are not testing
> situations with more than one worker process at all, which would be
> rather a large omission no?

That's what Andres, thought, too.  From my point of view, the big
thing is to be using workers at all.  It is of course possible that
there could be some bugs where a single worker is not enough, but
there's a lot of types of bug where even one worker would probably
find the problem.  But I'm OK with changing the default to 2.

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


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

Re: [HACKERS] pg_stat_activity crashes

[Robert Haas]

On Thu, Apr 21, 2016 at 2:04 PM, Robert Haas  wrote:
> On Wed, Apr 20, 2016 at 10:56 PM, Kyotaro HORIGUCHI
>  wrote:
>> At Wed, 20 Apr 2016 15:14:16 +0200, Petr Jelinek  
>> wrote in <571780a8.4070...@2ndquadrant.com>
>>> I noticed sporadic segfaults when selecting from pg_stat_activity on
>>> current HEAD.
>>>
>>> The culprit is the 53be0b1add7064ca5db3cd884302dfc3268d884e commit
>>> which added more wait info into the pg_stat_get_activity(). More
>>> specifically, the following code is broken:
>>>
>>> +   proc = BackendPidGetProc(beentry->st_procpid);
>>> + wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info);
>>>
>>> This needs to check if proc is NULL. When reading the code I noticed
>>> that the new functions pg_stat_get_backend_wait_event_type() and
>>> pg_stat_get_backend_wait_event() suffer from the same problem.
>>
>> Good catch.
>
> Agreed.
>
>>> Here is PoC patch which fixes the problem. I am wondering if we should
>>> raise warning in the pg_stat_get_backend_wait_event_type() and
>>> pg_stat_get_backend_wait_event() like the pg_signal_backend() does
>>> when proc is NULL instead of just returning NULL which is what this
>>> patch does though.
>>
>> It still makes the two relevant columns in pg_stat_activity
>> inconsistent each other since it reads the procarray entry twice
>> without a lock on procarray.
>>
>> The attached patch adds pgstat_get_wait_event_info to read
>> wait_event_info in more appropriate way. Then change
>> pg_stat_get_wait_event(_type) to take the wait_event_info.
>>
>> Does this work for you?
>
> This is a hideously way of fixing this problem.  The whole point of
> storing the wait event in a 4-byte integer is that we already assume
> reads of 4 byte integers are atomic and thus no lock is needed.  The
> only thing we need to do is to prevent the value from being read
> twice, and we already have precedent for how to prevent that in
> freelist.c.

That was intended to say "a hideously expensive way".

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


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

Re: [HACKERS] pg_stat_activity crashes

[Robert Haas]

On Wed, Apr 20, 2016 at 10:56 PM, Kyotaro HORIGUCHI
 wrote:
> At Wed, 20 Apr 2016 15:14:16 +0200, Petr Jelinek  wrote 
> in <571780a8.4070...@2ndquadrant.com>
>> I noticed sporadic segfaults when selecting from pg_stat_activity on
>> current HEAD.
>>
>> The culprit is the 53be0b1add7064ca5db3cd884302dfc3268d884e commit
>> which added more wait info into the pg_stat_get_activity(). More
>> specifically, the following code is broken:
>>
>> +   proc = BackendPidGetProc(beentry->st_procpid);
>> + wait_event_type = pgstat_get_wait_event_type(proc->wait_event_info);
>>
>> This needs to check if proc is NULL. When reading the code I noticed
>> that the new functions pg_stat_get_backend_wait_event_type() and
>> pg_stat_get_backend_wait_event() suffer from the same problem.
>
> Good catch.

Agreed.

>> Here is PoC patch which fixes the problem. I am wondering if we should
>> raise warning in the pg_stat_get_backend_wait_event_type() and
>> pg_stat_get_backend_wait_event() like the pg_signal_backend() does
>> when proc is NULL instead of just returning NULL which is what this
>> patch does though.
>
> It still makes the two relevant columns in pg_stat_activity
> inconsistent each other since it reads the procarray entry twice
> without a lock on procarray.
>
> The attached patch adds pgstat_get_wait_event_info to read
> wait_event_info in more appropriate way. Then change
> pg_stat_get_wait_event(_type) to take the wait_event_info.
>
> Does this work for you?

This is a hideously way of fixing this problem.  The whole point of
storing the wait event in a 4-byte integer is that we already assume
reads of 4 byte integers are atomic and thus no lock is needed.  The
only thing we need to do is to prevent the value from being read
twice, and we already have precedent for how to prevent that in
freelist.c.

I've committed a modified version of Petr's patch which I think should
be sufficient.  It survived this test on my machine:

pgbench -n -f f -c 64 -j 64 -T 120

Where f contained:

SELECT * FROM pg_stat_activity;

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Inline initial comparisons in TestForOldSnapshot()

[Kevin Grittner]

On Thu, Apr 21, 2016 at 11:58 AM, Kevin Grittner  wrote:
> On Thu, Apr 21, 2016 at 9:58 AM, Kevin Grittner  wrote:
>> On Thu, Apr 21, 2016 at 8:47 AM, Kevin Grittner  
>> wrote:
>>> Inline initial comparisons in TestForOldSnapshot()
>>
>> This seems to have broken Windows builds.  Is there something
>> special that needs to be done if a function referenced from a
>> contrib module changes to an inline function?
>
> I've attempted to fix this by adding an include to blscan.c so that
> it can find old_snapshot_threshold.  The fact that this was only
> failing in a contrib module on Windows builds has me suspicious
> that there is some other way this would be better fixed, but I have
> no clue what that would be.  We'll see whether this brings things
> green again.

No joy with that shot in the dark.

Should I add PGDLLEXPORT to the declaration of
old_snapshot_threshold.  That doesn't seem to be documented
anywhere, or have any useful comments; but it looks like it *might*
be the arcane incantation needed here.

--
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] max_parallel_degree > 0 for 9.6 beta

[Tom Lane]

Robert Haas  writes:
> On Wed, Apr 20, 2016 at 2:28 PM, Tom Lane  wrote:
>> Andres Freund  writes:
>>> max_parallel_degree currently defaults to 0.  I think we should enable
>>> it by default for at least the beta period. Otherwise we're primarily
>>> going to get reports back after the release.

> So, I suggest that the only sensible non-zero values here are probably
> "1" or "2", given a default pool of 8 worker processes system-wide.
> Andres told me yesterday he'd vote for "2".  Any other opinions?

It has to be at least 2 for beta purposes, else you are not testing
situations with more than one worker process at all, which would be
rather a large omission no?

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] [BUGS] Breakage with VACUUM ANALYSE + partitions

[Andres Freund]

On 2016-04-15 13:42:34 -0400, Robert Haas wrote:
> On Thu, Apr 14, 2016 at 1:39 AM, Abhijit Menon-Sen  
> wrote:
> > At 2016-04-12 09:00:57 -0400, robertmh...@gmail.com wrote:
> >>
> >> On Mon, Apr 11, 2016 at 1:17 PM, Andres Freund  wrote:
> >> >
> >> > 3) Actually handle the case of the last open segment not being
> >> >RELSEG_SIZE properly in _mdfd_getseg() - mdnblocks() does so.
> >>
> >> #3 seems like it's probably about 15 years overdue, so let's do that
> >> anyway.
> >
> > Do I understand correctly that the case of the "last open segment"
> > (i.e., the one for which mdfd_chain == NULL) not being RELSEG_SIZE
> > (i.e., _mdnblocks(reln, forknum, v) < RELSEG_SIZE) should not call
> > _mfdf_openseg on nextsegno if behaviour is not EXTENSION_CREATE or
> > InRecovery?
> >
> > And that "We won't create segment if not existent" should happen, but
> > doesn't only because the next segment file wasn't removed earlier, so
> > we have to add an extra check for that case?
> >
> > In other words, is something like the following what's needed here, or
> > is there more to it?

Yes, I think that should solve the bug reported here. Did you check?

What makes me rather worried about solely using this as a solution right
now is the InRecovery check, which triggers segment extensions/creations
even with EXTENSION_RETURN_NULL.  Afaics it's more or less a dormant bug
that _mdfd_getseg() ignores RETURN_NULL in the InRecovery case, because
mdopen() (and other places) do *not* behave that way!

> Something like that is what I was thinking about, but I notice it has
> the disadvantage of adding lseeks to cater to a shouldn't-happen
> condition.  Not sure if we should care.

I'm not particularly concerned about that, my gues sit'd barely impact
the number of lseeks, because most callers will have called mdnblocks()
before anyway.


> My attempts to test things were also singularly unrewarding.  Even
> after messing with the filesystem in various ways, I couldn't figure
> out exactly how this makes a difference.

What do you mean by this? You couldn't reproduce the bug? Or not how to
trigger such a test?


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] Description of ForeignPath

[Amit Langote]

On Fri, Apr 22, 2016 at 2:32 AM, Robert Haas  wrote:
> On Wed, Apr 20, 2016 at 3:37 AM, Amit Langote wrote:
>>> I think it'd be better to match the comment with that for
>>> create_foreignscan_path().  So how about "ForeignPath represents a
>>> potential scan of a foreign table, foreign join, or foreign upper-relation
>>> processing"?  I think we would probably need to update the comment in
>>> src/backend/optimizer/README (L358), too.
>>
>> That's a lot better.  Updated patch attached.
>
> Looks OK to me, too.  Committed.

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] max_parallel_degree > 0 for 9.6 beta

[Robert Haas]

On Wed, Apr 20, 2016 at 2:28 PM, Tom Lane  wrote:
> Andres Freund  writes:
>> max_parallel_degree currently defaults to 0.  I think we should enable
>> it by default for at least the beta period. Otherwise we're primarily
>> going to get reports back after the release.
>
>> Then, at the end of beta, we can decide what the default should be.
>
> +1, but let's put an entry on the 9.6 open-items page to remind us to
> make that decision at the right time.

So, I suggest that the only sensible non-zero values here are probably
"1" or "2", given a default pool of 8 worker processes system-wide.
Andres told me yesterday he'd vote for "2".  Any other opinions?

-- 
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] Fix of doc for synchronous_standby_names.

[Robert Haas]

On Thu, Apr 21, 2016 at 1:01 AM, Amit Langote
 wrote:
> ISTM, the sentence describes what happens in a *single instance* of
> encountering duplicate (same name found in primary_conninfo of 2 or more
> standbys).  It's still one name but which of the standbys claims the spot
> (for that name) of being a synchronous standby with given priority is
> indeterminate.
>
> Now, there can be multiple instances of encountering duplicates, each for
> a different sync slot.  But this particular sentence seems to be talking
> about what's the case for any given slot.

Right, that's my reading also.

-- 
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] Description of ForeignPath

[Robert Haas]

On Wed, Apr 20, 2016 at 3:37 AM, Amit Langote
 wrote:
>> I think it'd be better to match the comment with that for
>> create_foreignscan_path().  So how about "ForeignPath represents a
>> potential scan of a foreign table, foreign join, or foreign upper-relation
>> processing"?  I think we would probably need to update the comment in
>> src/backend/optimizer/README (L358), too.
>
> That's a lot better.  Updated patch attached.

Looks OK to me, too.  Committed.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Move each SLRU's lwlocks to a separate tranche.

[Robert Haas]

On Mon, Apr 11, 2016 at 7:31 AM, Andres Freund  wrote:
> In the patch attached to 
> http://www.postgresql.org/message-id/20160411051004.yvniqb2pkc7re...@alap3.anarazel.de
> I did this instead by fixing up the actual shared memory allocation,
> which seems a tiny bit cleaner.

I don't really find that cleaner, though I think it's just a matter of taste.

> But the asserts added there seem like a
> worthwhile addition to me.

I adopted a couple of those and committed this.

-- 
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] Defaults for replication/backup

[Robert Haas]

On Wed, Apr 20, 2016 at 2:04 PM, Magnus Hagander  wrote:
> So what more do we need to just get going with this? Given feature freeze
> we're perhaps too late to actually build the parameter feature for initdb,
> but we could still change the defaults (and then we could add such a
> parameter for next release).

I think we are far too close to beta1 to begin bikeshedding this.
Changing the defaults is not going to be uncontroversial, and it's not
something I think we should rush into.

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

[Robert Haas]

On Tue, Apr 19, 2016 at 10:05 AM, Andres Freund  wrote:
> 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.

An "ad hominem" attack means against the person rather than on the
topic of the issue, but I don't think Aleksander did that.  I'm not
sure why you think what he wrote was out of line.  It reads OK to me.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Inline initial comparisons in TestForOldSnapshot()

[Kevin Grittner]

On Thu, Apr 21, 2016 at 9:58 AM, Kevin Grittner  wrote:
> On Thu, Apr 21, 2016 at 8:47 AM, Kevin Grittner  
> wrote:
>> Inline initial comparisons in TestForOldSnapshot()
>
> This seems to have broken Windows builds.  Is there something
> special that needs to be done if a function referenced from a
> contrib module changes to an inline function?

I've attempted to fix this by adding an include to blscan.c so that
it can find old_snapshot_threshold.  The fact that this was only
failing in a contrib module on Windows builds has me suspicious
that there is some other way this would be better fixed, but I have
no clue what that would be.  We'll see whether this brings things
green again.

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


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

Re: [HACKERS] Query Procedures

[David G. Johnston]

On Thu, Apr 21, 2016 at 8:26 AM, Andrea Adami  wrote:

>
> Hello, i'm a developer from italy and i need to make a query to get the
> list of stored procedures with its signature.
> Basically I would like to get the same list pgAdmin shows under the node
> functions of the database tree on left panel. with the query : p.oid AS
> SELECT oid , p.proname AS name , p.proargtypes FROM pg_proc p I get the
> list of procedures with the column "proargtypes" that contains the list
> of parameters's type.
> Each digit of proargtypes column refers to the corresponding tuple in
> pg_type So far all is fine, the problems arise when i want convert these
> numbers to strings as the previously mentioned pgAdmin did: 1 ) in typname
> column (of pg_type table) some time, I did not understand why , there is a
> space in front of the name , better: a space is displayed but there is not
> a space because a trim do not throw away . 2 ) I can not understand what
> colums tell me if the type of data in question is an array ( to display it
> with ' [ ] ' appended to the name ) Someone is kind enough to put me on the
> right track ?
>
> p.s.
> the function: pg_catalog.pg_get_function_arguments(p.oid) show what i need
> but after the parameter name, what i want is a list of parameter's datatype
> (the signature) without the parameter's name
>
>
​Using text as an example the system considers the type "_text" to be its
array form.  As you sure that what you are thinking is a leading space is
not this leading underscore?

David J.
​

[HACKERS] Wire protocol change

[Tatsuo Ishii]

If we are going to change the wire protocol, it would be nice if parse
complete message includes an identification of the previous parse
message. Same thing can be said to bind complete, command complete and
close complete.

The identification could be an either sequence number assigned to the
parse message or just the statement name (IMO sequence numbers are
better since duplicated statement names could be possible).

Background: in extended protocol, a reply to a message sent to server
could be asynchronously returned. This may raise problems with certain
applications which want to track down the reply messages. Suppose we
have:

Parse('q1')
Bind('s1', 'p1')
Execute('p1')
Parse('q2')
Bind('s2', 'p2')
Execute('p2')
Sync

The reply messages would be:

ParseComplete .. (a)
BindComplete
CommandComplete
ParseComplete .. (b)
BindComplete
CommandComplete
ReadyForQuery

Since (a) and (b) are compeletely identical message, it's not easy to
ditinguish which a and b.

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


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

[HACKERS] Query Procedures

[Andrea Adami]

Hello, i'm a developer from italy and i need to make a query to get the
list of stored procedures with its signature.
Basically I would like to get the same list pgAdmin shows under the node
functions of the database tree on left panel. with the query : p.oid AS
SELECT oid , p.proname AS name , p.proargtypes FROM pg_proc p I get the
list of procedures with the column "proargtypes" that contains the list of
parameters's type.
Each digit of proargtypes column refers to the corresponding tuple in
pg_type So far all is fine, the problems arise when i want convert these
numbers to strings as the previously mentioned pgAdmin did: 1 ) in typname
column (of pg_type table) some time, I did not understand why , there is a
space in front of the name , better: a space is displayed but there is not
a space because a trim do not throw away . 2 ) I can not understand what
colums tell me if the type of data in question is an array ( to display it
with ' [ ] ' appended to the name ) Someone is kind enough to put me on the
right track ?

p.s.
the function: pg_catalog.pg_get_function_arguments(p.oid) show what i need
but after the parameter name, what i want is a list of parameter's datatype
(the signature) without the parameter's name

thank you in advance
Andrea Adami

Re: [HACKERS] [PATCH] Phrase search ported to 9.6

[Dmitry Ivanov]

There are some previously unnoticed errors in docs (master branch), this patch 
fixes them.

-- 
Dmitry Ivanov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Companydiff --git a/doc/src/sgml/textsearch.sgml b/doc/src/sgml/textsearch.sgml
index 930c8f0..78eaf74 100644
--- a/doc/src/sgml/textsearch.sgml
+++ b/doc/src/sgml/textsearch.sgml
@@ -1523,34 +1523,6 @@ SELECT tsquery_phrase(to_tsquery('fat'), to_tsquery('cat'), 10);
 
  
  
-  setweight
- 
-
-  setweight(query tsquery, weight "char") returns tsquery
- 
-
- 
-  
-   setweight returns a copy of the input query in which every
-   position has been labeled with the given weight(s), either
-   A, B, C,
-   D or their combination. These labels are retained when
-   queries are concatenated, allowing words from different parts of a document
-   to be weighted differently by ranking functions.
-  
-
-  
-   Note that weight labels apply to positions, not
-   lexemes.  If the input query has been stripped of
-   positions then setweight does nothing.
-  
- 
-
-
-
-
- 
- 
   numnode
  
 
@@ -2588,7 +2560,7 @@ more sample word(s) : more indexed word(s)
 

 Specific stop words recognized by the subdictionary cannot be
-specified;  instead use - to mark the location where any
+specified;  instead use ? to mark the location where any
 stop word can appear.  For example, assuming that a and
 the are stop words according to the subdictionary:
 

-- 
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: Inline initial comparisons in TestForOldSnapshot()

[Kevin Grittner]

On Thu, Apr 21, 2016 at 8:47 AM, Kevin Grittner  wrote:
> Inline initial comparisons in TestForOldSnapshot()

This seems to have broken Windows builds.  Is there something
special that needs to be done if a function referenced from a
contrib module changes to an inline function?

--
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] Optimization for updating foreign tables in Postgres FDW

[Robert Haas]

On Thu, Apr 21, 2016 at 8:44 AM, Michael Paquier
 wrote:
> On Thu, Apr 21, 2016 at 5:22 PM, Etsuro Fujita
>  wrote:
>> Attached is an updated version of the patch, which modified Michael's
>> version of the patch, as I proposed in [1] (see "Other changes:").  I
>> modified comments for pgfdw_get_result/pgfdw_exec_query also, mainly because
>> words like "non-blocking mode" there seems confusing (note that we have
>> PQsetnonbloking).
>
> OK, so that is what I sent except that the comments mentioning PG_TRY
> are moved to their correct places. That's fine for me. Thanks for
> gathering everything in a single patch and correcting it.

I have committed this patch.  Thanks for working on this.  Sorry for the delay.

-- 
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] Why doesn't src/backend/port/win32/socket.c implement bind()?

[Tom Lane]

I wrote:
> Michael Paquier  writes:
>> And this gives the patch attached, just took the time to hack it.

> I think this is a good idea, but (1) I'm inclined not to restrict it to
> Windows, and (2) I think we should hold off applying it until we've seen
> a failure or two more, and can confirm whether d1b7d4877 does anything
> useful for the error messages.

OK, we now have failures from both bowerbird and jacana with the error
reporting patch applied:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird=2016-04-21%2012%3A03%3A02
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=jacana=2016-04-19%2021%3A00%3A39

and they both boil down to this:

pg_ctl: could not start server
Examine the log output.
# pg_ctl failed; logfile:
LOG:  could not bind IPv4 socket: Permission denied
HINT:  Is another postmaster already running on port 60200? If not, wait a few 
seconds and retry.
WARNING:  could not create listen socket for "127.0.0.1"
FATAL:  could not create any TCP/IP sockets
LOG:  database system is shut down

So "permission denied" is certainly more useful than "no error", which
makes me feel that d1b7d4877+22989a8e3 are doing what they intended to
and should get back-patched --- any objections?

However, it's still not entirely clear what is the root cause of the
failure and whether a patch along the discussed lines would prevent its
recurrence.  Looking at TranslateSocketError, it seems we must be seeing
an underlying error code of WSAEACCES.  A little googling says that
Windows might indeed return that, rather than the more expected
WSAEADDRINUSE, if someone else has the port open with SO_EXCLUSIVEADDRUSE:

Another possible reason for the WSAEACCES error is that when the
bind function is called (on Windows NT 4.0 with SP4 and later),
another application, service, or kernel mode driver is bound to
the same address with exclusive access. Such exclusive access is a
new feature of Windows NT 4.0 with SP4 and later, and is
implemented by using the SO_EXCLUSIVEADDRUSE option.

So theory A is that some other program is binding random high port numbers
with SO_EXCLUSIVEADDRUSE.  Theory B is that this is the handiwork of
Windows antivirus software doing what Windows antivirus software typically
does, ie inject random permissions failures depending on the phase of the
moon.  It's not very clear that a test along the lines described (that is,
attempt to connect to, not bind to, the target port) would pre-detect
either type of error.  Under theory A, a connect() test would recognize
the problem only if the other program were using the port to listen rather
than make an outbound connection; and the latter seems much more likely.
(Possibly we could detect the latter case by checking the error code
returned by connect(), but Michael's proposed patch does no such thing.)
Under theory B, we're pretty much screwed, we don't know what will happen.

I wonder what Andrew can tell us about what else is running on that
machine and whether either theory has any credibility.

BTW, if Windows *had* returned WSAEADDRINUSE, TranslateSocketError would
have failed to translate it --- surely that's an oversight?

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

[Kevin Grittner]

On Wed, Apr 20, 2016 at 8:08 PM, Ants Aasma  wrote:

> However, while checking out if my proof of concept patch actually
> works I hit another issue. I couldn't get my test for the feature to
> actually work. The test script I used is attached.

Could you provide enough to make that a self-contained reproducible
test case (i.e., that I don't need to infer or re-write any steps
or guess how to call it)?  In previous cases people have given me
where they felt that the feature wasn't working there have have
been valid reasons for it to behave as it was (e.g., a transaction
with a transaction ID and an xmin which prevented cleanup from
advancing).  I'll be happy to look at your case and see whether
it's another such case or some bug, but it seems a waste to reverse
engineer or rewrite parts of the test case to do so.

--
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] Wire protocol compression

[Andreas Karlsson]

On 04/21/2016 03:04 PM, Aleksander Alekseev wrote:

I guess since the usual answer for compression was "use what SSL
provides you for free", it's rather unlikely that someone bothered to
make a proxy just for that purpose, and really, a proxy is just
another moving part in your setup: not everyone will be thrilled to
add that.


It just doesn't sound like a feature that should be implemented
separately for every single application that uses TCP. Granted TCP proxy
is not the most convenient way to solve a task. Maybe it could be
implemented in OpenVPN or on Linux TCP/IP stack level.


Wouldn't such a solution be just as vulnerable to CRIME as TLS is? I 
thought the reason for removing compression from TLS is to discourage 
people from writing applications which are vulnerable to compression 
based attacks by not proving an easy for people to just compress everything.


Andreas


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

Re: [HACKERS] Wire protocol compression

[Tom Lane]

Craig Ringer  writes:
> On 21 April 2016 at 14:19, Shay Rojansky  wrote:
>> There are potentially huge bandwidth savings which could benefit both WAN
>> and non-WAN scenarios, and decoupling this problem from TLS would make it
>> both accessible to everyone (assuming PostgreSQL clients follow). It would
>> be a protocol change though.

> The problem there is that suddenly everyone wants to get their desired
> protocol features in, since we're changing things anyway, and "enabling
> protocol compression" becomes ... rather more.
> https://wiki.postgresql.org/wiki/Todo#Wire_Protocol_Changes

Yeah.  I think this should definitely be in the list of things we want
to add when we do the fabled 4.0 protocol revision (and, indeed, it's
been in the above-cited list for awhile).  Whether we've yet gotten to
the point of having critical mass for a revision ... meh, I doubt it.

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

[Kevin Grittner]

On Wed, Apr 20, 2016 at 8:50 AM, Kevin Grittner  wrote:

> In case anyone notices some code left at the bottom of bufmgr.h
> related to inline functions, that was left on purpose, because I am
> pretty sure that the fix for the performance regression observed
> when the "snapshot too old" feature is disabled will involve making
> at least part of TestForOldSnapshot() an inline function -- so it
> seemed dumb to rip that out now only to put it back again right
> away.

I pushed something along those lines.  I didn't want to inline the
whole function because IsCatalogRelation() and
RelationIsAccessibleInLogicalDecoding() seemed kinda big to inline
and require rel.h to be included; so bringing them into bufmgr.h
would have spread that around too far.  Putting the quick tests in
an inline function which calls a non-inlined _impl function seemed
like the best compromise.

My connectivity problems to our big NUMA machines have not yet
been resolved, so I didn't have a better test case for this than
200 read-only clients at saturation on my single-socket i7, which
was only a 2.2% to 2.3% regression -- so I encourage anyone who was
able to create something more significant with
old_snapshot_threshold = -1 to try with the latest and report the
impact for your environment.  I'm not sure whether any more is
needed here.

--
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] Wire protocol compression

[Craig Ringer]

On 21 April 2016 at 14:19, Shay Rojansky  wrote:


> Does it make sense to you guys to discuss compression outside of TLS?
>

Yes.


> There are potentially huge bandwidth savings which could benefit both WAN
> and non-WAN scenarios, and decoupling this problem from TLS would make it
> both accessible to everyone (assuming PostgreSQL clients follow). It would
> be a protocol change though.
>

It would, but not necessarily a drastic one.

The only reason it has to be any drama at all is that we can't securely use
some GUC like allow_compression=on set in the startup message. As Tom has
pointed out in the past, users may be able to set these "through" a  client
library that is not expecting compressed responses, causing the client
library to fail in a possibly-exploitable manner. I'm not nearly as
concerned about that, but I don't deny that it's theoretically possible.

So we'd have to do a protocol version bump from the 3.0 protocol, allowing
clients to send 3.1 (or 4.0 or whatever) protocol requests. Newer clients
connecting to older servers would get rejected and have to reconnect with
3.0 protocol, or be configured with protocol=3.0 in their setup (JDBC URI,
libpq connstring, etc). Much like we already do for SSL when the server
doesn't support it.

The problem there is that suddenly everyone wants to get their desired
protocol features in, since we're changing things anyway, and "enabling
protocol compression" becomes ... rather more.

https://wiki.postgresql.org/wiki/Todo#Wire_Protocol_Changes

It'd be pretty stupid not to add some form of capabilities negotiation, at
least, so we can avoid having the same drama next time. In the process we'd
probably want to get proper STARTTLS-like support in place to avoid the
disconnect/reconnect dance when connecting to a host that doesn't support
SSL.

So this isn't quite as simple as just adding compression if both client and
server support it. But I do think it's very worthwhile, and that we've been
relying on a pretty sad cop-out for some time by telling people to use SSL
protocol layer compression.

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

Re: [HACKERS] Wire protocol compression

[Shulgin, Oleksandr]

On Thu, Apr 21, 2016 at 3:17 PM, Aleksander Alekseev <
a.aleks...@postgrespro.ru> wrote:

> > > or on Linux TCP/IP stack level.
> > >
> >
> > Yes, but if you want to have both compression and encryption it is
> > crucial to apply compression *before* encryption and I don't see how
> > this can happen with this approach.
>
> If I'm not mistaken IPSec gives you both compression and encryption.
> Just as an example.
>

True, but that setup (as well as any other hypothetical TCP/IP stack level
solution) would require network tweaking on both server and the clients.
With protocol-level compression you can avoid that altogether and
encryption is already solved with use of TLS.

--
Alex

Re: [HACKERS] Wire protocol compression

[Aleksander Alekseev]

> > or on Linux TCP/IP stack level.
> >
> 
> Yes, but if you want to have both compression and encryption it is
> crucial to apply compression *before* encryption and I don't see how
> this can happen with this approach.

If I'm not mistaken IPSec gives you both compression and encryption.
Just as an example. 

-- 
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] Wire protocol compression

[Shulgin, Oleksandr]

On Thu, Apr 21, 2016 at 3:04 PM, Aleksander Alekseev <
a.aleks...@postgrespro.ru> wrote:

> > I guess since the usual answer for compression was "use what SSL
> > provides you for free", it's rather unlikely that someone bothered to
> > make a proxy just for that purpose, and really, a proxy is just
> > another moving part in your setup: not everyone will be thrilled to
> > add that.
>
> It just doesn't sound like a feature that should be implemented
> separately for every single application that uses TCP. Granted TCP proxy
> is not the most convenient way to solve a task. Maybe it could be
> implemented in OpenVPN


Which is another moving part with its own setup and maintenance overhead.


> or on Linux TCP/IP stack level.
>

Yes, but if you want to have both compression and encryption it is crucial
to apply compression *before* encryption and I don't see how this can
happen with this approach.

--
Alex

Re: [HACKERS] Wire protocol compression

[Aleksander Alekseev]

> I guess since the usual answer for compression was "use what SSL
> provides you for free", it's rather unlikely that someone bothered to
> make a proxy just for that purpose, and really, a proxy is just
> another moving part in your setup: not everyone will be thrilled to
> add that.

It just doesn't sound like a feature that should be implemented
separately for every single application that uses TCP. Granted TCP proxy
is not the most convenient way to solve a task. Maybe it could be
implemented in OpenVPN or on Linux TCP/IP stack level.

-- 
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] Optimization for updating foreign tables in Postgres FDW

[Michael Paquier]

On Thu, Apr 21, 2016 at 5:22 PM, Etsuro Fujita
 wrote:
> Attached is an updated version of the patch, which modified Michael's
> version of the patch, as I proposed in [1] (see "Other changes:").  I
> modified comments for pgfdw_get_result/pgfdw_exec_query also, mainly because
> words like "non-blocking mode" there seems confusing (note that we have
> PQsetnonbloking).

OK, so that is what I sent except that the comments mentioning PG_TRY
are moved to their correct places. That's fine for me. Thanks for
gathering everything in a single patch and correcting it.
-- 
Michael


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

Re: [HACKERS] Wire protocol compression

[Shulgin, Oleksandr]

On Thu, Apr 21, 2016 at 11:07 AM, Aleksander Alekseev <
a.aleks...@postgrespro.ru> wrote:

> > Does it make sense to you guys to discuss compression outside of TLS?
> > There are potentially huge bandwidth savings which could benefit both
> > WAN and non-WAN scenarios, and decoupling this problem from TLS would
> > make it both accessible to everyone (assuming PostgreSQL clients
> > follow). It would be a protocol change though.
>
> I personally don't think it's something that should be implemented in
> PostgreSQL core. As a third-party TCP-proxy (on both client and server
> sides) with gzip/lz4 support perhaps. I'll be not surprised if it turns
> out that such projects already exist.
>

Hm, did you see this recent discussion on -hackers:
http://www.postgresql.org/message-id/flat/CAMkU=1zt9cjaQjVYAmywcP9iyxMJxFBUaVeB1eiaqBP=gej...@mail.gmail.com#CAMkU=1zt9cjaQjVYAmywcP9iyxMJxFBUaVeB1eiaqBP=gej...@mail.gmail.com
?

I guess since the usual answer for compression was "use what SSL provides
you for free", it's rather unlikely that someone bothered to make a proxy
just for that purpose, and really, a proxy is just another moving part in
your setup: not everyone will be thrilled to add that.

--
Alex

Re: [HACKERS] Timeline following for logical slots

[Alvaro Herrera]

Noah Misch wrote:

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

I plan to get Craig's patch applied on Monday 25th, giving me some more
time for study.
.
-- 
Á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] Wire protocol compression

[Aleksander Alekseev]

> Does it make sense to you guys to discuss compression outside of TLS?
> There are potentially huge bandwidth savings which could benefit both
> WAN and non-WAN scenarios, and decoupling this problem from TLS would
> make it both accessible to everyone (assuming PostgreSQL clients
> follow). It would be a protocol change though.

I personally don't think it's something that should be implemented in
PostgreSQL core. As a third-party TCP-proxy (on both client and server
sides) with gzip/lz4 support perhaps. I'll be not surprised if it turns
out that such projects already exist.

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


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

Re: [HACKERS] "parallel= " information is not coming in pg_dumpall for create aggregate

[tushar]

On 04/21/2016 08:36 AM, Robert Haas wrote:

Nice catch, Tushar.  Thanks for the patch, Fabrízio.  Committed.
Thanks, Verified against the latest sources of PG9.6 - issue has been 
fixed now.


--
regards,tushar



--
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] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

On 2016/04/19 14:25, Etsuro Fujita wrote:

On 2016/04/19 13:59, Michael Paquier wrote:

But huge objection to that because this fragilizes the current logic
postgres_fdw is based on: PQexec returns the last result to caller,
I'd rather not break that logic for 9.6 stability's sake.



IIUC, I think each query submitted by PQexec in postgres_fdw.c contains
just a single command.  Maybe I'm missing something, though.



A even better proof of that is the following, which just emulates what
your version of pgfdw_get_result is doing when consuming the results.
+   /* Verify that there are no more results */
+   res = pgfdw_get_result(fmstate->conn, fmstate->query);
+   if (res != NULL)
+   pgfdw_report_error(ERROR, res, fmstate->conn, true,
fmstate->query);
This could even lead to incorrect errors in the future if multiple
queries are combined with those DMLs for a reason or another.



I'd like to leave such enhancements for future work...


On reflection, I'd like to agree with Michael on that point.  As 
mentioned by him, we should not lose the future extendability.  And I 
don't think his version of pgfdw_get_result would damage the robustness 
of in-postgres_fdw.c functions using that, which was my concern.  Sorry 
for the noise.


Attached is an updated version of the patch, which modified Michael's 
version of the patch, as I proposed in [1] (see "Other changes:").  I 
modified comments for pgfdw_get_result/pgfdw_exec_query also, mainly 
because words like "non-blocking mode" there seems confusing (note that 
we have PQsetnonbloking).


Best regards,
Etsuro Fujita

[1] http://www.postgresql.org/message-id/5715b08a.2030...@lab.ntt.co.jp
diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c
index 189f290..16ef38f 100644
--- a/contrib/postgres_fdw/connection.c
+++ b/contrib/postgres_fdw/connection.c
@@ -17,6 +17,7 @@
 #include "access/xact.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
+#include "storage/latch.h"
 #include "utils/hsearch.h"
 #include "utils/memutils.h"
 
@@ -448,6 +449,78 @@ GetPrepStmtNumber(PGconn *conn)
 }
 
 /*
+ * Submit a query and wait for the result.
+ *
+ * This function is interruptible by signals.
+ *
+ * Caller is responsible for the error handling on the result.
+ */
+PGresult *
+pgfdw_exec_query(PGconn *conn, const char *query)
+{
+	/*
+	 * Submit a query.  Since we don't use non-blocking mode, this also can
+	 * block.  But its risk is relatively small, so we ignore that for now.
+	 */
+	if (!PQsendQuery(conn, query))
+		pgfdw_report_error(ERROR, NULL, conn, false, query);
+
+	/* Wait for the result. */
+	return pgfdw_get_result(conn, query);
+}
+
+/*
+ * Wait for the result from a prior asynchronous execution function call.
+ *
+ * This function offers quick responsiveness by checking for any interruptions.
+ *
+ * This function emulates the PQexec()'s behavior of returning the last result
+ * when there are many.
+ *
+ * Caller is responsible for the error handling on the result.
+ */
+PGresult *
+pgfdw_get_result(PGconn *conn, const char *query)
+{
+	PGresult   *last_res = NULL;
+
+	for (;;)
+	{
+		PGresult *res;
+
+		while (PQisBusy(conn))
+		{
+			int		wc;
+
+			/* Sleep until there's something to do */
+			wc = WaitLatchOrSocket(MyLatch,
+   WL_LATCH_SET | WL_SOCKET_READABLE,
+   PQsocket(conn),
+   -1L);
+			ResetLatch(MyLatch);
+
+			CHECK_FOR_INTERRUPTS();
+
+			/* Data available in socket */
+			if (wc & WL_SOCKET_READABLE)
+			{
+if (!PQconsumeInput(conn))
+	pgfdw_report_error(ERROR, NULL, conn, false, query);
+			}
+		}
+
+		res = PQgetResult(conn);
+		if (res == NULL)
+			break;/* query is complete */
+
+		PQclear(last_res);
+		last_res = res;
+	}
+
+	return last_res;
+}
+
+/*
  * Report an error we got from the remote server.
  *
  * elevel: error level to use (typically ERROR, but might be less)
@@ -598,6 +671,32 @@ pgfdw_xact_callback(XactEvent event, void *arg)
 case XACT_EVENT_ABORT:
 	/* Assume we might have lost track of prepared statements */
 	entry->have_error = true;
+
+	/*
+	 * If a command has been submitted to the remote server by
+	 * using an asynchronous execution function, the command
+	 * might not have yet completed.  Check to see if a command
+	 * is still being processed by the remote server, and if so,
+	 * request cancellation of the command; if not, abort
+	 * gracefully.
+	 */
+	if (PQtransactionStatus(entry->conn) == PQTRANS_ACTIVE)
+	{
+		PGcancel   *cancel;
+		char		errbuf[256];
+
+		if ((cancel = PQgetCancel(entry->conn)))
+		{
+			if (!PQcancel(cancel, errbuf, sizeof(errbuf)))
+ereport(WARNING,
+		(errcode(ERRCODE_CONNECTION_FAILURE),
+		 errmsg("could not send cancel request: %s",
+errbuf)));
+			PQfreeCancel(cancel);
+		}
+		break;
+	}
+
 	/* If we're aborting, abort all remote transactions too */
 	res = PQexec(entry->conn, 

[HACKERS] Wire protocol compression

[Shay Rojansky]

I know this has been discussed before (
http://postgresql.nabble.com/Compression-on-SSL-links-td2261205.html,
http://www.postgresql.org/message-id/BANLkTi=Ba1ZCmBuwwn7M1wvPFioT=6n...@mail.gmail.com),
but it seems to make sense to revisit this in 2016.

Since CRIME in 2012, AFAIK compression with encryption is considered
insecure, and the feature is removed entirely in the TLS 1.3 draft. In
addition (and because of that), many (most?) client TLS implementations
don't support compression (Java, .NET), meaning that a considerable number
of PostgreSQL users don't have access to compression.

Does it make sense to you guys to discuss compression outside of TLS? There
are potentially huge bandwidth savings which could benefit both WAN and
non-WAN scenarios, and decoupling this problem from TLS would make it both
accessible to everyone (assuming PostgreSQL clients follow). It would be a
protocol change though.

Shay