Re: [HACKERS] WIP: Covering + unique indexes.

[Amit Kapila]

On Tue, Sep 6, 2016 at 10:18 PM, Anastasia Lubennikova
 wrote:
> 28.08.2016 09:13, Amit Kapila:
>
> On Mon, Aug 15, 2016 at 8:15 PM, Anastasia Lubennikova
>  wrote:
>
>
> So the patch is correct.
> We can go further and remove this index_truncate_tuple() call, because
> the first key of any inner (or root) page doesn't need any key at all.
>

Anyway, I think truncation happens if the page is at leaf level and
that is ensured by check, so I think we can't remove this:
+ if (indnkeyatts != indnatts && P_ISLEAF(pageop))


-- I have one more question regarding this truncate high-key concept.
I think if high key is truncated, then during insertion, for cases
like below it move to next page, whereas current page needs to be
splitted.

Assume index on c1,c2,c3 and c2,c3 are including columns.

Actual high key on leaf Page X -
3, 2 , 2
Truncated high key on leaf Page X
3

New insertion key
3, 1, 2

Now, I think for such cases during insertion if the page X doesn't
have enough space, it will move to next page whereas ideally, it
should split current page.  Refer function _bt_findinsertloc() for
this logic.

Is this truncation concept of high key needed for correctness of patch
or is it just to save space in index?   If you need this, then I think
nbtree/Readme needs to be updated.


-- I am getting Assertion failure when I use this patch with database
created with a build before this patch.  However, if I create a fresh
database it works fine.  Assertion failure details are as below:

