ITAGAKI Takahiro wrote:
> ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> wrote:
>
> > Here is a patch that cancels autovacuum workers conflicting with
> > DROP TABLE, TRUNCATE and CLUSTER. It was discussed here:
> > http://archives.postgresql.org/pgsql...6/msg00556.php
>
> I made an adjustment for the latest 'more autovacuum fixes' patch.
> http://archives.postgresql.org/pgsql...6/msg00217.php
>
> Now, autovacuum workers handles ProcDie signals using ERROR
> instead of FATAL. The exception is caught by the worker and
> converted to the following logs.
>
> SIGINT -- Cancel the current job.
> LOG: autovacuum on <db>.<schema>.<table> is canceled
> SIGTERM -- Cancel all jobs.
> LOG: autovacuum on <db> is canceled

Thanks for the patch. I think there are actually three patches in here:

1. changing SIGINT so that it cancels the current table instead of
shutting down the entire worker.

2. changing DROP TABLE and TRUNCATE so that they cancel an autovac
worker by sending SIGINT.

3. change the interrupt code so that autovac workers are terminated with
ERROR instead of FATAL, knowing that the final outcome is the same. If
I'm not mistaken the only point of the change is to be able to change
the error message, is that correct?


I don't feel very much comfortable with the patch (3). I would prefer
that we keep errcode FATAL and select a different error message in
ProcessInterrupts instead. I don't see the point in complicating the
sigjmp target without need.

I agree with the (2) change.

The change in (1) I don't feel comfortable with commenting. It seems
strange to me, and although it seems like it would be safe, I cannot
help having a strange feeling about it. I'll try to digest it a bit
more.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

---------------------------(end of broadcast)---------------------------
TIP 1: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

Alvaro Herrera <alvherre@commandprompt.com> wrote:

> 1. changing SIGINT so that it cancels the current table instead of
> shutting down the entire worker.
>
> 2. changing DROP TABLE and TRUNCATE so that they cancel an autovac
> worker by sending SIGINT.

Quite so.

> 3. change the interrupt code so that autovac workers are terminated with
> ERROR instead of FATAL, knowing that the final outcome is the same. If
> I'm not mistaken the only point of the change is to be able to change
> the error message, is that correct?

Yes, I used ERROR instead of FATAL just for changing the message.
In addition, the actual message level will be LOG, instead of FATAL.

> I don't feel very much comfortable with the patch (3). I would prefer
> that we keep errcode FATAL and select a different error message in
> ProcessInterrupts instead. I don't see the point in complicating the
> sigjmp target without need.

My first patch did so and I changed it in the second patch because
I feel the FATAL entry in syslog is too obtrusive; Internal termination
of autovacuum workers in this way is ignorable for normal users.
However, I'm sure that my complaint is not so important. Please don't
apply thie part of the patch if you don't like it.


> I agree with the (2) change.
>
> The change in (1) I don't feel comfortable with commenting. It seems
> strange to me, and although it seems like it would be safe, I cannot
> help having a strange feeling about it. I'll try to digest it a bit
> more.

Thanks. I forgot to adjust comments in the code, sorry.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center



---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend

ITAGAKI Takahiro wrote:
>
> Alvaro Herrera <alvherre@commandprompt.com> wrote:
>
> > 1. changing SIGINT so that it cancels the current table instead of
> > shutting down the entire worker.

I applied this part of the patch, thanks.

In passing I noticed that apparently we are leaking memory, because the
vacuum memory context is created with a parent of PortalContext, which
ISTM to be NULL in autovacuum. So when we cancel the vacuuming work, we
never delete or reset that context.

I think what we should be doing is creating a context to act as
PortalContext, and reset it after each vacuuming operation.

--
Alvaro Herrera http://www.amazon.com/gp/registry/5ZYLFMCVHXC
"El que vive para el futuro es un iluso, y el que vive para el pasado,
un imbécil" (Luis Adler, "Los tripulantes de la noche")

---------------------------(end of broadcast)---------------------------
TIP 6: explain analyze is your friend