Re: [HACKERS] Initdb-cs_CZ.WIN-1250 buildfarm failures
On Sat, Dec 20, 2014 at 07:28:33PM +0100, Tomas Vondra wrote: On 20.12.2014 19:05, Tom Lane wrote: Locale cs_CZ.WIN-1250 is evidently marked with a codeset property of ANSI_X3.4-1968 (which means old-school US-ASCII). That's certainly wrong. I believe the correct thing would be CP1250. Yes. I fixed the locales and added the locales back to the client configuration. Thanks. These animals are now OK except on REL9_0_STABLE. The log message of commit 2dfa15d explains their ongoing 9.0 failures. I recommend adding --skip-steps=pl-install-check to their 9.0 invocations. Dropping the affected locales is another option, but we benefit from the rare encoding coverage more than we benefit from the src/bin/pl coverage. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_basebackup fails with long tablespace paths
23.12.2014, 05:00, Amit Kapila kirjoitti: On Tue, Dec 23, 2014 at 4:10 AM, Oskari Saarenmaa wrote: 08.11.2014, 04:03, Peter Eisentraut kirjoitti: It errors when the file name is too long and adds tests for that. This could be applied to 9.5 and backpatched, if we so choose. It might become obsolete if https://commitfest.postgresql.org/action/patch_view?id=1512 is accepted. If that patch doesn't get accepted, I might add my patch to a future commit fest. I think we should just use the UStar tar format (http://en.wikipedia.org/wiki/Tar_%28computing%29#UStar_format) and allow long file names; all actively used tar implementations should be able to handle them. I'll try to write a patch for that soonish. I think even using UStar format won't make it work for Windows where the standard utilities are not able to understand the symlinks in tar. There is already a patch [1] in this CF which will handle both cases, so I am not sure if it is very good idea to go with a new tar format to handle this issue. [1] : https://commitfest.postgresql.org/action/patch_view?id=1512 That patch makes sense for 9.5, but I don't think it's going to be backpatched to previous releases? I think we should also apply Peter's patch to master and backbranches to avoid creating invalid tar files anywhere. And optionally implement and backpatch long filename support in tar even if 9.5 no longer creates tar files with long names. / Oskari -- 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] REINDEX CONCURRENTLY 2.0
13.11.2014, 23:50, Andres Freund kirjoitti: On November 13, 2014 10:23:41 PM CET, Peter Eisentraut pete...@gmx.net wrote: On 11/12/14 7:31 PM, Andres Freund wrote: Yes, it sucks. But it beats not being able to reindex a relation with a primary key (referenced by a fkey) without waiting several hours by a couple magnitudes. And that's the current situation. That's fine, but we have, for better or worse, defined CONCURRENTLY := does not take exclusive locks. Use a different adverb for an in-between facility. I think that's not actually a service to our users. They'll have to adapt their scripts and knowledge when we get around to the more concurrent version. What exactly CONCURRENTLY means is already not strictly defined and differs between the actions. I'll note that DROP INDEX CONCURRENTLY actually already internally acquires an AEL lock. Although it's a bit harder to see the consequences of that. If the short-lived lock is the only blocker for this feature at the moment could we just require an additional qualifier for CONCURRENTLY (FORCE?) until the lock can be removed, something like: tmp# REINDEX INDEX CONCURRENTLY tmp_pkey; ERROR: REINDEX INDEX CONCURRENTLY is not fully concurrent; use REINDEX INDEX CONCURRENTLY FORCE to perform reindex with a short-lived lock. tmp=# REINDEX INDEX CONCURRENTLY FORCE tmp_pkey; REINDEX It's not optimal, but currently there's no way to reindex a primary key anywhere close to concurrently and a short lock would be a huge improvement over the current situation. / Oskari -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg
It seems that these two patches are being reviewed together. Should I just combine them into one? My understanding was that some wanted to review the memory accounting patch separately. On Sun, 2014-12-21 at 20:19 +0100, Tomas Vondra wrote: That's the only conflict, and after fixing it it compiles OK. However, I got a segfault on the very first query I tried :-( If lookup_hash_entry doesn't find the group, and there's not enough memory to create it, then it returns NULL; but the caller wasn't checking for NULL. My apologies for such a trivial mistake, I was doing most of my testing using DISTINCT. My fix here was done quickly, so I'll take a closer look later to make sure I didn't miss something else. New patch attached (rebased, as well). I also see your other message about adding regression testing. I'm hesitant to slow down the tests for everyone to run through this code path though. Should I add regression tests, and then remove them later after we're more comfortable that it works? Regards Jeff Davis *** a/doc/src/sgml/config.sgml --- b/doc/src/sgml/config.sgml *** *** 3045,3050 include_dir 'conf.d' --- 3045,3065 /listitem /varlistentry + varlistentry id=guc-enable-hashagg-disk xreflabel=enable_hashagg_disk + termvarnameenable_hashagg_disk/varname (typeboolean/type) + indexterm +primaryvarnameenable_hashagg_disk/ configuration parameter/primary + /indexterm + /term + listitem +para + Enables or disables the query planner's use of hashed aggregation plan + types when the planner expects the hash table size to exceed + varnamework_mem/varname. The default is literalon/. +/para + /listitem + /varlistentry + varlistentry id=guc-enable-hashjoin xreflabel=enable_hashjoin termvarnameenable_hashjoin/varname (typeboolean/type) indexterm *** a/src/backend/commands/explain.c --- b/src/backend/commands/explain.c *** *** 86,91 static void show_sort_group_keys(PlanState *planstate, const char *qlabel, --- 86,92 List *ancestors, ExplainState *es); static void show_sort_info(SortState *sortstate, ExplainState *es); static void show_hash_info(HashState *hashstate, ExplainState *es); + static void show_hashagg_info(AggState *hashstate, ExplainState *es); static void show_tidbitmap_info(BitmapHeapScanState *planstate, ExplainState *es); static void show_instrumentation_count(const char *qlabel, int which, *** *** 1422,1427 ExplainNode(PlanState *planstate, List *ancestors, --- 1423,1429 case T_Agg: show_agg_keys((AggState *) planstate, ancestors, es); show_upper_qual(plan-qual, Filter, planstate, ancestors, es); + show_hashagg_info((AggState *) planstate, es); if (plan-qual) show_instrumentation_count(Rows Removed by Filter, 1, planstate, es); *** *** 1912,1917 show_sort_info(SortState *sortstate, ExplainState *es) --- 1914,1955 } /* + * Show information on hash aggregate buckets and batches + */ + static void + show_hashagg_info(AggState *aggstate, ExplainState *es) + { + Agg *agg = (Agg *)aggstate-ss.ps.plan; + + Assert(IsA(aggstate, AggState)); + + if (agg-aggstrategy != AGG_HASHED) + return; + + if (!aggstate-hash_init_state) + { + long memPeakKb = (aggstate-hash_mem_peak + 1023) / 1024; + long diskKb = (aggstate-hash_disk + 1023) / 1024; + + if (es-format == EXPLAIN_FORMAT_TEXT) + { + appendStringInfoSpaces(es-str, es-indent * 2); + appendStringInfo( + es-str, + Batches: %d Memory Usage: %ldkB Disk Usage:%ldkB\n, + aggstate-hash_num_batches, memPeakKb, diskKb); + } + else + { + ExplainPropertyLong(HashAgg Batches, + aggstate-hash_num_batches, es); + ExplainPropertyLong(Peak Memory Usage, memPeakKb, es); + ExplainPropertyLong(Disk Usage, diskKb, es); + } + } + } + + /* * Show information on hash buckets/batches. */ static void *** a/src/backend/executor/execGrouping.c --- b/src/backend/executor/execGrouping.c *** *** 310,316 BuildTupleHashTable(int numCols, AttrNumber *keyColIdx, hash_ctl.hcxt = tablecxt; hashtable-hashtab = hash_create(TupleHashTable, nbuckets, hash_ctl, ! HASH_ELEM | HASH_FUNCTION | HASH_COMPARE | HASH_CONTEXT); return hashtable; } --- 310,317 hash_ctl.hcxt = tablecxt; hashtable-hashtab = hash_create(TupleHashTable, nbuckets, hash_ctl, ! HASH_ELEM | HASH_FUNCTION | HASH_COMPARE | ! HASH_CONTEXT | HASH_NOCHILDCXT); return hashtable; } *** *** 331,336 TupleHashEntry --- 332,386 LookupTupleHashEntry(TupleHashTable hashtable, TupleTableSlot *slot, bool *isnew) { + uint32 hashvalue; + + hashvalue = TupleHashEntryHash(hashtable, slot); + return LookupTupleHashEntryHash(hashtable,
[HACKERS] mysql with postgres
hi all, Is postgres source code compatible with mysql database?? If it is, could someone could give me some links so that I can do that. I want to hack into the postgres source code, but as I am comfortable with mysql, I want to use the mysql database not postgres. any references would be fine. thank you
Re: [HACKERS] mysql with postgres
On Tue, Dec 23, 2014 at 3:06 PM, Ravi Kiran ravi.kolanp...@gmail.com wrote: hi all, Is postgres source code compatible with mysql database?? If it is, could someone could give me some links so that I can do that. I want to hack into the postgres source code, but as I am comfortable with mysql, I want to use the mysql database not postgres. any references would be fine. thank you Eh...what?
Re: [HACKERS] compress method for spgist - 2
On 12/16/2014 07:48 PM, Teodor Sigaev wrote: /* * This struct is what we actually keep in index-rd_amcache. It includes * static configuration information as well as the lastUsedPages cache. */ typedef struct SpGistCache { spgConfigOut config;/* filled in by opclass config method */ SpGistTypeDesc attType; /* type of input data and leaf values */ SpGistTypeDesc attPrefixType; /* type of inner-tuple prefix values */ SpGistTypeDesc attLabelType;/* type of node label values */ SpGistLUPCache lastUsedPages; /* local storage of last-used info */ } SpGistCache; Now that the input data type and leaf data type can be different, which one is attType? It's the leaf data type, as the patch stands. I renamed that to attLeafType, and went fixing all the references to it. In most places it's just a matter of search replace, but what about the reconstructed datum? In freeScanStackEntry, we assume that att[Leaf]Type is the datatype for reconstructedValue, but I believe assume elsewhere that reconstructedValue is of the same data type as the input. At least if the opclass supports index-only scans. I think we'll need a separate SpGistTypeDesc for the input type. Or perhaps a separate SpGistTypeDesc for the reconstructed value and an optional decompress method to turn the reconstructedValue back into an actual reconstructed input datum. Or something like that. Attached is a patch with the kibitzing I did so far. - Heikki diff --git a/doc/src/sgml/spgist.sgml b/doc/src/sgml/spgist.sgml index 56827e5..de158c3 100644 --- a/doc/src/sgml/spgist.sgml +++ b/doc/src/sgml/spgist.sgml @@ -201,20 +201,21 @@ para There are five user-defined methods that an index operator class for - acronymSP-GiST/acronym must provide. All five follow the convention - of accepting two typeinternal/ arguments, the first of which is a - pointer to a C struct containing input values for the support method, - while the second argument is a pointer to a C struct where output values - must be placed. Four of the methods just return typevoid/, since - all their results appear in the output struct; but + acronymSP-GiST/acronym must provide and one optional. All five mandatory + methos follow the convention of accepting two typeinternal/ arguments, + the first of which is a pointer to a C struct containing input values for + the support method, while the second argument is a pointer to a C struct + where output values must be placed. Four of the methods just return + typevoid/, since all their results appear in the output struct; but functionleaf_consistent/ additionally returns a typeboolean/ result. The methods must not modify any fields of their input structs. In all cases, the output struct is initialized to zeroes before calling the - user-defined method. + user-defined method. Optional method functioncompress/ accepts + datum to be indexed and returns values which actually will be indexed. /para para - The five user-defined methods are: + The five mandatory user-defined methods are: /para variablelist @@ -244,6 +245,7 @@ typedef struct spgConfigOut { Oid prefixType; /* Data type of inner-tuple prefixes */ Oid labelType; /* Data type of inner-tuple node labels */ +Oid leafType; /* Data type of leaf */ boolcanReturnData; /* Opclass can reconstruct original data */ boollongValuesOK; /* Opclass can cope with values gt; 1 page */ } spgConfigOut; @@ -264,7 +266,15 @@ typedef struct spgConfigOut structfieldlongValuesOK/ should be set true only when the structfieldattType/ is of variable length and the operator class is capable of segmenting long values by repeated suffixing - (see xref linkend=spgist-limits). + (see xref linkend=spgist-limits). structfieldleafType/ + usually has the same value as structfieldattType/ but if + it's different then optional method functioncompress/ + should be provided. Method functioncompress/ is responsible + for transformation from structfieldattType/ to + structfieldleafType/. In this case all other function should + accept structfieldleafType/ values. Note: both consistent + functions will get structfieldscankeys/ unchanged, without + functioncompress/ transformation. /para /listitem /varlistentry @@ -690,6 +700,24 @@ typedef struct spgLeafConsistentOut /varlistentry /variablelist + para + The optional user-defined method is: + /para + + variablelist +varlistentry + termfunctionDatum compress(Datum in)//term + listitem + para + Converts the data item into a format suitable for physical storage in + an index page. It accepts structnamespgConfigIn/.structfieldattType/ + value and return structnamespgConfigOut/.structfieldleafType/ + value.
Re: [HACKERS] PATCH: adaptive ndistinct estimator v3 (WAS: Re: [PERFORM] Yet another abort-early plan disaster on 9.3)
On 12/07/2014 03:54 AM, Tomas Vondra wrote: The one interesting case is the 'step skew' with statistics_target=10, i.e. estimates based on mere 3000 rows. In that case, the adaptive estimator significantly overestimates: values currentadaptive -- 106 99 107 1068 6449190 1006 38 6449190 10006327 42441 I don't know why I didn't get these errors in the previous runs, because when I repeat the tests with the old patches I get similar results with a 'good' result from time to time. Apparently I had a lucky day back then :-/ I've been messing with the code for a few hours, and I haven't found any significant error in the implementation, so it seems that the estimator does not perform terribly well for very small samples (in this case it's 3000 rows out of 10.000.000 (i.e. ~0.03%). The paper [1] gives an equation for an upper bound of the error of this GEE estimator. How do the above numbers compare with that bound? [1] http://ftp.cse.buffalo.edu/users/azhang/disc/disc01/cd1/out/papers/pods/towardsestimatimosur.pdf - Heikki -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] speedup tidbitmap patch: cache page
Oh, that makes sense. Though I wonder if you need to clear the caches at all when calling tbm_lossify(). Surely it never becomes un-lossified and plus, at least for lossy_page it would never be set to the current page anyway, it's either going to be set to InvalidBlockNumber, or some other previous page that agree, fixed was lossy. I also can't quite see the need to set page to NULL. Surely doing this would just mean we'd have to lookup the page again once tbm_lossify() is called if the next loop happened to be the same page? I think this would only be needed if the hash lookup was going to return a new instance of the page after we've lossified it, which from what I can tell won't happen. Page could become an invalid pointer, because during tbm_mark_page_lossy() called from tbm_lossify() it could be freed. I've also attached some benchmark results using your original table from up-thread. It seems that the caching if the page was seen as lossy is not much of a help in this test case. Did you find another one where you saw some better gains? All what I found is about 0.5%... v3 contains your comments but it doesn't use lossy_page cache. -- Teodor Sigaev E-mail: teo...@sigaev.ru WWW: http://www.sigaev.ru/ tbm_cachepage-2.3.patch.gz Description: GNU Zip compressed data tbm_cachepage-3.patch.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] Missing updates at few places for row level security
Amit, * Amit Kapila (amit.kapil...@gmail.com) wrote: There is a new column added in pg_authid (rolbypassrls) and the updation for same is missed in below places: a. System catalog page for pg_authid http://www.postgresql.org/docs/devel/static/catalog-pg-authid.html Yup, thanks, will fix. b. Do we want to add this new column in pg_shadow view? This was intentionally not done as I had really viewed pg_user, pg_group and pg_shadow as deprecated and only for backwards-compatibility. That's certainly why those views were added originally. On the other hand, I do see that 'userepl' was added, so we've been keeping it updated. I am inclined to suggest that we actually mark pg_user, pg_group, and pg_shadow as deprecated and planned for removal. We can't simply remove them as we haven't actually said that but I don't think we should continue to carry these pre-8.1 views around forever. Still, until we do that, we should keep them updated. I'll make these changes in the next couple of days (after the role attribute bitmask patch, as that touches a lot of the same places). Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Initdb-cs_CZ.WIN-1250 buildfarm failures
On 23.12.2014 09:19, Noah Misch wrote: On Sat, Dec 20, 2014 at 07:28:33PM +0100, Tomas Vondra wrote: On 20.12.2014 19:05, Tom Lane wrote: Locale cs_CZ.WIN-1250 is evidently marked with a codeset property of ANSI_X3.4-1968 (which means old-school US-ASCII). That's certainly wrong. I believe the correct thing would be CP1250. Yes. I fixed the locales and added the locales back to the client configuration. Thanks. These animals are now OK except on REL9_0_STABLE. The log message of commit 2dfa15d explains their ongoing 9.0 failures. I recommend adding --skip-steps=pl-install-check to their 9.0 invocations. Dropping the affected locales is another option, but we benefit from the rare encoding coverage more than we benefit from the src/bin/pl coverage. OK, can do. Something like this in build-farm.conf should work, right? if ($branch eq 'REL9_0_STABLE') { push(@{$conf{config_opts}},--skip-steps=pl-install-check); } regards Tomas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] 9.5: Better memory accounting, towards memory-bounded HashAgg
On 23.12.2014 10:16, Jeff Davis wrote: It seems that these two patches are being reviewed together. Should I just combine them into one? My understanding was that some wanted to review the memory accounting patch separately. I think we should keep the patches separate. Applying two patches is trivial, splitting them not so much. On Sun, 2014-12-21 at 20:19 +0100, Tomas Vondra wrote: That's the only conflict, and after fixing it it compiles OK. However, I got a segfault on the very first query I tried :-( If lookup_hash_entry doesn't find the group, and there's not enough memory to create it, then it returns NULL; but the caller wasn't checking for NULL. My apologies for such a trivial mistake, I was doing most of my testing using DISTINCT. My fix here was done quickly, so I'll take a closer look later to make sure I didn't miss something else. New patch attached (rebased, as well). I also see your other message about adding regression testing. I'm hesitant to slow down the tests for everyone to run through this code path though. Should I add regression tests, and then remove them later after we're more comfortable that it works? I think when done right, the additional time will be negligible. By setting the work_mem low, we don't need that much data. regards Tomas -- 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] compress method for spgist - 2
Now that the input data type and leaf data type can be different, which one is attType? It's the leaf data type, as the patch stands. I renamed that to attLeafType, and went fixing all the references to it. In most places it's just a matter of search replace, but what about the reconstructed datum? In freeScanStackEntry, we assume that att[Leaf]Type is the datatype for reconstructedValue, but I believe assume elsewhere that reconstructedValue is of the same data type as the input. At least if the opclass supports index-only scans. Agree with rename. I doubt that there is a real-world example of datatype which can be a) effectivly compressed and b) restored to original form. If so, why don't store it in compressed state in database? In GiST all compress methods uses one-way compress. In PostGIS example, polygons are compressed into bounding box, and, obviously, they cannot be restored. I think we'll need a separate SpGistTypeDesc for the input type. Or perhaps a separate SpGistTypeDesc for the reconstructed value and an optional decompress method to turn the reconstructedValue back into an actual reconstructed input datum. Or something like that. I suppose that compress and reconstruct are mutual exclusive options. -- Teodor Sigaev E-mail: teo...@sigaev.ru WWW: http://www.sigaev.ru/ -- 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] Role Attribute Bitmask Catalog Representation
Adam Brightwell wrote: Alvaro and Stephen, I propose this patch on top of Adam's v5. Also included is a full patch against master. I have attached an updated patch for review (role-attribute-bitmask-v7.patch). This patch incorporates the 'v5a' patch proposed by Alvaro, input validation (Assert) check in 'check_role_attribute' and the documentation updates requested by Stephen. Pushed with a couple of small changes (Catalog.pm complained about the lack of a CATALOG() line in the new acldefs.h file; you had pg_role_all_attributes as returning bool in the table, but it returns text[]; and I added index entries for the new functions.) -- Á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] Proposal VACUUM SCHEMA
On Mon, Dec 22, 2014 at 5:00 PM, Jim Nasby jim.na...@bluetreble.com wrote: I would MUCH rather that we find a way to special-case executing non-transactional commands dynamically, because VACUUM isn't the only one that suffers from this problem. Is pg_background a solution to this problem? -- 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] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On Mon, Dec 22, 2014 at 5:04 PM, Peter Geoghegan p...@heroku.com wrote: If you're dead set on having an escape hatch, maybe we should just get over it and add a way of specifying a unique index by name. As I said, these under-served use cases are either exceedingly rare or entirely theoretical. I'm decidedly unenthusiastic about that. People don't expect CREATE INDEX CONCURRENTLY + DROP INDEX CONCURRENTLY to break their DML. I think the solution in this case would be a gateway to problems larger than the one we're trying to solve. -- 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] Final Patch for GROUPING SETS
On Mon, Dec 22, 2014 at 6:57 PM, Andrew Gierth and...@tao11.riddles.org.uk wrote: In the case of cube(a,b,c,d), our code currently gives: b,d,a,c: (b,d,a,c),(b,d) a,b,d:(a,b,d),(a,b) d,a,c:(d,a,c),(d,a),(d) c,d: (c,d),(c) b,c,d:(b,c,d),(b,c),(b) a,c,b:(a,c,b),(a,c),(a),() That's pretty cool. -- 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] Proposal VACUUM SCHEMA
On Mon, Dec 22, 2014 at 11:51 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Multi-table CLUSTER uses multiple transactions, so this should not be an issue. That said, I don't think there's much point in CLUSTER SCHEMA, much less TRUNCATE SCHEMA. Do you normally organize your schemas so that there are some that contain only tables that need to be truncated together? That would be a strange use case. Overall, this whole line of development seems like bloating the parse tables for little gain. We added REINDEX SCHEMA less than three weeks ago; if we accept that that was a good change, but think this is a bad one, it's not clear to me that there is any guiding principle here beyond who happened to weigh in on which threads. -- 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] [bug fix or improvement?] Correctly place DLLs for ECPG apps in bin folder
On Wed, Jul 9, 2014 at 9:53 PM, MauMau maumau...@gmail.com wrote: But the source tree contains as many as 206 Makefiles. I'm worried that it will waste the install time. Should all these Makefiles be examined, or 16 Makefiles in src/interfaces/? Not really. IMO the best solution is to extract the path of the Makefile in the vcxproj file of the project in Install.pm by looking at ModuleDefinitionFile or ResourceFile, and then to scan it for SO_MAJOR_VERSION. This way it is possible to determine in CopySolutionOutput where a library needs to be copied. I'd also rather go with the def file, and not the rc file for the path identification. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Initdb-cs_CZ.WIN-1250 buildfarm failures
On 12/23/2014 07:14 AM, Tomas Vondra wrote: On 23.12.2014 09:19, Noah Misch wrote: On Sat, Dec 20, 2014 at 07:28:33PM +0100, Tomas Vondra wrote: On 20.12.2014 19:05, Tom Lane wrote: Locale cs_CZ.WIN-1250 is evidently marked with a codeset property of ANSI_X3.4-1968 (which means old-school US-ASCII). That's certainly wrong. I believe the correct thing would be CP1250. Yes. I fixed the locales and added the locales back to the client configuration. Thanks. These animals are now OK except on REL9_0_STABLE. The log message of commit 2dfa15d explains their ongoing 9.0 failures. I recommend adding --skip-steps=pl-install-check to their 9.0 invocations. Dropping the affected locales is another option, but we benefit from the rare encoding coverage more than we benefit from the src/bin/pl coverage. OK, can do. Something like this in build-farm.conf should work, right? if ($branch eq 'REL9_0_STABLE') { push(@{$conf{config_opts}},--skip-steps=pl-install-check); } No, config_opts is what's passed to configure. Try something like: if ($branch eq 'REL9_0_STABLE') { $skip_steps{'pl-install-check'} = 1; } cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] mysql with postgres
On 12/23/2014 04:36 AM, Ravi Kiran wrote: hi all, Is postgres source code compatible with mysql database?? If it is, could someone could give me some links so that I can do that. I want to hack into the postgres source code, but as I am comfortable with mysql, I want to use the mysql database not postgres. any references would be fine. No. That's like asking if Unix is compatible with Windows, because you want to hack on Unix but you're comfortable with windows and want to use it. If you want to use mysql you should hack on it. If you want to use postgres hack on it. But you can't mix those two. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Initdb-cs_CZ.WIN-1250 buildfarm failures
On 23.12.2014 15:21, Andrew Dunstan wrote: No, config_opts is what's passed to configure. Try something like: if ($branch eq 'REL9_0_STABLE') { $skip_steps{'pl-install-check'} = 1; } Applied to all three animals. Tomas -- 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] pgbench -f and vacuum
On Mon, Dec 22, 2014 at 6:55 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Here's a completely different idea. How about we add an option that means vacuum this table before running the test (can be given several times); by default the set of vacuumed tables is the current pgbench_* list, but if -f is specified then the default set is cleared. So if you have a -f script and want to vacuum the default tables, you're forced to give a few --vacuum-table=foo options. But this gives the option to vacuum some other table before the test, not just the pgbench default ones. Well, really, you might want arbitrary initialization steps, not just vacuums. We could have --init-script=FILENAME. Although that might be taking this thread rather far off-topic. -- 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] Proposal VACUUM SCHEMA
On Mon, Dec 22, 2014 at 8:02 PM, Jim Nasby jim.na...@bluetreble.com wrote: On 12/21/14, 8:55 PM, Fabrízio de Royes Mello wrote: And why that, but not say schema-wide ANALYZE, CLUSTER, TRUNCATE, ... +1. I can write patches for each of this maintenance statement too. If we're going to go that route, then perhaps it would make more sense to create a command that allows you to apply a second command to every object in a schema. We would have to be careful about PreventTransactionChain commands. Sorry but I don't understand what you meant. Can you explain more about your idea? There's a very large number of commands that could be useful to execute on every object in a schema. (RE)INDEX, CLUSTER, ALTER come to mind besides VACUUM. ANALYZE too... Right now a lot of people just work around this with things like DO blocks, but as mentioned elsewhere in the thread that fails for commands that can't be in a transaction. I use dblink to solve it. :-) Regards, -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] Role Attribute Bitmask Catalog Representation
Alvarao, Pushed with a couple of small changes (Catalog.pm complained about the lack of a CATALOG() line in the new acldefs.h file; you had pg_role_all_attributes as returning bool in the table, but it returns text[]; and I added index entries for the new functions.) That's fantastic! Thanks, I appreciate it! -Adam -- Adam Brightwell - adam.brightw...@crunchydatasolutions.com Database Engineer - www.crunchydatasolutions.com
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Alvaro Herrera alvhe...@alvh.no-ip.org writes: Use a bitmask to represent role attributes The previous representation using a boolean column for each attribute would not scale as well as we want to add further attributes. Extra auxilliary functions are added to go along with this change, to make up for the lost convenience of access of the old representation. I have to apologize for not having paid more attention, but ... is this *really* such a great idea? You've just broken any client-side code that looks directly at pg_authid. Moreover, I don't particularly buy the idea that this somehow insulates us from the compatibility costs of adding new role properties: you're still going to have to add columns to the pg_roles view, and adjust clients that look at that, every time. Replacing bool-column accesses with bitmask manipulation doesn't seem like it's a win on a micro-optimization level either, certainly not for SQL-level coding where you've probably made it two orders of magnitude more expensive. And lastly, what happens when you run out of bits in that bigint column? Again, I suppose I should have objected earlier, but I really seriously doubt that this is a good idea. 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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Tom Lane wrote: Again, I suppose I should have objected earlier, but I really seriously doubt that this is a good idea. Ugh. I thought we had a consensus that this was the accepted way forward; that's my reading of the old thread, http://www.postgresql.org/message-id/flat/20141016133218.gw28...@tamriel.snowman.net#20141016133218.gw28...@tamriel.snowman.net Breaking clients was considered acceptable, which is why some of these functions were introduced. There were some differing opinions; Simon for instance suggested the use of an array rather than a bitmask, but that would have broken clients all the same. If there's strong opposition to this whole line of development, I can revert. Anyone else wants to give an opinion? -- Á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] Proposal VACUUM SCHEMA
Robert Haas wrote: On Mon, Dec 22, 2014 at 11:51 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Multi-table CLUSTER uses multiple transactions, so this should not be an issue. That said, I don't think there's much point in CLUSTER SCHEMA, much less TRUNCATE SCHEMA. Do you normally organize your schemas so that there are some that contain only tables that need to be truncated together? That would be a strange use case. Overall, this whole line of development seems like bloating the parse tables for little gain. We added REINDEX SCHEMA less than three weeks ago; if we accept that that was a good change, but think this is a bad one, it's not clear to me that there is any guiding principle here beyond who happened to weigh in on which threads. I didn't think much of REINDEX SCHEMA, TBH. -- Á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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Tom Lane (t...@sss.pgh.pa.us) wrote: Alvaro Herrera alvhe...@alvh.no-ip.org writes: Use a bitmask to represent role attributes The previous representation using a boolean column for each attribute would not scale as well as we want to add further attributes. Extra auxilliary functions are added to go along with this change, to make up for the lost convenience of access of the old representation. I have to apologize for not having paid more attention, but ... is this *really* such a great idea? You've just broken any client-side code that looks directly at pg_authid. Anything which looks at pg_authid for the relevant columns needs to be updated for the changes which were made for rolreplication previously, and now rolbypassrls and any other role attributes added. While I agree that it's something to consider (and even mentioned it earlier in the thread..), there wasn't any argument about it from those who were following the discussion. Perhaps we could have gone out of our way to ask the pgAdmin authors and other client-side utilities which look at these columns if this will more issues than new columns do. Moreover, I don't particularly buy the idea that this somehow insulates us from the compatibility costs of adding new role properties: you're still going to have to add columns to the pg_roles view, and adjust clients that look at that, every time. That's correct, however, this change wasn't intended to insulate anyone from those compatibility changes but rather to make better use of the bytes in each pg_authid record. We were already up to 8 individual bool columns, wasting space for each. Replacing bool-column accesses with bitmask manipulation doesn't seem like it's a win on a micro-optimization level either, certainly not for SQL-level coding where you've probably made it two orders of magnitude more expensive. I agree that it's more expensive for SQL users, but it's not our first use of a bitmask in the catalog. Perhaps this is more user-visible than other use-cases but we also provide views to address common usages in this case already, where we don't in other situations. And lastly, what happens when you run out of bits in that bigint column? We can add another, though this seems unlikely to happen given the previous discussion and review of what additional role attributes would be nice to add. There are some scenarios I've considered which would lead to using perhaps another 15-20, but 64 sure seems far off. Again, I suppose I should have objected earlier, but I really seriously doubt that this is a good idea. I tried to solicit discussion about these concerns earlier but we're all busy, no problem. For my 2c, I do think this is the right direction to go in as adding another 15 boolean columns to pg_authid definitely strikes me as a bad idea, but we can certainly discuss the trade-offs. Another proposal (from Simon, as I recall) was to use an array. That runs into nearly all the same problems and the on-disk representation ends up being even larger, which is why I didn't see that being a good idea. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Alvaro Herrera alvhe...@2ndquadrant.com writes: Tom Lane wrote: Again, I suppose I should have objected earlier, but I really seriously doubt that this is a good idea. Ugh. I thought we had a consensus that this was the accepted way forward; that's my reading of the old thread, http://www.postgresql.org/message-id/flat/20141016133218.gw28...@tamriel.snowman.net#20141016133218.gw28...@tamriel.snowman.net I was aware that we were thinking of introducing a bunch more role attributes, but I'm wondering what's the rationale for assuming that (a) they'll all be booleans, and (b) there will never, ever, be more than 64 of them. The argument that lots of boolean columns won't scale nicely doesn't seem to lead to the conclusion that a fixed-size bitmap is better. I'd have gone with just adding more bool columns as needed. 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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On Tue, Dec 23, 2014 at 10:26 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Tom Lane wrote: Again, I suppose I should have objected earlier, but I really seriously doubt that this is a good idea. Ugh. I thought we had a consensus that this was the accepted way forward; that's my reading of the old thread, http://www.postgresql.org/message-id/flat/20141016133218.gw28...@tamriel.snowman.net#20141016133218.gw28...@tamriel.snowman.net Breaking clients was considered acceptable, which is why some of these functions were introduced. There were some differing opinions; Simon for instance suggested the use of an array rather than a bitmask, but that would have broken clients all the same. If there's strong opposition to this whole line of development, I can revert. Anyone else wants to give an opinion? I would have preferred (and I believe argued for) keeping the existing catalog representation for existing attributes and using a bitmask for new ones, to avoid breaking client code. But I am not sure if that's actually the best decision. I find Tom's concern about needing more than 64 attributes to be ill-founded; I can't really see that happening on any timescale that matters. -- 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] pgbench -f and vacuum
Robert Haas wrote: On Mon, Dec 22, 2014 at 6:55 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Here's a completely different idea. How about we add an option that means vacuum this table before running the test (can be given several times); by default the set of vacuumed tables is the current pgbench_* list, but if -f is specified then the default set is cleared. So if you have a -f script and want to vacuum the default tables, you're forced to give a few --vacuum-table=foo options. But this gives the option to vacuum some other table before the test, not just the pgbench default ones. Well, really, you might want arbitrary initialization steps, not just vacuums. We could have --init-script=FILENAME. Init (pgbench -i) is the step that creates the tables and populates them, so I think this would need a different name, maybe startup, but otherwise yeah. Although that might be taking this thread rather far off-topic. Not really sure about that, because the only outstanding objection to this discussion is what happens in the startup stage if you specify -f. Right now vacuum is attempted on the standard tables, which is probably not the right thing in the vast majority of cases. But if we turn that off, how do we reinstate it for the rare cases that want it? Personally I would just leave it turned off and be done with it, but if we want to provide some way to re-enable it, this --startup-script=FILE gadget sounds like a pretty decent idea. -- Á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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On 2014-12-23 10:40:15 -0500, Robert Haas wrote: On Tue, Dec 23, 2014 at 10:26 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Tom Lane wrote: Again, I suppose I should have objected earlier, but I really seriously doubt that this is a good idea. Ugh. I thought we had a consensus that this was the accepted way forward; that's my reading of the old thread, http://www.postgresql.org/message-id/flat/20141016133218.gw28...@tamriel.snowman.net#20141016133218.gw28...@tamriel.snowman.net Breaking clients was considered acceptable, which is why some of these functions were introduced. There were some differing opinions; Simon for instance suggested the use of an array rather than a bitmask, but that would have broken clients all the same. If there's strong opposition to this whole line of development, I can revert. Anyone else wants to give an opinion? I would have preferred (and I believe argued for) keeping the existing catalog representation for existing attributes and using a bitmask for new ones, to avoid breaking client code. But I am not sure if that's actually the best decision. I personally think in this case the clear break is slightly better than having different styles of representation around for a long while. I find Tom's concern about needing more than 64 attributes to be ill-founded; I can't really see that happening on any timescale that matters. I personally would prefer a 'custom' type to represent the permissions. Internally that could very well be current bitmask, but the external representation could be more complex (i.e. some textual representation). That'd make it easy to make the representation wider/more complex if needed. 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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: I have to apologize for not having paid more attention, but ... is this *really* such a great idea? You've just broken any client-side code that looks directly at pg_authid. Anything which looks at pg_authid for the relevant columns needs to be updated for the changes which were made for rolreplication previously, and now rolbypassrls and any other role attributes added. Right. 95% of the compatibility costs of adding a property are going to be sunk in any case because clients will need to know what flags exist, how to spell them in CREATE/ALTER USER commands, etc. (Some of this could be alleviated perhaps if we invented a server-side function that produced a text string representing all of a user's properties, but that's quite orthogonal to this patch.) That's correct, however, this change wasn't intended to insulate anyone from those compatibility changes but rather to make better use of the bytes in each pg_authid record. We were already up to 8 individual bool columns, wasting space for each. You're really seriously concerned about a couple of dozen bytes per role? That is micro-optimization of the very worst sort. We are routinely wasting multiples of that on things like using name rather than a variable-length text representation for name columns, and I don't think anyone is especially concerned about that anymore. Maybe back in the nineties it'd have been worth bit-shaving that way. To me, a bitmask might make sense if the properties could usefully be manipulated as a unit, but I'm not seeing any such advantage here. For my 2c, I do think this is the right direction to go in as adding another 15 boolean columns to pg_authid definitely strikes me as a bad idea, but we can certainly discuss the trade-offs. Meh. To the extent that users look at pg_roles rather than pg_authid, it's going to look like another 15 boolean columns to them anyway ... except that now, those columns are suddenly a lot more expensive to read. 15-20 more bool columns sounds entirely fine to me. If we were talking a couple of hundred, I'd start to worry; but this approach would not handle that very well either. 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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Robert Haas robertmh...@gmail.com writes: I would have preferred (and I believe argued for) keeping the existing catalog representation for existing attributes and using a bitmask for new ones, to avoid breaking client code. But I am not sure if that's actually the best decision. I find Tom's concern about needing more than 64 attributes to be ill-founded; I can't really see that happening on any timescale that matters. I tend to agree, which is why I'm questioning the decision to not just keep adding bool columns. I don't see how that's not both more convenient and less surprising. 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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Tom Lane (t...@sss.pgh.pa.us) wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: Tom Lane wrote: Again, I suppose I should have objected earlier, but I really seriously doubt that this is a good idea. Ugh. I thought we had a consensus that this was the accepted way forward; that's my reading of the old thread, http://www.postgresql.org/message-id/flat/20141016133218.gw28...@tamriel.snowman.net#20141016133218.gw28...@tamriel.snowman.net I was aware that we were thinking of introducing a bunch more role attributes, but I'm wondering what's the rationale for assuming that (a) they'll all be booleans, I attempted to do a comprehensive review of the case by looking at all of the existing superuser checks and considering where it made sense to make things more flexible. The result was specifically that they were all boolean cases except for the previously-discussed 'create directory' idea. Had there been other cases which weren't boolean, I would have been looking for another representation. That said, this does not wall-off additional columns going into pg_authid later, if there are any non-boolean cases which would make sense. Those cases, I suspect, would lead to new catalog tables in their own right anyway though, rather than additional columns in pg_authid. The 'create directory' idea certainly made more sense to me with a new catalog table. Having an array of directories in pg_authid was never considered, though I did consider adding a catalog table which was essentially role+key/value where 'value' was an arbitrary string or perhaps bytea blob, but all the boolean cases would have gone into pg_authid and that new catalog would have only been used for the 'directory' case (or at least, I couldn't find any other use-cases for it in my review). and (b) there will never, ever, be more than 64 of them. I do not think this approach walls off adding more than 64. On the other hand, I don't like the idea of doubling the size of pg_authid if we do get to 64 because we really want to use boolean columns instead of a bitmap. The argument that lots of boolean columns won't scale nicely doesn't seem to lead to the conclusion that a fixed-size bitmap is better. Perhaps it's because I just recently came out of an environment where we were constantly fighting with size issues for boolean values, but boolean columns definitely don't scale nicely. I admit that we still have larger issues when it comes to running databases with lots of roles (which means people tend to not do it), such as being unable to partition catalog tables, but I'm still hopefull we'll one day get to a point where more users can exist as real database users (and therefore have the database enforcing the permission system, rather than each application having to write its own) and I'd rather not have pg_authid bloated without cause. I'd have gone with just adding more bool columns as needed. I don't think I was the only one concerned that adding a bunch of new columns would bloat the size of pg_authid and the C structure behind it, but I'm not remembering offhand who else considered it. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On 12/23/2014 04:46 PM, Andres Freund wrote: [snip] I find Tom's concern about needing more than 64 attributes to be ill-founded; I can't really see that happening on any timescale that matters. Hmm... most probably, not (or so I hope)... Unless we begin to add many differerent capabilities, like it was recently suggested. I, for one, have at least two of them to propose, but I guess not that many more should be needed. I personally would prefer a 'custom' type to represent the permissions. Internally that could very well be current bitmask, but the external representation could be more complex (i.e. some textual representation). That'd make it easy to make the representation wider/more complex if needed. Indeed, though this would imply adding a new bitstring? type to core Postgres. Do you have any further input on what this type would look like ? Any operators that might be useful? ISTM that this would actually be the greatest strength of a type proper (vs. hardcoded bit-wise operations in core) In any case, having the type's input/output perform the conversion from/to text is quite equivalent to the current implementation. Considering that this custom type would need to be in core, the differences should be minimal. Or am I missing something obvious? Thanks, / J.L. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On Tue, Dec 23, 2014 at 10:51 AM, Tom Lane t...@sss.pgh.pa.us wrote: Meh. To the extent that users look at pg_roles rather than pg_authid, it's going to look like another 15 boolean columns to them anyway ... except that now, those columns are suddenly a lot more expensive to read. Ugh. I think that's actually a really good point. I guess I'll +1 reverting this, then. -- 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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: I'd have gone with just adding more bool columns as needed. I don't think I was the only one concerned that adding a bunch of new columns would bloat the size of pg_authid and the C structure behind it, but I'm not remembering offhand who else considered it. Lessee, as of 9.4 pg_authid required 76 bytes per row, plus row header overhead that'd have probably pushed it to 104 bytes per row (more if you had non-null rolpassword or rolvaliduntil). If we add as many as 20 more booleans we'd be at 124 bytes per row, whereas with this approach we'd have, well, 104 bytes per row. I'm not seeing much benefit to justify such a drastic change of approach. 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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Andres Freund (and...@2ndquadrant.com) wrote: On 2014-12-23 10:40:15 -0500, Robert Haas wrote: I would have preferred (and I believe argued for) keeping the existing catalog representation for existing attributes and using a bitmask for new ones, to avoid breaking client code. But I am not sure if that's actually the best decision. I personally think in this case the clear break is slightly better than having different styles of representation around for a long while. Yes, I'm completely with Andres on this point. Having a mixed case where there are some boolean columns and then a bitmask strikes as the worst approach- it doesn't save anyone from the client-side code changes but rather makes them have to consider both ways and keep an internal list somewhere of which ones are in boolean columns and which are in the bitmask, yuck. I find Tom's concern about needing more than 64 attributes to be ill-founded; I can't really see that happening on any timescale that matters. I personally would prefer a 'custom' type to represent the permissions. Internally that could very well be current bitmask, but the external representation could be more complex (i.e. some textual representation). That'd make it easy to make the representation wider/more complex if needed. In some ways, I feel like this is what we actually have now.. If you consider pg_authid to be 'internal' and pg_roles to be 'external'. That said, I'm not against what you're proposing, but at the same time I'm not quite sure what that would end up looking like or how difficult it would be to support a complex type in the catalog and I don't think there's any way it would address the on-disk size concern, and if we have to start having a different C-level representation for pg_authid than the on-disk representation, well, that strikes me as a lot more complicated too.. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
José Luis Tallón wrote: On 12/23/2014 04:46 PM, Andres Freund wrote: I personally would prefer a 'custom' type to represent the permissions. Internally that could very well be current bitmask, but the external representation could be more complex (i.e. some textual representation). That'd make it easy to make the representation wider/more complex if needed. Indeed, though this would imply adding a new bitstring? type to core Postgres. We already have varlena bitstrings, in the guise of types bit and varbit. Do you have any further input on what this type would look like ? Any operators that might be useful? ISTM that this would actually be the greatest strength of a type proper (vs. hardcoded bit-wise operations in core) I imagine something like the reg* types (regclass, regtype etc): on input you can pass them an OID, or an possibly-qualified object name; internally they store the OID. You can cast them to OID to obtain the numerical value, or just print them out to get the possibly-qualified name. In the case at hand, on output you would get the equivalent of the text[] you get from pg_role_all_attributes(), and you can input it in the same way or you can input the bitmask; and the underlying storage is the bitmask. This doesn't solve the client compatibility break, or the issue that querying pg_roles is expensive. -- Á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] bin checks taking too long.
On 12/22/2014 10:41 PM, Peter Eisentraut wrote: On 12/22/14 7:56 PM, Andrew Dunstan wrote: Currently the buildfarm animal crake (my development instance) is running the bin check, but not any other animal. These tests still take for too long, not least because each set of tests requires a separate install. You can avoid that by using make installcheck. Not working too well: make -C pg_ctl installcheck make[1]: Entering directory `/home/bf/bfr/root/HEAD/pgsql.build/src/bin/pg_ctl' cd /home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/bin/pg_ctl TESTDIR='/home/bf/bfr/root/HEAD/pgsql.build/src/bin/pg_ctl' PATH=/home/bf/bfr/root/HEAD/inst/bin:$PATH PGPORT='65648' prove -I /home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/test/perl/ --verbose t/*.pl Use of uninitialized value $ENV{top_builddir} in concatenation (.) or string at t/001_start_stop.pl line 17. file not found: /src/test/regress/pg_regress at /home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/test/perl/TestLib.pm line 142. This was invoked like this: cd $pgsql/src/bin make NO_LOCALE=1 installcheck Note that it's a vpath build. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Robert Haas (robertmh...@gmail.com) wrote: On Tue, Dec 23, 2014 at 10:51 AM, Tom Lane t...@sss.pgh.pa.us wrote: Meh. To the extent that users look at pg_roles rather than pg_authid, it's going to look like another 15 boolean columns to them anyway ... except that now, those columns are suddenly a lot more expensive to read. Ugh. I think that's actually a really good point. I guess I'll +1 reverting this, then. If that's the only consideration for this, well, that's certainly quite straight-forward to change in the other direction too. The new function suggested by Andres actually makes it really easy to get a textual list of all the role attributes which a role has from the bitmask too. I was more concerned with the on-disk and C-level structure and size than about the time required to get at the value of each bit at the SQL-level. Thanks, Stephen signature.asc Description: Digital signature
[HACKERS] Proposal: two new role attributes and/or capabilities?
Hello, I've found myself needing two role capabilities? as of lately, when thinking about restricting some roles to the barely minimum allowed permissions needed to perform their duties ... as opposed to having a superuser role devoted to these task. The capabilities would be: * MAINTENANCE --- Ability to run VACUUM [ANALYZE | FREEZE] (but not VACUUM FULL), ANALYZE (including SET LOCAL statistics_target TO 1), REINDEX CONCURRENTLY (but not the blocking, regular, one) REFRESH MATERIALIZED VIEW CONCURRENTLY (but not the blocking one) COPY ??? Rationale: delegate the routine maintenance tasks to a low privilege role, which can't do harm (apart from some performance degradation) --- hence the no exclusive locking operations requirement. * IMPERSONATE --- Ability to do SET AUTHORIZATION TO some_role; and RESET AUTHORIZATION This might be further refined to provide a way to say This role is authorized to impersonate role1 but no other Rationale: for use by connection poolers (esp. pgBouncer), where the role used for connection would only have the LOGIN and IMPERSONATE privileges. The remaining operations would be authorized against the supplanted role (i.e. ability to create tables/indexes or views, perform DML and/or DDL, etc) AFAIK, a superuser role is needed for this purpose currently. The relevant code is quite simple and looks like it could be very useful. Any suggestions / input on this? I can certainly prepare a patch for this (bear with me, It'll be my first here), and I'm willing to include more features if deemed useful. Regards, / J.L. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Tom Lane (t...@sss.pgh.pa.us) wrote: Stephen Frost sfr...@snowman.net writes: * Tom Lane (t...@sss.pgh.pa.us) wrote: I'd have gone with just adding more bool columns as needed. I don't think I was the only one concerned that adding a bunch of new columns would bloat the size of pg_authid and the C structure behind it, but I'm not remembering offhand who else considered it. Lessee, as of 9.4 pg_authid required 76 bytes per row, plus row header overhead that'd have probably pushed it to 104 bytes per row (more if you had non-null rolpassword or rolvaliduntil). If we add as many as 20 more booleans we'd be at 124 bytes per row, whereas with this approach we'd have, well, 104 bytes per row. I'm not seeing much benefit to justify such a drastic change of approach. I suppose. I didn't consider it to be a terribly drastic change but rather simply using a better representation for a mostly-internal bit of data. It also lended itself pretty nicely to maniuplation (at least, imv, the code is a lot cleaner with the bitmask, but it's not a huge deal). Guess I had been expecting concerns to be raised around adding many more bytes where there wouldn't have been. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: two new role attributes and/or capabilities?
* José Luis Tallón (jltal...@adv-solutions.net) wrote: I've found myself needing two role capabilities? as of lately, when thinking about restricting some roles to the barely minimum allowed permissions needed to perform their duties ... as opposed to having a superuser role devoted to these task. Excellent. We've been looking at the same considerations. The capabilities would be: * MAINTENANCE --- Ability to run VACUUM [ANALYZE | FREEZE] (but not VACUUM FULL), ANALYZE (including SET LOCAL statistics_target TO 1), There's likely to be discussion about these from the perspective that you really shouldn't need to run them all that much. Why isn't autovacuum able to handle this? REINDEX CONCURRENTLY (but not the blocking, regular, one) REFRESH MATERIALIZED VIEW CONCURRENTLY (but not the blocking one) These are interesting, but would these make sense at the role level? Both of these commands explicitly take specific relations to operate against, after all. COPY ??? The question around this one goes back to the CREATE DIRECTORY discussion that happened this fall. I'm still hopeful that we can do *something* in this area, but I'm not sure what that's going to end up looking like. The problem with COPY is that it's either trivial to use it to become a superuser, or insanely difficult to secure sufficiently. Rationale: delegate the routine maintenance tasks to a low privilege role, which can't do harm (apart from some performance degradation) --- hence the no exclusive locking operations requirement. This makes sense for the reindex/refresh cases, though no harm might be over-stating it. * IMPERSONATE --- Ability to do SET AUTHORIZATION TO some_role; and RESET AUTHORIZATION This might be further refined to provide a way to say This role is authorized to impersonate role1 but no other Rationale: for use by connection poolers (esp. pgBouncer), where the role used for connection would only have the LOGIN and IMPERSONATE privileges. The remaining operations would be authorized against the supplanted role (i.e. ability to create tables/indexes or views, perform DML and/or DDL, etc) AFAIK, a superuser role is needed for this purpose currently. No.. You can have 'no-inherit' roles which you can use for exactly this purpose. The initial login role can have no rights on the database, except to SET ROLE to other roles which have been granted to it. You should never have your pgBouncer or other pooling connection logging in as a superuser. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Stephen Frost sfr...@snowman.net writes: If that's the only consideration for this, well, that's certainly quite straight-forward to change in the other direction too. The new function suggested by Andres actually makes it really easy to get a textual list of all the role attributes which a role has from the bitmask too. We could have that regardless of the representation, if the function is defined along the lines of given a user OID, give me a text string representing the user's attributes. However, that only helps for pg_dumpall and any other clients whose requirement is exactly satisfied by a string that fits into CREATE/ALTER USER. The current formatting of psql's \du, for example, absolutely requires adding more client-side code every time we add a property; whether the catalog representation is bools or a bitmask really isn't going to change the pain level much there. 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] explain sortorder
Looking forward to the new patch. I'll give it a more complete testing when you post it. Mike __ *Mike Blackwell | Technical Analyst, Distribution Services/Rollout Management | RR Donnelley* 1750 Wallace Ave | St Charles, IL 60174-3401 Office: 630.313.7818 mike.blackw...@rrd.com http://www.rrdonnelley.com http://www.rrdonnelley.com/ * mike.blackw...@rrd.com* On Tue, Dec 23, 2014 at 5:11 AM, Arne Scheffer arne.schef...@uni-muenster.de wrote: Heikki Linnakangas hlinnakangas(at)vmware(dot)com writes: I would suggest just adding the information to the Sort Key line. As long as you don't print the modifiers when they are defaults (ASC and NULLS LAST), we could print the information even in non-VERBOSE mode. +1. I had assumed without looking that that was what it did already, else I'd have complained too. regards, tom lane We will change the patch according to Heikkis suggestions. A nice Christmas all the best in the New Year Arne Scheffer http://www.uni-muenster.de/ZIV/Mitarbeiter/ArneScheffer.shtml
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On Tue, Dec 23, 2014 at 11:34:09AM -0500, Tom Lane wrote: Stephen Frost sfr...@snowman.net writes: If that's the only consideration for this, well, that's certainly quite straight-forward to change in the other direction too. The new function suggested by Andres actually makes it really easy to get a textual list of all the role attributes which a role has from the bitmask too. We could have that regardless of the representation, if the function is defined along the lines of given a user OID, give me a text string representing the user's attributes. However, that only helps for pg_dumpall and any other clients whose requirement is exactly satisfied by a string that fits into CREATE/ALTER USER. The current formatting of psql's \du, for example, absolutely requires adding more client-side code every time we add a property; whether the catalog representation is bools or a bitmask really isn't going to change the pain level much there. I am with Tom on this --- there is more wasted space in the 'name' column pg_authid.rolname than by shoving 40 boolean values into a bitmap. Adding the complexity of a bitmap doesn't make sense here. I also apologize for the late feedback. Offtopic, what I would really _love_ to see improved is our display of object permissions: Access privileges Schema | Name | Type | Access privileges | Column privileges | Policies ++---+---+---+-- public | crypto | table | postgres=arwdDxt/postgres+| | || | =r/postgres | | That is nasty user display --- it looks like line noise. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Proposal: two new role attributes and/or capabilities?
On 12/23/2014 05:29 PM, Stephen Frost wrote: * José Luis Tallón (jltal...@adv-solutions.net) wrote: I've found myself needing two role capabilities? as of lately, when thinking about restricting some roles to the barely minimum allowed permissions needed to perform their duties ... as opposed to having a superuser role devoted to these tasks. Excellent. We've been looking at the same considerations. The capabilities would be: * MAINTENANCE --- Ability to run VACUUM [ANALYZE | FREEZE] (but not VACUUM FULL), ANALYZE (including SET LOCAL statistics_target TO 1), There's likely to be discussion about these from the perspective that you really shouldn't need to run them all that much. Why isn't autovacuum able to handle this? For some (arguably, ill-devised) use cases of INSERT - SELECT aggregate - DELETE (third party, closed-source app, massive insert rate) at the very least, autovacuum can't possibly cope with the change rate in some tables, given that there are quite many other interactive queries running. Manually performing VACUUM / VACUUM ANALYZE on the (few) affected tables every 12h or so fixes the performance problem for the particular queries without impacting the other users too much --- the tables and indexes in question have been moved to a separate tablespace/disk volume of their own. In short, this addresses situations where some tables have a much higher update rate than the rest of the database so that performance degrades with time --- the application became unusable after about 6 days' worth of updates until the manual vacuums were setup REINDEX CONCURRENTLY (but not the blocking, regular, one) REFRESH MATERIALIZED VIEW CONCURRENTLY (but not the blocking one) These are interesting, but would these make sense at the role level? Both of these commands explicitly take specific relations to operate against, after all. Yup. Let's imagine a cron job invoking psql in order to perform maintenance routine. The particular command(s) can be generated on-the-fly by querying the catalog and then send them in one go to be run sequentially by the one backend as a crude form of rate limiting/quality-of-service of sorts (renice -p or even ionice -p seems quite inadequate). This automation becomes impossible to do if the object owners differ (only the owner or a superuser can perform these operations AFAICS -- there is no mention of it in the current documentation) unless the DBA makes the maintenance role a member of every other role ... which quickly becomes a problem. COPY ??? The question around this one goes back to the CREATE DIRECTORY discussion that happened this fall. I'm still hopeful that we can do *something* in this area, but I'm not sure what that's going to end up looking like. The problem with COPY is that it's either trivial to use it to become a superuser, or insanely difficult to secure sufficiently. Yes. That's the reason for the question marks :-\ Some dump to csv then load somewhere else kind of jobs might benefit from this feature, but I'm not sure the convenience is worth the risk. Rationale: delegate the routine maintenance tasks to a low privilege role, which can't do harm (apart from some performance degradation) --- hence the no exclusive locking operations requirement. This makes sense for the reindex/refresh cases, though no harm might be over-stating it. Well it's performance degradation vs DoS due to massive (exclusive) locking :S At least restricting it to one backend (connection_limit=1) allows quite some rate limit. * IMPERSONATE --- Ability to do SET AUTHORIZATION TO some_role; and RESET AUTHORIZATION This might be further refined to provide a way to say This role is authorized to impersonate role1 but no other Rationale: for use by connection poolers (esp. pgBouncer), where the role used for connection would only have the LOGIN and IMPERSONATE privileges. The remaining operations would be authorized against the supplanted role (i.e. ability to create tables/indexes or views, perform DML and/or DDL, etc) AFAIK, a superuser role is needed for this purpose currently. No.. You can have 'no-inherit' roles which you can use for exactly this purpose. The initial login role can have no rights on the database, except to SET ROLE to other roles which have been granted to it. Hmm the current documentation states that: The specified role_name must be a role that the current session user is a member of. I can see use cases where making the login role a member of every other used role quickly becomes a burden, and that's the main driver for this feature (I'm thinking about multiple app servers running several applications each, minimum two roles per application) You should never have your pgBouncer or other pooling connection logging in as a superuser. At least the default pgBouncer config explicitly says (albeit for 8.2)
Re: Suppressing elog.c context messages (was Re: [HACKERS] Wait free LW_SHARED acquisition)
On 2014-12-22 10:35:35 +0530, Amit Kapila wrote: On Fri, Dec 19, 2014 at 9:36 PM, Andres Freund and...@2ndquadrant.com wrote: Hi, When debugging lwlock issues I found PRINT_LWDEBUG/LOG_LWDEBUG rather painful to use because of the amount of elog contexts/statements emitted. Given the number of lwlock acquirations that's just not doable. To solve that during development I've solved that by basically replacing: if (Trace_lwlocks) elog(LOG, %s(%s %d): %s, where, name, index, msg); with something like if (Trace_lwlocks) { ErrorContextCallback *old_error_context_stack; ... old_error_context_stack = error_context_stack; error_context_stack = NULL; ereport(LOG, (errhidestmt(true), errmsg(%s(%s %d): %s, where, T_NAME(lock), T_ID(lock), msg))); I think it'd generally be useful to have something like errhidecontext() akin to errhidestatement() to avoid things like the above. Under this proposal, do you want to suppress the context/statement unconditionally or via some hook/variable, because it might be useful to print the contexts/statements in certain cases where there is complex application and we don't know which part of application code causes problem. I'm proposing to do model it after errhidestatement(). I.e. add errhidecontext(). I've attached what I was tinkering with when I wrote this message. The usecases wher eI see this as being useful is high volume debug logging, not normal messages... I think usecase is valid, it is really difficult to dig such a log especially when statement size is big. Right, that was what triggered to look me into it. I'd cases where the same context was printed thousands of times. Also I think even with above, the number of logs generated are high for any statement which could still make debugging difficult, do you think it would be helpful if PRINT_LWDEBUG and LOG_LWDEBUG are used with separate defines (LOCK_DEBUG and LOCK_BLOCK_DEBUG) as in certain cases we might want to print info about locks which are acquired after waiting or in other words that gets blocked. Hm, that seems like a separate thing. Personally I don't find it interesting enough to write a patch for it, although I could see it being interesting for somebody. If you're looking at that level it's easy enough to just add a early return in either routine... Greetings, Andres Freund From 5e6af912377a1b43ea0168951238cb6a5e0b233e Mon Sep 17 00:00:00 2001 From: Andres Freund and...@anarazel.de Date: Sun, 21 Dec 2014 15:45:55 +0100 Subject: [PATCH 1/4] Add capability to suppress CONTEXT: messages to elog machinery. Hiding context messages usually is not a good idea - except for rather verbose debugging/development utensils like LOG_DEBUG. There the amount of repeated context messages just bloat the log without adding information. --- src/backend/utils/error/elog.c | 24 ++-- src/include/utils/elog.h | 2 ++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index 2316464..8f04b19 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -1081,6 +1081,25 @@ errhidestmt(bool hide_stmt) return 0; /* return value does not matter */ } +/* + * errhidestmt --- optionally suppress CONTEXT: field of log entry + * + * This should only be used for verbose debugging messages where the repeated + * inclusion of CONTEXT: bloats the log volume too much. + */ +int +errhidecontext(bool hide_ctx) +{ + ErrorData *edata = errordata[errordata_stack_depth]; + + /* we don't bother incrementing recursion_depth */ + CHECK_STACK_DEPTH(); + + edata-hide_ctx = hide_ctx; + + return 0; /* return value does not matter */ +} + /* * errfunction --- add reporting function name to the current error @@ -2724,7 +2743,8 @@ write_csvlog(ErrorData *edata) appendStringInfoChar(buf, ','); /* errcontext */ - appendCSVLiteral(buf, edata-context); + if (!edata-hide_ctx) + appendCSVLiteral(buf, edata-context); appendStringInfoChar(buf, ','); /* user query --- only reported if not disabled by the caller */ @@ -2856,7 +2876,7 @@ send_message_to_server_log(ErrorData *edata) append_with_tabs(buf, edata-internalquery); appendStringInfoChar(buf, '\n'); } - if (edata-context) + if (edata-context !edata-hide_ctx) { log_line_prefix(buf, edata); appendStringInfoString(buf, _(CONTEXT: )); diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 87438b8..ec7ed22 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -221,6 +221,7 @@ errcontext_msg(const char *fmt,...) __attribute__((format(PG_PRINTF_ATTRIBUTE, 1, 2))); extern int errhidestmt(bool hide_stmt); +extern int errhidecontext(bool hide_ctx); extern int errfunction(const char *funcname); extern int errposition(int
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On 12/23/2014 06:06 PM, Bruce Momjian wrote: On Tue, Dec 23, 2014 at 11:34:09AM -0500, Tom Lane wrote: Stephen Frost sfr...@snowman.net writes: If that's the only consideration for this, well, that's certainly quite straight-forward to change in the other direction too. The new function suggested by Andres actually makes it really easy to get a textual list of all the role attributes which a role has from the bitmask too. We could have that regardless of the representation, if the function is defined along the lines of given a user OID, give me a text string representing the user's attributes. However, that only helps for pg_dumpall and any other clients whose requirement is exactly satisfied by a string that fits into CREATE/ALTER USER. The current formatting of psql's \du, for example, absolutely requires adding more client-side code every time we add a property; whether the catalog representation is bools or a bitmask really isn't going to change the pain level much there. I am with Tom on this --- there is more wasted space in the 'name' column pg_authid.rolname than by shoving 40 boolean values into a bitmap. Adding the complexity of a bitmap doesn't make sense here. Well, the code simplification alone might be worth the effort... and it does make adding additional attributes easier. I also apologize for the late feedback. Offtopic, what I would really _love_ to see improved is our display of object permissions: Access privileges Schema | Name | Type | Access privileges | Column privileges | Policies ++---+---+---+-- public | crypto | table | postgres=arwdDxt/postgres+| | || | =r/postgres | | That is nasty user display --- it looks like line noise. Hmm... http://www.postgresql.org/docs/9.4/static/sql-grant.html does describe the mapping from letters to permissions, but I agree that it could be easier for beginners. Any idea on how this display can be made more human friendly? (just for the sake of discussion --- I don't think I have time to do much about that, unfortunately) Cheers, / J.L. -- 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] Proposal: two new role attributes and/or capabilities?
José Luis Tallón-2 wrote On 12/23/2014 05:29 PM, Stephen Frost wrote: * José Luis Tallón ( jltallon@ ) wrote: * IMPERSONATE --- Ability to do SET AUTHORIZATION TO some_role; and RESET AUTHORIZATION This might be further refined to provide a way to say This role is authorized to impersonate role1 but no other Rationale: for use by connection poolers (esp. pgBouncer), where the role used for connection would only have the LOGIN and IMPERSONATE privileges. The remaining operations would be authorized against the supplanted role (i.e. ability to create tables/indexes or views, perform DML and/or DDL, etc) AFAIK, a superuser role is needed for this purpose currently. No.. You can have 'no-inherit' roles which you can use for exactly this purpose. The initial login role can have no rights on the database, except to SET ROLE to other roles which have been granted to it. Hmm the current documentation states that: The specified role_name must be a role that the current session user is a member of. I can see use cases where making the login role a member of every other used role quickly becomes a burden, and that's the main driver for this feature (I'm thinking about multiple app servers running several applications each, minimum two roles per application) So you want to say: GRANT IMPERSONATE TO bouncer; --covers the ALL requirement instead of GRANT victim1 TO bouncer; GRANT victim2 TO bouncer; etc... -- these would still be used to cover the limited users requirement ? Seems contrary to the principle of least privilege goal... I'd rather there be better, more user friendly, SQL-based APIs to the permissions system that would facilitate performing and reviewing grants. If something like IMPERSONATE was added I would strongly suggest a corresponding [NO]IMPERSONATE for CREATE USER so that the admin can make specific roles unimpersonable - and also make SUPERUSER roles unimpersonable by rule. David J. -- View this message in context: http://postgresql.nabble.com/Proposal-two-new-role-attributes-and-or-capabilities-tp5831859p5831868.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
José Luis Tallón-2 wrote On 12/23/2014 06:06 PM, Bruce Momjian wrote: On Tue, Dec 23, 2014 at 11:34:09AM -0500, Tom Lane wrote: Stephen Frost lt; sfrost@ gt; writes: If that's the only consideration for this, well, that's certainly quite straight-forward to change in the other direction too. The new function suggested by Andres actually makes it really easy to get a textual list of all the role attributes which a role has from the bitmask too. We could have that regardless of the representation, if the function is defined along the lines of given a user OID, give me a text string representing the user's attributes. However, that only helps for pg_dumpall and any other clients whose requirement is exactly satisfied by a string that fits into CREATE/ALTER USER. The current formatting of psql's \du, for example, absolutely requires adding more client-side code every time we add a property; whether the catalog representation is bools or a bitmask really isn't going to change the pain level much there. I am with Tom on this --- there is more wasted space in the 'name' column pg_authid.rolname than by shoving 40 boolean values into a bitmap. Adding the complexity of a bitmap doesn't make sense here. Well, the code simplification alone might be worth the effort... and it does make adding additional attributes easier. I also apologize for the late feedback. Offtopic, what I would really _love_ to see improved is our display of object permissions: Access privileges Schema | Name | Type | Access privileges | Column privileges | Policies ++---+---+---+-- public | crypto | table | postgres=arwdDxt/postgres+| | || | =r/postgres | | That is nasty user display --- it looks like line noise. Hmm... http://www.postgresql.org/docs/9.4/static/sql-grant.html does describe the mapping from letters to permissions, but I agree that it could be easier for beginners. Any idea on how this display can be made more human friendly? (just for the sake of discussion --- I don't think I have time to do much about that, unfortunately) I'm not exploring this at the moment but creating an ASCII table that looks similar to what pgAdmin outputs would be the best solution. While pgAdmin would allow for interaction on the table the presentation there is likely more user-friendly than this (not a high bar to clear...) Ideally the column headers would go vertical for narrow columns; but even using header abbreviations with a key would work but the number of columns should be constant and true indicators used to denote permission. Spatial factors (position, space/fill, size) provide stronger cues compared to trying to read a sequence of characters. David J. -- View this message in context: http://postgresql.nabble.com/Re-COMMITTERS-pgsql-Use-a-bitmask-to-represent-role-attributes-tp5831838p5831869.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
Bruce Momjian wrote: I am with Tom on this --- there is more wasted space in the 'name' column pg_authid.rolname than by shoving 40 boolean values into a bitmap. Adding the complexity of a bitmap doesn't make sense here. I also apologize for the late feedback. Okay, it seems we have a majority that does not want this patch -- at least Tom, Robert and Bruce plus-one'd the reversion. I am going to revert it, mainly because I don't want to be on the hook for fixing it later on. I'm also going to mark it Rejected in commitfest. Adam and Stephen can rework as they see fit, according to whatever acceptable design is found. -- Á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] replicating DROP commands across servers
I have pushed patches 0001 through 0004, with some revisions. Only the final part is missing. -- Á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] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Bruce Momjian (br...@momjian.us) wrote: I am with Tom on this --- there is more wasted space in the 'name' column pg_authid.rolname than by shoving 40 boolean values into a bitmap. I agree that we waste a lot of space in 'name' but I don't think there's an easy solution to that problem. This, on the other hand, seemed like a relatively minor change to an internal catalog definition. Adding the complexity of a bitmap doesn't make sense here. I also apologize for the late feedback. The complexity of the bitmap on the SQL side actually makes the code side cleaner. :/ It would be great to figure out a way to get feedback like this earlier on in the development. This patch has been floating around for quite a while, with intentional breaks for feedback taken prior to incremental improvements and documentation additions. Clearly that gets back to the discussion around the commitfest situation. Offtopic, what I would really _love_ to see improved is our display of object permissions: That's a whole different discussion and really belongs on a new thread. I'm certainly curious what ideas you have regarding how to fix this; it's not like Unix permission display is particularly elegant. Is there something you've seen that looks nicer while still conveying the necessary information in a relatively small amount of space? Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: two new role attributes and/or capabilities?
On 12/23/2014 07:01 PM, David G Johnston wrote: Hmm the current documentation states that: The specified role_name must be a role that the current session user is a member of. I can see use cases where making the login role a member of every other used role quickly becomes a burden, and that's the main driver for this feature (I'm thinking about multiple app servers running several applications each, minimum two roles per application) So you want to say: GRANT IMPERSONATE TO bouncer; --covers the ALL requirement Yes, and exclusively for this purpose. instead of GRANT victim1 TO bouncer; GRANT victim2 TO bouncer; etc... -- these would still be used to cover the limited users requirement ? Yup. Seems contrary to the principle of least privilege goal... We still wouldn't grant any CREATE DATABASE, CREATE TABLESPACE, CREATE/LOAD EXTENSION, CREATE LANGUAGE, etc (and the ability to create/use/manipulate data within the database will still be constrained by the impersonated login) I'd rather there be better, more user friendly, SQL-based APIs to the permissions system that would facilitate performing and reviewing grants. +1. All suggestions welcome. If something like IMPERSONATE was added I would strongly suggest a corresponding [NO]IMPERSONATE for CREATE USER so that the admin can make specific roles unimpersonable Indeed, I had thought about this too. - and also make SUPERUSER roles unimpersonable by rule. Yes, of course. Otherwise, the distinction would not have any sense. Thanks, J.L. -- 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] Proposal: two new role attributes and/or capabilities?
* José Luis Tallón (jltal...@adv-solutions.net) wrote: On 12/23/2014 05:29 PM, Stephen Frost wrote: The capabilities would be: * MAINTENANCE --- Ability to run VACUUM [ANALYZE | FREEZE] (but not VACUUM FULL), ANALYZE (including SET LOCAL statistics_target TO 1), There's likely to be discussion about these from the perspective that you really shouldn't need to run them all that much. Why isn't autovacuum able to handle this? For some (arguably, ill-devised) use cases of INSERT - SELECT aggregate - DELETE (third party, closed-source app, massive insert rate) at the very least, autovacuum can't possibly cope with the change rate in some tables, given that there are quite many other interactive queries running. Manually performing VACUUM / VACUUM ANALYZE on the (few) affected tables every 12h or so fixes the performance problem for the particular queries without impacting the other users too much --- the tables and indexes in question have been moved to a separate tablespace/disk volume of their own. Autovacuum can certainly run vacuum/analyze on a few tables every 12 hours, so I'm not really following where you see autovacuum being unable to cope. I agree that there *are* such cases, but getting more information about those cases and exactly what solution *does* work would really help us improve autovacuum to address those use-cases. In short, this addresses situations where some tables have a much higher update rate than the rest of the database so that performance degrades with time --- the application became unusable after about 6 days' worth of updates until the manual vacuums were setup This really looks like a configuration issue with autovacuum.. Perhaps you need to make it more aggressive than the default and have it run more threads? Have you turned the autovacuum logging up all the way? Is autovacuum giving up due to locking? REINDEX CONCURRENTLY (but not the blocking, regular, one) REFRESH MATERIALIZED VIEW CONCURRENTLY (but not the blocking one) These are interesting, but would these make sense at the role level? Both of these commands explicitly take specific relations to operate against, after all. Yup. Let's imagine a cron job invoking psql in order to perform maintenance routine. If they make sense at a relation level then they should be relation-level GRANT'd permissions, not role-level attributes. The particular command(s) can be generated on-the-fly by querying the catalog and then send them in one go to be run sequentially by the one backend as a crude form of rate limiting/quality-of-service of sorts (renice -p or even ionice -p seems quite inadequate). This sounds like it's something that we might want an autovacuum-like background process to handle.. Some kind of auto-reindex-concurrently. There are already plans to deal with updating of materialized views, as I understand it. This automation becomes impossible to do if the object owners differ (only the owner or a superuser can perform these operations AFAICS -- there is no mention of it in the current documentation) unless the DBA makes the maintenance role a member of every other role ... which quickly becomes a problem. I agree that having a maintenance role which is a member of every other role isn't a very good solution. COPY ??? The question around this one goes back to the CREATE DIRECTORY discussion that happened this fall. I'm still hopeful that we can do *something* in this area, but I'm not sure what that's going to end up looking like. The problem with COPY is that it's either trivial to use it to become a superuser, or insanely difficult to secure sufficiently. Yes. That's the reason for the question marks :-\ Some dump to csv then load somewhere else kind of jobs might benefit from this feature, but I'm not sure the convenience is worth the risk. I've run into quite a few processes which would really benefit from this, and would even be safe to use (the processes running the COPY commands don't have any rights on the directories except through PG), but it's not clear if that use-case is sufficiently broad for the feature to be worthwhile.. At least, some feel it isn't. Can you describe your use-case more and perhaps the needle will move on that point? * IMPERSONATE --- Ability to do SET AUTHORIZATION TO some_role; and RESET AUTHORIZATION This might be further refined to provide a way to say This role is authorized to impersonate role1 but no other Rationale: for use by connection poolers (esp. pgBouncer), where the role used for connection would only have the LOGIN and IMPERSONATE privileges. The remaining operations would be authorized against the supplanted role (i.e. ability to create tables/indexes or views, perform DML and/or DDL, etc) AFAIK, a superuser role is needed for this purpose currently. No.. You can have 'no-inherit' roles which you can use for exactly this
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On Tue, Dec 23, 2014 at 01:43:47PM -0500, Stephen Frost wrote: Offtopic, what I would really _love_ to see improved is our display of object permissions: That's a whole different discussion and really belongs on a new thread. I'm certainly curious what ideas you have regarding how to fix this; it's not like Unix permission display is particularly elegant. Is there something you've seen that looks nicer while still conveying the necessary information in a relatively small amount of space? No, just hoping for something better. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: Bruce Momjian wrote: I am with Tom on this --- there is more wasted space in the 'name' column pg_authid.rolname than by shoving 40 boolean values into a bitmap. Adding the complexity of a bitmap doesn't make sense here. I also apologize for the late feedback. Okay, it seems we have a majority that does not want this patch -- at least Tom, Robert and Bruce plus-one'd the reversion. I am going to revert it, mainly because I don't want to be on the hook for fixing it later on. I'm also going to mark it Rejected in commitfest. Well, I'd be happy for fixing it, but that's not what is at issue here. If it was only about getting someone to maintain it, I don't think there'd be a discussion. Adam and Stephen can rework as they see fit, according to whatever acceptable design is found. In the end, this is simpler, and the patch to add the other role attributes which we were looking to add is probably closer to being done with this outcome anyway, since it doesn't need to be rebased on top of the bitmap work. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] bin checks taking too long.
On 12/23/2014 11:11 AM, Andrew Dunstan wrote: On 12/22/2014 10:41 PM, Peter Eisentraut wrote: On 12/22/14 7:56 PM, Andrew Dunstan wrote: Currently the buildfarm animal crake (my development instance) is running the bin check, but not any other animal. These tests still take for too long, not least because each set of tests requires a separate install. You can avoid that by using make installcheck. Not working too well: make -C pg_ctl installcheck make[1]: Entering directory `/home/bf/bfr/root/HEAD/pgsql.build/src/bin/pg_ctl' cd /home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/bin/pg_ctl TESTDIR='/home/bf/bfr/root/HEAD/pgsql.build/src/bin/pg_ctl' PATH=/home/bf/bfr/root/HEAD/inst/bin:$PATH PGPORT='65648' prove -I /home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/test/perl/ --verbose t/*.pl Use of uninitialized value $ENV{top_builddir} in concatenation (.) or string at t/001_start_stop.pl line 17. file not found: /src/test/regress/pg_regress at /home/bf/bfr/root/HEAD/pgsql.build/../pgsql/src/test/perl/TestLib.pm line 142. This was invoked like this: cd $pgsql/src/bin make NO_LOCALE=1 installcheck Note that it's a vpath build. The attached patch seems to fix this. However, running installcheck also makes very little difference to the time. These checks still take around 100 seconds on my machine, including around 60 for the script tests. That seems ridiculously long, when the whole core regression suite takes 23 seconds. cheers andrew diff --git a/src/Makefile.global.in b/src/Makefile.global.in index a77b5f1..7c39d82 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -317,7 +317,7 @@ endef ifeq ($(enable_tap_tests),yes) define prove_installcheck -cd $(srcdir) TESTDIR='$(CURDIR)' PATH=$(bindir):$$PATH PGPORT='6$(DEF_PGPORT)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl +cd $(srcdir) TESTDIR='$(CURDIR)' PATH=$(bindir):$$PATH PGPORT='6$(DEF_PGPORT)' top_builddir='$(CURDIR)/$(top_builddir)' $(PROVE) $(PG_PROVE_FLAGS) $(PROVE_FLAGS) t/*.pl endef define prove_check -- 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] Reducing lock strength of adding foreign keys
On 12/17/2014 03:41 PM, Simon Riggs wrote: All of this is just a replay of the earlier conversations about reducing lock levels. My original patch did reduce lock levels for CREATE TRIGGER, but we stepped back from doing that in 9.4 until we had feedback as to whether the whole idea of reducing lock levels was safe, which Robert, Tom and others were slightly uncertain about. Is there anything different here from work in my original patch series? As far as I know, no. -- Andreas Karlsson -- 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] Proposal: two new role attributes and/or capabilities?
On 12/23/2014 07:01 PM, David G Johnston wrote: [snip] So you want to say: GRANT IMPERSONATE TO bouncer; --covers the ALL requirement instead of GRANT victim1 TO bouncer; GRANT victim2 TO bouncer; etc... -- these would still be used to cover the limited users requirement ? |GRANT IMPERSONATE ON actual_role TO login_role| would actually get us closer to how some other databases do, now that I think of it. This could be just some syntactic sugar. Might definitively ease migrations, if nothing else. I appreciate the feedback. Thanks! / J.L.
Re: [HACKERS] Proposal: two new role attributes and/or capabilities?
* David G Johnston (david.g.johns...@gmail.com) wrote: I'd rather there be better, more user friendly, SQL-based APIs to the permissions system that would facilitate performing and reviewing grants. This would be *really* nice, I agree. I've heard tale of people writing functions that go through the catalog based on a given user and spit back everything that they have permissions to. Would be really nice if we had those kinds of functions built-in. If something like IMPERSONATE was added I would strongly suggest a corresponding [NO]IMPERSONATE for CREATE USER so that the admin can make specific roles unimpersonable - and also make SUPERUSER roles unimpersonable by rule. I agree that this would be necessary.. but strikes me as less of a complete solution than what the existing pg_auth_members approach grants you. Perhaps a better idea would be to simply make the bouncer unnecessary by having a in-PG connection pooler type of system. That's been discussed previously and shot down but it's still one of those things that's on my wish-list for PG. Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: two new role attributes and/or capabilities?
* José Luis Tallón (jltal...@adv-solutions.net) wrote: On 12/23/2014 07:01 PM, David G Johnston wrote: [snip] So you want to say: GRANT IMPERSONATE TO bouncer; --covers the ALL requirement instead of GRANT victim1 TO bouncer; GRANT victim2 TO bouncer; etc... -- these would still be used to cover the limited users requirement ? |GRANT IMPERSONATE ON actual_role TO login_role| would actually get us closer to how some other databases do, now that I think of it. This could be just some syntactic sugar. Might definitively ease migrations, if nothing else. Uh, how is this different from GRANT actual_role TO login_role, with use of noinherit..? THanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Wait free LW_SHARED acquisition - v0.10
Hi, Attached is a new version of the patchset which I intend to commit soon. Stuff changed since 0.9: * Greatly simplified locking logic - the whole concept that a lock could be spuriously acquired is gone. That cost a small bit of performance (0.5%, I thought it'd be much bigger) on x86, but is a noticeable performance *benefit* on PPC. * releaseOK (and other internal flags) are rolled into the former 'lockcount' variable which is now named state. By having it inside the same atomic reasoning about the state gets easier as there's no skew between observing the lockcount and other variables. * The number of queued waiters isn't required anymore, it's only a debugging aid (#ifdef LOCK_DEBUG) at this point. Patches: 0001: errhidecontext() patch 0002: dlist()ify lwWaitLink 0003: LW_SHARED scalability I've done a fair amount of benchmarking and on bigger system the new code seems to be a win pretty much generally. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services From 5e6af912377a1b43ea0168951238cb6a5e0b233e Mon Sep 17 00:00:00 2001 From: Andres Freund and...@anarazel.de Date: Sun, 21 Dec 2014 15:45:55 +0100 Subject: [PATCH 1/4] Add capability to suppress CONTEXT: messages to elog machinery. Hiding context messages usually is not a good idea - except for rather verbose debugging/development utensils like LOG_DEBUG. There the amount of repeated context messages just bloat the log without adding information. --- src/backend/utils/error/elog.c | 24 ++-- src/include/utils/elog.h | 2 ++ 2 files changed, 24 insertions(+), 2 deletions(-) diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index 2316464..8f04b19 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -1081,6 +1081,25 @@ errhidestmt(bool hide_stmt) return 0; /* return value does not matter */ } +/* + * errhidestmt --- optionally suppress CONTEXT: field of log entry + * + * This should only be used for verbose debugging messages where the repeated + * inclusion of CONTEXT: bloats the log volume too much. + */ +int +errhidecontext(bool hide_ctx) +{ + ErrorData *edata = errordata[errordata_stack_depth]; + + /* we don't bother incrementing recursion_depth */ + CHECK_STACK_DEPTH(); + + edata-hide_ctx = hide_ctx; + + return 0; /* return value does not matter */ +} + /* * errfunction --- add reporting function name to the current error @@ -2724,7 +2743,8 @@ write_csvlog(ErrorData *edata) appendStringInfoChar(buf, ','); /* errcontext */ - appendCSVLiteral(buf, edata-context); + if (!edata-hide_ctx) + appendCSVLiteral(buf, edata-context); appendStringInfoChar(buf, ','); /* user query --- only reported if not disabled by the caller */ @@ -2856,7 +2876,7 @@ send_message_to_server_log(ErrorData *edata) append_with_tabs(buf, edata-internalquery); appendStringInfoChar(buf, '\n'); } - if (edata-context) + if (edata-context !edata-hide_ctx) { log_line_prefix(buf, edata); appendStringInfoString(buf, _(CONTEXT: )); diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h index 87438b8..ec7ed22 100644 --- a/src/include/utils/elog.h +++ b/src/include/utils/elog.h @@ -221,6 +221,7 @@ errcontext_msg(const char *fmt,...) __attribute__((format(PG_PRINTF_ATTRIBUTE, 1, 2))); extern int errhidestmt(bool hide_stmt); +extern int errhidecontext(bool hide_ctx); extern int errfunction(const char *funcname); extern int errposition(int cursorpos); @@ -385,6 +386,7 @@ typedef struct ErrorData bool output_to_client; /* will report to client? */ bool show_funcname; /* true to force funcname inclusion */ bool hide_stmt; /* true to prevent STATEMENT: inclusion */ + bool hide_ctx; /* true to prevent CONTEXT: inclusion */ const char *filename; /* __FILE__ of ereport() call */ int lineno; /* __LINE__ of ereport() call */ const char *funcname; /* __func__ of ereport() call */ -- 2.2.0.rc0.18.ga1ad247 From 0ef51c826faa53620fc7d8d39d5df6206be729f3 Mon Sep 17 00:00:00 2001 From: Andres Freund and...@anarazel.de Date: Tue, 7 Oct 2014 15:32:34 +0200 Subject: [PATCH 2/4] Convert the PGPROC-lwWaitLink list into a dlist instead of open coding it. Besides being shorter and much easier to read it changes the logic in LWLockRelease() to release all shared lockers when waking up any. This can yield some significant performance improvements - and the fairness isn't really much worse than before, as we always allowed new shared lockers to jump the queue. --- src/backend/access/transam/twophase.c | 1 - src/backend/storage/lmgr/lwlock.c | 146 -- src/backend/storage/lmgr/proc.c | 2 - src/include/storage/lwlock.h | 5 +- src/include/storage/proc.h| 3 +- 5 files changed, 57 insertions(+), 100 deletions(-) diff --git
[HACKERS] Commit timestamp abbreviations
I noticed this when looking at the allocated shared memory structures in head: shared memory alignment 64-byte of CommitTs Ctl: 0 shared memory alignment 64-byte of CommitTs shared: 0 I thought we got rid of the idea that 'Ts' means timestamp. Was this part forgotten? -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On Tue, Dec 23, 2014 at 11:30 AM, Peter Geoghegan p...@heroku.com wrote: I tend to agree. I think we should just live with the fact that not every conceivable use case will be covered, at least initially. To be clear: I still think I should go and make the changes that will make the feature play nice with all shipped non-default B-Tree operator classes, and will make it work with partial unique indexes [1]. That isn't difficult or controversial, AFAICT, and gets us very close to satisfying every conceivable use case. [1] http://www.postgresql.org/message-id/CAM3SWZQdv7GDLwPRv7=re-gg1qjlool3vcmaricbctyk8gw...@mail.gmail.com -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Turning recovery.conf into GUCs
On 12/12/14 16:34, Alex Shulgin wrote: Alex Shulgin a...@commandprompt.com writes: Alex Shulgin a...@commandprompt.com writes: Here's an attempt to revive this patch. Here's the patch rebased against current HEAD, that is including the recently committed action_at_recovery_target option. The default for the new GUC is 'pause', as in HEAD, and pause_at_recovery_target is removed completely in favor of it. I've also taken the liberty to remove that part that errors out when finding $PGDATA/recovery.conf. Now get your rotten tomatoes ready. ;-) This was rather short-sighted, so I've restored that part. Also, rebased on current HEAD, following the rename of action_at_recovery_target to recovery_target_action. Hi, I had a quick look, the patch does not apply cleanly anymore but it's just release notes so nothing too bad. I did not do any testing yet, but I have few comments about the code: - the patch mixes functionality change with the lowercasing of the config variables, I wonder if those could be separated into 2 separate diffs - it would make review somewhat easier, but I can live with it as it is if it's too much work do separate into 2 patches - the StandbyModeRequested and StandbyMode should be lowercased like the rest of the GUCs - I am wondering if StandbyMode and hot_standby should be merged into one GUC if we are breaking backwards compatibility anyway - Could you explain the reasoning behind: + if ((*newval)[0] == 0) + xid = InvalidTransactionId; + else in check_recovery_target_xid() (and same check in check_recovery_target_timeline()), isn't the strtoul which is called later enough? - The whole CopyErrorData and memory context logic is not needed in check_recovery_target_time() as the FlushErrorState() is not called there - The new code in StartupXLOG() like + if (recovery_target_xid_string != NULL) + InitRecoveryTarget(RECOVERY_TARGET_XID); + + if (recovery_target_time_string != NULL) + InitRecoveryTarget(RECOVERY_TARGET_TIME); + + if (recovery_target_name != NULL) + InitRecoveryTarget(RECOVERY_TARGET_NAME); + + if (recovery_target_string != NULL) + InitRecoveryTarget(RECOVERY_TARGET_IMMEDIATE); would probably be better in separate function that you then call StartupXLOG() as StartupXLOG() is crazy long already so I think it's preferable to not make it worse. - I wonder if we should rename trigger_file to standby_tigger_file -- 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] INSERT ... ON CONFLICT {UPDATE | IGNORE}
* Peter Geoghegan (p...@heroku.com) wrote: On Tue, Dec 23, 2014 at 5:46 AM, Robert Haas robertmh...@gmail.com wrote: On Mon, Dec 22, 2014 at 5:04 PM, Peter Geoghegan p...@heroku.com wrote: If you're dead set on having an escape hatch, maybe we should just get over it and add a way of specifying a unique index by name. As I said, these under-served use cases are either exceedingly rare or entirely theoretical. I'm decidedly unenthusiastic about that. People don't expect CREATE INDEX CONCURRENTLY + DROP INDEX CONCURRENTLY to break their DML. I think the solution in this case would be a gateway to problems larger than the one we're trying to solve. I tend to agree. I think we should just live with the fact that not every conceivable use case will be covered, at least initially. Then, if an appreciable demand for even more flexibility emerges, we can revisit this. We already have a syntax that is significantly more flexible than the equivalent feature in any other system. Let's not lose sight of that. +1 Thanks, Stephen signature.asc Description: Digital signature
Re: [HACKERS] Proposal: two new role attributes and/or capabilities?
On 12/23/2014 07:52 PM, Stephen Frost wrote: [snip] Manually performing VACUUM / VACUUM ANALYZE on the (few) affected tables every 12h or so fixes the performance problem for the particular queries without impacting the other users too much --- the tables and indexes in question have been moved to a separate tablespace/disk volume of their own. Autovacuum can certainly run vacuum/analyze on a few tables every 12 hours, so I'm not really following where you see autovacuum being unable to cope. I agree that there *are* such cases, but getting more information about those cases and exactly what solution *does* work would really help us improve autovacuum to address those use-cases. I'll try to. I don't have direct access, and the use case is quite edgy to be fair. Plus, the configuration and hardware leaves quite a bit to be desired... ...but it's a real use case and the solution (even if only treating the symptoms) is quite straight-forward and easy. In short, this addresses situations where some tables have a much higher update rate than the rest of the database so that performance degrades with time --- the application became unusable after about 6 days' worth of updates until the manual vacuums were setup This really looks like a configuration issue with autovacuum.. Perhaps you need to make it more aggressive than the default and have it run more threads? Yes to both. Up to something which actually affected performance a bit. But basically only a few tables exhibited this behaviour among several hundreds in this particular situation. Have you turned the autovacuum logging up all the way? Is autovacuum giving up due to locking? Not one of my systems, and I don't have access to it anymore, but I don't think this was the reason. However, having some hundred million deleted rows piling every few hours quite increases the load. For the record, the (closed-source) application did issue the DELETEs on the table, so partitioning + TRUNCATE child_part was not applicable. In any case, I was aiming at making this kind of operations possible and easier --- regardless of whether they are solving the right problem or not, or whether there exists an optimal solution --- since I have seen some real life solutions that could benefit from it. I agree that routine index maintenance is a better match for this feature, though :) REINDEX CONCURRENTLY (but not the blocking, regular, one) REFRESH MATERIALIZED VIEW CONCURRENTLY (but not the blocking one) These are interesting, but would these make sense at the role level? Both of these commands explicitly take specific relations to operate against, after all. Yup. Let's imagine a cron job invoking psql in order to perform maintenance routine. If they make sense at a relation level then they should be relation-level GRANT'd permissions, not role-level attributes. Same as before. Let's imagine this coupled with REINDEX SCHEMA CONCURRENTLY ... or simply when constructing the list of tables dynamically and there is no other use for such a grant. Arguably, this isn't that much of a problem if there exists a way to easily revoke all such permissions from all objects in one go (just like recently discussed in another thread) The particular command(s) can be generated on-the-fly by querying the catalog and then send them in one go to be run sequentially by the one backend as a crude form of rate limiting/quality-of-service of sorts (renice -p or even ionice -p seems quite inadequate). This sounds like it's something that we might want an autovacuum-like background process to handle.. Some kind of auto-reindex-concurrently. There are already plans to deal with updating of materialized views, as I understand it. While I can definitively see it for materialized views (they *are* views, after all), this pattern potentially gets us adding everything but the kitchen sink inside the database. FWIW, it's only a matter of providing a mechanism for maintenance routines to use very unprivileged users to perform their duties on the whole cluster without having to explicitly grant permissions and/or include these into another, regular, role. Please keep in mind that these roles [having only LOGIN and MAINTENANCE] would NOT be able to perform any DML or DDL whatsoever, nor any queries (unless explicitly granted permission for SELECTs). [snip] Yes. That's the reason for the question marks :-\ Some dump to csv then load somewhere else kind of jobs might benefit from this feature, but I'm not sure the convenience is worth the risk. I've run into quite a few processes which would really benefit from this, and would even be safe to use (the processes running the COPY commands don't have any rights on the directories except through PG), but it's not clear if that use-case is sufficiently broad for the feature to be worthwhile.. At least, some feel it isn't. Can you describe your use-case more and perhaps the needle will move on
Re: [HACKERS] [COMMITTERS] pgsql: Use a bitmask to represent role attributes
On Tue, Dec 23, 2014 at 11:44 AM, Stephen Frost [via PostgreSQL] ml-node+s1045698n5831875...@n5.nabble.com wrote: It would be great to figure out a way to get feedback like this earlier on in the development. This patch has been floating around for quite a while, with intentional breaks for feedback taken prior to incremental improvements and documentation additions. Clearly that gets back to the discussion around the commitfest situation. There four possible situations here: Seen, agreeable Seen, not agreeable Seen, abstain Not Seen Tracking, for the committers in particular, the not seen and directly soliciting their agree/disagree/abstain opinion is really the only way to avoid this situation where Tom probably saw the subject lines but ended up filtering them out since his focus was elsewhere. However, something getting committed definitely gets his attention. FWIW my initial reaction to this idea of introducing bitmaps was why? but I didn't have anything to go on but the feeling that bitmaps are not the most obvious API in modern coding. I didn't have anything else to support that, including coding experience, so I didn't voice it and figured when no one else did that I likely was missing something. I'm not sure how an email to Tom saying: hey, this doesn't smell right to me would have been taken but changing the underlying authorization mechanisms does seem like something Tom should comment on before development gets to far along - and that input should be prompted for if not seen. That's my current feeling as strictly a monitor of these lists and observing a subset of the threads and new features that are being currently developed - take it as you will. David J. -- View this message in context: http://postgresql.nabble.com/Re-COMMITTERS-pgsql-Use-a-bitmask-to-represent-role-attributes-tp5831838p5831894.html Sent from the PostgreSQL - hackers mailing list archive at Nabble.com.
Re: [HACKERS] INSERT ... ON CONFLICT {UPDATE | IGNORE}
On Thu, Dec 18, 2014 at 9:20 AM, Jeff Janes jeff.ja...@gmail.com wrote: I've put this through an adaptation of my usual torture test, and it ran fine until wraparound shutdown. I'll poke at it more later. Could you elaborate, please? What are the details of the torture test you're performing? -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] mysql with postgres
On 23/12/14 22:36, Ravi Kiran wrote: hi all, Is postgres source code compatible with mysql database?? If it is, could someone could give me some links so that I can do that. I want to hack into the postgres source code, but as I am comfortable with mysql, I want to use the mysql database not postgres. any references would be fine. I'm wondering if you are thinking that you can use Postgres as a Mysql storage engine? While Mysql does has pluggable storage engines...Postgres is not designed to be able to be used in this way (that would be an interesting - but big and probably controversial project to undertake). So if you are more familiar with Mysql, why not hack Innodb? Regards Mark -- 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] Lockless StrategyGetBuffer() clock sweep
On 2014-12-23 16:42:41 -0500, Robert Haas wrote: I don't think I have anything to say about the substance of the patch. If fetch-and-add is faster than a spinlock cycle, then it is. And it's good to be fast. I don't think the primary advantage is that it's fast (even though it should be as fast as a single TAS on x86). It's that you can never sleep while holding the spinlock when there's no such spinlock and that everytime you transfer the cacheline from another cpu to you you'll also make progress... Will fix the other stuff. 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] Using 128-bit integers for sum, avg and statistics aggregates
On 12/22/2014 11:47 PM, Oskari Saarenmaa wrote: __int128_t and __uint128_t are GCC extensions and are not related to stdint.h. [...] These changes don't match what my autoconf does. Not a big deal I guess, but if this is merged as-is the next time someone runs autoreconf it'll write these lines differently to a different location of the file and the change will end up as a part of an unrelated commit. Thanks for the feedback. These two issues will be fixed in the next version. *** a/src/backend/utils/adt/numeric.c --- b/src/backend/utils/adt/numeric.c *** static void apply_typmod(NumericVar *var *** 402,407 --- 402,410 static int32 numericvar_to_int4(NumericVar *var); static bool numericvar_to_int8(NumericVar *var, int64 *result); static void int8_to_numericvar(int64 val, NumericVar *var); + #ifdef HAVE_INT128 + static void int16_to_numericvar(int128 val, NumericVar *var); + #endif Existing code uses int4 and int8 to refer to 32 and 64 bit integers as they're also PG datatypes, but int16 isn't one and it looks a lot like int16_t. I think it would make sense to just call it int128 internally everywhere instead of int16 which isn't used anywhere else to refer to 128 bit integers. Perhaps. I switched opinion on this several times while coding. On one side there is consistency, on the other there is the risk of confusing the different meanings of int16. I am still not sure which of these I think is the least bad. new file mode 100755 I guess src/tools/git-external-diff generated these bogus new file mode lines? I know the project policy says that context diffs should be used, but it seems to me that most people just use unified diffs these days so I'd just use git show or git format-patch -1 to generate the patches. The ones generated by git format-patch can be easily applied by reviewers using git am. At the time of submitting my patch I had not noticed the slow change from git-external-diff to regular git diffs. The change snuck up on me. The new version of the patch will be submitted in the standard git format which is what I am more used to work with. -- Andreas Karlsson -- 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] Proposal VACUUM SCHEMA
On 12/23/14, 8:54 AM, Fabrízio de Royes Mello wrote: Right now a lot of people just work around this with things like DO blocks, but as mentioned elsewhere in the thread that fails for commands that can't be in a transaction. I use dblink to solve it. :-) So... how about instead of solving this only for vacuum we create something generic? :) Possibly using Robert's background worker work? -- 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] Proposal VACUUM SCHEMA
On 12/23/14, 7:44 AM, Robert Haas wrote: On Mon, Dec 22, 2014 at 5:00 PM, Jim Nasby jim.na...@bluetreble.com wrote: I would MUCH rather that we find a way to special-case executing non-transactional commands dynamically, because VACUUM isn't the only one that suffers from this problem. Is pg_background a solution to this problem? Yes, since it allows you to do autonomous transactions. It's probably not the most efficient way to solve this, but it should work. -- 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] Proposal VACUUM SCHEMA
Em terça-feira, 23 de dezembro de 2014, Jim Nasby jim.na...@bluetreble.com escreveu: On 12/23/14, 8:54 AM, Fabrízio de Royes Mello wrote: Right now a lot of people just work around this with things like DO blocks, but as mentioned elsewhere in the thread that fails for commands that can't be in a transaction. I use dblink to solve it. :-) So... how about instead of solving this only for vacuum we create something generic? :) Possibly using Robert's background worker work? Interesting idea. But and what about the idea of improve the --table option from clients: vaccumdb and clusterdb? Regards, Fabrízio Mello -- Fabrízio de Royes Mello Consultoria/Coaching PostgreSQL Timbira: http://www.timbira.com.br Blog: http://fabriziomello.github.io Linkedin: http://br.linkedin.com/in/fabriziomello Twitter: http://twitter.com/fabriziomello Github: http://github.com/fabriziomello
Re: [HACKERS] Using 128-bit integers for sum, avg and statistics aggregates
On 12/16/2014 11:04 AM, David Rowley wrote: These are some very promising performance increases. I've done a quick pass of reading the patch. I currently don't have a system with a 128bit int type, but I'm working on that. Sorry for taking some time to get back. I have been busy before Christmas. A new version of the patch is attached. This fragment needs fixed to put braces on new lines Fixed! It also looks like your OIDs have been nabbed by some jsonb stuff. Fixed! I'm also wondering why in numeric_int16_sum() you're doing: #else return numeric_sum(fcinfo); #endif but you're not doing return int8_accum() in the #else part of int8_avg_accum() The same goes for int8_accum_inv() and int8_avg_accum_inv(), though perhaps you're doing it here because of the elog() showing the wrong function name. Although that's a pretty much shouldn't ever happen case that mightn't be worth worrying about. No strong reason. I did it for symmetry with int2_accum() and int4_accum(). Also since I don't currently have a machine with a working int128, I decided to benchmark master vs patched to see if there was any sort of performance regression due to numeric_int16_sum calling numeric_sum, but I'm a bit confused with the performance results as it seems there's quite a good increase in performance with the patch, I'd have expected there to be no change. Weird, I noticed similar results when doing my benchmarks, but given that I did not change the accumulator function other than adding an ifdef I am not totally sure if this difference is real. master tps = 1.001984 (excluding connections establishing) Without int128 tps = 1.014511 (excluding connections establishing) With int128 tps = 3.185956 (excluding connections establishing) -- Andreas Karlsson diff --git a/configure b/configure index 7594401..15f4eaf 100755 --- a/configure +++ b/configure @@ -13771,6 +13771,27 @@ _ACEOF fi +# Check if platform support gcc style 128-bit integers. +ac_fn_c_check_type $LINENO __int128_t ac_cv_type___int128_t $ac_includes_default +if test x$ac_cv_type___int128_t = xyes; then : + +cat confdefs.h _ACEOF +#define HAVE___INT128_T 1 +_ACEOF + + +fi +ac_fn_c_check_type $LINENO __uint128_t ac_cv_type___uint128_t $ac_includes_default +if test x$ac_cv_type___uint128_t = xyes; then : + +cat confdefs.h _ACEOF +#define HAVE___UINT128_T 1 +_ACEOF + + +fi + + # We also check for sig_atomic_t, which *should* be defined per ANSI # C, but is missing on some old platforms. ac_fn_c_check_type $LINENO sig_atomic_t ac_cv_type_sig_atomic_t #include signal.h diff --git a/configure.in b/configure.in index 0dc3f18..1271682 100644 --- a/configure.in +++ b/configure.in @@ -1752,6 +1752,9 @@ AC_DEFINE_UNQUOTED(MAXIMUM_ALIGNOF, $MAX_ALIGNOF, [Define as the maximum alignme AC_CHECK_TYPES([int8, uint8, int64, uint64], [], [], [#include stdio.h]) +# Check if platform support gcc style 128-bit integers. +AC_CHECK_TYPES([__int128_t, __uint128_t], [], [], []) + # We also check for sig_atomic_t, which *should* be defined per ANSI # C, but is missing on some old platforms. AC_CHECK_TYPES(sig_atomic_t, [], [], [#include signal.h]) diff --git a/src/backend/utils/adt/numeric.c b/src/backend/utils/adt/numeric.c index d841b6f..eb0fef4 100644 --- a/src/backend/utils/adt/numeric.c +++ b/src/backend/utils/adt/numeric.c @@ -402,6 +402,9 @@ static void apply_typmod(NumericVar *var, int32 typmod); static int32 numericvar_to_int4(NumericVar *var); static bool numericvar_to_int8(NumericVar *var, int64 *result); static void int8_to_numericvar(int64 val, NumericVar *var); +#ifdef HAVE_INT128 +static void int16_to_numericvar(int128 val, NumericVar *var); +#endif static double numeric_to_double_no_overflow(Numeric num); static double numericvar_to_double_no_overflow(NumericVar *var); @@ -2659,6 +2662,9 @@ numeric_float4(PG_FUNCTION_ARGS) * Actually, it's a pointer to a NumericAggState allocated in the aggregate * context. The digit buffers for the NumericVars will be there too. * + * On platforms which support 128-bit integers some aggergates instead use a + * 128-bit integer based transition datatype to speed up calculations. + * * -- */ @@ -2917,6 +2923,65 @@ numeric_accum_inv(PG_FUNCTION_ARGS) PG_RETURN_POINTER(state); } +#ifdef HAVE_INT128 +typedef struct Int16AggState +{ + bool calcSumX2; /* if true, calculate sumX2 */ + int64 N; /* count of processed numbers */ + int128 sumX; /* sum of processed numbers */ + int128 sumX2; /* sum of squares of processed numbers */ +} Int16AggState; + +/* + * Prepare state data for a 128-bit aggregate function that needs to compute + * sum, count and optionally sum of squares of the input. + */ +static Int16AggState * +makeInt16AggState(FunctionCallInfo fcinfo, bool calcSumX2) +{ + Int16AggState *state; + MemoryContext agg_context; + MemoryContext old_context; + + if (!AggCheckCallContext(fcinfo, agg_context)) +
Re: [HACKERS] Misaligned BufferDescriptors causing major performance problems on AMD
On Thu, Apr 17, 2014 at 11:23:24AM +0200, Andres Freund wrote: On 2014-04-16 19:18:02 -0400, Bruce Momjian wrote: On Thu, Feb 6, 2014 at 09:40:32AM +0100, Andres Freund wrote: On 2014-02-05 12:36:42 -0500, Robert Haas wrote: It may well be that your proposal is spot on. But I'd like to see some data-structure-by-data-structure measurements, rather than assuming that alignment must be a good thing. I am fine with just aligning BufferDescriptors properly. That has clearly shown massive improvements. I thought your previous idea of increasing BUFFERALIGN to 64 bytes had a lot to recommend it. Good. I wonder if we shouldn't move that bit of logic: if (size = BUFSIZ) newStart = BUFFERALIGN(newStart); out of ShmemAlloc() and instead have a ShmemAllocAligned() and ShmemInitStructAligned() that does it. So we can sensibly can control it per struct. But that doesn't mean it doesn't need testing. I feel the need here, to say that I never said it doesn't need testing and never thought it didn't... Where are we on this? It needs somebody with time to evaluate possible performance regressions - I personally won't have time to look into this in detail before pgcon. I am doing performance testing to try to complete this item. I used the first attached patch to report which structures are 64-byte aligned: 64-byte shared memory alignment of Control File: 0 64-byte shared memory alignment of XLOG Ctl: 1 64-byte shared memory alignment of CLOG Ctl: 0 64-byte shared memory alignment of CommitTs Ctl: 0 64-byte shared memory alignment of CommitTs shared: 0 64-byte shared memory alignment of SUBTRANS Ctl: 1 64-byte shared memory alignment of MultiXactOffset Ctl: 1 64-byte shared memory alignment of MultiXactMember Ctl: 1 64-byte shared memory alignment of Shared MultiXact State: 1 64-byte shared memory alignment of Buffer Descriptors: 1 64-byte shared memory alignment of Buffer Blocks: 1 64-byte shared memory alignment of Shared Buffer Lookup Table: 1 64-byte shared memory alignment of Buffer Strategy Status: 1 64-byte shared memory alignment of LOCK hash: 0 64-byte shared memory alignment of PROCLOCK hash: 0 64-byte shared memory alignment of Fast Path Strong Relation Lock Data: 0 64-byte shared memory alignment of PREDICATELOCKTARGET hash: 0 64-byte shared memory alignment of PREDICATELOCK hash: 0 64-byte shared memory alignment of PredXactList: 0 64-byte shared memory alignment of SERIALIZABLEXID hash: 1 64-byte shared memory alignment of RWConflictPool: 1 64-byte shared memory alignment of FinishedSerializableTransactions: 0 64-byte shared memory alignment of OldSerXid SLRU Ctl: 1 64-byte shared memory alignment of OldSerXidControlData: 1 64-byte shared memory alignment of Proc Header: 0 64-byte shared memory alignment of Proc Array: 0 64-byte shared memory alignment of Backend Status Array: 0 64-byte shared memory alignment of Backend Application Name Buffer: 0 64-byte shared memory alignment of Backend Client Host Name Buffer: 0 64-byte shared memory alignment of Backend Activity Buffer: 0 64-byte shared memory alignment of Prepared Transaction Table: 0 64-byte shared memory alignment of Background Worker Data: 0 64-byte shared memory alignment of shmInvalBuffer: 1 64-byte shared memory alignment of PMSignalState: 0 64-byte shared memory alignment of ProcSignalSlots: 0 64-byte shared memory alignment of Checkpointer Data: 0 64-byte shared memory alignment of AutoVacuum Data: 0 64-byte shared memory alignment of Wal Sender Ctl: 0 64-byte shared memory alignment of Wal Receiver Ctl: 0 64-byte shared memory alignment of BTree Vacuum State: 0 64-byte shared memory alignment of Sync Scan Locations List: 0 64-byte shared memory alignment of Async Queue Control: 0 64-byte shared memory alignment of Async Ctl: 0 Many of these are 64-byte aligned, including Buffer Descriptors. I tested pgbench with these commands: $ pgbench -i -s 95 pgbench $ pgbench -S -c 95 -j 95 -t 10 pgbench on a 16-core Xeon server and got 84k tps. I then applied another patch, attached, which causes all the structures to be non-64-byte aligned, but got the same tps number. Can someone test these patches on an AMD CPU and see if you see a difference? Thanks. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + Everyone has their own god. + diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c new file mode 100644
Re: [HACKERS] install libpq.dll in bin directory on Windows / Cygwin
On Mon, Dec 22, 2014 at 2:05 PM, Michael Paquier michael.paqu...@gmail.com wrote: Peter, could it be possible to merge this patch with its MSVC portion for simplicity? I think that it would more readable to do all the changes at the same time once and for all. Also, that's still a bug, so are we still considering a backpatch? I wouldn't mind putting some time into a patch to get that fixed.. Attached are two patches, one for MinGW/cygwin, a slightly modified version from Peter and the second implementing the same thing but for the MSVC scripts. The method for MSVC is similar to what is done in Peter's patch: roughly it checks if SO_MAJOR_VERSION is present in the Makefile of a given library, the path of Makefile is found by looking at the location of the .rc in the vcproj file (could be better but I could not come up with a better method). TBH, it would be good to be completely consistent in the way we build things on Windows, and we may as well consider a backpatch to fix this long-standing bug. The MSVC patch removes of course the hack copying libpq.dll from lib/ to bin/. I mentioned the fix for MSVC scripts as well here: http://www.postgresql.org/message-id/cab7npqqiuepzphd3mmk+q7_cqqrkk_v1fvxknymri660z4d...@mail.gmail.com Regards, -- Michael From 23c3fd58d65309e2ee6cc5ada9c3dee37110c70d Mon Sep 17 00:00:00 2001 From: Michael Paquier michael@otacoo.com Date: Tue, 23 Dec 2014 05:01:38 -0800 Subject: [PATCH 1/2] Install shared libraries in bin/ and lib/ with MinGW/cygwin Those libraries can be found by scanning for SO_MAJOR_VERSION in their respective Makefiles. --- src/Makefile.shlib| 11 ++- src/interfaces/libpq/Makefile | 9 - 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/Makefile.shlib b/src/Makefile.shlib index 674fe7e..c3af1fe 100644 --- a/src/Makefile.shlib +++ b/src/Makefile.shlib @@ -486,6 +486,9 @@ endif endif # not win32 endif # not cygwin endif # not aix +ifneq (,$(findstring $(PORTNAME),win32 cygwin)) + $(INSTALL_SHLIB) $ '$(DESTDIR)$(bindir)/$(shlib)' +endif else # no soname $(INSTALL_SHLIB) $ '$(DESTDIR)$(pkglibdir)/$(shlib)' endif @@ -494,7 +497,10 @@ endif installdirs-lib: ifdef soname $(MKDIR_P) '$(DESTDIR)$(libdir)' '$(DESTDIR)$(pkgconfigdir)' -else +ifneq (,$(findstring $(PORTNAME),win32 cygwin)) + $(MKDIR_P) '$(DESTDIR)$(bindir)' +endif +else # no soname $(MKDIR_P) '$(DESTDIR)$(pkglibdir)' endif @@ -511,6 +517,9 @@ ifdef soname '$(DESTDIR)$(libdir)/$(shlib_major)' \ '$(DESTDIR)$(libdir)/$(shlib)' \ '$(DESTDIR)$(pkgconfigdir)/lib$(NAME).pc' +ifneq (,$(findstring $(PORTNAME),win32 cygwin)) + rm -f '$(DESTDIR)$(bindir)/$(shlib)' +endif else # no soname rm -f '$(DESTDIR)$(pkglibdir)/$(shlib)' endif # no soname diff --git a/src/interfaces/libpq/Makefile b/src/interfaces/libpq/Makefile index 18d9b85..5f0042e 100644 --- a/src/interfaces/libpq/Makefile +++ b/src/interfaces/libpq/Makefile @@ -125,18 +125,12 @@ install: all installdirs install-lib $(INSTALL_DATA) $(srcdir)/libpq-int.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/pqexpbuffer.h '$(DESTDIR)$(includedir_internal)' $(INSTALL_DATA) $(srcdir)/pg_service.conf.sample '$(DESTDIR)$(datadir)/pg_service.conf.sample' -ifneq (,$(findstring $(PORTNAME), win32 cygwin)) - $(INSTALL_SHLIB) $(shlib) '$(DESTDIR)$(bindir)/$(shlib)' -endif installcheck: $(MAKE) -C test $@ installdirs: installdirs-lib $(MKDIR_P) '$(DESTDIR)$(includedir)' '$(DESTDIR)$(includedir_internal)' '$(DESTDIR)$(datadir)' -ifneq (,$(findstring $(PORTNAME), win32 cygwin)) - $(MKDIR_P) '$(DESTDIR)$(bindir)' -endif uninstall: uninstall-lib rm -f '$(DESTDIR)$(includedir)/libpq-fe.h' @@ -144,9 +138,6 @@ uninstall: uninstall-lib rm -f '$(DESTDIR)$(includedir_internal)/libpq-int.h' rm -f '$(DESTDIR)$(includedir_internal)/pqexpbuffer.h' rm -f '$(DESTDIR)$(datadir)/pg_service.conf.sample' -ifneq (,$(findstring $(PORTNAME), win32 cygwin)) - rm -f '$(DESTDIR)$(bindir)/$(shlib)' -endif clean distclean: clean-lib $(MAKE) -C test $@ -- 2.2.1 From 13ca64cb5cdae419d6ee1bbf7c7a04f6bd3d2388 Mon Sep 17 00:00:00 2001 From: Michael Paquier michael@otacoo.com Date: Tue, 23 Dec 2014 21:58:50 -0800 Subject: [PATCH 2/2] Install shared libraries in bin/ and lib/ with MSVC This is the MSVC part of the previous commit, to ensure that install is completely consistent with the multiple methods supported. --- src/tools/msvc/Install.pm | 43 +++ 1 file changed, 35 insertions(+), 8 deletions(-) diff --git a/src/tools/msvc/Install.pm b/src/tools/msvc/Install.pm index eba9aa0..616cd9d 100644 --- a/src/tools/msvc/Install.pm +++ b/src/tools/msvc/Install.pm @@ -91,7 +91,6 @@ sub Install } CopySolutionOutput($conf, $target); - lcopy($target . '/lib/libpq.dll', $target . '/bin/libpq.dll'); my $sample_files = []; my @top_dir = (src); @top_dir = (src\\bin, src\\interfaces) if ($insttype eq client); @@ -236,8 +235,9 @@
Re: hash_create API changes (was Re: [HACKERS] speedup tidbitmap patch: hash BlockNumber)
On 12/20/14, 2:13 PM, Jim Nasby wrote: On 12/20/14, 11:51 AM, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2014-12-19 22:03:55 -0600, Jim Nasby wrote: What I am thinking is not using all of those fields in their raw form to calculate the hash value. IE: something analogous to: hash_any(SharedBufHash, (rot(forkNum, 2) | dbNode) ^ relNode) 32 | blockNum) perhaps that actual code wouldn't work, but I don't see why we couldn't do something similar... am I missing something? I don't think that'd improve anything. Jenkin's hash does have a quite mixing properties, I don't believe that the above would improve the quality of the hash. I think what Jim is suggesting is to intentionally degrade the quality of the hash in order to let it be calculated a tad faster. We could do that but I doubt it would be a win, especially in systems with lots of buffers. IIRC, when we put in Jenkins hashing to replace the older homebrew hash function, it improved performance even though the hash itself was slower. Right. Now that you mention it, I vaguely recall the discussions about changing the hash function to reduce collisions. I'll still take a look at fash-hash, but it's looking like there may not be anything we can do here unless we change how we identify relation files (combining dbid, tablespace id, fork number and file id, at least for searching). If we had 64bit hash support then maybe that'd be a significant win, since you wouldn't need to hash at all. But that certainly doesn't seem to be low-hanging fruit to me... I've taken a look at a number of different hash algorithms, testing them with initially with SMHasher [1] and then with pgbench. It's worth noting that a lot of work has been done in the area of hash algo's since we last updated the hash_any algorithm in 2009 [2]. It's probably worth revisiting this every other release or so. Most of my SMHasher results are at [3]. I suspect SpookyHash is close to what we currently have, so that's what I used for a baseline. I determined that fash-hash (called superfast in results) was faster than Spooky, but not as fast as CityHash64[4] or xxhash[5]. CityHash and xxhash had similar performance, but xxhash seems to have slightly better characteristics according to SMHasher, and more important, it's in C, not C++. However, CityHash has been replaced by farmhash[7], which might be faster than xxhash. I did a quick hack at using xxhash ONLY for shared buffer lookup [6]. I've attached the full patch, as well as a version that omits the new files. Note that currently xxhash is setup in such a way that we'd get different results depending on endian-ness, so we couldn't just drop this in as-is across the board. Of course, there's also the question of if we'd want to force a REINDEX on hash indexes. pgbench results are below. Select-only testing was done first, then read-write. There were several select-only runs on both to warm shared_buffers (set to 512MB for this test, and fsync is off). Database initialized with bin/pgbench -i -F 100 -s 10. pgbench -S -T10 -c 4 -j 4 master: tps = 9556.356145 (excluding connections establishing) tps = 9897.324917 (excluding connections establishing) tps = 9287.286907 (excluding connections establishing) tps = 10210.130833 (excluding connections establishing) XXH32: tps = 32462.754437 (excluding connections establishing) tps = 33232.144511 (excluding connections establishing) tps = 33082.436760 (excluding connections establishing) tps = 33597.904532 (excluding connections establishing) pgbench -T10 -c 4 -j 4 master: tps = 2299.314145 (excluding connections establishing) tps = 2029.956749 (excluding connections establishing) tps = 2067.462362 (excluding connections establishing) XXH32: tps = 2653.919321 (excluding connections establishing) tps = 2615.764370 (excluding connections establishing) tps = 2952.144461 (excluding connections establishing) Questions: Do we want to do what we've previously done and cut/paste/modify this code into our repo? Given how rapidly hash algorithms seem to be changing I'm inclined to minimize the amount of effort required to pull a new one in... Can someone with a big-endian CPU see what the impact of XXH_FORCE_NATIVE_FORMAT 1 vs 0? If there's a notable difference we might want to do different things for on-disk vs in-memory hashes. For that matter, assuming we adopt this, do we want to replace the index hash functions too? SMHasher shows that CityHash is ~50% faster than XXHash for 262144 byte keys. Even SpookyHash is about 17% faster on that key size. So if we want to do something with hash indexes, we'd probably be better off with City or Farm hash than XXHash. [1] https://code.google.com/p/smhasher/ [2] https://github.com/postgres/postgres/commit/8205258fa675115439017b626c4932d5fefe2ea8 [3] https://github.com/decibel/hash_testing/tree/master/results [4] https://code.google.com/p/cityhash/ [5] https://code.google.com/p/xxhash/ [6]
Re: hash_create API changes (was Re: [HACKERS] speedup tidbitmap patch: hash BlockNumber)
On 12/24/14, 12:27 AM, Jim Nasby wrote: There were several select-only runs on both to warm shared_buffers (set to 512MB for this test, and fsync is off). BTW, presumably this ~380M database isn't big enough to show any problems with hash collisions, and I'm guessing you'd need way more memory than what I have on this laptop. Might be worth looking into that on a machine with a serious amount of memory. -- 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] alter user set local_preload_libraries.
Hello, sorry for the absense, Thank you for committing. On 8/29/14 4:01 PM, Peter Eisentraut wrote: On 8/28/14 9:01 AM, Kyotaro HORIGUCHI wrote: I found this issue when trying per-pg_user (role) loading of auto_analyze and some tweaking tool. It is not necessarily set by the user by own, but the function to decide whether to load some module by the session-user would be usable, at least, as for me:) I think we could just set local_preload_libraries to PGC_USERSET and document that subsequent changes won't take effect. That's the same way session_preload_libraries works. Committed this way. This doesn't prevent future fine-tuning in this area, but in the subsequent discussion, there was no clear enthusiasm for any particular direction. -- Kyotaro Horiguchi NTT Open Source Software Center -- 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] Async execution of postgres_fdw.
Hello, thank you for the comment, Ashutosh. I'll return after the New Year holidays. Very sorry not addressing them sooner but then I will have more time on this. Have a happy holidays. regards, = Hi Horiguchi-san, Here are my comments for the patches together Sanity 1. The patch applies cleanly but has trailing white spaces. [ashutosh@ubuntu coderoot]git apply /mnt/hgfs/tmp/0001-Async-exec-of-postgres_fdw.patch /mnt/hgfs/tmp/0001-Async-exec-of-postgres_fdw.patch:41: trailing whitespace. entry-conn = /mnt/hgfs/tmp/0001-Async-exec-of-postgres_fdw.patch:44: trailing whitespace. /mnt/hgfs/tmp/0001-Async-exec-of-postgres_fdw.patch:611: trailing whitespace. warning: 3 lines add whitespace errors. 2. The patches compile cleanly. 3. The regression is clean, even in contrib/postgres_fdw and contrib/file_fdw Tests --- We need tests to make sure that the logic remains intact even after further changes in this area. Couple of tests which require multiple foreign scans within the same query fetching rows more than fetch size (100) would be required. Also, some DMLs, which involve multiple foreign scans would test the sanity when UPDATE/DELETE interleave such scans. By multiple foreign scans I mean both multiple scans on a single foreign server and multiple scans spread across multiple foreign servers. Code --- Because previous conn is now replaced by conn-pgconn, the double indirection makes the code a bit ugly and prone to segfaults (conn being NULL or invalid pointer). Can we minimize such code or wrap it under a macro? We need some comments about the structure definition of PgFdwConn and its members explaining the purpose of this structure and its members. Same is the case with enum PgFdwConnState. In fact, the state diagram of a connection has become more complicated with the async connections, so it might be better to explain that state diagram at one place in the code (through comments). The definition of the enum might be a good place to do that. Otherwise, the logic of connection maintenance is spread at multiple places and is difficult to understand by looking at the code. In function GetConnection(), at line elog(DEBUG3, new postgres_fdw connection %p for server \%s\, -entry-conn, server-servername); +entry-conn-pgconn, server-servername); entry-conn-pgconn may not necessarily be a new connection and may be NULL (as the next line check it for being NULL). So, I think this line should be moved within the following if block after pgconn has been initialised with the new connection. + if (entry-conn-pgconn == NULL) + { + entry-conn-pgconn = connect_pg_server(server, user); + entry-conn-nscans = 0; + entry-conn-async_state = PGFDW_CONN_IDLE; + entry-conn-async_scan = NULL; + } The if condition if (entry-conn == NULL) in GetConnection(), used to track whether there is a PGConn active for the given entry, now it tracks whether it has PgFdwConn for the same. Please see more comments inline. On Mon, Dec 15, 2014 at 2:39 PM, Kyotaro HORIGUCHI horiguchi.kyot...@lab.ntt.co.jp wrote: * Outline of this patch From some consideration after the previous discussion and comments from others, I judged the original (WIP) patch was overdone as the first step. So I reduced the patch to minimal function. The new patch does the following, - Wrapping PGconn by PgFdwConn in order to handle multiple scans on one connection. - The core async logic was added in fetch_more_data(). It might help if you can explain this logic in this mail as well as in code (as per my comment above). - Invoking remote commands asynchronously in ExecInitForeignScan. - Canceling async invocation if any other foreign scans, modifies, deletes use the same connection. Cancellation is done by immediately fetching the return of already-invoked acync command. * Where this patch will be effective. With upcoming inheritance-partition feature, this patch enables stating and running foreign scans asynchronously. It will be more effective for longer TAT or remote startup times, and larger number of foreign servers. No negative performance effect on other situations. AFAIU, this logic sends only the first query in asynchronous manner not all of them. Is that right? If yes, I think it's a sever limitation of the feature. For a query involving multiple foreign scans, only the first one will be done in async fashion and not the rest. Sorry, if my understanding is wrong. I think, we need some data which shows the speed up by this patch. You may construct a case, where a single query involved multiple foreign scans, and we can check what is the speed up obtained against the number of foreign scans. * Concerns about this patch. - This breaks the assumption that scan starts at ExecForeignScan, not