LOG:  database system is ready to accept connections
LOG:  autovacuum launcher started
TRAP: unrecognized TOAST vartag("((bool) 1)", File: "src/backend/access/common/h
eaptuple.c", Line: 532)
LOG:  server process (PID 1404) was terminated by exception 0x8003
HINT:  See C include file "ntstatus.h" for a description of the hexadecimal valu
e.
LOG:  terminating any other active server processes

--
@@ -1260,14 +1262,14 @@ RelationInitIndexAccessInfo(Relation relation)
  * Allocate arrays to hold data
  */
  relation->rd_opfamily = (Oid *)
- MemoryContextAllocZero(indexcxt, natts * sizeof(Oid));
+ MemoryContextAllocZero(indexcxt, indnkeyatts * sizeof(Oid));
  relation->rd_opcintype = (Oid *)
- MemoryContextAllocZero(indexcxt, natts * sizeof(Oid));
+ MemoryContextAllocZero(indexcxt, indnkeyatts * sizeof(Oid));

  amsupport = relation->rd_amroutine->amsupport;
  if (amsupport > 0)
  {
- int nsupport = natts * amsupport;
+ int nsupport = indnatts * amsupport;

  relation->rd_support = (RegProcedure *)
  MemoryContextAllocZero(indexcxt, nsupport * sizeof(RegProcedure));
@@ -1281,10 +1283,10 @@ RelationInitIndexAccessInfo(Relation relation)
  }

  relation->rd_indcollation = (Oid *)
- MemoryContextAllocZero(indexcxt, natts * sizeof(Oid));
+ MemoryContextAllocZero(indexcxt, indnatts * sizeof(Oid));

Can you add a comment in above code or some other related place as to
why you need some attributes in relcache entry of size indnkeyatts and
others of size indnatts?

--
@@ -63,17 +63,26 @@ _bt_mkscankey(Relation rel, IndexTuple itup)
 {
  ScanKey skey;
  TupleDesc itupdesc;
- int natts;
+ int indnatts,
+ indnkeyatts;
  int16   *indoption;
  int i;

  itupdesc = RelationGetDescr(rel);
- natts = RelationGetNumberOfAttributes(rel);
+ indnatts = IndexRelationGetNumberOfAttributes(rel);
+ indnkeyatts = IndexRelationGetNumberOfKeyAttributes(rel);
  indoption = rel->rd_indoption;

- skey = (ScanKey) palloc(natts * sizeof(ScanKeyData));
+ Assert(indnkeyatts != 0);
+ Assert(indnkeyatts <= indnatts);

Here I think you need to declare indnatts as PG_USED_FOR_ASSERTS_ONLY,
otherwise it will give warning on some platforms.


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


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

Re: [HACKERS] pgbench - minor fix for meta command only scripts

[Fabien COELHO]

Hello Heikki,

Yeah, it really is quite a mess. I tried to review your patch, and I think 
it's correct, but I couldn't totally convince myself, because of the existing 
messiness of the logic.


Alas:-(


So I bit the bullet and started refactoring.


Wow!

I came up with the attached. It refactors the logic in doCustom() into a 
state machine.


Sounds good! This can only help.


I think this is much clearer, what do you think?


I think that something was really needed. I'm going to review and test 
this patch very carefully, probably over next week-end, and report.


--
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] Improvements in psql hooks for variables

[Ashish Tyagi]

> Sorry about that, I forgot to make clean, here's an updated patch.
Ongoing CMake changes will help to avoid such things, "out of source build".

On Mon, Sep 19, 2016 at 6:20 PM, Daniel Verite 
wrote:

> Rahila Syed wrote:
>
>
> > I am beginning to review this patch. Initial comment. I got following
> > compilation error when I applied the patch on latest sources and run
> make.
>
> Sorry about that, I forgot to make clean, here's an updated patch.
>
> 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] Speed up Clog Access by increasing CLOG buffers

[Dilip Kumar]

On Tue, Sep 20, 2016 at 8:37 AM, Amit Kapila  wrote:
> I think it is former (8 socket machine).

I confirm this is 8 sockets machine(cthulhu)
>

>
> You point related to high-client count is valid and I am sure it won't
> give noticeable benefit at lower client-count as the the
> CLOGControlLock contention starts impacting only at high-client count.
> I am not sure if it is good idea to reject a patch which helps in
> stabilising the performance (helps in falling off the cliff) when the
> processes increases the number of cores (or hardware threads)
>
>>  If you have to work that hard
>> to find a big win, and tests under more reasonable conditions show no
>> benefit, it's not clear to me that it's really worth the time we're
>> all spending benchmarking and reviewing this, or the risk of bugs, or
>> the damage to the SLRU abstraction layer.
>
> I agree with you unless it shows benefit on somewhat more usual
> scenario's, we should not accept it.  So shouldn't we wait for results
> of other workloads like simple-update or tpc-b on bigger machines
> before reaching to conclusion?

+1

My test are under run, I will post it soon..


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[Amit Kapila]

On Tue, Sep 20, 2016 at 12:40 AM, Robert Haas  wrote:
> On Sun, Sep 18, 2016 at 5:11 PM, Tomas Vondra
>  wrote:
>> IMHO in the ideal case the first message in this thread would provide a test
>> case, demonstrating the effect of the patch. Then we wouldn't have the issue
>> of looking for a good workload two years later.
>>
>> But now that I look at the first post, I see it apparently used a plain
>> tpc-b pgbench (with synchronous_commit=on) to show the benefits, which is
>> the workload I'm running right now (results sometime tomorrow).
>>
>> That workload clearly uses no savepoints at all, so I'm wondering why you
>> suggested to use several of them - I know you said that it's to show
>> differences between the approaches, but why should that matter to any of the
>> patches (and if it matters, why I got almost no differences in the
>> benchmarks)?
>>
>> Pardon my ignorance, CLOG is not my area of expertise ...
>
> It's possible that the effect of this patch depends on the number of
> sockets.  EDB test machine cthulhu as 8 sockets, and power2 has 4
> sockets.  I assume Dilip's tests were run on one of those two,
>

I think it is former (8 socket machine).

> although he doesn't seem to have mentioned which one.  Your system is
> probably 2 or 4 sockets, which might make a difference.  Results might
> also depend on CPU architecture; power2 is, unsurprisingly, a POWER
> system, whereas I assume you are testing x86.  Maybe somebody who has
> access should test on hydra.pg.osuosl.org, which is a community POWER
> resource.  (Send me a private email if you are a known community
> member who wants access for benchmarking purposes.)
>
> Personally, I find the results so far posted on this thread thoroughly
> unimpressive.  I acknowledge that Dilip's results appear to show that
> in a best-case scenario these patches produce a rather large gain.
> However, that gain seems to happen in a completely contrived scenario:
> astronomical client counts, unlogged tables, and a test script that
> maximizes pressure on CLogControlLock.
>

You are right that the scenario is somewhat contrived, but I think he
hasn't posted the results for simple-update or tpc-b kind of scenarios
for pgbench, so we can't conclude that those won't show benefit.  I
think we can see benefits with synchronous_commit=off as well may not
be as good as with unlogged tables.  The other thing to keep in mind
is that reducing contention on one lock (assume in this case
CLOGControlLock) also gives benefits when we reduce contention on
other locks (like ProcArrayLock, WALWriteLock, ..).  Last time we have
verified this effect with Andres's patch (cache the snapshot) which
reduces the remaining contention on ProcArrayLock.  We have seen that
individually that patch gives some benefit, but by removing the
contention on CLOGControlLock with the patches (increase the clog
buffers and grouping stuff, each one helps) discussed in this thread,
it gives much bigger benefit.

You point related to high-client count is valid and I am sure it won't
give noticeable benefit at lower client-count as the the
CLOGControlLock contention starts impacting only at high-client count.
I am not sure if it is good idea to reject a patch which helps in
stabilising the performance (helps in falling off the cliff) when the
processes increases the number of cores (or hardware threads)

>  If you have to work that hard
> to find a big win, and tests under more reasonable conditions show no
> benefit, it's not clear to me that it's really worth the time we're
> all spending benchmarking and reviewing this, or the risk of bugs, or
> the damage to the SLRU abstraction layer.

I agree with you unless it shows benefit on somewhat more usual
scenario's, we should not accept it.  So shouldn't we wait for results
of other workloads like simple-update or tpc-b on bigger machines
before reaching to conclusion?


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


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

[HACKERS] Remove the comment on the countereffectiveness of large shared_buffers on Windows

[Tsunakawa, Takayuki]

Hello,

> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Magnus Hagander
> On Wed, Aug 24, 2016 at 4:35 AM, Tsunakawa, Takayuki
>  wrote:
>   As a similar topic, I wonder whether the following still holds true,
> after many improvements on shared buffer lock contention.
> 
>   https://www.postgresql.org/docs/devel/static/runtime-config-re
> source.html
> 
>   "The useful range for shared_buffers on Windows systems
> is generally from 64MB to 512MB."
> 
> 
> 
> 
> Yes, that may very much be out of date as well. A good set of benchmarks
> around that would definitely be welcome.


I'd like to propose the above-mentioned comment from the manual.  The patch is 
attached.

I ran read-only and read-write modes of pgbench, and could not see any apparent 
decrease in performance when I increased shared_buffers.  The scaling factor is 
200, where the database size is roughly 3GB.  I ran the benchmark on my Windows 
10 PC with 6 CPU cores and 16GB of RAM.  The database and WAL is stored on the 
same HDD.

<>
@echo off
for %%s in (256MB 512MB 1GB 2GB 4GB) do (
  pg_ctl -w -o "-c shared_buffers=%%s" start
  pgbench -c18 -j6 -T60 -S bench >> g:\b.txt 2>&1
  pg_ctl -t 3600 stop
)

<>
shared_buffers  tps
256MB  63056
512MB  63918
1GB  65520
2GB  66840
4GB  68270

<>
shared_buffers  tps
256MB  1138
512MB  1187
1GB  1571
2GB  1650
4GB  1598

Regards
Takayuki Tsunakawa



win_shrdbuf_perf.patch
Description: win_shrdbuf_perf.patch

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

Re: [HACKERS] Hash Indexes

[AP]

On Mon, Sep 19, 2016 at 05:50:13PM +1200, Mark Kirkwood wrote:
> >I'm rather unenthused about having a hash index implementation that's
> >mildly better in some corner cases, but otherwise doesn't have much
> >benefit. That'll mean we'll have to step up our user education a lot,
> >and we'll have to maintain something for little benefit.
> 
> While I see the point of what you are saying here, I recall all previous
> discussions about has indexes tended to go a bit like this:
> 
> - until WAL logging of hash indexes is written it is not worthwhile trying
> to make improvements to them
> - WAL logging will be a lot of work, patches 1st please
> 
> Now someone has done that work, and we seem to be objecting that because
> they are not improved then the patches are (maybe) not worthwhile. I think
> that is - essentially - somewhat unfair.

My understanding of hash indexes is that they'd be good for indexing
random(esque) data (such as UUIDs or, well, hashes like shaX). If so
then I've got a DB that'll be rather big that is the very embodiment
of such a use case. It indexes such data for equality comparisons
and runs on SELECT, INSERT and, eventually, DELETE.

Lack of WAL and that big warning in the docs is why I haven't used it.

Given the above, many lamentations from me that it wont be available
for 9.6. :( When 10.0 comes I'd probably go to the bother of re-indexing
with hash indexes.

Andrew


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

Re: [HACKERS] [PATCH] Transaction traceability - txid_status(bigint)

[Craig Ringer]

On 16 September 2016 at 21:28, Robert Haas  wrote:
> On Thu, Sep 15, 2016 at 8:52 PM, Craig Ringer  wrote:
>> On 2 September 2016 at 23:29, Petr Jelinek  wrote:
>>
>>> You could put it to txid.c where all the other txid stuff is in?
>>
>> Yeah, even though it's in adt/ I think it'll do.
>>
>> I thought I'd need get_xid_in_recent_past() for catalog_xmin hot
>> standby feedback, but upon closer examination the needed logic isn't
>> the same anymore. txid_status() wants to ensure clog lookups are safe
>> and limit by oldest xid, wheras the walsender doesn't actually care
>> about that and is just avoiding wrapped xids.
>>
>> I'm just going back to how it was, all in adt/txid.c, and making it
>> static again. We can move it and make it non-static if a need to do so
>> comes up.
>>
>> Attached rebased patch updated and vs master.
>
> You appear to have attached the wrong patch set.

Whoops, multitasking fail.

Sorry for the late response, hospitals are "fun".


-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 3d3cfd16dfc167f3cf4b8790e1625a2084dbce6b Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Fri, 19 Aug 2016 14:44:15 +0800
Subject: [PATCH 1/3] Introduce txid_status(bigint) to get status of an xact

If an appliation is disconnected while a COMMIT request is in flight,
the backend crashes mid-commit, etc, then an application may not be
sure whether or not a commit completed successfully or was rolled
back. While two-phase commit solves this it does so at a considerable
overhead, so introduce a lighter alternative.

txid_status(bigint) lets an application determine the status of a
a commit based on an xid-with-epoch as returned by txid_current()
or similar. Status may be committed, aborted, in-progress (including
prepared xacts) or null if the xact is too old for its commit status
to still be retained because it has passed the wrap-around epoch
boundary.

Applications must call txid_current() in their transactions to make
much use of this since PostgreSQL does not automatically report an xid
to the client when one is assigned.

Introduces TransactionIdInRecentPast(...) for the use of other functions
that need similar logic in future.
---
 doc/src/sgml/func.sgml |  31 +
 src/backend/access/transam/clog.c  |  23 ---
 src/backend/utils/adt/txid.c   | 130 +
 src/include/access/clog.h  |  23 +++
 src/include/catalog/pg_proc.h  |   2 +
 src/include/utils/builtins.h   |   1 +
 src/test/regress/expected/txid.out |  68 +++
 src/test/regress/sql/txid.sql  |  38 +++
 8 files changed, 293 insertions(+), 23 deletions(-)

diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 47fcb30..3123232 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17143,6 +17143,10 @@ SELECT collation for ('foo' COLLATE "de_DE");
 txid_visible_in_snapshot

 
+   
+txid_status
+   
+

 The functions shown in 
 provide server transaction information in an exportable form.  The main
@@ -17193,6 +17197,11 @@ SELECT collation for ('foo' COLLATE "de_DE");
boolean
is transaction ID visible in snapshot? (do not use with subtransaction ids)
   
+  
+   txid_status(bigint)
+   txid_status
+   report the status of the given xact - committed, aborted, in progress, or NULL if the xid is too old
+  
  
 

@@ -17263,6 +17272,28 @@ SELECT collation for ('foo' COLLATE "de_DE");

 

+txid_status(bigint) reports the commit status of a recent
+transaction.  Applications may use it to determine whether a transaction
+committed or aborted when the application and/or database server crashed or
+lost connection while a COMMIT command was in progress.
+The status of a transaction will be reported as one of:
+
+ 'in progress'
+ 'committed'
+ 'aborted'
+ NULL if xid too old
+
+PostgreSQL discards the commit status transactions after no references to
+the transaction survive in other active transactions, tables, replication
+slots, etc. This means that the status of older transactions cannot be
+determined.  txid_status(bigint) returns NULL if a
+transaction is too old to look up.  Prepared transactions are reported as
+in progress; applications must check pg_prepared_xacts if they
+need to determine if the xid is a prepared transaction.
+   
+
+   
 The functions shown in 
 provide information about transactions that have been already committed.
 These functions mainly provide information about when the transactions
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index 2634476..1a6e26d 100644
--- a/src/backend/access/transam/clog.c
+++ 

Re: [HACKERS] PATCH: Keep one postmaster monitoring pipe per process

[Andres Freund]

On 2016-09-20 11:07:03 +1200, Thomas Munro wrote:
> Yeah, I wondered why that was different than the pattern established
> elsewhere when I was hacking on replication code.  There are actually
> several places where we call PostmasterIsAlive() unconditionally in a
> loop that waits for WL_POSTMASTER_DEATH but ignores the return code:

Note that just changing this implies a behavioural change in at least
some of those: If the loop is busy with work, the WaitLatch might never
be reached.  I think that might be ok in most of those, but it does
require examination.

- 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] PATCH: Keep one postmaster monitoring pipe per process

[Thomas Munro]

On Sat, Sep 17, 2016 at 6:23 AM, Andres Freund  wrote:
> On 2016-09-16 09:55:48 +0200, Marco Pfatschbacher wrote:
>> On Thu, Sep 15, 2016 at 12:26:16PM -0700, Andres Freund wrote:
>> > Yikes, that's a pretty absurd implementation.
>>
>> Not when you take into account that it's been written over 20 years ago ;)
>
> Well, that doesn't mean it can't be fixed ;)
>
>> > I'm not quite sure I understand why this an issue here - there shouldn't
>> > ever be events on this fd, so why is the kernel waking up all processes?
>> > It'd kinda makes sense it'd wake up all processes if there's one
>> > waiting, but ... ?
>>
>> Every read is an event, and that's what PostmasterIsAlive does.
>
> But in most places we only do a PostmasterIsAlive if WaitLatch returns
> WL_POSTMASTER_DEATH.  The only walreceiver related place that doesn't is
> WalRcvWaitForStartPosition(). If that's indeed the cause of your issues
> this quite possibly could be fixed by doing the
> if (!PostmasterIsAlive())
> exit(1);
> check not unconditionally, but only after the WaitLatch at the end of
> the loop, and only if WL_POSTMASTER_DEATH is returned by WaitLatch()?
> That'll be a minor behaviour change for the WALRCV_RESTARTING, but that
> seems fine, we'll just loop once more outside (after a quick glance at
> least).

