Re: [HACKERS] Ordering in guc.c vs. config.sgml vs. postgresql.sample.conf

[Fujii Masao]

On Mon, Apr 25, 2016 at 3:58 AM, Andres Freund  wrote:
> Hi,
>
> While working on
> http://www.postgresql.org/message-id/cabuevezwma9y+bp4fi4fe4hmpfzmjozomulvtbhhpwtcujr...@mail.gmail.com
> I once more taken aback by the total lack of consistency between the
> three files in $subject. Some of the inconsistency of guc.c vs. the rest
> comes from having separate lists for different datatypes - hard to avoid
> - but even disregarding that, there seems to be little to no
> consistency.
>
> How about we try to order them the same?

+1, especially for the ordering in config.sgml vs. postgresql.conf.sample
because that consistency makes it easy for users to use them.

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] checkpoint_flush_after documentation inconsistency

[Fujii Masao]

On Mon, Apr 25, 2016 at 4:27 AM, Andres Freund  wrote:
> On 2016-04-21 11:20:38 -0700, Andres Freund wrote:
>> On 2016-04-18 14:33:28 +0900, Fujii Masao wrote:
>> > On Fri, Apr 15, 2016 at 6:56 PM, Magnus Hagander  
>> > wrote:
>> > > The documentation says that the default value is 128Kb on Linux, but the
>> > > code says it's 256Kb.
>> > >
>> > > Not sure which one is correct, but the other one should be updated :) I'm
>> > > guessing it's a copy/paste mistake in the docs, but since I'm not sure 
>> > > I'm
>> > > not just pushing a fix.
>> >
>> > I think you're right.
>> >
>> > I also found another several small problems regarding XXX_flush_after
>> > parameters.
>>
>> Thanks for finding these, once I'm back home/office from pgconf.nyc
>> (tonight / tomorrow) I'll push a fix.
>
> Pushed a fix, could you check that you're ok with the result?

The fix looks good to me. Thanks!

Regards,

-- 
Fujii Masao


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

Re: [HACKERS] pg_dump dump catalog ACLs

[Stephen Frost]

Noah, all,

* Noah Misch (n...@leadboat.com) wrote:
> On Fri, Apr 22, 2016 at 12:31:41PM -0400, Stephen Frost wrote:
> > After looking through the code a bit, I realized that there are a lot of
> > object types which don't have ACLs at all but which exist in pg_catalog
> > and were being analyzed because the bitmask for pg_catalog included ACLs
> > and therefore was non-zero.
> > 
> > Clearing that bit for object types which don't have ACLs improved the
> > performance for empty databases quite a bit (from about 3s to a bit
> > under 1s on my laptop).  That's a 42-line patch, with comment lines
> > being half of that, which I'll push once I've looked into the other
> > concerns which were brought up on this thread.
> 
> That's good news.

Attached patch-set includes this change in patch #2.

Patch #1 is the fix for the incorrect WHERE clause.

> > Much of the remaining inefficiancy is how we query for column
> > information one table at a time (that appears to be around 300ms of the
> > 900ms or so total time).  I'm certainly interested in improving that but
> > that would require adding more complex data structures to pg_dump than
> > what we use currently (we'd want to grab all of the columns we care
> > about in an entire schema and store it locally and then provide a way to
> > look up that information, etc), so I'm not sure it'd be appropriate to
> > do now.
> 
> I'm not sure, either; I'd need to see more to decide.  If I were you, I would
> draft a patch to the point where the community can see the complexity and the
> performance change.  That should be enough to inform the choice among moving
> forward with the proposed complexity, soliciting other designs, reverting the
> original changes, or accepting for 9.6 the slowdown as it stands at that time.
> Other people may have more definite opinions already.

I'll look at doing this once I'm done with the regression test
improvements (see below).

* Noah Misch (n...@leadboat.com) wrote:
> On Fri, Apr 22, 2016 at 08:24:53PM -0400, Stephen Frost wrote:
> > * Noah Misch (n...@leadboat.com) wrote:
> > > I wrote the attached test script to verify which types of ACLs dump/reload
> > > covers.  Based on the commit message, I expected these results:
> > > 
> > >   Dumped: type, class, attribute, proc, largeobject_metadata,
> > >   foreign_data_wrapper, foreign_server,
> > >   language(in-extension), namespace(in-extension)
> > >   Not dumped: database, tablespace,
> > >   language(from-initdb), namespace(from-initdb)
> > > 
> > > Did I interpret the commit message correctly?  The script gave these 
> > > results:
> > > 
> > >   Dumped: type, class, attribute, namespace(from-initdb)
> > >   Not dumped: database, tablespace, proc, language(from-initdb)
> > 
> > You interpreted the commit message correctly and in a number of cases
> > the correct results are generated, but there's an issue in the WHERE
> > clause being used for some of the object types.
> 
> I'm wondering whether it would be a slightly better design to treat
> language(from-initdb) like language(in-extension).  Likewise for namespaces.
> The code apparently already exists to dump from-initdb namespace ACLs.  Is
> there a user interface design reason to want to distinguish the from-initdb
> and in-extension cases?

Reviewing the code, we do record from-initdb namespace privileges, and
we also record in-extension namespace privileges (see
ExecGrant_Namespace()).

We also record from-initdb language ACLs (initdb.c:2071) and
in-extension language ACLs (see ExecGrant_Language()), so I'm not
entirely sure what, if any, issue exists here either.

For both, we also grab the ACL info vs. pg_init_privs in pg_dump.

The issue in these cases is a bit of an interesting one- they are not
part of a namespace; this patch was focused on allowing users to modify,
specifically, the ACLs of objects in the 'pg_catalog' namespace.

For languages, I believe that means that we simply need to modify the
selectDumpableProcLang() function to use the same default we use for the
'pg_catalog' namespace- DUMP_COMPONENT_ACL, instead of
DUMP_COMPONENT_NONE.

What's not clear to me is what, if any, issue there is with namespaces.

Certainly, in my testing at least, if you do:

REVOKE CREATE ON SCHEMA public FROM public;

Then you get the appropriate commands from pg_dump to implement the
resulting ACLs on the public schema.  If you change the permissions back
to match what is there at initdb-time (or you just don't change them),
then there aren't any GRANT or REVOKE commands from pg_dump, but that's
correct, isn't it?

> > That's relatively straight-forward to fix, but I'm going to put
> > together a series of TAP tests to go with these fixes.  While I tested
> > various options and bits and pieces of the code while developing, this
> > really needs a proper test suite that runs through all of these
> > permutations with each change.
> 
> Sounds great.  Thanks.
> 
> > I'm planning to have that done by the end of the 

Re: [HACKERS] Why doesn't src/backend/port/win32/socket.c implement bind()?

[Tom Lane]

Michael Paquier  writes:
> Not worse, and still not enough... bowerbird complained again:
> http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird=2016-04-25%2002%3A13%3A54

That's a different symptom that seems unrelated:

cannot remove directory for 
C:\prog\bf\root\HEAD\pgsql.build\src\bin\scripts\tmp_check\data_main_21Nw\pgdata\global:
 Directory not empty at C:/Perl64/lib/File/Temp.pm line 902.
cannot remove directory for 
C:\prog\bf\root\HEAD\pgsql.build\src\bin\scripts\tmp_check\data_main_21Nw\pgdata\pg_xlog:
 Directory not empty at C:/Perl64/lib/File/Temp.pm line 902.
cannot remove directory for 
C:\prog\bf\root\HEAD\pgsql.build\src\bin\scripts\tmp_check\data_main_21Nw\pgdata:
 Permission denied at C:/Perl64/lib/File/Temp.pm line 902.
cannot remove directory for 
C:\prog\bf\root\HEAD\pgsql.build\src\bin\scripts\tmp_check\data_main_21Nw: 
Directory not empty at C:/Perl64/lib/File/Temp.pm line 902.
### Signalling QUIT to 12200 for node "main"
# Running: pg_ctl kill QUIT 12200

We've seen that one before, though less often than the port-in-use errors.
Maybe it's failing to wait long enough for server shutdown?

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] Why doesn't src/backend/port/win32/socket.c implement bind()?

