Re: [HACKERS] [sqlsmith] crashes in RestoreSnapshot on hot standby

[Andreas Seltenreich]

Amit Kapila writes:
> On Fri, Jul 1, 2016 at 9:38 AM, Thomas Munro  
> wrote:
>> Or maybe just like this?
>>
>> -   snapshot->subxip = snapshot->xip + serialized_snapshot->xcnt;
>> +   snapshot->subxip = ((TransactionId *) (snapshot + 1)) +
>> +   serialized_snapshot->xcnt;
>>
>
> This way it looks better to me.  Thanks for the patch.

I no longer see these crashes when testing with the patch applied.

thanks,
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] Forthcoming SQL standards about JSON and Multi-Dimensional Arrays (FYI)

[Pavel Stehule]

Hi

2016-06-29 1:51 GMT+02:00 Stefan Keller :

> Hi,
>
> FYI: I'd just like to point you to following two forthcoming standard
> parts from "ISO/IEC JTS 1/SC 32" comittee: one on JSON, and one on
> "Multi-Dimensional Arrays" (SQL/MDA).
>
> They define there some things different as already in PG. See also
> Peter Baumann's slides [1] and e.g. [2]
>
> :Stefan
>
> [1]
> https://www.unibw.de/inf4/professors/geoinformatics/agile-2016-workshop-gis-with-nosql
> [2]
> http://jtc1sc32.org/doc/N2501-2550/32N2528-WG3-Tutorial-Opening-Plenary.pdf
>
>
Can be nice if we can write API with respects ANSI SQL. The main problem is
query language - we doesn't support JSONPath. But if we have a own regular
expression library, probably we can have i own implementation of JSONPath

Pavel



>
> --
> 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] [sqlsmith] crashes in RestoreSnapshot on hot standby

[Amit Kapila]

On Fri, Jul 1, 2016 at 9:38 AM, Thomas Munro
 wrote:
> On Fri, Jul 1, 2016 at 3:25 PM, Amit Kapila  wrote:
>> On Fri, Jul 1, 2016 at 8:48 AM, Thomas Munro 
>> wrote:
>>> If serialized_snapshot->xcnt == 0, then snapshot->xip never gets
>>> initialized to a non-NULL value.  Then if serialized_snapshot->subxcnt
>>> > 0, we set snapshot->subxip = snapshot->xip +
>>> serialized_snapshot->xcnt (so that's NULL too).  Then in line the line
>>> you show we call memcpy(snapshot->subxip, ...).  The fix might be
>>> something like the attached.
>>
>> I was just typing the mail, when I see this mail.  I also reached to the
>> conclusion that this is the reason of crash.  You can see how CopySnapshot
>> calculates the subxipoff, may be writing code that way will be more
>> consistent.
>
> Or maybe just like this?
>
> -   snapshot->subxip = snapshot->xip + serialized_snapshot->xcnt;
> +   snapshot->subxip = ((TransactionId *) (snapshot + 1)) +
> +   serialized_snapshot->xcnt;
>

This way it looks better to me.  Thanks for the patch.


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


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

Re: [HACKERS] Broken handling of lwlocknames.h

[Noah Misch]

On Tue, Jun 28, 2016 at 12:26:00PM +0900, Michael Paquier wrote:
> On Tue, Jun 28, 2016 at 3:22 AM, Christoph Berg  wrote:
> > Re: Tom Lane 2016-06-27 <31398.1467036...@sss.pgh.pa.us>
> >> Bjorn Munch reported off-list that this sequence:
> >>
> >> unpack tarball, cd into it
> >> ./configure ...
> >> cd src/test/regress
> >> make
> >>
> >> no longer works in 9.6beta2, where it did work in previous releases.
> >> I have confirmed both statements.  The failure looks like
> >>
> >> gcc -Wall -Wmissing-prototypes -Wpointer-arith 
> >> -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute 
> >> -Wformat-security -fno-strict-aliasing -fwrapv -g -O1 -fpic 
> >> -I../../../src/include -D_GNU_SOURCE   -c -o regress.o regress.c
> >> In file included from ../../../src/include/storage/lock.h:23,
> >>  from ../../../src/include/access/heapam.h:22,
> >>  from ../../../src/include/nodes/execnodes.h:18,
> >>  from ../../../src/include/commands/trigger.h:17,
> >>  from regress.c:29:
> >> ../../../src/include/storage/lwlock.h:129:33: error: 
> >> storage/lwlocknames.h: No such file or directory
> >> make: *** [regress.o] Error 1
> >>
> >> So this is some sort of fallout from commit aa65de042f582896, which
> >> invented that as a generated file.
> >>
> >> Perhaps the solution is to extend src/test/regress/GNUmakefile to know
> >> about this file explicitly (as it already does know about
> >> src/port/pg_config_paths.h).  But that seems rather brute-force; in
> >> particular it seems like that does nothing to keep us from getting burnt
> >> again the same way in future.  I wonder if we should modify
> >> src/backend/Makefile so that it exposes a phony target for "update all the
> >> generated include files", and then have src/test/regress/GNUmakefile call
> >> that.  Or maybe there are other ways.
> >
> > That would also fix the "build plpython3 only" problem I was aiming at
> > in https://www.postgresql.org/message-id/20150916200959.gb32...@msg.df7cb.de
> >
> > So another +1 from a packagers perspective.
> 
> Yes that would be indeed cleaner this way. I have poked a bit at that
> and finished with the attached that defines some rules to generate all
> the files needed. But actually it does not seem to be enough, for
> example on OSX this would fail to compile because it cannot find the
> postgres binary in src/backend/postgres. Does somebody have an idea
> what this is about? It seems that we have two problems here.

[Action required within 72 hours.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within 72 hours of this
message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
efforts toward speedy resolution.  Thanks.

[1] 
http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.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] [sqlsmith] crashes in RestoreSnapshot on hot standby

[Thomas Munro]

On Fri, Jul 1, 2016 at 3:25 PM, Amit Kapila  wrote:
> On Fri, Jul 1, 2016 at 8:48 AM, Thomas Munro 
> wrote:
>> If serialized_snapshot->xcnt == 0, then snapshot->xip never gets
>> initialized to a non-NULL value.  Then if serialized_snapshot->subxcnt
>> > 0, we set snapshot->subxip = snapshot->xip +
>> serialized_snapshot->xcnt (so that's NULL too).  Then in line the line
>> you show we call memcpy(snapshot->subxip, ...).  The fix might be
>> something like the attached.
>
> I was just typing the mail, when I see this mail.  I also reached to the
> conclusion that this is the reason of crash.  You can see how CopySnapshot
> calculates the subxipoff, may be writing code that way will be more
> consistent.

Or maybe just like this?

-   snapshot->subxip = snapshot->xip + serialized_snapshot->xcnt;
+   snapshot->subxip = ((TransactionId *) (snapshot + 1)) +
+   serialized_snapshot->xcnt;

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


fix-subxip-v2.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] Bug in batch tuplesort memory CLUSTER case (9.6 only)

[Noah Misch]

On Sun, Jun 26, 2016 at 09:14:05PM -0700, Peter Geoghegan wrote:
> In general, moving tuplesort.c batch memory caller tuples around
> happens when batch memory needs to be recycled, or freed outright with
> pfree().
> 
> I failed to take into account that CLUSTER tuplesorts need an extra
> step when moving caller tuples to a new location (i.e. when moving
> HeapTuple caller tuples using memmove()), because their particular
> variety of caller tuple happens to itself contain a pointer to
> palloc()'d memory. Attached patch fixes this use-after-free bug.

[Action required within 72 hours.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 9.6 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
9.6 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within 72 hours of this
message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping 9.6rc1.  Consequently, I will appreciate your
efforts toward speedy resolution.  Thanks.

[1] 
http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.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] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Tsunakawa, Takayuki]

> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> So perhaps the best answer, is not 1 nor 2. Just saying that the routines
> are carefully maintained with a best effort, though sometimes you may need
> to rebuild depending on unavoidable changes in routine signatures that had
> to be introduced.

Good, I'd like to use that "mild" expression in the manual.  Although the 
expression is mild, the reality for users is not, is it?
Because the UDF developers and users cannot easily or correctly determine if 
rebuilding is necessary, nervous (enterprise) users will rebuild their UDFs 
with each minor release for the maximum safety as Michael does.

Regards
Takayuki Tsunakawa


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

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

[Noah Misch]

On Thu, Jun 16, 2016 at 01:56:44PM -0500, Kevin Grittner wrote:
> On Thu, Jun 16, 2016 at 1:32 PM, Andres Freund  wrote:
> 
> > With old_snapshot_threshold=1 I indeed can reproduce the issue. I
> > disabled autovacuum, to make the scheduling more predictable. But it
> > should "work" just as well with autovacuum.
> >
> > S1: CREATE TABLE toasted(largecol text);
> > INSERT INTO toasted SELECT string_agg(random()::text, '-') FROM 
> > generate_series(1, 1000);
> > BEGIN;
> > DELETE FROM toasted;
> > S2: BEGIN TRANSACTION ISOLATION LEVEL REPEATABLE READ;
> > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> >> ...
> > S1: COMMIT;
> > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> >> ...
> > S1: /* wait for snapshot threshold to be passed */
> > S1: VACUUM pg_toast.pg_toast_16437;
> >> INFO:  0: "pg_toast_16437": found 61942 removable, 0 nonremovable row 
> >> versions in 15486 out of 15486 pages
> >> DETAIL:  0 dead row versions cannot be removed yet.
> > S2: SELECT hashtext(largecol), length(largecol) FROM toasted;
> > ERROR:  XX000: missing chunk number 0 for toast value 16455 in 
> > pg_toast_16437
> > LOCATION:  toast_fetch_datum, tuptoaster.c:1945
> 
> Thanks!  That's something I should be able to work with.
> Unfortunately, I am going to be on vacation, so I won't have any
> results until sometime after 28 June.

This PostgreSQL 9.6 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
http://www.postgresql.org/message-id/20160527025039.ga447...@tornado.leadboat.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] [sqlsmith] crashes in RestoreSnapshot on hot standby

[Amit Kapila]

