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