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

[Tom Lane]

Michael Paquier  writes:
> On Tue, Jul 12, 2016 at 11:36 AM, Alvaro Herrera
>  wrote:
>> So prosrc for internal/C and NULL for others?  WFM.

> And so we'd remove "Language" at the same time? That does not sound bad to me.

Hm, I wasn't thinking of that step.  The main knock on "Source code" is
that it is usually too large to fit into the display grid --- but that
argument doesn't work against "Language".  Also, while "Language" is
certainly an implementation detail in some sense, it is a pretty useful
detail: it gives you a good hint about the likely speed of the function,
for instance.

regards, tom lane


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

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

[Michael Paquier]

On Tue, Jul 12, 2016 at 11:36 AM, Alvaro Herrera
 wrote:
> Stephen Frost wrote:
>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>
>> > It would certainly be easy enough to do that, as long as you don't mind
>> > hard-wiring into psql the knowledge that "internal" and "C" are the
>> > languages to show prosrc for.  "Source code" would no longer be a very
>> > appropriate column name, though it already was not for these cases.
>> > I'd be inclined to call it "Internal name" instead.
>>
>> That would certainly work for me.
>
> So prosrc for internal/C and NULL for others?  WFM.

And so we'd remove "Language" at the same time? That does not sound bad to me.
-- 
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] [BUG] pg_basebackup from disconnected standby fails

[Amit Kapila]

On Thu, Jun 23, 2016 at 12:20 PM, Michael Paquier
 wrote:
> On Wed, Jun 15, 2016 at 4:52 PM, Kyotaro HORIGUCHI
>  wrote:
>> Hello,
>
> Sorry for the late reply, Horiguchi-san. I have finally been able to
> put some mind power into that.
>
>> This is somewhat artificial but the same situation could be made
>> also in the nature. The exact condition for this is replaying a
>> checkpoint record having no buffer modification since the
>> preceding checkpoint in the previous WAL segments.
>
> After thinking about that more, I am seeing your point.
> CreateRestartPoint is clearly missing the shot for the update of
> minRecoveryPoint even when a restart point can be created.
>

I think updating minRecoveryPoint unconditionally can change it's
purpose in some cases.  Refer below comments in code:

* minRecoveryPoint is updated to the latest replayed LSN whenever we
* flush a data change during archive recovery. That guards against
* starting archive recovery, aborting it, and restarting with an earlier
* stop location. If we've already flushed data changes from WAL record X
* to disk, we mustn't start up until we reach X again.

Now, as per above rule, the value of minRecoveryPoint can be much
smaller than XLogCtl->replayEndRecPtr.  I think this can change the
rules when we can allow read-only connections.

Another point to note is that we are updating checkpoint location
during restart points, when the database state is
DB_IN_ARCHIVE_RECOVERY and updating minRecoveryPoint unconditionally
doesn't look in sync with that as well.

I think your and Kyotaro-san's point that minRecoveryPoint should be
updated to support back-ups on standby has merits, but I think doing
it unconditionally might lead to change in behaviour in some cases.


-- 
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] asynchronous and vectorized execution

[Kyotaro HORIGUCHI]

I forgot to mention.

At Tue, 12 Jul 2016 11:04:17 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20160712.110417.145469826.horiguchi.kyot...@lab.ntt.co.jp>
> Cooled down then measured performance again.
> 
> I show you the true result briefly for now.
> 
> At Mon, 11 Jul 2016 19:07:22 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
>  wrote in 
> <20160711.190722.145849861.horiguchi.kyot...@lab.ntt.co.jp>
> > Anyway I need some time to cool down..
> 
> I recalled that I put Makefile.custom that contains
> CFLAGS="-O0". Removing that gave me a sainer result.

Different from the previous measurements, the remote side in
these measurements is unpatched-O2 postgres, so the differences
are made only by the local-side changes.

> patched- -O2
> 
> table   10-average(ms)  stddev  runtime-diff from unpatched(%) 
> t0   441.78 0.32 3.4
> pl   201.77 0.3213.6
> pf0 6619.2218.99   -19.7
> pf1 1800.7232.72   -78.0
> ---
> unpatched- -O2
> 
> t0   427.21 0.42
> pl   177.54 0.25
> pf0 8250.4223.29
> pf1 8206.0212.91
> 
> ==
> 
>   3% slower for local 1*seqscan (2-parallel)
>  14% slower for append-4*seqscan (no-prallel)
>  19% faster for append-4*foreignscan (all scans on one connection)
>  78% faster for append-4*foreignscan (scans have dedicate connection)
> 
> ExecProcNode might be able to be optimized a bit.
> ExecAppend seems to need some fix.
> 
> Addition to the aboves, I will try reentrant ExecAsyncWaitForNode
> or something.

regards,
-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

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

[Alvaro Herrera]

Stephen Frost wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:

> > It would certainly be easy enough to do that, as long as you don't mind
> > hard-wiring into psql the knowledge that "internal" and "C" are the
> > languages to show prosrc for.  "Source code" would no longer be a very
> > appropriate column name, though it already was not for these cases.
> > I'd be inclined to call it "Internal name" instead.
> 
> That would certainly work for me.

So prosrc for internal/C and NULL for others?  WFM.

-- 
Á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] asynchronous and vectorized execution

[Kyotaro HORIGUCHI]

Cooled down then measured performance again.

I show you the true result briefly for now.

At Mon, 11 Jul 2016 19:07:22 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20160711.190722.145849861.horiguchi.kyot...@lab.ntt.co.jp>
> Anyway I need some time to cool down..

I recalled that I put Makefile.custom that contains
CFLAGS="-O0". Removing that gave me a sainer result.


patched- -O2

table   10-average(ms)  stddev  runtime-diff from unpatched(%) 
t0   441.78 0.32 3.4
pl   201.77 0.3213.6
pf0 6619.2218.99   -19.7
pf1 1800.7232.72   -78.0
---
unpatched- -O2

t0   427.21 0.42
pl   177.54 0.25
pf0 8250.4223.29
pf1 8206.0212.91

==

  3% slower for local 1*seqscan (2-parallel)
 14% slower for append-4*seqscan (no-prallel)
 19% faster for append-4*foreignscan (all scans on one connection)
 78% faster for append-4*foreignscan (scans have dedicate connection)

ExecProcNode might be able to be optimized a bit.
ExecAppend seems to need some fix.

Addition to the aboves, I will try reentrant ExecAsyncWaitForNode
or something.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

