Re: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

2017-09-10 Thread Michael Paquier
On Mon, Aug 21, 2017 at 9:51 PM, Michael Paquier
 wrote:
> On Tue, Jun 20, 2017 at 1:11 PM, Michael Paquier
>  wrote:
>> With the tests directly in the patch, things are easy to run. WIth
>> PG10 stabilization work, of course I don't expect much feedback :)
>> But this set of patches looks like the direction we want to go so as
>> JDBC and libpq users can take advantage of channel binding with SCRAM.
>
> Attached is a new patch set, rebased as of c6293249.

And again a new set to fix the rotten bits caused by 85f4d63.
-- 
Michael


0001-Refactor-routine-to-test-connection-to-SSL-server.patch
Description: Binary data


0002-Support-channel-binding-tls-unique-in-SCRAM.patch
Description: Binary data


0003-Add-connection-parameters-saslname-and-saslchannelbi.patch
Description: Binary data


0004-Implement-channel-binding-tls-server-end-point-for-S.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] CLUSTER command progress monitor

2017-09-10 Thread Tatsuro Yamada

On 2017/09/08 18:55, Robert Haas wrote:

On Wed, Aug 30, 2017 at 10:12 PM, Tatsuro Yamada
 wrote:

   1. scanning heap
   2. sort tuples


These two phases overlap, though. I believe progress reporting for
sorts is really hard.  In the simple case where the data fits in
work_mem, none of the work of the sort gets done until all the data is
read.  Once you switch to an external sort, you're writing batch
files, so a lot of the work is now being done during data loading.
But as the number of batch files grows, the final merge at the end
becomes an increasingly noticeable part of the cost, and eventually
you end up needing multiple merge passes.  I think we need some smart
way to report on sorts so that we can tell how much of the work has
really been done, but I don't know how to do it.


Thanks for the comment.

As you know, CLUSTER command uses SEQ SCAN or INDEX SCAN as a scan method by
cost estimation. In the case of SEQ SCAN, these two phases not overlap.
However, in INDEX SCAN, it overlaps. Therefore I created the phase of "scan
heap and write new heap" when INDEX SCAN was selected.

I agree that progress reporting for sort is difficult. So it only reports
the phase ("sorting tuples") in the current design of progress monitor of 
cluster.
It doesn't report counter of sort.



  heap_tuples_total   | bigint  |   |  |


The patch is getting the value reported as heap_tuples_total from
OldHeap->rd_rel->reltuples.  I think this is pointless: the user can
see that value anyway if they wish.  The point of the progress
counters is to expose things the user couldn't otherwise see.  It's
also not necessarily accurate: it's only an estimate in the best case,
and may be way off if the relation has recently be extended by a large
amount.  I think it's pretty important that we try hard to only report
values that are known to be accurate, because users hate (and mock)
inaccurate progress reports.


Do you mean to use the number of rows by using below calculation instead
OldHeap->rd_rel->reltuples?

 estimate rows = physical table size / average row length

I understand that OldHeap->rd_rel->reltuples is sometimes useless because
it is correct by auto analyze and it can't perform when under a threshold.

I'll add it in next patch and also share more detailed the current design of
progress monitor for cluster.

Regards,
Tatsuro Yamada




--
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] Setting pd_lower in GIN metapage

2017-09-10 Thread Amit Kapila
On Sun, Sep 10, 2017 at 9:56 PM, Tom Lane  wrote:
> Amit Kapila  writes:
>> On Sun, Sep 10, 2017 at 11:52 AM, Michael Paquier
>>  wrote:
>>> Coordinating efforts here would be nice. If you, Amit K, are taking
>>> care of a patch for btree and hash
>
>> I think here we should first agree on what we want to do.  Based on
>> Tom's comment, I was thinking of changing comments in btree/hash part
>> and additionally for hash indexes, I can see if we can pass
>> REGBUF_STANDARD for all usages of metapage.  I am not sure if we want
>> similar exercise for btree as well.
>
> FWIW, now that we've noticed the discrepancy, I'm for using
> REGBUF_STANDARD or equivalent for all metapage calls.  Even if it
> saves no space, inconsistency is bad because it's confusing.
>

Agreed.  However, I feel there is no harm in doing in two patches, one
for hash/btree and second for all other indexes (or maybe separate
patches for them as well; I haven't yet looked into the work involved
for other indexes) unless you prefer to do it all at a one-shot.

-- 
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] The case for removing replacement selection sort

2017-09-10 Thread Peter Geoghegan
On Sun, Sep 10, 2017 at 5:59 PM, Tomas Vondra
 wrote:
> OK, so 1MB, 4MB, 8MB, 32MB?

Right.

> Ah, so you suggest doing all the tests on current master, by only
> tweaking the replacement_sort_tuples value? I've been testing master vs.
> your patch, but I guess setting replacement_sort_tuples=0 should have
> the same effect.

I think that there may be a very slight advantage to RS-free
performance with my patch actually applied, because it removes the
number of instructions that heap maintenance (merging) routines
consist of. This is due to my removing HEAPCOMPARE()/tupindex
handling. However, it's probably a low single digit percentage
difference -- not enough to be worth taking into account, probably. I
assume that just setting replacement_sort_tuples to zero is more
convenient for you (that's what I did).

To be clear, you'll still need to set replacement_sort_tuples high
when testing RS, to make sure that we really use it for at least the
first run when we're expected to. (There is no easy way to have
testing mechanically verify that we really do only have one run in the
end with RS, but I assume that such paranoia is unneeded.)

> I probably won't eliminate the random/DESC data sets, though. At least
> not from the two smaller data sets - I want to do a bit of benchmarking
> on Heikki's polyphase merge removal patch, and for that patch those data
> sets are still relevant. Also, it's useful to have a subset of results
> where we know we don't expect any change.

Okay. The DESC dataset is going to make my patch look good, because it
won't change anything for merging (same number of runs in the end),
but sorting will be slower for the first run with RS.

> Meh, more data is probably better. And with the reduced work_mem values
> and skipping of random/DESC data sets it should complete much faster.

Note that my own test case had a much higher number of tuples than
even 10 million -- it had 100 million tuples. I think that if any of
your test cases bring about a new insight, it will not be due to the
number of distinct tuples. But, if the extra time it takes doesn't
matter to you, then it doesn't matter to me either.

-- 
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] Setting pd_lower in GIN metapage

2017-09-10 Thread Michael Paquier
On Mon, Sep 11, 2017 at 1:26 AM, Tom Lane  wrote:
> FWIW, now that we've noticed the discrepancy, I'm for using
> REGBUF_STANDARD or equivalent for all metapage calls.  Even if it
> saves no space, inconsistency is bad because it's confusing.

OK, I don't mind having a more aggressive approach, but it would
definitely be nicer if the REGBUF additions are done in a second patch
that can be applied on top of the one setting pd_lower where needed.
Both things are linked, still are aimed to solve different problems.
-- 
Michael


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


