Re: [HACKERS] kqueue

[Thomas Munro]

On Wed, Apr 11, 2018 at 1:05 PM, Thomas Munro
 wrote:
> I heard through the grapevine of some people currently investigating
> performance problems on busy FreeBSD systems, possibly related to the
> postmaster pipe.  I suspect this patch might be a part of the solution
> (other patches probably needed to get maximum value out of this patch:
> reuse WaitEventSet objects in some key places, and get rid of high
> frequency PostmasterIsAlive() read() calls).  The autoconf-fu in the
> last version bit-rotted so it seemed like a good time to post a
> rebased patch.

Once I knew how to get a message resent to someone who wasn't
subscribed to our mailing list at the time it was sent[1] so they
could join an existing thread.  I don't know how to do that with the
new mailing list software, so I'm CC'ing Mateusz so he can share his
results on-thread.  Sorry for the noise.

[1] 
https://www.postgresql.org/message-id/CAEepm=0-KsV4Sj-0Qd4rMCg7UYdOQA=tujlkezox7h_qiqq...@mail.gmail.com

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

Re: [HACKERS] kqueue

[Mateusz Guzik]

On Mon, May 21, 2018 at 9:03 AM, Thomas Munro  wrote:

> On Wed, Apr 11, 2018 at 1:05 PM, Thomas Munro
>  wrote:
> > I heard through the grapevine of some people currently investigating
> > performance problems on busy FreeBSD systems, possibly related to the
> > postmaster pipe.  I suspect this patch might be a part of the solution
> > (other patches probably needed to get maximum value out of this patch:
> > reuse WaitEventSet objects in some key places, and get rid of high
> > frequency PostmasterIsAlive() read() calls).  The autoconf-fu in the
> > last version bit-rotted so it seemed like a good time to post a
> > rebased patch.
>
>
Hi everyone,

I have benchmarked the change on a FreeBSD box and found an big
performance win once the number of clients goes beyond the number of
hardware threads on the target machine. For smaller number of clients
the win was very modest.

The test was performed few weeks ago.

For convenience PostgreSQL 10.3 as found in the ports tree was used.

3 variants were tested:
- stock 10.3
- stock 10.3 + pdeathsig
- stock 10.3 + pdeathsig + kqueue

Appropriate patches were provided by Thomas.

In order to keep this message PG-13 I'm not going to show the actual
script, but a mere outline:

for i in $(seq 1 10): do
for t in vanilla pdeathsig pdeathsig_kqueue; do
start up the relevant version
for c in 32 64 96; do
pgbench -j 96 -c $c -T 120 -M prepared -S -U bench
-h 172.16.0.2 -P1 bench > ${t}-${c}-out-warmup 2>&1
pgbench -j 96 -c $c -T 120 -M prepared -S -U bench
-h 172.16.0.2 -P1 bench > ${t}-${c}-out 2>&1
done
shutdown the relevant version
done

Data from the warmup is not used. All the data was pre-read prior to the
test.

PostgreSQL was configured with 32GB of shared buffers and 200 max
connections, otherwise it was the default.

The server is:
Intel(R) Xeon(R) Gold 6134 CPU @ 3.20GHz
2 package(s) x 8 core(s) x 2 hardware threads

i.e. 32 threads in total.

running FreeBSD -head with 'options NUMA' in kernel config and
sysctl net.inet.tcp.per_cpu_timers=1 on top of zfs.

The load was generated from a different box over a 100Gbit ethernet link.

x cumulative-tps-vanilla-32
+ cumulative-tps-pdeathsig-32
* cumulative-tps-pdeathsig_kqueue-32
++
|+   + x+* x+  *  x   *+ * *   * * **  *  ***|
|   |_|__M_A___M_A_|| |MA|   |
++
N   Min   MaxMedian   AvgStddev
x  10 442898.77 448476.81 444805.17 445062.08 1679.7169
+  10  442057.2 447835.46 443840.28 444235.01 1771.2254
No difference proven at 95.0% confidence
*  10 448138.07 452786.41 450274.56 450311.51 1387.2927
Difference at 95.0% confidence
5249.43 +/- 1447.41
1.17948% +/- 0.327501%
(Student's t, pooled s = 1540.46)
x cumulative-tps-vanilla-64
+ cumulative-tps-pdeathsig-64
* cumulative-tps-pdeathsig_kqueue-64
++
| ** |
| ** |
|  xx  x +***|
|++**x *+*++  ***|
|  ||_A|M_|   |A |
++
N   Min   MaxMedian   AvgStddev
x  10 411849.26  422145.5 416043.77  416061.9 3763.2545
+  10 407123.74 425727.84 419908.73  417480.7 6817.5549
No difference proven at 95.0% confidence
*  10 542032.71 546106.93 543948.05 543874.06 1234.1788
Difference at 95.0% confidence
127812 +/- 2631.31
30.7195% +/- 0.809892%
(Student's t, pooled s = 2800.47)
x cumulative-tps-vanilla-96
+ cumulative-tps-pdeathsig-96
* cumulative-tps-pdeathsig_kqueue-96
++
|  * |
|  * |
|  * |
|  * |
|  + x * |
|  *xxx+   **|
|+ *+* **|
|  |MA||  |A||
+--

Message on end of cascading physical replica timeline is unhelpful

[Craig Ringer]

Hi all

By default, cascading replicas don't follow an upstream's timeline change.
I'm not arguing that decision, but as I think it violates POLA somewhat
it'd be nice to have an informative message.

Currently the user gets repeating messages like

LOG:  restarted WAL streaming at 25/CA00 on timeline 2
LOG:  replication terminated by primary server
DETAIL:  End of WAL reached on timeline 2 at 25/CAA66EE8.

which is IMO not super helpful. We'll either continue on the next timeline,
or reconnect and see if the server has more to send us on this timeline,
depending on the value of recovery_target_timeline. The standby knows what
it's doing, and it's not something coming from the upstream like this
message seems to say.

But it's not telling the user why it keeps on asking for the same timeline
over and over, though it knows perfectly well why.

xlog.c doesn't expose recoveryTargetTLI and recoveryTargetIsLatest so
walreceiver.c can't say much about them.

I'm inclined to just add a read-only accessor function for recoveryTargetTLI
and recoveryTargetIsLatest that lets the walreceiver look them up.

I'm thinking of something like

recoveryTargetIsLatest || startpointTLI < recoveryTargetTLI ?
errhint("Will look for new timelines on server and restart
streaming")
: errhint("recovery_target_timeline limits streaming to timeline
%u", startpointTLI)

It's ugly, but it's hard to come up with something succinct and
translator-friendly here, especially without duplicating the whole rest of
the error message. I can break it out to a simple function if desired.

In the mean time this message and the archives should help people whose
cascading replicas decide not to keep up with the times.

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

Re: Postgres 11 release notes

[David Rowley]

On 19 May 2018 at 03:58, Amit Langote  wrote:
> I wonder what you think about including this little performance item:
>
> https://www.postgresql.org/message-id/e1eotsq-0005v0...@gemulon.postgresql.org
>
> especially considering the part of the commit message which states
>
> ...Still, testing shows
> that this makes single-row inserts significantly faster on a table
> with many partitions without harming the bulk-insert case.
>
> I recall seeing those inserts being as much as 2x faster as partition
> count grows beyond hundreds.  One might argue that we should think
> about publicizing this only after we've dealt with the
> lock-all-partitions issue that's also mentioned in the commit message
> which is still a significant portion of the time spent and I'm totally
> fine with that.

While I do think that was a good change, I do think there's much still
left to do to speed up usage of partitioned tables with many
partitions.

I've been working a bit in this area over the past few weeks and with
PG11 I measured a single INSERT into a 10k RANGE partitioned table at
just 84 tps (!), while inserting the same row into a non-partitioned
table was about 11.1k tps. I have patches locally that take this up to
~9.8k tps, which I'll submit for PG12. I'm unsure if we should be
shouting anything from the rooftops about the work done in this area
for PG11, since it's still got a long way to go still before the
feature is usable with higher numbers of partitions. I do think your
change was a good one to make, but I just don't want users to think
that we're done here when we all know that much work remains.

If we're going to add an item in the release notes about this then I
wouldn't object, providing it could be done in a way that indicates
we've not finished here yet, but if that's the case then maybe it's
better to say nothing at all.

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

Re: Postgres, fsync, and OSs (specifically linux)

[Craig Ringer]

On 21 May 2018 at 12:57, Craig Ringer  wrote:

> On 18 May 2018 at 00:44, Andres Freund  wrote:
>
>> Hi,
>>
>> On 2018-05-10 09:50:03 +0800, Craig Ringer wrote:
>> >   while ((src = (RewriteMappingFile *)
>> hash_seq_search(&seq_status)) != NULL)
>> >   {
>> >   if (FileSync(src->vfd, WAIT_EVENT_LOGICAL_REWRITE_SYNC)
>> != 0)
>> > - ereport(ERROR,
>> > + ereport(PANIC,
>> >   (errcode_for_file_access(),
>> >errmsg("could not fsync file
>> \"%s\": %m", src->path)));
>>
>> To me this (and the other callers) doesn't quite look right. First, I
>> think we should probably be a bit more restrictive about when PANIC
>> out. It seems like we should PANIC on ENOSPC and EIO, but possibly not
>> others.  Secondly, I think we should centralize the error handling. It
>> seems likely that we'll acrue some platform specific workarounds, and I
>> don't want to copy that knowledge everywhere.
>>
>> Also, don't we need the same on close()?
>>
>>
> Yes, we do, and that expands the scope a bit.
>
> I agree with Robert that some sort of filter/macro is wise, though naming
> it clearly will be tricky.
>
> I'll have a look.
>
>
On the queue for tomorrow.


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

Re: Postgres 11 release notes

[Haribabu Kommi]

On Sat, May 12, 2018 at 1:08 AM, Bruce Momjian  wrote:

> I have committed the first draft of the Postgres 11 release notes.  I
> will add more markup soon.  You can view the most current version here:
>
> http://momjian.us/pgsql_docs/release-11.html
>

Thanks for preparing the release notes.

>Have pg_dump dump all aspects of a database (Haribabu Kommi)
>
>pg_dump and pg_restore, without --clean, no longer dump/restore database
> comments and security labels.

There is small change in option name, the option to print database comments
is --create not --clean.


>Have libpq's PQhost() always return the actual connected host (Hari Babu)
>
>Previously PQhost() often returned the supplied host parameters, which
could contain
>several hosts. The same is true of PQport(), which now returns the actual
port number,
>not the multiple supplied port numbers. ACCURATE?

Now PQhost() can return hostaddr also if host is not available. How about
changing as below?

Have libpq's PQhost() always return the actual connected host/hostaddr
(Haribabu Kommi)

hostaddr is returned, when there is no host available with the connected
host.


Regards,
Haribabu Kommi
Fujitsu Australia

Add --include-table-data-where option to pg_dump, to export only a subset of table data

[Carter Thaxton]

Many times I've wanted to export a subset of a database, using some sort of
row filter condition on some of the large tables.  E.g. copying a
production database to a staging environment, but with some time series
data only from the past month.

We have the existing options:
  --include-table=table(and its -t synonym)
  --exclude-table=table
  --exclude-table-data=table

I propose a new option:
  --include-table-data-where=table:filter_clause

One would use this option as follows:

  pg_dump --include-table-data-where=largetable:"created_at >=
'2018-05-01'" database_name

The filter_clause is used as the contents of a WHERE clause when querying
the data to generate the COPY statement produced by pg_dump.

I've prepared a proposed patch for this, which is attached.  The code
changes are rather straightforward.  I did have to add the ability to carry
around an extra pointer-sized object to the simple_list implementation, in
order to allow the filter clause to be associated to the matching oids of
the table pattern.  It seemed the best way to augment the existing
simple_list implementation, but change as little as possible elsewhere in
the codebase.  (Note that SimpleOidList is actually only used by pg_dump).

Feel free to review and propose any amendments.


pgdump-include-table-data-where-v1.patch
Description: Binary data

PostgreSQL: PY3 files in PY2 installation

[Devrim Gündüz]

Hi,

The following files are installed even when I build PostgreSQL 9.5+ on RHEL 7 -
Python 2.7:

hstore_plpython3u--1.0.sql
hstore_plpython3u.control
jsonb_plpython3u--1.0.sql
jsonb_plpython3u.control
ltree_plpython3u--1.0.sql
ltree_plpython3u.control

Is this expected?

Regards,
-- 
Devrim Gündüz
EnterpriseDB: https://www.enterprisedb.com
PostgreSQL Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

signature.asc
Description: This is a digitally signed message part

Re: PostgreSQL: PY3 files in PY2 installation

[Devrim Gündüz]

Hi,

On Mon, 2018-05-21 at 12:42 +0100, Devrim Gündüz wrote:
> The following files are installed even when I build PostgreSQL 9.5+ on RHEL 7
> -
> Python 2.7:
> 
> hstore_plpython3u--1.0.sql
> hstore_plpython3u.control
> jsonb_plpython3u--1.0.sql
> jsonb_plpython3u.control
> ltree_plpython3u--1.0.sql
> ltree_plpython3u.control
> 
> Is this expected?

This is likely a spec file issue. Sorry for the noise.

Regards,
-- 
Devrim Gündüz
EnterpriseDB: https://www.enterprisedb.com
PostgreSQL Consultant, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

signature.asc
Description: This is a digitally signed message part

Re: Flexible permissions for REFRESH MATERIALIZED VIEW

[Robert Haas]

On Sat, May 19, 2018 at 12:59 PM, Greg Stark  wrote:
> On 19 May 2018 at 01:13, Stephen Frost  wrote:
>> I'm not entirely sure about the varlena suggestion, seems like that
>> would change a great deal more code and be slower, though perhaps not
>> enough to matter; it's not like our aclitem arrays are exactly optimized
>> for speed today.
>
> I don't actually understand the reason y'all are talking about
> varlena.

Because aclitem's typlen value in pg_type is currently "12".

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

Re: PostgreSQL: PY3 files in PY2 installation

[Tom Lane]

Devrim =?ISO-8859-1?Q?G=FCnd=FCz?=  writes:
> On Mon, 2018-05-21 at 12:42 +0100, Devrim Gündüz wrote:
>> The following files are installed even when I build PostgreSQL 9.5+ on RHEL 7
>> Python 2.7:
>> 
>> hstore_plpython3u--1.0.sql
>> hstore_plpython3u.control
>> jsonb_plpython3u--1.0.sql
>> jsonb_plpython3u.control
>> ltree_plpython3u--1.0.sql
>> ltree_plpython3u.control
>> 
>> Is this expected?

> This is likely a spec file issue. Sorry for the noise.

No, I see it happen here too, eg

$ cd contrib/hstore_plpython
$ make install
...
/bin/mkdir -p '/home/postgres/installdir/lib'
/bin/mkdir -p '/home/postgres/installdir/share/extension'
/bin/mkdir -p '/home/postgres/installdir/share/extension'
/usr/bin/install -c -m 755  hstore_plpython2.so 
'/home/postgres/installdir/lib/hstore_plpython2.so'
/usr/bin/install -c -m 644 ./hstore_plpythonu.control 
./hstore_plpython2u.control ./hstore_plpython3u.control 
'/home/postgres/installdir/share/extension/'
/usr/bin/install -c -m 644 ./hstore_plpythonu--1.0.sql 
./hstore_plpython2u--1.0.sql ./hstore_plpython3u--1.0.sql  
'/home/postgres/installdir/share/extension/'

Seems pretty bogus, since only one .so is getting installed.

regards, tom lane

Re: [HACKERS] Aggregate transition state merging vs. hypothetical set functions

[Peter Eisentraut]

On 10/13/17 19:01, Tom Lane wrote:
>> Moving on to the exact color of the bikeshed: it seems like the right
>> way to present this to users of CREATE AGGREGATE is in terms of "does
>> the final function modify the transition state?".  So maybe the values
>> could be spelled
>> SMODIFY = READ_ONLY   ffunc never touches state, ok as window agg
>> SMODIFY = SHARABLEffunc does some one-time change like sorting,
>>   so state merging is OK but not window agg
>> SMODIFY = READ_WRITE  ffunc trashes state, can't do merging either
>> I'm not set on these names by any means; anyone have a better idea?

Is "sharable" the preferred spelling, as opposed to "shareable"?

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

Re: [HACKERS] Aggregate transition state merging vs. hypothetical set functions

[Tom Lane]

Peter Eisentraut  writes:
> On 10/13/17 19:01, Tom Lane wrote:
> SMODIFY = SHARABLEffunc does some one-time change like sorting,
> so state merging is OK but not window agg

> Is "sharable" the preferred spelling, as opposed to "shareable"?

Hmm ... I looked in two different dictionaries, and they both say
sharable is a variant of shareable.  So while it's not wrong exactly,
I seem to have picked the less preferred spelling.

I could run around and change it, but I'd have to do so *right now*
I think --- once we release beta1 the costs of changing this would
go up.

Will do so if there's not objections in an hour or so.

regards, tom lane

Re: pg_basebackup -k option

[Peter Eisentraut]

On 5/18/18 11:26, Michael Banck wrote:
>>> How about using capital -K in pg_basebackup?  Or maybe it doesn't need a
>>> short option at all.
>>
>> +1 for no short option.
> 
> Makes sense to me, I wasn't happy about the -k back then (and I think I
> solicited feedback on that).
> 
> PFA a patch which should remove the short option.

committed (needed test and documentation adjustments, too)

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

Re: Postgres 11 release notes

[Amit Langote]

On Mon, May 21, 2018 at 4:34 PM, David Rowley
 wrote:
> On 19 May 2018 at 03:58, Amit Langote  wrote:
>> I wonder what you think about including this little performance item:
>>
>> https://www.postgresql.org/message-id/e1eotsq-0005v0...@gemulon.postgresql.org
>>
>> especially considering the part of the commit message which states
>>
>> ...Still, testing shows
>> that this makes single-row inserts significantly faster on a table
>> with many partitions without harming the bulk-insert case.
>>
>> I recall seeing those inserts being as much as 2x faster as partition
>> count grows beyond hundreds.  One might argue that we should think
>> about publicizing this only after we've dealt with the
>> lock-all-partitions issue that's also mentioned in the commit message
>> which is still a significant portion of the time spent and I'm totally
>> fine with that.
>
> While I do think that was a good change, I do think there's much still
> left to do to speed up usage of partitioned tables with many
> partitions.
>
> I've been working a bit in this area over the past few weeks and with
> PG11 I measured a single INSERT into a 10k RANGE partitioned table at
> just 84 tps (!), while inserting the same row into a non-partitioned
> table was about 11.1k tps. I have patches locally that take this up to
> ~9.8k tps, which I'll submit for PG12. I'm unsure if we should be
> shouting anything from the rooftops about the work done in this area
> for PG11, since it's still got a long way to go still before the
> feature is usable with higher numbers of partitions. I do think your
> change was a good one to make, but I just don't want users to think
> that we're done here when we all know that much work remains.
>
> If we're going to add an item in the release notes about this then I
> wouldn't object, providing it could be done in a way that indicates
> we've not finished here yet, but if that's the case then maybe it's
> better to say nothing at all.

You're right, it surely isn't time yet to make fanfare about this.  I
will look forward to being able to review your patches. :-)

Thanks,
Amit

RE: [HACKERS] Moving relation extension locks out of heavyweight lock manager

[Alex Ignatov]

-Original Message-
From: Robert Haas  
Sent: Thursday, April 26, 2018 10:25 PM
To: Andres Freund 
Cc: Masahiko Sawada ; Michael Paquier 
; Mithun Cy ; Tom Lane 
; Thomas Munro ; Amit Kapila 
; PostgreSQL-development 
Subject: Re: [HACKERS] Moving relation extension locks out of heavyweight lock 
manager

On Thu, Apr 26, 2018 at 3:10 PM, Andres Freund  wrote:
>> I think the real question is whether the scenario is common enough to 
>> worry about.  In practice, you'd have to be extremely unlucky to be 
>> doing many bulk loads at the same time that all happened to hash to 
>> the same bucket.
>
> With a bunch of parallel bulkloads into partitioned tables that really 
> doesn't seem that unlikely?

It increases the likelihood of collisions, but probably decreases the number of 
cases where the contention gets really bad.

For example, suppose each table has 100 partitions and you are bulk-loading 10 
of them at a time.  It's virtually certain that you will have some collisions, 
but the amount of contention within each bucket will remain fairly low because 
each backend spends only 1% of its time in the bucket corresponding to any 
given partition.

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

Hello!
I want to try to test this patch on 302(704 ht) core machine.

Patching on master (commit 81256cd05f0745353c6572362155b57250a0d2a0) is ok but
got some error while compiling :

gistvacuum.c: In function ‘gistvacuumcleanup’:
gistvacuum.c:92:3: error: too many arguments to function 
‘LockRelationForExtension’
   LockRelationForExtension(rel, ExclusiveLock);
   ^
In file included from gistvacuum.c:21:0:
../../../../src/include/storage/extension_lock.h:30:13: note: declared here
 extern void LockRelationForExtension(Relation relation);
 ^
gistvacuum.c:95:3: error: too many arguments to function 
‘UnlockRelationForExtension’
   UnlockRelationForExtension(rel, ExclusiveLock);
   ^
In file included from gistvacuum.c:21:0:
../../../../src/include/storage/extension_lock.h:31:13: note: declared here
 extern void UnlockRelationForExtension(Relation relation);


--
Alex Ignatov 
Postgres Professional: http://www.postgrespro.com 
The Russian Postgres Company

RE: [HACKERS] Moving relation extension locks out of heavyweight lock manager

[Alex Ignatov]

--
Alex Ignatov 
Postgres Professional: http://www.postgrespro.com 
The Russian Postgres Company

-Original Message-
From: Alex Ignatov  
Sent: Monday, May 21, 2018 6:00 PM
To: 'Robert Haas' ; 'Andres Freund' 
Cc: 'Masahiko Sawada' ; 'Michael Paquier' 
; 'Mithun Cy' ; 'Tom Lane' 
; 'Thomas Munro' ; 'Amit 
Kapila' ; 'PostgreSQL-development' 

Subject: RE: [HACKERS] Moving relation extension locks out of heavyweight lock 
manager




-Original Message-
From: Robert Haas 
Sent: Thursday, April 26, 2018 10:25 PM
To: Andres Freund 
Cc: Masahiko Sawada ; Michael Paquier 
; Mithun Cy ; Tom Lane 
; Thomas Munro ; Amit Kapila 
; PostgreSQL-development 
Subject: Re: [HACKERS] Moving relation extension locks out of heavyweight lock 
manager

On Thu, Apr 26, 2018 at 3:10 PM, Andres Freund  wrote:
>> I think the real question is whether the scenario is common enough to 
>> worry about.  In practice, you'd have to be extremely unlucky to be 
>> doing many bulk loads at the same time that all happened to hash to 
>> the same bucket.
>
> With a bunch of parallel bulkloads into partitioned tables that really 
> doesn't seem that unlikely?

It increases the likelihood of collisions, but probably decreases the number of 
cases where the contention gets really bad.

For example, suppose each table has 100 partitions and you are bulk-loading 10 
of them at a time.  It's virtually certain that you will have some collisions, 
but the amount of contention within each bucket will remain fairly low because 
each backend spends only 1% of its time in the bucket corresponding to any 
given partition.

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

Hello!
I want to try to test this patch on 302(704 ht) core machine.

Patching on master (commit 81256cd05f0745353c6572362155b57250a0d2a0) is ok but 
got some error while compiling :

gistvacuum.c: In function ‘gistvacuumcleanup’:
gistvacuum.c:92:3: error: too many arguments to function 
‘LockRelationForExtension’
   LockRelationForExtension(rel, ExclusiveLock);
   ^
In file included from gistvacuum.c:21:0:
../../../../src/include/storage/extension_lock.h:30:13: note: declared here  
extern void LockRelationForExtension(Relation relation);
 ^
gistvacuum.c:95:3: error: too many arguments to function 
‘UnlockRelationForExtension’
   UnlockRelationForExtension(rel, ExclusiveLock);
   ^
In file included from gistvacuum.c:21:0:
../../../../src/include/storage/extension_lock.h:31:13: note: declared here  
extern void UnlockRelationForExtension(Relation relation);



Sorry, forgot to mention that patch version is extension-lock-v12.patch

--
Alex Ignatov
Postgres Professional: http://www.postgrespro.com The Russian Postgres Company

Re: Allowing printf("%m") only where it actually works

[Tom Lane]

Thomas Munro  writes:
> On Mon, May 21, 2018 at 4:36 PM, Tom Lane  wrote:
>> I am wondering whether the elog/ereport macros can locally define some
>> version of "errno" that would cause a compile failure if it's referenced
>> within the macro args.  But I'm too tired to work it out in detail.

> Here's an experimental way to do that, if you don't mind depending on
> gory details of libc implementations (ie knowledge of what it expands
> too).  Not sure how to avoid that since it's a macro on all modern
> systems, and we don't have a way to temporarily redefine a macro.  If
> you enable it for just ereport(), it compiles cleanly after 81256cd
> (but fails on earlier commits).  If you enable it for elog() too then
> it finds problems with exec.c.

Hmm ... that's pretty duff code in exec.c, isn't it.  Aside from the
question of errno unsafety, it's using elog where it really ought to be
using ereport, it's not taking any thought for the reported SQLSTATE,
etc.  I'm hesitant to mess with it mere hours before the beta wrap,
but we really oughta improve that.

I noticed another can of worms here, too: on Windows, doesn't use of
GetLastError() in elog/ereport have exactly the same hazard as errno?
Or is there some reason to think it can't change value during errstart()?

regards, tom lane

Re: Add --include-table-data-where option to pg_dump, to export only a subset of table data

[Stephen Frost]

Greetings,

* Carter Thaxton (carter.thax...@gmail.com) wrote:
> Many times I've wanted to export a subset of a database, using some sort of
> row filter condition on some of the large tables.  E.g. copying a
> production database to a staging environment, but with some time series
> data only from the past month.
> 
> We have the existing options:
>   --include-table=table(and its -t synonym)
>   --exclude-table=table
>   --exclude-table-data=table
> 
> I propose a new option:
>   --include-table-data-where=table:filter_clause
> 
> One would use this option as follows:
> 
>   pg_dump --include-table-data-where=largetable:"created_at >=
> '2018-05-01'" database_name
> 
> The filter_clause is used as the contents of a WHERE clause when querying
> the data to generate the COPY statement produced by pg_dump.

I've wanted something similar to this in the past as well, and, as
you've seen, we have some support for this kind of thing in pg_dump
already and what you're doing is exposing that.

> I've prepared a proposed patch for this, which is attached.  The code
> changes are rather straightforward.  I did have to add the ability to carry
> around an extra pointer-sized object to the simple_list implementation, in
> order to allow the filter clause to be associated to the matching oids of
> the table pattern.  It seemed the best way to augment the existing
> simple_list implementation, but change as little as possible elsewhere in
> the codebase.  (Note that SimpleOidList is actually only used by pg_dump).
> 
> Feel free to review and propose any amendments.

I've only taken a quick look but I don't see any regression tests, for
starters, and it's not clear if this can be passed multiple times for
one pg_dump run (I'd certainly hope that it could be...).

Also, if you haven't already, this should be registered on the
commitfest app, so we don't lose track of it.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: Postgres 11 release notes

[Tom Lane]

"Jonathan S. Katz"  writes:
> Per feedback from many asynchronous threads, attached is the proposed
> patch for the list of major features. I also expect a torrent of feedback. I
> will have a corresponding press release for Beta 1 in the very near future.
> This being my first doc patch proposal, I tried to follow the formatting I
> saw in the existing SGML. Please let me know if I can make improvements.

Pushed with some very minor editorialization.

regards, tom lane

PostgreSQL Buildfarm Client Release 8

[Andrew Dunstan]

I have just release version 8 of the PostgreSQL Buildfarm client

It can be downloaded from 
 
or 


This release contains a number of small changes to make using the 
--from-source feature a bit nicer. It also contains a good deal of code 
cleanup to be perlcritic clean, with some exceptions, down to severity 
level 3, and also to remove some out of date code that referred to 
Postgresql branches we no longer support.


It also contains the following features:

 * --tests and --schedule now apply to the check step as well as the
   installcheck step
 * a new --delay-check switch delays the check step until after
   install. This helps work around a bug or lack of capacity w.r.t.
   LD_LIBRARY_PATH on Alpine Linux
 * if the environment value BFLIB exists it is added to the perl search
   path. That means it is possible to install the top level scripts
   separately from the support modules.

Enjoy


andrew

--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Postgres 11 release notes

[Jonathan S. Katz]

> On May 21, 2018, at 12:38 PM, Tom Lane  wrote:
> 
> "Jonathan S. Katz"  writes:
>> Per feedback from many asynchronous threads, attached is the proposed
>> patch for the list of major features. I also expect a torrent of feedback. I
>> will have a corresponding press release for Beta 1 in the very near future.
>> This being my first doc patch proposal, I tried to follow the formatting I
>> saw in the existing SGML. Please let me know if I can make improvements.
> 
> Pushed with some very minor editorialization.

Thanks! I do agree with your editorial, the main goal now is to give exposure 
to features for testing.

Jonathan

Re: Add --include-table-data-where option to pg_dump, to export only a subset of table data

[Thomas Munro]

On Tue, May 22, 2018 at 4:05 AM, Stephen Frost  wrote:
> * Carter Thaxton (carter.thax...@gmail.com) wrote:
>>   pg_dump --include-table-data-where=largetable:"created_at >=
>> '2018-05-01'" database_name
>
> I've wanted something similar to this in the past as well, and, as
> you've seen, we have some support for this kind of thing in pg_dump
> already and what you're doing is exposing that.

+1

>> I've prepared a proposed patch for this, which is attached.  The code
>> changes are rather straightforward.  I did have to add the ability to carry
>> around an extra pointer-sized object to the simple_list implementation, in
>> order to allow the filter clause to be associated to the matching oids of
>> the table pattern.  It seemed the best way to augment the existing
>> simple_list implementation, but change as little as possible elsewhere in
>> the codebase.  (Note that SimpleOidList is actually only used by pg_dump).
>>
>> Feel free to review and propose any amendments.
>
> I've only taken a quick look but I don't see any regression tests, for
> starters, and it's not clear if this can be passed multiple times for
> one pg_dump run (I'd certainly hope that it could be...).
>
> Also, if you haven't already, this should be registered on the
> commitfest app, so we don't lose track of it.

Thanks for doing that.  Unfortunately the patch seems to be corrupted
in some way, maybe ANSI control characters or something... perhaps you
set colour.ui = always in your git config, instead of auto?  You might
also consider using git format-patch so you can include a brief commit
message that explains the feature.

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

Re: [HACKERS] kqueue

[Thomas Munro]

On Mon, May 21, 2018 at 7:27 PM, Mateusz Guzik  wrote:
> I have benchmarked the change on a FreeBSD box and found an big
> performance win once the number of clients goes beyond the number of
> hardware threads on the target machine. For smaller number of clients
> the win was very modest.

Thanks for the report!  This is good news for the patch, if we can
explain a few mysteries.

> 3 variants were tested:
> - stock 10.3
> - stock 10.3 + pdeathsig
> - stock 10.3 + pdeathsig + kqueue

For the record, "pdeathsig" refers to another patch of mine[1] that is
not relevant to this test (it's a small change in the recovery loop,
important for replication but not even reached here).

> [a bunch of neat output from ministat]

So to summarise your results:

32 connections: ~445k -> ~450k = +1.2%
64 connections: ~416k -> ~544k = +30.7%
96 connections: ~331k -> ~508k = +53.6%

As you added more connections above your thread count, stock 10.3's
TPS number went down, but with the patch it went up.  So now we have
to explain why you see a huge performance boost but others reported a
modest gain or in some cases loss.  The main things that jump out:

1.  You used TCP sockets and ran pgbench on another machine, while
others used Unix domain sockets.
2.  You're running a newer/bleeding edge kernel.
3.  You used more CPUs than most reporters.

For the record, Mateusz and others discovered some fixable global lock
contention in the Unix domain socket layer that is now being hacked
on[2], though it's not clear if that'd affect the results reported
earlier or not.

[1] 
https://www.postgresql.org/message-id/CAEepm%3D0w9AAHAH73-tkZ8VS2Lg6JzY4ii3TG7t-R%2B_MWyUAk9g%40mail.gmail.com
[2] https://reviews.freebsd.org/D15430

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

Re: Add --include-table-data-where option to pg_dump, to export only a subset of table data

[Euler Taveira]

2018-05-20 20:48 GMT-03:00 Carter Thaxton :
> Many times I've wanted to export a subset of a database, using some sort of
> row filter condition on some of the large tables.  E.g. copying a production
> database to a staging environment, but with some time series data only from
> the past month.
>
How would you handle foreign keys? It seems easier to produce a dump
that won't restore.

> We have the existing options:
>   --include-table=table(and its -t synonym)
>   --exclude-table=table
>   --exclude-table-data=table
>
> I propose a new option:
>   --include-table-data-where=table:filter_clause
>
I remembered an old thread [1]. At that time pg_dump was not so
decoupled from the backend. We are far from being decoupled in a way
that someone can write his own pg_dump using only calls from a
library. I'm not sure pg_dump is the right place to add another ETL
parameter. We already have too much parameters that could break a
restore (flexibility is always welcome but too much is not so good).

> One would use this option as follows:
>
>   pg_dump --include-table-data-where=largetable:"created_at >= '2018-05-01'"
> database_name
>
How would you check that that expression is correct? Every parameter
could quote its value. It means that your parameter have to escape the
quote in '2018-05-01'. Another problem is that your spec does not show
us how you would handle tables like Foo.Bar or "foo:bar" (colon have
to be escaped)?

> The filter_clause is used as the contents of a WHERE clause when querying
> the data to generate the COPY statement produced by pg_dump.
>
You are forgetting about --inserts parameter. Could I use
--include-table-data-where and --inserts?


[1] https://www.postgresql.org/message-id/1212299813.17810.17.camel%40ubuntu


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: Postgres, fsync, and OSs (specifically linux)

[Andres Freund]

On 2018-05-19 18:12:52 +1200, Thomas Munro wrote:
> On Sat, May 19, 2018 at 4:51 PM, Thomas Munro
>  wrote:
> > Next, make check hangs in initdb on both of my pet OSes when md.c
> > raises an error (fseek fails) and we raise and error while raising and
> > error and deadlock against ourselves.  Backtrace here:
> > https://paste.debian.net/1025336/
> 
> Ah, I see now that something similar is happening on Linux too, so I
> guess you already knew this.

I didn't. I cleaned something up and only tested installcheck
after... Singleuser mode was broken.

Attached is a new version.

I've changed my previous attempt at using transient files to using File
type files, but unliked from the LRU so that they're kept open. Not sure
if that's perfect, but seems cleaner.

Greetings,

Andres Freund
>From 96435b05c9546b6da829043fb10b2a7309216bd2 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Mon, 21 May 2018 15:43:30 -0700
Subject: [PATCH v2 1/6] freespace: Don't constantly close files when reading
 buffer.

fsm_readbuf() used to always do an smgrexists() when reading a buffer
beyond the known file size. That currently implies closing the md.c
handle, loosing all the data cached therein.  Change this to only
check for file existance when not already known to be larger than 0
blocks.

Author: Andres Freund
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/storage/freespace/freespace.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/backend/storage/freespace/freespace.c b/src/backend/storage/freespace/freespace.c
index 65c4e74999f..d7569cec5ed 100644
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -556,7 +556,7 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend)
 	 * not on extension.)
 	 */
 	if (rel->rd_smgr->smgr_fsm_nblocks == InvalidBlockNumber ||
-		blkno >= rel->rd_smgr->smgr_fsm_nblocks)
+		rel->rd_smgr->smgr_fsm_nblocks == 0)
 	{
 		if (smgrexists(rel->rd_smgr, FSM_FORKNUM))
 			rel->rd_smgr->smgr_fsm_nblocks = smgrnblocks(rel->rd_smgr,
@@ -564,6 +564,9 @@ fsm_readbuf(Relation rel, FSMAddress addr, bool extend)
 		else
 			rel->rd_smgr->smgr_fsm_nblocks = 0;
 	}
+	else if (blkno >= rel->rd_smgr->smgr_fsm_nblocks)
+		rel->rd_smgr->smgr_fsm_nblocks = smgrnblocks(rel->rd_smgr,
+	 FSM_FORKNUM);
 
 	/* Handle requests beyond EOF */
 	if (blkno >= rel->rd_smgr->smgr_fsm_nblocks)
-- 
2.17.0.rc1.dirty

>From aa533828e6164731006dab92665fa92b7b058d6f Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Mon, 21 May 2018 15:43:30 -0700
Subject: [PATCH v2 2/6] Add functions to send/receive data & FD over a unix
 domain socket.

This'll be used by a followup patch changing how the fsync request
queue works, to make it safe on linux.

TODO: This probably should live elsewhere.

Author: Andres Freund
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/storage/file/fd.c | 102 ++
 src/include/storage/fd.h  |   4 ++
 2 files changed, 106 insertions(+)

diff --git a/src/backend/storage/file/fd.c b/src/backend/storage/file/fd.c
index 441f18dcf56..65e46483a44 100644
--- a/src/backend/storage/file/fd.c
+++ b/src/backend/storage/file/fd.c
@@ -3572,3 +3572,105 @@ MakePGDirectory(const char *directoryName)
 {
 	return mkdir(directoryName, pg_dir_create_mode);
 }
+
+/*
+ * Send data over a unix domain socket, optionally (when fd != -1) including a
+ * file descriptor.
+ */
+ssize_t
+pg_uds_send_with_fd(int sock, void *buf, ssize_t buflen, int fd)
+{
+	ssize_t size;
+	struct msghdr   msg = {0};
+	struct ioveciov;
+	/* cmsg header, union for correct alignment */
+	union
+	{
+		struct cmsghdr  cmsghdr;
+		charcontrol[CMSG_SPACE(sizeof (int))];
+	} cmsgu;
+	struct cmsghdr  *cmsg;
+
+	iov.iov_base = buf;
+	iov.iov_len = buflen;
+
+	msg.msg_name = NULL;
+	msg.msg_namelen = 0;
+	msg.msg_iov = &iov;
+	msg.msg_iovlen = 1;
+
+	if (fd >= 0)
+	{
+		msg.msg_control = cmsgu.control;
+		msg.msg_controllen = sizeof(cmsgu.control);
+
+		cmsg = CMSG_FIRSTHDR(&msg);
+		cmsg->cmsg_len = CMSG_LEN(sizeof (int));
+		cmsg->cmsg_level = SOL_SOCKET;
+		cmsg->cmsg_type = SCM_RIGHTS;
+
+		*((int *) CMSG_DATA(cmsg)) = fd;
+	}
+
+	size = sendmsg(sock, &msg, 0);
+
+	/* errors are returned directly */
+	return size;
+}
+
+/*
+ * Receive data from a unix domain socket. If a file is sent over the socket,
+ * store it in *fd.
+ */
+ssize_t
+pg_uds_recv_with_fd(int sock, void *buf, ssize_t bufsize, int *fd)
+{
+	ssize_t size;
+	struct msghdr   msg;
+	struct ioveciov;
+	/* cmsg header, union for correct alignment */
+	union
+	{
+		struct cmsghdr  cmsghdr;
+		charcontrol[CMSG_SPACE(sizeof (int))];
+	} cmsgu;
+	struct cmsghdr  *cmsg;
+
+	Assert(fd != NULL);
+
+	iov.iov_base = buf;
+	iov.iov_len = bufsize;
+
+	msg.msg_name = NULL;
+	msg.msg_namelen = 0;
+	msg.msg_iov = &iov;
+	msg.msg_iovlen = 1;
+	msg.msg_control = cmsgu.control;
+	msg.msg_controllen = sizeof(cmsgu.con

Re: Problem while updating a foreign table pointing to a partitioned table on foreign server

[Kyotaro HORIGUCHI]

At Fri, 18 May 2018 15:31:07 -0400, Robert Haas  wrote 
in 
> On Fri, May 18, 2018 at 4:29 AM, Kyotaro HORIGUCHI
>  wrote:
> > I have reached to the same thought.
> >
> > The point here is that it is a base relation, which is not
> > assumed to have additional columns not in its definition,
> > including nonsystem junk columns. I'm not sure but it seems not
> > that simple to give base relations an ability to have junk
> > columns.
> 
> Do you know where that assumption is embedded specifically?

Taking the question literally, I see that add_vars_to_targetlist
accepts neither nonsystem (including whole row vars) junk columns
nor nonjunk columns that is not defined in the base relation. The
first line of the following code is that.

> Assert(attno >= rel->min_attr && attno <= rel->max_attr);
> attno -= rel->min_attr;
> if (rel->attr_needed[attno] == NULL)

In the last line attr_needed is of an array of (max_attr -
min_attr) elements, which is allocated in get_relation_info. I
didn't go further so it might be easier than I'm thinking but
anyway core-side modification (seems to me) is required at any
rate.

> If you're correct, then the FDW API is and always has been broken by
> design for any remote data source that uses a row identifier other
> than CTID, unless every foreign table definition always includes the
> row identifier as an explicit column. 

I actually see that. Oracle-FDW needs to compose row
identification by specifying "key" column option in relation
definition and the key columns are added as resjunk column. This
is the third (or, forth?) option of my comment upthread that was
said as "not only bothersome".

https://github.com/laurenz/oracle_fdw

| Column options (from PostgreSQL 9.2 on)
| key (optional, defaults to "false")
| 
| If set to yes/on/true, the corresponding column on the foreign
| Oracle table is considered a primary key column.  For UPDATE and
| DELETE to work, you must set this option on all columns that
| belong to the table's primary key.

>  I might be wrong here, but I'm
> pretty sure Tom wouldn't have committed this API in the first place
> with such a glaring hole in the design.

I see the API is still not broken in a sense, the ctid of
postgres_fdw is necessarily that of remote table. If we have a
reasonable mapping between remote tableoid:ctid and local ctid,
it works as expected. But such mapping seems to be rather
difficult to create since I don't find a generic way wihtout
needing auxiliary information, and at least there's no guarantee
that ctid has enough space for rows from multiple tables.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager

[Masahiko Sawada]

On Tue, May 22, 2018 at 12:05 AM, Alex Ignatov  wrote:
>
>
> --
> Alex Ignatov
> Postgres Professional: http://www.postgrespro.com
> The Russian Postgres Company
>
> -Original Message-
> From: Alex Ignatov 
> Sent: Monday, May 21, 2018 6:00 PM
> To: 'Robert Haas' ; 'Andres Freund' 
> 
> Cc: 'Masahiko Sawada' ; 'Michael Paquier' 
> ; 'Mithun Cy' ; 'Tom Lane' 
> ; 'Thomas Munro' ; 'Amit 
> Kapila' ; 'PostgreSQL-development' 
> 
> Subject: RE: [HACKERS] Moving relation extension locks out of heavyweight 
> lock manager
>
>
>
>
> -Original Message-
> From: Robert Haas 
> Sent: Thursday, April 26, 2018 10:25 PM
> To: Andres Freund 
> Cc: Masahiko Sawada ; Michael Paquier 
> ; Mithun Cy ; Tom Lane 
> ; Thomas Munro ; Amit 
> Kapila ; PostgreSQL-development 
> 
> Subject: Re: [HACKERS] Moving relation extension locks out of heavyweight 
> lock manager
>
> On Thu, Apr 26, 2018 at 3:10 PM, Andres Freund  wrote:
>>> I think the real question is whether the scenario is common enough to
>>> worry about.  In practice, you'd have to be extremely unlucky to be
>>> doing many bulk loads at the same time that all happened to hash to
>>> the same bucket.
>>
>> With a bunch of parallel bulkloads into partitioned tables that really
>> doesn't seem that unlikely?
>
> It increases the likelihood of collisions, but probably decreases the number 
> of cases where the contention gets really bad.
>
> For example, suppose each table has 100 partitions and you are bulk-loading 
> 10 of them at a time.  It's virtually certain that you will have some 
> collisions, but the amount of contention within each bucket will remain 
> fairly low because each backend spends only 1% of its time in the bucket 
> corresponding to any given partition.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
>
> Hello!
> I want to try to test this patch on 302(704 ht) core machine.
>
> Patching on master (commit 81256cd05f0745353c6572362155b57250a0d2a0) is ok 
> but got some error while compiling :

Thank you for reporting.
Attached an rebased patch with current HEAD.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


extension-lock-v13.patch
Description: Binary data

Time to put context diffs in the grave

[Andrew Dunstan]

We haven't insisted on context diffs in years now, and one of my 
interlocutors has just turned handsprings trying to follow the advice at 
 to produce his first 
patch.



Unless someone objects really violently I'm going to rip all that stuff 
out and let sanity prevail.



cheers


andrew

--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [doc fix] Add operation of freeing output SQLDA

[Kyotaro HORIGUCHI]

Hello.

At Fri, 18 May 2018 06:03:59 +, "Kato, Sho"  wrote 
in <25C1C6B2E7BE044889E4FE8643A58BA963A42097@G01JPEXMBKW03>
> Hello
> 
> I think it is better to add freeing operation of output SQLDA to the current 
> PostgreSQL documentation.
> As far as I can see src/interfaces/ecpg/ecpglib/execute.c,
> if a previously existing sqlda is set to output SQLDA, 
> then a previously existing sqlda is freed.
> But, the new output SQLDA's memory space remain.

I didn't look it closer but generally speaking output sqlda
should be freed automatically (or by ecpg API) since users don't
know it in detail. It is a linked list so just freeing the first
one is not sufficient(*1). On the other hand ecpg library cannot
free input sqlda since it doesn't know how it was provided. Thus
I'm on the documentation side. I'm not sure of the reason for
freeing output sqlda explicitly in the test code, maybe it is to
avoid valgrind complaint or such like..

I think if output sqlda is not freed and finally orphaned after
correct API usage, it should be fixed. I suppose that EXEC SQL
DEALLOCATE DESCRIPTOR is responsible..

> ecpg regression test also free output SQLDA's memory space.
> The attached patch fixes the documentation.


*1: The test code knows the shape exactly so it can properly free
them in proper way.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

RE: Postgres 11 release notes

[Ideriha, Takeshi]

>-Original Message-
>From: Bruce Momjian [mailto:br...@momjian.us]
>I have committed the first draft of the Postgres 11 release notes.  I will add 
>more
>markup soon.  You can view the most current version here:

Hi, there is a small typo.

I think "These function" should be "These functions".
(At the next sentece "these functions" is used.) 

Regards,
Ideriha, Takeshi


release_note_typo.patch
Description: release_note_typo.patch

Re: Time to put context diffs in the grave

[Andres Freund]

Hi,

On 2018-05-21 21:51:11 -0400, Andrew Dunstan wrote:
> We haven't insisted on context diffs in years now, and one of my
> interlocutors has just turned handsprings trying to follow the advice at
>  to produce his first
> patch.
> 
> 
> Unless someone objects really violently I'm going to rip all that stuff out
> and let sanity prevail.

Yes. Please.

Greetings,

Andres Freund

Re: Time to put context diffs in the grave

[David Fetter]

On Mon, May 21, 2018 at 09:51:11PM -0400, Andrew Dunstan wrote:
> 
> We haven't insisted on context diffs in years now, and one of my
> interlocutors has just turned handsprings trying to follow the advice at
>  to produce his first
> patch.
> 
> Unless someone objects really violently I'm going to rip all that stuff out
> and let sanity prevail.

+10 for sanity!

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

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

[GSoC] github repo and initial work

[Charles Cui]

Hi mentors and hackers,

   I have set up a github repo for the pg thrift plugin work, and here is
the address (https://github.com/charles-cui/pg_thrift). And I have first
version of binary protocol implemented, although it is still a very early
stage. The current interface is to return value for simple data
structure(byte, string, int32, int16, int64, bool) and return raw bytes for
complex data structure(list, map, set, struct). As suggested by Aleksander,
I can add json interfaces to use this plugin easily. Let me know if you
guys have any comments on this.

Thanks, Charles.

[PATCH] (Windows) psql echoes password when reading from pipe

[Matthew Stickney]

This is my first time submitting a patch here; apologies in advance if I 
flub the process.


On windows, if you pipe data to psql, the password prompt correctly 
reads from and writes to the console, but the password text is echoed to 
the console. This is because echoing is disabled on the handle for 
stdin, but as part of a pipeline stdin doesn't refer to the console. 
I've attached a patch that gets a handle to the console's input buffer 
by opening CONIN$ instead, which corrects the problem.


I think the change is straightforward enough to apply directly, but 
there's another concern that might bear discussion: SetConsoleMode can 
fail, and when it does prompt input will be echoed (i.e. it's 
fail-open). Is it worth check for and reporting for an error there? 
Given that this is meant to be used interactively, I think the risk of 
someone not noticing the echo is low.


-Matt Stickney

From 27a0f9b2036680564ef6dfa54f3961af221cbc50 Mon Sep 17 00:00:00 2001
From: Matthew Stickney 
Date: Mon, 21 May 2018 23:24:34 -0400
Subject: [PATCH] Disable input echo on the console, not stdin.

When data is piped to psql, a handle to stdin will no longer be a handle
to the console; SetConsoleMode will fail, and prompt input will be echoed
to the screen.

Instead, use CreateFile to get a handle to the console's input buffer by
opening CONIN$, and set the console mode there. Prompt input is already
being read from CONIN$, so now prompt-related I/O modifications alter the
same buffer.
the same buffer.
consistently.
---
 src/port/sprompt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/port/sprompt.c b/src/port/sprompt.c
index 70dfa69d7b..f57b49bd3a 100644
--- a/src/port/sprompt.c
+++ b/src/port/sprompt.c
@@ -112,7 +112,7 @@ simple_prompt(const char *prompt, char *destination, size_t 
destlen, bool echo)
if (!echo)
{
/* get a new handle to turn echo off */
-   t = GetStdHandle(STD_INPUT_HANDLE);
+   t = CreateFile("CONIN$", GENERIC_READ | GENERIC_WRITE, 
FILE_SHARE_READ | FILE_SHARE_WRITE, NULL, OPEN_EXISTING, 0, NULL);
 
/* save the old configuration first */
GetConsoleMode(t, &t_orig);
@@ -164,6 +164,7 @@ simple_prompt(const char *prompt, char *destination, size_t 
destlen, bool echo)
{
/* reset to the original console mode */
SetConsoleMode(t, t_orig);
+   CloseHandle(t);
fputs("\n", termout);
fflush(termout);
}
-- 
2.16.2.windows.1

Re: Add --include-table-data-where option to pg_dump, to export only a subset of table data

[Carter Thaxton]

Hello,

I've only taken a quick look but I don't see any regression tests, for
> starters, and it's not clear if this can be passed multiple times for
> one pg_dump run (I'd certainly hope that it could be...).
>

Yes, this will absolutely accept multiple options for one run, which is how
I'd imagine it would typically be used.

In fact, for each table_pattern:filter_clause you provide as an option, it
will apply a corresponding WHERE clause for *every* table that matches the
table_pattern.
So if you happened to use a wildcard in the table_pattern, you could
actually end up with multiple tables filtered by the same WHERE clause.

For example:
  pg_dump --include-table-data-where="table_*:created_at >= '2018-05-01'"
--include-table-data-where="other_table:id < 100"  db_name

This will filter every table named "table_*", e.g. ["table_0", "table_1",
"table_2", "table_associated"], each with "WHERE created_at >=
'2018-05-01'", and it will also filter "other_table" with "WHERE id < 100".

Not sure how useful the wildcard feature is, but it matches the behavior
of the other pg_dump options that specify tables, and came along for free
by reusing that implementation.


Also, if you haven't already, this should be registered on the
> commitfest app, so we don't lose track of it.
>

Done!
https://commitfest.postgresql.org/18/1644/

[PATCH] Clear up perlcritic 'missing return' warning

[Mike Blackwell]

This is the first in a series of patches to reduce the number of warnings
from perlcritic.   The current plan is to submit a separate patch for each
warning or small set of related warnings, to make reviewing more
straightforward.

This particular patch addresses the warning caused by falling off the end
of a subroutine rather than explicitly returning.  It also contains
additions to the perlcritic configuration file to ignore a couple of
warnings that seem acceptable, bringing it in line with the buildfarm's
configuration for those warnings.

Thanks to Andrew for assistance getting my environment set up.

Mike Blackwell
From d3ec3cbeb496ea9d2b285aaa32359cae528a983b Mon Sep 17 00:00:00 2001
From: Mike Blackwell 
Date: Tue, 15 May 2018 16:38:59 -0500
Subject: [PATCH] add returns to Perl files where missing


diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl
index 55f2167d7f..0f2628b557 100644
--- a/contrib/bloom/t/001_wal.pl
+++ b/contrib/bloom/t/001_wal.pl
@@ -36,6 +36,7 @@ SELECT * FROM tst WHERE i = 7 AND t = 'e';
 	my $standby_result = $node_standby->safe_psql("postgres", $queries);
 
 	is($master_result, $standby_result, "$test_name: query result matches");
+	return;
 }
 
 # Initialize master node
diff --git a/contrib/intarray/bench/create_test.pl b/contrib/intarray/bench/create_test.pl
index f3262df05b..d2c678bb53 100755
--- a/contrib/intarray/bench/create_test.pl
+++ b/contrib/intarray/bench/create_test.pl
@@ -83,4 +83,5 @@ sub copytable
 	while (<$fff>) { print; }
 	close $fff;
 	print "\\.\n";
+	return;
 }
diff --git a/src/backend/catalog/Catalog.pm b/src/backend/catalog/Catalog.pm
index 95bf619c91..ae5b499b6a 100644
--- a/src/backend/catalog/Catalog.pm
+++ b/src/backend/catalog/Catalog.pm
@@ -367,6 +367,7 @@ sub RenameTempFile
 	{
 		rename($temp_name, $final_name) || die "rename: $temp_name: $!";
 	}
+	return;
 }
 
 # Find a symbol defined in a particular header file and extract the value.
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index ebdc919414..9be51d28b0 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -649,6 +649,7 @@ sub gen_pg_attribute
 			}
 		}
 	}
+	return;
 }
 
 # Given $pgattr_schema (the pg_attribute schema for a catalog sufficient for
@@ -706,6 +707,7 @@ sub morph_row_for_pgattr
 	}
 
 	Catalog::AddDefaultValues($row, $pgattr_schema, 'pg_attribute');
+	return;
 }
 
 # Write an entry to postgres.bki.
@@ -744,6 +746,7 @@ sub print_bki_insert
 		push @bki_values, $bki_value;
 	}
 	printf $bki "insert %s( %s )\n", $oid, join(' ', @bki_values);
+	return;
 }
 
 # Given a row reference, modify it so that it becomes a valid entry for
@@ -786,6 +789,7 @@ sub morph_row_for_schemapg
 		# Only the fixed-size portions of the descriptors are ever used.
 		delete $row->{$attname} if $column->{is_varlen};
 	}
+	return;
 }
 
 # Perform OID lookups on an array of OID names.
diff --git a/src/backend/parser/check_keywords.pl b/src/backend/parser/check_keywords.pl
index ad41e134ac..718441c215 100644
--- a/src/backend/parser/check_keywords.pl
+++ b/src/backend/parser/check_keywords.pl
@@ -18,6 +18,7 @@ sub error
 {
 	print STDERR @_;
 	$errors = 1;
+	return;
 }
 
 $, = ' '; # set output field separator
diff --git a/src/backend/utils/mb/Unicode/convutils.pm b/src/backend/utils/mb/Unicode/convutils.pm
index 69ec099f29..103bd0264e 100644
--- a/src/backend/utils/mb/Unicode/convutils.pm
+++ b/src/backend/utils/mb/Unicode/convutils.pm
@@ -99,6 +99,7 @@ sub print_conversion_tables
 		$charset);
 	print_conversion_tables_direction($this_script, $csname, TO_UNICODE,
 		$charset);
