Re: [HACKERS] PATCH: Spinlock Documentation
David Fetter da...@fetter.org writes: One issue with this patch is that it is not localized. If someone goes and changes the S_LOCK implementation for one of the platforms below, or adds a new platform, etc., without changing this comment too, this comment becomes confusingly obsolete. Indeed. Moreover, this header comment is supposed to be an overview and specification of the macros that need to be provided. I think it's an actively bad idea to clutter it with platform-by-platform details; that will create a can't see the forest for the trees problem. If we need more info here, I think a comment block before each section of the file would make more sense. But the patch as provided seems like it would just be redundant if it were refactored in that form. What would possibly be useful that's not there now is a paragraph or two describing the overall layout of the file (eg gcc then non gcc, or whatever can be said at more or less that level of detail). But please don't stick that into the middle of the specification part. 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] Upper-case error in docs regarding PQmakeEmptyPGresult
On Mon, Apr 6, 2015 at 11:40 AM, Michael Paquier michael.paqu...@gmail.com wrote: Hi all, I just spotted that PQmakeEmptyPGresult is named PQmakeEmptyPGResult in one place (not Result, but result) in the docs. A patch is attached. Pushed. Thanks! Regards, -- Fujii Masao -- 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_rewind and log messages
On Mon, Apr 6, 2015 at 12:57 PM, Fujii Masao masao.fu...@gmail.com wrote:
(1)
It outputs an error message to stdout not stderr.
(2)
The tool name should be added at the head of log message as follows,
but not in pg_rewind.
pg_basebackup: no target directory specified
Agreed. That's inconsistent.
(3)
if (datadir_source == NULL connstr_source == NULL)
{
pg_fatal(no source specified (--source-pgdata or
--source-server)\n);
pg_fatal(Try \%s --help\ for more information.\n, progname);
exit(1);
Since the first call of pg_fatal exits with 1, the subsequent pg_fatal and
exit
will never be called.
True.
(4)
ISTM that set_pglocale_pgservice() needs to be called, but not in pg_rewind.
True. I think that we should consider a call to get_restricted_token()
as well now that I look at it.
(5)
printf() is used to output an error in some files, e.g., timeline.c and
parsexlog.c. These printf() should be replaced with pg_log or pg_fatal?
On top of that, datapagemap.c has a debug message as well using
printf, as well as filemap.c in print_filemap().
I guess that you are working on a patch? If not, you are looking for one?
Regards,
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Upper-case error in docs regarding PQmakeEmptyPGresult
Hi all, I just spotted that PQmakeEmptyPGresult is named PQmakeEmptyPGResult in one place (not Result, but result) in the docs. A patch is attached. Regards, -- Michael diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index c08e6b6..b5533cd 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -5793,7 +5793,7 @@ int PQfireResultCreateEvents(PGconn *conn, PGresult *res); para The main reason that this function is separate from - functionPQmakeEmptyPGResult/function is that it is often appropriate + functionPQmakeEmptyPGresult/function is that it is often appropriate to create a structnamePGresult/structname and fill it with data before invoking the event procedures. /para -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Re: [HACKERS] GSoC 2015 proposal: Improve the performance of “ALTER TABLE .. SET LOGGED / UNLOGGED” statement
On Thu, Apr 2, 2015 at 3:24 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Mar 25, 2015 at 9:46 PM, Fabrízio de Royes Mello fabriziome...@gmail.com wrote: http://www.postgresql.org/message-id/ca+tgmozm+-0r7h0edpzzjbokvvq+gavkchmno4fypveccw-...@mail.gmail.com I like the idea of the feature a lot, but the proposal to which you refer here mentions some problems, which I'm curious how you think you might solve. (I don't have any good ideas myself, beyond what I mentioned there.) How the cleanup will happens? 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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
Fabrízio de Royes Mello wrote: Ok guys. The attached patch refactor the reloptions adding a new field lockmode in relopt_gen struct and a new method to determine the required lock level from an option list. We need determine the appropriate lock level for each reloption: I don't think AccessShareLock is appropriate for any option change. You should be using a lock level that's self-conflicting, as a minimum requirement, to avoid two processes changing the value concurrently. (I would probably go as far as ensuring that the lock level specified in the table DoLockModesConflict() with itself in an Assert somewhere.) -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] pg_rewind and log messages
Hi,
I found that pg_rewind has several problems about its log messages.
(1)
It outputs an error message to stdout not stderr.
(2)
The tool name should be added at the head of log message as follows,
but not in pg_rewind.
pg_basebackup: no target directory specified
(3)
if (datadir_source == NULL connstr_source == NULL)
{
pg_fatal(no source specified (--source-pgdata or --source-server)\n);
pg_fatal(Try \%s --help\ for more information.\n, progname);
exit(1);
Since the first call of pg_fatal exits with 1, the subsequent pg_fatal and exit
will never be called.
(4)
ISTM that set_pglocale_pgservice() needs to be called, but not in pg_rewind.
(5)
printf() is used to output an error in some files, e.g., timeline.c and
parsexlog.c. These printf() should be replaced with pg_log or pg_fatal?
Regards,
--
Fujii Masao
--
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: Reducing lock strength of trigger and foreign key DDL
On 04/05/2015 05:56 PM, Simon Riggs wrote: Committed. We move forwards, slowly but surely. Thanks for the patch. Thanks to you and the reviewers for helping me out with this patch! -- 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] PATCH: Reducing lock strength of trigger and foreign key DDL
On 5 April 2015 at 19:19, Michael Paquier michael.paqu...@gmail.com wrote: Cool! Thanks for showing up. Visibility Activity. How is REINDEX CONCURRENTLY doing? -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Spinlock Documentation
Hello, Thank you very much for your feedback! I will work on the changes as soon as I can. Respectfully, Artem Luzyanin On Sunday, April 5, 2015 5:45 PM, Tom Lane t...@sss.pgh.pa.us wrote: David Fetter da...@fetter.org writes: One issue with this patch is that it is not localized. If someone goes and changes the S_LOCK implementation for one of the platforms below, or adds a new platform, etc., without changing this comment too, this comment becomes confusingly obsolete. Indeed. Moreover, this header comment is supposed to be an overview and specification of the macros that need to be provided. I think it's an actively bad idea to clutter it with platform-by-platform details; that will create a can't see the forest for the trees problem. If we need more info here, I think a comment block before each section of the file would make more sense. But the patch as provided seems like it would just be redundant if it were refactored in that form. What would possibly be useful that's not there now is a paragraph or two describing the overall layout of the file (eg gcc then non gcc, or whatever can be said at more or less that level of detail). But please don't stick that into the middle of the specification part. regards, tom lane
Re: [HACKERS] PATCH: Reducing lock strength of trigger and foreign key DDL
On Mon, Apr 6, 2015 at 12:56 AM, Simon Riggs si...@2ndquadrant.com wrote: On 7 February 2015 at 20:05, Andreas Karlsson andr...@proxel.se wrote: On 01/30/2015 07:48 AM, Michael Paquier wrote: Looking at the latest patch, it seems that in AlterTableGetLockLevel@tablecmds.c we ought to put AT_ReAddConstraint, AT_AddConstraintRecurse and AT_ProcessedConstraint under the same banner as AT_AddConstraint. Thoughts? A new version of the patch is attached which treats them as the same for locking. I think it is correct and improves readability to do so. Committed. We move forwards, slowly but surely. Thanks for the patch. Cool! Thanks for showing up. -- 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] Doubt about AccessExclusiveLock in ALTER TABLE .. SET ( .. );
On Wed, Apr 1, 2015 at 1:45 AM, Noah Misch n...@leadboat.com wrote:
On Tue, Mar 31, 2015 at 01:17:03PM -0400, Robert Haas wrote:
On Tue, Mar 31, 2015 at 9:11 AM, Fabrízio de Royes Mello
fabriziome...@gmail.com wrote:
Attached a very WIP patch to reduce lock level when setting autovacuum
reloptions in ALTER TABLE .. SET ( .. ) statement.
I think the first thing we need to here is analyze all of the options
and determine what the appropriate lock level is for each, and why.
Agreed. Fabrízio, see this message for the discussion that led to the
code
comment you found (search for relopt_gen):
http://www.postgresql.org/message-id/20140321034556.ga3927...@tornado.leadboat.com
Ok guys. The attached patch refactor the reloptions adding a new field
lockmode in relopt_gen struct and a new method to determine the
required lock level from an option list.
We need determine the appropriate lock level for each reloption:
- boolRelopts:
* autovacuum_enabled (AccessShareLock)
* user_catalog_table (AccessExclusiveLock)
* fastupdate (AccessExclusiveLock)
* security_barrier (AccessExclusiveLock)
- intRelOpts:
* fillfactor (heap) (AccessExclusiveLock)
* fillfactor (btree) (AccessExclusiveLock)
* fillfactor (gist) (AccessExclusiveLock)
* fillfactor (spgist) (AccessExclusiveLock)
* autovacuum_vacuum_threshold (AccessShareLock)
* autovacuum_analyze_threshold (AccessShareLock)
* autovacuum_vacuum_cost_delay (AccessShareLock)
* autovacuum_vacuum_cost_limit (AccessShareLock)
* autovacuum_freeze_min_age (AccessShareLock)
* autovacuum_multixact_freeze_min_age (AccessShareLock)
* autovacuum_freeze_max_age (AccessShareLock)
* autovacuum_multixact_freeze_max_age (AccessShareLock)
* autovacuum_freeze_table_age (AccessShareLock)
* autovacuum_multixact_freeze_table_age (AccessShareLock)
* log_autovacuum_min_duration (AccessShareLock)
* pages_per_range (AccessExclusiveLock)
* gin_pending_list_limit (AccessExclusiveLock)
- realRelOpts:
* autovacuum_vacuum_scale_factor (AccessShareLock)
* autovacuum_analyze_scale_factor (AccessShareLock)
* seq_page_cost (AccessExclusiveLock)
* random_page_cost (AccessExclusiveLock)
* n_distinct (AccessExclusiveLock)
* n_distinct_inherited (AccessExclusiveLock)
- stringRelOpts:
* buffering (AccessExclusiveLock)
* check_option (AccessExclusiveLock)
In the above list I just change lock level from AccessExclusiveLock to
AccessShareLock to all autovacuum related reloptions because it was the
motivation of this patch.
I need some help to define the others.
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
diff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 8176b6a..f83ce2f 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -57,7 +57,8 @@ static relopt_bool boolRelOpts[] =
{
autovacuum_enabled,
Enables autovacuum in this relation,
- RELOPT_KIND_HEAP | RELOPT_KIND_TOAST
+ RELOPT_KIND_HEAP | RELOPT_KIND_TOAST,
+ AccessExclusiveLock
},
true
},
@@ -65,7 +66,8 @@ static relopt_bool boolRelOpts[] =
{
user_catalog_table,
Declare a table as an additional catalog table, e.g. for the purpose of logical replication,
- RELOPT_KIND_HEAP
+ RELOPT_KIND_HEAP,
+ AccessExclusiveLock
},
false
},
@@ -73,7 +75,8 @@ static relopt_bool boolRelOpts[] =
{
fastupdate,
Enables \fast update\ feature for this GIN index,
- RELOPT_KIND_GIN
+ RELOPT_KIND_GIN,
+ AccessExclusiveLock
},
true
},
@@ -81,7 +84,8 @@ static relopt_bool boolRelOpts[] =
{
security_barrier,
View acts as a row security barrier,
- RELOPT_KIND_VIEW
+ RELOPT_KIND_VIEW,
+ AccessExclusiveLock
},
false
},
@@ -95,7 +99,8 @@ static relopt_int intRelOpts[] =
{
fillfactor,
Packs table pages only to this percentage,
- RELOPT_KIND_HEAP
+ RELOPT_KIND_HEAP,
+ AccessExclusiveLock
},
HEAP_DEFAULT_FILLFACTOR, HEAP_MIN_FILLFACTOR, 100
},
@@ -103,7 +108,8 @@ static relopt_int intRelOpts[] =
{
fillfactor,
Packs btree index pages only to this percentage,
- RELOPT_KIND_BTREE
+ RELOPT_KIND_BTREE,
+ AccessExclusiveLock
},
BTREE_DEFAULT_FILLFACTOR, BTREE_MIN_FILLFACTOR, 100
},
@@ -111,7 +117,8 @@ static relopt_int intRelOpts[] =
{
fillfactor,
Packs hash index pages only to this percentage,
- RELOPT_KIND_HASH
+ RELOPT_KIND_HASH,
+ AccessExclusiveLock
},
HASH_DEFAULT_FILLFACTOR, HASH_MIN_FILLFACTOR, 100
},
@@ -119,7 +126,8 @@ static relopt_int intRelOpts[] =
{
fillfactor,
Packs gist index pages only to this percentage,
- RELOPT_KIND_GIST
+ RELOPT_KIND_GIST,
+ AccessExclusiveLock
[HACKERS] PATCH: Spinlock Documentation
Hello, I am new to PostgreSQLcommunity, but I would like to become a contributer eventually. I have readthrough your Submitting Patch guide and decided to follow Start with submitting a patch that is small anduncontroversial to help them understand you, and to get you familiar with theoverall process suggestion. I am interested inplatform-specific spinlock implementation, so I looked at s_lock.h file for possibleimprovement. Since it took me some time to find possible areas of improvement,I would like to submit a small patch that would facilitate the process forfuture contributors (including myself). Since this is my first e-mail, pleaselet me know if I should have done something differently in order to submit apatch for the community. Project name: Spinlock Documentation Uniquely identifiable file name: s_lock.h What the patch does: The patch implements addition to documentation in thementioned above file. This addition outlines the current platform-specificimplementations for an easy road map to what else could be done. Whether the patch is for discussion or forapplication: This patch is for application. Which branch the patch is against: This patch is against master branch. Whether it compiles and tests successfully: The changes allow for successful compilation andtesting. Whether it contains any platform-specificitems and if so, has it been tested on other platforms: This patch doesn’t have any platform-specific items. Confirm that the patch includes regression tests to check the newfeature actually works as described. Since this is documentation improvement, regressiontests are not needed. Include documentation on how to use the newfeature, including examples: Since it’s documentation improvement, nodocumentation is needed for documentation. Describe the effect your patch has onperformance, if any: No effect on performance. Unless we are talking aboutdeveloper’s performance. Try to include a few lines about why youchose to do things particular ways: I have decided to include the mentioned documentationto outline the areas that need improvement. Any developer, looking forplatform-specific code improvement implementation can now easily find theneeded area. Thankyou for your time and help, ArtemLuzyanin spinlock-docs.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: Reducing lock strength of trigger and foreign key DDL
On 7 February 2015 at 20:05, Andreas Karlsson andr...@proxel.se wrote: On 01/30/2015 07:48 AM, Michael Paquier wrote: Looking at the latest patch, it seems that in AlterTableGetLockLevel@tablecmds.c we ought to put AT_ReAddConstraint, AT_AddConstraintRecurse and AT_ProcessedConstraint under the same banner as AT_AddConstraint. Thoughts? A new version of the patch is attached which treats them as the same for locking. I think it is correct and improves readability to do so. Committed. We move forwards, slowly but surely. Thanks for the patch. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, RemoteDBA, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Column mis-spelling hints vs case folding
A newbie error that we see *constantly* is misunderstanding identifier case-folding rules. ISTM that commit e529cd4ff missed a chance to help with that. You do get a hint for this: regression=# create table t1 (foo int, Bar int); CREATE TABLE regression=# select bar from t1; ERROR: column bar does not exist LINE 1: select bar from t1; ^ HINT: Perhaps you meant to reference the column t1.Bar. but apparently it's just treating that as a vanilla one-mistyped-character error, because there's no hint for this: regression=# create table t2 (foo int, BAR int); CREATE TABLE regression=# select BAR from t2; ERROR: column bar does not exist LINE 1: select BAR from t2; ^ I think this hint might be a lot more useful if its comparison mechanism were case-insensitive. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Bringing text abbreviation debug output under the control of trace_sort
Attached patch makes trace_sort control abbreviation debug output for
the text opclass, which makes it consistent with the numeric opclass.
This seems better than relying on someone going to the trouble of
building Postgres themselves to debug cases where abbreviation is the
wrong thing, which we're not 100% sure will not occur. It also allows
wider analysis of where abbreviation helps the most and the least in
production, which is surely a good thing.
I have added this to the next commitfest, but I suggest that it be
quickly committed to the master branch as a maintenance commit.
--
Peter Geoghegan
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index d874a1a..18cbdd0 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -34,9 +34,6 @@
#include utils/pg_locale.h
#include utils/sortsupport.h
-#ifdef DEBUG_ABBREV_KEYS
-#define DEBUG_elog_output DEBUG1
-#endif
/* GUC variable */
int bytea_output = BYTEA_OUTPUT_HEX;
@@ -2149,11 +2146,13 @@ bttext_abbrev_abort(int memtupcount, SortSupport ssup)
* time there are differences within full key strings not captured in
* abbreviations.
*/
-#ifdef DEBUG_ABBREV_KEYS
+#ifdef TRACE_SORT
+ if (trace_sort)
{
double norm_abbrev_card = abbrev_distinct / (double) memtupcount;
- elog(DEBUG_elog_output, abbrev_distinct after %d: %f (key_distinct: %f, norm_abbrev_card: %f, prop_card: %f),
+ elog(LOG, bttext_abbrev: abbrev_distinct after %d: %f
+ (key_distinct: %f, norm_abbrev_card: %f, prop_card: %f),
memtupcount, abbrev_distinct, key_distinct, norm_abbrev_card,
tss-prop_card);
}
@@ -2219,11 +2218,11 @@ bttext_abbrev_abort(int memtupcount, SortSupport ssup)
* of moderately high to high abbreviated cardinality. There is little to
* lose but much to gain, which our strategy reflects.
*/
-#ifdef DEBUG_ABBREV_KEYS
- elog(DEBUG_elog_output, would have aborted abbreviation due to worst-case at %d. abbrev_distinct: %f, key_distinct: %f, prop_card: %f,
- memtupcount, abbrev_distinct, key_distinct, tss-prop_card);
- /* Actually abort only when debugging is disabled */
- return false;
+#ifdef TRACE_SORT
+ if (trace_sort)
+ elog(LOG, bttext_abbrev: aborted abbreviation at %d
+ (abbrev_distinct: %f, key_distinct: %f, prop_card: %f),
+ memtupcount, abbrev_distinct, key_distinct, tss-prop_card);
#endif
return true;
--
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: Spinlock Documentation
On Sun, Apr 05, 2015 at 06:50:59PM +, Artem Luzyanin wrote: Hello, I am new to PostgreSQLcommunity, but I would like to become a contributer eventually. I have readthrough your Submitting Patch guide and decided to follow Start with submitting a patch that is small anduncontroversial to help them understand you, and to get you familiar with theoverall process suggestion. I am interested inplatform-specific spinlock implementation, so I looked at s_lock.h file for possibleimprovement. Since it took me some time to find possible areas of improvement,I would like to submit a small patch that would facilitate the process forfuture contributors (including myself). Since this is my first e-mail, pleaselet me know if I should have done something differently in order to submit apatch for the community. One issue with this patch is that it is not localized. If someone goes and changes the S_LOCK implementation for one of the platforms below, or adds a new platform, etc., without changing this comment too, this comment becomes confusingly obsolete. How do you plan to address this issue? Cheers, David. -- David Fetter da...@fetter.org http://fetter.org/ Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter Skype: davidfetter XMPP: david.fet...@gmail.com Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