On Fri, Jul 1, 2016 at 8:48 AM, Thomas Munro 
wrote:
>
> On Fri, Jul 1, 2016 at 2:17 PM, Michael Paquier
>  wrote:
> > On Fri, Jul 1, 2016 at 6:26 AM, Andreas Seltenreich 
wrote:
> >> #1  0x00822032 in RestoreSnapshot
(start_address=start_address@entry=0x7f2701d5a110 ) at snapmgr.c:2020
> >
> > memcpy(snapshot->subxip, serialized_xids +
serialized_snapshot->xcnt,
> >serialized_snapshot->subxcnt * sizeof(TransactionId));
> > So this is choking here? Is one of those pointers NULL?
>
> Theory 1:
> If serialized_snapshot->xcnt == 0, then snapshot->xip never gets
> initialized to a non-NULL value.  Then if serialized_snapshot->subxcnt
> > 0, we set snapshot->subxip = snapshot->xip +
> serialized_snapshot->xcnt (so that's NULL too).  Then in line the line
> you show we call memcpy(snapshot->subxip, ...).  The fix might be
> something like the attached.
>

I was just typing the mail, when I see this mail.  I also reached to the
conclusion that this is the reason of crash.  You can see how CopySnapshot
calculates the subxipoff, may be writing code that way will be more
consistent.  In case of recovery, I think serialized_snapshot->xcnt will
always be zero as we fill everything in subxip array (refer below code in
GetSnapshotData).

GetSnapshotData()
{
/*
* We're in hot standby, so get XIDs from KnownAssignedXids.
..
..
}


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

Re: [HACKERS] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Michael Paquier]

On Fri, Jul 1, 2016 at 12:19 PM, Tsunakawa, Takayuki
 wrote:
>> From: Tom Lane [mailto:t...@sss.pgh.pa.us]
>> To make this situation better, what we'd really need is a bunch of work
>> to identify and document the specific APIs that we would promise won't change
>> within a release branch.  That idea has been batted around before, but
>> nobody's stepped up to do all the tedious (and, no doubt, contentious) work
>> that would be involved.
>
> I can't yet imagine if such API (including data structures) can really be 
> defined so that UDF developers feel comfortable with its flexibility.  I 
> wonder how other OSes provide such API and ABI.

That would be a lot of work, for little result. And at the end the
risk 0 does not exist and things may change. I still quite like the
answer being the mix between 1 and 2: we do our best to maintain the
backend APIs stable, but be careful that things may break if a change
is proving to be necessary.
-- 
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] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Tsunakawa, Takayuki]

> From: Tom Lane [mailto:t...@sss.pgh.pa.us]
> "Tsunakawa, Takayuki"  writes:
> > Option 2:
> > Rebuild UDFs with the target PostgreSQL distribution.
> > You do not have to rebuild UDFs when you upgrade or downgrade the
> > minor release.  (If your UDF doesn't work after changing the minor
> > release, it's the bug of PostgreSQL.  You can report it to
> > pgsql-bugs.)
> 
> I do not like either of those.  We try hard not to break extensions in minor
> releases, but I'm not willing to state it as a hard-and-fast policy that
> we never will --- especially because there's no bright line as to which
> internal APIs extensions can rely on or not.  With sufficiently negative
> assumptions about what third-party authors might have chosen to do, it could
> become impossible to fix anything at all in released branches.

I feel empathy, but I think something needs to be documented for users to 
upgrade and/or change distributions with relief.  In practice, though it may be 
a shame, isn't option 1 the current answer?

Again, the current situation seems similar to the Linux loadable kernel 
modules.  So PostgreSQL is not alone.  See "Binary compatibility" section in:

https://en.wikipedia.org/wiki/Loadable_kernel_module


> In practice, extensions seldom need to be modified for new minor releases.
> But there's a long way between that statement and a promise that it won't
> ever happen for any conceivable extension.

I think so, too.

> To make this situation better, what we'd really need is a bunch of work
> to identify and document the specific APIs that we would promise won't change
> within a release branch.  That idea has been batted around before, but
> nobody's stepped up to do all the tedious (and, no doubt, contentious) work
> that would be involved.

I can't yet imagine if such API (including data structures) can really be 
defined so that UDF developers feel comfortable with its flexibility.  I wonder 
how other OSes provide such API and ABI.

Regards
Takayuki Tsunakawa



-- 
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] [sqlsmith] crashes in RestoreSnapshot on hot standby

[Thomas Munro]

On Fri, Jul 1, 2016 at 2:17 PM, Michael Paquier
 wrote:
> On Fri, Jul 1, 2016 at 6:26 AM, Andreas Seltenreich  
> wrote:
>> #1  0x00822032 in RestoreSnapshot 
>> (start_address=start_address@entry=0x7f2701d5a110 > memory at address 0x7f2701d5a110>) at snapmgr.c:2020
>
> memcpy(snapshot->subxip, serialized_xids + serialized_snapshot->xcnt,
>serialized_snapshot->subxcnt * sizeof(TransactionId));
> So this is choking here? Is one of those pointers NULL?

Theory 1:
If serialized_snapshot->xcnt == 0, then snapshot->xip never gets
initialized to a non-NULL value.  Then if serialized_snapshot->subxcnt
> 0, we set snapshot->subxip = snapshot->xip +
serialized_snapshot->xcnt (so that's NULL too).  Then in line the line
you show we call memcpy(snapshot->subxip, ...).  The fix might be
something like the attached.

Theory 2:
The DSM segment was deleted underneath us.  We can see that it was not
mapped by the time GDB dumped that (start_address is not accessible).

Theory 3:
Somehow the xcnt or xsubcnt was wrong or the serialized snapshot was
truncated, and we read past the end of the DSM, who knows...

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


fix-subxip.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] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Michael Paquier]

On Fri, Jul 1, 2016 at 11:33 AM, Tsunakawa, Takayuki
 wrote:
> OK, I understood that your choice is option 2.  And the UDF developer should 
> report the problem and ask for its reason on pgsql-bugs, possibly end up 
> haveing to rebuild the UDF.  But if so, it sounds like option 1.  That is, 
> "For safety, rebuild your UDF with each minor release.  That way, you can 
> avoid severe problems that might take time to pop up above water."  I wonder 
> if this is similar to the Linux's loadable kernel modules.
> I'd like to hear opinions from other decision makers here before proceeding, 
> as well as Michael.

Speaking of some past experience, I got once bitten the change of
signature of IndexDefine() done in 820ab11 with 9.2, because at some
point, the tree I was maintaining kept a static copy of Postgres code,
and bootparse.c (!) was in the set. Guess the result. That was a lot
of fun to debug to find why Postgres kept crashing at initdb, and
extensions could blow up similarly if they expect routines with a
different shape.

Since then I take it on the safest side and all my in-house backend
extensions get recompiled, for each minor releases, as well as each
point in-between. So that's clearly the option 1, I get to do in for
the internal stuff I work on.

Even if there is a list of routines that are listed as in the docs
telling that those will not get broken, in some cases it is really
hard to not break that promise. Looking at for example the diffs of
820ab11, my guess is that there has been a lot of discussions around
this change, and at the end the signature of DefineIndex had to
change, for the best.

Now, speaking from the heart, it is somewhat a waste to have to
recompile that all the time... But by looking at any package
maintainer history, for example that, there are rebuilds triggered
from time to time because of changes of dependent libraries like
OpenSSL. Take here for example:
https://git.archlinux.org/svntogit/packages.git/log/trunk?h=packages/postgresql

So perhaps the best answer, is not 1 nor 2. Just saying that the
routines are carefully maintained with a best effort, though sometimes
you may need to rebuild depending on unavoidable changes in routine
signatures that had to be introduced.
-- 
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] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Tsunakawa, Takayuki]

> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> On Fri, Jul 1, 2016 at 10:35 AM, Tsunakawa, Takayuki
>  wrote:
> > I'd like to document the policy clearly in the upgrade section of
> PostgreSQL manual, eliminating any ambiguity, so that users can determine
> what they should do without fear like "may or may not work".  Which of the
> following policies should I base on?
> >
> > Option 1:
> > Rebuild UDFs with the target PostgreSQL distribution and minor release.
> >
> > Option 2:
> > Rebuild UDFs with the target PostgreSQL distribution.
> > You do not have to rebuild UDFs when you upgrade or downgrade the
> > minor release.  (If your UDF doesn't work after changing the minor
> > release, it's the bug of PostgreSQL.  You can report it to
> > pgsql-bugs.)
> 
> That would not be a bug of PostgreSQL, the terms are incorrect. If there
> is an API breakage, the extension needs to keep up in this case, so it would
> be better to mention asking on the lists what may have gone wrong.

OK, I understood that your choice is option 2.  And the UDF developer should 
report the problem and ask for its reason on pgsql-bugs, possibly end up 
haveing to rebuild the UDF.  But if so, it sounds like option 1.  That is, "For 
safety, rebuild your UDF with each minor release.  That way, you can avoid 
severe problems that might take time to pop up above water."  I wonder if this 
is similar to the Linux's loadable kernel modules.

I'd like to hear opinions from other decision makers here before proceeding, as 
well as Michael.


Regards
Takayuki Tsunakawa


-- 
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] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Tom Lane]

"Tsunakawa, Takayuki"  writes:
> I'd like to document the policy clearly in the upgrade section of PostgreSQL 
> manual, eliminating any ambiguity, so that users can determine what they 
> should do without fear like "may or may not work".  Which of the following 
> policies should I base on?

> Option 1:
> Rebuild UDFs with the target PostgreSQL distribution and minor release.

> Option 2:
> Rebuild UDFs with the target PostgreSQL distribution.
> You do not have to rebuild UDFs when you upgrade or downgrade the minor 
> release.  (If your UDF doesn't work after changing the minor release, it's 
> the bug of PostgreSQL.  You can report it to pgsql-bugs.)

I do not like either of those.  We try hard not to break extensions in
minor releases, but I'm not willing to state it as a hard-and-fast policy
that we never will --- especially because there's no bright line as to
which internal APIs extensions can rely on or not.  With sufficiently
negative assumptions about what third-party authors might have chosen to
do, it could become impossible to fix anything at all in released
branches.

In practice, extensions seldom need to be modified for new minor releases.
But there's a long way between that statement and a promise that it won't
ever happen for any conceivable extension.

To make this situation better, what we'd really need is a bunch of work
to identify and document the specific APIs that we would promise won't
change within a release branch.  That idea has been batted around before,
but nobody's stepped up to do all the tedious (and, no doubt, contentious)
work that would be involved.

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] [sqlsmith] crashes in RestoreSnapshot on hot standby

[Michael Paquier]

On Fri, Jul 1, 2016 at 6:26 AM, Andreas Seltenreich  wrote:
> #1  0x00822032 in RestoreSnapshot 
> (start_address=start_address@entry=0x7f2701d5a110  memory at address 0x7f2701d5a110>) at snapmgr.c:2020

memcpy(snapshot->subxip, serialized_xids + serialized_snapshot->xcnt,
   serialized_snapshot->subxcnt * sizeof(TransactionId));
So this is choking here? Is one of those pointers NULL?
-- 
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] Reviewing freeze map code

[Amit Kapila]

On Thu, Jun 30, 2016 at 8:10 PM, Masahiko Sawada  wrote:
> On Thu, Jun 30, 2016 at 3:12 PM, Amit Kapila  wrote:
>> On Thu, Jun 30, 2016 at 9:13 AM, Andres Freund  wrote:
>>> On 2016-06-30 08:59:16 +0530, Amit Kapila wrote:
 On Wed, Jun 29, 2016 at 10:30 PM, Andres Freund  wrote:
 > On 2016-06-29 19:04:31 +0530, Amit Kapila wrote:
 >> There is nothing in this record which recorded the information about
 >> visibility clear flag.
 >
 > I think we can actually defer the clearing to the lock release?

 How about the case if after we release the lock on page, the heap page
 gets flushed, but not vm and then server crashes?
>>>
>>> In the released branches there's no need to clear all visible at that
>>> point. Note how heap_lock_tuple doesn't clear it at all. So we should be
>>> fine there, and that's the part where reusing an existing record is
>>> important (for compatibility).
>>>
>>
>> For back branches, I agree that heap_lock_tuple is sufficient,
>
> Even if we use heap_lock_tuple, If server crashed after flushed heap
> but not vm, after crash recovery the heap is still marked all-visible
> on vm.

So, in this case both vm and page will be marked as all_visible.

> This case could be happen even on released branched, and could make
> IndexOnlyScan returns wrong result?
>

Why do you think IndexOnlyScan will return wrong result?  If the
server crash in the way as you described, the transaction that has
made modifications will anyway be considered aborted, so the result of
IndexOnlyScan should not be wrong.

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


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

Re: [HACKERS] OpenSSL 1.1 breaks configure and more

[Michael Paquier]

On Fri, Jul 1, 2016 at 9:27 AM, Andreas Karlsson  wrote:
> Hi,
>
> Here is an initial set of patches related to OpenSSL 1.1. Everything should
> still build fine on older OpenSSL versions (and did when I tested with
> 1.0.2h).
>
> 0001-Fixes-for-compiling-with-OpenSSL-1.1.patch
>
> This patch fixes the code so it builds with OpenSSL 1.1 (except the
> CRYPTO_LOCK issue I have reported to the OpenSSL team).
>
> - Makes our configure script check for SSL_new instead
> - Uses functions instead of direct access to struct members
>
> 0002-Define-CRYPTO_LOCK-for-OpenSSL-1.1-compat.patch
>
> Fix for the removal of the CRYPTO_LOCK define. I am trying to convince them
> to add the define back. :)
>
> 0003-Remove-OpenSSL-1.1-deprecation-warnings.patch
>
> Silence all warnings. This commit changes more things and is not necessary
> for getting PostgreSQL to build against 1.1.
>
> - Silences deprecation other warnings related to that OpenSSL 1.1 now 1)
> automatically initializes the library and 2) no longer uses the locking
> callback.
> - Silences deprecation warning when generating DH parameters.

Those patches are going to need a careful review by looking at the
areas they are changing, and a backpatch. On Arch there is no test
package available except in AUR. And that's the pre3 release, OpenSSL
folks are on pre5 now with their beta2. It would be annoying to
compile it manually, but if there is no other way... Is Debian up to
date with 1.1.0 beta2 in its snapshot packages?
-- 
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] _mdfd_getseg can be expensive

[Andres Freund]

On 2016-06-30 18:34:20 -0700, Peter Geoghegan wrote:
> On Thu, Jun 30, 2016 at 6:23 PM, Andres Freund  wrote:
> > I plan to, once the tree opens again. Likely needs some considerable
> > updates for recent changes.
> 
> Offhand, do you think that CREATE INDEX calls to smgrextend() could be
> appreciably affected by this bottleneck? If that's a very involved or
> difficult question, then no need to answer now.

If you have a big enough index (maybe ~150GB+), sure. Before that,
probably not.

It's usually pretty easy to see in cpu profiles whether this issue
exists.

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] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Michael Paquier]

On Fri, Jul 1, 2016 at 10:35 AM, Tsunakawa, Takayuki
 wrote:
> I'd like to document the policy clearly in the upgrade section of PostgreSQL 
> manual, eliminating any ambiguity, so that users can determine what they 
> should do without fear like "may or may not work".  Which of the following 
> policies should I base on?
>
> Option 1:
> Rebuild UDFs with the target PostgreSQL distribution and minor release.
>
> Option 2:
> Rebuild UDFs with the target PostgreSQL distribution.
> You do not have to rebuild UDFs when you upgrade or downgrade the minor 
> release.  (If your UDF doesn't work after changing the minor release, it's 
> the bug of PostgreSQL.  You can report it to pgsql-bugs.)

That would not be a bug of PostgreSQL, the terms are incorrect. If
there is an API breakage, the extension needs to keep up in this case,
so it would be better to mention asking on the lists what may have
gone wrong.
-- 
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] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Tsunakawa, Takayuki]

> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Michael Paquier
> On Fri, Jul 1, 2016 at 9:33 AM, Tsunakawa, Takayuki
>  wrote:
> > [Q1]
> > Can the same UDF binary be used with different PostgreSQL minor releases?
> If it is, is it a defined policy (e.g. written somewhere in the manual,
> wiki, documentation in the source code)?
> >
> > For example, suppose you build a UDF X (some_extension.so/dll) with
> PostgreSQL 9.5.0.  Can I use the binary with PostgreSQL 9.5.x without
> rebuilding?
> 
> Yes, that works properly. There could be problems with potential changes
> in the backend APIs in a stable branch, but this usually does not happen
> much.
> 
> > Here, the UDF references the contents of server-side data structures,
> like pgstattuple accesses the members of HeapScanData.  If some bug fix
> of PostgreSQL changes the member layout of those structures, the UDF binary
> would possibly misbehave.  Basically, should all UDFs be rebuilt with the
> new minor release?
> 
> Not necessarily.
> 
> > Or, are PostgreSQL developers aware of such incompatibility and careful
> not to change data structure layout?
> 
> Committers are aware and careful about that, that's why exposed APIs and
> structures are normally kept stable. At least that's what I see.
> 
> > [Q2]
> > Can the same UDF binary be used with different PostgreSQL distributions
> (EnterpriseDB, OpenSCG, RHEL packages, etc.)?  Or should the UDF be built
> with the target distribution?
> 
> Each distribution has usually its own compilation options (say page size,
> etc.) even if I recall that most of them use the defaults, so it clearly
> depends on what kind of things each of them uses. I would recommend a
> recompilation just to be safe. It may not be worth spending time at looking
> and checking each one's differences.

Thanks for sharing your experience, Michael.

I'd like to document the policy clearly in the upgrade section of PostgreSQL 
manual, eliminating any ambiguity, so that users can determine what they should 
do without fear like "may or may not work".  Which of the following policies 
should I base on?

Option 1:
Rebuild UDFs with the target PostgreSQL distribution and minor release.

Option 2:
Rebuild UDFs with the target PostgreSQL distribution.
You do not have to rebuild UDFs when you upgrade or downgrade the minor 
release.  (If your UDF doesn't work after changing the minor release, it's the 
bug of PostgreSQL.  You can report it to pgsql-bugs.)


Regards
Takayuki Tsunakawa


-- 
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] _mdfd_getseg can be expensive

[Peter Geoghegan]

On Thu, Jun 30, 2016 at 6:23 PM, Andres Freund  wrote:
> I plan to, once the tree opens again. Likely needs some considerable
> updates for recent changes.

Offhand, do you think that CREATE INDEX calls to smgrextend() could be
appreciably affected by this bottleneck? If that's a very involved or
difficult question, then no need to answer now.

-- 
Peter Geoghegan


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

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 1 July 2016 at 09:02, Michael Paquier  wrote:

> On Fri, Jul 1, 2016 at 9:57 AM, Alvaro Herrera 
> wrote:
> > Craig Ringer wrote:
> >> On 30 June 2016 at 20:19, Alvaro Herrera 
> wrote:
> >>
> >> > Hmm, so what about a pure 32bit build, if such a thing still exists?
> If
> >> > so and it causes the same crash, perhaps we should have one member for
> >> > each VS version running on 32bit x86.
> >>
> >> It's fine for a pure 32-bit build, i.e. 32-bit tools and 32-bit target.
> I
> >> tested that.
> >
> > Ah, okay.  I doubt it's worth setting up buildfarm members testing all
> > cross-compiles just to try and catch possible compiler bugs that way, so
> > unless somebody wants to invest more effort in this area, it seems we're
> > done here.
>
> Sure. To be honest just using the latest version of MSVC available for
> the builds is fine I think. Windows is very careful regarding
> backward-compatibility of its compiled stuff usually, even if by using
> VS2015 you make the builds of Postgres incompatible with XP. But
> software is a world that keeps moving on, and XP is already out of
> support by Redmond.
>

I agree. I'm happier now that we've got evidence it's a compiler bug,
though.

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

[HACKERS] Bad behavior from plpython 'return []'

[Jim Nasby]

CREATE FUNCTION pg_temp.bad() RETURNS text[] LANGUAGE plpythonu AS 
$$return []$$;

SELECT pg_temp.bad();
 bad
-
 {}
(1 row)

SELECT pg_temp.bad() = '{}'::text[];
 ?column?
--
 f
(1 row)

Erm?? Turns out this is because

SELECT array_dims(pg_temp.bad()), array_dims('{}'::text[]);
 array_dims | array_dims
+
 [1:0]  |
(1 row)

and array_eq does this right off the bat:


/* fast path if the arrays do not have the same dimensionality */
if (ndims1 != ndims2 ||
memcmp(dims1, dims2, ndims1 * sizeof(int)) != 0 ||
memcmp(lbs1, lbs2, ndims1 * sizeof(int)) != 0)
result = false;


plpython is calling construct_md_array() with ndims set to 1, *lbs=1 and 
(I'm pretty sure) *dims=0. array_in throws that combination out as 
bogus; I think that construct_md_array should at least assert() that as 
well. It's only used in a few places outside of arrayfuncs.c, but I find 
it rather disturbing that an included PL has been broken in this fashion 
for quite some time (PLySequence_ToArray() is the same in 9.0). There's 
at least one plpython unit test that would have thrown an assert.


plperl appears to be immune from this because it calls 
accumArrayResult() inside a loop that shouldn't execute for a 0 length 
array. Would that be the preferred method of building arrays in 
plpython? ISTM that'd be wasteful since it would incur a useless copy 
for everything that's varlena (AFAICT plperl already suffers from this).

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)   mobile: 512-569-9461


--
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] _mdfd_getseg can be expensive

[Andres Freund]

On 2016-06-30 18:14:15 -0700, Peter Geoghegan wrote:
> On Tue, Dec 15, 2015 at 10:04 AM, Andres Freund  wrote:
> > Took a while. But here we go. The attached version is a significantly
> > revised version of my earlier patch. Notably I've pretty much entirely
> > revised the code in _mdfd_getseg() to be more similar to the state in
> > master. Also some comment policing.
> 
> Are you planning to pick this up again, Andres?

I plan to, once the tree opens again. Likely needs some considerable
updates for recent changes.

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] _mdfd_getseg can be expensive