Yeah, I wondered why that was different than the pattern established
elsewhere when I was hacking on replication code.  There are actually
several places where we call PostmasterIsAlive() unconditionally in a
loop that waits for WL_POSTMASTER_DEATH but ignores the return code:
in pgarch.c, syncrep.c, walsender.c and walreceiver.c.  Should we just
change them all to check the return code and exit/break/ereport/etc as
appropriate?  That would match the code from autovacuum.c,
checkpointer.c, pgstat.c, be-secure.c and bgworker.c.  Something like
the attached.

The code in basebackup.c also waits for WL_POSTMASTER_DEATH but
doesn't check for it in the return value *or* call
PostmasterIsAlive().  I'm not sure what to make of that.  I didn't
test it but it looks like maybe it would continue running after
postmaster death but not honour the throttling rate limit because
WaitLatch would keep returning immediately.

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


check-return-code-for-postmaster-death.patch
Description: Binary data

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

Re: [HACKERS] [bug fix] pg_recvlogical is missing in the Windows installation

[MauMau]

From: Robert Haas
On Sat, Sep 17, 2016 at 7:44 PM, Michael Paquier
 wrote:
> On Sun, Sep 18, 2016 at 7:01 AM, MauMau  wrote:
>> pg_recvlogical is not included in the Windows client installation,
>> which is performed by running "install  client".  The
>> attached patch based on HEAD fixes this.  I confirmed nothing else
is
>> missing in the client installation.
>
> Good cacth. This has been missed for a couple of years.

OK, committed and back-patched to 9.4.  Not having a Windows build
environment, I did that blindly, so hopefully the BF won't blow up


Thank you for quick committing.

Regards
MauMau




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

Re: [HACKERS] pgbench - fix stats when using \sleep

[Heikki Linnakangas]

On 08/23/2016 05:47 PM, Fabien COELHO wrote:

When \sleep is used within a pgbench script it resets txn_scheduled which
is used for computing stats about the transaction, resulting in absurd
statistics:

  latency average = 0.649 ms *** ??? ***
  ...
  script statistics:
   - statement latencies in milliseconds:
   0.235  BEGIN;
 100.301  \sleep 100 ms
   0.351  END;

I probably created this bug when adding "--rate" in 9.4 and trying to be
too clever. As nobody complained yet about it, I'm not sure it is worth
fixing it there, though.

The fix is that "\sleep" does not have to interfere with the txn_scheduled
field, see the attached patch.

  latency average = 100.237 ms  *** BETTER ***
  ...
  script statistics:
   - statement latencies in milliseconds:
   0.099  BEGIN;
 100.001  \sleep 100 ms
   0.135  END;


Yep, it's clearly broken. Committed and backpatched down to 9.4. Thanks!

- Heikki



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

Re: [HACKERS] Declarative partitioning - another take

[Robert Haas]

On Mon, Aug 15, 2016 at 7:21 AM, Amit Langote
 wrote:
>> +if (partexprs)
>> +recordDependencyOnSingleRelExpr(,
>> +(Node *) partexprs,
>> +RelationGetRelid(rel),
>> +DEPENDENCY_NORMAL,
>> +DEPENDENCY_IGNORE);
>>
>> I don't think introducing a new DEPENDENCY_IGNORE type is a good idea
>> here.  Instead, you could just add an additional Boolean argument to
>> recordDependencyOnSingleRelExpr.  That seems less likely to create
>> bugs in unrelated portions of the code.
>
> I did consider a Boolean argument instead of a new DependencyType value,
> however it felt a bit strange to pass a valid value for the fifth argument
> (self_behavior) and then ask using a separate parameter that it (a
> self-dependency) is to be ignored.  By the way, no pg_depend entry is
> created on such a call, so the effect of the new type's usage seems
> localized to me. Thoughts?

I think that's not a very plausible argument.  If you add a fifth
argument to that function, then only that function needs to know about
the possibility of ignoring self-dependencies.  If you add a new
dependency type, then everything that knows about DependencyType needs
to know about them.  That's got to be a much larger surface area for
bugs.  Also, if you look around a bit, I believe you will find other
examples of cases where one argument is used only for certain values
of some other argument.  That's not a novel design pattern.

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

[Robert Haas]

On Fri, Sep 16, 2016 at 2:38 PM, Andres Freund  wrote:
>> I think that exploring it well requires good code.  If the code is good,
>> why not commit it?
>
> Because getting there requires a lot of effort, debugging it afterwards
> would take effort, and maintaining it would also takes a fair amount?
> Adding code isn't free.

Of course not, but nobody's saying you have to be the one to put in
any of that effort.  I was a bit afraid that nobody outside of
EnterpriseDB was going to take any interest in this patch, and I'm
really pretty pleased by the amount of interest that it's generated.
It's pretty clear that multiple smart people are working pretty hard
to break this, and Amit is fixing it, and at least for me that makes
me a lot less scared that the final result will be horribly broken.
It will probably have some bugs, but they probably won't be worse than
the status quo:

WARNING: hash indexes are not WAL-logged and their use is discouraged

Personally, I think it's outright embarrassing that we've had that
limitation for years; it boils down to "hey, we have this feature but
it doesn't work", which is a pretty crummy position for the world's
most advanced open-source database to take.

> I'm rather unenthused about having a hash index implementation that's
> mildly better in some corner cases, but otherwise doesn't have much
> benefit. That'll mean we'll have to step up our user education a lot,
> and we'll have to maintain something for little benefit.

If it turns out that it has little benefit, then we don't really need
to step up our user education.  People can just keep using btree like
they do now and that will be fine.  The only time we *really* need to
step up our user education is if it *does* have a benefit.  I think
that's a real possibility, because it's pretty clear to me - based in
part on off-list conversations with Amit - that the hash index code
has gotten very little love compared to btree, and there are lots of
optimizations that have been done for btree that have not been done
for hash indexes, but which could be done.  So I think there's a very
good chance that once we fix hash indexes to the point where they can
realistically be used, there will be further patches - either from
Amit or others - which improve performance even more.  Even the
preliminary results are not bad, though.

Also, Oracle offers hash indexes, and SQL Server offers them for
memory-optimized tables.  DB2 offers a "hash access path" which is not
described as an index but seems to work like one.  MySQL, like SQL
Server, offers them only for memory-optimized tables.  When all of the
other database products that we're competing against offer something,
it's not crazy to think that we should have it, too - and that it
should actually work, rather than being some kind of half-supported
wart.

By the way, I think that one thing which limits the performance
improvement we can get from hash indexes is the overall slowness of
the executor.  You can't save more by speeding something up than the
percentage of time you were spending on it in the first place.  IOW,
if you're spending all of your time in src/backend/executor then you
can't be spending it in src/backend/access, so making
src/backend/access faster doesn't help much.  However, as the executor
gets faster, which I hope it will, the potential gains from a faster
index go up.

-- 
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[David Fetter]

On Mon, Sep 19, 2016 at 03:00:51PM -0400, Peter Eisentraut wrote:
> On 9/19/16 12:02 AM, David Fetter wrote:
> >> - The claim in the documentation that only superusers can do things
> >> >   with this module is not generally correct.
> > I think that the claims are fixed.  This is SUSET, at least in this
> > patch, because anything short of that that changes query behavior
> > seems incautious.
> 
> Your last patch, which I looked at, had them as USERSET.  I think that
> is the right setting.

Will work one up this evening that has that.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778  AIM: dfetter666  Yahoo!: dfetter
Skype: davidfetter  XMPP: david(dot)fetter(at)gmail(dot)com

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate


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

Re: [HACKERS] Exclude schema during pg_restore

[Michael Banck]

Hi,

sorry, it took me a while to find time to look at this.

On Thu, Sep 01, 2016 at 09:39:56PM -0400, Peter Eisentraut wrote:
> On 8/31/16 4:10 AM, Michael Banck wrote:
> > attached is a small patch that adds an -N option to pg_restore, in order
> > to exclude a schema, in addition to -n for the restriction to a schema.
> 
> I think this is a good idea, and the approach looks sound.  However,
> something doesn't work right.  If I take an empty database and dump it,
> it will dump the plpgsql extension.  If I run pg_dump in plain-text mode
> with -N, then the plpgsql extension is also dumped (since it is not in
> the excluded schema).  But if I use the new pg_restore -N option, the
> plpgsql extension is not dumped.  Maybe this is because it doesn't have
> a schema, but I haven't checked.

I was afraid that this might need major code surgery, but in the end it
seems this was just a thinko on my part in tocEntryRequired(). For the
exclude-schema case, we shouldn't skip objects without a namespace (like
the plpgsql extension you mentioned above).
 
> pg_dump does not apply --strict-names to -N, but your patch for
> pg_restore does that.  I think that should be made the same as pg_dump.

Ok, I've removed that hunk.

Version 2 attached.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index c906919..e5eb18e 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -315,6 +315,17 @@
  
 
  
+  -N namespace
+  --exclude-schema=schema
+  
+   
+Do not restore objects that are in the named schema.  Multiple schemas
+to be excluded may be specified with multiple -N switches.
+   
+  
+ 
+
+ 
   -O
   --no-owner
   
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 4afa92f..0a28124 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -99,6 +99,7 @@ typedef struct _restoreOptions
 	SimpleStringList indexNames;
 	SimpleStringList functionNames;
 	SimpleStringList schemaNames;
+	SimpleStringList schemaExcludeNames;
 	SimpleStringList triggerNames;
 	SimpleStringList tableNames;
 
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 05bdbdb..0081d2f 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2751,6 +2751,9 @@ _tocEntryRequired(TocEntry *te, teSection curSection, RestoreOptions *ropt)
 			return 0;
 	}
 
