Re: [HACKERS] tap tests remove working directories
On 08/07/2015 05:11 PM, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: One of the things that makes the TAP tests very difficult and annoying to debug is their insistence on removing their data directories. I'm not sure why they are doing that. We don't do that with pg_regress. Instead we have clean targets to remove them if necessary. I suggest that we either disable that altogether, and provide cleanup make targets, or at least make it optional, say by setting an environment variable, say TMP_CLEANUP or some such. There is probably a good case for defaulting that to off, but I could live with it being on. I thought we'd decided awhile ago that best practice would be to auto-remove temp directories only on success. Is that a workable behavior for you, or are you concerned about being able to poke around even after the test thinks it succeeded? That certainly isn't what happens, and given the way this is done in TestLib.pm, using the CLEANUP parameter of File::Temp's tempdir() function, it's not clear how we could do that easily. The deletion behaviour is set when you create the directory, not afterwards. What I suggested could be done with a couple of lines of code. I could probably live with your suggestion, especially if I could change the behaviour easily. But what we have now is quite frustrating. I have to hack the source just to be able to diagnose an error. That's really pretty unacceptable. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reduce ProcArrayLock contention
On 2015-08-07 14:20:55 -0400, Jesper Pedersen wrote: On 08/07/2015 02:03 PM, Andres Freund wrote: Confirmed. Running w/o -P x and the problem goes away. Pushed the fix. Thanks for pointing the problem out! - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On Fri, Aug 07, 2015 at 03:18:06PM +0200, Andres Freund wrote: On 2015-08-06 23:23:43 -0400, Noah Misch wrote: On Thu, Aug 06, 2015 at 05:34:50PM +0200, Andres Freund wrote: On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: Andres Freund wrote: @@ -0,0 +1,56 @@ +/*- + * + * lockdefs.h + * Frontend exposed parts of postgres' low level lock mechanism + * + * The split between lockdefs.h and lock.h is not very principled. No kidding! Do you have a good suggestion about the split? I wanted to expose the minimal amount necessary, and those were the ones. lock.h includes lwlock.h only for the benefit of the three LockHashPartition* macros. Those macros are pieces of the lock.c implementation that cross into proc.c, not pieces of the lock.c public API. I argued that way as well upthread. But I do think that Tom has a good point when he argues that frontend code really has no business including lock.h in total. The only reason we need it is because a few headers we need to include have LOCKMODE parameters and/or contain macros that refer to lock levels. So I do think that having a version that doesn't expose any unnecessary details is a good idea. I agree that lock.h offers little to frontend code. Headers that the lockdefs.h patch made usable in the frontend, particularly genam.h and hash.h, are no better. The lock.h/lockdefs.h unprincipled split would do more harm than letting frontends continue to pull in lock.h. If we're going to do something unprincipled, let's make it small, perhaps this: --- a/src/include/storage/lock.h +++ b/src/include/storage/lock.h @@ -17,3 +17,5 @@ #include storage/backendid.h +#ifndef FRONTEND #include storage/lwlock.h +#endif #include storage/shmem.h On another note, I'm perplexed by the high speed commits from this thread. Commit de6fd1c landed just 191 minutes after you posted the first version of its patch. Now lockdefs.h is committed, right in the middle of discussing it. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On 2015-08-07 20:16:20 -0400, Noah Misch wrote: I agree that lock.h offers little to frontend code. Headers that the lockdefs.h patch made usable in the frontend, particularly genam.h and hash.h, are no better. It's not that simple. Those two, and tuptoaster.h, are actually somewhat validly included by frontend code via the rmgr descriptor routines. The lock.h/lockdefs.h unprincipled split would do more harm than letting frontends continue to pull in lock.h. Why? Consider what happens when lock.h/c gets more complicated and e.g. grows some atomics. None of the frontend code should see that, and it's not much effort to keep it that way. Allowing client code to see LOCKMODE isn't something that's going to cause any harm. On another note, I'm perplexed by the high speed commits from this thread. Commit de6fd1c landed just 191 minutes after you posted the first version of its patch. Now lockdefs.h is committed, right in the middle of discussing it. Hm. We'd essentially decided what we're going to do about the inline stuff weeks ago, so I don't feel particularly bad pushing it quickly. A lot of platform dependent stuff like this is going to have some iterations to deal with compilers you don't have access to. The only reason I committed the lockdefs.h split relatively quickly is that I wanted to get the buildfarm green to see wether there are other problems hiding behind the linker error. Which does, so far, not appear to be the case. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
Andres Freund and...@anarazel.de writes: On 2015-08-07 20:16:20 -0400, Noah Misch wrote: On another note, I'm perplexed by the high speed commits from this thread. Commit de6fd1c landed just 191 minutes after you posted the first version of its patch. Now lockdefs.h is committed, right in the middle of discussing it. Hm. We'd essentially decided what we're going to do about the inline stuff weeks ago, so I don't feel particularly bad pushing it quickly. A lot of platform dependent stuff like this is going to have some iterations to deal with compilers you don't have access to. The only reason I committed the lockdefs.h split relatively quickly is that I wanted to get the buildfarm green to see wether there are other problems hiding behind the linker error. Which does, so far, not appear to be the case. FWIW, I agree with that: leaving buildfarm members red for any sustained amount of time is a bad practice. Code cleanliness is something we can argue about at leisure, but if you have critters that aren't building then you don't know what might be hiding behind that. We've had bad experiences in the past with leaving that sort of thing unfixed. 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] Bug? Small samples in TABLESAMPLE SYSTEM returns zero rows
Petr, Just user-tested SYSTEM_ROWS and SYSTEM_TIME. They work as expected. Useful! -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.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] [DESIGN] ParallelAppend
On Fri, Aug 7, 2015 at 2:15 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote: On Sat, Aug 1, 2015 at 6:39 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote: If we pull Funnel here, I think the plan shall be as follows: Funnel -- SeqScan on rel1 -- PartialSeqScan on rel2 -- IndexScan on rel3 So if we go this route, then Funnel should have capability to execute non-parallel part of plan as well, like in this case it should be able to execute non-parallel IndexScan on rel3 as well and then it might need to distinguish between parallel and non-parallel part of plans. I think this could make Funnel node complex. It is difference from what I plan now. In the above example, Funnel node has two non-parallel aware node (rel1 and rel3) and one parallel aware node, then three PlannedStmt for each shall be put on the TOC segment. Even though the background workers pick up a PlannedStmt from the three, only one worker can pick up the PlannedStmt for rel1 and rel3, however, rel2 can be executed by multiple workers simultaneously. Okay, now I got your point, but I think the cost of execution of non-parallel node by additional worker is not small considering the communication cost and setting up an addional worker for each sub-plan (assume the case where out of 100-child nodes only few (2 or 3) nodes actually need multiple workers). It is a competition between traditional Append that takes Funnel children and the new appendable Funnel that takes parallel and non-parallel children. Probably, key factors are cpu_tuple_comm_cost, parallel_setup_cost and degree of selectivity of sub-plans. Both cases has advantage and disadvantage depending on the query, so we can never determine which is better without path consideration. Sure, that is what we should do, however the tricky part would be when the path for doing local scan is extremely cheaper than path for parallel scan for one of the child nodes. For such cases, pulling up Funnel-node can incur more cost. I think some of the other possible ways to make this work could be to extend Funnel so that it is capable of executing both parallel and non-parallel nodes, have a new Funnel like node which has such a capability. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Re: [COMMITTERS] pgsql: Map basebackup tablespaces using a tablespace_map file
On Tue, Aug 4, 2015 at 8:45 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Mon, Aug 3, 2015 at 7:44 PM, Fujii Masao masao.fu...@gmail.com wrote: BTW, while reading the code related to tablespace_map, I found that CancelBackup() emits the WARNING message online backup mode was not canceled when rename() fails. Isn't this confusing (or incorrect)? Yes, it looks confusing. Though this is *not* a major bug, still I feel it is better to do something about it. So ideally, this should be tracked in 9.5 Open Items, but if you think otherwise, then also I think we should track it as part of CF for 9.6, anybody having any opinion? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Raising our compiler requirements for 9.6
On 2015-08-07 14:48:43 -0400, Tom Lane wrote:
Indeed, but the buildfarm results say that the set of such platforms is
nearly empty. It's very likely that a lot of third-party authors won't
ever care if their code doesn't build on such platforms; certainly not
nearly as much as they'll care if their code doesn't build *at all*,
and they have no recourse except to modify the PG headers because they
need some symbol that happens to be defined in a header that also has
some lock-related junk.
I'm all for de-supporting platforms without working inlining support,
but if we do so, we should do it explicitly. Imo that's what it comes
down to.
It's imo more or less a happy optimization accident that apparently all
inlining supporting compilers never emit references from the contents of
static inline functions that aren't referenced.
There is one instance of code that tried to work around that:
#ifndef FRONTEND
#ifndef PG_USE_INLINE
extern MemoryContext MemoryContextSwitchTo(MemoryContext context);
#endif /* !PG_USE_INLINE */
#if defined(PG_USE_INLINE) || defined(MCXT_INCLUDE_DEFINITIONS)
STATIC_IF_INLINE MemoryContext
MemoryContextSwitchTo(MemoryContext context)
{
MemoryContext old = CurrentMemoryContext;
CurrentMemoryContext = context;
return old;
}
#endif /* PG_USE_INLINE || MCXT_INCLUDE_DEFINITIONS */
#endif /* FRONTEND */
You re-added the #ifndef FRONTEND there in a9baeb361d635 referencing a
buildfarm failure. It seems to originally have been added in
7507b193bc54 referencing buildfarm member warthog which unfortunately
doesn't exist anymore...
My point is simply that adding those #errors represents a large bet that
the separation between frontend and backend headers is clean enough.
I really doubt that it is, and I don't see anyone volunteering to put
adequate time into fixing that right now.
I agree that there's a lot of work needed to make that separation
cleaner. I'm not so sure these files are relevantly often needed in
frontend cdoe.
I'm afraid we'll put in ugly, hurried, piecemeal hacks in response to
complaints.
I think we should leave them in for now. It's the beginning of the cycle
and imo the removal of lock.h from the headers where it was referenced
was a good step. We might find some more easy cases - the removal of
lock.h from tuptoaster.h and other headers included by frontend code imo
is a good thing. If it proves to bothersome we can still take it out.
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On 2015-08-06 23:23:43 -0400, Noah Misch wrote: On Thu, Aug 06, 2015 at 05:34:50PM +0200, Andres Freund wrote: On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: Andres Freund wrote: @@ -0,0 +1,56 @@ +/*- + * + * lockdefs.h + *Frontend exposed parts of postgres' low level lock mechanism + * + * The split between lockdefs.h and lock.h is not very principled. No kidding! Do you have a good suggestion about the split? I wanted to expose the minimal amount necessary, and those were the ones. lock.h includes lwlock.h only for the benefit of the three LockHashPartition* macros. Those macros are pieces of the lock.c implementation that cross into proc.c, not pieces of the lock.c public API. I argued that way as well upthread. But I do think that Tom has a good point when he argues that frontend code really has no business including lock.h in total. The only reason we need it is because a few headers we need to include have LOCKMODE parameters and/or contain macros that refer to lock levels. So I do think that having a version that doesn't expose any unnecessary details is a good idea. With that, indirect frontend lock.h inclusion will work fine. But there seems little reason trying to support doing so. It's not very hard to imagine that lock.c and by extension lock.h get more complicated in the future as it's already a scalability bottleneck. that very well might require including atomics, spinlocks and the like in lock.h. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On 2015-08-05 21:39:26 -0400, Noah Misch wrote: On Wed, Aug 05, 2015 at 10:32:48AM -0400, Tom Lane wrote: Andres Freund and...@anarazel.de writes: Wasn't the point that 32 bit AIX as a whole hasn't been supported for a couple years now? My willingness to expend effort for that is rather limited. Well, there's one in the buildfarm. We don't generally turn off buildfarm critters just because the underlying OS is out of support (the population would be quite a bit lower if we did). For the record, mandrill's OS and compiler are both in support and not so old (June 2012 compiler, February 2013 OS). The last 32-bit AIX *kernel* did exit support in 2012, but 32-bit executables remain the norm. Ugh, sorry, I misunderstood you then in the earlier thread. Unless you have a better idea I'll now move the detection of that case to aix.h. I rather liked being able to emit a warning about disabling inlines *once* during configuration, but it's also probably not worth a lot of effort given how few users that platform has. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] ALTER SYSTEM and GUC_LIST_QUOTE
All, While testing some behaviors with ALTER SYSTEM I discovered that GUC parameters with the GUC_LIST_QUOTE flag have a potential issue. As an example, ALTER SYSTEM SET shared_preload_libraries = ''; Results in the following output in postgresql.auto.conf: shared_preload_libraries = ''; Therefore, when attempting to restart postgres the following error is encountered: FATAL: could not access file : No such file or directory As well, specifying multiple items: ALTER SYSTEM SET shared_preload_libraries = 'foo,bar'; Results in: shared_preload_libraries = 'foo,bar'; Which doesn't parse out into separate list items and therefore obviously fails. I think from an ALTER SYSTEM perspective this is an issue, as I would expect to be able to set these types of parameters to any valid value, including an empty list. I'm willing to accept that there might be something here that I am missing or not understanding about how this should work, but this doesn't seem right. Thoughts? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.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] tap tests remove working directories
Andrew Dunstan and...@dunslane.net writes: One of the things that makes the TAP tests very difficult and annoying to debug is their insistence on removing their data directories. I'm not sure why they are doing that. We don't do that with pg_regress. Instead we have clean targets to remove them if necessary. I suggest that we either disable that altogether, and provide cleanup make targets, or at least make it optional, say by setting an environment variable, say TMP_CLEANUP or some such. There is probably a good case for defaulting that to off, but I could live with it being on. I thought we'd decided awhile ago that best practice would be to auto-remove temp directories only on success. Is that a workable behavior for you, or are you concerned about being able to poke around even after the test thinks it succeeded? 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] 9.5 release notes
On 08/08/15 06:43, Bruce Momjian wrote: On Fri, Aug 7, 2015 at 11:53:30AM -0400, Robert Haas wrote: [...] Well, we could just throw a Postgres 9.5 is faster release note item in there and call it a day. ;-) Nah! Just say it is Much Shinier, I'll buy it. Unfortunately, we have to have much more professional sounding reasons to upgrade, to tell our managers - so I guess you DO need more details... [...] (I realize now that compiling the release notes must be a somewhat thankless task, so let me just say that I do appreciate the work you've put into this very much and the comments above shouldn't be understood to take anything away from that. The fact that we don't agree on what the criteria ought to be does not mean that I don't appreciate you doing the work.) Considering the number of almost-arbitrary decisions I have to make to write the major release notes, I am surprised at how few complaints I get. Of course, I have been clearly told by core that no one else wants this job. All joking aside, I appreciate your efforts. I read the release notes, even though currently I don't have an immediate need to use PostgreSQL. Cheers, Gavin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: SCRAM authentication
On Sat, Aug 8, 2015 at 3:45 AM, Heikki Linnakangas hlinn...@iki.fi wrote: On 08/07/2015 09:26 PM, Robert Haas wrote: Maybe I'm chiming in too late here but I am sorta unimpressed by this. If the user's password is stored both MD5-hashed and hashed some other way in the system catalogs, that's less secure than storing it in the least secure of those ways. And I'm afraid that if we introduce this new mechanism, we won't really gain any security, because everybody will just pg_dump or pg_upgrade and the old passwords will stick around in the system forever. In fact we might lose security if somebody changes one password verifier but doesn't realize that the other one is still floating around, memorializing the old password, and still available to be used for login. Yeah, that's certainly a risk. You wouldn't want to keep around verifiers for authentication methods you don't use. Yep, I cannot refute that. And there is actually the same problem with the first version of the patch proposed on this thread if that's what you are referring at below. I think we should look for a solution that either (a) allows SCRAM authentication without requiring any changes to the contents of pg_authid, like what Heikki proposed before; or (b) forces a hard break, where at each password change you can decide if you want the old or new format (probably based on the current value of some compatibility GUC). FWIW, the patch resets all the existing entries should any CREATE/ALTER ROLE involving a password should be run, even if pg_auth_verifiers has entries for method not specified with PASSWORD VERIFIERS. Yeah, something to force a hard break when you want it would be really good. Perhaps a command you can run to remove all MD5 hashes, or at least find all the roles that have them. And a GUC to disallow creating new ones. This filtering machinery definitely looks like a GUC to me, something like password_forbidden_encryption that PASSWORD VERIFIERS looks at and discards the methods listed in there. This definitely needs to be separated from password_encryption. -- 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] Using quicksort and a merge step to significantly improve on tuplesort's single run external sort
On Fri, Jul 31, 2015 at 12:59 AM, Heikki Linnakangas hlinn...@iki.fi wrote: On 07/31/2015 02:01 AM, Peter Geoghegan wrote: What prevents the tuple at the top of the in-memory heap at the point of tuplesort_performsort() (say, one of the ones added to the heap as our glut of memory was*partially* consumed) being less than the last/greatest tuple on tape? If the answer is nothing, a merge step is clearly required. Oh, ok, I was confused on how the heap works. I think I explained this badly, by referencing a secondary reason why we must do a merge. I will now do a better job of explaining why a merge of in-memory and on disk tuples is necessary, for the benefit of other people (I think you get it). The main reason why a merge step is required is that the memtuples array will contain some tuples that were classified as belonging to a second run. Therefore, those tuples may well be lower than the highest on-tape tuples in terms of sort order (in fact, they may be lower than any on-tape tuple). I cannot simply return all tape tuples followed by all in-memory tuples to the caller, and so I must merge, and so only !randomAccess callers may get a quicksort with spillover. I can only get away with **avoiding dumping all tuples** and just merging instead because I reject this determination that a second *traditional/tape* run is needed. I am therefore free of any obligation to merge this would-be traditional second run separately. Another way of explaining it is that I do an all-in-memory merge of some part of the first run, and all the second run (by quicksorting). I then merge this with the original chunk of the first run that is sorted on tape (that was sorted by incremental spilling from the heap). The next version of the patch (the patch may be split in two -- quicksort with spillover, and merge sort optimization) will make sure that any comparisons that go into maintaining the heap invariant are not wasted on the second run, since it will always be quicksorted. We only need to compare the second run tuples pre-quicksort in order to determine that they belong to that run and not the current (first) run. Does that make sense? Have I explained that well? -- 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] Foreign join pushdown vs EvalPlanQual
I could have a discussion with Fujita-san about this topic.
Also, let me share with the discussion towards entire solution.
The primitive reason of this problem is, Scan node with scanrelid==0
represents a relation join that can involve multiple relations, thus,
its TupleDesc of the records will not fit base relations, however,
ExecScanFetch() was not updated when scanrelid==0 gets supported.
FDW/CSP on behalf of the Scan node with scanrelid==0 are responsible
to generate records according to the fdw_/custom_scan_tlist that
reflects the definition of relation join, and only FDW/CSP know how
to combine these base relations.
In addition, host-side expressions (like Plan-qual) are initialized
to reference the records generated by FDW/CSP, so the least invasive
approach is to allow FDW/CSP to have own logic to recheck, I think.
Below is the structure of ExecScanFetch().
ExecScanFetch(ScanState *node,
ExecScanAccessMtd accessMtd,
ExecScanRecheckMtd recheckMtd)
{
EState *estate = node-ps.state;
if (estate-es_epqTuple != NULL)
{
/*
* We are inside an EvalPlanQual recheck. Return the test tuple if
* one is available, after rechecking any access-method-specific
* conditions.
*/
Index scanrelid = ((Scan *) node-ps.plan)-scanrelid;
Assert(scanrelid 0);
if (estate-es_epqTupleSet[scanrelid - 1])
{
TupleTableSlot *slot = node-ss_ScanTupleSlot;
:
return slot;
}
}
return (*accessMtd) (node);
}
When we are inside of EPQ, it fetches a tuple in es_epqTuple[] array and
checks its visibility (ForeignRecheck() always say 'yep, it is visible'),
then ExecScan() applies its qualifiers by ExecQual().
So, as long as FDW/CSP can return a record that satisfies the TupleDesc
of this relation, made by the tuples in es_epqTuple[] array, rest of the
code paths are common.
I have an idea to solve the problem.
It adds recheckMtd() call if scanrelid==0 just before the assertion above,
and add a callback of FDW on ForeignRecheck().
The role of this new callback is to set up the supplied TupleTableSlot
and check its visibility, but does not define how to do this.
It is arbitrarily by FDW driver, like invocation of alternative plan
consists of only built-in logic.
Invocation of alternative plan is one of the most feasible way to
implement EPQ logic on FDW, so I think FDW also needs a mechanism
that takes child path-nodes like custom_paths in CustomPath node.
Once a valid path node is linked to this list, createplan.c transform
them to relevant plan node, then FDW can initialize and invoke this
plan node during execution, like ForeignRecheck().
This design can solve another problem Fujita-san has also mentioned.
If scan qualifier is pushed-down to the remote query and its expression
node is saved in the private area of ForeignScan, the callback on
ForeignRecheck() can evaluate the qualifier by itself. (Note that only
FDW driver can know where and how expression node being pushed-down
is saved in the private area.)
In the summary, the following three enhancements are a straightforward
way to fix up the problem he reported.
1. Add a special path to call recheckMtd in ExecScanFetch if scanrelid==0
2. Add a callback of FDW in ForeignRecheck() - to construct a record
according to the fdw_scan_tlist definition and to evaluate its
visibility, or to evaluate qualifier pushed-down if base relation.
3. Add List *fdw_paths in ForeignPath like custom_paths of CustomPaths,
to construct plan nodes for EPQ evaluation.
On the other hands, we also need to pay attention the development
timeline. It is a really problem of v9.5, however, it looks to me
the straight forward solution needs enhancement of FDW APIs.
I'd like to see people's comment.
--
NEC Business Creation Division / PG-Strom Project
KaiGai Kohei kai...@ak.jp.nec.com
-Original Message-
From: pgsql-hackers-ow...@postgresql.org
[mailto:pgsql-hackers-ow...@postgresql.org] On Behalf Of Kouhei Kaigai
Sent: Saturday, August 01, 2015 10:35 PM
To: Robert Haas; Etsuro Fujita
Cc: PostgreSQL-development; 花田茂
Subject: Re: [HACKERS] Foreign join pushdown vs EvalPlanQual
On Fri, Jul 3, 2015 at 6:25 AM, Etsuro Fujita
fujita.ets...@lab.ntt.co.jp wrote:
Can't FDWs get the join information through the root, which I think we
would
pass to the API as the argument?
This is exactly what Tom suggested originally, and it has some appeal,
but neither KaiGai nor I could see how to make it work . Do you have
an idea? It's not too late to go back and change the API.
The problem that was bothering us (or at least what was bothering me)
is that the PlannerInfo provides only a list of SpecialJoinInfo
structures, which don't directly give you the original join order. In
fact, min_righthand and min_lefthand are intended to
[HACKERS] [sqlsmith] ERROR: too late to create a new PlaceHolderInfo
Hi, on master at 36d9345, the attached queries raised too late to create a new PlaceHolderInfo. regards, Andreas select subq_218206.c0 as c0 from (select rel_1375794.sl_name as c0, coalesce(rel_1375793.f2, rel_1375793.f2) as c1 from public.subselect_tbl as rel_1375793 inner join public.shoe_ready as rel_1375794 on (rel_1375793.f1 = rel_1375794.sh_avail ) where 34 21 fetch first 99 rows only) as subq_218205, lateral (select rel_1375795.b as c0 from public.rtest_t5 as rel_1375795 where rel_1375795.b coalesce(rel_1375795.b, rel_1375795.b) fetch first 122 rows only) as subq_218206 where EXISTS ( select coalesce(rel_1375802.tablename, rel_1375802.schemaname) as c0 from (select rel_1375801.b as c0, rel_1375801.b as c1, rel_1375801.b as c2, rel_1375801.a as c3 from public.rtest_t9 as rel_1375801 where rel_1375801.b ~~ rel_1375801.b fetch first 180 rows only) as subq_218208 left join pg_catalog.pg_policies as rel_1375802 on (subq_218208.c2 = rel_1375802.cmd ) where rel_1375802.qual !~* rel_1375802.cmd fetch first 118 rows only) fetch first 154 rows only; select coalesce(subq_734225.c1, subq_734225.c1) as c0, rel_4650803.srvversion as c1 from (select rel_4650756.name as c0, subq_734226.c0 as c1 from public.street as rel_4650756, lateral (select rel_4650757.conproc as c0 from pg_catalog.pg_conversion as rel_4650757 where rel_4650757.conowner = rel_4650757.connamespace fetch first 52 rows only) as subq_734226 where rel_4650756.name ~~* coalesce(rel_4650756.cname, rel_4650756.cname)) as subq_734225 right join (select rel_4650790.thepath as c0, rel_4650790.name as c1 from public.road as rel_4650790 where rel_4650790.thepath rel_4650790.thepath) as subq_734232 inner join pg_catalog.pg_policies as rel_4650802 right join pg_catalog.pg_foreign_server as rel_4650803 on (rel_4650802.tablename = rel_4650803.srvname ) inner join pg_catalog.pg_roles as rel_4650804 on (rel_4650803.srvtype = rel_4650804.rolpassword ) on (subq_734232.c1 = rel_4650802.cmd ) on (subq_734225.c0 = rel_4650803.srvtype ) where (((rel_4650804.rolname !~ subq_734225.c0) and (((rel_4650802.tablename is NULL) or (rel_4650802.qual !~* subq_734225.c0)) and (subq_734225.c0 ~ subq_734225.c0))) and ((rel_4650802.policyname is not NULL) or (rel_4650802.roles is NULL))) and (rel_4650802.with_check ~~ subq_734225.c0) fetch first 63 rows only; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [DESIGN] ParallelAppend
On Sat, Aug 1, 2015 at 6:39 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote: On Tue, Jul 28, 2015 at 6:08 PM, Kouhei Kaigai kai...@ak.jp.nec.com wrote: I am not sure, but what problem do you see in putting Funnel node for one of the relation scans and not for the others. At this moment, I'm not certain whether background worker can/ought to launch another background workers. If sub-Funnel node is executed by 10-processes then it also launch 10-processes for each, 100-processes will run for each? Yes, that could be more work than current, but what I had in mind is not that way, rather I was thinking that master backend will only kick of workers for Funnel nodes in plan. I agree with, it is fair enough approach, so I mention about pull-up of Funnel node. If we pull Funnel here, I think the plan shall be as follows: Funnel -- SeqScan on rel1 -- PartialSeqScan on rel2 -- IndexScan on rel3 So if we go this route, then Funnel should have capability to execute non-parallel part of plan as well, like in this case it should be able to execute non-parallel IndexScan on rel3 as well and then it might need to distinguish between parallel and non-parallel part of plans. I think this could make Funnel node complex. It is difference from what I plan now. In the above example, Funnel node has two non-parallel aware node (rel1 and rel3) and one parallel aware node, then three PlannedStmt for each shall be put on the TOC segment. Even though the background workers pick up a PlannedStmt from the three, only one worker can pick up the PlannedStmt for rel1 and rel3, however, rel2 can be executed by multiple workers simultaneously. Okay, now I got your point, but I think the cost of execution of non-parallel node by additional worker is not small considering the communication cost and setting up an addional worker for each sub-plan (assume the case where out of 100-child nodes only few (2 or 3) nodes actually need multiple workers). It is a competition between traditional Append that takes Funnel children and the new appendable Funnel that takes parallel and non-parallel children. Probably, key factors are cpu_tuple_comm_cost, parallel_setup_cost and degree of selectivity of sub-plans. Both cases has advantage and disadvantage depending on the query, so we can never determine which is better without path consideration. I think for a particular PlannedStmt, number of workers must have been decided before start of execution, so if those many workers are available to work on that particular PlannedStmt, then next/new worker should work on next PlannedStmt. My concern about pre-determined number of workers is, it depends on the run-time circumstances of concurrent sessions. Even if planner wants to assign 10-workers on a particular sub-plan, only 4-workers may be available on the run-time because of consumption by side sessions. So, I expect only maximum number of workers is meaningful configuration. In that case, there is possibility that many of the workers are just working on one or two of the nodes and other nodes execution might get starved. I understand this is tricky problem to allocate number of workers for different nodes, however we should try to develop any algorithm where there is some degree of fairness in allocation of workers for different nodes. I like to agree, however, I also want to keep the first version as simple as possible we can. We can develop alternative logic to assign suitable number of workers later. 2. Execution of work by workers and Funnel node and then pass the results back to upper node. I think this needs some more work in addition to ParallelSeqScan patch. I expect we can utilize existing infrastructure here. It just picks up the records come from the underlying workers, then raise it to the upper node. Sure, but still you need some work atleast in the area of making workers understand different node types, I am guessing you need to modify readfuncs.c to support new plan node if any for this work. Yes, it was not a creative work. :-) https://github.com/kaigai/sepgsql/blob/fappend/src/backend/nodes/readfuncs.c#L1479 Thanks, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei kai...@ak.jp.nec.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] 9.5 release notes
On Thu, Aug 6, 2015 at 10:24 PM, Bruce Momjian br...@momjian.us wrote: * 2014-10-02 [3acc10c9] Robert..: Increase the number of buffer mapping partition.. should we mention this? This has been patched by a number of downstream vendors and users, so it's probably worth calling out? Uh, I am not sure why general users would care. Because it significantly improves performance on workloads that don't fit into shared_buffers and have concurrency. * 2014-07-14 [91f03ba] Noah M..: MSVC: Recognize PGFILEDESC in contrib and conv.. 2015-04-16 [22d0053] Alvaro..: MSVC: install src/test/modules together with c.. Don't seem to warrant a release note entry. User-visible changes. It seems really strange to me to say that things which improve performance or change query plans are somehow not user-visible, but applying a file description to Windows binaries is user-visible. I am willing to bet you a beverage of your choice that a lot more people are going to notice the performance improvements and planner changes than will ever notice that stuff. (I realize now that compiling the release notes must be a somewhat thankless task, so let me just say that I do appreciate the work you've put into this very much and the comments above shouldn't be understood to take anything away from that. The fact that we don't agree on what the criteria ought to be does not mean that I don't appreciate you doing the work.) -- 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] Raising our compiler requirements for 9.6
On Thu, Aug 6, 2015 at 11:34 AM, Andres Freund and...@anarazel.de wrote: On 2015-08-06 12:29:15 -0300, Alvaro Herrera wrote: Andres Freund wrote: I had to split of three things: LOCKMASK, the individual lock levels and xl_standby_lock to be able to prohibit lock.h to be included by frontend code. lockdefs.h works for me, counter proposals? There weren't any places that needed additional lock.h includes. Ah, but that's because you cheated and didn't remove the include from namespace.h ... Well, it's not included from frontend code, so I didn't see the need? Going through all the backend code and replacing lock.h by lockdefs.h and some other includes doesn't seem particularly beneficial to me. It may not be included from any IN CORE frontend code, but that is not the same thing as saying it's not included from any frontend code at all. For example, EDB has code that includes namespace.h in frontend code. That compiled before this commit; now it doesn't. It turns out the fix is pretty simple: the code in question is including namespace.h, but it doesn't need to. So I can just remove the include in this instance. But I don't think it's always going to be that simple. A significant reduction in the number of headers that can be compiled from frontend code WILL inconvenience extension authors. -- 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] Reduce ProcArrayLock contention
On 08/07/2015 12:41 AM, Amit Kapila wrote: On Thu, Aug 6, 2015 at 9:36 PM, Robert Haas robertmh...@gmail.com wrote: OK, committed. Thank you. Fyi, there is something in pgbench that has caused a testing regression - havn't tracked down what yet. Against 9.6 server (846f8c9483a8f31e45bf949db1721706a2765771) 9.6 pgbench: progress: 10.0 s, 53525.0 tps, lat 1.485 ms stddev 0.523 progress: 20.0 s, 15750.6 tps, lat 5.077 ms stddev 1.950 ... progress: 300.0 s, 15636.9 tps, lat 5.114 ms stddev 1.989 9.5 pgbench: progress: 10.0 s, 50119.5 tps, lat 1.587 ms stddev 0.576 progress: 20.0 s, 51413.1 tps, lat 1.555 ms stddev 0.553 ... progress: 300.0 s, 52951.6 tps, lat 1.509 ms stddev 0.657 Both done with -c 80 -j 80 -M prepared -P 10 -T 300. Just thought I would post it in this thread, because this change does help on the performance numbers compared to 9.5 :) Best regards, Jesper -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reduce ProcArrayLock contention
On Fri, Aug 7, 2015 at 10:30 AM, Jesper Pedersen jesper.peder...@redhat.com wrote: Just thought I would post it in this thread, because this change does help on the performance numbers compared to 9.5 :) So are you saying that the performance was already worse before this patch landed, and then this patch made it somewhat better? Or are you saying you think this patch broke it? -- 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] Raising our compiler requirements for 9.6
On Fri, Aug 07, 2015 at 03:20:00PM +0200, Andres Freund wrote: Unless you have a better idea I'll now move the detection of that case to aix.h. Nope, that seemed right to me. I rather liked being able to emit a warning about disabling inlines *once* during configuration, but it's also probably not worth a lot of effort given how few users that platform has. If we were precisely detecting buggy compilers, the warning would have more value; users could take it as a signal to consider upgrading or downgrading. Given the test's present coarseness, I don't see much value. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5 release notes
On 2015-08-07 11:53:30 -0400, Robert Haas wrote: On Thu, Aug 6, 2015 at 10:24 PM, Bruce Momjian br...@momjian.us wrote: * 2014-10-02 [3acc10c9] Robert..: Increase the number of buffer mapping partition.. should we mention this? This has been patched by a number of downstream vendors and users, so it's probably worth calling out? Uh, I am not sure why general users would care. Because it significantly improves performance on workloads that don't fit into shared_buffers and have concurrency. Exactly. * 2014-07-14 [91f03ba] Noah M..: MSVC: Recognize PGFILEDESC in contrib and conv.. 2015-04-16 [22d0053] Alvaro..: MSVC: install src/test/modules together with c.. Don't seem to warrant a release note entry. User-visible changes. It seems really strange to me to say that things which improve performance or change query plans are somehow not user-visible, but applying a file description to Windows binaries is user-visible. I am willing to bet you a beverage of your choice that a lot more people are going to notice the performance improvements and planner changes than will ever notice that stuff. + many. (I realize now that compiling the release notes must be a somewhat thankless task, so let me just say that I do appreciate the work you've put into this very much and the comments above shouldn't be understood to take anything away from that. The fact that we don't agree on what the criteria ought to be does not mean that I don't appreciate you doing the work.) Same here. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reduce ProcArrayLock contention
On Fri, Aug 7, 2015 at 8:00 PM, Jesper Pedersen jesper.peder...@redhat.com wrote: On 08/07/2015 12:41 AM, Amit Kapila wrote: On Thu, Aug 6, 2015 at 9:36 PM, Robert Haas robertmh...@gmail.com wrote: OK, committed. Thank you. Fyi, there is something in pgbench that has caused a testing regression - havn't tracked down what yet. Against 9.6 server (846f8c9483a8f31e45bf949db1721706a2765771) 9.6 pgbench: progress: 10.0 s, 53525.0 tps, lat 1.485 ms stddev 0.523 progress: 20.0 s, 15750.6 tps, lat 5.077 ms stddev 1.950 ... progress: 300.0 s, 15636.9 tps, lat 5.114 ms stddev 1.989 9.5 pgbench: progress: 10.0 s, 50119.5 tps, lat 1.587 ms stddev 0.576 progress: 20.0 s, 51413.1 tps, lat 1.555 ms stddev 0.553 ... progress: 300.0 s, 52951.6 tps, lat 1.509 ms stddev 0.657 Both done with -c 80 -j 80 -M prepared -P 10 -T 300. I will look into it. Could you please share some of the settings used for test like scale_factor in pgbench and shared_buffers settings or if you have changed any other default setting in postgresql.conf? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Reduce ProcArrayLock contention
On 2015-08-07 20:17:28 +0530, Amit Kapila wrote: On Fri, Aug 7, 2015 at 8:00 PM, Jesper Pedersen jesper.peder...@redhat.com wrote: On 08/07/2015 12:41 AM, Amit Kapila wrote: On Thu, Aug 6, 2015 at 9:36 PM, Robert Haas robertmh...@gmail.com wrote: OK, committed. Thank you. Fyi, there is something in pgbench that has caused a testing regression - havn't tracked down what yet. Against 9.6 server (846f8c9483a8f31e45bf949db1721706a2765771) 9.6 pgbench: progress: 10.0 s, 53525.0 tps, lat 1.485 ms stddev 0.523 progress: 20.0 s, 15750.6 tps, lat 5.077 ms stddev 1.950 ... progress: 300.0 s, 15636.9 tps, lat 5.114 ms stddev 1.989 9.5 pgbench: progress: 10.0 s, 50119.5 tps, lat 1.587 ms stddev 0.576 progress: 20.0 s, 51413.1 tps, lat 1.555 ms stddev 0.553 ... progress: 300.0 s, 52951.6 tps, lat 1.509 ms stddev 0.657 Both done with -c 80 -j 80 -M prepared -P 10 -T 300. I will look into it. Could you please share some of the settings used for test like scale_factor in pgbench and shared_buffers settings or if you have changed any other default setting in postgresql.conf? FWIW, I've seen regressions on my workstation too. I've not yet had time to investigate. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Autonomous Transaction is back
On Thu, Aug 6, 2015 at 11:04 PM, Merlin Moncure mmonc...@gmail.com wrote: I don't necessarily disagree with what you're saying, but it's not clear to me what the proposed behavior is. Since the AT can commit before the outer, ISTM *any* ungranted lock requested by the AT but held by the outer leads to either A: functional deadlock (regardless of implementation details) or B: special behavior. I don't accept that. We've already GOT cases where a query can be suspended and other queries can be running in the same backend. You can do that via cursors. Those cases work fine, and the deadlock detector doesn't know anything about them. How is this any different? -- 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] Raising our compiler requirements for 9.6
Andres Freund wrote: lock.h includes lwlock.h only for the benefit of the three LockHashPartition* macros. Those macros are pieces of the lock.c implementation that cross into proc.c, not pieces of the lock.c public API. I argued that way as well upthread. But I do think that Tom has a good point when he argues that frontend code really has no business including lock.h in total. The only reason we need it is because a few headers we need to include have LOCKMODE parameters and/or contain macros that refer to lock levels. So I do think that having a version that doesn't expose any unnecessary details is a good idea. With that, indirect frontend lock.h inclusion will work fine. But there seems little reason trying to support doing so. It's not very hard to imagine that lock.c and by extension lock.h get more complicated in the future as it's already a scalability bottleneck. that very well might require including atomics, spinlocks and the like in lock.h. I don't disagree with any of your points, but nevertheless I think the split suggested by Noah is a good one (it's more principled than the one you suggest, at any rate) and perhaps it would be a good compromise to do both things in a fell swoop. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On Fri, Aug 7, 2015 at 2:27 PM, Andres Freund and...@anarazel.de wrote: On 2015-08-07 14:15:58 -0400, Robert Haas wrote: On Fri, Aug 7, 2015 at 1:11 PM, Andres Freund and...@anarazel.de wrote: On 2015-08-07 12:30:04 -0400, Robert Haas wrote: It may not be included from any IN CORE frontend code, but that is not the same thing as saying it's not included from any frontend code at all. For example, EDB has code that includes namespace.h in frontend code. That compiled before this commit; now it doesn't. Nothing in namespace.h seems to be of any possible use for frontend code. If there were possible use-cases I'd be inclined to agree, but you obvoiusly can't use any of the functions, the structs and the guc make no sense either. So I really don't why we should cater for that? I think the likelihood of actually breaking correct working extension code that uses namespace.h that'd be broken if we removed lock.h from namespace.h is an order of magnitude bigger than the possible impact on frontend code. Well, I just work here, but it seems silly to me to reorgnize the headers so that you can include fewer definitions where necessary, but then not revise the existing headers to use the slimmed-down versions where possible. Yeah, somebody might have to adjust their #includes and that is annoying, but I don't think the cost of your new #error directives is going to be zero. So first your argument was that it broke stuff and now you want to break more? I am not really against removing removing lock.h from a few more places, but normally you were the one arguing against breaking working code by reorganizing headers when there's no really clear win. Removing lock.h from namespace.h and removing lwlock.h from lock.h will have a noticeably higher cost than what I committed and in contrast to the benefit of separating frontend code from backend implementation details (which caused linker errors) the only benefit will be a bit less code included. Well, we can wait and see if we get any more complaints before doing anything, if you like. We've got a year before any of this is going to be released, so there's no rush. My point, which I think is valid, is that if the set of #includes you need to compile your stuff changes, that's easy to fix. If one of the #includes you need to compile your stuff starts doing #error, you're hosed. -- 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] Raising our compiler requirements for 9.6
Andres Freund and...@anarazel.de writes: On 2015-08-07 14:32:35 -0400, Tom Lane wrote: Eventually I think we're going to have to spend some effort on making a clearer separation between front end safe and not front end safe header files. Until we do that, though, adding these #error directives may just do more harm than good. We don't know which backend headers are being used by third-party code, but we can be 100% sure it's more than what's used by the frontend code in the core distribution. Right now the #errors are in only in places that'd also break without them, but only on the old platforms without inline support and in a more subtle way. Indeed, but the buildfarm results say that the set of such platforms is nearly empty. It's very likely that a lot of third-party authors won't ever care if their code doesn't build on such platforms; certainly not nearly as much as they'll care if their code doesn't build *at all*, and they have no recourse except to modify the PG headers because they need some symbol that happens to be defined in a header that also has some lock-related junk. My point is simply that adding those #errors represents a large bet that the separation between frontend and backend headers is clean enough. I really doubt that it is, and I don't see anyone volunteering to put adequate time into fixing that right now. I'm afraid we'll put in ugly, hurried, piecemeal hacks in response to complaints. I'm ok with getting lock.h from the list of headers where that's the case, done by removing lwlock.h from it, which I proposed that upthread. But then you objected to that approach. Well, we're still discussing what's the best compromise. 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] Reduce ProcArrayLock contention
Hi,
On 2015-08-07 19:30:46 +0200, Andres Freund wrote:
On 2015-08-07 12:49:20 -0400, Jesper Pedersen wrote:
No, this patch helps on performance - there is an improvement in numbers
between
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=253de7e1eb9abbcf57e6c229a8a38abd6455c7de
and
http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0e141c0fbb211bdd23783afa731e3eef95c9ad7a
but you will have to use a 9.5 pgbench to see it, especially with higher
client counts.
Hm, you were using -P X, is that right?
This bisects down to 1bc90f7a7b7441a88e2c6d4a0e9b6f9c1499ad30 - Remove
thread-emulation support from pgbench.
And the apparent reason seems to be that too much code has been removed
in that commit:
@@ -3650,11 +3631,7 @@ threadRun(void *arg)
}
/* also wake up to print the next progress report on time */
- if (progress min_usec 0
-#if !defined(PTHREAD_FORK_EMULATION)
-thread-tid == 0
-#endif /* !PTHREAD_FORK_EMULATION */
- )
+ if (progress min_usec 0)
{
/* get current time if needed */
if (now_usec == 0)
@@ -3710,7 +3687,7 @@ threadRun(void *arg)
This causes all threads but thread 0 (i.e. the primary process) to busy
loop around select: min_usec will be set to 0 once the first progress
report interval has been reached:
if (now_usec = next_report)
min_usec = 0;
else if ((next_report - now_usec) min_usec)
min_usec = next_report - now_usec;
but since we never actually print the progress interval in any thread
but the the main process that's always true from then on:
/* progress report by thread 0 for all threads */
if (progress thread-tid == 0)
{
...
/*
* Ensure that the next report is in the
future, in case
* pgbench/postgres got stuck somewhere.
*/
do
{
next_report += (int64) progress
*100;
} while (now = next_report);
Hrmpf.
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: SCRAM authentication
On Fri, Aug 7, 2015 at 3:22 AM, Michael Paquier michael.paqu...@gmail.com wrote: On Tue, Aug 4, 2015 at 4:20 PM, Michael Paquier wrote: I have been looking more in depths at this one, which adds essential infrastructure to support multiple authentication hashes for more protocols. Here are some comments: [spec lines] I am willing to write a patch for the next CF following more or less those lines, depending of course on the outcome of the discussion we can have here, so feel free to comment. OK, attached as 0001 is the patch that respects those lines for the support of multiple password verifiers in system catalogs. I have added a new catalog called pg_auth_verifiers that is used at authentication to fetch a password value depending on the protocol used. With only this patch attached there are two types of verifiers: plain and md5. This new catalog is REVOKE'd like pg_authid (pg_authid could be made readable be this seems sensitive to me so I am not changing it). I have as well done the following things: - Added PASSWORD VERIFIER (md5 = 'hoge', plain = 'hoge') which is used as well by pg_dump all to be able to specify password verifiers one by one. Maybe I'm chiming in too late here but I am sorta unimpressed by this. If the user's password is stored both MD5-hashed and hashed some other way in the system catalogs, that's less secure than storing it in the least secure of those ways. And I'm afraid that if we introduce this new mechanism, we won't really gain any security, because everybody will just pg_dump or pg_upgrade and the old passwords will stick around in the system forever. In fact we might lose security if somebody changes one password verifier but doesn't realize that the other one is still floating around, memorializing the old password, and still available to be used for login. I think we should look for a solution that either (a) allows SCRAM authentication without requiring any changes to the contents of pg_authid, like what Heikki proposed before; or (b) forces a hard break, where at each password change you can decide if you want the old or new format (probably based on the current value of some compatibility GUC). -- 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] Raising our compiler requirements for 9.6
Robert Haas robertmh...@gmail.com writes: Well, I just work here, but it seems silly to me to reorgnize the headers so that you can include fewer definitions where necessary, but then not revise the existing headers to use the slimmed-down versions where possible. Yeah, somebody might have to adjust their #includes and that is annoying, but I don't think the cost of your new #error directives is going to be zero. I'm a bit concerned about that too; what it means is that any addition of new #includes in backend header files carries a nontrivial risk of breaking frontend code that used to be fine (at least on most platforms). As an example, the proximate cause of the pademelon breakage was that pg_resetxlog needs to #include tuptoaster.h to get TOAST_MAX_CHUNK_SIZE. That was perfectly safe up till commit 2ef085d0e6960f50, when somebody semi-randomly decided that it'd be a good idea to declare a function taking a LOCKMODE argument in that header. Eventually I think we're going to have to spend some effort on making a clearer separation between front end safe and not front end safe header files. Until we do that, though, adding these #error directives may just do more harm than good. We don't know which backend headers are being used by third-party code, but we can be 100% sure it's more than what's used by the frontend code in the core distribution. 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] 9.5 release notes
On Fri, Aug 7, 2015 at 11:53:30AM -0400, Robert Haas wrote: On Thu, Aug 6, 2015 at 10:24 PM, Bruce Momjian br...@momjian.us wrote: * 2014-10-02 [3acc10c9] Robert..: Increase the number of buffer mapping partition.. should we mention this? This has been patched by a number of downstream vendors and users, so it's probably worth calling out? Uh, I am not sure why general users would care. Because it significantly improves performance on workloads that don't fit into shared_buffers and have concurrency. * 2014-07-14 [91f03ba] Noah M..: MSVC: Recognize PGFILEDESC in contrib and conv.. 2015-04-16 [22d0053] Alvaro..: MSVC: install src/test/modules together with c.. Don't seem to warrant a release note entry. User-visible changes. It seems really strange to me to say that things which improve performance or change query plans are somehow not user-visible, but applying a file description to Windows binaries is user-visible. I am willing to bet you a beverage of your choice that a lot more people are going to notice the performance improvements and planner changes than will ever notice that stuff. Well, we could just throw a Postgres 9.5 is faster release note item in there and call it a day. ;-) Of course, there are certain performance items we have to list: user-visible changes, e.g. new features (BRIN), and faster behavior for common operations, i.e. things that will cause users to try things that were too slow in the past. On the opposite end, we have many changes that shave 1% off of query execution time with no change in user behavior, which we probably don't want to list. So, the line is between those somewhere, and the question is where is that line? If two people think this item is above that line, then let's add it. If you can provide text, I can add it. If you have others, we can add those too. What I _am_ saying is that you should use the same criteria I am using, and just disagree on the place for the line, rather than use a different criteria, which will lead to perpetual complaints. We can change the criteria, but that is a different discussion. (I realize now that compiling the release notes must be a somewhat thankless task, so let me just say that I do appreciate the work you've put into this very much and the comments above shouldn't be understood to take anything away from that. The fact that we don't agree on what the criteria ought to be does not mean that I don't appreciate you doing the work.) Considering the number of almost-arbitrary decisions I have to make to write the major release notes, I am surprised at how few complaints I get. Of course, I have been clearly told by core that no one else wants this job. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5 release notes
On 2015-08-07 14:43:11 -0400, Bruce Momjian wrote: Well, we could just throw a Postgres 9.5 is faster release note item in there and call it a day. ;-) Based on my experience one of the prime reason people move to a new version of postgres is performance. And it's not like 'faster!!!' is really helpful for them to evaluate the benefits appropriately. A scalability improvement isn't going to help somebody concerned with single query performance. Somebody concerned about OLTP write performance isn't going to be overly excited about the sort performance improvements, but somebody doing OLAP style queries very much so. The consequence of not listing that is that we're essentially asking our users to read through all the changes between two releases. Something that takes a long while even for people like you and me who are very familiar with the project.. The *by far* biggest complaint about upgrades with postgres isn't binary compatibility (even before pg_upgrade), it's not that there's minor incompatibilities (at least not since 8.3, and maybe bytea_output). It's that previously working queries don't work anymore. It's always just a minority, but they're there. And it's very hard to figure out what's up. Is it stats? Different settings? Planner changes? If we then don't list changes to the planner, they're screwed. In comparison to that just about nobody cares about the rest of the update but new SQL level stuff and a few other major things? A new field in EXPLAIN, debug_assertions read only, effective_io_concurrency settable without effect, VACUUM logs new one more data point, an 10+ year old obsolete undocumented option being removed, new icons for binaries. They just don't matter. With regard to the point about the number of buffer mappings: This has forced peoples sites down. People have found this out themselves and patched postgres. That's an entirely different league. What I _am_ saying is that you should use the same criteria I am using, and just disagree on the place for the line, rather than use a different criteria, which will lead to perpetual complaints. We can change the criteria, but that is a different discussion. We need to change that criteria then. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On 2015-08-07 12:30:04 -0400, Robert Haas wrote: It may not be included from any IN CORE frontend code, but that is not the same thing as saying it's not included from any frontend code at all. For example, EDB has code that includes namespace.h in frontend code. That compiled before this commit; now it doesn't. Nothing in namespace.h seems to be of any possible use for frontend code. If there were possible use-cases I'd be inclined to agree, but you obvoiusly can't use any of the functions, the structs and the guc make no sense either. So I really don't why we should cater for that? I think the likelihood of actually breaking correct working extension code that uses namespace.h that'd be broken if we removed lock.h from namespace.h is an order of magnitude bigger than the possible impact on frontend code. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On 2015-08-07 14:32:35 -0400, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: Well, I just work here, but it seems silly to me to reorgnize the headers so that you can include fewer definitions where necessary, but then not revise the existing headers to use the slimmed-down versions where possible. Yeah, somebody might have to adjust their #includes and that is annoying, but I don't think the cost of your new #error directives is going to be zero. I'm a bit concerned about that too; what it means is that any addition of new #includes in backend header files carries a nontrivial risk of breaking frontend code that used to be fine (at least on most platforms). As an example, the proximate cause of the pademelon breakage was that pg_resetxlog needs to #include tuptoaster.h to get TOAST_MAX_CHUNK_SIZE. That was perfectly safe up till commit 2ef085d0e6960f50, when somebody semi-randomly decided that it'd be a good idea to declare a function taking a LOCKMODE argument in that header. Eventually I think we're going to have to spend some effort on making a clearer separation between front end safe and not front end safe header files. Until we do that, though, adding these #error directives may just do more harm than good. We don't know which backend headers are being used by third-party code, but we can be 100% sure it's more than what's used by the frontend code in the core distribution. Right now the #errors are in only in places that'd also break without them, but only on the old platforms without inline support and in a more subtle way. I'm ok with getting lock.h from the list of headers where that's the case, done by removing lwlock.h from it, which I proposed that upthread. But then you objected to that approach. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reduce ProcArrayLock contention
On 08/07/2015 11:40 AM, Robert Haas wrote: On Fri, Aug 7, 2015 at 10:30 AM, Jesper Pedersen jesper.peder...@redhat.com wrote: Just thought I would post it in this thread, because this change does help on the performance numbers compared to 9.5 :) So are you saying that the performance was already worse before this patch landed, and then this patch made it somewhat better? Or are you saying you think this patch broke it? No, this patch helps on performance - there is an improvement in numbers between http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=253de7e1eb9abbcf57e6c229a8a38abd6455c7de and http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0e141c0fbb211bdd23783afa731e3eef95c9ad7a but you will have to use a 9.5 pgbench to see it, especially with higher client counts. Best regards, Jesper -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: SCRAM authentication
On 08/07/2015 09:26 PM, Robert Haas wrote: Maybe I'm chiming in too late here but I am sorta unimpressed by this. If the user's password is stored both MD5-hashed and hashed some other way in the system catalogs, that's less secure than storing it in the least secure of those ways. And I'm afraid that if we introduce this new mechanism, we won't really gain any security, because everybody will just pg_dump or pg_upgrade and the old passwords will stick around in the system forever. In fact we might lose security if somebody changes one password verifier but doesn't realize that the other one is still floating around, memorializing the old password, and still available to be used for login. Yeah, that's certainly a risk. You wouldn't want to keep around verifiers for authentication methods you don't use. I think we should look for a solution that either (a) allows SCRAM authentication without requiring any changes to the contents of pg_authid, like what Heikki proposed before; or (b) forces a hard break, where at each password change you can decide if you want the old or new format (probably based on the current value of some compatibility GUC). Yeah, something to force a hard break when you want it would be really good. Perhaps a command you can run to remove all MD5 hashes, or at least find all the roles that have them. And a GUC to disallow creating new ones. - 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] Reduce ProcArrayLock contention
On 2015-08-07 12:49:20 -0400, Jesper Pedersen wrote: On 08/07/2015 11:40 AM, Robert Haas wrote: On Fri, Aug 7, 2015 at 10:30 AM, Jesper Pedersen jesper.peder...@redhat.com wrote: Just thought I would post it in this thread, because this change does help on the performance numbers compared to 9.5 :) So are you saying that the performance was already worse before this patch landed, and then this patch made it somewhat better? Or are you saying you think this patch broke it? No, this patch helps on performance - there is an improvement in numbers between http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=253de7e1eb9abbcf57e6c229a8a38abd6455c7de and http://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0e141c0fbb211bdd23783afa731e3eef95c9ad7a but you will have to use a 9.5 pgbench to see it, especially with higher client counts. This bisects down to 1bc90f7a7b7441a88e2c6d4a0e9b6f9c1499ad30 - Remove thread-emulation support from pgbench. Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On Fri, Aug 7, 2015 at 1:11 PM, Andres Freund and...@anarazel.de wrote: On 2015-08-07 12:30:04 -0400, Robert Haas wrote: It may not be included from any IN CORE frontend code, but that is not the same thing as saying it's not included from any frontend code at all. For example, EDB has code that includes namespace.h in frontend code. That compiled before this commit; now it doesn't. Nothing in namespace.h seems to be of any possible use for frontend code. If there were possible use-cases I'd be inclined to agree, but you obvoiusly can't use any of the functions, the structs and the guc make no sense either. So I really don't why we should cater for that? I think the likelihood of actually breaking correct working extension code that uses namespace.h that'd be broken if we removed lock.h from namespace.h is an order of magnitude bigger than the possible impact on frontend code. Well, I just work here, but it seems silly to me to reorgnize the headers so that you can include fewer definitions where necessary, but then not revise the existing headers to use the slimmed-down versions where possible. Yeah, somebody might have to adjust their #includes and that is annoying, but I don't think the cost of your new #error directives is going to be zero. -- 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] Reduce ProcArrayLock contention
On 08/07/2015 02:03 PM, Andres Freund wrote:
but you will have to use a 9.5 pgbench to see it, especially with higher
client counts.
Hm, you were using -P X, is that right?
This bisects down to 1bc90f7a7b7441a88e2c6d4a0e9b6f9c1499ad30 - Remove
thread-emulation support from pgbench.
And the apparent reason seems to be that too much code has been removed
in that commit:
@@ -3650,11 +3631,7 @@ threadRun(void *arg)
}
/* also wake up to print the next progress report on time */
- if (progress min_usec 0
-#if !defined(PTHREAD_FORK_EMULATION)
-thread-tid == 0
-#endif /* !PTHREAD_FORK_EMULATION */
- )
+ if (progress min_usec 0)
{
/* get current time if needed */
if (now_usec == 0)
@@ -3710,7 +3687,7 @@ threadRun(void *arg)
This causes all threads but thread 0 (i.e. the primary process) to busy
loop around select: min_usec will be set to 0 once the first progress
report interval has been reached:
if (now_usec = next_report)
min_usec = 0;
else if ((next_report - now_usec) min_usec)
min_usec = next_report - now_usec;
but since we never actually print the progress interval in any thread
but the the main process that's always true from then on:
/* progress report by thread 0 for all threads */
if (progress thread-tid == 0)
{
...
/*
* Ensure that the next report is in the
future, in case
* pgbench/postgres got stuck somewhere.
*/
do
{
next_report += (int64) progress
*100;
} while (now = next_report);
Hrmpf.
Confirmed.
Running w/o -P x and the problem goes away.
Thanks !
Best regards,
Jesper
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On 2015-08-07 19:11:52 +0200, Andres Freund wrote: I think the likelihood of actually breaking correct working extension code that uses namespace.h that'd be broken if we removed lock.h from namespace.h is an order of magnitude bigger than the possible impact on frontend code. Same with dropping lwlock.h from lock.h. I tried both and the former required more than 20 added headers in backend code, and the latter about 5. I'm pretty sure the same would be true external extensions. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] All-zero page in GIN index causes assertion failure
--
Álvaro Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training Services
diff --git a/src/backend/access/brin/brin_pageops.c b/src/backend/access/brin/brin_pageops.c
index 0b257d9..909e4c6 100644
--- a/src/backend/access/brin/brin_pageops.c
+++ b/src/backend/access/brin/brin_pageops.c
@@ -24,8 +24,9 @@
static Buffer brin_getinsertbuffer(Relation irel, Buffer oldbuf, Size itemsz,
- bool *was_extended);
+ bool *extended);
static Size br_page_get_freespace(Page page);
+static void initialize_empty_new_buffer(Relation idxrel, Buffer buffer);
/*
@@ -53,7 +54,7 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
BrinTuple *oldtup;
Size oldsz;
Buffer newbuf;
- bool extended = false;
+ bool extended;
Assert(newsz == MAXALIGN(newsz));
@@ -64,11 +65,11 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
{
/* need a page on which to put the item */
newbuf = brin_getinsertbuffer(idxrel, oldbuf, newsz, extended);
- /* XXX delay vacuuming FSM until locks are released? */
- if (extended)
- FreeSpaceMapVacuum(idxrel);
if (!BufferIsValid(newbuf))
+ {
+ Assert(!extended);
return false;
+ }
/*
* Note: it's possible (though unlikely) that the returned newbuf is
@@ -76,7 +77,10 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
* buffer does in fact have enough space.
*/
if (newbuf == oldbuf)
+ {
+ Assert(!extended);
newbuf = InvalidBuffer;
+ }
}
else
{
@@ -94,7 +98,13 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
{
LockBuffer(oldbuf, BUFFER_LOCK_UNLOCK);
if (BufferIsValid(newbuf))
+ {
+ if (extended)
+initialize_empty_new_buffer(idxrel, newbuf);
UnlockReleaseBuffer(newbuf);
+ if (extended)
+FreeSpaceMapVacuum(idxrel);
+ }
return false;
}
@@ -106,9 +116,14 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
*/
if (!brin_tuples_equal(oldtup, oldsz, origtup, origsz))
{
- LockBuffer(oldbuf, BUFFER_LOCK_UNLOCK);
if (BufferIsValid(newbuf))
+ {
+ if (extended)
+initialize_empty_new_buffer(idxrel, newbuf);
UnlockReleaseBuffer(newbuf);
+ if (extended)
+FreeSpaceMapVacuum(idxrel);
+ }
return false;
}
@@ -125,7 +140,12 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
brin_can_do_samepage_update(oldbuf, origsz, newsz))
{
if (BufferIsValid(newbuf))
+ {
+ /* as above */
+ if (extended)
+initialize_empty_new_buffer(idxrel, newbuf);
UnlockReleaseBuffer(newbuf);
+ }
START_CRIT_SECTION();
PageIndexDeleteNoCompact(oldpage, oldoff, 1);
@@ -157,6 +177,10 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
END_CRIT_SECTION();
LockBuffer(oldbuf, BUFFER_LOCK_UNLOCK);
+
+ if (extended)
+ FreeSpaceMapVacuum(idxrel);
+
return true;
}
else if (newbuf == InvalidBuffer)
@@ -178,11 +202,21 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
Buffer revmapbuf;
ItemPointerData newtid;
OffsetNumber newoff;
+ BlockNumber newblk = InvalidBlockNumber;
+ Size freespace = 0;
revmapbuf = brinLockRevmapPageForUpdate(revmap, heapBlk);
START_CRIT_SECTION();
+ /*
+ * We need to initialize the page if it's newly obtained. Note we will
+ * WAL-log the initialization as part of the update, so we don't need
+ * to do that here.
+ */
+ if (extended)
+ brin_page_init(BufferGetPage(newbuf), BRIN_PAGETYPE_REGULAR);
+
PageIndexDeleteNoCompact(oldpage, oldoff, 1);
newoff = PageAddItem(newpage, (Item) newtup, newsz,
InvalidOffsetNumber, false, false);
@@ -191,6 +225,13 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
MarkBufferDirty(oldbuf);
MarkBufferDirty(newbuf);
+ /* needed to update FSM below */
+ if (extended)
+ {
+ newblk = BufferGetBlockNumber(newbuf);
+ freespace = br_page_get_freespace(newpage);
+ }
+
ItemPointerSet(newtid, BufferGetBlockNumber(newbuf), newoff);
brinSetHeapBlockItemptr(revmapbuf, pagesPerRange, heapBlk, newtid);
MarkBufferDirty(revmapbuf);
@@ -235,6 +276,14 @@ brin_doupdate(Relation idxrel, BlockNumber pagesPerRange,
LockBuffer(revmapbuf, BUFFER_LOCK_UNLOCK);
LockBuffer(oldbuf, BUFFER_LOCK_UNLOCK);
UnlockReleaseBuffer(newbuf);
+
+ if (extended)
+ {
+ Assert(BlockNumberIsValid(newblk));
+ RecordPageWithFreeSpace(idxrel, newblk, freespace);
+ FreeSpaceMapVacuum(idxrel);
+ }
+
return true;
}
}
@@ -271,7 +320,7 @@ brin_doinsert(Relation idxrel, BlockNumber pagesPerRange,
OffsetNumber off;
Buffer revmapbuf;
ItemPointerData tid;
- bool extended = false;
+ bool extended;
Assert(itemsz == MAXALIGN(itemsz));
@@ -302,7 +351,7 @@ brin_doinsert(Relation idxrel, BlockNumber pagesPerRange,
{
*buffer = brin_getinsertbuffer(idxrel, InvalidBuffer, itemsz, extended);
Assert(BufferIsValid(*buffer));
- Assert(br_page_get_freespace(BufferGetPage(*buffer)) = itemsz);
+ Assert(extended ||
Re: [HACKERS] All-zero page in GIN index causes assertion failure
Heikki Linnakangas wrote: BRIN update is not quite right, however. brin_getinsertbuffer() can initialize a page, but the caller might bail out without using the page and WAL-logging the change. If that happens, the next update that uses the same page will WAL-log the change but it will not use the XLOG_BRIN_INIT_PAGE option, and redo will not initialize the page. Redo will fail. Here's a fix for BRIN: when brin_getinsertbuffer extends the relation and doesn't return the page, it initializes and logs the page as an empty regular page before returning, and also records it into the FSM. That way, some future process that gets a page from FSM will use it, preventing this type of problem from bloating the index forever. Also, this function no longer initializes the page when it returns it to the caller: instead the caller (brin_doinsert or brin_doupdate) must do that. (Previously, the code was initializing the page outside the critical section that WAL-logs this action). BTW, shouldn't there be a step in BRIN vacuum that scans all the BRIN pages? If an empty page is missing from the FSM for any reason, there's nothing to add it there. Probably. I didn't change this part yet. There are two things to fix: 1. since we use log_newpage_buffer(), we log the initialization but not the recording into FSM, so the page would be forgotten about. This can be tested with PageIsEmpty(). An alternative to the vacuum scan is to use our own WAL record that not only logs the initialization itself but also the FSM update. Not sure this is worth the trouble. 2. additionally, if brin_getinsertbuffer extends the relation but we crash before the caller initializes it, the page would be detected by PageIsNew instead and would also need initialization. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Raising our compiler requirements for 9.6
On 2015-08-07 14:15:58 -0400, Robert Haas wrote: On Fri, Aug 7, 2015 at 1:11 PM, Andres Freund and...@anarazel.de wrote: On 2015-08-07 12:30:04 -0400, Robert Haas wrote: It may not be included from any IN CORE frontend code, but that is not the same thing as saying it's not included from any frontend code at all. For example, EDB has code that includes namespace.h in frontend code. That compiled before this commit; now it doesn't. Nothing in namespace.h seems to be of any possible use for frontend code. If there were possible use-cases I'd be inclined to agree, but you obvoiusly can't use any of the functions, the structs and the guc make no sense either. So I really don't why we should cater for that? I think the likelihood of actually breaking correct working extension code that uses namespace.h that'd be broken if we removed lock.h from namespace.h is an order of magnitude bigger than the possible impact on frontend code. Well, I just work here, but it seems silly to me to reorgnize the headers so that you can include fewer definitions where necessary, but then not revise the existing headers to use the slimmed-down versions where possible. Yeah, somebody might have to adjust their #includes and that is annoying, but I don't think the cost of your new #error directives is going to be zero. So first your argument was that it broke stuff and now you want to break more? I am not really against removing removing lock.h from a few more places, but normally you were the one arguing against breaking working code by reorganizing headers when there's no really clear win. Removing lock.h from namespace.h and removing lwlock.h from lock.h will have a noticeably higher cost than what I committed and in contrast to the benefit of separating frontend code from backend implementation details (which caused linker errors) the only benefit will be a bit less code included. Greetings, Andres Freund -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Race conditions in shm_mq.c
On Thu, Aug 6, 2015 at 5:59 PM, Antonin Houska a...@cybertec.at wrote: Robert Haas robertmh...@gmail.com wrote: On Thu, Aug 6, 2015 at 2:38 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Aug 6, 2015 at 10:10 AM, Antonin Houska a...@cybertec.at wrote: During my experiments with parallel workers I sometimes saw the master and worker process blocked. The master uses shm queue to send data to the worker, both sides nowait==false. I concluded that the following happened: The worker process set itself as a receiver on the queue after shm_mq_wait_internal() has completed its first check of ptr, so this function left sender's procLatch in reset state. But before the procLatch was reset, the receiver still managed to read some data and set sender's procLatch to signal the reading, and eventually called its (receiver's) WaitLatch(). So sender has effectively missed the receiver's notification and called WaitLatch() too (if the receiver already waits on its latch, it does not help for sender to call shm_mq_notify_receiver(): receiver won't do anything because there's no new data in the queue). Below is my patch proposal. Another good catch. However, I would prefer to fix this without introducing a continue as I think that will make the control flow clearer. Therefore, I propose the attached variant of your idea. Err, that doesn't work at all. Have a look at this version instead. This makes sense to me. One advantage of continue was that I could apply the patch to my test code (containing the appropriate sleep() calls, to simulate the race conditions) with no conflicts and see the difference. The restructuring you do does not allow for such a mechanical testing, but it's clear enough. OK, I've committed that and back-patched it to 9.5 and 9.4. -- 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] Reduce ProcArrayLock contention
On 08/07/2015 10:47 AM, Amit Kapila wrote: Fyi, there is something in pgbench that has caused a testing regression - havn't tracked down what yet. Against 9.6 server (846f8c9483a8f31e45bf949db1721706a2765771) 9.6 pgbench: progress: 10.0 s, 53525.0 tps, lat 1.485 ms stddev 0.523 progress: 20.0 s, 15750.6 tps, lat 5.077 ms stddev 1.950 ... progress: 300.0 s, 15636.9 tps, lat 5.114 ms stddev 1.989 9.5 pgbench: progress: 10.0 s, 50119.5 tps, lat 1.587 ms stddev 0.576 progress: 20.0 s, 51413.1 tps, lat 1.555 ms stddev 0.553 ... progress: 300.0 s, 52951.6 tps, lat 1.509 ms stddev 0.657 Both done with -c 80 -j 80 -M prepared -P 10 -T 300. I will look into it. Could you please share some of the settings used for test like scale_factor in pgbench and shared_buffers settings or if you have changed any other default setting in postgresql.conf? Compiled with export CFLAGS=-O -fno-omit-frame-pointer ./configure --prefix /opt/postgresql-9.6 --with-openssl --with-gssapi --enable-debug Scale factor is 3000 shared_buffers = 64GB max_prepared_transactions = 10 work_mem = 64MB maintenance_work_mem = 512MB effective_io_concurrency = 4 max_wal_size = 100GB effective_cache_size = 160GB Machine has 28C / 56T with 256Gb mem, and 2 x RAID10 (SSD) Note, that the server setup is the same in both run, just the pgbench version is changed. Latency and stddev goes up between 10.0s and 20.0s when using 9.6 pgbench. Best regards, Jesper -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ALTER SYSTEM and GUC_LIST_QUOTE
Adam Brightwell adam.brightw...@crunchydatasolutions.com writes: While testing some behaviors with ALTER SYSTEM I discovered that GUC parameters with the GUC_LIST_QUOTE flag have a potential issue. As an example, ALTER SYSTEM SET shared_preload_libraries = ''; Results in the following output in postgresql.auto.conf: shared_preload_libraries = ''; Therefore, when attempting to restart postgres the following error is encountered: FATAL: could not access file : No such file or directory I think this is correct. You specified one empty item in the list. As well, specifying multiple items: ALTER SYSTEM SET shared_preload_libraries = 'foo,bar'; Results in: shared_preload_libraries = 'foo,bar'; This one is definitely pilot error; you should have said ALTER SYSTEM SET shared_preload_libraries = foo,bar; or ALTER SYSTEM SET shared_preload_libraries = 'foo','bar'; The quotes make it a single item. There isn't a way to use ALTER SYSTEM SET to set a list variable to an empty list, but that is true of SET as well, and nobody's ever complained. In the ALTER SYSTEM case at least you can use RESET. 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] 9.5 release notes
On Fri, Aug 7, 2015 at 12:02 PM, Andres Freund and...@anarazel.de wrote: Based on my experience one of the prime reason people move to a new version of postgres is performance. And it's not like 'faster!!!' is really helpful for them to evaluate the benefits appropriately. A scalability improvement isn't going to help somebody concerned with single query performance. Somebody concerned about OLTP write performance isn't going to be overly excited about the sort performance improvements, but somebody doing OLAP style queries very much so. The consequence of not listing that is that we're essentially asking our users to read through all the changes between two releases. Something that takes a long while even for people like you and me who are very familiar with the project.. Well put. I don't see much of a downside to having smaller performance improvements listed, provided they're towards the end of the performance section. It is true that many users don't really care much about performance stuff, but among those that give the release notes any more than a cursory look, I bet most care a lot. It's not as if the release notes are the only way, or even the best way of learning about new features for those with a more casual interest. Keeping the features list short and sweet is more of a job for advocacy people that prepare press releases and so on. What I _am_ saying is that you should use the same criteria I am using, and just disagree on the place for the line, rather than use a different criteria, which will lead to perpetual complaints. We can change the criteria, but that is a different discussion. We need to change that criteria then. I strongly agree. -- 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] Raising our compiler requirements for 9.6
Andres Freund wrote: You re-added the #ifndef FRONTEND there in a9baeb361d635 referencing a buildfarm failure. It seems to originally have been added in 7507b193bc54 referencing buildfarm member warthog which unfortunately doesn't exist anymore... FWIW we make a point of not reusing buildfarm member names, so that this kind of history is not completely obliterated. You can still see warthog's history here: http://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=warthogbr=HEAD -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] tap tests remove working directories
One of the things that makes the TAP tests very difficult and annoying to debug is their insistence on removing their data directories. I'm not sure why they are doing that. We don't do that with pg_regress. Instead we have clean targets to remove them if necessary. I suggest that we either disable that altogether, and provide cleanup make targets, or at least make it optional, say by setting an environment variable, say TMP_CLEANUP or some such. There is probably a good case for defaulting that to off, but I could live with it being on. Thoughts? cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
