Thread: Re: [HACKERS] Patch to implement pg_current_logfile() function
On Wed, 18 Jan 2017 19:27:40 -0300
Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Karl O. Pinc wrote:
>
> > @@ -511,10 +519,16 @@ int
> > SysLogger_Start(void)
> > {
> > pid_t sysloggerPid;
> > - char *filename;
> >
> > + /*
> > + * Logging collector is not enabled. We don't know where
> > messages are
> > + * logged. Remove outdated file holding the current log
> > filenames.
> > + */
> > if (!Logging_collector)
> > + {
> > + unlink(LOG_METAINFO_DATAFILE);
> > return 0;
> > + }
>
> I thought this part was odd -- I mean, why is SysLogger_Start() being
> called if the collector is not enabled? Turns out we do it and return
> early if not enabled. But not in all cases -- there is one callsite
> in postmaster.c that avoids the call if the collector is disabled.
> That needs to be changed if we want this to work reliably.
Is this an argument for having the current_logfiles always exist
and be empty when there is no in-filesystem logfile? It always felt
to me that the code would be simpler that way.
Karl <kop@meme.com>
Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
Karl O. Pinc wrote: > On Wed, 18 Jan 2017 19:27:40 -0300 > Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > I thought this part was odd -- I mean, why is SysLogger_Start() being > > called if the collector is not enabled? Turns out we do it and return > > early if not enabled. But not in all cases -- there is one callsite > > in postmaster.c that avoids the call if the collector is disabled. > > That needs to be changed if we want this to work reliably. > > Is this an argument for having the current_logfiles always exist > and be empty when there is no in-filesystem logfile? It always felt > to me that the code would be simpler that way. I don't know. I am just saying that you need to patch postmaster.c line 1726 to remove the second arm of the &&. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, 19 Jan 2017 12:12:18 -0300 Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Karl O. Pinc wrote: > > On Wed, 18 Jan 2017 19:27:40 -0300 > > Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > > > > I thought this part was odd -- I mean, why is SysLogger_Start() > > > being called if the collector is not enabled? Turns out we do it > > > and return early if not enabled. But not in all cases -- there > > > is one callsite in postmaster.c that avoids the call if the > > > collector is disabled. That needs to be changed if we want this > > > to work reliably. > > > > Is this an argument for having the current_logfiles always exist > > and be empty when there is no in-filesystem logfile? It always felt > > to me that the code would be simpler that way. > > I don't know. I am just saying that you need to patch postmaster.c > line 1726 to remove the second arm of the &&. Gilles, I'm not available just now. Can you do this or enlist Michael? Karl <kop@meme.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
On Fri, Jan 20, 2017 at 12:08 AM, Karl O. Pinc <kop@meme.com> wrote: > Is this an argument for having the current_logfiles always exist > and be empty when there is no in-filesystem logfile? It always felt > to me that the code would be simpler that way. Well, you'll need to do something in any case if the logging_collector is found disabled and the syslogger process is restarted. So just removing it looked cleaner to me. I am not strongly attached to one way of doing or the other though. -- Michael
On Fri, Jan 20, 2017 at 3:29 AM, Karl O. Pinc <kop@meme.com> wrote: > On Thu, 19 Jan 2017 12:12:18 -0300 > Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > >> Karl O. Pinc wrote: >> > On Wed, 18 Jan 2017 19:27:40 -0300 >> > Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> >> > > I thought this part was odd -- I mean, why is SysLogger_Start() >> > > being called if the collector is not enabled? Turns out we do it >> > > and return early if not enabled. But not in all cases -- there >> > > is one callsite in postmaster.c that avoids the call if the >> > > collector is disabled. That needs to be changed if we want this >> > > to work reliably. >> > >> > Is this an argument for having the current_logfiles always exist >> > and be empty when there is no in-filesystem logfile? It always felt >> > to me that the code would be simpler that way. >> >> I don't know. I am just saying that you need to patch postmaster.c >> line 1726 to remove the second arm of the &&. > > Gilles, > > I'm not available just now. Can you do this or enlist Michael? Okay I just did it. At the same time the check for ferror is not necessary as fgets() returns NULL on an error as well so that's dead code. I have also removed the useless call to FreeFile(). -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
> diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c > @@ -148,6 +149,9 @@ static const char *excludeFiles[] = > /* Skip auto conf temporary file. */ > PG_AUTOCONF_FILENAME ".tmp", > > + /* Skip current log file temporary file */ > + LOG_METAINFO_DATAFILE_TMP, > + Sorry if this has already been answered, but why are we not also skipping LOG_METAINFO_DATAFILE itself? I can't see a scenario where it's useful to have that file in a base backup, since it's very likely that the log file used when the backup is restored will be different. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jan 20, 2017 at 10:11 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: >> diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c > >> @@ -148,6 +149,9 @@ static const char *excludeFiles[] = >> /* Skip auto conf temporary file. */ >> PG_AUTOCONF_FILENAME ".tmp", >> >> + /* Skip current log file temporary file */ >> + LOG_METAINFO_DATAFILE_TMP, >> + > > Sorry if this has already been answered, but why are we not also > skipping LOG_METAINFO_DATAFILE itself? I can't see a scenario where > it's useful to have that file in a base backup, since it's very likely > that the log file used when the backup is restored will be different. I have done the same remark upthread, and Gilles has pointed out that it would be useful to get the last log file that was used by a backup. Including this file represents no harm as well. -- Michael
On Fri, Jan 20, 2017 at 10:14 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, Jan 20, 2017 at 10:11 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: >>> diff --git a/src/backend/replication/basebackup.c b/src/backend/replication/basebackup.c >> >>> @@ -148,6 +149,9 @@ static const char *excludeFiles[] = >>> /* Skip auto conf temporary file. */ >>> PG_AUTOCONF_FILENAME ".tmp", >>> >>> + /* Skip current log file temporary file */ >>> + LOG_METAINFO_DATAFILE_TMP, >>> + >> >> Sorry if this has already been answered, but why are we not also >> skipping LOG_METAINFO_DATAFILE itself? I can't see a scenario where >> it's useful to have that file in a base backup, since it's very likely >> that the log file used when the backup is restored will be different. > > I have done the same remark upthread, and Gilles has pointed out that > it would be useful to get the last log file that was used by a backup. > Including this file represents no harm as well. Moved to CF 2017-03. -- Michael
On Fri, Jan 20, 2017 at 2:09 AM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> Okay I just did it. At the same time the check for ferror is not
> necessary as fgets() returns NULL on an error as well so that's dead
> code. I have also removed the useless call to FreeFile().
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 271c492..a7ebb74 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1733,7 +1733,7 @@ ServerLoop(void) }
/* If we have lost the log collector, try to start a new one */
- if (SysLoggerPID == 0 && Logging_collector)
+ if (SysLoggerPID == 0) SysLoggerPID = SysLogger_Start();
/*
This hunk has zero chance of being acceptable, I think. We're not
going to start running the syslogger in even when it's not configured
to run.
I think what should happen is that the postmaster should try to remove
any old metainfo datafile before it reaches this point, and then if
the logging collector is started it will recreate it.
+ ereport(WARNING,
+ (errcode(ERRCODE_INTERNAL_ERROR),
+ errmsg("corrupted data found in \"%s\"",
+ LOG_METAINFO_DATAFILE)));
elog seems fine here. There's no need for this to be translatable.
Also, I'd change the level to ERROR.
+ errhint("The supported log formats are \"stderr\""
+ " and \"csvlog\".")));
I think our preferred style is not to break strings across lines like this.
+ log_filepath[strcspn(log_filepath, "\n")] = '\0';
We have no existing dependency on strcspn(). It might be better not
to add one just for this feature; I suggest using strchr() instead,
which is widely used in our existing code.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, 15 Feb 2017 15:23:00 -0500
Robert Haas <robertmhaas@gmail.com> wrote:
> + errhint("The supported log formats are
> \"stderr\""
> + " and \"csvlog\".")));
>
> I think our preferred style is not to break strings across lines like
> this.
How do you do that and not exceed the 80 character line limit?
Just ignore the line length limit?
Karl <kop@meme.com>
Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
Robert Haas wrote: > On Fri, Jan 20, 2017 at 2:09 AM, Michael Paquier > <michael.paquier@gmail.com> wrote: > > Okay I just did it. At the same time the check for ferror is not > > necessary as fgets() returns NULL on an error as well so that's dead > > code. I have also removed the useless call to FreeFile(). > > diff --git a/src/backend/postmaster/postmaster.c > b/src/backend/postmaster/postmaster.c > index 271c492..a7ebb74 100644 > --- a/src/backend/postmaster/postmaster.c > +++ b/src/backend/postmaster/postmaster.c > @@ -1733,7 +1733,7 @@ ServerLoop(void) > } > > /* If we have lost the log collector, try to start a new one */ > - if (SysLoggerPID == 0 && Logging_collector) > + if (SysLoggerPID == 0) > SysLoggerPID = SysLogger_Start(); > > /* > > This hunk has zero chance of being acceptable, I think. We're not > going to start running the syslogger in even when it's not configured > to run. So what is going on here is that SysLogger_Start() wants to unlink the current-logfile file if the collector is not enabled. This should probably be split out into a separate new function, for two reasons: first, it doesn't seem good idea to have SysLogger_Start do something other than start the logger; and second, so that we don't have a syscall on each ServerLoop iteration. That new function should be called from some other place -- perhaps reaper() and just before entering ServerLoop, so that the file is deleted if the syslogger goes away or is not started. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
"Karl O. Pinc" <kop@meme.com> writes:
> On Wed, 15 Feb 2017 15:23:00 -0500
> Robert Haas <robertmhaas@gmail.com> wrote:
>> I think our preferred style is not to break strings across lines like
>> this.
> How do you do that and not exceed the 80 character line limit?
> Just ignore the line length limit?
Right. It depends on context, but letting isolated lines wrap doesn't
do that much damage to readability, IMO anyway. (If you've got a bunch
of these adjacent to each other, you might choose to break them to
reduce confusion.)
The advantage of not breaking up error strings is that it makes grepping
for them more reliable, when you're wondering "where in the sources did
this error come from?". If you get a report about "could not frob a
whatzit" and grep for "frob a whatzit", but somebody decided to break
that string in the middle to avoid a line wrap, you won't find the spot.
regards, tom lane
On Wed, 15 Feb 2017 15:23:00 -0500
Robert Haas <robertmhaas@gmail.com> wrote:
> + ereport(WARNING,
> + (errcode(ERRCODE_INTERNAL_ERROR),
> + errmsg("corrupted data found in \"%s\"",
> + LOG_METAINFO_DATAFILE)));
>
> elog seems fine here. There's no need for this to be translatable.
> Also, I'd change the level to ERROR.
>
> + errhint("The supported log formats are
> \"stderr\""
> + " and \"csvlog\".")));
>
> I think our preferred style is not to break strings across lines like
> this.
>
> + log_filepath[strcspn(log_filepath, "\n")] = '\0';
>
> We have no existing dependency on strcspn(). It might be better not
> to add one just for this feature; I suggest using strchr() instead,
> which is widely used in our existing code.
Attached is a v29 patch which fixes the above problems.
The Syslogger hunk remains to be fixed. I have no plans
to do so at this time.
Note that since I have to write an "if" anyway, I'm going ahead
and raising an error condition when there's no newline in the
current_logfiles file. The strcspn() ignored the missing newline.
The new code could do so as well by negating the if condition
should that be preferable.
On a different topic, I pulled from master and now
I see that git finds the following untracked:
src/bin/pg_basebackup/pg_receivexlog
src/bin/pg_resetxlog/
src/bin/pg_xlogdump/
I'd appreciate knowing if I'm doing something dumb
on my end to make this happen. Thanks.
Karl <kop@meme.com>
Free Software: "You don't pay back, you pay forward."
-- Robert A. Heinlein
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
"Karl O. Pinc" <kop@meme.com> writes: > On a different topic, I pulled from master and now > I see that git finds the following untracked: > src/bin/pg_basebackup/pg_receivexlog > src/bin/pg_resetxlog/ > src/bin/pg_xlogdump/ Those got renamed, cf https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=85c11324cabaddcfaf3347df78555b30d27c5b5a regards, tom lane
On Wed, Feb 15, 2017 at 4:03 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > So what is going on here is that SysLogger_Start() wants to unlink the > current-logfile file if the collector is not enabled. This should > probably be split out into a separate new function, for two reasons: > first, it doesn't seem good idea to have SysLogger_Start do something > other than start the logger; and second, so that we don't have a syscall > on each ServerLoop iteration. That new function should be called from > some other place -- perhaps reaper() and just before entering > ServerLoop, so that the file is deleted if the syslogger goes away or is > not started. I think it's sufficient to just remove the file once on postmaster startup before trying to launch the syslogger for the first time. logging_collector is PGC_POSTMASTER, so if it's not turned on when the cluster first starts, it can't be activated later. If it dies, that doesn't seem like a reason to remove the file. We're going to restart the syslogger, and when we do, it can update the file. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Le 16/02/2017 à 16:13, Robert Haas a écrit : > On Wed, Feb 15, 2017 at 4:03 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: >> So what is going on here is that SysLogger_Start() wants to unlink the >> current-logfile file if the collector is not enabled. This should >> probably be split out into a separate new function, for two reasons: >> first, it doesn't seem good idea to have SysLogger_Start do something >> other than start the logger; and second, so that we don't have a syscall >> on each ServerLoop iteration. That new function should be called from >> some other place -- perhaps reaper() and just before entering >> ServerLoop, so that the file is deleted if the syslogger goes away or is >> not started. > I think it's sufficient to just remove the file once on postmaster > startup before trying to launch the syslogger for the first time. > logging_collector is PGC_POSTMASTER, so if it's not turned on when the > cluster first starts, it can't be activated later. If it dies, that > doesn't seem like a reason to remove the file. We're going to restart > the syslogger, and when we do, it can update the file. > I've attached a new full v30 patch that include last patch from Karl. Now the current_logfile file is removed only at postmaster startup, just before launching the syslogger for the first time. -- Gilles Darold Consultant PostgreSQL http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
On Mon, Feb 20, 2017 at 4:37 AM, Gilles Darold <gilles.darold@dalibo.com> wrote: >> I think it's sufficient to just remove the file once on postmaster >> startup before trying to launch the syslogger for the first time. >> logging_collector is PGC_POSTMASTER, so if it's not turned on when the >> cluster first starts, it can't be activated later. If it dies, that >> doesn't seem like a reason to remove the file. We're going to restart >> the syslogger, and when we do, it can update the file. >> > > I've attached a new full v30 patch that include last patch from Karl. > Now the current_logfile file is removed only at postmaster startup, just > before launching the syslogger for the first time. Committed with some changes. - Added missing REVOKE for pg_current_logfile(text). - Added error checks for unlink() and logfile_open(). - Changed error messages to use standard wording (among other reasons, this helps minimize translation work). - Removed update_metainfo_datafile() call that ran in the postmaster and added a similar call that runs in the syslogger. - doc: Removed an index entry that had no existing parallel. Hopefully I haven't broken anything; please let me know if you encounter any issues. Thanks, -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Mar 3, 2017 at 3:18 PM, Robert Haas <robertmhaas@gmail.com> wrote: > Hopefully I haven't broken anything; please let me know if you > encounter any issues. Reading what has just been committed... + /* + * No space found, file content is corrupted. Return NULL to the + * caller and inform him on the situation. + */ + elog(ERROR, + "missing space character in \"%s\"", LOG_METAINFO_DATAFILE); + break; There is no need to issue a break after a elog(ERROR). + * No newlinei found, file content is corrupted. Return NULL to s/newlinei/newline/ -- Michael
On Fri, 3 Mar 2017 15:24:53 +0900 Michael Paquier <michael.paquier@gmail.com> wrote: > > + /* > + * No space found, file content is corrupted. Return > NULL to the > + * caller and inform him on the situation. > + */ > + elog(ERROR, > + "missing space character in \"%s\"", > LOG_METAINFO_DATAFILE); > + break; > There is no need to issue a break after a elog(ERROR). There's 2 cases of issuing a break after a elog(ERROR), both in the same loop. And the comment could then be amended as well to just: No space found, file content is corrupted Because (I presume) there's no returning of any value going on. But if the code does not exit the loop then before calling elog(ERROR), shouldn't it also call FreeFile(fd)? Regards, Karl <kop@meme.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
On Fri, Mar 3, 2017 at 11:54 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Fri, Mar 3, 2017 at 3:18 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> Hopefully I haven't broken anything; please let me know if you >> encounter any issues. > > Reading what has just been committed... > > + /* > + * No space found, file content is corrupted. Return NULL to the > + * caller and inform him on the situation. > + */ > + elog(ERROR, > + "missing space character in \"%s\"", LOG_METAINFO_DATAFILE); > + break; > There is no need to issue a break after a elog(ERROR). True, but it's not wrong, either. We do it all the time. git grep -A2 elog.*ERROR /break <press n until you get bored> The fact that the comment doesn't match the code, though, is wrong. Oops. > + * No newlinei found, file content is corrupted. Return NULL to > s/newlinei/newline/ That's also a problem, and that comment also refers to returning, which we don't. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Mar 3, 2017 at 7:21 PM, Karl O. Pinc <kop@meme.com> wrote: > But if the code does not exit the loop then > before calling elog(ERROR), shouldn't it > also call FreeFile(fd)? Hmm. Normally error recovery cleans up opened files, but since logfile_open() directly calls fopen(), that's not going to work here. So that does seem to be a problem. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Sat, 4 Mar 2017 14:26:54 +0530 Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Mar 3, 2017 at 7:21 PM, Karl O. Pinc <kop@meme.com> wrote: > > But if the code does not exit the loop then > > before calling elog(ERROR), shouldn't it > > also call FreeFile(fd)? > > Hmm. Normally error recovery cleans up opened files, but since > logfile_open() directly calls fopen(), that's not going to work here. > So that does seem to be a problem. Should something different be done to open the file or is it enough to call FreeFile(fd) to clean up properly? Karl <kop@meme.com> Free Software: "You don't pay back, you pay forward." -- Robert A. Heinlein
"Karl O. Pinc" <kop@meme.com> writes:
> On Sat, 4 Mar 2017 14:26:54 +0530
> Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Mar 3, 2017 at 7:21 PM, Karl O. Pinc <kop@meme.com> wrote:
>>> But if the code does not exit the loop then
>>> before calling elog(ERROR), shouldn't it
>>> also call FreeFile(fd)?
>> Hmm. Normally error recovery cleans up opened files, but since
>> logfile_open() directly calls fopen(), that's not going to work here.
>> So that does seem to be a problem.
> Should something different be done to open the file or is it
> enough to call FreeFile(fd) to clean up properly?
I would say that in at least 99.44% of cases, if you think you need to
do some cleanup action immediately before an elog(ERROR), that means
You're Doing It Wrong. That can only be a safe coding pattern in a
segment of code in which no called function does, *or ever will*, throw
elog(ERROR) itself. Straight-line code with no reason ever to call
anything else might qualify, but otherwise you're taking too much risk
of current or future breakage. You need some mechanism that will ensure
cleanup after any elog call anywhere, and the backend environment offers
lots of such mechanisms.
Without having actually looked at this patch, I would say that if it added
a direct call of fopen() to backend-side code, that was already the wrong
thing. Almost always, AllocateFile() would be a better choice, not only
because it's tied into transaction abort, but also because it knows how to
release virtual FDs in event of ENFILE/EMFILE errors. If there is some
convincing reason why you shouldn't use AllocateFile(), then a safe
cleanup pattern would be to have the fclose() in a PG_CATCH stanza.
(FWIW, I don't particularly agree with Michael's objection to "break"
after elog(ERROR). Our traditional coding style is to write such things
anyway, so as to avoid drawing complaints from compilers that don't know
that elog(ERROR) doesn't return. You could argue that that's an obsolete
reason, but I don't think I want to see it done some places and not
others. Consistency of coding style is a good thing.)
regards, tom lane
On Sat, Mar 4, 2017 at 10:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Without having actually looked at this patch, I would say that if it added > a direct call of fopen() to backend-side code, that was already the wrong > thing. Almost always, AllocateFile() would be a better choice, not only > because it's tied into transaction abort, but also because it knows how to > release virtual FDs in event of ENFILE/EMFILE errors. If there is some > convincing reason why you shouldn't use AllocateFile(), then a safe > cleanup pattern would be to have the fclose() in a PG_CATCH stanza. I think that my previous remarks on this issue were simply muddled thinking. The SQL-callable function pg_current_logfile() does use AllocateFile(), so the ERROR which may occur afterward if the file is corrupted is no problem. The syslogger, on the other hand, uses logfile_open() to open the file, but it's careful not to throw an ERROR while the file is open, just like other code which runs in the syslogger. So now I think there's no bug here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Tue, Mar 7, 2017 at 3:21 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sat, Mar 4, 2017 at 10:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Without having actually looked at this patch, I would say that if it added >> a direct call of fopen() to backend-side code, that was already the wrong >> thing. Almost always, AllocateFile() would be a better choice, not only >> because it's tied into transaction abort, but also because it knows how to >> release virtual FDs in event of ENFILE/EMFILE errors. If there is some >> convincing reason why you shouldn't use AllocateFile(), then a safe >> cleanup pattern would be to have the fclose() in a PG_CATCH stanza. > > I think that my previous remarks on this issue were simply muddled > thinking. The SQL-callable function pg_current_logfile() does use > AllocateFile(), so the ERROR which may occur afterward if the file is > corrupted is no problem. The syslogger, on the other hand, uses > logfile_open() to open the file, but it's careful not to throw an > ERROR while the file is open, just like other code which runs in the > syslogger. So now I think there's no bug here. - /* - * No space found, file content is corrupted. Return NULL to the - * caller and inform him on the situation. - */ + /* Uh oh. No newline found, so file content is corrupted. */ This one just made me smile. -- Michael