Re: [HACKERS] LDAP URI decoding bugs
On 11/6/17 23:30, Michael Paquier wrote: > On Fri, Nov 3, 2017 at 12:57 PM, Thomas Munro > 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
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
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
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
On 11/8/17 11:11, Merlin Moncure wrote: > On Wed, Nov 8, 2017 at 9:13 AM, Peter Eisentraut > 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
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(&btree->itemptr, iptr) > 0) ? TRUE : FALSE; > + return (ginCompareItemPointers(&btree->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
On 11/7/17 19:58, Michael Paquier wrote: > On Wed, Nov 8, 2017 at 9:50 AM, Haribabu Kommi > 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
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
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
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
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
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
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
On 10/29/17 08:50, Michael Paquier wrote: > On Thu, Oct 26, 2017 at 9:25 AM, Peter Eisentraut > 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
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 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 | 4
Re: [HACKERS] Dynamic result sets from procedures
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
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
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
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
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
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
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'
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
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
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
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
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
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
On 9/15/17 00:20, Michael Paquier wrote: > On Fri, Sep 15, 2017 at 12:02 PM, Michael Paquier > wrote: >> On Fri, Sep 15, 2017 at 11:46 AM, Peter Eisentraut wrote: >>> passwordcheck: Add test suite >>> >>> Also improve one error message. >>> >>> Reviewed-by: David Steele >> >> 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
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
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)
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
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
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
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
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
On 11/1/17 10:29, Michael Paquier wrote: > On Wed, Nov 1, 2017 at 2:24 PM, Peter Eisentraut > 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
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
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
On 9/13/17 03:45, Alexey Chernyshov wrote: > On Sat, 9 Sep 2017 13:53:35 +0530 > Ashutosh Sharma 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
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
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
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 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(&results); - - 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(&results); + + 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 fro
[HACKERS] Transaction control in procedures
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 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/functioncmds.c
[HACKERS] postgres_fdw: Add support for INSERT OVERRIDING clause
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 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
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 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(&cursor->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(&cursor->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/plpyth
[HACKERS] Add some const decorations to prototypes
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 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 +- src/include/storage/lwlo
Re: [HACKERS] [GENERAL] Postgres 10 manual breaks links with anchors
On 10/26/17 16:10, Tom Lane wrote: > Peter Eisentraut 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
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
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 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?
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
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
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"?
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
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 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 OBJECT_FUNCTION:
Re: [HACKERS] show precise repos version for dev builds?
On 10/11/17 04:19, Craig Ringer wrote: > On 11 October 2017 at 11:44, Jeremy Schneider > wrote: >> On Sun, Oct 1, 2017 at 8:10 AM, Tom Lane 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?
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()
On 10/2/17 03:28, Daniel Gustafsson wrote: >> On 06 Sep 2017, at 14:25, Tom Lane wrote: >> >> Michael Paquier 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?
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?
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 >>> 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?
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?
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?
On 9/29/17 19:38, Peter Geoghegan wrote: > On Mon, Sep 25, 2017 at 11:40 AM, Peter Eisentraut > 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
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
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
On 9/29/17 11:35, Robert Haas wrote: > On Wed, Sep 27, 2017 at 8:29 PM, Michael Paquier > 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?)
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?)
On 8/31/17 23:22, Michael Paquier wrote: > On Fri, Sep 1, 2017 at 4:49 AM, Peter Eisentraut > 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
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
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 >> 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
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 zam...@gmail.com bug #14654 reported by James C. Jov in bug #14749 yxq 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 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 Hernández 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 Mannsåker +Daisuke Higuchi +Damian Quiroga +Daniel Gustafsson +Daniel Vérité +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 Björklund +Devrim Gündüz +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 Nordström +Erik Rijkers +Erwin Brandstetter +Etsuro Fujita +Eugen Konkov +Eugene Kazakov +Euler Taveira +Fabien Coelho +Fabrízio de Royes Mello +Fakhroutdinov Evgenievich +Feike Steenbergen +Felix Gerzaguet +Filip Jirsák +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 +Ian Barwick +Igor Korot +Ildus Kurbangali
Re: [HACKERS] Create replication slot in pg_basebackup if requested and not yet present
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
On 9/26/17 18:39, Michael Paquier wrote: > On Wed, Sep 27, 2017 at 2:41 AM, Mark Dilger 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
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?)
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?
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
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 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(&buf, "SELECT * FROM %s", fmtId(schemaname)); - /* must be separate because fmtId isn't reentrant */ - appendPQExpBuffer(&buf, ".%s;", fmtId(relationname)); + PGresult *result = NULL; + printQueryOpt myopt = pset.popt; + char *footers[2] = {NULL, NULL}; + + if (pset.sversion >= 10) + { + printfPQExpBuffer(&buf, + "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", + gett
Re: bgw_type (was Re: [HACKERS] Why does logical replication launcher set application_name?)
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
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(&buf, "p=%s", SCRAM_CHANNEL_TLS_UNIQUE); + else + appendPQExpBuffer(&buf, "y"); +#else + appendPQExpBuffer(&buf, "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
On 9/22/17 12:25, Peter Geoghegan wrote: > On Fri, Sep 22, 2017 at 7:25 AM, Peter Eisentraut > 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
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
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
On 9/19/17 19:00, Michael Paquier wrote: > On Wed, Sep 20, 2017 at 7:57 AM, Peter Eisentraut > 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
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
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
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?
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 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
Re: [HACKERS] additional contrib test suites
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
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
On 9/21/17 15:21, Peter Geoghegan wrote: > On Thu, Sep 21, 2017 at 12:12 PM, Peter Eisentraut > 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 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 --- 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+
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
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
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
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
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+
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 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
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
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
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
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 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. + + +