Обсуждение: [HACKERS] Use of RangeVar for partitioned tables in autovacuum
Hi all, I have been looking more closely at the problem in $subject, that I have mentioned a couple of times, like here: https://www.postgresql.org/message-id/CAB7nPqQa37OUne_rJzpmWC4EXQaLX9f27-Ma_-rSFL_3mj+HFQ@mail.gmail.com As of HEAD, the RangeVar defined in calls of vacuum() is used for error reporting, only in two cases: for autoanalyze and autovacuum when reporting to users that a lock cannot be taken on a relation. The thing is that autovacuum has the good idea to call vacuum() on only one relation at the same time, with always a relation OID and a RangeVar defined, so the code currently on HEAD is basically fine with its error reporting, because get_rel_oids() will always work on the relation OID with its RangeVar, and because code paths with manual VACUUMs don't use the RangeVar for any reports. While v10 is safe, I think that this is wrong in the long-term and that may be a cause of bugs if people begin to generate reports for manual VACUUMs, which is plausible in my opinion. The patch attached, is what one solution would look like if we would like to make things more robust as long as manual VACUUM commands can only specify one relation at the same time. I have found that tracking the parent OID by tweaking a bit get_rel_oids() was the most elegant solution. Please note that this range of problems is something that I think is better solved with the infrastructure that support for VACUUM with multiple relations includes (last version here https://www.postgresql.org/message-id/766556DD-AA3C-42F7-ACF4-5DC97F41F151@amazon.com). So I don't think that the patch attached should be integrated, still I am attaching it to satisfy the curiosity of anybody looking at this message. In conclusion, I think that the open item of $subject should be removed from the list, and we should try to get the multi-VACUUM patch in to cover any future problems. I'll do so if there are no objections. One comment on top of vacuum() is wrong by the way: in the case of a manual VACUUM or ANALYZE, a NULL RangeVar is used if no relation is specified in the command. Thanks, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Вложения
On 9/26/17, 9:28 PM, "Michael Paquier" <michael.paquier@gmail.com> wrote: > In conclusion, I think that the open item of $subject should be > removed from the list, and we should try to get the multi-VACUUM patch > in to cover any future problems. I'll do so if there are no > objections. If someone did want to add logging for vacuum_rel() and analyze_rel() in v10 after your patch was applied, wouldn't the NULL RangeVars force us to skip the new log statements for partitions? I think we might instead want to back-patch the VacuumRelation infrastructure so that we can appropriately log for partitions. However, I'm dubious that it is necessary to make such a big change so close to release for hypothetical log statements. So, in the end, I agree with you. Nathan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Sep 28, 2017 at 3:11 AM, Bossart, Nathan <bossartn@amazon.com> wrote: > On 9/26/17, 9:28 PM, "Michael Paquier" <michael.paquier@gmail.com> wrote: >> In conclusion, I think that the open item of $subject should be >> removed from the list, and we should try to get the multi-VACUUM patch >> in to cover any future problems. I'll do so if there are no >> objections. > > If someone did want to add logging for vacuum_rel() and analyze_rel() in > v10 after your patch was applied, wouldn't the NULL RangeVars force us to > skip the new log statements for partitions? I think we might instead > want to back-patch the VacuumRelation infrastructure so that we can > appropriately log for partitions. Yes, in this case that would be a problem. But that problem does not exist yet. If that was to happen, something like the patch I attached would be actually enough. It is simple and non-intrusive as well. > However, I'm dubious that it is necessary to make such a big change so > close to release for hypothetical log statements. So, in the end, I agree > with you. Yeah... As long as there is no problem yet. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Sep 27, 2017 at 11:28 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > In conclusion, I think that the open item of $subject should be > removed from the list, and we should try to get the multi-VACUUM patch > in to cover any future problems. I'll do so if there are no > objections. And done. Moved to the non-bug section. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Thanks Michael for working on this. On 2017/09/27 11:28, Michael Paquier wrote: > Hi all, > > I have been looking more closely at the problem in $subject, that I > have mentioned a couple of times, like here: > https://www.postgresql.org/message-id/CAB7nPqQa37OUne_rJzpmWC4EXQaLX9f27-Ma_-rSFL_3mj+HFQ@mail.gmail.com > > As of HEAD, the RangeVar defined in calls of vacuum() is used for > error reporting, only in two cases: for autoanalyze and autovacuum > when reporting to users that a lock cannot be taken on a relation. The > thing is that autovacuum has the good idea to call vacuum() on only > one relation at the same time, with always a relation OID and a > RangeVar defined, so the code currently on HEAD is basically fine with > its error reporting, because get_rel_oids() will always work on the > relation OID with its RangeVar, and because code paths with manual > VACUUMs don't use the RangeVar for any reports. Yeah, autovacuum_do_vac_analyze will never pass partitioned table OIDs to vacuum, for example, because they're not RELKIND_RELATION relations. > While v10 is safe, I think that this is wrong in the long-term and > that may be a cause of bugs if people begin to generate reports for > manual VACUUMs, which is plausible in my opinion. The patch attached, > is what one solution would look like if we would like to make things > more robust as long as manual VACUUM commands can only specify one > relation at the same time. I have found that tracking the parent OID > by tweaking a bit get_rel_oids() was the most elegant solution. The patch basically looks good to me, FWIW. > Please > note that this range of problems is something that I think is better > solved with the infrastructure that support for VACUUM with multiple > relations includes (last version here > https://www.postgresql.org/message-id/766556DD-AA3C-42F7-ACF4-5DC97F41F151@amazon.com). > So I don't think that the patch attached should be integrated, still I > am attaching it to satisfy the curiosity of anybody looking at this > message. Yeah, after looking at the code a bit, it seems that the problem won't really occur for the case we're trying to apply this patch for. > In conclusion, I think that the open item of $subject should be > removed from the list, and we should try to get the multi-VACUUM patch > in to cover any future problems. I'll do so if there are no > objections. +1 to this, taking the previous +1 back [1]. :) > One comment on top of vacuum() is wrong by the way: in the case of a > manual VACUUM or ANALYZE, a NULL RangeVar is used if no relation is > specified in the command. Yep, but it doesn't ever get accessed per our observations. Thanks, Amit [1] https://www.postgresql.org/message-id/8d810dd9-5f64-a5f3-c016-a81f05528fa8%40lab.ntt.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers