date:20151003

Re: [HACKERS] creating extension including dependencies

2015-10-03 Thread Petr Jelinek


On 2015-10-03 17:16, Andres Freund wrote:

On 2015-10-03 17:15:54 +0200, Andres Freund wrote:

Here's an updated patch. Petr, could you please expand the test to
handle a bit more complex cascading setups?




Okay, I changed the test to make the dependencies bit more complex - 
more than one dependency per extension + also non-cyclic 
interdependency. It still works as expected.


--
 Petr Jelinek  http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
>From 54ce5b981e6d0009572562a989a287371a8c7146 Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Sat, 3 Oct 2015 17:48:13 +0200
Subject: [PATCH] Add CASCADE support for CREATE EXTENSION.

Without CASCADE, if an extension has an unfullfilled dependency on
another extension, CREATE EXTENSION ERRORs out with "required extension
... is not installed". That is annoying, especially when that dependency
is an implementation detail of the extension, rather than something the
extension's user can make sense of.

In addition to CASCADE this also includes a small set of regression
tests around CREATE EXTENSION.

Author: Petr Jelinek, editorialized by Michael Paquier, Andres Freund
Reviewed-By: Michael Paquier, Andres Freund, Jeff Janes
Discussion: 557e0520.3040...@2ndquadrant.com
---
 contrib/hstore_plperl/expected/hstore_plperl.out   |   6 +-
 contrib/hstore_plperl/expected/hstore_plperlu.out  |   6 +-
 contrib/hstore_plperl/sql/hstore_plperl.sql|   4 +-
 contrib/hstore_plperl/sql/hstore_plperlu.sql   |   4 +-
 .../hstore_plpython/expected/hstore_plpython.out   |   4 +-
 contrib/hstore_plpython/sql/hstore_plpython.sql|   3 +-
 contrib/ltree_plpython/expected/ltree_plpython.out |   4 +-
 contrib/ltree_plpython/sql/ltree_plpython.sql  |   3 +-
 doc/src/sgml/ref/create_extension.sgml |  42 
 src/backend/commands/extension.c   | 217 ++---
 src/backend/parser/gram.y  |   4 +
 src/bin/psql/tab-complete.c|   7 +-
 src/test/modules/Makefile  |   1 +
 src/test/modules/test_extensions/.gitignore|   4 +
 src/test/modules/test_extensions/Makefile  |  23 +++
 .../test_extensions/expected/test_extensions.out   |  37 
 .../test_extensions/sql/test_extensions.sql|  15 ++
 .../modules/test_extensions/test_ext1--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext1.control |   5 +
 .../modules/test_extensions/test_ext2--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext2.control |   4 +
 .../modules/test_extensions/test_ext3--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext3.control |   3 +
 .../modules/test_extensions/test_ext4--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext4.control |   4 +
 .../modules/test_extensions/test_ext5--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext5.control |   3 +
 .../test_extensions/test_ext_cyclic1--1.0.sql  |   3 +
 .../test_extensions/test_ext_cyclic1.control   |   4 +
 .../test_extensions/test_ext_cyclic2--1.0.sql  |   3 +
 .../test_extensions/test_ext_cyclic2.control   |   4 +
 src/tools/msvc/Mkvcbuild.pm|   3 +-
 32 files changed, 346 insertions(+), 89 deletions(-)
 create mode 100644 src/test/modules/test_extensions/.gitignore
 create mode 100644 src/test/modules/test_extensions/Makefile
 create mode 100644 src/test/modules/test_extensions/expected/test_extensions.out
 create mode 100644 src/test/modules/test_extensions/sql/test_extensions.sql
 create mode 100644 src/test/modules/test_extensions/test_ext1--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext1.control
 create mode 100644 src/test/modules/test_extensions/test_ext2--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext2.control
 create mode 100644 src/test/modules/test_extensions/test_ext3--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext3.control
 create mode 100644 src/test/modules/test_extensions/test_ext4--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext4.control
 create mode 100644 src/test/modules/test_extensions/test_ext5--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext5.control
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic1--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic1.control
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic2--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic2.control

diff --git a/contrib/hstore_plperl/expected/hstore_plperl.out b/contrib/hstore_plperl/expected/hstore_plperl.out
index cf384eb..25fc506 100644
--- a/contrib/hstore_plperl/expected/hstore_plperl.out
+++ b/contrib/hstore_plperl/expected/hstore_plperl.out
@@ -1,6 +1,6 @@
-CREATE EXTENSION hstore;
-CREATE EXTENSION plperl;
-CREATE EXTENSION hstore_plperl;

Re: [HACKERS] ON CONFLICT issues around whole row vars,

2015-10-03 Thread Andres Freund

> My proposal in this WIP patch is to make it a bit clearer that
> 'EXCLUDED' isn't a real relation. I played around with adding a
> different rtekind, but that's too heavy a hammer. What I instead did was
> to set relkind to composite - which seems to signal pretty well that
> we're not dealing with a real relation. That immediately fixes the RLS
> issue as fireRIRrules has the following check:
>   if (rte->rtekind != RTE_RELATION ||
>   rte->relkind != RELKIND_RELATION)
>   continue;
> It also makes it relatively straightforward to fix the system column
> issue by adding an additional relkind check to scanRTEForColumn's system
> column handling.

That works, but also precludes referencing 'oid' in a WITH OIDs table
via EXCLUDED.oid - to me that looks correct since a to-be-inserted row
can't yet have an oid assigned. Differing opinions?

Greetings,

Andres Freund


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

[HACKERS] Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

2015-10-03 Thread Michael Paquier

On Fri, Oct 2, 2015 at 11:10 PM, Amir Rohan wrote:
> On 10/02/2015 03:33 PM, Michael Paquier wrote:
> > Any server instances created during the tests should never use a
> > user-defined port for portability. Hence using those ports as keys
> > just made sense. We could have for example custom names, that have
> > port values assigned to them, but that's actually an overkill and
> > complicates the whole facility.
> >
>
> Something like:
>
> global nPortsAssigned = 0;
> AssignPort() -> return is_ok(nPortsAssigned++)
>
> was what I used.

Why do you need that. Creating a node is in the most basic way a
matter of calling make_master, make_*_standby where a port number, or
identifier gets uniquely assigned to a node. The main point of the
approach taken by this patch is to make port assignment transparent
for the caller.

> >> 4) Port assignment relies on liveness checks on running servers.
> >> If a server is shut down and a new instantiated, the port will get
> >> reused, data will get trashed, and various confusing things can happen.
> >
> > Right. The safest way to do that is to check in get_free_port if a
> > port number is used by a registered node, and continue to loop in if
> > that's the case. So done.
> >
>
> That eliminates the "sweet and gentle" variant of the scenario, but it's
> susceptible to the "ABA problem":
> https://en.wikipedia.org/wiki/ABA_problem
> https://youtu.be/CmxkPChOcvw?t=786

I learnt a new thing here. That's basically an existing problem even
with the existing perl test infrastructure relying on TestLib.pm when
tests are run in parallel. What we would need here is a global mapping
file storing all the port numbers used by all the nodes currently in
the tests.

>
> Granted, you have to try fairly hard to shoot yourself in the leg,
> but since the solution is so simple, why not? If we never reuse ports
> within a single test, this goes away.

Well, you can reuse the same port number in a test. Simply teardown
the existing node and then recreate a new one. I think that port
number assignment to a node should be transparent to the caller, in
our case the perl test script holding a scenario.

> >> 5) Servers are shutdown with -m 'immediate', which can lead to races
> >> in the script when archiving is turned on. That may be good for some
> >> tests, but there's no control over it.
> >
> > I hesitated with fast here actually. So changed this way. We would
> > want as wall a teardown command to stop the node with immediate and
> > unregister the node from the active list.
> >
>
> In particular, I was shutting down an archiving node and the archiving
> was truncated. I *think* smart doesn't do this. But again, it's really
> that the test writer can't easily override, not that the default is wrong.

Ah, OK. Then fast is just fine. It shuts down the node correctly.
"smart" would wait for all the current connections to finish but I am
wondering if it currently matters here: I don't see yet a clear use
case yet where it would make sense to have multi-threaded script... If
somebody comes back with a clear idea here perhaps we could revisit
that but it does not seem worth it now.

> >> Other issues:
> >> 6. Directory structure, used one directory per thing but more logical
> >> to place all things related to an instance under a single directory,
> >> and name them according to role (57333_backup, and so on).
> >
> > Er, well. The first version of the patch did so, and then I switched
> > to an approach closer to what the existing TAP facility is doing. But
> > well let's simplify things a bit.
> >
>
> I know, I know, but:
> 1) an instance is a "thing" in your script, so having its associated
> paraphernalia in one place makes more sense (maybe only to me).
> 2) That's often how folks (well, how I) arrange things in deployment,
> at least with archive/backup as symlinks to the nas.
>
> Alternatively, naming the dirs with a prefix (srv_foo_HASH,
> backup_foo_backupname_HASH, etc') would work as well.

The useful portion about tempdir is that it cleans up itself
automatically should an error happen. It does not seem to me we want
use that.

> >> 7. enable_restoring() uses "cp -i" 'archive_command', not a good fit
> >> for an automated test.
> >
> > This seems like a good default to me, and actually that's portable on
> > Windows easily. One could always append a custom archive_command in a
> > test when for example testing conflicting archiving when archive_mode
> > = always.
> >
>
> Ok, I wasn't sure about this, but specifically activating a switch that
> asks for input from the user during a test? hmm.

Er... The -i switch is a bad idea. I removed it. Honestly I don't
recall why it was here to begin with...

>  >> 8. No canned way to output a pprinted overview of the running system
> >> (paths, ports, for manual checking).
> >
> > Hm. Why not... Are you suggesting something like print_current_conf
> > that goes through all the registered nodes and outputs that? How would
> > you use

Re: [HACKERS] Idea for improving buildfarm robustness

2015-10-03 Thread Tom Lane

Michael Paquier  writes:
> On Sat, Oct 3, 2015 at 1:39 PM, Tom Lane  wrote:
>> BTW, my thought at the moment is to wait till after next week's releases
>> to push this in.  I think it's probably solid, but it doesn't seem like
>> it's worth taking the risk of pushing shortly before a wrap date.

> That seems a wiser approach to me. Down to which version are you planning a
> backpatch? As this is aimed for the buildfarm stability with TAP stuff, 9.4?

What we'd discussed was applying this to all branches that contain the
5-second-timeout logic, which is everything back to 9.1.  The branches
that have TAP tests have a wider cross-section for failure in the
buildfarm because more postmaster starts are involved, but all of them
are capable of getting burnt this way --- see shearwater's results for
instance.

regards, tom lane

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

Re: [HACKERS] ON CONFLICT DO UPDATE using EXCLUDED.column gives an error about mismatched types

2015-10-03 Thread Andres Freund

Hi,

> db=# INSERT INTO brokentab(id, k1,k2,k3,k4,k5,k6,k7, smallval) VALUES
> (5,0,0,0,1,0,1,0, 0) ON CONFLICT (id, k1,k2,k3,k4,k5,k6,k7) DO UPDATE SET
> smallval=EXCLUDED.smallval;
> ERROR:  attribute 29 has wrong type
> DETAIL:  Table has type integer, but query expects smallint.

I pushed a fix for the issue. Could you verify that your original
problem does not exist anymore?

Thanks for testing Geoff, thanks for helping to nail this down Amit and
Peter.

Regards,

Andres


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

Re: [HACKERS] creating extension including dependencies

2015-10-03 Thread Andres Freund

On 2015-10-03 17:15:54 +0200, Andres Freund wrote:
> Here's an updated patch. Petr, could you please expand the test to
> handle a bit more complex cascading setups?

...
>From fa11dc75500eb91b68baeeb07a00a789ed0050b3 Mon Sep 17 00:00:00 2001
From: Andres Freund 
Date: Sat, 3 Oct 2015 17:01:32 +0200
Subject: [PATCH] Add CASCADE support for CREATE EXTENSION.

Without CASCADE, if an extension has an unfullfilled dependency on
another extension, CREATE EXTENSION ERRORs out with "required extension
... is not installed". That is annoying, especially when that dependency
is an implementation detail of the extension, rather than something the
extension's user can make sense of.

In addition to CASCADE this also includes a small set of regression
tests around CREATE EXTENSION.

Author: Petr Jelinek, editorialized by Michael Paquier, Andres Freund
Reviewed-By: Michael Paquier, Andres Freund, Jeff Janes,
Discussion: 557e0520.3040...@2ndquadrant.com
---
 contrib/hstore_plperl/expected/hstore_plperl.out   |   6 +-
 contrib/hstore_plperl/expected/hstore_plperlu.out  |   6 +-
 contrib/hstore_plperl/sql/hstore_plperl.sql|   4 +-
 contrib/hstore_plperl/sql/hstore_plperlu.sql   |   4 +-
 .../hstore_plpython/expected/hstore_plpython.out   |   4 +-
 contrib/hstore_plpython/sql/hstore_plpython.sql|   3 +-
 contrib/ltree_plpython/expected/ltree_plpython.out |   4 +-
 contrib/ltree_plpython/sql/ltree_plpython.sql  |   3 +-
 doc/src/sgml/ref/create_extension.sgml |  42 
 src/backend/commands/extension.c   | 217 ++---
 src/backend/parser/gram.y  |   4 +
 src/bin/psql/tab-complete.c|   7 +-
 src/test/modules/Makefile  |   1 +
 src/test/modules/test_extensions/.gitignore|   4 +
 src/test/modules/test_extensions/Makefile  |  21 ++
 .../test_extensions/expected/test_extensions.out   |  30 +++
 .../test_extensions/sql/test_extensions.sql|  18 ++
 .../modules/test_extensions/test_ext1--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext1.control |   5 +
 .../modules/test_extensions/test_ext2--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext2.control |   4 +
 .../modules/test_extensions/test_ext3--1.0.sql |   3 +
 src/test/modules/test_extensions/test_ext3.control |   3 +
 .../test_extensions/test_ext_cyclic1--1.0.sql  |   3 +
 .../test_extensions/test_ext_cyclic1.control   |   4 +
 .../test_extensions/test_ext_cyclic2--1.0.sql  |   3 +
 .../test_extensions/test_ext_cyclic2.control   |   4 +
 src/tools/msvc/Mkvcbuild.pm|   3 +-
 28 files changed, 327 insertions(+), 89 deletions(-)
 create mode 100644 src/test/modules/test_extensions/.gitignore
 create mode 100644 src/test/modules/test_extensions/Makefile
 create mode 100644 src/test/modules/test_extensions/expected/test_extensions.out
 create mode 100644 src/test/modules/test_extensions/sql/test_extensions.sql
 create mode 100644 src/test/modules/test_extensions/test_ext1--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext1.control
 create mode 100644 src/test/modules/test_extensions/test_ext2--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext2.control
 create mode 100644 src/test/modules/test_extensions/test_ext3--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext3.control
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic1--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic1.control
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic2--1.0.sql
 create mode 100644 src/test/modules/test_extensions/test_ext_cyclic2.control

diff --git a/contrib/hstore_plperl/expected/hstore_plperl.out b/contrib/hstore_plperl/expected/hstore_plperl.out
index cf384eb..25fc506 100644
--- a/contrib/hstore_plperl/expected/hstore_plperl.out
+++ b/contrib/hstore_plperl/expected/hstore_plperl.out
@@ -1,6 +1,6 @@
-CREATE EXTENSION hstore;
-CREATE EXTENSION plperl;
-CREATE EXTENSION hstore_plperl;
+CREATE EXTENSION hstore_plperl CASCADE;
+NOTICE:  installing required extension "hstore"
+NOTICE:  installing required extension "plperl"
 SELECT transforms.udt_schema, transforms.udt_name,
routine_schema, routine_name,
group_name, transform_type
diff --git a/contrib/hstore_plperl/expected/hstore_plperlu.out b/contrib/hstore_plperl/expected/hstore_plperlu.out
index c97fd3f..b09fb78 100644
--- a/contrib/hstore_plperl/expected/hstore_plperlu.out
+++ b/contrib/hstore_plperl/expected/hstore_plperlu.out
@@ -1,6 +1,6 @@
-CREATE EXTENSION hstore;
-CREATE EXTENSION plperlu;
-CREATE EXTENSION hstore_plperlu;
+CREATE EXTENSION hstore_plperlu CASCADE;
+NOTICE:  installing required extension "hstore"
+NOTICE:  installing required extension "plperlu"
 SELECT transforms.udt_schema, transforms.udt_name,
routine_schema, routine_name,

Re: [HACKERS] WIP: Rework access method interface

2015-10-03 Thread Andres Freund

Hi,

this topic has seen a lot of activity/review. As development is ongoing
I'm moving the patch to the next CF.

Greetings,

Andres Freund


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

Re: [HACKERS] WIP: Rework access method interface

2015-10-03 Thread Petr Jelinek

On 2015-10-03 08:27, Amit Kapila wrote:

On Fri, Oct 2, 2015 at 8:14 PM, Alexander Korotkov
> wrote:
 >
 >
 > I agree about staying with one SQL-visible function.

Okay, this does not necessarily mean there should be only one validation 
function in the C struct though. I wonder if it would be more future 
proof to name the C interface as something else than the current generic 
amvalidate. Especially considering that it basically only does opclass 
validation at the moment (It's IMHO saner in terms of API evolution to 
expand the struct with more validator functions in the future compared 
to adding arguments to the existing function).

Few assorted comments:

1.
+  * Get IndexAmRoutine structure from access method oid.
+  */
+ IndexAmRoutine *
+ GetIndexAmRoutine(Oid
amoid)
...
+ if (!RegProcedureIsValid
(amhandler))
+ elog(ERROR, "invalid %u regproc", amhandler);

I have noticed that currently, the above kind of error is reported slightly
differently as in below code:
if (!RegProcedureIsValid(procOid)) \
elog(ERROR, "invalid %s regproc", CppAsString
(pname)); \

If you feel it is better to do the way as it is in current code, then you
can change accordingly.

It's completely different use-case from existing code. And tbh I think 
it should have completely different and more informative error message 
something in the style of "index access method %s does not have a 
handler" (see for example GetFdwRoutineByServerId or 
transformRangeTableSample how this is handled for similar cases currently).

This however brings another comment - I think it would be better if the 
GetIndexAmRoutine would be split into two interfaces. The 
GetIndexAmRoutine itself would accept the amhandler Oid and should just 
do the OidFunctionCall and then check the result is not NULL and 
possibly that it is an IndexAmRoutine node. And then all the

(IndexAmRoutine*)DatumGetPointer(!OidFunctionCall0(accessMethodForm->amhandler));
calls in the code should be replaced with calls to the GetIndexAmRoutine 
instead.