+	return;
 }
 
 #
@@ -160,6 +161,7 @@ sub print_conversion_tables_direction
 	}
 
 	close($out);
+	return;
 }
 
 sub print_from_utf8_combined_map
@@ -194,6 +196,7 @@ sub print_from_utf8_combined_map
 	}
 	print $out "\t/* $last_comment */" if ($verbose && $last_comment ne "");
 	print $out "\n};\n";
+	return;
 }
 
 sub print_to_utf8_combined_map
@@ -230,6 +233,7 @@ sub print_to_utf8_combined_map
 	}
 	print $out "\t/* $last_comment */" if ($verbose && $last_comment ne "");
 	print $out "\n};\n";
+	return;
 }
 
 ###
@@ -625,6 +629,7 @@ sub print_radix_table
 	if ($off != $tblsize) { die "table size didn't match!"; }
 
 	print $out "};\n";
+	return;
 }
 
 ###
diff --git a/src/backend/utils/sort/gen_qsort_tuple.pl b/src/backend/utils/sort/gen_qsort_tuple.pl
index 6186d0a5ba..b6b2ffa7d0 100644
--- a/src/backend/utils/sort/gen_qsort_tuple.pl
+++ b/src/backend/utils/sort/gen_qsort_tuple.pl
@@ -130,6 +130,8 @@ swapfunc(SortTuple *a, SortTuple *b, size_t n)
 #define vecswap(a, b, n) if ((n) > 0) swapfunc(a, b, n)
 
 EOM
+
+	return;
 }
 
 sub emit_qsort_implementation
@@ -263,4 +265,6 @@ loop:
 	}
 }
 EOM
+
+	return;
 }
diff --git a/src/bin/pg_archivecleanup/t/010_pg_archivecleanup.pl b/src/bin/pg_archivecleanup/t/0