Re: [HACKERS] Fix for OpenSSL error queue bug

[Tom Lane]

Peter Geoghegan  writes:
> On Fri, May 6, 2016 at 9:12 PM, Peter Eisentraut
>  wrote:
>> All committed.

> Thanks!

> This should no longer be referenced in the 9.6 release notes. It
> should just appear in the next batch of point releases. Tom has an
> sgml comment in the draft 9.6 release notes to that effect.

Yeah, I was just on that ...

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] Fix for OpenSSL error queue bug

[Peter Geoghegan]

On Fri, May 6, 2016 at 9:12 PM, Peter Eisentraut
 wrote:
> All committed.

Thanks!

This should no longer be referenced in the 9.6 release notes. It
should just appear in the next batch of point releases. Tom has an
sgml comment in the draft 9.6 release notes to that effect.

-- 
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] A population of population counts

[Tom Lane]

David Rowley  writes:
> I'd like to see us using those functions, when they're available and
> falling back on the array when they're not. Sounds like that would
> just be a new configure test. Perhaps a good home for some shared code
> would be numutils.c.

Meh --- numutils.c is about numbers.  Maybe "bitutils.c"?

Another point here is that there might easily be a use-case for such
functionality in frontend code, so I'd lean towards putting the support in
src/common if we do this.

regards, tom lane


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

Re: [HACKERS] Fix for OpenSSL error queue bug

[Peter Eisentraut]

On 4/25/16 8:37 PM, Peter Geoghegan wrote:

Attached is a series of patches for each supported release branch. As
discussed, I would like to target every released version of Postgres
with this bugfix. This is causing users real pain at the moment.


All committed.

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

Re: [HACKERS] A population of population counts

[David Rowley]

On 7 May 2016 at 12:41, Thomas Munro  wrote:
> Hi
>
> I noticed that we have three "number_of_ones" tables under contrib and
> two under src, and some new specially masked variants for visibility
> maps.
>
> Would it be an improvement if we just defined one table with external
> linkage, and accessed it via a macros/functions popcount_uint8, and
> wider versions _uint32, popcount_array(data, len) that sum the
> popcounts of their component bytes?
>
> Then there would be less duplication, and future opportunities to use
> fancy built-ins/assembler instructions/vectorisation in one central
> place, and to work in larger sizes than bytes.
>
> Perhaps we could also get rid of the new special masked popcount
> tables by masking the value we look up instead, eg walk through the
> page calling popcount_uint64(value & FROZEN_BITMASK_64).
>
> As for the rightmost_one_pos table in bitmapset.c, couldn't the
> various bms_XXX functions just use ffs(n) - 1 and work word-at-a-time?
>  That generates a bsf instruction at -O2 on this machine.
>
> The micro-optimisation opportunities probably don't matter, but I
> wondered if it might at least be interesting to delete a bunch of
> code, and re-use a standard interface for something that apparently
> several modules need to do.

I remember looking at GCC's __builtin_popcount() about 6 months ago
with thoughts about using it for bms_num_members(). I did see
performance improvements from micro-benchmarks to compare it to the
number_of_ones[] array, and saw a small improvement. Likely the
improvements I did see with those would actually have been better in a
real world case, since not having a number_of_ones[] array in a CPU
cache might be more useful for a workload with a more contended cache.

I'd like to see us using those functions, when they're available and
falling back on the array when they're not. Sounds like that would
just be a new configure test. Perhaps a good home for some shared code
would be numutils.c. I see the functions there declared in builtins.h
which I see used in contrib/spi. I think it could all be made to work
quite nicely with types like bitmapword just with some preprocessor
trickery.

I'd say go for it. Sounds like a like these would be some nice
reusable functions that we already have a suitable home for.

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


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

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

[Kevin Grittner]

On Fri, May 6, 2016 at 7:48 PM, Andres Freund  wrote:
> On 2016-05-06 19:43:24 -0500, Kevin Grittner wrote:
>> It's disappointing that I am not getting more consistent numbers,
>> but NUMA can be hard to manage that way.
>
> FWIW, in my experience, unless you disable autovacuum (or rather
> auto-analyze), the effects from non-predicable analyze runs with
> long-running snapshots are worse.  I mean the numa effects suck, but in
> r/w workload effects of analyze are often much worse.

Hm.  But the benefits of the patch are not there if autovacuum is
off.  I'm gonna need to ponder the best way to test given all that.

> That comment reminds me of a question I had: Did you consider the effect
> of this patch on analyze? It uses a snapshot, and by memory you've not
> built in a defense against analyze being cancelled.

Will need to check on that.

>> Will push shortly with the nit-pick fixes you requested.
>
> Cool.

Done.

I will be checking in on the buildfarm, but if it starts causing
problems while I'm, say, sleeping -- I won't be offended if someone
else reverts 7e3da1c4737fd6582e12c80983987e4d2cbc1d17.

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

Re: [HACKERS] what to revert

[Tomas Vondra]

Hi,

On 05/04/2016 12:42 AM, Andres Freund wrote:

On 2016-05-03 20:57:13 +0200, Tomas Vondra wrote:

On 05/03/2016 07:41 PM, Andres Freund wrote:

...

I'm pretty sure that I said that somewhere else at least once: But to
be absolutely clear, I'm *not* really concerned with the performance
with the feature turned off. I'm concerned about the performance with
it turned on.


If you tell me how to best test it, I do have a 4-socket server sitting idly
in the corner (well, a corner reachable by SSH). I can get us some numbers,
but I haven't been following the snapshot_too_old so I'll need some guidance
on what to test.


I think it'd be cool if you could test the effect of the feature in
read-only (and additionally read-mostly?) workload with various client
counts and snapshot_too_old values. For the latter maybe -1, 0, 10, 60
or such?  I've done so (accidentally comparing 0 and 1 instead of -1 and
1) on a two socket machine in:
www.postgresql.org/message-id/20160413171955.i53me46fqqhdl...@alap3.anarazel.de

It'd be very interesting to see how big the penalty is on a bigger box.


OK. I do have results from mater with different values for the GUC (-1, 
0, 10, 60), but I'm struggling with the reverts. Can you provide a patch 
against current master (commit 4bbc1a7e) that does the revert?


regards

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


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

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

[Andres Freund]

On 2016-05-06 19:43:24 -0500, Kevin Grittner wrote:
> It's disappointing that I am not getting more consistent numbers,
> but NUMA can be hard to manage that way.

FWIW, in my experience, unless you disable autovacuum (or rather
auto-analyze), the effects from non-predicable analyze runs with
long-running snapshots are worse.  I mean the numa effects suck, but in
r/w workload effects of analyze are often much worse.


That comment reminds me of a question I had: Did you consider the effect
of this patch on analyze? It uses a snapshot, and by memory you've not
built in a defense against analyze being cancelled.


> Will push shortly with the nit-pick fixes you requested.

Cool.

Greetings,

Andres Freund


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

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

[Kevin Grittner]

On Fri, May 6, 2016 at 5:07 PM, Andres Freund  wrote:

> On 2016-05-06 14:18:22 -0500, Kevin Grittner wrote:
>> I rebased the patch Ants posted (attached), and am running
>> benchmarks on a cthulhu (a big NUMA machine with 8 memory nodes).
>> Normally I wouldn't post results without a lot more data points
>> with multiple samples at each, but the initial results have me
>> wondering whether people would like to see this pushed later today
>> so that it has some time in the buildfarm and then into beta1.
>
> I think that generally would make sense. We quite possibly need some
> further changes, but it seems more likely that we can find them if the
> patch runs close to the disabled performance.

ok

>> Running the r/w TPC-B (sort of) load with scale, jobs, and threads
>> at 1000, and the database configured as I would for a production
>> server of that size, preliminary TPS results are:
>>
>> master, -1:  8158
>> master, 10min: 2019
>> Ants' patch, 10min: 7804
>
> That's rather nice.  Did you test read-only as well?

Not yet.  I don't trust short runs, so I've been going with -T2400;
with setup times and so on, that limits me to one run per hour of
time I book the machine, and I'm competing with others for that.  I
do plan to run read-only, too.

>From the 40 minute tests so far with Ants' patch (alternating settings):

old_snapshot_threshold = 10
7804
9524
9512

old_snapshot_threshold = -1
10421
8691
8977

It's disappointing that I am not getting more consistent numbers,
but NUMA can be hard to manage that way.

> If you'd feel more comfortable committing after I've run some
> performance tests, I could kick off some soon.

I think I should get it onto the buildfarm if we're going for
beta2, so there's time to recognize any problem (unlikely as that
*seems*) and back this out before beta if needed.  That said, all
additional data points welcome!

>> I can see arguments for tuning this far in time for the beta, as
>> well as the argument to wait until after the beta, so I'm just
>> throwing it out there to see what other people think.  I wouldn't
>> do it unless I have three runs at -1 and 10min with the patch, all
>> showing similar numbers.  If the BF chokes on it I would revert
>> this optimization attempt.
>
> +1 for going forward.  I'm still doubtful that it's a good idea to the
> map maintenance from GetSnapshotData(), but the issue becomes much less
> severe when addressed like this.
>
> The primary reasons why I'd like to move it is because of the
> significant amount of added gettimeofday() calls which are a real hog in
> some virtualized environments, and because I'm doubtful of tying the
> current time to the xmin horizon.

When I initially presented the proof of concept patch during the
9.5 development cycle it was based on transaction counts, and that
was the biggest criticism, and it came from many quarters.  Using
time was the big demand from just about everyone, and I'm not sure
how you do that without a mapping of time to xmin horizon.  If you
have some other idea, I'm all ears.

> Looks roughly sensible.

Will push shortly with the nit-pick fixes you requested.

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

[HACKERS] A population of population counts

[Thomas Munro]

Hi

I noticed that we have three "number_of_ones" tables under contrib and
two under src, and some new specially masked variants for visibility
maps.

Would it be an improvement if we just defined one table with external
linkage, and accessed it via a macros/functions popcount_uint8, and
wider versions _uint32, popcount_array(data, len) that sum the
popcounts of their component bytes?

Then there would be less duplication, and future opportunities to use
fancy built-ins/assembler instructions/vectorisation in one central
place, and to work in larger sizes than bytes.

Perhaps we could also get rid of the new special masked popcount
tables by masking the value we look up instead, eg walk through the
page calling popcount_uint64(value & FROZEN_BITMASK_64).

As for the rightmost_one_pos table in bitmapset.c, couldn't the
various bms_XXX functions just use ffs(n) - 1 and work word-at-a-time?
 That generates a bsf instruction at -O2 on this machine.

The micro-optimisation opportunities probably don't matter, but I
wondered if it might at least be interesting to delete a bunch of
code, and re-use a standard interface for something that apparently
several modules need to do.

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

[HACKERS] "Allow usage of huge maintenance_work_mem for GIN build" patch

[Peter Geoghegan]

I noticed that commit 30bb26b5 ("Allow usage of huge
maintenance_work_mem for GIN build") made the following modification:

--- a/src/include/access/gin_private.h
+++ b/src/include/access/gin_private.h
@@ -903,7 +903,7 @@ typedef struct GinEntryAccumulator
 typedef struct
 {
GinState   *ginstate;
-   longallocatedMemory;
+   SizeallocatedMemory;
GinEntryAccumulator *entryallocator;
uint32  eas_used;
RBTree *tree;

Are you sure this is safe, Teodor? I don't have time to study the
patch in detail, but offhand I think that it might have been better to
make allocatedMemory of type int64, just like the tuplesort.c memory
accounting variables are post-MaxAllocHuge. It's not obvious to me
that this variable isn't allowed to occasionally become negative, just
like in tuplesort.c. It looks like that *might* be true -- ginbulk.c
may let allocatedMemory go negative for a period, which would now be
broken.

If you did make this exact error, you would not be the first. If it
isn't actually broken, perhaps you should still make this change,
simply on general principle. I'd like to hear other opinions on that,
though.

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

[HACKERS] First-draft release notes for next week's back-branch releases

[Tom Lane]

If you're not tired of reviewing release notes (I'm sure getting a bit
tired of writing them), see

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=eb7de00ac2d282263541ece849ec71e2809e9467

guaibasaurus should have 'em up on the web in an hour or so, too, at

http://www.postgresql.org/docs/devel/static/release-9-5-3.html

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

[Alvaro Herrera]

Alvaro Herrera wrote:

> We touched this question in connection with multixact freezing and
> wraparound.  Testers seem to want to be given a script that they can
> install and run, then go for a beer and get back to a bunch of errors to
> report.

Here I spent some time trying to explain what to test to try and find
certain multixact bugs
http://www.postgresql.org/message-id/20150605213832.gz133...@postgresql.org

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


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

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

[Andres Freund]

Hi,

On 2016-05-06 14:18:22 -0500, Kevin Grittner wrote:
> I rebased the patch Ants posted (attached), and am running
> benchmarks on a cthulhu (a big NUMA machine with 8 memory nodes).
> Normally I wouldn't post results without a lot more data points
> with multiple samples at each, but the initial results have me
> wondering whether people would like to see this pushed later today
> so that it has some time in the buildfarm and then into beta1.

I think that generally would make sense. We quite possibly need some
further changes, but it seems more likely that we can find them if the
patch runs close to the disabled performance.


> Running the r/w TPC-B (sort of) load with scale, jobs, and threads
> at 1000, and the database configured as I would for a production
> server of that size, preliminary TPS results are:
> 
> master, -1:  8158
> master, 10min: 2019
> Ants' patch, 10min: 7804

That's rather nice.  Did you test read-only as well?


If you'd feel more comfortable committing after I've run some
performance tests, I could kick off some soon.


> I can see arguments for tuning this far in time for the beta, as
> well as the argument to wait until after the beta, so I'm just
> throwing it out there to see what other people think.  I wouldn't
> do it unless I have three runs at -1 and 10min with the patch, all
> showing similar numbers.  If the BF chokes on it I would revert
> this optimization attempt.

+1 for going forward.  I'm still doubtful that it's a good idea to the
map maintenance from GetSnapshotData(), but the issue becomes much less
severe when addressed like this.

The primary reasons why I'd like to move it is because of the
significant amount of added gettimeofday() calls which are a real hog in
some virtualized environments, and because I'm doubtful of tying the
current time to the xmin horizon.


> diff --git a/src/backend/utils/time/snapmgr.c 
> b/src/backend/utils/time/snapmgr.c
> index e1551a3..b7d965a 100644
> --- a/src/backend/utils/time/snapmgr.c
> +++ b/src/backend/utils/time/snapmgr.c
> @@ -80,8 +80,11 @@ typedef struct OldSnapshotControlData
>*/
>   slock_t mutex_current;  /* protect current 
> timestamp */
>   int64   current_timestamp;  /* latest snapshot 
> timestamp */
> - slock_t mutex_latest_xmin;  /* protect latest 
> snapshot xmin */
> + slock_t mutex_latest_xmin;  /* protect latest 
> snapshot xmin
> + 
>  * and next_map_update
> + 
>  */
>   TransactionId latest_xmin;  /* latest snapshot xmin 
> */
> + int64   next_map_update;/* latest snapshot 
> valid up to */
>   slock_t mutex_threshold;/* protect threshold 
> fields */
>   int64   threshold_timestamp;/* earlier snapshot is old */
>   TransactionId threshold_xid;/* earlier xid may be gone */

Overly nitpickily I'd refer to the actual variable name (instead of
"latest snapshot xmin") in the mutex_latest_xmin comment.


>   if (!same_ts_as_threshold)
>   {
> + if (ts == update_ts)
> + {
> + xlimit = latest_xmin;
> + if (NormalTransactionIdFollows(xlimit, 
> recentXmin))
> + SetOldSnapshotThresholdTimestamp(ts, 
> xlimit);
> + }
> + else
> + {
>   LWLockAcquire(OldSnapshotTimeMapLock, LW_SHARED);
>  
>   if (oldSnapshotControl->count_used > 0

I guess it's just an issue in my mail-reader, but the indentation looks
funky here.


Looks roughly sensible.


Greetings,

Andres Freund


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

Re: [HACKERS] Reviewing freeze map code

[Andres Freund]

On 2016-05-07 10:00:27 +1200, Thomas Munro wrote:
> On Sat, May 7, 2016 at 8:34 AM, Robert Haas  wrote:
> >> Did somebody verify the new contents are correct?
> >
> > I admit that I didn't.  It seemed like an unlikely place for a goof,
> > but I guess we should verify.
> 
> Looks correct.  The tables match the output of the attached script.

Great!


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

[Thomas Munro]

On Sat, May 7, 2016 at 8:34 AM, Robert Haas  wrote:
> On Mon, May 2, 2016 at 8:25 PM, Andres Freund  wrote:
>> +static const uint8 number_of_ones_for_visible[256] = {
>> ...
>> +};
>> +static const uint8 number_of_ones_for_frozen[256] = {
>> ...
>>  };
>>
>> Did somebody verify the new contents are correct?
>
> I admit that I didn't.  It seemed like an unlikely place for a goof,
> but I guess we should verify.

Looks correct.  The tables match the output of the attached script.

-- 
Thomas Munro
http://www.enterprisedb.com
def popcount(value):
  return bin(value).count("1")

def make_popcount_table(mask):
  for high_nibble in range(0, 16):
line = "\t"
for low_nibble in range(0, 16):
  value = (high_nibble << 4) | low_nibble
  line += str(popcount(value & mask))
  if low_nibble != 15:
line += ", "
  elif high_nibble != 15:
line += ","
print line

if __name__ == "__main__":
  print "static const uint8 number_of_ones_for_visible[256] = {"
  make_popcount_table(0b01010101) # for each bit pair, the lower bit
  print "};"
  print "static const uint8 number_of_ones_for_frozen[256] = {"
  make_popcount_table(0b10101010) # for each bit pair, the higher bit
  print "};"

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

[Peter Geoghegan]

On Fri, May 6, 2016 at 2:49 PM, Andres Freund  wrote:
>> Jeff Janes has done astounding work in these matters.  (I don't think
>> we credit him enough for that.)
>
> +many.

Agreed. I'm a huge fan of what Jeff has been able to do in this area.
I often say so. It would be even better if Jeff's approach to testing
was followed as an example by other people, but I wouldn't bet on it
ever happening. It requires real persistence and deep understanding to
do well.

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

[Andres Freund]

On 2016-05-06 18:31:03 -0300, Alvaro Herrera wrote:
> I don't know what happens when the freeze_table_age threshold is
> reached.

We scan all non-frozen pages, whereas we earlier had to scan all pages.

That's really both the significant benefit, and the danger. Because if
we screw up the all-frozen bits in the visibilitymap, we'll be screwed
soon after.

> Do we scan the whole table when that happens?

No, there's atm no way to force a whole-table vacuum, besides manually
rm'ing the _vm fork.


> Another question on this feature is what happens with the table age
> (relfrozenxid, relminmxid) when the table is not wholly scanned by
> vacuum.

Basically we increase the horizons whenever scanning all pages that are
not known to be frozen (+ potentially some frozen ones due to the
skipping logic). Without that there'd really not be a point in the
freeze map feature, as we'd continue to have the expensive anti
wraparound vacuums.

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

[Joshua D. Drake]

On 05/06/2016 02:48 PM, Andres Freund wrote:

On 2016-05-06 14:39:57 -0700, Joshua D. Drake wrote:



Yes, this is true but with a proper testing framework, I don't need a 15
minute break. I need 1 hour to configure, the rest just "happens" and
reports back.


That only works if somebody writes such tests.


Agreed.


And in that case the
tester having run will often suffice (until related changes are being
made). I'm not arguing against introducing more tests into the codebase
- I rather fervently for that. But that really isn't what's going to
avoid issues like this feature (or multixact) causing problems, because
those tests will just test what the author thought of.



Good point. I am not sure how to address the alternative though.




You want me (or people like me) to test more? Give us an easy way to
do it.


Useful additional testing and easy just don't go well together. By the
time I have made it easy I've done the testing that's needed.


I don't know that I can agree with this. A proper harness allows you to
execute: go.sh and boom... 2, 4, even 8 hours later you get a report. I will
not argue that it isn't easy to implement but I know it can be done.


The problem is that the contents of go.sh are the much more relevant
part than the 8 hours.


True.

Please don't misunderstand, I am not saying this is "easy". I just hope 
that it is something we work for.


Sincerely,

JD



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


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

Re: [HACKERS] Reviewing freeze map code

[Andres Freund]

On 2016-05-06 18:36:52 -0300, Alvaro Herrera wrote:
> Andres Freund wrote:
> 
> > On 2016-05-06 14:17:13 -0700, Joshua D. Drake wrote:
> > > How do I test?
> > > 
> > > Is there a script I can run?
> > 
> > Unfortunately there's few interesting things to test with pre-made
> > scripts. There's no relevant OS dependency here, so each already
> > existing test doesn't really lead to significantly increased coverage
> > being run by other people.  Generally, when testing for correctness
> > issues, it's often of limited benefit to run tests written by the author
> > of reviewer - such scripts will usually just test things either has
> > thought of.  The dangerous areas are the ones neither author or reviewer
> > has considered.
> 
> We touched this question in connection with multixact freezing and
> wraparound.  Testers seem to want to be given a script that they can
> install and run, then go for a beer and get back to a bunch of errors to
> report.  But it doesn't work that way; writing a useful test script
> requires a lot of effort.

Right. And once written, often enough running it on a lot more instances
only marginally increases the coverage.


> Jeff Janes has done astounding work in these matters.  (I don't think
> we credit him enough for that.)

+many.


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

[Andres Freund]

On 2016-05-06 14:39:57 -0700, Joshua D. Drake wrote:
> > > What are we looking for exactly?
> > 
> > Data corruption, efficiency problems.
> > 
> 
> I am really not trying to be difficult here but Data Corruption is an easy
> one... what is the metric we accept as an efficiency problem?

That's indeed not easy to define.  In this case I'd say vacuums taking
longer, index only scans being slower, more WAL being generated would
count?


> > I think tests without reading the code are quite sensible and
> > important. And it perfectly makes sense to ask for information about
> > what to test.  But fundamentally testing is a lot of work, as is writing
> > and reviewing code; unless you're really really good at destructive
> > testing, you won't find much in a 15 minute break.
> > 
> 
> Yes, this is true but with a proper testing framework, I don't need a 15
> minute break. I need 1 hour to configure, the rest just "happens" and
> reports back.

That only works if somebody writes such tests. And in that case the
tester having run will often suffice (until related changes are being
made). I'm not arguing against introducing more tests into the codebase
- I rather fervently for that. But that really isn't what's going to
avoid issues like this feature (or multixact) causing problems, because
those tests will just test what the author thought of.


> > > You want me (or people like me) to test more? Give us an easy way to
> > > do it.
> > 
> > Useful additional testing and easy just don't go well together. By the
> > time I have made it easy I've done the testing that's needed.
> 
> I don't know that I can agree with this. A proper harness allows you to
> execute: go.sh and boom... 2, 4, even 8 hours later you get a report. I will
> not argue that it isn't easy to implement but I know it can be done.

The problem is that the contents of go.sh are the much more relevant
part than the 8 hours.


Greetings,

Andres Freund


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

Re: [HACKERS] [COMMITTERS] pgsql: Add TAP tests for pg_dump

[Stephen Frost]

Tom, all,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> If you like, you can try the @contrib_excludes addition that was mentioned
> before and see if that fixes it.  But if it doesn't, it's time to cut our
> losses.

Alright, it certainly *appears* to be working.  All of the Windows
buildfarm animals which have reported back since I added test_pg_dump to
that list have been green.  There are still a few boxes outstanding, but
that seems like a good sign too- they failed pretty quickly before.

I'm saying all this because I have to step out for a couple of hours.
I'll keep an eye on it and will be around later tonight to revert the
tests if necessary, but I'm not going to complain if someone else
chooses to do so if those last few Windows boxes come back red again
before I'm around again.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Reviewing freeze map code

[Joshua D. Drake]

On 05/06/2016 02:29 PM, Andres Freund wrote:

Hi,


On 2016-05-06 14:17:13 -0700, Joshua D. Drake wrote:

How do I test?

Is there a script I can run?


Unfortunately there's few interesting things to test with pre-made
scripts. There's no relevant OS dependency here, so each already
existing test doesn't really lead to significantly increased coverage
being run by other people.  Generally, when testing for correctness
issues, it's often of limited benefit to run tests written by the author
of reviewer - such scripts will usually just test things either has
thought of.  The dangerous areas are the ones neither author or reviewer
has considered.


I can't argue with that.





Are there specific things I can do to try and break it?


Upgrade clusters using pg_upgrade and make sure things like index only
scans still work and yield correct data.  Set up workloads that involve
freezing, and check that less WAL (and not more!) is generated with 9.6
than with 9.5.  Make sure queries still work.



What are we looking for exactly?


Data corruption, efficiency problems.



I am really not trying to be difficult here but Data Corruption is an 
easy one... what is the metric we accept as an efficiency problem?





A lot of -hackers seem to forget that although we have 100 -hackers, we have
1 "consultant/practitioners". Could I read the code and with a weekend
of WTF and -hackers questions figure out what is going on, yes but a lot of
people couldn't and I don't have the time.


I think tests without reading the code are quite sensible and
important. And it perfectly makes sense to ask for information about
what to test.  But fundamentally testing is a lot of work, as is writing
and reviewing code; unless you're really really good at destructive
testing, you won't find much in a 15 minute break.



Yes, this is true but with a proper testing framework, I don't need a 15 
minute break. I need 1 hour to configure, the rest just "happens" and 
reports back.


I have cycles to test, I have team members to help test (as do *lots* of 
other people) but sometimes we just get lost in how to help.





You want me (or people like me) to test more? Give us an easy way to
do it.


Useful additional testing and easy just don't go well together. By the
time I have made it easy I've done the testing that's needed.


I don't know that I can agree with this. A proper harness allows you to 
execute: go.sh and boom... 2, 4, even 8 hours later you get a report. I 
will not argue that it isn't easy to implement but I know it can be done.



Otherwise, we do what we can, which is try and interface on the things that
will directly and immediately affect us (like keywords and syntax).


The amount of bikeshedding on -hackers steals energy and time for
actually working on stuff, including testing. So I have little sympathy
for the amount of bike shedding done.


Insuring a reasonable and thought out interface for users is not bike 
shedding, it is at least as important and possibly more important than 
any feature we add.


Sincerely,

JD



Greetings,

Andres Freund




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


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

Re: [HACKERS] Reviewing freeze map code

[Alvaro Herrera]

Andres Freund wrote:

> On 2016-05-06 14:17:13 -0700, Joshua D. Drake wrote:
> > How do I test?
> > 
> > Is there a script I can run?
> 
> Unfortunately there's few interesting things to test with pre-made
> scripts. There's no relevant OS dependency here, so each already
> existing test doesn't really lead to significantly increased coverage
> being run by other people.  Generally, when testing for correctness
> issues, it's often of limited benefit to run tests written by the author
> of reviewer - such scripts will usually just test things either has
> thought of.  The dangerous areas are the ones neither author or reviewer
> has considered.

We touched this question in connection with multixact freezing and
wraparound.  Testers seem to want to be given a script that they can
install and run, then go for a beer and get back to a bunch of errors to
report.  But it doesn't work that way; writing a useful test script
requires a lot of effort.  Jeff Janes has done astounding work in these
matters.  (I don't think we credit him enough for that.)

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

[Alvaro Herrera]

Joshua D. Drake wrote:
> On 05/06/2016 01:40 PM, Robert Haas wrote:
> >On Wed, May 4, 2016 at 8:08 PM, Andres Freund  wrote:
> >>On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
> >>>77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.
> >>
> >>Nothing to say here.
> >>
> >>>fd31cd2 Don't vacuum all-frozen pages.
> >>
> >>Hm. I do wonder if it's going to bite us that we don't have a way to
> >>actually force vacuuming of the whole table (besides manually rm'ing the
> >>VM). I've more than once seen VACUUM used to try to do some integrity
> >>checking of the database.  How are we actually going to test that the
> >>feature works correctly? They'd have to write checks ontop of
> >>pg_visibility to see whether things are borked.
> >
> >Let's add VACUUM (FORCE) or something like that.
> 
> This is actually inverted. Vacuum by default should vacuum the entire
> relation, however if we are going to keep the existing behavior of this
> patch, VACUUM (FROZEN) seems to be better than (FORCE)?

Prior to some 7.x release, VACUUM actually did what we ripped out in
9.0 release as VACUUM FULL.  We actually changed the mode of operation
quite heavily into the "lazy" mode which didn't acquire access exclusive
lock, and it was a huge relief.  I think that changing the mode of
operation to be the lightest possible thing that gets the job done
is convenient for users, because their existing scripts continue to
clean their tables only they take less time.  No need to tweak the
maintenance scripts.

I don't know what happens when the freeze_table_age threshold is
reached.  Do we scan the whole table when that happens?  Because if we
do, then we don't need a new keyword: just invoke the command after
lowering the setting.

Another question on this feature is what happens with the table age
(relfrozenxid, relminmxid) when the table is not wholly scanned by
vacuum.

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

[Andres Freund]

Hi,


On 2016-05-06 14:17:13 -0700, Joshua D. Drake wrote:
> How do I test?
> 
> Is there a script I can run?

Unfortunately there's few interesting things to test with pre-made
scripts. There's no relevant OS dependency here, so each already
existing test doesn't really lead to significantly increased coverage
being run by other people.  Generally, when testing for correctness
issues, it's often of limited benefit to run tests written by the author
of reviewer - such scripts will usually just test things either has
thought of.  The dangerous areas are the ones neither author or reviewer
has considered.


> Are there specific things I can do to try and break it?

Upgrade clusters using pg_upgrade and make sure things like index only
scans still work and yield correct data.  Set up workloads that involve
freezing, and check that less WAL (and not more!) is generated with 9.6
than with 9.5.  Make sure queries still work.


> What are we looking for exactly?

Data corruption, efficiency problems.


> A lot of -hackers seem to forget that although we have 100 -hackers, we have
> 1 "consultant/practitioners". Could I read the code and with a weekend
> of WTF and -hackers questions figure out what is going on, yes but a lot of
> people couldn't and I don't have the time.

I think tests without reading the code are quite sensible and
important. And it perfectly makes sense to ask for information about
what to test.  But fundamentally testing is a lot of work, as is writing
and reviewing code; unless you're really really good at destructive
testing, you won't find much in a 15 minute break.


> You want me (or people like me) to test more? Give us an easy way to
> do it.

Useful additional testing and easy just don't go well together. By the
time I have made it easy I've done the testing that's needed.


> Otherwise, we do what we can, which is try and interface on the things that
> will directly and immediately affect us (like keywords and syntax).

The amount of bikeshedding on -hackers steals energy and time for
actually working on stuff, including testing. So I have little sympathy
for the amount of bike shedding done.

Greetings,

Andres Freund


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

Re: [HACKERS] Feature request: make cluster_name GUC useful for psql prompts

[Steve Crawford]

Although this is getting slightly off the original topic, rereading .psqlrc
is a potential can of worms. What triggers a reread? What portions of
.psqlrc are re-read?

For example, say I have just set tuples-only, extended-display, or output
file. Would they all get reset just because I changed connections?

You can use variables to approximate the behavior of aliases so you can
hack an alias that includes the reconnect and re-read. Or just \i ~/.psqlrc
as you deem necessary.

Cheers,
Steve




On Fri, May 6, 2016 at 12:50 PM, Jerry Sievers 
wrote:

> Steve Crawford  writes:
>
> > That is almost identical to the solution I suggested a week or two ago
> to someone tackling the issue and the hack works on initial connection.
> >
> > Connect to a different cluster with "\c", however, and it will leave the
> prompt showing you connected to the original database which is not good.
>
> True and I've always thought of it as a possible misfeature of psql that
> it scans .psqlrc only once.
>
> > Cheers,
> > Steve
> >
> > On Fri, May 6, 2016 at 11:42 AM, Jerry Sievers 
> wrote:
> >
> > Peter Eisentraut  writes:
> >
> > > On 5/5/16 9:21 PM, Steve Crawford wrote:
> > >
> > >> Adding an escape sequence that references cluster_name would
> enable
> > >> prompts to identify the cluster in a manner that is both
> consistent and
> > >> distinct regardless of access path.
> > >
> > > I think that would be a good idea.  You could probably design it so
> > > that any server parameter reported to the client can be put in a
> psql
> > > prompt.
> >
> > The OP can easily work around that lack of support with something
> such as follow...
> >
> > Add this to ~/.psqlrc[-optional version stuff]
> >
> > select setting as cluster_name from pg_settings where name =
> 'cluster_name'  -- do not simicolon terminate this line
> > \gset
> >
> > \set PROMPT1 :cluster_name ': how cool is this:'
> >
> > >
> > >> Potential issues/improvements:
> > >>
> > >> What should the escape-sequence display if cluster_name is not
> set or
> > >> the cluster is a pre-9.5 version. %M? %m?
> > >>
> > >> In future server versions should there be a default for
> cluster_name if
> > >> it is not set? If so, what should it be? Would the server
> canonical
> > >> hostname + listen-port be reasonable?
> > >
> > > Those are good questions.  I don't really like the proposed
> answers,
> > > because that could cause confusion in practical use.
> > >
> > > --
> > > Peter Eisentraut  http://www.2ndQuadrant.com/
> > > PostgreSQL Development, 24x7 Support, Remote DBA, Training &
> Services
> >
> > --
> > Jerry Sievers
> > Postgres DBA/Development Consulting
> > e: postgres.consult...@comcast.net
> > p: 312.241.7800
> >
>
> --
> Jerry Sievers
> e: jerry.siev...@comcast.net
> p: 312.241.7800
>

Re: [HACKERS] Initial release notes created for 9.6

[Tom Lane]

Christian Ullrich  writes:
> * Tom Lane wrote:
>> To my mind, an option that's set to 1 is "enabled".  Should the second
>> para read "Do not disable ..."?  Or maybe we should reverse the sense
>> of the flag, so that the default state can be 0 == disabled?

> Well spotted, thanks. It should be "disable" instead.

> This is left from when the sense of the option _was_ the other way 
> around (it was called "real_realm" then). I reversed and renamed it 
> after Magnus reviewed the patch and was -- correctly -- opposed to the name.

Oh, OK, I don't wish to re-open the issue if it's already been thought
about.  Will just s/enable/disable/ 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] Reviewing freeze map code

[Andres Freund]

On 2016-05-06 14:15:47 -0700, Josh berkus wrote:
> For the serious testing, does anyone have a good technique for creating
> loads which would stress-test vacuum freezing?  It's hard for me to come
> up with anything which wouldn't be very time-and-resource intensive
> (like running at 10,000 TPS for a week).

I've changed the limits for freezing options a while back, so you can
now set autovacuum_freeze_max as low as 10 (best set
vacuum_freeze_table_age accordingly).  You'll have to come up with a
workload that doesn't overwrite all data continuously (otherwise
there'll never be old rows), but otherwise it should now be fairly easy
to test that kind of scenario.

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] ALTER TABLE lock downgrades have broken pg_upgrade

[Tom Lane]

I wrote:
> I have no more time to work on this, but I think it needs to be fixed, and
> I definitely think we had better put in test coverage when we do fix it.

Actually, there is a really easy fix we could put in, which is to decide
that optionally_create_toast_tables() is useless and get rid of it.

Point 1: if a table did not have a TOAST table in the old database,
any decision that it needs one in the new database must be a very
close-to-the-edge situation; certainly the 9.2 change we had in this area
was a matter of rounding things off differently.  needs_toast_table()'s
threshold for max tuple length is only a quarter page, so there's a huge
amount of daylight between where we'd choose to create a toast table and
where users would actually see failures from not having one.  It's pretty
hard to believe that cross-version differences in the do-we-need-a-toast-
table heuristic would exceed that.

Point 2: the current code is broken, and will cause a pg_upgrade failure
if the situation of concern occurs.  That's certainly much worse than
not adding a marginally-useful toast table.

Point 3: in view of point 2, the lack of field complaints says that this
situation doesn't actually happen in the field.

Barring complaints, I'll fix this bug by removing that code 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

Re: [HACKERS] Reviewing freeze map code

[Joshua D. Drake]

On 05/06/2016 02:08 PM, Andres Freund wrote:


VACUUM THEWHOLEDAMNTHING



I know that would never fly but damn if that wouldn't be an awesome keyword
for VACUUM.


It bothers me more than it probably should: Nobdy tests, reviews,
whatever a complex patch with significant data-loss potential. But as
soon somebody dares to mention an option name...


That is a fair complaint but let me ask you something:

How do I test?

Is there a script I can run? Are there specific things I can do to try 
and break it? What are we looking for exactly?


A lot of -hackers seem to forget that although we have 100 -hackers, we 
have 1 "consultant/practitioners". Could I read the code and with a 
weekend of WTF and -hackers questions figure out what is going on, yes 
but a lot of people couldn't and I don't have the time.


You want me (or people like me) to test more? Give us an easy way to do 
it. Otherwise, we do what we can, which is try and interface on the 
things that will directly and immediately affect us (like keywords and 
syntax).


Sincerely,


JD


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


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

Re: [HACKERS] Reviewing freeze map code

[Josh berkus]

On 05/06/2016 02:12 PM, Andres Freund wrote:
> On 2016-05-06 14:10:04 -0700, Josh berkus wrote:
>> On 05/06/2016 02:08 PM, Andres Freund wrote:
>>
>>> It bothers me more than it probably should: Nobdy tests, reviews,
>>> whatever a complex patch with significant data-loss potential. But as
>>> soon somebody dares to mention an option name...
>>
>> Definitely more than it should, because it's gonna happen *every* time.
>>
>> https://en.wikipedia.org/wiki/Law_of_triviality
> 
> Doesn't mean it should not be frowned upon.

Or made light of, hence my post.  Personally I don't care what the
option is called, as long as we have docs for it.

For the serious testing, does anyone have a good technique for creating
loads which would stress-test vacuum freezing?  It's hard for me to come
up with anything which wouldn't be very time-and-resource intensive
(like running at 10,000 TPS for a week).

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)


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

[Andres Freund]

On 2016-05-06 14:10:04 -0700, Josh berkus wrote:
> On 05/06/2016 02:08 PM, Andres Freund wrote:
> 
> > It bothers me more than it probably should: Nobdy tests, reviews,
> > whatever a complex patch with significant data-loss potential. But as
> > soon somebody dares to mention an option name...
> 
> Definitely more than it should, because it's gonna happen *every* time.
> 
> https://en.wikipedia.org/wiki/Law_of_triviality

Doesn't mean it should not be frowned upon.


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

[Josh berkus]

On 05/06/2016 02:08 PM, Andres Freund wrote:

> It bothers me more than it probably should: Nobdy tests, reviews,
> whatever a complex patch with significant data-loss potential. But as
> soon somebody dares to mention an option name...

Definitely more than it should, because it's gonna happen *every* time.

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

-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)


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

