Re: [HACKERS] spinlocks on HP-UX
On Tue, Oct 18, 2011 at 10:04 AM, Robert Haas robertmh...@gmail.com wrote: Hmm, so you added the non-locked test in TAS()? Did you try adding it just to TAS_SPIN()? On Itanium, I found that it was slightly better to do it only in TAS_SPIN() - i.e. in the contended case. Would it be a good change for S_LOCK() to use TAS_SPIN() as well ? Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Online base backup from the hot-standby
+ /* + * The backend writes WAL of FPW at checkpoint. However, The backend do + * not need to write WAL of FPW at checkpoint shutdown because it + * performs when startup finishes. + */ + XLogReportParameters(REPORT_ON_BACKEND); I'm still unclear why that WAL doesn't need to be written at shutdown checkpoint. Anyway, the first sentence in the above comments is not right. Not a backend but a bgwriter writes that WAL at checkpoint. The second also seems not to be right. It implies that a shutdown checkpoint is performed only at end of startup. But it may be done when smart or fast shutdown is requested. Okay. I change to the following messages. /* * The bgwriter writes WAL of FPW at checkpoint. But does not at shutdown. * Because XLogReportParameters() is always called at the end of startup * process, it does not need to be called at shutdown. */ In addition, I change macro name. REPORT_ON_BACKEND - REPORT_ON_BGWRITER Regards. Jun Ishizuka NTT Software Corporation TEL:045-317-7018 E-Mail: ishizuka@po.ntts.co.jp -- 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 or strange behaviour of update on primary key
Hi there, I could workaround the behavior with deferred constraint, and it's ok, but as I show, I have different behavior for constraint with the same definition in two rdbms and Postgresql depends on the physical order of row (with the same definition of constraint NOT DEFERRABLE INITIALLY IMMEDIATE) , or better Postgresql seems to check for every row, even if the command is one (I am doing one update on all of rows) , right? . Moreover , in documentation the definition says that a not deferrable constraints will check after every command , not after every row of the command: http://www.postgresql.org/docs/9.1/static/sql-createtable.html DEFERRABLE NOT DEFERRABLE This controls whether the constraint can be deferred.* A constraint that is not deferrable will be checked immediately after every command*. Checking of constraints that are deferrable can be postponed until the end of the transaction (using the SET CONSTRAINTShttp://www.postgresql.org/docs/9.0/static/sql-set-constraints.html command). NOT DEFERRABLE is the default. Currently, only UNIQUE, PRIMARY KEY, EXCLUDE, and REFERENCES (foreign key) constraints accept this clause. NOT NULL and CHECK constraints are not deferrable. --- If this is historical buggy behavior for performance , I think we have to change the definition of NOT DEFERRABLE in documentation, because Postgresql is not checking at end of a dml, but for every row modified by the command or there is something needs a patch. Regards, Mat 2011/10/18 Robert Haas robertmh...@gmail.com On Mon, Oct 17, 2011 at 7:30 PM, desmodemone desmodem...@gmail.com wrote: Seems an Oracle bug not Postgresql one! I don't think it's a bug for it to work. It'd probably work in PostgreSQL too, if you inserted (2) first and then (1). It's just that, as Tom says, if you want it to be certain to work (rather than depending on the order in which the rows are inserted), you need the checks to be deferred. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] WIP: Collecting statistics on CSV file data
New API AnalyzeForeignTable I didn't look at the patch, but I'm using CSV foreign tables with named pipes to get near-realtime KPI calculated by postgresql. Of course, pipes can be read just once, so I wouldn't want an automatic analyze of foreign tables... -- 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) regression diffs on collate.linux.utf8 test
On Sun, 2011-10-16 at 16:00 -0400, Tom Lane wrote: Jeff Davis pg...@j-davis.com writes: On master, I see a minor test error (at least on my machine) as well as a diff. Patch attached. Hmm, yeah, I forgot to fix this regression test when I added that DETAIL line. However, I don't see the need for fooling with the lc_time value? regards, tom lane Here is the diff that I'm seeing on master right now with: make -s check EXTRA_TESTS=collate.linux.utf8 If I qualify it as tr_TR.UTF-8 it works. Perhaps I have something misconfigured on my system (Ubuntu 11.10)? I just installed: language-pack-de language-pack-tr language-pack-sv in an attempt to make the test work, and it works all except for that lc_time settng. Regards, Jeff Davis *** /home/jdavis/wd/git/postgresql/src/test/regress/expected/collate.linux.utf8.out 2011-10-18 00:47:06.817223853 -0700 --- /home/jdavis/wd/git/postgresql/src/test/regress/results/collate.linux.utf8.out 2011-10-18 01:02:06.509206748 -0700 *** *** 396,411 -- to_char SET lc_time TO 'tr_TR'; SELECT to_char(date '2010-04-01', 'DD TMMON '); to_char - ! 01 NIS 2010 (1 row) SELECT to_char(date '2010-04-01', 'DD TMMON ' COLLATE tr_TR); to_char - ! 01 NİS 2010 (1 row) -- backwards parsing --- 396,412 -- to_char SET lc_time TO 'tr_TR'; + ERROR: invalid value for parameter lc_time: tr_TR SELECT to_char(date '2010-04-01', 'DD TMMON '); to_char - ! 01 APR 2010 (1 row) SELECT to_char(date '2010-04-01', 'DD TMMON ' COLLATE tr_TR); to_char - ! 01 APR 2010 (1 row) -- backwards parsing == -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] new compiler warnings
I'm not sure if these can/should be fixed or not, but here are the compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with -O2. The gcc ones are mostly new. GCC $ gcc --version gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1 Copyright (C) 2011 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. warnings generated: execQual.c: In function ‘GetAttributeByNum’: execQual.c:1112:11: warning: the comparison will always evaluate as ‘true’ for the address of ‘tmptup’ will never be NULL [-Waddress] execQual.c: In function ‘GetAttributeByName’: execQual.c:1173:11: warning: the comparison will always evaluate as ‘true’ for the address of ‘tmptup’ will never be NULL [-Waddress] execQual.c: In function ‘ExecEvalFieldSelect’: execQual.c:3922:11: warning: the comparison will always evaluate as ‘true’ for the address of ‘tmptup’ will never be NULL [-Waddress] In file included from gram.y:12962:0: scan.c: In function ‘yy_try_NUL_trans’: scan.c:16257:23: warning: unused variable ‘yyg’ [-Wunused-variable] elog.c: In function ‘write_pipe_chunks’: elog.c:2479:8: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] elog.c:2488:7: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] elog.c: In function ‘write_console’: elog.c:1797:7: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] tuplesort.c: In function ‘comparetup_heap’: tuplesort.c:2751:12: warning: the comparison will always evaluate as ‘true’ for the address of ‘ltup’ will never be NULL [-Waddress] tuplesort.c:2752:12: warning: the comparison will always evaluate as ‘true’ for the address of ‘rtup’ will never be NULL [-Waddress] tuplesort.c: In function ‘copytup_heap’: tuplesort.c:2783:17: warning: the comparison will always evaluate as ‘true’ for the address of ‘htup’ will never be NULL [-Waddress] tuplesort.c: In function ‘readtup_heap’: tuplesort.c:2835:17: warning: the comparison will always evaluate as ‘true’ for the address of ‘htup’ will never be NULL [-Waddress] fe-connect.c: In function ‘PQconndefaults’: fe-connect.c:832:6: warning: the comparison will always evaluate as ‘false’ for the address of ‘errorBuf’ will never be NULL [-Waddress] fe-connect.c: In function ‘PQconninfoParse’: fe-connect.c:3970:6: warning: the comparison will always evaluate as ‘false’ for the address of ‘errorBuf’ will never be NULL [-Waddress] common.c: In function ‘handle_sigint’: common.c:247:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c:250:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c:251:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] In file included from mainloop.c:425:0: psqlscan.l: In function ‘evaluate_backtick’: psqlscan.l:1677:6: warning: the comparison will always evaluate as ‘false’ for the address of ‘cmd_output’ will never be NULL [-Waddress] CLANG $ clang --version clang version 2.9 (tags/RELEASE_29/final) Target: x86_64-pc-linux-gnu Thread model: posix A lot of warnings of the form: ruleutils.c:698:11: warning: array index of '1' indexes past the end of an array (that contains 1 elements) [-Warray-bounds] value = fastgetattr(ht_trig, Anum_pg_trigger_tgargs, ^~~~ In file included from ruleutils.c:23: In file included from ../../../../src/include/catalog/indexing.h:18: ../../../../src/include/access/htup.h:808:3: note: instantiated from: att_isnull((attnum)-1, (tup)-t_data-t_bits) ? \ ^ In file included from ruleutils.c:23: In file included from ../../../../src/include/catalog/indexing.h:18: In file included from ../../../../src/include/access/htup.h:18: ../../../../src/include/access/tupmacs.h:21:34: note: instantiated from: #define att_isnull(ATT, BITS) (!((BITS)[(ATT) 3] (1 ((ATT) 0x07 ^ ~~ In file included from ruleutils.c:23: In file included from ../../../../src/include/catalog/indexing.h:18: ../../../../src/include/access/htup.h:153:9: note: array 't_bits' declared here bits8 t_bits[1]; /* bitmap of NULLs -- VARIABLE LENGTH */ == Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] HeapTupleHeaderAdvanceLatestRemovedXid doing the wrong thing with multixacts
On Tue, Oct 18, 2011 at 3:14 AM, Alvaro Herrera alvhe...@alvh.no-ip.org wrote: I'll add an assert to check this and a comment to explain. This means I'll have to hack it up further in my FK locks patch. No problem with that. OK, I'll hold back to avoid interfering with your patch. -- Simon Riggs 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] BUG or strange behaviour of update on primary key
2011/10/18 Robert Haas robertmh...@gmail.com On Mon, Oct 17, 2011 at 7:30 PM, desmodemone desmodem...@gmail.com wrote: Seems an Oracle bug not Postgresql one! I don't think it's a bug for it to work. It'd probably work in PostgreSQL too, if you inserted (2) first and then (1). It's just that, as Tom says, if you want it to be certain to work (rather than depending on the order in which the rows are inserted), you need the checks to be deferred. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company 2011/10/18 desmodemone desmodem...@gmail.com Hi there, I could workaround the behavior with deferred constraint, and it's ok, but as I show, I have different behavior for constraint with the same definition in two rdbms and Postgresql depends on the physical order of row (with the same definition of constraint NOT DEFERRABLE INITIALLY IMMEDIATE) , or better Postgresql seems to check for every row, even if the command is one (I am doing one update on all of rows) , right? . Moreover , in documentation the definition says that a not deferrable constraints will check after every command , not after every row of the command: http://www.postgresql.org/docs/9.1/static/sql-createtable.html DEFERRABLE NOT DEFERRABLE This controls whether the constraint can be deferred.* A constraint that is not deferrable will be checked immediately after every command*. Checking of constraints that are deferrable can be postponed until the end of the transaction (using the SET CONSTRAINTShttp://www.postgresql.org/docs/9.0/static/sql-set-constraints.html command). NOT DEFERRABLE is the default. Currently, only UNIQUE, PRIMARY KEY, EXCLUDE, and REFERENCES (foreign key) constraints accept this clause. NOT NULL and CHECK constraints are not deferrable. --- If this is historical buggy behavior for performance , I think we have to change the definition of NOT DEFERRABLE in documentation, because Postgresql is not checking at end of a dml, but for every row modified by the command or there is something needs a patch. Regards, Mat Hello there, I think I have find a limit of this workaround. Imagine I have two tables in Oracle or other rdbms with a foreign key between them : testup3 ( a int) primary key on a NOT DEFERRABLE INITIALLY IMMEDIATE ; testup4 ( a int) foreign key on a references testup3(a) ; For first table I could create this (to have a normal sql standard behavior on update with multiple rows) : testup3 ( a int) primary key on a DEFERRABLE INITIALLY IMMEDIATE ; By the way I could not create a foreign key on a DEFERRABLE constraint , in fact I obtain an error like this : ERROR: cannot use a deferrable unique constraint for referenced table So if I have a normal ERD schema with FK , I could not use the workaround of DEFERRABLE constraints . I found an old discussion on this : http://archives.postgresql.org/pgsql-hackers/2010-06/msg00151.php http://archives.postgresql.org/pgsql-hackers/2010-06/msg00168.php In my opinion it could be a big limitation for who want migrate applications or is developing applications on different db. Any suggest or idea ? Regards, Mat
[HACKERS] Silent failure with invalid hba_file setting
Hi, I noticed that if the hba_file setting in the config is uncommented and set to a directory instead of the full path to the file, no error occurs when the service starts. For example: hba_file = '/home/thom/Development/data' The problem with this is you cannot get into the database as it acts as if it did find the hba file but found it empty. Shouldn't a check be in place to ensure that the parameter resolves to a file rather than anything else? And even if it does find a file, but it's empty, shouldn't it also produce a warning of some kind? -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] (patch) regression diffs on collate.linux.utf8 test
Jeff Davis pg...@j-davis.com writes: On Sun, 2011-10-16 at 16:00 -0400, Tom Lane wrote: Hmm, yeah, I forgot to fix this regression test when I added that DETAIL line. However, I don't see the need for fooling with the lc_time value? Here is the diff that I'm seeing on master right now with: make -s check EXTRA_TESTS=collate.linux.utf8 If I qualify it as tr_TR.UTF-8 it works. Perhaps I have something misconfigured on my system (Ubuntu 11.10)? I just installed: language-pack-de language-pack-tr language-pack-sv That's very strange. It works as-is for me on Fedora 13, 14, and 15, which you'd expect to have essentially the same I18N infrastructure as Ubuntu (and I'm pretty sure I didn't install any optional language support for Turkish). Anybody know what the problem is? The reason I'm resisting just changing it is that I'd prefer to minimize the number of dependencies this regression test has on the exact spelling of UTF-8, as that is not terribly well standardized. On my Fedora boxes, for instance, locale -a says that tr_TR.utf8 is the name of that particular locale. 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) regression diffs on collate.linux.utf8 test
On tis, 2011-10-18 at 01:07 -0700, Jeff Davis wrote: On Sun, 2011-10-16 at 16:00 -0400, Tom Lane wrote: Jeff Davis pg...@j-davis.com writes: On master, I see a minor test error (at least on my machine) as well as a diff. Patch attached. Hmm, yeah, I forgot to fix this regression test when I added that DETAIL line. However, I don't see the need for fooling with the lc_time value? regards, tom lane Here is the diff that I'm seeing on master right now with: make -s check EXTRA_TESTS=collate.linux.utf8 If I qualify it as tr_TR.UTF-8 it works. Perhaps I have something misconfigured on my system (Ubuntu 11.10)? I just installed: language-pack-de language-pack-tr language-pack-sv in an attempt to make the test work, and it works all except for that lc_time settng. I think the language-pack packages have nothing to do with it; they only supply translations. Possibly, things are set up so that only UTF-8 locales are installed by default. Since the collate.linux.utf8 requires a UTF-8 environment, it seems reasonable to use the tr_TR.UTF-8 locale for LC_TIME, instead of requiring an unrelated (ISO-8859-9) locale to be installed. So I think the change you propose is reasonable. -- 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] new compiler warnings
On tis, 2011-10-18 at 01:00 -0700, Jeff Davis wrote: I'm not sure if these can/should be fixed or not, but here are the compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with -O2. The gcc ones are mostly new. They are expected with gcc 4.6. There isn't anything we can do about them. The clang warnings are also expected. I understand the next clang version will address them. GCC $ gcc --version gcc (Ubuntu/Linaro 4.6.1-9ubuntu3) 4.6.1 Copyright (C) 2011 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. warnings generated: execQual.c: In function ‘GetAttributeByNum’: execQual.c:1112:11: warning: the comparison will always evaluate as ‘true’ for the address of ‘tmptup’ will never be NULL [-Waddress] execQual.c: In function ‘GetAttributeByName’: execQual.c:1173:11: warning: the comparison will always evaluate as ‘true’ for the address of ‘tmptup’ will never be NULL [-Waddress] execQual.c: In function ‘ExecEvalFieldSelect’: execQual.c:3922:11: warning: the comparison will always evaluate as ‘true’ for the address of ‘tmptup’ will never be NULL [-Waddress] In file included from gram.y:12962:0: scan.c: In function ‘yy_try_NUL_trans’: scan.c:16257:23: warning: unused variable ‘yyg’ [-Wunused-variable] elog.c: In function ‘write_pipe_chunks’: elog.c:2479:8: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] elog.c:2488:7: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] elog.c: In function ‘write_console’: elog.c:1797:7: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] tuplesort.c: In function ‘comparetup_heap’: tuplesort.c:2751:12: warning: the comparison will always evaluate as ‘true’ for the address of ‘ltup’ will never be NULL [-Waddress] tuplesort.c:2752:12: warning: the comparison will always evaluate as ‘true’ for the address of ‘rtup’ will never be NULL [-Waddress] tuplesort.c: In function ‘copytup_heap’: tuplesort.c:2783:17: warning: the comparison will always evaluate as ‘true’ for the address of ‘htup’ will never be NULL [-Waddress] tuplesort.c: In function ‘readtup_heap’: tuplesort.c:2835:17: warning: the comparison will always evaluate as ‘true’ for the address of ‘htup’ will never be NULL [-Waddress] fe-connect.c: In function ‘PQconndefaults’: fe-connect.c:832:6: warning: the comparison will always evaluate as ‘false’ for the address of ‘errorBuf’ will never be NULL [-Waddress] fe-connect.c: In function ‘PQconninfoParse’: fe-connect.c:3970:6: warning: the comparison will always evaluate as ‘false’ for the address of ‘errorBuf’ will never be NULL [-Waddress] common.c: In function ‘handle_sigint’: common.c:247:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c:250:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c:251:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] In file included from mainloop.c:425:0: psqlscan.l: In function ‘evaluate_backtick’: psqlscan.l:1677:6: warning: the comparison will always evaluate as ‘false’ for the address of ‘cmd_output’ will never be NULL [-Waddress] CLANG $ clang --version clang version 2.9 (tags/RELEASE_29/final) Target: x86_64-pc-linux-gnu Thread model: posix A lot of warnings of the form: ruleutils.c:698:11: warning: array index of '1' indexes past the end of an array (that contains 1 elements) [-Warray-bounds] value = fastgetattr(ht_trig, Anum_pg_trigger_tgargs, ^~~~ In file included from ruleutils.c:23: In file included from ../../../../src/include/catalog/indexing.h:18: ../../../../src/include/access/htup.h:808:3: note: instantiated from: att_isnull((attnum)-1, (tup)-t_data-t_bits) ? \ ^ In file included from ruleutils.c:23: In file included from ../../../../src/include/catalog/indexing.h:18: In file included from ../../../../src/include/access/htup.h:18: ../../../../src/include/access/tupmacs.h:21:34: note: instantiated from: #define att_isnull(ATT, BITS) (!((BITS)[(ATT) 3] (1 ((ATT) 0x07 ^ ~~ In file included from ruleutils.c:23: In file included from ../../../../src/include/catalog/indexing.h:18: ../../../../src/include/access/htup.h:153:9: note: array 't_bits' declared here bits8 t_bits[1]; /* bitmap of NULLs -- VARIABLE LENGTH */
Re: [HACKERS] spinlocks on HP-UX
On Tue, Oct 18, 2011 at 2:20 AM, Pavan Deolasee pavan.deola...@gmail.com wrote: On Tue, Oct 18, 2011 at 10:04 AM, Robert Haas robertmh...@gmail.com wrote: Hmm, so you added the non-locked test in TAS()? Did you try adding it just to TAS_SPIN()? On Itanium, I found that it was slightly better to do it only in TAS_SPIN() - i.e. in the contended case. Would it be a good change for S_LOCK() to use TAS_SPIN() as well ? Well, that would be sort of missing the point of why we invented TAS_SPIN() in the first place. What we found on Itanium is that using the unlocked test always was better than never doing it, but what was even slightly better was to use the unlocked first test *only when spinning*. In other words, on the very first go-around, we use the atomic instruction right away. Only if that fails do we switch to using the unlocked test first. Now it's possible that on some other architecture it's better to do the unlocked test first every time. But it seems somewhat unlikely, because in the hopefully-common case where the spinlock is uncontended, it's just a waste. If you're having enough spinlock contention that the first TAS() is failing frequently, you need to fix the underlying cause of the spinlock contention... -- 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] new compiler warnings
Jeff Davis wrote: I'm not sure if these can/should be fixed or not, but here are the compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with -O2. elog.c: In function ‘write_pipe_chunks’: elog.c:2479:8: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] elog.c:2488:7: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] elog.c: In function ‘write_console’: elog.c:1797:7: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c: In function ‘handle_sigint’: common.c:247:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c:250:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c:251:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] In file included from mainloop.c:425:0: These we are getting only because of a stubborn insistence on coding to the current implementation rather than the API. It's not that much code to code to the API instead. I've already offered to provide the (trivial) patch for this if there is buy-in on the idea of coding to the API. The argument against is that no implementer of the API would ever exercise the freedom the documented API gives them to do *part* of a write to disk and return to the caller the number of bytes written and then allow a subsequent write request to continue the output. I think that the rise of virtual machine environments in big shops provides a fairly obvious reason someone might want to do that. -Kevin -- 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] new compiler warnings
On Tue, Oct 18, 2011 at 9:03 AM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Jeff Davis wrote: I'm not sure if these can/should be fixed or not, but here are the compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with -O2. elog.c: In function ‘write_pipe_chunks’: elog.c:2479:8: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] elog.c:2488:7: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] elog.c: In function ‘write_console’: elog.c:1797:7: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c: In function ‘handle_sigint’: common.c:247:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c:250:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c:251:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] In file included from mainloop.c:425:0: These we are getting only because of a stubborn insistence on coding to the current implementation rather than the API. It's not that much code to code to the API instead. I've already offered to provide the (trivial) patch for this if there is buy-in on the idea of coding to the API. The argument against is that no implementer of the API would ever exercise the freedom the documented API gives them to do *part* of a write to disk and return to the caller the number of bytes written and then allow a subsequent write request to continue the output. I think that the rise of virtual machine environments in big shops provides a fairly obvious reason someone might want to do that. Even if all we got out of it was that the compiler warnings went away, I think that would still be a sufficient reason to do it. And I tend to agree with you that the warnings are legit; and defending against them is virtually free. -- 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] new compiler warnings
On 10/18/2011 09:03 AM, Kevin Grittner wrote: Jeff Davis wrote: I'm not sure if these can/should be fixed or not, but here are the compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with -O2. elog.c: In function ‘write_pipe_chunks’: elog.c:2479:8: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] elog.c:2488:7: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] elog.c: In function ‘write_console’: elog.c:1797:7: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c: In function ‘handle_sigint’: common.c:247:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c:250:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] common.c:251:4: warning: ignoring return value of ‘write’, declared with attribute warn_unused_result [-Wunused-result] In file included from mainloop.c:425:0: These we are getting only because of a stubborn insistence on coding to the current implementation rather than the API. It's not that much code to code to the API instead. I've already offered to provide the (trivial) patch for this if there is buy-in on the idea of coding to the API. The argument against is that no implementer of the API would ever exercise the freedom the documented API gives them to do *part* of a write to disk and return to the caller the number of bytes written and then allow a subsequent write request to continue the output. I think that the rise of virtual machine environments in big shops provides a fairly obvious reason someone might want to do that. There are non-disk uses of write() where partial writes are legitimate (e.g. pipes under some circumstances on Linux). It is a pity we can't just tell the compiler to turn off the warning in a particular case. 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] new compiler warnings
Andrew Dunstan and...@dunslane.net writes: It is a pity we can't just tell the compiler to turn off the warning in a particular case. I haven't tested, but won't an explicit cast to void silence the warning? (void) fwrite(...); There are places, notably the calls in elog.c, where ignoring write failures is the right thing. I think that what Kevin was on about was something else entirely, namely whether we need to retry writes to disk. I would hope that we're not simply not bothering to check in any cases where it matters. 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] [v9.2] make_greater_string() does not return a string in some cases
On Tue, Oct 18, 2011 at 12:11 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Oct 17, 2011 at 11:54 PM, Tom Lane t...@sss.pgh.pa.us wrote: http://www.faqs.org/rfcs/rfc3629.html I'm still confused. The input string is already known to be valid UTF-8, so the second byte (if there is one) must be between 0x80 and 0xBF. Therefore it will be neither 0xED nor 0xF4. I haven't read the patch lately, but ED and F4 are special as *first* bytes. Maybe the logic isn't quite right, or you read it wrong? I think I'll let the patch author comment on that. It looks wrong to me, but I just work here. -- 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] new compiler warnings
Peter Eisentraut pete...@gmx.net writes: On tis, 2011-10-18 at 01:00 -0700, Jeff Davis wrote: I'm not sure if these can/should be fixed or not, but here are the compiler warnings I'm getting on gcc and clang on ubuntu 11.10 with -O2. The gcc ones are mostly new. They are expected with gcc 4.6. There isn't anything we can do about them. Well, we're going to have to think of something, because as more of us move onto the newer gcc releases the annoyance level is going to become intolerable. I think a large fraction of the -Waddress warnings are coming from this line in the heap_getattr macro: AssertMacro((tup) != NULL), \ Seems to me we could just lose that test and be no worse off, since the macro is surely gonna dump core anyway on a null pointer. But some of the remaining -Waddress warnings are not so painless to get rid of. Ultimately we might have to add -Wno-address to the default CFLAGS. 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] new compiler warnings
Tom Lane t...@sss.pgh.pa.us wrote: I think that what Kevin was on about was something else entirely, namely whether we need to retry writes to disk. I would phrase it that we need to *continue* a write to disk if the OS chooses to write a portion of it and return to the caller with the number actually written. If the first write, or any later write, actually gets an error or fails to make progress, *then* we should consider the attempt to be done. I don't understand the point of not coding to the API. -Kevin -- 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_ctl restart - behaviour based on wrong instance
On Wednesday, October 12, 2011, Bruce Momjian wrote: Magnus Hagander wrote: I looked over this issue and I don't thinking having pg_ctl restart fall back to 'start' is a good solution. ?I am concerned about cases where we start a different server without shutting down the old server, for some reason. ?When they say 'restart', I think we have to assume they want a restart. What I did do was to document that not backing up postmaster.pid and postmaster.opts might help prevent pg_ctl from getting confused. Should we exclude postmaster.opts from streaming base backups? We already exclude postmaster.pid... Uh, I think so, unless my analysis was wrong. Ok, fixed and applied. //Magnus -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
Re: [HACKERS] new compiler warnings
On tis, 2011-10-18 at 09:32 -0400, Tom Lane wrote: Andrew Dunstan and...@dunslane.net writes: It is a pity we can't just tell the compiler to turn off the warning in a particular case. I haven't tested, but won't an explicit cast to void silence the warning? (void) fwrite(...); No, tried that already. You could try rc = write(...); (void) rc; There are places, notably the calls in elog.c, where ignoring write failures is the right thing. I think that what Kevin was on about was something else entirely, namely whether we need to retry writes to disk. I would hope that we're not simply not bothering to check in any cases where it matters. No, I believe we are OK everywhere else. We are only ignoring the result in cases where we are trying to report errors in the first place. -- 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] new compiler warnings
On Tue, Oct 18, 2011 at 10:06 AM, Peter Eisentraut pete...@gmx.net wrote: No, I believe we are OK everywhere else. We are only ignoring the result in cases where we are trying to report errors in the first place. The relevant code is: while (len PIPE_MAX_PAYLOAD) { p.proto.is_last = (dest == LOG_DESTINATION_CSVLOG ? 'F' : 'f'); p.proto.len = PIPE_MAX_PAYLOAD; memcpy(p.proto.data, data, PIPE_MAX_PAYLOAD); write(fd, p, PIPE_HEADER_SIZE + PIPE_MAX_PAYLOAD); data += PIPE_MAX_PAYLOAD; len -= PIPE_MAX_PAYLOAD; } Which it seems to me we could change by doing rc = write(). Then if rc = 0, we bail out. If not, we add and subtract rc, rather than PIPE_MAX_PAYLOAD. That would be barely more code, probably safer, and would silence the warning. -- 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] SSPI authentication fails on Windows when server parameter is localhost or domain name
Hi, My apologies for a very late reply. I agree the fix you applied is a better one. I have verified the fix by testing the 'postgresql-9.1.1-1-windows-x64' installer. Thank you. On Thu, Jul 14, 2011 at 7:23 PM, Magnus Hagander mag...@hagander.netwrote: On Wed, Jun 15, 2011 at 10:53, Ahmed Shinwari ahmed.shinw...@gmail.com wrote: Hi All, I faced a bug on Windows while connecting via SSPI authentication. I was able to find the bug and have attached the patch. Details listed below; Postgres Installer: Version 9.0.4 OS: Windows Server 2008 R2/Windows 7 big snip Thanks - great analysis! However, I think there is a better fix for this - simply moving a } one line. In particular, I'm concerned about passing the same pointer both as input and output to the function - I couldn't find anything in the documentation saying this was safe (nor did I find anything saying it's unsafe, but.) Especially since this code clearly behaves different on different versions - I've been completely unable to reproduce this on any of my test machines, but they are all Windows Server 2003. So - attached is a new version of the patch, how does this look to you? FYI, I've had Thom test this new version and it does appear to work fine in his scenario. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Ahmed Shinwari EnterpriseDB Corporation : www.enterprisedb.com The Enterprise Postgres Company
Re: [HACKERS] new compiler warnings
Robert Haas robertmh...@gmail.com wrote: Which it seems to me we could change by doing rc = write(). Then if rc = 0, we bail out. If not, we add and subtract rc, rather than PIPE_MAX_PAYLOAD. Something along the general lines of this?: http://archives.postgresql.org/pgsql-hackers/2011-02/msg01719.php That would be barely more code, probably safer, and would silence the warning. Exactly. -Kevin -- 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] new compiler warnings
Kevin Grittner kevin.gritt...@wicourts.gov wrote: http://archives.postgresql.org/pgsql-hackers/2011-02/msg01719.php Although, it being a quick example of the general idea, I have an obvious bug there -- the write location would have to be buffer + t. I think Noah might have also posted some example code a month or two back. -Kevin -- 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] Separating bgwriter and checkpointer
On Wed, Oct 5, 2011 at 8:02 AM, Simon Riggs si...@2ndquadrant.com wrote: On Wed, Oct 5, 2011 at 5:10 AM, Dickson S. Guedes lis...@guedesoft.net wrote: Ah ok! I started reviewing the v4 patch version, this is my comments: ... Well, all the tests was running with the default postgresql.conf in my laptop but I'll setup a more real world environment to test for performance regression. Until now I couldn't notice any significant difference in TPS before and after patch in a small environment. I'll post something soon. Great testing, thanks. Likely will have no effect in non-I/O swamped environment, but no regression expected either. Any reason or objection to committing this patch? -- Simon Riggs 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] synchronized snapshots
On Mon, Oct 3, 2011 at 7:09 AM, Marko Tiikkaja marko.tiikk...@2ndquadrant.com wrote: On 2011-09-28 15:25, Joachim Wieland wrote: Yes, that's the desired behaviour, the patch add this paragraph to the documentation already: I can't believe I missed that. My apologies. On 2011-09-29 05:16, Joachim Wieland wrote: The attached patch addresses the reported issues. Thanks, this one looks good to me. Going to mark this patch as ready for committer. I don't see any tests with this patch, so I personally won't be the committer on this just yet. Also, not sure why the snapshot id syntax has leading zeroes on first part of the number, but not on second part. It will still sort incorrectly if that's what we were trying to achieve. Either leave off the leading zeroes on first part of add them to second. We probably need some more discussion added to the README about this. I'm also concerned that we are adding this to the BEGIN statement as the only option. I don't have a problem with it being there, but I do think it is a problem to make it the *only* way to use this. Altering BEGIN gives clear problems with any API that does the begin and commit for you, such as perl DBI, java JDBC to name just two. I can't really see its a good implementation if we say this won't work until client APIs follow our new non-standard syntax. I wouldn't block commit on this point, but I think we should work on alternative ways to invoke this feature as well. -- Simon Riggs 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] [v9.2] Object access hooks with arguments support (v1)
2011/10/18 Robert Haas robertmh...@gmail.com: On Thu, Oct 13, 2011 at 6:48 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: struct ObjectAccessInfoData { ObjectAccessType oa_type; ObjectAddress oa_address; union { struct { HeapTuple newtuple; TupleDesc tupdesc; /* only if create a new relation */ : } post_create; /* additional info for OAT_POST_CREATE event */ } } That's possibly an improvement over just passing everything opaquely, but it still doesn't seem very good. I mean, this is C, not Perl or Python. Anything where you pass a bunch of arguments of indeterminate type and hope that the person you're calling can figure it out is inherently pretty dangerous. I had it in mind that the object access hook mechanism could serve as a simple and generic way of letting an external module know that an event of a certain type has occurred on a certain object, and to let that module gain control. But if we have to pass a raft of arguments in, then it's not generic any more - it's just a bunch of things that are probably really need to be separate hooks shoved into a single function. I got an inspiration of this idea from APIs of foreign-data-wrapper that trusts values of each fields that contains function pointers. Indeed, we may have a risk of backend crash, if we try to run modules built towards v9.1 on the pgsql-v9.2devel. However, it is not avoidable, unless we rewrite whole of the code by Java?, Python? or something? :-( At least, we don't promise binary compatible across major versions? If so, I don't think dangerousness is a reasonable reason why the hook does not take any additional arguments, as long as auther of modules can be noticed by compiler warnings. The point to be focused on is how do we implement the target feature (DDL permissions by sepgsql in my case) with simpleness. Moreover, if you did document it, I think it would boil down to this is what sepgsql happens to need, and I don't think that's an acceptable answer. ?We have repeatedly refused to adopt that approach in the past. Unfortunately, I still don't have a clear answer for this point. However, in general terms, it is impossible to design any interface without knowledge of its usage more or less. Well, that's true. But the hook also has to be maintainable. ISTM that there's no reasonable way for someone making a modification to the code to know whether the additional local variable that they just added to the function should be passed to the hook, or not. Here, at least as far as I can see, there's no guiding principle that would enable future contributors to make an intelligent decision about that. And if someone wanted to write another client for the hook, it's not very obvious whether the particular things you've chosen to pass here would be relevant or not. I think if you look over the code you'll find that there's nowhere else that we have a hook that looks anything like what you're proposing here. In the case when the second client of the hook is proposed, a straight- forward approach is to expand the above ObjectAccessInfoData to satisfy the requirement of new one, if existing arguments are not enough. Because existing modules ignore the new fields, it is harmless. Linux adopts this approch to host multiple security modules without confusion, although one makes decision by security label and one other makes decision by pathname of files; they require different information on the point to make its decision, because they ignore unrelevant arguments; such as pathname in selinux. I remember you worry about the number of arguments getting increased infinity, however, it is an extreme situation. We are not an A.I to commit proposed patches automatically, so auther of modules (including me) will explain why the new arguments are additionally needed here, like as pathname based security module did in Linux kernel development. At least, this proposition enables modules being informed using commonly used data type (such as HeapTuple, TupleDesc), compared to the past approach that tried to push all the necessary information into argument list individually. That does seem like a good direction to go in, but you're still passing a lot of other stuff in there. I guess my feeling is that if we can't boil down the argument list to a short list of things that are more-or-less common to all the call sites, we probably need to rethink the whole design. Honestly, the arguments of object_access_hook is a method to implement security model of sepgsql, not a purpose of mine. So, an alternative idea is quite welcome to implement sepgsql. However, I think it is less invasive way than other ideas right now. For example, I hope sepgsql to perform as follows when user create a new table. - It computes a default security label that needs Oid of the
Re: [HACKERS] pg_ctl restart - behaviour based on wrong instance
On Tue, Oct 18, 2011 at 11:02 PM, Magnus Hagander mag...@hagander.net wrote: Ok, fixed and applied. You seem to have forgot to change protocol.sgml. Patch attached. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center protocol_sgml_v1.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] [v9.2] Object access hooks with arguments support (v1)
On Tue, Oct 18, 2011 at 11:25 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: For example, I hope sepgsql to perform as follows when user create a new table. - It computes a default security label that needs Oid of the namespace. - It checks db_table:{create} permission on the security label being computed. - In addition, it checks db_table:{insert} permission when SELECT INTO - Also, it checks these permissions on columns being newly created. - However, It does not check permissions when the tables are internally created, such as toast tables or temporary relations created by make_new_heap(). That's superficially reasonable, but I don't feel good about passing is_select_info to the object access hook here. If insert permission is required there, then it ought to be getting checked by selinux at the same place that it's getting checked for at the DAC level. If we get into a situation where sepgsql is checking permissions using some system that's totally different from what we do for DAC, I think that's going to produce confusing and illogical results. This is ground we've been over before. Similarly with fdw_srvname. The only excuse for passing that is to handle a permissions check for foreign data wrappers that isn't being done for DAC: if there is a DAC permissions check, then the MAC one should be done there also, not someplace totally different. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_ctl restart - behaviour based on wrong instance
On Tue, Oct 18, 2011 at 11:02 PM, Magnus Hagander mag...@hagander.net wrote: Ok, fixed and applied. You seem to have forgot to change protocol.sgml. Patch attached. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center protocol_sgml_v1.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] new compiler warnings
Robert Haas robertmh...@gmail.com writes: On Tue, Oct 18, 2011 at 10:06 AM, Peter Eisentraut pete...@gmx.net wrote: No, I believe we are OK everywhere else. We are only ignoring the result in cases where we are trying to report errors in the first place. The relevant code is: while (len PIPE_MAX_PAYLOAD) { p.proto.is_last = (dest == LOG_DESTINATION_CSVLOG ? 'F' : 'f'); p.proto.len = PIPE_MAX_PAYLOAD; memcpy(p.proto.data, data, PIPE_MAX_PAYLOAD); write(fd, p, PIPE_HEADER_SIZE + PIPE_MAX_PAYLOAD); data += PIPE_MAX_PAYLOAD; len -= PIPE_MAX_PAYLOAD; } Which it seems to me we could change by doing rc = write(). Then if rc = 0, we bail out. If not, we add and subtract rc, rather than PIPE_MAX_PAYLOAD. That would be barely more code, probably safer, and would silence the warning. And it would break the code. The whole point here is that the message must be sent indivisibly. 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] new compiler warnings
Tom Lane t...@sss.pgh.pa.us wrote: And it would break the code. The whole point here is that the message must be sent indivisibly. If the new code splits the message, it would previously have been truncated. Is that less broken? -Kevin -- 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] Separating bgwriter and checkpointer
On Tue, Oct 18, 2011 at 9:18 AM, Simon Riggs si...@2ndquadrant.com wrote: Any reason or objection to committing this patch? Not on my end, though I haven't reviewed it in detail. One minor note - I was mildly surprised to see that you moved this to the checkpointer rather than leaving it in the bgwriter: + /* Do this once before starting the loop, then just at SIGHUP time. */ + SyncRepUpdateSyncStandbysDefined(); My preference would probably have been to leave that in the background writer, on the theory that the checkpointer's work is likely to be more bursty and therefore it might be less responsive. -- 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] pg_ctl restart - behaviour based on wrong instance
On Tue, Oct 18, 2011 at 11:02 PM, Magnus Hagander mag...@hagander.net wrote: Ok, fixed and applied. You seem to have forgot to change protocol.sgml. Patch attached. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center protocol_sgml_v1.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] pg_ctl restart - behaviour based on wrong instance
On Tue, Oct 18, 2011 at 11:02 PM, Magnus Hagander mag...@hagander.net wrote: Ok, fixed and applied. You seem to have forgot to change protocol.sgml. Patch attached. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center protocol_sgml_v1.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] new compiler warnings
On Tue, Oct 18, 2011 at 12:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Oct 18, 2011 at 10:06 AM, Peter Eisentraut pete...@gmx.net wrote: No, I believe we are OK everywhere else. We are only ignoring the result in cases where we are trying to report errors in the first place. The relevant code is: while (len PIPE_MAX_PAYLOAD) { p.proto.is_last = (dest == LOG_DESTINATION_CSVLOG ? 'F' : 'f'); p.proto.len = PIPE_MAX_PAYLOAD; memcpy(p.proto.data, data, PIPE_MAX_PAYLOAD); write(fd, p, PIPE_HEADER_SIZE + PIPE_MAX_PAYLOAD); data += PIPE_MAX_PAYLOAD; len -= PIPE_MAX_PAYLOAD; } Which it seems to me we could change by doing rc = write(). Then if rc = 0, we bail out. If not, we add and subtract rc, rather than PIPE_MAX_PAYLOAD. That would be barely more code, probably safer, and would silence the warning. And it would break the code. The whole point here is that the message must be sent indivisibly. How is that different than the chunking that the while loop is already doing? -- 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] Separating bgwriter and checkpointer
On Tue, Oct 18, 2011 at 5:39 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Oct 18, 2011 at 9:18 AM, Simon Riggs si...@2ndquadrant.com wrote: Any reason or objection to committing this patch? Not on my end, though I haven't reviewed it in detail. One minor note - I was mildly surprised to see that you moved this to the checkpointer rather than leaving it in the bgwriter: + /* Do this once before starting the loop, then just at SIGHUP time. */ + SyncRepUpdateSyncStandbysDefined(); My preference would probably have been to leave that in the background writer, on the theory that the checkpointer's work is likely to be more bursty and therefore it might be less responsive. That needs to be in the checkpointer because that is the process that shuts down last. The bgwriter is now more like the walwriter. It shuts down early in the shutdown process, while the checkpointer is last out. So it wasn't preference, it was a requirement of the new role definitions. -- Simon Riggs 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] Separating bgwriter and checkpointer
On Tue, Oct 18, 2011 at 12:53 PM, Simon Riggs si...@2ndquadrant.com wrote: On Tue, Oct 18, 2011 at 5:39 PM, Robert Haas robertmh...@gmail.com wrote: On Tue, Oct 18, 2011 at 9:18 AM, Simon Riggs si...@2ndquadrant.com wrote: Any reason or objection to committing this patch? Not on my end, though I haven't reviewed it in detail. One minor note - I was mildly surprised to see that you moved this to the checkpointer rather than leaving it in the bgwriter: + /* Do this once before starting the loop, then just at SIGHUP time. */ + SyncRepUpdateSyncStandbysDefined(); My preference would probably have been to leave that in the background writer, on the theory that the checkpointer's work is likely to be more bursty and therefore it might be less responsive. That needs to be in the checkpointer because that is the process that shuts down last. The bgwriter is now more like the walwriter. It shuts down early in the shutdown process, while the checkpointer is last out. So it wasn't preference, it was a requirement of the new role definitions. Oh, I see. -- 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] new compiler warnings
Robert Haas robertmh...@gmail.com writes: On Tue, Oct 18, 2011 at 12:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: And it would break the code. The whole point here is that the message must be sent indivisibly. How is that different than the chunking that the while loop is already doing? The chunks are sent indivisibly, because they are less than the pipe buffer size. Read the pipe man page. It's guaranteed that the write will either succeed or fail as a whole, not write a partial message. If we cared to retry a failure, there would be some point in checking the return code. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_ctl restart - behaviour based on wrong instance
Oh, sorry for repeating the same posts. Gmail seems to have not worked fine... :( On Wed, Oct 19, 2011 at 1:24 AM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Oct 18, 2011 at 11:02 PM, Magnus Hagander mag...@hagander.net wrote: Ok, fixed and applied. You seem to have forgot to change protocol.sgml. Patch attached. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION 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] new compiler warnings
On Tue, Oct 18, 2011 at 1:01 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Tue, Oct 18, 2011 at 12:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: And it would break the code. The whole point here is that the message must be sent indivisibly. How is that different than the chunking that the while loop is already doing? The chunks are sent indivisibly, because they are less than the pipe buffer size. Read the pipe man page. It's guaranteed that the write will either succeed or fail as a whole, not write a partial message. If we cared to retry a failure, there would be some point in checking the return code. On MacOS X v10.6.8, I see no such guarantee in the pipe(2) man page. The man page for read(2) says: Upon successful completion, read(), readv(), and pread() return the number of bytes actually read and placed in the buffer. The system guarantees to read the number of bytes requested if the descriptor references a normal file that has that many bytes left before the end-of-file, but in no other case. In any event, whether or not it's *possible* to have a short read is a separate question from what we should do if it happens. Retrying an error doesn't seem practical, because in all likelihood the error will recur forever and we'll go into an infinite loop. But if we do somehow get a short write, sending the rest of the current chunk in the next write() does not seem materially worse than sending the next chunk. -- 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] new compiler warnings
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@sss.pgh.pa.us wrote: I think that what Kevin was on about was something else entirely, namely whether we need to retry writes to disk. I would phrase it that we need to *continue* a write to disk if the OS chooses to write a portion of it and return to the caller with the number actually written. If the first write, or any later write, actually gets an error or fails to make progress, *then* we should consider the attempt to be done. I don't understand the point of not coding to the API. My point here is just that that's a different discussion. You are not talking about places where this new compiler warning is getting raised. 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] new compiler warnings
On tis, 2011-10-18 at 09:36 -0400, Tom Lane wrote: But some of the remaining -Waddress warnings are not so painless to get rid of. Ultimately we might have to add -Wno-address to the default CFLAGS. Here is the bug report to gcc on this issue: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48778 FWIW, I've been building with -Wno-error=address for months, ever since gcc 4.6 because the default on my machine. I don't know what other issues one might be missing that way. -- 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] synchronized snapshots
Simon Riggs si...@2ndquadrant.com writes: On Mon, Oct 3, 2011 at 7:09 AM, Marko Tiikkaja Thanks, this one looks good to me. Going to mark this patch as ready for committer. I don't see any tests with this patch, so I personally won't be the committer on this just yet. I've already taken it according to the commitfest app. There's a lot of things I don't like stylistically, but they all seem fixable, and I'm working through them now. The only actual bug I've found so far is a race condition while setting MyProc-xmin (you can't do that separately from verifying that the source transaction is still running, else somebody else could see a global xmin that's gone backwards). Also, not sure why the snapshot id syntax has leading zeroes on first part of the number, but not on second part. It will still sort incorrectly if that's what we were trying to achieve. Either leave off the leading zeroes on first part of add them to second. The first part is of fixed length, the second not so much. I'm not wedded to the syntax but I don't see anything wrong with it either. I'm also concerned that we are adding this to the BEGIN statement as the only option. Huh? The last version of the patch has it only as SET TRANSACTION SNAPSHOT, which I think is the right way. 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] [v9.2] Object access hooks with arguments support (v1)
2011/10/18 Robert Haas robertmh...@gmail.com: On Tue, Oct 18, 2011 at 11:25 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: For example, I hope sepgsql to perform as follows when user create a new table. - It computes a default security label that needs Oid of the namespace. - It checks db_table:{create} permission on the security label being computed. - In addition, it checks db_table:{insert} permission when SELECT INTO - Also, it checks these permissions on columns being newly created. - However, It does not check permissions when the tables are internally created, such as toast tables or temporary relations created by make_new_heap(). That's superficially reasonable, but I don't feel good about passing is_select_info to the object access hook here. If insert permission is required there, then it ought to be getting checked by selinux at the same place that it's getting checked for at the DAC level. If we get into a situation where sepgsql is checking permissions using some system that's totally different from what we do for DAC, I think that's going to produce confusing and illogical results. This is ground we've been over before. Similarly with fdw_srvname. The only excuse for passing that is to handle a permissions check for foreign data wrappers that isn't being done for DAC: if there is a DAC permissions check, then the MAC one should be done there also, not someplace totally different. If you are suggesting DAC and MAC permissions should be checked on the same place like as we already doing at ExecCheckRTPerms(), I'd like to agree with the suggestion, rather than all the checks within object_access_hook, although it will take code reworking around access controls. In the example table creation, heap_create_with_catalog() is invoked by 5 routines, however, 3 of them are just internal usages, so it is not preferable to apply permission checks on table creation If we may have a hook on table creation next to the place currently we checks permission on the namespace being created, it is quite helpful to implement sepgsql. BTW, it seems to me SELECT INTO should also check insert permission on DAC level, because it become an incorrect assumption that owner of table has insert permission on its creation time. postgres=# ALTER DEFAULT PRIVILEGES FOR USER tak GRANT select ON TABLES TO tak; ALTER DEFAULT PRIVILEGES postgres=# \ddp Default access privileges Owner | Schema | Type | Access privileges ---++---+--- tak || table | tak=r/tak (1 row) postgres=# SET SESSION AUTHORIZATION tak; SET postgres= SELECT * INTO t1 FROM (VALUES(1,'aaa'), (2,'bbb'), (3,'ccc')) AS v; SELECT 3 postgres= SELECT * FROM t1; column1 | column2 -+- 1 | aaa 2 | bbb 3 | ccc (3 rows) postgres= INSERT INTO t1 VALUES (4,'ddd'); ERROR: permission denied for relation t1 Oops! -- KaiGai Kohei kai...@kaigai.gr.jp -- 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] synchronized snapshots
On Tue, Oct 18, 2011 at 6:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: I'm also concerned that we are adding this to the BEGIN statement as the only option. Huh? The last version of the patch has it only as SET TRANSACTION SNAPSHOT, which I think is the right way. Sorry Tom, didn't see your name on it earlier, thats not shown on the main CF display and I didn't think to check on the detail. Please continue. I misread the SET TRANSACTION docs changes. Happy with that. -- Simon Riggs 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] new compiler warnings
Robert Haas robertmh...@gmail.com writes: On Tue, Oct 18, 2011 at 1:01 PM, Tom Lane t...@sss.pgh.pa.us wrote: The chunks are sent indivisibly, because they are less than the pipe buffer size. Read the pipe man page. It's guaranteed that the write will either succeed or fail as a whole, not write a partial message. If we cared to retry a failure, there would be some point in checking the return code. On MacOS X v10.6.8, I see no such guarantee in the pipe(2) man page. Sorry, maybe write(2) is the place to look. The Single Unix Spec quoth (at http://pubs.opengroup.org/onlinepubs/9699919799/): Write requests to a pipe or FIFO shall be handled in the same way as a regular file with the following exceptions: There is no file offset associated with a pipe, hence each write request shall append to the end of the pipe. Write requests of {PIPE_BUF} bytes or less shall not be interleaved with data from other processes doing writes on the same pipe. Writes of greater than {PIPE_BUF} bytes may have data interleaved, on arbitrary boundaries, with writes by other processes, whether or not the O_NONBLOCK flag of the file status flags is set. If the O_NONBLOCK flag is clear, a write request may cause the thread to block, but on normal completion it shall return nbyte. Note the last in particular. Short writes are specifically disallowed on pipes. If this were not the case, the logging collector protocol would be useless. 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] new compiler warnings
On 10/18/2011 01:35 PM, Tom Lane wrote: Robert Haasrobertmh...@gmail.com writes: On Tue, Oct 18, 2011 at 1:01 PM, Tom Lanet...@sss.pgh.pa.us wrote: The chunks are sent indivisibly, because they are less than the pipe buffer size. Read the pipe man page. It's guaranteed that the write will either succeed or fail as a whole, not write a partial message. If we cared to retry a failure, there would be some point in checking the return code. On MacOS X v10.6.8, I see no such guarantee in the pipe(2) man page. Sorry, maybe write(2) is the place to look. The Single Unix Spec quoth (at http://pubs.opengroup.org/onlinepubs/9699919799/): Write requests to a pipe or FIFO shall be handled in the same way as a regular file with the following exceptions: There is no file offset associated with a pipe, hence each write request shall append to the end of the pipe. Write requests of {PIPE_BUF} bytes or less shall not be interleaved with data from other processes doing writes on the same pipe. Writes of greater than {PIPE_BUF} bytes may have data interleaved, on arbitrary boundaries, with writes by other processes, whether or not the O_NONBLOCK flag of the file status flags is set. If the O_NONBLOCK flag is clear, a write request may cause the thread to block, but on normal completion it shall return nbyte. Note the last in particular. Short writes are specifically disallowed on pipes. If this were not the case, the logging collector protocol would be useless. My dim recollection is that Tom and I and maybe some others did tests on a bunch of platforms at the time we introduced the protocol to make sure it did work this way, since it's crucial to making sure we don't get interleaved log lines. 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] [v9.2] Object access hooks with arguments support (v1)
On Tue, Oct 18, 2011 at 1:23 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote: If you are suggesting DAC and MAC permissions should be checked on the same place like as we already doing at ExecCheckRTPerms(), I'd like to agree with the suggestion, rather than all the checks within object_access_hook, although it will take code reworking around access controls. Agreed. In the example table creation, heap_create_with_catalog() is invoked by 5 routines, however, 3 of them are just internal usages, so it is not preferable to apply permission checks on table creation Some wit once made the remark that if a function has 10 arguments, it has 11 arguments, meaning that functions with very large numbers of arguments typically indicate a poor choice of abstraction boundary. That's pretty much what I think is going on with heap_create_with_catalog(). I think maybe some refactoring is in order there, though I am not sure quite what. If we may have a hook on table creation next to the place currently we checks permission on the namespace being created, it is quite helpful to implement sepgsql. Makes sense. BTW, it seems to me SELECT INTO should also check insert permission on DAC level, because it become an incorrect assumption that owner of table has insert permission on its creation time. postgres=# ALTER DEFAULT PRIVILEGES FOR USER tak GRANT select ON TABLES TO tak; ALTER DEFAULT PRIVILEGES postgres=# \ddp Default access privileges Owner | Schema | Type | Access privileges ---++---+--- tak | | table | tak=r/tak (1 row) postgres=# SET SESSION AUTHORIZATION tak; SET postgres= SELECT * INTO t1 FROM (VALUES(1,'aaa'), (2,'bbb'), (3,'ccc')) AS v; SELECT 3 postgres= SELECT * FROM t1; column1 | column2 -+- 1 | aaa 2 | bbb 3 | ccc (3 rows) postgres= INSERT INTO t1 VALUES (4,'ddd'); ERROR: permission denied for relation t1 Oops! You could make the argument that there's no real security hole there, because the user could give himself those same privileges right back. You could also make the argument that a SELECT .. INTO is not the same as an INSERT and therefore INSERT privileges shouldn't be checked. I think there are valid arguments on both sides, but my main point is that we shouldn't have core do it one way and sepgsql do it the other way. That's going to be impossible to maintain and doesn't really make any logical sense anyway. -- 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] new compiler warnings
Tom Lane t...@sss.pgh.pa.us wrote: If the O_NONBLOCK flag is clear, a write request may cause the thread to block, but on normal completion it shall return nbyte. Note the last in particular. Short writes are specifically disallowed on pipes. OK, that's pretty definitive. I yield the point. -Kevin -- 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] new compiler warnings
Andrew Dunstan and...@dunslane.net wrote: My dim recollection is that Tom and I and maybe some others did tests on a bunch of platforms at the time we introduced the protocol to make sure it did work this way, since it's crucial to making sure we don't get interleaved log lines. Testing is good; I like testing. But I've seen people code to implementation details in such a way that things worked fine until the next release of a product, when the implementation changed. I was surprised to see Tom, who is normally such a stickler for doing such things correctly, apparently going the other way this time; but it turns out that he had noted a guarantee in the API that I'd missed. Mystery solved. Perhaps something in the comments would help people avoid making the same mistake I did. -Kevin -- 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) regression diffs on collate.linux.utf8 test
Peter Eisentraut pete...@gmx.net writes: On tis, 2011-10-18 at 01:07 -0700, Jeff Davis wrote: If I qualify it as tr_TR.UTF-8 it works. Perhaps I have something misconfigured on my system (Ubuntu 11.10)? I just installed: language-pack-de language-pack-tr language-pack-sv in an attempt to make the test work, and it works all except for that lc_time settng. I think the language-pack packages have nothing to do with it; they only supply translations. Possibly, things are set up so that only UTF-8 locales are installed by default. Since the collate.linux.utf8 requires a UTF-8 environment, it seems reasonable to use the tr_TR.UTF-8 locale for LC_TIME, instead of requiring an unrelated (ISO-8859-9) locale to be installed. So I think the change you propose is reasonable. As I said to Jeff earlier, I'd rather not embed assumptions about the spelling of encoding names into this test. So I don't want to do this just to get rid of an unexplained failure. I don't entirely believe the above theory, because it's not clear why Jeff's machine is behaving differently from mine. 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) regression diffs on collate.linux.utf8 test
On tis, 2011-10-18 at 15:21 -0400, Tom Lane wrote: Peter Eisentraut pete...@gmx.net writes: On tis, 2011-10-18 at 01:07 -0700, Jeff Davis wrote: If I qualify it as tr_TR.UTF-8 it works. Perhaps I have something misconfigured on my system (Ubuntu 11.10)? I just installed: language-pack-de language-pack-tr language-pack-sv in an attempt to make the test work, and it works all except for that lc_time settng. I think the language-pack packages have nothing to do with it; they only supply translations. Possibly, things are set up so that only UTF-8 locales are installed by default. Since the collate.linux.utf8 requires a UTF-8 environment, it seems reasonable to use the tr_TR.UTF-8 locale for LC_TIME, instead of requiring an unrelated (ISO-8859-9) locale to be installed. So I think the change you propose is reasonable. As I said to Jeff earlier, I'd rather not embed assumptions about the spelling of encoding names into this test. So I don't want to do this just to get rid of an unexplained failure. I don't entirely believe the above theory, because it's not clear why Jeff's machine is behaving differently from mine. Presumably because Jeff doesn't have that particular locale installed. locale -a would clarify that. glibc has always accepted variant locale name spellings such as UTF-8 vs utf8, so it's not a problem. -- 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] termination of backend waiting for sync rep generates a junk log message
On Mon, Oct 17, 2011 at 6:53 AM, Fujii Masao masao.fu...@gmail.com wrote: WARNING: canceling the wait for synchronous replication and terminating connection due to administrator command DETAIL: The transaction has already committed locally, but might not have been replicated to the standby. backend FATAL: terminating connection due to administrator command The above is the server log messages that I got when I did the procedure. backend is a junk. If a backend is terminated while it's waiting for synchronous replication, whereToSendOutput is set to DestNone. Then, whereToSendOutput=DestNone makes ReadCommand() call InteractiveBackend() which outputs backend . This junk message might mess up the server log monitoring tools. I think it should be removed. The simple fix is to change InteractiveBackend() so that it calls CHECK_FOR_INTERRUPTS() before it outputs backend . Thought? I'm tempted to say we should do that in PostgresMain() instead, maybe something like this: diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 976a832..9e5557c 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3769,6 +3769,8 @@ PostgresMain(int argc, char *argv[], const char *username) MemoryContextSwitchTo(MessageContext); MemoryContextResetAndDeleteChildren(MessageContext); + CHECK_FOR_INTERRUPTS(); + initStringInfo(input_message); /* -- 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] new compiler warnings
On Tue, Oct 18, 2011 at 3:03 PM, Kevin Grittner kevin.gritt...@wicourts.gov wrote: Andrew Dunstan and...@dunslane.net wrote: My dim recollection is that Tom and I and maybe some others did tests on a bunch of platforms at the time we introduced the protocol to make sure it did work this way, since it's crucial to making sure we don't get interleaved log lines. Testing is good; I like testing. But I've seen people code to implementation details in such a way that things worked fine until the next release of a product, when the implementation changed. I was surprised to see Tom, who is normally such a stickler for doing such things correctly, apparently going the other way this time; but it turns out that he had noted a guarantee in the API that I'd missed. Mystery solved. Perhaps something in the comments would help people avoid making the same mistake I did. Unfortunately, whether Tom's right or not, we still don't have a solution to the compiler warning. -- 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] new compiler warnings
Robert Haas robertmh...@gmail.com writes: Unfortunately, whether Tom's right or not, we still don't have a solution to the compiler warning. I don't actually see that warning on my Fedora 15 machine, with gcc version 4.6.1 20110908 (Red Hat 4.6.1-9) (GCC) What are the people who do see it using? (I do see the -Waddress ones.) 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] new compiler warnings
On tis, 2011-10-18 at 15:43 -0400, Tom Lane wrote: Robert Haas robertmh...@gmail.com writes: Unfortunately, whether Tom's right or not, we still don't have a solution to the compiler warning. I don't actually see that warning on my Fedora 15 machine, with gcc version 4.6.1 20110908 (Red Hat 4.6.1-9) (GCC) What are the people who do see it using? (I do see the -Waddress ones.) regards, tom lane You get the unused return value warnings with -D_FORTIFY_SOURCE=2, which has been the default on Ubuntu for years, and has been the default on Debian for a few weeks (if you have the hardening-wrapper package installed or running under dpkg-buildpackage). -- 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] new compiler warnings
Tom Lane t...@sss.pgh.pa.us wrote: What are the people who do see it using? Currently: gcc (Ubuntu/Linaro 4.5.2-8ubuntu4) 4.5.2 on Linux version 2.6.38-11-generic (buildd@allspice) (gcc version 4.5.2 (Ubuntu/Linaro 4.5.2-8ubuntu4) ) #50-Ubuntu SMP Mon Sep 12 21:17:25 UTC 2011 I've seen it on earlier versions of Ubuntu and Kubuntu, but not sure which exactly. As Peter says, it goes back for years. -Kevin -- 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] new compiler warnings
Robert Haas robertmh...@gmail.com wrote: Unfortunately, whether Tom's right or not, we still don't have a solution to the compiler warning. Would it be too weird to do something like this for each?: diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c index f0b3b1f..bea5489 100644 --- a/src/backend/utils/error/elog.c +++ b/src/backend/utils/error/elog.c @@ -1747,6 +1747,7 @@ write_eventlog(int level, const char *line, int len) static void write_console(const char *line, int len) { +ssize_t rc; #ifdef WIN32 /* @@ -1794,7 +1795,12 @@ write_console(const char *line, int len) */ #endif - write(fileno(stderr), line, len); + rc = write(fileno(stderr), line, len); + if (rc = 0 rc != len) + { + Assert(false); + return; + } } /* -Kevin -- 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] new compiler warnings
Peter Eisentraut pete...@gmx.net writes: On tis, 2011-10-18 at 15:43 -0400, Tom Lane wrote: I don't actually see that warning on my Fedora 15 machine, with gcc version 4.6.1 20110908 (Red Hat 4.6.1-9) (GCC) You get the unused return value warnings with -D_FORTIFY_SOURCE=2, which has been the default on Ubuntu for years, and has been the default on Debian for a few weeks (if you have the hardening-wrapper package installed or running under dpkg-buildpackage). Ah-hah. That's also the default on Red Hat platforms, *if* you are building RPMs, and now that I think of it, I do see this warning when building RPMs. Seems weird that they'd have set it up that way though rather than with a -W switch. 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] new compiler warnings
On Tue, Oct 18, 2011 at 4:13 PM, Tom Lane t...@sss.pgh.pa.us wrote: Peter Eisentraut pete...@gmx.net writes: On tis, 2011-10-18 at 15:43 -0400, Tom Lane wrote: I don't actually see that warning on my Fedora 15 machine, with gcc version 4.6.1 20110908 (Red Hat 4.6.1-9) (GCC) You get the unused return value warnings with -D_FORTIFY_SOURCE=2, which has been the default on Ubuntu for years, and has been the default on Debian for a few weeks (if you have the hardening-wrapper package installed or running under dpkg-buildpackage). Ah-hah. That's also the default on Red Hat platforms, *if* you are building RPMs, and now that I think of it, I do see this warning when building RPMs. Seems weird that they'd have set it up that way though rather than with a -W switch. Yeah, that's *quite* odd. -- 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] new compiler warnings
Kevin Grittner kevin.gritt...@wicourts.gov writes: Would it be too weird to do something like this for each?: - write(fileno(stderr), line, len); + rc = write(fileno(stderr), line, len); + if (rc = 0 rc != len) + { + Assert(false); + return; + } I don't think the assert is a good idea. If it ever did happen, that would promote the problem from corrupted data in the log to database crash. 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] new compiler warnings
On 18.10.2011 23:28, Tom Lane wrote: Kevin Grittnerkevin.gritt...@wicourts.gov writes: Would it be too weird to do something like this for each?: - write(fileno(stderr), line, len); + rc = write(fileno(stderr), line, len); + if (rc= 0 rc != len) + { + Assert(false); + return; + } I don't think the assert is a good idea. If it ever did happen, that would promote the problem from corrupted data in the log to database crash. I believe the idea is that if there's a platform that does that, we want to know. In production, you don't run with assertions enabled. It makes sense to me, or can we fall back to logging a warning to stderr or something? -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] new compiler warnings
Tom Lane t...@sss.pgh.pa.us wrote: I don't think the assert is a good idea. If it ever did happen, that would promote the problem from corrupted data in the log to database crash. ... on a --enable-cassert build. If we think it's even remotely possible that it could happen, maybe we should do the loop. That would change the current missing log information situation to interleaved log information. But if we think it would be better for data to be missing from the log than interleaved, the Assert could be removed and it still suppresses the error (at least on my machine). -Kevin -- 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] termination of backend waiting for sync rep generates a junk log message
Robert Haas robertmh...@gmail.com writes: On Mon, Oct 17, 2011 at 6:53 AM, Fujii Masao masao.fu...@gmail.com wrote: The simple fix is to change InteractiveBackend() so that it calls CHECK_FOR_INTERRUPTS() before it outputs backend . Thought? I'm tempted to say we should do that in PostgresMain() instead, maybe something like this: diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 976a832..9e5557c 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3769,6 +3769,8 @@ PostgresMain(int argc, char *argv[], const char *username) MemoryContextSwitchTo(MessageContext); MemoryContextResetAndDeleteChildren(MessageContext); + CHECK_FOR_INTERRUPTS(); + initStringInfo(input_message); /* I don't like putting a CHECK_FOR_INTERRUPTS there, because it's way too late to throw an error for the previous query. The real problem here is probably that we're overloading the meaning of whereToSendOutput. The reset of that variable during shutdown was only ever meant to prevent output from being sent to a no-longer-present client. It should *not* result in trying to read a query from stdin. Another question worth asking is how is it that we're getting to ReadCommand at all, if we have already determined that the client is gone. Fixing that with an additional CHECK_FOR_INTERRUPTS seems like a crock. 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] new compiler warnings
Kevin Grittner kevin.gritt...@wicourts.gov writes: Tom Lane t...@sss.pgh.pa.us wrote: I don't think the assert is a good idea. If it ever did happen, that would promote the problem from corrupted data in the log to database crash. ... on a --enable-cassert build. If we think it's even remotely possible that it could happen, maybe we should do the loop. That would change the current missing log information situation to interleaved log information. The logging protocol is hosed either way. But if we think it would be better for data to be missing from the log than interleaved, the Assert could be removed and it still suppresses the error (at least on my machine). As far as getting rid of the compiler warning is concerned, I find that the rc = write(...); (void) rc; suggestion works for me (gcc 4.6.1). I'm inclined to do that (and document why) rather than put in looping code that will not make anything better. 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] new compiler warnings
Tom Lane t...@sss.pgh.pa.us wrote: As far as getting rid of the compiler warning is concerned, I find that the rc = write(...); (void) rc; suggestion works for me (gcc 4.6.1). That silences the warning on my machine, too. -Kevin -- 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] new compiler warnings
I wrote: I think a large fraction of the -Waddress warnings are coming from this line in the heap_getattr macro: AssertMacro((tup) != NULL), \ Seems to me we could just lose that test and be no worse off, since the macro is surely gonna dump core anyway on a null pointer. Actually, all the ones in the backend are coming from that, so I went ahead and removed that. But some of the remaining -Waddress warnings are not so painless to get rid of. Ultimately we might have to add -Wno-address to the default CFLAGS. The remaining -Waddress warnings are all from applying PQExpBufferBroken to the address of a local variable. We could silence them along these lines: *** src/interfaces/libpq/pqexpbuffer.h.orig Thu Apr 28 16:07:00 2011 --- src/interfaces/libpq/pqexpbuffer.h Tue Oct 18 17:46:18 2011 *** *** 60,65 --- 60,73 ((str) == NULL || (str)-maxlen == 0) /* + * Same, but for use when using a static or local PQExpBufferData struct. + * For that, a null-pointer test is useless and may draw compiler warnings. + * + */ + #define PQExpBufferDataBroken(buf)\ + ((buf).maxlen == 0) + + /* * Initial size of the data buffer in a PQExpBuffer. * NB: this must be large enough to hold error messages that might * be returned by PQrequestCancel(). *** src/interfaces/libpq/fe-connect.c.orig Sun Sep 25 18:43:15 2011 --- src/interfaces/libpq/fe-connect.c Tue Oct 18 17:46:58 2011 *** *** 829,835 PQconninfoOption *connOptions; initPQExpBuffer(errorBuf); ! if (PQExpBufferBroken(errorBuf)) return NULL;/* out of memory already :-( */ connOptions = conninfo_parse(, errorBuf, true); termPQExpBuffer(errorBuf); --- 829,835 PQconninfoOption *connOptions; initPQExpBuffer(errorBuf); ! if (PQExpBufferDataBroken(errorBuf)) return NULL;/* out of memory already :-( */ connOptions = conninfo_parse(, errorBuf, true); termPQExpBuffer(errorBuf); *** *** 3967,3973 if (errmsg) *errmsg = NULL; /* default */ initPQExpBuffer(errorBuf); ! if (PQExpBufferBroken(errorBuf)) return NULL;/* out of memory already :-( */ connOptions = conninfo_parse(conninfo, errorBuf, false); if (connOptions == NULL errmsg) --- 3967,3973 if (errmsg) *errmsg = NULL; /* default */ initPQExpBuffer(errorBuf); ! if (PQExpBufferDataBroken(errorBuf)) return NULL;/* out of memory already :-( */ connOptions = conninfo_parse(conninfo, errorBuf, false); if (connOptions == NULL errmsg) (and one similar place in psql, which I did not bother to test). Any objections or better ideas? 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] new compiler warnings
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes: On 18.10.2011 23:28, Tom Lane wrote: I don't think the assert is a good idea. If it ever did happen, that would promote the problem from corrupted data in the log to database crash. I believe the idea is that if there's a platform that does that, we want to know. In production, you don't run with assertions enabled. It makes sense to me, or can we fall back to logging a warning to stderr or something? Unfortunately, the problem we're dealing with here is exactly that we can't write to stderr. So it's a bit hard to see what we can usefully do to report that we have a problem (short of crashing, which isn't a net improvement). In practice, the lack of field reports of corrupted postmaster logs seems to me to be adequate evidence that the code does work as intended. All we really need to do is shut gcc up about it. 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] Silent failure with invalid hba_file setting
Thom Brown t...@linux.com writes: I noticed that if the hba_file setting in the config is uncommented and set to a directory instead of the full path to the file, no error occurs when the service starts. When I try that, I get a boatload of errors ending with FATAL: could not load pg_hba.conf I suspect what happened to you is that the directory read like an empty file, so Postgres didn't see any error condition. I suppose we could add an fstat test to see if we'd opened something other than a regular file, but I'm not terribly excited about it. The problem with this is you cannot get into the database as it acts as if it did find the hba file but found it empty. Well, an actually empty pg_hba.conf file would have the same problem, and it's pretty hard to see any situation where it would be useful to start the postmaster and not let it accept any connections. Should we add a check to consider it an error if the file doesn't contain at least one HBA record? 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] Silent failure with invalid hba_file setting
On 19 October 2011 00:38, Tom Lane t...@sss.pgh.pa.us wrote: Thom Brown t...@linux.com writes: I noticed that if the hba_file setting in the config is uncommented and set to a directory instead of the full path to the file, no error occurs when the service starts. When I try that, I get a boatload of errors ending with FATAL: could not load pg_hba.conf I suspect what happened to you is that the directory read like an empty file, so Postgres didn't see any error condition. I suppose we could add an fstat test to see if we'd opened something other than a regular file, but I'm not terribly excited about it. The problem with this is you cannot get into the database as it acts as if it did find the hba file but found it empty. Well, an actually empty pg_hba.conf file would have the same problem, and it's pretty hard to see any situation where it would be useful to start the postmaster and not let it accept any connections. Should we add a check to consider it an error if the file doesn't contain at least one HBA record? That would solve both problems, so +1 for that. -- Thom Brown Twitter: @darkixion IRC (freenode): dark_ixion Registered Linux user: #516935 EnterpriseDB UK: 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] termination of backend waiting for sync rep generates a junk log message
On Tue, Oct 18, 2011 at 5:18 PM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: On Mon, Oct 17, 2011 at 6:53 AM, Fujii Masao masao.fu...@gmail.com wrote: The simple fix is to change InteractiveBackend() so that it calls CHECK_FOR_INTERRUPTS() before it outputs backend . Thought? I'm tempted to say we should do that in PostgresMain() instead, maybe something like this: diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c index 976a832..9e5557c 100644 --- a/src/backend/tcop/postgres.c +++ b/src/backend/tcop/postgres.c @@ -3769,6 +3769,8 @@ PostgresMain(int argc, char *argv[], const char *username) MemoryContextSwitchTo(MessageContext); MemoryContextResetAndDeleteChildren(MessageContext); + CHECK_FOR_INTERRUPTS(); + initStringInfo(input_message); /* I don't like putting a CHECK_FOR_INTERRUPTS there, because it's way too late to throw an error for the previous query. The real problem here is probably that we're overloading the meaning of whereToSendOutput. The reset of that variable during shutdown was only ever meant to prevent output from being sent to a no-longer-present client. It should *not* result in trying to read a query from stdin. I think you're right, but am not sure how to fix it. Another question worth asking is how is it that we're getting to ReadCommand at all, if we have already determined that the client is gone. Fixing that with an additional CHECK_FOR_INTERRUPTS seems like a crock. We haven't determined the client is gone; we're trying to close the connection unexpectedly. As the comment in SyncRepWaitForLSN explains: /* * If a wait for synchronous replication is pending, we can neither * acknowledge the commit nor raise ERROR or FATAL. The latter would * lead the client to believe that that the transaction aborted, which * is not true: it's already committed locally. The former is no good * either: the client has requested synchronous replication, and is * entitled to assume that an acknowledged commit is also replicated, * which might not be true. So in this case we issue a WARNING (which * some clients may be able to interpret) and shut off further output. * We do NOT reset ProcDiePending, so that the process will die after * the commit is cleaned up. */ -- 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] [v9.2] Fix Leaky View Problem
On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: Hi Robert, I'm a bit confusing about this sentence. If you can make this work, I think it could be a pretty sweet plannner optimization even apart from the implications for security views. Consider a query of this form: A LEFT JOIN B LEFT JOIN C where B is a view defined as: B1 JOIN B2 JOIN B3 LEFT JOIN B4 LEFT JOIN B5 Now let's suppose that from_collapse_limit/join_collapse_limit are set low enough that we decline to fold these subproblems together. If there happens to be a qual B.x = 1, where B.x is really B1.x, then the generated plan sucks, because it will basically lose the ability to filter B1 early, very possibly on, say, a unique index. Or at least a highly selective index. I tried to reproduce the scenario with enough small from/join_collapse_limit (typically 1), but it allows to push down qualifiers into the least scan plan. Hmm, you're right. LIMIT 10 prevents qual pushdown, but hitting from_collapse_limit/join_collapse_limit apparently doesn't. I could have sworn I've seen this work the other way, but I guess not. E.g) mytest=# SET from_collapse_limit = 1; mytest=# SET join_collapse_limit = 1; mytest=# CREATE VIEW B AS SELECT B1.* FROM B1,B2,B3 WHERE B1.x = B2.x AND B2.x = B3.x; mytest=# EXPLAIN SELECT * FROM A,B,C WHERE A.x=B.x AND B.x=C.x AND f_leak(B.y); This I wouldn't expect to have any effect anyway, because you're using the ad-hoc join syntax rather than explicit join syntax. But I tried it with explicit join syntax and it seems to only constrain the join order, not prevent qual pushdown. I agree with the following approach to tackle this problem in 100%. However, I'm unclear how from/join_collapse_limit affects to keep sub-queries unflatten. It seems to me it is determined based on the result of is_simple_subquery(). I think you are right, but I'm not sure it's right to hack is_simple_subquery() directly. Perhaps what we want to do is modify pull_up_subquery()? -- 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] loss of transactions in streaming replication
On Fri, Oct 14, 2011 at 7:51 AM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Oct 13, 2011 at 10:08 AM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Oct 12, 2011 at 10:29 PM, Robert Haas robertmh...@gmail.com wrote: On Wed, Oct 12, 2011 at 5:45 AM, Fujii Masao masao.fu...@gmail.com wrote: In 9.2dev and 9.1, when walreceiver detects an error while sending data to WAL stream, it always emits ERROR even if there are data available in the receive buffer. This might lead to loss of transactions because such remaining data are not received by walreceiver :( Won't it just reconnect? Yes if the master is running normally. OTOH, if the master is not running (i.e., failover case), the standby cannot receive again the data which it failed to receive. I found this issue when I shut down the master. When the master shuts down, it sends the shutdown checkpoint record, but I found that the standby failed to receive it. Patch attached. The patch changes walreceiver so that it doesn't emit ERROR just yet even if it fails to send data to WAL stream. Then, after all available data have been received and flushed to the disk, it emits ERROR. If the patch is OK, it should be backported to v9.1. Convince me. :-) My reading of the situation is that you're talking about a problem that will only occur if, while the master is in the process of shutting down, a network error occurs. I am not sure it's a good idea to convolute the code to handle that case, because (1) there are going to be many similar situations where nothing within our power is sufficient to prevent WAL from failing to make it to the standby and (2) for this marginal improvement, you're giving up including PQerrorMessage(streamConn) in the error message that ultimately gets omitted, which seems like a substantial regression as far as debuggability is concerned. Even if we do decide that we want the change in behavior, I see no compelling reason to back-patch it. Stable releases are supposed to be stable, not change behavior because we thought of something we like better than what we originally released. -- 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] Online base backup from the hot-standby
+ /* +* The backend writes WAL of FPW at checkpoint. However, The backend do +* not need to write WAL of FPW at checkpoint shutdown because it +* performs when startup finishes. +*/ + XLogReportParameters(REPORT_ON_BACKEND); I'm still unclear why that WAL doesn't need to be written at shutdown checkpoint. Anyway, the first sentence in the above comments is not right. Not a backend but a bgwriter writes that WAL at checkpoint. The second also seems not to be right. It implies that a shutdown checkpoint is performed only at end of startup. But it may be done when smart or fast shutdown is requested. Okay. I change to the following messages. /* * The bgwriter writes WAL of FPW at checkpoint. But does not at shutdown. * Because XLogReportParameters() is always called at the end of startup * process, it does not need to be called at shutdown. */ In addition, I change macro name. REPORT_ON_BACKEND - REPORT_ON_BGWRITER I have updated as above-comment. Please check this. Regards. Jun Ishizuka NTT Software Corporation TEL:045-317-7018 E-Mail: ishizuka@po.ntts.co.jp standby_online_backup_09base-06fpw.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] [v9.2] Fix Leaky View Problem
Robert Haas robertmh...@gmail.com writes: On Sun, Oct 16, 2011 at 4:46 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote: I tried to reproduce the scenario with enough small from/join_collapse_limit (typically 1), but it allows to push down qualifiers into the least scan plan. Hmm, you're right. LIMIT 10 prevents qual pushdown, but hitting from_collapse_limit/join_collapse_limit apparently doesn't. I could have sworn I've seen this work the other way, but I guess not. No, the collapse_limit variables are entirely unrelated to subquery flattening, or to qual pushdown for that matter. They only restrict the number of join paths we consider. And we will attempt to push down quals into an unflattened subquery, too, if it looks safe. See subquery_is_pushdown_safe, qual_is_pushdown_safe, etc in allpaths.c. 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] termination of backend waiting for sync rep generates a junk log message
Robert Haas robertmh...@gmail.com writes: On Tue, Oct 18, 2011 at 5:18 PM, Tom Lane t...@sss.pgh.pa.us wrote: Another question worth asking is how is it that we're getting to ReadCommand at all, if we have already determined that the client is gone. Fixing that with an additional CHECK_FOR_INTERRUPTS seems like a crock. We haven't determined the client is gone; we're trying to close the connection unexpectedly. As the comment in SyncRepWaitForLSN explains: /* * If a wait for synchronous replication is pending, we can neither * acknowledge the commit nor raise ERROR or FATAL. The latter would * lead the client to believe that that the transaction aborted, which * is not true: it's already committed locally. The former is no good * either: the client has requested synchronous replication, and is * entitled to assume that an acknowledged commit is also replicated, * which might not be true. So in this case we issue a WARNING (which * some clients may be able to interpret) and shut off further output. * We do NOT reset ProcDiePending, so that the process will die after * the commit is cleaned up. */ Hmm. Maybe the real answer is this code is abusing whereToSendOutput (and about six other things besides). I'll try to think of a better solution, but not tonight. One thing worth asking is why we're willing to violate half a dozen different coding rules if we see ProcDiePending, yet we're perfectly happy to rely on the client understanding a WARNING for the QueryCancelPending case. Another is whether this whole function isn't complete BS in the first place, since it appears to be coded on the obviously-false assumption that nothing it calls can throw elog(ERROR) --- and of course, if any of those functions do throw ERROR, all the argumentation here goes out the window. 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] new compiler warnings
On tis, 2011-10-18 at 16:13 -0400, Tom Lane wrote: Peter Eisentraut pete...@gmx.net writes: On tis, 2011-10-18 at 15:43 -0400, Tom Lane wrote: I don't actually see that warning on my Fedora 15 machine, with gcc version 4.6.1 20110908 (Red Hat 4.6.1-9) (GCC) You get the unused return value warnings with -D_FORTIFY_SOURCE=2, which has been the default on Ubuntu for years, and has been the default on Debian for a few weeks (if you have the hardening-wrapper package installed or running under dpkg-buildpackage). Ah-hah. That's also the default on Red Hat platforms, *if* you are building RPMs, and now that I think of it, I do see this warning when building RPMs. Seems weird that they'd have set it up that way though rather than with a -W switch. There is a switch -Wunused-result, but it's on by default. -- 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] Silent failure with invalid hba_file setting
On tis, 2011-10-18 at 18:38 -0400, Tom Lane wrote: The problem with this is you cannot get into the database as it acts as if it did find the hba file but found it empty. Well, an actually empty pg_hba.conf file would have the same problem, and it's pretty hard to see any situation where it would be useful to start the postmaster and not let it accept any connections. Should we add a check to consider it an error if the file doesn't contain at least one HBA record? If you try to connect and it doesn't find a record, it will tell you. I wouldn't add extra special checks for that. It might not be completely unreasonable to have a standby that no one can connect to, for example. -- 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] termination of backend waiting for sync rep generates a junk log message
On Tue, Oct 18, 2011 at 11:27 PM, Tom Lane t...@sss.pgh.pa.us wrote: One thing worth asking is why we're willing to violate half a dozen different coding rules if we see ProcDiePending, yet we're perfectly happy to rely on the client understanding a WARNING for the QueryCancelPending case. Another is whether this whole function isn't complete BS in the first place, since it appears to be coded on the obviously-false assumption that nothing it calls can throw elog(ERROR) --- and of course, if any of those functions do throw ERROR, all the argumentation here goes out the window. Well, there is a general problem that anything which throws an ERROR too late in the commit path is Evil; and sync rep makes that worse to the extent that it adds more stuff late in the commit path, but it didn't invent the problem. What it did do is add stuff late in the commit path that can block for a potentially unbounded period of time, and I don't see that there are any solutions to that problem that aren't somewhat grotty. -- 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] Silent failure with invalid hba_file setting
Peter Eisentraut pete...@gmx.net writes: On tis, 2011-10-18 at 18:38 -0400, Tom Lane wrote: Well, an actually empty pg_hba.conf file would have the same problem, and it's pretty hard to see any situation where it would be useful to start the postmaster and not let it accept any connections. Should we add a check to consider it an error if the file doesn't contain at least one HBA record? If you try to connect and it doesn't find a record, it will tell you. Yeah, but the damage is already done. I see the main practical benefit of this being to prevent accidental loading of a trashed pg_hba file. I wouldn't add extra special checks for that. It might not be completely unreasonable to have a standby that no one can connect to, for example. Well, you couldn't monitor its state then, so I don't find that example very convincing. But if you were intent on having that, you could easily set up a pg_hba file containing only reject entries. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] pg_ctl restart - behaviour based on wrong instance
On Tue, Oct 18, 2011 at 12:18 PM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Oct 18, 2011 at 11:02 PM, Magnus Hagander mag...@hagander.net wrote: Ok, fixed and applied. You seem to have forgot to change protocol.sgml. Patch attached. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Adding CORRESPONDING to Set Operations
On Sun, Oct 16, 2011 at 7:46 PM, Kerem Kat kerem...@gmail.com wrote: CORRESPONDING clause take 2 You should probably read this: http://wiki.postgresql.org/wiki/Submitting_a_Patch And add your patch here: https://commitfest.postgresql.org/action/commitfest_view/open -- 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] termination of backend waiting for sync rep generates a junk log message
Robert Haas robertmh...@gmail.com writes: On Tue, Oct 18, 2011 at 11:27 PM, Tom Lane t...@sss.pgh.pa.us wrote: One thing worth asking is why we're willing to violate half a dozen different coding rules if we see ProcDiePending, yet we're perfectly happy to rely on the client understanding a WARNING for the QueryCancelPending case. Another is whether this whole function isn't complete BS in the first place, since it appears to be coded on the obviously-false assumption that nothing it calls can throw elog(ERROR) --- and of course, if any of those functions do throw ERROR, all the argumentation here goes out the window. Well, there is a general problem that anything which throws an ERROR too late in the commit path is Evil; and sync rep makes that worse to the extent that it adds more stuff late in the commit path, but it didn't invent the problem. What it did do is add stuff late in the commit path that can block for a potentially unbounded period of time, and I don't see that there are any solutions to that problem that aren't somewhat grotty. No doubt, but fantasizing about what you are or are not controlling doesn't help ... and AFAICT this code is mostly fantasy. Anyway, I don't have a better proposal right offhand; will think about it. 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) regression diffs on collate.linux.utf8 test
On Tue, 2011-10-18 at 22:25 +0300, Peter Eisentraut wrote: Presumably because Jeff doesn't have that particular locale installed. locale -a would clarify that. $ locale -a |grep -i tr tr_CY.utf8 tr_TR.utf8 So, yes, I only have the UTF8 version. I didn't realize they were different -- do you happen to know what package I need for just plain tr_TR? Regards, Jeff Davis -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] (patch) regression diffs on collate.linux.utf8 test
Jeff Davis pg...@j-davis.com writes: On Tue, 2011-10-18 at 22:25 +0300, Peter Eisentraut wrote: Presumably because Jeff doesn't have that particular locale installed. locale -a would clarify that. $ locale -a |grep -i tr tr_CY.utf8 tr_TR.utf8 So, yes, I only have the UTF8 version. Wow, that's interesting. Digging around on my Fedora box, I can't find any suggestion that it's even possible to subdivide the locale settings like that. I only see one source file for tr_TR --- that's /usr/share/i18n/locales/tr_TR --- and it looks like all the stuff under /usr/share/i18n/locales/ is compiled into one big run-time file /usr/lib/locale/locale-archive. 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] Silent failure with invalid hba_file setting
On Oct 19, 2011 6:21 AM, Tom Lane t...@sss.pgh.pa.us wrote: Peter Eisentraut pete...@gmx.net writes: On tis, 2011-10-18 at 18:38 -0400, Tom Lane wrote: Well, an actually empty pg_hba.conf file would have the same problem, and it's pretty hard to see any situation where it would be useful to start the postmaster and not let it accept any connections. Should we add a check to consider it an error if the file doesn't contain at least one HBA record? If you try to connect and it doesn't find a record, it will tell you. Yeah, but the damage is already done. I see the main practical benefit of this being to prevent accidental loading of a trashed pg_hba file. Yeah, definitely. It's very much a pita when you accidentally do that with a syntax error on 8.4, %. So while I haven't actually managed to hit his specific problem myself, +1 for this approach. I wouldn't add extra special checks for that. It might not be completely unreasonable to have a standby that no one can connect to, for example. Well, you couldn't monitor its state then, so I don't find that example very convincing. But if you were intent on having that, you could easily set up a pg_hba file containing only reject entries. Yeah, seems reasonable to put a (very) small amount of extra work in the path of a very uncommon scenario in order to protect users in the common one... /Magnus