On 15.07.2020 18:03, Alvaro Herrera wrote:
> On 2020-Jul-15, Konstantin Knizhnik wrote:
>
>>
>> On 15.07.2020 02:17, Alvaro Herrera wrote:
>>> On 2020-Jul-10, Konstantin Knizhnik wrote:
>>>
>>>> @@ -3076,6 +3080,10 @@ relation_needs_vacanalyze(Oid relid,
>>>> instuples = tabentry->inserts_since_vacuum;
>>>> anltuples = tabentry->changes_since_analyze;
>>>> + rel = RelationIdGetRelation(relid);
>>>> + oldestXmin = TransactionIdLimitedForOldSnapshots(GetOldestXmin(rel, PROCARRAY_FLAGS_VACUUM), rel);
>>>> + RelationClose(rel);
>>> *cough*
>>>
>> Sorry, Alvaro.
>> Can you explain this *cough*
>> You didn't like that relation is opened just to call GetOldestXmin?
>> But this functions requires Relation. Do you suggest to rewrite it so that
>> it is possible to pass just Oid of relation?
> At that point of autovacuum, you don't have a lock on the relation; the
> only thing you have is a pg_class tuple (and we do it that way on
> purpose as I recall). I think asking relcache for it is dangerous, and
> moreover requesting relcache for it directly goes counter our normal
> coding pattern. At the very least you should have a comment explaining
> why you do it and why it's okay to do it, and also handle the case when
> RelationIdGetRelation returns null.
>
> However, looking at the bigger picture I wonder if it would be better to
> test the getoldestxmin much later in the process to avoid this whole
> issue. Just carry forward the relation until the point where vacuum is
> called ... that may be cleaner? And the extra cost is not that much.
>
Thank you for explanation.
I have prepared new version of the patch which opens relation with care.
Concerning your suggestion to perform this check later (in vacuum_rel()
I guess?)
I tried to implement it but faced with another problem.
Right now information about autovacuum_oldest_xmin for relationis stored
in statistic (PgStat_StatTabEntry)
together with other atovacuum related fields like
autovac_vacuum_timestamp, autovac_analyze_timestamp,...)
I am not sure that it is right place for storing autovacuum_oldest_xmin
but I didn't want to organize in shared memory yet another hash table
just for keeping this field. May be you can suggest something better...
But right now it is stored here when vacuum is completed.
PgStat_StatTabEntry is obtained by get_pgstat_tabentry_relid() which
also needs pgstat_fetch_stat_dbentry(MyDatabaseId) and
pgstat_fetch_stat_dbentry(InvalidOid). I do not want to copy all this
stuff to vacuum.c.
It seems to me to be easier to open relation in
relation_needs_vacanalyze(), isn;t it?