[Joshua D. Drake]

On 05/06/2016 02:03 PM, Stephen Frost wrote:



VACUUM THEWHOLEDAMNTHING


+100

(hahahaha)


You know what? Why not? Seriously? We aren't product. This is supposed 
to be a bit fun. Let's have some fun with it? It would be so easy to 
turn that into a positive advocacy opportunity.




JD




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


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

Re: [HACKERS] Reviewing freeze map code

[Andres Freund]

On 2016-05-06 14:03:11 -0700, Joshua D. Drake wrote:
> On 05/06/2016 02:01 PM, Josh berkus wrote:
> > On 05/06/2016 01:58 PM, Andres Freund wrote:
> > > On 2016-05-06 13:54:09 -0700, Joshua D. Drake wrote:
> > > > On 05/06/2016 01:50 PM, Andres Freund wrote:
> > 
> > > > > There already is FREEZE - meaning something different - so I doubt it.
> > > > 
> > > > Yeah I thought about that, it is the word "FORCE" that bothers me. When 
> > > > you
> > > > use FORCE there is an assumption that no matter what, it plows through
> > > > (think rm -f). So if we don't use FROZEN, that's cool but FORCE doesn't 
> > > > work
> > > > either.
> > > 
> > > SCANALL?
> > > 
> > 
> > VACUUM THEWHOLEDAMNTHING
> > 
> 
> I know that would never fly but damn if that wouldn't be an awesome keyword
> for VACUUM.

It bothers me more than it probably should: Nobdy tests, reviews,
whatever a complex patch with significant data-loss potential. But as
soon somebody dares to mention an option name...


-- 
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] atomic pin/unpin causing errors

[Jeff Janes]

On Fri, May 6, 2016 at 11:24 AM, Andres Freund  wrote:
>
>> I have been trying (and failing) to reproduce the problem in more
>> recent releases, with and without cassert.  Here is pg_config output
>> of one of my current attempts:
>
> If you say "recent releases" you mean that you've not been able to
> reproduce it in 9.5, 9.4, ..., not that the issue has vanished in
> master, right?

Sorry, I meant recent commits, not releases.  I have not been able to
reproduce it in master (anything after 8f1911d5e6), but I also can't
find the commit which fixed it, because I don't know how many hours of
running without an error is enough to declare it good.  I've already
gotten burned by that by declaring 8f1911d5e6 good once, only to find
out it was still bad just with a lower probability of finding it.

I also can't reproduce it in 9.5, but I wouldn't expect it to given
the large changes in the way GIN indexes operates between 9.5 and 9.6,
which completely changes the timing and types of races (even ones
outside of the GIN code) one might expect to see.  I'm pretty sure I
would have caught it during 9.5's beta if it were findable with this
test, as this test was run back then in pretty close to the current
form.  I'm not saying their isn't a bug in 9.5, just that if there is
this test is not efficient at invoking it.

So the issue *has* vanished in master, but without knowing what fixed
it I have no confidence it was actually fixed, rather than the test
just stopped being effective.

What I plan on doing now is going back to the part of the commit
history where I could reproduce it reliably, and start throwing
unnecessary things of the ./configure and the postgresql.conf to see
what the minimal configuration is at which I can reproduce it.

Sorry for the confusion.

Cheers,

Jeff


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

[Stephen Frost]

* Josh berkus (j...@agliodbs.com) wrote:
> On 05/06/2016 01:58 PM, Andres Freund wrote:
> > On 2016-05-06 13:54:09 -0700, Joshua D. Drake wrote:
> >> On 05/06/2016 01:50 PM, Andres Freund wrote:
> 
> >>> There already is FREEZE - meaning something different - so I doubt it.
> >>
> >> Yeah I thought about that, it is the word "FORCE" that bothers me. When you
> >> use FORCE there is an assumption that no matter what, it plows through
> >> (think rm -f). So if we don't use FROZEN, that's cool but FORCE doesn't 
> >> work
> >> either.
> > 
> > SCANALL?
> > 
> 
> VACUUM THEWHOLEDAMNTHING

+100

(hahahaha)

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Reviewing freeze map code

[Joshua D. Drake]

On 05/06/2016 02:01 PM, Josh berkus wrote:

On 05/06/2016 01:58 PM, Andres Freund wrote:

On 2016-05-06 13:54:09 -0700, Joshua D. Drake wrote:

On 05/06/2016 01:50 PM, Andres Freund wrote:



There already is FREEZE - meaning something different - so I doubt it.


Yeah I thought about that, it is the word "FORCE" that bothers me. When you
use FORCE there is an assumption that no matter what, it plows through
(think rm -f). So if we don't use FROZEN, that's cool but FORCE doesn't work
either.


SCANALL?



VACUUM THEWHOLEDAMNTHING



I know that would never fly but damn if that wouldn't be an awesome 
keyword for VACUUM.


JD


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


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

Re: [HACKERS] Reviewing freeze map code

[Josh berkus]

On 05/06/2016 01:58 PM, Andres Freund wrote:
> On 2016-05-06 13:54:09 -0700, Joshua D. Drake wrote:
>> On 05/06/2016 01:50 PM, Andres Freund wrote:

>>> There already is FREEZE - meaning something different - so I doubt it.
>>
>> Yeah I thought about that, it is the word "FORCE" that bothers me. When you
>> use FORCE there is an assumption that no matter what, it plows through
>> (think rm -f). So if we don't use FROZEN, that's cool but FORCE doesn't work
>> either.
> 
> SCANALL?
> 

VACUUM THEWHOLEDAMNTHING


-- 
--
Josh Berkus
Red Hat OSAS
(any opinions are my own)


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

[Joshua D. Drake]

On 05/06/2016 01:58 PM, Stephen Frost wrote:

* Joshua D. Drake (j...@commandprompt.com) wrote:

Yeah I thought about that, it is the word "FORCE" that bothers me.
When you use FORCE there is an assumption that no matter what, it
plows through (think rm -f). So if we don't use FROZEN, that's cool
but FORCE doesn't work either.


Isn't that exactly what this FORCE option being contemplated would do
though?  Plow through the entire relation, regardless of what the VM
says is all frozen or not?

Seems like FORCE is a good word for that to me.


Except that we aren't FORCING a vacuum. That is the part I have 
contention with. To me, FORCE means:


No matter what else is happening, we are vacuuming this relation (think 
locks).


But I am also not going to dig in my heals. If that is truly what 
-hackers come up with, thank you at least considering what I said.


Sincerely,

JD



Thanks!

Stephen




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


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

Re: [HACKERS] Reviewing freeze map code

[Stephen Frost]

* Joshua D. Drake (j...@commandprompt.com) wrote:
> Yeah I thought about that, it is the word "FORCE" that bothers me.
> When you use FORCE there is an assumption that no matter what, it
> plows through (think rm -f). So if we don't use FROZEN, that's cool
> but FORCE doesn't work either.

Isn't that exactly what this FORCE option being contemplated would do
though?  Plow through the entire relation, regardless of what the VM
says is all frozen or not?

Seems like FORCE is a good word for that to me.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Reviewing freeze map code

[Andres Freund]

On 2016-05-06 13:54:09 -0700, Joshua D. Drake wrote:
> On 05/06/2016 01:50 PM, Andres Freund wrote:
> 
> > > > Let's add VACUUM (FORCE) or something like that.
> > 
> > Yes, that makes sense.
> > 
> > 
> > > This is actually inverted. Vacuum by default should vacuum the entire
> > > relation
> > 
> > What? Why on earth would that be a good idea? Not to speak of hte fact
> > that that's not been the case since ~8.4?
> 
> Sorry, I just meant the default behavior shouldn't change but I do agree
> that we need the ability to keep the same behavior.

Which default behaviour shouldn't change? The one in master where we
skip known frozen pages? Or the released branches where can't skip those?

> > > ,however if we are going to keep the existing behavior of this
> > > patch, VACUUM (FROZEN) seems to be better than (FORCE)?
> > 
> > There already is FREEZE - meaning something different - so I doubt it.
> 
> Yeah I thought about that, it is the word "FORCE" that bothers me. When you
> use FORCE there is an assumption that no matter what, it plows through
> (think rm -f). So if we don't use FROZEN, that's cool but FORCE doesn't work
> either.

SCANALL?


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

[Joshua D. Drake]

On 05/06/2016 01:50 PM, Andres Freund wrote:


Let's add VACUUM (FORCE) or something like that.


Yes, that makes sense.



This is actually inverted. Vacuum by default should vacuum the entire
relation


What? Why on earth would that be a good idea? Not to speak of hte fact
that that's not been the case since ~8.4?


Sorry, I just meant the default behavior shouldn't change but I do agree 
that we need the ability to keep the same behavior.



,however if we are going to keep the existing behavior of this
patch, VACUUM (FROZEN) seems to be better than (FORCE)?


There already is FREEZE - meaning something different - so I doubt it.


Yeah I thought about that, it is the word "FORCE" that bothers me. When 
you use FORCE there is an assumption that no matter what, it plows 
through (think rm -f). So if we don't use FROZEN, that's cool but FORCE 
doesn't work either.


Sincerely,

JD


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


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

Re: [HACKERS] Initial release notes created for 9.6

[Christian Ullrich]

* Tom Lane wrote:


Christian Ullrich  writes:



I suggest writing "use the Kerberos realm name for authentication
instead of the NetBIOS name" either in place of the existing description
or together with it.


OK, how about this:

   
Add new SSPI authentication parameters compat_realm
and upn_username, to control whether NetBIOS or Kerberos
realm names and user names are used during SSPI authentication
(Christian Ullrich)
   


Perfect, except for the implied idea of a "NetBIOS realm name", see 
below. I can live with that in release notes, though.



BTW, I went to read the descriptions of those parameters again, and this
one seems a bit confusing:

 
  compat_realm
  
   
If set to 1, the domain's SAM-compatible name (also known as the
NetBIOS name) is used for the include_realm
option. This is the default. If set to 0, the true realm name from
the Kerberos user principal name is used.
   
   
Do not enable this option unless your server runs under a domain
account (this includes virtual service accounts on a domain member
system) and all clients authenticating through SSPI are also using
domain accounts, or authentication will fail.
   
  
 

To my mind, an option that's set to 1 is "enabled".  Should the second
para read "Do not disable ..."?  Or maybe we should reverse the sense
of the flag, so that the default state can be 0 == disabled?


Well spotted, thanks. It should be "disable" instead.

This is left from when the sense of the option _was_ the other way 
around (it was called "real_realm" then). I reversed and renamed it 
after Magnus reviewed the patch and was -- correctly -- opposed to the name.


If the default state should be off, we're back to inventing a useful new 
name. Magnus suggested "sspi_netbios_realm", which could be shortened to 
just "netbios_realm", but I don't like to have both "NetBIOS" and 
"realm" in the name because nobody else calls a domain's NetBIOS name a 
"realm". (For the release notes, on the other hand, there is no need to 
split this hair quite so thin.)


Unless you _really_ want the default (that is, backwards compatible) 
behavior with the option off, I would rather keep it the way it is.


--
Christian



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

Re: [HACKERS] Reviewing freeze map code

[Andres Freund]

On 2016-05-06 13:48:09 -0700, Joshua D. Drake wrote:
> On 05/06/2016 01:40 PM, Robert Haas wrote:
> > On Wed, May 4, 2016 at 8:08 PM, Andres Freund  wrote:
> > > On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
> > > > 77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.
> > >
> > > Nothing to say here.
> > >
> > > > fd31cd2 Don't vacuum all-frozen pages.
> > >
> > > Hm. I do wonder if it's going to bite us that we don't have a way to
> > > actually force vacuuming of the whole table (besides manually rm'ing the
> > > VM). I've more than once seen VACUUM used to try to do some integrity
> > > checking of the database.  How are we actually going to test that the
> > > feature works correctly? They'd have to write checks ontop of
> > > pg_visibility to see whether things are borked.
> >
> > Let's add VACUUM (FORCE) or something like that.

Yes, that makes sense.


> This is actually inverted. Vacuum by default should vacuum the entire
> relation

What? Why on earth would that be a good idea? Not to speak of hte fact
that that's not been the case since ~8.4?


>,however if we are going to keep the existing behavior of this
> patch, VACUUM (FROZEN) seems to be better than (FORCE)?

There already is FREEZE - meaning something different - so I doubt it.

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

[Joshua D. Drake]

On 05/06/2016 01:40 PM, Robert Haas wrote:

On Wed, May 4, 2016 at 8:08 PM, Andres Freund  wrote:

On 2016-05-02 14:48:18 -0700, Andres Freund wrote:

77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.


Nothing to say here.


fd31cd2 Don't vacuum all-frozen pages.


Hm. I do wonder if it's going to bite us that we don't have a way to
actually force vacuuming of the whole table (besides manually rm'ing the
VM). I've more than once seen VACUUM used to try to do some integrity
checking of the database.  How are we actually going to test that the
feature works correctly? They'd have to write checks ontop of
pg_visibility to see whether things are borked.


Let's add VACUUM (FORCE) or something like that.


This is actually inverted. Vacuum by default should vacuum the entire 
relation, however if we are going to keep the existing behavior of this 
patch, VACUUM (FROZEN) seems to be better than (FORCE)?


Sincerely,

JD

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


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

Re: [HACKERS] Reviewing freeze map code

[Robert Haas]

On Thu, May 5, 2016 at 2:20 PM, Andres Freund  wrote:
> On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
>> 7087166 pg_upgrade: Convert old visibility map format to new format.
>
> +const char *
> +rewriteVisibilityMap(const char *fromfile, const char *tofile, bool force)
> ...
>
> +   while ((bytesRead = read(src_fd, buffer, BLCKSZ)) == BLCKSZ)
> +   {
> ..
>
> Uh, shouldn't we actually fail if we read incompletely? Rather than
> silently ignoring the problem? Ok, this causes no corruption, but it
> indicates that something went significantly wrong.

Sure, that's reasonable.

> +   charnew_vmbuf[BLCKSZ];
> +   char   *new_cur = new_vmbuf;
> +   boolempty = true;
> +   boolold_lastpart;
> +
> +   /* Copy page header in advance */
> +   memcpy(new_vmbuf, , SizeOfPageHeaderData);
>
> Shouldn't we zero out new_vmbuf? Afaics we're not necessarily zeroing it
> with old_lastpart && !empty, right?

Oh, dear.  That seems like a possible data corruption bug.  Maybe we'd
better fix that right away (although I don't actually have time before
the wrap).

> +   if ((dst_fd = open(tofile, O_RDWR | O_CREAT | (force ? 0 : O_EXCL), 
> S_IRUSR | S_IWUSR)) < 0)
> +   {
> +   close(src_fd);
> +   return getErrorText();
> +   }
>
> I know you guys copied this, but what's the force thing about?
> Expecially as it's always set to true by the callers (i.e. what is the
> parameter even about?)?  Wouldn't we at least have to specify O_TRUNC in
> the force case?

