Thread: autovauum integration patch: Attempt #4
Ok, sorry about the mis-fire on the last patch I submitted, this one is what I was meant to send. As before: This patch adds the following since attempt #2: * updated to latest CVS * changed GUC vars from autovac_* to autovacuum_* * changed autovac_enabled GUC variable to autovacuum * changed autovacuum GUC variable default to false * improved autovacuum failure when row level stats aren't enabled As before, to apply this patch: 1) Move pg_autovacuum.c and .h get from contrib to src/backend/postmaster and src/include/postmaster respectively. 2) Place the attached pg_autovacuum.h in src/include/catelog/ 3) Apply the attached diff file Please apply to CVS or tell me what I need to change to get it applied. Thanks again, Matthew
Attachment
Matthew T. O'Connor wrote: > As before, to apply this patch: > 1) Move pg_autovacuum.c and .h get from contrib to > src/backend/postmaster and src/include/postmaster respectively. Trivial comment: maybe we can drop the pg_ prefix on the file names. > 2) Place the attached pg_autovacuum.h in src/include/catelog/ I'm not sure whether we can allow int8 columns in system tables, for portability reasons. -- Peter Eisentraut http://developer.postgresql.org/~petere/
Peter Eisentraut wrote: > Matthew T. O'Connor wrote: > >>As before, to apply this patch: >>1) Move pg_autovacuum.c and .h get from contrib to >>src/backend/postmaster and src/include/postmaster respectively. > > Trivial comment: maybe we can drop the pg_ prefix on the file names. Ok, that's not a problem. Bruce would you like me to submit another patch that makes this change, or will you make the change when you apply the patch? >>2) Place the attached pg_autovacuum.h in src/include/catelog/ > > I'm not sure whether we can allow int8 columns in system tables, for > portability reasons. Someone will have to tell me if this is really a problem, I have no idea. I'm using using int8 since one could set a threshold to a number greater than 4 million. I could probably change it to use a float value. Comments?
Matthew T. O'Connor wrote: > Peter Eisentraut wrote: > > > Matthew T. O'Connor wrote: > > > >>As before, to apply this patch: > >>1) Move pg_autovacuum.c and .h get from contrib to > >>src/backend/postmaster and src/include/postmaster respectively. > > > > Trivial comment: maybe we can drop the pg_ prefix on the file names. > > Ok, that's not a problem. Bruce would you like me to submit another > patch that makes this change, or will you make the change when you apply > the patch? I can make the change. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Peter Eisentraut wrote: > Matthew T. O'Connor wrote: > > As before, to apply this patch: > > 1) Move pg_autovacuum.c and .h get from contrib to > > src/backend/postmaster and src/include/postmaster respectively. > > Trivial comment: maybe we can drop the pg_ prefix on the file names. > > > 2) Place the attached pg_autovacuum.h in src/include/catelog/ > > I'm not sure whether we can allow int8 columns in system tables, for > portability reasons. I notice pg_class uses real for the tuple count. Is that better? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
"Matthew T. O'Connor" <matthew@zeut.net> writes:
> Peter Eisentraut wrote:
>> I'm not sure whether we can allow int8 columns in system tables, for
>> portability reasons.
> Someone will have to tell me if this is really a problem,
Depending on int8 to actually work is right out.
I'd go with float or double depending on your precision needs.
regards, tom lane
On Fri, 2004-07-23 at 23:25, Tom Lane wrote: > "Matthew T. O'Connor" <matthew@zeut.net> writes: > > Peter Eisentraut wrote: > >> I'm not sure whether we can allow int8 columns in system tables, for > >> portability reasons. > > > Someone will have to tell me if this is really a problem, > > Depending on int8 to actually work is right out. > > I'd go with float or double depending on your precision needs. Ok, I went with float. Also, I changed pg_autovacuum.[ch] to autovacuum.[ch] as Peter suggested. So now to apply this patch: 1) Move pg_autovacuum.[ch] from contrib to src/backend/postmaster/autovacuum.c and src/include/postmaster/autovacuum.h respectively. 2) Place the attached pg_autovacuum.h in src/include/catelog/ 3) Apply the attached diff file Please apply to CVS or tell me what I need to change to get it applied. Thanks again, Matthew
Attachment
Patch applied. Thanks. --------------------------------------------------------------------------- Matthew T. O'Connor wrote: > On Fri, 2004-07-23 at 23:25, Tom Lane wrote: > > "Matthew T. O'Connor" <matthew@zeut.net> writes: > > > Peter Eisentraut wrote: > > >> I'm not sure whether we can allow int8 columns in system tables, for > > >> portability reasons. > > > > > Someone will have to tell me if this is really a problem, > > > > Depending on int8 to actually work is right out. > > > > I'd go with float or double depending on your precision needs. > > Ok, I went with float. > > Also, I changed pg_autovacuum.[ch] to autovacuum.[ch] as Peter > suggested. So now to apply this patch: > > 1) Move pg_autovacuum.[ch] from contrib to > src/backend/postmaster/autovacuum.c and > src/include/postmaster/autovacuum.h respectively. > 2) Place the attached pg_autovacuum.h in src/include/catelog/ > 3) Apply the attached diff file > > Please apply to CVS or tell me what I need to change to get it applied. > > Thanks again, > > > Matthew > > [ Attachment, skipping... ] [ Attachment, skipping... ] -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
[ Oops, I mean "added", not "applied". ]
Your patch has been added to the PostgreSQL unapplied patches list at:
http://momjian.postgresql.org/cgi-bin/pgpatches
It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.
---------------------------------------------------------------------------
Bruce Momjian wrote:
>
> Patch applied. Thanks.
>
> ---------------------------------------------------------------------------
>
>
> Matthew T. O'Connor wrote:
> > On Fri, 2004-07-23 at 23:25, Tom Lane wrote:
> > > "Matthew T. O'Connor" <matthew@zeut.net> writes:
> > > > Peter Eisentraut wrote:
> > > >> I'm not sure whether we can allow int8 columns in system tables, for
> > > >> portability reasons.
> > >
> > > > Someone will have to tell me if this is really a problem,
> > >
> > > Depending on int8 to actually work is right out.
> > >
> > > I'd go with float or double depending on your precision needs.
> >
> > Ok, I went with float.
> >
> > Also, I changed pg_autovacuum.[ch] to autovacuum.[ch] as Peter
> > suggested. So now to apply this patch:
> >
> > 1) Move pg_autovacuum.[ch] from contrib to
> > src/backend/postmaster/autovacuum.c and
> > src/include/postmaster/autovacuum.h respectively.
> > 2) Place the attached pg_autovacuum.h in src/include/catelog/
> > 3) Apply the attached diff file
> >
> > Please apply to CVS or tell me what I need to change to get it applied.
> >
> > Thanks again,
> >
> >
> > Matthew
> >
> >
>
> [ Attachment, skipping... ]
>
> [ Attachment, skipping... ]
>
> --
> Bruce Momjian | http://candle.pha.pa.us
> pgman@candle.pha.pa.us | (610) 359-1001
> + If your life is a hard drive, | 13 Roberts Road
> + Christ can be your backup. | Newtown Square, Pennsylvania 19073
>
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
> joining column's datatypes do not match
>
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Patch applied. Thanks.
What happened to the "review" part?
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Patch applied. Thanks. > > What happened to the "review" part? See correction email. I put the wrong header on the email. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> What happened to the "review" part?
> See correction email. I put the wrong header on the email.
Right, I saw that a bit later. Sorry for the noise.
regards, tom lane
"Matthew T. O'Connor" <matthew@zeut.net> writes:
> Please apply to CVS or tell me what I need to change to get it applied.
I looked over this patch (sorry for the delay), and found a number of
problems.
Bigger problems:
* I don't think you've thought through system shutdown at all. The
postmaster code as given will not try to shutdown the autovac daemon
until the same time it shuts down the bgwriter, which is no good for
a couple of reasons:
(a) None of this code will trigger as long as there is any ordinary
backend running; like say the one(s) invoked by autovac itself.
This means that autovac-driven vacuuming is considered just as good a
reason to keep the system going as an ordinary user query, which I think
is not cool. IMHO a SIGTERM to the postmaster should result in
canceling whatever autovacuum operation is currently running.
(b) It's really not cool that autovac isn't shut down *before* we start
shutting down bgwriter. I think that it might not make too much
difference right at the moment, since the autovac daemon isn't actually
making any use of its connection to shared memory, but the moment that
autovac tries to do anything on its own behalf rather than via a backend
this is going to be a serious risk. There can't be anything going on in
parallel with the shutdown checkpoint.
I think really we want autovac to shut down at the beginning of the
shutdown cycle, not the end, and not to start bgwriter shutdown until
autovac is gone.
* I hadn't quite focused before on the fact that this patch requires
statically binding frontend libpq into the backend. There are a number
of issues with that, the most risky being that if libpq.so is compiled
thread-aware then it's going to create problems on platforms where
there's a difference between thread-aware and non-thread-aware C library
code. Even without threading worries there are conflicts: if someone
calls a dllist.c routine, which instance will they get? I'm also
concerned about the implications for modules like contrib/dblink, which
expect to load libpq.so dynamically. There could be conflicts between
the dynamically linked libpq and the inbuilt one.
* AFAICS there is no provision for setting pg_user or pg_user_password,
which means that the daemon won't actually be able to connect in
non-TRUST environments. I don't know what we do about this: putting
such a password in a GUC variable is right out (because any user could
read it) and putting it on the postmaster command line is no better
(anyone on the same machine can see it). Right at the moment we do not
have any secure place for postmaster parameters.
Smaller problems:
* It still contains too much code copied-and-pasted from bgwriter,
such as
ShutdownXLOG(0, 0);
DumpFreeSpaceMap(0, 0);
autovac has *no* business doing that. I don't have time to go through
it line-by-line to see what else shouldn't have been copied.
* The patch reverts some recent changes in postmaster.c (write_stderr
changes for instance).
* Emitting this warning on every single iteration of the postmaster idle
loop is excessive:
elog(WARNING, "pg_autovacuum: autovac is enabled, but requires stats_row_level which is not enabled");
and this one even more so:
if (!autovacuum_start_daemon)
elog(DEBUG1, "pg_autovacuum: not enabled");
* Any message that's actually likely to be seen by users should be an
ereport, not elog. elog is okay for debugging messages and can't-happen
errors, but not for messages that will be seen by ordinary users,
because there's no support for message translation in elog.
* I don't think you've actually thought very carefully about elog-based
error handling; for instance this bit:
dbs->conn = db_connect(dbs);
if (dbs->conn == NULL)
{ /* Serious problem: We can't connect to
* template1 */
elog(ERROR, "pg_autovacuum: Cannot connect to template1, exiting.");
exit(1);
}
Control won't make it to the exit(1) (which is a darn good thing in
itself, seeing that you are connected to shared memory and hence had
better be using proc_exit()). What *will* happen in the code as given
is that control will bounce back to the sigsetjmp, which will recover
and re-call AutoVacLoop, which would be okay except you just forgot
any open backend connections you have (plus leaked all the memory in
your datastructures). Maybe it would be better if you did not have
an error catcher but just aborted the process on any serious error,
letting the postmaster start a new one. (This ties into your FIXME
note about how the postmaster should react to autovac crashes...)
* autovacuum.h exports way too much stuff that is not relevant to
other modules. (Rule #1: you don't declare static functions in a
header file; these *will* provoke warnings.)
I'm not sure what we do now. I can't apply this in its current state,
and I do not have time to fix it. I don't really want to push it in
and assume we can fix the problems during beta ...
regards, tom lane
Tom Lane wrote: > I'm not sure what we do now. I can't apply this in its current state, > and I do not have time to fix it. I don't really want to push it in > and assume we can fix the problems during beta ... I see. :-( I know Matthew just got back from being away so perhaps he has time to address some of these. I say we see if he can and move on the other open items and see where this is when they are complete. As far as libpq, can't pg_autovacuum dynamically load libpq like dblink does? On the password issue, can't we use .pgpass in the postgres home directory? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> As far as libpq, can't pg_autovacuum dynamically load libpq like dblink
> does?
Hmm, make the bulk of the autovac daemon be a shlib that is dynamically
linked by just that subprocess? Yeah, that might work.
> On the password issue, can't we use .pgpass in the postgres home
> directory?
I thought of that after sending my initial email. It's a fairly ugly
solution, because it confuses logins/passwords that would be appropriate
for interactive use with what you'd want the daemon to use. But it
might be sufficient as a stopgap. Or possibly better, we could hack the
autovac code to read the user and password from some protected file
under $PGDATA.
Both of these issues are things that would probably go away in the long
run, since I doubt that we want to keep the fire-up-an-ordinary-backend
model forever. The thing that's troubling me at the moment is that we
might need to spend a fair amount of work on turning what's only an
intermediate code state into something that's usable and doesn't break
any other stuff. It might be better to hold off and spend that same
work on the longer-range goal of direct integration.
regards, tom lane
Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > As far as libpq, can't pg_autovacuum dynamically load libpq like dblink > > does? > > Hmm, make the bulk of the autovac daemon be a shlib that is dynamically > linked by just that subprocess? Yeah, that might work. We certainly don't want to put any of these issues into a normal backend. > > > On the password issue, can't we use .pgpass in the postgres home > > directory? > > I thought of that after sending my initial email. It's a fairly ugly > solution, because it confuses logins/passwords that would be appropriate > for interactive use with what you'd want the daemon to use. But it > might be sufficient as a stopgap. Or possibly better, we could hack the > autovac code to read the user and password from some protected file > under $PGDATA. > > Both of these issues are things that would probably go away in the long > run, since I doubt that we want to keep the fire-up-an-ordinary-backend > model forever. The thing that's troubling me at the moment is that we > might need to spend a fair amount of work on turning what's only an > intermediate code state into something that's usable and doesn't break > any other stuff. It might be better to hold off and spend that same > work on the longer-range goal of direct integration. Well, it is going to be a local connect so hopefully some can use local/ident and not worry about the passswords. The big question is how much flexibility do we have on getting this into 7.5 while still being fair to Matthew? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073
On Mon, 2004-08-02 at 21:36, Bruce Momjian wrote: > Tom Lane wrote: > > I'm not sure what we do now. I can't apply this in its current state, > > and I do not have time to fix it. I don't really want to push it in > > and assume we can fix the problems during beta ... > > I see. :-( > > I know Matthew just got back from being away so perhaps he has time to > address some of these. I say we see if he can and move on the other > open items and see where this is when they are complete. Yes I'm here, and I'm quite willing to put some more work into this. I really want to see autovac in 7.5/8.0. I have been pushing for a while to get some feedback on my patch since I knew it would be considered a little "rough around the edges" to put it mildly. I will do what I can to improve the patch as long as people think there is a decent chance of getting it applied. Matthew
On Mon, 2004-08-02 at 21:53, Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > As far as libpq, can't pg_autovacuum dynamically load libpq like dblink > > does? > > Hmm, make the bulk of the autovac daemon be a shlib that is dynamically > linked by just that subprocess? Yeah, that might work. Ok, but I will need someone else to help with this. > > On the password issue, can't we use .pgpass in the postgres home > > directory? > > I thought of that after sending my initial email. It's a fairly ugly > solution, because it confuses logins/passwords that would be appropriate > for interactive use with what you'd want the daemon to use. But it > might be sufficient as a stopgap. Or possibly better, we could hack the > autovac code to read the user and password from some protected file > under $PGDATA. Ok, I can do this. > Both of these issues are things that would probably go away in the long > run, since I doubt that we want to keep the fire-up-an-ordinary-backend > model forever. The thing that's troubling me at the moment is that we > might need to spend a fair amount of work on turning what's only an > intermediate code state into something that's usable and doesn't break > any other stuff. It might be better to hold off and spend that same > work on the longer-range goal of direct integration. I understand the concern. I'm just pushing to get autovac into 7.5, I'm sure there are lots of people on this list who could have done a better job, but nobody working on it... Anyway, I'm very willing to do as much of the lifting as I can. Matthew
On Mon, 2004-08-02 at 19:26, Tom Lane wrote:
> I looked over this patch (sorry for the delay), and found a number of
> problems.
Thanks for the feedback, hopefully we can still get something in place
for 7.5.
> Bigger problems:
>
> * I don't think you've thought through system shutdown at all. The
> postmaster code as given will not try to shutdown the autovac daemon
> until the same time it shuts down the bgwriter, which is no good for
> a couple of reasons:
[ snip: lots of good reasons autovac should shutdown before bgwriter ]
I agree. The thought had crossed my mind that autovac should shut down
first, but I'm really not sure how to make that happen. I was only able
todo the backend integration at all by cribbing off the bgwriter example
(as it was was suggested I do). So....
I'll take a stab at this, but some direction would be appreciated.
> * I hadn't quite focused before on the fact that this patch requires
> statically binding frontend libpq into the backend.
[ snip ]
Based on the other part of this thread, I think this is solved by
dynamic linking? But as I said in the other email, I will need help
making this happen.
> * AFAICS there is no provision for setting pg_user or pg_user_password,
> which means that the daemon won't actually be able to connect in
> non-TRUST environments. I don't know what we do about this: putting
> such a password in a GUC variable is right out (because any user could
> read it) and putting it on the postmaster command line is no better
> (anyone on the same machine can see it). Right at the moment we do not
> have any secure place for postmaster parameters.
Right, I didn't want to use GUC vars for this. As I said in the other
email, I will take a whack at using a new special .autovacpasswd file in
the $PGDATA dir.
BTW, is this really something that needs to get solved? It could just
be considered a limitation of the 7.5 implementation that it requires
local trust or ident authentication.
> Smaller problems:
>
> * It still contains too much code copied-and-pasted from bgwriter,
> such as
> ShutdownXLOG(0, 0);
> DumpFreeSpaceMap(0, 0);
> autovac has *no* business doing that. I don't have time to go through
> it line-by-line to see what else shouldn't have been copied.
Yeah, as I said above, I cribbed heavily off the bgwriter example. I
left in most of the things that I was unsure about thinking that it
would be more obvious to remove the code then to readd it. I'll remove
the above code and try to see if there is anything else that should go
away.
> * The patch reverts some recent changes in postmaster.c (write_stderr
> changes for instance).
I'm sure this is just due to the fact that the patch was submitted prior
to that work going in. I'll update the patch.
> * Emitting this warning on every single iteration of the postmaster idle
> loop is excessive:
> elog(WARNING, "pg_autovacuum: autovac is enabled, but requires stats_row_level which is not enabled");
> and this one even more so:
> if (!autovacuum_start_daemon)
> elog(DEBUG1, "pg_autovacuum: not enabled");
I left it this way on purpose as it didn't want the admin to miss it,
and I thought they might if it only emits the warning once on postmaster
startup. I figure that the admin see this while setting up their server
initially and never see it again. I'm more then happy to change this
if you still think I should.
> * Any message that's actually likely to be seen by users should be an
> ereport, not elog. elog is okay for debugging messages and can't-happen
> errors, but not for messages that will be seen by ordinary users,
> because there's no support for message translation in elog.
I looked into changing my elog calls to ereport, but I thought it wasn't
necessary since ordinary users won't see these messages, they will only
be in the postmaster log file.
> * I don't think you've actually thought very carefully about elog-based
> error handling; for instance this bit:
> dbs->conn = db_connect(dbs);
> if (dbs->conn == NULL)
> { /* Serious problem: We can't connect to
> * template1 */
> elog(ERROR, "pg_autovacuum: Cannot connect to template1, exiting.");
> exit(1);
> }
> Control won't make it to the exit(1) (which is a darn good thing in
> itself, seeing that you are connected to shared memory and hence had
> better be using proc_exit()). What *will* happen in the code as given
> is that control will bounce back to the sigsetjmp, which will recover
> and re-call AutoVacLoop, which would be okay except you just forgot
> any open backend connections you have (plus leaked all the memory in
> your datastructures). Maybe it would be better if you did not have
> an error catcher but just aborted the process on any serious error,
> letting the postmaster start a new one. (This ties into your FIXME
> note about how the postmaster should react to autovac crashes...)
Ok, some of this cruft is related to its roots as a stand-alone app. I
will fix this too. So the correct answer here is to skip the
elog/ereport and call proc_exit(), or should I just downgrade the elog
to NOTICE and then call proc_exit()? Lemme know and I'll fix.
> * autovacuum.h exports way too much stuff that is not relevant to
> other modules. (Rule #1: you don't declare static functions in a
> header file; these *will* provoke warnings.)
Yeah, someone made these changes to pg_autovacuum.h while it was still
in contrib. I just assumed that who ever made the changes knew what
they were doing :-) I'll change them back.
> I'm not sure what we do now. I can't apply this in its current state,
> and I do not have time to fix it. I don't really want to push it in
> and assume we can fix the problems during beta ...
I don't know either as it's not my decision to make :-) As I said
though, I'm willing to make all the fixes I mentioned above if you tell
me there is a decent change it will get applied 7.5.
BTW, despite the simplicity of autovac, lots of people are using the
contrib version and it seems to be helping. So while it certainly has
room for improvement, I think even in it's current state, it is
considered and important and useful tool by lots of users.
Thanks again for the code review, let me know what I should do, and I'll
get on it ASAP.
Matthew
"Matthew T. O'Connor" <matthew@zeut.net> writes:
> I agree. The thought had crossed my mind that autovac should shut down
> first, but I'm really not sure how to make that happen.
You have to issue the kill() when the postmaster first receives the
shutdown signal, rather than waiting till after all backends exit.
Also, there needs to be some provision for sending a SIGINT to whichever
backend is actually running an autovac-commanded vacuum. One way to do
this is for autovac, when it gets the SIGUSR2, to turn around and do
PQqueryCancel (sp?) against the active connection. I'm not sure offhand
how to shoehorn that into your code.
> Right, I didn't want to use GUC vars for this. As I said in the other
> email, I will take a whack at using a new special .autovacpasswd file in
> the $PGDATA dir.
I'd go with autovac.parms or some such. $PGDATA isn't anyone's home dir
(normally) and there's no need to tuck config files out of sight.
> BTW, is this really something that needs to get solved? It could just
> be considered a limitation of the 7.5 implementation that it requires
> local trust or ident authentication.
On platforms with no local ident support, that would equate to making
autovac unusable. I think it's a must-fix.
> I looked into changing my elog calls to ereport, but I thought it wasn't
> necessary since ordinary users won't see these messages, they will only
> be in the postmaster log file.
DBAs are ordinary users in this context. I think elog is okay for stuff
that only a developer would ever see, but DBAs want translated messages.
> Ok, some of this cruft is related to its roots as a stand-alone app. I
> will fix this too. So the correct answer here is to skip the
> elog/ereport and call proc_exit(), or should I just downgrade the elog
> to NOTICE and then call proc_exit()? Lemme know and I'll fix.
Actually what I was thinking of was removing the sigsetjmp block (which
would need updated anyway if you keep it). That would cause elog(ERROR)
to go to proc_exit itself. You cannot avoid the issue by not using
elog(ERROR), because you are (or will be) calling subroutines that might
use it.
regards, tom lane
Tom Lane wrote:
> "Matthew T. O'Connor" <matthew@zeut.net> writes:
>>I agree. The thought had crossed my mind that autovac should shut down
>>first, but I'm really not sure how to make that happen.
>
> You have to issue the kill() when the postmaster first receives the
> shutdown signal, rather than waiting till after all backends exit.
> Also, there needs to be some provision for sending a SIGINT to whichever
> backend is actually running an autovac-commanded vacuum. One way to do
> this is for autovac, when it gets the SIGUSR2, to turn around and do
> PQqueryCancel (sp?) against the active connection. I'm not sure offhand
> how to shoehorn that into your code.
Ok, I'll see what I can do about this.
>>Right, I didn't want to use GUC vars for this. As I said in the other
>>email, I will take a whack at using a new special .autovacpasswd file in
>>the $PGDATA dir.
>
> I'd go with autovac.parms or some such. $PGDATA isn't anyone's home dir
> (normally) and there's no need to tuck config files out of sight.
Ok. I assume I can crib from the code that uses the .pgpass file.
> On platforms with no local ident support, that would equate to making
> autovac unusable. I think it's a must-fix.
Ok.
>>I looked into changing my elog calls to ereport, but ....
>
> DBAs are ordinary users in this context. I think elog is okay for stuff
> that only a developer would ever see, but DBAs want translated messages.
Oh, I didn't understand that ereport provided translatable messages
whereas elog does not. That makes sense. I'll change the elog calls.
>>Ok, some of this cruft is related to its roots as a stand-alone app. I
>>will fix this too. So the correct answer here is to skip the
>>elog/ereport and call proc_exit(), or should I just downgrade the elog
>>to NOTICE and then call proc_exit()? Lemme know and I'll fix.
>
> Actually what I was thinking of was removing the sigsetjmp block (which
> would need updated anyway if you keep it). That would cause elog(ERROR)
> to go to proc_exit itself. You cannot avoid the issue by not using
> elog(ERROR), because you are (or will be) calling subroutines that might
> use it.
Ok, If I remove the sigsetjmp block, is there anything else I need to
do, or can I just rip out everything between the {}'s?
Thanks again for the comments. I have a full plate at work today, but
I'm going to try to leave at five and spend the evening fixing as much
of this as possible.
Matthew
"Matthew T. O'Connor" <matthew@zeut.net> writes:
> Ok, If I remove the sigsetjmp block, is there anything else I need to
> do, or can I just rip out everything between the {}'s?
I think you just pull it out. Of course you'll want to test what
happens when an elog occurs ...
regards, tom lane