Re: Shaky coding for vacuuming partitioned relations

On 2017/09/25 18:37, Michael Paquier wrote:
> On Mon, Sep 25, 2017 at 4:42 PM, Amit Langote
> <Langote_Amit_f8(at)lab(dot)ntt(dot)co(dot)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