Re: [PATCHES] autovauum integration patch: Attempt #4
Matthew T. O'Connor [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] autovauum integration patch: Attempt #4
Matthew T. O'Connor [EMAIL PROTECTED] 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
Re: [PATCHES] autovauum integration patch: Attempt #4
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 [EMAIL PROTECTED] | (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 7: don't forget to increase your free space map settings
Re: [PATCHES] autovauum integration patch: Attempt #4
Bruce Momjian [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] autovauum integration patch: Attempt #4
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] 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 [EMAIL PROTECTED] | (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 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] autovauum integration patch: Attempt #4
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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] autovauum integration patch: Attempt #4
On Mon, 2004-08-02 at 21:53, Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] autovauum integration patch: Attempt #4
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
Re: [PATCHES] autovauum integration patch: Attempt #4
Patch applied. Thanks. --- Matthew T. O'Connor wrote: On Fri, 2004-07-23 at 23:25, Tom Lane wrote: Matthew T. O'Connor [EMAIL PROTECTED] 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 [EMAIL PROTECTED] | (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
Re: [PATCHES] autovauum integration patch: Attempt #4
[ 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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] | (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 [EMAIL PROTECTED] | (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 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] autovauum integration patch: Attempt #4
Bruce Momjian [EMAIL PROTECTED] writes: Patch applied. Thanks. What happened to the review part? regards, tom lane ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] autovauum integration patch: Attempt #4
Bruce Momjian [EMAIL PROTECTED] 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 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] autovauum integration patch: Attempt #4
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] 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 [EMAIL PROTECTED] | (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 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] autovauum integration patch: Attempt #4
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/ ---(end of broadcast)--- TIP 3: 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] autovauum integration patch: Attempt #4
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? ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] autovauum integration patch: Attempt #4
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 [EMAIL PROTECTED] | (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
Re: [PATCHES] autovauum integration patch: Attempt #4
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 [EMAIL PROTECTED] | (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 8: explain analyze is your friend
