Re: [HACKERS] WIP: Collecting statistics on CSV file data
(2011/11/07 20:26), Shigeru Hanada wrote: > (2011/10/20 18:56), Etsuro Fujita wrote: >> I revised the patch according to Hanada-san's comments. Attached is the >> updated version of the patch. >> >> Changes: >> >> * pull up of logging "analyzing foo.bar" >> * new vac_update_relstats always called >> * tab-completion in psql >> * add "foreign tables are not analyzed automatically..." to 23.1.3 >> Updating Planner Statistics >> * some other modifications > > Submission review > = > > - Patch can be applied, and all regression tests passed. :) Thank you for your testing. I updated the patch according to your comments. Attached is the updated version of the patch. > - file_fdw_do_analyze_rel is almost copy of do_analyze_rel. IIUC, > difference against do_analyze_rel are: > * don't logging analyze target > * don't switch userid to the owner of target table > * don't measure elapsed time for autoanalyze deamon > * don't handle index > * some comments are removed. > * sample rows are acquired by file_fdw's routine > > I don't see any problem here, but would you confirm that all of them are > intentional? Yes. But in the updated version, I've refactored analyze.c a little bit to allow FDW authors to simply call do_analyze_rel(). > - In your design, each FDW have to copy most of do_analyze_rel to their > own source. It means that FDW authors must know much details of ANALYZE > to implement ANALYZE handler. Actually, your patch exports some static > functions from analyze.c. Have you considered hooking > acquire_sample_rows()? Such handler should be more simple, and > FDW-specific. As you say, such design requires FDWs to skip some > records, but it would be difficult for some FDW (e.g. twitter_fdw) which > can't pick sample data up easily. IMHO such problem *must* be solved by > FDW itself. The updated version enables FDW authors to just write their own acquire_sample_rows(). On the other hand, by retaining to hook AnalyzeForeignTable routine at analyze_rel(), higher level than acquire_sample_rows() as before, it allows FDW authors to write AnalyzeForeignTable routine for foreign tables on a remote server to ask the server for its current stats instead, as pointed out earlier by Tom Lane. Best regards, Etsuro Fujita *** a/contrib/file_fdw/file_fdw.c --- b/contrib/file_fdw/file_fdw.c *** *** 16,31 #include #include "access/reloptions.h" #include "catalog/pg_foreign_table.h" #include "commands/copy.h" #include "commands/defrem.h" #include "commands/explain.h" #include "foreign/fdwapi.h" #include "foreign/foreign.h" #include "miscadmin.h" #include "nodes/makefuncs.h" #include "optimizer/cost.h" ! #include "utils/rel.h" #include "utils/syscache.h" PG_MODULE_MAGIC; --- 16,36 #include #include "access/reloptions.h" + #include "access/transam.h" #include "catalog/pg_foreign_table.h" #include "commands/copy.h" #include "commands/defrem.h" #include "commands/explain.h" + #include "commands/vacuum.h" #include "foreign/fdwapi.h" #include "foreign/foreign.h" #include "miscadmin.h" #include "nodes/makefuncs.h" #include "optimizer/cost.h" ! #include "parser/parse_relation.h" ! #include "pgstat.h" ! #include "utils/attoptcache.h" ! #include "utils/memutils.h" #include "utils/syscache.h" PG_MODULE_MAGIC; *** *** 101,106 static void fileBeginForeignScan(ForeignScanState *node, int eflags); --- 106,112 static TupleTableSlot *fileIterateForeignScan(ForeignScanState *node); static void fileReScanForeignScan(ForeignScanState *node); static void fileEndForeignScan(ForeignScanState *node); + static void fileAnalyzeForeignTable(Relation onerel, VacuumStmt *vacstmt, int elevel); /* * Helper functions *** *** 112,118 static List *get_file_fdw_attribute_options(Oid relid); static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel, const char *filename, Cost *startup_cost, Cost *total_cost); ! /* * Foreign-data wrapper handler function: return a struct with pointers --- 118,127 static void estimate_costs(PlannerInfo *root, RelOptInfo *baserel, const char *filename, Cost *startup_cost, Cost *total_cost); ! static intacquire_sample_rows(Relation onerel, ! HeapTuple *rows, int targrows, ! double *totalrows, double *totaldeadrows, ! BlockNumber *totalpages, int elevel); /* * Foreign-data wrapper handler function: return a struct with pointers *** *** 129,134 file_fdw_handler(PG_FUNCTION_ARGS) --- 138,144 fdwroutine->IterateForeignScan = fileIterateForeignScan; fdwroutine->ReScanForeignScan = fileReScanForeignScan; fdwroutine->EndForeignScan = fileEndForeignSca
Re: [HACKERS] Inlining comparators as a performance optimisation
On Sun, Sep 25, 2011 at 10:12 PM, Peter Geoghegan wrote: > I've produced something much neater than my first patch, attached, > although I still consider this to be at the POC stage, not least since > I'm not exactly sure how I should be selecting the right > specialisation in tuplesort_performsort() (a hack is used right now > that does a direct pg_proc OID comparison), and also because I haven't > implemented anything other than qsort for heap tuples yet (a minor > detail, I suppose). I'm pleased to say that a much clearer picture of > what's really going on here has emerged. > > Statistics: Total runtime according to explain analyze for query > "select * from orderlines order by prod_id" (dellstore2 sample db), at > GCC 4.5's -02 optimisation level, after warming the cache, on my > desktop: > > Without the patch: > > ~82ms > > With the patch, but with the "inline" keyword commented out for all > new functions/meta-functions: > > ~60ms > > with the patch, unmodified: > > ~52ms Reviewing away when I should be sleeping, wahoo...! I think that we should really consider doing with this patch what Tom suggested upthread; namely, looking for a mechanism to allow individual datatypes to offer up a comparator function that doesn't require bouncing through FunctionCall2Coll(). It seems to me that duplicating the entire qsort() algorithm is a little iffy. Sure, in this case it works out to a win. But it's only a small win - three-quarters of it is in the uncontroversial activity of reducing the impedance mismatch - and how do we know it will always be a win? Adding more copies of the same code can be an anti-optimization if it means that a smaller percentage of it fits in the instruction cache, and sometimes small changes in runtime are caused by random shifts in the layout of memory that align things more or less favorably across cache lines rather than by real effects. Now it may well be that this is a real effect, but will it still look as good when we do this for 10 data types? For 100 data types? In contrast, it seems to me that reducing the impedance mismatch is something that we could go and do across our entire code base, and every single data type would benefit from it. It would also be potentially usable by other sorting algorithms, not just quick sort. -- 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] RangeVarGetRelid()
On Thu, Nov 17, 2011 at 10:48 PM, Noah Misch wrote: > On Thu, Nov 17, 2011 at 08:59:58PM -0500, Robert Haas wrote: >> On Thu, Nov 17, 2011 at 4:12 PM, Alvaro Herrera >> wrote: >> > Excerpts from Robert Haas's message of jue nov 17 17:51:06 -0300 2011: >> >> The trouble is, I'm not quite sure how to do that. ?It seems like >> >> permissions checks and lock-the-heap-for-this-index should be done in >> >> RangeVarGetRelid() just after the block that says "if (retry)" and >> >> just before the block that calls LockRelationOid(). ?That way, if we >> >> end up deciding we need to retry the name lookup, we'll retry all that >> >> other stuff as well, which is exactly right. ?The difficulty is that >> >> different callers have different needs for what should go in that >> >> space, to the degree that I'm a bit nervous about continuing to add >> >> arguments to that function to satisfy what everybody needs. ?Maybe we >> >> could make most of them Booleans and pass an "int flags" argument. >> >> Another option would be to add a "callback" argument to that function >> >> that would be called at that point with the relation, relId, and >> >> oldRelId as arguments. ?Alternatively, we could just resign ourselves >> >> to duplicating the loop in this function into each place in the code >> >> that has a special-purpose requirement, but the function is complex >> >> enough to make that a pretty unappealing prospect. >> > >> > I'm for the callback. > >> Having now written the patch, I'm convinced that the callback is in >> fact the right choice. It requires only very minor reorganization of >> the existing code, which is always a plus. > > +1 > > How about invoking the callback earlier, before "if (lockmode == NoLock)"? > That way, a REINDEX TABLE that blocks against an ALTER TABLE OWNER TO will > respect the new owner. Also, the callback will still get used in the NoLock > case. Callbacks that would have preferred the old positioning can check relId > == oldRelId to distinguish. Hmm, I guess that would work. >> --- a/src/include/catalog/namespace.h >> +++ b/src/include/catalog/namespace.h >> @@ -47,9 +47,15 @@ typedef struct OverrideSearchPath >> bool addTemp; /* implicitly prepend temp >> schema? */ >> } OverrideSearchPath; >> >> +typedef void (*RangeVarGetRelidCallback)(const RangeVar *relation, Oid >> relId, >> + Oid oldRelId); >> >> -extern Oid RangeVarGetRelid(const RangeVar *relation, LOCKMODE lockmode, >> - bool missing_ok, bool nowait); >> +#define RangeVarGetRelid(relation, lockmode, missing_ok, nowait) \ >> + RangeVarGetRelidExtended(relation, lockmode, missing_ok, >> nowait, NULL) >> + >> +extern Oid RangeVarGetRelidExtended(const RangeVar *relation, >> + LOCKMODE lockmode, bool >> missing_ok, bool nowait, >> + RangeVarGetRelidCallback >> callback); > > I like the split of RangeVarGetRelidExtended from RangeVarGetRelid. Shall we > also default missing_ok=false and nowait=false for RangeVarGetRelid? I thought about that, but just did it this way for now to keep the patch small for review purposes. nowait = true is only used in one place, so it probably makes sense to default it to false in the non-extended version. But there are a number of callers who want missing_ok = true behavior, so I'm inclined not to think that one should be available in the non-extended version. -- 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
[HACKERS] vpath builds and verbose error messages
When using verbose error messages (psql \set VERBOSITY verbose) with a vpath build, you get this sort of thing: ERROR: 42703: column "foo" does not exist LINE 1: select foo; ^ LOCATION: transformColumnRef, /build/buildd-postgresql-9.1_9.1.1-3-i386-AP0ovQ/postgresql-9.1-9.1.1/build/../src/backend/parser/parse_expr.c:766 Can we shorten that path somehow? Either in the C code when printing it out, or during the build. Any ideas? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai wrote: > Part-2) Groundworks on objectaddress.c > This patch adds necessary groundworks for Part-3 and Part-4. > It adds ObjectPropertyType of objectaddress.c index-oid and cache-id > for name lookup and attribute number of object name; these field is > used to detect namespace conflict on object_exists_namespace() that > shall be called on refactored ALTER SET SCHEMA/RENAME TO. > It also reduce some arguments of check_object_ownership(), and allows > to call this function without knowledge of ObjectType and text > representation. It allows to check ownership using this function from > the code path of AlterObjectNamespace_oid() that does not have (or > need to look up catalog to obtain) ObjectType information. I spent some time wading through the code for parts 2 through 4, and I guess I'm not sure this is headed in the right direction. If we need this much infrastructure in order to consolidate the handling of ALTER BLAH .. SET SCHEMA, then maybe it's not worth doing. But I'm not sure why we do. My thought here was that we should extended the ObjectProperty array in objectaddress.c so that AlterObjectNamespace can get by with fewer arguments - specifically, it seems like the following ought to be things that can be looked up via the ObjectProperty mechanism: int oidCacheId, int nameCacheId, int Anum_name, int Anum_namespace, int Anum_owner, AclObjectKind acl_kind The advantage of that, or so it seems to me, is that all this information is in a table in objectaddress.c where multiple clients can get at it at need, as opposed to the current system where it's passed as arguments to AlterObjectNamespace(), and all that would need to be duplicated if we had another function that did something similar. Now, what you have here is a much broader reworking. And that's not necessarily bad, but at the moment I'm not really seeing how it benefits us. -- 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] RangeVarGetRelid()
On Thu, Nov 17, 2011 at 08:59:58PM -0500, Robert Haas wrote: > On Thu, Nov 17, 2011 at 4:12 PM, Alvaro Herrera > wrote: > > Excerpts from Robert Haas's message of jue nov 17 17:51:06 -0300 2011: > >> The trouble is, I'm not quite sure how to do that. ?It seems like > >> permissions checks and lock-the-heap-for-this-index should be done in > >> RangeVarGetRelid() just after the block that says "if (retry)" and > >> just before the block that calls LockRelationOid(). ?That way, if we > >> end up deciding we need to retry the name lookup, we'll retry all that > >> other stuff as well, which is exactly right. ?The difficulty is that > >> different callers have different needs for what should go in that > >> space, to the degree that I'm a bit nervous about continuing to add > >> arguments to that function to satisfy what everybody needs. ?Maybe we > >> could make most of them Booleans and pass an "int flags" argument. > >> Another option would be to add a "callback" argument to that function > >> that would be called at that point with the relation, relId, and > >> oldRelId as arguments. ?Alternatively, we could just resign ourselves > >> to duplicating the loop in this function into each place in the code > >> that has a special-purpose requirement, but the function is complex > >> enough to make that a pretty unappealing prospect. > > > > I'm for the callback. > Having now written the patch, I'm convinced that the callback is in > fact the right choice. It requires only very minor reorganization of > the existing code, which is always a plus. +1 How about invoking the callback earlier, before "if (lockmode == NoLock)"? That way, a REINDEX TABLE that blocks against an ALTER TABLE OWNER TO will respect the new owner. Also, the callback will still get used in the NoLock case. Callbacks that would have preferred the old positioning can check relId == oldRelId to distinguish. > --- a/src/include/catalog/namespace.h > +++ b/src/include/catalog/namespace.h > @@ -47,9 +47,15 @@ typedef struct OverrideSearchPath > booladdTemp;/* implicitly prepend temp > schema? */ > } OverrideSearchPath; > > +typedef void (*RangeVarGetRelidCallback)(const RangeVar *relation, Oid relId, > + Oid oldRelId); > > -extern Oid RangeVarGetRelid(const RangeVar *relation, LOCKMODE lockmode, > - bool missing_ok, bool nowait); > +#define RangeVarGetRelid(relation, lockmode, missing_ok, nowait) \ > + RangeVarGetRelidExtended(relation, lockmode, missing_ok, > nowait, NULL) > + > +extern Oid RangeVarGetRelidExtended(const RangeVar *relation, > + LOCKMODE lockmode, bool > missing_ok, bool nowait, > + RangeVarGetRelidCallback > callback); I like the split of RangeVarGetRelidExtended from RangeVarGetRelid. Shall we also default missing_ok=false and nowait=false for RangeVarGetRelid? Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai wrote: > Part-2) Groundworks on objectaddress.c > This patch adds necessary groundworks for Part-3 and Part-4. > It adds ObjectPropertyType of objectaddress.c index-oid and cache-id > for name lookup and attribute number of object name; these field is > used to detect namespace conflict on object_exists_namespace() that > shall be called on refactored ALTER SET SCHEMA/RENAME TO. > It also reduce some arguments of check_object_ownership(), and allows > to call this function without knowledge of ObjectType and text > representation. It allows to check ownership using this function from > the code path of AlterObjectNamespace_oid() that does not have (or > need to look up catalog to obtain) ObjectType information. > In addition, it adds regression test cases for ALTER SET SCHEMA/RENAME TO. This part adds a new regression test to a parallel group with the following comments: # NB: temp.sql does a reconnect which transiently uses 2 connections, # so keep this parallel group to at most 19 tests ...and this would be test #20. So we either need to move it elsewhere, or move something else elsewhere. I'm tempted to add it to this rather scrawny-looking group, unless someone sees a reason to do otherwise: test: privileges security_label collate -- 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] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
On Thu, Nov 17, 2011 at 11:21 AM, Robert Haas wrote: > On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai wrote: >> Part-1) DROP statement refactoring >> It is a remaining portion of what I submitted in the last commit fest. >> It allows object types that didn't used DropStmt in gram.y to go >> through RemoveObjects(), instead of individual Remove(). > > Review of just this part: > > - I think we can remove the special case for foreign data wrappers > because (1) the only case in which there's any behavioral difference > at all is if a superuser creates a foreign data wrapper (or the > ownership of one is reassigned to him) and he is then made not a > superuser; non-superusers can't create foreign data wrappers, and > existing foreign data wrappers can't be given to non-superusers; > moreover, (2) removing the special case causes the behavior to match > the documentation, which it currently doesn't (but only in the > aforementioned, extremely minor way). > > - On the other hand, this patch blithely nukes the prohibition on > using DROP FUNCTION to remove an aggregate. I'm not sure that's a > good idea. It also eliminates the NOTICE when removing a built-in > function, which I think is OK because you don't actually get that far: > > rhaas=# drop function int4pl(integer, integer); > ERROR: cannot drop function int4pl(integer,integer) because it is > required by the database system > > - For some reason, we have code that causes procedural language names > to be downcased before use. Given that unquoted identifiers are > downcased anyway, this seems a bit redundant. I'm inclined to think > we don't need to preserve that behavior for DROP, especially because > other parts of the code - such as COMMENT - don't know about it > anyway. But rather than just changing it for DROP, I think we should > go through and rip out case_translate_language_name() across the > board, probably as a a separate commit. I've committed part 1 of this patch series after correcting the above items. -- 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
[HACKERS] Schedule for upcoming back-branch releases
The core and packagers lists have agreed to the schedule that was proposed towards the end of last week's discussion: http://archives.postgresql.org/pgsql-hackers/2011-11/msg00578.php That is, we'll wrap tarballs on Thursday Dec 1 for public announcement Monday Dec 5. Since the announced EOL date for the 8.2.x branch is "December 2011", I think we can treat this release as the last one for 8.2.x. 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] RangeVarGetRelid()
On Thu, Nov 17, 2011 at 4:12 PM, Alvaro Herrera wrote: > Excerpts from Robert Haas's message of jue nov 17 17:51:06 -0300 2011: >> The trouble is, I'm not quite sure how to do that. It seems like >> permissions checks and lock-the-heap-for-this-index should be done in >> RangeVarGetRelid() just after the block that says "if (retry)" and >> just before the block that calls LockRelationOid(). That way, if we >> end up deciding we need to retry the name lookup, we'll retry all that >> other stuff as well, which is exactly right. The difficulty is that >> different callers have different needs for what should go in that >> space, to the degree that I'm a bit nervous about continuing to add >> arguments to that function to satisfy what everybody needs. Maybe we >> could make most of them Booleans and pass an "int flags" argument. >> Another option would be to add a "callback" argument to that function >> that would be called at that point with the relation, relId, and >> oldRelId as arguments. Alternatively, we could just resign ourselves >> to duplicating the loop in this function into each place in the code >> that has a special-purpose requirement, but the function is complex >> enough to make that a pretty unappealing prospect. > > I'm for the callback. I worked up the attached patch showing how this might work. For demonstration purposes, the attached patch modifies REINDEX INDEX and REINDEX TABLE to use this facility. It turns out that, in the current code, they have opposite problems, so this is a good example of how this gives you the best of both worlds. Suppose we do: rhaas=# create table foo (a int primary key); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey" for table "foo" CREATE TABLE rhaas=# begin; BEGIN rhaas=# insert into foo values (1); INSERT 0 1 At this point, if we start up another session as non-privileged user, REINDEX INDEX foo_pkey will immediately fail with a permissions error (which is good), but REINDEX TABLE foo will queue up for a table lock (which is bad). Now suppose we set up like this: rhaas=# create table foo (a int primary key); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey" for table "foo" CREATE TABLE rhaas=# begin; BEGIN rhaas=# drop table foo; DROP TABLE rhaas=# create table foo (a int primary key); NOTICE: CREATE TABLE / PRIMARY KEY will create implicit index "foo_pkey" for table "foo" CREATE TABLE If we open another session as superuser and say "REINDEX INDEX foo_pkey", and then commit the open transaction in the first session, it will fail: rhaas=# reindex index foo_pkey; ERROR: could not open relation with OID 16481 However, with the same setup, "REINDEX TABLE foo" will work. With the attached patch, both situations are handled correctly by both commands. Having now written the patch, I'm convinced that the callback is in fact the right choice. It requires only very minor reorganization of the existing code, which is always a plus. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company rangevargetrelid-callback.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
Robert Haas writes: > On Thu, Nov 17, 2011 at 5:29 PM, Tom Lane wrote: >> I still think it's reasonable to remove the extra downcasing step, >> but we'll have to document it as a change. > So, should we add a note to all the LANGUAGE command pages in the > manual? Or just include this in the release notes? Release note seems sufficient. I looked at the text in CREATE FUNCTION and it doesn't seem to need changing. 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] [PATCH] Unremovable tuple monitoring
On 18/11/2011, at 10:44 AM, Robert Haas wrote: > On Thu, Nov 17, 2011 at 5:49 PM, Royce Ausburn wrote: >> Thanks for the discussion so far all. Would it be worthwhile to make >> another patch that addresses the points from Yeb's reviews? It's not >> sounding like this unremovable tuple count is something that postgres wants, >> but I'm happy to keep the patch up to scratch if we're still not sure. > > One question to ask yourself at this point in the process is whether > *you* still think the feature is useful. I'm fairly persuaded by > Tom's point that the value monitored by this patch will change so > quickly that it won't be useful to have VACUUM store it; it'll be > obsolete by the time you look at the numbers. If you are also > persuaded by that argument, then clearly it's time to throw in the > towel! > > But let's suppose you're NOT persuaded by that argument and you still > want the feature. In that case, don't just wait for someone else to > stick up for the feature; tell us why you still think it's a good > idea. Make a case for what you want. People here are usually > receptive to good ideas, well-presented. Okay - thanks Robert. I think I'll drop it. Now that I know what to look for in this situation, the changes this patch includes aren't of value to me. That's a pretty good indicator that it's probably not valuable to anyone else. I've changed the status of the patch to rejected in the commit fest. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
On Thu, Nov 17, 2011 at 5:29 PM, Tom Lane wrote: > Robert Haas writes: >> On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera >> wrote: >>> So the buildfarm broke due to this change, because citext does > >> Thanks for fixing it. Should we revert the original change? > > I still think it's reasonable to remove the extra downcasing step, > but we'll have to document it as a change. For instance, spelling > C as either "C" or 'C' would work differently now. The fact that > the former is downcased seems quite surprising to me, so I don't > think anybody would say that this isn't a better definition, but > undoubtedly it could force people to change their source files. So, should we add a note to all the LANGUAGE command pages in the manual? Or just include this in the release notes? -- 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] [PATCH] Unremovable tuple monitoring
On Thu, Nov 17, 2011 at 5:49 PM, Royce Ausburn wrote: > Thanks for the discussion so far all. Would it be worthwhile to make another > patch that addresses the points from Yeb's reviews? It's not sounding like > this unremovable tuple count is something that postgres wants, but I'm happy > to keep the patch up to scratch if we're still not sure. One question to ask yourself at this point in the process is whether *you* still think the feature is useful. I'm fairly persuaded by Tom's point that the value monitored by this patch will change so quickly that it won't be useful to have VACUUM store it; it'll be obsolete by the time you look at the numbers. If you are also persuaded by that argument, then clearly it's time to throw in the towel! But let's suppose you're NOT persuaded by that argument and you still want the feature. In that case, don't just wait for someone else to stick up for the feature; tell us why you still think it's a good idea. Make a case for what you want. People here are usually receptive to good ideas, well-presented. -- 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] [PATCH] Unremovable tuple monitoring
On 17/11/2011, at 1:47 AM, Robert Haas wrote: > On Wed, Nov 16, 2011 at 9:34 AM, Tom Lane wrote: >> Robert Haas writes: >>> Not sure about the log line, but allowing pgstattuple to distinguish >>> between recently-dead and quite-thoroughly-dead seems useful. >> >> The dividing line is enormously unstable though. pgstattuple's idea of >> RecentGlobalXmin could even be significantly different from that of a >> concurrently running VACUUM. I can see the point of having VACUUM log >> what it did, but opinions from the peanut gallery aren't worth much. > > Hmm, you have a point. > > But as Yeb points out, it seems like we should at least try to be more > consistent about the terminology. Thanks for the discussion so far all. Would it be worthwhile to make another patch that addresses the points from Yeb's reviews? It's not sounding like this unremovable tuple count is something that postgres wants, but I'm happy to keep the patch up to scratch if we're still not sure. Cheers, --Royce -- 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: psql + libedit command history truncation (was: psql history vs. dearmor (pgcrypto))
On Mon, Nov 14, 2011 at 7:04 PM, Josh Kupershmidt wrote: > But it reminded me of another issue. With OS X 10.6.8, and otool -L > reporting that psql depends on libedit version 2.11.0, the up-arrow > recall of Tomas' query gets truncated around here: > 5I0/NTm+fFkB0McY9E2fAA [rest of the line missing] For the curious, this does appear to be a hardcoded limit in libedit. A bit of digging through a tarball of libedit-20110802-3.0 turned up this line in el.h: #define EL_BUFSIZ ((size_t)1024) /* Maximum line size*/ which you can bump up as a work-around. Josh -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
Robert Haas writes: > On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera > wrote: >> So the buildfarm broke due to this change, because citext does > Thanks for fixing it. Should we revert the original change? I still think it's reasonable to remove the extra downcasing step, but we'll have to document it as a change. For instance, spelling C as either "C" or 'C' would work differently now. The fact that the former is downcased seems quite surprising to me, so I don't think anybody would say that this isn't a better definition, but undoubtedly it could force people to change their source files. 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 get tuple
On 17/11/11 19:16, Robert Haas wrote: On Wed, Nov 16, 2011 at 9:38 AM, Rudyar wrote: Hello, I'm new in postgreSQL programming. I try to print a tuple from tupleTableSlot structure.. for example.. outerTupleSlot = ExecHashJoinOuterGetTuple(outerNode, node, &hashvalue); how to extract a tuple value? there are any kind of documentation about that? A quick grep suggests that the "debugtup" function might be about what your're looking for. Thanks! -- Rudyar Cortés. Estudiante de Ingeniería Civil Informática Universidad Técnica Federico Santa María. -- Sent 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 get tuple
On Wed, Nov 16, 2011 at 9:38 AM, Rudyar wrote: > Hello, > > I'm new in postgreSQL programming. I try to print a tuple from > tupleTableSlot structure.. > for example.. > > outerTupleSlot = ExecHashJoinOuterGetTuple(outerNode, > node, > &hashvalue); > > how to extract a tuple value? > > there are any kind of documentation about that? A quick grep suggests that the "debugtup" function might be about what your're looking for. -- 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] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
On Thu, Nov 17, 2011 at 4:26 PM, Alvaro Herrera wrote: > So the buildfarm broke due to this change, because citext does Thanks for fixing it. Should we revert the original change? -- 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] Are range_before and range_after commutator operators?
Jeff Davis writes: > Yikes! While commenting the code, it turns out that I missed the case > where the values match and they are both exclusive; but one is upper and > the other lower. Worse than that, there were apparently some bogus test > results that expected the wrong output. Mea culpa. > Patch attached. I'll do another pass over some of the comments in other > parts of the code. Applied, thanks. These comments aren't quite what I'd hoped for though. What I'm lacking is the conceptual context, ie, why is a less-equal-greater primitive for bounds a good thing? It seems like when you consider the four possible directional (lower/upper) combinations, the same result from range_cmp_bounds means something different in each case, and I find that confusing. I wonder whether functions defined along set-theoretic lines (this bound is strictly weaker or stronger than this other one, or these bounds together define an empty or singleton or non-singleton set) might be more transparent. Maybe it would be enough just to document what the results mean in set-theoretic terms for each of the four cases. > And to your original question, it seems that << and >> should be > commutators. Perhaps I had a reason earlier, but it is escaping me now. > What edge cases did you have in mind? Well, I was (and remain) sufficiently unclear about the behavior of range_cmp_bounds to not be totally sure that they are commutators --- it's the dependency on the "lower" flags that makes this so unobvious for me. In particular, it seems like this reduces to the statement that range_cmp_bounds(x, y) > 0 is equivalent to range_cmp_bounds(y, x) < 0, at least when x and y have different directionality. After working through the code again I guess that's true, but it's less than obvious. 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] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
Excerpts from Robert Haas's message of jue nov 17 16:25:03 -0300 2011: > On Thu, Nov 17, 2011 at 1:00 PM, Tom Lane wrote: > > Robert Haas writes: > >> - For some reason, we have code that causes procedural language names > >> to be downcased before use. > > > > I think this is a hangover from the fact that CREATE FUNCTION's LANGUAGE > > clause used to insist on the language name being a string literal, and > > of course the lexer didn't case-fold it then. That's been deprecated > > for long enough that we probably don't need to have the extra case-fold > > step anymore. > > OK, great. So the buildfarm broke due to this change, because citext does -- -- Aggregates. -- CREATE FUNCTION citext_smaller(citext, citext) RETURNS citext AS 'MODULE_PATHNAME' LANGUAGE 'C' IMMUTABLE STRICT; CREATE FUNCTION citext_larger(citext, citext) RETURNS citext AS 'MODULE_PATHNAME' LANGUAGE 'C' IMMUTABLE STRICT; -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] RangeVarGetRelid()
Excerpts from Robert Haas's message of jue nov 17 17:51:06 -0300 2011: > The trouble is, I'm not quite sure how to do that. It seems like > permissions checks and lock-the-heap-for-this-index should be done in > RangeVarGetRelid() just after the block that says "if (retry)" and > just before the block that calls LockRelationOid(). That way, if we > end up deciding we need to retry the name lookup, we'll retry all that > other stuff as well, which is exactly right. The difficulty is that > different callers have different needs for what should go in that > space, to the degree that I'm a bit nervous about continuing to add > arguments to that function to satisfy what everybody needs. Maybe we > could make most of them Booleans and pass an "int flags" argument. > Another option would be to add a "callback" argument to that function > that would be called at that point with the relation, relId, and > oldRelId as arguments. Alternatively, we could just resign ourselves > to duplicating the loop in this function into each place in the code > that has a special-purpose requirement, but the function is complex > enough to make that a pretty unappealing prospect. I'm for the callback. -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] RangeVarGetRelid()
In commit 4240e429d0c2d889d0cda23c618f94e12c13ade7, we modified RangeVarGetRelid() so that it acquires a lock on the target relation "atomically" with respect to the name lookup. Since we lock OIDs, not names, that's not possible, strictly speaking, but the idea is that we detect whether any invalidation messages were processed between the time we did the lookup and the time we acquired the relation lock. If so, we double-check that the name still refers to the same object, and if not, we release the lock on the object we had locked previously and instead lock the new one. This is a good thing, because it means that if you, for example, start a transaction, drop a table, create a new one in its place, and commit the transaction, people who were blocked waiting for the table lock will correctly latch onto the new table instead of erroring out all over the place. This is obviously advantageous in production situations where switching out tables in this way has historically been quite difficult to do without user-visible disruption. The trouble with this mechanism, however, is that sometimes atomically looking up the relation and locking it is not what you want to do. For example, you might want to have a permission check in the middle, so that you don't even briefly lock a table on which the user has no permissions. Or, in the case of DROP INDEX, you might find it necessary to lock the heap before the index, as a result of our general rule that the heap locks should be acquired before index locks to reduce deadlocks. Right now, there's no good way to do this. Some code just opens the target relation with whatever lock is needed from the get-go, and we just hope the transaction will abort before it causes anyone else too much trouble. Other code looks up the relation OID without getting a lock, does its checks, and then gets the lock - but all of the places that take this approach uniformly lack the sort of retry loop that is present in RangeVarGetRelid() - and are therefore prone to latching onto a dropped relation, or maybe even checking permissions on the relation with OID 123 and then dropping the (eponymous) relation with OID 456. Although none of this seems like a huge problem in practice, it's awfully ugly, and it seems like it would be nice to clean it up. The trouble is, I'm not quite sure how to do that. It seems like permissions checks and lock-the-heap-for-this-index should be done in RangeVarGetRelid() just after the block that says "if (retry)" and just before the block that calls LockRelationOid(). That way, if we end up deciding we need to retry the name lookup, we'll retry all that other stuff as well, which is exactly right. The difficulty is that different callers have different needs for what should go in that space, to the degree that I'm a bit nervous about continuing to add arguments to that function to satisfy what everybody needs. Maybe we could make most of them Booleans and pass an "int flags" argument. Another option would be to add a "callback" argument to that function that would be called at that point with the relation, relId, and oldRelId as arguments. Alternatively, we could just resign ourselves to duplicating the loop in this function into each place in the code that has a special-purpose requirement, but the function is complex enough to make that a pretty unappealing prospect. Or we could just punt the whole thing and do nothing, but I don't like that either. Right now, for example, if user A is doing something with a table, and user B (who has no permissions) tries to use it, the pending request for an AccessExclusiveLock will block user C (who does have permissions) from doing anything until A's transaction commits or rolls back; only at that point do we realize that B has been naughty. I'd like to figure out some way to fix that. -- 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] declarations of range-vs-element <@ and @>
Jeff Davis writes:
> On Wed, 2011-11-16 at 16:41 -0500, Tom Lane wrote:
>> I propose adding a step to func_select_candidate
>> that tries to resolve things that way, ie, if all the known-type inputs
>> have the same type, then try assuming that the unknown-type ones are of
>> that type, and see if that leads to a unique match. There actually is a
>> comment in there that claims we do that, but the code it's attached to
>> is really doing something else that involves preferred types within
>> type categories...
>>
>> Thoughts?
> That sounds reasonable to me.
Here's a draft patch (sans doc changes as yet) that extends the
ambiguous-function resolution rules that way. It adds the heuristic at
the very end, at the point where we would otherwise fail, and therefore
it cannot change the system's behavior for any case that didn't
previously draw an "ambiguous function/operator" error. I experimented
with placing the heuristic earlier in func_select_candidate, but found
that that caused some changes in regression test cases, which made me a
bit nervous. Those changes were not clearly worse results, but this
isn't an area that I think we should toy with lightly.
I haven't yet tried again on changing the <@ and @> declarations, but
will do that next.
regards, tom lane
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 75f1e20475d1c2df628f0a866fc081c601340e98..01ed85b563d23e9288430a76b28aa5a7b2550b74 100644
*** a/src/backend/parser/parse_func.c
--- b/src/backend/parser/parse_func.c
*** func_select_candidate(int nargs,
*** 618,631
Oid *input_typeids,
FuncCandidateList candidates)
{
! FuncCandidateList current_candidate;
! FuncCandidateList last_candidate;
Oid *current_typeids;
Oid current_type;
int i;
int ncandidates;
int nbestMatch,
! nmatch;
Oid input_base_typeids[FUNC_MAX_ARGS];
TYPCATEGORY slot_category[FUNC_MAX_ARGS],
current_category;
--- 618,633
Oid *input_typeids,
FuncCandidateList candidates)
{
! FuncCandidateList current_candidate,
! first_candidate,
! last_candidate;
Oid *current_typeids;
Oid current_type;
int i;
int ncandidates;
int nbestMatch,
! nmatch,
! nunknowns;
Oid input_base_typeids[FUNC_MAX_ARGS];
TYPCATEGORY slot_category[FUNC_MAX_ARGS],
current_category;
*** func_select_candidate(int nargs,
*** 651,659
* take a domain as an input datatype. Such a function will be selected
* over the base-type function only if it is an exact match at all
* argument positions, and so was already chosen by our caller.
*/
for (i = 0; i < nargs; i++)
! input_base_typeids[i] = getBaseType(input_typeids[i]);
/*
* Run through all candidates and keep those with the most matches on
--- 653,674
* take a domain as an input datatype. Such a function will be selected
* over the base-type function only if it is an exact match at all
* argument positions, and so was already chosen by our caller.
+ *
+ * While we're at it, count the number of unknown-type arguments for use
+ * later.
*/
+ nunknowns = 0;
for (i = 0; i < nargs; i++)
! {
! if (input_typeids[i] != UNKNOWNOID)
! input_base_typeids[i] = getBaseType(input_typeids[i]);
! else
! {
! /* no need to call getBaseType on UNKNOWNOID */
! input_base_typeids[i] = UNKNOWNOID;
! nunknowns++;
! }
! }
/*
* Run through all candidates and keep those with the most matches on
*** func_select_candidate(int nargs,
*** 749,762
return candidates;
/*
! * Still too many candidates? Try assigning types for the unknown columns.
! *
! * NOTE: for a binary operator with one unknown and one non-unknown input,
! * we already tried the heuristic of looking for a candidate with the
! * known input type on both sides (see binary_oper_exact()). That's
! * essentially a special case of the general algorithm we try next.
*
! * We do this by examining each unknown argument position to see if we can
* determine a "type category" for it. If any candidate has an input
* datatype of STRING category, use STRING category (this bias towards
* STRING is appropriate since unknown-type literals look like strings).
--- 764,779
return candidates;
/*
! * Still too many candidates? Try assigning types for the unknown inputs.
*
! * If there are no unknown inputs, we have no more heuristics that apply,
! * and must fail.
! */
! if (nunknowns == 0)
! return NULL; /* failed to select a best candidate */
!
! /*
! * The next step examines each unknown argument position to see if we can
* determine a "type category" for it. If any candidate has an input
* datatype of STRING category, use STRING category (this bias towards
* STRING is appropriate since unknown-type literals look like
Re: [HACKERS] Core Extensions relocation
On mån, 2011-11-14 at 20:44 -0500, Greg Smith wrote: > The very specific problem I was most concerned about eliminating was > people discovering they needed an extension to troubleshoot > performance or corruption issues, only to discover it wasn't > available--because they hadn't installed the postgresql-contrib > package. Who's to say that after this, the core extensions won't end up in a new separate package postgresql-extensions (or similar) or might even stay in postgresql-contrib, for compatibility? -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
On Thu, Nov 17, 2011 at 1:00 PM, Tom Lane wrote: > Robert Haas writes: >> It also eliminates the NOTICE when removing a built-in >> function, which I think is OK because you don't actually get that far: > > There are paths that can reach that notice --- I think what you have to > do is create a new function that references a built-in one. But why > we bother to warn for that isn't clear to me. > >> - For some reason, we have code that causes procedural language names >> to be downcased before use. > > I think this is a hangover from the fact that CREATE FUNCTION's LANGUAGE > clause used to insist on the language name being a string literal, and > of course the lexer didn't case-fold it then. That's been deprecated > for long enough that we probably don't need to have the extra case-fold > step anymore. OK, great. -- 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] declarations of range-vs-element <@ and @>
On Wed, 2011-11-16 at 16:41 -0500, Tom Lane wrote: > But what surprises me about this example is that I'd have expected the > heuristic "assume the unknown is of the same type as the other input" > to resolve it. Looking more closely, I see that we apply that heuristic > in such a way that it works only for exact operator matches, not for > matches requiring coercion (including polymorphic-type matches). This > seems a bit weird. I propose adding a step to func_select_candidate > that tries to resolve things that way, ie, if all the known-type inputs > have the same type, then try assuming that the unknown-type ones are of > that type, and see if that leads to a unique match. There actually is a > comment in there that claims we do that, but the code it's attached to > is really doing something else that involves preferred types within > type categories... > > Thoughts? That sounds reasonable to me. Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Are range_before and range_after commutator operators?
On Wed, 2011-11-16 at 17:53 -0500, Tom Lane wrote: > I noticed that << and >> are not marked as commutator operators, > though a naive view of their semantics suggests they should be. > However, I realized that there might be edge cases I wasn't thinking > about, so I went looking in the patch to try to confirm this. And > I found neither a single line of documentation about it, nor a single > comment in that hairy little nest of unobvious tests that calls itself > range_cmp_bounds. I am of the opinion that that routine not only > requires a comment, but very possibly a comment longer than the routine > itself. What's more, if it's this complicated to code, surely it would > be a good idea for the user-facing documentation to explain exactly > what we think before/after mean? > > In general, the level of commenting in the rangetypes code seems far short > of what I'd consider acceptable for Postgres code. I plan to fix some > of that myself, but I do not wish to reverse-engineer what the heck > range_cmp_bounds thinks it's doing. Yikes! While commenting the code, it turns out that I missed the case where the values match and they are both exclusive; but one is upper and the other lower. Worse than that, there were apparently some bogus test results that expected the wrong output. Mea culpa. Patch attached. I'll do another pass over some of the comments in other parts of the code. And to your original question, it seems that << and >> should be commutators. Perhaps I had a reason earlier, but it is escaping me now. What edge cases did you have in mind? Regards, Jeff Davis cmp-bounds-2017.gz Description: GNU Zip compressed data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
Robert Haas writes: > It also eliminates the NOTICE when removing a built-in > function, which I think is OK because you don't actually get that far: There are paths that can reach that notice --- I think what you have to do is create a new function that references a built-in one. But why we bother to warn for that isn't clear to me. > - For some reason, we have code that causes procedural language names > to be downcased before use. I think this is a hangover from the fact that CREATE FUNCTION's LANGUAGE clause used to insist on the language name being a string literal, and of course the lexer didn't case-fold it then. That's been deprecated for long enough that we probably don't need to have the extra case-fold step anymore. 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] ISN was: Core Extensions relocation
Robert Haas writes: > At the same time, I still think we should push this out to PGXN or > pgfoundry or something. The fact that it's useful to some people does > not mean that it's a good example for other people to follow, or that > we want the core distribution to be in the process of tracking ISBN > prefixes on behalf of PostgreSQL users everywhere. I wouldn't object to that as long as we replaced it with some other module that had a similar relationship to core types. We'd never have realized the need for CREATE TYPE's LIKE option, until it was far too late, if we'd not had contrib/isn to show up the problem (cf commit 3f936aacc057e4b391ab953fea2ffb689a12a8bc). But really I think this discussion is making a mountain out of a molehill. If tracking ISBN prefix changes is such a time-consuming activity, why have we not seen a steady stream of update patches from users? By my count there's been just one such patch since 2006 (commit 6d1af7b2180719102a907bd3e35d218b43e76ad1). 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] ISN was: Core Extensions relocation
On Thu, Nov 17, 2011 at 10:44 AM, Peter Geoghegan wrote: > I think that's it's rather unlikely that removing hyphenation and > prefix validation would adversely affect anyone, provided that it was > well documented and wasn't applied to stable branches. If it were up > to me, I might remove validation from stable branches but keep > hyphenation, while removing both for 9.2 . After all, hyphenation will > break anyway, so they're worse off continuing to rely on hyphenation > when it cannot actually be relied on. Well, keep in mind that most people test their code. It seems likely that it actually DOES work pretty well for the people who are using the module. The ones for whom it didn't work presumably would have complained (and, mostly, they haven't) or abandoned the module (in which case they're irrelevant to the discussion of who might be hurt by this change). I'd be willing to bet a nickle that we'll get complaints if we rip that hyphenation behavior out. At the same time, I still think we should push this out to PGXN or pgfoundry or something. The fact that it's useful to some people does not mean that it's a good example for other people to follow, or that we want the core distribution to be in the process of tracking ISBN prefixes on behalf of PostgreSQL users everywhere. -- 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] IDLE in transaction introspection
On Wed, Nov 16, 2011 at 4:09 PM, Scott Mead wrote: > > > On Tue, Nov 15, 2011 at 1:18 PM, Robert Treat wrote: > >> On Tue, Nov 15, 2011 at 12:00 PM, Greg Smith >> wrote: >> > On 11/15/2011 09:44 AM, Scott Mead wrote: >> >> >> >> Fell off the map last week, here's v2 that: >> >> * RUNNING => active >> >> * all states from CAPS to lower case >> >> >> > >> > This looks like what I was hoping someone would add here now. Patch >> looks >> > good, only issue I noticed was a spaces instead of a tab goof where you >> set >> > beentry_>st_state at line 2419 in src/backend/postmaster/pgstat.c >> > >> > Missing pieces: >> > >> > -There is one regression test that uses pg_stat_activity that is broken >> now. >> > -The documentation should list the new field and all of the states it >> might >> > include. That's a serious doc update from the minimal info available >> right >> > now. >> > I'm working on the doc update now, and just realized something interesting: My patch doesn't take the 'query_start' column into account. Basically, the query_start column actually corresponds to the most recent update of the 'state' field. It'd be easy enough to tie it to the 'query' field so that we are hooked in. The problem is, if I'm monitoring this, now I don't know how long I've been 'idle in transaction' for, I would only know that the last query started at 'query_start'. AFAICS: *) Adjust the query_start column to point only to the actual 'query' start time *) Allow the query_start column to be updated as part of the 'state' change *) Add a 'state_change' column (timestamp) Looking for opinions here... -- Scott Mead OpenSCG, http://www.openscg.com > >> > I know this issue has been beat up already some, but let me summarize >> and >> > extend that thinking a moment. I see two equally valid schools of >> thought >> > on how exactly to deal with introducing this change: >> > >> > -Add the new state field just as you've done it, but keep updating the >> query >> > text anyway. Do not rename current_query. Declare the overloading of >> > current_query as both a state and the query text to be deprecated in >> 9.3. >> > This would keep existing tools working fine against 9.2 and give a >> clean >> > transition period. >> > >> > -Forget about backward compatibility and just put all the breaking stuff >> > we've been meaning to do in here. If we're going to rename >> current_query to >> > query--what Scott's patch does here--that will force all code using >> > pg_stat_activity to be rewritten. This seems like the perfect time to >> also >> > change "procpid" to "pid", finally blow away that wart. >> > >> >> +1 >> > +1 for me as well. > > Anybody else? > > >> >> > I'll happily update all of the tools and samples I deal with to support >> this >> > change. Most of the ones I can think of will be simplified; they're >> already >> > parsing query_text and extracting the implicit state. Just operating >> on an >> > explicit one instead will be simpler and more robust. >> > >> >> lmk if you need help, we'll be doing this with some of our tools / >> projects anyway, it probably wouldn't hurt to coordinate. >> >> >> Robert Treat >> conjecture: xzilla.net >> consulting: omniti.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] Refactoring on DROP/ALTER SET SCHEMA/ALTER RENAME TO statement
On Tue, Nov 15, 2011 at 4:43 AM, Kohei KaiGai wrote: > Part-1) DROP statement refactoring > It is a remaining portion of what I submitted in the last commit fest. > It allows object types that didn't used DropStmt in gram.y to go > through RemoveObjects(), instead of individual Remove(). Review of just this part: - I think we can remove the special case for foreign data wrappers because (1) the only case in which there's any behavioral difference at all is if a superuser creates a foreign data wrapper (or the ownership of one is reassigned to him) and he is then made not a superuser; non-superusers can't create foreign data wrappers, and existing foreign data wrappers can't be given to non-superusers; moreover, (2) removing the special case causes the behavior to match the documentation, which it currently doesn't (but only in the aforementioned, extremely minor way). - On the other hand, this patch blithely nukes the prohibition on using DROP FUNCTION to remove an aggregate. I'm not sure that's a good idea. It also eliminates the NOTICE when removing a built-in function, which I think is OK because you don't actually get that far: rhaas=# drop function int4pl(integer, integer); ERROR: cannot drop function int4pl(integer,integer) because it is required by the database system - For some reason, we have code that causes procedural language names to be downcased before use. Given that unquoted identifiers are downcased anyway, this seems a bit redundant. I'm inclined to think we don't need to preserve that behavior for DROP, especially because other parts of the code - such as COMMENT - don't know about it anyway. But rather than just changing it for DROP, I think we should go through and rip out case_translate_language_name() across the board, probably as a a separate commit. -- 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] Configuration include directory
Alvaro Herrera writes: > Excerpts from Tom Lane's message of mié nov 16 22:52:35 -0300 2011: >> (Do we guard against recursive inclusion via plain old include? If >> not, maybe this isn't worth worrying about.) > Yes, we do > FATAL: could not open configuration file "foo.conf": maximum nesting depth > exceeded Oh, right. So as long as the include-directory code path doesn't interfere with tracking that nesting depth, I don't think it needs any extra protection against include-the-same-directory. 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] Removing postgres -f command line option
Heikki Linnakangas writes: > While looking at Shigeru Hanada's foreign join pushdown patch, I noticed > a command line option that I didn't know to exist: > $ postgres --help > ... > Developer options: >-f s|i|n|m|hforbid use of some plan types Hmm, I thought I'd fixed that help message to match reality recently. > That seems completely useless to me, because you can also do "-c > enable_seqscan=off". Any objections to removing the -f option altogether? I use it. See also src/test/regress/regressplans.sh, which would become greatly less wieldy if it had to spell out the switches long-form. 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] ISN was: Core Extensions relocation
On 17 November 2011 03:54, Tom Lane wrote: > It's not reasonable to suppose > that nobody is using it today. I didn't suppose that no one is using it, but that those that are using it are unaware of the risks with prefix validation, and that there will be a rude awakening for them. > Ergo, we can't just summarily break > backwards compatibility on the grounds that we don't like the design. > Heck, we don't even have a field bug report that the design limitation > is causing any real problems for real users ... so IMO, the claims that > this is dangerously broken are a bit overblown. I think that's it's rather unlikely that removing hyphenation and prefix validation would adversely affect anyone, provided that it was well documented and wasn't applied to stable branches. If it were up to me, I might remove validation from stable branches but keep hyphenation, while removing both for 9.2 . After all, hyphenation will break anyway, so they're worse off continuing to rely on hyphenation when it cannot actually be relied on. On 17 November 2011 05:03, Josh Berkus wrote: > I do get the feeling that Peter got burned by ISN, given his vehemence > in erasing it from the face of the earth. So that's one bug report. ;-) Actually, I reviewed a band-aid patch about a year ago, and I was fairly concerned at the obvious wrong-headedness of something in our tree. I have a certain amount of domain knowledge here, but I've never actually attempted to use it in production. For all its faults, it is at least obviously broken to someone that knows about GS1 standards (having separate bar code datatypes is just not useful at all), although that tends to not be Americans. This may account for why we've heard so few complaints. It's also really easy and effective to create your own domain, and the flexibility that this affords tends to make an off-the-shelf solution unattractive (I've done things like store "compacted" bar codes that will subsequently be used to match a full bar code with an embedded price - I didn't want to enforce a check digit for the compacted representation). If I had a lot of time to work on fixing contrib/isn, I still wouldn't, because the correct thing to do is to produce your own domain. -- Peter Geoghegan http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training and Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Join push-down for foreign tables
Heikki Linnakangas writes: > When the FDW recognizes it's being asked to join a ForeignJoinPath and a > ForeignPath, or two ForeignJoinPaths, it throws away the old SQL it > constructed to do the two-way join, and builds a new one to join all > three tables. It should certainly not "throw away" the old SQL --- that path could still be chosen. > That seems tedious, when there are a lot of tables > involved. A FDW like the pgsql_fdw that constructs an SQL query doesn't > need to consider pairs of joins. It could just as well build the SQL for > the three-way join directly. I think the API needs to reflect that. > I wonder if we should have a heuristic to not even consider doing a join > locally, if it can be done remotely. I think this is a bad idea. It will require major restructuring of the planner, and sometimes it will fail to find the best plan, in return for not much. The nature of join planning is that we investigate a lot of dead ends. 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] Removing postgres -f command line option
Heikki, On 17.11.2011 10:19, Heikki Linnakangas wrote: $ postgres --help ... Developer options: -f s|i|n|m|hforbid use of some plan types That doesn't include all the options we support, the documentation lists: s|i|o|b|t|n|m|h. These are aliases for enable_* planner options, e.g -fs is equal to enable_seqscan=off. That seems completely useless to me, because you can also do "-c enable_seqscan=off". Any objections to removing the -f option altogether? I knew about it. But - I never needed it and I always scroll over it when I show --help in my trainings. When I was young - some when in last century - I learned that you never should remove a feature without pre-announcing it as deprecated. I think it is better to mark it deprecated in 9.2 and totally remove it in 9.3. Susanne -- Susanne Ebrecht - 2ndQuadrant PostgreSQL Development, 24x7 Support, Training and Services www.2ndQuadrant.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] SQLDA fix for ECPG
On Mon, Nov 14, 2011 at 09:06:30AM +0100, Boszormenyi Zoltan wrote: > Yes, you are right. For timestamp and interval, the safe alignment is int64. > Patch is attached. Applied, thanks. Michael -- Michael Meskes Michael at Fam-Meskes dot De, Michael at Meskes dot (De|Com|Net|Org) Michael at BorussiaFan dot De, Meskes at (Debian|Postgresql) dot Org Jabber: michael.meskes at googlemail dot com VfL Borussia! Força Barça! Go SF 49ers! Use Debian GNU/Linux, PostgreSQL -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Configuration include directory
Excerpts from Tom Lane's message of mié nov 16 22:52:35 -0300 2011: > (Do we guard against recursive inclusion via plain old include? If > not, maybe this isn't worth worrying about.) Yes, we do FATAL: could not open configuration file "foo.conf": maximum nesting depth exceeded -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent 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 postgres -f command line option
Excerpts from Robert Haas's message of jue nov 17 10:14:21 -0300 2011: > On Thu, Nov 17, 2011 at 4:19 AM, Heikki Linnakangas > wrote: > > While looking at Shigeru Hanada's foreign join pushdown patch, I noticed a > > command line option that I didn't know to exist: > > > > $ postgres --help > > ... > > Developer options: > > -f s|i|n|m|h forbid use of some plan types > > > > That doesn't include all the options we support, the documentation lists: > > s|i|o|b|t|n|m|h. These are aliases for enable_* planner options, e.g -fs is > > equal to enable_seqscan=off. > > > > That seems completely useless to me, because you can also do "-c > > enable_seqscan=off". Any objections to removing the -f option altogether? > > No. Seems useless to me. Dunno. It seems awfully convenient if you're testing optimizer stuff that requires turning one of these off, though of course I haven't ever done that. The alternative requires a lot more keystrokes. It doesn't matter to me either way, though. (To be honest I wasn't aware of these options -- not sure if I'll use them in the future) $ PGOPTIONS=-fs psql psql (9.2devel) Digite «help» para obtener ayuda. alvherre=# show enable_seqscan ; enable_seqscan off (1 fila) $ PGOPTIONS="-c enable_seqscan=0" psql psql (9.2devel) Digite «help» para obtener ayuda. alvherre=# show enable_seqscan ; enable_seqscan off (1 fila) -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent 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 postgres -f command line option
On Thu, Nov 17, 2011 at 4:19 AM, Heikki Linnakangas wrote: > While looking at Shigeru Hanada's foreign join pushdown patch, I noticed a > command line option that I didn't know to exist: > > $ postgres --help > ... > Developer options: > -f s|i|n|m|h forbid use of some plan types > > That doesn't include all the options we support, the documentation lists: > s|i|o|b|t|n|m|h. These are aliases for enable_* planner options, e.g -fs is > equal to enable_seqscan=off. > > That seems completely useless to me, because you can also do "-c > enable_seqscan=off". Any objections to removing the -f option altogether? No. Seems useless to me. -- 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] Adding Node support in outfuncs.c and readfuncs.c
Tom Lane writes: > What you've got here could be useful > to people who use emacs and understand they've got to hand-check the > results. I'm not sure how much further it'd be useful to go. Agreed. That's the reason why I'm proposing src/tools/editors in the first place. I find that it's enough for most of the Nodes I've been dealing with recently (all the ones that initdb uses, for starters), and for the other ones it helps a lot in adding the to-be-hand-edited code at the right place in the right files. The goal for this tool is to be more useful an advice to Emacs users than the usual "pick another patch that added syntax in the past and try to reproduce what it did as far as nodes support functions goes". I can also maintain that in a separate git repository on github, but that only reduces the already very thin population that could find it useful. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Removing postgres -f command line option
While looking at Shigeru Hanada's foreign join pushdown patch, I noticed a command line option that I didn't know to exist: $ postgres --help ... Developer options: -f s|i|n|m|hforbid use of some plan types That doesn't include all the options we support, the documentation lists: s|i|o|b|t|n|m|h. These are aliases for enable_* planner options, e.g -fs is equal to enable_seqscan=off. That seems completely useless to me, because you can also do "-c enable_seqscan=off". Any objections to removing the -f option altogether? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Disable OpenSSL compression
I wrote: > Here it is. I'll add it to the November commitfest. Here is the second version. I realized that it is better to set the option on the SSL object and not on the SSL context so that it is possible to change it per connection. I also improved the documentation. Yours, Laurenz Albe ssl.v2.patch Description: ssl.v2.patch -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WIP: Join push-down for foreign tables
On 15.11.2011 19:16, Shigeru Hanada wrote:
This is the second effort for $SUBJECT. Attached patch requires
pgsql_fdw patches[1] to be applied previously. This patch provides:
* Changes for backend
* Add new planner node ForeignJoinPath and related routines. In
current design, planner consider all of possible join combinations
between foreign tables, similar to local joins such as nested loop,
hash join and merge join. And if foreign join is cheapest, planner
produces a ForeignScan plan node for a join. So executor is not
modified heavily since 9.1.
* Add new FDW callback for planning join push-down between foreign
tables on same server. This function is optional, and allowed to
return NULL to tell planner that that join can't be handled by the
FDW.
So the way a three-way join is planned, is that the planner first asks
the FDW to plan ForeignPaths of scanning the individual tables. Then it
asks the FDW to consider pairwise joins of those ForeignPaths. Then it
asks the FDW to consider joins of the constructed ForeignPaths and
ForeignJoinPaths. Ie. the plan involving a join of three or more remote
tables is built bottom-up, just like a join of local tables.
When the FDW recognizes it's being asked to join a ForeignJoinPath and a
ForeignPath, or two ForeignJoinPaths, it throws away the old SQL it
constructed to do the two-way join, and builds a new one to join all
three tables. That seems tedious, when there are a lot of tables
involved. A FDW like the pgsql_fdw that constructs an SQL query doesn't
need to consider pairs of joins. It could just as well build the SQL for
the three-way join directly. I think the API needs to reflect that.
I wonder if we should have a heuristic to not even consider doing a join
locally, if it can be done remotely. For a query like this:
SELECT * FROM remote1 a, remote2 b, remote3 c WHERE a.id = b.id AND c.id
= b.id
it's quite obvious that the best plan is to do the join remotely, rather
than pull all the rows from all tables, and do the join locally. In
theory, if the remote database is remarkably bad at performing joins, it
might be faster to pull in all the data and do it locally, but I can't
really imagine that happening in practice.
* Changes for pgsql_fdw
* Implemente PlanForeignJoin callback function.
A couple of basic bugs I bumped into:
* WHERE-clause building fails on a cartesian product ("SELECT * FROM
remote1, remote2")
* The join planning in pgsql_fdw seems to get confused and gives up if
there are any local tables also involved in the query (e.g "explain
SELECT * FROM remote1, remote2 LEFT OUTER JOIN local1 on (local1.a =
remote2.a) WHERE remote1.a = remote2.a;")
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] (PATCH) Adding CORRESPONDING to Set Operations
On Mon, Nov 14, 2011 at 6:09 AM, Kerem Kat wrote: > On Mon, Nov 14, 2011 at 15:32, Tom Lane wrote: >> Kerem Kat writes: >>> Corresponding is currently implemented in the parse/analyze phase. If >>> it were to be implemented in the planning phase, explain output would >>> likely be as you expect it to be. >> >> It's already been pointed out to you that doing this at parse time is >> unacceptable, because of the implications for reverse-listing of rules >> (views). >> >> regards, tom lane >> > > I am well aware of that thank you. I took a quick look at the patch and found some miscellaneous points including: - don't use // style comment - keep consistent in terms of space around parenthesis like if and foreach - ereport should have error position as long as possible, especially in syntax error - I'm not convinced that new correspoinding_union.sql test is added. I prefer to include new tests in union.sql - CORRESPONDING BY should have column name list, not expression list. It's not legal to say CORRESPONDING BY (1 + 1) - column list rule should be presented in document, too - I don't see why you call checkWellFormedRecursionWalker on corresponding clause And more than above, Tom's point is the biggest blocker. I'd suggest to rework it so that target list of Query of RangeTblEntry on the top of tree have less columns that match the filter. By that way, I guess you can keep original information as well as filtered top-most target list. Eventually you need to work on the planner, too. Though I've not read all of the patch, the design rework should be done first. I'll mark this as Waiting on Author. Regards, -- Hitoshi Harada -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
