Re: [HACKERS] LDAP URI decoding bugs

2017-11-10 Thread Peter Eisentraut
On 11/6/17 23:30, Michael Paquier wrote:
> On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro
> <thomas.mu...@enterprisedb.com> wrote:
>> 1.  If you set up a pg_hba.conf with a URL that lacks a base DN or
>> hostname, hba.c will segfault on startup when it tries to pstrdup a
>> null pointer.  Examples: ldapurl="ldap://localhost; and
>> ldapurl="ldap://;.
>>
>> 2.  If we fail to bind but have no binddn configured, we'll pass NULL
>> to ereport (snprint?) for %s, which segfaults on some libc
>> implementations.  That crash requires more effort to reproduce but you
>> can see pretty clearly a few lines above in auth.c that it can be
>> NULL.  (I'm surprised Coverity didn't complain about that.  Maybe it
>> can't see this code due to macros.)

committed and backpatched

-- 
Peter Eisentraut  http://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] Add some const decorations to prototypes

2017-11-10 Thread Peter Eisentraut
On 11/10/17 10:29, Fabien COELHO wrote:
> Or basically all is fine, I'm just nitpicking for nothing, shame on me.
> 
> As I said, I rather like more precise declarations.

committed

-- 
Peter Eisentraut  http://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] Add some const decorations to prototypes

2017-11-10 Thread Peter Eisentraut
On 11/10/17 11:42, Fabien COELHO wrote:
> After your explanation, and on third thoughts, ISTM that the assignment 
> should not include "const" in the explicit cast, i.e., use
> 
>extern void * msg_func(void);
>const char * msg = (char *) msg_func();
> 
> The variable or field is constant, not what the function returns, so
> 
>const char * msg = (const char *) msg_func();
> 
> does not really make full sense to me, and moreover the compiler does not 
> complain without the const.

The compiler knows how to handle the char * -> const char * case, but
not the char ** -> const char ** case.

-- 
Peter Eisentraut  http://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] Add some const decorations to prototypes

2017-11-10 Thread Peter Eisentraut
On 11/4/17 16:50, Fabien COELHO wrote:
>>> Just leave it as char*.  If you change the endptr argument you're going to
>>> force every call site to change their return variable, and some of them
>>> would end up having to cast away the const on their end.
>>
>> OK, here is an updated patch with the controversial bits removed.
> 
> I'm in general favor in helping compilers, but if you have to cheat.
> 
> ISTM That there is still at least one strange cast:
> 
>   +static const char **LWLockTrancheArray = NULL;
>   +   LWLockTrancheArray = (const char **) // twice

These are not cases of "cheating".  This is just the return value of a
memory allocation function being cast from void * to the appropriate
result type.  That is an orthogonal style decision that I have
maintained in these cases.

-- 
Peter Eisentraut  http://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] SQL procedures

2017-11-08 Thread Peter Eisentraut
On 11/8/17 11:11, Merlin Moncure wrote:
> On Wed, Nov 8, 2017 at 9:13 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> I have already submitted a separate patch that addresses these questions.
> 
> Maybe I'm obtuse, but I'm not seeing it? In very interested in the
> general approach to transaction management; if you've described it in
> the patch I'll read it there.  Thanks for doing this.

https://www.postgresql.org/message-id/178d3380-0fae-2982-00d6-c43100bc8...@2ndquadrant.com

-- 
Peter Eisentraut  http://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] taking stdbool.h into use

2017-11-08 Thread Peter Eisentraut
On 10/29/17 08:50, Michael Paquier wrote:
> I had a look at this patch series. Patches 1, 2 (macos headers indeed
> show that NSUNLINKMODULE_OPTION_NONE is set to 0x0), 3 to 7 look fine
> to me.

Committed 4 and 5 together.

> I spotted a couple of other things while looking at your patches and
> the code tree.
> 
> -   return (ginCompareItemPointers(>itemptr, iptr) > 0) ? TRUE : FALSE;
> +   return (ginCompareItemPointers(>itemptr, iptr) > 0) ? true : false;
> You could simplify that at the same time by removing such things. The
> "false : true" pattern is less frequent than the "true : false"
> pattern.

I have found many more instances like that.  It might be worth
simplifying a bit, but that sounds like a separate undertaking.

> Some comments in the code still mention FALSE or TRUE:
> - hashsearch.c uses FALSE in some comments.
> - In execExpr.c, ExecCheck mentions TRUE.

That one is an SQL TRUE, so I left it.

> - AggStateIsShared mentions TRUE and FALSE.
> - In arrayfuncs.c, ReadArrayStr, CopyArrayEls and ReadArrayBinary use TRUE.

Fixed the other ones.

-- 
Peter Eisentraut  http://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] Exclude pg_internal.init from base backup

2017-11-08 Thread Peter Eisentraut
On 11/7/17 19:58, Michael Paquier wrote:
> On Wed, Nov 8, 2017 at 9:50 AM, Haribabu Kommi <kommi.harib...@gmail.com> 
> wrote:
>> Thanks for the correction. I was not much aware of SGML markup usage.
>> While building the documentation, it raises an warning message of "empty
>> end-tag".
>> So I just added the end tag. Attached the update patch with the suggested
>> correction.
> 
> Ah, I can see the warning as well. Using empty tags is forbidden since
> c29c5789, which is really recent. Sorry for missing it. Simon got
> trapped by that as well visibly. Your patch looks good to me.

fixed

-- 
Peter Eisentraut  http://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] Horrible CREATE DATABASE Performance in High Sierra

2017-11-08 Thread Peter Eisentraut
On 10/7/17 16:46, Tom Lane wrote:
> I wrote:
>> Current status is that I've filed a bug report with Apple and am waiting
>> to see their response before deciding what to do next.  If they fix the
>> issue promptly then there's little need for us to do anything.

> Accordingly I propose the attached patch.  If anyone's interested in
> experimenting on other platforms, we might be able to refine/complicate
> the FLUSH_DISTANCE selection further, but I think this is probably good
> enough for a first cut.

What is the status of this?  Is performance on High Sierra still bad?

-- 
Peter Eisentraut  http://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] SQL procedures

2017-11-08 Thread Peter Eisentraut
On 11/8/17 09:23, Merlin Moncure wrote:
> I do wonder how transaction control could be added later.
> 
> The last time I (lightly) looked at this, I was starting to think that
> working transaction control into the SPI interface was the wrong
> approach; pl/pgsql would have to adopt a very different set of
> behaviors if it was called in a function or a proc.  If you restricted
> language choice to purely SQL, you could work around this problem; SPI
> languages would be totally abstracted from those sets of
> considerations and you could always call an arbitrary language
> function if you needed to.  SQL has no flow control but I'm not too
> concerned about that.

I have already submitted a separate patch that addresses these questions.

-- 
Peter Eisentraut  http://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] SQL procedures

2017-11-08 Thread Peter Eisentraut
On 11/8/17 09:33, Pavel Stehule wrote:
> We can create auto session variable STATUS. This variable can be 0
> if procedure was returned without explicit RETURN value. Or it can
> hold different value specified by RETURN expr.
> 
> This value can be read by GET DIAGNOSTICS xxx = STATUS
> 
> or some similar.
> 
> The motivation is allow some mechanism cheaper than our exceptions.

I suppose this could be a separately discussed feature.  We'd also want
to consider various things that PL/pgSQL pretends to be compatible with.

One of the main motivations for procedures is to do more complex and
expensive things including transaction control.  So saving exception
overhead is not really on the priority list there.

-- 
Peter Eisentraut  http://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] SQL procedures

2017-11-08 Thread Peter Eisentraut
On 11/6/17 16:27, Simon Riggs wrote:
> You mention PARALLEL SAFE is not used for procedures. Isn't it an
> architectural restriction that procedures would not be able to execute
> in parallel? (At least this year)

I'm not sure what you are referring to here.  I don't think the
functionality I'm proposing does anything in parallel or has any
interaction with it.

> I think we need an explanatory section of the docs, but there doesn't
> seem to be one for Functions, so there is no place to add some text
> that says the above.
> 
> I found it confusing that ALTER and DROP ROUTINE exists but not CREATE
> ROUTINE. At very least we should say somewhere "there is no CREATE
> ROUTINE", so its absence is clearly intentional. I did wonder whether
> we should have it as well, but its just one less thing to review, so
> good.

I'll look for a place to add some documentation around this.

> Was surprised that pg_dump didn't use DROP ROUTINE, when appropriate.

It's not clear to me why that would be preferred.

-- 
Peter Eisentraut  http://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] SQL procedures

2017-11-08 Thread Peter Eisentraut
On 10/31/17 16:50, Pavel Stehule wrote:
> Not sure if disabling RETURN is good idea. I can imagine so optional
> returning something like int status can be good idea. Cheaper than
> raising a exception.

We could allow a RETURN without argument in PL/pgSQL, if you just want
to exit early.  That syntax is currently not available, but it should
not be hard to add.

I don't understand the point about wanting to return an int.  How would
you pass that around, since there is no declared return type?

-- 
Peter Eisentraut  http://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] SQL procedures

2017-11-08 Thread Peter Eisentraut
On 10/31/17 14:23, Tom Lane wrote:
> Putting 0 in prorettype seems like a pretty bad idea.

It seemed like the natural thing to do, since we use a zero OID to
indicate "nothing" in many other places.

> Why not use VOIDOID for the prorettype value?

We need a way to distinguish functions that are callable by SELECT and
procedures that are callable by CALL.

> Or if there is some reason why "void" isn't the
> right pseudotype, maybe you should invent a new one, analogous to the
> "trigger" and "event_trigger" pseudotypes.

I guess that would be doable, but I think it would make things more
complicated without any gain that I can see.  In the case of the
pseudotypes you mention, those are the actual types mentioned in the
CREATE FUNCTION command.  If we invented a new pseudotype, that would
run the risk of existing code creating nonsensical reverse compilations
like CREATE FUNCTION RETURNS PROCEDURE.  Catalog queries using
prorettype == 0 would behave sensibly by default.  For example, an inner
or outer join against pg_type would automatically make sense.

-- 
Peter Eisentraut  http://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] taking stdbool.h into use

2017-11-04 Thread Peter Eisentraut
On 10/29/17 08:50, Michael Paquier wrote:
> On Thu, Oct 26, 2017 at 9:25 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> Here is an updated patch set.  This is just a rebase of the previous
>> set, no substantial changes.  Based on the discussion so far, I'm
>> proposing that 0001 through 0007 could be ready to commit after review,
>> whereas the remaining two need more work at some later time.
> 
> I had a look at this patch series. Patches 1, 2 (macos headers indeed
> show that NSUNLINKMODULE_OPTION_NONE is set to 0x0), 3 to 7 look fine
> to me.

Committed 1, 2, 3; will work on the rest later and incorporate your
findings.

-- 
Peter Eisentraut  http://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] Add some const decorations to prototypes

2017-11-04 Thread Peter Eisentraut
On 11/3/17 13:54, Tom Lane wrote:
>> Would you prefer leaving the input argument as char *, or change the
>> endptr argument to const as well?
> 
> Just leave it as char*.  If you change the endptr argument you're going to
> force every call site to change their return variable, and some of them
> would end up having to cast away the const on their end.

OK, here is an updated patch with the controversial bits removed.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From b0fe8bb86a37938dad6bd6d6b7a51ded6afbf78a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Tue, 31 Oct 2017 10:34:31 -0400
Subject: [PATCH v2] Add some const decorations to prototypes

---
 contrib/dict_xsyn/dict_xsyn.c  |  2 +-
 contrib/fuzzystrmatch/dmetaphone.c |  4 +--
 contrib/pgcrypto/pgcrypto.c|  4 +--
 contrib/seg/seg.c  |  4 +--
 contrib/seg/segdata.h  |  2 +-
 contrib/seg/segparse.y |  4 +--
 contrib/unaccent/unaccent.c|  2 +-
 contrib/uuid-ossp/uuid-ossp.c  |  2 +-
 src/backend/access/common/reloptions.c | 12 
 src/backend/access/gist/gistbuild.c|  2 +-
 src/backend/access/transam/xact.c  |  6 ++--
 src/backend/access/transam/xlogarchive.c   |  4 +--
 src/backend/catalog/heap.c | 10 +++
 src/backend/commands/comment.c |  4 +--
 src/backend/commands/event_trigger.c   |  4 +--
 src/backend/commands/extension.c   |  4 +--
 src/backend/commands/indexcmds.c   |  8 +++---
 src/backend/commands/opclasscmds.c |  2 +-
 src/backend/commands/tablecmds.c   | 16 +--
 src/backend/commands/typecmds.c|  6 ++--
 src/backend/commands/view.c|  2 +-
 src/backend/libpq/auth.c   | 24 
 src/backend/libpq/hba.c|  6 ++--
 src/backend/parser/parse_expr.c|  2 +-
 src/backend/parser/parse_func.c|  4 +--
 src/backend/parser/parse_relation.c|  8 +++---
 src/backend/parser/parse_target.c  |  2 +-
 src/backend/port/dynloader/darwin.c|  8 +++---
 src/backend/port/dynloader/darwin.h|  4 +--
 src/backend/port/dynloader/hpux.c  |  4 +--
 src/backend/port/dynloader/hpux.h  |  4 +--
 src/backend/port/dynloader/linux.c |  4 +--
 src/backend/postmaster/postmaster.c|  2 +-
 src/backend/replication/basebackup.c   |  8 +++---
 src/backend/rewrite/rewriteDefine.c|  4 +--
 src/backend/snowball/dict_snowball.c   |  2 +-
 src/backend/storage/lmgr/lwlock.c  |  8 +++---
 src/backend/tsearch/dict_thesaurus.c   |  2 +-
 src/backend/tsearch/spell.c|  4 +--
 src/backend/utils/adt/genfile.c|  2 +-
 src/backend/utils/adt/ruleutils.c  |  4 +--
 src/backend/utils/adt/varlena.c|  2 +-
 src/backend/utils/adt/xml.c| 32 +++---
 src/bin/initdb/initdb.c| 12 
 src/bin/pg_dump/pg_backup_db.c |  2 +-
 src/bin/pg_dump/pg_backup_db.h |  2 +-
 src/bin/pg_rewind/fetch.c  |  2 +-
 src/bin/pg_rewind/fetch.h  |  2 +-
 src/bin/pg_upgrade/option.c|  6 ++--
 src/bin/pg_upgrade/pg_upgrade.c|  4 +--
 src/bin/pg_waldump/pg_waldump.c|  2 +-
 src/bin/pgbench/pgbench.c  |  4 +--
 src/include/access/gist_private.h  |  2 +-
 src/include/access/reloptions.h| 14 +-
 src/include/access/xact.h  |  6 ++--
 src/include/access/xlog_internal.h |  4 +--
 src/include/catalog/heap.h |  2 +-
 src/include/commands/comment.h |  4 +--
 src/include/commands/defrem.h  |  4 +--
 src/include/commands/typecmds.h|  2 +-
 src/include/commands/view.h|  2 +-
 src/include/executor/tablefunc.h   |  8 +++---
 src/include/parser/parse_relation.h|  6 ++--
 src/include/parser/parse_target.h  |  2 +-
 src/include/postmaster/bgworker.h  |  2 +-
 src/include/rewrite/rewriteDefine.h|  2 +-
 src/include/storage/lwlock.h   |  2 +-
 src/include/utils/dynamic_loader.h  

Re: [HACKERS] Dynamic result sets from procedures

2017-11-03 Thread Peter Eisentraut
On 11/2/17 16:40, Daniel Verite wrote:
> But instead of having procedures not return anything,
> couldn't they return whatever resultset(s) they want to
> ("no resultset" being just a particular case of "anything"),
> so that we could leave out cursors and simply write:

We could in general design this any way we want.  I'm just going by
what's in the SQL standard and in existing implementations.

> CREATE PROCEDURE test()
> LANGUAGE plpgsql
> AS $$
>   RETURN QUERYEXECUTE 'SELECT 1 AS col1, 2 AS col2';
> END;
> $$;
> 
> Or is that not possible or not desirable?

RETURN means the execution ends there, so how would you return multiple
result sets?

> Similarly, for the SQL language, I wonder if the above example
> could be simplified to:
> 
> CREATE PROCEDURE pdrstest1()
> LANGUAGE SQL
> AS $$
>  SELECT * FROM cp_test2;
>  SELECT * FROM cp_test3;
> $$;
> by which the two result sets would go back to the client again
> without declaring explicit cursors.
> Currently, it does not error out and no result set is sent.

But maybe you don't want to return all those results, so you'd need a
way to designate which ones, e.g.,

AS $$
SELECT set_config('something', 'value');
SELECT * FROM interesting_table;  -- return only this one
SELECT set_config('something', 'oldvalue');
$$;

-- 
Peter Eisentraut  http://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] Dynamic result sets from procedures

2017-11-03 Thread Peter Eisentraut
On 11/1/17 22:40, Robert Haas wrote:
> That seems like it is at least arguably a wire protocol break.  Today,
> if you send a string containing only one command, you will only get
> one answer.

The wire protocol already supports this.  And the wire protocol doesn't
really know about statements, only about a command string that might
contain multiple commands.  So I don't think anything would break.

-- 
Peter Eisentraut  http://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] Dynamic result sets from procedures

2017-11-03 Thread Peter Eisentraut
On 11/1/17 06:23, Pavel Stehule wrote:
> We need to think about how the \timing option should work in such
> scenarios.  Right now it does

> Has the total time sense  in this case?
> 
> should not be total time related to any fetched result?

The \timing option in psql measures from the time you send off the
statement until you get all the results back.  I don't see a fundamental
reason to change that if a statement happens to produce multiple
results.  The results already come over the write and are processed by
libpq.  So the time is already measured.  It's just that psql doesn't
print the non-last result sets.

We don't have any way to measure from psql how long each individual
result set took to compose.  For that you will need deeper tools like
EXPLAIN ANALYZE and some way to process that data.  That is way beyond
what \timing currently does.

-- 
Peter Eisentraut  http://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] Linking libpq statically to libssl

2017-11-03 Thread Peter Eisentraut
On 11/2/17 19:52, Stephen Frost wrote:
> Uh, libpq doesn't actually have symbol versioning, at least not on the
> installed Ubuntu packages for PG10, as shown by objdump -T :

You're right, I was dreaming.  But in any case, he would need symbol
versioning of libssl.

-- 
Peter Eisentraut  http://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] [PATCH] Add ALWAYS DEFERRED option for constraints

2017-11-03 Thread Peter Eisentraut
On 11/2/17 16:54, Nico Williams wrote:
> Replacing condeferred and condeferrable with a char columns also
> occurred to me, and though I assume backwards-incompatible changes to
> pg_catalog tables are fair game, I assumed everyone would prefer
> avoiding such changes where possible.

I don't think there is an overriding mandate to avoid such catalog
changes.  Consider old clients that don't know about your new column.
They might look at the catalog entries and derive information about a
constraint, not being aware that there is additional information in
another column that overrides that.  So in such cases it's arguably
better to make a break.

(In any case, it might be worth waiting for a review of the rest of the
patch before taking on a significant rewrite of the catalog structures.)

> Hmmm, must I do anything special about _downgrades_?  Does PostgreSQL
> support downgrades?

no

-- 
Peter Eisentraut  http://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] Add some const decorations to prototypes

2017-11-03 Thread Peter Eisentraut
On 10/31/17 10:56, Tom Lane wrote:
>> Some functions have a strtol()-like behavior
>> where they take in a const char * and return a pointer into that as
>> another argument.  In those cases, I added a cast or two.
> ... but I'm not sure that it's an improvement in cases where you have to
> cast away the const somewhere else.  I realize that strtol has an ancient
> pedigree, but I do not think it's very good design.

Would you prefer leaving the input argument as char *, or change the
endptr argument to const as well?

-- 
Peter Eisentraut  http://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] Subscriber resets additional columns to NULL on UPDATE

2017-11-03 Thread Peter Eisentraut
On 10/26/17 05:20, Petr Jelinek wrote:
> I found bug in logical replication where extra (nullable) columns on
> subscriber will be reset to NULL value when update comes from provider.
> 
> The issue is apparently that we /points finger at himself/ forgot to
> check specifically for columns that are not part of attribute map in
> slot_modify_cstrings() so the extra columns will fall through to the
> else block which sets the value to NULL.
> 
> Attached patch fixes it and adds couple of tests for this scenario.
> 
> This is rather serious issue so it would be good if we could get it
> fixed in 10.1.

done

-- 
Peter Eisentraut  http://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] Skip unneeded temp file in 'make html'

2017-11-03 Thread Peter Eisentraut
On 11/2/17 22:07, David Fetter wrote:
>  postgres.xml: $(srcdir)/postgres.sgml $(ALLSGML)
> - $(OSX) $(SPFLAGS) $(SGMLINCLUDE) -x lower $< >$@.tmp
> - $(call mangle-xml,book)
> + $(OSX) $(SPFLAGS) $(SGMLINCLUDE) -x lower $< | $(call mangle-xml,book)

The reason why it's not done that way is that this would not catch
errors of the command before the pipe.

-- 
Peter Eisentraut  http://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] Deadlock in ALTER SUBSCRIPTION REFRESH PUBLICATION

2017-11-02 Thread Peter Eisentraut
On 10/24/17 13:13, Konstantin Knizhnik wrote:
> The reason of this deadlock seems to be clear: ALTER SUBSCRIPTION starts 
> transaction at one node and tries to create slot at other node, which waiting 
> for completion of all active transaction while building scnapshpot.
> Is there any way to avoid this deadlock?

I don't see a way to avoid it in general, unless we come up with a novel
way of creating replication slots.

-- 
Peter Eisentraut  http://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] Linking libpq statically to libssl

2017-11-02 Thread Peter Eisentraut
On 10/27/17 08:24, Daniele Varrazzo wrote:
> I have a problem building binary packages for psycopg2. Binary
> packages ship with their own copies of libpq and libssl;

Aside from the advice of "don't do that" ...

> however if
> another python package links to libssl the library will be imported
> twice with conflicting symbols, likely resulting in a segfault (see
> https://github.com/psycopg/psycopg2/issues/543). This happens e.g. if
> a python script both connects to postgres and opens an https resource.

... the standard solutions to this problem are symbol versioning and
linker flags to avoid making all symbols globally available.  libpq has
symbol versioning.  Maybe the libssl you are using does not.  Also, for
example, if you dlopen() with RTLD_LOCAL, the symbols will not be
globally available, so there should be no conflicts.

This needs cooperation from various different parties, and the details
will likely be platform dependent.  But it's generally a solved problem.

-- 
Peter Eisentraut  http://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] [PATCH] Add ALWAYS DEFERRED option for constraints

2017-11-02 Thread Peter Eisentraut
I haven't really thought about this feature too hard; I just want to
give you a couple of code comments.

I think the catalog structure, and relatedly also the parser structures,
could be made more compact.  We currently have condeferrable and
condeferred to represent three valid states (NOT DEFERRABLE, DEFERRABLE
INITIALLY IMMEDIATE, DEFERRABLE INITIALLY DEFERRED).  You are adding
conalwaysdeferred, but you are adding only additional state (ALWAYS
DEFERRED).  So we end up with three bool fields to represent four
states.  I think this should all be rolled into one char field with four
states.

In psql and pg_dump, if you are query new catalog fields, you need to
have a version check to have a different query for >=PG11.  (This would
likely apply whether you adopt my suggestion above or not.)

Maybe a test case in pg_dump would be useful.

Other than that, this looks like a pretty complete patch.

-- 
Peter Eisentraut  http://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] initdb w/ restart

2017-11-02 Thread Peter Eisentraut
On 9/28/17 15:40, Jesper Pedersen wrote:
> In a case of
> 
> initdb /tmp/pgsql
> 
> followed by
> 
> pg_ctl -D /tmp/pgsql/ -l /tmp/logfile restart
> 
> you'll get
> 
> pg_ctl: PID file "/tmp/pgsql/postmaster.pid" does not exist
> Is server running?
> starting server anyway
> pg_ctl: could not read file "/tmp/pgsql/postmaster.opts"
> 
> The attached patch changes the message to "trying to start server 
> anyway" to highlight it is an attempt, not something that will happen.

committed

-- 
Peter Eisentraut  http://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] [PATCH] Minor patch to correct symbol name in logs

2017-11-02 Thread Peter Eisentraut
On 9/20/17 01:56, Vaishnavi Prabakaran wrote:
> Backend's lo_functions were renamed to avoid conflicting with libpq
> prototypes in commit - 6fc547960dbe0b8bd6cefae5ab7ec3605a5c46fc. 
> 
> Logs inside those functions still refer to old symbol names, Here is the
> small patch to update the same.

I think those debug messages refer to the SQL name of the function, so
they look correct to me as is.

-- 
Peter Eisentraut  http://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] [COMMITTERS] pgsql: passwordcheck: Add test suite

2017-11-02 Thread Peter Eisentraut
On 9/15/17 00:20, Michael Paquier wrote:
> On Fri, Sep 15, 2017 at 12:02 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> On Fri, Sep 15, 2017 at 11:46 AM, Peter Eisentraut <pete...@gmx.net> wrote:
>>> passwordcheck: Add test suite
>>>
>>> Also improve one error message.
>>>
>>> Reviewed-by: David Steele <da...@pgmasters.net>
>>
>> Sorry for showing up late for this topic.
>> +REGRESS_OPTS = --temp-config $(srcdir)/passwordcheck.conf
>> +REGRESS = passwordcheck
>> +# disabled because these tests require setting shared_preload_libraries
>> +NO_INSTALLCHECK = 1
>> You could have avoided all that by just issuing "load
>> 'passwordcheck';" at the beginning of the test. And you gain support
>> for installcheck this way.
> 
> In short you just need the attached ;)

committed

-- 
Peter Eisentraut  http://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] Removing wal_keep_segments as default configuration in PostgresNode.pm

2017-11-02 Thread Peter Eisentraut
On 9/11/17 21:55, Michael Paquier wrote:
> I tend to think that while all the other parameters make sense to
> deploy instances that need few resources, wal_keep_segments may cause
> up to 350MB of WAL segments to be kept in each pg_wal's instance,
> while max_wal_size is set at 128MB. The only test in the code tree in
> need of wal_keep_segments is actually pg_rewind, which enforces
> checkpoints after the rewind to update the source's control file.
> 
> So, thoughts about the attached that reworks this portion of PostgresNode.pm?

Committed.

Besides the resource usage, it would probably be bad if a
wal_keep_segments setting papered over problems with replication slots
for example.

-- 
Peter Eisentraut  http://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] Document pgstattuple privileges without ambiguity

2017-11-02 Thread Peter Eisentraut
On 8/21/17 03:47, Feike Steenbergen wrote:
> When installing pgstattuple on 10, the documentation about its
> privileges was unclear to me. (Does the pg_stat_scan_tables role get
> EXECUTE privileges by default or not?).

I agree that this has gotten a bit confusing after apparently being
patched around a bit recently.  I have rewritten it a bit to make it
clearer.

-- 
Peter Eisentraut  http://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] [PATCH] Add two-arg for of current_setting(NAME, FALLBACK)

2017-11-02 Thread Peter Eisentraut
On 11/1/17 14:02, Nico Williams wrote:
> There already _is_ a two-argument form of current_setting() that yours
> somewhat conflicts with:
> 
>current_setting(setting_name [, missing_ok ])
> 
> https://www.postgresql.org/docs/current/static/functions-admin.html
> 
> I often use
> 
>   coalesce(current_setting(setting_name, true), default_value_here)
> 
> as an implementation of current_setting() with a default value.

That appears to address this use case then.  Do we need the new proposed
variant still?

-- 
Peter Eisentraut  http://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] list of credits for release notes

2017-11-02 Thread Peter Eisentraut
On 10/3/17 12:31, Dang Minh Huong wrote:
> Sorry but please change "Huong Dangminh"(my name) to "Dang Minh Huong".

done

-- 
Peter Eisentraut  http://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] list of credits for release notes

2017-11-01 Thread Peter Eisentraut
On 10/2/17 18:34, Bruce Momjian wrote:
> My smaller question is how will this list be generated in PG 11?  From
> the commit log when the release notes are created, or some other method?

I don't think it should be done at the same time as the release notes.

Specifically, we have recently put an emphasis on having the release
notes ready for beta, and the natural flow of things should be that
there are relatively few changes to the the substance of the release
notes once beta gets rolling.  On the other hand, I have found that a
significant amount of new contributors appear in commit messages during
beta, which also makes some sense.  So making the contributor list
fairly late and then not changing it much is more efficient.

-- 
Peter Eisentraut  http://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] SQL/JSON in PostgreSQL

2017-11-01 Thread Peter Eisentraut
Could someone clarify the status of this patch set?  It has been in
"Waiting" mode since the previous CF and no new patch, just a few
questions from the author.

-- 
Peter Eisentraut  http://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] Enhancements to passwordcheck

2017-11-01 Thread Peter Eisentraut
On 9/25/17 14:04, Bossart, Nathan wrote:
> I'd like to use this thread to gauge community interest in adding this
> functionality to this module.  If there is interest, I'll add it to the next
> commitfest.

This thread has no patch, which is not really the intended use of the
commit fest, so I'm closing the commit fest entry for now.

-- 
Peter Eisentraut  http://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] pg_basebackup fails on Windows when using tablespace mapping

2017-11-01 Thread Peter Eisentraut
On 11/1/17 10:29, Michael Paquier wrote:
> On Wed, Nov 1, 2017 at 2:24 PM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> Committed to master.  I suppose this should be backpatched?
> 
> Thanks! Yes this should be back-patched.

OK, done for 10, 9.6, and 9.5.

The tablespace mapping feature started in 9.4 (has it been that long
already?), but the canonicalization was only added in 9.5.

-- 
Peter Eisentraut  http://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] Statement-level rollback

2017-11-01 Thread Peter Eisentraut
On 10/31/17 13:47, MauMau wrote:
> I'm very sorry I couldn't reply to your kind offer.  I rebased the
> patch and will add it to CF 2017/11.  I hope I will complete the patch
> in this CF.

I've been thinking about this a little bit.  Many are worried about
repeating the mistakes of the autocommit feature, so it's worth
comparing that.

The problem with the autocommit setting, or at least the one I remember,
is that code is currently written expecting that

connect
exec SQL statement
disconnect

will succeed in executing and committing the SQL statement, unless an
error is reported.

If you turned the autocommit setting off, then this code would
effectively silently do nothing, and that is obviously quite bad.  So
the autocommit setting would break a large proportion of all code out
there, and was thus not really usable, and hence it was removed.

The proposed statement-level rollback feature works in a slightly
different context.  It does not change when or how a transaction or
transaction block begins and ends.  It only changes what happens inside
explicit transaction blocks.  Considering code like

START TRANSACTION;
SQL1;
SQL2;
SQL3;
COMMIT;

currently an error would cause all subsequent commands to fail.  Under
statement-level rollback, a failed command would effectively be ignored
and the transaction would continue until COMMIT.

Therefore, a successful transaction block would always work the same way
under either setting.

The difference is how error recovery works.  So this will necessarily be
tied to how the client code or other surrounding code is structured or
what the driver or framework is doing in the background to manage
transactions.  It would also be bad if client code was not prepared for
this new behavior, reported the transaction as complete while some
commands in the middle were omitted.

Drivers can already achieve this behavior and do do that by issuing
savepoint commands internally.  The point raised in this thread was that
that creates too much network overhead, so a backend-based solution
would be preferable.  We haven't seen any numbers or other evidence to
quantify that claim, so maybe it's worth looking into that some more.

In principle, a backend-based solution that drivers just have to opt
into would save a lot of duplication.  But the drivers that care or
require it according to their standards presumably already implement
this behavior in some other way, so it comes back to whether there is a
performance or other efficiency gain here.

Another argument was that other SQL implementations have this behavior.
This appears to be the case.  But as far as I can tell, it is also tied
to their particular interfaces and the structure and flow control they
provide.  So a client-side solution like psql already provides or
something in the various drivers would work just fine here.

So my summary for the moment is that a GUC or similar run-time setting
might be fine, with appropriate explanation and warnings.  But it's not
clear whether it's worth it given the existing alternatives.

-- 
Peter Eisentraut  http://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] Custom compression methods

2017-11-01 Thread Peter Eisentraut
On 9/12/17 10:55, Ildus Kurbangaliev wrote:
>> The patch also includes custom compression method for tsvector which
>> is used in tests.
>>
>> [1]
>> https://www.postgresql.org/message-id/CAPpHfdsdTA5uZeq6MNXL5ZRuNx%2BSig4ykWzWEAfkC6ZKMDy6%3DQ%40mail.gmail.com
> Attached rebased version of the patch. Added support of pg_dump, the
> code was simplified, and a separate cache for compression options was
> added.

I would like to see some more examples of how this would be used, so we
can see how it should all fit together.

So far, it's not clear to me that we need a compression method as a
standalone top-level object.  It would make sense, perhaps, to have a
compression function attached to a type, so a type can provide a
compression function that is suitable for its specific storage.

The proposal here is very general: You can use any of the eligible
compression methods for any attribute.  That seems very complicated to
manage.  Any attribute could be compressed using either a choice of
general compression methods or a type-specific compression method, or
perhaps another type-specific compression method.  That's a lot.  Is
this about packing certain types better, or trying out different
compression algorithms, or about changing the TOAST thresholds, and so on?

Ideally, we would like something that just works, with minimal
configuration and nudging.  Let's see a list of problems to be solved
and then we can discuss what the right set of primitives might be to
address them.

-- 
Peter Eisentraut  http://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] [PATCH] Pageinspect - add functions on GIN and GiST indexes from gevel

2017-11-01 Thread Peter Eisentraut
On 9/13/17 03:45, Alexey Chernyshov wrote:
> On Sat, 9 Sep 2017 13:53:35 +0530
> Ashutosh Sharma <ashu.coe...@gmail.com> wrote:
> 
>>
>> Finally, i got some time to look into this patch and surprisingly I
>> didn't find any function returning information at page level instead
>> all the SQL functions are returning information at index level.
>> Therefore, i too feel that it should be either integrated with
>> pgstattuple which could a better match as Tomas also mentioned or may
>> be add a new contrib module itself. I think, adding a new contrib
>> module looks like a better option. The reason being, it doesn't simply
>> include the function for showing index level statistics (for e.g..
>> index size, no of rows, values..e.t.c) like pgstattuple does but,
>> also, displays information contained in a page for e.g. the object
>> stored in the page,  it's tid e.t.c. So, more or less, I would say
>> that, it's the mixture of pageinspect and pgstattuple module and
>> therefore, i feel, adding it as a new extension would be a better
>> choice. Thought?
> 
> Thank you for your interest, I will add a new contrib module named,
> say, indexstat. I think we can add statistics on other indexes in the
> future.

A few thoughts on this current patch:

The patch no longer compiles, because of changes in the way the tuple
descriptors are accessed (I think).

I agree with the conclusion so far that this should be a new extension,
not being a good fit for an existing extension.

This kind of thing doesn't look like good design:

+
+test=# SELECT spgist_stats('spgist_idx');
+   spgist_stats
+--
+ totalPages:21   +
+ deletedPages:  0+
+ innerPages:3+
+ leafPages: 18   +
+ emptyPages:1+
+ usedSpace: 121.27 kbytes+
+ freeSpace: 46.07 kbytes +
+ fillRatio: 72.47%   +
+ leafTuples:3669 +
+ innerTuples:   20   +
+ innerAllTheSame:   0+
+ leafPlaceholders:  569  +
+ innerPlaceholders: 0+
+ leafRedirects: 0+
+ innerRedirects:0+
+

This function should return a row with columns for each of these pieces
of information.

So to summarize, I think there is interest in this functionality, but
the patch needs some work.

-- 
Peter Eisentraut  http://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] [PATCH] Document the order of changing certain settings when using hot-standby servers

2017-11-01 Thread Peter Eisentraut
On 9/1/17 13:00, Robert Haas wrote:
> Now you're proposing to add:
> 
> If you want to increase these values you
> should do so on all standby servers first, before applying the changes to
> the primary. If you instead want to decrease these values you should do so
> on the primary first, before applying the changes to all standby servers.
> 
> But that's just the obvious logical consequence of the existing statement.
> 
> If we're going to add this text, I'd move it one sentence earlier and
> stick "Therefore, " at the beginning.

Committed incorporating that suggestion.

-- 
Peter Eisentraut  http://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] pg_basebackup fails on Windows when using tablespace mapping

2017-11-01 Thread Peter Eisentraut
On 9/10/17 00:39, Michael Paquier wrote:
>> Okay. I have once again reviewed your patch and tested it on Windows
>> as well as Linux. The patch LGTM. I am now marking it as Ready For
>> Committer. Thanks.
> 
> Thanks for the review, Ashutosh.

Committed to master.  I suppose this should be backpatched?

(I changed the strcpy() to strlcpy() for better karma.)

-- 
Peter Eisentraut  http://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


[HACKERS] Dynamic result sets from procedures

2017-10-31 Thread Peter Eisentraut
This patch is more of a demo of what could be done, not my primary
focus, but if there is interest and some assistance, maybe we can make
something out of it.  This patch also goes on top of "SQL procedures"
version 1.

The purpose is to return multiple result sets from a procedure.  This
is, I think, a common request when coming from MS SQL and DB2.  MS SQL
has a completely different procedure syntax, but this proposal is
compatible with DB2, which as usual was the model for the SQL standard.
So this is what it can do:

CREATE PROCEDURE pdrstest1()
LANGUAGE SQL
AS $$
DECLARE c1 CURSOR WITH RETURN FOR SELECT * FROM cp_test2;
DECLARE c2 CURSOR WITH RETURN FOR SELECT * FROM cp_test3;
$$;

CALL pdrstest1();

and that returns those two result sets to the client.

That's all it does for now.  Things get more complex when you consider
nested calls.  The SQL standard describes additional facilities how an
outer procedure can accept a called procedure's result sets, or not.  In
the thread on transaction control, I mentioned that we might need some
kind of procedure call stack.  Something like that would be needed here
as well.  There are also probably some namespacing issues around the
cursors that need more investigation.

A more mundane issue is how we get psql to print multiple result sets.
I have included here a patch that does that, and you can see that new
result sets start popping up in the regression tests already.  There is
also one need error that needs further investigation.

We need to think about how the \timing option should work in such
scenarios.  Right now it does

start timer
run query
fetch result
stop timer
print result

If we had multiple result sets, the most natural flow would be

start timer
run query
while result sets
fetch result
print result
stop timer
print time

but that would include the printing time in the total time, which the
current code explicitly does not.  We could also temporarily save the
result sets, like

start timer
run query
while result sets
fetch result
stop timer
foreach result set
print result

but that would have a lot more overhead, potentially.

Thoughts?

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 2e5d50cb39b926b29a6081f2387b95621357a4a0 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Thu, 19 Oct 2017 08:18:47 -0400
Subject: [PATCH v1 1/2] psql: Display multiple result sets

If a query returns multiple result sets, display all of them instead of
only the one that PQexec() returns.

Adjust various regression tests to handle the new additional output.
---
 src/bin/psql/common.c  | 25 +++--
 src/test/regress/expected/copyselect.out   |  5 +++
 src/test/regress/expected/psql.out |  6 +---
 src/test/regress/expected/transactions.out | 56 ++
 4 files changed, 76 insertions(+), 16 deletions(-)

diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 9b59ee840b..2b6bd56e12 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1390,22 +1390,25 @@ SendQuery(const char *query)
if (pset.timing)
INSTR_TIME_SET_CURRENT(before);
 
-   results = PQexec(pset.db, query);
+   PQsendQuery(pset.db, query);
 
/* these operations are included in the timing result: */
ResetCancelConn();
-   OK = ProcessResult();
-
-   if (pset.timing)
+   while ((results = PQgetResult(pset.db)))
{
-   INSTR_TIME_SET_CURRENT(after);
-   INSTR_TIME_SUBTRACT(after, before);
-   elapsed_msec = INSTR_TIME_GET_MILLISEC(after);
-   }
+   OK = ProcessResult();
+
+   if (pset.timing)
+   {
+   INSTR_TIME_SET_CURRENT(after);
+   INSTR_TIME_SUBTRACT(after, before);
+   elapsed_msec = INSTR_TIME_GET_MILLISEC(after);
+   }
 
-   /* but printing results isn't: */
-   if (OK && results)
-   OK = PrintQueryResults(results);
+   /* but printing results isn't: */
+   if (OK && results)
+   OK = PrintQueryResults(results);
+   }
}
else
{
diff --git a/src/test/regress/expected/copyselect.out 
b/src/test/regress/expected/copyselect.out
index 72865fe1eb..a13e1b411b 100644
--- a/src/test/regress/expected/copyselect.out
+++ b/src/test/regress/expected/copyselect.out
@@ -136,6 +136,11 @@ copy (select 1) to stdout\; copy (select 2) to stdout\; 
select 0\; select 3; --
 
 create table test3 (c int);
 select 0\; copy test3 from stdin\; copy test3 f

[HACKERS] Transaction control in procedures

2017-10-31 Thread Peter Eisentraut
Here is a patch that implements transaction control in PL/Python
procedures.  (This patch goes on top of "SQL procedures" patch v1.)

So you can do this:

CREATE PROCEDURE transaction_test1()
LANGUAGE plpythonu
AS $$
for i in range(0, 10):
plpy.execute("INSERT INTO test1 (a) VALUES (%d)" % i)
if i % 2 == 0:
 plpy.commit()
else:
 plpy.rollback()
$$;

CALL transaction_test1();

I started with PL/Python because the internal structures there are more
manageable.  Obviously, people will want this for PL/pgSQL as well, and
I have that in the works.  It's not in a usable state, but I have found
that the work needed is essentially the same as in PL/Python for example.

I have discovered three groups of obstacles that needed addressing to
make this work.  At this point, the patch is more of a demo of what
these issues are, and if we come to satisfactory solutions for each of
them, things should fall into place more easily afterwards.

1) While calling CommitTransactionCommand() in the middle of a utility
command works just fine (several utility commands do this, of course),
calling AbortCurrentTransaction() in a similar way does not.  There are
a few pieces of code that think that a transaction abort will always
result in a return to the main control loop, and so they will just clean
away everything.  This is what the changes in portalmem.c are about.
Some comments there already hint about the issue.  No doubt this will
need further refinement.  I think it would be desirable in general to
separate the control flow concerns from the transaction management
concerns more cleanly.

2) SPI needs some work.  It thinks that it can clean everything away at
transaction end.  I have found that instead of TopTransactionContext one
can use PortalContext and get a more suitable life cycle for the memory.
 I have played with some variants to make this configurable (e.g.,
argument to SPI_connect()), but that didn't seem very useful.  There are
some comments indicating that there might not always be a PortalContext,
but the existing tests don't seem to mind.  (There was a thread recently
about making a fake PortalContext for autovacuum, so maybe the current
idea is that we make sure there always is a PortalContext.)  Maybe we
need another context like StatementContext or ProcedureContext.

There also needs to be a way via SPI to end transactions and allowing
*some* cleanup to happen but leaving the memory around.  I have done
that via additional SPI API functions like SPI_commit(), which are then
available to PL implementations.  I also tried making it possible
calling transaction statements directly via SPI_exec() or similar, but
that ended up a total disaster.  So from the API perspective, I like the
current implementation, but the details will no doubt need refinement.

3) The PL implementations themselves allocate memory in
transaction-bound contexts for convenience as well.  This is usually
easy to fix by switching to PortalContext as well.  As you see, the
PL/Python code part of the patch is actually very small.  Changes in
other PLs would be similar.

Two issues have not been addressed yet:

A) It would be desirable to be able to run commands such as VACUUM and
CREATE INDEX CONCURRENTLY in a procedure.  This needs a bit of thinking
and standards-lawyering about the semantics, like where exactly do
transactions begin and end in various combinations.  It will probably
also need a different entry point into SPI, because SPI_exec cannot
handle statements ending transactions.  But so far my assessment is that
this can be added in a mostly independent way later on.

B) There needs to be some kind of call stack for procedure and function
invocations, so that we can track when we are allowed to make
transaction controlling calls.  The key term in the SQL standard is
"non-atomic execution context", which seems specifically devised to
cover this scenario.  So for example, if you have CALL -> CALL -> CALL,
the third call can issue a transaction statement.  But if you have CALL
-> SELECT -> CALL, then the last call cannot, because the SELECT
introduces an atomic execution context.  I don't know if we have such a
thing yet or something that we could easily latch on to.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 1b3318154540d0fe71480ca58938433ecfccabbd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Tue, 31 Oct 2017 14:48:51 -0400
Subject: [PATCH v1] Transaction control in PL/Python procedures

Add .commit, .rollback, and .start_transaction functions to plpy module
to control transactions in a PL/Python function.  Add similar underlying
functions to SPI.  Some additional cleanup so that transaction commit or
abort doesn't blow away data structures still used by the procedure
call.
---
 src/backend/commands/functioncmd

[HACKERS] postgres_fdw: Add support for INSERT OVERRIDING clause

2017-10-31 Thread Peter Eisentraut
It has been pointed out to me that the command deparsing in postgres_fdw
does not support the INSERT OVERRIDING clause that was added in PG10.
Here is a patch that seems to fix that.  I don't know much about this,
whether anything else needs to be added or whether there should be
tests.  Perhaps someone more familiar with postgres_fdw details can
check it.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 097ecf61a9c2740b02a21be19a92aed92388346a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Tue, 31 Oct 2017 11:44:14 -0400
Subject: [PATCH] postgres_fdw: Add support for INSERT OVERRIDING clause

---
 contrib/postgres_fdw/deparse.c | 16 +++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 2af8364010..8eb227605f 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -1573,7 +1573,21 @@ deparseInsertSql(StringInfo buf, PlannerInfo *root,
deparseColumnRef(buf, rtindex, attnum, root, false);
}
 
-   appendStringInfoString(buf, ") VALUES (");
+   appendStringInfoString(buf, ") ");
+
+   switch (root->parse->override)
+   {
+   case OVERRIDING_USER_VALUE:
+   appendStringInfoString(buf, "OVERRIDING USER 
VALUE ");
+   break;
+   case OVERRIDING_SYSTEM_VALUE:
+   appendStringInfoString(buf, "OVERRIDING SYSTEM 
VALUE ");
+   break;
+   default:
+   break;
+   }
+
+   appendStringInfoString(buf, "VALUES (");
 
pindex = 1;
first = true;
-- 
2.14.3


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


[HACKERS] Consistently catch errors from Python _New() functions

2017-10-31 Thread Peter Eisentraut
While reviewing some unrelated code, I noticed that we are handling
error conditions from Python API functions such as PyList_New() and
PyDict_New() in pretty random ways or not at all.  Here is a patch to
fix that.

Arguably, this is a bug fix, but I'm not sure whether it's worth
meddling with this in the back branches.  Maybe only the places where
the errors are not caught at all should be fixed there.  Comments welcome.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From eca2709cd1654e2fc37b75fa3bdfe5e199c86cb9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Tue, 31 Oct 2017 10:49:36 -0400
Subject: [PATCH] Consistently catch errors from Python _New() functions

Python Py*_New() functions can fail and return NULL in out-of-memory
conditions.  The previous code handled that inconsistently or not at
all.  This change organizes that better.  If we are in a function that
is called from Python, we just check for failure and return NULL
ourselves, which will cause any exception information to be passed up.
If we are called from PostgreSQL, we consistently create an "out of
memory" error.
---
 contrib/hstore_plpython/hstore_plpython.c |  4 
 contrib/ltree_plpython/ltree_plpython.c   |  4 
 src/pl/plpython/plpy_cursorobject.c   | 19 ---
 src/pl/plpython/plpy_exec.c   | 10 +-
 src/pl/plpython/plpy_main.c   |  2 +-
 src/pl/plpython/plpy_plpymodule.c |  6 --
 src/pl/plpython/plpy_procedure.c  |  2 ++
 src/pl/plpython/plpy_resultobject.c   | 11 +++
 src/pl/plpython/plpy_spi.c| 25 +
 src/pl/plpython/plpy_typeio.c |  4 +++-
 10 files changed, 67 insertions(+), 20 deletions(-)

diff --git a/contrib/hstore_plpython/hstore_plpython.c 
b/contrib/hstore_plpython/hstore_plpython.c
index 22366bd40f..218e6612b1 100644
--- a/contrib/hstore_plpython/hstore_plpython.c
+++ b/contrib/hstore_plpython/hstore_plpython.c
@@ -93,6 +93,10 @@ hstore_to_plpython(PG_FUNCTION_ARGS)
PyObject   *dict;
 
dict = PyDict_New();
+   if (!dict)
+   ereport(ERROR,
+   (errcode(ERRCODE_OUT_OF_MEMORY),
+errmsg("out of memory")));
 
for (i = 0; i < count; i++)
{
diff --git a/contrib/ltree_plpython/ltree_plpython.c 
b/contrib/ltree_plpython/ltree_plpython.c
index ae9b90dd10..e88636a0a9 100644
--- a/contrib/ltree_plpython/ltree_plpython.c
+++ b/contrib/ltree_plpython/ltree_plpython.c
@@ -46,6 +46,10 @@ ltree_to_plpython(PG_FUNCTION_ARGS)
ltree_level *curlevel;
 
list = PyList_New(in->numlevel);
+   if (!list)
+   ereport(ERROR,
+   (errcode(ERRCODE_OUT_OF_MEMORY),
+errmsg("out of memory")));
 
curlevel = LTREE_FIRST(in);
for (i = 0; i < in->numlevel; i++)
diff --git a/src/pl/plpython/plpy_cursorobject.c 
b/src/pl/plpython/plpy_cursorobject.c
index 0108471bfe..4af7c6621f 100644
--- a/src/pl/plpython/plpy_cursorobject.c
+++ b/src/pl/plpython/plpy_cursorobject.c
@@ -464,15 +464,20 @@ PLy_cursor_fetch(PyObject *self, PyObject *args)
 
Py_DECREF(ret->rows);
ret->rows = PyList_New(SPI_processed);
-
-   for (i = 0; i < SPI_processed; i++)
+   if (!ret->rows)
{
-   PyObject   *row = 
PLyDict_FromTuple(>result,
-   
SPI_tuptable->vals[i],
-   
SPI_tuptable->tupdesc);
-
-   PyList_SetItem(ret->rows, i, row);
+   Py_DECREF(ret);
+   ret = NULL;
}
+   else
+   for (i = 0; i < SPI_processed; i++)
+   {
+   PyObject   *row = 
PLyDict_FromTuple(>result,
+   
SPI_tuptable->vals[i],
+   
SPI_tuptable->tupdesc);
+
+   PyList_SetItem(ret->rows, i, row);
+   }
}
 
SPI_freetuptable(SPI_tuptable);
diff --git a/src/pl/plpython/plpy_exec.c b/src/pl/plpython/plpy_exec.c
index 26f61dd0f3..7cfa146c82 100644
--- a/src/pl/plpython/plpy_exec.c
+++ b/src/pl/plpython/plpy_exec

[HACKERS] Add some const decorations to prototypes

2017-10-31 Thread Peter Eisentraut
Here is a patch that adds const decorations to many char * arguments in
functions.  It should have no impact otherwise; there are very few code
changes caused by it.  Some functions have a strtol()-like behavior
where they take in a const char * and return a pointer into that as
another argument.  In those cases, I added a cast or two.

Generally, I find these const decorations useful as easy function
documentation.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From a8163e84c83887e4a3b81642c137995932701bb5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Tue, 31 Oct 2017 10:34:31 -0400
Subject: [PATCH] Add some const decorations to prototypes

---
 contrib/cube/cubeparse.y   | 12 
 contrib/dict_xsyn/dict_xsyn.c  |  2 +-
 contrib/fuzzystrmatch/dmetaphone.c |  4 +--
 contrib/pgcrypto/pgcrypto.c|  4 +--
 contrib/seg/seg.c  |  4 +--
 contrib/seg/segdata.h  |  2 +-
 contrib/seg/segparse.y |  4 +--
 contrib/unaccent/unaccent.c|  2 +-
 contrib/uuid-ossp/uuid-ossp.c  |  2 +-
 src/backend/access/common/reloptions.c | 12 
 src/backend/access/gist/gistbuild.c|  2 +-
 src/backend/access/transam/xact.c  |  6 ++--
 src/backend/access/transam/xlogarchive.c   |  4 +--
 src/backend/catalog/heap.c | 10 +++
 src/backend/commands/comment.c |  4 +--
 src/backend/commands/event_trigger.c   |  4 +--
 src/backend/commands/extension.c   |  4 +--
 src/backend/commands/indexcmds.c   |  8 +++---
 src/backend/commands/opclasscmds.c |  2 +-
 src/backend/commands/tablecmds.c   | 16 +--
 src/backend/commands/typecmds.c|  6 ++--
 src/backend/commands/view.c|  2 +-
 src/backend/libpq/auth.c   | 24 
 src/backend/libpq/hba.c|  6 ++--
 src/backend/parser/parse_expr.c|  2 +-
 src/backend/parser/parse_func.c|  4 +--
 src/backend/parser/parse_relation.c|  8 +++---
 src/backend/parser/parse_target.c  |  2 +-
 src/backend/port/dynloader/darwin.c|  8 +++---
 src/backend/port/dynloader/darwin.h|  4 +--
 src/backend/port/dynloader/hpux.c  |  4 +--
 src/backend/port/dynloader/hpux.h  |  4 +--
 src/backend/port/dynloader/linux.c |  4 +--
 src/backend/postmaster/postmaster.c|  2 +-
 src/backend/replication/basebackup.c   |  8 +++---
 src/backend/rewrite/rewriteDefine.c|  4 +--
 src/backend/snowball/dict_snowball.c   |  2 +-
 src/backend/storage/lmgr/lwlock.c  |  8 +++---
 src/backend/tsearch/dict_thesaurus.c   |  2 +-
 src/backend/tsearch/spell.c|  4 +--
 src/backend/utils/adt/float.c  | 16 +--
 src/backend/utils/adt/genfile.c|  2 +-
 src/backend/utils/adt/ruleutils.c  |  4 +--
 src/backend/utils/adt/varlena.c|  2 +-
 src/backend/utils/adt/xml.c| 32 +++---
 src/bin/initdb/initdb.c| 12 
 src/bin/pg_dump/pg_backup_db.c |  2 +-
 src/bin/pg_dump/pg_backup_db.h |  2 +-
 src/bin/pg_rewind/fetch.c  |  2 +-
 src/bin/pg_rewind/fetch.h  |  2 +-
 src/bin/pg_upgrade/option.c|  6 ++--
 src/bin/pg_upgrade/pg_upgrade.c|  4 +--
 src/bin/pg_waldump/pg_waldump.c|  2 +-
 src/bin/pgbench/pgbench.c  |  4 +--
 src/include/access/gist_private.h  |  2 +-
 src/include/access/reloptions.h| 14 +-
 src/include/access/xact.h  |  6 ++--
 src/include/access/xlog_internal.h |  4 +--
 src/include/catalog/heap.h |  2 +-
 src/include/commands/comment.h |  4 +--
 src/include/commands/defrem.h  |  4 +--
 src/include/commands/typecmds.h|  2 +-
 src/include/commands/view.h|  2 +-
 src/include/executor/tablefunc.h   |  8 +++---
 src/include/parser/parse_relation.h|  6 ++--
 src/include/parser/parse_target.h  |  2 +-
 src/include/postmaster/bgworker.h  |  2 +-
 src/include/rewrite/rewriteDefine.h|  2 +-
 s

Re: [HACKERS] [GENERAL] Postgres 10 manual breaks links with anchors

2017-10-27 Thread Peter Eisentraut
On 10/26/17 16:10, Tom Lane wrote:
> Peter Eisentraut <peter.eisentr...@2ndquadrant.com> writes:
>> On 10/16/17 03:19, Thomas Kellerer wrote:
>>> I don't know if this is intentional, but the Postgres 10 manual started to 
>>> use lowercase IDs as anchors in the manual.
> 
>> Here is a patch that can be applied to PG 10 to put the upper case
>> anchors back.
>> The question perhaps is whether we want to maintain this patch
>> indefinitely, or whether a clean break is better.
> 
> In view of commit 1ff01b390, aren't we more or less locked into
> lower-case anchors going forward?  I'm not sure I see the point
> of changing v10 back to the old way if v11 will be incompatible
> anyhow.

The details are more complicated.

The IDs in DocBook documents have two purposes.

One is to ensure non-broken links between things like 
and .  This is set up in the DTD and checked during
parsing (validation, more precisely).  In DocBook SGML, many things
including tag names, attribute names, and IDs are case insensitive.  But
in DocBook XML, everything is case sensitive.  So in order to make
things compatible for a conversion, we had to consolidate some variant
spellings that have accumulated in our sources.  For simplicity, I have
converted everything to lower case.

The other purpose is that the DocBook XSL and DSSSL stylesheets use the
IDs for creating anchors in HTML documents (and also for the HTML file
names themselves).  This is merely a useful choice of those stylesheets.

In PG 9.6 and earlier, we used a straight SGML toolchain, using Jade and
DSSSL.  The internal representation of a DocBook SGML document after
parsing converts all the case insensitive bits to upper case.  (This
might be configured somewhere; I'm not sure.)  So the stylesheets see
all the IDs as upper case to begin with, and that's why all the anchors
come out in upper case in the HTML output.

In PG 10, the build first converts the SGML sources to XML, redeclares
them as DocBook XML, then builds using XSLT.  Because DocBook XML
requires lower-case tags and attribute names, we have to use the osx -x
lower option to convert all the case-insensitive bits to lower case
instead of the default upper case.  That's why the XSLT stylesheets see
the IDs as lower case and that's why they are like that in the output.
(If there were options more detailed than -x lower, that could have been
useful.)

The proposed patch works much later in the build process and converts
IDs to upper case only when they are being considered for making an HTML
anchor.  The structure of the document as far as the XML parser is
concerned stays the same.

For PG 11, the idea is to convert the sources to a pure XML document.
XML is case insensitive, so the XML parser would see the IDs as what
they are.  Without the mentioned patch to convert all IDs to lower case
in the source, the XSL processor would see the IDs in whatever case they
are, and anchors would end up in the HTML output using whatever case
they are.  So the conversion to lower case in the source also ensured
anchor compatibility to PG 10.  Otherwise, someone might well have
complained in a similar manner a year from now.

Applying the proposed patch to master/PG 11 would have the same effect
as in PG 10.  It would convert anchors to upper case in the HTML output
but leave the logical and physical structure of the XML document alone.

So the options are simply

1) Use the patch and keep indefinitely, keeping anchors compatible back
to forever and forward indefinitely.

2) Don't use the patch, breaking anchors from <=9.6, but keeping them
compatible going forward.

Considering how small the patch is compared to some other customizations
we carry, #1 seems reasonable to me.  I just didn't know to what extent
people had actually bookmarked fragment links.

-- 
Peter Eisentraut  http://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] inconsistency in process names - bgworker: logical replication launcher

2017-10-27 Thread Peter Eisentraut
On 10/27/17 04:06, Pavel Stehule wrote:
> Why buildin process has prefix bgworker?

Implementation detail.  This has been changed in master already.

-- 
Peter Eisentraut  http://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] [GENERAL] Postgres 10 manual breaks links with anchors

2017-10-26 Thread Peter Eisentraut
On 10/16/17 03:19, Thomas Kellerer wrote:
> I don't know if this is intentional, but the Postgres 10 manual started to 
> use lowercase IDs as anchors in the manual.
> 
> So, if I have e.g.: the following URL open in my browser:
> 
>
> https://www.postgresql.org/docs/current/static/sql-createindex.html#sql-createindex-concurrently
> 
> I cannot simply switch to an older version by replacing "current" with e.g. 
> "9.5" because in the 9.5 manual the anchor was all uppercase, and the URL 
> would need to be: 
> 
>
> https://www.postgresql.org/docs/9.5/static/sql-createindex.html#SQL-CREATEINDEX-CONCURRENTLY
> 
> Is this intentional? 
> 
> This also makes "cleaning" up links in e.g. StackOverflow that point to 
> outdated versions of the manual a bit more cumbersome. 

Here is a patch that can be applied to PG 10 to put the upper case
anchors back.

The question perhaps is whether we want to maintain this patch
indefinitely, or whether a clean break is better.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 9ae0f3c465228a28c2fe50eb2a038e87ccc15b2d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Thu, 26 Oct 2017 15:19:56 -0400
Subject: [PATCH] doc: Convert ids to upper case at build time

This makes the produced HTML anchors upper case, making it backward
compatible with the previous (9.6) build system.
---
 doc/src/sgml/stylesheet-html-common.xsl | 25 +
 1 file changed, 25 insertions(+)

diff --git a/doc/src/sgml/stylesheet-html-common.xsl 
b/doc/src/sgml/stylesheet-html-common.xsl
index 72fac1e806..9d0d10f776 100644
--- a/doc/src/sgml/stylesheet-html-common.xsl
+++ b/doc/src/sgml/stylesheet-html-common.xsl
@@ -263,4 +263,29 @@ set   toc,title
   
 
 
+
+
+
+
+  
+  
+
+  
+
+
+  
+
+
+  
+  
+id-
+
+  
+
+
+  
+
+  
+
+
 
-- 
2.14.3


-- 
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] Is it time to kill support for very old servers?

2017-10-16 Thread Peter Eisentraut
On 9/20/17 04:32, Andres Freund wrote:
> Here's what I roughly was thinking of.  I don't quite like the name, and
> the way the version is specified for libpq (basically just the "raw"
> integer).

"forced_protocol_version" reads wrong to me.  I think
"force_protocol_version" might be better.  Other than that, no issues
with this concept.

-- 
Peter Eisentraut  http://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] coverage analysis improvements

2017-10-16 Thread Peter Eisentraut
On 9/27/17 01:52, Michael Paquier wrote:
> I am marking the full set of patches as ready for committer.

All these patches have now been committed.

-- 
Peter Eisentraut  http://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] pg_regress help output

2017-10-14 Thread Peter Eisentraut
On 10/10/17 22:31, Joe Conway wrote:
>> Also, why is the patch apparently changing whitespace in all the help
>> lines?  Seems like that will create a lot of make-work for translators.
> I debated with myself about that.

Well, there are no translations of pg_regress, so please change the
whitespace to make it look best.

-- 
Peter Eisentraut  http://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] Log LDAP "diagnostic messages"?

2017-10-12 Thread Peter Eisentraut
On 9/24/17 07:00, Thomas Munro wrote:
> Fair point.  In that case there are a few others we should consider
> moving down too for consistency, like in the attached.

> Thanks, that is much tidier.  Done that way in the attached.
> 
> Here also is a small addition to your TAP test which exercises the
> non-NULL code path because slapd rejects TLS by default with a
> diagnostic message.  I'm not sure if this is worth adding, since it
> doesn't actually verify that the code path is reached (though you can
> see that it is from the logs).

Committed.

-- 
Peter Eisentraut  http://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


[HACKERS] replace GrantObjectType with ObjectType

2017-10-11 Thread Peter Eisentraut
It seems to me that having ACL_OBJECT_* symbols alongside OBJECT_*
symbols is not useful and leads to duplication.  Digging around in the
past suggests that we used to have a lot of these command-specific
symbols but got rid of them over time, except that the ACL stuff was
never touched.  The attached patch accomplishes that.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From f80e5aced1cedd2682fd50f6fa067bf455f66f4d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Wed, 11 Oct 2017 18:35:19 -0400
Subject: [PATCH] Replace GrantObjectType with ObjectType

There used to be a lot of different *Type and *Kind symbol groups to
address objects within different commands, most of which have been
replaced by ObjectType, starting with
b256f2426433c56b4bea3a8102757749885b81ba.  But this conversion was never
done for the ACL commands until now.