The other routine (let's call it GetIndexAmRoutineByAmId for example) 
would get IndexAmRoutine from amoid by looking up catalog, doing that 
validation of amhandler Oid/regproc and calling the GetIndexAmRoutine.

3.
!Handler function must be written in a compiled language such as C,
using
!the version-1 interface.

Here, it is not completely clear, what do you refer to as version-1
interface.

This seems to be copy paste from fdw docs, if we decide this should be 
explained differently then it should be explained differently there as well.

4.
xindex.sgml
Index Methods and Operator Classes
..
It is possible to add a
new index method by defining the required interface routines and
then creating a row in pg_am  but that is
beyond the scope of this chapter (see ).

I think changing above to indicate something about handler function
could be useful.

+1

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

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

[HACKERS] Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

2015-10-03 Thread Amir Rohan

On 10/03/2015 03:50 PM, Amir Rohan wrote:
> On 10/03/2015 02:38 PM, Michael Paquier wrote:
>> On Fri, Oct 2, 2015 at 11:10 PM, Amir Rohan wrote:
>>> On 10/02/2015 03:33 PM, Michael Paquier wrote:
>>>
>>> Granted, you have to try fairly hard to shoot yourself in the leg,
>>> but since the solution is so simple, why not? If we never reuse ports
>>> within a single test, this goes away.
>>
>> Well, you can reuse the same port number in a test. Simply teardown
>> the existing node and then recreate a new one. I think that port
>> number assignment to a node should be transparent to the caller, in
>> our case the perl test script holding a scenario.
>>
> 
> What part of "Never assign the same port twice during one test"
> makes this "not transparent to the user"?
> 
> If you're thinking about parallel tests, I don't think you
> need to worry. Availability checks take care of one part,

Except now that I think of it, that's definitely a race:

Thread1: is_available(5432) -> True
Thread2: is_available(5432) -> True
Thread1: listen(5432) -> True
Thread2: listen(5432) -> #$@#$&@#$^&$#@&

I don't know if parallel tests are actually supported, though.
If theye are, you're right that this is a shared global
resource wrt concurrency.

Amir



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

Re: [HACKERS] Refactoring speculative insertion with unique indexes a little

2015-10-03 Thread Andres Freund

Hi,

On 2015-06-10 16:19:27 -0700, Peter Geoghegan wrote:
> Currently, speculative insertion (the INSERT ... ON CONFLICT DO UPDATE
> executor/storage infrastructure) uses checkUnique ==
> UNIQUE_CHECK_PARTIAL for unique indexes, which is a constant
> originally only used by deferred unique constraints. It occurred to me
> that this has a number of disadvantages:
> ...
> Attached patch updates speculative insertion along these lines.
> ...
> The patch also updates the AM interface documentation (the part
> covering unique indexes). It seems quite natural to me to document the
> theory of operation for speculative insertion there.

I'm not arguing against any of this - but I don't think this needs to be
on the 9.5 open items list. I plan to remove from there.

Greetings,

Andres Freund


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

Re: [HACKERS] Unicode mapping scripts cleanup

2015-10-03 Thread Andres Freund

Hi,

On 2015-09-01 00:13:07 -0400, Peter Eisentraut wrote:
> Here is a series of patches to clean up the Unicode mapping script
> business in src/backend/utils/mb/Unicode/.  It overlaps with the
> perlcritic work that I recently wrote about, except that these pieces
> are not strictly related to Perl, but wrong comments, missing makefile
> pieces, and such.

I looked through the patches, and afaics they're generally a good
idea. And they're all, IIUC, independent of us applying or not applying
updates. So why don't we go ahead with these changes?

I've marked this as returned-with-feedback for now, since there hasn't
been much progress lately.

Greetings,

Andres Freund

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

Re: [HACKERS] creating extension including dependencies

2015-10-03 Thread Andres Freund

On 2015-09-20 16:38:58 +0200, Petr Jelinek wrote:
> Here it is.

I went over the patch, trying to commit it. Changed a bunch of stylistic
issues (comments, NOTICE location, ...) . But also found a bug: Namely
cascade_parent was set wrongly in a bunch of situations: When an
extension has multiple dependencies the current name would end up
multiple times on the list, and more importantly a parent's
cascade_parent would be corrupted because the list was being modified
in-place in the child.

Here's an updated patch. Petr, could you please expand the test to
handle a bit more complex cascading setups?

Michael: Why did you exclude test_extensions in Mkvcbuild.pm?

Greetings,

Andres Freund


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

Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

2015-10-03 Thread Andres Freund

On 2015-08-27 01:12:54 +0200, Andreas Karlsson wrote:
> I think this is a real concern and one that I will look into, to see if it
> can be fixed with a reasonable amount of work.

This patch has been in waiting-for-author for a month. Marking it as
returned-with-feedback.

Greetings,

Andres Freund


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

Re: [HACKERS] [PATCH] postgres_fdw extension support

2015-10-03 Thread Andres Freund

Hi,

On 2015-10-01 11:46:43 +0900, Michael Paquier wrote:
> diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c
> index 7547ec2..864bf53 100644
> --- a/contrib/postgres_fdw/option.c
> +++ b/contrib/postgres_fdw/option.c
> @@ -19,6 +19,8 @@
>  #include "catalog/pg_foreign_table.h"
>  #include "catalog/pg_user_mapping.h"
>  #include "commands/defrem.h"
> +#include "commands/extension.h"
> +#include "utils/builtins.h"
>  
>  
>  /*
> @@ -124,6 +126,11 @@ postgres_fdw_validator(PG_FUNCTION_ARGS)
>errmsg("%s requires a 
> non-negative numeric value",
>   def->defname)));
>   }
> + else if (strcmp(def->defname, "extensions") == 0)
> + {
> + /* this must have already-installed extensions */

I don't understand that comment.

>   PG_RETURN_VOID();
> @@ -153,6 +160,8 @@ InitPgFdwOptions(void)
>   /* updatable is available on both server and table */
>   {"updatable", ForeignServerRelationId, false},
>   {"updatable", ForeignTableRelationId, false},
> + /* extensions is available on server */
> + {"extensions", ForeignServerRelationId, false},

awkward spelling in comment.


> +/*
> + * Parse a comma-separated string and return a List of the Oids of the
> + * extensions in the string. If an extension provided cannot be looked
> + * up in the catalog (it hasn't been installed or doesn't exist) then
> + * throw up an error.
> + */

s/throw up an error/throw an error/ or raise an error.

> +/*
> + * FDW-specific planner information kept in RelOptInfo.fdw_private for a
> + * foreign table.  This information is collected by 
> postgresGetForeignRelSize.
> + */
> +typedef struct PgFdwRelationInfo
> +{
> + /* baserestrictinfo clauses, broken down into safe and unsafe subsets. 
> */
> + List   *remote_conds;
> + List   *local_conds;
> +
> + /* Bitmap of attr numbers we need to fetch from the remote server. */
> + Bitmapset  *attrs_used;
> +
> + /* Cost and selectivity of local_conds. */
> + QualCostlocal_conds_cost;
> + Selectivity local_conds_sel;
> +
> + /* Estimated size and cost for a scan with baserestrictinfo quals. */
> + double  rows;
> + int width;
> + Coststartup_cost;
> + Costtotal_cost;
> +
> + /* Options extracted from catalogs. */
> + booluse_remote_estimate;
> + Costfdw_startup_cost;
> + Costfdw_tuple_cost;
> +
> + /* Optional extensions to support (list of oid) */

*oids

> +/* objid is the lookup key, must appear first */
> +typedef struct
> +{
> + /* extension the object appears within, or InvalidOid if none */
> + Oid objid;
> +} ShippableCacheKey;
> +
> +typedef struct
> +{
> + /* lookup key - must be first */
> + ShippableCacheKey key;
> + bool shippable;
> +} ShippableCacheEntry;
> +
> +/*
> + * Flush all cache entries when pg_foreign_server is updated.
> + */
> +static void
> +InvalidateShippableCacheCallback(Datum arg, int cacheid, uint32 hashvalue)
> +{
> + HASH_SEQ_STATUS status;
> + ShippableCacheEntry *entry;
> +
> + hash_seq_init(, ShippableCacheHash);
> + while ((entry = (ShippableCacheEntry *) hash_seq_search()) != 
> NULL)
> + {
> + if (hash_search(ShippableCacheHash,
> + (void *) >key,
> + HASH_REMOVE,
> + NULL) == NULL)
> + elog(ERROR, "hash table corrupted");
> + }
> +}

> +/*
> + * Returns true if given operator/function is part of an extension declared 
> in
> + * the server options.
> + */
> +static bool
> +lookup_shippable(Oid objnumber, List *extension_list)
> +{
> + static int nkeys = 1;
> + ScanKeyData key[nkeys];
> + HeapTuple tup;
> + Relation depRel;
> + SysScanDesc scan;
> + bool is_shippable = false;
> +
> + /* Always return false if we don't have any declared extensions */

Imo there's nothing declared here...

> + if (extension_list == NIL)
> + return false;
> +
> + /* We need this relation to scan */

Not sure what that means.

> + depRel = heap_open(DependRelationId, RowExclusiveLock);
> +
> + /*
> +  * Scan the system dependency table for all entries this object
> +  * depends on, then iterate through and see if one of them
> +  * is an extension declared by the user in the options
> +  */
> + ScanKeyInit([0],
> + Anum_pg_depend_objid,
> + BTEqualStrategyNumber, F_OIDEQ,
> + ObjectIdGetDatum(objnumber));
> +
> + scan = systable_beginscan(depRel, DependDependerIndexId, true,
> +

[HACKERS] Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

2015-10-03 Thread Michael Paquier

On Sat, Oct 3, 2015 at 9:50 PM, Amir Rohan  wrote:
> On 10/03/2015 02:38 PM, Michael Paquier wrote:
>> On Fri, Oct 2, 2015 at 11:10 PM, Amir Rohan wrote:
>>> On 10/02/2015 03:33 PM, Michael Paquier wrote:
> 4) Port assignment relies on liveness checks on running servers.
> If a server is shut down and a new instantiated, the port will get
> reused, data will get trashed, and various confusing things can happen.

 Right. The safest way to do that is to check in get_free_port if a
 port number is used by a registered node, and continue to loop in if
 that's the case. So done.

>>>
>>> That eliminates the "sweet and gentle" variant of the scenario, but it's
>>> susceptible to the "ABA problem":
>>> https://en.wikipedia.org/wiki/ABA_problem
>>> https://youtu.be/CmxkPChOcvw?t=786
>>
>> I learnt a new thing here. That's basically an existing problem even
>> with the existing perl test infrastructure relying on TestLib.pm when
>> tests are run in parallel. What we would need here is a global mapping
>> file storing all the port numbers used by all the nodes currently in
>> the tests.
>>
>
> Yeah, a poorman's way to ensure ports aren't reused (I wasn't very
> clear at top of post ) is something like:
>
> global nPortsAssigned = 0;
>
> AssignPort():
>basePort = BASE_PORT;  # the lowest port we use
>while(!available(basePort+nPortsAssigned)):
>basePort++
>
>nPortsAssigned++
>
>return basePort;
>
> It has its glaring faults, but would probably work ok.
> In any case, I'm sure you can do better.

Yeah, this would improve the exiting port lookup. I don't mind adding
a global variable in get_free_port for this purpose. This would
accelerate finding a free port in may cases for sure.

>>>
>>> Granted, you have to try fairly hard to shoot yourself in the leg,
>>> but since the solution is so simple, why not? If we never reuse ports
>>> within a single test, this goes away.
>>
>> Well, you can reuse the same port number in a test. Simply teardown
>> the existing node and then recreate a new one. I think that port
>> number assignment to a node should be transparent to the caller, in
>> our case the perl test script holding a scenario.
>>
>
> I was using you *never* want to reuse port numbers. That is, as long
> as the lib ensures we never reuse ports within one test, all kinds
> of corner cases are eliminated.

Hm, sure. Though I don't really why that would be mandatory to enforce
this condition as long as the list of ports occupied is in a single
place (as long as tests are not run in parallel...).

> 5) Servers are shutdown with -m 'immediate', which can lead to races
> in the script when archiving is turned on. That may be good for some
> tests, but there's no control over it.

 I hesitated with fast here actually. So changed this way. We would
 want as wall a teardown command to stop the node with immediate and
 unregister the node from the active list.

>>>
>>> In particular, I was shutting down an archiving node and the archiving
>>> was truncated. I *think* smart doesn't do this. But again, it's really
>>> that the test writer can't easily override, not that the default is wrong.
>>
>> Ah, OK. Then fast is just fine. It shuts down the node correctly.
>> "smart" would wait for all the current connections to finish but I am
>> wondering if it currently matters here: I don't see yet a clear use
>> case yet where it would make sense to have multi-threaded script... If
>> somebody comes back with a clear idea here perhaps we could revisit
>> that but it does not seem worth it now.
>>
>
> My mistake. Perhaps what would be useful though is a way
> to force "messy" shutdown. a kill -9, basically. It's a recovery
> test suite, right?.

That's what the teardown is aimed at having, the immediate stop mode
would play that fairly good enough. There has been a patch from Tom
Lane around to stop a server should its postmaster.pid be missing as
well...

> Other issues:
> 6. Directory structure, used one directory per thing but more logical
> to place all things related to an instance under a single directory,
> and name them according to role (57333_backup, and so on).

 Er, well. The first version of the patch did so, and then I switched
 to an approach closer to what the existing TAP facility is doing. But
 well let's simplify things a bit.

>>>
>>> I know, I know, but:
>>> 1) an instance is a "thing" in your script, so having its associated
>>> paraphernalia in one place makes more sense (maybe only to me).
>>> 2) That's often how folks (well, how I) arrange things in deployment,
>>> at least with archive/backup as symlinks to the nas.
>>>
>>> Alternatively, naming the dirs with a prefix (srv_foo_HASH,
>>> backup_foo_backupname_HASH, etc') would work as well.
>>
>> The useful portion about tempdir is that it cleans up itself
>> automatically should an error happen. It does not seem to

[HACKERS] Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

2015-10-03 Thread Amir Rohan

On 10/03/2015 02:38 PM, Michael Paquier wrote:
> On Fri, Oct 2, 2015 at 11:10 PM, Amir Rohan wrote:
>> On 10/02/2015 03:33 PM, Michael Paquier wrote:
>>> Any server instances created during the tests should never use a
>>> user-defined port for portability. Hence using those ports as keys
>>> just made sense. We could have for example custom names, that have
>>> port values assigned to them, but that's actually an overkill and
>>> complicates the whole facility.
>>>
>>
>> Something like:
>>
>> global nPortsAssigned = 0;
>> AssignPort() -> return is_ok(nPortsAssigned++)
>>
>> was what I used.
> 
> Why do you need that. Creating a node is in the most basic way a
> matter of calling make_master, make_*_standby where a port number, or
> identifier gets uniquely assigned to a node. The main point of the
> approach taken by this patch is to make port assignment transparent
> for the caller.
> 

See next.

>> Granted, you have to try fairly hard to shoot yourself in the leg,
>> but since the solution is so simple, why not? If we never reuse ports
>> within a single test, this goes away.
> 
> Well, you can reuse the same port number in a test. Simply teardown
> the existing node and then recreate a new one. I think that port
> number assignment to a node should be transparent to the caller, in
> our case the perl test script holding a scenario.
> 

What part of "Never assign the same port twice during one test"
makes this "not transparent to the user"?

If you're thinking about parallel test, I don't think you
need to worry. Availability checks take care of one part,
and the portnum-as-map-key-is-test-local takes care of the
other.

But, see next.

> 
 4) Port assignment relies on liveness checks on running servers.
 If a server is shut down and a new instantiated, the port will get
 reused, data will get trashed, and various confusing things can happen.