I just work here.

> +   old_cur += BITS_PER_HEAPBLOCK_OLD;
> +   new_cur += BITS_PER_HEAPBLOCK;
>
> I'm not sure I'm understanding the point of the BITS_PER_HEAPBLOCK_OLD
> stuff - as long as it's hardcoded into rewriteVisibilityMap() we'll not
> be able to have differing ones anyway, should we decide to add a third
> bit?

I think that's just a matter of style.

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

[Robert Haas]

On Wed, May 4, 2016 at 8:08 PM, Andres Freund  wrote:
> On 2016-05-02 14:48:18 -0700, Andres Freund wrote:
>> 77a1d1e Department of second thoughts: remove PD_ALL_FROZEN.
>
> Nothing to say here.
>
>> fd31cd2 Don't vacuum all-frozen pages.
>
> Hm. I do wonder if it's going to bite us that we don't have a way to
> actually force vacuuming of the whole table (besides manually rm'ing the
> VM). I've more than once seen VACUUM used to try to do some integrity
> checking of the database.  How are we actually going to test that the
> feature works correctly? They'd have to write checks ontop of
> pg_visibility to see whether things are borked.

Let's add VACUUM (FORCE) or something like that.

> /*
>  * Compute whether we actually scanned the whole relation. If we did, 
> we
>  * can adjust relfrozenxid and relminmxid.
>  *
>  * NB: We need to check this before truncating the relation, because 
> that
>  * will change ->rel_pages.
>  */
>
> Comment is out-of-date now.

OK.

> -   if (blkno == next_not_all_visible_block)
> +   if (blkno == next_unskippable_block)
> {
> -   /* Time to advance next_not_all_visible_block */
> -   for (next_not_all_visible_block++;
> -next_not_all_visible_block < nblocks;
> -next_not_all_visible_block++)
> +   /* Time to advance next_unskippable_block */
> +   for (next_unskippable_block++;
> +next_unskippable_block < nblocks;
> +next_unskippable_block++)
>
> Hm. So we continue with the course of re-processing pages, even if
> they're marked all-frozen. For all-visible there at least can be a
> benefit by freezing earlier, but for all-frozen pages there's really no
> point.  I don't really buy the arguments for the skipping logic. But
> even disregarding that, maybe we should skip processing a block if it's
> all-frozen (without preventing the page from being read?); as there's no
> possible benefit?  Acquring the exclusive/content lock and stuff is far
> from free.

I wanted to tinker with this logic as little as possible in the
interest of ending up with something that worked.  I would not have
written it this way.

> Not really related to this patch, but the FORCE_CHECK_PAGE is rather
> ugly.

+1.

> +   /*
> +* The current block is potentially skippable; if 
> we've seen a
> +* long enough run of skippable blocks to justify 
> skipping it, and
> +* we're not forced to check it, then go ahead and 
> skip.
> +* Otherwise, the page must be at least all-visible 
> if not
> +* all-frozen, so we can set 
> all_visible_according_to_vm = true.
> +*/
> +   if (skipping_blocks && !FORCE_CHECK_PAGE())
> +   {
> +   /*
> +* Tricky, tricky.  If this is in aggressive 
> vacuum, the page
> +* must have been all-frozen at the time we 
> checked whether it
> +* was skippable, but it might not be any 
> more.  We must be
> +* careful to count it as a skipped 
> all-frozen page in that
> +* case, or else we'll think we can't update 
> relfrozenxid and
> +* relminmxid.  If it's not an aggressive 
> vacuum, we don't
> +* know whether it was all-frozen, so we have 
> to recheck; but
> +* in this case an approximate answer is OK.
> +*/
> +   if (aggressive || VM_ALL_FROZEN(onerel, 
> blkno, ))
> +   vacrelstats->frozenskipped_pages++;
> continue;
> +   }
>
> Hm. This indeed seems a bit tricky.  Not sure how to make it easier
> though without just ripping out the SKIP_PAGES_THRESHOLD stuff.

Yep, I had the same problem.

> Hm. This also doubles the number of VM accesses. While I guess that's
> not noticeable most of the time, it's still not nice; especially when a
> large relation is entirely frozen, because it'll mean we'll sequentially
> go through the visibilityma twice.

Compared to what we're saving, that's obviously a trivial cost.
That's not to say that we might not want to improve it, but it's
hardly a disaster.

In short: wah, wah, wah.

> I wondered for a minute whether #14057 could cause really bad issues
> here
> http://www.postgresql.org/message-id/20160331103739.8956.94...@wrigleys.postgresql.org
> but I don't see it being more relevant here.

I 

Re: [HACKERS] Reviewing freeze map code

[Robert Haas]

On Mon, May 2, 2016 at 8:25 PM, Andres Freund  wrote:
> + * heap_tuple_needs_eventual_freeze
> + *
> + * Check to see whether any of the XID fields of a tuple (xmin, xmax, xvac)
> + * will eventually require freezing.  Similar to heap_tuple_needs_freeze,
> + * but there's no cutoff, since we're trying to figure out whether freezing
> + * will ever be needed, not whether it's needed now.
> + */
> +bool
> +heap_tuple_needs_eventual_freeze(HeapTupleHeader tuple)
>
> Wouldn't redefining this to heap_tuple_is_frozen() and then inverting the
> checks be easier to understand?

I thought it much safer to keep this as close to a copy of
heap_tuple_needs_freeze() as possible.  Copying a function and
inverting all of the return values is much more likely to introduce
bugs, IME.

> +   /*
> +* If xmax is a valid xact or multixact, this tuple is also not 
> frozen.
> +*/
> +   if (tuple->t_infomask & HEAP_XMAX_IS_MULTI)
> +   {
> +   MultiXactId multi;
> +
> +   multi = HeapTupleHeaderGetRawXmax(tuple);
> +   if (MultiXactIdIsValid(multi))
> +   return true;
> +   }
>
> Hm. What's the test inside the if() for? There shouldn't be any case
> where xmax is invalid if HEAP_XMAX_IS_MULTI is set.   Now there's a
> check like that outside of this commit, but it seems strange to me
> (Alvaro, perhaps you could comment on this?).

Here again I was copying existing code, with appropriate simplifications.

> + *
> + * Clearing both visibility map bits is not separately WAL-logged.  The 
> callers
>   * must make sure that whenever a bit is cleared, the bit is cleared on WAL
>   * replay of the updating operation as well.
>
> I think including "both" here makes things less clear, because it
> differentiates clearing one bit from clearing both. There's no practical
> differentce atm, but still.

I agree.

>   *
>   * VACUUM will normally skip pages for which the visibility map bit is set;
>   * such pages can't contain any dead tuples and therefore don't need 
> vacuuming.
> - * The visibility map is not used for anti-wraparound vacuums, because
> - * an anti-wraparound vacuum needs to freeze tuples and observe the latest 
> xid
> - * present in the table, even on pages that don't have any dead tuples.
>   *
>
> I think the remaining sentence isn't entirely accurate, there's now more
> than one bit, and they're different with regard to scan_all/!scan_all
> vacuums (or will be - maybe this updated further in a later commit? But
> if so, that sentence shouldn't yet be removed...).

We can adjust the language, but I don't really see a big problem here.

> -/* Number of heap blocks we can represent in one byte. */
> -#define HEAPBLOCKS_PER_BYTE 8
> -
> Hm, why was this moved to the header? Sounds like something the outside
> shouldn't care about.

Oh... yeah.  Let's undo that.

> #define HEAPBLK_TO_MAPBIT(x) (((x) % HEAPBLOCKS_PER_BYTE) * 
> BITS_PER_HEAPBLOCK)
>
> Hm. This isn't really a mapping to an individual bit anymore - but I
> don't really have a better name in mind. Maybe TO_OFFSET?

Well, it sorta is... but we could change it, I suppose.

> +static const uint8 number_of_ones_for_visible[256] = {
> ...
> +};
> +static const uint8 number_of_ones_for_frozen[256] = {
> ...
>  };
>
> Did somebody verify the new contents are correct?

I admit that I didn't.  It seemed like an unlikely place for a goof,
but I guess we should verify.

> /*
> - * visibilitymap_clear - clear a bit in visibility map
> + * visibilitymap_clear - clear all bits in visibility map
>   *
>
> This seems rather easy to misunderstand, as this really only clears all
> the bits for one page, not actually all the bits.

We could change "in" to "for one page in the".

>   * the bit for heapBlk, or InvalidBuffer. The caller is responsible for
> - * releasing *buf after it's done testing and setting bits.
> + * releasing *buf after it's done testing and setting bits, and must pass 
> flags
> + * for which it needs to check the value in visibility map.
>   *
>   * NOTE: This function is typically called without a lock on the heap page,
>   * so somebody else could change the bit just after we look at it.  In fact,
> @@ -327,17 +351,16 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, 
> Buffer heapBuf,
>
> I'm not seing what flags the above comment change is referring to?

Ugh.  I think that's leftover cruft from an earlier patch version that
should have been excised from what got committed.

> /*
> -* A single-bit read is atomic.  There could be memory-ordering 
> effects
> +* A single byte read is atomic.  There could be memory-ordering 
> effects
>  * here, but for performance reasons we make it the caller's job to 
> worry
>  * about that.
>  */
> -   result = (map[mapByte] & (1 << mapBit)) ? true : false;
> -
> -   return result;
> +   return ((map[mapByte] >> mapBit) & 

Re: [HACKERS] Initial release notes created for 9.6

[Alvaro Herrera]

Tom Lane wrote:
> Alvaro Herrera  writes:
> > Is it really sensible to group f1f5ec1ef together with the others?  I
> > think that one should be its own entry.
> 
> Peter seemed happy with the idea, cf
> http://www.postgresql.org/message-id/cam3swzsoouy3equzic32he-fxyccxfv5rkots-8of20rzgc...@mail.gmail.com

Oh, okay.

-- 
Á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] Initial release notes created for 9.6

[Tom Lane]

Alvaro Herrera  writes:
> Is it really sensible to group f1f5ec1ef together with the others?  I
> think that one should be its own entry.

Peter seemed happy with the idea, cf
http://www.postgresql.org/message-id/cam3swzsoouy3equzic32he-fxyccxfv5rkots-8of20rzgc...@mail.gmail.com

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] Initial release notes created for 9.6

[Alvaro Herrera]

  

   
Speed up sorting of uuid, bytea,
and char(n) fields by using abbreviated keys
(Peter Geoghegan)
   

   
Support for abbreviated keys has also been added to the non-default
operator classes text_pattern_ops,
varchar_pattern_ops, and bpchar_pattern_ops.
Processing of ordered-set aggregates can also now exploit
abbreviated keys.
   
  

Is it really sensible to group f1f5ec1ef together with the others?  I
think that one should be its own entry.

-- 
Á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] [COMMITTERS] pgsql: Add TAP tests for pg_dump

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Fri, May 6, 2016 at 4:07 PM, Stephen Frost  wrote:
> >> * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >>> But at this point I think Peter's complaint has some force to it, and that
> >>> what you ought to do is revert the testing patch.  You can have another go
> >>> after beta1.
> 
> >> Are you suggesting commiting to still-9.6-HEAD post-beta1?  I took
> >> Peter's comment as suggesting that adding the tests would have to wait
> >> til after we branched 9.6, as we do for features.
> >> 
> >> I'd really like to have these tests included as that will make them
> >> available to others more easily to add on to, and I'm certainly planning
> >> to continue adding tests until I get pg_dump.c's coverage a lot better.
> >> That seems like the perfect kind of effort that should be happening
> >> right now- adding more tests and working to make sure that what's been
> >> committed is correct (and fixing it when it isn't, as discovered by the
> >> test suite with transforms and casts...).
> 
> > I think what he's suggesting right now is that you revert the patch
> > that is turning the BF red right before beta.  We can iron out what
> > else to do later.
> 
> Yes.  I have no objection to adding more test cases post-beta1, but
> I'd like to have the buildfarm green through the weekend.  This isn't
> the best time to be debugging-by-buildfarm.

Understood.

> If you like, you can try the @contrib_excludes addition that was mentioned
> before and see if that fixes it.  But if it doesn't, it's time to cut our
> losses.

Ok, I like that quite a bit better and will give it a shot, but if it
doesn't work, I'll revert it immediately.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Add TAP tests for pg_dump

[Tom Lane]

Robert Haas  writes:
> On Fri, May 6, 2016 at 4:07 PM, Stephen Frost  wrote:
>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>>> But at this point I think Peter's complaint has some force to it, and that
>>> what you ought to do is revert the testing patch.  You can have another go
>>> after beta1.

>> Are you suggesting commiting to still-9.6-HEAD post-beta1?  I took
>> Peter's comment as suggesting that adding the tests would have to wait
>> til after we branched 9.6, as we do for features.
>> 
>> I'd really like to have these tests included as that will make them
>> available to others more easily to add on to, and I'm certainly planning
>> to continue adding tests until I get pg_dump.c's coverage a lot better.
>> That seems like the perfect kind of effort that should be happening
>> right now- adding more tests and working to make sure that what's been
>> committed is correct (and fixing it when it isn't, as discovered by the
>> test suite with transforms and casts...).

> I think what he's suggesting right now is that you revert the patch
> that is turning the BF red right before beta.  We can iron out what
> else to do later.

Yes.  I have no objection to adding more test cases post-beta1, but
I'd like to have the buildfarm green through the weekend.  This isn't
the best time to be debugging-by-buildfarm.

If you like, you can try the @contrib_excludes addition that was mentioned
before and see if that fixes it.  But if it doesn't, it's time to cut our
losses.

regards, tom lane


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

Re: [HACKERS] [COMMITTERS] pgsql: Add TAP tests for pg_dump

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, May 6, 2016 at 4:07 PM, Stephen Frost  wrote:
> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
> >> Stephen Frost  writes:
> >> > * Stephen Frost (sfr...@snowman.net) wrote:
> >> >> Looks like the test_pg_dump extension made the Windows builds upset.
> >> >> I'm guessing that's because I set 'MODULES_big' even though there isn't
> >> >> a .c component.
> >> >>
> >> >> Doing a local build with that commented out, assuming that works and
> >> >> doesn't generate the .so any more on my Linux box, I'll push the change
> >> >> up to hopefully fix those buildfarm members.
> >>
> >> > Alright, apparently that made other Windows buildfarm members unhappy...
> >>
> >> > I guess the next approach will be to add back MODULES_big and add in a
> >> > .c file for the Windows systems to be happy about.  I'm certainly open
> >> > to other suggestions.
> >>
> >> You should not need to do that; cf src/test/modules/test_extensions,
> >> which has got SQL-only extensions.
> >
> > test_extensions is also included in the "contrib_excludes" list in
> > src/tools/msvc/Mkvcbuild.pm that I mentioned in my last email, so I'm
> > thinking that's what is needed.
> >
> >> But at this point I think Peter's complaint has some force to it, and that
> >> what you ought to do is revert the testing patch.  You can have another go
> >> after beta1.
> >
> > Are you suggesting commiting to still-9.6-HEAD post-beta1?  I took
> > Peter's comment as suggesting that adding the tests would have to wait
> > til after we branched 9.6, as we do for features.
> >
> > I'd really like to have these tests included as that will make them
> > available to others more easily to add on to, and I'm certainly planning
> > to continue adding tests until I get pg_dump.c's coverage a lot better.
> > That seems like the perfect kind of effort that should be happening
> > right now- adding more tests and working to make sure that what's been
> > committed is correct (and fixing it when it isn't, as discovered by the
> > test suite with transforms and casts...).
> 
> I think what he's suggesting right now is that you revert the patch
> that is turning the BF red right before beta.  We can iron out what
> else to do later.

Alright, I'll revert the TAP tests and we can discuss what to do next
post-beta1.  At least a few runs got in and looked clean, other than the
Windows issue with the extension building.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [COMMITTERS] pgsql: Add TAP tests for pg_dump

[Robert Haas]

On Fri, May 6, 2016 at 4:07 PM, Stephen Frost  wrote:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> Stephen Frost  writes:
>> > * Stephen Frost (sfr...@snowman.net) wrote:
>> >> Looks like the test_pg_dump extension made the Windows builds upset.
>> >> I'm guessing that's because I set 'MODULES_big' even though there isn't
>> >> a .c component.
>> >>
>> >> Doing a local build with that commented out, assuming that works and
>> >> doesn't generate the .so any more on my Linux box, I'll push the change
>> >> up to hopefully fix those buildfarm members.
>>
>> > Alright, apparently that made other Windows buildfarm members unhappy...
>>
>> > I guess the next approach will be to add back MODULES_big and add in a
>> > .c file for the Windows systems to be happy about.  I'm certainly open
>> > to other suggestions.
>>
>> You should not need to do that; cf src/test/modules/test_extensions,
>> which has got SQL-only extensions.
>
> test_extensions is also included in the "contrib_excludes" list in
> src/tools/msvc/Mkvcbuild.pm that I mentioned in my last email, so I'm
> thinking that's what is needed.
>
>> But at this point I think Peter's complaint has some force to it, and that
>> what you ought to do is revert the testing patch.  You can have another go
>> after beta1.
>
> Are you suggesting commiting to still-9.6-HEAD post-beta1?  I took
> Peter's comment as suggesting that adding the tests would have to wait
> til after we branched 9.6, as we do for features.
>
> I'd really like to have these tests included as that will make them
> available to others more easily to add on to, and I'm certainly planning
> to continue adding tests until I get pg_dump.c's coverage a lot better.
> That seems like the perfect kind of effort that should be happening
> right now- adding more tests and working to make sure that what's been
> committed is correct (and fixing it when it isn't, as discovered by the
> test suite with transforms and casts...).