Re: [HACKERS] Still another race condition in recovery TAP tests

2017-09-10 Thread Michael Paquier
On Sun, Sep 10, 2017 at 12:38 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> On Sat, Sep 9, 2017 at 1:43 PM, Tom Lane  wrote:
>>> Yeah, even if we fixed this particular call site, I'm sure the issue
>>> would come up again.  Certainly we expect hot backups to work with
>>> a changing source directory.
>
>> In short, I'd still like to keep RecursiveCopy for now, but change its
>> code so as a copy() is not a hard failure. What do you think?
>
> The specific case we need to allow is "ENOENT on a file/dir that was
> there a moment ago".  I think it still behooves us to complain about
> anything else.  If you think it's a simple fix, have at it.  But
> I see at least three ways for _copypath_recurse to fail depending on
> exactly when the file disappears.

I have finally been able to take a couple of hours and looked at
problems in this module.

Errno is supported down to 5.8.8 per the docs, and those are set also
on Windows, copy() and opendir() complain with ENOENT for missing
entries:
http://perldoc.perl.org/5.8.8/Errno.html
readdir() does not though. If for example a directory gets removed in
the middle of a scan, then an empty list is returned. mkdir() would
not fail on ENOENT either, the parent directory would have already
been created if a child repository is being scanned.

With the check for -d and -f, this brings indeed the count to three
code paths. With the patch attached, I have added some manual sleep
calls in RecursiveCopy.pm and triggered manual deletions of the source
repository. The copy is able to complete in any case, warning about
missing directories or files. I have added warn messages in the patch
when ENOENT is triggered, though I think that those should be removed
in the final patch.

By the way, 010_logical_decoding_timelines.pl does not need to include
RecursiveCopy.
-- 
Michael


tap-copypath-enoent.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] The case for removing replacement selection sort

2017-09-10 Thread Tomas Vondra
On 09/11/2017 02:22 AM, Peter Geoghegan wrote:
> On Sun, Sep 10, 2017 at 5:07 PM, Tomas Vondra
>  wrote:
>> I'm currently re-running the benchmarks we did in 2016 for 9.6, but
>> those are all sorts with a single column (see the attached script). But
>> it'd be good to add a few queries testing sorts with multiple keys. We
>> can either tweak some of the existing data sets + queries, or come up
>> with entirely new tests.
> 
> I see that work_mem is set like this in the script:
> 
> "for wm in '1MB' '8MB' '32MB' '128MB' '512MB' '1GB'; do"
> 
> I suggest that we forget about values over 32MB, since the question of
> how well quicksort does there was settled by your tests in 2016. I
> would also add '4MB' to the list of wm values that you'll actually
> test.

OK, so 1MB, 4MB, 8MB, 32MB?

> 
> Any case with input that is initially in random order or DESC sort 
> order is not interesting, either. I suggest you remove those, too.
> 

OK.

> I think we're only interested in benchmarks where replacement
> selection really does get its putative best case (no merge needed in
> the end). Any (almost) sorted cases (the only cases that you are
> interesting to test now) will always manage that, once you set
> replacement_sort_tuples high enough, and provided there isn't even a
> single tuple that is completely out of order. The "before" cases here
> should have a replacement_sort_tuples of 1 billion (so that we're sure
> to not have the limit prevent the use of replacement selection in the
> first place), versus the "after" cases, which should have a
> replacement_sort_tuples of 0 to represent my proposal (to represent
> performance in a world where replacement selection is totally
> removed).
> 

Ah, so you suggest doing all the tests on current master, by only
tweaking the replacement_sort_tuples value? I've been testing master vs.
your patch, but I guess setting replacement_sort_tuples=0 should have
the same effect.

I probably won't eliminate the random/DESC data sets, though. At least
not from the two smaller data sets - I want to do a bit of benchmarking
on Heikki's polyphase merge removal patch, and for that patch those data
sets are still relevant. Also, it's useful to have a subset of results
where we know we don't expect any change.

>> For the existing queries, I should have some initial results
>> tomorrow, at least for the data sets with 100k and 1M rows. The
>> tests with 10M rows will take much more time (it takes 1-2hours for
>> a single work_mem value, and we're testing 6 of them).
> 
> I myself don't see that much value in a 10M row test.
> 

Meh, more data is probably better. And with the reduced work_mem values
and skipping of random/DESC data sets it should complete much faster.

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] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-10 Thread Michael Paquier
On Mon, Sep 11, 2017 at 9:38 AM, Bossart, Nathan  wrote:
> I agree that it is nice to see when relations are skipped, but I do not
> know if the WARNING messages would provide much value for this
> particular use case (i.e. 'VACUUM;').  If a user does not provide a list
> of tables to VACUUM, they might not care too much about WARNINGs for
> dropped tables.

Some users trigger manual VACUUM with cron jobs in moments of
low-activity as autovacuum may sometimes not be able to keep up with
hte bloat cleanup. It seems to me that getting WARNING messages is
particularly important for partitioned tables.
-- 
Michael


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


Re: [HACKERS] [Proposal] Allow users to specify multiple tables in VACUUM commands

2017-09-10 Thread Bossart, Nathan
On 9/9/17, 7:28 AM, "Michael Paquier"  wrote:
> In the duplicate patch, it seems to me that you can save one lookup at
> the list of VacuumRelation items by checking for column duplicates
> after checking that all the columns are defined. If you put the
> duplicate check before closing the relation you can also use the
> schema name associated with the Relation.

I did this so that the ERROR prioritization would be enforced across all
relations.  For example:

VACUUM ANALYZE table1 (a, a), table2 (does_not_exist);

If I combine the 'for' loops to save a lookup, this example behaves
differently.  Instead of an ERROR for the nonexistent column, you would
hit an ERROR for the duplicate column in table1's list.  However, I do
not mind changing this.

