Test patch for partitionwise join with partitioned tables containing default partition

[Rajkumar Raghuwanshi]

Hi,

As of now partition_join.sql is not having test cases covering cases
where partition table have default partition, attaching a small test
case patch to cover those.

Here is a link of previous discussion :
https://www.postgresql.org/message-id/CAKcux6%3DLO-
XK9G0yLe634%2B0SP2UOn5ksVnmF-OntTBOEEaUGTg%40mail.gmail.com

As found by Thomas, The regression test currently fails with v4 patch
because a
redundant Result node has been removed from a query plan. here is the
updated
v5 patch fixing this.

Thanks & Regards,
Rajkumar Raghuwanshi
QMG, EnterpriseDB Corporation
diff --git a/src/test/regress/expected/partition_join.out b/src/test/regress/expected/partition_join.out
index b983f9c..8b3798e 100644
--- a/src/test/regress/expected/partition_join.out
+++ b/src/test/regress/expected/partition_join.out
@@ -1328,6 +1328,76 @@ SELECT avg(t1.a), avg(t2.b), avg(t3.a + t3.b), t1.c, t2.c, t3.c FROM pht1 t1, ph
  273. | 273. | 548. | 0005 | 0005 | A0005
 (6 rows)
 
+-- test default partition behavior for range
+ALTER TABLE prt1 DETACH PARTITION prt1_p3;
+ALTER TABLE prt1 ATTACH PARTITION prt1_p3 DEFAULT;
+ANALYZE prt1;
+ALTER TABLE prt2 DETACH PARTITION prt2_p3;
+ALTER TABLE prt2 ATTACH PARTITION prt2_p3 DEFAULT;
+ANALYZE prt2;
+EXPLAIN (COSTS OFF)
+SELECT t1.a, t1.c, t2.b, t2.c FROM prt1 t1, prt2 t2 WHERE t1.a = t2.b AND t1.b = 0 ORDER BY t1.a, t2.b;
+QUERY PLAN
+--
+ Sort
+   Sort Key: t1.a
+   ->  Append
+ ->  Hash Join
+   Hash Cond: (t2.b = t1.a)
+   ->  Seq Scan on prt2_p1 t2
+   ->  Hash
+ ->  Seq Scan on prt1_p1 t1
+   Filter: (b = 0)
+ ->  Hash Join
+   Hash Cond: (t2_1.b = t1_1.a)
+   ->  Seq Scan on prt2_p2 t2_1
+   ->  Hash
+ ->  Seq Scan on prt1_p2 t1_1
+   Filter: (b = 0)
+ ->  Hash Join
+   Hash Cond: (t2_2.b = t1_2.a)
+   ->  Seq Scan on prt2_p3 t2_2
+   ->  Hash
+ ->  Seq Scan on prt1_p3 t1_2
+   Filter: (b = 0)
+(21 rows)
+
+-- test default partition behavior for list
+ALTER TABLE plt1 DETACH PARTITION plt1_p3;
+ALTER TABLE plt1 ATTACH PARTITION plt1_p3 DEFAULT;
+ANALYZE plt1;
+ALTER TABLE plt2 DETACH PARTITION plt2_p3;
+ALTER TABLE plt2 ATTACH PARTITION plt2_p3 DEFAULT;
+ANALYZE plt2;
+EXPLAIN (COSTS OFF)
+SELECT avg(t1.a), avg(t2.b), t1.c, t2.c FROM plt1 t1 RIGHT JOIN plt2 t2 ON t1.c = t2.c WHERE t1.a % 25 = 0 GROUP BY t1.c, t2.c ORDER BY t1.c, t2.c;
+   QUERY PLAN   
+
+ Sort
+   Sort Key: t1.c
+   ->  HashAggregate
+ Group Key: t1.c, t2.c
+ ->  Append
+   ->  Hash Join
+ Hash Cond: (t2.c = t1.c)
+ ->  Seq Scan on plt2_p1 t2
+ ->  Hash
+   ->  Seq Scan on plt1_p1 t1
+ Filter: ((a % 25) = 0)
+   ->  Hash Join
+ Hash Cond: (t2_1.c = t1_1.c)
+ ->  Seq Scan on plt2_p2 t2_1
+ ->  Hash
+   ->  Seq Scan on plt1_p2 t1_1
+ Filter: ((a % 25) = 0)
+   ->  Hash Join
+ Hash Cond: (t2_2.c = t1_2.c)
+ ->  Seq Scan on plt2_p3 t2_2
+ ->  Hash
+   ->  Seq Scan on plt1_p3 t1_2
+ Filter: ((a % 25) = 0)
+(23 rows)
+
 --
 -- multiple levels of partitioning
 --
@@ -1857,3 +1927,30 @@ SELECT t1.a, t1.c, t2.b, t2.c FROM prt1_n t1 FULL JOIN prt1 t2 ON (t1.c = t2.c);
->  Seq Scan on prt1_n_p2 t1_1
 (10 rows)
 
+-- partitionwise join can not be applied if only one of joining table has
+-- default partition
+ALTER TABLE prt2 DETACH PARTITION prt2_p3;
+ALTER TABLE prt2 ATTACH PARTITION prt2_p3 FOR VALUES FROM (500) TO (600);
+ANALYZE prt2;
+EXPLAIN (COSTS OFF)
+SELECT t1.a, t1.c, t2.b, t2.c FROM prt1 t1, prt2 t2 WHERE t1.a = t2.b AND t1.b = 0 ORDER BY t1.a, t2.b;
+QUERY PLAN
+--
+ Sort
+   Sort Key: t1.a
+   ->  Hash Join
+ Hash Cond: (t2.b = t1.a)
+ ->  Append
+   ->  Seq Scan on prt2_p1 t2
+   ->  Seq Scan on prt2_p2 t2_1
+   ->  Seq Scan on prt2_p3 t2_2
+ ->  Hash
+   ->  Append
+ ->  Seq Scan on prt1_p1 t1
+   Filter: (b = 0)
+ ->  Seq Scan on prt1_p2 t1_1
+   Filter: (b = 0)
+ ->  Seq Scan on prt1_p3 t1_2
+   Filter: (b = 0)
+(16 rows)
+

Re: why partition pruning doesn't work?

[David Rowley]

On 6 June 2018 at 03:07, David Rowley  wrote:
> Please see the attached patch. I've only just finished with it and
> it's not fully done yet as there's still an XXX comment where I've not
> quite thought about Exprs with Vars from higher levels. These might
> always be converted to Params, so the code might be okay as is, but
> I've not checked this yet, hence the comment remains.

I looked at this again today and decided that the XXX comment could
just be removed. I also changed contains_only_safeparams into
contains_unsafeparams and reversed the condition. I then decided that
I didn't like the way we need to check which params are in the Expr
each time we call partkey_datum_from_expr. It seems better to prepare
this in advance when building the pruning steps. I started work on
that, but soon realised that I'd need to pass a List of Bitmapsets to
the executor. This is a problem as Bitmapset is not a Node type and
cannot be copied with COPY_NODE_FIELD(). Probably this could be
refactored to instead of passing 3 Lists in the PartitionPruneStepOp
we could invent a new node type that just has 3 fields and store a
single List.

Anyway, I didn't do that because I'm not sure what the fate of this
patch is going to be. I'd offer to change things around to add a new
Node type, but I don't really want to work on that now if this is v12
material.

I've attached a cleaned up version of the patch I posted yesterday.

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


run-time_pruning_for_exprs_v2.patch
Description: Binary data

Re: Why is fncollation in FunctionCallInfoData rather than fmgr_info?

[Tom Lane]

Andres Freund  writes:
> In my understanding FunctionCallInfoData is basically per-call data,
> whereas FmgrInfo is information about the function.  It makes some sense
> that ->context is in FunctionCallInfoData, after all it's used for
> per-row data like the trigger context.  But we don't really change the
> collation of function invocations per-call.  Thus I don't quite get why
> FunctionCallInfoData contains information about it rather than FmgrInfo.

[squint]  I would say that the call collation is an argument, not a
property of the function, and therefore is correctly located in
FunctionCallInfoData.

It's true that we often abuse fn_extra to hold data that's essentially
call-site-dependent, but I don't think that's a good reason to push
collation into FmgrInfo.

regards, tom lane

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Wed, Jun 06, 2018 at 09:58:34AM +0530, Amit Kapila wrote:
> On Wed, Jun 6, 2018 at 8:28 AM, Michael Paquier  wrote:
>> Ouch.  Including directly c.h as you do here is against project policy
>> code.  See recent commit a72f0365 for example.  pgwin32_open() is
>> visibly able to handle frontend code if I read this code correctly, so
>> could we just remove the "#ifndef FRONTEND" restriction?
> 
> I think we need to explore a bit, if we want to remove that, for example
> some of the frontend modules (like pg_receivewal, pg_recvlogical) call two
> argument open which would require change.

Yeah, sure.  I am not saying that this is straight-forward, but it may
be worth the switch in the long term.  What I am sure about is that the
proposed patch is simple, but that does not look like a correct
approach to me as other tools may benefit from it.

>> It could be
>> risky for existing callers of open() for tool maintainers, or on the
>> contrary people could welcome a wrapper of open() which is
>> concurrent-safe in their own tools.
> 
> I am not sure if we can safely assume that because using these functions
> would allow users to concurrently delete the files, but may be it is okay
> for all the FRONTEND modules.  One another alternative could be that we
> define open as pgwin32_open (for WIN32) wherever we need it.

Which is what basically happens on any *nix platform, are you foreseeing
anything bad here?  Windows has the characteristic in being particular
in everything, which is its main characteristic, so I may be missing
something of course.
--
Michael


signature.asc
Description: PGP signature

Re: Loaded footgun open_datasync on Windows

[Amit Kapila]

On Wed, Jun 6, 2018 at 8:28 AM, Michael Paquier  wrote:

> On Tue, Jun 05, 2018 at 04:15:00PM +0200, Laurenz Albe wrote:
>
> > --- a/src/bin/pg_test_fsync/pg_test_fsync.c
> > +++ b/src/bin/pg_test_fsync/pg_test_fsync.c
> > @@ -3,6 +3,8 @@
> >   *   tests all supported fsync() methods
> >   */
> >
> > +/* we have to include c.h first so that pgwin32_open is used on Windows
> */
> > +#include "c.h"
> >  #include "postgres_fe.h"
> >
> >  #include 
>
> Ouch.  Including directly c.h as you do here is against project policy
> code.  See recent commit a72f0365 for example.  pgwin32_open() is
> visibly able to handle frontend code if I read this code correctly, so
> could we just remove the "#ifndef FRONTEND" restriction?


I think we need to explore a bit, if we want to remove that, for example
some of the frontend modules (like pg_receivewal, pg_recvlogical) call two
argument open which would require change.



> It could be
> risky for existing callers of open() for tool maintainers, or on the
> contrary people could welcome a wrapper of open() which is
> concurrent-safe in their own tools.


I am not sure if we can safely assume that because using these functions
would allow users to concurrently delete the files, but may be it is okay
for all the FRONTEND modules.  One another alternative could be that we
define open as pgwin32_open (for WIN32) wherever we need it.

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

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

[Ashutosh Bapat]

On Wed, Jun 6, 2018 at 8:11 AM, Thomas Munro
 wrote:
> On Mon, Mar 5, 2018 at 8:13 PM, Rajkumar Raghuwanshi
>  wrote:
>> On Wed, Feb 7, 2018 at 2:00 PM, Ashutosh Bapat
>> Changed partition-wise statement to partitionwise.
>> Attached re-based patch.
>>
>>> The patch looks good to me. I don't think we can reduce it further.
>>> But we need some tests to test PWJ with default partitions. Marking
>>> this as ready for committer.
>
> Hi Rajkumar,
>
> partition_join ... FAILED
>

That made my heart stop for fraction of a second. I thought, something
happened which caused partition_join test fail in master. But then I
realised you are talking about Rajkumar's patch and test in that
patch. I think it's better to start a separate thread discussing his
patch, before I loose my heart ;)

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

Re: I'd like to discuss scaleout at PGCon

[Ashutosh Bapat]

On Tue, Jun 5, 2018 at 10:04 PM, MauMau  wrote:
> From: Ashutosh Bapat
>> In order to normalize parse trees, we need to at
>> least replace various OIDs in parse-tree with something that the
>> foreign server will understand correctly like table name on the
>> foreign table pointed to by local foreign table OR (schema
> qualified)
>> function names  and so on.
>
> Yes, that's the drawback of each node in the cluster having different
> OIDs for the same object.  That applies to XL, too.

Keeping OIDs same across the nodes would require extra communication
between nodes to keep track of next OID, dropped OIDs etc. We need to
weigh the time spent in that communication and the time saved during
parsing.

>  In XL, the data
> node returns the data type names of the columns in the result set to
> the coordinator.  Then the coordinator seemed to parse each data type
> name with base_yyparse() to convert the name to its OID on the
> coordinator.  That's why base_yyparse() appeared at the top in the
> perf profile.

I do not understand, why do we need base_yyparse() to parse type name.
We already have functions specifically for parsing object names.

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

Re: buildfarm vs code

[Thomas Munro]

On Wed, Jun 6, 2018 at 4:09 AM, Andrew Dunstan
 wrote:
> At my talk at pgcon last Friday [1] I presented some ideas for how people
> could run a full buildfarm run against their code, including a 4 line recipe
> using some Docker recipes I had created. Thomas Munro suggested it would be
> even nicer if you could just point something like Appveypr at the code and
> have it do the buildfarm run. That intrigued me a bit, so I created a recipe
> for that. Along the way there are a few adjustments to how the buildfarm
> client runs, which is why the recipe [2] runs with a git clone rather than a
> published release. Nevertheless it does seem to work [3]

Very nice!  Thank you for working on this.  So it seems like we're
pretty close to being able to use the standard buildfarm software to
manage automatic Windows testing of PostgreSQL from any personal git
repo, and also for cfbot's testing of CF submissions.

Would this dump the build log to stderr if it fails, so you could see
it on appveyor?  I'd also like to see compiler warnings (and perhaps
enable the equivalent of -Werror).  How should we access that -- is
there a just-write-everything-to-stdout mode?  It suppose we could
just add some scripting to find and dump all log files.

The quick-and-dirty configuration I've been using for cfbot also runs
a bit of perl on failure to locate and dump out any regression.diffs
files, like this:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.1127
https://github.com/macdice/cfbot/blob/master/appveyor/appveyor.yml#L22
https://github.com/macdice/cfbot/blob/master/appveyor/dumpregr.pl

I wonder if it's possible to teach it to locate and backtrace any core
files, as cfbot does on Linux:

https://github.com/macdice/cfbot/blob/master/travis/.travis.yml#L28

> The second change the recipe makes is to disable the tablespace tests. On
> Windows, when running as the Administrator, the application gives up
> privilege to such an extent that the tablespace tests fail.

Ohh.  I was confused about why Windows was giving us fewer privileges
when we had a more privileged account, but in fact *we* did that.
Where does the privilege-giving-up happen?

> The answer is
> usually to run as a non-Admin user, and this is what I do for animals like
> jacana and bowerbird. However, setting that up so that it hooks up nicely to
> the appveyor console is ugly and fragile. So I'd like either to solve this
> issue (perhaps be more discriminating about what privileges we give up) or
> provide a builtin way to skip these tests. In the latter case, maybe a
> skip-tests setting for pg_regress would work well. I can imagine other uses
> for it ("I know I'm going to break this test but I want to run all the
> others.")

I really want this to actually work rather than being skipped, because
I'm working on stuff related to tablespaces and I want CI for that on
Windows.

If we fixed those things it seems we'd almost have an .appveyor.yml
good enough to start talking about putting it in our source tree, to
provide a solid default for anyone working on PostgreSQL.

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

Re: I'd like to discuss scaleout at PGCon

[Michael Paquier]

On Wed, Jun 06, 2018 at 01:14:04AM +0900, MauMau wrote:
> I don't think an immediate server like the coordinators in XL is
> necessary.  That extra hop can be eliminated by putting both the
> coordinator and the data node roles in the same server process.  That
> is, the node to which an application connects communicates with other
> nodes only when it does not necessary data.

Yes, I agree with that.  This was actually a concern I had over the
original XC design after a couple of years working on it.  The less
nodes, the easier the HA, even if applying any PITR logic in N nodes
instead of N*2 nodes with 2PC checks and cleanups is far from trivial
either..  It happens that the code resulting in splitting coordinator
and datanode was simpler to maintain than merging both, at the cost of
operation maintenance and complexity in running the thing.

> Furthermore, an extra hop and double parsing/planning could matter for
> analytic queries, too.  For example, SAP HANA boasts of scanning 1
> billion rows in one second.  In HANA's scaleout architecture, an
> application can connect to any worker node and the node communicates
> with other nodes only when necessary (there's one special node called
> "master", but it manages the catalog and transactions; it's not an
> extra hop like the coordinator in XL).  Vertica is an MPP analytics
> database, but it doesn't have a node like the coordinator, either.  To
> achieve maximum performance for real-time queries, the scaleout
> architecture should avoid an extra hop when possible.

Greenplum's orca planner (and Citus?) have such facilities if I recall
correctly, just mentioning that pushing down directly to remote nodes
compiled plans ready for execution exists here and there (that's not the
case of XC/XL).  For queries whose planning time is way shorter than its
actual execution, like analytical work that would not matter much.  But
not for OLTP and short transaction workloads.

>> Using a central coordinator also allows multi-node transaction
>> control, global deadlock detection etc..
> 
> VoltDB does not have an always-pass hop like the coordinator in XL.

Greenplum uses also a single-coordinator, multi-datanode instance.  That
looks similar, right?

> Our proprietary RDBMS named Symfoware, which is not based on
> PostgreSQL, also doesn't have an extra hop, and can handle distributed
> transactions and deadlock detection/resolution without any special
> node like GTM.

Interesting to know that.  This is an area with difficult problems.  At
the closer to merge with Postgres head, the more fun (?) you get into
trying to support new SQL features, and sometimes you finish with hard
ERRORs or extra GUC switches to prevent any kind of inconsistent
operations.
--
Michael


signature.asc
Description: PGP signature

Re: install doesn't work on HEAD

[Amit Kapila]

On Tue, Jun 5, 2018 at 7:49 PM, Ashutosh Sharma 
wrote:

> Hi,
>
> I am not able to reproduce this issue on HEAD. Seems like the
> following git-commit fixes it,
>
> commit 01deec5f8ae64b5120cc8c93d54fe0e19e477b02
> Author: Andrew Dunstan 
> Date:   Mon May 28 16:44:13 2018 -0400
>
> Return a value from Install.pm's lcopy function
>
>
Yes, you are right, it is fixed after this.  I missed to refresh my
branch.  Thanks for pointing out.

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

Re: Loaded footgun open_datasync on Windows

[Michael Paquier]

On Tue, Jun 05, 2018 at 04:15:00PM +0200, Laurenz Albe wrote:
> The attached patch makes pg_test_fsync use pgwin32_open on Windows, which is 
> what
> we use elsewhere.

Well, pg_upgrade may benefit from that as one example, as any other
tools.

> That should fix the problem.
> Ist there a better way to do this?  The problem is that "c.h" is only included
> at the very end of "postgres-fe.h", which makes front-end code use "open"
> rather than "pgwin32_open" on Windows.

And port.h is included at the end of c.h.

> Having read it again, I think that the documentation is fine as it is:
> After all, this is just advice what you can do if you are running on unsafe 
> hardware,
> which doesn't flush to disk like it should.

People tend to ignore this part from the docs.  Well I won't fight hard
on that if folks don't want to change that...

> --- a/src/bin/pg_test_fsync/pg_test_fsync.c
> +++ b/src/bin/pg_test_fsync/pg_test_fsync.c
> @@ -3,6 +3,8 @@
>   *   tests all supported fsync() methods
>   */
>  
> +/* we have to include c.h first so that pgwin32_open is used on Windows */
> +#include "c.h"
>  #include "postgres_fe.h"
>  
>  #include 

Ouch.  Including directly c.h as you do here is against project policy
code.  See recent commit a72f0365 for example.  pgwin32_open() is
visibly able to handle frontend code if I read this code correctly, so
could we just remove the "#ifndef FRONTEND" restriction?  It could be
risky for existing callers of open() for tool maintainers, or on the
contrary people could welcome a wrapper of open() which is
concurrent-safe in their own tools.  I am not sure which position is
better here, but thinking that all in-core frontend code could benefit
from a couple of simplifications that could be worth the shot.
--
Michael


signature.asc
Description: PGP signature

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

[Thomas Munro]

On Mon, Mar 5, 2018 at 8:13 PM, Rajkumar Raghuwanshi
 wrote:
> On Wed, Feb 7, 2018 at 2:00 PM, Ashutosh Bapat
> Changed partition-wise statement to partitionwise.
> Attached re-based patch.
>
>> The patch looks good to me. I don't think we can reduce it further.
>> But we need some tests to test PWJ with default partitions. Marking
>> this as ready for committer.

Hi Rajkumar,

partition_join ... FAILED

The regression test currently fails with your v4 patch because a
redundant Result node has been removed from a query plan.  That may be
due to commit 11cf92f6 or nearby commits.

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

Re: pg_replication_slot_advance to return NULL instead of 0/0 if slot not advanced

[Michael Paquier]

On Tue, Jun 05, 2018 at 01:00:30PM +0200, Petr Jelinek wrote:
> I didn't say anything about CreateDecodingContext though. I am talking
> about the fact that we then use the same variable as input to
> XLogReadRecord later in the logical slot code path. The whole point of
> having restart_lsn for logical slot is to have correct start point for
> XLogReadRecord so using the confirmed_flush there is wrong (and has been
> wrong since the original commit).

OK, I finally see the point you are coming at after a couple of hours
brainstorming about it, and indeed using confirmed_lsn is logically
incorrect so the current code is inconsistent with what
DecodingContextFindStartpoint() does, so we rely on restart_lsn to scan
for all the records and the decoder state is here to make sure that the
slot's confirmed_lsn is updated to a consistent position.  I have added
your feedback in the attached (indented and such), which results in some
simplifications with a couple of routines.

I am attaching as well the patch I sent yesterday.  0001 is candidate
for a back-patch, 0002 is for HEAD to fix the slot advance stuff.

There is another open item related to slot advancing here:
https://www.postgresql.org/message-id/2840048a-1184-417a-9da8-3299d207a1d7%40postgrespro.ru
And per my tests the patch is solving this item as well.  I have just
used the test mentioned in the thread which:
1) creates a slot
2) does some activity to generate a couple of WAL pages
3) advances the slot at page boundary
4) Moves again the slot.
This test crashes on HEAD at step 4, and not with the attached.