[HACKERS] use of SEQ_MINVALUE in btree_gin

[Peter Eisentraut]

btree_gin uses SEQ_MINVALUE as a way to get the smallest int64 value.
This is actually wrong because the smallest int64 value is
SEQ_MINVALUE-1, so this might be slightly broken.

The whole thing was done as a convenience when INT64_IS_BUSTED had to be
considered, but I think we can get rid of that now.  See attached
proposed patch.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/contrib/btree_gin/btree_gin.c b/contrib/btree_gin/btree_gin.c
index f74e912..030b610 100644
--- a/contrib/btree_gin/btree_gin.c
+++ b/contrib/btree_gin/btree_gin.c
@@ -223,10 +223,7 @@ GIN_SUPPORT(int4, false, leftmostvalue_int4, btint4cmp)
 static Datum
 leftmostvalue_int8(void)
 {
-   /*
-* Use sequence's definition to keep compatibility.
-*/
-   return Int64GetDatum(SEQ_MINVALUE);
+   return Int64GetDatum(PG_INT64_MIN);
 }
 
 GIN_SUPPORT(int8, false, leftmostvalue_int8, btint8cmp)
@@ -250,10 +247,7 @@ GIN_SUPPORT(float8, false, leftmostvalue_float8, 
btfloat8cmp)
 static Datum
 leftmostvalue_money(void)
 {
-   /*
-* Use sequence's definition to keep compatibility.
-*/
-   return Int64GetDatum(SEQ_MINVALUE);
+   return Int64GetDatum(PG_INT64_MIN);
 }
 
 GIN_SUPPORT(money, false, leftmostvalue_money, cash_cmp)

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

Re: [HACKERS] remove checkpoint_warning

[Craig Ringer]

On 11 July 2016 at 22:25, Stephen Frost  wrote:

> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> > Alvaro Herrera  writes:
> > > the checkpoint_warning feature was added by commit
> 2986aa6a668bce3cfb836
> > > in November 2002 when we didn't have any logging of checkpointing at
> > > all.  I propose to remove it: surely anyone who cares about analyzing
> > > checkpointing behavior nowadays is using the log_checkpoint feature
> > > instead, which contains much more detail.  The other one is just noise
> > > now, and probably ignored amidst the number of other warning traffic.
> >
> > Hmm, not sure.  ISTM log_checkpoint is oriented to people who know what
> > they are doing, whereas checkpoint_warning is more targeted to trying
> > to help people who don't.  Perhaps you could make an argument that
> > checkpoint_warning is useless because the people whom it's meant to help
> > won't notice the warning anyway --- but I doubt that it's been
> > "superseded" by log_checkpoint, because the latter would only be enabled
> > by people who already have a clue that checkpoint performance is
> something
> > to worry about.
> >
> > Or in short, this may be a fine change to make, but I don't like your
> > argument for it.
>
> I don't agree that it's sensible to get rid of.  Having just
> log_checkpoints will have the logs filled with checkpoints starting
> because of XLOG, but there's no indication of that being a bad thing.
>
>
Also, the warning is greppable-for and easily spotted by log ingesting
tools. I see no real reason to remove it.



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

Re: [HACKERS] Changing the result set to contain the cost of the optimizer's chosen plan

[Craig Ringer]

On 11 July 2016 at 23:29, Tom Lane  wrote:

> Srinivas Karthik V  writes:
> > Specifically, I have a Java program which calls
> > ResultSet rs = statement.executeQuery("explain select * from table");
> > I would like to change PostgreSQL such that ResultSet rs should contain a
> > field that contains also the cost of the optimizer chosen plan.
>
> Why do you need to change anything?  The cost is right there in the
> first line of the result text.  It might be easier to parse out if
> you use one of EXPLAIN's intended-to-be-machine-readable output
> formats, though. 


Yeah - if we were going to do this at all, it'd want to be output that
decomposes _all_ the explain output into columns.  But since we can emit
json, xml, etc, I don't really see the point.

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

Re: [HACKERS] remove checkpoint_warning

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
> On 2016-07-11 11:14:29 -0700, Peter Geoghegan wrote:
> > On Mon, Jul 11, 2016 at 7:25 AM, Stephen Frost  wrote:
> > >> Or in short, this may be a fine change to make, but I don't like your
> > >> argument for it.
> > >
> > > I don't agree that it's sensible to get rid of.  Having just
> > > log_checkpoints will have the logs filled with checkpoints starting
> > > because of XLOG, but there's no indication of that being a bad thing.
> > 
> > I agree. checkpoint_warning exists for the benefit of novice DBAs.
> > I've seen those warnings in customer logs on several occasions, at
> > least back when I was a consultant.
> 
> Note that the situation changed a bit with 9.5, because our defaults
> aren't absurdly conservative (checkpoint_segments = 3) anymore.

Agreed, but I don't think that means we'll never see that warning
again..

Thanks!

Stephen


signature.asc
Description: Digital signature

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

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > I agree with removing the source code field, though I did like the
> > suggestion mentioned elsewhere for having it shown when it's just a C
> > symbol but not otherwise.  If we can find a way to have the C symbol
> > shown when it's a C or internal function, I'm fine with that, but the
> > source code field having entier pl/sql and pl/pgsql functions in it
> > doesn't work and \sf should be used instead.
> 
> It would certainly be easy enough to do that, as long as you don't mind
> hard-wiring into psql the knowledge that "internal" and "C" are the
> languages to show prosrc for.  "Source code" would no longer be a very
> appropriate column name, though it already was not for these cases.
> I'd be inclined to call it "Internal name" instead.

That would certainly work for me.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] GiST index build versus NaN coordinates

[Tom Lane]

Emre Hasegeli  writes:
>> I think that probably the most reasonable answer is to replace all the
>> raw "double" comparisons in this code with float8_cmp_internal() or
>> something that's the moral equivalent of that.  Does anyone want to
>> propose something else?

> We can probably get away by changing the comparison on the GiST code.
> It is not likely to cause inconsistent results.  Comparisons with NaN
> coordinates don't return true to anything, anyway:

Yes, and that is itself inconsistent with the behavior of the primitive
float8 datatype:

regression=# select '4'::float8 < 'NaN'::float8;
 ?column? 
--
 t
(1 row)

I'm inclined to think that we ought to try to make NaNs in geometric types
work like float8 thinks they work, ie they compare higher than non-NaNs.
Yeah, it would make an IEEE-spec purist blanch, but there is no room for
unordered values in a datatype that you would like to be indexable, or
groupable.