[Michael Paquier]

On Mon, Apr 25, 2016 at 4:43 AM, Tom Lane  wrote:
> I took a second look at the above-quoted Microsoft documentation, and
> noticed that it specifies that this error occurs when another application
> is *bound* to the target address.  If by that they mean that the other
> app did a bind(), then indeed what we're seeing here is a conflict with
> a listening app, so that the proposed patch would detect it.  So I went
> ahead and pushed the patch --- in any case, it shouldn't make things
> any worse.

Not worse, and still not enough... bowerbird complained again:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=bowerbird=2016-04-25%2002%3A13%3A54
--
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] Rename max_parallel_degree?

[Robert Haas]

On Sun, Apr 24, 2016 at 2:42 PM, Tom Lane  wrote:
> Magnus Hagander  writes:
>> On Sun, Apr 24, 2016 at 8:23 PM, Tom Lane  wrote:
>>> FWIW, I agree with Bruce that using "degree" here is a poor choice.
>>> It's an unnecessary dependence on technical terminology that many people
>>> will not be familiar with.
>
>> FWIW, SQL Server calls it "degree of parallelism" as well (
>> https://technet.microsoft.com/en-us/library/ms188611(v=sql.105).aspx). And
>> their configuration option is "max degree of parallelism":
>> https://technet.microsoft.com/en-us/library/ms181007(v=sql.105).aspx.
>
> Yes, but both they and Oracle appear to consider "degree" to mean the
> total number of processors used, not the number of secondary jobs in
> addition to the main one.  The only thing worse than employing obscure
> technical terminology is employing it incorrectly: that way, you get to
> confuse both the users who know what it means and those who don't.

This is not so clear-cut as you are making it out to be.  For example,
see http://www.akadia.com/services/ora_parallel_processing.html - viz
"The number of parallel slave processes associated with an operation
is called its degree of parallelism", which is pretty close to what
the parameter currently called max_parallel_degree actually does.

See also 
http://searchitchannel.techtarget.com/feature/Using-parallel-SQL-to-improve-Oracle-database-performance
- "The Degree of Parallelism (DOP) defines the number of parallel
streams of execution that will be created. In the simplest case, this
translates to the number of parallel slave processes enlisted to
support your SQL's execution. However, the number of parallel
processes is more often twice the DOP. This is because each step in a
nontrivial execution plan needs to feed data into the subsequent step,
so two sets of processes are required to maintain the parallel stream
of processing."

Other sources disagree, of course, as is often the case with Oracle.  Wahoo.

Of course, we could make this value 1-based rather than 0-based, as
Peter Geoghegan suggested a while back.  But as I think I said at the
time, I think that's more misleading than helpful.  The leader
participates in the parallel plan, but typically does far less of the
work beneath the Gather node than the other nodes involved in the
query, often almost none.  In short, the leader is special.
Pretending that it's just another process involved in the parallel
group isn't doing anyone a favor.

-- 
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] [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

[Tom Lane]

Peter Eisentraut  writes:
> On 04/21/2016 08:18 PM, Tom Lane wrote:
>> Could you send along the assembler code generated by the compiler (-S
>> output) for float.c?  Maybe that would shed some light.  Probably the
>> -O0 version would be easier to read.

> Attached is a smaller test program that prints 29.9964 (same
> as failing test result) as well as its assembler code.

This doesn't prove much.  It's clear from the assembly code that the
compiler is evaluating "asin(0.5)" at compile time (and, presumably,
getting a different answer than would be produced at runtime).  But
that's entirely unsurprising given this source text: you've got a
naked application of asin() to a constant.  What we need to know,
and what this doesn't quite prove, is whether the compiler is managing
to see through the tricks that float.c uses to prevent compile-time
calculation of that value.

The fact that this produces the same number as we see in the regression
test does point to the idea that that's what's happening.  But it's not
conclusive.  Part of the reason I'm not sold is that even if the compiler
can see through those tricks when optimizing, it hardly seems like it
should do so at -O0.

If that is the answer, then the next question is how we can put more
roadblocks in the way of compile-time evaluation of asin(0.5).  Dean
suggested that creative use of "volatile" might do it, and I agree
that that sounds like a promising thing to pursue.

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] Rename max_parallel_degree?

[Robert Haas]

On Sun, Apr 24, 2016 at 2:23 PM, Tom Lane  wrote:
> FWIW, I agree with Bruce that using "degree" here is a poor choice.
> It's an unnecessary dependence on technical terminology that many people
> will not be familiar with.

And many others will.  Some made-up term that is entirely
PostgreSQL-specific is not going to be better.

> Well, that just says that we'd better reconsider *both* of those names.
> Frankly, neither of them is well chosen, and the fact that they currently
> sound unrelated is a bug not a feature.  What about something like
> "max_overall_worker_processes" and "max_session_worker_processes"?

The first one is fine except for the IMHO-fatal defect that
max_worker_processes has been around since 9.4, and I think it's far
too late to rename it.  Should we rename max_connections to
max_overall_connections at the same time?

The second one is wrong for at least two reasons: it's not a
per-session maximum, and it's not a maximum of all worker processes,
just parallel workers.

-- 
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] Add jsonb_compact(...) for whitespace-free jsonb to text

[Stephen Frost]

* Andrew Dunstan (and...@dunslane.net) wrote:
> On 04/24/2016 06:02 PM, Sehrope Sarkuni wrote:
> >AFAIK, there's also no guarantee on the specific order of the
> >resulting properties in the text representation either. I would
> >suppose it's fixed for a given jsonb value within a database major
> >version but across major versions it could change (if the
> >underlying representation changes).
> 
> The order is fixed and very unlikely to change, as it was chosen
> quite deliberately to help ensure efficient processing. Any change
> in on-disk representation of data types is something we work very
> hard to avoid, as it makes it impossible to run pg_upgrade.

We do, from time-to-time, change on-disk formats in a
backwards-compatible way though.  In any case, it's my understanding
that we don't *guarantee* any ordering currently and therefore we should
discourage users from depending on it.  If we *are* going to guarantee
ordering, then we should document what that ordering is.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Add jsonb_compact(...) for whitespace-free jsonb to text

[Andrew Dunstan]

On 04/24/2016 06:02 PM, Sehrope Sarkuni wrote:


AFAIK, there's also no guarantee on the specific order of the 
resulting properties in the text representation either. I would 
suppose it's fixed for a given jsonb value within a database major 
version but across major versions it could change (if the underlying 
representation changes).



The order is fixed and very unlikely to change, as it was chosen quite 
deliberately to help ensure efficient processing. Any change in on-disk 
representation of data types is something we work very hard to avoid, as 
it makes it impossible to run pg_upgrade.