+	if ((ropt->schemaExcludeNames.head != NULL) && te->namespace && simple_string_list_member(>schemaExcludeNames, te->namespace))
+		return 0;
+
 	if (ropt->selTypes)
 	{
 		if (strcmp(te->desc, "TABLE") == 0 ||
diff --git a/src/bin/pg_dump/pg_restore.c b/src/bin/pg_dump/pg_restore.c
index fb08e6b..3be8654 100644
--- a/src/bin/pg_dump/pg_restore.c
+++ b/src/bin/pg_dump/pg_restore.c
@@ -85,6 +85,7 @@ main(int argc, char **argv)
 		{"data-only", 0, NULL, 'a'},
 		{"dbname", 1, NULL, 'd'},
 		{"exit-on-error", 0, NULL, 'e'},
+		{"exclude-schema", 1, NULL, 'N'},
 		{"file", 1, NULL, 'f'},
 		{"format", 1, NULL, 'F'},
 		{"function", 1, NULL, 'P'},
@@ -148,7 +149,7 @@ main(int argc, char **argv)
 		}
 	}
 
-	while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:Op:P:RsS:t:T:U:vwWx1",
+	while ((c = getopt_long(argc, argv, "acCd:ef:F:h:I:j:lL:n:N:Op:P:RsS:t:T:U:vwWx1",
 			cmdopts, NULL)) != -1)
 	{
 		switch (c)
@@ -196,6 +197,10 @@ main(int argc, char **argv)
 simple_string_list_append(>schemaNames, optarg);
 break;
 
+			case 'N':			/* Do not dump data for this schema */
+simple_string_list_append(>schemaExcludeNames, optarg);
+break;
+
 			case 'O':
 opts->noOwner = 1;
 break;

-- 
Sent 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 PL/Python slow input arrays traversal issue

[Dave Cramer]

Yes, this should be closed as it is contained in 
https://commitfest.postgresql.org/10/697/
-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[Andres Freund]

On 2016-09-19 15:10:58 -0400, Robert Haas wrote:
> Personally, I find the results so far posted on this thread thoroughly
> unimpressive.  I acknowledge that Dilip's results appear to show that
> in a best-case scenario these patches produce a rather large gain.
> However, that gain seems to happen in a completely contrived scenario:
> astronomical client counts, unlogged tables, and a test script that
> maximizes pressure on CLogControlLock.  If you have to work that hard
> to find a big win, and tests under more reasonable conditions show no
> benefit, it's not clear to me that it's really worth the time we're
> all spending benchmarking and reviewing this, or the risk of bugs, or
> the damage to the SLRU abstraction layer.  I think there's a very good
> chance that we're better off moving on to projects that have a better
> chance of helping in the real world.

+1


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

Re: [HACKERS] Speed up Clog Access by increasing CLOG buffers

[Robert Haas]

On Sun, Sep 18, 2016 at 5:11 PM, Tomas Vondra
 wrote:
> IMHO in the ideal case the first message in this thread would provide a test
> case, demonstrating the effect of the patch. Then we wouldn't have the issue
> of looking for a good workload two years later.
>
> But now that I look at the first post, I see it apparently used a plain
> tpc-b pgbench (with synchronous_commit=on) to show the benefits, which is
> the workload I'm running right now (results sometime tomorrow).
>
> That workload clearly uses no savepoints at all, so I'm wondering why you
> suggested to use several of them - I know you said that it's to show
> differences between the approaches, but why should that matter to any of the
> patches (and if it matters, why I got almost no differences in the
> benchmarks)?
>
> Pardon my ignorance, CLOG is not my area of expertise ...

It's possible that the effect of this patch depends on the number of
sockets.  EDB test machine cthulhu as 8 sockets, and power2 has 4
sockets.  I assume Dilip's tests were run on one of those two,
although he doesn't seem to have mentioned which one.  Your system is
probably 2 or 4 sockets, which might make a difference.  Results might
also depend on CPU architecture; power2 is, unsurprisingly, a POWER
system, whereas I assume you are testing x86.  Maybe somebody who has
access should test on hydra.pg.osuosl.org, which is a community POWER
resource.  (Send me a private email if you are a known community
member who wants access for benchmarking purposes.)

Personally, I find the results so far posted on this thread thoroughly
unimpressive.  I acknowledge that Dilip's results appear to show that
in a best-case scenario these patches produce a rather large gain.
However, that gain seems to happen in a completely contrived scenario:
astronomical client counts, unlogged tables, and a test script that
maximizes pressure on CLogControlLock.  If you have to work that hard
to find a big win, and tests under more reasonable conditions show no
benefit, it's not clear to me that it's really worth the time we're
all spending benchmarking and reviewing this, or the risk of bugs, or
the damage to the SLRU abstraction layer.  I think there's a very good
chance that we're better off moving on to projects that have a better
chance of helping in the real world.

-- 
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Peter Eisentraut]

On 9/19/16 12:02 AM, David Fetter wrote:
>> - The claim in the documentation that only superusers can do things
>> >   with this module is not generally correct.
> I think that the claims are fixed.  This is SUSET, at least in this
> patch, because anything short of that that changes query behavior
> seems incautious.

Your last patch, which I looked at, had them as USERSET.  I think that
is the right setting.

-- 
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] PoC: Make it possible to disallow WHERE-less UPDATE and DELETE

[Robert Haas]

On Mon, Sep 19, 2016 at 12:02 AM, David Fetter  wrote:
>> - The claim in the documentation that only superusers can do things
>>   with this module is not generally correct.
>
> I think that the claims are fixed.  This is SUSET, at least in this
> patch, because anything short of that that changes query behavior
> seems incautious.

Uggh, I disagree strongly with that, as do lots of existing GUCs.  I
think it's for the superuser to decide whether this should be enabled
by default (e.g. by setting it in postgresql.conf) and for individual
users to decide whether they want to override the superuser's decision
for particular sessions.  Therefore, I think this should be
PGC_USERSET.

I think PGC_SUSET GUCs are pretty annoying, and we should have a
really compelling reason why it's not OK for users to change the value
of a setting before resorting to PGC_SUSET.  For example, log_duration
is PGC_SUSET and that makes sense because the log is "owned" by the
administrator, not the individual user.  But work_mem, for example,
changes query behavior and that is PGC_USERSET.  I think that's right.
We have talked before about wanting a system that restricts the values
to which users can legally set values which they are in principle
allowed to change, and someday we might have that.  In the meantime,
letting regular users change settings that they don't like is, in
general, a feature, not a bug.

Someone who feels otherwise can, of course, hack up their own version
of this module.

-- 
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] [bug fix] pg_recvlogical is missing in the Windows installation

[Robert Haas]

On Sat, Sep 17, 2016 at 7:44 PM, Michael Paquier
 wrote:
> On Sun, Sep 18, 2016 at 7:01 AM, MauMau  wrote:
>> pg_recvlogical is not included in the Windows client installation,
>> which is performed by running "install  client".  The
>> attached patch based on HEAD fixes this.  I confirmed nothing else is
>> missing in the client installation.
>
> Good cacth. This has been missed for a couple of years.

OK, committed and back-patched to 9.4.  Not having a Windows build
environment, I did that blindly, so hopefully the BF won't blow up

-- 
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 Sat, Sep 17, 2016 at 2:40 AM, Amit Kapila  wrote:
> On Fri, Sep 16, 2016 at 11:54 PM, Robert Haas  wrote:
>>
>> +   /*
>> +* We need a write barrier to make sure the update of
>> +* parallel_terminate_count is done before the store to in_use
>> +*/
>>
>> Does the order actually matter here?
>>
>
> I think so.  If slot->in_use is reordered before the check of
> is_parallel_worker, then it is possible that concurrent registration
> of worker can mark the is_parallel_worker as false before we check the
> flag here.  See explanation in previous e-mail [1].

Tricky.  I believe you're right.

-- 
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 Fri, Sep 16, 2016 at 3:53 PM, Julien Rouhaud
 wrote:
> That's fine by me.  Should this be done (if there's no objection) in the
> same patch, or in another one?

I'd say "same patch".

>> I'd suggest renaming the "parallel" flag to BackgroundWorkerSlot to
>> "is_parallel_worker".  Or, actually, what I think would be better is
>> to give it a name like worker_class, and then we can have
>> BGWORKER_CLASS_PARALLEL and perhaps eventually
>> BGWORKER_CLASS_REPLICATION, etc.
>
> For now I just renamed "parallel" to "is_parallel_worker", since this is
> straightforward.  For a new "worker_class", I guess we'd need a new enum
> stored in BackgroundWorker struct instead of the
> BGWORKER_IS_PARALLEL_WORKER flag, and store it in the
> BackgroundWorkerSlot. Should I do that instead?

I suggest that we make it part of bgw_flags, but use a bitmask for it,
like this:

#define BGWORKER_CLASS_MASK   0x00f0
#define BGWORKER_CLASS_PARALLEL  0x0010
/* add additional bgworker classes here */

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


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

Re: [HACKERS] Parallel sec scan in plpgsql

[Robert Haas]

On Sat, Sep 17, 2016 at 11:54 PM, Amit Kapila  wrote:
> In general, I think we should support the cases as required (or
> written) by you from plpgsql or sql functions.  We need more work to
> support such cases. There are probably two ways of supporting such
> cases, we can build some intelligence in plpgsql execution such that
> it can recognise such queries and allow to use parallelism or we need
> to think of enabling parallelism for cases where we don't run the plan
> to completion.  Most of the use cases from plpgsql or sql function
> fall into later category as they don't generally run the plan to
> completion.

I think there's certainly more work that could be done to teach
PL/pgsql about cases where the query will run to completion.  I didn't
work very hard to make sure we covered all of those; there are
probably several different cases where parallelism could be safely
enabled but currently isn't.  Also, I didn't do anything at all to
update the other PLs, and that would be good, too.

