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

[Pavel Stehule]

> >
> > SET x = .., y = .. SELECT ... ;
>
> This seems pretty ugly from a syntax perspective.
>
> We already have 'SET LOCAL', which manages scope to the current
> transaction.  How about SET BLOCK which would set until you've left
> the current statement block?
>

This is reason why PRAGMA was designed - it can works on function, block,
or statement level. But only in block based PL languages.


>
> merlin
>

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

[Tom Lane]

Michael Paquier  writes:
> On Sat, Sep 9, 2017 at 11:32 AM, Tom Lane  wrote:
>> In a moment of idleness I tried to run the TAP tests on pademelon,
>> which is a mighty old and slow machine.

> How long did it take?

The last time I tried it, make check-world took about 3 hours IIRC.
But that was a few months ago, I suspect it's more now.  I re-launched
the run after diagnosing this failure; it's gotten past the recovery
tests (proving that this is a race condition not a hard failure) and
is somewhere in the contrib tests, about 2 hours in.

>> I'm not real sure if the appropriate answer to this is "we need to fix
>> RecursiveCopy" or "we need to fix the callers to not call RecursiveCopy
>> until the source directory is stable".  Thoughts?

> ... So making RecursiveCopy really looks
> like the best bet in the long term.

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.

>> (I do kinda wonder why we rolled our own RecursiveCopy; surely there's
>> a better implementation in CPAN?)

> This comes from here, which is something I got involved in:

> So the idea here is really to have:
> 1) Something compatible down to perl 5.8.
> 2) Something which is not external, which is where we could have used
> File::Copy::Recursive (only compatible from 5.12 if I recall
> correctly? I am too lazy to check now).

Hm.  Even if we don't want to use File::Copy::Recursive, maybe we should
look at it and see what (if anything) it does about source-tree changes.

regards, tom lane


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

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

[Michael Paquier]

On Sat, Sep 9, 2017 at 11:32 AM, Tom Lane  wrote:
> In a moment of idleness I tried to run the TAP tests on pademelon,
> which is a mighty old and slow machine.

How long did it take? Just wondering if that's actually the slowest
one or not to run the full set of recovery tests. This would be mighty
slow. Even hamster never detected this failure I think.

> It looks to me like the archiver removed 00010001.ready
> between where RecursiveCopy.pm checks that $srcpath is a regular file
> or directory (line 95) and where it rechecks whether it's a regular
> file (line 105).  Then the "-f" test on line 105 fails, allowing it to
> fall through to the its-a-directory path, and unsurprisingly the opendir
> at line 115 fails with the above symptom.
>
> In short, RecursiveCopy.pm is woefully unprepared for the idea that the
> source directory tree might be changing underneath it.

Yes, before 010_logical_decoding_timelines and _backup_fs were
introduced, we only used RecursiveCopy with a static source in
init_from_backup(), which represented no risks.

> I'm not real sure if the appropriate answer to this is "we need to fix
> RecursiveCopy" or "we need to fix the callers to not call RecursiveCopy
> until the source directory is stable".  Thoughts?

We could make RecursiveCopy smarter here. In backup_fs_hot, if for
example another segment switch happens between pg_start_backup() and
CopyRecursive, say with a low archive_timeout with a TAP test using
this configuration, then we would stay at the same point. Another
thing that we could actually use here is the fact that _backup_fs
assumes that archives must be enabled (a sanity check would be nice!),
so we could just exclude pg_wal from the FS-backup and avoid any
problems, at least for this test butthat won't save from any other
paths getting modified on disk. So making RecursiveCopy really looks
like the best bet in the long term.

> (I do kinda wonder why we rolled our own RecursiveCopy; surely there's
> a better implementation in CPAN?)

This comes from here, which is something I got involved in:
commit: 1caef31d9e550408d0cbc5788a422dcb69736df5
author: Alvaro Herrera 
date: Wed, 2 Dec 2015 18:46:16 -0300
Refactor Perl test code

So the idea here is really to have:
1) Something compatible down to perl 5.8.
2) Something which is not external, which is where we could have used
File::Copy::Recursive (only compatible from 5.12 if I recall
correctly? I am too lazy to check now). Note as well that a copy of
the whole directory is used for backups instead of tar format of
pg_basebackup because there is no easy support for tar files down to
5.8.
-- 
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: add --if-exists to pg_recvlogical

[Thomas Munro]

On Sat, Sep 2, 2017 at 5:22 AM, Peter Eisentraut
 wrote:
>   
> +  --if-exists
> +  
> +   
> +Do not error out when --drop-slot or
> --start are
> +specified and a slot with the specified name does not exist.
> +   
> +  
> + 
>
> I understand the --drop-slot part.  But I don't understand what it means
> to ignore a missing replication slot when running --start.

Also "--start" breaks the documentation build (missing
slash on the closing tag).

-- 
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] Cached plans and statement generalization

[Thomas Munro]

On Fri, May 26, 2017 at 3:54 AM, Konstantin Knizhnik
 wrote:
> Attached please find rebased version of the autoprepare patch based on Tom's
> proposal (perform analyze for tree with constant literals and then replace
> them with parameters).
> Also I submitted this patch for the Autum commitfest.

The patch didn't survive the Summer bitrotfest.  Could you please rebase it?

-- 
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] Refactor handling of database attributes between pg_dump and pg_dumpall

[Haribabu Kommi]

On Fri, Sep 8, 2017 at 10:24 AM, Thomas Munro  wrote:

> On Mon, Aug 21, 2017 at 4:35 PM, Haribabu Kommi
>  wrote:
> > On Tue, Aug 15, 2017 at 7:29 AM, Peter Eisentraut
> >  wrote:
> >> On 4/4/17 01:06, Haribabu Kommi wrote:
> >> > Both pg_dump and pg_upgrade tests are passed. Updated patch attached
> >> > I will add this patch to the next commitfest.
> >>
> >> This patch needs to be rebased for the upcoming commit fest.
> >
> > Thanks for checking. Rebased patch is attached.
>
> Hi Haribabu,
>
> This patch breaks the documentation build, possibly because of these empty
> tags:
>
> +--create option to dump <>ALTER ROLE IN
> DATABASE ... SET
>

Thanks for checking the patch.
Fixed patch is attached.


Regards,
Hari Babu
Fujitsu Australia


0001-pg_dump-and-pg_dumpall-database-handling-refactoring_v7.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] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Sat, Sep 9, 2017 at 3:05 AM, Robert Haas  wrote:

> On Fri, Sep 8, 2017 at 10:08 AM, Jeevan Ladhe
>  wrote:
> > Thanks Robert for taking care of this.
> > My V29 patch series[1] is based on this commit now.
>
> Committed 0001-0003, 0005 with assorted modifications, mostly
> cosmetic, but with some actual changes to describeOneTableDetails and
> ATExecAttachPartition and minor additions to the regression tests.
>
>
Thanks Robert!!

Re: [HACKERS] new function for tsquery creartion

[Thomas Munro]

On Thu, Jul 20, 2017 at 4:58 AM, Robert Haas  wrote:
> On Wed, Jul 19, 2017 at 12:43 PM, Victor Drobny  
> wrote:
>> Let me introduce new function for full text search query creation(which is
>> called 'queryto_tsquery'). It takes 'google like' query string and
>> translates it to tsquery.
>
> I haven't looked at the code, but that sounds like a neat idea.

+1

This is a very cool feature making tsquery much more accessible.  Many
people know that sort of defacto search engine query language that
many websites accept using quotes, AND, OR, - etc.

Calling this search syntax just "query" seems too general and
overloaded.  "Simple search", "simple query", "web search", "web
syntax", "web query", "Google-style query", "Poogle" (kidding!) ...
well I'm not sure, but I feel like it deserves a proper name.
websearch_to_tsquery()?

I see that your AROUND(n) is an undocumented Google search syntax.
That's a good trick to know.

Please send a rebased version of the patch for people to review and
test as that one has bit-rotted.

-- 
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] [POC] Faster processing at Gather node

[Amit Kapila]

On Fri, Sep 8, 2017 at 11:07 PM, Alexander Kuzmenkov
 wrote:
> Hi Rafia,
>
> I like the idea of reducing locking overhead by sending tuples in bulk. The
> implementation could probably be simpler: you could extend the API of shm_mq
> to decouple notifying the sender from actually putting data into the queue
> (i.e., make shm_mq_notify_receiver public and make a variant of shm_mq_sendv
> that doesn't send the notification).
>

Rafia can comment on details, but I would like to bring it to your
notice that we need kind of local buffer (queue) for gathermerge
processing as well where the data needs to be fetched in order from
queues.  So, there is always a chance that some of the workers have
filled their queues while waiting for the master to extract the data.
I think the patch posted by Rafia on the nearby thread [1] addresses
both the problems by one patch.


[1] - 
https://www.postgresql.org/message-id/CAOGQiiNiMhq5Pg3LiYxjfi2B9eAQ_q5YjS%3DfHiBJmbSOF74aBQ%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

[HACKERS] Still another race condition in recovery TAP tests

[Tom Lane]

In a moment of idleness I tried to run the TAP tests on pademelon,
which is a mighty old and slow machine.  Behold,
src/test/recovery/t/010_logical_decoding_timelines.pl fell over,
with the relevant section of its log contents being:

# testing logical timeline following with a filesystem-level copy
# Taking filesystem backup b1 from node "master"
# pg_start_backup: 0/228
could not 
opendir(/home/postgres/pgsql/src/test/recovery/tmp_check/t_010_logical_decoding_timelines_master_data/pgdata/pg_wal/archive_status/00010001.ready):
 No such file or directory at ../../../src/test/perl//RecursiveCopy.pm line 115.
### Stopping node "master" using mode immediate

The postmaster log has this relevant entry:

2017-09-08 22:03:22.917 EDT [19160] DEBUG:  archived write-ahead log file 
"00010001"

It looks to me like the archiver removed 00010001.ready
between where RecursiveCopy.pm checks that $srcpath is a regular file
or directory (line 95) and where it rechecks whether it's a regular
file (line 105).  Then the "-f" test on line 105 fails, allowing it to
fall through to the its-a-directory path, and unsurprisingly the opendir
at line 115 fails with the above symptom.

In short, RecursiveCopy.pm is woefully unprepared for the idea that the
source directory tree might be changing underneath it.

I'm not real sure if the appropriate answer to this is "we need to fix
RecursiveCopy" or "we need to fix the callers to not call RecursiveCopy
until the source directory is stable".  Thoughts?

(I do kinda wonder why we rolled our own RecursiveCopy; surely there's
a better implementation in CPAN?)

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] Parallel worker error

[Amit Kapila]

On Fri, Sep 8, 2017 at 3:13 PM, Robert Haas  wrote:
> On Fri, Sep 8, 2017 at 1:17 AM, Amit Kapila  wrote:
>> You are right.  I have changed the ordering and passed OuterUserId via
>> FixedParallelState.
>
> This looks a little strange:
>
> +SetCurrentRoleId(fps->outer_user_id, fps->is_current_user_superuser);
>
> The first argument says "outer" but the second says "current".  I'm
> wondering if we can just make the second one is_superuser.
>

No issues changed as per suggestion.

> I'm also wondering if, rather than using GetConfigOptionByName, we
> should just make the GUC underlying is_superuser non-static and use
> the value directly.  If not, then I'm alternatively wondering whether
> we should maybe use a less-generic name than varval.
>

I think we can go either way.  So prepared patches with both
approaches. In fix_role_handling_parallel_worker_v3_1.patch, I have
changed the variable name and in
fix_role_handling_parallel_worker_v3_2.patch, I have exposed the guc
is_superuser.



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


fix_role_handling_parallel_worker_v3_1.patch
Description: Binary data


fix_role_handling_parallel_worker_v3_2.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] More flexible LDAP auth search filters?

[Thomas Munro]

On Sat, Sep 9, 2017 at 3:33 AM, Peter Eisentraut
 wrote:
> A couple of comments on this patch.  I have attached a "fixup" patch on
> top of your v4 that should address them.
>
> - I think the bracketing of the LDAP URL synopsis is wrong.

+1

> - I have dropped the sentence that LDAP URL extensions are not
> supported.  That sentence was written mainly to point out that filters
> are not supported, which they are now.  There is nothing beyond filters
> and extensions left in LDAP URLs, AFAIK.

+1

> - We have previously used the ldapsearchattribute a bit strangely.  We
> use it as both the search filter and as the attribute to return from the
> search, but we don't actually care about the returned attribute (we only
> use the DN (which is not a real attribute)).  That hasn't been a real
> problem, but now if you specify a filter, as the code stands it will
> always request the "uid" attribute, which won't work if there is no such
> attribute.  I have found that there is an official way to request "no
> attribute", so I have fixed the code to do that.

Ah.  Good.

> - I was wondering whether we need a way to escape the % (%%?) but it
> doesn't seem worth bothering.
>
> I have been looking around the Internet how this functionality compares
> to other LDAP authentication systems.
>
> I didn't see the origin of the %u idea in this thread, but perhaps it
> came from Dovecot.  But there it's a general configuration file syntax,
> nothing specific to LDAP.  In nginx you use %(username), which again is
> general configuration file syntax.  I'm OK with using %u.
>
> The original LDAP URL design was adapted from Apache
> (https://httpd.apache.org/docs/2.4/mod/mod_authnz_ldap.html#authldapurl).
>  They combine the attribute filter and the general filter if both are
> specified, but I think they got that a bit wrong.  The attribute field
> in the URL is actually not supposed to be a filter but a return field,
> which is also the confusion mentioned above.
>
> The PAM module (https://linux.die.net/man/5/pam_ldap) also has a way to
> specify a search attribute and a general filter and combines them.
>
> Neither of these allows substituting the user name into the search filter.
>
> I think there would be a case to be made to allow the searchattribute
> and the searchfilter both be specified.  One typical use would be to use
> the first one to locate the user and the second one to "filter" out
> certain things, which I think is the approach the PAM and Apache modules
> take.  This wouldn't work well, however, if you use the %u placeholder,
> because then you'd have to explicitly unset ldapsearchattribute, which
> would be annoying.  So maybe not.

I like this way, because it doesn't leave the user wondering how the
filter is constructed.  It's either the user's filter using %u
placeholders or a system-built one, but not a combination where you
have to wonder whether it's an implicit AND, OR or something else...

> Please check my patch.  I think it's ready to go then.

+1 from me to all your changes except this one:

- buffer_size += user_name_size;
+ buffer_size += user_name_size - 2;

The algorithm is: start with buffer_size = 0; add user_name_size if
you see "%u" but otherwise just add one per character; finally add one
for the terminator.  There is no reason to subtract 2, since we didn't
count the "%u" characters.  Consider a username of "x" and a search
filter of "%u": your change would underflow buffer_size.

Here's a patch with your fixup squashed (except for the above-noted thing).

Thanks for all your work on this.

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


ldap-search-filters-v5.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] More flexible LDAP auth search filters?

[Thomas Munro]

On Sat, Sep 9, 2017 at 3:36 AM, Peter Eisentraut
 wrote:
> For additional entertainment I have written a test suite for this LDAP
> authentication functionality.  It's not quite robust enough to be run by
> default, because it needs a full OpenLDAP installation, but it's been
> very helpful for reviewing this patch.  Here it is.

Very nice!

+if ($^O eq 'darwin')
+{
+   $slapd = '/usr/local/opt/openldap/libexec/slapd';
+   $ldap_schema_dir = '/usr/local/etc/openldap/schema';
+}

I'm guessing this is the MacPorts location, and someone from that
other tribe that uses Brew can eventually post a patch to make this
look in more places.

+my $ldap_port = int(rand() * 16384) + 49152;

Hmm.  I guess ldapi (Unix domain sockets) would be less roulette-like,
but require client side support too.

Here's a change I needed to make to run this here.  It seems that to
use "database mdb" I'd need to add a config line to tell it the path
to load back_mdb.so from.  I could have done, but I noticed that if I
tell it to use raw ldif files instead it's happy.  Does this still
work for you on the systems you tested?

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


0001-fixup-Add-LDAP-authentication-test-suite.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] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Fri, Sep 8, 2017 at 1:38 PM, Ashutosh Bapat
 wrote:
>> I also confirmed that the partition-pruning patch set works fine with this
>> patch instead of the patch on that thread with the same functionality,
>> which I will now drop from that patch set.  Sorry about the wasted time.
>
> Thanks a lot. Please review the patch in the updated patchset.

In set_append_rel_size(), I don't find the comment too clear (and this
part was taken from Amit's patch, right?).  I suggest:

+/*
+ * Associate the partitioned tables which are descendents of the table
+ * named in the query with the topmost append path (i.e. the one where
+ * rel->reloptkind is RELOPT_BASEREL).  This ensures that they get properly
+ * locked at execution time.
+ */

I'm a bit suspicious about the fact that there are now executor
changes related to the PlanRowMarks.  If the rowmark's prti is now the
intermediate parent's RT index rather than the top-parent's RT index,
it'd seem like that'd matter somehow.  Maybe it doesn't, because the
code that cares about prti seems to only care about whether it's
different from rti.  But if that's true everywhere, then why even
change this?  I think we might be well off not to tinker with things
that don't need to be changed.

Apart from that concern, now that I understand (from my own failed
attempt and some off-list discussion) why this patch works the way it
does, I think this is in fairly good shape.

-- 
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] psql: new help related to variables are not too readable

[Tom Lane]

Tomas Vondra  writes:
> The translator has exactly the same context in both cases, and the
> struggle to wrap it at 80 characters will be pretty much the same too.

Really?  With the old way, you had something under 60 characters to
work in, now it's nearly 80.  I don't buy that that's not a significant
difference.  It's also much less ugly if you decide you need one more
line than the English version uses.

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] psql: new help related to variables are not too readable

[Tomas Vondra]

Hi,

On 09/08/2017 07:25 AM, Fabien COELHO wrote:
> 
> Hello,
> 
>>>   PSQL_HISTORY   alternative location for the command history file
>>>
>>> I would prefer to revert to that more compact 9.6-formatting.
>>
>> There was a problem with line width .. its hard to respect 80 chars
> 
> Yes.
> 
> Scrolling in two dimensions because it does not fit either way is not
> too practical, so the idea was the it should at least fit a reasonable
> terminal in the horizontal dimension, the vertical one having been
> unfittable anyway for a long time.
> 
> Once you want to do strict 80 columns, a lot of/most descriptions do not
> fit and need to be split somehow on two lines, one way or another. It
> seemed that
> 
>   XXX
>    xxx xx xxx xxx 
> 
> Is a good way to do that systematically and with giving more space and
> chance for a description to fit in its line. ISTM that it was already
> done like for environment variables, so it is also for homogeneity.
> 

FWIW I also find the new formatting hard to read, as it does not clearly
separate the items. The old formatting was not ideal either, though.

>
> It also simplify work for translators that can just follow the same
> formatting and know what to do if a one line English explanation does
> not fit once translated.
> 

As someone responsible for a significant part of the Czech translation,
I find this argument somewhat dubious. I don't see how this

  _(" "
"  ")