> +   if (i == InvalidAttrNumber)
> +   ereport(ERROR,
> +   (errcode(ERRCODE_UNDEFINED_COLUMN),
> +errmsg("column \"%s\" of relation \"%s\" does not exist",
> +   col, RelationGetRelationName(rel;
> This could use the schema name unconditionally as you hold a Relation
> here, using RelationGetNamespace().

Sure, I think this is a good idea.  I'll make this change in the next
version of the patch.

> if (!onerel)
> +   {
> +   /*
> +* If one of the relations specified by the user has disappeared
> +* since we last looked it up, let them know so that they do not
> +* think it was actually analyzed.
> +*/
> +   if (!IsAutoVacuumWorkerProcess() && relation)
> +   ereport(WARNING,
> + (errmsg("skipping \"%s\" --- relation no longer exists",
> + relation->relname)));
> +
> return;
> +   }
> Hm. So if the relation with the defined OID is not found, then we'd
> use the RangeVar, but this one may not be set here:
> +   /*
> +* It is safe to leave everything except the OID empty here.
> +* Since no tables were specified in the VacuumStmt, we know
> +* we don't have any columns to keep track of.  Also, we do
> +* not need the RangeVar, because it is only used for error
> +* messaging when specific relations are chosen.
> +*/
> +   rel_oid = HeapTupleGetOid(tuple);
> +   relinfo = makeVacuumRelation(NULL, NIL, rel_oid);
> +   vacrels_tmp = lappend(vacrels_tmp, relinfo);
> So if the relation is analyzed but skipped, we would have no idea that
> it actually got skipped because there are no reports about it. That's
> not really user-friendly. I am wondering if we should not instead have
> analyze_rel also enforce the presence of a RangeVar, and adding an
> assert at the beginning of the function to undertline that, and also
> do the same for vacuum(). It would make things also consistent with
> vacuum() which now implies on HEAD that a RangeVar *must* be
> specified.

I agree that it is nice to see when relations are skipped, but I do not
know if the WARNING messages would provide much value for this
particular use case (i.e. 'VACUUM;').  If a user does not provide a list
of tables to VACUUM, they might not care too much about WARNINGs for
dropped tables.

> Are there opinions about back-patching the patch checking for
> duplicate columns? Stable branches now complain about an unhelpful
> error message.

I wouldn't mind drafting something up for the stable branches.

Nathan


-- 
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] The case for removing replacement selection sort

2017-09-10 Thread Peter Geoghegan
On Sun, Sep 10, 2017 at 5:07 PM, Tomas Vondra
 wrote:
> I'm currently re-running the benchmarks we did in 2016 for 9.6, but
> those are all sorts with a single column (see the attached script). But
> it'd be good to add a few queries testing sorts with multiple keys. We
> can either tweak some of the existing data sets + queries, or come up
> with entirely new tests.

I see that work_mem is set like this in the script:

"for wm in '1MB' '8MB' '32MB' '128MB' '512MB' '1GB'; do"

I suggest that we forget about values over 32MB, since the question of
how well quicksort does there was settled by your tests in 2016. I
would also add '4MB' to the list of wm values that you'll actually
test.

Any case with input that is initially in random order or DESC sort
order is not interesting, either. I suggest you remove those, too.

I think we're only interested in benchmarks where replacement
selection really does get its putative best case (no merge needed in
the end). Any (almost) sorted cases (the only cases that you are
interesting to test now) will always manage that, once you set
replacement_sort_tuples high enough, and provided there isn't even a
single tuple that is completely out of order. The "before" cases here
should have a replacement_sort_tuples of 1 billion (so that we're sure
to not have the limit prevent the use of replacement selection in the
first place), versus the "after" cases, which should have a
replacement_sort_tuples of 0 to represent my proposal (to represent
performance in a world where replacement selection is totally
removed).

> For the existing queries, I should have some initial results tomorrow,
> at least for the data sets with 100k and 1M rows. The tests with 10M
> rows will take much more time (it takes 1-2hours for a single work_mem
> value, and we're testing 6 of them).

I myself don't see that much value in a 10M row test.

Thanks for volunteering to test!
-- 
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] Automatic testing of patches in commit fest

2017-09-10 Thread Thomas Munro
On Mon, Sep 11, 2017 at 12:10 PM, Jeff Janes  wrote:
> On Sun, Sep 10, 2017 at 4:40 PM, Michael Paquier 
>> http://commitfest.cputube.org/
>
> This looks very interesting.  But when I click on a "build: failing" icon,
> it takes me to a generic page,  not one describing how that specific build
> is failing.

True.  The problem is that travis-ci.org doesn't seem to provide a URL
that can take you directly to the latest build for a given branch,
instead I'd need to figure out how to talk to their API to ask for the
build ID of the latest build for that branch.  For now you have to
note the Commitfest entry ID, and then when find the corresponding
branch (ie commitfest/14/1234) on the page it dumps you on.  It would
be nice to fix that.

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


Re: [HACKERS] Automatic testing of patches in commit fest

2017-09-10 Thread Thomas Munro
On Mon, Sep 11, 2017 at 11:40 AM, Michael Paquier
 wrote:
> Thomas Munro has hacked up a prototype of application testing
> automatically if patches submitted apply and build:
> http://commitfest.cputube.org/
>
> I would recommend have a look at it from time to time if you are a
> patch author (or a reviewer) as any failure may say that your patch
> has rotten over time and needs a rebase. It is good to keep the commit
> fest entries build-clean at least for testers.

I should add: this is a spare-time effort, a work-in-progress and
building on top of a bunch of hairy web scraping, so it may take some
time to perfect.  One known problem at the moment is that the mail
archive (where patches are fetched from) cuts off long threads so it
doesn't manage to find the latest version of (for example) the
Partition-Wise Join patch.  Other problems include getting confused by
incidental material posted in .patch form, patches that depend on
other patches or patches whose apply order is not obvious, or ... etc
etc.  There are also a few other patches missing currently for various
reasons, which I intend to fix, gradually.

That said, it might already be useful for catching bitrot and things
like TAP test failures, contrib regressions and documentation build
errors which (based on the evidence I've seen so far) many submitters
aren't actually running themselves.  YMMV.

I've been having conversations with Magnus and Stephen about what a
more robust version integrated with the CF app might eventually look
like.  The basic idea here is:  Commitfest submissions are analogous
to pull request, which many comparable projects test automatically
using CI technology that is generously made available to open source
projects and well integrated with popular free git hosts.  We should
too!

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


Re: [HACKERS] Statement timeout behavior in extended queries

2017-09-10 Thread Andres Freund
Hi,

On 2017-09-11 09:10:49 +0900, Tatsuo Ishii wrote:
> If you don't mind, can you please commit/push the patch?

Ok, will do so.

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] Automatic testing of patches in commit fest

2017-09-10 Thread Jeff Janes
On Sun, Sep 10, 2017 at 4:40 PM, Michael Paquier 
wrote:

> Hi all,
>
> Thomas Munro has hacked up a prototype of application testing
> automatically if patches submitted apply and build:
> http://commitfest.cputube.org/
>
> I would recommend have a look at it from time to time if you are a
> patch author (or a reviewer) as any failure may say that your patch
> has rotten over time and needs a rebase. It is good to keep the commit
> fest entries build-clean at least for testers.
> Thanks,


This looks very interesting.  But when I click on a "build: failing" icon,
it takes me to a generic page,  not one describing how that specific build
is failing.

Cheers,

Jeff


Re: [HACKERS] Statement timeout behavior in extended queries

2017-09-10 Thread Tatsuo Ishii
Andres,

If you don't mind, can you please commit/push the patch?

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

>> What do you think?  I've not really tested this with the extended
>> protocol, so I'd appreciate if you could rerun your test from the
>> older thread.
> 
> Attached is your patch just rebased against master branch.
> 
> Also I attached the test cases and results using pgproto on patched
> master branch. All looks good to me.
> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


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


Re: [HACKERS] The case for removing replacement selection sort

2017-09-10 Thread Tomas Vondra

On 09/11/2017 01:03 AM, Peter Geoghegan wrote:
> On Sat, Sep 9, 2017 at 8:28 AM, Greg Stark  wrote:
>> This may be a bit "how long is a piece of string" but how do those two
>> compare with string sorting in an interesting encoding/locale -- say
>> /usr/share/dict/polish in pl_PL for example. It's certainly true that
>> people do sort text as well as numbers.
> 
> I haven't looked at text, because I presume that it's very rare for 
> tuples within a table to be more or less ordered by a text
> attribute. While the traditional big advantage of replacement
> selection has always been its ability to double initial run length on
> average, where text performance is quite interesting because
> localized clustering still happens, that doesn't really seem relevant
> here. The remaining use of replacement selection is expressly only
> about the "single run, no merge" best case, and in particular about
> avoiding regressions when upgrading from versions prior to 9.6 (9.6
> is the version where we began to generally prefer quicksort).
> 
>> Also, people often sort on keys of more than one column
> 
> Very true. I should test this.
> 

I'm currently re-running the benchmarks we did in 2016 for 9.6, but
those are all sorts with a single column (see the attached script). But
it'd be good to add a few queries testing sorts with multiple keys. We
can either tweak some of the existing data sets + queries, or come up
with entirely new tests.

The current tests construct data sets with different features (unique or
low/high-cardinality, random/sorted/almost-sorted). How should that work
for multi-key sorts? Same features for all columns, or some mix?

For the existing queries, I should have some initial results tomorrow,
at least for the data sets with 100k and 1M rows. The tests with 10M
rows will take much more time (it takes 1-2hours for a single work_mem
value, and we're testing 6 of them).

But perhaps we can reduce the number of tests somehow, only testing the
largest/smallest work_mem values? So that we could get some numbers now,
possibly running the complete test suite later.

regards

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


sort-bench.sh
Description: application/shellscript

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


[HACKERS] Automatic testing of patches in commit fest

2017-09-10 Thread Michael Paquier
Hi all,

Thomas Munro has hacked up a prototype of application testing
automatically if patches submitted apply and build:
http://commitfest.cputube.org/

I would recommend have a look at it from time to time if you are a
patch author (or a reviewer) as any failure may say that your patch
has rotten over time and needs a rebase. It is good to keep the commit
fest entries build-clean at least for testers.
Thanks,
-- 
Michael


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


Re: [HACKERS] [PATCH] Pattern based listeners for asynchronous messaging (LISTEN/NOTIFY)

2017-09-10 Thread Markus Sintonen
Hi Marko!
Thanks for the good feedback.

Good point on the pg_listening_channels(). Do you think we could change the
interface of the function? At least PG v10 has changed functions elsewhere
quite dramatically eg. related to xlog functions.
We could change the pg_listening_channels() return type to 'setof record'
which would include name (text) and isPattern (bool). This would also allow
some future additions (eg. time of last received notification). One other
way with better backward compatibility would be to add some kind of postfix
to patterns to identify them and keep the 'text' return type. It would
return eg. 'foo% - PATTERN' for a pattern, but this is quite nasty. Second
way would be to add totally new function eg. 'pg_listening_channels_info'
with return type of 'setof record' and keep the original function as it is
(just change the behaviour on duplicated names and patterns as you showed).

You also have a good point on the UNLISTEN. At first I thought that
UNLISTEN SIMILAR TO 'xxx' would be semantically confusing since it fells
that it would do some pattern based unregistering. But by documenting the
behaviour it might be ok. I also agree that it would be best to have
distinction between unlistening patterns and regular names.

I'll reflect on the rest of the feedback on the next patch if we reach some
conclusion on the pg_listening_channels and UNLISTEN.

BR
Markus

2017-09-08 2:44 GMT+03:00 Marko Tiikkaja :

> Hi Markus,
>
> On Sun, Aug 20, 2017 at 9:56 PM, Markus Sintonen <
> markus.sinto...@gmail.com> wrote:
>
>> I also encountered this when I built it with different configuration. I
>> attached updated patch with the correct number of arguments to
>> 'similar_escape'. I also added preliminary documentation to the patch.
>> (Unfortunately unable to currently compile the documentation for testing
>> purpose on Windows probably because of commit https://github.com/post
>> gres/postgres/commit/510074f9f0131a04322d6a3d2a51c87e6db243f9. I
>> followed https://www.postgresql.org/docs/devel/static/install-windows
>> -full.html#idm45412738673840.)
>>
>> What do you think about the syntax? There was a suggestion to specify
>> type of the pattern (eg ltree extension) but to me this feels like a
>> overkill.
>> One option here would be eg:
>> LISTEN PATTERN 'foo%' TYPE 'similar'
>> LISTEN PATTERN 'foo.*' TYPE 'ltree'
>>
>
> Not that my opinion matters, but I think we should pick one pattern style
> and stick to it.  SIMILAR TO doesn't seem like the worst choice.  ltree
> seems useless.
>
> As for the rest of the interface..
>
> First, I think mixing patterns and non-patterns is weird.  This is
> apparent in at least two cases:
>
> marko=# listen "foo%";
> LISTEN
> marko=# listen similar to 'foo%';
> LISTEN
> marko=# select * from pg_listening_channels();
>  pg_listening_channels
> ---
>  foo%
> (1 row)
>
> -- Not actually listening on the pattern.  Confusion.
>
> The second case being the way UNLISTEN can be used to unlisten patterns,
> too.  It kind of makes sense given that you can't really end up with both a
> channel name and a pattern with the same source string, but it's still
> weird.  I think it would be much better to keep these completely separate
> so that you could be listening on both the channel "foo%" and the pattern
> 'foo%' at the same time, and you'd use a bare UNLISTEN to unsubscribe from
> the former, and UNLISTEN SIMILAR TO for the latter.  As you said in the
> original email:
>
> > Allow *UNLISTEN SIMILAR TO 'xxx'* which would unregister matching
> listeners. To me this feels confusing therefore it is not in the patch.
>
> I agree, starting to match the patterns themselves would be confusing.  So
> I think we should use that syntax for unsubscribing from patterns.  But
> others should feel free to express their opinions on this.
>
> Secondly -- and this is a kind of continuation to my previous point of
> conflating patterns and non-patterns -- I don't think you can get away with
> not changing the interface for pg_listening_channels().  Not knowing which
> ones are compared byte-for-byte and which ones are patterns just seems
> weird.
>
>
> As for the patch itself, I have a couple of comments.  I'm writing this
> based on the latest commit in your git branch, commit
> fded070f2a56024f931b9a0f174320eebc362458.
>
> In queue_listen(), the new variables would be better declared at the
> innermost scope possible.  The datum is only used if isSimilarToPattern is
> true, errormsg only if compile_regex didn't return REG_OKAY, etc..
>
> I found this comment confusing at first: "If compiled RE was not applied
> as a listener then it is freed at transaction commit."  The past tense
> makes it seem like something that has already happened when that code runs,
> when in reality it happens later in the transaction.
>
> I'm not a fan of the dance you're doing with pcompreg.  I think it would
> be better to 

[HACKERS] Constifying numeric.c's local vars

2017-09-10 Thread Andres Freund
Hi,

For JIT inlining currently functions can't be inlined if they reference
non-constant static variables. That's because there's no way, at least
none I know of, to link to the correct variable, instead of duplicating,
the linker explicitly renames symbols after all (that's the whole point
of static vars).  There's a bunch of weird errors if you ignore that ;)

One large user of unnecessary non-constant static variables is
numeric.c.  More out of curiosity - numeric is slow enough in itself to
make inlining not a huge win - I converted it to use consts.

before:
andres@alap4:~/build/postgres/dev-assert/vpath$ size --format=SysV 
src/backend/utils/adt/numeric.o |grep -v debug|grep -v '^.group'
src/backend/utils/adt/numeric.o  :
section size   addr
.text  68099  0
.data 64  0
.bss   2  0
.rodata 4256  0
.data.rel.local  224  0
.comment  29  0
.note.GNU-stack0  0
.eh_frame   5976  0
Total 395590


after:

$ size --format=SysV src/backend/utils/adt/numeric.o |grep -v debug|grep -v 
'^.group'
src/backend/utils/adt/numeric.o  :
sectionsize   addr
.text 68108  0
.data 0  0
.bss  0  0
.rodata4288  0
.data.rel.ro.local  224  0
.comment 29  0
.note.GNU-stack   0  0
.eh_frame  5976  0
Total395586

Nicely visible that the data is moved from a mutable segment to a
readonly one.

It's a bit ugly that some consts have to be casted away in the constant
definitions, but aside from just inlining the values, I don't quite see
a better solution?

Leaving JIT aside, I think stuff like this is worthwhile on its own...

Greetings,

Andres Freund
>From 2b114f2c91c7bbf80e48ef5c9653748c957bf15f Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Sun, 10 Sep 2017 16:20:41 -0700
Subject: [PATCH] Constify numeric.c.

---
 src/backend/utils/adt/numeric.c | 210 +---
 1 file changed, 110 insertions(+), 100 deletions(-)

diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c
index bc01f3c284..ddc44d5179 100644
--- a/src/backend/utils/adt/numeric.c
+++ b/src/backend/utils/adt/numeric.c
@@ -367,59 +367,59 @@ typedef struct NumericSumAccum
  * Some preinitialized constants
  * --
  */
-static NumericDigit const_zero_data[1] = {0};
-static NumericVar const_zero =
-{0, 0, NUMERIC_POS, 0, NULL, const_zero_data};
+static const NumericDigit const_zero_data[1] = {0};
+static const NumericVar const_zero =
+{0, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_zero_data};
 
-static NumericDigit const_one_data[1] = {1};
-static NumericVar const_one =
-{1, 0, NUMERIC_POS, 0, NULL, const_one_data};
+static const NumericDigit const_one_data[1] = {1};
+static const NumericVar const_one =
+{1, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_one_data};
 
-static NumericDigit const_two_data[1] = {2};
-static NumericVar const_two =
-{1, 0, NUMERIC_POS, 0, NULL, const_two_data};
+static const NumericDigit const_two_data[1] = {2};
+static const NumericVar const_two =
+{1, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_two_data};
 
 #if DEC_DIGITS == 4 || DEC_DIGITS == 2
-static NumericDigit const_ten_data[1] = {10};
-static NumericVar const_ten =
-{1, 0, NUMERIC_POS, 0, NULL, const_ten_data};
+static const NumericDigit const_ten_data[1] = {10};
+static const NumericVar const_ten =
+{1, 0, NUMERIC_POS, 0, NULL, (NumericDigit *) const_ten_data};
 #elif DEC_DIGITS == 1
-static NumericDigit const_ten_data[1] = {1};
-static NumericVar const_ten =
-{1, 1, NUMERIC_POS, 0, NULL, const_ten_data};
+static const NumericDigit const_ten_data[1] = {1};
+static const NumericVar const_ten =
+{1, 1, NUMERIC_POS, 0, NULL, (NumericDigit *) const_ten_data};
 #endif
 
 #if DEC_DIGITS == 4
-static NumericDigit const_zero_point_five_data[1] = {5000};
+static const NumericDigit const_zero_point_five_data[1] = {5000};
 #elif DEC_DIGITS == 2
-static NumericDigit const_zero_point_five_data[1] = {50};
+static const NumericDigit const_zero_point_five_data[1] = {50};
 #elif DEC_DIGITS == 1
-static NumericDigit const_zero_point_five_data[1] = {5};
+static const NumericDigit const_zero_point_five_data[1] = {5};
 #endif
-static NumericVar const_zero_point_five =
-{1, -1, NUMERIC_POS, 1, NULL, const_zero_point_five_data};
+static const NumericVar const_zero_point_five =
+{1, -1, NUMERIC_POS, 1, NULL, (NumericDigit *) const_zero_point_five_data};
 
 #if DEC_DIGITS == 4
-static NumericDigit const_zero_point_nine_data[1] = {9000};
+static const NumericDigit const_zero_point_nine_data[1] = {9000};
 #elif DEC_DIGITS == 2
-static NumericDigit const_zero_point_nine_data[1] = {90};
+static const NumericDigit const_zero_point_nine_data[1] = {90};
 #elif DEC_DIGITS == 1
-static NumericDigit 

Re: [HACKERS] The case for removing replacement selection sort

2017-09-10 Thread Peter Geoghegan
On Sat, Sep 9, 2017 at 8:28 AM, Greg Stark  wrote:
> This may be a bit "how long is a piece of string" but how do those two
> compare with string sorting in an interesting encoding/locale -- say
> /usr/share/dict/polish in pl_PL for example. It's certainly true that
> people do sort text as well as numbers.

I haven't looked at text, because I presume that it's very rare for
tuples within a table to be more or less ordered by a text attribute.
While the traditional big advantage of replacement selection has
always been its ability to double initial run length on average, where
text performance is quite interesting because localized clustering
still happens, that doesn't really seem relevant here. The remaining
use of replacement selection is expressly only about the "single run,
no merge" best case, and in particular about avoiding regressions when
upgrading from versions prior to 9.6 (9.6 is the version where we
began to generally prefer quicksort).

> Also, people often sort on
> keys of more than one column

Very true. I should test this.

-- 
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] Patch: Add --no-comments to skip COMMENTs with pg_dump

2017-09-10 Thread Stephen Frost
Michael,

* Michael Paquier (michael.paqu...@gmail.com) wrote:
> As there begins to be many switches of this kind and much code
> duplication, I think that some refactoring into a more generic switch
> infrastructure would be nicer.

I have been thinking about this also and agree that it would be nicer,
but then these options would just become "shorthand" for the generic
switch.

Thanks!

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] mysql_fdw + PG10: unrecognized node type: 217

2017-09-10 Thread Tom Lane
Christoph Berg  writes:
> I'm not sure if this is a bug in mysql_fdw, or in PG10:

> ! ERROR:  unrecognized node type: 217

Hm, nodetag 217 is T_List according to gdb.  Wouldn't expect that
failure in very many places.  If you could get a stack trace from
the errfinish call, it might help narrow things down.

Offhand my bet is on mysql_fdw needing an update for some PG10
change, but that's just a guess.

regards, tom lane


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


[HACKERS] mysql_fdw + PG10: unrecognized node type: 217

2017-09-10 Thread Christoph Berg
Hi,

I'm not sure if this is a bug in mysql_fdw, or in PG10:

== running regression test queries==
test mysql_fdw... FAILED

*** 345,359 
  NOTICE:  Found number Three
  NOTICE:  Found number Four
  NOTICE:  Found number Five
! NOTICE:  Found number Six
! NOTICE:  Found number Seven
! NOTICE:  Found number Eight
! NOTICE:  Found number Nine
!  test_param_where 
! --
!  
! (1 row)
! 
  DELETE FROM employee;
  DELETE FROM department;
  DELETE FROM empdata;
--- 344,352 
  NOTICE:  Found number Three
  NOTICE:  Found number Four
  NOTICE:  Found number Five
! ERROR:  unrecognized node type: 217
! CONTEXT:  SQL statement "select bfrom numbers where a=x"
! PL/pgSQL function test_param_where() line 6 at SQL statement
  DELETE FROM employee;
  DELETE FROM department;
  DELETE FROM empdata;

mysql_fdw master at 7d084c59, PG10 at 6913d066.


The testsuite was running against this mysql schema:

CREATE DATABASE testdb;
USE testdb;

CREATE USER 'foo'@'127.0.0.1' IDENTIFIED BY 'bar';
GRANT ALL PRIVILEGES ON testdb.* TO 'foo'@'127.0.0.1';

CREATE TABLE department(department_id int, department_name text, PRIMARY KEY 
(department_id));
CREATE TABLE employee(emp_id int, emp_name text, emp_dept_id int, PRIMARY KEY 
(emp_id));
CREATE TABLE empdata (emp_id int, emp_dat blob, PRIMARY KEY (emp_id));
CREATE TABLE numbers(a int PRIMARY KEY, b varchar(255));

Christoph


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


[HACKERS] relation mapping file checksum failure

2017-09-10 Thread Tom Lane
A pretty routine "make check-world" run on HEAD just blew up here,
with the symptom

2017-09-10 14:27:33.318 EDT [2535] FATAL:  relation mapping file 
"global/pg_filenode.map" contains incorrect checksum
2017-09-10 14:27:33.319 EDT [2519] LOG:  autovacuum launcher process (PID 2535) 
exited with exit code 1
2017-09-10 14:27:33.319 EDT [2519] LOG:  terminating any other active server 
processes

This was during a REINDEX DATABASE operation in
src/bin/scripts/t/200_connstr.pl, for what that's worth.

Not sure what to make of this.  Random bit flip?  My machine does have
ECC memory, though.  Race condition against some updater?  But the file
is only 512 bytes, and we've always assumed that that size of disk
read or write is atomic.  (Platform is RHEL6/x86_64, kernel is
Red Hat 2.6.32-696.10.1.el6.x86_64.)

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] Red-Black tree traversal tests

2017-09-10 Thread Tom Lane
I wrote:
> In the meantime, here's my version.  Notable changes:

I went ahead and pushed this, with the removal of the preorder/postorder
code, so we can see if the buildfarm finds out anything interesting.
Feel free to continue to submit improvements though.

One thing that occurred to me is that as-is, this is entirely black-box
testing.  It doesn't try to check that the tree actually satisfies the
RB invariants, which is something that is interesting for performance
reasons.  (That is, the code could pass these tests even though it
produces an unbalanced tree with horrible performance.)  Is it worth
adding logic for that?  Not sure.  It's not like we are actively
changing the RB code or have reason to think it is buggy.

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] Setting pd_lower in GIN metapage

2017-09-10 Thread Tom Lane
Amit Kapila  writes:
> On Sun, Sep 10, 2017 at 11:52 AM, Michael Paquier
>  wrote:
>> Coordinating efforts here would be nice. If you, Amit K, are taking
>> care of a patch for btree and hash

> I think here we should first agree on what we want to do.  Based on
> Tom's comment, I was thinking of changing comments in btree/hash part
> and additionally for hash indexes, I can see if we can pass
> REGBUF_STANDARD for all usages of metapage.  I am not sure if we want
> similar exercise for btree as well.

FWIW, now that we've noticed the discrepancy, I'm for using
REGBUF_STANDARD or equivalent for all metapage calls.  Even if it
saves no space, inconsistency is bad because it's confusing.  And
Michael is correct to point out that we can exploit this to
improve WAL consistency checking.

regards, tom lane


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


[HACKERS] Re: DROP SUBSCRIPTION hangs if sub is disabled in the same transaction

2017-09-10 Thread Noah Misch
On Thu, Sep 07, 2017 at 04:53:12AM +, Noah Misch wrote:
> On Wed, Sep 06, 2017 at 03:28:47PM +0900, Masahiko Sawada wrote:
> > On Mon, Sep 4, 2017 at 11:43 PM, Arseny Sher  wrote:
> > > Arseny Sher  writes:
> > >
> > >> Attached patch fixes this by stopping workers before RO drop, as
> > >> already done in case when we drop replication slot.
> > >
> > > Sorry, here is the patch.
> > >
> > 
> > I could reproduce this issue, it's a bug. Added this to the open item.
> > The cause of this is commit 7e174fa7 which make subscription ddls kill
> > the apply worker only when transaction commit. I didn't look the patch
> > deeply yet but I'm not sure the approach that kills apply worker
> > before commit would be good.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.
> 
> [1] 
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


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


Re: [HACKERS] Red-black trees: why would anyone want preorder or postorder traversal?

2017-09-10 Thread Alexander Korotkov
On Sun, Sep 10, 2017 at 3:25 AM, Tom Lane  wrote:

> In short, therefore, I propose we rip out the DirectWalk and InvertedWalk
> options along with their support code, and then drop the portions of
> test_rbtree that are needed to exercise them.  Any objections?
>

+1,
I don't see any point in leaving DirectWalk and InvertedWalk in RB-tree.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


Re: [HACKERS] log_destination=file

2017-09-10 Thread Dmitry Dolgov
> On 7 September 2017 at 15:46, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>
> It seems that for this particular workload it was about 20-25% slower.‚Äč

Hmm...looks like I provided misleading data, sorry. The numbers from
previous
email are correct and I'm able to reproduce them, but surprisingly for me
only
for one particular situation when `log_statement=all;
log_destination=stderr`
and stderr is sent to a console with a tmux session running in it (and
apparently
it has some impact on the performance, but I'm not sure how exactly). In all
other cases (when stderr is sent to a plain console, /dev/null, or we send
logs
to a file) numbers are different, and the difference between patch and
master
is quite less significant.

average latency:

clients patch   master

10  0.321   0.286

20  0.669   0.602

30  1.016   0.942

40  1.358   1.280

50  1.727   1.637


Re: [HACKERS] Setting pd_lower in GIN metapage

2017-09-10 Thread Michael Paquier
On Sun, Sep 10, 2017 at 3:28 PM, Amit Kapila  wrote:
> I think here we should first agree on what we want to do.  Based on
> Tom's comment, I was thinking of changing comments in btree/hash part
> and additionally for hash indexes, I can see if we can pass
> REGBUF_STANDARD for all usages of metapage.  I am not sure if we want
> similar exercise for btree as well.

Changing only comments for now looks like a good idea to me.
-- 
Michael


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


Re: [HACKERS] WIP: Separate log file for extension

2017-09-10 Thread Magnus Hagander
On Fri, Aug 25, 2017 at 12:12 AM, Antonin Houska  wrote:

> Attached is a draft patch to allow extension to write log messages to a
> separate file. It introduces a concept of a "log stream". The extension's
> shared library gets its stream assigned by calling this function from
> _PG_init()


> my_stream_id = get_log_stream("my_extension", _log_stream);
>

I like this idea in general.



> Then it's supposed to change some of its attributes
>
> adjust_log_stream_attr(>filename, "my_extension.log");
>

This, however, seems to be wrong.

The logfile name does not belong in the extension, it belongs in the
configuration file. I think the extension should set it's "stream id" or
whatever you want to call it, and then it should be possible to control in
postgresql.conf where that log is sent.

Also, what if this extension is loaded on demand in a session and not via
shared_preload_libraries? It looks like the syslogger only gets the list of
configured streams when it starts?

In short, I think the solution should be more generic, and not "just for
extensions".

I completely missed this thread when I did my quick-wip at
https://www.postgresql.org/message-id/flat/cabuevexztl0gorywm9s4tr_ft3fmjbraxqdxj+bqzjpvmru...@mail.gmail.com#cabuevexztl0gorywm9s4tr_ft3fmjbraxqdxj+bqzjpvmru...@mail.gmail.com
 -- some of the changes made were close enough that I got the two confused
:) Based on the feedback of that one, have you done any performance checks?


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


Re: [HACKERS] PoC plpgsql - possibility to force custom or generic plan

2017-09-10 Thread Pavel Stehule
2017-09-08 23:09 GMT+02:00 Tom Lane :

> Pavel Stehule  writes:
> > personally I prefer syntax without FOR keyword - because following
> keyword
> > must be reserved keyword
>
> > SET x = .., y = .. SELECT ... ;
>
> Nope.  Most of the statement-starting keywords are *not* fully reserved;
> they don't need to be as long as they lead off the statement.  But this
> proposal would break that.  We need to put FOR or IN or another
> already-fully-reserved keyword after the SET list, or something's going
> to bite us.
>

the possibility to control plan cache for any command via GUC outside
PLpgSQL can introduce lot of question.

What impact will be on PREPARE command and on EXECUTE command?

If we disable plan cache for EXECUTE, should we remove plan from plan
cache? ...

Can we have some diagnostic to get info if some command has cached plan or
not?

Regards

Pavel


>
> regards, tom lane
>


Re: [HACKERS] pgbench more operators & functions

2017-09-10 Thread Fabien COELHO



Here is a v13. No code changes, but TAP tests added to maintain pgbench
coverage to green.

Summary of patch contents: [...]



1. there are no any problem with compilation, patching
2. all tests passed
3. doc is ok

I'll mark this patch as ready for commiter


Ok. Thanks for the reviews.

--
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] Setting pd_lower in GIN metapage