What do you think?
--
Michael
From 0cd02e359cbf5b74c8231f6619bc479a314213bc Mon Sep 17 00:00:00 2001
From: Michael Paquier 
Date: Fri, 1 Jun 2018 14:30:55 -0400
Subject: [PATCH 1/2] Fix and document lock handling for in-memory replication
 slot data

While debugging issues on HEAD for the new slot forwarding feature of
Postgres 11, some monitoring of the code surrounding in-memory slot data
has proved that the lock handling may cause inconsistent data to be read
by read-only callers of slot functions, particularly
pg_get_replication_slots() which fetches data for the system view
pg_replication_slots.

The code paths involved in those problems concern logical decoding
initialization (down to 9.4) and WAL reservation for slots (new as of
10).

A set of comments documenting all the lock handlings, particularly the
dependency with LW locks for slots and the in_use flag as well as the
internal mutex lock is added, based on a suggested by Simon Riggs.

Backpatch down to 9.4, where WAL decoding has been introduced.

Discussion: https://postgr.es/m/CANP8+jLyS=X-CAk59BJnsxKQfjwrmKicHQykyn52Qj-Q=9g...@mail.gmail.com
---
 src/backend/replication/logical/logical.c | 13 +
 src/backend/replication/slot.c|  4 
 src/include/replication/slot.h| 13 +
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/src/backend/replication/logical/logical.c b/src/backend/replication/logical/logical.c
index 1393591538..61588d626f 100644
--- a/src/backend/replication/logical/logical.c
+++ b/src/backend/replication/logical/logical.c
@@ -297,10 +297,12 @@ CreateInitDecodingContext(char *plugin,
 
 	xmin_horizon = GetOldestSafeDecodingTransactionId(!need_full_snapshot);
 
+	SpinLockAcquire(>mutex);
 	slot->effective_catalog_xmin = xmin_horizon;
 	slot->data.catalog_xmin = xmin_horizon;
 	if (need_full_snapshot)
 		slot->effective_xmin = xmin_horizon;
+	SpinLockRelease(>mutex);
 
 	ReplicationSlotsComputeRequiredXmin(true);
 
@@ -445,13 +447,14 @@ void
 DecodingContextFindStartpoint(LogicalDecodingContext *ctx)
 {
 	XLogRecPtr	startptr;
+	ReplicationSlot *slot = ctx->slot;
 
 	/* Initialize from where to start reading WAL. */
-	startptr = ctx->slot->data.restart_lsn;
+	startptr = slot->data.restart_lsn;
 
 	elog(DEBUG1, "searching for logical decoding starting point, starting at %X/%X",
-		 (uint32) (ctx->slot->data.restart_lsn >> 32),
-		 (uint32) ctx->slot->data.restart_lsn);
+		 (uint32) (slot->data.restart_lsn >> 32),
+		 (uint32) slot->data.restart_lsn);
 
 	/* Wait for a consistent starting point */
 	for (;;)
@@ -477,7 +480,9 @@ DecodingContextFindStartpoint(LogicalDecodingContext *ctx)
 		CHECK_FOR_INTERRUPTS();
 	}
 
-	ctx->slot->data.confirmed_flush = ctx->reader->EndRecPtr;
+	SpinLockAcquire(>mutex);
+	slot->data.confirmed_flush = ctx->reader->EndRecPtr;
+	SpinLockRelease(>mutex);
 }
 
 /*
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 056628fe8e..79d7a57d67 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -1016,7 +1016,9 @@ ReplicationSlotReserveWal(void)
 			XLogRecPtr	flushptr;
 
 			/* start at current insert position */
+			SpinLockAcquire(>mutex);
 			slot->data.restart_lsn = GetXLogInsertRecPtr();
+			SpinLockRelease(>mutex);
 
 			/* make sure we have enough information to start */
 			flushptr = LogStandbySnapshot();
@@ -1026,7 

Re: Code of Conduct plan

[Isaac Morland]

On 5 June 2018 at 17:34, Ozz Nixon  wrote:

> Sorry...
>
> > 1) CoC might result in developers leaving projects
>
>
> I know this on going regurgitation is going to cause my team to
> leave the project, right around 100 posts on this off topic topic it
> was bad enough when the original idea came up (2 years ago I think). It
> used to be exciting to sit back and review the day or weeks posts... not
> much anymore.
>

With all due respect, it is completely unreasonable to quit just because
there has been some discussion of the rules for co-existing within the
project. The intent of codes of conduct is usually supposed to be to make
it clear that bullying and harassment are not permitted, something that is
not always clear to everybody. That doesn't mean that any particular
position on them is required, only that discussion of them is definitely
*not* off topic. In any event, if you aren't interested in a thread, you
can easily mute it. Personally, I have about 95% of pgsql-hackers muted,
because I simply don't have time to be interested in every topic that is
discussed, and I suspect many subscribers are similar. If somebody is so
sensitive to even being aware of a discussion of the issue that they feel
they have to leave, then I would expect them to leave at some point anyway
due to becoming offended by some trivial matter that nobody else would even
notice.

GSOC 2018 Project Progress

[Kefan Yang]

Hello Hackers!

My name is Kefan Yang, and I am working on my Google Summer of Code 2018 
project. For the first evaluation, I should be able to hand in:
1.  the benchmark implementations of introsort, timsort, dual-pivot quicksort 
and radixsort.
2. Phase 1(random array) and 2(worst case) of the benchmark

I have a repo for this project here. My current progress:
1. sort.h implemented sorting routines including introsort, timsort, dual-pivot 
quicksort and radixsort. I used some macros to support multiple data type(int, 
char, and string).
2. test.c implemented some simple test cases to test functions in sort.h, which 
is able to check the correctness of sorting functions, count number of 
comparisons and evaluate performance on random input.
3. benchmark is still in process. Current it’s able generating unsorted, 
sorted, reversed, mostly sorted and mostly reversed test data(integers only). 

In general, I’ve kept up with the schedule. I should be able to make the 
benchmark work before the evaluation deadline. Please feel free to tell me if 
you have any suggestion

Re: libpq compression

[Thomas Munro]

On Wed, Jun 6, 2018 at 2:06 AM, Konstantin Knizhnik
 wrote:
> Thank you for review. Updated version of the patch fixing all reported
> problems is attached.

Small problem on Windows[1]:

  C:\projects\postgresql\src\include\common/zpq_stream.h(17): error
C2143: syntax error : missing ')' before '*'
[C:\projects\postgresql\libpq.vcxproj]
2395

You used ssize_t in zpq_stream.h, but Windows doesn't have that type.
We have our own typedef in win32_port.h.  Perhaps zpq_stream.c should
include postgres.h/postgres_fe.h (depending on FRONTEND) like the
other .c files in src/common, before it includes zpq_stream.h?
Instead of "c.h".

[1] https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.1106

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

Re: commitfest 2018-07

[Michael Paquier]

On Tue, Jun 05, 2018 at 11:20:57AM -0400, Peter Eisentraut wrote:
> On 6/5/18 09:12, Andres Freund wrote:
>> I'd rather create a new 2018-07, and just manually move old patches to
>> it.
> 
> My concern is whether the commitfest app will handle that well.  There
> is no "move to previous commit fest" button.  So you'd have to do it in
> some evil way, possibly confusing the continuity of the patch records.
> There might be other issues of this sort.  I don't think this use case
> is fully worked out.

Yeah, that's the point I just raised before seeing this email.  Renaming
is just going to be more simple at the end.
--
Michael


signature.asc
Description: PGP signature

Re: commitfest 2018-07

[Magnus Hagander]

On Wed, Jun 6, 2018 at 12:29 AM, Michael Paquier 
wrote:

> On Tue, Jun 05, 2018 at 11:03:33AM -0400, Stephen Frost wrote:
> > Let's keep the tech side of this simple and just do the rename as
> > suggested and then we can encourage committers to review the
> > smaller/older patches by providing information about the objective size
> > and age of them, which will likely lead to the same result without all
> > the fuss over what patch should be in what commitfest.
>
> From a technical point of view with the CF app, it is possible to move a
> patch to the "next" CF but it is not possible to choose to which commit
> fest a patch is moved.  I am not sure how the CF app chooses this next
> CF, does it choose based on the ID number of a CF, which increases for
> each new creation or based on its name?  Magnus would know that, my bet
> goes for the ID-based selection..  If my guess is right and that you
> create a CF with a name older than an existing entry, then the whole
> patch flow would be messed up.  So a rename is just much more simple at
> the end.
>

The logic for what it moves it to is basically:
* If there is an open CF, move it to that one unless we are moving from
that one
* If there is more than one open CF, error
* Else find the "future" commitfest, if one exists, and movei t ther
* If more than one future commitfest exists, error
* If no future commitfest exists, error.

I think it's also setting us up for all sorts of fun errors if it get
bounced *again*. E.g. if you "move to next cf" from the 09 cf to the 07,
but then have to "move to next cf" *again* back to 09. That would violate a
unique constraint, so it wouldn't work, and I'm unsure how the system would
actually react to it.

Thus, if we don't want to have to risk doing surgery on the system (or
guarantee we won't bounce any patches from the 07 CF, but that seems like
the wrong thing to do), we should rename the existing 09 CF to 07, so all
patches automatically end up there, and stick to only "bouncing patches in
the forward direction".

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

Re: commitfest 2018-07

[Michael Paquier]

On Tue, Jun 05, 2018 at 11:03:33AM -0400, Stephen Frost wrote:
> Let's keep the tech side of this simple and just do the rename as
> suggested and then we can encourage committers to review the
> smaller/older patches by providing information about the objective size
> and age of them, which will likely lead to the same result without all
> the fuss over what patch should be in what commitfest.

From a technical point of view with the CF app, it is possible to move a
patch to the "next" CF but it is not possible to choose to which commit
fest a patch is moved.  I am not sure how the CF app chooses this next
CF, does it choose based on the ID number of a CF, which increases for
each new creation or based on its name?  Magnus would know that, my bet
goes for the ID-based selection..  If my guess is right and that you
create a CF with a name older than an existing entry, then the whole
patch flow would be messed up.  So a rename is just much more simple at
the end.
--
Michael


signature.asc
Description: PGP signature

Re: Code of Conduct plan

[Christophe Pettus]

> On Jun 5, 2018, at 15:20, Peter Geoghegan  wrote:
> I don't follow. Practically any organized group has rules around
> conduct, with varying degrees of formality, means of enforcement, etc.

I believe the objection is to setting up a separate CoC committee, rather than 
using the core team as the enforcement mechanism.

This is more important than may be obvious.  Having a separation of the CoC 
committee and the organization that sets up and supervises the CoC committee is 
very important to prevent the perception, or the fact, that the CoC enforcement 
mechanism is a Star Chamber that is answerable only to itself.  It also allows 
for an appeal mechanism.

--
-- Christophe Pettus
   x...@thebuild.com

Re: Code of Conduct plan

[Peter Geoghegan]

On Tue, Jun 5, 2018 at 2:06 PM, Sven R. Kunze  wrote:
> 1) CoC might result in developers leaving projects
> http://lists.llvm.org/pipermail/llvm-dev/2018-May/122922.html

This guy left LLVM for several reasons. The pertinent reason for us
was that he had to agree to a code of conduct in order to attend
conferences, which he found to be unacceptable. He did not have to
agree that the idea of a code of conduct was a good one, though. It
would have been perfectly possible for him to be opposed in principle
to the idea of a CoC, while also formally agreeing to it and attending
those conferences. I gather that his objections were around questions
of unintended consequences, the role of a certain authority to assess
violations of the CoC, and so on (I surmise that he was not actually
opposed to or constrained by any of the specific rules around content
in technical presentations and so on).

I for one accept that these may have been reasonable concerns, even
though I don't really agree, since the LLVM CoC seems quite
reasonable. Anybody that participates in an open source community soon
learns that their opinion on almost any matter may not be the one that
prevails. There are often differences of opinion on -hackers that seem
to fundamentally be down to a difference in values. We still manage to
make it work, somehow.

> 2) CoC might result in not so equal peers and friends, might result in a
> committee which feels above their peers, and might promote conceit and
> denunciation.

I think that having a code of conduct is better than not having one,
and I think that the one that we came up with is appropriate and
proportionate. We could speculate all day about specific unintended
consequences that may or may not follow. That doesn't seem very
constructive, though. Besides, the time for that has passed.

> In related discussions, people recurringly ask not to establish a secondary
> judicial system but to use the already existing ones.

I don't follow. Practically any organized group has rules around
conduct, with varying degrees of formality, means of enforcement, etc.
Naturally, the rules across disparate groups vary widely for all kinds
of reasons. Formalizing and being more transparent about how this
works seems like the opposite of paternalism to me.

-- 
Peter Geoghegan

Re: \d t: ERROR: XX000: cache lookup failed for relation

[Teodor Sigaev]

Teodor Sigaev wrote:

Ah, I think this is the missing, essential component:
CREATE INDEX ON t(right(i::text,1)) WHERE i::text LIKE '%1';

Finally, I reproduce it with attached script.
In attachment simplified version of script. psql uses ordinary sql query 
to get info about index with usual transaction isolation/MVCC. To create 
a description of index it calls pg_get_indexdef() which doesn't use 
transaction snapshot, it uses catalog snapshot because it accesses to 
catalog through system catalog cache. So the difference is used snapshot 
 between ordinary SQL query and  pg_get_indexdef(). I'm not sure that 
easy to fix and should it be fixed at all.


Simplified query:
SELECT c2.relname, i.indexrelid,
pg_catalog.pg_get_indexdef(i.indexrelid, 0, true)
FROM pg_catalog.pg_class c, pg_catalog.pg_class c2,
pg_catalog.pg_index i
WHERE c.relname = 't' AND c.oid = i.indrelid AND i.indexrelid = c2.oid
--
Teodor Sigaev  E-mail: teo...@sigaev.ru
  WWW: http://www.sigaev.ru/


1.sh
Description: application/shellscript

RE: Code of Conduct plan

[Ozz Nixon]

Sorry...

> 1) CoC might result in developers leaving projects


I know this on going regurgitation is going to cause my team to leave 
the project, right around 100 posts on this off topic topic it was bad 
enough when the original idea came up (2 years ago I think). It used to be 
exciting to sit back and review the day or weeks posts... not much anymore.

Regards,
Ozz

Re: [PATCH] Trim trailing whitespace in vim and emacs

[Andrew Gierth]

> "Matthew" == Matthew Woodcraft  writes:

 Matthew> A few days ago there was a suggestion on this list to add a
 Matthew> .editorconfig file specifying tab width:
 Matthew> 
https://www.postgresql.org/message-id/87efhuz9be.fsf%40news-spur.riddles.org.uk

 Matthew> The .editorconfig format also supports a
 Matthew> trim_trailing_whitespace option (see https://editorconfig.org/
 Matthew> ).

Yes. I was actually planning to ask if any .editorconfig users (of which
I'm not one - my only use for it is to improve display output on github)
would like to contribute the most useful content for such a file.

-- 
Andrew (irc:RhodiumToad)

Re: [PATCH] Trim trailing whitespace in vim and emacs

[Andrew Gierth]

> "Peter" == Peter Eisentraut  writes:

 >> +   (lambda () (add-to-list 'write-file-functions 
 >> 'delete-trailing-whitespace)

 Peter> dir-locals doesn't work this way.  It's not a general lisp file.

Right. The correct approach is

+   (eval add-hook 'before-save-hook 'delete-trailing-whitespace nil t)))

(see the value of safe-local-eval-forms for why)

-- 
Andrew (irc:RhodiumToad)

Re: Periods

[Vik Fearing]

On June 5, 2018 9:47 PM, Paul A Jungwirth p...@illuminatedcomputing.com wrote:

> On Sat, May 26, 2018 at 1:56 PM, Vik Fearing vik.fear...@protonmail.com wrote:
>
>> SQL:2011 introduced the concept of a "period". It takes two existing columns
>> and basically does the same thing as our range types except there is no new
>> storage. I believe if Jeff Davis had given us range types a few years later
>> than he did, it would have been using periods.
>
> Hi Vik, I'm really glad to see someone working on temporal features!
> I've only dabbled in Postgres hacking, but I've been following
> temporal database research for several years, so I hope you won't mind
> my comments. I already shared some thoughts on this other thread:
> http://www.postgresql-archive.org/SQL-2011-Valid-Time-Support-td6020221.html

Hi! No, of course I don't mind your comments; I welcome them. I had not seen 
that thread so I'll go take a look at it.

> I would love to see Postgres support the standard but also let
> people use range types. I'm not sure I agree that Jeff Davis would
> have preferred the SQL:2011 period idea, which is an extra-relational
> concept. Since it is attached to a table, it doesn't carry through
> cleanly to a result set, so what happens if you want to apply temporal
> features to a view, subquery, CTE, or set-returning function?

As far as I can see, the standard doesn't say what should happen if you select 
a period, or even if that's possible.  It does however define how to create a 
period not attached to a table (PERIOD   
  ) so it would be possible to use that 
for views, subqueries, and the rest of your examples.

> A range on the other hand is just a type, so as long as temporal operators
> support that type, everything still composes nicely and just works.
> The Date/Darwen/Lorenztos book has more to say about that, and I think
> it's worth reading. They are unrealistically extreme in their purism,
> but here I think they have some good points---points they also raised
> against an earlier proposed temporal-SQL standard circa 1998. By the
> way here are some thoughts Jeff shared with me about that book, which
> he says inspired range types:
> https://news.ycombinator.com/item?id=14738655

Thanks, I will read this, too.

> I understand that your patch is just to allow defining periods, but I
> thought I'd share my own hopes earlier rather than later, in case you
> are doing more work on this.

Yes, I plan on doing much more work on this.  My goal is to implement (by 
myself or with help from other) the entire SQL:2016 spec on periods and system 
versioned tables.  This current patch is just infrastructure.

> Also, it might be nice if Postgres let
> you also define periods from a single range column, so that people who
> want to use intervals can still stick closer to the standard---I
> dunno, just an idea.

That's a nice idea, but I'm not sure how I'd fit it into the pg_period catalog 
which expects two columns.

> Also, this may not be very helpful, but I started an extension to
> support temporal foreign keys here:
> https://github.com/pjungwir/time_for_keys
> It uses intervals, not periods, but maybe you can steal some ideas.
> :-) I have a half-finished branch porting it from plpgsql to C, so
> that I could give them more catalog integration, and also I have hopes
> of defining temporal primary keys, although that isn't implemented
> yet. Anyway, I mention it because you said, "Application periods can
> be used in PRIMARY/UNIQUE keys, foreign keys," but feel free to ignore
> it. :-)

While I'm waiting for comments on how best to do inheritance and other aspects 
of my patch, I'm working on getting PRIMARY/UNIQUE keys with periods.  That's 
far from finished though as it is touching parts of the code that I have never 
looked at before.

> In general, I would love Postgres to have some lower-level primitives
> like range types and the Dingös operators, and then build the
> SQL:2011 support on top of those. I'm happy to contribute work to help
> make that happen, although I'd probably need to work with someone with
> more Postgres hacking experience to get it done.

Any help you can give me (or that I could give you) is greatly appreciated.  
I'm hoping we can get *something* in v12 with periods.

Re: [PATCH] Trim trailing whitespace in vim and emacs

[Matthew Woodcraft]

On 2018-06-05 18:22, David Fetter wrote:
> Folks,
> 
> Here's my attempt to $subject. I've tested with vim, but I'm much less
> certain I got the EMACS code right.
> 
> Is there any interest in such a feature?

A few days ago there was a suggestion on this list to add a
.editorconfig file specifying tab width:
https://www.postgresql.org/message-id/87efhuz9be.fsf%40news-spur.riddles.org.uk

The .editorconfig format also supports a trim_trailing_whitespace option
(see https://editorconfig.org/ ).

So that might be a simpler way to achieve this (I suppose people who
don't want this behaviour just wouldn't configure their editors to
honour .editorconfig files).

-M-

Re: automating perl compile time checking

[Andrew Dunstan]

On 06/05/2018 05:14 PM, Daniel Gustafsson wrote:


This comment should say “perlcheck/..” and not “pgperlcritic/.." I assume:



Thanks. will fix.

cheers

andrew

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

Re: automating perl compile time checking

[Daniel Gustafsson]

> On 5 Jun 2018, at 16:31, Andrew Dunstan  
> wrote:

> The patch contains a simple script to run the checks. The code that finds 
> perl files is put in a function in a single file that is sourced by the three 
> locations that need it.

+1 on centralizing the find-files function.

> The directory pgperlcritic is renamed to perlcheck, as it not contains the 
> new script as well as pgperlcritic.

This comment should say “perlcheck/..” and not “pgperlcritic/.." I assume:

--- /dev/null
+++ b/src/tools/perlcheck/pgperlcritic
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+# src/tools/pgperlcritic/pgperlcritic

cheers ./daniel

Re: Why is fncollation in FunctionCallInfoData rather than fmgr_info?

[Chapman Flack]

On 06/05/2018 04:57 PM, Andres Freund wrote:
> But we don't really change the
> collation of function invocations per-call. 

Is that true? (Not a rhetorical question; I'm unsure.)

Is your argument that the expression's collation is determined once
for each call /site/, and thereafter doesn't change over repeated
invocations via any one call site, and a unique FmgrInfo is
populated at each call site?

This is at the edges of my current understanding of how those gears
turn, so it could very well be true, but it seems to me that's how
the question would have to be put.

-Chap

Re: Code of Conduct plan

[Sven R. Kunze]

Hi PostgreSQL Community,

some points I like to make mainly because of observations of how other 
open source projects handle this topic:



1) CoC might result in developers leaving projects
http://lists.llvm.org/pipermail/llvm-dev/2018-May/122922.html

2) CoC might result in not so equal peers and friends, might result in a 
committee which feels above their peers, and might promote conceit and 
denunciation. That is why some projects choose not to have one
https://freie-software.org/verein/coc.html - they say: "we're friends - 
that's our CoC, more would be harmful" [1]


3) https://shiromarieke.github.io/coc.html explains why there's no safe 
space and CoC won't change that (she's a queer woman who experienced 
harassment and sexual assault)



In related discussions, people recurringly ask not to establish a 
secondary judicial system but to use the already existing ones.



I hope these points can influence what is in the CoC or whether there 
will a CoC at all.
Personally, I find 2) a very good case against CoC (although I like the 
"we're friends - that's our CoC, more would be harmful").



Best,
Sven


On 03.06.2018 20:29, Tom Lane wrote:

Two years ago, there was considerable discussion about creating a
Code of Conduct for the Postgres community, as a result of which
the core team announced a plan to create an exploration committee
to draft a CoC [1].  That process has taken far longer than expected,
but the committee has not been idle.  They worked through many comments
and many drafts to produce a version that seems acceptable in the view
of the core team.  This final(?) draft can be found at

https://wiki.postgresql.org/wiki/Code_of_Conduct


[1] Appendix - Google translation of the CoC of Freie Software:

Code of Conduct
Don't have it. Don't want to have.

That's the short version. The long version follows.

A "Code of Conduct" is a code of conduct in the sense of a set of norms 
intended to determine the behavior of addressees of the Code.


Thoughts on the normalization of the self-evident
If one reads current, relevant regulations, one finds that normal 
self-evident behaviors are normalized there. What is required there is 
the attitude and behavior of a reasonably reasonable, reasonably well 
behaved person.


That seems remarkable. Rules are set up when there is a risk that they 
will be broken. You should act on the addressee from the outside, 
because you fear that he will not behave properly without this impact.


Such a framework thus says something about the constitution of the 
community or society to which the rules apply. In this case, a 
reasonable behavior is obviously not (of course) obvious.


Among friends, the behaviors and attitudes described in the relevant 
regulations, such as respect, attention and helpfulness, 
non-discrimination, the will to cooperate, rule-free intercourse, etc., 
are self-evident. Friends behave as each other as required in these 
rules. At least most. If not always.


The biggest lump in the whole country ...
The relevant regulations then provide for the appointment of persons or 
bodies to whom, if one believes the rules have been violated, one can 
turn to oneself.


In most cases such a complaint is permissible not only in case of 
personal concern, but also if one thinks that the rules have been 
violated to the detriment of one or the other. Experience teaches that 
this often challenges behaviors that can kill any friendship. Knowing 
better and being feeling informers usually have only like-minded people 
as social contact.


But we do not want to promote either conceit or denunciation.

If someone does not behave as it is self-evident, then there are 
reasons. These can be different types. A clear word among friends in 
private or in a small circle is then helpful - for the "victim", as well 
as for the "perpetrator". The latter deserves respect, 
non-discrimination, attention, helpfulness and understanding. The latter 
should actually be self-evident, but it is often not the case when 
executing a Code of Conduct.


Nor is a rule-free, friendly dealing with the accused possible. The 
roles of the judge and a friend are incompatible. Friends meet at eye 
level; the judge has power and authority to exercise, even if he acquits.


Penalties among friends?
Finally, a Code of Conduct will include a sanctioning apparatus to 
sanction undesirable behavior. Deliberate addition of evils 
(punishments) among friends is a contradiction in terms.


From this, it can be concluded that the moment a Code of Conduct takes 
effect, the friendship is already over. When we get to that point, we 
should dissolve our club, because then we failed - all together.


Therefore, we do not need and do not want a code of conduct in the sense 
of a set of rules.


Resistance to unreasonableness
Sometimes, in recent times, the demand for a code of conduct in the form 
of a corresponding set of rules is unfortunately linked with a 
(financial) aid offer. Help under 

Why is fncollation in FunctionCallInfoData rather than fmgr_info?

[Andres Freund]

Hi,

In my understanding FunctionCallInfoData is basically per-call data,
whereas FmgrInfo is information about the function.  It makes some sense
that ->context is in FunctionCallInfoData, after all it's used for
per-row data like the trigger context.  But we don't really change the
collation of function invocations per-call.  Thus I don't quite get why
FunctionCallInfoData contains information about it rather than FmgrInfo.

Now I get that there's the practical reason that we want to pass the
collation to function invocations made with DirectionFunctionCall*Coll,
and they currently don't set up a FmgrInfo.  But that doesn't really
convince me this is a good idea.

The reason I'm asking is that I'm fixing the JIT code generation for
expressions to be more efficient. And for functions that do not get
inlined I currently need to set up a separate *per-call*
FunctionCallInfoData to allow the optimizer to be free enough to do it's
thing.  And the less metadata has to be copied around, the better.  I'm
not sure it's realistic to change this, I mostly want to understand the
reasoning.

Greetings,

Andres Freund

Re: Recovery performance of DROP DATABASE with many tablespaces

[Ashwin Agrawal]

On Mon, Jun 4, 2018 at 9:46 AM, Fujii Masao  wrote:

> Generally the recovery performance of DROP DATABASE is not critical
> for many users. But unfortunately my colleague's project might need to
> sometimes drop the database using multiple tablespaces, for some reasons.
> So, if the fix is not so complicated, I think that it's worth applying
> that.
>

Agree, in isolation need for this improvement is not felt, but yes any
improvements for single serialized replay process is definitely helpful.


> The straight approach to avoid such unnecessary scans is to change
> DROP DATABASE so that it generates only one XLOG_DBASE_DROP record,
> and register the information of all the tablespace into it. Then, WAL
> replay
> of XLOG_DBASE_DROP record scans shared_buffers once and deletes
> all tablespaces. POC patch is attached.
>

Also, irrespective of performance improvement looks better to just have
single xlog record for the same.

Re: [PATCH] Trim trailing whitespace in vim and emacs

[Peter Eisentraut]

On 6/5/18 13:37, Tom Lane wrote:
> I note that Peter E. seems to have a recipe for finding such issues,
> which I suspect is grounded in some obscure git feature or other.
> That might be easier to work with, since you'd only need one fix
> not one per editor.

I have a git alias:

check-whitespace = !git diff-tree --check $(git hash-object -t tree
/dev/null) HEAD ${1:-$GIT_PREFIX}

Also, in Emacs I use

(global-whitespace-mode 1)
(setq whitespace-style
  '(face
trailing empty indentation space-after-tab space-before-tab))

With source code other than PostgreSQL, this will occasionally result in
angry fruit salad.

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

Re: [PATCH] Trim trailing whitespace in vim and emacs

[Peter Eisentraut]

On 6/5/18 13:22, David Fetter wrote:
> diff --git a/.dir-locals.el b/.dir-locals.el
> index 9525d6b604..858461e6bd 100644
> --- a/.dir-locals.el
> +++ b/.dir-locals.el
> @@ -4,7 +4,8 @@
>  (c-file-style . "bsd")
>  (fill-column . 78)
>  (indent-tabs-mode . t)
> -(tab-width . 4)))
> +(tab-width . 4)
> + (lambda () (add-to-list 'write-file-functions 
> 'delete-trailing-whitespace)
>   (dsssl-mode . ((indent-tabs-mode . nil)))
>   (nxml-mode . ((indent-tabs-mode . nil)))
>   (perl-mode . ((perl-indent-level . 4)

dir-locals doesn't work this way.  It's not a general lisp file.

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

Re: libpq compression

[Dave Cramer]

On 5 June 2018 at 13:06, Peter Eisentraut 
wrote:

> On 6/5/18 03:09, Michael Paquier wrote:
> > I just had a quick look at this patch, lured by the smell of your latest
> > messages...  And it seems to me that this patch needs a heavy amount of
> > work as presented.  There are a couple of things which are not really
> > nice, like forcing the presentation of the compression option in the
> > startup packet to begin with.
>
> Yeah, at this point we will probably need a discussion and explanation
> of the protocol behavior this is adding, such as how to negotiate
> different compression settings.
>
> Unrelatedly, I suggest skipping the addition of -Z options to various
> client-side tools.  This is unnecessary, since generic connection
> options can already be specified via -d typically, and it creates
> confusion because -Z is already used to specify output compression by
> some programs.
>
>
As the maintainer of the JDBC driver I would think we would also like to
leverage this as well.

There are a few other drivers that implement the protocol as well and I'm
sure they would want in as well.
I haven't looked at the patch but if we get to the point of negotiating
compression please let me know.

Thanks,

Dave Cramer

da...@postgresintl.com
www.postgresintl.com

Re: Periods

[Paul A Jungwirth]

On Tue, Jun 5, 2018 at 12:47 PM, Paul A Jungwirth
 wrote:
> Also, this may not be very helpful, but I started an extension to
> support temporal foreign keys here:
>
> https://github.com/pjungwir/time_for_keys
>
> It uses intervals, not periods, but maybe you can steal some ideas.

Sorry for two emails but I wanted to add: the more stealable thing are
the tests, which are pretty thorough and took a lot of hours to write.
They are yours if you want them. :-)

Paul

Re: Variable-length FunctionCallInfoData

[Andres Freund]

Hi,

On 2018-06-05 15:08:33 -0400, Peter Eisentraut wrote:
> On 6/5/18 13:29, Andres Freund wrote:
> > Besides the change here, I think we should also go much further with the
> > conversion to NullableDatum.  There's two main areas of change: I want
> > to move the execExpr.c related code so steps return data into
> > NullableDatums - that removes a good chunk of pointer dereferences and
> > allocations. Secondly I think we should move TupleTableSlot to this
> > format - the issue with nulls / datums being on separate cachelines is
> > noticeable in profiles, but more importantly the code looks more
> > consistent with it.
> 
> There are also a variety of utility functions that return a Datum and
> have an extra bool *isnull argument.

I was thinking of, for now at least, not touching those.


> What are your thoughts on using NullableDatum more in those cases?  Is
> returning structs a problem for low-level performance?

It vastly depends on architecture, compiler and ABI. It'd be a lot
better if we were using C++ (because the caller reserves the space for
such things, and then it's basically free, see return value optimization
/ copy elision).  Thus I'm not wild in immediately changing all of them.
I think there aren't that many that aren't performance critical however,
so I guess an argument could be made to change this regardless.

Greetings,

Andres Freund

Re: SQL:2011 Valid-Time Support

[Paul A Jungwirth]

On Fri, May 11, 2018 at 4:48 AM, Paul A Jungwirth
 wrote:
> I'm traveling and can't write much more at the moment, but I'll try to
> reply more fully in a week or two.

Sorry it took awhile to continue this discussion! If people are
interested in implementing temporal features in Postgres, I wrote an
annotated bibliography here about the research:

https://illuminatedcomputing.com/posts/2017/12/temporal-databases-bibliography/

A shorter version, focused on the patches offered by Dingös et al, is here:

https://www.mail-archive.com/pgsql-hackers@postgresql.org/msg324631.html

There is a lot of history, and temporal features tend to be pretty
drastic and far-reaching (e.g. a "temporal primary key" is more like
an exclusion constraint). It's really worth reading a bit from the
research. The Date/Darwen/Lorentzos book is very good, although I'd
take some of it with a grain of salt (e.g. no nulls).

I think SQL:2011 is so limited & over-specific, it would be great to
offer it on top of some more generalized infrastructure. The Dingös
work is great, so that's what I'd like to see. With their new
operators, it would be very easy to build the stuff from the standard.
This paper is only 10.5 pages (not counting bibliography & appendix)
and easy to understand:

https://files.ifi.uzh.ch/boehlen/Papers/modf174-dignoes.pdf

Paul

Re: Variable-length FunctionCallInfoData

[Peter Eisentraut]

On 6/5/18 13:29, Andres Freund wrote:
> Besides the change here, I think we should also go much further with the
> conversion to NullableDatum.  There's two main areas of change: I want
> to move the execExpr.c related code so steps return data into
> NullableDatums - that removes a good chunk of pointer dereferences and
> allocations. Secondly I think we should move TupleTableSlot to this
> format - the issue with nulls / datums being on separate cachelines is
> noticeable in profiles, but more importantly the code looks more
> consistent with it.

There are also a variety of utility functions that return a Datum and
have an extra bool *isnull argument.  What are your thoughts on using
NullableDatum more in those cases?  Is returning structs a problem for
low-level performance?

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

Re: [PATCH v17] GSSAPI encryption support

[Robbie Harwood]

Thomas Munro  writes:

> On Sat, May 26, 2018 at 6:58 AM, Robbie Harwood  wrote:
>> Me and the bot are having an argument.  This should green Linux but I
>> dunno about Windows.
>
> BTW if you're looking for a way to try stuff out on Windows exactly
> the way cfbot does it without posting a new patch to the mailing list,
> I put some instructions here:
>
> https://wiki.postgresql.org/wiki/Continuous_Integration
>
> The .patch there could certainly be improved (ideally, I think, it'd
> run the whole build farm animal script) but it's a start.

Appreciated.  I think I've got it now.

Thanks,
--Robbie


signature.asc
Description: PGP signature
>From 43a83485a487f8aaf78109dea5d63b26b395c683 Mon Sep 17 00:00:00 2001
From: Robbie Harwood 
Date: Thu, 10 May 2018 16:12:03 -0400
Subject: [PATCH] libpq GSSAPI encryption support

On both the frontend and backend, prepare for GSSAPI encryption
support by moving common code for error handling into a separate file.
Fix a TODO for handling multiple status messages in the process.
Eliminate the OIDs, which have not been needed for some time.

Add frontend and backend encryption support functions.  Keep the
context initiation for authentication-only separate on both the
frontend and backend in order to avoid concerns about changing the
requested flags to include encryption support.

In postmaster, pull GSSAPI authorization checking into a shared
function.  Also share the initiator name between the encryption and
non-encryption codepaths.

Modify pqsecure_write() to take a non-const pointer.  The pointer will
not be modified, but this change (or a const-discarding cast, or a
malloc()+memcpy()) is necessary for GSSAPI due to const/struct
interactions in C.

For HBA, add "hostgss" and "hostnogss" entries that behave similarly
to their SSL counterparts.  "hostgss" requires either "gss", "trust",
or "reject" for its authentication.

Simiarly, add a "gssmode" parameter to libpq.  Supported values are
"disable", "require", and "prefer".  Notably, negotiation will only be
attempted if credentials can be acquired.  Move credential acquisition
into its own function to support this behavior.

Finally, add documentation for everything new, and update existing
documentation on connection security.

Thanks to Michael Paquier for the Windows fixes.
---
 doc/src/sgml/client-auth.sgml   |  75 --
 doc/src/sgml/libpq.sgml |  57 +++-
 doc/src/sgml/runtime.sgml   |  77 +-
 src/backend/libpq/Makefile  |   4 +
 src/backend/libpq/auth.c| 103 +++
 src/backend/libpq/be-gssapi-common.c|  64 +
 src/backend/libpq/be-gssapi-common.h|  26 ++
 src/backend/libpq/be-secure-gssapi.c| 319 ++
 src/backend/libpq/be-secure.c   |  16 ++
 src/backend/libpq/hba.c |  51 +++-
 src/backend/postmaster/pgstat.c |   3 +
 src/backend/postmaster/postmaster.c |  64 -
 src/include/libpq/hba.h |   4 +-
 src/include/libpq/libpq-be.h|  11 +-
 src/include/libpq/libpq.h   |   3 +
 src/include/libpq/pqcomm.h  |   5 +-
 src/include/pgstat.h|   3 +-
 src/interfaces/libpq/Makefile   |   4 +
 src/interfaces/libpq/fe-auth.c  |  90 +--
 src/interfaces/libpq/fe-connect.c   | 232 +++-
 src/interfaces/libpq/fe-gssapi-common.c | 128 +
 src/interfaces/libpq/fe-gssapi-common.h |  23 ++
 src/interfaces/libpq/fe-secure-gssapi.c | 343 
 src/interfaces/libpq/fe-secure.c|  16 +-
 src/interfaces/libpq/libpq-fe.h |   3 +-
 src/interfaces/libpq/libpq-int.h|  30 ++-
 src/tools/msvc/Mkvcbuild.pm |  22 +-
 27 files changed, 1574 insertions(+), 202 deletions(-)
 create mode 100644 src/backend/libpq/be-gssapi-common.c
 create mode 100644 src/backend/libpq/be-gssapi-common.h
 create mode 100644 src/backend/libpq/be-secure-gssapi.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.c
 create mode 100644 src/interfaces/libpq/fe-gssapi-common.h
 create mode 100644 src/interfaces/libpq/fe-secure-gssapi.c

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 656d5f9417..38cf32e3be 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -108,6 +108,8 @@ hostnossl  database  user
 host   database  user  IP-address  IP-mask  auth-method  auth-options
 hostssldatabase  user  IP-address  IP-mask  auth-method  auth-options
 hostnossl  database  user  IP-address  IP-mask  auth-method  auth-options
+hostgssdatabase  user  IP-address  IP-mask  auth-method  auth-options
+hostnogss  database  user  IP-address  IP-mask  auth-method  auth-options
 
The meaning of the fields is as follows:
 
@@ -128,9 +130,10 @@ hostnossl  database  user
  
   
This record matches connection attempts made using TCP/IP.
-   host records match either
+   host records 

Re: [PATCH] Trim trailing whitespace in vim and emacs

[Teodor Sigaev]

I use FileStly plugin to vim [1]. But I slightly modify it,  see in attachment.

FileStyle, sorry.

--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/

Re: [PATCH] Trim trailing whitespace in vim and emacs

[Teodor Sigaev]

I once tried to have vim highlight trailing whitespace, spaces before
tabs, and overlength lines (>80 chars), and while I could do each thing
in isolation, I wasn't able to get it to highlight the three of them at
the same time.  If you have a recipe for that, I welcome it.


I use FileStly plugin to vim [1]. But I slightly modify it,  see in attachment.
And addition in .vimrc:

if expand('%:e') == "c" ||  expand('%:e') == "h" || expand('%:e')  == "cpp" || 
expand('%:e')  == "m" || expand('%:e') == "hpp" || expand('%:e') == "pl" || 
expand('%:e') == "pm" ||  expand('%:e') == "y" ||  expand('%:e') == "l"

else
let g:filestyle_plugin = 1
endif


[1]  https://www.vim.org/scripts/script.php?script_id=5065
--
Teodor Sigaev   E-mail: teo...@sigaev.ru
   WWW: http://www.sigaev.ru/
"   Copyright 2014 Alexander Serebryakov
"
"   Licensed under the Apache License, Version 2.0 (the "License");
"   you may not use this file except in compliance with the License.
"   You may obtain a copy of the License at
"
"   http://www.apache.org/licenses/LICENSE-2.0
"
"   Unless required by applicable law or agreed to in writing, software
"   distributed under the License is distributed on an "AS IS" BASIS,
"   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
"   See the License for the specific language governing permissions and
"   limitations under the License.

"Plugin checking the file to follow Your Vim settings

if !exists('g:filestyle_plugin')
  let g:filestyle_plugin = 1

  highligh FileStyleTabsError ctermbg=Red guibg=Red
  highligh FileStyleTrailngSpacesError ctermbg=Cyan guibg=Cyan
  highligh FileStyleSpacesError ctermbg=Yellow guibg=Yellow
  highligh FileStyleTooLongLine cterm=inverse gui=inverse

  "Defining auto commands
  augroup filestyle_auto_commands
autocmd!
autocmd BufReadPost,BufNewFile * call FileStyleActivate()
autocmd FileType * call FileStyleCheckFiletype()
autocmd WinEnter * call FileStyleCheck()
  augroup end

  "Defining plugin commands
  command! FileStyleActivate call FileStyleActivate()
  command! FileStyleDeactivate call FileStyleDeactivate()
  command! FileStyleCheck call FileStyleCheck()

endif


"Turn plugin on
function FileStyleActivate()
  let b:filestyle_active = 1
  call FileStyleCheck()
endfunction


"Turn plugin off
function FileStyleDeactivate()
  let b:filestyle_active = 0
  call clearmatches()
endfunction


"Check filetype to handle specific cases
function FileStyleCheckFiletype()
  "Avoid checking of help files
  if =='help'
call FileStyleDeactivate()
  endif
endfunction


"Highlighting specified pattern
function FileStyleHighlightPattern(highlight)
  call matchadd(a:highlight['highlight'], a:highlight['pattern'])
endfunction


"Checking expandtab option
function FileStyleExpandtabCheck()
  if 
let l:highlight = {'highlight' : 'FileStyleTabsError',
 \ 'pattern': '\t\+'}
  else
let l:highlight = {'highlight' : 'FileStyleSpacesError',
 \ 'pattern': '^\t* \{'.',\}'}
  endif
  call FileStyleHighlightPattern(l:highlight)
endfunction


"Checking trailing spaces
function FileStyleTrailingSpaces()
let l:highlight = {'highlight' : 'FileStyleTrailngSpacesError',
 \ 'pattern': '\s\+$'}
  call FileStyleHighlightPattern(l:highlight)
endfunction

"Checking inner spaces
function FileStyleInnerSpaces()
let l:highlight = {'highlight' : 'FileStyleTabsError',
 \ 'pattern': '\ \+\t'}
  call FileStyleHighlightPattern(l:highlight)
endfunction

function FileStyleAllInnerSpaces()
let l:highlight = {'highlight' : 'FileStyleTabsError',
 \ 'pattern': '\ \{4,\}'}
  call FileStyleHighlightPattern(l:highlight)
endfunction


"Checking long lines
function FileStyleLongLines()
  if  > 0
let l:highlight = {'highlight' : 'FileStyleTooLongLine',
 \ 'pattern': '\%' . (+1) . 'v.*' }
call FileStyleHighlightPattern(l:highlight)
  endif
endfunction


"Checking file dependenly on settings
function FileStyleCheck()
  if b:filestyle_active == 1
call clearmatches()
call FileStyleExpandtabCheck()
call FileStyleTrailingSpaces()
call FileStyleInnerSpaces()
call FileStyleAllInnerSpaces()
call FileStyleLongLines()
  endif
endfunction

Re: POC: GROUP BY optimization

[Teodor Sigaev]

Thanks for the patch. This (missing) optimization popped-up repeatedly recently, 
and I was planning to look into it for PG12. So now I don't have to, because 
you've done all the hard work ;-)
You are welcome. Actually one of out customers faced the problem with  GROUP BY 
column order and exactly with reordering without any indexes, you mean it as 
problem 2). Index optimization was noticed by me later. But based on your 
suggested patch's order I split the patch to index  and non-index part and 
second part depends of first one. They touch the same part of code and they 
could not be independent


1) add_path() ensures that we only keep the one cheapest path sorted path for 
each pathkeys. This patch somewhat defeats that because it considers additional 
pathkeys (essentially any permutation of group keys) as interesting. So we may 
end up with more paths in the list.

Seems, index scan here could give benefits here only if:
  1) it's a index only scan
  2) it's a index full (opposite to only) scan but physical order of heap is
 close to logical index order (ie table is clustered)