simplifies the work for translators, compared to this

  _(" ")

The translator has exactly the same context in both cases, and the
struggle to wrap it at 80 characters will be pretty much the same too.

The thing that would make a difference is automatic wrapping, i.e.
split on spaces, then re-build into shorter than 80 characters ...

> Finally, as vertical scrolling is mandatory, I would be fine with
> skipping lines with entries for readability, but it is just a matter of
> taste and I expect there should be half a dozen different opinions on
> the matter of formatting.
> 

FWIW, +1 to extra lines from me - I find it way more readable, as it
clearly separates the items. You're right this is also a matter of taste
to some degree, so I understand Erik's point about vertical scrolling.

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] SAP Application deployment on PostgreSQL

[Christopher Browne]

On 8 September 2017 at 15:34, chiru r  wrote:
> We have multiple SAP applications running on Oracle as backend and looking
> for an opportunity to migrate from Oracle to PostgreSQL. Has anyone ever
> deployed SAP on PostgreSQL community edition?
>
> Is PostgreSQL community involved in any future road-map of SAP application
> deployment on PostgreSQL?
>
> Thanks
> chiru

This has been asked about on SAP's forum, and that's the most appropriate
place, in that their applications are very much database-specific.

https://archive.sap.com/discussions/thread/1941255

I imagine that it would be a "broadly interesting" idea to run R/3
against PostgreSQL,
but, as observed in the discussion thread, the "SAP kernel" is very much NOT
database-agnostic.  The work that would need to be done to do so would require
considerable work on the part of SAP AG, and I'd be somewhat surprised to see
them do it.

Recall that once upon a time, SAP AG acquired the sources for ADABAS-D,
renamed it SAP-DB, and, for a while, made it available as "open source."  At
the time, it appeared that this was some sort of corporate gamesmanship
relating to a vendor selling what one might call "Product O".

SAP AG presumably spent a fair bit of effort (and money) establishing that
port for some of their products.  (I imagine that a port to run R/3 on
PostgreSQL
might be easier/simpler than running it on SAP-DB, but that's just me
imagining...)  They subsequently drew the code back to be proprietary, and
have released several versions of MaxDB since.

I'd be curious as to reasons to expect that SAP AG would want to do a PostgreSQL
port.  (No doubt other ex-BASIS consultants would also be interested!)
-- 
When confronted by a difficult problem, solve it by reducing it to the
question, "How would the Lone Ranger handle this?"


-- 
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] Adding support for Default partition in partitioning

[Robert Haas]

On Fri, Sep 8, 2017 at 10:08 AM, Jeevan Ladhe
 wrote:
> Thanks Robert for taking care of this.
> My V29 patch series[1] is based on this commit now.

Committed 0001-0003, 0005 with assorted modifications, mostly
cosmetic, but with some actual changes to describeOneTableDetails and
ATExecAttachPartition and minor additions to the regression tests.

-- 
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 plpgsql - possibility to force custom or generic plan

[David G. Johnston]

On Fri, Sep 8, 2017 at 2:09 PM, Tom Lane  wrote:

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

Just throwing it ​out there but can we making putting SET inside a CTE work?

David J.

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

[Tom Lane]

Merlin Moncure  writes:
> We already have 'SET LOCAL', which manages scope to the current
> transaction.  How about SET BLOCK which would set until you've left
> the current statement block?

(1) I do not think this approach will play terribly well in any of the
PLs; their notions of statement blocks all differ from whatever we might
think that is at the interactive-SQL-command level.

(2) AIUI the feature request is specifically for a single-statement
variable change, not any larger scope than that.

regards, tom lane


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

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

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

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] tupconvert.c API change in v10 release notes

[Tom Lane]

Bruce Momjian  writes:
> On Wed, Jul 19, 2017 at 12:39:07PM -0400, Tom Lane wrote:
>> Yeah, I see nothing about 3f902354b in release-10.sgml either.
>> We've had varying policies over the years about whether to mention
>> internal API changes in the release notes or not, but this one
>> I think does belong there, since it's a silent API break rather
>> than one that would easily be caught due to compiler errors.
>> Bruce, did you have any specific reasoning for leaving it out?

> I doubt I saw that sentence in the paragraph.  For long text like that,
> I am usually looking for "BACKWARDS INCOMPATIBLE CHANGE" or something
> like that.  Sorry I missed it.

I added something about this to the notes.

regards, tom lane


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

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

[Merlin Moncure]

On Fri, Sep 8, 2017 at 2:48 PM, Pavel Stehule  wrote:
>
>
> 2017-09-08 21:21 GMT+02:00 Daniel Gustafsson :
>>
>> > On 08 Sep 2017, at 19:14, Simon Riggs  wrote:
>> >
>> > On 6 September 2017 at 07:43, Robert Haas  wrote:
>> >
>> >> LET custom_plan_tries = 0 IN SELECT ...
>> >
>> > Tom has pointed me at this proposal, since on another thread I asked
>> > for something very similar. (No need to reprise that discussion, but I
>> > wanted prepared queries to be able to do SET work_mem = X; SELECT).
>> > This idea looks a good way forward to me.
>> >
>> > Since we're all in roughly the same place, I'd like to propose that we
>> > proceed with the following syntax... whether or not this precisely
>> > solves OP's issue on this thread.
>> >
>> > 1. Allow SET to set multiple parameters...
>> > SET guc1 = x, guc2 = y
>> > This looks fairly straightforward
>> >
>> > 2. Allow a SET to apply only for a single statement
>> > SET guc1 = x, guc2 = y FOR stmt
>> > e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
>> > Internally a GUC setting already exists for a single use, via
>> > GUC_ACTION_SAVE, so we just need to invoke it.
>>
>> This syntax proposal makes sense, +1.  My immediate thought was that the
>> per-statement GUCs were sort of like options, and most options in our
>> syntax
>> are enclosed with (), like: SET (guc1 = x, guc2 = y) FOR SELECT ..;
>
> we newer support this syntax in combination with SET keyword
>
> see - CREATE FUNCTION command
>
> personally I prefer syntax without FOR keyword - because following keyword
> must be reserved keyword
>
> SET x = .., y = .. SELECT ... ;

This seems pretty ugly from a syntax perspective.

We already have 'SET LOCAL', which manages scope to the current
transaction.  How about SET BLOCK which would set until you've left
the current statement block?

merlin


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

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

[Pavel Stehule]

2017-09-08 21:21 GMT+02:00 Daniel Gustafsson :

> > On 08 Sep 2017, at 19:14, Simon Riggs  wrote:
> >
> > On 6 September 2017 at 07:43, Robert Haas  wrote:
> >
> >> LET custom_plan_tries = 0 IN SELECT ...
> >
> > Tom has pointed me at this proposal, since on another thread I asked
> > for something very similar. (No need to reprise that discussion, but I
> > wanted prepared queries to be able to do SET work_mem = X; SELECT).
> > This idea looks a good way forward to me.
> >
> > Since we're all in roughly the same place, I'd like to propose that we
> > proceed with the following syntax... whether or not this precisely
> > solves OP's issue on this thread.
> >
> > 1. Allow SET to set multiple parameters...
> > SET guc1 = x, guc2 = y
> > This looks fairly straightforward
> >
> > 2. Allow a SET to apply only for a single statement
> > SET guc1 = x, guc2 = y FOR stmt
> > e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
> > Internally a GUC setting already exists for a single use, via
> > GUC_ACTION_SAVE, so we just need to invoke it.
>
> This syntax proposal makes sense, +1.  My immediate thought was that the
> per-statement GUCs were sort of like options, and most options in our
> syntax
> are enclosed with (), like: SET (guc1 = x, guc2 = y) FOR SELECT ..;
>

we newer support this syntax in combination with SET keyword

see - CREATE FUNCTION command

personally I prefer syntax without FOR keyword - because following keyword
must be reserved keyword

SET x = .., y = .. SELECT ... ;

Regards

Pavel


> cheers ./daniel
>

Re: [HACKERS] Fix performance degradation of contended LWLock on NUMA

[Sokolov Yura]

Hi, Jesper

Thank you for reviewing.

On 2017-09-08 18:33, Jesper Pedersen wrote:

Hi,

On 07/18/2017 01:20 PM, Sokolov Yura wrote:

I'm sending rebased version with couple of one-line tweaks.
(less skip_wait_list on shared lock, and don't update spin-stat on 
aquiring)




I have been running this patch on a 2S/28C/56T/256Gb w/ 2 x RAID10 SSD
setup (1 to 375 clients on logged tables).

I'm seeing

-M prepared: Up to 11% improvement
-M prepared -S: No improvement, no regression ("noise")
-M prepared -N: Up to 12% improvement

for all runs the improvement shows up the closer you get to the number
of CPU threads, or above. Although I'm not seeing the same
improvements as you on very large client counts there are definitely
improvements :)


It is expected:
- patch "fixes NUMA": for example, it doesn't give improvement on 1 
socket

  at all (I've tested it using numactl to bind to 1 socket)
- and certainly it gives less improvement on 2 sockets than on 4 sockets
  (and 28 cores vs 72 cores also gives difference),
- one of hot points were CLogControlLock, and it were fixed with
  "Group mode for CLOG updates" [1]



Some comments:
==

lwlock.c:
-

...

+static void
+add_lwlock_stats_spin_stat(LWLock *lock, SpinDelayStatus *delayStatus)

Add a method description.


Neighbor functions above also has no description, that is why I didn't 
add.

But ok, I've added now.



+static inline bool
+LWLockAttemptLockOrQueueSelf(LWLock *lock, LWLockMode mode,
LWLockMode waitmode)

I'll leave it to the Committer to decide if this method is too big to
be "inline".


GCC 4.9 doesn't want to inline it without directive, and function call
is then remarkable in profile.

Attach contains version with all suggestions applied except remove of
"inline".


Open questions:
---
* spins_per_delay as extern
* Calculation of skip_wait_list


Currently calculation of skip_wait_list is mostly empirical (ie best
i measured).

I strongly think that instead of spins_per_delay something dependent
on concrete lock should be used. I tried to store it in a LWLock
itself, but it were worse.
Recently I understand it should be stored in array indexed by tranche,
but I didn't implement it yet, and therefore didn't measure.

[1]: https://commitfest.postgresql.org/14/358/

--
With regards,
Sokolov Yura aka funny_falcon
Postgres Professional: https://postgrespro.ru
The Russian Postgres CompanyFrom 722a8bed17f82738135264212dde7170b8c0f397 Mon Sep 17 00:00:00 2001
From: Sokolov Yura 
Date: Mon, 29 May 2017 09:25:41 +
Subject: [PATCH 1/6] More correct use of spinlock inside LWLockWaitListLock.

SpinDelayStatus should be function global to count iterations correctly,
and produce more correct delays.

Also if spin didn't spin, do not count it in spins_per_delay correction.
It wasn't necessary before cause delayStatus were used only in contented
cases.
---
 src/backend/storage/lmgr/lwlock.c | 37 +
 src/backend/storage/lmgr/s_lock.c |  5 +
 2 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/src/backend/storage/lmgr/lwlock.c b/src/backend/storage/lmgr/lwlock.c
index 82a1cf5150..7714085663 100644
--- a/src/backend/storage/lmgr/lwlock.c
+++ b/src/backend/storage/lmgr/lwlock.c
@@ -323,6 +323,16 @@ get_lwlock_stats_entry(LWLock *lock)
 	}
 	return lwstats;
 }
+
+/* just shortcut to not declare lwlock_stats variable at the end of function */
+static void
+add_lwlock_stats_spin_stat(LWLock *lock, SpinDelayStatus *delayStatus)
+{
+	lwlock_stats *lwstats;
+
+	lwstats = get_lwlock_stats_entry(lock);
+	lwstats->spin_delay_count += delayStatus->delays;
+}
 #endif			/* LWLOCK_STATS */
 
 
@@ -800,13 +810,9 @@ static void
 LWLockWaitListLock(LWLock *lock)
 {
 	uint32		old_state;
-#ifdef LWLOCK_STATS
-	lwlock_stats *lwstats;
-	uint32		delays = 0;
-
-	lwstats = get_lwlock_stats_entry(lock);
-#endif
+	SpinDelayStatus delayStatus;
 
+	init_local_spin_delay();
 	while (true)
 	{
 		/* always try once to acquire lock directly */
@@ -815,20 +821,10 @@ LWLockWaitListLock(LWLock *lock)
 			break;/* got lock */
 
 		/* and then spin without atomic operations until lock is released */
+		while (old_state & LW_FLAG_LOCKED)
 		{
-			SpinDelayStatus delayStatus;
-
-			init_local_spin_delay();
-
-			while (old_state & LW_FLAG_LOCKED)
-			{
-perform_spin_delay();
-old_state = pg_atomic_read_u32(>state);
-			}
-#ifdef LWLOCK_STATS
-			delays += delayStatus.delays;
-#endif
-			finish_spin_delay();
+			perform_spin_delay();
+			old_state = pg_atomic_read_u32(>state);
 		}
 
 		/*
@@ -836,9 +832,10 @@ LWLockWaitListLock(LWLock *lock)
 		 * we're attempting to get it again.
 		 */
 	}
+	finish_spin_delay();
 
 #ifdef LWLOCK_STATS
-	lwstats->spin_delay_count += delays;
+	add_lwlock_stats_spin_stat(lock, );
 #endif
 }
 
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 40d8156331..b75c432773 100644
--- 

[HACKERS] SAP Application deployment on PostgreSQL

[chiru r]

Hi All,

We have multiple SAP applications running on Oracle as backend and looking
for an opportunity to migrate from Oracle to PostgreSQL. Has anyone ever
deployed SAP on PostgreSQL community edition?

Is PostgreSQL community involved in any future road-map of SAP application
deployment on PostgreSQL?

Thanks
chiru

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

[Daniel Gustafsson]

> On 08 Sep 2017, at 19:14, Simon Riggs  wrote:
> 
> On 6 September 2017 at 07:43, Robert Haas  wrote:
> 
>> LET custom_plan_tries = 0 IN SELECT ...
> 
> Tom has pointed me at this proposal, since on another thread I asked
> for something very similar. (No need to reprise that discussion, but I
> wanted prepared queries to be able to do SET work_mem = X; SELECT).
> This idea looks a good way forward to me.
> 
> Since we're all in roughly the same place, I'd like to propose that we
> proceed with the following syntax... whether or not this precisely
> solves OP's issue on this thread.
> 
> 1. Allow SET to set multiple parameters...
> SET guc1 = x, guc2 = y
> This looks fairly straightforward
> 
> 2. Allow a SET to apply only for a single statement
> SET guc1 = x, guc2 = y FOR stmt
> e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
> Internally a GUC setting already exists for a single use, via
> GUC_ACTION_SAVE, so we just need to invoke it.

This syntax proposal makes sense, +1.  My immediate thought was that the
per-statement GUCs were sort of like options, and most options in our syntax
are enclosed with (), like: SET (guc1 = x, guc2 = y) FOR SELECT ..;

cheers ./daniel


-- 
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] Savepoint-related statements terminates connection

[Tom Lane]

Catalin Iacob  writes:
> When reading this I also realized that the backend does send responses for
> every individual query in a multi-query request, it's only libpq's PQexec
> that throws away the intermediate results and only provides access to the
> last one.

If you want to see them all, you can use PQsendQuery/PQgetResult.

https://www.postgresql.org/docs/current/static/libpq-async.html

There's a case to be made that we should change psql to use these
and print all the results not just the last one.  I've not looked
to see how much work that would be; but now that we're actually
documenting how to script multi-command queries, it might be
a good idea to fix it before too many people have scripts that
rely on the current behavior.

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] [POC] Faster processing at Gather node

[Alexander Kuzmenkov]

Hi Rafia,