I think what he's suggesting right now is that you revert the patch
that is turning the BF red right before beta.  We can iron out what
else to do later.

-- 
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] Initial release notes created for 9.6

[Tom Lane]

Christian Ullrich  writes:
> * Tom Lane wrote:
> +   
> +Add new SSPI authentication parameters compat_realm
> +and upn_usename, to make it possible to make SSPI
> +work more like GSSAPI (Christian Ullrich)
> +   

> It is upn_username, not usename. Typo in the commit message.

> "Make SSPI work more like GSSAPI" reads like it changed authentication 
> behavior in some fundamental way, and as if SSPI did not work like 
> GSSAPI without it. The difference in behavior of include_realm between 
> GSSAPI and SSPI is not caused by SSPI, but is an implementation detail 
> on our end.

> I suggest writing "use the Kerberos realm name for authentication 
> instead of the NetBIOS name" either in place of the existing description 
> or together with it.

OK, how about this:

   
Add new SSPI authentication parameters compat_realm
and upn_username, to control whether NetBIOS or Kerberos
realm names and user names are used during SSPI authentication
(Christian Ullrich)
   

BTW, I went to read the descriptions of those parameters again, and this
one seems a bit confusing:

 
  compat_realm
  
   
If set to 1, the domain's SAM-compatible name (also known as the
NetBIOS name) is used for the include_realm
option. This is the default. If set to 0, the true realm name from
the Kerberos user principal name is used.
   
   
Do not enable this option unless your server runs under a domain
account (this includes virtual service accounts on a domain member
system) and all clients authenticating through SSPI are also using
domain accounts, or authentication will fail.
   
  
 

To my mind, an option that's set to 1 is "enabled".  Should the second
para read "Do not disable ..."?  Or maybe we should reverse the sense
of the flag, so that the default state can be 0 == disabled?

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] Feature request: make cluster_name GUC useful for psql prompts

[Alvaro Herrera]

Jerry Sievers wrote:
> Steve Crawford  writes:
> 
> > That is almost identical to the solution I suggested a week or two ago to 
> > someone tackling the issue and the hack works on initial connection.
> >
> > Connect to a different cluster with "\c", however, and it will leave the 
> > prompt showing you connected to the original database which is not good.
> 
> True and I've always thought of it as a possible misfeature of psql that
> it scans .psqlrc only once.

Interesting point.  I think what you're saying boils down to there being
two init files, one that is read once at program start (sets up the
general environment) and another one that's executed each time a
connection is established.

I wonder where does this leave Greg Stark's concurrent psql sessions
feature.

Now, for the current case I think we should definitely have a specifier
for the prompt.

-- 
Á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] Initial release notes created for 9.6

[Tom Lane]

Jeff Janes  writes:
> This item:

> "Avoid some spurious waits for AccessExclusiveLocks in hot-standby queries"

> Should be something like

> Avoid some unnecessary cancellations of hot-standy queries due to
> AccessExclusiveLocks replay.

> It was the cancellations, not the waits, which were spurious.

OK, changing to

   
Avoid some unnecessary cancellations of hot-standby queries during
replay of actions that take AccessExclusiveLocks (Jeff Janes)
   

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

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Sun, Apr 17, 2016 at 11:05 PM, Noah Misch  wrote:
> > On Sun, Apr 17, 2016 at 08:04:03PM -0400, Noah Misch wrote:
> >> On Thu, Apr 07, 2016 at 03:50:47PM -0400, Stephen Frost wrote:
> >> > I'm planning to continue going over the patch tomorrow morning with
> >> > plans to push this before the feature freeze deadline.
> >>
> >> > --- a/src/test/regress/expected/rolenames.out
> >> > +++ b/src/test/regress/expected/rolenames.out
> >>
> >> > +GRANT testrol0 TO pg_abc; -- error
> >> > +ERROR:  role "pg_abc" is reserved
> >> > +DETAIL:  Cannot GRANT roles to a reserved role.
> >>
> >> The server still accepts "ALTER ROLE testrol0 USER pg_signal_backend".  It
> >> should block this ALTER ROLE if it blocks the corresponding GRANT.
> >
> > One more thing:
> >
> >> --- a/src/bin/pg_dump/pg_dumpall.c
> >> +++ b/src/bin/pg_dump/pg_dumpall.c
> >> @@ -665,7 +665,7 @@ dumpRoles(PGconn *conn)
> >>   int i;
> >>
> >>   /* note: rolconfig is dumped later */
> >> - if (server_version >= 90500)
> >> + if (server_version >= 90600)
> >
> > This need distinct branches for 9.5 and for 9.6+.  Today's code would treat 
> > a
> > 9.5 cluster like a 9.1 cluster and fail to dump rolbypassrls attributes.
> 
> Stephen, did something in today's pile o' commits fix this?  If so, which one?

No.  I had it in my list but missed it while being anxious about getting
the other patches pushed.

I'll push the fix shortly.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Feature request: make cluster_name GUC useful for psql prompts

[Jerry Sievers]

Steve Crawford  writes:

> That is almost identical to the solution I suggested a week or two ago to 
> someone tackling the issue and the hack works on initial connection.
>
> Connect to a different cluster with "\c", however, and it will leave the 
> prompt showing you connected to the original database which is not good.

True and I've always thought of it as a possible misfeature of psql that
it scans .psqlrc only once.

> Cheers,
> Steve
>
> On Fri, May 6, 2016 at 11:42 AM, Jerry Sievers  wrote:
>
> Peter Eisentraut  writes:
>
> > On 5/5/16 9:21 PM, Steve Crawford wrote:
> >
> >> Adding an escape sequence that references cluster_name would enable
> >> prompts to identify the cluster in a manner that is both consistent and
> >> distinct regardless of access path.
> >
> > I think that would be a good idea.  You could probably design it so
> > that any server parameter reported to the client can be put in a psql
> > prompt.
>
> The OP can easily work around that lack of support with something such as 
> follow...
>
> Add this to ~/.psqlrc[-optional version stuff]
>
> select setting as cluster_name from pg_settings where name = 
> 'cluster_name'  -- do not simicolon terminate this line
> \gset
>
> \set PROMPT1 :cluster_name ': how cool is this:'
>
> >
> >> Potential issues/improvements:
> >>
> >> What should the escape-sequence display if cluster_name is not set or
> >> the cluster is a pre-9.5 version. %M? %m?
> >>
> >> In future server versions should there be a default for cluster_name if
> >> it is not set? If so, what should it be? Would the server canonical
> >> hostname + listen-port be reasonable?
> >
> > Those are good questions.  I don't really like the proposed answers,
> > because that could cause confusion in practical use.
> >
> > --
> > Peter Eisentraut              http://www.2ndQuadrant.com/
> > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
> --
> Jerry Sievers
> Postgres DBA/Development Consulting
> e: postgres.consult...@comcast.net
> p: 312.241.7800
>

-- 
Jerry Sievers
e: jerry.siev...@comcast.net
p: 312.241.7800


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

[Robert Haas]

On Sun, Apr 17, 2016 at 11:05 PM, Noah Misch  wrote:
> On Sun, Apr 17, 2016 at 08:04:03PM -0400, Noah Misch wrote:
>> On Thu, Apr 07, 2016 at 03:50:47PM -0400, Stephen Frost wrote:
>> > I'm planning to continue going over the patch tomorrow morning with
>> > plans to push this before the feature freeze deadline.
>>
>> > --- a/src/test/regress/expected/rolenames.out
>> > +++ b/src/test/regress/expected/rolenames.out
>>
>> > +GRANT testrol0 TO pg_abc; -- error
>> > +ERROR:  role "pg_abc" is reserved
>> > +DETAIL:  Cannot GRANT roles to a reserved role.
>>
>> The server still accepts "ALTER ROLE testrol0 USER pg_signal_backend".  It
>> should block this ALTER ROLE if it blocks the corresponding GRANT.
>
> One more thing:
>
>> --- a/src/bin/pg_dump/pg_dumpall.c
>> +++ b/src/bin/pg_dump/pg_dumpall.c
>> @@ -665,7 +665,7 @@ dumpRoles(PGconn *conn)
>>   int i;
>>
>>   /* note: rolconfig is dumped later */
>> - if (server_version >= 90500)
>> + if (server_version >= 90600)
>
> This need distinct branches for 9.5 and for 9.6+.  Today's code would treat a
> 9.5 cluster like a 9.1 cluster and fail to dump rolbypassrls attributes.

Stephen, did something in today's pile o' commits fix this?  If so, which one?

-- 
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] Feature request: make cluster_name GUC useful for psql prompts

[Steve Crawford]

That is almost identical to the solution I suggested a week or two ago to
someone tackling the issue and the hack works on initial connection.

Connect to a different cluster with "\c", however, and it will leave the
prompt showing you connected to the original database which is not good.

Cheers,
Steve

On Fri, May 6, 2016 at 11:42 AM, Jerry Sievers 
wrote:

> Peter Eisentraut  writes:
>
> > On 5/5/16 9:21 PM, Steve Crawford wrote:
> >
> >> Adding an escape sequence that references cluster_name would enable
> >> prompts to identify the cluster in a manner that is both consistent and
> >> distinct regardless of access path.
> >
> > I think that would be a good idea.  You could probably design it so
> > that any server parameter reported to the client can be put in a psql
> > prompt.
>
> The OP can easily work around that lack of support with something such as
> follow...
>
> Add this to ~/.psqlrc[-optional version stuff]
>
> select setting as cluster_name from pg_settings where name =
> 'cluster_name'  -- do not simicolon terminate this line
> \gset
>
> \set PROMPT1 :cluster_name ': how cool is this:'
>
> >
> >> Potential issues/improvements:
> >>
> >> What should the escape-sequence display if cluster_name is not set or
> >> the cluster is a pre-9.5 version. %M? %m?
> >>
> >> In future server versions should there be a default for cluster_name if
> >> it is not set? If so, what should it be? Would the server canonical
> >> hostname + listen-port be reasonable?
> >
> > Those are good questions.  I don't really like the proposed answers,
> > because that could cause confusion in practical use.
> >
> > --
> > Peter Eisentraut  http://www.2ndQuadrant.com/
> > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
> --
> Jerry Sievers
> Postgres DBA/Development Consulting
> e: postgres.consult...@comcast.net
> p: 312.241.7800
>

Re: [HACKERS] ALTER TABLE lock downgrades have broken pg_upgrade

[Tom Lane]

I wrote:
> I haven't tried to construct a pre-9.1 database that would trigger
> this, but you can make it happen by applying the attached patch
> to create a toast-table-less table in the regression tests,
> and then doing "make check" in src/bin/pg_upgrade.  You get this:

> ...
> Restoring database schemas in the new cluster
> ok
> Creating newly-required TOAST tablesSQL command failed
> ALTER TABLE "public"."i_once_had_a_toast_table" RESET 
> (binary_upgrade_dummy_option);
> ERROR:  AccessExclusiveLock required to add toast table.
> Failure, exiting

> I think possibly the easiest fix for this is to have pg_upgrade,
> instead of RESETting a nonexistent option, RESET something that's
> still considered to require AccessExclusiveLock.  "user_catalog_table"
> would work, looks like; though I'd want to annotate its entry in
> reloptions.c to warn people away from downgrading its lock level.

I tried fixing it like that.  The alternate RESET target had behaved as
expected when I'd tested by hand, but in pg_upgrade it still fails,
only now with

Creating newly-required TOAST tablesSQL command failed
ALTER TABLE "public"."i_need_a_toast_table" RESET (user_catalog_table);
ERROR:  pg_type OID value not set when in binary upgrade mode

This implies that there was some totally other patch, probably quite
pg_upgrade-specific, that broke this case independently of the
lock-downgrade change.

My conclusion is that we probably do need a specific pg_upgrade support
function to handle the case, rather than trying to sneak it through via
ALTER TABLE, which means that we won't be able to back-patch a fix.