> Is it reasonable to disallow NaN coordinates on the next major
> release.  Are there any reason to deal with them?

I don't see how we can do that; what would you do about tables already
containing NaNs?  Even without that consideration, assuming that there's
no way a NaN could creep in seems a pretty fragile assumption, considering
that operations like Infinity/Infinity will produce one.

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] GiST index build versus NaN coordinates

[Emre Hasegeli]

> I think that probably the most reasonable answer is to replace all the
> raw "double" comparisons in this code with float8_cmp_internal() or
> something that's the moral equivalent of that.  Does anyone want to
> propose something else?

We can probably get away by changing the comparison on the GiST code.
It is not likely to cause inconsistent results.  Comparisons with NaN
coordinates don't return true to anything, anyway:

# select '(3,4),(nan,5)'::box = '(3,4),(nan,5)'::box;
 ?column?
--
 f
(1 row)

# select '(3,4),(nan,5)'::box < '(3,4),(nan,5)'::box;
 ?column?
--
 f
(1 row)

# select '(3,4),(nan,5)'::box > '(3,4),(nan,5)'::box;
 ?column?
--
 f
(1 row)

> More generally, this example makes me fearful that NaN coordinates in
> geometric values are likely to cause all sorts of issues.  It's too late
> to disallow them, probably, but I wonder how can we identify other bugs
> of this ilk.

Is it reasonable to disallow NaN coordinates on the next major
release.  Are there any reason to deal with them?


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

Re: [HACKERS] IMPORT FOREIGN SCHEMA can't be run in in pl/pgsql due to INTO

[Tom Lane]

Merlin Moncure  writes:
> Currently pl/pgsql interprets the mandatory INTO of IMPORT FOREIGN
> SCHEMA as INTO variable.

Ugh, that's definitely a bug.

> I estimate this to be minor oversight in
> pl/pgsql parsing with respect to the introduction of this statement.

While we can certainly hack it by something along the lines of not
recognizing INTO when the first token was IMPORT, the whole thing
seems awfully messy and fragile.  And it will certainly break again
the next time somebody decides that INTO is le mot juste in some new
SQL command.  I wish we could think of a safer, more future-proof
solution.  I have no idea what that would be, though, short of
deprecating INTO altogether.

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

[HACKERS] IMPORT FOREIGN SCHEMA can't be run in in pl/pgsql due to INTO

[Merlin Moncure]

Currently pl/pgsql interprets the mandatory INTO of IMPORT FOREIGN
SCHEMA as INTO variable.  I estimate this to be minor oversight in
pl/pgsql parsing with respect to the introduction of this statement.
Assuming it's easily fixed, would a patch to fix pl/pgsql parsing be
accepted?

merlin


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

Re: [HACKERS] Disable WAL completely - Performance and Persistency research

[Jeff Janes]

On Thu, Jul 7, 2016 at 1:01 AM, Netanel Katzburg  wrote:
> Hi All,
>
> As part of my masters at TAU, I'm currently conducting some research
> regarding new persistent memory technology.
> I'm using PG for this research and would like to better understand some of
> the performance bottlenecks.
> For this reason I'm trying to disable the WAL completely, using some hacks
> on the source code and compiling my own version.
>
> So what I'm actually looking for, is some guidance about a simple way to:
>
> 1. Disable the WAL by not writing anything to the xlog directory. I don't
> care about recovery/fault tolerance or PITR/ replication etc at the moment.
> I'm aware that the WAL and checkpoint are bind in many ways and are crucial
> for PG core features.
> I tried changing the status of all tables to "unlogged" tables by changing
> RelationNeedsWAL MACRO, as well as "needs_wal" parameter at storage.c.
> But, got no performance benefit, so I guess this was the wrong place to
> change.
>
> 2. Cancel the locking around WAL files  - I don't care about corrupted files
> at the moment, I just want to see what is the maximum performance benefit
> that I can get without lock contention.
>
> Any guidance on how to do so would be appreciated :)

I have a very old patch which introduces a config variable (JJNOWAL)
that skips all WAL, except for the WAL of certain checkpoints (which
are needed for initdb and to restart the server after a clean
shutdown).

I have rebased it up to HEAD.  It seems to work, but I haven't tested
thoroughly that it still does the correct thing in every corner case.
(a lot of changes have been made to xlog code since last time I used
this.)

Obviously if the server goes down uncleanly while this setting is
active, it will not be usable anymore.

Cheers,

Jeff


nowal.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] remove checkpoint_warning

[Andres Freund]

On 2016-07-11 11:14:29 -0700, Peter Geoghegan wrote:
> On Mon, Jul 11, 2016 at 7:25 AM, Stephen Frost  wrote:
> >> Or in short, this may be a fine change to make, but I don't like your
> >> argument for it.
> >
> > I don't agree that it's sensible to get rid of.  Having just
> > log_checkpoints will have the logs filled with checkpoints starting
> > because of XLOG, but there's no indication of that being a bad thing.
> 
> I agree. checkpoint_warning exists for the benefit of novice DBAs.
> I've seen those warnings in customer logs on several occasions, at
> least back when I was a consultant.

Note that the situation changed a bit with 9.5, because our defaults
aren't absurdly conservative (checkpoint_segments = 3) anymore.


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

Re: [HACKERS] remove checkpoint_warning

[Peter Geoghegan]

On Mon, Jul 11, 2016 at 7:25 AM, Stephen Frost  wrote:
>> Or in short, this may be a fine change to make, but I don't like your
>> argument for it.
>
> I don't agree that it's sensible to get rid of.  Having just
> log_checkpoints will have the logs filled with checkpoints starting
> because of XLOG, but there's no indication of that being a bad thing.

I agree. checkpoint_warning exists for the benefit of novice DBAs.
I've seen those warnings in customer logs on several occasions, at
least back when I was a consultant.

-- 
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] Showing parallel status in \df+

[Tom Lane]

Stephen Frost  writes:
> I agree with removing the source code field, though I did like the
> suggestion mentioned elsewhere for having it shown when it's just a C
> symbol but not otherwise.  If we can find a way to have the C symbol
> shown when it's a C or internal function, I'm fine with that, but the
> source code field having entier pl/sql and pl/pgsql functions in it
> doesn't work and \sf should be used instead.

It would certainly be easy enough to do that, as long as you don't mind
hard-wiring into psql the knowledge that "internal" and "C" are the
languages to show prosrc for.  "Source code" would no longer be a very
appropriate column name, though it already was not for these cases.
I'd be inclined to call it "Internal name" instead.

