Re: [HACKERS] assessing parallel-safety
On Mon, Mar 16, 2015 at 02:38:34PM -0400, Robert Haas wrote: > On Sun, Mar 15, 2015 at 2:39 AM, Noah Misch wrote: > > On Thu, Mar 12, 2015 at 11:21:37AM -0400, Robert Haas wrote: > >> On Thu, Feb 19, 2015 at 1:19 AM, Noah Misch wrote: > >> > Rereading my previous message, I failed to make the bottom line clear: I > >> > recommend marking eqsel etc. PROPARALLEL_UNSAFE but *not* checking an > >> > estimator's proparallel before calling it in the planner. > >> > >> But what do these functions do that is actually unsafe? > > > > They call the oprcode function of any operator naming them as an estimator. > > Users can add operators that use eqsel() as an estimator, and we have no > > bound > > on what those operators' oprcode can do. (In practice, parallel-unsafe > > operators using eqsel() as an estimator will be rare.) > > Is there a reason not to make a rule that opclass members must be > parallel-safe? I ask because I think it's important that the process > of planning a query be categorically parallel-safe. If we can't count > on that, life gets a lot more difficult - what happens when we're in a > parallel worker and call a SQL or PL/pgsql function? Neither that rule, nor its variant downthread, would hurt operator authors too much. To make the planner categorically parallel-safe, though, means limiting evaluate_function() to parallel-safe functions. That would dramatically slow selected queries. It's enough for the PL scenario if planning a parallel-safe query is itself parallel-safe. If the planner is parallel-unsafe when planning a parallel-unsafe query, what would suffer? > > RecordTransactionAbort() skips this for subtransaction aborts. I would omit > > it here, because a parallel worker abort is, in this respect, more like a > > subtransaction abort than like a top-level transaction abort. > > No, I don't think so. A subtransaction abort will be followed by > either a toplevel commit or a toplevel abort, so any xlog written by > the subtransaction will be flushed either synchronously or > asynchronously at that time. But for an aborting worker, that's not > true: there's nothing to force the worker's xlog out to disk if it's > ahead of the master's XactLastRecEnd. If our XactLastRecEnd is behind > the master's, then it doesn't matter what we do: an extra flush > attempt is a no-op anyway. If it's ahead, then we need it to be sure > of getting the same behavior that we would have gotten in the > non-parallel case. Fair enough. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
> -Original Message- > From: Shigeru Hanada [mailto:shigeru.han...@gmail.com] > Sent: Monday, March 16, 2015 9:59 PM > To: Robert Haas > Cc: Tom Lane; Thom Brown; Kaigai Kouhei(海外 浩平); pgsql-hackers@postgreSQL.org > Subject: ##freemail## Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] > Custom > Plan API) > > 2015-03-14 7:18 GMT+09:00 Robert Haas : > > I think the foreign data wrapper join pushdown case, which also aims > > to substitute a scan for a join, is interesting to think about, even > > though it's likely to be handled by a new FDW method instead of via > > the hook. Where should the FDW method get called from? Currently, > > the FDW method in KaiGai's patch is GetForeignJoinPaths, and that gets > > called from add_paths_to_joinrel(). The patch at > > > http://www.postgresql.org/message-id/CAEZqfEfy7p=uRpwN-Q-NNgzb8kwHbfqF82YSb9 > ztfzg7zn6...@mail.gmail.com > > uses that to implement join pushdown in postgres_fdw; if you have A > > JOIN B JOIN C all on server X, we'll notice that the join with A and B > > can be turned into a foreign scan on A JOIN B, and similarly for A-C > > and B-C. Then, if it turns out that the cheapest path for A-B is the > > foreign join, and the cheapest path for C is a foreign scan, we'll > > arrive at the idea of a foreign scan on A-B-C, and we'll realize the > > same thing in each of the other combinations as well. So, eventually > > the foreign join gets pushed down. > > From the viewpoint of postgres_fdw, incremental approach seemed > natural way, although postgres_fdw should consider paths in pathlist > in additon to cheapest one as you mentioned in another thread. This > approarch allows FDW to use SQL statement generated for underlying > scans as parts of FROM clause, as postgres_fdw does in the join > push-down patch. > > > But there's another possible approach: suppose that > > join_search_one_level, after considering left-sided and right-sided > > joins and after considering bushy joins, checks whether every relation > > it's got is from the same foreign server, and if so, asks that foreign > > server whether it would like to contribute any paths. Would that be > > better or worse? A disadvantage is that if you've got something like > > A LEFT JOIN B LEFT JOIN C LEFT JOIN D LEFT JOIN E LEFT JOIN F LEFT > > JOIN G LEFT JOIN H LEFT JOIN I but none of the joins can be pushed > > down (say, each join clause calls a non-pushdown-safe function) you'll > > end up examining a pile of joinrels - at every level of the join tree > > - and individually rejecting each one. With the > > build-it-up-incrementally approach, you'll figure that all out at > > level 2, and then after that there's nothing to do but give up > > quickly. On the other hand, I'm afraid the incremental approach might > > miss a trick: consider small LEFT JOIN (big INNER JOIN huge ON big.x = > > huge.x) ON small.y = big.y AND small.z = huge.z, where all three are > > foreign tables on the same server. If the output of the big/huge join > > is big, none of those paths are going to survive at level 2, but the > > overall join size might be very small, so we surely want a chance to > > recover at level 3. (We discussed test cases of this form quite a bit > > in the context of e2fa76d80ba571d4de8992de6386536867250474.) > > Interesting, I overlooked that pattern. As you pointed out, join > between big foregin tables might be dominated, perhaps by a MergeJoin > path. Leaving dominated ForeignPath in pathlist for more optimization > in the future (in higher join level) is an idea, but it would make > planning time longer (and use more cycle and memory). > > Tom's idea sounds good for saving the path b), but I worry that > whether FDW can get enough information at that timing, just before > set_cheapest. It would not be good I/F if each FDW needs to copy many > code form joinrel.c... > I had a call to discuss this topic with Hanada-san. Even though he expected FDW driver needs to check and extract relations involved in a particular join, it also means we have less problem as long as core backend can handle these common portion for all FDW/CSP drivers. Thus, we need care about two hook locations. The first one is add_paths_to_joinrel() as current patch doing, for custom-scan that adds an alternative join logic and takes underlying child nodes as input. The other one is standard_join_search() as Tom pointed out, for foreign-scan of remote join, or for custom-scan that replaces an entire join subtree. One positive aspect of this approach is, postgres_fdw can handle whole-row-reference much simpler than bottom-up approach, according to Hanada-san. Remaining issue is, how to implement the core portion that extracts relations in a particular join, and to identify join type to be applied on a particular relations. One rough idea is, we pull relids bitmap from the target joinrel, then references the SpecialJoinInfo with identical union bitmap of left/righthand. It allows to inform FDW
Re: [HACKERS] Moving Pivotal's Greenplum work upstream
On 13 March 2015 at 06:24, Ewan Higgs wrote: > Hi all, > There has been some press regarding Pivotal's intent to release Greenplum > source as part of an Open Development Platform (along with some of their > Hadoop projects). Can anyone speak on whether any of Greenplum might find > its way upstream? For example, if(!) the work is being released under an > appropriate license, are people at Pivotal planning to push patches for the > parallel architecture and associated query planner upstream? > Greenplum appears from what's visible on the outside to make heavy modifications across a large part of the codebase, doing so with little concern about removing support for existing features, breaking other use cases, etc. So it does what it's meant to do well, but you can't necessarily expect to drop it in place of PostgreSQL and have everything just work. My understanding is that they've written pretty much a new planner/executor for plannable statements, retaining PostgreSQL's parser, protocol code, utility statement handling, etc. But I'm finding it hard to find much hard detail on the system's innards. It's a valid approach, but it's one that means it's unlikely to be practical to just cherry-pick a few features. There's sure to be a lot of divergence between the codebases, and no doubt Greenplum will have implemented infrastructure that overlaps with or duplicates things since added in newer PostgreSQL releases - dynamic shmem, bgworkers, etc. Even if it were feasible to pull in their features with the underlying infrastructure it'd create a significant maintenance burden. So I expect there'd need to be work done to move things over to use PostgreSQL features where they exist. Then there's the fact that Greenplum is based on a heavily modified PostgreSQL 8.2. So even if desirable features were simple standalone patches against 8.2 (which they won't be) there'd be a lot of work getting them forward ported to 9.6. I think it's more realistic that Greenplum's code would serve as an interesting example of how something can be done, and maybe if the license permits parts can be extracted and adapted where it takes less time than rewriting. I wouldn't pin my hopes on seeing a big influx of Greenplum code. I'd love to hear from someone at Pivotal, though, as the above is somewhere between educated guesswork and complete hand-waving. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] Redesigning checkpoint_segments
On Mon, Feb 23, 2015 at 8:56 AM, Heikki Linnakangas wrote: > > Everyone seems to be happy with the names and behaviour of the GUCs, so > committed. The docs suggest that max_wal_size will be respected during archive recovery (causing restartpoints and recycling), but I'm not seeing that happening. Is this a doc bug or an implementation bug? Cheers, Jeff
Re: [HACKERS] Question about TEMP tables
On 16 March 2015 at 16:31, Dmitry Voronin wrote: > Hello, all. > > We can create temp namespaces and temp objects that contains it. So, for > example, temp table will be create at pg_temp_N (N - backendID). But afrer > cluster init we have pg_temp_1 and pg_toast_temp_1 namespaces with OIDs > 11333 and 11334. Those namespaces are visible from any cluster database, > but we cannot create any temp objects (please, correct me). > > This is better suited to the pgsql-general or pgsql-admin mailing lists. Make sure to show your full command(s) and the full, exact text of any errors. -- Craig Ringer http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: [HACKERS] Parallel Seq Scan
On Fri, Mar 13, 2015 at 7:00 PM, Robert Haas wrote: > > On Fri, Mar 13, 2015 at 8:59 AM, Amit Kapila wrote: > > We can't directly call DestroyParallelContext() to terminate workers as > > it can so happen that by that time some of the workers are still not > > started. > > That shouldn't be a problem. TerminateBackgroundWorker() not only > kills an existing worker if there is one, but also tells the > postmaster that if it hasn't started the worker yet, it should not > bother. So at the conclusion of the first loop inside > DestroyParallelContext(), every running worker will have received > SIGTERM and no more workers will be started. > The problem occurs in second loop inside DestroyParallelContext() where it calls WaitForBackgroundWorkerShutdown(). Basically WaitForBackgroundWorkerShutdown() just checks for BGWH_STOPPED status, refer below code in parallel-mode patch: + status = GetBackgroundWorkerPid(handle, &pid); + if (status == BGWH_STOPPED) + return status; So if the status here returned is BGWH_NOT_YET_STARTED, then it will go for WaitLatch and will there forever. I think fix is to check if status is BGWH_STOPPED or BGWH_NOT_YET_STARTED, then just return the status. What do you say? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
[HACKERS] Resetting crash time of background worker
When the postmaster recovers from a backend or worker crash, it resets bg
worker's crash time (rw->rw_crashed_at) so that the bgworker will
immediately restart (ResetBackgroundWorkerCrashTimes).
But resetting rw->rw_crashed_at to 0 means that we have lost the
information that the bgworker had actuallly crashed. So later when
postmaster tries to find any workers that should start
(maybe_start_bgworker), it treats this worker as a new worker, as against
treating it as one that had crashed and is to be restarted. So for this
bgworker, it does not consider BGW_NEVER_RESTART :
if (rw->rw_crashed_at != 0) { if (rw->rw_worker.bgw_restart_time ==
BGW_NEVER_RESTART) { ForgetBackgroundWorker(&iter); continue; }
That means, it will not remove the worker, and it will be restarted. Now if
the worker again crashes, postmaster would keep on repeating the crash and
restart cycle for the whole system.
>From what I understand, BGW_NEVER_RESTART applies even to a crashed server.
But let me know if I am missing anything.
I think we either have to retain the knowledge that the worker has crashed
using some new field, or else, we should reset the crash time only if it is
not flagged BGW_NEVER_RESTART.
-Amit Khandekar
Re: [HACKERS] [PATCH] Add transforms feature
2015-03-17 2:51 GMT+01:00 Peter Eisentraut : > On 3/12/15 8:12 AM, Pavel Stehule wrote: > > 1. fix missing semicolon pg_proc.h > > > > Oid protrftypes[1]; /* types for which to apply > > transforms */ > > Darn, I thought I had fixed that. > > > 2. strange load lib by in sql scripts: > > > > DO '' LANGUAGE plperl; > > SELECT NULL::hstore; > > > > use load plperl; load hstore; instead > > OK > > > 3. missing documentation for new contrib modules, > > OK > > > 4. pg_dump - wrong comment > > > > +<-><-->/* > > +<-><--> * protrftypes was added at v9.4 > > +<-><--> */ > > OK > > > 4. Why guc-use-transforms? Is there some possible negative side effect > > of transformations, so we have to disable it? If somebody don't would to > > use some transformations, then he should not to install some specific > > transformation. > > Well, there was extensive discussion last time around where people > disagreed with that assertion. > I don't like it, but I can accept it - it should not to impact a functionality. > > > 5. I don't understand to motivation for introduction of protrftypes in > > pg_proc and TRANSFORM clause for CREATE FUNCTION - it is not clean from > > documentation, and examples in contribs works without it. Is it this > > functionality really necessary? Missing tests, missing examples. > > Again, this came out from the last round of discussion that people > wanted to select which transforms to use and that the function needs to > remember that choice, so it doesn't depend on whether a transform > happens to be installed or not. Also, it's in the SQL standard that way > (by analogy). > > I am sorry, I didn't discuss this topic and I don't agree so it is good idea. I looked to standard, and I found CREATE TRANSFORM part there. But nothing else. Personally I am thinking, so it is terrible wrong idea, unclean, redundant. If we define TRANSFORM, then we should to use it. Not prepare bypass in same moment. Can be it faster, safer with it? I don't think. Regards Pavel
Re: [HACKERS] One question about security label command
> * Tom Lane (t...@sss.pgh.pa.us) wrote: > > The idea of making the regression test entirely independent of the > > system's policy would presumably solve this problem, so I'd kind of > > like to see progress on that front. > > Apologies, I guess it wasn't clear, but that's what I was intending to > advocate. > OK, I'll try to design a new regression test policy that is independent from the system's policy assumption, like unconfined domain. Please give me time for this work. Thanks, -- NEC OSS Promotion Center / PG-Strom Project KaiGai Kohei > -Original Message- > From: Stephen Frost [mailto:sfr...@snowman.net] > Sent: Monday, March 16, 2015 7:16 AM > To: Tom Lane > Cc: Alvaro Herrera; Kohei KaiGai; Robert Haas; Kaigai Kouhei(海外 浩平); 张元 > 超; pgsql-hackers@postgresql.org > Subject: Re: [HACKERS] One question about security label command > > Tom, > > * Tom Lane (t...@sss.pgh.pa.us) wrote: > > The idea of making the regression test entirely independent of the > > system's policy would presumably solve this problem, so I'd kind of > > like to see progress on that front. > > Apologies, I guess it wasn't clear, but that's what I was intending to > advocate. > > Thanks, > > Stephen -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
2015-03-14 7:18 GMT+09:00 Robert Haas : > I think the foreign data wrapper join pushdown case, which also aims > to substitute a scan for a join, is interesting to think about, even > though it's likely to be handled by a new FDW method instead of via > the hook. Where should the FDW method get called from? Currently, > the FDW method in KaiGai's patch is GetForeignJoinPaths, and that gets > called from add_paths_to_joinrel(). The patch at > http://www.postgresql.org/message-id/CAEZqfEfy7p=urpwn-q-nngzb8kwhbfqf82ysb9ztfzg7zn6...@mail.gmail.com > uses that to implement join pushdown in postgres_fdw; if you have A > JOIN B JOIN C all on server X, we'll notice that the join with A and B > can be turned into a foreign scan on A JOIN B, and similarly for A-C > and B-C. Then, if it turns out that the cheapest path for A-B is the > foreign join, and the cheapest path for C is a foreign scan, we'll > arrive at the idea of a foreign scan on A-B-C, and we'll realize the > same thing in each of the other combinations as well. So, eventually > the foreign join gets pushed down. >From the viewpoint of postgres_fdw, incremental approach seemed natural way, although postgres_fdw should consider paths in pathlist in additon to cheapest one as you mentioned in another thread. This approarch allows FDW to use SQL statement generated for underlying scans as parts of FROM clause, as postgres_fdw does in the join push-down patch. > But there's another possible approach: suppose that > join_search_one_level, after considering left-sided and right-sided > joins and after considering bushy joins, checks whether every relation > it's got is from the same foreign server, and if so, asks that foreign > server whether it would like to contribute any paths. Would that be > better or worse? A disadvantage is that if you've got something like > A LEFT JOIN B LEFT JOIN C LEFT JOIN D LEFT JOIN E LEFT JOIN F LEFT > JOIN G LEFT JOIN H LEFT JOIN I but none of the joins can be pushed > down (say, each join clause calls a non-pushdown-safe function) you'll > end up examining a pile of joinrels - at every level of the join tree > - and individually rejecting each one. With the > build-it-up-incrementally approach, you'll figure that all out at > level 2, and then after that there's nothing to do but give up > quickly. On the other hand, I'm afraid the incremental approach might > miss a trick: consider small LEFT JOIN (big INNER JOIN huge ON big.x = > huge.x) ON small.y = big.y AND small.z = huge.z, where all three are > foreign tables on the same server. If the output of the big/huge join > is big, none of those paths are going to survive at level 2, but the > overall join size might be very small, so we surely want a chance to > recover at level 3. (We discussed test cases of this form quite a bit > in the context of e2fa76d80ba571d4de8992de6386536867250474.) Interesting, I overlooked that pattern. As you pointed out, join between big foregin tables might be dominated, perhaps by a MergeJoin path. Leaving dominated ForeignPath in pathlist for more optimization in the future (in higher join level) is an idea, but it would make planning time longer (and use more cycle and memory). Tom's idea sounds good for saving the path b), but I worry that whether FDW can get enough information at that timing, just before set_cheapest. It would not be good I/F if each FDW needs to copy many code form joinrel.c... -- Shigeru HANADA -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] OOM-killer issue when updating a inheritance table which has large number of child tables
>At the moment, partitioning into thousands of tables is not supported. Thank you for your reply. And thanks Tom Lane and Stephen Frost！ The following(with createsql.sql and update.sql as attachment) is my complete test case. And i reproduced this problem in PostgreSQL 9.4.1 . 1)create table and data createdb db1000 psql -q -v total=1000 -v pnum=1000 -f createsql.sql |psql db1000 psql -c "insert into maintb values(1,'abcde12345')" db1000 2)update the parent table with one connection, 955MB memory has been used. [chenhj@node2 part]$ pgbench -c 1 -n -T 10 -r -f update.sql db1000; transaction type: Custom query scaling factor: 1 query mode: simple number of clients: 1 number of threads: 1 duration: 10 s number of transactions actually processed: 20 tps = 1.933407 (including connections establishing) tps = 1.934807 (excluding connections establishing) statement latencies in milliseconds: 516.836800update maintb set name = 'a12345' where id=1; part of output from "top" when runing pgbench: ... PID USER PR NI VIRT RES SHR S %CPU %MEMTIME+ COMMAND 22537 chenhj20 0 955m 667m 11m R 99.4 33.3 0:06.12 postgres 3)update the parent table with ten connections simultaneously, OOM ocurrs. Now,to run pgbench 955MB * 10 memory are needed,but my machine only has 2GB physical memory and 4GB Swap. [chenhj@node2 part]$ pgbench -c 10 -n -T 2 -r -f update.sql db1000; Client 0 aborted in state 0. Probably the backend died while processing. WARNING: terminating connection because of crash of another server process DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeat your command. WARNING: terminating connection because of crash of another server process DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeat your command. WARNING: terminating connection because of crash of another server process DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeat your command. WARNING: terminating connection because of crash of another server process DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeat your command. WARNING: terminating connection because of crash of another server process DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeat your command. WARNING: terminating connection because of crash of another server process DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeat your command. Client 3 aborted in state 0. Probably the backend died while processing. Client 6 aborted in state 0. Probably the backend died while processing. Client 1 aborted in state 0. Probably the backend died while processing. Client 5 aborted in state 0. Probably the backend died while processing. Client 8 aborted in state 0. Probably the backend died while processing. Client 9 aborted in state 0. Probably the backend died while processing. WARNING: terminating connection because of crash of another server process DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeat your command. Client 7 aborted in state 0. Probably the backend died while processing. WARNING: terminating connection because of crash of another server process DETAIL: The postmaster has commanded this server process to roll back the current transaction and exit, because another server process exited abnormally and possibly corrupted shared memory. HINT: In a moment you should be able to reconnect to the database and repeat your command. Client 4 aborted in state 0. Probably the backend di
Re: [HACKERS] get_object_address support for additional object types
I pushed a fix for this. -- Á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] [PATCH] Add transforms feature
On 3/12/15 8:12 AM, Pavel Stehule wrote: > 1. fix missing semicolon pg_proc.h > > Oid protrftypes[1]; /* types for which to apply > transforms */ Darn, I thought I had fixed that. > 2. strange load lib by in sql scripts: > > DO '' LANGUAGE plperl; > SELECT NULL::hstore; > > use load plperl; load hstore; instead OK > 3. missing documentation for new contrib modules, OK > 4. pg_dump - wrong comment > > +<-><-->/* > +<-><--> * protrftypes was added at v9.4 > +<-><--> */ OK > 4. Why guc-use-transforms? Is there some possible negative side effect > of transformations, so we have to disable it? If somebody don't would to > use some transformations, then he should not to install some specific > transformation. Well, there was extensive discussion last time around where people disagreed with that assertion. > 5. I don't understand to motivation for introduction of protrftypes in > pg_proc and TRANSFORM clause for CREATE FUNCTION - it is not clean from > documentation, and examples in contribs works without it. Is it this > functionality really necessary? Missing tests, missing examples. Again, this came out from the last round of discussion that people wanted to select which transforms to use and that the function needs to remember that choice, so it doesn't depend on whether a transform happens to be installed or not. Also, it's in the SQL standard that way (by analogy). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ranlib bleating about dirmod.o being empty
On 3/14/15 12:58 PM, Tom Lane wrote: > I'm getting rather tired of reading these warning messages in OS X builds: > > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: > file: libpgport.a(dirmod.o) has no symbols > /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: > file: libpgport_srv.a(dirmod_srv.o) has no symbols > > The reason for this warning is that dirmod.o contains no functions that > aren't Windows-specific. As such, it seems like putting it into the > list of "always compiled" port modules is really a mistake. Any > objections to switching it to be built only on Windows? It looks like ar isn't even the preferred method to build static libraries on OS X anymore. Instead, one should use libtool (not GNU libtool), which has a -no_warning_for_no_symbols option. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] get_object_address support for additional object types
On 16 March 2015 at 15:11, Alvaro Herrera wrote: >> > Actually, on second thought I revisited this and changed the >> > representation for opfamilies and opclasses: instead of putting the AM >> > name in objargs, we can put it as the first element of objname instead. >> > That way, objargs is unused for opfamilies and opclasses, and we're free >> > to use it for the type arguments in amops and amprocs. This makes the >> > lists consistent for the four cases: in objname, amname first, then >> > qualified opclass/opfamily name. For amop/amproc, the member number >> > follows. Objargs is unused in opclass/opfamily, and it's a two-element >> > list of types in amop/amproc. >> >> Agreed, that makes more sense to me also. > > Great, thanks for checking -- pushed that way. > I'm getting a couple of compiler warnings that I think are coming from this commit: objectaddress.c: In function ‘get_object_address’: objectaddress.c:1428:12: warning: array subscript is above array bounds objectaddress.c:1430:11: warning: array subscript is above array bounds I think because the compiler doesn't know the list size, so i could be 0,1,2. Regards, Dean -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Additional role attributes && superuser review
Stephen Frost writes: > * Tom Lane (t...@sss.pgh.pa.us) wrote: >> That seems fairly ugly. Why would we need a new, duplicative function >> here? (Apologies if the reasoning was spelled out upthread, I've not >> been paying much attention.) > Currently, those functions allow users to signal backends which are > owned by them, which means they can be used by anyone. Simply > REVOKE'ing access to them would remove that capability and an admin who > then GRANT's access to the function would need to understand that > they're allowing that user the ability to cancel/terminate any backends > (except those initiated by superusers, at least if we keep that check as > discussed upthread). > If those functions just had simply superuser() checks that prevented > anyone else from using them then this wouldn't be an issue. > REVOKE'ing access *without* removing the permissions checks would defeat > the intent of these changes, which is to allow an administrator to grant > the ability for a certain set of users to cancel and/or terminate > backends started by other users, without also granting those users > superuser rights. I see: we have two different use-cases and no way for GRANT/REVOKE to manage both cases using permissions on a single object. Carry on then. 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] Improving RLS qual pushdown
I took another look at this and came up with some alternate comment
rewording. I also added a couple of additional comments that should
hopefully clarify the code a bit.
Regards,
Dean
diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c
new file mode 100644
index 58d78e6..9e05fa8
*** a/src/backend/optimizer/path/allpaths.c
--- b/src/backend/optimizer/path/allpaths.c
*** targetIsInAllPartitionLists(TargetEntry
*** 1982,1988
* 2. If unsafeVolatile is set, the qual must not contain any volatile
* functions.
*
! * 3. If unsafeLeaky is set, the qual must not contain any leaky functions.
*
* 4. The qual must not refer to the whole-row output of the subquery
* (since there is no easy way to name that within the subquery itself).
--- 1982,1990
* 2. If unsafeVolatile is set, the qual must not contain any volatile
* functions.
*
! * 3. If unsafeLeaky is set, the qual must not contain any leaky functions
! * that are passed Var nodes, and therefore might reveal values from the
! * subquery as side effects.
*
* 4. The qual must not refer to the whole-row output of the subquery
* (since there is no easy way to name that within the subquery itself).
*** qual_is_pushdown_safe(Query *subquery, I
*** 2009,2015
/* Refuse leaky quals if told to (point 3) */
if (safetyInfo->unsafeLeaky &&
! contain_leaky_functions(qual))
return false;
/*
--- 2011,2017
/* Refuse leaky quals if told to (point 3) */
if (safetyInfo->unsafeLeaky &&
! contain_leaked_vars(qual))
return false;
/*
diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
new file mode 100644
index 84d58ae..0098375
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*** static bool contain_mutable_functions_wa
*** 97,103
static bool contain_volatile_functions_walker(Node *node, void *context);
static bool contain_volatile_functions_not_nextval_walker(Node *node, void *context);
static bool contain_nonstrict_functions_walker(Node *node, void *context);
! static bool contain_leaky_functions_walker(Node *node, void *context);
static Relids find_nonnullable_rels_walker(Node *node, bool top_level);
static List *find_nonnullable_vars_walker(Node *node, bool top_level);
static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK);
--- 97,103
static bool contain_volatile_functions_walker(Node *node, void *context);
static bool contain_volatile_functions_not_nextval_walker(Node *node, void *context);
static bool contain_nonstrict_functions_walker(Node *node, void *context);
! static bool contain_leaked_vars_walker(Node *node, void *context);
static Relids find_nonnullable_rels_walker(Node *node, bool top_level);
static List *find_nonnullable_vars_walker(Node *node, bool top_level);
static bool is_strict_saop(ScalarArrayOpExpr *expr, bool falseOK);
*** contain_nonstrict_functions_walker(Node
*** 1318,1343
}
/*
! * Check clauses for non-leakproof functions
*/
/*
! * contain_leaky_functions
! * Recursively search for leaky functions within a clause.
*
! * Returns true if any function call with side-effect may be present in the
! * clause. Qualifiers from outside the a security_barrier view should not
! * be pushed down into the view, lest the contents of tuples intended to be
! * filtered out be revealed via side effects.
*/
bool
! contain_leaky_functions(Node *clause)
{
! return contain_leaky_functions_walker(clause, NULL);
}
static bool
! contain_leaky_functions_walker(Node *node, void *context)
{
if (node == NULL)
return false;
--- 1318,1347
}
/*
! * Check clauses for Vars passed to non-leakproof functions
*/
/*
! * contain_leaked_vars
! * Recursively scan a clause to discover whether it contains any Var
! * nodes (of the current query level) that are passed as arguments to
! * leaky functions.
*
! * Returns true if the clause contains any non-leakproof functions that are
! * passed Var nodes of the current query level, and which might therefore leak
! * data. Qualifiers from outside a security_barrier view that might leak data
! * in this way should not be pushed down into the view in case the contents of
! * tuples intended to be filtered out by the view are revealed by the leaky
! * functions.
*/
bool
! contain_leaked_vars(Node *clause)
{
! return contain_leaked_vars_walker(clause, NULL);
}
static bool
! contain_leaked_vars_walker(Node *node, void *context)
{
if (node ==
Re: [HACKERS] Additional role attributes && superuser review
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > ... Lastly, there is the question of pg_cancel_backend and > > pg_terminate_backend. My thinking on this is to create a new > > 'pg_signal_backend' which admins could grant access to and leave the > > existing functions alone (modulo the change for has_privs_of_role as > > discussed previously). We'd rename the current 'pg_signal_backend' to > > something else (maybe '_helper'); it's not exposed anywhere and > > therefore renaming it shouldn't cause any heartache. > > That seems fairly ugly. Why would we need a new, duplicative function > here? (Apologies if the reasoning was spelled out upthread, I've not > been paying much attention.) Currently, those functions allow users to signal backends which are owned by them, which means they can be used by anyone. Simply REVOKE'ing access to them would remove that capability and an admin who then GRANT's access to the function would need to understand that they're allowing that user the ability to cancel/terminate any backends (except those initiated by superusers, at least if we keep that check as discussed upthread). If those functions just had simply superuser() checks that prevented anyone else from using them then this wouldn't be an issue. REVOKE'ing access *without* removing the permissions checks would defeat the intent of these changes, which is to allow an administrator to grant the ability for a certain set of users to cancel and/or terminate backends started by other users, without also granting those users superuser rights. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Additional role attributes && superuser review
Stephen Frost writes: > ... Lastly, there is the question of pg_cancel_backend and > pg_terminate_backend. My thinking on this is to create a new > 'pg_signal_backend' which admins could grant access to and leave the > existing functions alone (modulo the change for has_privs_of_role as > discussed previously). We'd rename the current 'pg_signal_backend' to > something else (maybe '_helper'); it's not exposed anywhere and > therefore renaming it shouldn't cause any heartache. That seems fairly ugly. Why would we need a new, duplicative function here? (Apologies if the reasoning was spelled out upthread, I've not been paying much attention.) regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_xlog_replay_resume() considered armed and dangerous
On 3/12/15 10:08 AM, Andres Freund wrote: I think this, at the very least, needs a big caveat in the documentation of the resume function. But a different API would probably be better. I'd actually argue that for now pg_xlog_replay_resume() should refuse to work if paused due to a recovery target. Promotion should be done via the normal promotion methods. +1. "replay resume" certainly doesn't make me think "promote". -- Jim Nasby, Data Architect, Blue Treble Consulting Data in Trouble? Get it in Treble! http://BlueTreble.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] Additional role attributes && superuser review
All, * Stephen Frost (sfr...@snowman.net) wrote: > Alright, I've got an initial patch to do this for pg_start/stop_backup, > pg_switch_xlog, and pg_create_restore_point. The actual backend changes > are quite small, as expected. I'll add in the changes for the other > functions being discussed and adapt the documentation changes from > the earlier patch to make sense, but what I'd really appreciate are any > comments or thoughts regarding the changes to pg_dump (which are generic > to all of the function changes, of course). So, I've tested this approach with extensions and binary upgrade and things appear to all be working correctly, but there's a few issues remaining to discuss: The functions pg_start_backup and pg_stop_backup can currently be run by replication roles but those roles won't have any permissions on those functions with the new approach unless we GRANT those rights during pg_dump and/or pg_upgrade. Note that this isn't an issue for tools which use the replication protocol (eg: pg_basebackup) since the replication protocol calls do_pg_start_bacup() directly. As such, my first question is- do we want to worry about this? We should certainly mention it in the release notes but I'm not sure if we really want to do anything else. The second question is in regards to pg_stat_activity. My first suggestion for how to address that would be to have the function return everything and then have the view perform the filtering to return only what's shown today for users. Admins could then grant access to the function for whichever users they want to have access to everything. That strikes me as the best way to avoid changing common usage while still providing the flexibility. Another option would be to still invent the MONITOR role attribute and keep the rest the same. Again, we'd want to mention something in the release notes regarding this change. Lastly, there is the question of pg_cancel_backend and pg_terminate_backend. My thinking on this is to create a new 'pg_signal_backend' which admins could grant access to and leave the existing functions alone (modulo the change for has_privs_of_role as discussed previously). We'd rename the current 'pg_signal_backend' to something else (maybe '_helper'); it's not exposed anywhere and therefore renaming it shouldn't cause any heartache. For pg_create_restore_point, pg_switch_xlog, pg_xlog_replay_pause, pg_xlog_replay_resume, pg_rotate_logfile, we can just remove the if(!superuser()) ereport() checks and REVOKE rights on them during the initdb process, since there isn't anything complicated happening for those today. One other question which I've been thinking about is if we want to try and preserve permission changes associated with other catalog objects (besides functions), or if we even want to change how functions are handled regarding permission changes. We don't currently preserve any such changes across dump/restore or even binary upgrades and this would be changing that. I don't recall any complaints about not preserving permission changes to objects in pg_catalog, but one could argue that our lack of doing so today is a bug. Thoughts? Thanks! Stephen signature.asc Description: Digital signature
[HACKERS] Future directions for inheritance-hierarchy statistics
A few days ago I posted a very-much-WIP patch for making the planner dynamically combine statistics for each member of an appendrel: http://www.postgresql.org/message-id/22598.1425686...@sss.pgh.pa.us That patch was only intended to handle the case of an appendrel generated by a UNION ALL construct. But it occurs to me that we could easily change it to also apply to appendrels generated from inheritance trees. Then we'd no longer need the whole-inheritance-tree statistics that ANALYZE currently produces, because we'd only ever look at per-table statistics in pg_statistic. This would have one significant drawback, which is that planning for large inheritance trees (many children) would probably get noticeably slower. (But in the common case that constraint exclusion limits a query to scanning just one or a few children, the hit would be small.) On the other hand, there would be two very significant benefits. First, that we would automatically get statistics that account for partitions being eliminated by constraint exclusion, because only the non-eliminated partitions are present in the appendrel. And second, that we'd be able to forget the whole problem of getting autovacuum to create whole-inheritance-tree stats. Right now I'm doubtful that typical users are getting good up-to-date stats at all for queries of this sort, because autovacuum will only update those stats if it decides it needs to analyze the parent table. Which is commonly empty, so that there's never a reason to fire an analyze on it. (We'd left this as a problem to be solved later when we put in the whole-tree stats feature in 9.0, but no progress has been made on solving it.) So I think that going in this direction is clearly a win and we ought to pursue it. It's not happening for 9.5 of course, because there's still a great deal of work to do before anything like this would be committable. But I would like to establish a consensus that this would be a sensible thing to do in 9.6. The reason I bring it up now is that the inheritance-for-foreign-tables patch has some code that I don't much like for controlling what happens with those whole-tree stats when some of the children are foreign tables that lack ANALYZE support. If the long-term plan is that whole-tree stats are going away altogether, then it won't be terribly important exactly what happens in that case, so we can just do some simple/easy kluge in the short term and not have to have an argument about what's the best thing to do. Comments? 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] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 16, 2015 at 11:11 AM, Greg Stark wrote: > > On Mon, Mar 16, 2015 at 5:46 PM, David G. Johnston < > david.g.johns...@gmail.com> wrote: > >> Why not just leave the double-quoting requirements intact. An unquoted >> or (etc) would represent the special keyword while the >> quoted version would mean that the name is used literally. >> > > For users that would be worse than not quoting. Then if they look up users > they can't say WHERE username =ANY (users). They would have to do > sumersaults like CASE WHEN username = 'all' then '"all"' =ANY (users) else > username =ALL (users). > > The whole point of having a view should be that you don't need to know the > syntax rules for pg_hba.conf to interpret the data. If you do then you > might as well just write a parser and read the file. > > > Create a "pg_hba_user" type, and an implicit cast from text to that type, so when you say: "WHERE 'any' = ANY(...)" the system does the syntax conversion for you and the user doesn't have to, for the most part, be aware of the special rules for quoting names. Otherwise I don't see how you can meet the requirement to accommodate "any" as a valid user identifier without using two columns - one for keywords and one for users using the quoting rules of PostgreSQL pg_role instead of using the, more restrictive, rules of pg_hba.conf In that case I would not leave the users column with an empty array when "any" is specified but would incorporate all known roles into the value; though maybe it is just noise during typical usage...it would likely be a long field that would be useful for querying but not necessarily for display. David J.
Re: [HACKERS] assessing parallel-safety
Robert Haas writes: > On Mon, Mar 16, 2015 at 2:55 PM, Tom Lane wrote: >> Robert Haas writes: >>> Is there a reason not to make a rule that opclass members must be >>> parallel-safe? I ask because I think it's important that the process >>> of planning a query be categorically parallel-safe. >> I'm not seeing the connection between those two statements. The planner >> doesn't usually execute opclass members, at least not as such. > Hmm, I guess I'm spouting nonsense there. The way the operator gets > invoked during planning is that eqsel() calls it. But that doesn't > require it to be part of an opclass; it just has to be an operator > that's chosen that eqsel as its selectivity estimator. Yeah. So what we'd want here is a rule that selectivity estimator functions must be parallel-safe. For operators using estimators similar to eqsel() that would imply a requirement on the operator's function as well, but it's the estimator not any opclass connection that creates that requirement. 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] assessing parallel-safety
On Mon, Mar 16, 2015 at 2:55 PM, Tom Lane wrote: > Robert Haas writes: >> Is there a reason not to make a rule that opclass members must be >> parallel-safe? I ask because I think it's important that the process >> of planning a query be categorically parallel-safe. > > I'm not seeing the connection between those two statements. The planner > doesn't usually execute opclass members, at least not as such. Hmm, I guess I'm spouting nonsense there. The way the operator gets invoked during planning is that eqsel() calls it. But that doesn't require it to be part of an opclass; it just has to be an operator that's chosen that eqsel as its selectivity estimator. -- 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] assessing parallel-safety
Robert Haas writes: > Is there a reason not to make a rule that opclass members must be > parallel-safe? I ask because I think it's important that the process > of planning a query be categorically parallel-safe. I'm not seeing the connection between those two statements. The planner doesn't usually execute opclass members, at least not as such. 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] How to create shared_ptr for PGconn?
2015-03-16 19:32 GMT+03:00 Chengyu Fan :
> Hi,
>
> Does anyone know how to create the shared_ptr for PGconn? I always get
> compile errors ...
>
> Below is my code:
> ---
> const char * dbInfo = "xxx";
> PGconn *conn = PGconnectdb(dbInfo);
> if (PGstatus(conn) != CONNECTION_OK) {
> std::cout << PQerrorMessage(conn) << std::endl;
> return 1;
> }
>
> std::shared_ptr pConn(conn);
> ---
>
> Error messages for the code above:
>
> *main.cpp:153:27: **note: *in instantiation of function template
> specialization 'std::__1::shared_ptr::shared_ptr'
> requested here
>
> std::shared_ptr pConn(rawConn);
>
>
> */usr/local/Cellar/postgresql/9.4.1/include/libpq-fe.h:129:16: note: *forward
> declaration of 'pg_conn'
>
> typedef struct pg_conn PGconn;
>
The complete example below:
#include
#include
int main(int argc, char *argv[])
{
PGconn* cn = PQconnectdb("");
std::shared_ptr shared_cn(cn, &PQfinish);
return 0;
}
--
// Dmitry.
Re: [HACKERS] assessing parallel-safety
On Sun, Mar 15, 2015 at 2:39 AM, Noah Misch wrote: > On Thu, Mar 12, 2015 at 11:21:37AM -0400, Robert Haas wrote: >> On Thu, Feb 19, 2015 at 1:19 AM, Noah Misch wrote: >> > Rereading my previous message, I failed to make the bottom line clear: I >> > recommend marking eqsel etc. PROPARALLEL_UNSAFE but *not* checking an >> > estimator's proparallel before calling it in the planner. >> >> But what do these functions do that is actually unsafe? > > They call the oprcode function of any operator naming them as an estimator. > Users can add operators that use eqsel() as an estimator, and we have no bound > on what those operators' oprcode can do. (In practice, parallel-unsafe > operators using eqsel() as an estimator will be rare.) Is there a reason not to make a rule that opclass members must be parallel-safe? I ask because I think it's important that the process of planning a query be categorically parallel-safe. If we can't count on that, life gets a lot more difficult - what happens when we're in a parallel worker and call a SQL or PL/pgsql function? Thanks for reviewing the incremental patch. A few comments: > Exactly. At entry to RecordTransactionCommit(), XactLastRecEnd must point > past the end of all records whose replay is required to satisfy the user's > expectation of transaction durability. At other times, harm from its value > being wrong is negligible. I do suggest adding a comment to its definition > explaining when one can rely on it. Good idea. > RecordTransactionAbort() skips this for subtransaction aborts. I would omit > it here, because a parallel worker abort is, in this respect, more like a > subtransaction abort than like a top-level transaction abort. No, I don't think so. A subtransaction abort will be followed by either a toplevel commit or a toplevel abort, so any xlog written by the subtransaction will be flushed either synchronously or asynchronously at that time. But for an aborting worker, that's not true: there's nothing to force the worker's xlog out to disk if it's ahead of the master's XactLastRecEnd. If our XactLastRecEnd is behind the master's, then it doesn't matter what we do: an extra flush attempt is a no-op anyway. If it's ahead, then we need it to be sure of getting the same behavior that we would have gotten in the non-parallel case. > I took this to mean that workers normally persist until the master commits a > transaction. Rather, their lifecycle is like the lifecycle of a buffer pin. > When an error interrupts a parallel operation, transaction abort destroys > workers. Otherwise, the code driving the specific parallel task destroys them > as early as is practical. (Strictly to catch bugs, transaction commit does > detect and destroy leaked workers.) Good point; will revise. -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 16, 2015 at 5:46 PM, David G. Johnston < david.g.johns...@gmail.com> wrote: > Why not just leave the double-quoting requirements intact. An unquoted > or (etc) would represent the special keyword while the > quoted version would mean that the name is used literally. > For users that would be worse than not quoting. Then if they look up users they can't say WHERE username =ANY (users). They would have to do sumersaults like CASE WHEN username = 'all' then '"all"' =ANY (users) else username =ALL (users). The whole point of having a view should be that you don't need to know the syntax rules for pg_hba.conf to interpret the data. If you do then you might as well just write a parser and read the file. -- greg
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 16, 2015 at 1:46 PM, David G. Johnston wrote: > Why not just leave the double-quoting requirements intact. An unquoted > or (etc) would represent the special keyword while the > quoted version would mean that the name is used literally. That would be OK with me, I think. > I'm not totally opposed to using some other quoting symbol to represent > keywords as well - like "*" (e.g., *all*). Like Greg, I'm not to keen on > the idea of adding additional keyword columns. We probably should not use one quoting convention in the file and another in the view. -- 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] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 16, 2015 at 9:29 AM, Alvaro Herrera wrote: > Robert Haas wrote: > > On Wed, Mar 11, 2015 at 1:32 PM, Greg Stark wrote: > > > I think what we have here is already a good semantic representation. It > > > doesn't handle all the corner cases but those corner cases are a) very > > > unlikely and b) easy to check for. A tool can check for any users > starting > > > with + or named "all" or any databases called "sameuser" or > "samerole". If > > > they exist then the view isn't good enough to reconstruct the raw > file. But > > > they're very unlikely to exist, I've never heard of anyone with such > things > > > and can't imagine why someone would make them. > > > > -1. Like Peter, I think this is a bad plan. Somebody looking at the > > view should be able to understand with 100% confidence, and without > > additional parsing, what the semantics of the pg_hba.conf file are. > > Saying "those cases are unlikely so we're not going to handle them" is > > really selling ourselves short. > > +1 what Robert said. I think the additional "keyword" columns are a > good solution to the issue. > > Why not just leave the double-quoting requirements intact. An unquoted or (etc) would represent the special keyword while the quoted version would mean that the name is used literally. Have the: "A separate file containing [database|user] names can be specified by preceding the file name with @" possibilities been added to the test suite? I'm not totally opposed to using some other quoting symbol to represent keywords as well - like "*" (e.g., *all*). Like Greg, I'm not to keen on the idea of adding additional keyword columns. *Logical View - Keyword Expansion* My other thought was to not use the keywords at all and simply resolve their meaning to actual role/database names and list them explicitly. That would be a true logical view and allow for simple user checking by saying: 'username' = ANY(users); without the need for ([...] OR '*any*' = ANY(users) or similar). If going that route I guess it would behoove us to either incorporate a "physical" view of the actual file converted to a table or to then maybe have some kind of "tags" fields with the special values encoded should someone want to know how the user list was generated. In effect, the "pg_hba" view in the original file but with actual names of users and databases instead of empty arrays. The "sameuser" => "all" pairing is a pure physical representation in that instance. What I would do in a logical representation is have a single record for each user that has a database of the same name in the current database. However, because of that limitation you either would be duplicating information by keeping "sameuser,all" or you would have to have a separate view representing the physical view of the file. I think having two views, one logical and one physical, would solve the two use cases nicely without having to compromise or incorporate too much redundancy and complexity. Likewise, if we know the host ip address and subnet mask the keywords "samehost" and "samenet" should be resolvable to actual values at the time of viewing. Though that would add the complication of multi-network listening... I guess a full logical view is a bit much but if we keep the same quoting mechanics as mandated by the file then there should be no ambiguity in terms of label meaning and whomever is looking at this stuff has to understand about the keywords and using quote to remove the special-ness anyway. David J.
Re: [HACKERS] CATUPDATE confusion?
All, Sure, if we never deprecate anything then tool authors would never need > to change their existing code. I don't think that's actually a viable > solution though; the reason we're discussing the removal of these > particular views is that they aren't really being maintained and, when > they are, they're making work for us. That's certainly a trade-off to > consider, of course, but in this case I'm coming down on the side of > dropping support and our own maintenance costs associated with these > views in favor of asking the tooling community to complete the migration > to the new views which have been around for the past 10 years. > Perhaps this is naive or has been attempted in the past without success, but would it be possible to maintain a list of deprecated features? I noticed the following wiki page, (though it hasn't been updated recently) that I think could be used for this purpose. https://wiki.postgresql.org/wiki/Deprecated_Features Using this page as a model, having an "official deprecation list" that does the following might be very useful: * Lists feature that is deprecated. * Reason it was deprecated. * What to use instead, perhaps with example. * Version the feature will be removed. Or perhaps such a list could be included as part of the official documentation? In either case, if it is well known that such a list is available/exists then tool developers, etc. should have adequate time, opportunity and information to make the appropriate changes to their products with a "minimal" impact. Thoughts? -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
On Mon, Mar 16, 2015 at 4:29 PM, Alvaro Herrera wrote: > +1 what Robert said. I think the additional "keyword" columns are a > good solution to the issue. > Huh. Well I disagree but obviously I'm in the minority. I'll put fix it up accordingly today and post the resulting view output (which I expect will look like the example in the previous message). I think it's cumbersome but it shouldn't be hard to implement. -- greg
Re: [HACKERS] Reduce pinning in btree indexes
On Mon, Mar 16, 2015 at 12:07 PM, Simon Riggs wrote: >> What Simon actually proposed was by *bloat*; that is, if the overall >> amount of bloat in the system exceeds some metric, start whacking old >> queries in order to control it. The appeal of that is obvious, > > Agreed > >> but I >> think it would be very hard to make work, because canceling queries >> won't get rid of the bloat you've already introduced, and so you'd >> tend to cancel nothing until you hit some threshold, and then start >> canceling everything. > > That would just be an unhelpful way of using that info. There are many > possible designs that wouldn't need to work that way. > > We have many cases where we begin a cleanup action before we run out > of space for other server resources. VACUUM is itself a good example, > but there are many others. > > The main point is that if we blindly decide things based upon xid age > or time then it will be wrong in many cases, since apps with uniform > write rates are rare. We need to work out how to limit bloat itself. > If we can't do that easily at a macro level, then we'll have to do > that more precisely at the table level, for example having VACUUM > decide where to put the limit if we find too much unremovable/recently > dead data. I am sure there are more sophisticated things to be done here, but I guess my feeling is that time is a good way to go here for a first cut - lots of people have suggested it, and there's clearly a use case for it. If the setting turns out to be popular, we can fine-tune it more once we've got some experience with it. But I'm nervous about trying to to design something more complicated than that right now, especially so late in the release cycle. We know that this setting, with time-based units, will meet the needs of the customer for whom it was developed, and that is a good-enough reason to think that time is a reasonable metric for this, even if we eventually have others. -- 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] get_object_address support for additional object types
* Andres Freund (and...@2ndquadrant.com) wrote: > On 2015-03-16 12:50:13 -0300, Alvaro Herrera wrote: > > Well, we already have make targets for gcov and friends; you get some > > HTML charts and marked-up source lines with coverage counts, etc. I > > don't think we've made any use of that. It'd be neat to have something > > similar to our doxygen service, running some test suite and publishing > > the reports on the web. I remember trying to convince someone to set > > that up for the community, but that seems to have yield no results. > > Actually I think Peter E. has: > http://pgci.eisentraut.org/jenkins/job/postgresql_master_coverage/Coverage/ Neat! Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] get_object_address support for additional object types
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > I don't think we've made use of it either. If the targets/code are > > already there to make it happen and it's just a matter of having a > > system running which is generating the website then I can probably get > > that going. I was under the impression that there was more to it than > > that though. > > "configure --enable-coverage"; install the resulting tree; then run > whatever tests you want, then "make coverage". That's enough to get the > HTML reports. Ok, cool, I'll take a look at that. > > > We had someone else trying to submit patches to improve coverage of the > > > regression tests, but (probably due to wrong stars alignment) they > > > started with CREATE DATABASE which made the tests a lot slower, which > > > got the patches turned down -- the submitter disappeared after that > > > IIRC, probably discouraged by the lack of results. > > > > Agreed, and I think that's unfortunate. It's an area which we could > > really improve in and would be a good place for someone new to the > > community to be able to contribute- but we need to provide the right way > > for those tests to be added and that way isn't to include them in the > > main suite of tests which are run during development. > > Well, I don't disagree that we could do with some tests that are run > additionally to the ones we currently have, but some basic stuff that > takes almost no time to run would be adequate to have in the basic > regression tests. The one I'm thinking is something like generate a > VALUES of all the supported object types, then running > "pg_get_object_address" on them to make sure we support all object types > (or at least that we're aware which object types are not supported.) Agreed, that sounds perfectly reasonable to me. I didn't mean to imply that we shouldn't add tests where they make sense, including to the core regression suites (particularly coverage tests like that one you're suggesting here), just pointing out that we need a way to address code coverage based tested also which is done outside of the main regression suite. > For instance, Petr Jelinek's patch to add the TABLESAMPLE clause adds a > new object type which needs support in get_object_address, but I'm not > sure it's added to the stuff in the object_address test. It's a very > easy omission to make. Agreed. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] get_object_address support for additional object types
On 2015-03-16 12:50:13 -0300, Alvaro Herrera wrote: > Well, we already have make targets for gcov and friends; you get some > HTML charts and marked-up source lines with coverage counts, etc. I > don't think we've made any use of that. It'd be neat to have something > similar to our doxygen service, running some test suite and publishing > the reports on the web. I remember trying to convince someone to set > that up for the community, but that seems to have yield no results. Actually I think Peter E. has: http://pgci.eisentraut.org/jenkins/job/postgresql_master_coverage/Coverage/ > We had someone else trying to submit patches to improve coverage of the > regression tests, but (probably due to wrong stars alignment) they > started with CREATE DATABASE which made the tests a lot slower, which > got the patches turned down -- the submitter disappeared after that > IIRC, probably discouraged by the lack of results. I seem to recall that he'd also submitted a bunch of other things, and that some of it was applied? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] How to create shared_ptr for PGconn?
Hi,
Does anyone know how to create the shared_ptr for PGconn? I always get compile
errors ...
Below is my code:---const char * dbInfo = "xxx";PGconn
*conn = PGconnectdb(dbInfo);if (PGstatus(conn) != CONNECTION_OK) {std::cout
<< PQerrorMessage(conn) << std::endl;return 1;}
std::shared_ptr pConn(conn);---
Error messages for the code above:
main.cpp:153:27: note: in instantiation of function template specialization
'std::__1::shared_ptr::shared_ptr' requested here
std::shared_ptr pConn(rawConn);
/usr/local/Cellar/postgresql/9.4.1/include/libpq-fe.h:129:16: note: forward
declaration of 'pg_conn'
typedef struct pg_conn PGconn;
Thanks,Chengyu
Re: [HACKERS] Providing catalog view to pg_hba.conf file - Patch submission
Robert Haas wrote: > On Wed, Mar 11, 2015 at 1:32 PM, Greg Stark wrote: > > I think what we have here is already a good semantic representation. It > > doesn't handle all the corner cases but those corner cases are a) very > > unlikely and b) easy to check for. A tool can check for any users starting > > with + or named "all" or any databases called "sameuser" or "samerole". If > > they exist then the view isn't good enough to reconstruct the raw file. But > > they're very unlikely to exist, I've never heard of anyone with such things > > and can't imagine why someone would make them. > > -1. Like Peter, I think this is a bad plan. Somebody looking at the > view should be able to understand with 100% confidence, and without > additional parsing, what the semantics of the pg_hba.conf file are. > Saying "those cases are unlikely so we're not going to handle them" is > really selling ourselves short. +1 what Robert said. I think the additional "keyword" columns are a good solution to the issue. -- Á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] Allow "snapshot too old" error, to prevent bloat
On Mon, Mar 16, 2015 at 11:46 AM, Robert Haas wrote: > On Sun, Mar 15, 2015 at 8:04 PM, Rui DeSousa wrote: > > Would a parameter to auto terminate long running transactions be a > better solution? Terminating the long running transaction would allow for > the normal vacuuming process to cleanup the deleted records thus avoiding > database bloat without introducing new semantics to Postgres's MVCC. I > would also recommend that the default be disabled. > > An advantage of Kevin's approach is that you don't have to abort *all* > long-running transactions, only those that touch data which has been > modified since then. If your system is read-mostly, that could > dramatically reduce the number of aborts. > > Indeed. The suggestion of auto-terminating long running transactions misses the point, ISTM. Most of the use cases I can see for this involve vacuuming tables that the long running transaction will have no interest in at all (which is why I suggested being able to set it on a per table basis). I certainly don't want to be terminating my long running report transaction so that my queue table which is only ever touched by very short-lived transactions can be reasonably vacuumed. But that's what we have to do now. cheers andrew
Re: [HACKERS] get_object_address support for additional object types
Stephen Frost wrote: > Alvaro, > > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > > Well, we already have make targets for gcov and friends; you get some > > HTML charts and marked-up source lines with coverage counts, etc. I > > don't think we've made any use of that. It'd be neat to have something > > similar to our doxygen service, running some test suite and publishing > > the reports on the web. I remember trying to convince someone to set > > that up for the community, but that seems to have yield no results. > > I don't think we've made use of it either. If the targets/code are > already there to make it happen and it's just a matter of having a > system running which is generating the website then I can probably get > that going. I was under the impression that there was more to it than > that though. "configure --enable-coverage"; install the resulting tree; then run whatever tests you want, then "make coverage". That's enough to get the HTML reports. > > We had someone else trying to submit patches to improve coverage of the > > regression tests, but (probably due to wrong stars alignment) they > > started with CREATE DATABASE which made the tests a lot slower, which > > got the patches turned down -- the submitter disappeared after that > > IIRC, probably discouraged by the lack of results. > > Agreed, and I think that's unfortunate. It's an area which we could > really improve in and would be a good place for someone new to the > community to be able to contribute- but we need to provide the right way > for those tests to be added and that way isn't to include them in the > main suite of tests which are run during development. Well, I don't disagree that we could do with some tests that are run additionally to the ones we currently have, but some basic stuff that takes almost no time to run would be adequate to have in the basic regression tests. The one I'm thinking is something like generate a VALUES of all the supported object types, then running "pg_get_object_address" on them to make sure we support all object types (or at least that we're aware which object types are not supported.) For instance, Petr Jelinek's patch to add the TABLESAMPLE clause adds a new object type which needs support in get_object_address, but I'm not sure it's added to the stuff in the object_address test. It's a very easy omission to make. -- Á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] Reduce pinning in btree indexes
On 16 March 2015 at 13:02, Robert Haas wrote: > I'm not sure what the value of basing this on WAL volume is, and I > think we should talk about that a bit more first. The value of doing > this by rate of XID consumption is easy to understand: it's simple to > implement. The value of doing this by time is also simple to > understand: if you set the setting to 20 minutes, then you can be sure > that queries will not get cancelled unless they run for more than 20 > minutes. This clearly makes the system easier to reason about. I > don't see what the benefit of doing this by WAL volume would be. I think we can assess that more clearly as the amount of now-dead space left by an action. So we can include Updates and Deletes, or in the case of aborted transactions, the total WAL written. I assumed that was what Kevin meant. > What Simon actually proposed was by *bloat*; that is, if the overall > amount of bloat in the system exceeds some metric, start whacking old > queries in order to control it. The appeal of that is obvious, Agreed > but I > think it would be very hard to make work, because canceling queries > won't get rid of the bloat you've already introduced, and so you'd > tend to cancel nothing until you hit some threshold, and then start > canceling everything. That would just be an unhelpful way of using that info. There are many possible designs that wouldn't need to work that way. We have many cases where we begin a cleanup action before we run out of space for other server resources. VACUUM is itself a good example, but there are many others. The main point is that if we blindly decide things based upon xid age or time then it will be wrong in many cases, since apps with uniform write rates are rare. We need to work out how to limit bloat itself. If we can't do that easily at a macro level, then we'll have to do that more precisely at the table level, for example having VACUUM decide where to put the limit if we find too much unremovable/recently dead data. > If we want this feature, let's try to keep the spec simple enough that > we actually get it. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, 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] Providing catalog view to pg_hba.conf file - Patch submission
On Wed, Mar 11, 2015 at 1:32 PM, Greg Stark wrote: > I think what we have here is already a good semantic representation. It > doesn't handle all the corner cases but those corner cases are a) very > unlikely and b) easy to check for. A tool can check for any users starting > with + or named "all" or any databases called "sameuser" or "samerole". If > they exist then the view isn't good enough to reconstruct the raw file. But > they're very unlikely to exist, I've never heard of anyone with such things > and can't imagine why someone would make them. -1. Like Peter, I think this is a bad plan. Somebody looking at the view should be able to understand with 100% confidence, and without additional parsing, what the semantics of the pg_hba.conf file are. Saying "those cases are unlikely so we're not going to handle them" is really selling ourselves short. -- 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] CATUPDATE confusion?
* Robert Haas (robertmh...@gmail.com) wrote: > On Fri, Mar 13, 2015 at 9:52 AM, Stephen Frost wrote: > > * Stephen Frost (sfr...@snowman.net) wrote: > >> I should have been more specific. I don't believe they've moved to > >> using pg_roles completely (which was created specifically to address the > >> issue that regular users can't select from pg_authid). > > > > Err, forgot to finish that thought, sorry. Let's try again: > > > > I should have been more specific. I don't believe they've moved to > > using pg_roles completely (which was created specifically to address the > > issue that regular users can't select from pg_authid) simply because > > they haven't had reason to. > > That's another way of saying "removing pg_user would be creating extra > work for tool authors that otherwise wouldn't need to be done". Sure, if we never deprecate anything then tool authors would never need to change their existing code. I don't think that's actually a viable solution though; the reason we're discussing the removal of these particular views is that they aren't really being maintained and, when they are, they're making work for us. That's certainly a trade-off to consider, of course, but in this case I'm coming down on the side of dropping support and our own maintenance costs associated with these views in favor of asking the tooling community to complete the migration to the new views which have been around for the past 10 years. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] get_object_address support for additional object types
Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > It'd certainly be great to have more testing in general, but we're not > > going to be able to include complete code coverage tests in the normal > > set of regression tests which are run.. What I've been thinking for a > > while (probably influenced by other discussion) is that we should have > > the buildfarm running tests for code coverage as those are async from > > the development process. That isn't to say we shouldn't add more tests > > to the main regression suite when it makes sense to, we certainly > > should, but we really need to be looking at code coverage tools and > > adding tests to improve our test coverage which can be run by the > > buildfarm animals (or even just a subset of them, if we feel that having > > all the animals running them would be excessive). > > Well, we already have make targets for gcov and friends; you get some > HTML charts and marked-up source lines with coverage counts, etc. I > don't think we've made any use of that. It'd be neat to have something > similar to our doxygen service, running some test suite and publishing > the reports on the web. I remember trying to convince someone to set > that up for the community, but that seems to have yield no results. I don't think we've made use of it either. If the targets/code are already there to make it happen and it's just a matter of having a system running which is generating the website then I can probably get that going. I was under the impression that there was more to it than that though. > We had someone else trying to submit patches to improve coverage of the > regression tests, but (probably due to wrong stars alignment) they > started with CREATE DATABASE which made the tests a lot slower, which > got the patches turned down -- the submitter disappeared after that > IIRC, probably discouraged by the lack of results. Agreed, and I think that's unfortunate. It's an area which we could really improve in and would be a good place for someone new to the community to be able to contribute- but we need to provide the right way for those tests to be added and that way isn't to include them in the main suite of tests which are run during development. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] CATUPDATE confusion?
On Fri, Mar 13, 2015 at 9:52 AM, Stephen Frost wrote: > * Stephen Frost (sfr...@snowman.net) wrote: >> I should have been more specific. I don't believe they've moved to >> using pg_roles completely (which was created specifically to address the >> issue that regular users can't select from pg_authid). > > Err, forgot to finish that thought, sorry. Let's try again: > > I should have been more specific. I don't believe they've moved to > using pg_roles completely (which was created specifically to address the > issue that regular users can't select from pg_authid) simply because > they haven't had reason to. That's another way of saying "removing pg_user would be creating extra work for tool authors that otherwise wouldn't need to be done". -- 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] get_object_address support for additional object types
Stephen Frost wrote: > It'd certainly be great to have more testing in general, but we're not > going to be able to include complete code coverage tests in the normal > set of regression tests which are run.. What I've been thinking for a > while (probably influenced by other discussion) is that we should have > the buildfarm running tests for code coverage as those are async from > the development process. That isn't to say we shouldn't add more tests > to the main regression suite when it makes sense to, we certainly > should, but we really need to be looking at code coverage tools and > adding tests to improve our test coverage which can be run by the > buildfarm animals (or even just a subset of them, if we feel that having > all the animals running them would be excessive). Well, we already have make targets for gcov and friends; you get some HTML charts and marked-up source lines with coverage counts, etc. I don't think we've made any use of that. It'd be neat to have something similar to our doxygen service, running some test suite and publishing the reports on the web. I remember trying to convince someone to set that up for the community, but that seems to have yield no results. We had someone else trying to submit patches to improve coverage of the regression tests, but (probably due to wrong stars alignment) they started with CREATE DATABASE which made the tests a lot slower, which got the patches turned down -- the submitter disappeared after that IIRC, probably discouraged by the lack of results. -- Á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] Allow "snapshot too old" error, to prevent bloat
On Sun, Mar 15, 2015 at 8:04 PM, Rui DeSousa wrote: > Would a parameter to auto terminate long running transactions be a better > solution? Terminating the long running transaction would allow for the normal > vacuuming process to cleanup the deleted records thus avoiding database bloat > without introducing new semantics to Postgres's MVCC. I would also recommend > that the default be disabled. An advantage of Kevin's approach is that you don't have to abort *all* long-running transactions, only those that touch data which has been modified since then. If your system is read-mostly, that could dramatically reduce the number of aborts. -- 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] get_object_address support for additional object types
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Actually, there was a bug in the changes of the rule for ALTER EXTENSION > ADD OPERATOR CLASS. I noticed by chance only, and upon testing it > manually I realized I had made a mistake. I then remembered I made the > same bug previously, fixed by 5c5ffee80f35, and I'm not wondering why do > we not have any test for ALTER EXTENSION ADD other than pg_upgrading > some database that contains an extension which uses each command. This > seems pretty dangerous to me, generally speaking ... we should > definitely be testing all these ALTER EXTENSION commands. It'd certainly be great to have more testing in general, but we're not going to be able to include complete code coverage tests in the normal set of regression tests which are run.. What I've been thinking for a while (probably influenced by other discussion) is that we should have the buildfarm running tests for code coverage as those are async from the development process. That isn't to say we shouldn't add more tests to the main regression suite when it makes sense to, we certainly should, but we really need to be looking at code coverage tools and adding tests to improve our test coverage which can be run by the buildfarm animals (or even just a subset of them, if we feel that having all the animals running them would be excessive). Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] INSERT ... ON CONFLICT IGNORE (and UPDATE) 3.0
On 5 March 2015 at 23:44, Peter Geoghegan wrote: > On Wed, Mar 4, 2015 at 5:18 PM, Peter Geoghegan wrote: >> Attached patch series forms what I'm calling V3.0 of the INSERT ... ON >> CONFLICT IGNORE/UPDATE feature. (Sorry about all the threads. I feel >> this development justifies a new thread, though.) > Hi, I had a play with this, mainly looking at the interaction with RLS. (Note there is some bitrot in gram.y that prevents the first patch from applying cleanly to HEAD) I tested using the attached script, and one test didn't behave as I expected. I believe the following should have been a valid upsert (following the update path) but actually it failed: INSERT INTO t1 VALUES (4, 0) ON CONFLICT (a) UPDATE SET b = 1; AFAICT, it is applying a WITH CHECK OPTION with qual "b > 0 AND a % 2 = 0" to the about-to-be-updated tuple (a=4, b=0), which is wrong because the "b > 0" check (policy p3) should only be applied to the post-update tuple. Possibly I'm missing something though. Regards, Dean \set ECHO all DROP TABLE IF EXISTS t1; CREATE TABLE t1(a int PRIMARY KEY, b int); CREATE POLICY p1 ON t1 FOR INSERT WITH CHECK (b = 0); CREATE POLICY p2 ON t1 FOR UPDATE USING (a % 2 = 0); CREATE POLICY p3 ON t1 FOR UPDATE WITH CHECK (b > 0); ALTER TABLE t1 ENABLE ROW LEVEL SECURITY; SET row_security = force; -- Valid inserts INSERT INTO t1 VALUES (1, 0); INSERT INTO t1 VALUES (2, 0); -- Insert that should fail policy p1 INSERT INTO t1 VALUES (3, 1); -- Valid update UPDATE t1 SET b = 1 WHERE a = 2; -- Update that should fail policy p2 (not an error, just no rows updated) UPDATE t1 SET b = 1 WHERE a = 1; -- Update that should fail policy p3 UPDATE t1 SET b = 0 WHERE a = 2; -- Valid upserts (insert path) INSERT INTO t1 VALUES (3, 0) ON CONFLICT (a) UPDATE SET b = 0; INSERT INTO t1 VALUES (4, 0) ON CONFLICT (a) UPDATE SET b = 0; -- Upsert (insert path) that should fail policy p1 INSERT INTO t1 VALUES (5, 1) ON CONFLICT (a) UPDATE SET b = 1; -- Upsert (update path) that should fail policy p1 INSERT INTO t1 VALUES (3, 1) ON CONFLICT (a) UPDATE SET b = 1; -- Valid upsert (update path) -- XXX: Should pass, but fails WCO on t1 -- Failing WCO: (b > 0 AND a % 2 = 0) = p3 AND p2 INSERT INTO t1 VALUES (4, 0) ON CONFLICT (a) UPDATE SET b = 1; -- Upsert (update path) that should fail policy p2 INSERT INTO t1 VALUES (3, 0) ON CONFLICT (a) UPDATE SET b = 1; -- Upsert (update path) that should fail policy p3 INSERT INTO t1 VALUES (4, 0) ON CONFLICT (a) UPDATE SET b = 0; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] get_object_address support for additional object types
Stephen Frost wrote: > Alvaro, > > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > > Stephen Frost wrote: > > > I thought the rest of it looked alright. I agree it's a bit odd how the > > > opfamily is handled but I agree with your assessment that there's not > > > much better we can do with this object representation. > > > > Actually, on second thought I revisited this and changed the > > representation for opfamilies and opclasses: instead of putting the AM > > name in objargs, we can put it as the first element of objname instead. > > That way, objargs is unused for opfamilies and opclasses, and we're free > > to use it for the type arguments in amops and amprocs. This makes the > > lists consistent for the four cases: in objname, amname first, then > > qualified opclass/opfamily name. For amop/amproc, the member number > > follows. Objargs is unused in opclass/opfamily, and it's a two-element > > list of types in amop/amproc. > > Agreed, that makes more sense to me also. Great, thanks for checking -- pushed that way. > > The attached patch changes the grammar to comply with the above, and > > adds the necessary get_object_address and getObjectIdentityParts support > > code for amop/amproc objects. > > I took a quick look through and it looked fine to me. Actually, there was a bug in the changes of the rule for ALTER EXTENSION ADD OPERATOR CLASS. I noticed by chance only, and upon testing it manually I realized I had made a mistake. I then remembered I made the same bug previously, fixed by 5c5ffee80f35, and I'm not wondering why do we not have any test for ALTER EXTENSION ADD other than pg_upgrading some database that contains an extension which uses each command. This seems pretty dangerous to me, generally speaking ... we should definitely be testing all these ALTER EXTENSION commands. -- Á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] One question about security label command
Tom, * Tom Lane (t...@sss.pgh.pa.us) wrote: > The idea of making the regression test entirely independent of the > system's policy would presumably solve this problem, so I'd kind of > like to see progress on that front. Apologies, I guess it wasn't clear, but that's what I was intending to advocate. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] One question about security label command
Stephen Frost writes: > * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: >> Kohei KaiGai wrote: >>> The attached patch fixes the policy module of regression test. >> Is this something we would backpatch? > As it's just a change to the regression tests, it seems like it'd be a > good idea to backpatch it to me as there's very low risk of it breaking > anything and it'd actually fix the tests when they're run. Do we need to worry about machines that are still running the older selinux policy? I'm not sure it's a net win if we fix the regression tests for latest-and-shiniest by breaking them elsewhere. Especially not if we're going to back-patch into older branches, which are likely to be getting run on older platforms. The idea of making the regression test entirely independent of the system's policy would presumably solve this problem, so I'd kind of like to see progress on that front. 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] One question about security label command
Alvaro, KaiGai, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Kohei KaiGai wrote: > > > This regression test fail come from the base security policy of selinux. > > In the recent selinux-policy package, "unconfined" domain was changed > > to have unrestricted permission as literal. So, this test case relies multi- > > category policy restricts unconfined domain, but its assumption is not > > correct now. > > Makes sense. > > > The attached patch fixes the policy module of regression test. > > What branches need this patch? Do we need a modified patch for > earlier branches? > > Could you provide a buildfarm animal that runs the sepgsql test in all > branches on a regular basis? Would be great if KaiGai can, of course, but I'm planning to stand one up here soon in any case. > > However, I also think we may stop to rely permission set of pre-defined > > selinux domains. Instead of pre-defined one, sepgsql-regtest.te may be > > ought to define own domain with appropriate permission set independent > > from the base selinux-policy version. > > Is this something we would backpatch? As it's just a change to the regression tests, it seems like it'd be a good idea to backpatch it to me as there's very low risk of it breaking anything and it'd actually fix the tests when they're run. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] One question about security label command
Kohei KaiGai wrote: > This regression test fail come from the base security policy of selinux. > In the recent selinux-policy package, "unconfined" domain was changed > to have unrestricted permission as literal. So, this test case relies multi- > category policy restricts unconfined domain, but its assumption is not > correct now. Makes sense. > The attached patch fixes the policy module of regression test. What branches need this patch? Do we need a modified patch for earlier branches? Could you provide a buildfarm animal that runs the sepgsql test in all branches on a regular basis? > However, I also think we may stop to rely permission set of pre-defined > selinux domains. Instead of pre-defined one, sepgsql-regtest.te may be > ought to define own domain with appropriate permission set independent > from the base selinux-policy version. Is this something we would backpatch? -- Á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] Reduce pinning in btree indexes
On 16 March 2015 at 12:48, Kevin Grittner wrote: > Simon Riggs wrote: >> On 13 March 2015 at 15:41, Kevin Grittner wrote: >> >>> The feedback was generally fairly positive except for the fact that >>> snapshot "age" (for purposes of being too old) was measured in >>> transaction IDs assigned. There seemed to be a pretty universal >>> feeling that this needed to be changed to a time-based setting. >> >> -1 for a time based setting. >> >> After years of consideration, bloat is now controllable by altering >> the size of the undo tablespace. >> >> I think PostgreSQL needs something size-based also. It would need some >> estimation to get it to work like that, true, but it is actually the >> size of the bloat we care about, not the time. So we should be >> thinking in terms of limits that we actually care about. > > Are you thinking, then, that WAL volume generated (as determined by > LSN) would be the appropriate unit of measure for this? (We would > still need to map that back to transaction IDs for vacuuming, of > course.) If we did that we could allow the "size" units of > measure, like '5GB' and similar. Or are you thinking of something > else? It's probably the closest and easiest measure, and the most meaningful. We can easily accumulate that in a data structure in clog, like async commit LSN. For next release though, since it will take a little bit of thought to interpret that. With commit timestamp enabled in 9.5, we can easily judge time limit, but it is less useful because its not a measure of bloat. As I've said, I'd be happy with just an xid limit for 9.5, if that was the only thing we had. But I think timestamp is just as easy. > Given that there seems to be disagreement on what is the more > useful metric, do we want to consider allowing more than one? If > so, would it be when *all* conditions are met or when *any* > conditions are met? Yours was the first reply to my idea, so I think its too early to describe that as disagreement. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, 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] Reduce pinning in btree indexes
Kyotaro HORIGUCHI wrote: > Thank you for rewriting. Thank *you* for pointing out where more work was needed in the comments and README! > I attached the your latest patch to this mail as > bt-nopin-v4.patch for now. Please check that there's no problem > in it. I checked out master, applied the patch and checked it against my latest code (merged with master), and it matched. So, looks good. > - By this patch, index scan becomes to release buffer pins while > fetching index tuples in a page, so it should reduce the chance > of index scans with long duration to block vacuum, because > vacuums now can easily overtake the current position of an > index scan. I didn't actually measured how effective it is, > though. That part got pretty thorough testing on end-user software. The whole reason for writing the patch was that after applying the snapshot-too-old PoC patch they still saw massive bloat because all autovacuum workers blocked behind cursors which were left idle. The initial version of this patch fixed that, preventing (in combination with the other patch) uncontrolled bloat in a 48 hour test. > - It makes no performance deterioration, on the contrary it > accelerates index scans. It seems to be because of removal of > lock and unlock surrounding _bt_steppage in bt_next. I fear that the performance improvement may only show up on forward scans -- because the whole L&Y algorithm depends on splits only moving right, walking right is almost trivial to perform correctly with minimal locking and pinning. Our left pointers help with scanning backward, but there are a lot of conditions that can complicate that, leaving us with the choice between keeping some of the locking or potentially scanning more pages than we now do on a backward scan. Since it wasn't clear how many cases would benefit from a change and how many would lose, I pretty much left the backward scan locking alone in this patch. Changing the code to work the other way would not be outrageously difficult; but benchmarking to determine whether the alternative was a net win would be pretty tricky and very time-consuming. If that is to be done, I strongly feel it should be a separate patch. Because this patch makes a forward scan faster without having much affect on a backward scan, the performance difference between them (which has always existed) will get a little wider. I wonder whether this difference should perhaps be reflected in plan costing. -- Kevin Grittner EDB: 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
[HACKERS] Question about TEMP tables
Hello, all. We can create temp namespaces and temp objects that contains it. So, for example, temp table will be create at pg_temp_N (N - backendID). But afrer cluster init we have pg_temp_1 and pg_toast_temp_1 namespaces with OIDs 11333 and 11334. Those namespaces are visible from any cluster database, but we cannot create any temp objects (please, correct me). So, how can we use those namespaces and what are needed for? Thank you. -- Best regards, Dmitry Voronin -- Sent 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 128-bit integers for sum, avg and statistics aggregates
On 16/03/15 00:37, Andreas Karlsson wrote: On 03/15/2015 09:47 PM, Petr Jelinek wrote: It's almost the same thing as you wrote but the 128 bit and 64 bit optimizations are put on the same "level" of optimized routines. But this is nitpicking at this point, I am happy with the patch as it stands right now. Do you think it is ready for committer? In my opinion, yes. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Reduce pinning in btree indexes
On Mon, Mar 16, 2015 at 8:48 AM, Kevin Grittner wrote: > Given that there seems to be disagreement on what is the more > useful metric, do we want to consider allowing more than one? If > so, would it be when *all* conditions are met or when *any* > conditions are met? Oh, gosh. Maybe we could invent a whole policy language for this. Or, if we want to do something less painful, we could fire nail-guns into our eyeballs. I'm not sure what the value of basing this on WAL volume is, and I think we should talk about that a bit more first. The value of doing this by rate of XID consumption is easy to understand: it's simple to implement. The value of doing this by time is also simple to understand: if you set the setting to 20 minutes, then you can be sure that queries will not get cancelled unless they run for more than 20 minutes. This clearly makes the system easier to reason about. I don't see what the benefit of doing this by WAL volume would be. What Simon actually proposed was by *bloat*; that is, if the overall amount of bloat in the system exceeds some metric, start whacking old queries in order to control it. The appeal of that is obvious, but I think it would be very hard to make work, because canceling queries won't get rid of the bloat you've already introduced, and so you'd tend to cancel nothing until you hit some threshold, and then start canceling everything. If we want this feature, let's try to keep the spec simple enough that we actually get 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] Reduce pinning in btree indexes
Simon Riggs wrote: > On 13 March 2015 at 15:41, Kevin Grittner wrote: > >> The feedback was generally fairly positive except for the fact that >> snapshot "age" (for purposes of being too old) was measured in >> transaction IDs assigned. There seemed to be a pretty universal >> feeling that this needed to be changed to a time-based setting. > > -1 for a time based setting. > > After years of consideration, bloat is now controllable by altering > the size of the undo tablespace. > > I think PostgreSQL needs something size-based also. It would need some > estimation to get it to work like that, true, but it is actually the > size of the bloat we care about, not the time. So we should be > thinking in terms of limits that we actually care about. Are you thinking, then, that WAL volume generated (as determined by LSN) would be the appropriate unit of measure for this? (We would still need to map that back to transaction IDs for vacuuming, of course.) If we did that we could allow the "size" units of measure, like '5GB' and similar. Or are you thinking of something else? Given that there seems to be disagreement on what is the more useful metric, do we want to consider allowing more than one? If so, would it be when *all* conditions are met or when *any* conditions are met? -- Kevin Grittner EDB: 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] Join push-down support for foreign tables
On Wed, Mar 4, 2015 at 4:26 AM, Shigeru Hanada wrote: > Here is v4 patch of Join push-down support for foreign tables. This > patch requires Custom/Foreign join patch v7 posted by Kaigai-san. Hi, I just want to point out to the folks on this thread that the action in this area is happening on the other thread, about the custom/foreign join patch, and that Tom and I are suspecting that we do not have the right design here. Your input is needed. >From my end, I am quite skeptical about the way postgresGetForeignJoinPath in this patch works. It looks only at the cheapest total path of the relations to be joined, which seems like it could easily be wrong. What about some other path that is more expensive but provides a convenient sort order? What about something like A LEFT JOIN (B JOIN C ON B.x = C.x) ON A.y = B.y AND A.z = C.z, which can't make a legal join until level 3? Tom's proposed hook placement would instead invoke the FDW once per joinrel, passing root and the joinrel. Then, you could cost a path based on the idea of pushing that join entirely to the remote side, or exit without doing anything if pushdown is not feasible. Please read the other thread and then respond either there or here with thoughts on that design. If you don't provide some input on this, both of these patches are going to get rejected as lacking consensus, and we'll move on to other things. I'd really rather not ship yet another release without this important feature, but that's where we're heading if we can't talk this through. Thanks, -- 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: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
On Sat, Mar 14, 2015 at 10:37 PM, Kouhei Kaigai wrote: > From the standpoint of extension development, I'm uncertain whether we > can easily reproduce information needed to compute alternative paths on > the hook at standard_join_search(), like a hook at add_paths_to_joinrel(). > > (Please correct me, if I misunderstood.) > For example, it is not obvious which path is inner/outer of the joinrel > on which custom-scan provider tries to add an alternative scan path. That's a problem for the GPU-join use case, where you are essentially trying to add new join types to the system. But it's NOT a problem if what you're actually trying to do is substitute a *scan* from a *join*. If you're going to produce the join output by scanning a materialized view, or by scanning the results of a query pushed down to a foreign server, you don't need to divide the rels into inner rels and outer rels; indeed, any such division would be artificial. You just need to generate a query that produces the right answer *for the entire joinrel* and push it down. I'd really like to hear what the folks who care about FDW join pushdown think about this hook placement issue. -- 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] recovery_target_action = pause & hot_standby = off
On 2015-03-16 07:52:20 +, Simon Riggs wrote: > On 15 March 2015 at 22:38, Andres Freund wrote: > > > Sorry, I don't buy this. If I have "recovery_target_action = 'pause'" in > > the config file, I want it to pause. > > You want it to enter a state where you cannot perform any action other > than shutdown? > > Why would anyone want that? You actually still could promote. But I'd be perfectly happy if postgres said ERROR: recovery_target_action = 'pause' in "%s" cannot be used without hot_standby DETAIL: Recovery pauses cannot be resumed without SQL level access. HINT: Configure hot_standby and try again. or something roughly like that. If postgres tells me what to change to achieve what I want, I have a much higher chance of getting there. Especially if it just does something else otherwise. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Removing INNER JOINs
On 16 March 2015 at 09:55, David Rowley wrote: > I think it's probably possible to do this, but I think it would require > calling make_one_rel() with every combination of each possibly removable > relations included and not included in the join list. I'm thinking this > could end up a lot of work as the number of calls to make_one_rel() would be > N^2, where N is the number of relations that may be removable. > > My line of thought was more along the lines of that the backup/all purpose > plan will only be used in very rare cases. Either when a fk has been > deferred or if the query is being executed from within a volatile function > which has been called by an UPDATE statement which has just modified the > table causing a foreign key trigger to be queued. I'm willing to bet someone > does this somewhere in the world, but the query that's run would also have > to have a removable join. (One of the regression tests I've added exercises > this) > > For that reason I thought it was best to generate only 2 plans. One with > *all* possible removable rels removed, and a backup one with nothing removed > which will be executed if there's any FK triggers queued up. Agreed, just 2 plans. > The two ways of doing this have a massively different look in the EXPLAIN > output. With the method the patch currently implements only 1 of the 2 > alternative plans are seen by EXPLAIN, this is because I've coded > ExecInitAlternativePlan() to return the root node only 1 of the 2 plans. If > I had kept the AlternativePlan node around then the EXPLAIN output would > have 2 plans, both sitting under the AlternativePlan node. I guess that is at least compatible with how we currently handle other join elimination, so that is acceptable to me. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, 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] Question about TEMP tables
Hello, all.We can create temp namespaces and temp objects that contains it. So, for example, temp table will be create at pg_temp_N (N - backendID). But afrer cluster init we have pg_temp_1 and pg_toast_temp_1 namespaces with OIDs 11333 and 11334. Those namespaces are visible from any cluster database, but we cannot create any temp objects (please, correct me). So, how can we use those namespaces and what are needed for? Thank you. -- Best regards, Dmitry Voronin
Re: [HACKERS] Performance improvement for joins where outer side is unique
Hello, I don't have enough time for now but made some considerations on this. It might be a way that marking unique join peer at bottom and propagate up, not searching from top of join list. Around create_join_clause might be a candidate for it. I'll investigate that later. regards, At Sat, 14 Mar 2015 23:05:24 +1300, David Rowley wrote in dgrowleyml> On 14 March 2015 at 14:51, David Rowley wrote: dgrowleyml> dgrowleyml> > On 13 March 2015 at 20:34, Kyotaro HORIGUCHI < dgrowleyml> > horiguchi.kyot...@lab.ntt.co.jp> wrote: dgrowleyml> > dgrowleyml> >> Unfortunately I can't decide this to be 'ready for commiter' for dgrowleyml> >> dgrowleyml> > now. I think we should have this on smaller footprint, in a dgrowleyml> >> method without separate exhauxtive searching. I also had very dgrowleyml> >> similar problem in the past but I haven't find such a way for my dgrowleyml> >> problem.. dgrowleyml> >> dgrowleyml> >> dgrowleyml> > I don't think it's ready yet either. I've just been going over a few dgrowleyml> > things and looking at Tom's recent commit b557226 in pathnode.c I've got a dgrowleyml> > feeling that this patch would need to re-factor some code that's been dgrowleyml> > modified around the usage of relation_has_unique_index_for() as when this dgrowleyml> > code is called, the semi joins have already been analysed to see if they're dgrowleyml> > unique, so it could be just a case of ripping all of that out dgrowleyml> > of create_unique_path() and just putting a check to say rel->is_unique_join dgrowleyml> > in there. But if I do that then I'm not quite sure if dgrowleyml> > SpecialJoinInfo->semi_rhs_exprs and SpecialJoinInfo->semi_operators would dgrowleyml> > still be needed at all. These were only added in b557226. Changing this dgrowleyml> > would help reduce the extra planning time when the query contains dgrowleyml> > semi-joins. To be quite honest, this type of analysis belongs in dgrowleyml> > analyzejoin.c anyway. I'm tempted to hack at this part some more, but I'd dgrowleyml> > rather Tom had a quick glance at what I'm trying to do here first. dgrowleyml> > dgrowleyml> > dgrowleyml> dgrowleyml> I decided to hack away any change the code Tom added in b557226. I've dgrowleyml> changed it so that create_unique_path() now simply just uses if dgrowleyml> (rel->is_unique_join), instead off all the calls to dgrowleyml> relation_has_unique_index_for() and query_is_distinct_for(). This vastly dgrowleyml> simplifies that code. One small change is that Tom's checks for uniqueness dgrowleyml> on semi joins included checks for volatile functions, this check didn't dgrowleyml> exist in the original join removal code, so I've left it out. We'll never dgrowleyml> match a expression with a volatile function to a unique index as indexes dgrowleyml> don't allow volatile function expressions anyway. So as I understand it dgrowleyml> this only serves as a fast path out if the join condition has a volatile dgrowleyml> function... But I'd assume that check is not all that cheap. dgrowleyml> dgrowleyml> I ended up making query_supports_distinctness() and query_is_distinct_for() dgrowleyml> static in analyzejoins.c as they're not used in any other files. dgrowleyml> relation_has_unique_index_for() is also now only used in analyzejoins.c, dgrowleyml> but I've not moved it into that file yet as I don't want to bloat the dgrowleyml> patch. I just added a comment to say it needs moved. dgrowleyml> dgrowleyml> I've also added a small amount of code to query_is_distinct_for() which dgrowleyml> allows subqueries such as (select 1 a offset 0) to be marked as unique. I dgrowleyml> thought it was a little silly that these were not being detected as unique, dgrowleyml> so I fixed it. This has the side effect of allowing left join removals for dgrowleyml> queries such as: select t1.* from t1 left join (select 1 a offset 0) a on dgrowleyml> t1.id=a.a; dgrowleyml> dgrowleyml> Updated patch attached. dgrowleyml> dgrowleyml> Regards dgrowleyml> dgrowleyml> David Rowley -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Re: Install shared libs in lib/ and bin/ with MSVC (Was: install libpq.dll in bin directory on Windows / Cygwin)
On Mon, Mar 16, 2015 at 4:56 PM, Asif Naeem wrote: > The following review has been posted through the commitfest application: > make installcheck-world: not tested > Implements feature: tested, failed > Spec compliant: not tested > Documentation:not tested > > Thank you Michael. v4 patch looks good to me. > > The new status of this patch is: Ready for Committer Thanks! -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: Install shared libs in lib/ and bin/ with MSVC (Was: install libpq.dll in bin directory on Windows / Cygwin)
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: tested, failed Spec compliant: not tested Documentation:not tested Thank you Michael. v4 patch looks good to me. The new status of this patch is: Ready for Committer -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] recovery_target_action = pause & hot_standby = off
On 15 March 2015 at 22:38, Andres Freund wrote: > Sorry, I don't buy this. If I have "recovery_target_action = 'pause'" in > the config file, I want it to pause. You want it to enter a state where you cannot perform any action other than shutdown? Why would anyone want that? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, 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] Parallel Seq Scan
On 16-03-2015 PM 04:14, Amit Kapila wrote:
> On Mon, Mar 16, 2015 at 9:40 AM, Amit Langote
> wrote:
>> Or if the just-detached queue happens to be the last one, we'll make
>> shm_mq_receive() to read from a potentially already-detached queue in the
>> immediately next iteration.
>
> Won't the last queue case already handled by below code:
> else
> {
> --funnel->nqueues;
> if (funnel->nqueues == 0)
> {
> if (done != NULL)
> *done = true;
> return NULL;
> }
>
Actually I meant "currently the last" or:
funnel->nextqueue == funnel->nqueue - 1
So the code you quote would only take care of subset of the cases.
Imagine funnel->nqueues going down from 5 to 3 in successive iterations while
funnel->nextqueue remains set to 4 (which would have been the "currently last"
when funnel->nqueues was 5).
>> I can't seem to really figure out the other problem of waiting forever in
>> WaitLatch()
>>
>
> The reason seems that for certain scenarios, the way we set the latch before
> exiting needs some more thought. Currently we are setting the latch in
> HandleParallelMessageInterrupt(), that doesn't seem to be sufficient.
>
How about shm_mq_detach() called from ParallelQueryMain() right after
exec_parallel_stmt() returns? Doesn't that do the SetLatch() that needs to be
done by a worker?
Thanks,
Amit
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Parallel Seq Scan
On Fri, Mar 13, 2015 at 7:06 PM, Alvaro Herrera wrote: > > Amit Kapila wrote: > > > I think this can happen if funnel->nextqueue is greater > > than funnel->nqueues. > > Please see if attached patch fixes the issue, else could you share the > > scenario in more detail where you hit this issue. > > Uh, isn't this copying an overlapping memory region? If so you should > be using memmove instead. > Agreed, will update this in next version of patch. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Parallel Seq Scan
On Mon, Mar 16, 2015 at 9:40 AM, Amit Langote
wrote:
>
> On 13-03-2015 PM 11:03, Amit Kapila wrote:
> > On Fri, Mar 13, 2015 at 7:15 PM, Robert Haas
wrote:
> >>
> >> I don't think this is the right fix; the point of that code is to
> >> remove a tuple queue from the funnel when it gets detached, which is a
> >> correct thing to want to do. funnel->nextqueue should always be less
> >> than funnel->nqueues; how is that failing to be the case here?
> >>
> >
> > I could not reproduce the issue, neither the exact scenario is
> > mentioned in mail. However what I think can lead to funnel->nextqueue
> > greater than funnel->nqueues is something like below:
> >
> > Assume 5 queues, so value of funnel->nqueues will be 5 and
> > assume value of funnel->nextqueue is 2, so now let us say 4 workers
> > got detached one-by-one, so for such a case it will always go in else
loop
> > and will never change funnel->nextqueue whereas value of funnel->nqueues
> > will become 1.
> >
>
> Or if the just-detached queue happens to be the last one, we'll make
> shm_mq_receive() to read from a potentially already-detached queue in the
> immediately next iteration.
Won't the last queue case already handled by below code:
else
{
--funnel->nqueues;
if (funnel->nqueues == 0)
{
if (done != NULL)
*done = true;
return NULL;
}
> That seems to be caused by not having updated the
> funnel->nextqueue. With the returned value being SHM_MQ_DETACHED, we'll
again
> try to remove it from the queue. In this case, it causes the third
argument to
> memcpy be negative and hence the segfault.
>
In anycase, I think we need some handling for such cases.
> I can't seem to really figure out the other problem of waiting forever in
> WaitLatch()
>
The reason seems that for certain scenarios, the way we set the latch before
exiting needs some more thought. Currently we are setting the latch in
HandleParallelMessageInterrupt(), that doesn't seem to be sufficient.
> By the way, you can try reproducing this with the example I posted on
Friday.
>
Sure.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] PATCH: pgbench - merging transaction logs
I agree that inter-process stuff should be avoided. This is not what I had in mind. I was thinking of "fprintf" on the same file handler by different threads. That still involves some sort of 'implicit' locking, no? I fully agree that it would need locking, wether it is implicit if fprintf is thread safe, or explicit if not. And as I mentioned, I'm not sure fprintf() is thread-safe on all the platforms we support. I do not know, but the question is relevant ! The answer must be thought for. Adding a MUTEX to be on the safe side may not be too big an issue. Another issue raised by your patch is that the log format may be improved, say by providing a one-field timestamp at the beginning of the line. I don't see how this is relevant to this patch? For the simple log format, all the parsing needed would be for the timestamp, and the remainder would just be text to pass along, no need to %d %f... whatever. Oh, ok. Well, that's true, but I don't think that significantly changes the overall complexity. For your current implementation, it would remove plenty of variables, conditions about options, just one scan one print would remain, and no need to update the format later, so it would simplify the code greatly for the "simple" log. I would suggest this that I would support: implement this feature the simple way (aka fprintf, maybe a mutex) when compiled with threads, and generate an error "feature not available with process-based thread emulation" when compiled with processes. This way we avoid a, lot of heavy code to maintain in the future, and you still get the feature within pgbench. There are already some things which are not the same with thread emulation because it would have been tiring to implement for it for very little benefit. I really dislike the features supported only in some cases (although most deployments probably support proper threading these days). I agree on the general principle, but not in this particular case, because for me thread emulation is more or less obsolete, useless and unused. My view is that you're code is on the too-heavy side, and adds a burden for later maintenance for rather a should-be-simple feature, really just so it works for "thread emulation" that probably nought people use or really need. So I'm not enthousiastic at all to get that in pgbench as is. If the code is much simpler, these reservations would be dropped, it would be a small and useful feature for a small code and maintenance impact. And I do not see the point in using the heavy stuff in my view obsolete stuff. -- Fabien. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