In other cases costs of disk seeking will be very high. But on this stage of 
planing we don't know that facts yet. So we couldn't make a good decision here 
and should believe in add_path() logic.


> I wonder if we should make the add_path() logic smarter to recognize when two
> paths have different pathkeys but were generated to match the same grouping,
> to reduce the number of paths added by this optimization. Currently we do 
that > for each pathkeys list independently, but we're considering many more 
pathkeys > orderings ...


Hm. I tend to say no.
select .. from t1 group by a, b
union
select .. from t2 group by a, b

t1 and t2 could have different set of indexes and different distribution, so 
locally it could be cheaper to use one index (for example, one defined as (b, a) 
and second as (a,b,c,d) - second is larger) but totally - another (example: 
second table doesn't have (b,a) index)



2) sort reordering based on ndistinct estimates


But thinking about this optimization, I'm worried it relies on a couple of 
important assumptions. For now those decisions could have be made by the person 
writing the SQL query, but this optimization makes that impossible. So we really 
need to get this right.

Hm, sql by design should not be used that way, but, of course, it's used :(

For example, it seems to disregard that different data types have different 
comparison costs. For example comparing bytea will be far more expensive 
compared to int4, so it may be much more efficient to compare int4 columns 
first, even if there are far fewer distinct values in them.
as I can see cost_sort() doesn't pay attention to this details. And it should be 
a separated patch to improve that.


Also, simply sorting the columns by their ndistinct estimate is somewhat naive, 
because it assumes the columns are independent. Imagine for example a table with 
three columns:
So clearly, when evaluating GROUP BY a,b,c it'd be more efficient to use 
"(a,c,b)" than "(a,b,c)" but there is no way to decide this merely using 
per-column ndistinct values. Luckily, we now have ndistinct multi-column 
coefficients which could be used to decide this I believe (but I haven't tried).

Agree, but I don't know how to use it here. Except, may be:
1) first column - the column with bigger estimated number of groups
2) second column - the pair of (first, second) with bigger estimated number of 
groups

3) third column - the triple of (first, second, third) with bigger ...

But seems even with that algorithm, ISTM, it could be implemented in cheaper 
manner.


The real issue however is that this decision can't be made entirely locally. 
Consider for example this:


     explain select a,b,c, count(*) from t group by a,b,c order by c,b,a;

Which is clearly cheaper (at least according to the optimizer) than doing two 
separate sorts. So the optimization actually does the wrong thing here, and it 
needs to somehow consider the other ordering requirements (in this case ORDER 
BY) - either by generating multiple paths with different orderings or some 
heuristics.
Hm, thank you. I consider it is a bug of my implementation - basic idea was that 
we try to match already existing or needed order and only if we fail or have 
unmatched tail of pathkey list than we will try to find cheapest column order.
Fixed in v7 (0002-opt_group_by_index_and_order-v7.patch), but may be by naive 
way: if we don't have a path pathkey first try to reorder columns accordingly to 
order by clause. Test for your is also added.



I'm also wondering how freely we can change the group by result ordering. Assume 
you disable hash aggregate and parallel query - currently we are guaranteed to 
use group aggregate that produces exactly the ordering as specified in GROUP BY, 
but this patch removes that "guarantee" and we can produce arbitrary permutation 
of the ordering. But as I argued in other 

Re: Code of Conduct plan

[Stephen Frost]

Greetings,

* Magnus Hagander (mag...@hagander.net) wrote:
> On Tue, Jun 5, 2018 at 4:45 PM, Chris Travers 
> wrote:
> > If I may suggest:  The committee should be international as well and
> > include people from around the world.  The last thing we want is for it to
> > be dominated by people from one particular cultural viewpoint.
> 
> It will be. This is the PostgreSQL *global* development group and project,
> after all. Yes, there is definitely a slant in the project in general
> towards the US side, as is true in many other such projects, but in general
> we have decent coverage of other cultures and countries as well. We can't
> cover them all  on the committee (that would make for a gicantic
> committee), but we can cover it with people who are used to communicating
> and working with people from other areas as well, which makes for a better
> understanding.
> 
> It won't be perfect in the first attempt, of course, but that one is
> covered.

This drives to a point which I was thinking about also- what is needed
on the committee are people who are worldly to the point of
understanding that there are different cultures and viewpoints, and
knowing when and how to ask during an investigation to get an
understanding of if the issue is one of cultural differences (leading
potentially to education and not to reprimand, as discussed in the CoC),
something else, or perhaps both.

The CoC committee doesn't need to be comprimised of individuals from
every culture to which the community extends, as that quickly becomes
untenable.

I'm confident that the Core team will work to ensure that the initial
committee is comprised of such individuals and that both Core and the
subsequent CoC committees will work to maintain that.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: I'd like to discuss scaleout at PGCon

[MauMau]

From: Simon Riggs
On 1 June 2018 at 16:56, Ashutosh Bapat
 wrote:
>> I think partitioning + FDW provide basic infrastructure for
>> distributing data, planning queries working with such data. We need
>> more glue to support node management, cluster configuration. So, I
>> agree with your statement. But I think it was clear from the
beginning
>> that we need more than FDW and partitioning.
>
> No, it wasn't clear. But I'm glad to hear it. It might actually work
then.

I found a possibly interesting description in the XL manual.  Although
XL performs various pushdowns like FDW, XL seems to perform some kind
of joins with datanode-to-datanode communication.  Doesn't this prove
that the FDW approach can't handle those joins optimally?  What kind
of joins use the shared queue?



https://www.postgres-xl.org/documentation/pg-xc-specifics.html
--
shared_queues (integer)
Datanode Only

For some joins that occur in queries, data from one Datanode may need
to be joined with data from another Datanode. Postgres-XL uses shared
queues for this purpose. During execution each Datanode knows if it
needs to produce or consume tuples, or both.

Note that there may be mulitple shared_queues used even for a single
query. So a value should be set taking into account the number of
connections it can accept and expected number of such joins occurring
simultaneously.
--


Regards
MauMau

Re: Spilling hashed SetOps and aggregates to disk

[Andres Freund]

Hi,

On 2018-06-05 10:47:49 -0700, Jeff Davis wrote:
> The thing I don't like about it is that it requires running two memory-
> hungry operations at once. How much of work_mem do we use for sorted
> runs, and how much do we use for the hash table?

Is that necessarily true? I'd assume that we'd use a small amount of
memory for the tuplesort, enough to avoid unnecessary disk spills for
each tuple. But a few kb should be enough - think it's fine to
aggressively spill to disk, we after all already have handled the case
of smaller number of input rows.  Then at the end of the run, we empty
out the hashtable, and free it. Only then we do to the sort.

One thing this wouldn't handle are datatypes that support hashing, but
no sorting. Not exactly common.

Greetings,

Andres Freund

Re: [PATCH] Trim trailing whitespace in vim and emacs

[David Fetter]

On Tue, Jun 05, 2018 at 01:28:54PM -0400, Alvaro Herrera wrote:
> On 2018-Jun-05, David Fetter wrote:
> Hi David
> 
> > Here's my attempt to $subject. I've tested with vim, but I'm much less
> > certain I got the EMACS code right.
> > 
> > Is there any interest in such a feature?
> 
> I'd rather have the editor warn me (highlight) such things rather than
> fix them silently (I wonder if it'd cause a mess with regression .out
> files for example, which I do edit on occasion).

For what it's worth, the patch trims trailing whitespace only on
certain types of files in vim and just one (C) in emacs for just this
reason.

> I once tried to have vim highlight trailing whitespace, spaces before
> tabs, and overlength lines (>80 chars), and while I could do each thing
> in isolation, I wasn't able to get it to highlight the three of them at
> the same time.  If you have a recipe for that, I welcome it.

In vim, one way to do it is:

highlight link sensibleWhitespaceError Error
autocmd Syntax * syntax match sensibleWhitespaceError excludenl /\s\+\%#\@ http://fetter.org/
Phone: +1 415 235 3778

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

Re: Spilling hashed SetOps and aggregates to disk

[Jeff Davis]

On Tue, 2018-06-05 at 05:42 -0700, Andres Freund wrote:
> That's an interesting idea, but it seems simpler to stick to
> > hashing
> > rather than using a combination strategy. It also seems like it
> > would
> > take less CPU effort.
> Isn't the locality of access going to considerably better with the
> sort
> based approach?

I think I see what you mean, but without measuring it's hard to say.
You may be able to achieve similar locality in the hashing approach by
using a smaller hash table -- an effect that I think I observed
previously but I'd have to rerun the numbers.

> > 
> > What advantages do you have in mind? My patch partitions the
> > spilled
> > data, so it should have similar disk costs as a sort approach.
> I think one part of it is that I think the amount of code is going to
> be
> lower - we essentially have already all the code to handle sort based
> aggs, and to have both sort and hash based aggs in the same query.
> We'd
> mostly need a way to scan the hashtable and stuff it into a
> tuplesort,
> that's not hard.  nodeAgg.c is already more than complex enough, I'm
> not
> sure that full blown partitioning is worth the cost.

The thing I like about your idea is that we wouldn't need to make a
choice at plan time, we just always do the hybrid hashing/sorting
unless we know that it's already sorted.

The thing I don't like about it is that it requires running two memory-
hungry operations at once. How much of work_mem do we use for sorted
runs, and how much do we use for the hash table?

Regards,
Jeff Davis

Re: Variable-length FunctionCallInfoData

[Andres Freund]

Hi,

On 2018-06-05 10:40:22 -0700, se...@rielau.com wrote:
> Big +1 on this one.

Cool.


> Here is what we did. It's very crude, but minimized the amount of pain:

I think I'd rather go for my approach in core though. Needlessly
carrying around a bunch of pointers, and adding the necessary
indirections on accesses, and more allocations don't seem to buy us that
much. Nor do I really like the hackyness...

Greetings,

Andres Freund

RE: Variable-length FunctionCallInfoData

[serge]

Big +1 on this one.
 
Here is what we did. It's very crude, but minimized the amount of pain:


It helps that the C compiler treats arrays and pointers the same.
 
I can dig for the complete patch if you want...
 
Cheers
Serge
/*
 * This struct is the data actually passed to an fmgr-called function.
 * There are three flavors:
 * FunctionCallInfoData:
 *   Used when the number of arguments is both known and fixed small
 *   This structure is used for direct function calls involving
 *   builtin functions
 *   This structure must be initialized with: InitFunctionCallInfoData()
 * FunctionCallInfoDataVariable:
 *   Used when the number of arguments is unknown and possibly large
 *   This structure must be allocated with allocFCInfoVar() and initialized with
 *   InitFunctionCallInfoData().
 * FunctionCallInfoDataLarge:
 *   Used when the number of arguments is unknown, possibly large and
 *   the struct is embedded somewhere where a variable length is not acceptable
 *   This structure must be initialized with: InitFunctionCallInfoData()
 *
 * All structures have the same header and the arg/argnull fields shoule not be
 * accessed directly but via the below accessor macros.
 */
 typedef struct FunctionCallInfoData
{
FmgrInfo   *flinfo; /* ptr to lookup info used for this call */
fmNodePtr   context;/* pass info about context of call */
fmNodePtr   resultinfo; /* pass or return extra info about result */
Oid fncollation;/* collation for function to use */
boolisnull; /* function must set true if result is NULL */
boolisFixed;/* Must be true */
short   nargs;  /* # arguments actually passed */
Datum  *arg;/* pointer to function arg array */
bool   *argnull;/* pointer to null indicator array */
Datum   __arg[FUNC_MAX_ARGS_FIX]; /* Arguments passed to function */
bool__argnull[FUNC_MAX_ARGS_FIX]; /* T if arg[i] is actually NULL */
} FunctionCallInfoData;
  typedef struct FunctionCallInfoDataVariable
{
 FmgrInfo   *flinfo; /* ptr to lookup info used for this call */
fmNodePtr   context;/* pass info about context of call */
fmNodePtr   resultinfo; /* pass or return extra info about result */
Oid fncollation;/* collation for function to use */
boolisnull; /* function must set true if result is NULL */
boolisFixed;/* Must be false */
short   nargs;  /* # arguments actually passed */
Datum  *arg;/* pointer to function arg array */
bool   *argnull;/* pointer to null indicator array */
} FunctionCallInfoDataVariable;
 
 typedef struct FunctionCallInfoDataLarge
{
FmgrInfo   *flinfo; /* ptr to lookup info used for this call */
fmNodePtr   context;/* pass info about context of call */
fmNodePtr   resultinfo; /* pass or return extra info about result */
Oid fncollation;/* collation for function to use */
boolisnull; /* function must set true if result is NULL */
boolisFixed;/* Must be false */
short   nargs;  /* # arguments actually passed */
Datum  *arg;/* pointer to function arg array */
bool   *argnull;/* pointer to null indicator array */
Datum   __arg[FUNC_MAX_ARGS]; /* Arguments passed to function */
bool__argnull[FUNC_MAX_ARGS]; /* T if arg[i] is actually NULL */
} FunctionCallInfoDataLarge;

Re: [PATCH] Trim trailing whitespace in vim and emacs

[Tom Lane]

Alvaro Herrera  writes:
> On 2018-Jun-05, David Fetter wrote:
>> Is there any interest in such a feature?

> I'd rather have the editor warn me (highlight) such things rather than
> fix them silently (I wonder if it'd cause a mess with regression .out
> files for example, which I do edit on occasion).

Yeah, agreed.  FWIW, I'm not that fussed about this for .h/.c files,
since pgindent will fix it sooner or later (or even right away, if the
committer is anal enough to pgindent before committing, as some of us
are).  It's a bit more of a problem for other file types.

I note that Peter E. seems to have a recipe for finding such issues,
which I suspect is grounded in some obscure git feature or other.
That might be easier to work with, since you'd only need one fix
not one per editor.

regards, tom lane

Re: commitfest 2018-07

[Jonathan S. Katz]

> On Jun 5, 2018, at 10:14 AM, Tom Lane  wrote:
> 
> Michael Paquier  writes:
>> Okay.  If we tend toward this direction, I propose to do this switch in
>> two days my time (Thursday afternoon in Tokyo) if there are no
>> objections, so as anybody has hopefully time to argue back.
> 
> I think we probably have to wait longer.  It's not clear to me when the
> RMT will decide that the 07 fest is go or no-go, but surely they've
> not decided yet.

The RMT has its every-other-week catchup planned for tomorrow,
and we will discuss if we see any obstacles then and report back
posthaste.

Jonathan

Variable-length FunctionCallInfoData

[Andres Freund]

Hi,

While prototyping codegen improvements for JITed expression evaluation,
I once more hit the issue that the FunctionCallInfoData structs are
really large (936 bytes), despite arguments beyond the fourth barely
every being used.  I think we should fix that.

What I think we should do is convert
FunctionCallInfoData->{arg,argisnull} into an array of NullableDatum
(new type, a struct of Datum and bool), and then use a variable length
array for the arguments.  In the super common case of 2 arguments that
reduces the size of the array from 936 to 64 bytes.  Besides the size
reduction this also noticably reduces the number of cachelines accessed
- before it's absolutely guaranteed that the arg and argnull arrays for
the same argument aren't on the same cacheline, after it's almost
guaranteed to be the case.

Attached is a *PROTOTYPE* patch doing so.  Note I was too lazy to fully
fix up the jit code, I didn't want to do the legwork before we've some
agreement on this.  We also can get rid of FUNC_MAX_ARGS after this, but
there's surrounding code that still relies on it.

There's some added uglyness, which I hope we can polish a bit
further. Right now we allocate a good number of FunctionCallInfoData
struct on the stack - which doesn't quite work afterwards anymore.  So
the stack allocations, for the majoroity cases where the argument number
is known, currently looks like:

union {
FunctionCallInfoData fcinfo;
char *fcinfo_data[SizeForFunctionCallInfoData(0)];
} fcinfodata;
FunctionCallInfo fcinfo = 

that's not pretty, but also not that bad.

It's a bit unfortunate that this'll break some extensions, but I don't
really see a way around that.  The current approach, to me, clearly
doesn't have a future.  I wonder if we should add a bunch of accessor
macros / inline functions that we (or extension authors) can backpatch
to reduce the pain of maintaining different code paths.

Besides the change here, I think we should also go much further with the
conversion to NullableDatum.  There's two main areas of change: I want
to move the execExpr.c related code so steps return data into
NullableDatums - that removes a good chunk of pointer dereferences and
allocations. Secondly I think we should move TupleTableSlot to this
format - the issue with nulls / datums being on separate cachelines is
noticeable in profiles, but more importantly the code looks more
consistent with it.


As an example for the difference in memory usage, here's the memory
consumption at ExecutorRun time, for TPCH's Q01:

master:
TopPortalContext: 8192 total in 1 blocks; 7664 free (0 chunks); 528 used
  PortalContext: 1024 total in 1 blocks; 576 free (0 chunks); 448 used:
ExecutorState: 90744 total in 5 blocks; 31568 free (2 chunks); 59176 used
  ExprContext: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
  ExprContext: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
  ExprContext: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
  ExprContext: 8192 total in 1 blocks; 3488 free (0 chunks); 4704 used
  ExprContext: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
  ExprContext: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
Grand total: 149112 bytes in 13 blocks; 82976 free (2 chunks); 66136 used

patch:
TopPortalContext: 8192 total in 1 blocks; 7664 free (0 chunks); 528 used
  PortalContext: 1024 total in 1 blocks; 576 free (0 chunks); 448 used:
ExecutorState: 65536 total in 4 blocks; 33536 free (6 chunks); 32000 used
  ExprContext: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
  ExprContext: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
  ExprContext: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
  ExprContext: 8192 total in 1 blocks; 5408 free (0 chunks); 2784 used
  ExprContext: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
  ExprContext: 8192 total in 1 blocks; 7936 free (0 chunks); 256 used
Grand total: 123904 bytes in 12 blocks; 86864 free (6 chunks); 37040 used

As you can see, the ExecutorState context uses nearly half the amount of
memory as before. In a lot of cases a good chunk of the benefit is going
to be hidden due to memory context sizing, but I'd expect that to matter
much less for more complex statements and plpgsql functions etc.

Comments?

Greetings,

Andres Freund
>From 50ac86917da52cbab61cffaabb1d443184b3aa87 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Tue, 5 Jun 2018 10:24:31 -0700
Subject: [PATCH v1] Variable length FunctionCallInfoData

Author: Andres Freund
Reviewed-By:
Discussion: https://postgr.es/m/
Backpatch:
---
 src/backend/commands/event_trigger.c  |  12 +-
 src/backend/commands/functioncmds.c   |  16 +-
 src/backend/commands/tablecmds.c  |  12 +-
 src/backend/commands/trigger.c|  16 +-
 src/backend/executor/execExpr.c   |  81 +--
 src/backend/executor/execExprInterp.c | 131 ++--
 src/backend/executor/execSRF.c   

Re: Re: Spilling hashed SetOps and aggregates to disk

[Jeff Davis]

On Tue, 2018-06-05 at 08:39 -0700, se...@rielau.com wrote:
> Strange. We don't see this overeahd and measure a lot more than just
> a single metric:

Let's investigate again. I wasn't able to repro the overhead on x86;
Robert saw it on a POWER machine, and it was somewhat noisy. I don't
think we were ever very sure the overhead existed.

My basic opinion is that we make small changes all the time that may
have a small performance impact, and we shouldn't let that become a
blocker for an important feature. Nor should we let it make the design
overly complicated or awkward. We should just see what we can
reasonably do to understand and mitigate it.

Regards,
Jeff Davis

Re: [PATCH] Trim trailing whitespace in vim and emacs

[Alvaro Herrera]

On 2018-Jun-05, David Fetter wrote:

Hi David

> Here's my attempt to $subject. I've tested with vim, but I'm much less
> certain I got the EMACS code right.
> 
> Is there any interest in such a feature?

I'd rather have the editor warn me (highlight) such things rather than
fix them silently (I wonder if it'd cause a mess with regression .out
files for example, which I do edit on occasion).

I once tried to have vim highlight trailing whitespace, spaces before
tabs, and overlength lines (>80 chars), and while I could do each thing
in isolation, I wasn't able to get it to highlight the three of them at
the same time.  If you have a recipe for that, I welcome it.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Spilling hashed SetOps and aggregates to disk

[Tom Lane]

David Fetter  writes:
> On Tue, Jun 05, 2018 at 02:56:23PM +1200, David Rowley wrote:
>> True. Although not all built in aggregates have those defined.

> Just out of curiosity, which ones don't? As of
> 3f85c62d9e825eedd1315d249ef1ad793ca78ed4, pg_aggregate has both of
> those as NOT NULL.

NOT NULL isn't too relevant; that's just protecting the fixed-width
nature of the catalog rows.  What's important is which ones are zero.

# select aggfnoid::regprocedure, aggkind from pg_aggregate where (aggserialfn=0 
or aggdeserialfn=0) and aggtranstype = 'internal'::regtype;
   aggfnoid   | aggkind 
--+-
 array_agg(anynonarray)   | n
 array_agg(anyarray)  | n
 string_agg(text,text)| n
 string_agg(bytea,bytea)  | n
 json_agg(anyelement) | n
 json_object_agg("any","any") | n
 jsonb_agg(anyelement)| n
 jsonb_object_agg("any","any")| n
 percentile_disc(double precision,anyelement) | o
 percentile_cont(double precision,double precision)   | o
 percentile_cont(double precision,interval)   | o
 percentile_disc(double precision[],anyelement)   | o
 percentile_cont(double precision[],double precision) | o
 percentile_cont(double precision[],interval) | o
 mode(anyelement) | o
 rank("any")  | h
 percent_rank("any")  | h
 cume_dist("any") | h
 dense_rank("any")| h
(19 rows)

Probably the ordered-set/hypothetical ones aren't relevant for this
issue.

Whether or not we feel like fixing the above "normal" aggs for this,
the patch would have to not fail on extension aggregates that don't
support serialization.

regards, tom lane

[PATCH] Trim trailing whitespace in vim and emacs

[David Fetter]

Folks,

Here's my attempt to $subject. I've tested with vim, but I'm much less
certain I got the EMACS code right.

Is there any interest in such a feature?

Best,
David.
---
 .dir-locals.el| 3 ++-
 src/tools/editors/vim.samples | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/.dir-locals.el b/.dir-locals.el
index 9525d6b604..858461e6bd 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -4,7 +4,8 @@
 (c-file-style . "bsd")
 (fill-column . 78)
 (indent-tabs-mode . t)
-(tab-width . 4)))
+(tab-width . 4)
+   (lambda () (add-to-list 'write-file-functions 
'delete-trailing-whitespace)
  (dsssl-mode . ((indent-tabs-mode . nil)))
  (nxml-mode . ((indent-tabs-mode . nil)))
  (perl-mode . ((perl-indent-level . 4)
diff --git a/src/tools/editors/vim.samples b/src/tools/editors/vim.samples
index ccbc93f69b..f5e2181bc3 100644
--- a/src/tools/editors/vim.samples
+++ b/src/tools/editors/vim.samples
@@ -11,6 +11,7 @@
 :  set cinoptions=(0
 :  set tabstop=4
 :  set shiftwidth=4
+:  autocmd FileType asm,c,conf,sql,xml,xs,xslt,yacc autocmd BufWritePre 
 %s/\s\+$//e
 
 :endif
 
-- 
2.17.1


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

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

Re: Spilling hashed SetOps and aggregates to disk

[Andres Freund]

On 2018-06-05 19:04:11 +0200, David Fetter wrote:
> On Tue, Jun 05, 2018 at 02:56:23PM +1200, David Rowley wrote:
> > On 5 June 2018 at 06:52, Andres Freund  wrote:
> > > That part has gotten a bit easier since, because we have serialize
> > > / deserialize operations for aggregates these days.
> > 
> > True. Although not all built in aggregates have those defined.
> 
> Just out of curiosity, which ones don't? As of
> 3f85c62d9e825eedd1315d249ef1ad793ca78ed4, pg_aggregate has both of
> those as NOT NULL.

That doesn't mean much. We commonly store 0 / InvalidOid for such things
in the catalog. Allows us to still map the tuples to structs (due to
fixed width).

Greetings,

Andres Freund

Re: I'd like to discuss scaleout at PGCon

[MauMau]

From: Merlin Moncure
> FWIW, Distributed analytical queries is the right market to be in.
> This is the field in which I work, and this is where the action is
at.
> I am very, very, sure about this.  My view is that many of the
> existing solutions to this problem (in particular hadoop class
> soltuions) have major architectural downsides that make them
> inappropriate in use cases that postgres really shines at; direct
> hookups to low latency applications for example.  postgres is
> fundamentally a more capable 'node' with its multiple man-millennia
of
> engineering behind it.  Unlimited vertical scaling (RAC etc) is
> interesting too, but this is not the way the market is moving as
> hardware advancements have reduced or eliminated the need for that
in
> many spheres.

I'm feeling the same.  As the Moore's Law ceases to hold, software
needs to make most of the processor power.  Hadoop and Spark are
written in Java and Scala.  According to Google [1] (see Fig. 8), Java
is slower than C++ by 3.7x - 12.6x, and Scala is slower than C++ by
2.5x - 3.6x.

Won't PostgreSQL be able to cover the workloads of Hadoop and Spark
someday, when PostgreSQL supports scaleout, in-memory database,
multi-model capability, and in-database filesystem?  That may be a
pipedream, but why do people have to tolerate the separation of the
relational-based data  warehouse and Hadoop-based data lake?


[1]Robert Hundt. "Loop Recognition in C++/Java/Go/Scala".
Proceedings of Scala Days 2011

Regards
MauMau

Re: Spilling hashed SetOps and aggregates to disk

[David Fetter]

On Tue, Jun 05, 2018 at 02:56:23PM +1200, David Rowley wrote:
> On 5 June 2018 at 06:52, Andres Freund  wrote:
> > That part has gotten a bit easier since, because we have serialize
> > / deserialize operations for aggregates these days.
> 
> True. Although not all built in aggregates have those defined.

Just out of curiosity, which ones don't? As of
3f85c62d9e825eedd1315d249ef1ad793ca78ed4, pg_aggregate has both of
those as NOT NULL.

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

Re: Remove mention in docs that foreign keys on partitioned tables are not supported

[Alvaro Herrera]

On 2018-Jun-05, Amit Langote wrote:

> On 2018/06/05 16:41, Ashutosh Bapat wrote:
> > On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote
> >  wrote:
> >> On 2018/06/05 1:25, Alvaro Herrera wrote:
> >>> In the meantime, my inclination is to fix the documentation to point out
> >>> that AFTER triggers are allowed but BEFORE triggers are not.
> >>
> >> Wasn't that already fixed by bcded2609ade6?

Ah, yes, so it's already OK.

> > Thought correct that suggestion is problematic for upgrades. Probably
> > users will create triggers using same function on all the partitions.
> > After the upgrade when we start supporting BEFORE TRIGGER on
> > partitioned table, the user will have to drop these triggers and
> > create one trigger on the partitioned table to have the trigger to be
> > applicable to the new partitions created.
> 
> Hmm yes, maybe there is scope for rewording then.

Reading that subsection in its entirety, I'm not sure how to do better
-- it's all about restrictions that currently exist and we think we
could do better in the future, with the exception of the one about an
UPDATE/DELETE not seeing the updated version after a row migrating to
another partition.  One idea would be to split it into its own list of
issues; something like:

"The following limitations apply, and might be lifted in the future:
 - no way to create exclusion constraint
 - foreign keys cannot refer to partitioned tables
 - BEFORE row triggers are not supported

The following caveat applies to partitioned tables:
 - When an UPDATE causes a row to move ..."

In other words, make a distinction of things we expect to change soon
(which might be too optimistic), vs. others we don't expect to change.

Or we could leave it alone; any behavior change would be called out in
the future version's release notes anyway.  This is what I propose.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: libpq compression

[Peter Eisentraut]

On 6/5/18 03:09, Michael Paquier wrote:
> I just had a quick look at this patch, lured by the smell of your latest
> messages...  And it seems to me that this patch needs a heavy amount of
> work as presented.  There are a couple of things which are not really
> nice, like forcing the presentation of the compression option in the
> startup packet to begin with.

Yeah, at this point we will probably need a discussion and explanation
of the protocol behavior this is adding, such as how to negotiate
different compression settings.

Unrelatedly, I suggest skipping the addition of -Z options to various
client-side tools.  This is unnecessary, since generic connection
options can already be specified via -d typically, and it creates
confusion because -Z is already used to specify output compression by
some programs.

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

Re: I'd like to discuss scaleout at PGCon

[MauMau]

From: Ashutosh Bapat
> Each node need to be confiugred and maintained. That requires
efforts.
> So we need to keep the number of nodes to a minimum. With a
> coordinator and worker node segregation, we require at least two
nodes
> in a cluster and just that configuration doesn't provide much
> scalability. With each node functioning as coordinator (facing
> clients) and worker (facing other coordinators) keeps the number of
> nodes to a minimum. It is good for HA.

I think so, too.  Every node should behave as both the coordinator and
the data node in XL parlance.  But I don't preclude a central node.
Some node needs to manage sequences, and it may as well manage the
system catalog.


Regards
MauMau

Re: I'd like to discuss scaleout at PGCon

[MauMau]

From: Ashutosh Bapat
> In order to normalize parse trees, we need to at
> least replace various OIDs in parse-tree with something that the
> foreign server will understand correctly like table name on the
> foreign table pointed to by local foreign table OR (schema
qualified)
> function names  and so on.

Yes, that's the drawback of each node in the cluster having different
OIDs for the same object.  That applies to XL, too.  In XL, the data
node returns the data type names of the columns in the result set to
the coordinator.  Then the coordinator seemed to parse each data type
name with base_yyparse() to convert the name to its OID on the
coordinator.  That's why base_yyparse() appeared at the top in the
perf profile.  That repeated typename-to-OID conversion would be
reduced by caching the conversion result, like the logical replication
of PostgreSQL does.  But managing the catalog at one place and using
the same OID values seems to concise to me as a concept.


Regards
MauMau


-Original Message- 
From: Ashutosh Bapat
Sent: Saturday, June 2, 2018 1:00 AM
To: Tom Lane
Cc: MauMau ; Robert Haas ; PostgreSQL Hackers
Subject: Re: I'd like to discuss scaleout at PGCon

On Fri, Jun 1, 2018 at 11:27 AM, Tom Lane  wrote:
> Ashutosh Bapat  writes:
>> In order to avoid double parsing, we might want to find a way to
pass
>> a "normalized" parse tree down to the foreign server. We need to
>> normalize the OIDs in the parse tree since those may be different
>> across the nodes.
>
> I don't think this is a good idea at all.  It breaks any hope of
> supporting remote servers that are not the identical version to the
local
> one (since their parsetrees might be different).  And "normalized
OIDs"
> sounds like "pie in the sky".  You might get away with asssuming
that
> built-in functions have stable OIDs, but you can't expect that for
> functions in extensions.

Sorry for confusing writeup. I didn't mean "normalized OIDs" as I
mentioned in my last sentence. I meant "normalized parse-tree" as in
the first sentence. In order to normalize parse trees, we need to at
least replace various OIDs in parse-tree with something that the
foreign server will understand correctly like table name on the
foreign table pointed to by local foreign table OR (schema qualified)
function names  and so on. There might be more things to "normalize"
in the parse tree other than OIDs, but I can't think of anything right
now.


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

Re: I'd like to discuss scaleout at PGCon

[MauMau]

From: Ashutosh Bapat
> In order to normalize parse trees, we need to at
> least replace various OIDs in parse-tree with something that the
> foreign server will understand correctly like table name on the
> foreign table pointed to by local foreign table OR (schema
qualified)
> function names  and so on.

Yes, that's the drawback of each node in the cluster having different
OIDs for the same object.  That applies to XL, too.  In XL, the data
node returns the data type names of the columns in the result set to
the coordinator.  Then the coordinator seemed to parse each data type
name with base_yyparse() to convert the name to its OID on the
coordinator.  That's why base_yyparse() appeared at the top in the
perf profile.  That repeated typename-to-OID conversion would be
reduced by caching the conversion result, like the logical replication
of PostgreSQL does.  But managing the catalog at one place and using
the same OID values seems to concise to me as a concept.


Regards
MauMau

Re: [PATCH] Improve geometric types

[Emre Hasegeli]

> Those underscore-prefixed names are defined in Microsoft's
> [3][4].  So now I'm wondering if win32_port.h needs to
> #include  if (_MSC_VER < 1800).

I don't have the C experience to decide the correct way.  There are
currently many .c files that are including float.h conditionally or
unconditionally.  The condition they use is "#ifdef _MSC_VER" without
a version.

One idea is to include float.h from the new utils/float.h file
together with math.h, and remove those includes from the .c files
which would include utils/float.h.  We can do this only, or together
with what you suggest, or by also keeping the includes on the .c
files.  Which way do you think is the proper?

Re: buildfarm vs code

[Andrew Dunstan]

On 06/05/2018 12:09 PM, Andrew Dunstan wrote:


notice at lines 36 and 34 or the Appveyor output, (from lines 16 and 
19 of the recipe appveyor.yml) that this recipe does make a couple of 
adjustments.



Of course that's lines 36 and 43. I seem to be getting more dyslectic as 
I get older ...


cheers

andrew

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

Re: I'd like to discuss scaleout at PGCon

[MauMau]

From: Robert Haas
On Thu, May 31, 2018 at 8:12 AM, MauMau  wrote:
>> Oh, I didn't know you support FDW approach mainly for analytics.  I
>> guessed the first target was OLTP read-write scalability.
>
> That seems like a harder target to me, because you will have an
extra
> hop involved -- SQL from the client to the first server, then via
SQL
> to a second server.  The work of parsing and planning also has to be
> done twice, once for the foreign table and again for the table.  For
> longer-running queries this overhead doesn't matter as much, but for
> short-running queries it is significant.


From: Simon Riggs
On 1 June 2018 at 04:00, MauMau  wrote:
>> The SQL processor should be one layer, not two layers.

> For OLTP, that would be best. But it would be restricted to
> single-node requests, leaving you the problem of how you know ahead
of
> time whether an SQL statement was single node or not.
>
> Using a central coordinator node allows us to hide the decision of
> single-node/multi-node from the user which seems essential for
general
> SQL. If you are able to restrict the types of requests users make
then
> we can do direct access to partitions - so there is scope for a
> single-node API, as Mongo provides.

I don't think an immediate server like the coordinators in XL is
necessary.  That extra hop can be eliminated by putting both the
coordinator and the data node roles in the same server process.  That
is, the node to which an application connects communicates with other
nodes only when it does not necessary data.

Furthermore, an extra hop and double parsing/planning could matter for
analytic queries, too.  For example, SAP HANA boasts of scanning 1
billion rows in one second.  In HANA's scaleout architecture, an
application can connect to any worker node and the node communicates
with other nodes only when necessary (there's one special node called
"master", but it manages the catalog and transactions; it's not an
extra hop like the coordinator in XL).  Vertica is an MPP analytics
database, but it doesn't have a node like the coordinator, either.  To
achieve maximum performance for real-time queries, the scaleout
architecture should avoid an extra hop when possible.


> Using a central coordinator also allows multi-node transaction
> control, global deadlock detection etc..

VoltDB does not have an always-pass hop like the coordinator in XL.
Our proprietary RDBMS named Symfoware, which is not based on
PostgreSQL, also doesn't have an extra hop, and can handle distributed
transactions and deadlock detection/resolution without any special
node like GTM.


Regards
MauMau

buildfarm vs code

[Andrew Dunstan]

At my talk at pgcon last Friday [1] I presented some ideas for how 
people could run a full buildfarm run against their code, including a 4 
line recipe using some Docker recipes I had created. Thomas Munro 
suggested it would be even nicer if you could just point something like 
Appveypr at the code and have it do the buildfarm run. That intrigued me 
a bit, so I created a recipe for that. Along the way there are a few 
adjustments to how the buildfarm client runs, which is why the recipe 
[2] runs with a git clone rather than a published release. Nevertheless 
it does seem to work [3]


However, it has made two adjustments to the perl source, which is rather 
against the philosophy of the buildfarm. The buildfarm client does not 
ever make any adjustments or patches to postgres code, and in normal 
reporting operation refuses to run if it detects any such changes. 
However, notice at lines 36 and 34 or the Appveyor output, (from lines 
16 and 19 of the recipe appveyor.yml) that this recipe does make a 
couple of adjustments.


The first should be simple and non-controversial. It allows 
src/tools/msvc/build.pl to be called in such a way that it only creates 
the project files and then stops. This is a one line addition to the 
script and should affect nobody not using the option. A patch for it is 
attached.


The second change the recipe makes is to disable the tablespace tests. 
On Windows, when running as the Administrator, the application gives up 
privilege to such an extent that the tablespace tests fail. The answer 
is usually to run as a non-Admin user, and this is what I do for animals 
like jacana and bowerbird. However, setting that up so that it hooks up 
nicely to the appveyor console is ugly and fragile. So I'd like either 
to solve this issue (perhaps be more discriminating about what 
privileges we give up) or provide a builtin way to skip these tests. In 
the latter case, maybe a skip-tests setting for pg_regress would work 
well. I can imagine other uses for it ("I know I'm going to break this 
test but I want to run all the others.")



cheers


andrew


[1] http://www.pgcon.org/2018/schedule/events/1183.en.html

[2] https://github.com/PGBuildFarm/appveyor-build

[3] https://ci.appveyor.com/project/AndrewDunstan/pgdevel-6sfth/build/1.0.40

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

diff --git a/src/tools/msvc/build.pl b/src/tools/msvc/build.pl
index 9a234d1..3760b56 100644
--- a/src/tools/msvc/build.pl
+++ b/src/tools/msvc/build.pl
@@ -37,6 +37,9 @@ do "config.pl" if (-f "src/tools/msvc/config.pl");
 
 my $vcver = Mkvcbuild::mkvcbuild($config);
 
+# Check if we only want the project files. If so stop now.
+exit 0 if ($ARGV[0] && $ARGV[0] eq 'ProjectOnly');
+
 # check what sort of build we are doing
 
 my $bconf = $ENV{CONFIG}   || "Release";

Re: Code of Conduct plan

[Magnus Hagander]

On Tue, Jun 5, 2018 at 4:45 PM, Chris Travers 
wrote:

>
>
> On Mon, Jun 4, 2018 at 6:59 PM, Joshua D. Drake 
> wrote:
>
>> On 06/03/2018 04:08 PM, Gavin Flower wrote:
>>
>> My comments:

 1) Reiterate my contention that this is a solution is search of
 problem. Still it looks like it is going forward, so see below.

 2) "... engaging in behavior that may bring the PostgreSQL project into
 disrepute, ..."
 This to me is overly broad and pulls in actions that may happen outside
 the community. Those if they are actually an issue should be handled where
 they occur not here.

>>>
>> This is good point. There are those who would think that one has
>> performed an action that brings the project into disrepute and a similar
>> sized bias that suggests that in fact that isn't the case. This based on
>> the CoC would be judged by the CoC committee.
>>
>> It is my hope that PostgreSQL.Org -Core chooses members for that
>> committee that are exceedingly diverse otherwise it is just an echo chamber
>> for a single ideology and that will destroy this community.
>
>
> If I may suggest:  The committee should be international as well and
> include people from around the world.  The last thing we want is for it to
> be dominated by people from one particular cultural viewpoint.
>

It will be. This is the PostgreSQL *global* development group and project,
after all. Yes, there is definitely a slant in the project in general
towards the US side, as is true in many other such projects, but in general
we have decent coverage of other cultures and countries as well. We can't
cover them all  on the committee (that would make for a gicantic
committee), but we can cover it with people who are used to communicating
and working with people from other areas as well, which makes for a better
understanding.

It won't be perfect in the first attempt, of course, but that one is
covered.

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

Re: libpq compression

[Konstantin Knizhnik]

On 05.06.2018 10:09, Michael Paquier wrote:

On Tue, Jun 05, 2018 at 06:04:21PM +1200, Thomas Munro wrote:

On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik
Speaking of configuration, are you planning to support multiple
compression libraries at the same time?  It looks like the current
patch implicitly requires client and server to use the same configure
option, without any attempt to detect or negotiate.  Do I guess
correctly that a library mismatch would produce an incomprehensible
corrupt stream message?

I just had a quick look at this patch, lured by the smell of your latest
messages...  And it seems to me that this patch needs a heavy amount of
work as presented.  There are a couple of things which are not really
nice, like forcing the presentation of the compression option in the
startup packet to begin with.  The high-jacking around secure_read() is
not nice either as it is aimed at being a rather high-level API on top
of the method used with the backend.  On top of adding some
documentation, I think that you could get some inspiration from the
recent GSSAPI encription patch which has been submitted again for v12
cycle, which has spent a large amount of time designing its set of
options.
--
Michael

Thank you for feedback,
I have considered this patch mostly as prototype to estimate efficiency 
of libpq  protocol compression and compare it with SSL compression.

So I agree with you that there are a lot of things which should be improved.

But can you please clarify what is wrong with "forcing the presentation 
of the compression option in the startup packet to begin with"?
Do you mean that it will be better to be able switch on/off compression 
during session?


Also I do not completely understand what do you mean by "high-jacking 
around secure_read()".

I looked at GSSAPI patch. It does injection in secure_read:

+#ifdef ENABLE_GSS
+    if (port->gss->enc)
+    {
+        n = be_gssapi_read(port, ptr, len);
+        waitfor = WL_SOCKET_READABLE;
+    }
+    else

But the main difference between encryption and compression is that 
encryption is not changing data size, while compression does.
To be able to use streaming compression, I need to specify some function 
for reading data from the stream. I am using secure_read for this purpose:


       PqStream = zpq_create((zpq_tx_func)secure_write, 
(zpq_rx_func)secure_read, MyProcPort);


Can you please explain what is the problem with it?

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Spilling hashed SetOps and aggregates to disk

[David Fetter]

On Tue, Jun 05, 2018 at 02:56:23PM +1200, David Rowley wrote:
> On 5 June 2018 at 06:52, Andres Freund  wrote:
> > That part has gotten a bit easier since, because we have serialize /
> > deserialize operations for aggregates these days.
> 
> True. Although not all built in aggregates have those defined.

That is a SMoP which we could, at some point, enforce by requiring
that they always be defined.  Is there something other than round
tuits that's preventing that?

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

Re: why partition pruning doesn't work?

[Pavel Stehule]

2018-06-05 17:07 GMT+02:00 David Rowley :

> On 5 June 2018 at 22:31, Amit Langote 
> wrote:
> > Maybe, David (added to cc) has something to say about all this,
> especially
> > whether he considers this a feature and not a bug fix.
>
> Thanks, Amit. I had missed this thread.
>
> Yeah. I admit if I'd thought about this case when I wrote the code,
> then I'd have made any non-volatile Expr work, but I didn't :-(
>
> It was pointed out to be a few months ago in a comment in [1]. I
> initially thought that this was v12 material, but it seems there are a
> few people here that are pretty unhappy about it.
>
> I was going to describe what such a patch should look like here, but
> that seemed like about as much work as writing it, so:
>
> Please see the attached patch. I've only just finished with it and
> it's not fully done yet as there's still an XXX comment where I've not
> quite thought about Exprs with Vars from higher levels. These might
> always be converted to Params, so the code might be okay as is, but
> I've not checked this yet, hence the comment remains.
>
> I'm slightly leaning towards this being included in v11. Without this
> people are forced into hacks like WHERE partkey = (SELECT
> stablefunc()); to get pruning working at all. If that SQL remains
> after this patch then pruning can only take place during actual
> execution. With the attached patch the pruning can take place during
> the initialization of the executor, which in cases with many
> partitions can be significantly faster, providing actual execution is
> short.  I'd rather people didn't get into bad habits like that if we
> can avoid it.
>

This is really great

Regards

Pavel


> [1] https://blog.2ndquadrant.com/partition-elimination-postgresql-11/
>
> --
>  David Rowley   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>

Re: Code of Conduct plan

[Joshua D. Drake]

On 06/05/2018 07:45 AM, Chris Travers wrote:


It is my hope that PostgreSQL.Org -Core chooses members for that
committee that are exceedingly diverse otherwise it is just an echo
chamber for a single ideology and that will destroy this community.


If I may suggest:  The committee should be international as well and 
include people from around the world.  The last thing we want is for it 
to be dominated by people from one particular cultural viewpoint.




+1



"considered offensive by fellow members"

Is definitely too broad. The problem comes in here:

I might possibly say that "I'm the master in this area" when
talking to someone on a technical subject.  In the sense that
I'm better at that particular skill, but some hypersensitive
American could get their knickers in a twist (notice, that in
this context, no gender is implied -- also in using that that
expression "get their knickers in a twist" could offend some
snowflake) claiming that I'm suggesting that whoever


"snowflake", I find that term hilarious others find it highly
offensive. Which is correct?


I agree with both concerns in the above exchange.

This is an economic common project.  The goal should be for people to 
come together and act civilly.  Waging culture war using the code of 
conduct itself should be a violation of the code of conduct and this 
goes on *all* (not just one or two) sides.




[snip]



Yes and that is a problem. We need to have some simple barrier of
acceptance that we are all adults here (or should act like adults).
Knowing your audience is important.


I would point out also that the PostgreSQL community is nice and 
mature.  At PGConf US I saw what appeared to be two individuals with red 
MAGA hats.  And yet everyone managed to be civil.  We manage to do 
better than the US does on the whole in this regard and we should be 
proud of ourselves.


To be fair, those were South Africans but yes, nobody gave them any 
public grief as far as I know.




Correct. I think one way to look at all of this is, "if you wouldn't
say it to your boss or a client don't say it here". That too has
problems but generally speaking I think it keeps the restrictions
rational.


I will post a more specific set of thoughts here but in general I think 
the presumption ought to be that people are trying to work together.  
Misunderstanding can happen.  But let's try to act in a collegial and 
generally respectful way around eachother.


+1

JD




--
Command Prompt, Inc. || http://the.postgres.company/ || @cmdpromptinc
***  A fault and talent of mine is to tell it exactly how it is.  ***
PostgreSQL centered full stack support, consulting and development.
Advocate: @amplifypostgres || Learn: https://postgresconf.org
* Unless otherwise stated, opinions are my own.   *

location reporting in TAP test failures

[Peter Eisentraut]

Right now, when a TAP test reports a failure, it looks something like this:

 #   Failed test 'creating a replication slot'
 #   at
/../postgresql/src/bin/pg_basebackup/../../../src/test/perl/TestLib.pm
line 371.

That file location is where we call out to the test function provided by
Test::More.

What we'd really want is

 #   Failed test 'creating a replication slot'
 #   at t/020_pg_receivewal.pl line 36.

because that's where the code that's doing the testing is.

To achieve that, we need to have our test library functions tell that
they are support functions and not the actual tests.  The attached patch
does that.  The mechanism is (somewhat) explained in the Test::Builder
man page.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 54113ae8d1921cdd5b161e2bc3cbfd31b24bb4d2 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Tue, 22 May 2018 14:25:01 -0400
Subject: [PATCH] Use $Test::Builder::Level in TAP test functions

In TAP test functions, that is, those that produce test results, locally
increment $Test::Builder::Level.  This has the effect that test failures
are reported at the callers location rather than somewhere in the test
support libraries.
---
 src/bin/pg_rewind/RewindTest.pm |  2 ++
 src/test/perl/PostgresNode.pm   | 10 ++
 src/test/perl/TestLib.pm| 11 +++
 src/test/ssl/ServerSetup.pm |  4 
 4 files changed, 27 insertions(+)

diff --git a/src/bin/pg_rewind/RewindTest.pm b/src/bin/pg_rewind/RewindTest.pm
index 60b54119e7..057b08f9a4 100644
--- a/src/bin/pg_rewind/RewindTest.pm
+++ b/src/bin/pg_rewind/RewindTest.pm
@@ -87,6 +87,8 @@ sub standby_psql
 # expected
 sub check_query
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($query, $expected_stdout, $test_name) = @_;
my ($stdout, $stderr);
 
diff --git a/src/test/perl/PostgresNode.pm b/src/test/perl/PostgresNode.pm
index d12dd60e73..4475eda001 100644
--- a/src/test/perl/PostgresNode.pm
+++ b/src/test/perl/PostgresNode.pm
@@ -1366,6 +1366,8 @@ PostgresNode.
 
 sub command_ok
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my $self = shift;
 
local $ENV{PGPORT} = $self->port;
@@ -1384,6 +1386,8 @@ TestLib::command_fails with our PGPORT. See 
command_ok(...)
 
 sub command_fails
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my $self = shift;
 
local $ENV{PGPORT} = $self->port;
@@ -1402,6 +1406,8 @@ TestLib::command_like with our PGPORT. See command_ok(...)
 
 sub command_like
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my $self = shift;
 
local $ENV{PGPORT} = $self->port;
@@ -1420,6 +1426,8 @@ TestLib::command_checks_all with our PGPORT. See 
command_ok(...)
 
 sub command_checks_all
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my $self = shift;
 
local $ENV{PGPORT} = $self->port;
@@ -1442,6 +1450,8 @@ The log file is truncated prior to running the command, 
however.
 
 sub issues_sql_like
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
+
my ($self, $cmd, $expected_sql, $test_name) = @_;
 
local $ENV{PGPORT} = $self->port;
diff --git a/src/test/perl/TestLib.pm b/src/test/perl/TestLib.pm
index 77499c01e9..7fd27ec247 100644
--- a/src/test/perl/TestLib.pm
+++ b/src/test/perl/TestLib.pm
@@ -366,6 +366,7 @@ sub check_pg_config
 #
 sub command_ok
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd, $test_name) = @_;
my $result = run_log($cmd);
ok($result, $test_name);
@@ -374,6 +375,7 @@ sub command_ok
 
 sub command_fails
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd, $test_name) = @_;
my $result = run_log($cmd);
ok(!$result, $test_name);
@@ -382,6 +384,7 @@ sub command_fails
 
 sub command_exit_is
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd, $expected, $test_name) = @_;
print("# Running: " . join(" ", @{$cmd}) . "\n");
my $h = IPC::Run::start $cmd;
@@ -404,6 +407,7 @@ sub command_exit_is
 
 sub program_help_ok
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd) = @_;
my ($stdout, $stderr);
print("# Running: $cmd --help\n");
@@ -417,6 +421,7 @@ sub program_help_ok
 
 sub program_version_ok
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd) = @_;
my ($stdout, $stderr);
print("# Running: $cmd --version\n");
@@ -430,6 +435,7 @@ sub program_version_ok
 
 sub program_options_handling_ok
 {
+   local $Test::Builder::Level = $Test::Builder::Level + 1;
my ($cmd) = @_;
my ($stdout, $stderr);
print("# Running: $cmd --not-a-valid-option\n");
@@ -443,6 +449,7 @@ sub program_options_handling_ok
 
 sub command_like
 

Re: commitfest 2018-07

[David Rowley]

On 6 June 2018 at 03:17, Andres Freund  wrote:
> On 2018-06-06 03:14:58 +1200, David Rowley wrote:
>> On 6 June 2018 at 02:20, Tom Lane  wrote:
>> > I thought the idea was to clear out the underbrush of small, ready-to-go
>> > patches.  How old they are doesn't enter into that.
>>
>> I don't recall a 'fest where small ready to go patches getting
>> anything but priority.
>
> I'm not following what you mean. Tom's referencing a discussion at the
> developer meeting last week...

I mean that if the idea was to use the July 'fest to just process the
small patches that are ready to go, then I didn't see the point as
those ones are generally the first to get looked at anyway.  It's the
big patches that take the time.  Getting rid of the smaller ones first
has always made sense since it reduces the list and admin time
maintaining the status page.

I personally think that some of the trouble we have on the final 'fest
is due to 4 fests not being enough for larger patches and people (I'll
include myself) are often stubborn to concede to waiting another year.
If larger patches can get looked at even earlier, then perhaps the
situation will be slightly better in March 2019.


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

Re: commitfest 2018-07

[Peter Eisentraut]

On 6/5/18 09:12, Andres Freund wrote:
> I'd rather create a new 2018-07, and just manually move old patches to
> it.

My concern is whether the commitfest app will handle that well.  There
is no "move to previous commit fest" button.  So you'd have to do it in
some evil way, possibly confusing the continuity of the patch records.
There might be other issues of this sort.  I don't think this use case
is fully worked out.

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

Re: commitfest 2018-07

[Andres Freund]

On 2018-06-06 03:14:58 +1200, David Rowley wrote:
> On 6 June 2018 at 02:20, Tom Lane  wrote:
> > I thought the idea was to clear out the underbrush of small, ready-to-go
> > patches.  How old they are doesn't enter into that.
> 
> I don't recall a 'fest where small ready to go patches getting
> anything but priority.

I'm not following what you mean. Tom's referencing a discussion at the
developer meeting last week...

Greetings,

Andres Freund

Re: commitfest 2018-07

[David Rowley]

On 6 June 2018 at 02:20, Tom Lane  wrote:
> I thought the idea was to clear out the underbrush of small, ready-to-go
> patches.  How old they are doesn't enter into that.

I don't recall a 'fest where small ready to go patches getting
anything but priority.

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

Re: commitfest 2018-07

[Stephen Frost]

Greetings,

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Joe Conway  writes:
> > On 06/05/2018 10:43 AM, Andres Freund wrote:
> >> I think we've not fully agreed on that.  I'd argue we should manually
> >> filter things into the next CF. And both small patches and older things
> >> should qualify.
> 
> > Would it work to rename 2018-09 to 2018-07 and then make a pass through
> > and move some of the entries to the next commitfest right away? That
> > seems easier than the alternative unless you think less than half of
> > what is there should be in 2018-07.
> 
> Either way, we need some consensus on which patches are not going to be
> considered in 07.

I have a pretty hard time believing that we're going to actually manage
to pull this off, and the argument against larger patches going in
during the July commitfest was largely shot down during the dev meeting
anyway.

Let's keep the tech side of this simple and just do the rename as
suggested and then we can encourage committers to review the
smaller/older patches by providing information about the objective size
and age of them, which will likely lead to the same result without all
the fuss over what patch should be in what commitfest.

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: commitfest 2018-07

[Andrew Dunstan]

On 06/05/2018 10:43 AM, Andres Freund wrote:

On 2018-06-05 10:20:55 -0400, Tom Lane wrote:

Andres Freund  writes:

I'd rather create a new 2018-07, and just manually move old patches to
it. Otherwise we'll not really focus on the glut of old things, but
everyone just restarts working on their own new thing.

I thought the idea was to clear out the underbrush of small, ready-to-go
patches.  How old they are doesn't enter into that.

There's a separate issue about what to do to prioritize old patches so
they don't linger indefinitely.  We had a discussion about that at the
dev meeting, but I don't think any specific mechanism was agreed to?

I think we've not fully agreed on that.  I'd argue we should manually
filter things into the next CF. And both small patches and older things
should qualify.




Maybe we should just move everything that was there on say May 1 and not 
accept anything new for July.


I note a substantial number of items in the bug fix category. It would 
certainly be good to reduce that. On a positive note, as a result of 
adding the new committers, quite a large number of patches now have a 
committer as author and/or reviewer. So I'm somewhat hopeful we can 
clear away a lot of the deadwood in July.


cheers

andrew

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

Re: [MASSMAIL]Re: Code of Conduct plan

[gilberto . castillo]

El 2018-06-05 10:54, gilberto.casti...@etecsa.cu escribió:

Hello,

Maybe must include policy of money support from several at member from
country less earnings.


El 2018-06-05 10:45, Chris Travers escribió:

On Mon, Jun 4, 2018 at 6:59 PM, Joshua D. Drake 
wrote:


On 06/03/2018 04:08 PM, Gavin Flower wrote:

My comments:

1) Reiterate my contention that this is a solution is search of
problem. Still it looks like it is going forward, so see below.