However, I think the chances of supporting parallel query for queries
not executed to completion any time in the near future are very poor.
Once the query is suspended, the user can do anything they like,
including stuff that's parallel-unsafe, and then we have no choice but
to error out, and that's not what we want to happen.  If we had
absolutely no parallel-unsafe operations - which would be quite a feat
- then we might be able to get there.  But even then you have the
problem that while the query is suspended, you are still nailing down
workers that are sitting around idle, waiting for the leader to resume
the query, and that's not very good either.

-- 
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] recovery_min_apply-delay and remote_apply

[Robert Haas]

On Fri, Sep 16, 2016 at 5:55 PM, Thomas Munro
 wrote:
> On Sat, Sep 17, 2016 at 8:45 AM, Bernd Helmle  wrote:
>> Current PostgreSQL Documentation on recovery.conf has this about
>> recovery_min_apply_delay[1]:
>>
>> ---<---
>>
>> This parameter is intended for use with streaming replication deployments;
>> however, if the parameter is specified it will be honored in all cases.
>> Synchronous replication is not affected by this setting because there is
>> not yet any setting to request synchronous apply of transaction commits.
>>
>> --->---
>>
>> If i understand correctly, this is not true anymore with 9.6, where
>> remote_apply will have exactly the behavior the paragraph above wants to
>> contradict: any transaction executed with synchronous_commit=remote_apply
>> will wait at least recovery_min_apply_delay to finish. Given that
>> synchronous_commit can be controlled by any user, this might be dangerous
>> if someone doesn't take care enough.
>
> Yes, I missed that sentence.  Thanks.
>
>> I think we need a doc patch for that at least, see attached patch against
>> master, but 9.6 should have a corrected one, too.
>
> +1

Committed with a bit of adjustment, and back-patched to 9.6.

-- 
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] more parallel query documentation

[Robert Haas]

On Fri, Sep 16, 2016 at 4:28 PM, Alvaro Herrera
 wrote:
> I agree it should be added.  I suggest that it could even be added to
> the 9.6 docs, if you can make it.

Here's a patch.  I intend to commit this pretty quickly unless
somebody objects, and also to backpatch it into 9.6.  I'm sure it's
not perfect, but imperfect documentation is better than no
documentation.

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


parallel-query-doc-v1.patch
Description: invalid/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] Hash Indexes

[Jeff Janes]

On Sun, Sep 18, 2016 at 11:44 PM, Amit Kapila 
wrote:

> On Mon, Sep 19, 2016 at 11:20 AM, Mark Kirkwood
>  wrote:
> > On 17/09/16 06:38, Andres Freund wrote:
> >
> > While I see the point of what you are saying here, I recall all previous
> > discussions about has indexes tended to go a bit like this:
> >
> > - until WAL logging of hash indexes is written it is not worthwhile
> trying
> > to make improvements to them
> > - WAL logging will be a lot of work, patches 1st please
> >
> > Now someone has done that work, and we seem to be objecting that because
> > they are not improved then the patches are (maybe) not worthwhile.
> >
>

+1


>
> I think saying hash indexes are not improved after proposed set of
> patches is an understatement.  The read performance has improved by
> more than 80% as compare to HEAD [1] (refer data in Mithun's mail).
> Also, tests by Mithun and Jesper has indicated that in multiple
> workloads, they are better than BTREE by 30~60% (in fact Jesper
> mentioned that he is seeing 40~60% benefit on production database,
> Jesper correct me if I am wrong.).  I agree that when index column is
> updated they are much worse than btree as of now,


Has anyone tested that with the relcache patch applied?  I would expect
that to improve things by a lot (compared to hash-HEAD, not necessarily
compared to btree-HEAD), but if I am following the emails correctly, that
has not been done.


> but no work has been
> done improve it and I am sure that it can be improved for those cases
> as well.
>
> In general, I thought the tests done till now are sufficient to prove
> the importance of work, but if still Andres and others have doubt and
> they want to test some specific cases, then sure we can do more
> performance benchmarking.
>

I think that a precursor to WAL is enough to justify it even if the
verified performance improvements were not impressive.  But they are pretty
impressive, at least for some situations.

Cheers,

Jeff

Re: [HACKERS] pageinspect: Hash index support

[Jesper Pedersen]

On 09/14/2016 04:21 PM, Jeff Janes wrote:

I suggest that pageinspect functions are more convenient to use via the
get_raw_page interface, that is, instead of reading the buffer
themselves, the buffer is handed over from elsewhere and they receive it
as bytea.  This enables other use cases such as grabbing a page from one
server and examining it in another one.



I've never needed to do that, but it does sound like it might be useful.
But it is also annoying to have to do that when you want to examine a
current server.  Could we use overloading, so that it can be used in either
way?

For hash_page_items, the 'data' is always a 32 bit integer, isn't it?
(unlike other pageinspect features, where the data could be any
user-defined data type).  If so, I'd rather see it in plain hex (without
the spaces, without the trailing zeros)

+   /* page type (flags) */
+   if (opaque->hasho_flag & LH_UNUSED_PAGE)
+   stat->type = 'u';

This can never be true, because LH_UNUSED_PAGE is zero.  You should make
this be the fall-through case, and have LH_META_PAGE be explicitly tested
for.



This version adds the overloaded get_raw_page based methods. However, 
I'm not verifying the page other than page_id, as hasho_flag can contain 
multiple flags in Amit's patches. And, I don't see having different page 
ids as a benefit - at least not part of this patch.


We should probably just have one of the method sets, so I'll send a v3 
with the set voted for.


I kept the 'data' field as is, for now.

Best regards,
 Jesper

>From 0dc44e4b3cc1d31c53684a41fbd7959978e69089 Mon Sep 17 00:00:00 2001
From: jesperpedersen 
Date: Fri, 5 Aug 2016 10:16:32 -0400
Subject: [PATCH] pageinspect: Hash index support

---
 contrib/pageinspect/Makefile  |  10 +-
 contrib/pageinspect/hashfuncs.c   | 766 ++
 contrib/pageinspect/pageinspect--1.5--1.6.sql | 111 
 contrib/pageinspect/pageinspect--1.5.sql  | 279 --
 contrib/pageinspect/pageinspect--1.6.sql  | 386 +
 contrib/pageinspect/pageinspect.control   |   2 +-
 doc/src/sgml/pageinspect.sgml | 103 
 7 files changed, 1372 insertions(+), 285 deletions(-)
 create mode 100644 contrib/pageinspect/hashfuncs.c
 create mode 100644 contrib/pageinspect/pageinspect--1.5--1.6.sql
 delete mode 100644 contrib/pageinspect/pageinspect--1.5.sql
 create mode 100644 contrib/pageinspect/pageinspect--1.6.sql

diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index a98237e..c73d728 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -2,13 +2,13 @@
 
 MODULE_big	= pageinspect
 OBJS		= rawpage.o heapfuncs.o btreefuncs.o fsmfuncs.o \
-		  brinfuncs.o ginfuncs.o $(WIN32RES)
+		  brinfuncs.o ginfuncs.o hashfuncs.o $(WIN32RES)
 
 EXTENSION = pageinspect
-DATA = pageinspect--1.5.sql pageinspect--1.4--1.5.sql \
-	pageinspect--1.3--1.4.sql pageinspect--1.2--1.3.sql \
-	pageinspect--1.1--1.2.sql pageinspect--1.0--1.1.sql \
-	pageinspect--unpackaged--1.0.sql
+DATA = pageinspect--1.6.sql pageinspect--1.5--1.6.sql \
+	pageinspect--1.4--1.5.sql pageinspect--1.3--1.4.sql \
+	pageinspect--1.2--1.3.sql pageinspect--1.1--1.2.sql \
+	pageinspect--1.0--1.1.sql pageinspect--unpackaged--1.0.sql
 PGFILEDESC = "pageinspect - functions to inspect contents of database pages"
 
 ifdef USE_PGXS