2017-09-10 Thread Amit Kapila
On Sun, Sep 10, 2017 at 11:52 AM, Michael Paquier
 wrote:
> On Sun, Sep 10, 2017 at 3:15 PM, Amit Kapila  wrote:
>> On Sat, Sep 9, 2017 at 9:00 PM, Tom Lane  wrote:
>>> Michael Paquier  writes:
 On Fri, Sep 8, 2017 at 5:24 AM, Tom Lane  wrote:
> In short, this patch needs a significant rewrite, and more analysis than
> you've done so far on whether there's actually any benefit to be gained.
> It might not be worth messing with.
>>>
 I did some measurements of the compressibility of the GIN meta page,
 looking at its FPWs with and without wal_compression and you are
 right: there is no direct compressibility effect when setting pd_lower
 on the meta page. However, it seems to me that there is an argument
 still pleading on favor of this patch for wal_consistency_checking.
>>>
>>> I think that would be true if we did both my point 1 and 2, so that
>>> the wal replay functions could trust pd_lower to be sane in all cases.
>>> But really, if you have to touch all the places that write these
>>> metapages, you might as well mark them REGBUF_STANDARD while at it.
>>>
 The same comment ought to be mentioned for btree.
>>>
>>> Yeah, I was wondering if we ought not clean up btree/hash while at it.
>>> At the very least, their existing comments saying that it's inessential
>>> to set pd_lower could use some more detail about why or why not.
>>>
>>
>> +1.  I think we can even use REGBUF_STANDARD in the hash for metapage
>> where currently it is not used.  I can give a try to write a patch for
>> hash/btree part if you want.
>
> Coordinating efforts here would be nice. If you, Amit K, are taking
> care of a patch for btree and hash
>

