On 2017/09/25 18:37, Michael Paquier wrote: > On Mon, Sep 25, 2017 at 4:42 PM, Amit Langote > <langote_amit...@lab.ntt.co.jp> wrote: >> On 2017/09/25 12:10, Michael Paquier wrote: >> Hmm, I'm not sure if we need to lock the partitions, too. Locks taken by >> find_all_inheritors() will be gone once the already-running transaction is >> ended by the caller (vacuum()). get_rel_oids() should just lock the table >> mentioned in the command (the parent table), so that find_all_inheritors() >> returns the correct partition list, as Tom perhaps alluded to when he said >> "it would also make the find_all_inheritors call a lot more meaningful.", >> but... > > It is important to hold the lock for child tables until the end of the > transaction actually to fill in correctly the RangeVar associated to > each partition when scanning them.
I think that's right, although, I don't see any new RangeVar created under vacuum() at the moment. Maybe, you're referring to the Nathan's patch that perhaps does that. >> When vacuum() iterates that list and calls vacuum_rel() for each relation >> in the list, it might be the case that a relation in that list is no >> longer a partition of the parent. So, one might say that it would get >> vacuumed unnecessarily. Perhaps that's fine? > > I am personally fine with that. Any checks would prove to complicate > the code for not much additional value. Tend to agree here. >>> I am not >>> saying that this needs to be fixed in REL_10_STABLE at this stage >>> though as this would require some refactoring similar to what the >>> patch adding support for VACUUM with multiple relations does. >> >> I think it would be better to fix that independently somehow. VACUUM on >> partitioned tables is handled by calling vacuum_rel() on individual >> partitions. It would be nice if vacuum_rel() didn't have to refer to the >> RangeVar. Could we not use get_rel_name(relid) in place of >> relation->relname? I haven't seen the other patch yet though, so maybe >> I'm missing something. > > The RangeVar is used for error reporting, and once you reach > vacuum_rel, the relation and its OID may be gone. So you need to save > the information of the RangeVar beforehand when scanning the > relations. That's one reason behind having the VacuumRelation stuff > discussed on the thread for multiple table VACUUM, this way you store > for each item to be vacuumed: > - A RangeVar. > - A list of columns. > - An OID saved by vacuum deduced from the RangeVar. > That's quite some refactoring, so my opinion is that this ship has > sailed already for REL_10_STABLE, and that we had better live with > that in 10. Yeah, your simple patch seems enough for v10. Thanks, Amit -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers