Re: [PATCHES] HOT latest patch - version 8
On 7/15/07, Heikki Linnakangas [EMAIL PROTECTED] wrote: Actually storing InvalidOffsetNumber in lp_off is a bit bogus in the first place since lp_off is unsigned, and InvalidOffsetNumber is -1, so I fixed that as well. I see InvalidOffsetNumber as 0 in off.h:26 #define InvalidOffsetNumber ((OffsetNumber) 0) So I think we should be OK to use that to indicate redirect-dead pointers. Thanks, Pavan -- Pavan Deolasee EnterpriseDB http://www.enterprisedb.com
Re: [PATCHES] COPYable logs
Andrew Dunstan wrote: Here is my latest version of this patch. The good news is that it now seems to work on Windows. Please review carefully (esp Magnus, Dave, Tom). Hi Andrew, I've eyeballed the code quite thoroughly and given it a whirl under VC++. The only problem I found was that the assert on line 794 of syslogger.c in syslogger_parseArgs should be : Assert(argc == 5); not Assert(argc == 6); I'm not sure I like the file handling however - as it stands we get a logfile (eg postgresql-2007-07-23_135007.log) which never gets written to except by any pre-logger startup problems, and a corresponding csv file. I can see why it's done this way, but it does seem somewhat messy (I have a bunch of zero byte logfiles for each csv file) - can we clean that up somehow, perhaps by only creating the initial logfile at startup if we're not in CSV mode? I'm also not crazy about the way the csv filename is generated by appending .csv to the logfilename. This leads to a double file extension by default which is largely considered to be evil on Windows (it's a common method for 'hiding' viruses). Perhaps we could replace the .log with .csv instead of appending it? If the name doesn't end in .log, then append it (non-default config == users choice). Regards, Dave. ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] [HACKERS] Oops in fe-auth.c
On Mon, Jul 23, 2007 at 03:36:21PM +0200, Magnus Hagander wrote:
I've been debugging some really weird crashes in libpq on win32, and I
think I've finally found the reason for the heap corruption that shows up
in msvc debug mode.
When run in debug mode, the runtime for msvc will *zero-pad the entire
buffer* in a strncpy() call. This in itself is not bad (just slow), but it
shows a rather bad bug in libpq.
In a bunch of places in fe-auth.c, we do:
strncpy(PQerrormsg, libpq_gettext(out of memory\n), PQERRORMSG_LENGTH);
Except when calling it, the size of the buffer is 256 bytes. But
PQERRORMSG_LENGTH is 1024.
Naturally, this causes a heap corruption. It doesn't happen in production,
because the string length fits as long as there is no padding.
One way to get around this on win32 is to just use snprintf() instead of
strncpy(), since it doesn't pad. But that's just hiding the underlying
problem, so I think that's a really bad fix.
I assume the comment in the header:
* NOTE: the error message strings returned by this module must not
* exceed INITIAL_EXPBUFFER_SIZE (currently 256 bytes).
refers to this, but it's hard to guarantee that from the code since it's
translated strings.
I see a comment in fe-connect.c that has
* XXX fe-auth.c has not been fixed to support PQExpBuffers,
Given this, I'll go ahead and fix fe-connect to support PQExpBuffers,
unless there are any objections. Also, is this something we shuold
backpatch - or just ignore since we've had no actual reports of it in the
field?
Actually coding up a patch for that was just a bunch of simple
search/replace ops. Attached is one that appears to work fine for me.
Actually coding up a patch for that was just a bunch of simple
search/replace ops. Attached is one that appears to work fine for me.
Was there any reason why this wasn't done before, or just nobody had the
time? If there was a reason, please let me know what it was :-) Otherwise
I'll apply this patch to fix it.
(Question about backpatch remains)
//Magnus
Index: src/interfaces/libpq/fe-auth.c
===
RCS file: /projects/cvsroot/pgsql/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.129
diff -c -r1.129 fe-auth.c
*** src/interfaces/libpq/fe-auth.c 23 Jul 2007 10:57:36 - 1.129
--- src/interfaces/libpq/fe-auth.c 23 Jul 2007 14:02:28 -
***
*** 6,14
* Portions Copyright (c) 1996-2007, PostgreSQL Global Development Group
* Portions Copyright (c) 1994, Regents of the University of California
*
- * NOTE: the error message strings returned by this module must not
- * exceed INITIAL_EXPBUFFER_SIZE (currently 256 bytes).
- *
* IDENTIFICATION
* $PostgreSQL: pgsql/src/interfaces/libpq/fe-auth.c,v 1.129 2007/07/23
10:57:36 mha Exp $
*
--- 6,11
***
*** 131,137
static int
! pg_krb5_init(char *PQerrormsg, struct krb5_info * info)
{
krb5_error_code retval;
--- 128,134
static int
! pg_krb5_init(PQExpBuffer errorMessage, struct krb5_info * info)
{
krb5_error_code retval;
***
*** 141,147
retval = krb5_init_context((info-pg_krb5_context));
if (retval)
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
pg_krb5_init: krb5_init_context: %s\n,
error_message(retval));
return STATUS_ERROR;
--- 138,144
retval = krb5_init_context((info-pg_krb5_context));
if (retval)
{
! printfPQExpBuffer(errorMessage,
pg_krb5_init: krb5_init_context: %s\n,
error_message(retval));
return STATUS_ERROR;
***
*** 150,156
retval = krb5_cc_default(info-pg_krb5_context,
(info-pg_krb5_ccache));
if (retval)
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
pg_krb5_init: krb5_cc_default: %s\n,
error_message(retval));
krb5_free_context(info-pg_krb5_context);
--- 147,153
retval = krb5_cc_default(info-pg_krb5_context,
(info-pg_krb5_ccache));
if (retval)
{
! printfPQExpBuffer(errorMessage,
pg_krb5_init: krb5_cc_default: %s\n,
error_message(retval));
krb5_free_context(info-pg_krb5_context);
***
*** 161,167
(info-pg_krb5_client));
if (retval)
{
! snprintf(PQerrormsg, PQERRORMSG_LENGTH,
pg_krb5_init: krb5_cc_get_principal: %s\n,
error_message(retval));
krb5_cc_close(info-pg_krb5_context,
Re: [PATCHES] COPYable logs
Dave Page wrote: Andrew Dunstan wrote: Here is my latest version of this patch. The good news is that it now seems to work on Windows. Please review carefully (esp Magnus, Dave, Tom). Hi Andrew, I've eyeballed the code quite thoroughly and given it a whirl under VC++. The only problem I found was that the assert on line 794 of syslogger.c in syslogger_parseArgs should be : Assert(argc == 5); not Assert(argc == 6); I'm not sure I like the file handling however - as it stands we get a logfile (eg postgresql-2007-07-23_135007.log) which never gets written to except by any pre-logger startup problems, and a corresponding csv file. I can see why it's done this way, but it does seem somewhat messy (I have a bunch of zero byte logfiles for each csv file) - can we clean that up somehow, perhaps by only creating the initial logfile at startup if we're not in CSV mode? I'm also not crazy about the way the csv filename is generated by appending .csv to the logfilename. This leads to a double file extension by default which is largely considered to be evil on Windows (it's a common method for 'hiding' viruses). Perhaps we could replace the .log with .csv instead of appending it? If the name doesn't end in .log, then append it (non-default config == users choice). Thanks - I will attend to these items. There's also something screwy going on - if we have redirect_stderr = off and log_destination = 'csvlog' then we get tied up in knots and the postmaster won't die - I had to use kill -9. cheers andrew ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
[PATCHES] tsearch core path, v0.58
http://www.sigaev.ru/misc/tsearch_core-0.58.gz Changes since 0.52 version: 1) Introduce dictionary's template which contains only methods of dictionary and can be managed only by superuser. CREATE TEXT SEARCH DICTIONARY dictname TEMPLATE dicttmplname [OPTION opt_text ] ; CREATE TEXT SEARCH DICTIONARY TEMPLATE dicttmplname LEXIZE lexize_function [INIT init_function ] ; DROP TEXT SEARCH DICTIONARY TEMPLATE [IF EXISTS] dicttmplname [CASCADE] ALTER TEXT SEARCH DICTIONARY TEMPLATE dicttmplname RENAME TO newname; psql has \dFt command operated templates 2) parser and dictionary template could be managed only by superuser (due to security reasons pointed by Tom). So, they don't have owner columns and removed ALTER .. PARSER .. OWNER TO command 4) As Bruce suggests, GUC variable tsearch_conf_name is renamed to default_text_search_config and trigger tsearch is renamed to tsvector_update_trigger 5) remove cfglocale and cfgdefault columns in configuration. So, CREATE/ALTER .. CONFIGURATION hasn't AS DEFAULT and LOCALE options. Instead of that initdb tries to find suitable configuration name for selected locale. Or it uses -T, --text-search-config=CFG switch. 6) pg_dump, psql are changed accordingly. -- Teodor Sigaev E-mail: [EMAIL PROTECTED] WWW: http://www.sigaev.ru/ ---(end of broadcast)--- TIP 1: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] Oops in fe-auth.c
Magnus Hagander [EMAIL PROTECTED] writes: Actually coding up a patch for that was just a bunch of simple search/replace ops. Attached is one that appears to work fine for me. Was there any reason why this wasn't done before, or just nobody had the time? If there was a reason, please let me know what it was :-) AFAIR nobody got round to it because it hadn't seemed important. (Question about backpatch remains) I'd vote against backpatching. The appropriate fix for back branches is probably just to reduce the strncpy and snprintf arguments to INITIAL_EXPBUFFER_SIZE, ie, make the code do what that header comment says it should do. Style point: in the places where you've chosen to pass the whole PGconn, you should remove any separate arguments that are actually just PGconn fields; eg for pg_krb5_sendauth it looks like sock and servicename are now redundant. Otherwise there are risks of programmer confusion, and maybe even wrong code generation, due to aliasing. It would be more consistent to pass PGconn around to all of these functions instead of trying to have them have just partial views of it, but I dunno if you want to engage in purely cosmetic changes. regards, tom lane ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Updated bitmap index patch
Simon Riggs wrote: Alvaro, As you note above there is some linkage between bit map indexes and clustered indexes, so it seems like we'll either get both or neither. I notice the GIT patch is being listed as under review by Alexey and yourself. Are you actively working on this, or is anyone else planning on working on this review? Sorry, no, I'm currently stuck elsewhere. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [HACKERS] Oops in fe-auth.c
Tom Lane wrote: Magnus Hagander [EMAIL PROTECTED] writes: Actually coding up a patch for that was just a bunch of simple search/replace ops. Attached is one that appears to work fine for me. Was there any reason why this wasn't done before, or just nobody had the time? If there was a reason, please let me know what it was :-) AFAIR nobody got round to it because it hadn't seemed important. Ok. I actually managed to provoke a GSSAPI error that got cut off at 256 characters in testing. Which is kind of amazing in itself, but... (Question about backpatch remains) I'd vote against backpatching. The appropriate fix for back branches is probably just to reduce the strncpy and snprintf arguments to INITIAL_EXPBUFFER_SIZE, ie, make the code do what that header comment says it should do. Right. See other mail as well. Style point: in the places where you've chosen to pass the whole PGconn, you should remove any separate arguments that are actually just PGconn fields; eg for pg_krb5_sendauth it looks like sock and servicename are now redundant. Otherwise there are risks of programmer confusion, and maybe even wrong code generation, due to aliasing. It would be more consistent to pass PGconn around to all of these functions instead of trying to have them have just partial views of it, but I dunno if you want to engage in purely cosmetic changes. I'll go ahead and do that now, while I'm whacking the code around. //Magnus ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] COPYable logs
Andrew Dunstan [EMAIL PROTECTED] writes:
Here is my latest version of this patch. The good news is that it now
seems to work on Windows. Please review carefully (esp Magnus, Dave, Tom).
The GUC arrangements for this patch are utterly broken. The reason for
the separation between Redirect_stderr and Log_destination is that
Log_destination is allowed to change at SIGHUP, whereas Redirect_stderr
can't change after startup (since it'd be too late to create the pipe).
The exact same problem applies to the CSV pipe, and therefore you can't
use (Log_destination LOG_DESTINATION_CSVLOG) to control startup-time
decisions.
There are two approaches we could take to fixing this:
1. Redirect_stderr is the start-time flag for both features, ie, if it's
set we always create both pipes and start the syslogger, but
Log_destination controls what actually gets used. (Possibly
Redirect_stderr should be renamed if we go this way.)
2. Invent a new PGC_POSTMASTER GUC variable that says whether to create
the CSV pipe; then the syslogger process is started if either flag is
set.
#1 seems simpler but it might be too restrictive. (In particular I'm
not too sure what happens to the syslogger's own stderr.)
Also, given the creation of the chunking protocol for the stderr pipe,
I would argue that there is no longer any value in actually having two
pipes: you could include a flag in each chunk to say which protocol it
belongs to. (This wouldn't even cost any extra bits, since is_last
could be replaced with a four-valued field.) The patch could be
simplified a lot if we got rid of the second pipe.
! if (Redirect_stderr)
! {
! unsigned int tid;
! int logtype = STDERR_LOGFILE;
+ InitializeCriticalSection(sysfileSection);
+ stderrThreadHandle = (HANDLE) _beginthreadex(0, 0, pipeThread,
+ logtype, 0,
tid);
+ }
That can't possibly be a safe programming technique, can it? The
local variable logtype could disappear from the stack long before
pipeThread gets a chance to read it. I think you'd have to make
it static to make this reliable. However, if you adopt suggestion
above then this code goes away anyway.
extern bool redirection_done;
+ char timestamp[128];
+
Why in the world is this made a global variable? AFAICS it should be
local in get_timestamp. Or at least static.
*** src/include/miscadmin.h 16 Apr 2007 18:29:56 - 1.194
--- src/include/miscadmin.h 22 Jul 2007 22:18:45 -
***
*** 132,137
--- 132,138
extern int MaxConnections;
extern DLLIMPORT int MyProcPid;
+ extern DLLIMPORT time_t MyStartTime;
extern DLLIMPORT struct Port *MyProcPort;
extern long MyCancelKey;
Does this even compile? I don't think we necessarily include time.h
before this.
+ #define LOG_BUFFER_SIZE 32768
+
+ #define STDERR_LOGFILE 1
+ #define CSV_LOGFILE 2
Doesn't seem like LOG_BUFFER_SIZE should be exported to the world.
Does seem like the other two require a comment. Should they be an
enum type?
Another thing that needs to be looked at carefully is how much memory
write_csvlog() eats. I'm a little bit concerned about whether it will
result in infinite recursion when our backs are against the wall and
we only have the original 8K in ErrorContext to work in. (We could
increase that figure if need be, but we need to know by how much.)
As a general style note, the patch seems to add and remove blank
lines in a most arbitrary and inconsistent way. Please try to
clean that up. pgindent would fix some of that, but it's not very
good about removing blank lines that shouldn't be there.
regards, tom lane
---(end of broadcast)---
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [PATCHES] COPYable logs
Tom Lane wrote: Andrew Dunstan [EMAIL PROTECTED] writes: Here is my latest version of this patch. The good news is that it now seems to work on Windows. Please review carefully (esp Magnus, Dave, Tom). The GUC arrangements for this patch are utterly broken. As always I submit to your gentle reproofs. I will try to attend to all this next weekend. The reason for the separation between Redirect_stderr and Log_destination is that Log_destination is allowed to change at SIGHUP, whereas Redirect_stderr can't change after startup (since it'd be too late to create the pipe). The exact same problem applies to the CSV pipe, and therefore you can't use (Log_destination LOG_DESTINATION_CSVLOG) to control startup-time decisions. There are two approaches we could take to fixing this: 1. Redirect_stderr is the start-time flag for both features, ie, if it's set we always create both pipes and start the syslogger, but Log_destination controls what actually gets used. (Possibly Redirect_stderr should be renamed if we go this way.) 2. Invent a new PGC_POSTMASTER GUC variable that says whether to create the CSV pipe; then the syslogger process is started if either flag is set. #1 seems simpler but it might be too restrictive. (In particular I'm not too sure what happens to the syslogger's own stderr.) #1 seems more intuitive to me, although it should be renamed, with a caveat that we shouldn't allow csvlog as a destination if it's off. Syslogger's own stderr isn't redirected, as we have discussed previously. Also, given the creation of the chunking protocol for the stderr pipe, I would argue that there is no longer any value in actually having two pipes: you could include a flag in each chunk to say which protocol it belongs to. (This wouldn't even cost any extra bits, since is_last could be replaced with a four-valued field.) The patch could be simplified a lot if we got rid of the second pipe. yeah, probably. Another thing that needs to be looked at carefully is how much memory write_csvlog() eats. I'm a little bit concerned about whether it will result in infinite recursion when our backs are against the wall and we only have the original 8K in ErrorContext to work in. (We could increase that figure if need be, but we need to know by how much.) well, it will be a lot less than it was originally ... I guess the major extra cost comes from having to CSV escape the error message. Using 8k for one copy let alone two seems way too small. Not sure what we should do about it. As a general style note, the patch seems to add and remove blank lines in a most arbitrary and inconsistent way. Please try to clean that up. pgindent would fix some of that, but it's not very good about removing blank lines that shouldn't be there. sure. cheers andrew ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Async Commit, v21 (now: v22)
Simon Riggs [EMAIL PROTECTED] writes: Thanks for reading. Updated version in new patch. What was the reasoning for basing walwriter.c on autovacuum (which needs to be able to execute transactions) rather than bgwriter (which does not)? The shutdown logic in particular seems all wrong; you can't have a process connected to shared memory that is going to outlive the postmaster. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Async Commit, v21 (now: v22)
On Mon, 2007-07-23 at 18:59 -0400, Tom Lane wrote: Simon Riggs [EMAIL PROTECTED] writes: Thanks for reading. Updated version in new patch. What was the reasoning for basing walwriter.c on autovacuum (which needs to be able to execute transactions) rather than bgwriter (which does not)? Writing WAL means we have to have xlog access and therefore shared memory access. Don't really need the ability to execute transactions though, tis true, but I wasn't aware there was a distinction. The shutdown logic in particular seems all wrong; you can't have a process connected to shared memory that is going to outlive the postmaster. It seemed to work cleanly when I tested it initially, but I'll take another look tomorrow. By version 23 I was going code-blind. Autovac is the most clean implementation of a special process, so seemed like a good prototype. I'd thought I'd combed out any pointless code though. -- Simon Riggs EnterpriseDB http://www.enterprisedb.com ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] Async Commit, v21 (now: v22)
Simon Riggs wrote: Autovac is the most clean implementation of a special process, so seemed like a good prototype. I'd thought I'd combed out any pointless code though. What, you mean there's pointless code in autovac? Hey, be sure to let me know about it! -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Async Commit, v21 (now: v22)
Simon Riggs wrote: Here's v23, including all suggested changes, plus some reworking of the transaction APIs to reduce the footprint of the patch. What's the thing about doing the flush twice in a couple of comments in calls to XLogBackgroundFlush? Are they just leftover comments from older code? -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] Async Commit, v21 (now: v22)
Simon Riggs [EMAIL PROTECTED] writes: On Mon, 2007-07-23 at 18:59 -0400, Tom Lane wrote: The shutdown logic in particular seems all wrong; you can't have a process connected to shared memory that is going to outlive the postmaster. It seemed to work cleanly when I tested it initially, but I'll take another look tomorrow. By version 23 I was going code-blind. Leave it be for the moment --- I'm working on it now so it'd just be duplicated effort. I'm inclined though to rebase at least the signal handling on bgwriter.c; don't like different processes using different signal interpretations unless there's a good reason for it. I came across another point worthy of mention: as given, the patch turns XLogWrite's flexible write logic into dead code, because there are no callers that pass flexible = true. We could rip that out, but it seems to me there's still some value to it under high load conditions (high load meaning we fill multiple wal pages per wal_writer_delay). What I'm inclined to do is modify XLogBackgroundFlush so that it uses flexible = true when it's flushing whole pages. The downside to that is that it could take as many as three walwriter cycles, instead of two, to guararantee an async commit is down to disk: * first write/flush stops at buffer wraparound point * second one stops at last complete page * third finally is certain to write any remaining commit record This seems like no big problem to me, but does anyone want to object? (BTW, in case you can't tell from the drift of my questions, I've separated the patch into add background wal writer and add async commit, and am working on the first half.) regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Async Commit, v21 (now: v22)
Alvaro Herrera [EMAIL PROTECTED] writes: What's the thing about doing the flush twice in a couple of comments in calls to XLogBackgroundFlush? Are they just leftover comments from older code? I was wondering that too --- they looked like obsolete comments to me. My current thinking BTW is that trying to make XLogBackgroundFlush serve two purposes is counterproductive. It should be dedicated to use by the walwriter only, and the checkpoint case should simply read the async commit pointer and call regular XLogFlush with it. regards, tom lane ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Async Commit, v21 (now: v22)
I wrote: (BTW, in case you can't tell from the drift of my questions, I've separated the patch into add background wal writer and add async commit, and am working on the first half.) I've committed the first half of that. Something that still needs investigation is what the default wal_writer_delay should be. I left it at 200ms as submitted, but in some crude testing here (just running the regression tests and strace'ing the walwriter) it seemed that I had to crank it down to 10ms before the walwriter was really doing the majority of the wal-flushing work. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