2) "... engaging in behavior that may bring the PostgreSQL project
into disrepute, ..."
This to me is overly broad and pulls in actions that may happen
outside the community. Those if they are actually an issue should be
handled where they occur not here.


 This is good point. There are those who would think that one has
performed an action that brings the project into disrepute and a
similar sized bias that suggests that in fact that isn't the case.
This based on the CoC would be judged by the CoC committee.

It is my hope that PostgreSQL.Org -Core chooses members for that
committee that are exceedingly diverse otherwise it is just an echo
chamber for a single ideology and that will destroy this community.

If I may suggest:  The committee should be international as well and
include people from around the world.  The last thing we want is for
it to be dominated by people from one particular cultural viewpoint.


3) "... members must be sensitive to conduct that may be considered
offensive by fellow members and must refrain from engaging in such
conduct. "



Again overly broad, especially given the hypersensitivity of
people these days. I have found that it is enough to disagree with
someone to have it called offensive. This section should be
removed as proscribed behavior is called out in detail in the
paragraphs above it.


"considered offensive by fellow members"

 Is definitely too broad. The problem comes in here:


I might possibly say that "I'm the master in this area" when talking
to someone on a technical subject.  In the sense that I'm better at
that particular skill, but some hypersensitive American could get
their knickers in a twist (notice, that in this context, no gender
is implied -- also in using that that expression "get their knickers
in a twist" could offend some snowflake) claiming that I'm
suggesting that whoever


 "snowflake", I find that term hilarious others find it highly
offensive. Which is correct?

I agree with both concerns in the above exchange.

This is an economic common project.  The goal should be for people to
come together and act civilly.  Waging culture war using the code of
conduct itself should be a violation of the code of conduct and this
goes on *all* (not just one or two) sides.


I'm talking to is my slave!  I heard of an American university
that doesn't want people to use the term master, like in an MSc,
because of the history of slavery.


The PostgreSQL project already has this problem, note we don't use
the terms Master and Slave in reference to replication anymore.


I've used the expressions "sacrifice a willing virgin" and
"offering my first born to the gods" as ways to ensure success of
resolving a technical issue.  The people I say that to, know what
I mean -- and they implicitly know that I'm not seriously
suggesting such conduct.  Yet, if I wrote that publicly, it is
conceivable that someone might object!


Yes and that is a problem. We need to have some simple barrier of
acceptance that we are all adults here (or should act like adults).
Knowing your audience is important.


I would point out also that the PostgreSQL community is nice and
mature.  At PGConf US I saw what appeared to be two individuals with
red MAGA hats.  And yet everyone managed to be civil.  We manage to do
better than the US does on the whole in this regard and we should be
proud of ourselves.


Consider a past advertising campaign in Australia to sell
government Bonds.  They used two very common hand gestures that
are very Australian.  Bond sales dropped.  On investigation, they
found the bonds were mainly bought by old Greek people, who found
the gestures obscene. The gestures?  Thumbs up, and the okay
gesture formed by touching the thumb with the next finger --
nothing sexually suggestive to most Australians, but traditional
Greeks found them offensive.


Using Australia as an example, my understanding is that the word
c**t is part of nomenclature but in the states the word is taboo and
highly frowned upon.


Again key point that a CoC committee needs to be international and
used to addressing these sorts of issues.


Be very careful in attempting to codify 'correct' behaviour!


Correct. I think one way to look at all of this is, "if you
wouldn't say it to your boss or a client don't say it here". That
too has problems but generally speaking I think it keeps the
restrictions rational.


I will post a more specific set of thoughts here but in general I
think the presumption ought to be that people are trying to work

Re: commitfest 2018-07

[Tom Lane]

Joe Conway  writes:
> On 06/05/2018 10:43 AM, Andres Freund wrote:
>> I think we've not fully agreed on that.  I'd argue we should manually
>> filter things into the next CF. And both small patches and older things
>> should qualify.

> Would it work to rename 2018-09 to 2018-07 and then make a pass through
> and move some of the entries to the next commitfest right away? That
> seems easier than the alternative unless you think less than half of
> what is there should be in 2018-07.

Either way, we need some consensus on which patches are not going to be
considered in 07.

regards, tom lane

Re: commitfest 2018-07

[Joe Conway]

On 06/05/2018 10:43 AM, Andres Freund wrote:
> On 2018-06-05 10:20:55 -0400, Tom Lane wrote:
>> Andres Freund  writes:
>>> I'd rather create a new 2018-07, and just manually move old patches to
>>> it. Otherwise we'll not really focus on the glut of old things, but
>>> everyone just restarts working on their own new thing.
>>
>> I thought the idea was to clear out the underbrush of small, ready-to-go
>> patches.  How old they are doesn't enter into that.
>>
>> There's a separate issue about what to do to prioritize old patches so
>> they don't linger indefinitely.  We had a discussion about that at the
>> dev meeting, but I don't think any specific mechanism was agreed to?
> 
> I think we've not fully agreed on that.  I'd argue we should manually
> filter things into the next CF. And both small patches and older things
> should qualify.

Would it work to rename 2018-09 to 2018-07 and then make a pass through
and move some of the entries to the next commitfest right away? That
seems easier than the alternative unless you think less than half of
what is there should be in 2018-07.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: Code of Conduct plan

[Chris Travers]

On Mon, Jun 4, 2018 at 6:59 PM, Joshua D. Drake 
wrote:

> On 06/03/2018 04:08 PM, Gavin Flower wrote:
>
> My comments:
>>>
>>> 1) Reiterate my contention that this is a solution is search of problem.
>>> Still it looks like it is going forward, so see below.
>>>
>>> 2) "... engaging in behavior that may bring the PostgreSQL project into
>>> disrepute, ..."
>>> This to me is overly broad and pulls in actions that may happen outside
>>> the community. Those if they are actually an issue should be handled where
>>> they occur not here.
>>>
>>
> This is good point. There are those who would think that one has performed
> an action that brings the project into disrepute and a similar sized bias
> that suggests that in fact that isn't the case. This based on the CoC would
> be judged by the CoC committee.
>
> It is my hope that PostgreSQL.Org -Core chooses members for that committee
> that are exceedingly diverse otherwise it is just an echo chamber for a
> single ideology and that will destroy this community.


If I may suggest:  The committee should be international as well and
include people from around the world.  The last thing we want is for it to
be dominated by people from one particular cultural viewpoint.


>
>
>
>>> 3) "... members must be sensitive to conduct that may be considered
>>> offensive by fellow members and must refrain from engaging in such conduct.
>>> "
>>>
>>
> Again overly broad, especially given the hypersensitivity of people these
>>> days. I have found that it is enough to disagree with someone to have it
>>> called offensive. This section should be removed as proscribed behavior is
>>> called out in detail in the paragraphs above it.
>>>
>>
> "considered offensive by fellow members"
>
> Is definitely too broad. The problem comes in here:
>
> I might possibly say that "I'm the master in this area" when talking to
>> someone on a technical subject.  In the sense that I'm better at that
>> particular skill, but some hypersensitive American could get their knickers
>> in a twist (notice, that in this context, no gender is implied -- also in
>> using that that expression "get their knickers in a twist" could offend
>> some snowflake) claiming that I'm suggesting that whoever
>>
>
> "snowflake", I find that term hilarious others find it highly offensive.
> Which is correct?


I agree with both concerns in the above exchange.

This is an economic common project.  The goal should be for people to come
together and act civilly.  Waging culture war using the code of conduct
itself should be a violation of the code of conduct and this goes on *all*
(not just one or two) sides.


>
>
> I'm talking to is my slave!  I heard of an American university that
>> doesn't want people to use the term master, like in an MSc, because of the
>> history of slavery.
>>
>
> The PostgreSQL project already has this problem, note we don't use the
> terms Master and Slave in reference to replication anymore.
>
>
>> I've used the expressions "sacrifice a willing virgin" and "offering my
>> first born to the gods" as ways to ensure success of resolving a technical
>> issue.  The people I say that to, know what I mean -- and they implicitly
>> know that I'm not seriously suggesting such conduct.  Yet, if I wrote that
>> publicly, it is conceivable that someone might object!
>>
>
> Yes and that is a problem. We need to have some simple barrier of
> acceptance that we are all adults here (or should act like adults). Knowing
> your audience is important.


I would point out also that the PostgreSQL community is nice and mature.
At PGConf US I saw what appeared to be two individuals with red MAGA hats.
And yet everyone managed to be civil.  We manage to do better than the US
does on the whole in this regard and we should be proud of ourselves.


>
>
> Consider a past advertising campaign in Australia to sell government
>> Bonds.  They used two very common hand gestures that are very Australian.
>> Bond sales dropped.  On investigation, they found the bonds were mainly
>> bought by old Greek people, who found the gestures obscene. The gestures?
>> Thumbs up, and the okay gesture formed by touching the thumb with the next
>> finger -- nothing sexually suggestive to most Australians, but traditional
>> Greeks found them offensive.
>>
>
> Using Australia as an example, my understanding is that the word c**t is
> part of nomenclature but in the states the word is taboo and highly frowned
> upon.


Again key point that a CoC committee needs to be international and used to
addressing these sorts of issues.


>
>
> Be very careful in attempting to codify 'correct' behaviour!
>>
>>
> Correct. I think one way to look at all of this is, "if you wouldn't say
> it to your boss or a client don't say it here". That too has problems but
> generally speaking I think it keeps the restrictions rational.
>
>
I will post a more specific set of thoughts here but in general I think the
presumption ought to be that people are trying to work 

Re: commitfest 2018-07

[Andres Freund]

On 2018-06-05 10:20:55 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > I'd rather create a new 2018-07, and just manually move old patches to
> > it. Otherwise we'll not really focus on the glut of old things, but
> > everyone just restarts working on their own new thing.
> 
> I thought the idea was to clear out the underbrush of small, ready-to-go
> patches.  How old they are doesn't enter into that.
> 
> There's a separate issue about what to do to prioritize old patches so
> they don't linger indefinitely.  We had a discussion about that at the
> dev meeting, but I don't think any specific mechanism was agreed to?

I think we've not fully agreed on that.  I'd argue we should manually
filter things into the next CF. And both small patches and older things
should qualify.

Greetings,

Andres Freund

Re: Concurrency bug in UPDATE of partition-key

[Amit Khandekar]

Attached is a rebased patch version. Also included it in the upcoming
commitfest :
https://commitfest.postgresql.org/18/1660/

In the rebased version, the new test cases are added in the existing
isolation/specs/partition-key-update-1.spec test.


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


fix_concurrency_bug_rebased.patch
Description: Binary data

automating perl compile time checking

[Andrew Dunstan]

Here is a follow up patch to last weeks commit allowing all perl files 
to be checked clean for compile time errors and warnings.



The patch contains a simple script to run the checks. The code that 
finds perl files is put in a function in a single file that is sourced 
by the three locations that need it.



The directory pgperlcritic is renamed to perlcheck, as it not contains 
the new script as well as pgperlcritic.



cheers


andrew


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

>From 6247cd1fbba16426bb1745521d7b444d60ebbd0a Mon Sep 17 00:00:00 2001
From: Andrew Dunstan 
Date: Tue, 5 Jun 2018 10:25:55 -0400
Subject: [PATCH] Extend and slightly refactor perl checking

A script is added to check for perl compile time errors and warnings.
The code to detect all the perl files is put in a central place and
included in the three places it is now needed. The directory
pgperlcritic is renamed to perlcheck, and contains the new check script
as well as pgperlcritic.
---
 src/tools/perlcheck/find_perl_files| 15 
 src/tools/{pgperlcritic => perlcheck}/perlcriticrc |  0
 src/tools/perlcheck/pgperlcritic   | 20 
 src/tools/perlcheck/pgperlsyncheck | 17 +
 src/tools/pgindent/pgperltidy  | 14 +++
 src/tools/pgperlcritic/pgperlcritic| 28 --
 6 files changed, 55 insertions(+), 39 deletions(-)
 create mode 100644 src/tools/perlcheck/find_perl_files
 rename src/tools/{pgperlcritic => perlcheck}/perlcriticrc (100%)
 create mode 100755 src/tools/perlcheck/pgperlcritic
 create mode 100755 src/tools/perlcheck/pgperlsyncheck
 delete mode 100755 src/tools/pgperlcritic/pgperlcritic

diff --git a/src/tools/perlcheck/find_perl_files b/src/tools/perlcheck/find_perl_files
new file mode 100644
index 000..e10466a
--- /dev/null
+++ b/src/tools/perlcheck/find_perl_files
@@ -0,0 +1,15 @@
+
+# src/tools/perlcheck/find_perl_files
+
+# shell function to find all perl files in the source tree
+
+find_perl_files () {
+{
+		# take all .pl and .pm files
+		find . -type f -name '*.p[lm]' -print
+		# take executable files that file(1) thinks are perl files
+		find . -type f -perm -100 -exec file {} \; -print |
+		egrep -i ':.*perl[0-9]*\>' |
+		cut -d: -f1
+	} | sort -u
+}
diff --git a/src/tools/pgperlcritic/perlcriticrc b/src/tools/perlcheck/perlcriticrc
similarity index 100%
rename from src/tools/pgperlcritic/perlcriticrc
rename to src/tools/perlcheck/perlcriticrc
diff --git a/src/tools/perlcheck/pgperlcritic b/src/tools/perlcheck/pgperlcritic
new file mode 100755
index 000..6a31f67
--- /dev/null
+++ b/src/tools/perlcheck/pgperlcritic
@@ -0,0 +1,20 @@
+#!/bin/sh
+
+# src/tools/pgperlcritic/pgperlcritic
+
+test -f src/tools/perlcheck/perlcriticrc || {
+	echo could not find src/tools/perlcheck/perlcriticrc
+	exit 1
+	}
+
+set -e
+
+# set this to override default perlcritic program:
+PERLCRITIC=${PERLCRITIC:-perlcritic}
+
+. src/tools/perlcheck/find_perl_files
+
+find_perl_files | xargs $PERLCRITIC \
+	  --quiet \
+	  --program-extensions .pl \
+	  --profile=src/tools/perlcheck/perlcriticrc
diff --git a/src/tools/perlcheck/pgperlsyncheck b/src/tools/perlcheck/pgperlsyncheck
new file mode 100755
index 000..4595d16
--- /dev/null
+++ b/src/tools/perlcheck/pgperlsyncheck
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+# script to detect compile time errors and warnings in all perl files
+
+INCLUDES="-I src/tools/msvc -I src/tools/msvc/dummylib -I src/backend/catalog"
+INCLUDES="-I src/test/perl -I src/backend/utils/mb/Unicode $INCLUDES"
+INCLUDES="-I src/bin/pg_rewind -I src/test/ssl $INCLUDES"
+
+set -e
+
+. src/tools/perlcheck/find_perl_files
+
+# for zsh
+setopt shwordsplit 2>/dev/null || true
+
+find_perl_files | xargs -L 1 perl $INCLUDES -cw 2>&1 | grep -v OK
+
diff --git a/src/tools/pgindent/pgperltidy b/src/tools/pgindent/pgperltidy
index 5d9aa7c..5e70411 100755
--- a/src/tools/pgindent/pgperltidy
+++ b/src/tools/pgindent/pgperltidy
@@ -7,14 +7,6 @@ set -e
 # set this to override default perltidy program:
 PERLTIDY=${PERLTIDY:-perltidy}
 
