Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW

[Etsuro Fujita]

On 2016/02/22 20:13, Rushabh Lathia wrote:

I did another round of review for the latest patch and well as performed
the sanity test, and
haven't found any functional issues. Found couple of issue, see in-line
comments
for the same.


Thanks!


On Thu, Feb 18, 2016 at 3:15 PM, Etsuro Fujita
> wrote:

On 2016/02/12 21:19, Etsuro Fujita wrote:



+   /* Check point 1 */
+   if (operation == CMD_INSERT)
+   return false;
+
+   /* Check point 2 */
+   if (nodeTag(subplan) != T_ForeignScan)
+   return false;
+
+   /* Check point 3 */
+   if (subplan->qual != NIL)
+   return false;
+
+   /* Check point 4 */
+   if (operation == CMD_UPDATE)

These comments are referring to something in the function header
further up, but you could instead just delete the stuff from the
header and mention the actual conditions here.



Will fix.



Done.

The patch doesn't allow the postgres_fdw to push down an
UPDATE/DELETE on a foreign join, so I added one more condition here
not to handle such cases.  (I'm planning to propose a patch to
handle such cases, in the next CF.)



I think we should place the checking foreign join condition before the
target columns, as foreign join condition is less costly then the target
columns.


Agreed.


Other changes:



* I keep Rushabh's code change that we call PlanDMLPushdown only
when all the required APIs are available with FDW, but for
CheckValidResultRel, I left the code as-is (no changes to that
function), to match the docs saying that the FDW needs to provide
the DML pushdown callback functions together with existing
table-updating functions such as ExecForeignInsert,
ExecForeignUpdate and ExecForeignDelete.



I think we should also update the CheckValidResultRel(), because even
though ExecForeignInsert,
ExecForeignUpdate and ExecForeignDelete not present, FDW still can perform
UPDATE/DELETE/INSERT using DML Pushdown APIs. Lets take committer's view
on this.


OK


PFA update patch, which includes changes into postgresPlanDMLPushdown()
to check for join
condition before target columns and also fixed couple of whitespace issues.


Thanks again for updating the patch and fixing the issues!

Best regards,
Etsuro Fujita




--
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] postgres_fdw vs. force_parallel_mode on ppc

[Tom Lane]

Robert Haas  writes:
> On Tue, Feb 23, 2016 at 2:06 AM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> Foreign tables are supposed to be categorically excluded from
>>> parallelism.  Not sure why that's not working in this instance.

>> BTW, I wonder where you think that's supposed to be enforced, because
>> I sure can't find any such logic.

> RTEs are checked in set_rel_consider_parallel(), and I thought there
> was a check there related to foreign tables, but there isn't.  Oops.

Even if there were, it would not fix this bug, because AFAICS the only
thing that set_rel_consider_parallel is chartered to do is set the
per-relation consider_parallel flag.  The failure that is happening in
that regression test with force_parallel_mode turned on happens because
standard_planner plasters a Gather node at the top of the plan, causing
the whole plan including the FDW access to happen inside a parallel
worker.  The only way to prevent that is to clear the
wholePlanParallelSafe flag, which as far as I can tell (not that any of
this is documented worth a damn) isn't something that
set_rel_consider_parallel is supposed to do.

It looks to me like there is a good deal of fuzzy thinking here about the
difference between locally parallelizable and globally parallelizable
plans, ie Gather at the top vs Gather somewhere else.  I also note with
dismay that turning force_parallel_mode on seems to pretty much disable
any testing of local parallelism.

> In view of 69d34408e5e7adcef8ef2f4e9c4f2919637e9a06, we shouldn't
> blindly assume that foreign scans are not parallel-safe, but we can't
> blindly assume the opposite either.  Maybe we should assume that the
> foreign scan is parallel-safe only if one or more of the new methods
> introduced by the aforementioned commit are set, but actually that
> doesn't seem quite right.  That would tell us whether the scan itself
> can be parallelized, not whether it's safe to run serially but within
> a parallel worker.  I think maybe we need a new FDW API that gets
> called from set_rel_consider_parallel() with the root, rel, and rte as
> arguments and which can return a Boolean.  If the callback is not set,
> assume false.

Meh.  As things stand, postgres_fdw would have to aver that it can't ever
be safely parallelized, which doesn't seem like a very satisfactory answer
even if there are other FDWs that work differently (and which would those
be?  None that use a socket-style connection to an external server.)

The commit you mention above seems to me to highlight the dangers of
accepting hook patches with no working use-case to back them up.
AFAICT it's basically useless for typical FDWs because of this
multiple-connection problem.

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] postgres_fdw vs. force_parallel_mode on ppc

[Robert Haas]

On Tue, Feb 23, 2016 at 2:06 AM, Tom Lane  wrote:
> Robert Haas  writes:
>>> Foreign tables are supposed to be categorically excluded from
>>> parallelism.  Not sure why that's not working in this instance.
>
> BTW, I wonder where you think that's supposed to be enforced, because
> I sure can't find any such logic.
>
> I suppose that has_parallel_hazard() would be the logical place to
> notice foreign tables, but it currently doesn't even visit RTEs,
> much less contain any code to check if their tables are foreign.
> Or did you have another place in mind to do that?

RTEs are checked in set_rel_consider_parallel(), and I thought there
was a check there related to foreign tables, but there isn't.  Oops.
In view of 69d34408e5e7adcef8ef2f4e9c4f2919637e9a06, we shouldn't
blindly assume that foreign scans are not parallel-safe, but we can't
blindly assume the opposite either.  Maybe we should assume that the
foreign scan is parallel-safe only if one or more of the new methods
introduced by the aforementioned commit are set, but actually that
doesn't seem quite right.  That would tell us whether the scan itself
can be parallelized, not whether it's safe to run serially but within
a parallel worker.  I think maybe we need a new FDW API that gets
called from set_rel_consider_parallel() with the root, rel, and rte as
arguments and which can return a Boolean.  If the callback is not set,
assume false.

-- 
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] postgres_fdw vs. force_parallel_mode on ppc

[Tom Lane]

Thomas Munro  writes:
> On Tue, Feb 23, 2016 at 4:03 AM, Tom Lane  wrote:
>> I've not looked at the test case to see if this is exactly what's
>> going wrong, but it's pretty easy to see how there might be a problem:
>> consider a STABLE user-defined function that does a SELECT from a foreign
>> table.  If that function call gets pushed down into a parallel worker
>> then it would fail as described.

> I thought user defined functions were not a problem since it's the
> user's responsibility to declare functions' parallel safety correctly.
> The manual says: "In general, if a function is labeled as being safe
> when it is restricted or unsafe, or if it is labeled as being
> restricted when it is in fact unsafe, it may throw errors or produce
> wrong answers when used in a parallel query"[1].

Hm.  I'm not terribly happy with this its-the-users-problem approach to
things, mainly because I have little confidence that somebody couldn't
figure out a security exploit based on it.

> The case of a plain old SELECT (as seen in the failing regression
> test) is definitely a problem though and FDW access there needs to be
> detected automatically.

Yes, the problem we're actually seeing in that regression test is not
dependent on a function wrapper.

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] [PATH] Correct negative/zero year in to_date/to_timestamp

[Vitaly Burovoy]

On 2/22/16, Thomas Munro  wrote:
> On Tue, Feb 23, 2016 at 11:58 AM, Vitaly Burovoy
>  wrote:
>> Hello, Hackers!
>>
>> I'm writing another patch and while I was trying to cover corner cases
>> I found that to_date and to_timestamp work wrong if year in input
>> value is zero or negative:
>>
>> postgres=# SELECT
>> postgres-#  y || '-06-01' as src
>> postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN
>> ('00'||(-y)||'-06-01 BC') END::date
>> postgres-# ,to_date(y || '-06-01', '-MM-DD')
>> postgres-# ,to_timestamp(y || '-06-01', '-MM-DD')
>> postgres-# FROM (VALUES(2),(1),(0),(-1),(-2))t(y);
>>src| date  |to_date|   to_timestamp
>> --+---+---+---
>>  2-06-01  | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00
>>  1-06-01  | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00
>>  0-06-01  |   | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC
>>  -1-06-01 | 0001-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC
>>  -2-06-01 | 0002-06-01 BC | 0003-06-01 BC | 0003-06-01 00:00:00+00 BC
>> (5 rows)
>>
>> Zero year (and century) is accepted and negative years differs by 1
>> from what they should be.
>>
>>
>> I've written a patch fixes that. With it results are correct:
>> postgres=# SELECT
>> postgres-#  y || '-06-01' as src
>> postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN
>> ('00'||(-y)||'-06-01 BC') END::date
>> postgres-# ,to_date(y || '-06-01', '-MM-DD')
>> postgres-# ,to_timestamp(y || '-06-01', '-MM-DD')
>> postgres-# FROM (VALUES(2),(1),(-1),(-2))t(y);
>>src| date  |to_date|   to_timestamp
>> --+---+---+---
>>  2-06-01  | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00
>>  1-06-01  | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00
>>  -1-06-01 | 0001-06-01 BC | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC
>>  -2-06-01 | 0002-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC
>> (4 rows)
>>
>>
>> When year "0" is given, it raises an ERROR:
>> postgres=# SELECT to_timestamp('*01*01', '*MM*DD');
>> ERROR:  invalid input string for ""
>> DETAIL:  Year cannot be 0.
>>
>>
>> Also I change behavior for era indicator when negatives century or
>> year are given. In such case era indicator is ignored (for me it is
>> obvious signs should be OR-ed):
>> postgres=# SELECT to_timestamp('-0010*01*01 BC', '*MM*DD BC')
>> postgres-#   ,to_timestamp(' 0010*01*01 BC', '*MM*DD BC');
>>to_timestamp|   to_timestamp
>> ---+---
>>  0010-01-01 00:00:00+00 BC | 0010-01-01 00:00:00+00 BC
>> (1 row)
>>
>>
>> Testings, complains, advice, comment improvements are very appreciated.
>
> This seems to be a messy topic.  The usage of "AD" and "BC" imply that
> TO_DATE is using the anno domini system which doesn't have a year 0,
> but in the DATE type perhaps we are using the ISO 8601 model[2] where
> 1 BC is represented as , leading to the difference of one in all
> years before 1 AD?
>
> [1] https://en.wikipedia.org/wiki/Anno_Domini
> [2] https://en.wikipedia.org/wiki/ISO_8601#Years
>
> --
> Thomas Munro
> http://www.enterprisedb.com

Thank you for fast reply and for the link[2]. Be honest I didn't know
ISO8601 specifies 1 BC as +.

But the documentation[3] doesn't points that ISO8601 is used for
"", but it is mentioned for "IYYY", and it is slightly deceiving.
Also I remember that the other part of the documentation says[4] that
"Keep in mind there is no 0 AD" that's why I decided it is impossible
to pass  for .

Currently behavior with =0 is still surprising in some cases:
postgres=# SELECT
postgres-# to_date('20 -01-01', 'CC -MM-DD'),
postgres-# to_date('20 0001-01-01', 'CC -MM-DD');
  to_date   |  to_date
+
 1901-01-01 | 0001-01-01
(1 row)

but the documentation[3] says "In conversions from string to timestamp
or date, the CC (century) field is ignored if there is a YYY,  or
Y,YYY field."

So is it shared opinion that ISO8601 is used for ""?

[3]http://www.postgresql.org/docs/devel/static/functions-formatting.html
[4]http://www.postgresql.org/docs/devel/static/functions-datetime.html
-- 
Best regards,
Vitaly Burovoy


-- 
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] Writing new unit tests with PostgresNode

[Michael Paquier]

On Tue, Feb 23, 2016 at 12:29 AM, Alvaro Herrera
 wrote:
> Craig Ringer wrote:
>
>> > +=pod
>> > +
>> > +=head2 Set up a node
>> > pod format... Do we really want that? Considering that those modules
>> > are only aimed at being dedicated for in-core testing, I would say no.
>>
>> If it's plain comments you have to scan through massive piles of verbose
>> Perl to find what you want. If it's pod you can just perldoc
>> /path/to/module it and get a nice summary of the functions etc.
>>
>> If these are intended to become usable facilities for people to write tests
>> with then I think it's important that the docs be reasonably accessible.
>
> Yes, I think adding POD here is a good idea.  I considered doing it
> myself back when I was messing with PostgresNode ...

OK, withdrawal from here. If there are patches to add that to the
existing tests, I'll review them, and rebase what I have depending on
what gets in first. Could a proper patch split be done please?
-- 
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: format() changes discussion (was: Re: [HACKERS] psql metaqueries with \gexec)

[Jim Nasby]

