Re: [HACKERS] Multi column range partition table
On Sun, Jul 9, 2017 at 2:42 AM, Dean Rasheedwrote: > On 6 July 2017 at 22:43, Joe Conway wrote: >> I agree we should get this right the first time and I also agree with >> Dean's proposal, so I guess I'm a +2 > > On 7 July 2017 at 03:21, Amit Langote wrote: >> +1 to releasing this syntax in PG 10. > > So, that's 3 votes in favour of replacing UNBOUNDED with > MINVALUE/MAXVALUE for range partition bounds in PG 10. Not a huge > consensus, but no objections either. Any one else have an opinion? > > Robert, have you been following this thread? Uh, no. Sorry. I agree that it's a big problem that (10, UNBOUNDED) interpreted as a maximum value means first_column <= 10 and when interpreted as a minimum value means first_column >= 10, because those things aren't opposites of each other. I guess the proposal here would make (10, MAXVALUE) as a maximum value mean first_column <= 10 and as a minimum would mean first_column > 10, and contrariwise for MINVALUE. That seems to restore the intended design principle of the system, which is good, but... ...originally, Amit proposed to attach a postfix INCLUSIVE or EXCLUSIVE to each bound specification, and this does feel like a bit of a back door to the same place, kinda. A partition defined to run from (10, MAXVALUE) TO (11, MAXVALUE) is a lot like a partition defined to run from (10) EXCLUSIVE to (11) EXCLUSIVE. And if we eventually decide to allow that, then what will be the difference between a partition which starts at (10, MAXVALUE) EXCLUSIVE and one which starts from (10, MAXVALUE) INCLUSIVE? I haven't thought through this well enough to be sure that there's any problem with what is being proposed, and I definitely don't have a better solution off the top of my head, but I feel slightly nervous. Apologies again for the slow response - will update again by Monday. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Subscription code improvements
On Wed, Jul 12, 2017 at 11:19 AM, Masahiko Sawadawrote: > On Sat, Jul 8, 2017 at 5:19 AM, Petr Jelinek > wrote: >> Hi, >> >> I have done some review of subscription handling (well self-review) and >> here is the result of that (It's slightly improved version from another >> thread [1]). > > Thank you for working on this and patches! > >> I split it into several patches: >> >> 0001 - Makes subscription worker exit nicely when there is subscription >> missing (ie was removed while it was starting) and also makes disabled >> message look same as the message when disabled state was detected while >> worker is running. This is mostly just nicer behavior for user (ie no >> unexpected errors in log when you just disabled the subscription). >> >> 0002 - This is bugfix - the sync worker should exit when waiting for >> apply and apply dies otherwise there is possibility of not being >> correctly synchronized. >> >> 0003 - Splits the schizophrenic SetSubscriptionRelState function into >> two which don't try to do broken upsert and report proper errors instead. >> >> 0004 - Solves the issue which Masahiko Sawada reported [2] about ALTER >> SUBSCRIPTION handling of workers not being transactional - we now do >> killing of workers transactionally (and we do the same when possible in >> DROP SUBSCRIPTION). >> >> 0005 - Bugfix to allow syscache access to subscription without being >> connected to database - this should work since subscription is pinned >> catalog, it wasn't caught because nothing in core is using it (yet). >> >> 0006 - Makes the locking of subscriptions more granular (this depends on >> 0005). This removes the ugly AccessExclusiveLock on the pg_subscription >> catalog from DROP SUBSCRIPTION and also solves some potential race >> conditions between launcher, ALTER SUBSCRIPTION and >> process_syncing_tables_for_apply(). Also simplifies memory handling in >> launcher as well as logicalrep_worker_stop() function. This basically >> makes subscriptions behave like every other object in the database in >> terms of locking. >> >> Only the 0002, 0004 and 0005 are actual bug fixes, but I'd still like to >> get it all into PG10 as especially the locking now behaves really >> differently than everything else and that does not seem like a good idea. >> > > I'm now planing to review 0002, 0004 and 0005 patches first as they > are bug fixes. I've reviewed 0002, 0004 and 0005 patches briefly. Here are some comments. -- 0002-Exit-in-sync-worker-if-relation-was-removed-during-s.patch As Petr mentioned, if the table subscription is removed before the table sync worker gets the subscription relation entry, GetSubscriptionRelState could returns SUBREL_STATE_UNKNOWN and this issue happens. Actually we now can handle this case properly by commit 8dc7c338129d22a52d4afcf2f83a73041119efda. So this seems to be an improvement and not be a release blocker. Therefore for this patch, I think it's better to do ereport in the switch(MyLogicalRepWorker->relstate) block. BTW, since missing_ok of GetSubscriptionRelState() is always set to true I think that we can remove it. Also because the reason I mention at later part this fix might not be necessary. -- 0004-Only-kill-sync-workers-at-commit-time-in-SUBSCRIPTIO.patch + /* +* If we are dropping slot, stop all the subscription workers immediately +* so that the slot is accessible, otherwise just shedule the stop at the +* end of the transaction. +* +* New workers won't be started because we hold exclusive lock on the +* subscription till the end of transaction. +*/ + LWLockAcquire(LogicalRepWorkerLock, LW_SHARED); + subworkers = logicalrep_sub_workers_find(subid, false); + LWLockRelease(LogicalRepWorkerLock); + foreach (lc, subworkers) + { + LogicalRepWorker *w = (LogicalRepWorker *) lfirst(lc); + if (sub->slotname) + logicalrep_worker_stop(w->subid, w->relid); + else + logicalrep_worker_stop_at_commit(w->subid, w->relid); + } + list_free(subworkers); I think if we kill the all workers including table sync workers then the fix by 0002 patch is not necessary actually, because the table sync worker will not see that the subscription relation state has been removed. Also, logicalrep_sub_workers_find() function is defined in 0006 patch but it would be better to move it to 0004 patch. -- 0005-Allow-syscache-access-to-subscriptions-in-database-l.patch Do we need to update the comment at the top of IndexScanOK()? To summary, I think we now have only one issue; ALTER SUBSCRIPTION is not transactional, 0004 patch is addressing this issue . 0002 patch seems an improvement patch to me, and it might be resolved by 0004 patch. 0005 patch is required by 0006 patch which is an improvement patch. Am I missing something? Regards, -- Masahiko Sawada
Re: [HACKERS] SCRAM auth and Pgpool-II
On 07/13/17 21:54, Tatsuo Ishii wrote: >>> The comment in pg_hba.conf.sample seem to prefer md5 over clear text >>> password. >>> >>> # Note that "password" sends passwords in clear text; "md5" or >>> # "scram-sha-256" are preferred since they send encrypted passwords. >> >> Should that be reworded to eliminate "md5"? I'd consider "scram-sha-256" >> suitable over a clear channel, but I've never recommended "md5" for that. > > I don't think so unless clear text password is superior than md5. Neither is suitable on an unencrypted channel (as has been repeatedly observed back to 2005 at least [1], so I guess I'm not spilling the beans). At last, scram-sha-256 is an option that is believable for that use. So, allowing that neither "password" nor "md5" should ever be used on an unencrypted channel, as long as the channel is encrypted they are both protected (by the channel encryption) from eavesdropping, so they score a tie on that dimension. For a tiebreaker, you could look at the consequences of revealing rolpassword from pg_authid. On that dimension, with "md5" you have revealed a password-equivalent, while with "password" you have not [2], so on that dimension "password" indeed is superior to "md5". -Chap [1]: https://www.postgresql.org/message-id/8764ygc7i6.fsf%40stark.xeocode.com [2]: https://www.postgresql.org/message-id/20050421190637.GF29028%40ns.snowman.net -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #14634: On Windows pg_basebackup should write tar to stdout in binary mode
On Fri, Jul 14, 2017 at 2:54 AM, Heikki Linnakangaswrote: > On 05/03/2017 07:32 AM, Haribabu Kommi wrote: > >> [Adding -hackers mailing list] >> >> On Fri, Apr 28, 2017 at 6:28 PM, wrote: >> >> The following bug has been logged on the website: >>> >>> Bug reference: 14634 >>> Logged by: Henry Boehlert >>> Email address: henry_boehl...@agilent.com >>> PostgreSQL version: 9.6.2 >>> Operating system: Windows Server 2012 R2 6.3.9600 >>> Description: >>> >>> Executing command pg_basebackup -D -F t writes its output to stdout, >>> which >>> is open in text mode, causing LF to be converted to CR LF thus corrupting >>> the resulting archive. >>> >>> To write the tar to stdout, on Windows stdout's mode should be >>> temporarily >>> switched to binary. >>> >>> https://msdn.microsoft.com/en-us/library/tw4k6df8.aspx >>> >>> >> Thanks for reporting the issue. >> With the attached patch, I was able to extract the tar file that gets >> generated when the tar file is written into stdout. I tested the >> the compressed tar also. >> >> This bug needs to be fixed in back branches also. >> > > Seems reasonable. One question: > > In the patch, you used "_setmode" function, while the calls in > src/bin/pg_dump/pg_backup_archiver.c use "setmode". There are a few > places in the backend that also use "_setmode". What's the difference? > Should we change some of them to be consistent? > Actually there is no functional difference between these two functions. one is a POSIX variant and another one is ISO C++ variant [1]. The support of POSIX variant is deprecated in windows, because of this reason we should use the _setmode instead of setmode. I attached the patch to change the pg_dump code to use _setmode function instead of _setmode to be consistent with other functions. [1] - https://docs.microsoft.com/en-gb/cpp/c-runtime-library/reference/posix-setmode Regards, Hari Babu Fujitsu Australia 0002-Replace-setmode-with-_setmode-function.patch Description: Binary data 0001-pg_basebackup-windows-tar-mode-to-stdout-fix.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] SCRAM auth and Pgpool-II
>> The comment in pg_hba.conf.sample seem to prefer md5 over clear text >> password. >> >> # Note that "password" sends passwords in clear text; "md5" or >> # "scram-sha-256" are preferred since they send encrypted passwords. > > Should that be reworded to eliminate "md5"? I'd consider "scram-sha-256" > suitable over a clear channel, but I've never recommended "md5" for that. I don't think so unless clear text password is superior than md5. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- 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] Inadequate infrastructure for NextValueExpr
On Fri, Jul 14, 2017 at 9:34 AM, Tom Lanewrote: > Somebody decided they could add a new primnode type without bothering to > build out very much infrastructure for it. Thus: > > regression=# create table foo (f1 int, f2 int generated always as identity); > CREATE TABLE > regression=# insert into foo values(1); > INSERT 0 1 > regression=# explain verbose insert into foo values(1); > ERROR: unrecognized node type: 146 > > because ruleutils.c has never heard of NextValueExpr. The lack of > outfuncs/readfuncs support for it is rather distressing as well. > That doesn't break parallel queries today, because (I think) > you can't get one of these nodes in a parallelizable query, but it > is going to cause problems for debugging. It will also break > (more or less) pg_stat_statements. I also wonder whether costsize.c > oughtn't be charging some estimated execution cost for it. I wrote a script to cross-check various node handling functions and it told me: T_NextValueExpr not handled in outfuncs.c T_ObjectWithArgs not handled in outfuncs.c T_AccessPriv not handled in outfuncs.c T_CreateOpClassItem not handled in outfuncs.c T_FunctionParameter not handled in outfuncs.c T_InferClause not handled in outfuncs.c T_OnConflictClause not handled in outfuncs.c T_RoleSpec not handled in outfuncs.c T_PartitionCmd not handled in outfuncs.c T_NamedTuplestoreScan can be produced by outfuncs.c with tagname NAMEDTUPLESTORESCAN but that tagname is not recognized by readfuncs.c T_Alias not handled in ruleutils.c T_RangeVar not handled in ruleutils.c T_Expr not handled in ruleutils.c T_CaseWhen not handled in ruleutils.c T_TargetEntry not handled in ruleutils.c T_RangeTblRef not handled in ruleutils.c T_JoinExpr not handled in ruleutils.c T_FromExpr not handled in ruleutils.c T_OnConflictExpr not handled in ruleutils.c T_IntoClause not handled in ruleutils.c T_NextValueExpr not handled in ruleutils.c It classifies all node types into categories PLAN NODES, PRIMITIVE NODES, ... using the per-group comments in nodes.h, and then checks some rules that I inferred (obviously incorrectly) about which of those categories should be handled by copy, out, equal, read and get_rule_expr functions and also checks that readfuncs.c and outfuncs.c agree on the name string. It needs some work though: anyone got any ideas on how to improve the categories and rules to make it right? To use this approach, it would need to be the case that each checked function exhaustively handles a subset of node tags that can be described by listing those categories. That revealed a defect in commit 18ce3a4ab22d2984f8540ab480979c851dae5338 which I think should be corrected with something like the attached, though I'm not sure if it's possible to reach it. It would be nice to do something more mechanised for this type of code... -- Thomas Munro http://www.enterprisedb.com read-namedtuplestorescan.patch Description: Binary data #!/usr/bin/env python # # Cross-check node handling code in PostgreSQL looking for bugs. import re def scan_node_categories(path): """Find all type tags and put them into categories.""" results = {} with open(path) as file: current_list = [] results["INVALID"] = current_list for line in file: matches = re.search(r"TAGS FOR ([A-Z].*(NODES|STUFF))", line) if matches: category = matches.group(1) current_list = [] results[category] = current_list continue matches = re.search(r"(T_[a-zA-Z0-9_]+)", line) if matches: assert current_list != None tag = matches.group(1) current_list.append(tag) return results def scan_switch(path, function): """Find all cases handled by "function" in translation unit "path".""" cases = [] in_function = False with open(path) as file: for line in file: if line.startswith(function + "("): in_function = True elif line.startswith("}"): in_function = False elif in_function: matches = re.search(r"case (T_.+):", line) if matches: cases.append(matches.group(1)) return cases def scan_read_tagnames(path): """Find all tagnames that readfuncs.c recognizes.""" results = [] with open(path) as file: for line in file: matches = re.search("MATCH\(\"([A-Z]+)\", ([0-9]+)", line) if matches: tagname = matches.group(1) length = int(matches.group(2)) results.append(tagname) if length != len(tagname): print "Unexpected length:", line return results def scan_out_tagnames(path): """Learn what tagname outfuncs.c uses to output each type.""" results = {} current_tag = None with open(path) as file: for line in file: matches = re.match(r"_out.*, const ([^ ]+) \*", line) if matches: # see makeNode macro definition: typedef Foo must have enum T_Foo current_tag = "T_" + matches.group(1) elif line.startswith("}"):
Re: [HACKERS] SCRAM auth and Pgpool-II
On 07/13/17 20:09, Tatsuo Ishii wrote: > The comment in pg_hba.conf.sample seem to prefer md5 over clear text > password. > > # Note that "password" sends passwords in clear text; "md5" or > # "scram-sha-256" are preferred since they send encrypted passwords. Should that be reworded to eliminate "md5"? I'd consider "scram-sha-256" suitable over a clear channel, but I've never recommended "md5" for that. -Chap -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Fwd: [HACKERS] Vacuum: allow usage of more than 1GB of work mem
On Wed, Jul 12, 2017 at 1:29 PM, Claudio Freirewrote: > On Wed, Jul 12, 2017 at 1:08 PM, Claudio Freire > wrote: >> On Wed, Jul 12, 2017 at 11:48 AM, Alexey Chernyshov >> wrote: >>> Thank you for the patch and benchmark results, I have a couple remarks. >>> Firstly, padding in DeadTuplesSegment >>> >>> typedef struct DeadTuplesSegment >>> >>> { >>> >>> ItemPointerData last_dead_tuple;/* Copy of the last dead tuple >>> (unset >>> >>> * until the segment is fully >>> >>> * populated). Keep it first to >>> simplify >>> >>> * binary searches */ >>> >>> unsigned short padding;/* Align dt_tids to 32-bits, >>> >>> * sizeof(ItemPointerData) is aligned to >>> >>> * short, so add a padding short, to make >>> the >>> >>> * size of DeadTuplesSegment a multiple of >>> >>> * 32-bits and align integer components for >>> >>> * better performance during lookups into >>> the >>> >>> * multiarray */ >>> >>> intnum_dead_tuples;/* # of entries in the segment */ >>> >>> intmax_dead_tuples;/* # of entries allocated in the >>> segment */ >>> >>> ItemPointer dt_tids;/* Array of dead tuples */ >>> >>> }DeadTuplesSegment; >>> >>> In the comments to ItemPointerData is written that it is 6 bytes long, but >>> can be padded to 8 bytes by some compilers, so if we add padding in a >>> current way, there is no guaranty that it will be done as it is expected. >>> The other way to do it with pg_attribute_alligned. But in my opinion, there >>> is no need to do it manually, because the compiler will do this optimization >>> itself. >> >> I'll look into it. But my experience is that compilers won't align >> struct size like this, only attributes, and this attribute is composed >> of 16-bit attributes so it doesn't get aligned by default. > > Doing sizeof(DeadTuplesSegment) suggests you were indeed right, at > least in GCC. I'll remove the padding. > > Seems I just got the wrong impression at some point. Updated versions of the patches attached. A few runs of the benchmark show no significant difference, as it should (being all cosmetic changes). The bigger benchmark will take longer. From b2bf9a671c2c4d517d59628f12a7f564ccf152f6 Mon Sep 17 00:00:00 2001 From: Claudio Freire Date: Mon, 12 Sep 2016 23:36:42 -0300 Subject: [PATCH 1/2] Vacuum: allow using more than 1GB work mem Turn the dead_tuples array into a structure composed of several exponentially bigger arrays, to enable usage of more than 1GB of work mem during vacuum and thus reduce the number of full index scans necessary to remove all dead tids when the memory is available. Improve test ability to spot vacuum errors --- src/backend/commands/vacuumlazy.c| 402 --- src/test/regress/expected/vacuum.out | 26 +++ src/test/regress/sql/vacuum.sql | 19 ++ 3 files changed, 373 insertions(+), 74 deletions(-) diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c index 30a0050..287accd 100644 --- a/src/backend/commands/vacuumlazy.c +++ b/src/backend/commands/vacuumlazy.c @@ -11,11 +11,24 @@ * * We are willing to use at most maintenance_work_mem (or perhaps * autovacuum_work_mem) memory space to keep track of dead tuples. We - * initially allocate an array of TIDs of that size, with an upper limit that + * initially allocate an array of TIDs of 128MB, or an upper limit that * depends on table size (this limit ensures we don't allocate a huge area - * uselessly for vacuuming small tables). If the array threatens to overflow, + * uselessly for vacuuming small tables). Additional arrays of increasingly + * large sizes are allocated as they become necessary. + * + * The TID array is thus represented as a list of multiple segments of + * varying size, beginning with the initial size of up to 128MB, and growing + * exponentially until the whole budget of (autovacuum_)maintenance_work_mem + * is used up. + * + * Lookup in that structure happens with a binary search of segments, and then + * with a binary search within each segment. Since segment's size grows + * exponentially, this retains O(log N) lookup complexity. + * + * If the multiarray's total size threatens to grow beyond maintenance_work_mem, * we suspend the heap scan phase and perform a pass of index cleanup and page - * compaction, then resume the heap scan with an empty TID array. + * compaction, then resume the heap scan with an array of logically empty but + * already preallocated TID segments to be refilled with more dead tuple TIDs. * * If we're
Re: [HACKERS] Update description of \d[S+] in \?
On 2017/07/13 19:57, Ashutosh Bapat wrote: > On Thu, Jul 13, 2017 at 12:01 PM, Amit Langote >wrote: >> The description of \d[S+] currently does not mention that it will list >> materialized views and foreign tables. Attached fixes that. >> > > I guess the same change is applicable to the description of \d[S+] NAME as > well. Thanks for the review. Fixed in the attached. Thanks, Amit diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index b3dbb5946e..3fad56109b 100644 --- a/src/bin/psql/help.c +++ b/src/bin/psql/help.c @@ -219,8 +219,8 @@ slashUsage(unsigned short int pager) fprintf(output, _("Informational\n")); fprintf(output, _(" (options: S = show system objects, + = additional detail)\n")); - fprintf(output, _(" \\d[S+] list tables, views, and sequences\n")); - fprintf(output, _(" \\d[S+] NAME describe table, view, sequence, or index\n")); + fprintf(output, _(" \\d[S+] list tables, views, materialized views, sequences, and foreign tables\n")); + fprintf(output, _(" \\d[S+] NAME describe table, view, materialized view, sequence, index, or foreign table\n")); fprintf(output, _(" \\da[S] [PATTERN] list aggregates\n")); fprintf(output, _(" \\dA[+] [PATTERN] list access methods\n")); fprintf(output, _(" \\db[+] [PATTERN] list tablespaces\n")); -- 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 auth and Pgpool-II
>> Using a clear text password would not be acceptable for users even >> through an encrypted connection, I think. > > Really, I don't think users who are concerned with security should be > using the md5 method either. The comment in pg_hba.conf.sample seem to prefer md5 over clear text password. # Note that "password" sends passwords in clear text; "md5" or # "scram-sha-256" are preferred since they send encrypted passwords. > What would be really nice for such cases is support for Kerberos and > delegated Kerberos credentials. Having pgpool support that would remove > the need to deal with passwords at all. > > Ditto for having postgres_fdw support same. More often than not, > Kerberos deployments (via AD, generally) is what I find in the > enterprises that I work with and they're happy to see we have Kerberos > but it's disappointing when they can't use Kerberos with either > connection poolers or with FDWs. I would add supporting Kerberos to the Pgpool-II todo list. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- 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_stop_backup(wait_for_archive := true) on standby server
Michael, all, * Michael Paquier (michael.paqu...@gmail.com) wrote: > On Thu, Jul 13, 2017 at 7:13 AM, Masahiko Sawada> wrote: > > Sorry, I missed lots of typo in the last patch. All comments from you > > are incorporated into the attached latest patch and I've checked it > > whether there is other typos. Please review it. > > Thanks for providing a new version of the patch very quickly. I have > spent some time looking at it again and testing it, and this version > looks correct to me. Stephen, as a potential committer, would you look > at it to address this open item? Yes, this weekend. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] SCRAM auth and Pgpool-II
Greetings Tatsuo, * Tatsuo Ishii (is...@sraoss.co.jp) wrote: > > What I am suggesting here is that in order to handle properly SCRAM > > with channel binding, pgpool has to provide a different handling for > > client <-> pgpool and pgpool <-> Postgres. In short, I don't have a > > better answer than having pgpool impersonate the server and request > > for a password in cleartext through an encrypted connection between > > pgpool and the client if pgpool does not know about it, and then let > > pgpool do by itself the SCRAM authentication on a per-connection basis > > with each Postgres instances. When using channel binding, what would > > matter is the TLS finish (for tls-unique) or the hash server > > certificate between Postgres and pgpool, not between the client and > > pgpool. But that's actually the point you are raising here: > > Using a clear text password would not be acceptable for users even > through an encrypted connection, I think. Really, I don't think users who are concerned with security should be using the md5 method either. What would be really nice for such cases is support for Kerberos and delegated Kerberos credentials. Having pgpool support that would remove the need to deal with passwords at all. Ditto for having postgres_fdw support same. More often than not, Kerberos deployments (via AD, generally) is what I find in the enterprises that I work with and they're happy to see we have Kerberos but it's disappointing when they can't use Kerberos with either connection poolers or with FDWs. Thanks! Stephen signature.asc Description: Digital signature
[HACKERS] Inadequate infrastructure for NextValueExpr
Somebody decided they could add a new primnode type without bothering to build out very much infrastructure for it. Thus: regression=# create table foo (f1 int, f2 int generated always as identity); CREATE TABLE regression=# insert into foo values(1); INSERT 0 1 regression=# explain verbose insert into foo values(1); ERROR: unrecognized node type: 146 because ruleutils.c has never heard of NextValueExpr. The lack of outfuncs/readfuncs support for it is rather distressing as well. That doesn't break parallel queries today, because (I think) you can't get one of these nodes in a parallelizable query, but it is going to cause problems for debugging. It will also break (more or less) pg_stat_statements. I also wonder whether costsize.c oughtn't be charging some estimated execution cost for it. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] CAST vs ::
"David G. Johnston"writes: > On Thursday, July 13, 2017, Tom Lane wrote: >> Maybe we can hack ruleutils to use >> the CAST syntax only in this specific context. > Given the lack of complaints, and ubiquity of ::, this would seem ideal > and sufficient. While there is something to be said for using standard > compliant syntax changing just this like doesn't seem like it would move > the goalposts meaningfully. Hm, it's worse than I thought: the argument of the CAST expression needn't be a function call at all, and on top of that, the parser will throw away a no-op cast altogether. So for example: regression=# create view vvc as select * from cast(1+2 as int) c(x); CREATE VIEW regression=# \d+ vvc View "public.vvc" Column | Type | Collation | Nullable | Default | Storage | Description +-+---+--+-+-+- x | integer | | | | plain | View definition: SELECT c.x FROM 1 + 2 c(x); To make the world safe for this behavior by extending the grammar, we'd have to be prepared to accept an arbitrary a_expr, without even surrounding parentheses, as a FROM item. I don't think there's much chance of making that work without grammar conflicts, and even if we managed, the SQL committee would probably find a way to break it with some future feature addition. So what I'm now thinking is to make ruleutils.c look at the expression in an RTE_FUNCTION FROM item and see if it will decompile as something with the syntax of a function call. If not, or if there's any doubt, emit a dummy CAST(... AS sametype) around it. That would cause the above example to come out the way it went in. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] Zipfian distribution in pgbench
On Thu, Jul 13, 2017 at 12:49 PM, Peter Geogheganwrote: > To reiterate what I say above: > > The number of leaf pages with dead items is 20 with this most recent > run (128 clients, patched + unpatched). The leftmost internal page one > level up from the leaf level contains 289 items. Whereas last time it > was 353 items. > > That's a difference between having 20 hot/bloated leaf pages, and > probably 84 hot/bloated pages, which I infer must have been the total > number of bloated leaf pages within "result.txt". I think that > something about all the "pgbench_index_*txt" tests are very different > to what we see within "result.txt". It's as if there was a problem > when "result.txt" ran, but that problem somehow didn't come up when > you did new tests. I just figured out that "result.txt" is only a 60 second pgbench run. Is the same true for other tests? It would be much more interesting to see runs that lasted 10 minutes or more. Maybe with pgbench-tools. I would expect that a decline in performance due to bloating the index could happen only after a certain threshold was crossed. Things like HOT pruning and LP_DEAD cleanup could be pretty effective, until finally a tipping point is reached and they're much less effective. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PostgreSQL - Weak DH group
On 07/13/2017 10:13 PM, Robert Haas wrote: On Thu, Jul 13, 2017 at 1:30 PM, Tom Lanewrote: Heikki Linnakangas writes: I don't think this can be backpatched. It changes the default DH parameters from 1024 bits to 2048 bits. That's a good thing for security, but older clients might not support it, and would refuse to connect or would fall back to something less secure. Do we have any hard information about which versions of which clients might not support that? (In particular I'm wondering if any still exist in the wild.) Yeah. If we break clients for v10 two months from release, some drivers won't be updated by release time, and that sounds pretty unfriendly to me. On the other hand, if there is only a theoretical risk of breakage and no clients that we actually know about will have a problem with it, then the argument for waiting is weaker. I'm not generally very excited about changing things after beta2, which is where are, but if this is a security issue then we might need to hold our nose and go ahead. I'm against it if it's likely to cause real-world connectivity problems, though. Googling around, I believe Java 6 is the only straggler [1]. So we would be breaking that. Java 7 also doesn't support DH parameters > 1024 bits, but it supports ECDHE, which is prioritized over DH ciphers, so it doesn't matter. Java 6 was released back in 2006. The last public release was in 2013. It wouldn't surprise me to still see it bundled with random proprietary software packages, though. The official PostgreSQL JDBC driver still supports it, but there has been discussion recently on dropping support for it, and even for Java 7. [2] I would be OK with breaking DH with Java 6 in PostgreSQL 10, especially since there's a simple workaround (generate a 1024-bit DH parameters file). I would be less enthusiastic about doing that in a minor release, although maybe that wouldn't be too bad either, if we put a prominent notice with the workaround in the release notes. [1] https://wiki.mozilla.org/Security/Server_Side_TLS#DHE_and_ECDHE_support [2] https://www.postgresql.org/message-id/69ae857b-15cc-36dd-f380-6620ef1effb9%408kdata.com - Heikki -- 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] pl/perl extension fails on Windows
Re: Dave Page 2017-07-12> > Well, we have various buildfarm machines running perls newer than that, > > eg, crake, with 5.24.1. So I'd say there is something busted about your > > perl installation. Perhaps leftover bits of an older version somewhere? > > > > Well crake is a Fedora box - and we have no problems on Linux, only on > Windows. The plperl segfault on Debian's kfreebsd port I reported back in 2013 is also still present: https://www.postgresql.org/message-id/20130515064201.GC704%40msgid.df7cb.de https://buildd.debian.org/status/fetch.php?pkg=postgresql-10=kfreebsd-amd64=10~beta2-1=1499947011=0 (Arguably, this is a toy architecture, so we can just leave it unfixed there without any harm...) Christoph -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] Zipfian distribution in pgbench
On Thu, Jul 13, 2017 at 10:02 AM, Peter Geogheganwrote: > The number of leaf pages at the left hand side of the leaf level seems > to be ~50 less than the unpatched 128 client case was the first time > around, which seems like a significant difference. I wonder why. Maybe > autovacuum ran at the right/wrong time this time around? To reiterate what I say above: The number of leaf pages with dead items is 20 with this most recent run (128 clients, patched + unpatched). The leftmost internal page one level up from the leaf level contains 289 items. Whereas last time it was 353 items. That's a difference between having 20 hot/bloated leaf pages, and probably 84 hot/bloated pages, which I infer must have been the total number of bloated leaf pages within "result.txt". I think that something about all the "pgbench_index_*txt" tests are very different to what we see within "result.txt". It's as if there was a problem when "result.txt" ran, but that problem somehow didn't come up when you did new tests. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Domains and arrays and composites, oh my
"David G. Johnston"writes: > On Thursday, July 13, 2017, Tom Lane wrote: >> regression=# select * from fdc(); >> fdc >> --- >> (1,2) >> (1 row) > Select (fdc).* from fdc(); is considerably more intuitive that the cast. > Does that give the expected multi-column result? Yeah, it does, although I'm not sure how intuitive it is that the parentheses are significant ... regression=# select * from fdc(); fdc --- (1,2) (1 row) regression=# select fdc from fdc(); fdc --- (1,2) (1 row) regression=# select fdc.* from fdc(); fdc --- (1,2) (1 row) regression=# select (fdc).* from fdc(); r | i ---+--- 1 | 2 (1 row) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Domains and arrays and composites, oh my
On Thursday, July 13, 2017, Tom Lanewrote: > > regression=# select * from fdc(); > fdc > --- > (1,2) > (1 row) > > Select (fdc).* from fdc(); is considerably more intuitive that the cast. Does that give the expected multi-column result? David J.
Re: [HACKERS] CAST vs ::
On Thursday, July 13, 2017, Tom Lanewrote: > Maybe we can hack ruleutils to use > the CAST syntax only in this specific context. > Given the lack of complaints, and ubiquity of ::, this would seem ideal and sufficient. While there is something to be said for using standard compliant syntax changing just this like doesn't seem like it would move the goalposts meaningfully. David J.
Re: [HACKERS] Domains and arrays and composites, oh my
I wrote: > I started to look into allowing domains over composite types, which is > another never-implemented case that there's no very good reason not to > allow. Well, other than the argument that the SQL standard only allows > domains over "predefined" (built-in) types ... but we blew past that > restriction ages ago. Attached is a draft patch that allows domains over composite types. I think it's probably complete on its own terms, but there are some questions around behavior of functions returning domain-over-composite that could use discussion, and some of the PLs need some follow-on work. The core principle here was to not modify execution-time behavior by adding domain checks to existing code paths. Rather, I wanted the parser to insert CoerceToDomain nodes wherever checks would be needed. Row-returning node types such as RowExpr and FieldStore only return regular composite types, as before; to generate a value of a composite domain, we construct a value of the base type and then CoerceToDomain. (This is pretty analogous to what happens for domains over arrays.) Whole-row Vars can only have regular composite types as vartype, TupleDescs can only have regular composite types as tdtypeid, composite Datums only have regular composite type OIDs in datum_typeid. (The last would be expected anyway, since the physical representation of a domain value is never different from that of its base type.) Doing it that way led to a nicely small patch, only about 160 net new lines of code. However, not touching the executor meant not touching the behavior of functions that return domains, even if the domain is domain-over-composite. In code terms this means that get_call_result_type() and sibling functions will return TYPEFUNC_SCALAR not TYPEFUNC_COMPOSITE for such a result type. The difficulty with changing that is that if these functions look through the domain, then the calling code (in, usually, a PL) will simply build and return a result of the underlying composite type, failing to apply any domain constraints. Trying to get out-of-core PLs on board with a change in those requirements seems like a risky proposition. Concretely, consider create type complex as (r float8, i float8); create domain dcomplex as complex; You can make a SQL-language function to return complex in either of two ways: create function fc() returns complex language sql as 'select 1.0::float8, 2.0::float8'; create function fc() returns complex language sql as 'select row(1.0::float8, 2.0::float8)::complex'; As the patch stands, though, only the second way works for domains over composite: regression=# create function fdc() returns dcomplex language sql as 'select 1.0::float8, 2.0::float8'; ERROR: return type mismatch in function declared to return dcomplex DETAIL: Final statement must return exactly one column. CONTEXT: SQL function "fdc" regression=# create function fdc() returns dcomplex language sql as 'select row(1.0::float8, 2.0)::dcomplex'; CREATE FUNCTION Now, maybe that's fine. SQL-language functions have never been very willing to insert implicit casts to get to the function result type, and certainly the only way that the first definition could be legal is if there were an implicit up-cast to the domain type. It might be OK to just leave it like this, though some documentation about it would be a good idea. plpgsql functions seem generally okay as far as composite domain return types go, because they don't have anything corresponding to the row return convention of SQL functions. And plpgsql's greater willingness to do implicit coercions reduces the notational burden, too. But there's some work yet to be done to get plpgsql to realize that composite domain local variables have substructure. For example, this works: declare x complex; ... x.r := 1; but it fails if x is dcomplex. But ISTM that that would be better handled as a followon feature patch. I suspect that the other PLs may have similar issues where it'd be nice to allow domain-over-composite to act like a plain composite for specific purposes; but I've not looked. Another issue related to function result types is that the parser considers a function-in-FROM returning a composite domain to be producing a scalar result not a rowtype. Thus you get this for a function returning complex: regression=# select * from fc(); r | i ---+--- 1 | 2 (1 row) but this for a function returning dcomplex: regression=# select * from fdc(); fdc --- (1,2) (1 row) I think that that could be changed with only local changes in parse analysis, but do we want to change it? Arguably, making fdc() act the same as fc() here would amount to implicitly downcasting the domain to its base type. But doing so here is optional, not necessary in order to make the statement sane at all, and it's arguable that we shouldn't do that if the user didn't tell us to. A user who does want that to happen can downcast explicitly: regression=#
Re: [HACKERS] PostgreSQL - Weak DH group
On Thu, Jul 13, 2017 at 1:30 PM, Tom Lanewrote: > Heikki Linnakangas writes: >> I don't think this can be backpatched. It changes the default DH >> parameters from 1024 bits to 2048 bits. That's a good thing for >> security, but older clients might not support it, and would refuse to >> connect or would fall back to something less secure. > > Do we have any hard information about which versions of which clients > might not support that? (In particular I'm wondering if any still exist > in the wild.) Yeah. If we break clients for v10 two months from release, some drivers won't be updated by release time, and that sounds pretty unfriendly to me. On the other hand, if there is only a theoretical risk of breakage and no clients that we actually know about will have a problem with it, then the argument for waiting is weaker. I'm not generally very excited about changing things after beta2, which is where are, but if this is a security issue then we might need to hold our nose and go ahead. I'm against it if it's likely to cause real-world connectivity problems, though. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PostgreSQL - Weak DH group
Heikki Linnakangaswrites: > I don't think this can be backpatched. It changes the default DH > parameters from 1024 bits to 2048 bits. That's a good thing for > security, but older clients might not support it, and would refuse to > connect or would fall back to something less secure. Do we have any hard information about which versions of which clients might not support that? (In particular I'm wondering if any still exist in the wild.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] CAST vs ::
In most places, you can write CAST(x AS t) and x::t interchangeably. But that doesn't work for function-in-FROM. This is OK: select * from cast(fdc() as complex); but this is not: select * from fdc()::complex; ERROR: syntax error at or near "::" I just realized that this is a problem for ruleutils.c, which thinks it can always use the short form: regression=# create view vv as select * from cast(fdc() as complex); CREATE VIEW regression=# \d+ vv View "public.vv" Column | Type | Collation | Nullable | Default | Storage | Descript ion +--+---+--+-+-+- r | double precision | | | | plain | i | double precision | | | | plain | View definition: SELECT fdc.r, fdc.i FROM fdc()::complex fdc(r, i); That view definition will not reload. Not sure about the most reasonable fix. It might be possible to tweak the grammar to allow this case, but I'm not at all sure about that. An easy fix would be to make ruleutils print casts as CAST() all the time, but that would probably annoy a lot of people (it'd certainly break a lot of regression tests). Maybe we can hack ruleutils to use the CAST syntax only in this specific context. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fixup some misusage of appendStringInfo and friends
On 04/27/2017 03:14 AM, David Rowley wrote: On 27 April 2017 at 06:41, Peter Eisentrautwrote: On 4/19/17 08:42, Ashutosh Bapat wrote: I reviewed the patch. It compiles clean, make check-world passes. I do not see any issue with it. Looks reasonable. Let's keep it for the next commit fest. Thank you to both of you for looking. I'd thought that maybe the new stuff in PG10 should be fixed before the release. If we waited, and fix in PG11 then backpatching is more of a pain. However, I wasn't careful in the patch to touch only new to PG10 code. I'll defer to your better judgment and add to the next 'fest. I think that's a very good argument. Cleaning up code that's new in this version seems like a fair game, and a good idea. The places that are not new in PostgreSQL 10 are more questionable, but seems harmless enough anyway. Did you have an outright objection to this, Peter? The patch looks good to me at a quick glance, I think we should commit this now. - Heikki -- 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] PostgreSQL - Weak DH group
On 07/13/2017 01:07 PM, Simon Riggs wrote: > On 13 July 2017 at 16:32, Heikki Linnakangaswrote: >> (We dropped the ball back in October, continuing the discussion now) >> >> On 10/10/2016 06:24 PM, Heikki Linnakangas wrote: >>> >>> On 10/06/2016 10:26 PM, Christoph Berg wrote: Re: Heikki Linnakangas 2016-10-06 > > I propose the attached patch. It gives up on trying to deal with > multiple > key lengths (as noted earlier, OpenSSL just always passed > keylength=1024, so > that was useless). Instead of using the callback, it just sets fixed DH > parameters with SSL_CTX_set_tmp_dh(), like we do for the ECDH curve. The > DH > parameters are loaded from a file called "dh_params.pem" (instead of > "dh1024.pem"), if present, otherwise the built-in 2048 bit parameters > are > used. Shouldn't this be a GUC pointing to a configurable location like ssl_cert_file? This way, people reading the security section of the default postgresql.conf would notice that there's something (new) to configure. (And I wouldn't want to start creating symlinks from PGDATA to /etc/ssl/something again...) >>> >>> >>> Perhaps. The DH parameters are not quite like other configuration files, >>> though. The actual parameters used don't matter, you just want to >>> generate different parameters for every cluster. The key length of the >>> parameters can be considered configuration, though. >> >> >> Actually, adding a GUC also solves another grief I had about this. There is >> currently no easy way to tell if your parameter file is being used, or if >> the server is failing to read it and is falling back to the hard-coded >> parameters. With a GUC, if the GUC is set it's a good indication that the >> admin expects the file to really exist and to contain valid parameters. So >> if the GUC is set, we should throw an error if it cannot be used. And if >> it's not set, we use the built-in defaults. >> >> I rebased the patch, did some other clean up of error reporting, and added a >> GUC along those lines, as well as docs. How does this look? >> >> It's late in the release cycle, but it would be nice to sneak this into v10. >> Using weak 1024 bit DH parameters is arguably a security issue; it was >> originally reported as such. There's a work-around for older versions: >> generate custom 2048 bit parameters and place them in a file called >> "dh1024.pem", but that's completely undocumented. >> >> Thoughts? Objections to committing this now, instead of waiting for v11? > > +1 to include important open items such as this I have not looked at the patch itself, but conceptually +1 to include in pg10. -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: [HACKERS] PostgreSQL - Weak DH group
On 07/13/2017 08:04 PM, Alvaro Herrera wrote: Michael Paquier wrote: On Thu, Jul 13, 2017 at 5:32 PM, Heikki Linnakangaswrote: Objections to committing this now, instead of waiting for v11? But I am -1 for the sneak part. It is not the time to have a new feature in 10, the focus is to stabilize. But if we were treating it as a security issue, would we backpatch it? If we do, then it definitely makes sense to put something in pg10. I'm not sure that this patch is it, though -- perhaps it makes sense to put a minimal fix in older branches, and let the new feature wait for pg11? I don't think this can be backpatched. It changes the default DH parameters from 1024 bits to 2048 bits. That's a good thing for security, but older clients might not support it, and would refuse to connect or would fall back to something less secure. I don't think there are many such clients around anymore, but it's nevertheless not something we want to do in a stable release I think the best we can do is to document the issue and the workaround. To recap, to use stronger DH parameters in stable versions, you need to do "openssl dhparam -out $PGDATA/dh1024.pem 2048". But I'd like to take the opportunity to change this for new installations, with v10, instead of waiting for another year. Of course, you could say that for any new feature, too, but that doesn't necessarily mean that it's a bad argument :-). It's a judgment call, for sure. - Heikki -- 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] PostgreSQL - Weak DH group
Re: Alvaro Herrera 2017-07-13 <20170713170402.74uuoivrgd3c6tnw@alvherre.pgsql> > > > Objections to committing this now, instead of waiting for v11? > > > > But I am -1 for the sneak part. It is not the time to have a new > > feature in 10, the focus is to stabilize. > > But if we were treating it as a security issue, would we backpatch it? > If we do, then it definitely makes sense to put something in pg10. I'm > not sure that this patch is it, though -- perhaps it makes sense to put > a minimal fix in older branches, and let the new feature wait for pg11? Making it user-configurable seems pretty minimal to me. Everything else would probably require lengthy explanations about which file could hold which contents, and this confusion seems to be part of the problem. Fwiw, wouldn't it make sense to recreate the default 2048 DH group as well, maybe each time a new major is branched? Christoph -- 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] UPDATE of partition key
On 5 July 2017 at 15:12, Amit Khandekarwrote: > Like I mentioned upthread... in expand_inherited_rtentry(), if we > replace find_all_inheritors() with something else that returns oids in > canonical order, that will change the order in which children tables > get locked, which increases the chance of deadlock. Because, then the > callers of find_all_inheritors() will lock them in one order, while > callers of expand_inherited_rtentry() will lock them in a different > order. Even in the current code, I think there is a chance of > deadlocks because RelationGetPartitionDispatchInfo() and > find_all_inheritors() have different lock ordering. > > Now, to get the oids of a partitioned table children sorted by > canonical ordering, (i.e. using the partition bound values) we need to > either use the partition bounds to sort the oids like the way it is > done in RelationBuildPartitionDesc() or, open the parent table and get > it's Relation->rd_partdesc->oids[] which are already sorted in > canonical order. So if we generate oids using this way in > find_all_inheritors() and find_inheritance_children(), that will > generate consistent ordering everywhere. But this method is quite > expensive as compared to the way oids are generated and sorted using > oid values in find_inheritance_children(). > > In both expand_inherited_rtentry() and > RelationGetPartitionDispatchInfo(), each of the child tables are > opened. > > So, in both of these functions, what we can do is : call a new > function partition_tree_walker() which does following : > 1. Lock the children using the existing order (i.e. sorted by oid > values) using the same function find_all_inheritors(). Rename > find_all_inheritors() to lock_all_inheritors(... , bool return_oids) > which returns the oid list only if requested. > 2. And then scan through each of the partitions in canonical order, by > opening the parent table, then opening the partition descriptor oids, > and then doing whatever needs to be done with that partition rel. > > partition_tree_walker() will look something like this : > > void partition_tree_walker(Oid parentOid, LOCKMODE lockmode, >void (*walker_func) (), void *context) > { > Relation parentrel; > List *rels_list; > ListCell *cell; > > (void) lock_all_inheritors(parentOid, lockmode, >false /* don't generate oids */); > > parentrel = heap_open(parentOid, NoLock); > rels_list = append_rel_partition_oids(NIL, parentrel); > > /* Scan through all partitioned rels, and at the > * same time append their children. */ > foreach(cell, rels_list) > { > /* Open partrel without locking; lock_all_inheritors() has locked it > */ > Relationpartrel = heap_open(lfirst_oid(cell), NoLock); > > /* Append the children of a partitioned rel to the same list > * that we are iterating on */ > if (RelationGetPartitionDesc(partrel)) > rels_list = append_rel_partition_oids(rels_list, partrel); > > /* > * Do whatever processing needs to be done on this partel. > * The walker function is free to either close the partel > * or keep it opened, but it needs to make sure the opened > * ones are closed later > */ > walker_func(partrel, context); > } > } > > List *append_rel_partition_oids(List *rel_list, Relation rel) > { > int i; > for (i = 0; i < rel->rd_partdesc->nparts; i++) > rel_list = lappend_oid(rel_list, rel->rd_partdesc->oids[i]); > > return rel_list; > } > > > So, in expand_inherited_rtentry() the foreach(l, inhOIDs) loop will be > replaced by partition_tree_walker(parentOid, expand_rte_walker_func) > where expand_rte_walker_func() will do all the work done in the for > loop for each of the partition rels. > > Similarly, in RelationGetPartitionDispatchInfo() the initial part > where it uses APPEND_REL_PARTITION_OIDS() can be replaced by > partition_tree_walker(rel, dispatch_info_walkerfunc) where > dispatch_info_walkerfunc() will generate the oids, or may be populate > the complete PartitionDispatchData structure. 'pd' variable can be > passed as context to the partition_tree_walker(..., context) > > Generating the resultrels in canonical order by opening the tables > using the above way wouldn't be more expensive than the existing code, > because even currently we anyways have to open all the tables in both > of these functions. > Attached is a WIP patch (make_resultrels_ordered.patch) that generates the result rels in canonical order. This patch is kept separate from the update-partition-key patch, and can be applied on master branch. In this patch, rather than partition_tree_walker() called with a context, I have provided a function partition_walker_next() using which we iterate over all the partitions in canonical order. partition_walker_next() will take care of appending oids from partition
Re: [HACKERS] PostgreSQL - Weak DH group
On 13 July 2017 at 16:32, Heikki Linnakangaswrote: > (We dropped the ball back in October, continuing the discussion now) > > On 10/10/2016 06:24 PM, Heikki Linnakangas wrote: >> >> On 10/06/2016 10:26 PM, Christoph Berg wrote: >>> >>> Re: Heikki Linnakangas 2016-10-06 >>> I propose the attached patch. It gives up on trying to deal with multiple key lengths (as noted earlier, OpenSSL just always passed keylength=1024, so that was useless). Instead of using the callback, it just sets fixed DH parameters with SSL_CTX_set_tmp_dh(), like we do for the ECDH curve. The DH parameters are loaded from a file called "dh_params.pem" (instead of "dh1024.pem"), if present, otherwise the built-in 2048 bit parameters are used. >>> >>> >>> Shouldn't this be a GUC pointing to a configurable location like >>> ssl_cert_file? This way, people reading the security section of the >>> default postgresql.conf would notice that there's something (new) to >>> configure. (And I wouldn't want to start creating symlinks from PGDATA >>> to /etc/ssl/something again...) >> >> >> Perhaps. The DH parameters are not quite like other configuration files, >> though. The actual parameters used don't matter, you just want to >> generate different parameters for every cluster. The key length of the >> parameters can be considered configuration, though. > > > Actually, adding a GUC also solves another grief I had about this. There is > currently no easy way to tell if your parameter file is being used, or if > the server is failing to read it and is falling back to the hard-coded > parameters. With a GUC, if the GUC is set it's a good indication that the > admin expects the file to really exist and to contain valid parameters. So > if the GUC is set, we should throw an error if it cannot be used. And if > it's not set, we use the built-in defaults. > > I rebased the patch, did some other clean up of error reporting, and added a > GUC along those lines, as well as docs. How does this look? > > It's late in the release cycle, but it would be nice to sneak this into v10. > Using weak 1024 bit DH parameters is arguably a security issue; it was > originally reported as such. There's a work-around for older versions: > generate custom 2048 bit parameters and place them in a file called > "dh1024.pem", but that's completely undocumented. > > Thoughts? Objections to committing this now, instead of waiting for v11? +1 to include important open items such as this -- Simon Riggshttp://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] 10beta1 sequence regression failure on sparc64
Re: To Andres Freund 2017-05-24 <20170524170921.7pykzbt54dlfk...@msg.df7cb.de> > > > If we had a typo or something in that code, the build farm should have > > > caught it by now. > > > > > > I would try compiling with lower -O and see what happens. > > > > Trying -O0 now. > > Sorry for the late reply, I was hoping to catch you on IRC, but then > didn't manage to be around at a time that would fit the timezone > shift. > > The -O0 build passed the regression tests. Not sure what that means > for our problem, though. Fwiw, the problem is fixed with beta2, even with -O2: https://buildd.debian.org/status/logs.php?pkg=postgresql-10=sparc64 Christoph -- 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] PostgreSQL - Weak DH group
Michael Paquier wrote: > On Thu, Jul 13, 2017 at 5:32 PM, Heikki Linnakangaswrote: > > Objections to committing this now, instead of waiting for v11? > > But I am -1 for the sneak part. It is not the time to have a new > feature in 10, the focus is to stabilize. But if we were treating it as a security issue, would we backpatch it? If we do, then it definitely makes sense to put something in pg10. I'm not sure that this patch is it, though -- perhaps it makes sense to put a minimal fix in older branches, and let the new feature wait for pg11? -- Álvaro Herrerahttps://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] [WIP] Zipfian distribution in pgbench
On Thu, Jul 13, 2017 at 4:38 AM, Alik Khilazhevwrote: > I am attaching results of test for 32 and 128 clients for original and > patched(_bt_doinsert) variants. Thanks. The number of leaf pages at the left hand side of the leaf level seems to be ~50 less than the unpatched 128 client case was the first time around, which seems like a significant difference. I wonder why. Maybe autovacuum ran at the right/wrong time this time around? Did you happen to record TPS for this most recent set of tests? I notice one possibly interesting thing from these new numbers: the 32 client case is slightly more bloated when unique index enforcement is removed. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pl/perl extension fails on Windows
Andrew Dunstanwrites: > It would be nice to get to the bottom of why we're getting a version > mismatch on Windows, since we're clearly not getting one on Linux. Yeah, that's what's bothering me: as long as that remains unexplained, I don't have any confidence that we're fixing the right thing. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [BUGS] BUG #14634: On Windows pg_basebackup should write tar to stdout in binary mode
On 05/03/2017 07:32 AM, Haribabu Kommi wrote: [Adding -hackers mailing list] On Fri, Apr 28, 2017 at 6:28 PM,wrote: The following bug has been logged on the website: Bug reference: 14634 Logged by: Henry Boehlert Email address: henry_boehl...@agilent.com PostgreSQL version: 9.6.2 Operating system: Windows Server 2012 R2 6.3.9600 Description: Executing command pg_basebackup -D -F t writes its output to stdout, which is open in text mode, causing LF to be converted to CR LF thus corrupting the resulting archive. To write the tar to stdout, on Windows stdout's mode should be temporarily switched to binary. https://msdn.microsoft.com/en-us/library/tw4k6df8.aspx Thanks for reporting the issue. With the attached patch, I was able to extract the tar file that gets generated when the tar file is written into stdout. I tested the the compressed tar also. This bug needs to be fixed in back branches also. Seems reasonable. One question: In the patch, you used "_setmode" function, while the calls in src/bin/pg_dump/pg_backup_archiver.c use "setmode". There are a few places in the backend that also use "_setmode". What's the difference? Should we change some of them to be consistent? - Heikki -- 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] PostgreSQL - Weak DH group
On Thu, Jul 13, 2017 at 5:32 PM, Heikki Linnakangaswrote: > I rebased the patch, did some other clean up of error reporting, and added a > GUC along those lines, as well as docs. How does this look? > > It's late in the release cycle, but it would be nice to sneak this into v10. > Using weak 1024 bit DH parameters is arguably a security issue; it was > originally reported as such. There's a work-around for older versions: > generate custom 2048 bit parameters and place them in a file called > "dh1024.pem", but that's completely undocumented. > > Thoughts? The patch looks in good shape to me. #include "utils/memutils.h" - static intmy_sock_read(BIO *h, char *buf, int size); That's unnecessary noise. + *Very uncool. Alternatively, the system could refuse to start + *if a DH parameters if not specified, but this would tend to + *piss off DBAs. "is not specified". > Objections to committing this now, instead of waiting for v11? But I am -1 for the sneak part. It is not the time to have a new feature in 10, the focus is to stabilize. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
I was not convinced by the overall memory management around variables to begin with, and it is even less so with their new copy management. Maybe having a clean "Variables" data structure could help improve the situation. Ok! Note that there is something for psql (src/bin/psql/variable.c) which may or may not be shared. It should be checked before recoding eventually the same thing. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
Hello, [...] I didn't make rollbacks to savepoints after the failure because they cannot help for serialization failures at all: after rollback to savepoint a new attempt will be always unsuccessful. Not necessarily? It depends on where the locks triggering the issue are set, if they are all set after the savepoint it could work on a second attempt. "SimpleStats attempts": I disagree with using this floating poiunt oriented structures to count integers. I would suggest "int64 tries" instead, which should be enough for the purpose. I'm not sure that it is enough. Firstly it may be several transactions in script so to count the average attempts number you should know the total number of runned transactions. Secondly I think that stddev for attempts number can be quite interesting and often it is not close to zero. I would prefer to have a real motivation to add this complexity in the report and in the code. Without that, a simple int seems better for now. It can be improved later if the need really arises. Some variables, such as "int attempt_number", should be in the client structure, not in the client? Generally, try to use block variables if possible to keep the state clearly disjoints. If there could be NO new variable at the doCustom level that would be great, because that would ensure that there is no machine state mixup hidden in these variables. Do you mean the code cleanup for doCustom function? Because if I do so there will be two code styles for state blocks and their variables in this function.. I think that any variable shared between state is a recipee for bugs if it is not reset properly, so they should be avoided. Maybe there are already too many of them, then too bad, not a reason to add more. The status before the automaton was a nightmare. I wondering whether the RETRY & FAILURE states could/should be merged: I divided these states because if there's a failed transaction block you should end it before retrying. Hmmm. Maybe I'm wrong. I'll think about it. I would suggest a more compact one-line report about failures: "number of failures: 12 (0.001%, deadlock: 7, serialization: 5)" I think, there may be a misunderstanding. Because script can contain several transactions and get both failures. I do not understand. Both failures number are on the compact line I suggested. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] Zipfian distribution in pgbench
Hello Alik, A few comments about the patch v2. Patch applies and compiles. Documentation says that the closer theta is from 0 the flatter the distribution but the implementation requires at least 1, including strange error messages: zipfian parameter must be greater than 1.00 (not 1.00) Could theta be allowed between 0 & 1 ? I've tried forcing with theta = 0.1 and it worked well, so I'm not sure that I understand the restriction. I also tried with theta=0.001 but it seemed less good. I have also tried to check the distribution wrt the explanations, with the attached scripts, n=100, theta=1.01/1.5/3.0: It does not seem to work, there is repeatable +15% bias on i=3 and repeatable -3% to -30% bias for values in i=10-100, this for different values of theta (1.01,1.5, 3.0). If you try the script, beware to set parameters (theta, n) consistently. About the code: Remove spaces and tabs at end of lines or on empty lines. zipfn: I would suggest to move the pg_erand48 out and pass the result instead of passing the thread. the erand call would move to getZipfianRand. I'm wondering whether the "nearest" hack could be removed so as to simplify the cache functions code... Avoid editing lines without changes (spacesn/tabs?) - thread->logfile = NULL; /* filled in later */ + thread->logfile = NULL; /* filled in later */ The documentation explaining the intuitive distribution talks about a N but does not provide any hint about its value depending on the parameters. There is an useless empty lien before "" after that. -- Fabien. compte_init.sql Description: application/sql compte_bench.sql Description: application/sql compte_expected.sql Description: application/sql compte_results.sql Description: application/sql -- 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] pl/perl extension fails on Windows
On 07/13/2017 10:36 AM, Tom Lane wrote: > Andrew Dunstanwrites: >> On 07/13/2017 08:08 AM, Ashutosh Sharma wrote: >>> -dVAR; dXSBOOTARGSAPIVERCHK; >>> +dVAR; dXSBOOTARGSNOVERCHK; >> Good job hunting this down! >> One suggestion I saw in a little googling was that we add this to the XS >> file after the inclusion of XSUB.h: >> #undef dXSBOOTARGSAPIVERCHK >> #define dXSBOOTARGSAPIVERCHK dXSBOOTARGSNOVERCHK > I don't see anything even vaguely like that in the Util.c file generated > by Perl 5.10.1, which is what I've got on my RHEL machine. This is all fairly modern, so it's hardly surprising that it doesn't happen with the ancient perl 5.10. here's a snippet from the generated Util.c on crake (Fedora 25, perl 5.24): XS_EXTERNAL(boot_PostgreSQL__InServer__Util); /* prototype to pass -Wmissing-prototypes */ XS_EXTERNAL(boot_PostgreSQL__InServer__Util) { #if PERL_VERSION_LE(5, 21, 5) dVAR; dXSARGS; #else dVAR; dXSBOOTARGSAPIVERCHK; #endif > > What I do notice is this in Util.xs: > > VERSIONCHECK: DISABLE > > which leads immediately to two questions: > > 1. Why is your version of xsubpp apparently ignoring this directive > and generating a version check anyway? > > 2. Why do we have this directive in the first place? It does not seem > to me like a terribly great idea to ignore low-level version mismatches. > > In the same vein, I'm suspicious of proposals to "fix" this problem > by removing the version check, which seems to be where Ashutosh > is headed. In the long run that seems certain to cause huge headaches. That is a different version check. It's the equivalent of xsubpp's --noversioncheck flag. The versions it would check are the object file and the corresponding pm file. In fact the usage I suggested seems to be blessed in XSUB.h in this comment: /* dXSBOOTARGSNOVERCHK has no API in xsubpp to choose it so do #undef dXSBOOTARGSXSAPIVERCHK #define dXSBOOTARGSXSAPIVERCHK dXSBOOTARGSNOVERCHK */ It would be nice to get to the bottom of why we're getting a version mismatch on Windows, since we're clearly not getting one on Linux. But since we've got on happily all these years without the API version check we might well survive a few more going on as we are. cheers andrew -- Andrew Dunstanhttps://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] Function Volatility and Views Unexpected Behavior
Thanks for the reminder about explain verbose, that's helpful. But optimization does massively change the number of calls of a volatile function in a naive evaluation of a query: `explain analyze verbose select data1 from table1_silly_view where id >=10 and id <= 100;` does an index scan and only runs the volatile function for rows in the view where id >= 10 and id <=100 Subquery Scan on table1_silly_view (cost=0.29..33.77 rows=91 width=8) (actual time=2.552..206.563 rows=91 loops=1) Output: table1_silly_view.data1 -> Index Scan using table1_pkey on public.table1 (cost=0.29..32.86 rows=91 width=20) (actual time=2.550..206.425 rows=91 loops=1) Output: NULL::integer, table1.data1, something_silly(table1.id) Index Cond: ((table1.id >= 10) AND (table1.id <= 100)) Planning time: 0.526 ms Execution time: 206.724 ms whereas `explain analyze verbose select data1 from table1_silly_view where id in ( select id from table1 where id >= 10 and id <=100);` does a full sequential scan, over the view, producing whatever side effects the volatile function does for every row in the view even though they produce the same output and have what should be equivalent quals. Hash Semi Join (cost=11.24..2793.50 rows=91 width=8) (actual time=23.603..22759.297 rows=91 loops=1) Output: table1_1.data1 Hash Cond: (table1_1.id = table1.id) -> Seq Scan on public.table1 table1_1 (cost=0.00..2655.00 rows=1 width=20) (actual time=2.468..22720.942 rows=1 loops=1) Output: table1_1.id, table1_1.data1, something_silly(table1_1.id) -> Hash (cost=10.11..10.11 rows=91 width=4) (actual time=0.484..0.484 rows=91 loops=1) Output: table1.id Buckets: 1024 Batches: 1 Memory Usage: 12kB -> Index Only Scan using table1_pkey on public.table1 (cost=0.29..10.11 rows=91 width=4) (actual time=0.383..0.430 rows=91 loops=1) Output: table1.id Index Cond: ((table1.id >= 10) AND (table1.id <= 100)) Heap Fetches: 91 Planning time: 0.877 ms Execution time: 22759.448 ms I recognize that it is an anti-pattern to put a volatile function call in a view, and don't know that there's a better way of dealing with it, as not using indexes in a view that has a volatile function call in it at all seems like a very bad choice, but still think it might be something to document better. -David On Wed, Jul 12, 2017 at 3:23 PM Tom Lanewrote: > David Kohn writes: > > I encountered some unexpected behavior when debugging a query that was > > taking longer than expected, basically, a volatile function that makes a > > column in a view is called even when that column is not selected in the > > query, making it so that the function is called for every row in the > view, > > I'm not sure that that would necessarily be the expected behavior, as it > > was my understanding that columns that are not selected are not > evaluated, > > for instance if there was a join in a view that produced some columns and > > said columns were not selected, I would expect it to be optimized away. > > No, this is the expected behavior; we don't like optimization to change > the number of calls of a volatile function from what would occur in naive > evaluation of the query. If that prospect doesn't bother you, it's > likely because your function isn't really volatile ... > > > The other problem is that the function call does not appear in the query > > plan. > > I think "explain verbose" will fix that for you. > > regards, tom lane >
Re: [HACKERS] Replacing lfirst() with lfirst_node() appropriately in planner.c
Ashutosh Bapat wrote: > Happened to stumble across some instances of lfirst() which could use > lfirst_node() in planner.c. Here's patch which replaces calls to > lfirst() extracting node pointers by lfirst_node() in planner.c. Sounds good. > Are we carrying out such replacements in master or this will be > considered for v11? This is for pg11 definitely; please add to the open commitfest. -- Álvaro Herrerahttps://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] PostgreSQL - Weak DH group
(We dropped the ball back in October, continuing the discussion now) On 10/10/2016 06:24 PM, Heikki Linnakangas wrote: On 10/06/2016 10:26 PM, Christoph Berg wrote: Re: Heikki Linnakangas 2016-10-06I propose the attached patch. It gives up on trying to deal with multiple key lengths (as noted earlier, OpenSSL just always passed keylength=1024, so that was useless). Instead of using the callback, it just sets fixed DH parameters with SSL_CTX_set_tmp_dh(), like we do for the ECDH curve. The DH parameters are loaded from a file called "dh_params.pem" (instead of "dh1024.pem"), if present, otherwise the built-in 2048 bit parameters are used. Shouldn't this be a GUC pointing to a configurable location like ssl_cert_file? This way, people reading the security section of the default postgresql.conf would notice that there's something (new) to configure. (And I wouldn't want to start creating symlinks from PGDATA to /etc/ssl/something again...) Perhaps. The DH parameters are not quite like other configuration files, though. The actual parameters used don't matter, you just want to generate different parameters for every cluster. The key length of the parameters can be considered configuration, though. Actually, adding a GUC also solves another grief I had about this. There is currently no easy way to tell if your parameter file is being used, or if the server is failing to read it and is falling back to the hard-coded parameters. With a GUC, if the GUC is set it's a good indication that the admin expects the file to really exist and to contain valid parameters. So if the GUC is set, we should throw an error if it cannot be used. And if it's not set, we use the built-in defaults. I rebased the patch, did some other clean up of error reporting, and added a GUC along those lines, as well as docs. How does this look? It's late in the release cycle, but it would be nice to sneak this into v10. Using weak 1024 bit DH parameters is arguably a security issue; it was originally reported as such. There's a work-around for older versions: generate custom 2048 bit parameters and place them in a file called "dh1024.pem", but that's completely undocumented. Thoughts? Objections to committing this now, instead of waiting for v11? - Heikki >From 481e019441dadc0826e6e9b7d4a56c49e63c654b Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas Date: Thu, 13 Jul 2017 18:29:48 +0300 Subject: [PATCH 1/1] Always use 2048 bit DH parameters for OpenSSL ephemeral DH ciphers. 1024 bits is considered weak these days, but OpenSSL always passes 1024 as the key length to the tmp_dh callback. All the code to handle other key lengths as in fact dead. To remedy those issues: * Only include hard-coded 2048-bit parameters. * Set the parameters directly with SSL_CTX_set_tmp_dh(), without the callback * The name of the file containing the DH parameters is now a GUC. This replaces the old hardcoded "dh1024.pem" filename. (The files for other key lengths, dh512.pem, dh2048.pem, etc. were never actually used.) Per report by Nicolas Guini Discussion: https://www.postgresql.org/message-id/camxbouyjooautvozn6ofzym828anrdjuccotccquxjws-l2...@mail.gmail.com --- doc/src/sgml/config.sgml | 23 +++ src/backend/libpq/be-secure-openssl.c | 265 +- src/backend/libpq/be-secure.c | 1 + src/backend/utils/misc/guc.c | 11 ++ src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/libpq/libpq.h | 1 + 6 files changed, 132 insertions(+), 170 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index 80d1679b14..6c1452a9bc 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1203,6 +1203,29 @@ include_dir 'conf.d' + + ssl_dh_params_file (string) + + ssl_ecdh_curve configuration parameter + + + + +Specifies the name of the file containing Diffie-Hellman parameters used for +so-called Perfect Forward Secrecy SSL ciphers. The default is empty, in +which case compiled-in default DH parameters used. Using custom DH +parameters reduces the exposure if an attacker manages to crack the +well-known compiled-in DH parameters. You can create your own DH parameters +file with the command openssl dhparam -out dhparams.pem 2048. + + + +This parameter can only be set in the postgresql.conf +file or on the server command line. + + + + krb_server_keyfile (string) diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 67145e9412..ed1779d6b6 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -61,23 +61,22 @@ #include "libpq/libpq.h" #include
Re: [HACKERS] building libpq.a static library
On Thu, Jul 13, 2017 at 4:58 AM, Craig Ringerwrote: > You shouldn't ever need static libraries on Windows, though. Because it > searches the CWD first on its linker search path, you can just drop > libpq.dll in the same directory as your binary/library and link to the stub > libpq.lib . This is not possible in our case. The R programming language binaries are installed in the "program files" directory which is only writable by the sys admin. There are over 10.000 contributed packages (including one with postgres drivers) which are installed by the user in the home dir and the package DLL's need to get dynamically loaded at runtime. We have been working with this system for a very long time and static linking external libs to the package DLL is really the only reliable way to prevent DLL problems across Windows versions/configurations > I'm not trying to block support for a static libpq, I just don't see the > point. This is a bit overgeneralized, there are many use cases where static linking is the only feasible way. Most libs support --enable static and many distros ship both static and shared libs and leave it up to user or developer author how to configure their application. -- 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] pl/perl extension fails on Windows
Andrew Dunstanwrites: > On 07/13/2017 08:08 AM, Ashutosh Sharma wrote: >> -dVAR; dXSBOOTARGSAPIVERCHK; >> +dVAR; dXSBOOTARGSNOVERCHK; > Good job hunting this down! > One suggestion I saw in a little googling was that we add this to the XS > file after the inclusion of XSUB.h: > #undef dXSBOOTARGSAPIVERCHK > #define dXSBOOTARGSAPIVERCHK dXSBOOTARGSNOVERCHK I don't see anything even vaguely like that in the Util.c file generated by Perl 5.10.1, which is what I've got on my RHEL machine. What I do notice is this in Util.xs: VERSIONCHECK: DISABLE which leads immediately to two questions: 1. Why is your version of xsubpp apparently ignoring this directive and generating a version check anyway? 2. Why do we have this directive in the first place? It does not seem to me like a terribly great idea to ignore low-level version mismatches. In the same vein, I'm suspicious of proposals to "fix" this problem by removing the version check, which seems to be where Ashutosh is headed. In the long run that seems certain to cause huge headaches. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PG 10 release notes
Hello hackers, From: Peter Geoghegan> Date: Wed, 5 Jul 2017 15:19:57 -0700 > Subject: Re: [BUGS] BUG #14722: Segfault in tuplesort_heap_siftup, 32 bit > overflow > On pgsql-b...@postgresql.org On 07/06/2017 12:19 AM, Peter Geoghegan wrote: > In Postgres 10, tuplesort external sort run merging became much faster > following commit 24598337c8d. It might be noticeable if such a machine > were using Postgres 10 [...] Should-we mention this improvement in release notes? Regards, -- Adrien NAYRAT http://dalibo.com - http://dalibo.org signature.asc Description: OpenPGP digital signature
Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
Another detail I forgot about this point: there may be a memory leak on variables copies, ISTM that the "variables" array is never freed. I was not convinced by the overall memory management around variables to begin with, and it is even less so with their new copy management. Maybe having a clean "Variables" data structure could help improve the situation. Ok! -- Marina Polyakova Postgres Professional: http://www.postgrespro.com The Russian Postgres Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors
Here are a round of comments on the current version of the patch: Thank you very much again! There is a latent issue about what is a transaction. For pgbench a transaction is a full script execution. For postgresql, it is a statement or a BEGIN/END block, several of which may appear in a script. From a retry perspective, you may retry from a SAVEPOINT within a BEGIN/END block... I'm not sure how to make general sense of all this, so this is just a comment without attached action for now. Yes it is. That's why I wrote several notes about it in documentation where there may be a misunderstanding: +Transactions with serialization or deadlock failures (or with both +of them if used script contains several transactions; see ++endterm="transactions-and-scripts-title"> for more information) are +marked separately and their time is not reported as for skipped +transactions. + + What is the Transaction Actually Performed in pgbench? +If a transaction has serialization and/or deadlock failures, its + time will be reported as serialization failure, + deadlock failure, or + serialization and deadlock failures, respectively. + + + Transactions can have both serialization and deadlock failures if the + used script contained several transactions. See + for more information. + + + + +The number of transactions attempts within the interval can be greater than +the number of transactions within this interval multiplied by the maximum +attempts number. See for more information. + + + + The total sum of per-command failures of each type can be greater + than the number of transactions with reported failures. + See + endterm="transactions-and-scripts-title"> for more information. + + And I didn't make rollbacks to savepoints after the failure because they cannot help for serialization failures at all: after rollback to savepoint a new attempt will be always unsuccessful. I would consider using "try/tries" instead of "attempt/attempts" as it is shorter. An English native speaker opinion would be welcome on that point. Thank you, I'll change it. I'm fine with renaming "is_latencies" to "report_per_command", which is more logical & generic. Glad to hear it! "max_attempt_number": I'm against typing fields again in their name, aka "hungarian naming". I'd suggest "max_tries" or "max_attempts". Ok! "SimpleStats attempts": I disagree with using this floating poiunt oriented structures to count integers. I would suggest "int64 tries" instead, which should be enough for the purpose. I'm not sure that it is enough. Firstly it may be several transactions in script so to count the average attempts number you should know the total number of runned transactions. Secondly I think that stddev for attempts number can be quite interesting and often it is not close to zero. LastBeginState -> RetryState? I'm not sure why this state is a pointer in CState. Putting the struct would avoid malloc/free cycles. Index "-1" may be used to tell it is not set if necessary. Thanks, I agree that it's better to do in this way. "CSTATE_RETRY_FAILED_TRANSACTION" -> "CSTATE_RETRY" is simpler and clear enough. Ok! In CState and some code, a failure is a failure, maybe one boolean would be enough. It need only be differentiated when counting, and you have (deadlock_failure || serialization_failure) everywhere. I agree with you. I'll change it. Some variables, such as "int attempt_number", should be in the client structure, not in the client? Generally, try to use block variables if possible to keep the state clearly disjoints. If there could be NO new variable at the doCustom level that would be great, because that would ensure that there is no machine state mixup hidden in these variables. Do you mean the code cleanup for doCustom function? Because if I do so there will be two code styles for state blocks and their variables in this function.. I wondering whether the RETRY & FAILURE states could/should be merged: on RETRY: -> count retry -> actually retry if < max_tries (reset client state, jump to command) -> else count failure and skip to end of script The start and end of transaction detection seem expensive (malloc, ...) and assume a one statement per command (what about "BEGIN \; ... \; COMMIT;", which is not necessarily the case, this limitation should be documented. ISTM that the space normalization should be avoided, and something simpler/lighter should be devised? Possibly it should consider handling SAVEPOINT. I divided these states because if there's a failed transaction block you should end it before retrying. It means to go to states CSTATE_START_COMMAND -> CSTATE_WAIT_RESULT -> CSTATE_END_COMMAND with the appropriate command. How do you propose not to go to these states? About
Re: [HACKERS] Race condition in GetOldestActiveTransactionId()
On 08/22/2016 01:46 PM, Heikki Linnakangas wrote: While hacking on the CSN patch, I spotted a race condition between GetOldestActiveTransactionId() and GetNewTransactionId(). GetOldestActiveTransactionId() calculates the oldest XID that's still running, by doing: 1. Read nextXid, without a lock. This is the upper bound, if there are no active XIDs in the proc array. 2. Loop through the proc array, making note of the oldest XID. While GetNewTransactionId() does: 1. Read and Increment nextXid 2. Set MyPgXact->xid. It seems possible that if you call GetNewTransactionId() concurrently with GetOldestActiveTransactionId(), GetOldestActiveTransactionId() sees the new nextXid value that the concurrent GetNewTransactionId() set, but doesn't see the old XID in the proc array. It will return a value that doesn't cover the old XID, i.e. it won't consider the just-assigned XID as in-progress. Am I missing something? Commit c6d76d7c added a comment to GetOldestActiveTransactionId() explaining why it's not necessary to acquire XidGenLock there, but I think it missed the above race condition. GetOldestActiveTransactionId() is not performance-critical, it's only called when performing a checkpoint, so I think we should just bite the bullet and grab the lock. Per attached patch. I did some testing, and was able to indeed construct a case where oldestActiveXid was off-by-one in an online checkpoint record. However, looking at how it's used, I think it happened to not have any user-visible effect. The oldestActiveXid value of an online checkpoint is only used to determine the point where pg_subtrans is initialized, and the XID missed in the computation could not be a subtransaction. Nevertheless, it's clearly a bug and the fix is simple, so I committed a fix. - Heikki -- 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] pl/perl extension fails on Windows
On 07/13/2017 08:08 AM, Ashutosh Sharma wrote: > > After doing some study, I could understand that Util.c is generated > from Util.xs by xsubpp compiler at build time. This is being done in > Mkvcbuild.pm file in postgres. If I manually replace > 'dXSBOOTARGSAPIVERCHK' macro with 'dXSBOOTARGSNOVERCHK' macro in > src/pl/plperl/Util.c, the things work fine. The diff is as follows, > > XS_EXTERNAL(boot_PostgreSQL__InServer__Util) > { > #if PERL_VERSION_LE(5, 21, 5) > dVAR; dXSARGS; > #else > -dVAR; dXSBOOTARGSAPIVERCHK; > +dVAR; dXSBOOTARGSNOVERCHK; > > I need to further investigate, but let me know if you have any ideas. Good job hunting this down! One suggestion I saw in a little googling was that we add this to the XS file after the inclusion of XSUB.h: #undef dXSBOOTARGSAPIVERCHK #define dXSBOOTARGSAPIVERCHK dXSBOOTARGSNOVERCHK cheers andrew -- Andrew Dunstanhttps://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] Bug in ExecModifyTable function and trigger issues for foreign tables
On 2017/06/30 18:44, Etsuro Fujita wrote: On 2017/06/16 21:29, Etsuro Fujita wrote: I'll have second thought about this, so I'll mark this as waiting on author. I spent quite a bit of time on this and came up with a solution for addressing the concern mentioned by Ashutosh [1]. The basic idea is, as I said before, to move rewriteTargetListUD from the rewriter to the planner (whether the update or delete is inherited or not), except for the view case. In case of inherited UPDATE/DELETE, the planner adds a necessary junk column needed for the update or delete for each child relation, without the assumption I made before about junk tlist entries, so I think this would be more robust for future changes as mentioned in [1]. It wouldn't work well that the planner does the same thing for the view case (ie, add a whole-row reference to the given target relation), unlike other cases, because what we need to do for that case is to add a whole-row reference to the view as the source for an INSTEAD OF trigger, not the target. So, ISTM that it's reasonable to handle that case in the rewriter as-is, not in the planner, but one thing I'd like to propose to simplify the code in rewriteHandler.c is to move the code for the view case in rewriteTargetListUD to ApplyRetrieveRule. By that change, we won't add a whole-row reference to the view in RewriteQuery, so we don't need this annoying thing in rewriteTargetView any more: /* * For UPDATE/DELETE, rewriteTargetListUD will have added a wholerow junk * TLE for the view to the end of the targetlist, which we no longer need. * Remove it to avoid unnecessary work when we process the targetlist. * Note that when we recurse through rewriteQuery a new junk TLE will be * added to allow the executor to find the proper row in the new target * relation. (So, if we failed to do this, we might have multiple junk * TLEs with the same name, which would be disastrous.) */ if (parsetree->commandType != CMD_INSERT) { TargetEntry *tle = (TargetEntry *) llast(parsetree->targetList); Assert(tle->resjunk); Assert(IsA(tle->expr, Var) && ((Var *) tle->expr)->varno == parsetree->resultRelation && ((Var *) tle->expr)->varattno == 0); parsetree->targetList = list_delete_ptr(parsetree->targetList, tle); } Attached is an updated version of the patch. Comments are welcome! Best regards, Etsuro Fujita [1] https://www.postgresql.org/message-id/CAFjFpRfTpamoUz6fNyk6gPh%3DecfBJjbUHJNKbDxscpyPJ3FfjQ%40mail.gmail.com *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 6924,6929 update bar set f2 = f2 + 100 returning *; --- 6924,6988 7 | 277 (6 rows) + -- Test that UPDATE/DELETE with inherited target works with row-level triggers + CREATE TRIGGER trig_row_before + BEFORE UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + CREATE TRIGGER trig_row_after + AFTER UPDATE OR DELETE ON bar2 + FOR EACH ROW EXECUTE PROCEDURE trigger_data(23,'skidoo'); + explain (verbose, costs off) + update bar set f2 = f2 + 100; + QUERY PLAN + -- + Update on public.bar +Update on public.bar +Foreign Update on public.bar2 + Remote SQL: UPDATE public.loct2 SET f2 = $2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on public.bar + Output: bar.f1, (bar.f2 + 100), bar.ctid +-> Foreign Scan on public.bar2 + Output: bar2.f1, (bar2.f2 + 100), bar2.f3, bar2.ctid, bar2.* + Remote SQL: SELECT f1, f2, f3, ctid FROM public.loct2 FOR UPDATE + (9 rows) + + update bar set f2 = f2 + 100; + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_before(23, skidoo) BEFORE ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (3,333,33),NEW: (3,433,33) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (4,344,44),NEW: (4,444,44) + NOTICE: trig_row_after(23, skidoo) AFTER ROW UPDATE ON bar2 + NOTICE: OLD: (7,277,77),NEW: (7,377,77) + explain (verbose, costs off) + delete from bar where f2 < 400; + QUERY PLAN + - + Delete on public.bar +Delete on public.bar +Foreign Delete on public.bar2 + Remote SQL: DELETE FROM public.loct2 WHERE ctid = $1 RETURNING f1, f2, f3 +-> Seq Scan on
Re: [HACKERS] pl/perl extension fails on Windows
On Thu, Jul 13, 2017 at 12:01 AM, Andrew Dunstanwrote: > On 07/12/2017 11:49 AM, Dave Page wrote: >> >> >> Well crake is a Fedora box - and we have no problems on Linux, only on >> Windows. >> >> > > > Yeah, I have this on one of my Windows boxes, and haven't had time to > get to the bottom of it yet ;-( > I could also see this problem in my Windows machine and I have spent some time to analyse it. Here are my findings: The stacktrace where the crash happens is: perl524.dll!Perl_xs_handshake(const unsigned long key, void *v_my_perl, const char * file, ...) Line 5569 C plperl.dll!boot_PostgreSQL__InServer__Util(interpreter * my_perl, cv * cv) Line 490 + 0x22 bytes C perl524.dll!Perl_pp_entersub(interpreter * my_perl) Line 3987 + 0xc bytes C perl524.dll!Perl_runops_standard(interpreter * my_perl) Line 41 + 0x6 bytes C perl524.dll!S_run_body(interpreter * my_perl, long oldscope) Line 2485 C perl524.dll!perl_run(interpreter * my_perl) Line 2406 + 0xc bytes C plperl.dll!plperl_init_interp() Line 829 + 0xb bytes C plperl.dll!_PG_init() Line 470 + 0x5 bytes C I couldn't get much out of above bt, but, one thing i could notice is that the input key passed to 'Perl_xs_handshake()' function is not matching with the key being generated inside 'Perl_xs_handshake()', hence, the handshaking is failing. Please have a look into the following code snippet from 'perl 5.24' and 'Util.c' file in plperl respectively, Perl-5.24: === got = INT2PTR(void*, (UV)(key & HSm_KEY_MATCH)); need = (void *)(HS_KEY(FALSE, FALSE, "", "") & HSm_KEY_MATCH); if (UNLIKELY(got != need)) goto bad_handshake; Util.c in plperl: == 485 XS_EXTERNAL(boot_PostgreSQL__InServer__Util) 486 { 487 #if PERL_VERSION_LE(5, 21, 5) 488dVAR; dXSARGS; 489 #else 490dVAR; dXSBOOTARGSAPIVERCHK; Actually the macro 'dXSBOOTARGSAPIVERCHK' in line #490 gets expanded to, #define XS_APIVERSION_SETXSUBFN_POPMARK_BOOTCHECK \ Perl_xs_handshake(HS_KEY(TRUE, TRUE, "v" PERL_API_VERSION_STRING, ""), \ HS_CXT, __FILE__, "v" PERL_API_VERSION_STRING) And the point to be noted is, the way, the key is being generated in above macro and in Perl_xs_handshake(). As shown above, 'XS_APIVERSION_SETXSUBFN_POPMARK_BOOTCHECK' macro passes 'PERL_API_VERSION_STRING' as input to HS_KEY() to generate the key and this key is passed to Perl_xs_handshake function whereas inside Perl_xs_handshake(), there is no such version string being used to generate the key. Hence, the key mismatch is seen thereby resulting in a bad_handshake error. After doing some study, I could understand that Util.c is generated from Util.xs by xsubpp compiler at build time. This is being done in Mkvcbuild.pm file in postgres. If I manually replace 'dXSBOOTARGSAPIVERCHK' macro with 'dXSBOOTARGSNOVERCHK' macro in src/pl/plperl/Util.c, the things work fine. The diff is as follows, XS_EXTERNAL(boot_PostgreSQL__InServer__Util) { #if PERL_VERSION_LE(5, 21, 5) dVAR; dXSARGS; #else -dVAR; dXSBOOTARGSAPIVERCHK; +dVAR; dXSBOOTARGSNOVERCHK; I need to further investigate, but let me know if you have any ideas. > > Latest versions of ActivePerl don't ship with library descriptor files, > either, which is unpleasant. > I too had similar observation and surprisingly, I am seeing 'libperl*.a' file instead of 'perl*.lib' file in most of the ActiverPerl versions for Windows. -- With Regards, Ashutosh Sharma EnterpriseDB:http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_stop_backup(wait_for_archive := true) on standby server
On Thu, Jul 13, 2017 at 7:13 AM, Masahiko Sawadawrote: > Sorry, I missed lots of typo in the last patch. All comments from you > are incorporated into the attached latest patch and I've checked it > whether there is other typos. Please review it. Thanks for providing a new version of the patch very quickly. I have spent some time looking at it again and testing it, and this version looks correct to me. Stephen, as a potential committer, would you look at it to address this open item? -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [WIP] Zipfian distribution in pgbench
On 13 Jul 2017, at 00:20, Peter Geogheganwrote:Actually, I mean that I wonder how much of a difference it would makeif this entire block was commented out within _bt_doinsert():if (checkUnique != UNIQUE_CHECK_NO){ …}I am attaching results of test for 32 and 128 clients for original and patched(_bt_doinsert) variants.— Thanks and Regards,Alik KhilazhevPostgres Professional:http://www.postgrespro.comThe Russian Postgres Company idx | level | l_item | blkno | btpo_prev | btpo_next | btpo_flags | type | live_items | dead_items | avg_item_size | page_size | free_size | highkey ---+---++---+---+---++--+++---+---+---+- pgbench_accounts_pkey | 2 | 1 | 290 | 0 | 0 | 2 | r| 10 | 0 |15 | 8192 | 7956 | pgbench_accounts_pkey | 1 | 1 | 3 | 0 | 289 | 0 | i|298 | 0 |15 | 8192 | 2196 | 09 96 01 00 00 00 00 00 pgbench_accounts_pkey | 1 | 2 | 289 | 3 | 575 | 0 | i|285 | 0 |15 | 8192 | 2456 | 11 2c 03 00 00 00 00 00 pgbench_accounts_pkey | 1 | 3 | 575 | 289 | 860 | 0 | i|285 | 0 |15 | 8192 | 2456 | 19 c2 04 00 00 00 00 00 pgbench_accounts_pkey | 1 | 4 | 860 | 575 | 1145 | 0 | i|285 | 0 |15 | 8192 | 2456 | 21 58 06 00 00 00 00 00 pgbench_accounts_pkey | 1 | 5 | 1145 | 860 | 1430 | 0 | i|285 | 0 |15 | 8192 | 2456 | 29 ee 07 00 00 00 00 00 pgbench_accounts_pkey | 1 | 6 | 1430 | 1145 | 1715 | 0 | i|285 | 0 |15 | 8192 | 2456 | 31 84 09 00 00 00 00 00 pgbench_accounts_pkey | 1 | 7 | 1715 | 1430 | 2000 | 0 | i|285 | 0 |15 | 8192 | 2456 | 39 1a 0b 00 00 00 00 00 pgbench_accounts_pkey | 1 | 8 | 2000 | 1715 | 2285 | 0 | i|285 | 0 |15 | 8192 | 2456 | 41 b0 0c 00 00 00 00 00 pgbench_accounts_pkey | 1 | 9 | 2285 | 2000 | 2570 | 0 | i|285 | 0 |15 | 8192 | 2456 | 49 46 0e 00 00 00 00 00 pgbench_accounts_pkey | 1 | 10 | 2570 | 2285 | 0 | 0 | i|177 | 0 |15 | 8192 | 4616 | pgbench_accounts_pkey | 0 | 1 | 1 | 0 | 2751 | 65 | l| 21 |225 |16 | 8192 | 3228 | 14 00 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 2 | 2751 | 1 | 2746 | 65 | l| 40 |182 |16 | 8192 | 3708 | 3b 00 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 3 | 2746 | 2751 | 2750 | 65 | l| 43 |240 |16 | 8192 | 2488 | 65 00 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 4 | 2750 | 2746 | 2745 | 65 | l| 51 | 57 |16 | 8192 | 5988 | 97 00 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 5 | 2745 | 2750 | 2756 | 65 | l| 42 | 47 |16 | 8192 | 6368 | c0 00 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 6 | 2756 | 2745 | 2748 | 65 | l| 54 |139 |16 | 8192 | 4288 | f5 00 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 7 | 2748 | 2756 | 2755 | 65 | l| 57 |333 |16 | 8192 | 348 | 2d 01 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 8 | 2755 | 2748 | 2 | 65 | l| 67 |308 |16 | 8192 | 648 | 6f 01 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 9 | 2 | 2755 | 2753 | 65 | l| 75 |280 |16 | 8192 | 1048 | b9 01 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 10 | 2753 | 2 | 2747 | 65 | l| 83 |260 |16 | 8192 | 1288 | 0b 02 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 11 | 2747 | 2753 | 2754 | 65 | l| 91 |192 |16 | 8192 | 2488 | 65 02 00 00 00 00 00 00 pgbench_accounts_pkey | 0 | 12 | 2754 | 2747 | 4 | 65 | l|121 |196 |16
Re: [HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING
On Thu, Jul 13, 2017 at 2:53 PM, Kyotaro HORIGUCHIwrote: > Hello, moved to pgsql-hackers. > > This is the revased and revised version of the previous patch. > > At Thu, 13 Jul 2017 13:42:49 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI > wrote in > <20170713.134249.97825982.horiguchi.kyot...@lab.ntt.co.jp> >> At Tue, 11 Jul 2017 15:39:14 -0400, Tom Lane wrote in >> <6234.1499801...@sss.pgh.pa.us> >> > Amit Langote writes: >> > > Horiguchi-san, >> > > On 2017/07/11 10:28, Kyotaro HORIGUCHI wrote: >> > >> I faintly recall such discussion was held aroud that time and >> > >> maybe we concluded that we don't do that but I haven't find such >> > >> a thread in pgsql-hackers.. >> > >> > > I mentioned it in my reply. Here again: >> > > https://www.postgresql.org/message-id/20160405.184408.166437663.horiguchi.kyotaro%40lab.ntt.co.jp >> > >> > The followup discussion noted that that approach was no good because it >> > would only close connections in the same session that had done the ALTER >> > SERVER. I think the basic idea of marking postgres_fdw connections as >> > needing to be remade when next possible is OK, but we have to drive it >> > off catcache invalidation events, the same as we did in c52d37c8b. An >> > advantage of that way is we don't need any new hooks in the core code. >> > >> > Kyotaro-san, are you planning to update your old patch? >> >> I'm pleased to do that. I will reconsider the way shown in a mail >> in the thread soon. > > done. > > (As a recap) Changing of some options such as host of a foreign > server or password of a user mapping make the existing > connections stale, but postgres_fdw continue using them. > > The attached patch does the following things. > > - postgres_fdw registers two invalidation callbacks on loading. > > - On any change on a foreign server or a user mapping, the > callbacks mark the affected connection as 'invalid' > > - The invalidated connections will be once disconnected before > the next execution if no transaction exists. > > As the consequence, changes of options properly affects > subsequent queries in the next transaction on any session. It > occurs after whatever option has been modifed. > > == > create server sv1 foreign data wrapper postgres_fdw options (host '/tmp', > port '5432', dbname 'postgres'); > create user mapping for public server sv1; > create table t (a int); > create foreign table ft1 (a int) server sv1 options (table_name 't1'); > begin; > select * from ft1; -- no error > alter server sv1 options (set host '/tmpe'); > select * from ft1; -- no error because transaction is living. > commit; > select * from ft1; > ERROR: could not connect to server "sv1" - NEW > == > > This patch is postgres_fdw-private but it's annoying that hash > value of syscache is handled in connection.c. If we allow to add > something to the core for this feature, I could add a new member > in FdwRoutine to notify invalidation of mapping and server by > oid. (Of course it is not back-patcheable, though) > > Does anyone have opinitons or suggestions? > The patch and the idea looks good to me. I haven't reviewed it thoroughly though. I noticed a type "suporious", I think you meant "spurious"? Probably that comment should be part of the function which marks the connection as invalid e.g. InvalidateConnectionForMapping(). pgfdw_xact_callback() reports the reason for disconnection while closing a connection. May be we want to report the reason for disconnection here as well. Also, may be we want to create a function to discard connection from an entry so that we consistently do the same things while discarding a connection. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Update description of \d[S+] in \?
On Thu, Jul 13, 2017 at 12:01 PM, Amit Langotewrote: > The description of \d[S+] currently does not mention that it will list > materialized views and foreign tables. Attached fixes that. > I guess the same change is applicable to the description of \d[S+] NAME as well. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Row Level Security Documentation
Hello Rod, This version of the table attempts to stipulate which section of the process the rule applies to. A few comments about this patch. It applies cleanly, make html is ok. It adds a summary table which shows for each case what happens. Although the information can be guessed/infered from the text, I think that a summary table is a good thing, at least because it breaks an otherwise dense presentation. I would suggest the following changes: The table should be referenced from the description, something like "Table xxx summarizes the ..." ISTM that it would be clearer to split the Policy column into "FOR xxx ..." and "USING" or "WITH CHECK", and to merge the rows which have the same "FOR xxx ..." contents, something like: POLICY | ---++- | USING | ... FOR ALL ...++- | WITH CHECK | ... ---++- FOR SELECT ... | USING | ... So that it is clear that only ALL & UPDATE can get both USING & WITH CHECK. This can be done with "morerows=1" on an entry so that it spans more rows. For empty cells, maybe a dash would be clearer. Not sure. -- Fabien -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] New partitioning - some feedback
On 2017/07/13 7:23, Dean Rasheed wrote: > On 12 July 2017 at 15:58, Alvaro Herrerawrote: >> Amit Langote wrote: >>> On 2017/07/11 13:34, Alvaro Herrera wrote: However, the "list tables" command \dt should definitely IMO not list partitions. >>> >>> Do you mean never? Even if a modifier is specified? In the patch I >>> proposed, \d! (or \d+ or \d++, if '!' turns out to be unpopular) will list >>> partitions, but \d or \dt won't. That is, partitions are hidden by default. >> >> I don't think there is any need for a single list of all partition of >> all tables -- is there? I can't think of anything, but then I haven't >> been exposed very much to this feature yet. For now, I lean towards "never". >> > > So just focusing on the listing issue for now... > > I tend to agree with some of the upstream comments that a bare \d > should list everything, including partitions, because partitions are > still tables that you might want to do DML or DDL on. > > Also, if you look at what we already have, \d lists all types of > relations, and then there are 2-letter commands \dE, \di, \dm, \ds, > \dt and \dv that list just specific kinds of relations, for example > \dE lists foreign tables, and \dt lists local tables, specifically > excluding foreign tables. > > So ISTM that the most logical extension of that is: > > \d - list all relations, including partitions \d does leave out indexes, but that seems okay. I think it might be okay to show partitions after all. If we do so, do we indicate somehow that they are partitions of some table? Maybe an additional column "Partition" with values "yes" or "no" that occurs right next to the Type column. Output would look something like below: \d List of relations Schema | Name| Type| Partition | Owner +---+---+---+--- public | foo | table | no| amit public | foo_a_seq | sequence | no| amit public | xyz | partitioned table | no| amit public | xyz1 | table | yes | amit public | xyz2 | table | yes | amit public | xyz3 | partitioned table | yes | amit public | xyz4 | foreign table | yes | amit (7 rows) > \dt - list only tables that are not foreign tables or partitions > of other tables Note that that list will include partitioned tables. > (that's not quite an exact extension of the existing logic, because of > course it's partitioned tables that have the different relkind, not > the partitions, but the above seems like the most useful behaviour) We allow creating regular tables, partitioned tables, and foreign tables as partitions. Being a partition is really independent from the considerations with which these 2-letter commands are designed, that is, the second letters map one-to-one with relkinds (again, an exception made when showing both regular tables and partitioned table with \dt.) If we establish a rule that each such 2-letter command will only show the tables of the corresponding relkind that are not partitions, that is, only those for which relispartition=false will be shown, then we should find an extension/modifier such that for each command it enables listing partitions as well. Perhaps the idea you mentioned at [1] of using letter 'P' for that purpose could work. As you described, \dtP or \dPt shows tables (partitioned or not) including those that are partitions. Bare \d will mean \dPtvmsE. > I also agree that there probably isn't much need for a list that > *only* includes partitions, but if someone comes up with a convincing > use case, then we could add another 2-letter command for that. I too can't imagine needing to see only partitions. Thanks, Amit [1] https://www.postgresql.org/message-id/CAEZATCWcfFtsbKYcVyqUzoOsxkikQjpi_GdjZ_vL6RcX8iLEsg%40mail.gmail.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PgFDW connection invalidation by ALTER SERVER/ALTER USER MAPPING
Hello, moved to pgsql-hackers. This is the revased and revised version of the previous patch. At Thu, 13 Jul 2017 13:42:49 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHIwrote in <20170713.134249.97825982.horiguchi.kyot...@lab.ntt.co.jp> > At Tue, 11 Jul 2017 15:39:14 -0400, Tom Lane wrote in > <6234.1499801...@sss.pgh.pa.us> > > Amit Langote writes: > > > Horiguchi-san, > > > On 2017/07/11 10:28, Kyotaro HORIGUCHI wrote: > > >> I faintly recall such discussion was held aroud that time and > > >> maybe we concluded that we don't do that but I haven't find such > > >> a thread in pgsql-hackers.. > > > > > I mentioned it in my reply. Here again: > > > https://www.postgresql.org/message-id/20160405.184408.166437663.horiguchi.kyotaro%40lab.ntt.co.jp > > > > The followup discussion noted that that approach was no good because it > > would only close connections in the same session that had done the ALTER > > SERVER. I think the basic idea of marking postgres_fdw connections as > > needing to be remade when next possible is OK, but we have to drive it > > off catcache invalidation events, the same as we did in c52d37c8b. An > > advantage of that way is we don't need any new hooks in the core code. > > > > Kyotaro-san, are you planning to update your old patch? > > I'm pleased to do that. I will reconsider the way shown in a mail > in the thread soon. done. (As a recap) Changing of some options such as host of a foreign server or password of a user mapping make the existing connections stale, but postgres_fdw continue using them. The attached patch does the following things. - postgres_fdw registers two invalidation callbacks on loading. - On any change on a foreign server or a user mapping, the callbacks mark the affected connection as 'invalid' - The invalidated connections will be once disconnected before the next execution if no transaction exists. As the consequence, changes of options properly affects subsequent queries in the next transaction on any session. It occurs after whatever option has been modifed. == create server sv1 foreign data wrapper postgres_fdw options (host '/tmp', port '5432', dbname 'postgres'); create user mapping for public server sv1; create table t (a int); create foreign table ft1 (a int) server sv1 options (table_name 't1'); begin; select * from ft1; -- no error alter server sv1 options (set host '/tmpe'); select * from ft1; -- no error because transaction is living. commit; select * from ft1; ERROR: could not connect to server "sv1" - NEW == This patch is postgres_fdw-private but it's annoying that hash value of syscache is handled in connection.c. If we allow to add something to the core for this feature, I could add a new member in FdwRoutine to notify invalidation of mapping and server by oid. (Of course it is not back-patcheable, though) Does anyone have opinitons or suggestions? regards, -- Kyotaro Horiguchi NTT Open Source Software Center *** a/contrib/postgres_fdw/connection.c --- b/contrib/postgres_fdw/connection.c *** *** 53,58 typedef struct ConnCacheEntry --- 53,61 bool have_prep_stmt; /* have we prepared any stmts in this xact? */ bool have_error; /* have any subxacts aborted in this xact? */ bool changing_xact_state; /* xact state change in process */ + bool invalidated; /* true if reconnect is requried */ + uint32 server_hashvalue; /* hash value of foreign server oid */ + uint32 mapping_hashvalue; /* hash value of user mapping oid */ } ConnCacheEntry; /* *** *** 150,160 GetConnection(UserMapping *user, bool will_prep_stmt) --- 153,180 entry->have_prep_stmt = false; entry->have_error = false; entry->changing_xact_state = false; + entry->invalidated = false; + entry->server_hashvalue = 0; + entry->mapping_hashvalue = 0; } /* Reject further use of connections which failed abort cleanup. */ pgfdw_reject_incomplete_xact_state_change(entry); + + /* + * This connection is no longer valid. Disconnect such connections if no + * transaction is running. We could avoid suporious disconnection by + * examining individual option values but it would be too-much for the + * gain. + */ + if (entry->conn != NULL && entry->invalidated && entry->xact_depth == 0) + { + PQfinish(entry->conn); + entry->conn = NULL; + entry->invalidated = false; + } + /* * We don't check the health of cached connection here, because it would * require some overhead. Broken connection will be detected when the *** *** 173,178 GetConnection(UserMapping *user, bool will_prep_stmt) --- 193,202 entry->xact_depth = 0; /* just to be sure */ entry->have_prep_stmt = false; entry->have_error = false; + entry->server_hashvalue = + GetSysCacheHashValue1(FOREIGNSERVEROID, server->serverid); + entry->mapping_hashvalue
Re: [HACKERS] SCRAM auth and Pgpool-II
> What I am suggesting here is that in order to handle properly SCRAM > with channel binding, pgpool has to provide a different handling for > client <-> pgpool and pgpool <-> Postgres. In short, I don't have a > better answer than having pgpool impersonate the server and request > for a password in cleartext through an encrypted connection between > pgpool and the client if pgpool does not know about it, and then let > pgpool do by itself the SCRAM authentication on a per-connection basis > with each Postgres instances. When using channel binding, what would > matter is the TLS finish (for tls-unique) or the hash server > certificate between Postgres and pgpool, not between the client and > pgpool. But that's actually the point you are raising here: Using a clear text password would not be acceptable for users even through an encrypted connection, I think. Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese:http://www.sraoss.co.jp -- 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] Multi column range partition table
On 12 July 2017 at 10:46, Ashutosh Bapatwrote: > On Wed, Jul 12, 2017 at 12:54 AM, Dean Rasheed > wrote: >> On 11 July 2017 at 13:29, Ashutosh Bapat >>> The description in this paragraph seems to be attaching intuitive >>> meaning of word "unbounded" to MAXVALUE and MINVALUE, which have >>> different intuitive meanings of themselves. Not sure if that's how we >>> should describe MAXVALUE/MINVALUE. >>> >> I'm not sure I understand your point. MINVALUE and MAXVALUE do mean >> unbounded below and above respectively. This paragraph is just making >> the point that that isn't the same as -/+ infinity. >> > What confuses me and probably users is something named min/max"value" > is not a value but something lesser or greater than any other "value". Ah OK, I see what you're saying. It's worth noting though that, after a little looking around, I found that Oracle, MySQL and DB2 all use MINVALUE/MAXVALUE for unbounded range partitions (although in the case of Oracle and MySQL, they currently only support specifying upper bounds, and only use MAXVALUE at the moment). So MINVALUE/MAXVALUE are likely to be familiar to at least some people coming from other databases. Of course, for those other databases, the surrounding syntax for creating partitioned tables is completely different, but at least this makes the bounds themselves portable (our supported set of bounds will be a superset of those supported by Oracle and MySQL, and I think the same as those supported by DB2). I also personally quite like those terms, because they're nice and concise, and it's pretty obvious which is which. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] More flexible LDAP auth search filters?
Hi hackers, A customer asked how to use pg_hba.conf LDAP search+bind authentication to restrict logins to users in one of a small number of groups. ldapsearchattribute only lets you make filters like "(foo=username)", so it couldn't be done. Is there any reason we should allow a more general kind of search filter constructions? A post on planet.postgresql.org today reminded me that a colleague had asked me to post this POC patch here for discussion. It allows custom filters with ldapsearchprefix and ldapsearchsuffix. Another approach might be to take a filter pattern with "%USERNAME%" or whatever in it. There's an existing precedent for the prefix and suffix approach, but on the other hand a pattern approach would allow filters where the username is inserted more than once. Motivating example: ldapsearchprefix="(&(cn=" ldapsearchsuffix = ")(|(memberof=cn=Paris DBA Team)(memberof=cn=Tokyo DBA Team))" Note that with this patch ldapsearchattribute=cn is equivalent to: ldasearchprefix="(cn=" ldapsearchsuffix=")" Perhaps there are better ways to organise your LDAP servers so that this sort of thing isn't necessary. I don't know. Thoughts? -- Thomas Munro http://www.enterprisedb.com ldap-search-filters-v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Fix a typo in pg_upgrade/info.c
Hi, Attached patch for $subject. s/reporing/reporting/g Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center fix_typo_in_info_c.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Update description of \d[S+] in \?
The description of \d[S+] currently does not mention that it will list materialized views and foreign tables. Attached fixes that. Thanks, Amit diff --git a/src/bin/psql/help.c b/src/bin/psql/help.c index b3dbb5946e..66fd8b36f9 100644 --- a/src/bin/psql/help.c +++ b/src/bin/psql/help.c @@ -219,7 +219,7 @@ slashUsage(unsigned short int pager) fprintf(output, _("Informational\n")); fprintf(output, _(" (options: S = show system objects, + = additional detail)\n")); - fprintf(output, _(" \\d[S+] list tables, views, and sequences\n")); + fprintf(output, _(" \\d[S+] list tables, views, materialized views, sequences, and foreign tables\n")); fprintf(output, _(" \\d[S+] NAME describe table, view, sequence, or index\n")); fprintf(output, _(" \\da[S] [PATTERN] list aggregates\n")); fprintf(output, _(" \\dA[+] [PATTERN] list access methods\n")); -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers