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. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