diff --git a/contrib/pageinspect/hashfuncs.c b/contrib/pageinspect/hashfuncs.c
new file mode 100644
index 000..380203c
--- /dev/null
+++ b/contrib/pageinspect/hashfuncs.c
@@ -0,0 +1,766 @@
+/*
+ * hashfuncs.c
+ *		Functions to investigate the content of HASH indexes
+ *
+ * Copyright (c) 2016, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		contrib/pageinspect/hashfuncs.c
+ */
+
+#include "postgres.h"
+
+#include "access/hash.h"
+#include "access/htup_details.h"
+#include "catalog/namespace.h"
+#include "catalog/pg_am.h"
+#include "funcapi.h"
+#include "miscadmin.h"
+#include "utils/builtins.h"
+#include "utils/rel.h"
+
+
+PG_FUNCTION_INFO_V1(hash_metap_bytea);
+PG_FUNCTION_INFO_V1(hash_metap);
+PG_FUNCTION_INFO_V1(hash_page_items_bytea);
+PG_FUNCTION_INFO_V1(hash_page_items);
+PG_FUNCTION_INFO_V1(hash_page_stats_bytea);
+PG_FUNCTION_INFO_V1(hash_page_stats);
+
+#define IS_INDEX(r) ((r)->rd_rel->relkind == RELKIND_INDEX)
+#define IS_HASH(r) ((r)->rd_rel->relam == HASH_AM_OID)
+
+/* note: BlockNumber is unsigned, hence can't be negative */
+#define CHECK_RELATION_BLOCK_RANGE(rel, blkno) { \
+		if ( RelationGetNumberOfBlocks(rel) <= (BlockNumber) (blkno) ) \
+			 elog(ERROR, "block number out of range"); }
+
+/* 
+ * structure for single hash page statistics
+ * 
+ */
+typedef struct HashPageStat
+{
+	uint32		blkno;
+	uint32		live_items;
+	uint32		dead_items;
+	uint32		page_size;
+	uint32		max_avail;
+	uint32		free_size;
+	uint32		avg_item_size;
+	char		

Re: [HACKERS] Hash Indexes

[Kenneth Marshall]

On Mon, Sep 19, 2016 at 12:14:26PM +0530, Amit Kapila wrote:
> On Mon, Sep 19, 2016 at 11:20 AM, Mark Kirkwood
>  wrote:
> >
> >
> > On 17/09/16 06:38, Andres Freund wrote:
> >>
> >> On 2016-09-16 09:12:22 -0700, Jeff Janes wrote:
> >>>
> >>> On Thu, Sep 15, 2016 at 7:23 AM, Andres Freund 
> >>> wrote:
> 
>  One earlier question about this is whether that is actually a worthwhile
>  goal.  Are the speed and space benefits big enough in the general case?
>  Could those benefits not be achieved in a more maintainable manner by
>  adding a layer that uses a btree over hash(columns), and adds
>  appropriate rechecks after heap scans?
> 
>  Note that I'm not saying that hash indexes are not worthwhile, I'm just
>  doubtful that question has been explored sufficiently.
> >>>
> >>> I think that exploring it well requires good code.  If the code is good,
> >>> why not commit it?
> >>
> >> Because getting there requires a lot of effort, debugging it afterwards
> >> would take effort, and maintaining it would also takes a fair amount?
> >> Adding code isn't free.
> >>
> >> I'm rather unenthused about having a hash index implementation that's
> >> mildly better in some corner cases, but otherwise doesn't have much
> >> benefit. That'll mean we'll have to step up our user education a lot,
> >> and we'll have to maintain something for little benefit.
> >>
> >
> > While I see the point of what you are saying here, I recall all previous
> > discussions about has indexes tended to go a bit like this:
> >
> > - until WAL logging of hash indexes is written it is not worthwhile trying
> > to make improvements to them
> > - WAL logging will be a lot of work, patches 1st please
> >
> > Now someone has done that work, and we seem to be objecting that because
> > they are not improved then the patches are (maybe) not worthwhile.
> >
> 
> I think saying hash indexes are not improved after proposed set of
> patches is an understatement.  The read performance has improved by
> more than 80% as compare to HEAD [1] (refer data in Mithun's mail).
> Also, tests by Mithun and Jesper has indicated that in multiple
> workloads, they are better than BTREE by 30~60% (in fact Jesper
> mentioned that he is seeing 40~60% benefit on production database,
> Jesper correct me if I am wrong.).  I agree that when index column is
> updated they are much worse than btree as of now, but no work has been
> done improve it and I am sure that it can be improved for those cases
> as well.
> 
> In general, I thought the tests done till now are sufficient to prove
> the importance of work, but if still Andres and others have doubt and
> they want to test some specific cases, then sure we can do more
> performance benchmarking.
> 
> Mark, thanks for supporting the case for improving Hash Indexes.
> 
> 
> [1] - 
> https://www.postgresql.org/message-id/CAD__OugX0aOa7qopz3d-nbBAoVmvSmdFJOX4mv5tFRpijqH47A%40mail.gmail.com
> -- 
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
> 

+1 Throughout the years, I have seen benchmarks that demonstrated the
performance advantages of even the initial hash index (without WAL)
over the btree of a hash variant. It is pretty hard to dismiss the
O(1) versus O(log(n)) difference. There are classes of problems for
which a hash index is the best solution. Lack of WAL has hamstrung
development in those areas for years.

Regards,
Ken


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

Re: [HACKERS] "Some tests to cover hash_index"

[Mithun Cy]

On Sat, Aug 6, 2016 at 9:41 AM, Amit Kapila  wrote:
> I wonder why you have included a new file for these tests, why can't be
these added to existing hash_index.sql.
tests in hash_index.sql did not cover overflow pages, above tests were for
mainly for them. So I thought having a separate test file can help
enabling/disabling them in schedule files, when we do not want them running
as it take slightly high time. If you think otherwise I will reconsider
will add tests to hash_index.sql.

>The relation name con_hash_index* choosen in above tests doesn't seem to
be appropriate, how about hash_split_heap* or something like that.
Fixed. Have renamed relation, index and test filename accordingly.

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


commit-hash_coverage_test_v2_no_wal.patch
Description: Binary data

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

Re: [HACKERS] PATCH: Keep one postmaster monitoring pipe per process

[Robert Haas]

On Fri, Sep 16, 2016 at 2:23 PM, Andres Freund  wrote:
>> Every read is an event, and that's what PostmasterIsAlive does.
>
> But in most places we only do a PostmasterIsAlive if WaitLatch returns
> WL_POSTMASTER_DEATH.  The only walreceiver related place that doesn't is
> WalRcvWaitForStartPosition(). If that's indeed the cause of your issues
> this quite possibly could be fixed by doing the
> if (!PostmasterIsAlive())
> exit(1);
> check not unconditionally, but only after the WaitLatch at the end of
> the loop, and only if WL_POSTMASTER_DEATH is returned by WaitLatch()?
> That'll be a minor behaviour change for the WALRCV_RESTARTING, but that
> seems fine, we'll just loop once more outside (after a quick glance at
> least).

At least some of the latch implementations already check
PostmasterIsAlive() internally to avoid returning spurious events; and
secure_read() at least assumes that the WL_POSTMASTER_DEATH return is
reliable and doesn't need a double-check.

So we can probably just remove the check altogether and instead bail
out if it returns WL_POSTMASTER_DEATH.  That probably saves a system
call per loop iteration even on platforms where the kernel doesn't
exhibit any surprising behavior.

--
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: Change the way that LWLocks for extensions are allocated.

[Amit Kapila]

On Sat, Sep 10, 2016 at 5:02 PM, Amit Kapila  wrote:
> On Tue, Aug 30, 2016 at 8:01 AM, Andres Freund  wrote:
>> On 2016-08-30 07:57:19 +0530, Amit Kapila wrote:
>>> I will write such a test case either in this week or early next week.
>>
>> Great.
>>
>
> Okay, attached patch adds some simple tests for pg_stat_statements.
> One thing to note is that we can't add pg_stat_statements tests for
> installcheck as this module requires shared_preload_libraries to be
> set.  Hope this suffices the need.
>

Registered this patch for next CF.

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


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

Re: [HACKERS] Improvements in psql hooks for variables

[Daniel Verite]

Rahila Syed wrote:


> I am beginning to review this patch. Initial comment. I got following
> compilation error when I applied the patch on latest sources and run make.

Sorry about that, I forgot to make clean, here's an updated patch.

Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index a9a2fdb..61b2304 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -254,7 +254,7 @@ exec_command(const char *cmd,
if (opt1 != NULL && strncmp(opt1, prefix, sizeof(prefix) - 1) 
== 0)
{
reuse_previous =
-   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix) ?
+   ParseVariableBool(opt1 + sizeof(prefix) - 1, 
prefix, NULL) ?
TRI_YES : TRI_NO;
 
free(opt1);
@@ -1548,7 +1548,7 @@ exec_command(const char *cmd,

 OT_NORMAL, NULL, false);
 
if (opt)
-   pset.timing = ParseVariableBool(opt, "\\timing");
+   pset.timing = ParseVariableBool(opt, "\\timing", NULL);
else
pset.timing = !pset.timing;
if (!pset.quiet)
@@ -2660,7 +2660,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
if (value && pg_strcasecmp(value, "auto") == 0)
popt->topt.expanded = 2;
else if (value)
-   popt->topt.expanded = ParseVariableBool(value, param);
+   popt->topt.expanded = ParseVariableBool(value, param, 
NULL);
else
popt->topt.expanded = !popt->topt.expanded;
}
@@ -2669,7 +2669,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "numericlocale") == 0)
{
if (value)
-   popt->topt.numericLocale = ParseVariableBool(value, 
param);
+   popt->topt.numericLocale = ParseVariableBool(value, 
param, NULL);
else
popt->topt.numericLocale = !popt->topt.numericLocale;
}
@@ -2724,7 +2724,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "t") == 0 || strcmp(param, "tuples_only") == 0)
{
if (value)
-   popt->topt.tuples_only = ParseVariableBool(value, 
param);
+   popt->topt.tuples_only = ParseVariableBool(value, 
param, NULL);
else
popt->topt.tuples_only = !popt->topt.tuples_only;
}
@@ -2756,7 +2756,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
popt->topt.pager = 2;
else if (value)
{
-   if (ParseVariableBool(value, param))
+   if (ParseVariableBool(value, param, NULL))
popt->topt.pager = 1;
else
popt->topt.pager = 0;
@@ -2778,7 +2778,7 @@ do_pset(const char *param, const char *value, 
printQueryOpt *popt, bool quiet)
else if (strcmp(param, "footer") == 0)
{
if (value)
-   popt->topt.default_footer = ParseVariableBool(value, 
param);
+   popt->topt.default_footer = ParseVariableBool(value, 
param, NULL);
else
popt->topt.default_footer = !popt->topt.default_footer;
}
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
index 7ce05fb..9dcfc0a 100644
--- a/src/bin/psql/startup.c
+++ b/src/bin/psql/startup.c
@@ -786,43 +786,59 @@ showVersion(void)
  * This isn't an amazingly good place for them, but neither is anywhere else.
  */
 
-static void
+/*
+ * Hook to set an internal flag from a user-supplied string value.
+ * If the syntax is correct, affect *flag and return true.
+ * Otherwise, keep *flag untouched and return false.
+ */
+static bool
+generic_boolean_hook(const char *newval, const char* varname, bool *flag)
+{
+   bool isvalid;
+   bool val = ParseVariableBool(newval, varname, );
+   if (isvalid)
+   *flag = val;
+   return isvalid;
+}
+
+static bool
 autocommit_hook(const char *newval)
 {
-   pset.autocommit = ParseVariableBool(newval, "AUTOCOMMIT");
+   return generic_boolean_hook(newval, "AUTOCOMMIT", );
 }
 
-static void
+static bool
 on_error_stop_hook(const char *newval)
 {
-   pset.on_error_stop = ParseVariableBool(newval, "ON_ERROR_STOP");
+   return generic_boolean_hook(newval, "ON_ERROR_STOP", 
_error_stop);
 }
 
-static 

Re: [HACKERS] Possibly too stringent Assert() in b-tree code

[Robert Haas]

On Mon, Sep 19, 2016 at 7:07 AM, Amit Kapila  wrote:
>> Of course, the database could have been corrupted after having encountered
>> many crashes during my experiments. Neverthelesss, even without in-depth
>> knowledge of the b-tree code I suspect that this stack trace might reflect a
>> legal situation. In partcular, if _bt_page_recyclable() returned on this
>> condition:
>>
>> if (PageIsNew(page))
>> return true;
>>
>
> I think you have a valid point.  It seems we don't need to write WAL
> for reuse page (aka don't call _bt_log_reuse_page()), if the page is
> new, as the only purpose of that log is to handle conflict based on
> transaction id stored in special area which will be anyway zero.

+1.

-- 
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] Possibly too stringent Assert() in b-tree code

[Amit Kapila]

On Mon, Sep 19, 2016 at 1:32 PM, Antonin Houska  wrote:
> I've recently seen this when using 9.6:
>
> #0  0x7f147892f0c7 in raise () from /lib64/libc.so.6
> #1  0x7f1478930478 in abort () from /lib64/libc.so.6
> #2  0x009683a1 in ExceptionalCondition (conditionName=0x9f2ef8 
> "!(((PageHeader) (page))->pd_special >= (__builtin_offsetof (PageHeaderData, 
> pd_linp)))", errorType=0x9f2e8a "FailedAssertion",
> fileName=0x9f2e60 "../../../../src/include/storage/bufpage.h", 
> lineNumber=314) at assert.c:54
> #3  0x004ee3d2 in PageValidateSpecialPointer (page=0x7f14700f3480 "") 
> at ../../../../src/include/storage/bufpage.h:314
> #4  0x004ef913 in _bt_getbuf (rel=0x7f14795a1a50, blkno=11, access=2) 
> at nbtpage.c:629
> #5  0x004eae8c in _bt_split (rel=0x7f14795a1a50, buf=136, cbuf=0, 
> firstright=367, newitemoff=408, newitemsz=16, newitem=0x2608330, 
> newitemonleft=0 '\000') at nbtinsert.c:986
> #6  0x004ea563 in _bt_insertonpg (rel=0x7f14795a1a50, buf=136, 
> cbuf=0, stack=0x26090c8, itup=0x2608330, newitemoff=408, split_only_page=0 
> '\000') at nbtinsert.c:781
> #7  0x004e954c in _bt_doinsert (rel=0x7f14795a1a50, itup=0x2608330, 
> checkUnique=UNIQUE_CHECK_YES, heapRel=0x25aa0a0) at nbtinsert.c:204
> #8  0x004f3b73 in btinsert (rel=0x7f14795a1a50, 
> values=0x7ffe46c6f7f0, isnull=0x7ffe46c6f7d0 "", ht_ctid=0x260927c, 
> heapRel=0x25aa0a0, checkUnique=UNIQUE_CHECK_YES) at nbtree.c:279
> #9  0x004e7964 in index_insert (indexRelation=0x7f14795a1a50, 
> values=0x7ffe46c6f7f0, isnull=0x7ffe46c6f7d0 "", heap_t_ctid=0x260927c, 
> heapRelation=0x25aa0a0, checkUnique=UNIQUE_CHECK_YES)
> at indexam.c:204
>
> Of course, the database could have been corrupted after having encountered
> many crashes during my experiments. Neverthelesss, even without in-depth
> knowledge of the b-tree code I suspect that this stack trace might reflect a
> legal situation. In partcular, if _bt_page_recyclable() returned on this
> condition:
>
> if (PageIsNew(page))
> return true;
>

