Re: [HACKERS] Patch - Tcl 8.6 version support for PostgreSQL

[Andres Freund]

Hi,

On 2017-04-22 23:28:49 +0530, Sandeep Thakkar wrote:
> diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
> index 304edf9..d3ef89f 100644
> --- a/src/tools/msvc/Mkvcbuild.pm
> +++ b/src/tools/msvc/Mkvcbuild.pm
> @@ -253,7 +253,12 @@ sub mkvcbuild
> $solution->AddProject('pltcl', 'dll', 'PLs', 'src\pl\tcl');
>   $pltcl->AddIncludeDir($solution->{options}->{tcl} . '\include');
>   $pltcl->AddReference($postgres);
> - if (-e $solution->{options}->{tcl} . '\lib\tcl85.lib')
> + if (-e $solution->{options}->{tcl} . '\lib\tcl86t.lib')
> + {
> + $pltcl->AddLibrary(
> + $solution->{options}->{tcl} . 
> '\lib\tcl86t.lib');
> + }
> + elsif (-e $solution->{options}->{tcl} . '\lib\tcl85.lib')
>   {
>   $pltcl->AddLibrary(
>   $solution->{options}->{tcl} . '\lib\tcl85.lib');

> diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
> index 30c1265..2667591 100644
> --- a/src/tools/msvc/Mkvcbuild.pm
> +++ b/src/tools/msvc/Mkvcbuild.pm
> @@ -208,7 +208,12 @@ sub mkvcbuild
> $solution->AddProject('pltcl', 'dll', 'PLs', 'src/pl/tcl');
>   $pltcl->AddIncludeDir($solution->{options}->{tcl} . '/include');
>   $pltcl->AddReference($postgres);
> - if (-e $solution->{options}->{tcl} . '/lib/tcl85.lib')
> + if (-e $solution->{options}->{tcl} . '/lib/tcl86t.lib')
> + {
> + $pltcl->AddLibrary(
> + $solution->{options}->{tcl} . 
> '/lib/tcl86t.lib');
> + }
> + elsif (-e $solution->{options}->{tcl} . '/lib/tcl85.lib')
>   {
>   $pltcl->AddLibrary(
>   $solution->{options}->{tcl} . '/lib/tcl85.lib');

Any chance of formulating these in a version agnostic way, instead of
copying the same stanza for every version?  E.g. using a wildcard or
such...

Greetings,

Andres Freund


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

Re: [HACKERS] Patch - Tcl 8.6 version support for PostgreSQL

[Sandeep Thakkar]

On Tue, Apr 25, 2017 at 1:59 AM, Dave Page  wrote:

>
>
> On Mon, Apr 24, 2017 at 9:26 PM, Andres Freund  wrote:
>
>> On 2017-04-24 16:18:30 -0400, Robert Haas wrote:
>> > On Sat, Apr 22, 2017 at 1:58 PM, Sandeep Thakkar <
>> > sandeep.thak...@enterprisedb.com> wrote:
>> >
>> > > Tcl8.6 is already supported in PostgreSQL.
>> > >
>> >
>> > What commit added support for it?
>>
>> I don't think the main build mechanism requires explicit support of new
>> versions. configure just checks for some prerequisites.
>>
>
> Right - and they were adjusted here: https://git.postgresql.o
> rg/gitweb/?p=postgresql.git;a=commit;h=eaba54c20c5ab2cb6aaff
> a57fd4990dfe2c7
>
> Right and it was back-patched till 9.2. But, it missed the changes
required for Windows build. So, can anyone please review those two patches
i.e Mkvcbuild_Tcl86_94-92.patch and Mkvcbuild_Tcl86_95-master.patch and
commit? With this patch, we could build the server with Tcl8.6 on Windows.
Thanks!



> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>



-- 
Sandeep Thakkar
EDB

Re: [HACKERS] Unportable implementation of background worker start

[Rémi Zara]

> Le 25 avr. 2017 à 01:47, Tom Lane  a écrit :
> 
> I wrote:
>> What I'm inclined to do is to revert the pselect change but not the other,
>> to see if that fixes these two animals.  If it does, we could look into
>> blacklisting these particular platforms when choosing pselect.
> 
> It looks like coypu is going to need manual intervention (ie, kill -9
> on the leftover postmaster) to get unwedged :-(.  That's particularly
> disturbing because it implies that ServerLoop isn't iterating at all;
> otherwise, it'd have noticed by now that the buildfarm script deleted
> its data directory out from under it.  Even if NetBSD's pselect had
> forgotten to unblock signals, you'd figure it'd time out after a
> minute ... so it's even more broken than that.
> 

Hi,

coypu was not stuck (no buildfarm related process running), but failed to 
clean-up shared memory and semaphores.
I’ve done the clean-up.

Regards,

Rémi



smime.p7s
Description: S/MIME cryptographic signature

Re: [HACKERS] PG 10 release notes

[Michael Paquier]

On Tue, Apr 25, 2017 at 10:54 AM, Bruce Momjian  wrote:
> On Tue, Apr 25, 2017 at 03:45:52AM +0200, Andreas Karlsson wrote:
>> On 04/25/2017 03:31 AM, Bruce Momjian wrote:
>> >I have committed the first draft of the Postgres 10 release notes.  They
>> >are current as of two days ago, and I will keep them current.  Please
>> >give me any feedback you have.
>>
>> This item is incorrectly attributed to me. I was only the reviewer, Peter is
>> the author.
>>
>> +
>> +
>> +
>> +Create a pg_sequence
>> system catalog to store sequence metadata (Andreas
>> +Karlsson)
>> +
>
> OK, fixed.  I also moved some "incompatibility" items into the right
> section.  FYI, you can see the most recent doc build here:
>
> http://momjian.us/pgsql_docs/release-10.html

Here is some feedback based on a first read.

+
+Issue fsync on the output files generated by pg_dump
and pg_dumpall(Michael Paquier)
+
Missing a space here between pg_dumpall and my name.


This is accessed via ts_headline() and to_tsvector. RIGHT SECTION?


This should use a  markup for both names.


Add SCRAM-SHA-256
support for password negotiation and storage (Michael
Paquier, Heikki Linnakangas)


This proves better security than the existing 'md5' negotiation and
storage method.

This is quite vague...




Remove orphaned temporary tables more aggressively (Robert Haas, Tom Lane)


Previously such tables were removed only when necessary. SECTION?


Well, I wrote the first patch that has been committed, until Tom
showed up and rewrote everything :)
perhaps this could be in a section dedicated to autovacuum?




Add GUC  to
add details to WAL that can be
sanity-checked on the standby (Kuntal Ghosh, Michael Paquier, Robert
Haas)

I would be fine if my name is removed here. Kuntal has done all the
work, I just reviewed his code.


Previously only specification of the stop name, time, and xid were
supported.


Missing timeline and 'immediate' here.




Speed up two-phase commit recovery performance (Simon Riggs)


Simon is the committer here, authors are marked as Stas Kelvich,
Nikhil Sontakke and Michael Paquier.




New major version numbering (Peter Eisentraut, Tom Lane)


Major versions will now increase just the first number, and minor
releases will increase just the second number.  A third number will no
longer be used in Postgres version numbers.


Perhaps this should be higher in the list?
-- 
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] PG 10 release notes

[Ashutosh Bapat]

Commit f9b1a0dd403ec0931213c66d5f979a3d3e8e7e30 mentions "Ashutosh
Bapat" as author, which is not reflected in the release notes

--
Allow explicit control over EXPLAIN's display of planning and
execution time (Stephen Frost)

By default planning and execution is display by EXPLAIN ANALYZE and
not display in other cases. The new EXPLAIN option SUMMARY allows
explicit control of this.
--


On Tue, Apr 25, 2017 at 7:24 AM, Bruce Momjian  wrote:
> On Tue, Apr 25, 2017 at 03:45:52AM +0200, Andreas Karlsson wrote:
>> On 04/25/2017 03:31 AM, Bruce Momjian wrote:
>> >I have committed the first draft of the Postgres 10 release notes.  They
>> >are current as of two days ago, and I will keep them current.  Please
>> >give me any feedback you have.
>>
>> This item is incorrectly attributed to me. I was only the reviewer, Peter is
>> the author.
>>
>> +
>> +
>> +
>> +Create a pg_sequence
>> system catalog to store sequence metadata (Andreas
>> +Karlsson)
>> +
>
> OK, fixed.  I also moved some "incompatibility" items into the right
> section.  FYI, you can see the most recent doc build here:
>
> http://momjian.us/pgsql_docs/release-10.html
>
> --
>   Bruce Momjian  http://momjian.us
>   EnterpriseDB http://enterprisedb.com
>
> + As you are, so once was I.  As I am, so you will be. +
> +  Ancient Roman grave inscription +
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers



-- 
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] OK, so culicidae is *still* broken

[Heikki Linnakangas]

On 04/24/2017 09:50 PM, Andres Freund wrote:

On 2017-04-24 14:43:11 -0400, Tom Lane wrote:

(We have accepted that kind of overhead for DSM segments, but the
intention I think is to allow only very trivial data structures in
the DSM segments.  Losing compiler pointer type checking for data
structures like the lock or PGPROC tables would be horrid.)


The relptr.h infrastructure brings some of the type-checking back, but
it's still pretty painful.  And just as important, it's quite noticeable
performance-wise.  So we have to do it for dynamic shm (until/unless we
go to using threads), but that doesn't mean we should do it for some of
the most performance critical data structures in PG...


Agreed.

For some data shared memory structures, that store no pointers, we 
wouldn't need to insist that they are mapped to the same address in 
every backend, though. In particular, shared_buffers. It wouldn't 
eliminate the problem, though, only make it less likely, so we'd still 
need to retry when it does happen.


- Heikki



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

Re: [HACKERS] Foreign Join pushdowns not working properly for outer joins

[Ashutosh Bapat]

On Tue, Apr 25, 2017 at 8:20 AM, Peter Eisentraut
 wrote:
> On 4/14/17 00:24, Ashutosh Bapat wrote:
>> This looks better. Here are patches for master and 9.6.
>> Since join pushdown was supported in 9.6 the patch should be
>> backported to 9.6 as well. Attached is the patch (_96) for 9.6,
>> created by rebasing on 9.6 branch and removing conflict. _v6 is
>> applicable on master.
>
> Committed to PG10.  I'll work on backpatching next.
>

Thanks.

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

[Ashutosh Bapat]

On Tue, Apr 25, 2017 at 1:46 AM, Robert Haas  wrote:
> On Mon, Apr 24, 2017 at 8:14 AM, Ashutosh Bapat
>  wrote:
>> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas  wrote:
>>> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed  wrote:
 Following can also be considered as it specifies more clearly that the
 partition holds default values.

 CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
>>>
>>> The partition doesn't contain default values; it is itself a default.
>>
>> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more 
>> natural.
>
> I suspect it could be done as of now, but I'm a little worried that it
> might create grammar conflicts in the future as we extend the syntax
> further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
> word DEFAULT appears in the same position where we'd normally have FOR
> VALUES, and so the parser will definitely be able to figure out what's
> going on.  When it gets to that position, it will see FOR or it will
> see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
> DEFAULT PARTITION OF ..., then we have action at a distance: whether
> or not the word DEFAULT is present before PARTITION affects which
> tokens are legal after the parent table name.

As long as we handle this at the transformation stage, it shouldn't be
a problem. The grammar would be something like
CREATE TABLE ... optDefault PARTITION OF ...

If user specifies DEFAULT PARTITION OF t1 FOR VALUES ..., parser will
allow that but in transformation stage, we will detect it and throw an
error "DEFAULT partitions can not contains partition bound clause" or
something like that. Also, documentation would say that DEFAULT and
partition bound specification are not allowed together.

-- 
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] scram and \password

[Michael Paquier]

On Sat, Apr 22, 2017 at 7:20 AM, Michael Paquier
 wrote:
> Do we really want to add a new function or have a hard failure? Any
> application calling PQencryptPassword may trap itself silently if the
> server uses scram as hba key or if the default is switched to that,
> from this point of view extending the function makes sense as well.

Attached is a new patch set, taking into account the new format.
-- 
Michael


0001-Move-routine-to-build-SCRAM-verifier-into-src-common.patch
Description: Binary data


0002-Refactor-frontend-side-random-number-generation.patch
Description: Binary data


0003-Extend-PQencryptPassword-with-a-hashing-method.patch
Description: Binary data


0004-Extend-psql-s-password-and-createuser-to-handle-SCRA.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] PG 10 release notes

[Petr Jelinek]

On 25/04/17 03:31, Bruce Momjian wrote:
> I have committed the first draft of the Postgres 10 release notes.  They
> are current as of two days ago, and I will keep them current.  Please
> give me any feedback you have.
> 

Cool!

> The only unusual thing is that this release has ~180 items while most
> recent release have had ~220.  The pattern I see that there are more
> large features in this release than previous ones.
> 

Well I think for example

> Optimizer
> 
> Add the ability to compute a correlation ratio and the number of distinct 
> values on several columns (Tomas Vondra, David Rowley)
> 

could be easily 2 or 3 items (explicitly defining additional statistics,
multicolumn ndistinct and functional dependencies).

I also wonder if ability to run SQL queries on walsender connected to a
database is worth mentioning (replication=database kind of connection).

Or the ability of logical decoding to follow timeline switches.

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


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

Re: [HACKERS] walsender & parallelism

[Petr Jelinek]

On 25/04/17 01:25, Andres Freund wrote:
> Hi,
> 
> On 2017-04-24 07:31:18 +0200, Petr Jelinek wrote:
>> The previous coding tried to run the unknown string throur lexer which
>> could fail for some valid SQL statements as the replication command
>> parser is too simple to handle all the complexities of SQL language.
>>
>> Instead just fall back to SQL parser on any unknown command.
>>
>> Also remove SHOW command handling from walsender as it's handled by the
>> simple query code path.
> 
> This break SHOW var; over the plain replication connections though
> (which was quite intentionally supported), so I don't think that's ok?
> 

Hmm I guess we don't use this anywhere in tests (which is not surprising
as it would have to be used in plain walsender connection). Fixed.

> 
> I don't like much how SHOW and walsender understanding SQL statements
> turned out, I think code like
> 
>   if (am_walsender)
>   {
>   if 
> (!exec_replication_command(query_string))
>   
> exec_simple_query(query_string);
>   }
>   else
>   exec_simple_query(query_string);
> 
> shows some of the issues here.
> 

Not sure how it's related to SHOW, but in any case, it's just result of
the parsing fallback.

> 
>> New alternative output for largeobject test has been added as the
>> replication connection does not support fastpath function calls.
> 
> I think just supporting fastpath over the replication protocol is less
> work than maintaining the alternative file.
> 
> 
>> --- a/src/Makefile.global.in
>> +++ b/src/Makefile.global.in
>> @@ -321,6 +321,7 @@ BZIP2= bzip2
>>  # Testing
>>  
>>  check: temp-install
>> +check-walsender-sql: temp-install
> 
> This doesn't strike me as something that should go into
> a/src/Makefile.global.in - I'd rather put it in
> b/src/test/regress/GNUmakefile.  check is in .global because it's used
> in a lot of different makefiles, but that's not true for this target, right?
> 

Shows how well I know our build system :)

Anyway, I gave it a try to support as much as possible and the result is
attached.

First patch adds the new make target that runs regression tests through
walsender. I had to change one test to run SHOW TIMEZONE instead of SHOW
TIME ZONE, otherwise no tests need to be changed.

The second patch unifies the sighup handling across all kinds of
processes. This is mostly replacing got_SIGHuP with new variable and
removing local SIGHUP handlers in favor of generic one where possible.

And the third patch changes walsender. There are 3 things it does a) it
overhauls signal handling there, b) it allows both fast path function
calls and extended protocol messages (once I did function fast path I
didn't really see any difference in terms of extended protocol, please
correct me if I am wrong in that) and c) improves the fallback in parser.

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


0001-Add-check-walsender-sql-make-target.patch
Description: binary/octet-stream


0002-Unify-SIGHUP-hadnling-across-all-types-of-processes.patch
Description: binary/octet-stream


0003-Make-walsender-behave-more-like-normal-backend.patch
Description: binary/octet-stream

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

Re: [HACKERS] PG 10 release notes

[Amit Langote]

On 2017/04/25 10:31, Bruce Momjian wrote:
> I have committed the first draft of the Postgres 10 release notes.  They
> are current as of two days ago, and I will keep them current.  Please
> give me any feedback you have.
> 
> The only unusual thing is that this release has ~180 items while most
> recent release have had ~220.  The pattern I see that there are more
> large features in this release than previous ones.

Thanks for this.

Wondering if the following really belongs under postgres_fdw improvements:

+
+
+
+Improve optimization of FULL JOIN queries containing
subqueries in the
+FROM clause (Etsuro Fujita)
+
+

Maybe, nearby the following:

+   
+Additional Modules
...
+
+
+
+Push aggregates to foreign data wrapper servers, where possible (Jeevan
+Chalke, Ashutosh Bapat)
+

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] PG 10 release notes

[Rafia Sabih]

On Tue, Apr 25, 2017 at 9:15 AM, Tom Lane  wrote:
> Andres Freund  writes:
>> On 2017-04-24 23:37:42 -0400, Bruce Momjian wrote:
>>> I remember seeing those and those are normally details I do not put in
>>> the release notes as there isn't a clear user experience change except
>>> "Postgres is faster".  Yeah, a bummer, and I can change my filter, but
>>> it would require discussion.
>
>> I think "postgres is faster" is one of the bigger user demands, so I
>> don't think that policy makes much sense.  A large number of the changes
>> over the next few releases will focus solely on that.  Nor do I think
>> past release notes particularly filtered such changes out.
>
> I think it has been pretty common to accumulate a lot of such changes
> into generic entries like, say, "speedups for hash joins".  More detail
> than that simply isn't useful to end users; and as a rule, our release
> notes are too long anyway.
>
> regards, tom lane
>
>
Just wondering if the mention of commit
0414b26bac09379a4cbf1fbd847d1cee2293c5e4 is missed? Not sure if this
requires a separate entry or could be merged with -- Support parallel
btree index scans.

-- 
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] PG 10 release notes

[David Rowley]

 ..On 25 April 2017 at 13:31, Bruce Momjian  wrote:
> The only unusual thing is that this release has ~180 items while most
> recent release have had ~220.  The pattern I see that there are more
> large features in this release than previous ones.

Thanks for drafting this up.

I understand that it may have been filtered out, but I'd say that
7e534adcdc70 is worth a mention.

Users creating BRIN indexes previously would have had to know
beforehand that the table would be sufficiently correlated on the
indexed columns for the index to be worthwhile, whereas now there's a
lesser need for the user to know this beforehand.

Also:


New commands are CREATE,
ALTER, and
DROP STATISTICS.
This is helpful in
estimating query memory usage and ... HOW IS RATIO USED?


HOW IS RATIO USED?  There are two types of new stats;

ndistinct, which can improve row estimations in the query planner for
estimating the number of distinct value groups over multiple columns.
functionaldeps, which provides the planner with a better understanding
of functional depdenences between columns on which the statistics are
defined. The planner takes the functional dependency degree into
account when multiplying selectivity estimations for multiple columns.

Unsure how best to trim that down to something short enough for the
release notes.

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


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

[HACKERS] Link to commits in PG 10 release notes

[Andres Freund]

Hi,

I wonder if there's a reasonable way that allows to add links to the
more crucial commits for changelog entries. The source e.g. has




Support parallel bitmap heap scans (Dilip Kumar)



This allows a single index scan to dispatch parallel workers to process
different areas of the heap.



for an item, and it'd be pretty cool if we could have a link to those
two commits from the entry.  It'd need be pretty unobstrusive to avoid
making things hard to read, but it'd obviate some of the need to list
details, and it gives curious people changes to see what actually
changed.

- Andres


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

Re: [HACKERS] PG 10 release notes

[Andres Freund]

On 2017-04-24 23:45:06 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-04-24 23:37:42 -0400, Bruce Momjian wrote:
> >> I remember seeing those and those are normally details I do not put in
> >> the release notes as there isn't a clear user experience change except
> >> "Postgres is faster".  Yeah, a bummer, and I can change my filter, but
> >> it would require discussion.
> 
> > I think "postgres is faster" is one of the bigger user demands, so I
> > don't think that policy makes much sense.  A large number of the changes
> > over the next few releases will focus solely on that.  Nor do I think
> > past release notes particularly filtered such changes out.
> 
> I think it has been pretty common to accumulate a lot of such changes
> into generic entries like, say, "speedups for hash joins".  More detail
> than that simply isn't useful to end users; and as a rule, our release
> notes are too long anyway.

Oh, I completely agree with accumulating related changes, and that
code-level details aren't useful.  I think we skipped them entirely
here.  And I just listed my own changes because I could find them
quickly, but they're not alone, e.g:
090010f2ec9b1f9ac1124dc628b89586f911b641 - Improve performance of 
find_tabstat_entry()/get_tabstat_entry()
which makes it realistic to have sessions touching many relations, which
previously was O(#relations^2), and which caused repeated complaints
over the years, and allows for different usecases.

- Andres


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

Re: [HACKERS] PG 10 release notes

[Tom Lane]

Andres Freund  writes:
> On 2017-04-24 23:37:42 -0400, Bruce Momjian wrote:
>> I remember seeing those and those are normally details I do not put in
>> the release notes as there isn't a clear user experience change except
>> "Postgres is faster".  Yeah, a bummer, and I can change my filter, but
>> it would require discussion.

> I think "postgres is faster" is one of the bigger user demands, so I
> don't think that policy makes much sense.  A large number of the changes
> over the next few releases will focus solely on that.  Nor do I think
> past release notes particularly filtered such changes out.

I think it has been pretty common to accumulate a lot of such changes
into generic entries like, say, "speedups for hash joins".  More detail
than that simply isn't useful to end users; and as a rule, our release
notes are too long anyway.

regards, tom lane


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

Re: [HACKERS] PG 10 release notes

[Andres Freund]

On 2017-04-24 23:37:42 -0400, Bruce Momjian wrote:
> On Mon, Apr 24, 2017 at 08:36:00PM -0700, Andres Freund wrote:
> > On 2017-04-24 21:31:44 -0400, Bruce Momjian wrote:
> > > I have committed the first draft of the Postgres 10 release notes.  They
> > > are current as of two days ago, and I will keep them current.  Please
> > > give me any feedback you have.
> > > 
> > > The only unusual thing is that this release has ~180 items while most
> > > recent release have had ~220.  The pattern I see that there are more
> > > large features in this release than previous ones.
> > 
> > I think that might also be because you skipped a few things that should
> > get their own entries.  I've not yet made a pass through your draft (and
> > won't for some days), but a quick search shows the draft to e.g miss:
> > b30d3ea824c5ccba43d3e942704f20686e7dbab8 - Add a macro templatized 
> > hashtable.
> > 75ae538bc3168bf44475240d4e0487ee2f3bb376 - Use more efficient hashtable for 
> > tidbitmap.c to speed up bitmap scans.
> > 5dfc198146b49ce7ecc8a1fc9d5e171fb75f6ba5 - Use more efficient hashtable for 
> > execGrouping.c to speed up hash aggregation.
> > fc4b3dea2950e4f6081f1ed2380f82c9efd672e0 - User narrower representative 
> > tuples in the hash-agg hashtable.
> > 8ed3f11bb045ad7a3607690be668dbd5b3cc31d7 - Perform one only projection to 
> > compute agg arguments.
> > (not that this needs to five entries)
> 
> I remember seeing those and those are normally details I do not put in
> the release notes as there isn't a clear user experience change except
> "Postgres is faster".  Yeah, a bummer, and I can change my filter, but
> it would require discussion.

I think "postgres is faster" is one of the bigger user demands, so I
don't think that policy makes much sense.  A large number of the changes
over the next few releases will focus solely on that.  Nor do I think
past release notes particularly filtered such changes out.

- Andres


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

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Mon, Apr 24, 2017 at 08:36:00PM -0700, Andres Freund wrote:
> On 2017-04-24 21:31:44 -0400, Bruce Momjian wrote:
> > I have committed the first draft of the Postgres 10 release notes.  They
> > are current as of two days ago, and I will keep them current.  Please
> > give me any feedback you have.
> > 
> > The only unusual thing is that this release has ~180 items while most
> > recent release have had ~220.  The pattern I see that there are more
> > large features in this release than previous ones.
> 
> I think that might also be because you skipped a few things that should
> get their own entries.  I've not yet made a pass through your draft (and
> won't for some days), but a quick search shows the draft to e.g miss:
> b30d3ea824c5ccba43d3e942704f20686e7dbab8 - Add a macro templatized hashtable.
> 75ae538bc3168bf44475240d4e0487ee2f3bb376 - Use more efficient hashtable for 
> tidbitmap.c to speed up bitmap scans.
> 5dfc198146b49ce7ecc8a1fc9d5e171fb75f6ba5 - Use more efficient hashtable for 
> execGrouping.c to speed up hash aggregation.
> fc4b3dea2950e4f6081f1ed2380f82c9efd672e0 - User narrower representative 
> tuples in the hash-agg hashtable.
> 8ed3f11bb045ad7a3607690be668dbd5b3cc31d7 - Perform one only projection to 
> compute agg arguments.
> (not that this needs to five entries)

I remember seeing those and those are normally details I do not put in
the release notes as there isn't a clear user experience change except
"Postgres is faster".  Yeah, a bummer, and I can change my filter, but
it would require discussion.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] PG 10 release notes

[Andres Freund]

On 2017-04-24 21:31:44 -0400, Bruce Momjian wrote:
> I have committed the first draft of the Postgres 10 release notes.  They
> are current as of two days ago, and I will keep them current.  Please
> give me any feedback you have.
> 
> The only unusual thing is that this release has ~180 items while most
> recent release have had ~220.  The pattern I see that there are more
> large features in this release than previous ones.

I think that might also be because you skipped a few things that should
get their own entries.  I've not yet made a pass through your draft (and
won't for some days), but a quick search shows the draft to e.g miss:
b30d3ea824c5ccba43d3e942704f20686e7dbab8 - Add a macro templatized hashtable.
75ae538bc3168bf44475240d4e0487ee2f3bb376 - Use more efficient hashtable for 
tidbitmap.c to speed up bitmap scans.
5dfc198146b49ce7ecc8a1fc9d5e171fb75f6ba5 - Use more efficient hashtable for 
execGrouping.c to speed up hash aggregation.
fc4b3dea2950e4f6081f1ed2380f82c9efd672e0 - User narrower representative tuples 
in the hash-agg hashtable.
8ed3f11bb045ad7a3607690be668dbd5b3cc31d7 - Perform one only projection to 
compute agg arguments.
(not that this needs to five entries)

- Andres


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

Re: [HACKERS] PG 10 release notes

[Amit Kapila]

On Tue, Apr 25, 2017 at 8:35 AM, Bruce Momjian  wrote:
> On Tue, Apr 25, 2017 at 08:30:50AM +0530, Amit Kapila wrote:
>> On Tue, Apr 25, 2017 at 7:01 AM, Bruce Momjian  wrote:
>> > I have committed the first draft of the Postgres 10 release notes.  They
>> > are current as of two days ago, and I will keep them current.  Please
>> > give me any feedback you have.
>> >
>>
>> Some of the items which I feel could be added:
>>
>> 5e6d8d2bbbcace304450b309e79366c0da4063e4
>> Allow parallel workers to execute subplans.
>
> Uh, can you show me the commit on that and give some text ideas?
>

I have already mentioned the commit id (5e6d8d2b).  Text can be "Allow
queries containing subplans to execute in parallel".  We should also
mention in some way that this applies only when the query contains
uncorrelated subplan.

>> 61c2e1a95f94bb904953a6281ce17a18ac38ee6d
>> Improve access to parallel query from procedural languages.
>
> I think I have that:
>
> Increase parallel query usage in procedural language functions (Robert
> Haas)
>
>> In Parallel Queries section, we can add above two items as they
>> increase the usage of the parallel query in many cases.
>>
>> ea69a0dead5128c421140dc53fac165ba4af8520
>> Expand hash indexes more gradually.
>
> That is in this item:
>
> Improve hash bucket split performance by reducing locking requirements
> (Amit Kapila, Mithun Cy)
>
> Also cache hash index meta-information for faster lookups. Additional
> hash performance improvements have also been made. pg_upgrade'd hash
> indexes from previous major Postgres versions must be rebuilt.
>
> Can you suggest additional wording?
>

Allow hash indexes to expand slowly

This will help in controlling the size of hash indexes after the split.

>  I did merge many of the hash items
> into this so it would be understandable.  You can see the commits in the
> SGML source.
>
>> I think the above commit needs a separate mention, as this is a really
>> huge step forward to control the size of hash indexes.
>
> Yes, it is unfotunate that the item is in the incompatibility item.  I
> wonder if I should split out the need to rebuild the hash indexes and
> keep it there and move this item into the "Index" section.
>

That sounds sensible.




-- 
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] PG 10 release notes

[Bruce Momjian]

On Mon, Apr 24, 2017 at 11:05:41PM -0400, Bruce Momjian wrote:
> > I think the above commit needs a separate mention, as this is a really
> > huge step forward to control the size of hash indexes.
> 
> Yes, it is unfotunate that the item is in the incompatibility item.  I
> wonder if I should split out the need to rebuild the hash indexes and
> keep it there and move this item into the "Index" section.

Done, items split.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Tue, Apr 25, 2017 at 08:36:38AM +0530, Amit Kapila wrote:
> On Tue, Apr 25, 2017 at 8:30 AM, Amit Kapila  wrote:
> > On Tue, Apr 25, 2017 at 7:01 AM, Bruce Momjian  wrote:
> >> I have committed the first draft of the Postgres 10 release notes.  They
> >> are current as of two days ago, and I will keep them current.  Please
> >> give me any feedback you have.
> >>
> >
> > Some of the items which I feel could be added:
> >
> > 5e6d8d2bbbcace304450b309e79366c0da4063e4
> > Allow parallel workers to execute subplans.
> >
> > 61c2e1a95f94bb904953a6281ce17a18ac38ee6d
> > Improve access to parallel query from procedural languages.
> >
> > In Parallel Queries section, we can add above two items as they
> > increase the usage of the parallel query in many cases.
> >
> 
> I see the second one in release notes, but maybe we should credit
> Rafia Sabih as well for that feature as she was the original author of
> the patch and has helped in fixing the bugs that occurred due to that
> feature.

Added.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] PG 10 release notes

[Amit Kapila]

On Tue, Apr 25, 2017 at 8:30 AM, Amit Kapila  wrote:
> On Tue, Apr 25, 2017 at 7:01 AM, Bruce Momjian  wrote:
>> I have committed the first draft of the Postgres 10 release notes.  They
>> are current as of two days ago, and I will keep them current.  Please
>> give me any feedback you have.
>>
>
> Some of the items which I feel could be added:
>
> 5e6d8d2bbbcace304450b309e79366c0da4063e4
> Allow parallel workers to execute subplans.
>
> 61c2e1a95f94bb904953a6281ce17a18ac38ee6d
> Improve access to parallel query from procedural languages.
>
> In Parallel Queries section, we can add above two items as they
> increase the usage of the parallel query in many cases.
>

I see the second one in release notes, but maybe we should credit
Rafia Sabih as well for that feature as she was the original author of
the patch and has helped in fixing the bugs that occurred due to that
feature.


-- 
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] PG 10 release notes

[Bruce Momjian]

On Tue, Apr 25, 2017 at 08:30:50AM +0530, Amit Kapila wrote:
> On Tue, Apr 25, 2017 at 7:01 AM, Bruce Momjian  wrote:
> > I have committed the first draft of the Postgres 10 release notes.  They
> > are current as of two days ago, and I will keep them current.  Please
> > give me any feedback you have.
> >
> 
> Some of the items which I feel could be added:
> 
> 5e6d8d2bbbcace304450b309e79366c0da4063e4
> Allow parallel workers to execute subplans.

Uh, can you show me the commit on that and give some text ideas?

> 61c2e1a95f94bb904953a6281ce17a18ac38ee6d
> Improve access to parallel query from procedural languages.

I think I have that:

Increase parallel query usage in procedural language functions (Robert
Haas)

> In Parallel Queries section, we can add above two items as they
> increase the usage of the parallel query in many cases.
> 
> ea69a0dead5128c421140dc53fac165ba4af8520
> Expand hash indexes more gradually.

That is in this item:

Improve hash bucket split performance by reducing locking requirements
(Amit Kapila, Mithun Cy)

Also cache hash index meta-information for faster lookups. Additional
hash performance improvements have also been made. pg_upgrade'd hash
indexes from previous major Postgres versions must be rebuilt.

Can you suggest additional wording?  I did merge many of the hash items
into this so it would be understandable.  You can see the commits in the
SGML source.

> I think the above commit needs a separate mention, as this is a really
> huge step forward to control the size of hash indexes.

Yes, it is unfotunate that the item is in the incompatibility item.  I
wonder if I should split out the need to rebuild the hash indexes and
keep it there and move this item into the "Index" section.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] PG 10 release notes

[Amit Kapila]

On Tue, Apr 25, 2017 at 7:01 AM, Bruce Momjian  wrote:
> I have committed the first draft of the Postgres 10 release notes.  They
> are current as of two days ago, and I will keep them current.  Please
> give me any feedback you have.
>

Some of the items which I feel could be added:

5e6d8d2bbbcace304450b309e79366c0da4063e4
Allow parallel workers to execute subplans.

61c2e1a95f94bb904953a6281ce17a18ac38ee6d
Improve access to parallel query from procedural languages.

In Parallel Queries section, we can add above two items as they
increase the usage of the parallel query in many cases.

ea69a0dead5128c421140dc53fac165ba4af8520
Expand hash indexes more gradually.

I think the above commit needs a separate mention, as this is a really
huge step forward to control the size of hash indexes.

-- 
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] Foreign Join pushdowns not working properly for outer joins

[Peter Eisentraut]

On 4/14/17 00:24, Ashutosh Bapat wrote:
> This looks better. Here are patches for master and 9.6.
> Since join pushdown was supported in 9.6 the patch should be
> backported to 9.6 as well. Attached is the patch (_96) for 9.6,
> created by rebasing on 9.6 branch and removing conflict. _v6 is
> applicable on master.

Committed to PG10.  I'll work on backpatching next.

-- 
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] PG 10 release notes

['Bruce Momjian']

All fixed, thanks.

---