>>>
>>> Right. The safest way to do that is to check in get_free_port if a
>>> port number is used by a registered node, and continue to loop in if
>>> that's the case. So done.
>>>
>>
>> That eliminates the "sweet and gentle" variant of the scenario, but it's
>> susceptible to the "ABA problem":
>> https://en.wikipedia.org/wiki/ABA_problem
>> https://youtu.be/CmxkPChOcvw?t=786
> 
> I learnt a new thing here. That's basically an existing problem even
> with the existing perl test infrastructure relying on TestLib.pm when
> tests are run in parallel. What we would need here is a global mapping
> file storing all the port numbers used by all the nodes currently in
> the tests.
> 

Yeah, a poorman's way to ensure ports aren't reused (I wasn't very
clear at top of post ) is something like:

global nPortsAssigned = 0;

AssignPort():
   basePort = BASE_PORT;  # the lowest port we use
   while(!available(basePort+nPortsAssigned)):
   basePort++

   nPortsAssigned++

   return basePort;

It has its glaring faults, but would probably work ok.
In any case, I'm sure you can do better.

>>
>> Granted, you have to try fairly hard to shoot yourself in the leg,
>> but since the solution is so simple, why not? If we never reuse ports
>> within a single test, this goes away.
> 
> Well, you can reuse the same port number in a test. Simply teardown
> the existing node and then recreate a new one. I think that port
> number assignment to a node should be transparent to the caller, in
> our case the perl test script holding a scenario.
> 

I was using you *never* want to reuse port numbers. That is, as long
as the lib ensures we never reuse ports within one test, all kinds
of corner cases are eliminated.

 5) Servers are shutdown with -m 'immediate', which can lead to races
 in the script when archiving is turned on. That may be good for some
 tests, but there's no control over it.
>>>
>>> I hesitated with fast here actually. So changed this way. We would
>>> want as wall a teardown command to stop the node with immediate and
>>> unregister the node from the active list.
>>>
>>
>> In particular, I was shutting down an archiving node and the archiving
>> was truncated. I *think* smart doesn't do this. But again, it's really
>> that the test writer can't easily override, not that the default is wrong.
> 
> Ah, OK. Then fast is just fine. It shuts down the node correctly.
> "smart" would wait for all the current connections to finish but I am
> wondering if it currently matters here: I don't see yet a clear use
> case yet where it would make sense to have multi-threaded script... If
> somebody comes back with a clear idea here perhaps we could revisit
> that but it does not seem worth it now.
> 

My mistake. Perhaps what would be useful though is a way
to force "messy" shutdown. a kill -9, basically. It's a recovery
test suite, right?.

 Other issues:
 6. Directory structure, used one directory per thing but more logical
 to place all things related to an instance under a single directory,
 and name them according to role

Re: [HACKERS] Why can't we used CAPITAL LETTERS into replication slot_name?

2015-10-03 Thread Andres Freund

Hi,
On 2015-09-25 17:39:41 +0530, Rushabh Lathia wrote:
> PFA patch to fix the hint message.

The patched missed updating the regression test output files ;)

Committed and backpatched back to 9.4. Thanks!

- Andres


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