This change ends up being just a plain replacement of the types and
symbols, without any code restructuring needed, except deleting some now
redundant code.
---
 src/backend/catalog/aclchk.c | 208 +--
 src/backend/catalog/heap.c   |   4 +-
 src/backend/catalog/pg_namespace.c   |   2 +-
 src/backend/catalog/pg_proc.c|   2 +-
 src/backend/catalog/pg_type.c|   2 +-
 src/backend/commands/event_trigger.c | 109 +++---
 src/backend/parser/gram.y|  44 
 src/backend/tcop/utility.c   |   2 +-
 src/backend/utils/adt/acl.c  |  56 +-
 src/include/commands/event_trigger.h |   1 -
 src/include/nodes/parsenodes.h   |  19 +---
 src/include/tcop/deparse_utility.h   |   2 +-
 src/include/utils/acl.h  |   4 +-
 src/include/utils/aclchk_internal.h  |   2 +-
 14 files changed, 203 insertions(+), 254 deletions(-)

diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index ccde66a7dd..2aee4f2751 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -86,7 +86,7 @@ typedef struct
Oid nspid;  /* namespace, or 
InvalidOid if none */
/* remaining fields are same as in InternalGrant: */
boolis_grant;
-   GrantObjectType objtype;
+   ObjectType  objtype;
boolall_privs;
AclMode privileges;
List   *grantees;
@@ -116,8 +116,8 @@ static void ExecGrant_Type(InternalGrant *grantStmt);
 static void SetDefaultACLsInSchemas(InternalDefaultACL *iacls, List *nspnames);
 static void SetDefaultACL(InternalDefaultACL *iacls);
 
-static List *objectNamesToOids(GrantObjectType objtype, List *objnames);
-static List *objectsInSchemaToOids(GrantObjectType objtype, List *nspnames);
+static List *objectNamesToOids(ObjectType objtype, List *objnames);
+static List *objectsInSchemaToOids(ObjectType objtype, List *nspnames);
 static List *getRelationsInNamespace(Oid namespaceId, char relkind);
 static void expand_col_privileges(List *colnames, Oid table_oid,
  AclMode this_privileges,
@@ -441,60 +441,60 @@ ExecuteGrantStmt(GrantStmt *stmt)
 
/*
 * Convert stmt->privileges, a list of AccessPriv nodes, into an AclMode
-* bitmask.  Note: objtype can't be ACL_OBJECT_COLUMN.
+* bitmask.  Note: objtype can't be OBJECT_COLUMN.
 */
switch (stmt->objtype)
{
+   case OBJECT_TABLE:
/*
 * Because this might be a sequence, we test both 
relation and
 * sequence bits, and later do a more limited test when 
we know
 * the object type.
 */
-   case ACL_OBJECT_RELATION:
all_privileges = ACL_ALL_RIGHTS_RELATION | 
ACL_ALL_RIGHTS_SEQUENCE;
errormsg = gettext_noop("invalid privilege type %s for 
relation");
break;
-   case ACL_OBJECT_SEQUENCE:
+   case OBJECT_SEQUENCE:
all_privileges = ACL_ALL_RIGHTS_SEQUENCE;
errormsg = gettext_noop("invalid privilege type %s for 
sequence");
break;
-   case ACL_OBJECT_DATABASE:
+   case OBJECT_DATABASE:
all_privileges = ACL_ALL_RIGHTS_DATABASE;
errormsg = gettext_noop("invalid privilege type %s for 
database");
break;
-   case ACL_OBJECT_DOMAIN:
+   case OBJECT_DOMAIN:
all_privileges = ACL_ALL_RIGHTS_TYPE;
errormsg = gettext_noop("invalid privilege type %s for 
domain");
break;
-   case ACL_OBJECT_FUNCTION:
+   case

Re: [HACKERS] show precise repos version for dev builds?

2017-10-11 Thread Peter Eisentraut
On 10/11/17 04:19, Craig Ringer wrote:
> On 11 October 2017 at 11:44, Jeremy Schneider <schnei...@ardentperf.com> 
> wrote:
>> On Sun, Oct 1, 2017 at 8:10 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>>
>>> configure --with-extra-version=whateveryouwant
>>
>> I see that this build option has been around since 9.4; is anyone
>> using it to mark patched production builds?  EnterpriseDB or
>> 2ndQuadrant? How about the cloud providers?
> 
> We started using it for BDR, but unfortunately too much software
> explodes spectacularly when you use it, due to simplistic/buggy
> version parsing.

I've been using

--with-extra-version=+git`date +%Y%m%d`"~"`git rev-parse --short HEAD`

for my local builds for some time, and I've not experienced any such
problems.

However, using the various numeric reporting options is clearly better
if you want to do version comparisons.  The "extra version" stuff should
be mainly for labeling.

-- 
Peter Eisentraut  http://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


[HACKERS] separate serial_schedule useful?

2017-10-06 Thread Peter Eisentraut
I noticed that the test "hash_func" was listed in parallel_schedule but
not in serial_schedule.  I have seen that a few times recently where a
patch proposes to add a new test file but forgets to add it to the
serial_schedule.

I wonder whether it's still useful to keep two separate test lists.  I
think we could just replace make installcheck with what make
installcheck-parallel MAX_CONNECTIONS=1 does.  Thoughts?

-- 
Peter Eisentraut  http://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] document and use SPI_result_code_string()

2017-10-04 Thread Peter Eisentraut
On 10/2/17 03:28, Daniel Gustafsson wrote:
>> On 06 Sep 2017, at 14:25, Tom Lane <t...@sss.pgh.pa.us> wrote:
>>
>> Michael Paquier <michael.paqu...@gmail.com> writes:
>>> Fine for 0002. This reminds me of LockGXact and RemoveGXact in
>>> twophase.c, as well as _hash_squeezebucket that have some code paths
>>> that cannot return... Any thoughts about having some kind of
>>> PG_NOTREACHED defined to 0 which could be put in an assertion?
>>
>> Generally we just do "Assert(false)", maybe with "not reached" in a
>> comment.  I don't feel a strong need to invent a new way to do that.
> 
> Moving this to the next commitfest and bumping status to Ready for committer
> based on the discussion in this thread.

committed

-- 
Peter Eisentraut  http://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] CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

2017-10-02 Thread Peter Eisentraut
On 10/1/17 14:26, Peter Geoghegan wrote:
> It does seem too late. It's disappointing that we did not do better
> here. This problem was entirely avoidable.

I understand where you are coming from.  My experience with developing
this feature has been that it is quite fragile in some ways.  We have
had this out there for testing for many months, and we have fixed many
bugs, and follow-up bugs, up to very recently.  We don't have good
automatic test coverage, so we need to rely on user testing.  Making
significant code and design changes a week or two before the final
release is just too late, even if the changes were undisputed.  And this
wasn't just one specific isolated change, it was a set of overlapping
changes that seemingly kept changing and expanding.  Analyzing and
reviewing that under pressure, and then effectively owning it going
forward, too, is not something I could commit to.

I'm satisfied that what we are shipping is "good enough".  It has been
in development for over a year, it has been reviewed and tested for
months, and a lot of credit for that goes to you.  The "old" locale
support took many years to take shape, and this one probably will, too,
but at some point I feel we have to let it be for a while and take a
breather and get some feedback and field experience.

I'm available to discuss the changes you are envisioning, preferably one
at a time, in the near future as part of the PG11 development process.

-- 
Peter Eisentraut  http://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] Re: CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

2017-10-01 Thread Peter Eisentraut
On 9/30/17 03:01, Noah Misch wrote:
> On Mon, Sep 25, 2017 at 08:26:21AM +, Noah Misch wrote:
>> On Tue, Sep 19, 2017 at 07:01:47PM -0700, Peter Geoghegan wrote:
>>> On Tue, Sep 19, 2017 at 5:52 PM, Peter Eisentraut
>>> <peter.eisentr...@2ndquadrant.com> wrote:
>>>> On 9/18/17 18:46, Peter Geoghegan wrote:
>>>>> As I pointed out a couple of times already [1], we don't currently
>>>>> sanitize ICU's BCP 47 language tags within CREATE COLLATION.
>>>>
>>>> There is no requirement that the locale strings for ICU need to be BCP
>>>> 47.  ICU locale names like 'de@collation=phonebook' are also acceptable.
>>>
>>> Right. But, we only document that BCP 47 is supported by Postgres.
>>> Maybe we can use get_icu_language_tag()/uloc_toLanguageTag() to ensure
>>> that we end up with BCP 47, even when the user happens to specify the
>>> legacy syntax. Should we be "canonicalizing" legacy ICU locale strings
>>> as BCP 47, too?
>>>
>>>> The reason they are not validated is that, as you know, ICU accepts any
>>>> locale string as valid.  You appear to have found a way to do some
>>>> validation, but I would like to see that code.
>>>
>>> As I mentioned, I'm simply calling
>>> get_icu_language_tag()/uloc_toLanguageTag() to do that sanitization.
>>> The code to get the extra sanitization is completely trivial.
>>>
>>> I didn't post the patch that generates the errors in my opening e-mail
>>> because I'm not confident it's correct just yet. And, I think that I
>>> see a bigger problem: we pass a string that is almost certainly a BCP
>>> 47 string to ucol_open() from within pg_newlocale_from_collation(). We
>>> do so despite the fact that ucol_open() apparently doesn't accept BCP
>>> 47 syntax locale strings until ICU 54 [1]. Seems entirely possible
>>> that this accounts for the problems you saw on ICU 4.2, back when we
>>> were still creating keyword variants (I guess that the keyword
>>> variants seem very "BCP 47-ish" to me).
>>>
>>> I really think we need to add some kind of debug mode that makes ICU
>>> optionally spit out a locale display name at key points. We need this
>>> to gain confidence that the behavior that ICU provides actually
>>> matches what is expected across ICU different versions for different
>>> locale formats. We talked about this as a user-facing feature before,
>>> which can wait till v11; I just want this to debug problems like this
>>> one.
>>>
>>> [1] 
>>> https://ssl.icu-project.org/apiref/icu4c/ucol_8h.html#a4721e4c0a519bb0139a874e191223590
>>
>> [Action required within three days.  This is a generic notification.]
>>
>> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
>> since you committed the patch believed to have created it, you own this open
>> item.  If some other commit is more relevant or if this does not belong as a
>> v10 open item, please let us know.  Otherwise, please observe the policy on
>> open item ownership[1] and send a status update within three calendar days of
>> this message.  Include a date for your subsequent status update.  Testers may
>> discover new open items at any time, and I want to plan to get them all fixed
>> well in advance of shipping v10.  Consequently, I will appreciate your 
>> efforts
>> toward speedy resolution.  Thanks.
>>
>> [1] 
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 
> This PostgreSQL 10 open item is past due for your status update.  On the worst
> week to be violating open item policies.  Kindly send a status update within
> 24 hours, and include a date for your subsequent status update.  Refer to the
> policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

I had previously replied that I think nothing should be changed.  What
process should be applied in that case?


-- 
Peter Eisentraut  http://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] Re: CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

2017-10-01 Thread Peter Eisentraut
On 9/30/17 15:28, Tom Lane wrote:
> This suggests to me that arguing about canonicalization is moot so
> far as avoiding reindexing goes: if you change ICU library versions,
> you're screwed and will have to jump through all the reindexing hoops,
> no matter what we do or don't do.

One reason for that is that the collation version also encodes things
like if the internal method for computing sort keys changes.

-- 
Peter Eisentraut  http://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] Re: CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

2017-10-01 Thread Peter Eisentraut
On 9/30/17 15:28, Tom Lane wrote:
> Now, it may still be worthwhile to argue about whether canonicalization
> will help the other component of the problem, which is whether you can
> dump and reload CREATE COLLATION commands into a new ICU version and
> expect to get more-or-less-the-same behavior as before.

Arguably, you don't always want that.  Maybe you selected a locale name
like 'en-NEWCOUNTRY', and in old ICU versions you want that to fall back
to default 'en' behavior and in newer you get the updated custom behavior.

-- 
Peter Eisentraut  http://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] CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

2017-10-01 Thread Peter Eisentraut
On 9/29/17 19:38, Peter Geoghegan wrote:
> On Mon, Sep 25, 2017 at 11:40 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> Reading over this code again, it is admittedly not quite clear why this
>> "canonicalization" is in there right now.  I think it had something to
>> do with how we built the keyword variants at one point.  It might not
>> make sense.  I'd be glad to take that out and use the result straight
>> from uloc_getAvailable() for collcollate.  That is, after all, the
>> "canonical" version that ICU chooses to report to us.
> 
> Is anything going to happen here ahead of shipping v10? This remains
> an open item.

I'm still pondering committing my documentation patch I had posted, and
I've been reviewing your patches to see if there is anything else
documentation-wise that could be added.

As I had mentioned before, I disagree with the substance of the
functionality changes you are proposing, and I think it would be way too
late to make such significant changes.  The general community opinion
also seems to be in favor of letting things be.

-- 
Peter Eisentraut  http://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] list of credits for release notes

2017-10-01 Thread Peter Eisentraut
On 9/27/17 14:55, Dave Page wrote:
>> Attached is the proposed documentation commit as well as the raw list.
> At the very least my name is missing (I contributed the monitoring roles 
> patch and pg_ls_log/waldir. I have no idea if others are.
> 
> 
>> Thoughts on the heading?  I have considered "Credits",
>> "Acknowledgements", "Thanks", but the first seemed better than the other
>> ones
> I prefer Acknowledgments.

Committed with those changes.

-- 
Peter Eisentraut  http://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 \d sequence display

2017-09-29 Thread Peter Eisentraut
On 9/25/17 13:53, Fabien COELHO wrote:
> \d+ does not show more.
> 
> Maybe Type, Min, Max, Inc & Cycles are enough for \d?

That seems kind of arbitrary.  Start and Cache are just as relevant.

> The next/future or last/previous value is not shown. If one could be 
> available it would be nice to have?

You can get those from the sequence.  Running \d on a table doesn't show
the table data either.  So I think this is a valid distinction.

> Maybe some names are a little large, eg "Increment" could be "Inc.".
> The value is nearly always 1?

Yeah, but then that would look weird if only one heading were
abbreviated.  The total display width is good, so we don't need to
squeeze it too hard.

> Not sure why it is "Cycles" (plural) instead of "Cycle".

This is meant to be a verb, as in "does it cycle?".  I have changed it
to "Cycles?" to match other similar headings.

> I do not understand why some queries have "... \n" "... \n" and others
> have "\n ..." "\n ...". I would suggest to homogeneize around the former,
> because "\nFROM ..." is less readable.

Yeah that is a bit ugly, but I have just moved existing code around.

I have now committed my patch with minor adjustments, so we have
something working for PG10.

-- 
Peter Eisentraut  http://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] list of credits for release notes

2017-09-29 Thread Peter Eisentraut
On 9/29/17 11:35, Robert Haas wrote:
> On Wed, Sep 27, 2017 at 8:29 PM, Michael Paquier
> <michael.paqu...@gmail.com> wrote:
>> Looking at this list, the first name is followed by the family name,
>> so there are inconsistencies with some Japanese names:
>> - Fujii Masao should be Masao Fujii.
>> - KaiGai Kohei should be Kohei Kaigai.
> 
> But his emails say Fujii Masao, not Masao Fujii.
> 
> KaiGai's case is a bit trickier, as his email name has changed over time.

Yes, I used the form that the person used in their emails.

-- 
Peter Eisentraut  http://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: bgw_type (was Re: [HACKERS] Why does logical replication launcher set application_name?)

2017-09-29 Thread Peter Eisentraut
On 9/27/17 18:59, Daniel Gustafsson wrote:
> The patch applies with minor fuzz, compiles without introduced warnings and
> work the way it says on the tin.  The utility of the proposed functionality is
> a clear win so +1 on getting that in.

I have committed this patch incorporating the feedback in this thread.

> Regarding the following hunk:
> 
> +   process listing, for example.  bgw_name on the
> +   other hand can contain additional information about the specific process.
> +   (Typically, the string for bgw_name will 
> contain
> +   the string for bgw_type somehow, but that is 
> not
> +   strictly required.)
> 
> This reads a bit too (oddly) detailed to me, I would’ve phrased it as 
> something
> along the lines of:
> 
> "Typically, the string for bgw_name will contain the type somehow, but 
> that
>  is not strictly required.”

Yes, that's better.

> I find omitting “background worker” in the following hunk is leaving out
> valuable information for bgworkers with badly configured types, but it’s
> definitely a matter of taste rather than a straight-up patch critique:
> 
> -  errmsg("terminating background worker \"%s\" due to administrator 
> command",
> - MyBgworkerEntry->bgw_name)));
> +  errmsg("terminating %s due to administrator command",
> + MyBgworkerEntry->bgw_type)));

Also changed.

-- 
Peter Eisentraut  http://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: bgw_type (was Re: [HACKERS] Why does logical replication launcher set application_name?)

2017-09-29 Thread Peter Eisentraut
On 8/31/17 23:22, Michael Paquier wrote:
> On Fri, Sep 1, 2017 at 4:49 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> On 5/30/17 23:10, Peter Eisentraut wrote:
>>> Here is a proposed solution that splits bgw_name into bgw_type and
>>> bgw_name_extra.  bgw_type shows up in pg_stat_activity.backend_type.
>>> Uses of application_name are removed, because they are no longer
>>> necessary to identity the process type.
>>
>> Updated patch incorporating the feedback.  I have kept bgw_name as it
>> was and just added bgw_type completely independently.
> 
> - errmsg("terminating background worker \"%s\" due to
> administrator command",
> -MyBgworkerEntry->bgw_name)));
> + errmsg("terminating %s due to administrator command",
> +MyBgworkerEntry->bgw_type)));
> "terminating background worker %s of type %s due to administrator
> command", bgw_name, bgw_type?

