Re: [PATCHES] Revised patch for fixing archiver shutdown behavior
Tom Lane wrote: Hence, attached revised patch ... Looks good. Something I'm still wondering is about the archiver/logger combination. What happens if a postmaster is stopped by the user and the archiver is still running, and the user starts a new postmaster? This would launch a new archiver and logger; and there are now two loggers possibly writing to the same files, and truncated log lines could occur. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] reference problem of manifest.(win32.mak of libpq.dll)
On Wed, Jan 09, 2008 at 04:01:20PM +0900, Hiroshi Saito wrote: Hi Magnus, and Dave. It seems that it is different in nmake although the initial value of VisualStudio is embedding. Then, It sees a reference problem by the dll independent. Therefore, embedding considers like an ideal. Please take this into consideration. + !IF $(_NMAKE_VER) != 6.00.9782.0 I don't think that's safe. We need to do a range check. Perhaps check if vesion is = 7.0 or something? There can be other 6.00.something versions, no? Say with different servicepacks or something? //Magnus ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] Revised patch for fixing archiver shutdown behavior
Alvaro Herrera [EMAIL PROTECTED] writes: Something I'm still wondering is about the archiver/logger combination. What happens if a postmaster is stopped by the user and the archiver is still running, and the user starts a new postmaster? This would launch a new archiver and logger; and there are now two loggers possibly writing to the same files, and truncated log lines could occur. I'm not nearly as worried about that as I am about the prospect of two concurrent archivers :-( There was discussion of having a lock file for the archiver, but it's still an open issue. I'm not sure how to solve the problem of stale lockfiles --- unlike the postmaster, the archiver can't assume that it's the only live process with the postgres userid. For example, after a system crash-and-restart, it's entirely likely that the PID formerly held by the archiver is now held by the bgwriter, making the lockfile (if any) look live. Maybe we should go back to the plan of having the postmaster wait for the archiver to exit. regards, tom lane ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] reference problem of manifest.(win32.mak of libpq.dll)
From: Magnus Hagander [EMAIL PROTECTED] Condition understanding of '=' is not made as for namke of VC6 to a regrettable thing, but it causes an error to it:-( So, except all thought that it was good. Hmm. Crap. Perhaps there's something else we can check on? Like a feature or environment variable only present in newer versions or something? I don't think of a good idea. However, since our document has announced officially, saying =VC7.1. Therefore, I think that it is satisfactory. Regards, Hiroshi Saito ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] reference problem of manifest.(win32.mak of libpq.dll)
On Fri, Jan 11, 2008 at 12:15:53AM +0900, Hiroshi Saito wrote: From: Magnus Hagander [EMAIL PROTECTED] Condition understanding of '=' is not made as for namke of VC6 to a regrettable thing, but it causes an error to it:-( So, except all thought that it was good. Hmm. Crap. Perhaps there's something else we can check on? Like a feature or environment variable only present in newer versions or something? I don't think of a good idea. However, since our document has announced officially, saying =VC7.1. Therefore, I think that it is satisfactory. Ah, good point, I forgot about that. But - if we do that, why do we need that IF check *at all*? //Magnus ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] reference problem of manifest.(win32.mak of libpq.dll)
Hi. - Original Message - From: Magnus Hagander [EMAIL PROTECTED] On Wed, Jan 09, 2008 at 04:01:20PM +0900, Hiroshi Saito wrote: Hi Magnus, and Dave. It seems that it is different in nmake although the initial value of VisualStudio is embedding. Then, It sees a reference problem by the dll independent. Therefore, embedding considers like an ideal. Please take this into consideration. + !IF $(_NMAKE_VER) != 6.00.9782.0 I don't think that's safe. We need to do a range check. Perhaps check if vesion is = 7.0 or something? There can be other 6.00.something versions, no? Say with different servicepacks or something? Condition understanding of '=' is not made as for namke of VC6 to a regrettable thing, but it causes an error to it:-( So, except all thought that it was good. Regards, Hiroshi Saito ---(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] reference problem of manifest.(win32.mak of libpq.dll)
From: Magnus Hagander [EMAIL PROTECTED] I don't think of a good idea. However, since our document has announced officially, saying =VC7.1. Therefore, I think that it is satisfactory. Ah, good point, I forgot about that. But - if we do that, why do we need that IF check *at all*? It is because it is saved by it although VC6 is private. probably, present all will be possible by it. I feel that it is better than exclusion for it. Regards, Hiroshi Saito ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] reference problem of manifest.(win32.mak of libpq.dll)
On Thu, Jan 10, 2008 at 11:52:15PM +0900, Hiroshi Saito wrote: Hi. - Original Message - From: Magnus Hagander [EMAIL PROTECTED] On Wed, Jan 09, 2008 at 04:01:20PM +0900, Hiroshi Saito wrote: Hi Magnus, and Dave. It seems that it is different in nmake although the initial value of VisualStudio is embedding. Then, It sees a reference problem by the dll independent. Therefore, embedding considers like an ideal. Please take this into consideration. + !IF $(_NMAKE_VER) != 6.00.9782.0 I don't think that's safe. We need to do a range check. Perhaps check if vesion is = 7.0 or something? There can be other 6.00.something versions, no? Say with different servicepacks or something? Condition understanding of '=' is not made as for namke of VC6 to a regrettable thing, but it causes an error to it:-( So, except all thought that it was good. Hmm. Crap. Perhaps there's something else we can check on? Like a feature or environment variable only present in newer versions or something? /Magnus ---(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] Revised patch for fixing archiver shutdown behavior
On Thu, 2008-01-10 at 12:28 -0300, Alvaro Herrera wrote: Yeah, that seems the safest to me -- the problem is that it complicates the shutdown sequence a fair bit, because postmaster must act differently depending on whether archiving is enabled or not: wait for bgwriter exit if disabled, or for archiver exit otherwise. If there's nothing to write, archiver will exit quickly, so in most cases they will look the same. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Revised patch for fixing archiver shutdown behavior
Tom Lane wrote: There was discussion of having a lock file for the archiver, but it's still an open issue. I'm not sure how to solve the problem of stale lockfiles --- unlike the postmaster, the archiver can't assume that it's the only live process with the postgres userid. For example, after a system crash-and-restart, it's entirely likely that the PID formerly held by the archiver is now held by the bgwriter, making the lockfile (if any) look live. Ah, right :-( Maybe we should go back to the plan of having the postmaster wait for the archiver to exit. Yeah, that seems the safest to me -- the problem is that it complicates the shutdown sequence a fair bit, because postmaster must act differently depending on whether archiving is enabled or not: wait for bgwriter exit if disabled, or for archiver exit otherwise. -- 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] Revised patch for fixing archiver shutdown behavior
Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: Maybe we should go back to the plan of having the postmaster wait for the archiver to exit. Yeah, that seems the safest to me -- the problem is that it complicates the shutdown sequence a fair bit, because postmaster must act differently depending on whether archiving is enabled or not: wait for bgwriter exit if disabled, or for archiver exit otherwise. Given the recent changes to make the postmaster act as a state machine, I don't think this is really a big deal --- it's just one more state. The bigger part is that the archiver can't wait for postmaster exit. We'll need a proper shutdown signal for the archiver, but since it's not using SIGUSR2 that can be commandeered easily. So it'd be like SIGUSR1 - do an archive cycle SIGUSR2 - do an archive cycle and exit no postmaster - just exit The rationale for the last is that it's a crash situation, and furthermore there's a risk of someone starting a new postmaster and a conflicting archiver. So we should put back the behavior my last patch removed of aborting archiving immediately on postmaster death. I'll respin my patch this way... regards, tom lane ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] reference problem of manifest.(win32.mak of libpq.dll)
On Fri, Jan 11, 2008 at 12:29:53AM +0900, Hiroshi Saito wrote: From: Magnus Hagander [EMAIL PROTECTED] I don't think of a good idea. However, since our document has announced officially, saying =VC7.1. Therefore, I think that it is satisfactory. Ah, good point, I forgot about that. But - if we do that, why do we need that IF check *at all*? It is because it is saved by it although VC6 is private. probably, present all will be possible by it. I feel that it is better than exclusion for it. Ok. Applied. //Magnus ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] Revised patch for fixing archiver shutdown behavior
On Thu, 2008-01-10 at 10:13 -0500, Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Something I'm still wondering is about the archiver/logger combination. What happens if a postmaster is stopped by the user and the archiver is still running, and the user starts a new postmaster? This would launch a new archiver and logger; and there are now two loggers possibly writing to the same files, and truncated log lines could occur. I'm not nearly as worried about that as I am about the prospect of two concurrent archivers :-( How strange, I was just looking at that particular possibility when your mail arrived. The earlier patch looks good, but I see you've reverted the PostmasterIsAlive() check in pgarch_ArchiverCopyLoop(). That was put in to prevent multiple archivers. Can we keep that? -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com ---(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] Revised patch for fixing archiver shutdown behavior
I wrote: I'll respin my patch this way... Third time's the charm? regards, tom lane binFKkWVCJKov.bin Description: archiver-shutdown-3.patch ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
[PATCHES] Re: [BUGS] BUG #3860: xpath crashes backend when is querying xmlagg result
Tom Lane escribió:
Alvaro Herrera [EMAIL PROTECTED] writes:
Perhaps a better idea is to create a separate LibxmlContext memcxt,
child of QueryContext, and have xml_palloc etc always use that. That
way it won't be reset between calls. It probably also means we could
wire xml reset to transaction abort, making it a bit simpler.
Might as well go back to letting it use malloc :-(.
I actually don't see a problem with letting it use malloc for stuff that
it is going to manage for itself. I guess the problem comes with
transient return values of libxml functions; we'd want to explicitly
move those into palloc-based storage and then free() them.
This looks like a rather large rewrite though. Peter, have you any
better ideas?
OK, after a lot of research I think the best way to deal with this is
what I suggest above. With the attached patch, I cannot cause the
system to crash with any of the given examples.
Furthermore this may help clean up the PG_TRY blocks that are currently
sprinkled through the xml.c code.
Handling of subtransactions is no good at this point, but I think that
could easily be improved.
--
Alvaro Herrerahttp://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Index: src/backend/access/transam/xact.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/access/transam/xact.c,v
retrieving revision 1.256
diff -c -p -r1.256 xact.c
*** src/backend/access/transam/xact.c 3 Jan 2008 21:23:15 - 1.256
--- src/backend/access/transam/xact.c 10 Jan 2008 21:00:58 -
***
*** 45,50
--- 45,51
#include utils/inval.h
#include utils/memutils.h
#include utils/relcache.h
+ #include utils/xml.h
/*
*** CommitTransaction(void)
*** 1678,1683
--- 1679,1685
AtEOXact_ComboCid();
AtEOXact_HashTables(true);
AtEOXact_PgStat(true);
+ AtEOXact_xml();
pgstat_report_xact_timestamp(0);
CurrentResourceOwner = NULL;
*** AbortTransaction(void)
*** 2028,2033
--- 2030,2036
AtEOXact_ComboCid();
AtEOXact_HashTables(false);
AtEOXact_PgStat(false);
+ AtEOXact_xml();
pgstat_report_xact_timestamp(0);
/*
Index: src/backend/utils/adt/xml.c
===
RCS file: /home/alvherre/Code/cvs/pgsql/src/backend/utils/adt/xml.c,v
retrieving revision 1.64
diff -c -p -r1.64 xml.c
*** src/backend/utils/adt/xml.c 1 Jan 2008 19:45:53 - 1.64
--- src/backend/utils/adt/xml.c 10 Jan 2008 20:45:48 -
*** XmlOptionType xmloption;
*** 80,85
--- 80,86
#ifdef USE_LIBXML
static StringInfo xml_err_buf = NULL;
+ static MemoryContext LibxmlContext = NULL;
static void xml_init(void);
static void *xml_palloc(size_t size);
*** xml_init(void)
*** 953,958
--- 954,965
xmlSetGenericErrorFunc(NULL, xml_errorHandler);
/* Set up memory allocation our way, too */
+ Assert(LibxmlContext == NULL);
+ LibxmlContext = AllocSetContextCreate(TopTransactionContext,
+ LibxmlContext,
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
xmlMemSetup(xml_pfree, xml_palloc, xml_repalloc, xml_pstrdup);
/* Check library compatibility */
*** xml_init(void)
*** 974,983
--- 981,1007
* about, anyway.
*/
xmlSetGenericErrorFunc(NULL, xml_errorHandler);
+ if (LibxmlContext == NULL)
+ LibxmlContext = AllocSetContextCreate(TopTransactionContext,
+ LibxmlContext,
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
xmlMemSetup(xml_pfree, xml_palloc, xml_repalloc, xml_pstrdup);
}
}
+ void
+ AtEOXact_xml(void)
+ {
+ if (LibxmlContext == NULL)
+ return;
+
+ MemoryContextDelete(LibxmlContext);
+ LibxmlContext = NULL;
+
+ xmlCleanupParser();
+ }
/*
* SQL/XML allows storing XML documents or XML content. XML
*** print_xml_decl(StringInfo buf, const xml
*** 1207,1213
* Convert a C string to XML internal representation
*
* TODO maybe, libxml2's xmlreader is better? (do not construct DOM,
! * yet do not use SAX - see xml_reader.c)
*/
static xmlDocPtr
xml_parse(text *data, XmlOptionType xmloption_arg, bool preserve_whitespace,
--- 1231,1237
* Convert a C string to XML internal representation
*
* TODO maybe, libxml2's xmlreader is better? (do not construct DOM,
! * yet do not use SAX - see xmlreader.c)
*/
static xmlDocPtr
xml_parse(text *data, XmlOptionType xmloption_arg, bool preserve_whitespace,
*** xml_parse(text *data, XmlOptionType xmlo
*** 1245,1251
/*
* Note, that here we try to apply DTD defaults
* (XML_PARSE_DTDATTR) according to
Re: [PATCHES] [BUGS] BUG #3860: xpath crashes backend when is querying xmlagg result
Alvaro Herrera [EMAIL PROTECTED] writes:
+ void
+ AtEOXact_xml(void)
+ {
+ if (LibxmlContext == NULL)
+ return;
+
+ MemoryContextDelete(LibxmlContext);
+ LibxmlContext = NULL;
+
+ xmlCleanupParser();
+ }
[ blink... ] Shouldn't that be the other way around? It looks to me
like xmlCleanupParser will be pfree'ing already-deleted data. Did you
test this with CLOBBER_FREED_MEMORY active?
Also, I don't see how this patch fixes what I believe to be the
fundamental problem, which is that we have code paths that invoke
libxml without having previously initialized the alloc support.
I think the sequence
if (LibxmlContext == NULL)
{
create LibxmlContext;
xmlMemSetup(...);
}
ought to be executed before *any* use of libxml stuff (which suggests
factoring it out as a subroutine...)
We also need to do some testing to see if letting the thing go until
transaction end is OK, or whether we will see unacceptable memory leaks
over long series of XML calls. (In particular I suspect that we'd
better zap the context at subtransaction abort; but even without any
error, are we sure that normal operations don't leak memory?)
One thing I was wondering about earlier today is whether libxml isn't
expecting NULL-return-on-failure from the malloc-substitute routine.
If we take control away from it unexpectedly, I wouldn't be a bit
surprised if its data structures are left corrupt. This might lead to
failures during cleanup.
I do like the idea of getting rid of the PG_TRY blocks and the
associated cleanup attempts; with the approach you're converging on
here, we need only assume that xmlCleanupParser() will get rid of
all staticly-allocated pointers and not crash, whereas right now we
are assuming a great deal of libxml stuff still works after an ereport
(which makes throwing ereport from xml_palloc even more risky).
regards, tom lane
---(end of broadcast)---
TIP 7: You can help support the PostgreSQL project by donating at
http://www.postgresql.org/about/donate
Re: [PATCHES] [BUGS] BUG #3860: xpath crashes backend when is querying xmlagg result
Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane escribió: One thing I was wondering about earlier today is whether libxml isn't expecting NULL-return-on-failure from the malloc-substitute routine. If we take control away from it unexpectedly, I wouldn't be a bit surprised if its data structures are left corrupt. This might lead to failures during cleanup. Hmm, this is a very good point. I quick look at the source shows that they are not very consistent on its own checking for memory allocation errors. For example, see a bug I just reported: http://bugzilla.gnome.org/show_bug.cgi?id=508662 Ugh. So we're pretty much damned if we do and damned if we don't. Given what you showed, it is certain that we are at risk if we return NULL, whereas it is merely hypothetical that we are at risk if we longjmp. So let's stick to the palloc infrastructure for now. 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