-# locate all Perl files in the tree
-(
-	# take all .pl and .pm files
-	find . -type f -a \( -name '*.pl' -o -name '*.pm' \)
-	# take executable files that file(1) thinks are perl files
-	find . -type f -perm -100 -exec file {} \; |
-	egrep -i ':.*perl[0-9]*\>' |
-	cut -d: -f1
-) |
-sort -u |
-xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc
+. src/tools/perlcheck/find_perl_files
+
+find_perl_files | xargs $PERLTIDY --profile=src/tools/pgindent/perltidyrc
diff --git a/src/tools/pgperlcritic/pgperlcritic b/src/tools/pgperlcritic/pgperlcritic
deleted file mode 100755
index 28264b1..000
--- a/src/tools/pgperlcritic/pgperlcritic
+++ /dev/null
@@ -1,28 +0,0 @@
-#!/bin/sh
-
-# src/tools/pgperlcritic/pgperlcritic
-
-test -f src/tools/pgperlcritic/perlcriticrc || {

Re: adding tab completions

[Arthur Zakirov]

On Sun, Jun 03, 2018 at 10:39:22PM -0500, Justin Pryzby wrote:
> Find attached an update which also supports column completion using the legacy
> non-parenthesized syntax.

Thank you!

> BTW..should that be toast.tuple_target ??

I think shouldn't. From the documentation "This parameter cannot be set for
TOAST tables".

> > Also I think it could be good to list column names after parentheses,
> > but I'm not sure if it easy to implement.
> I tried this and nearly gave up, but see attached.

After some thought now I think that this is not so good idea. The code
doesn't look good too. It doesn't worth it and sorry for the distraction.

Moreover there is no such completion for example for the command (it shows
only first column):

CREATE INDEX ON test (

> - "SERVER", "INDEX", "LANGUAGE", "POLICY", "PUBLICATION", 
> "RULE",
> + "SERVER", "INDEX", "LANGUAGE", "POLICY", "PUBLICATION",

Is this a typo? I think still there is a possibility to comment rules.

> else if (HeadMatches1("EXPLAIN") && previous_words_count==2 && 
> prev_wd[0]=='(' && ends_with(prev_wd, ')'))

I think this condition can be replaced by:

else if (TailMatches2("EXPLAIN", MatchAny) && ends_with(prev_wd, ')'))

It looks better to me. Such condition is used for other commands and
works the same way.

The last point I've noticed, there is no VERBOSE entry after VACUUM FULL
ANALYZE command anymore.


I'm not sure how this patch should be commited. Can it be commited
outside the commitfest? Otherwise add it to the next commitfest please
in order not to forget it.

Thoughts?

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Re: commitfest 2018-07

[Tom Lane]

Andres Freund  writes:
> I'd rather create a new 2018-07, and just manually move old patches to
> it. Otherwise we'll not really focus on the glut of old things, but
> everyone just restarts working on their own new thing.

I thought the idea was to clear out the underbrush of small, ready-to-go
patches.  How old they are doesn't enter into that.

There's a separate issue about what to do to prioritize old patches so
they don't linger indefinitely.  We had a discussion about that at the
dev meeting, but I don't think any specific mechanism was agreed to?

regards, tom lane

Re: Loaded footgun open_datasync on Windows

[Laurenz Albe]

Amit Kapila wrote:
> On Fri, Jun 1, 2018 at 8:18 PM, Laurenz Albe  wrote:
> > Amit Kapila wrote:
> > > On Fri, Jun 1, 2018 at 3:13 PM, Laurenz Albe  
> > > wrote:
> > > > I recently read our documentation about reliability on Windows:
> > > > 
> > > > > On Windows, if wal_sync_method is open_datasync (the default), write 
> > > > > caching can
> > > > > be disabled by unchecking
> > > > > My Computer\Open\disk 
> > > > > drive\Properties\Hardware\Properties\Policies\Enable write caching
> > > > > on the disk. Alternatively, set wal_sync_method to fsync or 
> > > > > fsync_writethrough,
> > > > > which prevent write caching.
> > > > 
> > > > It seems dangerous to me to initialize "wal_sync_method" to a method 
> > > > that is unsafe
> > > > by default.  Admittedly I am not a Windows man, but the fact that this 
> > > > has eluded me
> > > > up to now leads me to believe that other people running PostgreSQL on 
> > > > Windows might
> > > > also have missed that important piece of advice and are consequently 
> > > > running with
> > > > an unsafe setup.
> > > > 
> > > > Wouldn't it be smarter to set a different default value on Windows, 
> > > > like we do on
> > > > Linux (for other reasons)?
> > > > 
> > > 
> > > One thing to note is that it seems that in code we use 
> > > FILE_FLAG_WRITE_THROUGH for
> > > open_datasync which according to MSDN [1] will bypass any intermediate 
> > > cache .
> > > See pgwin32_open.  Have you experimented to set any other option as we 
> > > have a comment
> > > in code which say Win32 only has O_DSYNC?
> > > 
> > > 
> > > [1] - 
> > > https://msdn.microsoft.com/en-us/library/windows/desktop/aa363858(v=vs.85).aspx
> > 
> > After studying the code I feel somewhat safer; it looks like the code is ok.
> > I have no Windows at hand, so I cannot test right now.
> > 
> > What happened is that I ran "pg_test_fsync" at a customer site on Windows, 
> > and
> > it returned ridiculously high rates got open_datasync.
> > 
> > So I think that the following should be fixed:
> > 
> > - Change pg_test_fsync to actually use FILE_FLAG_WRITE_THROUGH.
> 
> It sounds sensible to me to make a Windows specific change in pg_test_fsync 
> for open_datasync method.
> That will make pg_test_fsync behave similar to server.

The attached patch makes pg_test_fsync use pgwin32_open on Windows, which is 
what
we use elsewhere.  That should fix the problem.
Ist there a better way to do this?  The problem is that "c.h" is only included
at the very end of "postgres-fe.h", which makes front-end code use "open"
rather than "pgwin32_open" on Windows.

Having read it again, I think that the documentation is fine as it is:
After all, this is just advice what you can do if you are running on unsafe 
hardware,
which doesn't flush to disk like it should.

Yours,
Laurenz AlbeFrom 6ab651c48df66ec93b8d6730bebeaf60e2bb865b Mon Sep 17 00:00:00 2001
From: Laurenz Albe 
Date: Tue, 5 Jun 2018 16:05:10 +0200
Subject: [PATCH] Make pg_test_fsync use pgwin32_open on Windows

---
 src/bin/pg_test_fsync/pg_test_fsync.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/bin/pg_test_fsync/pg_test_fsync.c b/src/bin/pg_test_fsync/pg_test_fsync.c
index e6f7ef8557..a0139a1302 100644
--- a/src/bin/pg_test_fsync/pg_test_fsync.c
+++ b/src/bin/pg_test_fsync/pg_test_fsync.c
@@ -3,6 +3,8 @@
  *		tests all supported fsync() methods
  */
 
+/* we have to include c.h first so that pgwin32_open is used on Windows */
+#include "c.h"
 #include "postgres_fe.h"
 
 #include 
-- 
2.14.3

Re: commitfest 2018-07

[Tom Lane]

Michael Paquier  writes:
> Okay.  If we tend toward this direction, I propose to do this switch in
> two days my time (Thursday afternoon in Tokyo) if there are no
> objections, so as anybody has hopefully time to argue back.

I think we probably have to wait longer.  It's not clear to me when the
RMT will decide that the 07 fest is go or no-go, but surely they've
not decided yet.

regards, tom lane

Re: libpq compression

[Konstantin Knizhnik]

On 05.06.2018 09:04, Thomas Munro wrote:

On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik
 wrote:

Concerning specification of compression level: I have made many experiments
with different data sets and both zlib/zstd and in both cases using
compression level higher than default doesn't cause some noticeable increase
of compression ratio, but quite significantly reduce speed. Moreover, for
"pgbench -i" zstd provides better compression ratio (63 times!) with
compression level 1 than with with largest recommended compression level 22!
This is why I decided not to allow user to choose compression level.

Speaking of configuration, are you planning to support multiple
compression libraries at the same time?  It looks like the current
patch implicitly requires client and server to use the same configure
option, without any attempt to detect or negotiate.  Do I guess
correctly that a library mismatch would produce an incomprehensible
corrupt stream message?

Frankly speaking I am not sure that support of multiple compression 
libraries at the same time is actually needed.
If we build Postgres from sources, then both frontend and backend 
libraries will use the same compression library.

zlib is available almost everywhere and Postgres in any case is using it.
zstd is faster and provides better compression ratio. So in principle it 
may be useful to try first to use zstd and if it is not available then 
use zlib.

It will require dynamic loading of this libraries.

libpq stream compression is not the only place where compression is used 
in Postgres. So I think that the problem of  choosing compression algorithm
and supporting custom compression methods should be fixed at some upper 
level. There is patch for custom compression method at commit fest.

May be it should be combined with this one.

Right now if client and server libpq libraries were built with different 
compression libraries, then decompress error will be reported.
Supporting multiple compression methods will require more sophisticated 
handshake protocol so that client and server can choose compression 
method which is supported by both of them.

But certainly it can be done.

--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: libpq compression

[Konstantin Knizhnik]

On 05.06.2018 08:26, Thomas Munro wrote:

On Thu, May 17, 2018 at 3:54 AM, Konstantin Knizhnik
 wrote:

Thank you for this notice.
Updated and rebased patch is attached.

Hi Konstantin,

Seems very useful.  +1.

+ rc = inflate(>rx, Z_SYNC_FLUSH);
+ if (rc != Z_OK)
+ {
+ return ZPQ_DECOMPRESS_ERROR;
+ }

Does this actually guarantee that zs->rx.msg is set to a string?  I
looked at some documentation here:

https://www.zlib.net/manual.html

It looks like return value Z_DATA_ERROR means that msg is set, but for
the other error codes Z_STREAM_ERROR, Z_BUF_ERROR, Z_MEM_ERROR it
doesn't explicitly say that.  From a casual glance at
https://github.com/madler/zlib/blob/master/inflate.c I think it might
be set to Z_NULL and then never set to a string except in the mode =
BAD paths that produce the Z_DATA_ERROR return code.  That's
interesting because later we do this:

+ if (r == ZPQ_DECOMPRESS_ERROR)
+ {
+ ereport(COMMERROR,
+ (errcode_for_socket_access(),
+ errmsg("Failed to decompress data: %s", zpq_error(PqStream;
+ return EOF;

... where zpq_error() returns zs->rx.msg.  That might crash or show
"(null)" depending on libc.

Also, message style: s/F/f/

+ssize_t zpq_read(ZpqStream* zs, void* buf, size_t size, size_t* processed)

Code style:  We write "Type *foo", not "Type* var".  We put the return
type of a function definition on its own line.

It looks like there is at least one place where zpq_stream.o's symbols
are needed but it isn't being linked in, so the build fails in some
ecpg stuff reached by make check-world:

gcc -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -fexcess-precision=standard -g -O2 -pthread -D_REENTRANT
-D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS test1.o
-L../../../../../src/port -L../../../../../src/common -L../../ecpglib
-lecpg -L../../pgtypeslib -lpgtypes
-L../../../../../src/interfaces/libpq -lpq   -Wl,--as-needed
-Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags  -lpgcommon
-lpgport -lpthread -lz -lreadline -lrt -lcrypt -ldl -lm   -o test1
../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_free'
../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_error'
../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_read'
../../../../../src/interfaces/libpq/libpq.so: undefined reference to
`zpq_buffered'
../../../../../src/interfaces/libpq/libpq.so: undefined reference to
`zpq_create'
../../../../../src/interfaces/libpq/libpq.so: undefined reference to `zpq_write'



Hi Thomas,
Thank you for review. Updated version of the patch fixing all reported 
problems is attached.


--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

diff --git a/configure b/configure
index 0aafd9c..fc5685c 100755
--- a/configure
+++ b/configure
@@ -699,6 +699,7 @@ ELF_SYS
 EGREP
 GREP
 with_zlib
+with_zstd
 with_system_tzdata
 with_libxslt
 with_libxml
@@ -863,6 +864,7 @@ with_libxml
 with_libxslt
 with_system_tzdata
 with_zlib
+with_zstd
 with_gnu_ld
 enable_largefile
 enable_float4_byval
@@ -8017,6 +8019,86 @@ fi
 
 
 #
+# ZStd
+#
+
+
+
+# Check whether --with-zstd was given.
+if test "${with_zstd+set}" = set; then :
+  withval=$with_zstd;
+  case $withval in
+yes)
+  ;;
+no)
+  :
+  ;;
+*)
+  as_fn_error $? "no argument expected for --with-zstd option" "$LINENO" 5
+  ;;
+  esac
+
+else
+  with_zstd=no
+
+fi
+
+
+
+
+if test "$with_zstd" = yes ; then
+  { $as_echo "$as_me:${as_lineno-$LINENO}: checking for ZSTD_compress in -lzstd" >&5
+$as_echo_n "checking for ZSTD_compress in -lzstd... " >&6; }
+if ${ac_cv_lib_zstd_ZSTD_compress+:} false; then :
+  $as_echo_n "(cached) " >&6
+else
+  ac_check_lib_save_LIBS=$LIBS
+LIBS="-lzstd  $LIBS"
+cat confdefs.h - <<_ACEOF >conftest.$ac_ext
+/* end confdefs.h.  */
+
+/* Override any GCC internal prototype to avoid an error.
+   Use char because int might match the return type of a GCC
+   builtin and then its argument prototype would still apply.  */
+#ifdef __cplusplus
+extern "C"
+#endif
+char ZSTD_compress ();
+int
+main ()
+{
+return ZSTD_compress ();
+  ;
+  return 0;
+}
+_ACEOF
+if ac_fn_c_try_link "$LINENO"; then :
+  ac_cv_lib_zstd_ZSTD_compress=yes
+else
+  ac_cv_lib_zstd_ZSTD_compress=no
+fi
+rm -f core conftest.err conftest.$ac_objext \
+conftest$ac_exeext conftest.$ac_ext
+LIBS=$ac_check_lib_save_LIBS
+fi
+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $ac_cv_lib_zstd_ZSTD_compress" >&5
+$as_echo "$ac_cv_lib_zstd_ZSTD_compress" >&6; }
+if test "x$ac_cv_lib_zstd_ZSTD_compress" = xyes; then :
+  cat >>confdefs.h <<_ACEOF
+#define HAVE_LIBZSTD 1
+_ACEOF
+
+  LIBS="-lzstd $LIBS"
+
+else
+  as_fn_error $? "library 'zstd' is required for ZSTD support" "$LINENO" 5
+fi
+
+fi
+
+
+
+#
 # Elf
 #
 
diff --git a/src/Makefile.global.in b/src/Makefile.global.in
index 95d090e..b59629c 100644
--- 

Re: Spilling hashed SetOps and aggregates to disk

[Tomas Vondra]

On 06/05/2018 03:41 PM, se...@rielau.com wrote:
In our code base we have added a WithStats-Flavor for creating memory 
contexts.
This api accepts a pointer to metric for accounting and it is inherited 
by all subcontexts unless overridden.
So we only needed to change context creation API where we wanted (such 
as TopTansactionContext, Message Context, ..)

That's quite trivial, actually.


I think that's pretty much what we tried when this patch was first 
discussed in 2015, but it added measurable overhead (1-3%) even when the 
accounting was disabled. Which is not quite desirable, of course.


Also we have fixed all those missing hash spills - albeit based on the 
9.6 hash table design I think.


I don't think this part of the code changed all that much.


If there is interest by the community we are very willing to share.


Absolutely! I think it'd be great to share both the code and the 
reasoning behind the design.



Cheers
Serge
Salesforce


cheers

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

RE: Re: Spilling hashed SetOps and aggregates to disk

[serge]

In our code base we have added a WithStats-Flavor for creating memory contexts.
This api accepts a pointer to metric for accounting and it is inherited by all 
subcontexts unless overridden.
So we only needed to change context creation API where we wanted (such as 
TopTansactionContext, Message Context, ..)
That's quite trivial, actually.
 
Also we have fixed all those missing hash spills - albeit based on the 9.6 hash 
table design I think.
 
If there is interest by the community we are very willing to share.
 
Cheers
Serge
Salesforce

Re: why partition pruning doesn't work?

[Ashutosh Bapat]

On Tue, Jun 5, 2018 at 6:24 PM, Dmitry Dolgov <9erthali...@gmail.com> wrote:
>> On 5 June 2018 at 12:31, Amit Langote  wrote:
>>
>> doesn't look quite right.  What says expr is really a Param?  The patch
>> appears to work because, by setting pinfo->execparams to *something*, it
>> triggers execution-time pruning to run; its contents aren't necessarily
>> used during execution pruning.  In fact, it would've crashed if the
>> execution-time pruning code had required execparams to contain *valid*
>> param id, but currently it doesn't.
>>
>> What I think we'd need to do to make this work is to make execution-time
>> pruning be invoked even if there aren't any Params involved.  IOW, let's
>> try to teach make_partition_pruneinfo that it can go ahead also in the
>> cases where there are expressions being compared with the partition key
>> that contain (only) stable functions.  Then, go and fix the
>> execution-pruning code to not *always* expect there to be Params to prune
>> with.
>
> Yeah, I agree - I copied this approach mindlessly from the original hacky
> patch. So, looks like it's necessary to have something like got_stable_expr
> together with gotparam.

I think the current code is heavily relying on Params to be present
for partition pruning, which isn't true. Runtime partition pruning is
possible when there are comparison conditions with partition key
expressions on one side and "execution time constant" expressions on
the other side. By "execution time constant" expression, I mean any
expression that evaluates to a constant at the time of execution like
a stable expressions (not just functions) or a Param expression. I can
think of only these two at this time, but there can be more. So,
gotparam should be renamed as "gotprunable_cond" to be generic.
pull_partkey_params() should be renamed as "pull_partkey_conds" or
something generic. That function would return true if there exists an
expression in steps which can be evaluated to a constant at runtime,
otherwise it returns false. My guess is there will be false-positives
which need to be dealt with later, but there will be no
false-negatives.

> And after that the only place where I see Params
> are in use is partkey_datum_from_expr where all the stuff is actually
> evaluated. So apparently this part about "fix the execution-pruning code to 
> not
> *always* expect there to be Params to prune with" will be only about this
> function - am I correct or there is something else that I missed?

Yes. But I think trying to evaluate parameters in this function is not
good. The approach of folding constant expressions before or
immediately after the execution starts doesn't require the expressions
to be evaluated in partkey_datum_from_expr and might benefit other
places where stable expressions or params can appear.

Other problem with partkey_datum_from_expr() seems to be that it
evaluated only param nodes but not the expressions involving
parameters which can folded into constants at runtime. Take for
example following queries on table t1 with two partitions (0, 100) and
(100, 200), populated using "insert into t1 select i, i from
generate_series(0, 199) i;". There's an index on t1(a).

explain analyze select * from t1 x left join t1 y on x.a = y.b where y.a = 5;
 QUERY PLAN

 Nested Loop  (cost=0.00..6.78 rows=1 width=16) (actual
time=0.033..0.066 rows=1 loops=1)
   ->  Append  (cost=0.00..2.25 rows=1 width=8) (actual
time=0.019..0.035 rows=1 loops=1)
 ->  Seq Scan on t1p1 y  (cost=0.00..2.25 rows=1 width=8)
(actual time=0.018..0.035 rows=1 loops=1)
   Filter: (a = 5)
   Rows Removed by Filter: 99
   ->  Append  (cost=0.00..4.51 rows=2 width=8) (actual
time=0.011..0.027 rows=1 loops=1)
 ->  Seq Scan on t1p1 x  (cost=0.00..2.25 rows=1 width=8)
(actual time=0.006..0.022 rows=1 loops=1)
   Filter: (y.b = a)
   Rows Removed by Filter: 99
 ->  Seq Scan on t1p2 x_1  (cost=0.00..2.25 rows=1 width=8)
(never executed)
   Filter: (y.b = a)
 Planning Time: 0.644 ms
 Execution Time: 0.115 ms
(13 rows)

t1p2 x_1 is never scanned indicating that run time partition pruning
happened. But then see the following query

postgres:17889=#explain analyze select * from t1 x left join t1 y on
x.a = y.b + 100 where y.a = 5;
  QUERY PLAN
--
 Nested Loop  (cost=0.00..7.28 rows=1 width=16) (actual
time=0.055..0.093 rows=1 loops=1)
   ->  Append  (cost=0.00..2.25 rows=1 width=8) (actual
time=0.017..0.034 rows=1 loops=1)
 ->  Seq Scan on t1p1 y  (cost=0.00..2.25 rows=1 width=8)
(actual time=0.016..0.033 rows=1 loops=1)
   Filter: (a = 5)
   Rows Removed by Filter: 99

install doesn't work on HEAD

[Amit Kapila]

Hi,

On my win-7, I am facing $SUBJECT.  I am consistently getting below error:
Copying build output files...Could not copy postgres.exe
 at install.pl line 26.

On digging, I found that it started failing with commit 3a7cc727 (Don't
fall off the end of perl functions).  It seems that we have added empty
'return' in all of the functions which seems to break one of the case:

lcopy("$conf\\$pf\\$pf.$ext", "$target\\$dir\\$pf.$ext")
|| croak "Could not copy $pf.$ext\n";

I think the return from lcopy implies 0 (or undef) which cause it to fail.
By reading couple of articles on net [1][2] related to this, it seems we
can't blindly return in all cases.  On changing, the lcopy as below,
install appears to be working.

@@ -40,7 +40,7 @@ sub lcopy
copy($src, $target)
  || confess "Could not copy $src to $target\n";
-   return;
+   return 1;
 }

I have randomly check some of the other functions where this patch has
added 'return' and those seem to be fine.

Is it by any chance related Perl version or some other settings?  If not,
then we should do something for it.

[1] - https://perlmaven.com/how-to-return-undef-from-a-function
[2] -
http://search.cpan.org/dist/Perl-Critic/lib/Perl/Critic/Policy/Subroutines/RequireFinalReturn.pm

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

Re: Spilling hashed SetOps and aggregates to disk

[Tomas Vondra]

On 06/05/2018 03:17 PM, David Rowley wrote:

On 6 June 2018 at 01:09, Andres Freund  wrote:

On 2018-06-06 01:06:39 +1200, David Rowley wrote:

My concern is that only accounting memory for the group and not the
state is only solving half the problem. It might be fine for
aggregates that don't stray far from their aggtransspace, but for the
other ones, we could still see OOM.



If solving the problem completely is too hard, then a half fix (maybe
3/4) is better than nothing, but if we can get a design for a full fix
before too much work is done, then isn't that better?


I don't think we actually disagree.  I was really primarily talking
about the case where we can't really do better because we don't have
serialization support.  I mean we could just rescan from scratch, using
a groupagg, but that obviously sucks.


I don't think we do. To take yours to the 100% solution might just
take adding the memory accounting to palloc that Jeff proposed a few
years ago and use that accounting to decide when we should switch
method.

However, I don't quite fully recall how the patch accounted for
memory consumed by sub-contexts and if getting the entire
consumption required recursively looking at subcontexts. If that's
the case then checking the consumption would likely cost too much if
it was done after each tuple was aggregated.


Yeah, a simple wrapper would not really fix the issue, because 
allocating memory in the subcontext would not update the accounting 
information. So we'd be oblivious of possibly large amounts of memory. I 
don't quite see how is this related to (not) having serialization 
support, as it's more about not knowing we already hit the memory limit.



That being said, I don't think array_agg actually has the issue, at 
least since b419865a814abbca12bdd6eef6a3d5ed67f432e1 (it does behave 
differently in aggregate and non-aggregate contexts, IIRC).


I don't know how common this issue is, though. I don't think any 
built-in aggregates create additional contexts, but I may be wrong. But 
we have this damn extensibility thing, where users can write their own 
aggregates ...


regards

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