[Peter Geoghegan]

On Tue, Dec 15, 2015 at 10:04 AM, Andres Freund  wrote:
> Took a while. But here we go. The attached version is a significantly
> revised version of my earlier patch. Notably I've pretty much entirely
> revised the code in _mdfd_getseg() to be more similar to the state in
> master. Also some comment policing.

Are you planning to pick this up again, Andres?

-- 
Peter Geoghegan


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

Re: [HACKERS] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Michael Paquier]

On Fri, Jul 1, 2016 at 9:57 AM, Alvaro Herrera  wrote:
> Craig Ringer wrote:
>> On 30 June 2016 at 20:19, Alvaro Herrera  wrote:
>>
>> > Hmm, so what about a pure 32bit build, if such a thing still exists?  If
>> > so and it causes the same crash, perhaps we should have one member for
>> > each VS version running on 32bit x86.
>>
>> It's fine for a pure 32-bit build, i.e. 32-bit tools and 32-bit target. I
>> tested that.
>
> Ah, okay.  I doubt it's worth setting up buildfarm members testing all
> cross-compiles just to try and catch possible compiler bugs that way, so
> unless somebody wants to invest more effort in this area, it seems we're
> done here.

Sure. To be honest just using the latest version of MSVC available for
the builds is fine I think. Windows is very careful regarding
backward-compatibility of its compiled stuff usually, even if by using
VS2015 you make the builds of Postgres incompatible with XP. But
software is a world that keeps moving on, and XP is already out of
support by Redmond.
-- 
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] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Michael Paquier]

On Fri, Jul 1, 2016 at 9:33 AM, Tsunakawa, Takayuki
 wrote:
> [Q1]
> Can the same UDF binary be used with different PostgreSQL minor releases?  If 
> it is, is it a defined policy (e.g. written somewhere in the manual, wiki, 
> documentation in the source code)?
>
> For example, suppose you build a UDF X (some_extension.so/dll) with 
> PostgreSQL 9.5.0.  Can I use the binary with PostgreSQL 9.5.x without 
> rebuilding?

Yes, that works properly. There could be problems with potential
changes in the backend APIs in a stable branch, but this usually does
not happen much.

> Here, the UDF references the contents of server-side data structures, like 
> pgstattuple accesses the members of HeapScanData.  If some bug fix of 
> PostgreSQL changes the member layout of those structures, the UDF binary 
> would possibly misbehave.  Basically, should all UDFs be rebuilt with the new 
> minor release?

Not necessarily.

> Or, are PostgreSQL developers aware of such incompatibility and careful not 
> to change data structure layout?

Committers are aware and careful about that, that's why exposed APIs
and structures are normally kept stable. At least that's what I see.

> [Q2]
> Can the same UDF binary be used with different PostgreSQL distributions 
> (EnterpriseDB, OpenSCG, RHEL packages, etc.)?  Or should the UDF be built 
> with the target distribution?

Each distribution has usually its own compilation options (say page
size, etc.) even if I recall that most of them use the defaults, so it
clearly depends on what kind of things each of them uses. I would
recommend a recompilation just to be safe. It may not be worth
spending time at looking and checking each one's differences.

> I guess the rebuild is necessary if the distribution modified the source code 
> of PostgreSQL.  That is, the UDF binary built with the bare PostgreSQL cannot 
> be used with EnterpriseDB's advanced edition, which may modify various data 
> structures.

That's for sure.

> How about other distributions which probably don't modify the source code?  
> Should the UDF be built with the target PostgreSQL because configure options 
> may differ, which affects data structures?

It depends on how they build it, but recompiling is the safest bet to
avoid any surprises... I recall seeing an extension code that caused a
SIGSEV with fclose(NULL) on SLES and only reported an error with
Ubuntu. The code was faulty in this case.. But recompiling is usually
a better bet of stability.
-- 
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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Alvaro Herrera]

Craig Ringer wrote:
> On 30 June 2016 at 20:19, Alvaro Herrera  wrote:
> 
> > Hmm, so what about a pure 32bit build, if such a thing still exists?  If
> > so and it causes the same crash, perhaps we should have one member for
> > each VS version running on 32bit x86.
> 
> It's fine for a pure 32-bit build, i.e. 32-bit tools and 32-bit target. I
> tested that.

Ah, okay.  I doubt it's worth setting up buildfarm members testing all
cross-compiles just to try and catch possible compiler bugs that way, so
unless somebody wants to invest more effort in this area, it seems we're
done here.

-- 
Á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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Craig Ringer]

On 30 June 2016 at 20:19, Alvaro Herrera  wrote:

> Craig Ringer wrote:
> > On 30 June 2016 at 07:21, Tom Lane  wrote:
> >
> > > Alvaro Herrera  writes:
> > > > Tom Lane wrote:
> > > >> Thanks for investigating!  I'll go commit that change.  I wish
> someone
> > > >> would put up a buildfarm critter using VS2013, though.
> > >
> > > > Uh, isn't that what woodlouse is using?
> > >
> > > Well, it wasn't reporting this crash, so there's *something* different.
>
> > It may only affect the i386 to x86_64 cross compiler. If Woodlouse is
> using
> > native x86_64 compilers perhaps that's why?
>
> Hmm, so what about a pure 32bit build, if such a thing still exists?  If
> so and it causes the same crash, perhaps we should have one member for
> each VS version running on 32bit x86.
>

It's fine for a pure 32-bit build, i.e. 32-bit tools and 32-bit target. I
tested that.


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

Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver

[Michael Paquier]

On Fri, Jul 1, 2016 at 8:50 AM, Michael Paquier
 wrote:
> On Fri, Jul 1, 2016 at 8:48 AM, Alvaro Herrera  
> wrote:
>> Michael Paquier wrote:
>>> Yeah, I know. Now my opinion regarding this view is that we should
>>> show information about a currently-working WAL receiver, and that it
>>> has nothing to do with reporting information of its previous startup state.
>>> That's more consistent with the WAL sender.
>>
>> Okay, that argument I buy.
>>
>> I suppose this function/view should report no row at all if there is no
>> wal receiver connected, rather than a view with nulls.
>
> The function returns PG_RETURN_NULL() so as we don't have to use a
> SRF, and the view checks for IS NOT NULL, so there would be no rows
> popping up.

In short, I would just go with the attached and call it a day.
-- 
Michael
diff --git a/src/backend/replication/walreceiver.c b/src/backend/replication/walreceiver.c
index d552f04..9d858ce 100644
--- a/src/backend/replication/walreceiver.c
+++ b/src/backend/replication/walreceiver.c
@@ -770,6 +770,7 @@ WalRcvDie(int code, Datum arg)
 	Assert(walrcv->pid == MyProcPid);
 	walrcv->walRcvState = WALRCV_STOPPED;
 	walrcv->pid = 0;
+	walrcv->ready_to_display = false;
 	SpinLockRelease(>mutex);
 
 	/* Terminate the connection gracefully. */
@@ -1344,24 +1345,9 @@ pg_stat_get_wal_receiver(PG_FUNCTION_ARGS)
 	char	   *conninfo;
 
 	/* No WAL receiver, just return a tuple with NULL values */
-	if (walrcv->pid == 0)
+	if (walrcv->pid == 0 || !walrcv->ready_to_display)
 		PG_RETURN_NULL();
 
-	/*
-	 * Users attempting to read this data mustn't be shown security sensitive
-	 * data, so sleep until everything has been properly obfuscated.
-	 */
-retry:
-	SpinLockAcquire(>mutex);
-	if (!walrcv->ready_to_display)
-	{
-		SpinLockRelease(>mutex);
-		CHECK_FOR_INTERRUPTS();
-		pg_usleep(1000);
-		goto retry;
-	}
-	SpinLockRelease(>mutex);
-
 	/* determine result type */
 	if (get_call_result_type(fcinfo, NULL, ) != TYPEFUNC_COMPOSITE)
 		elog(ERROR, "return type must be a row type");

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

[HACKERS] Is a UDF binary portable across different minor releases and PostgreSQL distributions?

[Tsunakawa, Takayuki]

Hello,

While I was thinking of application binary compatibility between PostgreSQL 
releases, some questions arose about C language user-defined functions (UDFs) 
and extensions that depend on them.