I think you have a valid point.  It seems we don't need to write WAL
for reuse page (aka don't call _bt_log_reuse_page()), if the page is
new, as the only purpose of that log is to handle conflict based on
transaction id stored in special area which will be anyway zero.


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


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

Re: [HACKERS] IF (NOT) EXISTS in psql-completion

[Pavel Stehule]

Hi



> Pavel
>
>
>> 2. Make keywords' case follow to input
>>
>>   Allow the keywords suggested along with databse objects to
>>   follow the input letter case. The core part of this patch is a
>>   new function additional_kw_query(), which dynamically generates
>>   additional query string with specified keywords in the desired
>>   letter case. COMPLETE_WITH_* macros are modified to accept the
>>   function.
>>
>>
second patch is working, but I don't think it is enough documented

what is addon in COMPLETE_WITH_QUERY(query, addon)? semantics, usage?

in 99% the addon is "" when macro
COMPLETE_WITH_SCHEMA_QUERY,COMPLETE_WITH_QUERY is used. Maybe a
introduction of new macros with nonempty addon parameter should be better.





> 3. Fix suggested keywords to follow input in tab-completion session 2
>>
>>   The 2nd patch above leaves some query string containing static
>>   keyword strings, which results in failure to follow input
>>   letter cases. Most of them are naturally removed but role names
>>   are a bother. This patch puts additional query strings for
>>   several usage of roles but it might be overdone.
>>
>
this patch looks well


>
>> 4. Introduce word shift and removal feature to psql-completion
>>
>>   This is the second core for the flexibility of completion code.
>>   The word shift feature is the ability to omit first several
>>   words in *MatchesN macros. For example this allows complete
>>   create-schema's schema elements in a natural code. (Currently
>>   those syntaxes that can be a schema elements are using
>>   TailMatches instead of Matches, as the result HeadMatches are
>>   not available there). The words removing feature is the ability
>>   to (desructively) clip multiple suceessive words in the
>>   previous_words list. This feature allows suceeding completion
>>   code not to care about the removed words, such like UNIQUE,
>>   CONCURRENTLY, VERBOSE and so on.
>>
>
I am thinking so commit's description should be inside README

Regards

Pavel


>
>> 5. Add suggestion for IF (NOT) EXISTS for some syntaxes
>>
>>   This adds IF (NOT) EXISTS suggestion, as a PoC.  This patch no
>>   loger covers all adoptable syntaces since the places where more
>>   than boilerplating is required are omitted.
>>
>> regards,
>>
>> --
>> Kyotaro Horiguchi
>> NTT Open Source Software Center
>>
>>
>>
>>
>>
>

[HACKERS] Possibly too stringent Assert() in b-tree code

[Antonin Houska]

I've recently seen this when using 9.6:

#0  0x7f147892f0c7 in raise () from /lib64/libc.so.6
#1  0x7f1478930478 in abort () from /lib64/libc.so.6
#2  0x009683a1 in ExceptionalCondition (conditionName=0x9f2ef8 
"!(((PageHeader) (page))->pd_special >= (__builtin_offsetof (PageHeaderData, 
pd_linp)))", errorType=0x9f2e8a "FailedAssertion",
fileName=0x9f2e60 "../../../../src/include/storage/bufpage.h", 
lineNumber=314) at assert.c:54
#3  0x004ee3d2 in PageValidateSpecialPointer (page=0x7f14700f3480 "") 
at ../../../../src/include/storage/bufpage.h:314
#4  0x004ef913 in _bt_getbuf (rel=0x7f14795a1a50, blkno=11, access=2) 
at nbtpage.c:629
#5  0x004eae8c in _bt_split (rel=0x7f14795a1a50, buf=136, cbuf=0, 
firstright=367, newitemoff=408, newitemsz=16, newitem=0x2608330, 
newitemonleft=0 '\000') at nbtinsert.c:986
#6  0x004ea563 in _bt_insertonpg (rel=0x7f14795a1a50, buf=136, cbuf=0, 
stack=0x26090c8, itup=0x2608330, newitemoff=408, split_only_page=0 '\000') at 
nbtinsert.c:781
#7  0x004e954c in _bt_doinsert (rel=0x7f14795a1a50, itup=0x2608330, 
checkUnique=UNIQUE_CHECK_YES, heapRel=0x25aa0a0) at nbtinsert.c:204
#8  0x004f3b73 in btinsert (rel=0x7f14795a1a50, values=0x7ffe46c6f7f0, 
isnull=0x7ffe46c6f7d0 "", ht_ctid=0x260927c, heapRel=0x25aa0a0, 
checkUnique=UNIQUE_CHECK_YES) at nbtree.c:279
#9  0x004e7964 in index_insert (indexRelation=0x7f14795a1a50, 
values=0x7ffe46c6f7f0, isnull=0x7ffe46c6f7d0 "", heap_t_ctid=0x260927c, 
heapRelation=0x25aa0a0, checkUnique=UNIQUE_CHECK_YES)
at indexam.c:204

Of course, the database could have been corrupted after having encountered
many crashes during my experiments. Neverthelesss, even without in-depth
knowledge of the b-tree code I suspect that this stack trace might reflect a
legal situation. In partcular, if _bt_page_recyclable() returned on this
condition:

if (PageIsNew(page))
return true;


Unfortunately I no longer have the cluster nor the core dump, but I remember
all the fields of PageHeader were indeed zeroed.

-- 
Antonin Houska
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt
Web: http://www.postgresql-support.de, http://www.cybertec.at


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

Re: [HACKERS] Improvements in psql hooks for variables

[Rahila Syed]

Hello,

I am beginning to review this patch. Initial comment. I got following
compilation error when I applied the patch on latest sources and run make.

command.c: In function ‘exec_command’:
*command.c:257:5*: error: too few arguments to function ‘ParseVariableBool’
 ParseVariableBool(opt1 + sizeof(prefix) - 1, prefix) ?
 ^