I think here we should first agree on what we want to do.  Based on
Tom's comment, I was thinking of changing comments in btree/hash part
and additionally for hash indexes, I can see if we can pass
REGBUF_STANDARD for all usages of metapage.  I am not sure if we want
similar exercise for btree as well.


-- 
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] Setting pd_lower in GIN metapage

2017-09-10 Thread Michael Paquier
On Sun, Sep 10, 2017 at 3:15 PM, Amit Kapila  wrote:
> On Sat, Sep 9, 2017 at 9:00 PM, Tom Lane  wrote:
>> Michael Paquier  writes:
>>> On Fri, Sep 8, 2017 at 5:24 AM, Tom Lane  wrote:
 In short, this patch needs a significant rewrite, and more analysis than
 you've done so far on whether there's actually any benefit to be gained.
 It might not be worth messing with.
>>
>>> I did some measurements of the compressibility of the GIN meta page,
>>> looking at its FPWs with and without wal_compression and you are
>>> right: there is no direct compressibility effect when setting pd_lower
>>> on the meta page. However, it seems to me that there is an argument
>>> still pleading on favor of this patch for wal_consistency_checking.
>>
>> I think that would be true if we did both my point 1 and 2, so that
>> the wal replay functions could trust pd_lower to be sane in all cases.
>> But really, if you have to touch all the places that write these
>> metapages, you might as well mark them REGBUF_STANDARD while at it.
>>
>>> The same comment ought to be mentioned for btree.
>>
>> Yeah, I was wondering if we ought not clean up btree/hash while at it.
>> At the very least, their existing comments saying that it's inessential
>> to set pd_lower could use some more detail about why or why not.
>>
>
> +1.  I think we can even use REGBUF_STANDARD in the hash for metapage
> where currently it is not used.  I can give a try to write a patch for
> hash/btree part if you want.