I have no more time to work on this, but I think it needs to be fixed, and
I definitely think we had better put in test coverage when we do fix it.
Attached is a proposed patch that adds regression test coverage for this
and a related case (and triggers the failures I've been complaining of).

regards, tom lane

diff --git a/src/test/regress/expected/indirect_toast.out b/src/test/regress/expected/indirect_toast.out
index 4f4bf41..3ed0189 100644
*** a/src/test/regress/expected/indirect_toast.out
--- b/src/test/regress/expected/indirect_toast.out
*** SELECT substring(toasttest::text, 1, 200
*** 149,151 
--- 149,177 
  
  DROP TABLE toasttest;
  DROP FUNCTION update_using_indirect();
+ --
+ -- Create a couple of tables that have unusual TOAST situations, and leave
+ -- them around so that they'll be in the final regression database state.
+ -- This enables testing of these scenarios for pg_upgrade.
+ --
+ -- Table that has a TOAST table, but doesn't really need it.
+ create table i_have_useless_toast_table(f1 int, f2 text);
+ insert into i_have_useless_toast_table values(1, 'foo');
+ alter table i_have_useless_toast_table drop column f2;
+ -- Table that needs a TOAST table and has not got one.  This is uglier...
+ -- we can't actually remove the TOAST table, only unlink it from parent.
+ -- But leaving an orphan TOAST table is good for testing pg_upgrade, anyway.
+ create table i_need_a_toast_table(f1 int, f2 text);
+ insert into i_need_a_toast_table values(1, 'foo');
+ update pg_class set reltoastrelid = 0
+   where relname = 'i_need_a_toast_table';
+ SELECT relname, reltoastrelid <> 0 AS has_toast_table
+FROM pg_class
+WHERE oid::regclass IN ('i_have_useless_toast_table', 'i_need_a_toast_table')
+ORDER BY 1;
+   relname   | has_toast_table 
+ +-
+  i_have_useless_toast_table | t
+  i_need_a_toast_table   | f
+ (2 rows)
+ 
diff --git a/src/test/regress/sql/indirect_toast.sql b/src/test/regress/sql/indirect_toast.sql
index d502480..15640fc 100644
*** a/src/test/regress/sql/indirect_toast.sql
--- b/src/test/regress/sql/indirect_toast.sql
*** SELECT substring(toasttest::text, 1, 200
*** 59,61 
--- 59,85 
  
  DROP TABLE toasttest;
  DROP FUNCTION update_using_indirect();
+ 
+ --
+ -- Create a couple of tables that have unusual TOAST situations, and leave
+ -- them around so that they'll be in the final regression database state.
+ -- This enables testing of these scenarios for pg_upgrade.
+ --
+ 
+ -- Table that has a TOAST table, but doesn't really need it.
+ create table i_have_useless_toast_table(f1 int, f2 text);
+ insert into i_have_useless_toast_table values(1, 'foo');
+ alter table i_have_useless_toast_table drop column f2;
+ 
+ -- Table that needs a TOAST table and has not got one.  This is uglier...
+ -- we can't actually remove the TOAST table, only unlink it from parent.
+ -- But leaving an orphan TOAST table is good for testing pg_upgrade, anyway.
+ create table i_need_a_toast_table(f1 int, f2 text);
+ insert into i_need_a_toast_table values(1, 'foo');
+ update pg_class set reltoastrelid = 0
+   where relname = 'i_need_a_toast_table';
+ 
+ SELECT relname, reltoastrelid 

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Fri, May 6, 2016 at 12:12 PM, Stephen Frost  wrote:
> I've been thinking the same, my flight just arrived into DC and I'll be
> pushing it all shortly after I get home.

To be clear, I wasn't simply saying that you should commit these
patches today instead of tomorrow, although I'm glad you did that and
fully endorse that decision.  I was saying they should have been
committed last week or sooner instead of one business day before beta1
wraps.  I also would have preferred to see each patch go in as it got
done rather than pushing a whole stack of commits all at once.  I
realize that there's nothing you can do about any of this at this
point short of developing a time machine, but I'm mentioning it for
next time.

Thanks for committing, and thanks for watching the BF.

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


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

Re: [HACKERS] [COMMITTERS] pgsql: Add TAP tests for pg_dump

[Robert Haas]

On Fri, May 6, 2016 at 3:14 PM, Andres Freund  wrote:
> On 2016-05-06 15:11:53 -0400, Stephen Frost wrote:
>> * Peter Eisentraut (peter.eisentr...@2ndquadrant.com) wrote:
>> > On 5/6/16 2:06 PM, Stephen Frost wrote:
>> > >Add TAP tests for pg_dump
>> >
>> > I'd be the first to welcome this, but what happened to feature freeze?
>>
>> These are just new tests..?  I assumed that would be welcome during post
>> feature-freeze, and certainly no one raised any concerns about adding
>> these tests during the discussion prior to my commiting them.
>>
>> We back-patch new tests from time to time too, when they're associated
>> with bug fixes, so I'm pretty confused why TAP tests would be an issue
>> to add on HEAD post feature-freeze.
>>
>> If the consensus is that we shouldn't add new tests during feature
>> freeze, I'll revert the patch that added them and add them later, but,
>> for my 2c at least, I think we should be happy to add these even after
>> feature freeze.
>
> +1

I think it's appropriate at least in this case.  It seems to be
related to Stephen wanting to make sure he found all of the bugs he
introduced in pre-freeze commits, which I think can be included under
an expansive definition of a mop-up commit.  I think there's some
limit to what we ought to add to the 9.6 testing infrastructure at
this late date, but I'm not inclined to fight about this one.

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


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

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

[Kevin Grittner]

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

> I had an idea I wanted to test out. The gist of it is to effectively
> have the last slot of timestamp to xid map stored in the latest_xmin
> field and only update the mapping when slot boundaries are crossed.
> See attached WIP patch for details. This way the exclusive lock only
> needs to be acquired once per minute. The common case is a spinlock
> that could be replaced with atomics later.

I rebased the patch Ants posted (attached), and am running
benchmarks on a cthulhu (a big NUMA machine with 8 memory nodes).
Normally I wouldn't post results without a lot more data points
with multiple samples at each, but the initial results have me
wondering whether people would like to see this pushed later today
so that it has some time in the buildfarm and then into beta1.

Running the r/w TPC-B (sort of) load with scale, jobs, and threads
at 1000, and the database configured as I would for a production
server of that size, preliminary TPS results are:

master, -1:  8158
master, 10min: 2019
Ants' patch, 10min: 7804

Basically it just skips the maintenance of the time/xid mapping
unless current time has advanced to a new minute.

I can see arguments for tuning this far in time for the beta, as
well as the argument to wait until after the beta, so I'm just
throwing it out there to see what other people think.  I wouldn't
do it unless I have three runs at -1 and 10min with the patch, all
showing similar numbers.  If the BF chokes on it I would revert
this optimization attempt.

Thoughts?

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


update-old-snapshot-map-once-per-tick-v2.patch
Description: binary/octet-stream

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

Re: [HACKERS] NOT EXIST for PREPARE

[Robert Haas]

On Fri, May 6, 2016 at 2:38 PM, Merlin Moncure  wrote:
> On Fri, Mar 25, 2016 at 8:36 AM, Merlin Moncure  wrote:
>> On Wed, Mar 23, 2016 at 3:10 PM, Stephen Frost  wrote:
>>> Just a thought.  I do still like the general idea of INE support for
>>> PREPARE, but perhaps there's a better option.
>>
>> Admittedly, you make some pretty good points here.  I guess one
>> strategy would be to move them all to a function that sets an advisory
>> lock to signal they've been prepared.  That's pretty safe even in
>> multi-threaded scenarios since only one thread can send queries to the
>> backend at a time.  Advisory locks are pretty arcane though.  Still,
>> I'm coming round to your (and Andres's) point of view. :/
>
> I signed up to review this patch as I thought I'd be a pretty fair
> arbitrator of the "is this or is this not actually useful?" debate.  I
> was initially pretty enthusiastic about this patch but after reviewing
> the various objections I've gradually come around to the opinion that
> the author really ought to demonstrate specific use cases where the
> new syntax actually helps with pain points.  On the one hand, IF NOT
> EXISTS is seems pretty harmless but on the other we try not to accept
> patches for SQL level features that will not (or should not) ever by
> used.

Yeah.  I would assume that if you have a large number of statements
that you want to potentially prepare, it would be smarter to first
issue "SELECT name FROM pg_prepared_statements", and then prepare any
you need that aren't already there.  Blindly pre-preparing everything
doesn't seem like it will be good for performance, and if you do want
to do it for some reason, you can always ignore the error on the
client side.  So I'm not really seeing the use case for this.

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


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

Re: [HACKERS] pg9.6 segfault using simple query (related to use fk for join estimates)

[Robert Haas]

On Thu, May 5, 2016 at 10:48 AM, David Rowley
 wrote:
> On 5 May 2016 at 16:04, David Rowley  wrote:
>> I've started making some improvements to this, but need to talk to
>> Tomas. It's currently in the middle of his night, but will try to
>> catch him in his morning to discuss this with him.
>
> Ok, so I spoke to Tomas about this briefly, and he's asked me to send
> in this patch. He didn't get time to look over it due to some other
> commitments he has today.
>
> I do personally feel that if the attached is not good enough, or not
> very close to good enough then probably the best course of action is
> to revert the whole thing.

Tom, what do you think about this patch?  Is it good enough, or should
we revert the whole thing?

-- 
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] Rename max_parallel_degree?

[Robert Haas]

On Tue, May 3, 2016 at 11:42 PM, David Rowley
 wrote:
>>> Magnus seems OK with the way things are.
>>> Peter wants to change either the fact that it is 0-based or the fact
>>> that it is called degree, but is OK with either.
>>> Tom doesn't like "degree" and also thinks anything called degree
>>> should 1-based, but it sounds like he's more interested in changing
>>> the first thing than the second one
>>> Bruce and JD seemed to like degree -> workers.
>>> JD also suggested another option that nobody else endorsed.
>>> Alvaro suggested another option that nobody else endorsed.
>>>
>>> Does anyone else want to vote?
>>
>> I think the way it is currently in HEAD seems easy to correlate how the
>> feature works, but may be it appears to me that way because I am involved
>> from long time with this implementation.   I also think one can easily
>> confused among max_parallel_workers and max_worker_processes, so if we want
>> to change, my vote goes with changing the default of max_parallel_degree to
>> 1 (as suggested by Peter G.).
>
> I'd like to put my +1 against making the current GUCs with their
> current names 1-based, rather than 0-based. Doing anything else like
> giving them new names seems like reinventing the wheel.
>
> My reasoning is that the only gripe that I understand against the
> current names is that the "degree" term appears not to be aligned with
> what other databases do.
>
> I think that actual rows / (degree+1) might get confusing for people,
> such as in the following EXPLAIN ANALYZE output.
>
>  Workers Launched: 2
>  ->  Partial Aggregate  (cost=96331.21..96331.22 rows=1
> width=8) (actual time=584.297..584.297 rows=1 loops=3)
>->  Parallel Seq Scan on a  (cost=0.00..85914.57
> rows=4166657 width=0) (actual time=1.566..347.091 rows=333
> loops=3)
>
> The above would make more sense with max_parallel_degree=3.
>
> I also think that the parallel query, at its best will have the
> workers working hard for their tuples. In such cases the main process
> will be helping out much more, and the more it helps the more a
> 1-based degree makes sense. Also I can't stretch my imagination enough
> to imagine how any other database can handle worker tuples any
> differently than us... Surely their main process/thread must marshal
> worker's tuples the same as what we do? And if they use a 1-based
> degree for that, then surely we can too.

OK, my reading of this thread is that there is a consensus is to
redefine max_parallel_degree=1 as "no parallelism" and
max_parallel_degree>1 as "parallelism using a leader plus N-1
workers", and along with that, to keep the names unchanged.  However,
I don't think I can get that done before beta1, at least not without a
serious risk of breaking stuff.  I can look at this post-beta1.

-- 
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] Feature request: make cluster_name GUC useful for psql prompts

[Jerry Sievers]

Peter Eisentraut  writes:

> On 5/5/16 9:21 PM, Steve Crawford wrote:
>
>> Adding an escape sequence that references cluster_name would enable
>> prompts to identify the cluster in a manner that is both consistent and
>> distinct regardless of access path.
>
> I think that would be a good idea.  You could probably design it so
> that any server parameter reported to the client can be put in a psql
> prompt.

The OP can easily work around that lack of support with something such as 
follow...

Add this to ~/.psqlrc[-optional version stuff]

select setting as cluster_name from pg_settings where name = 'cluster_name'  -- 
do not simicolon terminate this line
\gset

\set PROMPT1 :cluster_name ': how cool is this:'

>
>> Potential issues/improvements:
>>
>> What should the escape-sequence display if cluster_name is not set or
>> the cluster is a pre-9.5 version. %M? %m?
>>
>> In future server versions should there be a default for cluster_name if
>> it is not set? If so, what should it be? Would the server canonical
>> hostname + listen-port be reasonable?
>
> Those are good questions.  I don't really like the proposed answers,
> because that could cause confusion in practical use.
>
> -- 
> Peter Eisentraut  http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

-- 
Jerry Sievers
Postgres DBA/Development Consulting
e: postgres.consult...@comcast.net
p: 312.241.7800


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

Re: [HACKERS] NOT EXIST for PREPARE

[Merlin Moncure]

On Fri, Mar 25, 2016 at 8:36 AM, Merlin Moncure  wrote:
> On Wed, Mar 23, 2016 at 3:10 PM, Stephen Frost  wrote:
>> Just a thought.  I do still like the general idea of INE support for
>> PREPARE, but perhaps there's a better option.
>
> Admittedly, you make some pretty good points here.  I guess one
> strategy would be to move them all to a function that sets an advisory
> lock to signal they've been prepared.  That's pretty safe even in
> multi-threaded scenarios since only one thread can send queries to the
> backend at a time.  Advisory locks are pretty arcane though.  Still,
> I'm coming round to your (and Andres's) point of view. :/

I signed up to review this patch as I thought I'd be a pretty fair
arbitrator of the "is this or is this not actually useful?" debate.  I
was initially pretty enthusiastic about this patch but after reviewing
the various objections I've gradually come around to the opinion that
the author really ought to demonstrate specific use cases where the
new syntax actually helps with pain points.  On the one hand, IF NOT
EXISTS is seems pretty harmless but on the other we try not to accept
patches for SQL level features that will not (or should not) ever by
used.

So, to the OP, if you could reiterate some specific cases where you
think this would be useful, that will help move the review forward.
(if not, it makes it more likely you'll get marked, "returned with
feedback" -- FYI, and thanks.  Note, I'm not the final word here but I
think I speak for the prevailing opinion in this regard.

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] atomic pin/unpin causing errors

[Andres Freund]

On 2016-05-06 11:15:03 -0700, Jeff Janes wrote:
> > Running the test with cassert enabled I actually get assertion failures,
> > due to the FATAL you added.
> >
> > #1  0x00958dde in ExceptionalCondition (conditionName=0xb36c2a 
> > "!(RefCountErrors == 0)", errorType=0xb361af "FailedAssertion",
> > fileName=0xb36170 
> > "/home/admin/src/postgresql/src/backend/storage/buffer/bufmgr.c", 
> > lineNumber=2506) at 
> > /home/admin/src/postgresql/src/backend/utils/error/assert.c:54
> > #2  0x007c9fc9 in CheckForBufferLeaks () at 
> > /home/admin/src/postgresql/src/backend/storage/buffer/bufmgr.c:2506
> ...
> >
> > You didn't see those?
> 
> Yes, I have been seeing those on assert-enabled builds going back as
> far as I can remember (long before this particular problem started
> showing up). > \I just assumed it was a natural consequence of throwing
> an ERROR from inside a critical section.

There's no critical section here...  The reason we're restarting is
because the FATAL makes the checkpointer exit, which postmaster treats
as a cause to trigger a PANIC:
/*
 * Any unexpected exit of the checkpointer 
(including FATAL
 * exit) is treated as a crash.
 */
HandleChildCrash(pid, exitstatus,
 
_("checkpointer process"));


> I never really understood
> it, why would a panicking process bother to check for buffer leaks in
> the first place?  It is leaking everything, which is why the entire
> system has to be brought down immediately.

Panicing ones don't...


> I have been trying (and failing) to reproduce the problem in more
> recent releases, with and without cassert.  Here is pg_config output
> of one of my current attempts:

If you say "recent releases" you mean that you've not been able to
reproduce it in 9.5, 9.4, ..., not that the issue has vanished in
master, right?


Greetings,

Andres Freund


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

Re: [HACKERS] atomic pin/unpin causing errors

[Jeff Janes]

On Thu, May 5, 2016 at 11:52 AM, Andres Freund  wrote:
> Hi Jeff,
>
> On 2016-04-29 10:38:55 -0700, Jeff Janes wrote:
>> I don't see the problem with an cassert-enabled, probably because it
>> is just too slow to ever reach the point where the problem occurs.
>
> Running the test with cassert enabled I actually get assertion failures,
> due to the FATAL you added.
>
> #1  0x00958dde in ExceptionalCondition (conditionName=0xb36c2a 
> "!(RefCountErrors == 0)", errorType=0xb361af "FailedAssertion",
> fileName=0xb36170 
> "/home/admin/src/postgresql/src/backend/storage/buffer/bufmgr.c", 
> lineNumber=2506) at 
> /home/admin/src/postgresql/src/backend/utils/error/assert.c:54
> #2  0x007c9fc9 in CheckForBufferLeaks () at 
> /home/admin/src/postgresql/src/backend/storage/buffer/bufmgr.c:2506
...
>
> You didn't see those?

Yes, I have been seeing those on assert-enabled builds going back as
far as I can remember (long before this particular problem started
showing up).  I just assumed it was a natural consequence of throwing
an ERROR from inside a critical section.  I never really understood
it, why would a panicking process bother to check for buffer leaks in
the first place?  It is leaking everything, which is why the entire
system has to be brought down immediately.

I have been trying (and failing) to reproduce the problem in more
recent releases, with and without cassert.  Here is pg_config output
of one of my current attempts:

BINDIR = /home/jjanes/pgsql/torn_bisect/bin
DOCDIR = /home/jjanes/pgsql/torn_bisect/share/doc
HTMLDIR = /home/jjanes/pgsql/torn_bisect/share/doc
INCLUDEDIR = /home/jjanes/pgsql/torn_bisect/include
PKGINCLUDEDIR = /home/jjanes/pgsql/torn_bisect/include
INCLUDEDIR-SERVER = /home/jjanes/pgsql/torn_bisect/include/server
LIBDIR = /home/jjanes/pgsql/torn_bisect/lib
PKGLIBDIR = /home/jjanes/pgsql/torn_bisect/lib
LOCALEDIR = /home/jjanes/pgsql/torn_bisect/share/locale
MANDIR = /home/jjanes/pgsql/torn_bisect/share/man
SHAREDIR = /home/jjanes/pgsql/torn_bisect/share
SYSCONFDIR = /home/jjanes/pgsql/torn_bisect/etc
PGXS = /home/jjanes/pgsql/torn_bisect/lib/pgxs/src/makefiles/pgxs.mk
CONFIGURE = 'CFLAGS=-ggdb' '--with-extra-version=-c1543a8'
'--enable-debug' '--with-libxml' '--with-perl' '--with-python'
'--with-ldap' '--with-openssl' '--with-gssapi' '--enable-cassert'
'--prefix=/home/jjanes/pgsql/torn_bisect/'
CC = gcc
CPPFLAGS = -DFRONTEND -D_GNU_SOURCE -I/usr/include/libxml2
CFLAGS = -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -ggdb
CFLAGS_SL = -fpic
LDFLAGS = -L../../src/common -Wl,--as-needed
-Wl,-rpath,'/home/jjanes/pgsql/torn_bisect/lib',--enable-new-dtags
LDFLAGS_EX =
LDFLAGS_SL =
LIBS = -lpgcommon -lpgport -lxml2 -lssl -lcrypto -lgssapi_krb5 -lz
-lreadline -lrt -lcrypt -ldl -lm
VERSION = PostgreSQL 9.6devel-c1543a8

The only difference between this and the ones that did find the ERR
would be toggling --enable-cassert and changing which git commit was
used (and manually applying the gin_alone patch when testing commits
that precede that one's committal.

Linux:  2.6.32-573.22.1.el6.x86_64 #1 SMP Wed Mar 23 03:35:39 UTC 2016
x86_64 x86_64 x86_64 GNU/Linux

/proc/cpu_info:

processor   : 7
vendor_id   : GenuineIntel
cpu family  : 6
model   : 62
model name  : Intel(R) Xeon(R) CPU E5-4640 v2 @ 2.20GHz
stepping: 4
microcode   : 4294967295
cpu MHz : 2199.933
cache size  : 20480 KB
physical id : 0
siblings: 8
core id : 7
cpu cores   : 8
apicid  : 7
initial apicid  : 7
fpu : yes
fpu_exception   : yes
cpuid level : 13
wp  : yes
flags   : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge
mca cmov pat pse36 clflush mmx fxsr sse sse2 ss ht syscall nx lm
constant_tsc rep_good unfair_spinlock pni pclmulqdq ssse3 cx16 sse4_1
sse4_2 popcnt aes xsave avx f16c rdrand hypervisor lahf_lm xsaveopt
fsgsbase smep erms
bogomips: 4399.86
clflush size: 64
cache_alignment : 64
address sizes   : 42 bits physical, 48 bits virtual
power management:

Cheers,

Jeff


-- 
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] SET ROLE and reserved roles

[Stephen Frost]

* Robert Haas (robertmh...@gmail.com) wrote:
> On Thu, May 5, 2016 at 10:41 AM, Stephen Frost  wrote:
> > Barring objections or concerns, I'll push this sometime tomorrow
> > (probably after I get back to DC).
> 
> It really would have been good to get this stuff done sooner.  By the
> time you push this, there will barely be enough time for a buildfarm
> cycle, let alone time for any testing people may be doing against
> latest master to find problems.

Alright, I've pushed all of my pending patches.  If anyone needs me,
I'll be busy reloading the buildfarm page.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Segmentation fault when max_parallel degree is very High

[Robert Haas]

On Wed, May 4, 2016 at 11:01 AM, Tom Lane  wrote:
> Dilip Kumar  writes:
>> When parallel degree is set to very high say 7, there is a segmentation
>> fault in parallel code,
>> and that is because type casting is missing in the code..
>
> I'd say the cause is not having a sane range limit on the GUC.
>
>> or corrupt some memory. Need to typecast
>> *i * PARALLEL_TUPLE_QUEUE_SIZE  --> (Size)i * **PARALLEL_TUPLE_QUEUE_SIZE 
>> *and
>> this will fix
>
> That might "fix" it on 64-bit machines, but not 32-bit.

Yeah, I think what we should do here is use mul_size(), which will
error out instead of crashing.