In file included from settings.h:12:0,
 from command.c:50:
variables.h:38:7: note: declared here
 bool  ParseVariableBool(const char *value, const char *name, bool *valid);
   ^
*command.c:1551:4*: error: too few arguments to function ‘ParseVariableBool’
pset.timing = ParseVariableBool(opt, "\\timing");
^
In file included from settings.h:12:0,
 from command.c:50:
variables.h:38:7: note: declared here
 bool  ParseVariableBool(const char *value, const char *name, bool *valid);
   ^
command.c: In function ‘do_pset’:
*command.c:2663:4*: error: too few arguments to function ‘ParseVariableBool’
popt->topt.expanded = ParseVariableBool(value, param);
^
In file included from settings.h:12:0,
 from command.c:50:
variables.h:38:7: note: declared here
 bool  ParseVariableBool(const char *value, const char *name, bool *valid);
   ^
*command.c:2672:4*: error: too few arguments to function ‘ParseVariableBool’
popt->topt.numericLocale = ParseVariableBool(value, param);
^
In file included from settings.h:12:0,
 from command.c:50:
variables.h:38:7: note: declared here
 bool  ParseVariableBool(const char *value, const char *name, bool *valid);
   ^
*command.c:2727:4*: error: too few arguments to function ‘ParseVariableBool’
popt->topt.tuples_only = ParseVariableBool(value, param);
^
In file included from settings.h:12:0,
 from command.c:50:
variables.h:38:7: note: declared here
 bool  ParseVariableBool(const char *value, const char *name, bool *valid);
   ^
*command.c:2759:4*: error: too few arguments to function ‘ParseVariableBool’
if (ParseVariableBool(value, param))
^
In file included from settings.h:12:0,
 from command.c:50:
variables.h:38:7: note: declared here
 bool  ParseVariableBool(const char *value, const char *name, bool *valid);
   ^
*command.c:2781:4:* error: too few arguments to function ‘ParseVariableBool’
popt->topt.default_footer = ParseVariableBool(value, param);
^
In file included from settings.h:12:0,
 from command.c:50:
variables.h:38:7: note: declared here
 bool  ParseVariableBool(const char *value, const char *name, bool *valid);
   ^

Thank you,
Rahila Syed


On Mon, Sep 19, 2016 at 11:21 AM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> You may want to add this to the November commitfest
> https://commitfest.postgresql.org/11/.
>
> On Fri, Sep 16, 2016 at 6:05 PM, Daniel Verite 
> wrote:
> >  Hi,
> >
> > Following the discussion on forbidding an AUTOCOMMIT off->on
> > switch mid-transaction [1], attached is a patch that let the hooks
> > return a boolean indicating whether a change is allowed.
> >
> > Using the hooks, bogus assignments to built-in variables can
> > be dealt with more strictly.
> >
> > For example, pre-patch behavior:
> >
> >   =# \set ECHO errors
> >   =# \set ECHO on
> >   unrecognized value "on" for "ECHO"; assuming "none"
> >   =# \echo :ECHO
> >   on
> >
> > which has two problems:
> > - we have to assume a value, even though we can't know what the user
> meant.
> > - after assignment, the user-visible value of the variable diverges from
> its
> > internal counterpart (pset.echo in this case).
> >
> >
> > Post-patch:
> >   =# \set ECHO errors
> >   =# \set ECHO on
> >   unrecognized value "on" for "ECHO"
> >   \set: error while setting variable
> >   =# \echo :ECHO
> >   errors
> >
> > Both the internal pset.* state and the user-visible value are kept
> unchanged
> > is the input value is incorrect.
> >
> > Concerning AUTOCOMMIT, autocommit_hook() could return false to forbid
> > a switch when the conditions are not met.
> >
> >
> > Another user-visible effect of the patch is that, using a bogus value
> > for a built-in variable on the command-line becomes a fatal error
> > that prevents psql to continue.
> > This is not directly intended by the patch but is a consequence
> > of SetVariable() failing.
> >
> > Example:
> >   $ ./psql -vECHO=bogus
> >   unrecognized value "bogus" for "ECHO"
> >   psql: could not set variable "ECHO"
> >   $ echo $?
> >   1
> >
> > The built-in vars concerned by the change are:
> >
> > booleans: AUTOCOMMIT, ON_ERROR_STOP, QUIET, SINGLELINE, SINGLESTEP
> >
> > non-booleans: ECHO, ECHO_HIDDEN, ON_ERROR_ROLLBACK, COMP_KEYWORD_CASE,
> >  HISTCONTROL, VERBOSITY, SHOW_CONTEXT
> >
> > We could go further to close the gap between pset.* and the built-in
> > variables,
> > by changing how they're initialized and forbidding deletion as Tom
> > suggests in [2], but if there's negative feedback on the above 

Re: [HACKERS] Hash Indexes

[Amit Kapila]

On Mon, Sep 19, 2016 at 11:20 AM, Mark Kirkwood
 wrote:
>
>
> On 17/09/16 06:38, Andres Freund wrote:
>>
>> On 2016-09-16 09:12:22 -0700, Jeff Janes wrote:
>>>
>>> On Thu, Sep 15, 2016 at 7:23 AM, Andres Freund 
>>> wrote:

 One earlier question about this is whether that is actually a worthwhile
 goal.  Are the speed and space benefits big enough in the general case?
 Could those benefits not be achieved in a more maintainable manner by
 adding a layer that uses a btree over hash(columns), and adds
 appropriate rechecks after heap scans?

 Note that I'm not saying that hash indexes are not worthwhile, I'm just
 doubtful that question has been explored sufficiently.
>>>
>>> I think that exploring it well requires good code.  If the code is good,
>>> why not commit it?
>>
>> Because getting there requires a lot of effort, debugging it afterwards
>> would take effort, and maintaining it would also takes a fair amount?
>> Adding code isn't free.
>>
>> I'm rather unenthused about having a hash index implementation that's
>> mildly better in some corner cases, but otherwise doesn't have much
>> benefit. That'll mean we'll have to step up our user education a lot,
>> and we'll have to maintain something for little benefit.
>>
>
> While I see the point of what you are saying here, I recall all previous
> discussions about has indexes tended to go a bit like this:
>
> - until WAL logging of hash indexes is written it is not worthwhile trying
> to make improvements to them
> - WAL logging will be a lot of work, patches 1st please
>
> Now someone has done that work, and we seem to be objecting that because
> they are not improved then the patches are (maybe) not worthwhile.
>

I think saying hash indexes are not improved after proposed set of
patches is an understatement.  The read performance has improved by
more than 80% as compare to HEAD [1] (refer data in Mithun's mail).
Also, tests by Mithun and Jesper has indicated that in multiple
workloads, they are better than BTREE by 30~60% (in fact Jesper
mentioned that he is seeing 40~60% benefit on production database,
Jesper correct me if I am wrong.).  I agree that when index column is
updated they are much worse than btree as of now, but no work has been
done improve it and I am sure that it can be improved for those cases
as well.

In general, I thought the tests done till now are sufficient to prove
the importance of work, but if still Andres and others have doubt and
they want to test some specific cases, then sure we can do more
performance benchmarking.

Mark, thanks for supporting the case for improving Hash Indexes.


[1] - 
https://www.postgresql.org/message-id/CAD__OugX0aOa7qopz3d-nbBAoVmvSmdFJOX4mv5tFRpijqH47A%40mail.gmail.com
-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


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

Re: [HACKERS] Printing bitmap objects in the debugger

[Ashutosh Bapat]

Thanks a lot.

On Fri, Sep 16, 2016 at 7:07 PM, Tom Lane  wrote:
> Ashutosh Bapat  writes:
>>> I'd suggest that this is parallel to nodeToString() and therefore
>>> (a) should be placed beside it,
>
>> Done. Added it after nodeToString().
>
> Pushed, thanks.
>
> regards, tom lane



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database 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] Partition-wise join for join between (declaratively) partitioned tables

[Ashutosh Bapat]

On Fri, Sep 16, 2016 at 6:00 PM, Rajkumar Raghuwanshi
 wrote:
>
> On Fri, Sep 9, 2016 at 3:17 PM, Ashutosh Bapat
>  wrote:
>>
>> Hi All,
>>
>> PFA the patch to support partition-wise joins for partitioned tables. The
>> patch
>> is based on the declarative parition support patches provided by Amit
>> Langote
>> on 26th August 2016.
>
>
> I have applied declarative partitioning patches posted by Amit Langote on 26
> Aug 2016 and then partition-wise-join patch,  getting below error while make
> install.
>
> ../../../../src/include/nodes/relation.h:706: error: redefinition of typedef
> ‘PartitionOptInfo’
> ../../../../src/include/nodes/relation.h:490: note: previous declaration of
> ‘PartitionOptInfo’ was here
> make[4]: *** [gistbuild.o] Error 1
> make[4]: Leaving directory
> `/home/edb/Desktop/edb_work/WORKDB/PG/postgresql/src/backend/access/gist'
> make[3]: *** [gist-recursive] Error 2
> make[3]: Leaving directory
> `/home/edb/Desktop/edb_work/WORKDB/PG/postgresql/src/backend/access'
> make[2]: *** [access-recursive] Error 2
> make[2]: Leaving directory
> `/home/edb/Desktop/edb_work/WORKDB/PG/postgresql/src/backend'
> make[1]: *** [all-backend-recurse] Error 2
> make[1]: Leaving directory
> `/home/edb/Desktop/edb_work/WORKDB/PG/postgresql/src'
> make: *** [all-src-recurse] Error 2
>
> PS : I am using - gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-17)
>
> Attached the patch for the fix of above error.

Thanks for the report. I will fix this in the next patch.



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


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