Coordinating efforts here would be nice. If you, Amit K, are taking
care of a patch for btree and hash, would you, Amit L, write the part
for GIN, BRIN and SpGist? This needs a careful lookup as many code
paths need a lookup so it may take time. Please note that I don't mind
double-checking this part if you don't have enough room to do so.
-- 
Michael


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


Re: [HACKERS] Setting pd_lower in GIN metapage

2017-09-10 Thread Amit Kapila
On Sat, Sep 9, 2017 at 9:00 PM, Tom Lane  wrote:
> Michael Paquier  writes:
>> On Fri, Sep 8, 2017 at 5:24 AM, Tom Lane  wrote:
>>> In short, this patch needs a significant rewrite, and more analysis than
>>> you've done so far on whether there's actually any benefit to be gained.
>>> It might not be worth messing with.
>
>> I did some measurements of the compressibility of the GIN meta page,
>> looking at its FPWs with and without wal_compression and you are
>> right: there is no direct compressibility effect when setting pd_lower
>> on the meta page. However, it seems to me that there is an argument
>> still pleading on favor of this patch for wal_consistency_checking.
>
> I think that would be true if we did both my point 1 and 2, so that
> the wal replay functions could trust pd_lower to be sane in all cases.
> But really, if you have to touch all the places that write these
> metapages, you might as well mark them REGBUF_STANDARD while at it.
>
>> The same comment ought to be mentioned for btree.
>
> Yeah, I was wondering if we ought not clean up btree/hash while at it.
> At the very least, their existing comments saying that it's inessential
> to set pd_lower could use some more detail about why or why not.
>