Re: [HACKERS] perlcritic

2015-10-03 Thread Andres Freund

Hi,

On 2015-08-31 23:57:25 -0400, Peter Eisentraut wrote:
> We now have 80+ Perl files in our tree, and it's growing.  Some of those
> files were originally written for Perl 4, and the coding styles and
> quality are quite, uh, divergent.  So I figured it's time to clean up
> that code a bit.  I ran perlcritic over the tree and cleaned up all the
> warnings at level 5 (the default, least severe).

As far as I can see we haven't really come to any conclusion in this
thread?

Peter, where do you want to go from here? As this patch has been in
waiting-for-author for a month, I'm marking it as
returned-with-feedback.

Greetings,

Andres Freund


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

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

2015-10-03 Thread Andres Freund

Hi!

On 2015-09-22 15:24:38 +, Syed, Rahila wrote:
> Please find attached patch with bugs reported by Thom and Sawada-san solved.

This thread has seen a bunch of reviews and new patch versions, but
doesnt yet seem to have arrived in a committable state. As the
commitfest ended and this patch has gotten attention, I'm moving the
entry to the next fest.

Greetings,

Andres Freund


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

Re: [HACKERS] Mention column name in error messages

2015-10-03 Thread Andres Freund

On 2015-09-15 12:00:25 +0200, Franck Verrot wrote:
> diff --git a/src/backend/parser/parse_target.c 
> b/src/backend/parser/parse_target.c
> index 1b3fcd6..78f82cd 100644
> --- a/src/backend/parser/parse_target.c
> +++ b/src/backend/parser/parse_target.c
> @@ -389,6 +389,17 @@ markTargetListOrigin(ParseState *pstate, TargetEntry 
> *tle,
>   * omits the column name list.  So we should usually prefer to use
>   * exprLocation(expr) for errors that can happen in a default INSERT.
>   */
> +
> +void
> +TransformExprCallback(void *arg)
> +{
> + TransformExprState *state = (TransformExprState *) arg;
> +
> + errcontext("coercion failed for column \"%s\" of type %s",
> + state->column_name,
> + format_type_be(state->expected_type));
> +}

Why is this not a static function?

>  Expr *
>  transformAssignedExpr(ParseState *pstate,
> Expr *expr,
> @@ -405,6 +416,14 @@ transformAssignedExpr(ParseState *pstate,
>   Oid attrcollation;  /* collation of target column */
>   ParseExprKind sv_expr_kind;
>  
> + ErrorContextCallback errcallback;
> + TransformExprState testate;

Why the newline between the declarations? Why none afterwards?

> diff --git a/src/include/parser/parse_target.h 
> b/src/include/parser/parse_target.h
> index a86b79d..5e12c12 100644
> --- a/src/include/parser/parse_target.h
> +++ b/src/include/parser/parse_target.h
> @@ -42,4 +42,11 @@ extern TupleDesc expandRecordVariable(ParseState *pstate, 
> Var *var,
>  extern char *FigureColname(Node *node);
>  extern char *FigureIndexColname(Node *node);
>  
> +/* Support for TransformExprCallback */
> +typedef struct TransformExprState
> +{
> + const char *column_name;
> + Oid expected_type;
> +} TransformExprState;

I see no need for this to be a struct defined in the header. Given that
TransformExprCallback isn't public, and the whole thing is specific to
TransformExprState...


My major complaint though, is that this doesn't contain any tests...


Could you update the patch to address these issues?

Greetings,

Andres Freund


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

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

2015-10-03 Thread Peter Geoghegan

On Fri, Oct 2, 2015 at 4:27 PM, Tom Lane  wrote:
> ... do you want to produce an updated patch?  I'm not planning to look at
> it until tomorrow anyway.

Attached, revised patch deals with the issues around removing the
query text file when garbage collection encounters trouble. There is
no reason to be optimistic about any error within gc_qtexts() not
recurring during a future garbage collection. OOM might be an
exception, but probably not, since gc_qtexts() is reached when a new
entry is created with a new query text, which in general makes OOM
progressively more likely.

Thanks for accommodating me here.

-- 
Peter Geoghegan
From 9295ecac575d6bed76e3b66a3b4cc1577517f2fc Mon Sep 17 00:00:00 2001
From: Peter Geoghegan 
Date: Tue, 29 Sep 2015 15:32:51 -0700
Subject: [PATCH] Fix pg_stat_statements garbage collection bugs

Previously, pg_stat_statements garbage collection could encounter
problems when many large, technically distinct queries were executed
without ever reaching the end of execution due to some error.  Garbage
collection could enter a state where it would never complete
successfully, with the external query text file left to grow
indefinitely.  For example, this could happen when a data integration
process fails repeatedly before finally succeeding (perhaps this process
could recur intermittently over a fairly long period).  Problematic
queries might have referenced what are technically distinct tables each
time, as when a CREATE TABLE is performed as part of the data
integration operation, giving each execution a unique queryid.

A spuriously high mean query length was previously possible due to
failed queries' sticky entries artificially inflating mean query length
during hash table eviction.  The mechanism by which garbage collection
is throttled to prevent thrashing could therefore put it off for far too
long.  By the time a garbage collection actually occurred, the file may
have become so big that it may not be possible for memory to be
allocated for all query texts.  Worse still, once the quasi-arbitrary
MaxAllocSize threshold was crossed, it became impossible for the system
to recover even with sufficient memory to allocate a buffer for all
query texts.

To fix these issues, pg_stat_statements no longer considers sticky
entries when calculating mean query text, which should prevent garbage
collection from getting an inaccurate idea of mean query text.
MaxAllocSize no longer caps the size of the buffer that is used to store
query texts in memory during garbage collection; the cap is changed to
MaxAllocHugeSize.  Garbage collection no longer tolerates OOMs (or
exceeding the new MaxAllocHugeSize cap) when allocating a buffer to
store all existing query texts.  Garbage collection now unlink()s the
query text file at the first sign of trouble, giving it a clean slate,
rather than assuming the issue will somehow later resolve itself, which
seemed over optimistic in general.

As a precaution, pg_stat_statements may now avoid storing very large
query texts after parse analysis but ahead of query execution (entries
for which no statistics exist yet).

Backpatch to 9.4, where external query text storage was introduced.
---
 contrib/pg_stat_statements/pg_stat_statements.c | 84 ++---
 1 file changed, 75 insertions(+), 9 deletions(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 59b8a2e..3e26213 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -103,6 +103,7 @@ static const uint32 PGSS_PG_MAJOR_VERSION = PG_VERSION_NUM / 100;
 #define USAGE_INIT(1.0)	/* including initial planning */
 #define ASSUMED_MEDIAN_INIT		(10.0)	/* initial assumed median usage */
 #define ASSUMED_LENGTH_INIT		1024	/* initial assumed mean query length */
+#define LENGTH_OUTLIER_FACTOR	5		/* mean query length multiplier */
 #define USAGE_DECREASE_FACTOR	(0.99)	/* decreased every entry_dealloc */
 #define STICKY_DECREASE_FACTOR	(0.50)	/* factor for sticky entries */
 #define USAGE_DEALLOC_PERCENT	5		/* free this % of entries at once */
@@ -,6 +1112,10 @@ pgss_hash_string(const char *str)
  * If jstate is not NULL then we're trying to create an entry for which
  * we have no statistics as yet; we just want to record the normalized
  * query string.  total_time, rows, bufusage are ignored in this case.
+ * A query text may not actually end up being stored in this case
+ * (storing oversized texts is sometimes avoided), and in any event is
+ * not guaranteed to still be around if and when actual results need to
+ * be stored in a subsequent pgss_store() call.
  */
 static void
 pgss_store(const char *query, uint32 queryId,
@@ -1159,7 +1164,27 @@ pgss_store(const char *query, uint32 queryId,
 		 */
 		if (jstate)
 		{
+			Size		mean_query_len = Max(ASSUMED_LENGTH_INIT,
+			 pgss->mean_query_len);
+

Re: [HACKERS] creating extension including dependencies

2015-10-03 Thread Andres Freund

On 2015-10-03 17:56:07 +0200, Petr Jelinek wrote:
> On 2015-10-03 17:16, Andres Freund wrote:
> >On 2015-10-03 17:15:54 +0200, Andres Freund wrote:
> >>Here's an updated patch. Petr, could you please expand the test to
> >>handle a bit more complex cascading setups?
> >
> 
> Okay, I changed the test to make the dependencies bit more complex - more
> than one dependency per extension + also non-cyclic interdependency. It
> still works as expected.

Ok, pushed this way. We can update the windows testing bit later if
necessary.

Thanks!

Andres


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

Re: [HACKERS] Refactoring speculative insertion with unique indexes a little

2015-10-03 Thread Peter Geoghegan

On Sat, Oct 3, 2015 at 6:38 AM, Andres Freund  wrote:
> I'm not arguing against any of this - but I don't think this needs to be
> on the 9.5 open items list. I plan to remove from there.

Obviously I don't think that this is a critical fix. I do think that
it would be nice to keep the branches in sync, and that might become a
bit more difficult after 9.5 is released.

-- 
Peter Geoghegan

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

Re: [HACKERS] Parallel Seq Scan

2015-10-03 Thread Robert Haas

On Fri, Oct 2, 2015 at 11:44 PM, Amit Kapila  wrote:
> On Thu, Oct 1, 2015 at 7:41 PM, Robert Haas  wrote:
>> Does heap_parallelscan_nextpage really need a pscan_finished output
>> parameter, or can it just return InvalidBlockNumber to indicate end of
>> scan?  I think the latter can be done and would be cleaner.
>
> I think we can do that way as well, however we have to keep the check
> of page == InvalidBlockNumber at all the callers to indicate finish
> of scan which made me write the function as it exists in patch.  However,
> I don't mind changing it the way you have suggested if you feel that would
> be cleaner.

I think it would.  I mean, you just end up testing the other thing instead.

>>  I think that heap_parallelscan_initialize() should taken an
>> additional Boolean argument, allow_sync.  It should decide whether to
>> actually perform a syncscan using the logic from initscan(), and then
>> it should store a phs_syncscan flag into the ParallelHeapScanDesc.
>> heap_beginscan_internal should set rs_syncscan based on phs_syncscan,
>> regardless of anything else.
>
> I think this will ensure that any future change in this area won't break the
> guarantee for sync scans for parallel workers, is that the reason you
> prefer to implement this function in the way suggested by you?

Basically.  It seems pretty fragile the way you have it now.

>> I think heap_parallel_rescan() is an unworkable API.  When rescanning
>> a synchronized scan, the existing logic keeps the original start-block
>> so that the scan's results are reproducible,
>
> It seems from the code comments in initscan, the reason for keeping
> previous startblock is to allow rewinding the cursor which doesn't hold for
> parallel scan.  We might or might not want to support such cases with
> parallel query, but even if we want to there is a way we can do with
> current logic (as mentioned in one of my replies below).

You don't need to rewind a cursor; you just need to restart the scan.
So for example if we were on the inner side of a NestLoop, this would
be a real issue.

>> but no longer reports the
>> scan position since we're presumably out of step with other backends.
>
> Is it true for all form of rescans or are you talking about some
> case (like SampleScan) in particular?  As per my reading of code
> (and verified by debugging that code path), it doesn't seem to be true
> for rescan in case of seqscan.

I think it is:

if (keep_startblock)
{
/*
 * When rescanning, we want to keep the previous startblock setting,
 * so that rewinding a cursor doesn't generate surprising results.
 * Reset the active syncscan setting, though.
 */
scan->rs_syncscan = (allow_sync && synchronize_seqscans);
}

>> This isn't going to work at all with this API.  I don't think you can
>> swap out the ParallelHeapScanDesc for another one once you've
>> installed it;
>>
>
> Sure, but if we really need some such parameters like startblock position,
> then we can preserve those in ScanDesc.

Sure, we could transfer the information out of the
ParallelHeapScanDesc and then transfer it back into the new one, but I
have a hard time thinking that's a good design.

> PARAM_EXEC parameters will be used to support initPlan in parallel query (as
> it is done in the initial patch), so it seems to me this is the main
> downside of
> this approach.  I think rather than trying to come up with various possible
> solutions for this problem, lets prohibit sync scans with parallel query if
> you
> see some problem with the suggestions made by me above.  Do you see any
> main use case getting hit due to non support of sync scans with
> parallel seq. scan?

Yes.  Synchronized scans are particularly important with large tables,
and those are the kind you're likely to want to use a parallel
sequential scan on.

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


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

Re: [HACKERS] [PATCH v1] GSSAPI encryption support

2015-10-03 Thread Andres Freund

Hi,

I quickly read through the patch, trying to understand what exactly is
happening here. To me the way the patch is split doesn't make much sense
- I don't mind incremental patches, but right now the steps don't
individually make sense.

On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
> +#include 

postgres.h should be the first header included.

> +size_t
> +be_gss_encrypt(Port *port, char msgtype, const char **msgptr, size_t len)
> +{
> + OM_uint32 major, minor;
> + gss_buffer_desc input, output;
> + uint32 len_n;
> + int conf;
> + char *ptr = *((char **)msgptr);

trivial nitpick, missing spaces...

> +int
> +be_gss_inplace_decrypt(StringInfo inBuf)
> +{
> + OM_uint32 major, minor;
> + gss_buffer_desc input, output;
> + int qtype, conf;
> + size_t msglen = 0;
> +
> + input.length = inBuf->len;
> + input.value = inBuf->data;
> + output.length = 0;
> + output.value = NULL;
> +
> + major = gss_unwrap(, MyProcPort->gss->ctx, , ,
> +, NULL);
> + if (GSS_ERROR(major))
> + {
> + pg_GSS_error(ERROR, gettext_noop("wrapping GSS message failed"),
> +  major, minor);
> + return -1;
> + }
> + else if (conf == 0)
> + {
> + ereport(COMMERROR,
> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
> +  errmsg("Expected GSSAPI confidentiality but it 
> was not received")));
> + return -1;
> + }

Hm. Aren't we leaking the gss buffer here?

> + qtype = ((char *)output.value)[0]; /* first byte is message type */
> + inBuf->len = output.length - 5; /* message starts */
> +
> + memcpy((char *), ((char *)output.value) + 1, 4);
> + msglen = ntohl(msglen);
> + if (msglen - 4 != inBuf->len)
> + {
> + ereport(COMMERROR,
> + (errcode(ERRCODE_PROTOCOL_VIOLATION),
> +  errmsg("Length value inside GSSAPI-encrypted 
> packet was malformed")));
> + return -1;
> + }

and here?

Arguably it doesn't matter that much, since we'll usually disconnect
around here, but ...

> + memcpy(inBuf->data, ((char *)output.value) + 5, inBuf->len);
> + inBuf->data[inBuf->len] = '\0'; /* invariant */
> + gss_release_buffer(, );
> +
> + return qtype;
> +}

> diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
> index a4b37ed..5a929a8 100644
> --- a/src/backend/libpq/pqcomm.c
> +++ b/src/backend/libpq/pqcomm.c
> @@ -1485,6 +1485,19 @@ socket_putmessage(char msgtype, const char *s, size_t 
> len)
>  {
>   if (DoingCopyOut || PqCommBusy)
>   return 0;
> +
> +#ifdef ENABLE_GSS
> + /* Do not wrap auth requests. */
> + if (MyProcPort->hba->auth_method == uaGSS && gss_encrypt &&
> + msgtype != 'R' && msgtype != 'g')
> + {
> + len = be_gss_encrypt(MyProcPort, msgtype, , len);
> + if (len < 0)
> + goto fail;
> + msgtype = 'g';
> + }
> +#endif

Putting encryption specific code here feels rather wrong to me.


> diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
> index 6171ef3..58712fc 100644
> --- a/src/include/libpq/libpq-be.h
> +++ b/src/include/libpq/libpq-be.h
> @@ -30,6 +30,8 @@
>  #endif
>  
>  #ifdef ENABLE_GSS
> +#include "lib/stringinfo.h"
> +

Conditionally including headers providing generic infrastructure strikes
me as a recipe for build failures in different configurations.

>  /* TCP keepalives configuration. These are no-ops on an AF_UNIX socket. */
> diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
> index c408e5b..e788cc8 100644
> --- a/src/include/libpq/libpq.h
> +++ b/src/include/libpq/libpq.h
> @@ -99,4 +99,8 @@ extern char *SSLCipherSuites;
>  extern char *SSLECDHCurve;
>  extern bool SSLPreferServerCiphers;
>  
> +#ifdef ENABLE_GSS
> +extern bool gss_encrypt;
> +#endif

> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
>  
> +#ifdef ENABLE_GSS
> +static void assign_gss_encrypt(bool newval, void *extra);
> +#endif
> +
>  
>  /*
>   * Options for enum values defined in this module.
> @@ -1618,6 +1622,15 @@ static struct config_bool ConfigureNamesBool[] =
>   NULL, NULL, NULL
>   },
>  
> + {
> + {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
> +  gettext_noop("Whether client wants encryption for this 
> connection."),
> +  NULL,
> +  GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
> + },
> + _encrypt, false, NULL, assign_gss_encrypt, NULL
> + },
> +
>   /* End-of-list marker */
>   {
>   {NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
> @@ -10114,4 +10127,10 @@ show_log_file_mode(void)
>   return buf;
>  }

The guc should always be present, not just when gss is built in. It

Re: [HACKERS] jsonb_set array append hack?

2015-10-03 Thread Peter Geoghegan

On Mon, Sep 21, 2015 at 2:21 PM, Andrew Dunstan  wrote:
>> Thanks for the explanation. So, basically, it should be like this, am I
>> right?
>>
>> postgres=# SELECT jsonb_set(
>> '{"name": "Joe", "vehicle_types": ["car", "van"]}'::jsonb,
>> '{vehicle_types, nonsense}',
>> '"motorcycle"', true);
>> ERROR:  path element at the position 2 is not an integer
>
> That seems reasonable. For that matter, we should probably disallow NULL
> path elements also, shouldn't we?

Are you planning on getting this in by Monday, Andrew? It would be
nice to have this fixed going into beta.

-- 
Peter Geoghegan


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

Re: [HACKERS] jsonb_set array append hack?

2015-10-03 Thread Andrew Dunstan




On 10/03/2015 04:49 PM, Peter Geoghegan wrote:

On Mon, Sep 21, 2015 at 2:21 PM, Andrew Dunstan  wrote:

Thanks for the explanation. So, basically, it should be like this, am I
right?

postgres=# SELECT jsonb_set(
 '{"name": "Joe", "vehicle_types": ["car", "van"]}'::jsonb,
 '{vehicle_types, nonsense}',
 '"motorcycle"', true);
ERROR:  path element at the position 2 is not an integer

That seems reasonable. For that matter, we should probably disallow NULL
path elements also, shouldn't we?

Are you planning on getting this in by Monday, Andrew? It would be
nice to have this fixed going into beta.




Yeah, will look at it tonight or tomorrow.

cheers

andrew


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

Re: [HACKERS] Idea for improving buildfarm robustness

2015-10-03 Thread Josh Berkus

On 10/02/2015 09:39 PM, Tom Lane wrote:
> I wrote:
>> Here's a rewritten patch that looks at postmaster.pid instead of
>> pg_control.  It should be effectively the same as the prior patch in terms
>> of response to directory-removal cases, and it should also catch many
>> overwrite cases.
> 
> BTW, my thought at the moment is to wait till after next week's releases
> to push this in.  I think it's probably solid, but it doesn't seem like
> it's worth taking the risk of pushing shortly before a wrap date.
> 
> If anyone wants to argue for including it in the releases, speak up ...

Wait, we're backpatching this?


-- 
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com


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

Re: [HACKERS] [PATCH] postgres_fdw extension support

2015-10-03 Thread Paul Ramsey

Andres, 
Thanks so much for the review!

I put all changes relative to your review here if you want a nice colorized 
place to check

https://github.com/pramsey/postgres/commit/ed33e7489601e659f436d6afda3cce28304eba50

On October 3, 2015 at 8:49:04 AM, Andres Freund (and...@anarazel.de) wrote:

> +  /* this must have already-installed extensions */ 

I don't understand that comment. 
Fixed, I hope.

> +  /* extensions is available on server */ 
> +  {"extensions", ForeignServerRelationId, false}, 

awkward spelling in comment. 
Fixed, I hope.

> + * throw up an error. 
> + */ 

s/throw up an error/throw an error/ or raise an error. 
But “throw up” is so evocative :) fixed.

> + /* Optional extensions to support (list of oid) */ 

*oids 
Fixed.

> + /* Always return false if we don't have any declared extensions */ 

Imo there's nothing declared here... 
Changed...

> + if (extension_list == NIL) 
> +  return false; 
> + 
> + /* We need this relation to scan */ 

Not sure what that means. 
Me neither, removed.

> +  if (foundDep->deptype == DEPENDENCY_EXTENSION && 
> +  list_member_oid(extension_list, foundDep->refobjid)) 
> +  { 
> +  is_shippable = true; 
> +  break; 
> +  } 
> + } 

Hm. 
I think this “hm” is addressed lower down.

> + /* Always return false if we don't have any declared extensions */ 
> + if (extension_list == NIL) 
> +  return false; 

I again dislike declared here ;) 
Altered.

> + key.objid = objnumber; 

Hm. Oids can conflict between different system relations. Shouldn't the 
key be over class (i.e. pg_proc, pg_type etc.) and object id? 
I’ve changed the lookup to use class/obj instead. I’m *hoping* I don’t get 
burned by it, but it regresses fine at least. Each call to is_shippable now has 
a hard-coded class oid in it depending on the context of the call. It seemed 
like the right way to do it.

> +  /* 
> +  * Right now "shippability" is exclusively a function of whether 
> +  * the obj (proc/op/type) is in an extension declared by the user. 
> +  * In the future we could additionally have a whitelist of functions 
> +  * declared one at a time. 
> +  */ 
> +  bool shippable = lookup_shippable(objnumber, extension_list); 
> + 
> +  entry = (ShippableCacheEntry *) 
> +  hash_search(ShippableCacheHash, 
> +  (void *) , 
> +  HASH_ENTER, 
> +  NULL); 
> + 
> +  entry->shippable = shippable; 
> + } 

I suggest adding a comment that we consciously are only HASH_ENTERing 
the key after doing the lookup_shippable(), to avoid the issue that the 
lookup might accept cache invalidations and thus delete the entry again. 
I’m not understanding this one. I only ever delete cache entries in bulk, when 
InvalidateShippableCacheCallback gets called on updates to the foreign server 
definition. I must not be quite understanding your gist.

Thanks!

P

20151003_postgres_fdw_extensions.patch
Description: Binary data

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

Re: [HACKERS] Parallel Seq Scan

2015-10-03 Thread Amit Kapila

On Sat, Oct 3, 2015 at 11:35 PM, Robert Haas  wrote:
>
> On Fri, Oct 2, 2015 at 11:44 PM, Amit Kapila 
wrote:
> > On Thu, Oct 1, 2015 at 7:41 PM, Robert Haas 
wrote:
> >> Does heap_parallelscan_nextpage really need a pscan_finished output
> >> parameter, or can it just return InvalidBlockNumber to indicate end of
> >> scan?  I think the latter can be done and would be cleaner.
> >
> > I think we can do that way as well, however we have to keep the check
> > of page == InvalidBlockNumber at all the callers to indicate finish
> > of scan which made me write the function as it exists in patch.
However,
> > I don't mind changing it the way you have suggested if you feel that
would
> > be cleaner.
>
> I think it would.  I mean, you just end up testing the other thing
instead.
>

No issues, will change in next version of patch.

> >>  I think that heap_parallelscan_initialize() should taken an
> >> additional Boolean argument, allow_sync.  It should decide whether to
> >> actually perform a syncscan using the logic from initscan(), and then
> >> it should store a phs_syncscan flag into the ParallelHeapScanDesc.
> >> heap_beginscan_internal should set rs_syncscan based on phs_syncscan,
> >> regardless of anything else.
> >
> > I think this will ensure that any future change in this area won't
break the
> > guarantee for sync scans for parallel workers, is that the reason you
> > prefer to implement this function in the way suggested by you?
>
> Basically.  It seems pretty fragile the way you have it now.
>

Okay, in that case I will change it as per your suggestion.

> >> I think heap_parallel_rescan() is an unworkable API.  When rescanning
> >> a synchronized scan, the existing logic keeps the original start-block
> >> so that the scan's results are reproducible,
> >
> > It seems from the code comments in initscan, the reason for keeping
> > previous startblock is to allow rewinding the cursor which doesn't hold
for
> > parallel scan.  We might or might not want to support such cases with
> > parallel query, but even if we want to there is a way we can do with
> > current logic (as mentioned in one of my replies below).
>
> You don't need to rewind a cursor; you just need to restart the scan.
> So for example if we were on the inner side of a NestLoop, this would
> be a real issue.
>

Sorry, but I am not able to see the exact issue you have in mind for
NestLoop,
if we don't preserve the start block position for parallel scan.  The code
in
discussion is added by below commit which doesn't indicate any such problem.

**
commit 61dd4185ffb034a22b4b40425d56fe37e7178488
Author: Tom Lane   Thu Jun 11 00:24:16 2009
Committer: Tom Lane   Thu Jun 11 00:24:16 2009

Keep rs_startblock the same during heap_rescan, so that a rescan of a
SeqScan
node starts from the same place as the first scan did.  This avoids
surprising
behavior of scrollable and WITH HOLD cursors, as seen in Mark Kirkwood's bug
report of yesterday.

It's not entirely clear whether a rescan should be forced to drop out of the
syncscan mode, but for the moment I left the code behaving the same on that
point.  Any change there would only be a performance and not a correctness
issue, anyway.
**

> >> but no longer reports the
> >> scan position since we're presumably out of step with other backends.
> >
> > Is it true for all form of rescans or are you talking about some
> > case (like SampleScan) in particular?  As per my reading of code
> > (and verified by debugging that code path), it doesn't seem to be true
> > for rescan in case of seqscan.
>
> I think it is:
>
> if (keep_startblock)
> {
> /*
>  * When rescanning, we want to keep the previous startblock
setting,
>  * so that rewinding a cursor doesn't generate surprising results.
>  * Reset the active syncscan setting, though.
>  */
> scan->rs_syncscan = (allow_sync && synchronize_seqscans);
> }
>

Okay, but this code doesn't indicate that scan position will not be
reported.
It just resets the flag based on which scan position is reported.  It won't
change during rescan as compare to first scan unless the value of
synchronize_seqscans is changed in-between.  So under some special
circumstances, it may change but not as a general rule.


> >> This isn't going to work at all with this API.  I don't think you can
> >> swap out the ParallelHeapScanDesc for another one once you've
> >> installed it;
> >>
> >
> > Sure, but if we really need some such parameters like startblock
position,
> > then we can preserve those in ScanDesc.
>
> Sure, we could transfer the information out of the
> ParallelHeapScanDesc and then transfer it back into the new one, but I
> have a hard time thinking that's a good design.
>

I also don't think that is the best way to achieve it, but on the other hand
doing it the other way will pose some restrictions

[HACKERS] Draft release notes are up for review

2015-10-03 Thread Tom Lane

I've prepared first-draft release notes for Monday's minor releases:

http://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=01ef33701bf6d475deeb550c18a5c3fd698c9623

(If you don't like reading raw SGML, it should be up at
http://www.postgresql.org/docs/devel/static/release.html
a couple of hours from now, after guaibasaurus does its next buildfarm
run.)

As in the past, everything is just in the 9.4.5 subsection for now,
and I'll slice-and-dice it to construct the entries for the older
branches later.  Please send any comments before, say, 2200 UTC tomorrow.

regards, tom lane


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

Re: [HACKERS] Idea for improving buildfarm robustness

2015-10-03 Thread Tom Lane

Josh Berkus  writes:
> On 10/02/2015 09:39 PM, Tom Lane wrote:
>> I wrote:
>>> Here's a rewritten patch that looks at postmaster.pid instead of
>>> pg_control.  It should be effectively the same as the prior patch in terms
>>> of response to directory-removal cases, and it should also catch many
>>> overwrite cases.

>> BTW, my thought at the moment is to wait till after next week's releases
>> to push this in.  I think it's probably solid, but it doesn't seem like
>> it's worth taking the risk of pushing shortly before a wrap date.
>> 
>> If anyone wants to argue for including it in the releases, speak up ...

> Wait, we're backpatching this?

Of course.  It's not going to do much for buildfarm stability if it's
only in HEAD.

regards, tom lane


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

Re: [HACKERS] creating extension including dependencies

2015-10-03 Thread Michael Paquier

On Sun, Oct 4, 2015 at 12:15 AM, Andres Freund  wrote:
> On 2015-09-20 16:38:58 +0200, Petr Jelinek wrote:
> Michael: Why did you exclude test_extensions in Mkvcbuild.pm?

test_extensions contains nothing that should be compiled, only things
that should be installed.
-- 
Michael


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

Re: [HACKERS] pgbench stats per script & other stuff

2015-10-03 Thread Fabien COELHO



Here is a v10, which is a rebase because of the "--progress-timestamp" option 
addition.


Here is a v11, which is a rebase after some recent changes committed to 
pgbench.


--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 0ac40f1..e3a90e5 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -261,6 +261,25 @@ pgbench  options  dbname
 benchmarking arguments:
 
 
+ 
+  -b scriptname[@weight]
+  --builtin scriptname[@weight]
+  
+   
+Add the specified builtin script to the list of executed scripts.
+An optional integer weight after @ allows to adjust the
+probability of drawing the test.
+Available builtin scripts are: tpcb-like,
+simple-update and select-only.
+The provided scriptname needs only to be a prefix
+of the builtin name, hence simp would be enough to select
+simple-update.
+With special name list, show the list of builtin scripts
+and exit immediately.
+   
+  
+ 
+
 
  
   -c clients
@@ -307,14 +326,15 @@ pgbench  options  dbname
  
 
  
-  -f filename
-  --file=filename
+  -f filename[@weight]
+  --file=filename[@weight]
   

-Read transaction script from filename.
+Add a transaction script read from filename to
+the list of executed scripts.
+An optional integer weight after @ allows to adjust the
+probability of drawing the test.
 See below for details.
--N, -S, and -f
-are mutually exclusive.

   
  
@@ -404,10 +424,7 @@ pgbench  options  dbname
   --skip-some-updates
   

-Do not update pgbench_tellers and
-pgbench_branches.
-This will avoid update contention on these tables, but
-it makes the test case even less like TPC-B.
+Shorthand for -b simple-update@1.

   
  
@@ -512,9 +529,9 @@ pgbench  options  dbname
 Report the specified scale factor in pgbench's
 output.  With the built-in tests, this is not necessary; the
 correct scale factor will be detected by counting the number of
-rows in the pgbench_branches table.  However, when testing
-custom benchmarks (-f option), the scale factor
-will be reported as 1 unless this option is used.
+rows in the pgbench_branches table.
+However, when testing only custom benchmarks (-f option),
+the scale factor will be reported as 1 unless this option is used.

   
  
@@ -524,7 +541,7 @@ pgbench  options  dbname
   --select-only
   

-Perform select-only transactions instead of TPC-B-like test.
+Shorthand for -b select-only@1.

   
  
@@ -674,7 +691,20 @@ pgbench  options  dbname
   What is the Transaction Actually Performed in pgbench?
 
   
-   The default transaction script issues seven commands per transaction:
+   Pgbench executes test scripts chosen randomly from a specified list.
+   They include built-in scripts with -b and
+   user-provided custom scripts with -f.
+   Each script may be given a relative weight specified after a
+   @ so as to change its drawing probability.
+   The default weight is 1.
+ 
+
+  
+   The default builtin transaction script (also invoked with -b tpcb-like)
+   issues seven commands per transaction over randomly chosen aid,
+   tid, bid and balance.
+   The scenario is inspired by the TPC-B benchmark, but is not actually TPC-B,
+   hence the name.
   
 
   
@@ -688,9 +718,15 @@ pgbench  options  dbname
   
 
   
-   If you specify -N, steps 4 and 5 aren't included in the
-   transaction.  If you specify -S, only the SELECT is
-   issued.
+   If you select the simple-update builtin (also -N),
+   steps 4 and 5 aren't included in the transaction.
+   This will avoid update contention on these tables, but
+   it makes the test case even less like TPC-B.
+  
+
+  
+   If you select the select-only builtin (also -S),
+   only the SELECT is issued.
   
  
 
@@ -702,10 +738,7 @@ pgbench  options  dbname
benchmark scenarios by replacing the default transaction script
(described above) with a transaction script read from a file
(-f option).  In this case a transaction
-   counts as one execution of a script file.  You can even specify
-   multiple scripts (multiple -f options), in which
-   case a random one of the scripts is chosen each time a client session
-   starts a new transaction.
+   counts as one execution of a script file.
   
 
   
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index f2d435b..0e56ed7 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -164,12 +164,11 @@ bool		use_log;			/* log transaction latencies to a file */
 bool		use_quiet;			/* quiet logging onto stderr */
 int			agg_interval;		/* log aggregates instead of individual

Re: [HACKERS] WIP: Rework access method interface

2015-10-03 Thread Amit Kapila

On Fri, Oct 2, 2015 at 8:14 PM, Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:
>
>
> I agree about staying with one SQL-visible function.
>
> Other changes:
>  * Documentation reflects interface changes.
>  * IndexAmRoutine moved from CacheMemoryContext to indexcxt.
>  * Various minor formatting improvements.
>  * Error messages are corrected.
>

Few assorted comments:

1.
+  * Get IndexAmRoutine structure from access method oid.
+  */
+ IndexAmRoutine *
+ GetIndexAmRoutine(Oid
amoid)
+ {
+ IndexAmRoutine *result;
+ HeapTuple tuple;
+ regproc
amhandler;
+
+ tuple = SearchSysCache1(AMOID, ObjectIdGetDatum(amoid));
+ if (!HeapTupleIsValid
(tuple))
+ elog(ERROR, "cache lookup failed for access method %u",
+
amoid);
+ amhandler = ((Form_pg_am)GETSTRUCT(tuple))->amhandler;
+
+ if (!RegProcedureIsValid
(amhandler))
+ elog(ERROR, "invalid %u regproc", amhandler);

I have noticed that currently, the above kind of error is reported slightly
differently as in below code:
if (!RegProcedureIsValid(procOid)) \
elog(ERROR, "invalid %s regproc", CppAsString
(pname)); \

If you feel it is better to do the way as it is in current code, then you
can change accordingly.

2.

 Access methods that always return entries in the natural ordering
 of their data (such
as btree) should set
!pg_am.amcanorder to true.
 Currently, such
access methods must use btree-compatible strategy
 numbers for their equality and ordering operators.

--- 545,551 

 Access methods that always return entries in the natural
ordering
 of their data (such as btree) should set
!amcanorder to true.

Currently, such access methods must use btree-compatible strategy
 numbers for their equality and
ordering operators.

Isn't it better to use structure while referencing the field of it?

3.
!Handler function must be written in a compiled language such as C,
using
!the version-1 interface.

Here, it is not completely clear, what do you refer to as version-1
interface.

4.
xindex.sgml
Index Methods and Operator Classes
..
It is possible to add a
   new index method by defining the required interface routines and
   then creating a row in pg_am  but that is
   beyond the scope of this chapter (see ).

I think changing above to indicate something about handler function
could be useful.

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

Re: [HACKERS] creating extension including dependencies

Re: [HACKERS] ON CONFLICT issues around whole row vars,

[HACKERS] Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

Re: [HACKERS] Idea for improving buildfarm robustness

Re: [HACKERS] ON CONFLICT DO UPDATE using EXCLUDED.column gives an error about mismatched types

Re: [HACKERS] creating extension including dependencies

Re: [HACKERS] WIP: Rework access method interface

Re: [HACKERS] WIP: Rework access method interface

[HACKERS] Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

Re: [HACKERS] Refactoring speculative insertion with unique indexes a little

Re: [HACKERS] Unicode mapping scripts cleanup

Re: [HACKERS] creating extension including dependencies

Re: [HACKERS] [PATCH] Reload SSL certificates on SIGHUP

Re: [HACKERS] [PATCH] postgres_fdw extension support

[HACKERS] Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

[HACKERS] Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

Re: [HACKERS] Why can't we used CAPITAL LETTERS into replication slot_name?

Re: [HACKERS] perlcritic

Re: [HACKERS] [PROPOSAL] VACUUM Progress Checker.

Re: [HACKERS] Mention column name in error messages

Re: [HACKERS] Less than ideal error reporting in pg_stat_statements

Re: [HACKERS] creating extension including dependencies

Re: [HACKERS] Refactoring speculative insertion with unique indexes a little

Re: [HACKERS] Parallel Seq Scan

Re: [HACKERS] [PATCH v1] GSSAPI encryption support

Re: [HACKERS] jsonb_set array append hack?

Re: [HACKERS] jsonb_set array append hack?

Re: [HACKERS] Idea for improving buildfarm robustness

Re: [HACKERS] [PATCH] postgres_fdw extension support

Re: [HACKERS] Parallel Seq Scan

[HACKERS] Draft release notes are up for review

Re: [HACKERS] Idea for improving buildfarm robustness

Re: [HACKERS] creating extension including dependencies

Re: [HACKERS] pgbench stats per script & other stuff

Re: [HACKERS] WIP: Rework access method interface

35 matches

Site Navigation

Mail list logo

Footer information