Putting a range limit on the GUC is a good idea, too, but I like
having overflow checks built into these code paths as a backstop, in
case a value that we think is a safe upper limit turns out to be less
safe than we think ... especially on 32-bit platforms.

I'll go do that, and also limit the maximum parallel degree to 1024,
which ought to be enough for anyone (see what I did there?).

-- 
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] Stopping logical replication protocol

[Vladimir Gordiychuk]

Replication work via Copy API, so for stop replication walcender.c wait
CopyDone. My communication seems like this

FE=> StartReplication(query: START_REPLICATION SLOT
pgjdbc_logical_replication_slot LOGICAL 0/18FCFD0 ("include-xids" 'false',
"skip-empty-xacts" 'true'))
FE=> Query(CopyStart)
<=BE CopyBothResponse
FE=> StandbyStatusUpdate(received: 0/18FCFD0, flushed: 0/0, applied: 0/0,
clock: Tue May 03 22:13:14 MSK 2016)
FE=> CopyData(34)
<=BE CopyData
<=BE CopyData
 <=BE Keepalive(lastServerWal: 0/18FCFD0, clock: Tue May 03 22:13:14 MSK
2016 needReply: false)
 <=BE XLogData(currWal: 0/0, lastServerWal: 0/0, clock: 0)
<=BE CopyData
 <=BE XLogData(currWal: 0/18FD0A0, lastServerWal: 0/18FD0A0, clock: 0)
<=BE CopyData
 <=BE XLogData(currWal: 0/18FD1B0, lastServerWal: 0/18FD1B0, clock: 0)
FE=> StopReplication
<=BE CopyData
FE=> CopyDone
<=BE CopyDone
<=BE CopyData
... after few seconds
<=BE CommandStatus(COPY 0)
<=BE ReadyForQuery(I)

The main idea that i want work with same connection without close it.

2016-05-06 19:45 GMT+03:00 Oleksandr Shulgin :

> On Fri, May 6, 2016 at 5:23 PM, Vladimir Gordiychuk 
> wrote:
>
>> Hi all,
>>
>> During implementing logical replication protocol for pgjdbc
>> https://github.com/pgjdbc/pgjdbc/pull/550 I faced with strange behavior
>> of the *walcender.c*:
>>
>>1. When WAL consumer catchup master and change his state to
>>streaming, not available normally complete replication by send CopyDone
>>message until will not generate/create new WAL record. It occurs because
>>logical decoding located in *WalSndWaitForWal* until will not
>>available next WAL record, and it method receive message from consumer 
>> even
>>reply on CopyDone with CopyDone but ignore exit from loop and we can wait
>>many times waiting CommandStatus & ReadyForQuery packages on consumer.
>>2. Logical decoding ignore message from consumer during decoding and
>>writing transaction in socket(*WalSndWriteData*). It affect long
>>transactions with many changes. Because for example if we start decoding
>>transaction that insert 1 million records and after consume 1% of it date
>>we
>>decide stop replication, it will be not available until whole million
>>record will not send to consumer.
>>
>> How exactly are you stopping the replication?  If you just stop reading
> you'll likely to hit some problems, but what if you also close the
> connection?  I don't think there is any other supported way to do it.
>
> I was working last year on adding support for replication protocol to the
> Python driver: https://github.com/psycopg/psycopg2/pull/322   It would be
> nice if you could skim through this implementation, since it didn't receive
> a whole lot of review.
>
> Cheers.
> --
> Alex
>
>

Re: [HACKERS] Poorly-thought-out handling of double variables in pgbench

[Tom Lane]

Fabien COELHO  writes:
>> Neither way allows for trailing whitespace, though.  I don't see that
>> that's important here.

> No, this is not an issue here for variables specified from the command 
> line. Consider pushing this fix?

Done already.

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] Stopping logical replication protocol

[Oleksandr Shulgin]

On Fri, May 6, 2016 at 5:23 PM, Vladimir Gordiychuk 
wrote:

> Hi all,
>
> During implementing logical replication protocol for pgjdbc
> https://github.com/pgjdbc/pgjdbc/pull/550 I faced with strange behavior
> of the *walcender.c*:
>
>1. When WAL consumer catchup master and change his state to streaming,
>not available normally complete replication by send CopyDone message until
>will not generate/create new WAL record. It occurs because logical decoding
>located in *WalSndWaitForWal* until will not available next WAL
>record, and it method receive message from consumer even reply on CopyDone
>with CopyDone but ignore exit from loop and we can wait many times waiting
>CommandStatus & ReadyForQuery packages on consumer.
>2. Logical decoding ignore message from consumer during decoding and
>writing transaction in socket(*WalSndWriteData*). It affect long
>transactions with many changes. Because for example if we start decoding
>transaction that insert 1 million records and after consume 1% of it date
>we
>decide stop replication, it will be not available until whole million
>record will not send to consumer.
>
> How exactly are you stopping the replication?  If you just stop reading
you'll likely to hit some problems, but what if you also close the
connection?  I don't think there is any other supported way to do it.

I was working last year on adding support for replication protocol to the
Python driver: https://github.com/psycopg/psycopg2/pull/322   It would be
nice if you could skim through this implementation, since it didn't receive
a whole lot of review.

Cheers.
--
Alex

Re: [HACKERS] Poorly-thought-out handling of double variables in pgbench

[Fabien COELHO]

While testing the patch I found a minor preexisting (mine...) bug: when
string-scanning doubles, whether the whole string is consumed or not is
not checked. This means that -D x=0one is interpreted as double 0.



I came up with the attached check, but maybe there is a cleaner way to do
that.


I like the way the zic code does it:



+   char xs;
!   if (sscanf(var->value, "%lf%c", , ) != 1)


Indeed... I cannot say that it is cleaner such, but at least it is short 
and it avoids the strlen call.



Neither way allows for trailing whitespace, though.  I don't see that
that's important here.


No, this is not an issue here for variables specified from the command 
line. Consider pushing this fix?


--
Fabien.


--
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] \crosstabview fixes

[Daniel Verite]

Tom Lane wrote:

> Pushed, thanks.
> BTW, I see we've been spelling your name with an insufficient number
> of accents in the commit logs and release notes.  Can't do much about
> the logs, but will fix the release notes.

I use myself the nonaccented version of my name in "From" headers, as
a concession to mailers that are not always rfc2047-friendly, so I can't
blame anyone for leaving out one or both of these accents :)
Thanks for taking care of this anyway!


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


-- 
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] SET ROLE and reserved roles

[Stephen Frost]

Robert,

On Friday, May 6, 2016, Robert Haas  wrote:

> On Thu, May 5, 2016 at 10:41 AM, Stephen Frost  > wrote:
> > Barring objections or concerns, I'll push this sometime tomorrow
> > (probably after I get back to DC).
>
> It really would have been good to get this stuff done sooner.  By the
> time you push this, there will barely be enough time for a buildfarm
> cycle, let alone time for any testing people may be doing against
> latest master to find problems.
>

I've been thinking the same, my flight just arrived into DC and I'll be
pushing it all shortly after I get home.

Thanks!

Stephen

Re: [HACKERS] SET ROLE and reserved roles

[Robert Haas]

On Thu, May 5, 2016 at 10:41 AM, Stephen Frost  wrote:
> Barring objections or concerns, I'll push this sometime tomorrow
> (probably after I get back to DC).

It really would have been good to get this stuff done sooner.  By the
time you push this, there will barely be enough time for a buildfarm
cycle, let alone time for any testing people may be doing against
latest master to find problems.

-- 
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] Perf Benchmarking and regression.

[Mithun Cy]

On Fri, May 6, 2016 at 8:35 PM, Andres Freund  wrote:
> Also, do you see read-only workloads to be affected too?
Thanks, I have not tested with above specific commitid which reported
performance issue but
At HEAD commit 72a98a639574d2e25ed94652848555900c81a799
Author: Andres Freund 
Date:   Tue Apr 26 20:32:51 2016 -0700

READ-Only (prepared) tests (both when data fits to shared buffers or it
exceeds shared buffer=8GB) performance of master has improved over 9.5

*Sessions* *PostgreSQL-9.5 scale 300* *PostgreSQL-9.6 scale 300* *%diff*
*1* 5287.561594 5213.723197 -1.396454598
*8* 84265.389083 84871.305689 0.719057507
*16* 148330.4155 158661.128315 6.9646624936
*24* 207062.803697 219958.12974 6.2277366155
*32* 265145.089888 290190.501443 9.4459269699
*40* 311688.752973 34.551772 9.0833559212
*48* 327169.9673 372408.073033 13.8270960829
*56* 274426.530496 390629.24948 42.3438356248
*64* 261777.692042 384613.9666 46.9238893505
*72* 210747.55937 376390.162022 78.5976374517
*80* 220192.818648 398128.779329 80.8091570713
*88* 185176.91888 423906.711882 128.9198429512
*96* 161579.719039 421541.656474 160.8877271115
*104* 146935.568434 450672.740567 206.7145316618
*112* 136605.466232 432047.309248 216.2738074582
*120* 127687.175016 455458.086889 256.6983816753
*128* 120413.936453 428127.879242 255.5467845776

*Sessions* *PostgreSQL-9.5 scale 1000* *PostgreSQL-9.6 scale 1000* %diff
*1* 5103.812202 5155.434808 1.01145191
*8* 47741.9041 53117.805096 11.2603405694
*16* 89722.57031 86965.10079 -3.0733287182
*24* 130914.537373 153849.634245 17.5191367836
*32* 197125.725706 212454.474264 7.7761279017
*40* 248489.551052 270304.093767 8.7788571482
*48* 291884.652232 317257.836746 8.6928806705
*56* 304526.216047 359676.785476 18.1102862489
*64* 301440.463174 388324.710185 28.8230206709
*72* 194239.941979 393676.628802 102.6754254511
*80* 144879.527847 383365.678053 164.6099719885
*88* 122894.325326 372905.436117 203.4358463076
*96* 109836.31148 362208.867756 229.7715144249
*104* 103791.981583 352330.402278 239.4582094921
*112* 105189.206682 345722.499429 228.6672752217
*120* 108095.811432 342597.969088 216.939171416
*128* 113242.59492 333821.98763 194.7848270925

Even for READ-WRITE when data fits into shared buffer (scale_factor=300 and
shared_buffers=8GB) performance has improved.
Only case is when data exceeds shared_buffer(scale_factor=1000 and
shared_buffers=8GB) I see some regression.

I will try to run the tests as you have suggested and will report the same.


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

[HACKERS] Stopping logical replication protocol

[Vladimir Gordiychuk]

Hi all,

During implementing logical replication protocol for pgjdbc
https://github.com/pgjdbc/pgjdbc/pull/550 I faced with strange behavior of
the *walcender.c*:

   1. When WAL consumer catchup master and change his state to streaming,
   not available normally complete replication by send CopyDone message until
   will not generate/create new WAL record. It occurs because logical decoding
   located in *WalSndWaitForWal* until will not available next WAL record,
   and it method receive message from consumer even reply on CopyDone with
   CopyDone but ignore exit from loop and we can wait many times waiting
   CommandStatus & ReadyForQuery packages on consumer.
   2. Logical decoding ignore message from consumer during decoding and
   writing transaction in socket(*WalSndWriteData*). It affect long
   transactions with many changes. Because for example if we start decoding
   transaction that insert 1 million records and after consume 1% of it date
   we
   decide stop replication, it will be not available until whole million
   record will not send to consumer. And I see the following problems because
   of this:


   - It will generate many not necessary network traffic.
   - Not available normally stop logical replication, because consumer will
   fail with timeout and it broke scenario when consumer put data to external
   system asynchronously and by success callback send feedback to master
   about flushed LSN, but by fail callback stop replication, and restart it
   from the last successfully sent LSN to external system, because slot
   will be busy and it will increase latency for streaming system.
   - Consumer can send keepalive message with required reply flag to master
   that right now decoding and sending huge transaction, and after some time
   disconnect master because because reply on keepalive will not get. I
   faced with it problem during research  bottledwater-pg
    extension that fail
   each time during receive transaction in that was modify 1 million
   record(restart_lsn was before huge transaction so extension fail again and
   again) disconnect master because not keep keepalive package too long.

I prepare small patch that fix problems describe below. Now *WalSndWriteData
*first check message from consumer and during decode transaction check that
replication still active. KeppAlive message now not send if was get
CopyDone package(keep alive now not necessary we preparing to
complete). *WalSndWaitForWal
*after get CopyDone exit from loop. With apply this patch I get next
measurements
Before
-
logical start and stopping: *15446ms*
logical stopping: *13820ms*

physical start and stopping: 462ms
physical stopping: 348

After
-
logical start and stopping: 2424ms
logical stopping: *26ms*

physical start and stopping: 458ms
physical stopping: 329ms

Where for measurements was use code from pgjdbc

For physical replicaion:

LogSequenceNumber startLSN = getCurrentLSN();
>
> Statement st = sqlConnection.createStatement();
> st.execute("insert into test_logic_table\n"
> + "  select id, md5(random()::text) as name from
> generate_series(1, 100) as id;");
> st.close();
>
> long start = System.nanoTime();
>
> PGReplicationStream stream =
> pgConnection
> .replicationStream()
> .physical()
> .withStartPosition(startLSN)
> .start();
>
> //read single message
> stream.read();
> long startStopping = System.nanoTime();
>
> stream.close();
>
> long now = System.nanoTime();
>
> long startAndStopTime = now - start;
> long stopTime = now - startStopping;
>
> System.out.println(TimeUnit.NANOSECONDS.toMillis(startAndStopTime));
> System.out.println(TimeUnit.NANOSECONDS.toMillis(stopTime));
>

For logical replication:

LogSequenceNumber startLSN = getCurrentLSN();
>
> Statement st = sqlConnection.createStatement();
> st.execute("insert into test_logic_table\n"
> + "  select id, md5(random()::text) as name from
> generate_series(1, 100) as id;");
> st.close();
>
> long start = System.nanoTime();
>
> PGReplicationStream stream =
> pgConnection
> .replicationStream()
> .logical()
> .withSlotName(SLOT_NAME)
> .withStartPosition(startLSN)
> .withSlotOption("include-xids", false)
> .withSlotOption("skip-empty-xacts", true)
> .start();
>
> //read single message
> stream.read();
> long startStopping = System.nanoTime();
>
> stream.close();
>
> long now = System.nanoTime();
>
> long startAndStopTime = now - start;
> long stopTime = now - startStopping;
>
> System.out.println(TimeUnit.NANOSECONDS.toMillis(startAndStopTime));
> System.out.println(TimeUnit.NANOSECONDS.toMillis(stopTime));
>


https://github.com/Gordiychuk/postgres/tree/stopping_logical_replication
From 

Re: [HACKERS] Poorly-thought-out handling of double variables in pgbench

[Tom Lane]

Fabien COELHO  writes:
> This is a definite improvement.

> I like the lazyness between string & numeric forms, and for sorting, that 
> is what was needed doing to have something clean.

> Applied on head, it works for me.

OK, pushed.

> While testing the patch I found a minor preexisting (mine...) bug: when 
> string-scanning doubles, whether the whole string is consumed or not is 
> not checked. This means that -D x=0one is interpreted as double 0.

> I came up with the attached check, but maybe there is a cleaner way to do 
> that.

I like the way the zic code does it:

else /* type should be double */
{
double dv;
  
!   if (sscanf(var->value, "%lf", ) != 1)
{
fprintf(stderr,
"malformed variable \"%s\" value: 
\"%s\"\n",
--- 928,936 
else /* type should be double */
{
double dv;
+   char xs;
  
!   if (sscanf(var->value, "%lf%c", , ) != 1)
{
fprintf(stderr,
"malformed variable \"%s\" value: 
\"%s\"\n",

This relies on the idea that if there is any trailing junk, one character
will get assigned into "xs" and then sscanf will say it filled two fields.
Otherwise, it'll report filling only one field (or none, if there's
no number either).  This requires only a minimal amount of code and no
extra strlen() calculations.

Neither way allows for trailing whitespace, though.  I don't see that
that's important here.

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] Perf Benchmarking and regression.

[Andres Freund]

Hi,

Thanks for benchmarking!

On 2016-05-06 19:43:52 +0530, Mithun Cy wrote:
> 1. # first bad commit: [ac1d7945f866b1928c2554c0f80fd52d7f92] Make idle
> backends exit if the postmaster dies.
> this made performance to drop from
> 
> 15947.21546 (15K +) to 13409.758510 (arround 13K+).

Let's debug this one first, it's a lot more local.  I'm rather surprised
that you're seing a big effect with that "few" TPS/socket operations;
and even more that our efforts to address that problem haven't been
fruitful (given we've verified the fix on a number of machines).

Can you verify that removing
AddWaitEventToSet(FeBeWaitSet, WL_POSTMASTER_DEATH, -1, NULL, NULL);
in src/backend/libpq/pqcomm.c : pq_init() restores performance?

I think it'd be best to test the back/forth on master with
bgwriter_flush_after = 0
checkpointer_flush_after = 0
backend_flush_after = 0
to isolate the issue.

Also, do you see read-only workloads to be affected too?

> 2. # first bad commit: [428b1d6b29ca599c5700d4bc4f4ce4c5880369bf] Allow to
> trigger kernel writeback after a configurable number of writes.

FWIW, it'd be very interesting to test again with a bigger
backend_flush_after setting.


Greetings,

Andres Freund


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

Re: [HACKERS] textlike under the LIKE operator for char(n)

[Kohei KaiGai]

2016-05-06 23:17 GMT+09:00 Kevin Grittner :
> On Fri, May 6, 2016 at 8:58 AM, Kohei KaiGai  wrote:
>
>> postgres=# select 'abcd'::char(20) LIKE 'ab%cd';
>>  ?column?
>> --
>>  f
>> (1 row)
>>
>> postgres=# select 'abcd'::char(4) LIKE 'ab%cd';
>>  ?column?
>> --
>>  t
>> (1 row)
>>
>> LIKE operator (that is eventually processed by textlike) considers the
>> padding space of char(n) data type as a part of string.
>
> The SQL standard generally requires this for CHAR(n) columns.
>
>> On the other hands, equal operator ignores the padding space when it
>> compares two strings.
>>
>> postgres=# select 'abcd'::char(20) = 'abcd';
>>  ?column?
>> --
>>  t
>> (1 row)
>>
>> postgres=# select 'abcd'::char(4) = 'abcd';
>>  ?column?
>> --
>>  t
>> (1 row)
>
> The SQL standard specifically requires this exception to the
> general rule.
>
>> Is this behavior as expected? or, bug?
>
> This has been discussed on community lists multiple times in the
> past; you might want to search the archives.  I'm not inclined to
> dig through the standard for details on this point again right now,
> but in general the behaviors we provide for CHAR(n) are mandated by
> standard.  It would not entirely shock me if there are some corner
> cases where different behavior could be allowed or even more
> correct, but my recollection is that what you have shown is all
> required to work that way.
>
Thanks, I couldn't find out the reason of the behavior shortly.
Requirement by SQL standard is a clear guidance even if it looks
a bit mysterious.

> Generally, I recommend avoiding CHAR(n) columns like the plague.
>
Yep, I agree. I found this matter when I port LIKE operator on GPU,
not a time when some real-life query tried to use char(n).

Best regards,
-- 
KaiGai Kohei 


-- 
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] \crosstabview fixes

[Tom Lane]

"Daniel Verite"  writes:
> Checking http://www.postgresql.org/docs/devel/static/app-psql.html ,
> I notice that the last example is still using the syntax for arguments
> that has been deprecated by commit 6f0d6a507, as discussed in this
> thread.

Ooops.

> A fix to psql-ref.sgml is attached.

Pushed, thanks.

BTW, I see we've been spelling your name with an insufficient number
of accents in the commit logs and release notes.  Can't do much about
the logs, but will fix the release notes.

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] textlike under the LIKE operator for char(n)

[Kevin Grittner]

On Fri, May 6, 2016 at 8:58 AM, Kohei KaiGai  wrote:

> postgres=# select 'abcd'::char(20) LIKE 'ab%cd';
>  ?column?
> --
>  f
> (1 row)
>
> postgres=# select 'abcd'::char(4) LIKE 'ab%cd';
>  ?column?
> --
>  t
> (1 row)
>
> LIKE operator (that is eventually processed by textlike) considers the
> padding space of char(n) data type as a part of string.

The SQL standard generally requires this for CHAR(n) columns.

> On the other hands, equal operator ignores the padding space when it
> compares two strings.
>
> postgres=# select 'abcd'::char(20) = 'abcd';
>  ?column?
> --
>  t
> (1 row)
>
> postgres=# select 'abcd'::char(4) = 'abcd';
>  ?column?
> --
>  t
> (1 row)

The SQL standard specifically requires this exception to the
general rule.

> Is this behavior as expected? or, bug?

This has been discussed on community lists multiple times in the
past; you might want to search the archives.  I'm not inclined to
dig through the standard for details on this point again right now,
but in general the behaviors we provide for CHAR(n) are mandated by
standard.  It would not entirely shock me if there are some corner
cases where different behavior could be allowed or even more
correct, but my recollection is that what you have shown is all
required to work that way.

Generally, I recommend avoiding CHAR(n) columns like the plague.

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

[HACKERS] Perf Benchmarking and regression.

[Mithun Cy]

I tried to do some benchmarking on postgres master head
commit 72a98a639574d2e25ed94652848555900c81a799
Author: Andres Freund 
Date:   Tue Apr 26 20:32:51 2016 -0700

CASE : Read-Write Tests when data exceeds shared buffers.

Non Default settings and test
./postgres -c shared_buffers=8GB -N 200 -c min_wal_size=15GB -c
max_wal_size=20GB -c checkpoint_timeout=900 -c maintenance_work_mem=1GB -c
checkpoint_completion_target=0.9 &

./pgbench -i -s 1000 postgres

./pgbench -c $threads -j $threads -T 1800 -M prepared postgres


Machine : "cthulhu" 8 node numa machine with 128 hyper threads.
>numactl --hardware
available: 8 nodes (0-7)
node 0 cpus: 0 65 66 67 68 69 70 71 96 97 98 99 100 101 102 103
node 0 size: 65498 MB
node 0 free: 37885 MB
node 1 cpus: 72 73 74 75 76 77 78 79 104 105 106 107 108 109 110 111
node 1 size: 65536 MB
node 1 free: 31215 MB
node 2 cpus: 80 81 82 83 84 85 86 87 112 113 114 115 116 117 118 119
node 2 size: 65536 MB
node 2 free: 15331 MB
node 3 cpus: 88 89 90 91 92 93 94 95 120 121 122 123 124 125 126 127
node 3 size: 65536 MB
node 3 free: 36774 MB
node 4 cpus: 1 2 3 4 5 6 7 8 33 34 35 36 37 38 39 40
node 4 size: 65536 MB
node 4 free: 62 MB
node 5 cpus: 9 10 11 12 13 14 15 16 41 42 43 44 45 46 47 48
node 5 size: 65536 MB
node 5 free: 9653 MB
node 6 cpus: 17 18 19 20 21 22 23 24 49 50 51 52 53 54 55 56
node 6 size: 65536 MB
node 6 free: 50209 MB
node 7 cpus: 25 26 27 28 29 30 31 32 57 58 59 60 61 62 63 64
node 7 size: 65536 MB
node 7 free: 43966 MB
node distances:
node   0   1   2   3   4   5   6   7
  0:  10  21  21  21  21  21  21  21
  1:  21  10  21  21  21  21  21  21
  2:  21  21  10  21  21  21  21  21
  3:  21  21  21  10  21  21  21  21
  4:  21  21  21  21  10  21  21  21
  5:  21  21  21  21  21  10  21  21
  6:  21  21  21  21  21  21  10  21
  7:  21  21  21  21  21  21  21  10


I see some regression when compared to 9.5

*Sessions* *PostgreSQL-9.5 scale 1000* *PostgreSQL-9.6 scale 1000* %diff
*1* 747.367249 892.149891 19.3723557185
*8* 5281.282799 4941.905008 -6.4260484416
*16* 9000.915419 8695.396233 -3.3943123758
*24* 11852.839627 10843.328776 -8.5170379653
*32* 14323.048334 11977.505153 -16.3760054864
*40* 16098.926583 12195.447024 -24.2468312336
*48* 16959.646965 12639.951087 -25.4704351271
*56* 17157.737762 12543.212929 -26.894715941
*64* 17201.914922 12628.002422 -26.5895542487
*72* 16956.994835 11280.870599 -33.4736448954
*80* 16775.954896 11348.830603 -32.3506132834
*88* 16609.137558 10823.465121 -34.834273705
*96* 16510.099404 11091.757753 -32.8183466278
*104* 16275.724927 10665.743275 -34.4683980416
*112* 16141.815128 10977.84664 -31.9912503461
*120* 15904.086614 10716.17755 -32.6199749153
*128* 15738.391503 10962.333439 -30.3465450271

When I run git bisect on master (And this is for 128 clients).

2 commitIds which affected the performance

1. # first bad commit: [ac1d7945f866b1928c2554c0f80fd52d7f92] Make idle
backends exit if the postmaster dies.
this made performance to drop from

15947.21546 (15K +) to 13409.758510 (arround 13K+).

2. # first bad commit: [428b1d6b29ca599c5700d4bc4f4ce4c5880369bf] Allow to
trigger kernel writeback after a configurable number of writes.

this made performance to drop further to 10962.333439 (10K +)

I think It did not recover afterwards.
-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

[HACKERS] textlike under the LIKE operator for char(n)

[Kohei KaiGai]

Hi,

I found a mysterious behavior when we use LIKE operator on char(n) data type.


postgres=# select 'abcd'::char(20) LIKE 'ab%cd';
 ?column?
--
 f
(1 row)

postgres=# select 'abcd'::char(4) LIKE 'ab%cd';
 ?column?
--
 t
(1 row)

LIKE operator (that is eventually processed by textlike) considers the
padding space of char(n) data type as a part of string.

On the other hands, equal operator ignores the padding space when it
compares two strings.

postgres=# select 'abcd'::char(20) = 'abcd';
 ?column?
--
 t
(1 row)

postgres=# select 'abcd'::char(4) = 'abcd';
 ?column?
--
 t
(1 row)

The LIKE operator on char(n) data type is implemented by textlike().

at pg_proc.h:
DATA(insert OID = 1631 (  bpcharlike   PGNSP PGUID 12 1 0 0 0 f f
f f t f i s 2 0 16 "1042 25" _null_ _null_ _null_ _null_ _null_
textlike _null_ _null_ _null_ ));

It calls GenericMatchText() with length of the target string,
calculated by VARSIZE_ANY_EXHDR, however, it includes the padding
space.
It seems to me bcTruelen() gives the correct length for char(n) data
types, instead of this macro.

Is this behavior as expected? or, bug?

Thanks,
-- 
KaiGai Kohei 


-- 
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] \crosstabview fixes

[Daniel Verite]

Tom Lane wrote:

> > "Daniel Verite"  writes:
> >> To avoid the confusion between "2:4" and "2":"4" or 2:4,
> >> and the ambiguity with a possibly existing "2:4" column,
> >> maybe we should abandon this syntax and require the optional
> >> scolH to be on its own at the end of the command.
> 
> > That would be OK with me; it's certainly less of a hack than what's
> > there now.  (I went back and forth about how much effort to put into
> > dealing with the colon syntax; I think the version I have in my patch
> > would be all right, but it's not perfect.)
> 
> Here's a patch along those lines.  Any objections?

Checking http://www.postgresql.org/docs/devel/static/app-psql.html ,
I notice that the last example is still using the syntax for arguments
that has been deprecated by commit 6f0d6a507, as discussed in this
thread.
A fix to psql-ref.sgml is attached.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 4160665..df79a37 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -4173,7 +4173,7 @@ numerical order and columns with an independant, ascending numerical order.
 testdb= SELECT t1.first as "A", t2.first+100 AS "B", t1.first*(t2.first+100) as "AxB",
 testdb( row_number() over(order by t2.first) AS ord
 testdb( FROM my_table t1 CROSS JOIN my_table t2 ORDER BY 1 DESC
-testdb( \crosstabview A B:ord AxB
+testdb( \crosstabview "A" "B" "AxB" ord
  A | 101 | 102 | 103 | 104 
 ---+-+-+-+-
  4 | 404 | 408 | 412 | 416

-- 
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] old_snapshot_threshold's interaction with hash index

[Kevin Grittner]

On Fri, May 6, 2016 at 12:45 AM, Amit Kapila 
wrote:
> On Wed, May 4, 2016 at 7:48 PM, Kevin Grittner  wrote:
>> On Tue, May 3, 2016 at 11:48 AM, Robert Haas 
wrote:
>>
>>> OK, I see now: the basic idea here is that we can't prune based on the
>>> newer XID unless the page LSN is guaranteed to advance whenever data
>>> is removed.  Currently, we attempt to limit bloat in non-unlogged,
>>> non-catalog tables.  You're saying we can instead attempt to limit
>>> bloat only in non-unlogged, non-catalog tables without hash indexes,
>>> and that will fix this issue.  Am I right?
>>
>> As a first cut, something like the attached.
>
> Patch looks good to me.  I have done some testing with hash and
> btree indexes and it works as expected.

Pushed with the addition of a paragraph to the docs regarding this
and some other situations where people have been unclear about what
to expect.

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

Re: [HACKERS] psql :: support for \ev viewname and \sv viewname

[Dean Rasheed]

On 4 May 2016 at 13:23, Peter Eisentraut
 wrote:
> On 5/4/16 3:21 AM, Dean Rasheed wrote:
>> Well, appendStringLiteralAH() takes both, so that sets a precedent.
> Works for me.  Could you supply an updated patch with a static function
> instead of a macro?  Then I think this is good to go.
>
> (bonus points for some tests)
>

OK, pushed that way.

I didn't get round to adding any tests though. I strikes me that the
most likely bugs in this area are bugs of omission, like this and the
missing PARALLEL SAFE recently fixed for functions. Ideally tests
would be able to spot those kinds of issues, but it's not obvious how
to write such tests.

Regards,
Dean


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

Re: [HACKERS] Feature request: make cluster_name GUC useful for psql prompts

[Peter Eisentraut]

On 5/5/16 9:21 PM, Steve Crawford wrote:

Adding an escape sequence that references cluster_name would enable
prompts to identify the cluster in a manner that is both consistent and
distinct regardless of access path.


I think that would be a good idea.  You could probably design it so that 
any server parameter reported to the client can be put in a psql prompt.



Potential issues/improvements:

What should the escape-sequence display if cluster_name is not set or
the cluster is a pre-9.5 version. %M? %m?

In future server versions should there be a default for cluster_name if
it is not set? If so, what should it be? Would the server canonical
hostname + listen-port be reasonable?


Those are good questions.  I don't really like the proposed answers, 
because that could cause confusion in practical use.


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

Re: [HACKERS] Is pg_control file crashsafe?

[Alex Ignatov]

On 05.05.2016 7:16, Amit Kapila wrote:

On Wed, May 4, 2016 at 8:03 PM, Tom Lane > wrote:
 >
 > Amit Kapila > writes:
 > > On Wed, May 4, 2016 at 4:02 PM, Alex Ignatov
>
 > > wrote:
 > >> On 03.05.2016 2:17, Tom Lane wrote:
 > >>> Writing a single sector ought to be atomic too.
 >
 > >> pg_control is 8k long(i think it is legth of one page in default PG
 > >> compile settings).
 >
 > > The actual data written is always sizeof(ControlFileData) which
should be
 > > less than one sector.
 >
 > Yes.  We don't care what happens to the rest of the file as long as the
 > first sector's worth is updated atomically.  See the comments for
 > PG_CONTROL_SIZE and the code in ReadControlFile/WriteControlFile.
 >
 > We could change to a different PG_CONTROL_SIZE pretty easily, and there's
 > certainly room to argue that reducing it to 512 or 1024 would be more
 > efficient.  I think the motivation for setting it at 8K was basically
 > "we're already assuming that 8K writes are efficient, so let's assume
 > it here too".  But since the file is only written once per checkpoint,
 > efficiency is not really a key selling point anyway.  If you could make
 > an argument that some other size would reduce the risk of failures,
 > it would be interesting --- but I suspect any such argument would be
 > very dependent on the quirks of a specific file system.
 >

How about using 512 bytes as a write size and perform direct writes
rather than going via OS buffer cache for control file?   Alex, is the
issue reproducible (to ensure that if we try to solve it in some way, do
we have way to test it as well)?

 >
 > One point worth considering is that on most file systems, rewriting
 > a fraction of a page is *less* efficient than rewriting a full page,
 > because the kernel first has to read in the old contents to fill
 > the disk buffer it's going to partially overwrite with new data.
 > This motivates against trying to reduce the write size too much.
 >

Yes, you are very much right and I have observed that recently during my
work on WAL Re-Writes [1].  However, I think that won't be the issue if
we use direct writes for control file.


[1] -
http://www.postgresql.org/message-id/CAA4eK1+=O33dZZ=jbtjxbfyd67r5dlcqfyomj4f-qmfxbp1...@mail.gmail.com

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


Hi!
No issue happened only once. Also any attempts to reproduce it is not 
successful yet



--
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 pg_control file crashsafe?

[Alex Ignatov]

On 06.05.2016 0:42, Greg Stark wrote:

On 5 May 2016 12:32 am, "Tom Lane" > wrote:
 >
 > To repeat, I'm pretty hesitant to change this logic.  While this is not
 > the first report we've ever heard of loss of pg_control, I believe I
could
 > count those reports without running out of fingers on one hand --- and
 > that's counting since the last century. It will take quite a lot of
 > evidence to convince me that some other implementation will be more
 > reliable.  If you just come and present a patch to use direct write, or
 > rename, or anything else for that matter, I'm going to reject it out of
 > hand unless you provide very strong evidence that it's going to be more
 > reliable than the current code across all the systems we support.

One thing we could do without much worry of being less reliable would be
to keep two copies of pg_control. Write one, fsync, then write to the
other and fsync that one.

Oracle keeps a copy of the old control file so that you can always go
back to an older version if a hardware or software bug currupts it. But
they keep a lot more data in their control file and they can be quite large.

Oracle can create more then one copy of control file. They are the same, 
not old copy and current. And their advise is just to store this copies 
on separate storage to be more fault tolerant.


PS By the way on my initial post about "is pg_control safe" i wrote in p 
3. some thoughts about multiple copies of pg_control file. Glad to see 
identity of views on this issue



--
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] Initial release notes created for 9.6

[Christian Ullrich]

* Tom Lane wrote:


I've pushed a first cut at release notes for 9.6.  There's a good deal
of work to do yet:


+
+   
+Add new SSPI authentication parameters compat_realm
+and upn_usename, to make it possible to make SSPI
+work more like GSSAPI (Christian Ullrich)
+   

It is upn_username, not usename. Typo in the commit message.

"Make SSPI work more like GSSAPI" reads like it changed authentication 
behavior in some fundamental way, and as if SSPI did not work like 
GSSAPI without it. The difference in behavior of include_realm between 
GSSAPI and SSPI is not caused by SSPI, but is an implementation detail 
on our end.


I suggest writing "use the Kerberos realm name for authentication 
instead of the NetBIOS name" either in place of the existing description 
or together with it.


Thanks,

--
Christian





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