On Tue, Apr 25, 2017 at 02:40:23AM +, Tsunakawa, Takayuki wrote:
> Hello, Bruce
> 
> > From: pgsql-hackers-ow...@postgresql.org
> > [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Bruce Momjian
> > I have committed the first draft of the Postgres 10 release notes.  They
> > are current as of two days ago, and I will keep them current.  Please give
> > me any feedback you have.
> 
> 
> Thanks for a nice release note.  Below is what I noticed:
> 
> 
> (1)
> Support parallel bitmap heap scans (Dilip Kum)
> 
> This item appears twice.
> 
> 
> (2)
> E.1.3.1.4. Optimizer
> Remove SCO and Unixware ports (Tom Lane)
> 
> The section doesn't seem appropriate.  Is OS-specific code related to the 
> optimizer?
> 
> 
> 
> (3)
> Remove documented restriction about using large shared buffers on Windows 
> (Tsunakawa, Takayuki)
> 
> Please change the name to "Takayuki Tsunakawa".
> 
> 
> (4)
> Have to_timestamp() and to_date() check check input values for validity 
> (Artur Zakirov)
> 
> "check" is repeated.
> 
> 
> (5)
> Use POSIX semaphores rather than SysV semaphores on Linux and FreeBSD (Tom 
> Lane)
> This avoids some limits on SysV semaphores usage.
> 
> These two lines are put in two different items.
> 
> 
> Regards
> Takayuki Tsunakawa
> 

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] PG 10 release notes

[Tsunakawa, Takayuki]

Hello, Bruce

> From: pgsql-hackers-ow...@postgresql.org
> [mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Bruce Momjian
> I have committed the first draft of the Postgres 10 release notes.  They
> are current as of two days ago, and I will keep them current.  Please give
> me any feedback you have.


Thanks for a nice release note.  Below is what I noticed:


(1)
Support parallel bitmap heap scans (Dilip Kum)

This item appears twice.


(2)
E.1.3.1.4. Optimizer
Remove SCO and Unixware ports (Tom Lane)

The section doesn't seem appropriate.  Is OS-specific code related to the 
optimizer?



(3)
Remove documented restriction about using large shared buffers on Windows 
(Tsunakawa, Takayuki)

Please change the name to "Takayuki Tsunakawa".


(4)
Have to_timestamp() and to_date() check check input values for validity (Artur 
Zakirov)

"check" is repeated.


(5)
Use POSIX semaphores rather than SysV semaphores on Linux and FreeBSD (Tom Lane)
This avoids some limits on SysV semaphores usage.

These two lines are put in two different items.


Regards
Takayuki Tsunakawa



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

Re: [HACKERS] PG 10 release notes

[Bruce Momjian]

On Tue, Apr 25, 2017 at 03:45:52AM +0200, Andreas Karlsson wrote:
> On 04/25/2017 03:31 AM, Bruce Momjian wrote:
> >I have committed the first draft of the Postgres 10 release notes.  They
> >are current as of two days ago, and I will keep them current.  Please
> >give me any feedback you have.
> 
> This item is incorrectly attributed to me. I was only the reviewer, Peter is
> the author.
> 
> +
> +
> +
> +Create a pg_sequence
> system catalog to store sequence metadata (Andreas
> +Karlsson)
> +

OK, fixed.  I also moved some "incompatibility" items into the right
section.  FYI, you can see the most recent doc build here:

http://momjian.us/pgsql_docs/release-10.html

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


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

Re: [HACKERS] PG 10 release notes

[Andreas Karlsson]

On 04/25/2017 03:31 AM, Bruce Momjian wrote:

I have committed the first draft of the Postgres 10 release notes.  They
are current as of two days ago, and I will keep them current.  Please
give me any feedback you have.


This item is incorrectly attributed to me. I was only the reviewer, 
Peter is the author.


+
+
+
+Create a linkend="catalog-pg-sequence">pg_sequence system 
catalog to store sequence metadata (Andreas

+Karlsson)
+

Andreas


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

[HACKERS] PG 10 release notes

[Bruce Momjian]

I have committed the first draft of the Postgres 10 release notes.  They
are current as of two days ago, and I will keep them current.  Please
give me any feedback you have.

The only unusual thing is that this release has ~180 items while most
recent release have had ~220.  The pattern I see that there are more
large features in this release than previous ones.

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] pgrowlocks relkind check

[Amit Langote]

Hi Stephen,

On 2017/04/11 22:17, Stephen Frost wrote:
>> create extension pgrowlocks;
>> create view one as select 1;
>> select pgrowlocks('one');
>> -- ERROR:  could not open file "base/68730/68748": No such file or directory
>>
>> With the attached patch:
>>
>> select pgrowlocks('one');
>> ERROR:  "one" is not a table, index, materialized view, sequence, or TOAST
>> table
> 
> Good point.
> 
> Thanks, I'll see about committing this shortly.

Should I add this to the next commitfest (not an open item per se) or will
you be committing it?

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] some review comments on logical rep code

[Masahiko Sawada]

On Mon, Apr 24, 2017 at 7:57 PM, Kyotaro HORIGUCHI
 wrote:
> Hello,
>
> At Mon, 24 Apr 2017 11:18:32 +0900, Masahiko Sawada  
> wrote in 
>> >> BEGIN;
>> >> ALTER SUBSCRIPTION hoge_sub ENABLE;
>> >> PREPARE TRANSACTION 'g';
>> >> BEGIN;
>> >> SELECT 1;
>> >> COMMIT;  -- wake up the launcher at this point.
>> >> COMMIT PREPARED 'g';
>> >>
>> >> Even if we wake up the launcher even when a ROLLBACK PREPARED, it's
>> >> not a big deal to the launcher process actually. Compared with the
>> >> complexly of changing the logic so that on_commit_launcher_wakeup
>> >> corresponds to prepared transaction, we might want to accept this
>> >> behavior.
>> >
>> > on_commit_launcher_wakeup needs to be recoreded in 2PC state
>> > file to handle this issue properly. But I agree with you, that would
>> > be overkill for small gain. So I'm thinking to add the following
>> > comment into your patch and push it. Thought?
>> >
>> > -
>> > Note that on_commit_launcher_wakeup flag doesn't work as expected in 2PC 
>> > case.
>> > For example, COMMIT PREPARED on the transaction enabling the flag
>> > doesn't wake up
>> > the logical replication launcher if ROLLBACK on another transaction
>> > runs before it.
>> > To handle this case properly, the flag needs to be recorded in 2PC
>> > state file so that
>> > COMMIT PREPARED and ROLLBACK PREPARED can determine whether to wake up
>> > the launcher. However this is overkill for small gain and false wakeup
>> > of the launcher
>> > is not so harmful (probably we can live with that), so we do nothing
>> > here for this issue.
>> > -
>> >
>>
>> Agreed.
>>
>> I added this comment to the patch and attached it.
>
> The following "However" may need a follwoing comma.
>
>> However this is overkill for small gain and false wakeup of the
>> launcher is not so harmful (probably we can live with that), so
>> we do nothing here for this issue.
>
> I agree this as a whole. But I think that the main issue here is
> not false wakeups, but 'possible delay of launching new workers
> by 3 minutes at most'

In what case does launching of the apply worker delay 3 minutes at most?

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
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] PG_GETARG_GISTENTRY?

[Noah Misch]

On Mon, Apr 24, 2017 at 09:25:25AM -0700, Mark Dilger wrote:
> Here is a small patch for the next open commitfest which handles a case
> that Noah's commits 9d7726c2ba06b932f791f2d0cc5acf73cc0b4dca and
> 3a0d473192b2045cbaf997df8437e7762d34f3ba apparently missed.

The scope for those commits was wrappers of PG_DETOAST_DATUM_PACKED(), which
does not include PG_DETOAST_DATUM_SLICE().

> Noah, if you left this case out intentionally, sorry for the noise.  I did not
> immediately see any reason not to follow your lead for this function.

This is not following my lead, but that doesn't make it bad.  It's just a
different topic.


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

[HACKERS] warning in twophase.c

[Amit Langote]

Been seeing this warning recently:

twophase.c: In function ‘RecoverPreparedTransactions’:
twophase.c:1916:9: warning: variable ‘overwriteOK’ set but not used
[-Wunused-but-set-variable]
   bool  overwriteOK = false;
 ^~~

As the message says, the value of overwriteOK is not used anywhere in
RecoverPreparedTransactions:

 booloverwriteOK = false;

/*
 * It's possible that SubTransSetParent has been set before, if
 * the prepared transaction generated xid assignment records. Test
 * here must match one used in AssignTransactionId().
 */
if (InHotStandby && (hdr->nsubxacts >= PGPROC_MAX_CACHED_SUBXIDS ||
 XLogLogicalInfoActive()))
overwriteOK = true;

Couldn't we get rid of it?

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

[Amit Langote]

On 2017/04/25 5:16, Robert Haas wrote:
> On Mon, Apr 24, 2017 at 8:14 AM, Ashutosh Bapat
>  wrote:
>> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas  wrote:
>>> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed  wrote:
 Following can also be considered as it specifies more clearly that the
 partition holds default values.

 CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
>>>
>>> The partition doesn't contain default values; it is itself a default.
>>
>> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more 
>> natural.
> 
> I suspect it could be done as of now, but I'm a little worried that it
> might create grammar conflicts in the future as we extend the syntax
> further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
> word DEFAULT appears in the same position where we'd normally have FOR
> VALUES, and so the parser will definitely be able to figure out what's
> going on.  When it gets to that position, it will see FOR or it will
> see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
> DEFAULT PARTITION OF ..., then we have action at a distance: whether
> or not the word DEFAULT is present before PARTITION affects which
> tokens are legal after the parent table name.  bison isn't always very
> smart about that kind of thing.  No particular dangers come to mind at
> the moment, but it makes me nervous anyway.

+1 to CREATE TABLE .. PARTITION OF .. DEFAULT

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] Quorum commit for multiple synchronous replication.

[Masahiko Sawada]

On Tue, Apr 25, 2017 at 12:56 AM, Fujii Masao  wrote:
> On Mon, Apr 24, 2017 at 9:02 AM, Noah Misch  wrote:
>> On Thu, Apr 20, 2017 at 11:34:34PM -0700, Noah Misch wrote:
>>> On Fri, Apr 21, 2017 at 01:20:05PM +0900, Masahiko Sawada wrote:
>>> > On Fri, Apr 21, 2017 at 12:02 PM, Noah Misch  wrote:
>>> > > On Wed, Apr 19, 2017 at 01:52:53PM +0900, Masahiko Sawada wrote:
>>> > >> On Wed, Apr 19, 2017 at 12:34 PM, Noah Misch  wrote:
>>> > >> > On Sun, Apr 16, 2017 at 07:25:28PM +0900, Fujii Masao wrote:
>>> > >> >> As I told firstly this is not a bug. There are some proposals for 
>>> > >> >> better design
>>> > >> >> of priority column in pg_stat_replication, but we've not reached 
>>> > >> >> the consensus
>>> > >> >> yet. So I think that it's better to move this open item to "Design 
>>> > >> >> Decisions to
>>> > >> >> Recheck Mid-Beta" section so that we can hear more opinions.
>>> > >> >
>>> > >> > I'm reading that some people want to report NULL priority, some 
>>> > >> > people want to
>>> > >> > report a constant 1 priority, and nobody wants the current behavior. 
>>> > >> >  Is that
>>> > >> > an accurate summary?
>>> > >>
>>> > >> Yes, I think that's correct.
>>> > >
>>> > > Okay, but ...
>>> > >
>>> > >> FWIW the reason of current behavior is that it would be useful for the
>>> > >> user who is willing to switch from ANY to FIRST. They can know which
>>> > >> standbys will become sync or potential.
>>> > >
>>> > > ... does this mean you personally want to keep the current behavior?  
>>> > > If not,
>>> > > has some other person stated a wish to keep the current behavior?
>>> >
>>> > No, I want to change the current behavior. IMO it's better to set
>>> > priority 1 to all standbys in quorum set. I guess there is no longer
>>> > person who supports the current behavior.
>>>
>>> In that case, this open item is not eligible for section "Design Decisions 
>>> to
>>> Recheck Mid-Beta".  That section is for items where we'll probably change
>>> nothing, but we plan to recheck later just in case.  Here, we expect to 
>>> change
>>> the behavior; the open question is which replacement behavior to prefer.
>>>
>>> Fujii, as the owner of this open item, you are responsible for moderating 
>>> the
>>> debate until there's adequate consensus to make a particular change or to 
>>> keep
>>> the current behavior after all.  Please proceed to do that.  Beta testers
>>> deserve a UI they may like, not a UI you already plan to change later.
>>
>> Please observe the policy on open item ownership[1] and send a status update
>> within three calendar days of this message.  Include a date for your
>> subsequent status update.
>
> Okay, so our consensus is to always set the priorities of sync standbys
> to 1 in quorum-based syncrep case. Attached patch does this change.
> Barrying any objection, I will commit this.

+1

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
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] OK, so culicidae is *still* broken

[Craig Ringer]

On 25 Apr. 2017 02:51, "Andres Freund"  wrote:

On 2017-04-24 11:08:48 -0700, Andres Freund wrote:
> On 2017-04-24 23:14:40 +0800, Craig Ringer wrote:
> > In the long run we'll probably be forced toward threading or far
pointers.
>
> I'll vote for removing the windows port, before going for that.  And I'm
> not even joking.

Just to clarify: I'm talking about far pointers here, not threading.


Yeah, I'm pretty unimpressed that the accepted solution seems to be to
return to the early '90s.

Why don't platforms offer any sensible way to reserve a virtual address
range at process creation time?

It looks like you need a minimal loader process that rebases its self in
memory if it finds its self loaded in the desired area, then maps the
required memory range and loads the real process. Hard to imagine that not
causing more problems than it solves.

Re: [HACKERS] Unportable implementation of background worker start

[Tom Lane]

I wrote:
> What I'm inclined to do is to revert the pselect change but not the other,
> to see if that fixes these two animals.  If it does, we could look into
> blacklisting these particular platforms when choosing pselect.

It looks like coypu is going to need manual intervention (ie, kill -9
on the leftover postmaster) to get unwedged :-(.  That's particularly
disturbing because it implies that ServerLoop isn't iterating at all;
otherwise, it'd have noticed by now that the buildfarm script deleted
its data directory out from under it.  Even if NetBSD's pselect had
forgotten to unblock signals, you'd figure it'd time out after a
minute ... so it's even more broken 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] walsender & parallelism

[Andres Freund]

Hi,

On 2017-04-24 07:31:18 +0200, Petr Jelinek wrote:
> The previous coding tried to run the unknown string throur lexer which
> could fail for some valid SQL statements as the replication command
> parser is too simple to handle all the complexities of SQL language.
> 
> Instead just fall back to SQL parser on any unknown command.
> 
> Also remove SHOW command handling from walsender as it's handled by the
> simple query code path.

This break SHOW var; over the plain replication connections though
(which was quite intentionally supported), so I don't think that's ok?


I don't like much how SHOW and walsender understanding SQL statements
turned out, I think code like

if (am_walsender)
{
if 
(!exec_replication_command(query_string))

exec_simple_query(query_string);
}
else
exec_simple_query(query_string);

shows some of the issues here.


> Checks the SQL over walsender interface by running the standard set of
> regression tests against it.

I like that approach a lot.


> New alternative output for largeobject test has been added as the
> replication connection does not support fastpath function calls.

I think just supporting fastpath over the replication protocol is less
work than maintaining the alternative file.


> --- a/src/Makefile.global.in
> +++ b/src/Makefile.global.in
> @@ -321,6 +321,7 @@ BZIP2 = bzip2
>  # Testing
>  
>  check: temp-install
> +check-walsender-sql: temp-install

This doesn't strike me as something that should go into
a/src/Makefile.global.in - I'd rather put it in
b/src/test/regress/GNUmakefile.  check is in .global because it's used
in a lot of different makefiles, but that's not true for this target, right?



Greetings,

Andres Freund


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

Re: [HACKERS] snapbuild woes

[Petr Jelinek]

On 25/04/17 00:59, Andres Freund wrote:
> Hi,
> 
> On 2017-04-15 05:18:49 +0200, Petr Jelinek wrote:
>> Hi, here is updated patch (details inline).
> 
> I'm not yet all that happy, sorry:
> 
> Looking at 0001:
> - GetOldestSafeDecodingTransactionId() only guarantees to return an xid
>   safe for decoding (note how procArray->replication_slot_catalog_xmin
>   is checked), not one for the initial snapshot - so afaics this whole
>   exercise doesn't guarantee much so far.
> - A later commit introduces need_full_snapshot as a
>   CreateInitDecodingContext, but you don't use it, not here.  That seems
>   wrong.

Ah yeah looks like that optimization is useful even here.

> - I remain unhappy with the handling of the reset of effective_xmin in
>   FreeDecodingContext(). What if we ERROR/FATAL out before that happens?
> 

Oh your problem was that I did it in FreeDecodingContext() instead of
slot release, that I didn't get, yeah sure it's possibly better place.

> What do you think about something like the attached?  I've not yet
> tested it any way except running the regression tests.
> 

> - if (!already_locked)
> - LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);
> + LWLockAcquire(ReplicationSlotControlLock, LW_SHARED);

Don't really understand this change much, but otherwise the patch looks
good to me.

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


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

Re: [HACKERS] snapbuild woes

[Andres Freund]

Hi,

On 2017-04-15 05:18:49 +0200, Petr Jelinek wrote:
> Hi, here is updated patch (details inline).

I'm not yet all that happy, sorry:

Looking at 0001:
- GetOldestSafeDecodingTransactionId() only guarantees to return an xid
  safe for decoding (note how procArray->replication_slot_catalog_xmin
  is checked), not one for the initial snapshot - so afaics this whole
  exercise doesn't guarantee much so far.
- A later commit introduces need_full_snapshot as a
  CreateInitDecodingContext, but you don't use it, not here.  That seems
  wrong.
- I remain unhappy with the handling of the reset of effective_xmin in
  FreeDecodingContext(). What if we ERROR/FATAL out before that happens?

What do you think about something like the attached?  I've not yet
tested it any way except running the regression tests.

- Andres
>From b20c8e1edb31d517ecb714467a7acbeec1b926dc Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Sun, 23 Apr 2017 20:41:29 -0700
Subject: [PATCH] Preserve required !catalog tuples while computing initial
 decoding snapshot.

The logical decoding machinery already preserved all the required
catalog tuples, which is sufficient in the course of normal logical
decoding, but did not guarantee that non-catalog tuples were preserved
during computation of the initial snapshot when creating a slot over
the replication protocol.

This could cause a corrupted initial snapshot being exported.  The
time window for issues is usually not terribly large, but on a busy
server it's perfectly possible to it hit it.  Ongoing decoding is not
affected by this bug.

To avoid increased overhead for the SQL API, only retain additional
tuples when a logical slot is being created over the replication
protocol.  To do so this commit changes the signature of
CreateInitDecodingContext(), but it seems unlikely that it's being
used in an extension, so that's probably ok.

In a drive-by fix, fix handling of
ReplicationSlotsComputeRequiredXmin's already_locked argument, which
should only apply to ProcArrayLock, not ReplicationSlotControlLock.

Reported-By: Erik Rijkers
Analyzed-By: Petr Jelinek
Author: Petr Jelinek, heavily editorialized by Andres Freund
Reviewed-By: Andres Freund
Discussion: https://postgr.es/m/9a897b86-46e1-9915-ee4c-da02e4ff6...@2ndquadrant.com
Backport: 9.4, where logical decoding was introduced.
---
 src/backend/replication/logical/logical.c   | 25 +
 src/backend/replication/logical/snapbuild.c | 12 
 src/backend/replication/slot.c  | 25 +
 src/backend/replication/slotfuncs.c |  4 ++--
 src/backend/replication/walsender.c |  1 +
 src/backend/storage/ipc/procarray.c | 14 +++---
 src/include/replication/logical.h   |  1 +
 src/include/storage/procarray.h |  2 +-
 8 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 5529ac8fb4..032e91c371 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -210,6 +210,7 @@ StartupDecodingContext(List *output_plugin_options,
 LogicalDecodingContext *
 CreateInitDecodingContext(char *plugin,
 		  List *output_plugin_options,
+		  bool need_full_snapshot,
 		  XLogPageReadCB read_page,
 		  LogicalOutputPluginWriterPrepareWrite prepare_write,
 		  LogicalOutputPluginWriterWrite do_write)
@@ -267,23 +268,31 @@ CreateInitDecodingContext(char *plugin,
 	 * the slot machinery about the new limit. Once that's done the
 	 * ProcArrayLock can be released as the slot machinery now is
 	 * protecting against vacuum.
+	 *
+	 * Note that, temporarily, the data, not just the catalog, xmin has to be
+	 * reserved if a data snapshot is to be exported.  Otherwise the initial
+	 * data snapshot created here is not guaranteed to be valid. After that
+	 * the data xmin doesn't need to be managed anymore and the global xmin
+	 * should be recomputed. As we are fine with losing the pegged data xmin
+	 * after crash - no chance a snapshot would get exported anymore - we can
+	 * get away with just setting the slot's
+	 * effective_xmin. ReplicationSlotRelease will reset it again.
+	 *
 	 * 
 	 */
 	LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
 
-	slot->effective_catalog_xmin = GetOldestSafeDecodingTransactionId();
-	slot->data.catalog_xmin = slot->effective_catalog_xmin;
+	xmin_horizon = GetOldestSafeDecodingTransactionId(need_full_snapshot);
+
+	slot->effective_catalog_xmin = xmin_horizon;
+	slot->data.catalog_xmin = xmin_horizon;
+	if (need_full_snapshot)
+		slot->effective_xmin = xmin_horizon;
 
 	ReplicationSlotsComputeRequiredXmin(true);
 
 	LWLockRelease(ProcArrayLock);
 
-	/*
-	 * tell the snapshot builder to only assemble snapshot once reaching the
-	 * running_xact's record with the respective xmin.
-	 */
-	xmin_horizon = slot->data.catalog_xmin;
-
 	

Re: [HACKERS] [COMMITTERS] pgsql: Replication lag tracking for walsenders

[Andres Freund]

On 2017-04-24 15:41:25 -0700, Mark Dilger wrote:
> The recent fix in 546c13e11b29a5408b9d6a6e3cca301380b47f7f has local variable 
> overwriteOK
> assigned but not used in twophase.c RecoverPreparedTransactions(void).  I'm 
> not sure if that's
> future-proofing or an oversight.  It seems to be used in other functions.  
> Just FYI.

That's just a temporary workaround while the problem's being analyzed.

- Andres


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

Re: [HACKERS] [COMMITTERS] pgsql: Replication lag tracking for walsenders

[Mark Dilger]

> On Apr 23, 2017, at 7:53 PM, Tom Lane  wrote:
> 
> Thomas Munro  writes:
>> On Sun, Apr 23, 2017 at 6:01 PM, Tom Lane  wrote:
>>> Fair enough.  But I'd still like an explanation of why only about
>>> half of the population is showing a failure here.  Seems like every
>>> machine should be seeing the LSN as moving backwards in this test.
>>> So (a) why aren't they all failing, and (b) should we change the
>>> test to make sure every platform sees that happening?
> 
>> Every machine sees the LSN moving backwards, but the code path that
>> had the assertion only reached if it decides to interpolate, which is
>> timing dependent: there needs to be a future sample in the lag
>> tracking buffer, which I guess is not the case in those runs.
> 
> I'm dissatisfied with this explanation because if it's just timing,
> it doesn't seem very likely that some machines would reproduce the
> failure every single time while others never would.  Maybe that can be
> blamed on kernel scheduler vagaries + different numbers of cores, but
> I can't escape the feeling that there's something here we've not
> fully understood.
> 
> While chasing after this earlier today, I turned on some debug logging
> and noted that the standby's reports look like
> 
> 2017-04-23 15:46:46.206 EDT [34829] LOG:  database system is ready to accept 
> read only connections
> 2017-04-23 15:46:46.212 EDT [34834] LOG:  fetching timeline history file for 
> timeline 2 from primary server
> 2017-04-23 15:46:46.212 EDT [34834] LOG:  started streaming WAL from primary 
> at 0/300 on timeline 1
> 2017-04-23 15:46:46.213 EDT [34834] LOG:  sending write 0/302 flush 
> 0/3028470 apply 0/3028470
> 2017-04-23 15:46:46.214 EDT [34834] LOG:  replication terminated by primary 
> server
> 2017-04-23 15:46:46.214 EDT [34834] DETAIL:  End of WAL reached on timeline 1 
> at 0/3028470.
> 2017-04-23 15:46:46.214 EDT [34834] LOG:  sending write 0/3028470 flush 
> 0/3028470 apply 0/3028470
> 2017-04-23 15:46:46.214 EDT [34830] LOG:  new target timeline is 2
> 2017-04-23 15:46:46.214 EDT [34834] LOG:  restarted WAL streaming at 
> 0/300 on timeline 2
> 2017-04-23 15:46:46.228 EDT [34834] LOG:  sending write 0/302 flush 
> 0/3028470 apply 0/3028470
> 
> So you're right that the standby's reported "write" position can go
> backward, but it seems pretty darn odd that the flush and apply
> positions didn't go backward too.  Is there a bug there?
> 
> I remain of the opinion that if we can't tell from the transmitted
> data whether a timeline switch has caused the position to go backward,
> then that's a protocol shortcoming that ought to be fixed.

The recent fix in 546c13e11b29a5408b9d6a6e3cca301380b47f7f has local variable 
overwriteOK
assigned but not used in twophase.c RecoverPreparedTransactions(void).  I'm not 
sure if that's
future-proofing or an oversight.  It seems to be used in other functions.  Just 
FYI.

Mark Dilger



-- 
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] Unportable implementation of background worker start

[Tom Lane]

Andres Freund  writes:
> On 2017-04-24 18:14:41 -0400, Tom Lane wrote:
>> A bit of googling establishes that NetBSD 5.1 has a broken pselect
>> implementation:
>> 
>> http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=43625

> Yikes.  Do I understand correctly that they effectively just mapped
> pselect to select?

That's what it sounds like.  Probably not intentionally, but in effect.

>> What I'm inclined to do is to revert the pselect change but not the other,
>> to see if that fixes these two animals.  If it does, we could look into
>> blacklisting these particular platforms when choosing pselect.

> Seems sensible.

Will do.

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] Unportable implementation of background worker start

[Andres Freund]

On 2017-04-24 18:14:41 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-04-24 17:33:39 -0400, Tom Lane wrote:
> >> coypu's problem is unrelated:
> 
> > Note I was linking the 9.6 report form coypu, not HEAD. Afaics the 9.6
> > failure is the same as gharial's mode of failure.
> 
> [ looks closer... ]  Oh: the 9.6 run occurred first, and the failures on
> HEAD and 9.5 are presumably follow-on damage because the stuck postmaster
> hasn't released semaphores.
> 
> A bit of googling establishes that NetBSD 5.1 has a broken pselect
> implementation:
> 
> http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=43625

Yikes.  Do I understand correctly that they effectively just mapped
pselect to select?


> What I'm inclined to do is to revert the pselect change but not the other,
> to see if that fixes these two animals.  If it does, we could look into
> blacklisting these particular platforms when choosing pselect.

Seems sensible.

- Andres


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

Re: [HACKERS] Unportable implementation of background worker start

[Tom Lane]

Andres Freund  writes:
> On 2017-04-24 17:33:39 -0400, Tom Lane wrote:
>> coypu's problem is unrelated:

> Note I was linking the 9.6 report form coypu, not HEAD. Afaics the 9.6
> failure is the same as gharial's mode of failure.

[ looks closer... ]  Oh: the 9.6 run occurred first, and the failures on
HEAD and 9.5 are presumably follow-on damage because the stuck postmaster
hasn't released semaphores.

A bit of googling establishes that NetBSD 5.1 has a broken pselect
implementation:

http://gnats.netbsd.org/cgi-bin/query-pr-single.pl?number=43625

That says they fixed it in later versions but not 5.1 :-(

I can't find any similar smoking gun on the web for HPUX, but
I'd fully expect their bug database to be behind a paywall.

What I'm inclined to do is to revert the pselect change but not the other,
to see if that fixes these two animals.  If it does, we could look into
blacklisting these particular platforms when choosing pselect.

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] walsender & parallelism

[Petr Jelinek]

On 24/04/17 20:00, Andres Freund wrote:
> On 2017-04-24 18:29:51 +0200, Petr Jelinek wrote:
>> On 24/04/17 07:42, Andres Freund wrote:
>>>
>>>
>>> On April 23, 2017 10:31:18 PM PDT, Petr Jelinek 
>>>  wrote:
 On 24/04/17 04:31, Petr Jelinek wrote:
 So actually maybe running regression tests through it might be
 reasonable approach if we add new make target for it.
>>>
>>> That sounds like a good plan.
>>>
>>>
 Note that the first patch is huge. That's because I needed to add
 alternative output for largeobject test because it uses fastpath
 function calls which are not allowed over replication protocol.
>>>
>>> There's no need for that restriction, is there?  At least for db 
>>> walsenders...
>>>
>>
>> No, there is no real need to restring the extended protocol either but
>> we do so currently. The point of allowing SQL was to allow logical
>> replication to work, not to merge walsender completely into normal
>> backend code.
> 
> Well, that's understandable, but there's also the competing issue that
> we need something that is well defined and behaved.
> 

It's well defined, it supports simple protocol queries, that's it.

> 
>> Obviously it
>> means walsender is still special but as I said, my plan was to make it
>> work for logical replication not to merge it completely with existing
>> backends.
> 
> Yea, and I don't think that's an argument for anything on its own,
> sorry.
> 

It's not meant argument, it's plain statement of my intentions. I am not
stopping you from doing more if you want, however I don't see that it's
needed or any arguments about why it is needed.

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


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

Re: [HACKERS] Unportable implementation of background worker start

[Andres Freund]

On 2017-04-24 17:33:39 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > On 2017-04-24 13:16:44 -0700, Andres Freund wrote:
> >> Unclear if related, but
> >> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gharial=2017-04-24%2019%3A30%3A42
> >> has a suspicious timing of failing in a weird way.
> 
> > Given that gharial is also failing on 9.6 (same set of commits) and
> > coypu fails (again same set) on 9.6
> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=coypu=2017-04-24%2018%3A20%3A33
> 
> coypu's problem is unrelated:
> 
> running bootstrap script ... 2017-04-24 23:04:53.084 CEST [21114] FATAL:  
> could not create semaphores: No space left on device
> 2017-04-24 23:04:53.084 CEST [21114] DETAIL:  Failed system call was 
> semget(1, 17, 03600).

Note I was linking the 9.6 report form coypu, not HEAD. Afaics the 9.6
failure is the same as gharial's mode of failure.

- Andres


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

Re: [HACKERS] Unportable implementation of background worker start

[Tom Lane]

Andres Freund  writes:
> On 2017-04-24 13:16:44 -0700, Andres Freund wrote:
>> Unclear if related, but
>> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gharial=2017-04-24%2019%3A30%3A42
>> has a suspicious timing of failing in a weird way.

> Given that gharial is also failing on 9.6 (same set of commits) and
> coypu fails (again same set) on 9.6
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=coypu=2017-04-24%2018%3A20%3A33

coypu's problem is unrelated:

running bootstrap script ... 2017-04-24 23:04:53.084 CEST [21114] FATAL:  could 
not create semaphores: No space left on device
2017-04-24 23:04:53.084 CEST [21114] DETAIL:  Failed system call was semget(1, 
17, 03600).

but it does seem likely that one of these patches broke gharial.
That's pretty annoying :-(

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] DELETE and UPDATE with LIMIT and ORDER BY

[Jeff Janes]

On Mon, Apr 24, 2017 at 8:09 AM, Surafel Temesgen 
wrote:

> the necessity of allowing limit and order by clause to be used with delete
> and
> update statement is discussed in the past and added to the todo list
>
> preveouse mailing list descissions
>
>  http://archives.postgresql.org/pgadmin-hackers/2010-04/msg00078.php
>  http://archives.postgresql.org/pgsql-hackers/2010-11/msg01997.php
>

See this more recent one:

https://www.postgresql.org/message-id/flat/54102581.2020207%40joh.to#54102581.2020...@joh.to

That patch was not adopted, as I recall, mostly due to the requirement that
it support partitioned tables.


> i attached a small patch for its implementation.
>
> Notice : inorder to avoid unpredictable result the patch did not allow
> limit clause without order by and vise versal.
>

I think both of those are ill-advised.  To avoid deadlock, it is perfectly
fine to want an order by without a limit.

And to facilitate the reorganization of partitions or the population of new
columns in bite-size chunks, it is also fine to want limit without order by.

Cheers,

Jeff

Re: [HACKERS] to-do item for explain analyze of hash aggregates?

[Andres Freund]

On 2017-04-24 21:13:16 +0200, Tomas Vondra wrote:
> On 04/24/2017 08:52 PM, Andres Freund wrote:
> > On 2017-04-24 11:42:12 -0700, Jeff Janes wrote:
> > > The explain analyze of the hash step of a hash join reports something like
> > > this:
> > >
> > >->  Hash  (cost=458287.68..458287.68 rows=24995368 width=37) (actual
> > > rows=24995353 loops=1)
> > >  Buckets: 33554432  Batches: 1  Memory Usage: 2019630kB
> > >
> > >
> > > Should the HashAggregate node also report on Buckets and Memory Usage?  I
> > > would have found that useful several times.  Is there some reason this is
> > > not wanted, or not possible?
> >
> > I've wanted that too.  It's not impossible at all.

> Why wouldn't that be possible? We probably can't use exactly the same
> approach as Hash, because hashjoins use custom hash table while hashagg uses
> dynahash IIRC. But why couldn't measure the amount of memory by looking at
> the memory context, for example?

Doesn't use dynahash anymore (but a simplehash.h style table) anymore,
but that should actually make it simpler, not harder.

- Andres


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

Re: [HACKERS] to-do item for explain analyze of hash aggregates?

[Andres Freund]

On 2017-04-24 13:55:57 -0700, Jeff Janes wrote:
> On Mon, Apr 24, 2017 at 12:13 PM, Tomas Vondra  I've added it to the wiki Todo page.  (Hopefully that has not doomed it to
> be forgotten about)

The easiest way to avoid that fate is to implement it yourself ;)


-- 
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] to-do item for explain analyze of hash aggregates?

[Tomas Vondra]

On 04/24/2017 10:55 PM, Jeff Janes wrote:

On Mon, Apr 24, 2017 at 12:13 PM, Tomas Vondra
> wrote:

On 04/24/2017 08:52 PM, Andres Freund wrote:

...

I've wanted that too.  It's not impossible at all.


Why wouldn't that be possible? We probably can't use exactly the
same approach as Hash, because hashjoins use custom hash table while
hashagg uses dynahash IIRC. But why couldn't measure the amount of
memory by looking at the memory context, for example?


He said "not impossible", meaning it is possible.



Ah, the dreaded double negative ...


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] to-do item for explain analyze of hash aggregates?

[Jeff Janes]

On Mon, Apr 24, 2017 at 12:13 PM, Tomas Vondra  wrote:

> On 04/24/2017 08:52 PM, Andres Freund wrote:
>
>> On 2017-04-24 11:42:12 -0700, Jeff Janes wrote:
>>
>>> The explain analyze of the hash step of a hash join reports something
>>> like
>>> this:
>>>
>>>->  Hash  (cost=458287.68..458287.68 rows=24995368 width=37) (actual
>>> rows=24995353 loops=1)
>>>  Buckets: 33554432  Batches: 1  Memory Usage: 2019630kB
>>>
>>>
>>> Should the HashAggregate node also report on Buckets and Memory Usage?  I
>>> would have found that useful several times.  Is there some reason this is
>>> not wanted, or not possible?
>>>
>>
>> I've wanted that too.  It's not impossible at all.
>>
>>
> Why wouldn't that be possible? We probably can't use exactly the same
> approach as Hash, because hashjoins use custom hash table while hashagg
> uses dynahash IIRC. But why couldn't measure the amount of memory by
> looking at the memory context, for example?
>

He said "not impossible", meaning it is possible.

I've added it to the wiki Todo page.  (Hopefully that has not doomed it to
be forgotten about)

Cheers,

Jeff

Re: [HACKERS] pgbench tap tests & minor fixes

[Nikolay Shaplov]

В письме от 24 апреля 2017 09:01:18 пользователь Fabien COELHO написал:
> > To sum up:
> > 
> > - I agree to add a generic command TestLib & a wrapper in PostgresNode,
> > 
> >   instead of having pgbench specific things in the later, then call
> >   them from pgbench test script.
> > 
> > - I still think that moving the pgbench scripts inside the test script
> > 
> >   is a bad idea (tm).

My sum up is the following:

I see my job as a reviewer is to tell "I've read the code, and I am sure it is 
good".

I can tell this about this code, if:

- There is no pgbench specific staff in src/test/perl. Or there should be 
_really_big_ reason for it. 

- All the testing is done via calls of TestLib.pm functions. May be these 
functions should be improved somehow. May be there should be some warper 
around them. But not direct IPC::Run::run call.

- All the pgbench scripts are stored in one file. 36 files are not acceptable.  
I would include them in the test script itself. May be it can be done in other 
ways. But not 36 less then 100 byte files in source code tree...


May be I am wrong. I am not a guru. But then somebody else should say "I've 
read the code, and I am sure it is good" instead of me. And it would be his 
responsibility then. But if you ask me, issues listed above are very 
important, and until they are solved I can not tell "the code is good", and I 
see no way to persuade me. May be just ask somebody else...


-- 
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.


-- 
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 - Tcl 8.6 version support for PostgreSQL

[Dave Page]

On Mon, Apr 24, 2017 at 9:26 PM, Andres Freund  wrote:

> On 2017-04-24 16:18:30 -0400, Robert Haas wrote:
> > On Sat, Apr 22, 2017 at 1:58 PM, Sandeep Thakkar <
> > sandeep.thak...@enterprisedb.com> wrote:
> >
> > > Tcl8.6 is already supported in PostgreSQL.
> > >
> >
> > What commit added support for it?
>
> I don't think the main build mechanism requires explicit support of new
> versions. configure just checks for some prerequisites.
>

Right - and they were adjusted here: https://git.postgresql.
org/gitweb/?p=postgresql.git;a=commit;h=eaba54c20c5ab2cb6aaffa57fd
4990dfe2c7


-- 
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Patch - Tcl 8.6 version support for PostgreSQL

[Andres Freund]

On 2017-04-24 16:18:30 -0400, Robert Haas wrote:
> On Sat, Apr 22, 2017 at 1:58 PM, Sandeep Thakkar <
> sandeep.thak...@enterprisedb.com> wrote:
> 
> > Tcl8.6 is already supported in PostgreSQL.
> >
> 
> What commit added support for it?

I don't think the main build mechanism requires explicit support of new
versions. configure just checks for some prerequisites.

- Andres


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

Re: [HACKERS] Unportable implementation of background worker start

[Andres Freund]

On 2017-04-24 13:16:44 -0700, Andres Freund wrote:
> Unclear if related, but
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gharial=2017-04-24%2019%3A30%3A42
> has a suspicious timing of failing in a weird way.

Given that gharial is also failing on 9.6 (same set of commits) and
coypu fails (again same set) on 9.6
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=coypu=2017-04-24%2018%3A20%3A33

I'm afraid it's more likely to be related.

gharial & coypu owners, any chance you could try starting postgres with
log_min_messages=debug5 on one of the affected machines?

- Andres


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

Re: [HACKERS] Patch - Tcl 8.6 version support for PostgreSQL

[Robert Haas]

On Sat, Apr 22, 2017 at 1:58 PM, Sandeep Thakkar <
sandeep.thak...@enterprisedb.com> wrote:

> Tcl8.6 is already supported in PostgreSQL.
>

What commit added support for it?

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

Re: [HACKERS] Adding support for Default partition in partitioning

[Robert Haas]

On Mon, Apr 24, 2017 at 8:14 AM, Ashutosh Bapat
 wrote:
> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas  wrote:
>> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed  wrote:
>>> Following can also be considered as it specifies more clearly that the
>>> partition holds default values.
>>>
>>> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
>>
>> The partition doesn't contain default values; it is itself a default.
>
> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more 
> natural.

I suspect it could be done as of now, but I'm a little worried that it
might create grammar conflicts in the future as we extend the syntax
further.  If we use CREATE TABLE ... PARTITION OF .. DEFAULT, then the
word DEFAULT appears in the same position where we'd normally have FOR
VALUES, and so the parser will definitely be able to figure out what's
going on.  When it gets to that position, it will see FOR or it will
see DEFAULT, and all is clear.  OTOH, if we use CREATE TABLE ...
DEFAULT PARTITION OF ..., then we have action at a distance: whether
or not the word DEFAULT is present before PARTITION affects which
tokens are legal after the parent table name.  bison isn't always very
smart about that kind of thing.  No particular dangers come to mind at
the moment, but it makes me nervous anyway.

-- 
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] Unportable implementation of background worker start

[Andres Freund]

On 2017-04-21 23:50:41 -0400, Tom Lane wrote:
> I wrote:
> > Attached is a lightly-tested draft patch that converts the postmaster to
> > use a WaitEventSet for waiting in ServerLoop.  I've got mixed emotions
> > about whether this is the direction to proceed, though.
> 
> Attached are a couple of patches that represent a plausible Plan B.
> The first one changes the postmaster to run its signal handlers without
> specifying SA_RESTART.  I've confirmed that that seems to fix the
> select_parallel-test-takes-a-long-time problem on gaur/pademelon.
> The second one uses pselect, if available, to replace the unblock-signals/
> select()/block-signals dance in ServerLoop.  On platforms where pselect
> exists and works properly, that should fix the race condition I described
> previously.  On platforms where it doesn't, we're no worse off than
> before.
> 
> As mentioned in the comments for the second patch, even if we don't
> have working pselect(), the only problem is that ServerLoop's response
> to an interrupt might be delayed by as much as the up-to-1-minute timeout.
> The only existing case where that's really bad is launching multiple
> bgworkers.  I would therefore advocate also changing maybe_start_bgworker
> to start up to N bgworkers per call, where N is large enough to pretty
> much always satisfy simultaneously-arriving requests.  I'd pick 100 or
> so, but am willing to negotiate.
> 
> I think that these patches represent something we could back-patch
> without a lot of trepidation, unlike the WaitEventSet-based approach.
> Therefore, my proposal is to apply and backpatch these changes, and
> call it good for v10.  For v11, we could work on changing the postmaster
> to not do work in signal handlers, as discussed upthread.  That would
> supersede these two patches completely, though I'd still advocate for
> keeping the change in maybe_start_bgworker.
> 
> Note: for testing purposes, these patches are quite independent; just
> ignore the hunk in the second patch that changes a comment added by
> the first one.

Unclear if related, but
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=gharial=2017-04-24%2019%3A30%3A42
has a suspicious timing of failing in a weird way.

- Andres


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

Re: [HACKERS] pgbench tap tests & minor fixes

[Nikolay Shaplov]

В письме от 23 апреля 2017 22:02:25 пользователь Fabien COELHO написал:
> Hello Nikolay,
> 
> >> Hmmm. The pre-existing TAP test in pgbench is about concurrent commits,
> >> it
> >> is not to test pgbench itself. Pgbench allows to run some programmable
> >> clients in parallel very easily, which cannot be done simply otherwise. I
> >> think that having it there would encourage such uses.
> > 
> > Since none of us is Tom Lane, I think we have no moral right to encourage
> > somebody doing somebody in postgres.
> 
> I do not get it.
> 
> > People do what they think better to do.
> 
> Probably.
> 
> >> [...] abstraction. For me, all pg standard executables could have their
> >> methods in PostgresNode.pm.
> > 
> > you are speaking about
> > local $ENV{PGPORT} = $self->port;
> > ?
> 
> Yes. My point is that this is the internal stuff of PostgresNode and that
> it should stay inside that class. The point of the class is to organize
> postgres instances for testing, including client-server connections. From
> an object oriented point of view, the method to identify the server could
> vary, thus the testing part should not need to know, unless what is tested
> is this connection variations, hence it make sense to have it there.
> 
> > Why do you need it here after all? Lots of TAP tests for bin utilities
> > runs
> > them using command_like function from TestLib.pm and need no setting
> > $ENV{PGPORT}.
> 
> The test I've seen that do not need it do not connect to the server.
> In order to connect to the server they get through variants from
> PostgresNode which set the variables before calling the TestLib function.
> 
> > Is pgbench so special? If it is so, may be it is good reason to
> > fix utilities from TestLib.pm, so they can take port from PostgresNode.
> 
> Nope. TestLib does not know about PostgresNode, the idea is rather that
> PostgresNode knows and wraps around TestLib when needed.

Actually as I look at this part, I feeling an urge to rewrite this code, and 
change it so, that all command_like calls were called in a context of certain 
node, but it is subject for another patch. In this patch it would be good to 
use TestLib functions as they are now, or with minimum modifications.
 
> > If in these tests we can use command_like instead of pgbench_likes it
> > would increase maintainability of the code.
> 

> "command_like" variants do not look precisely at all results (status,
> stdout, stderr) and are limited to what they check (eg one regex at a
> time). Another thing I do not like is that they expect commands as a list
> of pieces, which is not very readable.
checking all this things sounds as a good idea. 

> 
> Now I can add a command_likes which allows to manage status, stdout and
> stderr and add that in TestLib & PostgresNode .
This is good idea too, I still do not understand why do you want to add it to 
PostgresNode. 

And name command_likes -- a very bad solution, as it can easily be confused 
with command_like. If it is possible to do it backward compatible, I would try 
to improve command_like, so it can check all the results. If it is not, I 
would give this function a name that brings no confusion.

> >>> I think all the data from this file should be somehow imported into
> >>> 001_pgbench.pl and these files should be created only when tests is
> >>> running, and then removed when testing is done.
> >> 
> >> Hmmm. I thought of that but was very unconvinced by the approach: pgbench
> >> basically always requires a file, so what the stuff was doing was
> >> writting
> >> the script into a file, checking for possible errors when doing so, then
> >> unlinking the file and checking for errors again after the run.
> > 
> > I think I was wrong about deleting file after tests are finished. Better
> > keep them.
> 
> Hmmm... Then what is the point not to have them as files to begin with?

Not to have them in source code tree in git.

The rest would be in the answer to another sum up letter.

-- 
Nikolay Shaplov, independent Perl & C/C++ developer. Available for hire.


-- 
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] Vacuum: allow usage of more than 1GB of work mem

[Claudio Freire]

On Sun, Apr 23, 2017 at 12:41 PM, Robert Haas  wrote:
>> That's after inlining the compare on both the linear and sequential
>> code, and it seems it lets the compiler optimize the binary search to
>> the point where it outperforms the sequential search.
>>
>> That's not the case when the compare isn't inlined.
>>
>> That seems in line with [1], that show the impact of various
>> optimizations on both algorithms. It's clearly a close enough race
>> that optimizations play a huge role.
>>
>> Since we're not likely to go and implement SSE2-optimized versions, I
>> believe I'll leave the binary search only. That's the attached patch
>> set.
>
> That sounds reasonable based on your test results.  I guess part of
> what I was wondering is whether a vacuum on a table large enough to
> require multiple gigabytes of work_mem isn't likely to be I/O-bound
> anyway.  If so, a few cycles one way or the other other isn't likely
> to matter much.  If not, where exactly are all of those CPU cycles
> going?

I haven't been able to produce a table large enough to get a CPU-bound
vacuum, so such a case is likely to require huge storage and a very
powerful I/O system. Mine can only get about 100MB/s tops, and at that
speed, vacuum is I/O bound even for multi-GB work_mem. That's why I've
been using the reported CPU time as benchmark.

BTW, I left the benchmark script running all weekend at the office,
and when I got back a power outage had aborted it. In a few days I'll
be out on vacation, so I'm not sure I'll get the benchmark results
anytime soon. But this patch moved to 11.0 I guess there's no rush.

Just FTR, in case I leave before the script is done, the script got to
scale 400 before the outage:

INFO:  vacuuming "public.pgbench_accounts"
INFO:  scanned index "pgbench_accounts_pkey" to remove 4000 row versions
DETAIL:  CPU: user: 5.94 s, system: 1.26 s, elapsed: 26.77 s.
INFO:  "pgbench_accounts": removed 4000 row versions in 655739 pages
DETAIL:  CPU: user: 3.36 s, system: 2.57 s, elapsed: 61.67 s.
INFO:  index "pgbench_accounts_pkey" now contains 0 row versions in 109679 pages
DETAIL:  4000 index row versions were removed.
109289 index pages have been deleted, 0 are currently reusable.
CPU: user: 0.00 s, system: 0.00 s, elapsed: 0.06 s.
INFO:  "pgbench_accounts": found 38925546 removable, 0 nonremovable
row versions in 655738 out of 655738 pages
DETAIL:  0 dead row versions cannot be removed yet, oldest xmin: 1098
There were 0 unused item pointers.
Skipped 0 pages due to buffer pins, 0 frozen pages.
0 pages are entirely empty.
CPU: user: 15.34 s, system: 6.95 s, elapsed: 126.21 s.
INFO:  "pgbench_accounts": truncated 655738 to 0 pages
DETAIL:  CPU: user: 0.22 s, system: 2.10 s, elapsed: 8.10 s.

In summary:

binsrch v10:

s100: CPU: user: 3.02 s, system: 1.51 s, elapsed: 16.43 s.
s400: CPU: user: 15.34 s, system: 6.95 s, elapsed: 126.21 s.

The old results:

Old Patched (sequential search):

s100: CPU: user: 3.21 s, system: 1.54 s, elapsed: 18.95 s.
s400: CPU: user: 14.03 s, system: 6.35 s, elapsed: 107.71 s.
s4000: CPU: user: 228.17 s, system: 108.33 s, elapsed: 3017.30 s.

Unpatched:

s100: CPU: user: 3.39 s, system: 1.64 s, elapsed: 18.67 s.
s400: CPU: user: 15.39 s, system: 7.03 s, elapsed: 114.91 s.
s4000: CPU: user: 282.21 s, system: 105.95 s, elapsed: 3017.28 s.

I wouldn't fret over the slight slowdown vs the old patch, it could be
noise (the script only completed a single run at scale 400).


-- 
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] DELETE and UPDATE with LIMIT and ORDER BY

[Jeevan Ladhe]

Hi Surafel,

IIUC, the requirement of the feature also had one of the consideration where
one needs to delete large data and that takes long time, and adding LIMIT
should reduce the overhead by allowing to delete the data in batches.

I did a quick performance test, and in following example you can see the
conventional delete taking "355.288 ms" VS "1137.248 ms" with new LIMIT
syntax.

postgres=# create table mytab(a int, b varchar(50));
CREATE TABLE
postgres=# insert into mytab(a, b)
select i,md5(random()::text) from generate_series(1, 100) s(i);
INSERT 0 100
postgres=# \timing
Timing is on.
postgres=# delete from mytab order by a limit 20 offset 0;
DELETE 20
*Time: 1137.248 ms (00:01.137)*
postgres=# truncate mytab;
TRUNCATE TABLE
Time: 21.717 ms
postgres=# insert into mytab(a, b)
select i,md5(random()::text) from generate_series(1, 100) s(i);
INSERT 0 100
Time: 3166.445 ms (00:03.166)
postgres=# delete from mytab where a < 21;
DELETE 20
*Time: 355.288 ms*

Am I missing something here?

Regards,
Jeevan Ladhe

Re: [HACKERS] to-do item for explain analyze of hash aggregates?

[Tomas Vondra]

On 04/24/2017 08:52 PM, Andres Freund wrote:

On 2017-04-24 11:42:12 -0700, Jeff Janes wrote:

The explain analyze of the hash step of a hash join reports something like
this:

   ->  Hash  (cost=458287.68..458287.68 rows=24995368 width=37) (actual
rows=24995353 loops=1)
 Buckets: 33554432  Batches: 1  Memory Usage: 2019630kB


Should the HashAggregate node also report on Buckets and Memory Usage?  I
would have found that useful several times.  Is there some reason this is
not wanted, or not possible?


I've wanted that too.  It's not impossible at all.



Why wouldn't that be possible? We probably can't use exactly the same 
approach as Hash, because hashjoins use custom hash table while hashagg 
uses dynahash IIRC. But why couldn't measure the amount of memory by 
looking at the memory context, for example?


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] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

[Andres Freund]

On 2017-04-24 13:29:11 +0100, Simon Riggs wrote:
> On 24 April 2017 at 00:25, Andres Freund  wrote:
> > if the subxid->xid mapping doesn't actually exist - as it's the case
> > with this bug afaics - we'll not get the correct toplevel
> > transaction.
> 
> The nature of the corruption is that in some cases
> * a subxid will point to nothing (even though in most cases it was
> already set correctly)
> * the parent will point to a subxid

Right. Those cases aren't that different from the point of trying to
find the parent of an subxid.


> > Which'll mean the following block:
> > /*
> >  * We now have either a top-level xid higher than xmin or an
> >  * indeterminate xid. We don't know whether it's top level 
> > or subxact
> >  * but it doesn't matter. If it's present, the xid is 
> > visible.
> >  */
> > for (j = 0; j < snapshot->subxcnt; j++)
> > {
> > if (TransactionIdEquals(xid, snapshot->subxip[j]))
> > return true;
> > }
> > won't work correctly if suboverflowed.
> 
> Your example of snapshots taken during recovery is not correct.

Oh?


> Note that SubTransGetTopmostTransaction() returns a valid, running
> xid, even though it is the wrong one.

Sure.


> Snapshots work differently on standbys - we store all known running
> xids, so the test still passes correctly, even when overflowed.

I don't think that's generally true.  Isn't that precisely what
ProcArrayStruct->lastOverflowedXid is about?  If we have a snapshot
that's suboverflowed due to the lastOverflowedXid cutoff, then we the
subxip array does *not* contain all known running xids anymore, we rely
on pg_subtrans to only guarantee that toplevel xids are stored in the
KnownAssignedXids machinery.

See:
 * When we throw away subXIDs from KnownAssignedXids, we need to keep track of
 * that, similarly to tracking overflow of a PGPROC's subxids array.  We do
 * that by remembering the lastOverflowedXID, ie the last thrown-away subXID.
 * As long as that is within the range of interesting XIDs, we have to assume
 * that subXIDs are missing from snapshots.  (Note that subXID overflow occurs
 * on primary when 65th subXID arrives, whereas on standby it occurs when 64th
 * subXID arrives - that is not an error.)

/*
 * Highest subxid that has been removed from KnownAssignedXids array to
 * prevent overflow; or InvalidTransactionId if none.  We track this for
 * similar reasons to tracking overflowing cached subxids in PGXACT
 * entries.  Must hold exclusive ProcArrayLock to change this, and 
shared
 * lock to read it.
 */
TransactionId lastOverflowedXid;


Greetings,

Andres Freund


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

Re: [HACKERS] to-do item for explain analyze of hash aggregates?

[Andres Freund]

On 2017-04-24 11:42:12 -0700, Jeff Janes wrote:
> The explain analyze of the hash step of a hash join reports something like
> this:
> 
>->  Hash  (cost=458287.68..458287.68 rows=24995368 width=37) (actual
> rows=24995353 loops=1)
>  Buckets: 33554432  Batches: 1  Memory Usage: 2019630kB
> 
> 
> Should the HashAggregate node also report on Buckets and Memory Usage?  I
> would have found that useful several times.  Is there some reason this is
> not wanted, or not possible?

I've wanted that too.  It's not impossible at all.

- Andres


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

Re: [HACKERS] OK, so culicidae is *still* broken

[Andres Freund]

On 2017-04-24 11:08:48 -0700, Andres Freund wrote:
> On 2017-04-24 23:14:40 +0800, Craig Ringer wrote:
> > In the long run we'll probably be forced toward threading or far pointers.
> 
> I'll vote for removing the windows port, before going for that.  And I'm
> not even joking.

Just to clarify: I'm talking about far pointers here, not threading.


-- 
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] OK, so culicidae is *still* broken

[Andres Freund]

On 2017-04-24 14:43:11 -0400, Tom Lane wrote:
> (We have accepted that kind of overhead for DSM segments, but the
> intention I think is to allow only very trivial data structures in
> the DSM segments.  Losing compiler pointer type checking for data
> structures like the lock or PGPROC tables would be horrid.)

The relptr.h infrastructure brings some of the type-checking back, but
it's still pretty painful.  And just as important, it's quite noticeable
performance-wise.  So we have to do it for dynamic shm (until/unless we
go to using threads), but that doesn't mean we should do it for some of
the most performance critical data structures in PG...

- Andres


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

Re: [HACKERS] Cached plans and statement generalization

[Andres Freund]

Hi,

On 2017-04-24 11:46:02 +0300, Konstantin Knizhnik wrote:
> So what I am thinking now is implicit query caching. If the same query with
> different literal values is repeated many times, then we can try to
> generalize this query and replace it with prepared query with
> parameters.

That's not actuall all that easy:
- You pretty much do parse analysis to be able to do an accurate match.
  How much overhead is parse analysis vs. planning in your cases?
- The invalidation infrastructure for this, if not tied to to fully
  parse-analyzed statements, is going to be hell.
- Migrating to parameters can actually cause significant slowdowns, not
  nice if that happens implicitly.

Greetings,

Andres Freund


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

Re: [HACKERS] OK, so culicidae is *still* broken

[Tom Lane]

Andres Freund  writes:
> On 2017-04-24 23:14:40 +0800, Craig Ringer wrote:
>> In the long run we'll probably be forced toward threading or far pointers.

> I'll vote for removing the windows port, before going for that.  And I'm
> not even joking.

Me too.  We used to *have* that kind of code, ie relative pointers into
the shmem segment, and it was a tremendous notational mess and very
bug-prone.  I do not wish to go back.

(We have accepted that kind of overhead for DSM segments, but the
intention I think is to allow only very trivial data structures in
the DSM segments.  Losing compiler pointer type checking for data
structures like the lock or PGPROC tables would be horrid.)

regards, tom lane


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

[HACKERS] to-do item for explain analyze of hash aggregates?

[Jeff Janes]

The explain analyze of the hash step of a hash join reports something like
this:

   ->  Hash  (cost=458287.68..458287.68 rows=24995368 width=37) (actual
rows=24995353 loops=1)
 Buckets: 33554432  Batches: 1  Memory Usage: 2019630kB


Should the HashAggregate node also report on Buckets and Memory Usage?  I
would have found that useful several times.  Is there some reason this is
not wanted, or not possible?

Cheers,

Jeff

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

On Mon, Apr 24, 2017 at 5:44 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas 
> wrote:
> > On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed 
> wrote:
> >> Following can also be considered as it specifies more clearly that the
> >> partition holds default values.
> >>
> >> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
> >
> > Yes, that could be done.  But I don't think it's correct to say that
> > the partition holds default values.  Let's back up and ask what the
> > word "default" means.  The relevant definition (according to Google or
> > whoever they stole it from) is:
> >
> > a preselected option adopted by a computer program or other mechanism
> > when no alternative is specified by the user or programmer.
> >
> > So, a default *value* is the value that is used when no alternative is
> > specified by the user or programmer. We have that concept, but it's
> > not what we're talking about here: that's configured by applying the
> > DEFAULT property to a column.  Here, we're talking about the default
> > *partition*, or in other words the *partition* that is used when no
> > alternative is specified by the user or programmer.  So, that's why I
> > proposed the syntax I did.  The partition doesn't contain default
> > values; it is itself a default.
>
> Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more
> natural.
>

+1

Re: [HACKERS] Adding support for Default partition in partitioning

[Jeevan Ladhe]

Hi Rahila,

I tried to go through your v7 patch, and following are my comments:

1.
With -Werrors I see following compilation failure:

parse_utilcmd.c: In function ‘transformPartitionBound’:
parse_utilcmd.c:3309:4: error: implicit declaration of function
‘isDefaultPartitionBound’ [-Werror=implicit-function-declaration]
if (!(isDefaultPartitionBound(value)))
^
cc1: all warnings being treated as errors

You need to include, "catalog/partitions.h".

2.
Once I made above change pass, I see following error:
tablecmds.c: In function ‘DefineRelation’:
tablecmds.c:762:17: error: unused variable ‘partdesc’
[-Werror=unused-variable]
   PartitionDesc partdesc;
 ^
cc1: all warnings being treated as errors

3.
Please remove the extra line at the end of the function
check_new_partition_bound:
+ MemoryContextSwitchTo(oldCxt);
+ heap_endscan(scan);
+ UnregisterSnapshot(snapshot);
+ heap_close(defrel, AccessExclusiveLock);
+ ExecDropSingleTupleTableSlot(tupslot);
+ }
+
 }

4.
In generate_qual_for_defaultpart() you do not need 2 pointers for looping
over
bound specs:
+ ListCell   *cell1;
+ ListCell   *cell3;
You can iterate twice using one pointer itself.

Same is for:
+ ListCell   *cell2;
+ ListCell   *cell4;

Similarly, in get_qual_from_partbound(), you can use one pointer below,
instead of cell1 and cell3:
+ PartitionBoundSpec *bspec;
+ ListCell *cell1;
+ ListCell *cell3;

5.
Should this have a break in if block?
+ foreach(cell1, bspec->listdatums)
+ {
+ Node *value = lfirst(cell1);
+ if (isDefaultPartitionBound(value))
+ {
+ def_elem = true;
+ *defid = inhrelid;
+ }
+ }

6.
I am wondering, isn't it possible to retrieve the has_default and
default_index
here to find out if default partition exists and if exist then find it's oid
using rd_partdesc, that would avoid above(7) loop to check if partition
bound is
default.

7.
The output of describe needs to be improved.
Consider following case:
postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
(4,5,4,4,4,6,2);
ERROR:  relation "list_partitioned" does not exist
postgres=# CREATE TABLE list_partitioned (
a int
) PARTITION BY LIST (a);
CREATE TABLE
postgres=# CREATE TABLE part_1 PARTITION OF list_partitioned FOR VALUES IN
(4,5,4,4,4,6,2);
CREATE TABLE
postgres=# CREATE TABLE part_default PARTITION OF list_partitioned FOR
VALUES IN (DEFAULT, 3, DEFAULT, 3, DEFAULT);
CREATE TABLE
postgres=# \d+ part_1;
  Table "public.part_1"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
 a  | integer |   |  | | plain   |
 |
Partition of: list_partitioned FOR VALUES IN (4, 5, 6, 2)

postgres=# \d+ part_default;
   Table "public.part_default"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
 a  | integer |   |  | | plain   |
 |
Partition of: list_partitioned FOR VALUES IN (DEFAULT3DEFAULTDEFAULT)

As you can see in above example, part_1 has multiple entries for 4 while
creating the partition, but describe shows only one entry for 4 in values
set.
Similarly, part_default has multiple entries for 3 and DEFAULT while
creating
the partition, but the describe shows a weired output. Instead, we should
have
just one entry saying "VALUES IN (DEFAULT, 3)":

postgres=# \d+ part_default;
   Table "public.part_default"
 Column |  Type   | Collation | Nullable | Default | Storage | Stats target
| Description
+-+---+--+-+-+--+-
 a  | integer |   |  | | plain   |
 |
Partition of: list_partitioned FOR VALUES IN (DEFAULT, 3)

8.
Following call to find_inheritance_children() in
generate_qual_for_defaultpart()
is an overhead, instead we can simply use an array of oids in rd_partdesc.

+ spec = (PartitionBoundSpec *) bound;
+
+ inhoids = find_inheritance_children(RelationGetRelid(parent), NoLock);
+
+ foreach(cell2, inhoids)

Same is for the call in get_qual_from_partbound:

+ /* Collect bound spec nodes in a list. This is done if the partition is
+ * a default partition. In case of default partition, constraint is formed
+ * by performing <> operation over the partition constraints of the
+ * existing partitions.
+ */
+ inhoids = find_inheritance_children(RelationGetRelid(parent), NoLock);
+ foreach(cell2, inhoids)

9.
How about rephrasing following error message:
postgres=# CREATE TABLE part_2 PARTITION OF list_partitioned FOR VALUES IN
(14);
ERROR:  new default partition constraint is violated by some row

To,
"ERROR: some existing row in default partition violates new default
partition constraint"

10.
Additionally, I did test your given sample test in first 

Re: [HACKERS] OK, so culicidae is *still* broken

[Andres Freund]

Hi,

On 2017-04-24 14:25:34 +0530, Amit Kapila wrote:
> Error code 87 means "invalid parameter".  Some googling [1] indicates
> such an error occurs if we pass the out-of-range address to
> MapViewOfFileEx. Another possible theory is that we must pass the
> address as multiple of the system's memory allocation granularity as
> that is expected by specs of MapViewOfFileEx.  I can try doing that
> but I am not confident that this is the right approach because of
> below text mentioned in docs (msdn) of MapViewOfFileEx.
> "While it is possible to specify an address that is safe now (not used
> by the operating system), there is no guarantee that the address will
> remain safe over time. Therefore, it is better to let the operating
> system choose the address.".

Sure, that's the same as mmap() etc say. I'd not be overly deterred by
that.


On 2017-04-24 23:14:40 +0800, Craig Ringer wrote:
> In the long run we'll probably be forced toward threading or far pointers.

I'll vote for removing the windows port, before going for that.  And I'm
not even joking.


Greetings,

Andres Freund


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

Re: [HACKERS] pg_dump emits ALTER TABLE ONLY partitioned_table

[Robert Haas]

On Mon, Apr 24, 2017 at 9:17 AM, Stephen Frost  wrote:
> I wonder why the restriction is there, which is probably part of the
> reason that I'm thinking of phrasing the documentation that way.
>
> Beyond a matter of round to-its, is there a reason why it couldn't (or
> shouldn't) be supported?  I'm not suggesting now, of course, but in a
> future release.

I don't see any particular reason, but I haven't looked into the
matter deeply.  One thing to consider is that you can already more or
less do exactly that thing, like this:

rhaas=# create table foo (a int, b text) partition by range (a);
CREATE TABLE
Time: 4.738 ms
rhaas=# create table foo1 partition of foo for values from (0) to (100);
CREATE TABLE
Time: 5.162 ms
rhaas=# insert into foo1 select 1, 'snarf' from generate_series(1,1000) g;
INSERT 0 1000
Time: 12835.153 ms (00:12.835)
rhaas=# alter table foo1 add constraint xyz check (b != 'nope');
ALTER TABLE
Time: 1059.728 ms (00:01.060)
rhaas=# alter table foo add constraint xyz check (b != 'nope');
NOTICE:  merging constraint "xyz" with inherited definition
ALTER TABLE
Time: 1.243 ms

So we may not really need another way to do it.

>> Also, I think saying that it it will result in an error is a bit more
>> helpful than saying that it is is not supported, since the latter
>> might lead someone to believe that it could result in undefined
>> behavior (i.e. arbitrarily awful misbehavior) which is not the case.
>> So I like what he wrote, for whatever that's worth.
>
> I don't mind stating that it'll result in an error, I just don't want to
> imply that there's some way to get around that error if things were done
> differently.
>
> How about:
>
> ---
> Once partitions exist, using ONLY will always result
> in an error as adding or dropping constraints on only the partitiioned
> table, when partitions exist, is not supported.
> ---

I still prefer Amit's language, but it's not worth to me to keep
arguing about it.  If you prefer what you've written there, fine, but
let's get something committed and move on.  I think we understand what
needs to be fixed here now, and I'd like to do get that done.  If you
don't want to do it or don't have time to do it, I'll pick it up
again, but let's not let this keep dragging out.

-- 
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] walsender & parallelism

[Andres Freund]

On 2017-04-24 18:29:51 +0200, Petr Jelinek wrote:
> On 24/04/17 07:42, Andres Freund wrote:
> > 
> > 
> > On April 23, 2017 10:31:18 PM PDT, Petr Jelinek 
> >  wrote:
> >> On 24/04/17 04:31, Petr Jelinek wrote:
> >> So actually maybe running regression tests through it might be
> >> reasonable approach if we add new make target for it.
> > 
> > That sounds like a good plan.
> > 
> > 
> >> Note that the first patch is huge. That's because I needed to add
> >> alternative output for largeobject test because it uses fastpath
> >> function calls which are not allowed over replication protocol.
> > 
> > There's no need for that restriction, is there?  At least for db 
> > walsenders...
> > 
> 
> No, there is no real need to restring the extended protocol either but
> we do so currently. The point of allowing SQL was to allow logical
> replication to work, not to merge walsender completely into normal
> backend code.

Well, that's understandable, but there's also the competing issue that
we need something that is well defined and behaved.


> Obviously it
> means walsender is still special but as I said, my plan was to make it
> work for logical replication not to merge it completely with existing
> backends.

Yea, and I don't think that's an argument for anything on its own,
sorry.

- Andres


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

Re: [HACKERS] Interval for launching the table sync worker

[Masahiko Sawada]

On Tue, Apr 25, 2017 at 1:42 AM, Petr Jelinek
 wrote:
> On 24/04/17 17:52, Masahiko Sawada wrote:
>> On Mon, Apr 24, 2017 at 4:41 PM, Kyotaro HORIGUCHI
>>  wrote:
>> + /*
>> + * Remove entries no longer necessary. The flag signals nothing if
>> + * subrel_local_state is not updated above. We can remove entries in
>> + * frozen hash safely.
>> + */
>> + if (local_state_updated && !wstate->alive)
>> + {
>> + hash_search(subrel_local_state, >rs.relid,
>> + HASH_REMOVE, NULL);
>> + continue;
>> + }
>>
>> IIUC since the apply worker can change the status from
>> SUBREL_STATE_SYNCWAIT to SUBREL_STATE_READY in a hash_seq_search loop
>> the table sync worker which is changed to SUBREL_STATE_READY by the
>> apply worker before updating the subrel_local_state could be remained
>> in the hash table. I think that we should scan pg_subscription_rel by
>> using only a condition "subid".
>>
>
> I don't follow this.
>