I like the idea of reducing locking overhead by sending tuples in bulk. 
The implementation could probably be simpler: you could extend the API 
of shm_mq to decouple notifying the sender from actually putting data 
into the queue (i.e., make shm_mq_notify_receiver public and make a 
variant of shm_mq_sendv that doesn't send the notification). From Amit's 
letter I understand that you have already tried something along these 
lines and the performance wasn't good. What was the bottleneck then? If 
it's the locking around mq_bytes_read/written, it can be rewritten with 
atomics. I think it would be great to try this approach because it 
doesn't add much code, doesn't add any additional copying and improves 
shm_mq performance in general.


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



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

[HACKERS] Re: [COMMITTERS] pgsql: Fix assorted portability issues in new pgbench TAP tests.

[Fabien COELHO]

Hello,

Please find attached "blind" additional fixes for Windows & AIX.

 - more nan/inf variants
 - different message on non existing user
 - illegal vs unrecognized options

I suspect that $windows_os is not true on "bowerbird", in order to fix it 
the value of "$Config{osname}" is needed...


--
Fabien.diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 66df4bc..54a6039 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -73,7 +73,11 @@ pgbench(
 	1,
 	[qr{^$}],
 	[   qr{connection to database "template0" failed},
-		qr{FATAL:  role "no-such-user" does not exist} ],
+	# FATAL:  role "no-such-user" does not exist
+	# FATAL:  SSPI authentication failed for user "no-such-user"
+	qr{FATAL:.* (role|user) "no-such-user"},
+	qr{FATAL:.* (does not exist|authentication failed)}
+	],
 	'no such user');
 
 pgbench(
@@ -217,9 +221,9 @@ pgbench(
 		qr{command=18.: double 18\b},
 		qr{command=19.: double 19\b},
 		qr{command=20.: double 20\b},
-		qr{command=21.: double -?nan}i,
-		qr{command=22.: double inf}i,
-		qr{command=23.: double -inf}i,
+		qr{command=21.: double (-?nan|-1\.#IND)}i,
+		qr{command=22.: double (inf|1\.#INF)}i,
+		qr{command=23.: double (-inf|-1\.#INF)}i,
 		qr{command=24.: int 9223372036854775807\b}, ],
 	'pgbench expressions',
 	{   '001_pgbench_expressions' => q{-- integer functions
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index 631aa73..d6b3d4f 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -26,7 +26,7 @@ my @options = (
 	# name, options, stderr checks
 	[   'bad option',
 		'-h home -p 5432 -U calvin -d --bad-option',
-		[ qr{unrecognized option}, qr{--help.*more information} ] ],
+		[ qr{(unrecognized|illegal) option}, qr{--help.*more information} ] ],
 	[   'no file',
 		'-f no-such-file',
 		[qr{could not open file "no-such-file":}] ],

-- 
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] additional contrib test suites

[Peter Eisentraut]

On 9/6/17 07:11, Thomas Munro wrote:
> After applying these patches cleanly on top of
> 0b554e4e63a4ba4852c01951311713e23acdae02 and running "./configure
> --enable-tap-tests --with-tcl --with-python --with-perl --with-ldap
> --with-icu && make && make check-world" I saw this failure:

Yes, some of the error messages had changed.  Fixed patches attached.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From bd790a0125729edd44e3d70fba325d3bfcf6da94 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 11 Aug 2017 21:04:04 -0400
Subject: [PATCH v2 1/7] adminpack: Add test suite

---
 contrib/adminpack/.gitignore |   4 +
 contrib/adminpack/Makefile   |   2 +
 contrib/adminpack/expected/adminpack.out | 144 +++
 contrib/adminpack/sql/adminpack.sql  |  56 
 4 files changed, 206 insertions(+)
 create mode 100644 contrib/adminpack/.gitignore
 create mode 100644 contrib/adminpack/expected/adminpack.out
 create mode 100644 contrib/adminpack/sql/adminpack.sql

diff --git a/contrib/adminpack/.gitignore b/contrib/adminpack/.gitignore
new file mode 100644
index 00..5dcb3ff972
--- /dev/null
+++ b/contrib/adminpack/.gitignore
@@ -0,0 +1,4 @@
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
diff --git a/contrib/adminpack/Makefile b/contrib/adminpack/Makefile
index f065f84bfb..89c249bc0d 100644
--- a/contrib/adminpack/Makefile
+++ b/contrib/adminpack/Makefile
@@ -8,6 +8,8 @@ EXTENSION = adminpack
 DATA = adminpack--1.0.sql
 PGFILEDESC = "adminpack - support functions for pgAdmin"
 
+REGRESS = adminpack
+
 ifdef USE_PGXS
 PG_CONFIG = pg_config
 PGXS := $(shell $(PG_CONFIG) --pgxs)
diff --git a/contrib/adminpack/expected/adminpack.out 
b/contrib/adminpack/expected/adminpack.out
new file mode 100644
index 00..83cbb741da
--- /dev/null
+++ b/contrib/adminpack/expected/adminpack.out
@@ -0,0 +1,144 @@
+CREATE EXTENSION adminpack;
+-- create new file
+SELECT pg_file_write('test_file1', 'test1', false);
+ pg_file_write 
+---
+ 5
+(1 row)
+
+SELECT pg_read_file('test_file1');
+ pg_read_file 
+--
+ test1
+(1 row)
+
+-- append
+SELECT pg_file_write('test_file1', 'test1', true);
+ pg_file_write 
+---
+ 5
+(1 row)
+
+SELECT pg_read_file('test_file1');
+ pg_read_file 
+--
+ test1test1
+(1 row)
+
+-- error, already exists
+SELECT pg_file_write('test_file1', 'test1', false);
+ERROR:  file "test_file1" exists
+SELECT pg_read_file('test_file1');
+ pg_read_file 
+--
+ test1test1
+(1 row)
+
+-- disallowed file paths
+SELECT pg_file_write('../test_file0', 'test0', false);
+ERROR:  path must be in or below the current directory
+SELECT pg_file_write('/tmp/test_file0', 'test0', false);
+ERROR:  absolute path not allowed
+SELECT pg_file_write(current_setting('data_directory') || '/test_file4', 
'test4', false);
+ pg_file_write 
+---
+ 5
+(1 row)
+
+SELECT pg_file_write(current_setting('data_directory') || '/../test_file4', 
'test4', false);
+ERROR:  reference to parent directory ("..") not allowed
+-- rename file
+SELECT pg_file_rename('test_file1', 'test_file2');
+ pg_file_rename 
+
+ t
+(1 row)
+
+SELECT pg_read_file('test_file1');  -- not there
+ERROR:  could not stat file "test_file1": No such file or directory
+SELECT pg_read_file('test_file2');
+ pg_read_file 
+--
+ test1test1
+(1 row)
+
+-- error
+SELECT pg_file_rename('test_file1', 'test_file2');
+WARNING:  file "test_file1" is not accessible: No such file or directory
+ pg_file_rename 
+
+ f
+(1 row)
+
+-- rename file and archive
+SELECT pg_file_write('test_file3', 'test3', false);
+ pg_file_write 
+---
+ 5
+(1 row)
+
+SELECT pg_file_rename('test_file2', 'test_file3', 'test_file3_archive');
+ pg_file_rename 
+
+ t
+(1 row)
+
+SELECT pg_read_file('test_file2');  -- not there
+ERROR:  could not stat file "test_file2": No such file or directory
+SELECT pg_read_file('test_file3');
+ pg_read_file 
+--
+ test1test1
+(1 row)
+
+SELECT pg_read_file('test_file3_archive');
+ pg_read_file 
+--
+ test3
+(1 row)
+
+-- unlink
+SELECT pg_file_unlink('test_file1');  -- does not exist
+ pg_file_unlink 
+
+ f
+(1 row)
+
+SELECT pg_file_unlink('test_file2');  -- does not exist
+ pg_file_unlink 
+
+ f
+(1 row)
+
+SELECT pg_file_unlink('test_file3');
+ pg_file_unlink 
+
+ t
+(1 row)
+
+SELECT pg_file_unlink('test_file3_archive');
+ pg_file_unlink 
+
+ t
+(1 row)
+
+SELECT pg_file_unlink('test_file4');
+ pg_file_unlink 
+
+ t
+(1 row)
+
+-- superuser checks
+CREATE USER regress_user1;
+SET ROLE regress_user1;
+SELECT pg_file_write('test_file0', 'test0', false);
+ERROR:  only superuser may access generic file functions

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

[Pavel Stehule]

2017-09-08 19:14 GMT+02:00 Simon Riggs :

> On 6 September 2017 at 07:43, Robert Haas  wrote:
>
> > LET custom_plan_tries = 0 IN SELECT ...
>
> Tom has pointed me at this proposal, since on another thread I asked
> for something very similar. (No need to reprise that discussion, but I
> wanted prepared queries to be able to do SET work_mem = X; SELECT).
> This idea looks a good way forward to me.
>
> Since we're all in roughly the same place, I'd like to propose that we
> proceed with the following syntax... whether or not this precisely
> solves OP's issue on this thread.
>
> 1. Allow SET to set multiple parameters...
> SET guc1 = x, guc2 = y
> This looks fairly straightforward
>
> 2. Allow a SET to apply only for a single statement
> SET guc1 = x, guc2 = y FOR stmt
> e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
> Internally a GUC setting already exists for a single use, via
> GUC_ACTION_SAVE, so we just need to invoke it.
>
> Quick prototype seems like it will deliver quite quickly. I couldn't
> see a reason to use "LET" rather than just "SET" which would be the
> POLA choice.
>
> Thoughts?
>

why not

Pavel


> --
> Simon Riggshttp://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Jeff Janes]

On Wed, Sep 6, 2017 at 9:22 AM, Peter Eisentraut <
peter.eisentr...@2ndquadrant.com> wrote:

> On 8/18/17 05:28, Michael Banck wrote:
> >>> Rebased, squashed and slighly edited version attached. I've added this
> >>> to the 2017-07 commitfest now as well:
> >>>
> >>> https://commitfest.postgresql.org/14/1112/
> >> Can you rebase this past some conflicting changes?
> > Thanks for letting me know, PFA a rebased version.
>
> I have reviewed the thread so far.  I think there is general agreement
> that something like this would be good to have.
>
> I have not found any explanation, however, why the "if not exists"
> behavior is desirable, let alone as the default.  I can only think of
> two workflows here:  Either you have scripts for previous PG versions
> that create the slot externally, in which can you omit --create, or you
> use the new functionality to have pg_basebackup create the slot.  I
> don't see any use for pg_basebackup to opportunistically use a slot if
> it happens to exist.  Even if there is one, it should not be the
> default.  So please change that.
>

+1.  I'd rather just get an error and re-run without the --create switch.
That way you are forced to think about what you are doing.


Is there a race condition here?  The slot is created after the checkpoint
is completed.  But you have to start streaming from the LSN where the
checkpoint started, so shouldn't the slot be created before the checkpoint
is started?

Cheers,

Jeff

Re: [HACKERS] [bug fix] Savepoint-related statements terminates connection

[Catalin Iacob]

On Thu, Sep 7, 2017 at 8:07 PM, Tom Lane  wrote:
> I've pushed up an attempt at this:
>
>
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b976499480bdbab6d69a11e47991febe53865adc
>
> Feel free to suggest improvements.

Thank you, this helps a lot. Especially since some of the behavior is a bit
surprising, for example stopping on error leading to ROLLBACK not being
done and the retroactive upgrade of preceding commands in an implicit block
to a transaction block when a BEGIN appears.

When reading this I also realized that the backend does send responses for
every individual query in a multi-query request, it's only libpq's PQexec
that throws away the intermediate results and only provides access to the
last one. I always thought the backend did that. The docs hinted that it's
the frontend ("psql only prints the last one", "PGresult describes the
result of the last command") but to assure myself I looked with tcpdump.

It's a pity that the underlying protocol has 2 ways to do batching of
queries but the official library hides both. I guess I should go review the
"Batch/pipelining support for libpq" patch rather than complaining.

Re: [HACKERS] More flexible LDAP auth search filters?

[Mark Cave-Ayland]

On 08/09/17 16:33, Peter Eisentraut wrote:

> A couple of comments on this patch.  I have attached a "fixup" patch on
> top of your v4 that should address them.
> 
> - I think the bracketing of the LDAP URL synopsis is wrong.
> 
> - I have dropped the sentence that LDAP URL extensions are not
> supported.  That sentence was written mainly to point out that filters
> are not supported, which they are now.  There is nothing beyond filters
> and extensions left in LDAP URLs, AFAIK.
> 
> - We have previously used the ldapsearchattribute a bit strangely.  We
> use it as both the search filter and as the attribute to return from the
> search, but we don't actually care about the returned attribute (we only
> use the DN (which is not a real attribute)).  That hasn't been a real
> problem, but now if you specify a filter, as the code stands it will
> always request the "uid" attribute, which won't work if there is no such
> attribute.  I have found that there is an official way to request "no
> attribute", so I have fixed the code to do that.
> 
> - I was wondering whether we need a way to escape the % (%%?) but it
> doesn't seem worth bothering.
> 
> I have been looking around the Internet how this functionality compares
> to other LDAP authentication systems.
> 
> I didn't see the origin of the %u idea in this thread, but perhaps it
> came from Dovecot.  But there it's a general configuration file syntax,
> nothing specific to LDAP.  In nginx you use %(username), which again is
> general configuration file syntax.  I'm OK with using %u.
> 
> The original LDAP URL design was adapted from Apache
> (https://httpd.apache.org/docs/2.4/mod/mod_authnz_ldap.html#authldapurl).
>  They combine the attribute filter and the general filter if both are
> specified, but I think they got that a bit wrong.  The attribute field
> in the URL is actually not supposed to be a filter but a return field,
> which is also the confusion mentioned above.
> 
> The PAM module (https://linux.die.net/man/5/pam_ldap) also has a way to
> specify a search attribute and a general filter and combines them.
> 
> Neither of these allows substituting the user name into the search filter.

Great work! Having installed quite a few LDAP-based authentication
setups in the past, I can promise you that pam_ldap is the last tool I
would consider for the job so please don't hold to this as being the
gold standard(!).

My weapon of choice for LDAP deployments on POSIX-based systems is
Arthur De Jong's nss-pam-ldapd (https://arthurdejong.org/nss-pam-ldapd)
which is far more flexible than pam_ldap and fixes a large number of
bugs, including the tendency for pam_ldap to hang infinitely if it can't
contact its LDAP server.

Take a look at nss-pam-ldapd's man page for nslcd.conf and in particular
pam_authz_search - this is exactly the type of filters I would end up
deploying onto servers. This happens a lot in large organisations
whereby getting group memberships updated in the main directory can take
days/weeks whereas someone with root access to the server itself can
hard-code an authentication list of users and/or groups in an LDAP
filter in just a few minutes.


ATB,

Mark.


-- 
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 - allow to store select results into variables

[Fabien COELHO]

Here is a v12.

There is no changes in the code or documentation, only TAP tests are 
added.


--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index f5db8d1..9ad82d4 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -818,6 +818,51 @@ pgbench  options  dbname
   
 
   
+   
+
+ \cset [prefix] or
+ \gset [prefix]
+
+
+
+ 
+  These commands may be used to end SQL queries, replacing a semicolon.
+  \cset replaces an embedded semicolon (\;) within
+  a compound SQL command, and \gset replaces a final
+  (;) semicolon which ends the SQL command. 
+ 
+
+ 
+  When these commands are used, the preceding SQL query is expected to
+  return one row, the columns of which are stored into variables named after
+  column names, and prefixed with prefix if provided.
+ 
+
+ 
+  The following example puts the final account balance from the first query
+  into variable abalance, and fills variables
+  one, two and p_three with
+  integers from a compound query.
+
+UPDATE pgbench_accounts
+  SET abalance = abalance + :delta
+  WHERE aid = :aid
+  RETURNING abalance \gset
+-- compound of two queries
+SELECT 1 AS one, 2 AS two \cset
+SELECT 3 AS three \gset p_
+
+ 
+
+ 
+  
+\cset and \gset commands do not work when
+empty SQL queries appear within a compound SQL command.
+  
+ 
+
+   
+

 
  \set varname expression
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index e37496c..454127c 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -375,11 +375,14 @@ static const char *QUERYMODE[] = {"simple", "extended", "prepared"};
 
 typedef struct
 {
-	char	   *line;			/* text of command line */
+	char	   *line;			/* first line for short display */
+	char	   *lines;			/* full multi-line text of command */
 	int			command_num;	/* unique index of this Command struct */
 	int			type;			/* command type (SQL_COMMAND or META_COMMAND) */
 	int			argc;			/* number of command words */
 	char	   *argv[MAX_ARGS]; /* command word list */
+	int			compound;   /* last compound command (number of \;) */
+	char	  **gset;   /* per-compound command prefix */
 	PgBenchExpr *expr;			/* parsed expression, if needed */
 	SimpleStats stats;			/* time spent in this command */
 } Command;
@@ -1254,6 +1257,104 @@ getQueryParams(CState *st, const Command *command, const char **params)
 		params[i] = getVariable(st, command->argv[i + 1]);
 }
 
+/* read all responses from backend */
+static bool
+read_response(CState *st, char **gset)
+{
+	PGresult   *res;
+	int			compound = 0;
+
+	while ((res = PQgetResult(st->con)) != NULL)
+	{
+		switch (PQresultStatus(res))
+		{
+			case PGRES_COMMAND_OK: /* non-SELECT commands */
+			case PGRES_EMPTY_QUERY: /* may be used for testing no-op overhead */
+if (gset[compound] != NULL)
+{
+	fprintf(stderr,
+			"client %d file %d command %d compound %d: "
+			"\\gset expects a row\n",
+			st->id, st->use_file, st->command, compound);
+	st->ecnt++;
+	return false;
+}
+break; /* OK */
+
+			case PGRES_TUPLES_OK:
+if (gset[compound] != NULL)
+{
+	/* store result into variables */
+	int ntuples = PQntuples(res),
+		nfields = PQnfields(res),
+		f;
+
+	if (ntuples != 1)
+	{
+		fprintf(stderr,
+"client %d file %d command %d compound %d: "
+"expecting one row, got %d\n",
+st->id, st->use_file, st->command, compound, ntuples);
+		st->ecnt++;
+		PQclear(res);
+		discard_response(st);
+		return false;
+	}
+
+	for (f = 0; f < nfields ; f++)
+	{
+		char *varname = PQfname(res, f);
+		if (*gset[compound] != '\0')
+			varname = psprintf("%s%s", gset[compound], varname);
+
+		/* store result as a string */
+		if (!putVariable(st, "gset", varname,
+		 PQgetvalue(res, 0, f)))
+		{
+			/* internal error, should it rather abort? */
+			fprintf(stderr,
+	"client %d file %d command %d compound %d: "
+	"error storing into var %s\n",
+	st->id, st->use_file, st->command, compound,
+	varname);
+			st->ecnt++;
+			PQclear(res);
+			discard_response(st);
+			return false;
+		}
+
+		if (*gset[compound] != '\0')
+			free(varname);
+	}
+}
+break;	/* OK */
+
+			default:
+/* everything else is unexpected, so probably an error */
+fprintf(stderr,
+		"client %d file %d aborted in command %d compound %d: %s",
+		st->id, st->use_file, st->command, compound,
+		PQerrorMessage(st->con));
+st->ecnt++;
+PQclear(res);
+discard_response(st);
+return false;
+		}
+
+		PQclear(res);
+		compound += 1;
+	}
+
+	if (compound == 0)
+	{
+		fprintf(stderr, "client %d command %d: no results\n", st->id, st->command);
+		st->ecnt++;
+		return false;
+	}
+
+	

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

[Simon Riggs]

On 6 September 2017 at 07:43, Robert Haas  wrote:

> LET custom_plan_tries = 0 IN SELECT ...

Tom has pointed me at this proposal, since on another thread I asked
for something very similar. (No need to reprise that discussion, but I
wanted prepared queries to be able to do SET work_mem = X; SELECT).
This idea looks a good way forward to me.

Since we're all in roughly the same place, I'd like to propose that we
proceed with the following syntax... whether or not this precisely
solves OP's issue on this thread.

1. Allow SET to set multiple parameters...
SET guc1 = x, guc2 = y
This looks fairly straightforward

2. Allow a SET to apply only for a single statement
SET guc1 = x, guc2 = y FOR stmt
e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
Internally a GUC setting already exists for a single use, via
GUC_ACTION_SAVE, so we just need to invoke it.

Quick prototype seems like it will deliver quite quickly. I couldn't
see a reason to use "LET" rather than just "SET" which would be the
POLA choice.

Thoughts?

-- 
Simon Riggshttp://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 plpgsql - possibility to force custom or generic plan

[Simon Riggs]

On 6 September 2017 at 07:43, Robert Haas  wrote:

> LET custom_plan_tries = 0 IN SELECT ...

Tom has pointed me at this proposal, since on another thread I asked
for something very similar. (No need to reprise that discussion, but I
wanted prepared queries to be able to do SET work_mem = X; SELECT).
This idea looks a good way forward to me.

Since we're all in roughly the same place, I'd like to propose that we
proceed with the following syntax... whether or not this precisely
solves OP's issue on this thread.

1. Allow SET to set multiple parameters...
SET guc1 = x, guc2 = y
This looks fairly straightforward

2. Allow a SET to apply only for a single statement
SET guc1 = x, guc2 = y FOR stmt
e.g. SET max_parallel_workers = 4 FOR SELECT count(*) FROM bigtable
Internally a GUC setting already exists for a single use, via
GUC_ACTION_SAVE, so we just need to invoke it.

Quick prototype seems like it will deliver quite quickly. I couldn't
see a reason to use "LET" rather than just "SET" which would be the
POLA choice.

Thoughts?

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

[Peter Geoghegan]

On Thu, Sep 7, 2017 at 2:49 PM, Robert Haas  wrote:
> On Thu, Sep 7, 2017 at 5:38 PM, Tomas Vondra
>  wrote:
>> Do we need/want to repeat some of that benchmarking on these patches? I
>> don't recall how much this code changed since those benchmarks were done
>> in the 9.6 cycle.
>
> +1 for some new benchmarking.  I'm all for removing this code if we
> don't need it any more, but it'd be a lot better to have more numbers
> before deciding that.

It isn't always a win. For example, if I alter the pgbench_accounts
table so that the column "aid" is of type numeric, the picture changes
for my test case -- replacement selection is still somewhat faster for
the "select count(distinct aid) from pgbench_accounts" query with
work_mem=2MB. It takes about 5.4 seconds with replacement selection,
versus 7.4 seconds for quicksorting. But, I still think we should
proceed with my patch, because:

* It's still faster with int4/int8 comparisons on modern hardware, and
I think that most ordered inputs will be of those types in practice.
The trade-off between those two properties (faster for int4/int8 when
ordered, slower for numeric) recommends killing replacement selection
entirely. It's not a slam dunk, but it makes sense on performance
grounds, IMV.

* The comparator numeric_fast_cmp() is not well optimized -- it
doesn't avoid allocating memory with each call, for example, unlike
varstrfastcmp_locale(). This could and should be improved, and might
even change the picture for this second test case.

* With the default work_mem of 8MB, we never use replacement selection
anyway. Whatever its merits, it's pretty rare to use replacement
selection simply because the default replacement_sort_tuples just
isn't that  many tuples (150,000).

* The upside of my patch is not inconsiderable: We can remove a lot of
code, which will enable further improvements in the future, which are
far more compelling (cleaner memory management, the use of memory
batches during run generation).

-- 
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] code cleanup empty string initializations

[Peter Eisentraut]

On 9/8/17 03:45, Aleksandr Parfenov wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world:  tested, passed
> Implements feature:   tested, passed
> Spec compliant:   tested, passed
> Documentation:tested, passed
> 
> Hi Peter,
> 
> I looked through your patches and its look good to me.
> Patches make code more readable and clear, especially in case of encodingid.
> 
> The new status of this patch is: Ready for Committer

Committed.  Thanks!

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

[Victor Drobny]

On 2017-09-08 15:23, Thomas Munro wrote:
On Fri, Sep 8, 2017 at 9:03 PM, Victor Drobny  
wrote:
Thank you very much for your review. In the attachment you can find v2 
of

the patch.


FYI this version crashes for me:

test test_rbtree  ... FAILED (test process exited with exit 
code 2)


It's trying to call rb->combiner which is null.


Thank you for pointing on it. Here is a fixed version.

--
Victor Drobny
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 3ce9904..b7ed0af 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -13,6 +13,7 @@ SUBDIRS = \
 		  test_extensions \
 		  test_parser \
 		  test_pg_dump \
+		  test_rbtree \
 		  test_rls_hooks \
 		  test_shm_mq \
 		  worker_spi
diff --git a/src/test/modules/test_rbtree/Makefile b/src/test/modules/test_rbtree/Makefile
new file mode 100644
index 000..4e53e86
--- /dev/null
+++ b/src/test/modules/test_rbtree/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_rbtree/Makefile
+
+MODULE_big = test_rbtree
+OBJS = test.o $(WIN32RES)
+PGFILEDESC = "test_rbtree - rbtree traversal testing"
+
+EXTENSION = test_rbtree
+DATA = test_rbtree--1.0.sql
+
+REGRESS = test_rbtree
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_rbtree
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_rbtree/README b/src/test/modules/test_rbtree/README
new file mode 100644
index 000..40f586d
--- /dev/null
+++ b/src/test/modules/test_rbtree/README
@@ -0,0 +1,20 @@
+test_rbtree is a module tests for checking the correctness of all kinds of
+traversal of red-black tree. Right now rbtree in postgres has 4 kinds of
+traversals: Left-Current-Right, Right-Current-Left, Current-Left-Right and
+Left-Right-Current.
+
+This extension has 4 functions. Each function checks one traversal.
+The checking the correctness of first two types are based on the fact that
+red-black tree is a binary search tree, so the elements should be iterated in
+increasing(for Left-Current-Right) or decreasing(for Right-Current-Left)
+order.
+In order to verify last two strategies, we will check the sequence if it is
+correct or not. For given pre- or post- order traversing of binary search tree
+it is always possible to say is it correct or not and to rebuild original tree.
+The idea is based on the fact that in such traversal sequence is always
+possible to determine current node, left subtree and right subtree.
+
+Also, this module is checking the correctness of the find, delete and leftmost
+operation.
+
+These tests are performed on red-black trees that store integers.
diff --git a/src/test/modules/test_rbtree/expected/test_rbtree.out b/src/test/modules/test_rbtree/expected/test_rbtree.out
new file mode 100644
index 000..8223e81
--- /dev/null
+++ b/src/test/modules/test_rbtree/expected/test_rbtree.out
@@ -0,0 +1,7 @@
+CREATE EXTENSION test_rbtree;
+SELECT testrbtree(10);
+ testrbtree 
+
+ 
+(1 row)
+
diff --git a/src/test/modules/test_rbtree/int_rbtree.h b/src/test/modules/test_rbtree/int_rbtree.h
new file mode 100644
index 000..57754bf
--- /dev/null
+++ b/src/test/modules/test_rbtree/int_rbtree.h
@@ -0,0 +1,49 @@
+/*--
+ *
+ * int_rbtree.h
+ *		Definitions for integer red-black tree
+ *
+ * Copyright (c) 2013-2017, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		src/test/modules/test_rbtree/int_rbtree.h
+ *
+ * -
+ */
+
+#ifndef INT_RBTREE_H
+#define INT_RBTREE_H
+
+#include "lib/rbtree.h"
+
+typedef struct IntRBTreeNode
+{
+	RBNode		rbnode;
+	int			key;
+
+} IntRBTreeNode;
+
+static int
+cmp(const RBNode *a, const RBNode *b, void *arg)
+{
+	const IntRBTreeNode *ea = (const IntRBTreeNode *) a;
+	const IntRBTreeNode *eb = (const IntRBTreeNode *) b;
+
+	return ea->key - eb->key;
+}
+
+static RBNode *
+alloc(void *arg)
+{
+	IntRBTreeNode *ea;
+	ea = malloc(sizeof(IntRBTreeNode));
+	return (RBNode *) ea;
+}
+
+static void
+fr(RBNode * node, void *arg)
+{
+	free(node);
+}
+
+#endif /* INT_RBTREE_H */
diff --git a/src/test/modules/test_rbtree/sql/test_rbtree.sql b/src/test/modules/test_rbtree/sql/test_rbtree.sql
new file mode 100644
index 000..8263df3
--- /dev/null
+++ b/src/test/modules/test_rbtree/sql/test_rbtree.sql
@@ -0,0 +1,3 @@
+CREATE EXTENSION test_rbtree;
+
+SELECT testrbtree(10);
diff --git a/src/test/modules/test_rbtree/test.c b/src/test/modules/test_rbtree/test.c
new file mode 100644
index 000..e47e637
--- /dev/null
+++ b/src/test/modules/test_rbtree/test.c
@@ -0,0 +1,625 @@
+/*--
+ 

Re: [HACKERS] More flexible LDAP auth search filters?

[Peter Eisentraut]

For additional entertainment I have written a test suite for this LDAP
authentication functionality.  It's not quite robust enough to be run by
default, because it needs a full OpenLDAP installation, but it's been
very helpful for reviewing this patch.  Here it is.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 9457ef272d011dbb34b1a204cac9cbac08e4d652 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 8 Sep 2017 10:33:49 -0400
Subject: [PATCH 1/3] Add LDAP authentication test suite

---
 src/test/Makefile   |   2 +-
 src/test/ldap/.gitignore|   2 +
 src/test/ldap/Makefile  |  20 +++
 src/test/ldap/README|  16 +
 src/test/ldap/data.ldif |  19 ++
 src/test/ldap/t/001_auth.pl | 139 
 6 files changed, 197 insertions(+), 1 deletion(-)
 create mode 100644 src/test/ldap/.gitignore
 create mode 100644 src/test/ldap/Makefile
 create mode 100644 src/test/ldap/README
 create mode 100644 src/test/ldap/data.ldif
 create mode 100644 src/test/ldap/t/001_auth.pl

diff --git a/src/test/Makefile b/src/test/Makefile
index dbfa799a84..193b977bf3 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -17,7 +17,7 @@ SUBDIRS = perl regress isolation modules authentication 
recovery subscription
 # We don't build or execute examples/, locale/, or thread/ by default,
 # but we do want "make clean" etc to recurse into them.  Likewise for ssl/,
 # because the SSL test suite is not secure to run on a multi-user system.
-ALWAYS_SUBDIRS = examples locale thread ssl
+ALWAYS_SUBDIRS = examples ldap locale thread ssl
 
 # We want to recurse to all subdirs for all standard targets, except that
 # installcheck and install should not recurse into the subdirectory "modules".
diff --git a/src/test/ldap/.gitignore b/src/test/ldap/.gitignore
new file mode 100644
index 00..871e943d50
--- /dev/null
+++ b/src/test/ldap/.gitignore
@@ -0,0 +1,2 @@
+# Generated by test suite
+/tmp_check/
diff --git a/src/test/ldap/Makefile b/src/test/ldap/Makefile
new file mode 100644
index 00..9dd1bbeade
--- /dev/null
+++ b/src/test/ldap/Makefile
@@ -0,0 +1,20 @@
+#-
+#
+# Makefile for src/test/ldap
+#
+# Portions Copyright (c) 1996-2017, PostgreSQL Global Development Group
+# Portions Copyright (c) 1994, Regents of the University of California
+#
+# src/test/ldap/Makefile
+#
+#-
+
+subdir = src/test/ldap
+top_builddir = ../../..
+include $(top_builddir)/src/Makefile.global
+
+check:
+   $(prove_check)
+
+clean distclean maintainer-clean:
+   rm -rf tmp_check
diff --git a/src/test/ldap/README b/src/test/ldap/README
new file mode 100644
index 00..20a7a0b5de
--- /dev/null
+++ b/src/test/ldap/README
@@ -0,0 +1,16 @@
+src/test/ldap/README
+
+Tests for LDAP functionality
+
+
+This directory contains a test suite for LDAP functionality.  This
+requires a full OpenLDAP installation, including server and client
+tools, and is therefore kept separate and not run by default.
+
+
+Running the tests
+=
+
+make check
+
+NOTE: This requires the --enable-tap-tests argument to configure.
diff --git a/src/test/ldap/data.ldif b/src/test/ldap/data.ldif
new file mode 100644
index 00..b30604e1f0
--- /dev/null
+++ b/src/test/ldap/data.ldif
@@ -0,0 +1,19 @@
+dn: dc=example,dc=net
+objectClass: top
+objectClass: dcObject
+objectClass: organization
+dc: example
+o: ExmapleCo
+
+dn: uid=test1,dc=example,dc=net
+objectClass: inetOrgPerson
+objectClass: posixAccount
+uid: test1
+sn: Lastname
+givenName: Firstname
+cn: First Test User
+displayName: First Test User
+uidNumber: 101
+gidNumber: 100
+homeDirectory: /home/test1
+mail: te...@example.net
diff --git a/src/test/ldap/t/001_auth.pl b/src/test/ldap/t/001_auth.pl
new file mode 100644
index 00..af9e34d7cf
--- /dev/null
+++ b/src/test/ldap/t/001_auth.pl
@@ -0,0 +1,139 @@
+use strict;
+use warnings;
+use TestLib;
+use PostgresNode;
+use Test::More tests => 9;
+
+my ($slapd, $ldap_bin_dir, $ldap_schema_dir);
+
+$ldap_bin_dir = undef; # usually in PATH
+
+if ($^O eq 'darwin')
+{
+   $slapd = '/usr/local/opt/openldap/libexec/slapd';
+   $ldap_schema_dir = '/usr/local/etc/openldap/schema';
+}
+elsif ($^O eq 'linux')
+{
+   $slapd = '/usr/sbin/slapd';
+   $ldap_schema_dir = '/etc/ldap/schema' if -f '/etc/ldap/schema';
+   $ldap_schema_dir = '/etc/openldap/schema' if -f '/etc/openldap/schema';
+}
+
+# make your own edits here
+#$slapd = '';
+#$ldap_bin_dir = '';
+#$ldap_schema_dir = '';
+
+$ENV{PATH} = "$ldap_bin_dir:$ENV{PATH}" if $ldap_bin_dir;
+
+my $ldap_datadir = "${TestLib::tmp_check}/openldap-data";
+my $slapd_conf = "${TestLib::tmp_check}/slapd.conf";
+my $slapd_pidfile 

Re: [HACKERS] More flexible LDAP auth search filters?

[Peter Eisentraut]

A couple of comments on this patch.  I have attached a "fixup" patch on
top of your v4 that should address them.

- I think the bracketing of the LDAP URL synopsis is wrong.

- I have dropped the sentence that LDAP URL extensions are not
supported.  That sentence was written mainly to point out that filters
are not supported, which they are now.  There is nothing beyond filters
and extensions left in LDAP URLs, AFAIK.

- We have previously used the ldapsearchattribute a bit strangely.  We
use it as both the search filter and as the attribute to return from the
search, but we don't actually care about the returned attribute (we only
use the DN (which is not a real attribute)).  That hasn't been a real
problem, but now if you specify a filter, as the code stands it will
always request the "uid" attribute, which won't work if there is no such
attribute.  I have found that there is an official way to request "no
attribute", so I have fixed the code to do that.

- I was wondering whether we need a way to escape the % (%%?) but it
doesn't seem worth bothering.

I have been looking around the Internet how this functionality compares
to other LDAP authentication systems.

I didn't see the origin of the %u idea in this thread, but perhaps it
came from Dovecot.  But there it's a general configuration file syntax,
nothing specific to LDAP.  In nginx you use %(username), which again is
general configuration file syntax.  I'm OK with using %u.

The original LDAP URL design was adapted from Apache
(https://httpd.apache.org/docs/2.4/mod/mod_authnz_ldap.html#authldapurl).
 They combine the attribute filter and the general filter if both are
specified, but I think they got that a bit wrong.  The attribute field
in the URL is actually not supposed to be a filter but a return field,
which is also the confusion mentioned above.

The PAM module (https://linux.die.net/man/5/pam_ldap) also has a way to
specify a search attribute and a general filter and combines them.

Neither of these allows substituting the user name into the search filter.

I think there would be a case to be made to allow the searchattribute
and the searchfilter both be specified.  One typical use would be to use
the first one to locate the user and the second one to "filter" out
certain things, which I think is the approach the PAM and Apache modules
take.  This wouldn't work well, however, if you use the %u placeholder,
because then you'd have to explicitly unset ldapsearchattribute, which
would be annoying.  So maybe not.

Please check my patch.  I think it's ready to go then.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From cd5536fa70eed74f8b1aa4f856edae8b84d76ef5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Fri, 8 Sep 2017 10:58:53 -0400
Subject: [PATCH] fixup! Allow custom search filters to be configured for LDAP
 auth.

---
 doc/src/sgml/client-auth.sgml |  4 +---
 src/backend/libpq/auth.c  | 18 --
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 1773ce29a9..1c3db96134 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -1525,7 +1525,7 @@ LDAP Authentication
  An RFC 4516 LDAP URL.  This is an alternative way to write some of the
  other LDAP options in a more compact and standard form.  The format is
 
-ldap://host[:port]/basedn[[[?[attribute]][?scope]][?filter]]
+ldap://host[:port]/basedn[?[attribute][?[scope][?[filter
 
  scope must be one
  of base, one, 
sub,
@@ -1537,8 +1537,6 @@ LDAP Authentication
  ldapsearchfilter.  When specifying a search filter
  as part of a URL, the special token %u standing
  for the user name must be written as %25u.
- Some other components of standard LDAP URLs such as extensions are
- not supported.
 
 
 
diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
index 6c96e87d37..c43f203eb3 100644
--- a/src/backend/libpq/auth.c
+++ b/src/backend/libpq/auth.c
@@ -2401,8 +2401,8 @@ InitializeLDAPConnection(Port *port, LDAP **ldap)
 static char *
 FormatSearchFilter(const char *pattern, const char *user_name)
 {
-   Size user_name_size = strlen(user_name);
-   Size buffer_size = 0;
+   size_t user_name_size = strlen(user_name);
+   size_t buffer_size = 0;
const char *in;
char *out;
char *result;
@@ -2413,7 +2413,7 @@ FormatSearchFilter(const char *pattern, const char 
*user_name)
{
if (in[0] == '%' && in[1] == 'u')
{
-   buffer_size += user_name_size;
+   buffer_size += user_name_size - 2;
in += 2;
}
else
@@ -2486,7 +2486,7 @@ CheckLDAPAuth(Port *port)
char   *filter;

Re: [HACKERS] Fix performance degradation of contended LWLock on NUMA

[Jesper Pedersen]

Hi,

On 07/18/2017 01:20 PM, Sokolov Yura wrote:

I'm sending rebased version with couple of one-line tweaks.
(less skip_wait_list on shared lock, and don't update spin-stat on 
aquiring)




I have been running this patch on a 2S/28C/56T/256Gb w/ 2 x RAID10 SSD 
setup (1 to 375 clients on logged tables).


I'm seeing

-M prepared: Up to 11% improvement
-M prepared -S: No improvement, no regression ("noise")
-M prepared -N: Up to 12% improvement

for all runs the improvement shows up the closer you get to the number 
of CPU threads, or above. Although I'm not seeing the same improvements 
as you on very large client counts there are definitely improvements :)


Some comments:
==

lwlock.c:
-
+ * This race is avoiding by taking lock for wait list using CAS with a old
+ * state value, so it could succeed only if lock is still held. 
Necessary flag

+ * is set with same CAS.
+ *
+ * Inside LWLockConflictsWithVar wait list lock is reused to protect 
variable

+ * value. So first it is acquired to check variable value, then flags are
+ * set with second CAS before queueing to wait list to ensure lock were not
+ * released yet.

 * This race is avoided by taking a lock for the wait list using CAS 
with the old
 * state value, so it would only succeed if lock is still held. 
Necessary flag

 * is set using the same CAS.
 *
 * Inside LWLockConflictsWithVar the wait list lock is reused to 
protect the variable
 * value. First the lock is acquired to check the variable value, then 
flags are
 * set with a second CAS before queuing to the wait list in order to 
ensure that the lock was not

 * released yet.


+static void
+add_lwlock_stats_spin_stat(LWLock *lock, SpinDelayStatus *delayStatus)

Add a method description.

+   /*
+* This barrier is never needed for correctness, and it 
is no-op
+* on x86. But probably first iteration of cas loop in
+* ProcArrayGroupClearXid will succeed oftener with it.
+*/

* "more often"

+static inline bool
+LWLockAttemptLockOrQueueSelf(LWLock *lock, LWLockMode mode, LWLockMode 
waitmode)


I'll leave it to the Committer to decide if this method is too big to be 
"inline".


+   /*
+* We intentionally do not call finish_spin_delay here, cause loop above
+* usually finished in queueing into wait list on contention, and 
doesn't
+* reach spins_per_delay (and so, doesn't sleep inside of
+* perform_spin_delay). Also, different LWLocks has very different
+* contention pattern, and it is wrong to update spin-lock statistic 
based
+* on LWLock contention.
+*/

/*
 * We intentionally do not call finish_spin_delay here, because the 
loop above
 * usually finished by queuing into the wait list on contention, and 
doesn't

 * reach spins_per_delay thereby doesn't sleep inside of
 * perform_spin_delay. Also, different LWLocks has very different
 * contention pattern, and it is wrong to update spin-lock statistic based
 * on LWLock contention.
 */


s_lock.c:
-
+   if (status->spins == 0)
+   /* but we didn't spin either, so ignore */
+   return;

Use { } for the if, or move the comment out of the nesting for readability.

Open questions:
---
* spins_per_delay as extern
* Calculation of skip_wait_list


You could run the patch through pgindent too.

Passes make check-world.

Status: Waiting on Author

Best regards,
 Jesper


--
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 tap tests & minor fixes.

[Fabien COELHO]

Hello Tom,


Pushed, with some minor fooling with comments and after running it
through perltidy.  (I have no opinions about Perl code formatting,
but perltidy does ...)


Why not. I do not like the result much, but it homogeneity is not a bad 
thing.



The only substantive change I made was to drop the test that attempted
to connect to no-such-host.postgresql.org.  That's (a) unnecessary,
as this is a test of pgbench not libpq; (b) likely to provoke a wide
range of platform-specific error messages, which we'd have to account
for given that the test is looking for specific output; and (c) likely
to draw complaints from buildfarm owners and packagers who do not like
test scripts that try to make random external network connections.


Ok. Note that it was only an unsuccessful DNS request, but I understand 
the concern. The libpq tap test mostly focus on url parsing but seem to 
include host names ("example.com"/host) and various IPs.



Like you, I'm a bit worried about the code for extracting an exit
status from IPC::Run::run.  We'll have to keep an eye on the buildfarm
for a bit.  If there's any trouble, I'd be inclined to drop it down
to just success/fail rather than checking the exact exit code.


Ok. If this occurs, there is a $windows_os variable that can be tested to 
soften the test only if necessary, and keep the exact check for systems 
that can.


Thanks for the review, the improvements and the push. Now the various 
patches about pgbench in the queue should include tap-tests.


Hopefully one line will be greener on https://coverage.postgresql.org/.

--
Fabien.


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

[HACKERS] Commitfest 201709 Progress

[Daniel Gustafsson]

We are now a few days in on Commitfest 201709 and there has been lots of
activity on the patches.  22% of the patches have been committed* with another
10% being marked Ready for Committer.  Thank you all for the hard work!

There is still a lot to do though, this fest has a recordbreaking 256 patches
and will need lots more hard work to close more patches.  There are, at the
time of writing, 131 patches that need review with 65 of those also waiting for
a reviewer.  Quite a few of these patches are large and attention early in the
fest will be crucial to iron out the details.

If you’ve never reviewed a patch before, there won’t be a better time to get
involved.  Remember that everyones input is just as valuable, PostgreSQL is a
community effort!

Again, thank you for all the hard work so far.

cheers ./daniel, as CFM 201709

* The keen observer will have noted that some patches were committed even
  before the commitfest began, but hey, that still counts.

-- 
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] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Fri, Sep 8, 2017 at 6:46 AM, Robert Haas  wrote:

> On Thu, Sep 7, 2017 at 8:13 AM, Jeevan Ladhe
>  wrote:
> > The fix would be much easier if the refactoring patch 0001 by Amul in
> hash
> > partitioning thread[2] is committed.
>
> Done.
>

Thanks Robert for taking care of this.
My V29 patch series[1] is based on this commit now.

[1]
https://www.postgresql.org/message-id/CAOgcT0PCM5mJPCOyj3c0D1mLxwaVz6DJLO=nmz5j-5jgywx...@mail.gmail.com

Regards,
Jeevan Ladhe

[HACKERS] Reminder: 10rc1 wraps Monday

[Tom Lane]

see subject.

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] Parallel Append implementation

[Amit Kapila]

On Fri, Sep 8, 2017 at 3:59 PM, Amit Khandekar  wrote:
> On 7 September 2017 at 11:05, Amit Kapila  wrote:
>> On Thu, Aug 31, 2017 at 12:47 PM, Amit Khandekar  
>> wrote:
>> 3.
>> +/* 
>> + * exec_append_leader_next
>> + *
>> + * To be used only if it's a parallel leader. The backend should scan
>> + * backwards from the last plan. This is to prevent it from taking up
>> + * the most expensive non-partial plan, i.e. the first subplan.
>> + * 
>> + */
>> +static bool
>> +exec_append_leader_next(AppendState *state)
>>
>> From above explanation, it is clear that you don't want backend to
>> pick an expensive plan for a leader, but the reason for this different
>> treatment is not clear.
>
> Explained it, saying that for more workers, a leader spends more work
> in processing the worker tuples , and less work contributing to
> parallel processing. So it should not take expensive plans, otherwise
> it will affect the total time to finish Append plan.
>

In that case, why can't we keep the workers also process in same
order, what is the harm in that?  Also, the leader will always scan
the subplans from last subplan even if all the subplans are partial
plans.  I think this will be the unnecessary difference in the
strategy of leader and worker especially when all paths are partial.
I think the selection of next subplan might become simpler if we use
the same strategy for worker and leader.

Few more comments:

1.
+ else if (IsA(subpath, MergeAppendPath))
+ {
+ MergeAppendPath *mpath = (MergeAppendPath *) subpath;
+
+ /*
+ * If at all MergeAppend is partial, all its child plans have to be
+ * partial : we don't currently support a mix of partial and
+ * non-partial MergeAppend subpaths.
+ */
+ if (is_partial)
+ return list_concat(partial_subpaths, list_copy(mpath->subpaths));

In which situation partial MergeAppendPath is generated?  Can you
provide one example of such path?

2.
add_paths_to_append_rel()
{
..
+ /* Consider parallel append path. */
+ if (pa_subpaths_valid)
+ {
+ AppendPath *appendpath;
+ int parallel_workers;
+
+ parallel_workers = get_append_num_workers(pa_partial_subpaths,
+  pa_nonpartial_subpaths);
+ appendpath = create_append_path(rel, pa_nonpartial_subpaths,
+ pa_partial_subpaths,
+ NULL, parallel_workers, true,
+ partitioned_rels);
+ add_partial_path(rel, (Path *) appendpath);
+ }
+
  /*
- * Consider an append of partial unordered, unparameterized partial paths.
+ * Consider non-parallel partial append path. But if the parallel append
+ * path is made out of all partial subpaths, don't create another partial
+ * path; we will keep only the parallel append path in that case.
  */
- if (partial_subpaths_valid)
+ if (partial_subpaths_valid && !pa_all_partial_subpaths)
  {
  AppendPath *appendpath;
  ListCell   *lc;
  int parallel_workers = 0;

  /*
- * Decide on the number of workers to request for this append path.
- * For now, we just use the maximum value from among the members.  It
- * might be useful to use a higher number if the Append node were
- * smart enough to spread out the workers, but it currently isn't.
+ * To decide the number of workers, just use the maximum value from
+ * among the children.
  */
  foreach(lc, partial_subpaths)
  {
@@ -1421,9 +1502,9 @@ add_paths_to_append_rel(PlannerInfo *root,
RelOptInfo *rel,
  }
  Assert(parallel_workers > 0);

- /* Generate a partial append path. */
- appendpath = create_append_path(rel, partial_subpaths, NULL,
- parallel_workers, partitioned_rels);
+ appendpath = create_append_path(rel, NIL, partial_subpaths,
+ NULL, parallel_workers, false,
+ partitioned_rels);
  add_partial_path(rel, (Path *) appendpath);
  }
..
}

I think it might be better to add a sentence why we choose a different
way to decide a number of workers in the second case
(non-parallel-aware append).  Do you think non-parallel-aware Append
will be better in any case when there is a parallel-aware append?  I
mean to say let's try to create non-parallel-aware append only when
parallel-aware append is not possible.

3.
+ * evaluates to a value just a bit greater than max(w1,w2, w3). So, we

The spacing between w1, w2, w3 is not same.

4.
-  select count(*) from a_star;
-select count(*) from a_star;
+  select round(avg(aa)), sum(aa) from a_star;
+select round(avg(aa)), sum(aa) from a_star;

Why you have changed the existing test. It seems count(*) will also
give what you are expecting.



-- 
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 tap tests & minor fixes.

[Tom Lane]

Fabien COELHO  writes:
> [ pgbench-tap-12.patch ]

Pushed, with some minor fooling with comments and after running it
through perltidy.  (I have no opinions about Perl code formatting,
but perltidy does ...)

The only substantive change I made was to drop the test that attempted
to connect to no-such-host.postgresql.org.  That's (a) unnecessary,
as this is a test of pgbench not libpq; (b) likely to provoke a wide
range of platform-specific error messages, which we'd have to account
for given that the test is looking for specific output; and (c) likely
to draw complaints from buildfarm owners and packagers who do not like
test scripts that try to make random external network connections.

Like you, I'm a bit worried about the code for extracting an exit
status from IPC::Run::run.  We'll have to keep an eye on the buildfarm
for a bit.  If there's any trouble, I'd be inclined to drop it down
to just success/fail rather than checking the exact exit code.

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] WIP: Aggregation push-down

[Antonin Houska]

Ashutosh Bapat  wrote:

> On Thu, Aug 17, 2017 at 8:52 PM, Antonin Houska  wrote:
> > Antonin Houska  wrote:
> >
> >> Antonin Houska  wrote:
> >>
> >> > This is a new version of the patch I presented in [1].
> >>
> >> Rebased.
> >>
> >> cat .git/refs/heads/master
> >> b9a3ef55b253d885081c2d0e9dc45802cab71c7b
> >
> > This is another version of the patch.
> >
> > Besides other changes, it enables the aggregation push-down for 
> > postgres_fdw,
> > although only for aggregates whose transient aggregation state is equal to 
> > the
> > output type. For other aggregates (like avg()) the remote nodes will have to
> > return the transient state value in an appropriate form (maybe bytea type),
> > which does not depend on PG version.
> 
> Having transient aggregation state type same as output type doesn't
> mean that we can feed the output of remote aggregation as is to the
> finalization stage locally or finalization is not needed at all. I am
> not sure why is that test being used to decide whether or not to push
> an aggregate (I assume they are partial aggregates) to the foreign
> server.

I agree. This seems to be the same problem as reported in [2].

> postgres_fdw doesn't have a feature to fetch partial aggregate
> results from the foreign server. Till we do that, I don't think this
> patch can push any partial aggregates down to the foreign server.

Even if the query contains only aggregates like sum(int4), i.e. aggfinalfn
does not exist and the transient type can be transfered from the remote server
in textual form? (Of course there's a question is if such behaviour is
consistent from user's perspective.)

> >
> > One thing I'm not sure about is whether the patch should remove
> > GetForeignUpperPaths function from FdwRoutine, which it essentially makes
> > obsolete. Or should it only be deprecated so far? I understand that
> > deprecation is important for "ordinary SQL users", but FdwRoutine is an
> > interface for extension developers, so the policy might be different.
> 
> I doubt if that's correct. We can do that only when we know that all
> kinds aggregates/grouping can be pushed down the join tree, which
> looks impossible to me. Apart from that that FdwRoutine handles all
> kinds of upper relations like windowing, distinct, ordered which are
> not pushed down by this set of patches.

Good point.

> Here's review of the patchset
> The patches compile cleanly when applied together.
> 
> Testcase "limit" fails in make check.

If I remember correctly, this test generates different plan because the
aggregate push-down gets involved. I tend to ignore this error until the patch
has cost estimates refined. Then let's see if the test will pass or if the
expected output should be adjusted.

> This is a pretty large patchset and the patches are divided by stages in the
> planner rather than functionality. IOW, no single patch provides a full
> functionality. Reviewing and probably committing such patches is not easy and
> may take quite long. Instead, if the patches are divided by functionality, 
> i.e.
> each patch provides some functionality completely, it will be easy to review
> and commit such patches with each commit giving something useful. I had
> suggested breaking patches on that line at [1]. The patches also refactor
> existing code. It's better to move such refactoring in a patch/es by itself.

I definitely saw commits whose purpose is preparation for something else. But
you're right that some splitting of the actual funcionality wouldn't
harm. I'll at least separate partial aggregation of base relations and that of
joins. And maybe some more preparation steps.

> The patches need more comments, explaining various decisions

o.k.

> The patch maintains an extra rel target, two pathlists, partial pathlist and
> non-partial pathlist for grouping in RelOptInfo. These two extra
> pathlists require "grouped" argument to be passed to a number of functions.
> Instead probably it makes sense to create a RelOptInfo for 
> aggregation/grouping
> and set pathlist and partial_pathlist in that RelOptInfo. That will also make 
> a
> place for keeping grouped estimates.

If grouped paths require a separate RelOptInfo, why the existing partial paths
do not?

> For placeholders we have two function add_placeholders_to_base_rels() and
> add_placeholders_to_joinrel(). We have add_grouping_info_to_base_rels(), but 
> do
> not have corresponding function for joinrels. How do we push aggregates
> involving two or more relations to the corresponding joinrels?

add_grouping_info_to_base_rels() calls prepare_rel_for_grouping() which
actually adds the "grouping info". For join, prepare_rel_for_grouping() is
called from build_join_rel().

While PlaceHolderVars need to be finalized before planner starts to create
joins, the GroupedPathInfo has to fit particular pair of joined relations.

Perhaps create_grouping_info_... would be better, 

Re: [HACKERS] [POC] hash partitioning

[amul sul]

On Fri, Sep 8, 2017 at 6:45 AM, Robert Haas  wrote:

> On Mon, Sep 4, 2017 at 6:38 AM, amul sul  wrote:
> > I've updated patch to use an extended hash function (Commit #
> > 81c5e46c490e2426db243eada186995da5bb0ba7) for the partitioning.
>
> Committed 0001 after noticing that Jeevan Ladhe also found that change
> convenient for default partitioning.  I made a few minor cleanups;
> hopefully I didn't break anything.
>

​Thanks you.

Rebased 0002 against this commit & renamed to 0001, PFA.

Regards,
Amul​


0001-hash-partitioning_another_design-v18.patch
Description: Binary data

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

Re: [HACKERS] WIP: Aggregation push-down

[Antonin Houska]

Jeevan Chalke  wrote:

> 1.
> + if (aggref->aggvariadic ||
> + aggref->aggdirectargs || aggref->aggorder ||
> + aggref->aggdistinct || aggref->aggfilter)
> 
> I did not understand, why you are not pushing aggregate in above cases?
> Can you please explain?

Currently I'm trying to implement infrastructure to propagate result of
partial aggregation from base relation or join to the root of the join tree
and to use these as input for the final aggregation. Some special cases are
disabled because I haven't thought about them enough so far. Some of these
restrictions may be easy to relax, others may be hard. I'll get back to them
at some point.

> 2. "make check" in contrib/postgres_fdw crashes.
> 
> SELECT COUNT(*) FROM ft1 t1;
> ! server closed the connection unexpectedly
> ! This probably means the server terminated abnormally
> ! before or while processing the request.
> ! connection to server was lost
> 
> From your given setup, if I wrote a query like:
> EXPLAIN VERBOSE SELECT count(*) FROM orders_0;
> it crashes.
> 
> Seems like missing few checks.

Please consider this a temporary limitation.

> 3. In case of pushing partial aggregation on the remote side, you use schema
> named "partial", I did not get that change. If I have AVG() aggregate,
> then I end up getting an error saying
> "ERROR: schema "partial" does not exist".

Attached to his email is an extension that I hacked quickly at some point, for
experimental purposes only. It's pretty raw but may be useful if you just want
to play with it.

> 4. I am not sure about the code changes related to pushing partial
> aggregate on the remote side. Basically, you are pushing a whole aggregate
> on the remote side and result of that is treated as partial on the
> basis of aggtype = transtype. But I am not sure that is correct unless
> I miss something here. The type may be same but the result may not.

You're right. I mostly used sum() and count() in my examples but these are
special cases. It should also be checked whether aggfinalfn exists or
not. I'll fix it, thanks.

> 5. There are lot many TODO comments in the patch-set, are you working
> on those?

Yes. I wasn't too eager to complete all the TODOs soon because reviews of the
partch may result in a major rework. And if large parts of the code will have
to be wasted, some / many TODOs will be gone as well.

> Thanks
> 
> On Thu, Aug 17, 2017 at 8:52 PM, Antonin Houska  wrote:
> 
>  Antonin Houska  wrote:
> 
>  > Antonin Houska  wrote:
>  >
>  > > This is a new version of the patch I presented in [1].
>  >
>  > Rebased.
>  >
>  > cat .git/refs/heads/master
>  > b9a3ef55b253d885081c2d0e9dc45802cab71c7b
> 
>  This is another version of the patch.
> 
>  Besides other changes, it enables the aggregation push-down for postgres_fdw,
>  although only for aggregates whose transient aggregation state is equal to 
> the
>  output type. For other aggregates (like avg()) the remote nodes will have to
>  return the transient state value in an appropriate form (maybe bytea type),
>  which does not depend on PG version.
> 
>  shard.tgz demonstrates the typical postgres_fdw use case. One query shows 
> base
>  scans of base relation's partitions being pushed to shard nodes, the other
>  pushes down a join and performs aggregation of the join result on the remote
>  node. Of course, the query can only references one particular partition, 
> until
>  the "partition-wise join" [1] patch gets committed and merged with this my
>  patch.
> 
>  One thing I'm not sure about is whether the patch should remove
>  GetForeignUpperPaths function from FdwRoutine, which it essentially makes
>  obsolete. Or should it only be deprecated so far? I understand that
>  deprecation is important for "ordinary SQL users", but FdwRoutine is an
>  interface for extension developers, so the policy might be different.
> 
>  [1] https://commitfest.postgresql.org/14/994/
> 
>  Any feedback is appreciated.
> 
>  --
>  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
> 
> -- 
> Jeevan Chalke
> Principal Software Engineer, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
> 
> 

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



partial.tgz
Description: GNU Zip compressed 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] Red-Black tree traversal tests

[Thomas Munro]

On Fri, Sep 8, 2017 at 9:03 PM, Victor Drobny  wrote:
> Thank you very much for your review. In the attachment you can find v2 of
> the patch.

FYI this version crashes for me:

test test_rbtree  ... FAILED (test process exited with exit code 2)

It's trying to call rb->combiner which is null.

(lldb) bt
* thread #1: tid = 0x, 0x, stop reason = signal SIGSTOP
frame #0: 0x
  * frame #1: 0x00010c6fd9e0
postgres`rb_insert(rb=0x7fe7e2029850, data=0x7fff5380aa10,
isNew="") + 128 at rbtree.c:419
frame #2: 0x00010cffbfb9
test_rbtree.so`testdelete(size=10, delsize=1) + 425 at
test.c:558
frame #3: 0x00010cffb298
test_rbtree.so`testrbtree(fcinfo=0x7fe7e200d9a8) + 104 at
test.c:630
frame #4: 0x00010c6a03be
postgres`ExecInterpExpr(state=0x7fe7e200d8c0,
econtext=0x7fe7e200d570, isnull="") + 2702 at execExprInterp.c:672
frame #5: 0x00010c6e005b
postgres`ExecEvalExprSwitchContext(state=0x7fe7e200d8c0,
econtext=0x7fe7e200d570, isNull="") + 59 at executor.h:309
frame #6: 0x00010c6dffee
postgres`ExecProject(projInfo=0x7fe7e200d8b8) + 78 at
executor.h:343
frame #7: 0x00010c6dfd5c
postgres`ExecResult(pstate=0x7fe7e200d458) + 332 at
nodeResult.c:136
frame #8: 0x00010c6b2912
postgres`ExecProcNodeFirst(node=0x7fe7e200d458) + 82 at
execProcnode.c:430
frame #9: 0x00010c6af352
postgres`ExecProcNode(node=0x7fe7e200d458) + 50 at executor.h:251
frame #10: 0x00010c6ab0f6
postgres`ExecutePlan(estate=0x7fe7e200d240,
planstate=0x7fe7e200d458, use_parallel_mode='\0',
operation=CMD_SELECT, sendTuples='\x01', numberTuples=0,
direction=ForwardScanDirection, dest=0x7fe7e200aa20,
execute_once='\x01') + 182 at execMain.c:1720
frame #11: 0x00010c6aafcb
postgres`standard_ExecutorRun(queryDesc=0x7fe7e2004040,
direction=ForwardScanDirection, count=0, execute_once='\x01') + 571 at
execMain.c:363
frame #12: 0x00010c6aad87
postgres`ExecutorRun(queryDesc=0x7fe7e2004040,
direction=ForwardScanDirection, count=0, execute_once='\x01') + 87 at
execMain.c:306
frame #13: 0x00010c8b5bf2
postgres`PortalRunSelect(portal=0x7fe7e240, forward='\x01',
count=0, dest=0x7fe7e200aa20) + 306 at pquery.c:932
frame #14: 0x00010c8b55ba
postgres`PortalRun(portal=0x7fe7e240,
count=9223372036854775807, isTopLevel='\x01', run_once='\x01',
dest=0x7fe7e200aa20, altdest=0x7fe7e200aa20, completionTag="")
+ 762 at pquery.c:773
frame #15: 0x00010c8b0f24
postgres`exec_simple_query(query_string="SELECT testrbtree(10);")
+ 1316 at postgres.c:1109
frame #16: 0x00010c8b0127 postgres`PostgresMain(argc=1,
argv=0x7fe7e180bd10, dbname="contrib_regression",
username="munro") + 2375 at postgres.c:4103
frame #17: 0x00010c7f712e
postgres`BackendRun(port=0x7fe7e0d00db0) + 654 at
postmaster.c:4357
frame #18: 0x00010c7f64b3
postgres`BackendStartup(port=0x7fe7e0d00db0) + 483 at
postmaster.c:4029
frame #19: 0x00010c7f54a5 postgres`ServerLoop + 597 at postmaster.c:1753
frame #20: 0x00010c7f2c91 postgres`PostmasterMain(argc=8,
argv=0x7fe7e0c03860) + 5553 at postmaster.c:1361
frame #21: 0x00010c716799 postgres`main(argc=8,
argv=0x7fe7e0c03860) + 761 at main.c:228
frame #22: 0x7fff8333a5ad libdyld.dylib`start + 1
(lldb) f 1
frame #1: 0x00010c6fd9e0 postgres`rb_insert(rb=0x7fe7e2029850,
data=0x7fff5380aa10, isNew="") + 128 at rbtree.c:419
   416 /*
   417 * Found node with given key.  Apply combiner.
   418 */
-> 419 rb->combiner(current, data, rb->arg);
   420 *isNew = false;
   421 return current;
   422 }
(lldb) print *rb
(RBTree) $2 = {
  root = 0x7fe7e4419b60
  node_size = 40
  comparator = 0x00010cffc310 (test_rbtree.so`cmp at int_rbtree.h:28)
  combiner = 0x
  allocfunc = 0x00010cffc340 (test_rbtree.so`alloc at int_rbtree.h:37)
  freefunc = 0x00010cffc370 (test_rbtree.so`fr at int_rbtree.h:45)
  arg = 0x
}

-- 
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] Partition-wise aggregation/grouping

[Jeevan Chalke]

On Wed, Aug 23, 2017 at 4:43 PM, Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:

> Hi,
>
> Attached is the patch to implement partition-wise aggregation/grouping.
>
> As explained earlier, we produce a full aggregation for each partition when
> partition keys are leading group by clauses and then append is performed.
> Else we do a partial aggregation on each partition, append them and then
> add
> finalization step over it.
>
> I have observed that cost estimated for partition-wise aggregation and cost
> for the plans without partition-wise aggregation is almost same. However,
> execution time shows significant improvement (as explained my in the very
> first email) with partition-wise aggregates. Planner chooses a plan
> according
> to the costs, and thus most of the time plan without partition-wise
> aggregation is chosen. Hence, to force partition-wise plans and for the
> regression runs, I have added a GUC named partition_wise_agg_cost_factor to
> adjust the costings.
>
> This feature is only used when enable_partition_wise_agg GUC is set to on.
>
> Here are the details of the patches in the patch-set:
>

Here are the new patch-set re-based on HEAD (f0a0c17) and
latest partition-wise join (v29) patches.


>
> 0001 - Refactors sort and hash final grouping paths into separate
> functions.
> Since partition-wise aggregation too builds paths same as that of
> create_grouping_paths(), separated path creation for sort and hash agg into
> separate functions. These functions later used by main partition-wise
> aggregation/grouping patch.
>
> 0002 - Passes targetlist to get_number_of_groups().
> We need to estimate groups for individual child relations and thus need to
> pass targetlist corresponding to the child rel.
>
> 0003 - Adds enable_partition_wise_agg and partition_wise_agg_cost_factor
> GUCs.
>
> 0004 - Implements partition-wise aggregation.
>
> 0005 - Adds test-cases.
>
> 0006 - postgres_fdw changes which enable pushing aggregation for other
> upper
> relations.
>

0007 - Provides infrastructure to allow partial aggregation
This will allow us to push the partial aggregation over fdw.
With this one can write SUM(PARTIAL x) to get a partial sum
result. Since PARTIAL is used in syntax, I need to move that
to a reserved keywords category. This is kind of PoC patch
and needs input over approach and the way it is implemented.

0008 - Teaches postgres_fdw to push partial aggregation
With this we can push aggregate on remote server when
GROUP BY key does not match with the PARTITION key too.


>
>
> Since this patch is highly dependent on partition-wise join [1], one needs
> to
> apply all those patches on HEAD (my repository head was at:
> 66ed3829df959adb47f71d7c903ac59f0670f3e1) before applying these patches in
> order.
>
> Suggestions / feedback / inputs ?
>
> [1] https://www.postgresql.org/message-id/CAFjFpRd9Vqh_=-Ldv-
> XqWY006d07TJ+VXuhXCbdj=p1juky...@mail.gmail.com
>
>
>
-- 
Jeevan Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company


partition-wise-agg-v2.tar.gz
Description: GNU Zip compressed 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] UPDATE of partition key

[amul sul]

On Thu, May 18, 2017 at 9:13 AM, Amit Kapila 
wrote:

>  On Wed, May 17, 2017 at 5:17 PM, Robert Haas 
> wrote:
> > On Wed, May 17, 2017 at 6:29 AM, Amit Kapila 
> wrote:
> >> I think we can do this even without using an additional infomask bit.
> >> As suggested by Greg up thread, we can set InvalidBlockId in ctid to
> >> indicate such an update.
> >
> > Hmm.  How would that work?
> >
>
> We can pass a flag say row_moved (or require_row_movement) to
> heap_delete which will in turn set InvalidBlockId in ctid instead of
> setting it to self. Then the ExecUpdate needs to check for the same
> and return an error when heap_update is not successful (result !=
> HeapTupleMayBeUpdated).  Can you explain what difficulty are you
> envisioning?
>
>
Attaching WIP patch incorporates the above logic, although I am yet to check
all the code for places which might be using ip_blkid.  I have got a small
query here,
do we need an error on HeapTupleSelfUpdated case as well?

Note that patch should be applied to the top of Amit Khandekar's latest
patch(v17_rebased).

Regards,
Amul


0002-invalidate-ctid.ip_blkid-WIP.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] Parallel Append implementation

[Rafia Sabih]

On Wed, Aug 30, 2017 at 5:32 PM, Amit Khandekar  wrote:
> Hi Rafia,
>
> On 17 August 2017 at 14:12, Amit Khandekar  wrote:
>> But for all of the cases here, partial
>> subplans seem possible, and so even on HEAD it executed Partial
>> Append. So between a Parallel Append having partial subplans and a
>> Partial Append having partial subplans , the cost difference would not
>> be significant. Even if we assume that Parallel Append was chosen
>> because its cost turned out to be a bit cheaper, the actual
>> performance gain seems quite large as compared to the expected cost
>> difference. So it might be even possible that the performance gain
>> might be due to some other reasons. I will investigate this, and the
>> other queries.
>>
>
> I ran all the queries that were showing performance benefits in your
> run. But for me, the ParallelAppend benefits are shown only for plans
> that use Partition-Wise-Join.
>
> For all the queries that use only PA plans but not PWJ plans, I got
> the exact same plan for HEAD as for PA+PWJ patch, except that for the
> later, the Append is a ParallelAppend. Whereas, for you, the plans
> have join-order changed.
>
> Regarding actual costs; consequtively, for me the actual-cost are more
> or less the same for HEAD and PA+PWJ. Whereas, for your runs, you have
> quite different costs naturally because the plans themselves are
> different on head versus PA+PWJ.
>
> My PA+PWJ plan outputs (and actual costs) match exactly what you get
> with PA+PWJ patch. But like I said, I get the same join order and same
> plans (and actual costs) for HEAD as well (except
> ParallelAppend=>Append).
>
> May be, if you have the latest HEAD code with your setup, you can
> yourself check some of the queries again to see if they are still
> seeing higher costs as compared to PA ? I suspect that some changes in
> latest code might be causing this discrepancy; because when I tested
> some of the explains with a HEAD-branch server running with your
> database, I got results matching PA figures.
>
> Attached is my explain-analyze outputs.
>

Now, when I compare your results with the ones I posted I could see
one major difference between them -- selectivity estimation errors.
In the results I posted, e.g. Q3, on head it gives following

->  Finalize GroupAggregate  (cost=41131358.89..101076015.45
rows=455492628 width=44) (actual time=126436.642..129247.972
rows=226765 loops=1)
   Group Key: lineitem_001.l_orderkey,
orders_001.o_orderdate, orders_001.o_shippriority
   ->  Gather Merge  (cost=41131358.89..90637642.73
rows=379577190 width=44) (actual time=126436.602..127791.768
rows=235461 loops=1)
 Workers Planned: 2
 Workers Launched: 2

and in your results it is,
->  Finalize GroupAggregate  (cost=4940619.86..6652725.07
rows=13009521 width=44) (actual time=89573.830..91956.956 rows=226460
loops=1)
   Group Key: lineitem_001.l_orderkey,
orders_001.o_orderdate, orders_001.o_shippriority
   ->  Gather Merge  (cost=4940619.86..6354590.21
rows=10841268 width=44) (actual time=89573.752..90747.393 rows=235465
loops=1)
 Workers Planned: 2
 Workers Launched: 2

However, for the results with the patch/es this is not the case,

in my results, with patch,

 ->  Finalize GroupAggregate  (cost=4933450.21..663.01
rows=12899766 width=44) (actual time=87250.039..90593.716 rows=226765
loops=1)
   Group Key: lineitem_001.l_orderkey,
orders_001.o_orderdate, orders_001.o_shippriority
   ->  Gather Merge  (cost=4933450.21..6335491.38
rows=10749804 width=44) (actual time=87250.020..89125.279 rows=227291
loops=1)
 Workers Planned: 2
 Workers Launched: 2

I think this explains the reason for drastic different in the plan
choices and thus the performance for both the cases.

Since I was using same database for the cases, I don't have much
reasons for such difference in selectivity estimation for these
queries. The only thing might be a missing vacuum analyse, but since I
checked it a couple of times I am not sure if even that could be the
reason. Additionally, it is not the case for all the queries, like in
Q10 and Q21, the estimates are similar.

However, on a fresh database the selectivity-estimates and plans as
reported by you and with the patched version I posted seems to be the
correct one. I'll see if I may check performance of these queries once
again to verify these.

-- 
Regards,
Rafia Sabih
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] Block level parallel vacuum WIP

[Masahiko Sawada]

On Tue, Aug 15, 2017 at 10:13 AM, Masahiko Sawada  wrote:
> On Wed, Jul 26, 2017 at 5:38 PM, Masahiko Sawada  
> wrote:
>> On Sun, Mar 5, 2017 at 4:09 PM, Masahiko Sawada  
>> wrote:
>>> On Sun, Mar 5, 2017 at 12:14 PM, David Steele  wrote:
 On 3/4/17 9:08 PM, Masahiko Sawada wrote:
> On Sat, Mar 4, 2017 at 5:47 PM, Robert Haas  wrote:
>> On Fri, Mar 3, 2017 at 9:50 PM, Masahiko Sawada  
>> wrote:
>>> Yes, it's taking a time to update logic and measurement but it's
>>> coming along. Also I'm working on changing deadlock detection. Will
>>> post new patch and measurement result.
>>
>> I think that we should push this patch out to v11.  I think there are
>> too many issues here to address in the limited time we have remaining
>> this cycle, and I believe that if we try to get them all solved in the
>> next few weeks we're likely to end up getting backed into some choices
>> by time pressure that we may later regret bitterly.  Since I created
>> the deadlock issues that this patch is facing, I'm willing to try to
>> help solve them, but I think it's going to require considerable and
>> delicate surgery, and I don't think doing that under time pressure is
>> a good idea.
>>
>> From a fairness point of view, a patch that's not in reviewable shape
>> on March 1st should really be pushed out, and we're several days past
>> that.
>>
>
> Agreed. There are surely some rooms to discuss about the design yet,
> and it will take long time. it's good to push this out to CF2017-07.
> Thank you for the comment.

 I have marked this patch "Returned with Feedback."  Of course you are
 welcome to submit this patch to the 2017-07 CF, or whenever you feel it
 is ready.
>>>
>>> Thank you!
>>>
>>
>> I re-considered the basic design of parallel lazy vacuum. I didn't
>> change the basic concept of this feature and usage, the lazy vacuum
>> still executes with some parallel workers. In current design, dead
>> tuple TIDs are shared with all vacuum workers including leader process
>> when table has index. If we share dead tuple TIDs, we have to make two
>> synchronization points: before starting vacuum and before clearing
>> dead tuple TIDs. Before starting vacuum we have to make sure that the
>> dead tuple TIDs are not added no more. And before clearing dead tuple
>> TIDs we have to make sure that it's used no more.
>>
>> For index vacuum, each indexes is assigned to a vacuum workers based
>> on ParallelWorkerNumber. For example, if a table has 5 indexes and
>> vacuum with 2 workers, the leader process and one vacuum worker are
>> assigned to 2 indexes, and another vacuum process is assigned the
>> remaining one. The following steps are how the parallel vacuum
>> processes if table has indexes.
>>
>> 1. The leader process and workers scan the table in parallel using
>> ParallelHeapScanDesc, and collect dead tuple TIDs to shared memory.
>> 2. Before vacuum on table, the leader process sort the dead tuple TIDs
>> in physical order once all workers completes to scan the table.
>> 3. In vacuum on table, the leader process and workers reclaim garbage
>> on table in block-level parallel.
>> 4. In vacuum on indexes, the indexes on table is assigned to
>> particular parallel worker or leader process. The process assigned to
>> a index vacuums on the index.
>> 5. Before back to scanning the table, the leader process clears the
>> dead tuple TIDs once all workers completes to vacuum on table and
>> indexes.
>>
>> Attached the latest patch but it's still PoC version patch and
>> contains some debug codes. Note that this patch still requires another
>> patch which moves the relation extension lock out of heavy-weight
>> lock[1]. The parallel lazy vacuum patch could work even without [1]
>> patch but could fail during vacuum in some cases.
>>
>> Also, I attached the result of performance evaluation. The table size
>> is approximately 300MB ( > shared_buffers) and I deleted tuples on
>> every blocks before execute vacuum so that vacuum visits every blocks.
>> The server spec is
>> * Intel Xeon E5620 @ 2.4Ghz (8cores)
>> * 32GB RAM
>> * ioDrive
>>
>> According to the result of table with indexes, performance of lazy
>> vacuum improved up to a point where the number of indexes and parallel
>> degree are the same. If a table has 16 indexes and vacuum with 16
>> workers, parallel vacuum  is 10x faster than single process execution.
>> Also according to the result of table with no indexes, the parallel
>> vacuum is 5x faster than single process execution at 8 parallel
>> degree. Of course we can vacuum only for indexes
>>
>> I'm planning to work on that in PG11, will register it to next CF.
>> Comment and feedback are very welcome.
>>
>
> Since the previous patch conflicts with current HEAD I 

Re: [HACKERS] Parallel Append implementation

[Amit Khandekar]

On 7 September 2017 at 11:05, Amit Kapila  wrote:
> On Thu, Aug 31, 2017 at 12:47 PM, Amit Khandekar  
> wrote:
>> The last updated patch needs a rebase. Attached is the rebased version.
>>
>
> Few comments on the first read of the patch:

Thanks !

>
> 1.
> @@ -279,6 +347,7 @@ void
>  ExecReScanAppend(AppendState *node)
>  {
>   int i;
> + ParallelAppendDesc padesc = node->as_padesc;
>
>   for (i = 0; i < node->as_nplans; i++)
>   {
> @@ -298,6 +367,276 @@ ExecReScanAppend(AppendState *node)
>   if (subnode->chgParam == NULL)
>   ExecReScan(subnode);
>   }
> +
> + if (padesc)
> + {
> + padesc->pa_first_plan = padesc->pa_next_plan = 0;
> + memset(padesc->pa_finished, 0, sizeof(bool) * node->as_nplans);
> + }
> +
>
> For rescan purpose, the parallel state should be reinitialized via
> ExecParallelReInitializeDSM.  We need to do that way mainly to avoid
> cases where sometimes in rescan machinery we don't perform rescan of
> all the nodes.  See commit 41b0dd987d44089dc48e9c70024277e253b396b7.

Right. I didn't notice this while I rebased my patch over that commit.
Fixed it. Also added an exec_append_parallel_next() call in
ExecAppendReInitializeDSM(), otherwise the next ExecAppend() in leader
will get an uninitialized as_whichplan.

>
> 2.
> + * shared next_subplan counter index to start looking for unfinished plan,

Done.

>
> Here using "counter index" seems slightly confusing. I think using one
> of those will be better.

Re-worded it a bit. See whether that's what you wanted.

>
> 3.
> +/* 
> + * exec_append_leader_next
> + *
> + * To be used only if it's a parallel leader. The backend should scan
> + * backwards from the last plan. This is to prevent it from taking up
> + * the most expensive non-partial plan, i.e. the first subplan.
> + * 
> + */
> +static bool
> +exec_append_leader_next(AppendState *state)
>
> From above explanation, it is clear that you don't want backend to
> pick an expensive plan for a leader, but the reason for this different
> treatment is not clear.

Explained it, saying that for more workers, a leader spends more work
in processing the worker tuples , and less work contributing to
parallel processing. So it should not take expensive plans, otherwise
it will affect the total time to finish Append plan.

>
> 4.
> accumulate_partialappend_subpath()
> {
> ..
> + /* Add partial subpaths, if any. */
> + return list_concat(partial_subpaths, apath_partial_paths);
> ..
> + return partial_subpaths;
> ..
> + if (is_partial)
> + return lappend(partial_subpaths, subpath);
> ..
> }
>
> In this function, instead of returning from multiple places
> partial_subpaths list, you can just return it at the end and in all
> other places just append to it if required.  That way code will look
> more clear and simpler.

Agreed. Did it that way.

>
> 5.
>  * is created to represent the case that a relation is provably empty.
> + *
>   */
>  typedef struct AppendPath
>
> Spurious line addition.
Removed.

>
> 6.
> if (!node->as_padesc)
> {
> /*
> * This is Parallel-aware append. Follow it's own logic of choosing
> * the next subplan.
> */
> if (!exec_append_seq_next(node))
>
> I think this is the case of non-parallel-aware appends, but the
> comments are indicating the opposite.

Yeah, this comment got left over there when the relevant code got
changed. Shifted that comment upwards where it is appropriate.

Attached is the updated patch v14.

-- 
Thanks,
-Amit Khandekar
EnterpriseDB Corporation
The Postgres Database Company


ParallelAppend_v14.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] expanding inheritance in partition bound order

[Amit Langote]

On 2017/09/05 14:11, Amit Khandekar wrote:
> Great, thanks. Just wanted to make sure someone is working on that,
> because, as you said, it is no longer an EIBO patch. Since you are
> doing that, I won't work on that.

Here is that patch (actually two patches).  Sorry it took me a bit.

Description:

[PATCH 1/2] Decouple RelationGetPartitionDispatchInfo() from executor

Currently it and the structure it generates viz. PartitionDispatch
objects are too coupled with the executor's tuple-routing code.  In
particular, it's pretty undesirable that it makes it the responsibility
of the caller to release some resources, such as executor tuple table
slots, tuple-conversion maps, etc.  After this refactoring,
ExecSetupPartitionTupleRouting() now needs to
do some of the work that was previously done in
RelationGetPartitionDispatchInfo().

[PATCH 2/2] Make RelationGetPartitionDispatch expansion order
 depth-first

This is so as it matches what the planner is doing with partitioning
inheritance expansion.  Matching with planner order helps because
it helps ease matching the executor's per-partition objects with
planner-created per-partition nodes.


Actually, I'm coming to a conclusion that we should keep any
whole-partition-tree stuff out of partition.c and its interface, as Robert
has also alluded to in an earlier message on this thread [1].  But since
that's a different topic, I'll shut up about it on this thread and start a
new thread to discuss what kind of code rearrangement is possible.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoafr%3DhUrM%3Dcbx-k%3DBDHOF2OfXaw95HQSNAK4mHBwmSjtw%40mail.gmail.com
From 6956ac321df169f6c26c383ddcb5ea48c1a0071b Mon Sep 17 00:00:00 2001
From: amit 
Date: Wed, 30 Aug 2017 10:02:05 +0900
Subject: [PATCH 1/3] Decouple RelationGetPartitionDispatchInfo() from executor

Currently it and the structure it generates viz. PartitionDispatch
objects are too coupled with the executor's tuple-routing code.  In
particular, it's pretty undesirable that it makes it the responsibility
of the caller to release some resources, such as executor tuple table
slots, tuple-conversion maps, etc.  That makes it harder to use in
places other than where it's currently being used.

After this refactoring, ExecSetupPartitionTupleRouting() now needs to
do some of the work that was previously done in
RelationGetPartitionDispatchInfo().
---
 src/backend/catalog/partition.c| 53 +++-
 src/backend/commands/copy.c| 32 +++--
 src/backend/executor/execMain.c| 88 ++
 src/backend/executor/nodeModifyTable.c | 37 +++---
 src/include/catalog/partition.h| 20 +++-
 src/include/executor/executor.h|  4 +-
 src/include/nodes/execnodes.h  | 40 +++-
 7 files changed, 181 insertions(+), 93 deletions(-)

diff --git a/src/backend/catalog/partition.c b/src/backend/catalog/partition.c
index c6bd02f77d..4f594243d3 100644
--- a/src/backend/catalog/partition.c
+++ b/src/backend/catalog/partition.c
@@ -1061,7 +1061,6 @@ RelationGetPartitionDispatchInfo(Relation rel,
Relationpartrel = lfirst(lc1);
Relationparent = lfirst(lc2);
PartitionKey partkey = RelationGetPartitionKey(partrel);
-   TupleDesc   tupdesc = RelationGetDescr(partrel);
PartitionDesc partdesc = RelationGetPartitionDesc(partrel);
int j,
m;
@@ -1069,29 +1068,12 @@ RelationGetPartitionDispatchInfo(Relation rel,
pd[i] = (PartitionDispatch) 
palloc(sizeof(PartitionDispatchData));
pd[i]->reldesc = partrel;
pd[i]->key = partkey;
-   pd[i]->keystate = NIL;
pd[i]->partdesc = partdesc;
if (parent != NULL)
-   {
-   /*
-* For every partitioned table other than root, we must 
store a
-* tuple table slot initialized with its tuple 
descriptor and a
-* tuple conversion map to convert a tuple from its 
parent's
-* rowtype to its own. That is to make sure that we are 
looking at
-* the correct row using the correct tuple descriptor 
when
-* computing its partition key for tuple routing.
-*/
-   pd[i]->tupslot = MakeSingleTupleTableSlot(tupdesc);
-   pd[i]->tupmap = 
convert_tuples_by_name(RelationGetDescr(parent),
-   
   tupdesc,
-   
   gettext_noop("could not convert row type"));
-   }
+   pd[i]->parentoid = 

Re: [HACKERS] UPDATE of partition key

[Amit Langote]

On 2017/09/08 18:57, Robert Haas wrote:
>> As mentioned by Amit Langote in the above mail thread, he is going to
>> do changes for making RelationGetPartitionDispatchInfo() return the
>> leaf partitions in depth-first order. Once that is done, I will then
>> remove the hash table method for finding leaf partitions in update
>> result rels, and instead use the earlier efficient method that takes
>> advantage of the fact that update result rels and leaf partitions are
>> in the same order.
> 
> Has he posted that patch yet?  I don't think I saw it, but maybe I
> missed something.

I will post on that thread in a moment.

Thanks,
Amit



-- 
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] UPDATE of partition key

[Robert Haas]

On Thu, Sep 7, 2017 at 6:17 AM, Amit Khandekar  wrote:
> On 3 September 2017 at 17:10, Amit Khandekar  wrote:
>> After recent commit 30833ba154, now the partitions are expanded in
>> depth-first order. It didn't seem worthwhile rebasing my partition
>> walker changes onto the latest code. So in the attached patch, I have
>> removed all the partition walker changes. But
>> RelationGetPartitionDispatchInfo() traverses in breadth-first order,
>> which is different than the update result rels order (because
>> inheritance expansion order is depth-first). So, in order to make the
>> tuple-routing-related leaf partitions in the same order as that of the
>> update result rels, we would have to make changes in
>> RelationGetPartitionDispatchInfo(), which I am not sure whether it is
>> going to be done as part of the thread "expanding inheritance in
>> partition bound order" [1]. For now, in the attached patch, I have
>> reverted back to the hash table method to find the leaf partitions in
>> the update result rels.
>>
>> [1] 
>> https://www.postgresql.org/message-id/CAJ3gD9eyudCNU6V-veMme%2BeyzfX_ey%2BgEzULMzOw26c3f9rzdg%40mail.gmail.com
>
> As mentioned by Amit Langote in the above mail thread, he is going to
> do changes for making RelationGetPartitionDispatchInfo() return the
> leaf partitions in depth-first order. Once that is done, I will then
> remove the hash table method for finding leaf partitions in update
> result rels, and instead use the earlier efficient method that takes
> advantage of the fact that update result rels and leaf partitions are
> in the same order.

Has he posted that patch yet?  I don't think I saw it, but maybe I
missed something.

-- 
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] CLUSTER command progress monitor

[Robert Haas]

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.

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

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

[Robert Haas]

On Fri, Sep 8, 2017 at 1:17 AM, Amit Kapila  wrote:
> You are right.  I have changed the ordering and passed OuterUserId via
> FixedParallelState.

This looks a little strange:

+SetCurrentRoleId(fps->outer_user_id, fps->is_current_user_superuser);

The first argument says "outer" but the second says "current".  I'm
wondering if we can just make the second one is_superuser.

I'm also wondering if, rather than using GetConfigOptionByName, we
should just make the GUC underlying is_superuser non-static and use
the value directly.  If not, then I'm alternatively wondering whether
we should maybe use a less-generic name than varval.

-- 
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] Partition-wise join for join between (declaratively) partitioned tables

[Robert Haas]

On Fri, Sep 8, 2017 at 1:47 AM, Amit Langote
 wrote:
> When I tried the attached patch, it doesn't seem to expand partitioning
> inheritance in step-wise manner as the patch's title says.  I think the
> rewritten patch forgot to include Ashutosh's changes to
> expand_single_inheritance_child() whereby the AppendRelInfo of the child
> will be marked with the direct parent instead of always the root parent.

Woops.

> I updated the patch to include just those changes.  I'm not sure about
> one of the Ashutosh's changes whereby the child PlanRowMark is also passed
> to expand_partitioned_rtentry() to use as the parent PlanRowMark.  I think
> the child RTE, child RT index and child Relation are fine, because they
> are necessary for creating AppendRelInfos in a desired way for later
> planning steps.  But PlanRowMarks are not processed within the planner
> afterwards and do not need to be marked with the immediate parent-child
> association in the same way that AppendRelInfos need to be.

We probably need some better comments to explain which things need to
be marked using the immediate parent and which need to be marked using
the baserel, and why.

> I also included the changes to add_paths_to_append_rel() from my patch on
> the "path toward faster partition pruning" thread.  We'd need that change,
> because while add_paths_to_append_rel() is called for all partitioned
> table RTEs in a given partition tree, expand_inherited_rtentry() would
> have set up a PartitionedChildRelInfo only for the root parent, so
> get_partitioned_child_rels() would not find the same for non-root
> partitioned table rels and crash failing the Assert.  The changes I made
> are such that we call get_partitioned_child_rels() only for the parent
> rels that are known to correspond root partitioned tables (or as you
> pointed out on the thread, "the table named in the query" as opposed those
> added to the query as result of inheritance expansion).  In addition to
> the relkind check on the input RTE, it would seem that checking that the
> reloptkind is RELOPT_BASEREL would be enough.  But actually not, because
> if a partitioned table is accessed in a UNION ALL query, reloptkind even
> for the root partitioned table (the table named in the query) would be
> RELOPT_OTHER_MEMBER_REL.  The only way to confirm that the input rel is
> actually the root partitioned table is to check whether its parent rel is
> not RTE_RELATION, because the parent in case of UNION ALL Append is a
> RTE_SUBQUERY RT entry.

OK, so this needs some good comments, too...

-- 
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] GnuTLS support

[Robert Haas]

On Thu, Sep 7, 2017 at 10:35 PM, Tom Lane  wrote:
> I think we might be best off just playing it straight and providing
> a config file that contains a section along these lines:
>
> # Parameters for OpenSSL.  Leave these commented out if not using OpenSSL.
> #
> #ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL' # allowed SSL ciphers
> #ssl_prefer_server_ciphers = on
> #ssl_ecdh_curve = 'prime256v1'
> #ssl_dh_params_file = ''
> #ssl_cert_file = 'server.crt'
> #ssl_key_file = 'server.key'
> #ssl_ca_file = ''
> #ssl_crl_file = ''
> #
> # Parameters for GnuTLS.  Leave these commented out if not using GnuTLS.
> #
> #gnufoo=...
> #gnubar=...
> #
> # Parameters for macOS TLS.  ... you get the idea.
>
> As previously noted, it'd be a good idea to rename the existing
> ssl_xxx parameters to openssl_xxx, except maybe ones that we think
> will be universal.  (But even if we do think that, it might be
> simpler in the long run to just have three or four totally independent
> sections of the config file, instead of some common and some library-
> specific parameters.)

+1 to all of that.

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

[Victor Drobny]

Dear Tom,

Thank you very much for your review. In the attachment you can find v2 
of the patch.


On 2017-09-07 01:38, Tom Lane wrote:

[ btw, please don't cc pgsql-hackers-owner, the list moderators don't
need the noise ]

Aleksander Alekseev  writes:

I would say that this patch is in a pretty good shape now. And I see a
99% code coverage of rbtree.c. Let's see what committers think.


I took a quick look through the patch --- haven't tried to compile it
or anything like that --- and have a few comments:

* There's some typos, eg extention should be extension, triversal
should be traversal.  Maybe try a spell checker?


Done. I fixed all spelling mistakes that i found.



* The diff complains about lack of file-ending newlines in several
places

* There's something weird at the start of test.c:

@@ -0,0 +1,577 @@
+/*--

Maybe your compiler thinks that's a BOM?  It's hard to see how it
compiles otherwise.


Now it is in UTF-8 without BOM. It seems that there is no such data in 
the beginning

of the test.c


* I think it might be simpler to have the module expose just one SQL
function that invokes all these individual tests internally.  Less
boilerplate text that way, and less to change if you add more tests
later.  Also, you might consider passing in TEST_SIZE as an argument
of the SQL function instead of having it be hard-wired.

You are right. Done.


* We don't do C++-style (//) comments around here.  Please use C
style.  (You might consider running the code through pgindent,
which will convert those comments automatically.)


Fixed.



* It's also generally not per project style to use malloc/calloc/free
directly in backend code; and it's certainly not cool to use malloc or
calloc and then not check for a null result.  Use palloc instead.  
Given

the short runtime of this test, you likely needn't be too worried about
pfree'ing stuff.

* _PG_init() declaration seems to be a leftover?   doesn't
belong here either, as postgres.h will bring that in for you.

* I know next to zip about red-black trees, but it disturbs me that
the traversal tests use trees whose values were inserted in strictly
increasing order.  Seems like that's not proving as much as it could.
I wonder how hard it would be to insert those integers in random order.

* I'm not too pleased that the rb_find calls mostly just assume that
the find won't return NULL.  You should be testing for that and 
reporting

the problem, not just dying on a null pointer crash if it happens.


Done.



* Possibly the tests should exercise rb_delete on keys *not* present.
And how about insertion of already-existing keys?  Although that's
a bit outside the scope of testing traversal, so if you want to leave
it for some future expansion, that'd be OK.


Deletion requires to get pointer to the tree node. Otherwise it could 
break

the tree. It is mentioned in the description of the rb_delete function.
" * "node" must have previously been found via rb_find or rb_leftmost."

Insertion of the same elements is managed by the specific implementation
of the tree. One of the input arguments of the rb_create function is
combiner function that will be called in case of repeated insertion.

However, during looking through this i found that nobody checks that
combiner function(as well as comparator, freefunc and allocfunc) is
not NULL. So if it was not specified, postgres will fall. I think that
it is better to add this checks.



I'll set this back to Waiting on Author.  I do encourage you to finish
it up.

regards, tom lane


--
Victor Drobny
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Companydiff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index 3ce9904..b7ed0af 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -13,6 +13,7 @@ SUBDIRS = \
 		  test_extensions \
 		  test_parser \
 		  test_pg_dump \
+		  test_rbtree \
 		  test_rls_hooks \
 		  test_shm_mq \
 		  worker_spi
diff --git a/src/test/modules/test_rbtree/Makefile b/src/test/modules/test_rbtree/Makefile
new file mode 100644
index 000..4e53e86
--- /dev/null
+++ b/src/test/modules/test_rbtree/Makefile
@@ -0,0 +1,21 @@
+# src/test/modules/test_rbtree/Makefile
+
+MODULE_big = test_rbtree
+OBJS = test.o $(WIN32RES)
+PGFILEDESC = "test_rbtree - rbtree traversal testing"
+
+EXTENSION = test_rbtree
+DATA = test_rbtree--1.0.sql
+
+REGRESS = test_rbtree
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/test_rbtree
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
diff --git a/src/test/modules/test_rbtree/README b/src/test/modules/test_rbtree/README
new file mode 100644
index 000..40f586d
--- /dev/null
+++ b/src/test/modules/test_rbtree/README
@@ -0,0 +1,20 @@

Re: [HACKERS] Hash Functions

[amul sul]

On Fri, Sep 1, 2017 at 8:01 AM, Robert Haas  wrote:

> On Thu, Aug 31, 2017 at 8:40 AM, amul sul  wrote:
> > Fixed in the attached version.
>
> I fixed these up a bit and committed them.  Thanks.
>
> I think this takes care of adding not only the infrastructure but
> support for all the core data types, but I'm not quite sure how to
> handle upgrading types in contrib.  It looks like citext, hstore, and
> several data types provided by isn have hash opclasses, and I think
> that there's no syntax for adding a support function to an existing
> opclass.  We could add that, but I'm not sure how safe it would be.
>
> TBH, I really don't care much about fixing isn, but it seems like
> fixing citext and hstore would be worthwhile.
>

Attached patch proposes the fix for the citext and hstore contrib.

To make it easy to understand I've split these patch in two part.  0001 adds
a new file for the contrib upgrade & renames an existing file to the higher
version, and 0002 is the actual implementation of extended hash function for
that contrib's data type.

Regards,
Amul


0001-hstore-File-renaming-v1.patch
Description: Binary data


0002-hstore-add-extended-hash-function-v1.patch
Description: Binary data


0001-citext-File-renaming-v1.patch
Description: Binary data


0002-citext-add-extended-hash-function-v1.patch
Description: Binary data

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

Re: [HACKERS] code cleanup empty string initializations

[Aleksandr Parfenov]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

Hi Peter,

I looked through your patches and its look good to me.
Patches make code more readable and clear, especially in case of encodingid.

The new status of this patch is: Ready for Committer

-- 
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 8, 2017 at 12:34 AM, Robert Haas  wrote:
> On Tue, Sep 5, 2017 at 7:01 AM, Ashutosh Bapat
>  wrote:
>> accumulate_append_subpath() is executed for every path instead of
>> every relation, so changing it would collect the same list multiple
>> times. Instead, I found the old way of associating all intermediate
>> partitions with the root partitioned relation work better. Here's the
>> updated patch set.
>
> When I tried out patch 0001, it crashed repeatedly during 'make check'
> because of an assertion failure in get_partitioned_child_rels.

Running "make check" on the whole patchset doesn't give that failure.
So I didn't notice the crash since I was running regression on the
whole patchset. Sorry for that. Fortunately git rebase -i allows us to
execute shell commands while applying patches, so I have set it up to
compile each patch and run regression. Hopefully that will catch such
errors in future. That process showed me that patch
0003-In-add_paths_to_append_rel-get-partitioned_rels-for-.patch fixes
that crash by calling get_partitioned_child_rels() only on the root
partitioned table for which we have set up child_rels list. Amit
Langote has a similar fix reported in his reply. So, we will discuss
it there.

> It
> seemed to me that the way the patch was refactoring
> expand_inherited_rtentry involved more code rearrangement than
> necessary, so I reverted all the code rearrangement and just kept the
> functional changes, and all the crashes went away.  (That refactoring
> also failed to initialize has_child properly.)  In so doing, I
> reintroduced the problem that the PartitionedChildRelInfo lists
> weren't getting set up correctly, but after some thought I realized
> that was just because expand_single_inheritance_child() was choosing
> between adding an RTE and adding the OID to partitioned_child_rels,
> whereas for an intermediate partitioned table it needs to do both.  So
> I inserted a trivial fix for that problem (replacing "else" with a new
> "if"-test), basically:
>
> -else
> +
> +if (childrte->relkind == RELKIND_PARTITIONED_TABLE)
>
> Please check out the attached version of the patch.  In addition to
> the above simplifications, I did some adjustments to the comments in
> various places - some just grammar and others a bit more substantive.
> And I think I broke a long line in one place, too.
>
> One thing I notice is that if I rip out the changes to initsplan.c,
> the new regression test still passes.  If it's possible to write a
> test that fails without those changes, I think it would be a good idea
> to include one in the patch.  That's certainly one of the subtler
> parts of this patch, IMHO.

Amit Langote has replied on these points as well. So, I will comment
in a reply to his reply.

-- 
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] Re: proposal - using names as primary names of plpgsql function parameters instead $ based names

[Jeevan Chalke]

Hi Pavel,

On Sat, May 20, 2017 at 11:55 AM, Pavel Stehule 
wrote:

>
>
>
> 2017-05-19 5:48 GMT+02:00 Pavel Stehule :
>
>>
>>
>> 2017-05-19 3:14 GMT+02:00 Peter Eisentraut > com>:
>>
>>> On 5/15/17 14:34, Pavel Stehule wrote:
>>> > Now, I when I working on plpgsql_check, I have to check function
>>> > parameters. I can use fn_vargargnos and out_param_varno for list of
>>> > arguments and related varno(s). when I detect some issue, I am
>>> using
>>> > refname. It is not too nice now, because these refnames are $
>>> based.
>>> > Long names are alias only. There are not a possibility to find
>>> > related alias.
>>> >
>>> > So, my proposal. Now, we can use names as refname of parameter
>>> > variable. $ based name can be used as alias. From user perspective
>>> > there are not any change.
>>> >
>>> > Comments, notes?
>>> >
>>> > here is a patch
>>>
>>>
I like the idea of using parameter name instead of $n symbols.

However, I am slightly worried that, at execution time if we want to
know the parameter position in the actual function signature, then it
will become difficult to get that from the corresponding datum
variable. I don't have any use-case for that though. But apart from
this concern, idea looks good to me.

Here are review comments on the patch:

1.
+char   *argname = NULL;

There is no need to initialize argname here. The Later code does that.

2.
+argname = (argnames && argnames[i][0] != 0) ? argnames[i]
: NULL;

It will be better to check '\0' instead of 0, like we have that already.

3.
Check for argname exists is not consistent. At one place you have used
"argname != NULL" and other place it is "argname != '\0'".
Better to have "argname != NULL" at both the places.

4.
-- should fail -- message should to contain argument name
Should be something like this:
-- Should fail, error message should contain argument name

5.
+argvariable = plpgsql_build_variable(argname != NULL ?
+   argname : buf,
+   0, argdtype,
false);

Please correct indentation.

---

BTW, instead of doing all these changes, I have done these changes this way:

-   /* Build variable and add to datum list */
-   argvariable = plpgsql_build_variable(buf, 0,
-argdtype, false);
+   /*
+* Build variable and add to datum list.  If there's a name
for
+* the argument, then use that else use $n name.
+*/
+   argvariable = plpgsql_build_variable((argnames &&
argnames[i][0] != '\0') ?
+argnames[i] : buf,
+0, argdtype, false);

This requires no new variable and thus no more changes elsewhere.

Attached patch with these changes. Please have a look.

Thanks


-- 
Jeevan Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
diff --git a/src/pl/plpgsql/src/pl_comp.c b/src/pl/plpgsql/src/pl_comp.c
index e9d7ef5..dd17bb5 100644
--- a/src/pl/plpgsql/src/pl_comp.c
+++ b/src/pl/plpgsql/src/pl_comp.c
@@ -433,9 +433,13 @@ do_compile(FunctionCallInfo fcinfo,
 			 errmsg("PL/pgSQL functions cannot accept type %s",
 	format_type_be(argtypeid;
 
-/* Build variable and add to datum list */
-argvariable = plpgsql_build_variable(buf, 0,
-	 argdtype, false);
+/*
+ * Build variable and add to datum list.  If there's a name for
+ * the argument, then use that else use $n name.
+ */
+argvariable = plpgsql_build_variable((argnames && argnames[i][0] != '\0') ?
+	 argnames[i] : buf,
+	 0, argdtype, false);
 
 if (argvariable->dtype == PLPGSQL_DTYPE_VAR)
 {
diff --git a/src/test/regress/expected/plpgsql.out b/src/test/regress/expected/plpgsql.out
index 7109996..cf589d5 100644
--- a/src/test/regress/expected/plpgsql.out
+++ b/src/test/regress/expected/plpgsql.out
@@ -6029,3 +6029,17 @@ SELECT * FROM list_partitioned_table() AS t;
  2
 (2 rows)
 
+--
+-- Check argument name is used instead of $n
+--
+CREATE TYPE ct AS (a int, b int);
+-- Should fail, error message should contain argument name instead of $1
+CREATE OR REPLACE FUNCTION fx(x ct) RETURNS void AS $$
+BEGIN
+  GET DIAGNOSTICS x = ROW_COUNT;
+  RETURN;
+END; $$ LANGUAGE plpgsql;
+ERROR:  "x" is not a scalar variable
+LINE 3:   GET DIAGNOSTICS x = ROW_COUNT;
+  ^
+DROP TYPE ct;
diff --git a/src/test/regress/sql/plpgsql.sql b/src/test/regress/sql/plpgsql.sql
index 771d682..42f51e9 100644
--- a/src/test/regress/sql/plpgsql.sql
+++ b/src/test/regress/sql/plpgsql.sql
@@ -4811,3 +4811,17 @@ BEGIN
 END; $$ LANGUAGE 

Re: [HACKERS] WAL logging problem in 9.4.3?

[Kyotaro HORIGUCHI]

Thank you for your notification.

At Tue, 5 Sep 2017 12:05:01 +0200, Daniel Gustafsson  wrote in 

> > On 13 Apr 2017, at 11:42, Kyotaro HORIGUCHI 
> >  wrote:
> > 
> > At Thu, 13 Apr 2017 13:52:40 +0900, Michael Paquier 
> >  wrote in 
> > 
> >> On Tue, Apr 11, 2017 at 5:38 PM, Kyotaro HORIGUCHI
> >>  wrote:
> >>> Sorry, what I have just sent was broken.
> >> 
> >> You can use PROVE_TESTS when running make check to select a subset of
> >> tests you want to run. I use that all the time when working on patches
> >> dedicated to certain code paths.
> > 
> > Thank you for the information. Removing unwanted test scripts
> > from t/ directories was annoyance. This makes me happy.
> > 
>  - Relation has new members no_pending_sync and pending_sync that
>   works as instant cache of an entry in pendingSync hash.
>  - Commit-time synchronizing is restored as Michael's patch.
>  - If relfilenode is replaced, pending_sync for the old node is
>   removed. Anyway this is ignored on abort and meaningless on
>   commit.
>  - TAP test is renamed to 012 since some new files have been added.
>  
>  Accessing pending sync hash occurred on every calling of
>  HeapNeedsWAL() (per insertion/update/freeze of a tuple) if any of
>  accessing relations has pending sync.  Almost of them are
>  eliminated as the result.
> >> 
> >> Did you actually test this patch? One of the logs added makes the
> >> tests a long time to run:
> > 
> > Maybe this patch requires make clean since it extends the
> > structure RelationData. (Perhaps I saw the same trouble.)
> > 
> >> 2017-04-13 12:11:27.065 JST [85441] t/102_vacuumdb_stages.pl
> >> STATEMENT:  ANALYZE;
> >> 2017-04-13 12:12:25.766 JST [85492] LOG:  BufferNeedsWAL: pendingSyncs
> >> = 0x0, no_pending_sync = 0
> >> 
> >> -   lsn = XLogInsert(RM_SMGR_ID,
> >> -XLOG_SMGR_TRUNCATE | XLR_SPECIAL_REL_UPDATE);
> >> +   rel->no_pending_sync= false;
> >> +   rel->pending_sync = pending;
> >> +   }
> >> 
> >> It seems to me that those flags and the pending_sync data should be
> >> kept in the context of backend process and not be part of the Relation
> >> data...
> > 
> > I understand that the context of "backend process" means
> > storage.c local. I don't mind the context on which the data is,
> > but I found only there that can get rid of frequent hash
> > searching. For pending deletions, just appending to a list is
> > enough and costs almost nothing, on the other hand pendig syncs
> > are required to be referenced, sometimes very frequently.
> > 
> >> +void
> >> +RecordPendingSync(Relation rel)
> >> I don't think that I agree that this should be part of relcache.c. The
> >> syncs are tracked should be tracked out of the relation context.
> > 
> > Yeah.. It's in storage.c in the latest patch. (Sorry for the
> > duplicate name). I think it is a kind of bond between smgr and
> > relation.
> > 
> >> Seeing how invasive this change is, I would also advocate for this
> >> patch as only being a HEAD-only change, not many people are
> >> complaining about this optimization of TRUNCATE missing when wal_level
> >> = minimal, and this needs a very careful review.
> > 
> > Agreed.
> > 
> >> Should I code something? Or Horiguchi-san, would you take care of it?
> >> The previous crash I saw has been taken care of, but it's been really
> >> some time since I looked at this patch...
> > 
> > My point is hash-search on every tuple insertion should be evaded
> > even if it happens rearely. Once it was a bit apart from your
> > original patch, but in the latest patch the significant part
> > (pending-sync hash) is revived from the original one.
> 
> This patch has followed along since CF 2016-03, do we think we can reach a
> conclusion in this CF?  It was marked as "Waiting on Author”, based on
> developments since in this thread, I’ve changed it back to “Needs Review”
> again.

I manged to reload its context into my head. It doesn't apply on
the current master and needs some amendment. I'm going to work on
this.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

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

Re: [HACKERS] Support to COMMENT ON DATABASE CURRENT_DATABASE

[Surafel Temesgen]

On Fri, Aug 25, 2017 at 11:16 AM, Jing Wang  wrote:

> Hi all,
>
> Enclosed please find the updated patch with covering security labels on
> database.
>
> The patch cover the following commands:
>

i can't apply your patch cleanly i think it needs rebase

Regards

Surafel

Re: [HACKERS] log_destination=file

[Dmitry Dolgov]

> On 8 September 2017 at 01:32, Magnus Hagander  wrote:
>
> 1. where was stderr actually sent? To the console, to /dev/null or to a
file?

To the console (but I can also try other options, although I'm not sure if
it would have any impact).

> 2. Could you run the same thing (on the same machine) with the logging
collector on and logging to file, without the patch? I'd assume that gives
performance similar to running with the patch, but it would be good to
confirm that.

Sure, I'll do it this weekend.

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

[Michael Banck]

Hi,

Am Mittwoch, den 06.09.2017, 12:22 -0400 schrieb Peter Eisentraut:
> On 8/18/17 05:28, Michael Banck wrote:
> > > > Rebased, squashed and slighly edited version attached. I've added this
> > > > to the 2017-07 commitfest now as well:
> > > > 
> > > > https://commitfest.postgresql.org/14/1112/
> > > 
> > > Can you rebase this past some conflicting changes?
> > 
> > Thanks for letting me know, PFA a rebased version.
> 
> I have reviewed the thread so far.  I think there is general agreement
> that something like this would be good to have.
> 
> I have not found any explanation, however, why the "if not exists"
> behavior is desirable, let alone as the default.  I can only think of
> two workflows here:  Either you have scripts for previous PG versions
> that create the slot externally, in which can you omit --create, or you
> use the new functionality to have pg_basebackup create the slot.  I
> don't see any use for pg_basebackup to opportunistically use a slot if
> it happens to exist.  Even if there is one, it should not be the
> default.  So please change that.

Ok, I tried to research why that was the case and couldn't find any
trace of a discussion either.

So we should just error out in CreateReplicationSlot() in case a slot
exists, right? I think having yet another option like --create-if-not-
exists does not sound needed from what you wrote above.

> A minor point, I suggest to print the message about the replication slot
> being created *after* the slot has been created.  This aligns with how
> logical replication subscriptions work.  

Ok.

> I don't see the need for printing a message about temporary slots. 
> Since they are temporary, they will go away automatically, so there is
> nothing the user needs to know there.

Ok. I thought I'd remembered some request around having this reported
always (maybe from Magnus), but I couldn't find anything in the prior
discussions either.

If we don't print the message for temporary slots, then the
CreateReplicationSlot() refactoring and the addition of the
temp_replication_slot argument would be no longer needed, or is this
something useful on its own?


Thanks,

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


-- 
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] path toward faster partition pruning

[Amit Langote]

On 2017/09/08 4:41, Robert Haas wrote:
> On Thu, Sep 7, 2017 at 7:16 AM, Amit Langote
>  wrote:
>> There is a patch in the Ashutosh's posted series of patches, which does
>> more or less the same thing that this patch does.  He included it in his
>> series of patches, because, IIUC, the main partitionwise-join planning
>> logic that one of the later patch implements depends on being able to
>> consider applying that new planning technique individually for every
>> partition sub-tree, instead of just at the whole tree root.
>>
>> One notable difference from his patch is that while his patch will expand
>> in non-flattened manner even in the case where the parent is the result
>> relation of a query, my patch doesn't in that case, because the new
>> partition-pruning technique cannot be applied to inheritance parent that
>> is a result relation, for example,
>>
>> update partitioned_table set ...
>>
>> And AFAICS, partitionwise-join cannot be applied to such a parent either.
>>
>> Note however that if there are other instances of the same partitioned
>> table (in the FROM list of an update statement) or other partitioned
>> tables in the query, they will be expanded in a non-flattened manner,
>> because they are themselves not the result relations of the query.  So,
>> the new partition-pruning and (supposedly) partitionwise-join can be
>> applied for those other partitioned tables.
> 
> It seems to me that it would be better not to write new patches for
> things that already have patches without a really clear explanation
> with what's wrong with the already-existing patch; I don't see any
> such explanation here.  Instead of writing your own patch for this to
> duel with his his, why not review his and help him correct any
> deficiencies which you can spot?  Then we have one patch with more
> review instead of two patches with less review both of which I have to
> read and try to decide which is better.

Sorry, I think I should have just used the Ashutosh's patch.

> In this case, I think Ashutosh has the right idea.  I think that
> handling the result-relation and non-result-relation differently
> creates an unpleasant asymmetry.  With your patch, we have to deal
> with three cases: (a) partitioned tables that were expanded
> level-by-level because they are not result relations, (b) partitioned
> tables that were expanded "flattened" because they are result
> relations, and (c) non-partitioned tables that were expanded
> "flattened".  With Ashutosh's approach, we only have two cases to
> worry about in the future rather than three, and I like that better.

I tend to agree with this now.

> Your patch also appears to change things so that the table actually
> referenced in the query ends up with an AppendRelInfo for the parent,
> which seems pointless.

Actually, it wouldn't, because my patch also got rid of the notion of
adding the duplicate RTE for original parent, because I thought the
duplicate RTE was pointless in the partitioning case.

> There are a couple of hunks from your patch that we might want or need
> to incorporate into Ashutosh's patch.  The change to
> relation_excluded_by_constraints() looks like it might be useful,
> although it needs a better comment and some tests.

I think we could just drop that part from this patch.  It also looks like
Ashutosh has a patch elsewhere concerning this.

https://commitfest.postgresql.org/14/1108/

Maybe, we could discuss what do about this on that thread.  Now that not
only the root partitioned table, but also other partitioned tables in the
tree get an RTE with inh = true, I think it would be interesting to
consider his patch.

> Also, Ashutosh's
> patch has no equivalent of your change to add_paths_to_append_rel().
> I'm not clear what the code you've introduced there is supposed to be
> doing, and I'm suspicious that it is confusing "partition root" with
> "table named in the query", which will often be the same but not
> always; the user could have named an intermediate partition.  Can you
> expand on what this is doing?

I've replied on the partition-wise thread explaining why changes in the
add_paths_to_append_rel() are necessary.

Anyway, I'm dropping my patch in favor of the patch on the other thread.
Sorry for the duplicated effort involved in having to look at both the
patches.

Thanks,
Amit

[1]
https://www.postgresql.org/message-id/CA%2BTgmoZEUonD9dUZH1FBEyq%3DPEv_KvE3wC%3DA%3D0zm-_tRz_917A%40mail.gmail.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] [Proposal] Allow users to specify multiple tables in VACUUM commands

[Michael Paquier]

On Fri, Sep 8, 2017 at 7:27 AM, Bossart, Nathan  wrote:
> On 9/7/17, 2:33 AM, "Michael Paquier"  wrote:
>> Using the patch checking for duplicate columns:
>> =# create table aa (a int);
>> CREATE TABLE
>> =# vacuum ANALYZE aa(z, z);
>> ERROR:  0A000: column lists cannot have duplicate entries
>> HINT:  the column list specified for relation "aa" contains duplicates
>> LOCATION:  check_column_lists, vacuum.c:619
>> Shouldn't the priority be given to undefined columns instead of
>> duplicates? You may want to add a test for that as well.
>
> I agree.  I've fixed this and added a couple relevant tests cases in
> v2.

Thanks. This looks now correct to me. Except that:
+   ereport(ERROR,
+   (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+errmsg("column lists cannot have duplicate entries"),
+errhint("the column list specified for relation
\"%s\" contains duplicates",
+   relation->relation->relname)));
This should use ERRCODE_DUPLICATE_COLUMN.

> I've also attached a v15 of the main patch.  In check_columns_exist(),
> there was a 'return' that should be a 'continue'.  This caused us to
> skip the column existence checks for column lists defined after a table
> with no column list.

I can see that. Nicely spotted.
-- 
Michael


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