It's true that the storage order of keys is not terribly intuitive.

Note that the json type, unlike jsonb, preserves exactly the white space 
and key order of its input. In fact, the input is exactly what it stores.


cheers

andrew



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

Re: [HACKERS] Rename max_parallel_degree?

[Bruce Momjian]

On Sun, Apr 24, 2016 at 02:23:43PM -0400, Tom Lane wrote:
> Robert Haas  writes:
> > Also, consider that we have the related but actually sorta different
> > GUC max_worker_processes.  I think max_parallel_workers to control the
> > per-query behavior and max_worker_processes to control the global
> > system behavior would be confusing - those names are very close
> > together.
> 
> Well, that just says that we'd better reconsider *both* of those names.
> Frankly, neither of them is well chosen, and the fact that they currently
> sound unrelated is a bug not a feature.  What about something like
> "max_overall_worker_processes" and "max_session_worker_processes"?

I don't think "overall" works.  I am think
"max_cluster_worker_processes" and "max_session_worker_processes" works.
I guess you could also use "max_server_worker_processes", but that is
referring to the database server, not the OS-level server.

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

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


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

[HACKERS] Add jsonb_compact(...) for whitespace-free jsonb to text

[Sehrope Sarkuni]

Hi,

The default text representation of jsonb adds whitespace in between
key/value pairs (after the colon ":") and after successive properties
(after the comma ","):

postgres=# SELECT '{"b":2,"a":1}'::jsonb::text;
   text
--
 {"a": 1, "b": 2}
(1 row)

AFAIK, there's also no guarantee on the specific order of the resulting
properties in the text representation either. I would suppose it's fixed
for a given jsonb value within a database major version but across major
versions it could change (if the underlying representation changes).

I originally ran into this when comparing the hashes of the text
representation of jsonb columns. The results didn't match up because the
javascript function JSON.stringify(...) does not add any extra whitespace.

It'd be nice to have a stable text representation of a jsonb value with
minimal whitespace. The latter would also save a few bytes per record in
text output formats, on the wire, and in backups (ex: COPY ... TO STDOUT).

Attached is a *very* work in progress patch that adds a
jsonb_compact(jsonb)::text function. It generates a text representation
without extra whitespace but does not yet try to enforce a stable order of
the properties within a jsonb value.

Here's some sample usage:

postgres=# SELECT x::text, jsonb_compact(x) FROM (VALUES ('{}'::jsonb),
('{"a":1,"b":2}'), ('[1,2,3]'), ('{"a":{"b":1}}')) AS t(x);
x | jsonb_compact
--+---
 {}   | {}
 {"a": 1, "b": 2} | {"a":1,"b":2}
 [1, 2, 3]| [1,2,3]
 {"a": {"b": 1}}  | {"a":{"b":1}}
(4 rows)

There aren't any tests yet but I'd like to continue working on it for
inclusion in 9.7. I'm posting it now to see if there's interest in the idea
and get some feedback on the approach (this is my first real patch...).

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/
diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
index 256ef80..1ecc17a 100644
--- a/src/backend/utils/adt/jsonb.c
+++ b/src/backend/utils/adt/jsonb.c
@@ -60,6 +60,13 @@ typedef struct JsonbAggState
 	Oid			val_output_func;
 } JsonbAggState;
 
+typedef enum /* type categories for jsonb to string whitespace */
+{
+	JSONBWHITESPACE_DEFAULT, /* Default style with space between keys but no indentation */
+	JSONBWHITESPACE_INDENT,  /* Extra indentation of four spaces for each nested level */
+	JSONBWHITESPACE_COMPACT  /* Compact style without extra whitespace between keys */
+} JsonbWhitespaceStyle;
+
 static inline Datum jsonb_from_cstring(char *json, int len);
 static size_t checkStringLen(size_t len);
 static void jsonb_in_object_start(void *pstate);
@@ -86,7 +93,7 @@ static void datum_to_jsonb(Datum val, bool is_null, JsonbInState *result,
 static void add_jsonb(Datum val, bool is_null, JsonbInState *result,
 		  Oid val_type, bool key_scalar);
 static JsonbParseState *clone_parse_state(JsonbParseState *state);
-static char *JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool indent);
+static char *JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, JsonbWhitespaceStyle whitespace_style);
 static void add_indent(StringInfo out, bool indent, int level);
 
 /*
@@ -427,7 +434,7 @@ jsonb_in_scalar(void *pstate, char *token, JsonTokenType tokentype)
 char *
 JsonbToCString(StringInfo out, JsonbContainer *in, int estimated_len)
 {
-	return JsonbToCStringWorker(out, in, estimated_len, false);
+	return JsonbToCStringWorker(out, in, estimated_len, JSONBWHITESPACE_DEFAULT);
 }
 
 /*
@@ -436,14 +443,23 @@ JsonbToCString(StringInfo out, JsonbContainer *in, int estimated_len)
 char *
 JsonbToCStringIndent(StringInfo out, JsonbContainer *in, int estimated_len)
 {
-	return JsonbToCStringWorker(out, in, estimated_len, true);
+	return JsonbToCStringWorker(out, in, estimated_len, JSONBWHITESPACE_INDENT);
+}
+
+/*
+ * same thing but in compact form (no extra whitespace)
+ */
+char *
+JsonbToCStringCompact(StringInfo out, JsonbContainer *in, int estimated_len)
+{
+	return JsonbToCStringWorker(out, in, estimated_len, JSONBWHITESPACE_COMPACT);
 }
 
 /*
  * common worker for above two functions
  */
 static char *
-JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool indent)
+JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, JsonbWhitespaceStyle whitespace_style)
 {
 	bool		first = true;
 	JsonbIterator *it;
@@ -452,8 +468,24 @@ JsonbToCStringWorker(StringInfo out, JsonbContainer *in, int estimated_len, bool
 	int			level = 0;
 	bool		redo_switch = false;
 
-	/* If we are indenting, don't add a space after a comma */
-	int			ispaces = indent ? 1 : 2;
+	bool		indent = false;
+	char*		prop_sep  = ", "; /* Separator between successive properties */
+	char*		key_value_sep = ": "; /* Separator between a key and it's value */
+	int 		prop_sep_len;
+	int 		key_value_sep_len;
+	if (whitespace_style == 

Re: [HACKERS] Proposed change to make cancellations safe

[Tom Lane]

Simon Riggs  writes:
> On 24 April 2016 at 17:54, Shay Rojansky  wrote:
>> I definitely agree that simply tracking message sequence numbers on both
>> sides is better. It's also a powerful feature to be able to cancel all
>> messages "up to N" - I'm thinking of a scenario where, for example, many
>> simple queries are sent and the whole process needs to be cancelled.

> For security, I think any non-matching cancellation would be ignored so
> that only someone with proper context could issue the cancel.

Well, we already have a random cancel key in the requests.  As a separate
matter for a protocol change, it might be nice to consider widening the
cancel key to make it harder to brute-force; but I disagree that the
current proposal has anything whatever to do with security.

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] Proposed change to make cancellations safe

[Simon Riggs]

On 24 April 2016 at 17:54, Shay Rojansky  wrote:


> I definitely agree that simply tracking message sequence numbers on both
> sides is better. It's also a powerful feature to be able to cancel all
> messages "up to N" - I'm thinking of a scenario where, for example, many
> simple queries are sent and the whole process needs to be cancelled.
>

For security, I think any non-matching cancellation would be ignored so
that only someone with proper context could issue the cancel.

Issuing bulk cancellations sounds like a bad plan.

Yes, this has been happening to some Npgsql users, it's not very frequent
> but it does happen from time to time. I also bumped into this in some
> automated testing scenarios. It's not the highest-value feature, but it is
> an improvement to have if you plan on working on a new protocol version.
>
> Let me know if you'd like me to update the TODO.
>

If you've got an itch, expecting someone else to scratch it is less likely
to succeed.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Suspicious behaviour on applying XLOG_HEAP2_VISIBLE.

[Simon Riggs]

On 18 April 2016 at 13:15, Simon Riggs  wrote:

> On 18 April 2016 at 12:43, Michael Paquier 
> wrote:
>
>> On Sat, Apr 16, 2016 at 5:37 AM, Alvaro Herrera
>>  wrote:
>> > Andres Freund wrote:
>> >> On 2016-04-15 15:26:17 -0400, Tom Lane wrote:
>> >> > I think the bottom line is that we misdesigned the WAL representation
>> >> > by assuming that this sort of info could always be piggybacked on a
>> >> > transaction commit record.  It's time to fix that.
>> >>
>> >> I think we got to piggyback it onto a commit record, as long as there's
>> >> one. Otherwise it's going to be more complex (queuing messages when
>> >> reading an inval record) and slower (more wal records).  I can easily
>> >> develop a patch for that, the question is what we do on the back
>> >> branches...
>> >
>> > We have introduced new wal records in back branches previously --
>> > nothing new (c.f. 8e9a16ab8f7f0e5813644975cc3f336e5b064b6e).  The user
>> > just needs to make sure to upgrade the standbys first.  If they don't,
>> > they would die upon replay of the first such record, which they can take
>> > as an indication that they need to be upgraded; the standby is down for
>> > some time, but there is no data loss or corruption.
>>
>> Yeah, introducing a new WAL record to address this issue in
>> back-branches would not be an issue, and that's what we should do. For
>> HEAD, let's add that in the commit record.
>>
>
> (non-reply just because of travel)
>
> OK, I'll write up a patch today to fix, with a view to backpatching.
>

Patch from Tuesday.  On various planes.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


inval_only.v1.patch
Description: Binary data

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

Re: [HACKERS] pgsql: Add putenv support for msvcrt from Visual Studio 2013

[Christian Ullrich]

* Magnus Hagander wrote:


Add putenv support for msvcrt from Visual Studio 2013

This was missed when VS 2013 support was added.

Michael Paquier

Branch
--
master

Details
---
http://git.postgresql.org/pg/commitdiff/9f633b404cb3be6139f8dfdea00538489ffef9ab


Just noticed something. This DLL detection by name has never worked in 
debug builds where the DLL names end in "d". Is that important?


--
Christian



--
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] Why doesn't src/backend/port/win32/socket.c implement bind()?

[Tom Lane]

I wrote:
> However, it's still not entirely clear what is the root cause of the
> failure and whether a patch along the discussed lines would prevent its
> recurrence.  Looking at TranslateSocketError, it seems we must be seeing
> an underlying error code of WSAEACCES.  A little googling says that
> Windows might indeed return that, rather than the more expected
> WSAEADDRINUSE, if someone else has the port open with SO_EXCLUSIVEADDRUSE:

>   Another possible reason for the WSAEACCES error is that when the
>   bind function is called (on Windows NT 4.0 with SP4 and later),
>   another application, service, or kernel mode driver is bound to
>   the same address with exclusive access. Such exclusive access is a
>   new feature of Windows NT 4.0 with SP4 and later, and is
>   implemented by using the SO_EXCLUSIVEADDRUSE option.

> So theory A is that some other program is binding random high port numbers
> with SO_EXCLUSIVEADDRUSE.  Theory B is that this is the handiwork of
> Windows antivirus software doing what Windows antivirus software typically
> does, ie inject random permissions failures depending on the phase of the
> moon.  It's not very clear that a test along the lines described (that is,
> attempt to connect to, not bind to, the target port) would pre-detect
> either type of error.  Under theory A, a connect() test would recognize
> the problem only if the other program were using the port to listen rather
> than make an outbound connection; and the latter seems much more likely.

I took a second look at the above-quoted Microsoft documentation, and
noticed that it specifies that this error occurs when another application
is *bound* to the target address.  If by that they mean that the other
app did a bind(), then indeed what we're seeing here is a conflict with
a listening app, so that the proposed patch would detect it.  So I went
ahead and pushed the patch --- in any case, it shouldn't make things
any worse.

Also, I did a bit of digging in the buildfarm logs, and noticed that
bowerbird and jacana together have reported 34 "could not bind socket"
failures in BinInstallCheck since 2015-12-07 (when the current logic for
selecting a random port went in).  Between 2015-01-01 and 2015-12-07,
they reported only *one* such failure.  So whatever the exact explanation
is, we've greatly increased the probability of such failures by using a
random port rather than the fixed port 65432 that was used before.
I'm not entirely sure what to make of this observation, but the statistics
seem pretty clear.

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] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan wrote:


OK, here's my final version of the patch, which I will apply in 24 hours
or so unless there is an objection.


+  Visual Studio 2008 and above. Compilation
+  is supported down to Windows XP and
+  Windows Server 2003 when building with
+  Visual Studio 2005 to
+  Visual Studio 2013. Building with
+  Visual Studio 2015 is supported down to
+  Windows Vista and Windows Server
   2008.

This paragraph contradicts itself; it first says 2008 is the minimum 
version, right after that it supports 2005, too. The last version of
Visual Studio that will install on XP, 2003, Vista, and 2008 is VS 2010 
anyway, anything released after that requires at least 7/2008R2.


I would actually just say "is supported with Visual Studio 2005 [or 
possibly 2008] and higher" instead of listing versions. I don't think we 
need to recapitulate the system requirements of individual VS releases 
and anyone trying to install 2012+ on Vista will quickly find out it 
doesn't work. It would be different if we actually excluded particular 
VS versions or VS/OS combinations that are supported by VS itself, but 
we don't.



+   /*
+* get a pointer sized version of bgchild to avoid
warnings about
+* casting to a different size on WIN64.
+*/
+   intptr_tbgchild_handle = bgchild;

If you're going to copy it anyway, why not just use a HANDLE rather than 
cast it again later? It's only used in two places, cast to HANDLE in 
both, and the special case of -1 does not matter in either.



+ * Leave a higher value in place. When building with at least Visual
+ * Studio 2015 the minimum requirement is Windows Vista (0x0600) to
+ * get support for GetLocaleInfoEx() with locales. For everything else
+ * the minumum version os Windows XP (0x0501).

s/os/is/ in the last line.


This one doesn't matter, but just for perfection's sake:

+#if (_MSC_VER >= 1900)
+   uint32  cp;
+   WCHAR   wctype[80];
+
+   memset(wctype, 0, 80 * sizeof(WCHAR));
+   MultiByteToWideChar(CP_ACP, 0, ctype, -1, wctype, 80);

The maximum length is documented as 85 characters, also:

: 
'Note   Your application must use the constant [LOCALE_NAME_MAX_LENGTH] 
for the maximum locale name length, instead of hard-coding the value "85".'


--
Christian



--
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] checkpoint_flush_after documentation inconsistency

[Andres Freund]

On 2016-04-21 11:20:38 -0700, Andres Freund wrote:
> On 2016-04-18 14:33:28 +0900, Fujii Masao wrote:
> > On Fri, Apr 15, 2016 at 6:56 PM, Magnus Hagander  
> > wrote:
> > > The documentation says that the default value is 128Kb on Linux, but the
> > > code says it's 256Kb.
> > >
> > > Not sure which one is correct, but the other one should be updated :) I'm
> > > guessing it's a copy/paste mistake in the docs, but since I'm not sure I'm
> > > not just pushing a fix.
> > 
> > I think you're right.
> > 
> > I also found another several small problems regarding XXX_flush_after
> > parameters.
> 
> Thanks for finding these, once I'm back home/office from pgconf.nyc
> (tonight / tomorrow) I'll push a fix.

Pushed a fix, could you check that you're ok with the result?

Also started 
http://archives.postgresql.org/message-id/20160424185814.n3nj2ahlutoykfou%40
to discuss making the ordering of config variables more consistent.

Thanks Magnus and Fujii

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] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan wrote:


On 04/24/2016 03:16 PM, Christian Ullrich wrote:



* Andrew Dunstan wrote:



msvcr120.dll seems to be the highest numbered one on my system, and we
already cover that. If you like we can add to the comments in that file.


There won't be a higher one, with VS 2015, CRT became a system
component, namely, UCRT. However, ucrtbase.dll does export putenv, so
it might need to be added.



OK. Does that mean we won't have to add anything more for future VS
releases?


Microsoft is swearing up one side and down the other that they will keep 
binary compatibility forever, and ever, hallelujah, hallelujah, and 
hence the name of the DLL is not expected to change again.


OTOH, the next manager of the development tools division might have 
different ideas. You never know.


--
Christian




--
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] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/24/2016 03:16 PM, Christian Ullrich wrote:

* Andrew Dunstan wrote:


On 04/24/2016 12:14 PM, Tom Lane wrote:



Andrew Dunstan  writes:
OK, here's my final version of the patch, which I will apply in 24 
hours

or so unless there is an objection.



BTW, in view of 9f633b404, shouldn't there be a similar addition to
win32env.c in this patch?



msvcr120.dll seems to be the highest numbered one on my system, and we
already cover that. If you like we can add to the comments in that file.


There won't be a higher one, with VS 2015, CRT became a system 
component, namely, UCRT. However, ucrtbase.dll does export putenv, so 
it might need to be added.





OK. Does that mean we won't have to add anything more for future VS 
releases?


cheers

andrew



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

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Christian Ullrich]

* Andrew Dunstan wrote:


On 04/24/2016 12:14 PM, Tom Lane wrote:



Andrew Dunstan  writes:

OK, here's my final version of the patch, which I will apply in 24 hours
or so unless there is an objection.



BTW, in view of 9f633b404, shouldn't there be a similar addition to
win32env.c in this patch?



msvcr120.dll seems to be the highest numbered one on my system, and we
already cover that. If you like we can add to the comments in that file.


There won't be a higher one, with VS 2015, CRT became a system 
component, namely, UCRT. However, ucrtbase.dll does export putenv, so it 
might need to be added.


--
Christian




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

[HACKERS] Ordering in guc.c vs. config.sgml vs. postgresql.sample.conf

[Andres Freund]

Hi,

While working on
http://www.postgresql.org/message-id/cabuevezwma9y+bp4fi4fe4hmpfzmjozomulvtbhhpwtcujr...@mail.gmail.com
I once more taken aback by the total lack of consistency between the
three files in $subject. Some of the inconsistency of guc.c vs. the rest
comes from having separate lists for different datatypes - hard to avoid
- but even disregarding that, there seems to be little to no
consistency.

How about we try to order them the same? That's obviously not a 9.6
topic at this point, but it'd probably be good to that early in 9.7.

- 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: Transactional enum additions - was Re: [HACKERS] Alter or rename enum value

[Andrew Dunstan]

On 04/02/2016 01:20 PM, Tom Lane wrote:

Andrew Dunstan  writes:

Looking at this briefly. It looks like the check should be called from
enum_in() and enum_recv(). What error should be raised if the enum row's
xmin isn't committed? ERRCODE_FEATURE_NOT_SUPPORTED? or maybe
ERRCODE_DATA_EXCEPTION? I don't see anything that fits very well.

ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE is something we use in some
other places where the meaning is "just wait awhile, dude".  Or you
could invent a new ERRCODE.







OK, did that. Here is a patch that is undocumented but I think is 
otherwise complete. It's been tested a bit and we haven't been able to 
break it. Comments welcome.


cheers

andrew


transactional_enum-additions-v1x.patch
Description: binary/octet-stream

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

Re: [HACKERS] Rename max_parallel_degree?

[Tom Lane]

Magnus Hagander  writes:
> On Sun, Apr 24, 2016 at 8:23 PM, Tom Lane  wrote:
>> FWIW, I agree with Bruce that using "degree" here is a poor choice.
>> It's an unnecessary dependence on technical terminology that many people
>> will not be familiar with.

> FWIW, SQL Server calls it "degree of parallelism" as well (
> https://technet.microsoft.com/en-us/library/ms188611(v=sql.105).aspx). And
> their configuration option is "max degree of parallelism":
> https://technet.microsoft.com/en-us/library/ms181007(v=sql.105).aspx.

Yes, but both they and Oracle appear to consider "degree" to mean the
total number of processors used, not the number of secondary jobs in
addition to the main one.  The only thing worse than employing obscure
technical terminology is employing it incorrectly: that way, you get to
confuse both the users who know what it means and those who don't.

The fact that we couldn't get this right seems to me to be sufficient
evidence that we should stay away from the term "degree".

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] Rename max_parallel_degree?

[Magnus Hagander]

On Sun, Apr 24, 2016 at 8:23 PM, Tom Lane  wrote:

> Robert Haas  writes:
> > On Sat, Apr 23, 2016 at 11:58 PM, Bruce Momjian 
> wrote:
> >> Why is the parallelism variable called "max_parallel_degree"?  Is that a
> >> descriptive name?  What does "degree" mean?  Why is it not called
> >> "max_parallel_workers"?
>
> > Because "degree of parallelism" is standard terminology, I guess.
>
> FWIW, I agree with Bruce that using "degree" here is a poor choice.
> It's an unnecessary dependence on technical terminology that many people
> will not be familiar with.
>