On 2/22/16 5:16 PM, Corey Huinker wrote:

(One thing I had to come up with was processing of arrays, which you
also see in that example JSON -- it's the specifiers that have a colon
inside the {}.  The part after the colon is used as separator between
the array elements, and each element is expanded separately.)


I'm splitting the subject line because it seems like two very different
patches may come out of this.


Oops. Just saw this.

I don't think it'd make sense to make format() itself as capable as what 
Alvaro did for event triggers. Something that geared towards dealing 
with catalog objects should be it's own thing.


What would be very useful IMHO is a version of format() that accepted 
hstore as it's second argument and then let you use names in the format 
specification. IE:


format( 'blahblah name1%I blahblah literal_a%L blah "some string"%s'
  , 'name1 => name, literal_a => a, "some string" => string
)

I suggest hstore over json because it's easier to work with by hand than 
json is, and there's no ambiguity with things like objects or arrays.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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] psql metaqueries with \gexec

[Jim Nasby]

On 2/22/16 5:13 PM, Alvaro Herrera wrote:

Jim Nasby wrote:

On 2/22/16 11:47 AM, Alvaro Herrera wrote:

Pavel Stehule wrote:


The design of the "format" function is not closed. Try to send prototype
and patch. The possibility to do PostgreSQL customization was strong reason
why we didn't implemented "sprintf" and we implemented "format".


Probably not terribly useful here, but for the DDL-deparse patch I came
up with a syntax to format JSON objects, which used %-escapes; each
escaped element corresponds to a string literal, or to an object.  So
you'd have %{table}D where the "table" element in the JSON object could
be a simple string which is expanded verbatim (plus quoting if
necessary), or it could be a JSON object with something like { schema =>
"public", name => "students" }, where each element is expanded and
quoted as necessary; if the "schema" is null or it doesn't exist, it
expands only the name, obviously omitting the dot.


Where did the "D" in "%{table}D" come from?


The I in %{foo}I was for "identifier" (of course) and I *think* the D
was for "double identifiers" (that is, qualified).  I expanded the idea
afterwards to allow for a third name for things like
catalog.schema.name, so I guess it's a misnomer already.

It's not released code yet.  You can see an example here
https://www.postgresql.org/message-id/%3C20150224175152.GI5169%40alvh.no-ip.org%3E
just scroll down a few hundred lines to about 7/16ths of the page (yes,
really)

(One thing I had to come up with was processing of arrays, which you
also see in that example JSON -- it's the specifiers that have a colon
inside the {}.  The part after the colon is used as separator between
the array elements, and each element is expanded separately.)


Ahh, very interesting.

Something that would probably be helpful for these kind of things is if 
we had a set of complex types available that represented things like the 
arguments to a function. Something like (parameter_mode enum(IN, OUT, 
INOUT), parameter_name name, parameter_type regtype, parameter_default 
text). A function might be represented by (function_schema name, 
function_name name, function_parameters ..., function_language, 
function_options, function_body).


In any case, having anything along these lines in core would be useful, 
assuming that the individual facility was exposed as well (as opposed to 
only being available inside an event trigger).

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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] [PATH] Correct negative/zero year in to_date/to_timestamp

[Thomas Munro]

On Tue, Feb 23, 2016 at 11:58 AM, Vitaly Burovoy
 wrote:
> Hello, Hackers!
>
> I'm writing another patch and while I was trying to cover corner cases
> I found that to_date and to_timestamp work wrong if year in input
> value is zero or negative:
>
> postgres=# SELECT
> postgres-#  y || '-06-01' as src
> postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN
> ('00'||(-y)||'-06-01 BC') END::date
> postgres-# ,to_date(y || '-06-01', '-MM-DD')
> postgres-# ,to_timestamp(y || '-06-01', '-MM-DD')
> postgres-# FROM (VALUES(2),(1),(0),(-1),(-2))t(y);
>src| date  |to_date|   to_timestamp
> --+---+---+---
>  2-06-01  | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00
>  1-06-01  | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00
>  0-06-01  |   | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC
>  -1-06-01 | 0001-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC
>  -2-06-01 | 0002-06-01 BC | 0003-06-01 BC | 0003-06-01 00:00:00+00 BC
> (5 rows)
>
> Zero year (and century) is accepted and negative years differs by 1
> from what they should be.
>
>
> I've written a patch fixes that. With it results are correct:
> postgres=# SELECT
> postgres-#  y || '-06-01' as src
> postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN
> ('00'||(-y)||'-06-01 BC') END::date
> postgres-# ,to_date(y || '-06-01', '-MM-DD')
> postgres-# ,to_timestamp(y || '-06-01', '-MM-DD')
> postgres-# FROM (VALUES(2),(1),(-1),(-2))t(y);
>src| date  |to_date|   to_timestamp
> --+---+---+---
>  2-06-01  | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00
>  1-06-01  | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00
>  -1-06-01 | 0001-06-01 BC | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC
>  -2-06-01 | 0002-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC
> (4 rows)
>
>
> When year "0" is given, it raises an ERROR:
> postgres=# SELECT to_timestamp('*01*01', '*MM*DD');
> ERROR:  invalid input string for ""
> DETAIL:  Year cannot be 0.
>
>
> Also I change behavior for era indicator when negatives century or
> year are given. In such case era indicator is ignored (for me it is
> obvious signs should be OR-ed):
> postgres=# SELECT to_timestamp('-0010*01*01 BC', '*MM*DD BC')
> postgres-#   ,to_timestamp(' 0010*01*01 BC', '*MM*DD BC');
>to_timestamp|   to_timestamp
> ---+---
>  0010-01-01 00:00:00+00 BC | 0010-01-01 00:00:00+00 BC
> (1 row)
>
>
> Testings, complains, advice, comment improvements are very appreciated.

This seems to be a messy topic.  The usage of "AD" and "BC" imply that
TO_DATE is using the anno domini system which doesn't have a year 0,
but in the DATE type perhaps we are using the ISO 8601 model[2] where
1 BC is represented as , leading to the difference of one in all
years before 1 AD?

[1] https://en.wikipedia.org/wiki/Anno_Domini
[2] https://en.wikipedia.org/wiki/ISO_8601#Years

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


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

Re: [HACKERS] Convert pltcl from strings to objects

[Jim Nasby]

On 2/18/16 6:26 AM, Victor Wagner wrote:

On Tue, 9 Feb 2016 16:23:21 -0600
There is suspicious  place at the end of compile_pltcl_fuction function,
where you've put comment that old prodesc cannot be deallocated,
because it might be used by other call.

It seems that reference counting mechanism which Tcl already has, can
help here. Would there be serious performance impact if you'll use Tcl
list instead of C structure to store procedure description?
If not, you can rely on Tcl_IncrRefCount/Tcl_DecrRefCount to free a
procedure description when last active call finishes.


Are you referring to this comment?


/
 * Add the proc description block to the hashtable.  Note we do 
not
 * attempt to free any previously existing prodesc block.  This 
is
 * annoying, but necessary since there could be active calls 
using
 * the old prodesc.
 /


That is not changed by the patch, and I don't think it's in the scope of 
this patch. I agree it would be nice to get rid of that and the related 
malloc() call, as well as what perm_fmgr_info() is doing, but those are 
unrelated to this work.


(BTW, I also disagree about using a Tcl list for prodesc. That's an 
object in a *Postgres* hash table; as such it needs to be managed by 
Postgres, not Tcl. AFAIK the Postgres ResourceOwner stuff would be the 
correct way to do that (but I'm not exactly an expert on that area).



Function pltcl_elog have a big switch case to convert enum logpriority,
local to this function to PostgreSql log levels.

It seems not a most readable solution, because this enumeration is
desined to be passed to Tcl_GetIndexFromObj, so it can be used and is
used to index an array (logpriorities array of string representation).
You can define simular array with PostgreSQL integer constant and
replace page-sized switch with just two lines - this array definition
and getting value from it by index

static CONST84 int
loglevels[]={DEBUG,LOG,INFO,NOTICE,WARNING,ERROR,FATAL};


Tcl_GetIndexFromObj(

level=loglevels[priIndex];


Done.


It seems that you are losing some diagnostic information by
extracting just message field from ErrorData, which you do in
pltcl_elog and pltcl_subtrans_abort.

Tcl has  mechanisms for passing around additional error information.
See Tcl_SetErrorCode and Tcl_AddErrorInfo functions

pltcl_process_SPI_result might benefit from providing SPI result code
in machine readable way via Tcl_SetErrorCode too.


Is there any backwards compatibility risk to these changes? Could having 
that new info break someone's existing code?



In some cases you use Tcl_SetObjResult(interp, Tcl_NewStringObj("error
message",-1)) to report error with constant message, where in other
places Tcl_SetResult(interp,"error message",TCL_STATIC) left in place.

I see no harm in using old API with static messages, especially if
Tcl_AppendResult is used, but mixing two patterns above seems to be a
bit inconsistent.


Switched everything to using the new API.


As far as I can understand, using Tcl_StringObj to represent all
postgresql primitive (non-array) types and making no attempt to convert
tuple data into integer, boolean or double objects is design decision.

It really can spare few unnecessary type conversions.


Yeah, that's on the list. But IMHO it's outside the scope of this patch.


Thanks for your effort.


Thanks for the review!
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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] Sanity checking for ./configure options?

[Jim Nasby]

On 2/5/16 10:08 AM, David Fetter wrote:

On Wed, Feb 03, 2016 at 06:02:57PM -0600, Jim Nasby wrote:

I just discovered that ./configure will happily accept '--with-pgport=' (I
was actually doing =$PGPORT, and didn't realize $PGPORT was empty). What you
end up with is a compile error in guc.c, with no idea why it's broken. Any
reason not to have configure or at least make puke if pgport isn't valid?


That seems like a good idea.


Patch attached. I've verified it with --with-pgport=, =0, =7 and =1. 
It catches what you'd expect it to.


As the comment states, it doesn't catch things like --with-pgport=1a in 
configure, but the compile error you get with that isn't too hard to 
figure out, so I think it's OK.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
diff --git a/configure b/configure
index b3f3abe..2beee31 100755
--- a/configure
+++ b/configure
@@ -3099,6 +3099,14 @@ cat >>confdefs.h <<_ACEOF
 _ACEOF
 
 
+# It's worth testing for this because it creates a very confusing error
+if test "$default_port" == ""; then
+   as_fn_error $? "Invalid empty string supplied for \$PGPORT or 
--with-pgport" "$LINENO" 5
+# This won't catch something like "PGPORT=11a" but that produces a pretty easy
+# to understand compile error.
+elif test "$default_port" -lt "1" -o "$default_port" -gt "65535"; then
+   as_fn_error $? "port must be between 1 and 65535" "$LINENO" 5
+fi
 
 #
 # '-rpath'-like feature can be disabled
diff --git a/configure.in b/configure.in
index 0bd90d7..54e9a16 100644
--- a/configure.in
+++ b/configure.in
@@ -164,6 +164,14 @@ but it's convenient if your clients have the right default 
compiled in.
 AC_DEFINE_UNQUOTED(DEF_PGPORT_STR, "${default_port}",
[Define to the default TCP port number as a string 
constant.])
 AC_SUBST(default_port)
+# It's worth testing for this because it creates a very confusing error
+if test "$default_port" == ""; then
+   AC_MSG_ERROR([Invalid empty string supplied for \$PGPORT or 
--with-pgport])
+# This won't catch something like "PGPORT=11a" but that produces a pretty easy
+# to understand compile error.
+elif test "$default_port" -lt "1" -o "$default_port" -gt "65535"; then
+   AC_MSG_ERROR([port must be between 1 and 65535])
+fi
 
 #
 # '-rpath'-like feature can be disabled

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

format() changes discussion (was: Re: [HACKERS] psql metaqueries with \gexec)

[Corey Huinker]

>
> (One thing I had to come up with was processing of arrays, which you
> also see in that example JSON -- it's the specifiers that have a colon
> inside the {}.  The part after the colon is used as separator between
> the array elements, and each element is expanded separately.)
>
>
I'm splitting the subject line because it seems like two very different
patches may come out of this.

Re: [HACKERS] psql metaqueries with \gexec

[Alvaro Herrera]

Jim Nasby wrote:
> On 2/22/16 11:47 AM, Alvaro Herrera wrote:
> >Pavel Stehule wrote:
> >
> >>The design of the "format" function is not closed. Try to send prototype
> >>and patch. The possibility to do PostgreSQL customization was strong reason
> >>why we didn't implemented "sprintf" and we implemented "format".
> >
> >Probably not terribly useful here, but for the DDL-deparse patch I came
> >up with a syntax to format JSON objects, which used %-escapes; each
> >escaped element corresponds to a string literal, or to an object.  So
> >you'd have %{table}D where the "table" element in the JSON object could
> >be a simple string which is expanded verbatim (plus quoting if
> >necessary), or it could be a JSON object with something like { schema =>
> >"public", name => "students" }, where each element is expanded and
> >quoted as necessary; if the "schema" is null or it doesn't exist, it
> >expands only the name, obviously omitting the dot.
> 
> Where did the "D" in "%{table}D" come from?

The I in %{foo}I was for "identifier" (of course) and I *think* the D
was for "double identifiers" (that is, qualified).  I expanded the idea
afterwards to allow for a third name for things like
catalog.schema.name, so I guess it's a misnomer already.

It's not released code yet.  You can see an example here
https://www.postgresql.org/message-id/%3C20150224175152.GI5169%40alvh.no-ip.org%3E
just scroll down a few hundred lines to about 7/16ths of the page (yes,
really)

(One thing I had to come up with was processing of arrays, which you
also see in that example JSON -- it's the specifiers that have a colon
inside the {}.  The part after the colon is used as separator between
the array elements, and each element is expanded separately.)

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


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

Re: [HACKERS] postgres_fdw vs. force_parallel_mode on ppc

[Thomas Munro]

On Tue, Feb 23, 2016 at 4:03 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Mon, Feb 22, 2016 at 11:02 AM, Thomas Munro
>>  wrote:
>>> The postgres_fdw failure is a visibility-of-my-own-uncommitted-work
>>> problem.  The first command in a transaction updates a row via an FDW,
>>> and then the SELECT expects to see the effects, but when run in a
>>> background worker it creates a new FDW connection that can't see the
>>> uncommitted UPDATE.
>
>> Foreign tables are supposed to be categorically excluded from
>> parallelism.  Not sure why that's not working in this instance.
>
> I've not looked at the test case to see if this is exactly what's
> going wrong, but it's pretty easy to see how there might be a problem:
> consider a STABLE user-defined function that does a SELECT from a foreign
> table.  If that function call gets pushed down into a parallel worker
> then it would fail as described.

I thought user defined functions were not a problem since it's the
user's responsibility to declare functions' parallel safety correctly.
The manual says: "In general, if a function is labeled as being safe
when it is restricted or unsafe, or if it is labeled as being
restricted when it is in fact unsafe, it may throw errors or produce
wrong answers when used in a parallel query"[1].  Uncommitted changes
on foreign tables are indeed invisible to functions declared as
PARALLEL SAFE, when run with force_parallel_mode = on,
max_parallel_degree = 2, but the default is UNSAFE and in that case
the containing query is never parallelised.  Perhaps the documentation
could use a specific mention of this subtlety with FDWs in the
PARALLEL section?

The case of a plain old SELECT (as seen in the failing regression
test) is definitely a problem though and FDW access there needs to be
detected automatically.  I also thought that
has_parallel_hazard_walker might be the right place for that logic, as
you suggested in your later message.

[1] http://www.postgresql.org/docs/devel/static/sql-createfunction.html

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


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

Re: [HACKERS] [PATH] Correct negative/zero year in to_date/to_timestamp

[Vitaly Burovoy]

On 2/22/16, Vitaly Burovoy  wrote:
> Testings, complains, advice, comment improvements are very appreciated.

The patch seems simple, but it can lead to a discussion, so I've added it to CF.

[CF]https://commitfest.postgresql.org/9/533/

-- 
Best regards,
Vitaly Burovoy


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

[HACKERS] [PATH] Correct negative/zero year in to_date/to_timestamp

[Vitaly Burovoy]

Hello, Hackers!

I'm writing another patch and while I was trying to cover corner cases
I found that to_date and to_timestamp work wrong if year in input
value is zero or negative:

postgres=# SELECT
postgres-#  y || '-06-01' as src
postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN
('00'||(-y)||'-06-01 BC') END::date
postgres-# ,to_date(y || '-06-01', '-MM-DD')
postgres-# ,to_timestamp(y || '-06-01', '-MM-DD')
postgres-# FROM (VALUES(2),(1),(0),(-1),(-2))t(y);
   src| date  |to_date|   to_timestamp
--+---+---+---
 2-06-01  | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00
 1-06-01  | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00
 0-06-01  |   | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC
 -1-06-01 | 0001-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC
 -2-06-01 | 0002-06-01 BC | 0003-06-01 BC | 0003-06-01 00:00:00+00 BC
(5 rows)

Zero year (and century) is accepted and negative years differs by 1
from what they should be.


I've written a patch fixes that. With it results are correct:
postgres=# SELECT
postgres-#  y || '-06-01' as src
postgres-# ,CASE WHEN y>0 THEN ('00'||y||'-06-01') WHEN y<0 THEN
('00'||(-y)||'-06-01 BC') END::date
postgres-# ,to_date(y || '-06-01', '-MM-DD')
postgres-# ,to_timestamp(y || '-06-01', '-MM-DD')
postgres-# FROM (VALUES(2),(1),(-1),(-2))t(y);
   src| date  |to_date|   to_timestamp
--+---+---+---
 2-06-01  | 0002-06-01| 0002-06-01| 0002-06-01 00:00:00+00
 1-06-01  | 0001-06-01| 0001-06-01| 0001-06-01 00:00:00+00
 -1-06-01 | 0001-06-01 BC | 0001-06-01 BC | 0001-06-01 00:00:00+00 BC
 -2-06-01 | 0002-06-01 BC | 0002-06-01 BC | 0002-06-01 00:00:00+00 BC
(4 rows)


When year "0" is given, it raises an ERROR:
postgres=# SELECT to_timestamp('*01*01', '*MM*DD');
ERROR:  invalid input string for ""
DETAIL:  Year cannot be 0.


Also I change behavior for era indicator when negatives century or
year are given. In such case era indicator is ignored (for me it is
obvious signs should be OR-ed):
postgres=# SELECT to_timestamp('-0010*01*01 BC', '*MM*DD BC')
postgres-#   ,to_timestamp(' 0010*01*01 BC', '*MM*DD BC');
   to_timestamp|   to_timestamp
---+---
 0010-01-01 00:00:00+00 BC | 0010-01-01 00:00:00+00 BC
(1 row)


Testings, complains, advice, comment improvements are very appreciated.

-- 
Best regards,
Vitaly Burovoy


negative_years_in_to_date.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] psql metaqueries with \gexec

[Jim Nasby]

On 2/22/16 11:47 AM, Alvaro Herrera wrote:

Pavel Stehule wrote:


The design of the "format" function is not closed. Try to send prototype
and patch. The possibility to do PostgreSQL customization was strong reason
why we didn't implemented "sprintf" and we implemented "format".


Probably not terribly useful here, but for the DDL-deparse patch I came
up with a syntax to format JSON objects, which used %-escapes; each
escaped element corresponds to a string literal, or to an object.  So
you'd have %{table}D where the "table" element in the JSON object could
be a simple string which is expanded verbatim (plus quoting if
necessary), or it could be a JSON object with something like { schema =>
"public", name => "students" }, where each element is expanded and
quoted as necessary; if the "schema" is null or it doesn't exist, it
expands only the name, obviously omitting the dot.


Where did the "D" in "%{table}D" come from?

BTW, the syntax I chose for [1] is similar to format's, except I elected 
to stick with % instead of $. So you do


%parameter_name%type

where type is s, L or I. I don't think it'd be hard to support an object 
with 'schema' and 'name' keys.


[1] 
https://github.com/decibel/trunklet-format/blob/master/doc/trunklet-format.asc#2-template-specification

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.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] about google summer of code 2016

[Álvaro Hernández Tortosa]

On 22/02/16 23:34, Tom Lane wrote:

=?UTF-8?Q?=c3=81lvaro_Hern=c3=a1ndez_Tortosa?=  writes:

On 22/02/16 05:10, Tom Lane wrote:

Another variable is that your answers might depend on what format you
assume the client is trying to convert from/to.  (It's presumably not
text JSON, but then what is it?)

  As I mentioned before, there are many well-known JSON serialization
formats, like:
- http://ubjson.org/
- http://cbor.io/
- http://msgpack.org/
- BSON (ok, let's skip that one hehehe)
- http://wiki.fasterxml.com/SmileFormatSpec

Ah, the great thing about standards is there are so many to choose from :-(

So I guess part of the GSoC project would have to be figuring out which
one of these would make the most sense for us to adopt.

regards, tom lane


Yes.

And unless I'm mistaken, there's an int16 to identify the data 
format. Apart from the chosen format, others may be provided as an 
alternative using different data formats. Or alternatives (like 
compressed text json). Of course, this may be better suited for a next 
GSoC project, of course.


Álvaro


--
Álvaro Hernández Tortosa


---
8Kdata



--
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] about google summer of code 2016

[Tom Lane]

=?UTF-8?Q?=c3=81lvaro_Hern=c3=a1ndez_Tortosa?=  writes:
> On 22/02/16 05:10, Tom Lane wrote:
>> Another variable is that your answers might depend on what format you
>> assume the client is trying to convert from/to.  (It's presumably not
>> text JSON, but then what is it?)

>  As I mentioned before, there are many well-known JSON serialization 
> formats, like:

> - http://ubjson.org/
> - http://cbor.io/
> - http://msgpack.org/
> - BSON (ok, let's skip that one hehehe)
> - http://wiki.fasterxml.com/SmileFormatSpec

Ah, the great thing about standards is there are so many to choose from :-(

So I guess part of the GSoC project would have to be figuring out which
one of these would make the most sense for us to adopt.

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] about google summer of code 2016

[Álvaro Hernández Tortosa]

On 22/02/16 05:10, Tom Lane wrote:

Heikki Linnakangas  writes:

On 19/02/16 10:10, �lvaro Hernández Tortosa wrote:

Oleg and I discussed recently that a really good addition to a GSoC
item would be to study whether it's convenient to have a binary
serialization format for jsonb over the wire.

Seems a bit risky for a GSoC project. We don't know if a different
serialization format will be a win, or whether we want to do it in the
end, until the benchmarking is done. It's also not clear what we're
trying to achieve with the serialization format: smaller on-the-wire
size, faster serialization in the server, faster parsing in the client,
or what?

Another variable is that your answers might depend on what format you
assume the client is trying to convert from/to.  (It's presumably not
text JSON, but then what is it?)


As I mentioned before, there are many well-known JSON serialization 
formats, like:


- http://ubjson.org/
- http://cbor.io/
- http://msgpack.org/
- BSON (ok, let's skip that one hehehe)
- http://wiki.fasterxml.com/SmileFormatSpec



Having said that, I'm not sure that risk is a blocking factor here.
History says that a large fraction of our GSoC projects don't result
in a commit to core PG.  As long as we're clear that "success" in this
project isn't measured by getting a feature committed, it doesn't seem
riskier than any other one.  Maybe it's even less risky, because there's
less of the success condition that's not under the GSoC student's control.


Agreed :)

Álvaro


--
Álvaro Hernández Tortosa


---
8Kdata



--
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] about google summer of code 2016

[Álvaro Hernández Tortosa]

On 21/02/16 21:15, Heikki Linnakangas wrote:

On 19/02/16 10:10, Álvaro Hernández Tortosa wrote:
  Oleg and I discussed recently that a really good addition to a 
GSoC

item would be to study whether it's convenient to have a binary
serialization format for jsonb over the wire. Some argue this should be
benchmarked first. So the scope for this project would be to benchmark
and analyze the potential improvements and then agree on which format
jsonb could be serialized to (apart from the current on-disk format,
there are many json or nested k-v formats that could be used for sending
over the wire).


Seems a bit risky for a GSoC project. We don't know if a different 
serialization format will be a win, 


Over the current serialization (text) is hard to believe there will 
be no wins.


or whether we want to do it in the end, until the benchmarking is 
done. It's also not clear what we're trying to achieve with the 
serialization format: smaller on-the-wire size, faster serialization 
in the server, faster parsing in the client, or what?


Probably all of them (it would be ideal if it could be selectable). 
Some may favor small on-the-wire size (which can be significant with 
several serialization formats) or faster decoding (de-serialization 
takes a significant execution time). Of course, all this should be 
tested and benchmarked before, but we're not alone here.


This is a significant request from many, at least from the Java 
users, where it has been discussed many times. Specially if wire format 
adheres to one well-known (or even Standard) format, so that the 
receiving side and the drivers could expose an API based on that format 
--one of the other big pains today in this side.


I think it fits very well for a GSoC! :)

Álvaro


--
Álvaro Hernández Tortosa


---
8Kdata



--
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] postgres_fdw vs. force_parallel_mode on ppc

[Tom Lane]

Robert Haas  writes:
>> Foreign tables are supposed to be categorically excluded from
>> parallelism.  Not sure why that's not working in this instance.

BTW, I wonder where you think that's supposed to be enforced, because
I sure can't find any such logic.

I suppose that has_parallel_hazard() would be the logical place to
notice foreign tables, but it currently doesn't even visit RTEs,
much less contain any code to check if their tables are foreign.
Or did you have another place in mind to do that?

regards, tom lane


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

Re: [HACKERS] checkpointer continuous flushing - V18

[Fabien COELHO]

Random updates on 16 tables which total to 1.1GB of data, so this is in
buffer, no significant "read" traffic.

(1) with 16 tablespaces (1 per table) on 1 disk : 680.0 tps
per second avg, stddev [ min q1 median d3 max ] <=300tps
679.6 ± 750.4 [0.0, 317.0, 371.0, 438.5, 2724.0] 19.5%

(2) with 1 tablespace on 1 disk : 956.0 tps
per second avg, stddev [ min q1 median d3 max ] <=300tps
956.2 ± 796.5 [3.0, 488.0, 583.0, 742.0, 2774.0] 2.1%


Interesting. That doesn't reflect my own tests, even on rotating media,
at all. I wonder if it's related to:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=23d0127096cb91cb6d354bdc71bd88a7bae3a1d5

If you use your 12.04 kernel, that'd not be fixed. Which might be a
reason to do it as you suggest.

Could you share the exact details of that workload?


See attached scripts (sh to create the 16 tables in the default or 16 
table spaces, small sql bench script, stat computation script).


The per-second stats were computed with:

  grep progress: pgbench.out | cut -d' ' -f4 | avg.py --length=1000 --limit=300

Host is 8 cpu 16 GB, 2 HDD in RAID 1.

--
Fabien.

ts_create.sh
Description: Bourne shell script


ts_test.sql
Description: application/sql
#! /usr/bin/env python
# -*- coding: utf-8 -*-
#
# $Id: avg.py 1242 2016-02-06 14:44:02Z coelho $
#

import argparse
ap = argparse.ArgumentParser(description='show stats about data: count average stddev [min q1 median q3 max]...')
ap.add_argument('--median', default=True, action='store_true',
help='compute median and quartile values')
ap.add_argument('--no-median', dest='median', default=True,
action='store_false',
help='do not compute median and quartile values')
ap.add_argument('--more', default=False, action='store_true',
help='show some more stats')
ap.add_argument('--limit', type=float, default=None,
help='set limit for counting below limit values')
ap.add_argument('--length', type=int, default=None,
help='set expected length, assume 0 if beyond')
ap.add_argument('--precision', type=int, default=1,
help='floating point precision')
ap.add_argument('file', nargs='*', help='list of files to process')
opt = ap.parse_args()

# option consistency
if opt.limit != None:
	opt.more = True
if opt.more:
	opt.median = True

# reset arguments for fileinput
import sys
sys.argv[1:] = opt.file

import fileinput

n, skipped, vals = 0, 0, []
k, vmin, vmax = None, None, None
sum1, sum2 = 0.0, 0.0

for line in fileinput.input():
	try:
		v = float(line)
		if opt.median: # keep track only if needed
			vals.append(v)
		if k is None: # first time
			k, vmin, vmax = v, v, v
		else: # next time
			vmin = min(vmin, v)
			vmax = max(vmax, v)
		n += 1
		vmk = v - k
		sum1 += vmk
		sum2 += vmk * vmk
	except ValueError: # float conversion failed
		skipped += 1

if n == 0:
	# avoid ops on None below
	k, vmin, vmax = 0.0, 0.0, 0.0

if opt.length:
	assert "some data seen", n > 0
	missing = int(opt.length) - len(vals)
	assert "positive number of missing data", missing >= 0
	if missing > 0:
		print("warning: %d missing data, expanding with zeros" % missing)
		if opt.median:
			vals += [ 0.0 ] * missing
		vmin = min(vmin, 0.0)
		sum1 += - k * missing
		sum2 += k * k * missing
		n += missing
		assert len(vals) == int(opt.length)

if opt.median:
	assert "consistent length", len(vals) == n

# five numbers...
# numpy.percentile requires numpy at least 1.9 to use 'midpoint'
# statistics.median requires python 3.4 (?)
def median(vals, start, length):
	if len(vals) == 1:
		start, length = 0, 1
	m, odd = divmod(length, 2)
	#return 0.5 * (vals[start + m + odd - 1] + vals[start + m])
	return  vals[start + m] if odd else \
		0.5 * (vals[start + m-1] + vals[start + m])

# return ratio of below limit (limit included) values
def below(vals, limit):
	# hmmm... short but generates a list
	#return float(len([ v for v in vals if v <= limit ])) / len(vals)
	below_limit = 0
	for v in vals:
		if v <= limit:
			below_limit += 1
	return float(below_limit) / len(vals)

# float prettyprint with precision
def f(v):
	return ('%.' + str(opt.precision) + 'f') % v

# output
if skipped:
	print("warning: %d lines skipped" % skipped)

if n > 0:
	# show result (hmmm, precision is truncated...)
	from math import sqrt
	avg, stddev = k + sum1 / n, sqrt((sum2 - (sum1 * sum1) / n) / n)
	if opt.median:
		vals.sort()
		med = median(vals, 0, len(vals))
		# not sure about odd/even issues here... q3 needs fixing if len is 1
		q1 = median(vals, 0, len(vals) // 2)
		q3 = median(vals, (len(vals)+1) // 2, len(vals) // 2)
		# build summary message
		msg = "avg over %d: %s ± %s [%s, %s, %s, %s, %s]" % \
			  (n, f(avg), f(stddev), f(vmin), f(q1), f(med), f(q3), f(vmax))
		if opt.more:
			limit = opt.limit if opt.limit != None else 0.1 * med
			# msg += " <=%s:" % f(limit)
			msg += " %s%%" % f(100.0 * below(vals, limit))
	else:
		msg = "avg over %d: %s ± %s [%s, %s]" % \
			  (n, f(avg), f(stddev), f(vmin), f(vmax))
else:
	msg = "no data 

Re: [HACKERS] Add generate_series(date,date) and generate_series(date,date,integer)

[Corey Huinker]

>
> >
> > Given that counterexample, I think we not only shouldn't back-patch such
> a
> > change but should reject it altogether.
>
> Ouch, good point. The overflows are a different problem that we had
> better address though (still on my own TODO list)...
>

So I should remove the bounds checking from
generate_series(date,date,[int]) altogether?

Re: [HACKERS] psql metaqueries with \gexec

[Corey Huinker]

>
> In the mean time, update patch attached.
>
>
Really attached this time.
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 9750a5b..5ca769f 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -849,6 +849,13 @@ exec_command(const char *cmd,
status = PSQL_CMD_SEND;
}
 
+   /* \gexec -- send query and treat every result cell as a query to be 
executed */
+   else if (strcmp(cmd, "gexec") == 0)
+   {
+   pset.gexec_flag = true;
+   status = PSQL_CMD_SEND;
+   }
+
/* \gset [prefix] -- send query and store result into variables */
else if (strcmp(cmd, "gset") == 0)
{
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 2cb2e9b..54b7790 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -710,6 +710,46 @@ StoreQueryTuple(const PGresult *result)
return success;
 }
 
+/*
+ * ExecQueryTuples: assuming query result is OK, execute every query
+ * result as its own statement
+ *
+ * Returns true if successful, false otherwise.
+ */
+static bool
+ExecQueryTuples(const PGresult *result)
+{
+   boolsuccess = true;
+   int nrows = PQntuples(result);
+   int ncolumns = PQnfields(result);
+   int r, c;
+
+   for (r = 0; r < nrows; r++)
+   {
+   for (c = 0; c < ncolumns; c++)
+   {
+   if (! PQgetisnull(result, r, c))
+   {
+   if ( ! SendQuery(PQgetvalue(result, r, c)) )
+   {
+   if (pset.on_error_stop)
+   {
+   return false;
+   }
+   else
+   {
+   success = false;
+   }
+   }
+   }
+   }
+   }
+
+   /* Return true if all queries were successful */
+   return success;
+}
+
+
 
 /*
  * ProcessResult: utility function for use by SendQuery() only
@@ -903,8 +943,14 @@ PrintQueryResults(PGresult *results)
switch (PQresultStatus(results))
{
case PGRES_TUPLES_OK:
-   /* store or print the data ... */
-   if (pset.gset_prefix)
+   /* execute or store or print the data ... */
+   if (pset.gexec_flag)
+   {
+   /* Turn off gexec_flag to avoid infinite loop */
+   pset.gexec_flag = false;
+   ExecQueryTuples(results);
+   }
+   else if (pset.gset_prefix)
success = StoreQueryTuple(results);
else
success = PrintQueryTuples(results);
diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c
index 59f6f25..251dd1e 100644
--- a/src/bin/psql/help.c
+++ b/src/bin/psql/help.c
@@ -173,6 +173,7 @@ slashUsage(unsigned short int pager)
fprintf(output, _("General\n"));
fprintf(output, _("  \\copyright show PostgreSQL usage and 
distribution terms\n"));
fprintf(output, _("  \\g [FILE] or ; execute query (and send 
results to file or |pipe)\n"));
+   fprintf(output, _("  \\gexec execute query and treat 
every result cell as a query to be executed )\n"));
fprintf(output, _("  \\gset [PREFIX] execute query and store 
results in psql variables\n"));
fprintf(output, _("  \\q quit psql\n"));
fprintf(output, _("  \\watch [SEC]   execute query every SEC 
seconds\n"));
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
index 20a6470..9f1e94b 100644
--- a/src/bin/psql/settings.h
+++ b/src/bin/psql/settings.h
@@ -91,6 +91,9 @@ typedef struct _psqlSettings
char   *gfname; /* one-shot file output 
argument for \g */
char   *gset_prefix;/* one-shot prefix argument for \gset */
 
+   boolgexec_flag; /* true if query results are to 
be treated as
+* queries to 
be executed. Set by \gexec */
+
boolnotty;  /* stdin or stdout is not a tty 
(as determined
 * on startup) 
*/
enum trivalue getPassword;  /* prompt the user for a username and 
password */
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5f27120..0f87f29 100644
--- a/src/bin/psql/tab-complete.c
+++ 

Re: [HACKERS] psql metaqueries with \gexec

[Corey Huinker]

On Mon, Feb 22, 2016 at 11:30 AM, Corey Huinker 
wrote:

> On Mon, Feb 22, 2016 at 10:08 AM, Daniel Verite 
> wrote:
>
>> Corey Huinker wrote:
>>
>> > ...and query text visibility, and result visibility, and error handling,
>> > etc. In this case, we're leveraging the psql environment we'd already
>> set
>> > up, and if there's an error, \set ECHO queries shows us the errant SQL
>> as
>> > if we typed it ourselves..
>>
>> BTW, about error handling, shouldn't it honor ON_ERROR_STOP ?
>>
>> With the patch when trying this:
>>
>> => set ON_ERROR_STOP on
>> => select * from (values ('select 1/0', 'select 1/0')) AS n \gexec
>>
>> it produces two errors:
>> ERROR:  division by zero
>> ERROR:  division by zero
>>
>> I'd rather have the execution stop immediately after the first error,
>> like it's the case with successive queries entered normally via the
>> query buffer:
>>
>> => \set ON_ERROR_STOP on
>> => select 1/0; select 1/0;
>> ERROR:  division by zero
>>
>> as opposed to:
>>
>> => \set ON_ERROR_STOP off
>> => select 1/0; select 1/0;
>> ERROR:  division by zero
>> ERROR:  division by zero
>>
>>
> Yes, I would like it to honor ON_ERROR_STOP. I'll look into that.
>
>
Well, that was easy enough. Turns out that pset.on_error_stop is checked in
MainLoop, whereas the other pset.on_* vars are checked in SendQuery().

My original idea had been to push each cell into a in-memory temp file
handle and call MainLoop() on each. Pavel suggested that temp files of any
sort were a bad idea, hence using SendQuery instead. It's probably for the
best.


# select 'select 1,2,3', 'select 1/0', 'select 4,5,6'
... # \gexec
 ?column? | ?column? | ?column?
--+--+--
1 |2 |3
(1 row)

Time: 0.151 ms
ERROR:  22012: division by zero
LOCATION:  int4div, int.c:719
Time: 0.528 ms
 ?column? | ?column? | ?column?
--+--+--
4 |5 |6
(1 row)

Time: 0.139 ms
Time: 0.595 ms
# \set ON_ERROR_STOP 1
# select 'select 1,2,3', 'select 1/0', 'select 4,5,6' \gexec
 ?column? | ?column? | ?column?
--+--+--
1 |2 |3
(1 row)

Time: 0.137 ms
ERROR:  22012: division by zero
LOCATION:  int4div, int.c:719
Time: 0.165 ms
Time: 0.284 ms


Does \set ON_ERROR_STOP mess up regression tests? If not, I'll add the test
above (minus the \set VERBOSITY verbose-isms) to the regression.

In the mean time, update patch attached.

Re: [HACKERS] psql metaqueries with \gexec

[Alvaro Herrera]

Pavel Stehule wrote:

> The design of the "format" function is not closed. Try to send prototype
> and patch. The possibility to do PostgreSQL customization was strong reason
> why we didn't implemented "sprintf" and we implemented "format".

Probably not terribly useful here, but for the DDL-deparse patch I came
up with a syntax to format JSON objects, which used %-escapes; each
escaped element corresponds to a string literal, or to an object.  So
you'd have %{table}D where the "table" element in the JSON object could
be a simple string which is expanded verbatim (plus quoting if
necessary), or it could be a JSON object with something like { schema =>
"public", name => "students" }, where each element is expanded and
quoted as necessary; if the "schema" is null or it doesn't exist, it
expands only the name, obviously omitting the dot.

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


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

Re: [HACKERS] psql metaqueries with \gexec

[Corey Huinker]

On Mon, Feb 22, 2016 at 10:08 AM, Daniel Verite 
wrote:

> Corey Huinker wrote:
>
> > ...and query text visibility, and result visibility, and error handling,
> > etc. In this case, we're leveraging the psql environment we'd already set
> > up, and if there's an error, \set ECHO queries shows us the errant SQL as
> > if we typed it ourselves..
>
> BTW, about error handling, shouldn't it honor ON_ERROR_STOP ?
>
> With the patch when trying this:
>
> => set ON_ERROR_STOP on
> => select * from (values ('select 1/0', 'select 1/0')) AS n \gexec
>
> it produces two errors:
> ERROR:  division by zero
> ERROR:  division by zero
>
> I'd rather have the execution stop immediately after the first error,
> like it's the case with successive queries entered normally via the
> query buffer:
>
> => \set ON_ERROR_STOP on
> => select 1/0; select 1/0;
> ERROR:  division by zero
>
> as opposed to:
>
> => \set ON_ERROR_STOP off
> => select 1/0; select 1/0;
> ERROR:  division by zero
> ERROR:  division by zero
>
>
Yes, I would like it to honor ON_ERROR_STOP. I'll look into that.

Re: [HACKERS] checkpointer continuous flushing - V18

[Andres Freund]

On 2016-02-22 11:05:20 -0500, Tom Lane wrote:
> Andres Freund  writes:
> > Interesting. That doesn't reflect my own tests, even on rotating media,
> > at all. I wonder if it's related to:
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=23d0127096cb91cb6d354bdc71bd88a7bae3a1d5
> 
> > If you use your 12.04 kernel, that'd not be fixed. Which might be a
> > reason to do it as you suggest.
> 
> Hmm ... that kernel commit is less than 4 months old.  Would it be
> reflected in *any* production kernels yet?

Probably not - so far I though it mainly has some performance benefits
on relatively extreme workloads; where without the patch, flushing still
is better performancewise than not flushing. But in the scenario Fabien
has brought up it seems quite possible that sync_file_range emitting
"storage cache flush" instructions, could explain the rather large
performance difference between his and my experiments.

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] Typo in bufmgr.c that result in waste of memory

[Bruce Momjian]

On Sat, Feb 20, 2016 at 01:55:55PM +0530, Robert Haas wrote:
> On Fri, Feb 19, 2016 at 7:20 PM, Andres Freund  wrote:
> > On February 19, 2016 2:42:08 PM GMT+01:00, Tom Lane  
> > wrote:
> >>> I think we should fix it, but not backpatch.
> >>
> >>I don't think that's particularly good policy.  It's a clear bug, why
> >>would we not fix it?  Leaving it as-is in the back branches can have
> >>no good effect, and what it does do is create a merge hazard for other
> >>back-patchable bug fixes in the same area.
> >
> > Agreed.
> 
> +1.  I think this is clearly a back-patchable fix.

Fix applied to head and 9.5.

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

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


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

Re: [HACKERS] checkpointer continuous flushing - V18

[Tom Lane]

Andres Freund  writes:
> Interesting. That doesn't reflect my own tests, even on rotating media,
> at all. I wonder if it's related to:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=23d0127096cb91cb6d354bdc71bd88a7bae3a1d5

> If you use your 12.04 kernel, that'd not be fixed. Which might be a
> reason to do it as you suggest.

Hmm ... that kernel commit is less than 4 months old.  Would it be
reflected in *any* production kernels yet?

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] checkpointer continuous flushing - V18

[Andres Freund]

On 2016-02-22 14:11:05 +0100, Fabien COELHO wrote:
> 
> >I did a quick & small test with random updates on 16 tables with
> >checkpoint_flush_after=16 checkpoint_timeout=30
> 
> Another run with more "normal" settings and over 1000 seconds, so less
> "quick & small" that the previous one.
> 
>  checkpoint_flush_after = 16
>  checkpoint_timeout = 5min # default
>  shared_buffers = 2GB # 1/8 of available memory
> 
> Random updates on 16 tables which total to 1.1GB of data, so this is in
> buffer, no significant "read" traffic.
> 
> (1) with 16 tablespaces (1 per table) on 1 disk : 680.0 tps
> per second avg, stddev [ min q1 median d3 max ] <=300tps
> 679.6 ± 750.4 [0.0, 317.0, 371.0, 438.5, 2724.0] 19.5%
> 
> (2) with 1 tablespace on 1 disk : 956.0 tps
> per second avg, stddev [ min q1 median d3 max ] <=300tps
> 956.2 ± 796.5 [3.0, 488.0, 583.0, 742.0, 2774.0] 2.1%

Interesting. That doesn't reflect my own tests, even on rotating media,
at all. I wonder if it's related to:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=23d0127096cb91cb6d354bdc71bd88a7bae3a1d5

If you use your 12.04 kernel, that'd not be fixed. Which might be a
reason to do it as you suggest.

Could you share the exact details of that workload?

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: Failover Slots

[Oleksii Kliukin]

Hi,

> On 16 Feb 2016, at 09:11, Craig Ringer  wrote:
> 
> 
> 
> Revision attached. There was a file missing from the patch too.
> 

All attached patches apply normally. I only took a look at first 2, but also 
tried to run the Patroni with the modified version to check whether the basic 
replication works.

What it’s doing is calling pg_basebackup first to initialize the replica, and 
that actually failed with:

_basebackup: unexpected termination of replication stream: ERROR:  requested 
WAL segment 0001 has already been removed

The segment name definitely looks bogus to me.

The actual command causing the failure was an attempt to clone the replica 
using pg_basebackup, turning on xlog streaming:

pg_basebackup --pgdata data/postgres1 --xlog-method=stream 
--dbname="host=localhost port=5432  user=replicator”

I checked the same command against the  git master without the patches applied 
and could not reproduce this problem there.

On the code level, I have no comments on 0001, it’s well documented and I have 
no questions about the approach, although I might be not too knowledgable to 
judge the specifics of the implementation.

On the 0002, there are a few rough edges:


slots.c:294
elog(LOG, "persistency is %i", (int)slot->data.persistency);

Should be changed to DEBUG?

slot.c:468
Why did you drop “void" as a parameter type of ReplicationSlotDropAcquired?

walsender.c: 1509 at PhysicalConfirmReceivedLocation

I’ve noticed a comment stating that we don’t need to call 
ReplicationSlotSave(), but that pre-dated the WAL-logging of replication slot 
changes. Don’t we need to call it now, the same way it’s done for the logical 
slots in logical.c:at LogicalConfirmReceivedLocation?

Kind regards,
--
Oleksii

Re: [HACKERS] Writing new unit tests with PostgresNode

[Alvaro Herrera]

Craig Ringer wrote:

> > +=pod
> > +
> > +=head2 Set up a node
> > pod format... Do we really want that? Considering that those modules
> > are only aimed at being dedicated for in-core testing, I would say no.
> 
> If it's plain comments you have to scan through massive piles of verbose
> Perl to find what you want. If it's pod you can just perldoc
> /path/to/module it and get a nice summary of the functions etc.
> 
> If these are intended to become usable facilities for people to write tests
> with then I think it's important that the docs be reasonably accessible.

Yes, I think adding POD here is a good idea.  I considered doing it
myself back when I was messing with PostgresNode ...

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


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

Re: [HACKERS] psql metaqueries with \gexec

[Daniel Verite]

Corey Huinker wrote:

> ...and query text visibility, and result visibility, and error handling,
> etc. In this case, we're leveraging the psql environment we'd already set
> up, and if there's an error, \set ECHO queries shows us the errant SQL as
> if we typed it ourselves..

BTW, about error handling, shouldn't it honor ON_ERROR_STOP ?

With the patch when trying this:

=> set ON_ERROR_STOP on
=> select * from (values ('select 1/0', 'select 1/0')) AS n \gexec

it produces two errors:
ERROR:  division by zero
ERROR:  division by zero

I'd rather have the execution stop immediately after the first error,
like it's the case with successive queries entered normally via the
query buffer:

=> \set ON_ERROR_STOP on
=> select 1/0; select 1/0;
ERROR:  division by zero

as opposed to:

=> \set ON_ERROR_STOP off
=> select 1/0; select 1/0;
ERROR:  division by zero
ERROR:  division by zero


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite


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

Re: [HACKERS] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

[Tom Lane]

Stephen Frost  writes:
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> ... However, this is one of the big problems that
>> we'd have to have a solution for before we ever consider allowing
>> read-write parallelism.

> Having such a blocker for read-write parallelism would be unfortunate,
> though perhaps there isn't much help for it, and having read-only query
> parallelism is certainly far better than nothing.

IIUC, this is not the only large problem standing between us and
read-write parallelism, and probably not even the biggest one.
So I'm basically just asking that it gets documented while it's
fresh in mind, rather than leaving it for some future hackers to
rediscover the hard way.  (Wouldn't be bad to doc the other known
stumbling blocks, too.)

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] postgres_fdw vs. force_parallel_mode on ppc

[Tom Lane]

Robert Haas  writes:
> On Mon, Feb 22, 2016 at 11:02 AM, Thomas Munro
>  wrote:
>> The postgres_fdw failure is a visibility-of-my-own-uncommitted-work
>> problem.  The first command in a transaction updates a row via an FDW,
>> and then the SELECT expects to see the effects, but when run in a
>> background worker it creates a new FDW connection that can't see the
>> uncommitted UPDATE.

> Foreign tables are supposed to be categorically excluded from
> parallelism.  Not sure why that's not working in this instance.

I've not looked at the test case to see if this is exactly what's
going wrong, but it's pretty easy to see how there might be a problem:
consider a STABLE user-defined function that does a SELECT from a foreign
table.  If that function call gets pushed down into a parallel worker
then it would fail as described.

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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Robert Haas  writes:
> > On Wed, Feb 17, 2016 at 9:48 PM, Tom Lane  wrote:
> >> I just had a rather disturbing thought to the effect that this entire
> >> design --- ie, parallel workers taking out locks for themselves --- is
> >> fundamentally flawed.  As far as I can tell from README.parallel,
> >> parallel workers are supposed to exit (and, presumably, release their
> >> locks) before the leader's transaction commits.  Releasing locks before
> >> commit is wrong.  Do I need to rehearse why?
> 
> > No, you don't.  I've spent a good deal of time thinking about that problem.
> > [ much snipped ]
> > Unless I'm missing something, though, this is a fairly obscure
> > problem.  Early release of catalog locks is desirable, and locks on
> > scanned tables should be the same locks (or weaker) than already held
> > by the master.  Other cases are rare.  I think.  It would be good to
> > know if you think otherwise.
> 
> After further thought, what I think about this is that it's safe so long
> as parallel workers are strictly read-only.  Given that, early lock
> release after user table access is okay for the same reasons it's okay
> after catalog accesses.  However, this is one of the big problems that
> we'd have to have a solution for before we ever consider allowing
> read-write parallelism.

Having such a blocker for read-write parallelism would be unfortunate,
though perhaps there isn't much help for it, and having read-only query
parallelism is certainly far better than nothing.

> So what distresses me about the current situation is that this is a large
> stumbling block that I don't see documented anywhere.  It'd be good if you
> transcribed some of this conversation into README.parallel.

Agreed.

> (BTW, I don't believe your statement that transferring locks back to the
> master would be deadlock-prone.  If the lock system treats locks held by
> a lock group as effectively all belonging to one entity, then granting a
> lock identical to one already held by another group member should never
> fail.  I concur that it might be expensive performance-wise, though it
> hardly seems like this would be a dominant factor compared to all the
> other costs of setting up and tearing down parallel workers.)

This is only when a parallel worker is finished, no?  Isn't there
already some indication of when a parallel worker is done that the
master handles, where it could also check the shared lock table and see
if any locks were transferred to it on worker exit?

Only following this thread from afar, so take my suggestions with a
grain of salt.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] FDW: should GetFdwRoutine be called when drop table?

[Tom Lane]

Robert Haas  writes:
> On Sat, Feb 20, 2016 at 1:43 AM, Tom Lane  wrote:
>> Yes, that's exactly the problem: you'd need some sort of atomic commit
>> mechanism to make this work safely.
>> 
>> It's possible we could give FDWs a bunch of hooks that would let them
>> manage post-commit cleanup the same way smgr does, but it's a far larger
>> project than it might have seemed.

> I've been thinking about the idea of letting foreign data wrappers
> have either (a) a relfilenode that is not zero, representing local
> storage; or perhaps even (b) an array of relfilenodes.  The
> relfilenode, or relfilenodes, would be automatically dropped.  It
> seems like this would be handy for things like cstore_fdw or the
> problem mentioned here, where you do want to manage local storage.

Hmm, mumble.  This assumes that the "FDW" is willing to keep its data
in something that looks externally just like a Postgres heap file (as
opposed to, say, keeping it somewhere else in the filesystem).  That
pretty much gives up the notion that this is "foreign" data access and
instead means that you're trying to force-fit an alternate storage
manager into our FDW-shaped slot.  I doubt it will fit very well.
For one thing, we have never supposed that FDWs were 100% responsible
for managing the data they access, which is why they are not hooked up
to either DROP or a boatload of other maintenance activities like VACUUM,
CLUSTER, REINDEX, etc.  Not to mention that an alternate storage manager
might have its own maintenance activities that don't really fit any of
those concepts.

> If you then also had the generic XLOG patch, maybe you could make it
> WAL-logged, too, if you wanted...

While I've not paid close attention, I had the idea that the "generic
XLOG" patches that have been discussed would still be restricted to
dealing with data that fits into Postgres-style pages (because, for
example, it would have to pass bufmgr's page sanity checks).  That's a
restriction that an alternate storage manager would likely not want.

My point remains that building anything actually useful in this space
is not a small task.

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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

[Tom Lane]

Robert Haas  writes:
> On Mon, Feb 22, 2016 at 2:56 AM, Tom Lane  wrote:
> !held by the indicated process.  False indicates that this process is
> !currently waiting to acquire this lock, which implies that at
> least one other
> !process is holding a conflicting lock mode on the same lockable object.

> I know you're just updating existing language here, but this is false.
> It only implies that one other process is holding *or waiting for* a
> conflicting lock mode on the same lockable object.

True.  I had considered whether to fix that point as well, and decided
that it might just be overcomplicating matters.  But since you complain,
I'll add "or waiting for".

It also occurred to me last night that pg_blocking_pids() needs a
disclaimer similar to the existing one for pg_locks about how using it
a lot could put a performance drag on the system.

Other than adjusting those points, I think this is ready to go, and
will commit later today if I hear no objections.

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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

[Tom Lane]

Robert Haas  writes:
> On Wed, Feb 17, 2016 at 9:48 PM, Tom Lane  wrote:
>> I just had a rather disturbing thought to the effect that this entire
>> design --- ie, parallel workers taking out locks for themselves --- is
>> fundamentally flawed.  As far as I can tell from README.parallel,
>> parallel workers are supposed to exit (and, presumably, release their
>> locks) before the leader's transaction commits.  Releasing locks before
>> commit is wrong.  Do I need to rehearse why?

> No, you don't.  I've spent a good deal of time thinking about that problem.
> [ much snipped ]
> Unless I'm missing something, though, this is a fairly obscure
> problem.  Early release of catalog locks is desirable, and locks on
> scanned tables should be the same locks (or weaker) than already held
> by the master.  Other cases are rare.  I think.  It would be good to
> know if you think otherwise.

After further thought, what I think about this is that it's safe so long
as parallel workers are strictly read-only.  Given that, early lock
release after user table access is okay for the same reasons it's okay
after catalog accesses.  However, this is one of the big problems that
we'd have to have a solution for before we ever consider allowing
read-write parallelism.

So what distresses me about the current situation is that this is a large
stumbling block that I don't see documented anywhere.  It'd be good if you
transcribed some of this conversation into README.parallel.

(BTW, I don't believe your statement that transferring locks back to the
master would be deadlock-prone.  If the lock system treats locks held by
a lock group as effectively all belonging to one entity, then granting a
lock identical to one already held by another group member should never
fail.  I concur that it might be expensive performance-wise, though it
hardly seems like this would be a dominant factor compared to all the
other costs of setting up and tearing down parallel workers.)

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] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Tue, Feb 16, 2016 at 4:19 PM, Masahiko Sawada  wrote:
> On Mon, Feb 15, 2016 at 2:54 PM, Michael Paquier
>  wrote:
>> On Mon, Feb 15, 2016 at 2:11 PM, Kyotaro HORIGUCHI wrote:
>>> Surprizingly yes. The list is handled as an identifier list and
>>> parsed by SplitIdentifierString thus it can accept double-quoted
>>> names.
>>
>
> Attached latest version patch which has only feature logic so far.
> I'm writing document patch about this feature now, so this version
> patch doesn't have document and regression test patch.

Thanks for updating the patch!

When I changed s_s_names to 'hoge*' and reloaded the configuration file,
the server crashed unexpectedly with the following error message.
This is obviously a bug.

FATAL:  syntax error

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] FDW: should GetFdwRoutine be called when drop table?

[Robert Haas]

On Sat, Feb 20, 2016 at 1:43 AM, Tom Lane  wrote:
> Andres Freund  writes:
>> On 2016-02-19 14:18:19 -0500, Peter Eisentraut wrote:
>>> On 2/19/16 12:21 PM, Feng Tian wrote:
 I have an fdw that each foreign table will acquire some persisted resource.
>
>>> But foreign data wrappers are meant to be wrappers around data managed
>>> elsewhere, not their own storage managers (although that is clearly
>>> tempting), so there might well be other places where this breaks down.
>
>> Sounds like even a BEGIN;DROP TABLE foo;ROLLBACK; will break this
>> approach.
>
> Yes, that's exactly the problem: you'd need some sort of atomic commit
> mechanism to make this work safely.
>
> It's possible we could give FDWs a bunch of hooks that would let them
> manage post-commit cleanup the same way smgr does, but it's a far larger
> project than it might have seemed.

I've been thinking about the idea of letting foreign data wrappers
have either (a) a relfilenode that is not zero, representing local
storage; or perhaps even (b) an array of relfilenodes.  The
relfilenode, or relfilenodes, would be automatically dropped.  It
seems like this would be handy for things like cstore_fdw or the
problem mentioned here, where you do want to manage local storage.  If
you then also had the generic XLOG patch, maybe you could make it
WAL-logged, too, if you wanted...

-- 
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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

[Robert Haas]

On Mon, Feb 22, 2016 at 2:56 AM, Tom Lane  wrote:
> I wrote:
>> Robert Haas  writes:
>>> As for the patch itself, I'm having trouble grokking what it's trying
>>> to do.  I think it might be worth having a comment defining precisely
>>> what we mean by "A blocks B".  I would define "A blocks B" in general
>>> as either A holds a lock which conflicts with one sought by B
>>> (hard-blocked) or A awaits a lock which conflicts with one sought by B
>>> and precedes it in the wait queue (soft-blocked).
>
>> Yes, that is exactly what I implemented ... and it's something you can't
>> find out from pg_locks.  I'm not sure how that view could be made to
>> expose wait-queue ordering.
>
> Here's an updated version of this patch, now with user-facing docs.
>
> I decided that "pg_blocking_pids()" is a better function name than
> "pg_blocker_pids()".  The code's otherwise the same, although I
> revisited some of the comments.
>
> I also changed quite a few references to "transaction" into "process"
> in the discussion of pg_locks.  The previous choice to conflate
> processes with transactions was never terribly wise in my view, and
> it's certainly completely broken by parallel query.

!held by the indicated process.  False indicates that this process is
!currently waiting to acquire this lock, which implies that at
least one other
!process is holding a conflicting lock mode on the same lockable object.

I know you're just updating existing language here, but this is false.
It only implies that one other process is holding *or waiting for* a
conflicting lock mode on the same lockable object.  Other than that, I
think the documentation changes look good.

-- 
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] Re: [COMMITTERS] pgsql: Add some isolation tests for deadlock detection and resolution.

[Robert Haas]

On Wed, Feb 17, 2016 at 9:48 PM, Tom Lane  wrote:
> I just had a rather disturbing thought to the effect that this entire
> design --- ie, parallel workers taking out locks for themselves --- is
> fundamentally flawed.  As far as I can tell from README.parallel,
> parallel workers are supposed to exit (and, presumably, release their
> locks) before the leader's transaction commits.  Releasing locks before
> commit is wrong.  Do I need to rehearse why?

No, you don't.  I've spent a good deal of time thinking about that problem.

In typical cases, workers are going to be acquiring either catalog
locks (which are released before commit) or locks on relations which
the leader has already locked (in which case the leader will still
hold the lock - or possibly a stronger one - even after the worker
releases that lock).  Suppose, however, that you write a function
which goes and queries some other table not involved in the query, and
therefore acquires a lock on it.  If you mark that function PARALLEL
SAFE and it runs only in the worker and not in in the leader, then you
could end up with a parallel query that releases the lock before
commit where a non-parallel version of that query would have held the
lock until transaction commit.  Of course, one answer to this problem
is - if the early lock release is apt to be a problem for you - don't
mark such functions PARALLEL SAFE.

I've thought about engineering a better solution.  Two possible
designs come to mind.  First, we could have the worker send to the
leader a list of locks that it holds at the end of its work, and the
leader could acquire all of those before confirming to the worker that
it is OK to terminate.  That has some noteworthy disadvantages, like
being prone to deadlock and requiring workers to stick around
potentially quite a bit longer than they do at present, thus limiting
the ability of other processes to access parallel query.  Second, we
could have the workers reassign all of their locks to the leader in
the lock table (unless the leader already holds that lock).  The
problem with that is that then the leader is in the weird situation of
having locks in the shared lock table that it doesn't know anything
about - they don't appear in it's local lock table.  How does the
leader decide which resource owner they go with?

Unless I'm missing something, though, this is a fairly obscure
problem.  Early release of catalog locks is desirable, and locks on
scanned tables should be the same locks (or weaker) than already held
by the master.  Other cases are rare.  I think.  It would be good to
know if you think otherwise.

-- 
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] Writing new unit tests with PostgresNode

[Craig Ringer]

On 22 February 2016 at 20:21, Michael Paquier 
wrote:

> On Mon, Feb 22, 2016 at 7:55 PM, Craig Ringer 
> wrote:
> > Er, the patch is attached this time.
>
>  top_builddir = ../..
>  include $(top_builddir)/src/Makefile.global
>
> -SUBDIRS = regress isolation modules
> +SUBDIRS = regress isolation modules example_suite
>
> I have no problem with a test suite to be included in SUBDIRS, but if
> that's a template test it would be better if added in ALWAYS_SUBDIRS.
>

Fair enough. My thinking in adding it to SUBDIRS is that it's trivial
enough to cost practically nothing, and things that aren't run by default
tend to bitrot.


> I think as well that having it in src/test/perl/example_suite would
> make more sense.
>

Yeah, it probably would.


> --- /dev/null
> +++ b/src/test/README
> @@ -0,0 +1,37 @@
> +This directory contains a variety of test infrastructure as well as some
> of the
> +tests in PostgreSQL. Not all tests are here - in particular, there are
> more in
> +individual contrib/ modules and in src/bin.
> The usual format for README files is more like that:
>

Can do.


>
> +use strict;
> +use warnings;
> +use 5.8.8;
> +use PostgresNode;
> +use TestLib;
> I am -1 for including a forced version number in the list of
> inclusions. We have been careful about not using routines that are not
> supported with perl < 5.8.8. Let's continue like that. Simply
> mentioning in the README this minimal requirement is enough IMO.
>

Ok, no complaint from me.


>
> +
> +=pod
> +
> +=head2 Set up a node
> pod format... Do we really want that? Considering that those modules
> are only aimed at being dedicated for in-core testing, I would say no.
>

If it's plain comments you have to scan through massive piles of verbose
Perl to find what you want. If it's pod you can just perldoc
/path/to/module it and get a nice summary of the functions etc.

If these are intended to become usable facilities for people to write tests
with then I think it's important that the docs be reasonably accessible.
You don't write Test::More tests by reading Test/More.pm, you read its
perldoc. Same deal.

SGML ghastly to write and would separate the test docs from the tests
themselves, so I think it's pretty much the worst of all worlds.

So. Yes. I do think pod is desirable. Plus anything but the most extremely
primitive editor highlights it nicely and it's trivial to write even if,
like me, you haven't really used Perl in five years.

Have a look at

perldoc src/test/perl/PostgresNode.pm

(and imagine there was a SYNOPSIS and EXAMPLES section, which there isn't
yet). I think that's a *lot* more readable than dredging through the
sources, and it's only going to get more so as the functionality grows.

diff --git a/src/test/mb/.gitignore b/src/test/mb/.gitignore
> new file mode 100644
> index 000..6628455
> --- /dev/null
> +++ b/src/test/mb/.gitignore
> @@ -0,0 +1 @@
> +/results/
> This should be a separate patch.
>

Eh, sure.


> This patch is going a bit more far than I expected first, I thought
> that this would be only a set of README files added into core.


Yeah, as I said I realised while writing the "how to write tests" and "how
to write modules" parts that I'd basically be writing code-in-a-README that
was guaranteed to bitrot into uselessness since it couldn't be run and
tested automatically. A sample module with some comments/pod describing
what it does is a much, much more useful way to achieve that end and the
README can just refer to it.


> And it
> is not completely clear what we would gain by using perlpod in the
> case of those in-core modules


 As above, readability when you're authoring tests not hacking on the test
framework.

I really don't like writing Perl. The last thing I want to do is read
through reams of Perl to find out if TestLib or PostgresNode have some
functionality I need in a test or if I need to add it / do it in my own
code. I certainly don't want to be doing that repeatedly when I'm just
trying to look up the signature for methods as I write tests.

It might make more sense if you think of it as infrastructure that lets
people write tests, not the end product its self. In an ideal world nobody
would have to look at it, they'd just use it. Do you read pg_regress.c when
you're writing regression tests? Nope, you read the README and some
examples. Same deal.

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

Re: [HACKERS] checkpointer continuous flushing - V18

[Fabien COELHO]

I did a quick & small test with random updates on 16 tables with 
checkpoint_flush_after=16 checkpoint_timeout=30


Another run with more "normal" settings and over 1000 seconds, so less 
"quick & small" that the previous one.


 checkpoint_flush_after = 16
 checkpoint_timeout = 5min # default
 shared_buffers = 2GB # 1/8 of available memory

Random updates on 16 tables which total to 1.1GB of data, so this is in 
buffer, no significant "read" traffic.


(1) with 16 tablespaces (1 per table) on 1 disk : 680.0 tps
per second avg, stddev [ min q1 median d3 max ] <=300tps
679.6 ± 750.4 [0.0, 317.0, 371.0, 438.5, 2724.0] 19.5%

(2) with 1 tablespace on 1 disk : 956.0 tps
per second avg, stddev [ min q1 median d3 max ] <=300tps
956.2 ± 796.5 [3.0, 488.0, 583.0, 742.0, 2774.0] 2.1%

--
Fabien.
--
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] Writing new unit tests with PostgresNode

[Michael Paquier]

On Mon, Feb 22, 2016 at 7:55 PM, Craig Ringer  wrote:
> Er, the patch is attached this time.

 top_builddir = ../..
 include $(top_builddir)/src/Makefile.global

-SUBDIRS = regress isolation modules
+SUBDIRS = regress isolation modules example_suite

I have no problem with a test suite to be included in SUBDIRS, but if
that's a template test it would be better if added in ALWAYS_SUBDIRS.
I think as well that having it in src/test/perl/example_suite would
make more sense.

--- /dev/null
+++ b/src/test/README
@@ -0,0 +1,37 @@
+This directory contains a variety of test infrastructure as well as some of the
+tests in PostgreSQL. Not all tests are here - in particular, there are more in
+individual contrib/ modules and in src/bin.
The usual format for README files is more like that:
path/to/README

Nice title


Text, blah.

I think that it would be better to stick with the common style in
place. Except that this file is definitely a good addition.

+use strict;
+use warnings;
+use 5.8.8;
+use PostgresNode;
+use TestLib;
I am -1 for including a forced version number in the list of
inclusions. We have been careful about not using routines that are not
supported with perl < 5.8.8. Let's continue like that. Simply
mentioning in the README this minimal requirement is enough IMO.

+
+=pod
+
+=head2 Set up a node
pod format... Do we really want that? Considering that those modules
are only aimed at being dedicated for in-core testing, I would say no.

diff --git a/src/test/mb/.gitignore b/src/test/mb/.gitignore
new file mode 100644
index 000..6628455
--- /dev/null
+++ b/src/test/mb/.gitignore
@@ -0,0 +1 @@
+/results/
This should be a separate patch.

--- /dev/null
+++ b/src/test/modules/README
@@ -0,0 +1,10 @@
+src/test/modules contains PostgreSQL extensions that are primarily or entirely
+intended for testing PostgreSQL and/or to serve as example code. The extensions
+here aren't intended to be installed in a production server and aren't useful
+for "real work".
Header format should be reworked here as well.

This patch is going a bit more far than I expected first, I thought
that this would be only a set of README files added into core. And it
is not completely clear what we would gain by using perlpod in the
case of those in-core modules.
-- 
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] Optimization for updating foreign tables in Postgres FDW

[Rushabh Lathia]

I did another round of review for the latest patch and well as performed
the sanity test, and
haven't found any functional issues. Found couple of issue, see in-line
comments
for the same.

On Thu, Feb 18, 2016 at 3:15 PM, Etsuro Fujita 
wrote:

> On 2016/02/12 21:19, Etsuro Fujita wrote:
>
>> Comments on specific points follow.
>>>
>>> This seems to need minor rebasing in the wake of the join pushdown patch.
>>>
>>
> Will do.
>>
>
> Done.
>
> +   /* Likewise for ForeignScan that has pushed down
>>> INSERT/UPDATE/DELETE */
>>> +   if (IsA(plan, ForeignScan) &&
>>> +   (((ForeignScan *) plan)->operation == CMD_INSERT ||
>>> +((ForeignScan *) plan)->operation == CMD_UPDATE ||
>>> +((ForeignScan *) plan)->operation == CMD_DELETE))
>>> +   return;
>>>
>>> I don't really understand why this is a good idea.  Since target lists
>>> are only displayed with EXPLAIN (VERBOSE), I don't really understand
>>> what is to be gained by suppressing them in any case at all, and
>>> therefore I don't understand why it's a good idea to do it here,
>>> either.  If there is a good reason, maybe the comment should explain
>>> what it is.
>>>
>>
> I think that displaying target lists would be confusing for users.
>>
>
> There seems no objection from you (or anyone), I left that as proposed,
> and added more comments.
>
> +   /* Check point 1 */
>>> +   if (operation == CMD_INSERT)
>>> +   return false;
>>> +
>>> +   /* Check point 2 */
>>> +   if (nodeTag(subplan) != T_ForeignScan)
>>> +   return false;
>>> +
>>> +   /* Check point 3 */
>>> +   if (subplan->qual != NIL)
>>> +   return false;
>>> +
>>> +   /* Check point 4 */
>>> +   if (operation == CMD_UPDATE)
>>>
>>> These comments are referring to something in the function header
>>> further up, but you could instead just delete the stuff from the
>>> header and mention the actual conditions here.
>>>
>>
> Will fix.
>>
>
> Done.
>
> The patch doesn't allow the postgres_fdw to push down an UPDATE/DELETE on
> a foreign join, so I added one more condition here not to handle such
> cases.  (I'm planning to propose a patch to handle such cases, in the next
> CF.)
>

I think we should place the checking foreign join condition before the
target columns, as foreign join condition is less costly then the target
columns.


>
> - If the first condition is supposed accept only CMD_UPDATE or
>>> CMD_DELETE, say if (operation != CMD_UPDATE || operation !=
>>> CMD_DELETE) rather than doing it this way.  Is-not-insert may in this
>>> context be functionally equivalent to is-update-or-delete, but it's
>>> better to write the comment and the code so that they exactly match
>>> rather than kinda-match.
>>>
>>> - For point 2, use IsA(subplan, ForiegnScan).
>>>
>>
> Will fix.
>>
>
> Done.
>
> +   /*
>>> +* ignore subplan if the FDW pushes down the
>>> command to the remote
>>> +* server
>>> +*/
>>>
>>> This comment states what the code does, instead of explaining why it
>>> does it.  Please update it so that it explains why it does it.
>>>
>>
> Will update.
>>
>
> Done.
>
> +   List   *fdwPushdowns;   /* per-target-table FDW
>>> pushdown flags */
>>>
>>> Isn't a list of booleans an awfully inefficient representation?  How
>>> about a Bitmapset?
>>>
>>
> OK, will do.
>>
>
> Done.
>
> +   /*
>>> +* Prepare remote-parameter expressions for evaluation.
>>> (Note: in
>>> +* practice, we expect that all these expressions will be just
>>> Params, so
>>> +* we could possibly do something more efficient than using
>>> the full
>>> +* expression-eval machinery for this.  But probably there
>>> would be little
>>> +* benefit, and it'd require postgres_fdw to know more than is
>>> desirable
>>> +* about Param evaluation.)
>>> +*/
>>> +   dpstate->param_exprs = (List *)
>>> +   ExecInitExpr((Expr *) fsplan->fdw_exprs,
>>> +(PlanState *) node);
>>>
>>> This is an exact copy of an existing piece of code and its associated
>>> comment.  A good bit of the surrounding code is copied, too.  You need
>>> to refactor to avoid duplication, like by putting some of the code in
>>> a new function that both postgresBeginForeignScan and
>>> postgresBeginForeignModify can call.
>>>
>>> execute_dml_stmt() has some of the same disease.
>>>
>>
> Will do.
>>
>
> Done.
>
> Other changes:
>
> * I fixed docs as discussed before with Rushabh Lathia and Thom Brown.
>
> * I keep Rushabh's code change that we call PlanDMLPushdown only when all
> the required APIs are available with FDW, but for CheckValidResultRel, I
> left the code as-is (no changes to that function), to match the docs saying
> that the FDW needs to provide the DML pushdown callback functions 

Re: [HACKERS] postgres_fdw vs. force_parallel_mode on ppc

[Robert Haas]

On Mon, Feb 22, 2016 at 11:02 AM, Thomas Munro
 wrote:
> On Tue, Feb 16, 2016 at 12:12 PM, Noah Misch  wrote:
>> On Mon, Feb 15, 2016 at 06:07:48PM -0500, Tom Lane wrote:
>>> Noah Misch  writes:
>>> > I configured a copy of animal "mandrill" that way and launched a test run.
>>> > The postgres_fdw suite failed as attached.  A manual "make -C contrib
>>> > installcheck" fails the same way on a ppc64 GNU/Linux box, but it passes 
>>> > on
>>> > x86_64 and aarch64.  Since contrib test suites don't recognize 
>>> > TEMP_CONFIG,
>>> > check-world passes everywhere.
>>>
>>> Hm, is this with or without the ppc-related atomics fix you just found?
>>
>> Without those.  The ppc64 GNU/Linux configuration used gcc, though, and the
>> atomics change affects xlC only.  Also, the postgres_fdw behavior does not
>> appear probabilistic; it failed twenty times in a row.
>
> The postgres_fdw failure is a visibility-of-my-own-uncommitted-work
> problem.  The first command in a transaction updates a row via an FDW,
> and then the SELECT expects to see the effects, but when run in a
> background worker it creates a new FDW connection that can't see the
> uncommitted UPDATE.
>
> I wonder if parallelism of queries involving an FDW should not be
> allowed if your transaction has written through the FDW.

Foreign tables are supposed to be categorically excluded from
parallelism.  Not sure why that's not working in this instance.

-- 
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] Writing new unit tests with PostgresNode

[Craig Ringer]

Er, the patch is attached this time.

On 22 February 2016 at 18:54, Craig Ringer  wrote:

> On 22 February 2016 at 15:41, Michael Paquier 
> wrote:
>
>
>>
>> >> > Sound about right? I can tidy that up a bit and turn it into a README
>> >> > and
>> >> > add a reference to that to the public tap docs to tell users where
>> to go
>> >> > if
>> >> > they want to write more tests.
>> >>
>> >> Yes, please.
>> >
>> > Will do that now.
>>
>> This is definitely independent from the efforts of the other patches.
>>
>
> Done.
>
> I got a bit carried away and added:
>
> - src/test/perl/README
> - src/test/README
> - src/test/modules/README
> - POD for src/test/perl/PostgresNode.pm
> - $node->info() function (that just returns what dump_info() printed, but
> as a string
> - $node->backup(...) support for passing -R to pg_basebackup
> - $node->backup(...) support for passing -X stream to pg_basebackup
> -  src/test/example_suite/ with some simple demo tests
>
> I found that I was writing documentation for how to write tests that'd
> bitrot quickly and landed up writing a demo/sample test that can be run as
> part of 'make check' with --enable-tap-tests instead. Hopefully it's not
> overkill.
>
> LMK if you think it's too much and I can trim out what's unwanted.
>
> In the process I noticed a few helpers I think should be added to
> PostgresNode. I haven't added them since I didn't want this to turn into a
> big patch when it was meant to just be 'add a README', but the main things
> are:
>
> - promote (you've done this)
>
> - Facilities to make it easier to set a master up as replication-enabled
> (your patch does this, but it should use wal_level = 'logical' by default
> IMO; also I think setting autovacuum=off is very wrong and will mask
> problems)
>
> - wait_for_recovery_lsn($lsn) to wait until a hot standby passes a given
> LSN
>
> - wait_for_replication_lsn($lsn, $col, $appname) - wait until standby with
> name $appname (or any standby, if unspecified) passes $lsn for $col, where
> $col can be 'sent', 'write', 'flush' or 'replay'
>
>
>
>> > Not committed yet, I see. That's
>> https://commitfest.postgresql.org/9/438/
>> > right?
>>
>> Yeah... That's life.
>>
>>
> I'll comment separately on that thread.
>
> --
>  Craig Ringer   http://www.2ndQuadrant.com/
>  PostgreSQL Development, 24x7 Support, Training & Services
>



-- 
 Craig Ringer   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services
From 0271314446293a61457f0b8e6a77d784804bae43 Mon Sep 17 00:00:00 2001
From: Craig Ringer 
Date: Mon, 22 Feb 2016 18:54:24 +0800
Subject: [PATCH] Add README and example to document how to create TAP tests

---
 src/test/Makefile   |   2 +-
 src/test/README |  37 
 src/test/example_suite/.gitignore   |   1 +
 src/test/example_suite/Makefile |   9 +
 src/test/example_suite/t/001_minimal.pl | 157 ++
 src/test/mb/.gitignore  |   1 +
 src/test/modules/README |  10 +
 src/test/perl/PostgresNode.pm   | 374 +---
 src/test/perl/README|  37 
 9 files changed, 596 insertions(+), 32 deletions(-)
 create mode 100644 src/test/README
 create mode 100644 src/test/example_suite/.gitignore
 create mode 100644 src/test/example_suite/Makefile
 create mode 100644 src/test/example_suite/t/001_minimal.pl
 create mode 100644 src/test/mb/.gitignore
 create mode 100644 src/test/modules/README
 create mode 100644 src/test/perl/README

diff --git a/src/test/Makefile b/src/test/Makefile
index b713c2c..e9b0136 100644
--- a/src/test/Makefile
+++ b/src/test/Makefile
@@ -12,7 +12,7 @@ subdir = src/test
 top_builddir = ../..
 include $(top_builddir)/src/Makefile.global
 
-SUBDIRS = regress isolation modules
+SUBDIRS = regress isolation modules example_suite
 
 # We don't build or execute examples/, locale/, or thread/ by default,
 # but we do want "make clean" etc to recurse into them.  Likewise for ssl/,
diff --git a/src/test/README b/src/test/README
new file mode 100644
index 000..485f20b
--- /dev/null
+++ b/src/test/README
@@ -0,0 +1,37 @@
+This directory contains a variety of test infrastructure as well as some of the
+tests in PostgreSQL. Not all tests are here - in particular, there are more in
+individual contrib/ modules and in src/bin.
+
+Not all these tests get run by "make check". Check src/test/Makefile to see
+which tests get run automatically.
+
+examples/
+  demo programs for libpq that double as regression tests via "make check"
+
+isolation/
+  tests for concurrent behaviours at the SQL level
+
+locale/
+  sanity checks for locale data, encodings, etc
+
+mb/
+  tests for multibyte encoding (UTF-8) support
+
+modules/
+  extensions used only or mainly for test purposes, generally not useful
+  or suitable for installing in production databases. 

Re: [HACKERS] Writing new unit tests with PostgresNode

[Craig Ringer]

On 22 February 2016 at 15:41, Michael Paquier 
wrote:


>
> >> > Sound about right? I can tidy that up a bit and turn it into a README
> >> > and
> >> > add a reference to that to the public tap docs to tell users where to
> go
> >> > if
> >> > they want to write more tests.
> >>
> >> Yes, please.
> >
> > Will do that now.
>
> This is definitely independent from the efforts of the other patches.
>

Done.

I got a bit carried away and added:

- src/test/perl/README
- src/test/README
- src/test/modules/README
- POD for src/test/perl/PostgresNode.pm
- $node->info() function (that just returns what dump_info() printed, but
as a string
- $node->backup(...) support for passing -R to pg_basebackup
- $node->backup(...) support for passing -X stream to pg_basebackup
-  src/test/example_suite/ with some simple demo tests

I found that I was writing documentation for how to write tests that'd
bitrot quickly and landed up writing a demo/sample test that can be run as
part of 'make check' with --enable-tap-tests instead. Hopefully it's not
overkill.

LMK if you think it's too much and I can trim out what's unwanted.

In the process I noticed a few helpers I think should be added to
PostgresNode. I haven't added them since I didn't want this to turn into a
big patch when it was meant to just be 'add a README', but the main things
are:

- promote (you've done this)

- Facilities to make it easier to set a master up as replication-enabled
(your patch does this, but it should use wal_level = 'logical' by default
IMO; also I think setting autovacuum=off is very wrong and will mask
problems)

- wait_for_recovery_lsn($lsn) to wait until a hot standby passes a given LSN

- wait_for_replication_lsn($lsn, $col, $appname) - wait until standby with
name $appname (or any standby, if unspecified) passes $lsn for $col, where
$col can be 'sent', 'write', 'flush' or 'replay'



> > Not committed yet, I see. That's
> https://commitfest.postgresql.org/9/438/
> > right?
>
> Yeah... That's life.
>
>
I'll comment separately on that thread.

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

Re: [HACKERS] psql metaqueries with \gexec

[Pavel Stehule]

>
> FWIW, I also wish we had something better than format() for this stuff. I
> did create [1] towards that end, but it currently depends on some C code,
> which is cumbersome.


For the most party, I'm pretty thrilled with format(), though:
- I'll admit to being grumpy about the %1$s notation, but I have no better
suggestion.
- I'd also like it if there were a %I variant that accepted schema
qualified names and %I-ed both, though I see the inability to tell the
difference between a schema dot and a really-named-that dot.
- I'd love it if there were a %C format that took a pg_class oid and
formatted the qualified schema name. As it is i just use %s and cast the
parameter as ::regclass

The design of the "format" function is not closed. Try to send prototype
and patch. The possibility to do PostgreSQL customization was strong reason
why we didn't implemented "sprintf" and we implemented "format".

Regards

Pavel

Re: [HACKERS] Relaxing SSL key permission checks

[Christoph Berg]

Re: Tom Lane 2016-02-22 <21507.1456099...@sss.pgh.pa.us>
> Stephen Frost  writes:
> > Just to be clear, I'm not really against this patch as-is, but it
> > shouldn't be a precedent or limit us from supporting more permissive
> > permissions in other areas (or even here) if there are sensible
> > use-cases for more permissive permissions.
> 
> OK, and to be clear, I'm not against considering other use-cases and
> trying to do something appropriate for them.  I just reject the idea
> that it's unnecessary or inappropriate for us to be concerned about
> whether secret-holding files are secure.

I added the patch to the CF: https://commitfest.postgresql.org/9/532/

(I put it under "System administration" and not under "Security"
because it concerns operation.)

Christoph
-- 
c...@df7cb.de | http://www.df7cb.de/


-- 
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] checkpointer continuous flushing - V18

[Fabien COELHO]

Hallo Andres,


AFAICR I used a "flush context" for each table space in some version
I submitted, because I do think that this whole writeback logic really
does make sense *per table space*, which suggest that there should be as
many write backs contexts as table spaces, otherwise the positive effect
may going to be totally lost of tables spaces are used. Any thoughts?


Leads to less regular IO, because if your tablespaces are evenly sized
(somewhat common) you'll sometimes end up issuing sync_file_range's
shortly after each other.  For latency outside checkpoints it's
important to control the total amount of dirty buffers, and that's
obviously independent of tablespaces.


I did a quick & small test with random updates on 16 tables with 
checkpoint_flush_after=16 checkpoint_timeout=30


(1) with 16 tablespaces (1 per table, but same disk) :
tps = 1100, 27% time under 100 tps

(2) with 1 tablespace :
tps = 1200,  3% time under 100 tps

This result is logical: with one writeback context shared between 
tablespaces the sync_file_range is issued on a few buffers per file at a 
time on the 16 files, no coalescing occurs there, so this result in random 
IOs, while with one table space all writes are aggregated per file.


ISTM that this quick test shows that a writeback context are relevant per 
tablespace, as I expected.


--
Fabien.


--
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: SCRAM authentication

[Michael Paquier]

On Mon, Feb 15, 2016 at 11:05 AM, Michael Paquier
 wrote:
> On Mon, Feb 15, 2016 at 10:51 AM, Stephen Frost  wrote:
>> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>>> Stephen Frost  writes:
>>> > Why do we need pg_shadow or pg_user or related views at all..?
>>>
>>> A lot of code looks at those just to get usernames.  I am not in favor of
>>> breaking such stuff without need.
>>
>> Alright.
>>
>>> How about we just say that the password in these old views always reads
>>> out as '' even when there is a password, and we invent new views
>>> that carry real auth information?  (Hopefully in an extensible way.)
>>
>> I'd be alright with that approach, I'd just rather that any clients
>> which actually want to read the password field be updated to look at the
>> extensible and sensible base catalogs, and not some hacked up array that
>> we shoved into that field.
>
> Well, then let's mask it, and just have pg_auth_verifiers. Another
> possible problem that I can see with this patch is what do we do with
> valid_until? The last set of patches sent did not switch this field to
> be per-verifier settable. I would consider a saner approach to keep
> things simple and still do that. Allowing multiple verifiers per
> protocol is a problem, and having a solution for it would be nice.
> Should this be prioritized before having more protocols like SCRAM?
>
> FWIW, browsing through pgbouncer, it has a look at pg_shadow for
> user's password to build a basic configuration file.
>
> (My mistake, while pg_user is world-readable, that's not the case of 
> pg_shadow).

FWIW, I am going to create a new thread once I am done with the set of
patches I have in mind for the upcoming CF (yes there will be
refreshed patches), because this thread has moved on a bit larger
discussion than SCRAM itself, summarizing what is more or less the
conclusion of this thread, explaining what the patches are doing, what
they are not doing, what could be done afterwards, etc, etc. I'll keep
a clear scope regarding what I am aiming at.
-- 
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] psql metaqueries with \gexec

[Corey Huinker]

>
> I like what you've proposed, though I am wondering if you considered doing
> something server-side instead? It seems a shame to do all this work and
> exclude all other tools.
>

I have, but my solutions closely mirror the one you mention in the next
paragraph.


> I frequently find myself creating a function that is just a wrapper on
> EXECUTE for this purpose, but obviously that has transactional limitations.
>

...and query text visibility, and result visibility, and error handling,
etc. In this case, we're leveraging the psql environment we'd already set
up, and if there's an error, \set ECHO queries shows us the errant SQL as
if we typed it ourselves..


>
> FWIW, I also wish we had something better than format() for this stuff. I
> did create [1] towards that end, but it currently depends on some C code,
> which is cumbersome.


For the most party, I'm pretty thrilled with format(), though:
- I'll admit to being grumpy about the %1$s notation, but I have no better
suggestion.
- I'd also like it if there were a %I variant that accepted schema
qualified names and %I-ed both, though I see the inability to tell the
difference between a schema dot and a really-named-that dot.
- I'd love it if there were a %C format that took a pg_class oid and
formatted the qualified schema name. As it is i just use %s and cast the
parameter as ::regclass


>
> [1]
> https://github.com/decibel/trunklet-format/blob/master/doc/trunklet-format.asc


That's intense. I'll ask you about that in an off-list thread.