regards, tom lane


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

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

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Michael Paquier  writes:
> > On Mon, Jul 11, 2016 at 12:42 AM, Tom Lane  wrote:
> >> (Of course, if we were to get rid of "Source code", the point
> >> would be moot ...)
> 
> > I still think that having source code is useful for debugging, so I
> > left it out. Note for the committer who will perhaps pick up this
> > patch: I left out "Source Code", but feel free to remove it if you
> > think the contrary. It is easier to remove code than adding it back.
> 
> I still think removing it would make \df+ output substantially more
> readable whenever any PLs are involved.  I'm tempted to propose adding
> something like \df++ to include the source code for those who really
> want that.
> 
> However, by my count the vote is two in favor of removing it versus two
> against, which is certainly not any kind of consensus, so nothing is going
> to happen on that front right away.  Meanwhile, we definitely need to get
> the "Parallel" column into 9.6, so I'll review and push the rest of the
> changes.

I agree with removing the source code field, though I did like the
suggestion mentioned elsewhere for having it shown when it's just a C
symbol but not otherwise.  If we can find a way to have the C symbol
shown when it's a C or internal function, I'm fine with that, but the
source code field having entier pl/sql and pl/pgsql functions in it
doesn't work and \sf should be used instead.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Changing the result set to contain the cost of the optimizer's chosen plan

[Tom Lane]

Srinivas Karthik V  writes:
> Specifically, I have a Java program which calls
> ResultSet rs = statement.executeQuery("explain select * from table");
> I would like to change PostgreSQL such that ResultSet rs should contain a
> field that contains also the cost of the optimizer chosen plan.

Why do you need to change anything?  The cost is right there in the
first line of the result text.  It might be easier to parse out if
you use one of EXPLAIN's intended-to-be-machine-readable output
formats, though.

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] [CF2016-9] Allow spaces in working path on tap-tests

[Tom Lane]

Michael Paquier  writes:
> On Sun, Jul 10, 2016 at 11:52 PM, Tom Lane  wrote:
>> Yes, please --- I thought it'd all gotten done.

> OK, here are patches for 9.1, 9.2 and 9.3.

Pushed, thanks.

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

[HACKERS] Changing the result set to contain the cost of the optimizer's chosen plan

[Srinivas Karthik V]

Hello,

I am Srinivas and have been working inside PostgreSQL (mostly in the
optimizer module and few times in the executor module). I would like to
change PostgreSQL code such that it also returns the cost of the optimizer
chosen plan to the Java program (through JDBC) as part of the result set.

Specifically, I have a Java program which calls

ResultSet rs = statement.executeQuery("explain select * from table");

I would like to change PostgreSQL such that ResultSet rs should contain a
field that contains also the cost of the optimizer chosen plan.

If the above is not possible, also please let me know if there is some
other way I could get the cost of the plan in a single call to engine
through Java program.

Regards,
Srinivas Karthik

Re: [HACKERS] pg_xlogdump bad error msg?

[Andres Freund]

On 2016-07-11 13:36:37 +0200, Magnus Hagander wrote:
> When you don't specify a start segment to pg_xlogdump, you get:
> 
> pg_xlogdump: no start log position given in range mode.
> 
> 
> What is "range mode", and is there any other mode for pg_xlogdump? Should
> it not just be "no start log position or segment given" or something like
> that?

Works for me as well.

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

[Masahiko Sawada]

On Fri, Jul 8, 2016 at 10:24 PM, Amit Kapila  wrote:
> On Thu, Jul 7, 2016 at 12:34 PM, Masahiko Sawada  
> wrote:
>> Than you for reviewing!
>>
>> On Thu, Jul 7, 2016 at 7:06 AM, Andres Freund  wrote:
>>> On 2016-07-05 23:37:59 +0900, Masahiko Sawada wrote:
 diff --git a/src/backend/access/heap/heapam.c 
 b/src/backend/access/heap/heapam.c
 index 57da57a..fd66527 100644
 --- a/src/backend/access/heap/heapam.c
 +++ b/src/backend/access/heap/heapam.c
 @@ -3923,6 +3923,17 @@ l2:

   if (need_toast || newtupsize > pagefree)
   {
 + /*
 +  * To prevent data corruption due to updating old tuple by
 +  * other backends after released buffer
>>>
>>> That's not really the reason, is it? The prime problem is crash safety /
>>> replication. The row-lock we're faking (by setting xmax to our xid),
>>> prevents concurrent updates until this backend died.
>>
>> Fixed.
>>
 , we need to emit that
 +  * xmax of old tuple is set and clear visibility map bits if
 +  * needed before releasing buffer. We can reuse xl_heap_lock
 +  * for this purpose. It should be fine even if we crash 
 midway
 +  * from this section and the actual updating one later, since
 +  * the xmax will appear to come from an aborted xid.
 +  */
 + START_CRIT_SECTION();
 +
   /* Clear obsolete visibility flags ... */
   oldtup.t_data->t_infomask &= ~(HEAP_XMAX_BITS | HEAP_MOVED);
   oldtup.t_data->t_infomask2 &= ~HEAP_KEYS_UPDATED;
 @@ -3936,6 +3947,46 @@ l2:
   /* temporarily make it look not-updated */
   oldtup.t_data->t_ctid = oldtup.t_self;
   already_marked = true;
 +
 + /* Clear PD_ALL_VISIBLE flags */
 + if (PageIsAllVisible(BufferGetPage(buffer)))
 + {
 + all_visible_cleared = true;
 + PageClearAllVisible(BufferGetPage(buffer));
 + visibilitymap_clear(relation, 
 BufferGetBlockNumber(buffer),
 + vmbuffer);
 + }
 +
 + MarkBufferDirty(buffer);
 +
 + if (RelationNeedsWAL(relation))
 + {
 + xl_heap_lock xlrec;
 + XLogRecPtr recptr;
 +
 + /*
 +  * For logical decoding we need combocids to 
 properly decode the
 +  * catalog.
 +  */
 + if (RelationIsAccessibleInLogicalDecoding(relation))
 + log_heap_new_cid(relation, );
>>>
>>> Hm, I don't see that being necessary here. Row locks aren't logically
>>> decoded, so there's no need to emit this here.
>>
>> Fixed.
>>
>>>
 + /* Clear PD_ALL_VISIBLE flags */
 + if (PageIsAllVisible(page))
 + {
 + Buffer  vmbuffer = InvalidBuffer;
 + BlockNumber block = BufferGetBlockNumber(*buffer);
 +
 + all_visible_cleared = true;
 + PageClearAllVisible(page);
 + visibilitymap_pin(relation, block, );
 + visibilitymap_clear(relation, block, vmbuffer);
 + }
 +
>>>
>>> That clears all-visible unnecessarily, we only need to clear all-frozen.
>>>
>>
>> Fixed.
>>
>>>
 @@ -8694,6 +8761,23 @@ heap_xlog_lock(XLogReaderState *record)
   }
   HeapTupleHeaderSetXmax(htup, xlrec->locking_xid);
   HeapTupleHeaderSetCmax(htup, FirstCommandId, false);
 +
 + /* The visibility map need to be cleared */
 + if ((xlrec->infobits_set & XLHL_ALL_VISIBLE_CLEARED) != 0)
 + {
 + RelFileNode rnode;
 + Buffer  vmbuffer = InvalidBuffer;
 + BlockNumber blkno;
 + Relationreln;
 +
 + XLogRecGetBlockTag(record, 0, , NULL, );
 + reln = CreateFakeRelcacheEntry(rnode);
 +
 + visibilitymap_pin(reln, blkno, );
 + visibilitymap_clear(reln, blkno, vmbuffer);
 + PageClearAllVisible(page);
 + }
 +
>>>
>>>
   PageSetLSN(page, lsn);
   MarkBufferDirty(buffer);
   }
 diff --git a/src/include/access/heapam_xlog.h 
 b/src/include/access/heapam_xlog.h
 index a822d0b..41b3c54 100644
 --- a/src/include/access/heapam_xlog.h
 +++ 

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