+1.  I think we can even use REGBUF_STANDARD in the hash for metapage
where currently it is not used.  I can give a try to write a patch for
hash/btree part if you want.


-- 
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] Setting pd_lower in GIN metapage

2017-09-10 Thread Amit Kapila
On Fri, Sep 8, 2017 at 1:54 AM, Tom Lane  wrote:
> Michael Paquier  writes:
>> On Thu, Sep 7, 2017 at 2:40 PM, Amit Langote
>>  wrote:
>>> On 2017/09/07 13:09, Michael Paquier wrote:
 pd_lower should remain at 0 on pre-10 servers.
>
>>> Doesn't PageInit(), which is where any page gets initialized, has always
>>> set pd_lower to SizeOfPageHeaderData?
>
>> Yes, sorry. I had a mind slippery here..
>
> Indeed, which raises the question of how did any of this code work at all?
> Up to now, these pages have been initialized with pd_lower =
> SizeOfPageHeaderData, *not* zero.  Which means that the FPI code would
> have considered the metadata to be part of a valid "hole" and not stored
> it, if we'd ever told the FPI code that the metadata page was of standard
> format.  I've just looked through the code for these three index types and
> can't find any place where they do that --- for instance, they don't pass
> REGBUF_STANDARD to XLogRegisterBuffer when writing their metapages, and
> they pass page_std = false to log_newpage when using that for metapages.
> Good thing, because they'd be completely broken otherwise.
>
> This means that the premise of this patch is wrong.  Adjusting pd_lower,
> by itself, would accomplish precisely zip for WAL compression, because
> we'd still not be telling the WAL code to compress out the hole.
>
> To get any benefit, I think we'd need to do all of the following:
>
> 1. Initialize pd_lower correctly in the metapage init functions, as here.
>
> 2. Any place we are about to write the metapage, set its pd_lower to the
> correct value, in case it's an old index that doesn't have that set
> correctly.  Fortunately this is cheap enough that we might as well just
> do it unconditionally.
>
> 3. Adjust all the xlog calls to tell them the page is of standard format.
>
> Now, one advantage we'd get from this is that we could say confidently
> that any index metapage appearing in a WAL stream generated by v11 or
> later has got the right pd_lower; therefore we could dispense with
> checking for wrong pd_lower in the mask functions (unless they're used
> in some way I don't know about, which is surely possible).
>
> BTW, while nbtree correctly initializes pd_lower, it looks to me like it
> is not exploiting that, because it seems never to pass REGBUF_STANDARD for
> the metapage anyway.
>

During the creation of the index, it uses log_newpage to log metapage
and there it uses REGBUF_STANDARD.  So, there seems to be some use of
it.

-- 
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 more operators & functions

2017-09-10 Thread Pavel Stehule
Hi

2017-09-09 11:02 GMT+02:00 Fabien COELHO :

>
> Hello Pavel,
>
> Here is a v13. No code changes, but TAP tests added to maintain pgbench
> coverage to green.
>
>
> Summary of patch contents:
>
> This patch extends pgbench expressions syntax while keeping compatibility
> with SQL expressions.
>
> It adds support for NULL and BOOLEAN, as well as assorted logical,
> comparison and test operators (AND, <>, <=, IS NULL...).
>
> A CASE construct is provided which takes advantage of the added BOOLEAN.
>
> Integer and double functions and operators are also extended: bitwise
> operators (<< & ...), exp/ln, mod() as synonymous to % (matching pg).
>
> Added TAP tests maintain pgbench source coverage to green (if you ignore
> lexer & parser generated files...).
>
>
> Future plans include extending and synchronizing psql & pgbench variable
> and expression syntaxes:
>  - move expression parsing and evaluation in fe_utils,
>which would allow to
>  - extend psql with some \let i  cliend-side syntax
>(ISTM that extending the \set syntax cannot be upward compatible)
>and probably handle \let as a synonymous to \set in pgbench.
>  - allow \if  in psql instead of just \if 
>  - add \if ... support to pgbench
>  - maybe add TEXT type support to the expression engine, if useful
>  - maybe add :'var" and :"var" support to pgbench, if useful
>
> There are already patches in the queue for:
>  - testing whether a variable is defined in psql
>feature could eventually be added to pgbench as well
>  - adding \gset (& \cset) to pgbench to get output of possibly
>combined queries into variables, which can be used for making
>decisions later in the script.
>
> --
> Fabien.


1. there are no any problem with compilation, patching
2. all tests passed
3. doc is ok

I'll mark this patch as ready for commiter

Regards

Pavel