FWIW, SQL Server calls it "degree of parallelism" as well (
https://technet.microsoft.com/en-us/library/ms188611(v=sql.105).aspx). And
their configuration option is "max degree of parallelism":
https://technet.microsoft.com/en-us/library/ms181007(v=sql.105).aspx.

Oracle also call it "degree of parallelism" (
https://docs.oracle.com/cd/E18283_01/server.112/e17110/initparams176.htm).

So it's certainly not a made-up term. And I'd go as far as to say that most
people coming from other databases would be familiar with it. It may not be
a standard, but clearly it's very close to being that.

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

Re: [HACKERS] Rename max_parallel_degree?

[Tom Lane]

Robert Haas  writes:
> On Sat, Apr 23, 2016 at 11:58 PM, Bruce Momjian  wrote:
>> Why is the parallelism variable called "max_parallel_degree"?  Is that a
>> descriptive name?  What does "degree" mean?  Why is it not called
>> "max_parallel_workers"?

> Because "degree of parallelism" is standard terminology, I guess.

FWIW, I agree with Bruce that using "degree" here is a poor choice.
It's an unnecessary dependence on technical terminology that many people
will not be familiar with.

> Also, consider that we have the related but actually sorta different
> GUC max_worker_processes.  I think max_parallel_workers to control the
> per-query behavior and max_worker_processes to control the global
> system behavior would be confusing - those names are very close
> together.

Well, that just says that we'd better reconsider *both* of those names.
Frankly, neither of them is well chosen, and the fact that they currently
sound unrelated is a bug not a feature.  What about something like
"max_overall_worker_processes" and "max_session_worker_processes"?

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] Rename max_parallel_degree?

[Robert Haas]

On Sat, Apr 23, 2016 at 11:58 PM, Bruce Momjian  wrote:
> Why is the parallelism variable called "max_parallel_degree"?  Is that a
> descriptive name?  What does "degree" mean?  Why is it not called
> "max_parallel_workers"?

Because "degree of parallelism" is standard terminology, I guess.

Also, consider that we have the related but actually sorta different
GUC max_worker_processes.  I think max_parallel_workers to control the
per-query behavior and max_worker_processes to control the global
system behavior would be confusing - those names are very close
together.

-- 
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] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> On 04/24/2016 12:14 PM, Tom Lane wrote:
>> BTW, in view of 9f633b404, shouldn't there be a similar addition to
>> win32env.c in this patch?

> msvcr120.dll seems to be the highest numbered one on my system, and we 
> already cover that. If you like we can add to the comments in that file.

Yeah, something like s/Visual Studio 2013/Visual Studio 2013 and 2105/
seems like it'd be appropriate.  The main thing I'm concerned about is
that this patch touch that list somehow, so that if someone uses it as
a reference for what to consider when adding support for VS N+1, they
know to check there.

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] Rename max_parallel_degree?

[Joshua D. Drake]

On 04/23/2016 11:00 PM, Peter Geoghegan wrote:

On Sat, Apr 23, 2016 at 8:58 PM, Bruce Momjian  wrote:

Why is the parallelism variable called "max_parallel_degree"?  Is that a
descriptive name?  What does "degree" mean?  Why is it not called
"max_parallel_workers"?


I also think that "max_parallel_workers" works better.


+1.

JD


--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.


--
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] Proposed change to make cancellations safe

[Shay Rojansky]

I definitely agree that simply tracking message sequence numbers on both
sides is better. It's also a powerful feature to be able to cancel all
messages "up to N" - I'm thinking of a scenario where, for example, many
simple queries are sent and the whole process needs to be cancelled.

Yes, this has been happening to some Npgsql users, it's not very frequent
but it does happen from time to time. I also bumped into this in some
automated testing scenarios. It's not the highest-value feature, but it is
an improvement to have if you plan on working on a new protocol version.

Let me know if you'd like me to update the TODO.

On Sun, Apr 24, 2016 at 6:11 PM, Tom Lane  wrote:

> Shay Rojansky  writes:
> > The issue I'd like to tackle is the fact that it's not possible to make
> > sure a cancellation request affects a specific query.
>
> Right ...
>
> > A simple fix for this would be to have a sequence number returned in the
> > BindComplete message.
>
> First, that doesn't fix anything for simple query protocol, which doesn't
> use such a message.
>
> Second, making sure that the BindComplete gets delivered soon enough to be
> useful would require issuing a Sync between Bind and Execute.  At the
> least that doubles the number of network packets for small query results
> (so I dispute your assumption that the added network load would be
> negligible).  But a bigger issue is that it would have subtle effects
> on error handling.  An application could not blindly fire out
> Parse/Bind/Sync/Execute/Sync as one packet; it would have to wait for
> the Bind result to come back before it could know if it's safe to send
> Execute, unlike the case where it sends Parse/Bind/Execute/Sync.  (The
> server's skip-to-Sync-after-error rule is critical here.)  So actually,
> making use of this feature would double the number of network round trips,
> as well as requiring carefully thought-through changes to application
> error handling.
>
> Third, if a query is not cancellable till Execute, that prevents
> cancellation of scenarios where the parser or planner takes a long time
> for some reason.  Long planner runtimes are certainly not uncommon.
>
>
> So this is worth thinking about, but that doesn't sound like much of
> a solution to me.
>
> I wonder though why we need the server to tell the client a sequence
> number at all.  Surely both sides of the connection could easily count
> the number of messages the client has transmitted.  We could simply extend
> the cancel packet to include a sequence number field, with the semantics
> "this should only take effect if you're still working on message N".
> Or I guess maybe it should read "working on message N or before", so
> that in a case like Parse/Bind/Execute/Sync you would mention the number
> of the Execute message and still get cancellation if the query was hung
> up in Parse.
>
> Have you seen this to be a problem in practice, or is it just
> theoretical?  I do not recall many, if any, field complaints
> about the issue.
>
> regards, tom lane
>

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/24/2016 12:14 PM, Tom Lane wrote:

Andrew Dunstan  writes:

OK, here's my final version of the patch, which I will apply in 24 hours
or so unless there is an objection.

BTW, in view of 9f633b404, shouldn't there be a similar addition to
win32env.c in this patch?





msvcr120.dll seems to be the highest numbered one on my system, and we 
already cover that. If you like we can add to the comments in that file.


cheers

andrew


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

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> On 04/24/2016 11:58 AM, Tom Lane wrote:
>> Hmm ... does this pragma work on *every* compiler we're going to use
>> on Windows?

> According to my research it works on all the MSVC versions we support. I 
> didn't research the others (i.e. gcc), but we already use the pragma in 
> float.c without ill effect. Isn't the way #pragma works that compilers 
> that don't understand the particular pragma are just supposed to ignore it?

Well, the ones in float.c are guarded by "#if (_MSC_VER >= 1800)" and
will therefore not get compiled by any non-MSVC compiler.  I don't see
any entirely-unprotected #pragma uses in our tree, indicating that
we've not depended on any such assumption up to now.

Given that the whole file is platform-specific, it may well be fine;
I'm just voicing some concern.  I suppose the buildfarm will soon
tell us if it's not fine, though.

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] Proposed change to make cancellations safe

[Simon Riggs]

On 24 April 2016 at 13:04, Shay Rojansky  wrote:

The issue I'd like to tackle is the fact that it's not possible to make
> sure a cancellation request affects a specific query. Since cancellations
> are totally asynchronous and occur on another socket, they affect whatever
> query is currently being processed. This makes it possible to inadvertently
> kill a later query, because by the time the cancellation request is
> delivered and "applied" an the intended query already completed and a later
> one gets killed.
>
> A simple fix for this would be to have a sequence number returned in the
> BindComplete message. When submitting a cancellation request, the frontend
> would have the option (but not the obligation) to pass such as sequence
> number to the backend. When cancelling, the backend would make sure the
> sequence number corresponds to the currently running query.
>
> The only drawback seems to be the increased payload of the BindComplete
> message (4 bytes, or possibly less if we're really worried about it).
>
> If this makes sense, I'll add it to the TODO wiki.
>

The topic makes sense, an alternate solution may be better.

If things are sequential, both sides can +1 as appropriate. So an extra
message isn't required to confirm that. We can resync with optional
additional info at various points. Requesting cancellation of a former
sequence number can still be blocked.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Defaults for replication/backup

[Magnus Hagander]

On Sun, Apr 24, 2016 at 5:50 PM, Tom Lane  wrote:

> Magnus Hagander  writes:
> > On Thu, Apr 21, 2016 at 7:20 PM, Robert Haas 
> wrote:
> >> I think we are far too close to beta1 to begin bikeshedding this.
> >> Changing the defaults is not going to be uncontroversial, and it's not
> >> something I think we should rush into.
>
> > I think that may be a case of the good old letting perfection get in the
> > way of progress. We can be pretty sure that whatever we decide to do (now
> > or later) will not be perfect. But we can be equally sure that it will be
> > better than what we have today in most cases. But that's just me...
>
> I'm with Robert.  Such a discussion is morally indistinguishable from a
> new feature, and so we shouldn't consider it at this point in the cycle
> --- even if it seemed likely that we could easily get to a consensus,
> which I doubt.  If we'd dealt with the issue back in February when the
> previous discussion happened, that'd be fine, but now's not the time
> to decide it has to be resolved for 9.6.
>
>

Fair enough, I'll drop it for now. And try to figure out how to move it
along again once we've opened the next branch.


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

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/24/2016 11:58 AM, Tom Lane wrote:

Andrew Dunstan  writes:

OK, here's my final version of the patch, which I will apply in 24 hours
or so unless there is an objection.
+#pragma warning(push)
+#pragma warning(disable : 4091)
  #include 
+#pragma warning(pop)

Hmm ... does this pragma work on *every* compiler we're going to use
on Windows?  I'm afraid that trying to suppress a warning in VS2015
is going to result in outright failure with other compilers.





According to my research it works on all the MSVC versions we support. I 
didn't research the others (i.e. gcc), but we already use the pragma in 
float.c without ill effect. Isn't the way #pragma works that compilers 
that don't understand the particular pragma are just supposed to ignore it?


cheers

andrew


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

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> OK, here's my final version of the patch, which I will apply in 24 hours 
> or so unless there is an objection.

BTW, in view of 9f633b404, shouldn't there be a similar addition to
win32env.c in this patch?

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] VS 2015 support in src/tools/msvc

[Tom Lane]

Andrew Dunstan  writes:
> OK, here's my final version of the patch, which I will apply in 24 hours 
> or so unless there is an objection.

> +#pragma warning(push)
> +#pragma warning(disable : 4091)
>  #include 
> +#pragma warning(pop)

Hmm ... does this pragma work on *every* compiler we're going to use
on Windows?  I'm afraid that trying to suppress a warning in VS2015
is going to result in outright failure with other compilers.

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] Defaults for replication/backup

[Tom Lane]

Magnus Hagander  writes:
> On Thu, Apr 21, 2016 at 7:20 PM, Robert Haas  wrote:
>> I think we are far too close to beta1 to begin bikeshedding this.
>> Changing the defaults is not going to be uncontroversial, and it's not
>> something I think we should rush into.

> I think that may be a case of the good old letting perfection get in the
> way of progress. We can be pretty sure that whatever we decide to do (now
> or later) will not be perfect. But we can be equally sure that it will be
> better than what we have today in most cases. But that's just me...

I'm with Robert.  Such a discussion is morally indistinguishable from a
new feature, and so we shouldn't consider it at this point in the cycle
--- even if it seemed likely that we could easily get to a consensus,
which I doubt.  If we'd dealt with the issue back in February when the
previous discussion happened, that'd be fine, but now's not the time
to decide it has to be resolved for 9.6.

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] Updated backup APIs for non-exclusive backups

[Stephen Frost]

* Magnus Hagander (mag...@hagander.net) wrote:
> On Sun, Apr 24, 2016 at 5:37 AM, Bruce Momjian  wrote:
> 
> > On Fri, Apr 22, 2016 at 11:53:46AM -0400, Robert Haas wrote:
> > > On Wed, Apr 20, 2016 at 1:32 PM, Magnus Hagander 
> > wrote:
> > > > Note that we have not marked them as deprecated. We're just giving
> > warnings
> > > > that they will be deprecated.
> > >
> > > But I think that is being said here is that maybe they won't be
> > > deprecated, at least not any time soon.  And therefore maybe we
> > > shouldn't say so.
> > >
> > > Frankly, I think that's right.  It is one thing to say that the new
> > > method is preferred - +1 for that.  But the old method is going to
> > > continue to be used by many people for a long time, and in some cases
> > > will be superior.  That's not something we can deprecate, unless I'm
> > > misunderstanding the situation.
> >
> > I agree with Robert.  One the one hand we are saying pg_stop_backup()
> > doesn't work well in psql because you get those two file contents output
> > that you have to write, and on the other hand we are saying we are going
> > to deprecate the method that does work well in psql?  I must be missing
> > something too, as that makes no sense.
> 
> I don't agree. I don't see how "making a backup using psql" is more
> important than "making a backup without potentially dangerous sideeffects".
> But if others don't agree, could one of you at least provide an example of
> how you'd like the docs to read in a way that doesn't deprecate the unsafe
> way but still informs the user properly?

I'm with Magnus on this, primairly because I've come to understand just
how dangerous the old backup method is.  That method *should* be
deprecated and discouraged.  A backup method which means your database
doesn't restart properly if the system crashes during the backup is
*bad*.

Fixing that means using something more complicated than the old method
and that's a bit of a pain in psql, but that doesn't mean we should tell
people that the old method is an acceptable approach.

Perhaps we can look at improving psql to make it easier to use it for
the new backup method but, honestly, all these hackish scripts to do
backups aren't doing a lot of things that a real backup solution needs
to do.  Improving psql for this is a bit like new paint on the titanic.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] Defaults for replication/backup

[Magnus Hagander]

On Thu, Apr 21, 2016 at 7:20 PM, Robert Haas  wrote:

> On Wed, Apr 20, 2016 at 2:04 PM, Magnus Hagander 
> wrote:
> > So what more do we need to just get going with this? Given feature freeze
> > we're perhaps too late to actually build the parameter feature for
> initdb,
> > but we could still change the defaults (and then we could add such a
> > parameter for next release).
>
> I think we are far too close to beta1 to begin bikeshedding this.
> Changing the defaults is not going to be uncontroversial, and it's not
> something I think we should rush into.
>
>
I think that may be a case of the good old letting perfection get in the
way of progress. We can be pretty sure that whatever we decide to do (now
or later) will not be perfect. But we can be equally sure that it will be
better than what we have today in most cases. But that's just me...

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

Re: [HACKERS] Updated backup APIs for non-exclusive backups

[Magnus Hagander]

On Sun, Apr 24, 2016 at 5:37 AM, Bruce Momjian  wrote:

> On Fri, Apr 22, 2016 at 11:53:46AM -0400, Robert Haas wrote:
> > On Wed, Apr 20, 2016 at 1:32 PM, Magnus Hagander 
> wrote:
> > > Note that we have not marked them as deprecated. We're just giving
> warnings
> > > that they will be deprecated.
> >
> > But I think that is being said here is that maybe they won't be
> > deprecated, at least not any time soon.  And therefore maybe we
> > shouldn't say so.
> >
> > Frankly, I think that's right.  It is one thing to say that the new
> > method is preferred - +1 for that.  But the old method is going to
> > continue to be used by many people for a long time, and in some cases
> > will be superior.  That's not something we can deprecate, unless I'm
> > misunderstanding the situation.
>
> I agree with Robert.  One the one hand we are saying pg_stop_backup()
> doesn't work well in psql because you get those two file contents output
> that you have to write, and on the other hand we are saying we are going
> to deprecate the method that does work well in psql?  I must be missing
> something too, as that makes no sense.
>

I don't agree. I don't see how "making a backup using psql" is more
important than "making a backup without potentially dangerous sideeffects".
But if others don't agree, could one of you at least provide an example of
how you'd like the docs to read in a way that doesn't deprecate the unsafe
way but still informs the user properly?

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

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Andrew Dunstan]

On 04/23/2016 11:25 PM, Andrew Dunstan wrote:



On 04/23/2016 06:33 PM, Tom Lane wrote:

I wrote:

Andrew Dunstan  writes:

On 04/23/2016 05:30 PM, Christian Ullrich wrote:

In this case, I would prefer this:

#ifdef WIN32_ONLY_COMPILER
-typedef int pid_t;
+typedef intptr_t pid_t;
#endif

That's a change that will have a pretty wide effect. Everything up to
now has been pretty low risk, but this worries me rather more. Maybe
it's safe, but I'd like to hear others' comments.

Yeah, it makes me a bit nervous too.

One other thought: even if this is safe for HEAD, I think we could
*not* back-patch it into 9.5, because it would amount to an ABI
break on Windows anywhere that pid_t is used in globally visible
structs or function signatures.  (Maybe there are no such places,
but I doubt it.)  So we'd need to go with the messy-cast solution
for 9.5.




It's not that messy. I'm inclined just to make minimal changed to 
pg_basebackup.c and be done with it. I don't think a compiler warning 
is worth doing more for.






OK, here's my final version of the patch, which I will apply in 24 hours 
or so unless there is an objection.


cheers

andrew



VS2015-support.patch
Description: binary/octet-stream

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

Re: [HACKERS] Proposed change to make cancellations safe

[Tom Lane]

Shay Rojansky  writes:
> The issue I'd like to tackle is the fact that it's not possible to make
> sure a cancellation request affects a specific query.

Right ...

> A simple fix for this would be to have a sequence number returned in the
> BindComplete message.

First, that doesn't fix anything for simple query protocol, which doesn't
use such a message.

Second, making sure that the BindComplete gets delivered soon enough to be
useful would require issuing a Sync between Bind and Execute.  At the
least that doubles the number of network packets for small query results
(so I dispute your assumption that the added network load would be
negligible).  But a bigger issue is that it would have subtle effects
on error handling.  An application could not blindly fire out
Parse/Bind/Sync/Execute/Sync as one packet; it would have to wait for
the Bind result to come back before it could know if it's safe to send
Execute, unlike the case where it sends Parse/Bind/Execute/Sync.  (The
server's skip-to-Sync-after-error rule is critical here.)  So actually,
making use of this feature would double the number of network round trips,
as well as requiring carefully thought-through changes to application
error handling.

Third, if a query is not cancellable till Execute, that prevents
cancellation of scenarios where the parser or planner takes a long time
for some reason.  Long planner runtimes are certainly not uncommon.


So this is worth thinking about, but that doesn't sound like much of
a solution to me.

I wonder though why we need the server to tell the client a sequence
number at all.  Surely both sides of the connection could easily count
the number of messages the client has transmitted.  We could simply extend
the cancel packet to include a sequence number field, with the semantics
"this should only take effect if you're still working on message N".
Or I guess maybe it should read "working on message N or before", so
that in a case like Parse/Bind/Execute/Sync you would mention the number
of the Execute message and still get cancellation if the query was hung
up in Parse.

Have you seen this to be a problem in practice, or is it just
theoretical?  I do not recall many, if any, field complaints
about the issue.

regards, tom lane


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

[HACKERS] Proposed change to make cancellations safe

[Shay Rojansky]

Hi.

A while ago I discussed some reliability issues when using cancellations (
http://www.postgresql.org/message-id/CADT4RqAk0E10=9ba8v+uu0dq9tr+pn8x+ptqbxfc1fbivh3...@mail.gmail.com).
Since we were discussing some protocol wire changes recently I'd like to
propose one to help with that.

The issue I'd like to tackle is the fact that it's not possible to make
sure a cancellation request affects a specific query. Since cancellations
are totally asynchronous and occur on another socket, they affect whatever
query is currently being processed. This makes it possible to inadvertently
kill a later query, because by the time the cancellation request is
delivered and "applied" an the intended query already completed and a later
one gets killed.

A simple fix for this would be to have a sequence number returned in the
BindComplete message. When submitting a cancellation request, the frontend
would have the option (but not the obligation) to pass such as sequence
number to the backend. When cancelling, the backend would make sure the
sequence number corresponds to the currently running query.

The only drawback seems to be the increased payload of the BindComplete
message (4 bytes, or possibly less if we're really worried about it).

If this makes sense, I'll add it to the TODO wiki.

Shay

Re: [HACKERS] VS 2015 support in src/tools/msvc

[Petr Jelinek]

On 23/04/16 08:08, Christian Ullrich wrote:

* Christian Ullrich wrote:


* Andrew Dunstan wrote:


On 04/22/2016 02:46 AM, Michael Paquier wrote:



Progress report:

1. My VS 2015 installations (I now have several) all generate
solution file
with:
Microsoft Visual Studio Solution File, Format Version 12.00
so I propose to set that as the solution file version.


I am wondering why it happens this way for you..


It's not just me. See the reply at



and notice that in both cases the Solution file format is version 12.00.


Try as I may, I cannot get my VS 2015 to write a version 14 .sln file.
It's always "12.00". If I change it to 14 by hand, it still opens and
appears to work fine.

I tried to find a real-world version 14 solution to see if I can spot a
difference between it and the version 12 files, but there appears to be
very little out there, and what there is, looks like it was either
autogenerated or edited manually. Examples:


OK, so searching on GitHub yielded a few more results, but I could not
identify a single one that was clearly not created or edited by anything
other than Visual Studio itself.



I did some checking too, and yes it seems having version 12 .sln is fine 
or maybe even desirable. We mainly need to make sure that the tools 
version is 14 and platform toolset is v140 for MSVC15.


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


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

Re: [HACKERS] [COMMITTERS] pgsql: doc: Fix typos

[Fabien COELHO]

Hello Peter,


http://git.postgresql.org/pg/commitdiff/b87b2f4bda1a3b98f8dea867b8bc419ace7a9ea9
doc/src/sgml/ref/pgbench.sgml | 8 


 -   Here is example outputs:
 +   Here is example output:

I think that something is missing now on this line. Should it rather be 
"Here is an example output", if singular is better? It looks awkward to me 
without some article.


--
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] Rename max_parallel_degree?

[Peter Geoghegan]

On Sat, Apr 23, 2016 at 8:58 PM, Bruce Momjian  wrote:
> Why is the parallelism variable called "max_parallel_degree"?  Is that a
> descriptive name?  What does "degree" mean?  Why is it not called
> "max_parallel_workers"?

I also think that "max_parallel_workers" works better.

While certain other systems have a concept of a "maximum parallel
degree", it is not the same. Those other systems disable parallelism
altogether when "max parallel degree" is 1, whereas Postgres uses 1
parallel worker along with a leader process. And so, parallelism
*will* still be used on Postgres. That's a pretty significant
difference.

-- 
Peter Geoghegan


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