[Tom Lane]

Michael Paquier  writes:
> On Mon, Jul 11, 2016 at 12:42 AM, Tom Lane  wrote:
>> (Of course, if we were to get rid of "Source code", the point
>> would be moot ...)

> I still think that having source code is useful for debugging, so I
> left it out. Note for the committer who will perhaps pick up this
> patch: I left out "Source Code", but feel free to remove it if you
> think the contrary. It is easier to remove code than adding it back.

I still think removing it would make \df+ output substantially more
readable whenever any PLs are involved.  I'm tempted to propose adding
something like \df++ to include the source code for those who really
want that.

However, by my count the vote is two in favor of removing it versus two
against, which is certainly not any kind of consensus, so nothing is going
to happen on that front right away.  Meanwhile, we definitely need to get
the "Parallel" column into 9.6, so I'll review and push the rest of the
changes.

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] sslmode=require fallback

[Magnus Hagander]

On Thu, Jun 23, 2016 at 1:50 AM, Bruce Momjian  wrote:

> On Thu, Jun 16, 2016 at 10:42:56AM +0200, Magnus Hagander wrote:
> > However, if this is the expected behavior, the documentation
> at https://
> > www.postgresql.org/docs/current/static/libpq-ssl.html should be
> updated to
> > make this more clear. It should be made clear that the existence of
> the
> > file ~/.postgresql/root.crt changes the behavior of sslmode=require
> and
> > sslmode=prefer.
> >
> >
> >
> > Agreed. It's basically backwards compatibility with something that was
> badly
> > documented in the first place :) That's not a particularly strong
> argument for
> > the way it is. Clarifying the documentation would definitely be a good
> > improvement.
>
> Does this have to remain backward-compatible forever?
>

In general no. But I think the problem here is that if somebody misses the
removal of something backwards compatible, it turns off their security.
Which is not good...

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] remove checkpoint_warning

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Alvaro Herrera  writes:
> > the checkpoint_warning feature was added by commit 2986aa6a668bce3cfb836
> > in November 2002 when we didn't have any logging of checkpointing at
> > all.  I propose to remove it: surely anyone who cares about analyzing
> > checkpointing behavior nowadays is using the log_checkpoint feature
> > instead, which contains much more detail.  The other one is just noise
> > now, and probably ignored amidst the number of other warning traffic.
> 
> Hmm, not sure.  ISTM log_checkpoint is oriented to people who know what
> they are doing, whereas checkpoint_warning is more targeted to trying
> to help people who don't.  Perhaps you could make an argument that
> checkpoint_warning is useless because the people whom it's meant to help
> won't notice the warning anyway --- but I doubt that it's been
> "superseded" by log_checkpoint, because the latter would only be enabled
> by people who already have a clue that checkpoint performance is something
> to worry about.
> 
> Or in short, this may be a fine change to make, but I don't like your
> argument for it.

I don't agree that it's sensible to get rid of.  Having just
log_checkpoints will have the logs filled with checkpoints starting
because of XLOG, but there's no indication of that being a bad thing.

Thanks!

Stephen


signature.asc
Description: Digital signature

[HACKERS] GiST index build versus NaN coordinates

[Tom Lane]

I looked into the problem reported in bug #14238,
https://www.postgresql.org/message-id/20160708151747.1426.60...@wrigleys.postgresql.org
The submitter was kind enough to give me a copy of the problem data,
and it turns out that the issue is that a few of the boxes contain
NaN coordinates.  Armed with that knowledge, it's trivial to reproduce:

regression=# create table foo (f1 box);
CREATE TABLE
regression=# insert into foo values ('(3,4),(nan,5)');
INSERT 0 1
regression=# insert into foo select box(point(x,x+1),point(x,x+1)) from 
generate_series(1,1000) x;
INSERT 0 1000
regression=# create index on foo using gist(f1);
-- hangs, does not respond to control-C

The infinite loop is at lines 613ff in gistproc.c: once rightLower
contains a NaN, the test

while (i1 < nentries && rightLower == intervalsLower[i1].lower)

can never succeed, so i1 never increments again and the loop cannot exit.

I think that probably the most reasonable answer is to replace all the
raw "double" comparisons in this code with float8_cmp_internal() or
something that's the moral equivalent of that.  Does anyone want to
propose something else?

More generally, this example makes me fearful that NaN coordinates in
geometric values are likely to cause all sorts of issues.  It's too late
to disallow them, probably, but I wonder how can we identify other bugs
of this ilk.

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] pg_hba_lookup function to get all matching pg_hba.conf entries

[Tom Lane]

Haribabu Kommi  writes:
> On Sat, Apr 9, 2016 at 6:36 AM, Tom Lane  wrote:
>> More generally, I'm not convinced about the use-case for this patch.
>> What problem is it supposed to help in dealing with, exactly?  Not syntax
>> errors in the hba file, evidently, since it doesn't make any attempt to
>> instrument the file parser.  And it's not very clear that it'll help
>> with "I can't connect", either, because if you can't connect you're
>> not going to be running this function.

> Apologies for replying an old thread.

> The main reason behind of this patch is for the administrators to control
> and verify the authentication mechanism that was deployed for several
> users in the database is correctly picking the assigned hba config or not?

That doesn't really answer my question: what is a concrete use-case for
this function?  Reproducing the same behavior that would happen during
a login attempt does not seem terribly useful from here, because you
could simply attempt to log in, instead.  As I said upthread, maybe we
need a bit more logging in the authentication logic, but that doesn't
seem to lead to wanting a SQL function.

What I actually think we could use is something like the pg_file_settings
view, but for pg_hba.conf.  In particular, pg_file_settings has a specific
charter of being able to detect syntax errors in not-yet-loaded config
files.  That seems like clearly useful functionality to me, but we don't
have it for pg_hba.conf (and this patch didn't add it).

regards, tom lane


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

Re: [HACKERS] Disable WAL completely - Performance and Persistency research

[Netanel Katzburg]

Hi,

You were right, the method you described worked well. Thanks you!

But so far, could not get any noticeable improvement in Number of
transactions / latency.

I have tried:
1.  At xlog.c, CopyXLogRecordToWAL(int write_len, bool isLogSwitch,
XLogRecData *rdata,
XLogRecPtr StartPos, XLogRecPtr EndPos), Commenting the memcpy syscall:

memcpy(currpos, rdata_data, rdata_len);


2. *XLogInsert(RmgrId rmid, uint8 info), t*he primary insert function in
xloginsert.c.
I tried commenting the following line at this function, so I can return a
phony pointer every time the function is called,  just as in bootstrap mode.


*if (IsBootstrapProcessingMode() && rmid != RM_XLOG_ID)*


3. At xlog.c, XLogInsertRecord(XLogRecData *rdata, XLogRecPtr fpw_lsn).
Commenting the WALInsertLock(s), as well as, commenting the spinlocks
around - Update shared LogwrtRqst. (Write, if we crossed page boundary.)

4. The last thing I tried regarding *XLogInsertRecord* function is to
comment:
"/*

* All the record data, including the header, is now ready to be

* inserted. Copy the record in the space reserved.

*/

CopyXLogRecordToWAL(rechdr->xl_tot_len, isLogSwitch, rdata,
StartPos, EndPos);"


Regards,
Netanel

On Mon, Jul 11, 2016 at 8:27 AM, Craig Ringer  wrote:

> On 10 July 2016 at 18:27, Netanel Katzburg  wrote:
>
>
>> BUT, both options are not good, as they are stopping me from even running
>> i*nitdb.*
>>
>>
>>
> The easiest path for testing will be to use an unpatched PostgreSQL to
> `initdb` and create a new database. Then start up a patched one that simply
> skips WAL writing against an already-`initdb`'d data directory.
>
> You probably won't be able to safely restart PostgreSQL, but all you're
> doing is performance analsys so one-shot operation on a throw-away data
> directory is probably fine.
>
>
> --
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>

Re: [HACKERS] [BUGS] BUG #14230: Wrong timeline returned by pg_stop_backup on a standby

[Magnus Hagander]

On Mon, Jul 11, 2016 at 3:05 PM, Michael Paquier 
wrote:

> On Mon, Jul 11, 2016 at 7:01 PM, Magnus Hagander 
> wrote:
> > But isn't this also a pre-existing bug in 9.5? Or did we change something
> > else that suddenly made it visible?
>
> What has been patched here is a defect caused by pg_start_backup(),
> and not pg_basebackup. In the case of the latter, ThisTimelineID gets
> set by GetStandbyFlushRecPtr() in the context of the WAL sender used
> to send the base backup. In short, this is only a defect of 9.6, where
> pg_start_backup() can be used on standbys for the first time for
> non-exclusive backups.
>
> So the issue does not actually pre-exist, GetStandbyFlushRecPtr()
> playing its role to set up the timeline ID.


Ah, that's where we gt it from. Gotcha, makes sense. Thanks for confirming!

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] [BUGS] BUG #14230: Wrong timeline returned by pg_stop_backup on a standby

[Michael Paquier]

On Mon, Jul 11, 2016 at 7:01 PM, Magnus Hagander  wrote:
> But isn't this also a pre-existing bug in 9.5? Or did we change something
> else that suddenly made it visible?

What has been patched here is a defect caused by pg_start_backup(),
and not pg_basebackup. In the case of the latter, ThisTimelineID gets
set by GetStandbyFlushRecPtr() in the context of the WAL sender used
to send the base backup. In short, this is only a defect of 9.6, where
pg_start_backup() can be used on standbys for the first time for
non-exclusive backups.

So the issue does not actually pre-exist, GetStandbyFlushRecPtr()
playing its role to set up the timeline ID.
-- 
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] PSA: Systemd will kill PostgreSQL

[Craig Ringer]

On 11 July 2016 at 17:49, Bernd Helmle  wrote:

>
>
> --On 11. Juli 2016 13:25:51 +0800 Craig Ringer 
> wrote:
>
> > Perhaps by uid threshold in login.defs?
>
> systemd's configure.ac has this:
>
> AC_ARG_WITH(system-uid-max,
> AS_HELP_STRING([--with-system-uid-max=UID]
> [Maximum UID for system users]),
> [SYSTEM_UID_MAX="$withval"],
> [SYSTEM_UID_MAX="`awk 'BEGIN { uid=999 } /^\s*SYS_UID_MAX\s+/ {
> uid=$2 } END { print uid }' /etc/login.defs 2>/dev/null || echo 999`"])
>
> so yes, it's the definition from there.
>

At COMPILE TIME?

W.T.F?

In the thread about this, someone even says that's a bad idea. The systemd
folks aren't really big on listening, though...

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

Re: [HACKERS] Disable WAL completely - Performance and Persistency research

[Craig Ringer]

On 11 July 2016 at 19:14, Netanel Katzburg  wrote:

> Hi,
>
> You were right, the method you described worked well. Thanks you!
>
> But so far, could not get any noticeable improvement in Number of
> transactions / latency.
>
>
What are you comparing to?


To start with, compare with:

- an unpatched PostgreSQL, configured normally, with normal logged tables

- an unpatched PostgreSQL, using UNLOGGED tables

- an unpatched PostgreSQL, using UNLOGGED tables and synchronous_commit =
off (or fsync=off, but remember, that disables data integrity protections
for system catalogs and everything).


Make sure you're introducing a suitably write-concurrent workload that
might actually be waiting on WAL.

Personally I'd be surprised if you saw any significant difference over
using UNLOGGED tables. That's why we have them ;)

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

Re: [HACKERS] \timing interval

[Peter Eisentraut]

On 7/9/16 4:00 PM, Andrew Gierth wrote:

How about

Time: 1234567.666 ms (20m 34.6s)


That's similar to what I had in mind, so I'd be happy with that.

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


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

[HACKERS] pg_xlogdump bad error msg?

[Magnus Hagander]

When you don't specify a start segment to pg_xlogdump, you get:

pg_xlogdump: no start log position given in range mode.


What is "range mode", and is there any other mode for pg_xlogdump? Should
it not just be "no start log position or segment given" or something like
that?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] [BUGS] BUG #14230: Wrong timeline returned by pg_stop_backup on a standby