[Q1]
Can the same UDF binary be used with different PostgreSQL minor releases?  If 
it is, is it a defined policy (e.g. written somewhere in the manual, wiki, 
documentation in the source code)?

For example, suppose you build a UDF X (some_extension.so/dll) with PostgreSQL 
9.5.0.  Can I use the binary with PostgreSQL 9.5.x without rebuilding?

Here, the UDF references the contents of server-side data structures, like 
pgstattuple accesses the members of HeapScanData.  If some bug fix of 
PostgreSQL changes the member layout of those structures, the UDF binary would 
possibly misbehave.  Basically, should all UDFs be rebuilt with the new minor 
release?  Or, are PostgreSQL developers aware of such incompatibility and 
careful not to change data structure layout?


[Q2]
Can the same UDF binary be used with different PostgreSQL distributions 
(EnterpriseDB, OpenSCG, RHEL packages, etc.)?  Or should the UDF be built with 
the target distribution?

I guess the rebuild is necessary if the distribution modified the source code 
of PostgreSQL.  That is, the UDF binary built with the bare PostgreSQL cannot 
be used with EnterpriseDB's advanced edition, which may modify various data 
structures.

How about other distributions which probably don't modify the source code?  
Should the UDF be built with the target PostgreSQL because configure options 
may differ, which affects data structures?


Regards
Takayuki Tsunakawa




-- 
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] OpenSSL 1.1 breaks configure and more

[Andreas Karlsson]

Hi,

Here is an initial set of patches related to OpenSSL 1.1. Everything 
should still build fine on older OpenSSL versions (and did when I tested 
with 1.0.2h).


0001-Fixes-for-compiling-with-OpenSSL-1.1.patch

This patch fixes the code so it builds with OpenSSL 1.1 (except the 
CRYPTO_LOCK issue I have reported to the OpenSSL team).


- Makes our configure script check for SSL_new instead
- Uses functions instead of direct access to struct members

0002-Define-CRYPTO_LOCK-for-OpenSSL-1.1-compat.patch

Fix for the removal of the CRYPTO_LOCK define. I am trying to convince 
them to add the define back. :)


0003-Remove-OpenSSL-1.1-deprecation-warnings.patch

Silence all warnings. This commit changes more things and is not 
necessary for getting PostgreSQL to build against 1.1.


- Silences deprecation other warnings related to that OpenSSL 1.1 now 
1) automatically initializes the library and 2) no longer uses the 
locking callback.

- Silences deprecation warning when generating DH parameters.

Andreas
>From 16bda89ea853d7ee160e049a300771b967f966ff Mon Sep 17 00:00:00 2001
From: Andreas Karlsson 
Date: Tue, 28 Jun 2016 05:55:03 +0200
Subject: [PATCH 1/3] Fixes for compiling with OpenSSL 1.1

---
 configure| 44 
 configure.in |  4 +--
 src/backend/libpq/be-secure-openssl.c| 39 +++-
 src/interfaces/libpq/fe-secure-openssl.c | 39 +++-
 4 files changed, 78 insertions(+), 48 deletions(-)

diff --git a/configure b/configure
index 1f42c8a..ca83738 100755
--- a/configure
+++ b/configure
@@ -9538,9 +9538,9 @@ else
   as_fn_error $? "library 'crypto' is required for OpenSSL" "$LINENO" 5
 fi
 
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking for SSL_library_init in -lssl" >&5
-$as_echo_n "checking for SSL_library_init in -lssl... " >&6; }
-if ${ac_cv_lib_ssl_SSL_library_init+:} false; then :
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for SSL_new in -lssl" >&5
+$as_echo_n "checking for SSL_new in -lssl... " >&6; }
+if ${ac_cv_lib_ssl_SSL_new+:} false; then :
   $as_echo_n "(cached) " >&6
 else
   ac_check_lib_save_LIBS=$LIBS
@@ -9554,27 +9554,27 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 #ifdef __cplusplus
 extern "C"
 #endif
-char SSL_library_init ();
+char SSL_new ();
 int
 main ()
 {
-return SSL_library_init ();
+return SSL_new ();
   ;
   return 0;
 }
 _ACEOF
 if ac_fn_c_try_link "$LINENO"; then :
-  ac_cv_lib_ssl_SSL_library_init=yes
+  ac_cv_lib_ssl_SSL_new=yes
 else
-  ac_cv_lib_ssl_SSL_library_init=no
+  ac_cv_lib_ssl_SSL_new=no
 fi
 rm -f core conftest.err conftest.$ac_objext \
 conftest$ac_exeext conftest.$ac_ext
 LIBS=$ac_check_lib_save_LIBS
 fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_ssl_SSL_library_init" >&5
-$as_echo "$ac_cv_lib_ssl_SSL_library_init" >&6; }
-if test "x$ac_cv_lib_ssl_SSL_library_init" = xyes; then :
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_ssl_SSL_new" >&5
+$as_echo "$ac_cv_lib_ssl_SSL_new" >&6; }
+if test "x$ac_cv_lib_ssl_SSL_new" = xyes; then :
   cat >>confdefs.h <<_ACEOF
 #define HAVE_LIBSSL 1
 _ACEOF
@@ -9644,9 +9644,9 @@ else
   as_fn_error $? "library 'eay32' or 'crypto' is required for OpenSSL" "$LINENO" 5
 fi
 
- { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing SSL_library_init" >&5
-$as_echo_n "checking for library containing SSL_library_init... " >&6; }
-if ${ac_cv_search_SSL_library_init+:} false; then :
+ { $as_echo "$as_me:${as_lineno-$LINENO}: checking for library containing SSL_new" >&5
+$as_echo_n "checking for library containing SSL_new... " >&6; }
+if ${ac_cv_search_SSL_new+:} false; then :
   $as_echo_n "(cached) " >&6
 else
   ac_func_search_save_LIBS=$LIBS
@@ -9659,11 +9659,11 @@ cat confdefs.h - <<_ACEOF >conftest.$ac_ext
 #ifdef __cplusplus
 extern "C"
 #endif
-char SSL_library_init ();
+char SSL_new ();
 int
 main ()
 {
-return SSL_library_init ();
+return SSL_new ();
   ;
   return 0;
 }
@@ -9676,25 +9676,25 @@ for ac_lib in '' ssleay32 ssl; do
 LIBS="-l$ac_lib  $ac_func_search_save_LIBS"
   fi
   if ac_fn_c_try_link "$LINENO"; then :
-  ac_cv_search_SSL_library_init=$ac_res
+  ac_cv_search_SSL_new=$ac_res
 fi
 rm -f core conftest.err conftest.$ac_objext \
 conftest$ac_exeext
-  if ${ac_cv_search_SSL_library_init+:} false; then :
+  if ${ac_cv_search_SSL_new+:} false; then :
   break
 fi
 done
-if ${ac_cv_search_SSL_library_init+:} false; then :
+if ${ac_cv_search_SSL_new+:} false; then :
 
 else
-  ac_cv_search_SSL_library_init=no
+  ac_cv_search_SSL_new=no
 fi
 rm conftest.$ac_ext
 LIBS=$ac_func_search_save_LIBS
 fi
-{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_SSL_library_init" >&5
-$as_echo "$ac_cv_search_SSL_library_init" >&6; }
-ac_res=$ac_cv_search_SSL_library_init
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_search_SSL_new" >&5
+$as_echo 

[HACKERS] Allow INSTEAD OF DELETE triggers to modify the tuple for RETURNING

[Marko Tiikkaja]

Hi,

Currently the tuple returned by INSTEAD OF triggers on DELETEs is only 
used to determine whether to pretend that the DELETE happened or not, 
which is often not helpful enough; for example, the actual tuple might 
have been concurrently UPDATEd by another transaction and one or more of 
the columns now hold values different from those in the planSlot tuple. 
Attached is an example case which is impossible to properly implement 
under the current behavior.  For what it's worth, the current behavior 
seems to be an accident; before INSTEAD OF triggers either the tuple was 
already locked (in case of BEFORE triggers) or the actual pre-DELETE 
version of the tuple was fetched from the heap.


So I'm suggesting to change this behavior and allow INSTEAD OF DELETE 
triggers to modify the OLD tuple and use that for RETURNING instead of 
returning the tuple in planSlot.  Attached is a WIP patch implementing that.


Is there any reason why we wouldn't want to change the current behavior?


.m
BEGIN;

CREATE OR REPLACE FUNCTION foof() RETURNS TRIGGER AS $$
BEGIN
-- imagine someone concurrently incremented counter here
OLD.counter := OLD.counter + 1;
RETURN OLD;
END
$$ LANGUAGE plpgsql;

CREATE TABLE foo(counter int NOT NULL);

CREATE VIEW foov AS SELECT * FROM foo;

CREATE TRIGGER foov_instead
INSTEAD OF DELETE ON foov
FOR EACH ROW
EXECUTE PROCEDURE foof();

INSERT INTO foo VALUES (0);

DELETE FROM foov RETURNING counter;

ROLLBACK;
*** a/src/backend/commands/trigger.c
--- b/src/backend/commands/trigger.c
***
*** 2295,2307  ExecARDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
}
  }
  
! bool
  ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
!HeapTuple trigtuple)
  {
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
TriggerData LocTriggerData;
!   HeapTuple   rettuple;
int i;
  
LocTriggerData.type = T_TriggerData;
--- 2295,2307 
}
  }
  
! TupleTableSlot *
  ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
!HeapTuple trigtuple, TupleTableSlot 
*slot)
  {
TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
TriggerData LocTriggerData;
!   HeapTuple   rettuple = trigtuple;
int i;
  
LocTriggerData.type = T_TriggerData;
***
*** 2333,2343  ExecIRDeleteTriggers(EState *estate, ResultRelInfo *relinfo,
   
relinfo->ri_TrigInstrument,
   
GetPerTupleMemoryContext(estate));
if (rettuple == NULL)
!   return false;   /* Delete was suppressed */
!   if (rettuple != trigtuple)
!   heap_freetuple(rettuple);
}
!   return true;
  }
  
  void
--- 2333,2359 
   
relinfo->ri_TrigInstrument,
   
GetPerTupleMemoryContext(estate));
if (rettuple == NULL)
!   return NULL;/* Delete was suppressed */
}
! 
!   if (rettuple != trigtuple)
!   {
!   /*
!* Return the modified tuple using the es_trig_tuple_slot.  We 
assume
!* the tuple was allocated in per-tuple memory context, and 
therefore
!* will go away by itself. The tuple table slot should not try 
to
!* clear it.
!*/
!   TupleTableSlot *newslot = estate->es_trig_tuple_slot;
!   TupleDesc   tupdesc = 
RelationGetDescr(relinfo->ri_RelationDesc);
! 
!   if (newslot->tts_tupleDescriptor != tupdesc)
!   ExecSetSlotDescriptor(newslot, tupdesc);
!   ExecStoreTuple(rettuple, newslot, InvalidBuffer, false);
!   slot = newslot;
!   }
! 
!   return slot;
  }
  
  void