OK.

>> One open question is how to treat a missing (empty) bgw_type.  I
>> currently fill in bgw_name as a fallback.  We could also treat it as an
>> error or a warning as a transition measure.
> 
> Hm. Why not reporting an empty type string as NULL at SQL level and
> just let it empty them? I tend to like more interfaces that report
> exactly what is exactly registered at memory-level, because that's
> easier to explain to users and in the documentation, as well as easier
> to interpret and easier for module developers.

The problem here is that we refer to bgw_type in a bunch of places now,
and adding a suitable fallback in all of these places would be a lot of
code and it would create a regression in behavior.  In practice, I think
that would be a lot of trouble for no gain.

-- 
Peter Eisentraut  http://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] Patch: add --if-exists to pg_recvlogical

2017-09-29 Thread Peter Eisentraut
On 9/22/17 15:31, Peter Eisentraut wrote:
> I suggest you create a patch that focuses on the --create part.
> 
> You can create a separate follow-on patch for the --start part if you
> wish, but I think that that patch would be rejected.

I have moved this entry to the next commit fest, awaiting your updated
patch.

-- 
Peter Eisentraut  http://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] Rewriting the test of pg_upgrade as a TAP test

2017-09-29 Thread Peter Eisentraut
On 9/22/17 16:48, Peter Eisentraut wrote:
> On 9/19/17 19:00, Michael Paquier wrote:
>> On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut
>> <peter.eisentr...@2ndquadrant.com> wrote:
>>> To get things rolling, I have committed just the basic TAP tests, to
>>> give it a spin on the build farm.  I'll work through the rest in the
>>> coming days.
> 
> I have reverted this because of the build farm issue.  Putting the patch
> on hold in the CF until we have a new plan.

Set to "Returned with feedback" now.

-- 
Peter Eisentraut  http://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


[HACKERS] list of credits for release notes

2017-09-27 Thread Peter Eisentraut
At the PGCon Developer Meeting it was agreed[0] to add a list of credits
to the release notes, including everyone who was mentioned in a commit
message.  I have now completed that list.

Attached is the proposed documentation commit as well as the raw list.

Thoughts on the heading?  I have considered "Credits",
"Acknowledgements", "Thanks", but the first seemed better than the other
ones.

This was a manual process, so mistakes could have been made.  I have
gently edited variant spellings and obvious typos.


For the following mentions I could not identify a name:

mthrockmor...@hme.com
Tels
Zertrin <postgres_w...@zertrin.org>
zam...@gmail.com
bug #14654 reported by James C.
Jov in bug #14749
yxq <y...@o2.pl>

I respect that some people don't want their name on record, but then
they don't go into the release credits either, I think.

The considered commits have been

git log REL9_6_STABLE..REL_10_STABLE

currently up to 9ebc7781444fd15d56ed16e5312a954483e85cd9.

I have also cross-checked the list against all PG10 commit fests, the
committers list, and the contributors list on the web site.  (That
doesn't mean I added all those, but checked for obvious omissions
against those.)

The list is sorted using COLLATE "en-x-icu".


Any thoughts?


[0]:
https://wiki.postgresql.org/wiki/PgCon_2017_Developer_Meeting#Release_notes_scope.2C_and_giving_credit

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From e7384ae908f0f4f151debf20d851130a4342f6fd Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Wed, 27 Sep 2017 13:48:56 -0400
Subject: [PATCH] Add list of credits to release notes

---
 doc/src/sgml/release-10.sgml | 334 +++
 1 file changed, 334 insertions(+)

diff --git a/doc/src/sgml/release-10.sgml b/doc/src/sgml/release-10.sgml
index 9fd3b2c8ac..4b69db2c00 100644
--- a/doc/src/sgml/release-10.sgml
+++ b/doc/src/sgml/release-10.sgml
@@ -3169,4 +3169,338 @@ Additional Modules
 
   
 
+  
+   Credits
+
+   
+The following individuals have contributed to this release as patch
+authors, committers, reviewers, testers, or reporters of issues.
+   
+
+   
+Adam Brightwell
+Adam Brusselback
+Adam Gomaa
+Adam Sah
+Adrian Klaver
+Aidan Van Dyk
+Aleksander Alekseev
+Alexander Korotkov
+Alexander Lakhin
+Alexander Sosna
+Alexey Bashtanov
+Alexey Grishchenko
+Alexey Isayko
+lvaro Hernndez Tortosa
+lvaro Herrera
+Amit Kapila
+Amit Khandekar
+Amit Langote
+Amul Sul
+Anastasia Lubennikova
+Andreas Joseph Krogh
+Andreas Karlsson
+Andreas Scherbaum
+Andreas Seltenreich
+Andres Freund
+Andrew Dunstan
+Andrew Gierth
+Andrew Wheelwright
+Andrey Borodin
+Andrey Lizenko
+Andy Abelisto
+Antonin Houska
+Ants Aasma
+Arjen Nienhuis
+Arseny Sher
+Artur Zakirov
+Ashutosh Bapat
+Ashutosh Sharma
+Ashwin Agrawal
+Atsushi Torikoshi
+Ayumi Ishii
+Basil Bourque
+Beena Emerson
+Ben de Graaff
+Benedikt Grundmann
+Bernd Helmle
+Brad DeJong
+Brandur Leach
+Breen Hagan
+Bruce Momjian
+Bruno Wolff III
+Catalin Iacob
+Chapman Flack
+Chen Huajun
+Choi Doo-Won
+Chris Bandy
+Chris Richards
+Chris Ruprecht
+Christian Ullrich
+Christoph Berg
+Chuanting Wang
+Claudio Freire
+Clinton Adams
+Const Zhang
+Constantin Pan
+Corey Huinker
+Craig Ringer
+Cynthia Shang
+Dagfinn Ilmari Mannsker
+Daisuke Higuchi
+Damian Quiroga
+Daniel Gustafsson
+Daniel Vrit
+Daniel Westermann
+Daniele Varrazzo
+Danylo Hlynskyi
+Darko Prelec
+Dave Cramer
+David Christensen
+David Fetter
+David Johnston
+David Rader
+David Rowley
+David Steele
+Dean Rasheed
+Denis Smirnov
+Denish Patel
+Dennis Bjrklund
+Devrim Gndz
+Dilip Kumar
+Dilyan Palauzov
+Dima Pavlov
+Dimitry Ivanov
+Dmitriy Sarafannikov
+Dmitry Dolgov
+Dmitry Fedin
+Don Morrison
+Egor Rogov
+Eiji Seki
+Emil Iggland
+Emre Hasegeli
+Enrique Meneses
+Erik Nordstrm
+Erik Rijkers
+Erwin Brandstetter
+Etsuro Fujita
+Eugen Konkov
+Eugene Kazakov
+Euler Taveira
+Fabien Coelho
+Fabrzio de Royes Mello
+Fakhroutdinov Evgenievich
+Feike Steenbergen
+Felix Gerzaguet
+Filip Jirsk
+Fujii Masao
+Gabriele Bartolini
+Gabrielle Roth
+Gao Zengqi
+Gerdan Santos
+Gianni Ciolli
+Gilles Darold
+Giuseppe Broccolo
+Graham Dutton
+Greg Atkins
+Greg Burek
+Grigory Smolkin
+Guillaume Lelarge
+Hans Buschmann
+Haribabu Kommi
+Heikki Linnakangas
+Henry Boehlert
+Higuchi Daisuke
+Huan Ruan
+Huong Dangminh
+   

Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present

2017-09-27 Thread Peter Eisentraut
On 9/11/17 03:11, Michael Banck wrote:
> So my patch only moves the slot creation slightly further forward,
> AFAICT.

I have committed this patch, along with some associated cleanup.

> AIUI, wal streaming always begins at last checkpoint and from my tests
> the restart_lsn of the created replication slot is also before that
> checkpoint's lsn. However, I hope somebody more familiar with the
> WAL/replication slot code could comment on that.  What I dropped in the
> refactoring is the RESERVE_WAL that used to be there when the temporary
> slot gets created, I have readded that now.

I had to make some changes to that, because the way your patch was
written it would use RESERVE_WAL also for the calls from pg_receivewal
--create-slot, which would have been a behavior change.  So I added
another argument to CreateReplicationSlot() to control that.

> I also added a TAP test case that tries to check that the restart_lsn is
> lower than the checkpoint_lsn, which appears to be the case.

I think that test was written incorrectly, because it didn't actually
check the $checkpoint_lsn that it read.  I have not included that test
in the committed patch.  Feel free to send another patch if you want to
add more or different tests.

-- 
Peter Eisentraut  http://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] md5 still listed as an option in pg_hba.conf.sample

2017-09-27 Thread Peter Eisentraut
On 9/26/17 18:39, Michael Paquier wrote:
> On Wed, Sep 27, 2017 at 2:41 AM, Mark Dilger <hornschnor...@gmail.com> wrote:
>> I was under the impression that md5 was removed for this release, per other
>> threads that I must not have followed closely enough.
> 
> md5 is still present, its configuration in pg_hba.conf and
> password_encryption are still here. "plain" is what has been removed.

More precisely, the ability to store passwords in plain text in
pg_authid has been removed.  You can still use the authentication method
"password" that sends passwords in plain text over the wire.

-- 
Peter Eisentraut  http://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] Enhancements to passwordcheck

2017-09-27 Thread Peter Eisentraut
On 9/25/17 23:10, Bossart, Nathan wrote:
> One interesting design challenge will be how to handle pre-hashed
> passwords, since the number of checks we can do on those is pretty
> limited.  I'm currently thinking of a parameter that can be used to
> block, allow, or force pre-hashed passwords.

Pre-hashed passwords are the normal case.  You can't break that without
making this module a net loss in security.

-- 
Peter Eisentraut  http://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] Patch to address concerns about ICU collcollate stability in v10 (Was: CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?)

2017-09-25 Thread Peter Eisentraut
On 9/25/17 00:24, Peter Geoghegan wrote:
> * Creates root collation as root-x-icu (collcollate "root"), not
> und-x-icu. "und" means undefined language.

I'm curious about this point.  "und" is defined in BCP 47.  I don't see
"root" defined anywhere.  ICU converts the root collation to "und",
AFAIK, so it seems to agree with the current naming.

-- 
Peter Eisentraut  http://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] CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

2017-09-25 Thread Peter Eisentraut
On 9/22/17 16:46, Peter Geoghegan wrote:
> But you are *already* canonicalizing ICU collation names as BCP 47. My
> point here is: Why not finish the job off, and *also* canonicalize
> colcollate in the same way? This won't break ucol_open() if we take
> appropriate precautions when we go to use the Postgres collation/ICU
> locale.

Reading over this code again, it is admittedly not quite clear why this
"canonicalization" is in there right now.  I think it had something to
do with how we built the keyword variants at one point.  It might not
make sense.  I'd be glad to take that out and use the result straight
from uloc_getAvailable() for collcollate.  That is, after all, the
"canonical" version that ICU chooses to report to us.

> One concern that makes me suggest this is: What happens when
> the user *downgrades* ICU version, from a version where colcollate is
> BCP 47 to one where it would not have been at initdb time? That will
> break the downgrade in an unpleasant way, including in installations
> that never do a CREATE COLLATION themselves. We want to be able to
> restore a basebackup on a somewhat different OS, and have that still
> work following REINDEX. At least, that seems like it should be an
> important goal for us.

This is an interesting point, and my proposal above would fix that.
However, I think that taking a PostgreSQL data directory and moving or
copying it to an *older* OS installation is always going to have a
potential for problems.  So I wouldn't spend a huge amount of effort
just to fix this specific case.

-- 
Peter Eisentraut  http://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


[HACKERS] psql \d sequence display

2017-09-25 Thread Peter Eisentraut
In PostgreSQL 10, the sequence metadata moved from the sequence
"relation" to a system catalog.  The psql \d sequence command was not
updated for that.  (It just did SELECT * FROM seq and there were no
tests, so this was missed.)  Attached is a patch that fixes that up,
taking the opportunity to design a more useful sequence display that is
not merely hacked on to the general relation display.

This should be fixed for PG10, so if you have any feedback on the
design, please let me know soon.

Examples are in the attached patch.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 506cc57e751fa76ac5b5ce298faaa98e5e6462d4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Mon, 25 Sep 2017 11:59:46 -0400
Subject: [PATCH] psql: Update \d sequence display

For \d sequencename, the psql code just did SELECT * FROM sequencename
to get the information to display, but this does not contain much
interesting information anymore in PostgreSQL 10, because the metadata
has been moved to a separate system catalog.

This patch creates a newly designed sequence display that is not merely
an extension of the general relation/table display as it was previously.

Example:

PostgreSQL 9.6:

=> \d foobar
   Sequence "public.foobar"
Column |  Type   |Value
---+-+-
 sequence_name | name| foobar
 last_value| bigint  | 1
 start_value   | bigint  | 1
 increment_by  | bigint  | 1
 max_value | bigint  | 9223372036854775807
 min_value | bigint  | 1
 cache_value   | bigint  | 1
 log_cnt   | bigint  | 0
 is_cycled | boolean | f
 is_called | boolean | f

PostgreSQL 10 before this change:

=> \d foobar
   Sequence "public.foobar"
   Column   |  Type   | Value
+-+---
 last_value | bigint  | 1
 log_cnt| bigint  | 0
 is_called  | boolean | f

New:

=> \d foobar
  Sequence "public.foobar"
  Type  | Start | Minimum |   Maximum   | Increment | Cycles | Cache
+---+-+-+---++---
 bigint | 1 |   1 | 9223372036854775807 | 1 | no | 1
---
 src/bin/psql/describe.c| 189 +++--
 src/test/regress/expected/identity.out |   7 ++
 src/test/regress/expected/sequence.out |  13 +++
 src/test/regress/sql/identity.sql  |   2 +
 src/test/regress/sql/sequence.sql  |   4 +
 5 files changed, 135 insertions(+), 80 deletions(-)

diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 798e71045f..2a5c4408e8 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -1380,8 +1380,6 @@ describeOneTableDetails(const char *schemaname,
int i;
char   *view_def = NULL;
char   *headers[11];
-   char  **seq_values = NULL;
-   char  **ptr;
PQExpBufferData title;
PQExpBufferData tmpbuf;
int cols;
@@ -1563,27 +1561,125 @@ describeOneTableDetails(const char *schemaname,
res = NULL;
 
/*
-* If it's a sequence, fetch its values and store into an array that 
will
-* be used later.
+* If it's a sequence, deal with it here separately.
 */
if (tableinfo.relkind == RELKIND_SEQUENCE)
{
-   printfPQExpBuffer(, "SELECT * FROM %s", fmtId(schemaname));
-   /* must be separate because fmtId isn't reentrant */
-   appendPQExpBuffer(, ".%s;", fmtId(relationname));
+   PGresult   *result = NULL;
+   printQueryOpt myopt = pset.popt;
+   char   *footers[2] = {NULL, NULL};
+
+   if (pset.sversion >= 10)
+   {
+   printfPQExpBuffer(,
+ "SELECT 
pg_catalog.format_type(seqtypid, NULL) AS \"%s\",\n"
+ "   seqstart AS 
\"%s\",\n"
+ "   seqmin AS 
\"%s\",\n"
+ "   seqmax AS 
\"%s\",\n"
+ "   seqincrement 
AS \"%s\",\n"
+ "   CASE WHEN 
seqcycle THEN '%s' ELSE '%s' END AS \"%s\",\n"
+ "   seqcache AS 
\"%s\"\n",
+ gettext_noop("Type"),
+

Re: bgw_type (was Re: [HACKERS] Why does logical replication launcher set application_name?)

2017-09-25 Thread Peter Eisentraut
On 8/31/17 23:22, Michael Paquier wrote:
>> One open question is how to treat a missing (empty) bgw_type.  I
>> currently fill in bgw_name as a fallback.  We could also treat it as an
>> error or a warning as a transition measure.
> 
> Hm. Why not reporting an empty type string as NULL at SQL level and
> just let it empty them? I tend to like more interfaces that report
> exactly what is exactly registered at memory-level, because that's
> easier to explain to users and in the documentation, as well as easier
> to interpret and easier for module developers.

But then background workers that are not updated for, say, PG11 will not
show anything useful in pg_stat_activity.  We should have some amount of
backward compatibility here.

-- 
Peter Eisentraut  http://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: [JDBC] [HACKERS] Channel binding support for SCRAM-SHA-256

2017-09-25 Thread Peter Eisentraut
On 9/10/17 22:37, Michael Paquier wrote:
>>> With the tests directly in the patch, things are easy to run. WIth
>>> PG10 stabilization work, of course I don't expect much feedback :)
>>> But this set of patches looks like the direction we want to go so as
>>> JDBC and libpq users can take advantage of channel binding with SCRAM.
>>
>> Attached is a new patch set, rebased as of c6293249.
> 
> And again a new set to fix the rotten bits caused by 85f4d63.

Here is a review of the meat of the code, leaving aside the discussion
of the libpq connection parameters.

Overall, the structure of the code makes sense and it fits in well with
the existing SCRAM code.


I think the channel-binding negotiation on the client side is wrong.
The logic in the patch is

+#ifdef USE_SSL
+   if (state->ssl_in_use)
+   appendPQExpBuffer(, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE);
+   else
+   appendPQExpBuffer(, "y");
+#else
+   appendPQExpBuffer(, "n");
+#endif

But if SSL is compiled in but not used, the client does not in fact
support channel binding (for that connection), so it should send "n".

The "y" flag should be sent if ssl_in_use but the client did not see the
server advertise SCRAM-SHA256-PLUS.  That logic is missing entirely in
this patch.


You have the server reject a client that does not support channel
binding ("n") on all SSL connections.  I don't think this is correct.
It is up to the client to use channel binding or not, even on SSL
connections.


We should update pg_hba.conf to allow a method specification of
"scram-sha256-plus", i.e., only advertise the channel binding variant to
the client.  Then you could make policy decisions like rejecting clients
that do not use channel binding on SSL connections.  This could be a
separate patch later.


The error message in the "p" case if SSL is not in use is a bit
confusing: "but the server does not need it".  I think this could be
left at the old message "but it is not supported".  This ties into my
interpretation from above that whether channel binding is "supported"
depends on whether SSL is in use for a particular connection.


Some small code things:

- prefer to use size_t over int for length (tls_finish_len etc.)

- tls_finish should be tls_finished

- typos: certificate_bash -> certificate_hash


In the patch for tls-server-end-point, I think the selection of the hash
function deviates slightly from the RFC.  The RFC only says to
substitute MD5 and SHA-1.  It doesn't say to turn SHA-224 into SHA-256,
for example.  There is also the problem that the code as written will
turn any unrecognized hash method into SHA-256.  If think the code
should single out MD5 and SHA-1 only and then use EVP_get_digestbynid()
for the rest.  (I don't know anything about the details of OpenSSL APIs,
but that function sounded right to me.)

-- 
Peter Eisentraut  http://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] ICU locales and text/char(n) SortSupport on Windows

2017-09-24 Thread Peter Eisentraut
On 9/22/17 12:25, Peter Geoghegan wrote:
> On Fri, Sep 22, 2017 at 7:25 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> I agree.  The attached patch should do it.
> 
> I see one small issue here: You'll now need to set ssup->comparator
> back to NULL before you return early in the Windows' libc case. That
> way, a shim comparator (that goes through bttextcmp(), in the case of
> text) will be installed within FinishSortSupportFunction(). Other than
> that, looks good to me.

committed accordingly

-- 
Peter Eisentraut  http://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] SCRAM in the PG 10 release notes

2017-09-23 Thread Peter Eisentraut
On 9/21/17 14:15, Jeff Janes wrote:
> Here is a patch that expands the SCRAM documentation a bit, adds more
> explanation how the different options are related, and sets some better
> links.  I think now you can get from the release notes to the relevant
> documentation and have enough information on how to put the new features
> into use.
> 
> 
> 
> This looks good to me.  Might suggest adding verifying the clients as a
> specific step:

committed

-- 
Peter Eisentraut  http://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] OpenFile() Permissions Refactor

2017-09-23 Thread Peter Eisentraut
On 9/13/17 10:26, David Steele wrote:
> Here's a new patch based on your review.  Where I had a question I made
> a choice as described below:

I have committed the changes to the file APIs and a fix for the umask
save/restore issue.

The mkdir changes didn't really inspire me.  Replacing mkdir() with
MakeDirectoryPerm() without any change in functionality doesn't really
improve clarity.  Maybe we'll revisit that when your next patches arrive.

-- 
Peter Eisentraut  http://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] Rewriting the test of pg_upgrade as a TAP test

2017-09-22 Thread Peter Eisentraut
On 9/19/17 19:00, Michael Paquier wrote:
> On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>> To get things rolling, I have committed just the basic TAP tests, to
>> give it a spin on the build farm.  I'll work through the rest in the
>> coming days.

I have reverted this because of the build farm issue.  Putting the patch
on hold in the CF until we have a new plan.

-- 
Peter Eisentraut  http://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] Patch: add --if-exists to pg_recvlogical

2017-09-22 Thread Peter Eisentraut
On 9/20/17 02:26, Rosser Schwarz wrote:
> The more I think about it, I don't think it's nonsensical, though.
> --create-slot --if-exists or --drop-slot --if-not-exists, obviously. I
> mean, do you even logic?

Those pieces make sense.  We have many CREATE IF NOT EXISTS and DROP IF
EXISTS commands.  The use is clear.

> Those aside, --if-exists just means a non-existent slot isn't an error
> condition, doesn't it? --start --if-exists will start, if the slot
> exists. Otherwise it won't, in neither case raising an error. Exactly
> what it says on the tin. Perhaps the docs could make clear that
> combination implies --no-loop (or at least means we'll exit immediately)
> if the slot does not, in fact, exist?

A non-terrible definition could perhaps be arrived at here, but it's not
clear why one would need this.  We don't have SELECT FROM IF EXISTS, either.

I suggest you create a patch that focuses on the --create part.

You can create a separate follow-on patch for the --start part if you
wish, but I think that that patch would be rejected.

-- 
Peter Eisentraut  http://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] Re: proposal - psql: possibility to specify sort for describe commands, when size is printed

2017-09-22 Thread Peter Eisentraut
On 9/22/17 09:16, Pavel Stehule wrote:
> Example: somebody set SORT_COLUMNS to schema_name value. This is
> nonsense for \l command
> 
> Now, I am thinking so more correct and practical design is based on
> special mode, activated by variable
> 
> PREFER_SIZE_SORT .. (off, asc, desc)
> 
> This has sense for wide group of commands that can show size. And when
> size is not visible, then this option is not active.

Maybe this shouldn't be a variable at all.  It's not like you'll set
this as a global preference.  You probably want it for one command only.
 So a per-command option might make more sense.

-- 
Peter Eisentraut  http://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] DROP SUBSCRIPTION hangs if sub is disabled in the same transaction

2017-09-22 Thread Peter Eisentraut
On 9/21/17 04:43, Masahiko Sawada wrote:
>> Once we got this patch, DROP SUBSCRIPTION is not transactional either
>> if dropping a replication slot or if the subscription got disabled in
>> a transaction block. But we disallow to do DROP SUBSCRIPTION in a
>> transaction block only in the former case. In the latter case, we
>> adopted such non-transactional behaviour. Since these behaviours would
>> be complex for users I attached the documentation patch explaining it.
>>
> Hmm, isn't there necessary to care and mention about this kind of
> inconsistent behavior in docs?

I have added documentation that certain forms of CREATE/DROP
SUBSCRIPTION cannot be run inside a transaction block, which we have
done for other such commands.

I don't think we need to go into further detail.  We don't guarantee
continuous connections.  If a worker is stopped and restarted in the
background as an implementation detail, then the user is not impacted.

-- 
Peter Eisentraut  http://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] CREATE COLLATION does not sanitize ICU's BCP 47 language tags. Should it?

2017-09-22 Thread Peter Eisentraut
On 9/21/17 16:55, Peter Geoghegan wrote:
>> I strongly prefer if there, as much as possible, is only one format for
>> inputting ICU locales.
> I agree, but the bigger issue is that we're *half way* between
> supporting only one format, and supporting two formats. AFAICT, there
> is no reason that we can't simply support one format on all ICU
> versions, and keep what ends up within pg_collation at initdb time
> essentially the same across ICU versions (except for those that are
> due to cultural/political developments).

After reviewing this thread and testing around a bit, I think the code
is mostly fine as it is, but the documentation is lacking.  Hence,
attached is a patch to expand the documentation quite a bit, especially
to document in more detail what ICU locale strings are accepted.

The documentation has always stated, albeit perhaps in obscure ways,
that we accept for locales whatever ICU accepts.  I don't think we
should restrict or override that in any way.  That would cause existing
documentation and lore on ICU to be invalid for PostgreSQL.

I specifically disagree that we should, as appears to be suggested here,
restrict the input locale strings to BCP 47 and reject or transform the
traditional ICU-specific locale syntax.  Again, that would cause
existing ICU documentation material to become less applicable to
PostgreSQL.  And basically, there is no reason for it, because I am not
aware that ICU plans to get rid of that syntax.  Moreover, we need to
support that syntax for older ICU versions anyway.  A patch has been
posted that, as I understand it, would allow BCP 47 syntax to be used
with older versions as well.  That's a nice idea, but it's a new feature
and not appropriate for PG10 at this time.

(Note that we also don't canonicalize libc locale names.)

The attached documentation patch documents both locale naming forms in
parallel.

The other attached patch contains a test suite that verifies that the
examples in the documentation actually work.  It's not meant for
committing into the mainline, but it was useful for me.

During testing with various versions I have also discovered the
following things:

- The current code appears to be of the opinion that BCP 47 locale names
are accepted as of ICU 54.  That appears to be incorrect; I find that
they work fine in ICU 52, but they don't work in ICU 4.2.  I don't know
where the cutoff is.  That might be worth changing in the code if we can
obtain more information.

- What might have led to the above mistake is the following in the
ucol_open() documentation: q{Starting with ICU 54, collation attributes
can be specified via locale keywords as well, in the old locale
extension syntax ("el@colCaseFirst=upper") or in language tag syntax
("el-u-kf-upper").}  That is correct.  If you use that syntax in earlier
versions, the case-first specification in this example is ignored.  You
need to use ucol_setAttribute() then.  (Note that the phonebook stuff
still works, because that is not a "collation attribute".)

(One of my plans for PG11 had been to allow specifying such collation
attributes via additional CREATE COLLATION clauses and pg_collation
columns, but that might be obsolete now.)

- Moreover, it is not the case that ICU accepts just any sort of
nonsense as a locale name.  For example, "el@colCaseFirst=whatever" is
rejected with U_ILLEGAL_ARGUMENT_ERROR.  Now, it might in other cases be
more liberal than we might be hoping for.  But I think they have reasons
for what they do.

One conclusion here is that there are multiple dimensions to what sort
of thing is accepted as a locale in different versions of ICU and what
the canonical format is.  If we insist that everything has to be written
in the form that is preferred today, then we'll have awkward problems if
a future version of ICU establishes even more variants that will then be
preferred.

I think there is room for incremental refinement here.  Features like
optionally checking or printing the canonical or preferred locale format
or making the locale description available via a function or system view
might all be valuable additions to a future PostgreSQL release.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 5eac8a0df7163f8374382d37b32b9c2d3580238d Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Fri, 22 Sep 2017 13:51:01 -0400
Subject: [PATCH 1/2] Expand collation documentation

Document better how to create custom collations and what locale strings
ICU accepts.  Explain the ICU examples in more detail.  Also update the
text on the CREATE COLLATION reference page a bit to take ICU more into
account.
---
 doc/src/sgml/charset.sgml  | 135 ++---
 doc/src/sgml/ref/create_collation.sgml |  28 ---
 2 files changed, 124 insertions(+), 39 deletions(-)

dif

Re: [HACKERS] additional contrib test suites

2017-09-22 Thread Peter Eisentraut
On 9/18/17 09:54, Peter Eisentraut wrote:
> On 9/16/17 08:10, David Steele wrote:
>>>> (5) drop contrib/chkpass altogether, on the grounds that it's too badly
>>>> designed, and too obsolete crypto-wise, to be useful or supportable.
>>> crypt() uses the 7 lowest characters, which makes for 7.2e16 values,
>>> so I would be fine with (5), then (4) as the test suite is not
>>> portable.
>> I'd prefer 5, but can go with 4.
>>
>> I get that users need to store their own passwords, but we have support
>> for SHA1 via the crypto module which seems by far the better choice.
> 
> I'm also tempted to just remove it.  It uses bad/outdated security
> practices and it's also not ideal as an example module.  Any objections?

Hearing none, thus removed.

-- 
Peter Eisentraut  http://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] coverage analysis improvements

2017-09-22 Thread Peter Eisentraut
On 9/21/17 03:42, Michael Paquier wrote:
> -SPI.c: SPI.xs plperl_helpers.h
> +%.c: %.xs
> @if [ x"$(perl_privlibexp)" = x"" ]; then echo "configure switch
> --with-perl was not specified."; exit 1; fi
> -   $(PERL) $(XSUBPPDIR)/ExtUtils/xsubpp -typemap
> $(perl_privlibexp)/ExtUtils/typemap $< >$@
> Doing coverage on plperl with this patch applied, those do not seem
> necessary. But I don't know enough this code to give a clear opinion.

That patch is necessary for doing make coverage in vpath builds.
Otherwise it makes no difference.

> Running coverage-html with all the patches, I am seeing the following
> warnings with a fresh build on my macos laptop 10.11:
> geninfo: WARNING: gcov did not create any files for
> /Users/mpaquier/git/postgres/src/backend/access/transam/rmgr.gcda!
> I don't think that this is normal.

Apparently, rmgr.c doesn't contain any instrumentable code.  I don't see
this warning, but it might depend on tool versions and compiler options.

Note that rmgr.c doesn't show up here either:
https://coverage.postgresql.org/src/backend/access/transam/index.html

-- 
Peter Eisentraut  http://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] ICU locales and text/char(n) SortSupport on Windows

2017-09-22 Thread Peter Eisentraut
On 9/21/17 15:21, Peter Geoghegan wrote:
> On Thu, Sep 21, 2017 at 12:12 PM, Peter Eisentraut
> <peter.eisentr...@2ndquadrant.com> wrote:
>>> Attached patch shows what I'm getting at. This is untested, since I
>>> don't use Windows. Proceed with caution.
>>
>> Your analysis makes sense, but the patch doesn't work, because "locale"
>> is never set before reading it.
> 
> It was just for illustrative purposes. Seems fine to actually move the
> WIN32 block down to just before the existing TRUST_STRXFRM test, since
> the locale is going to be cached and then immediately reused where we
> return immediately (Windows libc) anyway. This would also be a win for
> clarity, IMV.

I agree.  The attached patch should do it.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From c2c875fe447eaf65f2ba5b28e77dcc2e42016455 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Fri, 22 Sep 2017 10:23:35 -0400
Subject: [PATCH v2] Allow ICU to use SortSupport on Windows with UTF-8

There is no reason to ever prevent the use of SortSupport on Windows
when ICU locales are used.  We previously avoided SortSupport on Windows
with UTF-8 server encoding and a non C-locale due to restrictions in
Windows' libc functionality.

This is now considered to be a restriction in one platform's libc
collation provider, and not a more general platform restriction.

Reported-by: Peter Geoghegan <p...@bowt.ie>
---
 src/backend/utils/adt/varlena.c | 25 +++--
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index ebfb823fb8..071bc83378 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -1823,12 +1823,6 @@ varstr_sortsupport(SortSupport ssup, Oid collid, bool 
bpchar)
 * requirements of BpChar callers.  However, if LC_COLLATE = C, we can
 * make things quite a bit faster with varstrfastcmp_c or 
bpcharfastcmp_c,
 * both of which use memcmp() rather than strcoll().
-*
-* There is a further exception on Windows.  When the database encoding 
is
-* UTF-8 and we are not using the C collation, complex hacks are 
required.
-* We don't currently have a comparator that handles that case, so we 
fall
-* back on the slow method of having the sort code invoke bttextcmp() 
(in
-* the case of text) via the fmgr trampoline.
 */
if (lc_collate_is_c(collid))
{
@@ -1839,10 +1833,6 @@ varstr_sortsupport(SortSupport ssup, Oid collid, bool 
bpchar)
 
collate_c = true;
}
-#ifdef WIN32
-   else if (GetDatabaseEncoding() == PG_UTF8)
-   return;
-#endif
else
{
ssup->comparator = varstrfastcmp_locale;
@@ -1869,6 +1859,21 @@ varstr_sortsupport(SortSupport ssup, Oid collid, bool 
bpchar)
}
}
 
+   /*
+* There is a further exception on Windows.  When the database encoding 
is
+* UTF-8 and we are not using the C collation, complex hacks are 
required.
+* We don't currently have a comparator that handles that case, so we 
fall
+* back on the slow method of having the sort code invoke bttextcmp() 
(in
+* the case of text) via the fmgr trampoline.  ICU locales work just the
+* same on Windows, however.
+*/
+#ifdef WIN32
+   if (!collate_c &&
+   GetDatabaseEncoding() == PG_UTF8 &&
+   !(locale && locale->provider == COLLPROVIDER_ICU))
+   return;
+#endif
+
/*
 * Unfortunately, it seems that abbreviation for non-C collations is
 * broken on many common platforms; testing of multiple versions of 
glibc
-- 
2.14.1


-- 
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] !USE_WIDE_UPPER_LOWER compile errors in v10+

2017-09-22 Thread Peter Eisentraut
On 9/21/17 17:38, Tom Lane wrote:
> Meanwhile, I see that Peter has posted a fix for the immediate problem.
> I propose that Peter should apply his fix in HEAD and v10, and then

done

> I'll rip out the !USE_WIDE_UPPER_LOWER code paths in HEAD only.

-- 
Peter Eisentraut  http://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] [COMMITTERS] pgsql: Fix bool/int type confusion

2017-09-22 Thread Peter Eisentraut
On 9/21/17 14:58, Tom Lane wrote:
> I've just been going through their git commit log to see what else has
> changed since tzcode2017b, and I note that there are half a dozen other
> portability-ish fixes.  I think that some of them affect only code we
> don't use, but I'm not sure that that's the case for all.  So I'm a bit
> inclined to go with plan B, that is sync with their current HEAD now.

I suppose it might be good to do this after 10.0 final is wrapped?

-- 
Peter Eisentraut  http://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] SCRAM in the PG 10 release notes

2017-09-22 Thread Peter Eisentraut
On 9/21/17 14:15, Jeff Janes wrote:
> This looks good to me.  Might suggest adding verifying the clients as a
> specific step:  
> 
> "To upgrade an existing installation from md5 to scram-sha-256, verify
> that all client software supports it, set password_encryption =
> 'scram-sha-256' in postgresql.conf..."

I don't think there is a well-defined way of verifying whether all
client software supports it other than making the switch described and
then checking what breaks.  So it's a bit of a circular process.

-- 
Peter Eisentraut  http://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] [PATCH] Generic type subscripting

2017-09-22 Thread Peter Eisentraut
On 9/21/17 11:24, Dmitry Dolgov wrote:
> One last thing that I need to clarify. Initially there was an idea to
> minimize changes in `pg_type`

I see, but there is no value in that if it makes everything else more
complicated.

-- 
Peter Eisentraut  http://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] ICU locales and text/char(n) SortSupport on Windows

2017-09-21 Thread Peter Eisentraut
On 9/16/17 18:33, Peter Geoghegan wrote:
> In summary, we're currently attaching the use of SortSupport to the
> wrong thing. We're treating this UTF-16 business as something that
> implies a broad OS/platform restriction, when in fact it should be
> treated as implying a restriction for one particular collation
> provider only (a collation provider that happens to be built into
> Windows, but isn't really special to us).
> 
> Attached patch shows what I'm getting at. This is untested, since I
> don't use Windows. Proceed with caution.

Your analysis makes sense, but the patch doesn't work, because "locale"
is never set before reading it.

-- 
Peter Eisentraut  http://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] !USE_WIDE_UPPER_LOWER compile errors in v10+

2017-09-21 Thread Peter Eisentraut
On 9/21/17 01:29, Noah Misch wrote:
> I checked !USE_WIDE_UPPER_LOWER by configuring v10 as follows:
> 
>   ./configure -C --prefix=$HOME/sw/nopath/pg10 --enable-debug \
>   --enable-cassert --enable-depend --enable-tap-tests --with-libxml \
>   --with-gssapi --with-openssl ac_cv_func_towlower=no
> 
> The result fails to compile:

Yeah, the placement of the ifdef blocks was pretty bogus.  This patch
fixes it.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 9e51e4e52ae565503934c97c84ee5a7d9bcb2dcc Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Thu, 21 Sep 2017 14:42:10 -0400
Subject: [PATCH] Fix build with !USE_WIDE_UPPER_LOWER

The placement of the ifdef blocks in formatting.c was pretty bogus, so
the code failed to compile if USE_WIDE_UPPER_LOWER was not defined.
---
 src/backend/utils/adt/formatting.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/backend/utils/adt/formatting.c 
b/src/backend/utils/adt/formatting.c
index 46f45f6654..2bf484cda3 100644
--- a/src/backend/utils/adt/formatting.c
+++ b/src/backend/utils/adt/formatting.c
@@ -1528,7 +1528,6 @@ str_tolower(const char *buff, size_t nbytes, Oid collid)
{
result = asc_tolower(buff, nbytes);
}
-#ifdef USE_WIDE_UPPER_LOWER
else
{
pg_locale_t mylocale = 0;
@@ -1566,6 +1565,7 @@ str_tolower(const char *buff, size_t nbytes, Oid collid)
else
 #endif
{
+#ifdef USE_WIDE_UPPER_LOWER
if (pg_database_encoding_max_length() > 1)
{
wchar_t*workspace;
@@ -1603,8 +1603,8 @@ str_tolower(const char *buff, size_t nbytes, Oid collid)
wchar2char(result, workspace, result_size, 
mylocale);
pfree(workspace);
}
-#endif /* USE_WIDE_UPPER_LOWER 
*/
else
+#endif /* USE_WIDE_UPPER_LOWER 
*/
{
char   *p;
 
@@ -1652,7 +1652,6 @@ str_toupper(const char *buff, size_t nbytes, Oid collid)
{
result = asc_toupper(buff, nbytes);
}
-#ifdef USE_WIDE_UPPER_LOWER
else
{
pg_locale_t mylocale = 0;
@@ -1690,6 +1689,7 @@ str_toupper(const char *buff, size_t nbytes, Oid collid)
else
 #endif
{
+#ifdef USE_WIDE_UPPER_LOWER
if (pg_database_encoding_max_length() > 1)
{
wchar_t*workspace;
@@ -1727,8 +1727,8 @@ str_toupper(const char *buff, size_t nbytes, Oid collid)
wchar2char(result, workspace, result_size, 
mylocale);
pfree(workspace);
}
-#endif /* USE_WIDE_UPPER_LOWER 
*/
else
+#endif /* USE_WIDE_UPPER_LOWER 
*/
{
char   *p;
 
@@ -1777,7 +1777,6 @@ str_initcap(const char *buff, size_t nbytes, Oid collid)
{
result = asc_initcap(buff, nbytes);
}
-#ifdef USE_WIDE_UPPER_LOWER
else
{
pg_locale_t mylocale = 0;
@@ -1815,6 +1814,7 @@ str_initcap(const char *buff, size_t nbytes, Oid collid)
else
 #endif
{
+#ifdef USE_WIDE_UPPER_LOWER
if (pg_database_encoding_max_length() > 1)
{
wchar_t*workspace;
@@ -1864,8 +1864,8 @@ str_initcap(const char *buff, size_t nbytes, Oid collid)
wchar2char(result, workspace, result_size, 
mylocale);
pfree(workspace);
}
-#endif /* USE_WIDE_UPPER_LOWER 
*/
else
+#endif /* USE_WIDE_UPPER_LOWER 
*/
{
char   *p;
 
-- 
2.14.1


-- 
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: Fix bool/int type confusion

2017-09-21 Thread Peter Eisentraut
On 9/21/17 11:46, Tom Lane wrote:
> So the code in their git repo still has the variable as bool, but
> there's no ++ operator on it anymore.
> 
> This also means that the portability problem is purely hypothetical;
> given valid tz reference data the code wouldn't ever try to increment
> "hit" to 2 anyway.  It's even more hypothetical for us, because we don't
> use leap-second-aware zones.

It's not so much that there is an actual portability problem.  The
problem is that the compiler issues a warning for this with stdbool.

> Therefore, what I would like to do is revert this commit (0ec2e908b),
> and then either leave the problem to be fixed by our next regular sync
> with a released tzcode version, or else force a sync with their git tip
> to absorb their fix now.  In either case we should keep all our live
> branches in sync.  I'm willing to do the code-sync work either way.

That makes sense.  There is no urgency on the stdbool patch set, so
waiting for this to be resolved properly around November seems reasonable.

-- 
Peter Eisentraut  http://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] Re: proposal - psql: possibility to specify sort for describe commands, when size is printed

2017-09-21 Thread Peter Eisentraut
On 9/21/17 13:54, Pavel Stehule wrote:
> I see where you are coming from, but there is no association in the
> existing UI that equates "+" to the word "verbose".  I think just
> removing the verbose prefix and applying the sorting behavior in all
> cases should be easier to explain and implement.
> 
> I though about it - but I am not sure if one kind of these variables is
> practical.
> 
> if I don't need a size, then sort by schema, name is ok (I didn't need
> any else ever). With only one kind of these variables, this setting is
> common - what is not practical.

But you are proposing also to add a variable configuring the sort
direction.  It would be weird that \dX+ observed the sort direction but
\dX did not.

-- 
Peter Eisentraut  http://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] SCRAM in the PG 10 release notes

2017-09-21 Thread Peter Eisentraut
On 9/20/17 15:52, Jeff Janes wrote:
> I think that the addition of a link to
> > https://wiki.postgresql.org/wiki/List_of_drivers
> <https://wiki.postgresql.org/wiki/List_of_drivers> would be appropriate.
> 
> I don't have any expectation that that list will be kept up to date.
> 
> I am not confident that it will be either, but what could we ever have
> more confidence in being kept up-to-date than something anyone can
> update which is hosted on a community asset?
If we put such a list linked from the documentation, we have to keep it
up to date for years, and no one is committing to do that.

I don't think it's our job to maintain lists of which third-party
products are ready to take advantage of new features in PostgreSQL.  I
don't see a list of GUIs ready to work with the new partitioning or
monitoring tools ready to work with the new xlog/wal naming.  If some
folks want to maintain such lists, that's great, but it's not a release
issue.

-- 
Peter Eisentraut  http://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] SCRAM in the PG 10 release notes

2017-09-21 Thread Peter Eisentraut
On 9/19/17 20:45, Peter Eisentraut wrote:
> On 9/19/17 17:55, Jeff Janes wrote:
>> I guess I'm late to the party, but I don't see why this is needed at
>> all.  We encourage people to use any and all new features which are
>> appropriate to them--that is why we implement new features.  Why does
>> this feature need a special invitation?
> 
> It's not clear to me how an average user would get from the press
> release or release notes to upgrading their installation to use
> SCRAM-based authentication and passwords.  A little bit more guidance
> somewhere would be helpful.

Here is a patch that expands the SCRAM documentation a bit, adds more
explanation how the different options are related, and sets some better
links.  I think now you can get from the release notes to the relevant
documentation and have enough information on how to put the new features
into use.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 2e7640d9a6d65664721fff8d4acdd3c9289027b0 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pete...@gmx.net>
Date: Thu, 21 Sep 2017 10:33:09 -0400
Subject: [PATCH] doc: Expand user documentation on SCRAM

Explain more about how the different password authentication methods and
the password_encryption settings relate to each other, give some
upgrading advice, and set a better link from the release notes.
---
 doc/src/sgml/client-auth.sgml | 122 --
 doc/src/sgml/config.sgml  |   2 +-
 doc/src/sgml/release-10.sgml  |   2 +-
 3 files changed, 95 insertions(+), 31 deletions(-)

diff --git a/doc/src/sgml/client-auth.sgml b/doc/src/sgml/client-auth.sgml
index 1b568683a4..f2f7527107 100644
--- a/doc/src/sgml/client-auth.sgml
+++ b/doc/src/sgml/client-auth.sgml
@@ -916,46 +916,78 @@ Password Authentication

 MD5

+   
+SCRAM
+   

 password
 authentication

 

-The password-based authentication methods are scram-sha-256,
-md5, and password. These methods operate
-similarly except for the way that the password is sent across the
+There are several password-based authentication methods.  These methods
+operate similarly but differ in how the users' passwords are stored on the
+server and how the password provided by a client is sent across the
 connection.

 
-   
-Plain password sends the password in clear-text, and is
-therefore vulnerable to password sniffing attacks. It should
-always be avoided if possible. If the connection is protected by SSL
-encryption then password can be used safely, though.
-(Though SSL certificate authentication might be a better choice if one
-is depending on using SSL).
-   
+   
+
+ scram-sha-256
+ 
+  
+   The method scram-sha-256 performs SCRAM-SHA-256
+   authentication, as described in
+   https://tools.ietf.org/html/rfc7677;>RFC 7677.  It
+   is a challenge-response scheme that prevents password sniffing on
+   untrusted connections and supports storing passwords on the server in a
+   cryptographically hashed form that is thought to be secure.
+  
 
+  
+   This is the most secure of the currently provided methods but might not
+   be supported by older client libraries.
+  
+ 
+
 
-   
-scram-sha-256 performs SCRAM-SHA-256 authentication, as
-described in
-https://tools.ietf.org/html/rfc7677;>RFC 7677. It
-is a challenge-response scheme, that prevents password sniffing on
-untrusted connections. It is more secure than the md5
-method, but might not be supported by older clients.
-   
+
+ md5
+ 
+  
+   The method md5 uses a custom less secure challenge-response
+   mechanism.  It prevents password sniffing and avoids storing passwords
+   on the server in plain text, but provides no protection if an attacker
+   manages to steal the password hash from the server.  Also, the MD5 hash
+   algorithm is nowadays no longer consider secure against determined
+   attacks.  The md5 method cannot be used with
+   the  feature.
+  
 
-   
-md5 allows falling back to a less secure challenge-response
-mechanism for those users with an MD5 hashed password.
-The fallback mechanism also prevents password sniffing, but provides no
-protection if an attacker manages to steal the password hash from the
-server, and it cannot be used with the  feature. For all other users,
-md5 works the same as scram-sha-256.
-   
+  
+   To ease transition from the md5 method to the newer
+   SCRAM method, if md5 is specified as a method
+   in pg_hba.conf but the user's password in the
+   server is encrypted for SCRAM (see below), then SCRAM-based
+   authentication will automatically be chosen instead.
+  
+ 
+
+
+
+

  1   2   3   4   5   6   7   8   9   10   >