[Amit Kapila]

On Mon, Jul 11, 2016 at 3:31 PM, Magnus Hagander  wrote:
>
>
> On Thu, Jul 7, 2016 at 8:38 AM, Michael Paquier 
> wrote:
>>
>> On Thu, Jul 7, 2016 at 12:57 AM, Marco Nenciarini
>>  wrote:
>> > After further analysis, the issue is that we retrieve the starttli from
>> > the ControlFile structure, but it was using ThisTimeLineID when writing
>> > the backup label.
>> >
>> > I've attached a very simple patch that fixes it.
>>
>> ThisTimeLineID is always set at 0 on purpose on a standby, so we
>> cannot rely on it (well it is set temporarily when recycling old
>> segments). At recovery when parsing the backup_label file there is no
>> actual use of the start segment name, so that's only a cosmetic
>> change. But surely it would be better to get that fixed, because
>> that's useful for debugging.
>>
>> While looking at your patch, I thought that it would have been
>> tempting to use GetXLogReplayRecPtr() to get the timeline ID when in
>> recovery, but what we really want to know here is the timeline of the
>> last REDO pointer, which is starttli, and that's more consistent with
>> the fact that we use startpoint when writing the backup_label file. In
>> short, +1 for this fix.
>>
>> I am adding that in the list of open items, adding Magnus in CC whose
>> commit for non-exclusive backups is at the origin of this defect.
>
>
> I agree this looks correct.
>
> But isn't this also a pre-existing bug in 9.5? Or did we change something
> else that suddenly made it visible?
>

I think the bug is pre-existing, but it becomes visible to user now by new API.


-- 
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] asynchronous and vectorized execution

[Kyotaro HORIGUCHI]

At Mon, 11 Jul 2016 17:10:11 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20160711.171011.133133724.horiguchi.kyot...@lab.ntt.co.jp>
> > Two things:
> > 
> > 1. That's not the scenario I'm talking about.  I'm concerned about
> > making sure that query plans that don't use asynchronous execution
> > don't get slower.
> 
> The first one donen't (select for t0) is that. It have any
> relation with asynchronous staff except the result_ready flag, a
> branch caused by it and calling ExecDispatchNode. The difference
> from the original is ExecProcNode uses ExecDispatchNode. Even
> ExecAsyncWaitForNode is not called.
> 
> > 2. I have to believe that's a defect in your implementation rather
> > than something intrinsic, or maybe your test scenario is bad.  It's
> > very hard - really impossible -  to believe that all queries involving
> > FDW pushdown are locally CPU-bound.
> 
> Sorry for hard-to-read result but the problem is not in a query
> involving FDW, but a query on a local table (but runs parallel
> seqscan).  The reason of the difference for the tests involving
> FDW should be local scans on the remote side.
> 
> Just reverting ExecProcNode and other related part doesn't change
> the situation. I proceed the confirmation reverting part by
> part.

Uggg. I had no difference even after finally bumped into master.
What is more strange, a binary built from what should be the same
"master" but extended by "git archive | tar" finishes the query
(select sum(a) from t0) in a half time to the master in my git
reposiotrty with -O2. In short, the patch doesn't seem to be the
cause of the difference.

I should investigate the difference between them, or begin again
with a clean environment..

Anyway I need some time to cool down..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] [BUGS] BUG #14230: Wrong timeline returned by pg_stop_backup on a standby

[Magnus Hagander]

On Thu, Jul 7, 2016 at 8:38 AM, Michael Paquier 
wrote:

> On Thu, Jul 7, 2016 at 12:57 AM, Marco Nenciarini
>  wrote:
> > After further analysis, the issue is that we retrieve the starttli from
> > the ControlFile structure, but it was using ThisTimeLineID when writing
> > the backup label.
> >
> > I've attached a very simple patch that fixes it.
>
> ThisTimeLineID is always set at 0 on purpose on a standby, so we
> cannot rely on it (well it is set temporarily when recycling old
> segments). At recovery when parsing the backup_label file there is no
> actual use of the start segment name, so that's only a cosmetic
> change. But surely it would be better to get that fixed, because
> that's useful for debugging.
>
> While looking at your patch, I thought that it would have been
> tempting to use GetXLogReplayRecPtr() to get the timeline ID when in
> recovery, but what we really want to know here is the timeline of the
> last REDO pointer, which is starttli, and that's more consistent with
> the fact that we use startpoint when writing the backup_label file. In
> short, +1 for this fix.
>
> I am adding that in the list of open items, adding Magnus in CC whose
> commit for non-exclusive backups is at the origin of this defect.
>

I agree this looks correct.

But isn't this also a pre-existing bug in 9.5? Or did we change something
else that suddenly made it visible?

-- 
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Re: [HACKERS] PSA: Systemd will kill PostgreSQL

[Bernd Helmle]

--On 11. Juli 2016 13:25:51 +0800 Craig Ringer 
wrote:

> Perhaps by uid threshold in login.defs?

systemd's configure.ac has this:

AC_ARG_WITH(system-uid-max,
AS_HELP_STRING([--with-system-uid-max=UID]
[Maximum UID for system users]),
[SYSTEM_UID_MAX="$withval"],
[SYSTEM_UID_MAX="`awk 'BEGIN { uid=999 } /^\s*SYS_UID_MAX\s+/ {
uid=$2 } END { print uid }' /etc/login.defs 2>/dev/null || echo 999`"])

so yes, it's the definition from there.

> But then what happens for people
> who're managing users via a directory, who need to avoid conflicting with
> host-local UIDs, but also need some of those users to have systemd
> "system user" like behaviour?

We had this in the past in some setups and this would add another reason
for unexpected headaches...

-- 
Thanks

Bernd


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

Re: [HACKERS] asynchronous and vectorized execution

[Kyotaro HORIGUCHI]

Hello,

At Thu, 7 Jul 2016 13:59:54 -0400, Robert Haas  wrote in 

> On Wed, Jul 6, 2016 at 3:29 AM, Kyotaro HORIGUCHI
>  wrote:
> > This seems to be a good opportunity to show this patch. The
> > attched patch set does async execution of foreignscan
> > (postgres_fdw) on the Robert's first infrastructure, with some
> > modification.
> 
> Cool.

Thank you.

> > ExecAsyncWaitForNode can get into an inifite-waiting by recursive
> > calls of ExecAsyncWaitForNode caused by ExecProcNode called from
> > async-unaware nodes. Such recursive calls cause a wait on
> > already-ready nodes.
> 
> Hmm, that's annoying.
> 
> > I solved that in the patch set by allocating a separate
> > async-execution context for every async-execution subtrees, which
> > is made by ExecProcNode, instead of one async-exec context for
> > the whole execution tree. This works fine but the way switching
> > contexts seems ugly.  This may also be solved by make
> > ExecAsyncWaitForNode return when no node to wait even if the
> > waiting node is not ready. This might keep the async-exec context
> > (state) simpler so I'll try this.
> 
> I think you should instead try to make ExecAsyncWaitForNode properly 
> reentrant.

I feel the same way. Will try to do that.

> > Does the ParallelWorkerSetLatchesForGroup use mutex or semaphore
> > or something like instead of latches?
> 
> Why would it do that?

I might misunderstand the original sentence but the reason of my
question there is that I didn't see the connection between "When
an executor node does something that might unblock other workers,
it calls ParallelWorkerSetLatchesForGroup()" and "and the async
stuff then tries calling all of the nodes in this array". I
supposed that the former takes place on each worker and the
latter should do the latter on the leader. So I asked the means
to signal the leader to do the latter thing. I should be wrong,
because I feel uneasy (or confused) with this statement..


> >> BTW, we also need to benchmark those changes to add the parent
> >> pointers and change the return conventions and see if they have any
> >> measurable impact on performance.
> >
> > I have to bring you a bad news.
> >
> > With the attached patch, an append on four foreign scans on one
> > server (at local) performs faster by about 10% and by twice for
> > three or four foreign scns on separate foreign servers
> > (connections) respectively, but only when compiled with -O0. I
> > found that it can take hopelessly small amount of advantage from
> > compiler optimization, while unpatched version gets faster.
> 
> Two things:
> 
> 1. That's not the scenario I'm talking about.  I'm concerned about
> making sure that query plans that don't use asynchronous execution
> don't get slower.

The first one donen't (select for t0) is that. It have any
relation with asynchronous staff except the result_ready flag, a
branch caused by it and calling ExecDispatchNode. The difference
from the original is ExecProcNode uses ExecDispatchNode. Even
ExecAsyncWaitForNode is not called.

> 2. I have to believe that's a defect in your implementation rather
> than something intrinsic, or maybe your test scenario is bad.  It's
> very hard - really impossible -  to believe that all queries involving
> FDW pushdown are locally CPU-bound.

Sorry for hard-to-read result but the problem is not in a query
involving FDW, but a query on a local table (but runs parallel
seqscan).  The reason of the difference for the tests involving
FDW should be local scans on the remote side.

Just reverting ExecProcNode and other related part doesn't change
the situation. I proceed the confirmation reverting part by
part.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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

Re: [HACKERS] [BUG] pg_basebackup from disconnected standby fails

[Michael Paquier]

On Mon, Jul 11, 2016 at 4:40 PM, Kyotaro HORIGUCHI
 wrote:
> That's true, but we don't always have a perfectly comprehensive
> test suite, consciously or unconsciously. The sentence was
> inattentive but the "bug" was just the negative comparable to
> "feature" in my mind. My point was the comparison between adding
> a test for a corner-case and its cost. It must be added if the
> fixed feature is fragile. It can be added it doesn't take a too
> long time to finish.

I'd just add it to be honest. Taking backups from standbys, with or
without the master being connected is a supported feature, and we want
to follow-up to be sure that it does not in the future. Having now the
infrastructure for more complex scenarios does not mean of course that
we need to test everything and all kind of things, of course, but here
the benefits are good compared to the cost that a single call of
pg_basebackup has in the complete the test suite run.
-- 
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] [BUG] pg_basebackup from disconnected standby fails

[Kyotaro HORIGUCHI]

Hello, thank you for the comment.

At Fri, 8 Jul 2016 14:42:20 -0400, Alvaro Herrera  
wrote in <20160708184220.GA733807@alvherre.pgsql>
> Kyotaro HORIGUCHI wrote:
> 
> > At Fri, 10 Jun 2016 17:39:59 +0900, Michael Paquier 
> >  wrote in 
> > 
> > > On Thu, Jun 9, 2016 at 9:55 PM, Kyotaro HORIGUCHI
> > >  wrote:
> 
> > > Indeed, and you could just do the following to reproduce the failure
> > > with the recovery test suite, so I would suggest adding this test in
> > > the patch:
> > > --- a/src/test/recovery/t/001_stream_rep.pl
> > > +++ b/src/test/recovery/t/001_stream_rep.pl
> > > @@ -24,6 +24,11 @@ $node_standby_1->start;
> > >  # pg_basebackup works on a standby).
> > >  $node_standby_1->backup($backup_name);
> > > 
> > > +# Take a second backup of the standby while the master is offline.
> > > +$node_master->stop;
> > > +$node_standby_1->backup('my_backup_2');
> > > +$node_master->start;
> > 
> > I'm not sure that adding the test case for a particular bug like
> > this is appropriate. But it would be acceptable because it
> > doesn't take long time and it is separate from standard checks.
> 
> The reason this test is appropiate is that it's testing a feature we
> want to support, namely that taking a backup from a standby works even
> when the master is stopped.  It's not a test for this particular bug,
> even though the feature doesn't work because of this bug.

That's true, but we don't always have a perfectly comprehensive
test suite, consciously or unconsciously. The sentence was
inattentive but the "bug" was just the negative comparable to
"feature" in my mind. My point was the comparison between adding
a test for a corner-case and its cost. It must be added if the
fixed feature is fragile. It can be added it doesn't take a too
long time to finish.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




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