Hmm, I'd misunderstood something. It should work fine. Sorry for the noise.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
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] Interval for launching the table sync worker

[Petr Jelinek]

On 24/04/17 17:52, Masahiko Sawada wrote:
> On Mon, Apr 24, 2017 at 4:41 PM, Kyotaro HORIGUCHI
>  wrote:
> + /*
> + * Remove entries no longer necessary. The flag signals nothing if
> + * subrel_local_state is not updated above. We can remove entries in
> + * frozen hash safely.
> + */
> + if (local_state_updated && !wstate->alive)
> + {
> + hash_search(subrel_local_state, >rs.relid,
> + HASH_REMOVE, NULL);
> + continue;
> + }
> 
> IIUC since the apply worker can change the status from
> SUBREL_STATE_SYNCWAIT to SUBREL_STATE_READY in a hash_seq_search loop
> the table sync worker which is changed to SUBREL_STATE_READY by the
> apply worker before updating the subrel_local_state could be remained
> in the hash table. I think that we should scan pg_subscription_rel by
> using only a condition "subid".
> 

I don't follow this.

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


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

Re: [HACKERS] Remove dead interfaces added by mistake in 7c4f52409

[Fujii Masao]

On Sun, Apr 23, 2017 at 8:15 AM, Petr Jelinek
 wrote:
> Hi,
>
> Seems like couple of declarations for couple of functions we never
> actually implemented and are not used got past review of logical
> replication support for initial copy path (commit 7c4f52409).
>
> Attached patch gets rid of them.

Pushed. Thanks!

Regards,

-- 
Fujii Masao


-- 
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] walsender & parallelism

[Petr Jelinek]

On 24/04/17 07:42, Andres Freund wrote:
> 
> 
> On April 23, 2017 10:31:18 PM PDT, Petr Jelinek 
>  wrote:
>> On 24/04/17 04:31, Petr Jelinek wrote:
>> So actually maybe running regression tests through it might be
>> reasonable approach if we add new make target for it.
> 
> That sounds like a good plan.
> 
> 
>> Note that the first patch is huge. That's because I needed to add
>> alternative output for largeobject test because it uses fastpath
>> function calls which are not allowed over replication protocol.
> 
> There's no need for that restriction, is there?  At least for db walsenders...
> 

No, there is no real need to restring the extended protocol either but
we do so currently. The point of allowing SQL was to allow logical
replication to work, not to merge walsender completely into normal
backend code. And I don't know about differences well enough to go for
the full merge, especially not at this stage of PG10 dev.

>> As part of this I realized that the parser fallback that I wrote
>> originally for handling SQL in walsender is not robust enough as the
>> lexer would fail on some valid SQL statements. So there is also second
>> patch that does this using different approach which allows any SQL
>> statement.
> 
> Haven't looked at the new thing yet, but I'm not particularly surprised...  
> Wonder of there's a good way to fully integrate both grammars, without 
> exploding keywords.
> 

I think we'd need to keyword the first word of every replication command
if we wanted to merge both grammar files together. I am not sure if
there is all that much need for that, the pass-through for everything
that is not replication command seems to work just fine. Obviously it
means walsender is still special but as I said, my plan was to make it
work for logical replication not to merge it completely with existing
backends.

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


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

Re: [HACKERS] PG_GETARG_GISTENTRY?

[Mark Dilger]

> On Apr 5, 2017, at 1:27 PM, Mark Dilger  wrote:
> 
> 
>> On Apr 5, 2017, at 1:12 PM, Tom Lane  wrote:
>> 
>> Mark Dilger  writes:
>>> I have written a patch to fix these macro definitions across src/ and 
>>> contrib/.
>>> Find the patch, attached.  All regression tests pass on my Mac laptop.
>> 
>> Thanks for doing the legwork on that.  
> 
> You are welcome.
> 
>> This seems a bit late for v10,
>> especially since it's only cosmetic
> 
> Agreed.
> 
>> , but please put it in the first
>> v11 commitfest.
> 
> Done.
> 
>> 
>>> I don't find any inappropriate uses of _P where _PP would be called for.  I 
>>> do,
>>> however, notice that some datatypes' functions are written to use 
>>> PG_GETARG_*_P
>>> where PG_GETARG_*_PP might be more efficient.
>> 
>> Yeah.  I think Noah did some work in that direction already, but I don't
>> believe he claimed to have caught everything.  Feel free to push further.
> 
> Thanks for clarifying.
> 

Here is a small patch for the next open commitfest which handles a case
that Noah's commits 9d7726c2ba06b932f791f2d0cc5acf73cc0b4dca and
3a0d473192b2045cbaf997df8437e7762d34f3ba apparently missed.

Noah, if you left this case out intentionally, sorry for the noise.  I did not
immediately see any reason not to follow your lead for this function.

Mark Dilger



varbit_packed.patch.1
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] Quorum commit for multiple synchronous replication.

[Fujii Masao]

On Mon, Apr 24, 2017 at 2:55 PM, Masahiko Sawada  wrote:
> On Thu, Apr 20, 2017 at 9:31 AM, Kyotaro HORIGUCHI
>  wrote:
>> Ok, I got the point.
>>
>> At Wed, 19 Apr 2017 17:39:01 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
>>  wrote in 
>> <20170419.173901.16598616.horiguchi.kyot...@lab.ntt.co.jp>
>>> > >> |
>>> > >> | Quorum-based synchronous replication is basically more
>>> > >> | efficient than priority-based one when you specify multiple
>>> > >> | standbys in synchronous_standby_names and want
>>> > >> | to synchronously replicate transactions to two or more of
>>> > >> | them.
>>
>> "Some" means "not all".
>>
>>> > >> | In the priority-based case, the replication master
>>> > >> | must wait for a reply from the slowest standby in the
>>> > >> | required number of standbys in priority order, which may
>>> > >> | slower than the rest.
>>
>>
>> Quorum-based synchronous replication is expected to be more
>> efficient than priority-based one when your master doesn't need
>> to be in sync with all of the nominated standbys by
>> synchronous_standby_names.

This description may be invalid in the case where the requested number
of sync standbys is smaller than the number of "nominated" standbys by
s_s_names. For example, please imagine the case where there are five
standbys nominated by s_s_name, the requested number of sync standbys
is 2, and only two sync standbys are running. In this case, the master
needs to wait for those two standbys whatever the sync rep method is.
I think that we should rewrite that to something like "quorum-based
synchronous replication is more effecient when the requested number
of synchronous standbys is smaller than the number of potential
synchronous standbys running".

>  While quorum-based
>> replication master waits only for a specified number of fastest
>> standbys, priority-based replicatoin master must wait for
>> standbys at the top of the list, which may be slower than the
>> rest.
>
> This description looks good to me. I've updated the patch based on
> this description and attached it.

But I still think that the original description that I used in my patch is
better than this

Regards,

-- 
Fujii Masao


-- 
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] Quorum commit for multiple synchronous replication.

[Fujii Masao]

On Mon, Apr 24, 2017 at 9:02 AM, Noah Misch  wrote:
> On Thu, Apr 20, 2017 at 11:34:34PM -0700, Noah Misch wrote:
>> On Fri, Apr 21, 2017 at 01:20:05PM +0900, Masahiko Sawada wrote:
>> > On Fri, Apr 21, 2017 at 12:02 PM, Noah Misch  wrote:
>> > > On Wed, Apr 19, 2017 at 01:52:53PM +0900, Masahiko Sawada wrote:
>> > >> On Wed, Apr 19, 2017 at 12:34 PM, Noah Misch  wrote:
>> > >> > On Sun, Apr 16, 2017 at 07:25:28PM +0900, Fujii Masao wrote:
>> > >> >> As I told firstly this is not a bug. There are some proposals for 
>> > >> >> better design
>> > >> >> of priority column in pg_stat_replication, but we've not reached the 
>> > >> >> consensus
>> > >> >> yet. So I think that it's better to move this open item to "Design 
>> > >> >> Decisions to
>> > >> >> Recheck Mid-Beta" section so that we can hear more opinions.
>> > >> >
>> > >> > I'm reading that some people want to report NULL priority, some 
>> > >> > people want to
>> > >> > report a constant 1 priority, and nobody wants the current behavior.  
>> > >> > Is that
>> > >> > an accurate summary?
>> > >>
>> > >> Yes, I think that's correct.
>> > >
>> > > Okay, but ...
>> > >
>> > >> FWIW the reason of current behavior is that it would be useful for the
>> > >> user who is willing to switch from ANY to FIRST. They can know which
>> > >> standbys will become sync or potential.
>> > >
>> > > ... does this mean you personally want to keep the current behavior?  If 
>> > > not,
>> > > has some other person stated a wish to keep the current behavior?
>> >
>> > No, I want to change the current behavior. IMO it's better to set
>> > priority 1 to all standbys in quorum set. I guess there is no longer
>> > person who supports the current behavior.
>>
>> In that case, this open item is not eligible for section "Design Decisions to
>> Recheck Mid-Beta".  That section is for items where we'll probably change
>> nothing, but we plan to recheck later just in case.  Here, we expect to 
>> change
>> the behavior; the open question is which replacement behavior to prefer.
>>
>> Fujii, as the owner of this open item, you are responsible for moderating the
>> debate until there's adequate consensus to make a particular change or to 
>> keep
>> the current behavior after all.  Please proceed to do that.  Beta testers
>> deserve a UI they may like, not a UI you already plan to change later.
>
> Please observe the policy on open item ownership[1] and send a status update
> within three calendar days of this message.  Include a date for your
> subsequent status update.

Okay, so our consensus is to always set the priorities of sync standbys
to 1 in quorum-based syncrep case. Attached patch does this change.
Barrying any objection, I will commit this.

I will commit something to close this open item by April 28th at the latest
(IOW before my vacation starts).

Regards,

-- 
Fujii Masao


sync_priority.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] Interval for launching the table sync worker

[Masahiko Sawada]

On Mon, Apr 24, 2017 at 4:41 PM, Kyotaro HORIGUCHI
 wrote:
> Hello,
>
> At Sun, 23 Apr 2017 00:51:52 +0900, Masahiko Sawada  
> wrote in 
>> On Fri, Apr 21, 2017 at 11:19 PM, Masahiko Sawada  
>> wrote:
>> > On Fri, Apr 21, 2017 at 5:33 PM, Kyotaro HORIGUCHI
>> >  wrote:
>> >> Hello,
>> >>
>> >> At Thu, 20 Apr 2017 13:21:14 +0900, Masahiko Sawada 
>> >>  wrote in 
>> >> 

Re: [HACKERS] A note about debugging TAP failures

[Craig Ringer]

On 24 April 2017 at 20:01, Daniel Gustafsson  wrote:

> I’m np Perl expert though so there might be better/cleaner ways to achieve
> this, in testing it seems to work though.  rmtree() is supported at least 
> since
> Perl 5.6 from what I can see.

I'd rather just have the 'make' target nuke the relevant tmp_check dir
before rerunning the tests. Right now it just deletes the logs.

-- 
 Craig Ringer   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] OK, so culicidae is *still* broken

[Craig Ringer]

On 24 April 2017 at 16:55, Amit Kapila  wrote:

> Another thing I have tried is to just start the server by setting
> RandomizedBaseAddress="TRUE".  I have tried about 15-20 times but
> could not reproduce the problem related to shared memory attach.  We
> have tried the same on one of my colleagues (Ashutosh Sharma) machine
> as well, there we could see that error once or twice out of many tries
> but couldn't get it consistently.  I think if the chances of this
> problem to occur are so less, then probably the suggestion made by Tom
> to retry if we get collision doesn't sound bad.

It's pretty uncommon, and honestly, we might well be best off just
trying again if we lose the lottery.

Most of what I read last time I looked into this essentially assumed
that you'd "fix" your code by reinventing far pointers[1], like the
good old Win16 days. Assume all addresses in shmem are relative to the
shmem base, and offset them when accessing/storing them. Fun and
efficient for everyone! That seems to be what Boost recommends[2].

Given that Pg doesn't make much effort to differentiate between
pointers to shmem and local memory, and has no pointer transformations
between shared and local pointers, adopting that would be a
horrifyingly intrusive change as well as incredibly tedious to
implement. We'd probably land up using size_t or ptrdiff_t for shmem
pointers and some kind of macro that was a noop on !windows. For once
I'd be thoroughly in agreement with Tom's complaints about
Windows-droppings.

Other people who've faced and worked around this[3] have come up with
solutions that look way scarier than just retrying if we lose the
random numbers game.

BTW, some Windows users face issues with large contiguous
allocations/mappings even without the process sharing side[4] due to
memory fragmentation created by ASLR, though this may only be a
concern for 32-bit executables. The relevant option /LARGEADDRESSAWARE
is enabled by default for 64-bit builds.

We might want to /DELAYLOAD [5] DLLs where possible to improve our
chances of winning the dice roll, but if we're going to support
retrying at all we don't need to care too much.

I looked at image binding (prelinking), but it's disabled if ASLR is in use.

In the long run we'll probably be forced toward threading or far pointers.



[1] https://en.wikipedia.org/wiki/Far_pointer,
https://en.wikipedia.org/wiki/Intel_Memory_Model#Pointer%5Fsizes

[2] 
http://www.boost.org/doc/libs/1_64_0/doc/html/interprocess/sharedmemorybetweenprocesses.html#interprocess.sharedmemorybetweenprocesses.mapped_region.mapped_region_address_mapping

[3] http://stackoverflow.com/a/36145019/398670

[4] https://github.com/golang/go/issues/2323

[5] On 24 April 2017 at 16:55, Amit Kapila 
wrote:   > Another thing I have tried is to just start the server by
setting  > RandomizedBaseAddress="TRUE".  I have tried about 15-20
times but  > could not reproduce the problem related to shared memory
attach.  We  > have tried the same on one of my colleagues (Ashutosh
Sharma) machine  > as well, there we could see that error once or
twice out of many tries  > but couldn't get it consistently.  I think
if the chances of this  > problem to occur are so less, then probably
the suggestion made by Tom  > to retry if we get collision doesn't
sound bad.   It's pretty uncommon, and honestly, we might well be best
off just trying again if we lose the lottery.   Most of what I read
last time I looked into this essentially assumed that you'd "fix" your
code by reinventing far pointers[1], like the good old Win16 days.
Assume all addresses in shmem are relative to the shmem base, and
offset them when accessing/storing them. Fun and efficient for
everyone! That seems to be what Boost recommends[2].   Given that Pg
doesn't make much effort to differentiate between pointers to shmem
and local memory, and has no pointer transformations between shared
and local pointers, adopting that would be a horrifyingly intrusive
change as well as incredibly tedious to implement. We'd probably land
up using size_t or ptrdiff_t for shmem pointers and some kind of macro
that was a noop on !windows. For once I'd be thoroughly in agreement
with Tom's complaints about Windows-droppings.   Other people who've
faced and worked around this[3] have come up with solutions that look
way scarier than just retrying if we lose the random numbers game.
BTW, some Windows users face issues with large contiguous
allocations/mappings even without the process sharing side[4] due to
memory fragmentation created by ASLR, though this may only be a
concern for 32-bit executables. The relevant option /LARGEADDRESSAWARE
is enabled by default for 64-bit builds.  We should /DELAYLOAD as many
DDLs as possible to improve our chances.   [1]
https://en.wikipedia.org/wiki/Far_pointer,
https://en.wikipedia.org/wiki/Intel_Memory_Model#Pointer%5Fsizes   [2]

[HACKERS] DELETE and UPDATE with LIMIT and ORDER BY

[Surafel Temesgen]

the necessity of allowing limit and order by clause to be used with delete
and
update statement is discussed in the past and added to the todo list

preveouse mailing list descissions

 http://archives.postgresql.org/pgadmin-hackers/2010-04/msg00078.php
 http://archives.postgresql.org/pgsql-hackers/2010-11/msg01997.php

i attached a small patch for its implementation.

Notice : inorder to avoid unpredictable result the patch did not allow
limit clause without order by and vise versal.

comment please?

Regareds

Surafel


delete_update_with_limit_orderby_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] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

[Nikhil Sontakke]

>
>  Also, when I fix that, it gets further but still crashes at the same
>  Assert in SubTransSetParent.  The proximate cause this time seems to
> be
>  that RecoverPreparedTransactions's calculation of overwriteOK is
> wrong:
>  it's computing that as "false", but in reality the subtrans link in
>  question has already been set.
> >>
> >>> Not sure about that, investigating.
> >>
> >> As a quick hack, I just hotwired RecoverPreparedTransactions to set
> >> overwriteOK = true always, and with that and the SubTransSetParent
> >> argument-order fix, HEAD passes the recovery tests.  Maybe we can
> >> be smarter than that, but this might be a good short-term fix to get
> >> the buildfarm green again.
> >
> > That would work. I've been looking into a fix I can explain, but "do
> > it always" may actually be it.
>
>
Here's what's happening:

On Master:

BEGIN;

INSERT INTO t_009_tbl VALUES (42);

SAVEPOINT s1;

INSERT INTO t_009_tbl VALUES (43);

PREPARE TRANSACTION 'xact_009_1';
On Master:

Do a fast shutdown

On Slave:

Restart the slave. This causes StandbyRecoverPreparedTransactions() to be
invoked which causes ProcessTwoPhaseBuffer() to be invoked with setParent
set to true. This sets up parent xid for the above subtransaction.

On Slave:

Promote the slave. This causes RecoverPreparedTransactions() to be invoked
which ends up calling SubTransSetParent() for the above subxid. The
overwriteOK bool remains false since we don't go over the
PGPROC_MAX_CACHED_SUBXIDS limit. Since the parent xid was already set on
restart above, we hit the assert.

---

I am wondering if StandbyRecoverPreparedTransactions() needs the parent
linkage at all? I don't see SubTransGetParent() and related functions being
called on the standby but we need to be careful here. As a quick test, If I
don't call SubTransSetParent() as part of
StandbyRecoverPreparedTransactions()  then all recovery tests pass ok. But
the fact that StandbyRecoverPreparedTransactions() takes overwriteOK as a
parameter means it might not be so simple..

Regards,
Nikhils
-- 
 Nikhil Sontakke   http://www.2ndQuadrant.com/
 PostgreSQL/Postgres-XL Development, 24x7 Support, Training & Services

Re: [HACKERS] Incorrect use of ERRCODE_UNDEFINED_COLUMN in extended stats

[Tom Lane]

David Rowley  writes:
> The attached small patched fixes an incorrect usage of an error code
> in the extended stats code.

Hmm, looks like all of that could do with an editorial pass (e.g.,
"duplicity" does not mean what the comments author seems to think).

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] Incorrect use of ERRCODE_UNDEFINED_COLUMN in extended stats

[Alvaro Herrera]

David Rowley wrote:
> The attached small patched fixes an incorrect usage of an error code
> in the extended stats code.

Thanks for the report.  I'm on vacation starting today until May 2nd.
If nobody commits this in the meantime, I'll get to it when I'm back.

-- 
Álvaro Herrerahttps://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] pg_dump emits ALTER TABLE ONLY partitioned_table

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Fri, Apr 21, 2017 at 1:43 AM, Stephen Frost  wrote:
> >> + Once
> >> +   partitions exist, we do not support using ONLY 
> >> to
> >> +   add or drop constraints on only the partitioned table.
> >>
> >> I wonder if the following sounds a bit more informative: Once partitions
> >> exist, using ONLY will result in an error, because the
> >> constraint being added or dropped must be added or dropped from the
> >> partitions too.
> >
> > My thinking here is that we may add support later on down the line for
> > using the ONLY keyword, when the constraint has already been created on
> > the partitions themselves, so I would rather just clarify that we don't
> > (yet) support that.  Your wording, at least to me, seems to imply that
> > if the constraint was added to or dropped from the partitions then the
> > ONLY keyword could be used, which isn't accurate today.
> 
> Well, I think that Amit's wording has the virtue of giving a reason
> which is basically valid, whereas someone reading your text might
> conceivably be left wondering what the reason for the restriction is.

I wonder why the restriction is there, which is probably part of the
reason that I'm thinking of phrasing the documentation that way.

Beyond a matter of round to-its, is there a reason why it couldn't (or
shouldn't) be supported?  I'm not suggesting now, of course, but in a
future release.

I could certainly see someone wanting to manage the process by which a
constriant is added by adding it one at a time to the individual
partitions and then wishing to be sure that adding it to parent only
checked that the individual partitions had the constraint and then added
it to the parent (in other words, a relatively short operation,
providing the locks are able to be acquired quickly).

> Also, I think saying that it it will result in an error is a bit more
> helpful than saying that it is is not supported, since the latter
> might lead someone to believe that it could result in undefined
> behavior (i.e. arbitrarily awful misbehavior) which is not the case.
> So I like what he wrote, for whatever that's worth.

I don't mind stating that it'll result in an error, I just don't want to
imply that there's some way to get around that error if things were done
differently.

How about:

---
Once partitions exist, using ONLY will always result
in an error as adding or dropping constraints on only the partitiioned
table, when partitions exist, is not supported.
---

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] OK, so culicidae is *still* broken

[Craig Ringer]

On 16 April 2017 at 05:18, Andres Freund  wrote:

> Because of ASLR of the main executable (i.e. something like PIE).  It'll
> supposedly become harder (as in only running in compatibility modes) if
> binaries don't enable that.  It's currently disabled somewhere in the VC
> project generated.

I thought we passed /DYNAMICBASE:NO directly , but I don't see that in
the code. A look at the git logs shows that we disabled it in
7f3e17b48 by emitting
false in the MSBuild
project. That'll pass /DYNAMICBASE:NO to the linker.

See https://msdn.microsoft.com/en-us/library/bb384887.aspx

It's rather better than the old registry hack, but it's a compat
option we're likely to lose at some point.

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


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

Re: [HACKERS] StandbyRecoverPreparedTransactions recovers subtrans links incorrectly

[Simon Riggs]

On 24 April 2017 at 00:25, Andres Freund  wrote:

>> >> It's not clear to me how much potential this has to create user data
>> >> corruption, but it doesn't look good at first glance.  Discuss.
>> >
>> > Hm. I think it can cause wrong tqual.c results in some edge cases.
>> > During HS, lastOverflowedXid will be set in some cases, and then
>> > XidInMVCCSnapshot etc will not find the actual toplevel xid, which'll in
>> > turn cause lookups snapshot->subxip (where all HS xids reside)
>> > potentially return wrong results.
>> >
>> > I was about to say that I don't see how it could result in persistent
>> > corruption however - the subtrans lookups are only done for
>> > (snapshot->xmin, snapshot->xmax] and subtrans is truncated
>> > regularly. But these days CHECKPOINT_END_OF_RECOVERY isn't obligatory
>> > anymore, so that might be delayed.  Hm.
>>
>> I've not found any reason, yet, to believe we return wrong answers in
>> any case, even though the transient data structure pg_subtrans is
>> corrupted by the switched call Tom discovers.
>
> I think I pointed out a danger above. Consider what happens if query on
> a standby has a suboverflowed snapshot:
> Snapshot
> GetSnapshotData(Snapshot snapshot)
> {
> ...
> if (TransactionIdPrecedesOrEquals(xmin, 
> procArray->lastOverflowedXid))
> suboverflowed = true;
> }
> ..
> snapshot->suboverflowed = suboverflowed;
> }
>
> In that case we rely on pg_subtrans for visibility determinations:
> bool
> HeapTupleSatisfiesMVCC(HeapTuple htup, Snapshot snapshot,
>Buffer buffer)
> {
> ...
> if (!HeapTupleHeaderXminCommitted(tuple))
> {
> ...
> else if (XidInMVCCSnapshot(HeapTupleHeaderGetRawXmin(tuple), 
> snapshot))
> return false;
>
> and
> static bool
> XidInMVCCSnapshot(TransactionId xid, Snapshot snapshot)
> {
> ...
> if (!snapshot->takenDuringRecovery)
> {
> ...
> else
> {
> ...
> if (snapshot->suboverflowed)
> {
> /*
>  * Snapshot overflowed, so convert xid to top-level.  
> This is safe
>  * because we eliminated too-old XIDs above.
>  */
> xid = SubTransGetTopmostTransaction(xid);
>
> if the subxid->xid mapping doesn't actually exist - as it's the case
> with this bug afaics - we'll not get the correct toplevel
> transaction.

The nature of the corruption is that in some cases
* a subxid will point to nothing (even though in most cases it was
already set correctly)
* the parent will point to a subxid

So both wrong.

> Which'll mean the following block:
> /*
>  * We now have either a top-level xid higher than xmin or an
>  * indeterminate xid. We don't know whether it's top level or 
> subxact
>  * but it doesn't matter. If it's present, the xid is visible.
>  */
> for (j = 0; j < snapshot->subxcnt; j++)
> {
> if (TransactionIdEquals(xid, snapshot->subxip[j]))
> return true;
> }
> won't work correctly if suboverflowed.

Your example of snapshots taken during recovery is not correct.

Note that SubTransGetTopmostTransaction() returns a valid, running
xid, even though it is the wrong one.

Snapshots work differently on standbys - we store all known running
xids, so the test still passes correctly, even when overflowed.

I'd call that just plain luck. We behave correctly, but for the wrong
reasons, at least in this case.


> I don't see anything prevent wrong results here?

I've had an even better look around now and I think I've found
something but need to turn it into a repeatable test case so I can
double-check this before reporting in full, so I don't cry wolf.

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

[Ashutosh Bapat]

On Mon, Apr 24, 2017 at 4:24 PM, Robert Haas  wrote:
> On Mon, Apr 24, 2017 at 5:10 AM, Rahila Syed  wrote:
>> Following can also be considered as it specifies more clearly that the
>> partition holds default values.
>>
>> CREATE TABLE ...PARTITION OF...FOR VALUES DEFAULT;
>
> Yes, that could be done.  But I don't think it's correct to say that
> the partition holds default values.  Let's back up and ask what the
> word "default" means.  The relevant definition (according to Google or
> whoever they stole it from) is:
>
> a preselected option adopted by a computer program or other mechanism
> when no alternative is specified by the user or programmer.
>
> So, a default *value* is the value that is used when no alternative is
> specified by the user or programmer. We have that concept, but it's
> not what we're talking about here: that's configured by applying the
> DEFAULT property to a column.  Here, we're talking about the default
> *partition*, or in other words the *partition* that is used when no
> alternative is specified by the user or programmer.  So, that's why I
> proposed the syntax I did.  The partition doesn't contain default
> values; it is itself a default.

Is CREATE TABLE ... DEFAULT PARTITION OF ... feasible? That sounds more natural.



-- 
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] A note about debugging TAP failures

[Daniel Gustafsson]

> On 23 Apr 2017, at 15:05, Craig Ringer  wrote:
> 
> On 23 Apr. 2017 10:32, "Michael Paquier"  > wrote:
> On Sun, Apr 23, 2017 at 7:48 AM, Daniel Gustafsson  > wrote:
> > Skipping the tempdir and instead using ${testname}_data_${name} without a
> > random suffix, we can achieve this with something along the lines of the
> > attached PoC.  It works as now (retain of failure, remove on success unless
> > overridden) but that logic can easily be turned around if we want that.  If
> > it’s of interest I can pursue this after some sleep (tomorrow has become 
> > today
> > at this point).
> 
> Yes, something like that may make sense as well for readability.
> 
> Keeping folders in case of failures is something that I have been
> advocating in favor of for some time, but this never got into the tree
> :(
> 
> Huh? We do keep test temp datadirs etc in case of failure. Just not on 
> success.
> 
> Our definition of failure there sucks a bit though. We keep the datadirs if 
> any test fails in a script. If the script its self crashes we still blow away 
> the datadirs which is kind of counterintuitive.
> 
> I'd like to change the __DIE__ sig handler to only delete on clean script 
> exit code, tap reporting success, and if some env bar like PG_TESTS_NOCLEAN 
> is undefined. The later could also be used in pg_regress etc.

That sounds like a good idea, even though END might be enough when not using
tempdir() for the datadirs since die() should set a non-zero $?  afaik (still
doesn’t hurt to capture with __DIE__ though).  I extended my previous test with
this, and other comments in this thread: it produces non-random datadirs,
retains them on die|exit|test-failure or if PG_TESTS_NOCLEAN is set.  Theres
also a check-clean target for cleaning out retained datadirs.

Given that the datadirs do occupy quite some space, perhaps a PG_TESTS_DOCLEAN
(or similar) would be good as well to always blow away the datadirs regardless
of test status?

I’m np Perl expert though so there might be better/cleaner ways to achieve
this, in testing it seems to work though.  rmtree() is supported at least since
Perl 5.6 from what I can see.

cheers ./daniel



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

[Rajkumar Raghuwanshi]

On Fri, Apr 21, 2017 at 7:59 PM, Ashutosh Bapat <
ashutosh.ba...@enterprisedb.com> wrote:

> Here's an updated patch set
>

Hi,

I have applied v18 patches and got a crash in m-way joins when partition
ranges differ, below are steps to reproduce this.

CREATE TABLE prt1 (a int, b int, c varchar) PARTITION BY RANGE(a);
CREATE TABLE prt1_p1 PARTITION OF prt1 FOR VALUES FROM (0) TO (250);
CREATE TABLE prt1_p3 PARTITION OF prt1 FOR VALUES FROM (500) TO (600);
CREATE TABLE prt1_p2 PARTITION OF prt1 FOR VALUES FROM (250) TO (500);
INSERT INTO prt1 SELECT i, i, to_char(i, 'FM') FROM generate_series(0,
599, 2) i;
ANALYZE prt1;

CREATE TABLE prt4_n (a int, b int, c text) PARTITION BY RANGE(a);
CREATE TABLE prt4_n_p1 PARTITION OF prt4_n FOR VALUES FROM (0) TO (300);
CREATE TABLE prt4_n_p2 PARTITION OF prt4_n FOR VALUES FROM (300) TO (500);
CREATE TABLE prt4_n_p3 PARTITION OF prt4_n FOR VALUES FROM (500) TO (600);
INSERT INTO prt4_n SELECT i, i, to_char(i, 'FM') FROM
generate_series(0, 599, 2) i;
ANALYZE prt4_n;

SET enable_partition_wise_join = on ;
EXPLAIN (COSTS OFF)
SELECT t1.a, t1.c, t2.b, t2.c FROM prt1 t1, prt4_n t2, prt1 t3 WHERE t1.a =
t2.a AND t2.a = t3.a;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation

Re: [HACKERS] Partition-wise join for join between (declaratively) partitioned tables

[Ashutosh Bapat]

On Sat, Apr 22, 2017 at 3:52 AM, Robert Haas  wrote:
> On Fri, Apr 21, 2017 at 8:41 AM, Ashutosh Bapat
>  wrote:
>> I don't see how is that fixed. For a join relation we need to come up
>> with one set of partition bounds by merging partition bounds of the
>> joining relation and in order to understand how to interpret the
>> datums in the partition bounds, we need to associate data types. The
>> question is which data type we should use if the relations being
>> joined have different data types associated with their respective
>> partition bounds.
>>
>> Or are you saying that we don't need to associate data type with
>> merged partition bounds? In that case, I don't know how do we compare
>> the partition bounds of two relations?
>
> Well, since there is no guarantee that a datatype exists which can be
> used to "merge" the partition bounds in the sense that you are
> describing, and even if there is one we have no opfamily
> infrastructure to find out which one it is, I think it would be smart
> to try to set things up so that we don't need to do that.  I believe
> that's probably possible.
>
>> In your example, A has partition key of type int8, has bound datums
>> X1.. X10. B has partition key of type int4 and has bounds datums X1 ..
>> X11. C has partition key type int2 and bound datums X1 .. X12.
>
> OK, sure.
>
>> The binary representation of X's is going to differ between A, B and C
>> although each Xk for A, B and C is equal, wherever exists.
>
> Agreed.
>
>> Join
>> between A and B will have merged bound datums X1 .. X10 (and X11
>> depending upon the join type). In order to match bounds of AB with C,
>> we need to know the data type of bounds of AB, so that we can choose
>> appropriate equality operator. The question is what should we choose
>> as data type of partition bounds of AB, int8 or int4. This is
>> different from applying join conditions between AB and C, which can
>> choose the right opfamily operator based on the join conditions.
>
> Well, the join is actually being performed either on A.keycol =
> C.keycol or on B.keycol = C.keycol, right?  It has to be one or the
> other; there's no "merged" join column in any relation's targetlist,
> but only columns derived from the various baserels.  So let's use that
> set of bounds for the matching.  It makes sense to use the set of
> bounds for the matching that corresponds to the column actually being
> joined, I think.
>
> It's late here and I'm tired, but it seems like it should be possible
> to relate the child joinrels of the AB join back to the child joinrels
> of either A or B.  (AB)1 .. (AB)10 related back to A1 .. A10 and B1 ..
> B10.  (AB)11 relates back to B11 but, of course not to A11, which
> doesn't exist.  If the join is INNER, (AB)11 is a dummy rel anyway and
> actually we should probably see whether we can omit it altogether.  If
> the join is an outer join of some kind, there's an interesting case
> where the user wrote A LEFT JOIN B or B RIGHT JOIN A so that A is not
> on the nullable side of the join; in that case, too, (AB)11 is dummy
> or nonexistent.  Otherwise, assuming A is nullable, (AB)11 maps only
> to B11 and not to A11.  But that's absolutely right: if the join to C
> uses A.keycol, either the join operator is strict and (AB)11 won't
> match anything anyway, or it's not and partition-wise join is illegal
> because A.keycol in (AB)11 can include not only values from X11 but
> also nulls.
>
> So, it seems to me that what you can do is loop over the childrels on
> the outer side of the join.  For each one, you've got a join clause
> that relates the outer rel to the inner rel, and that join clause
> mentions some baserel which is contained in the joinrel.  So drill
> down through the childrel to the corresponding partition of the
> baserel and get those bounds.  Then if you do the same thing for the
> inner childrels, you've now got two lists of bounds, and the type on
> the left matches the outer side of the join and the type on the right
> matches the inner side of the join and the opfamily of the operator in
> the join clause gives you a comparison operator that relates those two
> types, and now you can match them up.

This assumes that datums in partition bounds have one to one mapping
with the partitions, which isn't true for list partitions. For list
partitions we have multiple datums corresponding to the items listed
associated with a given partition. So, simply looping over the
partitions of outer relations doesn't work; in fact there are two
outer relations for a full outer join, so we have to loop over both of
them together in a merge join fashion.

Consider A join B where A has partitions A1 (a, b, c), A2(e, f), A3(g,
h) and B has partitions B1 (a, b), B2 (c, d, e), B3(f, g, h). If we
just look at the partitions, we won't recognize that list item c is
repeated in A1B1 and A2B2. That can be recognized only when we loop
over the datums of A and B