*** a/src/backend/executor/nodeModifyTable.c
--- b/src/backend/executor/nodeModifyTable.c
***
*** 573,585  ExecDelete(ItemPointer tupleid,
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
{
!   booldodelete;
  
!   Assert(oldtuple != NULL);
!   dodelete = ExecIRDeleteTriggers(estate, resultRelInfo, 
oldtuple);
  
!   if (!dodelete)  /* "do nothing" */
return NULL;
}
else if (resultRelInfo->ri_FdwRoutine)
{
--- 573,595 
if (resultRelInfo->ri_TrigDesc &&
resultRelInfo->ri_TrigDesc->trig_delete_instead_row)
{
!   /*
!* Store the heap tuple into the tuple table slot, making sure 

Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver

[Michael Paquier]

On Fri, Jul 1, 2016 at 8:48 AM, Alvaro Herrera  wrote:
> Michael Paquier wrote:
>> Yeah, I know. Now my opinion regarding this view is that we should
>> show information about a currently-working WAL receiver, and that it
>> has nothing to do with reporting information of its previous startup state.
>> That's more consistent with the WAL sender.
>
> Okay, that argument I buy.
>
> I suppose this function/view should report no row at all if there is no
> wal receiver connected, rather than a view with nulls.

The function returns PG_RETURN_NULL() so as we don't have to use a
SRF, and the view checks for IS NOT NULL, so there would be no rows
popping up.
-- 
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] primary_conninfo missing from pg_stat_wal_receiver

[Alvaro Herrera]

Michael Paquier wrote:
> On Fri, Jul 1, 2016 at 8:35 AM, Alvaro Herrera  
> wrote:
> > Michael Paquier wrote:
> >> On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera
> >>  wrote:
> >> > Also, actually, I see no reason for the conninfo to be shown differently
> >> > regardless of a connection being already established.  If we show the
> >> > conninfo that the server is trying to use, it might be easier to
> >> > diagnose a problem.  In short, I think this is all misconceived (mea
> >> > culpa) and that we should have two conninfo members in that struct as
> >> > initially proposed, one obfuscated and the other not.
> >>
> >> If the conninfo is leaking an incorrect password, say it has only a
> >> couple of characters of difference with the real one, we'd still leak
> >> information.
> >
> > No, I don't mean to leak any password.  It would still be obfuscated,
> > but all other details would be there (anything with default values).
> 
> OK. There is no need to use two fields by the way. The WAL receiver
> makes no attempts to reconnect with the same string and leaves immediately
> should a connection fail.

Yes, but the question is what happens if somebody queries before
walreceiver attempts to connect, no?  That's the case where the current
code loops.

> >> The window where the information of a failed connection is rather
> >> limited as well, the WAL receiver process shuts down immediately and
> >> would reset its PID to 0, hiding the information anyway.
> >
> > Some of the details are set by the startup process, such as the start
> > LSN etc, not the walreceiver.  Only the PID is reset AFAICS.
> 
> Yeah, I know. Now my opinion regarding this view is that we should
> show information about a currently-working WAL receiver, and that it
> has nothing to do with reporting information of its previous startup state.
> That's more consistent with the WAL sender.

Okay, that argument I buy.

I suppose this function/view should report no row at all if there is no
wal receiver connected, rather than a view with nulls.

-- 
Á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] primary_conninfo missing from pg_stat_wal_receiver

[Michael Paquier]

On Fri, Jul 1, 2016 at 8:35 AM, Alvaro Herrera  wrote:
> Michael Paquier wrote:
>> On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera
>>  wrote:
>> > Also, actually, I see no reason for the conninfo to be shown differently
>> > regardless of a connection being already established.  If we show the
>> > conninfo that the server is trying to use, it might be easier to
>> > diagnose a problem.  In short, I think this is all misconceived (mea
>> > culpa) and that we should have two conninfo members in that struct as
>> > initially proposed, one obfuscated and the other not.
>>
>> If the conninfo is leaking an incorrect password, say it has only a
>> couple of characters of difference with the real one, we'd still leak
>> information.
>
> No, I don't mean to leak any password.  It would still be obfuscated,
> but all other details would be there (anything with default values).

OK. There is no need to use two fields by the way. The WAL receiver
makes no attempts to reconnect with the same string and leaves immediately
should a connection fail.

>> The window where the information of a failed connection is rather
>> limited as well, the WAL receiver process shuts down immediately and
>> would reset its PID to 0, hiding the information anyway.
>
> Some of the details are set by the startup process, such as the start
> LSN etc, not the walreceiver.  Only the PID is reset AFAICS.

Yeah, I know. Now my opinion regarding this view is that we should
show information about a currently-working WAL receiver, and that it
has nothing to do with reporting information of its previous startup state.
That's more consistent with the WAL sender.
-- 
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] primary_conninfo missing from pg_stat_wal_receiver

[Alvaro Herrera]

Michael Paquier wrote:
> On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera
>  wrote:
> > Also, actually, I see no reason for the conninfo to be shown differently
> > regardless of a connection being already established.  If we show the
> > conninfo that the server is trying to use, it might be easier to
> > diagnose a problem.  In short, I think this is all misconceived (mea
> > culpa) and that we should have two conninfo members in that struct as
> > initially proposed, one obfuscated and the other not.
> 
> If the conninfo is leaking an incorrect password, say it has only a
> couple of characters of difference with the real one, we'd still leak
> information.

No, I don't mean to leak any password.  It would still be obfuscated,
but all other details would be there (anything with default values).

> The window where the information of a failed connection is rather
> limited as well, the WAL receiver process shuts down immediately and
> would reset its PID to 0, hiding the information anyway.

Some of the details are set by the startup process, such as the start
LSN etc, not the walreceiver.  Only the PID is reset AFAICS.

-- 
Á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] primary_conninfo missing from pg_stat_wal_receiver

[Michael Paquier]

On Thu, Jun 30, 2016 at 11:24 PM, Alvaro Herrera
 wrote:
> Also, actually, I see no reason for the conninfo to be shown differently
> regardless of a connection being already established.  If we show the
> conninfo that the server is trying to use, it might be easier to
> diagnose a problem.  In short, I think this is all misconceived (mea
> culpa) and that we should have two conninfo members in that struct as
> initially proposed, one obfuscated and the other not.

If the conninfo is leaking an incorrect password, say it has only a
couple of characters of difference with the real one, we'd still leak
information. That's not good IMO based on the concerns raised on this
thread. I'd just mark all the fields as NULL in this case and move on.
This way the code keeps being simple, and having this information
means that the WAL receiver is correctly working. The window where the
information of a failed connection is rather limited as well, the WAL
receiver process shuts down immediately and would reset its PID to 0,
hiding the information anyway.
-- 
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] WIP: About CMake v2

[Michael Paquier]

On Fri, Jul 1, 2016 at 5:19 AM, Andrew Dunstan  wrote:
> On 06/30/2016 03:13 AM, Yury Zhuravlev wrote:
>> Only one extra phase (mkdir build). I think it's not so important. For
>> windows and macosx I used cmake GUI it's so easy.
>
> We need this to be scriptable, not using a GUI.

Definitely agreed! Personally I am allergic to any kind of UIs for
development, and I am sure not to be the only one on this list.
-- 
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] fixing subplan/subquery confusion

[Robert Haas]

On Mon, Jun 27, 2016 at 4:12 PM, Robert Haas  wrote:
>>> Tom, do you want to commit this, or do you want me to handle it, or
>>> something else?
>>
>> I was not sure if you'd agreed that the patch was correct, and in any
>> case I thought you wanted to fold it into the upperrel consider_parallel
>> patch.  I feel no great need to commit it myself, if that's what you
>> meant.
>
> OK, I'll set aside some time for further review and either commit it
> or send an update by COB Thursday US time.

I haven't had a chance to do this yet, so I'm going to do it tomorrow instead.

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


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

Re: [HACKERS] parallel workers and client encoding

[Robert Haas]

On Wed, Jun 29, 2016 at 10:52 PM, Peter Eisentraut
 wrote:
> I'm happy with this patch.

Great.  I have committed it.

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


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

Re: [HACKERS] fixing consider_parallel for upper planner rels

[Robert Haas]

On Thu, Jun 30, 2016 at 5:54 PM, Tom Lane  wrote:
>> That's pretty much right, but there are two conceptually separate
>> things.  The first is whether or not we actually attempt to spawn
>> workers, and the second is whether or not we enter parallel mode.
>> Entering parallel mode enables various restrictions that make spawning
>> workers safe, so we cannot spawn workers unless we enter parallel
>> mode.  We can, however, enter parallel mode without spawning any
>> workers, and force_parallel_mode=on does so.  The point of that is
>> that even if the plan doesn't end up being run inside of a worker, it
>> will be run with most of the same restrictions on what it can do that
>> would be in place if a truly parallel plan were chosen.
>
> And the point of that is what, exactly?  If the only change is that
> "some restrictions get enforced", I am not clear on why we need such
> a test mode in cases where the planner is afraid to put a top Gather on
> the plan.  In particular, given the coding as you now have it, it seems
> like the only case where there's any difference is where we set
> glob->parallelModeOK but nonetheless end up with a not-parallel-safe
> topmost path (that doesn't have a Gather within it).  It's not clear
> to me why having the executor switch into parallel mode makes sense at
> all with such a plan.

Suppose you create a PL/pgsql function that does an UPDATE and mark it
PARALLEL RESTRICTED.  You wonder whether you've marked it correctly.
You can set force_parallel_mode=on and SELECT myfunc().  The
subsequent ERROR tells you that you've mismarked it.

>>> * I'm still not real happy with the has_parallel_hazard test added to
>>> apply_projection_to_path, and here is why not: if the target path is
>>> not projection-capable, we'll never get to that test.  We just pass
>>> the problem off to create_projection_path, which looks no further
>>> than rel->consider_parallel.  It seems like we have to add a
>>> has_parallel_hazard test there as well, which is a tad annoying,
>>> not least because all the direct callers of create_projection_path
>>> seem to have made the test already.
>
>> Thanks for noticing that problem; I've fixed it by inserting a test
>> into create_projection_path.  As far as the annoyance factor is
>> concerned, you obviously know that there was a flag there to reduce
>> the cost of that, but you insisted on removing it.  Maybe you should
>> consider putting it back.
>
> No, that flag was on apply_projection_to_path, and I didn't care for it
> because most of the callers didn't appear to want to go to the trouble of
> setting it correctly.  Adding such an argument to create_projection_path
> as well doesn't seem to make that better.

I'm open to suggestions.  As I've noted a few times already, though
maybe less clearly than I should have done, I think it's quite likely
to be a good idea to try to avoid the overhead of running
has_parallel_hazard repeatedly on the same tlists, or for that matter,
running it on tlists at all.  I don't have any evidence that's
expensive enough to cost, just a hunch.  Exactly how to do that best,
I'm not sure.

>> Well, the point of consider_parallel is that there are some things
>> that are specific to the individual path, and there are some that are
>> properties of the RelOptInfo.  It seems highly desirable to check
>> things that fall into the latter category exactly once, rather than
>> once per path.  You seem to be fighting hard against that idea, and
>> I'm pretty baffled as to why.
>
> What I'm not happy about is that as you've got things constituted,
> the GetForeignUpperPaths hook is broken so far as injecting parallel paths
> is concerned, because the consider_parallel flags for the upperrels
> aren't set yet when it gets called.  If we keep consider_parallel with
> its present usage, we're going to have to refactor things to fix that.

I see.  It seems to me, and I may be failing to understand something,
that the placement of create_upper_paths_hook is substantially better
than the placement of GetForeignUpperPaths.  If the latter were moved
to where the former now is, then consider_parallel would be set
sufficiently early and everything would be fine.  We could
alternatively fish out the code to set consider_parallel for the upper
rels and do all of that work before calling that hook.  That's a bit
hairy, because we'd basically go set all of the consider_parallel
flags, then call that hook, then circle back around for the core path
generation, but I don't see any intrinsic obstacle to that line of
attack.  I'm not very sure that one call for all upper rels is going
to be convenient, though.

>> Updated patch attached.   (Official status update: I'll commit this,
>> possibly over the long weekend or in any event no later than Tuesday,
>> unless there are further review comments before then, in which case I
>> will post a further official status update no later than Tuesday.)
>
> Don't have time to re-read this 

Re: [HACKERS] fixing consider_parallel for upper planner rels

[Robert Haas]

On Thu, Jun 30, 2016 at 1:13 PM, Tom Lane  wrote:
> BTW, I just had another thought about reducing the cost of
> has_parallel_hazard checks, to wit: you already made one pass over the
> entire query to verify that there's no PARALLEL UNSAFE functions anywhere.
> If that pass were to also track whether there are any PARALLEL RESTRICTED
> functions anywhere, then in very many common cases, subsequent tests on
> portions of the query would not have to do anything, because we'd already
> know there was nothing to worry about.

Yeah, that's true.  I'm not sure how much those has_parallel_hazard()
checks are costing us.  The ones on quals seem like they are probably
pretty cheap, because if you've got enough quals for the cycles we
spend checking them to matter, it's the proliferation of paths and
RelOptInfos that is going to kill you, not the cost of the
has_parallel_hazard() checks.  I think, anyway.  The ones on target
lists, which I didn't foresee, seem more troubling, because you could
easily be selecting a large number of columns and so we end up with
lots of RelOptInfos that all have long target lists and we've got to
keep checking those target lists over and over again.  I'd like to
find a way to do better there.  I still think that it might be better
to optimize the tlist-is-all-vars case instead of doing what you
propose here, but I'm not sure, and your intuition may well be better
than mine.

-- 
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] fixing consider_parallel for upper planner rels

[Tom Lane]

Robert Haas  writes:
> On Thu, Jun 30, 2016 at 12:04 PM, Tom Lane  wrote:
>> * It seems like the initialization of root->parallelModeNeeded is still
>> overcomplicated and/or underexplained.

> That's pretty much right, but there are two conceptually separate
> things.  The first is whether or not we actually attempt to spawn
> workers, and the second is whether or not we enter parallel mode.
> Entering parallel mode enables various restrictions that make spawning
> workers safe, so we cannot spawn workers unless we enter parallel
> mode.  We can, however, enter parallel mode without spawning any
> workers, and force_parallel_mode=on does so.  The point of that is
> that even if the plan doesn't end up being run inside of a worker, it
> will be run with most of the same restrictions on what it can do that
> would be in place if a truly parallel plan were chosen.

And the point of that is what, exactly?  If the only change is that
"some restrictions get enforced", I am not clear on why we need such
a test mode in cases where the planner is afraid to put a top Gather on
the plan.  In particular, given the coding as you now have it, it seems
like the only case where there's any difference is where we set
glob->parallelModeOK but nonetheless end up with a not-parallel-safe
topmost path (that doesn't have a Gather within it).  It's not clear
to me why having the executor switch into parallel mode makes sense at
all with such a plan.

>> * I'm still not real happy with the has_parallel_hazard test added to
>> apply_projection_to_path, and here is why not: if the target path is
>> not projection-capable, we'll never get to that test.  We just pass
>> the problem off to create_projection_path, which looks no further
>> than rel->consider_parallel.  It seems like we have to add a
>> has_parallel_hazard test there as well, which is a tad annoying,
>> not least because all the direct callers of create_projection_path
>> seem to have made the test already.

> Thanks for noticing that problem; I've fixed it by inserting a test
> into create_projection_path.  As far as the annoyance factor is
> concerned, you obviously know that there was a flag there to reduce
> the cost of that, but you insisted on removing it.  Maybe you should
> consider putting it back.

No, that flag was on apply_projection_to_path, and I didn't care for it
because most of the callers didn't appear to want to go to the trouble of
setting it correctly.  Adding such an argument to create_projection_path
as well doesn't seem to make that better.

> Well, the point of consider_parallel is that there are some things
> that are specific to the individual path, and there are some that are
> properties of the RelOptInfo.  It seems highly desirable to check
> things that fall into the latter category exactly once, rather than
> once per path.  You seem to be fighting hard against that idea, and
> I'm pretty baffled as to why.

What I'm not happy about is that as you've got things constituted,
the GetForeignUpperPaths hook is broken so far as injecting parallel paths
is concerned, because the consider_parallel flags for the upperrels
aren't set yet when it gets called.  If we keep consider_parallel with
its present usage, we're going to have to refactor things to fix that.

> Updated patch attached.   (Official status update: I'll commit this,
> possibly over the long weekend or in any event no later than Tuesday,
> unless there are further review comments before then, in which case I
> will post a further official status update no later than Tuesday.)

Don't have time to re-read this right now, but maybe tomorrow or
Saturday.

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] fixing consider_parallel for upper planner rels

[Robert Haas]

On Thu, Jun 30, 2016 at 12:04 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> Here is a new patch addressing (I hope) the above comments and a few
>> other things.
>
> Some more comments:
>
> * It seems like the initialization of root->parallelModeNeeded is still
> overcomplicated and/or underexplained.  Why do you not merely set it false
> to start with, and allow it to become true when/if a Gather is added to
> the plan?  AFAICS its only point is to tell the executor that there's a
> Gather someplace.

That's pretty much right, but there are two conceptually separate
things.  The first is whether or not we actually attempt to spawn
workers, and the second is whether or not we enter parallel mode.
Entering parallel mode enables various restrictions that make spawning
workers safe, so we cannot spawn workers unless we enter parallel
mode.  We can, however, enter parallel mode without spawning any
workers, and force_parallel_mode=on does so.  The point of that is
that even if the plan doesn't end up being run inside of a worker, it
will be run with most of the same restrictions on what it can do that
would be in place if a truly parallel plan were chosen.  So we
basically do exactly what you are proposing here when
force_parallel_mode=off, but when it's turned on then we, uh, force
parallel mode.

Actually, the original remit of force_parallel_mode was precisely
that: force parallel mode.  The behavior of also forcing the plan to
run inside of a single-copy Gather mode is an additional behavior that
was added later on in the development process, but having two separate
GUCs felt like overkill.  I believe I discussed this on-list with Noah
at the time.  It's a separate issue from what this patch is about, at
any rate.

> * typo in new comment in grouping_planner: "final_rel is can be marked"
>
> +* If the input relation is not parallel-safe, then the window 
> relation
> +* can't be parallel-safe, either.  Otherwise, we need to examine the
> +* target list and windows for non-parallel-safe constructs.
> +* (XXX: Do we also need to check wflists?)
>
> No, we don't, because the wflists just contain windowfuncs extracted from
> the target list.  But I'd say "target list and active windows for..." for
> more clarity (the point being it's okay to ignore any unused window
> clauses).

Thanks, fixed.

> * root->parallelModeOK is probably not being consulted in as many places
> as it would be useful; could we use it to short-circuit any more
> has_parallel_hazard tests?

I think it short-circuits essentially all of them, because
set_rel_consider_parallel() is not called if it's false.  If the
baserel's consider_parallel flag is false, that will force every other
flag to be false as you move up the tree.  We don't need to recheck it
in join relations or upper relations because those have input
relations which should never be consider_parallel unless
root->parallelModeOK is set.  If there's ever a case where any
consider_parallel flag is true when root->parallelModeOK is false,
that's a bug.

> * I'm still not real happy with the has_parallel_hazard test added to
> apply_projection_to_path, and here is why not: if the target path is
> not projection-capable, we'll never get to that test.  We just pass
> the problem off to create_projection_path, which looks no further
> than rel->consider_parallel.  It seems like we have to add a
> has_parallel_hazard test there as well, which is a tad annoying,
> not least because all the direct callers of create_projection_path
> seem to have made the test already.

Thanks for noticing that problem; I've fixed it by inserting a test
into create_projection_path.  As far as the annoyance factor is
concerned, you obviously know that there was a flag there to reduce
the cost of that, but you insisted on removing it.  Maybe you should
consider putting it back.

> This gets back to the question of whether rel->consider_parallel is even
> useful at the level of upperrels.  As far as I can tell, that flag is
> really little more than a crutch to let scan/join paths be marked parallel
> safe or not with minimum ado.  I would rather like to get to a point where
> consider_parallel is not used for upperrels, because that avoids the
> problem that you're not setting it early enough to be useful for
> GetForeignUpperPaths().  (After that I'd like to get to a point where we
> don't have it at all, but that may not happen for 9.6.)

Well, the point of consider_parallel is that there are some things
that are specific to the individual path, and there are some that are
properties of the RelOptInfo.  It seems highly desirable to check
things that fall into the latter category exactly once, rather than
once per path.  You seem to be fighting hard against that idea, and
I'm pretty baffled as to why.  That flag avoids a significant amount
of unnecessary recomputation in baserels, in joinrels, and in some
though not all upperrels, 

[HACKERS] [sqlsmith] crashes in RestoreSnapshot on hot standby

[Andreas Seltenreich]

Running sqlsmith on a streaming slave (master as of f8c5855) is
inconspicuous as long as the master is idle.  As soon as I start it on
the master as well, the standby frequently crashes in RestoreSnapshot.
It doesn't seem to be specific to the queries, as they don't trigger a
crash when re-run.

Backtraces always look like the ones below.

regards,
Andreas

 BEGIN BACKTRACE OF CORE FILE ./slave/postgres.9826@.core ON doombat 
Core was generated by `postgres: smith regression [local] SELECT
 '.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  __memcpy_sse2_unaligned () at 
../sysdeps/x86_64/multiarch/memcpy-sse2-unaligned.S:167
167 ../sysdeps/x86_64/multiarch/memcpy-sse2-unaligned.S: Datei oder 
Verzeichnis nicht gefunden.
(gdb) bt
#0  __memcpy_sse2_unaligned () at 
../sysdeps/x86_64/multiarch/memcpy-sse2-unaligned.S:167
#1  0x00822032 in RestoreSnapshot 
(start_address=start_address@entry=0x7f2701d5a110 ) at snapmgr.c:2020
#2  0x004a934a in heap_beginscan_parallel (relation=0x2060a90, 
parallel_scan=parallel_scan@entry=0x7f2701d5a0f8) at heapam.c:1657
#3  0x005fbedf in ExecSeqScanInitializeDSM (node=0x1f5b470, 
pcxt=0x221af88) at nodeSeqscan.c:327
#4  0x005dd0ad in ExecParallelInitializeDSM 
(planstate=planstate@entry=0x1f5b470, d=d@entry=0x7ffd4ba200d0) at 
execParallel.c:245
#5  0x005dd425 in ExecInitParallelPlan (planstate=0x1f5b470, 
estate=estate@entry=0x1f5ab28, nworkers=2) at execParallel.c:477
#6  0x005ef4a4 in ExecGather (node=node@entry=0x1f5b048) at 
nodeGather.c:159
#7  0x005dda48 in ExecProcNode (node=node@entry=0x1f5b048) at 
execProcnode.c:515
#8  0x005f4b30 in ExecLimit (node=node@entry=0x1f5acd0) at 
nodeLimit.c:91
#9  0x005dd9d8 in ExecProcNode (node=node@entry=0x1f5acd0) at 
execProcnode.c:531
#10 0x005fef7c in ExecSetParamPlan (node=, 
econtext=0x1f5c138) at nodeSubplan.c:999
#11 0x005e28b5 in ExecEvalParamExec (exprstate=, 
econtext=, isNull=0x22045b0 "", isDone=) at 
execQual.c:1135
#12 0x005deb6d in ExecMakeFunctionResultNoSets (fcache=0x2204200, 
econtext=0x1f5c138, isNull=0x2203d98 "", isDone=) at 
execQual.c:2015
#13 0x005de29a in ExecEvalCoalesce (coalesceExpr=, 
econtext=0x1f5c138, isNull=0x2203d98 "", isDone=) at 
execQual.c:3446
#14 0x005deb6d in ExecMakeFunctionResultNoSets (fcache=0x22039e8, 
econtext=0x1f5c138, isNull=0x7ffd4ba203df "", isDone=) at 
execQual.c:2015
#15 0x005e4939 in ExecQual (qual=, 
econtext=econtext@entry=0x1f5c138, resultForNull=resultForNull@entry=0 '\000') 
at execQual.c:5269
#16 0x005faef1 in ExecResult (node=node@entry=0x1f5c020) at 
nodeResult.c:82
#17 0x005ddbf8 in ExecProcNode (node=node@entry=0x1f5c020) at 
execProcnode.c:392
#18 0x005d9c1f in ExecutePlan (dest=0x1ebb7d0, direction=, numberTuples=0, sendTuples=, operation=CMD_SELECT, 
use_parallel_mode=, planstate=0x1f5c020, estate=0x1f5ab28) at 
execMain.c:1567
#19 standard_ExecutorRun (queryDesc=0x1f5a718, direction=, 
count=0) at execMain.c:338
#20 0x006f7238 in PortalRunSelect (portal=portal@entry=0x1d13be8, 
forward=forward@entry=1 '\001', count=0, count@entry=9223372036854775807, 
dest=dest@entry=0x1ebb7d0) at pquery.c:946
#21 0x006f875e in PortalRun (portal=0x1d13be8, 
count=9223372036854775807, isTopLevel=, dest=0x1ebb7d0, 
altdest=0x1ebb7d0, completionTag=0x7ffd4ba20840 "") at pquery.c:787
#22 0x006f6003 in exec_simple_query (query_string=) at 
postgres.c:1094
#23 PostgresMain (argc=30489576, argv=0x1ecfb08, dbname=0x1cf5a00 "regression", 
username=0x1ecfc20 "\b\373\354\001") at postgres.c:4074
#24 0x0046ca67 in BackendRun (port=0x1d17b50) at postmaster.c:4262
#25 BackendStartup (port=0x1d17b50) at postmaster.c:3936
#26 ServerLoop () at postmaster.c:1693
#27 0x00690ab7 in PostmasterMain (argc=argc@entry=3, 
argv=argv@entry=0x1cf45e0) at postmaster.c:1301
#28 0x0046d9cd in main (argc=3, argv=0x1cf45e0) at main.c:228
(gdb) p debug_query_string
$1 = 0x1d68a78 "select  \n  sample_0.j as c0\nfrom \n  public.testjsonb as 
sample_0 tablesample system (8) \nwhere cast(coalesce(pg_catalog.char_length(\n 
 cast((select comment from public.room limit 1 offset 20)\n as 
text)),\npg_catalog.pg_trigger_depth()) as integer) <> 3"

 BEGIN BACKTRACE OF CORE FILE ./slave/postgres.8104@.core ON marbit 
Core was generated by `postgres: bgworker: parallel worker for PID 2610 
 '.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  __memcpy_sse2_unaligned () at 
../sysdeps/x86_64/multiarch/memcpy-sse2-unaligned.S:167
167 ../sysdeps/x86_64/multiarch/memcpy-sse2-unaligned.S: Datei oder 
Verzeichnis nicht gefunden.
(gdb) bt
#0  __memcpy_sse2_unaligned () at 
../sysdeps/x86_64/multiarch/memcpy-sse2-unaligned.S:167
#1  0x00822032 in RestoreSnapshot (start_address=0x7f7b1ee4fa58 ) at snapmgr.c:2020
#2  

Re: [HACKERS] WIP: About CMake v2

[Andrew Dunstan]

On 06/30/2016 03:13 AM, Yury Zhuravlev wrote:

Michael Paquier wrote:

You have not tested with macOS, and so did I.


Thanks! I fixed this typo. But for XCode I see more corrections. I 
will can fix it today or maybe tomorrow.



It would be nice to come as well with simpler steps than all this
mkdir build, etc stanza. Perhaps with a wrapper of some kind, in perl
that may be a good idea, though perl is not a hard requirement in
source tarballs, and on Windows/MSVC it is.


Only one extra phase (mkdir build). I think it's not so important. For 
windows and macosx I used cmake GUI it's so easy.



We need this to be scriptable, not using a GUI.

cheers

andrew



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

Re: [HACKERS] WIP: About CMake v2

[Andrew Dunstan]

On 06/30/2016 06:26 AM, Oleg Bartunov wrote:

On Wed, Jun 29, 2016 at 7:23 PM, Yury Zhuravlev
 wrote:

Hello Hackers.

I decided to talk about the current state of the project:
1. Merge with 9.6 master. 2. plpython2, plpython3, plperl, pltcl, plsql all
work correctly (all tests pass).
3. Works done for all contrib modules. 4. You can use gettext, .po->.mo will
have converted by CMake.  5. All test pass under some Linux, FreeBSD,
Solaris10 (on Sparc), Windows MSVC 2015. MacOSX I think not big trouble too.
6. Prototype for PGXS (with MSVC support) done.
I think is very big progress but I came across one problem.
I not have access to many OS and platforms. For each platform need tests and
small fixes. I can't develop and give guarantee without it.

can we use our buildfarm infrastructure for this, Andrew ?



When I get some time I intend to do some work with this to see what 
might be involved in enabling this in the buildfarm. I imagine it will 
probably involve something similar to the effort that moving to git 
entailed.


cheers

andrew





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

Re: [HACKERS] fixing consider_parallel for upper planner rels

[Tom Lane]

BTW, I just had another thought about reducing the cost of
has_parallel_hazard checks, to wit: you already made one pass over the
entire query to verify that there's no PARALLEL UNSAFE functions anywhere.
If that pass were to also track whether there are any PARALLEL RESTRICTED
functions anywhere, then in very many common cases, subsequent tests on
portions of the query would not have to do anything, because we'd already
know there was nothing to worry about.

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] fixing consider_parallel for upper planner rels

[Tom Lane]

Robert Haas  writes:
> Here is a new patch addressing (I hope) the above comments and a few
> other things.

Some more comments:

* It seems like the initialization of root->parallelModeNeeded is still
overcomplicated and/or underexplained.  Why do you not merely set it false
to start with, and allow it to become true when/if a Gather is added to
the plan?  AFAICS its only point is to tell the executor that there's a
Gather someplace.

* typo in new comment in grouping_planner: "final_rel is can be marked"

+* If the input relation is not parallel-safe, then the window relation
+* can't be parallel-safe, either.  Otherwise, we need to examine the
+* target list and windows for non-parallel-safe constructs.
+* (XXX: Do we also need to check wflists?)

No, we don't, because the wflists just contain windowfuncs extracted from
the target list.  But I'd say "target list and active windows for..." for
more clarity (the point being it's okay to ignore any unused window
clauses).

* root->parallelModeOK is probably not being consulted in as many places
as it would be useful; could we use it to short-circuit any more
has_parallel_hazard tests?

* I'm still not real happy with the has_parallel_hazard test added to
apply_projection_to_path, and here is why not: if the target path is
not projection-capable, we'll never get to that test.  We just pass
the problem off to create_projection_path, which looks no further
than rel->consider_parallel.  It seems like we have to add a
has_parallel_hazard test there as well, which is a tad annoying,
not least because all the direct callers of create_projection_path
seem to have made the test already.

This gets back to the question of whether rel->consider_parallel is even
useful at the level of upperrels.  As far as I can tell, that flag is
really little more than a crutch to let scan/join paths be marked parallel
safe or not with minimum ado.  I would rather like to get to a point where
consider_parallel is not used for upperrels, because that avoids the
problem that you're not setting it early enough to be useful for
GetForeignUpperPaths().  (After that I'd like to get to a point where we
don't have it at all, but that may not happen for 9.6.)

> Unfortunately, I can't think of a clean way to make the "right" thing
> happen here.  In general, it's right to prefer a parallel-safe plan
> over an ever-so-slightly more expensive plan that isn't, because that
> might let us join it to a partial path later on.  However, if we're at
> the very top of the plan tree, that argument holds no force, so we
> could try to install some kind of exception for that case.  That seems
> like it would be fairly invasive and fairly ugly, though.  It seems
> like a better option would be to try to kludge the test case somehow
> so that the backward index scan looks at least 1% cheaper than the
> forward index scan, but I can't see how to do that.  I don't see, for
> example, that changing any of the usual costing factors would help
> much.  We could do something very specialized here, like adding a GUC
> that makes MinMaxAggPath always win, but that seems ugly, too.

If we made add_path not consider parallel safety as a preference item
when glob->parallelModeOK is false, then we could set
max_parallel_workers_per_gather to zero for this test case and the
problem would go away.  In general, that seems like it would be a good
thing for end users too: they could avoid plan changes that are made
on the basis of completely irrelevant parallel-safety.  It might be
that we don't actually need an explicit check for that in add_path,
as long as we make sure that no path ever gets marked parallel_safe
when parallelModeOK is false.

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] Reviewing freeze map code

[Masahiko Sawada]

On Thu, Jun 30, 2016 at 3:12 PM, Amit Kapila  wrote:
> On Thu, Jun 30, 2016 at 9:13 AM, Andres Freund  wrote:
>> On 2016-06-30 08:59:16 +0530, Amit Kapila wrote:
>>> On Wed, Jun 29, 2016 at 10:30 PM, Andres Freund  wrote:
>>> > On 2016-06-29 19:04:31 +0530, Amit Kapila wrote:
>>> >> There is nothing in this record which recorded the information about
>>> >> visibility clear flag.
>>> >
>>> > I think we can actually defer the clearing to the lock release?
>>>
>>> How about the case if after we release the lock on page, the heap page
>>> gets flushed, but not vm and then server crashes?
>>
>> In the released branches there's no need to clear all visible at that
>> point. Note how heap_lock_tuple doesn't clear it at all. So we should be
>> fine there, and that's the part where reusing an existing record is
>> important (for compatibility).
>>
>
> For back branches, I agree that heap_lock_tuple is sufficient,

Even if we use heap_lock_tuple, If server crashed after flushed heap
but not vm, after crash recovery the heap is still marked all-visible
on vm.
This case could be happen even on released branched, and could make
IndexOnlyScan returns wrong result?

Regards,

--
Masahiko Sawada


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

Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver

[Alvaro Herrera]

Fujii Masao wrote:
> On Thu, Jun 30, 2016 at 10:12 PM, Michael Paquier
>  wrote:
> > On Thu, Jun 30, 2016 at 9:41 PM, Alvaro Herrera
> >  wrote:
> >> Fujii Masao wrote:
> >>> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
> >>>  wrote:
> >>> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao  
> >>> > wrote:
> >>
> >>> >> ISTM that we will never be able to get out of this loop if walreceiver
> >>> >> fails to connect to the master (e.g., password is wrong) after we enter
> >>> >> this loop.
> >>> >
> >>> > Wouldn't it be cleaner to just return an error here instead of retrying?
> >>>
> >>> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
> >>> We can just change this logic so that NULL is returned pid is 0 OR the
> >>> flag is false.
> >>
> >> For the conninfo only, or for everything?
> >
> > All of them. If this connstr is not ready for display, the WAL
> > receiver does not have a proper connection yet, so there is nothing
> > worth showing anyway to the user.
> 
> +1

slotname seems worth showing.  And if this process just started after
some other process was already receiving, then the LSN fields surely can
have useful data too.

Also, actually, I see no reason for the conninfo to be shown differently
regardless of a connection being already established.  If we show the
conninfo that the server is trying to use, it might be easier to
diagnose a problem.  In short, I think this is all misconceived (mea
culpa) and that we should have two conninfo members in that struct as
initially proposed, one obfuscated and the other not.

-- 
Á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] pg_replication_origin_xact_reset() and its argument variables

[Tom Lane]

Fujii Masao  writes:
> The document explains that pg_replication_origin_xact_reset() doesn't have
> any argument variables. But it's been actually defined so as to have two
> argument variables with pg_lsn and timestamptz data types, as follows.

> =# \df pg_replication_origin_xact_reset
>   List of functions
>Schema   |   Name   | Result data type |
>Argument data types|  Type
> +--+--+--+
>  pg_catalog | pg_replication_origin_xact_reset | void |
> pg_lsn, timestamp with time zone | normal

> As far as I read the code of the function, those arguments don't seem to
> be necessary. So I'm afraid that the pg_proc entry for the function might
> be incorrect and those two arguments should be removed from the definition.
> Is this analysis right?

Sure looks that way from here.  Copy-and-paste from the previous
line in pg_proc.h, perhaps?

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] primary_conninfo missing from pg_stat_wal_receiver

[Fujii Masao]

On Thu, Jun 30, 2016 at 10:12 PM, Michael Paquier
 wrote:
> On Thu, Jun 30, 2016 at 9:41 PM, Alvaro Herrera
>  wrote:
>> Fujii Masao wrote:
>>> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
>>>  wrote:
>>> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao  
>>> > wrote:
>>
>>> >> ISTM that we will never be able to get out of this loop if walreceiver
>>> >> fails to connect to the master (e.g., password is wrong) after we enter
>>> >> this loop.
>>> >
>>> > Wouldn't it be cleaner to just return an error here instead of retrying?
>>>
>>> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
>>> We can just change this logic so that NULL is returned pid is 0 OR the
>>> flag is false.
>>
>> For the conninfo only, or for everything?
>
> All of them. If this connstr is not ready for display, the WAL
> receiver does not have a proper connection yet, so there is nothing
> worth showing anyway to the user.

+1

Regards,

-- 
Fujii Masao


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

[HACKERS] pg_replication_origin_xact_reset() and its argument variables

[Fujii Masao]

Hi,

The document explains that pg_replication_origin_xact_reset() doesn't have
any argument variables. But it's been actually defined so as to have two
argument variables with pg_lsn and timestamptz data types, as follows.

=# \df pg_replication_origin_xact_reset
  List of functions
   Schema   |   Name   | Result data type |
   Argument data types|  Type
+--+--+--+
 pg_catalog | pg_replication_origin_xact_reset | void |
pg_lsn, timestamp with time zone | normal

As far as I read the code of the function, those arguments don't seem to
be necessary. So I'm afraid that the pg_proc entry for the function might
be incorrect and those two arguments should be removed from the definition.
Is this analysis right?

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver

[Michael Paquier]

On Thu, Jun 30, 2016 at 9:41 PM, Alvaro Herrera
 wrote:
> Fujii Masao wrote:
>> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
>>  wrote:
>> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao  wrote:
>
>> >> ISTM that we will never be able to get out of this loop if walreceiver
>> >> fails to connect to the master (e.g., password is wrong) after we enter
>> >> this loop.
>> >
>> > Wouldn't it be cleaner to just return an error here instead of retrying?
>>
>> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
>> We can just change this logic so that NULL is returned pid is 0 OR the
>> flag is false.
>
> For the conninfo only, or for everything?

All of them. If this connstr is not ready for display, the WAL
receiver does not have a proper connection yet, so there is nothing
worth showing anyway to the user.
-- 
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] primary_conninfo missing from pg_stat_wal_receiver

[Alvaro Herrera]

Fujii Masao wrote:
> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
>  wrote:
> > On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao  wrote:

> >> ISTM that we will never be able to get out of this loop if walreceiver
> >> fails to connect to the master (e.g., password is wrong) after we enter
> >> this loop.
> >
> > Wouldn't it be cleaner to just return an error here instead of retrying?
> 
> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
> We can just change this logic so that NULL is returned pid is 0 OR the
> flag is false.

For the conninfo only, or for everything?

-- 
Á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] primary_conninfo missing from pg_stat_wal_receiver

[Michael Paquier]

On Thu, Jun 30, 2016 at 9:35 PM, Fujii Masao  wrote:
> On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
>  wrote:
>> On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao  wrote:
>>> (2)
>>> +retry:
>>> +SpinLockAcquire(>mutex);
>>> +if (!walrcv->ready_to_display)
>>> +{
>>> +SpinLockRelease(>mutex);
>>> +CHECK_FOR_INTERRUPTS();
>>> +pg_usleep(1000);
>>> +goto retry;
>>> +}
>>> +SpinLockRelease(>mutex);
>>>
>>> ISTM that we will never be able to get out of this loop if walreceiver
>>> fails to connect to the master (e.g., password is wrong) after we enter
>>> this loop.
>>
>> Wouldn't it be cleaner to just return an error here instead of retrying?
>
> I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
> We can just change this logic so that NULL is returned pid is 0 OR the
> flag is false.

OK, yes. That's indeed better this way. Need a patch?
-- 
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] primary_conninfo missing from pg_stat_wal_receiver

[Fujii Masao]

On Thu, Jun 30, 2016 at 9:30 PM, Michael Paquier
 wrote:
> On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao  wrote:
>> (2)
>> +retry:
>> +SpinLockAcquire(>mutex);
>> +if (!walrcv->ready_to_display)
>> +{
>> +SpinLockRelease(>mutex);
>> +CHECK_FOR_INTERRUPTS();
>> +pg_usleep(1000);
>> +goto retry;
>> +}
>> +SpinLockRelease(>mutex);
>>
>> ISTM that we will never be able to get out of this loop if walreceiver
>> fails to connect to the master (e.g., password is wrong) after we enter
>> this loop.
>
> Wouldn't it be cleaner to just return an error here instead of retrying?

I prefer to return NULL. Now NULL is returned when walreceiver's pid is 0.
We can just change this logic so that NULL is returned pid is 0 OR the
flag is false.

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] primary_conninfo missing from pg_stat_wal_receiver

[Alvaro Herrera]

Fujii Masao wrote:
> On Thu, Jun 30, 2016 at 6:01 AM, Alvaro Herrera
>  wrote:
> > Alvaro Herrera wrote:
> >
> >> I propose to push this patch, closing the open item, and you can rework
> >> on top -- I suppose you would completely remove the original conninfo
> >> from shared memory and instead only copy the obfuscated version there
> >> (and probably also remove the ready_to_display flag).  I think we'd need
> >> to see the patch before deciding whether we want it in 9.6 or not,
> >> keeping in mind that having the conninfo in shared memory is a
> >> pre-existing problem, unrelated to the pgstats view new in 9.6.
> >
> > Pushed this.
> 
> Thanks for pushing the patch!
> But I found two problems in the patch you pushed.
> 
> (1)
> ready_to_display flag must be reset to false when walreceiver dies.
> Otherwise, pg_stat_wal_receiver can report the password (i.e.,
> the problem that I reported upthread can happen) when walreceiver restarts
> because ready_to_display flag is true from the beginning in that case.
> But you forgot to reset the flag to false when walreceiver dies.

Oops, you're right, since it's in shmem it doesn't get reset in the new
process.  Will fix.

> (2)
> +retry:
> +SpinLockAcquire(>mutex);
> +if (!walrcv->ready_to_display)
> +{
> +SpinLockRelease(>mutex);
> +CHECK_FOR_INTERRUPTS();
> +pg_usleep(1000);
> +goto retry;
> +}
> +SpinLockRelease(>mutex);
> 
> ISTM that we will never be able to get out of this loop if walreceiver
> fails to connect to the master (e.g., password is wrong) after we enter
> this loop.

Yeah, I thought that was OK.

-- 
Á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] primary_conninfo missing from pg_stat_wal_receiver

[Michael Paquier]

On Thu, Jun 30, 2016 at 8:59 PM, Fujii Masao  wrote:
> (2)
> +retry:
> +SpinLockAcquire(>mutex);
> +if (!walrcv->ready_to_display)
> +{
> +SpinLockRelease(>mutex);
> +CHECK_FOR_INTERRUPTS();
> +pg_usleep(1000);
> +goto retry;
> +}
> +SpinLockRelease(>mutex);
>
> ISTM that we will never be able to get out of this loop if walreceiver
> fails to connect to the master (e.g., password is wrong) after we enter
> this loop.

Wouldn't it be cleaner to just return an error here instead of retrying?
-- 
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] A couple of cosmetic changes around shared memory code

[Alvaro Herrera]

Robert Haas wrote:

> It might be better to document this in bgworker.sgml instead.  That
> already documents some facts about exiting:
> 
>   
>If bgw_restart_time for a background worker is
>configured as BGW_NEVER_RESTART, or if it exits with an exit
>code of 0 or is terminated by TerminateBackgroundWorker,
>it will be automatically unregistered by the postmaster on exit.
>Otherwise, it will be restarted after the time period configured via
>bgw_restart_time, or immediately if the postmaster
>reinitializes the cluster due to a backend failure.  Backends which need
>to suspend execution only temporarily should use an interruptible sleep
>rather than exiting; this can be achieved by calling
>WaitLatch(). Make sure the
>WL_POSTMASTER_DEATH flag is set when calling that function, and
>verify the return code for a prompt exit in the emergency case that
>postgres itself has terminated.
>   
> 
> That paragraph leaves out a number of important details, like:
> 
> 1. A background worker that exits with any exit code other than 0 or 1
> will cause a postmaster crash-and-restart cycle.
> 
> 2. Therefore, an exit of code 1 should be used whenever the process
> wants to be restarted in accordance with bgw_restart_time, and is
> therefore in some sense the "normal" way for a background worker to
> exit.

Yeah, I think bgworker.sgml is the right place to document these
details.

> 3. The aforementioned details about how 9.3 behavior was different
> from current behavior.

Not really sure about this.  I think patching the 9.3 docs to state the
details differently from the 9.4--master details would not be
sufficient, as people moving code would not read the 9.4 docs again to
ensure the semantics remain the same (and since 9.4 has been out for
awhile, patching the release notes wouldn't suffice either).  Patching
the 9.3 docs to say "9.4 is different" would be odd, since 9.4 is "in
the future" for the POV of 9.3 docs.

I think the best option is to backpatch a doc change from 9.4 onwards
stating what is the new behavior, and add a note stating that 9.3 was
different.

-- 
Á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] initdb issue on 64-bit Windows - (Was: [pgsql-packagers] PG 9.6beta2 tarballs are ready)

[Alvaro Herrera]

Craig Ringer wrote:
> On 30 June 2016 at 07:21, Tom Lane  wrote:
> 
> > Alvaro Herrera  writes:
> > > Tom Lane wrote:
> > >> Thanks for investigating!  I'll go commit that change.  I wish someone
> > >> would put up a buildfarm critter using VS2013, though.
> >
> > > Uh, isn't that what woodlouse is using?
> >
> > Well, it wasn't reporting this crash, so there's *something* different.

> It may only affect the i386 to x86_64 cross compiler. If Woodlouse is using
> native x86_64 compilers perhaps that's why?

Hmm, so what about a pure 32bit build, if such a thing still exists?  If
so and it causes the same crash, perhaps we should have one member for
each VS version running on 32bit x86.

(I note that the coverage of MSVC versions has greatly improved in
recent months.)

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

[Alvaro Herrera]

Gavin Flower wrote:

> I hate the rampant inflation associated with numbering schemes like FireFox
> - the numbers of no meaning at all, other than something non-trivial has
> been changed, with no indication at all about how non-trivial!

I thought this horse had already been beaten to death -- apparently not?

It was stated somewhere in these threads that the problem with the
Firefox scheme is that they release new versions every six weeks, so
naturally there is enormous version number inflation.  In our case we
would still release new versions once a year, so by 2026 (ten years from
now) we would be releasing Postgres 19 while Firefox will be in version 133.

-- 
Á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] primary_conninfo missing from pg_stat_wal_receiver

[Fujii Masao]

On Thu, Jun 30, 2016 at 6:01 AM, Alvaro Herrera
 wrote:
> Alvaro Herrera wrote:
>
>> I propose to push this patch, closing the open item, and you can rework
>> on top -- I suppose you would completely remove the original conninfo
>> from shared memory and instead only copy the obfuscated version there
>> (and probably also remove the ready_to_display flag).  I think we'd need
>> to see the patch before deciding whether we want it in 9.6 or not,
>> keeping in mind that having the conninfo in shared memory is a
>> pre-existing problem, unrelated to the pgstats view new in 9.6.
>
> Pushed this.

Thanks for pushing the patch!
But I found two problems in the patch you pushed.

(1)
ready_to_display flag must be reset to false when walreceiver dies.
Otherwise, pg_stat_wal_receiver can report the password (i.e.,
the problem that I reported upthread can happen) when walreceiver restarts
because ready_to_display flag is true from the beginning in that case.
But you forgot to reset the flag to false when walreceiver dies.

(2)
+retry:
+SpinLockAcquire(>mutex);
+if (!walrcv->ready_to_display)
+{
+SpinLockRelease(>mutex);
+CHECK_FOR_INTERRUPTS();
+pg_usleep(1000);
+goto retry;
+}
+SpinLockRelease(>mutex);

ISTM that we will never be able to get out of this loop if walreceiver
fails to connect to the master (e.g., password is wrong) after we enter
this loop.

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] Re: Should phraseto_tsquery('simple', 'blue blue') @@ to_tsvector('simple', 'blue') be true ?

[Teodor Sigaev]

That didn't cover all the places that needed to be fixed, but I have
re-read the docs and believe I've made things good now.

I have reviewed this thread and verified that all the cases raised in it
now work as desired, so I have marked the open item closed.


Thank you very much!
--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/


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

Re: [HACKERS] fixing subplan/subquery confusion

[Amit Kapila]

On Tue, Jun 28, 2016 at 5:22 PM, Amit Kapila  wrote:
> On Tue, Jun 28, 2016 at 8:25 AM, Tom Lane  wrote:
>
>> In the appendrel case, I tend to agree that the easiest solution is to
>> scan all the children of the appendrel and just mark the whole thing as
>> not consider_parallel if any of them have unsafe functions.
>>
>
> Thats what I had in mind as well, but not sure which is the best place
> to set it.  Shall we do it in set_append_rel_size() after setting the
> size of each relation (after foreach loop) or is it better to do it in
> set_append_rel_pathlist().  Is it better to do it as a separate patch
> or to enhance your patch for this change?
>

I have done it as a separate patch.  I think doing it in
set_append_rel_size() has an advantage that we don't need to scan the
child rels separately.  If you think that attached patch is on right
lines, then I can add test cases as well.


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


set-consider-parallel-append-rels-v1.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] WIP: About CMake v2

[Oleg Bartunov]

On Wed, Jun 29, 2016 at 7:23 PM, Yury Zhuravlev
 wrote:
> Hello Hackers.
>
> I decided to talk about the current state of the project:
> 1. Merge with 9.6 master. 2. plpython2, plpython3, plperl, pltcl, plsql all
> work correctly (all tests pass).
> 3. Works done for all contrib modules. 4. You can use gettext, .po->.mo will
> have converted by CMake.  5. All test pass under some Linux, FreeBSD,
> Solaris10 (on Sparc), Windows MSVC 2015. MacOSX I think not big trouble too.
> 6. Prototype for PGXS (with MSVC support) done.
> I think is very big progress but I came across one problem.
> I not have access to many OS and platforms. For each platform need tests and
> small fixes. I can't develop and give guarantee without it.

can we use our buildfarm infrastructure for this, Andrew ?

>
> I think this is very important work which makes it easier further support
> Postgres but I can not do everything himself. It's physically impossible.
>
> I think without community support I can't do significantly more.
>
> Current version you can get here: https://github.com/stalkerg/postgres_cmake
>
> Thanks!
> --
> Yury Zhuravlev
>
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


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

Re: [HACKERS] WIP: About CMake v2

[Yury Zhuravlev]

Michael Paquier wrote:

You have not tested with macOS, and so did I.


Thanks! I fixed this typo. But for XCode I see more corrections. 
I will can fix it today or maybe tomorrow.



It would be nice to come as well with simpler steps than all this
mkdir build, etc stanza. Perhaps with a wrapper of some kind, in perl
that may be a good idea, though perl is not a hard requirement in
source tarballs, and on Windows/MSVC it is.


Only one extra phase (mkdir build). I think it's not so important. 
For windows and macosx I used cmake GUI it's so easy. 


--
Yury Zhuravlev
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


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

Re: [HACKERS] pgbench unable to scale beyond 100 concurrent connections

[Sachin Kotwal]

Hi All,

Sorry for trouble you with small environment setup for testing.
I should to test this with large machine.
What I was testing were involved multiple things same time so quite
confusing .

possible reason for this testing failure is :
1. small hardware
2. haproxy not able to balance connection 100-100 on each server.
3. postgres_fdw foreign server unable to established large number of
connection with remote server/Shard.


I was testing  multiple coordinator using postgres_fdw (sharding) and
haproxy on top of it for load balancing.

as below

pg_fdw (conn=100, diff pg
instance on diff machine)  |
/
\|
pgbench (haproxy-port)->Haproxy/ (should accept 200 conn)
   \ | Shards/Nodes (1…N)
  \
/ | remote
pg servers
\
/ |
   pg_fdw(conn=100, diff pg
instance on diff machine) |


Hope i will test this scenario in detail once i get time and good hardware.

If some one test this scenario please let me know.

Thanks and regards,
Sachin Kotwal




On Thu, Jun 30, 2016 at 4:03 AM, Craig Ringer  wrote:

> On 29 June 2016 at 21:49, Sachin Kotwal  wrote:
>
>> Hi,
>>
>>
>> On Wed, Jun 29, 2016 at 6:29 PM, Craig Ringer 
>> wrote:
>>
>>> On 29 June 2016 at 18:47, Sachin Kotwal  wrote:
>>>
>>>
 I am testing pgbench with more than 100 connections.
 also set max_connection in postgresql.conf more than 100.

 Initially pgbench tries to scale nearby 150 but later it come down to
 100 connections and stable there.

 It this limitation of pgbench? or bug? or i am doing it wrong way?

>>>
>>> What makes you think this is a pgbench limitation?
>>>
>>
>> As I mentioned when I tried same thing with sysbench It can give me 200+
>> concurrent connection with same method and same machine.
>>
>
> What command lines / configs are you using? Details are necessary, talking
> about this in general hand-waving terms is not getting anywhere.
>
>
>>
>>
>>> It sounds like you're benchmarking the client and server on the same
>>> system. Couldn't this be a limitation of the backend PostgreSQL server?
>>>
>>> I think having client and server on same server should not be problem.
>> As i can do this with different benchmarking tool It should not be
>> limitation of backend PostgreSQL server.
>>
>
> OK, so your sysbench use is actually talking to PostgreSQL as well. Then
> yes. Assuming they're testing roughly the same thing, which I somewhat
> doubt.
>
> There should not be connection and disconnection because I am not using -C
>> option of pgbench which cause connection and disconnection for each query.
>>
>
> OK, in that case it's hard to explain the behaviour you're seeing.
>
> More details please.
>
>
>> If I set max_connection of postgresql.conf to 200 and testing with -c 150
>> .
>> This should work fine, but it is not.
>>
>
> If you're using FDWs to connect to the same server again, you'll need a
> max_connections slot for each FDW connection as well.
>
>
>
>> I am testing one scenario of multiple coordinator with help of
>> postgres_fdw to enhance connection ability of postgres without any
>> connection pooling .
>> Setup might be difficult to explain here but will explain if required.
>>
>
> Yes, you need to explain it.
>
>
>> can you test simply 100 scale database size with pgbench and run pgbench
>> with 200+ connection of small virtual box to see same observation ?
>>
>
> It works fine - of course. There's more to this story than you've
> explained so far.
>
> --
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>



-- 

Thanks and Regards,
Sachin Kotwal

Re: [HACKERS] Reviewing freeze map code

[Amit Kapila]

On Thu, Jun 30, 2016 at 9:13 AM, Andres Freund  wrote:
> On 2016-06-30 08:59:16 +0530, Amit Kapila wrote:
>> On Wed, Jun 29, 2016 at 10:30 PM, Andres Freund  wrote:
>> > On 2016-06-29 19:04:31 +0530, Amit Kapila wrote:
>> >> There is nothing in this record which recorded the information about
>> >> visibility clear flag.
>> >
>> > I think we can actually defer the clearing to the lock release?
>>
>> How about the case if after we release the lock on page, the heap page
>> gets flushed, but not vm and then server crashes?
>
> In the released branches there's no need to clear all visible at that
> point. Note how heap_lock_tuple doesn't clear it at all. So we should be
> fine there, and that's the part where reusing an existing record is
> important (for compatibility).
>

For back branches, I agree that heap_lock_tuple is sufficient, but in
that case we should not clear the vm or page bit at all as done in
proposed patch.

> But your question made me realize that we despearately *do* need to
> clear the frozen bit in heap_lock_tuple in 9.6...
>

Right.


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


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